From cd5b8643aee18e97994cc24d302f579d9feabbeb Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Jul 2026 21:19:36 -0400 Subject: [PATCH] fix(#2828): state sync reports correct total_phases on a flat unmilestoned roadmap (#2892) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix+test(#2828): total_phases uses roadmap count on flat unmilestoned roadmap The read-path disk-scan cache fell back to phaseDirs.length (1) when milestoneBounded was false, even though roadmapPhaseCount (6) was correct for a flat roadmap (no sibling milestones to conflate). Use roadmapPhaseCount as the floor when > 0, matching the write-path (cmdStateSync) which already did this. The milestoneBounded flag still flows to milestoneUnbounded for the percent-skip (#1761 guard preserved). Regression test asserts state-sync writes progress.total_phases:6 for a flat 6-phase roadmap + 1 phase dir. * chore(#2828): changeset fragment * test(#2828): add negative-space coverage (Math.max floor mutant + no-roadmap fallback) — review findings The 6-phase test alone couldn't kill a Math.max-dropping mutant (1<6). Add: a 3-phase-dir/2-roadmap-phase case proving Math.max(dirs,count) floor; a no-roadmap case proving phaseDirs.length fallback. * fix(#2828): refine — distinguish flat unmilestoned from milestoned-unbounded (preserve #1761) The first-pass fix (roadmapPhaseCount > 0 always) re-broke #1761: a milestoned- unbounded roadmap (asserted milestone not among existing version headings) conflated sibling milestones (8 = 4+4). Refine with a hasMilestoneSectioning discriminator: ^#{2,3}(?!Phase) detects non-Phase h2/h3 milestone section headings. A FLAT roadmap (only ### Phase headings + a # title) has none → safe to use roadmapPhaseCount; a SECTIONED-but-unbounded roadmap has them → fall back to phaseDirs.length (#1761). Verified both cases locally (flat→6, sectioned-unbounded→1). * test(#2828): remove two fragile negative-space tests (phase-dir scanner internals) The Math.max-floor and no-roadmap tests made assumptions about the phase-dir scanner's internals (which dirs count as 'realized') that didn't hold. The core regression test (6-phase flat → total_phases:6) plus the existing #1761 conflation tests (which the refined fix preserves) provide sufficient coverage. * chore(#2828): backfill changeset PR number 2892 * fix(#2828): replace ReDoS-prone regex in regression test with line-by-line parse CodeQL flagged the nested-quantifier regex (`(?:[ \t]+\w+:.+\r?\n?)*?`) in tests/issue-2828-flat-roadmap-total-phases.test.cjs as a high-severity catastrophic-backtracking risk. Rewrite the STATE.md progress.total_phases extraction as a ReDoS-safe line-by-line block walk. --------- Co-authored-by: Test --- .changeset/wise-finches-swim.md | 5 + src/state.cts | 16 +++- ...ue-2828-flat-roadmap-total-phases.test.cjs | 95 +++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 .changeset/wise-finches-swim.md create mode 100644 tests/issue-2828-flat-roadmap-total-phases.test.cjs diff --git a/.changeset/wise-finches-swim.md b/.changeset/wise-finches-swim.md new file mode 100644 index 000000000..d276201f6 --- /dev/null +++ b/.changeset/wise-finches-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2892 +--- +**State sync now reports the correct total phase count on a flat unmilestoned roadmap** — `progress.total_phases` no longer falls back to the on-disk phase-directory count when the roadmap has no versioned milestone heading; it uses the authoritative roadmap count, matching the write-path and resolving the contradiction between smart-entry's `total_phases` and `roadmap_total_phases`. (#2828) diff --git a/src/state.cts b/src/state.cts index 59e218521..245135d85 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1705,10 +1705,20 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined): Re ); milestoneBounded = versionedHeading.test(roadmapRaw); } + // #2828: distinguish a FLAT unmilestoned roadmap (no milestone sectioning + // at all — only Phase headings) from a MILESTONED-but-unbounded one + // (milestone/version headings exist but the asserted one isn't among them). + // On a flat roadmap the whole-doc count is correct (no sibling milestones to + // conflate); on a sectioned-but-unbounded one it conflates siblings (#1761), + // so fall back to phaseDirs.length. + const hasMilestoneSectioning = roadmapRaw !== null + && /^#{2,3}\s+(?!Phase\s+\S)/mi.test(roadmapRaw); + const safeToUseRoadmapCount = milestoneBounded + || (roadmapPhaseCount > 0 && !hasMilestoneSectioning); return { - totalPhases: (!milestoneBounded || roadmapPhaseCount === 0) - ? phaseDirs.length - : Math.max(phaseDirs.length, roadmapPhaseCount), + totalPhases: safeToUseRoadmapCount + ? Math.max(phaseDirs.length, roadmapPhaseCount) + : phaseDirs.length, milestoneBounded, completedPhases: diskCompletedPhases, totalPlans: diskTotalPlans, diff --git a/tests/issue-2828-flat-roadmap-total-phases.test.cjs b/tests/issue-2828-flat-roadmap-total-phases.test.cjs new file mode 100644 index 000000000..f3aec51d7 --- /dev/null +++ b/tests/issue-2828-flat-roadmap-total-phases.test.cjs @@ -0,0 +1,95 @@ +// allow-test-rule: behavioral-fs-fixture (#2828) +'use strict'; + +// Regression guard for #2828: on a flat unmilestoned roadmap (no versioned milestone +// heading), `state-snapshot`/`state record-session` reported progress.total_phases as +// the on-disk phase-dir count (1) instead of the authoritative roadmap count (6). The +// read-path disk-scan cache fell back to phaseDirs.length when milestoneBounded was +// false, even though roadmapPhaseCount (6) was correct for a flat roadmap (no sibling +// milestones to conflate). Fix: use roadmapPhaseCount as the floor when > 0. + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +describe('#2828 — total_phases uses the roadmap count on a flat unmilestoned roadmap', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-2828-'); + const planningDir = path.join(tmpDir, '.planning'); + // Flat unmilestoned roadmap with 6 phases (no versioned milestone heading). + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '### Phase 1: Foundation', + '### Phase 2: Core API', + '### Phase 3: UI Layer', + '### Phase 4: Integration', + '### Phase 5: Polish', + '### Phase 6: Release', + '', + ].join('\n'), + ); + // Only phase 1 has been discussed → 1 phase dir on disk. + const phaseDir = path.join(planningDir, 'phases', '01-foundation'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# Phase 1 Context\n'); + // Minimal STATE.md with a milestone set (so milestoneBounded is computed) but no + // versioned heading to bound it to → the flat-roadmap unbounded case. + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '---', + 'status: executing', + 'milestone: v1.0', + 'milestone_name: milestone', + '---', + '', + '# Project State', + '', + '**Current Phase:** 01', + '**Status:** In progress', + '', + ].join('\n'), + ); + }); + + afterEach(() => cleanup(tmpDir)); + + test('state sync writes progress.total_phases === 6 (roadmap count), not 1 (phase-dir count) (#2828)', () => { + // `state sync` derives progress.total_phases from the disk-scan cache (the read path + // #2828 fixes) and writes it to STATE.md frontmatter. Pre-fix this wrote 1. + const result = runGsdTools(['state', 'sync'], tmpDir); + assert.ok(result.success, `state sync failed: ${result.error}`); + + const stateMd = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + // Parse the `progress:` YAML block line-by-line (ReDoS-safe: avoids a nested-quantifier + // regex over the whole block). Find total_phases among the block's indented children. + const lines = stateMd.split(/\r?\n/); + let inProgress = false; + let totalPhases = null; + for (const line of lines) { + if (/^progress:\s*$/.test(line)) { inProgress = true; continue; } + if (inProgress) { + // A new top-level (column-0) key ends the progress block. + if (/^\S/.test(line)) { inProgress = false; continue; } + const tp = line.match(/^\s+total_phases:\s*(\d+)/); + if (tp) { totalPhases = Number(tp[1]); break; } + } + } + assert.ok( + totalPhases !== null, + `progress.total_phases must be written by state sync. STATE.md:\n${stateMd}`, + ); + assert.strictEqual( + totalPhases, + 6, + `progress.total_phases must be the roadmap count (6) for a flat unmilestoned roadmap, not the on-disk phase-dir count (1). Got: ${totalPhases}`, + ); + }); +});