diff --git a/.changeset/curious-mice-munch.md b/.changeset/curious-mice-munch.md new file mode 100644 index 000000000..3fdb11f55 --- /dev/null +++ b/.changeset/curious-mice-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3480 +--- +A genuinely milestone-sectioned ROADMAP whose STATE.md asserts a milestone token matching no heading no longer has progress.total_phases clobbered to the on-disk phase-directory count (e.g. 25 -> 4) on every state-mutating command. The stored total is preserved (or the key omitted when nothing is stored), a stderr warning names the unbounded milestone token, and progress.percent stays withheld as before. diff --git a/src/state.cts b/src/state.cts index 8eac7f00c..295b4aa58 100644 --- a/src/state.cts +++ b/src/state.cts @@ -188,7 +188,11 @@ function shouldResyncStateProgress(fields: Iterable): boolean { // Avoids re-reading N+1 directories on every state write when the phase structure // hasn't changed within the same gsd-tools invocation. const _diskScanCache = new Map { * a YAML frontmatter object. Allows hooks and scripts to read state * reliably via `state json` instead of fragile regex parsing. */ -function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, storedMilestone?: string | null): Record { +function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, storedMilestone?: string | null, storedTotalPhases?: number | null): Record { // #2956: scope `Phase` extraction to ## Current Position (mirrors the read // path in cmdStateSnapshot and the Stopped At / Paused At ## Session scoping // below). Phase canonically lives in ## Current Position (templates/state.md); @@ -1974,10 +1978,31 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto && hasMilestoneSectioning(roadmapRaw); const safeToUseRoadmapCount = milestoneBounded || (roadmapPhaseCount > 0 && !roadmapHasMilestoneSectioning); + // #3354: the milestoned-but-unbounded sibling of the #2828/#3204 + // shapes. The whole-document roadmapPhaseCount is rightly rejected + // above (it would conflate sibling milestones, #1761), but the + // on-disk phase-dir count is NOT an authoritative substitute for + // the rejected total either — it counts only the current + // milestone's realized directories (25 declared → 4 written in the + // issue's report), silently shrinking progress.total_phases on + // every STATE.md write. Mirror the branch's own percent withhold + // (milestoneUnbounded below): return a null sentinel so the caller + // keeps the pre-existing stored value instead of writing the + // substitute, and warn on stderr naming the unbounded token so the + // operator can curate the ROADMAP heading or the STATE assertion. + // The degenerate un-sectioned zero-heading case keeps the + // phaseDirs.length fallback — with nothing declared anywhere else, + // the disk count is the only source and remains correct. + const milestonedButUnbounded = !milestoneBounded && roadmapHasMilestoneSectioning; + if (milestonedButUnbounded) { + process.stderr.write( + `gsd: warning — milestone '${String(assertedMilestoneVersion ?? '').trim()}' is asserted in STATE.md but matches no ROADMAP heading, and the ROADMAP carries multiple milestone sections; the on-disk phase-directory count would understate the declared total, so progress.total_phases is left at its stored value. (#3354)\n` + ); + } return { totalPhases: safeToUseRoadmapCount ? Math.max(phaseDirs.length, roadmapPhaseCount) - : phaseDirs.length, + : (milestonedButUnbounded ? null : phaseDirs.length), milestoneBounded, completedPhases: diskCompletedPhases, totalPlans: diskTotalPlans, @@ -1987,7 +2012,17 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto })(); _diskScanCache.set(cwd, cached); } - totalPhases = cached.totalPhases; + // #3354: cached.totalPhases === null is the milestoned-but-unbounded + // WITHHOLD sentinel — the scan refused to substitute the dir count for + // a rejected whole-document total, so keep the pre-existing value: + // the stored frontmatter total when the caller can supply it, else the + // body "Total Phases" annotation already parsed above, else leave null + // (the key is omitted from the progress block). + if (cached.totalPhases !== null) { + totalPhases = cached.totalPhases; + } else if (storedTotalPhases !== null && storedTotalPhases !== undefined) { + totalPhases = storedTotalPhases; + } completedPhases = cached.completedPhases; totalPlans = cached.totalPlans; completedPlans = cached.completedPlans; @@ -2250,6 +2285,23 @@ function readStateHeadFreshness( }; } +/** + * #3354: read `progress.total_phases` out of already-extracted STATE.md + * frontmatter as a finite number, or null. Feeds buildStateFrontmatter's + * milestoned-but-unbounded withhold so the stored total survives the write + * instead of being clobbered by the on-disk phase-directory count. + */ +function readStoredTotalPhases(existingFm: Record | null | undefined): number | null { + if (!existingFm || typeof existingFm !== 'object') return null; + const progress = existingFm['progress']; + if (!progress || typeof progress !== 'object') return null; + const raw = (progress as Record)['total_phases']; + if (raw === null || raw === undefined) return null; + if (typeof raw === 'string' && raw.trim() === '') return null; + const n = Number(raw); + return Number.isFinite(n) ? n : null; +} + function syncStateFrontmatter(content: string, cwd: string | undefined, authoritativeFm?: Record): string { // Read existing frontmatter BEFORE stripping — it may contain values // that the body no longer has (e.g., Status field removed by an agent). @@ -2264,7 +2316,11 @@ function syncStateFrontmatter(content: string, cwd: string | undefined, authorit // buildStateFrontmatter scopes its disk scan to the correct milestone // instead of auto-deriving (and potentially mis-binding). const storedMilestone = typeof existingFm['milestone'] === 'string' ? existingFm['milestone'] : null; - const derivedFm = buildStateFrontmatter(body, cwd, storedMilestone); + // #3354: also pass the stored total so buildStateFrontmatter's + // milestoned-but-unbounded withhold can preserve it across the write + // (the derived progress sub-block replaces the stored one wholesale below, + // so an omitted key would otherwise DELETE the stored value). + const derivedFm = buildStateFrontmatter(body, cwd, storedMilestone, readStoredTotalPhases(existingFm)); // Preserve existing frontmatter status when body-derived status is 'unknown'. // This prevents a missing Status: field in the body from overwriting a @@ -2835,7 +2891,9 @@ function cmdStateJson(cwd: string, raw: boolean): void { // Always rebuild from body + disk so progress counters reflect current state. // Returning cached frontmatter directly causes stale percent/completed_plans // when SUMMARY files were added after the last STATE.md write (#1589). - const built = buildStateFrontmatter(body, cwd); + // #3354: pass the stored total so the milestoned-but-unbounded withhold can + // report the preserved value instead of omitting the key. + const built = buildStateFrontmatter(body, cwd, undefined, readStoredTotalPhases(existingFm)); // Preserve frontmatter-only fields that cannot be recovered from the body. if (existingFm && existingFm['stopped_at'] && !built['stopped_at']) { diff --git a/tests/state-document.test.cjs b/tests/state-document.test.cjs index a51c31586..b8649e521 100644 --- a/tests/state-document.test.cjs +++ b/tests/state-document.test.cjs @@ -758,12 +758,13 @@ describe('#3204 buildStateFrontmatter total_phases — negative space / boundari ); }); - test('#1761 sibling milestone sections still fall back to the disk count', () => { + test('#1761/#3354 sibling milestone sections preserve the stored total', () => { // Row 5 — TWO sibling (unversioned) milestone sections, asserted // milestone ('v3.0') absent from either. This is genuinely // milestone-sectioned (2 phase-bearing sections would conflate if - // whole-doc counted), so total_phases must stay the disk count. Passes - // today; a fix that touches hasMilestoneSectioning must not break it. + // whole-doc counted), so neither the whole-doc count NOR the on-disk dir + // count is an authoritative total (#3354): the stored value must be + // preserved instead. Pre-#3354 this row asserted the disk count (3). const roadmap = [ '# Roadmap', '', @@ -790,8 +791,8 @@ describe('#3204 buildStateFrontmatter total_phases — negative space / boundari const out = recordSessionAndReadTotalPhases(tmpDir); assert.strictEqual( Number(out.progress.total_phases), - 3, - `#1761: unbounded sibling milestones must fall back to the disk count (3), got ${out.progress && out.progress.total_phases}`, + 8, + `#1761/#3354: unbounded sibling milestones must preserve the stored total (8), not clobber to the disk count (3). Got ${out.progress && out.progress.total_phases}`, ); }); @@ -977,7 +978,7 @@ describe('#3185 review — hasMilestoneSectioning shapes the original suite miss cleanup(tmpDir); }); - test('BLOCKER: same-level sibling milestones fall back to the disk count', () => { + test('BLOCKER: same-level sibling milestones preserve the stored total', () => { // Adversarial review BLOCKER (#1761 regression): the #3184 rewrite // required a candidate milestone heading's owned Phase heading to be // STRICTLY DEEPER (next.level > candidate.level). Real sibling @@ -985,8 +986,10 @@ describe('#3185 review — hasMilestoneSectioning shapes the original suite miss // headings ('## v1.0' / '## Phase 1:' / '## v2.0' / '## Phase 3:'), so // that predicate answered false and the whole-document count conflated // both milestones. The asserted milestone ('v3.0') is unbound (matches - // neither v1.0 nor v2.0), so this is genuinely sectioned and must fall - // back to the disk count. + // neither v1.0 nor v2.0), so this is genuinely sectioned and neither + // the whole-doc count NOR the disk count may be written (#3354): the + // stored value (4) must be preserved. Pre-#3354 this row asserted the + // disk count (2). const roadmap = [ '# Roadmap', '', @@ -1009,8 +1012,8 @@ describe('#3185 review — hasMilestoneSectioning shapes the original suite miss const out = recordSessionAndReadTotalPhases(tmpDir); assert.strictEqual( Number(out.progress.total_phases), - 2, - `same-level sibling milestones must fall back to the disk count (2), got ${out.progress && out.progress.total_phases}`, + 4, + `same-level sibling milestones must preserve the stored total (4), not clobber to the disk count (2). Got ${out.progress && out.progress.total_phases}`, ); }); @@ -1203,5 +1206,120 @@ describe('#3185 buildStateFrontmatter total_phases — directory-enumeration ind ); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3354 — milestoned-but-unbounded: a genuinely milestone-sectioned ROADMAP +// whose asserted milestone token matches no H1–H3 heading must not have its +// progress.total_phases clobbered to the on-disk phase-directory count. +// Surviving branch of #2828/#3204: #3204 closed only the flat-unmilestoned +// case; the sectioned-but-unbounded arm of `safeToUseRoadmapCount` still +// wrote phaseDirs.length (25 → 4 in the issue's report). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3354 buildStateFrontmatter total_phases — milestoned-but-unbounded roadmap', () => { + const { runNode } = require('./helpers/process-seam.cjs'); + const { TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs'); + + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** The #3354 crux fixture: 2 milestone-vocabulary sections, 25 phase headings across siblings. */ + function buildSectionedRoadmap() { + const lines = ['# Roadmap', '']; + lines.push('## Milestone v2.0 — Alpha', ''); + for (let i = 1; i <= 12; i++) lines.push(`### Phase ${i}: alpha-${i}`); + lines.push(''); + lines.push('## Milestone v3.0 — Beta', ''); + for (let i = 13; i <= 25; i++) lines.push(`### Phase ${i}: beta-${i}`); + lines.push(''); + return lines.join('\n'); + } + + test('#3354 stored total is preserved (not clobbered to the dir count), a stderr warning names the token, percent stays withheld', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + // milestone: v1.0 appears in NO heading of that roadmap — unbounded. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ milestone: 'v1.0', milestoneName: 'Unbounded', totalPhases: 25 }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + // Drive the mutating command with stderr captured (runGsdTools discards + // stderr on success). The warning is asserted on THIS invocation. + const rec = runNode( + [TOOLS_PATH, 'state', 'record-session', '--stopped-at', 'Phase 1, Plan 1', '--resume-file', 'none'], + { cwd: tmpDir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 }, + ); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + assert.ok( + /v1\.0/.test(rec.stderr || ''), + `#3354: expected a stderr warning naming the unbounded milestone token 'v1.0', got stderr=${JSON.stringify(rec.stderr)}`, + ); + + const jsonResult = runGsdTools(['state', 'json', '--raw'], tmpDir); + assert.ok(jsonResult.success, `state json --raw failed: ${jsonResult.error}`); + const out = JSON.parse(jsonResult.output); + + assert.strictEqual( + Number(out.progress.total_phases), + 25, + `#3354: total_phases must preserve the stored 25, not clobber to the on-disk dir count of 4. Got ${out.progress && out.progress.total_phases}`, + ); + assert.ok( + !(out.progress && ('percent' in out.progress)), + `#3354: percent must stay withheld for an unbounded milestone (existing #1761 guard); got progress=${JSON.stringify(out.progress)}`, + ); + }); + + test('#3354 with nothing stored, the key is omitted rather than written from the dir count', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + // No progress block in frontmatter, no "Total Phases" body annotation — + // nothing stored to preserve, so the key must be OMITTED (never 4). + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Unbounded', + 'current_phase: "01"', + 'status: executing', + '---', + '', + '# GSD State', + '', + '## Current Position', + '', + '**Current Phase:** 01', + '**Status:** Executing', + '', + ].join('\n'), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const recordResult = runGsdTools( + ['state', 'record-session', '--stopped-at', 'Phase 1, Plan 1', '--resume-file', 'none'], + tmpDir, + ); + assert.ok(recordResult.success, `state record-session failed: ${recordResult.error}`); + + const jsonResult = runGsdTools(['state', 'json', '--raw'], tmpDir); + assert.ok(jsonResult.success, `state json --raw failed: ${jsonResult.error}`); + const out = JSON.parse(jsonResult.output); + + assert.ok( + !(out.progress && ('total_phases' in out.progress)), + `#3354: with no stored total, the key must be omitted — never written from the dir count. Got progress=${JSON.stringify(out.progress)}`, + ); + }); +}); }); }