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).`);