diff --git a/.changeset/humble-newts-parade.md b/.changeset/humble-newts-parade.md new file mode 100644 index 000000000..1d5169daf --- /dev/null +++ b/.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) diff --git a/CONTEXT.md b/CONTEXT.md index 1a9f4065b..8455acbe3 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -222,7 +222,7 @@ Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-t Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/lib/write-set.cjs`; ADR-2143 §5/§6, epic #2143). Pure, Node built-ins only, no I/O. Exports: `Result` (`{ok:true,value}\|{ok:false,reason}` — ADR-2143 §5 fail-loud parse shape, never a bare `null` a caller can mistake for "empty but fine"; the single source of truth `markdown-table.cjs` re-exports so its existing importers are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`); `WriteOutcome` (`{surface: string, applied: boolean}` — one surface's outcome within a multi-surface write); `WriteSet` (`WriteOutcome[]`); `writeSetComplete(ws) → boolean` (true only when the set is non-empty AND every surface applied — ADR-2143 §6's "no OR-into-one-flag" rule: a command that mutates more than one surface must not collapse independent surface outcomes into a single boolean, the anti-pattern that let a checkbox-only partial write (#2140) report full success). `milestone.cts`'s `requirements mark-complete` handler is the first consumer: it reports a `write_set` (`checkbox`/`traceability` surfaces) and `write_set_complete` alongside its existing `updated`/`marked_complete`/`already_complete`/`not_found`/`table_unmatched` fields, which remain computed exactly as before — the write-set is additive, structured ADR-2143 documentation of the same per-surface facts #2140's tactical fix already exposed via `table_unmatched`. ### Roadmap Parser Module -Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Milestone WINDOWING — which headings bound a milestone — is owned here as of #3184 (epic #3180 Phase 2, ADR-3180 Decision 1): `computeMilestoneSectionEnd` (the section-end walk, formerly duplicated as two distinct nested `computeSectionEnd` functions plus an inline third copy in `getMilestonePhaseFilter`'s `versionOverride` branch), `locateMilestoneHeadings` (heading location, version token boundary-matched with `\b`, **NOT** the stricter `(?![\w.-])`: `v2.0` therefore DOES match inside `v2.0.1`, and a milestone STATE of `v8.0` legitimately selects a live `## v8.0-B …` over a closed `v8.0-A` sibling — deliberate, load-bearing #730 behavior that ADR-3180 Amendment 2 tried to tighten and then reverted; the earlier text here described that reverted alternative as if it had shipped, corrected by #3216), `listMilestoneHeadings` (#3216 — the version-AGNOSTIC enumeration of every milestone heading in document order, sharing ONE grammar source with `locateMilestoneHeadings` so the two cannot drift; `locateMilestoneHeadings` is now a version-filtered view over it rather than a second expression of the pattern). Milestone IDENTITY — which milestone is current and what it is CALLED — is owned by `getMilestoneInfo`, which since #3216 binds to that same locator instead of its own heading regexes and returns a `ScopedResult`: a name retains parentheses and drops a trailing `✅`/`📋`/`🚧` marker, a `### Phase N …` heading is never the milestone heading (#3197), and an identity that cannot be determined returns a non-`COMPLETE` scope rather than the former `{version:'v1.0', name:'milestone'}` default, which was output-identical to a successful read of a genuine v1.0 project. `buildStateFrontmatter` and `archivePhaseDirectories` branch on that scope, so a fabricated identity is never persisted to `STATE.md` nor used as a `milestones/-phases/` path component, `sliceMilestoneWindow` (the one composition of locate → prefer-non-closed → section-end, so a consumer cannot re-assemble its own window from the primitives), and `isMilestoneBoundedInRoadmap` (the named predicate replacing two byte-identical re-derivations in `state.cts`). `hasMilestoneSectioning(content)` is the sibling predicate answering the WEAKER question `buildStateFrontmatter` actually asks — "could a whole-document phase count conflate two different milestones?" — and since #3185 it is decided by milestone VOCABULARY, not by heading position: a heading is a milestone heading iff it is a non-Phase heading (level 1-3) carrying a version token, a shipped/active marker, or the word `Milestone`, and sectioning means two or more of them, since one section cannot conflate siblings. Three position-based models were tried and each shipped a defect — "any non-Phase heading" over-detects, so a flat ROADMAP carrying an ordinary `## Progress` was called sectioned and its declared phase count discarded for the on-disk directory count (#3204, #2828 regressing at 1.9.1 via #3184's own consolidation); strict nesting misses same-level siblings (regressing #1761) and false-positives on the bundled `templates/roadmap.md` shape, where a `## Phases` wrapper holds a single nested milestone; adjacency false-positives whenever a structural heading merely precedes a phase heading. Known limit: two milestone sections carrying none of the three signals are not detected. `extractCurrentMilestoneScoped` is the real extractor and returns the Planning Scope Module's `ScopedResult`; `extractCurrentMilestone` remains a one-line wrapper over `.value` because its blast radius is CRITICAL (200+ affected symbols, 20 direct callers) and its signature must not move. `getMilestonePhaseFilter` gains a `scope` field: its pass-all degrade is PRESERVED where its premise holds (a genuinely-empty, freshly-declared milestone reports `SCOPE.COMPLETE`) and is now labelled where it does not (`SCOPE.TRUNCATED` when the window reached no phase entries while the document has them), so the destructive consumer — `milestone.complete`, which MOVES phase directories — can refuse instead of archiving every phase directory on disk (#3166). The filter's function behavior is deliberately unchanged: making it deny-all on a non-COMPLETE scope would trade a silent over-inclusive answer for a silent under-inclusive one on the read paths that count with it. `findRoadmapProgressTable(content)` (#1956) locates the `## Progress` table — scoped to that heading via the markdown-sectionizer seam, falling back to the whole document for a headingless milestone slice — so a differently-headed table sharing the `Phase | Plans Complete | Status | Completed` columns cannot be read instead (the #2012 decoy class). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` expresses the same scope independently for the completion RATIO; the two are held in agreement by a parity test rather than by a shared call, because that symbol's blast radius does not justify a refactor. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). +Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Milestone WINDOWING — which headings bound a milestone — is owned here as of #3184 (epic #3180 Phase 2, ADR-3180 Decision 1): `computeMilestoneSectionEnd` (the section-end walk, formerly duplicated as two distinct nested `computeSectionEnd` functions plus an inline third copy in `getMilestonePhaseFilter`'s `versionOverride` branch), `locateMilestoneHeadings` (heading location, version token boundary-matched with `\b`, **NOT** the stricter `(?![\w.-])`: `v2.0` therefore DOES match inside `v2.0.1`, and a milestone STATE of `v8.0` legitimately selects a live `## v8.0-B …` over a closed `v8.0-A` sibling — deliberate, load-bearing #730 behavior that ADR-3180 Amendment 2 tried to tighten and then reverted; the earlier text here described that reverted alternative as if it had shipped, corrected by #3216), `listMilestoneHeadings` (#3216 — the version-AGNOSTIC enumeration of every milestone heading in document order, sharing ONE grammar source with `locateMilestoneHeadings` so the two cannot drift; `locateMilestoneHeadings` is now a version-filtered view over it rather than a second expression of the pattern). Milestone IDENTITY — which milestone is current and what it is CALLED — is owned by `getMilestoneInfo`, which since #3216 binds to that same locator instead of its own heading regexes and returns a `ScopedResult`: a name retains parentheses and drops a trailing `✅`/`📋`/`🚧` marker, a `### Phase N …` heading is never the milestone heading (#3197), and an identity that cannot be determined returns a non-`COMPLETE` scope rather than the former `{version:'v1.0', name:'milestone'}` default, which was output-identical to a successful read of a genuine v1.0 project. `buildStateFrontmatter` and `archivePhaseDirectories` branch on that scope, so a fabricated identity is never persisted to `STATE.md` nor used as a `milestones/-phases/` path component, `sliceMilestoneWindow` (the one composition of locate → prefer-non-closed → section-end, so a consumer cannot re-assemble its own window from the primitives), and `isMilestoneBoundedInRoadmap` (the named predicate replacing two byte-identical re-derivations in `state.cts`). `hasMilestoneSectioning(content)` is the sibling predicate answering "could a whole-document phase count conflate two different milestones?" — and since #3185 it is decided by milestone VOCABULARY, not by heading position: a heading is a milestone heading iff it is a non-Phase heading (level 1-3) carrying a version token, a shipped/active marker, or the word `Milestone`, and sectioning means two or more of them, since one section cannot conflate siblings. Since #3642, `buildStateFrontmatter`'s flat-vs-milestoned gate consumes the >=1 sibling `hasAnyMilestoneSection` over the same `countMilestoneHeadings` walk instead: asserted-vs-section is a different question than sibling conflation, and needs only ONE section to go wrong — with exactly one section and an asserted milestone absent from the ROADMAP, the whole-document count IS that foreign section's phases, so the #3354 withhold (stored value preserved + warning) governs; a zero-signal (genuinely flat) roadmap keeps the whole-document count per #2828. Three position-based models were tried and each shipped a defect — "any non-Phase heading" over-detects, so a flat ROADMAP carrying an ordinary `## Progress` was called sectioned and its declared phase count discarded for the on-disk directory count (#3204, #2828 regressing at 1.9.1 via #3184's own consolidation); strict nesting misses same-level siblings (regressing #1761) and false-positives on the bundled `templates/roadmap.md` shape, where a `## Phases` wrapper holds a single nested milestone; adjacency false-positives whenever a structural heading merely precedes a phase heading. Known limit: two milestone sections carrying none of the three signals are not detected. `extractCurrentMilestoneScoped` is the real extractor and returns the Planning Scope Module's `ScopedResult`; `extractCurrentMilestone` remains a one-line wrapper over `.value` because its blast radius is CRITICAL (200+ affected symbols, 20 direct callers) and its signature must not move. `getMilestonePhaseFilter` gains a `scope` field: its pass-all degrade is PRESERVED where its premise holds (a genuinely-empty, freshly-declared milestone reports `SCOPE.COMPLETE`) and is now labelled where it does not (`SCOPE.TRUNCATED` when the window reached no phase entries while the document has them), so the destructive consumer — `milestone.complete`, which MOVES phase directories — can refuse instead of archiving every phase directory on disk (#3166). The filter's function behavior is deliberately unchanged: making it deny-all on a non-COMPLETE scope would trade a silent over-inclusive answer for a silent under-inclusive one on the read paths that count with it. `findRoadmapProgressTable(content)` (#1956) locates the `## Progress` table — scoped to that heading via the markdown-sectionizer seam, falling back to the whole document for a headingless milestone slice — so a differently-headed table sharing the `Phase | Plans Complete | Status | Completed` columns cannot be read instead (the #2012 decoy class). `phase-lifecycle.cts`'s `deriveProgressFromRoadmap` expresses the same scope independently for the completion RATIO; the two are held in agreement by a parity test rather than by a shared call, because that symbol's blast radius does not justify a refactor. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), plan/summary pairing helpers (`countMatchedSummaries`, `findUnsummarizedPlans`, `findOrphanSummaries`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `extractCanonicalPlanId`, `timeAgo`). `filterPlanFiles`/`filterSummaryFiles` were retired by #3183 (ADR-3180 Decision 2) — `getPhaseFileStats` no longer re-derives plan/summary filename matching locally; it now sources `plans`/`summaries` (plus a `scope` field, `COMPLETE`/`TRUNCATED`/`UNREADABLE`) directly from `scanPhasePlans` (`src/plan-scan.cts`), the single owner of live-plan counting. Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 9c0079479..58db761a4 100644 --- a/src/roadmap-parser.cts +++ b/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, diff --git a/src/state.cts b/src/state.cts index c48aba33c..56c3ed150 100644 --- a/src/state.cts +++ b/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 diff --git a/tests/state-document.test.cjs b/tests/state-document.test.cjs index 2fb077eb7..119795dc2 100644 --- a/tests/state-document.test.cjs +++ b/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'); + }); +});