From 693f12ad56937355569fc8374c0f1217ef96a5a9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 9 Aug 2026 22:49:39 -0400 Subject: [PATCH] refactor(#3187): give state field extraction one canonical owner (#3283) * refactor(#3187): give state field extraction one canonical owner stateFieldValue in state-document.cts becomes the single owner of the #1760 frontmatter-then-body fallback chain. The new whole-repo guard found 14 independent re-derivations where the epic scoped 5, all now routed through it: cmdStateSnapshot (11), cmdStatePrune (2) and smart-entry fmScalar (1). state validate was a gate that could not fail. Every warning it could emit sat behind a phase resolved without the frontmatter tier, so a STATE.md whose phase lives only in frontmatter skipped the drift scan entirely and returned valid:true. It also read unstripped content, letting a frontmatter status: key shadow the body field (#1255 class). Both fixed; output gains a scope field so could-not-look stops being output-identical to looked-and-clean. Verified on the remote runner. * docs(#3187): document the state validate scope field and its reason codes Adds docs/how-to/interpret-state-validate-results.md so a reader can tell nothing-to-report from could-not-look, updates the COMMANDS.md and USER-GUIDE.md entries, corrects the CONTEXT.md glossary overstatement about Current Position sole ownership, and drops the changeset fragment. * fix(#3187): close three drift-guard evasion shapes and test the refuse path The isolated adversarial review found the ladder detector was evadable by ordinary reformatting, not just deliberately: a member or computed operand (fm.key / fm[key]) missed the bare-identifier backreference, a swapped tier order missed a hardcoded number-then-boolean sequence, and a ladder wrapped across lines missed single-line detection. All three now caught, each with its own test plus a proven boundary control. The frontmatter-parse refuse path on the destructive complete-phase route was unreachable and therefore untested. It is now driven by an injected parse failure and asserts STATE.md is byte-identical after the refusal, rather than shipping untested defensive code on a path that rewrites user state. Verified on the remote runner. * fix(#3187): widen the drift guard to the prompt layer and disclose tier-2 changes The code-review spec axis found the guard's scan surface was src/ only, which is Decision 4(d)'s forbidden allowlist one directory wide - and it had a live miss: gsd-core/workflows/smart-entry.md tells an agent to read status from frontmatter or the body, a prose expression of this same chain. The surface now covers the prompt layer. That one site carries a permanent written exemption rather than a ratchet: it is the gsd-tools-is-down fallback, so it cannot call the owner by construction, and a ratchet would imply removable debt that does not exist. Two tier-2 output changes shipped undisclosed and are now named in the changeset and docs: complete-phase's idempotency guard consulting frontmatter, and the workstream inventory resolving frontmatter-only fields. docs/COMMANDS.md gains a state complete-phase entry, which it never had. Also records Amendment 5 on ADR-3180, extracts the duplicated frontmatter-parse block the epic's own thesis forbids, and re-points two assertions from free-form warning prose onto the structured drift object. Verified on the remote runner. * chore(#3187): backfill changeset PR number pr:0 placeholder replaced with the real PR number now that #3283 exists. --------- Co-authored-by: sim --- .changeset/calm-bears-howl.md | 9 + CONTEXT.md | 4 +- docs/COMMANDS.md | 26 + docs/README.md | 1 + docs/USER-GUIDE.md | 4 +- ...80-planning-semantic-model-single-owner.md | 72 ++ .../interpret-state-validate-results.md | 57 ++ package.json | 2 +- scripts/lint-state-field-drift.cjs | 805 ++++++++++++++++++ scripts/lint-test-file-count.allowlist.json | 1 + src/smart-entry.cts | 32 +- src/state-document.cts | 78 ++ src/state.cts | 232 +++-- src/workstream-inventory.cts | 28 +- tests/state-field-drift.test.cjs | 649 ++++++++++++++ tests/state.test.cjs | 696 ++++++++++++++- 16 files changed, 2616 insertions(+), 80 deletions(-) create mode 100644 .changeset/calm-bears-howl.md create mode 100644 docs/how-to/interpret-state-validate-results.md create mode 100644 scripts/lint-state-field-drift.cjs create mode 100644 tests/state-field-drift.test.cjs diff --git a/.changeset/calm-bears-howl.md b/.changeset/calm-bears-howl.md new file mode 100644 index 000000000..b519e505d --- /dev/null +++ b/.changeset/calm-bears-howl.md @@ -0,0 +1,9 @@ +--- +type: Changed +pr: 3283 +--- +state validate now runs its drift scan for STATE.md files whose phase lives only in frontmatter, instead of silently skipping the scan and reporting a false-clean result (#3162); it also no longer lets a frontmatter status: key shadow the body Status field. Its output gains a scope field (complete/truncated/unscoped/unreadable) reporting whether the check could actually run — valid still means no drift was found, and is not derived from scope. (#3187) + +state complete-phase's idempotency guard now consults frontmatter current_phase (via the same fallback chain as state validate), so a STATE.md whose phase lives only in frontmatter is no longer silently rolled back on a re-run of `state complete-phase --phase N`. It also gains a new refusal path: when the frontmatter cannot be parsed, the command now errors out ("Unable to read STATE.md frontmatter; refusing to run complete-phase to avoid a destructive rollback") instead of guessing. (#3187) + +workstream list/status/progress's per-workstream state projection (status, current_phase, last_activity) now resolves those fields from frontmatter when the body has no corresponding field, instead of reporting them absent — a frontmatter-only STATE.md's workstream inventory output changes accordingly. (#3187) diff --git a/CONTEXT.md b/CONTEXT.md index 45ab04513..f2cc76289 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -56,7 +56,7 @@ Adapter Module that satisfies native query dispatch at the Dispatch Policy seam, Module owning projection from dispatch results/errors to CLI `{ exitCode, stdoutChunks, stderrLines }` output contract. ### STATE.md Document Module -Module owning STATE.md parse, field extraction, field replacement, status normalization, frontmatter reconstruction, and `## Current Position` section scoping (`stateCurrentPositionSlice`, #1956 — the one owner of that scope; `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). 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`. +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`. ### 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` and the direct `writeStateMd` caller in `milestone.cts:552` (phase.cts's former direct caller has since been migrated away); verify's `regenerateState` factory-reset primitive stays as a direct `writeStateMd` call (`verify.cts:1925`). 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`). @@ -109,6 +109,8 @@ Module owning `.planning` path resolution, active workstream pointer policy (`se ### Workstream Inventory Module Module owning workstream directory discovery, per-workstream state projection, phase/plan/summary counting, roadmap-declared phase count, active marker projection, and active-workstream collision inputs. Command handlers render list/status/progress outputs from this inventory instead of rescanning `.planning/workstreams/*` directly. Source of truth for the pure projection is `gsd-core/bin/lib/workstream-inventory-builder.cjs` (a Builder Module); the Reader Adapter `gsd-core/bin/lib/workstream-inventory.cjs` collects filesystem inputs and delegates projection to the Builder. +`readStateProjection` (`src/workstream-inventory.cts`) resolves the per-workstream `status`/`current_phase`/`last_activity` fields through the #1760 fallback chain owner (`stateFieldValue`, STATE.md Document Module, #3187): the YAML frontmatter scalar is preferred, falling back to the body field only when the frontmatter side is absent. A frontmatter-only STATE.md — previously projected as `unknown`/`null` for these fields — now reports its real values. + Completion is scoped to the workstream's CURRENT milestone (#2562): `roadmap_phase_count`, `completed_phases` and `progress_percent` describe that one milestone, not the workstream's lifetime, and `status: "milestone complete"` is derived from the current milestone's own close artifacts (an archived `milestones/-ROADMAP.md` snapshot, or `isMilestoneShippedInRoadmap` on its own ROADMAP heading) rather than any project-lifetime shipped marker. That marker is a claim, not a verdict: it is cross-validated against the milestone's own artifacts, and the two signals are checked at different strengths because one check cannot serve both. A live-ROADMAP `heading` marker is refused when the completion ratio is short. An archived `snapshot` is not ratio-gated alone — `milestone complete` moves the milestone's phase directories into `milestones/-phases/` while copying, never truncating, the live ROADMAP, so a CLEAN archive reads 0/N by construction — and is refused on the conjunction of a still-live in-milestone directory (the archive is not clean; a phase was added or reopened after it, reachable because `milestone complete` does not advance `STATE.md`'s `milestone:` field) AND a short completion ratio. The `legacy` project-lifetime fallback is ungated by signal. The cross-check as a whole engages only under milestone scoping, for all three signals: unscoped, the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check against. A refused marker does not fall through to a `STATE.md` field asserting the same thing, and the refusal is reported as `milestone_shipped_unverified`, projected by `workstream list`/`status`/`progress` so it is visible at the CLI — distinct from `status_conflict`, which reports only derived-vs-field disagreement. Membership and the denominator are derived from one canonical key surface — the phase-id owner module's `phaseKeyFromDir`/`phaseKeyFromProse` — with directory membership additionally consulting the Roadmap Parser Module's `getMilestonePhaseFilter` when that filter is genuinely version-scoped (that filter keeps its own internal normaliser, OR'd in, so a divergence can only widen membership); the Builder enforces `completed_phases <= denominator` and throws on violation rather than capping the percentage. A current milestone that is DECLARED but not yet populated is scoped, not unscoped: `versionSectionFound` / `missingExplicitVersion` / a Progress table attributing every row elsewhere each witness that state, and without them scoping switched off and the whole-lifetime fallback reported a milestone with no work done as 100%. Within such a milestone, membership inverts — a directory belongs unless another milestone's row claims it — so a phase scaffolded ahead of the ROADMAP is counted rather than hidden. Scoping is stated by the Reader (`milestoneScoped`) rather than inferred from `currentMilestonePhaseCount > 0`, which cannot represent a scoped, legitimately zero-phase milestone. A ROADMAP that attributes no versions at all matches none of the witnesses, so free-form legacy projects keep their whole-roadmap count. The Reader passes the workstream name to `getMilestonePhaseFilter`/`extractCurrentMilestone` so their `planningDir` resolution targets `.planning/workstreams//`, which a loop over workstreams cannot express through `GSD_WORKSTREAM`. ### Project-Root Resolution Module diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 299d97f27..87fc302a1 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1770,6 +1770,17 @@ Detect drift between STATE.md and the actual filesystem. node gsd-tools.cjs state validate ``` +The report also carries a `scope` field reporting whether the drift derivation could actually run: + +| `scope` | Meaning | +|---|---| +| `complete` | The derivation ran over usable input — a resolvable phase, a readable disk scan. `valid`/`warnings`/`drift` are a real answer. | +| `truncated` | Part of the input was cut short (e.g. the phase's plan/summary scan hit its cap) — the answer may be incomplete. | +| `unscoped` | `Current Phase` could not be resolved from either frontmatter or body — there was nothing to scope the disk lookup to, so the derivation never ran. | +| `unreadable` | The frontmatter parse or a filesystem read (the phases directory scan) failed — the derivation could not consult its input. | + +`valid` is **not** routed from `scope`: `valid` still means "no drift warnings were found," and `scope` says whether the scan could actually run. A freshly-initialized project reports `{valid:true, warnings:[], drift:{}, scope:'unscoped'}` — nothing was wrong, and the phase could not be checked. See [Interpret `state validate` results](how-to/interpret-state-validate-results.md) for how to act on each `scope` value. + --- ### `state sync [--verify]` @@ -1830,6 +1841,21 @@ node gsd-tools.cjs state planned-phase --phase 3 --plans 2 --- +### `state complete-phase [--phase N]` + +Mark the current phase as COMPLETE in STATE.md — updates the body `Status`, `Last Activity`, and `## Current Position` fields. `--phase` is optional; when omitted, the phase is resolved from STATE.md's `Current Phase`/`Phase` fields (frontmatter `current_phase` preferred, falling back to the body). + +**Idempotency guard (#3489):** if STATE.md's canonical current phase already names a phase distinct from the one being marked complete — including when that phase lives only in frontmatter `current_phase`, not the body — the command is a no-op (`idempotent: true`) rather than rolling STATE.md back to the requested phase's moment-of-completion. If the frontmatter cannot be parsed at all, the command refuses outright (`Unable to read STATE.md frontmatter; refusing to run complete-phase to avoid a destructive rollback`) instead of guessing. + +**Prerequisites:** `.planning/STATE.md` exists +**Produces:** Updated `STATE.md` marking the resolved phase complete, or a no-op when the guard determines the phase was already superseded + +```bash +node gsd-tools.cjs state complete-phase --phase 3 +``` + +--- + ## Community Commands ### Community Hooks diff --git a/docs/README.md b/docs/README.md index be16b91c6..a4749cc1c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -36,6 +36,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Isolate work with workspaces](how-to/isolate-work-with-workspaces.md) — use workspaces to sandbox experimental or risky changes - [Debug a failed execution](how-to/debug-a-failed-execution.md) — diagnose and recover from broken or incomplete phase execution - [Interpret scope-conformance warnings](how-to/interpret-scope-conformance-warnings.md) — read the advisory the worktree-wave merge emits when a plan branch commits outside its declared scope +- [Interpret `state validate` results](how-to/interpret-state-validate-results.md) — read the `scope` reason codes and tell "nothing to report" apart from "could not look" - [Spike and sketch](how-to/spike-and-sketch.md) — use `/gsd-spike` and `/gsd-sketch` for exploratory work before committing to a plan - [Design a UI phase](how-to/design-a-ui-phase.md) — use the UI phase loop for frontend and visual work - [Develop a Capability for GSD 1.5+](how-to/develop-a-capability.md) — add feature Capabilities, hook fragments, and registry entries diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index c4b7b8463..5e4da0dc9 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -644,6 +644,8 @@ node "$HOME/.claude/gsd-core/bin/gsd-tools.cjs" state sync --verify # Previe node "$HOME/.claude/gsd-core/bin/gsd-tools.cjs" state sync # Reconstruct STATE.md ``` +`state validate`'s report carries a `scope` field alongside `valid` — `valid:true` means no drift was found, but `scope` says whether the check could actually run at all (a phase that could not be resolved, or an unreadable frontmatter/phases directory, reports `valid:true` too, because there was nothing to flag). See [Interpret `state validate` results](how-to/interpret-state-validate-results.md) before treating a passing `state validate` as "clean." + ### A Command Looks Frozen After "Spawning..." GSD subagents run in a separate context window — their work is invisible to the parent session while in progress. Do not interrupt the session. Wait for the result; research and planning agents routinely take 1–5 minutes. @@ -967,7 +969,7 @@ To disable parallel execution entirely: `/gsd-settings` → set `parallelization | Phase went wrong | `git revert` the phase commits, then re-plan | | Need to change scope | `/gsd-phase` (default), `/gsd-phase --insert`, or `/gsd-phase --remove` | | Something broke | `/gsd-debug "description"` (add `--diagnose` for analysis without fixes) | -| STATE.md out of sync | `state validate` then `state sync` | +| STATE.md out of sync | `state validate` then `state sync` — check the report's `scope` field, not just `valid` ([interpret results](how-to/interpret-state-validate-results.md)) | | Workflow state seems corrupted | `/gsd-forensics` | | Quick targeted fix | `/gsd-quick` | | Plan doesn't match your vision | `/gsd-discuss-phase [N]` then re-plan | diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index 05d715ab4..fd94e88fb 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -739,3 +739,75 @@ out, and an incomplete list is the same defect in miniature that this epic exist as a *named canonical function* — the only sanctioned exemption form. `locateMilestoneHeadings` was refactored into a version-filtered view over one shared grammar source so the two primitives cannot drift, with a parity test asserting the filtered enumeration equals the locator's selection. + +### Amendment 5 — Phase 5 (#3187) validation: the contract held; the guard nearly reported a zero it had not earned + +Decision 2's contract needed **no change** for its fifth consumer. `stateFieldValue` returns +`{ value, scope }` over the frozen four-member `SCOPE`, and every case this phase hit was already +one of them. `src/planning-scope.cts` is untouched, so Phase 5 carries no `.cts` six-gate ripple. + +**Declared deviation from Decision 1's owner surface.** Decision 1 names the owner as +`stateExtractField` "carrying the #1760 fallback chain". Shipped instead: `stateExtractField` is +left **byte-identical** and the chain is a *new* owner beside it. `stateExtractField` answers a +narrower question — "what is field F in this body text" — which genuinely has no scope dimension, +and it has **20 direct callers with a CRITICAL blast radius** (53 affected symbols, 5 process +flows). Putting the scope on the primitive would have rewritten every one of those call sites to +buy nothing. The thing that can fail to run is the *chain*, so the chain is what carries the scope. +This is a signature deviation, not a contract change — the same class Amendment 3 declared for +`listMilestonePhaseDirs`. + +**The copy count was a lower bound for the SIXTH consecutive time — 14, where the epic scoped 5.** +Per Amendment 3's standing rule the guard was built and run **before** scope was fixed. Grouped by +ladder-bearing function: `cmdStateSnapshot` 11, `cmdStatePrune` 2, `smart-entry.cts::fmScalar` 1. +`find_symbol` separately reports 20 direct callers, which is the figure §7.7 already told Phase 5 to +drive from rather than the epic's "five call sites". + +**The first guard would have lied, and the mechanism is worth recording because it is new.** A +bounded-line-window detector was built first and found **7**. The other **7** — +`state.cts:1484–1497` — sat further from `cmdStateSnapshot`'s ladder than its 15-line window +reached. Migrating the visible 7 would have left a green guard with seven survivors: Decision 4(a)'s +"a zero it did not earn", arrived at by a mechanism no prior phase met. Replaced with +**function-scoped co-occurrence** — *a function that both reads a frontmatter scalar and calls the +body extractor is re-deriving the chain* — which needs no threshold constant and cannot be reflowed +around. **Generalized lesson: any numeric window in a drift guard is a Goodhart target; scope +detection to a syntactic unit instead.** Two further evasions (a member/computed operand, a swapped +tier order) were found by the isolated review and closed with their own tests; a guard is not +trusted until a deliberate re-derivation is shown to fail it. + +**Decision 4(d) applied to this derivation, and it bit again.** The guard's first scan surface was +`src/` — which is exactly the one-directory-wide allowlist 4(d) was written to forbid, repeated by a +phase that had read 4(d). `gsd-core/workflows/smart-entry.md` instructs an agent to read `status` +from "frontmatter `status:` or body `**Status:**`" — a prose expression of this chain. The surface +is now scanned. That site carries a **permanent exemption with a written reason**, not a Decision +4(e) ratchet: it is the `gsd-tools`-is-down fallback, so by construction it cannot call the owner, +and a ratchet implies removable debt with an owning issue. **Not every unconsolidated site is +debt** — 4(e) governs debt, and a documented impossibility is a different thing. + +**A seventh derivation family was found and deliberately NOT consolidated, on evidence.** Three +`## Current Position` locators exist. `state.cts::matchCurrentPositionSection` genuinely delegates +to `state-document.cts::stateCurrentPositionSlice`, so it is not a copy. But +`state-transition.cts::locateCurrentPosition` / `sliceCurrentPositionSection` is independent, and +both were run over eight adversarial bodies. Fence-awareness turned out to be **shared** (both reach +`tokenizeHeadings`), so that concern was unfounded — but the two diverge in *both* directions: +`locateCurrentPosition` misses `### Current Position` at h3, while `collectSection` applies +`.trimEnd()` where the byte-exact mutation callers (`mutateCurrentPositionFirstTime` / +`mutateCurrentPositionResume`) depend on the trailing newline for span reassembly. Folding would +silently drop a newline from every Current Position body on every write. **This is Amendment 3's +"0.x split" recurring: one owner governs who computes an answer, not how many questions share it.** +Left for its own phase; `CONTEXT.md`'s claim that the read-path slice is "the one owner of that +scope" is corrected to name the mutation-path locator. + +**Decision 3's Tier-2 table, re-derived for Phase 5:** + +| Command surface | Output change | +|---|---| +| `state validate` | gains `scope`. `valid` is **not** routed from it — Decision 2 rejected a boolean ok/degraded, and a legacy STATE.md with no `## Current Position` is `UNSCOPED`, a supported degrade, not an invalid document. It also stops resolving its phase from unstripped content, closing a #1255-class frontmatter shadowing where a frontmatter `status:` key won over the body field | +| `state complete-phase` — **not predicted** | the #3489 idempotency guard now consults frontmatter `current_phase`, so a project whose phase lives only in frontmatter is no longer silently rolled back on a re-run; adds a refusal when frontmatter cannot be read | +| `workstream` inventory — **not predicted** | `readStateProjection` resolves frontmatter-only `status`/`current_phase`/`last_activity` instead of reporting them absent | +| `state snapshot`, `state prune`, `/gsd:next` signals | resolve fields through the one owner, so they can no longer disagree about the same STATE.md. `smart-entry`'s read stays deliberately **unscoped** under a written exemption — it asks a different question than `state.cts`'s #1776/#2956-scoped copies, and folding it would silently change routing | + +**What this phase does NOT close.** §7.7's rule says `state validate` "reports invalid for a +genuinely invalid document" but never defines *genuinely invalid*. This phase does not invent that +definition — it makes the derivation's scope visible so the question becomes answerable, and emits +the drift warnings that were always intended but unreachable. Widening what counts as invalid stays +undecided, per §7's own rule. diff --git a/docs/how-to/interpret-state-validate-results.md b/docs/how-to/interpret-state-validate-results.md new file mode 100644 index 000000000..bb86ec15c --- /dev/null +++ b/docs/how-to/interpret-state-validate-results.md @@ -0,0 +1,57 @@ +# How to interpret `state validate` results + +**Goal:** Read a `state validate` report correctly — distinguish *"nothing to report"* from *"could not look"*, so a passing `valid:true` is never mistaken for a guarantee the check actually ran. + +**Prerequisites:** A `.planning/STATE.md` file. Run the check with: + +```bash +node gsd-tools.cjs state validate +``` + +For the flag/output reference, see [`state validate`](../COMMANDS.md#state-validate). This guide covers only how to *read and act on* the result. + +--- + +## Read the two fields separately + +`state validate` returns two fields that answer two different questions, and conflating them recreates the exact defect (#3162) this command's `scope` field exists to prevent: + +| Field | Question it answers | +|---|---| +| `valid` | "Did the drift scan find any problems?" — `true` means **no warnings were raised**. | +| `scope` | "Could the drift scan actually run?" — reports whether the check had what it needed to look. | + +`valid` is **never** derived from `scope`. A document the scan could not check at all (`scope` other than `complete`) still reports `valid:true`, because zero warnings really were raised — there just wasn't anything to raise them from. Reading `valid:true` alone as "STATE.md is clean" is the trap: always check `scope` first. + +--- + +## The `scope` reason-code table + +| `scope` | What it means | What caused it | What to do | +|---|---|---|---| +| `complete` | The derivation ran over usable input — a resolvable `Current Phase` and, if a matching phase directory exists, a readable disk scan of it. `valid`/`warnings`/`drift` are a real, trustworthy answer. | Normal operation: STATE.md's phase resolved (from frontmatter or body) and the filesystem was readable. | Trust the result as-is. If `valid:false`, act on the listed `warnings`/`drift` entries (typically `state sync`). | +| `truncated` | Part of the input was cut short before the scan finished — the phase directory's plan/summary scan hit an internal cap partway through. The `valid`/`drift` answer may be **incomplete**, not necessarily wrong. | An unusually large phase directory (many plan/summary files) exceeded the scan's bounded window. | Do not treat `valid:true` here as a clean bill of health. Inspect the phase directory directly (`ls .planning/phases//`) to confirm counts by hand, or reduce/split the phase's plan set if this recurs. | +| `unscoped` | `Current Phase` could not be resolved from **either** the frontmatter scalar or the body field — there was no phase to scope the disk lookup to, so the drift derivation never ran at all. | Most commonly a freshly-initialized project with no phase set yet (a genuine, supported state). Less commonly, a STATE.md whose `Current Phase` field was dropped or malformed. | If the project has not started a phase yet, this is expected — no action needed. If the project is active and you expect a phase to be set, open STATE.md and check the `current_phase` frontmatter key and the body's `**Current Phase:**` row; run `state sync` to reconstruct it from disk if it is missing. | +| `unreadable` | An input the scan needed could not be consulted at all — either the frontmatter block failed to parse, or a filesystem read (the phases directory scan) failed mid-scan. | An unterminated/malformed YAML frontmatter fence, or a filesystem error (permissions, a race with a concurrent write) while reading `.planning/phases/`. | Treat `valid:true` here as **not trustworthy** — the scan degraded silently before this field existed, and now surfaces that instead of hiding it. Check that STATE.md's frontmatter fence (`---` / `---`) is well-formed, and that `.planning/phases/` is readable by the current user. Re-run `state validate` after fixing either. | + +--- + +## The case this exists for: "`valid:true` — but is it trustworthy?" + +If you only ever read `valid`, every one of the four `scope` values above looks identical: `true`. That collapse is the exact bug this field was added to close (#3162) — a STATE.md whose phase lived only in frontmatter used to silently skip the entire drift scan and report `{valid:true, warnings:[], drift:{}}`, indistinguishable from a phase that was checked and found clean. + +So before trusting a green `state validate`, always inspect `scope`: + +- **`scope:'complete'`** — trustworthy. The scan ran; `valid:true` means clean. +- **Anything else** — not yet checked, or only partially checked. `valid:true` here means *"no problems were found in what could be looked at,"* which is a materially weaker claim. Use the table above to find out why, and whether that is expected (a fresh project, `unscoped`) or a problem worth fixing (`unreadable`, or a `truncated` scan on a large phase). + +A freshly-initialized project is the clearest example of a **legitimate** non-`complete` scope: it reports `{valid:true, warnings:[], drift:{}, scope:'unscoped'}`, which reads as *"nothing was found wrong, and the phase could not be checked"* — not as a defect to fix. + +--- + +## Related + +- [`state validate`](../COMMANDS.md#state-validate) — command reference, flags, and the full output shape +- [`state sync`](../COMMANDS.md#state-sync---verify) — reconstructs STATE.md from disk when drift or a missing `Current Phase` is found +- [Recover and troubleshoot](recover-and-troubleshoot.md) — broader STATE.md recovery guidance +- [docs index](../README.md) diff --git a/package.json b/package.json index c0e22fbd4..66f2a3e00 100644 --- a/package.json +++ b/package.json @@ -112,7 +112,7 @@ "lint": "eslint . --cache --cache-location node_modules/.cache/eslint/", "lint:fix": "eslint . --fix", "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-state-field-drift.cjs b/scripts/lint-state-field-drift.cjs new file mode 100644 index 000000000..281890f56 --- /dev/null +++ b/scripts/lint-state-field-drift.cjs @@ -0,0 +1,805 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Anti-divergence drift guard for the STATE.md FIELD-EXTRACTION FALLBACK + * CHAIN (epic #3180, issue #3187, ADR-3180 Decision 4, spec §7.7). + * + * The derivation this guard protects is "read a STATE.md field: prefer the + * YAML frontmatter scalar (string, trimmed-non-empty; or number/boolean + * coerced to a string), else fall back to the body field via + * `stateExtractField(body, bodyField)`". `src/state-document.cts` is + * DESIGNATED the single canonical owner of this grammar (issue #3187 Phase + * 5), but as of this guard's authorship (Phase 5's guard-first step, per + * ADR-3180 Amendment 3's standing rule) the canonical function does not yet + * exist there — this script is written to discover every existing + * re-derivation BEFORE any src/ file is touched, exactly the ordering + * `lint-plan-count-drift.cjs` and its siblings established. + * + * Per ADR-3180 Decision 4(a) this guard discovers call sites by SCANNING THE + * WHOLE `src/` TREE, not by consulting an allowlist of known files. Per + * Decision 4(d) that surface is widened further still: `src/` alone is + * itself the forbidden allowlist, one directory wide, so this guard ALSO + * scans the prompt-layer markdown (`gsd-core/workflows`, `commands`, + * `agents`, `skills`) for a PROSE re-derivation of the same chain — see the + * "PROMPT-LAYER PROSE DETECTION" section below `findStateFieldDrift`. + * + * DETECTION: FUNCTION-SCOPED CO-OCCURRENCE, not a bounded line-window. + * An earlier draft of this guard used a fixed line-distance window between + * the ladder and the fallback call. That shape was REJECTED: it produced a + * documented "known miss" on live copies inside the very function it was + * scanning (state.cts `cmdStateSnapshot`'s `Current Plan` / `Total Plans in + * Phase` / `Status` / `Progress` / `Last Activity` reads all sit further + * from the ladder than any defensible line count), which means the guard + * could report "0 re-derivations" after only the nearest few copies were + * migrated while several more of the SAME kind survived untouched in the + * SAME function — "a zero it did not earn" (ADR-3180 Decision 4(a)). A fixed + * N is also trivially gameable by reflow (Goodhart's law): moving a call one + * line further from its ladder silences the guard without changing the + * derivation at all. The window is dropped entirely; there is no magic + * number anywhere in this detection. + * + * The invariant instead: a NAMED FUNCTION that BOTH (a) contains a + * "frontmatter scalar coercion ladder" — the SAME operand compared via + * `typeof OPERAND === 'number'` and `typeof OPERAND === 'boolean'` + * (TYPEOF_TIER_CLAUSE_RE, order-independent, member/computed operands + * included, an optional `typeof OPERAND === 'string'` tier tolerated + * anywhere among them — see the ternary shape in `cmdStatePrune` below), + * within LADDER_WINDOW_LINES of each other — ANYWHERE in its own body, + * INCLUDING inside a nested closure it defines, AND (b) calls + * `stateExtractField(` anywhere in that same body, is re-deriving the + * fm-else-body fallback chain. EVERY `stateExtractField(` call line inside + * such a function is reported, not just the first. + * + * LADDER DETECTION, evasion-resistant shapes (see "LADDER DETECTION DETAIL" + * below for the full account, including the one shape still NOT caught): + * - operand may be a bare identifier (`v`), a dotted member expression + * (`fm.key`), or a computed access (`fm[key]`) — matched via + * OPERAND_SOURCE and compared as captured TEXT, not a bare-identifier + * backreference. + * - the two required tiers ('number', 'boolean') may appear in EITHER + * order; an optional third 'string' tier may sit anywhere among them. + * - the two clauses may sit on the SAME line (the classic single-line + * `||` chain) or on DIFFERENT lines within LADDER_WINDOW_LINES of each + * other — including as two entirely separate `if` statements, not just + * one expression wrapped across a line break. + * + * FUNCTION ATTRIBUTION. Two declaration shapes are recognised as opening a + * new named function scope: + * - `function NAME(...) {` (FUNCTION_DECL_RE) — top-level OR nested, at + * any indentation. `src/smart-entry.cts`'s `fmScalar` (line 144) is this + * shape, and is itself top-level. + * - `const NAME = (...): ReturnType => {` (ARROW_CONST_RE) — an arrow + * function assigned to a `const`, with a block body (`=> {`, not an + * expression body like `=> ({...})`, which never opens a new function + * frame — its `{` is an object literal, still counted toward brace + * depth, but attributes no name). `src/state.cts`'s `cmdStateSnapshot` + * defines its `fmScalar` (line 1464) this way, NESTED inside + * `cmdStateSnapshot` itself. + * `ARROW_CONST_RE`'s pattern requires `=>\s*\{` literally, so it only ever + * matches a line that already carries its opening brace — pushed + * immediately. `FUNCTION_DECL_RE` carries no such guarantee: a multi-line + * signature (parameters and/or a return-type annotation spilling onto later + * lines) matches on a line with NO `{` at all. An earlier version of this + * guard pushed such a match immediately anyway, recording `openDepth` at the + * ENCLOSING scope's depth rather than the function's own — for a top-level + * function, `openDepth: 0`, and because real code never reaches negative + * depth, a frame pushed with `openDepth: 0` could NEVER pop, sitting at the + * bottom of the stack for the rest of the file and silently misattributing + * every later line with no OTHER open frame to it. `src/state.cts`'s + * `preferNewerLastActivity` (a 4-line signature, `{` on its own line) is the + * live instance that surfaced this; it caused no observed false violation + * only because nothing ever called `stateExtractField(` at true module scope + * after it, not because the tracking was sound. `buildFunctionInfo` now + * DEFERS a `FUNCTION_DECL_RE` match (`pendingDeclName`) across lines until + * the first subsequent line whose brace count actually increases, and pushes + * the frame THERE, with `openDepth` computed from that line's `depth` — + * matching every other frame's push convention. A pending name is abandoned + * (never pushed) if a `;` terminates the statement before any `{` appears — + * a type-only declaration, `declare function`, or overload signature, none + * of which open a body. + * Function scopes NEST via a brace-depth stack: entering either shape pushes + * a frame; the frame pops once brace depth returns below the depth recorded + * when it was pushed. A line is attributed to the INNERMOST currently-open + * named frame (falling back to whatever enclosing frame IS open — typically + * the nearest enclosing top-level function — when a line sits between two + * sibling nested scopes; module-level code with no open frame is + * unattributed and therefore never a violation, matching "if you cannot + * identify one" in the design brief). + * + * TRANSITIVE ladder attribution is what makes `cmdStateSnapshot` (whose + * OWN top-level statements never spell the ladder themselves — only its + * nested `fmScalar` closure does) still register as ladder-bearing: when + * LADDER_RE matches a line, EVERY frame currently open on the stack at that + * point — not just the innermost — is marked ladder-bearing, because a + * nested closure's body is lexically part of every one of its enclosing + * functions' own bodies. `stateExtractField(` calls are attributed to the + * INNERMOST frame only (no transitivity needed there: the call already sits + * directly inside whichever frame is innermost at that point). + * + * BRACE-DEPTH COUNTING runs over `scanCode`'s per-line, cross-file output + * (see that function's own header for the full rationale and the concrete + * bug its cross-line comment/template tracking fixes) — comments and + * quoted/backtick string and template literal CONTENTS are already removed + * before a single brace is counted, escape-aware and threaded across line + * boundaries, so neither a brace inside a string (`{ label: '{' }`) nor one + * inside a multi-line block comment or template literal perturbs the depth + * count. It does NOT specially recognise regex literals (a `{` inside a + * `/.../ ` quantifier, e.g. `/x{2,3}/`, is counted as a plain character); + * see `scanCode`'s header for why every such literal actually present in + * `src/` today is harmless (balanced on its own line). + * + * MUST-NOT-FLAG case verified by running the guard (the earlier "case + * variant chain" exemption for `src/state.cts:1488` was WRONG and is + * SUPERSEDED — see the header of the guard's initial version in git history + * for the retracted reasoning; `cmdStateSnapshot` is ladder-bearing, so + * *every* `stateExtractField(` call inside it, including line 1488, is + * correctly a violation now): + * - `src/smart-entry.cts`'s `fmScalarKey` (lines 152-158): its own + * `typeof v === 'number' || typeof v === 'boolean'` ladder (line 156) + * reads a value out of a NESTED frontmatter object and never calls + * `stateExtractField(` anywhere in its own body — it is ladder-bearing + * but call-free, so it is correctly never flagged. This is the live + * control case proving the guard still distinguishes "has a ladder" from + * "re-derives the fallback chain": a ladder alone, with no body + * fallback call in the same function, is a different question (reading + * a nested frontmatter object, full stop) and stays silent. + * - Any ladder or `stateExtractField(` call appearing inside a `//` line + * comment or a `/* *\/`-style block comment (single- or multi-line). + * `scanCode` blanks comment text — cross-line-aware — before either + * regex runs, so prose describing this derivation is never mistaken for + * a copy of it (ADR-3180 Amendment 3's "trains readers to reflexively + * exempt documentation" note). + * + * FUNCTION-SCOPED EXEMPTIONS (per ADR-3180 Decision 4(a): NEVER a bare + * whole-file allowlist — Decision 4(d) records that a whole-file exemption + * on the owner is precisely how `getMilestoneInfo` stayed invisible to an + * earlier guard). `src/state-document.cts` is the owner of this grammar; + * its `stateFieldValue` (added for issue #3187 Phase 5, landed in this same + * working tree while this guard was being authored — see the guard's commit + * history / PR for the exact sequencing) IS the canonical + * frontmatter-scalar-then-body-field chain, not a copy of it, so it is the + * ONLY entry in FUNCTION_SCOPED_EXEMPTIONS. Every other function in + * `state-document.cts` — including any future re-derivation added anywhere + * else in that file — is still scanned and still flagged (mirrors + * `lint-completion-ratio-drift.cjs`'s `FUNCTION_SCOPED_EXEMPTIONS` for + * `clampPercent`/`clampPercentFromFraction`). + * + * Every regex below is small, bounded, and has no nested/overlapping + * quantifiers; the string-literal and brace scans are plain escape-aware + * character loops, not regexes, so there is nothing for a backtracking + * engine to explore. `npm run lint:ci` runs CodeQL js/redos over this repo; + * mirrors the ReDoS discipline of the sibling drift guards. + * + * The tree-walk / root-confinement / sanitizer machinery is SHARED with the + * sibling drift guards via `scripts/lib/drift-scan.cjs` (ADR-3180 Decision + * 4). This guard's detection shape needs no regex-literal extraction, so it + * does not use `readRegexLiteralAt`; the reported fragment is simply the + * trimmed source line, bounded to MAX_REGEX_LITERAL_LEN characters. + * + * LADDER DETECTION DETAIL (evasion history). An earlier version of this + * guard's ladder pattern was a single regex, `typeof (\w+) === 'number' \|\| + * typeof \1 === 'boolean'`, tested against ONE line. An isolated adversarial + * review found three live-shaped ways past it, all verified to produce ZERO + * violations against a real re-derivation: + * (a) a member-expression or computed operand — `typeof fm.key === + * 'number' || typeof fm.key === 'boolean'` — never matched, because + * the bare-identifier backreference (`\w+` then `\1`) cannot match + * `fm.key`/`fm[key]` at all. + * (b) the tiers in the opposite order — `typeof v === 'boolean' || + * typeof v === 'number'` — never matched, because the pattern + * hardcoded 'number' before 'boolean' with no alternative ordering. + * This one is plausible from an ordinary code-review reformat, not + * deliberate evasion. + * (c) the ladder split across two lines, or written as two separate `if` + * statements instead of one `||` chain — never matched, because + * detection ran per-line with no tolerance for the pair spanning more + * than one line. + * The fix: OPERAND_SOURCE (identifier / dotted member / computed access, + * compared as captured TEXT rather than a bare-identifier backreference), + * TYPEOF_TIER_CLAUSE_RE (one clause at a time, tier-order-independent, + * collected via a `Map>`), and a small bounded + * LADDER_WINDOW_LINES sliding window so the two required clauses need only + * sit within a few lines of each other, not on the identical line. All + * three evasion shapes above are now covered by + * `tests/state-field-drift.test.cjs` (D3c/D3d/D3e). + * + * KNOWN, ACCEPTED limits of this scan (honest, not exhaustive by + * construction — this is a bounded regex-based scan, not a parser): + * - Cross-function: a re-derivation whose ladder and fallback call sit in + * two DIFFERENT named functions with no shared enclosing scope (e.g. a + * ladder in one file-level helper, consumed by a caller in another + * function that itself calls `stateExtractField(` for an unrelated + * field) is not caught — this guard's unit is "one named function's own + * body, including its nested closures", not the whole call graph. That + * is left to code review, not this regex. This co-occurrence check is + * deliberately FUNCTION-SCOPED with NO line-distance window at all (see + * the module-level "DETECTION" note above) — do not confuse this with + * LADDER_WINDOW_LINES, which bounds a different, much tighter pairing + * (the ladder's own two clauses, which are always part of the SAME + * conditional expression by construction, not an arbitrary call + * anywhere later in the function). + * - Ladder clauses further apart than LADDER_WINDOW_LINES lines: a ladder + * whose 'number' and 'boolean' clauses are more than + * LADDER_WINDOW_LINES lines apart (e.g. separated by an unrelated + * intervening block of code, not just the couple of lines a single + * conditional or two adjacent `if`s span) is not caught. No such shape + * has been observed in this codebase; if one appears, raise + * LADDER_WINDOW_LINES rather than silently accepting the miss. + * - Non-`typeof`-shaped coercion checks: a ladder rewritten through a + * `switch (typeof v)`, a helper function abstracting the check (e.g. + * `isNumberOrBoolean(v)`), or any comparison operator other than + * `===` (e.g. `typeof v == 'number'`) is not recognised — the pattern + * is `typeof OPERAND === 'TIER'` literally, not "any type-coercion + * test with equivalent runtime behaviour." + * - Nested computed access: `a[b[c]]` is not modelled as a single + * operand — the computed-content class excludes `]`, so nested + * brackets truncate the captured operand at the first `]` rather than + * matching the whole expression. Not observed in this codebase's + * ladders today. + * - Whitespace-sensitive operand identity: `fm[key]` and `fm[ key ]` are + * compared as distinct operand TEXT (no normalisation), so a ladder + * whose two clauses format the same computed access differently could + * under-detect. Not observed in this codebase's ladders today. + */ + +const path = require('node:path'); +const driftScan = require('./lib/drift-scan.cjs'); +const { MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree } = driftScan; + +// A ladder operand: a bare identifier (`v`), a dotted member expression +// (`fm.key`, `fm.a.b`), or a computed/bracket access (`fm[key]`), repeated +// via a single bounded alternation. No nested/overlapping quantifiers: the +// bracket-content class (`[^\]\r\n]{1,80}`) is one bounded character class, +// not a quantifier nested inside another quantifier, so there is nothing +// for a backtracking engine to explore. The captured TEXT (not a bare- +// identifier backreference) is what two clauses are compared against for +// "same operand" — see TYPEOF_TIER_CLAUSE_RE below and buildFunctionInfo's +// ladder-window accumulation. +const OPERAND_SOURCE = String.raw`[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*|\[[^\]\r\n]{1,80}\])*`; + +// One ladder clause: `typeof OPERAND === 'TIER'`, TIER being one of the +// three coercion tiers this derivation ever compares against. Run with the +// `g` flag over a single line's `detect` text so every clause that line +// carries is collected (a single-line `||` chain carries two or three; a +// clause split onto its own line, or written as a standalone `if`, carries +// one). No trailing `\b` after the closing quote: the quote itself is +// already an unambiguous, non-word boundary — see the historical note this +// replaced for why a trailing `\b` there silently matches nothing at all. +const TYPEOF_TIER_CLAUSE_RE = new RegExp(String.raw`\btypeof\s+(${OPERAND_SOURCE})\s*===\s*'(number|boolean|string)'`, 'g'); + +// A ladder is CONFIRMED when the SAME operand carries both a 'number' clause +// and a 'boolean' clause (in either order; an additional 'string' clause +// anywhere among them does not prevent this) within this many consecutive +// lines of each other. Small and bounded, and DELIBERATELY NOT the same +// concept as the ladder-to-`stateExtractField(` co-occurrence check, which +// stays function-scoped with no window at all (see the module header's +// "DETECTION" and "LADDER DETECTION DETAIL" notes for why these two +// pairings are not interchangeable). +const LADDER_WINDOW_LINES = 6; + +// The body-fallback call whose presence, inside a ladder-bearing function, +// makes that function a re-derivation of the "frontmatter-else-body" grammar. +const STATE_EXTRACT_FIELD_CALL_RE = /\bstateExtractField\(/; + +// Named function scope openers. Each requires its own opening `{` on the +// SAME line as the signature — see the header's "FUNCTION ATTRIBUTION" +// paragraph for the documented limitation and why it does not affect either +// real copy this guard was written against. +// - `function NAME(...) {` — top-level OR nested, any indentation. +const FUNCTION_DECL_RE = /\bfunction\s+([A-Za-z_$][\w$]*)\s*\(/; +// - `const NAME = (...): ReturnType => {` — block-bodied arrow assigned to +// a const. `[^)]*` and `[^=]*` are bounded, single-purpose character +// classes (no nested/overlapping quantifiers): the former stops at the +// parameter list's closing paren, the latter at the arrow itself. +const ARROW_CONST_RE = /\bconst\s+([A-Za-z_$][\w$]*)\s*=\s*\([^)]*\)\s*(?::\s*[^=]+)?=>\s*\{/; + +// ADR-3180 Decision 4(d): a guard's scan surface is every AUTHORED surface +// that can EXPRESS the derivation, not `src/` — reading "whole-repo scan" as +// "the whole `src/` tree" is itself the forbidden allowlist, one directory +// wide. This derivation is expressed in TWO languages: TypeScript under +// `src/` (the ladder + `stateExtractField(` shape PASS 1/2 above detect), and +// PROSE in the workflow/command/agent/skill markdown that ships to every +// runtime — `gsd-core/workflows/smart-entry.md`'s "Extract: `status` +// (frontmatter `status:` or body `**Status:**`)" is exactly this chain, +// hand-described rather than called. `SCAN_DIRS` therefore covers both; +// `findPromptFieldDrift` (below `findStateFieldDrift`) is the markdown-side +// detector, dispatched by extension in `scanRepo`'s `onFile`. Mirrors +// `lint-planning-prompt-drift.cjs`'s own prompt-layer surface exactly +// (`gsd-core/workflows`, `commands`, `agents`, `skills`). +const SCAN_DIRS = ['src', 'gsd-core/workflows', 'commands', 'agents', 'skills']; +const SCAN_EXT = new Set(['.cts', '.ts', '.mts', '.md']); + +// The designated owner (issue #3187 Phase 5, ADR-3180 §7.7). +const OWNER_FILE = path.join('src', 'state-document.cts'); + +// Per ADR-3180 Decision 4(a): function-scoped, NEVER a bare file allowlist — +// Decision 4(d) records that a whole-file exemption on the owner is +// precisely how `getMilestoneInfo` stayed invisible to an earlier guard. +// Only `stateFieldValue` itself is exempt: it IS the canonical +// frontmatter-scalar-then-body-field chain (holds the ladder AND calls +// `stateExtractField(` by construction, in its own body — that is its whole +// job), not a copy of it. Every OTHER function in this same file — including +// any future re-derivation added anywhere else in state-document.cts — is +// still scanned and still flagged; nothing else in this file is exempt. +const FUNCTION_SCOPED_EXEMPTIONS = new Map([[OWNER_FILE, new Set(['stateFieldValue'])]]); + +/** + * Tokenize the WHOLE file into TWO parallel per-line views, from ONE + * single-pass, escape-aware character scan (not a regex — no backtracking + * cost to bound): + * - `detect[i]`: comments stripped, but string/template literal contents + * KEPT VERBATIM (quotes included). This is what the detection regexes + * (LADDER_RE / STATE_EXTRACT_FIELD_CALL_RE / FUNCTION_DECL_RE / + * ARROW_CONST_RE) run against — they need the literal quoted text + * `'number'` / `'boolean'` to still be present. + * - `braces[i]`: comments AND string/template literal CONTENTS stripped, + * used only for brace-depth counting, so a brace character written + * inside a string (e.g. `{ label: '{' }`) never perturbs the count. + * + * This REPLACES two earlier, narrower designs in turn: + * 1. Two independent per-line helpers (`stripComments` + a separate + * `stripStringLiterals`), each with no cross-line state. That design + * missed a `/* ... *\/`-style block comment whose CLOSING line also + * carries trailing real code (e.g. a `catch { /* comment` opener + * followed by ` * more comment. *\/ }` on a later line): the old + * per-line `stripComments` heuristic ("a line whose trimmed text starts + * with `*` is entirely comment") blanked that closing line WHOLESALE, + * silently dropping the real `}` it also carried, which left a + * function's brace-depth frame permanently open and made every LATER + * `stateExtractField(` call in the file — inside unrelated functions — + * inherit that stale frame's ladder-bearing status (`src/state.cts`'s + * `cmdStateValidate`, which has no ladder of its own, was falsely + * flagged this way). + * 2. A single merged output that dropped string CONTENTS unconditionally. + * That fixed (1) but broke detection outright: LADDER_RE needs the + * literal text `'number'`/`'boolean'` (with quotes) to match, and a + * merged output that strips quote contents for brace-safety also + * erases the very tokens the ladder regex looks for — every ladder + * line silently stopped matching. Two views, not one, is what lets + * each consumer see what it actually needs. + * + * `inBlockComment` and `inTemplate` are threaded ACROSS lines so a block + * comment or a multi-line backtick template literal spanning several source + * lines is tracked correctly regardless of what trails its closing + * delimiter. Regex literals are NOT specially recognised (documented, + * narrow, known limitation, same as the sibling drift guards' + * `readRegexLiteralAt`-free scans): a `/` is only ever treated as a comment + * opener when immediately followed by another `/` or `*`, so an ordinary + * regex literal's slashes pass through as plain characters and any brace + * inside one (e.g. `/x{2,3}/`) is counted like any other character — + * verified harmless for every regex literal actually present in `src/` + * today because each is balanced (equal opens/closes) on its own line, so it + * never desyncs the running depth total even though the individual + * characters are not "understood" as a literal. + */ +function scanCode(lines) { + const detect = new Array(lines.length); + const braces = new Array(lines.length); + let inBlockComment = false; + let inTemplate = false; + for (let li = 0; li < lines.length; li++) { + const line = lines[li]; + let outDetect = ''; + let outBraces = ''; + let i = 0; + if (inTemplate) { + const start = i; + while (i < line.length) { + if (line[i] === '\\') { + i += 2; + continue; + } + if (line[i] === '`') { + i++; + inTemplate = false; + break; + } + i++; + } + outDetect += line.slice(start, i); // template contents kept verbatim for detect + if (inTemplate) { + detect[li] = outDetect; + braces[li] = ''; // whole line still inside the unterminated template + continue; + } + } + while (i < line.length) { + if (inBlockComment) { + const close = line.indexOf('*/', i); + if (close === -1) { + i = line.length; + break; + } + i = close + 2; + inBlockComment = false; + continue; + } + const ch = line[i]; + if (ch === '/' && line[i + 1] === '/') { + i = line.length; // rest of line is a line comment + break; + } + if (ch === '/' && line[i + 1] === '*') { + inBlockComment = true; + i += 2; + continue; + } + if (ch === "'" || ch === '"') { + const quote = ch; + const start = i; + let j = i + 1; + while (j < line.length) { + if (line[j] === '\\') { + j += 2; // escape consumes the next character, whatever it is + continue; + } + if (line[j] === quote) { + j++; + break; + } + j++; + } + outDetect += line.slice(start, j); // string kept verbatim for detect + // (nothing appended to outBraces — string contents excluded from depth counting) + i = j; + continue; + } + if (ch === '`') { + const start = i; + let j = i + 1; + let closed = false; + while (j < line.length) { + if (line[j] === '\\') { + j += 2; + continue; + } + if (line[j] === '`') { + j++; + closed = true; + break; + } + j++; + } + if (!closed) { + outDetect += line.slice(start); // rest of line kept verbatim for detect + inTemplate = true; + i = line.length; + break; + } + outDetect += line.slice(start, j); // template kept verbatim for detect + i = j; + continue; + } + outDetect += ch; + outBraces += ch; + i++; + } + detect[li] = outDetect; + braces[li] = outBraces; + } + return { detect, braces }; +} + +/** + * Pure: find every unsanctioned STATE.md field-extraction fallback-chain + * re-derivation in `text`. `relPath` is the repo-relative path, used both to + * report file:line and to apply the function-scoped owner exemptions above. + * + * Two passes over the same line array: + * PASS 1 walks the file once, maintaining a brace-depth stack of open + * named function frames, and records which function NAMES are + * ladder-bearing (transitively — see the header's "TRANSITIVE ladder + * attribution" paragraph) and, per source line, which frame is + * INNERMOST at that line (for call attribution) — {@link buildFunctionInfo}. + * PASS 2 walks the lines again; any `stateExtractField(` call whose + * innermost enclosing function is ladder-bearing (and not + * function-scoped-exempt) is a violation. + * Returns [{ line, found }]. + */ +function buildFunctionInfo(lines) { + const { detect, braces } = scanCode(lines); + const ladderBearing = new Set(); + const innermostAt = new Array(lines.length).fill(null); + const stack = []; // { name, openDepth } + let depth = 0; + // A `function NAME(` match whose own line carries no `{` (a multi-line + // signature — parameters and/or a return-type annotation spilling onto + // later lines before the body opens) — awaiting the line that actually + // carries its opening brace. See the header's "FUNCTION ATTRIBUTION" + // paragraph and this function's own doc comment for why this cannot be + // pushed onto `stack` immediately: pushing it with `openDepth` recorded + // BEFORE its own `{` is counted produces a frame whose `openDepth` is the + // ENCLOSING scope's depth, not its own — for a top-level function that is + // `openDepth: 0`, and since real code never reaches negative depth, a + // frame pushed with `openDepth: 0` can NEVER pop (`depth < 0` never + // fires): it would sit at the bottom of `stack` for the rest of the file, + // and every subsequent line with no OTHER open frame would be + // misattributed to it. `ARROW_CONST_RE` never needs this deferral — its + // pattern requires `=>\s*\{` literally, so it only ever matches on a line + // that already carries the brace. + let pendingDeclName = null; + // LADDER WINDOW: a rolling buffer of every `typeof OPERAND === 'TIER'` + // clause seen in the last LADDER_WINDOW_LINES lines, oldest-first. On + // each line, new clauses from that line are appended, then entries older + // than the window are pruned from the front. A ladder is confirmed the + // moment the SAME operand has accumulated both a 'number' and a + // 'boolean' entry inside the buffer — order-independent, and regardless + // of whether the two clauses came from one `||` chain split across + // lines, one single-line chain, or two entirely separate `if` + // statements. See LADDER_WINDOW_LINES's own comment for why this is + // bounded small and is NOT the same concept as the (unbounded, + // function-scoped) ladder-to-`stateExtractField(` co-occurrence below. + const recentClauses = []; // { line, operand, tier } + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + const braceCode = braces[i]; + + let immediateName = null; + if (detectCode.trim()) { + const arrowMatch = ARROW_CONST_RE.exec(detectCode); + if (arrowMatch) { + immediateName = arrowMatch[1]; + } else { + const declMatch = FUNCTION_DECL_RE.exec(detectCode); + // A NEW `function NAME(` match replaces any still-pending name + // rather than stacking deferrals — this scanner tracks at most one + // pending declaration at a time (two `function` keywords on + // consecutive lines with neither closing its signature first is not + // a shape that occurs in this codebase's style). + if (declMatch) pendingDeclName = declMatch[1]; + } + } + + const opens = (braceCode.match(/\{/g) || []).length; + const closes = (braceCode.match(/\}/g) || []).length; + depth += opens - closes; + + if (immediateName) stack.push({ name: immediateName, openDepth: depth }); + + if (pendingDeclName) { + if (opens > 0) { + // First line whose brace count actually increases — its `{` is + // counted in THIS line's `opens`, so `depth` here correctly + // reflects "inside the function", matching every other frame's + // convention (push AFTER updating depth for the pushing line). + stack.push({ name: pendingDeclName, openDepth: depth }); + pendingDeclName = null; + } else if (detectCode.includes(';')) { + // The statement terminated before any `{` appeared — a type-only + // declaration, an ambient `declare function`, or an overload + // signature, none of which open a function body. Abandon the + // pending name rather than stranding it to match a `{` that + // belongs to unrelated later code. (A `;` inside a string literal + // on this line would also trigger this — accepted, narrow, known + // limitation: a default-parameter string containing `;` on the + // SAME line as an unterminated multi-line signature has not been + // observed in this codebase.) + pendingDeclName = null; + } + } + + while (stack.length > 0 && depth < stack[stack.length - 1].openDepth) stack.pop(); + + innermostAt[i] = stack.length > 0 ? stack[stack.length - 1].name : null; + + if (detectCode.trim()) { + TYPEOF_TIER_CLAUSE_RE.lastIndex = 0; + let clauseMatch; + while ((clauseMatch = TYPEOF_TIER_CLAUSE_RE.exec(detectCode)) !== null) { + recentClauses.push({ line: i, operand: clauseMatch[1], tier: clauseMatch[2] }); + } + } + // Prune clauses that have fallen outside the small bounded ladder window. + while (recentClauses.length > 0 && recentClauses[0].line <= i - LADDER_WINDOW_LINES) { + recentClauses.shift(); + } + + let ladderConfirmed = false; + if (recentClauses.length > 0) { + const tiersByOperand = new Map(); + for (const clause of recentClauses) { + let tiers = tiersByOperand.get(clause.operand); + if (!tiers) { + tiers = new Set(); + tiersByOperand.set(clause.operand, tiers); + } + tiers.add(clause.tier); + } + for (const tiers of tiersByOperand.values()) { + if (tiers.has('number') && tiers.has('boolean')) { + ladderConfirmed = true; + break; + } + } + } + + if (ladderConfirmed) { + // Transitive: every frame currently open (not just the innermost) is + // ladder-bearing, because a nested closure's body is lexically part of + // every one of its enclosing functions' own bodies. + for (const frame of stack) ladderBearing.add(frame.name); + } + } + return { ladderBearing, innermostAt, detect }; +} + +// ─── PROMPT-LAYER PROSE DETECTION (ADR-3180 Decision 4(d)) ───────────────── +// +// The SAME #1760 fallback chain — "prefer the frontmatter scalar, else fall +// back to the body field" — expressed as MARKDOWN PROSE describing the +// derivation to an agent, rather than TypeScript re-deriving it. Detection is +// intentionally narrow, mirroring `lint-planning-prompt-drift.cjs`'s own +// precedent: a markdown line is a prose re-derivation only when it carries +// ALL FOUR, on the SAME line: +// (a) a backtick-quoted YAML-style frontmatter key token — a lowercase +// identifier immediately followed by a colon, inside backticks, e.g. +// `` `status:` ``. `FRONTMATTER_KEY_TOKEN_RE`. +// (b) a backtick-quoted Markdown BOLD body-field token, e.g. +// `` `**Status:**` ``. `BODY_BOLD_TOKEN_RE`. +// (c) the word "frontmatter" (case-insensitive, word-bounded) anywhere on +// the line. +// (d) the word "body" (case-insensitive, word-bounded) anywhere on the +// line. +// (c) and (d) are what distinguish a line DESCRIBING an fm-then-body +// PRECEDENCE from a line that merely happens to carry two backtick-quoted +// tokens shaped like (a) and (b) for unrelated reasons — both words are +// required so an incidental co-occurrence (e.g. a table row naming an +// unrelated frontmatter key and, several columns over, an unrelated bold +// body label) cannot false-positive on tokens alone. +// All four regexes are small, bounded, single fixed character classes with +// no nested/overlapping quantifiers — nothing for a backtracking engine to +// explore (`npm run lint:ci` runs CodeQL js/redos over this repo, the same +// discipline `findStateFieldDrift`'s own regexes document above). +const FRONTMATTER_KEY_TOKEN_RE = /`[a-z][a-z0-9_]*:`/; +const BODY_BOLD_TOKEN_RE = /`\*\*[^*`]{1,80}\*\*`/; +const FRONTMATTER_WORD_RE = /\bfrontmatter\b/i; +const BODY_WORD_RE = /\bbody\b/i; + +// Per ADR-3180 Decision 4(d)/(e): `gsd-core/workflows/smart-entry.md`'s +// Fallback step 1 ("`gsd-tools` itself is broken") is a PERMANENT, by- +// construction exemption — NOT debt with an owner, and therefore NOT +// modelled as `lint-planning-prompt-drift.cjs`'s shrink-only ratchet +// baseline (which exists specifically to acknowledge sites with a removal +// issue, per Decision 4(e)). This site can never be migrated onto +// `src/state-document.cjs`'s `stateFieldValue`: the surrounding step exists +// PRECISELY for when the CLI itself cannot run (`Cannot find module ...` / +// Node crash — probed one line above this one), so by construction it cannot +// call the canonical owner it is standing in for. Fabricating a "removal +// issue" for something that can never be removed would misrepresent it as +// ratchetable debt it is not. +// +// Keyed on (file, TRIMMED source text) — never a line number, which churns +// on any unrelated edit to the same file — mirroring the ratchet's own key +// shape (`lint-planning-prompt-drift.cjs`) even though this exemption is not +// itself a ratchet. `file` uses the SAME native-separator relPath shape as +// `OWNER_FILE`/`FUNCTION_SCOPED_EXEMPTIONS` above (this guard does not +// POSIX-normalize elsewhere, so this exemption does not either, for +// consistency within the one file). +const PROMPT_LAYER_EXEMPTIONS = new Map([ + [ + path.join('gsd-core', 'workflows', 'smart-entry.md'), + new Map([ + [ + "- Read `.planning/STATE.md` (frontmatter + body) with the Read tool. Extract: `status` (frontmatter `status:` or body `**Status:**`), `Phase:` from the body, `total_phases`/`percent` from a nested `progress:` frontmatter object if present, and any `## Blockers` items.", + 'gsd-tools-down fallback (smart-entry.md Fallback step 1): runs only when gsd-tools itself cannot run, so it cannot call the canonical owner it substitutes for — permanent by construction, not removable debt.', + ], + ]), + ], +]); + +/** + * Pure: find every prose re-derivation of the STATE.md field-extraction + * fallback chain in a markdown file's `text`. Returns `[{ line, found }]` for + * every UNEXEMPTED match — `relPath` (native-separator, matching + * `PROMPT_LAYER_EXEMPTIONS`'s keys) is consulted only for the exemption + * lookup, exactly as `findStateFieldDrift` consults it for + * `FUNCTION_SCOPED_EXEMPTIONS`. + */ +function findPromptFieldDrift(text, relPath) { + const out = []; + const lines = text.split('\n'); + const exemptTexts = PROMPT_LAYER_EXEMPTIONS.get(relPath) || null; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + if (!FRONTMATTER_KEY_TOKEN_RE.test(line)) continue; + if (!BODY_BOLD_TOKEN_RE.test(line)) continue; + if (!FRONTMATTER_WORD_RE.test(line)) continue; + if (!BODY_WORD_RE.test(line)) continue; + const trimmed = line.trim(); + if (exemptTexts && exemptTexts.has(trimmed)) continue; + out.push({ line: i + 1, found: trimmed.slice(0, MAX_REGEX_LITERAL_LEN) }); + } + return out; +} + +function findStateFieldDrift(text, relPath) { + const out = []; + const lines = text.split('\n'); + const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null; + const { ladderBearing, innermostAt, detect } = buildFunctionInfo(lines); + + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + if (!STATE_EXTRACT_FIELD_CALL_RE.test(detectCode)) continue; + + const fn = innermostAt[i]; + if (!fn || !ladderBearing.has(fn)) continue; + if (exemptFunctions && exemptFunctions.has(fn)) continue; + + out.push({ line: i + 1, found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN) }); + } + return out; +} + +/** + * Scan the authored source tree (TypeScript under `src/`, prose in the + * prompt-layer markdown — ADR-3180 Decision 4(d)) and return every + * unsanctioned re-derivation, each annotated with the repo-relative file + * path. Dispatches by extension: `.md` files run the prose detector + * (`findPromptFieldDrift`), everything else (`.cts`/`.ts`/`.mts`) runs the + * code detector (`findStateFieldDrift`) — the two derivations are expressed + * in different languages and need different detection shapes over the same + * scan surface. + */ +function scanRepo(root) { + return scanTree({ + root, + scanDirs: SCAN_DIRS, + scanExt: SCAN_EXT, + onFile(rel, text) { + // `rel` is already the REAL (canonical) path (scanTree resolves + // symlinks before calling onFile), so this — and + // FUNCTION_SCOPED_EXEMPTIONS/PROMPT_LAYER_EXEMPTIONS above, also keyed + // on `rel` — match consistently regardless of which symlink reached + // the file. + const finder = path.extname(rel) === '.md' ? findPromptFieldDrift : findStateFieldDrift; + return finder(text, rel).map((d) => ({ file: rel, ...d })); + }, + }); +} + +function main() { + const root = path.join(__dirname, '..'); + const violations = scanRepo(root); + if (violations.length === 0) { + process.stdout.write('ok state-field-drift: no unsanctioned STATE.md field-extraction fallback-chain re-derivations found\n'); + return; + } + process.stderr.write('state-field-drift: independent re-derivation(s) of the STATE.md frontmatter-else-body field\n'); + process.stderr.write('fallback chain found. Route these call sites through src/state-document.cjs\n'); + process.stderr.write('`stateFieldValue` (issue #3187, ADR-3180 §7.7) instead of re-deriving the ladder:\n'); + for (const d of violations) { + // `d.file` is exactly as attacker-controlled as `d.found`: a repo can + // legally track a filename containing control bytes / bidi overrides, + // and it is a fork-PR-authored value reaching a CI log the same way the + // matched line text does — sanitize it at the same reporting boundary. + process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${sanitizeForReport(d.found)}\n`); + } + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { + findStateFieldDrift, + findPromptFieldDrift, + buildFunctionInfo, + scanRepo, + OPERAND_SOURCE, + TYPEOF_TIER_CLAUSE_RE, + LADDER_WINDOW_LINES, + STATE_EXTRACT_FIELD_CALL_RE, + FUNCTION_DECL_RE, + ARROW_CONST_RE, + OWNER_FILE, + FUNCTION_SCOPED_EXEMPTIONS, + FRONTMATTER_KEY_TOKEN_RE, + BODY_BOLD_TOKEN_RE, + PROMPT_LAYER_EXEMPTIONS, + SCAN_DIRS, + SCAN_EXT, + scanCode, + MAX_REGEX_LITERAL_LEN, +}; diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index 0191abba9..15b204d40 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -74,6 +74,7 @@ "files": [ "state-acquirestatelock-non-eexist.test.cjs", "state-command-cutover.test.cjs", + "state-field-drift.test.cjs", "state-prune.test.cjs", "state-rebuild-cli.test.cjs", "state-rebuild.test.cjs", diff --git a/src/smart-entry.cts b/src/smart-entry.cts index 20a81c711..dd5a54a1a 100644 --- a/src/smart-entry.cts +++ b/src/smart-entry.cts @@ -47,7 +47,7 @@ import phaseLifecycle = require('./phase-lifecycle.cjs'); const { deriveProgressFromRoadmap } = phaseLifecycle; // eslint-disable-next-line @typescript-eslint/no-require-imports import stateDocument = require('./state-document.cjs'); -const { stateExtractField } = stateDocument; +const { stateFieldValue } = stateDocument; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseId = require('./phase-id.cjs'); const { comparePhaseNum, extractPhaseToken, normalizePhaseName, phaseTokenMatches } = phaseId; @@ -140,14 +140,6 @@ export const SITUATIONS: readonly Situation[] = Object.freeze([ // ─── Detection ───────────────────────────────────────────────────────────────── -/** Frontmatter scalar helper: prefer YAML frontmatter, fall back to body field. */ -function fmScalar(fm: Record, body: string, key: string, bodyField: string): string | null { - const v = fm[key]; - if (typeof v === 'string' && v.trim()) return v.trim(); - if (typeof v === 'number' || typeof v === 'boolean') return String(v); - return stateExtractField(body, bodyField); -} - /** Read a scalar value from a nested frontmatter object (e.g. progress.total_phases). */ function fmScalarKey(obj: unknown, key: string): string | null { if (!obj || typeof obj !== 'object') return null; @@ -324,26 +316,34 @@ export function detectSignals(cwd: string, now: () => number = Date.now): SmartE // - body prose: `Phase: 3`, `**Status:** verifying`, `Total Phases: 5` // Read each field across every form, scalar-first then nested then body, so // the classifier works on real STATE.md files written by current GSD. - const statusRaw = fmScalar(fm, body, 'status', 'Status'); - const pausedAtRaw = fmScalar(fm, body, 'paused_at', 'Paused At'); - const lastActivityRaw = fmScalar(fm, body, 'last_activity', 'Last Activity'); + const statusRaw = stateFieldValue(fm, body, 'status', 'Status').value; + const pausedAtRaw = stateFieldValue(fm, body, 'paused_at', 'Paused At').value; + const lastActivityRaw = stateFieldValue(fm, body, 'last_activity', 'Last Activity').value; // current_phase: scalar fm → nested (none) → body "Current Phase" → body "Phase". // The body `Phase:` field is the canonical location in prose-form STATE.md // (e.g. "Phase: 3" or "Phase: 3 — ui-review"); parse the leading number. + // #3187 / ADR-3180 Amendment 3 ("0.x split"): this read is DELIBERATELY + // unscoped — smart-entry classifies `gsd next` routing over the whole body, + // whereas state.cts's copies of this same field scope it to `## Current + // Position` (#1776/#2956). Those are two different questions sharing a + // name; folding the scoped read in here would silently change smart-entry's + // routing, an undisclosed Tier-2 change (design's Rejected #3). Both owner + // calls below intentionally pass the unscoped `body`, never a Current- + // Position slice. const currentPhaseRaw = - fmScalar(fm, body, 'current_phase', 'Current Phase') ?? - stateExtractField(body, 'Phase'); + stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? + stateFieldValue(fm, body, null, 'Phase').value; // total_phases & percent: nested `progress:` object takes precedence in the // nested schema; scalar fm / body fields cover the flat schema. const progressFm = typeof fm.progress === 'object' ? fm.progress : null; const totalPhasesRaw: string | null = fmScalarKey(progressFm, 'total_phases') ?? - fmScalar(fm, body, 'total_phases', 'Total Phases'); + stateFieldValue(fm, body, 'total_phases', 'Total Phases').value; const progressRaw: string | null = fmScalarKey(progressFm, 'percent') ?? - fmScalar(fm, body, 'progress', 'Progress'); + stateFieldValue(fm, body, 'progress', 'Progress').value; // Blockers list: `- ` items under a `## Blockers` heading. const blockers: string[] = []; diff --git a/src/state-document.cts b/src/state-document.cts index 4363b85a8..9be0633f6 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -11,6 +11,10 @@ import { splitTableRow } from './markdown-table.cjs'; import { clampPercentFromFraction } from './phase-lifecycle.cjs'; import { collectSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // Internal helpers function escapeRegex(str: string): string { @@ -234,6 +238,80 @@ export function stateExtractField(content: string, fieldName: string): string | return null; } +/** + * Single owner of the #1760 STATE.md field-extraction fallback chain: "read + * field F, preferring the YAML frontmatter scalar, falling back to the body + * field." Added for #3187 (epic #3180, ADR-3180 §7.7) to collapse three + * independent re-derivations of this chain — `src/smart-entry.cts`'s + * `fmScalar` closure, and `src/state.cts`'s `cmdStateSnapshot` and + * `cmdStatePrune` — onto one function, per ADR-3180 Decision 1 ("keep N + * copies with a parity test" is rejected: a parity test proves today's + * agreement, not that copy N+1 won't happen). + * + * Takes ALREADY-PARSED `fm` and `body` rather than raw STATE.md content: the + * heaviest caller, `cmdStateSnapshot`, reads roughly ten fields off one parse + * and must not re-parse frontmatter per field. + * + * `stateExtractField` (above) is deliberately left untouched — it has 20 + * direct callers and a CRITICAL blast radius (ADR-3180 §7.7's Rejected #1) — + * so this function is additive: it calls `stateExtractField` rather than + * replacing it or changing its signature. + * + * Fallback ladder (unchanged from every prior copy this replaces): + * 1. `fm[fmKey]` is a non-empty (post-`.trim()`) string → that trimmed + * string. + * 2. `fm[fmKey]` is a `number` or `boolean` → `String(fm[fmKey])`, so `0` + * and `false` are VALUES, not absence. + * 3. Anything else (`null`, `undefined`, an object, an array, or an + * empty/whitespace-only string) → fall through to + * `stateExtractField(body, bodyField)`. + * + * `fmKey === null` skips steps 1–2 outright: for a caller whose chain has no + * frontmatter side for this particular field (e.g. `state.cts`'s body-only + * `Last Activity` / `Last activity` case-variant pair, which sits inside a + * function that DOES own a ladder for its other fields, so per this phase's + * function-scoped guard it must still route through this owner). + * `bodyField === null` skips step 3: for a caller whose "no frontmatter + * value" case falls through to an already-computed value instead of a fresh + * extractor call (e.g. `cmdStateSnapshot`'s `last_activity`, which falls to + * its already-parsed prose date rather than re-extracting the body). + * + * `scope` reports whether the chain ran over inputs it could actually + * consult (ADR-3180 Decision 2/§7.7 — mirrors `scanPhasePlans`'s + * scope-carrying result in `plan-scan.cts`; see `planning-scope.cjs`). This + * function's own ladder always runs to completion on whatever `fm`/`body` it + * is given, INCLUDING when the answer is `null` — a genuinely absent field is + * a real answer, not a failure to look (§7.7 behavior table row 4). So + * `scope` defaults to `SCOPE.COMPLETE` and is only ever something else when + * the CALLER passes `opts.scope`, because only the caller knows whether an + * input it handed in was itself degraded — e.g. `fm` came back `{}` from an + * unterminated frontmatter fence (`extractFrontmatter` swallows that parse + * failure), or `body` is an unscoped whole-document fallback because a + * required `## Current Position` section was not found (#2956). This + * function never invents a new `SCOPE` member — the enum is frozen at + * COMPLETE/TRUNCATED/UNSCOPED/UNREADABLE (`planning-scope.cjs`). + * + * #1760 is the fallback chain's origin. + */ +export function stateFieldValue( + fm: Record, + body: string, + fmKey: string | null, + bodyField: string | null, + opts?: { scope?: Scope }, +): { value: string | null; scope: Scope } { + const v = fmKey === null ? undefined : fm[fmKey]; + let value: string | null; + if (typeof v === 'string' && v.trim()) { + value = v.trim(); + } else if (typeof v === 'number' || typeof v === 'boolean') { + value = String(v); + } else { + value = bodyField === null ? null : stateExtractField(body, bodyField); + } + return { value, scope: opts?.scope ?? SCOPE.COMPLETE }; +} + /** * Match the "Current Position" section body from a STATE.md body. #2956: this * is the Phase analogue of state.cts's matchSessionSection. `Phase` canonically diff --git a/src/state.cts b/src/state.cts index 96e3fbb43..559e4c886 100644 --- a/src/state.cts +++ b/src/state.cts @@ -49,6 +49,7 @@ import { normalizeStateStatus, shouldPreserveExistingProgress, stateExtractField, + stateFieldValue, stateReplaceField, KNOWN_TEMPLATE_DEFAULTS, stateReplaceFieldIfTemplate, @@ -1456,18 +1457,9 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { const fm = extractFrontmatter(content, statePath) as Record; const body = stripFrontmatter(content); - // Helper: return frontmatter scalar value when present and non-empty. - // Accepts strings, numbers, and booleans — coercing non-string primitives to - // their string representation so callers always receive string | null. - // Returns null for missing, null/undefined, or empty-after-trim values so - // the caller falls back to body extraction. - const fmScalar = (key: string): string | null => { - const v = fm[key]; - if (v === null || v === undefined) return null; - if (typeof v === 'string') return v.trim() || null; - if (typeof v === 'number' || typeof v === 'boolean') return String(v); - return null; - }; + // #3187: frontmatter-scalar-then-body-field precedence is owned by + // state-document.cjs's `stateFieldValue` (ADR-3180 §7.7) — this function no + // longer holds its own fmScalar ladder. // Extract basic fields — frontmatter keys take precedence over body // #2956: scope `Phase` extraction to ## Current Position so a historical @@ -1477,24 +1469,24 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { // full-body search only when no ## Current Position section exists, so files // with no section heading keep their current behaviour. const currentPositionScope = matchCurrentPositionSection(body) ?? body; - const prosePhase = parseProsePhaseField(stateExtractField(currentPositionScope, 'Phase')); - const currentPhase = fmScalar('current_phase') ?? stateExtractField(body, 'Current Phase') ?? prosePhase.phase; - const currentPhaseName = fmScalar('current_phase_name') ?? stateExtractField(body, 'Current Phase Name') ?? prosePhase.name; - const totalPhasesRaw = fmScalar('total_phases') ?? stateExtractField(body, 'Total Phases'); - const currentPlan = fmScalar('current_plan') ?? stateExtractField(body, 'Current Plan'); - const totalPlansRaw = fmScalar('total_plans_in_phase') ?? stateExtractField(body, 'Total Plans in Phase'); - const status = fmScalar('status') ?? stateExtractField(body, 'Status'); - const progressRaw = fmScalar('progress') ?? stateExtractField(body, 'Progress'); - const rawLastActivity = stateExtractField(body, 'Last Activity') ?? stateExtractField(body, 'Last activity'); + const prosePhase = parseProsePhaseField(stateFieldValue(fm, currentPositionScope, null, 'Phase').value); + const currentPhase = stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? prosePhase.phase; + const currentPhaseName = stateFieldValue(fm, body, 'current_phase_name', 'Current Phase Name').value ?? prosePhase.name; + const totalPhasesRaw = stateFieldValue(fm, body, 'total_phases', 'Total Phases').value; + const currentPlan = stateFieldValue(fm, body, 'current_plan', 'Current Plan').value; + const totalPlansRaw = stateFieldValue(fm, body, 'total_plans_in_phase', 'Total Plans in Phase').value; + const status = stateFieldValue(fm, body, 'status', 'Status').value; + const progressRaw = stateFieldValue(fm, body, 'progress', 'Progress').value; + const rawLastActivity = stateFieldValue(fm, body, null, 'Last Activity').value ?? stateFieldValue(fm, body, null, 'Last activity').value; const proseLastActivity = parseProseLastActivityField(rawLastActivity); - const lastActivity = fmScalar('last_activity') ?? proseLastActivity.date ?? rawLastActivity; - const lastActivityDesc = fmScalar('last_activity_desc') ?? stateExtractField(body, 'Last Activity Description') ?? proseLastActivity.description; + const lastActivity = stateFieldValue(fm, body, 'last_activity', null).value ?? proseLastActivity.date ?? rawLastActivity; + const lastActivityDesc = stateFieldValue(fm, body, 'last_activity_desc', 'Last Activity Description').value ?? proseLastActivity.description; // #2956: Paused At canonically lives in ## Session (see the comment above // preferNewerLastActivity and the write seam in buildStateFrontmatter). The // write seam already scopes it to ## Session; this read seam must agree, so a // stale "Paused At:" in a Session Continuity Archive cannot win here either. const sessionScope = matchSessionSection(body) ?? body; - const pausedAt = fmScalar('paused_at') ?? stateExtractField(sessionScope, 'Paused At'); + const pausedAt = stateFieldValue(fm, sessionScope, 'paused_at', 'Paused At').value; // Parse numeric fields const totalPhases = totalPhasesRaw ? parseInt(totalPhasesRaw, 10) : null; @@ -2843,8 +2835,84 @@ function cmdStateMilestoneSwitch(cwd: string, version: string | undefined, name: /** * Gate 1: Validate STATE.md against filesystem. - * Returns { valid, warnings, drift } JSON. + * Returns { valid, warnings, drift, scope } JSON. + * + * #3187 (ADR-3180 §7.7, Decisions 2-4): two defects fixed here. + * + * (1) #3162 THE HEADLINE. Every warning this function can emit used to be + * gated behind `if (currentPhase && fs.existsSync(phasesDir))`, and + * `currentPhase` came from a body-only `stateExtractField(content, 'Current + * Phase')` call with no frontmatter fallback. A STATE.md whose phase lives + * ONLY in frontmatter therefore resolved `currentPhase` to `null`, the whole + * drift block was skipped, and the function returned + * `{valid:true, warnings:[], drift:{}}` — "could not look" was + * output-identical to "looked, all clean." Current Phase / Status / Total + * Plans in Phase now route through `stateFieldValue` (the single owner of the + * #1760 frontmatter-then-body fallback chain), so the frontmatter tier is + * actually consulted. + * + * (2) #1255 FRONTMATTER SHADOWING. The old code passed UNSTRIPPED `content` + * to the extractor. `stateExtractField`'s plain-format branch is + * `^Field:` with the `i` flag, so a frontmatter `status:` key matched the + * pattern for the body field `Status` and won, because the frontmatter block + * precedes the body. Parsed once now — `extractFrontmatter` + + * `stripFrontmatter` — and `fm`/`body` are handed to the chain owner, exactly + * as `advancePlanCore`/`beginPhaseCore`/`completePhaseCore`/ + * `readModifyWriteStateMd` already guard against this class of defect. + * + * `scope` (ADR-3180 Decision 2) reports whether the derivation actually ran: + * - `COMPLETE` — the phase-vs-disk derivation ran over usable input, + * including when it legitimately finds no VERIFICATION.md / no matching + * phase directory (a real answer, not a non-answer). + * - `UNSCOPED` — Current Phase could not be resolved by ANY chain step (no + * frontmatter scalar, no body field), so the drift derivation had no + * phase to scope its disk lookup to and could not run at all. Reporting + * this as COMPLETE would recreate the #3162 collapse this phase closes, + * one layer out. + * - `UNREADABLE` — the frontmatter parse or the phases-dir scan itself + * could not be consulted (an existing `catch` block used to swallow this + * silently; the degrade stays, but is now visible). + * + * ⛔ Rejected (ADR-3180 §7.7 Rejected #2): a non-`COMPLETE` scope is never + * routed to `valid:false`. `valid` keeps meaning "no drift warnings were + * found"; `scope` says whether the derivation could actually run. A caller + * branches on both — folding them into one boolean recreates the exact + * collapse this epic removes, in the opposite direction (a legacy STATE.md + * with no resolvable phase is a supported degrade, not an invalid document). */ + +/** + * #1255/#3187: parse frontmatter and strip it from the body ONCE, shared by + * `cmdStateValidate` and `cmdStateCompletePhase` so both consult the identical + * fm/body precedence and degrade identically when the frontmatter half of the + * chain cannot be consulted. Extracted (code-review finding, epic #3180): the + * two call sites previously carried a byte-identical try/catch, comments + * included — an epic whose own thesis is "one canonical owner per + * derivation" must not ship a duplicated derivation in its own diff. + * + * Returns `scope: SCOPE.COMPLETE` unless the frontmatter parse itself threw, + * in which case `fm` degrades to `{}` and `scope` becomes `SCOPE.UNREADABLE` + * — callers that mutate `scope` further (e.g. `cmdStateValidate`'s later + * UNSCOPED/disk-scan degrades) start from this returned value rather than a + * fresh `SCOPE.COMPLETE`. + */ +function readStateFrontmatterScoped(content: string, statePath: string): { fm: Record; body: string; scope: planningScopeMod.Scope } { + let fm: Record; + let scope: planningScopeMod.Scope = SCOPE.COMPLETE; + try { + fm = extractFrontmatter(content, statePath); + } catch { + // extractFrontmatter is documented never to throw, but this mirrors the + // defensive try/catch already used around it elsewhere in this file + // (e.g. spliceFrontmatter) — a parse hiccup here means the frontmatter + // half of the chain could not be consulted; degrade visibly. + fm = {}; + scope = SCOPE.UNREADABLE; + } + const body = stripFrontmatter(content); + return { fm, body, scope }; +} + function cmdStateValidate(cwd: string, raw: boolean): void { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { @@ -2864,22 +2932,36 @@ function cmdStateValidate(cwd: string, raw: boolean): void { const warnings: string[] = []; const drift: Record = {}; - const status = stateExtractField(content, 'Status') || ''; - const currentPhase = stateExtractField(content, 'Current Phase'); - const totalPlansRaw = stateExtractField(content, 'Total Plans in Phase'); + // #1255/#3187: parse frontmatter and strip it from the body ONCE, so the + // chain owner sees the same fm/body precedence every other migrated call + // site sees. Pass statePath so a truncated STATE.md is named in the #1882 + // diagnostic rather than reported under a content digest. + const { fm, body, scope: initialScope } = readStateFrontmatterScoped(content, statePath); + let scope: planningScopeMod.Scope = initialScope; + + const status = stateFieldValue(fm, body, 'status', 'Status').value || ''; + const currentPhase = stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + const totalPlansRaw = stateFieldValue(fm, body, 'total_plans_in_phase', 'Total Plans in Phase').value; const totalPlansInPhase = totalPlansRaw ? parseInt(totalPlansRaw, 10) : null; const phasesDir = planningPaths(cwd).phases; - // Scan disk for current phase - if (currentPhase && fs.existsSync(phasesDir)) { + if (currentPhase === null) { + // #3162: nothing to scope the disk lookup to — the derivation cannot run + // at all. Distinct from "ran, found nothing to warn about" (scope stays + // COMPLETE elsewhere in this function). + if (scope === SCOPE.COMPLETE) scope = SCOPE.UNSCOPED; + } else if (fs.existsSync(phasesDir)) { const normalized = currentPhase.replace(/\s+of\s+\d+.*/, '').trim(); try { const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); const phaseDir = entries.find(e => e.isDirectory() && e.name.startsWith(normalized.replace(/^0+/, '').padStart(2, '0'))); if (phaseDir) { const phaseDirPath = path.join(phasesDir, phaseDir.name); - const { planCount: diskPlans, summaryCount: diskSummaries } = scanPhasePlans(phaseDirPath); + const { planCount: diskPlans, summaryCount: diskSummaries, scope: planScanScope } = scanPhasePlans(phaseDirPath); + if (planScanScope !== SCOPE.COMPLETE && scope === SCOPE.COMPLETE) { + scope = planScanScope; + } // Check plan count mismatch if (totalPlansInPhase !== null && diskPlans !== totalPlansInPhase) { @@ -2900,7 +2982,8 @@ function cmdStateValidate(cwd: string, raw: boolean): void { } catch { /* best-effort (#2245 audit): cmdStateValidate is a diagnostic * warnings scan across N VERIFICATION.md files — one unreadable file * (permission/race) must not abort the scan of the rest; it's simply - * excluded from drift detection. */ } + * excluded from drift detection. Does not degrade `scope` — the other + * N-1 files were consulted fine. */ } } // Check if all plans have summaries but status still says executing @@ -2911,17 +2994,22 @@ function cmdStateValidate(cwd: string, raw: boolean): void { } } } - } catch { /* best-effort (#2245 audit): cmdStateValidate is a read-only - * diagnostic scan of the current phase's directory (readdirSync + - * scanPhasePlans). A disk-scan failure here means drift detection for - * this phase is skipped for this run, degrading to "no warnings from - * that scan" rather than crashing the validate command — the same - * degrade-on-scan-failure pattern buildStateFrontmatter's own disk scan - * already uses. */ } + // else: phase resolved, but no matching directory on disk — a real + // answer (row 22), not a look failure. scope stays COMPLETE. + } catch { + // #3162/#2245: previously a silent best-effort swallow. The disk-scan + // failure (readdirSync/scanPhasePlans) means drift detection for this + // phase could not run this pass — that degrade is kept (validate still + // does not crash), but is now visible via `scope` instead of being + // indistinguishable from a clean pass. + scope = SCOPE.UNREADABLE; + } } + // else: phasesDir itself does not exist — a real answer (no phases on disk + // yet), not a look failure. scope stays COMPLETE. const valid = warnings.length === 0; - output({ valid, warnings, drift }, raw, undefined); + output({ valid, warnings, drift, scope }, raw, undefined); } /** @@ -3113,18 +3201,16 @@ function cmdStatePrune(cwd: string, options: StatePruneOptions, raw: boolean): v const rawState = fs.readFileSync(statePath, 'utf-8'); const fm = extractFrontmatter(rawState, statePath) as Record; const body = stripFrontmatter(rawState); - // Mirror buildStateFrontmatter's fmScalar: only string/number/boolean - // frontmatter scalars are usable (an object/array `current_phase` is ignored, - // which also avoids a base-to-string on a non-primitive). - const fmRawPhase = fm.current_phase; - const fmCurrentPhase = - typeof fmRawPhase === 'string' ? (fmRawPhase.trim() || null) - : typeof fmRawPhase === 'number' || typeof fmRawPhase === 'boolean' ? String(fmRawPhase) - : null; + // #3187: frontmatter-scalar-then-body-field precedence is owned by + // state-document.cjs's `stateFieldValue` (ADR-3180 §7.7). This comment + // previously claimed to mirror `buildStateFrontmatter`'s fmScalar — that + // attribution was stale: buildStateFrontmatter (state.cts:1620) reads body + // only and never consults frontmatter at all; this actually mirrored + // cmdStateSnapshot's now-removed closure instead. const positionSection = sliceCurrentPositionSection(body); const prosePhase = - positionSection !== null ? parseProsePhaseField(stateExtractField(positionSection, 'Phase')).phase : null; - const currentPhaseRaw = fmCurrentPhase ?? stateExtractField(body, 'Current Phase') ?? prosePhase; + positionSection !== null ? parseProsePhaseField(stateFieldValue(fm, positionSection, null, 'Phase').value).phase : null; + const currentPhaseRaw = stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? prosePhase; const currentPhase = parseInt(String(currentPhaseRaw), 10) || 0; const cutoff = currentPhase - keepRecent; @@ -3366,10 +3452,17 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean * that the phase execution is finished and the project is ready for the next phase. * Implements the `gsd state complete-phase` subcommand (issue #2735). */ -function resolvePhaseIdForCompletePhase(content: string, overridePhase: string | undefined): string | null { +function resolvePhaseIdForCompletePhase(fm: Record, body: string, overridePhase: string | undefined): string | null { + // #3187: route through the single #1760 fallback-chain owner (fm scalar + // then body field) instead of two raw stateExtractField calls on + // frontmatter-blind content — a STATE.md whose phase lives only in + // frontmatter no longer resolves to null here. `Phase` (the historical + // second-choice field name) has no frontmatter counterpart, so its fmKey + // is null — same shape as cmdStateSnapshot's `stateFieldValue(fm, + // currentPositionScope, null, 'Phase')` fallback. const candidate = overridePhase || - stateExtractField(content, 'Current Phase') || - stateExtractField(content, 'Phase') || + stateFieldValue(fm, body, 'current_phase', 'Current Phase').value || + stateFieldValue(fm, body, null, 'Phase').value || ''; // #2125: parse via the canonical anchored parser so a narrative `Phase:` @@ -3389,7 +3482,36 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string } const content = fs.readFileSync(statePath, 'utf-8'); - const resolvedPhase = resolvePhaseIdForCompletePhase(content, overridePhase); + // #1255/#3187: parse frontmatter and strip it from the body ONCE, mirroring + // cmdStateValidate/cmdStateSnapshot, so resolvePhaseIdForCompletePhase and + // the idempotency guard below consult the identical fm/body precedence — + // the two sites cannot drift onto different chains, extending the #2125 + // "same canonical parser" guarantee one layer earlier. + const { fm, body, scope } = readStateFrontmatterScoped(content, statePath); + + // #3187 Postel/visibility (design doc's sharpest case): this whole handler + // is the DESTRUCTIVE path the #3489 idempotency guard below protects — it + // decides whether a re-run of `state complete-phase --phase N` is allowed + // to roll STATE.md back to N's moment-of-completion. If the frontmatter + // half of the chain could not be consulted (`scope` UNREADABLE), + // `existingCurrentPhase` below could read as null even though the + // project's true current phase lives only in that unreadable frontmatter — + // silently treating a non-COMPLETE scope as "not complete" would let the + // guard's `existingCurrentPhase &&` check fail OPEN and re-run an + // already-completed phase. Refuse outright instead of guessing; this + // applies even when `--phase` is explicit, because the guard's job is to + // protect against exactly that already-completed-phase case regardless of + // how the target phase was named. + if (scope !== SCOPE.COMPLETE) { + output( + { error: 'Unable to read STATE.md frontmatter; refusing to run complete-phase to avoid a destructive rollback (#3489). Fix or remove the malformed frontmatter and retry.' }, + raw, + undefined, + ); + return; + } + + const resolvedPhase = resolvePhaseIdForCompletePhase(fm, body, overridePhase); if (!resolvedPhase || /^phase$/i.test(resolvedPhase)) { output({ error: 'Unable to resolve current phase. Pass an explicit phase: state complete-phase --phase ' }, raw, undefined); return; @@ -3404,7 +3526,7 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string // Last Activity, Last Activity Description, and the Current Position body. // The handler is now a no-op in that case so re-invocation from downstream // workflows cannot regress the project state. - const existingCurrentPhaseRaw = stateExtractField(content, 'Current Phase') || ''; + const existingCurrentPhaseRaw = stateFieldValue(fm, body, 'current_phase', 'Current Phase').value || ''; // #2125: same canonical parser as resolvePhaseIdForCompletePhase so the two // sites cannot diverge on the token they extract. const existingCurrentPhase = parsePhaseFromProse(existingCurrentPhaseRaw).phase; diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index 4e3519492..e5e604fcc 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -23,7 +23,10 @@ import planScan = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningPaths, planningRoot, getActiveWorkstream } = planningWorkspace; -import { stateExtractField } from './state-document.cjs'; +import { stateFieldValue } from './state-document.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module +import frontmatterMod = require('./frontmatter.cjs'); +const { extractFrontmatter, stripFrontmatter } = frontmatterMod; import { findTableWithColumns } from './markdown-table.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- verification.cjs is an export= CommonJS module import verificationMod = require('./verification.cjs'); @@ -435,12 +438,29 @@ function writeVerificationLedger(wsDir: string, ledger: VerificationLedger): voi function readStateProjection(statePath: string): StateProjection { try { const stateContent = fs.readFileSync(statePath, 'utf-8'); + // #3187: route Status/Current Phase/Last Activity through the single + // #1760 fallback-chain owner (state-document.cjs's stateFieldValue) + // instead of a frontmatter-blind stateExtractField(stateContent, …) call, + // mirroring cmdStateValidate/cmdStateSnapshot — a STATE.md whose fields + // live only in frontmatter is no longer projected as absent here. + const fm = extractFrontmatter(stateContent, statePath) as Record; + const body = stripFrontmatter(stateContent); return { - status: stateExtractField(stateContent, 'Status') || 'unknown', - current_phase: stateExtractField(stateContent, 'Current Phase'), - last_activity: stateExtractField(stateContent, 'Last Activity'), + status: stateFieldValue(fm, body, 'status', 'Status').value || 'unknown', + current_phase: stateFieldValue(fm, body, 'current_phase', 'Current Phase').value, + last_activity: stateFieldValue(fm, body, 'last_activity', 'Last Activity').value, }; } catch { + // Read/parse failure (missing file, permission fault, etc.) degrades to + // an all-unknown projection — unchanged. Not widened to also carry a + // `scope` here: doing so would ripple `StateProjection` + // (workstream-inventory-builder.cjs) and every consumer of this + // read-only rollup — the design doc's blast-radius table rates + // `readStateProjection` "low"/Tier-2, and this call site's migration is + // scoped to routing the fallback chain, not to widening the return type. + // The existing all-unknown degrade already distinguishes "could not + // read" from any real field value; only its scope-vs-absence *reason* + // stays uncaptured, same as before this change. return { status: 'unknown', current_phase: null, diff --git a/tests/state-field-drift.test.cjs b/tests/state-field-drift.test.cjs new file mode 100644 index 000000000..890895111 --- /dev/null +++ b/tests/state-field-drift.test.cjs @@ -0,0 +1,649 @@ +'use strict'; + +/** + * Tests for the STATE.md field-extraction fallback-chain drift guard + * (epic #3180, issue #3187 Phase 5, ADR-3180 §7.7) — + * `scripts/lint-state-field-drift.cjs`. + * + * Per ADR-3180 Decision 4(b) the guard AND this paired test are both + * required; neither alone is sufficient. Every test here drives the + * guard's exported PURE functions (`findStateFieldDrift`, `buildFunctionInfo`, + * `scanRepo`) with string fixtures — no temp tree is needed for anything but + * `scanRepo`'s own tree-walk contract (D1), which runs against the real repo. + * + * Covers .gsd/phase/refactor-3187-state-field-extraction-single-owner/ + * 50-test-matrix.md section D, rows D1-D9 (D3b/D8b/D8c included), plus the + * multi-line-signature attribution regression the guard's own module header + * documents (`buildFunctionInfo`'s `pendingDeclName` deferral). + * + * Fixtures use array `.join('\n')`, never an indented template literal — + * indentation bleed would shift every asserted line number. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const fs = require('node:fs'); + +const drift = require('../scripts/lint-state-field-drift.cjs'); +const { sanitizeForReport } = require('../scripts/lib/drift-scan.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const OWNER_RELPATH = drift.OWNER_FILE; +const OTHER_RELPATH = path.join('src', 'unrelated.cts'); + +// ─── D1: the real repo tree reports zero re-derivations ─────────────────── + +describe('D1 — scanRepo against the real repo tree', () => { + test('guard reports zero on the consolidated tree', () => { + // This is the guard's actual contract, exercised through its own + // scanRepo tree-walk (not a synthetic fixture) — the test that fails + // the day an eighth copy of the fallback chain lands anywhere in src/. + const violations = drift.scanRepo(REPO_ROOT); + assert.deepStrictEqual(violations, []); + }); +}); + +// ─── D2: the guard MUST be able to fail ──────────────────────────────────── + +describe('D2 — the guard must be able to fail', () => { + test('guard catches a reintroduced ladder', () => { + const text = [ + 'function fmScalarLadder(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + // A guard that cannot fail is worse than no guard: this fixture is a + // synthetic re-derivation (coercion ladder + fallback call, same + // function), and it MUST produce exactly one violation at the exact + // line of the fallback call — not merely "an array". + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 6); + assert.match(out[0].found, /stateExtractField/); + }); +}); + +// ─── D3 / D3b: body-only case-variant chain, with and without a ladder ──── + +describe('D3 — body-only case-variant chain without a ladder is not drift', () => { + test('body-only case-variant chain without a ladder is not drift', () => { + const text = [ + 'function noLadderFunction(body) {', + " const a = stateExtractField(body, 'Last Activity') ?? stateExtractField(body, 'Last activity');", + ' return a;', + '}', + ].join('\n'); + + // No ladder anywhere in this function — a bare case-variant fallback + // chain over the body alone is not the derivation this guard owns. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +describe('D3b — case-variant chain beside a ladder is drift', () => { + test('case-variant chain beside a ladder is drift', () => { + const text = [ + 'function withLadderFunction(fm, body) {', + ' const v = fm.status;', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' // handled elsewhere in this same function', + ' }', + " const a = stateExtractField(body, 'Last Activity') ?? stateExtractField(body, 'Last activity');", + ' return a;', + '}', + ].join('\n'); + + // The SAME case-variant chain, now inside a function that also bears + // the coercion ladder — the function must route through the owner + // instead of hand-rolling its own body-side fallback. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 6); + }); +}); + +// ─── D3c / D3d / D3e: evasion shapes found by isolated adversarial review ── +// A previous version of LADDER_RE required a bare-identifier `\1` +// backreference and hardcoded 'number' before 'boolean' on a single line. +// All three fixtures below were verified to produce ZERO violations against +// that version despite being real re-derivation shapes (ladder + +// `stateExtractField(` in the same function). + +describe('D3c — member-expression / computed operand ladder is drift', () => { + test('a dotted member-expression operand (fm.key) is caught', () => { + const text = [ + 'function memberOperand(fm, body) {', + " if (typeof fm.key === 'number' || typeof fm.key === 'boolean') {", + ' return String(fm.key);', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 5); + }); + + test('a computed-access operand (fm[key]) is caught', () => { + const text = [ + 'function computedOperand(fm, body, key) {', + " if (typeof fm[key] === 'number' || typeof fm[key] === 'boolean') {", + ' return String(fm[key]);', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 5); + }); + + test('a control case: two DIFFERENT member operands never counts as one ladder', () => { + // fm.a and fm.b are different operands — this must never be mistaken + // for a coercion ladder over a single value. + const text = [ + 'function unrelatedOperands(fm, body) {', + " if (typeof fm.a === 'number' || typeof fm.b === 'boolean') {", + ' return null;', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + assert.deepStrictEqual(drift.findStateFieldDrift(text, OTHER_RELPATH), []); + }); +}); + +describe('D3d — either tier order is drift (order swap)', () => { + test("'boolean' before 'number' (the opposite of the historical example) is caught", () => { + const text = [ + 'function orderSwap(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'boolean' || typeof v === 'number') {", + ' return String(v);', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 6); + }); +}); + +describe('D3e — a ladder split across lines, or as two separate `if` statements, is drift', () => { + test('the same `||` expression wrapped across two lines is caught', () => { + const text = [ + 'function splitAcrossLines(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'number' ||", + " typeof v === 'boolean') {", + ' return String(v);', + ' }', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 7); + }); + + test('two separate `if` statements (no `||` at all) are caught', () => { + const text = [ + 'function separateIfStatements(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'number') { return String(v); }", + " if (typeof v === 'boolean') { return String(v); }", + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 5); + }); + + test('two separate `if` statements FURTHER apart than LADDER_WINDOW_LINES are NOT caught (documented bound)', () => { + // Proves LADDER_WINDOW_LINES is a real, enforced bound, not decorative — + // mirrors D8's "distance is irrelevant" proof for the OTHER (unbounded) + // pairing in this same guard, but for the ladder-clause pairing this is + // intentionally the opposite: bounded, and the bound is real. + const filler = []; + for (let i = 0; i < drift.LADDER_WINDOW_LINES + 5; i++) filler.push(` // filler line ${i}`); + const text = [ + 'function tooFarApart(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'number') { return String(v); }", + ...filler, + " if (typeof v === 'boolean') { return String(v); }", + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + assert.deepStrictEqual(drift.findStateFieldDrift(text, OTHER_RELPATH), []); + }); +}); + +// ─── D4: nested frontmatter object read is not drift ─────────────────────── + +describe('D4 — nested frontmatter read is not drift', () => { + test('nested frontmatter read is not drift', () => { + const text = [ + 'function fmScalarKey(fm) {', + ' const v = fm.nested ? fm.nested.key : undefined;', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return null;', + '}', + ].join('\n'); + + // Ladder-bearing (the coercion ladder is present) but call-free — this + // is the live smart-entry.cts `fmScalarKey` control case proving the + // guard distinguishes "has a ladder" from "re-derives the fallback + // chain": a ladder alone with no stateExtractField( call in the same + // function is a different question and must stay silent. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +// ─── D5: comment-awareness ────────────────────────────────────────────────── + +describe('D5 — documented ladder in prose is not drift', () => { + test('documented ladder in prose is not drift', () => { + const text = [ + 'function commented(fm, body) {', + " // typeof v === 'number' || typeof v === 'boolean' then stateExtractField(body, 'x')", + ' /*', + " * typeof v === 'number' || typeof v === 'boolean'", + " * stateExtractField(body, 'x')", + ' */', + ' return null;', + '}', + ].join('\n'); + + // Both a `//` line comment and a `/* */` block comment carry the exact + // ladder + call text. ADR-3180 Amendment 3: a guard that reports prose + // as drift trains readers to reflexively exempt documentation, so + // scanCode must blank both before either detection regex runs. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +// ─── D6 / D7: owner-file exemption is function-scoped, never file-scoped ── + +describe('D6 — owner file is scanned; only named functions exempt', () => { + test('owner file is scanned; only named functions exempt', () => { + const text = [ + 'function stateFieldValue(fm, body, fmKey, bodyField) {', + ' const v = fm[fmKey];', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return stateExtractField(body, bodyField);', + '}', + ].join('\n'); + + // The canonical owner function itself IS the derivation by + // construction — it must stay silent when scanned under the real + // owner relPath. + const out = drift.findStateFieldDrift(text, OWNER_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +describe('D7 — a second copy inside the owner file is still caught', () => { + test('a second copy inside the owner file is still caught', () => { + // The exact Amendment-4 blind spot: a whole-file exemption on the + // owner is precisely how getMilestoneInfo stayed invisible to an + // earlier guard. FUNCTION_SCOPED_EXEMPTIONS must never let a SECOND, + // unrelated re-derivation elsewhere in the SAME owner file go unseen. + const text = [ + 'function stateFieldValue(fm, body, fmKey, bodyField) {', + ' const v = fm[fmKey];', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return stateExtractField(body, bodyField);', + '}', + '', + 'function stateFieldValueDuplicate(fm, body) {', + ' const w = fm.other;', + " if (typeof w === 'number' || typeof w === 'boolean') {", + ' return String(w);', + ' }', + " return stateExtractField(body, 'other');", + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OWNER_RELPATH); + // The canonical stateFieldValue (lines 1-7) stays silent; the + // unrelated second copy (stateFieldValueDuplicate, lines 9-15) is + // flagged — proving the exemption is keyed on FUNCTION NAME, not on + // the whole file. + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 14); + }); +}); + +describe('D6/D7 combined — the same line in a non-owner file is always reported', () => { + test('the same canonical-shaped line outside the owner file is reported (no exemption applies)', () => { + const text = [ + 'function stateFieldValue(fm, body, fmKey, bodyField) {', + ' const v = fm[fmKey];', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return stateExtractField(body, bodyField);', + '}', + ].join('\n'); + + // FUNCTION_SCOPED_EXEMPTIONS is keyed on relPath === OWNER_RELPATH; a + // function of the SAME name in a DIFFERENT file gets no exemption at + // all. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 6); + }); +}); + +// ─── D8 / D8b: distance and attribution are per-function, not a window ──── + +describe('D8 — distance within a function is irrelevant', () => { + test('distance within a function is irrelevant', () => { + const filler = []; + for (let i = 0; i < 40; i++) filler.push(` // filler line ${i} — no ladder or call text here`); + + const text = [ + 'function farApart(fm, body) {', + ' const v = fm.x;', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ...filler, + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + // No fixed line-distance window anywhere in this detection — an + // earlier draft used one and was rejected (ADR-3180 Decision 4(a)): + // it produced "a zero it did not earn" on live copies further from + // their ladder than any defensible line count. 40 filler lines here + // proves the current shape has no such ceiling. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 5 + filler.length + 1); + }); +}); + +describe('D8b — a neighbouring function is not implicated', () => { + test('a neighbouring function is not implicated', () => { + const text = [ + 'function ladderOwner(fm) {', + ' const v = fm.x;', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return null;', + '}', + '', + 'function neighbour(body) {', + " return stateExtractField(body, 'x');", + '}', + ].join('\n'); + + // The file, taken as a whole, carries BOTH a ladder and a + // stateExtractField( call — but they sit in two DIFFERENT named + // functions with no shared enclosing scope. Attribution is + // per-function, not per-file, so neither function alone qualifies. + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +// ─── D8c: closure-scoped ladders (both real-world shapes) ───────────────── + +describe('D8c — closure-scoped ladder is attributed correctly', () => { + test('an arrow-function closure (const fmScalar = (...) => {...}) is attributed to its enclosing function', () => { + const text = [ + 'function cmdStateSnapshotLike(fm, body) {', + ' const fmScalar = (key) => {', + ' const v = fm[key];', + " if (typeof v === 'number' || typeof v === 'boolean') return String(v);", + ' return null;', + ' };', + " const status = fmScalar('status') ?? stateExtractField(body, 'Status');", + ' return status;', + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 7); + + // Attribution proof: the call line's innermost open frame is the + // OUTER named function, not the closure (which has already closed by + // that line) — and the ladder marks the outer frame transitively, + // because the closure's body is lexically part of the outer + // function's own body. + const lines = text.split('\n'); + const { ladderBearing, innermostAt } = drift.buildFunctionInfo(lines); + assert.strictEqual(innermostAt[6], 'cmdStateSnapshotLike'); // 0-indexed line 7 + assert.ok(ladderBearing.has('cmdStateSnapshotLike')); + }); + + test('a nested function-declaration closure (function fmScalar(...) {...}) is attributed to its enclosing function', () => { + const text = [ + 'function outerWithNestedFn(fm, body) {', + ' function fmScalarNested(key) {', + ' const v = fm[key];', + " if (typeof v === 'number' || typeof v === 'boolean') return String(v);", + ' return null;', + ' }', + " const status = fmScalarNested('status') ?? stateExtractField(body, 'Status');", + ' return status;', + '}', + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 7); + + const lines = text.split('\n'); + const { ladderBearing, innermostAt } = drift.buildFunctionInfo(lines); + assert.strictEqual(innermostAt[6], 'outerWithNestedFn'); // 0-indexed line 7 + assert.ok(ladderBearing.has('outerWithNestedFn')); + }); +}); + +// ─── Regression: multi-line function signature must not strand its frame ── +// buildFunctionInfo's module-header documents a real bug: FUNCTION_DECL_RE +// can match a line with no `{` at all (a signature spilling across several +// lines); pushing that frame immediately recorded `openDepth` at the +// ENCLOSING scope's depth rather than the function's own, so the frame could +// NEVER pop and silently misattributed every later top-level line to it. +// `pendingDeclName` defers the push to the line whose brace count actually +// increases. 229 functions across 76 files share this multi-line-signature +// shape, so this proof is load-bearing, not decorative. + +describe('regression — a multi-line function signature does not strand its frame', () => { + test('a ladder+call inside a function whose signature spans several lines is flagged and attributed there; a later top-level call is not', () => { + const text = [ + 'function preferNewerLike(', + ' fm,', + ' body,', + ' fmKey', + ')', + '{', + ' const v = fm[fmKey];', + " if (typeof v === 'number' || typeof v === 'boolean') {", + ' return String(v);', + ' }', + ' return stateExtractField(body, fmKey);', + '}', + '', + "stateExtractField(body, 'decoy');", + ].join('\n'); + + const out = drift.findStateFieldDrift(text, OTHER_RELPATH); + // Only the in-function call (line 11) is flagged. If the frame had + // been stranded open (the pre-fix bug), the decoy top-level call on + // line 14 would ALSO be flagged — this assertion is the whole proof + // the fix holds. + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 11); + + const lines = text.split('\n'); + const { innermostAt } = drift.buildFunctionInfo(lines); + assert.strictEqual(innermostAt[10], 'preferNewerLike'); // 0-indexed line 11 + assert.strictEqual(innermostAt[13], null); // 0-indexed line 14 — module scope, unattributed + }); +}); + +// ─── D9: report-output sanitization ──────────────────────────────────────── +// `main()` sanitizes both a violation's file path and its matched fragment +// through `sanitizeForReport` (scripts/lib/drift-scan.cjs) before writing +// either to stderr — the exact boundary a fork-authored filename or source +// line reaches a CI log through. Direct coverage of the sanitizer itself, +// mirroring tests/plan-count-single-owner.test.cjs's `sanitizeForReport` +// describe block. + +describe('D9 — report output is sanitized', () => { + test('C0 control bytes are escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x1b)), '\\x1b'); + }); + + test('DEL (0x7f) is escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x7f)), '\\x7f'); + }); + + test('C1 control bytes are escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x9b)), '\\x9b'); + }); + + test('zero-width space (U+200B) is escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x200b)), '\\u200b'); + }); + + test('Unicode LINE SEPARATOR (U+2028) is escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x2028)), '\\u2028'); + }); + + test('Unicode PARAGRAPH SEPARATOR (U+2029) is escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x2029)), '\\u2029'); + }); + + test('a bidi RIGHT-TO-LEFT OVERRIDE (U+202E) is escaped', () => { + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x202e)), '\\u202e'); + }); + + test('ordinary ASCII filename/fragment text passes through unchanged', () => { + const text = "src/state-document.cts:296 stateExtractField(body, 'x')"; + assert.strictEqual(sanitizeForReport(text), text); + }); +}); + +// ─── D10: prompt-layer prose detection (ADR-3180 Decision 4(d)) ─────────── +// Widening the scan surface from `src/` alone to also cover the prompt-layer +// markdown (`gsd-core/workflows`, `commands`, `agents`, `skills`) is unproven +// unless (a) the surface is genuinely declared, and (b) a NEW prose +// re-derivation added there is genuinely caught — not merely that the guard +// happens to report zero because nothing was ever exercised against it. + +describe('D10a — the prompt layer is in the scan surface', () => { + test('SCAN_DIRS covers gsd-core/workflows, commands, agents, skills (in addition to src)', () => { + assert.ok(drift.SCAN_DIRS.includes('src')); + assert.ok(drift.SCAN_DIRS.includes('gsd-core/workflows')); + assert.ok(drift.SCAN_DIRS.includes('commands')); + assert.ok(drift.SCAN_DIRS.includes('agents')); + assert.ok(drift.SCAN_DIRS.includes('skills')); + }); + + test('SCAN_EXT covers markdown', () => { + assert.ok(drift.SCAN_EXT.has('.md')); + }); +}); + +describe('D10b — a NEW prose re-derivation in a workflow file is flagged', () => { + const OTHER_WORKFLOW_RELPATH = path.join('gsd-core', 'workflows', 'unrelated-workflow.md'); + + test('a synthetic workflow line carrying the fm-key/body-bold tokens plus both words is caught', () => { + const text = [ + '# Some Workflow', + '', + '1. Read STATE.md. Extract: `owner` (frontmatter `owner:` or body `**Owner:**`), and proceed.', + '', + ].join('\n'); + + const out = drift.findPromptFieldDrift(text, OTHER_WORKFLOW_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].line, 3); + assert.match(out[0].found, /frontmatter/); + }); + + test('control case: the backtick tokens alone, with no bold body token, are not flagged', () => { + const text = '1. Read STATE.md. Extract: `owner` (frontmatter `owner:` or plain body Owner field), and proceed.'; + const out = drift.findPromptFieldDrift(text, OTHER_WORKFLOW_RELPATH); + assert.deepStrictEqual(out, []); + }); + + test('control case: both tokens present but the word "frontmatter" absent is not flagged', () => { + const text = '1. Extract: `owner` (config `owner:` or body `**Owner:**`).'; + const out = drift.findPromptFieldDrift(text, OTHER_WORKFLOW_RELPATH); + assert.deepStrictEqual(out, []); + }); + + test('control case: both tokens and "frontmatter" present but the word "body" absent is not flagged', () => { + const text = '1. Extract: `owner` (frontmatter `owner:` or fallback `**Owner:**`).'; + const out = drift.findPromptFieldDrift(text, OTHER_WORKFLOW_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); + +describe('D10c — the smart-entry.md gsd-tools-down fallback is a written, permanent exemption', () => { + const SMART_ENTRY_RELPATH = path.join('gsd-core', 'workflows', 'smart-entry.md'); + const LIVE_LINE = + "- Read `.planning/STATE.md` (frontmatter + body) with the Read tool. Extract: `status` (frontmatter `status:` or body `**Status:**`), `Phase:` from the body, `total_phases`/`percent` from a nested `progress:` frontmatter object if present, and any `## Blockers` items."; + + test('the real live line is caught when NOT under the exempted relPath (proves detection genuinely fires on this exact text — it is not silent by construction)', () => { + const out = drift.findPromptFieldDrift(LIVE_LINE, path.join('gsd-core', 'workflows', 'unrelated-workflow.md')); + assert.strictEqual(out.length, 1); + }); + + test('the SAME line under the real smart-entry.md relPath is exempted (silent), per the written PROMPT_LAYER_EXEMPTIONS reason', () => { + const out = drift.findPromptFieldDrift(LIVE_LINE, SMART_ENTRY_RELPATH); + assert.deepStrictEqual(out, []); + // The exemption carries a written reason (Decision 4(d)) — not a bare + // suppression — and is NOT a Decision 4(e) ratchet: this site cannot be + // migrated onto the owner by construction, so there is no removal issue + // to key it against. + const reasons = drift.PROMPT_LAYER_EXEMPTIONS.get(SMART_ENTRY_RELPATH); + assert.ok(reasons.has(LIVE_LINE.trim())); + assert.match(reasons.get(LIVE_LINE.trim()), /gsd-tools|cannot run|by construction/i); + }); + + test('the exemption is keyed on exact trimmed text, not merely the file — a DIFFERENT re-derivation in the SAME file is still caught', () => { + const text = '- Extract: `owner` (frontmatter `owner:` or body `**Owner:**`).'; + const out = drift.findPromptFieldDrift(text, SMART_ENTRY_RELPATH); + assert.strictEqual(out.length, 1); + }); + + test('the real smart-entry.md file on disk scans clean end-to-end (the actual contract, not just the extracted fixture)', () => { + const content = fs.readFileSync(path.join(REPO_ROOT, 'gsd-core', 'workflows', 'smart-entry.md'), 'utf8'); + const out = drift.findPromptFieldDrift(content, SMART_ENTRY_RELPATH); + assert.deepStrictEqual(out, []); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 8f0d61499..88b9d0d69 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -6,13 +6,22 @@ * GSD Tools Tests - State */ -const { test, describe, beforeEach, afterEach } = require('node:test'); +const { test, describe, beforeEach, afterEach, mock } = require('node:test'); 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 { createFixture } = require('./fixtures/index.cjs'); +const { createFixture, seedWorkstream } = require('./fixtures/index.cjs'); +// #3187 (ADR-3180 §7.7) matrix sections B/C: in-process access to the chain +// owner and its raw inputs, needed to compute the "owner's answer" a +// consumer's OBSERVABLE output is compared against (Decision 4c) — never the +// owner's return value against itself. +const stateLib = require('../gsd-core/bin/lib/state.cjs'); +const stateDocument = require('../gsd-core/bin/lib/state-document.cjs'); +const frontmatterLib = require('../gsd-core/bin/lib/frontmatter.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); +const workstreamInventory = require('../gsd-core/bin/lib/workstream-inventory.cjs'); function writePassedVerification(tmpDir, phaseDirName, paddedPhase) { fs.writeFileSync( @@ -21,6 +30,35 @@ function writePassedVerification(tmpDir, phaseDirName, paddedPhase) { ); } +/** + * Run `fn` while capturing every fd-1 write a `cmdState*` handler's + * `output()` performs (it writes via a raw `fs.writeSync(1, ...)`, never + * `console.log`/`process.stdout.write`). Standalone helper with no test + * context, so the try/finally restore is CONTRIBUTING-compliant (mirrors + * `tests/state-rebuild-cli.test.cjs`'s identical helper — needed here too + * because the #3187 B6/B7 IO-failure-injection rows require `mock.method` + * on `fs`, which only intercepts an IN-PROCESS call; a `runGsdTools` + * subprocess would not observe the parent process's mock). + */ +function captureStdout(fn) { + const chunks = []; + const original = fs.writeSync; + fs.writeSync = (fd, data, offset, length) => { + if (fd !== 1) return original(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + chunks.push(chunk); + return Buffer.byteLength(chunk, 'utf8'); + }; + try { + fn(); + } finally { + fs.writeSync = original; + } + return chunks.join(''); +} + describe('state-snapshot command', () => { let tmpDir; @@ -3185,6 +3223,611 @@ describe('state validate command', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3187 (ADR-3180 §7.7) — matrix section B: `state validate`'s scope field, +// including the #3162 headline regression and #1255 frontmatter shadowing. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3187 state validate — scope field (matrix section B)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('B1: validate detects drift when phase is frontmatter-only (#3162 regression — must fail before the fix)', () => { + // Phase lives ONLY in frontmatter; the body has no `Current Phase` field + // at all. Pre-#3187, cmdStateValidate read `Current Phase` off the body + // only, resolved null, and the ENTIRE drift block was skipped — + // "could not look" was output-identical to "looked, all clean" + // ({valid:true, warnings:[], drift:{}}). + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'current_phase: 2', + '---', + '# Project State', + '', + '**Status:** Executing Phase 2', + '**Total Plans in Phase:** 5', + '**Current Plan:** 1', + '', + ].join('\n'), + ); + + const phaseDir = path.join(tmpDir, '.planning', 'phases', '02-core'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '02-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '02-02-PLAN.md'), '# Plan\n'); + + const result = runGsdTools('state validate', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + // The RIGHT reason to fail before the fix: valid was true and no + // plan-count warning was ever generated, not a crash. + assert.strictEqual(output.valid, false, 'STATE.md says 5 plans, disk has 2 — drift must be reported'); + assert.ok(output.warnings.some((w) => /plan.*count|count.*mismatch/i.test(w))); + assert.deepEqual(output.drift.plan_count, { state: 5, disk: 2 }); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('B2: validate passes cleanly when it actually looked (frontmatter-only phase, counts match)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'current_phase: 2', + '---', + '# Project State', + '', + '**Status:** Executing Phase 2', + '**Total Plans in Phase:** 2', + '**Current Plan:** 1', + '', + ].join('\n'), + ); + + const phaseDir = path.join(tmpDir, '.planning', 'phases', '02-core'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '02-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '02-02-PLAN.md'), '# Plan\n'); + + const result = runGsdTools('state validate', tmpDir); + const output = JSON.parse(result.output); + assert.strictEqual(output.valid, true); + assert.strictEqual(output.warnings.length, 0); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('B3: validate still detects body-resolved drift (regression guard, unchanged today)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 3', + '**Current Plan:** 1', + '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(output.valid, false); + assert.ok(output.warnings.some((w) => /plan.*count|count.*mismatch/i.test(w))); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('B4: unresolvable phase is not reported as clean (distinguishable from B2)', () => { + // Neither frontmatter nor body carries a Current Phase field anywhere. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + ['# Project State', '', '**Status:** Planning', ''].join('\n'), + ); + + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + // ⛔ Rejected #2: a non-COMPLETE scope must NEVER be routed to valid:false. + assert.strictEqual(output.valid, true); + assert.strictEqual(output.warnings.length, 0); + assert.notStrictEqual(output.scope, SCOPE.COMPLETE, 'scope must be distinguishable from a real clean pass (B2)'); + assert.strictEqual(output.scope, SCOPE.UNSCOPED); + }); + + test('B5: missing phase dir differs from could-not-look (distinguishable from B4)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 99', + '**Current Phase:** 99', + '**Total Plans in Phase:** 2', + '', + ].join('\n'), + ); + // phases/ exists (createFixture) but has no matching 99-* directory. + + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(output.valid, true); + assert.strictEqual(output.warnings.length, 0); + // Resolvable phase + legitimately-absent directory is a real answer — + // COMPLETE — not the same non-answer as B4's totally unresolvable phase. + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('B6: unreadable phases dir is surfaced, not swallowed', (t) => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 1', + '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const originalReaddirSync = fs.readdirSync; + mock.method(fs, 'readdirSync', (p, ...rest) => { + if (p === phasesDir) { + const err = new Error('EACCES: permission denied, scandir'); + err.code = 'EACCES'; + throw err; + } + return originalReaddirSync.call(fs, p, ...rest); + }); + t.after(() => mock.restoreAll()); + + const raw = captureStdout(() => stateLib.cmdStateValidate(tmpDir, false)); + const output = JSON.parse(raw); + assert.strictEqual(output.valid, true, 'the previous silent degrade must not crash or fabricate a warning'); + assert.strictEqual(output.scope, SCOPE.UNREADABLE); + }); + + test('B7: one unreadable verification file does not abort the scan', (t) => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 1', + '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + const brokenPath = path.join(phaseDir, '01-A-VERIFICATION.md'); + const okPath = path.join(phaseDir, '01-B-VERIFICATION.md'); + fs.writeFileSync(brokenPath, ['---', 'status: passed', '---', ''].join('\n')); + fs.writeFileSync(okPath, ['---', 'status: passed', '---', ''].join('\n')); + + const originalReadFileSync = fs.readFileSync; + mock.method(fs, 'readFileSync', (p, ...rest) => { + if (p === brokenPath) { + const err = new Error('EACCES: permission denied, open'); + err.code = 'EACCES'; + throw err; + } + return originalReadFileSync.call(fs, p, ...rest); + }); + t.after(() => mock.restoreAll()); + + const raw = captureStdout(() => stateLib.cmdStateValidate(tmpDir, false)); + const output = JSON.parse(raw); + // Per-file swallow (#2245 audit) is unchanged: the other verification + // file is still consulted, so its drift still fires, and the whole + // scan is NOT degraded to UNREADABLE just because one file 404s. + // Asserted on the structured `drift` field (CONTRIBUTING.md's "Prohibited: + // Raw Text Matching on Test Outputs"), not a `warnings` prose regex — + // `drift.verification_status` is a typed record, not free-form text. + assert.deepStrictEqual(output.drift.verification_status, { state_status: 'Executing Phase 1', verification: 'passed' }); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('B8: absent STATE.md unchanged', () => { + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(output.error, 'STATE.md not found'); + assert.strictEqual(output.scope, undefined); + }); + + test('B9: binary STATE.md still fails loud (existing #2701 path unchanged, no scope key added)', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), Buffer.from('# State\0\0\0binary')); + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(output.valid, false); + assert.strictEqual(output.warnings.length, 1); + assert.strictEqual(output.scope, undefined, 'the #2701 early return is explicitly unchanged — no scope key'); + }); + + test('B10: json and default output agree (validate has no distinct raw-text mode; --raw is a no-op for it)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 1', + '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + + const withoutFlag = runGsdTools('state validate', tmpDir).output; + const withRaw = runGsdTools('state validate --raw', tmpDir).output; + assert.deepEqual(JSON.parse(withoutFlag), JSON.parse(withRaw)); + }); + + test('B11: plan-count boundary 0/1/2 — no false drift when counts match, real drift when they do not', () => { + for (const n of [0, 1, 2]) { + const dir = createFixture(); + fs.writeFileSync( + path.join(dir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + `**Total Plans in Phase:** ${n}`, + '', + ].join('\n'), + ); + const phaseDir = path.join(dir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + for (let i = 1; i <= n; i++) { + fs.writeFileSync(path.join(phaseDir, `01-${String(i).padStart(2, '0')}-PLAN.md`), '# Plan\n'); + } + const output = JSON.parse(runGsdTools('state validate', dir).output); + assert.strictEqual(output.valid, true, `n=${n} matching disk must be valid`); + assert.strictEqual(output.warnings.length, 0, `n=${n} matching disk must have no warnings`); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + cleanup(dir); + } + + for (const n of [0, 1, 2]) { + const dir = createFixture(); + fs.writeFileSync( + path.join(dir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + `**Total Plans in Phase:** ${n}`, + '', + ].join('\n'), + ); + const phaseDir = path.join(dir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + for (let i = 1; i <= n + 1; i++) { + fs.writeFileSync(path.join(phaseDir, `01-${String(i).padStart(2, '0')}-PLAN.md`), '# Plan\n'); + } + const output = JSON.parse(runGsdTools('state validate', dir).output); + assert.strictEqual(output.valid, false, `n=${n} vs disk n+1 must be flagged`); + // Asserted on the structured `drift.plan_count` record rather than a + // `warnings` prose regex (CONTRIBUTING.md's "Prohibited: Raw Text + // Matching on Test Outputs") — `drift` is typed, `warnings` is free-form. + assert.deepStrictEqual(output.drift.plan_count, { state: n, disk: n + 1 }); + cleanup(dir); + } + }); + + test('B12: a thrown extractFrontmatter degrades scope to UNREADABLE (frontmatter-parse catch, distinct from B6\'s phases-dir-scan-failure catch)', (t) => { + // extractFrontmatter is documented never to throw (src/frontmatter.cts + // has no `throw` in its extraction path), so this catch is a defensive + // branch normally unreachable through any real STATE.md content. Drive + // it directly by mocking the frontmatter module's export — since + // src/state.cts destructures `extractFrontmatter` out of the frontmatter + // module at REQUIRE time (`const { extractFrontmatter } = frontmatter;`), + // mocking the already-cached `frontmatterLib.extractFrontmatter` has no + // effect on a state.cjs module instance that was required BEFORE the + // mock was installed (its local binding already captured the original + // function). Busting state.cjs's own require-cache entry and re-requiring + // it AFTER the mock is installed forces its destructuring to capture the + // mocked function instead — the top-level `stateLib` binding used by + // every other test in this file is untouched throughout (a fresh module + // instance is a distinct exports object). + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '# Project State', + '', + '**Status:** Executing Phase 1', + '**Current Phase:** 1', + '**Total Plans in Phase:** 1', + '', + ].join('\n'), + ); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); + + const stateCjsPath = require.resolve('../gsd-core/bin/lib/state.cjs'); + delete require.cache[stateCjsPath]; + mock.method(frontmatterLib, 'extractFrontmatter', () => { + throw new Error('simulated frontmatter parse failure'); + }); + t.after(() => { + mock.restoreAll(); + delete require.cache[stateCjsPath]; + }); + const freshStateLib = require(stateCjsPath); + + const raw = captureStdout(() => freshStateLib.cmdStateValidate(tmpDir, false)); + const output = JSON.parse(raw); + assert.strictEqual(output.valid, true, 'a frontmatter-parse failure must not crash or fabricate a warning'); + assert.strictEqual(output.scope, SCOPE.UNREADABLE, 'a thrown extractFrontmatter must degrade scope, exactly like B6\'s phases-dir failure'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #3187 (ADR-3180 §7.7, Decision 4c) — matrix section C: identity tests +// asserted at each CONSUMER's observable output vs the chain owner's answer +// for the SAME input, never the owner's return value against itself. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3187 chain-owner identity — every consumer agrees with stateFieldValue (matrix section C)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('C1: snapshot output matches the chain owner', () => { + const content = [ + '---', + 'current_phase: 3', + 'status: verifying', + '---', + '# Project State', + '', + '**Current Phase:** 1', // shadowed by frontmatter — must NOT win + '**Status:** Planning', // shadowed by frontmatter — must NOT win + '**Total Plans in Phase:** 4', + '**Current Plan:** 2', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const output = JSON.parse(runGsdTools('state-snapshot', tmpDir).output); + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + + assert.strictEqual(output.current_phase, stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value); + assert.strictEqual(output.status, stateDocument.stateFieldValue(fm, body, 'status', 'Status').value); + assert.strictEqual(String(output.total_plans_in_phase), stateDocument.stateFieldValue(fm, body, 'total_plans_in_phase', 'Total Plans in Phase').value); + assert.strictEqual(output.current_plan, stateDocument.stateFieldValue(fm, body, 'current_plan', 'Current Plan').value); + }); + + test('C2: validate resolves the same phase as the owner', () => { + const content = [ + '---', + 'current_phase: 2', + '---', + '# Project State', + '', + '**Current Phase:** 1', // shadowed — must NOT be the phase validate scans against + '**Status:** Executing Phase 2', + '**Total Plans in Phase:** 1', + '**Current Plan:** 1', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + const ownerPhase = stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + assert.strictEqual(ownerPhase, '2', 'sanity: owner must resolve frontmatter phase 2, not shadowed body phase 1'); + + // Phase 1 (the wrong/shadowed candidate) gets a MISMATCHING disk count; + // phase 2 (the owner's actual answer) gets a MATCHING disk count. Only + // observable if validate scanned the owner's phase, not the shadowed one. + const phase1Dir = path.join(tmpDir, '.planning', 'phases', '01-setup'); + fs.mkdirSync(phase1Dir, { recursive: true }); + fs.writeFileSync(path.join(phase1Dir, '01-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phase1Dir, '01-02-PLAN.md'), '# Plan\n'); + + const phase2Dir = path.join(tmpDir, '.planning', 'phases', '02-core'); + fs.mkdirSync(phase2Dir, { recursive: true }); + fs.writeFileSync(path.join(phase2Dir, '02-01-PLAN.md'), '# Plan\n'); + + const output = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(output.valid, true, 'validate must have scanned phase 2 (the owner answer), which matches disk'); + assert.strictEqual(output.warnings.length, 0); + assert.strictEqual(output.scope, SCOPE.COMPLETE); + }); + + test('C3: prune resolves the same phase as the owner', () => { + const content = [ + '---', + 'current_phase: 7', + '---', + '# Project State', + '', + '**Current Phase:** 2', // shadowed + '**Status:** Executing Phase 7', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + const ownerPhase = stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + assert.strictEqual(ownerPhase, '7'); + + const keepRecent = 2; + const output = JSON.parse(runGsdTools(`state prune --keep-recent ${keepRecent} --dry-run`, tmpDir).output); + assert.strictEqual(output.cutoff_phase, Number(ownerPhase) - keepRecent); + }); + + test('C4: smart-entry matches the owner under its own (deliberately unscoped) declared read', () => { + const content = [ + '---', + 'current_phase: 5', + '---', + '# Project State', + '', + '**Current Phase:** 1', // shadowed + '**Status:** Executing Phase 5', + '**Total Phases:** 8', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + // smart-entry's own declared read is deliberately UNSCOPED over the whole + // body (design's Rejected #3 — an explicit, written exemption, not a + // silent fold) — same fm/body inputs, same fallback shape, just with the + // body-only `Phase` prose step it also declares. + const ownerPhaseRaw = + stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value ?? + stateDocument.stateFieldValue(fm, body, null, 'Phase').value; + + const output = JSON.parse(runGsdTools('smart-entry --json', tmpDir).output); + assert.strictEqual(output.signals.current_phase, parseInt(ownerPhaseRaw, 10)); + }); + + test('C5: workstream projection matches the owner', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-c5' }); + // Divergent frontmatter vs. body (mirrors C3/C4): frontmatter says 4, + // body says 1. `readStateProjection` (Derivation A site 6) is now routed + // through `stateFieldValue`, so this proves the consumer actually + // resolves the frontmatter-tier value rather than merely agreeing with + // the owner on a fixture where the two tiers could never disagree. + const content = [ + '---', + 'current_phase: 4', + '---', + '# Project State', + '', + '**Current Phase:** 1', // shadowed + '**Status:** Executing Phase 4', + '', + ].join('\n'); + const statePath = path.join(wsDir, 'STATE.md'); + fs.writeFileSync(statePath, content); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + const ownerPhase = stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + assert.strictEqual(ownerPhase, '4'); + + const inv = workstreamInventory.inspectWorkstream(tmpDir, 'ws-c5'); + assert.ok(inv, 'inspectWorkstream should find the seeded workstream'); + assert.strictEqual(inv.current_phase, ownerPhase); + }); + + test('C6: idempotency guard matches the owner', () => { + // Divergent frontmatter vs. body (mirrors C3/C4): frontmatter says 5, + // body says 1. `resolvePhaseIdForCompletePhase` / the complete-phase + // idempotency guard (Derivation A site 5) is now routed through + // `stateFieldValue`, so this proves the guard actually resolves the + // frontmatter-tier value 5 (not the shadowed body value 1) when deciding + // whether the requested phase 3 has already been superseded. + const content = [ + '---', + 'current_phase: 5', + '---', + '# Project State', + '', + '**Status:** in-progress', + '**Current Phase:** 1', // shadowed + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + const ownerPhase = stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + assert.strictEqual(ownerPhase, '5'); + + const output = JSON.parse(runGsdTools(['state', 'complete-phase', '--phase', '3'], tmpDir).output); + assert.strictEqual(output.idempotent, true, 'requested phase 3 precedes the owner-resolved current phase 5 (from frontmatter, not the shadowed body value 1), so the guard must fire'); + }); + + test('C7: every consumer agrees on the same STATE.md', () => { + const content = [ + '# Project State', + '', + '**Status:** Executing Phase 6', + '**Current Phase:** 6', + '**Total Plans in Phase:** 2', + '**Current Plan:** 1', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, content); + const phaseDir = path.join(tmpDir, '.planning', 'phases', '06-final'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '06-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(phaseDir, '06-02-PLAN.md'), '# Plan\n'); + + const fm = frontmatterLib.extractFrontmatter(content, statePath); + const body = frontmatterLib.stripFrontmatter(content); + const ownerPhase = stateDocument.stateFieldValue(fm, body, 'current_phase', 'Current Phase').value; + assert.strictEqual(ownerPhase, '6'); + + // C1: state snapshot + const snapshot = JSON.parse(runGsdTools('state-snapshot', tmpDir).output); + assert.strictEqual(snapshot.current_phase, ownerPhase); + + // C2: state validate (indirect — valid+no-warnings only holds if phase 6 was used) + const validate = JSON.parse(runGsdTools('state validate', tmpDir).output); + assert.strictEqual(validate.valid, true); + assert.strictEqual(validate.warnings.length, 0); + assert.strictEqual(validate.scope, SCOPE.COMPLETE); + + // C3: state prune (cutoff_phase + keepRecent must equal the owner phase) + const keepRecent = 2; + const prune = JSON.parse(runGsdTools(`state prune --keep-recent ${keepRecent} --dry-run`, tmpDir).output); + assert.strictEqual(prune.cutoff_phase + keepRecent, Number(ownerPhase)); + + // C4: smart-entry + const smartEntry = JSON.parse(runGsdTools('smart-entry --json', tmpDir).output); + assert.strictEqual(smartEntry.signals.current_phase, Number(ownerPhase)); + + // C6: complete-phase idempotency guard (no frontmatter in this fixture, + // so this row does not exercise the frontmatter tier — see C6's own test + // for that divergent-frontmatter case). + const complete = JSON.parse(runGsdTools(['state', 'complete-phase', '--phase', '3'], tmpDir).output); + assert.strictEqual(complete.idempotent, true); + + // C5 (workstream inventory) is intentionally NOT folded into this + // cross-consumer fixture: it reads a structurally different path + // (`.planning/workstreams//STATE.md`), so it cannot be "the same + // STATE.md" as the root-level consumers above without contradicting the + // literal same-input premise this row is about. See C5's own test. + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // state sync (Step 5 — Gate 2) // ───────────────────────────────────────────────────────────────────────────── @@ -7094,6 +7737,55 @@ describe('bug #3489: state complete-phase must be idempotent', () => { assert.notEqual(payload.idempotent, true, 'first completion must not be flagged idempotent'); assert.ok(Array.isArray(payload.updated) && payload.updated.length > 0, 'expected non-empty updated list'); }); + + test('a thrown extractFrontmatter refuses complete-phase rather than risking a destructive rollback (#3489)', (t) => { + // The fail-closed refusal this exercises: if the frontmatter half of the + // chain cannot be consulted, `existingCurrentPhase` could read as null + // even though the project's true current phase lives only in that + // unreadable frontmatter, which would let the idempotency guard above + // fail OPEN and re-run an already-completed phase. cmdStateCompletePhase + // refuses outright instead. extractFrontmatter is documented never to + // throw for real STATE.md content, so this is driven directly by + // mocking the frontmatter module's export — see B12's identical + // require-cache-busting rationale in tests/state.test.cjs (state.cts + // destructures `extractFrontmatter` at require time, so mocking the + // already-required `frontmatterLib` export has no effect on the + // top-level `stateLib` binding; a fresh module instance, required AFTER + // the mock is installed, is needed to observe it). + const stateMd = [ + '# State', + '', + '**Status:** in-progress', + '**Current Phase:** 03', + '**Last Activity:** 2026-05-13', + '', + ].join('\n'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, stateMd, 'utf8'); + const before = fs.readFileSync(statePath, 'utf8'); + + const stateCjsPath = require.resolve('../gsd-core/bin/lib/state.cjs'); + delete require.cache[stateCjsPath]; + mock.method(frontmatterLib, 'extractFrontmatter', () => { + throw new Error('simulated frontmatter parse failure'); + }); + t.after(() => { + mock.restoreAll(); + delete require.cache[stateCjsPath]; + }); + const freshStateLib = require(stateCjsPath); + + const raw = captureStdout(() => freshStateLib.cmdStateCompletePhase(tmpDir, false, '03')); + const output = JSON.parse(raw); + assert.match( + output.error || '', + /refusing to run complete-phase/i, + `expected the #3489 refuse-path error, got: ${raw}`, + ); + + const after = fs.readFileSync(statePath, 'utf8'); + assert.equal(after, before, 'STATE.md must not be rewritten when frontmatter could not be read'); + }); }); }); }