From 01dbda9c499bc0e8b15ab54b970c500194b7067d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 20 Sep 2026 23:47:33 -0400 Subject: [PATCH] =?UTF-8?q?docs(#4910):=20ADR-4910=20=E2=80=94=20the=20Pla?= =?UTF-8?q?nningDoc=20parse=20=E2=86=92=20mutate=20=E2=86=92=20serialize?= =?UTF-8?q?=20seam=20=E2=80=94=20Phase=200=20of=20#4906=20(#4911)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(#4910): ADR-4910 — the PlanningDoc parse → mutate → serialize seam — Phase 0 of #4906 Design lock for epic #4906. Docs-only; no production code lands here. Eight decisions: one PlanningDoc seam composing markdown-sectionizer, markdown-table and frontmatter as layers; node-replacement writes so a field write cannot reach past its own value; byte-stable serialization for untouched regions; escape-or-refuse shared between each artifact's writer and its reader, with an explicit accepted-superset-of-emittable split; a typed parse error scoped to the node rather than the document; a type-narrowed write boundary paired with a lint; a positive control per accepted grammar; and one implementation per shared pattern. Two corrections to the epic's stated mechanism, both load-bearing for later phases: - The epic asks for a ratchet where a reintroduced content.replace() "does not typecheck". It cannot: fs.writeFileSync(p, s.replace(...)) typechecks fine because fs has never heard of PlanningDoc. Enforcement is type-narrowing plus a lint that owns the bypass, and the ADR says so rather than shipping a guarantee one require() defeats. - The epic names scripts/lint-planning-artifact-writer-drift.cjs as the drain point. That script is a registry-completeness guard that states "No ratchet / no baseline" by design and never inspects how a write is performed. It is correct on its own axis and left untouched; the right home is local/no-adhoc-markdown-parsing. ADR-2143's Phase 4 already shipped the table-regex and replace-mutation detectors, so the ADR scopes the remaining gap precisely rather than asking for that work twice: adhocReplaceMutation keys on a table-or-section regex, and #4852's pattern is a bold-label field regex — which is why the defect sits in src/phase.cts, a file the rule does lint, with lint:ci green. Per docs/adr/README.md lifecycle rule 3, ADR-1372 and ADR-2143 are NOT given the reciprocal `Subsumed by` back-link here: a Proposed ADR's Subsumes claim is prospective, so its targets are not marked until ratification. Both back-links land in the Phase 6 ratification PR. ADR index regenerated. Refs #4906 Closes #4910 Co-Authored-By: Claude Opus 5 * docs(#4910): apply review findings — link every ADR cross-reference and state the node-scoping interpretation Three review passes ran against the ADR: an isolated adversarial fact-check of every citation, and both axes of /code-review as separate sub-agents. Standards axis (hard violation of docs/adr/README.md lifecycle rule 2): 14 bare ADR-1372 / ADR-2143 / ADR-1411 references in body prose. gen-adr-index.cjs does not catch this — its bare-id check runs only over relation-field values, never body prose — so a green lint:generated-sync did not clear it. Every bare cross-reference is now a file link; only the self-reference ADR-4910 remains bare, which is not a cross-reference. Spec axis: §5 scopes the parse error to the node, while #4906's criterion reads "an unparseable shape surfaces could-not-parse with the offending span" with no document-or-node qualifier. Both readings are faithful to that sentence and they produce materially different Phase 1 and Phase 4 work. §5 now records the distributive reading explicitly, names the document-scoped alternative, states what it would cost (one bad table failing phase list and init.progress alongside roadmap.analyze), and names what changes if the epic meant the other one. A silent narrowing became a stated one. Refs #4906 Closes #4910 Co-Authored-By: Claude Opus 5 * docs(#4910): make each phase's acceptance a structural property, not a list of fixed issues Phases 2-5 read as "fixes #4852, #4862, #4499" — a point-fix list with a seam attached. That is the failure the epic names in its own words: "an implementation that does that has not closed this epic, even with every symptom gone and CI green." New section "What makes a phase done" locks three criteria every phase carries: - Census -> zero. A phase enumerates every instance of its anti-pattern in the tree, publishes the count in its PR, and closes when it is zero. Not "the reported ones". - Deletion, not coexistence. Bespoke implementations are removed, not kept in sync beside the seam — including src/roadmap.cts:1196's three-arm planCountPattern, which is correct today and still goes, because a correct copy of a rule the seam owns is the two-copies-that-agree case. - Unrepresentable by construction. Each phase ships one property that makes its class impossible rather than currently absent: a property over generated documents, a type that does not admit the wrong shape, or a drift guard. The absorbed issues are demoted to fail-first regression evidence. A phase may not close on those tests alone. Each phase restated accordingly, with its own census / deletion / unrepresentable / evidence breakdown. The six community point-fix PRs and their issues were closed unmerged (#4762/#4736, #4897/#4837, #4848/#4661, #4610/#4605, #4609/#4606, #4530/#4499). The ADR now records that as executed rather than pending, which is what lets census-to-zero be an acceptance criterion at all — a landed point fix would make the tree look healthier than it is. Phase PRs reference those six with Refs, since CONTRIBUTING forbids a closing keyword against an already-closed issue. Refs #4906 Closes #4910 Co-Authored-By: Claude Opus 5 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- docs/adr/4910-planning-document-seam.md | 499 ++++++++++++++++++++++++ docs/adr/README.md | 1 + 2 files changed, 500 insertions(+) create mode 100644 docs/adr/4910-planning-document-seam.md diff --git a/docs/adr/4910-planning-document-seam.md b/docs/adr/4910-planning-document-seam.md new file mode 100644 index 000000000..dd0d46e8f --- /dev/null +++ b/docs/adr/4910-planning-document-seam.md @@ -0,0 +1,499 @@ +# ADR-4910: Planning documents are read and written through one parse → mutate → serialize seam [Proposed] + +- **Status:** Proposed — design lock for Phases 1–6 of epic [#4906](https://github.com/open-gsd/gsd-core/issues/4906). Ratify to `Accepted` at Phase 6 closeout, once the phases have demonstrably shipped. No production code lands in this PR. +- **Date:** 2026-09-20 +- **Issue:** [#4910](https://github.com/open-gsd/gsd-core/issues/4910) — Phase 0 of epic [#4906](https://github.com/open-gsd/gsd-core/issues/4906) +- **Subsumes as layers:** [ADR-1372](1372-markdown-sectionizer-seam.md) (`markdown-sectionizer` — structure), [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) (`markdown-table`, bounded mutation, fail-loud `Result`). Both remain in force and unchanged; this ADR frames them as the layers a planning document composes, and neither is a rewrite target. **Their reciprocal `Subsumed by` back-links are deliberately not added yet:** `docs/adr/README.md` lifecycle rule 3 states that only an `Accepted` ADR is owed the back-link, because a `Proposed` ADR's claim is prospective. They land in the Phase 6 ratification PR, when this ADR becomes `Accepted` and the index check begins demanding them. +- **Relationship to prior work:** the third consolidation in this family, after [#1372](https://github.com/open-gsd/gsd-core/issues/1372) (read seam) and [#2143](https://github.com/open-gsd/gsd-core/issues/2143) (tables, bounded mutation, fail-loud). Sibling: [#2121](https://github.com/open-gsd/gsd-core/issues/2121) (`phase-id.cts`). The node-scoped parse-error contract (§5) applies to documents the `Evidence` distinction [#4631](https://github.com/open-gsd/gsd-core/issues/4631) draws for gates. + +## Context + +Twelve open `confirmed-bug` issues are one defect: **GSD's durable state lives in prose markdown, +and every verb brings its own regex to it.** + +Two seams already exist for this and both are correct. `markdown-sectionizer` ([ADR-1372](1372-markdown-sectionizer-seam.md)) owns +fenced code, headings, sections and bullets. `markdown-table` ([ADR-2143](2143-markdown-table-and-mutation-consolidation.md)) owns GFM tables, the +bounded `withSection` mutation primitive, and the fail-loud `Result` in `write-set.cts`. What +neither owns is the **composition**: a planning document is frontmatter + a section tree + tables + +bold-label fields + checklists, and the verbs that mutate one reach past all three seams to a +`String.replace`. + +The result is a repo that ships a strict escaping reader and an unescaped prose writer for the same +table. + +### Four arms, one missing owner + +**(a) The writer emits what the reader is required to reject.** `quick.md` Step 7c interpolates a +raw `${DESCRIPTION}` into a pipe row; `escapeCell` exists at `src/markdown-table.cts:716` — and its +own doc comment describes a *caller-must-re-escape contract*, i.e. an obligation the caller may +simply not discharge — and is applied per column at `:852-862` on the *generated* path, not this +one. A task description containing a +Jinja filter permanently ragged the table, and `quick-tasks-migrate` cannot help because the next +`/gsd-quick` re-corrupts it ([#4736](https://github.com/open-gsd/gsd-core/issues/4736)). A project +that migrates is not out of the state. Symmetrically, `parseDecisions` rejects a second colon in a +plain-prose bold lead-in — text `discuss-phase` itself emits — forcing +`outcome: "could-not-parse"` and hard-blocking `check.decision-coverage-plan` +([#4793](https://github.com/open-gsd/gsd-core/issues/4793)). + +**(b) A structural write replaces a line span and destroys what shares the line.** +`src/phase.cts:3795` (on `next`) matches `/(\*\*Plans:\*\*\s*)[^\n]+/i` — one capture group, the +rest of the line matched and uncaptured — and `:3842` re-emits `$1` and a new count and nothing +else. On a real close that deleted 174 of the line's 205 characters, a paragraph documenting a +design decision, with no warning and a payload reporting success +([#4852](https://github.com/open-gsd/gsd-core/issues/4852)). + +This is the **third** occurrence of one defect. `src/roadmap.cts:1196` — the sibling writer for the +*same line* — already does it correctly, and the gap between the two sites is not subtle. Its +`planCountPattern` carries three capture groups (label, count token, trailing) and `:1213-1216` +re-emits `${label}${planCountText}${trailing}`, under a comment enumerating **three arms**: + +> 1. `$2` present (a real count token) → rewrite the token, preserve `$3` verbatim (an annotation a +> human wrote after a real count; #2853). […] 3. Anything else (freeform prose, `TBD` / `TBD — +> annotation`, a bracketed human note, the first line of a wrapped sentence, an empty value) → +> leave the whole matched line untouched. + +It even distinguishes a fresh-template placeholder from a bracketed *human* annotation that is +structurally identical (`[Deferred pending re-scope]`), positively, on wording — because treating +the two alike was bug #3584 Finding A. That is three arms of accumulated care at one site and none +of it at the other. + +The pattern arrived in `d49a7d0c4` (fix #2853, 2026-07-31) and was tightened in `0f417aa6d` (fix +#3584, 2026-08-18). **Neither touched `src/phase.cts`.** The same shape appears +again in `state.update "Last Activity"`, which deletes `last_activity_desc` and `state_head` while +its own payload lists `"Last Activity Description"` under `preserved` +([#4862](https://github.com/open-gsd/gsd-core/issues/4862)). + +The correct implementation existing beside the incorrect one, in the same repo, for the same line, +with a comment explaining the rule, is the evidence that a comment is not a mechanism. + +**(c) Unrecognised grammar returns empty instead of raising.** `roadmap.analyze` returns +`phases: []` and `phase_count: 0` for a checklist-only ROADMAP — a shape `templates/roadmap.md` +itself emits — *while the same call builds 26 `missing_phase_details` tokens and discards them*, +and while the same tree enumerates 20 phases through `init.progress` +([#4899](https://github.com/open-gsd/gsd-core/issues/4899)). `adr-parser` returns `decisions: []` +for `## 11. Locked decisions` and **reports success**, from two independent causes: the leading +section number is not stripped, and `locked decisions` is not a synonym +([#4900](https://github.com/open-gsd/gsd-core/issues/4900)). `extractPhaseFieldMultiline` folds +list items, fences, lowercase labels and inline `**mentions**` into a field value, and one shape +silently marks *another phase's* requirement complete +([#4837](https://github.com/open-gsd/gsd-core/issues/4837)). + +In each case a caller cannot distinguish *"this document records nothing"* from *"I could not read +this document."* + +**(d) The pattern is duplicated, so a fix lands at one copy.** #4478's phase-heading anchor fix +landed in `roadmap-parser.cjs`; `init.cjs:2229`, `:2401`, `:3044` and `milestone.cjs:646` keep +unanchored `gi` copies, while `init.cjs:116` — in the same file as three of them — is anchored +correctly ([#4865](https://github.com/open-gsd/gsd-core/issues/4865)). The codebase disagrees with +itself inside one file. Same shape in +[#4661](https://github.com/open-gsd/gsd-core/issues/4661) (`/gsd:undo`'s unanchored ERE over +`git log`), [#4605](https://github.com/open-gsd/gsd-core/issues/4605) / +[#4606](https://github.com/open-gsd/gsd-core/issues/4606) (`pr-branch`'s `STRUCTURAL_RE`), and +[#4499](https://github.com/open-gsd/gsd-core/issues/4499) (`frontmatter.set` reformatting block +sequences it was not asked to touch). + +### Why a design lock, before any code phase + +Three facts make this ADR necessary rather than ceremonial. + +1. **The composition layer is a genuine gap, not an oversight to patch.** [ADR-1372](1372-markdown-sectionizer-seam.md) explicitly + excluded tables, mutation and fail-loud; [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) covered those three and scoped itself to + `ROADMAP.md` / `STATE.md` mutation sites, listing *"migrating non-planning markdown"* as a + non-goal and document-model parsing as out of #3180's scope. A planning *document* as one + object has never had an owner. Six phases will build against whatever this file says. + +2. **The epic names a drain point that does a different job — and a ratchet the language cannot + deliver.** Both are corrected in §6 and §7 below. Discovering either mid-Phase-5 would strand + the phase. + +3. **Six of the twelve absorbed issues had open community point-fix PRs**, which is the epic's + stated non-goal already happening in the contribution queue. All six were closed unmerged, with + their issues, rather than landed — see *"The point fixes were closed, deliberately"* below. That + decision is what makes each phase's census reflect the real tree rather than a partially-patched + one, and it is the reason this ADR can specify census-to-zero as an acceptance criterion at all. + +There is no recorded ADR or Cortex decision governing this composition seam today +(`recall_decision` returns no governing contract; `.out-of-scope/` carries no prior denial), so +later phases have nothing authoritative to build against. + +## Decision + +Eight decisions, locked. Phases 1–6 execute against them as separate PRs. + +### 1. One `PlanningDoc` seam, composing the existing layers + +A new leaf module parses a planning artifact **once** into a `PlanningDoc`: frontmatter, a section +tree, and typed field nodes (bold-label line, GFM table, checklist). It is owned by neither +`phase` nor `roadmap` nor `state`, because all three need it — the same Conway's-Law reasoning +`src/plan-document.cts`'s own docblock records for itself. + +The three existing seams are its **layers, not its alternatives**: `markdown-sectionizer` for +structure (including `stripFencedCode` / `scanInlineCodeSpans`, so fence- and code-span-awareness +is delegated rather than re-decided), `markdown-table` for tables, `frontmatter.cts` for +frontmatter, `write-set.cts` for `Result` and `WriteOutcome`. **Extend, never mutate** — +[ADR-2143](2143-markdown-table-and-mutation-consolidation.md) §2's lock is inherited verbatim. Neither `Accepted` ADR is reopened by this work. + +The seam is **registry-scoped to planning artifacts**, not to markdown in general. `.planning/` +artifacts are already enumerated by `src/artifacts.cts`'s `isCanonicalPlanningFile`; the registry +extends that rather than inventing a second notion of what a planning document is. + +> **Naming, recorded as a live risk.** `src/plan-document.cts` already exists and parses one +> `*-PLAN.md` **body**. The ambiguity is resolved by **absorption** — that module becomes a typed +> field reader beneath this seam — not by a naming convention every future author must be told. +> Until that lands, the two names are close enough to confuse, and this note is the warning. + +### 2. A structural write replaces a node. There is no span, because there is no regex. + +A field write owns its token and re-emits everything else on its line **by construction**. The +parser decides where a value ends and hands the writer a node carrying a `valueSpan`; the writer +has no expression that can reach past it. + +This is the same rule `src/roadmap.cts:1196` encodes by hand in a third capture group and a +comment. The difference is that `$3` stops existing, because capture groups stop existing. A rule +that depends on an author reading a comment has now failed three times (#2853, #3584, #4852) at +one line in one file. + +Mutation is node-addressed, never path-string-addressed. A path string (`"phases.3.plans"`) is a +grammar, a grammar needs a parser, and that is how this epic started. A handle returned by the +parser cannot address a node the parser did not find. + +### 3. Serialization is byte-stable for untouched regions + +`serialize` splices mutated node spans back into the **original buffer**. A region the mutation did +not touch is the original bytes — not a faithful re-render of them. + +"Re-render faithfully" is an unbounded obligation: it must reproduce every whitespace, alignment +and quoting choice a human made. #4499 is exactly that obligation being missed, and it closes here +without a targeted fix. This is a deliberate **Hyrum's Law** commitment: every byte of an untouched +region is depended on by someone's diff review, and a semantically-equivalent reformat is a real +break. + +It also closes a round-trip corruption before it exists: an untouched escaped cell (`\|`) is not +re-escaped, because it is not re-rendered. + +### 4. One writer per artifact, sharing its escaping with the reader + +For each typed field kind, the escape-or-refuse function is **exported by the reader's module** and +called by the writer. Not a third shared module — that is a second place for the pair to drift, +which is the defect. A parity test per field kind asserts the pair. + +A value that cannot be represented in the grammar is **refused** by the writer, with a report. A +writer never emits a document its own reader must reject. + +Prose-interpolated table rows in shipped workflow markdown are replaced by a `gsd_run` verb that +appends through the seam, so the escaping is executed once rather than restated in prose. + +#### 4a. `accepted ⊇ emittable` — the accept-but-never-emit rule + +Each grammar is declared **accepted** (the reader takes it) and, separately, **emittable** (the +writer may produce it). `emittable ⊆ accepted`, asserted. A grammar may be accept-only. + +This is **Postel's Law applied at the right level**, and it is worth stating why, because the naive +reading argues the opposite. Postel's own guidance splits by level: *internal system-to-system — +stricter on both ends; user-facing input — more liberal.* GSD's planning artifacts are **both**. +They are written by the tool and hand-edited by humans — the epic's stated constraint is that they +*"stay human-readable and hand-editable; that is the constraint, not the problem."* So the reader is +liberal toward a human's variation (#4793 is a reader being wrong to reject prose a human, or +`discuss-phase` itself, legitimately wrote), the writer is conservative, and **neither is ever +silent**. "Be conservative in what you send" is the uncontroversial half and applies to the writer +without qualification. + +### 5. A parse miss is typed, carries its span, and is scoped to the node + +`[]` means **none**. An unrecognised shape surfaces `could-not-parse` with the offending span. + +**The error lives on the node, not the document.** This is the sharpest constraint in the epic and +the naive reading gets it wrong: making an unrecognised table fail the whole artifact would take +out `phase list`, `init.progress` and every other consumer of the same file. Instead the document +parses, the affected node carries the parse error, a consumer that reads that node gets +`could-not-parse` with the span, and a consumer reading a different node is unaffected. + +The document-level `Result` failure is reserved for a document that is not a planning +artifact at all — unreadable, no frontmatter terminator, not the declared kind. + +> **This is an interpretation of #4906's criterion, stated rather than assumed.** The epic's +> "Done when" reads *"an unparseable shape surfaces `could-not-parse` with the offending span"* and +> carries no document-or-node qualifier. This ADR reads it **distributively** — per unparseable +> *shape*, not per file — which is what node-scoping delivers. The reading is recorded here +> explicitly because the alternative (document-scoped) is also a faithful reading of the same +> sentence and would produce a materially different Phase 1 and Phase 4: one bad table would fail +> `phase list` and `init.progress` along with `roadmap.analyze`. If the epic intended the +> document-scoped reading, §5 and the Phase 1/4 acceptance criteria are what change. + +This is [ADR-1411](1411-resolution-provenance.md)'s "report provenance rather than fall open silently" and [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) §5's no-null- +swallow rule, at document-node granularity; it is the `Evidence` distinction #4631 draws for gates, +applied to documents. #4899's own bug is the strongest argument for it: the evidence was *already +computed* — 26 `missing_phase_details` tokens, built and then discarded — and thrown away on the +way to reporting `phases: []`. + +### 6. The ratchet is type-narrowing **plus** lint — not typechecking alone + +The epic states: *"the write boundary takes a `PlanningDoc` mutation, not a `string`, so a +reintroduced `content.replace(/…/)` against a planning artifact does not typecheck."* + +**The narrowed boundary is adopted. The guarantee, as stated, is not available.** + +```ts +fs.writeFileSync(planningPath, content.replace(/(\*\*Plans:\*\*\s*)[^\n]+/i, …)); +``` + +typechecks perfectly. `fs` has never heard of `PlanningDoc`, and no narrowing of *our* boundary +reaches a call that never goes near it. Publishing the epic's sentence unqualified would hand every +later reader a guarantee that one `require('node:fs')` defeats — and a false guarantee is worse +here than none, because it is the thing that stops someone writing the lint. + +So enforcement is two mechanisms with stated scopes: + +- **Type-narrowing at the seam's write boundary** — accepts a `PlanningDoc` mutation, not a + `string`. Makes the correct path the easy path and catches every bypass *through the seam*. +- **`local/no-adhoc-markdown-parsing`, extended** — owns the bypass. It is already the mechanism + [ADR-1372](1372-markdown-sectionizer-seam.md) §2 and [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) §7 both chose, and [ADR-2143](2143-markdown-table-and-mutation-consolidation.md)'s Phase 4 **shipped**: the rule today + guards `src/**/*.cts`, `tests/**/*.cjs` and `scripts/**/*.cjs`, and already carries + `tableRegex` and `adhocReplaceMutation` message ids alongside `fenceRegex` and `sectionCollect`. + This ADR does not ask for that work again. + + **What it does not yet catch is specific, and #4852 is the proof.** `adhocReplaceMutation` keys + on *"a `.replace()` mutation of a roadmap/state document using a hand-rolled **table or section** + regex"*. `/(\*\*Plans:\*\*\s*)[^\n]+/i` is neither: it is a **bold-label field** regex. So the + defect lives in `src/phase.cts` — a file this rule does lint — and `lint:ci` is green. That is a + behavioural demonstration of the gap, not an inference from reading the rule. + + The extension is therefore one detector, not a new mechanism: a field-shaped `.replace()` + mutation of a registry-recognised planning artifact, and a raw + `readFileSync` → `.replace()` → `writeFileSync` triple against one. Paired with an allowlist + drain (#4446 pattern) for what exists today, deleted per phase rather than renewed. + +> **Correction to the epic, recorded deliberately.** #4906 names +> `scripts/lint-planning-artifact-writer-drift.cjs` as *"the drain point; extend it."* That script +> exists, but its own module docblock scopes it as a **registry-completeness** guard: every writer +> of a `.planning/`-**root** file is represented in `isCanonicalPlanningFile`. It states *"No +> ratchet / no baseline"* as a design choice, silently skips any runtime-computed target, and never +> inspects *how* a write is performed. It is a correct guard on a different axis and is left +> untouched. Extending it would have meant teaching a registry checker about regex shapes. + +**Stated coverage limit.** A lint over `src/*.cts` cannot see #4736's writer, which is prose in +`gsd-core/workflows/quick.md`. Decision 4's `gsd_run` verb is what brings that surface inside the +fence; the lint does not, and this ADR does not pretend otherwise. + +### 7. Every parser ships a positive control per accepted grammar + +A parser declares the grammars it accepts (§4a's `accepted` set). A lint asserts one positive- +control fixture per declared grammar; a parser without one fails the build. Modelled on +`scripts/lint-table-schema-drift.cjs`, which already does this for table schemas and is wired into +`lint:ci`. + +This is what stops #4837's four continuation shapes and #4900's two heading forms regressing +silently. It is also the guard that would have caught #4900 at authoring time: `locked decisions` +was never a declared synonym, so no control ever existed for it. + +Per `CONTRIBUTING.md`'s fixture-provenance rule (#2371), a control may not be derived from the +parser's own writer or docstring examples — and §4a makes that mechanical rather than a matter of +discipline, because the accept-only grammars are by definition ones the writer cannot produce. + +### 8. Exactly one implementation of a shared pattern, proven by a drift guard + +The phase-heading pattern has one owner. Five copies (#4865) collapse to one, proven by the +existing drift-guard idiom rather than by review attention. Same rule for `/gsd:undo`'s scope +selector (#4661) and `pr-branch`'s `STRUCTURAL_RE` (#4605, #4606): the pattern is a property of the +seam's grammar, not a literal each call site owns. + +## What makes a phase done + +**A phase is done when a structural property holds, not when N symptoms are gone.** This is stated +as its own section because it is the single thing most likely to be lost between this file and the +implementation, and the epic says so in its own words: *"An implementation that does that has not +closed this epic, even with every symptom gone and CI green."* + +Every phase below carries the same three acceptance criteria, in this order: + +1. **Census → zero.** The phase opens by enumerating every instance of its anti-pattern in the tree + and publishing that count in its PR. It closes when the count is **zero**. The number is + measured by the phase, not guessed here — but the obligation to measure it, and to drive it to + zero rather than to "the reported ones", is locked now. +2. **Deletion, not coexistence.** The bespoke implementations are **removed**, not kept in sync + beside the seam. The epic's framing is exact: *"two copies that agree today are the same defect + as two that disagree."* A phase that leaves a migrated call site's old code path in place has + not finished. +3. **Unrepresentable by construction.** Each phase ships one property that makes its class + *impossible* rather than *currently absent* — a property test over generated inputs, a type that + does not admit the wrong shape, or a drift guard that fails CI on the next copy. A phase whose + only evidence is per-site regression tests has bought symptom coverage, not a seam. + +**The absorbed issues are regression evidence, never the deliverable.** Each is driven fail-first +and kept, because a structural change that does not demonstrably fix the reported symptom is +unproven. But a phase is not permitted to close on those tests alone, and "the three reported sites +are fixed" is not a passing census. + +> **The six absorbed issues whose point-fix PRs were closed are now themselves closed** (#4736, +> #4837, #4661, #4605, #4606, #4499), superseded by this epic. Their phase PRs therefore reference +> them with `Refs #NNNN`, never a closing keyword — `CONTRIBUTING.md` forbids a closing keyword +> against an already-closed issue. Their regression tests are still owed; closure changes the link +> form, not the evidence obligation. + +## Phases + +Each phase is one `chore(#4906): … — Phase N` sub-issue + PR, gated on `gsd-test`, and each carries +the three criteria above. + +- **Phase 0 — this ADR.** Design lock. Docs-only. Closes [#4910](https://github.com/open-gsd/gsd-core/issues/4910) only; #4906 stays open. +- **Phase 1 — the seam exists, and is the only way to express a planning-document write** (§1, §2, + §3, §5). Net-new module; **no call site migrated**, so there is nothing for a census to count + yet. + - *Unrepresentable:* the mutation API is node-addressed, so a caller cannot name a node the + parser did not find and cannot reach past a `valueSpan`. Byte-stability is asserted as a + **property over generated documents** (`fast-check`), not over the three known sites: for any + document and any single node mutation, every byte outside that node's span is identical. + - *Structural:* one positive control per grammar the module declares it accepts (§7); the + artifact registry derived from `isCanonicalPlanningFile` rather than a second list. + +- **Phase 2 — no verb writes a planning-document field with its own regex** (§2, §3). + - *Census:* every `.replace()`-based field write against a registry-recognised planning artifact + in `src/*.cts`. Published in the PR; driven to zero. + - *Deletion:* the bespoke patterns are **removed** — including `src/roadmap.cts:1196`'s three-arm + `planCountPattern`, which is *correct today* and still goes. A correct copy of a rule the seam + now owns is exactly the two-copies-that-agree case the epic names; keeping it "because it + works" re-creates the divergence this phase exists to end. + - *Unrepresentable:* Phase 1's byte-stability property is re-run against the **migrated** call + sites, so the guarantee is a property of the writer rather than three passing examples. + - *Evidence (fail-first, `Refs`):* #4852, #4862, #4499. + +- **Phase 3 — no artifact has a writer that can emit what its own reader rejects** (§4, §4a). + - *Census:* every typed field kind in the registry; each must resolve to exactly one + escape-or-refuse function, exported by the reader's module. + - *Deletion:* `quick.md` Step 7c's prose interpolation is **removed**, not corrected in place — + replaced by a `gsd_run` verb that appends through the seam. This is the arm no lint over + `src/*.cts` can reach, so deleting the prose is the only mechanism available. + - *Unrepresentable:* `emittable ⊆ accepted` asserted **over the registry**, so a new field kind + inherits the check instead of needing one written for it. + - *Evidence (fail-first; `Refs` for #4736):* #4736, #4793. + +- **Phase 4 — a caller can always distinguish "records nothing" from "I could not read it"** (§5). + - *Census:* every planning-artifact read path returning a collection. How many can return `[]` + for an input they failed to parse? Driven to zero. + - *Deletion:* the discard sites go. #4899's `missing_phase_details` evidence is *already + computed* — 26 tokens built and then thrown away on the way to reporting `phases: []` — and + that discard is the defect, not the parser. + - *Unrepresentable:* for a registry-recognised artifact the node type does not admit a silent + empty; a collection node carries either values or a parse error with its span, never neither. + - *Evidence (fail-first; `Refs` for #4837):* #4899, #4900, #4837. + +- **Phase 5 — every shared grammar has exactly one implementation** (§8). + - *Census:* copies per pattern. The phase-heading pattern is **five** today (#4865 — four + unanchored, one correct, three of them in the same file as the correct one). Driven to one. + - *Deletion:* the four copies are **removed**, not aligned. Aligning them is what #4478 did, and + it is why #4865 exists. + - *Unrepresentable:* a drift guard fails CI on the next copy, so the count cannot climb back — + the mechanism `scripts/lint-table-schema-drift.cjs` already applies to table schemas. + - *Evidence (fail-first; `Refs` for #4661, #4605, #4606):* #4865, #4661, #4605, #4606. #4661 + carries a constraint the phase must respect: the obvious anchor fix silently **under**-selects + fixups into a partial revert, so the remedy is selecting by the scope a commit *declares*, not + a better regex. +- **Phase 6 — the class cannot be reintroduced** (§6, §7). + - *Census:* the grandfather allowlist. It starts at whatever Phases 2–5 could not reach and ends + **empty** — an allowlist that survives the phase is a ratchet with nothing to hold. + - *Deletion:* allowlist entries are deleted as each site migrates, never renewed. This is the + idiom `local/no-source-grep` and ADR-2143 §7 both use, and ADR-2143 Phase 4's own experience is + the reason it is restated: entries that are renewed instead of drained become orphaned markers + pointing at closed epics. + - *Unrepresentable:* the narrowed write boundary plus the field-shaped `.replace()` detector, so + the next occurrence fails CI rather than review. Plus the positive-control lint (§7): a parser + declaring an accepted grammar with no control fails the build. + - *Also ratifies this ADR* to `Accepted` with a dated Ratification section, and adds the + reciprocal `Subsumed by` back-links to [ADR-1372](1372-markdown-sectionizer-seam.md) and + [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) — which lifecycle rule 3 begins + demanding at exactly that point. + +**Ordering constraints.** Phase 1 introduces the primitive every later phase consumes and must +precede all of them. Phase 6 must land **last**: a ratchet whose allowlist still grandfathers every +unmigrated site is a ratchet with nothing to hold. Phases 2–5 are mutually independent — their +censuses do not overlap — and may run in any order, or in parallel. + +## The point fixes were closed, deliberately + +Six of the twelve absorbed issues had open community PRs doing point fixes at their existing call +sites. **All six were closed unmerged on 2026-09-20**, with thanks and an explanation, along with +their issues: + +| PR | Issue | Contributor | Owning phase | +|---|---|---|---| +| #4762 | #4736 | @behruznassre | 3 | +| #4897 | #4837 | @behruznassre | 4 | +| #4848 | #4661 | @0xdhx | 5 | +| #4610 | #4605 | @tarc | 5 | +| #4609 | #4606 | @tarc | 5 | +| #4530 | #4499 | @drungrin | 2 | + +This is the epic's non-goal enforced rather than merely stated: *"Repairing the absorbed issues +individually at their existing call sites without building the seam. That is the pattern that +produced them — three times for #4852 alone — and it leaves the epic open."* Landing six correct +point fixes would have left the missing owner missing and made each phase's census start from a +tree that looks healthier than it is. + +**The work was not wasted, and this is not a quality judgement.** Every one of those PRs is cited +in this ADR's Context: #4736 is the sharpest statement of the writer/reader split, #4661 supplies +the constraint that the obvious anchor fix is *also wrong*, #4606 is the fifth attempt at one +defect and is the epic's strongest recurrence evidence, and #4499 is the quietest issue in the +cluster and the one that proves the writer re-renders regions it was never asked to touch. + +Two consequences for the phases: + +- **A closed issue changes the link form, not the evidence obligation.** Phase PRs reference these + six with `Refs #NNNN`; `CONTRIBUTING.md` forbids a closing keyword against an already-closed + issue. Each still owes its fail-first regression. +- **The census is now the whole tree, not the reported sites.** With no point fix landing, each + phase's census reflects the real state of the code, which is the number that matters. + +## Consequences + +- **Positive.** The class ends where it is generated rather than where it is reported: a field + write cannot destroy its line, a writer cannot emit what its reader rejects, an unreadable + document cannot report itself as empty, and a shared pattern has one copy. New verbs inherit + correctness from the seam instead of re-deriving it — which is what the previous two + consolidations each demonstrated. +- **Cost.** Seven PRs plus lint infrastructure. Phases 2, 4 and 5 carry High blast radius + (`cmdPhaseComplete`, the roadmap/ADR read paths, five call sites across three files) and need + per-consumer `get_impact` due diligence. +- **Risk — the honest one.** Behaviour-preserving migrations regress subtle formatting, and this + seam's whole value proposition is formatting fidelity. Mitigated by §3's byte-stability property + test, the fail-first regression per absorbed issue, §7's positive controls, and the per-phase + `gsd-test` gate. [ADR-2143](2143-markdown-table-and-mutation-consolidation.md)'s own Phase 5 exists because a coverage re-check found a seam its + Phase 0 had left unowned; `/adr-phase-coverage` runs again at epic closeout for that reason. +- **Risk — naming.** `planning-document` beside `plan-document` (§1). Live until absorption lands. +- **Non-goals.** Repairing the twelve at their existing call sites (the pattern that produced them + — three times for #4852 alone). Replacing markdown as the storage format; the artifacts stay + human-readable and hand-editable. Rewriting `markdown-sectionizer` or `markdown-table`. + Re-litigating [#1639](https://github.com/open-gsd/gsd-core/issues/1639)'s `[^:*]*` discipline as + policy — this ADR changes *where* a grammar is defined and *how* a miss is reported, not what a + decision title may contain, though §4a is why #4793 resolves anyway. + +## Alternatives considered + +1. **Repair the twelve issues at their call sites.** The epic's own non-goal, and the pattern that + produced them. Rejected **in practice, not only on paper**: six community PRs were doing exactly + this and all six were closed unmerged rather than landed. This is the alternative that was + genuinely available and genuinely declined, which is what makes the rejection load-bearing. +2. **Put the composition layer inside `markdown-sectionizer.cts`.** Rejected: that seam is generic + markdown structure with no planning-domain knowledge, and `CONTEXT.md` scopes it that way. + Teaching it what a `**Plans:**` line is inverts the dependency, makes a leaf module know about + its consumers, and puts an `Accepted` contract at risk for a reason unrelated to markdown + structure. +3. **Path-string-addressed mutation** (`doc.set('phases.3.plans', v)`). Rejected: a path string is + a grammar, and a grammar needs a parser. +4. **Re-render the document faithfully instead of splicing** (§3). Rejected: an unbounded fidelity + obligation, which #4499 is an instance of being missed. +5. **A third shared escaping module** (§4). Rejected: a second place for the reader/writer pair to + drift. +6. **One document-level `Result` that fails the whole artifact** (§5). Rejected: one bad table + takes out every unrelated consumer of the same file. +7. **Type-narrowing alone as the ratchet** (§6). Rejected on the merits, with the counter-example + in-line. This is the epic's stated mechanism and it does not hold. +8. **Extending `lint-planning-artifact-writer-drift.cjs`** (§6). Rejected: correct guard, different + axis, explicitly ratchet-free by design. + +## References + +- Epic: [#4906](https://github.com/open-gsd/gsd-core/issues/4906) · Phase 0 sub-issue: [#4910](https://github.com/open-gsd/gsd-core/issues/4910) +- Layers: [ADR-1372](1372-markdown-sectionizer-seam.md) (`markdown-sectionizer`), [ADR-2143](2143-markdown-table-and-mutation-consolidation.md) (`markdown-table`, bounded mutation, fail-loud) +- Fail-loud precedent: [ADR-1411](1411-resolution-provenance.md) — report provenance rather than fall open silently +- Sibling consolidation: [#2121](https://github.com/open-gsd/gsd-core/issues/2121) (`phase-id.cts`) +- Absorbed: [#4736](https://github.com/open-gsd/gsd-core/issues/4736), [#4793](https://github.com/open-gsd/gsd-core/issues/4793), [#4852](https://github.com/open-gsd/gsd-core/issues/4852), [#4862](https://github.com/open-gsd/gsd-core/issues/4862), [#4899](https://github.com/open-gsd/gsd-core/issues/4899), [#4900](https://github.com/open-gsd/gsd-core/issues/4900), [#4837](https://github.com/open-gsd/gsd-core/issues/4837), [#4865](https://github.com/open-gsd/gsd-core/issues/4865), [#4661](https://github.com/open-gsd/gsd-core/issues/4661), [#4605](https://github.com/open-gsd/gsd-core/issues/4605), [#4606](https://github.com/open-gsd/gsd-core/issues/4606), [#4499](https://github.com/open-gsd/gsd-core/issues/4499) diff --git a/docs/adr/README.md b/docs/adr/README.md index 82073d32e..ae8ba882e 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -292,6 +292,7 @@ Decided in principle, not yet ratified. Do not cite as settled architecture. | [ADR-3942](3942-emitted-drift-ack-commit-trailer.md) | The emitted-drift acknowledgment is PR-lifetime data — it belongs in a commit trailer, not the working tree | Proposed | — | | [ADR-4629](4629-state-write-intent-beyond-frontmatter.md) | STATE.md write intent beyond frontmatter — bounded, verified writes | Proposed | — | | [ADR-4650](4650-path-containment-and-filename-classification-seam.md) | One path-containment predicate and one filename-classification helper | Proposed | — | +| [ADR-4910](4910-planning-document-seam.md) | Planning documents are read and written through one parse → mutate → serialize seam | Proposed | — | ### Superseded, Retired, and Legacy