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