golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#3354): preserve stored total_phases when milestone is unbounded (#3480)

Browse Source 
* fix(#3354): preserve stored total_phases when milestone is unbounded

* chore(#3354): backfill PR number in changeset fragment

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-14 11:28:40 -04:00
committed by GitHub
parent 26f8015cc2
commit 70b5c1a1bf
3 changed files with 197 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/curious-mice-munch.md Normal file
View File

@@ -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.

70
src/state.cts
View File

@@ -188,7 +188,11 @@ function shouldResyncStateProgress(fields: Iterable<string>): 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<string, {
totalPhases: number;
// #3354: null is the milestoned-but-unbounded WITHHOLD sentinel — the scan
// refused to substitute the on-disk dir count for a rejected whole-document
// ROADMAP total, so the caller must keep the pre-existing value (stored
// frontmatter, body annotation) or omit the key. Never a scan result.
totalPhases: number | null;
completedPhases: number;
totalPlans: number;
completedPlans: number;
@@ -1731,7 +1735,7 @@ function extractRetiredPhaseNumbers(scope: string): Set<string> {
* 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<string, unknown> {
function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, storedMilestone?: string | null, storedTotalPhases?: number | null): Record<string, unknown> {
// #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<string, unknown> | 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<string, unknown>)['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, unknown>): 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']) {

138
tests/state-document.test.cjs
View File

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