From b7431a9259a9914ab65906baa5bfc88e60fd11cb Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 9 Aug 2026 14:23:35 -0400 Subject: [PATCH] feat(#1956): flag cross-artifact fact drift in the plan drift guard (#3259) * test(#1956): failing-first contract for cross-artifact fact-drift pass * feat(#1956): flag cross-artifact fact drift in the plan drift guard * fix(#1956): correct config-key assertion and bidirectional lifecycle-lag exemption * docs(#1956): document the cross-artifact axis in the architecture reference * feat(#1956): decide the phase-status drift axis deterministically * fix(#1956): scope the progress-table lookup, abstain without a position section, rank deferred * docs(#1956): backfill changeset pr number --------- Co-authored-by: sim --- .changeset/sunny-ravens-parade.md | 5 + CONTEXT.md | 4 +- docs/ARCHITECTURE.md | 2 + docs/CONFIGURATION.md | 2 +- docs/USER-GUIDE.md | 2 + gsd-core/bin/gsd-tools.cjs | 143 +++++- gsd-core/workflows/plan-review-convergence.md | 45 ++ src/plan-drift-guard.cts | 143 ++++++ src/roadmap-parser.cts | 46 +- src/state-document.cts | 34 ++ src/state.cts | 11 +- tests/adr-22-plan-drift-guard.test.cjs | 275 ++++++++++++ .../1956-cross-artifact-fact-drift.json | 6 + tests/plan-review-convergence.test.cjs | 409 ++++++++++++++++++ 14 files changed, 1116 insertions(+), 11 deletions(-) create mode 100644 .changeset/sunny-ravens-parade.md create mode 100644 tests/emitted-drift-acks/1956-cross-artifact-fact-drift.json diff --git a/.changeset/sunny-ravens-parade.md b/.changeset/sunny-ravens-parade.md new file mode 100644 index 000000000..c20e3e15f --- /dev/null +++ b/.changeset/sunny-ravens-parade.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3259 +--- +**The plan drift guard now flags the same fact stated two ways** — when ROADMAP.md, PLAN.md, STATE.md and CONTEXT.md contradict each other about a phase status, a success criterion, a requirement ID or a domain term, plan review reports it in REVIEWS.md naming both locations and which one is authoritative, instead of letting a fresh-context agent act on the stale copy. The phase-status axis is decided deterministically rather than by judgment, so a STATE/ROADMAP contradiction is caught the same way every time — and a disagreement about whether a phase is *complete* is always reported, never written off as one document lagging the other. Advisory only; it never blocks convergence, and the judgment axes key on contradicting knowledge rather than similar-looking text. Runs under the existing `plan_review.source_grounding` switch — no new setting. (#1956) diff --git a/CONTEXT.md b/CONTEXT.md index e7356ab9a..6e127b5da 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, and frontmatter reconstruction. 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; `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`. ### 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`). @@ -181,7 +181,7 @@ Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-t Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/lib/write-set.cjs`, generated from `src/write-set.cts`; ADR-2143 §5/§6, epic #2143). Pure, Node built-ins only, no I/O. Exports: `Result` (`{ok:true,value}\|{ok:false,reason}` — ADR-2143 §5 fail-loud parse shape, never a bare `null` a caller can mistake for "empty but fine"; the single source of truth `markdown-table.cjs` re-exports so its existing importers are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`); `WriteOutcome` (`{surface: string, applied: boolean}` — one surface's outcome within a multi-surface write); `WriteSet` (`WriteOutcome[]`); `writeSetComplete(ws) → boolean` (true only when the set is non-empty AND every surface applied — ADR-2143 §6's "no OR-into-one-flag" rule: a command that mutates more than one surface must not collapse independent surface outcomes into a single boolean, the anti-pattern that let a checkbox-only partial write (#2140) report full success). `milestone.cts`'s `requirements mark-complete` handler is the first consumer: it reports a `write_set` (`checkbox`/`traceability` surfaces) and `write_set_complete` alongside its existing `updated`/`marked_complete`/`already_complete`/`not_found`/`table_unmatched` fields, which remain computed exactly as before — the write-set is additive, structured ADR-2143 documentation of the same per-surface facts #2140's tactical fix already exposed via `table_unmatched`. ### Roadmap Parser Module -Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Milestone WINDOWING — which headings bound a milestone — is owned here as of #3184 (epic #3180 Phase 2, ADR-3180 Decision 1): `computeMilestoneSectionEnd` (the section-end walk, formerly duplicated as two distinct nested `computeSectionEnd` functions plus an inline third copy in `getMilestonePhaseFilter`'s `versionOverride` branch), `locateMilestoneHeadings` (heading location, version token boundary-matched with `\b`, **NOT** the stricter `(?![\w.-])`: `v2.0` therefore DOES match inside `v2.0.1`, and a milestone STATE of `v8.0` legitimately selects a live `## v8.0-B …` over a closed `v8.0-A` sibling — deliberate, load-bearing #730 behavior that ADR-3180 Amendment 2 tried to tighten and then reverted; the earlier text here described that reverted alternative as if it had shipped, corrected by #3216), `listMilestoneHeadings` (#3216 — the version-AGNOSTIC enumeration of every milestone heading in document order, sharing ONE grammar source with `locateMilestoneHeadings` so the two cannot drift; `locateMilestoneHeadings` is now a version-filtered view over it rather than a second expression of the pattern). Milestone IDENTITY — which milestone is current and what it is CALLED — is owned by `getMilestoneInfo`, which since #3216 binds to that same locator instead of its own heading regexes and returns a `ScopedResult`: a name retains parentheses and drops a trailing `✅`/`📋`/`🚧` marker, a `### Phase N …` heading is never the milestone heading (#3197), and an identity that cannot be determined returns a non-`COMPLETE` scope rather than the former `{version:'v1.0', name:'milestone'}` default, which was output-identical to a successful read of a genuine v1.0 project. `buildStateFrontmatter` and `archivePhaseDirectories` branch on that scope, so a fabricated identity is never persisted to `STATE.md` nor used as a `milestones/-phases/` path component, `sliceMilestoneWindow` (the one composition of locate → prefer-non-closed → section-end, so a consumer cannot re-assemble its own window from the primitives), and `isMilestoneBoundedInRoadmap` (the named predicate replacing two byte-identical re-derivations in `state.cts`). `hasMilestoneSectioning(content)` is the sibling predicate answering the WEAKER question `buildStateFrontmatter` actually asks — "could a whole-document phase count conflate two different milestones?" — and since #3185 it is decided by milestone VOCABULARY, not by heading position: a heading is a milestone heading iff it is a non-Phase heading (level 1-3) carrying a version token, a shipped/active marker, or the word `Milestone`, and sectioning means two or more of them, since one section cannot conflate siblings. Three position-based models were tried and each shipped a defect — "any non-Phase heading" over-detects, so a flat ROADMAP carrying an ordinary `## Progress` was called sectioned and its declared phase count discarded for the on-disk directory count (#3204, #2828 regressing at 1.9.1 via #3184's own consolidation); strict nesting misses same-level siblings (regressing #1761) and false-positives on the bundled `templates/roadmap.md` shape, where a `## Phases` wrapper holds a single nested milestone; adjacency false-positives whenever a structural heading merely precedes a phase heading. Known limit: two milestone sections carrying none of the three signals are not detected. `extractCurrentMilestoneScoped` is the real extractor and returns the Planning Scope Module's `ScopedResult`; `extractCurrentMilestone` remains a one-line wrapper over `.value` because its blast radius is CRITICAL (200+ affected symbols, 20 direct callers) and its signature must not move. `getMilestonePhaseFilter` gains a `scope` field: its pass-all degrade is PRESERVED where its premise holds (a genuinely-empty, freshly-declared milestone reports `SCOPE.COMPLETE`) and is now labelled where it does not (`SCOPE.TRUNCATED` when the window reached no phase entries while the document has them), so the destructive consumer — `milestone.complete`, which MOVES phase directories — can refuse instead of archiving every phase directory on disk (#3166). The filter's function behavior is deliberately unchanged: making it deny-all on a non-COMPLETE scope would trade a silent over-inclusive answer for a silent under-inclusive one on the read paths that count with it. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). +Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Milestone WINDOWING — which headings bound a milestone — is owned here as of #3184 (epic #3180 Phase 2, ADR-3180 Decision 1): `computeMilestoneSectionEnd` (the section-end walk, formerly duplicated as two distinct nested `computeSectionEnd` functions plus an inline third copy in `getMilestonePhaseFilter`'s `versionOverride` branch), `locateMilestoneHeadings` (heading location, version token boundary-matched with `\b`, **NOT** the stricter `(?![\w.-])`: `v2.0` therefore DOES match inside `v2.0.1`, and a milestone STATE of `v8.0` legitimately selects a live `## v8.0-B …` over a closed `v8.0-A` sibling — deliberate, load-bearing #730 behavior that ADR-3180 Amendment 2 tried to tighten and then reverted; the earlier text here described that reverted alternative as if it had shipped, corrected by #3216), `listMilestoneHeadings` (#3216 — the version-AGNOSTIC enumeration of every milestone heading in document order, sharing ONE grammar source with `locateMilestoneHeadings` so the two cannot drift; `locateMilestoneHeadings` is now a version-filtered view over it rather than a second expression of the pattern). Milestone IDENTITY — which milestone is current and what it is CALLED — is owned by `getMilestoneInfo`, which since #3216 binds to that same locator instead of its own heading regexes and returns a `ScopedResult`: a name retains parentheses and drops a trailing `✅`/`📋`/`🚧` marker, a `### Phase N …` heading is never the milestone heading (#3197), and an identity that cannot be determined returns a non-`COMPLETE` scope rather than the former `{version:'v1.0', name:'milestone'}` default, which was output-identical to a successful read of a genuine v1.0 project. `buildStateFrontmatter` and `archivePhaseDirectories` branch on that scope, so a fabricated identity is never persisted to `STATE.md` nor used as a `milestones/-phases/` path component, `sliceMilestoneWindow` (the one composition of locate → prefer-non-closed → section-end, so a consumer cannot re-assemble its own window from the primitives), and `isMilestoneBoundedInRoadmap` (the named predicate replacing two byte-identical re-derivations in `state.cts`). `hasMilestoneSectioning(content)` is the sibling predicate answering the WEAKER question `buildStateFrontmatter` actually asks — "could a whole-document phase count conflate two different milestones?" — and since #3185 it is decided by milestone VOCABULARY, not by heading position: a heading is a milestone heading iff it is a non-Phase heading (level 1-3) carrying a version token, a shipped/active marker, or the word `Milestone`, and sectioning means two or more of them, since one section cannot conflate siblings. Three position-based models were tried and each shipped a defect — "any non-Phase heading" over-detects, so a flat ROADMAP carrying an ordinary `## Progress` was called sectioned and its declared phase count discarded for the on-disk directory count (#3204, #2828 regressing at 1.9.1 via #3184's own consolidation); strict nesting misses same-level siblings (regressing #1761) and false-positives on the bundled `templates/roadmap.md` shape, where a `## Phases` wrapper holds a single nested milestone; adjacency false-positives whenever a structural heading merely precedes a phase heading. Known limit: two milestone sections carrying none of the three signals are not detected. `extractCurrentMilestoneScoped` is the real extractor and returns the Planning Scope Module's `ScopedResult`; `extractCurrentMilestone` remains a one-line wrapper over `.value` because its blast radius is CRITICAL (200+ affected symbols, 20 direct callers) and its signature must not move. `getMilestonePhaseFilter` gains a `scope` field: its pass-all degrade is PRESERVED where its premise holds (a genuinely-empty, freshly-declared milestone reports `SCOPE.COMPLETE`) and is now labelled where it does not (`SCOPE.TRUNCATED` when the window reached no phase entries while the document has them), so the destructive consumer — `milestone.complete`, which MOVES phase directories — can refuse instead of archiving every phase directory on disk (#3166). The filter's function behavior is deliberately unchanged: making it deny-all on a non-COMPLETE scope would trade a silent over-inclusive answer for a silent under-inclusive one on the read paths that count with it. `findRoadmapProgressTable(content)` (#1956) locates the `## Progress` table — scoped to that heading via the markdown-sectionizer seam, falling back to the whole document for a headingless milestone slice — so a differently-headed table sharing the `Phase | Plans Complete | Status | Completed` columns cannot be read instead (the #2012 decoy class). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` expresses the same scope independently for the completion RATIO; the two are held in agreement by a parity test rather than by a shared call, because that symbol's blast radius does not justify a refactor. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), plan/summary pairing helpers (`countMatchedSummaries`, `findUnsummarizedPlans`, `findOrphanSummaries`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `extractCanonicalPlanId`, `timeAgo`). `filterPlanFiles`/`filterSummaryFiles` were retired by #3183 (ADR-3180 Decision 2) — `getPhaseFileStats` no longer re-derives plan/summary filename matching locally; it now sources `plans`/`summaries` (plus a `scope` field, `COMPLETE`/`TRUNCATED`/`UNREADABLE`) directly from `scanPhasePlans` (`src/plan-scan.cts`), the single owner of live-plan counting. Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a851c0d99..65c49907e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -792,6 +792,8 @@ remove or rewrite anything. The plan drift guard (`plan_review.source_grounding`) — which verifies symbol references in generated plans against live source before execution — is specified in [ADR 22](adr/22-plan-drift-guard.md). +The same switch gates a second, cross-artifact axis: a fact-drift pass that compares the *same* fact as stated in `ROADMAP.md`, `PLAN.md`, `STATE.md` and `CONTEXT.md` and reports contradictions (a phase status, a success criterion, a requirement ID, a glossary term) with both locations and the authoritative side named. Where the source-grounding axis grounds a plan against code, this one grounds the planning artifacts against each other. It keys on contradicting knowledge rather than similar-looking text, and is advisory only — it never sets `hardBlock` and never contributes to the convergence counts. + ### Platform Handling - **Windows:** `windowsHide` on child processes, EPERM/EACCES protection on protected directories, path separator normalization diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 4bf5de961..b4d652d63 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -702,7 +702,7 @@ The `plan_review.*` namespace controls the plan drift guard, which verifies that | Setting | Type | Default | Description | |---------|------|---------|-------------| -| `plan_review.source_grounding` | boolean | `true` | Enable the plan drift guard. When `true` (the default), plan review resolves every symbol reference cited in a PLAN.md against the live source tree. Plans that cite a non-existent function, class, decorator, or CLI flag produce a `needs-acknowledgement` notice before the plan is approved. Disable with `false` to skip symbol verification entirely. Toggle during setup (`/gsd-new-project`) or at any time via `/gsd-settings`. | +| `plan_review.source_grounding` | boolean | `true` | Enable the plan drift guard. When `true` (the default), plan review resolves every symbol reference cited in a PLAN.md against the live source tree. Plans that cite a non-existent function, class, decorator, or CLI flag produce a `needs-acknowledgement` notice before the plan is approved. The same key also gates the cross-artifact fact-drift pass, which reports when ROADMAP.md, PLAN.md, STATE.md and CONTEXT.md state the same fact in contradictory ways (advisory only — it never blocks convergence). Disable with `false` to skip both passes entirely. Toggle during setup (`/gsd-new-project`) or at any time via `/gsd-settings`. | | `plan_review.source_grounding_authority` | enum | `grep` | Selects the resolver adapter used to verify symbol existence. Allowed values: `grep` (default — ripgrep/grep search of source files, works in any project without additional tooling), `intel` (query the `.planning/intel/api-map.json` index built by `/gsd-map-codebase`; requires `intel.enabled: true`), `treesitter` (reserved for future tree-sitter adapter), `lsp` (reserved for future LSP adapter), `scip` (reserved for future SCIP/LSIF adapter). Use `intel` when you have run `/gsd-map-codebase` and want the faster, pre-indexed lookup. All other values beyond `grep` and `intel` are reserved and have no effect in the current release. | diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index 8658e0979..c4b7b8463 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -560,6 +560,8 @@ claude --dangerously-skip-permissions **Default-on.** The plan drift guard (`plan_review.source_grounding: true`) runs during plan review and verifies that every symbol your plans cite — decorators, classes, functions, CLI flags — actually exists in your source tree at review time. This catches hallucinated names before any execution agent runs. +**Two axes, one switch.** The same guard also runs a cross-artifact fact-drift pass: when ROADMAP.md, PLAN.md, STATE.md and CONTEXT.md state the *same* fact in contradictory ways — a phase marked complete in one and in progress in the other, a success criterion the plan restates with a different outcome, a term used against its CONTEXT.md definition — you get an advisory finding in REVIEWS.md naming both locations and which one is authoritative. It keys on contradicting *knowledge*, not on similar-looking text, so a plan that simply restates a criterion in its own words is not flagged. The findings never block convergence. + **What it catches:** - Functions referenced in a PLAN.md step that don't exist in source diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index a2d0d3dd9..c3a2e6bc7 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -80,6 +80,7 @@ * drift-guard severity --status Classify a symbol verdict into { severity, hardBlock } * [--authority ] Status: VERIFIED|MISSING|AMBIGUOUS|UNCHECKABLE * Authority: grep|intel|treesitter|lsp|scip (default: config-resolved) + * drift-guard phase-status [--phase N] Compare STATE.md vs ROADMAP.md phase status * * Validation: * validate consistency Check phase numbering, disk/roadmap sync @@ -305,7 +306,7 @@ const { routeCheckCommand } = require('./lib/check-command-router.cjs'); const { routeTaskCommand } = require('./lib/task-command-router.cjs'); const { parseNamedArgs, parseMultiwordArg } = require('./lib/command-arg-projection.cjs'); const { cmdGitBaseBranch } = require('./lib/git-base-branch.cjs'); -const { getEffectiveAuthority, classifyDriftSeverity } = require('./lib/plan-drift-guard.cjs'); +const { getEffectiveAuthority, classifyDriftSeverity, comparePhaseStatus } = require('./lib/plan-drift-guard.cjs'); // ─── Bridge collapsed (Phase 4) ──────────────────────────────────────────────── // Non-family commands now run through their CJS handlers directly. Keep the @@ -3283,8 +3284,146 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load return; } + if (subcommand === 'phase-status') { + // #1956: deterministic STATE.md-vs-ROADMAP.md phase-status drift. + const { planningDir } = require('./lib/planning-workspace.cjs'); + const { stateExtractField, stateCurrentPositionSlice } = require('./lib/state-document.cjs'); + const { findRoadmapProgressTable } = require('./lib/roadmap-parser.cjs'); + const { phaseKeyFromProse } = require('./lib/phase-id.cjs'); + // STATE.md's YAML frontmatter carries its own lowercase `status:` + // scalar ahead of the body's `## Current Position` prose "Status:" + // line; stateExtractField's non-scoped regex would otherwise match + // that frontmatter line first (it comes first in the file) and + // silently report the wrong value. Strip frontmatter so extraction + // is scoped to the body. + const { stripFrontmatter } = require('./lib/frontmatter.cjs'); + + const phaseIdx = args.indexOf('--phase'); + const phaseArg = (phaseIdx !== -1 && args[phaseIdx + 1] && !args[phaseIdx + 1].startsWith('--')) + ? args[phaseIdx + 1] + : undefined; + + const dir = planningDir(cwd); + const statePath = path.join(dir, 'STATE.md'); + const roadmapPath = path.join(dir, 'ROADMAP.md'); + + let stateContent = null; + try { + stateContent = fs.readFileSync(statePath, 'utf-8'); + } catch { + // missing_state below + } + if (stateContent === null) { + const phase = phaseArg !== undefined ? phaseKeyFromProse(phaseArg) : null; + output({ + verdict: 'uncheckable', + reason: 'missing_state', + phase, + stateStatus: null, + roadmapStatus: null, + authority: 'STATE.md', + }, raw); + return; + } + + let roadmapContent = null; + try { + roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); + } catch { + // missing_roadmap below + } + + const stateBody = stripFrontmatter(stateContent); + // #1956 fix: scope extraction to `## Current Position` (or `###` + // in the bootstrap template) so a historical `Phase:` / `Status:` + // line in an archive section (e.g. `## Session Continuity + // Archive`) can't shadow the real one — same #2956 scope state.cts + // uses for current_phase, via the shared owner in + // state-document.cjs. + // + // Deliberately NO whole-body fallback here. `state.cts`'s WRITE + // path falls back to the whole body when no Current Position + // heading is found (legacy behavior it must preserve for + // backward-compatible writes) — but that fallback is wrong for a + // READ that feeds a drift finding: a STATE.md with no Current + // Position heading is exactly the shape that let a stray historical + // `Status:` line elsewhere in the body shadow the real value and + // fabricate a 'drifted' verdict. A guess is worse than an + // abstention for a drift detector, so an absent Current Position + // section reports 'uncheckable' instead of guessing from the whole + // document. + const currentPositionBody = stateCurrentPositionSlice(stateBody); + if (currentPositionBody === null) { + const phase = phaseArg !== undefined ? phaseKeyFromProse(phaseArg) : null; + output({ + verdict: 'uncheckable', + reason: 'no_current_position', + phase, + stateStatus: null, + roadmapStatus: null, + authority: 'STATE.md', + }, raw); + return; + } + + // Resolve the target phase: --phase if given, else whatever + // STATE.md's Current Position reports as current. + const phase = phaseArg !== undefined + ? phaseKeyFromProse(phaseArg) + : phaseKeyFromProse(stateExtractField(currentPositionBody, 'Phase')); + + if (roadmapContent === null) { + output({ + verdict: 'uncheckable', + reason: 'missing_roadmap', + phase, + stateStatus: null, + roadmapStatus: null, + authority: 'STATE.md', + }, raw); + return; + } + + const stateStatus = stateExtractField(currentPositionBody, 'Status'); + + // #1956/#2012: scoped to `## Progress` first (decoy-avoidance) — + // see findRoadmapProgressTable's doc comment (roadmap-parser.cts). + const table = findRoadmapProgressTable(roadmapContent); + const matchedRow = table + ? table.rows.find((row) => phaseKeyFromProse(row.Phase) === phase && phase !== null) + : undefined; + + if (!matchedRow) { + const result = comparePhaseStatus({ stateStatus, roadmapStatus: null }); + output({ + verdict: 'uncheckable', + reason: 'phase_not_in_roadmap', + phase, + stateStatus, + roadmapStatus: null, + stateRank: result.stateRank, + roadmapRank: result.roadmapRank, + authority: 'STATE.md', + }, raw); + return; + } + + const roadmapStatus = matchedRow.Status; + const result = comparePhaseStatus({ stateStatus, roadmapStatus }); + output({ + verdict: result.verdict, + phase, + stateStatus, + roadmapStatus, + stateRank: result.stateRank, + roadmapRank: result.roadmapRank, + authority: 'STATE.md', + }, raw); + return; + } + error( - `Unknown drift-guard subcommand: ${subcommand || '(none)'}. Available: authority, severity`, + `Unknown drift-guard subcommand: ${subcommand || '(none)'}. Available: authority, severity, phase-status`, ERROR_REASON.SDK_UNKNOWN_COMMAND, ); } diff --git a/gsd-core/workflows/plan-review-convergence.md b/gsd-core/workflows/plan-review-convergence.md index 2e38ff8e8..143e15bf0 100644 --- a/gsd-core/workflows/plan-review-convergence.md +++ b/gsd-core/workflows/plan-review-convergence.md @@ -254,6 +254,51 @@ Run this pass unless `plan_review.source_grounding` is `false`. It verifies ever - Signature mismatches cannot be asserted under `grep`/`intel`; report the signature as UNCHECKABLE. 5. **Coverage block.** Append a "Verification coverage" section to `REVIEWS.md` listing every UNCHECKABLE/skipped symbol and why — a clean review must never silently mean "nothing was checked." +### Cross-artifact fact-drift pass (same gate: `plan_review.source_grounding`) + +Run this pass whenever the source-grounding pass ran — it is the second axis of the same drift guard, gated by the same `plan_review.source_grounding` key and adding no config surface of its own. Where source-grounding asks *"does this symbol exist in the source?"*, this asks *"does the project state the same fact in two planning artifacts, and do the two disagree?"* Because each phase runs in a fresh context, an agent typically reads only one artifact and trusts it, so a stale duplicate silently steers it wrong. + +**Key on knowledge, not on similar text.** DRY is about a single authoritative representation of a piece of *knowledge*. Two passages that merely read alike, or that restate one fact at different levels of detail, are NOT drift. Only a contradiction is. + +1. **Phase status — decided by the seam, not by judgment.** Do not eyeball this axis: + + ```bash + DRIFT=$(gsd_run drift-guard phase-status --phase "${PHASE}") + # $DRIFT is JSON: {"verdict":"consistent|lag|drifted|uncheckable","stateStatus":…,"roadmapStatus":…} + ``` + + - `drifted` — STATE.md and ROADMAP.md contradict each other. Report it; the authority is STATE.md. + - `lag` — one lifecycle step apart between non-terminal statuses. NOT a finding. + - `consistent` — nothing to report. + - `uncheckable` — a document was absent or carried a status outside both vocabularies. Record it in the coverage block; never read it as consistent. + + Completeness is terminal: when exactly one side says the phase is complete, the verdict is `drifted` and never `lag`, however few steps apart the two words look. + +2. **Pair up the remaining facts by judgment.** The authority column names the source of truth, so a finding can say which side to keep: + + | Fact class | Artifact pair | Authority | Decided by | + |---|---|---|---| + | Success criteria / must-have truths | ROADMAP.md Success Criteria ↔ PLAN.md `must_haves.truths` | ROADMAP.md | judgment | + | Requirement IDs | ROADMAP.md `**Requirements:**` ↔ PLAN.md task requirement refs | ROADMAP.md | judgment | + | Phase status | STATE.md status ↔ ROADMAP.md phase state | STATE.md | step 1 (deterministic) | + | Glossary / domain term | CONTEXT.md `Decisions` ↔ PLAN.md usage of the term | CONTEXT.md | judgment | + +3. **Judge each judgment pair.** FLAG only when ALL THREE hold: + + 1. both sides name the *same* fact — same requirement ID, same success criterion, or the same defined term; and + 2. the two representations *contradict*, one asserting what the other denies, rather than differing in wording or in level of detail; and + 3. the pair is one of the judgment pairs above. + +4. **Record.** Emit each finding into `REVIEWS.md` beside the source-grounding coverage block, quoting both locations and naming the divergence and the authority, so the author can collapse the two copies to a single source of truth. + +**Do NOT flag:** a wording-only difference that asserts the same thing; a fact that appears in one artifact only — single-source is the target state, not a finding; a PLAN that ADDS a truth beyond the roadmap Success Criteria, which is sanctioned (plans may add, never subtract); a `lag` verdict from step 1 — two non-terminal statuses a single lifecycle step apart, in either direction, since STATE.md is written at planning time independently of ROADMAP.md and can lead as readily as trail (a disagreement about *completion* is never lag, and step 1 already reports it as `drifted`); anything under CONTEXT.md's `Claude's Discretion` or `Deferred Ideas`, which are non-authoritative by design. + +**Report once, not twice — these belong to `gsd-plan-checker`:** a PLAN that omits a roadmap Success Criterion is scope reduction (Dimension 7b); a requirement ID the ROADMAP never defines is requirement coverage (Dimension 1); two PLAN.md files in one phase disagreeing is cross-plan data contracts (Dimension 9). + +**Severity: advisory, never a blocker.** This pass never sets `hardBlock`, and its findings contribute to neither `HIGH_COUNT` nor `ACTIONABLE_COUNT` — a project carrying pre-existing drift must still be able to converge, or an advisory check becomes an endless replan loop. + +**Coverage, never silence.** If STATE.md or CONTEXT.md is absent, that axis is skipped and the skip is recorded in the same "Verification coverage" block. A clean pass must never mean "nothing was compared." + After agent returns, verify REVIEWS.md exists: ```bash REVIEWS_FILE=$(ls ${phase_dir}/${padded_phase}-REVIEWS.md 2>/dev/null) diff --git a/src/plan-drift-guard.cts b/src/plan-drift-guard.cts index 73c656a9c..b0a37470d 100644 --- a/src/plan-drift-guard.cts +++ b/src/plan-drift-guard.cts @@ -148,3 +148,146 @@ export function classifyDriftSeverity({ return { severity: 'INFO', hardBlock: false }; } } + +// ─── #1956 cross-artifact phase-status drift ──────────────────────────────── + +/** Verdict from comparing one phase's status across STATE.md and ROADMAP.md. */ +export type PhaseStatusVerdict = 'consistent' | 'lag' | 'drifted' | 'uncheckable'; + +/** Result of comparePhaseStatus. */ +export interface PhaseStatusResult { + verdict: PhaseStatusVerdict; + stateRank: number | null; + roadmapRank: number | null; +} + +/** + * Frozen map from lowercased phase-status text to a shared ordinal rank, + * covering the union of the STATE.md "Current Position" vocabulary + * (gsd-core/templates/state.md) and the FULL ROADMAP.md "## Progress" table + * Status column vocabulary declared by gsd-core/templates/roadmap.md:133 — + * `Not started | In progress | Complete | Deferred`. + * + * The two vocabularies overlap on 'in progress', which is rank 1 in both — + * no conflict. 'deferred' is rank 0 (no work done) — see comparePhaseStatus's + * doc comment for how a deferred/non-rank-0 mismatch is classified; it is NOT + * simply numeric distance from rank 0 like an ordinary lag. + */ +const PHASE_STATUS_RANKS: Readonly> = Object.freeze({ + // STATE.md "Current Position" vocabulary + 'ready to plan': 0, + 'planning': 0, + 'ready to execute': 1, + 'in progress': 1, + 'phase complete': 2, + // ROADMAP.md "## Progress" table Status column vocabulary + 'not started': 0, + 'complete': 2, + 'deferred': 0, +} as const); + +/** Rank at which a status asserts work is DONE (terminal, not comparative). */ +const TERMINAL_RANK = 2; + +/** + * Normalize a raw phase-status string for lookup/comparison: trims + * surrounding whitespace and lowercases. Returns null for missing/empty + * values. Single owner of this normalization so `resolvePhaseStatusRank` and + * the 'deferred' declared-intent check in `comparePhaseStatus` cannot drift + * apart on what counts as "empty". + */ +function normalizePhaseStatusText(value: string | null | undefined): string | null { + if (value === null || value === undefined) return null; + const normalized = value.trim().toLowerCase(); + return normalized === '' ? null : normalized; +} + +/** + * Resolve a raw phase-status string to its shared ordinal rank, or null when + * the value is missing/empty/unrecognized. Case-insensitive, trims + * surrounding whitespace. + * + * @param value - raw status text from STATE.md or ROADMAP.md + * @returns the resolved rank, or null if unresolvable + */ +function resolvePhaseStatusRank(value: string | null | undefined): number | null { + const normalized = normalizePhaseStatusText(value); + if (normalized === null) return null; + if (!Object.prototype.hasOwnProperty.call(PHASE_STATUS_RANKS, normalized)) return null; + return PHASE_STATUS_RANKS[normalized]; +} + +/** + * Compare a phase's status as reported by STATE.md against the same phase's + * status as reported by ROADMAP.md's "## Progress" table, and classify the + * result. + * + * Unlike classifyDriftSeverity, this never throws for an unrecognized + * status: the inputs are user document text (prose a human or agent typed + * into STATE.md/ROADMAP.md), not config, so an unrecognized value is data — + * surfaced as 'uncheckable' — not a programming error. + * + * Rank 2 ('phase complete' / 'Complete') is TERMINAL: it asserts the work is + * DONE. If exactly one side reports rank 2 and the other does not, that is + * always 'drifted', regardless of numeric distance — a document claiming + * "done" while another claims "still going" is a direct contradiction, not + * mere lag. This is the issue's canonical example: complete in STATE.md but + * in progress in ROADMAP.md. + * + * 'Deferred' is a second declared-intent rank-0 status (gsd-core/templates/ + * roadmap.md:133's full vocabulary: `Not started | In progress | Complete | + * Deferred`) that is NOT ordinary lag from rank 0: it is an explicit + * decision to STOP work, not merely "hasn't started yet". If exactly one + * side declares 'deferred' and the other side's rank is >= 1 (work is + * reported as in progress or complete), that is always 'drifted' — a phase + * declared deferred while the other document says work is happening is a + * direct contradiction, checked here (like the terminal-completeness rule + * above) BEFORE the numeric distance comparison. 'Deferred' against a + * rank-0 status on the other side (e.g. 'Not started') stays 'consistent' — + * both agree no work has happened. + * + * Otherwise, ranks are compared numerically: equal → 'consistent'; + * off-by-one → 'lag'; off-by-two-or-more → 'drifted'. + * + * @param opts.stateStatus - raw Status value from STATE.md's Current Position + * @param opts.roadmapStatus - raw Status cell from ROADMAP.md's Progress table + * @returns { verdict, stateRank, roadmapRank } + */ +export function comparePhaseStatus({ + stateStatus, + roadmapStatus, +}: { + stateStatus: string | null | undefined; + roadmapStatus: string | null | undefined; +}): PhaseStatusResult { + const stateRank = resolvePhaseStatusRank(stateStatus); + const roadmapRank = resolvePhaseStatusRank(roadmapStatus); + + if (stateRank === null || roadmapRank === null) { + return { verdict: 'uncheckable', stateRank, roadmapRank }; + } + + const stateIsTerminal = stateRank === TERMINAL_RANK; + const roadmapIsTerminal = roadmapRank === TERMINAL_RANK; + if (stateIsTerminal !== roadmapIsTerminal) { + return { verdict: 'drifted', stateRank, roadmapRank }; + } + + const stateIsDeferred = normalizePhaseStatusText(stateStatus) === 'deferred'; + const roadmapIsDeferred = normalizePhaseStatusText(roadmapStatus) === 'deferred'; + if (stateIsDeferred !== roadmapIsDeferred) { + const otherRank = stateIsDeferred ? roadmapRank : stateRank; + if (otherRank >= 1) { + return { verdict: 'drifted', stateRank, roadmapRank }; + } + } + + const diff = Math.abs(stateRank - roadmapRank); + if (diff === 0) { + return { verdict: 'consistent', stateRank, roadmapRank }; + } + if (diff === 1) { + return { verdict: 'lag', stateRank, roadmapRank }; + } + return { verdict: 'drifted', stateRank, roadmapRank }; +} diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 160494f75..e34a855ed 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -13,7 +13,8 @@ * - ./phase-id.cjs (escapeRegex, phaseMarkdownRegexSource) * - ./planning-workspace.cjs (planningDir) * - ./shell-command-projection.cjs (platformReadSync) - * - ./markdown-sectionizer.cjs (tokenizeHeadings, stripTaggedBlocks, withSection) + * - ./markdown-sectionizer.cjs (tokenizeHeadings, stripTaggedBlocks, withSection, collectSection) + * - ./markdown-table.cjs (findTableWithColumns) */ import fs from 'node:fs'; @@ -38,8 +39,10 @@ import { platformReadSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import unusableInputMod = require('./unusable-input.cjs'); const { UNUSABLE_REASON, warnUnusableInput } = unusableInputMod; -import { tokenizeHeadings, stripTaggedBlocks, withSection, stripFencedCode } from './markdown-sectionizer.cjs'; +import { tokenizeHeadings, stripTaggedBlocks, withSection, stripFencedCode, collectSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; +import { findTableWithColumns } from './markdown-table.cjs'; +import type { MarkdownTable } from './markdown-table.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; @@ -882,6 +885,42 @@ function reportUnreadableRoadmap(err: unknown, roadmapPath: string): void { warnUnusableInput({ reason: UNUSABLE_REASON.ROADMAP_UNREADABLE, source: roadmapPath }); } +// ─── Roadmap progress table (#1956/#2012 decoy avoidance) ───────────────────── + +/** + * Locate ROADMAP.md's "Progress" table — the sole owner of the #2012 + * decoy-avoidance scope for the `drift-guard phase-status` CLI seam (#1956). + * + * Scopes to the `## Progress` heading first (level-2, exact case-insensitive + * text `'progress'`, `{ levelBounded: true }`) via `collectSection` — the + * same CRLF-safe seam `stateCurrentPositionSlice` (state-document.cts) uses + * to scope STATE.md's `## Current Position` — so a differently-headed table + * that happens to share the same column names (e.g. an "Archive Notes" + * table) is never picked up instead of the real one (#2012). Falls back to + * scanning the WHOLE document when no `## Progress` heading exists, so a + * headingless milestone slice (#1445) still resolves rather than going + * uncheckable — the same fallback `deriveProgressFromRoadmap` + * (phase-lifecycle.cts) deliberately preserves. + * + * `deriveProgressFromRoadmap` independently expresses this same "scope to + * `## Progress`, else whole document" rule via its own regex-based scope + * (kept there deliberately rather than refactored onto this function — its + * blast radius is large). The two locators are therefore separate + * implementations of the same scoping rule and must agree about WHICH table + * is the Progress table; a parity test in + * tests/adr-22-plan-drift-guard.test.cjs asserts they do, per the repo's + * generative-fix-divergence guard. + * + * Returns the same shape `findTableWithColumns` returns (or `null`). + */ +function findRoadmapProgressTable(roadmapContent: string): MarkdownTable | null { + const isProgressHeading = (h: HeadingToken): boolean => + h.level === 2 && h.text.trim().toLowerCase() === 'progress'; + const section = collectSection(roadmapContent, isProgressHeading, { levelBounded: true }); + const scoped = section ? section.body : roadmapContent; + return findTableWithColumns(scoped, ['Phase', 'Plans Complete', 'Status', 'Completed']); +} + // ─── Milestone info lookup ──────────────────────────────────────────────────── interface MilestoneInfo { @@ -1465,4 +1504,7 @@ export = { sliceMilestoneWindow, hasVersionedMilestones, hasMilestoneSectioning, + // #1956: sole owner of the #2012 decoy-avoidance scope for the + // `drift-guard phase-status` CLI seam. + findRoadmapProgressTable, }; diff --git a/src/state-document.cts b/src/state-document.cts index 82f6fa347..4363b85a8 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -9,6 +9,8 @@ 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'; // Internal helpers function escapeRegex(str: string): string { @@ -232,6 +234,38 @@ export function stateExtractField(content: string, fieldName: string): string | return null; } +/** + * Match the "Current Position" section body from a STATE.md body. #2956: this + * is the Phase analogue of state.cts's matchSessionSection. `Phase` canonically + * lives under `## Current Position` (gsd-core/templates/state.md), so — like + * Stopped At / Paused At under `## Session` — it must be extracted from THAT + * section, not from the first `Phase:` / `**Phase:**` line anywhere in the + * body. Without the scope, a historical `Phase:` line in an archive section + * silently shadows the real one on every read/write, and because callers use + * this for routing (state.cts's current_phase) and for drift detection + * (gsd-tools.cjs's `drift-guard phase-status` CLI seam), a stale match either + * routes work to the wrong phase or fabricates a drift finding. + * + * Level-flexible: the canonical template uses an h2 `## Current Position`, the + * bootstrap template an h3 `### Current Position` (templates/state.md). Both + * must match — mirroring how matchSessionSection recognises `## Session` and + * `## Session Continuity`. Exact 'current position' text match (case- + * insensitive) excludes unrelated headings. Built on the `collectSection` + * seam, so it inherits that seam's CRLF tolerance (#2444 fix). + * + * This is the single owner of the scope — state.cts's private + * `matchCurrentPositionSection` delegates here rather than duplicating the + * logic, so the two consumers cannot drift apart. + * + * Returns the section body, or null (caller falls back to full-body search). + */ +export function stateCurrentPositionSlice(body: string): string | null { + const isCurrentPosition = (h: HeadingToken): boolean => + (h.level === 2 || h.level === 3) && h.text.trim().toLowerCase() === 'current position'; + const section = collectSection(body, isCurrentPosition, { levelBounded: true }); + return section ? section.body : null; +} + export function stateReplaceField(content: string, fieldName: string, newValue: string): string | null { const escaped = escapeRegex(fieldName); // Bold inline format: **FieldName:** value diff --git a/src/state.cts b/src/state.cts index de5e8317e..96e3fbb43 100644 --- a/src/state.cts +++ b/src/state.cts @@ -52,6 +52,7 @@ import { stateReplaceField, KNOWN_TEMPLATE_DEFAULTS, stateReplaceFieldIfTemplate, + stateCurrentPositionSlice, } from './state-document.cjs'; import { tokenizeHeadings, collectSection, replaceSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; @@ -1367,12 +1368,14 @@ function matchSessionSection(body: string): string | null { * excludes unrelated headings. Built on the same `collectSection` seam as * matchSessionSection, so it inherits that seam's CRLF tolerance (#2444 fix). * Returns the section body, or null (caller falls back to full-body search). + * + * The scoping logic now lives in state-document.cjs's `stateCurrentPositionSlice` + * (the module that owns STATE.md field extraction) — this is a thin alias kept + * for call-site stability. Two copies of this scope would be exactly the kind + * of generative-fix divergence the repo's parity rule exists to prevent. */ function matchCurrentPositionSection(body: string): string | null { - const isCurrentPosition = (h: HeadingToken): boolean => - (h.level === 2 || h.level === 3) && h.text.trim().toLowerCase() === 'current position'; - const section = collectSection(body, isCurrentPosition, { levelBounded: true }); - return section ? section.body : null; + return stateCurrentPositionSlice(body); } /** diff --git a/tests/adr-22-plan-drift-guard.test.cjs b/tests/adr-22-plan-drift-guard.test.cjs index fb868a50a..b3546e862 100644 --- a/tests/adr-22-plan-drift-guard.test.cjs +++ b/tests/adr-22-plan-drift-guard.test.cjs @@ -25,6 +25,7 @@ const { AUTHORITY_RUNGS, getEffectiveAuthority, classifyDriftSeverity, + comparePhaseStatus, } = require('../gsd-core/bin/lib/plan-drift-guard.cjs'); // ── 1. AUTHORITY_RUNGS sanity ──────────────────────────────────────────────── @@ -326,3 +327,277 @@ describe('plan-review-convergence.md uses gsd_run drift-guard seam', () => { ); }); }); + +// ── 6. comparePhaseStatus unit tests (#1956) ──────────────────────────────── + +describe('comparePhaseStatus', () => { + test('equal ranks (STATE vocabulary vs ROADMAP vocabulary) → consistent', () => { + const result = comparePhaseStatus({ stateStatus: 'In progress', roadmapStatus: 'In Progress' }); + assert.equal(result.verdict, 'consistent'); + assert.equal(result.stateRank, result.roadmapRank); + }); + + test('Phase complete vs In Progress → drifted, NOT lag (the issue\'s canonical case)', () => { + const result = comparePhaseStatus({ stateStatus: 'Phase complete', roadmapStatus: 'In Progress' }); + assert.equal(result.verdict, 'drifted'); + assert.notEqual(result.verdict, 'lag', 'a completion disagreement must never be classified as lag, even though the ranks are only 1 apart'); + }); + + test('Phase complete vs Not started → drifted', () => { + const result = comparePhaseStatus({ stateStatus: 'Phase complete', roadmapStatus: 'Not started' }); + assert.equal(result.verdict, 'drifted'); + }); + + test('Ready to plan vs In Progress → lag', () => { + const result = comparePhaseStatus({ stateStatus: 'Ready to plan', roadmapStatus: 'In Progress' }); + assert.equal(result.verdict, 'lag'); + }); + + test('unknown status on either side → uncheckable, and the other side\'s rank still resolves', () => { + const stateUnknown = comparePhaseStatus({ stateStatus: 'Frobnicating', roadmapStatus: 'In Progress' }); + assert.equal(stateUnknown.verdict, 'uncheckable'); + assert.equal(stateUnknown.stateRank, null); + assert.equal(stateUnknown.roadmapRank, 1, 'the resolvable side must still be diagnosable even when the other is unknown'); + + const roadmapUnknown = comparePhaseStatus({ stateStatus: 'Phase complete', roadmapStatus: 'Frobnicating' }); + assert.equal(roadmapUnknown.verdict, 'uncheckable'); + assert.equal(roadmapUnknown.roadmapRank, null); + assert.equal(roadmapUnknown.stateRank, 2, 'the resolvable side must still be diagnosable even when the other is unknown'); + }); + + test('null / undefined / empty-string on either side → uncheckable', () => { + assert.equal(comparePhaseStatus({ stateStatus: null, roadmapStatus: 'In Progress' }).verdict, 'uncheckable'); + assert.equal(comparePhaseStatus({ stateStatus: undefined, roadmapStatus: 'In Progress' }).verdict, 'uncheckable'); + assert.equal(comparePhaseStatus({ stateStatus: '', roadmapStatus: 'In Progress' }).verdict, 'uncheckable'); + assert.equal(comparePhaseStatus({ stateStatus: 'In Progress', roadmapStatus: null }).verdict, 'uncheckable'); + assert.equal(comparePhaseStatus({ stateStatus: 'In Progress', roadmapStatus: undefined }).verdict, 'uncheckable'); + assert.equal(comparePhaseStatus({ stateStatus: 'In Progress', roadmapStatus: '' }).verdict, 'uncheckable'); + }); + + test('case and surrounding whitespace are ignored', () => { + const result = comparePhaseStatus({ stateStatus: ' PHASE COMPLETE ', roadmapStatus: 'Phase complete' }); + assert.equal(result.verdict, 'consistent'); + assert.equal(result.stateRank, 2); + assert.equal(result.roadmapRank, 2); + }); + + test('does not throw for unrecognized input (unlike classifyDriftSeverity)', () => { + assert.doesNotThrow(() => comparePhaseStatus({ stateStatus: 'garbage', roadmapStatus: 'nonsense' })); + assert.doesNotThrow(() => comparePhaseStatus({ stateStatus: undefined, roadmapStatus: undefined })); + }); + + // #1956 review fix: 'Deferred' was missing from PHASE_STATUS_RANKS despite + // gsd-core/templates/roadmap.md:133 declaring it as part of the full + // ROADMAP Status vocabulary (`Not started | In progress | Complete | + // Deferred`) — it silently always resolved 'uncheckable', losing real drift. + test('Deferred vs Not started → consistent (both agree no work has happened)', () => { + const result = comparePhaseStatus({ stateStatus: 'Not started', roadmapStatus: 'Deferred' }); + assert.equal(result.verdict, 'consistent'); + }); + + test('Deferred vs In progress → drifted (declared stopped vs declared happening)', () => { + const result = comparePhaseStatus({ stateStatus: 'In progress', roadmapStatus: 'Deferred' }); + assert.equal(result.verdict, 'drifted'); + }); + + test('Deferred vs Phase complete → drifted (declared stopped vs declared done)', () => { + const result = comparePhaseStatus({ stateStatus: 'Phase complete', roadmapStatus: 'Deferred' }); + assert.equal(result.verdict, 'drifted'); + }); + + test('deferred resolves a real (non-null) rank on either side', () => { + const result = comparePhaseStatus({ stateStatus: 'Not started', roadmapStatus: 'Deferred' }); + assert.notEqual(result.roadmapRank, null, "'deferred' must not be uncheckable — it is a declared vocabulary value"); + assert.equal(result.roadmapRank, 0); + }); +}); + +// Shared ROADMAP.md "Progress" table fixture builder (#1956/#2012). Used by +// BOTH the CLI acceptance tests below (via writeRoadmap, which writes it to +// disk) and the findRoadmapProgressTable/deriveProgressFromRoadmap parity +// test (section 8), so the CLI-level decoy fixture and the parity fixture +// can never independently drift into slightly different shapes. +// +// `opts.decoy`, when true, prepends an earlier `## Archive Notes` section +// carrying a table with the EXACT SAME four column headers +// (`Phase | Plans Complete | Status | Completed`) as the real `## Progress` +// table, with the same phase row reporting a DIFFERENT status ('Not +// started') — the #2012 decoy shape a column-name-only lookup would pick up +// first. +function buildRoadmapProgressContent(phase, phaseName, roadmapStatus, opts = {}) { + const decoySection = opts.decoy + ? `## Archive Notes + +| Phase | Plans Complete | Status | Completed | +|-------|----------------|--------|-----------| +| ${phase}. ${phaseName} | 0/1 | Not started | - | + +` + : ''; + return `# Roadmap: Test Project + +${decoySection}## Progress + +| Phase | Plans Complete | Status | Completed | +|-------|----------------|--------|-----------| +| ${phase}. ${phaseName} | 0/1 | ${roadmapStatus} | - | +`; +} + +// ── 7. #1956 acceptance — drifted phase status across STATE/ROADMAP, via the real CLI ── + +describe('#1956 acceptance — a drifted phase status across STATE/ROADMAP yields a finding', () => { + let tmpDir; + let planningDir; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1956-acceptance-')); + planningDir = path.join(tmpDir, '.planning'); + fs.mkdirSync(planningDir, { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + const PHASE = 3; + const PHASE_NAME = 'Convergence'; + + // Writes .planning/STATE.md carrying frontmatter + a `## Current Position` + // section, matching gsd-core/templates/state.md. + function writeState(stateStatus) { + const content = `--- +gsd_state_version: '1.0' +status: planning +--- + +# Project State + +## Current Position + +Phase: ${PHASE} of 8 (${PHASE_NAME}) +Plan: 1 of 1 in current phase +Status: ${stateStatus} +Last activity: 2026-08-09 — test fixture + +Progress: [░░░░░░░░░░] 0% +`; + fs.writeFileSync(path.join(planningDir, 'STATE.md'), content); + } + + // Writes .planning/ROADMAP.md carrying a `## Progress` section with the + // table shape gsd-core/templates/roadmap.md declares. `opts.decoy` prepends + // a same-headers decoy table under a different heading — see + // buildRoadmapProgressContent above. + function writeRoadmap(roadmapStatus, opts = {}) { + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + buildRoadmapProgressContent(PHASE, PHASE_NAME, roadmapStatus, opts), + ); + } + + test('an intentionally-drifted phase status yields a finding', () => { + writeState('Phase complete'); + writeRoadmap('In Progress'); + const res = runGsdTools(['drift-guard', 'phase-status', '--phase', String(PHASE)], tmpDir); + assert.ok(res.success, `Expected success, got: ${res.error}`); + const result = JSON.parse(res.output); + assert.equal(result.verdict, 'drifted'); + assert.equal(result.authority, 'STATE.md', 'the finding must name STATE.md as authority so the reviewer knows which side to keep'); + }); + + test('consistent artifacts do not', () => { + writeState('Phase complete'); + writeRoadmap('Complete'); + const res = runGsdTools(['drift-guard', 'phase-status', '--phase', String(PHASE)], tmpDir); + assert.ok(res.success, `Expected success, got: ${res.error}`); + const result = JSON.parse(res.output); + assert.equal(result.verdict, 'consistent'); + }); + + test('a missing ROADMAP.md is uncheckable, not consistent', () => { + writeState('Phase complete'); + const res = runGsdTools(['drift-guard', 'phase-status', '--phase', String(PHASE)], tmpDir); + assert.ok(res.success, `Expected success, got: ${res.error}`); + const result = JSON.parse(res.output); + assert.equal(result.verdict, 'uncheckable'); + assert.ok(result.reason && result.reason.length > 0, 'a skipped axis must be observable via a non-empty reason'); + }); + + // #1956 review Fix 1: the Progress-table lookup used to scan the WHOLE + // ROADMAP for the first table matching the column headers, so an earlier + // decoy table sharing those headers (e.g. an "Archive Notes" table) was + // picked up instead of the real `## Progress` table (#2012 decoy + // avoidance). The decoy's phase-3 row says 'Not started'; the real + // `## Progress` table's phase-3 row says 'Complete', agreeing with STATE.md's + // 'Phase complete' — the CLI must read the real table. + test('a decoy table with the same headers is not mistaken for ## Progress', () => { + writeState('Phase complete'); + writeRoadmap('Complete', { decoy: true }); + const res = runGsdTools(['drift-guard', 'phase-status', '--phase', String(PHASE)], tmpDir); + assert.ok(res.success, `Expected success, got: ${res.error}`); + const result = JSON.parse(res.output); + assert.equal(result.verdict, 'consistent'); + assert.equal(result.roadmapStatus, 'Complete', 'must read the real ## Progress table\'s status, not the decoy\'s "Not started"'); + }); + + // #1956 review Fix 2: `stateCurrentPositionSlice(stateBody) ?? stateBody` + // fell back to the whole STATE.md body when no `## Current Position` + // heading was found, reintroducing the #2956 archive-shadowing bug — a + // stray historical `Status:` line elsewhere in the body would be read as + // if it were the real current status, fabricating a drift finding. A + // STATE.md with NO Current Position heading, plus an earlier stray + // `Status: Ready to plan` line, must abstain instead of guessing. + test('a STATE.md with no ## Current Position heading is uncheckable, never a fabricated finding', () => { + const stateContent = `--- +gsd_state_version: '1.0' +status: planning +--- + +# Project State + +Status: Ready to plan + +## Session + +Some unrelated notes with no Current Position section. +`; + fs.writeFileSync(path.join(planningDir, 'STATE.md'), stateContent); + writeRoadmap('Complete'); + const res = runGsdTools(['drift-guard', 'phase-status', '--phase', String(PHASE)], tmpDir); + assert.ok(res.success, `Expected success, got: ${res.error}`); + const result = JSON.parse(res.output); + assert.equal(result.verdict, 'uncheckable'); + assert.equal(result.reason, 'no_current_position'); + assert.notEqual(result.verdict, 'drifted', 'a shadowed/absent Current Position must never fabricate a drift finding'); + }); +}); + +// ── 8. #1956/#2012 parity — findRoadmapProgressTable vs deriveProgressFromRoadmap ── +// +// Two SEPARATE implementations locate "the" ROADMAP Progress table: +// roadmap-parser.cts's findRoadmapProgressTable (collectSection-based, used +// by the drift-guard CLI) and phase-lifecycle.cts's deriveProgressFromRoadmap +// (its own regex-based `## Progress` scope, used by the phase-lifecycle SDK +// handler — deliberately NOT refactored onto the shared owner; its blast +// radius is large). The repo requires a parity assertion whenever a parser is +// expressed twice, so this test feeds both the SAME decoy-bearing ROADMAP +// fixture from section 7 and asserts they agree about which table is real. + +describe('#1956/#2012 parity — findRoadmapProgressTable vs deriveProgressFromRoadmap agree', () => { + const { findRoadmapProgressTable } = require('../gsd-core/bin/lib/roadmap-parser.cjs'); + const { deriveProgressFromRoadmap } = require('../gsd-core/bin/lib/phase-lifecycle.cjs'); + + test('both locators pick the real ## Progress table, not the decoy', () => { + const content = buildRoadmapProgressContent(3, 'Convergence', 'Complete', { decoy: true }); + + const table = findRoadmapProgressTable(content); + assert.ok(table, 'findRoadmapProgressTable must find the real Progress table'); + const row = table.rows.find((r) => r.Phase.startsWith('3.')); + assert.ok(row, 'expected a phase 3 row in the located table'); + assert.equal(row.Status, 'Complete', 'must read the real ## Progress table\'s phase-3 status, not the decoy\'s "Not started"'); + + const progress = deriveProgressFromRoadmap(content); + assert.equal(progress.completedPhases, 1, 'deriveProgressFromRoadmap must count the real ## Progress table\'s Complete row, not be fooled by the decoy'); + }); +}); diff --git a/tests/emitted-drift-acks/1956-cross-artifact-fact-drift.json b/tests/emitted-drift-acks/1956-cross-artifact-fact-drift.json new file mode 100644 index 000000000..cb041da8d --- /dev/null +++ b/tests/emitted-drift-acks/1956-cross-artifact-fact-drift.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "plan-review-convergence.md": "#1956: the plan drift guard gains a second axis — a cross-artifact fact-drift pass sitting immediately after the existing source-grounding pass, under the SAME `plan_review.source_grounding` gate. Growth is the new section only (26195 -> 30672 bytes, +4477); no existing text was rewritten and no new config key was introduced. The addition is deliberately inline rather than extracted: ADR-1610 Decision 4 names eager `@`-import relocation as proxy-gaming (it shrinks the measured file while leaving loaded context unchanged or larger), legitimate extraction is Read-at-step lazy, and this pass has no lazy-read seam — it is prose the orchestrator must already hold when it runs the guard. The file remains far inside its DEFAULT tier hard cap of 40960 bytes (tests/workflow-size-budget.test.cjs), with ~11.4 KB of headroom. Content justification: source-grounding proves a plan's cited SYMBOLS exist in source; nothing proved that the same FACT stated in two planning artifacts still agrees. Because each phase runs in a fresh context an agent typically reads one artifact and trusts it, so a stale duplicate — a phase marked complete in STATE.md but in progress in ROADMAP.md, a success criterion the plan restates with a different outcome, a term used against its CONTEXT.md definition — silently steers it wrong. The pass keys on contradicting knowledge rather than similar-looking text (the false-positive mode the issue's own research comment names), gates on a three-way conjunction, defers the axes gsd-plan-checker already owns (Dimensions 1, 7b, 9) so nothing is reported twice, and is advisory only: it never sets hardBlock and contributes to neither HIGH_COUNT nor ACTIONABLE_COUNT, so a project carrying pre-existing drift can still converge." + } +} diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index 8e62e2829..4d0afb0c8 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -1502,3 +1502,412 @@ describe('bug-936 — plan-review-convergence runs plan-phase inline, not inside }); }); } + +// ───────────────────────────────────────────────────────────────────────────── +// #1956 — Cross-artifact fact-drift pass (second axis of the plan drift guard) +// +// The source-grounding pass answers "does this symbol exist in the source?". +// This pass answers "does the project state the same FACT in two planning +// artifacts, and do the two disagree?" — the DRY hazard the issue names, where +// one copy is updated and the other silently steers a fresh-context agent wrong. +// +// ## What this suite locks +// +// The deployed contract, plus the two Hyrum contracts that are invisible from +// the new section itself and would be silently broken by a plausible edit: +// 1. the pass is ORCHESTRATOR-side, not inside the Agent(prompt=…) string — +// the review agent's return message must end with its two "## " sections +// and carry no others, because the workflow awk-parses them; +// 2. the pass can never reach the convergence gate. Convergence is +// HIGH_COUNT + ACTIONABLE_COUNT == 0, and the workflow's own ACTIONABLE +// definition would otherwise swallow a fact-drift finding — which would +// turn an advisory check into an infinite replan loop for any project that +// already carries drift. +// +// ## What it cannot prove +// +// That the model acts on the text. The subject is an LLM prompt; no test in +// this repo proves behavior for the source-grounding pass either. Stated so the +// coverage claim is honest rather than implied. +// +// The file is read through readFileNormalized, the same LF-normalizing seam the +// rest of this suite uses, so a CRLF checkout cannot skew the offsets. +// ───────────────────────────────────────────────────────────────────────────── + +describe('plan-review-convergence: cross-artifact fact-drift pass (#1956)', () => { + const WORKFLOW = readFileNormalized(WORKFLOW_PATH); + + const DRIFT_HEADING = /^### Cross-artifact fact-drift pass/m; + const GROUNDING_HEADING = /^### Source-grounding pass/m; + const AFTER_AGENT_LINE = /^After agent returns, verify REVIEWS\.md exists/m; + + function offsetOf(content, pattern) { + const m = content.match(pattern); + return m && typeof m.index === 'number' ? m.index : -1; + } + + function driftSpan() { + const start = offsetOf(WORKFLOW, DRIFT_HEADING); + const end = offsetOf(WORKFLOW, AFTER_AGENT_LINE); + assert.ok(start >= 0, 'workflow must define a "### Cross-artifact fact-drift pass" heading'); + assert.ok(end >= 0, 'workflow must retain the "After agent returns…" line that bounds the pass'); + assert.ok(end > start, 'the fact-drift pass must precede the "After agent returns…" line'); + return WORKFLOW.slice(start, end); + } + + /** + * Top-level ordered-list items in a span — the trigger gate's arity, i.e. the + * "flag only when ALL N hold" conjunction. Widening it from 3 to 2 is exactly + * what turns a precise heuristic into a noise generator, so the COUNT is + * asserted rather than the prose. + */ + function countOrderedItems(span) { + const matches = span.match(/^\d+\. /gm); + return matches ? matches.length : 0; + } + + /** + * Conjunction clauses — the indented sub-list under step 3's "FLAG only when + * ALL THREE hold". Counted separately from the STEPS above because they are + * different things: `countOrderedItems` measures the pass's procedure, this + * measures the trigger gate's arity. Asserting one while claiming the other + * is how a weakened gate slips through a green suite. + */ + function countConjunctionItems(span) { + const matches = span.match(/^ {3}\d+\. /gm); + return matches ? matches.length : 0; + } + + describe('the pass exists and extends the drift guard in place', () => { + test('defines a cross-artifact fact-drift pass', () => { + const heading = WORKFLOW.match(/^### Cross-artifact fact-drift pass(.*)$/m); + assert.ok(heading, 'workflow must define a "### Cross-artifact fact-drift pass" heading'); + }); + + test('the pass extends the drift guard, in place', () => { + const grounding = offsetOf(WORKFLOW, GROUNDING_HEADING); + const drift = offsetOf(WORKFLOW, DRIFT_HEADING); + const after = offsetOf(WORKFLOW, AFTER_AGENT_LINE); + assert.ok(grounding >= 0 && drift >= 0 && after >= 0, 'all three anchors must be present'); + assert.ok(grounding < drift, 'the fact-drift pass must follow the source-grounding pass — it extends it'); + assert.ok(drift < after, 'the fact-drift pass must sit before the post-agent REVIEWS.md check'); + }); + + test('the pass region is outside the review-agent prompt', () => { + // Anchor on the review-agent return contract itself — the sentence inside + // the Agent(prompt=…) string that this test exists to protect — rather than + // on a generic mode-argument literal that a later Agent() block could reuse + // and thereby relocate the anchor past this section. + const RETURN_CONTRACT = 'These two sections MUST be the final content of your response'; + const contractAt = WORKFLOW.indexOf(RETURN_CONTRACT); + assert.ok(contractAt >= 0, 'the review agent prompt must still carry its return-message contract'); + assert.strictEqual( + WORKFLOW.indexOf(RETURN_CONTRACT, contractAt + 1), + -1, + 'the return-message contract must appear exactly once — a second copy makes this anchor ambiguous' + ); + assert.ok( + offsetOf(WORKFLOW, GROUNDING_HEADING) > contractAt, + 'source-grounding pass must remain orchestrator-side (after the Agent prompt)' + ); + assert.ok( + offsetOf(WORKFLOW, DRIFT_HEADING) > contractAt, + 'the fact-drift pass must be orchestrator-side — inside the Agent prompt its findings ' + + 'would break the "no additional ## headings" return contract the workflow parses' + ); + }); + }); + + describe('it adds no config surface', () => { + test('the pass is gated on the existing drift-guard key', () => { + assert.match( + driftSpan(), + /plan_review\.source_grounding/, + 'the fact-drift pass must name plan_review.source_grounding as its gate — issue #1956 ' + + 'scopes it as an extension of the existing guard, gated by the existing config' + ); + }); + + test('the pass introduces no new config key', () => { + // The issue's pre-submission checklist asserts the change adds no new + // concept, and its breaking-change mitigation reads "gated behind the + // EXISTING plan_review config". A third key would also force a 25th + // setting into gsd-core/workflows/settings.md's six-section UX. + // + // Two halves, both required. The gate must still be NAMED — otherwise a + // deleted or empty section would satisfy a bare "no novel keys" check + // vacuously — and nothing beyond the two keys the drift guard already owns + // may appear. (`source_grounding_authority` is resolved through + // `gsd_run drift-guard authority` and is not spelled out in this file + // today; it stays on the allowed list so naming it later is not a failure.) + const keys = new Set([...WORKFLOW.matchAll(/plan_review\.([a-z_]+)/g)].map((m) => m[1])); + assert.ok( + keys.has('source_grounding'), + 'the workflow must still name plan_review.source_grounding as the drift-guard gate' + ); + const novel = [...keys] + .filter((k) => k !== 'source_grounding' && k !== 'source_grounding_authority') + .sort(); + assert.deepStrictEqual( + novel, + [], + `plan-review-convergence must introduce no new plan_review key, found: ${novel.join(', ')}` + ); + }); + }); + + describe('the trigger gate is a three-way conjunction', () => { + test('the pass runs four procedure steps', () => { + assert.strictEqual( + countOrderedItems(driftSpan()), + 4, + 'the fact-drift pass must run four steps (deterministic phase-status, pair up judgment ' + + 'facts, judge them, record). This counts the PROCEDURE; the trigger gate arity is ' + + 'counted separately.' + ); + }); + + test('the trigger gate enumerates exactly three conditions', () => { + assert.strictEqual( + countConjunctionItems(driftSpan()), + 3, + 'the fact-drift pass must gate on exactly three AND-ed conditions (same fact named on ' + + 'both sides, the two representations contradict, and the pair is one of the declared ' + + 'authority pairs). Dropping one widens it into a noise generator; adding one silently ' + + 'narrows what it can catch.' + ); + }); + + test('condition counter fires at 2 / 3 / 4', () => { + // The assertion above can only ever observe the real document's arity, so + // its inequality branch never executes. Exercise the counter at + // limit-1 / limit / limit+1 through the SAME function the guard uses, in + // both LF and CRLF form, so a future edit cannot neuter it. + const item = (n) => `${n}. condition ${n}`; + const indentedItem = (n) => ` ${n}. condition ${n}`; + for (const eol of ['\n', '\r\n']) { + const spanOf = (count) => + ['### Cross-artifact fact-drift pass', ...Array.from({ length: count }, (_, i) => item(i + 1))] + .join(eol); + const indentedSpanOf = (count) => + ['### Cross-artifact fact-drift pass', ...Array.from({ length: count }, (_, i) => indentedItem(i + 1))] + .join(eol); + assert.strictEqual(countOrderedItems(spanOf(2)), 2, `2 items must count as 2 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countOrderedItems(spanOf(3)), 3, `3 items must count as 3 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countOrderedItems(spanOf(4)), 4, `4 items must count as 4 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countConjunctionItems(indentedSpanOf(2)), 2, `2 indented items must count as 2 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countConjunctionItems(indentedSpanOf(3)), 3, `3 indented items must count as 3 (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countConjunctionItems(indentedSpanOf(4)), 4, `4 indented items must count as 4 (eol=${JSON.stringify(eol)})`); + // The two counters must not see each other's items — a column-0-only + // span has no conjunction items, and an indented-only span has no + // procedure items. + assert.strictEqual(countOrderedItems(indentedSpanOf(3)), 0, `indented-only span must count 0 procedure items (eol=${JSON.stringify(eol)})`); + assert.strictEqual(countConjunctionItems(spanOf(3)), 0, `column-0-only span must count 0 conjunction items (eol=${JSON.stringify(eol)})`); + } + }); + }); + + describe('severity is advisory, and can never gate convergence', () => { + test('the finding is advisory and never a blocker', () => { + assert.match( + driftSpan(), + /never\s+a\s+blocker/i, + 'the fact-drift pass must state that its finding is never a blocker — issue #1956 asks ' + + 'for an advisory finding, and blocking would strand every project carrying prior drift' + ); + }); + + test('the pass can never gate convergence', () => { + // Convergence is HIGH_COUNT + ACTIONABLE_COUNT == 0, and the workflow's + // own ACTIONABLE definition ("a non-HIGH finding invisible to + // execute-phase unless incorporated into PLAN.md") would otherwise + // swallow a fact-drift finding — making pre-existing drift an infinite + // replan loop. This has to be WRITTEN DOWN, not merely true today. + const span = driftSpan(); + assert.match(span, /HIGH_COUNT/, 'the pass must name HIGH_COUNT when disclaiming the convergence gate'); + assert.match(span, /ACTIONABLE_COUNT/, 'the pass must name ACTIONABLE_COUNT when disclaiming the convergence gate'); + assert.match( + span, + /never sets\s+`?hardBlock/, + 'the pass must state that it never sets hardBlock — the source-grounding pass uses ' + + 'hardBlock to stop the review cycle, and this pass must not inherit that' + ); + }); + + test('findings land in REVIEWS.md', () => { + assert.match( + driftSpan(), + /REVIEWS\.md/, + 'the fact-drift pass must write to REVIEWS.md, matching the source-grounding pass — ' + + 'not to the review agent\'s return message' + ); + }); + + test('the phase-status axis is delegated to the deterministic seam', () => { + const span = driftSpan(); + assert.match( + span, + /gsd_run drift-guard phase-status/, + 'the phase-status axis must be decided by the drift-guard seam, not by model judgment — ' + + 'issue #1956 requires a drifted STATE/ROADMAP pair to yield a finding deterministically' + ); + for (const verdict of [/\bdrifted\b/, /\blag\b/, /\buncheckable\b/]) { + assert.match(span, verdict, `the pass must say how it treats the ${verdict} verdict`); + } + }); + }); + + describe('negative space is enumerated', () => { + test('the pass keys on knowledge, not similar text', () => { + // The maintainer's own research comment on #1956: "The check must key on + // 'same knowledge, drifting representations,' not 'similar-looking text,' + // or it will produce false positives." + const span = driftSpan(); + assert.match(span, /knowledge/i, 'the pass must frame the check in terms of knowledge'); + assert.match( + span, + /contradict/i, + 'the pass must require a CONTRADICTION, not a resemblance — this is the rule that ' + + 'keeps it from firing on every restatement' + ); + }); + + test('the pass enumerates its non-triggering cases', () => { + assert.match(driftSpan(), /Do NOT flag/, 'the fact-drift pass must carry an explicit non-triggering list'); + }); + + test('non-triggering list covers every exclusion class', () => { + const span = driftSpan(); + // Each token is a distinct exclusion class from the design's negative + // space. Their absence is what produces the false positives the issue's + // research comment warns about. + for (const [token, why] of [ + [/wording/i, 'a wording-only difference asserting the same thing is not drift'], + [/single-source/i, 'a fact held in one artifact only is the TARGET state, not a finding'], + [/\bADDS\b/, 'a PLAN may ADD truths beyond the roadmap SCs — only subtraction/contradiction counts'], + [/lifecycle/i, 'STATE trailing ROADMAP by one lifecycle step is lag, not drift'], + [/Deferred Ideas/, 'CONTEXT.md non-authoritative sections must not be compared'], + ]) { + assert.match(span, token, `the non-triggering list must cover: ${why}`); + } + }); + + test('the pass defers overlapping axes to the plan checker', () => { + // Report once, not twice. plan-checker already owns requirement coverage + // (D1), scope reduction (D7b) and cross-plan data contracts (D9). + const span = driftSpan(); + for (const dimension of [/Dimension 1\b/, /Dimension 7b\b/, /Dimension 9\b/]) { + assert.match(span, dimension, `the pass must defer the overlapping axis to ${dimension}`); + } + }); + + test('a completion disagreement is never exempted as lag', () => { + // The issue's canonical example is "complete in STATE.md but in progress + // in ROADMAP.md" — one lifecycle step apart, and exactly the case it wants + // FLAGGED. An exemption phrased purely as "one step apart" would exempt it. + const span = driftSpan(); + assert.match( + span, + /completion[^.]*never lag|never lag|Completeness is terminal/i, + 'the pass must state that a disagreement about completion is never lag' + ); + }); + }); + + describe('authority and coverage', () => { + test('the pass names an authority for every artifact pair', () => { + const span = driftSpan(); + for (const artifact of ['ROADMAP.md', 'PLAN.md', 'STATE.md', 'CONTEXT.md']) { + assert.ok( + span.includes(artifact), + `the fact-drift pass must name ${artifact} — issue #1956 spans all four planning artifacts` + ); + } + assert.match( + span, + /Authority/i, + 'the pass must declare which side of each pair is the source of truth — a finding that ' + + 'names a divergence without naming the authority cannot be acted on' + ); + }); + + test('a skipped axis is recorded, never silent', () => { + const span = driftSpan(); + assert.match(span, /skip(ped)?/i, 'the pass must describe what happens when an artifact is absent'); + assert.match( + span, + /coverage/i, + 'a skipped axis must be recorded in the Verification coverage block — a clean pass must ' + + 'never silently mean "nothing was compared"' + ); + }); + }); + + describe('independence — the surrounding contracts are unchanged', () => { + test('the source-grounding pass keeps its hard block', () => { + const start = offsetOf(WORKFLOW, GROUNDING_HEADING); + const end = offsetOf(WORKFLOW, DRIFT_HEADING); + assert.ok(start >= 0 && end > start, 'source-grounding pass must still precede the fact-drift pass'); + const groundingSpan = WORKFLOW.slice(start, end); + assert.match( + groundingSpan, + /hardBlock: true/, + 'the source-grounding pass must keep its hardBlock gating — #1956 is additive and must ' + + 'not downgrade the existing guard' + ); + }); + + test('the review-agent return contract is unchanged', () => { + assert.match( + WORKFLOW, + /no additional "## " headings after them/, + 'the review agent\'s return-message contract must survive — the workflow awk-parses ' + + 'those sections and a stray "## " heading breaks escalation-detail extraction' + ); + }); + }); + + describe('docs parity', () => { + test('CONFIGURATION.md documents both drift-guard axes', () => { + const configDoc = readFileNormalized(CONFIG_DOC_PATH); + const row = configDoc + .split('\n') + .find((l) => l.startsWith('|') && l.includes('`plan_review.source_grounding`')); + assert.ok(row, 'docs/CONFIGURATION.md must carry a table row for plan_review.source_grounding'); + assert.match( + row, + /cross-artifact|fact drift/i, + 'the plan_review.source_grounding row must document the second (fact-drift) axis — the ' + + 'key now gates two checks, and a reader disabling it must know what else goes dark' + ); + }); + + test('USER-GUIDE.md documents the second axis', () => { + const guide = readFileNormalized(path.join(__dirname, '..', 'docs', 'USER-GUIDE.md')); + const start = guide.indexOf('plan_review.source_grounding: true'); + assert.ok(start >= 0, 'docs/USER-GUIDE.md must retain its Drift Guard section'); + const section = guide.slice(start, start + 4000); + assert.match( + section, + /cross-artifact|fact drift/i, + 'the USER-GUIDE Drift Guard section must describe the cross-artifact fact-drift axis' + ); + }); + + test('ARCHITECTURE.md documents the second axis', () => { + // The issue's Scope of changes names ARCHITECTURE.md explicitly, and its + // existing drift-guard paragraph is the one place the architecture doc + // describes this guard at all — leaving it single-axis would state, in the + // architecture reference, that the guard does less than it does. + const arch = readFileNormalized(path.join(__dirname, '..', 'docs', 'ARCHITECTURE.md')); + const start = arch.indexOf('The plan drift guard (`plan_review.source_grounding`)'); + assert.ok(start >= 0, 'docs/ARCHITECTURE.md must retain its plan drift guard paragraph'); + const section = arch.slice(start, start + 2000); + assert.match( + section, + /cross-artifact|fact-drift/i, + 'the ARCHITECTURE.md drift-guard paragraph must describe the cross-artifact fact-drift axis' + ); + }); + }); +});