From c1ce47023e00340159dcea447b5dc2fdc19eea9a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 08:17:10 +0000 Subject: [PATCH] =?UTF-8?q?ci:=20`check:adr-anchors`=20=E2=80=94=20governe?= =?UTF-8?q?d=20code=20must=20keep=20naming=20its=20decision=20(#3723=20fol?= =?UTF-8?q?low-up)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_0186LhwkUBupmLJUUAMda5hU --- .changeset/adr-anchors-guard.md | 6 ++ .github/workflows/lint.yml | 8 ++ AGENTS.md | 1 + package.json | 1 + scripts/adr-anchors.json | 45 ++++++++++ scripts/check-adr-anchors.mjs | 148 ++++++++++++++++++++++++++++++++ 6 files changed, 209 insertions(+) create mode 100644 .changeset/adr-anchors-guard.md create mode 100644 scripts/adr-anchors.json create mode 100644 scripts/check-adr-anchors.mjs diff --git a/.changeset/adr-anchors-guard.md b/.changeset/adr-anchors-guard.md new file mode 100644 index 0000000000..464be4f39d --- /dev/null +++ b/.changeset/adr-anchors-guard.md @@ -0,0 +1,6 @@ +--- +--- + +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. + +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. diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 54fbebf852..ad0134b21c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -101,6 +101,14 @@ jobs: - name: Reserved-word ("role") docs ratchet run: pnpm check:role-word + # #3723 ADR anchors: code an accepted ADR governs must keep naming it. + # That incident reversed three accepted ADRs with a patch-level changeset, + # and the mechanism was simply that the edited file never mentioned them — + # so the author could not have known. Presence check only; the invariant + # travels in the failure message (scripts/adr-anchors.json). + - name: ADR anchors (governed code names its decision) + run: pnpm check:adr-anchors + # #3280/#3290 org-identifier guard: `organizationId` is the blessed # developer-facing name for the caller's active org in hook/action bodies; # the `session.tenantId` alias was REMOVED in v11 (#3290). Keeps our own diff --git a/AGENTS.md b/AGENTS.md index 57307ca3a5..0ae28e6351 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -94,6 +94,7 @@ Other scripts: `objectui:bump` (pull only), `objectui:build`, `objectui:clean`. 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**. 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- -b main && cd ../objectstack- && 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. 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. +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. --- diff --git a/package.json b/package.json index 5d32bf2e79..c80a66d470 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,7 @@ "check:nul-bytes": "node scripts/check-nul-bytes.mjs", "check:doc-authoring": "node scripts/check-doc-authoring.mjs", "check:role-word": "node scripts/check-role-word.mjs", + "check:adr-anchors": "node scripts/check-adr-anchors.mjs", "check:org-identifier": "node scripts/check-org-identifier.mjs", "check:authz-resolver": "node scripts/check-single-authz-resolver.mjs", "check:slot-lookup": "node scripts/check-slot-lookup-ratchet.mjs", diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json new file mode 100644 index 0000000000..6d9b6e1077 --- /dev/null +++ b/scripts/adr-anchors.json @@ -0,0 +1,45 @@ +{ + "//": "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.", + "anchors": [ + { + "file": "packages/spec/src/identity/membership-role.ts", + "adrs": ["ADR-0090", "ADR-0108"], + "invariant": "The membership-role vocabulary is CLOSED and framework-owned (owner/admin/delegated_admin/member). Capability travels through positions, never through `sys_member.role`." + }, + { + "file": "packages/plugins/plugin-auth/src/auth-manager.ts", + "adrs": ["ADR-0108"], + "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`." + }, + { + "file": "packages/plugins/plugin-auth/src/auth-plugin.ts", + "adrs": ["ADR-0108"], + "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." + }, + { + "file": "packages/platform-objects/src/identity/sys-member.object.ts", + "adrs": ["ADR-0108"], + "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." + }, + { + "file": "packages/platform-objects/src/identity/sys-invitation.object.ts", + "adrs": ["ADR-0105", "ADR-0108"], + "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." + }, + { + "file": "packages/core/src/security/resolve-authz-context.ts", + "adrs": ["ADR-0095"], + "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." + }, + { + "file": "packages/plugins/plugin-security/src/delegated-admin-gate.ts", + "adrs": ["ADR-0090"], + "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." + }, + { + "file": "packages/plugins/plugin-auth/src/invitation-role-cap.ts", + "adrs": ["ADR-0105"], + "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." + } + ] +} diff --git a/scripts/check-adr-anchors.mjs b/scripts/check-adr-anchors.mjs new file mode 100644 index 0000000000..ebffb47a03 --- /dev/null +++ b/scripts/check-adr-anchors.mjs @@ -0,0 +1,148 @@ +#!/usr/bin/env node +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// check-adr-anchors — keep an accepted ADR's decision readable from the code it +// governs. +// +// ## The failure this exists for +// +// framework#3723. Three accepted ADRs said the same thing — ADR-0057 D4 ("feed +// the names to better-auth ONLY so invitations are accepted, never as the +// authority for RBAC"), ADR-0090 D3's word ban ("distribution = position"), and +// ADR-0095 D3 ("no enforcement-time code path may consult the better-auth role +// directly"). 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. Nobody noticed for a day, and the issue tracking it +// was closed, reopened and rewritten three times while the cause moved. +// +// The mechanism of that failure is worth naming precisely, because it is not +// "someone was careless": **the file being edited never mentioned the ADRs that +// governed it.** `auth-manager.ts` cited ADR-0105 D8 (why `delegated_admin` is +// registered) and said nothing about why the app-role loop next to it was a +// violation. An author — human or agent — reading that file could not have +// known. ADRs are only binding if the code they bind points back at them. +// +// ## What this checks +// +// For each entry in `scripts/adr-anchors.json`: the file exists, every ADR id +// listed for it names a real record under `docs/adr/`, and every one of those +// ids still appears somewhere in the file. That is all — a presence check, +// deliberately dumb: +// +// - It cannot be satisfied usefully by a drive-by edit that guts the logic, +// because the failure text carries the INVARIANT, not just an id to paste +// back. +// - It costs nothing to keep green while the invariant holds, and it fails +// the moment someone rewrites a governed block and drops the rationale with +// it — which is exactly the diff that needs a second look. +// +// It does NOT verify the code still obeys the ADR; no static check can. It +// guarantees the next author is TOLD which decision they are standing on. The +// enforcement of each invariant lives in its own tests (see the ADR). +// +// ## Adding an entry +// +// Add one when an accepted ADR's decision is realized in code that would look +// arbitrary — or plausibly wrong, or improvable — to someone reading the file +// alone. That is the tell: if a reasonable engineer could "fix" it and be +// reverting a decision, anchor it. Do not anchor everything; a map of +// everything is a map of nothing, and each entry must earn its failure mode. +// +// node scripts/check-adr-anchors.mjs + +import { readFileSync, readdirSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; + +const ROOT = process.cwd(); +const MAP_PATH = 'scripts/adr-anchors.json'; +const ADR_DIR = 'docs/adr'; + +/** An ADR id as written in code comments: `ADR-0090`. */ +const ADR_ID = /^ADR-(\d{4})$/; + +let anchors; +try { + ({ anchors } = JSON.parse(readFileSync(join(ROOT, MAP_PATH), 'utf8'))); +} catch (e) { + console.error(`check-adr-anchors: cannot read ${MAP_PATH} — ${e.message}`); + process.exit(1); +} +if (!Array.isArray(anchors)) { + console.error(`check-adr-anchors: ${MAP_PATH} must carry an "anchors" array.`); + process.exit(1); +} + +/** Decision records that actually exist, by number: `0090` → `0090-permission-model-...md`. */ +const records = new Set(); +try { + for (const f of readdirSync(join(ROOT, ADR_DIR))) { + const m = /^(\d{4})-/.exec(f); + if (m) records.add(m[1]); + } +} catch { + console.error(`check-adr-anchors: no ${ADR_DIR}/ directory — run from the repo root.`); + process.exit(1); +} + +const errors = []; +let checked = 0; + +for (const entry of anchors) { + const { file, adrs, invariant } = entry ?? {}; + + if (typeof file !== 'string' || !Array.isArray(adrs) || adrs.length === 0) { + errors.push(`${MAP_PATH}: every anchor needs a "file" and a non-empty "adrs" array (got ${JSON.stringify(entry)}).`); + continue; + } + if (typeof invariant !== 'string' || invariant.trim() === '') { + // The invariant IS the value of this check — an entry without one degrades + // the failure into "put this string back", which teaches nothing. + errors.push(`${MAP_PATH}: anchor for ${file} has no "invariant" — state what the ADR decided, in a sentence or two.`); + continue; + } + + const abs = join(ROOT, file); + if (!existsSync(abs)) { + errors.push( + `${file}: anchored file is missing. If it moved, update ${MAP_PATH}; if the code is gone, say so in ` + + `the ADR — a decision whose implementation vanished is one to revisit, not to drop silently.`, + ); + continue; + } + + const body = readFileSync(abs, 'utf8'); + for (const adr of adrs) { + const m = ADR_ID.exec(adr); + if (!m) { + errors.push(`${MAP_PATH}: "${adr}" is not an ADR id (expected e.g. ADR-0090).`); + continue; + } + // Anchoring a withdrawn or never-written record sends the next author to a + // dead end (cf. ADR-0107, withdrawn before it landed). + if (!records.has(m[1])) { + errors.push(`${MAP_PATH}: ${adr} has no record under ${ADR_DIR}/ — anchor a decision that exists.`); + continue; + } + if (!body.includes(adr)) { + errors.push( + `${file}: no longer references ${adr}.\n` + + ` ${invariant}\n` + + ` If the code still obeys it, restore the reference where the decision shows up.\n` + + ` If you are deliberately changing it, that needs a superseding ADR under ${ADR_DIR}/ — ` + + `not a comment edit — and then an update to ${MAP_PATH}.`, + ); + } + } + checked++; +} + +if (errors.length) { + console.error(`check-adr-anchors: ${errors.length} problem(s)\n`); + for (const e of errors) console.error(' • ' + e); + console.error( + '\n Why this check exists: an accepted ADR was reversed by a patch-level changeset (#3723) because\n' + + ' the code it governed never named it. Anchors keep the decision reachable from the diff.\n', + ); + process.exit(1); +} +console.log(`check-adr-anchors: OK (${checked} anchored file(s), every governing ADR still referenced).`);