fix(#3642): stop the single-section total_phases leak into an absent milestone (#3727)

* test(#3642): failing-first single-section leak rows

* fix(#3642): gate the unbounded total on any-milestone-section, not >=2

* test(#3642): rewrite the 3185 wrapper row to the withhold contract

* docs(#3642): glossary amendment for the >=1 sibling; changeset

* chore(#3642): backfill changeset pr number

---------

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

.changeset/humble-newts-parade.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3727
+---
+**`state` no longer lets a lone non-matching milestone section's phases become another milestone's `total_phases`** — with exactly one milestone section in ROADMAP.md and a STATE.md asserting a different milestone, the section's phases were silently written as the asserted milestone's total (clobbering the stored value). Both that shape and the multi-section one now keep the stored total and warn, naming the asserted milestone. Flat roadmaps (no milestone headings at all) are unchanged. (#3642)

src/roadmap-parser.cts

@@ -354,7 +354,7 @@ const MILESTONE_HEADING_SIGNAL_PATTERN = /v\d+\.\d+|✅|📋|🚧|\bMilestone\b/
  * template or the #3204/#1761/#3185 reports exercises that shape; it is
  * recorded here rather than hidden.
  */
-function hasMilestoneSectioning(content: string): boolean {
+function countMilestoneHeadings(content: string): number {
   const isPhaseHeading = (text: string): boolean => /^Phase\s+\S/i.test(text);
   let milestoneHeadingCount = 0;
   for (const heading of tokenizeHeadings(content)) {
@@ -362,9 +362,31 @@ function hasMilestoneSectioning(content: string): boolean {
     if (isPhaseHeading(heading.text)) continue;
     if (!MILESTONE_HEADING_SIGNAL_PATTERN.test(heading.text)) continue;
     milestoneHeadingCount++;
-    if (milestoneHeadingCount >= 2) return true;
   }
-  return false;
+  return milestoneHeadingCount;
+}
+
+function hasMilestoneSectioning(content: string): boolean {
+  // The >=2 short-circuit the inline walk used to have is gone — a ROADMAP's
+  // heading count is small and tokenizeHeadings materializes the full token
+  // array regardless, so the shared walk pays nothing for it.
+  return countMilestoneHeadings(content) >= 2;
+}
+
+/**
+ * #3642: the >=1 sibling of `hasMilestoneSectioning`. The >=2 predicate
+ * answers SIBLING-conflation ("could two sections' phases mix") and is
+ * unchanged; but `buildStateFrontmatter`'s unbounded branch asks a question
+ * >=2 under-answers: "is there ANY milestone section whose phases a
+ * whole-document count would attribute to a milestone that matches no
+ * heading?" With exactly ONE section and an asserted milestone absent from
+ * the ROADMAP, >=2 said "flat" and the single section's phases leaked into
+ * the asserted milestone's total_phases (silent clobber of the stored
+ * value). Same walk, same vocabulary, threshold 1 — exported for that
+ * consumer only; every other consumer keeps the >=2 semantics.
+ */
+function hasAnyMilestoneSection(content: string): boolean {
+  return countMilestoneHeadings(content) >= 1;
 }
 
 /**
@@ -1694,6 +1716,8 @@ export = {
   sliceMilestoneWindow,
   hasVersionedMilestones,
   hasMilestoneSectioning,
+  // #3642: the >=1 sibling buildStateFrontmatter's unbounded branch consumes.
+  hasAnyMilestoneSection,
   // #1956: sole owner of the #2012 decoy-avoidance scope for the
   // `drift-guard phase-status` CLI seam.
   findRoadmapProgressTable,

src/state.cts

@@ -27,7 +27,8 @@ const {
 } = phaseIdMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import roadmapParserMod = require('./roadmap-parser.cjs');
-const { getMilestoneInfo, extractCurrentMilestone, isMilestoneBoundedInRoadmap, hasMilestoneSectioning } = roadmapParserMod;
+// #3642: hasMilestoneSectioning no longer consumed here — its >=2 semantics answered sibling conflation, but this branch asks asserted-vs-section (>=1). It stays exported from roadmap-parser.cjs for its unit pins.
+const { getMilestoneInfo, extractCurrentMilestone, isMilestoneBoundedInRoadmap, hasAnyMilestoneSection } = roadmapParserMod;
 import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync, toPosixPath, execGit } from './shell-command-projection.cjs';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import planningWorkspace = require('./planning-workspace.cjs');
@@ -2233,10 +2234,19 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto
             // deliberately weaker than isMilestoneBoundedInRoadmap above (no
             // version-token requirement); see hasMilestoneSectioning's own
             // doc comment for why that distinction is load-bearing.
-            const roadmapHasMilestoneSectioning = roadmapRaw !== null
-              && hasMilestoneSectioning(roadmapRaw);
+            // #3642: the flat test uses the >=1 sibling (hasAnyMilestoneSection),
+            // not the >=2 predicate. >=2 under-answers the question this branch
+            // asks: with EXACTLY ONE milestone section and an asserted milestone
+            // absent from the ROADMAP, >=2 read "flat" and the whole-document
+            // count — which IS that single section's phases — was written as the
+            // asserted milestone's total, silently clobbering the stored value.
+            // The >=2 threshold governs SIBLING conflation; asserted-vs-section
+            // needs only one section to go wrong. Zero sections (genuinely flat)
+            // keeps the whole-document count, per #2828.
+            const roadmapHasAnyMilestoneSection = roadmapRaw !== null
+              && hasAnyMilestoneSection(roadmapRaw);
             const safeToUseRoadmapCount = milestoneBounded
-              || (roadmapPhaseCount > 0 && !roadmapHasMilestoneSectioning);
+              || (roadmapPhaseCount > 0 && !roadmapHasAnyMilestoneSection);
             // #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
@@ -2252,10 +2262,10 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto
             // 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;
+            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 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`
+                `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`
               );
             }
             // #3573: the roadmap-absent sibling of the #3354 shape. With ROADMAP.md

tests/state-document.test.cjs

@@ -728,11 +728,18 @@ describe('#3204 buildStateFrontmatter total_phases — negative space / boundari
     );
   });
 
-  test('a single milestone section cannot conflate siblings', () => {
-    // Row 6 — exactly ONE '## v2.0' section owning phases, with the asserted
-    // milestone ('v9.9') absent from the roadmap entirely. One milestone
-    // heading can never satisfy hasMilestoneSectioning's >=2 threshold, so
-    // this is NOT sectioned and the roadmap-declared count is still used.
+  test('a single milestone section that is not the asserted one withholds the total (#3642)', () => {
+    // Row 6, REWRITTEN by #3642 (maintainer-confirmed bug; the old contract
+    // here was the bug). Exactly ONE '## v2.0' section owning phases, with
+    // the asserted milestone ('v9.9') absent from the roadmap entirely. The
+    // old row pinned that hasMilestoneSectioning's >=2 threshold reads this
+    // as flat, so the roadmap-declared count (4) was used for v9.9 — which
+    // IS the leak #3642 reports: the v2.0 section's phases became a
+    // different milestone's total. The #3354 withhold doctrine governs both
+    // faces now: neither the whole-document count (it is the foreign
+    // section's phases) NOR the on-disk dir count is authoritative for a
+    // milestone absent from the roadmap, so the STORED value (99, chosen to
+    // differ from every substitute) must be preserved.
     const roadmap = [
       '# Roadmap',
       '',
@@ -746,15 +753,29 @@ describe('#3204 buildStateFrontmatter total_phases — negative space / boundari
     fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap);
     fs.writeFileSync(
       path.join(tmpDir, '.planning', 'STATE.md'),
-      buildStateMd({ milestone: 'v9.9', milestoneName: 'Absent', totalPhases: 4 }),
+      buildStateMd({ milestone: 'v9.9', milestoneName: 'Absent', totalPhases: 99 }),
     );
     seedPhaseDirs(tmpDir, [1, 2]);
 
     const out = recordSessionAndReadTotalPhases(tmpDir);
     assert.strictEqual(
       Number(out.progress.total_phases),
-      4,
-      `a single milestone section cannot conflate siblings; expected the roadmap count (4), got ${out.progress && out.progress.total_phases}`,
+      99,
+      `#3642: a single non-matching section must not leak its phases into the asserted milestone's total; stored 99 expected, got ${out.progress && out.progress.total_phases}`,
+    );
+    // The clobber must also be SURFACED, not silent: drive the seam directly
+    // (runGsdTools discards stderr on success) and require the #3642 warning
+    // naming the asserted milestone.
+    const { runNode } = require('./helpers/process-seam.cjs');
+    const { TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs');
+    const rec = runNode(
+      [TOOLS_PATH, 'state', 'json', '--raw'],
+      { cwd: tmpDir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 },
+    );
+    assert.ok(rec.exitCode === 0, `state json --raw failed: ${rec.stderr}`);
+    assert.ok(
+      (rec.stderr || '').includes('v9.9') && (rec.stderr || '').includes('#3642'),
+      `#3642: expected a stderr warning naming the asserted milestone and the issue, got stderr=${JSON.stringify(rec.stderr)}`,
     );
   });
 
@@ -1052,16 +1073,19 @@ describe('#3185 review — hasMilestoneSectioning shapes the original suite miss
     );
   });
 
-  test('MAJOR: wrapper + single nested milestone keeps the roadmap count', () => {
-    // Adversarial review MAJOR (#3204 reintroduction): every ancestor in a
-    // nesting chain was counted as its own candidate section under the
-    // #3184 rewrite, so a generic wrapper heading with only ONE real
-    // milestone nested under it was misclassified as sectioned. Mirrors
-    // this repo's own bundled template shape (gsd-core/templates/roadmap.md:
-    // '## Phases' -> '### 🚧 v1.1 [Name] (In Progress)' -> '#### Phase N:').
-    // The asserted milestone ('v9.9') is deliberately unbound so the
-    // assertion exercises hasMilestoneSectioning itself, not
-    // isMilestoneBoundedInRoadmap.
+  test('MAJOR: wrapper + single nested milestone, asserted elsewhere, withholds the total (#3642)', () => {
+    // Adversarial review MAJOR (#3204 reintroduction), REWRITTEN by #3642
+    // (maintainer-confirmed bug). The row's original purpose survives: a
+    // generic wrapper ('## Phases') with ONE real milestone nested under it
+    // (bundled template shape: '### 🚧 v1.1 [Name]' -> '#### Phase N:') is
+    // NOT milestone-SECTIONED — hasMilestoneSectioning's >=2 still says
+    // false, pinned by the #3642 seam rows. But the row's old ASSERTION
+    // pinned the leak #3642 reports: with the asserted milestone ('v9.9')
+    // deliberately unbound, the roadmap count (4) — which IS the v1.1
+    // section's phases — was written as v9.9's total. Pre-#3354 that arm
+    // existed to stop a clobber to the disk count (2); the #3354/#3642
+    // withhold doctrine supersedes it: preserve the stored value (8, chosen
+    // to differ from every substitute) and warn.
     const roadmap = [
       '# Roadmap',
       '',
@@ -1078,15 +1102,15 @@ describe('#3185 review — hasMilestoneSectioning shapes the original suite miss
     fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap);
     fs.writeFileSync(
       path.join(tmpDir, '.planning', 'STATE.md'),
-      buildStateMd({ milestone: 'v9.9', milestoneName: 'Unbound', totalPhases: 2 }),
+      buildStateMd({ milestone: 'v9.9', milestoneName: 'Unbound', totalPhases: 8 }),
     );
     seedPhaseDirs(tmpDir, [1, 2]);
 
     const out = recordSessionAndReadTotalPhases(tmpDir);
     assert.strictEqual(
       Number(out.progress.total_phases),
-      4,
-      `wrapper + single nested milestone must keep the roadmap-declared count (4), not clobber to the disk count of 2. Got ${out.progress && out.progress.total_phases}`,
+      8,
+      `#3642: a single real section's phases must not become an absent milestone's total; stored 8 expected. Got ${out.progress && out.progress.total_phases}`,
     );
   });
 });
@@ -1597,3 +1621,122 @@ describe('#3573 total_phases — roadmap absent with an asserted milestone', ()
 });
   });
 }
+
+
+// ─────────────────────────────────────────────────────────────────────────────
+// #3642: hasMilestoneSectioning's >=2 threshold let a single non-matching
+// milestone section's phases leak into an unrelated asserted milestone's
+// total_phases. Fix: the >=1 sibling (hasAnyMilestoneSection) governs the
+// unbounded branch's flat test, so the single-section shape takes the #3354
+// withhold (stored value preserved + warning) instead of substituting the
+// whole-document count. Controls pin what must NOT change.
+// Matrix: .gsd/bug/fix-3642-milestone-sectioning-leak/50-test-matrix.md
+// ─────────────────────────────────────────────────────────────────────────────
+
+describe('#3642 — single-section leak controls and seam pins', () => {
+  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');
+
+  // Compact local builders (this file's sections are deliberately
+  // self-contained; the #3204 suite's identical builders live in its own
+  // scope).
+  function seedDirs(tmpDir, nums) {
+    for (const n of nums) {
+      const padded = String(n).padStart(2, '0');
+      const dir = path.join(tmpDir, '.planning', 'phases', `${padded}-phase-${n}`);
+      fs.mkdirSync(dir, { recursive: true });
+      fs.writeFileSync(path.join(dir, `${padded}-01-PLAN.md`), '# Plan\n');
+    }
+  }
+
+  function stateMd({ milestone, totalPhases }) {
+    return [
+      '---',
+      'gsd_state_version: 1.0',
+      `milestone: ${milestone}`,
+      'milestone_name: M',
+      'current_phase: "01"',
+      'status: executing',
+      'progress:',
+      `  total_phases: ${totalPhases}`,
+      '  completed_phases: 0',
+      '  total_plans: 0',
+      '  completed_plans: 0',
+      '  percent: 0',
+      '---',
+      '',
+      '# GSD State',
+      '',
+      '## Current Position',
+      '',
+      '**Current Phase:** 01',
+      '**Status:** Executing',
+      '',
+    ].join('\n');
+  }
+
+  function writeTotalAfterRecord(tmpDir) {
+    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}`);
+    return Number(JSON.parse(jsonResult.output).progress.total_phases);
+  }
+
+  let tmpDir;
+
+  beforeEach(() => {
+    tmpDir = createTempProject('gsd-3642-');
+  });
+
+  afterEach(() => {
+    cleanup(tmpDir);
+  });
+
+  test('control: a single section that MATCHES the assert keeps its own count', () => {
+    // Bounded arm unchanged: asserted v2.0 IS bound to the one heading, so
+    // the section's own count (4) is used — neither the stored (7) nor the
+    // disk count (2).
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [
+      '# Roadmap', '', '## v2.0', '## Phase 1: One', '## Phase 2: Two',
+      '## Phase 3: Three', '## Phase 4: Four', '',
+    ].join('\n'));
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateMd({ milestone: 'v2.0', totalPhases: 7 }));
+    seedDirs(tmpDir, [1, 2]);
+    assert.strictEqual(writeTotalAfterRecord(tmpDir), 4,
+      'bounded single section keeps its own count (4)');
+  });
+
+  test('control: a FLAT roadmap with an unbounded assert keeps the roadmap count (#2828 doctrine)', () => {
+    // Zero vocabulary headings → genuinely flat → the whole-document count
+    // is correct (no milestone section exists to conflate with).
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [
+      '# Roadmap', '', '## Phase 1: One', '## Phase 2: Two',
+      '## Phase 3: Three', '## Phase 4: Four', '',
+    ].join('\n'));
+    fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateMd({ milestone: 'v9.9', totalPhases: 7 }));
+    seedDirs(tmpDir, [1, 2]);
+    assert.strictEqual(writeTotalAfterRecord(tmpDir), 4,
+      'flat roadmap + unbounded assert keeps the roadmap count (4)');
+  });
+
+  test('seam pins: hasAnyMilestoneSection counts >=1; hasMilestoneSectioning stays >=2', () => {
+    const roadmapParser = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'roadmap-parser.cjs'));
+    assert.strictEqual(typeof roadmapParser.hasAnyMilestoneSection, 'function',
+      'hasAnyMilestoneSection must be exported for buildStateFrontmatter (#3642)');
+    const one = ['# Roadmap', '', '## v2.0', '## Phase 1: One'].join('\n');
+    const two = ['# Roadmap', '', '## v1.0', '## Phase 1: One', '', '## v2.0', '## Phase 2: Two'].join('\n');
+    const flat = ['# Roadmap', '', '## Phase 1: One'].join('\n');
+    assert.strictEqual(roadmapParser.hasAnyMilestoneSection(one), true, 'one signal heading is a section');
+    assert.strictEqual(roadmapParser.hasAnyMilestoneSection(two), true, 'two signal headings is a section');
+    assert.strictEqual(roadmapParser.hasAnyMilestoneSection(flat), false, 'zero signal headings is flat');
+    assert.strictEqual(roadmapParser.hasMilestoneSectioning(one), false, '>=2 predicate unchanged: one heading is NOT sectioning');
+    assert.strictEqual(roadmapParser.hasMilestoneSectioning(two), true, '>=2 predicate unchanged: two headings is sectioning');
+  });
+});