From 1863f5569c9b1c90793e675f6962b95cd991b434 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 25 Aug 2026 20:38:12 -0400 Subject: [PATCH] =?UTF-8?q?enhance(#3871):=20the=20state=20transaction=20?= =?UTF-8?q?=E2=80=94=20mandatory=20snapshot,=20open()/rebuild()=20(#3874)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3871): failing-first regressions for the dropped curated progress block Pins ADR-3473 §8.6 / #3756 at the consumer's output: state record-session and state add-decision on an archived-milestone project drop the curated progress frontmatter entirely, exit 0, and report nothing. Reproduced against the real CLI before writing the tests, not inferred from the issue text. Also adds the unit-level probe that applyStatePreservation's preserve-always row is inert on a resyncing write, and an over-preservation guard that an empty project is never inflated. Refs #3871 * feat(#3871): make the STATE.md pre-write snapshot mandatory via open()/rebuild() ADR-3473 §8.6. StatePreservationInput's nullable preFm and the always-present preFmSnapshot were the same extractFrontmatter call, one of them nulled on resync — a policy flag baked into a snapshot. Both collapse into a single StateTransaction whose snapshot cannot be absent: openStateTransaction() applies preservation, rebuildStateTransaction() does not, and both carry the snapshot because the reporting phase needs it either way. An absent snapshot is now a construction failure; an empty one stays legal, because that is what a document with no parseable frontmatter honestly has. writeStateMd requires a rebuild transaction, which types ADR-3408 §8.3's closed exception list at both call sites (state sync, health --repair) instead of matching them as strings in a ratcheted baseline. Fixes the dropped curated progress block: an all-zero or absent derived total set is an unmeasured scan, not a measurement, so the curated block stands. Also fixes two defects surfaced while building — preserve-always reported a mutation even when it restored an identical value, and it re-entered the curated object by reference, which would alias the snapshot the next phase diffs against. Refs #3871 * fix(#3871): close the three remaining subsumed defects and restore the arm the type does not replace Review of the first two commits found four things. The guard shrink deleted the seam-bypass axis whole, but only its writeStateMd( arm became redundant. Its other arm catches a call site re-assembling syncStateFrontmatter + applyPostSyncPreservation instead of the owned composition, which the transaction type does not make unrepresentable and which #3469 found live. Restored as findCompositionBypasses, terminal rather than ratcheted. Three of the four issues this phase claims were untouched. All three are the epic's own shape and are fixed at the seam: current_phase_name is reasserted from the curated value when the caller names none, and cmdStateJson stops carrying a hand-maintained list parallel to FIELD_CLASSIFICATION and projects it instead. The construction failure that is the point of this phase had no test. Every enumerated matrix row now has one, including the measured-versus-unmeasured coercion boundary and a seeded property that no curated key is ever dropped. ADR-3473 §8.6 said the guard 'keeps only its raw-write check'. Verified against next: there was no raw-write check, and four other checks it does not name. Amended in place with the evidence. ARCHITECTURE.md separately advertised a preservation policy the code had deleted. Refs #3871 * fix(#3871): do not let the unmeasured-scan rule block an explicitly-requested resync The remote matrix caught over-preservation, the failure this phase's own negative space says must not happen. state update Progress re-derives the block from the body the caller just rewrote; on a project with no phase dirs the derivation yields zero totals, the unmeasured rule read that as 'the scan measured nothing', and the stale curated percent was restored over the resync the user asked for. preserve-always already said what the missing condition was: never overwrite unless the caller explicitly names this field. explicitProgressField carries it and is derived from shouldResyncStateProgress, not set by hand at a call site, so it cannot drift from what the caller asked for. Two defects found in the same mechanism and fixed with it. readModifyWriteStateMd enumerates its option keys, so a new option was silently dropped rather than rejected. And the raw-write axis captured its first argument up to the first comma, which lands inside a nested path.join, so a write to a STATE.md literal was invisible to it — the prove-it-can-fail test caught that one immediately. No test assertion was weakened; all three frontmatter rows encode #3242, #1969 B3 and #1972 and stand unchanged. Refs #3871 * docs(#3871): record why the raw-write check is kept, not why it was named The amendment justified findRawStateWrites as 'written because §8.6 requires it to exist', which is cargo-culting the contract and would have been the wrong reason to keep anything. The real reason is that writeStateMd acquires the STATE.md lockfile and a raw fs.writeFileSync acquires nothing, so this is a lock bypass and lost-update is the #500/#905/#1230 family — and after this phase it is the one reachable path into the file that nothing else covers. Also records why ADR-3408 §8.6's deletion of the 'clear' policy is not the precedent it looks like: 'clear' was dead vocabulary in a closed enum, this is coverage of a reachable path. Refs #3871 * chore(#3871): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/sunny-foxes-click.md | 5 + CONTEXT.md | 4 +- docs/ARCHITECTURE.md | 6 +- docs/adr/3473-enforcement-by-construction.md | 14 +- scripts/lint-state-write-path-drift.cjs | 769 +++++++-------- scripts/state-write-path-drift-baseline.json | 19 - src/health-diagnostic.cts | 16 +- src/state-document.cts | 11 +- src/state-transition.cts | 329 +++++-- src/state.cts | 140 ++- .../m8-writestatemd-scan-after-lock.test.cjs | 15 +- .../perf-316-state-lock-buffer-alloc.test.cjs | 24 +- tests/state-transition.test.cjs | 918 +++++++++++++----- tests/state-write-path-drift-guard.test.cjs | 703 ++++++-------- tests/state.test.cjs | 630 +++++++++++- 15 files changed, 2429 insertions(+), 1174 deletions(-) create mode 100644 .changeset/sunny-foxes-click.md delete mode 100644 scripts/state-write-path-drift-baseline.json diff --git a/.changeset/sunny-foxes-click.md b/.changeset/sunny-foxes-click.md new file mode 100644 index 000000000..413dacbef --- /dev/null +++ b/.changeset/sunny-foxes-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3874 +--- +**Curated STATE.md content survives writes that measured nothing** — `state record-session`, `state add-decision` and the other resyncing verbs no longer drop a curated `progress:` block once a milestone's phases have been archived, `state planned-phase` without `--name` no longer overwrites `current_phase_name` with a placeholder, `state complete-phase` no longer deletes that key while reporting it as updated, and `state json` no longer serves `last_activity_desc` from stale body prose. (#3871) diff --git a/CONTEXT.md b/CONTEXT.md index 8858cb3c8..f60a7377c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -62,10 +62,10 @@ Adapter Module that satisfies native query dispatch at the Dispatch Policy seam, Module owning projection from dispatch results/errors to CLI `{ exitCode, stdoutChunks, stderrLines }` output contract. ### STATE.md Document Module -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. `isRealCalendarDate` (#3696) is the single owner of "does this y/m/d exist on the calendar" — moved here from a private copy in `smart-entry.cts` so that reader and `state validate`'s S008 cannot drift into disagreeing about whether a STATE.md is usable; it enforces ADR-227's shape-AND-value rule, rejecting `2026-02-30` rather than letting `Date.parse` roll it forward to `2026-03-02`. `stateFieldContinuation` (#3696) reports the prose a wrapped single-line field leaves behind, which `stateExtractField`'s newline-excluding `(.+)` silently drops (S009); it is additive beside `stateExtractField` rather than a widening of it, because ADR-3180 §7.7 Rejected #1 pins that function as untouchable (20 direct callers, CRITICAL blast radius) and a continuation join there would apply to every field — `Status:` would swallow the line beneath it. 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. +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. `isRealCalendarDate` (#3696) is the single owner of "does this y/m/d exist on the calendar" — moved here from a private copy in `smart-entry.cts` so that reader and `state validate`'s S008 cannot drift into disagreeing about whether a STATE.md is usable; it enforces ADR-227's shape-AND-value rule, rejecting `2026-02-30` rather than letting `Date.parse` roll it forward to `2026-03-02`. `stateFieldContinuation` (#3696) reports the prose a wrapped single-line field leaves behind, which `stateExtractField`'s newline-excluding `(.+)` silently drops (S009); it is additive beside `stateExtractField` rather than a widening of it, because ADR-3180 §7.7 Rejected #1 pins that function as untouchable (20 direct callers, CRITICAL blast radius) and a continuation join there would apply to every field — `Status:` would swallow the line beneath it. 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. `toFiniteNumber` (#3871) is now EXPORTED as the single owner of "coerce this frontmatter scalar to a finite number, or say it is not one" — the STATE.md Transition Module's progress-ratchet consults it to decide whether a derived total is a real measurement, and a private second copy there would have diverged immediately, because frontmatter scalars arrive as STRINGS (`"0"`, not `0`) and a `=== 0` test is wrong at both call sites. ### 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). `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. **Phase 4 (#3471):** `syncStateFrontmatter`'s six empty-only guards (D1) are now GATED — active only for the §8.3 sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`), which have no preservation executor downstream and for which body-beats-frontmatter is the deliberate contract; OFF on the write seam, where `applyStatePreservation` owns the empty case via the exported `applyPreserveWhenUnchanged` executor. `reconcileReportedFields` (`state.cts`, §8.4/D4) is the single owner of reconciling a command's reported `updated`/`failed` field array against what was actually persisted, closing both #3351's (reported-but-discarded) and #3345's (persisted-but-unreported) directions; used by seven commands rather than seven re-derivations. `cmdStateJson` (§8.5/D3) no longer carries a private copy of the empty-only guards; it routes through `applyPreserveWhenUnchanged` directly, scoped to the same six fields. This entry has now needed correcting three times inside this one epic; Phase 1's whole-repo drift guard, not this line, remains the authoritative count. +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. **Phase 4 (#3471):** `syncStateFrontmatter`'s six empty-only guards (D1) are now GATED — active only for the §8.3 sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`), which have no preservation executor downstream and for which body-beats-frontmatter is the deliberate contract; OFF on the write seam, where `applyStatePreservation` owns the empty case via the exported `applyPreserveWhenUnchanged` executor. `reconcileReportedFields` (`state.cts`, §8.4/D4) is the single owner of reconciling a command's reported `updated`/`failed` field array against what was actually persisted, closing both #3351's (reported-but-discarded) and #3345's (persisted-but-unreported) directions; used by seven commands rather than seven re-derivations. `cmdStateJson` (§8.5/D3) no longer carries a private copy of the empty-only guards; it routes through `applyPreserveWhenUnchanged` directly, scoped to the same six fields. This entry has now needed correcting three times inside this one epic; Phase 1's whole-repo drift guard, not this line, remains the authoritative count. **ADR-3473 §8.6 (#3871) — the state transaction.** `StateTransaction` is the Module's pre-write policy value, constructed only by `openStateTransaction` (preservation applies) or `rebuildStateTransaction` (it does not); both carry a MANDATORY `snapshot`, and an absent one is a **construction failure** (`STATE_TRANSACTION_SNAPSHOT_REQUIRED`), never a runtime no-op. This replaces the `preFm`/`preFmSnapshot` pair, which were the same `extractFrontmatter` call with one copy nulled on `resync` — a policy flag encoded as a missing input, which is why a declared `preserve-always` row silently skipped on the default write path (#3756). An EMPTY snapshot (`{}`) stays legal: that is the honest snapshot of a document with no parseable frontmatter, and `/gsd-health --repair` runs precisely on such documents. `rebuildStateTransaction` is the typed expression of ADR-3408 §8.3's closed exception list — `cmdStateSync` (#905) and `REGENERATE_STATE` — so `writeStateMd` takes a `rebuild` transaction and rejects anything else, and the two `sanctioned-permanent` entries the write-path drift guard used to ratchet as strings are retired with their baseline file. Within `applyPreserveAlways`, an all-zero or absent set of derived `progress` TOTALS is an **unmeasured scan, not a measurement** (the convention #3233 established, and why `computeProgressPercent` already returns `null` on an empty denominator), so the curated block stands rather than being overwritten with zeros; `completed_*` being zero is normal and does not decide it, and a measured scan still corrects totals downward (#1446/#2440). The rule carries a second, non-negotiable condition: it does **not** fire when the caller NAMED a progress field, because `preserve-always` has always meant "never overwrite unless the caller explicitly names this field" and `state update Progress` exists precisely to re-derive the block from the body just rewritten. `explicitProgressField` carries that and is **derived, never hand-set** — it comes from `shouldResyncStateProgress(fields)` (true iff the field set contains `Progress`, `Total Plans in Phase` or `Total Phases`), so it cannot drift from what the caller actually asked for. Omitting it silently discarded an explicitly-requested resync (#3242 / #1972), caught by the remote matrix, not by review. `getPreserveWhenUnchangedFields()` projects the `preserve-when-unchanged` rows out of `FIELD_CLASSIFICATION` so `cmdStateJson` consults the declaration instead of the hand-maintained parallel list that had drifted from it (#3836). ### 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/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3202a7361..c0c823808 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -551,7 +551,11 @@ Locking decides *who* writes. A separate contract decides *what survives the wri STATE.md carries the same fact in two places — YAML frontmatter and the document body — and the body is authoritative. Every write therefore re-derives frontmatter from the body, which raises the question the write path exists to answer: when a re-derived value disagrees with the one already in frontmatter, which wins? -`FIELD_CLASSIFICATION` (`src/state-transition.cts`) answers it per field, declaring a `preservation` policy — `preserve-when-unchanged`, `preserve-always`, `preserve-if-placeholder`, `derive`, `clear` — that `applyStatePreservation` executes after `syncStateFrontmatter` re-derives. +`FIELD_CLASSIFICATION` (`src/state-transition.cts`) answers it per field, declaring a `preservation` policy — `preserve-when-unchanged`, `preserve-always`, `preserve-if-placeholder`, `derive` — that `applyStatePreservation` executes after `syncStateFrontmatter` re-derives. (A fifth policy, `clear`, was listed here until ADR-3408 §8.6's amendment removed it: no row used it and no executor existed for it.) + +**The pipeline's precondition is a type, not a convention (ADR-3473 §8.6).** A policy row can only be honored if the pre-write frontmatter snapshot it compares against is actually present. That snapshot now travels as a `StateTransaction`, built by `openStateTransaction()` — preservation applies — or `rebuildStateTransaction()` — it does not. Both carry the snapshot, and a transaction cannot be constructed without one: an absent snapshot is a *construction failure*, not a runtime skip. That distinction is the whole point. Previously the snapshot was nulled to signal "re-derive from disk", so a declared `preserve-always` row and a silently-skipped one were indistinguishable at runtime, which is how a curated `progress:` block was erased by verbs that had nothing to do with progress. + +`rebuildStateTransaction()` is the typed form of ADR-3408 §8.3's closed exception list: `state sync`, which exists to let the body win, and `/gsd-health --repair`'s factory reset. Both are deliberate and permanent, not debt — and because the type names them, the write-path drift guard no longer has to track them as strings in a ratcheted baseline. **[ADR-3408](adr/3408-state-write-path-preservation.md) is the normative contract** for that path: one executor per declared policy, one write seam, and reports computed from what was actually persisted rather than from what the caller intended to write. Where the contract and the code disagree, the code is the defect. It is the write-side counterpart of [ADR-3180](adr/3180-planning-semantic-model-single-owner.md), which gave each read-side derivation a single owner. diff --git a/docs/adr/3473-enforcement-by-construction.md b/docs/adr/3473-enforcement-by-construction.md index f28d1eb8e..33cb1bf49 100644 --- a/docs/adr/3473-enforcement-by-construction.md +++ b/docs/adr/3473-enforcement-by-construction.md @@ -48,7 +48,7 @@ Two `/m`-anchored patterns in the same module use `\s` at a `^` position. Per EC This family is the reason Phases 1–3 exist and is recorded here rather than inferred later. -ADR-3408 shipped completely: all five phases closed, `scripts/lint-state-write-path-drift.cjs` is live, and `scripts/state-write-path-drift-baseline.json` stands at **two entries, both `owner: sanctioned-permanent`** — zero debt. The seam is real and it held. +ADR-3408 shipped completely: all five phases closed, `scripts/lint-state-write-path-drift.cjs` is live, and its write-path drift baseline stood at **two entries, both `owner: sanctioned-permanent`** — zero debt. The seam is real and it held. *(That baseline was retired, file and all, by Phase 1 — see §8.6's amendment. This paragraph records the state at filing.)* Eleven state defects were nonetheless filed between 2026-08-21 and 2026-08-25 (#3853, #3836, #3835, #3834, #3830, #3818, #3812, #3807, #3784, #3756, #3743). **None is a bypass of that seam.** They sit at four places it does not reach: @@ -200,7 +200,15 @@ Both close #3349 and #3360, which are **read-side** defects a real parser fixes **Rule — `rebuild()` is the typed expression of the sanctioned exceptions.** ADR-3408 §8.3's closed list of commands whose contract is to let the body win — `cmdStateSync` (#905: `state sync` re-derives frontmatter *from* the body) and `/gsd-health --repair`'s `REGENERATE_STATE` (a factory reset) — call `rebuild()`. They are **not** debt: a guard reporting them is reporting correctly, and a change that removes one is a regression. Adding to the list is an amendment to ADR-3408 §8.3. -**Consequence for the guard.** `scripts/state-write-path-drift-baseline.json`'s two `sanctioned-permanent` entries are retired: the exception becomes a constructor the type system names, not a ratcheted string match. `scripts/lint-state-write-path-drift.cjs` keeps only its raw-write check (`fs.writeFileSync` against the state path), which the type cannot make unrepresentable. +**Consequence for the guard.** The write-path drift baseline's two `sanctioned-permanent` entries are retired, and the baseline file with them: the exception becomes a constructor the type system names, not a ratcheted string match. `scripts/lint-state-write-path-drift.cjs` keeps every check the type does **not** make unrepresentable. + +> **Amendment, 2026-08-25 (Phase 1, #3871) — this paragraph originally read "keeps only its raw-write check (`fs.writeFileSync` against the state path)", and that sentence was wrong on both halves.** Verified against `next` while implementing: the guard contained **no raw-write check at all**, and it contained **four** checks besides the one §8.6 retires — policy-dispatch drift, unimplemented policies, unstripped content writes, and prompt-layer state writes. None of those is named by §8.6 and none is made unrepresentable by the transaction type, so all four are retained; the raw-write check is **net-new**. + +> It is kept — rather than dropped along with the sentence that mis-described it — because of what it covers, not because §8.6 named it. `writeStateMd` acquires the STATE.md lockfile before it writes; a raw `fs.writeFileSync` against the state path acquires nothing. That is not a preservation bypass, it is a **lock** bypass, and lost-update is the #500/#905/#1230 family. Every other route into the file is now closed by construction — an `open()` transaction preserves, a `rebuild()` transaction is the typed exception, and a re-assembled composition is caught by the axis above — so the raw write is the one remaining reachable path that nothing covers. Zero occurrences to date is not the test; reachability and blast radius are. +> +> The tempting counter-precedent does **not** apply. ADR-3408 §8.6's amendment deleted the `clear` preservation policy when it turned out no row used it and no executor existed — contract names X, X does not exist, delete the naming. `clear` was dead *vocabulary* in a closed enum: deleting it removed a way to express something meaningless. This is coverage of a *reachable path*. The two look alike and are not the same shape. +> +> The retired axis was the **seam-bypass** scan, and only half of it was redundant. Its `writeStateMd(` arm is genuinely replaced by the type and is gone with its ratchet. Its **composition-bypass** arm — a new call site re-assembling `syncStateFrontmatter` + `applyPostSyncPreservation` instead of routing through the owned composition, which is ADR-3408 §8.3's rule and the exact shape #3469 found live in `cmdPhaseComplete` — is **not** replaced by the type, which gates one parameter of one function and nothing more. It is retained, made terminal rather than ratcheted, and carries its own reason code. Decision 6 sanctions retiring a guard the change makes **redundant**; deleting this arm would have been a silent coverage regression dressed as a guard-count win, which is precisely the Goodhart outcome Decision 6 exists to prevent. #### 8.7 What a command reports it wrote — *Required — Phase 2* @@ -288,7 +296,7 @@ Net across the set: one guard retired, one increase recorded honestly. The incre | Guard | Status under this ADR | |---|---| -| `scripts/lint-state-write-path-drift.cjs` | retained, shrunk (§8.6) | +| `scripts/lint-state-write-path-drift.cjs` | retained, shrunk (§8.6) — seam-bypass `writeStateMd(` arm and its ratchet retired at Phase 1; composition-bypass arm retained and made terminal; raw-write check added net-new. See §8.6's amendment. | | `scripts/lint-state-field-drift.cjs` | **retired at Phase 3** (§8.8) | | `scripts/lint-vendored-deps.cjs` | reused as-is for §8.1's vendoring rule | | `local/no-external-require-in-bin` | reused as-is; enforces §8.1's packaging rule | diff --git a/scripts/lint-state-write-path-drift.cjs b/scripts/lint-state-write-path-drift.cjs index 7999f8341..868909d1a 100644 --- a/scripts/lint-state-write-path-drift.cjs +++ b/scripts/lint-state-write-path-drift.cjs @@ -5,12 +5,45 @@ * Anti-divergence drift guard for the STATE.md WRITE PATH — epic #3408, issue * #3468, ADR-3408 Decision 5, contract §8.1/§8.2/§8.3 * (`docs/adr/3408-state-write-path-preservation.md` is the contract this - * guard enforces; read it first). + * guard enforces; read it first) — SHRUNK per ADR-3473 §8.6 (issue #3871). * - * TWO AXES, ONE GUARD, because they fail together — a table that is - * bypassed on dispatch and a seam that is bypassed on write are the same - * failure mode ("policy declared, enforcement hand-rolled") applied to two - * different call shapes: + * WHAT MOVED INTO THE TYPE SYSTEM (ADR-3473 §8.6): `writeStateMd`'s third + * parameter now REQUIRES a `StateTransaction` of `kind: 'rebuild'`, produced + * only by `openStateTransaction()` / `rebuildStateTransaction()` + * (`src/state-transition.cts`) — `writeStateMd` itself throws + * `STATE_TRANSACTION_KIND_INVALID` for anything else. The two exceptions this + * guard used to track as a RATCHETED STRING MATCH against + * `scripts/state-write-path-drift-baseline.json` — `cmdStateSync` + * (`src/state.cts`, #905's "let the body win") and `REGENERATE_STATE` + * (`src/health-diagnostic.cts`, `/gsd-health --repair`'s factory reset) — are + * NO LONGER TRACKED HERE. Both are now a constructor the type system names + * (`rebuildStateTransaction`), not an entry a human had to remember to keep + * acknowledging in a baseline file. That baseline file, and the whole + * ratchet machinery that existed ONLY to support it (loading it, the + * STALE-entry check, `--baseline` regeneration), is retired along with it — + * the `writeStateMd(` ARM of the old `findSeamBypasses` axis is gone for + * good, because the type system now names both of its exceptions. + * + * WHAT STAYED, RETITLED, AND MADE TERMINAL (issue #3871 review): the OTHER + * half of `findSeamBypasses` — every direct `syncStateFrontmatter(` / + * `applyPostSyncPreservation(` call outside their owner + * (`syncAndPreserveStateMd`, `src/state.cts`) — is NOT redundant. The type + * system gates `writeStateMd`'s THIRD PARAMETER; it says nothing about a call + * site that re-assembles `syncStateFrontmatter` + `applyPostSyncPreservation` + * itself instead of calling the one write-seam composition. ADR-3408 §8.3: + * "Assembling the stages at a call site is a re-derivation even when every + * step calls the owner." #3469 found exactly that shape live in + * `cmdPhaseComplete`'s atomic-commit adapter (`src/phase.cts`) — every step + * called an owner, so an owner-level test and this guard's OLD, narrower + * scan both stayed green while the composition itself drifted from + * `readModifyWriteStateMd`'s. See `findCompositionBypasses` below. Unlike + * the retired `writeStateMd(` arm, this one ships TERMINAL, not ratcheted — + * mirrors `findPromptSeamUses`'s own conversion in this same shrink: no + * legitimate call site outside the owner exists today, so any occurrence is + * a violation, not an entry to acknowledge. + * + * WHAT THIS GUARD STILL OWNS, because the type system cannot make it + * unrepresentable: * * AXIS 1 — POLICY DISPATCH (§8.1). `applyStatePreservation` must select * its branch from a `FIELD_CLASSIFICATION` row's `preservation` value, @@ -27,12 +60,39 @@ * undetected by the call-shape check alone). Scoped to the identifier * `field` only; see `FIELD_VAR_EQ_LITERAL_RE`'s own comment for why. * - * AXIS 2 — WRITE SEAM (§8.3), RATCHETED. `readModifyWriteStateMd` is the - * only path meant to write STATE.md. Every direct `writeStateMd(` or - * `syncStateFrontmatter(` call outside the owner's own definitions is a - * bypass that skips preservation and the #948 no-op guard — how #3374 and - * #3350 reproduce. This axis ships RATCHETED (see below), never a bare - * 0-or-fail check. + * AXIS 2 — RAW STATE WRITE (§8.6, RETAINED). A direct `fs.writeFileSync(` + * call whose target argument is the state path (`statePath`, or a literal + * containing `STATE.md`) is a write that skips BOTH the write seam + * (`writeStateMd`/`syncAndPreserveStateMd`) AND the OS Shell Projection + * seam (`platformWriteSync`, `src/shell-command-projection.cts`) entirely. + * No constructor or type can make this unrepresentable — Node's `fs` + * module is always one `import` away — so this axis stays a plain + * string-match scan, unratcheted: any occurrence is a violation, because no + * legitimate call site in this codebase writes STATE.md this way (every + * real writer goes through `platformWriteSync`). + * + * AXIS 3 — FRONTMATTER-SHAPED WRITE (§8.3(b), closed Phase 2 / #3469). + * `findUnstrippedContentWrites` below flags a `stateReplaceField(` call + * only when BOTH (a) its field-name argument is a VARIABLE, not a fixed + * string literal, and (b) its content argument has not been run through + * `stripFrontmatter` first (a narrow backward-scan approximation, not full + * dataflow — see the function's own docstring). + * + * AXIS 4 — PROMPT-LAYER WRITE (§8.3, Decision 4(d)). Prose in the prompt + * layer instructing an agent to shell out to a write-side `gsd-tools` + * subcommand is the same write seam, expressed as markdown rather than + * TypeScript — a check the type system cannot reach at all, since markdown + * is never compiled. Any occurrence is a violation. + * + * AXIS 5 — COMPOSITION BYPASS (§8.3, RETAINED, issue #3871). A direct + * `syncStateFrontmatter(` or `applyPostSyncPreservation(` call outside + * their owner (`syncAndPreserveStateMd`) is a re-assembly of the write-seam + * composition — the exact shape #3469 found live in `cmdPhaseComplete`. + * `writeStateMd(` is deliberately NOT scanned here (that arm is retired, + * §8.6) — `writeStateMd`'s own legitimate direct `syncStateFrontmatter(` + * call (the sanctioned #905 exception) is instead exempted by function + * name, same as the composition owner itself; see + * `SEAM_OWNER_EXEMPT_FUNCTIONS`. Terminal: any occurrence is a violation. * * DESIGN CONSTRAINTS (ADR-3180 Decision 4, adopted verbatim by ADR-3408): * - 4(a) whole-repo scan, never an allowlist. ADR-3180's own phases found @@ -40,15 +100,9 @@ * nothing. * - 4(d) the scan surface is DECLARED and is NOT just `src/` — `src/` * alone is itself an allowlist one directory wide; #1762 traced a wrong - * count to a shell snippet in `gsd-core/workflows/progress.md`. And - * inward: the OWNER FILE (`src/state.cts`) is not exempt, only its - * named canonical FUNCTIONS are (`SEAM_OWNER_EXEMPT_FUNCTIONS` below). - * - 4(e) Axis 2 ships ratcheted because Phase 1 (this file) cannot - * consolidate the write seam — that is Phase 2 (#3469). Landing a - * guard later, against an already-clean tree, is the "found it, wrote - * it down, moved on" posture the epic removes. + * count to a shell snippet in `gsd-core/workflows/progress.md`. * - * GOODHART, PER ADR-3408 DECISION 5: "0 bypasses" is a LAGGING metric — a + * GOODHART, PER ADR-3408 DECISION 5: "0 violations" is a LAGGING metric — a * measure about to become a target. This guard's own `_comment` and its * human-readable success message both say so: the zero this guard reports * must NEVER be quoted alone; it is only meaningful beside the behavioral @@ -62,42 +116,37 @@ * to prevent. `stripComments` does not track quoted strings for exactly * this reason — see its own header. * - * 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. - * - * 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. + * CORRECTION TO ADR-3473 §8.6's TEXT, RECORDED HERE SO A FUTURE READER + * COMPARING THE ADR TO THIS FILE DOES NOT CONCLUDE THE FILE DRIFTED: + * §8.6 says this guard "keeps only its raw-write check (`fs.writeFileSync` + * against the state path), which the type cannot make unrepresentable." That + * sentence is wrong on both halves. First, no such check existed anywhere in + * this file before this shrink — `findRawStateWrites` (Axis 2, below) is + * NET-NEW, written for this shrink, not retained from a prior version. + * Second, this file did not (and does not) drop to "only" one check: besides + * the retired `writeStateMd(` arm of the old `findSeamBypasses` axis (the + * half §8.6 correctly names for removal, since the type system now names + * both of its exceptions), `findPolicyDispatchDrift`, `findUnimplementedPolicies`, + * `findUnstrippedContentWrites`, `findPromptSeamUses`, and the RETAINED + * `syncStateFrontmatter`/`applyPostSyncPreservation` composition-bypass half + * of `findSeamBypasses` (now `findCompositionBypasses`, terminal — issue + * #3871 review) all remain, because §8.6 names neither them nor anything + * that makes what they check unrepresentable — a field-name-keyed dispatch + * branch, an unimplemented `FieldPreservation` policy, an unstripped + * frontmatter write, prompt-layer prose shelling out to `gsd-tools`, and a + * re-assembled write-seam composition are all still exactly as representable + * in TypeScript (or in markdown, for the prompt-layer one) after the + * state-transaction constructor as they were before it — the constructor + * gates `writeStateMd`'s third parameter, nothing about a call site that + * never goes through `writeStateMd` at all. Do not edit the ADR to match + * this file; this paragraph is the correction of record. */ -const fs = require('node:fs'); const path = require('node:path'); const { scanTree, sanitizeForReport } = require('./lib/drift-scan.cjs'); const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); -const BASELINE_PATH = path.join(__dirname, 'state-write-path-drift-baseline.json'); // Frozen REASON enum — mirrors `lint-state-field-drift.cjs`'s house style of // naming every failure shape explicitly rather than reusing one generic @@ -110,44 +159,52 @@ const REASON = Object.freeze({ // 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', - BASELINE_ENTRY_STALE: 'baseline_entry_stale', - BASELINE_UNREADABLE: 'baseline_unreadable', + // Axis 2 (§8.6, retained): a raw `fs.writeFileSync(` call targeting the + // state path — see `findRawStateWrites`. + RAW_STATE_WRITE: 'raw_state_write', + // Axis 4 (§8.3, Decision 4(d)): prompt-layer prose shelling out to a + // write-side `gsd-tools` subcommand — see `findPromptSeamUses`. + PROMPT_LAYER_STATE_WRITE: 'prompt_layer_state_write', + // Axis 5 (§8.3, RETAINED, issue #3871): a direct `syncStateFrontmatter(` or + // `applyPostSyncPreservation(` call outside their owner + // (`syncAndPreserveStateMd`) — see `findCompositionBypasses`. + COMPOSITION_BYPASS: 'composition_bypass', }); // Scan surface — declared, per Decision 4(d), never inferred from `src/` -// alone. `src/` covers the executor and the write-seam owner; the prompt -// layer covers markdown that can shell out to `state.patch` / `phase.complete` -// and post-process the result outside any TypeScript this guard could see. +// alone. `src/` covers the executor; the prompt layer covers markdown that +// can shell out to `state.patch` / `phase.complete` and post-process the +// result outside any TypeScript this guard could see. const SRC_DIRS = ['src']; const SRC_EXT = new Set(['.cts']); const PROMPT_DIRS = ['gsd-core/workflows', 'commands', 'agents', 'skills']; const PROMPT_EXT = new Set(['.md']); -// The executor (Axis 1) and the write-seam owner (Axis 2). Forward-slash -// literals: every `rel` this guard compares against them is unconditionally -// POSIX-normalized first (`toPosixRel` below) — never gated on -// `process.platform`. +// The executor (Axis 1 / Axis 3). Forward-slash literal: every `rel` this +// guard compares against it is unconditionally POSIX-normalized first +// (`toPosixRel` below) — never gated on `process.platform`. const EXECUTOR_FILE = 'src/state-transition.cts'; + +// The write-seam composition owner (Axis 5). Forward-slash literal, same +// POSIX-normalization rule as `EXECUTOR_FILE` above. 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(`/ -// `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 +// FUNCTIONS are": a `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; ADR-3473 §8.6 gates ITS third parameter, which is +// an orthogonal, type-level check — this guard's exemption is about which +// FUNCTION BODY a raw call to the two seam stages is allowed to live in). +// `syncAndPreserveStateMd` is the ONE write-seam composition — every OTHER +// caller needing a non-standard I/O envelope routes through it. 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: it calls `syncAndPreserveStateMd` like everyone else, so if a +// direct `syncStateFrontmatter(`/`applyPostSyncPreservation(` call +// reappeared there it would be exactly the re-assembly shape this axis // exists to catch. const SEAM_OWNER_EXEMPT_FUNCTIONS = ['writeStateMd', 'syncAndPreserveStateMd']; @@ -211,8 +268,9 @@ function stripComments(text) { } // A named function declaration, tolerating `export`/`async` prefixes — the -// SAME shape `enclosingFunction` looks backward for and `findSeamBypasses` -// uses to recognise (and skip) the seam functions' own definitions. +// SAME shape `nearestPrecedingAssignment` uses as its backward-scan boundary, +// and `enclosingFunction` below uses to recognise (and skip) the seam +// functions' own definitions. const FUNCTION_DECL_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+([A-Za-z_$][\w$]*)\s*\(/; /** @@ -323,8 +381,8 @@ function findPolicyDispatchDrift(rel, text) { // `file` is sanitized here, at construction, not just at the human // formatter: `rel` is exactly as attacker-controlled as `source` on a // fork PR (a tracked filename can legally carry C1 bytes or bidi - // overrides), and it reaches the committed baseline and `--json` stdout - // unfiltered otherwise — see `sanitizeForReport`'s own header. + // overrides), and it reaches `--json` stdout unfiltered otherwise — see + // `sanitizeForReport`'s own header. FIELD_NAME_DISPATCH_RE.lastIndex = 0; let m; while ((m = FIELD_NAME_DISPATCH_RE.exec(line)) !== null) { @@ -336,7 +394,7 @@ function findPolicyDispatchDrift(rel, text) { // `field` is captured straight out of a quoted string literal in // repo source — attacker-controlled on the same fork-PR basis as // `file`/`source`, so sanitize it too rather than let it reach - // `--json` stdout / the baseline raw. + // `--json` stdout. field: sanitizeForReport(m[2]), source: sanitizeForReport(line.trim()), }); @@ -348,10 +406,6 @@ function findPolicyDispatchDrift(rel, text) { axis: 'policy-dispatch', file: sanitizeForReport(rel), line: i + 1, - // `field` is captured straight out of a quoted string literal in - // repo source — attacker-controlled on the same fork-PR basis as - // `file`/`source`, so sanitize it too rather than let it reach - // `--json` stdout / the baseline raw. field: sanitizeForReport(m[2]), source: sanitizeForReport(line.trim()), }); @@ -363,10 +417,6 @@ function findPolicyDispatchDrift(rel, text) { axis: 'policy-dispatch', file: sanitizeForReport(rel), line: i + 1, - // `field` is captured straight out of a quoted string literal in - // repo source — attacker-controlled on the same fork-PR basis as - // `file`/`source`, so sanitize it too rather than let it reach - // `--json` stdout / the baseline raw. field: sanitizeForReport(m[2]), source: sanitizeForReport(line.trim()), }); @@ -429,11 +479,9 @@ function isQuotedLiteralArg(arg) { * 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. + * declaration. 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 @@ -506,31 +554,115 @@ function findUnstrippedContentWrites(rel, text) { 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 2 (§8.6, retained): `fs.writeFileSync(, ...)`, capturing +// the target-path argument up to the next comma. The type system (ADR-3473 +// §8.6's `StateTransaction` constructors) closes the `writeStateMd`/ +// `syncStateFrontmatter`/`applyPostSyncPreservation` bypass shape this guard +// used to scan for by function name; it cannot close a call site that skips +// those functions ENTIRELY and reaches for Node's raw `fs` module directly — +// that residual risk is what this axis stays alive to catch. +const RAW_WRITE_CALL_START_RE = /\bfs\.writeFileSync\s*\(/g; /** - * 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. + * Capture `fs.writeFileSync`'s first-argument text starting at `startIdx` + * (the offset right after its opening `(`), stopping at the first TOP-LEVEL + * comma or the call's own closing paren — bracket/paren/brace depth and + * string-literal spans are tracked so a nested call in the target expression + * (e.g. `path.join(cwd, 'STATE.md')`) does not stop the scan at ITS internal + * comma. A naive `[^,]+` capture (the guard's prior encoding) stopped at + * `path.join(cwd` for exactly that shape, silently missing every + * `fs.writeFileSync(path.join(cwd, 'STATE.md'), …)` call in the wild + * (found via `tests/state-write-path-drift-guard.test.cjs` F1: "guard: + * fs.writeFileSync against a STATE.md literal is reported"). */ -function findSeamBypasses(rel, text) { +function captureFirstArg(line, startIdx) { + let depth = 0; + let inStr = null; + let i = startIdx; + for (; i < line.length; i++) { + const c = line[i]; + if (inStr) { + if (c === '\\') { i++; continue; } + if (c === inStr) inStr = null; + continue; + } + if (c === '\'' || c === '"' || c === '`') { inStr = c; continue; } + if (c === '(' || c === '[' || c === '{') { depth++; continue; } + if (c === ')' || c === ']' || c === '}') { + if (depth === 0) break; // the call's own closing paren — no comma found + depth--; + continue; + } + if (c === ',' && depth === 0) break; + } + return line.slice(startIdx, i); +} + +// True when the captured target-path expression plausibly names the STATE.md +// path: either the canonical `statePath` identifier this codebase uses at +// every real write site (see `src/state.cts`'s `writeStateMd`, +// `readModifyWriteStateMd`, `cmdStateMilestoneSwitch`), or a literal/template +// segment containing `STATE.md` outright. +function targetsStatePath(arg) { + return /\bstatePath\b/.test(arg) || /STATE\.md/.test(arg); +} + +/** + * AXIS 2: every `fs.writeFileSync(` call in `text` whose target argument + * names the state path is a raw write that bypasses BOTH the write seam + * (`writeStateMd` / `syncAndPreserveStateMd`) and the OS Shell Projection + * seam (`platformWriteSync`, `src/shell-command-projection.cts`) — no + * legitimate call site in this codebase writes STATE.md this way today + * (every real writer goes through `platformWriteSync`, whose OWN internal + * `fs.writeFileSync` calls take a generic `filePath`/`tmpPath` argument, not + * `statePath`, and are therefore never matched by `targetsStatePath` above). + * Unratcheted, unexempted: any occurrence is a violation. + */ +function findRawStateWrites(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; + RAW_WRITE_CALL_START_RE.lastIndex = 0; + let m; + while ((m = RAW_WRITE_CALL_START_RE.exec(line)) !== null) { + const argStart = m.index + m[0].length; + const targetArg = captureFirstArg(line, argStart).trim(); + if (!targetsStatePath(targetArg)) continue; + // `file`/`source` sanitized for the same fork-PR reason as every other + // finding in this guard. + out.push({ + reason: REASON.RAW_STATE_WRITE, + axis: 'raw-write', + file: sanitizeForReport(rel), + line: i + 1, + source: sanitizeForReport(rawLines[i].trim()), + }); + } + } + return out; +} + +// The two write-seam STAGE functions, matched only as CALLS (`\(` +// immediately after, modulo whitespace) — never as bare mentions of the +// name. `writeStateMd(` is deliberately NOT included here (that arm is +// retired — ADR-3473 §8.6 gates it at the type level instead). +const SEAM_CALL_RE = /\b(syncStateFrontmatter|applyPostSyncPreservation)\s*\(/g; +// A line that IS one of the two seam stage functions' own definitions — +// skipped outright, never counted as a call to itself. +const SEAM_DEF_LINE_RE = /^\s*(?:export\s+)?(?:async\s+)?function\s+(?:syncStateFrontmatter|applyPostSyncPreservation)\b/; + +/** + * AXIS 5 (§8.3, RETAINED, issue #3871): every direct `syncStateFrontmatter(`/ + * `applyPostSyncPreservation(` call in `text`, outside the two functions' own + * definitions and (only inside `SEAM_OWNER_FILE`) outside + * `SEAM_OWNER_EXEMPT_FUNCTIONS`'s own bodies. Terminal: unlike the old + * `findSeamBypasses` this axis descends from, there is no ratchet — any + * occurrence is `REASON.COMPOSITION_BYPASS` directly. + */ +function findCompositionBypasses(rel, text) { const rawLines = text.split('\n'); const stripped = stripComments(text); const out = []; @@ -545,13 +677,11 @@ function findSeamBypasses(rel, text) { const fn = enclosingFunction(stripped, i); if (fn && SEAM_OWNER_EXEMPT_FUNCTIONS.includes(fn)) continue; } - // `file` is sanitized here, at construction, not just at the human - // formatter: `rel` is exactly as attacker-controlled as `source` on a - // fork PR (a tracked filename can legally carry C1 bytes or bidi - // overrides), and it reaches the committed baseline and `--json` - // stdout unfiltered otherwise — see `sanitizeForReport`'s own header. + // `file`/`source` sanitized for the same fork-PR reason as every other + // finding in this guard. out.push({ - axis: 'write-seam', + reason: REASON.COMPOSITION_BYPASS, + axis: 'composition-bypass', file: sanitizeForReport(rel), line: i + 1, symbol: m[1], @@ -600,10 +730,11 @@ function isInsideCodeSpan(line, index) { } /** - * AXIS 2b: every prompt-layer line instructing an agent to shell out to a - * write-side `gsd-tools` subcommand. Same finding shape as - * `findSeamBypasses` (no `reason` — the ratchet assigns it), with a fixed - * `symbol` since there is no single function name to report for prose. + * AXIS 4: every prompt-layer line instructing an agent to shell out to a + * write-side `gsd-tools` subcommand. Terminal (unratcheted): any occurrence + * is a violation — this baseline was always empty for the prompt layer (no + * prompt-layer entry was ever acknowledged), so removing the ratchet changes + * nothing observable here. * * A candidate occurrence enclosed in backticks is a MENTION, not an * invocation, and is deliberately not reported — CONTRIBUTING.md's "Every @@ -614,8 +745,7 @@ function isInsideCodeSpan(line, index) { * wrong: the first `lint-phase-enumeration-drift.cjs` flagged JSDoc that * merely documented the canonical owner as drift, which trains readers to * reflexively exempt documentation instead of trusting the guard — the - * opposite of Decision 4(a)'s intent. All 5 of this guard's original - * prompt-layer baseline entries were exactly this false positive. + * opposite of Decision 4(a)'s intent. */ function findPromptSeamUses(rel, text) { const lines = text.split('\n'); @@ -626,10 +756,11 @@ function findPromptSeamUses(rel, text) { let m; while ((m = PROMPT_SEAM_RE.exec(line)) !== null) { if (isInsideCodeSpan(line, m.index)) continue; - // `file` is sanitized here for the same reason as `findSeamBypasses` - // above: `rel` is attacker-controlled on a fork PR, exactly like - // `source`. + // `file` is sanitized here for the same reason as every other finding + // in this guard: `rel` is attacker-controlled on a fork PR, exactly + // like `source`. out.push({ + reason: REASON.PROMPT_LAYER_STATE_WRITE, axis: 'write-seam', file: sanitizeForReport(rel), line: i + 1, @@ -642,149 +773,23 @@ function findPromptSeamUses(rel, text) { } /** - * Ratchet key for one write-seam finding — `(file, TRIMMED source text)`, - * NEVER a line number, which churns on any unrelated edit to the same file - * (mirrors `qa-smell-ratchet.cjs`'s own key shape). `v.source` is already - * the trimmed, sanitized line text by the time it reaches this function. - */ -function ratchetKey(v) { - return `${v.file} ${v.source}`; -} - -/** - * Read `BASELINE_PATH`. Returns `{ entries: [] }` when the file is ABSENT - * (`ENOENT` — first run, or a fully-shrunk Phase 4 baseline that deleted the - * file — ADR-3408 §8.3's roster foresees exactly this end state); returns - * `{ entries: null, code }` when the file is present but could not be read - * OR could not be parsed/shaped (missing/malformed `entries` array) — `code` - * carries the underlying `fs` error code (e.g. `'EACCES'`) when the failure - * happened at the read step, `null` when it happened at the parse/shape - * step, so the caller can fail closed rather than silently ratcheting - * against nothing. Returns the parsed object when the read+parse succeed. + * Run both scan passes (the `src/` tree for Axis 1 + Axis 2 + Axis 3 + Axis 5, + * the prompt layer for Axis 4) and return the flat, already-terminal finding + * list — every finding this guard produces carries its own `reason`; there + * is no longer a ratcheted axis needing a second pass against a baseline. * - * Absent-vs-unreadable is deliberately NOT collapsed into one arm. This - * guard exists to catch write paths whose failure and success are - * output-identical (ADR-3180 / ADR-3408, "The failure mode that hides all - * of it") — a `catch { return { entries: [] } }` around the read would - * reproduce exactly that shape in the tool built to detect it: an - * unreadable baseline (EACCES, EISDIR, EIO, ...) would be silently - * indistinguishable from a legitimate absent one. Do not simplify this back - * into a single catch arm. + * `root` defaults to `REPO_ROOT` (this repo) so every existing caller — + * `npm run lint:ci`, a bare `node scripts/lint-state-write-path-drift.cjs`, + * every other module that `require`s `collect` with no argument — is + * byte-identical to before this parameter existed. It is overridable so a + * test can exercise the guard's real scanning/reporting logic against a + * throwaway synthetic tree instead of mutating this repository's own `src/` + * to prove the guard can fail (see `--root` on the CLI, and F2 in + * `tests/state-write-path-drift-guard.test.cjs`). */ -function loadBaseline() { - let raw; - try { - raw = fs.readFileSync(BASELINE_PATH, 'utf8'); - } catch (err) { - if (err && err.code === 'ENOENT') return { entries: [] }; - return { entries: null, code: err && err.code ? err.code : 'UNKNOWN' }; - } - let doc; - try { - doc = JSON.parse(raw); - } catch { - return { entries: null, code: null }; - } - if (!doc || typeof doc !== 'object' || !Array.isArray(doc.entries)) return { entries: null, code: null }; - return doc; -} - -/** - * The ratchet — mirrors `scripts/qa-smell-ratchet.cjs`'s four invariants, - * applied to write-seam bypass COUNTS instead of QA-smell fingerprints: - * - * 1. An observed key absent from the baseline is a brand-new, - * unacknowledged bypass — `SEAM_BYPASS_UNRECORDED`. - * 2. An observed count greater than the acknowledged count is a NEW copy - * landing beside an already-acknowledged one — `SEAM_BYPASS_COUNT_GREW`. - * 3. An observed count less than the acknowledged count is a PARTIAL - * migration — some but not all call sites at this exact key were - * removed, and the baseline still claims the old, larger number — - * `SEAM_BYPASS_COUNT_SHRANK`. - * 4. A baseline key with zero current observations is a STALE - * acknowledgment: an entry may never outlive what it describes, and - * the baseline may only shrink (via `--baseline`, regenerated) — - * `BASELINE_ENTRY_STALE`. - * - * The occurrence COUNT (not just key presence) is what makes a partial - * migration visible at all: two byte-identical call sites in one file are - * otherwise a single indistinguishable key, so removing one of the two - * would silently vanish from a presence-only check while the acknowledgment - * still describes two. - * - * Every returned finding carries both `observed` and `acknowledged` counts. - */ -function applyRatchet(observed, baseline) { - const observedByKey = new Map(); - for (const finding of observed) { - const key = ratchetKey(finding); - let group = observedByKey.get(key); - if (!group) { - group = { count: 0, sample: finding }; - observedByKey.set(key, group); - } - group.count += 1; - } - - const baselineByKey = new Map(); - for (const entry of baseline.entries) { - baselineByKey.set(`${entry.file} ${entry.source}`, entry); - } - - const out = []; - for (const [key, group] of observedByKey) { - const entry = baselineByKey.get(key); - const acknowledged = entry && typeof entry.count === 'number' ? entry.count : 0; - let reason = null; - if (!entry) { - reason = REASON.SEAM_BYPASS_UNRECORDED; - } else if (group.count > acknowledged) { - reason = REASON.SEAM_BYPASS_COUNT_GREW; - } else if (group.count < acknowledged) { - reason = REASON.SEAM_BYPASS_COUNT_SHRANK; - } - if (!reason) continue; - out.push({ - reason, - axis: 'write-seam', - file: group.sample.file, - line: group.sample.line, - symbol: group.sample.symbol, - source: group.sample.source, - observed: group.count, - acknowledged, - }); - } - - for (const [key, entry] of baselineByKey) { - if (observedByKey.has(key)) continue; - out.push({ - reason: REASON.BASELINE_ENTRY_STALE, - axis: 'write-seam', - file: entry.file, - line: 0, - symbol: entry.symbol, - source: entry.source, - observed: 0, - acknowledged: typeof entry.count === 'number' ? entry.count : 0, - }); - } - - return out; -} - -/** - * 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`) — 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() { +function collect(root = REPO_ROOT) { const srcFindings = scanTree({ - root: REPO_ROOT, + root, scanDirs: SRC_DIRS, scanExt: SRC_EXT, onFile(rel, text) { @@ -795,13 +800,14 @@ function collect() { found.push(...findUnimplementedPolicies(text, relPosix)); found.push(...findUnstrippedContentWrites(relPosix, text)); } - found.push(...findSeamBypasses(relPosix, text)); + found.push(...findRawStateWrites(relPosix, text)); + found.push(...findCompositionBypasses(relPosix, text)); return found; }, }); const promptFindings = scanTree({ - root: REPO_ROOT, + root, scanDirs: PROMPT_DIRS, scanExt: PROMPT_EXT, onFile(rel, text) { @@ -809,96 +815,11 @@ function collect() { }, }); - const all = [...srcFindings, ...promptFindings]; - return { - policyFindings: all.filter((f) => f.axis !== 'write-seam'), - seamFindings: all.filter((f) => f.axis === 'write-seam'), - }; -} - -/** - * Group `seamFindings` by `ratchetKey` into the baseline entry shape - * (`{file, source, symbol, count, owner}`), sorted by `file+source`. - * - * `owner` is NEVER invented by this mechanical regeneration — the guard can - * observe WHERE a bypass is and HOW MANY there are, but not which issue owns - * removing it; inventing one would violate the same "never guess" discipline - * `qa-smell-ratchet.cjs` applies to its own `issue` field (its `--update` - * never invents an issue number either). ADR-3408 §8.3 requires every - * shipped entry to carry "a named ratchet entry carrying the issue that owns - * its removal, never an unrecorded pass" — that owner is HUMAN-CURATED and - * must be recorded before the entry ships. - * - * Because `--baseline` overwrites `BASELINE_PATH` wholesale, a naive - * mechanical regeneration would silently re-null every curated `owner` on - * each run. To avoid that, `existingEntries` (the baseline as it stood - * BEFORE this regeneration, i.e. `loadBaseline().entries`) is optional and, - * when supplied, its `owner` values are merged forward onto matching new - * entries keyed on `(file, source)` — the same key `ratchetKey` / - * `applyRatchet` use to identify a bypass. A key with no prior entry (a - * brand-new bypass) still gets `owner: null`, exactly as before; only - * already-curated owners survive the regeneration. Re-running `--baseline` - * twice in a row is therefore idempotent with respect to `owner`. - */ -function buildBaselineEntries(seamFindings, existingEntries) { - const priorOwnerByKey = new Map(); - if (Array.isArray(existingEntries)) { - for (const entry of existingEntries) { - priorOwnerByKey.set(ratchetKey(entry), entry.owner); - } - } - - const groups = new Map(); - for (const finding of seamFindings) { - const key = ratchetKey(finding); - let group = groups.get(key); - if (!group) { - group = { - file: finding.file, - source: finding.source, - symbol: finding.symbol, - count: 0, - owner: priorOwnerByKey.has(key) ? priorOwnerByKey.get(key) : null, - }; - groups.set(key, group); - } - group.count += 1; - } - return [...groups.values()].sort((a, b) => ratchetKey(a).localeCompare(ratchetKey(b))); -} - -const BASELINE_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.'; - -function writeBaseline(seamFindings) { - const priorBaseline = loadBaseline(); - const entries = buildBaselineEntries( - seamFindings, - Array.isArray(priorBaseline.entries) ? priorBaseline.entries : null, - ); - const doc = { _comment: BASELINE_COMMENT, entries }; - fs.writeFileSync(BASELINE_PATH, `${JSON.stringify(doc, null, 2)}\n`, 'utf8'); - return entries; + return { findings: [...srcFindings, ...promptFindings] }; } const GOODHART_NOTE = - 'Goodhart note (ADR-3408 Decision 5): this "0 write-path bypasses" is a LAGGING metric — report ' + + 'Goodhart note (ADR-3408 Decision 5): this "0 write-path violations" is a LAGGING metric — report ' + 'it only alongside the behavioral identity test\'s result (asserted at the consumer\'s output), ' + 'never alone.'; @@ -910,81 +831,66 @@ function printFindings(findings) { } /** - * `argv`: `--baseline` regenerates `BASELINE_PATH` from a fresh scan and - * exits 0; `--json` (check mode only) prints the machine-readable finding - * set instead of the human-readable report. Exit codes: 0 clean, 1 drift - * (policy-dispatch violation, ratchet violation, or an unreadable - * baseline), 2 usage error. + * Parse `argv` into `{ root, wantJson, unknown }`. `--root ` overrides + * the scan root (default `REPO_ROOT`, resolved relative to `process.cwd()` + * when given); `--json` is a bare flag. Anything else lands in `unknown` so + * `main` can report a usage error rather than silently ignoring a typo. + */ +function parseArgs(argv) { + const args = argv || []; + let root = REPO_ROOT; + let wantJson = false; + const unknown = []; + for (let i = 0; i < args.length; i++) { + const a = args[i]; + if (a === '--json') { + wantJson = true; + continue; + } + if (a === '--root') { + const value = args[i + 1]; + if (typeof value !== 'string' || value.length === 0) { + return { error: '--root requires a directory argument' }; + } + root = path.resolve(value); + i += 1; + continue; + } + unknown.push(a); + } + return { root, wantJson, unknown }; +} + +/** + * `argv`: `--json` prints the machine-readable finding set instead of the + * human-readable report; `--root ` overrides the scan root (defaults to + * this repo — see `parseArgs`'s own docstring). Exit codes: 0 clean, 1 a + * finding was reported, 2 usage error. */ function main(argv) { - const args = argv || []; - const recognized = new Set(['--baseline', '--json']); - const unknown = args.filter((a) => !recognized.has(a)); + const parsed = parseArgs(argv); + if (parsed.error) { + process.stderr.write(`lint-state-write-path-drift: ${parsed.error}\n`); + process.exitCode = 2; + return; + } + const { root, wantJson, unknown } = parsed; if (unknown.length > 0) { process.stderr.write( `lint-state-write-path-drift: unrecognized argument(s): ${unknown.map((a) => sanitizeForReport(a)).join(', ')} ` + - '(expected --baseline and/or --json)\n', + '(expected --json and/or --root )\n', ); process.exitCode = 2; return; } - if (args.includes('--baseline')) { - const { seamFindings } = collect(); - const entries = writeBaseline(seamFindings); - process.stdout.write(`lint-state-write-path-drift: wrote ${entries.length} entries to ${BASELINE_PATH}\n`); - process.exitCode = 0; - return; - } - - const wantJson = args.includes('--json'); - const baseline = loadBaseline(); - - if (baseline.entries === null) { - const finding = { - reason: REASON.BASELINE_UNREADABLE, - axis: 'write-seam', - file: path.relative(REPO_ROOT, BASELINE_PATH), - line: 0, - symbol: null, - code: baseline.code, - source: sanitizeForReport( - baseline.code - ? `${BASELINE_PATH} is present but could not be read (${baseline.code}) — run \`node ${__filename} --baseline\` to regenerate it` - : `${BASELINE_PATH} is present but unparseable — run \`node ${__filename} --baseline\` to regenerate it`, - ), - }; - if (wantJson) { - process.stdout.write( - `${JSON.stringify( - { - ok: false, - findings: [finding], - summary: { policyDispatchViolations: 0, seamBypassesObserved: 0, seamBypassesAcknowledged: 0, ratchetViolations: 0 }, - }, - null, - 2, - )}\n`, - ); - } else { - printFindings([finding]); - } - process.exitCode = 1; - return; - } - - const { policyFindings, seamFindings } = collect(); - const ratchetFindings = applyRatchet(seamFindings, baseline); - const findings = [...policyFindings, ...ratchetFindings]; - const acknowledgedTotal = baseline.entries.reduce( - (sum, e) => sum + (typeof e.count === 'number' ? e.count : 0), - 0, - ); + const { findings } = collect(root); const summary = { - policyDispatchViolations: policyFindings.length, - seamBypassesObserved: seamFindings.length, - seamBypassesAcknowledged: acknowledgedTotal, - ratchetViolations: ratchetFindings.length, + policyDispatchViolations: findings.filter((f) => f.axis === 'policy-dispatch').length, + frontmatterWriteViolations: findings.filter((f) => f.axis === 'frontmatter-write').length, + rawWriteViolations: findings.filter((f) => f.axis === 'raw-write').length, + writeSeamViolations: findings.filter((f) => f.axis === 'write-seam').length, + compositionBypassViolations: findings.filter((f) => f.axis === 'composition-bypass').length, }; if (wantJson) { @@ -995,8 +901,8 @@ function main(argv) { if (findings.length === 0) { process.stdout.write( - 'ok state-write-path-drift: no policy-dispatch violations, no unrecorded/grown/shrunk/stale ' + - 'write-seam entries against the acknowledged baseline\n', + 'ok state-write-path-drift: no policy-dispatch, frontmatter-write, raw-write, prompt-layer, or ' + + 'composition-bypass violations found\n', ); process.stdout.write(`${GOODHART_NOTE}\n`); process.exitCode = 0; @@ -1004,8 +910,9 @@ function main(argv) { } process.stderr.write( - 'state-write-path-drift: policy-dispatch and/or write-seam divergence found (ADR-3408 §8.1/§8.3). ' + - 'See docs/adr/3408-state-write-path-preservation.md for the contract:\n', + 'state-write-path-drift: violation(s) found (ADR-3408 §8.1/§8.3, ADR-3473 §8.6). See ' + + 'docs/adr/3408-state-write-path-preservation.md and docs/adr/3473-enforcement-by-construction.md ' + + 'for the contract:\n', ); printFindings(findings); process.exitCode = 1; @@ -1016,7 +923,6 @@ if (require.main === module) main(process.argv.slice(2)); module.exports = { REASON, REPO_ROOT, - BASELINE_PATH, SRC_DIRS, SRC_EXT, PROMPT_DIRS, @@ -1033,13 +939,12 @@ module.exports = { findUnstrippedContentWrites, isQuotedLiteralArg, nearestPrecedingAssignment, - findSeamBypasses, + findRawStateWrites, + targetsStatePath, + findCompositionBypasses, findPromptSeamUses, isInsideCodeSpan, - ratchetKey, - loadBaseline, - applyRatchet, + parseArgs, collect, - buildBaselineEntries, main, }; diff --git a/scripts/state-write-path-drift-baseline.json b/scripts/state-write-path-drift-baseline.json deleted file mode 100644 index fbfa9bb28..000000000 --- a/scripts/state-write-path-drift-baseline.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "_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", - "source": "writeStateMd(statePath, stateContent, cwd);", - "symbol": "writeStateMd", - "count": 1, - "owner": "sanctioned-permanent" - }, - { - "file": "src/state.cts", - "source": "writeStateMd(statePath, modified, cwd);", - "symbol": "writeStateMd", - "count": 1, - "owner": "sanctioned-permanent" - } - ] -} diff --git a/src/health-diagnostic.cts b/src/health-diagnostic.cts index 18917faeb..f72829f66 100644 --- a/src/health-diagnostic.cts +++ b/src/health-diagnostic.cts @@ -129,6 +129,12 @@ const { getMilestoneInfo } = roadmapParserMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import stateMod = require('./state.cjs'); const { writeStateMd } = stateMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import frontmatter = require('./frontmatter.cjs'); +const { extractFrontmatter } = frontmatter; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import stateTransitionMod = require('./state-transition.cjs'); +const { rebuildStateTransaction } = stateTransitionMod; import { realClock } from './clock.cjs'; import { platformReadSync as safeReadFile, platformWriteSync } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; @@ -337,7 +343,15 @@ function runRepairAction(cwd: string, action: RemedyAction, paths: RepairPaths): stateContent += `**Status:** Resuming\n\n`; stateContent += `## Session Log\n\n`; stateContent += `- ${realClock.localToday()}: STATE.md regenerated by ${slash('health')} --repair\n`; - writeStateMd(statePath, stateContent, cwd); + // ADR-3473 §8.6: `/gsd-health --repair` is a factory reset, so + // preservation must not run; the snapshot of the file being replaced is + // still captured, and `{}` is the correct, legal snapshot of a + // STATE.md that had no parseable frontmatter — which is the usual + // reason this repair fires. + const priorState = fs.existsSync(statePath) ? (safeReadFile(statePath) ?? '') : ''; + writeStateMd(statePath, stateContent, rebuildStateTransaction({ + snapshot: extractFrontmatter(priorState, statePath), + }), cwd); return { success: true, path: 'STATE.md', extraDetails }; } diff --git a/src/state-document.cts b/src/state-document.cts index 9352d2cc1..eb23c2050 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -17,7 +17,16 @@ import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; type Scope = planningScopeMod.Scope; -function toFiniteNumber(value: unknown): number | null { +/** + * Coerce an arbitrary frontmatter scalar to a finite number, or `null` if it + * is not one. Exported per ADR-3473 §8.6: `state-transition.cts`'s + * progress-ratchet unmeasured-scan check ("is this derived total a real + * measurement?") must ask through the SAME coercion this module already uses + * for `existingProgressExceedsDerived`, rather than growing a second private + * copy. This matters because frontmatter scalars arrive as STRINGS + * (`"0"`, not `0`) — a raw `=== 0` test is wrong at both call sites. + */ +export function toFiniteNumber(value: unknown): number | null { const number = Number(value); return Number.isFinite(number) ? number : null; } diff --git a/src/state-transition.cts b/src/state-transition.cts index 1373de70c..fbacdd34c 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -17,7 +17,7 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback, stateReplaceFieldInSession } from './state-document.cjs'; -import { KNOWN_TEMPLATE_DEFAULTS } from './state-document.cjs'; +import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; @@ -260,6 +260,147 @@ export function getFieldClassification(field: string): FieldClassification | nul return FIELD_CLASSIFICATION[field]; } +/** + * #3836: the single source of truth for "which frontmatter keys carry the + * `preserve-when-unchanged` policy" — read straight off `FIELD_CLASSIFICATION` + * rather than re-typed as a hand-maintained literal array at each consumer. + * `cmdStateJson` (`state.cts`) previously hardcoded a 6-field list that had + * already drifted from this table by one row (`last_activity_desc`, #3258) — + * exactly the "second table parallel to the first" shape ADR-3473 exists to + * remove. `progress`/`milestone`/`milestone_name` carry a different + * preservation policy (`preserve-always` / `preserve-if-placeholder`) and are + * naturally excluded by the filter, not by a separate exclusion list. + */ +export function getPreserveWhenUnchangedFields(): readonly string[] { + return Object.keys(FIELD_CLASSIFICATION).filter( + (field) => FIELD_CLASSIFICATION[field].preservation === 'preserve-when-unchanged', + ); +} + +// ---------------------------------------------------------------------------- +// The state transaction (ADR-3473 §8.6, issue #3871) +// ---------------------------------------------------------------------------- +// +// Collapses the two-field `preFm` / `preFmSnapshot` shape (same source, same +// arguments, same `extractFrontmatter` call — `preFm` was `preFmSnapshot` with +// a policy flag baked in by nulling it on `resync`) into ONE snapshot plus a +// typed constructor pair that names the policy explicitly instead of encoding +// it as a null. See `.gsd/phase/feat-3871-state-transaction-snapshot/40-design.md`. + +/** The two sanctioned write-path kinds (ADR-3408 §8.3's closed exception list, typed). */ +export type StateTransactionKind = 'open' | 'rebuild'; + +export type StateBodyDelta = { pre: string | null; post: string | null }; + +export type StateTransactionInit = { + /** Pre-write frontmatter snapshot. `{}` is legal (see `createStateTransaction`). */ + snapshot: Record; + resync?: boolean; + deriveProgressKeys?: boolean; + bodyDeltas?: Record; + /** + * True ONLY when `resync` is true BECAUSE the caller explicitly named a + * progress-affecting field (`state update Progress`, `state patch + * Progress=...` / `Total Plans in Phase` / `Total Phases` — + * `shouldResyncStateProgress`, state.cts), as opposed to `resync` + * defaulting true for an unrelated write. See `applyPreserveAlways`'s use + * of this flag for why the distinction matters (found while diagnosing a + * regression against the pre-existing #3242 "resyncs progress frontmatter + * from the updated body" spec, tests/frontmatter.test.cjs). + */ + explicitProgressField?: boolean; +}; + +export type StateTransaction = { + readonly kind: StateTransactionKind; + readonly snapshot: Readonly>; + readonly resync: boolean; + readonly deriveProgressKeys: boolean; + readonly bodyDeltas?: Readonly>; + readonly explicitProgressField: boolean; +}; + +/** + * Shared constructor body for `openStateTransaction` / `rebuildStateTransaction` + * (ADR-3473 §8.6 Decision 2/3). Validates `init.snapshot` and freezes the + * result so nothing downstream can mutate a transaction after construction + * (this is what makes the aliasing fix in `applyPreserveAlways`'s clone hold: + * the snapshot a caller passed in cannot be rewritten out from under it). + * + * `{}` and a null-prototype object are BOTH legal snapshots (Decision 2 / row + * 15 of the behavior table): `extractFrontmatter` returns `{}` for a document + * with no frontmatter or an unterminated one and never returns null or throws + * (`src/frontmatter.cts`), so `{}` is the honest snapshot of a real document — + * and `/gsd-health --repair`, which runs precisely when STATE.md is broken, + * depends on that staying legal. What is NOT legal is the snapshot being + * ABSENT (`null`/`undefined`/an array/a non-object): that is the caller + * forgetting to read the pre-write document at all, a construction failure, + * not a data question. Conflating "absent" with "empty" would turn the repair + * path's normal case into a hard throw. + */ +function createStateTransaction(kind: StateTransactionKind, init: StateTransactionInit, ctorName: string): StateTransaction { + if (init === null || typeof init !== 'object' || Array.isArray(init)) { + const err = new Error( + `${ctorName}: expected an init object, got ${init === null ? 'null' : typeof init}. ` + + 'Per ADR-3473 §8.6 / Decision 2, an absent init is a construction failure, distinct from ' + + 'a legal empty snapshot ({}) — do not "fix" this by tolerating null.', + ) as Error & { code: string; constructorName: string }; + err.code = 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'; + err.constructorName = ctorName; + throw err; + } + const snapshot = init.snapshot; + if (snapshot === null || snapshot === undefined || typeof snapshot !== 'object' || Array.isArray(snapshot)) { + const err = new Error( + `${ctorName}: init.snapshot is required and must be a non-array object (frontmatter map). ` + + `Per ADR-3473 §8.6 / Decision 2, an ABSENT snapshot is a construction failure — this is NOT ` + + 'the same as a legal EMPTY snapshot ({}), which every executor accepts and simply finds ' + + 'nothing to restore from (extractFrontmatter returns {} for a document with no parseable ' + + 'frontmatter, and /gsd-health --repair depends on that staying legal). Pass {} explicitly ' + + 'when the document truly has none; do not tolerate null/undefined here.', + ) as Error & { code: string; constructorName: string }; + err.code = 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'; + err.constructorName = ctorName; + throw err; + } + return Object.freeze({ + kind, + snapshot, + resync: init.resync === true, + deriveProgressKeys: init.deriveProgressKeys === true, + bodyDeltas: init.bodyDeltas, + explicitProgressField: init.explicitProgressField === true, + }); +} + +/** + * ADR-3473 §8.6's `open()`: the default write-path transaction. Carries the + * pre-write snapshot and applies preservation (`applyStatePreservation` runs + * its full dispatch loop against it) — this is every STATE.md write EXCEPT + * the two sanctioned exceptions below. + */ +export function openStateTransaction(init: StateTransactionInit): StateTransaction { + return createStateTransaction('open', init, 'openStateTransaction'); +} + +/** + * ADR-3473 §8.6's `rebuild()`: the TYPED expression of ADR-3408 §8.3's closed + * list of sanctioned exceptions to the preservation pipeline. Exactly two + * callers may construct this: `cmdStateSync` (`state sync` re-derives + * frontmatter FROM the body per #905 — the body is authoritative and + * preservation would fight it) and `REGENERATE_STATE` (`/gsd-health --repair`'s + * factory reset — the whole point is to replace what's there). The snapshot + * is still carried (for §8.7's reporting) but `applyStatePreservation` skips + * its dispatch loop entirely for a `rebuild` transaction. + * + * This list is NOT debt to be paid down later — it is a closed, deliberate + * set. Adding a third caller is an amendment to ADR-3408 §8.3, not a call site + * convenience. + */ +export function rebuildStateTransaction(init: StateTransactionInit): StateTransaction { + return createStateTransaction('rebuild', init, 'rebuildStateTransaction'); +} + // ---------------------------------------------------------------------------- // applyStatePreservation — table-driven post-sync preservation (ADR-1769 #1796) // ---------------------------------------------------------------------------- @@ -274,38 +415,18 @@ export function getFieldClassification(field: string): FieldClassification | nul // the pre-#1796 inline block; this is the consolidation ADR-1769 / CONTEXT.md // already claimed shipped. See issue #1796 (Path A: finish the consolidation). +/** + * ADR-3473 §8.6: the whole pre-write policy — snapshot, resync, deriveProgressKeys, + * bodyDeltas — now travels as ONE `StateTransaction` (see `openStateTransaction` + * / `rebuildStateTransaction` above), rather than as four separate fields the + * caller could set inconsistently (the `preFm`/`preFmSnapshot` split this + * replaces was exactly that: the same snapshot with a policy flag baked in by + * nulling one copy of it on resync). + */ export type StatePreservationInput = { - /** Pre-transform frontmatter; `null` when the transition re-derives from disk (resync=true). */ - preFm: Record | null; + transaction: StateTransaction; /** Post-`syncStateFrontmatter` frontmatter (the freshly recomputed one). */ postFm: Record; - /** Always-present pre-transform frontmatter snapshot (drives the #1230 deltas). */ - preFmSnapshot: Record; - /** True when the caller asked for a full disk re-derivation (sync / advancePlan / completePhase). */ - resync: boolean; - /** - * #2440: when true, total_plans and total_phases take the derived (post-sync) - * value even under !resync, instead of the wholesale curated restore. Used - * by callers (e.g. cmdStatePlannedPhase) where total_plans must correct to - * disk truth after plans are added. Body-only writes (state.update/patch) - * leave this false — the #3242 wholesale protection stays in force. - */ - deriveProgressKeys?: boolean; - /** - * #3258 / #3468: pre/post body-source values for every preserve-when-unchanged - * (#1230 delta heuristic) row, keyed by the FRONTMATTER field the policy - * guards — one channel for all seven rows (status, stopped_at, - * current_phase_name, current_phase, current_plan, paused_at, - * last_activity_desc), folded from the two channels (a bodyDeltas map plus - * three dedicated pre/post parameter pairs) #3468 replaced. The caller - * snapshots each field's body source before/after the transform (mirroring - * how buildStateFrontmatter derives it); `applyPreserveWhenUnchanged` - * restores the pre-write frontmatter value when that body source was left - * unchanged by this write. Adding a preserve-when-unchanged row is a - * one-row table edit PLUS a bodyDeltas entry from the caller — omitting the - * entry is not a silent no-op, it THROWS (ADR-3408 §8.2, `applyPreserveWhenUnchanged`). - */ - bodyDeltas?: Record; }; export type StatePreservationResult = { @@ -319,13 +440,17 @@ export type StatePreservationResult = { * §8.1 — one executor per policy, sharing one result). */ export type PreservationCtx = { - preFm: Record | null; postFm: Record; - preFmSnapshot: Record; + snapshot: Record; resync: boolean; deriveProgressKeys: boolean; bodyDeltas: Record | undefined; mutated: boolean; + /** See `StateTransactionInit.explicitProgressField`. Defaults false for a + * plain `PreservationCtx` built outside a `StateTransaction` (e.g. + * `cmdStateJson`'s direct `applyPreserveWhenUnchanged` call, row 19 of the + * behavior table) — that read path never reaches `applyPreserveAlways`. */ + explicitProgressField?: boolean; }; /** @@ -384,7 +509,7 @@ export function applyPreserveWhenUnchanged(field: string, cls: FieldClassificati // 2. Only a real, non-whitespace-only curated string is worth restoring // (#3468: tightened from `.length > 0` to a trimmed check — a whitespace- // only snapshot is not a real curated value). - const snapshot = ctx.preFmSnapshot[field]; + const snapshot = ctx.snapshot[field]; if (typeof snapshot !== 'string' || snapshot.trim().length === 0) return; // 3. Closed-vocabulary guard: status's 'unknown' sentinel is never restored. @@ -402,28 +527,112 @@ export function applyPreserveWhenUnchanged(field: string, cls: FieldClassificati ctx.mutated = true; } +/** + * The closed set of `progress` keys whose non-zero value means "a real + * measurement happened" (ADR-3473 §8.6 / #3756). + */ +const PROGRESS_TOTAL_KEYS = ['total_phases', 'total_plans'] as const; + +/** + * Did this row's derived (or curated) value represent a REAL measurement? + * + * For a `progress-ratchet` row (today, only `progress`): an empty + * milestone-scoped scan is "nothing was measured", not "zero is done" + * (#3756, and the convention #3233 established — `computeProgressPercent` + * already returns `null` for an empty denominator). Only the TOTALS decide: + * `completed_*` being zero is normal for a real project, so it is + * deliberately excluded from this check. A non-object / absent / negative / + * non-numeric total is NOT a measurement, so it degrades TOWARD preservation, + * never toward deletion — `toFiniteNumber` (not a raw `=== 0`/`> 0` test) + * because frontmatter scalars arrive as STRINGS (`"0"`, not `0`). + * + * For any other row (no `progress-ratchet` strategy) the question is + * meaningless, so it answers `true` and behavior is unchanged — this + * function is only ever consulted from inside the `preserve-always` / + * `progress-ratchet` branch below. + */ +function scanMeasuredSomething(cls: FieldClassification, value: unknown): boolean { + if (cls.mergeStrategy !== 'progress-ratchet') return true; + if (value === null || typeof value !== 'object' || Array.isArray(value)) return false; + const rec = value as Record; + return PROGRESS_TOTAL_KEYS.some((k) => (toFiniteNumber(rec[k]) ?? 0) > 0); +} + +/** + * Deep-clone a curated value before it re-enters `postFm` (ADR-3473 §8.6, + * "Defects fixed inline" / aliasing). `structuredClone` is a Node built-in; + * this repo takes no external deps for it. WHY a clone and not a reference + * assignment: the transaction's `snapshot` is now the SAME object §8.7's + * reporting will diff against. Assigning the nested curated object by + * reference would make `postFm.progress` alias that snapshot, so a later + * in-place mutation of `postFm` would silently rewrite the snapshot too, and + * the diff would report "no change" for a field that did change. + */ +function cloneCurated(value: unknown): unknown { + return structuredClone(value); +} + +/** + * Structural equality for a restored value vs. what `postFm` already held + * (ADR-3473 §8.6, "Defects fixed inline" / #948 no-op-write family). + * `JSON.stringify` compare when either side is an object (the `progress` + * block), `===` otherwise. WHY: `applyPreserveAlways` previously set + * `ctx.mutated = true` unconditionally at its tail, even when it restored a + * value identical to what was already there — driving a write that changes + * nothing but still bumps `last_updated` / restamps `state_head`. + * `applyPreserveWhenUnchanged` already guards this (its step 5); this brings + * the two executors into agreement. + */ +function preservedValuesEqual(a: unknown, b: unknown): boolean { + if (typeof a === 'object' || typeof b === 'object') { + return JSON.stringify(a) === JSON.stringify(b); + } + return a === b; +} + /** * Executor for `preservation: 'preserve-always'` (ADR-3408 §8.1). Only * `progress` carries this policy today. Preserves #3242/#1446/#2440/#2969 - * semantics byte-for-byte: gated on `!resync` and a truthy `preFm[field]`; - * the `mergeStrategy: 'progress-ratchet'` per-key merge only fires when the - * caller opts in via `deriveProgressKeys`, else the whole curated block wins - * wholesale. + * semantics byte-for-byte on every row the behavior table marks unchanged; + * ADR-3473 §8.6 fixes the #3756 defect (a resyncing write that measured + * nothing must not drop a real curated block) plus the two "Defects fixed + * inline" no-op-write / aliasing bugs. */ function applyPreserveAlways(field: string, cls: FieldClassification, ctx: PreservationCtx): void { - if (ctx.resync || !ctx.preFm || !ctx.preFm[field]) return; + const curated = ctx.snapshot[field]; + if (!curated) return; + const derived = ctx.postFm[field]; + const derivedMeasured = scanMeasuredSomething(cls, derived); + const curatedMeasured = scanMeasuredSomething(cls, curated); + // On a resyncing write the fresh derivation is authoritative — UNLESS it + // measured nothing while the curated block did (#3756), AND the caller did + // not explicitly name a progress-affecting field this write. The + // unmeasured-scan guard exists to stop an INCIDENTAL resync (e.g. `state + // add-decision`, whose `resync` defaults true for reasons that have + // nothing to do with `progress`) from dropping a real curated block when a + // milestone-scoped disk scan measures nothing (#3756's archived-milestone + // case). It must not also block a write the user pointed AT `progress` on + // purpose: `preserve-always`'s own contract is "never overwrite unless the + // caller explicitly names this field" (FIELD_CLASSIFICATION doc comment), + // and `state update Progress` / `state patch Progress=...` are exactly + // that naming — the resync they trigger must win even when the disk scan + // it also drives (e.g. because there are no phase dirs at all) reads as + // "unmeasured" (tests/frontmatter.test.cjs: "state.update \"Progress\" + // resyncs progress frontmatter from the updated body", pre-existing, #3242). + if (ctx.resync && (derivedMeasured || !curatedMeasured || ctx.explicitProgressField)) return; - if (cls.mergeStrategy === 'progress-ratchet' && ctx.deriveProgressKeys && ctx.postFm[field]) { + let next: unknown; + if (cls.mergeStrategy === 'progress-ratchet' && ctx.deriveProgressKeys && derived && derivedMeasured) { // #2440: total_plans and total_phases always take the derived (post-sync) // value even under !resync. This is used by cmdStatePlannedPhase where // total_plans must correct upward after plans are added. For body-only // writes (state.update/patch without the flag), the wholesale restore // below preserves everything as before — the #3242 Bug A protection // stays fully in force. - const curated = ctx.preFm[field] as Record | null; - const derived = (ctx.postFm[field] ?? {}) as Record; - const merged: Record = { ...derived }; - if (curated) { + const curatedRecord = curated as Record | null; + const derivedRecord = (derived ?? {}) as Record; + const merged: Record = { ...derivedRecord }; + if (curatedRecord) { // #2440: total_plans and total_phases always take the derived value. // #2969: completed_plans and completed_phases take the derived value // when it is GREATER than the curated value (gap-closure plans that @@ -434,10 +643,10 @@ function applyPreserveAlways(field: string, cls: FieldClassification, ctx: Prese // disk counts, and a stale curated percent would be incoherent against // the ratcheted-up completed counts (e.g. 54/54 at 93%). const ratchetUpKeys = new Set(['completed_plans', 'completed_phases']); - for (const [key, value] of Object.entries(curated)) { + for (const [key, value] of Object.entries(curatedRecord)) { if (key === 'total_plans' || key === 'total_phases' || key === 'percent') continue; if (ratchetUpKeys.has(key)) { - const derivedNum = typeof derived[key] === 'number' ? derived[key] : -Infinity; + const derivedNum = typeof derivedRecord[key] === 'number' ? derivedRecord[key] : -Infinity; const curatedNum = typeof value === 'number' ? value : -Infinity; // Take the derived value only when it ratchets up (strictly // greater — #2969's `>` not `>=`); else keep curated. @@ -448,10 +657,12 @@ function applyPreserveAlways(field: string, cls: FieldClassification, ctx: Prese } } } - ctx.postFm[field] = merged; + next = merged; } else { - ctx.postFm[field] = ctx.preFm[field]; + next = cloneCurated(curated); } + if (preservedValuesEqual(ctx.postFm[field], next)) return; + ctx.postFm[field] = next; ctx.mutated = true; } @@ -477,7 +688,7 @@ function applyPreserveIfPlaceholder(_field: string, _cls: FieldClassification, c && derivedName.length > 0 && derivedName !== MILESTONE_PLACEHOLDER && !/^[\s—–:-]/.test(derivedName); - const snapshotName = ctx.preFmSnapshot['milestone_name']; + const snapshotName = ctx.snapshot['milestone_name']; const snapshotNameIsReal = typeof snapshotName === 'string' && snapshotName.length > 0 && snapshotName !== MILESTONE_PLACEHOLDER; @@ -487,7 +698,7 @@ function applyPreserveIfPlaceholder(_field: string, _cls: FieldClassification, c ctx.postFm['milestone_name'] = snapshotName; ctx.mutated = true; } - const snapshotVersion = ctx.preFmSnapshot['milestone']; + const snapshotVersion = ctx.snapshot['milestone']; if ( typeof snapshotVersion === 'string' && snapshotVersion.length > 0 && ctx.postFm['milestone'] !== snapshotVersion @@ -517,14 +728,24 @@ function applyDerive(_field: string, _cls: FieldClassification, _ctx: Preservati * any field was restored. */ export function applyStatePreservation(input: StatePreservationInput): StatePreservationResult { + const { transaction } = input; + + // A `rebuild()` transaction still carries the snapshot (§8.7's reporting + // needs it) but must not run preservation at all: `state sync` / `REGENERATE_STATE` + // exist to let the body / factory-reset win, and restoring curated values + // over that would re-lock exactly what the command was invoked to replace. + if (transaction.kind === 'rebuild') { + return { postFm: input.postFm, mutated: false }; + } + const ctx: PreservationCtx = { - preFm: input.preFm, postFm: input.postFm, - preFmSnapshot: input.preFmSnapshot, - resync: input.resync, - deriveProgressKeys: input.deriveProgressKeys === true, - bodyDeltas: input.bodyDeltas, + snapshot: transaction.snapshot, + resync: transaction.resync, + deriveProgressKeys: transaction.deriveProgressKeys === true, + bodyDeltas: transaction.bodyDeltas, mutated: false, + explicitProgressField: transaction.explicitProgressField === true, }; for (const field of Object.keys(FIELD_CLASSIFICATION)) { diff --git a/src/state.cts b/src/state.cts index 5c0b7787f..3b11ae7d4 100644 --- a/src/state.cts +++ b/src/state.cts @@ -71,6 +71,9 @@ type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord; type PhaseInventoryResult = stateTransitionMod.PhaseInventoryResult; +// ADR-3473 §8.6: the state transaction type (see openStateTransaction / +// rebuildStateTransaction in state-transition.cts). +type StateTransaction = stateTransitionMod.StateTransaction; import { computeProgressPercent, normalizeProgressNumbers, @@ -133,6 +136,26 @@ interface ReadModifyWriteOptions { * callers that do not report per-field arrays; costs nothing extra. */ divergedFields?: string[]; + /** + * ADR-3473 §8.6 (found while diagnosing a regression against the + * pre-existing #3242 "resyncs progress frontmatter from the updated body" + * spec): true ONLY when `resync` was set to true BECAUSE the caller + * explicitly named a progress-affecting field (`Progress`, `Total Plans in + * Phase`, `Total Phases` — see `shouldResyncStateProgress`), as opposed to + * `resync` defaulting true for an unrelated write (e.g. `state + * add-decision`, `state advance-plan`). The `preserve-always` / + * `progress-ratchet` unmeasured-scan guard (`applyPreserveAlways`, + * state-transition.cts) exists to stop an INCIDENTAL resync from dropping a + * real curated block when the disk scan measured nothing (#3756, an + * archived-milestone side effect nobody asked for). It must NOT also block + * a write the user pointed AT `progress` on purpose — `preserve-always`'s + * own contract is "never overwrite unless the caller explicitly names this + * field" (FIELD_CLASSIFICATION doc comment), and `state update Progress` / + * `state patch Progress=...` are exactly that explicit naming. Set only by + * `cmdStateUpdate` / `cmdStatePatch`, the only two call sites where + * `resync` is driven by `shouldResyncStateProgress` rather than defaulting. + */ + explicitProgressField?: boolean; } /** @@ -560,7 +583,7 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea precomputed = (result.data as { updated: string[]; failed: string[] }) ?? precomputed; preSyncContent = result.content; return result.content; - }, cwd, { resync: shouldResync, divergedFields }); + }, cwd, { resync: shouldResync, divergedFields, explicitProgressField: shouldResync }); // ADR-3408 §8.4 (D4, fix(#3351) generalized — see `reconcileReportedFields`): // patchCore's bookkeeping says whether the stateReplaceField text-replace @@ -694,7 +717,7 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | } preSyncContent = result.content; return result.content; - }, cwd, { resync: shouldResync, divergedFields, authoritativeFm }); + }, cwd, { resync: shouldResync, divergedFields, authoritativeFm, explicitProgressField: shouldResync }); // ADR-3408 §8.4 (D4): reconcile against the bytes actually persisted — // `updateCore`'s own match does not know whether sync/preservation later @@ -3193,7 +3216,25 @@ function withStateLock(statePath: string, fn: () => T): T { * @param clock * Optional clock seam; defaults to realClock. Passed through to acquireStateLock. */ -function writeStateMd(statePath: string, content: string, cwd?: string, clock?: StateLockClock): void { +/** + * ADR-3473 §8.6: `writeStateMd` is ADR-3408 §8.3's sanctioned-exception write + * path — only a `rebuildStateTransaction` may travel it. Enforced here rather + * than left to caller discipline: the transaction TYPE is what makes the two + * sanctioned exceptions (`cmdStateSync`, `REGENERATE_STATE`) greppable and + * closed, and an `open()` transaction reaching this function would mean a + * preservation-governed write silently skipped preservation. + */ +function writeStateMd(statePath: string, content: string, transaction: StateTransaction, cwd?: string, clock?: StateLockClock): void { + if (transaction.kind !== 'rebuild') { + const err = new Error( + `writeStateMd: expected a 'rebuild' transaction, got '${transaction.kind}'. writeStateMd is ` + + 'ADR-3408 §8.3\'s sanctioned-exception write path (cmdStateSync / REGENERATE_STATE only) — ' + + 'only rebuildStateTransaction() may travel it (ADR-3473 §8.6). An open() transaction here ' + + 'would silently skip preservation for a write that was supposed to run it.', + ) as Error & { code: string }; + err.code = 'STATE_TRANSACTION_KIND_INVALID'; + throw err; + } const lockPath = acquireStateLock(statePath, clock); // Test seam (audit M8): fire AFTER the lock is taken so a test can simulate a // concurrent writer landing in the (now-closed) scan→lock window. @@ -3213,10 +3254,14 @@ function writeStateMd(statePath: string, content: string, cwd?: string, clock?: if (cwd) _diskScanCache.delete(cwd); // ADR-3408 §8.3: `writeStateMd` is the sole write path for the two // sanctioned-permanent exceptions (`cmdStateSync`, `REGENERATE_STATE`) — - // pass `sanctionedPermanentEmptyFallback: true` so their long-standing - // empty-field fallback behavior stays byte-identical (see - // `syncStateFrontmatter`'s docstring above the guard block). - const synced = syncStateFrontmatter(content, cwd, undefined, true); + // the sanctioned-permanent empty-field fallback is now DERIVED FROM THE + // TRANSACTION KIND (ADR-3473 §8.6) rather than asserted by a literal + // `true` at this call site: only a `rebuild` transaction can reach this + // function (enforced above), so `transaction.kind === 'rebuild'` is + // always `true` here today, but the derivation is what keeps the + // fallback's scope tied to the transaction type rather than a + // hard-coded constant that could silently drift from it. + const synced = syncStateFrontmatter(content, cwd, undefined, transaction.kind === 'rebuild'); platformWriteSync(statePath, synced); } finally { releaseStateLock(lockPath); @@ -3280,10 +3325,7 @@ function applyPostSyncPreservation( options: StatePreservationOptions, ): string { assertStatePreservationOptions(options, 'applyPostSyncPreservation'); - const { resync, authoritativeFm, deriveProgressKeys, divergedFields } = options; - // Snapshot the existing progress block BEFORE the transform so we can - // restore it when resync is false. - const preFm = resync ? null : extractFrontmatter(originalContent, statePath) as Record; + const { resync, authoritativeFm, deriveProgressKeys, divergedFields, explicitProgressField } = options; // Bug #1230: delta heuristic — snapshot pre-transform body source fields so // we can detect whether THIS write changed them. syncStateFrontmatter @@ -3399,11 +3441,20 @@ function applyPostSyncPreservation( // callers that omit it (readModifyWriteStateMd, cmdPhaseComplete) pay // nothing extra and see no change to `synced`/the returned content. const preservationInputSnapshot = divergedFields ? { ...postFm } : null; - const preservation = applyStatePreservation({ - preFm, postFm, preFmSnapshot, resync, + // ADR-3473 §8.6: the pre-write snapshot + policy flags now travel as ONE + // transaction rather than as a nullable `preFm` alongside the always-present + // `preFmSnapshot` (same source, same extractFrontmatter call — `preFm` was + // `preFmSnapshot` with the `resync` policy baked in by nulling it, which is + // what made `applyPreserveAlways` inert on the default resyncing write + // path — #3756). + const transaction = stateTransitionMod.openStateTransaction({ + snapshot: preFmSnapshot, + resync, deriveProgressKeys: deriveProgressKeys === true, bodyDeltas, + explicitProgressField: explicitProgressField === true, }); + const preservation = applyStatePreservation({ transaction, postFm }); if (divergedFields && preservationInputSnapshot) { // §8.5's "liberal but visible": every field whose value actually // differs before vs after `applyStatePreservation` is a field where the @@ -3567,6 +3618,7 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string authoritativeFm: options?.authoritativeFm, deriveProgressKeys: options?.deriveProgressKeys === true, divergedFields: options?.divergedFields, + explicitProgressField: options?.explicitProgressField === true, }, ); @@ -3795,12 +3847,22 @@ function cmdStateJson(cwd: string, raw: boolean): void { ?? parseProsePhaseField(stateExtractField(positionScope, 'Phase')).phase; const bodyCurrentPlan = stateExtractField(body, 'Current Plan'); const bodyStatus = stateExtractField(body, 'Status'); + // #3836: mirrors applyPostSyncPreservation's own derivation (state.cts + // bodyDeltas, `last_activity_desc`) — the `Last Activity Description` + // label, falling back to the prose `Last Activity:` line's parsed + // description. Read-side twin of #3258's write-side wiring; this field is + // `preserve-when-unchanged` per FIELD_CLASSIFICATION and was previously + // absent from this read path entirely (never derived here, never in the + // loop below), so a stale body annotation always beat a fresher curated + // frontmatter value on every `state json` read. + const bodyLastActivityRaw = stateExtractField(body, 'Last Activity') ?? stateExtractField(body, 'Last activity'); + const bodyLastActivityDesc = stateExtractField(body, 'Last Activity Description') + ?? parseProseLastActivityField(bodyLastActivityRaw).description; const unchanged = (v: string | null): { pre: string | null; post: string | null } => ({ pre: v, post: v }); const ctx = { - preFm: null, postFm: built, - preFmSnapshot: existingFm, + snapshot: existingFm, resync: true, deriveProgressKeys: false, bodyDeltas: { @@ -3810,10 +3872,15 @@ function cmdStateJson(cwd: string, raw: boolean): void { current_phase: unchanged(bodyCurrentPhase), current_plan: unchanged(bodyCurrentPlan), current_phase_name: unchanged(bodyPhaseSource), + last_activity_desc: unchanged(bodyLastActivityDesc), }, mutated: false, }; - for (const field of ['status', 'stopped_at', 'paused_at', 'current_phase', 'current_plan', 'current_phase_name']) { + // #3836: derive the field set from FIELD_CLASSIFICATION's + // `preserve-when-unchanged` rows (single source of truth) instead of a + // hand-typed literal that can drift from the table — this IS the fix, + // not merely an addition of one more name to the literal. + for (const field of stateTransitionMod.getPreserveWhenUnchangedFields()) { const cls = stateTransitionMod.getFieldClassification(field); if (cls) stateTransitionMod.applyPreserveWhenUnchanged(field, cls, ctx); } @@ -4190,6 +4257,16 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa // line, and the prose re-derivation of current_phase_name truncates names // that themselves contain a parenthetical — the authoritative override keeps // the exact value, exactly as cmdStateBeginPhase does for its EXECUTING line. + // + // #3834: without a name, the body-source delta rule that would normally + // preserve the curated `current_phase_name` (FIELD_CLASSIFICATION: + // preserve-when-unchanged) cannot fire — THIS write rewrites the `Phase:` + // source line to `N — READY TO EXECUTE` itself, so pre/post disagree by + // construction and the post-sync re-derivation harvests "READY TO EXECUTE" + // as if it were the name. The fix mirrors the named-arg path: reassert an + // authoritative override, falling back to the pre-write curated value (read + // inside the RMW callback, before this write's own body mutation) rather + // than leaving the field to a delta heuristic this exact transition defeats. const divergedFields: string[] = []; const rmwOptions: ReadModifyWriteOptions = { resync: false, @@ -4201,6 +4278,13 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa let precomputedUpdated: string[] = []; let preSyncContent = ''; readModifyWriteStateMd(statePath, (content) => { + if (!intent.phaseName) { + const preFm = extractFrontmatter(content, statePath) as Record; + const curatedName = preFm['current_phase_name']; + if (typeof curatedName === 'string' && curatedName.trim().length > 0) { + rmwOptions.authoritativeFm = { current_phase_name: curatedName }; + } + } const result = transitionCore(content, intent, deps); precomputedUpdated = result.updated; preSyncContent = result.content; @@ -4828,7 +4912,12 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b } if (changes.length > 0 || modified !== content) { - writeStateMd(statePath, modified, cwd); + // ADR-3473 §8.6: `rebuild()` is the typed expression of #905's contract — + // `state sync` exists to let the body win, so preservation must NOT run, + // and the snapshot is carried anyway because §8.7's reporting needs it. + writeStateMd(statePath, modified, stateTransitionMod.rebuildStateTransaction({ + snapshot: extractFrontmatter(content, statePath), + }), cwd); } output({ synced: true, changes, dry_run: false }, raw, undefined); @@ -5218,6 +5307,17 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string const updated: StateCompletePhaseUpdateEntry[] = []; let preSyncContent = ''; const divergedFields: string[] = []; + // #3835: complete-phase unconditionally rewrites the body `Phase:` line to + // `N — COMPLETE` below. That defeats current_phase_name's + // preserve-when-unchanged delta rule the same way #3834's no-`--name` + // planned-phase write does — pre/post body-source disagree BY CONSTRUCTION + // (this write is what changed the source line), so the post-sync + // re-derivation harvests nothing from "COMPLETE" and the curated key is + // dropped entirely rather than preserved. The write site already documents + // "an absent name does NOT clear an existing curated value" for the body + // (`Current Phase Name` section below) — this reasserts the same rule for + // the frontmatter key, mirroring cmdStatePlannedPhase's fix. + const rmwOptions: ReadModifyWriteOptions = { divergedFields }; const wrote = readModifyWriteStateMd(statePath, (content) => { const currentPhase = resolvedPhase; @@ -5227,6 +5327,10 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string const existingFm = extractFrontmatter(content, statePath) as Record; const hasFrontmatter = Object.keys(existingFm).length > 0; let body = stripFrontmatter(content); + const curatedPhaseName = existingFm['current_phase_name']; + if (typeof curatedPhaseName === 'string' && curatedPhaseName.trim().length > 0) { + rmwOptions.authoritativeFm = { current_phase_name: curatedPhaseName }; + } const reassemble = (b: string) => hasFrontmatter ? `---\n${reconstructFrontmatter(existingFm as unknown as Frontmatter)}\n---\n\n${b}` : b; @@ -5304,7 +5408,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string const out = reassemble(body); preSyncContent = out; return out; - }, cwd, { divergedFields }); + }, cwd, rmwOptions); // ADR-3408 §8.4 (D4): traced for this phase (design doc: "not traced in // the analysis pass"). Unlike the transitionCore-based commands, this diff --git a/tests/m8-writestatemd-scan-after-lock.test.cjs b/tests/m8-writestatemd-scan-after-lock.test.cjs index 4f3d30035..54477e889 100644 --- a/tests/m8-writestatemd-scan-after-lock.test.cjs +++ b/tests/m8-writestatemd-scan-after-lock.test.cjs @@ -37,8 +37,19 @@ const os = require('node:os'); const stateMod = require('../gsd-core/bin/lib/state.cjs'); const { writeStateMd } = stateMod; +const { rebuildStateTransaction } = require('../gsd-core/bin/lib/state-transition.cjs'); +const { extractFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); const { cleanup } = require('./helpers.cjs'); +// ADR-3473 §8.6: writeStateMd's third argument is now a transaction. Both +// tests below mirror MINIMAL_STATE_MD's own pre-write content back to itself +// (no frontmatter on disk yet, so the snapshot is legitimately {}) — this is +// the M8 concurrency invariant under test, not a preservation scenario, so +// a `rebuild()` transaction (no preservation applied) is correct here. +function rebuildTransactionFor(content) { + return rebuildStateTransaction({ snapshot: extractFrontmatter(content) }); +} + // ───────────────────────────────────────────────────────────────────────────── // Helpers // ───────────────────────────────────────────────────────────────────────────── @@ -99,7 +110,7 @@ describe('M8: writeStateMd scans disk AFTER acquiring the lock (scan-in-lock)', }, }); - writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir); + writeStateMd(statePath, MINIMAL_STATE_MD, rebuildTransactionFor(MINIMAL_STATE_MD), tmpDir); assert.equal(fired, 1, 'afterAcquire hook must fire exactly once inside writeStateMd'); @@ -116,7 +127,7 @@ describe('M8: writeStateMd scans disk AFTER acquiring the lock (scan-in-lock)', test('single-threaded callers (no hook) are byte-for-behaviour unchanged: count = 1', () => { // Regression guard: with no concurrent writer (hook unset), the count must be // exactly the on-disk truth — the fix must NOT change the uncontended result. - writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir); + writeStateMd(statePath, MINIMAL_STATE_MD, rebuildTransactionFor(MINIMAL_STATE_MD), tmpDir); assert.equal( readTotalPlans(statePath), 1, 'uncontended writeStateMd must stamp the real on-disk plan count (1)' diff --git a/tests/perf-316-state-lock-buffer-alloc.test.cjs b/tests/perf-316-state-lock-buffer-alloc.test.cjs index 2fab704fd..2edae5fc2 100644 --- a/tests/perf-316-state-lock-buffer-alloc.test.cjs +++ b/tests/perf-316-state-lock-buffer-alloc.test.cjs @@ -41,6 +41,15 @@ const { cleanup } = require('./helpers.cjs'); const STATE_CJS_PATH = path.join( __dirname, '..', 'gsd-core', 'bin', 'lib', 'state.cjs' ); +// ADR-3473 §8.6: the writer worker's writeStateMd call now needs a +// rebuildStateTransaction() — resolved as a separate require path since the +// worker script runs in its own isolated module scope. +const STATE_TRANSITION_CJS_PATH = path.join( + __dirname, '..', 'gsd-core', 'bin', 'lib', 'state-transition.cjs' +); +const FRONTMATTER_CJS_PATH = path.join( + __dirname, '..', 'gsd-core', 'bin', 'lib', 'frontmatter.cjs' +); const MINIMAL_STATE_MD = [ '# Project State', @@ -123,10 +132,21 @@ fs.openSync = function(filePath, flags, mode) { // any module-level SAB allocations from a prior require contaminating sabCount.) delete require.cache[workerData.stateCjsPath]; const { writeStateMd } = require(workerData.stateCjsPath); +const { rebuildStateTransaction } = require(workerData.stateTransitionCjsPath); +const { extractFrontmatter } = require(workerData.frontmatterCjsPath); let callErr = null; try { - writeStateMd(workerData.statePath, workerData.content, workerData.tmpDir); + // ADR-3473 §8.6: writeStateMd now requires a rebuild() transaction — this + // worker's write mirrors REGENERATE_STATE's shape (a fresh, no-frontmatter + // MINIMAL_STATE_MD body), so the snapshot is of whatever (if anything) + // already exists on disk at statePath before this write. + const priorContent = fs.existsSync(workerData.statePath) + ? fs.readFileSync(workerData.statePath, 'utf8') + : ''; + writeStateMd(workerData.statePath, workerData.content, rebuildStateTransaction({ + snapshot: extractFrontmatter(priorContent, workerData.statePath), + }), workerData.tmpDir); } catch (e) { callErr = (e && e.message) ? e.message : String(e); } @@ -244,6 +264,8 @@ describe('perf #316: acquireStateLock hoists sleep buffer — exactly one SAB pe eval: true, workerData: { stateCjsPath: STATE_CJS_PATH, + stateTransitionCjsPath: STATE_TRANSITION_CJS_PATH, + frontmatterCjsPath: FRONTMATTER_CJS_PATH, statePath, content: MINIMAL_STATE_MD, tmpDir, diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 54bac6d92..ea5be8b65 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -14,6 +14,8 @@ const fc = require('fast-check'); const { transitionCore, applyStatePreservation, + openStateTransaction, + rebuildStateTransaction, FIELD_CLASSIFICATION, getFieldClassification, STATE_MD_SECTIONS, @@ -1477,11 +1479,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // Default behavior: wholesale curated restore. #3242 Bug A protection. const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, - postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, // disk-derived clobber - resync: false, - ...untouched, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + ...untouched, + }), + postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } } // disk-derived clobber, }); assert.deepEqual(r.postFm.progress, { total_phases: 4, completed_phases: 3, percent: 75 }); assert.equal(r.mutated, true); @@ -1493,12 +1496,13 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // completed_phases keep curated protection. const curated = { progress: { total_plans: 50, completed_plans: 50, total_phases: 2, completed_phases: 1, percent: 100 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + deriveProgressKeys: true, + ...untouched, + }), postFm: { progress: { total_plans: 64, completed_plans: 49, total_phases: 2, completed_phases: 1, percent: 77 } }, - resync: false, - deriveProgressKeys: true, - ...untouched, }); assert.equal(r.postFm.progress.total_plans, 64, 'total_plans must take derived value (64) when deriveProgressKeys=true (#2440)'); @@ -1512,12 +1516,13 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('#2440 boundary: deriveProgressKeys=true, total_plans derived == curated → identity', () => { const curated = { progress: { total_plans: 64, completed_plans: 49 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + deriveProgressKeys: true, + ...untouched, + }), postFm: { progress: { total_plans: 64, completed_plans: 49, percent: 77 } }, - resync: false, - deriveProgressKeys: true, - ...untouched, }); assert.equal(r.postFm.progress.total_plans, 64, 'total_plans equality → derived value (identity)'); @@ -1531,12 +1536,13 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // completed_plans < total_plans forever even though every plan is summarized. const curated = { progress: { total_plans: 54, completed_plans: 50, total_phases: 2, completed_phases: 1, percent: 93 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + deriveProgressKeys: true, + ...untouched, + }), postFm: { progress: { total_plans: 54, completed_plans: 54, total_phases: 2, completed_phases: 1, percent: 100 } }, - resync: false, - deriveProgressKeys: true, - ...untouched, }); assert.equal(r.postFm.progress.total_plans, 54, 'total_plans takes derived value'); assert.equal(r.postFm.progress.completed_plans, 54, @@ -1551,12 +1557,13 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // derive downward. (#3242 curated-progress protection, scoped to deriveProgressKeys.) const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + deriveProgressKeys: true, + ...untouched, + }), postFm: { progress: { total_plans: 54, completed_plans: 47, percent: 87 } }, - resync: false, - deriveProgressKeys: true, - ...untouched, }); assert.equal(r.postFm.progress.completed_plans, 50, 'completed_plans must NOT derive downward (47 < curated 50) — ratchet-up only (#2969/#3242)'); @@ -1567,12 +1574,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons // wholesale curated restore — completed_plans never moves for a body-only edit. const curated = { progress: { total_plans: 54, completed_plans: 50, percent: 93 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, // deriveProgressKeys NOT set — body-only write path + ...untouched, + }), postFm: { progress: { total_plans: 54, completed_plans: 54, percent: 100 } }, - resync: false, - // deriveProgressKeys NOT set — body-only write path - ...untouched, }); assert.equal(r.postFm.progress.completed_plans, 50, 'body-only write must keep curated completed_plans (no deriveProgressKeys) (#2969/#3242)'); @@ -1581,11 +1588,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('progress: NOT restored when transition re-derives from disk (resync=true) — sync/advancePlan/completePhase path', () => { const recomputed = { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }; const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, + transaction: openStateTransaction({ + snapshot: {}, + resync: true, + ...untouched, + }), postFm: { ...recomputed }, - resync: true, - ...untouched, }); assert.deepEqual(r.postFm.progress, { total_phases: 5, completed_phases: 1, percent: 20 }); assert.equal(r.mutated, false); @@ -1593,11 +1601,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('status: preserves when body Status source is unchanged (preserve-when-unchanged) and snapshot holds a real status', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { status: 'completed' }, + transaction: openStateTransaction({ + snapshot: { status: 'completed' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Executing Phase 3' } }, + }), postFm: { status: 'verifying' }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Executing Phase 3' } }, }); assert.equal(r.postFm.status, 'completed'); assert.equal(r.mutated, true); @@ -1605,11 +1614,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('status: does NOT preserve when the body Status source line changed this write', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { status: 'completed' }, + transaction: openStateTransaction({ + snapshot: { status: 'completed' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Completed Phase 3' } }, + }), postFm: { status: 'verifying' }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), status: { pre: 'Executing Phase 3', post: 'Completed Phase 3' } }, // changed }); assert.equal(r.postFm.status, 'verifying'); assert.equal(r.mutated, false); @@ -1617,11 +1627,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('current_phase_name: preserves curated value when body Phase source unchanged (preserve-when-unchanged, #3468 reclassified)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_phase_name: 'curated-name' }, + transaction: openStateTransaction({ + snapshot: { current_phase_name: 'curated-name' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, + }), postFm: { current_phase_name: 'wrong-parenthetical-harvest' }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, }); assert.equal(r.postFm.current_phase_name, 'curated-name'); assert.equal(r.mutated, true); @@ -1630,11 +1641,12 @@ describe('ADR-1769 #1796: applyStatePreservation — table-driven post-sync cons test('returns mutated=false and untouched postFm when no preservation rule applies', () => { const postFm = { status: 'executing', progress: { percent: 10 } }; const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, - postFm, - resync: true, - ...untouched, + transaction: openStateTransaction({ + snapshot: {}, + resync: true, + ...untouched, + }), + postFm: postFm, }); assert.equal(r.mutated, false); assert.deepEqual(r.postFm, { status: 'executing', progress: { percent: 10 } }); @@ -1691,17 +1703,24 @@ describe('#3258: applyStatePreservation honors every declared preservation row', // reclassified to preserve-when-unchanged in #3468 — ADR-3408 §8.1). const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; const r = applyStatePreservation({ - preFm: curated, preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + bodyDeltas: unchangedBodyDeltas, + }), postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, - resync: false, bodyDeltas: unchangedBodyDeltas, }); return JSON.stringify(r.postFm.progress) === JSON.stringify(curated.progress); } if (policy === 'preserve-when-unchanged') { const r = applyStatePreservation({ - preFm: null, preFmSnapshot: { [field]: GOOD }, - postFm: { [field]: BAD }, resync: true, bodyDeltas: unchangedBodyDeltas, + transaction: openStateTransaction({ + snapshot: { [field]: GOOD }, + resync: true, + bodyDeltas: unchangedBodyDeltas, + }), + postFm: { [field]: BAD }, }); return r.postFm[field] === GOOD; } @@ -1711,10 +1730,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', // name+version. Mirrors the #948/#2135 contract: name restored to the // curated snapshot, version restored alongside it. const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: GOOD, milestone_name: GOOD }, + transaction: openStateTransaction({ + snapshot: { milestone: GOOD, milestone_name: GOOD }, + resync: true, + bodyDeltas: unchangedBodyDeltas, + }), postFm: { milestone: 'derived-version', milestone_name: 'milestone' }, - resync: true, bodyDeltas: unchangedBodyDeltas, }); return r.postFm[field] === GOOD; } @@ -1757,11 +1778,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', // error, never a silent no-op indistinguishable from a correct skip. assert.throws( () => applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: 'preserved-by-table' }, + transaction: openStateTransaction({ + snapshot: { current_plan: 'preserved-by-table' }, + resync: true, + bodyDeltas: {}, + }), postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas: {}, // caller forgot to wire current_plan's body-source delta }), (err) => { assert.strictEqual(err.code, 'STATE_PRESERVATION_UNWIRED_ROW'); @@ -1778,11 +1800,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', test('last_activity_desc: preserve-when-unchanged restores snapshot when body source unchanged', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { last_activity_desc: 'authoritative description' }, + transaction: openStateTransaction({ + snapshot: { last_activity_desc: 'authoritative description' }, + resync: true, + bodyDeltas: { ...unchangedBodyDeltas }, + }), postFm: { last_activity_desc: 'stale derived description' }, - resync: true, - bodyDeltas: { ...unchangedBodyDeltas }, }); assert.equal(r.postFm.last_activity_desc, 'authoritative description'); assert.equal(r.mutated, true); @@ -1790,11 +1813,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', test('last_activity_desc: derived wins when the body source changed this write (no over-preservation)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { last_activity_desc: 'old description' }, + transaction: openStateTransaction({ + snapshot: { last_activity_desc: 'old description' }, + resync: true, + bodyDeltas: { ...lastActivityDescChangedDeltas }, + }), postFm: { last_activity_desc: 'new description from transition' }, - resync: true, - bodyDeltas: { ...lastActivityDescChangedDeltas }, // body 'Last Activity Description' moved }); assert.equal(r.postFm.last_activity_desc, 'new description from transition'); assert.equal(r.mutated, false); @@ -1805,11 +1829,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', // absent-fallback. Derived is PRESENT but stale; body source unchanged → // curated frontmatter value wins. const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { paused_at: '2026-02-02' }, + transaction: openStateTransaction({ + snapshot: { paused_at: '2026-02-02' }, + resync: true, + bodyDeltas: { ...unchangedBodyDeltas }, + }), postFm: { paused_at: '2026-01-01' }, - resync: true, - bodyDeltas: { ...unchangedBodyDeltas }, }); assert.equal(r.postFm.paused_at, '2026-02-02'); assert.equal(r.mutated, true); @@ -1817,11 +1842,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', test('current_phase: preserve-when-unchanged restores curated value over a stale derived value', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_phase: '4' }, + transaction: openStateTransaction({ + snapshot: { current_phase: '4' }, + resync: true, + bodyDeltas: { ...unchangedBodyDeltas }, + }), postFm: { current_phase: '2' }, - resync: true, - bodyDeltas: { ...unchangedBodyDeltas }, }); assert.equal(r.postFm.current_phase, '4'); assert.equal(r.mutated, true); @@ -1829,11 +1855,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', test('current_plan: preserve-when-unchanged restores curated value over a stale derived value', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: '5' }, + transaction: openStateTransaction({ + snapshot: { current_plan: '5' }, + resync: true, + bodyDeltas: { ...unchangedBodyDeltas }, + }), postFm: { current_plan: '3' }, - resync: true, - bodyDeltas: { ...unchangedBodyDeltas }, }); assert.equal(r.postFm.current_plan, '5'); assert.equal(r.mutated, true); @@ -1841,11 +1868,12 @@ describe('#3258: applyStatePreservation honors every declared preservation row', test('milestone / milestone_name: preserve-if-placeholder restores curated name when derived is placeholder', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '0.1', milestone_name: 'Real Curated Name' }, - postFm: { milestone: '0.x', milestone_name: 'milestone' }, // placeholder derive - resync: true, - bodyDeltas: { ...unchangedBodyDeltas }, + transaction: openStateTransaction({ + snapshot: { milestone: '0.1', milestone_name: 'Real Curated Name' }, + resync: true, + bodyDeltas: { ...unchangedBodyDeltas }, + }), + postFm: { milestone: '0.x', milestone_name: 'milestone' } // placeholder derive, }); assert.equal(r.postFm.milestone_name, 'Real Curated Name', 'placeholder-derived milestone_name must yield to the curated snapshot (#948/#2135 contract)'); @@ -1897,11 +1925,12 @@ const dedicatedNoop = { describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boundary/hostile rows', () => { test('A3: preserve-when-unchanged — an empty-string snapshot is not restored', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: '' }, + transaction: openStateTransaction({ + snapshot: { current_plan: '' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.current_plan, 'derived'); @@ -1914,11 +1943,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun // tree until the refactor tightens the guard. test('A4: preserve-when-unchanged — a whitespace-only snapshot is not restored (".length > 0" is not enough)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: ' ' }, + transaction: openStateTransaction({ + snapshot: { current_plan: ' ' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.current_plan, 'derived', @@ -1929,11 +1959,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A5: preserve-when-unchanged — a non-string snapshot is ignored (no throw)', () => { for (const snapshot of [42, true, null, { nested: 1 }, undefined]) { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: snapshot }, + transaction: openStateTransaction({ + snapshot: { current_plan: snapshot }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.current_plan, 'derived', `snapshot=${JSON.stringify(snapshot)} must not be restored`); @@ -1942,11 +1973,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A6: preserve-when-unchanged — no-op when postFm already equals the snapshot', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: 'same-value' }, + transaction: openStateTransaction({ + snapshot: { current_plan: 'same-value' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { current_plan: 'same-value' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.current_plan, 'same-value'); @@ -1955,11 +1987,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A7: preserve-when-unchanged — a postFm missing the key entirely is restored (undefined !== snapshot)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: 'curated' }, + transaction: openStateTransaction({ + snapshot: { current_plan: 'curated' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: {}, // key absent entirely - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.current_plan, 'curated'); @@ -1968,14 +2001,18 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A8: status — the "unknown" sentinel snapshot is never restored', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { status: 'unknown' }, + transaction: openStateTransaction({ + snapshot: { status: 'unknown' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { status: 'verifying' }, - resync: true, - preBodyStatus: 'x', postBodyStatus: 'x', - preBodyStoppedAt: 'x', postBodyStoppedAt: 'x', - preBodyPhaseSource: 'x', postBodyPhaseSource: 'x', - bodyDeltas: neutralBodyDeltas(), + preBodyStatus: 'x', + postBodyStatus: 'x', + preBodyStoppedAt: 'x', + postBodyStoppedAt: 'x', + preBodyPhaseSource: 'x', + postBodyPhaseSource: 'x', }); assert.equal(r.postFm.status, 'verifying'); assert.equal(r.mutated, false); @@ -1983,14 +2020,18 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A9: status — the "unknown" sentinel guard is case-sensitive ("Unknown" is a real value, restored)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { status: 'Unknown' }, + transaction: openStateTransaction({ + snapshot: { status: 'Unknown' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { status: 'verifying' }, - resync: true, - preBodyStatus: 'x', postBodyStatus: 'x', - preBodyStoppedAt: 'x', postBodyStoppedAt: 'x', - preBodyPhaseSource: 'x', postBodyPhaseSource: 'x', - bodyDeltas: neutralBodyDeltas(), + preBodyStatus: 'x', + postBodyStatus: 'x', + preBodyStoppedAt: 'x', + postBodyStoppedAt: 'x', + preBodyPhaseSource: 'x', + postBodyPhaseSource: 'x', }); assert.equal(r.postFm.status, 'Unknown', 'the sentinel is an exact-match on the lowercase literal "unknown" — a case variant is a real value'); @@ -2000,11 +2041,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A12: preserve-always progress — preFm===null under !resync does not throw (skip)', () => { assert.doesNotThrow(() => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, + transaction: openStateTransaction({ + snapshot: {}, + resync: false, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { progress: { total_phases: 5, completed_phases: 1, percent: 20 } }, - resync: false, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.deepEqual(r.postFm.progress, { total_phases: 5, completed_phases: 1, percent: 20 }); @@ -2015,12 +2057,13 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A14: deriveProgressKeys — derived completed_plans === curated keeps curated (limit: ">" not ">=", #2969)', () => { const curated = { progress: { total_plans: 10, completed_plans: 7, percent: 70 } }; const r = applyStatePreservation({ - preFm: curated, - preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + deriveProgressKeys: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { progress: { total_plans: 10, completed_plans: 7, percent: 70 } }, // derived === curated - resync: false, - deriveProgressKeys: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.progress.completed_plans, 7, @@ -2030,11 +2073,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A18: preserve-if-placeholder — punctuation-led derived names are rejected for every delimiter', () => { for (const derived of ['— Foo', ': Foo', '-Foo']) { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + transaction: openStateTransaction({ + snapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { milestone: '1.1', milestone_name: derived }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.milestone_name, 'Real Curated Name', @@ -2044,11 +2088,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A19: preserve-if-placeholder — an empty-string derived name is rejected (restored)', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + transaction: openStateTransaction({ + snapshot: { milestone: '1.0', milestone_name: 'Real Curated Name' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { milestone: '1.1', milestone_name: '' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.milestone_name, 'Real Curated Name'); @@ -2056,11 +2101,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A20: preserve-if-placeholder — a placeholder snapshot is not restored over a placeholder derived value', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '1.0', milestone_name: 'milestone' }, // snapshot IS the placeholder + transaction: openStateTransaction({ + snapshot: { milestone: '1.0', milestone_name: 'milestone' }, // snapshot IS the placeholder + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { milestone: '1.1', milestone_name: 'milestone' }, // derived is also the placeholder - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.milestone_name, 'milestone', 'nothing better to restore — value passes through unchanged'); @@ -2069,11 +2115,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A21: preserve-if-placeholder — a real, different derived name wins over the curated snapshot', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '1.0', milestone_name: 'Old Curated Name' }, + transaction: openStateTransaction({ + snapshot: { milestone: '1.0', milestone_name: 'Old Curated Name' }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { milestone: '2.0', milestone_name: 'New Real Milestone Name' }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm.milestone_name, 'New Real Milestone Name'); @@ -2094,11 +2141,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun for (const field of ['last_updated', 'state_head', 'gsd_state_version', 'last_activity']) { assert.doesNotThrow(() => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { [field]: 'curated-value' }, + transaction: openStateTransaction({ + snapshot: { [field]: 'curated-value' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + }), postFm: { [field]: 'freshly-derived-value' }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, ...dedicatedNoop, }); assert.equal(r.postFm[field], 'freshly-derived-value', @@ -2109,11 +2157,12 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun test('A23: a field with no FIELD_CLASSIFICATION row passes through untouched', () => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { totally_unclassified_field: 'curated' }, + transaction: openStateTransaction({ + snapshot: { totally_unclassified_field: 'curated' }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), totally_unclassified_field: { pre: 'x', post: 'y' } }, + }), postFm: { totally_unclassified_field: 'derived' }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), totally_unclassified_field: { pre: 'x', post: 'y' } }, ...dedicatedNoop, }); assert.equal(r.postFm.totally_unclassified_field, 'derived'); @@ -2131,16 +2180,17 @@ describe('#3468 matrix A: executor policy dispatch (ADR-3408 §8.1) — new/boun // instead of mutating the object's prototype. assert.doesNotThrow(() => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { ['__proto__']: 'x', ['constructor']: 'y', ['toString']: 'z' }, + transaction: openStateTransaction({ + snapshot: { ['__proto__']: 'x', ['constructor']: 'y', ['toString']: 'z' }, + resync: true, + bodyDeltas: { + ...neutralBodyDeltas(), + ['__proto__']: { pre: 'x', post: 'y' }, + ['constructor']: { pre: 'x', post: 'y' }, + ['toString']: { pre: 'x', post: 'y' }, + }, + }), postFm: { ['__proto__']: 'a', ['constructor']: 'b', ['toString']: 'c' }, - resync: true, - bodyDeltas: { - ...neutralBodyDeltas(), - ['__proto__']: { pre: 'x', post: 'y' }, - ['constructor']: { pre: 'x', post: 'y' }, - ['toString']: { pre: 'x', post: 'y' }, - }, ...dedicatedNoop, }); assert.equal(typeof r.postFm, 'object'); @@ -2169,11 +2219,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § delete bodyDeltas.current_plan; // the ONLY unwired row assert.throws( () => applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_plan: 'curated' }, + transaction: openStateTransaction({ + snapshot: { current_plan: 'curated' }, + resync: true, + bodyDeltas: bodyDeltas, + }), postFm: { current_plan: 'derived' }, - resync: true, - bodyDeltas, ...dedicatedNoop, }), (err) => { @@ -2187,11 +2238,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § test('B2: bodyDeltas entirely absent throws, naming the first unwired row', () => { assert.throws( () => applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, + transaction: openStateTransaction({ + snapshot: {}, + resync: true, + // bodyDeltas omitted entirely + }), postFm: {}, - resync: true, - // bodyDeltas omitted entirely ...dedicatedNoop, }), (err) => { @@ -2210,11 +2262,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § test('B3: bodyDeltas present but {} throws, naming the first unwired row', () => { assert.throws( () => applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, + transaction: openStateTransaction({ + snapshot: {}, + resync: true, + bodyDeltas: {}, + }), postFm: {}, - resync: true, - bodyDeltas: {}, ...dedicatedNoop, }), (err) => { @@ -2232,11 +2285,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § }; assert.doesNotThrow(() => { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, // no snapshot — skip is a legitimate, non-throwing outcome + transaction: openStateTransaction({ + snapshot: {}, // no snapshot — skip is a legitimate, non-throwing outcome + resync: true, + bodyDeltas: bodyDeltas, + }), postFm: { current_phase: 'derived' }, - resync: true, - bodyDeltas, ...dedicatedNoop, }); assert.equal(r.postFm.current_phase, 'derived'); @@ -2250,11 +2304,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § }; assert.doesNotThrow(() => { applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_phase: 'curated' }, + transaction: openStateTransaction({ + snapshot: { current_phase: 'curated' }, + resync: true, + bodyDeltas: bodyDeltas, + }), postFm: { current_phase: 'derived' }, - resync: true, - bodyDeltas, ...dedicatedNoop, }); }); @@ -2266,11 +2321,12 @@ describe('#3468 matrix B: an unenforced preserve-when-unchanged row (ADR-3408 § // are deliberately absent from bodyDeltas and must not trigger the throw. assert.doesNotThrow(() => { applyStatePreservation({ - preFm: null, - preFmSnapshot: {}, + transaction: openStateTransaction({ + snapshot: {}, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: {}, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); }); @@ -2301,11 +2357,12 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou test('C1: shared preserve-when-unchanged loop — delta unchanged restores the snapshot (pinned)', () => { for (const field of LOOP_PWU_FIELDS) { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { [field]: GOOD }, + transaction: openStateTransaction({ + snapshot: { [field]: GOOD }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { [field]: BAD }, - resync: true, - bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, }); assert.equal(r.postFm[field], GOOD, `${field}: pinned identity — restore-when-unchanged`); @@ -2315,11 +2372,12 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou test('C1: shared preserve-when-unchanged loop — delta changed lets derived win (pinned)', () => { for (const field of LOOP_PWU_FIELDS) { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { [field]: GOOD }, + transaction: openStateTransaction({ + snapshot: { [field]: GOOD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + }), postFm: { [field]: BAD }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, ...dedicatedNoop, }); assert.equal(r.postFm[field], BAD, `${field}: pinned identity — derived wins when body source changed`); @@ -2328,14 +2386,22 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou test('C1: status / stopped_at (pinned)', () => { const rStatus = applyStatePreservation({ - preFm: null, preFmSnapshot: { status: GOOD }, postFm: { status: BAD }, resync: true, - bodyDeltas: neutralBodyDeltas(), + transaction: openStateTransaction({ + snapshot: { status: GOOD }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), + postFm: { status: BAD }, }); assert.equal(rStatus.postFm.status, GOOD); const rStopped = applyStatePreservation({ - preFm: null, preFmSnapshot: { stopped_at: GOOD }, postFm: { stopped_at: BAD }, resync: true, - bodyDeltas: neutralBodyDeltas(), + transaction: openStateTransaction({ + snapshot: { stopped_at: GOOD }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), + postFm: { stopped_at: BAD }, }); assert.equal(rStopped.postFm.stopped_at, GOOD); }); @@ -2343,17 +2409,24 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou test('C1: preserve-always progress and preserve-if-placeholder milestone/milestone_name (pinned)', () => { const curated = { progress: { total_phases: 4, completed_phases: 3, percent: 75 } }; const rProgress = applyStatePreservation({ - preFm: curated, preFmSnapshot: curated, + transaction: openStateTransaction({ + snapshot: curated, + resync: false, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { progress: { total_phases: 5, completed_phases: 0, percent: 0 } }, - resync: false, bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, + ...dedicatedNoop, }); assert.deepEqual(rProgress.postFm.progress, curated.progress); const rMilestone = applyStatePreservation({ - preFm: null, - preFmSnapshot: { milestone: '1.0', milestone_name: GOOD }, + transaction: openStateTransaction({ + snapshot: { milestone: '1.0', milestone_name: GOOD }, + resync: true, + bodyDeltas: neutralBodyDeltas(), + }), postFm: { milestone: '1.1', milestone_name: 'milestone' }, - resync: true, bodyDeltas: neutralBodyDeltas(), ...dedicatedNoop, + ...dedicatedNoop, }); assert.equal(rMilestone.postFm.milestone_name, GOOD); }); @@ -2361,11 +2434,12 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou test('C1: derive-classified fields pass through untouched regardless of snapshot (pinned)', () => { for (const field of ['gsd_state_version', 'last_updated', 'last_activity', 'state_head']) { const r = applyStatePreservation({ - preFm: null, - preFmSnapshot: { [field]: GOOD }, + transaction: openStateTransaction({ + snapshot: { [field]: GOOD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, + }), postFm: { [field]: BAD }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), [field]: { pre: 'old', post: 'new' } }, ...dedicatedNoop, }); assert.equal(r.postFm[field], BAD, `${field}: derive rows always take the derived (postFm) value`); @@ -2384,22 +2458,24 @@ describe('#3468 matrix C1/C2: identity across the refactor — pinned literal ou // to drive both an unchanged AND a changed delta for the SAME field. test('C2: current_phase_name (reclassified preserve-always → preserve-when-unchanged) — pinned outputs', () => { const rEqual = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_phase_name: GOOD }, + transaction: openStateTransaction({ + snapshot: { current_phase_name: GOOD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, + }), postFm: { current_phase_name: BAD }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '3' } }, // body Phase: source unchanged this write }); assert.equal(rEqual.postFm.current_phase_name, GOOD, 'reclassification must not change this: unchanged body Phase source still restores the curated name'); assert.equal(rEqual.mutated, true); const rDiffer = applyStatePreservation({ - preFm: null, - preFmSnapshot: { current_phase_name: GOOD }, + transaction: openStateTransaction({ + snapshot: { current_phase_name: GOOD }, + resync: true, + bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '4' } }, + }), postFm: { current_phase_name: BAD }, - resync: true, - bodyDeltas: { ...neutralBodyDeltas(), current_phase_name: { pre: '3', post: '4' } }, // body Phase: source changed this write }); assert.equal(rDiffer.postFm.current_phase_name, BAD, 'reclassification must not change this: changed body Phase source still lets derived win'); @@ -2432,11 +2508,12 @@ describe('#3468 matrix C3: executor dispatch is a pure function of the row polic const postFm = { [field]: postFmValue }; const delta = { pre: 'source', post: deltaChanged ? 'source-changed' : 'source' }; const r = applyStatePreservation({ - preFm: null, - preFmSnapshot, - postFm, - resync, - bodyDeltas: { ...neutralBodyDeltas(), [field]: delta }, + transaction: openStateTransaction({ + snapshot: preFmSnapshot, + resync: resync, + bodyDeltas: { ...neutralBodyDeltas(), [field]: delta }, + }), + postFm: postFm, ...dedicatedNoop, }); @@ -3028,3 +3105,402 @@ describe('state transitions do not consult a progress provider (#3118)', () => { )); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3871 / #3756 (ADR-3473 §8.6): the `preserve-always` executor for `progress` +// (state-transition.cjs ~line 280) gates on `ctx.resync || !ctx.preFm || +// !ctx.preFm[field]` — it reads `ctx.preFm`, never `ctx.preFmSnapshot`. But +// `applyPostSyncPreservation` (src/state.cts) computes +// `const preFm = resync ? null : extractFrontmatter(...)`, and +// `readModifyWriteStateMd` defaults `resync` to `true` — so on every +// resyncing write (record-session, add-decision, etc.) `ctx.preFm` is +// ALWAYS null and the preserve-always branch for `progress` returns +// immediately without ever consulting the curated snapshot, even though the +// snapshot (`ctx.preFmSnapshot`) still carries the pre-write curated block. +// This is the root cause of #3756 (progress zeroed on an archived milestone). +// +// Written against TODAY's `applyStatePreservation` signature +// (`{ preFm, postFm, preFmSnapshot, resync, bodyDeltas }`) so it fails for +// the RIGHT reason (the policy not running) rather than an import/signature +// mismatch. The signature is expected to change in the follow-up fix commit +// (the executor should consult `preFmSnapshot` when `preFm` is null due to +// resync), at which point this test migrates alongside it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3871 / #3756: preserve-always must still run on a resyncing write', () => { + test('preserveAlwaysRunsOnResyncingWrites', () => { + // #3756's exact repro shape: STATE.md carries a curated progress block + // (5/5/32/32/100%) but the post-sync disk-derived block from a + // milestone-scoped scan that found NONE of the current milestone's + // phases (they were archived to .planning/milestones/-phases/) is + // all STRING zeros with `percent` entirely ABSENT — exactly what the + // real sync path emits (never numeric zeros). + const curatedSnapshot = { + progress: { + total_phases: 5, + completed_phases: 5, + total_plans: 32, + completed_plans: 32, + percent: 100, + }, + }; + const zeroedPostFm = { + progress: { + total_phases: '0', + completed_phases: '0', + total_plans: '0', + completed_plans: '0', + }, + }; + const unchangedBodyDeltas = { + status: { pre: 'x', post: 'x' }, + stopped_at: { pre: 'x', post: 'x' }, + current_phase_name: { pre: 'x', post: 'x' }, + paused_at: { pre: 'x', post: 'x' }, + current_phase: { pre: 'x', post: 'x' }, + current_plan: { pre: 'x', post: 'x' }, + last_activity_desc: { pre: 'x', post: 'x' }, + }; + + const r = applyStatePreservation({ + transaction: openStateTransaction({ + snapshot: curatedSnapshot, + resync: true, + bodyDeltas: unchangedBodyDeltas, + }), + postFm: zeroedPostFm, + }); + + // FAILS TODAY (#3756/#3871): the preserve-always executor for `progress` + // only ever consults `ctx.preFm` (always null on a resyncing write, per + // the root-cause comment above), so it returns immediately and the + // curated block above is NOT restored — `r.postFm.progress` stays the + // zeroed/percent-less disk-derived block instead. + assert.deepStrictEqual( + r.postFm.progress, + curatedSnapshot.progress, + 'preserve-always for `progress` must restore the curated snapshot on a resyncing write, not just a non-resyncing one (#3756/#3871)', + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// ADR-3473 §8.6 test matrix (.gsd/phase/feat-3871-state-transaction-snapshot/ +// 50-test-matrix.md), rows 13-27 and 36: construction failures for +// `openStateTransaction` / `rebuildStateTransaction`, the legal-empty / +// null-prototype / prototype-pollution snapshot boundary, the aliasing fix +// (cloneCurated), the mutated-only-on-real-change fix, and the +// measured-vs-unmeasured coercion boundary that `applyPreserveAlways`'s +// `scanMeasuredSomething` decides on. Every call below wires +// `neutralBodyDeltasForMatrix()` on the transaction (not on the +// `applyStatePreservation` input — `bodyDeltas` is a TRANSACTION field, +// read via `transaction.bodyDeltas`) so a probe of ONE field never trips +// the §8.2 unwired-row throw for an unrelated preserve-when-unchanged row. +// ───────────────────────────────────────────────────────────────────────────── + +// Wires every currently-declared preserve-when-unchanged field with a +// neutral "unchanged this write" delta — local copy of the same pattern +// `neutralBodyDeltas()` (below, hoisted) already established in this file, +// kept separately named so this matrix block reads standalone. +function neutralBodyDeltasForMatrix() { + const deltas = {}; + for (const [field, cls] of Object.entries(FIELD_CLASSIFICATION)) { + if (cls.preservation === 'preserve-when-unchanged') { + deltas[field] = { pre: 'unchanged-source', post: 'unchanged-source' }; + } + } + return deltas; +} + +describe('ADR-3473 §8.6 matrix rows 18-21: construction is a typed failure, distinct from a legal empty snapshot', () => { + test('openWithoutSnapshotIsAConstructionFailure', () => { + assert.throws( + () => openStateTransaction({}), + (err) => { + assert.strictEqual( + Object.prototype.hasOwnProperty.call(err, 'code'), + true, + 'error must carry an own `code` property', + ); + assert.strictEqual(err.code, 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'); + assert.strictEqual(err.constructorName, 'openStateTransaction', 'the typed field must name the constructor, not just the message prose'); + return true; + }, + 'open({}) — no snapshot key at all — must throw the typed construction failure', + ); + }); + + test('openWithNullSnapshotIsAConstructionFailure', () => { + for (const snapshot of [null, undefined]) { + assert.throws( + () => openStateTransaction({ snapshot }), + (err) => { + assert.strictEqual(err.code, 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'); + assert.strictEqual(err.constructorName, 'openStateTransaction'); + return true; + }, + `open({snapshot: ${JSON.stringify(snapshot)}}) must throw the typed construction failure`, + ); + } + }); + + test('rebuildWithNullSnapshotIsAConstructionFailure', () => { + // The ADR's explicit wording: BOTH constructors, not just open(). + for (const init of [{ snapshot: null }, { snapshot: undefined }, {}]) { + assert.throws( + () => rebuildStateTransaction(init), + (err) => { + assert.strictEqual(err.code, 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'); + assert.strictEqual(err.constructorName, 'rebuildStateTransaction', 'rebuild()\'s own typed failure must name rebuildStateTransaction, not open'); + return true; + }, + `rebuildStateTransaction(${JSON.stringify(init)}) must throw the typed construction failure`, + ); + } + }); + + test('openRejectsNonObjectSnapshot', () => { + for (const snapshot of [[], 'str', 42]) { + assert.throws( + () => openStateTransaction({ snapshot }), + (err) => { + assert.strictEqual(err.code, 'STATE_TRANSACTION_SNAPSHOT_REQUIRED'); + assert.strictEqual(err.constructorName, 'openStateTransaction'); + return true; + }, + `open({snapshot: ${JSON.stringify(snapshot)}}) — a non-object (array/string/number) snapshot — must throw`, + ); + } + }); +}); + +describe('ADR-3473 §8.6 matrix rows 22-24: an empty / null-prototype / pollution-carrying snapshot is LEGAL', () => { + // This is as load-bearing as the throws above: it is what keeps + // /gsd-health --repair working on a broken (frontmatter-less) STATE.md — + // extractFrontmatter returns `{}` for such a document, never null, and + // `{}` must be accepted, not rejected as "absent". + test('emptySnapshotIsLegalAndRestoresNothing', () => { + const tx = openStateTransaction({ snapshot: {}, bodyDeltas: neutralBodyDeltasForMatrix() }); + assert.strictEqual(tx.kind, 'open'); + const r = applyStatePreservation({ transaction: tx, postFm: { status: 'executing' } }); + assert.strictEqual(r.mutated, false, 'an empty snapshot must find nothing to restore — mutated stays false'); + assert.strictEqual(r.postFm.status, 'executing', 'the derived value is left exactly as it was'); + }); + + test('nullPrototypeSnapshotIsAccepted', () => { + const nullProtoSnapshot = Object.create(null); + nullProtoSnapshot.status = 'executing'; + // Legal at construction — must not throw merely for lacking Object.prototype. + const tx = openStateTransaction({ snapshot: nullProtoSnapshot, bodyDeltas: neutralBodyDeltasForMatrix() }); + assert.strictEqual(tx.snapshot.status, 'executing', 'a null-prototype snapshot must still support ordinary property lookup'); + // And it must not break hasOwnProperty-style lookups inside the dispatch + // loop either — the call must complete and report what it did. + const r = applyStatePreservation({ transaction: tx, postFm: {} }); + assert.strictEqual(typeof r.mutated, 'boolean'); + }); + + test('snapshotWithPrototypeKeysDoesNotPollute', () => { + // Parsed via JSON.parse (not an object literal) so `__proto__` really is + // an OWN enumerable property of the snapshot, not the object's actual + // prototype link — the hostile shape a malformed/adversarial frontmatter + // parse could produce. + const evilSnapshot = JSON.parse('{"__proto__": {"polluted": true}, "constructor": "not-a-function", "toString": "not-a-method"}'); + assert.strictEqual( + Object.prototype.hasOwnProperty.call(evilSnapshot, '__proto__'), + true, + 'precondition: __proto__ must be an OWN key of the parsed snapshot, not the prototype link', + ); + + const tx = openStateTransaction({ snapshot: evilSnapshot, bodyDeltas: neutralBodyDeltasForMatrix() }); + applyStatePreservation({ transaction: tx, postFm: {} }); + + assert.strictEqual(({}).polluted, undefined, 'a fresh object literal must not have picked up a polluted prototype property'); + assert.strictEqual( + Object.prototype.hasOwnProperty.call(Object.prototype, 'polluted'), + false, + 'Object.prototype itself must not have gained an own `polluted` property', + ); + }); +}); + +describe('ADR-3473 §8.6 matrix row 27: the transaction object is frozen', () => { + test('transactionSnapshotIsFrozen', () => { + const tx = openStateTransaction({ snapshot: { status: 'executing' } }); + assert.strictEqual(Object.isFrozen(tx), true, 'the transaction object itself must be frozen'); + // NOTE (verified against the implementation, not assumed): only the + // transaction OBJECT is frozen at construction — `createStateTransaction` + // never calls Object.freeze on `snapshot` itself. So `tx.snapshot` is NOT + // frozen (Object.isFrozen(tx.snapshot) === false); what independence the + // snapshot enjoys against later mutation comes from `applyPreserveAlways` + // cloning on restore (see restoreClonesSoTheSnapshotCannotBeMutatedThroughPostFm + // below), not from freezing the snapshot object. Assigning `tx.snapshot` + // itself throws because this test file runs in strict mode ('use strict' + // at the top) and `tx` is frozen — a sloppy-mode module would instead + // silently no-op the assignment. + assert.strictEqual(Object.isFrozen(tx.snapshot), false, 'the snapshot OBJECT is not itself frozen by construction — only the transaction wrapper is'); + assert.throws( + () => { tx.snapshot = { replaced: true }; }, + TypeError, + 'assigning to a frozen transaction\'s property must throw under strict mode, and must not take effect', + ); + assert.strictEqual(tx.snapshot.status, 'executing', 'the original snapshot value must be unchanged after the failed assignment attempt'); + }); +}); + +describe('ADR-3473 §8.6 matrix rows 25-26: restore mutation-reporting and aliasing independence', () => { + test('identicalRestoreDoesNotReportMutation', () => { + const curatedProgress = { total_phases: 5, completed_phases: 5, total_plans: 32, completed_plans: 32, percent: 100 }; + const tx = openStateTransaction({ + snapshot: { progress: { ...curatedProgress } }, + resync: false, + bodyDeltas: neutralBodyDeltasForMatrix(), + }); + const r = applyStatePreservation({ transaction: tx, postFm: { progress: { ...curatedProgress } } }); + assert.strictEqual(r.mutated, false, 'restoring a value already identical to what postFm held must not report mutated=true (no spurious no-op write)'); + }); + + test('restoreClonesSoTheSnapshotCannotBeMutatedThroughPostFm', () => { + const curatedSnapshot = { progress: { total_phases: 5, completed_phases: 5, total_plans: 32, completed_plans: 32, percent: 100 } }; + const tx = openStateTransaction({ + snapshot: curatedSnapshot, + resync: true, + bodyDeltas: neutralBodyDeltasForMatrix(), + }); + // Derived measured NOTHING (all-string-zero, percent absent) — the + // #3756 shape — so preserve-always restores the curated block wholesale. + const postFm = { progress: { total_phases: '0', completed_phases: '0', total_plans: '0', completed_plans: '0' } }; + const r = applyStatePreservation({ transaction: tx, postFm }); + assert.strictEqual(r.mutated, true, 'precondition: the restore must actually have happened'); + assert.notStrictEqual(r.postFm.progress, tx.snapshot.progress, 'the restored value must be a CLONE, not the same object reference as the snapshot'); + + // Mutate the restored postFm.progress block IN PLACE, as a later step + // in the same write pipeline legitimately could. + r.postFm.progress.total_phases = 999; + r.postFm.progress.percent = 1; + + assert.strictEqual(tx.snapshot.progress.total_phases, 5, 'the transaction\'s OWN snapshot must be unaffected by a later in-place mutation of postFm.progress'); + assert.strictEqual(tx.snapshot.progress.percent, 100, 'same independence check on a second nested field'); + }); +}); + +describe('ADR-3473 §8.6 matrix rows 13-17 (+10/11 pinned): the measured-vs-unmeasured coercion boundary', () => { + // Every case here drives applyPreserveAlways through applyStatePreservation + // with resync:true and a real curated `progress` block, varying ONLY the + // derived totals — the boundary the design/matrix calls out as the + // riskiest logic in the change (`scanMeasuredSomething`'s `toFiniteNumber` + // coercion, never a raw `=== 0`). + const curatedProgress = { total_phases: 5, completed_phases: 5, total_plans: 32, completed_plans: 32, percent: 100 }; + + function restoreWithDerivedTotals(derivedProgress) { + const tx = openStateTransaction({ + snapshot: { progress: { ...curatedProgress } }, + resync: true, + bodyDeltas: neutralBodyDeltasForMatrix(), + }); + return applyStatePreservation({ transaction: tx, postFm: { progress: derivedProgress } }); + } + + test('stringZeroTotalsAreTreatedAsUnmeasured', () => { + // limit-1/limit/limit+1 triple on the STRING shape production actually + // emits ("0", not 0) — numeric coercion, never `=== 0`. + for (const zeroish of ['0', '00', '0.0']) { + const r = restoreWithDerivedTotals({ total_phases: zeroish, total_plans: zeroish, completed_phases: '0', completed_plans: '0' }); + assert.deepStrictEqual(r.postFm.progress, curatedProgress, `derived totals ${JSON.stringify(zeroish)} must be treated as unmeasured — curated block stands`); + assert.strictEqual(r.mutated, true); + } + }); + + test('stringNonZeroTotalIsMeasured', () => { + const r = restoreWithDerivedTotals({ total_phases: '1', total_plans: '0', completed_phases: '0', completed_plans: '0' }); + assert.strictEqual(r.mutated, false, 'a measured scan (total_phases:"1") must win — derived stands untouched'); + assert.deepStrictEqual(r.postFm.progress, { total_phases: '1', total_plans: '0', completed_phases: '0', completed_plans: '0' }); + }); + + test('absentTotalsAreUnmeasured', () => { + for (const derived of [{}, { total_phases: null, total_plans: null }, { total_phases: undefined, total_plans: undefined }, { total_phases: '', total_plans: '' }]) { + const r = restoreWithDerivedTotals(derived); + assert.deepStrictEqual(r.postFm.progress, curatedProgress, `derived ${JSON.stringify(derived)} (absent/null/undefined/empty totals) must be unmeasured — curated stands`); + } + }); + + test('nonNumericTotalsDegradeToPreservation', () => { + for (const derived of [ + { total_phases: 'abc', total_plans: 'abc' }, + { total_phases: NaN, total_plans: NaN }, + { total_phases: {}, total_plans: {} }, + { total_phases: [], total_plans: [] }, + ]) { + const r = restoreWithDerivedTotals(derived); + assert.deepStrictEqual(r.postFm.progress, curatedProgress, `hostile derived totals ${JSON.stringify(derived)} must degrade TOWARD preservation, never toward deletion`); + } + }); + + test('negativeTotalsAreNotAMeasurement', () => { + const r = restoreWithDerivedTotals({ total_phases: -1, total_plans: -1 }); + assert.deepStrictEqual(r.postFm.progress, curatedProgress, 'a negative total is not a valid count — unmeasured — curated stands'); + }); + + // Pinned mirror pair from the design's row 11/the existing matrix's row + // 10 — grouped here so the boundary (only both-zero is unmeasured) reads + // legibly against the coercion cases above. + test('oneNonZeroTotalCountsAsMeasuredEitherDirection', () => { + const r1 = restoreWithDerivedTotals({ total_phases: 1, total_plans: 0, completed_phases: 0, completed_plans: 0 }); + assert.strictEqual(r1.mutated, false, 'total_phases:1, total_plans:0 must count as measured'); + assert.deepStrictEqual(r1.postFm.progress, { total_phases: 1, total_plans: 0, completed_phases: 0, completed_plans: 0 }); + + const r2 = restoreWithDerivedTotals({ total_phases: 0, total_plans: 1, completed_phases: 0, completed_plans: 0 }); + assert.strictEqual(r2.mutated, false, 'total_phases:0, total_plans:1 must count as measured (the mirror) — only both-zero is unmeasured'); + assert.deepStrictEqual(r2.postFm.progress, { total_phases: 0, total_plans: 1, completed_phases: 0, completed_plans: 0 }); + }); +}); + +describe('ADR-3473 §8.6 matrix row 36: property — preservation never drops a curated key the derived block lacks', () => { + // Scoped to `progress`, the one preserve-always/progress-ratchet field + // this phase's centerpiece logic governs (this file's other property test, + // "#3468 matrix C3" above, already covers the generic preserve-when- + // unchanged dispatch as a property — a whitespace/empty-string curated + // value is deliberately NOT restored there, by long-standing design, so a + // property phrased over that generic field set would produce a false + // failure unrelated to this phase's change). + const progressCounterArb = fc.oneof( + fc.integer({ min: 0, max: 999 }), + fc.integer({ min: 0, max: 999 }).map(String), + ); + const progressBlockArb = fc.record({ + total_phases: progressCounterArb, + completed_phases: progressCounterArb, + total_plans: progressCounterArb, + completed_plans: progressCounterArb, + percent: progressCounterArb, + }); + + test('preservationNeverDropsACuratedKey', () => { + fc.assert( + fc.property( + fc.option(progressBlockArb, { nil: undefined }), + fc.option(progressBlockArb, { nil: undefined }), + fc.boolean(), + (curatedProgress, derivedProgress, resync) => { + const snapshot = curatedProgress !== undefined ? { progress: curatedProgress } : {}; + const postFm = derivedProgress !== undefined ? { progress: derivedProgress } : {}; + const tx = openStateTransaction({ snapshot, resync, bodyDeltas: neutralBodyDeltasForMatrix() }); + const r = applyStatePreservation({ transaction: tx, postFm }); + + if (curatedProgress !== undefined && derivedProgress === undefined) { + if (!Object.prototype.hasOwnProperty.call(r.postFm, 'progress')) { + throw new Error( + `curated key 'progress' dropped: curated=${JSON.stringify(curatedProgress)} ` + + `derived=${JSON.stringify(derivedProgress)} resync=${resync} result=${JSON.stringify(r.postFm)}`, + ); + } + } + return true; + }, + ), + // Seeded and bounded per repo convention; replay data (the exact + // curated/derived/resync triple) is printed via the thrown Error + // above on any failure. + { seed: 3871, numRuns: 300 }, + ); + }); +}); diff --git a/tests/state-write-path-drift-guard.test.cjs b/tests/state-write-path-drift-guard.test.cjs index 45da2224c..6b8db01b4 100644 --- a/tests/state-write-path-drift-guard.test.cjs +++ b/tests/state-write-path-drift-guard.test.cjs @@ -2,33 +2,46 @@ /** * Tests for the STATE.md write-path anti-divergence drift guard - * (epic #3408, issue #3468, ADR-3408 Decision 5) — - * `scripts/lint-state-write-path-drift.cjs`. + * (epic #3408, issue #3468, ADR-3408 Decision 5; SHRUNK per ADR-3473 §8.6, + * issue #3871) — `scripts/lint-state-write-path-drift.cjs`. * * Design contract: docs/adr/3408-state-write-path-preservation.md (§8.1/§8.2/§8.3) - * Test matrix: .gsd/phase/refactor-3468-table-driven-preservation/50-test-matrix.md - * (section D, rows D1-D13 — this file covers section D only) + * docs/adr/3473-enforcement-by-construction.md (§8.6) * - * Every row except D1 and D13 drives the guard's exported PURE functions - * (`findSeamBypasses`, `findPromptSeamUses`, `applyRatchet`, `loadBaseline`) - * directly with in-memory fixtures — no temp tree is needed, mirroring - * tests/state-field-drift.test.cjs's own house pattern for this class of - * guard. `REPO_ROOT` inside the guard module is a constant resolved from - * `__dirname` at require time, so it cannot be pointed at a synthetic tree - * without changing the guard's own interface — D1 (the real-tree contract) - * is therefore driven through the CLI's `--json` output instead, and D13 - * (an unreadable file) through an `fs.readFileSync` monkeypatch rather than - * a real synthetic tree. + * The guard covers five axes, each backed by its own exported pure function + * and each terminal (every finding carries its own `reason` — there is no + * ratchet and nothing here reads a baseline file): `findPolicyDispatchDrift`/ + * `findUnimplementedPolicies` (Axis 1, policy dispatch), `findRawStateWrites` + * (Axis 2, a raw `fs.writeFileSync` against the state path), + * `findUnstrippedContentWrites` (Axis 3, a frontmatter-shaped body write), + * `findPromptSeamUses` (Axis 4, prompt-layer prose shelling out to a + * write-side command), and `findCompositionBypasses` (Axis 5, a direct + * `syncStateFrontmatter`/`applyPostSyncPreservation` call outside their + * owner). ADR-3473 §8.6 retired one prior axis's `writeStateMd(` arm and the + * ratchet/retired-baseline machinery that backed it, once `writeStateMd`'s + * third parameter started requiring a `StateTransaction` the type system + * names; issue #3871 review kept that axis's OTHER arm (now + * `findCompositionBypasses`) because the type system gates only + * `writeStateMd`'s parameter, not a call site that never goes through + * `writeStateMd` at all — that arm was made terminal too. + * + * Sections D and E drive each pure function directly with in-memory + * fixtures — no temp tree needed, mirroring + * tests/state-field-drift.test.cjs's own house pattern. Section F instead + * drives the real CLI entry point, via the guard's `--root ` flag + * (`collect(root)` takes a matching parameter, default `REPO_ROOT`, so every + * existing invocation — `npm run lint:ci` included — is unaffected): this is + * what lets a real-tree fixture run inside a disposable `createTempDir()` + * tree instead of ever mutating this repository's own `src/`, where a + * planted fixture would be visible to any concurrent `tsc`/`npm run + * build:lib`/`npm run lint`/another guard invocation, and would survive as + * build-breaking debris if the process were killed before `t.after` ran. D1 + * is the one exception among the fixture-driven rows: it asserts the real + * repo's own `src/` and prompt layer are clean, so it runs through the CLI's + * `--json` output against the real `REPO_ROOT` with no `--root` override. * * Fixtures use array `.join('\n')`, never an indented template literal — - * indentation bleed would shift every asserted line number. Per D2/D5/D7's - * matrix note ("guard fixtures come from outside the guard's own writer"), - * the write-seam call lines reused below are copied VERBATIM from real, - * pre-existing production call sites (the guard's own current baseline - * entries) rather than invented by this test file: - * - `writeStateMd(statePath, modified, cwd);` src/state.cts:3682 - * - `writeStateMd(statePath, result.content, cwd);` src/milestone.cts:865 - * - `writeStateMd(statePath, stateContent, cwd);` src/health-diagnostic.cts:337 + * indentation bleed would shift every asserted line number. * * Assertions compare the frozen `REASON` enum values and the `--json`/pure * function return shapes only — never a substring/regex match on the human @@ -43,46 +56,33 @@ const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); const guard = require('../scripts/lint-state-write-path-drift.cjs'); const { REASON, - findSeamBypasses, findPromptSeamUses, findPolicyDispatchDrift, findUnstrippedContentWrites, - applyRatchet, - loadBaseline, - buildBaselineEntries, - collect, + findRawStateWrites, + findCompositionBypasses, + targetsStatePath, + EXECUTOR_FILE, SEAM_OWNER_FILE, SEAM_OWNER_EXEMPT_FUNCTIONS, - EXECUTOR_FILE, REPO_ROOT, - BASELINE_PATH, } = guard; const GUARD_PATH = path.join(REPO_ROOT, 'scripts', 'lint-state-write-path-drift.cjs'); -// A synthetic, non-owner, non-executor consumer file — never a real repo -// path — used as the `rel` argument wherever a row does not specifically -// need EXECUTOR_FILE or SEAM_OWNER_FILE behavior. +// A synthetic, non-executor consumer file — never a real repo path — used +// wherever a row does not specifically need EXECUTOR_FILE behavior. const OTHER_FILE = 'src/example-consumer.cts'; -const OTHER_FILE_2 = 'src/example-consumer-2.cts'; -const OTHER_FILE_3 = 'src/example-consumer-3.cts'; // ─── D1: the real tree, through the CLI's --json contract ───────────────── describe('D1 — the real tree', () => { test('guard: clean tree passes', () => { - // Expected RED until the sibling refactor of src/state-transition.cts - // (issue #3468 Phase 1, concurrent with this test file's own authorship) - // lands: at write time the executor still dispatches five fields by - // literal name and leaves `derive`/`clear` unimplemented, which this - // guard's policy-dispatch axis correctly reports as 7 findings. Mirrors - // tests/milestone-window-drift-guard.test.cjs's own precedent of an - // explicitly-documented real-tree row that is red until its companion - // consolidation lands. const result = runNode([GUARD_PATH, '--json'], { cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS }); assert.strictEqual(result.outcome, 'exited'); const body = JSON.parse(result.stdout); @@ -92,155 +92,14 @@ describe('D1 — the real tree', () => { }); }); -// ─── D2: the guard MUST be able to fail ──────────────────────────────────── - -describe('D2 — an unrecorded bypass fails', () => { - test('guard: an unrecorded bypass fails', () => { - const text = [ - 'function cmdSomethingElse(cwd) {', - ' const modified = deriveModifiedContent();', - ' writeStateMd(statePath, modified, cwd);', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE, text); - assert.strictEqual(observed.length, 1); - assert.strictEqual(observed[0].line, 3); - - const findings = applyRatchet(observed, { entries: [] }); - // A guard that cannot fail is worse than no guard: an unacknowledged - // bypass against an empty baseline MUST produce exactly one finding, - // reasoned, at the exact file and line — not merely "an array". - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_UNRECORDED); - assert.strictEqual(findings[0].file, OTHER_FILE); - assert.strictEqual(findings[0].line, 3); - assert.strictEqual(findings[0].source, 'writeStateMd(statePath, modified, cwd);'); - }); -}); - -// ─── D3: a recorded bypass is acknowledged ───────────────────────────────── - -describe('D3 — a recorded bypass is acknowledged', () => { - test('guard: a recorded bypass is acknowledged', () => { - const text = [ - 'function cmdSomethingElse(cwd) {', - ' const modified = deriveModifiedContent();', - ' writeStateMd(statePath, modified, cwd);', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE, text); - const baseline = { - entries: [{ file: OTHER_FILE, source: 'writeStateMd(statePath, modified, cwd);', symbol: 'writeStateMd', count: 1, owner: null }], - }; - assert.deepStrictEqual(applyRatchet(observed, baseline), []); - }); -}); - -// ─── D4: a stale acknowledgment fails ────────────────────────────────────── - -describe('D4 — a stale acknowledgment fails', () => { - test('guard: a stale acknowledgment fails', () => { - // The call site the baseline acknowledges no longer fires at all this - // scan — the acknowledgment has outlived what it describes. - const baseline = { - entries: [{ file: OTHER_FILE, source: 'writeStateMd(statePath, modified, cwd);', symbol: 'writeStateMd', count: 1, owner: null }], - }; - const findings = applyRatchet([], baseline); - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.BASELINE_ENTRY_STALE); - assert.strictEqual(findings[0].file, OTHER_FILE); - assert.strictEqual(findings[0].observed, 0); - assert.strictEqual(findings[0].acknowledged, 1); - }); -}); - -// ─── D5/D6/D7: the occurrence-count boundary triple (limit-1/limit/limit+1) ─ -// The baseline acknowledges 2 occurrences throughout ("the limit"); only the -// OBSERVED count in the fixture source varies. This is why the ratchet keys -// entries on (file, trimmed source) instead of line number: two -// byte-identical call sites in one file are otherwise indistinguishable. - -describe('D5 — occurrence count catches partial migration (limit-1)', () => { - test('guard: occurrence count catches partial migration', () => { - const text = [ - 'function cmdSomethingElse(cwd) {', - ' writeStateMd(statePath, result.content, cwd);', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE_2, text); - assert.strictEqual(observed.length, 1); - - const baseline = { - entries: [{ file: OTHER_FILE_2, source: 'writeStateMd(statePath, result.content, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], - }; - const findings = applyRatchet(observed, baseline); - // Only 1 of the 2 acknowledged call sites still fires — a genuine - // partial migration, not a clean removal — must fail, not silently pass. - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_COUNT_SHRANK); - assert.strictEqual(findings[0].observed, 1); - assert.strictEqual(findings[0].acknowledged, 2); - }); -}); - -describe('D6 — matching occurrence count passes (limit)', () => { - test('guard: matching occurrence count passes', () => { - const text = [ - 'function cmdSomethingElse(cwd) {', - ' writeStateMd(statePath, result.content, cwd);', - ' writeStateMd(statePath, result.content, cwd);', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE_2, text); - assert.strictEqual(observed.length, 2); - - const baseline = { - entries: [{ file: OTHER_FILE_2, source: 'writeStateMd(statePath, result.content, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], - }; - assert.deepStrictEqual(applyRatchet(observed, baseline), []); - }); -}); - -describe('D7 — a new copy beside an acknowledged one fails (limit+1)', () => { - test('guard: a new copy beside an acknowledged one fails', () => { - const text = [ - 'function cmdA(cwd) {', - ' writeStateMd(statePath, stateContent, cwd);', - '}', - 'function cmdB(cwd) {', - ' writeStateMd(statePath, stateContent, cwd);', - '}', - 'function cmdC(cwd) {', - ' writeStateMd(statePath, stateContent, cwd);', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE_3, text); - assert.strictEqual(observed.length, 3); - - const baseline = { - entries: [{ file: OTHER_FILE_3, source: 'writeStateMd(statePath, stateContent, cwd);', symbol: 'writeStateMd', count: 2, owner: null }], - }; - const findings = applyRatchet(observed, baseline); - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_COUNT_GREW); - assert.strictEqual(findings[0].observed, 3); - assert.strictEqual(findings[0].acknowledged, 2); - }); -}); - -// ─── D8: comments are not drift ──────────────────────────────────────────── +// ─── D8: comments are not drift (composition-bypass shape) ──────────────── describe('D8 — comments are not drift', () => { test('guard: comments are not drift', () => { const text = [ - '// writeStateMd(statePath, modified, cwd);', + '// syncStateFrontmatter(content, cwd);', '/**', - ' * writeStateMd(statePath, modified, cwd);', + ' * applyPostSyncPreservation(originalContent, content, synced, statePath, options);', ' */', 'function noop() {}', ].join('\n'); @@ -249,7 +108,7 @@ describe('D8 — comments are not drift', () => { // and a `/* */` block comment both carrying the exact call text must // stay silent — both are blanked by `stripComments` before the seam-call // regex ever runs. - assert.deepStrictEqual(findSeamBypasses(OTHER_FILE, text), []); + assert.deepStrictEqual(findCompositionBypasses(OTHER_FILE, text), []); }); }); @@ -257,24 +116,34 @@ describe('D8 — comments are not drift', () => { describe('D9 — owner functions are exempt', () => { test('guard: owner functions are exempt', () => { - // #3469: `readModifyWriteStateMd` now calls the single - // `syncAndPreserveStateMd` symbol rather than assembling the two seam - // calls itself, so it needs no exemption — `syncAndPreserveStateMd` is - // the sole legitimate place `syncStateFrontmatter(` and - // `applyPostSyncPreservation(` appear together (the composition every - // OTHER caller, including `readModifyWriteStateMd`, now routes through). + // `syncAndPreserveStateMd` is the sole legitimate place + // `syncStateFrontmatter(` and `applyPostSyncPreservation(` appear + // together (the composition every OTHER caller routes through). assert.ok(SEAM_OWNER_EXEMPT_FUNCTIONS.includes('syncAndPreserveStateMd')); const text = [ - 'function syncAndPreserveStateMd(originalContent, transformedContent, statePath, cwd, resync) {', - ' const synced = syncStateFrontmatter(transformedContent, cwd);', - ' return applyPostSyncPreservation(originalContent, transformedContent, synced, statePath, resync);', + 'function syncAndPreserveStateMd(originalContent, transformedContent, statePath, cwd, options) {', + ' const synced = syncStateFrontmatter(transformedContent, cwd, options.authoritativeFm);', + ' return applyPostSyncPreservation(originalContent, transformedContent, synced, statePath, options);', '}', ].join('\n'); // The seam's own internal plumbing (the one owned composition — // sync then post-sync preservation) is not a bypass. - assert.deepStrictEqual(findSeamBypasses(SEAM_OWNER_FILE, text), []); + assert.deepStrictEqual(findCompositionBypasses(SEAM_OWNER_FILE, text), []); + }); + + test('guard: writeStateMd is also exempt — its own sanctioned direct syncStateFrontmatter call is not a bypass', () => { + assert.ok(SEAM_OWNER_EXEMPT_FUNCTIONS.includes('writeStateMd')); + + const text = [ + 'function writeStateMd(statePath, content, transaction, cwd, clock) {', + " const synced = syncStateFrontmatter(content, cwd, undefined, transaction.kind === 'rebuild');", + ' platformWriteSync(statePath, synced);', + '}', + ].join('\n'); + + assert.deepStrictEqual(findCompositionBypasses(SEAM_OWNER_FILE, text), []); }); }); @@ -287,17 +156,90 @@ describe('D10 — the owner file is not exempt', () => { const text = [ 'function patchCore(cwd) {', ' const modified = compute();', - ' writeStateMd(statePath, modified, cwd);', - ' return modified;', + ' const synced = syncStateFrontmatter(modified, cwd);', + ' return synced;', '}', ].join('\n'); - const out = findSeamBypasses(SEAM_OWNER_FILE, text); + const out = findCompositionBypasses(SEAM_OWNER_FILE, text); assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.COMPOSITION_BYPASS); assert.strictEqual(out[0].line, 3); }); }); +// ─── D12: CRLF is scanned identically to LF ──────────────────────────────── + +describe('D12 — CRLF is scanned identically', () => { + test('guard: CRLF is scanned identically', () => { + const lfText = [ + 'function cmdSomethingElse(cwd) {', + ' const synced = syncStateFrontmatter(modified, cwd);', + '}', + ].join('\n'); + const crlfText = lfText.split('\n').join('\r\n'); + + const lfOut = findCompositionBypasses(OTHER_FILE, lfText); + const crlfOut = findCompositionBypasses(OTHER_FILE, crlfText); + + assert.strictEqual(crlfOut.length, 1); + const strip = (arr) => arr.map(({ line, symbol, source }) => ({ line, symbol, source })); + assert.deepStrictEqual(strip(crlfOut), strip(lfOut)); + // A stray trailing \r surviving into the reported source (the repo's + // documented \n-only-regex bug class) would show up here as a + // sanitized `\x0d` escape — it must not. + assert.strictEqual(crlfOut[0].source, 'const synced = syncStateFrontmatter(modified, cwd);'); + }); +}); + +// ─── E1/E2 (Phase 2 / #3469, RETAINED per issue #3871): the composition-pair +// re-assembly shape itself, and the legitimate single-call composition ───── + +describe('E1 — a re-assembled composition at a new call site is detected', () => { + test('guard: a call site invoking syncStateFrontmatter and applyPostSyncPreservation directly (bypassing syncAndPreserveStateMd) is caught on BOTH calls', () => { + // Finding 3's exact shape (ADR-3408 Amendment 2): every step calls an + // owner, so neither call alone is undeclared — but assembling the PAIR + // at a call site outside the seam composition is the re-derivation §8.3 + // forbids by name. Synthetic: the real instance of this shape + // (cmdPhaseComplete's pre-#3469 adapter) was fixed by that phase. + const text = [ + 'function cmdReassembledAdapter(cwd, statePath, stateContent) {', + ' let synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm);', + ' synced = applyPostSyncPreservation(originalStateContent, stateContent, synced, statePath, options);', + ' return synced;', + '}', + ].join('\n'); + + const out = findCompositionBypasses(OTHER_FILE, text); + assert.strictEqual(out.length, 2, 'both re-assembled stages must be caught, not just one'); + assert.deepStrictEqual(out.map((f) => f.symbol).sort(), ['applyPostSyncPreservation', 'syncStateFrontmatter']); + assert.ok(out.every((f) => f.reason === REASON.COMPOSITION_BYPASS)); + }); +}); + +describe('E2 — a legitimate single call to the composition is not detected', () => { + test('guard: calling syncAndPreserveStateMd (the ONE write-seam composition) is not a bypass', () => { + // Verbatim shape from a real caller of the composition (e.g. + // milestone.cts's cmdMilestoneComplete) — a single call to the owned + // composition function, never to its two internal stages directly. + const text = [ + ' const finalContent = syncAndPreserveStateMd(', + ' originalStateContent,', + ' result.content,', + ' statePath,', + ' cwd,', + ' {', + ' resync: true,', + ' authoritativeFm: Object.keys(authoritativeFm).length > 0 ? authoritativeFm : undefined,', + ' divergedFields,', + ' },', + ' );', + ].join('\n'); + + assert.deepStrictEqual(findCompositionBypasses(OTHER_FILE, text), []); + }); +}); + // ─── D11: the prompt layer is in the scan surface ────────────────────────── describe('D11 — the prompt layer is in the scan surface', () => { @@ -313,6 +255,7 @@ describe('D11 — the prompt layer is in the scan surface', () => { const out = findPromptSeamUses(PROMPT_FILE, text); assert.strictEqual(out.length, 1); assert.strictEqual(out[0].line, 3); + assert.strictEqual(out[0].reason, REASON.PROMPT_LAYER_STATE_WRITE); assert.strictEqual(out[0].symbol, 'prompt-layer-state-write'); }); @@ -324,104 +267,6 @@ describe('D11 — the prompt layer is in the scan surface', () => { }); }); -// ─── D12: CRLF is scanned identically to LF ──────────────────────────────── - -describe('D12 — CRLF is scanned identically', () => { - test('guard: CRLF is scanned identically', () => { - const lfText = [ - 'function cmdSomethingElse(cwd) {', - ' writeStateMd(statePath, modified, cwd);', - '}', - ].join('\n'); - const crlfText = lfText.split('\n').join('\r\n'); - - const lfOut = findSeamBypasses(OTHER_FILE, lfText); - const crlfOut = findSeamBypasses(OTHER_FILE, crlfText); - - assert.strictEqual(crlfOut.length, 1); - const strip = (arr) => arr.map(({ line, symbol, source }) => ({ line, symbol, source })); - assert.deepStrictEqual(strip(crlfOut), strip(lfOut)); - // A stray trailing \r surviving into the reported source (the repo's - // documented \n-only-regex bug class) would show up here as a - // sanitized `\x0d` escape — it must not. - assert.strictEqual(crlfOut[0].source, 'writeStateMd(statePath, modified, cwd);'); - }); -}); - -// ─── D13: an unreadable file degrades, never crashes ─────────────────────── - -describe('D13 — an unreadable file is reported, not fatal', () => { - test('guard: an unreadable file is reported, not fatal', (t) => { - const originalReadFileSync = fs.readFileSync; - t.after(() => { - fs.readFileSync = originalReadFileSync; - }); - - // Monkeypatch (never chmod 0o000, which root bypasses under Docker/CI - // and would leave this assertion covering nothing). Scoped to - // BASELINE_PATH only, so no other read in this process is disturbed. - fs.readFileSync = function patchedReadFileSync(target, ...rest) { - if (target === BASELINE_PATH) { - const err = new Error('simulated unreadable baseline file'); - err.code = 'EACCES'; - throw err; - } - return originalReadFileSync.call(fs, target, ...rest); - }; - - // loadBaseline() must not throw — it degrades to a returned value. - assert.doesNotThrow(() => loadBaseline()); - const result = loadBaseline(); - // An unreadable file (EACCES) is NOT the same state as an absent one - // (ENOENT) and must not degrade to the same "no baseline yet" shape — - // collapsing the two is the exact ADR-3180/ADR-3408 failure mode this - // guard exists to catch. loadBaseline() must surface a distinguishable - // `entries: null` result carrying the underlying fs error code. - assert.deepStrictEqual(result, { entries: null, code: 'EACCES' }); - }); - - test('CLI: an unreadable baseline reaches REASON.BASELINE_UNREADABLE with its error code, not the first-run shape', (t) => { - const originalReadFileSync = fs.readFileSync; - t.after(() => { - fs.readFileSync = originalReadFileSync; - }); - - fs.readFileSync = function patchedReadFileSync(target, ...rest) { - if (target === BASELINE_PATH) { - const err = new Error('simulated unreadable baseline file'); - err.code = 'EACCES'; - throw err; - } - return originalReadFileSync.call(fs, target, ...rest); - }; - - // Drive main() in-process (not via the CLI subprocess helper) so the - // monkeypatched fs.readFileSync is actually in effect for the call. - const originalArgv = process.argv; - const originalWrite = process.stdout.write; - t.after(() => { - process.stdout.write = originalWrite; - process.argv = originalArgv; - process.exitCode = 0; - }); - let captured = ''; - process.stdout.write = function patchedWrite(chunk) { - captured += chunk; - return true; - }; - process.argv = [originalArgv[0], GUARD_PATH, '--json']; - guard.main(['--json']); - const exitCode = process.exitCode; - - assert.strictEqual(exitCode, 1); - const parsed = JSON.parse(captured); - assert.strictEqual(parsed.ok, false); - assert.strictEqual(parsed.findings.length, 1); - assert.strictEqual(parsed.findings[0].reason, REASON.BASELINE_UNREADABLE); - assert.strictEqual(parsed.findings[0].code, 'EACCES'); - }); -}); - // ─── D14: field-name-keyed BRANCH comparisons (not just CALLS) ──────────── // #3468: `applyPreserveIfPlaceholder` shipped a `field !== 'milestone_name'` // branch — a field-name-keyed dispatch that routed around @@ -510,11 +355,10 @@ describe('D14 — field-name-keyed branch comparisons are caught', () => { // Security review finding: a repo can legally track a filename containing C1 // control bytes or bidi-override codepoints — exactly as attacker-controlled // on a fork PR as the `source` fragment this guard already sanitized before -// this fix. Before this fix `file` reached `--json` stdout and the committed -// baseline (`scripts/state-write-path-drift-baseline.json`) unsanitized — +// this fix. Before this fix `file` reached `--json` stdout unsanitized — // only the human formatter wrapped it. A finding's `file` (and any other // attacker-derived field, like `field`) must come back escaped from the -// FINDER itself, so every consumer (human, `--json`, baseline) inherits the +// FINDER itself, so every consumer (human, `--json`) inherits the // sanitization uniformly. // // The two attack codepoints are built via `String.fromCharCode` rather than @@ -527,10 +371,10 @@ describe('D15 — file (and field) values are sanitized at construction', () => const ATTACK_FILE = `src/evil${RLO}${C1_CSI}name.cts`; const ESCAPED_FILE = 'src/evil\\u202e\\x9bname.cts'; - test('findSeamBypasses: an attacker-controlled filename comes back escaped', () => { - const text = ['function cmdSomethingElse(cwd) {', ' writeStateMd(statePath, modified, cwd);', '}'].join('\n'); + test('findRawStateWrites: an attacker-controlled filename comes back escaped', () => { + const text = ['function cmdSomethingElse(cwd) {', ' fs.writeFileSync(statePath, modified);', '}'].join('\n'); - const out = findSeamBypasses(ATTACK_FILE, text); + const out = findRawStateWrites(ATTACK_FILE, text); assert.strictEqual(out.length, 1); assert.strictEqual(out[0].file, ESCAPED_FILE); // Neither raw attack codepoint survives in the finding at all — this is @@ -539,15 +383,6 @@ describe('D15 — file (and field) values are sanitized at construction', () => // construction-time escaping — not JSON.stringify — is load-bearing). assert.ok(!out[0].file.includes(RLO)); assert.ok(!out[0].file.includes(C1_CSI)); - - // The SAME escaped value is what a regenerated baseline entry persists — - // proving the fix reaches the committed - // scripts/state-write-path-drift-baseline.json, not just the finding. - const entries = buildBaselineEntries(out, null); - assert.strictEqual(entries.length, 1); - assert.strictEqual(entries[0].file, ESCAPED_FILE); - assert.ok(!entries[0].file.includes(RLO)); - assert.ok(!entries[0].file.includes(C1_CSI)); }); test('findPromptSeamUses: an attacker-controlled filename comes back escaped', () => { @@ -573,66 +408,13 @@ describe('D15 — file (and field) values are sanitized at construction', () => }); // ───────────────────────────────────────────────────────────────────────── -// Phase 2 (#3469) — ADR-3408 §8.3 Matrix section E: guard rows closing -// Phase 1's declared known gap (Axis 3, §8.3(b)) and pinning the ratchet's -// new 2-permanent-entry shape (Amendment 2). Test matrix: -// .gsd/phase/refactor-3469-one-write-seam/50-test-matrix.md -// -// E4/E5 are the false-positive guards — the exact shape that measured 29 -// false positives to 1 true positive in Phase 1's naive co-occurrence -// approximation (see this guard's own header, Axis 3). E7 is the inverse: a -// sanctioned-permanent entry vanishing from the observed tree must FAIL, not -// silently reach zero — a guard reaching zero here would only do so by -// having stopped looking at a real writer. +// Section E (Phase 2 / #3469) — ADR-3408 §8.3 Matrix section E: guard rows +// closing Phase 1's declared known gap (Axis 3, §8.3(b)). E1/E2/E6/E7/E8 +// (all of which drove the retired `findSeamBypasses`/ratchet machinery) are +// REMOVED along with it, per ADR-3473 §8.6. E3/E4/E5 (the frontmatter-write +// axis ADR-3473 §8.6 did NOT name for removal) are retained. // ───────────────────────────────────────────────────────────────────────── -describe('E1 — a re-assembled composition at a new call site is detected', () => { - test('guard: a call site invoking syncStateFrontmatter and applyPostSyncPreservation directly (bypassing syncAndPreserveStateMd) is caught on BOTH calls', () => { - // Finding 3's exact shape (ADR-3408 Amendment 2): every step calls an - // owner, so neither call alone is undeclared — but assembling the PAIR - // at a call site outside the seam composition is the re-derivation §8.3 - // forbids by name. Synthetic: the real instance of this shape - // (cmdPhaseComplete's pre-#3469 adapter) was fixed by this same phase. - const text = [ - 'function cmdReassembledAdapter(cwd, statePath, stateContent) {', - ' let synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm);', - ' synced = applyPostSyncPreservation(originalStateContent, stateContent, synced, statePath, true, authoritativeFm);', - ' return synced;', - '}', - ].join('\n'); - - const observed = findSeamBypasses(OTHER_FILE, text); - assert.strictEqual(observed.length, 2, 'both re-assembled stages must be caught, not just one'); - assert.deepStrictEqual(observed.map((f) => f.symbol).sort(), ['applyPostSyncPreservation', 'syncStateFrontmatter']); - - const findings = applyRatchet(observed, { entries: [] }); - assert.strictEqual(findings.length, 2); - assert.ok(findings.every((f) => f.reason === REASON.SEAM_BYPASS_UNRECORDED)); - }); -}); - -describe('E2 — a legitimate single call to the composition is not detected', () => { - test('guard: calling syncAndPreserveStateMd (the ONE write-seam composition) is not a bypass', () => { - // Verbatim from src/milestone.cts's real cmdMilestoneComplete call site - // (ADR-3408 Amendment 2's third caller). - const text = [ - ' const finalContent = syncAndPreserveStateMd(', - ' originalStateContent,', - ' result.content,', - ' statePath,', - ' cwd,', - ' {', - ' resync: true,', - ' authoritativeFm: Object.keys(authoritativeFm).length > 0 ? authoritativeFm : undefined,', - ' divergedFields,', - ' },', - ' );', - ].join('\n'); - - assert.deepStrictEqual(findSeamBypasses(OTHER_FILE, text), []); - }); -}); - describe('E3 — a patchCore-style frontmatter write is detected (closes the Phase 1 declared gap)', () => { test('guard: stateReplaceField over unstripped content with a variable field name is caught', () => { // The pre-Phase-2 shape #3469 fixed: patchCore ran stateReplaceField @@ -705,57 +487,150 @@ describe('E5 — sectionBody-scoped stateReplaceField calls are NOT detected', ( }); }); -describe('E6 — ratchet: exactly 2 sanctioned-permanent entries remain (limit)', () => { - test('guard: the real baseline has exactly 2 permanent entries, and the real tree matches it with zero findings', () => { - const baseline = loadBaseline(); - assert.strictEqual( - baseline.entries.length, - 2, - 'ADR-3408 Amendment 2: the ratchet holds exactly 2 sanctioned-permanent entries, not 0 — ' + - 'Phase 4 does not drive this baseline to empty', +// ───────────────────────────────────────────────────────────────────────── +// F — ADR-3473 §8.6: the raw-write axis (`findRawStateWrites`), and the +// guard's own real CLI entry point proving the shrunk guard can still fail. +// ───────────────────────────────────────────────────────────────────────── + +describe('F1 — the raw-write axis: pure function coverage', () => { + test('guard: fs.writeFileSync against statePath is reported', () => { + const text = [ + 'function bogusRawWrite(statePath, content) {', + ' fs.writeFileSync(statePath, content);', + '}', + ].join('\n'); + + const out = findRawStateWrites(OTHER_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.RAW_STATE_WRITE); + assert.strictEqual(out[0].line, 2); + assert.strictEqual(out[0].source, 'fs.writeFileSync(statePath, content);'); + }); + + test('guard: fs.writeFileSync against a STATE.md literal is reported', () => { + const text = [ + 'function bogusRawWrite(cwd, content) {', + " fs.writeFileSync(path.join(cwd, 'STATE.md'), content);", + '}', + ].join('\n'); + + const out = findRawStateWrites(OTHER_FILE, text); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].reason, REASON.RAW_STATE_WRITE); + }); + + test('control: fs.writeFileSync against an unrelated target is NOT reported', () => { + const text = ['function writeSomethingElse(otherPath, content) {', ' fs.writeFileSync(otherPath, content);', '}'].join( + '\n', ); - for (const entry of baseline.entries) { - assert.strictEqual(entry.owner, 'sanctioned-permanent'); - } - const { seamFindings } = collect(); - const findings = applyRatchet(seamFindings, baseline); - assert.deepStrictEqual(findings, [], 'the real tree must match the 2-entry baseline exactly'); + + assert.deepStrictEqual(findRawStateWrites(OTHER_FILE, text), []); + }); + + test('control: platformWriteSync (the sanctioned seam) against statePath is NOT reported — a different call, by name', () => { + // The type/seam this axis exists BESIDE, not instead of: every real + // STATE.md writer in this codebase calls `platformWriteSync`, never raw + // `fs.writeFileSync`, against `statePath`. This axis only matches the + // literal `fs.writeFileSync` call shape. + const text = ['function realWriter(statePath, content) {', ' platformWriteSync(statePath, content);', '}'].join( + '\n', + ); + + assert.deepStrictEqual(findRawStateWrites(OTHER_FILE, text), []); + }); + + test('guard: comments are not drift', () => { + const text = [ + '// fs.writeFileSync(statePath, modified);', + '/**', + ' * fs.writeFileSync(statePath, modified);', + ' */', + 'function noop() {}', + ].join('\n'); + + assert.deepStrictEqual(findRawStateWrites(OTHER_FILE, text), []); + }); + + test('targetsStatePath: bare identifier and STATE.md-literal both match; an unrelated identifier does not', () => { + assert.ok(targetsStatePath('statePath')); + assert.ok(targetsStatePath("path.join(cwd, 'STATE.md')")); + assert.ok(!targetsStatePath('otherPath')); }); }); -describe('E7 — ratchet: a sanctioned-permanent entry disappearing fails (limit-1)', () => { - test('guard: removing one of the two permanent entries from the observed tree is reported STALE, not silently accepted', () => { - const baseline = loadBaseline(); - assert.strictEqual(baseline.entries.length, 2); - // Simulate one sanctioned entry (cmdStateSync's writeStateMd call) - // vanishing from the observed tree — exactly the shape §8.3's closed - // exception list forbids: a sanctioned exception may not silently - // disappear (a guard reaching zero here would only do so by having - // stopped looking at a real writer). - const vanished = baseline.entries[0]; - const stillPresent = baseline.entries[1]; - const observed = [{ file: stillPresent.file, source: stillPresent.source, symbol: stillPresent.symbol, line: 1 }]; +describe('F2 — a guard that cannot fail is not a guard: the real CLI entry point catches a raw write', () => { + test('CLI: --root with fs.writeFileSync(statePath, ...) is reported, without touching the real src/ tree', (t) => { + // A throwaway tree in an OS temp dir, never inside this repository — the + // guard's own `--root` flag (default REPO_ROOT, so every OTHER caller of + // this CLI is unaffected) is what makes this possible without mutating + // the repo under test. See this file's header for why a real-src/ + // fixture was rejected. + const tmpRoot = createTempDir('state-write-path-drift-guard-'); + t.after(() => cleanup(tmpRoot)); - const findings = applyRatchet(observed, baseline); - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.BASELINE_ENTRY_STALE); - assert.strictEqual(findings[0].file, vanished.file); - assert.strictEqual(findings[0].observed, 0); - assert.strictEqual(findings[0].acknowledged, 1); + const srcDir = path.join(tmpRoot, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + const fixtureContent = [ + "import * as fs from 'node:fs';", + '', + 'function bogusRawWrite(statePath: string, content: string): void {', + ' fs.writeFileSync(statePath, content);', + '}', + ].join('\n'); + fs.writeFileSync(path.join(srcDir, 'bogus.cts'), fixtureContent, 'utf8'); + + const result = runNode([GUARD_PATH, '--root', tmpRoot, '--json'], { cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + assert.strictEqual(result.exitCode, 1, 'a real raw write against statePath must fail the CLI, not pass it'); + + const body = JSON.parse(result.stdout); + assert.strictEqual(body.ok, false); + const finding = body.findings.find((f) => f.file === 'src/bogus.cts'); + assert.ok(finding, 'the planted fixture must appear in --json findings'); + assert.strictEqual(finding.reason, REASON.RAW_STATE_WRITE); + assert.strictEqual(finding.line, 4); + }); + + test('CLI: --root passes, proving --root does not silently widen scope back to REPO_ROOT', (t) => { + const tmpRoot = createTempDir('state-write-path-drift-guard-clean-'); + t.after(() => cleanup(tmpRoot)); + + const srcDir = path.join(tmpRoot, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'clean.cts'), "export const noop = () => 'noop';\n", 'utf8'); + const result = runNode([GUARD_PATH, '--root', tmpRoot, '--json'], { cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + assert.strictEqual(result.exitCode, 0); + const body = JSON.parse(result.stdout); + assert.strictEqual(body.ok, true); + assert.deepStrictEqual(body.findings, []); }); }); -describe('E8 — ratchet: a 3rd bypass beside the 2 sanctioned entries fails as unrecorded (limit+1)', () => { - test('guard: a new, unacknowledged writeStateMd call alongside the 2 sanctioned entries fails', () => { - const baseline = loadBaseline(); - assert.strictEqual(baseline.entries.length, 2); - const matchingObserved = baseline.entries.map((e) => ({ file: e.file, source: e.source, symbol: e.symbol, line: 1 })); - const newBypass = { file: OTHER_FILE, source: 'writeStateMd(statePath, modified, cwd);', symbol: 'writeStateMd', line: 42 }; - const observed = [...matchingObserved, newBypass]; +describe('F3 — a guard that cannot fail is not a guard: the real CLI entry point catches a composition bypass', () => { + test('CLI: --root with a re-assembled syncStateFrontmatter + applyPostSyncPreservation pair is reported, without touching the real src/ tree', (t) => { + const tmpRoot = createTempDir('state-write-path-drift-guard-composition-'); + t.after(() => cleanup(tmpRoot)); - const findings = applyRatchet(observed, baseline); - assert.strictEqual(findings.length, 1); - assert.strictEqual(findings[0].reason, REASON.SEAM_BYPASS_UNRECORDED); - assert.strictEqual(findings[0].file, OTHER_FILE); + const srcDir = path.join(tmpRoot, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + const fixtureContent = [ + 'function cmdReassembledAdapter(cwd: string, statePath: string, stateContent: string): string {', + ' let synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm);', + ' synced = applyPostSyncPreservation(originalStateContent, stateContent, synced, statePath, options);', + ' return synced;', + '}', + ].join('\n'); + fs.writeFileSync(path.join(srcDir, 'bogus-composition.cts'), fixtureContent, 'utf8'); + + const result = runNode([GUARD_PATH, '--root', tmpRoot, '--json'], { cwd: REPO_ROOT, timeoutMs: PROBE_TIMEOUT_MS }); + assert.strictEqual(result.outcome, 'exited'); + assert.strictEqual(result.exitCode, 1, 'a re-assembled write-seam composition must fail the CLI, not pass it'); + + const body = JSON.parse(result.stdout); + assert.strictEqual(body.ok, false); + const findings = body.findings.filter((f) => f.file === 'src/bogus-composition.cts'); + assert.strictEqual(findings.length, 2, 'both re-assembled stages must appear in --json findings'); + assert.ok(findings.every((f) => f.reason === REASON.COMPOSITION_BYPASS)); }); }); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 876772e87..2ac239736 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -6624,7 +6624,9 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid ].join('\n'); fs.writeFileSync(statePath, original); - stateLib.writeStateMd(statePath, original, tmp); + stateLib.writeStateMd(statePath, original, stateTransitionMod.rebuildStateTransaction({ + snapshot: frontmatterLib.extractFrontmatter(original), + }), tmp); const onDisk = fs.readFileSync(statePath, 'utf8'); const fm = frontmatterLib.extractFrontmatter(onDisk); @@ -6657,7 +6659,9 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid ].join('\n'); fs.writeFileSync(statePath, original); - stateLib.writeStateMd(statePath, original, tmp); + stateLib.writeStateMd(statePath, original, stateTransitionMod.rebuildStateTransaction({ + snapshot: frontmatterLib.extractFrontmatter(original), + }), tmp); const expected = [ '---', 'gsd_state_version: 1.0', 'status: unknown', 'last_updated: "2023-11-14T22:13:20.000Z"', @@ -6693,7 +6697,9 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid '**Current phase:** (determining...)', '**Status:** Resuming', '', ].join('\n'); - stateLib.writeStateMd(statePath, regenerated, tmp); + stateLib.writeStateMd(statePath, regenerated, stateTransitionMod.rebuildStateTransaction({ + snapshot: frontmatterLib.extractFrontmatter(oldCurated), + }), tmp); const onDisk = fs.readFileSync(statePath, 'utf8'); const fm = frontmatterLib.extractFrontmatter(onDisk); @@ -14385,7 +14391,9 @@ describe('buildStateFrontmatter cache invalidation (#1967)', () => { test('writeStateMd invalidates cache so subsequent reads see new disk state', () => { // First write — populates cache via buildStateFrontmatter const content1 = fs.readFileSync(statePath, 'utf-8'); - state.writeStateMd(statePath, content1, tmpDir); + state.writeStateMd(statePath, content1, stateTransitionMod.rebuildStateTransaction({ + snapshot: frontmatterLib.extractFrontmatter(content1), + }), tmpDir); // Create a NEW phase directory AFTER the first write // Without cache invalidation, the second write would still see only 1 phase @@ -14400,7 +14408,9 @@ describe('buildStateFrontmatter cache invalidation (#1967)', () => { // Second write in the SAME process — must see the new phase const content2 = fs.readFileSync(statePath, 'utf-8'); - state.writeStateMd(statePath, content2, tmpDir); + state.writeStateMd(statePath, content2, stateTransitionMod.rebuildStateTransaction({ + snapshot: frontmatterLib.extractFrontmatter(content2), + }), tmpDir); // Read back and parse frontmatter to verify it reflects 2 phases, not 1 const result = fs.readFileSync(statePath, 'utf-8'); @@ -16895,3 +16905,613 @@ describe('#3699 state update — derived frontmatter keys explain themselves', ( } }); }); + +// ═════════════════════════════════════════════════════════════════════════ +// #3871 / #3756 (ADR-3473 §8.6): a curated `progress:` frontmatter block is +// LOST on a write by verbs that have nothing to do with progress (state +// record-session, state add-decision) once the CURRENT milestone's phase +// dirs have been archived to `.planning/milestones/-phases/` while +// STATE.md/ROADMAP.md still identify that milestone as the current one +// (ROADMAP heading "Current", Phase entries still listed in the ROADMAP +// text — only the on-disk phase directories moved). The milestone-scoped +// disk scan then finds none of the current milestone's phase directories +// under `.planning/phases/` and derives an empty/zero progress projection — +// verified empirically (see repro3756.js, run against the built lib): the +// persisted frontmatter's `progress` key is dropped ENTIRELY after the +// write (not merely zeroed-with-percent-omitted). Root cause is unchanged: +// `applyPostSyncPreservation` (src/state.cts) computes +// `const preFm = resync ? null : extractFrontmatter(...)` while +// `readModifyWriteStateMd` defaults `resync` to `true`, so the declared +// `preserve-always` policy row for `progress` never runs on this write path +// and has no chance to restore the curated block before it is discarded. +// +// Fixture pattern mirrors `tests/health-validation.test.cjs`'s +// `mkArchivePhases` (`.planning/milestones/-phases/-phase-N/`) +// and `tests/completion-ratio-scope-withholding.test.cjs`'s ROADMAP-heading +// fixture builders — a "Current" milestone heading (so scope classifies as +// SCOPE.COMPLETE / windowed rather than UNSCOPED — a "Shipped" heading gets +// stripped by `stripShippedMilestones` and reproduces rule-4 withholding +// instead, a different and pre-existing intentional behavior, NOT this +// defect), phase dirs that exist ONLY under the archive path, and nothing +// at all under `.planning/phases/` for the current milestone. +// ═════════════════════════════════════════════════════════════════════════ + +describe('#3871 / #3756: curated progress must survive a write on an archived milestone', () => { + // Builds an archived-milestone fixture: STATE.md asserts milestone v1.0 + // and carries the curated progress block (5/5/32/32/100%) plus the body + // sections `record-session` and `add-decision` each need (Session + // Continuity, Decisions). ROADMAP.md shows v1.0 as the CURRENT milestone + // ("Current 🚧" heading, with all 5 Phase entries still listed in the + // ROADMAP text) with 5 phases. The phase dirs themselves live ONLY under + // `.planning/milestones/v1.0-phases/` — `.planning/phases/` has nothing + // for the current milestone, exactly as it is right after `milestone + // complete --archive-phases` runs while STATE.md has not yet been + // advanced to a new milestone. + function buildArchivedMilestoneFixture(cwd) { + const planningDir = path.join(cwd, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + '### Phase 2: Bar', + '### Phase 3: Baz', + '### Phase 4: Qux', + '### Phase 5: Quux', + '', + ].join('\n'), + ); + + // 5 archived phases totalling 32 plans, every plan paired with a + // SUMMARY (all complete) — mirrors mkArchivePhases in + // tests/health-validation.test.cjs, but with real PLAN/SUMMARY files + // rather than empty dirs, since this fixture is about the DISK SCAN + // finding zero CURRENT-milestone phases, not about archive discovery. + const archiveDir = path.join(planningDir, 'milestones', 'v1.0-phases'); + const plansPerPhase = [7, 7, 6, 6, 6]; // sums to 32 + plansPerPhase.forEach((count, i) => { + const phaseNum = String(i + 1).padStart(2, '0'); + const phaseDir = path.join(archiveDir, `${phaseNum}-phase-${i + 1}`); + fs.mkdirSync(phaseDir, { recursive: true }); + for (let p = 1; p <= count; p += 1) { + const planNum = String(p).padStart(2, '0'); + fs.writeFileSync(path.join(phaseDir, `${phaseNum}-${planNum}-PLAN.md`), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, `${phaseNum}-${planNum}-SUMMARY.md`), '# Summary\n'); + } + }); + + // Deliberately NO .planning/phases/ directory at all for the current + // milestone — the archived-milestone shape this issue is about. + + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + 'progress:', + ' total_phases: 5', + ' completed_phases: 5', + ' total_plans: 32', + ' completed_plans: 32', + ' percent: 100', + '---', + '', + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** Phase 5, Plan 32', + '**Resume file:** None', + '', + '## Decisions', + 'No decisions yet.', + '', + '## Blockers', + 'None', + '', + ].join('\n'), + ); + } + + function assertCuratedProgressSurvived(cwd) { + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const content = fs.readFileSync(statePath, 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(content); + assert.ok(fm && fm.progress, 'STATE.md frontmatter must still carry a progress block'); + assert.strictEqual(Number(fm.progress.total_phases), 5, 'total_phases must remain the curated 5, not zeroed by the archived-milestone disk scan'); + assert.strictEqual(Number(fm.progress.completed_phases), 5, 'completed_phases must remain the curated 5'); + assert.strictEqual(Number(fm.progress.total_plans), 32, 'total_plans must remain the curated 32'); + assert.strictEqual(Number(fm.progress.completed_plans), 32, 'completed_plans must remain the curated 32'); + assert.strictEqual(Number(fm.progress.percent), 100, 'percent must remain the curated 100, not go missing'); + } + + // THIS TEST MUST FAIL TODAY (#3756): `state record-session` resyncs + // (readModifyWriteStateMd defaults resync:true), which forces `preFm` to + // null in applyPostSyncPreservation, which starves the preserve-always + // executor for `progress` of the one input (`ctx.preFm`) it actually + // reads — so the curated block is never restored and the persisted + // frontmatter's `progress` key is dropped entirely (verified via + // repro3756.js against the built lib, not merely inferred). + test('recordSessionOnArchivedMilestoneDoesNotZeroProgress', (t) => { + const cwd = createTempDir('gsd-3871-record-session-'); + t.after(() => cleanup(cwd)); + buildArchivedMilestoneFixture(cwd); + + const result = runGsdTools(['state', 'record-session', '--stopped-at', 'Phase 5 complete'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + + assertCuratedProgressSurvived(cwd); + }); + + // Same fixture, same failure mode, via `state add-decision` instead — + // pins that the defect is in the shared write seam (readModifyWriteStateMd + // / applyPostSyncPreservation), not something specific to record-session. + // THIS TEST MUST FAIL TODAY (#3756) for the same reason as above. + test('addDecisionOnArchivedMilestoneDoesNotZeroProgress', (t) => { + const cwd = createTempDir('gsd-3871-add-decision-'); + t.after(() => cleanup(cwd)); + buildArchivedMilestoneFixture(cwd); + + const result = runGsdTools( + ['state', 'add-decision', '--phase', '05-01', '--summary', 'Ship v1.0', '--rationale', 'milestone complete'], + cwd, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + assertCuratedProgressSurvived(cwd); + }); + + // The over-preservation guard: a genuinely empty project (no phases + // anywhere, no curated progress block at all) must still report zeros + // after the same verb — the fix for #3756 must not make + // applyStatePreservation invent a nonzero progress block out of nothing. + // This test MUST PASS both today and after the fix. + test('newProjectWithNoPhasesKeepsZeroProgress', (t) => { + const cwd = createTempProject('gsd-3871-empty-'); + t.after(() => cleanup(cwd)); + + fs.writeFileSync( + path.join(cwd, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + + const result = runGsdTools(['state', 'record-session', '--stopped-at', 'Nothing started yet'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const written = fs.readFileSync(statePath, 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(written); + const progress = fm && fm.progress; + + // No curated progress ever existed, so exactly one deterministic outcome + // is acceptable — the block is entirely absent, or every count in it is + // a genuine, well-formed 0. Some assertion MUST run either way (no `if` + // guard around the assertion itself); which branch it took is stated in + // the failure message so a silent pass-through cannot hide which case + // fired. `Number.isFinite` (not `Number(x) || 0`) so a garbage/NaN value + // is caught rather than laundered into a false 0. + if (!progress) { + assert.strictEqual(progress, undefined, 'case: no progress block at all — this is the accepted degraded outcome for a project with no curated progress'); + } else { + const counters = { + total_phases: Number(progress.total_phases), + completed_phases: Number(progress.completed_phases), + total_plans: Number(progress.total_plans), + completed_plans: Number(progress.completed_plans), + }; + for (const [key, value] of Object.entries(counters)) { + assert.ok(Number.isFinite(value), `case: progress block present — ${key} must coerce to a finite number, got ${JSON.stringify(progress[key])}`); + assert.strictEqual(value, 0, `case: progress block present — ${key} must be exactly 0 on an empty project, not inflated`); + } + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// ADR-3473 §8.6 test matrix rows 31-33 (.gsd/phase/feat-3871-state-transaction- +// snapshot/50-test-matrix.md): consumer-output identity for the two +// sanctioned `rebuild()` exceptions (`state sync`, `/gsd-health --repair`'s +// REGENERATE_STATE) and the second producer of the same composition +// (`phase complete`'s atomic-commit adapter in src/phase.cts). +// ───────────────────────────────────────────────────────────────────────────── + +describe('ADR-3473 §8.6 matrix row 31: state sync still lets the body win (#905, rebuild() did not invert the command)', () => { + let tmpDir; + beforeEach(() => { tmpDir = createFixture(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('stateSyncStillLetsTheBodyWin', () => { + const content = [ + '---', + 'gsd_state_version: 1.0', + 'stopped_at: "curated stale value — must NOT survive"', + '---', + '', + '# Project State', + '', + '## Session', + '', + '**Stopped at:** fresh body value from a contradicting body — must win', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), content); + + const result = runGsdTools('state sync', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(state); + assert.strictEqual( + fm.stopped_at, + 'fresh body value from a contradicting body — must win', + 'the body must win over the frontmatter it contradicts — proves routing cmdStateSync through rebuild() did not invert the command', + ); + }); +}); + +describe('ADR-3473 §8.6 matrix row 32: REGENERATE_STATE still factory-resets and tolerates NO frontmatter at all', () => { + // NOTE — discrepancy from the brief's literal instruction, verified + // empirically (not assumed): driving this through the real CLI + // (`runGsdTools('validate health --repair', ...)`) cannot exercise this + // row. Two independent reasons, both confirmed against the built lib: + // 1. `REMEDY_ACTION.REGENERATE_STATE`'s risk is DESTRUCTIVE + // (health-diagnostic-rules/root-existence.cjs's E004 rule), and + // `applyRepairs`'s dispatch gate refuses every DESTRUCTIVE remedy + // unconditionally — `--repair` NEVER actually calls + // `rebuildStateTransaction` for it (see + // tests/verify-health.test.cjs "refuses to regenerate STATE.md when + // missing" and tests/health-diagnostic.test.cjs row 15, both pinning + // the refusal-only contract). + // 2. Independently, E004 itself only fires when + // `snapshot.currentPhaseLabel.scope === SCOPE.UNREADABLE` — a STATE.md + // that exists, is readable, and simply has NO frontmatter block does + // NOT trip that scope. Verified live: `validate health --repair` json + // against exactly this fixture returns `"errors": []` and never + // surfaces a `regenerateState` action at all. + // So the CLI can never reach this code path for this row, by policy (1) + // and by detection (2) independently. The invocation shape that DOES + // reach it is the one `runRepairAction`'s REGENERATE_STATE case (and the + // existing "D3" test in this same file, ADR-3408 §8.5 Matrix, Section A) + // already use: `stateLib.writeStateMd` + `stateTransitionMod + // .rebuildStateTransaction({ snapshot: extractFrontmatter(priorState) })` + // directly — the exact call `runRepairAction`'s REGENERATE_STATE case + // makes. This test drives that shape with a prior STATE.md carrying + // literally NO frontmatter block (not merely a missing file). + test('regenerateStateStillFactoryResetsAndToleratesNoFrontmatter', (t) => { + const tmp = createTempDir('gsd-3871-row32-'); + t.after(() => cleanup(tmp)); + const statePath = path.join(tmp, 'STATE.md'); + const noFrontmatterContent = [ + '# Session State', + '', + 'No frontmatter here at all — a broken document, the usual reason', + 'REGENERATE_STATE fires in the first place.', + '', + ].join('\n'); + fs.writeFileSync(statePath, noFrontmatterContent); + + const priorSnapshot = frontmatterLib.extractFrontmatter(noFrontmatterContent, statePath); + assert.deepStrictEqual(priorSnapshot, {}, 'precondition: extractFrontmatter must return {} (never throw/null) for a document with no frontmatter'); + + let tx; + assert.doesNotThrow(() => { + tx = stateTransitionMod.rebuildStateTransaction({ snapshot: priorSnapshot }); + }, 'rebuildStateTransaction must NOT raise a construction failure for the {} snapshot of a frontmatter-less document'); + + const regenerated = [ + '# Session State', '', '## Position', '', + '**Current phase:** (determining...)', '**Status:** Resuming', '', + ].join('\n'); + + stateLib.writeStateMd(statePath, regenerated, tx, tmp); + + const onDisk = fs.readFileSync(statePath, 'utf8'); + const fm = frontmatterLib.extractFrontmatter(onDisk); + assert.ok(fm && fm.gsd_state_version, 'the factory reset must produce a fresh, well-formed frontmatter block'); + assert.strictEqual(fm.status, 'Resuming', 'the regenerated content must be what was written, not the old (nonexistent) curated content'); + assert.match(onDisk, /## Position/, 'the regenerated body must be the fresh factory-reset content, not the old prose'); + }); +}); + +describe('ADR-3473 §8.6 matrix row 33: phase complete\'s adapter uses the identical transaction/preservation composition (#3374 Variant A stays fixed)', () => { + let tmpDir; + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('phaseCompleteAdapterUsesTheSameTransaction', () => { + const planningDir = path.join(tmpDir, '.planning'); + const phase1Dir = path.join(planningDir, 'phases', '01-foundation'); + const phase2Dir = path.join(planningDir, 'phases', '02-api'); + fs.mkdirSync(phase1Dir, { recursive: true }); + fs.mkdirSync(phase2Dir, { recursive: true }); + + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '- [ ] Phase 1: Foundation', + '- [ ] Phase 2: API', + '', + '### Phase 1: Foundation', + '**Goal:** Setup', + '**Plans:** 1 plans', + '', + '### Phase 2: API', + '**Goal:** Build API', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Foundation | 0/1 | Not started | - |', + '| 02. API | 0/1 | Not started | - |', + '', + ].join('\n'), + ); + + // A curated `paused_at` frontmatter value with NO corresponding body + // source line — its body delta compares as unchanged (nothing to + // disagree with), so a stale/absent derived value would clobber it if + // cmdPhaseComplete's adapter did NOT route through the identical + // applyStatePreservation dispatch a `state` verb uses. + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'paused_at: "curated pause note — must survive phase complete"', + '---', + '', + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Current Plan:** 01-01', + '**Last Activity:** 2025-01-01', + '**Last Activity Description:** Working on phase 1', + '', + ].join('\n'), + ); + + fs.writeFileSync(path.join(phase1Dir, '01-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phase1Dir, '01-01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync( + path.join(phase1Dir, '01-VERIFICATION.md'), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + + const result = runGsdTools(['phase', 'complete', '1'], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.ok( + Array.isArray(output.preservation_warnings) && output.preservation_warnings.some((w) => w.field === 'paused_at'), + `expected paused_at named in preservation_warnings — same policy a state verb reports; got ${JSON.stringify(output.preservation_warnings)}`, + ); + + const state = fs.readFileSync(path.join(planningDir, 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(state); + assert.strictEqual( + fm.paused_at, + 'curated pause note — must survive phase complete', + 'the curated field must survive phase complete via the SAME composition (syncAndPreserveStateMd -> applyStatePreservation) a state verb uses', + ); + }); +}); + +// #3834/#3835/#3836 share the epic #3473 thesis: FIELD_CLASSIFICATION declares +// a field's preservation policy once (src/state-transition.cts), and each of +// these three is a call site that either defeats the delta heuristic by +// rewriting the exact body source it compares against in the same write +// (#3834, #3835), or maintains a hand-typed field list parallel to the table +// that had already drifted from it (#3836). +describe('#3834/#3835/#3836: current_phase_name / last_activity_desc preservation call-site gaps', () => { + function buildCuratedNameFixture(cwd) { + const planningDir = path.join(cwd, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '## v1.0 Demo Milestone', + '', + '### Phase 1: First Phase', + '**Goal:** demo', + '', + '### Phase 2: Second Phase Real Name', + '**Goal:** demo', + '', + '### Phase 3: Third Phase', + '**Goal:** demo', + '', + ].join('\n'), + ); + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Demo Milestone', + 'current_phase: 1', + 'current_phase_name: Real Curated Name', + 'current_plan: 0', + 'status: executing', + 'stopped_at: "Phase 1 done"', + 'last_updated: "2026-08-25T00:00:00.000Z"', + 'last_activity: "2026-08-25"', + 'last_activity_desc: "Fresh curated description"', + 'progress:', + ' total_phases: 3', + ' completed_phases: 1', + ' total_plans: 6', + ' completed_plans: 2', + ' percent: 33', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + '**Phase:** 1 — Real Curated Name', + '**Current Plan:** 0', + '**Status:** executing', + '', + '## Session', + '', + '**Stopped At:** Phase 1 done', + '**Last Activity:** 2026-08-25', + '**Last Activity Description:** Fresh curated description', + '', + ].join('\n'), + ); + } + + // THIS TEST MUST FAIL BEFORE THE #3834 FIX: `state planned-phase` without + // `--name` rewrites the body `Phase:` source line to `N — READY TO EXECUTE` + // in the same write the preserve-when-unchanged delta rule compares + // against, so the rule cannot fire and the post-sync re-derivation harvests + // the status fragment as if it were the curated name. + test('plannedPhaseWithoutNameDoesNotClobberCuratedPhaseName', (t) => { + const cwd = createTempDir('gsd-3834-planned-phase-'); + t.after(() => cleanup(cwd)); + buildCuratedNameFixture(cwd); + + const result = runGsdTools(['state', 'planned-phase', '--phase', '2', '--plans', '3'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const written = fs.readFileSync(statePath, 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(written); + assert.strictEqual( + fm.current_phase_name, + 'Real Curated Name', + `current_phase_name must keep the curated value, not the status fragment "READY TO EXECUTE" — got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + + // THIS TEST MUST FAIL BEFORE THE #3835 FIX: `state complete-phase` rewrites + // the body `Phase:` source line to `N — COMPLETE` unconditionally, which + // defeats the same delta rule and DROPS the current_phase_name key from + // persisted frontmatter entirely (not merely blanks it). + test('completePhaseDoesNotDropCuratedPhaseName', (t) => { + const cwd = createTempDir('gsd-3835-complete-phase-'); + t.after(() => cleanup(cwd)); + buildCuratedNameFixture(cwd); + // Phase directories so `progress` is legitimately re-derived from disk + // and does not confound the current_phase_name assertion below. + const phasesDir = path.join(cwd, '.planning', 'phases'); + fs.mkdirSync(path.join(phasesDir, '01-first-phase'), { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '02-second-phase'), { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '03-third-phase'), { recursive: true }); + fs.writeFileSync(path.join(phasesDir, '01-first-phase', '1-01-SUMMARY.md'), '---\nphase: 1\nplan: 01\nstatus: complete\n---\n# s\n'); + fs.writeFileSync(path.join(phasesDir, '01-first-phase', '1-01-PLAN.md'), '---\nphase: 1\nplan: 01\n---\n# p\n'); + fs.writeFileSync(path.join(phasesDir, '02-second-phase', '2-01-PLAN.md'), '---\nphase: 2\nplan: 01\n---\n# p\n'); + fs.writeFileSync(path.join(phasesDir, '03-third-phase', '3-01-PLAN.md'), '---\nphase: 3\nplan: 01\n---\n# p\n'); + + const result = runGsdTools(['state', 'complete-phase', '1'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const written = fs.readFileSync(statePath, 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(written); + assert.strictEqual( + fm.current_phase_name, + 'Real Curated Name', + `current_phase_name must survive complete-phase, not be deleted from frontmatter entirely — got ${JSON.stringify(fm.current_phase_name)}`, + ); + }); + + // THIS TEST MUST FAIL BEFORE THE #3836 FIX: `cmdStateJson`'s hand-maintained + // preserve-when-unchanged field list omits `last_activity_desc` (declared + // preserve-when-unchanged in FIELD_CLASSIFICATION and wired on the write + // path per #3258), so a stale body-prose "Last Activity Description:" line + // beats a fresher curated frontmatter value on every `state json` read. + test('stateJsonPreservesCuratedLastActivityDescOverStaleBodyProse', (t) => { + const cwd = createTempDir('gsd-3836-state-json-'); + t.after(() => cleanup(cwd)); + const planningDir = path.join(cwd, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '## v1.0 Demo Milestone', + '', + '### Phase 1: First Phase', + '**Goal:** demo', + '', + '### Phase 2: Second Phase Real Name', + '**Goal:** demo', + '', + ].join('\n'), + ); + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Demo Milestone', + 'current_phase: 2', + 'current_phase_name: Second Phase Real Name', + 'current_plan: 1', + 'status: executing', + 'stopped_at: "Phase 2 plan 1 in flight"', + 'last_updated: "2026-08-25T00:00:00.000Z"', + 'last_activity: "2026-08-25"', + 'last_activity_desc: "FRESH CURATED DESCRIPTION"', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + '**Phase:** 2 — Second Phase Real Name', + '**Current Plan:** 1', + '**Status:** executing', + '', + '## Session', + '', + '**Stopped At:** Phase 2 plan 1 in flight', + '**Last Activity:** 2026-08-25', + '', + '## Session Continuity Archive', + '', + '**Last Activity Description:** STALE ARCHIVED DESCRIPTION FROM 2025', + '', + ].join('\n'), + ); + + const before = fs.readFileSync(path.join(planningDir, 'STATE.md'), 'utf-8'); + const result = runGsdTools(['state', 'json'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const after = fs.readFileSync(path.join(planningDir, 'STATE.md'), 'utf-8'); + assert.strictEqual(after, before, '`state json` is read-only and must not mutate STATE.md'); + + const parsed = JSON.parse(result.output); + assert.strictEqual( + parsed.last_activity_desc, + 'FRESH CURATED DESCRIPTION', + `last_activity_desc must report the curated frontmatter value, not the stale archived body prose — got ${JSON.stringify(parsed.last_activity_desc)}`, + ); + }); +});