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, }));