Skip to content

Commit c1ce470

Browse files
committed
ci: check:adr-anchors — governed code must keep naming its decision (#3723 follow-up)
An accepted ADR is only binding if the code it binds points back at it. #3723 is the proof. Three accepted ADRs said the same thing — ADR-0057 D4 ("never as the authority for RBAC"), ADR-0090 D3's word ban ("distribution = position"), ADR-0095 D3 ("no enforcement-time code path may consult the better-auth role") — and a patch-level changeset reversed all three by making app-declared names storable in `sys_member.role`. A follow-up then made the derivation automatic in every host. The reversal held for a day; the tracking issue was closed, reopened and rewritten three times while the cause moved. The mechanism was not carelessness. `auth-manager.ts` cited ADR-0105 D8 (why `delegated_admin` is registered) and said nothing about why the app-role loop beside it was a violation. Reading that file, an author — human or agent — could not have known. In a repo worked by many agents in parallel, nobody greps the ADR corpus before an edit; the constraint has to be where the edit is. So: a curated map of file → governing ADR ids + the invariant, and a check that the ids still appear in the file. Deliberately dumb — it is a presence check, not a proof of obedience (no static check can be that). Its value is the failure text, which states the INVARIANT rather than "put this string back", and that it fires on exactly the diff worth a second look: someone rewriting a governed block and dropping the rationale with it. Seeded with 8 anchors, all in the incident's blast radius: the membership-role cluster (spec, auth-manager, auth-plugin, both platform objects), the authz resolver, the delegated-admin gate, the invitation role cap. Deliberately not more — a map of everything is a map of nothing, and each entry must earn its failure mode. Also AGENTS.md Prime Directive #13: reversing a recorded decision is itself a decision and needs a superseding ADR, not a changeset that quietly does the opposite; and when you implement an ADR, leave its id in the code. Verified by temporarily stripping `ADR-0108` from auth-manager.ts (fails, with the invariant), plus the missing-file, withdrawn-ADR (0107) and malformed-id paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186LhwkUBupmLJUUAMda5hU
1 parent dab35e0 commit c1ce470

6 files changed

Lines changed: 209 additions & 0 deletions

File tree

.changeset/adr-anchors-guard.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
---
3+
4+
Tooling-only: `pnpm check:adr-anchors` — code an accepted ADR governs must keep naming it (#3723 follow-up). Adds `scripts/check-adr-anchors.mjs` + `scripts/adr-anchors.json` (8 seeded anchors, all in the blast radius of the incident), a `Lint & Type Check` step, and Prime Directive #13 in `AGENTS.md` ("an accepted ADR binds until a superseding ADR says otherwise"). Releases nothing — no package changes.
5+
6+
The incident this closes: three accepted ADRs said `sys_member.role` must never carry RBAC authority, and a patch-level changeset made app-declared names storable there anyway; a follow-up then made it automatic in every host. The mechanism was not carelessness — the file being edited never named the ADRs that governed it, so the author could not have known. The check is a presence check (does the governed file still reference its ADR ids?), deliberately dumb; the value is that the failure carries the **invariant**, not just an id to paste back, and it fires on exactly the diff that warrants a second look — someone rewriting a governed block and dropping the rationale with it.

.github/workflows/lint.yml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,14 @@ jobs:
101101
- name: Reserved-word ("role") docs ratchet
102102
run: pnpm check:role-word
103103

104+
# #3723 ADR anchors: code an accepted ADR governs must keep naming it.
105+
# That incident reversed three accepted ADRs with a patch-level changeset,
106+
# and the mechanism was simply that the edited file never mentioned them —
107+
# so the author could not have known. Presence check only; the invariant
108+
# travels in the failure message (scripts/adr-anchors.json).
109+
- name: ADR anchors (governed code names its decision)
110+
run: pnpm check:adr-anchors
111+
104112
# #3280/#3290 org-identifier guard: `organizationId` is the blessed
105113
# developer-facing name for the caller's active org in hook/action bodies;
106114
# the `session.tenantId` alias was REMOVED in v11 (#3290). Keeps our own

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ Other scripts: `objectui:bump` (pull only), `objectui:build`, `objectui:clean`.
9494
10. **File issues for out-of-scope findings — don't silently expand scope or leave them buried.** When you hit a bug, gap, or unenforced capability that's unrelated to the current task, or too large to fix in scope, open a GitHub issue (`gh issue create`) with a clear repro/decision and link it from your PR. Corollary: **never advertise or demo a capability the runtime doesn't actually deliver** (declared ≠ enforced) — fix it, trim it, or file an issue, but don't fake coverage. Example: the spec once declared 9 validation-rule types while the write-path validator enforced only 3 (`state_machine`/`script`/`cross_field`); the gap was filed as #1475 rather than demoed in the showcase, then closed by **trimming** what could never be enforced (`unique`/`async`/`custom`) and **implementing** the rest — the spec now declares 6 and `rule-validator.ts` handles all 6. Note how narrow that claim stayed even so: the evaluator was wired into insert and single-id update only, so a bulk `updateMany` silently skipped every rule — a second `declared ≠ enforced` gap one layer down, at the **call site** rather than the `switch`; filed as #3106 and closed by evaluating the bulk match set per row. A `case` label is not enforcement; check the **call site**.
9595
11. **Worktree-first — never edit on the shared `main` checkout.** This repo is edited by **multiple agents at once**; the shared `main` tree has its HEAD switched and reset *under you*, silently clobbering uncommitted work. Before your **first file edit**, you MUST be in a dedicated worktree on a feature branch: `git worktree add ../objectstack-<task> -b <branch> main && cd ../objectstack-<task> && pnpm install`. A PreToolUse hook (`.claude/hooks/guard-main-checkout.sh`) **enforces** this — it blocks `Edit`/`Write`/`NotebookEdit` unless the edited file is in a dedicated **worktree** — a feature branch on the *shared* checkout is **not** enough (it still gets switched under you) — and it checks the **edited file's own repo**, so sibling repos (`objectui`/`cloud`) you touch are covered too (override for a deliberate non-task fix with `OS_ALLOW_MAIN_EDITS=1`). Full playbook below.
9696
12. **Contract-first — fix the metadata, not the runtime.** This is a metadata-driven framework: `packages/spec` is the one contract between metadata *producers* and the runtime/renderers that *consume* it. When a piece of metadata "doesn't work," ask **first**: *is it spec-compliant? is this the long-term-correct direction?* If the metadata is wrong, fix it at the **producer** and **reject it at authoring/publish** (validation / lint) so the error surfaces loudly — do **not** add a lenient alias or `??` fallback in the consumer (a node executor, the REST layer, a renderer) to tolerate off-spec input. A tolerant fallback fossilizes the wrong convention into a second de-facto contract, dilutes the spec, and hides the producer's bug — one strict contract beats N dialects. This is an **internal** contract (we own both ends), so "be liberal in what you accept" (Postel) does **not** apply — that's for untrusted boundaries. Change the **spec** only when the spec itself is genuinely wrong, and then deliberately (edit the Zod schema + migrate), never by accreting consumer-side fallbacks. The `cfg.filter ?? cfg.filters` / `cfg.objectName ?? cfg.object` fallbacks the flow executors once carried are **debt to pay down, not a pattern to copy** — and the way they are being paid down is the pattern to copy. `filters` → `filter` has **graduated** into the ADR-0087 D2 conversion layer (`flow-node-crud-filter-alias`): rewritten to the canonical key at load, including the `AutomationEngine.registerFlow` rehydration seam, so the CRUD executors read `cfg.filter` directly and no consumer-side fallback survives. `object` → `objectName` and the six open-coded stragglers #3796 tracked (notify `to`/`subject`/`body`/`url`, script `functionName`/`input`) graduated the same way at protocol 17 (`flow-node-crud-object-alias`, `flow-node-notify-config-aliases`, `flow-node-script-config-aliases`), emptying the `readAliasedConfig` executor shim — deleted with them. When you must tolerate an alias at all, declare it as a conversion-layer entry (never a bare `??`, and no new executor shims) so it is declared, loud, tested, and *removable on a schedule*. Stored `sys_metadata` rows (data at rest) are covered from the other side: every rehydration seam replays the **full** conversion chain — retired entries included — via `applyConversionsToStoredItem` (#3903, ADR-0087 addendum), so a consumer never needs its own accommodation for a legacy stored shape either. *Worked example:* an AI-authored `create_record` used `fieldValues` / `today()` / `{{trigger.record.id}}` while the executor reads `fields` / `{TODAY()}` / `{record.id}` → the fix was correcting the authoring skill + a publish-gate lint that rejects the wrong shape (cloud#688), **not** a `cfg.fields ?? cfg.fieldValues` runtime alias (framework#2419, rejected). Strengthens #5.
97+
13. **An accepted ADR binds until a superseding ADR says otherwise.** Reversing a recorded decision is itself a decision: it needs a **new ADR** (or an amended status line on the old one), not a changeset that quietly does the opposite. Before changing behaviour in `docs/adr/`-governed territory, **grep the ADRs for the surface you are touching** — the decision is often older and broader than the code comment in front of you. *Worked example:* three accepted ADRs said `sys_member.role` must never carry RBAC authority (ADR-0057 D4 "never as the authority for RBAC", ADR-0090 D3's word ban "distribution = `position`", ADR-0095 D3 "no enforcement-time code path may consult the better-auth role"). A patch-level changeset made app-declared names storable there anyway; a follow-up made it automatic in every host; the reversal held for a day and the tracking issue was closed, reopened and rewritten three times while the cause moved (#3723 → ADR-0108). The mechanism was not carelessness — **the file being edited never named the ADRs that governed it**, so the author could not have known. Hence the corollary: when you implement an ADR's decision, **leave its id in the code**, and anchor load-bearing spots in `scripts/adr-anchors.json` (`pnpm check:adr-anchors`) so the next author is told which decision they are standing on. A decision nobody can find is a decision that will be reversed.
9798

9899
---
99100

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
"check:nul-bytes": "node scripts/check-nul-bytes.mjs",
3434
"check:doc-authoring": "node scripts/check-doc-authoring.mjs",
3535
"check:role-word": "node scripts/check-role-word.mjs",
36+
"check:adr-anchors": "node scripts/check-adr-anchors.mjs",
3637
"check:org-identifier": "node scripts/check-org-identifier.mjs",
3738
"check:authz-resolver": "node scripts/check-single-authz-resolver.mjs",
3839
"check:slot-lookup": "node scripts/check-slot-lookup-ratchet.mjs",

scripts/adr-anchors.json

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
{
2+
"//": "ADR anchors — see scripts/check-adr-anchors.mjs. Each entry pins the ADR ids that MUST stay referenced in a file whose behaviour an accepted ADR decided. Add an entry when an ADR's decision is realized in code that would look arbitrary (or wrong) to someone reading the file alone.",
3+
"anchors": [
4+
{
5+
"file": "packages/spec/src/identity/membership-role.ts",
6+
"adrs": ["ADR-0090", "ADR-0108"],
7+
"invariant": "The membership-role vocabulary is CLOSED and framework-owned (owner/admin/delegated_admin/member). Capability travels through positions, never through `sys_member.role`."
8+
},
9+
{
10+
"file": "packages/plugins/plugin-auth/src/auth-manager.ts",
11+
"adrs": ["ADR-0108"],
12+
"invariant": "better-auth's organization roles map registers the closed framework vocabulary ONLY. App-declared `position` / `permission` names are not organization roles — registering one makes it storable in `sys_member.role`, which `resolve-authz-context` projects into `current_user.positions`."
13+
},
14+
{
15+
"file": "packages/plugins/plugin-auth/src/auth-plugin.ts",
16+
"adrs": ["ADR-0108"],
17+
"invariant": "Nothing widens the `sys_member.role` / `sys_invitation.role` selects at boot, and there is no organization-role derivation hook. The objects are registered as authored."
18+
},
19+
{
20+
"file": "packages/platform-objects/src/identity/sys-member.object.ts",
21+
"adrs": ["ADR-0108"],
22+
"invariant": "`role` is the closed four-name select. It is enforced on write, and that is the guardrail making an ungoverned capability grant unrepresentable — not a limitation to work around."
23+
},
24+
{
25+
"file": "packages/platform-objects/src/identity/sys-invitation.object.ts",
26+
"adrs": ["ADR-0105", "ADR-0108"],
27+
"invariant": "`role` mirrors `sys_member.role` (the value that lands there on acceptance). Capability rides in the `positions` placement field, authorized against the issuer's adminScope."
28+
},
29+
{
30+
"file": "packages/core/src/security/resolve-authz-context.ts",
31+
"adrs": ["ADR-0095"],
32+
"invariant": "No enforcement-time code path may consult the better-auth role directly. `mapMembershipRole` is a grant-PROVISIONING concern here; posture and adjudication run off the resulting capability grants."
33+
},
34+
{
35+
"file": "packages/plugins/plugin-security/src/delegated-admin-gate.ts",
36+
"adrs": ["ADR-0090"],
37+
"invariant": "Administration is a scoped capability (D12). Writes to the RBAC link tables and `sys_member` are governed: subtree anchoring, assignable-set allowlist, strict containment."
38+
},
39+
{
40+
"file": "packages/plugins/plugin-auth/src/invitation-role-cap.ts",
41+
"adrs": ["ADR-0105"],
42+
"invariant": "An invitation may add a person, never authority above the issuer's own grade. An issuer below admin grade may invite as plain `member` only."
43+
}
44+
]
45+
}

scripts/check-adr-anchors.mjs

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
#!/usr/bin/env node
2+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
3+
//
4+
// check-adr-anchors — keep an accepted ADR's decision readable from the code it
5+
// governs.
6+
//
7+
// ## The failure this exists for
8+
//
9+
// framework#3723. Three accepted ADRs said the same thing — ADR-0057 D4 ("feed
10+
// the names to better-auth ONLY so invitations are accepted, never as the
11+
// authority for RBAC"), ADR-0090 D3's word ban ("distribution = position"), and
12+
// ADR-0095 D3 ("no enforcement-time code path may consult the better-auth role
13+
// directly"). A patch-level changeset reversed all three by making app-declared
14+
// names storable in `sys_member.role`; a follow-up then made the derivation
15+
// automatic in every host. Nobody noticed for a day, and the issue tracking it
16+
// was closed, reopened and rewritten three times while the cause moved.
17+
//
18+
// The mechanism of that failure is worth naming precisely, because it is not
19+
// "someone was careless": **the file being edited never mentioned the ADRs that
20+
// governed it.** `auth-manager.ts` cited ADR-0105 D8 (why `delegated_admin` is
21+
// registered) and said nothing about why the app-role loop next to it was a
22+
// violation. An author — human or agent — reading that file could not have
23+
// known. ADRs are only binding if the code they bind points back at them.
24+
//
25+
// ## What this checks
26+
//
27+
// For each entry in `scripts/adr-anchors.json`: the file exists, every ADR id
28+
// listed for it names a real record under `docs/adr/`, and every one of those
29+
// ids still appears somewhere in the file. That is all — a presence check,
30+
// deliberately dumb:
31+
//
32+
// - It cannot be satisfied usefully by a drive-by edit that guts the logic,
33+
// because the failure text carries the INVARIANT, not just an id to paste
34+
// back.
35+
// - It costs nothing to keep green while the invariant holds, and it fails
36+
// the moment someone rewrites a governed block and drops the rationale with
37+
// it — which is exactly the diff that needs a second look.
38+
//
39+
// It does NOT verify the code still obeys the ADR; no static check can. It
40+
// guarantees the next author is TOLD which decision they are standing on. The
41+
// enforcement of each invariant lives in its own tests (see the ADR).
42+
//
43+
// ## Adding an entry
44+
//
45+
// Add one when an accepted ADR's decision is realized in code that would look
46+
// arbitrary — or plausibly wrong, or improvable — to someone reading the file
47+
// alone. That is the tell: if a reasonable engineer could "fix" it and be
48+
// reverting a decision, anchor it. Do not anchor everything; a map of
49+
// everything is a map of nothing, and each entry must earn its failure mode.
50+
//
51+
// node scripts/check-adr-anchors.mjs
52+
53+
import { readFileSync, readdirSync, existsSync } from 'node:fs';
54+
import { join } from 'node:path';
55+
56+
const ROOT = process.cwd();
57+
const MAP_PATH = 'scripts/adr-anchors.json';
58+
const ADR_DIR = 'docs/adr';
59+
60+
/** An ADR id as written in code comments: `ADR-0090`. */
61+
const ADR_ID = /^ADR-(\d{4})$/;
62+
63+
let anchors;
64+
try {
65+
({ anchors } = JSON.parse(readFileSync(join(ROOT, MAP_PATH), 'utf8')));
66+
} catch (e) {
67+
console.error(`check-adr-anchors: cannot read ${MAP_PATH}${e.message}`);
68+
process.exit(1);
69+
}
70+
if (!Array.isArray(anchors)) {
71+
console.error(`check-adr-anchors: ${MAP_PATH} must carry an "anchors" array.`);
72+
process.exit(1);
73+
}
74+
75+
/** Decision records that actually exist, by number: `0090` → `0090-permission-model-...md`. */
76+
const records = new Set();
77+
try {
78+
for (const f of readdirSync(join(ROOT, ADR_DIR))) {
79+
const m = /^(\d{4})-/.exec(f);
80+
if (m) records.add(m[1]);
81+
}
82+
} catch {
83+
console.error(`check-adr-anchors: no ${ADR_DIR}/ directory — run from the repo root.`);
84+
process.exit(1);
85+
}
86+
87+
const errors = [];
88+
let checked = 0;
89+
90+
for (const entry of anchors) {
91+
const { file, adrs, invariant } = entry ?? {};
92+
93+
if (typeof file !== 'string' || !Array.isArray(adrs) || adrs.length === 0) {
94+
errors.push(`${MAP_PATH}: every anchor needs a "file" and a non-empty "adrs" array (got ${JSON.stringify(entry)}).`);
95+
continue;
96+
}
97+
if (typeof invariant !== 'string' || invariant.trim() === '') {
98+
// The invariant IS the value of this check — an entry without one degrades
99+
// the failure into "put this string back", which teaches nothing.
100+
errors.push(`${MAP_PATH}: anchor for ${file} has no "invariant" — state what the ADR decided, in a sentence or two.`);
101+
continue;
102+
}
103+
104+
const abs = join(ROOT, file);
105+
if (!existsSync(abs)) {
106+
errors.push(
107+
`${file}: anchored file is missing. If it moved, update ${MAP_PATH}; if the code is gone, say so in ` +
108+
`the ADR — a decision whose implementation vanished is one to revisit, not to drop silently.`,
109+
);
110+
continue;
111+
}
112+
113+
const body = readFileSync(abs, 'utf8');
114+
for (const adr of adrs) {
115+
const m = ADR_ID.exec(adr);
116+
if (!m) {
117+
errors.push(`${MAP_PATH}: "${adr}" is not an ADR id (expected e.g. ADR-0090).`);
118+
continue;
119+
}
120+
// Anchoring a withdrawn or never-written record sends the next author to a
121+
// dead end (cf. ADR-0107, withdrawn before it landed).
122+
if (!records.has(m[1])) {
123+
errors.push(`${MAP_PATH}: ${adr} has no record under ${ADR_DIR}/ — anchor a decision that exists.`);
124+
continue;
125+
}
126+
if (!body.includes(adr)) {
127+
errors.push(
128+
`${file}: no longer references ${adr}.\n` +
129+
` ${invariant}\n` +
130+
` If the code still obeys it, restore the reference where the decision shows up.\n` +
131+
` If you are deliberately changing it, that needs a superseding ADR under ${ADR_DIR}/ — ` +
132+
`not a comment edit — and then an update to ${MAP_PATH}.`,
133+
);
134+
}
135+
}
136+
checked++;
137+
}
138+
139+
if (errors.length) {
140+
console.error(`check-adr-anchors: ${errors.length} problem(s)\n`);
141+
for (const e of errors) console.error(' • ' + e);
142+
console.error(
143+
'\n Why this check exists: an accepted ADR was reversed by a patch-level changeset (#3723) because\n' +
144+
' the code it governed never named it. Anchors keep the decision reachable from the diff.\n',
145+
);
146+
process.exit(1);
147+
}
148+
console.log(`check-adr-anchors: OK (${checked} anchored file(s), every governing ADR still referenced).`);

0 commit comments

Comments
 (0)