diff --git a/.changeset/wise-seals-zip.md b/.changeset/wise-seals-zip.md new file mode 100644 index 000000000..2afeef471 --- /dev/null +++ b/.changeset/wise-seals-zip.md @@ -0,0 +1,7 @@ +--- +type: Changed +pr: 3306 +--- +**Phase completion is now decided by a single disk-strict predicate — a ticked ROADMAP checkbox no longer carries machine authority.** `isPhaseComplete` (`src/verification.cts`) is the one owner: a phase is complete exactly when its `*-VERIFICATION.md` reads `passed`, read unconditionally — plan count is never a precondition. This changes four observable surfaces: `init manager` now reports a zero-plan phase with a passing verification as complete instead of the retired `not_required` sentinel (#3168); `roadmap analyze`'s checkbox override is removed, so a ticked checkbox with outstanding plans or no passing verification now reports incomplete instead of complete; `roadmap update-plan-progress` routes through the same owner (and, unchanged, still refuses to write a completion checkbox/date while any plan lacks a `*-SUMMARY.md`); and `gsd-core/workflows/mvp-phase.md` stops ORing a checkbox-derived `PHASE_COMPLETE` into its completion decision, deciding on disk status alone. A ticked checkbox is not deleted — only its authority over these commands is removed. + +`workstream list`/`workstream status`'s per-phase `complete` status (via `buildWorkstreamInventory`) is now routed through the same owner instead of its own `summaryCount >= planCount`-plus-verification-verdict rule — a zero-plan phase with a passing verification now reports `complete` there too, and a phase whose `*-VERIFICATION.md` is absent no longer reports `complete` on summary count alone (the pre-existing "verifier-disabled projects still complete" tolerance is retired under disk-strict). That tolerance was #2645's deliberate boundary — `missing`/`unknown`/`stale` verdicts counted as non-failing so a project that never runs the verifier would not report 0% forever. Disk-strict retires it and closes #2645's Goodhart hole from the other side: deleting a `*-VERIFICATION.md` now lowers the reported completion instead of raising it. A project that does not run the verifier will report its phases incomplete. (#3186) diff --git a/CONTEXT.md b/CONTEXT.md index 22f25daf7..02370623e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -24,7 +24,7 @@ Module owning phase create, rename, complete, remove, list, and plan-index opera Module owning phase-effort estimation and its calibration against measured reality (ADR-2629, epic #1952). Pure — no I/O, no config reads; the CLI seam (`src/estimate-cli.cts`, verbs `estimate-check` / `estimate-calibration`) owns reading `.planning/config.json` and `.planning/estimation-calibration.json`. Interface: `parseEstimate`/`renderEstimate` (the PLAN.md `estimate: {tokens, tasks, confidence}` block), `parseActuals`/`renderActuals` (the SUMMARY.md `actuals: {tokens, tasks, commits}` block), `deriveConfidence(sampleCount) → low|med|high`, `classifyAgainstBudget(estimate, budget) → {overBudget, ratio, recommendation, budgetValid}`, `computeCalibration(samples) → {factor, sampleCount, applied, confidence, clamped}`, `applyCalibration`, `parseCalibrationDocument`/`renderCalibrationDocument`, `extractFrontmatterBlock` (leading-`---`-anchored scalar-block reader; hand-rolled because core ships no external deps), `calibrationBasis` (returns `estimate.raw_tokens` when present, else `tokens` — calibration must measure actual/raw or the loop un-corrects itself), and `measureTokens` (a re-export of `prompt-budget`'s `estimateTokens`). **Domain terms: _raw_ vs _calibrated_ tokens** — the same token count in two mutually incompatible states, carried by the compile-time brands `RawTokens` (the planner's uncorrected projection, and the only legal calibration denominator) and `CalibratedTokens` (the projection with the project's factor applied, and the only figure meaningful against the budget), constructed at trust boundaries via `asRawTokens` / `asCalibratedTokens` (#2671). The brands erase at compile time — the emitted `.cjs`, the CLI JSON, and both frontmatter schemas are unchanged — and exist because mixing the two states was NOT catchable at runtime: both are positive integers of the same magnitude, and the mix-up shipped twice past a green ~26,800-test suite (#2631 factor², #2632 self-defeating loop). `asRawTokens` refuses a `CalibratedTokens` by design; the single legitimate crossover (a pre-#2632 plan whose `tokens` IS the raw projection) lives behind one commented assertion in `calibrationBasis`. Compile fixtures: `tests/fixtures/brand-typing/`. **_smart zone_** — the usable prefix of a model's context window before output quality degrades, expressed as the configurable `workflow.smart_zone_tokens` budget (default 100000, a *policy default* rather than a benchmark constant since the effective ceiling is model/task-dependent); **_estimate/actuals_** — a projected phase cost recorded at plan time and the measured cost recorded at completion, both on the **same `estimateTokens` scale** so their ratio measures the miss rather than a difference between two measurement methods. Two invariants: (1) every signal is **exogenous** — the correction routes on a measured actual/estimate ratio and `confidence` routes on a calibration sample count, never on a model's self-assessment (this project measured self-rated confidence and found it weak — `gsd-core/references/honest-verifier.md:25-29`; see `.out-of-scope/general-purpose-agent-prompt-skills.md`); (2) the over-budget flag is **advisory** — a warning plus a split recommendation, never a block. Calibration is median-of-ratios, clamped to `[0.5, 3.0]`, and inert below 3 samples. CLI seam verbs: `estimate-check` (classify one figure; `--calibrated` when the input already has the factor applied — omitting it squares the correction), `estimate-calibration` (report the current factor), `estimate-calibrate` (#2632 — pair every completed phase's PLAN `estimate` with its SUMMARY `actuals`, rebuild `.planning/estimation-calibration.json` idempotently, and report the result; this is what closes the loop). Source of truth: `gsd-core/bin/lib/phase-estimation.cjs` (generated from `src/phase-estimation.cts`) and `src/estimate-cli.cts`. Test anchors: `tests/phase-estimation.test.cjs`, `tests/estimate-calibrate.test.cjs`. ### Verification Module -Module owning the canonical phase-verification status projection shared by phase transition, progress, manager, autonomous, and closeout readiness paths. `readVerificationStatus(phaseDir, opts?)` reads the first `*-VERIFICATION.md` frontmatter `status`, maps it through `VERIFICATION_ROUTING_TABLE`, and fail-closes — only `{passed}` satisfies the canonical gate; `missing`/`unknown`/`gaps_found`/`human_needed`/`stale` all route away from "complete" (#1522). `findStaleVerificationSummary` flags a SUMMARY newer than the VERIFICATION file (status `stale`). Both honor a no-throw, degrade-to-safe contract (any FS error → `missing` / not-stale) and an injectable `opts.fs` seam. Source of truth: `gsd-core/bin/lib/verification.cjs` (generated from `src/verification.cts`). +Module owning the canonical phase-verification status projection shared by phase transition, progress, manager, autonomous, and closeout readiness paths. `readVerificationStatus(phaseDir, opts?)` reads the first `*-VERIFICATION.md` frontmatter `status`, maps it through `VERIFICATION_ROUTING_TABLE`, and fail-closes — only `{passed}` satisfies the canonical gate; `missing`/`unknown`/`gaps_found`/`human_needed`/`stale` all route away from "complete" (#1522). `findStaleVerificationSummary` flags a SUMMARY newer than the VERIFICATION file (status `stale`). Both honor a no-throw, degrade-to-safe contract (any FS error → `missing` / not-stale) and an injectable `opts.fs` seam. `isPhaseComplete(phaseDir, deps?)` is the single canonical owner of "is phase P complete?" (ADR-3180 §7.4, issue #3186, disk-strict per #2957): it wraps `readVerificationStatus`, calling it UNCONDITIONALLY — plan count is never a precondition, so a zero-plan phase with a passing `*-VERIFICATION.md` is complete (#3168) — and returns `{ value: { complete, verification }, scope }`; `complete` is exactly `verification.status === 'passed'`. A ROADMAP checkbox carries no machine authority and is never consulted. `cmdPhaseComplete`, `buildPhaseCompletionProjection`, and `buildStateFrontmatter` all route through it. Source of truth: `gsd-core/bin/lib/verification.cjs` (generated from `src/verification.cts`). ### Phase Locator Module Module owning phase-directory search and location: active-phase discovery against the `.planning/phases/` tree (`searchPhaseInDir`, `findPhaseInternal`) and archived-phase-dir enumeration (`getArchivedPhaseDirs`), matching phase ids/tokens against the filesystem. Depends only on leaf modules (`phase-id` for token/name matching, `core-utils` for fs-scan/path helpers, `planning-workspace` for `planningDir`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2d (#881); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/phase-locator.cjs` (generated from `src/phase-locator.cts`). Since #2830, `searchPhaseInDir` also parses each plan's `depends_on` and each completed plan's SUMMARY `status` and calls Plan Dependency Graph Module's `computeHaltPropagation` to populate `halted_plans`/`blocked_by`/`runnable_plans` — additive fields; `incomplete_plans` keeps its pre-#2830 meaning unchanged. Since #3185 (ADR-3180 Decision 1, Phase 3), the module also owns `listMilestonePhaseDirs(phasesDir, { cwd, ws, versionOverride, phaseIdConvention })`, the single canonical owner of milestone-scoped phase-directory enumeration: it applies the current milestone's `ROADMAP.md` window (via `getMilestonePhaseFilter`) and then the canonical `isSentinelPhaseId` sentinel filter, in that order, over the raw `phasesDir` directory listing. It returns `{ value: string[], scope }`, where `scope` is the `SCOPE` enum from `src/planning-scope.cts` (`complete`/`truncated`/`unscoped`/`unreadable`), so a caller can distinguish a genuinely empty milestone from an enumeration that could not be scoped. Consumed by `query progress`, `stats`, and the bare `phases list`, all of which need "which phases belong to this milestone." `phases list --phase` and `--include-archived` (lookup/archive questions) read the unscoped physical directory set and do not call this owner. `phases clear` and `milestone complete`'s phase-archival move call `isSentinelPhaseId` directly instead — they must sweep every non-sentinel phase directory regardless of milestone window, so they take the sentinel filter without this owner's window scoping. diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 9b0a7a253..a047906fd 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -721,6 +721,8 @@ Interactive command center for managing multiple phases from one terminal. /gsd-manager --analyze-deps # Scan ROADMAP phases for dependency relationships before parallel execution ``` +**Phase completion is disk-strict (ADR-3180 §7.4, issue #3186).** A phase's status here — and in `roadmap analyze`, `roadmap update-plan-progress`, and `phase complete` — is decided by one rule: a passing `*-VERIFICATION.md` on disk, checked unconditionally (plan count is never a precondition, so a zero-plan phase with a passing verification reports complete). A ticked `- [x]` checkbox in `ROADMAP.md` is a human annotation only; it carries no machine authority and is never consulted for these commands' completion verdicts. `roadmap update-plan-progress` additionally withholds writing the checkbox/completion date while any plan in the phase has no matching `*-SUMMARY.md`, mirroring `phase complete`'s own coverage gate. + **Checkpoint Heartbeats (#2410):** Background `execute-phase` runs emit `[checkpoint]` markers at every wave and plan diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index 00cd1359e..cd0df6cb3 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -278,7 +278,7 @@ This section is that written rule. It is **normative**, and it is what the guard **Note on #3204.** Routing `buildStateFrontmatter` through the owner will not by itself fix #3204: its defect is the discriminator one layer *above* enumeration — "is the ROADMAP's phase count safe to trust" — which misclassifies ordinary `## Overview` / `## Progress` headings as milestone sectioning. That discriminator **is** §7.1's `isMilestoneBoundedInRoadmap`. The enumeration routing and the discriminator replacement must ship together or the defect survives the consolidation. -#### 7.4 Phase completion — *Required — Phase 4, blocked* +#### 7.4 Phase completion — *Required — Phase 4 (decided: disk-strict)* **Question.** Is phase `P` complete? @@ -286,9 +286,29 @@ This section is that written rule. It is **normative**, and it is what the guard **Rule.** `readVerificationStatus` is called **unconditionally**. Plan count is not a precondition: a phase with zero plans and a passing `*-VERIFICATION.md` is complete. The read path and the write path share this predicate, so "`phase.complete` succeeds while `init.manager` reports incomplete" is unrepresentable for identical input. -**OPEN QUESTION — does a ROADMAP checkbox override disk state? (#2957).** There are **three** completion implementations, not the two the epic recorded: `cmdPhaseComplete`, `buildPhaseCompletionProjection`, and `buildStateFrontmatter`, which computes completed phases from plan scanning alone and never consults the ROADMAP checkbox that `cmdRoadmapAnalyze` deliberately honors. Checkbox-override versus disk-strict is a **product decision**, and it is not made here. +**DECIDED — disk state is authoritative; a ROADMAP checkbox does not override it (#2957, maintainer decision 2026-08-08).** This replaces the OPEN QUESTION this section previously carried, per §7's own rule that a behavior not stated here is not decided. -**Forcing function.** Phase 4's drift guard fails while more than one completion predicate exists. It cannot be satisfied by consolidating two of three and leaving the third, and Phase 4 must not ship before #2957 is decided — a shared predicate that silently adopts whichever semantics its author happened to hold is a product decision made by typing order. +There are **three** completion implementations, not the two the epic recorded: `cmdPhaseComplete`, `buildPhaseCompletionProjection`, and `buildStateFrontmatter`, which computes completed phases from plan scanning alone and never consults the ROADMAP checkbox that `cmdRoadmapAnalyze` deliberately honors. The resolution: + +1. **A ticked ROADMAP checkbox is a human annotation with no machine authority.** `cmdRoadmapAnalyze`'s deliberate honoring of it is the **divergent** behavior here, and it is **removed** rather than generalized. +2. `buildStateFrontmatter`'s existing plan-scanning semantics therefore become **canonical**, and all three implementations route through the one predicate. +3. The shared predicate derives completion from plan and verification state **on disk**, per the Rule above — `readVerificationStatus` unconditionally, plan count not a precondition. + +**Why disk-strict.** A stale tick asserting completion over contradicting disk state is precisely the confidently-wrong answer this epic exists to remove, and it is the same failure shape as the `100`-percent aggregate (#3161): a well-formed, plausible value that no caller can distinguish from a real one. + +**Tier-2 consequence (Decision 3).** A phase marked complete *solely* by a ticked checkbox — no passing `*-VERIFICATION.md`, plans outstanding — **stops reporting complete from `roadmap analyze`**. The break is deliberate and ships with its own breaking-change call-out, changeset fragment and docs update in Phase 4's PR. + +**A missing verdict is not a passing one (decided 2026-08-10, maintainer).** `isPhaseComplete` is `verification.status === 'passed'`, so an **absent** `*-VERIFICATION.md` means **not complete** — everywhere, including `workstream list` / `status` / `progress`. + +This retires #2645's deliberate boundary, which kept `missing` / `unknown` / `stale` out of the failing set specifically so a verifier-disabled project would not report 0% forever. Recorded rather than absorbed silently, because the consequence is real: **a project that never runs the verifier now reports its phases incomplete.** + +It also closes #2645's Goodhart hole from the opposite side. That issue's symptom was that *deleting* a `*-VERIFICATION.md` **raised** the reported percentage — the metric could be improved by destroying the evidence. Under disk-strict, deleting it **lowers** completion, so the incentive inverts and the ledger's deletion-memory role is no longer load-bearing. + +**Forcing function.** Phase 4's drift guard fails while more than one completion predicate exists. It cannot be satisfied by consolidating two of three and leaving the third. + +**Consumers that must route through the owner.** `cmdPhaseComplete`, `buildPhaseCompletionProjection`, `buildStateFrontmatter` (the three #2957 names), plus `cmdRoadmapAnalyze`, `cmdInitManager`, `cmdRoadmapUpdatePlanProgress`, `buildWorkstreamInventory`, and the prompt layer's `mvp-phase.md` — **nine re-derivations found by the guard where this section named three.** + +**A write-path gate is not a second predicate.** `cmdRoadmapUpdatePlanProgress` writes a completion checkbox, and completion alone does not mean every plan was executed — `readVerificationStatus`'s staleness check compares summary mtimes, never plan count. It therefore ANDs the owner's verdict with an explicit plan-coverage gate, mirroring the separate #2648 unexecuted-plan gate `cmdPhaseComplete` already carries. That composition is sanctioned; re-deriving completion beside it is not. #### 7.5 Live-plan counting — *Enforced (Phase 1), with a known representation gap* @@ -377,7 +397,7 @@ One row per derivation. A blank owner is a derivation whose contract is locked ( | Milestone windowing (§7.1) | `roadmap-parser.cts` | `lint-milestone-window-drift.cjs` | `src/` | enforced | | Milestone identity (§7.2) | `roadmap-parser.cts` (Phase 6) | same guard, token set widened by Phase 6 | `src/` | enforced | | Phase enumeration (§7.3) | `phase-locator.cts` | `lint-phase-enumeration-drift.cjs` | `src/` | enforced — the state writers included (#3185) | -| Phase completion (§7.4) | `verification.cts` (Phase 4) | Phase 4 | `src/` | blocked on #2957 | +| Phase completion (§7.4) | `verification.cts` (Phase 4) | Phase 4 | `src/` | contract decided (disk-strict, #2957); migration is Phase 4 | | Live-plan counting (§7.5) | `plan-scan.cts` | `lint-plan-count-drift.cjs` | `src/` | enforced | | Live-plan counting, prompt layer (§7.5) | — (Phase 8) | `lint-planning-prompt-drift.cjs` | `gsd-core/workflows`, `commands`, `agents`, `skills` | ratcheted, 7 sites | | Completion ratio (§7.6) | `phase-lifecycle.cts` | `lint-completion-ratio-drift.cjs` | `src/` | arithmetic + rule 3 enforced; rule 4 is Phase 7 | diff --git a/gsd-core/workflows/mvp-phase.md b/gsd-core/workflows/mvp-phase.md index 7ed310365..85b2ef150 100644 --- a/gsd-core/workflows/mvp-phase.md +++ b/gsd-core/workflows/mvp-phase.md @@ -41,12 +41,15 @@ PHASE_FOUND=$(echo "$PHASE_INFO" | jq -r '.found') PHASE_NAME=$(echo "$PHASE_INFO" | jq -r '.phase_name') PHASE_GOAL=$(echo "$PHASE_INFO" | jq -r '.goal') PHASE_MODE=$(echo "$PHASE_INFO" | jq -r '.mode // ""') -PHASE_COMPLETE=$(echo "$PHASE_INFO" | jq -r '.roadmap_complete // false') - ANALYZE=$(gsd_run query roadmap.analyze) if [[ "$ANALYZE" == @file:* ]]; then ANALYZE=$(cat "${ANALYZE#@file:}"); fi DISK_STATUS=$(echo "$ANALYZE" | jq -r --arg p "$PHASE" '.phases[] | select((.phase_number|tostring)==$p) | .disk_status' | head -1) -if [[ "$DISK_STATUS" == "complete" || "$PHASE_COMPLETE" == "true" ]]; then +# ADR-3180 §7.4 (issue #3186, disk-strict, #2957): DISK_STATUS alone decides +# completion — a ROADMAP checkbox carries no machine authority and is never +# ORed in here. `roadmap.analyze`'s `disk_status` already routes through the +# canonical owner (`isPhaseComplete`), so this is the same predicate the read +# and write paths both use. +if [[ "$DISK_STATUS" == "complete" ]]; then STATUS="completed" elif [[ "$DISK_STATUS" == "planned" || "$DISK_STATUS" == "partial" ]]; then STATUS="in_progress" diff --git a/package.json b/package.json index 66f2a3e00..1c089698d 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 && node scripts/lint-state-field-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 && node scripts/lint-completion-predicate-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-completion-predicate-drift.cjs b/scripts/lint-completion-predicate-drift.cjs new file mode 100644 index 000000000..e37512427 --- /dev/null +++ b/scripts/lint-completion-predicate-drift.cjs @@ -0,0 +1,932 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Anti-divergence drift guard for the PHASE-COMPLETION predicate (epic + * #3180, issue #3186, ADR-3180 Decision 4, spec §7.4). + * + * The derivation this guard protects is "is phase P complete?", under the + * DISK-STRICT rule §7.4 now locks (amended on this branch, commit + * af92fd4c9, per the #2957 maintainer decision): `readVerificationStatus` + * is called UNCONDITIONALLY (no plan-count precondition), and a ticked + * ROADMAP checkbox carries NO machine authority over disk state. The + * designated owner is `src/verification.cts` · `isPhaseComplete` (Decision + * 1). Per ADR-3180 Amendment 3's standing rule this guard was written BEFORE + * any src/ file was touched (guard-first discovery), with + * `FUNCTION_SCOPED_EXEMPTIONS` below INTENTIONALLY EMPTY at that point. The + * implementation step (issue #3186) has since landed `isPhaseComplete` and + * migrated its call sites onto it, and populated the map — see its own + * comment immediately above its definition for the per-entry reasoning, + * including two DECLARED DEVIATIONS (`buildPhaseCompletionProjection`'s + * retained `implementation_complete` and `buildWorkstreamInventory`'s + * pure-projection boundary) not named by the design doc's own + * DO-NOT-MIGRATE list. + * + * Per ADR-3180 Decision 4(a) this guard discovers call sites by SCANNING THE + * WHOLE `src/` TREE, never an allowlist of known files. Per Decision 4(d) + * that surface is widened further: `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`) — see the + * "PROMPT-LAYER PROSE DETECTION" section below. + * + * FOUR INDEPENDENT SHAPES are detected, matching issue #3186's dispatch (shape + * (d) added by the Phase 4 follow-up that closed the `cmdStateSync` / + * `src/state.cts` gap the remote matrix exposed — see shape (d)'s own header + * below): + * + * (a) CHECKBOX-DERIVED COMPLETION — a ROADMAP `- [x] Phase N` tick treated + * as completion evidence. Categorically wrong under disk-strict + * (§7.4's DECIDED resolution: "a ticked ROADMAP checkbox is a human + * annotation with no machine authority"). Detected as the PAIRED + * shape actually observed twice in this tree: an `if` statement whose + * condition references a roadmap/checkbox-derived "complete" boolean + * AND tests some status variable `!== 'complete'`, followed (within a + * small bounded window — this pairs ONE conditional with its OWN + * consequent statement, the same tight-pairing role + * `lint-state-field-drift.cjs`'s `LADDER_WINDOW_LINES` plays, NOT the + * big function-scoped co-occurrence Decision 4(a)'s Goodhart lesson + * targets) by an assignment of that SAME variable to the literal + * `'complete'`. + * (b) PLAN-COUNT PRECONDITION GATING A VERIFICATION READ — §7.4 names this + * exactly as `buildPhaseCompletionProjection`'s divergence: a + * `planCount > 0`-shaped gate that decides WHETHER + * `readVerificationStatus(` runs at all, rather than calling it + * unconditionally. Detected as FUNCTION-SCOPED co-occurrence (no + * window — Decision 4(a)'s Goodhart lesson, and Phase 5's "bounded + * window missed 7 of 14 copies" trap) of a count-gate ANYWHERE in the + * function's own body, together with a ternary- OR block/`if`-gated + * call to `readVerificationStatus(` (a call that is NOT the function's + * unconditional top-level statement) — the ternary form is a same-line + * `?` before the call; the block form is a call sitting inside the + * BODY of an `if (...)` whose OWN condition matches the count-gate + * (`findIfCountGateBlockLines`), which also catches `if (planCount > + * 0) { x = readVerificationStatus(...) }` — a #3186 review finding + * (4a): the prior version matched only the same-line ternary and + * produced zero hits on the block form. Known, disclosed limit: a + * multi-line `if` condition whose opening `{` lands on a later line + * than the condition's own closing `)` is not detected (every + * count-gate condition actually present in this tree is one line). + * (c) LOCAL RE-IMPLEMENTATION OF "COMPLETE" FROM COUNTS — a + * `summaryCount >= planCount`-shaped comparison (either operand + * order, or the algebraic `summaryCount - planCount >= 0` restatement + * and its mirror), computed locally instead of calling the owner. + * Known, disclosed limit: further algebraic restatements (an + * intermediate difference variable, `Math.max`/`!()`-wrapped forms, + * etc.) are not reliably matchable by a bounded, non-backtracking + * regex and are not attempted — see the regexes' own comment. This is + * the single sharpest textual signature this epic's divergent copies + * share: `buildPhaseCompletionProjection`, `buildStateFrontmatter` + * (via `scanPhasePlans`'s own `completed` field), + * `cmdRoadmapAnalyze`, `cmdRoadmapUpdatePlanProgress`, and + * `buildWorkstreamInventory` all independently hand-roll this exact + * comparison. + * (d) A BARE FIELD READ OF `scanPhasePlans(...).completed` OUTSIDE THE + * OWNER (`src/plan-scan.cts`) — the shape the remote matrix exposed: + * `cmdStateSync` (`src/state.cts`, now fixed) destructured + * `scanPhasePlans(dirPath).completed` directly and used it AS a + * completion verdict, with no comparison for shapes (a)/(b)/(c) to + * catch — a bare property read is not a re-derivation shape any of the + * other three detectors match. `scanPhasePlans` legitimately EXPOSES + * `.completed` (it is Phase 1's own owner for "are all plans + * summarized?", a real and distinct question from "is the phase + * complete?"), so reading the field is not inherently wrong — USING it + * as a completion verdict is. That distinction is DATA-FLOW (what the + * caller does with the value), which no textual guard can decide. + * KNOWN, DISCLOSED, HONEST LIMIT: this detector cannot tell a + * legitimate "are summaries caught up with plans" read from an illegal + * "is the phase complete" read — it flags EVERY read of `.completed` + * off a `scanPhasePlans(` result outside `src/plan-scan.cts` and + * relies on `FUNCTION_SCOPED_EXEMPTIONS` carrying a WRITTEN REASON per + * exempted function (see that map's own comment for each current + * entry's reasoning) rather than silently deciding the question for + * itself. Detected in three independent textual forms, ALL + * function-scoped (Decision 4(a), no line window — see below): + * - the direct chained form, `scanPhasePlans(...).completed`, on one + * statement — the call's own argument list is walked with a plain + * paren-depth counter (not a `[^)]*` regex), so a nested-paren + * argument (`scanPhasePlans(path.join(phasesDir, dir))`, the + * majority real shape in this tree) is still matched correctly; + * - the destructured form, `const { completed } = scanPhasePlans(…)` + * or the renamed-alias form `const { completed: isDone } = + * scanPhasePlans(…)`, on one statement; + * - the INDIRECT form — `const scan = scanPhasePlans(…)` binding the + * whole result to a variable, with `.completed` read off that same + * variable ANYWHERE else in the SAME named function, including on a + * different line — no bounded window (the same Phase-5 "bounded + * window missed 7 of 14 copies" lesson shape (b) already learned; + * `cmdStateSync`'s own real shape bound the result to a + * destructured `{ planCount: plans, summaryCount: summaries }` + * rather than a whole-object variable, so this indirect form is + * precautionary breadth for a shape not yet observed in this tree, + * not a shape already caught in the wild). + * + * DETECTION IS FUNCTION-SCOPED, NOT A BOUNDED LINE-WINDOW, for the + * *discovery* co-occurrence in each shape (shape (a)'s if/assignment pairing + * and shape (b)'s ternary-clause pairing are each ONE conditional's own two + * halves — the same narrow, unavoidable pairing `LADDER_WINDOW_LINES` bounds + * in the sibling state-field guard, not a re-derivation-hiding Goodhart + * target). Phase 5's first guard used a bounded line window between a + * ladder and its consuming call and MISSED 7 of 14 live copies inside the + * very function it scanned — that lesson is why shape (b)'s outer + * count-gate/gated-call pairing has NO line-distance bound at all: both + * signals need only appear somewhere in the SAME named function. + * + * COMMENT-AWARE. Phase 3's first `lint-phase-enumeration-drift.cjs` flagged + * JSDoc/inline comments that merely DOCUMENTED the derivation, not code that + * re-derives it (ADR-3180 Amendment 3). This guard reuses the SAME + * comment/string-stripping tokenizer `lint-state-field-drift.cjs` proved + * (`scanCode` below): comments and string/template literal CONTENTS are + * blanked before any detection regex runs, cross-line-aware for block + * comments and multi-line template literals. + * + * FUNCTION-SCOPED EXEMPTIONS ONLY, NEVER A WHOLE-FILE ALLOWLIST (Decision + * 4(a)/(d)): a whole-file exemption on the owner is precisely how + * `getMilestoneInfo` stayed invisible to an earlier guard (Decision 4(d)). + * `FUNCTION_SCOPED_EXEMPTIONS` is a `Map>`, kept + * EMPTY here because the owner (`src/verification.cts` · `isPhaseComplete`) + * does not exist yet — this comment, not a populated map, is what the + * implementation step (Phase 4's migration PR) replaces. + * + * Every regex below is small, bounded, and has no nested/overlapping + * quantifiers — the same ReDoS discipline `npm run lint:ci`'s CodeQL + * js/redos query verifies over every sibling drift guard. + * + * The tree-walk / root-confinement / sanitizer machinery is SHARED with the + * sibling drift guards via `scripts/lib/drift-scan.cjs` (ADR-3180 Decision + * 4). + */ + +const path = require('node:path'); +const driftScan = require('./lib/drift-scan.cjs'); +const { MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree } = driftScan; + +// ─── SHARED TOKENIZER + FUNCTION ATTRIBUTION (mirrors lint-state-field-drift.cjs) ── +// +// Two parallel per-line views from ONE single-pass, escape-aware character +// scan (not a regex — nothing for a backtracking engine to explore): +// - `detect[i]`: comments stripped, string/template CONTENTS kept verbatim +// (this is what every detection regex below runs against). +// - `braces[i]`: comments AND string/template CONTENTS stripped, used only +// for brace-depth counting, so a brace inside a string/comment never +// perturbs the depth count. +// `inBlockComment` / `inTemplate` are threaded ACROSS lines. Regex literals +// are not specially recognised — same documented, narrow, known limitation +// as the sibling guards (harmless for every regex literal actually present +// in this repo's completion-predicate call sites today, each balanced on +// its own line). +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); + if (inTemplate) { + detect[li] = outDetect; + braces[li] = ''; + 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; + 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; + continue; + } + if (line[j] === quote) { + j++; + break; + } + j++; + } + outDetect += line.slice(start, j); + 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); + inTemplate = true; + i = line.length; + break; + } + outDetect += line.slice(start, j); + i = j; + continue; + } + outDetect += ch; + outBraces += ch; + i++; + } + detect[li] = outDetect; + braces[li] = outBraces; + } + return { detect, braces }; +} + +// Named function scope openers (mirrors lint-state-field-drift.cjs exactly): +// - `function NAME(...) {` — top-level OR nested, any indentation, an +// optional leading `export ` tolerated by `\b` alone. +const FUNCTION_DECL_RE = /\bfunction\s+([A-Za-z_$][\w$]*)\s*\(/; +// - `const NAME = (...): ReturnType => {` — block-bodied arrow assigned to +// a const (an expression-bodied arrow `=> ({...})` never opens a new +// function frame; its `{` is an object literal, still counted toward +// brace depth, but attributes no name). +const ARROW_CONST_RE = /\bconst\s+([A-Za-z_$][\w$]*)\s*=\s*\([^)]*\)\s*(?::\s*[^=]+)?=>\s*\{/; + +/** + * Pure: walk `lines` once, maintaining a brace-depth stack of open named + * function frames (deferring a multi-line `function NAME(` signature until + * the line whose brace count actually increases — see + * `lint-state-field-drift.cjs`'s `buildFunctionInfo` header for the full + * rationale this mirrors verbatim). Returns `{ innermostAt, detect }`: + * `innermostAt[i]` is the name of the innermost named function open at line + * `i` (or `null` at module scope), `detect[i]` is the comment/string- + * preserving-but-stripped-of-comments view every detector regex runs + * against. + */ +function buildFunctionInfo(lines) { + const { detect, braces } = scanCode(lines); + const innermostAt = new Array(lines.length).fill(null); + const stack = []; // { name, openDepth } + let depth = 0; + let pendingDeclName = null; + 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); + 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) { + stack.push({ name: pendingDeclName, openDepth: depth }); + pendingDeclName = null; + } else if (detectCode.includes(';')) { + 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; + } + return { innermostAt, detect }; +} + +/** + * §7.4/#3186 review finding 4(a): the BLOCK form of shape (b)'s gate — `if + * (planCount > 0) { … readVerificationStatus(…) … }` — is textually + * indistinguishable from an unrelated `if` block by a same-line regex; the + * prior ternary-only `GATED_VERIFICATION_READ_RE` produced zero hits on it. + * Deliberately narrower than a generic "is this line nested at all" check + * (which would false-positive on every unrelated wrapper — a `try` block, a + * `withPlanningLock(cwd, () => { … })` callback, a `for` loop — none of + * which are a plan-count GATE): only lines inside the BODY of an `if (...)` + * whose OWN condition matches `COUNT_GATE_RE` are marked. Line-granularity + * brace bookkeeping (mirrors `buildFunctionInfo`'s own style), not a + * per-character brace matcher — a multi-line `if` condition whose `{` lands + * on a later line than `extractIfCondition`'s reported `endLine` is a known, + * disclosed limitation (real count-gates in this tree are one-line + * conditions; see the module header). + */ +function findIfCountGateBlockLines(lines) { + const { detect, braces } = scanCode(lines); + const gated = new Array(lines.length).fill(false); + const stack = []; // { openDepth } for open if-blocks whose condition is a count-gate + let depth = 0; + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + const braceCode = braces[i]; + + let isCountGateIfHeader = false; + if (detectCode.trim()) { + const ifMatch = IF_OPEN_RE.exec(detectCode); + if (ifMatch) { + const startCol = ifMatch.index + ifMatch[0].length; + const condition = extractIfCondition(detect, i, startCol); + if (condition && COUNT_GATE_RE.test(condition.text)) isCountGateIfHeader = true; + } + } + + const opens = (braceCode.match(/\{/g) || []).length; + const closes = (braceCode.match(/\}/g) || []).length; + depth += opens - closes; + + // A line is "inside" a count-gate if-block when either a PRIOR line + // already opened one and it has not yet closed, or THIS line's own `if` + // header both matches the gate and opens its body on the same line + // (`if (planCount > 0) { … }` — the exact shape in the finding). + gated[i] = stack.length > 0 || (isCountGateIfHeader && opens > closes); + + if (isCountGateIfHeader && opens > closes) stack.push({ openDepth: depth }); + + while (stack.length > 0 && depth < stack[stack.length - 1].openDepth) stack.pop(); + } + return gated; +} + +// ─── SHAPE (a): CHECKBOX-DERIVED COMPLETION ──────────────────────────────── +// +// The `if` half of the pairing: a condition referencing a roadmap/checkbox- +// derived "complete" boolean (`roadmapComplete`, `roadmap_complete`, any +// `\w*roadmap\w*complete\w*` spelling — case-insensitive, both real sites in +// this tree use exactly `roadmapComplete`) AND testing some OTHER status +// variable against the literal `!== 'complete'` (single or double quotes). +const IF_OPEN_RE = /\bif\s*\(/; +const ROADMAP_COMPLETE_IDENT_RE = /\broadmap\w*complete\w*\b/i; +const STATUS_NEQ_COMPLETE_RE = /\b([A-Za-z_$][\w$]*)\s*!==\s*['"]complete['"]/; + +// The consequent half: an assignment of the SAME status variable to the +// literal `'complete'`. The negative lookbehind excludes `!==`/`<=`/`>=`/`==` +// (each of which also contains a bare `=` immediately before a quote) so +// this never re-matches the `if` line's own `!== 'complete'` clause — a +// single bounded character class, not a nested quantifier. +const ASSIGN_COMPLETE_RE = /(?=])=\s*['"]complete['"]/; + +// How many lines the consequent assignment may trail its own `if` line by. +// This bounds ONE conditional's own two halves (condition, then its direct +// consequent statement) — the same narrow role `LADDER_WINDOW_LINES` plays +// in the sibling state-field guard, not the function-scoped, unbounded +// co-occurrence Decision 4(a)'s Goodhart lesson targets for shape (b) below. +const CHECKBOX_OVERRIDE_WINDOW_LINES = 4; + +// The `if (...)` condition in both real sites nests a SECOND, unrelated +// parenthesised group (`(completion.phase_complete || planCount === 0)`), so +// a single-line, no-nested-parens regex over the whole condition +// systematically MISSES the second site. Extracted by plain paren-depth +// counting instead — a linear character walk, not a regex, so there is +// nothing for a backtracking engine to explore regardless of nesting depth. +// Bounded to IF_CONDITION_MAX_LINES so a pathologically unterminated `if (` +// cannot walk the whole file. +const IF_CONDITION_MAX_LINES = 10; + +function extractIfCondition(detectLines, startLine, startCol) { + let depth = 1; // the `(` at startCol already opened the condition + let text = ''; + const endLineLimit = Math.min(detectLines.length, startLine + IF_CONDITION_MAX_LINES); + for (let li = startLine; li < endLineLimit; li++) { + const line = detectLines[li]; + const from = li === startLine ? startCol : 0; + for (let ci = from; ci < line.length; ci++) { + const ch = line[ci]; + if (ch === '(') depth++; + else if (ch === ')') { + depth--; + if (depth === 0) return { text, endLine: li }; + } + text += ch; + } + text += '\n'; + } + return null; // unterminated within the bound — treated as no match +} + +function findChecklistOverrideDrift(text, relPath, exemptFunctions) { + const out = []; + const lines = text.split('\n'); + const { innermostAt, detect } = buildFunctionInfo(lines); + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + const ifMatch = IF_OPEN_RE.exec(detectCode); + if (!ifMatch) continue; + const startCol = ifMatch.index + ifMatch[0].length; + const condition = extractIfCondition(detect, i, startCol); + if (!condition) continue; + if (!ROADMAP_COMPLETE_IDENT_RE.test(condition.text)) continue; + const neqMatch = STATUS_NEQ_COMPLETE_RE.exec(condition.text); + if (!neqMatch) continue; + const varName = neqMatch[1]; + const limit = Math.min(lines.length, condition.endLine + 1 + CHECKBOX_OVERRIDE_WINDOW_LINES); + for (let j = condition.endLine + 1; j < limit; j++) { + const assignCode = detect[j]; + if (!assignCode.trim()) continue; + if (!assignCode.includes(varName)) continue; + if (!ASSIGN_COMPLETE_RE.test(assignCode)) continue; + const fn = innermostAt[j] || innermostAt[i]; + if (fn && exemptFunctions && exemptFunctions.has(fn)) break; + out.push({ line: j + 1, found: lines[j].trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'a', fn: fn || null }); + break; + } + } + return out; +} + +// ─── SHAPE (b): PLAN-COUNT PRECONDITION GATING A VERIFICATION READ ───────── +// +// A "count > 0"-shaped gate — any identifier containing `count` +// (case-insensitive) compared `> 0`. Function-scoped presence only (no +// window): §7.4 names this exact shape as `planCount > 0`, but the +// identifier is matched generically so a differently-named count (or a +// future `isPhaseComplete` re-implementation reusing the same gate under a +// new name) is still caught. +const COUNT_GATE_RE = /\b[A-Za-z_$][\w$]*count\b\s*>\s*0\b/i; + +// The call this derivation's owner (`readVerificationStatus`, wrapped by the +// not-yet-existing `isPhaseComplete`) must run UNCONDITIONALLY per §7.4. A +// line where `readVerificationStatus(` is reached via a ternary — a `?` +// appearing anywhere earlier on the SAME line — is a GATED call, not an +// unconditional one. `[^\n]*` is bounded by the line itself (no backtracking +// blow-up: a single non-newline character class followed by one literal). +const VERIFICATION_READ_CALL_RE = /\breadVerificationStatus\(/; +const GATED_VERIFICATION_READ_RE = /\?[^\n]*\breadVerificationStatus\(/; + +function findGatedVerificationReadDrift(text, relPath, exemptFunctions) { + const out = []; + const lines = text.split('\n'); + const { innermostAt, detect } = buildFunctionInfo(lines); + const ifCountGateBlockLines = findIfCountGateBlockLines(lines); + + const countGateFns = new Set(); + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + if (COUNT_GATE_RE.test(detectCode)) { + const fn = innermostAt[i]; + if (fn) countGateFns.add(fn); + } + } + + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + if (!VERIFICATION_READ_CALL_RE.test(detectCode)) continue; + // #3186 review finding 4(a): "gated" is EITHER a same-line ternary `?` + // before the call, OR the call sitting inside the BODY of an `if (...)` + // block whose own condition is a count-gate (`findIfCountGateBlockLines`) + // — the latter catches the block form (`if (planCount > 0) { … + // readVerificationStatus(…) … }`), which the ternary-only regex produced + // zero hits on. + const ternaryGated = GATED_VERIFICATION_READ_RE.test(detectCode); + const blockGated = ifCountGateBlockLines[i]; + if (!ternaryGated && !blockGated) continue; + const fn = innermostAt[i]; + if (!fn || !countGateFns.has(fn)) continue; + if (exemptFunctions && exemptFunctions.has(fn)) continue; + out.push({ line: i + 1, found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'b', fn }); + } + return out; +} + +// ─── SHAPE (c): LOCAL RE-IMPLEMENTATION OF "COMPLETE" FROM COUNTS ────────── +// +// `summaryCount >= planCount` (either identifier order, either comparison +// direction) — the single textual signature `buildPhaseCompletionProjection`, +// `scanPhasePlans`, `cmdRoadmapAnalyze`, `cmdRoadmapUpdatePlanProgress`, and +// `buildWorkstreamInventory` each independently hand-roll. Case-insensitive +// so `SummaryCount`/`summary_count` spellings are still caught; both operand +// orders are covered by two small, non-overlapping alternatives. +const SUMMARY_GE_PLAN_RE = /\bsummar\w*count\w*\s*>=\s*\w*plan\w*count\w*/i; +const PLAN_LE_SUMMARY_RE = /\bplan\w*count\w*\s*<=\s*\w*summar\w*count\w*/i; + +// §7.4/#3186 review finding 4(b): the literal `>=`/`<=` regexes above missed +// the algebraic restatement `summaryCount - planCount >= 0` (and its mirror, +// `planCount - summaryCount <= 0`) — same comparison, no literal `>=`/`<=` +// between the two count identifiers. Widened to cover exactly these two +// zero-compared-difference shapes; each is a single bounded alternative, no +// nested/overlapping quantifiers. +// +// KNOWN, DISCLOSED LIMIT (not claimed covered): arbitrary further algebraic +// restatements — `!(planCount > summaryCount)`, a difference stored in an +// intermediate variable before the comparison, `Math.max(0, planCount - +// summaryCount) === 0`, etc. — are NOT reliably detectable by a bounded, +// non-backtracking regex and are not attempted here. This is a genuine gap, +// not swept under "et cetera": the header above disclosed it as such rather +// than claiming a wider net than the regexes actually cast. +const SUMMARY_MINUS_PLAN_GE_ZERO_RE = /\bsummar\w*count\w*\s*-\s*\w*plan\w*count\w*\s*>=\s*0\b/i; +const PLAN_MINUS_SUMMARY_LE_ZERO_RE = /\bplan\w*count\w*\s*-\s*\w*summar\w*count\w*\s*<=\s*0\b/i; + +function findLocalCompletionCountDerivationDrift(text, relPath, exemptFunctions) { + const out = []; + const lines = text.split('\n'); + const { innermostAt, detect } = buildFunctionInfo(lines); + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + if ( + !SUMMARY_GE_PLAN_RE.test(detectCode) + && !PLAN_LE_SUMMARY_RE.test(detectCode) + && !SUMMARY_MINUS_PLAN_GE_ZERO_RE.test(detectCode) + && !PLAN_MINUS_SUMMARY_LE_ZERO_RE.test(detectCode) + ) continue; + const fn = innermostAt[i]; + if (fn && exemptFunctions && exemptFunctions.has(fn)) continue; + out.push({ line: i + 1, found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'c', fn: fn || null }); + } + return out; +} + +// ─── SHAPE (d): scanPhasePlans(...).completed READ AS A COMPLETION VERDICT ─ +// +// See the module header's shape (d) entry for the full rationale and the +// three textual forms detected below. `FUNCTION_SCOPED_EXEMPTIONS` is +// SHARED with shapes (a)/(b)/(c) — the same per-file, per-function map, so a +// function already exempted for one shape (e.g. `scanPhasePlans` itself, +// which legitimately builds the `completed` field it returns) is exempted +// for shape (d) too, and a function newly exempted for shape (d) must carry +// its own written reason in that map's comment exactly like the others. +const SCAN_CALL_TOKEN = 'scanPhasePlans('; +const DOT_COMPLETED_RE = /^\.completed\b/; +const SCAN_ASSIGN_VAR_RE = /\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*scanPhasePlans\(/; +// Bounded: `[^{}]*` is a single, non-nested, non-overlapping quantifier — no +// backtracking blow-up regardless of destructure-pattern length. Real +// destructuring assignments in this tree are single-line (the same +// assumption `SCAN_ASSIGN_VAR_RE` and every sibling shape's regexes make). +const DESTRUCTURE_ASSIGN_RE = /\{([^{}]*)\}\s*=\s*scanPhasePlans\(/; +const COMPLETED_KEY_RE = /\bcompleted\b/; + +function isWordChar(ch) { + return !!ch && /[A-Za-z0-9_$]/.test(ch); +} + +// Plain paren-depth counter over a SINGLE line (not a regex — nothing for a +// backtracking engine to explore), so a nested-paren call argument (e.g. +// `scanPhasePlans(path.join(phasesDir, dir))`, the majority real shape in +// this tree) still resolves to the call's own true closing `)`. Confined to +// one line: every real `scanPhasePlans(` call site in this tree closes on +// the line it opens on (the same assumption the rest of this file's +// detectors make about this specific call). +function findCallEndOnLine(line, afterOpenParenIdx) { + let depth = 1; + for (let i = afterOpenParenIdx; i < line.length; i++) { + const ch = line[i]; + if (ch === '(') depth++; + else if (ch === ')') { + depth--; + if (depth === 0) return i + 1; + } + } + return -1; +} + +function findScanPhasePlansCompletedReadDrift(text, relPath, exemptFunctions) { + const out = []; + const lines = text.split('\n'); + const { innermostAt, detect } = buildFunctionInfo(lines); + const isExemptAt = (i) => { + const fn = innermostAt[i]; + return !!(fn && exemptFunctions && exemptFunctions.has(fn)); + }; + + // fnKey: the innermost function name at the ASSIGNMENT site, or this + // sentinel for module-scope assignments (never collides with a real + // identifier — function names cannot start with U+0000). + const FN_KEY_MODULE = 'module'; + const varsByFn = new Map(); // fnKey -> Set bound directly (non-destructured) to a scanPhasePlans( result + + for (let i = 0; i < lines.length; i++) { + const detectCode = detect[i]; + if (!detectCode.trim()) continue; + + // Direct chained form: scanPhasePlans(...).completed — every occurrence + // on the line, paren-matched (not a naive `[^)]*`, which would stop at + // the FIRST `)`, i.e. the inner `path.join(...)`'s own closing paren). + let searchFrom = 0; + for (;;) { + const idx = detectCode.indexOf(SCAN_CALL_TOKEN, searchFrom); + if (idx === -1) break; + if (isWordChar(detectCode[idx - 1])) { + searchFrom = idx + 1; + continue; + } + const callEnd = findCallEndOnLine(detectCode, idx + SCAN_CALL_TOKEN.length); + if (callEnd === -1) { + searchFrom = idx + 1; + continue; + } + if (DOT_COMPLETED_RE.test(detectCode.slice(callEnd)) && !isExemptAt(i)) { + out.push({ line: i + 1, found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'd', fn: innermostAt[i] || null }); + } + searchFrom = callEnd; + } + + // Destructured form: const { completed[, ...] } = scanPhasePlans(...) + // (bare key or a `completed: alias` rename). + const destructureMatch = DESTRUCTURE_ASSIGN_RE.exec(detectCode); + if (destructureMatch && COMPLETED_KEY_RE.test(destructureMatch[1]) && !isExemptAt(i)) { + out.push({ line: i + 1, found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'd', fn: innermostAt[i] || null }); + } + + // Indirect form, pass 1: const NAME = scanPhasePlans(...) — record the + // binding; the read (possibly on a LATER line) is matched in pass 2 + // below, function-scoped with no line window. + const assignMatch = SCAN_ASSIGN_VAR_RE.exec(detectCode); + if (assignMatch) { + const fnKey = innermostAt[i] || FN_KEY_MODULE; + if (!varsByFn.has(fnKey)) varsByFn.set(fnKey, new Set()); + varsByFn.get(fnKey).add(assignMatch[1]); + } + } + + // Indirect form, pass 2: NAME.completed anywhere in the SAME function that + // bound NAME to a scanPhasePlans( result — Decision 4(a)'s Goodhart + // lesson again: no bounded distance between the binding and the read. + for (const [fnKey, varNames] of varsByFn) { + if (fnKey !== FN_KEY_MODULE && exemptFunctions && exemptFunctions.has(fnKey)) continue; + for (const varName of varNames) { + const escaped = varName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const readRe = new RegExp(`\\b${escaped}\\.completed\\b`); + for (let i = 0; i < lines.length; i++) { + const fnHere = innermostAt[i] || FN_KEY_MODULE; + if (fnHere !== fnKey) continue; + if (!detect[i].trim()) continue; + if (readRe.test(detect[i])) { + out.push({ + line: i + 1, + found: lines[i].trim().slice(0, MAX_REGEX_LITERAL_LEN), + shape: 'd', + fn: fnKey === FN_KEY_MODULE ? null : fnKey, + }); + } + } + } + } + + return out; +} + +function findCompletionPredicateDrift(text, relPath) { + const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null; + return [ + ...findChecklistOverrideDrift(text, relPath, exemptFunctions), + ...findGatedVerificationReadDrift(text, relPath, exemptFunctions), + ...findLocalCompletionCountDerivationDrift(text, relPath, exemptFunctions), + ...findScanPhasePlansCompletedReadDrift(text, relPath, exemptFunctions), + ].sort((x, y) => x.line - y.line); +} + +// ─── PROMPT-LAYER PROSE/SHELL DETECTION (ADR-3180 Decision 4(d)) ─────────── +// +// The SAME question — "is phase P complete?" — expressed as shell/jq inside +// workflow markdown rather than TypeScript. `gsd-core/workflows/mvp-phase.md` +// reads `.roadmap_complete` (a ROADMAP-checkbox-derived JSON field) into a +// shell variable and later OR's it directly into a `STATUS="completed"` +// decision — shape (a), independently of and in addition to +// `cmdRoadmapAnalyze`'s own checkbox override at the source of that field. +// +// Two-line pairing, mirroring the TS-side shape (a) detector: line A assigns +// a shell variable from `.roadmap_complete` (or `.roadmap_complete` accessed +// any other way jq/shell might spell it); line B, within a bounded window +// (shell scripts interleave unrelated statements more than a single `if` +// body does, so this window is wider than the TS pairing's), uses that same +// variable in a `==` test against `"true"` — the same tight, unavoidable +// two-statement pairing as its TS counterpart, not a Goodhart-vulnerable +// function-scoped sweep (a Bash script has no function-scope concept this +// guard tracks). +const PROMPT_ROADMAP_COMPLETE_ASSIGN_RE = /^([A-Za-z_][A-Za-z0-9_]*)=.*\.roadmap_complete\b/; +const PROMPT_TRUE_TEST_RE = /==\s*['"]true['"]/; +const PROMPT_OVERRIDE_WINDOW_LINES = 10; + +// #3186 review finding 7(b): `relPath` is unused here — this detector has no +// per-file exemption map the way `findCompletionPredicateDrift` does (the +// prompt layer has no FUNCTION_SCOPED_EXEMPTIONS equivalent). Kept in the +// signature (underscore-prefixed) for parity with its sibling `find*Drift` +// detectors' `(text, relPath, …)` shape and with `scanRepo`'s uniform +// `finder(text, rel)` call, rather than diverging the call convention. +function findPromptCompletionDrift(text, _relPath) { + const out = []; + const lines = text.split('\n'); + for (let i = 0; i < lines.length; i++) { + const assignMatch = PROMPT_ROADMAP_COMPLETE_ASSIGN_RE.exec(lines[i]); + if (!assignMatch) continue; + const varName = assignMatch[1]; + const limit = Math.min(lines.length, i + 1 + PROMPT_OVERRIDE_WINDOW_LINES); + for (let j = i + 1; j < limit; j++) { + const line = lines[j]; + if (!line.includes(`$${varName}`)) continue; + if (!PROMPT_TRUE_TEST_RE.test(line)) continue; + out.push({ line: j + 1, found: line.trim().slice(0, MAX_REGEX_LITERAL_LEN), shape: 'a' }); + break; + } + } + return out; +} + +// Authored TypeScript source AND the prompt layer (ADR-3180 Decision 4(d)): +// `src/` alone is itself the forbidden allowlist Decision 4(d) names. +const SCAN_DIRS = ['src', 'gsd-core/workflows', 'commands', 'agents', 'skills']; +const SCAN_EXT = new Set(['.cts', '.ts', '.mts', '.md']); + +// The designated owner (issue #3186, ADR-3180 §7.4) — does NOT exist yet. +const OWNER_FILE = path.join('src', 'verification.cts'); + +// Per ADR-3180 Decision 4(a)/(d): function-scoped, NEVER a whole-file +// allowlist. Populated by Phase 4's implementation step (issue #3186): +// +// - `isPhaseComplete` (OWNER_FILE, src/verification.cts) — the canonical +// owner itself. It reads `verification.status === 'passed'` (shape (c)'s +// textual match is a false positive here: the owner's OWN comparison is +// not a re-derivation of itself) and its readdirSync-based readability +// check for `scope: UNREADABLE` sits beside a ternary-gated read that is +// NOT a `readVerificationStatus(` call, so shapes (a)/(b) never fire here +// either — this entry exists for defence in depth and Decision 4(d)'s +// "the owner file is not exempt, only its named function is" rule. +// +// - `scanPhasePlans` (src/plan-scan.cts) — ADR-3180 §7.4 / the design's +// "0.x split": `completed: allPlanFiles.length > 0 && summaryCount >= +// planCount` answers "are all plans summarized?", NOT "is the phase +// complete?" (completion additionally requires a passing +// `*-VERIFICATION.md`). Folding it onto `isPhaseComplete` would either +// over-report completion or drag a verification read into plan-scan.cts, +// inverting the dependency direction between Phase 1's owner and this +// one (the owner consumes plan counts, never the reverse). Left as its +// own, differently-scoped answer, per design's "Rejected" list item 1. +// +// - `buildWorkstreamInventory` (src/workstream-inventory-builder.cts) — a +// PURE, I/O-free projection (see the module's own header) over +// PRE-COLLECTED `planCount`/`summaryCount`/`verificationStatus` inputs. +// `summariesMeetPlans = summaryCount >= planCount && planCount > 0` +// answers "are all plans summarized?" (combined with the caller-supplied +// verification verdict for its own `status` projection) — it cannot call +// the I/O-bound `isPhaseComplete` without breaking its "No I/O. No +// async." contract, and re-plumbing its callers to pass a pre-computed +// `complete` boolean instead of raw counts is a larger architectural +// change than this phase's declared 6 call sites. Declared deviation — +// see the Phase 4 migration PR for the full reasoning. +// +// - `buildPhaseCompletionProjection` (src/init.cts) — MIGRATED onto +// `isPhaseComplete` for `phase_complete`/`verification_status` (shape +// (b) and the unconditional-read half of shape (c) are gone), but it +// independently retains `implementation_complete = planCount > 0 && +// summaryCount >= planCount` as a DIFFERENT, still-needed answer ("are +// plans done", not "is the phase complete") that `isPhaseComplete`'s +// locked `{ complete, verification }` return shape does not carry and +// that downstream consumers (`disk_status: 'executed'` vs `'planned'`) +// still depend on. This is a declared deviation, not named by the design +// doc's own DO-NOT-MIGRATE list (which named only `scanPhasePlans` and +// `buildWorkstreamInventory`) — flagged for orchestrator review rather +// than silently exempted. +// +// SHAPE (d) reuses this exact map (`findScanPhasePlansCompletedReadDrift` +// takes the same `exemptFunctions` set every other shape's finder does). The +// four entries above were entered for shapes (a)/(b)/(c); of them, only +// `scanPhasePlans` (src/plan-scan.cts) also legitimately reads its OWN +// `completed` field for shape (d)'s purposes (building the return value it +// itself defines — not a call-then-read of another `scanPhasePlans(` +// invocation). `isPhaseComplete`, `buildWorkstreamInventory`, and +// `buildPhaseCompletionProjection` do not read `.completed` off a +// `scanPhasePlans(` call result at all (verified by direct inspection of +// each function's body as of this guard's shape-(d) addition — they consume +// `planCount`/`summaryCount`/`summaryFiles`, never `.completed`), so their +// presence in this map exempts nothing NEW for shape (d); they are listed +// here only because the map is shared. As of this addition, a whole-repo +// scan found ZERO live shape-(d) sites needing a fresh exemption entry — the +// one real instance this shape exists to catch (`cmdStateSync`, +// src/state.cts) was fixed by routing through `isPhaseComplete` rather than +// being exempted, which is the outcome ADR-3180 §7.4 requires. If a future +// change legitimately needs a NEW shape-(d) exemption, add it here with its +// own written reason — do not silently extend an existing entry's Set. +const FUNCTION_SCOPED_EXEMPTIONS = new Map([ + [OWNER_FILE, new Set(['isPhaseComplete'])], + [path.join('src', 'plan-scan.cts'), new Set(['scanPhasePlans'])], + [path.join('src', 'workstream-inventory-builder.cts'), new Set(['buildWorkstreamInventory'])], + [path.join('src', 'init.cts'), new Set(['buildPhaseCompletionProjection'])], +]); + +function scanRepo(root) { + return scanTree({ + root, + scanDirs: SCAN_DIRS, + scanExt: SCAN_EXT, + onFile(rel, text) { + const finder = path.extname(rel) === '.md' ? findPromptCompletionDrift : findCompletionPredicateDrift; + 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 completion-predicate-drift: no unsanctioned phase-completion re-derivations found\n'); + return; + } + process.stderr.write('completion-predicate-drift: independent re-derivation(s) of the phase-completion\n'); + process.stderr.write('predicate ("is phase P complete?") found. Route these call sites through\n'); + process.stderr.write('src/verification.cts `isPhaseComplete` (issue #3186, ADR-3180 §7.4) instead of\n'); + process.stderr.write('re-deriving it locally:\n'); + for (const d of violations) { + const shapeLabel = d.shape ? `[shape ${d.shape}]` : ''; + const fnLabel = d.fn ? ` (in ${sanitizeForReport(d.fn)})` : ''; + process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${shapeLabel}${fnLabel} ${sanitizeForReport(d.found)}\n`); + } + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { + findCompletionPredicateDrift, + findChecklistOverrideDrift, + findGatedVerificationReadDrift, + findLocalCompletionCountDerivationDrift, + findScanPhasePlansCompletedReadDrift, + findPromptCompletionDrift, + buildFunctionInfo, + findIfCountGateBlockLines, + scanCode, + scanRepo, + IF_OPEN_RE, + ROADMAP_COMPLETE_IDENT_RE, + STATUS_NEQ_COMPLETE_RE, + ASSIGN_COMPLETE_RE, + CHECKBOX_OVERRIDE_WINDOW_LINES, + IF_CONDITION_MAX_LINES, + extractIfCondition, + COUNT_GATE_RE, + VERIFICATION_READ_CALL_RE, + GATED_VERIFICATION_READ_RE, + SUMMARY_GE_PLAN_RE, + PLAN_LE_SUMMARY_RE, + SUMMARY_MINUS_PLAN_GE_ZERO_RE, + PLAN_MINUS_SUMMARY_LE_ZERO_RE, + SCAN_CALL_TOKEN, + DOT_COMPLETED_RE, + SCAN_ASSIGN_VAR_RE, + DESTRUCTURE_ASSIGN_RE, + COMPLETED_KEY_RE, + findCallEndOnLine, + PROMPT_ROADMAP_COMPLETE_ASSIGN_RE, + PROMPT_TRUE_TEST_RE, + PROMPT_OVERRIDE_WINDOW_LINES, + FUNCTION_DECL_RE, + ARROW_CONST_RE, + OWNER_FILE, + FUNCTION_SCOPED_EXEMPTIONS, + SCAN_DIRS, + SCAN_EXT, + MAX_REGEX_LITERAL_LEN, +}; diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index 15b204d40..514494a09 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -43,6 +43,14 @@ ], "issue": "3216" }, + "phase": { + "files": [ + "phase-completion-single-owner.test.cjs", + "phase-dependency-levels.test.cjs", + "phase.test.cjs" + ], + "issue": "3186" + }, "roadmap": { "files": [ "roadmap-mode-field.test.cjs", diff --git a/src/init.cts b/src/init.cts index 47429e0ef..d489ac568 100644 --- a/src/init.cts +++ b/src/init.cts @@ -102,7 +102,7 @@ const { const { determinePhaseStatus } = commandsMod; const { extractFrontmatter } = frontmatterMod; -const { readVerificationStatus } = verificationMod; +const { isPhaseComplete } = verificationMod; const { evaluateUatPassed } = uatPredicateMod; const { resolveLoopHooks } = loopResolverMod; const { loadRegistry } = capabilityLoaderMod; @@ -244,9 +244,9 @@ interface PhaseCompletionProjection { function projectCompletionStatus( implementationComplete: boolean, - verificationPassed: boolean, + phaseComplete: boolean, ): string { - if (implementationComplete && verificationPassed) return 'complete'; + if (phaseComplete) return 'complete'; if (implementationComplete) return 'executed'; return 'incomplete'; } @@ -259,32 +259,42 @@ function buildPhaseCompletionProjection( summaryCount: number, slashRuntime: string, ): PhaseCompletionProjection { + // ADR-3180 §7.4 (issue #3186) / DO-NOT-MIGRATE exemption + // (scripts/lint-completion-predicate-drift.cjs FUNCTION_SCOPED_EXEMPTIONS, + // declared deviation): `implementation_complete` answers "are the plans + // done" (a `scanPhasePlans`-shaped different question, per the design's + // 0.x-split), NOT "is the phase complete" — it is kept for the + // 'executed'-vs-'planned' disk_status distinction downstream consumers + // still rely on, which `isPhaseComplete`'s locked `{ complete, verification + // }` return shape does not carry. const implementationComplete = planCount > 0 && summaryCount >= planCount; const phaseFullDir = phaseDir ? path.join(cwd, phaseDir) : ''; - // #2617: ONE verification-routing seam. init used to re-derive next_command - // from the status with its own projector, which had drifted from the router's - // table — it appended the phase number and answered `human_needed`; the table - // did neither. The router now owns both the content and the runtime - // projection, and init passes the phase number it already knows (its phaseDir + // #3168 / ADR-3180 §7.4 (disk-strict, #2957): route through the canonical + // owner (`src/verification.cts` · `isPhaseComplete`), which calls + // readVerificationStatus UNCONDITIONALLY — plan count is NOT a + // precondition. A zero-plan phase with a passing `*-VERIFICATION.md` is + // complete; init used to gate the read on `implementationComplete` and + // synthesize a `not_required` sentinel instead, which is the #3168 defect. + // #2617: the router still owns both the message content and the runtime + // projection; init passes the phase number it already knows (its phaseDir // is unresolved in some branches, where the router could not derive one). - const verificationStatus = implementationComplete - ? readVerificationStatus(phaseFullDir, { runtime: slashRuntime, phaseNumber }) - : { status: 'not_required', next_action: '', next_command: '' }; + const completionResult = isPhaseComplete(phaseFullDir, { runtime: slashRuntime, phaseNumber }); + const verificationStatus = completionResult.value.verification; const projectedVerificationStatus = verificationStatus.status; const projectedVerificationAction = verificationStatus.next_action; const verificationPassed = projectedVerificationStatus === 'passed'; - const phaseComplete = implementationComplete && verificationPassed; + const phaseComplete = completionResult.value.complete; return { implementation_complete: implementationComplete, verification_status: projectedVerificationStatus, verification_passed: verificationPassed, phase_complete: phaseComplete, - completion_status: projectCompletionStatus(implementationComplete, verificationPassed), + completion_status: projectCompletionStatus(implementationComplete, phaseComplete), verification_next_action: projectedVerificationAction, verification_next_command: verificationStatus.next_command, - // #3057 B3: only readVerificationStatus's result ever carries this flag — - // the `not_required` synthetic object above never does. + // #3057 B3: readVerificationStatus's result carries this flag when its + // internal staleness check could not run to completion. verification_stale_check_indeterminate: 'staleCheckIndeterminate' in verificationStatus && verificationStatus.staleCheckIndeterminate === true, }; @@ -2282,18 +2292,20 @@ function cmdInitManager(cwd: string, raw: boolean): void { /* intentionally empty */ } + // ADR-3180 §7.4 (disk-strict, #2957, maintainer decision 2026-08-08): + // `roadmapComplete` is reported below as metadata only — it carries NO + // machine authority over `diskStatus`. The #3033 checkbox override that + // used to live here (treating a zero-plan phase as complete whenever the + // ROADMAP checkbox was ticked, layered on top of + // buildPhaseCompletionProjection's own output) is DELETED, not + // generalized: `diskStatus` now comes entirely from `completion`, which + // already routes through the canonical owner (`isPhaseComplete`) and + // itself resolves a zero-plan phase as complete whenever a passing + // `*-VERIFICATION.md` exists (#3168) — with no dependency on the + // checkbox. A zero-plan phase whose completion previously relied SOLELY + // on a ticked checkbox (no passing verification) now reports incomplete; + // this is the deliberate Tier-2 break (ADR-3180 §7.4 Decision 3). const roadmapComplete = _checkboxStates.get(phaseNum) || false; - // #3033: a zero-plan phase (split parent — intentionally plan-less, holds - // shared context for sub-phases) whose roadmap checkbox is marked complete - // must resolve as complete. The original gate required completion.phase_complete - // (derived from plan/summary counts), which is always false for zero-plan - // phases — so the checkbox override never fired and the parent was permanently - // stuck as 'researched' (an in-progress state eligible for current-phase - // selection). Now: when the roadmap marks it complete AND it has zero plans, - // treat it as complete regardless of the plan-count derivation. - if (roadmapComplete && (completion.phase_complete || planCount === 0) && diskStatus !== 'complete') { - diskStatus = 'complete'; - } phases.push({ number: phaseNum, diff --git a/src/plan-scan.cts b/src/plan-scan.cts index ae05e8153..08cedac23 100644 --- a/src/plan-scan.cts +++ b/src/plan-scan.cts @@ -219,6 +219,25 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { // (0 >= 0) rather than being pinned below 100% forever, which is the very // failure this fix removes. A genuinely empty phase (no plans authored) // still has allPlanFiles.length 0 and stays not-completed, exactly as before. + // + // ADR-3180 §7.4 (issue #3186) — DELIBERATELY NOT routed through + // `isPhaseComplete` (src/verification.cts). This field answers "are all + // plans summarized?", NOT "is the phase complete?" — completion + // additionally requires a passing `*-VERIFICATION.md`, which is the + // whole point of that owner's unconditional readVerificationStatus call. + // Folding this field onto `isPhaseComplete` would either over-report + // completion (a phase whose plans are done but never verified) or drag a + // verification read into this module, inverting the dependency + // direction between this Phase-1 owner (plan counting) and the Phase-4 + // owner (completion) — the owner must consume plan counts, never the + // reverse. Kept as its own, differently-scoped answer per the design's + // "0.x split" and exempted (function-scoped, not file-scoped) in + // scripts/lint-completion-predicate-drift.cjs's FUNCTION_SCOPED_EXEMPTIONS. + // The field name is left unchanged (not renamed to e.g. + // `summariesMeetPlanCount`) — scanPhasePlans has 11 direct callers, and a + // rename's blast radius is out of this phase's declared scope; noted + // here as a deliberate, considered-and-declined option rather than an + // oversight. completed: allPlanFiles.length > 0 && summaryCount >= planCount, hasNestedPlans, planFiles, diff --git a/src/roadmap.cts b/src/roadmap.cts index 0f1266e91..eabc37740 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -32,13 +32,13 @@ const { planningPaths, withPlanningLock, findContextMdIn } = planningWorkspace; import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import coreUtils = require('./core-utils.cjs'); -const { countMatchedSummaries } = coreUtils; +const { countMatchedSummaries, findUnsummarizedPlans } = coreUtils; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); const { extractFrontmatter, parseMustHavesBlock } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import verificationMod = require('./verification.cjs'); -const { readVerificationStatus } = verificationMod; +const { isPhaseComplete } = verificationMod; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -411,7 +411,14 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { hasContext = counts.hasContext; hasResearch = counts.hasResearch; - if (summaryCount >= planCount && planCount > 0) diskStatus = 'complete'; + // ADR-3180 §7.4 (issue #3186, disk-strict, #3168 fix): route "is this + // phase complete" through the canonical owner (`isPhaseComplete`), + // which calls readVerificationStatus UNCONDITIONALLY — plan count is + // NOT a precondition, so a zero-plan phase with a passing + // `*-VERIFICATION.md` reports complete here too, not just via + // `phase.complete`. + const completionResult = isPhaseComplete(path.join(phasesDir, dirMatch)); + if (completionResult.value.complete) diskStatus = 'complete'; else if (summaryCount > 0) diskStatus = 'partial'; else if (planCount > 0) diskStatus = 'planned'; else if (hasResearch) diskStatus = 'researched'; @@ -419,21 +426,24 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { else diskStatus = 'empty'; } - // Check ROADMAP checkbox status. - // #3537: padding-tolerant fragment — the heading discovered above may use - // a different padding than the summary-bullet checkbox below it (mixed - // padding inside one ROADMAP is legal and seen in real projects). + // Check ROADMAP checkbox status. #3537: padding-tolerant fragment — the + // heading discovered above may use a different padding than the + // summary-bullet checkbox below it (mixed padding inside one ROADMAP is + // legal and seen in real projects). + // + // ADR-3180 §7.4 (disk-strict, #2957, maintainer decision 2026-08-08): + // `roadmapComplete` is reported below as metadata ONLY — it carries NO + // machine authority over `diskStatus`. The override that used to trust a + // ticked checkbox over disk file structure is DELETED, not generalized + // (#2957: "a ticked ROADMAP checkbox is a human annotation with no + // machine authority"). A phase marked complete solely by a ticked + // checkbox — no passing `*-VERIFICATION.md`, plans outstanding — now + // reports incomplete; this is the deliberate Tier-2 break (ADR-3180 §7.4 + // Decision 3). const checkboxPattern = new RegExp(`-\\s*\\[(x| )\\]\\s*.*Phase\\s+${phaseMarkdownRegexSource(phaseNum)}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s]`, 'i'); const checkboxMatch = content.match(checkboxPattern); const roadmapComplete = checkboxMatch ? checkboxMatch[1] === 'x' : false; - // If roadmap marks phase complete, trust that over disk file structure. - // Phases completed before GSD tracking (or via external tools) may lack - // the standard PLAN/SUMMARY pairs but are still done. - if (roadmapComplete && diskStatus !== 'complete') { - diskStatus = 'complete'; - } - phases.push({ number: phaseNum, name: phaseName, @@ -570,10 +580,34 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und // completion date until the phase's verification status is 'passed', matching // cmdPhaseComplete's gate (phase.cts:1436). Previously the checkbox fired the // moment the last plan summary landed — before gsd-verifier had verified. + // + // ADR-3180 §7.4 (issue #3186, disk-strict): routed through the canonical + // owner (`isPhaseComplete`) instead of hand-rolling `summaryCount >= + // planCount && verificationPassed` locally — the owner calls + // readVerificationStatus UNCONDITIONALLY, so `isComplete` here always + // agrees with `roadmap analyze` / `init manager` / `phase complete` for + // the same phase (ADR-3180 §7.4's headline: one predicate for the read + // path and the write path). const phaseDir = path.join(cwd, phaseInfo!.directory); - const verificationResult = readVerificationStatus(phaseDir); - const verificationPassed = verificationResult.status === 'passed'; - const isComplete = summaryCount >= planCount && verificationPassed; + const completionResult = isPhaseComplete(phaseDir); + const verificationResult = completionResult.value.verification; + // #2648 precedent, applied at this write site (ADR-3180 §7.4 / #3186): + // `isPhaseComplete` deliberately carries NO plan-count precondition — the + // owner's `complete` is exactly `verification.status === 'passed'`, and + // that must stay true (disk-strict: a zero-plan phase with a passing + // `*-VERIFICATION.md` IS complete, #3168). But `readVerificationStatus`'s + // staleness check only compares SUMMARY mtimes against the verification + // file — it has no idea a NEW plan was added after the file was written, + // so a still-fresh `passed` verification says nothing about a plan added + // afterward. This command WRITES a checkbox and a completion date into + // ROADMAP.md, a materially stronger claim than "verification passed" — + // mirroring cmdPhaseComplete's own fail-closed plan-coverage gate + // (phase.cts:~1995, #2648: "a coverage gate that passes when it cannot + // read the plans is no gate at all"), composed explicitly here rather than + // folded into the predicate: complete AND all plans executed. + const coverageScan = scanPhasePlans(phaseDir); + const unsummarizedPlans = findUnsummarizedPlans(coverageScan.planFiles, coverageScan.summaryFiles); + const isComplete = completionResult.value.complete && unsummarizedPlans.length === 0; // #3057 B3: routing above is unchanged (an indeterminate staleness check // still routes as if nothing were stale) — this only makes the fact visible // to whatever reads this command's JSON output. diff --git a/src/state.cts b/src/state.cts index 559e4c886..17564eabe 100644 --- a/src/state.cts +++ b/src/state.cts @@ -31,6 +31,9 @@ const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontma // eslint-disable-next-line @typescript-eslint/no-require-imports import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports +import verificationMod = require('./verification.cjs'); +const { isPhaseComplete } = verificationMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -1777,10 +1780,18 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto for (const dir of phaseDirs) { const phaseDir = path.join(phasesDir, dir); - const { planCount, summaryCount, completed } = scanPhasePlans(phaseDir); + const { planCount, summaryCount } = scanPhasePlans(phaseDir); diskTotalPlans += planCount; diskTotalSummaries += summaryCount; - if (completed) diskCompletedPhases++; + // ADR-3180 §7.4 (#3186, #2957 disk-strict): "which phases are + // complete" is the completion question, routed through the single + // canonical owner (isPhaseComplete, src/verification.cts) — NOT + // scanPhasePlans's own `completed` field, which only answers "are + // all plans summarized" (a different question; see plan-scan.cts's + // own comment on that field). Folding this consumer onto the raw + // summaries-met flag was the exact "consolidate two of three and + // leave the third" gap §7.4's forcing function rules out. + if (isPhaseComplete(phaseDir).value.complete) diskCompletedPhases++; } // Count phase headings from ROADMAP using a digit-containing pattern // that matches both numeric phases (01, 05.1) and project-code phases @@ -3075,10 +3086,17 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b for (const dir of entries) { const dirPath = path.join(phasesDir, dir); - const { planCount: plans, summaryCount: summaries, completed } = scanPhasePlans(dirPath); + const { planCount: plans, summaryCount: summaries } = scanPhasePlans(dirPath); totalDiskPlans += plans; totalDiskSummaries += summaries; - if (completed) diskCompletedPhases++; + // ADR-3180 §7.4 (#3186, #2957 disk-strict): route through the single + // canonical owner (isPhaseComplete), not scanPhasePlans's own `completed` + // field ("are all plans summarized?" — a different question). This is the + // same fix buildStateFrontmatter got above; cmdStateSync (`state sync`) + // was a second, independent consumer of the same raw field the initial + // migration missed — without it, `state sync` and `state json` disagreed + // on completed_phases for the identical disk state. + if (isPhaseComplete(dirPath).value.complete) diskCompletedPhases++; // Track the highest phase with incomplete plans (or any plans) const phaseMatch = dir.match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`, 'i')); diff --git a/src/verification.cts b/src/verification.cts index 80db8a39a..3d7c014e4 100644 --- a/src/verification.cts +++ b/src/verification.cts @@ -37,12 +37,16 @@ import phaseId = require('./phase-id.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module import scanPhasePlans = require('./plan-scan.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module +import planningScopeMod = require('./planning-scope.cjs'); import { execGit } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; const { output, error } = io; const { extractPhaseToken } = phaseId; const { extractFrontmatter } = frontmatterMod; +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // ─── Constants ──────────────────────────────────────────────────────────────── @@ -506,6 +510,78 @@ function readVerificationStatus( }; } +interface IsPhaseCompleteDeps { + fs?: FsLike; + /** Injectable per-phase clean-commit-time resolver, threaded through to readVerificationStatus. */ + phaseCleanCommitTimesMs?: PhaseCleanCommitTimesFn; + /** Runtime whose command surface next_command is projected into (#2617). */ + runtime?: string; + /** Phase number appended to the routed command (#2617). */ + phaseNumber?: string; +} + +interface PhaseCompletionValue { + complete: boolean; + verification: VerificationStatusResult; +} + +/** + * isPhaseComplete — the single canonical owner of "is phase P complete?" + * (ADR-3180 §7.4, Decision 1). Sited beside readVerificationStatus, which it + * wraps. + * + * DISK-STRICT (#2957, maintainer decision 2026-08-08; ADR-3180 §7.4 amended + * af92fd4c9): readVerificationStatus is called UNCONDITIONALLY here — plan + * count is NOT a precondition. A phase with zero plans and a passing + * `*-VERIFICATION.md` is complete (#3168). A ROADMAP checkbox has no machine + * authority and is never consulted — this function never reads ROADMAP.md. + * + * `complete` is exactly `verification.status === 'passed'`. `verification` + * carries the FULL routing result (status/next_action/next_command), so a + * caller can distinguish a failing verdict (`gaps_found`/`human_needed`/ + * `stale`/`unknown`) from an absent one (`missing`) — both are "not + * complete", but they are not the same non-answer. + * + * `scope` is UNREADABLE when `phaseDir` itself could not be listed — this is + * INDEPENDENT of readVerificationStatus's own no-throw fail-open contract for + * a missing `*-VERIFICATION.md` file (a well-formed answer, + * `verification.status === 'missing'`, scope COMPLETE): a caller must not + * read `value.complete: false` here as a confident "not complete" the way it + * can for a genuinely-checked missing file. + * + * Does NOT import scanPhasePlans / plan-scan.cjs — the owner consumes plan + * counts from its caller when a caller needs them for a different question + * (e.g. buildPhaseCompletionProjection's own `implementation_complete`); it + * never re-derives or requires them itself. + */ +function isPhaseComplete( + phaseDir: string, + deps: IsPhaseCompleteDeps = {}, +): { value: PhaseCompletionValue; scope: Scope } { + const fsImpl: FsLike = deps.fs ?? fs; + let readable = true; + try { + fsImpl.readdirSync(phaseDir); + } catch { + readable = false; + } + + const verification = readVerificationStatus(phaseDir, { + fs: deps.fs, + phaseCleanCommitTimesMs: deps.phaseCleanCommitTimesMs, + runtime: deps.runtime, + phaseNumber: deps.phaseNumber, + }); + + return { + value: { + complete: verification.status === 'passed', + verification, + }, + scope: readable ? SCOPE.COMPLETE : SCOPE.UNREADABLE, + }; +} + /** * CLI command handler: resolve phaseDir against cwd, call readVerificationStatus, * emit via io.output(). @@ -530,5 +606,6 @@ export = { defaultPhaseCleanCommitTimesMs, findStaleVerificationSummary, readVerificationStatus, + isPhaseComplete, cmdVerificationStatus, }; diff --git a/src/workstream-inventory-builder.cts b/src/workstream-inventory-builder.cts index 3ddf78646..6cefc46c3 100644 --- a/src/workstream-inventory-builder.cts +++ b/src/workstream-inventory-builder.cts @@ -16,26 +16,18 @@ function toPosixPath(p: string): string { return p.split('\\').join('/'); } -/** - * #2562: verification verdicts that DISQUALIFY a phase from `complete`, even - * when its SUMMARY count meets its PLAN count. Deliberately scoped to the two - * EXPLICIT failing verdicts the verifier emits — `missing`/`unknown` (verifier - * off / not yet run) and `stale` (mtime-derived, #2348) are intentionally left - * untouched so verifier-disabled projects do not regress to never-complete. - * - * #2645: `'unrecorded'` is NOT a verdict the verifier itself ever emits — it - * is an internal sentinel `workstream-inventory.cts`'s verification-deletion - * ledger substitutes for `'missing'` once that workstream has ADOPTED the - * ledger (a `.verification-ledger.json` file exists for it) but has no - * remembered entry for this specific phase. Pre-adoption (no ledger file at - * all) still resolves to plain `'missing'`, which stays OUTSIDE this set — - * that is what keeps a project untouched by this fix until it actually uses - * the verifier at least once. Post-adoption, an unrecorded phase fails - * CLOSED (counted here) rather than open, so a corrupt or evidence-absent - * ledger entry can no longer be read as "safe to complete" the way a bare - * `'missing'` sentinel is. - */ -const FAILING_VERIFICATION_STATUSES = new Set(['gaps_found', 'human_needed', 'unrecorded']); +// #2562/#2645's FAILING_VERIFICATION_STATUSES set (the verdicts that used to +// disqualify a phase from `complete` when combined with a local +// summary-count-meets-plan-count check) was removed by ADR-3180 §7.4 +// (#3186): `complete` is now the single canonical owner's verdict +// (`PhaseFilesCount.complete`, computed via `isPhaseComplete` by the +// I/O-capable caller — see the loop below), which already requires +// `verification.status === 'passed'` unconditionally. Disk-strict (#2957) +// deliberately DROPS the prior "verifier-disabled projects fall back to +// summaries-met" tolerance that set existed to preserve — a phase with no +// `*-VERIFICATION.md` (`missing`) is no longer treated as complete just +// because its summaries meet its plan count. Disclosed in this phase's +// changeset. /** * #2562 / Bug #2445 / #2645 review: pick ONE winning item per key from a @@ -109,10 +101,18 @@ export interface PhaseFilesCount { inMilestone?: boolean; /** * #2562: the phase's `*-VERIFICATION.md` verdict (`readVerificationStatus`). - * A phase with SUMMARY count ≥ PLAN count but a failing verdict - * (`gaps_found`/`human_needed`) must NOT count as complete. + * Informational only as of #3186 — see `complete` below for the field this + * builder actually derives `PhaseStatus.status` from. */ verificationStatus?: string; + /** + * ADR-3180 §7.4 (#3186): the phase's completion verdict from the single + * canonical owner (`isPhaseComplete`, src/verification.cts), computed by + * the I/O-capable CALLER (this module is a pure, I/O-free projection and + * cannot call the owner itself — see the module header). Absent/undefined + * is treated as not-complete (`?? false`), never as "unknown → complete". + */ + complete?: boolean; } export interface PhaseStatus { @@ -302,12 +302,21 @@ export function buildWorkstreamInventory(inputs: BuildWorkstreamInventoryInputs) const counts = countsMap.get(dir); const planCount = counts?.planCount ?? 0; const summaryCount = counts?.summaryCount ?? 0; - // #2562: SUMMARY≥PLAN parity is necessary but not sufficient — a phase whose - // verification verdict is an explicit failing one is still in progress. - const verificationStatus = counts?.verificationStatus ?? 'missing'; - const summariesMeetPlans = summaryCount >= planCount && planCount > 0; + // ADR-3180 §7.4 (issue #3186): routed through the single canonical owner + // (`isPhaseComplete`, src/verification.cts) — via `PhaseFilesCount.complete`, + // which the I/O-capable CALLER computes (this module is a PURE, I/O-free + // projection — see the module header: "No I/O. No async." — and cannot + // call the owner itself). The prior local derivation + // (`summaryCount >= planCount && planCount > 0` combined with a + // caller-supplied verification status) was this module's OWN completion + // verdict computed from raw counts — the exact "post-process a canonical + // result locally" bypass §7.4 rules out, and it reproduced the disk-strict + // headline case (#3168): a zero-plan phase with a passing verification + // read `pending` instead of `complete`. `complete` defaults to `false` + // when absent so a caller that has not been updated to pass it never + // silently reads as complete. const status: 'complete' | 'in_progress' | 'pending' = - summariesMeetPlans && !FAILING_VERIFICATION_STATUSES.has(verificationStatus) + (counts?.complete ?? false) ? 'complete' : planCount > 0 ? 'in_progress' diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index e5e604fcc..bcdb9bd96 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -30,7 +30,7 @@ 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'); -const { readVerificationStatus } = verificationMod; +const { isPhaseComplete } = verificationMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-id.cjs is an export= CommonJS module import phaseIdMod = require('./phase-id.cjs'); const { phaseKeyFromDir, phaseKeyFromProse, parentPhaseKey } = phaseIdMod; @@ -634,7 +634,16 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream const rawPhaseEntries = [...phaseDirNames].sort().map(dir => { const phaseDir = path.join(p.phases, dir); const counts = countPhaseFiles(phaseDir); - const verificationResult = readVerificationStatus(phaseDir); + // ADR-3180 §7.4 (#3186): routed through the single canonical owner + // (`isPhaseComplete`, src/verification.cts) instead of calling + // `readVerificationStatus` directly and re-deriving "is this phase + // complete" locally from its `.status`. `completionResult.value.complete` + // is threaded down to the builder below (as `PhaseFilesCount.complete`) + // so `buildWorkstreamInventory` — a pure, I/O-free projection that + // cannot call the owner itself — consumes the owner's verdict rather + // than re-deriving a second one from summary/plan counts. + const completionResult = isPhaseComplete(phaseDir); + const verificationResult = completionResult.value.verification; // #3057 B3: routing is UNCHANGED — `liveVerificationStatus` below is still // `.status`, exactly as before, so the ledger/rollup logic that consumes // it is unaffected. This only makes an indeterminate staleness check @@ -656,6 +665,12 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream summaryCount: counts.summaryCount, inMilestone: isDirInCurrentMilestone(dir), liveVerificationStatus: verificationResult.status, + // ADR-3180 §7.4 (#3186): the owner's verdict, read live off disk — never + // ledger-adjusted (see the phaseFilesCounts map below; the ledger only + // ever substitutes a 'missing' status with a remembered one, and under + // disk-strict neither 'missing' nor 'unrecorded' is ever complete, so + // there is nothing for the ledger to override here). + complete: completionResult.value.complete, }; }); @@ -732,6 +747,7 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream summaryCount: entry.summaryCount, inMilestone: entry.inMilestone, verificationStatus, + complete: entry.complete, }; }); diff --git a/tests/clock-seam.test.cjs b/tests/clock-seam.test.cjs index 48a8f905a..6d562853d 100644 --- a/tests/clock-seam.test.cjs +++ b/tests/clock-seam.test.cjs @@ -1115,6 +1115,9 @@ describe('roadmap analyze behavioral correctness (50-phase)', () => { fs.writeFileSync(path.join(phaseDir, `${pad}-01-PLAN.md`), `# Phase ${i} Plan 1\n`); if (i <= completedCount) { fs.writeFileSync(path.join(phaseDir, `${pad}-01-SUMMARY.md`), `# Phase ${i} Summary\n`); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes a phase count as complete now. + fs.writeFileSync(path.join(phaseDir, `${pad}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n'); } } } diff --git a/tests/completion-predicate-drift-guard.test.cjs b/tests/completion-predicate-drift-guard.test.cjs new file mode 100644 index 000000000..6909f80f3 --- /dev/null +++ b/tests/completion-predicate-drift-guard.test.cjs @@ -0,0 +1,627 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +// allow-test-rule: structural-regression-guard, see #3186 +// D3 below reads src/plan-scan.cts / gsd-core/bin/lib/plan-scan.cjs and +// src/verification.cts and regex-tests them for require/import statements. +// This asserts a DEPENDENCY-DIRECTION invariant (the owner consumes plan +// counts, never the reverse — ADR-3180 §7.4 HARD CONSTRAINT) that has no +// behavioral/runtime surface: `require`-ing plan-scan.cjs in-process cannot +// distinguish "verification.cjs is absent from its dependency graph" from +// "verification.cjs happens to already be in require.cache because an +// earlier test in this same file required it directly" (line 36 above does +// exactly that) — only source inspection can tell which import edge exists. +// #3186 review finding 6(a). + +/** + * Unit + whole-repo coverage for the PHASE-COMPLETION drift guard + * (scripts/lint-completion-predicate-drift.cjs, epic #3180, issue #3186, + * ADR-3180 §7.4, Decision 4). Modelled on tests/milestone-window-drift-guard.test.cjs + * / tests/completion-ratio-single-owner.test.cjs's guard sections: behavioral + * throughout — every assertion drives the guard's exported pure functions + * directly, never `readFileSync().includes()`. + * + * Covers 50-test-matrix.md section E (the guard) plus D3 (the dependency- + * direction guard: plan-scan.cts does not import verification.cts). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const drift = require('../scripts/lint-completion-predicate-drift.cjs'); +const { sanitizeForReport } = require('../scripts/lib/drift-scan.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const ROOT = path.join(__dirname, '..'); +const OWNER_RELPATH = drift.OWNER_FILE; // path.join('src', 'verification.cts') + +// ═════════════════════════════════════════════════════════════════════════ +// E1 — the real repo tree, post-migration: 0 violations, earned per shape. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E1 — scanRepo(repoRoot) against the real repo: earned zero', () => { + test('zero violations across the whole scan surface (src/ + prompt layer)', () => { + const violations = drift.scanRepo(ROOT); + assert.deepStrictEqual( + violations, + [], + 'unsanctioned phase-completion re-derivation(s) — route through src/verification.cts ' + + '`isPhaseComplete` (issue #3186, ADR-3180 §7.4):\n' + + violations.map((d) => ` ${d.file}:${d.line} [shape ${d.shape}] ${d.found}`).join('\n'), + ); + }); + + test('per-shape proof: a deliberate (a)/(b)/(c) fixture is NOT silently swallowed by scanRepo', (t) => { + // Distinguishes "0 because nothing to find" from "0 because the detector + // is broken" — scanRepo on a synthetic tree carrying all three shapes + // must report exactly 3, proving the same code path scanRepo(ROOT) took + // is capable of finding violations at all. + const root = createTempDir('gsd-completion-predicate-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'src', 'fake.cts'), + [ + 'function fakeConsumer(roadmapComplete, planCount, summaryCount) {', + " let status = 'pending';", + ' if (roadmapComplete && status !== \'complete\') {', + " status = 'complete';", + ' }', + ' const verificationStatus = planCount > 0', + ' ? readVerificationStatus(phaseDir)', + " : { status: 'not_required' };", + ' const done = summaryCount >= planCount && planCount > 0;', + ' return { status, verificationStatus, done };', + '}', + ].join('\n'), + ); + const violations = drift.scanRepo(root); + const shapes = violations.map((v) => v.shape).sort(); + assert.deepStrictEqual(shapes, ['a', 'b', 'c']); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E2 — fixture reintroducing shapes (a), (b), (c): each flagged, one hit +// apiece. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E2 — each shape flagged in isolation, exactly one hit apiece', () => { + test('shape (a): checkbox-derived completion override', () => { + const text = [ + 'function fakeConsumer(roadmapComplete) {', + " let diskStatus = 'planned';", + ' if (roadmapComplete && diskStatus !== \'complete\') {', + " diskStatus = 'complete';", + ' }', + ' return diskStatus;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'a'); + }); + + test('shape (b): plan-count precondition gating a verification read', () => { + const text = [ + 'function fakeConsumer(planCount, phaseDir) {', + ' return planCount > 0', + ' ? readVerificationStatus(phaseDir)', + " : { status: 'not_required' };", + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'b'); + }); + + test('shape (c): local re-implementation of "complete" from counts', () => { + const text = [ + 'function fakeConsumer(summaryCount, planCount) {', + ' return summaryCount >= planCount && planCount > 0;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'c'); + }); + + test('shape (c): the reversed operand order (planCount <= summaryCount) is also flagged', () => { + const text = [ + 'function fakeConsumer(summaryCount, planCount) {', + ' return planCount <= summaryCount;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'c'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E3 — shape (a) with a nested-paren if-condition: flagged. (The first draft +// missed exactly this on cmdInitManager's `(completion.phase_complete || +// planCount === 0)` nested group.) +// ═════════════════════════════════════════════════════════════════════════ + +describe('E3 — shape (a): nested-paren if-condition', () => { + test('a second parenthesised group inside the if-condition is still detected', () => { + const text = [ + 'function fakeConsumer(roadmapComplete, completion, planCount) {', + " let diskStatus = 'planned';", + ' if (roadmapComplete && (completion.phase_complete || planCount === 0) && diskStatus !== \'complete\') {', + " diskStatus = 'complete';", + ' }', + ' return diskStatus;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'a'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E4 — an unconditional readVerificationStatus( call (the cmdPhaseComplete +// shape): NOT flagged. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E4 — an unconditional readVerificationStatus( call is not flagged', () => { + test('no ternary gate on the call, no count-gate in the function: clean', () => { + const text = [ + 'function fakeConsumer(phaseDir) {', + ' const verificationStatus = readVerificationStatus(phaseDir, { runtime: \'claude\' });', + ' return verificationStatus;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.deepStrictEqual(out, []); + }); + + test('a count-gate present elsewhere in the SAME function but the read is unconditional: still clean', () => { + // Shape (b) requires the readVerificationStatus( call ITSELF to be + // ternary-gated on the same line — a count-gate merely coexisting with + // an unconditional call must not fire. + const text = [ + 'function fakeConsumer(phaseDir, retryCount) {', + ' if (retryCount > 0) { /* retry bookkeeping, unrelated */ }', + ' const verificationStatus = readVerificationStatus(phaseDir);', + ' return verificationStatus;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.deepStrictEqual(out, []); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E5 — all three shapes inside `//` and `/* */` comments: NOT flagged. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E5 — commented-out shapes are not flagged', () => { + test('shape (a) inside a // line comment', () => { + const text = [ + "// if (roadmapComplete && diskStatus !== 'complete') diskStatus = 'complete';", + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); + + test('shape (b) inside a /* */ block comment', () => { + const text = [ + '/*', + ' const v = planCount > 0 ? readVerificationStatus(phaseDir) : { status: "not_required" };', + '*/', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); + + test('shape (c) inside a JSDoc continuation comment', () => { + const text = [ + '/**', + ' * e.g. `summaryCount >= planCount && planCount > 0` is the old shape.', + ' */', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E6 — retryCount > 0 and other unrelated count gates: NOT flagged. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E6 — unrelated count gates alone are not flagged', () => { + test('retryCount > 0 with no readVerificationStatus( call anywhere in the function', () => { + const text = [ + 'function fakeRetry(retryCount) {', + ' if (retryCount > 0) return doRetry();', + ' return doOnce();', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); + + test('itemCount > 0 gating an unrelated ternary (no verification call at all)', () => { + const text = [ + 'function fakeList(itemCount) {', + " return itemCount > 0 ? 'has items' : 'empty';", + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E7 — owner file: `isPhaseComplete` exempt BY FUNCTION NAME, never whole-file. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E7 — owner-file exemption is function-scoped, not file-scoped', () => { + test('the canonical isPhaseComplete body is exempt in the owner file', () => { + const text = [ + 'function isPhaseComplete(phaseDir, deps) {', + ' const verification = readVerificationStatus(phaseDir, deps);', + " return { value: { complete: verification.status === 'passed', verification }, scope: SCOPE.COMPLETE };", + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, OWNER_RELPATH), []); + }); + + test('a differently-named function in the SAME owner file carrying shape (c) IS flagged', () => { + const text = [ + 'function isPhaseComplete(phaseDir) { return true; }', + '', + 'function someOtherHelper(summaryCount, planCount) {', + ' return summaryCount >= planCount && planCount > 0;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, OWNER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].fn, 'someOtherHelper'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E8 — a SECOND, unrelated predicate added elsewhere IN THE OWNER FILE: +// flagged (the Amendment-4 blind spot — a whole-file exemption on the owner +// is precisely how a prior guard's owner grew an invisible second copy). +// ═════════════════════════════════════════════════════════════════════════ + +describe('E8 — a second predicate elsewhere in the owner file is flagged (Amendment-4 blind spot)', () => { + test('shape (a) added to a non-canonical function in src/verification.cts is caught', () => { + const text = [ + 'function isPhaseComplete(phaseDir) { return true; }', + '', + 'function cmdSomeNewVerb(roadmapComplete) {', + " let status = 'pending';", + ' if (roadmapComplete && status !== \'complete\') {', + " status = 'complete';", + ' }', + ' return status;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, OWNER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'a'); + assert.strictEqual(out[0].fn, 'cmdSomeNewVerb'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E9 — prompt-layer fixture reintroducing the checkbox OR: flagged (the +// scan surface really covers gsd-core/workflows, not just src/). +// ═════════════════════════════════════════════════════════════════════════ + +describe('E9 — prompt-layer checkbox-OR re-derivation is flagged', () => { + test('findPromptCompletionDrift flags the two-line assign/test pairing', () => { + const lines = [ + 'PHASE_COMPLETE=$(echo "$PHASE_INFO" | jq -r \'.roadmap_complete // false\')', + 'DISK_STATUS=$(echo "$ANALYZE" | jq -r \'.disk_status\')', + 'if [[ "$DISK_STATUS" == "complete" || "$PHASE_COMPLETE" == "true" ]]; then', + ' STATUS="completed"', + 'fi', + ].join('\n'); + const out = drift.findPromptCompletionDrift(lines, path.join('gsd-core', 'workflows', 'fake.md')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'a'); + }); + + test('scanRepo finds the same fixture when it is a real .md file under gsd-core/workflows', (t) => { + const root = createTempDir('gsd-completion-predicate-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'gsd-core', 'workflows'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'gsd-core', 'workflows', 'fake.md'), + [ + 'PHASE_COMPLETE=$(echo "$PHASE_INFO" | jq -r \'.roadmap_complete // false\')', + 'if [[ "$DISK_STATUS" == "complete" || "$PHASE_COMPLETE" == "true" ]]; then', + ' STATUS="completed"', + 'fi', + ].join('\n'), + ); + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].shape, 'a'); + }); + + test('a DISK_STATUS-only check (no checkbox OR) is not flagged', () => { + const lines = [ + 'if [[ "$DISK_STATUS" == "complete" ]]; then', + ' STATUS="completed"', + 'fi', + ].join('\n'); + assert.deepStrictEqual(drift.findPromptCompletionDrift(lines, path.join('gsd-core', 'workflows', 'fake.md')), []); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E10 — filename with control bytes / bidi: sanitized in report output. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E10 — sanitizeForReport neutralizes control bytes and bidi overrides', () => { + test('a C0 control byte in a violation "found" fragment is escaped, not passed through raw', () => { + const raw = 'diskStatus = \x1b[31m\'complete\'\x1b[0m;'; + const sanitized = sanitizeForReport(raw); + assert.ok(!sanitized.includes('\x1b'), 'raw ESC byte must not survive sanitization'); + assert.ok(sanitized.includes('\\x1b'), 'ESC byte must be rendered as a visible \\xNN escape'); + }); + + test('a bidi right-to-left override codepoint in a reported file path is escaped', () => { + const raw = 'src/‮evil.cts'; + const sanitized = sanitizeForReport(raw); + assert.ok(!sanitized.includes('‮'), 'raw RLO codepoint must not survive sanitization'); + assert.ok(sanitized.includes('\\u202e'), 'RLO codepoint must be rendered as a visible \\uNNNN escape'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E11 — #3186 review finding 4: two evasion shapes the pre-review guard +// produced ZERO hits on, now caught. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E11 — finding 4(a): the BLOCK form of shape (b) is now caught', () => { + test('if (planCount > 0) { … readVerificationStatus(…) … } is flagged (was zero hits before)', () => { + const text = [ + 'function fakeConsumer(planCount, phaseDir) {', + ' let verificationStatus = { status: \'not_required\' };', + ' if (planCount > 0) {', + ' verificationStatus = readVerificationStatus(phaseDir);', + ' }', + ' return verificationStatus;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'b'); + }); + + test('an unrelated if-block (no count-gate condition) wrapping an unconditional call stays clean', () => { + // A generic nested-brace check (not "is this specifically an if-block + // whose OWN condition is a count-gate") would false-positive here. + const text = [ + 'function fakeConsumer(phaseDir, flag) {', + ' let verificationStatus = null;', + ' if (flag) {', + ' verificationStatus = readVerificationStatus(phaseDir);', + ' }', + ' return verificationStatus;', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); + + test('a non-conditional wrapper (a callback passed to another function) is NOT treated as gating', () => { + // Regression guard for the naive "any brace nesting deeper than the + // function's own top level = gated" approach, which false-positived on + // cmdPhaseComplete's real `withPlanningLock(cwd, () => { … })` shape: + // an UNCONDITIONAL readVerificationStatus( call wrapped only in a + // callback, with an unrelated count-gate elsewhere in the function. + const text = [ + 'function fakeConsumer(phaseDir, retryCount) {', + ' if (retryCount > 0) { /* unrelated */ }', + ' return withPlanningLock(phaseDir, () => {', + ' return readVerificationStatus(phaseDir);', + ' });', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); +}); + +describe('E11 — finding 4(b): the algebraic-restatement evasion of shape (c) is now caught', () => { + test('summaryCount - planCount >= 0 is flagged (was zero hits before)', () => { + const text = [ + 'function fakeConsumer(summaryCount, planCount) {', + ' return summaryCount - planCount >= 0;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'c'); + }); + + test('the mirrored form planCount - summaryCount <= 0 is also flagged', () => { + const text = [ + 'function fakeConsumer(summaryCount, planCount) {', + ' return planCount - summaryCount <= 0;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'c'); + }); + + test('an unrelated count-difference comparison (not summary/plan) stays clean', () => { + const text = [ + 'function fakeConsumer(retryCount, maxCount) {', + ' return retryCount - maxCount >= 0;', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E12 — shape (d): a bare `.completed` read off a `scanPhasePlans(` result, +// used as a completion verdict outside the owner (src/plan-scan.cts). The +// #3186 remote-matrix finding: cmdStateSync (src/state.cts, now fixed) +// destructured `scanPhasePlans(dirPath).completed` directly with no +// comparison for shapes (a)/(b)/(c) to catch. +// ═════════════════════════════════════════════════════════════════════════ + +describe('E12 — shape (d): scanPhasePlans(...).completed read as a completion verdict', () => { + test('direct chained form: scanPhasePlans(dir).completed is flagged', () => { + const text = [ + 'function cmdSomeVerb(dirPath) {', + ' return scanPhasePlans(dirPath).completed;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + assert.strictEqual(out[0].fn, 'cmdSomeVerb'); + }); + + test('direct chained form with a nested-paren call argument is still flagged (path.join(...) inside the call)', () => { + // A naive `[^)]*` regex would stop at path.join(...)'s OWN closing paren + // and miss the `.completed` that follows the call's TRUE closing paren — + // the real shape most scanPhasePlans( call sites in this tree use. + const text = [ + 'function cmdSomeVerb(phasesDir, dir) {', + ' return scanPhasePlans(path.join(phasesDir, dir)).completed;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + }); + + test('destructured form: const { completed } = scanPhasePlans(dirPath) is flagged (the exact #3186 cmdStateSync shape)', () => { + const text = [ + 'function cmdStateSyncLike(dirPath) {', + ' const { completed } = scanPhasePlans(dirPath);', + ' return completed;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + assert.strictEqual(out[0].fn, 'cmdStateSyncLike'); + }); + + test('destructured renamed-alias form: const { completed: isDone } = scanPhasePlans(dirPath) is flagged', () => { + const text = [ + 'function cmdSomeVerb(dirPath) {', + ' const { completed: isDone } = scanPhasePlans(dirPath);', + ' return isDone;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + }); + + test('indirect form: the call and the .completed read sit on DIFFERENT lines in the SAME function — flagged, proving no line window', () => { + const text = [ + 'function cmdSomeVerb(dirPath) {', + ' const scan = scanPhasePlans(dirPath);', + ' const summaryCount = scan.summaryFiles.length;', + ' const planCount = scan.planFiles.length;', + ' // several unrelated lines of bookkeeping in between', + ' const x = summaryCount + planCount;', + ' const y = x * 2;', + ' return scan.completed;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + assert.strictEqual(out[0].line, 8); + }); + + test('a `.completed` read inside src/plan-scan.cts itself (the owner, exempt function) is NOT flagged', () => { + const text = [ + 'function scanPhasePlans(phaseDir) {', + ' const inner = scanPhasePlans(phaseDir);', + ' return { completed: inner.completed, extra: true };', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'plan-scan.cts')), []); + }); + + test('an exempted function is not flagged, but a DIFFERENT function in the SAME file still is (function-scoped, never whole-file)', () => { + // OWNER_RELPATH (src/verification.cts) has `isPhaseComplete` exempt in + // FUNCTION_SCOPED_EXEMPTIONS — reused here to prove shape (d) shares that + // same per-function map rather than a whole-file allowlist. + const text = [ + 'function isPhaseComplete(phaseDir) {', + ' const scan = scanPhasePlans(phaseDir);', + ' return scan.completed;', + '}', + '', + 'function cmdSomeNewVerb(phaseDir) {', + ' return scanPhasePlans(phaseDir).completed;', + '}', + ].join('\n'); + const out = drift.findCompletionPredicateDrift(text, OWNER_RELPATH); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].shape, 'd'); + assert.strictEqual(out[0].fn, 'cmdSomeNewVerb'); + }); + + test('an unrelated .completed property on a non-scanPhasePlans object is NOT flagged', () => { + const text = [ + 'function cmdSomeVerb(job) {', + ' const result = someOtherFunction(job);', + ' return result.completed;', + '}', + ].join('\n'); + assert.deepStrictEqual(drift.findCompletionPredicateDrift(text, path.join('src', 'unrelated.cts')), []); + }); + + test('scanRepo against the real repo tree is capable of finding a deliberate shape (d) fixture (not silently swallowed)', (t) => { + const root = createTempDir('gsd-completion-predicate-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'src', 'fake-shape-d.cts'), + [ + 'function cmdSomeVerb(dirPath) {', + ' const { completed } = scanPhasePlans(dirPath);', + ' return completed;', + '}', + ].join('\n'), + ); + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].shape, 'd'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// D3 — the 0.x-split over-consolidation guard: plan-scan.cts does NOT +// import verification.cts (the owner consumes plan counts, never the +// reverse — ADR-3180 §7.4 HARD CONSTRAINT). +// ═════════════════════════════════════════════════════════════════════════ + +describe('D3 — dependency direction: plan-scan.cts does not import verification.cts', () => { + test('the compiled plan-scan.cjs source contains no reference to verification.cjs', () => { + const compiled = fs.readFileSync(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'plan-scan.cjs'), 'utf-8'); + assert.ok(!/require\(['"]\.\/verification(\.cjs)?['"]\)/.test(compiled), 'plan-scan.cjs must not require verification.cjs'); + }); + + test('the plan-scan.cts source has no import/require of verification.cjs/.cts (a prose comment MENTIONING it, e.g. explaining why a field is not routed through it, is not an import and must not false-positive)', () => { + const source = fs.readFileSync(path.join(ROOT, 'src', 'plan-scan.cts'), 'utf-8'); + assert.ok( + !/\b(?:require|import)\s*(?:\(|\{|[A-Za-z_$][\w$]*\s*=)[^;\r\n]*verification\.c(?:j|t)s/.test(source), + 'src/plan-scan.cts must not import/require verification.cts/.cjs', + ); + }); + + test('src/verification.cts DOES import plan-scan.cjs (the direction that is allowed: owner consumes counts)', () => { + // Documents the ALLOWED direction so the pair of assertions above reads + // as a genuine one-way constraint, not an accidental total decoupling. + const source = fs.readFileSync(path.join(ROOT, 'src', 'verification.cts'), 'utf-8'); + assert.ok(/require\(['"]\.\/plan-scan\.cjs['"]\)/.test(source), 'src/verification.cts is expected to import plan-scan.cjs (for staleness-check summary listing, pre-existing/unrelated to isPhaseComplete)'); + }); +}); diff --git a/tests/emitted-drift-acks/3186-mvp-phase-disk-strict.json b/tests/emitted-drift-acks/3186-mvp-phase-disk-strict.json new file mode 100644 index 000000000..6c2ccbd88 --- /dev/null +++ b/tests/emitted-drift-acks/3186-mvp-phase-disk-strict.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "mvp-phase.md": "#3186 (epic #3180 Phase 4, ADR-3180 §7.4, disk-strict per #2957 maintainer decision): removes the `PHASE_COMPLETE` ROADMAP-checkbox override this workflow ORed into its completion check (`if [[ \"$DISK_STATUS\" == \"complete\" || \"$PHASE_COMPLETE\" == \"true\" ]]`) — a ticked checkbox is a human annotation with no machine authority, and the prompt layer was one of the (previously undiscovered) sites still trusting it. The `PHASE_COMPLETE=$(...jq -r '.roadmap_complete // false')` assignment line is deleted outright; `DISK_STATUS` alone (already routed through the canonical `isPhaseComplete` owner via `roadmap.analyze`) now decides completion. Growth is a 4-line explanatory comment documenting why the OR was removed and that this is the same predicate the read and write paths share — the deleted assignment line and the shortened `if` condition are smaller than what they replace, so the net +233 bytes is entirely the comment, not new control flow. See ADR-3180 §7.4 Decision 4(d) (the prompt layer is in scope) and the design doc `.gsd/phase/refactor-3186-phase-completion-predicate/40-design.md` row `mvp-phase.md:49`." + } +} diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index fa46f8d57..37b51b9ae 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -1062,7 +1062,9 @@ function buildRoadmap(numPhases) { /** * Create phase dirs with full plan+summary coverage for the first `count` phases. - * Each dir gets 1 PLAN + 1 SUMMARY so the disk-scan treats them as complete. + * Each dir gets 1 PLAN + 1 SUMMARY + a passing *-VERIFICATION.md so the + * disk-strict predicate (ADR-3180 §7.4, #3186) treats them as complete — a + * summary alone no longer implies completion. */ function createPhaseDirs(phasesDir, count) { for (let i = 1; i <= count; i++) { @@ -1070,6 +1072,7 @@ function createPhaseDirs(phasesDir, count) { fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(path.join(dir, `01-PLAN.md`), `# Plan\n`); fs.writeFileSync(path.join(dir, `01-SUMMARY.md`), `# Summary\n`); + fs.writeFileSync(path.join(dir, `01-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n'); } } diff --git a/tests/phase-completion-single-owner.test.cjs b/tests/phase-completion-single-owner.test.cjs new file mode 100644 index 000000000..149de593a --- /dev/null +++ b/tests/phase-completion-single-owner.test.cjs @@ -0,0 +1,742 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +// allow-test-rule: source-text-is-the-product, see #3186 +// F2 below reads gsd-core/workflows/mvp-phase.md and regex-tests its shell +// content for the disk-strict OR removal (Decision 4(d)). A workflow .md +// file's text IS the deployed prompt-layer artifact the runtime executes — +// there is no runtime API that "runs" mvp-phase.md to observe its shell +// logic behaviorally, so asserting on its source text tests the actual +// deployed contract. #3186 review finding 6(b). + +/** + * Phase-completion single-owner tests (epic #3180, issue #3186, ADR-3180 + * §7.4, disk-strict per #2957). Covers `.gsd/phase/refactor-3186-phase- + * completion-predicate/50-test-matrix.md` sections A-F: + * + * A — the predicate itself (`src/verification.cts` · `isPhaseComplete`) + * B — disk-strict: the ROADMAP checkbox has no machine authority + * C — identity at each CONSUMER's observable output (Decision 4c) + * D — the 0.x-split: sites answering a DIFFERENT question keep answering it + * F — Tier-2 regression surface + * + * Section E (the drift guard itself) lives in + * tests/completion-predicate-drift-guard.test.cjs. + * + * A1 is the #3168 regression (zero plans + passing `*-VERIFICATION.md` must + * read complete). Verified RED-before/GREEN-after manually against this + * change (git stash the src/ edits, rebuild, and probe `init manager` on + * the exact A1 fixture below): pre-fix it reported + * `{ phase_complete: false, verification_status: 'not_required', + * disk_status: 'empty' }`; post-fix it reports + * `{ phase_complete: true, verification_status: 'passed', disk_status: + * 'complete' }`. That evidence is reported in the implementation PR/summary + * rather than re-run here (a stash/rebuild inside a test body would not be + * hermetic); the assertions below pin the GREEN (post-fix) behavior as a + * permanent regression net. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { isPhaseComplete } = require('../gsd-core/bin/lib/verification.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); +const { scanPhasePlans } = require('../gsd-core/bin/lib/plan-scan.cjs'); +const { buildWorkstreamInventory } = require('../gsd-core/bin/lib/workstream-inventory-builder.cjs'); +const { runGsdTools, createTempDir, createTempProject, cleanup } = require('./helpers.cjs'); + +// ─── Fixture helpers ──────────────────────────────────────────────────────── + +function writeRoadmap(tmpDir, phases) { + const sections = phases.map((p) => { + let section = `### Phase ${p.number}: ${p.name}\n\n**Goal:** ${p.goal || 'Do the thing'}\n`; + if (p.depends_on) section += `**Depends on:** ${p.depends_on}\n`; + return section; + }).join('\n'); + const checklist = phases.map((p) => { + const mark = p.complete ? 'x' : ' '; + return `- [${mark}] **Phase ${p.number}: ${p.name}**`; + }).join('\n'); + // A real Progress TABLE (gsd-core/templates/roadmap.md shape), not just the + // checklist — cmdRoadmapUpdatePlanProgress writes into this table via the + // markdown-table seam (editProgressTableSlice/updateTableCell), which no-ops + // when no table with these columns exists. G2 (#3186) needs a real table to + // observe the "Plans Complete / Status cells still update, only the + // completion checkbox is withheld" behavior. + const table = [ + '| Phase | Plans Complete | Status | Completed |', + '|-------|-----------------|--------|-----------|', + ...phases.map((p) => `| ${p.number}. ${p.name} | 0/0 | Not started | - |`), + ].join('\n'); + const content = `# Roadmap\n\n## Progress\n\n${checklist}\n\n${table}\n\n${sections}`; + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), content); +} + +function writeState(tmpDir) { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '---\nstatus: active\n---\n# State\n'); +} + +function scaffoldPhase(tmpDir, num, opts = {}) { + const padded = String(num).padStart(2, '0'); + const slug = opts.slug || 'test-phase'; + const dir = path.join(tmpDir, '.planning', 'phases', `${padded}-${slug}`); + fs.mkdirSync(dir, { recursive: true }); + if (opts.plans) { + for (let i = 1; i <= opts.plans; i++) { + fs.writeFileSync(path.join(dir, `${padded}-${String(i).padStart(2, '0')}-PLAN.md`), `# Plan ${i}`); + } + } + if (opts.summaries) { + for (let i = 1; i <= opts.summaries; i++) { + fs.writeFileSync(path.join(dir, `${padded}-${String(i).padStart(2, '0')}-SUMMARY.md`), `# Summary ${i}`); + } + } + return dir; +} + +function writeVerification(phaseDir, padded, status, filenameOverride) { + const filename = filenameOverride || `${padded}-VERIFICATION.md`; + fs.writeFileSync(path.join(phaseDir, filename), `---\nstatus: ${status}\n---\n# Verification\n`); +} + +// ═════════════════════════════════════════════════════════════════════════ +// A — isPhaseComplete: the predicate itself +// ═════════════════════════════════════════════════════════════════════════ + +describe('A — isPhaseComplete: disk-strict, unconditional readVerificationStatus', () => { + test('A1 (#3168 regression): zero plans + passing *-VERIFICATION.md -> complete', (t) => { + const dir = createTempDir('gsd-phase-complete-a1-'); + t.after(() => cleanup(dir)); + writeVerification(dir, '01', 'passed'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, true); + assert.strictEqual(result.value.verification.status, 'passed'); + assert.strictEqual(result.scope, SCOPE.COMPLETE); + }); + + test('A2: plans present, all summarized, passing verification -> complete', (t) => { + const dir = createTempDir('gsd-phase-complete-a2-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(dir, '01-01-SUMMARY.md'), '# Summary'); + writeVerification(dir, '01', 'passed'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, true); + }); + + test('A3: plans present, all summarized, NO verification -> not complete', (t) => { + const dir = createTempDir('gsd-phase-complete-a3-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(dir, '01-01-SUMMARY.md'), '# Summary'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, false); + assert.strictEqual(result.value.verification.status, 'missing'); + }); + + test('A4: verification present but FAILING -> not complete, distinguishable from absent', (t) => { + const dir = createTempDir('gsd-phase-complete-a4-'); + t.after(() => cleanup(dir)); + writeVerification(dir, '01', 'gaps_found'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, false); + assert.strictEqual(result.value.verification.status, 'gaps_found'); + assert.notStrictEqual(result.value.verification.status, 'missing'); + }); + + test('A5: zero plans, NO verification -> not complete', (t) => { + const dir = createTempDir('gsd-phase-complete-a5-'); + t.after(() => cleanup(dir)); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, false); + assert.strictEqual(result.value.verification.status, 'missing'); + }); + + test('A6: plan count boundary 0/1/2 with passing verification -> complete at every count', (t) => { + for (const planCount of [0, 1, 2]) { + const dir = createTempDir(`gsd-phase-complete-a6-${planCount}-`); + t.after(() => cleanup(dir)); + for (let i = 1; i <= planCount; i++) { + fs.writeFileSync(path.join(dir, `01-0${i}-PLAN.md`), `# Plan ${i}`); + fs.writeFileSync(path.join(dir, `01-0${i}-SUMMARY.md`), `# Summary ${i}`); + } + writeVerification(dir, '01', 'passed'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, true, `planCount=${planCount} must be complete`); + } + }); + + test('A7: phase dir unreadable -> non-COMPLETE scope, not a false "incomplete"', (t) => { + const dir = createTempDir('gsd-phase-complete-a7-'); + t.after(() => cleanup(dir)); + // Injected via a fake `deps.fs` (method monkeypatching through the + // function's own dependency-injection seam) rather than chmod 0o000 — + // chmod is bypassed by root/Docker CI and does not exercise the code + // path deterministically. + const fakeFs = { + readdirSync: () => { + throw new Error('EACCES: permission denied, scandir'); + }, + readFileSync: fs.readFileSync, + statSync: fs.statSync, + }; + + const result = isPhaseComplete(dir, { fs: fakeFs }); + assert.notStrictEqual(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.scope, SCOPE.UNREADABLE); + }); + + test('A8: multiple *-VERIFICATION.md files, one passing one failing -> defined, documented verdict', (t) => { + const dir = createTempDir('gsd-phase-complete-a8-'); + t.after(() => cleanup(dir)); + // readVerificationStatus (which isPhaseComplete wraps) takes the + // lexicographically-FIRST matching filename (`.sort()[0]`) — pin that + // contract here rather than leaving "multiple files" undefined. + writeVerification(dir, '01', 'gaps_found', '01-A-VERIFICATION.md'); + writeVerification(dir, '01', 'passed', '01-B-VERIFICATION.md'); + + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.verification.status, 'gaps_found', '01-A- sorts before 01-B-'); + assert.strictEqual(result.value.complete, false); + }); + + test('A9: CRLF in the verification file -> identical to LF', (t) => { + const dirLf = createTempDir('gsd-phase-complete-a9-lf-'); + const dirCrlf = createTempDir('gsd-phase-complete-a9-crlf-'); + t.after(() => { + cleanup(dirLf); + cleanup(dirCrlf); + }); + fs.writeFileSync(path.join(dirLf, '01-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); + fs.writeFileSync(path.join(dirCrlf, '01-VERIFICATION.md'), '---\r\nstatus: passed\r\n---\r\n# Verification\r\n'); + + const lfResult = isPhaseComplete(dirLf); + const crlfResult = isPhaseComplete(dirCrlf); + assert.strictEqual(lfResult.value.complete, true); + assert.strictEqual(crlfResult.value.complete, true); + assert.strictEqual(crlfResult.value.verification.status, lfResult.value.verification.status); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// B — Disk-strict: the checkbox has no machine authority +// ═════════════════════════════════════════════════════════════════════════ + +describe('B — disk-strict: ROADMAP checkbox carries no machine authority', () => { + function fixture(tmpDir, { checked, plans, summaries, verificationStatus }) { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo', complete: checked }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo', plans, summaries }); + if (verificationStatus) writeVerification(dir, '01', verificationStatus); + return dir; + } + + test('B1: checkbox ticked, plans outstanding, no verification -> NOT complete (the Tier-2 break)', () => { + const tmpDir = createTempProject(); + try { + fixture(tmpDir, { checked: true, plans: 2, summaries: 0 }); + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + // 2 plans, 0 summaries -> 'planned' (disk_status's own taxonomy: no + // summaries yet means "planned", not "partial" — 'partial' requires + // summaryCount > 0). The load-bearing assertion for the Tier-2 break is + // the notStrictEqual below: the checkbox alone must not read 'complete'. + assert.strictEqual(analyzed.phases[0].disk_status, 'planned'); + assert.notStrictEqual(analyzed.phases[0].disk_status, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + test('B2: checkbox ticked AND verification passing -> complete (checkbox contributed nothing)', () => { + const tmpDir = createTempProject(); + try { + fixture(tmpDir, { checked: true, plans: 1, summaries: 1, verificationStatus: 'passed' }); + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(analyzed.phases[0].disk_status, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + test('B3: checkbox UNTICKED, verification passing -> complete (disk wins in both directions)', () => { + const tmpDir = createTempProject(); + try { + fixture(tmpDir, { checked: false, plans: 1, summaries: 1, verificationStatus: 'passed' }); + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(analyzed.phases[0].disk_status, 'complete'); + assert.strictEqual(analyzed.phases[0].roadmap_complete, false, 'checkbox itself stays unticked/reported'); + } finally { + cleanup(tmpDir); + } + }); + + test('B4: checkbox ticked, verification FAILING -> not complete', () => { + const tmpDir = createTempProject(); + try { + fixture(tmpDir, { checked: true, plans: 1, summaries: 1, verificationStatus: 'gaps_found' }); + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + assert.notStrictEqual(analyzed.phases[0].disk_status, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + test('B5: ROADMAP.md absent entirely -> the predicate itself is unaffected (isPhaseComplete never reads it)', (t) => { + const dir = createTempDir('gsd-phase-complete-b5-'); + t.after(() => cleanup(dir)); + writeVerification(dir, '01', 'passed'); + // No ROADMAP.md anywhere near `dir` — isPhaseComplete takes a phase + // directory, not a project root, and never touches ROADMAP.md. + const result = isPhaseComplete(dir); + assert.strictEqual(result.value.complete, true); + }); + + test('B6: a ticked checkbox is NOT deleted from ROADMAP.md — only its authority is removed', () => { + const tmpDir = createTempProject(); + try { + fixture(tmpDir, { checked: true, plans: 2, summaries: 0 }); + const before = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(before.includes('[x] **Phase 1'), 'fixture sanity: checkbox starts ticked'); + + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.strictEqual(after, before, 'roadmap analyze is read-only: the human annotation survives untouched'); + assert.ok(after.includes('[x] **Phase 1'), 'the ticked checkbox itself is still present'); + } finally { + cleanup(tmpDir); + } + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// C — Identity at each CONSUMER's observable output (Decision 4c) +// ═════════════════════════════════════════════════════════════════════════ + +describe('C — consumer identity: every reader of "is phase P complete?" agrees', () => { + test('C1: init manager reports complete for the A1 fixture (the #3168 symptom)', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo' }); // zero plans + writeVerification(dir, '01', 'passed'); + + const result = runGsdTools('init manager --raw', tmpDir); + assert.ok(result.success, result.error); + const output = JSON.parse(result.output); + const phase1 = output.phases.find((p) => p.number === '1' || p.number === '01'); + assert.strictEqual(phase1.phase_complete, true); + assert.strictEqual(phase1.verification_status, 'passed'); + assert.notStrictEqual(phase1.verification_status, 'not_required'); + } finally { + cleanup(tmpDir); + } + }); + + test('C2: roadmap analyze disk_status matches the owner, with no checkbox arm', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); // checkbox UNTICKED + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo' }); + writeVerification(dir, '01', 'passed'); + + const owner = isPhaseComplete(dir); + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(analyzed.phases[0].disk_status === 'complete', owner.value.complete); + } finally { + cleanup(tmpDir); + } + }); + + test('C3: phase complete is unchanged — still succeeds exactly when the owner says complete', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo' }); // zero plans, A1 shape + writeVerification(dir, '01', 'passed'); + + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.complete, true); + + const result = runGsdTools('phase complete 1 --raw', tmpDir); + assert.ok(result.success, `phase complete must succeed when the owner reports complete: ${result.error}`); + } finally { + cleanup(tmpDir); + } + }); + + test('C4: roadmap update-plan-progress "complete" field matches the owner', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 1, summaries: 1 }); + writeVerification(dir, '01', 'gaps_found'); // failing -> owner says not complete + + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.complete, false); + + // No --raw: cmdRoadmapUpdatePlanProgress's output() call carries a + // non-undefined rawValue (a "N/N Status" text fallback), so --raw + // would switch this to plain text instead of JSON (unlike roadmap + // analyze / init manager, whose output() calls pass rawValue: + // undefined and always emit JSON regardless of --raw). + const result = runGsdTools('roadmap update-plan-progress 1', tmpDir); + assert.ok(result.success, result.error); + const output = JSON.parse(result.output); + assert.strictEqual(output.complete, owner.value.complete); + } finally { + cleanup(tmpDir); + } + }); + + test('C5: cross-consumer — one fixture, init manager AND roadmap analyze report the SAME verdict', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo' }); // zero plans + writeVerification(dir, '01', 'passed'); + + const initResult = runGsdTools('init manager --raw', tmpDir); + const roadmapResult = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(initResult.success, initResult.error); + assert.ok(roadmapResult.success, roadmapResult.error); + + const initPhase = JSON.parse(initResult.output).phases.find((p) => p.number === '1' || p.number === '01'); + const roadmapPhase = JSON.parse(roadmapResult.output).phases[0]; + assert.strictEqual(initPhase.phase_complete, true); + assert.strictEqual(roadmapPhase.disk_status, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + describe('C6: the §7.4 headline — "phase complete succeeds while init manager reports incomplete" is unrepresentable', () => { + test('agreement case: zero plans + passing verification -> BOTH succeed/report complete', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo' }); + writeVerification(dir, '01', 'passed'); + + const initResult = runGsdTools('init manager --raw', tmpDir); + assert.ok(initResult.success, initResult.error); + const initPhase = JSON.parse(initResult.output).phases.find((p) => p.number === '1' || p.number === '01'); + assert.strictEqual(initPhase.phase_complete, true); + + const completeResult = runGsdTools('phase complete 1 --raw', tmpDir); + assert.ok(completeResult.success, `phase complete must succeed to agree with init manager: ${completeResult.error}`); + } finally { + cleanup(tmpDir); + } + }); + + test('agreement case: plans outstanding, no verification -> BOTH report/refuse incomplete', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 1, summaries: 1 }); // no *-VERIFICATION.md written + + const initResult = runGsdTools('init manager --raw', tmpDir); + assert.ok(initResult.success, initResult.error); + const initPhase = JSON.parse(initResult.output).phases.find((p) => p.number === '1' || p.number === '01'); + assert.strictEqual(initPhase.phase_complete, false); + + const completeResult = runGsdTools('phase complete 1 --raw', tmpDir); + assert.strictEqual(completeResult.success, false, 'phase complete must be BLOCKED to agree with init manager reporting incomplete'); + } finally { + cleanup(tmpDir); + } + }); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// D — the 0.x-split: sites asking a DIFFERENT question keep answering it +// ═════════════════════════════════════════════════════════════════════════ + +describe('D — the 0.x split: "are plans summarized" stays a different, legitimate answer', () => { + function buildD1Fixture(t) { + // All plans summarized, but NO *-VERIFICATION.md — the exact fixture + // the design's "0.x split" section names as the trap. + const dir = createTempDir('gsd-phase-completion-d1-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(dir, '01-01-SUMMARY.md'), '# Summary'); + return dir; + } + + test('D1: scanPhasePlans.completed still reports true — "are plans summarized" is a different question', (t) => { + const dir = buildD1Fixture(t); + const scan = scanPhasePlans(dir); + assert.strictEqual(scan.completed, true, 'plan-scan.cts answers "are all plans summarized", not "is the phase complete"'); + }); + + test('D2: the SAME fixture through isPhaseComplete -> NOT complete (the two answers legitimately differ)', (t) => { + const dir = buildD1Fixture(t); + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.complete, false); + + const scan = scanPhasePlans(dir); + assert.notStrictEqual(scan.completed, owner.value.complete, 'the two derivations must legitimately disagree on this exact fixture'); + }); + + // ADR-3180 §7.4 (#3186 review finding 3, corrected from this phase's own + // design doc): `buildWorkstreamInventory` was ORIGINALLY (wrongly) + // classified as staying on the "different question" side of the 0.x + // split, the same way `scanPhasePlans.completed` legitimately does. Review + // found it reproduced the §7.4 headline case (#3168) in a third surface — + // it combined a local summaries-met derivation with caller-supplied + // verification data to decide the SAME "is phase P complete?" question, + // not a different one. Corrected: the module is a pure, I/O-free + // projection (module header: "No I/O. No async.") that cannot call + // `isPhaseComplete` itself, so per Decision 4(c) the CALLER computes the + // owner's real verdict and passes it in via `PhaseFilesCount.complete` — + // `workstream-inventory.cts` does this with a real `isPhaseComplete` call + // in production. D4 now pins that routing directly. + test('D4: buildWorkstreamInventory on the D1 fixture is NOT complete when the caller supplies the owner\'s real (false) verdict', (t) => { + const dir = buildD1Fixture(t); + const scan = scanPhasePlans(dir); + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.complete, false, 'fixture sanity: no *-VERIFICATION.md -> the owner says not complete'); + + const inventory = buildWorkstreamInventory({ + name: 'default', + projectDir: path.dirname(dir), + workstreamDir: path.dirname(dir), + phaseDirNames: ['01-fixture'], + activeWorkstreamName: 'default', + phaseFilesCounts: [ + { + directory: '01-fixture', + planCount: scan.planCount, + summaryCount: scan.summaryCount, + // ADR-3180 §7.4 (#3186): the caller-computed owner verdict, NOT a + // local re-derivation from planCount/summaryCount. + complete: owner.value.complete, + }, + ], + roadmapPhaseCount: 1, + stateProjection: { status: 'in_progress', current_phase: '01', last_activity: null }, + filesExist: { roadmap: true, state: true, requirements: false }, + }); + + assert.strictEqual( + inventory.phases[0].status, + 'in_progress', + 'summaries-met alone no longer resolves complete — the builder routes through the caller-supplied owner verdict (#3186 fix)', + ); + }); + + test('D5: buildWorkstreamInventory reports complete for a zero-plan phase when the caller supplies a true owner verdict (#3168 parity)', (t) => { + const dir = createTempDir('gsd-phase-completion-d5-'); + t.after(() => cleanup(dir)); + writeVerification(dir, '01', 'passed'); + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.complete, true); + + const inventory = buildWorkstreamInventory({ + name: 'default', + projectDir: path.dirname(dir), + workstreamDir: path.dirname(dir), + phaseDirNames: ['01-fixture'], + activeWorkstreamName: 'default', + phaseFilesCounts: [ + { directory: '01-fixture', planCount: 0, summaryCount: 0, complete: owner.value.complete }, + ], + roadmapPhaseCount: 1, + stateProjection: { status: 'in_progress', current_phase: '01', last_activity: null }, + filesExist: { roadmap: true, state: true, requirements: false }, + }); + + assert.strictEqual( + inventory.phases[0].status, + 'complete', + 'zero plans + a passing verification -> complete, matching isPhaseComplete (#3168) even with planCount 0', + ); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// F — Tier-2 regression surface +// ═════════════════════════════════════════════════════════════════════════ + +describe('F — Tier-2 regression surface', () => { + test('F1: a project relying on checkbox-only completion — roadmap analyze stops reporting complete (documented break)', () => { + // Same fixture shape as B1, framed as the Tier-2 regression this phase + // ships deliberately: a downstream project that was relying on a ticked + // checkbox alone (no passing verification, plans outstanding) now sees + // `disk_status` flip away from 'complete'. + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo', complete: true }]); + scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 3, summaries: 0 }); + + const result = runGsdTools('roadmap analyze --raw', tmpDir); + assert.ok(result.success, result.error); + const analyzed = JSON.parse(result.output); + assert.notStrictEqual(analyzed.phases[0].disk_status, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + test('F2: gsd-core/workflows/mvp-phase.md no longer ORs PHASE_COMPLETE with disk status', () => { + const content = fs.readFileSync( + path.join(__dirname, '..', 'gsd-core', 'workflows', 'mvp-phase.md'), + 'utf-8', + ); + assert.ok( + !/"\$DISK_STATUS"\s*==\s*"complete"\s*\|\|\s*"\$PHASE_COMPLETE"/.test(content), + 'the disk-strict OR must be gone from mvp-phase.md', + ); + assert.ok( + /if \[\[ "\$DISK_STATUS" == "complete" \]\]; then/.test(content), + 'DISK_STATUS alone must decide completion', + ); + }); + + test('F3: init manager never emits the old not_required sentinel for a zero-plan phase (regression net replacing it)', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + scaffoldPhase(tmpDir, 1, { slug: 'foo' }); // zero plans, no verification either + + const result = runGsdTools('init manager --raw', tmpDir); + assert.ok(result.success, result.error); + const output = JSON.parse(result.output); + const phase1 = output.phases.find((p) => p.number === '1' || p.number === '01'); + assert.notStrictEqual(phase1.verification_status, 'not_required', 'not_required is retired — the owner always reports a real readVerificationStatus verdict'); + assert.strictEqual(phase1.verification_status, 'missing'); + assert.strictEqual(phase1.phase_complete, false); + } finally { + cleanup(tmpDir); + } + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// G — write-path plan-coverage gate (#3186 review finding 1, #2648 +// precedent). `isPhaseComplete` deliberately has NO plan-count precondition +// (the owner), but `roadmap update-plan-progress` WRITES a checkbox + +// completion date into ROADMAP.md — a stronger claim than "verification +// passed" — and must additionally refuse when a plan has no completion +// record, exactly like `phase complete`'s own #2648 gate. Matrix rows A-F +// never covered "plans added AFTER a still-fresh passing verification"; +// this is that row. +// ═════════════════════════════════════════════════════════════════════════ + +describe('G — write-path plan-coverage gate: a plan added after a still-fresh passing verification', () => { + test('G1: the OWNER (isPhaseComplete) reports complete — no plan-count precondition, by design', () => { + const dir = createTempDir('gsd-phase-completion-g1-'); + try { + fs.writeFileSync(path.join(dir, '01-01-PLAN.md'), '# Plan 1'); + fs.writeFileSync(path.join(dir, '01-01-SUMMARY.md'), '# Summary 1'); + writeVerification(dir, '01', 'passed'); + // A second plan lands AFTER verification passed — never summarized. + // The verification file is untouched, so its status stays 'passed' + // (readVerificationStatus's staleness check compares only SUMMARY + // mtimes against the verification file, never plan count). + fs.writeFileSync(path.join(dir, '01-02-PLAN.md'), '# Plan 2'); + + const owner = isPhaseComplete(dir); + assert.strictEqual(owner.value.verification.status, 'passed', 'fixture sanity: verification reads as still-fresh passed'); + assert.strictEqual(owner.value.complete, true, 'the owner has no plan-count precondition by design (ADR-3180 §7.4 hard constraint) — this is not the bug'); + } finally { + cleanup(dir); + } + }); + + test('G2: roadmap update-plan-progress REFUSES to write completion for the identical fixture', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 1, summaries: 1 }); + writeVerification(dir, '01', 'passed'); + fs.writeFileSync(path.join(dir, '01-02-PLAN.md'), '# Plan 2'); // outstanding, no summary + + const roadmapBefore = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + + const result = runGsdTools('roadmap update-plan-progress 1', tmpDir); + assert.ok(result.success, result.error); + const output = JSON.parse(result.output); + assert.strictEqual(output.complete, false, 'the write site must refuse — an outstanding plan has no completion record (#2648 precedent)'); + + const roadmapAfter = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(!roadmapAfter.includes('[x] **Phase 1'), 'the checkbox must NOT be checked'); + assert.ok(!/\(completed \d{4}-\d{2}-\d{2}\)/.test(roadmapAfter), 'no completion date must be stamped'); + assert.notStrictEqual(roadmapAfter, roadmapBefore, 'the command still updates the Plans Complete / Status cells — only the completion checkbox is withheld'); + } finally { + cleanup(tmpDir); + } + }); + + test('G3: phase complete agrees — refuses for the identical fixture, same reason class', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 1, summaries: 1 }); + writeVerification(dir, '01', 'passed'); + fs.writeFileSync(path.join(dir, '01-02-PLAN.md'), '# Plan 2'); + + const updateResult = runGsdTools('roadmap update-plan-progress 1', tmpDir); + assert.ok(updateResult.success, updateResult.error); + const updateOutput = JSON.parse(updateResult.output); + + const completeResult = runGsdTools('phase complete 1 --raw', tmpDir); + assert.strictEqual(completeResult.success, false, 'phase complete must refuse (its own #2648 gate)'); + assert.strictEqual(updateOutput.complete, false, 'both write paths agree: incomplete'); + } finally { + cleanup(tmpDir); + } + }); + + test('G4: once the outstanding plan gets its summary, both write paths agree completion proceeds', () => { + const tmpDir = createTempProject(); + try { + writeState(tmpDir); + writeRoadmap(tmpDir, [{ number: '1', name: 'Foo' }]); + const dir = scaffoldPhase(tmpDir, 1, { slug: 'foo', plans: 2, summaries: 2 }); + writeVerification(dir, '01', 'passed'); + + const result = runGsdTools('roadmap update-plan-progress 1', tmpDir); + assert.ok(result.success, result.error); + const output = JSON.parse(result.output); + assert.strictEqual(output.complete, true, 'no outstanding plan — the gate does not withhold completion'); + + const roadmapAfter = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(/\(completed \d{4}-\d{2}-\d{2}\)/.test(roadmapAfter), 'completion date IS stamped once plan coverage is satisfied'); + } finally { + cleanup(tmpDir); + } + }); +}); diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index a0b69f482..b1399d3bd 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -5905,6 +5905,15 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c fs.writeFileSync(path.join(phase4Dir, `04-${padded}-PLAN.md`), `plan ${i}`, 'utf8'); fs.writeFileSync(path.join(phase4Dir, `04-${padded}-SUMMARY.md`), `summary ${i}`, 'utf8'); } + // Disk-strict completion (ADR-3180 §7.4, #3186): Phase 4 is already + // shipped per the ROADMAP checklist/table above — a passing + // *-VERIFICATION.md is what actually makes it count as complete now + // (runSdkQuery's writePassedVerificationForPhase only covers the phase + // under test, phase 5, not this already-complete phase 4 fixture). + fs.writeFileSync( + path.join(phase4Dir, '04-VERIFICATION.md'), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); const phase6Dir = path.join(phasesDir, '06-integration'); fs.mkdirSync(phase6Dir, { recursive: true }); diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 9b518f39b..b49ff78ff 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -261,6 +261,9 @@ describe('roadmap analyze command', () => { fs.mkdirSync(p1, { recursive: true }); fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan'); fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes phase 1 count as complete now. + fs.writeFileSync(path.join(p1, '01-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); const p2 = path.join(tmpDir, '.planning', 'phases', '02-authentication'); fs.mkdirSync(p2, { recursive: true }); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 88b9d0d69..55c846da5 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2054,6 +2054,11 @@ describe('milestone-scoped phase counting in frontmatter', () => { // Add a plan to each fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan'); fs.writeFileSync(path.join(phaseDir, `${padded}-01-SUMMARY.md`), '# Summary'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a plan+summary count no + // longer implies "complete" — only a passing *-VERIFICATION.md does. Both + // milestone phases (5 and 6) get one so this test still exercises milestone + // SCOPING, not the (now-separate) completion predicate. + if (i === 5 || i === 6) writePassedVerification(tmpDir, `${padded}-phase-${i}`, padded); } // Write a STATE.md and trigger a write that will sync frontmatter @@ -2071,7 +2076,7 @@ describe('milestone-scoped phase counting in frontmatter', () => { const output = JSON.parse(jsonResult.output); assert.strictEqual(Number(output.progress.total_phases), 2, 'should count only milestone phases (5 and 6), not all 6'); - assert.strictEqual(Number(output.progress.completed_phases), 2, 'both milestone phases have summaries'); + assert.strictEqual(Number(output.progress.completed_phases), 2, 'both milestone phases have a passing verification'); }); test('total_phases includes ROADMAP phases without directories', () => { @@ -2097,6 +2102,10 @@ describe('milestone-scoped phase counting in frontmatter', () => { fs.mkdirSync(phaseDir, { recursive: true }); fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan'); fs.writeFileSync(path.join(phaseDir, `${padded}-01-SUMMARY.md`), '# Summary'); + // Disk-strict completion (ADR-3180 §7.4, #3186): give each of the 4 + // phases-with-directories a passing verification so this test still + // exercises ROADMAP-vs-disk SCOPING, not the completion predicate. + writePassedVerification(tmpDir, `${padded}-phase-${i}`, padded); } fs.writeFileSync( @@ -2112,7 +2121,7 @@ describe('milestone-scoped phase counting in frontmatter', () => { const output = JSON.parse(jsonResult.output); assert.strictEqual(Number(output.progress.total_phases), 6, 'should count all 6 ROADMAP phases, not just 4 with directories'); - assert.strictEqual(Number(output.progress.completed_phases), 4, 'only 4 phases have summaries'); + assert.strictEqual(Number(output.progress.completed_phases), 4, 'only 4 phases have a passing verification'); }); test('without ROADMAP counts all phases (pass-all filter)', () => { @@ -2339,6 +2348,12 @@ describe('progress counters correct after plan execution (#1589)', () => { fs.writeFileSync(path.join(phase02Dir, '02-02-PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(phase02Dir, '02-02-SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing *-VERIFICATION.md + // is what makes a phase complete now, not plan/summary parity — this test is + // about percent DERIVATION, so give both phases one. + writePassedVerification(tmpDir, '01-foundation', '01'); + writePassedVerification(tmpDir, '02-api', '02'); + // Body Progress: still says 0% (stale — never updated by update-progress) fs.writeFileSync( path.join(tmpDir, '.planning', 'STATE.md'), @@ -2416,6 +2431,14 @@ describe('progress counters correct after plan execution (#1589)', () => { fs.writeFileSync(path.join(phase04Dir, '04-02-PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(phase04Dir, '04-02-SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): give all 4 phases a + // passing verification so this test still exercises the STALE-FRONTMATTER + // rebuild, not the (now-separate) completion predicate. + writePassedVerification(tmpDir, '01-phase', '01'); + writePassedVerification(tmpDir, '02-phase', '02'); + writePassedVerification(tmpDir, '03-phase', '03'); + writePassedVerification(tmpDir, '04-phase', '04'); + // Write STATE.md with stale frontmatter matching the bug report exactly fs.writeFileSync( path.join(tmpDir, '.planning', 'STATE.md'), @@ -9973,6 +9996,10 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { fs.writeFileSync(path.join(plansDir, planFile), '# Plan\n'); fs.writeFileSync(path.join(plansDir, summaryFile), '# Summary\n'); } + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes a phase complete, not plan/summary + // parity — this test is about nested plans/ COUNTING, not the predicate. + writePassedVerification(tmpDir, phaseSlug, `0${phase}`); } writeRoadmap(tmpDir, [1, 2]); @@ -9987,7 +10014,7 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { const progress = JSON.parse(jsonResult.output).progress; assert.strictEqual(Number(progress.total_plans), 6, 'total_plans must count nested plans/ files (2 phases × 3 plans)'); assert.strictEqual(Number(progress.completed_plans), 6, 'completed_plans must count nested summary files (2 phases × 3 summaries)'); - assert.strictEqual(Number(progress.completed_phases), 2, 'completed_phases: both phases have summaries >= plans'); + assert.strictEqual(Number(progress.completed_phases), 2, 'completed_phases: both phases have a passing verification'); }); test('counts PLAN-NN-slug form (bare PLAN- prefix, no phase prefix)', () => { @@ -10025,6 +10052,9 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { fs.writeFileSync(path.join(phaseDir, '01-02-PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n'); fs.writeFileSync(path.join(phaseDir, '01-02-SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes the phase complete now. + writePassedVerification(tmpDir, '01-init', '01'); writeRoadmap(tmpDir, [1]); writeStateFile(tmpDir, { phase: '01' }); @@ -10038,7 +10068,7 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { const progress = JSON.parse(jsonResult.output).progress; assert.strictEqual(Number(progress.total_plans), 2, 'flat layout: top-level *-PLAN.md files counted'); assert.strictEqual(Number(progress.completed_plans), 2, 'flat layout: top-level *-SUMMARY.md files counted'); - assert.strictEqual(Number(progress.completed_phases), 1, 'flat layout: phase complete when summaries >= plans'); + assert.strictEqual(Number(progress.completed_phases), 1, 'flat layout: phase complete with a passing verification'); }); test('no double-count when both top-level and nested plan files coexist', () => { @@ -10081,6 +10111,9 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { // One top-level plan fs.writeFileSync(path.join(phaseDir, '01-01-PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(phaseDir, '01-01-SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes the phase complete now. + writePassedVerification(tmpDir, '01-init', '01'); writeRoadmap(tmpDir, [1]); writeStateFile(tmpDir, { phase: '01' }); @@ -10094,7 +10127,7 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { const progress = JSON.parse(jsonResult.output).progress; assert.strictEqual(Number(progress.total_plans), 1, 'empty plans/ must not add phantom plan count'); assert.strictEqual(Number(progress.completed_plans), 1, 'empty plans/ must not affect summary count'); - assert.strictEqual(Number(progress.completed_phases), 1, 'phase complete: 1 summary >= 1 plan'); + assert.strictEqual(Number(progress.completed_phases), 1, 'phase complete with a passing verification'); }); test('PLAN-OUTLINE.md files are excluded from nested plan count', () => { @@ -10168,6 +10201,9 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { fs.writeFileSync(path.join(plansDir, `${num}-PLAN-${pad}-task${p}.md`), '# Plan\n'); fs.writeFileSync(path.join(plansDir, `${num}-SUMMARY-${pad}-task${p}.md`), '# Summary\n'); } + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes a phase complete now. + writePassedVerification(tmpDir, `0${num}-phase-${num}`, `0${num}`); } writeRoadmap(tmpDir, [1, 2]); @@ -10182,7 +10218,7 @@ describe('buildStateFrontmatter nested plans/ layout (#3257)', () => { const progress = JSON.parse(jsonResult.output).progress; assert.strictEqual(Number(progress.total_plans), 7, 'reporter scenario: total_plans must be 7'); assert.strictEqual(Number(progress.completed_plans), 7, 'reporter scenario: completed_plans must be 7'); - assert.strictEqual(Number(progress.completed_phases), 2, 'reporter scenario: both phases complete'); + assert.strictEqual(Number(progress.completed_phases), 2, 'reporter scenario: both phases have a passing verification'); assert.strictEqual(Number(progress.percent), 100, 'reporter scenario: 100% when all plans have summaries'); }); }); @@ -10419,6 +10455,13 @@ describe('cmdStateSync nested plans/ layout (#3257)', () => { fs.writeFileSync(path.join(plansDir, `1-SUMMARY-${pad}-t.md`), '# Summary\n'); } } + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing *-VERIFICATION.md + // is what makes a phase complete now, not plan/summary counts alone. Phase + // 01-alpha is the one this test intends to be "complete" (fully planned and + // summarized), so give it a passing verification too — otherwise completed + // phases = 0 and no Progress change is emitted, which isn't what this test + // (nested plans/ summing correctly) is about. + writePassedVerification(tmpDir, '01-alpha', '01'); const stateContent = [ '# Project State', @@ -10692,6 +10735,10 @@ describe('buildStateFrontmatter cache invalidation (#1967)', () => { fs.mkdirSync(phase2); fs.writeFileSync(path.join(phase2, '02-1-PLAN.md'), '---\nphase: 2\nplan: 1\n---\n# Plan\n'); fs.writeFileSync(path.join(phase2, '02-1-SUMMARY.md'), '---\nstatus: complete\n---\n# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes a phase complete now — this test is about CACHE + // invalidation, not the completion predicate, so give phase 2 one. + fs.writeFileSync(path.join(phase2, '02-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); // Second write in the SAME process — must see the new phase const content2 = fs.readFileSync(statePath, 'utf-8'); @@ -11551,9 +11598,12 @@ describe('bug #1446 — state sync writes corrected (lower) total_phases', () => const dir = path.join(planning, 'phases', d); fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); - // Mark 01 and 02 as complete (2 summaries) + // Mark 01 and 02 as complete: disk-strict completion (ADR-3180 §7.4, + // #3186) requires a passing *-VERIFICATION.md, not just a summary — a + // summary alone no longer implies completion. if (d !== '03-gamma') { fs.writeFileSync(path.join(dir, 'PLAN-SUMMARY.md'), '# Summary\n', 'utf-8'); + fs.writeFileSync(path.join(dir, `${d.slice(0, 2)}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n', 'utf-8'); } } }); @@ -11576,10 +11626,14 @@ describe('bug #1446 — state sync writes corrected (lower) total_phases', () => 3, `total_phases must be corrected to 3 (derived), not kept at 10 (stale). Got ${state.progress.total_phases}`, ); - // completed_phases ratchet still works: existing 2 ≥ disk-derived → keep 2 - assert.ok( - state.progress.completed_phases >= 2, - `completed_phases must be at least 2 (ratchet). Got ${state.progress.completed_phases}`, + // Disk-strict (ADR-3180 §7.4, #3186): completed_phases is recomputed from + // disk on every sync (resync=true bypasses the curated-progress ratchet), + // so it must equal the number of phases with a passing *-VERIFICATION.md + // (01 and 02), not a preserved/ratcheted stale frontmatter value. + assert.equal( + state.progress.completed_phases, + 2, + `completed_phases must be 2 (01 and 02 have a passing verification). Got ${state.progress.completed_phases}`, ); }); }); @@ -11663,6 +11717,11 @@ function seedProject(prefix, roadmap, completeDirs) { if (completeDirs.includes(d)) { fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); fs.writeFileSync(path.join(dir, 'SUMMARY.md'), '# Summary\n', 'utf-8'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes a phase complete now, not a summary + // alone — this suite is about RETIRED-phase exclusion, not the + // completion predicate itself. + fs.writeFileSync(path.join(dir, `${d}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n', 'utf-8'); } } return tmpDir; @@ -11800,6 +11859,9 @@ function seedFromSpecs(prefix, specs) { if (s.shipped) { fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); fs.writeFileSync(path.join(dir, 'SUMMARY.md'), '# Summary\n', 'utf-8'); + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes a phase complete now. + fs.writeFileSync(path.join(dir, `${s.dir}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n', 'utf-8'); } } return tmpDir; @@ -11889,6 +11951,9 @@ describe('bug #1514 — retired exclusion across phase shapes', () => { fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(path.join(dir, 'PLAN.md'), '# Plan\n', 'utf-8'); fs.writeFileSync(path.join(dir, 'SUMMARY.md'), '# Summary\n', 'utf-8'); + // Disk-strict completion (ADR-3180 §7.4, #3186): only a passing + // *-VERIFICATION.md makes a phase complete now. + fs.writeFileSync(path.join(dir, `${d}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n', 'utf-8'); } tmpDir = tmp; const result = runGsdTools(['state', 'json'], tmpDir); diff --git a/tests/workstream-inventory.test.cjs b/tests/workstream-inventory.test.cjs index f538cf2e7..d311bc66d 100644 --- a/tests/workstream-inventory.test.cjs +++ b/tests/workstream-inventory.test.cjs @@ -147,14 +147,22 @@ describe('#2562 — progress/status scoped to the current milestone (derived fro }; // ── Defect 3: verification-gated completeness (builder unit) ───────────────── + // ADR-3180 §7.4 (#3186 review finding 3): `complete` is now the CALLER- + // computed owner verdict (`isPhaseComplete`), passed in per phase via + // `PhaseFilesCount.complete` — the builder no longer re-derives it from + // `verificationStatus` + counts. These builder-unit fixtures pass + // `complete` directly (mirroring what `workstream-inventory.cts` computes + // from a real `isPhaseComplete(phaseDir)` call in production); `verificationStatus` + // stays on the fixture only because the `PhaseFilesCount` type still carries + // it (informational, unconsumed by `status`). test('builder: SUMMARY≥PLAN but a human_needed verdict is NOT complete', () => { const inv = buildWorkstreamInventory({ ...BUILDER_BASE, name: 'ws', phaseDirNames: ['1-a', '2-b'], phaseFilesCounts: [ - { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, - { directory: '2-b', planCount: 4, summaryCount: 4, inMilestone: true, verificationStatus: 'human_needed' }, + { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, + { directory: '2-b', planCount: 4, summaryCount: 4, inMilestone: true, verificationStatus: 'human_needed', complete: false }, ], roadmapPhaseCount: 2, currentMilestonePhaseCount: 2, @@ -164,19 +172,25 @@ describe('#2562 — progress/status scoped to the current milestone (derived fro assert.equal(inv.progress_percent, 50); }); - test('builder: missing/unknown verdict still counts complete (no verifier-off regression)', () => { + // ADR-3180 §7.4 (#3186 review finding 3): disk-strict retires this + // tolerance. `isPhaseComplete` requires `verification.status === 'passed'` + // UNCONDITIONALLY — a 'missing' verdict (no `*-VERIFICATION.md`, e.g. a + // verifier-disabled project) is never complete, matching `roadmap analyze` + // / `init manager` / `phase complete` exactly. Disclosed in this phase's + // changeset. + test('builder: a missing verdict is NOT complete (verifier-off tolerance retired, disk-strict)', () => { const inv = buildWorkstreamInventory({ ...BUILDER_BASE, name: 'ws', phaseDirNames: ['1-a'], phaseFilesCounts: [ - { directory: '1-a', planCount: 2, summaryCount: 2, inMilestone: true, verificationStatus: 'missing' }, + { directory: '1-a', planCount: 2, summaryCount: 2, inMilestone: true, verificationStatus: 'missing', complete: false }, ], roadmapPhaseCount: 1, currentMilestonePhaseCount: 1, }); - assert.equal(inv.phases[0].status, 'complete'); - assert.equal(inv.progress_percent, 100); + assert.equal(inv.phases[0].status, 'in_progress'); + assert.equal(inv.progress_percent, 0); }); // ── Defect 2: denominator includes declared-but-unscaffolded phases (builder) ─ @@ -186,8 +200,8 @@ describe('#2562 — progress/status scoped to the current milestone (derived fro name: 'ws', phaseDirNames: ['1-a', '2-b'], // phase 3 declared for the milestone but never scaffolded phaseFilesCounts: [ - { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, - { directory: '2-b', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, + { directory: '2-b', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, ], roadmapPhaseCount: 2, currentMilestonePhaseCount: 3, @@ -204,8 +218,8 @@ describe('#2562 — progress/status scoped to the current milestone (derived fro name: 'ws', phaseDirNames: ['1-old', '2-cur'], phaseFilesCounts: [ - { directory: '1-old', planCount: 3, summaryCount: 3, inMilestone: false, verificationStatus: 'passed' }, - { directory: '2-cur', planCount: 2, summaryCount: 0, inMilestone: true, verificationStatus: 'missing' }, + { directory: '1-old', planCount: 3, summaryCount: 3, inMilestone: false, verificationStatus: 'passed', complete: true }, + { directory: '2-cur', planCount: 2, summaryCount: 0, inMilestone: true, verificationStatus: 'missing', complete: false }, ], roadmapPhaseCount: 2, currentMilestonePhaseCount: 1, @@ -678,18 +692,21 @@ describe('#2562 — milestone scoping boundaries (one phase-key derivation)', () milestoneShipped: false, phaseDirNames: ['1-a', '2-b', '3-c'], phaseFilesCounts: [ - { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, - { directory: '2-b', phaseKey: '02', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, - { directory: '3-c', phaseKey: '03', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, + { directory: '2-b', phaseKey: '02', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, + { directory: '3-c', phaseKey: '03', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed', complete: true }, ], roadmapPhaseCount: 3, currentMilestonePhaseCount: 2, }), /invariant violated/); }); - // The builder hand-lists the verdicts that disqualify a phase from `complete`. - // Pin it to the verifier's own vocabulary so a new emitted status cannot land - // without a decision here. + // ADR-3180 §7.4 (#3186 review finding 3): the builder no longer hand-lists + // disqualifying verdicts itself (`FAILING_VERIFICATION_STATUSES` is + // retired) — it trusts the caller-supplied `complete` boolean entirely. + // The vocabulary pin now lives at the OWNER (`isPhaseComplete`, + // `complete: verification.status === 'passed'`), mirrored here for every + // verifier status other than 'passed'. test('parity: every verifier status other than passed blocks completeness', () => { const nonPassing = VERIFIER_STATUSES.filter(s => s !== 'passed'); assert.ok(nonPassing.length > 0, 'guard: the verifier must emit a non-passing status'); @@ -704,7 +721,7 @@ describe('#2562 — milestone scoping boundaries (one phase-key derivation)', () milestoneShipped: false, phaseDirNames: ['1-a'], phaseFilesCounts: [ - { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: status }, + { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: status, complete: status === 'passed' }, ], roadmapPhaseCount: 1, currentMilestonePhaseCount: 1, @@ -1089,9 +1106,15 @@ describe('#2645 — deleting a verification report must not raise completeness', assert.equal(after.progress_percent, 0); }); - // Row 3 — criterion 2: verifier-disabled projects (no report ever written) - // must still be able to reach 100%. - test('a phase that was never verified still reaches complete (#2645, criterion 2)', () => { + // Row 3 — SUPERSEDED by ADR-3180 §7.4 (#3186, disk-strict): #2645's + // criterion 2 ("verifier-disabled projects must still reach 100%") is + // exactly the site-local tolerance disk-strict retires. `complete` now + // routes through the single canonical owner (`isPhaseComplete`), which + // requires `verification.status === 'passed'` UNCONDITIONALLY — a phase + // with NO `*-VERIFICATION.md` reads 'missing', never complete, regardless + // of how many plans it has summarized. Disclosed in this phase's + // changeset. + test('a phase that was never verified is NOT complete (disk-strict; #2645 criterion 2 retired)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-never' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1101,15 +1124,15 @@ describe('#2645 — deleting a verification report must not raise completeness', const inv = inspectWorkstream(tmpDir, 'ws-2645-never', { active: null }); assert.ok(inv); - assert.equal(inv.phases[0].status, 'complete'); - assert.equal(inv.completed_phases, 1); - assert.equal(inv.progress_percent, 100); + assert.equal(inv.phases[0].status, 'in_progress'); + assert.equal(inv.completed_phases, 0); + assert.equal(inv.progress_percent, 0); }); - // Row 4 — criterion 3: same on-disk shape as row 3 (no file), across TWO - // reads, must behave IDENTICALLY — the ledger must not newly gate a phase - // that was simply never verified in the first place. - test('a not-yet-verified phase is not newly gated by the ledger (#2645, criterion 3)', () => { + // Row 4 — SUPERSEDED by ADR-3180 §7.4: same on-disk shape as row 3 (no + // file), across TWO reads — disk-strict requires this to behave + // IDENTICALLY (never complete) both times, not just consistently. + test('a not-yet-verified phase reads NOT complete on every read (disk-strict; #2645 criterion 3 retired)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-not-yet' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1119,14 +1142,21 @@ describe('#2645 — deleting a verification report must not raise completeness', const first = inspectWorkstream(tmpDir, 'ws-2645-not-yet', { active: null }); const second = inspectWorkstream(tmpDir, 'ws-2645-not-yet', { active: null }); - assert.equal(first.progress_percent, 100); - assert.equal(second.progress_percent, 100, 'a second read must not change the outcome'); - assert.equal(second.phases[0].status, 'complete'); + assert.equal(first.progress_percent, 0); + assert.equal(second.progress_percent, 0, 'a second read must not change the outcome'); + assert.equal(second.phases[0].status, 'in_progress'); }); - // Row 5 — recovery: a genuinely re-verified phase must not be pinned by an - // earlier failing verdict the ledger remembers. - test('a re-verified passed phase is not pinned by an earlier failing ledger entry (#2645)', () => { + // Row 5 — the FIRST half (a genuine re-verify counts) is unchanged. The + // SECOND half is SUPERSEDED by ADR-3180 §7.4: `isPhaseComplete` reads + // fresh off disk, UNCONDITIONALLY, with no memory — the ledger's "hold the + // newest real verdict after the file is deleted" behavior is structurally + // incompatible with a single owner that never consults a ledger. Deleting + // ANY `*-VERIFICATION.md` (passing or failing) now uniformly reads + // 'missing' → not complete, matching `roadmap analyze` / `init manager` / + // `phase complete` for the identical disk state. Disclosed in this + // phase's changeset. + test('a re-verified passed phase counts complete; deleting the report afterward is NOT complete (disk-strict; #2645 memory retired)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-recover' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1140,12 +1170,14 @@ describe('#2645 — deleting a verification report must not raise completeness', const reverified = inspectWorkstream(tmpDir, 'ws-2645-recover', { active: null }); assert.equal(reverified.phases[0].status, 'complete', 'a genuine re-verify must count'); - // Delete the now-passing report — the ledger's newest real verdict is - // 'passed', which is not in the failing set, so this must stay complete. + // Delete the now-passing report — the owner reads fresh off disk every + // time; no ledger memory feeds into `complete` anymore, so this reads + // 'missing' → not complete, exactly like every other disk-strict + // consumer for the same disk state. fs.unlinkSync(verificationFilePath(wsDir, '1-foo')); const afterDelete = inspectWorkstream(tmpDir, 'ws-2645-recover', { active: null }); - assert.equal(afterDelete.phases[0].status, 'complete', - 'the ledger must hold the newest verdict, not an earlier failing one'); + assert.equal(afterDelete.phases[0].status, 'in_progress', + 'disk-strict: a deleted verification file is never complete, regardless of what was previously observed'); }); // Row 6 — criterion 4: the ledger must not live inside the phase directory @@ -1171,10 +1203,17 @@ describe('#2645 — deleting a verification report must not raise completeness', assert.ok(fs.existsSync(ledgerPath), 'removing the phase directory must not remove the ledger'); }); - // Row 7 — Bug #2445 dedup safety: a stale duplicate directory sharing the - // same phase key must not be able to clobber the WINNING (newest) - // directory's ledger entry with its own stale/leftover verdict. - test('a stale duplicate directory cannot clobber the ledger entry of the live directory (#2645)', () => { + // Row 7 — Bug #2445 dedup safety (winner SELECTION, still real): a stale + // duplicate directory sharing the same phase key must never be the one + // whose live verdict the rollup counts. SUPERSEDED for the DELETION half + // by ADR-3180 §7.4: `isPhaseComplete` reads the WINNING directory fresh + // off disk on every call, unconditionally — once its report is deleted, + // the winner's own live read is 'missing', so this phase key correctly + // stops counting as complete (no ledger memory left to "remember" the + // pre-deletion 'passed'). The winner-selection guarantee itself (the stale + // duplicate's gaps_found never contaminates the live directory's result) + // still holds and is still what this test pins. + test('a stale duplicate directory never contaminates the live directory (winner selection; #2645 memory retired)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-dupe' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1192,31 +1231,27 @@ describe('#2645 — deleting a verification report must not raise completeness', const before7 = inspectWorkstream(tmpDir, 'ws-2645-dupe', { active: null }); assert.ok(before7); - assert.equal(before7.completed_phases, 1, 'the winning (newest) directory is passed'); + assert.equal(before7.completed_phases, 1, 'the winning (newest) directory is passed — the stale duplicate never won selection'); - // Delete the WINNING directory's report. If the stale directory's - // gaps_found had clobbered the ledger, this phase key would now - // incorrectly read gaps_found and never recover. It must instead read - // the winning directory's own remembered 'passed'. + // Delete the WINNING directory's report. Disk-strict: the winner's own + // live read is now 'missing', so this phase key stops counting complete + // — but it must NOT flip to the stale duplicate's gaps_found either + // (that would be a DIFFERENT bug: the stale directory winning selection). fs.unlinkSync(path.join(liveDir, '01-VERIFICATION.md')); const after7 = inspectWorkstream(tmpDir, 'ws-2645-dupe', { active: null }); assert.ok(after7); - assert.equal(after7.completed_phases, 1, - "the ledger must remember the WINNING directory's passed verdict, not the stale duplicate's gaps_found"); + assert.equal(after7.completed_phases, 0, + 'disk-strict: the winning directory\'s own deleted report is not complete — no ledger memory papers over it'); + assert.equal(after7.phases.find(p => p.directory === '1-foo').status, 'in_progress'); }); // Row 8 — boundary: an EXACT mtime tie between two same-keyed directories. // The builder's own tie-break (`rollupDirByKey`) walks // `[...phaseDirNames].sort()` and keeps the incumbent on a tie // (first-in-sort-order wins, since only a STRICTLY newer mtime replaces - // it). The ledger's winner selection must walk the SAME sorted order so - // the two deterministically agree on which directory wins — not merely - // "some directory wins" (a prior version of this test asserted only - // `completed_phases === 0 || 1`, which is true regardless of agreement and - // caught nothing; both `01-foo-a`/`01-foo-b` sort deterministically, so the - // outcome here is not a coin flip). - test('an exact mtime tie resolves the ledger winner by sort order, matching the builder (#2645)', () => { + // it). SUPERSEDED for the deletion half by ADR-3180 §7.4 — see Row 7. + test('an exact mtime tie resolves the winner by sort order, matching the builder (#2645 memory retired)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-tie' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1224,8 +1259,8 @@ describe('#2645 — deleting a verification report must not raise completeness', ])); const tieTime = new Date('2025-06-01T00:00:00Z'); // '01-foo-a' sorts before '01-foo-b' — with equal mtimes, BOTH the - // builder's rollup and the ledger's winner selection must keep the - // incumbent '01-foo-a' (passed), never adopt '01-foo-b' (gaps_found). + // builder's rollup and the winner selection must keep the incumbent + // '01-foo-a' (passed), never adopt '01-foo-b' (gaps_found). const dirA = writePhase(wsDir, '01-foo-a', { plans: 1, summaries: 1, verification: 'passed' }); fs.utimesSync(dirA, tieTime, tieTime); const dirB = writePhase(wsDir, '01-foo-b', { plans: 1, summaries: 1, verification: 'gaps_found' }); @@ -1239,44 +1274,29 @@ describe('#2645 — deleting a verification report must not raise completeness', assert.equal(inv.completed_phases, 1, 'the sort-order incumbent (01-foo-a, passed) must be the one the builder counts complete'); - // Concrete cross-check that the LEDGER's winner selection agrees with the - // builder's, not just that this run's numbers happen to match: delete - // 01-foo-a's report (unlink bumps the directory's mtime — restore it to - // the exact tie value so the SECOND read still sees a genuine tie, not a - // newest-mtime win). If the ledger's winner selection had instead picked - // 01-foo-b (the bug this test guards — unsorted iteration disagreeing - // with the builder's sorted `rollupDirByKey`), this phase key's - // remembered verdict would be 'gaps_found' and the phase would flip to - // in_progress. It must instead stay complete, because the ledger - // remembers 01-foo-a's 'passed' — the same directory the builder uses. + // Delete 01-foo-a's report (unlink bumps the directory's mtime — restore + // it to the exact tie value so the SECOND read still sees a genuine tie, + // not a newest-mtime win). Disk-strict: the incumbent's own live read is + // now 'missing', so completed_phases must drop to 0 — it must NOT flip + // to 01-foo-b's gaps_found winning selection instead (that would be the + // sort-order-disagreement bug this test also guards). fs.unlinkSync(path.join(dirA, '01-VERIFICATION.md')); fs.utimesSync(dirA, tieTime, tieTime); // restore the tie the unlink disturbed const afterDelete = inspectWorkstream(tmpDir, 'ws-2645-tie', { active: null }); - assert.equal(afterDelete.completed_phases, 1, - "the ledger must have remembered the sort-order incumbent's 'passed' verdict, not the other directory's gaps_found"); + assert.equal(afterDelete.completed_phases, 0, + 'disk-strict: the sort-order incumbent\'s own deleted report is not complete, and 01-foo-b never wins selection instead'); }); - // Row 9 — property: whatever sequence of REAL verdicts is observed for a - // phase key, once the file goes missing the ledger must replay exactly the - // LAST one observed — never an earlier one, never a synthesized value. - // - // #2645 review: `'stale'` is deliberately EXCLUDED here, not merely - // forgotten. Writing a literal `status: stale` frontmatter value does NOT - // round-trip as `'stale'` — `readVerificationStatus` - // (`src/verification.cts`) explicitly excludes `'stale'` from its raw-file - // routing lookup (`rawStatus !== 'stale'`) and falls through to the - // "Unknown value" branch, returning `'unknown'` instead. A genuine - // `'stale'` verdict is only reachable via `findStaleVerificationSummary`'s - // mtime comparison (a SUMMARY file newer than the VERIFICATION file), not - // by writing the word into the file. Including `'stale'` in this array - // without accounting for that would silently substitute `'unknown'` on - // every iteration and still pass (both are non-failing) — a docstring - // claiming "real verdicts" coverage it does not actually exercise. - // `'stale'` handling is explicitly out of scope for this issue (#2348) and - // is not gated by `FAILING_VERIFICATION_STATUSES` either way. - test('property: the ledger always replays the most recently observed real verdict after deletion (#2645)', () => { + // Row 9 — SUPERSEDED by ADR-3180 §7.4 (#3186, disk-strict): the ledger's + // "replay the last real verdict after deletion" memory is retired — + // `isPhaseComplete` reads fresh off disk, unconditionally, every call. + // The property this test now pins is simpler and STRONGER than the + // #2645-era one: whatever sequence of REAL verdicts was observed, once + // the file goes missing the phase is NEVER complete — full stop, not + // "unless the last real verdict was passed/unknown". Disclosed in this + // phase's changeset. + test('property: after the report goes missing, the phase is NEVER complete regardless of prior history (disk-strict; #2645 memory retired)', () => { const REAL_STATUSES = ['passed', 'gaps_found', 'human_needed', 'unknown']; - const FAILING = new Set(['gaps_found', 'human_needed']); fc.assert(fc.property( fc.array(fc.constantFrom(...REAL_STATUSES), { minLength: 1, maxLength: 6 }), (sequence) => { @@ -1289,18 +1309,15 @@ describe('#2645 — deleting a verification report must not raise completeness', const dir = writePhase(wsDir, '1-foo', { plans: 1, summaries: 1 }); const reportPath = path.join(dir, '01-VERIFICATION.md'); - let lastReal = null; for (const status of sequence) { fs.writeFileSync(reportPath, `---\nstatus: ${status}\n---\n`); inspectWorkstream(tmpDir, wsName, { active: null }); // observe - lastReal = status; } fs.unlinkSync(reportPath); const inv = inspectWorkstream(tmpDir, wsName, { active: null }); - const expectComplete = !FAILING.has(lastReal); - assert.equal(inv.phases[0].status === 'complete', expectComplete, - `after observing ${JSON.stringify(sequence)} then deleting, status must reflect the last real verdict (${lastReal})`); + assert.equal(inv.phases[0].status, 'in_progress', + `disk-strict: after observing ${JSON.stringify(sequence)} then deleting the report, the phase must never be complete`); cleanup(wsDir); }, @@ -1391,19 +1408,15 @@ describe('#2645 — deleting a verification report must not raise completeness', assert.ok(fs.existsSync(ledgerPath), 'a read that observes a real verdict must recreate/self-heal the ledger'); }); - // Row 14 — the DISCLOSED, ACCEPTED residual gap, pinned deliberately so it - // is never mistaken for a silent regression: deleting the report AND the - // ledger TOGETHER, after a failing verdict was already observed and - // recorded, returns this phase key to the pre-adoption `'absent'` ledger - // state — indistinguishable, by design, from a workstream that never used - // the verifier at all (criteria 2/3 require exactly that indistinguishability - // for a workstream that HASN'T adopted the ledger). This is the "prospective - // only" limitation named in the changeset: any durable store that must fail - // OPEN when wholly absent (to avoid gating every pre-existing project on - // upgrade) has this property at its own root. The bar is raised from "delete - // one file" to "delete two files in two different directories, one of which - // this issue's own reproduction never needed to touch" — not eliminated. - test('deleting the report AND the ledger together reopens the pre-adoption window (documented, not a regression) (#2645)', () => { + // Row 14 — SUPERSEDED by ADR-3180 §7.4 (#3186, disk-strict): the + // "prospective only" residual gap this row used to document (removing + // BOTH files reopens the pre-adoption 'absent' window, which USED to be + // allowed to complete because criteria 2/3 tolerated a never-verified + // phase) no longer exists — criteria 2/3 themselves are retired (Rows 3/4 + // above). Deleting the report and/or the ledger, in any combination, now + // uniformly reads 'missing' → not complete. There is no residual gap left + // to disclose for this row. + test('deleting the report AND the ledger together is still NOT complete (disk-strict; #2645 residual gap closed)', () => { const wsDir = seedWorkstream(tmpDir, { name: 'ws-2645-delete-both' }); fs.writeFileSync(path.join(wsDir, 'STATE.md'), FLAT_STATE); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), flatRoadmap([ @@ -1420,9 +1433,8 @@ describe('#2645 — deleting a verification report must not raise completeness', const inv = inspectWorkstream(tmpDir, 'ws-2645-delete-both', { active: null }); assert.ok(inv); - assert.equal(inv.phases[0].status, 'complete', - 'documented limitation: removing BOTH files returns this phase to the pre-adoption/never-verified state — ' + - 'this is the accepted "prospective only" boundary, not a bug in this fix'); + assert.equal(inv.phases[0].status, 'in_progress', + 'disk-strict: removing both files still reads \'missing\' — never complete, regardless of ledger adoption state'); }); // Row 11 — fault injection per CONTRIBUTING.md: read-only target directory diff --git a/tests/workstream.test.cjs b/tests/workstream.test.cjs index ed1620fcf..0455e3720 100644 --- a/tests/workstream.test.cjs +++ b/tests/workstream.test.cjs @@ -624,6 +624,12 @@ describe('getOtherActiveWorkstreams', () => { fs.writeFileSync(alphaPlan, '# Plan\n'); fs.writeFileSync(betaPlan, '# Plan\n'); fs.writeFileSync(betaSummary, '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes beta's phase count as complete now. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'workstreams', 'beta', 'phases', '01-beta', '01-VERIFICATION.md'), + '---\nstatus: passed\n---\n# Verification\n', + ); const others = getOtherActiveWorkstreams(tmpDir, 'alpha'); assert.strictEqual(others.length, 1); @@ -646,6 +652,9 @@ describe('workstream progress', () => { fs.mkdirSync(path.join(wsDir, 'phases', '01-init'), { recursive: true }); fs.writeFileSync(path.join(wsDir, 'phases', '01-init', 'PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(wsDir, 'phases', '01-init', 'SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes phase 01 count as complete now. + fs.writeFileSync(path.join(wsDir, 'phases', '01-init', '01-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); fs.writeFileSync(path.join(wsDir, 'STATE.md'), '# State\n**Status:** In progress\n**Current Phase:** 2\n'); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), '## Roadmap\n### Phase 1: Init\n### Phase 2: Build\n'); fs.writeFileSync(path.join(tmpDir, '.planning', 'active-workstream'), 'feature\n'); @@ -674,6 +683,9 @@ describe('workstream progress', () => { fs.mkdirSync(phaseDir, { recursive: true }); fs.writeFileSync(path.join(phaseDir, 'PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(phaseDir, 'SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes each phase count as complete now. + fs.writeFileSync(path.join(phaseDir, `${phase.slice(0, 2)}-VERIFICATION.md`), '---\nstatus: passed\n---\n# Verification\n'); } fs.writeFileSync(path.join(wsDir, 'STATE.md'), '# State\n**Status:** In progress\n'); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), '# Roadmap\n### Phase 1: One\n'); @@ -863,6 +875,9 @@ describe('#2562 — a refused shipped marker reaches every workstream command', fs.mkdirSync(path.join(wsDir, 'phases', '1-foo'), { recursive: true }); fs.writeFileSync(path.join(wsDir, 'phases', '1-foo', '01-PLAN.md'), '# Plan\n'); fs.writeFileSync(path.join(wsDir, 'phases', '1-foo', '01-SUMMARY.md'), '# Summary\n'); + // Disk-strict completion (ADR-3180 §7.4, #3186): a passing + // *-VERIFICATION.md is what makes phase 1 count as complete now. + fs.writeFileSync(path.join(wsDir, 'phases', '1-foo', '01-VERIFICATION.md'), '---\nstatus: passed\n---\n# Verification\n'); fs.writeFileSync(path.join(wsDir, 'STATE.md'), 'milestone: v2.0\nstatus: executing\n'); fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ '# Roadmap', '', '## Milestone v2.0 — Two', '', '## Progress', '',