diff --git a/.changeset/gentle-badgers-forage.md b/.changeset/gentle-badgers-forage.md new file mode 100644 index 000000000..dad82d15e --- /dev/null +++ b/.changeset/gentle-badgers-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3230 +--- +**`state.record-session` no longer shrinks your phase count** — a project whose ROADMAP declares more phases than it has directories on disk (phases 5 and 6 planned but not started yet) had `progress.total_phases` silently overwritten with the directory count, converging on the right number only once the last phase directory happened to exist. A flat roadmap carrying an ordinary heading like `## Progress` was being misread as milestone-sectioned. Known limit: two milestone sections carrying no version token, no status marker and not the word "Milestone" are still not detected as sectioning. (#3204) diff --git a/CONTEXT.md b/CONTEXT.md index 9a84e2aed..12a006a8f 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -181,7 +181,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`, generated from `src/write-set.cts`; 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`). `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. 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 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. 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/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index 37fc7af10..05d715ab4 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -260,7 +260,7 @@ This section is that written rule. It is **normative**, and it is what the guard **Guard.** `lint-milestone-window-drift.cjs`, token set widened by Phase 6 in the same change as the consolidation — never after, since a guard added later measures a surface already cleaned and reports a zero it did not earn. Token (a) now admits a **literal** `#`-run heading anchor in addition to the `#{N,M}` quantifier, but only inside a heading-**matcher** literal (a regex literal, or a string handed to `new RegExp(`), so a heading-**builder** template is not mistaken for a re-derivation. Token (b) additionally admits the grouped `v(\d+(?:\.\d+)+)` shape and an interpolated `${…Ver…}` placeholder. -#### 7.3 Phase enumeration — *Enforced for the four named consumers (Phase 3, #3222); the fifth copy is unowned* +#### 7.3 Phase enumeration — *Enforced (Phase 3, #3222 + #3185)* **Question.** Which directories under `/phases/` are phases of milestone `M`? @@ -270,7 +270,11 @@ This section is that written rule. It is **normative**, and it is what the guard **Consumers that must route through the owner.** `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats`, `cmdPhasesList`, **and `buildStateFrontmatter` / `syncStateFrontmatter`** — the fifth copy, reached by `state.record-session`, `state.sync`, `phase.complete` and every other state-mutating verb, which the epic's original scope did not name (coverage-audit gap 1). -**Status, precisely.** Phase 3 (#3222) enforced this rule for `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and its guard (`scripts/lint-phase-enumeration-drift.cjs`) found 54 violations where the epic scoped 4. **It did not reach `buildStateFrontmatter` / `syncStateFrontmatter`.** That copy is still live, still writes its answer to disk, and is now unowned by any phase — see Amendment 4's scope table, row 1. +**Status, precisely.** Phase 3 (#3222) enforced this rule for `cmdRoadmapAnalyze`, `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and its guard (`scripts/lint-phase-enumeration-drift.cjs`) found 54 violations where the epic scoped 4. It also routed `buildStateFrontmatter`'s enumeration through `listMilestonePhaseDirs`, which Amendment 4's scope table row 1 did not credit it for — the audit read the *absence of the symbol names from Amendment 3* as the absence of the work. #3185's remainder was therefore smaller than recorded: the local dedup-key regex that survived alongside the routed enumeration, now on `phaseKeyFromDir`. + +**What was actually unowned was one layer up.** `buildStateFrontmatter` decides whether it may *trust* the ROADMAP's declared count via `hasMilestoneSectioning` (§7.1), and that predicate — introduced by Phase 2 (#3184) to retire `state.cts`'s hand-rolled #2828 guard — was strictly more permissive than the guard it replaced, so a flat ROADMAP carrying an ordinary `## Progress` heading had its declared phase count discarded for the on-disk directory count (#3204). Fixed in #3185 by deciding milestone-ness from vocabulary rather than heading position; see §7.1 and the `CONTEXT.md` Roadmap Parser Module entry for the three position-based models that failed and why. + +**The lesson this phase adds to Amendment 3's standing rule:** a copy count derived from *which symbol names appear in a prior amendment* is as unreliable as one derived from the reported issues. Read the code, not the write-up. **Note on #3204.** Routing `buildStateFrontmatter` through the owner will not by itself fix #3204: its defect is the discriminator one layer *above* enumeration — "is the ROADMAP's phase count safe to trust" — which misclassifies ordinary `## Overview` / `## Progress` headings as milestone sectioning. That discriminator **is** §7.1's `isMilestoneBoundedInRoadmap`. The enumeration routing and the discriminator replacement must ship together or the defect survives the consolidation. @@ -372,7 +376,7 @@ One row per derivation. A blank owner is a derivation whose contract is locked ( |---|---|---|---|---| | Milestone windowing (§7.1) | `roadmap-parser.cts` | `lint-milestone-window-drift.cjs` | `src/` | enforced | | Milestone identity (§7.2) | `roadmap-parser.cts` (Phase 6) | same guard, token set widened by Phase 6 | `src/` | enforced | -| Phase enumeration (§7.3) | `phase-locator.cts` | `lint-phase-enumeration-drift.cjs` | `src/` | enforced for the four named consumers; `buildStateFrontmatter`/`syncStateFrontmatter` **unowned** | +| Phase enumeration (§7.3) | `phase-locator.cts` | `lint-phase-enumeration-drift.cjs` | `src/` | enforced — the state writers included (#3185) | | Phase completion (§7.4) | `verification.cts` (Phase 4) | Phase 4 | `src/` | blocked on #2957 | | Live-plan counting (§7.5) | `plan-scan.cts` | `lint-plan-count-drift.cjs` | `src/` | enforced | | Live-plan counting, prompt layer (§7.5) | — (Phase 8) | `lint-planning-prompt-drift.cjs` | `gsd-core/workflows`, `commands`, `agents`, `skills` | ratcheted, 7 sites | @@ -632,7 +636,7 @@ scoped 4 — so it is not restated here. | # | Change | Why the existing phases do not cover it | |---|---|---| -| 1 | ~~**Phase 3 widens** to include `buildStateFrontmatter` and `syncStateFrontmatter`~~ — **superseded: Phase 3 merged without them.** The fifth enumeration copy and #3204 are now unowned and need a phase of their own | Phase 3's Done-when named only `cmdProgressRender`, `cmdStats` and `cmdPhasesList`, and #3222 shipped exactly that. `buildStateFrontmatter`/`syncStateFrontmatter` appear nowhere in Amendment 3, and #3204 appears nowhere in this ADR outside this row. Routing alone would not have fixed #3204 anyway — its defect is the "is the ROADMAP count trustworthy" discriminator one layer *above* enumeration, which is §7.1's `isMilestoneBoundedInRoadmap` | +| 1 | ~~**Phase 3 widens** to include `buildStateFrontmatter` and `syncStateFrontmatter`~~ — **twice superseded; closed by #3185.** First reading ("Phase 3 merged without them") was itself wrong: #3222 *had* routed the enumeration, and the audit mistook Amendment 3's silence for absent work. The real gap was the trust discriminator one layer above — see §7.3 | Routing alone would never have fixed #3204: a correctly scoped enumeration still returns the directory count. The defect was `hasMilestoneSectioning`, introduced by Phase 2 (#3184) and more permissive than the #2828 guard it retired | | 2 | **Phase 4 blocks on #2957** and its guard must fail while a third predicate exists | The audit found `buildStateFrontmatter` computing completed phases from plan scanning alone, ignoring the ROADMAP checkbox `cmdRoadmapAnalyze` honors. Checkbox-override vs disk-strict is an undecided product question, not a consolidation | | 3 | **Phase 6 — milestone identity** (§7.2): bind `getMilestoneInfo` to `locateMilestoneHeadings`, widen the windowing guard's token set in the same change (#3171, #3197) | A sixth derivation family. Phase 2 consolidated *windowing*; `getMilestoneInfo` hand-rolls its own heading regexes to answer a different question — *which milestone is this and what is it called* — and no named phase touches it | | 4 | **Phase 7 — completion-ratio scoping** (§7.6 rules 3–4). **Corrected: #3161 is NOT fixed here — Amendment 3 subsumed it.** Phase 7 keeps rule 4 and whatever consumers Phase 3 did not reach | A seventh derivation family. This amendment ships the arithmetic half. Its original claim — that enumeration consolidation "changes nothing here" — was **wrong**, and Phase 3 proved it: routing `cmdStats`/`cmdProgressRender`'s `totalPlans`/`totalSummaries` accumulation through `listMilestonePhaseDirs`'s scoped set *is* §7.6 rule 3 for those two consumers, and it is what closed #3161 | diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 765ca2412..1f02297ae 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -254,19 +254,109 @@ function hasVersionedMilestones(content: string): boolean { return /^#{1,3}\s+.*v\d+\.\d+/mi.test(content); } +// This file's milestone-heading vocabulary: a version token (`v1.2`-style), +// a ✅/🚧/📋 status marker, or the word "Milestone". Tested against a +// non-Phase heading's own text by `hasMilestoneSectioning` below — this +// module's sole owner of "is this heading a milestone heading". +const MILESTONE_HEADING_SIGNAL_PATTERN = /v\d+\.\d+|✅|📋|🚧|\bMilestone\b/i; + /** - * #3184/#2828/#1761: does this ROADMAP use milestone SECTIONING at all — i.e. - * does it carry any non-Phase heading at level 2-3? Deliberately weaker than - * `hasVersionedMilestones`: this needs to distinguish a FLAT unmilestoned - * roadmap (Phase headings only, where a whole-document phase count is - * correct) from a MILESTONED-but-unbounded one (where that count conflates - * sibling milestones, #1761) — that distinction is load-bearing and must not - * be collapsed into the versioned-milestone check. Owned here so the - * milestone heading vocabulary has one home; routes `state.cts`'s - * `buildStateFrontmatter` #2828 guard instead of a third hand-rolled copy. + * #3184/#3204/#2828/#1761/#3185: could a WHOLE-DOCUMENT phase count conflate + * two different milestones? That is the only question `buildStateFrontmatter` + * (`state.cts`) asks its single caller of this predicate. + * + * Three prior models were tried, and all three tried to infer milestone-ness + * from POSITION — where a heading sits relative to other headings — and all + * three broke a real shape because position does not carry it: + * + * 1. "Is there ANY non-Phase level-2/3 heading" (pre-#3184). #3204: a FLAT + * roadmap carrying one ordinary structural heading (`## Progress`) was + * misclassified as milestone-sectioned, and `safeToUseRoadmapCount` + * clobbered a correct ROADMAP-declared count down to the on-disk directory + * count. Not-Phase-ness was never the right question. + * 2. "Do >=2 non-Phase headings EACH own a nested (STRICTLY DEEPER) Phase + * heading" (#3184's rewrite). Two independent review findings broke this: + * (a) #1761 regression — real sibling milestones are commonly at the SAME + * level as their own Phase headings (`## v1.0` / `## Phase 1:` / `## v2.0` + * / `## Phase 3:`), so "strictly deeper" never matches for either sibling + * and the predicate answers false, letting the whole-document count + * conflate them exactly as #1761 did. (b) #3204 reintroduced — the + * bundled greenfield template itself (`gsd-core/templates/roadmap.md:149-171`: + * `## Phases` -> `### 🚧 v1.1 — …` -> `#### Phase 5: …`) nests a Phase + * heading arbitrarily deep under EVERY ancestor in the chain, so a + * generic wrapper heading ("Phases") with no milestone meaning of its own + * counted as its own candidate section and single-milestone documents + * were misclassified as sectioned again. + * 3. "Immediate adjacency, at any level" (interim #3185 rewrite, never + * shipped past this file's own working tree). Fixed both #3184 defects + * above, but adjacency is STILL a positional signal, and #3185 reproduced + * a THIRD shape it cannot see: a flat roadmap where `## Overview` happens + * to sit immediately before `## Phase 1:` and, independently, `## Notes` + * sits immediately before `## Phase 4:` later in the same document. Two + * purely structural headings, zero milestone meaning, each "adjacent" to a + * Phase heading by coincidence of document layout — ≥2 owners, so the + * flat 6-phase roadmap was misclassified as sectioned and clobbered to the + * 2 on-disk phase directories. Same root defect as #3204's `## Progress`, + * wearing a different heading shape. + * + * The model that actually holds for every shape above abandons position + * entirely and asks about the heading's own text: is it a MILESTONE HEADING — + * a non-Phase heading at level 1-3 carrying a milestone VOCABULARY signal + * (a version token, a ✅/🚧/📋 status marker, or the word "Milestone")? + * Sectioning is present iff there are >=2 such headings — one or zero cannot + * conflate siblings by construction, no matter where they sit. This resolves + * every prior failure: + * - #3204 / this file's `## Progress`: no signal — 0 milestone headings. + * - #3185 `## Overview` / `## Notes` interleaved with flat phases: neither + * carries a signal — 0 milestone headings, regardless of adjacency. + * - #1761 same-level siblings (`## v1.0` / `## v2.0`): each carries a version + * token — 2 milestone headings, sectioned, no level or adjacency test + * needed. + * - #1761 unmarked prose siblings (`## Milestone 1: …` / `## Milestone 2: …`): + * each carries the word "Milestone" — 2 milestone headings, sectioned. + * - Bundled template wrapper (`## Phases` -> `### 🚧 v1.1` -> `#### Phase 5:`): + * `## Phases` carries no signal; `### 🚧 v1.1` carries a marker and a + * version token but is only ONE heading — 1 milestone heading, not + * sectioned. + * + * Deliberately NOT a denylist of heading names (fragile, unbounded) and NOT + * collapsed into `hasVersionedMilestones` (a non-versioned-but-marked or + * "Milestone"-named section still conflates siblings — see that function's + * own doc comment, which answers a narrower question: ANY version token + * anywhere, not "are there >=2 independently-signalled milestone headings"). + * Routed through `tokenizeHeadings` (fence- and CRLF-aware, single owner of + * ATX heading tokenisation) rather than a second regex pass, so a heading + * inside a fenced code block is never tokenised in the first place and + * cannot flip this result. The Phase-heading test (`/^Phase\s+\S/i`) is the + * SAME literal reused by `computeMilestoneSectionEnd` / `locateMilestoneHeadings` + * above, not a fresh copy. `MILESTONE_HEADING_SIGNAL_PATTERN`'s version-token + * and marker alternatives mirror the literal fragments already used by + * `hasVersionedMilestones` (`v\d+\.\d+`) and `computeMilestoneSectionEnd` + * (`✅|📋|🚧`) rather than inventing a fourth independent copy of the same + * vocabulary; the "Milestone" word is the one signal none of those three + * needed and this predicate does. + * + * Honest limit: this is a NARROWER signal than any of the three position-based + * attempts — a heading is only a candidate if its OWN TEXT carries a version + * token, a status marker, or the word "Milestone". Two milestone sections that + * carry NONE of the three (e.g. `## First Chapter` / `## Second Chapter`, each + * with their own Phase headings, no version, no marker, no "Milestone" word) + * are not detected as sectioned, and the whole-document count is trusted even + * though it may still conflate them. No fixture in this repo's bundled + * template or the #3204/#1761/#3185 reports exercises that shape; it is + * recorded here rather than hidden. */ function hasMilestoneSectioning(content: string): boolean { - return /^#{2,3}\s+(?!Phase\s+\S)/mi.test(content); + const isPhaseHeading = (text: string): boolean => /^Phase\s+\S/i.test(text); + let milestoneHeadingCount = 0; + for (const heading of tokenizeHeadings(content)) { + if (heading.level < 1 || heading.level > 3) continue; + if (isPhaseHeading(heading.text)) continue; + if (!MILESTONE_HEADING_SIGNAL_PATTERN.test(heading.text)) continue; + milestoneHeadingCount++; + if (milestoneHeadingCount >= 2) return true; + } + return false; } /** diff --git a/src/state.cts b/src/state.cts index 5e605bde7..de5e8317e 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1753,9 +1753,14 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // neither the denominator nor the numerator (mirrors the heading // exclusion below). Project-code-aware via phaseKeyFromDir. if (retiredPhaseNums.size > 0 && retiredPhaseNums.has(phaseKeyFromDir(dir))) continue; - // phase-id-owner: dir-name dedup grouping; diverges from extractPhaseToken/phaseKeyFromDir on project-code-prefixed and multi-segment milestone dirs. Kept local. - const m = dir.match(/^0*(\d+[A-Za-z]?(?:\.\d+)*)/); - const key = m ? m[1].toLowerCase() : dir; + // #3185: dedup grouping routed through the canonical phaseKeyFromDir + // (src/phase-id.cts) instead of a local leading-digits regex that + // diverged from extractPhaseToken/phaseKeyFromDir on + // project-code-prefixed dirs (whole dirname fell through as the key, + // so a `PROJ-05`/`PROJ-05-slug` pair never deduped) and on + // multi-segment milestone dirs. Same key surface used two lines + // above for the retiredPhaseNums exclusion, so both filters agree. + const key = phaseKeyFromDir(dir); if (!seenPhaseNums.has(key)) { seenPhaseNums.set(key, dir); } else { diff --git a/tests/issue-3204-state-writer-phase-count.test.cjs b/tests/issue-3204-state-writer-phase-count.test.cjs new file mode 100644 index 000000000..abcdb542a --- /dev/null +++ b/tests/issue-3204-state-writer-phase-count.test.cjs @@ -0,0 +1,784 @@ +// allow-test-rule: source-text-is-the-product, see #3204 +// Reads STATE.md/ROADMAP.md fixture files whose deployed text IS what the +// runtime loads — testing text content tests the deployed contract. + +/** + * #3204 / #3185 — failing-first regression suite for `buildStateFrontmatter`'s + * `total_phases` selection (`src/state.cts:1620`, guard at `:1795-1805`). + * + * `hasMilestoneSectioning` (`src/roadmap-parser.cts:195`) is + * /^#{2,3}\s+(?!Phase\s+\S)/mi + * — true for ANY non-Phase level-2/3 heading, so a FLAT roadmap carrying an + * ordinary structural heading (`## Progress`, `## Overview`, ...) is + * misclassified as milestone-sectioned. `safeToUseRoadmapCount` then goes + * false and the on-disk phase-directory count silently clobbers the + * ROADMAP-declared count — a regression of #2828, reported in #3204 as + * "roadmap declares 6 phases, 4 directories exist, state.record-session + * writes total_phases: 4". + * + * DO NOT fix src/state.cts or src/roadmap-parser.cts from this file. Rows 2, + * 3, and 14 below assert the CORRECT (post-fix) value and currently FAIL — + * that is the point of a failing-first suite. Every other row asserts + * behavior verified to already hold today (see phase-log for the manual CLI + * probes that established each expected value before this file was written). + * + * Rows and naming follow `.gsd/phase/fix-3185-state-writer-phase-count/50-test-matrix.md` + * verbatim (row numbers refer to that matrix, not the 8-row table in + * `40-design.md`). + * + * Driven via `state record-session` (the shape #3204's own report used), + * then read back with `state json --raw` — the product's own frontmatter + * parser — so `progress.total_phases` is asserted as a NUMBER, never a + * regex over rendered STATE.md text. `tests/helpers.cjs`'s `parseFrontmatter` + * only reads flat top-level keys (it does not descend into the nested + * `progress:` block), so `state json --raw` is the correct structured seam + * for a nested field — it is what `tests/state.test.cjs`'s own '#1761 + * read-path' and 'milestone-scoped phase counting' suites already use for + * this exact assertion shape. + */ + +const { test, describe, 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'); + +// ───────────────────────────────────────────────────────────────────────────── +// Fixture builders +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Seed `.planning/phases/-phase-` for each phase number in `nums`, + * each with a single PLAN.md so the directory is a recognizable phase dir. + */ +function seedPhaseDirs(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'); + } +} + +/** Seed one arbitrarily-named phase directory (sentinel / dup / pre-milestone cases). */ +function seedNamedPhaseDir(tmpDir, dirName, planBase) { + const dir = path.join(tmpDir, '.planning', 'phases', dirName); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, `${planBase}-01-PLAN.md`), '# Plan\n'); +} + +/** + * Build STATE.md frontmatter + minimal body. `milestone` is always set (the + * #3204 fixture needs it truthy — `getMilestoneInfo` defaults an absent + * `milestone:` field to 'v1.0' anyway, so this pins the same value + * explicitly for every row rather than relying on that fallback). + * Lines are joined with the caller-supplied `eol` (default '\n') — row 14 + * reuses this to build the CRLF variant without a second copy. + */ +function buildStateMd({ milestone = 'v1.0', milestoneName = 'Test', totalPhases, currentPhase = '01', eol = '\n' }) { + const lines = [ + '---', + 'gsd_state_version: 1.0', + `milestone: ${milestone}`, + `milestone_name: ${milestoneName}`, + `current_phase: "${currentPhase}"`, + 'status: executing', + 'progress:', + ` total_phases: ${totalPhases}`, + ' completed_phases: 0', + ' total_plans: 0', + ' completed_plans: 0', + ' percent: 0', + '---', + '', + '# GSD State', + '', + '## Current Position', + '', + `**Current Phase:** ${currentPhase}`, + '**Status:** Executing', + '', + ]; + return lines.join(eol); +} + +/** Invoke `state record-session` (the #3204 entry point) then read back `state json --raw`. */ +function recordSessionAndReadTotalPhases(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 JSON.parse(jsonResult.output); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Rows 1-3, 14 — #3204 regression: flat roadmap + a structural heading +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3204 buildStateFrontmatter total_phases — flat roadmap misclassified as milestone-sectioned', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('flat roadmap with no structural headings keeps the roadmap count', () => { + // Row 1 (happy path / control) — no non-Phase heading anywhere, so + // hasMilestoneSectioning is false today and this already passes. Guards + // against a fix that overcorrects and breaks the trivial flat case. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual(Number(out.progress.total_phases), 6, `expected roadmap count 6, got ${out.progress && out.progress.total_phases}`); + }); + + test('#3204 flat roadmap with a Progress heading is not treated as milestone-sectioned', () => { + // Row 2 — the crux repro, transcribed from #3204's own report: 6 + // declared phases, 4 directories, a flat '## Progress' heading. FAILS + // TODAY: hasMilestoneSectioning misclassifies '## Progress' as + // sectioning, safeToUseRoadmapCount goes false, and the write clobbers + // total_phases down to the disk count (4) instead of 6. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + '## Progress', + '', + 'Some progress notes.', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 6, + `#3204: total_phases must stay 6 (roadmap-declared), not clobber to the disk count of 4. Got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('#3204 multiple structural headings still count as flat', () => { + // Row 3 — same shape as row 2 with TWO structural headings ('## Overview', + // '## Phase Details'); '## Phase Details' is correctly excluded by the + // heading's own '(?!Phase\s+\S)' lookahead, but '## Overview' still trips + // the misclassification. FAILS TODAY for the same reason as row 2. + const roadmap = [ + '# Roadmap', + '', + '## Overview', + '', + 'Some overview text.', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + '## Phase Details', + '', + 'More detail prose.', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 6, + `#3204: multiple structural headings must still count as flat (6), got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('#3204 repro under CRLF', () => { + // Row 14 — row 2's exact repro, all fixture content authored with CRLF + // line endings, proving the bug (and required fix) is not an artifact of + // LF-only fixtures. FAILS TODAY for the same reason as row 2. + const roadmapLines = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + '## Progress', + '', + 'Some progress notes.', + '', + ]; + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmapLines.join('\r\n')); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ totalPhases: 6, eol: '\r\n' }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 6, + `#3204 under CRLF: total_phases must stay 6, got ${out.progress && out.progress.total_phases}`, + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Rows 4-11 — negative space and boundaries (must hold both before and after the fix) +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3204 buildStateFrontmatter total_phases — negative space / boundaries (must not regress)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('bounded milestone uses its own section count', () => { + // Row 4 — versioned roadmap with two sibling milestone sections ('## v1.0' + // owning phases 1-2, '## v2.0' owning phases 3-5). The asserted milestone + // ('v2.0') IS bound to its own heading, so `sliceMilestoneWindow`/ + // `extractCurrentMilestoneScoped` narrow to that section and + // roadmapPhaseCount is the SECTION's count (3), not the whole-document + // count (5) and not the disk count (2 dirs seeded). + const roadmap = [ + '# Roadmap', + '', + '## v1.0', + '## Phase 1: One', + '## Phase 2: Two', + '', + '## v2.0', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ milestone: 'v2.0', milestoneName: 'Second', totalPhases: 3 }), + ); + seedPhaseDirs(tmpDir, [3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 3, + `a bounded milestone must use its own section's phase count (3), not the whole document (5) or the disk count (2). Got ${out.progress && out.progress.total_phases}`, + ); + }); + + 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. + const roadmap = [ + '# Roadmap', + '', + '## v2.0', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '', + ].join('\n'); + 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 }), + ); + 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}`, + ); + }); + + test('#1761 sibling milestone sections still fall back to the disk count', () => { + // 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. + const roadmap = [ + '# Roadmap', + '', + '## Milestone 1: First Milestone', + '### Phase 1: a', + '### Phase 2: b', + '### Phase 3: c', + '### Phase 4: d', + '', + '## Milestone 2: Second Milestone', + '### Phase 5: e', + '### Phase 6: f', + '### Phase 7: g', + '### Phase 8: h', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ milestone: 'v3.0', milestoneName: 'Third', totalPhases: 8 }), + ); + seedPhaseDirs(tmpDir, [1, 2, 3]); + + 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}`, + ); + }); + + test('zero phase directories keeps the declared count', () => { + // Row 7 — boundary limit-1: 0 dirs vs 6 declared. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + // No phase dirs seeded. + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual(Number(out.progress.total_phases), 6, `expected 6, got ${out.progress && out.progress.total_phases}`); + }); + + test('equal counts agree', () => { + // Row 8 — boundary limit: 6 dirs vs 6 declared. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4, 5, 6]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual(Number(out.progress.total_phases), 6, `expected 6, got ${out.progress && out.progress.total_phases}`); + }); + + test('extra directories win via max()', () => { + // Row 9 — boundary limit+1. 6 heading-declared phases + a 7th phase + // declared only via the bullet-entry syntax ('- [ ] **Phase 7 — Extra**', + // #2199 bullet house style), which the directory-membership filter + // counts but the heading-only roadmapPhaseCount scan does not — so disk + // (7, all pass the membership filter) legitimately exceeds the + // heading-only roadmap count (6), and max() must pick 7. + // + // NOTE: a naive "N heading-declared phases + N+1 plain directories" does + // NOT exercise this path — the directory-membership filter + // (getMilestonePhaseFilter, roadmap-parser.cts) excludes any directory + // whose phase number has no matching roadmap entry at all, so an + // out-of-roadmap directory number is silently dropped from the disk + // count rather than inflating it. Verified against the running CLI + // before authoring this fixture. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + '- [ ] **Phase 7 — Extra**', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 6 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4, 5, 6, 7]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 7, + `expected max(7 dirs, 6 heading-declared) = 7, got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('absent roadmap falls back to disk', () => { + // Row 10 — no ROADMAP.md at all. + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 4 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual(Number(out.progress.total_phases), 4, `expected disk count 4, got ${out.progress && out.progress.total_phases}`); + }); + + test('roadmap with no phase headings falls back to disk', () => { + // Row 11 — ROADMAP.md present but zero Phase headings anywhere. + const roadmap = ['# Roadmap', '', '## Notes', '', 'No phases declared yet.', ''].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 4 })); + seedPhaseDirs(tmpDir, [1, 2, 3, 4]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual(Number(out.progress.total_phases), 4, `expected disk count 4, got ${out.progress && out.progress.total_phases}`); + }); + + test('a phase heading carrying a version token is not a milestone heading', () => { + // Row 12 (hostile, negative space) — '### Phase 3: Ship v2.0 gaps' carries + // a version token in its own text, but hasMilestoneSectioning's + // isPhaseHeading check excludes any heading matching '^Phase\s+\S' before + // the vocabulary signal is ever tested, so this must NOT count as a + // milestone heading. Otherwise-flat roadmap, so the roadmap-declared + // count must be used, not the disk count. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '### Phase 3: Ship v2.0 gaps', + '## Phase 4: Four', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 4 })); + seedPhaseDirs(tmpDir, [1, 2]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 4, + `a version token borne by a Phase heading must not trigger milestone sectioning; expected roadmap count 4, got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('a version heading inside a fence is not sectioning', () => { + // Row 13 (hostile, negative space) — '## v2.0' appears only inside a + // fenced code block (a documentation example of the heading syntax) on an + // otherwise flat roadmap. hasMilestoneSectioning is routed through + // tokenizeHeadings (fence-aware), so a fenced heading is never tokenised + // and must NOT count as sectioning. The roadmap-declared count must be + // used, not the disk count. + const roadmap = [ + '# Roadmap', + '', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Phase 4: Four', + '', + 'Example heading syntax:', + '', + '```', + '## v2.0', + '```', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 4 })); + seedPhaseDirs(tmpDir, [1, 2]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 4, + `a version heading inside a fence must not trigger milestone sectioning; expected roadmap count 4, got ${out.progress && out.progress.total_phases}`, + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #3185 adversarial review — hasMilestoneSectioning ownership-model shapes +// missed by the original suite (BLOCKER + MAJOR findings against the #3184 +// "strictly-deeper nesting" rewrite). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3185 review — hasMilestoneSectioning shapes the original suite missed', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('BLOCKER: same-level sibling milestones fall back to the disk count', () => { + // 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 + // milestones are frequently at the SAME level as their own Phase + // 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. + const roadmap = [ + '# Roadmap', + '', + '## v1.0', + '## Phase 1: One', + '## Phase 2: Two', + '', + '## v2.0', + '## Phase 3: Three', + '## Phase 4: Four', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ milestone: 'v3.0', milestoneName: 'Third', totalPhases: 4 }), + ); + seedPhaseDirs(tmpDir, [1, 2]); + + 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}`, + ); + }); + + test('#3185 repro: structural headings interleaved among flat phases keep the roadmap count', () => { + // #3185's own reproduction of the adjacency model this suite's + // predecessor shipped: '## Overview' sits immediately before + // '## Phase 1:' and '## Notes' sits immediately before '## Phase 4:', + // giving an adjacency-based predicate 2 "owning" candidates even though + // neither heading carries any milestone vocabulary (no version token, no + // status marker, no "Milestone" word) and the roadmap is genuinely flat. + const roadmap = [ + '# Roadmap', + '', + '## Overview', + '## Phase 1: One', + '## Phase 2: Two', + '## Phase 3: Three', + '## Notes', + '## Phase 4: Four', + '## Phase 5: Five', + '## Phase 6: Six', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + buildStateMd({ milestone: 'v9.9', milestoneName: 'Test', totalPhases: 6 }), + ); + seedPhaseDirs(tmpDir, [1, 2]); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 6, + `#3185: structural headings adjacent to phase headings must not be treated as milestone sectioning; expected roadmap count 6, got ${out.progress && out.progress.total_phases}`, + ); + }); + + 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. + const roadmap = [ + '# Roadmap', + '', + '## Phases', + '', + '### 🚧 v1.1 [Name] (In Progress)', + '', + '#### Phase 1: One', + '#### Phase 2: Two', + '#### Phase 3: Three', + '#### Phase 4: Four', + '', + ].join('\n'); + 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 }), + ); + 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}`, + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Rows 15-18 — #3185 consolidation independence checks +// +// These exercise the directory-enumeration owner (listMilestonePhaseDirs / +// getMilestonePhaseFilter), not hasMilestoneSectioning. Verified PASSING +// against the current build (manual CLI probe) before being added here — +// included per the dispatch brief's "include only if they pass today" +// condition. If a future change to the #3204 fix regresses one of these, +// that is a SEPARATE finding from the #3204 repro above, not folded into it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3185 buildStateFrontmatter total_phases — directory-enumeration independence (currently passing)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('sentinel directories are excluded by the canonical enumeration', () => { + // Row 15 — a 999.x backlog directory alongside 3 real phase directories + // must not inflate total_phases. + const roadmap = ['# Roadmap', '', '## Phase 1: One', '## Phase 2: Two', '## Phase 3: Three', ''].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 3 })); + seedPhaseDirs(tmpDir, [1, 2, 3]); + seedNamedPhaseDir(tmpDir, '999.1-backlog-idea', '999.1'); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 3, + `sentinel 999.x directory must be excluded, expected 3, got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('pre-milestone directories are excluded', () => { + // Row 16 — a '0-*' pre-milestone directory must not be counted. + const roadmap = ['# Roadmap', '', '## Phase 1: One', '## Phase 2: Two', '## Phase 3: Three', ''].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 3 })); + seedPhaseDirs(tmpDir, [1, 2, 3]); + seedNamedPhaseDir(tmpDir, '0-premilestone', '0'); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 3, + `pre-milestone '0-*' directory must be excluded, expected 3, got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('duplicate phase-number directories count once', () => { + // Row 17 — two directories both keyed to phase number 2 must dedup to a + // single count. + const roadmap = ['# Roadmap', '', '## Phase 1: One', '## Phase 2: Two', '## Phase 3: Three', ''].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 3 })); + seedPhaseDirs(tmpDir, [1, 2, 3]); + seedNamedPhaseDir(tmpDir, '02-phase-2-dup', '02'); + + const out = recordSessionAndReadTotalPhases(tmpDir); + assert.strictEqual( + Number(out.progress.total_phases), + 3, + `duplicate phase-2 directories must dedup to a single count (3), got ${out.progress && out.progress.total_phases}`, + ); + }); + + test('re-running record-session does not move total_phases', () => { + // Row 18 — idempotence: a second record-session call over an unchanged + // tree, with the clock pinned so 'Last session' does not itself vary, + // must produce a byte-identical STATE.md. + const roadmap = ['# Roadmap', '', '## Phase 1: One', '## Phase 2: Two', '## Phase 3: Three', ''].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + const sessionState = [ + '# GSD State', + '', + '## Session', + '', + '**Last session:** 2024-01-01T00:00:00.000Z', + '**Stopped at:** None', + '**Resume file:** None', + '', + '## Current Position', + '', + '**Current Phase:** 01', + '**Status:** Executing', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), sessionState); + seedPhaseDirs(tmpDir, [1, 2, 3]); + + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const pinnedEnv = { GSD_TEST_MODE: '1', GSD_NOW_MS: '1600000000000' }; + const args = ['state', 'record-session', '--stopped-at', 'Phase 1, Plan 1', '--resume-file', 'none']; + + const first = runGsdTools(args, tmpDir, pinnedEnv); + assert.ok(first.success, `first record-session failed: ${first.error}`); + const afterFirst = fs.readFileSync(statePath, 'utf8'); + + const second = runGsdTools(args, tmpDir, pinnedEnv); + assert.ok(second.success, `second record-session failed: ${second.error}`); + const afterSecond = fs.readFileSync(statePath, 'utf8'); + + assert.strictEqual( + afterSecond, + afterFirst, + 're-running record-session on an unchanged tree with a pinned clock must produce a byte-identical STATE.md', + ); + }); +});