diff --git a/docs/adr/1239-gsd-embeddable-orchestration-engine.md b/docs/adr/1239-gsd-embeddable-orchestration-engine.md index 41c6102e9..f146c8cd8 100644 --- a/docs/adr/1239-gsd-embeddable-orchestration-engine.md +++ b/docs/adr/1239-gsd-embeddable-orchestration-engine.md @@ -144,6 +144,21 @@ A closed, first-party vocabulary with a member no host can claim is an invitatio **Status:** delivered in #2481 — axis vocabulary, descriptor values, validator parity, fail-closed negotiation, the degradation row, and the consuming review-lane wiring all land together. `effortSurface` is a wired axis, not a declared-but-unconsumed one; it is the first negotiated axis whose consumer is an invocation-time argument rather than an install-time artifact. +## Amendment (2026-08-09): the Codex install-time ADR now exists — [ADR-2313](2313-codex-passive-model-posture.md) + +Recorded as a dated section rather than by editing the `effortSurface` amendment above, since ADRs here are append-only. + +That amendment's **"Boundary against #2313"** paragraph describes the Codex passive-model posture as "a separate ADR, **not yet written**". It is now written: **[ADR-2313](2313-codex-passive-model-posture.md)**, opened as Phase 0 of epic [#2313](https://github.com/open-gsd/gsd-core/issues/2313) under sub-issue [#3240](https://github.com/open-gsd/gsd-core/issues/3240). The boundary itself is unchanged and is restated from the other side in ADR-2313's own scope section: **ADR-2313 owns the static / install-time channel** (what GSD writes into `~/.codex/agents/.toml`, how it validates what is already there, how it repairs it); **this ADR's `effortSurface` amendment owns the invocation-time channel** and does not change install-time emission. + +Two things ADR-2313 settles that this ADR asserted but the tree contradicted: + +- **`modelMode: passive` for Codex is now honored by the installer.** This ADR classifies Codex as `passive` — "instruction-injection only; no tier routing" (interface point 3), "passive (session-only)" (appendix). `bin/install.js` nonetheless embedded a per-tier Codex model via the #2517 runtime resolver, which 400s on a ChatGPT-account Codex that does not expose the pinned model (#2310 / #2311). ADR-2313 removes that embedding on the default path, so the descriptor and the emitted artifact agree. +- **`model_reasoning_effort` in the generated `.toml` is coupled to a pinned model** (#838), and therefore disappears along with the default pin. An install-time effort value with no accompanying model is partial routing — model following the Codex UI, effort following GSD — and ADR-2313 rules it out. This does **not** touch the `argv` invocation-time effort surface this ADR's amendment governs; a Codex agent still receives `-c model_reasoning_effort=` at invocation time exactly as before. + +**Correction to the Codex-binding section.** That section cites `golden-install-parity/codex.json` as the gate holding Codex install/uninstall to byte parity. That fixture family and `tests/golden-install-parity.test.cjs` were **deleted** by [ADR-2719](2719-emitted-artifact-attribution.md) Phase 4 ([#2724](https://github.com/open-gsd/gsd-core/issues/2724)) and are no longer in the tree; the live gate is the differential attribution check (`tests/emitted-attribution.test.cjs`) plus the committed `tests/fixtures/install-tree/*.json` family ADR-2719 §7 retains. Recorded here rather than by editing that section. Anyone reasoning about what an emitted-`.toml` change trips should read ADR-2719, not the retired fixture name. + +**Numbering note.** ADR-2313 is prefixed with the **epic** number, not #2310. #2310 is the closed bug issue whose emission *guard* shipped separately in PR #2312; `CONTRIBUTING.md` § "Proposing an ADR or PRD" makes the approved issue's number the filename prefix. Earlier text on #2313 referring to "ADR-2310" predates that ruling. + ## Host-capability profiles (negotiation baselines) - **Programmatic-CLI** (Claude Code, pi, OpenCode): imperative; full dispatch; host hook bus; MCP; `slash` surface. The richest target — minimal degradation. diff --git a/docs/adr/2313-codex-passive-model-posture.md b/docs/adr/2313-codex-passive-model-posture.md new file mode 100644 index 000000000..bf8f273c9 --- /dev/null +++ b/docs/adr/2313-codex-passive-model-posture.md @@ -0,0 +1,281 @@ +# ADR-2313: Codex Adopts the Passive / Session-Only Model Posture + +- **Status:** Accepted (Phase 0 — ADR only; locks the contract Phases 1–5 execute against. **No production code lands in this PR, and the posture is not real until Phase 1 merges.**) +- **Date:** 2026-08-09 +- **Issue:** [#2313](https://github.com/open-gsd/gsd-core/issues/2313) — epic (`enhancement` + `approved-enhancement`). This Phase-0 sub-issue: [#3240](https://github.com/open-gsd/gsd-core/issues/3240) +- **Supersedes:** [#2517](https://github.com/open-gsd/gsd-core/issues/2517)'s Codex per-tier `model` embedding **on the default path only**. Explicit `model_overrides` pins are unaffected; other runtimes are untouched. +- **Builds on:** [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) (**EoS**), which classifies Codex `modelMode: passive`. This ADR is the install-time half its `:137` boundary note names as "not yet written". +- **Relationship to prior work:** completes [#2310](https://github.com/open-gsd/gsd-core/issues/2310) / PR [#2312](https://github.com/open-gsd/gsd-core/pull/2312), which shipped the emission *guard*. Related model defaults: [#2122](https://github.com/open-gsd/gsd-core/issues/2122) (GPT-5.6 family), [#838](https://github.com/open-gsd/gsd-core/issues/838) (model ⇄ effort coupling), [#774](https://github.com/open-gsd/gsd-core/issues/774) (light-tier `service_tier`/`model_verbosity`). + +## Context + +ADR-1239 classifies every supported host along eight negotiated axes. For Codex it records +`modelMode: passive` — and it is unusually explicit about what that means. Cited by section rather +than line number, because ADR-1239 is append-only and its line numbers move: + +> **Interface point 3, Model:** `passive`: instruction-injection only; **no tier routing** +> — *§ Per-interface-point capability + degradation ladder* + +> Codex … `max_depth=1` · **passive (session-only)** +> — *§ Appendix — per-host capability matrix* + +> `embeddingMode: declarative` · `commandSurface: slash-file` · **`modelMode: passive`** · … +> — *§ Codex binding (worked host-plugin)* + +The installer does not behave that way. `bin/install.js` `generateCodexAgentToml` resolves a +per-agent `model` for the generated `~/.codex/agents/.toml` in two steps: an explicit +`model_overrides` pin (#2256), and failing that **the runtime-aware tier resolver** added by +#2517, which embeds a per-tier Codex model (`opus→gpt-5.6-sol`, `sonnet→gpt-5.6-terra`, +`haiku→gpt-5.6-luna`, from the #2122 defaults). + +That second step treats Codex as a host that supports per-agent model routing. It does not. + +### The failure this produces + +On a **ChatGPT-account** Codex only the session model is exposed. A `.toml` pinning a model the +account does not carry fails the request outright: + +``` +400 invalid_request_error: "The 'sonnet' model is not supported when using Codex with a ChatGPT account." +``` + +That is [#2310](https://github.com/open-gsd/gsd-core/issues/2310) / #2311 (closed duplicate). The +blast is not confined to one agent: a typed agent spawn that 400s degrades the whole plan/execute +flow to the non-equivalent generic-agent workaround, so the user loses the routing GSD was trying +to give them *and* the agent specialization, in exchange for a pin that never worked. + +PR #2312 fixed the *alias* half — never write an Anthropic-flavored value (`opus`/`sonnet`/ +`haiku`/`fable`, or a `claude-*` id in any provider namespacing). It did not fix the general case: +a **real** Codex model id the account does not expose 400s exactly the same way, and the +runtime-resolver path still pins one by default. + +### Two adjacent gaps the same posture closes + +- **The install-check validates presence, not correctness.** `checkAgentsInstalled` confirms the + manifest is complete and the declared agents exist on disk. An install carrying `model = "sonnet"` + from before PR #2312 reports healthy until the spawn 400s. +- **There is no Codex `.toml` sync path.** `cmdEffortSync` (`src/commands.cts`) re-syncs `effort:` + frontmatter for Claude `.md` agents and returns early for every other runtime. A stale Codex + install is only fixable by a full reinstall. + +## Decision + +**Codex adopts the passive / session-only posture ADR-1239 already assigns it.** Concretely: + +### D1 — Omit the per-agent `model` by default + +`generateCodexAgentToml` emits **no** `model` line unless a model is explicitly pinned. The agent +inherits the Codex session model, which the account is guaranteed to expose. This cannot 400. + +### D2 — Embed a `model` only for an explicit real-Codex pin + +A `model_overrides` entry naming a real Codex model id (`gpt-5.6-sol`, …) is embedded verbatim. +This is the supported, and now the *only*, way to pin a Codex model. + +### D3 — Never emit an Anthropic-flavored model + +The #2310 guard stands: a bare tier alias (`opus`/`sonnet`/`haiku`/`fable`) or any `claude-*` id +in any provider namespacing is dropped with a deduped stderr warning. D1 makes the +runtime-resolver route to this gate unreachable by construction; **the gate is retained anyway**, +because the `model_overrides` route to it remains live. + +### D4 — `model_reasoning_effort` stays coupled to a pinned model + +No pin ⇒ no effort line (#838). A `.toml` with no `model` but a static `model_reasoning_effort` +is partial routing: the model follows the Codex UI while the effort follows GSD. The knobs move +together or not at all. + +### D5 — Supersede #2517 on the default path + +The runtime-resolver per-tier `model` embedding for Codex is removed. #2517's explicit-runtime +resolution is otherwise preserved, and every other runtime is untouched. + +### D6 — Validate posture, not just presence + +The install-check gains a Codex posture check: an installed `.toml` embedding an Anthropic-flavored +`model`, or carrying an orphaned `model_reasoning_effort`, is a reported violation. + +### D7 — Repair stale installs without a reinstall + +The effort/model sync gains a Codex `.toml` path that strips a stale Anthropic/tier `model` and an +orphaned effort, leaving legal pins intact. + +### What the posture does **not** cover + +`service_tier` and `model_verbosity` for light-tier agents (#774) are cost and verbosity knobs, not +routing. They are emitted independently of `model` and are unaffected by D1–D4. + +## The reader/writer boundary + +D6 and D7 both read a file D1–D4 write, and the file is user-editable — Codex reads it too. The +postures differ, deliberately, and conflating them is the trap: + +**Writing is conservative.** Emit the minimal legal document. Never emit a value known to be +rejected. + +**The health-check is liberal in parsing, strict in judging.** It tolerates comments, key ordering, +CRLF, and **extra keys GSD does not emit** — a user who hand-added `approval_policy` has not +violated the posture. The check is a predicate on the two fields the posture owns (`model`, +`model_reasoning_effort`), never a whitelist over the document. When it does find a violation it +names the agent and the offending value rather than reporting a bare count. + +**The sync is liberal but visible, and never guesses.** It rewrites the user's file, so +"be liberal in what you accept" is precisely the instinct that produces silent data loss here: + +- dry-run remains the default, and every strip is reported as a structured `{from, to}` change; +- a legal pin and its coupled effort survive untouched — reported `skipped`, not `synced`; +- **an unparseable document is skipped and reported, never partially rewritten.** A duplicate + `[table]` or trailing garbage is a refusal, not a best-effort edit; +- the literal text `model = ` occurring **inside** the `'''`-quoted `developer_instructions` block + is not a pin and must not be rewritten. The emitter writes agent prompts into that block and GSD's + agent prompts discuss models constantly, so a line-oriented `/^model\s*=/m` strip corrupts the + agent. This is the most likely way the sync ships a data-loss bug, and it is called out here so + it is a design constraint rather than a review finding. + +## Migration + +**This is a breaking change, and the recourse is explicit.** + +Codex agents installed with a `runtime` set in config and a non-`inherit` `model_profile` stop +receiving per-tier GPT-5.6 pins by default — they omit, and inherit the session model. + +**Who this actually reaches, stated precisely, because "non-`inherit`" understates it.** +`readGsdRuntimeProfileResolver` (`bin/install.js`) returns `null` — no resolver, therefore no +embedded model even today — in exactly two cases: no `runtime` in project or home config, or +`model_profile === 'inherit'`. Everyone else gets a resolver, and **`model_profile` defaults to +`'balanced'`**. So the affected population is *every* Codex user who set a `runtime` and did not +explicitly opt into `inherit` — the default configuration, not an exotic one. Conversely, a user +already on `inherit` sees **no change at all**; their `.toml` has never carried a pin. + +The users who lose something real are on an **API-key** Codex account whose account *does* expose +`gpt-5.6-sol`/`terra`/`luna`. For them the fix is one line per agent: + +```json +{ "model_overrides": { "gsd-planner": "gpt-5.6-sol" } } +``` + +`model_overrides` with a real Codex model id is the retained pin mechanism (D2). It is unaffected +by this ADR and is the supported path forward. + +**No deprecation window is offered.** The default flips in a single release rather than warning +first. That is a genuine departure from the usual "deprecate slowly and loudly" discipline, taken +because the current default hard-400s for the majority (ChatGPT) account type — the behavior being +removed is one most affected users could never successfully use. The cost is stated rather than +elided: an API-key user on a non-`inherit` profile will see their tier routing disappear in a minor +release and must consult this section to restore it. Phase 1's changeset leads with the migration +line for that reason. + +## Consequences + +**Positive.** Codex agents launch reliably, because the session model is always available. GSD's +Codex behavior matches ADR-1239's own classification of it instead of contradicting it. Stale +installs become detectable (D6) and repairable (D7) rather than requiring a reinstall. The +`.toml` GSD emits gets smaller and has fewer ways to be wrong. + +**Negative.** Per-tier routing on Codex is gone by default, including for the API-key users who +could use it — recovered only by an explicit pin. GSD now owns a posture *validator* and a +*repairer* as permanent surface, both of which must track any future change to the emitted `.toml` +shape. Three surfaces (emitter, checker, syncer) now read one rule, which is a +generative-fix-divergence risk; it is mitigated by extracting the predicate into a single module in +Phase 1 with a parity assertion test, not by discipline. + +**Expected breakage on landing.** Removing a line from an emitted artifact moves that artifact's +hash, so Phase 1 must expect the emitted-artifact gates to fire — correctly. The live gate is the +**differential attribution check** (`tests/emitted-attribution.test.cjs`, +[ADR-2719](2719-emitted-artifact-attribution.md)), which requires every moved hash to be +attributable to the diff, plus the committed `tests/fixtures/install-tree/*.json` family that +ADR-2719 §7 deliberately keeps and `npm run gen:install-tree` regenerates. + +*Not* `golden-install-parity/codex.json`: that fixture family and +`tests/golden-install-parity.test.cjs` were **deleted** by ADR-2719 Phase 4 (#2724) and no longer +exist in the tree. ADR-1239's Codex-binding section still names the retired fixture; it is recorded +here so a Phase-1 implementer does not go looking for a gate that was removed a release ago. + +`tests/codex-config.test.cjs` and `tests/issue-2517-runtime-aware-profiles.test.cjs` assert the +embedding today and flip to assert omission in the same phase. + +## Alternatives considered + +1. **Keep #2517's per-tier embedding and force `runtime="codex"` resolution at install time.** + Rejected: it still 400s on a ChatGPT-account Codex that lacks the pinned model — the resolution + path was never the defect — and it entrenches the contradiction with ADR-1239's `passive` + classification. +2. **Persist `runtime:"codex"` into the shared `~/.gsd/defaults.json`.** Rejected: that is exactly + the cross-runtime poisoning open bug [#2297](https://github.com/open-gsd/gsd-core/issues/2297) + flags. Recorded here as a standing constraint: no phase of this epic writes shared defaults. +3. **Hybrid — pin when the model is available, omit when it is not.** *Deferred, not rejected on + merit.* It needs a model-availability signal Codex does not clearly expose. If Codex later + exposes one, this is the design to revisit, and D1 becomes its fallback rung rather than its + replacement. +4. **An allowlist of legal Codex model ids** instead of the Anthropic-flavored predicate. Rejected: + an allowlist goes stale the moment OpenAI ships a model, which reintroduces "GSD pins a model + the account cannot use" one layer up — the same defect with a different cause. +5. **Fold this into ADR-1239 as an amendment.** Rejected: ADR-1239:137 scopes itself to the + *invocation-time* effort channel and names this as a separate ADR owning the *install-time* + channel. Folding it in erases a boundary that ADR was deliberate about. + +## Scope boundary + +**In scope:** the static / install-time channel — what GSD writes into +`~/.codex/agents/.toml`, how it validates what is already written, and how it repairs it. + +**Out of scope:** + +- **Invocation-time orchestrator effort-override drift.** ADR-1239:137 draws this line from the + other side; this ADR restates it. The two channels may share a descriptor once `EFFORT_RENDERING` + folds in, but not here. +- **Re-architecting `agent_runtime` derivation.** Phase 5 corrects `init`'s *report* of the + detected host (folded from [#2320](https://github.com/open-gsd/gsd-core/issues/2320)); the + broader runtime-identity model and its intersection with #2297 is not redesigned here. +- **Every non-Codex runtime.** Claude, OpenCode, Kilo, Hermes and the rest keep their current model + handling unchanged. + +## Phases + +Each phase is one sub-issue and one PR. `/adr-phase-coverage` reports every decision above owned by +exactly one phase, and every user-facing capability wired by an owning phase. + +| Phase | Sub-issue | Owns | Deliverable | +|---|---|---|---| +| 0 | [#3240](https://github.com/open-gsd/gsd-core/issues/3240) | this ADR | ADR + index regen + the ADR-1239 cross-ref | +| 1 | [#3241](https://github.com/open-gsd/gsd-core/issues/3241) | D1–D5 | emission rework in `generateCodexAgentToml`; extract the posture predicate into one module with a parity test | +| 2 | [#3242](https://github.com/open-gsd/gsd-core/issues/3242) | D6 | posture health-check, as a **new exported function** — `checkAgentsInstalled` carries 33 dependents and cyclomatic 25 and does not get more branches | +| 3 | [#3243](https://github.com/open-gsd/gsd-core/issues/3243) | D7 | Codex `.toml` sync path | +| 4 | [#3244](https://github.com/open-gsd/gsd-core/issues/3244) | end-to-end proof | smoke test: researcher/planner/checker under **both** `model_profile: balanced` (the default, and the path that actually changes) **and** `inherit` — see the scoping correction below | +| 5 | [#3245](https://github.com/open-gsd/gsd-core/issues/3245) | #2320 fold | `init` reports the detected host; explicit `config.runtime` still overrides; no `defaults.json` write | + +**Phase 5 exists because the coverage gate found it missing.** The #2320 fold was promised in a +maintainer comment on #2313 but claimed by none of the phases in the epic body, which lists 0–4 — +the promised-but-not-built shape that gate exists to catch. It is owned rather than dropped. + +**Phase 4's scope is corrected here, and the correction is the point.** The epic body scopes the +smoke test to `model_profile: inherit`. That profile is precisely the one this ADR does **not** +change: `readGsdRuntimeProfileResolver` already returns `null` for `inherit`, so a Codex install +under `inherit` omits the model today, before Phase 1. A smoke test scoped only to `inherit` would +therefore pass identically before and after the change it exists to prove — green, and vacuous. + +Phase 4 must cover `model_profile: balanced` (the default, and the path that actually loses its +pin) as the primary case, keeping `inherit` as the unchanged control. Asserting both is what makes +the test a regression test rather than a tautology, and it is the difference between proving the +posture and proving that `inherit` still behaves the way it always did. + +## Known limits + +What this ADR deliberately does **not** fix, gathered in one place so a later reader does not have +to assemble it from Consequences and Scope boundary: + +- **API-key Codex users lose per-tier routing, with no automatic migration.** The recourse is an + explicit `model_overrides` pin (see Migration). Accepted as the cost of the default flip; not + mitigated further. +- **No deprecation window.** The default flips in a single minor release. A genuine departure from + deprecate-slowly-and-loudly, taken because the behavior being removed hard-400s for the majority + account type — but it is a departure, and no warning release is offered. +- **No model-availability detection.** GSD does not learn which models a Codex account exposes; it + avoids the question by not pinning. Alternative 3 is the design to revisit if Codex ever exposes + such a signal. +- **Invocation-time effort-override drift is untouched.** ADR-1239's boundary. +- **`agent_runtime` derivation is not re-architected.** Phase 5 corrects `init`'s *report* only. +- **Every non-Codex runtime is untouched**, including hosts that also lack real tier routing. This + ADR does not generalize the posture; extending it to another host would be its own decision. +- **The posture is not real until Phase 1 merges.** `Accepted` locks the contract, not the tree — + until #3241 lands, `generateCodexAgentToml` still embeds a per-tier model. diff --git a/docs/adr/README.md b/docs/adr/README.md index a3affd6c3..6be900ce2 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -120,7 +120,7 @@ This replaces a hand-maintained table that had drifted to **40 of 65 ADRs** — -### Active decisions (57) +### Active decisions These govern the system as it stands. Cite these. @@ -174,6 +174,7 @@ These govern the system as it stands. Cite these. | [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) | Markdown Table Model, Bounded Mutation, and Fail-Loud Consolidation (#1372 part 2) | Accepted | — | | [ADR-2164](2164-statusline-scope-boundary.md) | Statusline draws its data boundary at local, read-only sources | Accepted | — | | [ADR-2207](2207-status-field-lifecycle-ownership.md) | STATE.md `Status` lifecycle — phase-completion writes an intermediate state; milestone-close owns termination | Accepted | — | +| [ADR-2313](2313-codex-passive-model-posture.md) | Codex Adopts the Passive / Session-Only Model Posture | Accepted | — | | [ADR-2346](2346-command-dispatch-completion.md) | Command Dispatch Completion | Accepted | — | | [ADR-2619](2619-observability-shareable-diagnostics.md) | Observability and shareable diagnostics — wire the dispatch seam, add the outbound trust boundary | Accepted | — | | [ADR-2629](2629-phase-effort-estimation-calibration.md) | Phase effort is estimated against a calibrated smart-zone budget, not a static heuristic | Accepted | — | @@ -184,7 +185,7 @@ These govern the system as it stands. Cite these. | [ADR-3212](3212-lexical-seam-consolidation.md) | The Lexical Seam — Safe Pattern Construction, Line-Terminator Normalization, and Tokenizer-First Stateful Grammars | Accepted | — | | [ADR-3660](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Accepted | [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) | -### Proposed (10) +### Proposed Decided in principle, not yet ratified. Do not cite as settled architecture. @@ -201,7 +202,7 @@ Decided in principle, not yet ratified. Do not cite as settled architecture. | [ADR-2363](2363-capability-instruction-surface-trust.md) | A capability's skill body is an instruction surface — trusted, unscanned, and disclosed | Proposed | — | | [ADR-3128](3128-adaptive-runtime-evidence.md) | Adaptive runtime evidence for GSD Debug | Proposed | — | -### Superseded, Retired, and Legacy (8) +### Superseded, Retired, and Legacy Historical record. **Do not follow these** — each names what replaced it, or why it was retired. @@ -216,7 +217,7 @@ Historical record. **Do not follow these** — each names what replaced it, or w | [ADR-2264](2264-golden-parity-redesign.md) | Redesign golden-install-parity — single-source manifest builder + split invariant | Superseded | [ADR-2719](2719-emitted-artifact-attribution.md) | | [ADR-3524](3524-cjs-sdk-hard-seam.md) | CJS↔SDK hard seam — one source of truth per Shared Module | Superseded | [ADR-0174](0174-retire-gsd-sdk-package-boundary.md) | -_75 ADRs. Generated by `scripts/gen-adr-index.cjs` — run `--write` after adding or restatusing an ADR._ +_Generated by `scripts/gen-adr-index.cjs` — run `--write` after adding or restatusing an ADR._ diff --git a/scripts/gen-adr-index.cjs b/scripts/gen-adr-index.cjs index 37b394da4..aaec6c116 100644 --- a/scripts/gen-adr-index.cjs +++ b/scripts/gen-adr-index.cjs @@ -442,7 +442,17 @@ function renderIndex(corpus) { const rows = corpus.adrs.filter(g.match).sort((x, y) => Number(x.fileId) - Number(y.fileId)); if (rows.length === 0) continue; - out.push(`### ${g.heading} (${rows.length})`, '', g.blurb, ''); + // No row count in the heading: it is a numeric cell inside the generated + // region, shared by every ADR-adding PR. Two PRs that add different ADRs + // touch different table rows and merge cleanly — but both rewrite this + // same count line, so whichever lands second gets a stale local --check + // pass and a red CI --check against the merged tree (#3251). Same failure + // mode CHANGELOG.md and drift-acks already solved by moving to per-PR + // fragment files (.changeset/, tests/emitted-drift-acks/); here the fix is + // simpler still — the count carries no verification value (--check + // regenerates and diffs the whole region regardless) and is trivially + // derivable by counting the table rows. Do not add it back. + out.push(`### ${g.heading}`, '', g.blurb, ''); const isHistorical = g.heading.startsWith('Superseded'); // "Read first" points at the broader ADR that now frames this one. It is how a // reader of a still-Accepted component decision (e.g. the runtime descriptor) @@ -464,8 +474,12 @@ function renderIndex(corpus) { out.push(''); } + // No total ADR count either, for the same reason as the per-group heading + // count above: it is a second shared mutable cell in the generated region + // that every ADR-adding PR would rewrite, guaranteeing the identical merge + // race (#3251). Leave it out; the count is derivable by reading the table. out.push( - `_${corpus.adrs.length} ADRs. Generated by \`scripts/gen-adr-index.cjs\` — run \`--write\` after adding or restatusing an ADR._`, + `_Generated by \`scripts/gen-adr-index.cjs\` — run \`--write\` after adding or restatusing an ADR._`, '', END_MARKER, ); diff --git a/tests/adr-index-gate.test.cjs b/tests/adr-index-gate.test.cjs index 6d48b529b..760120f58 100644 --- a/tests/adr-index-gate.test.cjs +++ b/tests/adr-index-gate.test.cjs @@ -86,8 +86,8 @@ test('a clean corpus generates an index and --check passes', (t) => { const readme = fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'); assert.match(readme, /\[ADR-0001\]\(0001-alpha\.md\)/, 'zero-padded id must render as written, not ADR-1'); assert.match(readme, /\[ADR-900\]\(900-beta\.md\)/); - assert.match(readme, /Active decisions \(1\)/); - assert.match(readme, /Proposed \(1\)/); + assert.match(readme, /### Active decisions\b/); + assert.match(readme, /### Proposed\b/); }); test('--check fails when an ADR is added but the index is not regenerated', (t) => { @@ -126,7 +126,7 @@ test('the table header form is parsed as legitimately as the bullet form', (t) = }); const res = run(root, ['--write']); assert.equal(res.status, 0, `table-form header must parse: ${res.stderr}`); - assert.match(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'), /Active decisions \(1\)/); + assert.match(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'), /### Active decisions\b/); }); test('Superseded must name its successor as a file link, not a bare id', (t) => { @@ -178,7 +178,7 @@ test('subsumption is symmetry-checked but does NOT mark the target superseded', }); assert.equal(run(root, ['--write']).status, 0); const readme = fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'); - assert.match(readme, /Active decisions \(2\)/, 'a subsumed ADR stays Active'); + assert.match(readme, /### Active decisions\b/, 'a subsumed ADR stays Active'); // The subsumer is surfaced in the "Read first" column so EoS is discoverable // from the component ADR. assert.match(readme, /\| \[ADR-0001\]\(0001-alpha\.md\) \|[^|]*\| Accepted \| \[ADR-900\]\(900-eos\.md\) \|/); @@ -353,7 +353,7 @@ test('--write still emits the index while reporting outstanding violations', (t) const res = run(root, ['--write']); assert.equal(res.status, 0, '--write proceeds'); assert.match(res.stderr, /lifecycle violation\(s\) remain/); - assert.match(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'), /Active decisions \(2\)/); + assert.match(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'), /### Active decisions\b/); }); test('an ADR title cannot hijack the README splice with an index marker', (t) => { @@ -403,7 +403,7 @@ test('a backslash-pipe in a title cannot break out of its table cell', (t) => { // Any unescaped pipe from the title would add a 6th boundary and shift the cells. const unescaped = [...row.matchAll(/(? { @@ -413,7 +413,7 @@ test('a pipe in a title cannot break out of its table cell', (t) => { assert.equal(run(root, ['--write']).status, 0); const readme = fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'); assert.match(readme, /Alpha \\\| Accepted \\\| fake/, 'pipes must be escaped'); - assert.match(readme, /Proposed \(1\)/, 'the forged cell must not land the ADR in Active'); + assert.match(readme, /### Proposed\b/, 'the forged cell must not land the ADR in Active'); }); test('a file that does not match the naming convention is reported, not crashed on', (t) => { @@ -437,6 +437,209 @@ test('the real repo corpus is clean and its index is current', () => { assert.equal(res.status, 0, `docs/adr/ must satisfy its own gate:\n${res.stderr}`); }); +// --- insert-only invariant (#3251) ------------------------------------------ +// +// The generated region is a shared, committed artifact rewritten by every +// ADR-adding PR. Two PRs that add different ADRs touch different table rows +// and merge cleanly — but if adding an ADR ever MODIFIES an existing line +// (not just appends new ones), two such PRs collide on that line and one +// lands with a locally-green, CI-red `--check` (exactly what happened when +// PR #3251's ADR-2313 and #3249's concurrently-landed ADR-3247 both rewrote +// the same count line). The property that makes concurrent PRs merge is +// stronger than "no count string is present": it is that render(N) is a +// strict line-subsequence of render(N+1) for every N. These tests lock that +// property directly, rather than the one symptom (a specific count format) +// that happened to trigger #3251. + +/** Slice the generated region (inclusive of both markers) out of a README, as lines. */ +function indexRegionLines(readme) { + const start = readme.indexOf(START); + const end = readme.indexOf(END); + assert.ok(start !== -1 && end !== -1, 'README must carry both index markers'); + return readme.slice(start, end + END.length).split(/\r?\n/); +} + +/** + * Assert `before` is a strict line-subsequence of `after`: every line of + * `before`, in order, is also found in `after` in order. That is precisely + * "adding an ADR only INSERTS lines; it never MODIFIES an existing one" — + * the property that lets two ADR-adding PRs merge without colliding. + * + * On failure, names the first `before` line that could not be matched (and + * its index) rather than a bare boolean — `assert.ok(isSubsequence)` gives a + * debugging dead end when a corpus of dozens of lines fails. + */ +function assertInsertOnly(before, after, message) { + let j = 0; + for (let i = 0; i < before.length; i++) { + while (j < after.length && after[j] !== before[i]) j++; + if (j >= after.length) { + assert.fail( + `${message}: "before" line ${i} was not found, in order, in "after" — ` + + `it was modified rather than merely followed by an insertion.\n` + + ` missing line (before[${i}]): ${JSON.stringify(before[i])}`, + ); + } + j++; // consume the match so later lines cannot re-match the same slot + } +} + +/** Render the region for a corpus, then write one more file and re-render. */ +function renderBeforeAfter(t, baseFiles, addName, addBody) { + const root = makeRepo(t, baseFiles); + assert.equal(run(root, ['--write']).status, 0); + const before = indexRegionLines(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8')); + + fs.writeFileSync(path.join(root, 'docs', 'adr', addName), addBody); + assert.equal(run(root, ['--write']).status, 0); + const after = indexRegionLines(fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8')); + + return { before, after }; +} + +test('adding an ADR only inserts lines (append position)', (t) => { + // Highest id: the new row lands at the bottom of an already-populated + // table. Pre-fix, `### Active decisions (2)` -> `(3)` and the footer + // `_2 ADRs...` -> `_3 ADRs...` both MODIFY an existing line, so this case + // fails the subsequence check against the pre-fix generator. + const { before, after } = renderBeforeAfter( + t, + { + '100-alpha.md': adr('Alpha', ['**Status:** Accepted']), + '200-beta.md': adr('Beta', ['**Status:** Accepted']), + }, + '300-gamma.md', + adr('Gamma', ['**Status:** Accepted']), + ); + assertInsertOnly(before, after, 'appending the highest-id ADR'); +}); + +test('adding a lowest-id or middle-id ADR is still insert-only', (t) => { + // Table-driven per the matrix's boundary cases #2 (lowest — the riskiest + // insertion point, at the very top of the table) and #3 (middle). Same + // pre-fix failure mode as the append case: the group-heading count and the + // footer count both change on every insertion, regardless of where the row + // lands. + const cases = [ + { label: 'lowest id (top of the table)', id: '050' }, + { label: 'middle id (between existing rows)', id: '150' }, + ]; + for (const { label, id } of cases) { + const { before, after } = renderBeforeAfter( + t, + { + '100-alpha.md': adr('Alpha', ['**Status:** Accepted']), + '300-gamma.md': adr('Gamma', ['**Status:** Accepted']), + }, + `${id}-beta.md`, + adr('Beta', ['**Status:** Accepted']), + ); + assertInsertOnly(before, after, `adding ADR-${id} (${label})`); + } +}); + +test('the first ADR in an empty corpus inserts a whole group block', (t) => { + // limit-1 -> limit: before has NO group blocks at all (every group's row + // count is 0, so `renderIndex` emits only the markers and the footer). + // Pre-fix, the footer itself carries the only count (`_0 ADRs...` -> + // `_1 ADRs...`), which is a MODIFIED line, not an insertion — so this case + // fails against the pre-fix generator even though no group heading exists + // yet to change. + const { before, after } = renderBeforeAfter(t, {}, '100-alpha.md', adr('Alpha', ['**Status:** Accepted'])); + assertInsertOnly(before, after, 'first ADR in an empty corpus'); +}); + +test('adding the first ADR of a new status group leaves other groups untouched', (t) => { + const { before, after } = renderBeforeAfter( + t, + { '100-alpha.md': adr('Alpha', ['**Status:** Accepted']) }, + '200-beta.md', + adr('Beta', ['**Status:** Proposed']), + ); + assertInsertOnly(before, after, 'adding the first Proposed ADR alongside an Accepted-only corpus'); + + // Stronger than insert-only: the whole Active decisions block (heading + // through its own trailing blank line) must be byte-identical, since the + // new Proposed section is inserted strictly after it, never inside it. + // Pre-fix, `### Active decisions (1)` would itself be a line INSIDE this + // block that survives unchanged here (the Proposed group is what's new, + // not Active's row count) — the real pre-fix failure for this case is the + // footer's total count, which sits after both blocks. + const footerIdx = before.findIndex((l) => l.startsWith('_Generated by')); + assert.notEqual(footerIdx, -1, 'before render must carry the footer line'); + assert.deepEqual( + after.slice(0, footerIdx), + before.slice(0, footerIdx), + 'the Active decisions block must be byte-identical after adding a Proposed ADR', + ); +}); + +test('superseding an ADR may edit its row, but never a count line', (t) => { + // The one case in the matrix flagged as a possible exception to strict + // insert-only: a new Superseded ADR naming an existing Accepted ADR as its + // successor. Empirically (tracing parseAdr/renderIndex) it is NOT actually + // an exception here: every row's cells are derived solely from that ADR's + // OWN header text, so adding a file that talks ABOUT Alpha cannot alter + // Alpha's already-computed row — only Alpha's own header, which this test + // never edits, could do that. Assert what the matrix requires at minimum + // (no count-bearing line, in either render) and, since it costs nothing + // and happens to hold, the full insert-only property too — this is a + // strictly stronger, still-true claim, not a weakened one. + // + // Pre-fix failing line: `_1 ADRs. Generated by ...` -> `_2 ADRs. ...`. The + // new ADR joins the Superseded group, so Active's own heading count stays + // `(1)`; it is the footer total that is MODIFIED and breaks the + // subsequence walk. Non-vacuous. + const { before, after } = renderBeforeAfter( + t, + { '100-alpha.md': adr('Alpha', ['**Status:** Accepted']) }, + '200-beta.md', + adr('Beta', ['**Status:** Superseded by [ADR-100](100-alpha.md)']), + ); + const countBearing = /^### .+\(\d+\)\s*$/; + const footerCount = /^_\d+ ADRs\./; + for (const region of [before, after]) { + for (const line of region) { + assert.ok(!countBearing.test(line), `no group heading may carry a count: ${JSON.stringify(line)}`); + assert.ok(!footerCount.test(line), `no footer line may carry a count: ${JSON.stringify(line)}`); + } + } + assertInsertOnly(before, after, 'adding a Superseded ADR that names the Accepted ADR as successor'); +}); + +test('insert-only holds for titles carrying markdown/HTML hazards', (t) => { + // Pipe, angle brackets, and backslash are all reachable through an H1 + // title and are exactly what `cellText` exists to neutralize (#: pipe + // would split the table cell, angle brackets could forge an HTML/marker + // sequence, backslash is markdown's escape char and must be escaped + // first). A literal newline is deliberately NOT included here: the title + // is always extracted from a single physical H1 line + // (`lines.find(l => /^#\s/.test(l))`), so a raw `\n` cannot survive into + // the title text at all — asserting `cellText`'s newline handling would be + // vacuous at this call site (there is no path from a corpus file to a + // multi-line title). + // + // Pre-fix failing lines: BOTH `### Active decisions (1)` -> `(2)` and + // `_1 ADRs. ...` -> `_2 ADRs. ...`. The hazardous title affects only the + // NEW row's cell text, so what actually breaks the subsequence walk + // pre-fix is the same pair of count lines as the plain cases — the title + // hazard rides along to prove escaping does not itself introduce a + // modified line. Non-vacuous. + const { before, after } = renderBeforeAfter( + t, + { '100-alpha.md': adr('Alpha', ['**Status:** Accepted']) }, + '200-beta.md', + adr('Beta \\| \\', ['**Status:** Accepted']), + ); + assertInsertOnly(before, after, 'adding an ADR whose title carries |, <, >, and \\'); + + const row = after.find((l) => l.includes('200-beta.md')); + assert.ok(row, 'the hostile-title ADR must still have a row'); + const cells = [...row.matchAll(/(?'), 'angle brackets must be escaped, not emitted raw'); +}); + // --- regressions ----------------------------------------------------------- // // The --check gate above passes on a corpus that still carries dangling