From 3d03ae65e6a084987ad5debaab911d5929fd1518 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 5 Sep 2026 13:01:28 -0400 Subject: [PATCH] fix(#4094): withhold all four STATE.md progress counters under the milestone-unbounded guard (#4322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4094): failing-first matrix for withholding all four progress counters * fix(#4094): withhold all four progress counters under the milestone-unbounded guard completed_phases/total_plans/completed_plans are accumulated from the same phaseDirs walk as total_phases, so the #3354/#3573 withhold condition makes them equally untrustworthy — yet only total_phases was withheld, and every resyncing state.* write silently clobbered the three stored siblings with the under-scoped disk numbers. Extend the withhold-then-fall-back-to-stored pattern to all three siblings: null sentinels in the disk-scan cache value, three new stored-counter readers threaded through all three buildStateFrontmatter call sites, and the same cached-else-stored consumer fallback. Milestone-bounded projects are untouched (gate-conditional). * fix(#4094): scope-requires for the new test block, keep the (#3573) warning token, and update two #3578 rows to the withheld-counter contract - the #4094 describe sat after the closing brace of the section that owned the module-level beforeEach destructure, so it needs its own local requires (mirroring the #3642 block); - the #3573 warning keeps its literal '(#3573)' tag (asserted by an existing test) with '#4094' appended as a separate token; - two #3578 status-guard rows in tests/state.test.cjs asserted the pre-#4094 unconditional disk-scan assignment of completed_phases under the roadmap-absent withhold — exactly the silent clobber #4094 removes; the status-guard conclusion (must not fire) is unchanged, the counter-value assertions now pin the withheld contract. * test(#4094): lint conformance — splitLines for the persisted-progress parser, local seeder, scoped rmSync disable * changeset(#4094) * changeset(#4094): backfill PR number --------- Co-authored-by: sim --- .changeset/patient-orcas-tumble.md | 5 + src/state.cts | 101 +++++++-- tests/state-document.test.cjs | 331 +++++++++++++++++++++++++++++ tests/state.test.cjs | 50 +++-- 4 files changed, 450 insertions(+), 37 deletions(-) create mode 100644 .changeset/patient-orcas-tumble.md diff --git a/.changeset/patient-orcas-tumble.md b/.changeset/patient-orcas-tumble.md new file mode 100644 index 000000000..8b2acddc1 --- /dev/null +++ b/.changeset/patient-orcas-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4322 +--- +**STATE.md progress counters are no longer silently regressed on projects whose asserted milestone has no matching ROADMAP heading** — under the milestone-unbounded (or ROADMAP-absent) condition, every resyncing `state.*` write kept `progress.total_phases` at its stored value but clobbered `completed_phases`, `total_plans`, and `completed_plans` with the under-scoped phase-directory scan; all four counters are now withheld together and keep their stored values. (#4094) diff --git a/src/state.cts b/src/state.cts index b44a2ef42..2ae2cdd61 100644 --- a/src/state.cts +++ b/src/state.cts @@ -331,9 +331,12 @@ const _diskScanCache = new Map; const preBody = stripFrontmatter(preContent); const storedMilestone = typeof existingFm['milestone'] === 'string' ? existingFm['milestone'] : null; - const builtFm = buildStateFrontmatter(preBody, cwd, storedMilestone, readStoredTotalPhases(existingFm)); + const builtFm = buildStateFrontmatter(preBody, cwd, storedMilestone, readStoredTotalPhases(existingFm), readStoredCompletedPhases(existingFm), readStoredTotalPlans(existingFm), readStoredCompletedPlans(existingFm)); const progress = builtFm['progress'] as Record | undefined; const percent = progress && typeof progress['percent'] === 'number' ? progress['percent'] : null; const completedPlans = progress && typeof progress['completed_plans'] === 'number' ? progress['completed_plans'] : null; @@ -2688,7 +2691,20 @@ function countRoadmapPhaseHeadings( * 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, storedTotalPhases?: number | null): Record { +function buildStateFrontmatter( + bodyContent: string, + cwd: string | undefined, + storedMilestone?: string | null, + storedTotalPhases?: number | null, + // #4094: the stored siblings of storedTotalPhases, threaded from each call + // site exactly the same way — see readStoredProgressCounter below. Under the + // #3354/#3573 withhold condition the disk scan returns null for all four + // counters, and these stored values are what the progress block falls back + // to (else the keys are omitted). + storedCompletedPhases?: number | null, + storedTotalPlans?: number | null, + storedCompletedPlans?: 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); @@ -2965,7 +2981,7 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto const milestonedButUnbounded = !milestoneBounded && roadmapHasAnyMilestoneSection; 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 milestone section(s) — one (#3642) or several (#3354) — none matching it; the whole-document count would attribute a foreign section's phases to this milestone and the on-disk phase-directory count would understate the declared total, so progress.total_phases is left at its stored value. (#3354/#3642)\n` + `gsd: warning — milestone '${String(assertedMilestoneVersion ?? '').trim()}' is asserted in STATE.md but matches no ROADMAP heading, and the ROADMAP carries milestone section(s) — one (#3642) or several (#3354) — none matching it; the whole-document count would attribute a foreign section's phases to this milestone and the on-disk phase-directory count would understate the declared total, so the progress counters (total_phases, completed_phases, total_plans, completed_plans) are left at their stored values. (#3354/#3642/#4094)\n` ); } // #3573: the roadmap-absent sibling of the #3354 shape. With ROADMAP.md @@ -2985,22 +3001,32 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto storedMilestone.trim() !== ''; if (roadmapAbsentWithAssertedMilestone) { process.stderr.write( - `gsd: warning — milestone '${storedMilestone.trim()}' is asserted in STATE.md but ROADMAP.md is absent or unreadable, so the phase-heading total cannot be derived; the on-disk phase-directory count would understate the declared total, so progress.total_phases is left at its stored value. (#3573)\n` + `gsd: warning — milestone '${storedMilestone.trim()}' is asserted in STATE.md but ROADMAP.md is absent or unreadable, so the phase-heading total cannot be derived; the on-disk phase-directory count would understate the declared total, so the progress counters (total_phases, completed_phases, total_plans, completed_plans) are left at their stored values. (#3573) (#4094)\n` ); } + // #4094: the withhold condition covers ALL FOUR progress counters, + // not just total_phases. completed_phases / total_plans / + // completed_plans are accumulated from the exact same phaseDirs + // walk as total_phases (same loop, same scope, same filters), so + // whenever that walk's scope is known-untrustworthy — the exact + // condition #3354 established — they are equally untrustworthy. + // Pre-#4094 only totalPhases was nulled here, so every resyncing + // write silently clobbered the three stored siblings with the + // under-scoped disk numbers. + const diskCountsWithheld = milestonedButUnbounded || roadmapAbsentWithAssertedMilestone; return { // The two WITHHOLD shapes (#3354 milestoned-but-unbounded, #3573 // roadmap-absent-with-asserted-milestone) must be evaluated BEFORE // safeToUseRoadmapCount — in the #3573 shape milestoneBounded is // vacuously true (its gate requires roadmapRaw), so the safe-count // arm would otherwise swallow the withhold. - totalPhases: (milestonedButUnbounded || roadmapAbsentWithAssertedMilestone) + totalPhases: diskCountsWithheld ? null : (safeToUseRoadmapCount ? Math.max(phaseDirs.length, roadmapPhaseCount) : phaseDirs.length), milestoneBounded, - completedPhases: diskCompletedPhases, - totalPlans: diskTotalPlans, - completedPlans: diskTotalSummaries, + completedPhases: diskCountsWithheld ? null : diskCompletedPhases, + totalPlans: diskCountsWithheld ? null : diskTotalPlans, + completedPlans: diskCountsWithheld ? null : diskTotalSummaries, phaseDirScope, }; })(); @@ -3017,9 +3043,28 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto } else if (storedTotalPhases !== null && storedTotalPhases !== undefined) { totalPhases = storedTotalPhases; } - completedPhases = cached.completedPhases; - totalPlans = cached.totalPlans; - completedPlans = cached.completedPlans; + // #4094: the same withhold-then-fall-back-to-stored pattern for the + // three sibling counters. They are derived from the identical + // phaseDirs walk, so cached.* === null here means the SAME withheld + // condition — keep the stored frontmatter value when the caller can + // supply it; else leave null (key omitted). Note completedPhases / + // completedPlans have NO body-annotation fallback (only the totals + // have body annotations), so an unstored-withheld counter is omitted. + if (cached.completedPhases !== null) { + completedPhases = cached.completedPhases; + } else if (storedCompletedPhases !== null && storedCompletedPhases !== undefined) { + completedPhases = storedCompletedPhases; + } + if (cached.totalPlans !== null) { + totalPlans = cached.totalPlans; + } else if (storedTotalPlans !== null && storedTotalPlans !== undefined) { + totalPlans = storedTotalPlans; + } + if (cached.completedPlans !== null) { + completedPlans = cached.completedPlans; + } else if (storedCompletedPlans !== null && storedCompletedPlans !== undefined) { + completedPlans = storedCompletedPlans; + } milestoneUnbounded = cached.milestoneBounded === false; diskScope = cached.phaseDirScope; } @@ -3318,16 +3363,38 @@ function readStateHeadFreshness( * instead of being clobbered by the on-disk phase-directory count. */ function readStoredTotalPhases(existingFm: Record | null | undefined): number | null { + return readStoredProgressCounter(existingFm, 'total_phases'); +} + +/** + * #4094: the three sibling readers of readStoredTotalPhases, one per progress + * counter the #3354/#3573 withhold now protects. All four counters come from + * the same disk-scan walk and are withheld together; these readers feed the + * stored-value fallback for the three that previously had none. + */ +function readStoredProgressCounter(existingFm: Record | null | undefined, key: string): 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']; + const raw = (progress as Record)[key]; 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 readStoredCompletedPhases(existingFm: Record | null | undefined): number | null { + return readStoredProgressCounter(existingFm, 'completed_phases'); +} + +function readStoredTotalPlans(existingFm: Record | null | undefined): number | null { + return readStoredProgressCounter(existingFm, 'total_plans'); +} + +function readStoredCompletedPlans(existingFm: Record | null | undefined): number | null { + return readStoredProgressCounter(existingFm, 'completed_plans'); +} + function syncStateFrontmatter( content: string, cwd: string | undefined, @@ -3375,7 +3442,7 @@ function syncStateFrontmatter( // 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)); + const derivedFm = buildStateFrontmatter(body, cwd, storedMilestone, readStoredTotalPhases(existingFm), readStoredCompletedPhases(existingFm), readStoredTotalPlans(existingFm), readStoredCompletedPlans(existingFm)); // Preserve existing frontmatter status when body-derived status is 'unknown'. // This prevents a missing Status: field in the body from overwriting a @@ -4750,7 +4817,7 @@ function cmdStateJson(cwd: string, raw: boolean): void { // reports the phase-directory count while the persisted file preserves the // stored total, exactly the write/read divergence #3354 closed for its shape. const storedMilestoneJson = typeof existingFm['milestone'] === 'string' ? existingFm['milestone'] : null; - const built = buildStateFrontmatter(body, cwd, storedMilestoneJson, readStoredTotalPhases(existingFm)); + const built = buildStateFrontmatter(body, cwd, storedMilestoneJson, readStoredTotalPhases(existingFm), readStoredCompletedPhases(existingFm), readStoredTotalPlans(existingFm), readStoredCompletedPlans(existingFm)); // ADR-3408 §8.5 / D3: route stopped_at / paused_at / status / current_phase / // current_phase_name / current_plan through the SAME `preserve-when-unchanged` diff --git a/tests/state-document.test.cjs b/tests/state-document.test.cjs index 214904e66..01569eefd 100644 --- a/tests/state-document.test.cjs +++ b/tests/state-document.test.cjs @@ -1787,6 +1787,337 @@ describe('#3573 total_phases — roadmap absent with an asserted milestone', () }); } +// ───────────────────────────────────────────────────────────────────────────── +// #4094 — the #3354 milestone-unbounded withhold only protects +// progress.total_phases. completed_phases / total_plans / completed_plans are +// accumulated from the SAME phaseDirs walk (same IIFE, same loop) but were +// returned unconditionally and assigned straight from cached.* on every +// resyncing write, silently clobbering stored values under the exact +// condition #3354 established the scan is untrustworthy for. Extend the +// withhold-then-fall-back-to-stored pattern to all three siblings. +// Matrix: .gsd/bug/fix-4094-milestone-withhold-all-counters/50-test-matrix.md +// ───────────────────────────────────────────────────────────────────────────── + +describe('#4094 milestone-unbounded withhold — all four progress counters', () => { + // Local requires: this block sits after the closing brace of the section + // that owned the module-level beforeEach/afterEach destructure above, so + // (mirroring the #3642 block below) everything it needs is required here. + const { test, 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'); + const { runNode } = require('./helpers/process-seam.cjs'); + const { TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs'); + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + + /** Seed `.planning/phases/-phase-` — local copy (the block-scoped seeder at the top of this file is not reachable from here). */ + function seedPhaseDirs(dir, nums) { + for (const n of nums) { + const padded = String(n).padStart(2, '0'); + const phaseDir = path.join(dir, '.planning', 'phases', `${padded}-phase-${n}`); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, `${padded}-01-PLAN.md`), '# Plan\n'); + } + } + + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** The #3354/#4094 crux fixture: two milestone sections, asserted milestone matches neither. */ + 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'); + } + + /** + * STATE.md fixture with an explicit stored progress block. `counters` + * entries that are null/undefined are OMITTED from the frontmatter block + * (the partial-stored rows rely on that). + */ + function buildStateMdWithCounters({ milestone = 'v1.0', counters = {} }) { + const { totalPhases, completedPhases, totalPlans, completedPlans } = counters; + const fm = [ + '---', + 'gsd_state_version: 1.0', + `milestone: ${milestone}`, + 'milestone_name: Unbounded', + 'current_phase: "01"', + 'status: executing', + ]; + const rows = [ + ['total_phases', totalPhases], + ['completed_phases', completedPhases], + ['total_plans', totalPlans], + ['completed_plans', completedPlans], + ].filter(([, v]) => v !== null && v !== undefined); + if (rows.length > 0) { + fm.push('progress:'); + for (const [k, v] of rows) fm.push(` ${k}: ${v}`); + } + fm.push('---', '', '# GSD State', '', '## Current Position', '', '**Current Phase:** 01', '**Status:** Executing', ''); + return fm.join('\n'); + } + + /** Read the PERSISTED progress block values straight out of STATE.md. */ + function persistedProgress(dir) { + const raw = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8'); + const lines = splitLines(raw); + const out = {}; + let inProgress = false; + for (const line of lines) { + if (/^progress:\s*$/.test(line)) { inProgress = true; continue; } + if (!inProgress) continue; + const kv = line.match(/^ {2}(\w+): (.+)$/); + if (!kv) break; // progress block ended + out[kv[1]] = kv[2].trim(); + } + return out; + } + + function recordSession(dir, stoppedAt = 'Phase 1, Plan 1') { + return runNode( + [TOOLS_PATH, 'state', 'record-session', '--stopped-at', stoppedAt, '--resume-file', 'none'], + { cwd: dir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 }, + ); + } + + function stateJson(dir) { + const jsonResult = runGsdTools(['state', 'json', '--raw'], dir); + assert.ok(jsonResult.success, `state json --raw failed: ${jsonResult.error}`); + return JSON.parse(jsonResult.output); + } + + // Row 1 — the failing-first regression: all four counters preserved. + test('#4094 stored completed_phases/total_plans/completed_plans are preserved alongside total_phases (milestoned-but-unbounded)', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 25, completedPhases: 9, totalPlans: 60, completedPlans: 40 } }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_phases, '25', `#4094: total_phases must keep the stored 25 (existing #3354 guard)`); + assert.strictEqual(p.completed_phases, '9', `#4094: completed_phases must keep the stored 9, not the disk-scan 0. Got ${p.completed_phases}`); + assert.strictEqual(p.total_plans, '60', `#4094: total_plans must keep the stored 60, not the disk-scan 4. Got ${p.total_plans}`); + assert.strictEqual(p.completed_plans, '40', `#4094: completed_plans must keep the stored 40, not the disk-scan 0. Got ${p.completed_plans}`); + }); + + // Row 2 — the #3573 roadmap-absent sibling of the same withhold. + test('#4094 roadmap-absent withhold preserves the three sibling counters too', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 5, completedPhases: 2, totalPlans: 11, completedPlans: 7 } }), + ); + seedPhaseDirs(tmpDir, [1]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_phases, '5', `#4094: total_phases must keep the stored 5`); + assert.strictEqual(p.completed_phases, '2', `#4094: completed_phases must keep the stored 2, not the disk-scan 0. Got ${p.completed_phases}`); + assert.strictEqual(p.total_plans, '11', `#4094: total_plans must keep the stored 11, not the disk-scan 1. Got ${p.total_plans}`); + assert.strictEqual(p.completed_plans, '7', `#4094: completed_plans must keep the stored 7, not the disk-scan 0. Got ${p.completed_plans}`); + }); + + // Row 3 — nothing stored: all four keys omitted, never written from the disk scan. + test('#4094 with nothing stored, all four counter keys are omitted', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMdWithCounters({})); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + for (const key of ['total_phases', 'completed_phases', 'total_plans', 'completed_plans']) { + assert.ok(!(key in p), `#4094: with nothing stored, '${key}' must be omitted — never written from the disk scan. Got ${JSON.stringify(p)}`); + } + }); + + // Row 4 — partial stored block: stored ones preserved, unstated ones omitted. + test('#4094 partial stored block: stored counters preserved, absent counters omitted', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 25, totalPlans: 60 } }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_phases, '25', `#4094: stored total_phases preserved`); + assert.strictEqual(p.total_plans, '60', `#4094: stored total_plans preserved. Got ${p.total_plans}`); + assert.ok(!('completed_phases' in p), `#4094: unstored completed_phases must be omitted. Got ${JSON.stringify(p)}`); + assert.ok(!('completed_plans' in p), `#4094: unstored completed_plans must be omitted. Got ${JSON.stringify(p)}`); + }); + + // Rows 5-8 — boundary hold-1/hold/hold+1 per counter: the gate is boolean, + // so preservation must not depend on how the stored value relates to the + // disk-scan value. Rows 5-7 are the three newly-protected counters; row 8 + // (total_phases) is the already-green control. + const BOUNDARY_SPECS = [ + { key: 'completed_phases', fixtureKey: 'completedPhases', disk: 0, name: 'completed_phases' }, + { key: 'total_plans', fixtureKey: 'totalPlans', disk: 4, name: 'total_plans' }, + { key: 'completed_plans', fixtureKey: 'completedPlans', disk: 0, name: 'completed_plans' }, + { key: 'total_phases', fixtureKey: 'totalPhases', disk: 4, name: 'total_phases (control)' }, + ]; + for (const spec of BOUNDARY_SPECS) { + for (const delta of [-1, 0, 1]) { + test(`#4094 boundary: ${spec.name} preserved at disk${delta >= 0 ? '+' : ''}${delta}`, () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + const stored = spec.disk + delta; + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { [spec.fixtureKey]: stored } }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual( + p[spec.key], + String(stored), + `#4094 boundary: ${spec.key} must keep the stored ${stored} regardless of the disk count ${spec.disk}. Got ${JSON.stringify(p)}`, + ); + }); + } + } + + // Row 9 — legit-decrease negative space: on a BOUNDED milestone the disk + // scan stays authoritative and corrections still land downward. + test('#4094 bounded milestone: disk-scan corrections still land (plan deletion)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'), + ); + seedPhaseDirs(tmpDir, [1, 2]); + // One plan in phase 1, two plans in phase 2. + fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md'), '# Plan\n'); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 3, completedPlans: 1 } }), + ); + + // Delete one of phase 2's plans — a legitimate decrease the scan must apply. + fs.unlinkSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md')); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_plans, '2', `#4094 negative space: bounded scan must correct total_plans 3 -> 2 after plan deletion. Got ${p.total_plans}`); + }); + + // Row 10 — legit-decrease negative space: superseded plans still excluded on a bounded milestone. + test('#4094 bounded milestone: superseded plans still excluded from counts', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'), + ); + seedPhaseDirs(tmpDir, [1, 2]); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'phases', '02-phase-2', '02-02-PLAN.md'), + '---\nstatus: superseded\n---\n# Superseded plan\n', + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 3, completedPlans: 0 } }), + ); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_plans, '2', `#4094 negative space: superseded plan must drop from total_plans on a bounded milestone. Got ${p.total_plans}`); + }); + + // Row 11 — legit-decrease negative space: phase-directory removal still corrects counters when bounded. + test('#4094 bounded milestone: phase-directory removal still corrects counters', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '## Milestone v1.0', '', ...[1, 2].map((i) => `### Phase ${i}: p${i}`), ''].join('\n'), + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 2, completedPhases: 0, totalPlans: 2, completedPlans: 0 } }), + ); + seedPhaseDirs(tmpDir, [1, 2]); + // Removing a FIXTURE subdirectory (a phase dir), not the temp dir itself. + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- deliberate fixture mutation inside tmpDir, not a temp-dir teardown + fs.rmSync(path.join(tmpDir, '.planning', 'phases', '02-phase-2'), { recursive: true }); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.total_phases, '2', `#4094 negative space: bounded roadmap keeps heading-derived total 2. Got ${p.total_phases}`); + assert.strictEqual(p.total_plans, '1', `#4094 negative space: bounded scan must correct total_plans 2 -> 1 after phase-dir removal. Got ${p.total_plans}`); + }); + + // Row 12 — read-surface parity: `state json` reports the preserved values. + test('#4094 state json reports preserved counters under the withhold', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), buildSectionedRoadmap()); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 25, completedPhases: 9, totalPlans: 60, completedPlans: 40 } }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const rec = recordSession(tmpDir); + assert.ok(rec.exitCode === 0, `state record-session failed: ${rec.stderr}`); + + const out = stateJson(tmpDir); + assert.strictEqual(Number(out.progress && out.progress.total_phases), 25, `#4094 json parity: total_phases 25`); + assert.strictEqual(Number(out.progress && out.progress.completed_phases), 9, `#4094 json parity: completed_phases must report the preserved 9. Got ${JSON.stringify(out.progress)}`); + assert.strictEqual(Number(out.progress && out.progress.total_plans), 60, `#4094 json parity: total_plans must report the preserved 60. Got ${JSON.stringify(out.progress)}`); + assert.strictEqual(Number(out.progress && out.progress.completed_plans), 40, `#4094 json parity: completed_plans must report the preserved 40. Got ${JSON.stringify(out.progress)}`); + }); + + // Row 13 — the `state sync` special write path keeps the stored counters too + // (same withhold, same gate — #3573 already threads the stored total there). + test('#4094 state sync keeps stored counters under the withhold', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMdWithCounters({ counters: { totalPhases: 5, completedPhases: 2, totalPlans: 11, completedPlans: 7 } }), + ); + seedPhaseDirs(tmpDir, [1]); + + const rec = runNode( + [TOOLS_PATH, 'state', 'sync'], + { cwd: tmpDir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 }, + ); + assert.ok(rec.exitCode === 0, `state sync failed: ${rec.stderr}`); + + const p = persistedProgress(tmpDir); + assert.strictEqual(p.completed_phases, '2', `#4094: state sync must keep stored completed_phases 2. Got ${p.completed_phases}`); + assert.strictEqual(p.total_plans, '11', `#4094: state sync must keep stored total_plans 11. Got ${p.total_plans}`); + assert.strictEqual(p.completed_plans, '7', `#4094: state sync must keep stored completed_plans 7. Got ${p.completed_plans}`); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // #3642: hasMilestoneSectioning's >=2 threshold let a single non-matching diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 099f9a24d..31ea8fe14 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -11149,21 +11149,20 @@ describe('#3578: complete-phase does not overwrite milestone status when phases // No ROADMAP.md at all + a stale "Total Phases: 2" body annotation drives // the #3573 roadmap-absent withhold path: totalPhases stays pinned at the // stale body-declared value (2) instead of being replaced by the live - // disk-scanned total, while completedPhases is UNCONDITIONALLY set from - // the disk scan (buildStateFrontmatter) regardless of that withhold — so - // completedPhases (4) ends up greater than totalPhases (2), making the - // guard's `completedPhases < totalPhases` conjunct false (verified by - // direct probe: status lands 'completed' with progress - // {total_phases:2, completed_phases:4}). Note this fixture necessarily - // also drives listMilestonePhaseDirs' own ROADMAP-absent scope to - // non-COMPLETE (same missing file, independent read), so it does not - // purely isolate the counter conjunct from `diskScope === SCOPE.COMPLETE` - // — src/state.cts's withhold-with-a-stale-numeric-total path is only - // reachable via ROADMAP absence, which always drags that second conjunct - // along with it; no fixture can decouple the two under the current - // implementation. Inconsistent counters deliberately fall through to - // normalizeStateStatus's answer rather than guessing which of the two - // disagreeing numbers is correct. + // disk-scanned total. #4094 extended that withhold to completedPhases too + // — it comes from the same phaseDirs walk and is equally untrustworthy + // here — so completed_phases is withheld (omitted) alongside any + // non-derivable counter, and the guard's `typeof completedPhases === + // 'number'` conjunct fails, so it cannot fire. Pre-#4094 completedPhases + // was UNCONDITIONALLY set from the disk scan (4 > 2 made the + // `completedPhases < totalPhases` conjunct false) — the exact + // "completed_phases larger than a total_phases-consistent value" symptom + // #4094's issue reports. Either way the guard does not demote; the row + // still pins that conclusion. Note this fixture necessarily also drives + // listMilestonePhaseDirs' own ROADMAP-absent scope to non-COMPLETE (same + // missing file, independent read). Inconsistent/untrustworthy counters + // deliberately fall through to normalizeStateStatus's answer rather than + // guessing which of the two disagreeing numbers is correct. seed4PhaseDirsNoRoadmap(4); writeStateAtPhase(4, ['Total Phases: 2']); @@ -11174,14 +11173,18 @@ describe('#3578: complete-phase does not overwrite milestone status when phases assert.strictEqual( frontmatterStatus(after), 'completed', - `guard must not demote when completedPhases > totalPhases; got frontmatter:\n${after}`, + `guard must not demote when the counters are withheld as untrustworthy; got frontmatter:\n${after}`, ); const jsonResult = runGsdTools('state json', tmpDir); assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); const output = JSON.parse(jsonResult.output); - assert.strictEqual(Number(output.progress.completed_phases), 4, 'completed_phases must reflect disk truth (4)'); - assert.strictEqual(Number(output.progress.total_phases), 2, 'total_phases must stay pinned at the stale declared value (2)'); + assert.strictEqual( + output.progress && output.progress.completed_phases, + undefined, + 'completed_phases must be withheld under the #4094 roadmap-absent withhold (no trustworthy scan, no stored value)', + ); + assert.strictEqual(Number(output.progress && output.progress.total_phases), 2, 'total_phases must stay pinned at the stale declared value (2)'); }); test('untrustworthy counters (no ROADMAP.md, no derivable total) must not demote status', () => { @@ -11208,12 +11211,19 @@ describe('#3578: complete-phase does not overwrite milestone status when phases const jsonResult = runGsdTools('state json', tmpDir); assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`); const output = JSON.parse(jsonResult.output); + // #4094: the withhold now covers all four counters (same untrustworthy + // phaseDirs walk), and this fixture stores none of them in frontmatter — + // so the whole progress block may be absent, not just total_phases. assert.strictEqual( - output.progress.total_phases, + output.progress && output.progress.total_phases, undefined, 'total_phases must be withheld (no ROADMAP to derive it from), proving the guard truly had no denominator to compare against', ); - assert.strictEqual(Number(output.progress.completed_phases), 2, 'completed_phases is still disk truth even when total_phases is withheld'); + assert.strictEqual( + output.progress && output.progress.completed_phases, + undefined, + 'completed_phases is withheld too under #4094 (same untrustworthy scan, no stored value)', + ); }); test('#3578 AC4: gsd_invoke_command (MCP dispatch) yields the same non-completed status as the CLI route (2 of 4 case)', () => {