diff --git a/.changeset/tidy-lynx-sing.md b/.changeset/tidy-lynx-sing.md new file mode 100644 index 000000000..e2f55aec7 --- /dev/null +++ b/.changeset/tidy-lynx-sing.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3226 +--- +**Milestone names are no longer truncated at a parenthesis, and a phase heading is never mistaken for the milestone** — a ROADMAP whose `### Phase N` heading mentioned a version could cause a wrong `milestone:` to be written to `STATE.md`, and a milestone named `v3.3 — Portability (Windows)` was recorded and rendered as `Portability`. Milestone identity now has one implementation; when it cannot be determined it is reported as absent instead of defaulting to a plausible-looking `v1.0`/`milestone`. (#3216) diff --git a/CONTEXT.md b/CONTEXT.md index e11bf76f9..9a84e2aed 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 `(?![\w.-])` so `v2.0` does not match inside `v2.0.1` — `\b` does, because `.` is a non-word character), `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`). `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/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 9e584f39c..f71eabc1f 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -226,7 +226,39 @@ Before this field existed, all four cases produced the same well-formed milestone from a scoping failure. Branch on `scope`, not on `phase_count` alone. A ROADMAP with no versioned milestone headings at all (the free-form legacy -shape) reports `complete`: the whole document *is* the milestone there. +shape) reports `complete`: the whole document *is* the milestone there. Note +this answer is specific to *windowing* — see the next section for why milestone +*identity* answers the same document differently. + +### Milestone identity (which milestone, and what it is called) + +Milestone identity — the version and name behind `STATE.md`'s `milestone:` +field, `roadmap analyze`'s `milestones[]` array, and the milestone shown by +`query progress`, `stats`, `init manager`, `validate health` and +`workstream create` — is resolved by one implementation: + +- `STATE.md`'s `milestone:` field selects the version when present. The ROADMAP + heuristics are the fallback, not the primary. +- The heading is located by the same canonical locator that computes the + milestone window, so a `### Phase N: …` heading is **never** read as the + milestone heading — even when it mentions a version. Previously a ROADMAP + whose phase heading preceded its milestone heading could write a wrong + `milestone:` to disk. +- The **name** is the heading text after that heading's own version token, with + one leading delimiter (`—`, `–`, `:`, `-`) and any trailing `✅`/`📋`/`🚧` + marker removed. Parentheses are ordinary characters: a milestone named + `v3.3 — Portability (Windows)` keeps its full name rather than being cut at + the `(`. +- When identity **cannot** be determined it is reported as absent rather than + defaulted. A free-form legacy ROADMAP with no version anywhere is `unscoped` + with no identity — unlike windowing above, there is no version token to + report, and inventing one would be indistinguishable from a real answer. + +Two consumers act on that distinction rather than just displaying it: +`state sync` / `state record-session` write `null` instead of a fabricated +`milestone:`/name, and `phases clear` falls back to its dated archive label +(`archived-`) instead of filing phase history under a fabricated +`milestones/-phases/` directory. ### `milestone complete` refuses an untrustworthy window diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 5e6a40f36..95a4fe9bb 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -656,6 +656,14 @@ Show status, next steps, and automatically advance to the next logical workflow | `--do "task description"` | Analyze freeform intent and dispatch to the most appropriate GSD command | | `--forensic` | Append a 6-check integrity audit after the standard report (STATE consistency, orphaned handoffs, deferred scope drift, memory-flagged pending work, blocking todos, uncommitted code) | +> **Milestone name and version.** The milestone this report shows comes from one +> implementation shared with `/gsd-stats`, `/gsd-manager` and `roadmap analyze`. +> A name is no longer cut short at a parenthesis (`v3.3 — Portability (Windows)` +> keeps its full name), a `### Phase N:` heading that mentions a version is never +> mistaken for the milestone heading, and a milestone that cannot be identified is +> shown as absent rather than as a plausible-looking `v1.0`/`milestone`. See +> [CLI-TOOLS.md → Milestone identity](CLI-TOOLS.md#milestone-identity-which-milestone-and-what-it-is-called). + **Auto-routing behavior (`--next`):** - No project → suggests `/gsd-new-project` - Phase needs discussion → runs `/gsd-discuss-phase` diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index da703462b..37fc7af10 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -244,19 +244,21 @@ This section is that written rule. It is **normative**, and it is what the guard **Guard.** `scripts/lint-milestone-window-drift.cjs`. -#### 7.2 Milestone identity — *Required — Phase 6* +#### 7.2 Milestone identity — *Enforced (Phase 6, #3216)* **Question.** Which milestone is current, and what is it called? -**Owner (to be).** `getMilestoneInfo` binds to `locateMilestoneHeadings` and deletes its own heading regexes. It is a **sixth derivation family** — the coverage audit's gap 2 — that no phase of the original decomposition touches. +**Owner.** `src/roadmap-parser.cts` — `getMilestoneInfo`, returning `ScopedResult`, plus the version-agnostic `listMilestoneHeadings` added by Phase 6 for consumers that need *every* milestone heading rather than one. Both consume a single shared grammar source; `locateMilestoneHeadings` is a **version-filtered view** over that source, not a second expression of it. It is a **sixth derivation family** — the coverage audit's gap 2 — that no phase of the original decomposition touched. **Rule.** 1. `STATE.md`'s `milestone:` field selects the version when present; the ROADMAP heuristics are the fallback, not the primary. 2. The heading is located by the canonical locator of §7.1, which already excludes phase headings. A `### Phase N: Close v3.3 gaps` heading is **never** the milestone heading (#3197 — reproduced live, writing a wrong `milestone:` to disk). -3. The **name** is the heading text following the version token with a leading delimiter (`—`, `–`, `:`, `-`) stripped. `(` is an ordinary name character: the name is **not** truncated at a parenthetical (#3171). +3. The **name** derives from the heading's **own** version token, not the requested one: remove everything up to and including that token, strip one leading delimiter (`—`, `–`, `:`, `-`), then strip any trailing run of the status markers `✅`/`📋`/`🚧`. `(` is an ordinary name character: the name is **not** truncated at a parenthetical (#3171). *(Requested `v2.0` against heading `## v2.0.1 — Portability` yields exactly `Portability`, not `.1 — Portability`; `## v2.0 — Old ✅` yields `Old`, because the shipped state is already carried structurally and must not be duplicated into the name.)* 4. A failure returns a `scope` other than `COMPLETE`. It does **not** return `{version: 'v1.0', name: 'milestone'}` presented as an answer — that default is output-identical to a successful read of a genuine `v1.0` project, which is this epic's defining failure mode. +5. **A free-form legacy ROADMAP carrying no version token anywhere is `UNSCOPED` with no identity** — *not* `COMPLETE`, and not a defaulted `v1.0`. §7.1's "free-form is `COMPLETE`" governs *windowing*, where whole-document genuinely is the window; identity has no version to report and must not invent one. **Decided 2026-08-08** (maintainer), closing the gap this section previously left unstated. **Corollary, stated because it is a distinct case and was initially left implicit:** a bare version token appearing only in prose or in a non-milestone heading — no `milestone:` field, no milestone heading — is weak but *real* evidence, and yields `TRUNCATED` with that version and a `null` name under rule 6, not `UNSCOPED`. `UNSCOPED` is reserved for a document carrying no version token at all. +6. A version known but no name resolvable is `TRUNCATED` carrying `{version, name: null}` — the version is a real answer, the name is a non-answer, and collapsing the two is the failure this contract exists to prevent. -**Guard.** `lint-milestone-window-drift.cjs` today keys on the `#{N,M}` heading-level quantifier; `getMilestoneInfo`'s regexes anchor on a literal `##` and therefore slip past it. **Phase 6 ships the token widening together with the consolidation**, never after — a guard added later measures a surface already cleaned and reports a zero it did not earn. +**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* @@ -369,7 +371,7 @@ One row per derivation. A blank owner is a derivation whose contract is locked ( | Derivation | Owner | Guard | Scan surface | Status | |---|---|---|---|---| | Milestone windowing (§7.1) | `roadmap-parser.cts` | `lint-milestone-window-drift.cjs` | `src/` | enforced | -| Milestone identity (§7.2) | — (Phase 6) | same guard, token set widened by Phase 6 | `src/` | contract only | +| 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 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 | @@ -662,3 +664,74 @@ underlying document-layout ambiguity is untouched by design. #3163 belongs to #2 layer and is not enumerated there yet; #3169 and #3170 are standalone parser/extraction defects with no epic home. Recording them here as *not covered* rather than leaving them to be re-tested by the next audit. + +### Amendment 4 — Phase 6 (#3216) validation: the contract held; the blessed implementation carried the defect again + +**The copy count was a lower bound for the FOURTH consecutive time.** The epic and §7.2 both scoped +this phase at one copy — `getMilestoneInfo`. The guard, built and run *before* the scope was fixed +per Amendment 3's standing rule, found **three**: + +| # | Site | Carries | +|---|---|---| +| 1 | `roadmap-parser.cts:785` — `^##[^\n]*${escapedVer}…` | #3171 + #3197 | +| 2 | `roadmap-parser.cts:806` — `/## (?!.*✅).*v(\d+(?:\.\d+)+)…/` | #3171 + #3197 | +| 3 | **`roadmap.cts:454`** — `cmdRoadmapAnalyze`'s own milestone enumeration | #3171 + #3197 | + +Site 3 is the notable one, and it repeats §7.6's finding exactly: **the implementation this epic +blessed carried the defect it was blessed over.** `cmdRoadmapAnalyze` is named in Decision 1 as the +correct enumeration implementation, and its `milestones[]` array was simultaneously truncating names +at a parenthetical and emitting `### Phase N` headings as milestones. Twice now the blessing has +been granted per-question rather than per-file, and twice the blessed file has held an unrelated +copy. **Blessing an implementation for one derivation says nothing about its others.** + +**Two mechanisms, not one.** Site 1 is line-anchored but level-blind (`^##` then `[^\n]*` absorbs a +third `#`); site 2 and site 3 have no anchor at all, so `## ` matches from the *second* `#` of +`###`. A reviewer checking "is it anchored?" would have passed site 1. This is why §7.2 rule 2 names +the canonical locator rather than describing the anchoring to be re-implemented. + +**Two under-specifications surfaced during implementation, both now closed in §7.2 rule 3.** Neither +was reachable by reading the contract alone; both appeared only when tests demanded an exact value. +(a) *Which* version token the name is measured from, when the requested version is a prefix of the +heading's own — `v2.0` against `## v2.0.1 — Portability` left `.1 — Portability` under the original +wording. (b) A trailing `✅` was being retained *in the name*, duplicating structural state into a +string that `buildStateFrontmatter` writes to disk. Both are now stated normatively rather than +settled inside the implementation. + +**§7.2's unstated case is now decided (rule 5).** Free-form legacy ROADMAP with no version anywhere +→ `UNSCOPED`, no identity. §7's own rule — *"a behavior not stated here is not decided"* — held: the +gap was raised and decided by the maintainer before implementation rather than resolved silently. + +**A latent defect was found in a caller, not by the guard.** `init.cts` carried +`getMilestoneInfo(cwd) as unknown as Record`. The cast masked the return-type change +across five call sites, so `tsc` stayed green while every one of them would have read `undefined`, +and one template would have rendered the literal string `"undefined"`. A structural drift guard +cannot see this class — an unsafe cast is not a re-derivation — which is the concrete argument for +Decision 4(b)'s pairing: the type checker was the output metric and it was green; migrating and +running the consumers was the outcome metric. + +**Tier-2 output changes (Decision 3).** `state sync` / `state record-session` persist a corrected +`milestone:` and name, or `null` rather than a fabricated identity; `roadmap analyze`'s `milestones[]` +no longer truncates names and no longer lists phase headings; `phases clear` falls back to its dated +archive label rather than misfiling under a fabricated version; `query progress`, `stats`, +`init manager`, `validate health` and `workstream create` render the full name. + +Five further Tier-2 surfaces were missed in this amendment's first draft and are recorded here after +the Phase 6 spec review caught the omission — Decision 3 requires *every* Tier-2 change be called +out, and an incomplete list is the same defect in miniature that this epic exists to remove: + +- **`commit`** (`cmdCommit`) — when `branching_strategy` is `milestone`, the constructed branch name + now derives from a scope-checked identity. A `COMPLETE` identity produces the same branch name as + before; a `TRUNCATED` one (real version, unresolved name) still produces a branch, deliberately, + because the version is real — the acceptance is now explicit in code rather than incidental to a + truthiness check. Contrast `archivePhaseDirectories`, which demands `COMPLETE` because it uses the + value as a filesystem path component. +- **The four `init` JSON bundles** — `init execute-phase`, `init new-milestone`, `init milestone-op` + and `init progress` — emit `milestone_version` / `milestone_name` / `current_milestone` as an + explicit `null` when identity is unavailable, where they previously carried the fabricated + `v1.0` / `milestone`. The keys are always PRESENT so the prompt layer cannot render a bare + placeholder; `JSON.stringify` had been dropping them when the value went `undefined`. + +**Guard.** The owner file stays scanned; `listMilestoneHeadings` joins `FUNCTION_SCOPED_EXEMPTIONS` +as a *named canonical function* — the only sanctioned exemption form. `locateMilestoneHeadings` was +refactored into a version-filtered view over one shared grammar source so the two primitives cannot +drift, with a parity test asserting the filtered enumeration equals the locator's selection. diff --git a/scripts/lint-milestone-window-drift.cjs b/scripts/lint-milestone-window-drift.cjs index d5474838f..7d54c0c0d 100644 --- a/scripts/lint-milestone-window-drift.cjs +++ b/scripts/lint-milestone-window-drift.cjs @@ -129,6 +129,41 @@ const VERSION_TOKEN_RE = /v\(?\\{1,2}d\+\)?\\{1,2}\.\\{1,2}d\+/; // prose-matching code); together, on one line, they are the boundary shape. const MARKER_EMOJI_RE = /[✅📋🚧]/u; +// (a-bis) #3216: a LITERAL markdown heading anchor — `##`, `###`, … — as +// opposed to the `#{N,M}` quantifier token (a) above. `getMilestoneInfo` +// hand-rolled its milestone-heading match with a literal `^##` / `## ` rather +// than a quantifier, so token (a) alone reported a clean zero on a file that +// carried two live re-derivations (#3171, #3197). The negative lookahead for +// `{` keeps this from double-matching the quantifier form. +// +// A `#` run is far more common in source than `#{N,M}` (private-field sigils, +// colour literals, fragment URLs, prose), so this token is admitted ONLY +// inside a heading-MATCHER literal — a regex literal, or a string/template +// literal handed to `new RegExp(` — never a bare line match. A template that +// BUILDS a heading for output (`## ${version}`) is not a re-derivation of +// where a milestone's section begins, and conflating the two would flag every +// heading writer in the tree. +const LITERAL_HEADING_RUN_RE = /#{2,6}(?!\{)/; + +// A line that constructs a regex from a string/template literal, which is what +// admits the `new RegExp(`^##…${escapedVer}…`)` shape while leaving ordinary +// heading-building templates alone. +const NEW_REGEXP_RE = /new\s+RegExp\s*\(/; + +// (b2-bis) #3216: the `v(\d+(?:\.\d+)+)` version shape — a capturing group +// around the major, then a NON-capturing `(?:\.\d+)+` repeat. VERSION_TOKEN_RE +// cannot see it: after `v(` + `\d+` it requires a backslash next, and this +// shape has `(` there instead. +const VERSION_TOKEN_GROUPED_RE = /v\(\\{1,2}d\+\(\?:\\{1,2}\.\\{1,2}d\+\)\+\)/; + +// (b3) #3216: an INTERPOLATED version placeholder — `${escapedVer}`, +// `${escapedVersion}`, `${version}`. A regex that interpolates its version +// spells no literal `v\d+\.\d+` anywhere, so (b) could never fire. Inside a +// heading-matcher literal, "a heading anchor plus an interpolated version" IS +// the milestone-heading shape the canonical `locateMilestoneHeadings` +// composes — and so is a copy of it. +const INTERPOLATED_VERSION_RE = /\$\{[A-Za-z0-9_.]*[Vv]er[A-Za-z0-9_.]*\}/; + // Authored TypeScript source only (the generated bin/lib/*.cjs mirror it). const SCAN_DIRS = ['src']; const SCAN_EXT = new Set(['.cts', '.ts', '.mts']); @@ -192,12 +227,26 @@ const OWNER_FILE = path.join('src', 'roadmap-parser.cts'); // `(?!Phase...)`/marker alternations to find "the next milestone // boundary" while assembling the current-milestone window) — not // re-derivations of a question answered elsewhere. +// - roadmap-parser.cts listMilestoneHeadings: #3216 (epic #3180 §7.2's +// Scope amendment) — the version-AGNOSTIC sibling of +// `locateMilestoneHeadings`, and the function that textually DEFINES +// `MILESTONE_HEADING_LINE_SOURCE` (the one shared grammar constant both +// it and `locateMilestoneHeadings` build their pattern from) in its own +// source span. It is a named canonical function defining the grammar, +// not a copy of it — replacing the third independent re-derivation the +// widened guard found at `roadmap.cts:454`. const FUNCTION_SCOPED_EXEMPTIONS = new Map([ [path.join('src', 'roadmap-command-router.cts'), new Set(['checkW021'])], [path.join('src', 'verify.cts'), new Set(['checkMilestonePrefixMismatches'])], [ OWNER_FILE, - new Set(['isMilestoneShippedInRoadmap', 'locateMilestoneHeadings', 'hasMilestoneSectioning', 'extractCurrentMilestoneScoped']), + new Set([ + 'isMilestoneShippedInRoadmap', + 'locateMilestoneHeadings', + 'listMilestoneHeadings', + 'hasMilestoneSectioning', + 'extractCurrentMilestoneScoped', + ]), ], ]); @@ -273,6 +322,33 @@ function stripComments(line) { * text — comment-stripping is applied only to the detection decision, never * to the reported fragment. */ +/** + * All heading-MATCHER literals on `line` — a regex literal always counts; a + * quoted/backtick string literal counts only when `code` (the comment-stripped + * line passed in from the caller) constructs a regex via `new RegExp(`. A + * plain string/template literal that is not fed to `new RegExp(` is not a + * matcher — most commonly a heading BUILT for output, not one matched against. + */ +function headingMatcherLiterals(line, code) { + const out = []; + const allowStrings = NEW_REGEXP_RE.test(code); + for (let i = 0; i < line.length; i++) { + const ch = line[i]; + let literal = null; + let isRegex = false; + if (ch === '/') { + literal = readRegexLiteralAt(line, i); + isRegex = true; + } else if (ch === "'" || ch === '"' || ch === '`') { + literal = readStringLiteralAt(line, i); + } + if (!literal) continue; + if (isRegex || allowStrings) out.push(literal.text); + i = literal.end - 1; + } + return out; +} + function extractFragment(line) { for (let i = 0; i < line.length; i++) { const ch = line[i]; @@ -280,7 +356,7 @@ function extractFragment(line) { if (ch === '/') literal = readRegexLiteralAt(line, i); else if (ch === "'" || ch === '"' || ch === '`') literal = readStringLiteralAt(line, i); if (!literal) continue; - if (HEADING_QUANTIFIER_RE.test(literal.text)) return literal.text; + if (HEADING_QUANTIFIER_RE.test(literal.text) || LITERAL_HEADING_RUN_RE.test(literal.text)) return literal.text; i = literal.end - 1; // resume scanning just past this literal } return line.trim().slice(0, MAX_REGEX_LITERAL_LEN); @@ -305,8 +381,16 @@ function findMilestoneWindowDrift(text, relPath) { const code = stripComments(line); if (!code.trim()) continue; - if (!HEADING_QUANTIFIER_RE.test(code)) continue; - const isMilestoneWindowToken = PHASE_LOOKAHEAD_RE.test(code) || (VERSION_TOKEN_RE.test(code) && MARKER_EMOJI_RE.test(code)); + const matcherLiterals = headingMatcherLiterals(line, code); + const hasQuantifier = HEADING_QUANTIFIER_RE.test(code); + const hasLiteralHeading = matcherLiterals.some((t) => LITERAL_HEADING_RUN_RE.test(t)); + if (!hasQuantifier && !hasLiteralHeading) continue; + + const anyVersionToken = VERSION_TOKEN_RE.test(code) || VERSION_TOKEN_GROUPED_RE.test(code); + const isMilestoneWindowToken = + PHASE_LOOKAHEAD_RE.test(code) + || (anyVersionToken && MARKER_EMOJI_RE.test(code)) + || (hasLiteralHeading && (anyVersionToken || INTERPOLATED_VERSION_RE.test(code))); if (!isMilestoneWindowToken) continue; if (exemptFunctions && exemptFunctions.has(currentFunction)) continue; @@ -372,4 +456,8 @@ module.exports = { readStringLiteralAt, extractFragment, stripComments, + LITERAL_HEADING_RUN_RE, + VERSION_TOKEN_GROUPED_RE, + INTERPOLATED_VERSION_RE, + headingMatcherLiterals, }; diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index dcef479a6..9ba37d9ee 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -37,10 +37,11 @@ "milestone-helper.test.cjs", "milestone-prefixed-convention.test.cjs", "milestone-summary.test.cjs", + "milestone-window-drift-guard.test.cjs", "milestone-window-single-owner.test.cjs", "milestone.test.cjs" ], - "issue": "TBD" + "issue": "3216" }, "roadmap": { "files": [ diff --git a/src/commands.cts b/src/commands.cts index 5054e99bb..defed4981 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -29,6 +29,9 @@ const { getArchivedPhaseDirs, findPhaseInternal, listMilestonePhaseDirs } = phas import roadmapParserMod = require('./roadmap-parser.cjs'); const { extractCurrentMilestone, stripShippedMilestones: _stripShippedMilestones, getMilestoneInfo, getRoadmapPhaseInternal } = roadmapParserMod; // eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports import modelResolverMod = require('./model-resolver.cjs'); const { resolveModelInternal, resolveModelForTier, resolveProviderEscalation, resolveEffortInternal, resolveFastModeInternal, resolveEffortForTier, resolveGranularityInternal, assertValidGranularityOverride } = modelResolverMod; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -864,7 +867,19 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u } } } else if (branchingStrategy === 'milestone') { - const milestone = getMilestoneInfo(cwd); + const milestoneInfo = getMilestoneInfo(cwd); + // #3216 review Finding 3: explicit scope gate instead of plain truthiness. + // COMPLETE and TRUNCATED both carry a real `version` (ADR-3180 §7.2 rule + // 6 — TRUNCATED means the version resolved but the milestone's NAME did + // not), so a TRUNCATED identity is acceptable here: `milestone.version` + // only feeds a BRANCH NAME, and `generateSlugInternal(null) || 'milestone'` + // already degrades the missing name to the literal "milestone" slug on + // purpose. This differs from `archivePhaseDirectories` (milestone.cts), + // which uses the same value as a DIRECTORY NAME and therefore demands + // COMPLETE only — a real-but-unnamed version is not safe enough there. + const milestone = milestoneInfo.scope === SCOPE.COMPLETE || milestoneInfo.scope === SCOPE.TRUNCATED + ? milestoneInfo.value + : null; if (milestone && milestone.version) { branchName = (config['milestone_branch_template'] as string) .replace('{milestone}', milestone.version) @@ -1561,7 +1576,7 @@ async function cmdWebsearch(query: string | undefined, options: WebsearchOptions function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean): void { const phasesDir = planningPaths(cwd).phases; - const milestone = getMilestoneInfo(cwd); + const milestone = getMilestoneInfo(cwd).value; const phases: PhaseProgress[] = []; let totalPlans = 0; @@ -1603,7 +1618,7 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean const barWidth = 10; const filled = Math.round((percent / 100) * barWidth); const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); - let out = `# ${milestone.version} ${milestone.name}\n\n`; + let out = `# ${milestone?.version ?? ''} ${milestone?.name ?? ''}\n\n`; out += `**Progress:** [${bar}] ${totalSummaries}/${totalPlans} plans (${percent}%)\n\n`; out += `| Phase | Name | Plans | Status |\n`; out += `|-------|------|-------|--------|\n`; @@ -1620,8 +1635,8 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean } else { // JSON format output({ - milestone_version: milestone.version, - milestone_name: milestone.name, + milestone_version: milestone?.version ?? null, + milestone_name: milestone?.name ?? null, phases, total_plans: totalPlans, total_summaries: totalSummaries, @@ -1865,7 +1880,7 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { const roadmapPath = planningPaths(cwd).roadmap; const reqPath = planningPaths(cwd).requirements; const statePath = planningPaths(cwd).state; - const milestone = getMilestoneInfo(cwd); + const milestone = getMilestoneInfo(cwd).value; // Phase & plan stats (reuse progress pattern) const phasesByNumber = new Map 0) { out += `**Plans:** ${totalSummaries}/${totalPlans} complete (${planPercent}%)\n`; diff --git a/src/init.cts b/src/init.cts index e449d05c7..3fa3625a6 100644 --- a/src/init.cts +++ b/src/init.cts @@ -813,6 +813,17 @@ function buildSectionManifestField( } } +/** + * #3216 review Finding 1: `getMilestoneInfo(cwd).value` unwrap-and-cast was + * repeated identically (comment included) at five init call sites — factored + * out once so the cast and its `?? {}` "no milestone resolved" fallback live + * in exactly one place. Behavior-preserving: same call, same fallback, same + * cast, for every caller. + */ +function milestoneRecord(cwd: string): Record { + return (getMilestoneInfo(cwd).value ?? {}) as unknown as Record; +} + function cmdInitExecutePhase( cwd: string, phase: string, @@ -825,7 +836,16 @@ function cmdInitExecutePhase( const config = loadConfig(cwd); let phaseInfo = guardedFindPhase(cwd, phase, config.project_code); - const milestone = getMilestoneInfo(cwd) as unknown as Record; + // #3216: getMilestoneInfo now returns a ScopedResult — `.value` carries the + // MilestoneInfo (or null on any non-COMPLETE scope). NOT display-only: when + // `branching_strategy === 'milestone'`, `milestone['version']`/`['name']` + // below feed `branch_name` construction (see the milestone_branch_template + // branch below), so an unresolved milestone changes the constructed branch + // name, not merely what gets printed. bracket-access below naturally reads + // `undefined` when unresolved; the `milestone_version`/`milestone_name` + // output fields below coerce that to an explicit `null` (#3216 review + // Finding 2) so the key is never silently omitted from the JSON bundle. + const milestone = milestoneRecord(cwd); const roadmapPhase = guardedGetRoadmapPhase(cwd, phase, config.project_code); phaseInfo = applyRoadmapFallback(phaseInfo, roadmapPhase, (rp) => { @@ -905,16 +925,16 @@ function cmdInitExecutePhase( .replace('{slug}', (phaseInfo['phase_slug'] as string) || 'phase') : config.branching_strategy === 'milestone' ? (config.milestone_branch_template as string) - .replace('{milestone}', milestone['version'] as string) + .replace('{milestone}', (milestone['version'] as string | undefined) ?? '') .replace( '{slug}', - generateSlugInternal(milestone['name'] as string) || 'milestone', + generateSlugInternal(milestone['name'] as string | undefined) || 'milestone', ) : null, - milestone_version: milestone['version'], - milestone_name: milestone['name'], - milestone_slug: generateSlugInternal(milestone['name'] as string), + milestone_version: milestone['version'] ?? null, + milestone_name: milestone['name'] ?? null, + milestone_slug: generateSlugInternal(milestone['name'] as string | undefined), state_exists: fs.existsSync(path.join(planningDir(cwd), 'STATE.md')), roadmap_exists: fs.existsSync(path.join(planningDir(cwd), 'ROADMAP.md')), @@ -1208,7 +1228,7 @@ function cmdInitNewProject(cwd: string, raw: boolean, options: Record = {}): void { const config = loadConfig(cwd); - const milestone = getMilestoneInfo(cwd) as unknown as Record; + const milestone = milestoneRecord(cwd); const latestCompleted = getLatestCompletedMilestone(cwd); const phasesDir = path.join(planningDir(cwd), 'phases'); // #3185 (ADR-3180 Decision 1): "how many phase directories belong to the @@ -1227,8 +1247,12 @@ function cmdInitNewMilestone(cwd: string, raw: boolean, options: Record; + const milestone = milestoneRecord(cwd); let phaseCount = 0; let completedPhases = 0; @@ -2076,9 +2100,13 @@ function cmdInitMilestoneOp(cwd: string, raw: boolean): void { const result: Record = { commit_docs: config.commit_docs, - milestone_version: milestone['version'], - milestone_name: milestone['name'], - milestone_slug: generateSlugInternal(milestone['name'] as string), + // #3216 review Finding 2: `?? null` so an unresolved milestone still emits + // the key with an explicit `null` rather than letting JSON.stringify drop + // it — an omitted key reaches the prompt layer's `{milestone_version}` + // placeholder as literal, un-substituted text. + milestone_version: milestone['version'] ?? null, + milestone_name: milestone['name'] ?? null, + milestone_slug: generateSlugInternal(milestone['name'] as string | undefined), phase_count: phaseCount, completed_phases: completedPhases, @@ -2134,7 +2162,7 @@ function cmdInitMapCodebase(cwd: string, raw: boolean): void { function cmdInitManager(cwd: string, raw: boolean): void { const config = loadConfig(cwd); - const milestone = getMilestoneInfo(cwd) as unknown as Record; + const milestone = milestoneRecord(cwd); const _slashRuntime = resolveRuntime(cwd); const paths = planningPaths(cwd); @@ -2476,8 +2504,12 @@ function cmdInitManager(cwd: string, raw: boolean): void { }; const result: Record = { - milestone_version: milestone['version'], - milestone_name: milestone['name'], + // #3216 review Finding 2: `?? null` so an unresolved milestone still emits + // the key with an explicit `null` rather than letting JSON.stringify drop + // it — an omitted key reaches the prompt layer's `{milestone_version}` + // placeholder as literal, un-substituted text. + milestone_version: milestone['version'] ?? null, + milestone_name: milestone['name'] ?? null, phases, phase_count: phases.length, completed_count: completedCount, @@ -2751,7 +2783,7 @@ function cmdInitProgress(cwd: string, raw: boolean, options: Record; + const milestone = milestoneRecord(cwd); const _slashRuntime = resolveRuntime(cwd); // #1912: fail safe in workstream mode with no active workstream. With no active @@ -2937,8 +2969,12 @@ function cmdInitProgress(cwd: string, raw: boolean, options: Record { + const pattern = new RegExp(MILESTONE_HEADING_LINE_SOURCE, 'gmi'); + const out: Array<{ heading: string; version: string; name: string | null; closed: boolean }> = []; + let m: RegExpExecArray | null; + while ((m = pattern.exec(content)) !== null) { + // #3216 review (Finding 4): the shared grammar's `[^\n]*` captures a + // trailing `\r` on a CRLF-encoded ROADMAP (the inline `cmdRoadmapAnalyze` + // regex this replaced called `.trim()`; this did not). `.trim()` here + // matches that prior behavior. `version` (digits/dots/letters only, via + // `extractMilestoneHeadingName`'s regex) and `name` (already run through + // `stripLeadingDelimiter`, which ends in `.trim()`) cannot carry a + // trailing `\r`, so only `heading` needs the fix. + // + // `heading` carries the heading text WITHOUT the leading `#{1,3}` run and + // its following whitespace — matching the inline `cmdRoadmapAnalyze` + // regex this function replaced (`/##\s*(.*v(\d+(?:\.\d+)+)[^(\n]*)/gi`, + // whose capture group 1 begins AFTER `##\s*`). `locateMilestoneHeadings` + // legitimately returns a DIFFERENT representation (`m[1]`, `#`s included) + // — the two owners agree on WHICH milestone headings are selected, not on + // raw heading text. + const heading = m[0].replace(/^#{1,3}\s+/, '').trim(); + const extracted = extractMilestoneHeadingName(heading); + if (extracted === null) continue; // no version token on this heading — not a milestone heading + out.push({ + heading, + version: extracted.version, + name: extracted.name, + closed: isClosedMilestoneHeading(heading), + }); + } + return out; +} + +// #3216: the ONE textual expression of "level-bounded (h1-h3), phase-excluded +// heading line" in this file. `listMilestoneHeadings` and +// `locateMilestoneHeadings` both build their pattern from this constant +// rather than typing `^#{1,3}\s+(?!Phase\s+\S)` a second time — the exact +// duplication class ADR-3180 §7.2's widened guard exists to catch. +const MILESTONE_HEADING_LINE_SOURCE = '^#{1,3}\\s+(?!Phase\\s+\\S)[^\\n]*'; + /** * #3184: the sole milestone-heading locator. Boundary-matched on the version * token with `\b`, NOT the stricter `(?![\w.-])`: this function keeps `\b` @@ -142,17 +206,26 @@ function computeMilestoneSectionEnd(content: string, headingText: string, headin * selection. `extractCurrentMilestoneScoped`, `currentMilestoneRawRanges`, * and `getMilestonePhaseFilter`'s versionOverride branch all consume this * instead of re-deriving their own heading-location regex. + * + * #3216: rewritten as a version-FILTERED VIEW over `MILESTONE_HEADING_LINE_SOURCE` + * — the SAME grammar `listMilestoneHeadings` enumerates — rather than a + * second expression of it. The returned `RegExpExecArray[]` contract + * (`m[0] === m[1]`, `m.index` at the heading's start) is byte-for-byte + * unchanged, so its 4 existing callers are unaffected. */ function locateMilestoneHeadings(content: string, version: string): RegExpExecArray[] { const escapedVersion = escapeRegex(version); - const pattern = new RegExp( - `(^#{1,3}\\s+(?!Phase\\s+\\S).*${escapedVersion}\\b[^\\n]*)`, - 'gmi', - ); + // ADR-3180 §7.1 locks this boundary as `\b`, not the stricter + // `(?![\w.-])` — Amendment 2 tried the stricter boundary and reverted it. + // `\b` alone is what preserves the #730 sub-milestone selection this + // function owns: `v2.0` still matches inside `v2.0.1`, `v8.0` still + // matches `## v8.0-B …`. + const boundary = new RegExp(`${escapedVersion}\\b`, 'i'); + const pattern = new RegExp(`(${MILESTONE_HEADING_LINE_SOURCE})`, 'gmi'); const matches: RegExpExecArray[] = []; let m: RegExpExecArray | null; while ((m = pattern.exec(content)) !== null) { - matches.push(m); + if (boundary.test(m[1])) matches.push(m); } return matches; } @@ -715,7 +788,7 @@ function reportUnreadableRoadmap(err: unknown, roadmapPath: string): void { interface MilestoneInfo { version: string; - name: string; + name: string | null; } /** @@ -731,7 +804,94 @@ function stripLeadingDelimiter(s: string): string { return s.replace(/^[\s—–:-]+/, '').trim(); } -function getMilestoneInfo(cwd: string): MilestoneInfo { +/** + * #3216 (ADR-3180 §7.2's "Name extraction — pinned rule"): the sole "milestone + * heading text → version + curated name" rule. Strips everything through the + * heading's OWN version token — NOT necessarily a version a caller is + * separately asking about (a `v2.0` STATE selecting a `## v2.0.1 — Portability` + * heading yields the name `Portability`, never `.1 — Portability`) — then ONE + * leading delimiter and surrounding whitespace via `stripLeadingDelimiter`. + * `(` is an ordinary name character and is never a terminator (#3171). Shared + * by `listMilestoneHeadings` and `getMilestoneInfo` so this rule has exactly + * one implementation. Returns `null` when `headingText` carries no version + * token at all (e.g. a non-milestone heading reached this by mistake). + * + * @param expectedVersion - When the caller already knows the exact version it + * is looking for (the STATE-anchored `getMilestoneInfo` path, which located + * this heading via `selectMilestoneHeading(roadmap, stateVersion)`), pass it + * here so the "own version token" is found by anchoring to that KNOWN + * literal (escaped, then extended by the same dash/dot continuation grammar + * for the row-16/17 sub-milestone cases) instead of independently + * re-deriving a version-shaped pattern from scratch. `listMilestoneHeadings` + * (version-agnostic enumeration — no target version exists) omits this and + * keeps the generic re-derivation. Anchoring on the known literal is what + * makes a hostile STATE `milestone:` value (regex metacharacters, single- + * segment `vN`, a literal `$&`/`$1`) resolve correctly: the generic pattern + * only recognizes the real GSD version grammar and stops early on anything + * outside it, leaving hostile characters in the extracted "name". + */ +function extractMilestoneHeadingName( + headingText: string, + expectedVersion?: string, +): { version: string; name: string | null } | null { + const versionMatch = expectedVersion + // Anchor to the KNOWN literal version, then extend across any immediate + // dash/dot continuation the heading's OWN token carries beyond it (e.g. + // requested v8.0 -> heading's own v8.0-B; requested v2.0 -> v2.0.1). + // `.match()` here — never `.replace()` — so a `$&`/`$1`-bearing version + // is located as a literal substring and never interpreted as a + // String.replace() substitution pattern. + ? headingText.match(new RegExp(`${escapeRegex(expectedVersion)}(?:[-.][A-Za-z0-9]+)*`, 'i')) + // No known target: re-derive a version-shaped token generically. `v3` / + // `v3.3` / `v3.3.3` must all resolve to themselves (§7.2), so the dotted + // continuation is zero-or-more, not one-or-more. + : headingText.match(/v\d+(?:\.\d+)*(?:[-.][A-Za-z0-9]+)*/i); + if (!versionMatch) return null; + const version = versionMatch[0]; + const afterVersion = headingText.slice((versionMatch.index ?? 0) + version.length); + // Amendment (§7.2 pinned rule): after stripping the leading delimiter, also + // strip a trailing run of status markers (✅ 📋 🚧) plus surrounding + // whitespace — the marker is already carried structurally by `closed`, so + // duplicating it inside `name` (e.g. "Old ✅") is redundant and wrong. Only + // these three markers, only at the end; a marker inside a name is untouched. + const name = stripLeadingDelimiter(afterVersion).replace(/\s*(?:[✅📋🚧]\s*)+$/, '') || null; + return { version, name }; +} + +/** + * #3216 (epic #3180 §7.2, "Milestone identity"): which milestone is current, + * and what is it called. Binds to the canonical `locateMilestoneHeadings` / + * `listMilestoneHeadings` / `extractMilestoneHeadingName` owners and deletes + * both hand-rolled heading regexes this function used to carry — the + * level-blind STATE-version regex (#3197) and the unanchored fallback regex + * (#3171) — so the class of defect they produced ("### Phase N: … v3.3 …" + * read as milestone `v3.3`; a name truncated at `(`) is structurally + * unrepresentable rather than merely fixed on this one copy. + * + * Never throws (#2245) — the outer try/catch returns `{value: null, scope: + * UNREADABLE}` on any failure, preserving `state.cts:1663`'s "this wrapper + * could never be triggered" invariant. Absence (ENOENT) is silent (#1881, + * ADR-1411); a genuine read fault (e.g. EACCES) still reports via + * `reportUnreadableRoadmap`, which discriminates on the errno exactly as + * before. + * + * The `{version:'v1.0', name:'milestone'}` default this function used to + * return on every unresolved path is DELETED per §7.2 rule 4 — it was + * output-identical to a successful read of a genuine v1.0 project. Every + * unresolved path now returns a `scope` other than `COMPLETE` instead. + */ +/** + * #3216 review Finding 2: `getMilestoneInfo`'s `{ value, scope }` return shape + * was hand-built as an inline object literal at every return point — factored + * out once so the shape itself cannot drift between call sites. Purely a + * literal-shape constructor: does not decide, validate, or alter any value or + * scope — every per-branch rationale comment stays exactly where it was. + */ +function scoped(value: MilestoneInfo | null, scope: Scope): { value: MilestoneInfo | null; scope: Scope } { + return { value, scope }; +} + +function getMilestoneInfo(cwd?: string): { value: MilestoneInfo | null; scope: Scope } { // Declared here but RESOLVED INSIDE the try, so the catch can name the file without // moving planningDir() out of the protected region. planningDir throws a plain Error // for an invalid GSD_WORKSTREAM/GSD_PROJECT segment, and hoisting the call let that @@ -740,6 +900,8 @@ function getMilestoneInfo(cwd: string): MilestoneInfo { // skipped and the default is returned exactly as before. let roadmapPath: string | undefined; try { + if (!cwd) return scoped(null, SCOPE.UNREADABLE); + roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); const roadmap = platformReadSync(roadmapPath); if (roadmap === null) throw new Error('missing'); @@ -770,58 +932,98 @@ function getMilestoneInfo(cwd: string): MilestoneInfo { // (the active-milestone bullet). A `##` heading is often nameless // ("## vX.Y — Active Milestone") and, when unanchored, was matched // spuriously on a copy quoted inside backticks in this very bullet. + // #3216 fix (progressMarkerBulletIsConsultedBeforeHeading): the version + // is commonly wrapped in its OWN bold pair — `🚧 **v3.3** Name` — so a + // trailing `\*?\*?` after the version (mirroring the leading one) is + // required before the `\s+` that anchors the name capture; without it + // the closing `**` sits between the version and the required whitespace + // and the whole match fails, silently falling through to the heading. const listMatch = roadmap.match( - new RegExp(`🚧\\s*\\*?\\*?${escapedVer}\\s+([^*\\n]+)`, 'i') + new RegExp(`🚧\\s*\\*?\\*?${escapedVer}\\*?\\*?\\s+([^*\\n]+)`, 'i') ); if (listMatch) { const name = stripLeadingDelimiter(listMatch[1]); - if (name) return { version: stateVersion, name }; + if (name) return scoped({ version: stateVersion, name }, SCOPE.COMPLETE); } - // Fall back to the `##` heading — ANCHORED to line start (`^` + `m` flag) - // so a heading quoted inside backticks or prose mid-line can no longer - // match. Skip shipped (✅) headings. - const headingMatch = roadmap.match( - new RegExp(`^##[^\\n]*${escapedVer}[:\\s]+([^\\n(]+)`, 'im') - ); - if (headingMatch && !headingMatch[0].includes('✅')) { - // Strip a leading delimiter — `.trim()` removes whitespace, not the - // em-dash/colon that conventionally separates version from name. - const name = stripLeadingDelimiter(headingMatch[1]); - if (name) return { version: stateVersion, name }; + // #3216: heading selection routes through the shared owner + // (`selectMilestoneHeading` — locate → prefer-non-closed, mirroring + // `sliceMilestoneWindow`), deleting the level-blind `^##…` regex (#3197) + // and the unanchored `[:\s]+([^\n(]+)` name capture that truncated at a + // parenthetical (#3171). A CLOSED/shipped heading is not "current" (row + // 5) — it falls through to the TRUNCATED return below exactly as a + // missing heading would. + const selected = selectMilestoneHeading(roadmap, stateVersion); + if (selected) { + const headingText = selected[1].replace(/^#{1,3}\s+/, ''); + if (!isClosedMilestoneHeading(headingText)) { + // #3216 fix: pass the KNOWN stateVersion so name extraction anchors + // to it (see extractMilestoneHeadingName's `expectedVersion` doc) — + // fixes single-segment versions (`v3`, no dot) and hostile STATE + // values (regex metacharacters, literal `$&`/`$1`) that the generic + // re-derivation used by listMilestoneHeadings cannot recognize. + const extracted = extractMilestoneHeadingName(headingText, stateVersion); + if (extracted && extracted.name) { + return scoped({ version: stateVersion, name: extracted.name }, SCOPE.COMPLETE); + } + } } - return { version: stateVersion, name: 'milestone' }; + // Version is known (STATE.md), but no name-bearing evidence resolved: + // no 🚧 bullet, no usable heading (absent, phase-only-excluded, shipped, + // or heading-but-nameless). §7.2 rule 4 — never fabricate a name. + return scoped({ version: stateVersion, name: null }, SCOPE.TRUNCATED); } + // No STATE.md version. The 🚧 in-progress bullet is still consulted first + // (unchanged from the pre-#3216 fallback). const inProgressMatch = roadmap.match(/🚧\s*\*\*v(\d+(?:\.\d+)+)\s+([^*]+)\*\*/); if (inProgressMatch) { - return { - version: 'v' + inProgressMatch[1], - name: inProgressMatch[2].trim(), - }; + return scoped( + { version: 'v' + inProgressMatch[1], name: inProgressMatch[2].trim() }, + SCOPE.COMPLETE, + ); } + // #3216: enumerate every OPEN (non-shipped) milestone heading via the + // shared owner and take the first in document order — deletes the + // unanchored `/## (?!.*✅).*v(\d+(?:\.\d+)+)[:\s]+([^\n(]+)/` fallback + // regex (#3171/#3197), whose `## ` prefix matched starting at the SECOND + // `#` of a `### Phase N: …` heading. const cleaned = stripShippedMilestones(roadmap); - const headingMatch = cleaned.match(/## (?!.*✅).*v(\d+(?:\.\d+)+)[:\s]+([^\n(]+)/); - if (headingMatch) { - return { - version: 'v' + headingMatch[1], - name: headingMatch[2].trim(), - }; + const openHeadings = listMilestoneHeadings(cleaned).filter((h) => !h.closed); + if (openHeadings.length > 0) { + const first = openHeadings[0]; + if (first.name) { + return scoped({ version: first.version, name: first.name }, SCOPE.COMPLETE); + } + return scoped({ version: first.version, name: null }, SCOPE.TRUNCATED); } - const versionMatch = cleaned.match(/v(\d+(?:\.\d+)+)/); - return { - version: versionMatch ? versionMatch[0] : 'v1.0', - name: 'milestone', - }; + + // No usable milestone heading anywhere. A version token mentioned ONLY + // inside an excluded `### Phase N: … vX.Y …` heading is not evidence + // (#3197) and must not be reported as if it were a real version — value + // stays null, scope UNSCOPED. A version token mentioned OUTSIDE any Phase + // heading (prose, a bullet, a non-milestone heading) is weak-but-real + // evidence — version retained, name null, scope TRUNCATED. + const withoutPhaseHeadingLines = cleaned.replace(/^#{1,4}\s*Phase\s+\S[^\n]*$/gim, ''); + const bareVersionMatch = withoutPhaseHeadingLines.match(/v\d+(?:\.\d+)+/i); + if (bareVersionMatch) { + return scoped({ version: bareVersionMatch[0], name: null }, SCOPE.TRUNCATED); + } + + // Free-form legacy ROADMAP with no version anywhere reachable, OR the + // only version-bearing heading was a `### Phase N` heading. §7.1's + // "free-form is COMPLETE" governs WINDOWING (whole document is the + // window); identity has no version to report and must not invent one. + return scoped(null, SCOPE.UNSCOPED); } catch (err) { // This function has no existsSync guard, so an absent ROADMAP arrives here too, as a - // synthetic Error with no errno. Only a real read fault is reported; the populated - // default is returned unchanged either way, and a plausible-looking default needs the - // diagnostic more than an empty sentinel does, not less (ADR-1411). + // synthetic Error with no errno. Only a real read fault is reported; `value: null` is + // returned unchanged either way, and a plausible-looking default needs the diagnostic + // more than an empty sentinel does, not less (ADR-1411). if (roadmapPath !== undefined) reportUnreadableRoadmap(err, roadmapPath); - return { version: 'v1.0', name: 'milestone' }; + return scoped(null, SCOPE.UNREADABLE); } } @@ -1156,6 +1358,7 @@ export = { withPhaseSection, computeMilestoneSectionEnd, locateMilestoneHeadings, + listMilestoneHeadings, selectMilestoneHeading, classifyMilestoneWindow, // #3184: the sole "give me this version's window" composition — see its diff --git a/src/roadmap.cts b/src/roadmap.cts index 912f267cd..0f1266e91 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -20,7 +20,7 @@ import phaseLocatorMod = require('./phase-locator.cjs'); const { findPhaseInternal } = phaseLocatorMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserModule = require('./roadmap-parser.cjs'); -const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone } = roadmapParserModule; +const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings } = roadmapParserModule; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import { updateTableCell } from './markdown-table.cjs'; import { clampPercent } from './phase-lifecycle.cjs'; @@ -449,16 +449,14 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { }); } - // Extract milestone info - const milestones: Array<{ heading: string; version: string }> = []; - const milestonePattern = /##\s*(.*v(\d+(?:\.\d+)+)[^(\n]*)/gi; - let mMatch: RegExpExecArray | null; - while ((mMatch = milestonePattern.exec(content)) !== null) { - milestones.push({ - heading: mMatch[1].trim(), - version: 'v' + mMatch[2], - }); - } + // Extract milestone info. #3216: routed through the canonical + // `listMilestoneHeadings` owner (deleted the inline `##…` regex, which + // truncated names at a parenthetical and had no phase-heading exclusion) + // rather than re-deriving the enumeration here. + const milestones: Array<{ heading: string; version: string }> = listMilestoneHeadings(content).map((m) => ({ + heading: m.heading, + version: m.version, + })); // Find current and next phase const currentPhase = phases.find(p => p.disk_status === 'planned' || p.disk_status === 'partial') || null; diff --git a/src/state.cts b/src/state.cts index 92f568c26..5e605bde7 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1659,14 +1659,46 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto let milestone: string | null = null; let milestoneName: string | null = null; + // #1761 regression fix (#3216): the milestone STATE.md actually ASSERTS, + // independent of whether getMilestoneInfo's identity scope is COMPLETE. + // Needed below by the disk-scan block's `isMilestoneBoundedInRoadmap` guard + // — that check answers "is the ASSERTED version bounded to a versioned + // ROADMAP heading", a different question from "is the identity trustworthy + // enough to persist" (`milestone` above). Conflating the two regressed + // #1761: when a real STATE `milestone:` value has no matching ROADMAP + // heading, `info.scope` is never COMPLETE (rightly — there's no curated + // name to persist), but the version was still genuinely asserted and the + // bounded check must still run on it, or the guard silently no-ops and + // `state json` reports a conflated whole-document total_phases/percent. + let assertedMilestoneVersion: string | null = null; if (cwd) { // DEAD catch removed (#2245 audit): getMilestoneInfo has its own outer // try/catch (roadmap-parser.cts) that already swallows every internal - // failure and always returns a MilestoneInfo — it never throws, so this + // failure and always returns a ScopedResult — it never throws, so this // wrapper could never be triggered. + // #3216 (ADR-3180 §7.2 rule 6): this is the #3197 disk-write path. Rule 6 + // draws the line at the FIELD, not the scope as a whole — "a version known + // but no name resolvable is TRUNCATED carrying {version, name: null} — the + // version is a real answer, the name is a non-answer, and collapsing the + // two is the failure this contract exists to prevent." So `milestone` + // (the version) is written whenever COMPLETE or TRUNCATED — both carry a + // genuine version per rule 6 — while `milestoneName` is written only on + // COMPLETE, since TRUNCATED's name is by definition unresolved and must + // never be fabricated. UNSCOPED/UNREADABLE have no real version either + // way, so both stay null there. This mirrors cmdCommit (src/commands.cts), + // which accepts COMPLETE or TRUNCATED for the same reason (the version is + // real), and deliberately diverges from archivePhaseDirectories + // (src/milestone.cts), which demands COMPLETE only because it uses the + // value as a filesystem path component and a TRUNCATED version is not + // safe to use there. const info = getMilestoneInfo(cwd); - milestone = info.version; - milestoneName = info.name; + assertedMilestoneVersion = info.value ? info.value.version : null; + if ((info.scope === SCOPE.COMPLETE || info.scope === SCOPE.TRUNCATED) && info.value) { + milestone = info.value.version; + } + if (info.scope === SCOPE.COMPLETE && info.value) { + milestoneName = info.value.name; + } } let totalPhases: number | null = totalPhasesRaw ? parseInt(totalPhasesRaw, 10) : null; @@ -1783,13 +1815,22 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // phase-dir count only, and mark unbounded so percent is skipped // downstream (mirrors the sync write-path guard). let milestoneBounded = true; - if (milestone && roadmapRaw !== null) { + // #3216 fix (#1761 regression): use `assertedMilestoneVersion` — + // the version STATE.md actually asserts — not the scope-gated + // `milestone`. `milestone` is null on any non-COMPLETE identity + // scope (deliberately, so a non-trustworthy identity never + // persists), but a real asserted version with no matching + // ROADMAP heading is EXACTLY the unbounded case this guard exists + // to catch; gating on `milestone` skipped the guard entirely and + // let the whole-document roadmapPhaseCount conflate sibling + // milestones again. + if (assertedMilestoneVersion && roadmapRaw !== null) { // #3184: routed through the single owner (roadmap-parser.cjs) // instead of a hand-rolled, unbounded-substring re-derivation — // the prior inline regex had no boundary assertion after the // version token, so `v2.0` matched inside `v2.0.1` (#2562-class // defect, design row 17). - milestoneBounded = isMilestoneBoundedInRoadmap(roadmapRaw, String(milestone).trim()); + milestoneBounded = isMilestoneBoundedInRoadmap(roadmapRaw, String(assertedMilestoneVersion).trim()); } // #2828: distinguish a FLAT unmilestoned roadmap (no milestone sectioning // at all — only Phase headings) from a MILESTONED-but-unbounded one diff --git a/src/verify.cts b/src/verify.cts index fd3b27eb0..5d0abc988 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -2320,7 +2320,7 @@ function cmdValidateHealth( fs.copyFileSync(statePath, backupPath); repairActions.push({ action: 'backupState', success: true, path: backupPath }); } - const milestone = getMilestoneInfo(cwd); + const milestone = getMilestoneInfo(cwd).value; const projectRef = path .relative(cwd, path.join(rootBase, 'PROJECT.md')) .split(path.sep) @@ -2329,7 +2329,7 @@ function cmdValidateHealth( stateContent += `## Project Reference\n\n`; stateContent += `See: ${projectRef}\n\n`; stateContent += `## Position\n\n`; - stateContent += `**Milestone:** ${milestone.version} ${milestone.name}\n`; + stateContent += `**Milestone:** ${milestone?.version ?? ''} ${milestone?.name ?? ''}\n`; stateContent += `**Current phase:** (determining...)\n`; stateContent += `**Status:** Resuming\n\n`; stateContent += `## Session Log\n\n`; diff --git a/src/workstream.cts b/src/workstream.cts index dba182022..63b662693 100644 --- a/src/workstream.cts +++ b/src/workstream.cts @@ -157,8 +157,8 @@ function cmdWorkstreamCreate(cwd: string, name: string | null | undefined, optio existingWsName = slugged; } else { try { - const milestone = getMilestoneInfo(cwd); - existingWsName = generateSlugInternal(milestone.name) || 'default'; + const milestone = getMilestoneInfo(cwd).value; + existingWsName = generateSlugInternal(milestone?.name ?? null) || 'default'; } catch { existingWsName = 'default'; } diff --git a/tests/milestone-window-drift-guard.test.cjs b/tests/milestone-window-drift-guard.test.cjs new file mode 100644 index 000000000..92429496e --- /dev/null +++ b/tests/milestone-window-drift-guard.test.cjs @@ -0,0 +1,161 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * Unit coverage for the #3216 WIDENING of the milestone-window drift guard + * (scripts/lint-milestone-window-drift.cjs, epic #3180, issue #3184/#3216, + * ADR-3180 Decision 4(a)). + * + * Modelled on tests/enumeration-drift-guard.test.cjs: this file covers ONLY + * the tokens/behavior #3216 added — the literal `##` heading-anchor path, + * the grouped and interpolated version shapes, and the function-scoped + * (not file-scoped) exemption boundary. Behavioral throughout: assertions + * drive `findMilestoneWindowDrift` / `headingMatcherLiterals` / `scanRepo` + * directly — no `readFileSync().includes()` in a test body. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..'); +const { + findMilestoneWindowDrift, + scanRepo, + headingMatcherLiterals, +} = require(path.join(ROOT, 'scripts', 'lint-milestone-window-drift.cjs')); + +describe('#3216 widened tokens: findMilestoneWindowDrift (pure)', () => { + test('an interpolated-version heading matcher (the real :785 shape) IS flagged', () => { + const line = + " const headingMatch = roadmap.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));"; + const v = findMilestoneWindowDrift(line, 'src/fake.cts'); + assert.equal(v.length, 1); + assert.equal(v[0].line, 1); + }); + + test('a grouped-version regex-literal heading matcher (the real :806 shape) IS flagged', () => { + const line = ' const headingMatch = cleaned.match(/## (?!.*✅).*v(\\d+(?:\\.\\d+)+)[:\\s]+([^\\n(]+)/);'; + const v = findMilestoneWindowDrift(line, 'src/fake.cts'); + assert.equal(v.length, 1); + assert.equal(v[0].line, 1); + }); + + test('an unanchored enumeration shape (the real :454 shape) IS flagged', () => { + const line = ' const milestonePattern = /##\\s*(.*v(\\d+(?:\\.\\d+)+)[^(\\n]*)/gi;'; + const v = findMilestoneWindowDrift(line, 'src/fake.cts'); + assert.equal(v.length, 1); + assert.equal(v[0].line, 1); + }); + + test('phaseHeadingRegexCarryingOnlyTheQuantifierIsNotFlagged', () => { + // Token (a) alone — no literal heading run, no version/marker token — is + // the "where is phase N's heading" question, already single-owned + // elsewhere (#2121), and must stay clean. + const line = ' const phaseRe = /^#{2,4}\\s*Phase\\s+(\\d+)/gm;'; + assert.deepEqual(findMilestoneWindowDrift(line, 'src/fake.cts'), []); + }); + + test('a heading-BUILDING template is NOT flagged (the false-positive #3216 narrowing exists to prevent)', () => { + // A template that BUILDS a heading for output (`## ${version} — ${name}`) + // is not a re-derivation of WHERE a milestone section begins — it never + // matches against document text at all — so it must stay outside the + // literal-heading-run detector, or every heading writer in the tree + // would be flagged as if it were `locateMilestoneHeadings`. + const line = ' const heading = `## ${version} — ${name}`;'; + assert.deepEqual(findMilestoneWindowDrift(line, 'src/fake.cts'), []); + }); + + test('literalHashShapeInACommentIsNotFlagged', () => { + const line = + " // const headingMatch = roadmap.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));"; + assert.deepEqual(findMilestoneWindowDrift(line, 'src/fake.cts'), []); + }); + + test('literalHashShapeInAJsDocContinuationIsNotFlagged', () => { + const line = + " * const headingMatch = roadmap.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));"; + assert.deepEqual(findMilestoneWindowDrift(line, 'src/fake.cts'), []); + }); + + test('a # run inside a string literal that is never fed to new RegExp( is NOT flagged', () => { + const line = " const colour = fgHash + '#ffffff';"; + assert.deepEqual(findMilestoneWindowDrift(line, 'src/fake.cts'), []); + }); +}); + +describe('#3216: headingMatcherLiterals (pure)', () => { + test('a regex literal on the line is returned', () => { + const line = 'const x = /abc/;'; + assert.deepEqual(headingMatcherLiterals(line, line), ['/abc/']); + }); + + test('a string/template literal is returned ONLY when the line also contains new RegExp(', () => { + const withNewRegExp = 'const re = new RegExp(`##foo`);'; + assert.deepEqual(headingMatcherLiterals(withNewRegExp, withNewRegExp), ['`##foo`']); + + const plainAssignment = 'const heading = `##foo`;'; + assert.deepEqual(headingMatcherLiterals(plainAssignment, plainAssignment), []); + }); + + test('a line with no literals returns an empty array', () => { + const line = 'const x = 1 + 2;'; + assert.deepEqual(headingMatcherLiterals(line, line), []); + }); +}); + +describe('#3216: function-scoped exemptions', () => { + const OWNER_REL = path.join('src', 'roadmap-parser.cts'); + + test('namedCanonicalFunctionsRemainExemptAfterWidening', () => { + const text = [ + 'function locateMilestoneHeadings(content, version) {', + " const headingMatch = content.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));", + ' return headingMatch;', + '}', + ].join('\n'); + assert.deepEqual(findMilestoneWindowDrift(text, OWNER_REL), []); + }); + + test('the same flagged shape inside getMilestoneInfo IS flagged, proving the exemption is function-scoped not file-scoped (row 40/53)', () => { + const text = [ + 'function getMilestoneInfo(cwd) {', + " const headingMatch = content.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));", + ' return headingMatch;', + '}', + ].join('\n'); + const v = findMilestoneWindowDrift(text, OWNER_REL); + assert.equal(v.length, 1); + assert.equal(v[0].line, 2); + }); + + test('the same shape in an unrelated file path IS flagged (no exemption entry at all)', () => { + const text = [ + 'function getMilestoneInfo(cwd) {', + " const headingMatch = content.match(new RegExp(`^##[^\\\\n]*${escapedVer}[:\\\\s]+([^\\\\n(]+)`, 'im'));", + ' return headingMatch;', + '}', + ].join('\n'); + const v = findMilestoneWindowDrift(text, path.join('src', 'somewhere-else.cts')); + assert.equal(v.length, 1); + }); +}); + +describe('#3216: the live repo', () => { + test('liveRepoHasZeroMilestoneWindowRederivations', () => { + // Expected to FAIL (RED) until the #3216 consolidation lands — at write + // time the live repo still carries 3 un-consolidated re-derivations + // (roadmap-parser.cts getMilestoneInfo x2, roadmap.cts:454 + // cmdRoadmapAnalyze — see 50-test-matrix.md's "Scope amendment" section). + // This is the failing-first proof the widened guard binds to real code, + // not just synthetic fixtures. + const violations = scanRepo(ROOT); + assert.deepEqual( + violations, + [], + 'unsanctioned milestone-window re-derivation(s) — route through roadmap-parser.cts ' + + 'computeMilestoneSectionEnd / locateMilestoneHeadings / isMilestoneBoundedInRoadmap:\n' + + violations.map((d) => ` ${d.file}:${d.line} ${d.found}`).join('\n'), + ); + }); +}); diff --git a/tests/milestone-window-single-owner.test.cjs b/tests/milestone-window-single-owner.test.cjs index fafa592c3..a1c340cf1 100644 --- a/tests/milestone-window-single-owner.test.cjs +++ b/tests/milestone-window-single-owner.test.cjs @@ -1319,3 +1319,1052 @@ test('classification is newline-invariant', (t) => { } assert.strictEqual(report.failed, false, 'classification must be newline-invariant'); }); + +// ═════════════════════════════════════════════════════════════════════════ +// Section H — Milestone identity has one owner (#3216, epic #3180 Phase 6) +// Matrix: .gsd/phase/refactor-3216-milestone-identity-single-owner/50-test-matrix.md +// ═════════════════════════════════════════════════════════════════════════ +// +// `getMilestoneInfo(cwd)` moves from a bare `{version, name}` return (with a +// `{v1.0,'milestone'}` failure default output-identical to a genuine v1.0 +// project) to `ScopedResult` -- `{value, scope}`, scope +// drawn from the same frozen SCOPE enum Section A uses. `listMilestoneHeadings` +// is a new version-agnostic sibling of `locateMilestoneHeadings`, consolidating +// the THIRD copy the widened drift guard found at `roadmap.cts:454` +// (`cmdRoadmapAnalyze`'s inline milestone-heading regex). +// +// Neither symbol exists in this shape yet -- every test below is failing-first +// by construction, not only the rows the matrix marks (RED). Both are +// destructured locally (not added to the top-of-file import block) so this +// section is a pure append. + +const { + getMilestoneInfo, + listMilestoneHeadings, +} = roadmapParser; +const unusableInputMod = require('../gsd-core/bin/lib/unusable-input.cjs'); +const { + _resetUnusableInputWarningsForTests, + _unusableInputEmissionCountForTests, +} = unusableInputMod; +// `createTempGitProject`/`gitOrThrow` (not added to the top-of-file import +// block, same "pure append" rationale as above): the four destructive +// `phases clear --confirm` tests below need a real, clean git repo so the +// command's own uncommitted-changes guard behaves deterministically instead +// of being bypassed with `--force`. +const { createTempGitProject } = require('./helpers.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); + +// ─── Shared CRLF-twinned fixture table (rows 1, 3, 4, 5, 18, 19, 24) ────── + +const H_CRLF_TABLE = [ + { + label: 'H1 named-heading', + version: 'v3.3', + lines: ['# Roadmap', '', '## v3.3 — Portability', '', 'body text'], + }, + { + label: 'H3 parenthetical-name', + version: 'v3.3', + lines: ['# Roadmap', '', '## v3.3 — Portability (Windows)', '', 'body text'], + }, + { + label: 'H4 phase-heading-only', + version: 'v3.3', + lines: ['# Roadmap', '', '### Phase 7: Close v3.3 gaps', '', 'body text'], + }, + { + label: 'H5 phase-heading-only-unscoped', + version: null, + lines: ['# Roadmap', '', '### Phase 7: Close v3.3 gaps', '', 'body text'], + }, + { + label: 'H18 phase-and-real-heading', + version: 'v3.3', + lines: ['# Roadmap', '', '### Phase 7: Close v3.3 gaps', '', '## v3.3 — Real Name', '', 'body text'], + }, + { + label: 'H19 closed-then-live', + version: 'v3.3', + lines: ['# Roadmap', '', '## v3.3 — Old ✅ SHIPPED', '', '## v3.3 — New', '', 'body text'], + }, +]; + +function buildHFixture(t, entry, crlf = false) { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + if (entry.version) writeState(cwd, { milestone: entry.version }); + const content = entry.lines.join('\n'); + writeRoadmap(cwd, crlf ? content.replace(/\n/g, '\r\n') : content); + return cwd; +} + +test('stateVersionWithNamedHeadingReportsBothAndCompleteScope', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[0]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: 'Portability' }); +}); + +test('progressMarkerBulletIsConsultedBeforeHeading', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + const content = [ + '# Roadmap', + '', + '🚧 **v3.3** Bullet Name (Windows)', + '', + '## v3.3 — Decoy Heading Name', + ].join('\n'); + writeRoadmap(cwd, content); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + // The 🚧 bullet is consulted BEFORE the heading (#2135) -- its name wins + // even though a differently-named heading for the same version exists too. + assert.strictEqual(result.value.name, 'Bullet Name (Windows)'); +}); + +// (RED) #3171: the heading-name capture must not truncate at '('. +test('headingNameRetainsParentheticalInsteadOfTruncating', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[1]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'Portability (Windows)'); +}); + +// (RED) #3197: a Phase heading mentioning the milestone's version is never a +// milestone heading -- even on the STATE-anchored path. +test('phaseHeadingIsNeverAcceptedAsTheMilestoneHeadingOnStatePath', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[2]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.TRUNCATED); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: null }); +}); + +// (RED) #3197: same defect, the UNANCHORED `:806` fallback path (no STATE +// version to anchor on). +test('phaseHeadingIsNeverAcceptedAsTheMilestoneHeadingOnFallbackPath', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[3]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNSCOPED); + assert.strictEqual(result.value, null); +}); + +test('shippedHeadingIsNotReportedAsCurrentMilestone', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['# Roadmap', '', '## v3.3 — Old ✅ SHIPPED', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + // A shipped heading for the STATE-stored version is not "current" -- this + // reduces to the same disposition as "no live heading found". + assert.equal(result.scope, SCOPE.TRUNCATED); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: null }); +}); + +test('knownVersionWithNoHeadingReportsVersionAndNullName', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['# Roadmap', '', '## Overview', '', 'No milestone headings here.'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.TRUNCATED); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: null }); +}); + +// #3216 review Finding 4 / design row 10: no STATE `milestone:` field, no +// milestone heading anywhere, but a bare version token appears in plain +// prose (outside any Phase heading). That is weak-but-real evidence -- the +// version is retained, the name stays `null` (never fabricated), scope +// TRUNCATED. Sibling negative case (no version token anywhere) is +// `freeFormRoadmapWithNoVersionYieldsUnscopedNotV1Default` immediately below. +test('bareVersionInProseYieldsTruncatedWithNoName', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['# Roadmap', '', '## Overview', '', 'Targeting v3.3 for the next release.'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.TRUNCATED); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: null }); +}); + +// (RED) #3197: free-form legacy ROADMAP with zero version tokens anywhere -- +// the version is genuinely unresolvable and must not be invented. +test('freeFormRoadmapWithNoVersionYieldsUnscopedNotV1Default', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['# Roadmap', '', '## Overview', '', '### Phase 1: Foo'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNSCOPED); + assert.strictEqual(result.value, null); +}); + +// (RED) THE load-bearing row. Every other row in this section can pass while +// the scope wiring is inverted (e.g. COMPLETE and UNREADABLE swapped) -- only +// a genuine v1.0 project reporting {v1.0, }/COMPLETE proves the +// old `{version:'v1.0', name:'milestone'}` failure default is actually GONE, +// rather than renamed to a scope label nobody checks. +test('genuineV1ProjectIsDistinguishableFromTheOldFailureDefault', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['# Roadmap', '', '## v1.0 — Foundation Release', '', '### Phase 1: Bootstrap'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.deepStrictEqual(result.value, { version: 'v1.0', name: 'Foundation Release' }); + // The name is deliberately NOT the literal string 'milestone' -- that is the + // old failure default's signature, and a project that legitimately named + // its v1.0 milestone "milestone" is not this test's concern. + assert.notStrictEqual(result.value.name, 'milestone'); +}); + +test('absentRoadmapIsUnreadableScopeAndStaysSilent', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + // No ROADMAP.md written at all -- ENOENT. + _resetUnusableInputWarningsForTests(); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNREADABLE); + assert.strictEqual(result.value, null); + // #1881: absence alone must never be reported -- every brand-new project + // has no ROADMAP.md yet, and flagging that as corrupt would be noise. + assert.strictEqual(_unusableInputEmissionCountForTests(), 0); +}); + +test('unreadableRoadmapReportsDiagnosticAndUnreadableScope', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + writeRoadmap(cwd, ['# Roadmap', '', '## v3.3 — Name'].join('\n')); + writeState(cwd, { milestone: 'v3.3' }); + _resetUnusableInputWarningsForTests(); + + mock.method(shellProj, 'platformReadSync', () => { + const err = new Error('EACCES: simulated permission denied'); + err.code = 'EACCES'; + throw err; + }); + t.after(() => { + mock.restoreAll(); + cleanup(cwd); + }); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNREADABLE); + assert.strictEqual(result.value, null); + // ADR-1411: unlike absence, a genuine read fault IS reported. + assert.strictEqual(_unusableInputEmissionCountForTests(), 1); +}); + +test('unreadableStateFallsBackWithoutFabricatingAVersion', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + // No versioned milestones anywhere in the ROADMAP, so the fallback path + // (once STATE.md is unreachable) has no version evidence to find either. + writeRoadmap(cwd, ['# Roadmap', '', '## Overview', '', '### Phase 1: Foo'].join('\n')); + writeState(cwd, { milestone: 'v3.3' }); + + const originalRead = shellProj.platformReadSync; + mock.method(shellProj, 'platformReadSync', (targetPath) => { + if (typeof targetPath === 'string' && targetPath.endsWith('STATE.md')) { + const err = new Error('EACCES: simulated permission denied'); + err.code = 'EACCES'; + throw err; + } + return originalRead(targetPath); + }); + t.after(() => { + mock.restoreAll(); + cleanup(cwd); + }); + + const result = getMilestoneInfo(cwd); + // Falls back to ROADMAP-only heuristics -- which find no version -- rather + // than fabricating v1.0 or trusting a version it could not actually read. + assert.equal(result.scope, SCOPE.UNSCOPED); + assert.strictEqual(result.value, null); +}); + +test('emptyRoadmapYieldsUnscoped', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ''); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNSCOPED); + assert.strictEqual(result.value, null); +}); + +test('whitespaceOnlyRoadmapYieldsUnscoped', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ' \n\n\t \n'); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.UNSCOPED); + assert.strictEqual(result.value, null); +}); + +test('headingLevelsOneThroughThreeAreAllAccepted', (t) => { + const tmpDirs = []; + t.after(() => { for (const d of tmpDirs) cleanup(d); }); + + for (const level of [1, 2, 3]) { + const cwd = createTempDir('gsd-milestone-identity-'); + tmpDirs.push(cwd); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, [`${'#'.repeat(level)} v3.3 — Name`, '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE, `level ${level}`); + assert.strictEqual(result.value.name, 'Name', `level ${level}`); + } +}); + +test('headingLevelFourIsRejected', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['#### v3.3 — Name', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + // #{1,3} is the owner's level ceiling -- a level-4 heading is invisible to + // it, same as no heading at all. + assert.equal(result.scope, SCOPE.TRUNCATED); + assert.deepStrictEqual(result.value, { version: 'v3.3', name: null }); +}); + +test('versionSegmentCountsAtAndAroundTwoAreHandled', (t) => { + const tmpDirs = []; + t.after(() => { for (const d of tmpDirs) cleanup(d); }); + + for (const version of ['v3', 'v3.3', 'v3.3.3']) { + const cwd = createTempDir('gsd-milestone-identity-'); + tmpDirs.push(cwd); + writeState(cwd, { milestone: version }); + writeRoadmap(cwd, [`## ${version} — Name`, '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE, version); + assert.deepStrictEqual(result.value, { version, name: 'Name' }, version); + } +}); + +test('realMilestoneHeadingWinsOverAPhaseHeadingMentioningTheSameVersion', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[4]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'Real Name'); +}); + +test('prefersTheNonClosedHeadingWhenAVersionAppearsTwice', (t) => { + const cwd = buildHFixture(t, H_CRLF_TABLE[5]); + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'New'); +}); + +// #730 regression guard: `v8.0` (STATE) must select the LIVE `v8.0-B` +// sub-milestone heading over the CLOSED `v8.0-A` one -- the `\b` boundary +// that makes this possible is deliberate, load-bearing behavior (§7.1). +test('subMilestoneSelectionIsPreservedForBoundaryWordChars', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v8.0' }); + writeRoadmap(cwd, [ + '# Roadmap', + '', + '## v8.0-A — Old ✅ SHIPPED', + '', + '## v8.0-B — LiveMarker', + ].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.version, 'v8.0'); + // Exact equality, not .includes(): the name derives from the heading's OWN + // version token (v8.0-B), not the requested v8.0 -- an .includes() check + // would also pass on a leaked prefix from the closed v8.0-A heading, which + // is the specific misselection this row guards against (pinned rule, + // .gsd/phase/refactor-3216-milestone-identity-single-owner/40-design.md). + assert.equal(result.value.name, 'LiveMarker', `expected exact name LiveMarker, got ${JSON.stringify(result.value.name)}`); +}); + +// ADR-3180 §7.1 locks the `\b` boundary deliberately: `v2.0` (STATE) matching +// `## v2.0.1 — Name` is CORRECT, not a bug. Amendment 2 tried the stricter +// `(?![\w.-])` boundary specifically to stop this and REVERTED it because it +// breaks #730 sub-milestone selection (the row above). Do not "fix" this. +test('versionPrefixMatchInsideLongerVersionIsRetainedDeliberately', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, ['## v2.0.1 — Name', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.version, 'v2.0'); + // Exact equality, not .includes(): the name derives from the heading's OWN + // version token (v2.0.1), not the requested v2.0 -- an .includes() check + // would also pass on the '.1 — Name' leftover that stripping the + // *requested* token (instead of the heading's own) would produce, which is + // precisely the defect the pinned name-extraction rule excludes + // (.gsd/phase/refactor-3216-milestone-identity-single-owner/40-design.md). + assert.equal(result.value.name, 'Name', `expected exact name Name, got ${JSON.stringify(result.value.name)}`); +}); + +test('leadingDelimiterIsStrippedFromName', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['## v3.3 — Delimited Name', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'Delimited Name'); +}); + +// #2135: a name beginning with '#' is a heading-parse failure -- it must stay +// loud (unstripped) rather than being silently cleaned, unlike a whitespace/ +// dash/colon/em-dash leading delimiter. +test('hashLedNameIsLeftLoudRatherThanCleaned', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['## v3.3 #HashName', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, '#HashName'); +}); + +test('crlfRoadmapsProduceIdenticalIdentityToLf', (t) => { + const tmpDirs = []; + t.after(() => { for (const d of tmpDirs) cleanup(d); }); + + for (const entry of H_CRLF_TABLE) { + const lfCwd = createTempDir('gsd-milestone-identity-crlf-lf-'); + const crlfCwd = createTempDir('gsd-milestone-identity-crlf-crlf-'); + tmpDirs.push(lfCwd, crlfCwd); + if (entry.version) { + writeState(lfCwd, { milestone: entry.version }); + writeState(crlfCwd, { milestone: entry.version }); + } + const lfContent = entry.lines.join('\n'); + writeRoadmap(lfCwd, lfContent); + writeRoadmap(crlfCwd, lfContent.replace(/\n/g, '\r\n')); + + const lfResult = getMilestoneInfo(lfCwd); + const crlfResult = getMilestoneInfo(crlfCwd); + assert.equal(crlfResult.scope, lfResult.scope, `scope mismatch for ${entry.label}`); + assert.strictEqual(crlfResult.value?.name ?? null, lfResult.value?.name ?? null, `name mismatch for ${entry.label}`); + if (crlfResult.value?.name) { + assert.ok(!crlfResult.value.name.includes('\r'), `CRLF leaked into name for ${entry.label}: ${JSON.stringify(crlfResult.value.name)}`); + } + } +}); + +test('unicodeNameIsPreserved', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['## v3.3 — Ünïcode ▲', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'Ünïcode ▲'); +}); + +test('veryLongNameIsNotTruncated', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + const longName = 'X'.repeat(4096); + writeRoadmap(cwd, [`## v3.3 — ${longName}`, '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name.length, 4096); + assert.strictEqual(result.value.name, longName); +}); + +// #2143: fence-awareness is explicitly out of scope for this phase (Decision +// 6) -- a milestone-shaped heading at line-start INSIDE a fenced code block +// still matches. Documented known limit, not a regression to fix here. +test('fencedHeadingStillMatchesAsADocumentedKnownLimit', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['# Roadmap', '', '```markdown', '## v3.3 — Fenced Name', '```', '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.name, 'Fenced Name'); +}); + +// #40-design.md row 28's actual requirement: `escapeRegex` holds -- no +// crash, no catastrophic match. It does NOT require a hostile version ending +// in regex metacharacters to successfully RESOLVE to a milestone (with `\b` +// restored per ADR-3180 §7.1, `v1.0+(x)` sits between two non-word +// characters on both sides of its own boundary and structurally cannot +// match there -- that is the LOCKED `\b` semantics, not a defect). +test('versionWithRegexMetacharactersIsEscaped', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + const hostileVersion = 'v1.0+(x)'; + writeState(cwd, { milestone: hostileVersion }); + writeRoadmap(cwd, [`## ${hostileVersion} — Name`, '', 'body'].join('\n')); + + // 1. No crash. + assert.doesNotThrow(() => getMilestoneInfo(cwd)); + const result = getMilestoneInfo(cwd); + assert.strictEqual(result.value.version, hostileVersion, 'the raw hostile version is preserved verbatim'); + + // 2. Metacharacters are treated as literals, never as regex operators. If + // `+` and `(x)` were left unescaped, `v1.0+(x)` would compile to "v1", + // then literal ".", then "one-or-more '0'", then a capturing group + // matching literal "x" -- which would falsely match a document like + // "v1.00000(x)" that the ESCAPED literal string must never match. + const unescapedWouldFalselyMatch = ['## v1.00000(x) — Wrong Match', '', 'body'].join('\n'); + assert.strictEqual( + locateMilestoneHeadings(unescapedWouldFalselyMatch, hostileVersion).length, + 0, + 'escaped metacharacters must not act as regex operators against an unrelated document', + ); + + // 3. Completes promptly -- no catastrophic backtracking. No elapsed-time + // bound is asserted (see normalize-test-command.test.cjs's identical + // rationale): a real ReDoS regression manifests as the run hanging / + // being killed, which is a louder, more reliable signal than a threshold. + const hostileRepeated = 'v1.0+(x)'.repeat(5000); + writeState(cwd, { milestone: hostileRepeated }); + writeRoadmap(cwd, [`## ${hostileRepeated} — Name`, '', 'body'].join('\n')); + assert.doesNotThrow(() => getMilestoneInfo(cwd)); +}); + +test('versionWithReplacementPatternIsTreatedLiterally', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + const hostileVersion = 'v1.0$&$1'; + writeState(cwd, { milestone: hostileVersion }); + writeRoadmap(cwd, [`## ${hostileVersion} — Name`, '', 'body'].join('\n')); + + const result = getMilestoneInfo(cwd); + assert.equal(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value.version, hostileVersion); + // '$&'/'$1' must never be interpreted as a String.replace() substitution + // pattern -- the extracted name must be exactly 'Name', not a corrupted + // expansion of the hostile version token. + assert.strictEqual(result.value.name, 'Name'); +}); + +// Hostile / sink (#2288 defense in depth): a traversal-shaped STATE +// `milestone:` value must never let `archivePhaseDirectories` move phase +// history outside `.planning/milestones/`. +test('traversalVersionNeverEscapesTheMilestonesDirectory', (t) => { + // `createTempGitProject` (git init + initial commit) gives the command's + // own uncommitted-changes guard a deterministic clean repo, so the test + // proves the real destructive-command guard rather than bypassing it with + // `--force` -- `--force` is exactly the safety this row exercises. + const cwd = createTempGitProject('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['# Roadmap', '', 'No milestone headings here.'].join('\n')); + const hostileVersion = '../../etc/passwd'; + writeState(cwd, { milestone: hostileVersion }); + writeFile(cwd, path.join('.planning', 'phases', '01-example', 'PLAN.md'), '# Plan'); + gitOrThrow(['add', '-A'], { cwd }); + gitOrThrow(['commit', '-m', 'seed fixture'], { cwd }); + + const escapeTarget = path.resolve(cwd, '.planning', 'milestones', '..', '..', 'etc', 'passwd-phases'); + assert.strictEqual(fs.existsSync(escapeTarget), false, 'precondition: escape target must not pre-exist'); + + const result = runGsdTools(['phases', 'clear', '--confirm', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + + // Negative proof: the traversal string never left `.planning/milestones/`. + assert.strictEqual(fs.existsSync(escapeTarget), false); + const milestonesDir = path.join(cwd, '.planning', 'milestones'); + const entries = fs.existsSync(milestonesDir) ? fs.readdirSync(milestonesDir) : []; + assert.ok(entries.length > 0, 'expected the dated-fallback archive dir to exist'); + for (const entry of entries) { + assert.ok(!entry.includes('etc') && !entry.includes('passwd'), `traversal string leaked into archive dir name: ${entry}`); + // ARCHIVE_VERSION_LABEL_RE rejects the traversal string, so the archive + // falls back to the dated label -- never the hostile STATE value. + assert.match(entry, /^archived-\d{8}-phases$/); + } +}); + +test('controlCharacterVersionIsRefusedAsAPathComponent', (t) => { + // `createTempGitProject`, not a bare temp dir + `--force`: see rationale on + // `traversalVersionNeverEscapesTheMilestonesDirectory` above. + const cwd = createTempGitProject('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['# Roadmap', '', 'No milestone headings here.'].join('\n')); + const ESC = String.fromCharCode(0x1b); + const hostileVersion = `v1.0${ESC}[31mHACK`; + writeState(cwd, { milestone: hostileVersion }); + writeFile(cwd, path.join('.planning', 'phases', '01-example', 'PLAN.md'), '# Plan'); + gitOrThrow(['add', '-A'], { cwd }); + gitOrThrow(['commit', '-m', 'seed fixture'], { cwd }); + + const result = runGsdTools(['phases', 'clear', '--confirm', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + // Nothing replayed to the terminal: the hostile version never reaches the + // command's own output. + assert.strictEqual((result.output || '').includes(ESC), false); + + const milestonesDir = path.join(cwd, '.planning', 'milestones'); + const entries = fs.existsSync(milestonesDir) ? fs.readdirSync(milestonesDir) : []; + assert.ok(entries.length > 0, 'expected the dated-fallback archive dir to exist'); + for (const entry of entries) { + assert.ok(!entry.includes(ESC), `control character leaked into archive dir name: ${JSON.stringify(entry)}`); + assert.match(entry, /^archived-\d{8}-phases$/); + } +}); + +test('undefinedCwdDoesNotThrow', () => { + // The shipping caller shape: buildStateFrontmatter guards `if (cwd)` before + // calling getMilestoneInfo, but getMilestoneInfo itself must survive being + // called with cwd === undefined directly (#2245). + let result; + assert.doesNotThrow(() => { result = getMilestoneInfo(undefined); }); + assert.ok(result && typeof result === 'object' && 'scope' in result && 'value' in result); + assert.ok(Object.values(SCOPE).includes(result.scope)); +}); + +test('getMilestoneInfoNeverThrowsAcrossEveryInputClass', (t) => { + const tmpDirs = []; + t.after(() => { for (const d of tmpDirs) cleanup(d); }); + + function freshDir() { + const cwd = createTempDir('gsd-milestone-identity-'); + tmpDirs.push(cwd); + return cwd; + } + + const inputClasses = [ + // 1. No ROADMAP.md at all (ENOENT). + () => freshDir(), + // 2. Empty ROADMAP. + () => { const c = freshDir(); writeRoadmap(c, ''); return c; }, + // 3. Whitespace-only ROADMAP. + () => { const c = freshDir(); writeRoadmap(c, ' \n\t\n'); return c; }, + // 4. Phase-heading-only, V set. + () => { + const c = freshDir(); + writeState(c, { milestone: 'v3.3' }); + writeRoadmap(c, '### Phase 7: Close v3.3 gaps'); + return c; + }, + // 5. Free-form legacy, no version anywhere. + () => { const c = freshDir(); writeRoadmap(c, '## Overview\n### Phase 1: Foo'); return c; }, + // 6. Hostile regex-metacharacter version. + () => { + const c = freshDir(); + writeState(c, { milestone: 'v1.0+(x)[y]' }); + writeRoadmap(c, '## v1.0+(x)[y] — Name'); + return c; + }, + // 7. Control-character version. + () => { + const c = freshDir(); + writeState(c, { milestone: `v1.0${String.fromCharCode(0x1b)}HACK` }); + writeRoadmap(c, '## Overview'); + return c; + }, + // 8. Traversal-shaped version. + () => { + const c = freshDir(); + writeState(c, { milestone: '../../etc/passwd' }); + writeRoadmap(c, '## Overview'); + return c; + }, + // 9. CRLF roadmap. + () => { + const c = freshDir(); + writeState(c, { milestone: 'v3.3' }); + writeRoadmap(c, '## v3.3 — Name\r\n\r\nbody\r\n'); + return c; + }, + // 10. Sub-milestone selection shape. + () => { + const c = freshDir(); + writeState(c, { milestone: 'v8.0' }); + writeRoadmap(c, '## v8.0-A — Old ✅ SHIPPED\n\n## v8.0-B — Live'); + return c; + }, + // 11. cwd === undefined. + () => undefined, + // 12. cwd pointing at a nonexistent directory. + () => path.join(freshDir(), 'does-not-exist-subdir'), + ]; + + for (const build of inputClasses) { + const cwd = build(); + assert.doesNotThrow(() => getMilestoneInfo(cwd), `getMilestoneInfo threw for input class producing cwd=${cwd}`); + } +}); + +// Identity (4c): the consumers' OBSERVABLE output must agree with the +// owner's ScopedResult for the same fixture -- not a locally-reassembled +// copy of it (ADR-3180 Decision 4c: "post-filtering the canonical result" is +// itself a bypass this row is designed to catch). +test('consumerOutputsMatchTheOwnersScopedResultForTheSameFixture', (t) => { + // `createTempGitProject`, not a bare temp dir + `--force`: see rationale on + // `traversalVersionNeverEscapesTheMilestonesDirectory` above. + const cwd = createTempGitProject('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + writeRoadmap(cwd, ['# Roadmap', '', '## v3.3 — Consolidated Name', '', 'body'].join('\n')); + writeFile(cwd, path.join('.planning', 'phases', '01-example', 'PLAN.md'), '# Plan'); + gitOrThrow(['add', '-A'], { cwd }); + gitOrThrow(['commit', '-m', 'seed fixture'], { cwd }); + + const owner = getMilestoneInfo(cwd); + assert.equal(owner.scope, SCOPE.COMPLETE); + + // Consumer 1: buildStateFrontmatter, via `state sync` -> `state json`. + const syncResult = runGsdTools(['state', 'sync', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(syncResult.success, true, syncResult.error); + const jsonResult = runGsdTools(['state', 'json', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(jsonResult.success, true, jsonResult.error); + const parsedJson = JSON.parse(jsonResult.output); + assert.strictEqual(parsedJson.milestone, owner.value.version); + assert.strictEqual(parsedJson.milestone_name, owner.value.name); + + // Consumer 2: archivePhaseDirectories, via `phases clear --confirm`. + const clearResult = runGsdTools(['phases', 'clear', '--confirm', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(clearResult.success, true, clearResult.error); + const milestonesDir = path.join(cwd, '.planning', 'milestones'); + const entries = fs.readdirSync(milestonesDir); + assert.deepStrictEqual(entries, [`${owner.value.version}-phases`]); +}); + +// (RED) Identity (4c), CLI: the full untruncated name must PERSIST through +// `state sync`, and a non-COMPLETE identity must never persist a fabricated +// name. +test('stateSyncPersistsTheOwnersIdentityNotAFabricatedOne', (t) => { + const tmpDirs = []; + t.after(() => { for (const d of tmpDirs) cleanup(d); }); + + // Row-3 fixture: parenthetical name must persist UNTRUNCATED. + const cwd3 = createTempDir('gsd-milestone-identity-'); + tmpDirs.push(cwd3); + writeState(cwd3, { milestone: 'v3.3' }); + writeRoadmap(cwd3, ['# Roadmap', '', '## v3.3 — Portability (Windows)', '', 'body'].join('\n')); + const sync3 = runGsdTools(['state', 'sync', '--cwd', cwd3, '--raw'], cwd3); + assert.strictEqual(sync3.success, true, sync3.error); + const json3result = runGsdTools(['state', 'json', '--cwd', cwd3, '--raw'], cwd3); + assert.strictEqual(json3result.success, true, json3result.error); + const json3 = JSON.parse(json3result.output); + assert.strictEqual(json3.milestone_name, 'Portability (Windows)'); + + // Row-4 fixture: only a Phase heading -- must persist NO fabricated name. + const cwd4 = createTempDir('gsd-milestone-identity-'); + tmpDirs.push(cwd4); + writeState(cwd4, { milestone: 'v3.3' }); + writeRoadmap(cwd4, ['# Roadmap', '', '### Phase 7: Close v3.3 gaps', '', 'body'].join('\n')); + const sync4 = runGsdTools(['state', 'sync', '--cwd', cwd4, '--raw'], cwd4); + assert.strictEqual(sync4.success, true, sync4.error); + const json4result = runGsdTools(['state', 'json', '--cwd', cwd4, '--raw'], cwd4); + assert.strictEqual(json4result.success, true, json4result.error); + const json4 = JSON.parse(json4result.output); + assert.strictEqual(json4.milestone, 'v3.3'); + assert.strictEqual('milestone_name' in json4, false, `milestone_name must be absent, not fabricated: ${JSON.stringify(json4.milestone_name)}`); +}); + +// (RED) Identity (4c), CLI: `archivePhaseDirectories` must refuse a +// syntactically-valid-but-not-COMPLETE version and fall back to the dated +// label instead -- #3197's exact bug (a fabricated-but-well-formed 'v3.3' +// passing ARCHIVE_VERSION_LABEL_RE and misfiling phase history). +test('phasesClearFallsBackToDatedLabelWhenIdentityIsNotComplete', (t) => { + // `createTempGitProject`, not a bare temp dir + `--force`: see rationale on + // `traversalVersionNeverEscapesTheMilestonesDirectory` above. + const cwd = createTempGitProject('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + // Row-4 fixture: only a Phase heading -- scope is TRUNCATED, not COMPLETE, + // even though 'v3.3' alone is a syntactically VALID archive label. + writeRoadmap(cwd, ['# Roadmap', '', '### Phase 7: Close v3.3 gaps', '', 'body'].join('\n')); + writeFile(cwd, path.join('.planning', 'phases', '01-example', 'PLAN.md'), '# Plan'); + gitOrThrow(['add', '-A'], { cwd }); + gitOrThrow(['commit', '-m', 'seed fixture'], { cwd }); + + const owner = getMilestoneInfo(cwd); + assert.notEqual(owner.scope, SCOPE.COMPLETE); + + const result = runGsdTools(['phases', 'clear', '--confirm', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const entries = fs.readdirSync(path.join(cwd, '.planning', 'milestones')); + assert.strictEqual(entries.length, 1); + // Must NOT be 'v3.3-phases' -- that is #3197's fabricated-but-well-formed + // label misfiling phase history under the wrong milestone. + assert.notStrictEqual(entries[0], 'v3.3-phases'); + assert.match(entries[0], /^archived-\d{8}-phases$/); +}); + +// (RED) Property: any name without a newline, rendered into a +// `## — ` heading, round-trips through getMilestoneInfo with no +// truncation -- document-shaped (#2371): built from a local grammar here, +// never via any renderer in roadmap-parser.cjs. +test('propertyHeadingNameRoundTripsWithoutTruncation', (t) => { + const cwd = createTempDir('gsd-milestone-identity-property-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + + // Starts with a letter so it can never collide with a leading-delimiter + // strip; letters/digits/spaces/parens exercise the #3171 parenthetical + // retention without ambiguity about what "round-trips" means. + const nameGen = fc.stringMatching(/^[A-Za-z][A-Za-z0-9 ()]{0,60}$/).filter((s) => s.trim() === s && s.length > 0); + + const report = fc.check( + fc.property(nameGen, (name) => { + writeRoadmap(cwd, [`## v3.3 — ${name}`, '', 'body'].join('\n')); + const result = getMilestoneInfo(cwd); + return result.scope === SCOPE.COMPLETE && result.value.name === name; + }), + { seed: 3216, numRuns: 100 }, + ); + if (report.failed) { + t.diagnostic(`H43 counterexample (replay seed=3216): ${JSON.stringify(report.counterexample)}`); + } + assert.strictEqual(report.failed, false, 'heading name must round-trip without truncation'); +}); + +// Property: no generated `### Phase N …` heading -- with or without an +// embedded version token -- ever yields COMPLETE from getMilestoneInfo when +// it is the only heading in the document. +test('propertyPhaseHeadingsNeverYieldCompleteScope', (t) => { + const cwd = createTempDir('gsd-milestone-identity-property-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + + const phaseWordGen = fc.stringMatching(/^[A-Za-z][A-Za-z0-9]{0,12}$/); + const phaseNumGen = fc.integer({ min: 1, max: 999 }); + + const report = fc.check( + fc.property(phaseNumGen, phaseWordGen, (num, word) => { + writeRoadmap(cwd, [`### Phase ${num}: Close v3.3 ${word}`, '', 'body'].join('\n')); + const result = getMilestoneInfo(cwd); + return result.scope !== SCOPE.COMPLETE; + }), + { seed: 3216, numRuns: 100 }, + ); + if (report.failed) { + t.diagnostic(`H44 counterexample (replay seed=3216): ${JSON.stringify(report.counterexample)}`); + } + assert.strictEqual(report.failed, false, 'a Phase heading alone must never yield COMPLETE'); +}); + +test('listMilestoneHeadingsEnumeratesEveryMilestoneInDocumentOrder', () => { + const content = [ + '# Roadmap', + '', + '## v1.0 — First ✅ SHIPPED', + '', + '## v2.0 — Second', + '', + '### Phase 1: Foo', + '', + '## v3.0 — Third 🚧', + ].join('\n'); + + const result = listMilestoneHeadings(content); + assert.strictEqual(result.length, 3); + assert.strictEqual(result[0].version, 'v1.0'); + assert.strictEqual(result[0].closed, true); + assert.strictEqual(result[1].version, 'v2.0'); + assert.strictEqual(result[1].closed, false); + assert.strictEqual(result[2].version, 'v3.0'); + assert.strictEqual(result[2].closed, false); +}); + +// (RED) #3197: a phase heading is never a milestone -- this is the SAME +// consolidation defect as site 1/2, in the THIRD copy (`roadmap.cts:454`). +test('listMilestoneHeadingsExcludesPhaseHeadings', () => { + const content = ['# Roadmap', '', '### Phase 7: Close v3.3 gaps'].join('\n'); + const result = listMilestoneHeadings(content); + assert.deepStrictEqual(result, []); +}); + +// (RED) #3171: the enumerated heading text must not be cut at a parenthetical. +test('listMilestoneHeadingsRetainsParentheticalNames', () => { + const content = ['# Roadmap', '', '## v3.3 — Portability (Windows)'].join('\n'); + const result = listMilestoneHeadings(content); + assert.strictEqual(result.length, 1); + assert.ok(result[0].heading.includes('Portability (Windows)'), result[0].heading); +}); + +// (RED) #3216 review Finding 4: the shared grammar's `[^\n]*` captures a +// trailing `\r` on a CRLF-encoded ROADMAP, and unlike the inline +// `cmdRoadmapAnalyze` regex this replaced (which called `.trim()`), +// `listMilestoneHeadings` did not trim its `heading` extraction -- so a CRLF +// ROADMAP's `heading` field carried a stray trailing `\r` a LF ROADMAP never +// would. Every entry's `heading` must be `\r`-free and carry no leading/ +// trailing whitespace, for every milestone in the document, not just one. +test('listMilestoneHeadingsHeadingFieldIsCrlfFreeAndTrimmed', () => { + const content = ['## v3.3 — Portability (Windows)', '', '## v2.0 — Old ✅'].join('\r\n'); + const result = listMilestoneHeadings(content); + assert.strictEqual(result.length, 2); + for (const entry of result) { + assert.ok(!entry.heading.includes('\r'), `CRLF leaked into heading: ${JSON.stringify(entry.heading)}`); + assert.strictEqual(entry.heading, entry.heading.trim(), `heading not trimmed: ${JSON.stringify(entry.heading)}`); + } + assert.strictEqual(result[0].name, 'Portability (Windows)'); + assert.strictEqual(result[1].name, 'Old'); +}); + +test('listMilestoneHeadingsRespectsTheOneToThreeLevelBound', () => { + const content = [ + '# v1.0 — Level One', + '', + '## v2.0 — Level Two', + '', + '### v3.0 — Level Three', + '', + '#### v4.0 — Level Four', + ].join('\n'); + + const result = listMilestoneHeadings(content); + const versions = result.map((h) => h.version); + assert.deepStrictEqual(versions, ['v1.0', 'v2.0', 'v3.0']); +}); + +test('listMilestoneHeadingsFlagsClosedMilestones', () => { + const content = ['## v1.0 — Closed ✅ SHIPPED', '', '## v2.0 — Live 🚧'].join('\n'); + const result = listMilestoneHeadings(content); + assert.strictEqual(result.length, 2); + assert.strictEqual(result[0].closed, true); + assert.strictEqual(result[1].closed, false); +}); + +// (RED) Identity (4c), CLI: `roadmap analyze`'s `milestones[]` must agree +// with `listMilestoneHeadings` over the SAME scoped content the consumer +// actually used (extractCurrentMilestoneScoped's `.value`), not a +// re-derivation of it. +test('roadmapAnalyzeMilestonesMatchTheOwnersEnumeration', (t) => { + const cwd = createTempDir('gsd-milestone-identity-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.3' }); + const rawContent = [ + '# Roadmap', + '', + '## v3.3 — Portability (Windows)', + '', + '### Phase 7: Close v3.3 gaps', + '', + '### Phase 1: Foo', + ].join('\n'); + writeRoadmap(cwd, rawContent); + + const result = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const parsed = JSON.parse(result.output); + + assert.strictEqual(parsed.milestones.length, 1); + assert.ok(parsed.milestones[0].heading.includes('Portability (Windows)'), parsed.milestones[0].heading); + assert.ok(!parsed.milestones.some((m) => /Phase\s+7/.test(m.heading)), 'a Phase heading must never be reported as a milestone'); + + // Agreement with the owner's enumeration over the SAME scoped content the + // consumer actually used. + const ownerContent = extractCurrentMilestoneScoped(rawContent, cwd).value; + const ownerHeadings = listMilestoneHeadings(ownerContent); + assert.strictEqual(parsed.milestones.length, ownerHeadings.length); + for (let i = 0; i < ownerHeadings.length; i += 1) { + assert.strictEqual(parsed.milestones[i].version, ownerHeadings[i].version); + } +}); + +// Parity (anti-drift seam): `listMilestoneHeadings` (version-agnostic +// enumeration) and `locateMilestoneHeadings` (version-filtered locator) must +// agree on WHICH milestone headings are selected for a given version — NOT +// on raw heading text. The two are legitimately different representations: +// `locateMilestoneHeadings` returns raw `RegExpExecArray`s (`m[1]` is the +// full matched line, `#`s included), while `listMilestoneHeadings` returns +// structured entries whose `heading` field has the `#{1,3}` prefix and +// following whitespace stripped (matching the inline `cmdRoadmapAnalyze` +// regex this replaced). Comparing raw strings between the two would make +// this test assert an accidental implementation detail rather than the +// actual parity contract, and would silently regress if either owner's +// textual representation ever changed for an unrelated reason. +test('versionFilteredEnumerationMatchesTheLocator', () => { + const content = [ + '# Roadmap', + '', + '## v1.0 — First ✅ SHIPPED', + '', + '## v2.0 — Second', + '', + '## v2.0 — Second Redux 🚧', + '', + '### Phase 1: Foo', + '', + '## v3.0 — Third', + ].join('\n'); + + const enumerated = listMilestoneHeadings(content); + const versions = [...new Set(enumerated.map((h) => h.version))]; + assert.ok(versions.length > 1, 'fixture must exercise more than one version'); + + for (const version of versions) { + const enumeratedCount = enumerated.filter((h) => h.version === version).length; + const located = locateMilestoneHeadings(content, version); + assert.strictEqual( + located.length > 0, + enumeratedCount > 0, + `selection disagreement for ${version}: listMilestoneHeadings found ${enumeratedCount}, locateMilestoneHeadings found ${located.length}`, + ); + assert.strictEqual( + located.length, + enumeratedCount, + `selection count mismatch for ${version}`, + ); + } +}); + +// H-P: document-shaped generator (#2371) local to this section -- built from +// a heading/prose grammar, never by calling any renderer in +// roadmap-parser.cjs. Deliberately independent of Section G's blockGen so a +// regression in one generator cannot mask a regression in the other. +const H_SAFE_WORD = fc.stringMatching(/^[A-Za-z][A-Za-z0-9]{0,8}$/); + +const hHeadingBlockGen = fc.record({ + level: fc.integer({ min: 1, max: 4 }), + isPhase: fc.boolean(), + hasVersion: fc.boolean(), + word: H_SAFE_WORD, +}).map((b) => ({ + render() { + const prefix = '#'.repeat(b.level); + const phasePart = b.isPhase ? 'Phase 3: ' : ''; + const versionPart = b.hasVersion ? ' v2.0' : ''; + return `${prefix} ${phasePart}${b.word}${versionPart}`; + }, +})); + +const hProseBlockGen = H_SAFE_WORD.map((word) => ({ render() { return `prose ${word} line`; } })); + +const hBlockGen = fc.oneof(hHeadingBlockGen, hProseBlockGen); + +// Property: for any generated document, no entry `listMilestoneHeadings` +// returns ever has a heading beginning with 'Phase '. +test('propertyEnumerationNeverReturnsAPhaseHeading', (t) => { + const documentGen = fc.array(hBlockGen, { minLength: 1, maxLength: 12 }); + + const report = fc.check( + fc.property(documentGen, (blocks) => { + const content = blocks.map((b) => b.render()).join('\n'); + const result = listMilestoneHeadings(content); + return result.every((h) => !/^Phase\s/i.test(h.heading.replace(/^#{1,3}\s+/, '').trim())); + }), + { seed: 3216, numRuns: 100 }, + ); + if (report.failed) { + t.diagnostic(`H54 counterexample (replay seed=3216): ${JSON.stringify(report.counterexample)}`); + } + assert.strictEqual(report.failed, false, 'listMilestoneHeadings must never enumerate a Phase heading'); +}); diff --git a/tests/new-milestone-clear-phases.test.cjs b/tests/new-milestone-clear-phases.test.cjs index fcc3bcf06..f39ecf0d8 100644 --- a/tests/new-milestone-clear-phases.test.cjs +++ b/tests/new-milestone-clear-phases.test.cjs @@ -328,12 +328,16 @@ describe('phases clear: archive-version override (#2288)', () => { ); }); - test('override omitted with no ROADMAP uses getMilestoneInfo default (no-override path unchanged)', () => { - // createTempProject writes no ROADMAP.md, so getMilestoneInfo does NOT throw — - // it returns its documented default version ('v1.0'). The dated `archived-*` - // label is only reached if no safe version label is resolvable at all, which - // this common case is not. This pins the no-override path to its pre-#2288 - // behavior (getMilestoneInfo-derived label), not a dated fallback. + test('override omitted with no ROADMAP archives under the dated fallback label (#3216: getMilestoneInfo default deleted)', () => { + // #3216 (ADR-3180 §7.2 Decision, roadmap-parser.cjs:788-791): getMilestoneInfo's + // plausible-looking {version:'v1.0', name:'milestone'} default — output-identical + // to a genuine v1.0 project — was deleted. With no ROADMAP.md, getMilestoneInfo + // now returns {value:null, scope:SCOPE.UNREADABLE}; archivePhaseDirectories only + // trusts a SCOPE.COMPLETE identity as a directory-name-safe version + // (milestone.cjs:959-965), so a non-COMPLETE scope falls through to the dated + // `archived-` label (milestone.cjs:977-978) instead of 'v1.0'. This + // was previously misfiled under 'v1.0-phases', which read as a genuine v1.0 + // milestone's archive rather than "no resolvable milestone identity". // eslint-disable-next-line local/no-raw-rmsync-in-tests -- ensure no ROADMAP.md (SUT fallback path, not teardown) fs.rmSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), { recursive: true, force: true }); @@ -346,8 +350,19 @@ describe('phases clear: archive-version override (#2288)', () => { assert.ok(result.success, `Command failed: ${result.error}`); assert.ok( - fs.existsSync(path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases', '01-foundation')), - 'no override + no ROADMAP archives under getMilestoneInfo default (v1.0), unchanged from pre-#2288' + !fs.existsSync(path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases')), + 'no version identity is resolvable — must NOT be misfiled under a plausible-looking v1.0-phases' + ); + const archive = findPhasesArchive(tmpDir); + assert.ok(archive, 'a milestones/*-phases/ archive should still be created'); + assert.match( + path.basename(archive), + /^archived-\d{8}-phases$/, + 'no resolvable milestone identity — must use the dated archived- fallback label' + ); + assert.ok( + fs.existsSync(path.join(archive, '01-foundation')), + 'the phase directory must still be archived (moved, not deleted) under the dated label' ); }); diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index df91773fb..ba04ad082 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -23,6 +23,7 @@ const fs = require('node:fs'); const path = require('node:path'); const roadmapParser = require('../gsd-core/bin/lib/roadmap-parser.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); const { @@ -512,25 +513,26 @@ describe('roadmap-parser: getMilestoneInfo', () => { beforeEach(() => { tmpDir = createTempProject(); }); afterEach(() => { cleanup(tmpDir); }); - test('returns default when ROADMAP.md missing', () => { + test('returns UNREADABLE scope (value null) when ROADMAP.md missing (#3216: the v1.0/"milestone" default was deleted per ADR-3180 §7.2 rule 4)', () => { const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v1.0'); - assert.strictEqual(info.name, 'milestone'); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNREADABLE }); }); test('reads version from STATE.md and heading name', () => { writeState(tmpDir, { milestone: 'v2.0' }); writeRoadmap(tmpDir, '## v2.0: The Big Launch\n### Phase 1: Setup\n'); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v2.0'); - assert.match(info.name, /Big Launch/); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v2.0'); + assert.match(info.value.name, /Big Launch/); }); test('falls back to 🚧 WIP marker when STATE.md has no milestone', () => { writeRoadmap(tmpDir, '## 🚧 **v1.5 Work In Progress**\n### Phase 1: Do stuff\n'); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v1.5'); - assert.match(info.name, /Work In Progress/i); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v1.5'); + assert.match(info.value.name, /Work In Progress/i); }); test('extracts from heading when no STATE.md and no WIP marker', () => { @@ -539,8 +541,9 @@ describe('roadmap-parser: getMilestoneInfo', () => { '### Phase 1: Not started', ].join('\n')); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v3.0'); - assert.match(info.name, /Future Milestone/); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v3.0'); + assert.match(info.value.name, /Future Milestone/); }); test('skips completed ✅ milestones', () => { @@ -550,7 +553,8 @@ describe('roadmap-parser: getMilestoneInfo', () => { ].join('\n')); const info = getMilestoneInfo(tmpDir); // Should not use the ✅-prefixed version as the current milestone - assert.strictEqual(info.version, 'v2.0'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v2.0'); }); }); @@ -580,8 +584,9 @@ describe('roadmap-parser: getMilestoneInfo #2135 — milestone_name clobber', () '### Phase 36: Something', ].join('\n')); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v1.8'); - assert.strictEqual(info.name, 'user session cleanup'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v1.8'); + assert.strictEqual(info.value.name, 'user session cleanup'); }); test('case B: nameless ## heading + 🚧 marker carries the real name', () => { @@ -594,32 +599,36 @@ describe('roadmap-parser: getMilestoneInfo #2135 — milestone_name clobber', () '### Phase 1: Hypothesis', ].join('\n')); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v1.9'); - assert.strictEqual(info.name, 'Falsifiability'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v1.9'); + assert.strictEqual(info.value.name, 'Falsifiability'); }); test('case C: canonical ## vX.Y: Name (no regression)', () => { writeState(tmpDir, { gsd_state_version: '1.0', milestone: 'v2.0' }); writeRoadmap(tmpDir, '## v2.0: The Big Launch\n### Phase 1: Setup\n'); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v2.0'); - assert.strictEqual(info.name, 'The Big Launch'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v2.0'); + assert.strictEqual(info.value.name, 'The Big Launch'); }); test('case D: canonical ## vX.Y — Name (em-dash delimiter stripped)', () => { writeState(tmpDir, { gsd_state_version: '1.0', milestone: 'v2.5' }); writeRoadmap(tmpDir, '## v2.5 — Galaxy Release\n### Phase 1: Start\n'); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v2.5'); - assert.strictEqual(info.name, 'Galaxy Release'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v2.5'); + assert.strictEqual(info.value.name, 'Galaxy Release'); }); test('case E: 🚧 bullet only, no ## heading (no regression)', () => { writeState(tmpDir, { gsd_state_version: '1.0', milestone: 'v1.5' }); writeRoadmap(tmpDir, 'Some intro text.\n\n- 🚧 **v1.5 Quick Fix** — minor\n'); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.version, 'v1.5'); - assert.strictEqual(info.name, 'Quick Fix'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.version, 'v1.5'); + assert.strictEqual(info.value.name, 'Quick Fix'); }); test('anchored regex never matches a ## heading quoted inside backticks mid-line', () => { @@ -632,7 +641,8 @@ describe('roadmap-parser: getMilestoneInfo #2135 — milestone_name clobber', () '## v3.0: Real Name', ].join('\n')); const info = getMilestoneInfo(tmpDir); - assert.strictEqual(info.name, 'Real Name'); + assert.strictEqual(info.scope, SCOPE.COMPLETE); + assert.strictEqual(info.value.name, 'Real Name'); }); }); @@ -2726,10 +2736,12 @@ describe('feat-3594: roadmap parser does not crash on ANY corpus fixture', () => // ─── #1881: an unreadable ROADMAP is not an absent one ─────────────────────── // // getRoadmapPhaseInternal returns null for a read failure exactly as it does for -// "phase not found", and getMilestoneInfo returns {v1.0, milestone} — which reads -// as a brand-new project — for a read failure exactly as it does for "no ROADMAP -// yet". Per ADR-1411's "corrupt is not absent" amendment both return values are -// preserved and the cause is surfaced out of band instead. +// "phase not found", and getMilestoneInfo (#3216: now {value, scope}) returns +// {value:null, scope:SCOPE.UNREADABLE} for a read failure exactly as it does for +// "no ROADMAP yet" — the shared return shape stays sentinel-identical between the +// two causes; only the out-of-band diagnostic distinguishes them. Per ADR-1411's +// "corrupt is not absent" amendment both return values are preserved and the +// cause is surfaced out of band instead. // // The discriminator is the errno, and it is load-bearing in the silent direction: // getMilestoneInfo has no existsSync guard, so platformReadSync's null-for-ENOENT @@ -2822,7 +2834,7 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { const dir = project(t, HEALTHY); let info; const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); - assert.deepStrictEqual(info, { version: 'v2.3', name: 'Alpha' }); + assert.deepStrictEqual(info, { value: { version: 'v2.3', name: 'Alpha' }, scope: SCOPE.COMPLETE }); assert.strictEqual(emitted, 0); }); @@ -2836,14 +2848,14 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { assert.strictEqual(emitted, 1); }); - test('an unreadable roadmap is reported on a milestone lookup, and still returns the default', (t) => { + test('an unreadable roadmap is reported on a milestone lookup, and returns {value:null, scope:UNREADABLE} (#3216: the plausible-looking v1.0 default was deleted)', (t) => { _resetUnusableInputWarningsForTests(); const dir = project(t, HEALTHY); failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); let info; const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); - assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }, - 'the plausible-looking default must be preserved exactly'); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNREADABLE }, + 'a read fault must surface as UNREADABLE, never a fabricated version/name'); assert.strictEqual(emitted, 1); }); @@ -2857,7 +2869,7 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { info = getMilestoneInfo(dir); }); assert.strictEqual(phase, null); - assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNREADABLE }); assert.strictEqual(emitted, 0, 'a missing ROADMAP.md must never be reported as unreadable'); }); @@ -2883,7 +2895,8 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { }); assert.strictEqual(phase, null); assert.strictEqual(emitted, 0); - assert.ok(info, 'still returns a milestone default'); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNSCOPED }, + 'no version token anywhere reachable — UNSCOPED, not a fabricated version'); }); test('an unreadable STATE.md alone stays silent — the inner catch is deliberate', (t) => { @@ -2897,7 +2910,7 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { failReads(t, (p) => p.endsWith('STATE.md'), eacces()); let info; const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); - assert.deepStrictEqual(info, { version: 'v2.3', name: 'Alpha' }); + assert.deepStrictEqual(info, { value: { version: 'v2.3', name: 'Alpha' }, scope: SCOPE.COMPLETE }); assert.strictEqual(emitted, 0); }); @@ -2980,7 +2993,7 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { emissionsDuring(() => { assert.doesNotThrow(() => { info = getMilestoneInfo(dir); }); }); - assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNREADABLE }); }); test('getRoadmapPhaseInternal does not throw when the read fails', (t) => { @@ -3014,7 +3027,7 @@ describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { assert.doesNotThrow(() => { info = getMilestoneInfo(dir); }); assert.doesNotThrow(() => { phase = getRoadmapPhaseInternal(dir, '1'); }); }); - assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + assert.deepStrictEqual(info, { value: null, scope: SCOPE.UNREADABLE }); assert.strictEqual(phase, null); assert.strictEqual(emitted, 0, 'the path never resolved, so there is no file to name');