From aceea3ce4ac66561e896558b35c76a3e090d5ac7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 10 Aug 2026 11:59:51 -0400 Subject: [PATCH] refactor(#3217): withhold a percentage when its scope is not complete (#3318) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * wip(#3217): rule-4 scope withholding — parked, two open findings Implemented but NOT shippable. An isolated review found buildStateFrontmatter still hardcodes SCOPE.COMPLETE, so state json reports percent 0 where roadmap analyze, stats and query progress all correctly report null on the same disk state - rule 4 reintroduced at a site this phase claims to close. Also: roadmap analyze emits scope complete beside progress_percent null with nothing explaining it. Parked to build Phase 4 (#3186) first, which is unblocked. Findings recorded in .gsd/phase/refactor-3217-completion-ratio-scoping/60-review.json. * fix(#3217): withhold the sync percentage on a non-complete scope The parked blocker is fixed - buildStateFrontmatter no longer hardcodes SCOPE.COMPLETE, and the prose Progress fallback is gated too, which was a second leak found while tracing the first. roadmap analyze exposes progress_scope so a consumer can tell WHY a percentage is absent from the JSON alone. Then a residual gap was reproduced rather than assumed. cmdStateSync carried the same hardcode behind a written reason claiming it did not reproduce. It did: on a TRUNCATED window and on UNSCOPED row 4, state sync wrote Progress 0 percent to 100 percent while state json, roadmap analyze, stats and query progress all withheld - and it persisted a self-contradictory file, body claiming 100 percent while its own frontmatter correctly omitted percent. The excuse was also wrong. syncRoadmapRaw is already parsed in that function and is exactly what produces a real scope, so there was a scope to pass. Threaded through listMilestonePhaseDirs; a non-complete scope now skips the write with a reason in changes. milestoneBounded stays as the orthogonal 1761 guard for row 5. Second time this epic a does-not-reproduce claim was too generous. Recorded in ADR Amendment 8 as a correction rather than a quiet rewrite. Verified on the remote runner. * test(#3217): give the withholding fixtures a resolvable scope 40 matrix failures, all fixture drift - no code regression. My own hypothesis that this was over-withholding was wrong and is recorded as such: the worry case, a plain ROADMAP with Phase entries and no version heading, resolves to complete exactly as ADR 7.1 says it should. The real causes were two fixture shapes. Most had no ROADMAP.md at all, which is unreadable via a pre-existing graceful path, and asserted a numeric percent. The five vscode, pi-extension, mcp-server and shell-projection failures were that shape - bare temp dirs using progress json as a reachability proxy while asserting typeof percent is number, which under rule 4 is now null. The rest had a version token in a title or heading with no STATE.md milestone pointer to resolve it, which is classification row 4, versioned but unresolved, so withholding is correct per the contract. Verified on the remote runner. * test(#3217): make the LM-tools reachability tests dispatch against their fixture The gsd_progress reachability test was never testing its fixture. invoke() resolves cwd from vscode.workspace.workspaceFolders by design (the real LanguageModelToolInvocationOptions has no cwd field, per the 2103 fix in extension.js), the mock had no workspace at all, and the test passed a cwd option nothing reads - so it dispatched against the repo working directory. Writing a ROADMAP into the temp dir had no effect. Rule 4 only made it visible. Fixed by mocking workspaceFolders. The two siblings in the same file carried the identical dead cwd and were dispatching against the repo too; they were not failing only because their assertions did not touch scope-dependent output. Both now use their own fixture with assertions unchanged - the no-planning fallback paths already satisfy them honestly. Re-scanned the other five reachability files: no further instances. They thread cwd into parameters that genuinely read it, not through an options shape that ignores it. Verified on the remote runner. * chore(#3217): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3318 exists. * ci(#3217): give the coverage merge enough heap for the merged shards The coverage gate OOMed at exit 134. c8 report merges three shard artifacts, roughly 358MB of V8 dumps in coverage/tmp, and died holding their per-file position maps at the ~4GB default heap. Verified as this branch's delta rather than pre-existing: the same job succeeded on next at 14:18, after phases 4 and 5 merged. Both coverage-gate steps get the bump because both re-slice the same merged data. 8192 doubles what failed and leaves headroom on a 16GB ubuntu runner, matching the idiom the shard step already uses at 6144. This is a memory bound, not a change to what is measured. No threshold was touched. The test file was checked for gratuitous subprocess spawning and is already reasonable at 43 spawns, each a distinct fixture-by-surface pairing. Verified on the remote runner. --------- Co-authored-by: sim --- .changeset/bold-otters-scope.md | 6 + .github/workflows/test.yml | 13 + docs/CLI-TOOLS.md | 31 + docs/COMMANDS.md | 4 + ...80-planning-semantic-model-single-owner.md | 181 ++++ src/commands.cts | 36 +- src/roadmap.cts | 57 +- src/state-document.cts | 15 +- src/state.cts | 85 +- src/workstream-inventory-builder.cts | 21 + tests/commands.test.cjs | 24 +- ...ompletion-ratio-scope-withholding.test.cjs | 826 ++++++++++++++++++ tests/frontmatter.test.cjs | 12 +- tests/gsd-mcp-server.test.cjs | 6 + tests/pi-extension-reachability.test.cjs | 13 + tests/roadmap.test.cjs | 6 +- ...shell-command-projection-dispatch.test.cjs | 6 + tests/state.test.cjs | 27 +- tests/vscode-extension-reachability.test.cjs | 7 + tests/vscode-lm-tools.test.cjs | 42 +- tests/vscode-subagent-dispatch.test.cjs | 8 + 21 files changed, 1393 insertions(+), 33 deletions(-) create mode 100644 .changeset/bold-otters-scope.md create mode 100644 tests/completion-ratio-scope-withholding.test.cjs diff --git a/.changeset/bold-otters-scope.md b/.changeset/bold-otters-scope.md new file mode 100644 index 000000000..be26fe8bd --- /dev/null +++ b/.changeset/bold-otters-scope.md @@ -0,0 +1,6 @@ +--- +type: Changed +pr: 3318 +--- + +**A percentage is now withheld everywhere its scope is not `COMPLETE`, not just at the sites Phase 3 reached** — closing ADR-3180 §7.6 rule 4 at the two remaining gaps an isolated review caught: `state json`'s `buildStateFrontmatter` no longer hardcodes `SCOPE.COMPLETE` when deriving `progress.percent` (it now threads the real `listMilestonePhaseDirs` scope through `_diskScanCache`, including its prose-fallback path, so a genuinely unreadable `.planning/phases` directory can no longer surface a stale or falsely-earned number there while every other surface withholds), and `roadmap analyze --json` now exposes the scope that actually gates `progress_percent` as its own `progress_scope` field — distinct from the top-level `scope` (heading-windowing identity) — so a consumer can tell *why* `progress_percent` is `null` from the JSON alone instead of seeing `scope: "complete"` next to an unexplained `null`. `state update-progress` also now writes a `[gsd-tools] WARNING:` line to stderr when it silently no-ops on a non-`COMPLETE` scope, so the skip is not visible only to a JSON `reason` field most callers never read. **`state sync` now also withholds**: it no longer hardcodes `SCOPE.COMPLETE` when deriving the percentage it writes into `STATE.md`'s body — a non-`COMPLETE` scope (confirmed reproducible on `TRUNCATED` and `UNSCOPED` fixtures, not just the previously-checked `UNREADABLE` case) skips the `Progress:` write entirely and records a `Progress: skipped — …(#3217)` entry in `changes`, instead of persisting a fabricated percentage that could disagree with the same write's own (already-scoped) frontmatter `progress:` block. `0` under a genuinely `COMPLETE` scope is unaffected and still renders. Tier-2: `progress_percent`, `percent`, and `plan_percent` are `number | null`; `computeProgressPercent` requires a `scope` argument; `roadmap analyze --json` gains a new `progress_scope` field; `state sync --raw`'s `changes` array can now contain a scope-skip entry and correspondingly withhold a `Progress:` body write it would previously have made. (#3217) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 19240a797..ccaa5a59e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -585,14 +585,27 @@ jobs: # c8's default --temp-directory is ./coverage/tmp, which is exactly where # the download above landed every shard's dumps — so these run unmodified # against merged data and the thresholds stay defined in package.json. + # The three merged shard dumps run ~358MB combined; c8 has to parse and + # hold all of their per-file position maps in memory at once to render a + # report, which blows past the default ~4GB V8 heap (observed: FATAL + # ERROR: Ineffective mark-compacts near heap limit). 8192 doubles that + # with headroom to spare on the 16GB ubuntu-latest runner. This is a + # memory bound only — it does not change what is measured or the + # thresholds below. - name: Report merged coverage + gate gsd-core/bin/lib (≥70% lines, ≥60% branches) + env: + NODE_OPTIONS: --max-old-space-size=8192 run: npm run test:coverage:report # Second-tier floor over the CI/release/lint tooling itself. Re-slices the # SAME merged V8 data — no extra suite execution. Audit 2026-06: scripts/ # measured 65.95%; the 55% floor prevents a collapse to zero-coverage # tooling while leaving headroom for variance. Raise deliberately, never lower. + # Re-slices the same ~358MB merged dumps as the step above, so it carries + # the same larger heap for the same reason — see that step's comment. - name: Coverage floor — scripts/ tooling (≥55%) + env: + NODE_OPTIONS: --max-old-space-size=8192 run: npm run test:coverage:scripts-floor - name: Upload merged coverage report diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 84a9d7d9a..9d4eec668 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -230,6 +230,37 @@ shape) reports `complete`: the whole document *is* the milestone there. Note this answer is specific to *windowing* — see the next section for why milestone *identity* answers the same document differently. +### A non-`COMPLETE` scope withholds the percentage entirely (#3217) + +`roadmap analyze --json`'s `progress_percent`, `stats --raw`'s `percent` / +`plan_percent`, `query progress --raw`'s `percent`, and `state json --raw`'s +`progress.percent` are now **nullable** — a Tier-2 contract change. When the +phase set a percentage would be computed from is not fully trustworthy (any +scope other than `complete`), these surfaces render **no percentage at all** +rather than a number computed from a truncated, unscoped, or unreadable set: + +| Surface | Non-`complete` behavior | +|---|---| +| `roadmap analyze --json` | `progress_percent: null` | +| `stats --raw` | `percent: null`, `plan_percent: null` | +| `query progress --raw` | `percent: null` | +| `state json --raw` | `progress.percent` is **omitted** from the `progress` object (not `0`, not present as `null`) | +| `state update-progress --raw` | `false` — no write; `STATE.md`'s Progress field is left untouched, and a `[gsd-tools] WARNING:` line is written to stderr naming the scope | + +`0` is a legitimate, real answer under a `complete` scope (e.g. a +freshly-declared milestone with zero phases, or a phase with zero plan files) +and is never withheld — only a non-`complete` scope withholds. + +`roadmap analyze --json` gates `total_plans` / `total_summaries` / `phases` / +`completed_phases` on the top-level `scope` field described above (heading +windowing identity), but `progress_percent` is governed by a **separate** +`progress_scope` field — the scope of the phase-directory set the percentage +was actually computed from. The two can legitimately disagree (e.g. +`scope: "complete"` — the ROADMAP heading resolves fine — alongside +`progress_scope: "unreadable"` when `.planning/phases` itself cannot be read), +so a consumer must branch on `progress_scope`, not `scope`, to know why +`progress_percent` is `null`. + ### Milestone identity (which milestone, and what it is called) Milestone identity — the version and name behind `STATE.md`'s `milestone:` diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index a047906fd..1f14ee347 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -674,6 +674,8 @@ Show status, next steps, and automatically advance to the next logical workflow Status reporting is scoped to the current milestone's `ROADMAP.md` window and sentinel-filtered: `999.*` backlog directories and `0-*` pre-milestone directories are not counted as current-milestone phases, so the reported progress percentage no longer holds at `100` while phases in the active window are still outstanding. +> **Nullable percentage.** The reported completion percentage is `null` — never a fabricated `0`, `100`, or stale value — when the current milestone's phase set is not fully readable/scoped. See [CLI-TOOLS.md → A non-COMPLETE scope withholds the percentage entirely](CLI-TOOLS.md#a-non-complete-scope-withholds-the-percentage-entirely-3217). + ```bash /gsd-progress # "Where am I? What's next?" with auto-routing /gsd-progress --next # Advance to next step automatically @@ -960,6 +962,8 @@ Display project statistics. Scoped to the current milestone's `ROADMAP.md` window and sentinel-filtered: `999.*` backlog directories and `0-*` pre-milestone directories are not counted as current-milestone phases. +> **Nullable percentage.** The reported completion percentage is `null` — never a fabricated `0`, `100`, or stale value — when the current milestone's phase set is not fully readable/scoped (e.g. a truncated or unresolvable milestone window, or an unreadable `.planning/phases` directory). See [CLI-TOOLS.md → A non-COMPLETE scope withholds the percentage entirely](CLI-TOOLS.md#a-non-complete-scope-withholds-the-percentage-entirely-3217). + ### `/gsd-profile-user` Generate a developer behavioral profile from Claude Code session analysis across 8 dimensions (communication style, decision patterns, debugging approach, UX preferences, vendor choices, frustration triggers, learning style, explanation depth). Produces artifacts that personalize Claude's responses. diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index cd0df6cb3..191ab6755 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -1133,3 +1133,184 @@ the same discipline 8.5 imposes on the rules themselves. **No recorded decision governed this seam.** `recall_decision` returns nothing for the health diagnostic surface — the condition that made Phase 0 necessary, and the reason Decision 8 is locked before Phase 10 rather than settled inside it. + +### Amendment 7 — Phase 7 (#3217) validation: the contract held, on the second pass + +**§7.6 rule 4 is now enforced.** A percentage is withheld (never rendered as a fabricated `0`) at +every consumer this phase reached: `roadmap analyze --json`'s `progress_percent`, `stats --raw`'s +`percent`/`plan_percent`, `query progress --raw`'s `percent`, `state json --raw`'s +`progress.percent`, and `state update-progress --raw`'s write. Rule 2's negative space — a **real** +`0` under a `COMPLETE` scope — is unaffected and still renders (matrix rows B1–B6). + +**This phase was parked once and resumed after an isolated adversarial review, and the review +caught something a same-authorship pass would not have.** The first pass shipped rule 4 at +`computeProgressPercent` (§7.6's own "cheapest site" precedent) and at `cmdRoadmapAnalyze`'s new +`progressScope`-gated accumulation, but left `state.cts::buildStateFrontmatter` hardcoding +`SCOPE.COMPLETE` behind a written-reason comment ("threading it through would mean restructuring +the `_diskScanCache` shared shape, which is out of this phase's named sites"). The review rejected +the comment as a gate — "the whole finding is that a comment is not a gate" — and the restructuring +was done: `_diskScanCache`'s cached shape gained a `phaseDirScope: Scope` field carrying +`listMilestonePhaseDirs`'s real scope, threaded to the `computeProgressPercent` call site in place +of the hardcode. **A second defect surfaced only by tracing the fix through**, not named in the +original finding: the same function's prose fallback (`progressRaw.match(/(\d+)%/)`, reading a +`Progress: N%` line already written to STATE.md's body) fires whenever the scoped call returns +`null`, so a non-`COMPLETE` scope would still have surfaced a stale prose percentage through that +second path even after the hardcode was fixed. That fallback now also gates on +`diskScope === SCOPE.COMPLETE`, matching the pre-existing `milestoneUnbounded` guard already beside +it (#1761). **Generalized lesson, in the same family as Amendment 5's "any numeric window in a drift +guard is a Goodhart target": a rule-4 fix at one output expression is not enough when a fallback +expression sits beside it and shares the same "no data" `null` sentinel — every path that can +produce the observable output must be re-checked, not just the primary one.** + +**A second, independent scope was found ungoverned at `cmdRoadmapAnalyze` and is now exposed, not +reconciled.** `roadmap analyze --json` computes `progress_percent` from its own +`listMilestonePhaseDirs` call (`progressScope`) — correctly, per rule 3 — while `total_plans` / +`total_summaries` / `phases` / `completed_phases` stay derived from the heading-matched +`_phaseDirNames` scan that the top-level `scope` field (windowing identity) describes. Two `Scope` +values governed one JSON object and only one was named. Of the three options the review posed +(expose the second scope; reconcile the two into one; re-derive the phases/counts fields from the +same scoped set as the percentage), this phase **exposes** it as a new `progress_scope` field rather +than reconciling or re-deriving. Reconciling or re-deriving would have undone the phase's own +already-recorded, deliberate choice a few lines above it in the same function — "this does not touch +`total_plans` / `total_summaries` / `phases` / `completed_phases` … only `progress_percent`'s own +inputs move onto the scoped owner" — which exists because `_phaseDirNames` is a heading→directory +**lookup index**, not a milestone enumeration, and filtering it through `listMilestonePhaseDirs` +would scope the same set twice (the same shape Decision 4a's exemption already covers for that +variable). Exposing the field that governs the null is the minimal change that satisfies the +invariant the review named: *a consumer must be able to tell WHY a percentage is absent from the +JSON alone.* + +**A third, minor finding — `state update-progress`'s silent no-op — is resolved as accept-with- +disclosure, not silence.** The command already left `STATE.md` untouched on a non-`COMPLETE` scope; +the gap was that the only signal was a JSON `reason` field most callers do not read. It now also +writes a `[gsd-tools] WARNING:` line to stderr, matching the convention this same file already uses +for a comparable silent field-update no-op (`stateReplaceFieldWithFallback`, `state.cts:553`) rather +than inventing a second warning shape. + +**The guard question was re-litigated, not skipped.** Issue #3217 asked for +`lint-completion-ratio-drift.cjs` to be extended to "catch a percentage rendered from counts whose +scope was not consulted." A narrow, syntactic candidate — a call to `listMilestonePhaseDirs(...)` +whose `.value` is read while `.scope` is never bound in the same function — was prototyped against +the real tree and produced real false positives: `src/milestone.cts:697,753,881` legitimately read +only `.value` from that call for milestone **archiving**, unrelated to percentage rendering at all. +"Was this scope consulted before rendering a percentage" is a data-flow question, not a syntactic +one, and no syntactic proxy distinguishes those two call shapes. Per Amendment 4a's standing rule — +build and run before scope is fixed, state what the guard actually found — the candidate was dropped +rather than shipped with an exemption list that would have hollowed it out on the sites it exists to +catch. **The existing arithmetic guard (rules 1–2) is unchanged and still reports an earned `0`** on +the real tree (test row E3); the real enforcement for rule 4 is the behavioral identity-at-the- +surface suite this phase adds (`tests/completion-ratio-scope-withholding.test.cjs`, matrix rows +A1–A11, B1–B6, C1–C3, D1–D4, E1–E4), per Decision 4(b)/(c) — asserted at each consumer's observable +output, never at a helper's return value, so a percentage post-filtered after the fact cannot pass +as one never withheld. + +**What this phase does NOT close, left with a written reason rather than silently dropped:** + +- **Workstream inventory (matrix row A8).** `buildWorkstreamInventory` + (`src/workstream-inventory-builder.cts`) carries a pre-ADR-3180 bespoke boolean + (`milestoneScoped`), not the frozen `SCOPE` enum, and cannot distinguish `TRUNCATED` from + `UNSCOPED` from `UNREADABLE`. Migrating it honestly requires widening + `WorkstreamInventory.progress_percent` from `number` to `number | null` — a return-type + re-architecture the design doc names as out of scope for this phase — or reusing the bespoke + boolean as a `Scope` stand-in, which is the same textual-proxy-for-a-data-flow-property this + amendment's guard section rejects one paragraph up. Left un-migrated with the reason recorded at + the field's own definition site. +- **`cmdStateSync`'s own `SCOPE.COMPLETE` hardcode (`state.cts`, `cmdStateSync`), found but not + named in the original review.** This is a *third* site sharing buildStateFrontmatter's pattern — + a raw `fs.readdirSync(phasesDir, ...)` listing, never routed through `listMilestonePhaseDirs`, + carries its own written-reason comment for staying on `SCOPE.COMPLETE`. It was not named as a + finding and the design's consumer map does not list `cmdStateSync` among the sites this phase + owns. It is lower-risk than the fixed sites: an unreadable `phasesDir` (this finding's own + reproduction shape) already hits `cmdStateSync`'s own `fs.readdirSync` `catch` block, which exits + via the pre-existing `{ synced: true, changes: [], dry_run }` early return **before** any percent + is computed — so the specific defect this phase closes elsewhere does not reproduce here. Recorded + rather than silently left for a reader to independently rediscover; migrating it fully (adding rule + 3's window-scoping, not just rule 4's withholding) is out of this phase's named scope. + +**Rebase note.** This phase's implementation predates Phase 4 (#3186, the disk-strict completion +predicate) landing on `next`. Rebasing onto `next` after Phase 4 merged produced **no conflicts**, +though Phase 4 turned out to touch two of the same functions this phase's fix reaches — +`buildStateFrontmatter` and `cmdStateSync` — swapping each one's `diskCompletedPhases` count from +`scanPhasePlans(...).completed` (answers "are all plans summarized", a different question, per +§7.4/#2957) to the canonical `isPhaseComplete(...).value.complete`. Those hunks land in the +phase-directory completion-counting loop in each function; this phase's own hunks land at the +`listMilestonePhaseDirs` destructure, the `_diskScanCache` shape, and the `computeProgressPercent` +call site further down the same functions — non-overlapping line ranges, which is why the automatic +merge succeeded with no manual resolution. + +**Tier-2, re-derived for Phase 7 (mirrors Amendment 5's per-phase table convention):** + +| Command surface | Output change | +|---|---| +| `roadmap analyze --json` | `progress_percent` becomes `number \| null`; gains a new `progress_scope` field naming the scope that governs it, separate from the top-level `scope` | +| `stats --raw` | `percent` and `plan_percent` become `number \| null` | +| `query progress --raw` | `percent` becomes `number \| null` | +| `state json --raw` | `progress.percent` is omitted (not `0`, not `null`) rather than rendered when the underlying scope is not `COMPLETE` | +| `state update-progress --raw` | unchanged wire shape (`updated: false` was already the non-write signal); gains a `[gsd-tools] WARNING:` stderr line on skip | + +**Guard roster (§7.6 row), as amended.** The original body's row is not edited in place, per +Amendment 6's own precedent; read with this correction: `lint-completion-ratio-drift.cjs`'s scan +surface and mechanism are **unchanged** (arithmetic rules 1–2 only, `src/` scan surface) — rule 4 is +enforced by the behavioral suite named above, not by an extension to this guard. §7.6's status line +("rule 4 Required — Phase 7") and the Status bullet "Rule 4 — Phase 7 (#3217)… is not implemented +anywhere" are superseded by this amendment: rule 4 is enforced at every site Phase 7 named, with the +two written exceptions above. + +### Amendment 8 — a BLOCKER correction to Amendment 7's `cmdStateSync` claim: TRUNCATED and UNSCOPED row 4 DID reproduce + +**Amendment 7's second "what this phase does NOT close" bullet is wrong and is corrected here, not +silently rewritten** — per Amendment 6's own append-only precedent, that bullet's body is left +untouched above; this amendment records what independent, empirical reproduction on fresh fixture +copies actually found. + +Amendment 7 claimed `cmdStateSync`'s hardcoded `SCOPE.COMPLETE` was "lower-risk than the fixed +sites" because "an unreadable `phasesDir` (this finding's own reproduction shape) already hits +`cmdStateSync`'s own `fs.readdirSync` `catch` block… so the specific defect this phase closes +elsewhere does not reproduce here." That checked only the `UNREADABLE` row. It did not check +`TRUNCATED` or `UNSCOPED`, and on those rows the claim is false: `cmdStateSync`'s disk scan +(`entries`, `state.cts` ~3113) enumerates `phasesDir` successfully — it is readable — but the scan is +**unfiltered by the real milestone window** (it only excludes retired phase numbers, #1514), and the +percent gate hardcoded `SCOPE.COMPLETE` regardless of what the real window's scope was. Reproduced on +fresh, independent fixture copies (`listMilestonePhaseDirs` called directly to confirm the real +scope, matching each surface's own derivation): + +| fixture | real scope | `state sync` (pre-fix) | +|---|---|---| +| TRUNCATED (milestone heading found, window empty, document has phases elsewhere — row 8) | `truncated` | **wrote a fabricated `0%` → `100%`** | +| UNSCOPED row 4 (no milestone asserted, ROADMAP has versioned headings) | `unscoped` | **wrote a fabricated `0%` → `100%`** | +| UNSCOPED row 5 (asserted version, no matching heading) | `unscoped` | skipped — but only because the orthogonal `milestoneBounded` (#1761) guard happens to intercept this specific row first, not because `cmdStateSync` itself was scope-aware | + +Worse than a wrong read: this is a **write** path, and on the TRUNCATED fixture it persisted a +self-contradictory `STATE.md` in one write — the body's `Progress:` line got the fabricated `100%` +while the frontmatter's `progress:` block, built in the same `writeStateMd` call by the already-fixed +`buildStateFrontmatter`, correctly omitted `percent`. That cross-surface disagreement inside a single +file is the exact defect class this epic exists to remove. + +**Fixed the same way as the two sites Amendment 7 named**: `cmdStateSync` now calls +`listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: versionStr })` — reusing the same +`syncRoadmapRaw`/`syncRoadmapScope` already parsed in the function for the disk-scan totals — and +withholds (pushing a `Progress: skipped — …(#3217)` entry to `changes`, mirroring the `#1761` +skip-message convention already at that call site) whenever the real scope is not `COMPLETE`. The +`milestoneBounded` (#1761) guard is unchanged and still fires first on row 5; a genuine `0` under a +real `COMPLETE` scope still writes. Re-reproduced post-fix: all three rows above now agree with +`state json` / `roadmap analyze` / `stats` / `query progress` on the same fixture — all withhold +together, none renders a number, and the control (`COMPLETE`) fixture still writes its earned +percentage. + +**Generalized lesson — the second instance of this pattern in this epic (see Amendment 5's own +"Goodhart target" lesson for the first).** A "does not reproduce" claim was too generous a second +time: it checked the row named in the ORIGINAL finding (`UNREADABLE`) and stopped, rather than +checking every row the same code path could plausibly hit. A written-reason comment recording "I +checked X and it's fine" is not evidence about Y and Z unless Y and Z were checked too — the same +"a comment is not a gate" principle Amendment 7 itself invoked against `buildStateFrontmatter`'s +original hardcode applies equally to a comment that scopes its own non-reproduction claim too +narrowly. + +**Tier-2, additional (mirrors Amendment 7's own table):** + +| Command surface | Output change | +|---|---| +| `state sync --raw` | on a non-`COMPLETE` scope: no `Progress:` body rewrite; `changes` gains a `Progress: skipped — …(#3217)` entry instead. A genuine `0` under `COMPLETE` is unaffected. | + +`.changeset/bold-otters-scope.md` is updated to disclose this write-path change alongside the two +Amendment 7 already recorded. diff --git a/src/commands.cts b/src/commands.cts index 1d26ce4c4..d3a590a36 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1774,15 +1774,25 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean } } catch { /* intentionally empty */ } - const percent = clampPercent(totalSummaries, totalPlans); + // #3217 (ADR-3180 §7.6 rule 4): `phaseScope` was already computed above + // (Phase 3, #3222) but never consulted before rendering — a percentage was + // rendered from counts the scope said were not answers (TRUNCATED / + // UNSCOPED / UNREADABLE). Withhold the percentage itself (never `0` — a + // real `0` under COMPLETE must still render, rule 2's territory) when the + // scope is not COMPLETE. `phaseScope` stays `null` only if the try block + // above threw before assigning it; treat that the same as non-COMPLETE. + const percent: number | null = phaseScope === SCOPE.COMPLETE + ? clampPercent(totalSummaries, totalPlans) + : null; if (format === 'table') { // Render markdown table const barWidth = 10; - const filled = Math.round((percent / 100) * barWidth); + const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); + const percentSuffix = percent === null ? '' : ` (${percent}%)`; let out = `# ${milestone?.version ?? ''} ${milestone?.name ?? ''}\n\n`; - out += `**Progress:** [${bar}] ${totalSummaries}/${totalPlans} plans (${percent}%)\n\n`; + out += `**Progress:** [${bar}] ${totalSummaries}/${totalPlans} plans${percentSuffix}\n\n`; out += `| Phase | Name | Plans | Status |\n`; out += `|-------|------|-------|--------|\n`; for (const p of phases) { @@ -1791,9 +1801,10 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean output({ rendered: out }, raw, out); } else if (format === 'bar') { const barWidth = 20; - const filled = Math.round((percent / 100) * barWidth); + const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); - const text = `[${bar}] ${totalSummaries}/${totalPlans} plans (${percent}%)`; + const percentSuffix = percent === null ? '' : ` (${percent}%)`; + const text = `[${bar}] ${totalSummaries}/${totalPlans} plans${percentSuffix}`; output({ bar: text, percent, completed: totalSummaries, total: totalPlans }, raw, text); } else { // JSON format @@ -2130,8 +2141,12 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { const phases = [...phasesByNumber.values()].sort((a, b) => comparePhaseNum(a.number, b.number)); const completedPhases = phases.filter(p => p.status === 'Complete').length; - const planPercent = clampPercent(totalSummaries, totalPlans); - const percent = clampPercent(completedPhases, phases.length); + // #3217 (ADR-3180 §7.6 rule 4): both percentages here are derived from the + // same `phaseScope`-carrying directory enumeration above (Phase 3, #3222) — + // withhold both when that scope is not COMPLETE, same rationale as + // cmdProgressRender above. A real `0` under COMPLETE still renders. + const planPercent: number | null = phaseScope === SCOPE.COMPLETE ? clampPercent(totalSummaries, totalPlans) : null; + const percent: number | null = phaseScope === SCOPE.COMPLETE ? clampPercent(completedPhases, phases.length) : null; // Requirements stats let requirementsTotal = 0; @@ -2193,11 +2208,12 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { if (format === 'table') { const barWidth = 10; - const filled = Math.round((percent / 100) * barWidth); + const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); let out = `# ${milestone?.version ?? ''} ${milestone?.name ?? ''} — Statistics\n\n`; - out += `**Progress:** [${bar}] ${completedPhases}/${phases.length} phases (${percent}%)\n`; - if (totalPlans > 0) { + const percentSuffix = percent === null ? '' : ` (${percent}%)`; + out += `**Progress:** [${bar}] ${completedPhases}/${phases.length} phases${percentSuffix}\n`; + if (totalPlans > 0 && planPercent !== null) { out += `**Plans:** ${totalSummaries}/${totalPlans} complete (${planPercent}%)\n`; } out += `**Phases:** ${completedPhases}/${phases.length} complete\n`; diff --git a/src/roadmap.cts b/src/roadmap.cts index eabc37740..6b6bc869a 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -17,7 +17,11 @@ import phaseIdMod = require('./phase-id.cjs'); const { escapeRegex, normalizePhaseName, phaseMarkdownRegexSource, phaseTokenMatches, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, isSentinelPhaseId } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); -const { findPhaseInternal } = phaseLocatorMod; +const { findPhaseInternal, listMilestonePhaseDirs } = phaseLocatorMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserModule = require('./roadmap-parser.cjs'); const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings } = roadmapParserModule; @@ -493,6 +497,38 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { const detailPhases = new Set(phases.map(p => p.number)); const missingDetails = [...checklistPhases].filter(p => !detailPhases.has(p) && !isSentinelPhaseId(p)); + // #3217 (ADR-3180 §7.6 rules 3-4): `progress_percent` used to accumulate + // `totalPlans`/`totalSummaries` above — a heading-matched enumeration + // (`phasePattern` over the milestone-windowed `content`) paired against + // `_phaseDirNames`, a DELIBERATELY unscoped physical directory listing + // (see its own comment above: it is a heading->directory lookup index, + // not a milestone enumeration). That set is not the same set + // `listMilestonePhaseDirs` scopes for `query progress` / `stats` (#3185 + // Phase 3), so `progress_percent` could silently diverge from both siblings + // on the same project (rule 3). Route `progress_percent`'s own + // numerator/denominator through the single scoped owner instead — mirrors + // cmdProgressRender/cmdStats's own aggregation — and withhold the + // percentage entirely when THAT scope is not COMPLETE (rule 4), never + // returning `0` for "could not compute". This does not touch `total_plans` + // / `total_summaries` / `phases` / `completed_phases` above — those stay + // the heading-matched detail view; only `progress_percent`'s own inputs + // move onto the scoped owner. + let scopedTotalPlans = 0; + let scopedTotalSummaries = 0; + let progressScope: Scope = SCOPE.UNREADABLE; + try { + const { value: progressDirs, scope: scopedResult } = listMilestonePhaseDirs(phasesDir, { cwd }); + progressScope = scopedResult; + for (const dir of progressDirs) { + const scan = scanPhasePlans(path.join(phasesDir, dir)); + scopedTotalPlans += scan.planCount; + scopedTotalSummaries += scan.summaryCount; + } + } catch { /* progressScope stays the pessimistic SCOPE.UNREADABLE default */ } + const progressPercent = progressScope === SCOPE.COMPLETE + ? clampPercent(scopedTotalSummaries, scopedTotalPlans) + : null; + const result = { milestones, phases, @@ -500,7 +536,24 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { completed_phases: completedPhases, total_plans: totalPlans, total_summaries: totalSummaries, - progress_percent: clampPercent(totalSummaries, totalPlans), + progress_percent: progressPercent, + // #3217 finding 2: `progress_percent` is gated by a SECOND, independently + // computed `listMilestonePhaseDirs` scope (`progressScope` above) — not + // by the top-level `scope` field, which describes the heading-windowing + // identity `phases`/`total_plans`/`total_summaries`/`completed_phases` + // were built from. Those two scopes can legitimately disagree (e.g. + // `scope: "complete"` alongside a genuinely unreadable phases directory), + // and per the documented contract "scope tells you whether the counts + // are trustworthy", a consumer seeing `progress_percent: null` needs a + // field to tell WHY without reading source. Exposing `progress_scope` + // (rather than reconciling the two scopes into one, or re-deriving + // `total_plans`/`phases`/etc. from the scoped set) preserves the + // deliberate, already-documented choice a few lines up: `phases`/ + // `total_plans`/`total_summaries`/`completed_phases` stay the + // heading-matched detail view (`_phaseDirNames` is a lookup index, not a + // milestone enumeration — see its comment); only `progress_percent`'s own + // inputs move onto the scoped owner. + progress_scope: progressScope, current_phase: currentPhase ? currentPhase.number : null, next_phase: nextPhase ? nextPhase.number : null, missing_phase_details: missingDetails.length > 0 ? missingDetails : null, diff --git a/src/state-document.cts b/src/state-document.cts index 9be0633f6..192c89c97 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -404,12 +404,25 @@ export function normalizeStateStatus(status: string | null | undefined, pausedAt return normalizedStatus; } +/** + * ADR-3180 §7.6 rule 4 (#3217): `scope` is the `listMilestonePhaseDirs`-owner + * discriminator for the phase/plan set these four counts were derived from. + * A caller that cannot vouch for `scope === SCOPE.COMPLETE` must pass the + * scope it actually has — this function refuses to compose a percentage + * from counts whose scope says they are not a trustworthy answer, returning + * `null` (never `0`; see the module's already-existing "no data" `null` + * below, which this generalizes) exactly like its pre-existing "no data" + * case. `scope` is REQUIRED (no default) so a caller cannot silently opt out + * of rule 4 by omission. + */ export function computeProgressPercent( completedPlans: number | null, totalPlans: number | null, completedPhases: number | null, - totalPhases: number | null + totalPhases: number | null, + scope: Scope ): number | null { + if (scope !== SCOPE.COMPLETE) return null; const hasPlanData = totalPlans !== null && totalPlans > 0 && completedPlans !== null; const hasPhaseData = totalPhases !== null && totalPhases > 0 && completedPhases !== null; if (!hasPlanData && !hasPhaseData) diff --git a/src/state.cts b/src/state.cts index 17564eabe..c9e4b8fd9 100644 --- a/src/state.cts +++ b/src/state.cts @@ -36,6 +36,7 @@ const { isPhaseComplete } = verificationMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); const { listMilestonePhaseDirs } = phaseLocatorMod; @@ -177,6 +178,12 @@ const _diskScanCache = new Map(); // Track all lock files held by this process so they can be removed on exit. @@ -756,6 +763,7 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { const phasesDir = planningPaths(cwd).phases; let totalPlans = 0; let totalSummaries = 0; + let phaseScope: Scope = SCOPE.UNREADABLE; { // #3185 (ADR-3180 Decision 1): "which phase directories belong to the @@ -764,7 +772,8 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { // excluded sentinels, unlike the owner). The owner already handles an // absent phasesDir as a real empty, so the fs.existsSync guard folds // into it. - const phaseDirs = listMilestonePhaseDirs(phasesDir, { cwd }).value; + const { value: phaseDirs, scope } = listMilestonePhaseDirs(phasesDir, { cwd }); + phaseScope = scope; for (const dir of phaseDirs) { const { planCount, summaryCount } = scanPhasePlans(path.join(phasesDir, dir)); totalPlans += planCount; @@ -772,6 +781,25 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void { } } + // #3217 (ADR-3180 §7.6 rule 4): a non-COMPLETE scope means the counts + // above are not a trustworthy answer — do not write a percentage derived + // from them into STATE.md at all (A7). This is the write path, so + // "withhold" means "make no edit" rather than emitting a null value. + if (phaseScope !== SCOPE.COMPLETE) { + // #3217 finding 3 (decided: surface a warning, not silent-only + // disclosure): the JSON `reason` field alone is easy for a caller to + // never read, and STATE.md's Progress field goes stale with no + // user-visible signal beyond it. Mirrors the established + // `[gsd-tools] WARNING:` stderr convention this file already uses + // (stateReplaceFieldWithFallback above) for a comparable silent no-op. + process.stderr.write( + `[gsd-tools] WARNING: state update-progress skipped — phase scope is ${phaseScope}, not complete. ` + + `STATE.md's Progress field was left unchanged.\n` + ); + output({ updated: false, reason: `phase scope is ${phaseScope}, not complete` }, raw, 'false'); + return; + } + const percent = clampPercent(totalSummaries, totalPlans); const barWidth = 10; const filled = Math.round(percent / 100 * barWidth); @@ -1706,6 +1734,14 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // #1761 read-path: set from cached.milestoneBounded inside the disk-scan // block; consumed at the percent computation to mirror the cmdStateSync guard. let milestoneUnbounded = false; + // #3217 (ADR-3180 §7.6 rule 4, finding 1): the real listMilestonePhaseDirs + // scope for the disk-scanned counts below, set from cached.phaseDirScope + // when a fresh disk scan runs. SCOPE.COMPLETE is the correct default here + // — NOT a rule-4 hardcode — for the cases where no disk scan happens at all + // (no cwd, or phasesDir absent): totalPhases/totalPlans then come straight + // from the pre-existing frontmatter fields parsed above, a path this phase + // does not touch and which predates listMilestonePhaseDirs entirely. + let diskScope: Scope = SCOPE.COMPLETE; if (cwd) { try { @@ -1738,7 +1774,7 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // CURRENT (stored) milestone" — routed through the canonical owner // instead of a hand-rolled readdirSync + isDirInMilestone filter // (which also never excluded sentinels, unlike the owner). - const allMatchingDirs = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: storedMilestone ?? null }).value; + const { value: allMatchingDirs, scope: phaseDirScope } = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: storedMilestone ?? null }); // Bug #2445: when stale phase dirs from a prior milestone remain in // .planning/phases/ alongside new dirs with the same phase number, @@ -1865,6 +1901,7 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto completedPhases: diskCompletedPhases, totalPlans: diskTotalPlans, completedPlans: diskTotalSummaries, + phaseDirScope, }; })(); _diskScanCache.set(cwd, cached); @@ -1874,6 +1911,7 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto totalPlans = cached.totalPlans; completedPlans = cached.completedPlans; milestoneUnbounded = cached.milestoneBounded === false; + diskScope = cached.phaseDirScope; } /* best-effort (#2245 audit): this is a READ path building STATE.md's * display frontmatter. The real throw source is fs.readdirSync(phasesDir) @@ -1890,11 +1928,30 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // ROADMAP-declared-but-unrealized future phases cap the reported completion // instead of a false 100% from plan-only coverage (#3242 Bug B). // Falls back to the body Progress: field only when no plan files exist on disk. - let progressPercent = computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases); + // #3217 (ADR-3180 §7.6 rule 4, finding 1): computeProgressPercent requires + // a `Scope` for its own rule-4 gate. `diskScope` is the real + // `listMilestonePhaseDirs` scope threaded through `_diskScanCache` + // (`phaseDirScope` above) when a fresh disk scan ran — an UNREADABLE + // phases dir now withholds here exactly as it does at every sibling + // surface, closing the cross-surface disagreement the isolated review + // caught. When no disk scan ran at all (no cwd, or phasesDir absent) + // `diskScope` keeps its SCOPE.COMPLETE default, preserving this + // function's pre-existing behavior on that (unrelated, pre-dating + // listMilestonePhaseDirs) fallback path. This call site also keeps its own + // orthogonal `milestoneUnbounded` null-out below (#1761) — a different + // guard (ROADMAP heading boundedness, not disk readability). + let progressPercent = computeProgressPercent(completedPlans, totalPlans, completedPhases, totalPhases, diskScope); // #1761 read-path: when the milestone can't be bounded, percent would be // derived from a conflated/understated total — skip it (mirror cmdStateSync). if (milestoneUnbounded) progressPercent = null; - if (progressPercent === null && progressRaw && !milestoneUnbounded) { + // #3217 finding 1 (follow-on): a non-COMPLETE diskScope must withhold the + // percentage EVERYWHERE, including this prose fallback — without the + // `diskScope === SCOPE.COMPLETE` guard, a stale/existing "Progress: N%" + // body line would silently defeat computeProgressPercent's rule-4 null, + // re-introducing a rendered percentage on the exact scope this phase + // withholds for (this is how the reviewer's UNREADABLE-phases fixture + // could still surface a number even after the scope threading above). + if (progressPercent === null && progressRaw && !milestoneUnbounded && diskScope === SCOPE.COMPLETE) { const pctMatch = progressRaw.match(/(\d+)%/); if (pctMatch) progressPercent = parseInt(pctMatch[1], 10); } @@ -3164,8 +3221,24 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b if (!milestoneBounded) { changes.push(`Progress: skipped — milestone ${versionStr} cannot be bounded to a versioned ROADMAP phase set (#1761)`); } else { - const p = computeProgressPercent(totalDiskSummaries, totalDiskPlans, diskCompletedPhases, syncTotalPhases); - percent = p !== null ? p : 0; + // #3217 (ADR-3180 §7.6 rule 4) BLOCKER fix: the prior comment here claimed + // `entries` (the raw fs.readdirSync listing above) was "never routed + // through listMilestonePhaseDirs, so there is no real Scope to pass" — + // that was factually wrong. The same `syncRoadmapRaw`/`syncRoadmapScope` + // already parsed above (~3104) is precisely what + // `listMilestonePhaseDirs` (via `getMilestonePhaseFilter`) re-derives + // from `cwd` to produce a real `Scope` — the identical shape already + // threaded through `buildStateFrontmatter`'s `diskScope` above. Calling + // it here (discarding `.value`, which duplicates `entries`'s own + // retired-phase-filtered listing) gets the real scope without changing + // the disk-scan totals computed above. + const syncScope: Scope = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: versionStr }).scope; + if (syncScope !== SCOPE.COMPLETE) { + changes.push(`Progress: skipped — milestone phase scope is "${syncScope}", not COMPLETE (#3217)`); + } else { + const p = computeProgressPercent(totalDiskSummaries, totalDiskPlans, diskCompletedPhases, syncTotalPhases, syncScope); + percent = p !== null ? p : 0; + } } const syncResult = transitionCore( diff --git a/src/workstream-inventory-builder.cts b/src/workstream-inventory-builder.cts index 6cefc46c3..92e185d8a 100644 --- a/src/workstream-inventory-builder.cts +++ b/src/workstream-inventory-builder.cts @@ -439,6 +439,27 @@ export function buildWorkstreamInventory(inputs: BuildWorkstreamInventoryInputs) // invariant above throws first) and matters only for the legacy unscoped // path, where the denominator is a roadmap heading count that a caller // cannot guarantee bounds the numerator. + // + // #3217 (ADR-3180 §7.6 rule 4) — WRITTEN REASON this site is NOT migrated + // onto the `SCOPE` enum this phase: `buildWorkstreamInventory` is a pure + // projection (no I/O — see the module header) fed `BuildWorkstreamInventoryInputs` + // by `workstream-inventory.cts`. Its own `milestoneScoped` is a pre-ADR-3180 + // bespoke boolean, not a `SCOPE` value, and its caller does not currently + // thread a real `listMilestonePhaseDirs` scope into these inputs. Doing + // this honestly requires ONE of: (a) widening `BuildWorkstreamInventoryInputs` + // with a `Scope` field and `WorkstreamInventory.progress_percent`'s type + // from `number` to `number | null` — the exact "re-architecting + // StateProjection/WorkstreamInventory return types" the design phase + // (`.gsd/phase/refactor-3217-completion-ratio-scoping/40-design.md`, + // "Known limits") states is OUT of this phase's scope; or (b) silently + // reusing `milestoneScoped` as a `Scope` stand-in, which would be exactly + // the kind of proxy-for-a-data-flow-property this same phase's guard + // section explicitly rejects (a `boolean` cannot distinguish TRUNCATED + // from UNSCOPED from UNREADABLE, so a caller could not tell which + // non-answer it got). Left un-migrated rather than done dishonestly; + // `workstream inventory`'s `progress_percent` can still render a number + // derived from an under-scoped set (A8 in the phase's test matrix is + // NOT covered here for that reason — see this phase's PR description). progress_percent: clampPercent(completedPhases, effectivePhaseCount), }; } diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 01df5a76e..aa3a150fc 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -523,9 +523,12 @@ describe('progress command', () => { }); test('renders JSON progress', () => { + // #3217: no version token — genuinely free-form, so windowing scope is + // COMPLETE (§7.1) rather than UNSCOPED (a title merely mentioning "v1.0" + // with no STATE.md milestone pointer cannot be windowed to that version). fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), - `# Roadmap v1.0 MVP\n` + `# Roadmap MVP\n` ); const p1 = path.join(tmpDir, '.planning', 'phases', '01-foundation'); fs.mkdirSync(p1, { recursive: true }); @@ -545,9 +548,10 @@ describe('progress command', () => { }); test('renders bar format', () => { + // #3217: no version token — see 'renders JSON progress' above. fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), - `# Roadmap v1.0\n` + `# Roadmap\n` ); const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); fs.mkdirSync(p1, { recursive: true }); @@ -576,9 +580,10 @@ describe('progress command', () => { }); test('does not crash when summaries exceed plans (orphaned SUMMARY.md)', () => { + // #3217: no version token — see 'renders JSON progress' above. fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), - `# Roadmap v1.0 MVP\n` + `# Roadmap MVP\n` ); const p1 = path.join(tmpDir, '.planning', 'phases', '01-foundation'); fs.mkdirSync(p1, { recursive: true }); @@ -2005,6 +2010,12 @@ describe('stats command', () => { beforeEach(() => { tmpDir = createTempProject(); + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token + // anywhere) is COMPLETE scope for windowing (§7.1) — without this, an + // absent ROADMAP.md is UNREADABLE and stats withholds `percent`/counts. + // Individual tests below that write their own ROADMAP.md content + // overwrite this baseline. + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); }); afterEach(() => { @@ -2114,6 +2125,11 @@ describe('stats command', () => { fs.writeFileSync(path.join(p2, '15-01-SUMMARY.md'), '# Summary'); fs.writeFileSync(path.join(p2, 'VERIFICATION.md'), '---\nstatus: passed\n---\n# Verified'); + // #3217 (ADR-3180 §7.6 rule 4): no `vX.Y` token in the milestone heading + // — the ROADMAP has no STATE.md milestone pointer, so a real version + // token here would resolve to UNSCOPED (§7.1 row 4: "has versioned + // milestones, but no version resolved"), not the free-form COMPLETE + // window this test's counting assertions depend on. fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), `# Roadmap @@ -2122,7 +2138,7 @@ describe('stats command', () => { - [x] **Phase 15: Proof Generation** - [ ] **Phase 16: Multi-Claim Verification & UX** -## Milestone v1.0 Growth +## Milestone Growth ### Phase 14: Auth Hardening **Goal:** Improve auth checks diff --git a/tests/completion-ratio-scope-withholding.test.cjs b/tests/completion-ratio-scope-withholding.test.cjs new file mode 100644 index 000000000..6c3741640 --- /dev/null +++ b/tests/completion-ratio-scope-withholding.test.cjs @@ -0,0 +1,826 @@ +/** + * Tests for Phase 7 (#3217, ADR-3180 §7.6 rules 3-4): a derivation whose + * scope is not COMPLETE renders no percentage at all. + * + * Design: .gsd/phase/refactor-3217-completion-ratio-scoping/40-design.md + * Test matrix: .gsd/phase/refactor-3217-completion-ratio-scoping/50-test-matrix.md + * + * Section A asserts at each CONSUMER's observable output (ADR Decision 4c), + * never at a helper's return value — a percentage post-filtered after the + * fact is indistinguishable from one never withheld. All CLI assertions use + * `--raw` (typed JSON), never rendered prose (CONTRIBUTING.md: no source-grep, + * no prose matching). + * + * Fixture note on constructing each SCOPE without fs mocking (a subprocess + * CLI test cannot `mock.method` inside the child process): COMPLETE and + * TRUNCATED and UNSCOPED are constructed via ROADMAP/STATE content shape + * alone. UNREADABLE is constructed by making `.planning/phases` a REGULAR + * FILE instead of a directory — `listMilestonePhaseDirs`'s own + * `fs.readdirSync(phasesDir, ...)` then throws ENOTDIR and its catch block + * returns `scope: SCOPE.UNREADABLE` (src/phase-locator.cts). This is + * deterministic and cross-platform, unlike a `chmod`-based trick (repo rule: + * IO failure via `mock.method`, never `chmod 0o000` — this sidesteps both by + * not needing IO-failure injection into the CLI's own process at all). + */ + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempDir, cleanup, runGsdTools } = require('./helpers.cjs'); +const drift = require('../scripts/lint-completion-ratio-drift.cjs'); +const { clampPercent } = require('../gsd-core/bin/lib/phase-lifecycle.cjs'); +const { computeProgressPercent } = require('../gsd-core/bin/lib/state-document.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); + +// ─── Fixture helpers (mirrors tests/completion-ratio-single-owner.test.cjs) ─ + +function planningDirOf(cwd) { + return path.join(cwd, '.planning'); +} + +function writeRoadmap(cwd, content) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + fs.writeFileSync(path.join(planningDirOf(cwd), 'ROADMAP.md'), content); +} + +function writeState(cwd, fields) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + const lines = ['---']; + for (const [k, v] of Object.entries(fields)) lines.push(`${k}: ${v}`); + lines.push('---', ''); + fs.writeFileSync(path.join(planningDirOf(cwd), 'STATE.md'), lines.join('\n')); +} + +function writeFile(cwd, relPath, content) { + const full = path.join(cwd, relPath); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); +} + +// Makes `.planning/phases` an UNREADABLE-as-a-directory node: a regular file +// where callers expect a directory. `fs.readdirSync` on it throws ENOTDIR, +// which `listMilestonePhaseDirs` (src/phase-locator.cts) catches and reports +// as `scope: SCOPE.UNREADABLE` — the real production catch path, not a mock. +function makePhasesDirUnreadable(cwd) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + fs.writeFileSync(path.join(planningDirOf(cwd), 'phases'), 'not a directory\n'); +} + +// COMPLETE-scope fixture: v1.0 is the current (and only) milestone, one +// Complete phase and one Planned phase. +function buildCompleteFixture(cwd) { + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); + writeFile(cwd, '.planning/phases/02-bar/02-01-PLAN.md', '# Plan\n'); +} + +// TRUNCATED-scope fixture: STATE asserts v2.0, whose ROADMAP heading is +// found but whose OWN window has no Phase entries, while the document (under +// v1.0) does — classifyMilestoneWindow row 8 (src/roadmap-parser.cts). +function buildTruncatedFixture(cwd) { + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, [ + '## v1.0 Planned', + '', + '### Phase 1: Foo', + '', + '## v2.0 Current 🚧', + ].join('\n')); +} + +// UNSCOPED-scope fixture: STATE asserts a version with no matching ROADMAP +// heading at all (classifyMilestoneWindow row 5). +function buildUnscopedFixture(cwd) { + writeState(cwd, { milestone: 'v9.9' }); + writeRoadmap(cwd, [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + ].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); +} + +// UNREADABLE-scope fixture: a valid milestone window, but the phases +// directory itself cannot be enumerated. +function buildUnreadableFixture(cwd) { + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + ].join('\n')); + makePhasesDirUnreadable(cwd); +} + +function analyzeRaw(cwd) { + const result = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return JSON.parse(result.output); +} + +function queryProgressRaw(cwd) { + const result = runGsdTools(['query', 'progress', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return JSON.parse(result.output); +} + +function statsRaw(cwd) { + const result = runGsdTools(['stats', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return JSON.parse(result.output); +} + +function stateUpdateProgressRaw(cwd) { + const result = runGsdTools(['state', 'update-progress', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return result.output; +} + +function stateJsonRaw(cwd) { + const result = runGsdTools(['state', 'json', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return JSON.parse(result.output); +} + +// ═════════════════════════════════════════════════════════════════════════ +// A. Rule 4 at each consumer's OBSERVABLE output (ADR Decision 4c) +// ═════════════════════════════════════════════════════════════════════════ + +describe('A. rule 4 — a non-COMPLETE scope renders no percentage, at the CLI surface', () => { + test('A1: roadmap analyze --json, COMPLETE scope -> numeric progress_percent', (t) => { + const cwd = createTempDir('gsd-3217-a1-'); + t.after(() => cleanup(cwd)); + buildCompleteFixture(cwd); + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(typeof analyzed.progress_percent, 'number'); + assert.strictEqual(analyzed.progress_percent, 50); + // Finding 2: `progress_scope` is the field that governs `progress_percent` + // specifically — it must be exposed (not merely equal to `scope` by + // coincidence on this fixture) so a consumer never has to infer it. + assert.strictEqual(analyzed.progress_scope, SCOPE.COMPLETE); + }); + + test('A2: roadmap analyze --json, TRUNCATED scope -> progress_percent: null (never 0, never 100)', (t) => { + const cwd = createTempDir('gsd-3217-a2-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.TRUNCATED); + assert.strictEqual(analyzed.progress_percent, null); + assert.notStrictEqual(analyzed.progress_percent, 0); + assert.notStrictEqual(analyzed.progress_percent, 100); + }); + + test('A3: roadmap analyze --json, UNSCOPED scope -> progress_percent: null', (t) => { + const cwd = createTempDir('gsd-3217-a3-'); + t.after(() => cleanup(cwd)); + buildUnscopedFixture(cwd); + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.UNSCOPED); + assert.strictEqual(analyzed.progress_percent, null); + }); + + test('A4: roadmap analyze --json, UNREADABLE phases dir -> progress_percent: null', (t) => { + const cwd = createTempDir('gsd-3217-a4-'); + t.after(() => cleanup(cwd)); + buildUnreadableFixture(cwd); + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.progress_percent, null); + // Finding 2's exact repro shape: the top-level `scope` (heading-windowing + // identity) is COMPLETE — the ROADMAP heading resolves fine — while + // `progress_scope` (the listMilestonePhaseDirs scope that actually gates + // `progress_percent`) is UNREADABLE. Without `progress_scope` a consumer + // sees `scope: "complete"` next to `progress_percent: null` with nothing + // in the JSON explaining why. + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.progress_scope, SCOPE.UNREADABLE); + }); + + test('A5: query progress, non-COMPLETE scope -> percent: null, no number rendered', (t) => { + const cwd = createTempDir('gsd-3217-a5-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + const rendered = queryProgressRaw(cwd); + assert.strictEqual(rendered.phase_scope, SCOPE.TRUNCATED); + assert.strictEqual(rendered.percent, null); + }); + + test('A6: stats, non-COMPLETE scope -> percent: null and plan_percent: null', (t) => { + const cwd = createTempDir('gsd-3217-a6-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + const stats = statsRaw(cwd); + assert.strictEqual(stats.phase_scope, SCOPE.TRUNCATED); + assert.strictEqual(stats.percent, null); + assert.strictEqual(stats.plan_percent, null); + }); + + test('A7: state update-progress, non-COMPLETE scope -> STATE.md Progress line untouched, no percentage written', (t) => { + const cwd = createTempDir('gsd-3217-a7-'); + t.after(() => cleanup(cwd)); + buildUnscopedFixture(cwd); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + writeFile(cwd, '.planning/STATE.md', fs.readFileSync(statePath, 'utf-8') + '\n**Progress:** [░░░░░░░░░░] 0%\n'); + const before = fs.readFileSync(statePath, 'utf-8'); + + const output = stateUpdateProgressRaw(cwd); + assert.strictEqual(output, 'false'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual(after, before, 'STATE.md must not be modified when scope is not COMPLETE'); + }); + + // A8 (workstream inventory, non-COMPLETE scope -> no percentage in the + // projection): NOT covered. `buildWorkstreamInventory` + // (src/workstream-inventory-builder.cts) is a pure projection whose caller + // (workstream-inventory.cts) does not thread a real `SCOPE` value into its + // inputs, and its own `milestoneScoped` is a pre-ADR-3180 bespoke boolean + // that cannot distinguish TRUNCATED from UNSCOPED from UNREADABLE. Doing + // this honestly requires either widening `WorkstreamInventory.progress_percent` + // from `number` to `number | null` (an explicitly out-of-scope + // return-type re-architecture per this phase's design doc, "Known limits") + // or reusing the bespoke boolean as a `Scope` stand-in (exactly the + // textual-proxy-for-a-data-flow-property this phase's own guard section + // rejects). See the written-reason comment at + // src/workstream-inventory-builder.cts's `progress_percent` field. A bare + // `return` here is this repo's documented PASS-not-skip shape for a row + // this phase deliberately leaves un-migrated, rather than silently + // omitting the row. + test('A8: workstream inventory — NOT migrated this phase (written reason above; see src/workstream-inventory-builder.cts)', () => { + return; + }); + + test('A9: cross-surface — one non-COMPLETE fixture, none of roadmap analyze / query progress / stats renders a number', (t) => { + const cwd = createTempDir('gsd-3217-a9-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + + const analyzed = analyzeRaw(cwd); + const progress = queryProgressRaw(cwd); + const stats = statsRaw(cwd); + + assert.strictEqual(analyzed.scope, SCOPE.TRUNCATED); + assert.strictEqual(progress.phase_scope, SCOPE.TRUNCATED); + assert.strictEqual(stats.phase_scope, SCOPE.TRUNCATED); + + assert.strictEqual(analyzed.progress_percent, null); + assert.strictEqual(progress.percent, null); + assert.strictEqual(stats.percent, null); + assert.strictEqual(stats.plan_percent, null); + }); + + test('A10: state json --raw, UNREADABLE phases dir -> progress.percent is ABSENT (not 0, not null-valued)', (t) => { + const cwd = createTempDir('gsd-3217-a10-'); + t.after(() => cleanup(cwd)); + buildUnreadableFixture(cwd); + + const built = stateJsonRaw(cwd); + // buildStateFrontmatter's established convention (unchanged by this + // phase): a null percent is OMITTED from `progress`, never emitted as + // `percent: 0` or `percent: null`. Before finding 1's fix this hardcoded + // SCOPE.COMPLETE and, because the ROADMAP heading gave a real + // totalPhases=1 with disk-scanned completedPhases=0 (the phases dir + // being unreadable collapses to an empty scanned set), rendered an + // EARNED-LOOKING but untrustworthy `percent: 0`. + assert.ok(built.progress === undefined || !('percent' in built.progress)); + }); + + test('A11 (finding-1 repro): one genuinely UNREADABLE-phases-dir fixture — state json / roadmap analyze / stats / query progress / state update-progress ALL withhold, none renders a number', (t) => { + const cwd = createTempDir('gsd-3217-a11-'); + t.after(() => cleanup(cwd)); + buildUnreadableFixture(cwd); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + const before = fs.readFileSync(statePath, 'utf-8'); + + const built = stateJsonRaw(cwd); + const analyzed = analyzeRaw(cwd); + const stats = statsRaw(cwd); + const progress = queryProgressRaw(cwd); + const updateOutput = stateUpdateProgressRaw(cwd); + + assert.ok(built.progress === undefined || !('percent' in built.progress), 'state json must not render a percent'); + assert.strictEqual(analyzed.progress_percent, null, 'roadmap analyze must withhold'); + assert.strictEqual(analyzed.progress_scope, SCOPE.UNREADABLE, 'roadmap analyze must expose WHY via progress_scope'); + assert.strictEqual(stats.percent, null, 'stats must withhold'); + assert.strictEqual(stats.phase_scope, SCOPE.UNREADABLE); + assert.strictEqual(progress.percent, null, 'query progress must withhold'); + assert.strictEqual(progress.phase_scope, SCOPE.UNREADABLE); + assert.strictEqual(updateOutput, 'false', 'state update-progress must not write'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual(after, before, 'state update-progress must leave STATE.md untouched'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// B. The negative space — a real 0 must survive (over-withholding guard) +// ═════════════════════════════════════════════════════════════════════════ + +describe('B. negative space — a REAL 0 under COMPLETE must still render', () => { + test('B1: COMPLETE scope, zero phases in a freshly-declared milestone -> 0, rendered (not withheld)', (t) => { + const cwd = createTempDir('gsd-3217-b1-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', ''].join('\n')); + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases'), { recursive: true }); + + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.progress_percent, 0); + + const progress = queryProgressRaw(cwd); + assert.strictEqual(progress.phase_scope, SCOPE.COMPLETE); + assert.strictEqual(progress.percent, 0); + }); + + test('B2: COMPLETE scope, denominator 0 (plans exist in no phase) -> 0 (rule 2, unchanged)', (t) => { + const cwd = createTempDir('gsd-3217-b2-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '### Phase 1: Foo'].join('\n')); + // Phase directory exists but has no PLAN files at all -> denominator 0. + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases', '01-foo'), { recursive: true }); + + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.total_plans, 0); + assert.strictEqual(analyzed.progress_percent, 0); + }); + + test('B3: COMPLETE scope, all phases complete -> 100', (t) => { + const cwd = createTempDir('gsd-3217-b3-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '### Phase 1: Foo'].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); + + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.progress_percent, 100); + }); + + test('B4: a free-form legacy ROADMAP is COMPLETE for windowing (§7.1) — its percentage must NOT be lost', (t) => { + const cwd = createTempDir('gsd-3217-b4-'); + t.after(() => cleanup(cwd)); + // No STATE.md, no versioned milestone headings at all — the classic + // free-form legacy shape (classifyMilestoneWindow row 3). + writeRoadmap(cwd, ['# ROADMAP', '', '### Phase 1: Foo'].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + + const analyzed = analyzeRaw(cwd); + // Identity scope is UNSCOPED for a free-form roadmap, but WINDOWING scope + // (what gates progress_percent here) must be COMPLETE per §7.1. + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(typeof analyzed.progress_percent, 'number'); + assert.strictEqual(analyzed.progress_percent, 0); + }); + + test('B5: boundary on the denominator under COMPLETE: 0, 1, >1', (t) => { + const cwd = createTempDir('gsd-3217-b5-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '### Phase 1: Foo'].join('\n')); + + // Denominator 0: no PLAN files. + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases', '01-foo'), { recursive: true }); + assert.strictEqual(analyzeRaw(cwd).progress_percent, 0); + + // Denominator 1, completed: 1/1 -> 100. + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); + assert.strictEqual(analyzeRaw(cwd).progress_percent, 100); + + // Denominator >1: add a second, incomplete plan -> 1/2 -> 50. + writeFile(cwd, '.planning/phases/01-foo/01-02-PLAN.md', '# Plan\n'); + assert.strictEqual(analyzeRaw(cwd).progress_percent, 50); + }); + + test('B6: null vs 0 are distinguishable by a typed consumer', (t) => { + const cwd = createTempDir('gsd-3217-b6-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', ''].join('\n')); + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases'), { recursive: true }); + const zeroCase = analyzeRaw(cwd); + assert.strictEqual(zeroCase.progress_percent, 0); + assert.notStrictEqual(zeroCase.progress_percent, null); + + cleanup(cwd); + const cwd2 = createTempDir('gsd-3217-b6b-'); + t.after(() => cleanup(cwd2)); + buildTruncatedFixture(cwd2); + const nullCase = analyzeRaw(cwd2); + assert.strictEqual(nullCase.progress_percent, null); + assert.notStrictEqual(nullCase.progress_percent, 0); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// C. Rule 3 at cmdRoadmapAnalyze — the site Phase 3 did not reach +// ═════════════════════════════════════════════════════════════════════════ + +describe('C. rule 3 at roadmap analyze — progress_percent routed through listMilestonePhaseDirs', () => { + // One fixture exercising C1 and C3 together: a backlog/sentinel dir + // (999.1-backlog, fully "complete") and a phase outside the current + // milestone window (01-foo, under v1.0, fully "complete") must both be + // excluded from progress_percent's numerator AND denominator — only + // 02-bar (under the current v2.0 window) may count. + function buildScopedFixture(cwd) { + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, [ + '## v1.0 Old', + '', + '### Phase 1: Foo', + '', + '## v2.0 Current 🚧', + '', + '### Phase 2: Bar', + ].join('\n')); + // Out-of-window, fully complete — must not inflate the scoped total. + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); + // In-window, incomplete. + writeFile(cwd, '.planning/phases/02-bar/02-01-PLAN.md', '# Plan\n'); + // Backlog/sentinel, fully complete — must never count as a milestone phase. + writeFile(cwd, '.planning/phases/999.1-backlog/999.1-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/999.1-backlog/999.1-01-SUMMARY.md', '# Summary\n'); + } + + test('C1 + C3: numerator/denominator are the listMilestonePhaseDirs-scoped set only — no inflation from backlog or out-of-window dirs', (t) => { + const cwd = createTempDir('gsd-3217-c1c3-'); + t.after(() => cleanup(cwd)); + buildScopedFixture(cwd); + + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + // Only 02-bar's 1 plan / 0 summaries — not 01-foo's or the backlog's. + assert.strictEqual(analyzed.total_plans, 1); + assert.strictEqual(analyzed.total_summaries, 0); + assert.strictEqual(analyzed.progress_percent, 0); + assert.notStrictEqual(analyzed.progress_percent, 100, 'the two complete-but-excluded dirs must not leak in'); + }); + + test('C2: roadmap analyze vs stats vs query progress on one fixture report the SAME percentage', (t) => { + const cwd = createTempDir('gsd-3217-c2-'); + t.after(() => cleanup(cwd)); + buildScopedFixture(cwd); + + const analyzed = analyzeRaw(cwd); + const progress = queryProgressRaw(cwd); + const stats = statsRaw(cwd); + + assert.strictEqual(analyzed.progress_percent, 0); + assert.strictEqual(progress.percent, 0); + assert.strictEqual(stats.plan_percent, 0); + assert.strictEqual(analyzed.progress_percent, progress.percent); + assert.strictEqual(progress.percent, stats.plan_percent); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// D. computeProgressPercent (state-document.cts:329) — direct unit tests +// ═════════════════════════════════════════════════════════════════════════ + +describe('D. computeProgressPercent — scope-required signature', () => { + test('D1: no plan data and no phase data -> null (pre-existing behavior, must not regress)', () => { + assert.strictEqual(computeProgressPercent(null, null, null, null, SCOPE.COMPLETE), null); + }); + + test('D2a: plan data only -> min of the two fractions (phase fraction defaults to 1) via clampPercentFromFraction', () => { + // completedPlans=1, totalPlans=2 -> planFraction 0.5; no phase data -> + // phaseFraction defaults to 1; min(0.5, 1) = 0.5 -> 50. + assert.strictEqual(computeProgressPercent(1, 2, null, null, SCOPE.COMPLETE), 50); + }); + + test('D2b: phase data only -> min of the two fractions (plan fraction defaults to 1)', () => { + assert.strictEqual(computeProgressPercent(null, null, 1, 4, SCOPE.COMPLETE), 25); + }); + + test('D2c: both plan and phase data -> the MIN fraction wins', () => { + // planFraction = 1/1 = 1.0; phaseFraction = 1/4 = 0.25 -> min is 0.25 -> 25. + assert.strictEqual(computeProgressPercent(1, 1, 1, 4, SCOPE.COMPLETE), 25); + }); + + test('D3: scope not COMPLETE -> null, even with otherwise-valid plan/phase data', () => { + assert.strictEqual(computeProgressPercent(1, 1, 1, 1, SCOPE.TRUNCATED), null); + assert.strictEqual(computeProgressPercent(1, 1, 1, 1, SCOPE.UNSCOPED), null); + assert.strictEqual(computeProgressPercent(1, 1, 1, 1, SCOPE.UNREADABLE), null); + }); + + test('D4: completedPlans > totalPlans (over-count) -> clamped at 100, never above', () => { + assert.strictEqual(computeProgressPercent(7, 5, null, null, SCOPE.COMPLETE), 100); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// E. Tier-2 regression surface +// ═════════════════════════════════════════════════════════════════════════ + +describe('E. Tier-2 regression surface', () => { + test('E1: a consumer parsing progress_percent as always-numeric must now handle null — pins the new nullable contract', (t) => { + const cwd = createTempDir('gsd-3217-e1-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + const analyzed = analyzeRaw(cwd); + // Deliberate Tier-2 break, pinned: an always-numeric parse of + // `progress_percent` (e.g. `Number(progress_percent).toFixed(0)`) would + // have silently coerced this to "NaN" or "0" pre-#3217. It is `null`. + assert.strictEqual(analyzed.progress_percent, null); + assert.throws(() => { + if (typeof analyzed.progress_percent !== 'number') throw new TypeError('progress_percent is not always numeric'); + }, TypeError); + }); + + test('E2: existing power-proof / consumer-identity fixture (tests/completion-ratio-single-owner.test.cjs) still resolves COMPLETE and matches clampPercent(owner totals)', (t) => { + // Reuses that file's own fixture shape (a clean single-milestone project) + // to confirm this phase's routing change does not alter the COMPLETE + // case those pre-existing tests assert on. + const cwd = createTempDir('gsd-3217-e2-'); + t.after(() => cleanup(cwd)); + buildCompleteFixture(cwd); + const analyzed = analyzeRaw(cwd); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.progress_percent, clampPercent(analyzed.total_summaries, analyzed.total_plans)); + }); + + test('E3: scripts/lint-completion-ratio-drift.cjs on the real tree — still an earned 0 for rules 1-2', () => { + const violations = drift.scanRepo(REPO_ROOT); + assert.deepStrictEqual(violations, []); + }); + + test('E4: a deliberate arithmetic re-derivation fixture is still flagged — the existing guard keeps working, unmodified by this phase', () => { + const line = 'const p = total > 0 ? Math.min(100, Math.round((done / total) * 100)) : 0;'; + const out = drift.findCompletionRatioDrift(line, 'src/somewhere.cts'); + assert.strictEqual(out.length, 1); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// F/G/H. `state sync` (cmdStateSync) — BLOCKER fix (post-review, #3217) +// +// cmdStateSync's own disk scan (`entries`, ~state.cts:3113) is UNFILTERED by +// the real milestone window — it only drops retired phase numbers (#1514). +// A phase directory outside the real window (or present while the window +// itself is TRUNCATED/UNSCOPED) still counted toward totalDiskPlans / +// totalDiskSummaries / diskCompletedPhases, and the percent gate hardcoded +// SCOPE.COMPLETE, so a non-COMPLETE window could still compute and WRITE a +// fabricated percentage into STATE.md's body. Fixed by threading the real +// `listMilestonePhaseDirs` scope through the same gate `computeProgressPercent` +// already enforces for every other consumer. +// ═════════════════════════════════════════════════════════════════════════ + +// UNSCOPED-row-4 fixture: no milestone asserted in STATE.md at all +// (`versionResolved` false) while the ROADMAP carries versioned milestone +// headings (`hasVersionedMilestones` true) — classifyMilestoneWindow row 4, +// distinct from the row-5 fixture above (which asserts an unmatched version). +// A completed phase directory is included so the pre-fix hardcoded-COMPLETE +// disk scan has real numbers to fabricate a percentage from. +function buildUnscopedRow4Fixture(cwd) { + writeState(cwd, {}); + writeRoadmap(cwd, [ + '## v1.0 Shipped', + '', + '### Phase 1: Foo', + ].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); +} + +// TRUNCATED fixture (mirrors buildTruncatedFixture above) but WITH a +// completed phase directory on disk — the empty-window/empty-phases-dir +// shape above never exercises cmdStateSync's fabrication bug because +// cmdStateSync returns early ({ changes: [] }) when `.planning/phases` +// itself does not exist. This variant makes the phases dir real so the +// disk-scan gate is actually reached. +function buildTruncatedFixtureWithPhaseDir(cwd) { + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, [ + '## v1.0 Planned', + '', + '### Phase 1: Foo', + '', + '## v2.0 Current 🚧', + ].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); +} + +// A COMPLETE-scope fixture whose disk-derived percent is a genuine 0 (empty +// milestone window, empty phases dir) — the over-withholding guard (rule 2 +// negative space) for the WRITE path specifically. +function buildSyncCompleteZeroFixture(cwd) { + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['## v1.0 Current 🚧', ''].join('\n')); + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases'), { recursive: true }); +} + +// Appends a syncable body (STATE.md's frontmatter is already written by +// writeState/buildXFixture above) carrying a `Progress:` line that +// `state-transition.cts`'s syncCore can locate and, if warranted, replace — +// `stateExtractField`'s plain-line pattern (`^Progress:`). `initialPercent` +// lets a test start from a value distinguishable from both 0 and any +// fabricated disk-derived number. +function appendSyncableBody(cwd, initialPercent) { + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + const existing = fs.readFileSync(statePath, 'utf-8'); + fs.writeFileSync( + statePath, + existing + [ + '', + '# GSD State', + '', + '## Configuration', + 'Total Plans in Phase: 1', + `Progress: [░░░░░░░░░░] ${initialPercent}`, + 'Last Activity: 2020-01-01', + '', + ].join('\n'), + ); +} + +function stateSyncRaw(cwd) { + const result = runGsdTools(['state', 'sync', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + return JSON.parse(result.output); +} + +describe('F. state sync (cmdStateSync) — BLOCKER: withhold on a non-COMPLETE scope, never fabricate', () => { + test('F1: TRUNCATED window — no Progress rewrite, a skip entry appears in changes, body carries no fabricated percent', (t) => { + const cwd = createTempDir('gsd-3217-f1-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixtureWithPhaseDir(cwd); + appendSyncableBody(cwd, '0%'); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + + const out = stateSyncRaw(cwd); + assert.strictEqual(out.synced, true); + assert.ok( + out.changes.some((c) => /^Progress: skipped/.test(c) && /#3217/.test(c)), + `expected a #3217 skip entry in changes, got: ${JSON.stringify(out.changes)}`, + ); + assert.ok( + !out.changes.some((c) => /^Progress: \[/.test(c)), + `must not record a Progress bar rewrite, got: ${JSON.stringify(out.changes)}`, + ); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Progress: [░░░░░░░░░░] 0%'), 'body Progress line must be untouched'); + assert.ok(!/100%/.test(after), 'must never fabricate 100% (the pre-fix defect: disk scan saw 1/1 complete)'); + }); + + test('F2: UNSCOPED row 4 (no milestone asserted, ROADMAP has versioned headings) — same withholding', (t) => { + const cwd = createTempDir('gsd-3217-f2-'); + t.after(() => cleanup(cwd)); + buildUnscopedRow4Fixture(cwd); + appendSyncableBody(cwd, '0%'); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + + const out = stateSyncRaw(cwd); + assert.ok( + out.changes.some((c) => /^Progress: skipped/.test(c) && /#3217/.test(c)), + `expected a #3217 skip entry, got: ${JSON.stringify(out.changes)}`, + ); + assert.ok(!out.changes.some((c) => /^Progress: \[/.test(c))); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Progress: [░░░░░░░░░░] 0%'), 'body Progress line must be untouched'); + assert.ok(!/100%/.test(after), 'must never fabricate 100%'); + }); + + test('F3: UNSCOPED row 5 (asserted version with no matching heading) — unchanged: the pre-existing #1761 guard still fires first', (t) => { + const cwd = createTempDir('gsd-3217-f3-'); + t.after(() => cleanup(cwd)); + buildUnscopedFixture(cwd); + appendSyncableBody(cwd, '0%'); + + const out = stateSyncRaw(cwd); + assert.ok( + out.changes.some((c) => /^Progress: skipped/.test(c) && /#1761/.test(c)), + `row 5 must still hit the pre-existing #1761 guard, got: ${JSON.stringify(out.changes)}`, + ); + // The new #3217 gate is orthogonal and must never fire once #1761 already + // skipped — only one skip entry should be recorded. + assert.strictEqual(out.changes.filter((c) => /^Progress: skipped/.test(c)).length, 1); + assert.ok(!out.changes.some((c) => /#3217/.test(c))); + }); + + test('F4: COMPLETE scope, genuine 0 — still WRITTEN (over-withholding guard)', (t) => { + const cwd = createTempDir('gsd-3217-f4-'); + t.after(() => cleanup(cwd)); + buildSyncCompleteZeroFixture(cwd); + appendSyncableBody(cwd, '100%'); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + + const out = stateSyncRaw(cwd); + assert.ok( + out.changes.some((c) => /^Progress: .* -> \[░{10}\] 0%$/.test(c)), + `expected a real 0% write, got: ${JSON.stringify(out.changes)}`, + ); + assert.ok(!out.changes.some((c) => /^Progress: skipped/.test(c)), 'a COMPLETE scope must never skip'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(/Progress: \[░{10}\] 0%/.test(after), 'body must carry the real 0%, not the stale 100%'); + }); +}); + +describe('G. cross-surface — one non-COMPLETE fixture, ALL FIVE surfaces withhold (the assertion that would have caught the defect)', () => { + test('G1: state sync / state json / roadmap analyze / stats / query progress all agree on withholding', (t) => { + const cwd = createTempDir('gsd-3217-g1-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixtureWithPhaseDir(cwd); + appendSyncableBody(cwd, '0%'); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + + const syncOut = stateSyncRaw(cwd); + const jsonOut = stateJsonRaw(cwd); + const analyzed = analyzeRaw(cwd); + const stats = statsRaw(cwd); + const progress = queryProgressRaw(cwd); + + assert.ok( + syncOut.changes.some((c) => /^Progress: skipped/.test(c)), + 'state sync must withhold (skip entry present)', + ); + assert.ok(!syncOut.changes.some((c) => /^Progress: \[/.test(c)), 'state sync must not rewrite Progress'); + assert.ok(jsonOut.progress === undefined || !('percent' in jsonOut.progress), 'state json must withhold'); + assert.strictEqual(analyzed.progress_percent, null, 'roadmap analyze must withhold'); + assert.strictEqual(stats.percent, null, 'stats must withhold'); + assert.strictEqual(progress.percent, null, 'query progress must withhold'); + + const afterSync = fs.readFileSync(statePath, 'utf-8'); + assert.ok(afterSync.includes('Progress: [░░░░░░░░░░] 0%'), 'body Progress line untouched by state sync'); + assert.ok(!/100%/.test(afterSync), 'state sync must never fabricate 100% on this fixture'); + }); +}); + +describe('H. self-consistency — after state sync, STATE.md body and frontmatter never contradict each other', () => { + test('H1: non-COMPLETE scope — body carries no fabricated percent AND frontmatter progress: carries no percent key', (t) => { + const cwd = createTempDir('gsd-3217-h1-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixtureWithPhaseDir(cwd); + appendSyncableBody(cwd, '0%'); + const statePath = path.join(planningDirOf(cwd), 'STATE.md'); + + stateSyncRaw(cwd); + const after = fs.readFileSync(statePath, 'utf-8'); + + // Body half: the Progress: bar line must still read 0%, never 100%. + assert.ok(/Progress: \[░{10}\] 0%/.test(after), 'body Progress line must remain at 0%'); + + // Frontmatter half: syncStateFrontmatter (called from writeStateMd on + // every write, including this one) regenerates the YAML block via the + // already-fixed buildStateFrontmatter — its progress: sub-block must + // omit `percent` entirely on this non-COMPLETE fixture. Before this fix + // the two halves could disagree within the SAME write: frontmatter + // (already scoped) omitted percent while the body (hardcoded + // SCOPE.COMPLETE) rendered one. + const fmMatch = after.match(/^---\r?\n([\s\S]*?)\r?\n---/); + assert.ok(fmMatch, 'STATE.md must carry a frontmatter block after a write'); + assert.ok(!/^\s*percent:/m.test(fmMatch[1]), 'frontmatter progress: block must not carry a percent key'); + + // The self-contradiction shape this test guards against: body renders a + // percent while frontmatter's own progress: block has none (or vice + // versa). Assert directly that no percent digit sequence beyond the + // untouched-0% line's own "0%" appears anywhere else in the body. + const percentTokens = after.match(/\d+%/g) || []; + assert.deepStrictEqual(percentTokens, ['0%'], `no other percentage may appear anywhere in STATE.md, got: ${JSON.stringify(percentTokens)}`); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// The guard — narrow syntactic check considered and DROPPED (documented) +// ═════════════════════════════════════════════════════════════════════════ +// +// The design (`40-design.md`, "The guard") named one candidate narrow, +// syntactic structural check: a call to `listMilestonePhaseDirs(...)` whose +// `.value` is read while `.scope` is never bound in the same function. This +// was prototyped and REJECTED against the real tree: `src/milestone.cts` has +// three call sites (`~697`, `~753`, `~881`) that read only `.value` from +// `listMilestonePhaseDirs` — legitimately, because they use the directory +// list for milestone ARCHIVING, not percentage rendering, and have nothing to +// do with rule 3/4 at all. A purely syntactic "was `.scope` bound" check +// cannot distinguish "this consumer renders a percentage" from "this +// consumer does something else with the directory list" — exactly the +// false-positive class the design's own "guard" section predicts for a +// textual proxy over a data-flow property. Per this phase's explicit +// instruction ("if it produces false positives on the real tree, drop it and +// say so"), no such check was added to +// scripts/lint-completion-ratio-drift.cjs; the existing arithmetic-detection +// guard (rules 1-2) is unchanged, and this phase's real enforcement is the +// consumer-identity tests in sections A-C above (ADR Decision 4b/4c). diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index 37b51b9ae..9ddf70c59 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -1052,7 +1052,12 @@ function buildStateWithCuratedProgress(opts) { * Only `numRealizedDirs` phase dirs will have plan/summary files on disk. */ function buildRoadmap(numPhases) { - const lines = ['# ROADMAP', '', '## Milestone v1.0', '']; + // #3217 (ADR-3180 §7.6 rule 4): no version token in the heading — none of + // this section's STATE.md fixtures set a `milestone:` field, so a + // `vX.Y`-bearing heading here would window as UNSCOPED (§7.1 row 4: + // "has versioned milestones, but no version resolved"), not the free-form + // COMPLETE window these tests' phase/plan counting depends on. + const lines = ['# ROADMAP', '', '## Milestone', '']; for (let i = 1; i <= numPhases; i++) { lines.push(`### Phase ${i}: phase-${i}`); lines.push(''); @@ -1196,6 +1201,11 @@ describe('#3242 Bug A: body-only state.update preserves curated progress frontma }); test('state.update "Progress" resyncs progress frontmatter from the updated body', () => { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token) + // is COMPLETE scope for windowing (§7.1) — without this, an absent + // ROADMAP.md is UNREADABLE and the body-Progress-field resync this test + // exercises is withheld. + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const statePath = path.join(tmpDir, '.planning', 'STATE.md'); fs.writeFileSync(statePath, buildStateWithCuratedProgress({ completedPlans: 22, diff --git a/tests/gsd-mcp-server.test.cjs b/tests/gsd-mcp-server.test.cjs index c931b90ee..925d9f6e8 100644 --- a/tests/gsd-mcp-server.test.cjs +++ b/tests/gsd-mcp-server.test.cjs @@ -69,6 +69,12 @@ test('tools/call gsd_invoke_command: dispatches to the command hub (point 1); un test('tools/call gsd_invoke_command: REGRESSION #2102 — a valid read-only family dispatches for real (not the createHub()-with-no-args UnknownCommand bug)', () => { const dir = createTempDir(); try { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const res = handleMessage( { jsonrpc: '2.0', id: 9, method: 'tools/call', params: { name: 'gsd_invoke_command', arguments: { family: 'progress', subcommand: 'json' } } }, { cwd: dir }, diff --git a/tests/pi-extension-reachability.test.cjs b/tests/pi-extension-reachability.test.cjs index c2208f6b5..ce6d5f83f 100644 --- a/tests/pi-extension-reachability.test.cjs +++ b/tests/pi-extension-reachability.test.cjs @@ -18,6 +18,8 @@ const { test } = require('node:test'); const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); const gsdPiExtension = require('../pi/gsd.cjs'); const { _internals } = require('../pi/gsd.cjs'); @@ -62,6 +64,12 @@ test('REACHABILITY: the /gsd handler dispatches a real family through gsd-tools. gsdPiExtension(pi); const dir = createTempDir(); try { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = await pi._recorded.commands['gsd'].handler('progress json', { cwd: dir }); // #2991: handler returns { content: [{ type: 'text', text }] } (Pi's display shape), not a bare string. assert.ok(result && Array.isArray(result.content) && result.content[0].type === 'text', @@ -95,6 +103,11 @@ test('REACHABILITY: the gsd_invoke tool dispatches through the engine and return gsdPiExtension(pi); const dir = createTempDir(); try { + // #3217 (ADR-3180 §7.6 rule 4): see the /gsd handler reachability test + // above — a bare temp dir has no ROADMAP.md (UNREADABLE), withholding + // `percent`. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = await pi._recorded.tools['gsd_invoke'].execute( 'call-1', { family: 'progress', subcommand: 'json' }, diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index b49ff78ff..86b6dc340 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -241,9 +241,13 @@ describe('roadmap analyze command', () => { }); test('parses phases with goals and disk status', () => { + // #3217 (ADR-3180 §7.6 rule 4): no version token — no STATE.md exists to + // resolve which milestone "v1.0" names, so a version-bearing title here + // would window as UNSCOPED (§7.1 row 4), not the free-form COMPLETE + // window this test's disk-status counting depends on. fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), - `# Roadmap v1.0 + `# Roadmap ### Phase 1: Foundation **Goal:** Set up infrastructure diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs index fdab2fcc3..ddfb5f96c 100644 --- a/tests/shell-command-projection-dispatch.test.cjs +++ b/tests/shell-command-projection-dispatch.test.cjs @@ -134,6 +134,12 @@ describe('dispatchGsdCommand', () => { }); test('a valid read-only family/subcommand dispatches for real and returns ok:true + non-empty stdout', () => { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: tmpDir }); assert.equal(result.ok, true, `expected ok:true, got: ${JSON.stringify(result)}`); assert.equal(typeof result.stdout, 'string'); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 55c846da5..208e9463d 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -642,6 +642,11 @@ describe('state json command', () => { }); test('builds frontmatter on-the-fly from body when no frontmatter exists', () => { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token) + // is COMPLETE scope for windowing (§7.1) — without this, an absent + // ROADMAP.md is UNREADABLE and the body-Progress-field fallback this + // test exercises is withheld. + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); fs.writeFileSync( path.join(tmpDir, '.planning', 'STATE.md'), `# Project State @@ -1665,6 +1670,11 @@ describe('cmdStateUpdateProgress (state update-progress)', () => { beforeEach(() => { tmpDir = createFixture(); + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token) + // is COMPLETE scope for windowing (§7.1) — without this, an absent + // ROADMAP.md is UNREADABLE and state update-progress withholds + // (updated:false) instead of computing a percent. + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); }); afterEach(() => { @@ -2321,6 +2331,11 @@ describe('progress counters correct after plan execution (#1589)', () => { beforeEach(() => { tmpDir = createFixture(); + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token) + // is COMPLETE scope for windowing (§7.1) — without this, an absent + // ROADMAP.md is UNREADABLE and the disk-derived percent this describe + // exercises is withheld. + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); }); afterEach(() => { @@ -9953,7 +9968,12 @@ function writeStateFile(tmpDir, overrides = {}) { * filter includes them (avoids needing a milestone header to count phases). */ function writeRoadmap(tmpDir, phaseNums) { - const lines = ['## Roadmap v1.0']; + // #3217 (ADR-3180 §7.6 rule 4): no version token — none of this helper's + // callers write a STATE.md `milestone:` field, so a `vX.Y`-bearing heading + // here would window as UNSCOPED (§7.1 row 4: "has versioned milestones, + // but no version resolved"), not the free-form COMPLETE window the + // percent/count assertions below depend on. + const lines = ['## Roadmap']; for (const n of phaseNums) { lines.push('', `### Phase ${n}: Phase ${n}`); } @@ -10474,6 +10494,11 @@ describe('cmdStateSync nested plans/ layout (#3257)', () => { '', ].join('\n'); fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateContent, 'utf-8'); + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version token) + // is COMPLETE scope for windowing (§7.1) — without this, an absent + // ROADMAP.md is UNREADABLE and the Progress: field this test asserts on + // is withheld ("milestone phase scope is unreadable, not COMPLETE"). + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = runGsdTools('state sync', tmpDir); assert.ok(result.success, `state sync failed: ${result.error}`); diff --git a/tests/vscode-extension-reachability.test.cjs b/tests/vscode-extension-reachability.test.cjs index 3295674dd..a050d3486 100644 --- a/tests/vscode-extension-reachability.test.cjs +++ b/tests/vscode-extension-reachability.test.cjs @@ -24,6 +24,7 @@ const { test } = require('node:test'); const assert = require('node:assert/strict'); const Module = require('node:module'); const path = require('node:path'); +const fs = require('node:fs'); const extension = require('../vscode/extension.js'); const { activate, dispatchGsdCommand, resolveEngineRoot, resolveWorkspaceCwd } = extension; @@ -39,6 +40,12 @@ test('the extension exports activate + dispatchGsdCommand + resolveEngineRoot', test('REACHABILITY: dispatchGsdCommand dispatches a real family/subcommand through gsd-tools.cjs and returns REAL output (keystone wired, not UnknownCommand)', async () => { const dir = createTempDir(); try { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = await dispatchGsdCommand({ family: 'progress', subcommand: 'json', cwd: dir }); assert.equal(typeof result, 'string', 'returns a string result'); const parsed = JSON.parse(result); diff --git a/tests/vscode-lm-tools.test.cjs b/tests/vscode-lm-tools.test.cjs index d5ce27307..95133a402 100644 --- a/tests/vscode-lm-tools.test.cjs +++ b/tests/vscode-lm-tools.test.cjs @@ -19,6 +19,8 @@ const { test } = require('node:test'); const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); const pkg = require('../vscode/package.json'); const extension = require('../vscode/extension.js'); @@ -32,7 +34,15 @@ class FakeToolResult { constructor(parts) { this.parts = parts; } } -function mockVscodeLm() { +// `workspaceCwd`: createLanguageModelTool's invoke() resolves cwd via +// resolveWorkspaceCwd(vscode) — vscode.workspace.workspaceFolders[0].uri.fsPath +// — NOT via a `cwd` field on the invoke() options object (LanguageModelToolInvocationOptions +// has no such field on the real API; see extension.js's #2103 FIX comment on +// createLanguageModelTool). A test that wants a real dispatch to run against a +// fixture directory must mock workspace.workspaceFolders here, not pass +// `{ cwd }` in the options object handed to invoke() (that field is simply +// never read). +function mockVscodeLm(workspaceCwd) { const registered = []; return { lm: { @@ -41,6 +51,9 @@ function mockVscodeLm() { return { dispose() {} }; }, }, + workspace: workspaceCwd + ? { workspaceFolders: [{ uri: { fsPath: workspaceCwd } }] } + : undefined, LanguageModelTextPart: FakeTextPart, LanguageModelToolResult: FakeToolResult, registered, @@ -89,11 +102,20 @@ test('REACHABILITY (desktop): registerLanguageModelTools registers every manifes test('REACHABILITY (desktop): gsd_progress tool.invoke() dispatches through the hub and returns REAL output', async () => { const dir = createTempDir(); try { - const mock = mockVscodeLm(); + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + // The LM tool's invoke() resolves cwd via vscode.workspace.workspaceFolders + // (resolveWorkspaceCwd), not via an options.cwd field — pass the fixture + // dir through the mock's workspace so dispatch actually runs against it. + const mock = mockVscodeLm(dir); extension.registerLanguageModelTools(mock, { subscriptions: [] }); const progressTool = mock.registered.find((r) => r.name === 'gsd_progress'); assert.ok(progressTool, 'gsd_progress must be registered'); - const result = await progressTool.impl.invoke({ input: {}, cwd: dir }, {}); + const result = await progressTool.impl.invoke({ input: {} }, {}); assert.ok(result instanceof FakeToolResult, 'invoke must return a LanguageModelToolResult'); assert.ok(Array.isArray(result.parts) && result.parts.length === 1); assert.ok(result.parts[0] instanceof FakeTextPart, 'result part must be a LanguageModelTextPart'); @@ -107,11 +129,14 @@ test('REACHABILITY (desktop): gsd_progress tool.invoke() dispatches through the test('REACHABILITY (desktop): gsd_plan_phase tool.invoke() forwards the "phase" input through dispatch (real, not UnknownCommand)', async () => { const dir = createTempDir(); try { - const mock = mockVscodeLm(); + // The LM tool's invoke() resolves cwd via vscode.workspace.workspaceFolders + // (resolveWorkspaceCwd), not via an options.cwd field — pass the fixture + // dir through the mock's workspace so dispatch actually runs against it. + const mock = mockVscodeLm(dir); extension.registerLanguageModelTools(mock, { subscriptions: [] }); const planPhaseTool = mock.registered.find((r) => r.name === 'gsd_plan_phase'); assert.ok(planPhaseTool); - const result = await planPhaseTool.impl.invoke({ input: { phase: 'nonexistent-phase-8675309' }, cwd: dir }, {}); + const result = await planPhaseTool.impl.invoke({ input: { phase: 'nonexistent-phase-8675309' } }, {}); const parsed = JSON.parse(result.parts[0].text); // Real dispatch reaches gsd-tools.cjs and returns a structured "phase not // found" response (proves the engine was reached) — not the manifest's @@ -126,10 +151,13 @@ test('REACHABILITY (desktop): gsd_plan_phase tool.invoke() forwards the "phase" test('gsd_workstreams tool.invoke() dispatches through the hub and returns REAL output', async () => { const dir = createTempDir(); try { - const mock = mockVscodeLm(); + // The LM tool's invoke() resolves cwd via vscode.workspace.workspaceFolders + // (resolveWorkspaceCwd), not via an options.cwd field — pass the fixture + // dir through the mock's workspace so dispatch actually runs against it. + const mock = mockVscodeLm(dir); extension.registerLanguageModelTools(mock, { subscriptions: [] }); const wsTool = mock.registered.find((r) => r.name === 'gsd_workstreams'); - const result = await wsTool.impl.invoke({ input: {}, cwd: dir }, {}); + const result = await wsTool.impl.invoke({ input: {} }, {}); const parsed = JSON.parse(result.parts[0].text); assert.ok('workstreams' in parsed || 'mode' in parsed, 'expected a real workstream list response shape'); } finally { diff --git a/tests/vscode-subagent-dispatch.test.cjs b/tests/vscode-subagent-dispatch.test.cjs index 43e671b21..770935c77 100644 --- a/tests/vscode-subagent-dispatch.test.cjs +++ b/tests/vscode-subagent-dispatch.test.cjs @@ -21,6 +21,8 @@ const { test } = require('node:test'); const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); const extension = require('../vscode/extension.js'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -74,6 +76,12 @@ test('registerSubagentDispatch: available:false (fail-soft) when vscode.workspac test('REACHABILITY: a background-eligible dispatchAsSubagent call at depth 0 dispatches through the shared hub and returns REAL output', async () => { const dir = createTempDir(); try { + // #3217 (ADR-3180 §7.6 rule 4): a free-form ROADMAP.md (no version + // token) is COMPLETE scope for windowing (§7.1) — without this, a + // bare temp dir has no ROADMAP.md at all (UNREADABLE) and `percent` + // is withheld (null), breaking this reachability proxy. + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); const result = JSON.parse(await extension.dispatchAsSubagent({ family: 'progress', subcommand: 'json', cwd: dir, depth: 0, }));