From 3b18eff3887967adcdfc49411b0642acc9ccf925 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 25 Aug 2026 23:51:16 -0400 Subject: [PATCH] =?UTF-8?q?enhance(#3872):=20what=20a=20command=20reports?= =?UTF-8?q?=20it=20wrote=20=E2=80=94=20the=20transaction=20diff=20(#3878)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3872): failing-first regressions for what a command reports it wrote Pins ADR-3473 §8.7 at the consumer's output. state planned-phase advances current_phase on disk and never reports it, and reports progress.total_plans which reconcileReportedFields silently drops because it cannot resolve a dotted key against nested frontmatter. Both directions of #3818's own before/after diff, reproduced against the real CLI. Also pins the two properties the change must not break: a fully-failed patch still reports an empty updated array, which is what state.cts:607's success boolean depends on; and two content-identical writes differ in last_updated alone. That second one measured state_head NOT to be ambient — it is recomputed every write but only changes when git HEAD moved — so the provenance exclusion is a one-element set, with a companion test pinning that state_head does change when HEAD moves. Refs #3872 * feat(#3872): derive what a command reports from the transaction diff ADR-3473 §8.7. reconcileReportedFields compared the transform's own output against persisted bytes and then filtered what preservation had restored by its FIELD_CLASSIFICATION policy. Both are replaced by one comparison of persisted against the pre-write state the transaction already holds, surfaced to the command through the same caller-allocates out-param idiom divergedFields established. Both of the old directions fall out of that single comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out, and a field nobody reported but the write moved is different and appears. The classification filter is deleted, not relocated — no policy test remains anywhere in the reporting path. Reporting is at dotted-leaf granularity, enumerated from the progress.* rows FIELD_CLASSIFICATION already declares rather than by walking user data to arbitrary depth. That closes a live defect: plannedPhaseCore already pushed progress.total_plans and reconcileReportedFields silently dropped it, because a flat hasOwnProperty cannot resolve a dotted key against nested frontmatter. Current Position was lost the same way and is fixed in the same place. The exclusion is one field, last_updated, and it is by provenance rather than by classification: it is the only field measured to change on every write regardless of content. state_head was measured NOT to qualify — it is recomputed every write but only changes when git HEAD moved. Without that exclusion state.patch's success boolean, which is updated.length > 0, would be permanently true and a fully-failed patch would report success. Refs #3872 * fix(#3872): cover the matrix, and close a prototype-chain read the coverage found Review found 20 of 29 test-matrix rows uncovered. Covering them found two real defects rather than merely documenting the intended behavior. bodyLabelFor read FRONTMATTER_KEY_TO_BODY_LABEL with a bare bracket index on a plain object literal, so a field named __proto__, constructor or toString resolved to the inherited prototype member and leaked a non-string value into the updated array. Fixed with an own-property check, mirroring the discipline resolveFrontmatterPath already had. The security-relevant matrix row proved it before the fix. applyPostSyncPreservation still carried its own inline copy of the value comparison alongside the new stateFieldValuesDiffer, which is two live copies of one rule introduced by the epic that exists to remove them. Routed through the single owner. Adds the fast-check property that a field appears iff its persisted value differs from the snapshot, the string-versus-number representation boundary, dotted paths into missing parents and into scalars, deleted and added keys, and the preserve-if-placeholder pair that proves no classification test survives in the reporting path. Refs #3872 * docs(#3872): document the transaction diff on the write path The updated array's contract belongs where the write path is described. States the iff rule, leaf granularity, the single provenance exclusion and why state_head is deliberately not one, and closes with the consequence a reader actually needs: these arrays are longer than they used to be, because they used to under-report. Refs #3872 * fix(#3872): a derived leaf materializing is not a change the caller made The remote matrix caught 17 failures with two causes. The substantive one is that progress is source: disk, and the disk cannot change during a STATE.md write — the write only touches STATE.md. So a progress block appearing where the snapshot had none is the scanner populating a document that had never been synced. The bytes moved; nothing the caller did moved them. That is the same shape as last_updated one level up, so the provenance rule is generalized rather than special-cased: a field appears iff its persisted value changed for a reason attributable to this write's action, and two cases are not attributable — a field stamped unconditionally on every save, and a declared derived leaf materializing from a source that did not change. Crucially this does not consult the preservation policy, so the filter §8.7 deleted stays deleted; it uses the declared leaf set to know which keys are derived. This had a second production consumer the earlier review concluded did not exist: cmdStatePlannedPhase gates publishStateContract on updated.length, and its own inline comment predicts exactly this failure. A no-op call was publishing state.json. advancePlanNoOpDoesNotPublish genuinely encoded pre-§8.7 behavior and moves. E2 and E6 had carved out total_plans as reportable-on-materialization, an error introduced earlier on this branch rather than a pre-existing pin, and are corrected with it. Refs #3872 * chore(#3872): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/sharp-cranes-wander.md | 5 + CONTEXT.md | 2 +- docs/ARCHITECTURE.md | 8 + src/state.cts | 565 +++++++++++--- tests/frontmatter.test.cjs | 17 +- tests/state-contract.test.cjs | 32 +- tests/state.test.cjs | 1205 ++++++++++++++++++++++++++++- 7 files changed, 1689 insertions(+), 145 deletions(-) create mode 100644 .changeset/sharp-cranes-wander.md diff --git a/.changeset/sharp-cranes-wander.md b/.changeset/sharp-cranes-wander.md new file mode 100644 index 000000000..8d749e002 --- /dev/null +++ b/.changeset/sharp-cranes-wander.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3878 +--- +**`state.*` commands now report every field they actually changed, and only those** — the `updated` array is derived by diffing what was persisted against the pre-write state, so a counter the write genuinely moved is no longer suppressed, a field the write merely preserved is no longer claimed as an update, and a changed sub-counter is named at leaf granularity (`progress.total_plans`) instead of being dropped. Callers that compared the array exactly will see more, and truer, entries. (#3872) diff --git a/CONTEXT.md b/CONTEXT.md index 741858a20..9c425605d 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -65,7 +65,7 @@ Module owning projection from dispatch results/errors to CLI `{ exitCode, stdout Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope for the read path; `state.cts`'s `matchCurrentPositionSection` is a thin alias over it, and the `drift-guard phase-status` seam consumes it, so the #2956 archive-shadowing fix cannot be re-derived into a second copy — the byte-exact mutation path served by `state-transition.cts`'s `locateCurrentPosition`/`sliceCurrentPositionSection` is a deliberately separate, un-consolidated locator, #3187). `stateFieldValue` (#3187) is the single owner of the #1760 frontmatter-then-body field fallback chain, consolidating the 14 re-derivations of that ladder onto one scope-carrying (`complete`/`truncated`/`unscoped`/`unreadable`) primitive. `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. **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). +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.7 (#3872) — the transaction diff.** `reconcileReportedFields` no longer compares the transform's own output against persisted bytes, nor filters `divergedFields` by policy class. It compares **persisted against the pre-write state the transaction already holds**, surfaced to the command through the same caller-allocates out-param idiom `divergedFields` established (and which `readModifyWriteStateMd`'s hand-enumerated option forwarding must list, or the field is silently dropped). Both prior directions fall out of that one comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out (#3351), and a field nobody reported but the write moved is different and appears (#3345's `total_phases` 7→4, #3818's `current_phase` 203→204). The classification filter is **deleted, not relocated** — no `getFieldClassification` test survives in the reporting path. Reporting is at **dotted-leaf** granularity, enumerated from the `progress.*` rows `FIELD_CLASSIFICATION` already declares rather than by walking user data to arbitrary depth; that closes a live drop, because `plannedPhaseCore` already pushed `progress.total_plans` and a flat `hasOwnProperty` could never resolve it against nested frontmatter (`Current Position` was lost the same way). The one exclusion is `last_updated`, and it is by **provenance, not classification**: it is the only field measured to change on every write regardless of content. `state_head` was measured NOT to qualify — recomputed every write, but its value moves only when git HEAD moved. Without that single exclusion, `state.patch`'s success signal (`updated.length > 0`, `state.cts`) would be permanently true and a fully-failed patch would report success. **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 c0c823808..a9d5579ea 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -557,6 +557,14 @@ STATE.md carries the same fact in two places — YAML frontmatter and the docume `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. +**What a command reports it wrote is the same snapshot, read back (ADR-3473 §8.7).** Every `state.*` command returns an `updated` array. That array is now derived by comparing what was actually persisted against the transaction's pre-write snapshot: **a field appears if, and only if, its persisted value changed.** One comparison answers both of the questions that used to need separate machinery — a field the caller asked for that the pipeline then discarded is persisted-equals-snapshot and drops out, and a field nobody asked for that the write moved anyway is different and appears. Nothing is filtered by its preservation policy. + +Reporting is at **leaf granularity**: when a single counter moves you are told `progress.total_plans`, not `progress`. The leaves are the ones the field-classification table already declares, so the report is bounded by a schema rather than by walking the document. + +One field is excluded, and it is excluded for its *provenance* rather than its policy: `last_updated` is stamped on every save regardless of what you changed, so admitting it would make `state patch`'s success signal — which is simply whether `updated` is non-empty — permanently true, and a patch in which every field failed would report success. `state_head` is deliberately **not** excluded: it is recomputed on every save but only *changes* when the commit it records actually moved, so reporting it tells you something true. + +A practical consequence worth knowing: these arrays are now longer than they used to be, because they used to under-report. If you compare one exactly, expect more entries — and expect them to be the ones that really changed. + **[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. **[ADR-3473](adr/3473-enforcement-by-construction.md) owns the invariants that sit outside that contract.** ADR-3408 governs what survives a write; it does not govern the pipeline's precondition, what a command reports it wrote, or where the set of STATE.md keys, types and enums is declared. ADR-3473 owns those, alongside document parsing, enumeration, and the return contract of every routine that can fail. It is the third application of ADR-3180's mechanism and the first whose success metric requires the guard surface to *shrink* as each seam lands. diff --git a/src/state.cts b/src/state.cts index 3b11ae7d4..600cf3f9a 100644 --- a/src/state.cts +++ b/src/state.cts @@ -67,6 +67,9 @@ const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = // #3699: the frontmatter-key <-> body-field routing behind `state update`'s // failure explanation, and the classification table it falls back to. const { getFieldClassification, getFrontmatterBodySource, frontmatterKeyForBodyField } = stateTransitionMod; +// ADR-3473 §8.7 (#3872): the declared dotted-leaf enumeration `reconcileReportedFields` +// diffs against — see `declaredLeavesOf` below. +const { FIELD_CLASSIFICATION } = stateTransitionMod; type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord; @@ -115,6 +118,25 @@ interface StateLockClock { sleep(ms: number): void; } +/** + * ADR-3473 §8.7 (#3872): the transaction's pre-write snapshot, threaded from + * `applyPostSyncPreservation` to `reconcileReportedFields` via + * `ReadModifyWriteOptions.preWriteState` (see that field's own docstring for + * the out-param idiom). `bodyDeltas` is the SAME pre/post body-source map + * `applyPostSyncPreservation` builds for preservation — reused (not + * re-derived) as the "did THIS write's own transform actually change the + * body source" signal for every `FRONTMATTER_BODY_SOURCE` key, since those + * keys are re-derived into frontmatter from the body on EVERY write + * regardless of whether this write touched them (see + * `computeChangedFrontmatterFields`'s docstring for why a raw frontmatter + * diff over-reports for this key set). + */ +interface StatePreWriteSnapshot { + fm?: Record; + body?: string; + bodyDeltas?: Record; +} + interface ReadModifyWriteOptions { resync?: boolean; /** #2440: when true, total_plans/total_phases take derived values even under !resync. */ @@ -156,6 +178,24 @@ interface ReadModifyWriteOptions { * `resync` is driven by `shouldResyncStateProgress` rather than defaulting. */ explicitProgressField?: boolean; + /** + * ADR-3473 §8.7 (issue #3872): caller-allocated out-param, following the + * SAME idiom as `divergedFields` above (caller allocates an empty box, + * the pipeline fills it) — `applyPostSyncPreservation` populates `.fm` + * with the transaction's OWN pre-write snapshot (`transaction.snapshot`, + * the exact object `openStateTransaction`/`rebuildStateTransaction` was + * constructed with — never a second `extractFrontmatter` derivation of + * `originalContent`, which is the divergence this epic exists to remove) + * and `.body` with the pre-write BODY (`stripFrontmatter(originalContent)`) + * that the body-label comparison in `reconcileReportedFields` needs and + * the frontmatter snapshot does not carry. Left entirely `undefined` when + * `readModifyWriteStateMd`'s own #948 no-op guard fires (the transform's + * raw output was byte-identical to the input, so nothing was ever + * synced/preserved/written) — a consumer must treat "unset" as "nothing + * changed", never coerce it to an empty snapshot (an empty `{}` would + * make every already-persisted key look newly ADDED per §8.7's diff). + */ + preWriteState?: StatePreWriteSnapshot; } /** @@ -576,14 +616,15 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea // delta (table-driven) that this phase adds. Field-name validation (security) // and the resync-progress decision stay in this adapter. let precomputed: { updated: string[]; failed: string[] } = { updated: [], failed: [] }; - let preSyncContent = ''; const divergedFields: string[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; readModifyWriteStateMd(statePath, (content) => { const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock }); precomputed = (result.data as { updated: string[]; failed: string[] }) ?? precomputed; - preSyncContent = result.content; return result.content; - }, cwd, { resync: shouldResync, divergedFields, explicitProgressField: shouldResync }); + }, cwd, { resync: shouldResync, divergedFields, explicitProgressField: shouldResync, preWriteState }); // ADR-3408 §8.4 (D4, fix(#3351) generalized — see `reconcileReportedFields`): // patchCore's bookkeeping says whether the stateReplaceField text-replace @@ -599,7 +640,7 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea // `applyStatePreservation` restored that this patch never named at all // (#3345's direction), a case the pre-#3471 version of this command never // covered. - const updated = reconcileReportedFields(statePath, preSyncContent, precomputed.updated, divergedFields); + const updated = reconcileReportedFields(statePath, preWriteState, precomputed.updated, divergedFields); const updatedSet = new Set(updated); const failed = Object.keys(patches).filter((field) => !updatedSet.has(field)); const results = { updated, failed }; @@ -674,7 +715,9 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | const statePath = planningPaths(cwd).state; try { let updated = false; - let preSyncContent = ''; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; let transitionData: Record | undefined; // #3699 case D: when `updateCore` falls back to writing the frontmatter key // directly, the value must survive the post-sync pass — otherwise the write @@ -715,9 +758,8 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | if (transitionData?.wroteFrontmatter === true) { authoritativeFm[field as string] = value; } - preSyncContent = result.content; return result.content; - }, cwd, { resync: shouldResync, divergedFields, authoritativeFm, explicitProgressField: shouldResync }); + }, cwd, { resync: shouldResync, divergedFields, authoritativeFm, explicitProgressField: shouldResync, preWriteState }); // ADR-3408 §8.4 (D4): reconcile against the bytes actually persisted — // `updateCore`'s own match does not know whether sync/preservation later @@ -726,7 +768,7 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | // restored during this write that this command never touched at all // (#3345's direction) — reported separately from `updated` because this // command's contract is a single-field boolean, not a per-field array. - const reconciled = reconcileReportedFields(statePath, preSyncContent, updated ? [field as string] : [], divergedFields); + const reconciled = reconcileReportedFields(statePath, preWriteState, updated ? [field as string] : [], divergedFields); updated = reconciled.includes(field as string); const preserved = reconciled.filter((f) => f !== field); @@ -803,8 +845,10 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { let resultData: Record | undefined; let precomputedUpdated: string[] = []; - let preSyncContent = ''; const divergedFields: string[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; // #3311: the milestone (phase + session) claim is consulted INSIDE the // STATE.md lock, so the position read and the claim read cannot interleave // with another session's Current Position write. @@ -827,9 +871,8 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { const result = transitionCore(content, intent, deps); resultData = result.data; precomputedUpdated = result.updated; - preSyncContent = result.content; return result.content; - }, cwd, { divergedFields }); + }, cwd, { divergedFields, preWriteState }); if (!resultData || resultData['error']) { output({ error: 'Cannot parse Current Plan or Total Plans in Phase from STATE.md' }, raw, undefined); @@ -842,7 +885,7 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { // Generalizes fix(#3351) (closes #3351's direction) and folds in any field // preservation restored that this transform never touched (#3345's // direction). - const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields); + const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields); if (resultData['advanced'] === false) { output({ ...resultData, updated, milestone_conflict: milestoneConflict }, raw, 'false'); @@ -1531,8 +1574,10 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, const now = realClock.nowIso(); const updated: string[] = []; let sessionCreated = false; - let preSyncContent = ''; const divergedFields: string[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; readModifyWriteStateMd(statePath, (content) => { // Update Last session / Last Date @@ -1738,15 +1783,14 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, } } - preSyncContent = content; return content; - }, cwd, { divergedFields }); + }, cwd, { divergedFields, preWriteState }); // ADR-3408 §8.4 (D4): reconcile this command's own success list against the // bytes actually persisted (fix(#3351) generalized) and fold in any field // preservation restored that this transform never touched (#3345's // direction). - const reconciledUpdated = reconcileReportedFields(statePath, preSyncContent, updated, divergedFields); + const reconciledUpdated = reconcileReportedFields(statePath, preWriteState, updated, divergedFields); if (reconciledUpdated.length > 0) { const result: Record = { recorded: true, updated: reconciledUpdated }; @@ -3325,7 +3369,7 @@ function applyPostSyncPreservation( options: StatePreservationOptions, ): string { assertStatePreservationOptions(options, 'applyPostSyncPreservation'); - const { resync, authoritativeFm, deriveProgressKeys, divergedFields, explicitProgressField } = options; + const { resync, authoritativeFm, deriveProgressKeys, divergedFields, explicitProgressField, preWriteState } = options; // Bug #1230: delta heuristic — snapshot pre-transform body source fields so // we can detect whether THIS write changed them. syncStateFrontmatter @@ -3421,6 +3465,16 @@ function applyPostSyncPreservation( status: { pre: preBodyStatus, post: postBodyStatus }, stopped_at: { pre: preBodyStoppedAt, post: postBodyStoppedAt }, current_phase_name: { pre: preBodyPhaseSource, post: postBodyPhaseSource }, + // ADR-3473 §8.7 (#3872): `last_activity` is the one `FRONTMATTER_BODY_SOURCE` + // key that is NOT `preserve-when-unchanged` (it is `derive` — always + // re-stamped from the body) and so was never part of this map before. + // Added ONLY for `reconcileReportedFields`'s consumption below (via + // `preWriteState.bodyDeltas`) — harmless here, since + // `applyPreserveWhenUnchanged` is dispatched by + // `getPreserveWhenUnchangedFields()`, never by iterating this object's + // keys, so an extra non-preserve-when-unchanged entry changes no + // preservation behavior. + last_activity: { pre: preBodyLastActivityRaw, post: postBodyLastActivityRaw }, }; // ADR-1769 #1796 (Path A — finish the consolidation): the post-sync @@ -3454,6 +3508,20 @@ function applyPostSyncPreservation( bodyDeltas, explicitProgressField: explicitProgressField === true, }); + // ADR-3473 §8.7 (#3872): fill the caller's out-param with the TRANSACTION'S + // OWN snapshot object (not a second `extractFrontmatter(originalContent)` + // derivation — `transaction.snapshot === preFmSnapshot`, reusing it is the + // whole point) plus the pre-write body, so `reconcileReportedFields` can + // diff persisted-vs-pre-write instead of re-deriving either side itself. + if (preWriteState) { + preWriteState.fm = transaction.snapshot; + preWriteState.body = preBody; + // ADR-3473 §8.7 (#3872): the pre/post body-source delta for every + // FRONTMATTER_BODY_SOURCE key — see `StatePreWriteSnapshot`'s docstring + // for why `reconcileReportedFields` needs this instead of a raw + // frontmatter diff for these specific keys. + preWriteState.bodyDeltas = bodyDeltas; + } const preservation = applyStatePreservation({ transaction, postFm }); if (divergedFields && preservationInputSnapshot) { // §8.5's "liberal but visible": every field whose value actually @@ -3466,10 +3534,13 @@ function applyPostSyncPreservation( for (const key of Object.keys(preservation.postFm)) { const before = preservationInputSnapshot[key]; const after = preservation.postFm[key]; - const changed = (typeof before === 'object' || typeof after === 'object') - ? JSON.stringify(before) !== JSON.stringify(after) - : before !== after; - if (changed) divergedFields.push(key); + // ADR-3473 §8.7 (#3872 standards-axis finding): route through the ONE + // owner of this comparison rule (`stateFieldValuesDiffer`, defined + // below) instead of carrying a second inline `JSON.stringify`-vs-`!==` + // copy — this is exactly the duplicated-rule shape this epic exists to + // remove. `stateFieldValuesDiffer` is a function declaration (hoisted), + // so calling it here, above its textual definition, is safe. + if (stateFieldValuesDiffer(before, after)) divergedFields.push(key); } // ADR-3408 §8.5 Row 2 (D1's actual bug, the reason the guards had to be // deleted rather than merely relocated): the loop above can only see a @@ -3619,6 +3690,11 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string deriveProgressKeys: options?.deriveProgressKeys === true, divergedFields: options?.divergedFields, explicitProgressField: options?.explicitProgressField === true, + // ADR-3473 §8.7 (#3872): forwarded so `applyPostSyncPreservation` can + // fill it — an unenumerated option here is silently dropped + // (Phase 1's commit message; #3871), which is exactly how a prior cut + // of this option would have gone missing. + preWriteState: options?.preWriteState, }, ); @@ -3676,8 +3752,19 @@ const FRONTMATTER_KEY_TO_BODY_LABEL: Readonly> = Object.f * (e.g. `progress`), not a silent degrade. */ function bodyLabelFor(field: string): string { - const label = FRONTMATTER_KEY_TO_BODY_LABEL[field]; - if (label !== undefined) return label; + // ADR-3473 §8.7 (#3872 review): an OWN-PROPERTY check, never a bare + // bracket read — `FRONTMATTER_KEY_TO_BODY_LABEL` is a plain object literal + // (real `Object.prototype` in its chain), so `[field]` for a hostile field + // named `__proto__`/`constructor`/`toString` returns the INHERITED + // prototype-chain member (`Object.prototype` itself, the `Object` + // constructor function, `Object.prototype.toString`) instead of + // `undefined` — which would then be returned as the "label" and leak a + // non-string value into the caller's `updated` array. Proven by + // `dottedResolutionDoesNotPollutePrototypes` (test matrix row 25) before + // this fix. Mirrors `resolveFrontmatterPath`'s own-property discipline. + if (Object.prototype.hasOwnProperty.call(FRONTMATTER_KEY_TO_BODY_LABEL, field)) { + return FRONTMATTER_KEY_TO_BODY_LABEL[field]; + } const cls = stateTransitionMod.getFieldClassification(field); if (cls && cls.preservation === 'preserve-when-unchanged') { const err = new Error( @@ -3693,102 +3780,347 @@ function bodyLabelFor(field: string): string { } /** - * ADR-3408 §8.4 (D4): shared persisted-bytes reconciliation, generalized - * from fix(#3351)'s `cmdStatePatch`-only version so every RMW-based command - * that reports a per-field `updated` array shares ONE comparison instead of - * re-deriving it per call site — duplicated policy is exactly what this - * epic exists to remove. + * ADR-3473 §8.7 (issue #3872): the provenance exclusion — the ONLY + * frontmatter key measured to change on EVERY write, regardless of content. + * Verified at the CLI (`40-design.md` "Two corrections from reproducing it"): + * two content-identical writes to a git-backed fixture differ in exactly + * this one key. `state_head` was deliberately measured OUT of this set — + * it restamps every write but its PERSISTED VALUE changes only when git HEAD + * actually moved, so it tracks a real fact and does not flood. * - * Closes BOTH directions: - * - **#3351's** (reported-but-discarded): a field the transform's own - * return value held a value for, that sync/preservation then discarded - * or overwrote before the file was saved, must NOT be reported. - * - **#3345's** (persisted-but-unreported): a field `applyStatePreservation` - * restored that the transform never touched at all must still be - * reported — preservation can mutate a field the pre-sync intent never - * knew about. + * A CLOSED, ENUMERATED set — not a predicate or a callback (Greenspun's + * Tenth Rule, ADR-3473 §8.7's Laws section: "the moment it takes a callback + * it has become the classification table again under a new name"). It + * exists to protect `src/state.cts:607` — `state.patch`'s ENTIRE + * success/failure signal is `results.updated.length > 0` — admitting an + * always-changing key here would make that boolean permanently `true`, so a + * fully-failed patch would report success. + */ +const STATE_UPDATED_PROVENANCE_EXCLUSION: readonly string[] = Object.freeze(['last_updated']); + +/** Sentinel: "this dotted path did not resolve to any value" — distinct from every real value including `undefined`/`null`, so absence and an explicit null are never confused. */ +const STATE_FIELD_ABSENT: unique symbol = Symbol('state-field-absent'); + +/** + * ADR-3473 §8.7 (#3872): resolve `path` against a parsed frontmatter object. + * Pure, never throws. * - * @param preSyncContent The transformFn's OWN return value — the content - * BEFORE `syncAndPreserveStateMd` ran this write — captured by the caller - * inside its own `readModifyWriteStateMd` callback. Comparing that against - * the actual bytes on disk after the full write pipeline settled is what - * makes the report reflect what POST-sync bytes hold (ADR-3408 §8.4), - * not what the pre-sync intent merely hoped for. - * @param reported The candidate field names — the transform's OWN - * success list (e.g. `beginPhaseCore`'s `updated`), never a raw intent - * list the transform might not have actually matched. Body Title-Case - * labels (`Status`, `Current Plan`) and frontmatter keys are both valid; - * each is looked up as a frontmatter key first, else as a body field — - * the same fallback chain `cmdStatePatch` used before this generalization. - * @param divergedFields Frontmatter field names `applyStatePreservation` - * actually restored during this write (ADR-3408 §8.5's out-param). + * Order is pinned (test matrix row 26, `literalDottedKeyResolvesBeforePathTraversal`): + * a LITERAL flat key wins first — a field name that happens to contain a `.` + * but is stored as one flat key must not be shadowed by path traversal — + * and only when no literal key exists does `path` get split and walked as a + * dotted path. + * + * Hostile-input rows (23-25 of the test matrix) all resolve to + * `STATE_FIELD_ABSENT` rather than throwing: a missing parent, a scalar + * parent (`typeof cursor !== 'object'`), and — the prototype-pollution + * case — a `__proto__`/`constructor`/`toString` segment. The own-property + * check (`Object.prototype.hasOwnProperty.call`, never a bare `in` or + * bracket read) is what makes the last one safe: an inherited + * `Object.prototype` member is never mistaken for an own data key, and + * because this function only ever READS a segment (never assigns one), + * no prototype can be polluted by walking it. + */ +function resolveFrontmatterPath(fm: Record, path: string): unknown { + if (Object.prototype.hasOwnProperty.call(fm, path)) return fm[path]; + if (!path.includes('.')) return STATE_FIELD_ABSENT; + let cursor: unknown = fm; + for (const segment of path.split('.')) { + if (typeof cursor !== 'object' || cursor === null || Array.isArray(cursor)) return STATE_FIELD_ABSENT; + if (!Object.prototype.hasOwnProperty.call(cursor, segment)) return STATE_FIELD_ABSENT; + cursor = (cursor as Record)[segment]; + } + return cursor; +} + +/** + * ADR-3473 §8.7 (#3872): representation-insensitive equality for a + * persisted-vs-snapshot leaf value (test matrix rows 21/22). Frontmatter + * scalars round-trip as STRINGS (`extractFrontmatter`, §8.1's open type + * question) while an in-memory derivation can hold a real number or boolean + * — a naive `!==` would report every numeric/boolean field changed on every + * write. Mirrors the existing `divergedFields` diff's typeof-object branch + * in `applyPostSyncPreservation` (JSON.stringify for objects, else a + * normalized scalar compare) rather than inventing a second comparison. + * Presence-vs-absence (`STATE_FIELD_ABSENT` on exactly one side) is always a + * change — a deleted or newly-added key (test matrix rows 16/17) — never + * folded into the scalar branch below it. + */ +/** + * ADR-3473 §8.7 (#3872): `String(v)` on an `unknown` is unsafe (a hostile + * object could carry a custom, throwing, or `[object Object]`-degrading + * `toString`) — narrowed per-branch here so each `String()` call below only + * ever runs on a primitive TypeScript itself knows is safe to stringify. + */ +function stateScalarString(v: unknown): string { + if (v === null || v === undefined) return ''; + if (typeof v === 'string') return v; + if (typeof v === 'number' || typeof v === 'boolean' || typeof v === 'bigint') return String(v); + return JSON.stringify(v) ?? ''; +} + +function stateFieldValuesDiffer(before: unknown, after: unknown): boolean { + if (before === STATE_FIELD_ABSENT && after === STATE_FIELD_ABSENT) return false; + if (before === STATE_FIELD_ABSENT || after === STATE_FIELD_ABSENT) return true; + if (typeof before === 'object' || typeof after === 'object') { + return JSON.stringify(before) !== JSON.stringify(after); + } + return stateScalarString(before).trim() !== stateScalarString(after).trim(); +} + +/** + * ADR-3473 §8.7 (#3872): the declared dotted-leaf children of a frontmatter + * key, read off `FIELD_CLASSIFICATION` (`progress` -> its five + * `progress.*` rows) rather than walked from arbitrary nesting depth of a + * user-authored document. A BOUNDED, DECLARED enumeration — the design + * doc's Rejected #5 and the "Emit dotted leaves, not the parent" rule both + * depend on this staying a closed set the schema names, not unbounded + * traversal of whatever object shape happens to be on disk. + */ +function declaredLeavesOf(key: string): string[] { + const prefix = `${key}.`; + return Object.keys(FIELD_CLASSIFICATION).filter((k) => k.startsWith(prefix)); +} + +/** + * ADR-3473 §8.7 (#3872): every frontmatter key — resolved at DOTTED-LEAF + * granularity for a key with declared leaves (`progress` -> only the + * `progress.*` leaves that actually moved, never bare `progress` itself; + * design doc rule 4/Rejected #5) — whose PERSISTED value differs from the + * transaction's pre-write SNAPSHOT. Pure: no I/O, no `FIELD_CLASSIFICATION` + * preservation-policy consultation (that filter is exactly what this rule + * deletes — ADR-3473 §8.7 "no field is excluded by classification"). + * `last_updated` is the one-element provenance exclusion; every other key, + * including `state_head`, is a candidate. + * + * **A `FRONTMATTER_BODY_SOURCE` key is diffed via `bodyDeltas`, never via a + * raw frontmatter compare.** Found while driving the #1264 regression check + * through this rewrite at the CLI: `syncStateFrontmatter` re-derives EVERY + * body-sourced key into frontmatter on EVERY write, independent of whether + * this write's own transform touched it. A hand-authored (or day-1 + * bootstrap) STATE.md whose frontmatter has not yet caught up to an + * already-stable body value — e.g. `current_phase_name` present in the body + * but absent from a pre-write frontmatter block that only ever recorded + * `status`/`progress` — makes that key look newly ADDED under a raw diff + * (rows 15/17) even though nothing changed. The real "did THIS write change + * it" signal for these keys is whether their BODY SOURCE moved, which is + * exactly what `bodyDeltas` (built once, in `applyPostSyncPreservation`, + * from `originalContent` vs `transformedContent`) already answers — reused + * here rather than re-derived, and it is what correctly REPORTS #3818's + * `current_phase` (the body source did move) while staying SILENT on a + * merely-backfilled, body-unchanged key (the #1264 false positive this + * function's first cut produced). + * + * **A declared dotted-leaf (`declaredLeavesOf`, e.g. every `progress.*` row) + * absent from the snapshot and present in persisted is materialization, not + * a change.** Found the same way as the paragraph above, one layer down: + * `progress` is `source: 'disk'` (state-transition.cts), re-derived by + * `buildStateFrontmatter`'s phase-directory scan on every write regardless + * of whether the caller's own action touched it — and the phases directory + * cannot move during a STATE.md write, so a fresh `progress` block appearing + * where the snapshot had none is the scanner catching a never-synced + * document up, not the caller changing anything. This is the SAME + * provenance principle `STATE_UPDATED_PROVENANCE_EXCLUSION` applies to + * `last_updated` (a field stamped by the write's occurrence, not its + * action) — generalized to the declared-leaf case, deliberately NOT a + * second classification-based exclusion: `progress`'s `preserve-always` + * policy plays no part in the check below, and a leaf already PRESENT in + * the snapshot is diffed exactly as every other field is, including + * reporting its outright disappearance (row 16) — only the absent-in- + * snapshot-but-materialized-in-persisted transition is suppressed. + */ +function computeChangedFrontmatterFields( + snapshotFm: Record, + persistedFm: Record, + bodyDeltas: Record | undefined, +): string[] { + const changed: string[] = []; + const topKeys = new Set([...Object.keys(snapshotFm), ...Object.keys(persistedFm)]); + for (const key of topKeys) { + if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(key)) continue; + + if (stateTransitionMod.getFrontmatterBodySource(key) !== null) { + const delta = bodyDeltas ? bodyDeltas[key] : undefined; + if (delta && stateFieldValuesDiffer(delta.pre ?? STATE_FIELD_ABSENT, delta.post ?? STATE_FIELD_ABSENT)) { + changed.push(key); + } + continue; + } + + const leaves = declaredLeavesOf(key); + if (leaves.length > 0) { + for (const leaf of leaves) { + const before = resolveFrontmatterPath(snapshotFm, leaf); + const after = resolveFrontmatterPath(persistedFm, leaf); + // Generalizes the SAME provenance principle STATE_UPDATED_PROVENANCE_EXCLUSION + // applies to `last_updated` one level up — this is NOT a classification-based + // exclusion (progress's `preserve-always` policy plays no part here; that filter + // stays deleted per §8.7). It is a fact about the DECLARED LEAF SET: every key + // enumerated by `declaredLeavesOf` is `source: 'disk'` (state-transition.cts), + // re-derived from a scan that cannot move during a STATE.md write (the write only + // touches STATE.md, never the phases directory). So a leaf ABSENT from the + // pre-write snapshot and PRESENT in persisted is the scanner catching a document + // up to a derivation it had never synced before — the write's own OCCURRENCE + // produced the bytes, not the caller's ACTION, exactly the `last_updated` shape. + // A leaf already PRESENT in the snapshot behaves normally: any difference + // (including disappearing entirely, row 16) is reported, because there the + // snapshot proves the derivation had already run once, so a new persisted value + // can only come from something genuinely moving (#3743/#3818). + if (before === STATE_FIELD_ABSENT && after !== STATE_FIELD_ABSENT) continue; + if (stateFieldValuesDiffer(before, after)) changed.push(leaf); + } + continue; + } + const before = resolveFrontmatterPath(snapshotFm, key); + const after = resolveFrontmatterPath(persistedFm, key); + if (stateFieldValuesDiffer(before, after)) changed.push(key); + } + return changed; +} + +/** + * ADR-3473 §8.7 (issue #3872): the transaction diff. `updated` is derived + * by comparing PERSISTED frontmatter against the transaction's pre-write + * SNAPSHOT — replacing the prior comparison of the transform's own OUTPUT + * against persisted bytes, which answered a different question ("did the + * transform's write survive to disk", #3351) from the one §8.7 asks ("what + * did this write actually change" — both #3351's direction and #3345/#3818's + * fall out of ONE comparison against the pre-write state; see the design + * doc's "ambiguity in §8.7" section for why the transform-output comparison + * was rejected). + * + * No field is excluded by classification — `getFieldClassification` / + * `preservation !== 'preserve-when-unchanged'` is gone, not relocated. The + * ONLY exclusion is `STATE_UPDATED_PROVENANCE_EXCLUSION` (provenance, not + * classification): an unchanged `progress` no longer needs a special filter + * to stay unreported (#1264) because the diff itself says "unchanged" — + * and a GENUINELY changed `progress.*` leaf (#3743, #3818) is no longer + * suppressed by the same filter. + * + * @param preWriteState The transaction's pre-write snapshot + body — the + * `preWriteState` out-param `applyPostSyncPreservation` filled during + * THIS write (see `ReadModifyWriteOptions.preWriteState`'s docstring). + * `.fm`/`.body` are `undefined` only when `readModifyWriteStateMd`'s own + * #948 no-op guard fired (transform output was byte-identical to input), + * in which case nothing was ever written and `[]` is the correct, + * short-circuited answer — never a diff against a synthesized empty `{}` + * snapshot, which would read every already-persisted key as newly ADDED. + * @param reported The candidate field names — the transform's OWN success + * list. Body Title-Case labels (`Status`, `Current Plan`, `Current + * Position`) and frontmatter keys (including dotted leaves like + * `progress.total_plans`) are both valid; each is resolved via the same + * `valueOf` fallback chain used for the inclusion test below. + * @param divergedFields Kept for signature/out-param stability (ADR-3408 + * §8.5) — populated exactly as before by `applyPostSyncPreservation` and + * still read directly by other code and `tests/state.test.cjs`'s A2f case + * — but no longer consulted here as a candidate SOURCE (design doc row + * 18): the frontmatter diff subsumes what it used to contribute, and it + * sees only what *preservation* changed, never what *sync* changed + * (#3818's own direction), which is why keeping it as the candidate + * source was rejected (design doc, Rejected #1). */ function reconcileReportedFields( statePath: string, - preSyncContent: string, + preWriteState: StatePreWriteSnapshot, reported: string[], divergedFields: string[], ): string[] { + void divergedFields; // ADR-3473 §8.7 D18: out-param only, not a candidate source here. + if (preWriteState.fm === undefined || preWriteState.body === undefined) return []; + const persisted = platformReadSync(statePath) || ''; const persistedFm = extractFrontmatter(persisted, statePath) as Record; const persistedBody = stripFrontmatter(persisted); - const preFm = extractFrontmatter(preSyncContent, statePath) as Record; - const preBody = stripFrontmatter(preSyncContent); + const snapshotFm = preWriteState.fm; + const snapshotBody = preWriteState.body; - // #3471 review: body-FIRST, frontmatter-fallback — mirrors the actual write - // precedence `patchCore`/`updateCore` apply (their own docstrings: "a key - // that resolves against the STRIPPED body ... wins deterministically even - // when the same key also happens to exist as a parsed frontmatter key"). - // The prior frontmatter-first order was silently correct for every - // Title-Case body label (`Status`, `Current Plan`, ...) only because those - // never case-exact-match a frontmatter key (frontmatter keys are always - // lowercase snake_case) — so `hasOwnProperty` always missed and it fell - // through to the body anyway. It broke the one case where `field` IS - // lowercase and DOES exact-match a frontmatter key: a table-format - // STATE.md with a lowercase field name (e.g. `state update status ...` - // against `| status | ... |`). There, `preFm` (extracted from the - // transform's own pre-sync output) never has a `status` key yet — but - // `persistedFm` (extracted after `syncStateFrontmatter` ran) always does, - // since `status` is a schema-owned frontmatter key re-derived on every - // write. Reading frontmatter first made `intended` (body text) and - // `persistedValue` (frontmatter-derived enum) compare two different - // representations of the same field, and the write was never reconciled - // (regression: #1162's "state update is case-insensitive for table field - // names"). + // #3471 review (unchanged by this rewrite): body-FIRST, frontmatter-key- + // FLAT-fallback, dotted-PATH-fallback last. Body-first mirrors the actual + // write precedence `patchCore`/`updateCore` apply (#1162's fix — a + // lowercase body label that happens to case-exact-match a frontmatter key + // must still resolve against the body). `field` a literal flat key (even + // one containing a `.`) is tried before it is split and walked as a + // dotted path (test matrix row 26) — `resolveFrontmatterPath` pins that + // same order for the frontmatter side alone. + // + // `Current Position` is special-cased: it names the WHOLE `## Current + // Position` section, not a single `Label: value` line, so + // `stateExtractField` can never resolve it (this is the root cause of the + // "Current Position undercount" — a transform can correctly push + // `'Current Position'` into its own `updated` list, and this function + // still silently dropped it, because `valueOf` returned `null` for BOTH + // sides and `null === null` failed the old `intended !== null` guard). + // `sliceCurrentPositionSection` is the existing fence-aware section + // locator (state-transition.cts) — reused rather than re-derived. const valueOf = (fm: Record, body: string, field: string): string | null => { + if (field === 'Current Position') { + const section = stateTransitionMod.sliceCurrentPositionSection(body); + return section !== null ? section.trim() : null; + } const bodyValue = stateExtractField(body, field); if (bodyValue !== null) return bodyValue; - return Object.prototype.hasOwnProperty.call(fm, field) ? String(fm[field]) : null; + if (Object.prototype.hasOwnProperty.call(fm, field)) return String(fm[field]); + if (field.includes('.')) { + const resolved = resolveFrontmatterPath(fm, field); + if (resolved !== STATE_FIELD_ABSENT) { + return stateScalarString(resolved); + } + } + return null; }; + // A field in `reported` can itself be a declared derived leaf (e.g. + // `plannedPhaseCore` pushing `'progress.total_plans'` — state- + // transition.cts:1752). `valueOf`'s null-vs-string convention cannot tell + // "absent from the frontmatter" apart from "resolved to the literal string + // 'null'/''", so it cannot carry the same materialization rule + // `computeChangedFrontmatterFields` applies below. Route these fields + // through the SAME primitives (`resolveFrontmatterPath` + the + // `STATE_FIELD_ABSENT` sentinel + `stateFieldValuesDiffer`) instead of a + // second, parallel absence convention — one rule, reused, not duplicated. + const isDeclaredDerivedLeaf = (candidate: string): boolean => + candidate.includes('.') && Object.prototype.hasOwnProperty.call(FIELD_CLASSIFICATION, candidate); + + const changed = (field: string): boolean => { + if (isDeclaredDerivedLeaf(field)) { + const before = resolveFrontmatterPath(snapshotFm, field); + const after = resolveFrontmatterPath(persistedFm, field); + // Same generalized provenance rule as computeChangedFrontmatterFields: + // absent-in-snapshot-materializing-in-persisted is the disk scan + // catching a never-synced document up, not this write's own action. + if (before === STATE_FIELD_ABSENT && after !== STATE_FIELD_ABSENT) return false; + return stateFieldValuesDiffer(before, after); + } + const before = valueOf(snapshotFm, snapshotBody, field); + const after = valueOf(persistedFm, persistedBody, field); + if (before === null && after === null) return false; + if (before === null || after === null) return true; + return before.trim() !== after.trim(); + }; + + // Candidate set = `reported` ∪ every frontmatter key (dotted-leaf + // granularity) whose persisted value differs from the snapshot, minus the + // provenance exclusion. A frontmatter-diff-discovered field is mapped + // through `bodyLabelFor` so it lands in the SAME output vocabulary a + // transform would have used (`status` -> `'Status'`; `progress.total_plans` + // has no body-line label and falls through to its raw dotted key, same as + // today's `progress`/`milestone*` fall-through). + const changedFrontmatterFields = computeChangedFrontmatterFields(snapshotFm, persistedFm, preWriteState.bodyDeltas); + const mappedFrontmatterFields = changedFrontmatterFields.map((field) => bodyLabelFor(field)); + + const seen = new Set(); const reconciled: string[] = []; for (const field of reported) { - const intended = valueOf(preFm, preBody, field); - const persistedValue = valueOf(persistedFm, persistedBody, field); - if (intended !== null && intended.trim() === (persistedValue ?? '').trim()) { + if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(field) || seen.has(field)) continue; + if (changed(field)) { + seen.add(field); reconciled.push(field); } } - // #3471 review: only fold a `divergedFields` entry into the reported array - // when it is a `preserve-when-unchanged` row (has a genuine body-line - // label — Status, Current Plan, Current Phase, ...). #3345's direction - // ("preservation restored a field the intent never named") is about a - // caller-visible BODY field the transform could plausibly have named — - // never about `progress` (`preserve-always`) or `milestone`/`milestone_name` - // (`preserve-if-placeholder`), which are structured/paired fields no - // caller ever names via a per-field body label and whose restoration is - // the long-standing, silent #3242/#948 protection, not a caller-visible - // "update". Folding them in unconditionally reported `progress` as - // `updated` on every `state.patch`/`state.update` write that happened to - // preserve it — even when the call never touched Current Phase's - // curated-progress-preserving field at all (regression: #1264's - // `state.patch` of `Current Phase` reporting `updated: ['Current Phase', - // 'progress']` instead of `['Current Phase']`). - for (const field of divergedFields) { - const cls = stateTransitionMod.getFieldClassification(field); - if (!cls || cls.preservation !== 'preserve-when-unchanged') continue; - const label = bodyLabelFor(field); - if (!reconciled.includes(label)) reconciled.push(label); + for (const field of mappedFrontmatterFields) { + if (STATE_UPDATED_PROVENANCE_EXCLUSION.includes(field) || seen.has(field)) continue; + seen.add(field); + reconciled.push(field); } return reconciled; } @@ -3937,12 +4269,15 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName // still runs after the sync; the override is re-asserted after it inside // readModifyWriteStateMd for layouts with no body `Phase:` line. const divergedFields: string[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; const rmwOptions: ReadModifyWriteOptions = { authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined, divergedFields, + preWriteState, }; let precomputedUpdated: string[] = []; - let preSyncContent = ''; // #3311: begin-phase is the claim point — it is the one Current Position // transition that explicitly names its phase, so it both records this // session's claim and detects a conflicting live claim for a different @@ -3956,7 +4291,6 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName } const result = transitionCore(content, intent, deps); precomputedUpdated = result.updated; - preSyncContent = result.content; // #3127 resume: the core preserved the mid-flight Current Phase Name, so // the intent-first override must not fire — it would drift frontmatter // away from the preserved body value. Dropping it here is safe because @@ -3971,7 +4305,7 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName // the bytes actually persisted (fix(#3351) generalized) and fold in any // field preservation restored that this transform never touched (#3345's // direction). - const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields); + const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields); output( { updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null, milestone_conflict: milestoneConflict }, @@ -4268,15 +4602,18 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa // 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[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; const rmwOptions: ReadModifyWriteOptions = { resync: false, deriveProgressKeys: true, authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined, divergedFields, + preWriteState, }; let precomputedUpdated: string[] = []; - let preSyncContent = ''; readModifyWriteStateMd(statePath, (content) => { if (!intent.phaseName) { const preFm = extractFrontmatter(content, statePath) as Record; @@ -4287,7 +4624,6 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa } const result = transitionCore(content, intent, deps); precomputedUpdated = result.updated; - preSyncContent = result.content; return result.content; }, cwd, rmwOptions); @@ -4297,7 +4633,7 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, phaseNa // (#3345's direction) — traced for this phase (design doc: "not traced in // the analysis pass") and found to need exactly the same treatment as // `cmdStateBeginPhase`. - const updated = reconcileReportedFields(statePath, preSyncContent, precomputedUpdated, divergedFields); + const updated = reconcileReportedFields(statePath, preWriteState, precomputedUpdated, divergedFields); const result = updated.length === 0 ? { updated, phase: phaseNumber, plan_count: planCount, warning: 'STATE.md Current Position has no recognized labels — transition was a no-op. Verify STATE.md uses the canonical labeled format (Status:, Total Plans in Phase:, etc.).' } @@ -5305,8 +5641,10 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // flat `string[]` (the command's OUTPUT CONTRACT — unchanged) happens once // below, right before `output()`. const updated: StateCompletePhaseUpdateEntry[] = []; - let preSyncContent = ''; const divergedFields: string[] = []; + // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the + // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. + const preWriteState: StatePreWriteSnapshot = {}; // #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` @@ -5317,7 +5655,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // "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 rmwOptions: ReadModifyWriteOptions = { divergedFields, preWriteState }; const wrote = readModifyWriteStateMd(statePath, (content) => { const currentPhase = resolvedPhase; @@ -5405,9 +5743,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string } } - const out = reassemble(body); - preSyncContent = out; - return out; + return reassemble(body); }, cwd, rmwOptions); // ADR-3408 §8.4 (D4): traced for this phase (design doc: "not traced in @@ -5425,7 +5761,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // string-matching a name against a Set. const sectionEntries = updated.filter((e) => e.kind === 'section').map((e) => e.name); const fieldEntries = updated.filter((e) => e.kind === 'field').map((e) => e.name); - const reconciled = [...sectionEntries, ...reconcileReportedFields(statePath, preSyncContent, fieldEntries, divergedFields)]; + const reconciled = [...sectionEntries, ...reconcileReportedFields(statePath, preWriteState, fieldEntries, divergedFields)]; output( { updated: reconciled, phase: resolvedPhase }, @@ -5495,6 +5831,17 @@ export = { // FIELD_CLASSIFICATION, exposed so a parity test can pin that every // `preserve-when-unchanged` row has a label here. _FRONTMATTER_KEY_TO_BODY_LABEL: FRONTMATTER_KEY_TO_BODY_LABEL, + // Test seam (ADR-3473 §8.7, #3872): the transaction diff and its pure + // building blocks, exposed so the ~15 boundary/hostile/property rows in + // the test matrix (dotted-path resolution, prototype-pollution safety, + // string/number representation insensitivity, the provenance exclusion) + // can be driven directly with fabricated snapshot/persisted objects + // instead of round-tripping every case through a full RMW write. + _reconcileReportedFields: reconcileReportedFields, + _computeChangedFrontmatterFields: computeChangedFrontmatterFields, + _resolveFrontmatterPath: resolveFrontmatterPath, + _stateFieldValuesDiffer: stateFieldValuesDiffer, + _STATE_UPDATED_PROVENANCE_EXCLUSION: STATE_UPDATED_PROVENANCE_EXCLUSION, // Test seam (audit M1): inject a deterministic isPidAlive so the liveness-gated // steal decision is exercised without real pids. Mirrors capability-lock.cts. _setLockProbes(probes: Partial<{ isPidAlive: (pid: number) => boolean }>): void { diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index 52eaa5980..260e318c9 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -1472,7 +1472,22 @@ describe('#1264: state.patch preserves curated progress frontmatter for non-prog assert.ok(patchResult.success, `state patch failed: ${patchResult.error}`); const output = JSON.parse(patchResult.output); - assert.deepEqual(output.updated, ['Total Plans in Phase']); + // MOVED under ADR-3473 §8.7 (#3872): every `progress.*` leaf here + // genuinely changed on disk (the curated 22/6/12/50 block is + // disk-derived-resynced to match the 8 real plan dirs this fixture + // creates) — this is exactly the #3743/#3818 direction §8.7 closes + // (design doc row 4): a field the OLD `reconcileReportedFields` + // suppressed via its `progress`-is-`preserve-always` classification + // filter, regardless of whether the value actually changed, is now + // reported at DOTTED-LEAF granularity because it actually did. + assert.deepEqual(output.updated.slice().sort(), [ + 'Total Plans in Phase', + 'progress.completed_phases', + 'progress.completed_plans', + 'progress.percent', + 'progress.total_phases', + 'progress.total_plans', + ].sort()); const progress = readPersistedProgress(statePath); assert.strictEqual(progress.total_plans, 8); diff --git a/tests/state-contract.test.cjs b/tests/state-contract.test.cjs index 934851a81..022e6a3cc 100644 --- a/tests/state-contract.test.cjs +++ b/tests/state-contract.test.cjs @@ -328,7 +328,16 @@ describe('state contract — call-site wiring (integration, real CLI)', () => { '| 1. A | 0/1 | Not started | - |', '', ]); - writeState(tmpDir, ['status: planning'], [ + // ADR-3473 §8.7 (#3872) / test-matrix row 17 (D15): `gsd_state_version` + // is pre-seeded here so this fixture isolates the ONE thing it is meant + // to prove (zero recognized Current Position labels is a genuine no-op) + // from an UNRELATED, already-correct §8.7 behavior — a frontmatter block + // synthesizing `gsd_state_version` for the first time on a document that + // never had one IS a real, reportable addition (row 17/D15, pinned + // separately by "state patch accepts JSON object input from workflows" + // above). Omitting it here would make THIS no-op test fail for a reason + // that has nothing to do with recognized labels. + writeState(tmpDir, ['gsd_state_version: 1.0', 'status: planning'], [ '# Project State', '', '(no recognized labels here)', ]); @@ -358,12 +367,19 @@ describe('state contract — call-site wiring (integration, real CLI)', () => { // Additional no-op path found while auditing `state advance-plan` // (cmdStateAdvancePlan, src/state.cts): re-invoking advance-plan while // already parked at "last plan, ready for verification" reproduces - // byte-identical STATE.md content on the SECOND call — `updated` still - // reports `Status`/`Last Activity` (reconcileReportedFields matches - // against the value ALREADY persisted from the first call), but nothing - // is written on the second call, so publishing on `updated.length > 0` - // alone would be wrong here; the fix gates on - // `readModifyWriteStateMd`'s own write-happened return value instead. + // byte-identical STATE.md content on the SECOND call. MOVED under + // ADR-3473 §8.7 (#3872): before that change, `reconcileReportedFields` + // compared the transform's OWN reported fields against the FINAL + // persisted bytes, so `Status`/`Last Activity` (values the transform + // always names, which trivially still matched the byte-identical file) + // came back non-empty even though nothing was written. §8.7 replaced + // that with a diff against the transaction's PRE-WRITE snapshot, so a + // genuinely byte-identical second call now correctly reconciles to + // `updated: []` — a true no-op is reported as a true no-op. Publishing + // still does not fire on this call: `cmdStateAdvancePlan` gates on + // `readModifyWriteStateMd`'s own write-happened return value, never on + // `updated.length`, so this remains a no-op-does-not-publish regression + // test — only the expected shape of the (now-correct) `updated` moved. test('advancePlanNoOpDoesNotPublish', (t) => { const tmpDir = createTempProject(); t.after(() => cleanup(tmpDir)); @@ -385,7 +401,7 @@ describe('state contract — call-site wiring (integration, real CLI)', () => { const second = runGsdTools(['state', 'advance-plan'], tmpDir); assert.ok(second.success, `second (no-op) advance-plan call must genuinely succeed: ${second.error}`); const output = JSON.parse(second.output); - assert.ok(Array.isArray(output.updated) && output.updated.length > 0, 'reconciled updated[] is non-empty even though nothing was written this call'); + assert.deepStrictEqual(output.updated, [], 'ADR-3473 §8.7: a byte-identical second call is a genuine no-op — `updated` must reconcile to empty, not merely reflect fields the transform always names'); assert.strictEqual(fs.existsSync(statePathOf(tmpDir)), false, 'the second, no-op advance-plan call must not publish state.json'); }); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 2ac239736..77000fef0 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -12,8 +12,11 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const os = require('os'); const path = require('path'); -const { runGsdTools, createTempDir, createTempProject, cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs'); const { createFixture, seedWorkstream, writeState } = require('./fixtures/index.cjs'); +// ADR-3473 §8.7 (#3872): git-fixture spawns for the state_head rows (10/11) +// go through the throw-preserving wrapper, never a raw execFileSync. +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); // #3578 AC4 (MCP dispatch parity): drives the same `state complete-phase` // command through the gsd_invoke_command MCP tool route instead of the CLI, // mirroring the gsd-mcp-server.test.cjs `tools/call gsd_invoke_command` @@ -1379,7 +1382,14 @@ describe('cmdStatePatch and cmdStateUpdate (state patch, state update)', () => { assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepEqual(output.updated.sort(), ['Current Phase', 'Status'].sort()); + // MOVED under ADR-3473 §8.7 (#3872): this fixture's STATE.md has NO + // frontmatter block at all before this write (`stateMd` above is a bare + // body). `syncStateFrontmatter` synthesizes one for the first time, + // adding `gsd_state_version` — a key going from ABSENT in the pre-write + // snapshot to PRESENT in the persisted document, which design doc row 15 + // ("a field absent from snapshot, present in persisted -> reported") says + // is a change like any other. + assert.deepEqual(output.updated.slice().sort(), ['Current Phase', 'Status', 'gsd_state_version'].sort()); const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); assert.ok(updated.includes('**Status:** Complete'), 'Status should be updated to Complete'); @@ -6719,7 +6729,21 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid beforeEach(() => { tmpDir = createFixture(); }); afterEach(() => { cleanup(tmpDir); }); - test('E1: cmdStateUpdate — `updated` reflects the persisted change, and `preserved` names a field the update never touched but preservation restored', () => { + // MOVED under ADR-3473 §8.7 (#3872): `current_phase_name` genuinely round- + // trips here — `syncStateFrontmatter` re-derives it from the (unchanged) + // body `Phase:` line, finds nothing, and `applyPreserveWhenUnchanged` + // restores the SAME curated snapshot value ("Curated Name") that was + // already on disk before this write. §8.7's rule is literal: a field + // appears in `updated`/`preserved` IFF its PERSISTED value differs from + // the pre-write SNAPSHOT — verified at the CLI, `current_phase_name` is + // `"Curated Name"` both before and after. The OLD assertion pinned the + // PRIOR (`ADR-3408 §8.4`) mechanism this phase replaces: it folded in any + // field `divergedFields` saw preservation touch MID-PIPELINE, regardless + // of whether the net effect was a real change — exactly the shape #1264 + // already forbids for `progress` (row 12 of `40-design.md`'s behavior + // table: "an identical restore is not a change"). This test is the same + // rule for a `preserve-when-unchanged` field instead of `preserve-always`. + test('E1: cmdStateUpdate — `updated` reflects the persisted change, and a field restored to its ORIGINAL value is not reported', () => { const content = [ '---', 'gsd_state_version: 1.0', 'current_phase_name: Curated Name', '---', '', '# Project State', '', '## Current Position', '', 'Status: Executing', '', @@ -6730,9 +6754,30 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); assert.strictEqual(output.updated, true); - assert.deepStrictEqual(output.preserved, ['Current Phase Name'], '#3345\'s direction: a field the update never named that preservation restored'); + assert.deepStrictEqual(output.preserved, [], '`current_phase_name` round-trips to its ORIGINAL curated value ("Curated Name" before and after) — not a change under the §8.7 diff'); }); + // MOVED under ADR-3473 §8.7 (#3872): `Current Position` — advancePlanCore + // rewrites text INSIDE the `## Current Position` section (the Current + // Plan line), so the section genuinely changed on disk. It was silently + // dropped before this phase (the same "Current Position undercount" + // class as row 27/#3818, generalized here beyond `plannedPhaseCore` — + // `reconcileReportedFields`'s `valueOf` could not resolve the + // WHOLE-SECTION field name against a single `Label: value` line, so + // `intended !== null` never held). + // + // This fixture's frontmatter has no `progress` block at all, and + // `syncStateFrontmatter`'s disk-derived resync (an empty `.planning/ + // phases/` from `createFixture`) materializes one — every leaf + // (`total_plans` included: `advancePlanCore` never pushes it to its own + // `reported` list, so it has no caller-attributable source here at all) + // is the generalized provenance rule's case (2): a declared derived leaf + // (`source: 'disk'`, state-transition.cts:136-140) appearing from a + // source that did not change during this write is the scanner catching a + // never-synced document up, not the caller's action — the SAME principle + // `STATE_UPDATED_PROVENANCE_EXCLUSION` applies to `last_updated`, + // generalized rather than special-cased per leaf. None of the four + // `progress.*` leaves are reported here. test('E2: cmdStateAdvancePlan — `updated` names only the fields whose persisted value actually changed (happy path)', () => { const content = [ '---', 'gsd_state_version: 1.0', '---', '', @@ -6743,37 +6788,56 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const result = runGsdTools(['state', 'advance-plan'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepStrictEqual(output.updated.slice().sort(), ['Current Plan', 'Status']); + assert.deepStrictEqual(output.updated.slice().sort(), ['Current Plan', 'Current Position', 'Status']); }); - // E6 (#3345's direction — the direction nothing has ever tested): a field - // preservation restored that advancePlanCore's OWN transform never - // touched at all IS in `updated`. Also demonstrates the report's item 4: - // cmdStateAdvancePlan previously exposed NO `updated` array whatsoever. - test('E6: cmdStateAdvancePlan — a field preservation restored, that the transform never touched, IS in `updated`', () => { + // MOVED under ADR-3473 §8.7 (#3872). advance-plan NEVER touches the body + // `Phase:` line (the comment below), so for THIS command + // `current_phase`/`current_phase_name`'s body-source delta is + // UNCONDITIONALLY unchanged — preservation restores the curated snapshot + // back to itself, byte for byte ("99" / "Curated Stale Name" both + // before and after, confirmed by the `fm.*` assertions below, which are + // unchanged). Under §8.7's literal rule ("a field appears iff its + // PERSISTED value differs from the snapshot") that is NOT a reportable + // change — the same "identical restore" rule #1264 already established + // for `progress` (design doc row 12), now applied to a + // `preserve-when-unchanged` field. This test's ORIGINAL premise (#3345's + // "genuinely restored, transform never touched" direction) cannot be + // demonstrated via `advance-plan` at all, precisely BECAUSE this command + // never perturbs the Phase-line delta — that direction is what row 8's + // `reportsCurrentPhaseWhenTheWriteAdvancedIt` test (via + // `state planned-phase`, which DOES rewrite the Phase line) now covers. + test('E6: cmdStateAdvancePlan — a curated field restored to its ORIGINAL value is not reported (that direction is covered by row 8 instead)', () => { const content = [ '---', 'gsd_state_version: 1.0', 'current_phase: "99"', 'current_phase_name: Curated Stale Name', '---', '', '# Project State', '', '## Current Position', '', 'Phase: 1 (Old Name)', 'Current Plan: 3', 'Total Plans in Phase: 3', 'Status: Executing', '', ].join('\n'); fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), content); - // Last plan in the phase — advance-plan reports readiness without - // touching the body Phase line, so its delta is unchanged and the - // curated current_phase/current_phase_name must be restored and - // reported, even though advancePlanCore's own intent never named them - // (confirmed empirically: the SAME fixture minus the curated conflict - // reports `updated: ["Status"]` only). const result = runGsdTools(['state', 'advance-plan'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.ok(output.updated.includes('Current Phase'), `expected 'Current Phase' in updated: ${JSON.stringify(output.updated)}`); - assert.ok(output.updated.includes('Current Phase Name'), `expected 'Current Phase Name' in updated: ${JSON.stringify(output.updated)}`); + assert.ok(!output.updated.includes('Current Phase'), `'Current Phase' round-trips to its original value and must not be reported: ${JSON.stringify(output.updated)}`); + assert.ok(!output.updated.includes('Current Phase Name'), `'Current Phase Name' round-trips to its original value and must not be reported: ${JSON.stringify(output.updated)}`); + // Generalized provenance rule (same as E2 above): this fixture also + // starts with no `progress` block, `advancePlanCore` never pushes + // `progress.total_plans` to its own `reported` list, and the disk + // scan materializing a fresh (zero-valued) `progress` block is the + // scanner catching up, not this write's action — none of the four + // leaves are reported. + assert.deepStrictEqual(output.updated.slice().sort(), ['Current Position', 'Status']); const fm = frontmatterLib.extractFrontmatter(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8')); - assert.strictEqual(fm.current_phase, '99', 'the reported field must match what was actually persisted'); + assert.strictEqual(fm.current_phase, '99', 'the curated value must still survive the write even though it is not reported as a change'); assert.strictEqual(fm.current_phase_name, 'Curated Stale Name'); }); + // MOVED under ADR-3473 §8.7 (#3872): `Status` lives INSIDE `## Current + // Position` in this fixture, and `beginPhaseCore` mutates that section + // (writes the Phase/Status/Last-activity lines) — so the section text + // genuinely changed on disk. Same generalized "Current Position + // undercount" fix as E2/row 27: previously silently dropped by + // `valueOf`'s inability to resolve a whole-section field name. test('E3: cmdStateBeginPhase — `updated` names only the fields whose persisted value actually changed (happy path)', () => { const content = [ '---', 'gsd_state_version: 1.0', '---', '', @@ -6784,10 +6848,16 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const result = runGsdTools(['state', 'begin-phase', '--phase', '2', '--name', 'Build', '--plans', '4'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepStrictEqual(output.updated, ['Status']); + assert.deepStrictEqual(output.updated.slice().sort(), ['Current Position', 'Status']); }); - test('E3 (preservation): cmdStateBeginPhase — a curated field the transition never touches IS in `updated`', () => { + // MOVED under ADR-3473 §8.7 (#3872): `stopped_at` round-trips to its + // ORIGINAL curated value ("curated stop must survive" before and after — + // begin-phase never touches the `## Session` `Stopped at:` line) so it is + // correctly EXCLUDED under the literal "persisted differs from snapshot" + // rule (the same class as E1/E6). `Current Position` is added for the + // same reason as E3 above. + test('E3 (preservation): cmdStateBeginPhase — a curated field round-tripped to its ORIGINAL value is not reported', () => { const content = [ '---', 'gsd_state_version: 1.0', 'stopped_at: "curated stop must survive"', '---', '', '# Project State', '', '## Current Position', '', 'Status: Not started', '', '## Session', '', @@ -6797,7 +6867,10 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const result = runGsdTools(['state', 'begin-phase', '--phase', '2', '--name', 'Build', '--plans', '4'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepStrictEqual(output.updated.slice().sort(), ['Status', 'Stopped At']); + assert.deepStrictEqual(output.updated.slice().sort(), ['Current Position', 'Status']); + + const fm = frontmatterLib.extractFrontmatter(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8')); + assert.strictEqual(fm.stopped_at, 'curated stop must survive', 'the curated value must still survive the write even though it is not reported as a change'); }); test('E4: cmdStateRecordSession — `updated` names only the fields whose persisted value actually changed (happy path)', () => { @@ -6813,7 +6886,12 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid assert.deepStrictEqual(output.updated.slice().sort(), ['Last session', 'Stopped At']); }); - test('E4 (preservation): cmdStateRecordSession — a curated field the transform never touches IS in `updated`', () => { + // MOVED under ADR-3473 §8.7 (#3872): `current_phase_name` round-trips to + // its ORIGINAL curated value ("Curated Name" before and after — + // record-session never touches the body `Phase:` line) so it is + // correctly EXCLUDED under the literal "persisted differs from snapshot" + // rule — the same class as E1/E6. + test('E4 (preservation): cmdStateRecordSession — a curated field round-tripped to its ORIGINAL value is not reported', () => { const content = [ '---', 'gsd_state_version: 1.0', 'current_phase_name: Curated Name', '---', '', '# Project State', '', '## Session', '', '**Last session:** old', '', @@ -6823,12 +6901,24 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const result = runGsdTools(['state', 'record-session', '--stopped-at', 'Phase 1 complete'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepStrictEqual(output.updated.slice().sort(), ['Current Phase Name', 'Last session', 'Stopped At']); + assert.deepStrictEqual(output.updated.slice().sort(), ['Last session', 'Stopped At']); + + const fm = frontmatterLib.extractFrontmatter(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8')); + assert.strictEqual(fm.current_phase_name, 'Curated Name', 'the curated value must still survive the write even though it is not reported as a change'); }); // E8 — untraced in the design's own analysis pass: cmdStatePlannedPhase // and cmdStateCompletePhase (the DIFFERENT legacy hand-rolled one) are // traced here rather than assumed, per the design's own instruction. + // + // MOVED under ADR-3473 §8.7 (#3872): this fixture's `## Current Position` + // has no recognized labels for `plannedPhaseCore` (no `Phase:`/`Total + // Plans in Phase:` line), so the transition is a documented no-op + // (`plannedPhaseCore`'s own `updated: []` plus a "no recognized labels" + // warning — confirmed at the CLI). `stopped_at` round-trips to its + // ORIGINAL curated value the same way as E3(preservation)/E4(preservation) + // above, so `updated` is correctly empty — nothing on disk actually + // differs from the pre-write snapshot. test('E8a: cmdStatePlannedPhase reconciles the same way as cmdStateBeginPhase (traced, not assumed)', () => { const content = [ '---', 'gsd_state_version: 1.0', 'stopped_at: "curated stop must survive"', '---', '', @@ -6839,7 +6929,10 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid const result = runGsdTools(['state', 'planned-phase', '--phase', '2', '--name', 'Build', '--plans', '4'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); - assert.deepStrictEqual(output.updated, ['Stopped At']); + assert.deepStrictEqual(output.updated, []); + + const fm = frontmatterLib.extractFrontmatter(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8')); + assert.strictEqual(fm.stopped_at, 'curated stop must survive', 'the curated value must still survive the write even though it is not reported as a change'); }); // E8b — cmdStateCompletePhase (the legacy hand-rolled path, NOT the @@ -6847,6 +6940,14 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid // FIELD names with the SECTION name 'Current Position'. Reconciliation // must apply only to the field-shaped entries and pass 'Current Position' // through unconditionally, never dropping it as a false negative. + // + // MOVED under ADR-3473 §8.7 (#3872): `paused_at` round-trips to its + // ORIGINAL curated value ("curated pause must survive" before and + // after — this fixture has no `## Session` section at all, so + // complete-phase's write never perturbs its body source) — the same + // "identical restore is not a change" class as E1/E3(preservation)/ + // E4(preservation)/E6/E8a. `fm.paused_at` below still proves the VALUE + // survives the write; it is simply no longer reported as an "update". test('E8b: cmdStateCompletePhase (legacy) reconciles field entries and passes the "Current Position" section entry through unconditionally', () => { const content = [ '---', 'gsd_state_version: 1.0', 'paused_at: "curated pause must survive"', 'current_phase: 1', '---', '', @@ -6858,7 +6959,7 @@ describe('ADR-3408 §8.5 Matrix (#3471): stale-but-present, and the report resid assert.ok(result.success, `Command failed: ${result.error}`); const output = JSON.parse(result.output); assert.ok(output.updated.includes('Current Position'), 'the whole-section entry must not be dropped as a false negative by field-shaped reconciliation'); - assert.ok(output.updated.includes('Paused At'), '#3345\'s direction must also apply to this legacy path'); + assert.ok(!output.updated.includes('Paused At'), '"curated pause must survive" round-trips to its ORIGINAL value and must not be reported'); const fm = frontmatterLib.extractFrontmatter(fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8')); assert.strictEqual(fm.paused_at, 'curated pause must survive'); @@ -17122,6 +17223,541 @@ describe('#3871 / #3756: curated progress must survive a write on an archived mi }); }); +// ═════════════════════════════════════════════════════════════════════════ +// #3872 / ADR-3473 §8.7: what a command reports it wrote +// (.gsd/phase/feat-3872-transaction-diff-reporting/{40-design,50-test-matrix}.md) +// +// `reconcileReportedFields` (src/state.cts:3726-3794) decides what a +// `state.*` command reports in its `updated` array. Rows below pin the +// currently-red rows (5, 4, 27) and the two guard rows that must stay green +// through any fix (12, 13). +// ═════════════════════════════════════════════════════════════════════════ + +describe('#3872 / ADR-3473 §8.7: what a command reports it wrote', () => { + function buildPlannedPhaseFixture(cwd, { totalPlans, completedPlans, percent, planFiles }) { + const planningDir = path.join(cwd, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + ['## v1.0 Current', '', '### Phase 1: Foo', ''].join('\n'), + ); + const phaseDir = path.join(planningDir, 'phases', '01-foo'); + fs.mkdirSync(phaseDir, { recursive: true }); + for (let p = 1; p <= planFiles.total; p += 1) { + const n = String(p).padStart(2, '0'); + fs.writeFileSync(path.join(phaseDir, `01-${n}-PLAN.md`), '# Plan\n'); + if (p <= planFiles.completed) { + fs.writeFileSync(path.join(phaseDir, `01-${n}-SUMMARY.md`), '# Summary\n'); + } + } + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + 'progress:', + ` total_phases: 1`, + ` completed_phases: 0`, + ` total_plans: ${totalPlans}`, + ` completed_plans: ${completedPlans}`, + ` percent: ${percent}`, + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Status: Executing', + 'Phase: 1', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + } + + describe('row 5 (PROVEN RED, CLI): dotted leaf `progress.total_plans` is silently dropped from `updated`', () => { + test('reportsDottedLeafWhenPlanCountChanges', (t) => { + const cwd = createTempDir('gsd-3872-row5-'); + t.after(() => cleanup(cwd)); + // 5 real PLAN.md files on disk so the disk-scan-derived progress this + // write's own ratchet-merge reads from (buildStateFrontmatter) actually + // measures 5 plans — the curated block starts at 0. + buildPlannedPhaseFixture(cwd, { totalPlans: 0, completedPlans: 0, percent: 0, planFiles: { total: 5, completed: 0 } }); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const before = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(Number(before.progress.total_plans), 0, 'setup: curated total_plans starts at 0'); + + const result = runGsdTools(['state', 'planned-phase', '--phase', '1', '--name', 'Foo', '--plans', '5'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + const after = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + // The persisted value really moved — this can never pass vacuously by + // the value not changing on disk. + assert.strictEqual(Number(after.progress.total_plans), 5, 'progress.total_plans must actually change 0 -> 5 on disk for this test to mean anything'); + + // TODAY: `updated` is `["Status"]` — `progress.total_plans` is absent + // because `reconcileReportedFields`'s `valueOf` cannot resolve a dotted + // key against the nested `fm.progress.total_plans` (src/state.cts:3758-3762). + assert.ok( + output.updated.includes('progress.total_plans'), + `updated must contain "progress.total_plans" (the dotted leaf plannedPhaseCore itself pushed to its own success list — src/state-transition.cts:1752); ` + + `got ${JSON.stringify(output.updated)}`, + ); + }); + }); + + describe('row 27 (regression, same run as row 5): `Current Position` changed on disk but is absent from `updated`', () => { + test('reportsCurrentPositionWhenItActuallyMoved', (t) => { + const cwd = createTempDir('gsd-3872-row27-'); + t.after(() => cleanup(cwd)); + buildPlannedPhaseFixture(cwd, { totalPlans: 0, completedPlans: 0, percent: 0, planFiles: { total: 5, completed: 0 } }); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const before = fs.readFileSync(statePath, 'utf-8'); + assert.ok(before.includes('Phase: 1\n'), 'setup: Current Position starts with the bare pre-plan Phase line'); + + const result = runGsdTools(['state', 'planned-phase', '--phase', '1', '--name', 'Foo', '--plans', '5'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + const after = fs.readFileSync(statePath, 'utf-8'); + // The Current Position section really moved on disk (src/state-transition.cts:1741 + // pushes 'Current Position' into plannedPhaseCore's own `updated` precisely + // when `body !== beforePos`; the persisted Phase line proves the byte-level + // change independent of that push). + assert.ok( + after.includes('Phase: 1 (Foo) — READY TO EXECUTE'), + 'Current Position must actually change on disk for this test to mean anything', + ); + + // TODAY: `updated` is `["Status"]` — `Current Position` is absent even + // though the section changed. + assert.ok( + output.updated.includes('Current Position'), + `updated must contain "Current Position"; got ${JSON.stringify(output.updated)}`, + ); + }); + }); + + describe('row 4 (regression #3743/#3818): `progress` genuinely changed by this write is suppressed by the classification filter', () => { + test('reportsProgressWhenItGenuinelyChanged', (t) => { + const cwd = createTempDir('gsd-3872-row4-'); + t.after(() => cleanup(cwd)); + // 3 plans on disk, all complete — curated block under-reports + // completed_plans (2 of 3) and a stale 66% (isolates this from row 5's + // dotted-key bug: --plans is omitted, so plannedPhaseCore never pushes + // 'progress.total_plans' to its own reported list at all). + buildPlannedPhaseFixture(cwd, { totalPlans: 3, completedPlans: 2, percent: 66, planFiles: { total: 3, completed: 3 } }); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + const before = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(Number(before.progress.percent), 66, 'setup: curated percent starts at 66'); + + const result = runGsdTools(['state', 'planned-phase', '--phase', '1', '--name', 'Foo'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.plan_count, null, 'setup: no --plans was passed, so the dotted-leaf mechanism (row 5) never engages'); + + const after = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.notStrictEqual( + Number(after.progress.percent), 66, + 'progress.percent must actually change on disk (via the ratchet-merge in applyPreserveAlways) for this test to mean anything', + ); + + // TODAY: `updated` is `["Status"]` — `progress` (or a leaf naming the + // changed value) is absent, filtered by reconcileReportedFields's + // divergedFields loop (src/state.cts:3787-3792), which only folds in a + // divergedFields entry when its classification is + // 'preserve-when-unchanged' — `progress` is classified 'preserve-always' + // (src/state-transition.cts:135) and so is unconditionally excluded + // regardless of whether it merely preserved an unchanged value (#1264, + // correct) or genuinely changed (#3743/#3818, this row). + assert.ok( + output.updated.some((f) => f === 'progress' || f.startsWith('progress.')), + `updated must contain a field naming the changed progress leaf; got ${JSON.stringify(output.updated)}`, + ); + }); + }); + + describe('row 8 (#3818, CLI-proven): `current_phase` advanced by the write is unreported — not restored, so never in `divergedFields`', () => { + // #3818's own report: a real `state planned-phase` run reported + // `updated: ["progress.total_plans"]` while `current_phase` moved 203 -> 204 + // on disk, unreported. `plannedPhaseCore` unconditionally rewrites the + // `## Current Position` `Phase:` line (system-derived, not template-gated), + // so `applyPostSyncPreservation`'s #1230 body-source delta for `current_phase` + // (src/state.cts:3369-3370, 3396-3397) sees pre != post THIS write and lets + // the freshly re-derived value win — `applyPreserveWhenUnchanged` only ever + // restores when the delta says unchanged, so a genuinely-advanced + // `current_phase` is never a restoration and never enters `divergedFields` + // (src/state.cts:3466-3473 diffs postFm before/after `applyStatePreservation` + // ran — a field the sync alone changed, that preservation never touched, + // produces no diff there). `plannedPhaseCore` also never pushes + // `current_phase` (or a `Current Phase` label) onto its own `updated` list + // (src/state-transition.cts:1655-1754) — it names `Status`, `Total Plans in + // Phase`, `Last Activity`, `Last Activity Description`, `Current Position`, + // `progress.total_plans`. So the field is in NEITHER candidate set. + test('reportsCurrentPhaseWhenTheWriteAdvancedIt', (t) => { + const cwd = createTempDir('gsd-3872-row8-'); + t.after(() => cleanup(cwd)); + const planningDir = path.join(cwd, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + ['## v1.0 Current', '', '### Phase 3: Foo', '### Phase 4: Bar', ''].join('\n'), + ); + fs.mkdirSync(path.join(planningDir, 'phases', '03-foo'), { recursive: true }); + const phase4Dir = path.join(planningDir, 'phases', '04-bar'); + fs.mkdirSync(phase4Dir, { recursive: true }); + for (let p = 1; p <= 2; p += 1) { + fs.writeFileSync(path.join(phase4Dir, `04-${String(p).padStart(2, '0')}-PLAN.md`), '# Plan\n'); + } + const statePath = path.join(planningDir, 'STATE.md'); + fs.writeFileSync( + statePath, + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + 'current_phase: "3"', + 'progress:', + ' total_phases: 2', + ' completed_phases: 0', + ' total_plans: 0', + ' completed_plans: 0', + ' percent: 0', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Status: Executing', + 'Phase: 3 (Foo) — EXECUTING', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + + const before = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(String(before.current_phase), '3', 'setup: current_phase starts at 3'); + + // Verified at the CLI (built lib): + // BEFORE current_phase: 3 + // CLI output: {"updated":["Status"],"phase":"4","plan_count":2} + // AFTER current_phase: 4 + const result = runGsdTools(['state', 'planned-phase', '--phase', '4', '--name', 'Bar', '--plans', '2'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + const after = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual( + String(after.current_phase), '4', + 'current_phase must actually advance 3 -> 4 on disk for this test to mean anything', + ); + + assert.ok( + output.updated.includes('current_phase') || output.updated.includes('Current Phase'), + `updated must name current_phase's advance; got ${JSON.stringify(output.updated)}`, + ); + }); + }); + + describe('row 13 (verdict guard, must PASS today and after): a fully-failed `state.patch` reports an empty `updated`', () => { + test('aFullyFailedPatchStillReportsFailure', (t) => { + const cwd = createTempProject('gsd-3872-row13-'); + t.after(() => cleanup(cwd)); + fs.writeFileSync( + path.join(cwd, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'status: planning', + '---', + '', + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + + // "Totally Fake Field" passes security.cts's validateFieldName format + // check but matches no body label and no frontmatter key, so + // patchCore (src/state-transition.cts) routes it straight to `failed` + // (never `updated`) — the every-field-fails case src/state.cts:607's + // `updated.length > 0` success boolean guards against. + const result = runGsdTools(['query', 'state.patch', JSON.stringify({ 'Totally Fake Field': 'x' })], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + assert.deepStrictEqual(output.updated, [], 'updated must be empty when every requested field failed'); + assert.ok(output.failed.includes('Totally Fake Field'), 'the failed field must be named in failed'); + }); + }); + + describe('row 12 (independence guard, must PASS today and after): two content-identical writes differ only in `last_updated`', () => { + // Corrected by this row's own CLI result: `state_head` is NOT ambient. + // It is recomputed on every write, but its VALUE changes only when git + // HEAD actually moved between the two writes — verified below at the CLI + // (same git-backed fixture, HEAD held fixed): `state_head` was IDENTICAL + // across both writes, so the provenance-exclusion set shrinks to the + // single element `last_updated`. The companion test right after this one + // pins the other half: `state_head` DOES change once HEAD moves. + test('identicalWritesDifferOnlyInLastUpdated', (t) => { + const { createTempGitProject: mkGit } = require('./helpers.cjs'); + const cwd = mkGit('gsd-3872-row12-'); + t.after(() => cleanup(cwd)); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + fs.writeFileSync( + statePath, + [ + '---', + 'gsd_state_version: 1.0', + 'status: planning', + '---', + '', + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + + // Two content-identical `record-session` calls against the SAME + // git-backed fixture, with HEAD held fixed between them — `state_head` + // is only ever set in a git-backed tree (row 11's null-guard), so a + // non-git fixture cannot exercise it. + // + // Verified at the CLI (built lib), HEAD never moved between the calls: + // r1 state_head: 672631f4bf5c7547b86239e83fce37b2473e83af + // r2 state_head: 672631f4bf5c7547b86239e83fce37b2473e83af (identical) + const result1 = runGsdTools(['state', 'record-session', '--stopped-at', 'Same stop text'], cwd); + assert.ok(result1.success, `first write failed: ${result1.error}`); + const fm1 = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + + const result2 = runGsdTools(['state', 'record-session', '--stopped-at', 'Same stop text'], cwd); + assert.ok(result2.success, `second write failed: ${result2.error}`); + const fm2 = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + + assert.strictEqual( + fm1.state_head, fm2.state_head, + 'state_head must be identical across two writes with no intervening commit (it tracks a real fact, not an ambient stamp)', + ); + + const AMBIENT = new Set(['last_updated']); + const keys = new Set([...Object.keys(fm1), ...Object.keys(fm2)]); + const nonAmbientDiffs = []; + for (const key of keys) { + if (AMBIENT.has(key)) continue; + if (JSON.stringify(fm1[key]) !== JSON.stringify(fm2[key])) nonAmbientDiffs.push(key); + } + assert.deepStrictEqual( + nonAmbientDiffs, [], + `two content-identical writes must differ only in last_updated; ` + + `also differed in ${JSON.stringify(nonAmbientDiffs)} — fm1=${JSON.stringify(fm1)} fm2=${JSON.stringify(fm2)}`, + ); + }); + }); + + describe('row 12 companion (must PASS today and after): `state_head` changes when HEAD actually moves between writes', () => { + // The other half of the row-12 correction: `state_head` is excluded from + // provenance not because it is ambient, but because it is a faithful, + // reportable projection of a real fact (git HEAD). This pins that it is + // NOT a constant that a future reader could mistake for ambient. + test('stateHeadChangesWhenHeadMoves', (t) => { + const { createTempGitProject: mkGit } = require('./helpers.cjs'); + const { gitOrThrow, GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/git-fixture.cjs'); + const cwd = mkGit('gsd-3872-row12-companion-'); + t.after(() => cleanup(cwd)); + + const statePath = path.join(cwd, '.planning', 'STATE.md'); + fs.writeFileSync( + statePath, + [ + '---', + 'gsd_state_version: 1.0', + 'status: planning', + '---', + '', + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '**Resume file:** None', + '', + ].join('\n'), + ); + + const result1 = runGsdTools(['state', 'record-session', '--stopped-at', 'First stop'], cwd); + assert.ok(result1.success, `first write failed: ${result1.error}`); + const fm1 = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + + // Advance HEAD between the two writes. + const gitOpts = { cwd, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }; + fs.writeFileSync(path.join(cwd, 'NOTE.txt'), 'advance head\n'); + gitOrThrow(['add', '-A'], gitOpts); + gitOrThrow(['commit', '-m', 'advance head'], gitOpts); + // Bounded via `gitOrThrow` (tests/helpers/git-fixture.cjs), not a raw + // `execFileSync` — every other git call in this fixture already goes + // through the timeout-bounded seam; an unbounded spawn here is the + // one call `local/no-unbounded-spawn` correctly flagged. + const newHead = gitOrThrow(['rev-parse', 'HEAD'], gitOpts).trim(); + + // Verified at the CLI (built lib): + // r1 state_head: 672631f4bf5c7547b86239e83fce37b2473e83af + // newHead: ed5927d76a691323ef61d0a19573bb9f75dcd852 + // r2 state_head: ed5927d76a691323ef61d0a19573bb9f75dcd852 (== newHead) + const result2 = runGsdTools(['state', 'record-session', '--stopped-at', 'Second stop'], cwd); + assert.ok(result2.success, `second write failed: ${result2.error}`); + const fm2 = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + + assert.notStrictEqual(fm1.state_head, fm2.state_head, 'state_head must change once HEAD actually moved'); + assert.strictEqual(fm2.state_head, newHead, 'state_head must track the real, current HEAD sha'); + }); + }); + + // Generalized provenance rule (coordinator directive, folded into this + // phase alongside rows 4/5/27 above): a DECLARED derived leaf + // (`declaredLeavesOf`, e.g. every `progress.*` row — `source: 'disk'`, + // state-transition.cts:136-140) materializing from ABSENT in the pre-write + // snapshot to PRESENT in persisted is the disk scan catching a + // never-synced document up, not the caller's action — the same + // provenance principle `STATE_UPDATED_PROVENANCE_EXCLUSION` already + // applies to `last_updated` one field up, generalized rather than turned + // into a second `progress`-specific classification exclusion (that would + // be exactly what §8.7 bans). A leaf already PRESENT in the snapshot gets + // no such pass — a genuine move is still reported (the sibling test + // below, and rows 4/5 above). + describe('generalized provenance rule: a derived leaf materializing from nothing is not a change, but one that genuinely moves still is', () => { + test('derivedLeafMaterializationIsNotAChange', (t) => { + const cwd = createTempDir('gsd-3872-materialize-'); + t.after(() => cleanup(cwd)); + const planningDir = path.join(cwd, '.planning'); + // Empty phases/ dir (present, zero subdirectories) + a STATE.md with NO + // `progress:` frontmatter block at all — the exact shape every one of + // the 11 failing fixtures reproduced. + fs.mkdirSync(path.join(planningDir, 'phases'), { recursive: true }); + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + '---', + '', + '# Project State', + '', + '## Session', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '', + ].join('\n'), + ); + const statePath = path.join(planningDir, 'STATE.md'); + const before = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(before.progress, undefined, 'setup: no progress block before this write'); + + const result = runGsdTools(['state', 'record-session', '--stopped-at', 'Now stopped'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + // The block really did land on disk — this can never pass vacuously by + // nothing being written; buildStateFrontmatter's disk scan of the empty + // phases/ dir materializes a real (zero-valued) progress block. + const after = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.ok(after.progress && typeof after.progress === 'object', 'progress must actually materialize on disk for this test to mean anything'); + + assert.ok( + !output.updated.some((f) => f === 'progress' || f.startsWith('progress.')), + `materialization from an absent snapshot must not be reported; got ${JSON.stringify(output.updated)}`, + ); + }); + + test('changedDerivedLeafIsStillAChange', (t) => { + const cwd = createTempDir('gsd-3872-genuine-change-'); + t.after(() => cleanup(cwd)); + const planningDir = path.join(cwd, '.planning'); + const phaseDir = path.join(planningDir, 'phases', '01-foo'); + fs.mkdirSync(phaseDir, { recursive: true }); + // 3 real PLAN.md files on disk — the curated block below UNDER-reports + // total_plans (1), so this write's disk scan disagrees with it and + // (source: 'disk', preserve-always/progress-ratchet) the fresh value wins. + for (let p = 1; p <= 3; p += 1) { + fs.writeFileSync(path.join(phaseDir, `01-${String(p).padStart(2, '0')}-PLAN.md`), '# Plan\n'); + } + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + ['## v1.0 Current', '', '### Phase 1: Foo', ''].join('\n'), + ); + const statePath = path.join(planningDir, 'STATE.md'); + fs.writeFileSync( + statePath, + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'status: executing', + 'progress:', + ' total_phases: 1', + ' completed_phases: 0', + ' total_plans: 1', + ' completed_plans: 0', + '---', + '', + '# Project State', + '', + '## Session', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** None', + '', + ].join('\n'), + ); + const before = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(Number(before.progress.total_plans), 1, 'setup: curated total_plans starts at 1, present in the snapshot'); + + const result = runGsdTools(['state', 'record-session', '--stopped-at', 'Now stopped'], cwd); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + const after = frontmatterLib.extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); + assert.strictEqual(Number(after.progress.total_plans), 3, 'progress.total_plans must actually move 1 -> 3 on disk for this test to mean anything'); + + assert.ok( + output.updated.includes('progress.total_plans'), + `a leaf already present in the snapshot that genuinely moved must still be reported; got ${JSON.stringify(output.updated)}`, + ); + }); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // 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 @@ -17515,3 +18151,520 @@ describe('#3834/#3835/#3836: current_phase_name / last_activity_desc preservatio ); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// ADR-3473 §8.7 (#3872): the transaction diff — remaining test-matrix rows +// (`.gsd/phase/feat-3872-transaction-diff-reporting/50-test-matrix.md`). Rows +// 3, 4, 5, 8, 12, 13, 20, 27 and the row-12 companion are covered elsewhere +// (tests/frontmatter.test.cjs's #1264 pin, and existing state.test.cjs A2f/ +// #3743/#3818 assertions) and are deliberately NOT duplicated here. +// ───────────────────────────────────────────────────────────────────────────── +describe('ADR-3473 §8.7 (#3872): reconcileReportedFields / the transaction diff', () => { + let tmpDir; + let statePath; + + beforeEach(() => { + tmpDir = createTempDir('gsd-3872-diff-'); + statePath = path.join(tmpDir, 'STATE.md'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function writePersisted(frontmatterLines, bodyLines) { + fs.writeFileSync( + statePath, + ['---', ...frontmatterLines, '---', '', ...bodyLines].join('\n'), + ); + } + + // Row 1 + test('reportsAFieldThatActuallyPersisted', () => { + writePersisted( + ['current_phase: "05"'], + ['# State', '', '**Current Phase:** 05', ''], + ); + const snapshotBody = ['# State', '', '**Current Phase:** 04', ''].join('\n'); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { current_phase: '04' }, body: snapshotBody }, + ['Current Phase'], + [], + ); + assert.deepEqual(updated, ['Current Phase'], 'a field the transform reported that genuinely persisted must be reported'); + }); + + // Row 2 — #3351 stays closed + test('doesNotReportAFieldTheWriteDiscarded', () => { + const body = ['# State', '', '**Current Phase:** 05', ''].join('\n'); + writePersisted(['current_phase: "05"'], ['# State', '', '**Current Phase:** 05', '']); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { current_phase: '05' }, body }, + ['Current Phase'], + [], + ); + assert.deepEqual(updated, [], '#3351: a field the transform claimed to write but sync/preservation discarded before the save must not be reported'); + }); + + // Rows 6, 7, 9 — pure diff, no I/O: computeChangedFrontmatterFields + test('doesNotReportAnUnchangedDottedLeaf / reportsOnlyTheLeafThatMoved / neverReportsAnUnconditionallyStampedField', () => { + const snapshotFm = { + progress: { total_plans: '0', completed_plans: '2' }, + last_updated: '2026-08-24T00:00:00.000Z', + }; + const persistedFm = { + progress: { total_plans: '5', completed_plans: '2' }, + last_updated: '2026-08-25T00:00:00.000Z', + }; + const changed = stateLib._computeChangedFrontmatterFields(snapshotFm, persistedFm, undefined); + assert.deepEqual( + changed, + ['progress.total_plans'], + 'total_plans moved and must be the only reported leaf: completed_plans (row 6) is unchanged so it must not appear, ' + + 'and last_updated (row 9) is the one-element provenance exclusion so it must never appear regardless of how much it moved', + ); + }); + + // Row 14 — preserve-always field restored to an identical value + test('anIdenticalRestoreIsNotAChange', () => { + writePersisted(['milestone: "v2"'], ['# State', '']); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { milestone: 'v2' }, body: '# State\n' }, + [], + [], + ); + assert.deepEqual(updated, [], 'milestone restored to the SAME value it already held is not a change'); + }); + + // Row 15 — the row that proves no classification-based exclusion survives + test('reportsAPlaceholderRestoreThatChangedTheValue', () => { + writePersisted(['milestone: "v2"'], ['# State', '']); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { milestone: 'unreleased' }, body: '# State\n' }, + [], + [], + ); + assert.deepEqual( + updated, + ['milestone'], + 'a preserve-if-placeholder field (milestone/milestone_name) genuinely restored to a DIFFERENT value must be ' + + 'reported — the old classification filter would have suppressed it, so this is the row that proves the filter is gone', + ); + }); + + // Row 16 — deletion + test('reportsADeletedKey', () => { + writePersisted(['status: "executing"'], ['# State', '']); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { status: 'executing', stale_key: 'x' }, body: '# State\n' }, + [], + [], + ); + assert.deepEqual(updated, ['stale_key'], 'a key present in the snapshot but absent from persisted is a deletion, which is a change'); + }); + + // Row 17 — addition + test('reportsAnAddedKey', () => { + writePersisted(['status: "executing"', 'new_key: "y"'], ['# State', '']); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { status: 'executing' }, body: '# State\n' }, + [], + [], + ); + assert.deepEqual(updated, ['new_key'], 'a key absent from the snapshot but present in persisted is an addition, which is a change'); + }); + + // Row 18 — #1162: body-first, frontmatter-key-flat fallback second. A field + // name that EXACT-MATCHES a frontmatter key must still resolve against the + // body first — proven by making the frontmatter value stay constant while + // only the body's label line moves, so a frontmatter-first reading would + // wrongly report "unchanged". + test('bodyLabelResolutionOrderUnchanged', () => { + writePersisted(['status: "A"'], ['# State', '', '**Status:** C', '']); + const snapshotBody = ['# State', '', '**Status:** B', ''].join('\n'); + const updated = stateLib._reconcileReportedFields( + statePath, + { fm: { status: 'A' }, body: snapshotBody }, + ['status'], + [], + ); + assert.deepEqual( + updated, + ['status'], + '#1162: a lowercase field name that exact-matches a frontmatter key must still resolve against the BODY first — ' + + 'the frontmatter value (A) never changed, but the body label did (B -> C), and only body-first resolution reports that', + ); + }); + + // Row 19 — internal-invariant throw for a preserve-when-unchanged field + // with no FRONTMATTER_KEY_TO_BODY_LABEL row. `getFieldClassification` is + // consulted via `stateTransitionMod.getFieldClassification` (a property + // read, not a destructured local), so mocking the shared module object is + // observed by the compiled seam under test. + test('unwiredLabelRowStillThrows', (t) => { + const original = stateTransitionMod.getFieldClassification; + mock.method(stateTransitionMod, 'getFieldClassification', (field) => { + if (field === 'fake_preserve_field') return { preservation: 'preserve-when-unchanged' }; + return original(field); + }); + t.after(() => mock.restoreAll()); + + writePersisted(['fake_preserve_field: "x"'], ['# State', '']); + assert.throws( + () => stateLib._reconcileReportedFields(statePath, { fm: {}, body: '# State\n' }, [], []), + (err) => err.code === 'STATE_BODY_LABEL_UNWIRED_ROW' && err.field === 'fake_preserve_field', + 'a preserve-when-unchanged field with no FRONTMATTER_KEY_TO_BODY_LABEL row must still throw STATE_BODY_LABEL_UNWIRED_ROW', + ); + }); + + // Row 21 — representation-insensitive equality (frontmatter scalars round-trip as strings) + test('stringAndNumberOfTheSameValueIsNotAChange', () => { + assert.equal(stateLib._stateFieldValuesDiffer('5', 5), false, '"5" vs 5 is the SAME value in two representations, not a change'); + assert.equal(stateLib._stateFieldValuesDiffer('0', 0), false, '"0" vs 0 is the SAME value, not a change'); + assert.equal( + stateLib._stateFieldValuesDiffer('1.0', 1), + true, + '"1.0" and 1 stringify to DIFFERENT representations ("1.0" vs "1") — this is a genuine representation change, not the same-value case row 21 protects', + ); + assert.equal(stateLib._stateFieldValuesDiffer('5', '6'), true, 'a genuine value change ("5" -> "6") must still count'); + }); + + // Row 22 — structural, not reference, equality + test('structuralEqualityForNestedValues', () => { + assert.equal( + stateLib._stateFieldValuesDiffer({ a: 1, b: { c: 2 } }, { a: 1, b: { c: 2 } }), + false, + 'two distinct object instances with the same structure are not a change', + ); + assert.equal( + stateLib._stateFieldValuesDiffer({ a: 1 }, { a: 2 }), + true, + 'a structural difference is a change', + ); + }); + + // Row 23 — hostile: missing parent + test('dottedPathWithMissingParentDoesNotThrow', () => { + let resolved; + assert.doesNotThrow(() => { + resolved = stateLib._resolveFrontmatterPath({}, 'progress.total_plans'); + }); + assert.equal(typeof resolved, 'symbol', 'a missing parent must resolve to the absence SENTINEL (a symbol), not undefined/null by coincidence'); + assert.ok( + resolved.toString().includes('state-field-absent'), + `expected the state-field-absent sentinel, got ${resolved.toString()}`, + ); + // Same input on both sides of the diff: absent-vs-absent is not a change. + assert.equal( + stateLib._computeChangedFrontmatterFields({}, {}, undefined).includes('progress.total_plans'), + false, + 'a leaf whose parent is absent on BOTH sides is not a change', + ); + }); + + // Row 24 — hostile: dotted path into a scalar parent + test('dottedPathIntoAScalarDoesNotThrow', () => { + let resolved; + assert.doesNotThrow(() => { + resolved = stateLib._resolveFrontmatterPath({ status: 'executing' }, 'status.foo'); + }); + assert.equal(typeof resolved, 'symbol', 'a dotted path into a scalar parent must resolve to the absence SENTINEL, not throw or return the scalar itself'); + assert.ok( + resolved.toString().includes('state-field-absent'), + `expected the state-field-absent sentinel, got ${resolved.toString()}`, + ); + assert.equal( + stateLib._computeChangedFrontmatterFields( + { status: 'executing' }, + { status: 'executing' }, + undefined, + ).includes('status.foo'), + false, + 'a dotted path into a scalar parent (unchanged on both sides) is not reported as a change', + ); + }); + + // Row 25 — security: prototype-pollution safety. Field names __proto__, + // constructor, prototype, toString both as PATH SEGMENTS (nested under a + // real object) and as actual frontmatter KEYS (own enumerable properties — + // built via JSON.parse, which — unlike object-literal syntax — creates a + // literal own property named "__proto__" rather than reassigning the + // object's prototype). + test('dottedResolutionDoesNotPollutePrototypes', () => { + const hostileKeys = ['__proto__', 'constructor', 'prototype', 'toString']; + + assert.doesNotThrow(() => { + // As path segments under a real nested object. + for (const key of hostileKeys) { + stateLib._resolveFrontmatterPath({ a: {} }, `a.${key}.polluted`); + } + // As flat top-level keys, and as own frontmatter keys via JSON.parse. + const hostileFm = JSON.parse( + '{"__proto__":"snap-proto","constructor":"snap-ctor","prototype":"snap-proto2","toString":"snap-tostr"}', + ); + for (const key of hostileKeys) { + stateLib._resolveFrontmatterPath(hostileFm, key); + } + }, 'hostile path segments and frontmatter keys must never throw'); + + const hostileSnapshot = JSON.parse( + '{"__proto__":"snap-proto","constructor":"snap-ctor","prototype":"snap-proto2","toString":"snap-tostr"}', + ); + const hostilePersisted = JSON.parse( + '{"__proto__":"persisted-proto","constructor":"snap-ctor","prototype":"snap-proto2","toString":"snap-tostr"}', + ); + writePersisted(['status: "executing"'], ['# State', '']); + stateLib._reconcileReportedFields( + statePath, + { fm: hostileSnapshot, body: '# State\n' }, + hostileKeys, + [], + ); + const changed = stateLib._computeChangedFrontmatterFields(hostileSnapshot, hostilePersisted, undefined); + assert.deepEqual( + changed, + ['__proto__'], + 'only the hostile key whose value genuinely differs must be reported — the diff must still function correctly, not merely avoid throwing', + ); + + assert.strictEqual(({}).polluted, undefined, 'a plain object must never gain a "polluted" own or inherited property AFTER running the resolution and the full diff'); + assert.strictEqual( + Object.prototype.hasOwnProperty.call(Object.prototype, 'polluted'), + false, + 'Object.prototype itself must gain no new member from hostile field names', + ); + }); + + // Row 26 — a literal key containing a dot, resolved BEFORE path traversal + // (pinned to the order `resolveFrontmatterPath` actually ships: a literal + // flat own-property wins first; only when no such flat key exists is the + // name split and walked as a dotted path). + test('literalDottedKeyResolvesBeforePathTraversal', () => { + const fm = { 'a.b': 'literal-value', a: { b: 'path-value' } }; + assert.equal( + stateLib._resolveFrontmatterPath(fm, 'a.b'), + 'literal-value', + 'a stored flat key containing a literal dot must win over dotted-path traversal into a same-named nested structure', + ); + }); + + // Row 10 — state_head IS reportable: it changes only when git HEAD actually + // moved, so a git-backed fixture across a real commit must surface it. + test('reportsStateHeadWhenHeadMoved', () => { + const dir = createTempGitProject('gsd-3872-statehead-'); + writeState(dir, ['---', 'status: "Paused"', '---', '', '# State', '', '**Status:** Paused', ''].join('\n')); + gitOrThrow(['add', '-A'], { cwd: dir }); + gitOrThrow(['commit', '-m', 'seed state'], { cwd: dir }); + + // Stabilizing write: the very first patch on a freshly-seeded fixture + // also reports gsd_state_version/progress.* from bootstrap resync noise, + // which is not this row's concern. + runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'In progress' })], dir); + + // Advance HEAD with a real commit, unrelated to STATE.md. + fs.writeFileSync(path.join(dir, 'dummy.txt'), 'x'); + gitOrThrow(['add', 'dummy.txt'], { cwd: dir }); + gitOrThrow(['commit', '-m', 'advance head'], { cwd: dir }); + + const result = runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'Executing' })], dir); + assert.ok(result.success, `state.patch failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.ok( + output.updated.includes('state_head'), + `state_head must be reported when git HEAD genuinely moved between writes; got updated=${JSON.stringify(output.updated)}`, + ); + }); + + // Row 11 — absence is not a change, in both of the matrix's named shapes: + // (a) HEAD did not move between two writes in a git-backed tree, and + // (b) the tree is not a git repo at all (state_head never appears at all). + test('absentStateHeadIsNotAChange', () => { + const gitDir = createTempGitProject('gsd-3872-statehead-unmoved-'); + writeState(gitDir, ['---', 'status: "Paused"', '---', '', '# State', '', '**Status:** Paused', ''].join('\n')); + gitOrThrow(['add', '-A'], { cwd: gitDir }); + gitOrThrow(['commit', '-m', 'seed state'], { cwd: gitDir }); + + runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'In progress' })], gitDir); + const unmoved = runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'Executing' })], gitDir); + assert.ok(unmoved.success, `state.patch failed: ${unmoved.error}`); + const unmovedOutput = JSON.parse(unmoved.output); + assert.ok( + !unmovedOutput.updated.includes('state_head'), + `state_head must NOT be reported when HEAD did not move; got updated=${JSON.stringify(unmovedOutput.updated)}`, + ); + + const plainDir = createTempProject('gsd-3872-statehead-nongit-'); + writeState(plainDir, ['---', 'status: "Paused"', '---', '', '# State', '', '**Status:** Paused', ''].join('\n')); + runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'In progress' })], plainDir); + const nonGit = runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'Executing' })], plainDir); + assert.ok(nonGit.success, `state.patch failed: ${nonGit.error}`); + const nonGitOutput = JSON.parse(nonGit.output); + assert.ok( + !nonGitOutput.updated.includes('state_head'), + `an absent state_head (non-git tree) must not read as a deletion/change; got updated=${JSON.stringify(nonGitOutput.updated)}`, + ); + cleanup(gitDir); + cleanup(plainDir); + }); + + // Row 28 — every reconcileReportedFields call site still compiles and + // reports coherently: one assertion per command family. + test('everyReportingCommandStillReportsCoherently', () => { + function assertArrayOfStrings(value, label) { + assert.ok(Array.isArray(value), `${label}: expected an array, got ${JSON.stringify(value)}`); + for (const entry of value) { + assert.equal(typeof entry, 'string', `${label}: every entry must be a string field name, got ${JSON.stringify(entry)}`); + } + } + + // cmdStatePatch + { + const dir = createFixture(); + writeState(dir, '# Project State\n\n**Status:** Ready\n'); + const r = runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'Executing now' })], dir); + assert.ok(r.success, `state.patch failed: ${r.error}`); + const out = JSON.parse(r.output); + assertArrayOfStrings(out.updated, 'cmdStatePatch'); + assert.ok(out.updated.includes('Status'), 'cmdStatePatch: the field that genuinely changed must be reported'); + cleanup(dir); + } + + // cmdStateUpdate + { + const dir = createFixture(); + writeState(dir, '# Project State\n\n**Status:** Ready\n'); + const r = runGsdTools(['state', 'update', 'Status', 'Executing now'], dir); + assert.ok(r.success, `state update failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.equal(typeof out.updated, 'boolean', 'cmdStateUpdate: updated is a single-field boolean, not an array'); + assertArrayOfStrings(out.preserved, 'cmdStateUpdate.preserved'); + assert.equal(out.updated, true, 'cmdStateUpdate: the requested field genuinely changed and must report success'); + cleanup(dir); + } + + // cmdStateAdvancePlan + { + const dir = createFixture(); + writeState(dir, [ + '# Project State', '', '**Current Plan:** 1', '**Total Plans in Phase:** 3', + '**Status:** Executing', '**Last Activity:** 2024-01-10', + ].join('\n') + '\n'); + const r = runGsdTools(['state', 'advance-plan'], dir); + assert.ok(r.success, `advance-plan failed: ${r.error}`); + const out = JSON.parse(r.output); + assertArrayOfStrings(out.updated, 'cmdStateAdvancePlan'); + assert.ok(out.updated.includes('Current Plan'), 'cmdStateAdvancePlan: the plan counter that genuinely advanced must be reported'); + cleanup(dir); + } + + // cmdStateRecordSession + { + const dir = createFixture(); + writeState(dir, [ + '# Project State', '', '## Session Continuity', '', + '**Last session:** 2024-01-10', '**Stopped at:** Phase 2, Plan 1', '**Resume file:** None', + ].join('\n') + '\n'); + const r = runGsdTools(['state', 'record-session', '--stopped-at', 'Phase 3, Plan 2'], dir); + assert.ok(r.success, `record-session failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.equal(out.recorded, true, 'cmdStateRecordSession: a genuine field write must record'); + assertArrayOfStrings(out.updated, 'cmdStateRecordSession'); + assert.ok(out.updated.includes('Stopped At'), 'cmdStateRecordSession: the field that genuinely changed must be reported'); + cleanup(dir); + } + + const phaseStateMd = [ + '# Project State', '', '**Current Phase:** 1', '**Current Phase Name:** setup', '**Total Phases:** 5', + '**Current Plan:** 0', '**Total Plans in Phase:** 0', '**Status:** Ready to plan', '**Last Activity:** 2026-03-20', + '**Last Activity Description:** Roadmap created', '', '## Current Position', + 'Phase: 1 of 5 (setup)', 'Plan: 0 of ? in current phase', 'Status: Ready to plan', + 'Last activity: 2026-03-20 -- Roadmap created', 'Progress: [..........] 0%', '', + ].join('\n'); + + // cmdStateBeginPhase + { + const dir = createFixture(); + writeState(dir, phaseStateMd); + const r = runGsdTools(['state', 'begin-phase', '--phase', '1', '--name', 'setup', '--plans', '4'], dir); + assert.ok(r.success, `begin-phase failed: ${r.error}`); + const out = JSON.parse(r.output); + assertArrayOfStrings(out.updated, 'cmdStateBeginPhase'); + assert.ok(out.updated.includes('Status'), 'cmdStateBeginPhase: Status genuinely changed and must be reported'); + cleanup(dir); + } + + // cmdStatePlannedPhase + { + const dir = createFixture(); + writeState(dir, phaseStateMd); + const r = runGsdTools(['state', 'planned-phase', '--phase', '3', '--name', 'API', '--plans', '5'], dir); + assert.ok(r.success, `planned-phase failed: ${r.error}`); + const out = JSON.parse(r.output); + assertArrayOfStrings(out.updated, 'cmdStatePlannedPhase'); + assert.ok(out.updated.includes('Total Plans in Phase'), 'cmdStatePlannedPhase: the plan count that genuinely changed must be reported'); + cleanup(dir); + } + + // cmdStateCompletePhase + { + const dir = createFixture(); + writeState(dir, phaseStateMd); + const r = runGsdTools(['state', 'complete-phase', '--phase', '1'], dir); + assert.ok(r.success, `complete-phase failed: ${r.error}`); + const out = JSON.parse(r.output); + assertArrayOfStrings(out.updated, 'cmdStateCompletePhase'); + assert.ok(out.updated.includes('Current Position'), 'cmdStateCompletePhase: Current Position genuinely changed and must be reported'); + cleanup(dir); + } + }); + + // Row 29 — property: a field appears in the changed set IFF its persisted + // value differs from the snapshot, over generated snapshot/persisted + // frontmatter pairs, ambient keys (last_updated) excluded. Arbitraries are + // declared INSIDE the property body (fast-check v4: a describe-body + // arbitrary kills the whole block). Seed pinned, numRuns bounded; a failing + // run's thrown error carries fast-check's own counterexample + seed for + // replay. + test('updatedIsExactlyTheChangedNonAmbientSet', () => { + // A closed, deliberately small key alphabet — none of these are + // body-sourced (current_phase/current_phase_name), declared-leaf parents + // (progress), or the ambient exclusion (last_updated), so the reference + // oracle below (plain hasOwnProperty + stateFieldValuesDiffer) is exactly + // what computeChangedFrontmatterFields is contractually required to match. + const KEYS = ['alpha', 'beta', 'gamma', 'delta', 'epsilon']; + const scalarArb = fc.oneof( + fc.string({ maxLength: 6 }), + fc.integer({ min: -100, max: 100 }), + fc.boolean(), + ); + const fmArb = fc.dictionary(fc.constantFrom(...KEYS), scalarArb, { maxKeys: KEYS.length }); + + fc.assert( + fc.property(fmArb, fmArb, (snapshotFm, persistedFm) => { + const changed = stateLib._computeChangedFrontmatterFields(snapshotFm, persistedFm, undefined); + const changedSet = new Set(changed); + const unionKeys = new Set([...Object.keys(snapshotFm), ...Object.keys(persistedFm)]); + for (const key of unionKeys) { + const inSnap = Object.prototype.hasOwnProperty.call(snapshotFm, key); + const inPers = Object.prototype.hasOwnProperty.call(persistedFm, key); + const expected = inSnap !== inPers + ? true + : stateLib._stateFieldValuesDiffer(snapshotFm[key], persistedFm[key]); + assert.equal( + changedSet.has(key), + expected, + `key=${key} expected changed=${expected} actual=${changedSet.has(key)} ` + + `snapshot=${JSON.stringify(snapshotFm)} persisted=${JSON.stringify(persistedFm)}`, + ); + } + }), + { seed: 20260825, numRuns: 200 }, + ); + }); +});