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

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

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

---------

Co-authored-by: sim <sim@local>

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

src/state.cts

@@ -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']) {

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
+    // whole-doc counted), so neither the whole-doc count NOR the on-disk dir
-    // today; a fix that touches hasMilestoneSectioning must not break it.
+    // 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,
+      8,
-      `#1761: unbounded sibling milestones must fall back to the disk count (3), got ${out.progress && out.progress.total_phases}`,
+      `#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
+    // neither v1.0 nor v2.0), so this is genuinely sectioned and neither
-    // back to the disk count.
+    // 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,
+      4,
-      `same-level sibling milestones must fall back to the disk count (2), got ${out.progress && out.progress.total_phases}`,
+      `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)}`,
+    );
+  });
+});
   });
 }