From e2f4c16d9e69dac080d7bb44096113d688cf9db8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 16:04:09 -0400 Subject: [PATCH] refactor(#3469): one composition for the STATE.md write seam (#3501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(#3469): amend ADR-3408 section 8.3 — the pipeline has sanctioned exceptions Section 8.3 read 'Every STATE.md write applies the pipeline.' That is false by design for two commands, and acting on it would have inverted a shipped feature. Preservation makes curated frontmatter win over a re-derived body value. state sync exists to do the opposite — #905's 'body annotation beats existing frontmatter when both are present'; it re-derives frontmatter FROM the body. REGENERATE_STATE is a factory reset that rebuilds STATE.md from scratch. Applying the pipeline to either would re-lock exactly what the command was invoked to replace. This issue's own scope line, inherited from the epic, said to route the direct writeStateMd callers through the pipeline. For cmdStateSync that would have shipped silently, with every gate green, because no test asserts that sync LETS the body win. Caught by reading the helper's docstring and then verifying the claim against the code — a stale comment had already misdirected this epic once. Both commands are now named in a closed exception list and are permanent ratchet entries. Consequence recorded rather than left to bite Phase 4: the 'drive the ratchet to 0 and delete the file' target in this ADR and in #3471 is wrong. Two entries are permanent, so the correct end state is 2, and the honest report is '0 removable bypasses, 2 sanctioned'. A guard reaching 0 here would only do so by having stopped looking at two real writers. * refactor(#3469): one composition for the write seam, not one per caller Implements ADR-3408 section 8.3 as amended. syncAndPreserveStateMd is now the single composition of syncStateFrontmatter and applyPostSyncPreservation. readModifyWriteStateMd and cmdPhaseComplete both CALL it instead of each assembling the two steps themselves. cmdPhaseComplete keeps its own writePlanningFileSet envelope — the composition returns content, it does not take over the write, so STATE.md still commits atomically with ROADMAP and REQUIREMENTS. Assembling the stages at a call site is a re-derivation even when every step calls an owner. Upstream's fix(#3374) routed cmdPhaseComplete through applyPostSyncPreservation but left it calling syncStateFrontmatter directly first, so the composition was duplicated and free to diverge with both guards green. That is ADR-3180 Amendment 2's finding repeating on the write side. cmdMilestoneComplete gains preservation. It wrote through writeStateMd, so it got sync and no preservation — the identical shape #3374 reported for phase.complete, and flagged upstream as a follow-up in the helper's own docstring. This is that follow-up. Divergence is now visible: preservation_warnings names each field restored over a disagreeing derived value. Deliberately NOT named warnings — cmdPhaseComplete already exposes warnings as a prose string array, and two sibling commands carrying that name with different element types is Generative Fix Divergence, the class this epic exists to remove. patchCore stops running stateReplaceField over the whole document. One observable consequence, intended per design row 9: a frontmatter-shaped patch key with no body counterpart now reports failed instead of silently succeeding, because the old whole-document match was literally hitting the YAML line case-insensitively. The guard closes Phase 1's DECLARED KNOWN GAP as promised rather than re-deferring it: section 8.3(b) detection is tractable now the composition exists. Scoped by two factors to avoid Phase 1's measured 29-to-1 false positive rate — a variable field-name argument AND a content argument whose nearest preceding assignment is not stripFrontmatter. Verified 0 findings and 0 false positives across all 33 call sites, plus 5 synthetic shapes. It also detects the re-assembly shape above. Ratchet: 4 entries to 2, both sanctioned-permanent. cmdStateSync's owner changes from #3471 to sanctioned-permanent per Amendment 2 — routing it through preservation would invert the #905 contract. Also fixed inline rather than deferred: cmdMilestoneComplete's STATE.md read now happens inside withStateLock. It previously read outside any lock before writeStateMd took its own, leaving a TOCTOU window under concurrent writers. * test(#3469): characterization coverage for the single write seam Matrix sections A-E. Criterion 6 was amended by maintainer decision — all five instances closed by point fixes while Phase 1 was in flight — so these are characterization tests at the consumer's output per ADR-3180 Decision 4(b)/(c), paired with the drift guard's count, never either alone. Section C is the one that earns its keep. cmdStateSync is a sanctioned permanent exception: state sync exists to re-derive frontmatter FROM the body, so preservation there re-locks exactly what the command was invoked to replace. C1 pins that the body wins; C4 pins that this phase left the command byte-identical. Nothing else in the suite would notice if a future change made sync start preserving, and the natural reading of 'one write seam' is to make precisely that change. Section E pins the guard's false-positive scoping. E4 (updateCore's strip-then-replace) and E5 (sectionBody-scoped calls) must NOT be reported — the naive detector measured 29 false positives to 1 true positive in Phase 1. E7 is the inverse: a sanctioned-permanent entry disappearing must FAIL, because a guard reaching zero here would only do so by having stopped looking at two real writers. Also corrects a stale test that asserted patchCore's old whole-document behavior, which this phase deliberately changes. One honest limitation, flagged rather than papered over: A1's 'byte-identical to pre-refactor' cannot be diffed against real pre-refactor bytes from inside the suite. It is implemented as the seeded fast-check property that cmdPhaseComplete's composed output equals readModifyWriteStateMd's for the same inputs — the strongest available proxy, not the literal claim. * docs(#3469): refresh the seam glossary entry and add the changeset Two spec-review gaps, both real. CONTEXT.md's STATE.md Transition Module entry named three direct writeStateMd callers including cmdMilestoneComplete. This phase routed that one through the composition, so the line was false the moment the refactor landed. Worth recording plainly: I wrote that sentence in Phase 0, correcting an older stale pointer in it, and my own Phase 2 change invalidated it again within the same epic. That is the exact drift this epic exists to remove, demonstrated on the epic's own documentation — and it is why the entry now ends by saying the whole-repo drift guard, not this line, is the authoritative count. The entry now records the composition (syncAndPreserveStateMd) and states that exactly two direct callers remain, both SANCTIONED PERMANENT rather than debt. Changeset: type Changed, because milestone complete's observable output moves. Tier-2 per ADR-3180 Decision 3 — a stale body line no longer wins over fresher frontmatter, and the command gains preservation_warnings. Docs requirement is met by the ADR amendment already in this diff. * test(#3469): register property-test temp-dir cleanup at creation time Standards review, minor but real: the new fast-check property cleaned up its temp dirs in a loop AFTER fc.assert returned. A genuine property failure throws, so that line never ran and every dir from the failing run — including all of fast-check's shrinking iterations — leaked. The failure path is exactly when a littered machine hurts most, and a failing property test is the case the test exists for. Cleanup is now registered with t.after() at dir-creation time, so teardown happens however the test exits. Not try/finally — CONTRIBUTING.md:356 bans it inside test bodies, which is why the after-the-assertion shape existed in the first place. Swept the rest of the branch's test diff for the same shape; phase.test.cjs already uses registered teardown and nothing else matched. * fix(#3469): patchCore routes frontmatter writes instead of dropping them Checkpoint returned 10 failures of 33880. One implementation defect, three test defects, one stale test — all fixed, and the implementation defect is the one that matters. patchCore stripped frontmatter and then reconstructed it VERBATIM, applying no patches to it. An arbitrary custom frontmatter key with no body counterpart and no FIELD_CLASSIFICATION row — risk_level in the upstream fix(#3351) test — therefore always reported failed and silently never wrote. It worked before, via the old whole-document match on the raw YAML line. That is a regression against this phase's own design row 9, which requires frontmatter changes to ROUTE THROUGH the seam — still work, policy-governed — not to stop working. Removing a capability is not routing it. An upstream test caught it, which is the argument for running the checkpoint before believing the refactor. patchCore now partitions by frontmatter shape, decided structurally from the parsed frontmatter's own keys rather than a naming heuristic: - classified keys still report failed — policy owns them and a raw patch may not bypass it; - unclassified keys apply to the frontmatter object and report updated — Phase 1's behavior-table row 19, a field with no row is not this contract's business; - body-shaped keys are unchanged. The property 'failure' was my own test breaking the repo's Clock Seams rule. The two paths agree byte-for-byte; the only difference was last_updated, stamped from the wall clock on two invocations milliseconds apart, so it could never pass. Time is now frozen with mock.timers across both — not by excluding last_updated from the comparison, which would have silently stopped comparing a field the composition writes. B4's fixture could not discriminate: normalizeStateStatus maps any text containing 'complete' to 'completed', and milestone complete's own new body value derives to exactly that — which was also the fixture's stale value. The stale value is now 'executing' so the assertion can tell 'body correctly won' from 'stale survived'. B5's fixture tripped a pre-existing unstarted-phase guard before reaching any write-seam code; it now has the matching phase directory. D9 asserted the old exempt set. readModifyWriteStateMd now calls one symbol rather than assembling two, so it needs no exemption; syncAndPreserveStateMd is the sole legitimate composition site. * fix(#3469): patchCore resolves body-first, so the body wins a name collision Re-verification returned 2 failures of 33880, both D4 — the hostile row for a key that exists as BOTH a frontmatter key and a body field. The partition checked frontmatter first, so 'status' — classified in FIELD_CLASSIFICATION and also present as a body 'Status:' line — routed to the frontmatter branch, was rejected as classified, and reported failed. Wrong order. Patching 'status' means the body field, and upstream fix(#3351) says so in its own comment: 'the legitimate working case for state.patch is display-cased BODY fields — Status, Current Plan, Phase.' The body is authoritative in this model; frontmatter is the projection. D4 asserted exactly that and was right. Resolution order is now body, then frontmatter: 1. resolves to a body field -> apply to body, updated 2. else an own key of the frontmatter: classified -> failed (policy owns it) unclassified -> apply to frontmatter, updated 3. else -> failed Verified by probe against the compiled lib for all four cases rather than asserted: risk_level (frontmatter-only, unclassified) still lands; current_phase still fails; display-cased Status unchanged; D4's lower-cased status now lands via the body with the frontmatter untouched. The current_phase case was the one that could have regressed silently, so its fixture was read rather than assumed — D1's body carries 'Phase: 3 (alpha)' and no 'Current Phase:' line, so body-first cannot reach it. * chore(#3469): backfill pr number in changeset fragment --------- Co-authored-by: sim --- .changeset/calm-ibex-travel.md | 5 + CONTEXT.md | 2 +- .../adr/3408-state-write-path-preservation.md | 33 ++- scripts/lint-state-write-path-drift.cjs | 257 +++++++++++++----- scripts/state-write-path-drift-baseline.json | 18 +- src/milestone.cts | 83 +++++- src/phase.cts | 52 ++-- src/state-transition.cts | 93 ++++++- src/state.cts | 99 ++++++- tests/frontmatter.test.cjs | 83 ++++++ tests/milestone.test.cjs | 139 ++++++++++ tests/phase.test.cjs | 131 +++++++++ tests/state-transition.test.cjs | 145 +++++++++- tests/state-write-path-drift-guard.test.cjs | 208 +++++++++++++- tests/state.test.cjs | 247 +++++++++++++++++ 15 files changed, 1443 insertions(+), 152 deletions(-) create mode 100644 .changeset/calm-ibex-travel.md diff --git a/.changeset/calm-ibex-travel.md b/.changeset/calm-ibex-travel.md new file mode 100644 index 000000000..3f7c8abc2 --- /dev/null +++ b/.changeset/calm-ibex-travel.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3501 +--- +**milestone complete no longer lets a stale STATE.md body line overwrite fresher frontmatter** — it wrote through a path that re-derived frontmatter from the body with no preservation pass, so a stale Stopped-at line silently replaced a newer curated value, exactly as phase complete did before it was fixed. It now runs the same preservation the rest of the write path uses, and reports each field it protected in a new preservation_warnings array instead of staying silent about the divergence. (#3469) diff --git a/CONTEXT.md b/CONTEXT.md index 27be496b2..a9335d363 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -59,7 +59,7 @@ Module owning projection from dispatch results/errors to CLI `{ exitCode, stdout Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope for the read path; `state.cts`'s `matchCurrentPositionSection` is a thin alias over it, and the `drift-guard phase-status` seam consumes it, so the #2956 archive-shadowing fix cannot be re-derived into a second copy — the byte-exact mutation path served by `state-transition.cts`'s `locateCurrentPosition`/`sliceCurrentPositionSection` is a deliberately separate, un-consolidated locator, #3187). `stateFieldValue` (#3187) is the single owner of the #1760 frontmatter-then-body field fallback chain, consolidating the 14 re-derivations of that ladder onto one scope-carrying (`complete`/`truncated`/`unscoped`/`unreadable`) primitive. It does not scan `.planning/phases` and does not own persistence or locking; phase/plan/summary counts arrive from inventory/progress Modules as inputs, and read-modify-write paths remain Adapters. Source of truth: `gsd-core/bin/lib/state-document.cjs`. **Commit provenance (#2573):** `state_head` records the full sha STATE.md was written against, stamped by `syncStateFrontmatter` and omitted entirely outside a git repo. `readStateHeadFreshness(cwd, stateHead)` (`src/state.cts`) is the single derivation consumed by both `validate.health` (W024) and smart-entry — it returns `{ state_head, current_commit, commits_behind, commit_stale }` with **tri-state** `commit_stale`: `null` = unknown (no stamp, no git, or a stamp that is not an ancestor of HEAD after a history rewrite), `false` = known fresh, `true` = the codebase has moved. Mirrors the graphify commit-staleness contract deliberately. It is a freshness PROXY, never a drift measurement: `rev-list` counts unrelated commits and the stamp restamps on every state write, so a low count means STATE.md was written recently, not that its contents are accurate — it must never gate. ### STATE.md Transition Module -Module owning STATE.md lifecycle/maintenance transitions as intent-based methods (`beginPhase`, `advancePlan`, `completePhase`, `plannedPhase`, `milestoneSwitch`, `milestoneComplete`, `patch`, `sync`, `prune`, `update`, `rebuild`). Pure core `(content, intent, deps) → newContent` with injected I/O (file read/write, lock, disk scan); consults a field-classification table that names each STATE.md field's class (`derived-from-body` | `derived-from-disk` | `derived-from-external` | `curated` | `free`) and its preservation policy. Supersedes the 14 scattered RMW callbacks in `state.cts` (phase.cts's former direct caller has since been migrated away). **Three direct `writeStateMd` callers remain as of `next` @ `5452f1a70`: `cmdStateSync` (`state.cts:3682`), `cmdMilestoneComplete` (`milestone.cts:865`), and the `REGENERATE_STATE` remedy (`health-diagnostic.cts:337`)** — the last is the factory-reset primitive, which rebuilds STATE.md from scratch and therefore deliberately wants no preservation. ADR-3408 §8.3 governs all three; Phase 1's whole-repo drift guard, not this line, is the authoritative count. *(Location correction: this entry previously placed the factory-reset primitive at `verify.cts:1925`. It moved to `health-diagnostic.cts` when `cmdValidateHealth` migrated onto the rule table (#3309); `verify.cts` now contains no `writeStateMd` call. The design intent was unchanged — only the address was stale.)* Absorbs `syncStateFrontmatter` + `readModifyWriteStateMd`'s post-sync preservation block; Encoding 3 (`cmdStateBuildFrontmatter`) stays separate — read path concern. Sibling/super-module of the STATE.md Document Module; consumes its `stateReplaceField`/`stateExtractField` primitives. Body section structure (`## Current Position`, `## Session`, etc.) lives as a constants block inside the Module. Append-only transitions (`addDecision`, `addBlocker`, etc.) stay on today's RMW seam for now. Targets the #1760/#1761/#1743/#1695/#1264/#1255/#1257/#3242 bug cluster. Migration per ADR-1372 §T6 sequenced as substrate + `beginPhase` first (PR1), then transition-by-transition with characterization tests first per transition. **ADR-1817 adds `rebuild` as the capstone 11th transition — the body-structure derivability contract.** Re-derives `## Current Position` prose from frontmatter and `## By-Phase Progress` table from phase dirs on disk; preserves `## Session` / `## Decisions` / unknown sections verbatim; de-duplicates `## Session Continuity Archive` (keep most-recent N, default 3); appends a structured audit entry to `## Rebuild Log` (`timestamp`, `kind`, `section`, `before`, `after`, `reason`) for every mutation. Hard idempotency guarantee: a no-mutation rebuild appends no log entry, so two successive invocations on a clean file are byte-identical. Non-overlapping with `sync` (3 lightweight frontmatter fields, auto-triggered) and orthogonal to `auto_prune_state` (age-based removal) — `rebuild` reconciles with current canonical sources, `prune` removes by retention policy, the two compose (rebuild first, then prune). Section ordering is invariant: rebuild rewrites content in place, never reorders. Targets the #1776/#1761/#1591 body-drift cluster that survived ADR-1769's per-field transitions. Phased per ADR-1817: Phase 0 = this ADR + predicates (closes #1817), Phase 1 = `rebuildCore` body + `rebuild` dispatch case + drift-class unit tests (#1827), Phase 2 = `cmdStateRebuild` CLI + `--dry-run`/`--verbose` + integration tests + docs + changeset (#1826). Source of truth: `gsd-core/bin/lib/state-transition.cjs` (generated from `src/state-transition.cts`). `state_head` (#2573) is classified `{ source: 'free', preservation: 'derive' }` — an ambient git read recomputed on every write, like `last_updated`; never preserved, because a stale stamp would claim STATE.md was written against a commit it wasn't. +Module owning STATE.md lifecycle/maintenance transitions as intent-based methods (`beginPhase`, `advancePlan`, `completePhase`, `plannedPhase`, `milestoneSwitch`, `milestoneComplete`, `patch`, `sync`, `prune`, `update`, `rebuild`). Pure core `(content, intent, deps) → newContent` with injected I/O (file read/write, lock, disk scan); consults a field-classification table that names each STATE.md field's class (`derived-from-body` | `derived-from-disk` | `derived-from-external` | `curated` | `free`) and its preservation policy. Supersedes the 14 scattered RMW callbacks in `state.cts` (phase.cts's former direct caller has since been migrated away). `syncAndPreserveStateMd` (`state.cts`, #3469) is the single composition of `syncStateFrontmatter` + `applyPostSyncPreservation`; `readModifyWriteStateMd` and `cmdPhaseComplete` both **call** it rather than assembling the two steps, because assembling them at a call site is a re-derivation even when every step calls an owner. **Exactly two direct `writeStateMd` callers remain, and both are SANCTIONED PERMANENT exceptions under ADR-3408 §8.3 (as amended): `cmdStateSync` and the `REGENERATE_STATE` remedy (`health-diagnostic.cts`).** Neither is debt — `state sync` exists to re-derive frontmatter *from* the body (#905), and `REGENERATE_STATE` is a factory reset; preservation on either would re-lock precisely what the command was invoked to replace. `cmdMilestoneComplete` was the third and is now routed through the composition (#3469). Phase 1's whole-repo drift guard, not this line, is the authoritative count. *(Location correction: this entry previously placed the factory-reset primitive at `verify.cts:1925`. It moved to `health-diagnostic.cts` when `cmdValidateHealth` migrated onto the rule table (#3309); `verify.cts` now contains no `writeStateMd` call. The design intent was unchanged — only the address was stale.)* Absorbs `syncStateFrontmatter` + `readModifyWriteStateMd`'s post-sync preservation block; Encoding 3 (`cmdStateBuildFrontmatter`) stays separate — read path concern. Sibling/super-module of the STATE.md Document Module; consumes its `stateReplaceField`/`stateExtractField` primitives. Body section structure (`## Current Position`, `## Session`, etc.) lives as a constants block inside the Module. Append-only transitions (`addDecision`, `addBlocker`, etc.) stay on today's RMW seam for now. Targets the #1760/#1761/#1743/#1695/#1264/#1255/#1257/#3242 bug cluster. Migration per ADR-1372 §T6 sequenced as substrate + `beginPhase` first (PR1), then transition-by-transition with characterization tests first per transition. **ADR-1817 adds `rebuild` as the capstone 11th transition — the body-structure derivability contract.** Re-derives `## Current Position` prose from frontmatter and `## By-Phase Progress` table from phase dirs on disk; preserves `## Session` / `## Decisions` / unknown sections verbatim; de-duplicates `## Session Continuity Archive` (keep most-recent N, default 3); appends a structured audit entry to `## Rebuild Log` (`timestamp`, `kind`, `section`, `before`, `after`, `reason`) for every mutation. Hard idempotency guarantee: a no-mutation rebuild appends no log entry, so two successive invocations on a clean file are byte-identical. Non-overlapping with `sync` (3 lightweight frontmatter fields, auto-triggered) and orthogonal to `auto_prune_state` (age-based removal) — `rebuild` reconciles with current canonical sources, `prune` removes by retention policy, the two compose (rebuild first, then prune). Section ordering is invariant: rebuild rewrites content in place, never reorders. Targets the #1776/#1761/#1591 body-drift cluster that survived ADR-1769's per-field transitions. Phased per ADR-1817: Phase 0 = this ADR + predicates (closes #1817), Phase 1 = `rebuildCore` body + `rebuild` dispatch case + drift-class unit tests (#1827), Phase 2 = `cmdStateRebuild` CLI + `--dry-run`/`--verbose` + integration tests + docs + changeset (#1826). Source of truth: `gsd-core/bin/lib/state-transition.cjs` (generated from `src/state-transition.cts`). `state_head` (#2573) is classified `{ source: 'free', preservation: 'derive' }` — an ambient git read recomputed on every write, like `last_updated`; never preserved, because a stale stamp would claim STATE.md was written against a commit it wasn't. ### STATE.md Status Lifecycle (ADR-2207) The `Status` field in STATE.md follows a strict lifecycle: `Ready to plan` → `All phases complete` (all phases done, milestone awaiting formal close) → ` milestone complete` (terminal, written only by the milestone-close verb `milestoneCompleteCore`) → `Awaiting next milestone` (archived). Phase-completion verbs write `All phases complete` on the last phase — never `Milestone complete` (the overloaded bare value was removed in #2204 per ADR-2207 to decouple phase-level writes from milestone termination). `normalizeStateStatus` maps any status containing "complete" → `completed`, so consumers using the normalized projection (workstream inventory's `status` field, statusline) recognize `All phases complete` without code changes. Note: `isCompletedInventory` (workstream-inventory-builder.cts) intentionally checks only for the terminal `\bmilestone\s+complete\b` / `\barchived\b` — `All phases complete` returns `false` (intermediate, not terminal). diff --git a/docs/adr/3408-state-write-path-preservation.md b/docs/adr/3408-state-write-path-preservation.md index 856abec18..e5f35c8b3 100644 --- a/docs/adr/3408-state-write-path-preservation.md +++ b/docs/adr/3408-state-write-path-preservation.md @@ -176,7 +176,18 @@ Decisions 1–7 answer *how* the write path is organized. This section says *wha **Owner.** `src/state.cts` · the pure sync + preservation pipeline, and `readModifyWriteStateMd` as its I/O wrapper. -**Rule.** Every STATE.md write applies the pipeline. A caller needing a different I/O envelope — `cmdPhaseComplete`'s atomic three-file commit via `writePlanningFileSet` — calls the pipeline and supplies its own envelope. It does not re-assemble the stages, and it does not skip them. Assembling the stages at a call site is a re-derivation even when every step calls the owner. +**Rule.** Every STATE.md write applies the pipeline **unless the command's own contract is to let the body win** (see the exception list below). A caller needing a different I/O envelope — `cmdPhaseComplete`'s atomic three-file commit via `writePlanningFileSet` — calls the pipeline and supplies its own envelope. It does not re-assemble the stages, and it does not skip them. Assembling the stages at a call site is a re-derivation even when every step calls the owner. + +**Sanctioned permanent exceptions — a closed list; adding to it is an amendment.** Preservation makes curated frontmatter win over a re-derived body value. Two commands exist precisely to do the opposite, and applying the pipeline to them would invert the feature rather than fix a bug: + +| Command | Why preservation must NOT apply | +|---|---| +| `state sync` (`cmdStateSync`) | Its contract is #905's *"body annotation beats existing frontmatter when both are present"* — `sync` exists to re-derive frontmatter **from** the body. A preservation pass re-locks the stale frontmatter the command was invoked to replace. | +| `/gsd-health --repair`'s `REGENERATE_STATE` | A factory reset that rebuilds STATE.md from scratch. Preservation would restore exactly the values it was invoked to discard. | + +Both are **permanent entries in the ratchet with `owner: sanctioned-permanent`**, never debt. A guard reporting them is reporting correctly; a change that removes one is a regression, not progress. + +*This paragraph is Amendment 2. The rule previously read "Every STATE.md write applies the pipeline", which is false by design for both rows and would have had Phase 2 route `cmdStateSync` through preservation — inverting a shipped feature with every gate green.* **Rule.** `current_phase` and `current_phase_name` are written as a **pair**. A transaction that computes one from a phase number writes both from that same number, so the two cannot describe different phases (#3350). @@ -304,4 +315,22 @@ The four are `cmdStateSync`, `cmdMilestoneComplete`, `cmdPhaseComplete`, and `RE **Complexity, measured before and after.** `applyStatePreservation` was 177 lines, cyclomatic 61, cognitive 107, `risk_level: critical`. It is now a 27-line dispatch loop over four executors of 28, 44, 26 and 3 lines. This was Kernighan's Law's contribution to the design — the justification for the refactor was debuggability, not line count, and the largest remaining unit is the one holding the genuinely intricate #2440/#2969 ratchet. -*(Phase 3 records the §8.4 bucket decision here.)* +### Amendment 2 — Phase 2 (#3469): §8.3 was over-broad, and the ratchet's target was wrong + +Decisions 1–5 held. §8.3's **rule** did not. + +**"Every STATE.md write applies the pipeline" is false by design.** `state sync` and `REGENERATE_STATE` exist to let the body win; preservation exists to stop the body winning. §8.3 now carries the closed exception list above, and both are permanent `owner: sanctioned-permanent` ratchet entries. + +This was not a theoretical over-reach. #3469's own scope line, inherited from the epic, said to route the direct `writeStateMd` callers through the pipeline — which for `cmdStateSync` would have inverted the command, silently, with every gate green. The correction came from reading `applyPostSyncPreservation`'s docstring and then **verifying the claim against the code**, because a stale comment had already misdirected this epic once (`CONTEXT.md` pointed at a `verify.cts:1925` call that had moved to `health-diagnostic.cts`). + +**Consequence: Decision 5's and Phase 4's "ratchet to 0" target is wrong.** This ADR's guard roster and #3471 both say Phase 4 drives the baseline to empty and deletes the file. It cannot — two entries are permanent. **The correct end state is 2 permanent entries, not 0**, and the honest report is *"0 removable bypasses, 2 sanctioned"*. A guard that could reach 0 here would only do so by having stopped looking at two real writers. + +**What Phase 2 actually found in the tree**, after `fix(#3374)` (#3491) landed part of §8.3 upstream mid-epic: + +1. **`cmdPhaseComplete` re-assembles the pipeline.** Upstream routed it through `applyPostSyncPreservation`, but it still calls `syncStateFrontmatter` directly first. Every step calls an owner, so Axis 2 and an owner-level test both stay green while the *composition* is duplicated between the adapter and `readModifyWriteStateMd`, free to diverge. This is ADR-3180 Amendment 2's composition-level re-derivation, repeating on the write side, and §8.3 already forbade it by name. +2. **`cmdMilestoneComplete` is the remaining real exposure** — it writes through `writeStateMd`, so it gets sync and no preservation, the identical shape #3374 reported for `phase.complete`. Upstream flagged it as a follow-up in the same docstring; Phase 2 is that follow-up. +3. **Phase 1's declared known gap closes here**, as promised rather than re-deferred: §8.3(b)'s frontmatter-write detection becomes tractable once the composition exists, because the invariant simplifies to "no transition core calls `stateReplaceField` on unstripped content". + +**Criterion 6 context.** All five of the epic's named instances were closed by point fixes while Phase 1 was in flight, so Phase 2 and Phase 4 are driven by **characterization tests at the consumer's output** (Decision 4(b)/(c)) rather than fail-first tests. Weaker, and stated as such. + +*(Phase 3 records the §8.4 bucket decision here — though see the epic: `fix(#3351)` (#3487) appears to have subsumed most of Phase 3.)* diff --git a/scripts/lint-state-write-path-drift.cjs b/scripts/lint-state-write-path-drift.cjs index 9ca016276..7999f8341 100644 --- a/scripts/lint-state-write-path-drift.cjs +++ b/scripts/lint-state-write-path-drift.cjs @@ -62,35 +62,33 @@ * to prevent. `stripComments` does not track quoted strings for exactly * this reason — see its own header. * - * DECLARED KNOWN GAP — §8.3(b) `patchCore` frontmatter-write shape is NOT - * detected by this guard. `patchCore` runs `stateReplaceField(` over the - * WHOLE document (body + frontmatter) instead of stripping frontmatter - * first, the way `updateCore` does — a real defect, but this guard does not - * catch it. + * AXIS 3 — FRONTMATTER-SHAPED WRITE (§8.3(b)), CLOSED IN PHASE 2 (#3469). + * Phase 1 left this as a DECLARED KNOWN GAP: `patchCore` ran + * `stateReplaceField(` over the WHOLE document (body + frontmatter) instead + * of stripping frontmatter first, the way `updateCore` does, and a naive + * co-occurrence approximation ("does the enclosing function also call + * `stripFrontmatter(`?") measured at 33 occurrences of `stateReplaceField(`, + * of which only 4 were genuine write-seam bypasses and 29 were noise — the + * definition of `stateReplaceField` itself, ~20 calls on `sectionBody` (a + * body slice that is frontmatter-free by construction), and several calls + * inside `readModifyWriteStateMd` callbacks. 29 false positives to 1 true + * positive would have buried the signal. * - * Why: catching it needs genuine DATAFLOW ("is this argument a variable - * holding the full document, or a body slice?"), not function-scoped - * co-occurrence. A co-occurrence approximation (does the enclosing function - * also call `stripFrontmatter(`?) was implemented and measured directly - * against this repo: 33 occurrences of `stateReplaceField(`, of which only - * 4 are genuine write-seam bypasses and 29 are noise — the definition of - * `stateReplaceField` itself (matched as a call), ~20 calls on `sectionBody` - * (a body slice that is frontmatter-free by construction), and several - * calls inside `readModifyWriteStateMd` callbacks (correct, because the RMW - * envelope applies preservation after the callback returns). 29 false - * positives to 1 true positive buries the signal and makes the ratchet's - * shrink-rate meaningless as a Phase 2 progress indicator — recorded here, - * with these numbers, so the next reader does not re-attempt the same - * approximation. - * - * Who owns closing it: Phase 2 (#3469), which also FIXES the defect by - * consolidating on the single write seam — after which detection becomes - * tractable, because once the pure pipeline exists the invariant simplifies - * to "no transition core calls `stateReplaceField` on unstripped content". - * - * This is a DECLARED gap with a named owner, not a silent omission — a - * guard that quietly does not look somewhere is the failure ADR-3180 - * Decision 4(d) records. + * Phase 2 fixes `patchCore` (it now strips frontmatter first, matching + * `updateCore`) AND closes the gap, using a narrower, two-factor shape that + * does not reproduce that ratio: `findUnstrippedContentWrites` below flags a + * `stateReplaceField(` call only when BOTH (a) its field-name argument is a + * VARIABLE, not a fixed string literal — every OTHER call site in + * `EXECUTOR_FILE` passes a fixed Title-Case literal (`'Phase'`, `'Total + * Plans in Phase'`, ...) that can never collide with a lowercase/snake_case + * YAML frontmatter key, so a literal field name is never a candidate + * regardless of whether its content argument is stripped — and (b) its + * content argument has not been run through `stripFrontmatter` first, + * checked by a simple backward scan (within the same function) for the + * nearest preceding assignment to that argument's variable name. This is + * deliberately NOT full alias/dataflow tracking — see the function's own + * docstring for the narrow, documented limitation this trades for + * tractability. */ const fs = require('node:fs'); @@ -108,6 +106,10 @@ const BASELINE_PATH = path.join(__dirname, 'state-write-path-drift-baseline.json const REASON = Object.freeze({ FIELD_NAME_DISPATCH: 'field_name_dispatch', UNIMPLEMENTED_POLICY: 'unimplemented_policy', + // Axis 3 (§8.3(b), closed Phase 2 / #3469): a `stateReplaceField(` call + // with a variable field-name argument whose content argument was not run + // through `stripFrontmatter` first — see `findUnstrippedContentWrites`. + UNSTRIPPED_CONTENT_WRITE: 'unstripped_content_write', SEAM_BYPASS_UNRECORDED: 'seam_bypass_unrecorded', SEAM_BYPASS_COUNT_GREW: 'seam_bypass_count_grew', SEAM_BYPASS_COUNT_SHRANK: 'seam_bypass_count_shrank', @@ -132,12 +134,22 @@ const EXECUTOR_FILE = 'src/state-transition.cts'; const SEAM_OWNER_FILE = 'src/state.cts'; // Per Decision 4(d)'s "owner FILE is not exempt, only its named canonical -// FUNCTIONS are": a `writeStateMd(`/`syncStateFrontmatter(` call inside one -// of these two functions, in `SEAM_OWNER_FILE` only, is the seam's own -// internal plumbing (the I/O wrapper calling the pure sync stage), not a -// bypass. Every OTHER function in `state.cts` — and every function in every -// OTHER file — is still scanned and still flagged. -const SEAM_OWNER_EXEMPT_FUNCTIONS = ['writeStateMd', 'readModifyWriteStateMd']; +// FUNCTIONS are": a `writeStateMd(`/`syncStateFrontmatter(`/ +// `applyPostSyncPreservation(` call inside one of these two functions, in +// `SEAM_OWNER_FILE` only, is the seam's own internal plumbing, not a bypass. +// `writeStateMd` is the `cmdStateSync`/`REGENERATE_STATE` path's own I/O +// wrapper calling `syncStateFrontmatter` directly (no preservation, by +// design — §8.3's closed exception list). `syncAndPreserveStateMd` (#3469) +// is the ONE write-seam composition — `syncStateFrontmatter` then +// `applyPostSyncPreservation` — every OTHER caller needing a non-standard +// I/O envelope routes through. Every OTHER function in `state.cts` — and +// every function in every OTHER file — is still scanned and still flagged; +// in particular, `readModifyWriteStateMd` is NOT exempt: after #3469 it no +// longer contains a direct `syncStateFrontmatter(`/`applyPostSyncPreservation(` +// call at all (it calls `syncAndPreserveStateMd` like everyone else), so if +// one reappeared there it would be exactly the re-assembly shape this axis +// exists to catch. +const SEAM_OWNER_EXEMPT_FUNCTIONS = ['writeStateMd', 'syncAndPreserveStateMd']; // Unconditional path-separator normalization (never gated on // `process.platform` — a Windows-authored fork PR must be judged by the @@ -395,20 +407,128 @@ function findUnimplementedPolicies(text, rel) { return out; } -// The two write-seam functions, matched only as CALLS (`\(` immediately -// after, modulo whitespace) — never as bare mentions of the name. -const SEAM_CALL_RE = /\b(writeStateMd|syncStateFrontmatter)\s*\(/g; -// A line that IS one of the two seam functions' own definitions — skipped -// outright, never counted as a call to itself. -const SEAM_DEF_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+(?:writeStateMd|syncStateFrontmatter)\b/; +// AXIS 3 (§8.3(b), closed Phase 2 / #3469): `stateReplaceField(, +// , ...)` on a single line, capturing both argument expressions. +// `contentArg` must be a bare identifier (a call expression or property +// access as the first argument is not matched — silently out of scope, per +// this axis's own narrow-limitation note below) so its assignments can be +// tracked; `fieldArg` is everything up to the next comma, trimmed, so its +// literal-vs-variable shape can be read off directly. +const STATE_REPLACE_FIELD_CALL_RE = /\bstateReplaceField\s*\(\s*([A-Za-z_$][\w$]*)\s*,\s*([^,()]+),/g; + +// True when `arg` (already trimmed) is a fixed string/template literal — +// the safe shape, since every literal field name this codebase actually +// uses is a Title-Case body label that cannot collide with a lowercase/ +// snake_case YAML frontmatter key. +function isQuotedLiteralArg(arg) { + const t = arg.trim(); + return t.startsWith("'") || t.startsWith('"') || t.startsWith('`'); +} /** - * AXIS 2a: every direct `writeStateMd(`/`syncStateFrontmatter(` call in - * `text`, outside the two functions' own definitions and (only inside - * `SEAM_OWNER_FILE`) outside `SEAM_OWNER_EXEMPT_FUNCTIONS`'s own bodies. No - * `reason` on these findings — `applyRatchet` assigns one, since the same - * observed call site is a different failure shape depending on whether the - * baseline already knows about it. + * The nearest assignment to `varName` (`varName = ` or + * `const|let|var varName = `), scanning `lines` BACKWARD from `index` + * (inclusive) and stopping at the nearest preceding named-function + * declaration (mirrors `enclosingFunction`'s own boundary, so the scan + * cannot walk into an unrelated function above the one containing the + * call). Returns the assigned expression's trimmed text, or `null` when no + * such assignment is found before the boundary — meaning `varName` is the + * enclosing function's own untouched parameter. + * + * Deliberately single-hop: this reports whatever the NEAREST assignment's + * right-hand side literally is, and does not itself follow a further alias + * (`let body = someOtherVar;` is reported as `"someOtherVar"`, not resolved + * further). Every real call site in this file assigns its body variable + * directly from `stripFrontmatter(content)` with no intermediate alias + * (`updateCore`, `patchCore`, `beginPhaseCore`'s `tryField` helper) — a + * future call site that introduces one extra hop of aliasing would evade + * this check. A declared, narrow limitation, not a silent one — mirrors + * this file's existing precedent (`FIELD_VAR_EQ_LITERAL_RE`'s own + * documented scope) of accepting a bounded risk in trade for not chasing + * full dataflow, which is exactly what made the Phase 1 approximation + * unusable (29 false positives to 1 true positive). + */ +function nearestPrecedingAssignment(lines, index, varName) { + const assignRe = new RegExp(`(?:^|[^.\\w$])(?:const|let|var)?\\s*${escapeRegex(varName)}\\s*=\\s*([^=].*)$`); + for (let i = index; i >= 0; i--) { + if (FUNCTION_DECL_LINE_RE.test(lines[i])) return null; + const m = assignRe.exec(lines[i]); + if (m) return m[1].trim(); + } + return null; +} + +/** + * AXIS 3: every `stateReplaceField(` call in `EXECUTOR_FILE` whose field-name + * argument is a VARIABLE (not a quoted literal) — the only shape that can + * ever rewrite YAML frontmatter, since `stateReplaceField`'s `^field:` line + * pattern is case-insensitive and matches any line starting with that name, + * literal or not — AND whose content argument was not assigned from + * `stripFrontmatter(` at the nearest preceding assignment. A literal + * field-name argument is never flagged regardless of stripping: every fixed + * string this file's `stateReplaceField` calls use is a Title-Case body + * label (`'Phase'`, `'Total Plans in Phase'`, ...) that cannot collide with + * a lowercase/snake_case frontmatter key by construction, so checking its + * content argument would only add false positives on the ~20 already-safe + * `sectionBody`-scoped calls this axis must NOT report (mirrors + * `updateCore`'s strip-then-replace shape, and `beginPhaseCore`'s + * `stateReplaceField(body, name, value)`, both legitimately unflagged). + */ +function findUnstrippedContentWrites(rel, text) { + const rawLines = text.split('\n'); + const stripped = stripComments(text); + const out = []; + for (let i = 0; i < stripped.length; i++) { + const line = stripped[i]; + if (!line.trim()) continue; + STATE_REPLACE_FIELD_CALL_RE.lastIndex = 0; + let m; + while ((m = STATE_REPLACE_FIELD_CALL_RE.exec(line)) !== null) { + const contentArg = m[1]; + const fieldArg = m[2]; + if (isQuotedLiteralArg(fieldArg)) continue; + const assignment = nearestPrecedingAssignment(stripped, i - 1, contentArg); + const isStripped = assignment !== null && /^stripFrontmatter\s*\(/.test(assignment); + if (isStripped) continue; + // `file`/`source` sanitized for the same fork-PR reason as every other + // finding in this guard; `contentArg` is captured out of repo source + // (an identifier name), attacker-controlled on the same basis. + out.push({ + reason: REASON.UNSTRIPPED_CONTENT_WRITE, + axis: 'frontmatter-write', + file: sanitizeForReport(rel), + line: i + 1, + field: sanitizeForReport(contentArg), + source: sanitizeForReport(rawLines[i].trim()), + }); + } + } + return out; +} + +// The three write-seam functions, matched only as CALLS (`\(` immediately +// after, modulo whitespace) — never as bare mentions of the name. +// `applyPostSyncPreservation` (#3469) is included alongside +// `writeStateMd`/`syncStateFrontmatter`: after Phase 2, it is ONLY ever +// legitimately called from inside `syncAndPreserveStateMd` (the seam +// composition), so any OTHER call to it is either a re-assembly of the pair +// (Phase 2's Finding 3 shape — a call site invoking both +// `syncStateFrontmatter` and `applyPostSyncPreservation` itself instead of +// the composition) or a bypass calling it alone; either way it belongs on +// this axis. +const SEAM_CALL_RE = /\b(writeStateMd|syncStateFrontmatter|applyPostSyncPreservation)\s*\(/g; +// A line that IS one of the three seam functions' own definitions — skipped +// outright, never counted as a call to itself. +const SEAM_DEF_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+(?:writeStateMd|syncStateFrontmatter|applyPostSyncPreservation)\b/; + +/** + * AXIS 2a: every direct `writeStateMd(`/`syncStateFrontmatter(`/ + * `applyPostSyncPreservation(` call in `text`, outside the three functions' + * own definitions and (only inside `SEAM_OWNER_FILE`) outside + * `SEAM_OWNER_EXEMPT_FUNCTIONS`'s own bodies. No `reason` on these + * findings — `applyRatchet` assigns one, since the same observed call site + * is a different failure shape depending on whether the baseline already + * knows about it. */ function findSeamBypasses(rel, text) { const rawLines = text.split('\n'); @@ -654,11 +774,13 @@ function applyRatchet(observed, baseline) { } /** - * Run both scan passes (the `src/` tree for Axis 1 + Axis 2a, the prompt - * layer for Axis 2b) and split the combined findings by `axis` into + * Run both scan passes (the `src/` tree for Axis 1 + Axis 2a + Axis 3, the + * prompt layer for Axis 2b) and split the combined findings by `axis` into * `{ policyFindings, seamFindings }`. `policyFindings` are already terminal - * (each carries its own `reason`); `seamFindings` are raw observations — - * `applyRatchet` is what turns them into (or clears them of) a finding. + * (each carries its own `reason`) — this bucket is every axis EXCEPT + * `write-seam` (Axis 2), which alone is ratcheted; `seamFindings` are raw + * write-seam observations — `applyRatchet` is what turns them into (or + * clears them of) a finding. */ function collect() { const srcFindings = scanTree({ @@ -671,6 +793,7 @@ function collect() { if (relPosix === EXECUTOR_FILE) { found.push(...findPolicyDispatchDrift(relPosix, text)); found.push(...findUnimplementedPolicies(text, relPosix)); + found.push(...findUnstrippedContentWrites(relPosix, text)); } found.push(...findSeamBypasses(relPosix, text)); return found; @@ -688,7 +811,7 @@ function collect() { const all = [...srcFindings, ...promptFindings]; return { - policyFindings: all.filter((f) => f.axis === 'policy-dispatch'), + policyFindings: all.filter((f) => f.axis !== 'write-seam'), seamFindings: all.filter((f) => f.axis === 'write-seam'), }; } @@ -745,18 +868,23 @@ function buildBaselineEntries(seamFindings, existingEntries) { } const BASELINE_COMMENT = - 'ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1). Every entry here is a ' + - '`writeStateMd(`/`syncStateFrontmatter(` bypass this guard found by a whole-repo scan (Decision ' + - '4(a)) — it is ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the ' + - 'issue owning its removal recorded in the entry\'s "owner" field. This baseline is SHRINK-ONLY — ' + - 'an entry that stops firing goes STALE and fails the plain run until `--baseline` is re-run to ' + - 'drop it (ADR-3180 Decision 4(e)\'s "the baseline may only shrink", adopted verbatim by ADR-3408). ' + - 'Phase 2 (#3469) removes the `cmdPhaseComplete` and `patchCore` entries when it lands the single ' + - 'write seam. Phase 4 (#3471) drives this baseline to empty and deletes this file. ' + - '`REGENERATE_STATE` (`src/health-diagnostic.cts`) is a SANCTIONED PERMANENT exception, not debt — ' + - 'it is `/gsd-health --repair`\'s factory reset, which rebuilds STATE.md from scratch, so ' + - 'preservation would restore exactly the values it was invoked to discard; do not "consolidate" ' + - 'its entry away.'; + 'ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1; Phase 2 / #3469 lands the ' + + 'single write seam and Amendment 2). Every entry here is a `writeStateMd(`/`syncStateFrontmatter(`/' + + '`applyPostSyncPreservation(` bypass this guard found by a whole-repo scan (Decision 4(a)) — it is ' + + 'ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the issue owning its ' + + 'removal recorded in the entry\'s "owner" field. This baseline is SHRINK-ONLY — an entry that stops ' + + 'firing goes STALE and fails the plain run until `--baseline` is re-run to drop it (ADR-3180 ' + + 'Decision 4(e)\'s "the baseline may only shrink", adopted verbatim by ADR-3408). Phase 2 (#3469) ' + + 'removed the `cmdPhaseComplete` (`src/phase.cts`) and `cmdMilestoneComplete` (`src/milestone.cts`) ' + + 'entries by routing both through the single write-seam composition (`syncAndPreserveStateMd`, ' + + '`src/state.cts`). ADR-3408 Amendment 2: "0 bypasses" was never this baseline\'s target — TWO ' + + 'entries are SANCTIONED PERMANENT, not debt, and Phase 4 (#3471) does NOT drive this file to empty: ' + + '`cmdStateSync` (`src/state.cts`) exists precisely to let the body win (#905 — `state sync` ' + + 're-derives frontmatter FROM the body), so routing it through preservation would invert the command ' + + 'rather than fix a bug; `REGENERATE_STATE` (`src/health-diagnostic.cts`) is `/gsd-health --repair`\'s ' + + 'factory reset, which rebuilds STATE.md from scratch, so preservation would restore exactly the ' + + 'values it was invoked to discard. Neither entry may be "consolidated" away — a guard reporting them ' + + 'is reporting correctly, and a change that removes one is a regression, not progress.'; function writeBaseline(seamFindings) { const priorBaseline = loadBaseline(); @@ -902,6 +1030,9 @@ module.exports = { readPolicyUnion, findPolicyDispatchDrift, findUnimplementedPolicies, + findUnstrippedContentWrites, + isQuotedLiteralArg, + nearestPrecedingAssignment, findSeamBypasses, findPromptSeamUses, isInsideCodeSpan, diff --git a/scripts/state-write-path-drift-baseline.json b/scripts/state-write-path-drift-baseline.json index 4f6b4ae9d..fbfa9bb28 100644 --- a/scripts/state-write-path-drift-baseline.json +++ b/scripts/state-write-path-drift-baseline.json @@ -1,5 +1,5 @@ { - "_comment": "ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1). Every entry here is a `writeStateMd(`/`syncStateFrontmatter(` bypass this guard found by a whole-repo scan (Decision 4(a)) — it is ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the issue owning its removal recorded in the entry's \"owner\" field. This baseline is SHRINK-ONLY — an entry that stops firing goes STALE and fails the plain run until `--baseline` is re-run to drop it (ADR-3180 Decision 4(e)'s \"the baseline may only shrink\", adopted verbatim by ADR-3408). Phase 2 (#3469) removes the `cmdPhaseComplete` and `patchCore` entries when it lands the single write seam. Phase 4 (#3471) drives this baseline to empty and deletes this file. `REGENERATE_STATE` (`src/health-diagnostic.cts`) is a SANCTIONED PERMANENT exception, not debt — it is `/gsd-health --repair`'s factory reset, which rebuilds STATE.md from scratch, so preservation would restore exactly the values it was invoked to discard; do not \"consolidate\" its entry away.", + "_comment": "ADR-3408 Decision 5 write-seam ratchet baseline (issue #3468, Phase 1; Phase 2 / #3469 lands the single write seam and Amendment 2). Every entry here is a `writeStateMd(`/`syncStateFrontmatter(`/`applyPostSyncPreservation(` bypass this guard found by a whole-repo scan (Decision 4(a)) — it is ACKNOWLEDGED, not endorsed: acknowledgment is in writing (this file), with the issue owning its removal recorded in the entry's \"owner\" field. This baseline is SHRINK-ONLY — an entry that stops firing goes STALE and fails the plain run until `--baseline` is re-run to drop it (ADR-3180 Decision 4(e)'s \"the baseline may only shrink\", adopted verbatim by ADR-3408). Phase 2 (#3469) removed the `cmdPhaseComplete` (`src/phase.cts`) and `cmdMilestoneComplete` (`src/milestone.cts`) entries by routing both through the single write-seam composition (`syncAndPreserveStateMd`, `src/state.cts`). ADR-3408 Amendment 2: \"0 bypasses\" was never this baseline's target — TWO entries are SANCTIONED PERMANENT, not debt, and Phase 4 (#3471) does NOT drive this file to empty: `cmdStateSync` (`src/state.cts`) exists precisely to let the body win (#905 — `state sync` re-derives frontmatter FROM the body), so routing it through preservation would invert the command rather than fix a bug; `REGENERATE_STATE` (`src/health-diagnostic.cts`) is `/gsd-health --repair`'s factory reset, which rebuilds STATE.md from scratch, so preservation would restore exactly the values it was invoked to discard. Neither entry may be \"consolidated\" away — a guard reporting them is reporting correctly, and a change that removes one is a regression, not progress.", "entries": [ { "file": "src/health-diagnostic.cts", @@ -8,26 +8,12 @@ "count": 1, "owner": "sanctioned-permanent" }, - { - "file": "src/milestone.cts", - "source": "writeStateMd(statePath, result.content, cwd);", - "symbol": "writeStateMd", - "count": 1, - "owner": "#3471" - }, - { - "file": "src/phase.cts", - "source": "const synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm);", - "symbol": "syncStateFrontmatter", - "count": 1, - "owner": "#3469" - }, { "file": "src/state.cts", "source": "writeStateMd(statePath, modified, cwd);", "symbol": "writeStateMd", "count": 1, - "owner": "#3471" + "owner": "sanctioned-permanent" } ] } diff --git a/src/milestone.cts b/src/milestone.cts index e80fb2c1a..f1bdd6174 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -14,7 +14,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- state.cjs is an export= CommonJS module import stateMod = require('./state.cjs'); -import { platformWriteSync, platformEnsureDir, execGit, retryRenameSync } from './shell-command-projection.cjs'; +import { platformWriteSync, platformReadSync, platformEnsureDir, execGit, retryRenameSync } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; import { transitionCore } from './state-transition.cjs'; @@ -50,7 +50,13 @@ import phaseLocatorMod = require('./phase-locator.cjs'); const { listMilestonePhaseDirs } = phaseLocatorMod; const { planningPaths } = planningWorkspace; const { extractFrontmatter } = frontmatterMod; -const { writeStateMd } = stateMod; +// ADR-3408 §8.3 / #3469: `writeStateMd` gets sync and NO preservation — the +// same #3374-shaped exposure the milestone-complete write used to carry (a +// stale body value silently clobbering fresher frontmatter, with no +// divergence signal). Routed through the single write-seam composition +// (`syncAndPreserveStateMd`) instead, under `withStateLock` — see +// `cmdMilestoneComplete`'s own STATE.md-update block for the full rationale. +const { syncAndPreserveStateMd, withStateLock } = stateMod; // #2288 security: a milestone version label becomes a filesystem directory // component (`milestones/