diff --git a/.changeset/serene-elks-sing.md b/.changeset/serene-elks-sing.md new file mode 100644 index 000000000..99c8619e4 --- /dev/null +++ b/.changeset/serene-elks-sing.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3209 +--- +**A truncated milestone window is no longer reported as an empty milestone** — `roadmap analyze` now emits a `scope` field (`complete`/`truncated`/`unscoped`/`unreadable`) so `phase_count: 0` from a genuinely fresh milestone is distinguishable from a window that closed before reaching the roadmap's phase sections, and `milestone complete` refuses to archive on a truncated window instead of moving every phase directory in the project. (#3184) diff --git a/CONTEXT.md b/CONTEXT.md index 2dae55b02..ef499005c 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). 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 `(?![\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`). ### 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/bin/install.js b/bin/install.js index 750ebcde4..566ae8a79 100755 --- a/bin/install.js +++ b/bin/install.js @@ -361,6 +361,21 @@ const GSD_HOOK_LIB_FILES = ['git-cmd.js', 'gsd-graphify-rebuild.sh', 'cursor-wor */ const SHARED_HOOKS_DIR_DEFAULT = 'hooks'; +// #3184 — GSD-managed file enumerations for scripts/changeset/ and scripts/lib/ +// uninstall. The install-side copy of both directories is wholesale ("copy every +// file present"), so these enumerations MUST be kept in parity with the real +// directory contents or an added file ships on install and then orphans on +// uninstall (survives removal, keeps the dir non-empty, blocks its rmdir). +// Hoisted to module scope (and exported below) so tests/install.test.cjs can +// assert parity against fs.readdirSync(scripts/lib) / fs.readdirSync(scripts/changeset) +// without source-grepping this file. +const GSD_CHANGESET_FILES = [ + 'cli.cjs', 'parse.cjs', 'render.cjs', 'serialize.cjs', + 'github-release-notes.cjs', 'lint.cjs', 'new.cjs', + 'README.md', // documentation only — not user-authored +]; +const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs']; + /** * Resolve a runtime's shared-hooks directory name from its descriptor. * @@ -8642,13 +8657,8 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { // Any file NOT in this set is user-owned and must survive uninstall. // After removing GSD files, attempt to rmdir — if the directory is still // non-empty (user has custom helpers) it stays; otherwise it goes cleanly. - const GSD_CHANGESET_FILES = [ - 'cli.cjs', 'parse.cjs', 'render.cjs', 'serialize.cjs', - 'github-release-notes.cjs', 'lint.cjs', 'new.cjs', - 'README.md', // documentation only — not user-authored - ]; - const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs']; - + // GSD_CHANGESET_FILES / GSD_SCRIPTS_LIB_FILES are module-scoped (#3184) so + // tests can assert their parity against the real directory contents. const changesetUninstallDir = path.join(targetDir, 'scripts', 'changeset'); if (fs.existsSync(changesetUninstallDir)) { let removedChangeset = 0; @@ -13482,6 +13492,10 @@ module.exports = { // #3023 — shared hook bundle directory name, descriptor-driven SHARED_HOOKS_DIR_DEFAULT, resolveSharedHooksDirName, + // #3184 — uninstall-side GSD-managed file enumerations, exported for + // parity assertions against the wholesale-copy source directories + GSD_CHANGESET_FILES, + GSD_SCRIPTS_LIB_FILES, convertSlashCommandsToCodexSkillMentions, convertClaudeCommandToCodexSkill, convertClaudeCommandToKimiSkill, diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 7d430e493..b7e1bff5f 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -184,6 +184,39 @@ node gsd-tools.cjs roadmap analyze node gsd-tools.cjs roadmap update-plan-progress ``` +### Milestone window scope (`roadmap analyze`) + +`roadmap analyze` scopes its phase list to the current milestone's section of +`ROADMAP.md`. Its JSON output carries a `scope` field describing how much of the +intended input that scoping actually saw: + +| `scope` | Meaning | +|---|---| +| `complete` | The window was computed over the whole intended input. `phase_count: 0` here is a **real** answer — a freshly-declared milestone genuinely has no phases yet. | +| `truncated` | The milestone's heading was found, but its window closed before reaching the document's phase region — typically because a closed-milestone heading sits between the active milestone and its `### Phase N:` sections. `phase_count: 0` here is a **non**-answer. | +| `unscoped` | No milestone version could be resolved (or its section is absent) on a ROADMAP that does use versioned milestones, so the result is not milestone-scoped. | +| `unreadable` | `ROADMAP.md` could not be read. | + +Before this field existed, all four cases produced the same well-formed +`phase_count: 0` with no error, so a consumer could not tell a genuinely empty +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. + +### `milestone complete` refuses an untrustworthy window + +`milestone complete` archives `ROADMAP.md`/`REQUIREMENTS.md` and **moves phase +directories** — a one-way door. When the milestone window's `scope` is +`truncated` — the milestone heading was found but its section closes before +reaching any phase entries, even though the ROADMAP has phase entries +elsewhere — phase scoping cannot be trusted, and the command now refuses +rather than falling back to an over-inclusive filter that would archive every +phase directory in the project. `unreadable` (no ROADMAP.md at all) and +`unscoped` (no section for this version) are pre-existing, legitimately +handled states and are not refused here. Pass `--force` to override, the same +affordance the unstarted-phase guard uses. + --- ## Config Commands diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 774a7bc14..495f2cd50 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -472,6 +472,8 @@ If any category is non-empty you are prompted with `[R] Resolve` / `[A] Acknowle > **Note:** the `deferred-items.md` category is the per-phase SCOPE BOUNDARY log a phase agent writes when it finds a defect it should not fix. It is a different artifact from the `## Deferred Items` section `[A]` writes into `STATE.md`, which records what you acknowledged at close. +> **Truncated-window guard.** Archiving also refuses when the milestone's ROADMAP window is truncated — `Cannot mark milestone complete: the ROADMAP window for "" is truncated`. This is the case where the milestone's heading is found but its section closes before reaching the roadmap's `### Phase N:` region (typically a closed-milestone heading sitting in between), which previously degraded to an over-inclusive filter and archived *every* phase directory in the project rather than the milestone's own. An unreadable ROADMAP.md or a version with no matching section at all are pre-existing, legitimately-handled states and are not refused here. Same override as below: `gsd-tools milestone complete --force`. A window that is genuinely empty — a freshly-declared milestone with no phases yet — is *not* affected and still completes normally. + > **Unstarted-phase guard.** Archiving refuses if the milestone's ROADMAP still lists a phase with no phase directory on disk — `Cannot mark milestone complete: ROADMAP lists N unstarted phase(s)`. If a phase was intentionally deferred or merged without a directory, run `gsd-tools milestone complete --force` (the `/gsd-complete-milestone` workflow runs the underlying command without `--force`, so use the CLI directly to override). A `STATE.md` `milestone:` value that does not match `` prints a WARNING and still runs the guard (#2946). --- diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index f7f3a38cd..8da2c7044 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -295,3 +295,59 @@ plan report as a naming violation — the diagnostic reads non-membership as a d `allPlanFiles`. The general rule: a **diagnostic about file naming** wants the physical set; only a question about outstanding *work* wants the live set. Later phases must make that choice explicitly per call site rather than swapping in `planFiles` mechanically. + +### Amendment 2 — Phase 2 (#3184) validation: the contract held; the copy count was low again + +Decision 2's contract needed **no change** for its second consumer: `SCOPE`'s four values covered +every row of the windowing derivation's behavior table, including the two rows the epic's text does +not distinguish (a free-form legacy ROADMAP with no versioned milestones is `COMPLETE`, not +`UNSCOPED` — whole-document genuinely *is* the milestone there). Phase 2 adds no member and changes +no semantics. `src/planning-scope.cts` needed no edit, so Phase 2 carries no `.cts` six-gate ripple. + +**What Phase 2 found.** The epic and this ADR both scope milestone windowing at **three** copies, all +inside `roadmap-parser.cts`. Building the Decision 4(a) whole-repo guard found **two more**, in a +different module and one function down: `state.cts` `buildStateFrontmatter` and `syncStateFrontmatter` +each hand-roll `^#{1,3}\s+(?!Phase\s+\S).*${escapeRegex(version)}` to answer "is this milestone +bounded to a versioned ROADMAP heading" — the heading-location half of the derivation, byte-identical +to each other. This is Phase 1's finding repeating with a different derivation: **the epic's copy +counts are a lower bound derived from the reported issues, and the whole-repo guard is what makes them +real.** Both sites now call the owner's `isMilestoneBoundedInRoadmap`, which is a straight +consolidation of the two identical `state.cts` regexes onto `locateMilestoneHeadings` with **no +behavior change** — which is all it should ever have been. + +**A boundary tightening was tried and reverted.** A first pass at `locateMilestoneHeadings` swapped its +`\b` version-token boundary for the stricter `(?![\w.-])` used by `isMilestoneShippedInRoadmap` +(#2562), reasoning that `v2.0` should not match inside `v2.0.1` anywhere windowing happens. That broke +`extractCurrentMilestoneScoped`'s #730 contract: a milestone STATE of `v8.0` legitimately selects the +`## v8.0-B …` active sub-milestone heading over a closed `v8.0-A` sibling (`0` is a word character, `-` +is not, so `\b` matches; `(?![\w.-])` does not, because `-` is in its excluded set). `\b` is restored in +`locateMilestoneHeadings`; the stricter boundary stays local to `isMilestoneShippedInRoadmap` and to the +#730 `detailsVersionBoundary`, which answer a narrower question ("is exactly this milestone shipped" / +"which Phase Details section is exactly this one's version token's") than "which heading does this +milestone STATE select." The consolidation itself (three `roadmap-parser.cts` copies plus the two +`state.cts` copies onto one owner) is behavior-preserving. + +**A composition-level re-derivation, caught in review of this phase's own diff.** Decision 4(c) +anticipated a consumer post-*filtering* an owner's result. The shape that actually appeared is its +mirror: two sites re-*assembling* a window out of the owner's primitives — +`locateMilestoneHeadings` → pick a heading → `computeMilestoneSectionEnd` → slice — in +`getMilestonePhaseFilter`'s `versionOverride` branch and in `milestone.cts`'s unstarted-phase guard. +Both call the canonical owner at every step, so the drift guard and an owner-level identity test are +both green, and the two compositions had **already diverged** on whether to skip a closed milestone +heading. Decision 4(c) is therefore read to cover **assembly as well as post-processing**: where a +derivation has a composition, the composition is itself an owner. Added as +`sliceMilestoneWindow`; both sites route through it. + +**Decision 3's Tier-2 table, re-derived for Phase 2** per its own contingency clause. The row this +ADR predicted lands as written, plus two the prediction did not contain: + +| Command surface | Output change | +|---|---| +| `roadmap analyze` | gains a `scope` field. `phase_count: 0` is still emitted verbatim — what changes is that a sibling field now says whether that zero is an answer. Stated precisely because the first draft of this row claimed the count itself changed, which is not what shipped | +| `/gsd:progress --next` Route 0 | `gsd-core/workflows/next.md` treats a non-`complete` scope as scan-failed (warn + fall through to the prior-phase check) instead of looping a phase list the scan could not populate. Without this the new field would be a diagnostic no consumer reads, and #3165's actual symptom — the resume invariant reporting clean because it could not run — would still reproduce | +| `milestone complete` | refuses (unless `--force`) when the window's scope is `TRUNCATED` — the milestone heading was found but its section closes before reaching any phase entries, even though the ROADMAP has phase entries elsewhere — instead of pass-all archiving every phase directory on disk (#3166). `UNREADABLE` and `UNSCOPED` are pre-existing, legitimately-handled states (`missingExplicitVersion` errors where that matters; a missing ROADMAP.md has its own documented graceful path) and are not refused here. | +| `milestone complete` unstarted-phase guard — **not predicted** | the guard scoped its window by STATE.md's `milestone:` field while the filter beside it scoped by the `version` argument; the two could disagree, and the guard under-detected unstarted phases on the destructive path. Both now use the `version` argument. | + +**Scope note.** Phase 3 (enumeration) inherits a window layer that is now single-owner and +scope-carrying; its own guard starts from a green windowing baseline, exactly as Phase 1 left plan +counting clean for Phase 3. diff --git a/gsd-core/workflows/next.md b/gsd-core/workflows/next.md index 0e4807fe3..96b7c0cc7 100644 --- a/gsd-core/workflows/next.md +++ b/gsd-core/workflows/next.md @@ -104,11 +104,22 @@ Illustrative bash: ```bash INCOMPLETE_PHASE="" ROADMAP_JSON=$(gsd_run query roadmap.analyze) +ROADMAP_SCOPE=$(echo "$ROADMAP_JSON" | jq -r '.scope // "complete"') if [ $? -ne 0 ] || [ -z "$ROADMAP_JSON" ]; then echo "⚠ WARNING: resume-incomplete-phase scan could not run (roadmap.analyze failed)." >&2 echo " The incomplete-phase invariant (#160) could not be verified." >&2 echo " Proceeding to prior-phase completeness check — review project state carefully." >&2 # Fall through to prior_phase_completeness rather than silently skipping +elif [ "$ROADMAP_SCOPE" != "complete" ]; then + # #3184/#3165: roadmap.analyze succeeded and returned a well-formed document, + # but its milestone window did not see all of its input, so `.phases[]` is a + # NON-answer rather than a real empty. Looping it would run the invariant over + # a phase list the scan could not populate and report "clean" — the silent + # disarm #3165 reports. Treated as scan-failed, same as an outright failure. + echo "⚠ WARNING: resume-incomplete-phase scan could not be scoped (roadmap.analyze scope: $ROADMAP_SCOPE)." >&2 + echo " The milestone window did not cover the whole ROADMAP, so the phase list is incomplete." >&2 + echo " The incomplete-phase invariant (#160) could not be verified — review project state carefully." >&2 + # Fall through to prior_phase_completeness rather than silently passing else for PHASE_NUM in $(echo "$ROADMAP_JSON" | jq -r '.phases[] | (.number // .phase_number // empty)'); do PHASE_JSON=$(gsd_run query find-phase "$PHASE_NUM") @@ -338,6 +349,7 @@ Resume with: `/gsd:progress --next --auto` once resolved. - [ ] `--no-resume`: Route 0 skipped, prior_phase_completeness defer prompt runs as before - [ ] `--force`: everything skipped (Gates, Route 0, prior_phase_completeness) → straight to `determine_next_action` - [ ] Scan uses `gsd_run` (canonical resolver form); errors are surfaced rather than suppressed +- [ ] A `roadmap.analyze` result whose `scope` is not `complete` is treated as scan-failed (warn + fall through), never as a clean empty scan (#3184/#3165) - [ ] Predicate is plans-without-summaries (`plans.length > summaries.length`) — consistent with `determine_next_action` Route 4 - [ ] Next action correctly determined from routing rules - [ ] Command invoked immediately without user confirmation diff --git a/package.json b/package.json index 88d2694b7..fe7e212c8 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,7 @@ "lint": "eslint . --cache --cache-location node_modules/.cache/eslint/", "lint:fix": "eslint . --fix", "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lib/drift-scan.cjs b/scripts/lib/drift-scan.cjs new file mode 100644 index 000000000..07ebac39c --- /dev/null +++ b/scripts/lib/drift-scan.cjs @@ -0,0 +1,278 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Shared anti-divergence scanner machinery (epic #3180, ADR-3180 Decision 4). + * + * Extracted from `scripts/lint-plan-count-drift.cjs` (#3183) so that + * `scripts/lint-milestone-window-drift.cjs` (#3184) and any future + * `scripts/lint--drift.cjs` guard consume ONE tree-walk / + * root-confinement / literal-tokenizer / sanitizer implementation instead of + * each copying it verbatim — the exact generative-fix-divergence class this + * epic exists to remove, now applied to the guards themselves (see + * `.gsd/phase/refactor-3184-milestone-window-single-owner/40-design.md`, + * "Rejected: let the new drift guard copy Phase 1's tree-walk / + * root-confinement / sanitizer"). + * + * `lint-plan-count-drift.cjs` re-exports every symbol it exported before this + * extraction (`readRegexLiteralAt`, `MAX_REGEX_LITERAL_LEN`, `isInsideRoot`, + * `sanitizeForReport`), so `tests/plan-count-single-owner.test.cjs` — the + * ReDoS and root-confinement regression net for this exact machinery — + * continues to pass unchanged and is the regression net for this move too. + */ + +const fs = require('node:fs'); +const path = require('node:path'); + +// Longest regex literal this scanner will consider, in characters. Real +// plan/summary filename filters (and milestone-window regex literals) are far +// shorter; the bound is what keeps the scan linear. The tokenizer restarts at +// every `/` on the line (so that a literal preceded by a stray unpaired `/` +// is still found, matching the previous regex's "find anywhere" behaviour), +// which without a per-literal bound would be quadratic on a pathological +// line. With it the whole-line cost is O(n * MAX_REGEX_LITERAL_LEN) with no +// backtracking at all. +const MAX_REGEX_LITERAL_LEN = 400; + +// Directory names this scanner never descends into or reports out of — +// `.git` (repo internals, e.g. a persisted CI token in `.git/config`), +// `node_modules` (thousands of third-party files, none of them authored +// source), `dist` (build output). Named once and used at BOTH skip sites +// below: the cheap `entry.name` fast path in `walk`, and the resolved-path +// component check in `isUnderSkippedDir` — a symlink whose OWN name is not +// in this set but whose target resolves through a directory that IS (e.g. +// `src/g -> ../.git`, `src/nm -> ../node_modules`) must still be skipped, or +// the name-only check is a trivial bypass. +const SKIP_DIR_NAMES = new Set(['node_modules', 'dist', '.git']); + +/** + * Read the JS regex literal starting at `line[start]` (which must be `/`). + * Returns `{ text, end }` — `text` includes the delimiters and any trailing + * flags, `end` is the index one past the literal — or null if no literal + * closes within MAX_REGEX_LITERAL_LEN characters. + * + * Single left-to-right pass, no backtracking. It models the two constructs a + * backtracking pattern gets wrong, which is why this is a tokenizer and not a + * regex: + * - `\x` escapes consume BOTH characters, so an escaped `\/` never + * terminates the literal; + * - inside a `[...]` character class a bare `/` does NOT terminate, so + * `/PLAN[\\/].*\.md$/` is one literal rather than two fragments. The + * previous regex silently MISSED every re-derivation using a + * cross-platform path-separator class for exactly this reason. + */ +function readRegexLiteralAt(line, start) { + if (line[start] !== '/') return null; + const limit = Math.min(line.length, start + MAX_REGEX_LITERAL_LEN); + let inClass = false; + for (let i = start + 1; i < limit; i++) { + const ch = line[i]; + if (ch === '\\') { + i++; // escape consumes the next character, whatever it is + continue; + } + if (ch === '\r' || ch === '\n') return null; // a literal cannot span lines + if (ch === '[') { + inClass = true; + } else if (ch === ']') { + inClass = false; + } else if (ch === '/' && !inClass) { + // Trailing flags are bounded by the SAME `limit` as the literal body + // itself (not `line.length`) — a literal followed by an unbounded run + // of lowercase letters must not make `text` grow past + // MAX_REGEX_LITERAL_LEN either. + let end = i + 1; + while (end < limit && line[end] >= 'a' && line[end] <= 'z') end++; + return { text: line.slice(start, end), end }; + } + } + return null; +} + +// Symlinks report `isDirectory()`/`isFile()` as false on the Dirent from +// `readdirSync`, so a symlinked `.cts` (or a symlinked directory containing +// one) was previously invisible to this scanner — an evasion of a guard +// whose stated design principle (ADR-3180 Decision 4a) is whole-repo +// discovery with no allowlist. Resolve each entry with `fs.statSync` (which +// follows symlinks) to classify it, skipping broken links. `ctx.visitedRealDirs` +// guards against a symlink cycle sending `walk` into infinite recursion. +// +// Every sibling drift guard in `scripts/` that does NOT import this module +// (`lint-phase-id-drift.cjs`, `lint-package-identity-drift.cjs`, +// `lint-portable-timeout.cjs`, `lint-test-file-count.cjs`, +// `lint-allow-test-rule-refs.cjs`) uses the `Dirent` classification straight +// off `readdirSync` and does NOT follow symlinks at all. This scanner follows +// them so a symlinked source file cannot evade ADR-3180 Decision 4a's +// whole-repo discovery — root confinement (`isInsideRoot` below) is the price +// of doing so: without it, a symlink planted anywhere under a scan directory +// could walk this scanner out to read and report arbitrary files elsewhere on +// disk. +// +// DIRECTORY vs FILE symlinks are confined to two DIFFERENT roots, tracked as +// `ctx.scanDirRoot` (the realpath of the current top-level scan-dir entry, +// e.g. `/src`) vs `ctx.realRoot` (the whole repo): +// - a DIRECTORY symlink is descended ONLY if its resolved realpath is +// inside `ctx.scanDirRoot` — NOT merely inside `ctx.realRoot`. Without +// this, `src/up -> ..` (or `-> `) resolves inside the repo +// root and `walk` descends the ENTIRE repo, reporting violations under +// paths like `tests/not-src.cts` or `docs/other.cts` — files the caller's +// scan-dir list scopes it OUT of. This is a deliberate, fail-CLOSED +// trade-off: a directory symlink pointing elsewhere INSIDE the repo (but +// outside the scan directory) is simply not followed. The alternative — +// descending it — is exactly the whole-repo sweep this rule exists to +// prevent, and a fork PR could use that sweep to redden `lint:ci` on +// files this guard was never meant to read. The narrower rule is worth +// more than the missed edge case. +// - a FILE symlink is still scanned if its resolved realpath is inside +// `ctx.realRoot` (the whole repo, not just the scan directory) — this is +// what keeps `src/alias.cts -> vendor/real.cts` covered (test (f)): an +// aliased file genuinely is part of the compiled surface even when its +// real target lives outside `src/`, and it is still reported under its +// canonical (real) path. +// +// A resolved path is inside a root only if it IS that root or begins with +// root + separator — a plain `startsWith(root)` would also accept a sibling +// directory whose name merely starts with the root's name (`/repo-evil`). +function isInsideRoot(realPath, realRoot) { + return realPath === realRoot || realPath.startsWith(realRoot + path.sep); +} + +// True when `realPath` (already confirmed inside `realRoot` by `isInsideRoot`) +// resolves THROUGH a skip-list directory anywhere along its path relative to +// the root — not just when `realPath` itself IS one. This is what closes the +// symlink bypass the `entry.name` fast path alone cannot: `walk` tests +// `entry.name` (the symlink's OWN name in its parent directory), but a +// symlink named something innocuous can still RESOLVE into `.git` / +// `node_modules` / `dist` (`src/g -> ../.git`, `src/leak.cts -> +// ../.git/config`, `src/nm -> ../node_modules`) — `isInsideRoot` alone admits +// all three, because every one of those real paths is still under the root. +function isUnderSkippedDir(realPath, realRoot) { + const rel = path.relative(realRoot, realPath); + return rel.split(path.sep).some((segment) => SKIP_DIR_NAMES.has(segment)); +} + +// `ctx.scanExt` is a `Set` of file extensions (e.g. `.cts`/`.ts`/`.mts`) the +// caller wants reported — threaded through `ctx` rather than as a positional +// parameter so recursive `walk(full, acc, ctx)` calls stay unchanged. +function walk(dir, acc, ctx) { + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return acc; + } + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (SKIP_DIR_NAMES.has(entry.name)) continue; // cheap fast path + let stat; + try { + stat = entry.isSymbolicLink() ? fs.statSync(full) : entry; + } catch { + continue; // broken symlink target + } + let realPath; + try { + realPath = fs.realpathSync(full); + } catch { + continue; // broken symlink target (race, or a link stat() followed but realpath cannot) + } + if (stat.isDirectory()) { + // Directories (symlinked or real) are confined to the CURRENT scan + // directory root, not merely the repo root — see the comment above + // `isInsideRoot` for why (`src/up -> ..` whole-repo sweep). + if (!isInsideRoot(realPath, ctx.scanDirRoot)) continue; + if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir + if (ctx.visitedRealDirs.has(realPath)) continue; // symlink cycle guard + ctx.visitedRealDirs.add(realPath); + walk(full, acc, ctx); + } else if (stat.isFile() && ctx.scanExt.has(path.extname(entry.name))) { + // Files are confined to the whole repo root — a symlinked FILE whose + // real target lives outside the scan directory but inside the repo + // (e.g. `src/alias.cts -> vendor/real.cts`) is still part of the + // compiled surface and must be scanned. + if (!isInsideRoot(realPath, ctx.realRoot)) continue; + if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir + if (ctx.visitedRealFiles.has(realPath)) continue; // two symlinks, same real file + ctx.visitedRealFiles.add(realPath); + acc.push(realPath); + } + } + return acc; +} + +/** + * Whole-repo tree-walk driver shared by every `lint--drift.cjs` + * guard. Resolves `root`, walks each of `scanDirs` filtered to `scanExt` + * (symlink-following, root-confined, cycle-guarded — see `walk` above), reads + * each discovered file, and calls `onFile(relPath, text)` for it — `relPath` + * is repo-relative and already the file's canonical (real) path, so a + * per-file exemption keyed on `relPath` matches consistently regardless of + * which symlink reached it. + * + * `onFile` returns an array of violation objects (or an empty array / null / + * undefined for "no violations in this file"); `scanTree` flattens them all + * into one returned array. Pure I/O orchestration — detection logic lives + * entirely in the caller's `onFile`. + */ +function scanTree({ root, scanDirs, scanExt, onFile }) { + const violations = []; + let realRoot; + try { + realRoot = fs.realpathSync(root); + } catch { + return violations; // root itself does not exist / is unreadable + } + for (const dir of scanDirs) { + const scanDirPath = path.join(root, dir); + let scanDirRoot; + try { + scanDirRoot = fs.realpathSync(scanDirPath); + } catch { + continue; // scan directory itself does not exist / is unreadable + } + const ctx = { realRoot, scanDirRoot, scanExt, visitedRealDirs: new Set(), visitedRealFiles: new Set() }; + for (const file of walk(scanDirPath, [], ctx)) { + const rel = path.relative(realRoot, file); + let text; + try { + text = fs.readFileSync(file, 'utf8'); + } catch { + continue; + } + const found = onFile(rel, text); + if (found && found.length > 0) violations.push(...found); + } + } + return violations; +} + +// A reported fragment AND a reported file path are both attacker-controlled +// source text on a fork PR (a repo can legally track a filename containing +// control bytes, so the path is exactly as attacker-controlled as the +// fragment), and both are written straight to a CI log. Replace C0/C1 +// control bytes (ANSI escapes included) with a visible \xNN, AND the +// non-Latin-1 formatting/bidi/line-separator codepoints below with \uNNNN, so +// a crafted literal or filename cannot rewrite the terminal rendering of the +// report or hide/reorder its own text: +// - U+200B-U+200F: zero-width space/joiners and directional marks +// - U+2028/U+2029: Unicode LINE SEPARATOR / PARAGRAPH SEPARATOR (line +// breaks a `\n`-only log scan would not catch) +// - U+202A-U+202E: bidi embedding/override controls (RLO etc.) +// - U+2066-U+2069: bidi isolate controls +function sanitizeForReport(text) { + return text + // eslint-disable-next-line no-control-regex -- the control range IS the target + .replace(/[\x00-\x1f\x7f-\x9f]/g, (c) => '\\x' + c.charCodeAt(0).toString(16).padStart(2, '0')) + .replace(/[\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069]/g, (c) => '\\u' + c.charCodeAt(0).toString(16).padStart(4, '0')); +} + +module.exports = { + SKIP_DIR_NAMES, + isInsideRoot, + isUnderSkippedDir, + walk, + readRegexLiteralAt, + MAX_REGEX_LITERAL_LEN, + sanitizeForReport, + scanTree, +}; diff --git a/scripts/lint-milestone-window-drift.cjs b/scripts/lint-milestone-window-drift.cjs new file mode 100644 index 000000000..49aa81685 --- /dev/null +++ b/scripts/lint-milestone-window-drift.cjs @@ -0,0 +1,300 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Anti-divergence drift guard for the milestone-WINDOWING seam + * (epic #3180, issue #3184, ADR-3180 "Planning Semantic Model Single Owner"). + * + * `src/roadmap-parser.cts` is the SINGLE canonical owner of "where does a + * given milestone's ROADMAP section begin and end" — `computeMilestoneSectionEnd`, + * `locateMilestoneHeadings`, and `isMilestoneBoundedInRoadmap`. Every other + * module that hand-rolls a heading-level quantifier together with the + * milestone-boundary shape (a non-Phase heading carrying a version token or a + * shipped/active marker) is a re-derivation that can silently drift from the + * owner — the exact defect class #2562 fixed in one copy and never reached + * the other two (design doc: `currentMilestoneRawRanges::computeSectionEnd` + * carried a "keep in sync" comment that was already evidence the risk was + * known, not controlled). + * + * Per ADR-3180 Decision 4(a) this guard discovers call sites by SCANNING THE + * WHOLE `src/` TREE, not by consulting an allowlist of known files — an + * allowlist only measures re-derivations in files someone remembered to + * list. Exemptions below are FUNCTION-SCOPED with a written reason, never a + * bare file allowlist, mirroring `lint-plan-count-drift.cjs`'s precedent. + * + * Detection is intentionally NARROW, mirroring the plan-count-drift and + * phase-id-drift precedents: a line is a re-derivation when it carries BOTH, + * in ONE source line: + * (a) a markdown heading-level quantifier token — `#{N,M}`, e.g. `#{1,3}`, + * `#{2,3}`, `#{2,4}` — inside either a regex literal or a + * quoted/backticked string, AND + * (b) a milestone-window token: either the negative-lookahead phase + * exclusion (`(?!Phase` / `(?!Phase\s+\S)`) or the milestone + * boundary-marker set — a `v\d+\.\d+`-shaped version token appearing + * together with any of the ✅ 📋 🚧 shipped/active markers. + * Token (b) is deliberately narrow: a PHASE-heading regex (`#{2,4}\s*Phase`) + * carries (a) alone, constantly, throughout this codebase (phase-numbering, + * plan-index, wave-scheduling call sites) and must NOT be flagged — it asks + * "where is phase N's heading", a different, already-single-owned question + * (#2121). Only a line that ALSO carries the milestone-boundary shape — the + * one `computeMilestoneSectionEnd`/`locateMilestoneHeadings` compute — is a + * candidate re-derivation of THIS derivation. + * + * Both `(a)` and `(b)` must be readable through JS regex-literal AND + * string/template-literal escaping: the two pre-#3184 `state.cts` + * re-derivations this design is modelled on were + * `new RegExp(\`^#{1,3}\\s+(?!Phase\\s+\\S)...\`)` — i.e. the SAME source + * text as a real `/.../ ` regex literal, just doubly backslash-escaped + * because it lives inside a template literal. `HEADING_QUANTIFIER_RE` and + * `PHASE_LOOKAHEAD_RE` match either escaping level unchanged (no backslash + * appears inside `#{`/`}`/`(?!Phase`'s literal characters); `VERSION_TOKEN_RE` + * explicitly tolerates ONE or TWO backslashes before each `d`/`.` for exactly + * this reason. Regex-LITERAL boundaries (used only to extract a reportable + * `found` fragment, never for detection itself, which tests the raw line) are + * located via the shared `readRegexLiteralAt` tokenizer + * (`scripts/lib/drift-scan.cjs`) — a single left-to-right, no-backtracking + * pass — never a backtracking "find the regex literal" regex (CodeQL js/redos + * runs on `lint:ci`; see that module's own header for the full rationale). + * `readStringLiteralAt` below is the same style, written locally for + * quoted/backticked strings (not shared — `lint-plan-count-drift.cjs` has no + * equivalent need, since its own literal-bearing shape is regex-only). + * + * Owner file (exempt by construction): `src/roadmap-parser.cts` — it not only + * DEFINES this grammar but composes `#{1,3}` with `(?!Phase...)`/marker + * alternations at several internal call sites (`computeMilestoneSectionEnd`, + * `locateMilestoneHeadings`, `extractCurrentMilestoneScoped`'s + * `anyMilestonePattern`/`anyMilestoneOrDetails`) that are the canonical + * implementation, not copies of it. + * + * The tree-walk / root-confinement / regex-literal-tokenizer / sanitizer + * machinery is SHARED with `scripts/lint-plan-count-drift.cjs` via + * `scripts/lib/drift-scan.cjs` (ADR-3180 Decision 4, design doc's own + * "Rejected: let the new drift guard copy Phase 1's tree-walk / + * root-confinement / sanitizer") — see that module for the `isInsideRoot` + * case-sensitivity note, the `walk` symlink-confinement rationale, and the + * `readRegexLiteralAt` ReDoS-avoidance rationale. + * + * KNOWN, ACCEPTED limits of a per-line textual scan (same tradeoff the + * sibling drift guards document): a re-derivation whose `(a)`/`(b)` tokens + * are split across two DIFFERENT lines with no single line carrying both is + * not caught by this narrow shape, nor is one routed through dynamic + * dispatch. That is left to code review and the design's identity test + * (ADR-3180 Decision 4b/4c), not this regex. + */ + +const path = require('node:path'); +const driftScan = require('./lib/drift-scan.cjs'); +const { readRegexLiteralAt, MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree } = driftScan; + +// (a) A markdown heading-level quantifier: `#{N,M}` — e.g. `#{1,3}`, +// `#{2,3}`, `#{2,4}`. Bounded to 1-2 digit levels (real Markdown headings +// never exceed level 6) so this stays a small, fixed, linear test — no +// unbounded quantifier, nothing for CodeQL js/redos to flag. +const HEADING_QUANTIFIER_RE = /#\{\d{1,2},\d{1,2}\}/; + +// (b1) The negative-lookahead phase exclusion `computeMilestoneSectionEnd`/ +// `locateMilestoneHeadings` use to skip `### Phase N: …` headings while +// scanning for the NEXT milestone boundary. +const PHASE_LOOKAHEAD_RE = /\(\?!Phase\b/; + +// (b2) A `v\d+\.\d+`-shaped version token, tolerant of ONE or TWO backslash +// escaping levels (a bare regex literal carries `\d`/`\.` with a single +// backslash; a template-literal regex SOURCE string carries the SAME source +// text doubly-escaped, `\\d`/`\\.`, because the template literal's own +// backslash must itself be escaped in the .cts source) and an OPTIONAL +// capturing group immediately around the digit run (`v(\d+)\.\d+`, the shape +// `roadmap-command-router.cts`'s `MILESTONE_RE` actually uses to capture the +// major version number). +const VERSION_TOKEN_RE = /v\(?\\{1,2}d\+\)?\\{1,2}\.\\{1,2}d\+/; + +// (b2) The milestone shipped/active marker set `isClosedMilestoneHeading`/ +// `computeMilestoneSectionEnd` test for. `(b)` fires when this appears on the +// SAME line as a VERSION_TOKEN_RE match — a version token alone is not +// milestone-boundary-specific (plenty of non-heading code compares version +// strings), and a marker alone is not either (it can appear in unrelated +// prose-matching code); together, on one line, they are the boundary shape. +const MARKER_EMOJI_RE = /[✅📋🚧]/u; + +// Authored TypeScript source only (the generated bin/lib/*.cjs mirror it). +const SCAN_DIRS = ['src']; +const SCAN_EXT = new Set(['.cts', '.ts', '.mts']); + +// The canonical owner defines the grammar; it is exempt by construction (see +// header comment for why its OWN internal composition of these tokens is not +// a re-derivation). +const OWNER_FILE = path.join('src', 'roadmap-parser.cts'); + +// Per ADR-3180 Decision 4(a): NOT a bare file allowlist — each entry below is +// scoped to the SPECIFIC function asking a documented, DIFFERENT question, so +// an unrelated re-derivation added anywhere else in these same files is still +// caught. Mirrors `lint-plan-count-drift.cjs`'s FUNCTION_SCOPED_EXEMPTIONS +// mechanism. +// +// - roadmap-command-router.cts checkW021: `MILESTONE_RE` CLASSIFIES a +// single heading LINE as "is this a milestone heading, and if so what is +// its major version" for the W021 phase/milestone-prefix-mismatch check +// — it is a per-line classifier consumed one line at a time via +// `content.split('\n')`, with no concept of a section END at all. It +// never computes "where does this milestone's content stop" — the +// question `computeMilestoneSectionEnd` answers — so it cannot diverge +// from that computation; it answers a narrower, different question this +// derivation does not own. +// - verify.cts checkMilestonePrefixMismatches: `sectionRx` ENUMERATES +// every milestone heading in the document to build a list of +// `{version, start, end}` sections (each section's `end` is provisionally +// "rest of document" until the NEXT heading is found, then backfilled) — +// it is answering "what are ALL the milestone sections", to check every +// phase against its OWN enclosing milestone, not "where does THIS ONE +// milestone (the current/asserted one) end" — `computeMilestoneSectionEnd` +// takes a single heading and returns a single boundary; this function +// never calls anything with that shape. (Design brief named this +// `cmdValidateConsistency` — the code actually lives in the sibling +// function `checkMilestonePrefixMismatches`, called from +// `cmdValidateHealth`; `cmdValidateConsistency` itself does not contain +// `sectionRx`. Exempted here under its ACTUAL containing function.) Also: +// `sectionRx` (`/^#{1,3}\s+(?:\[[^\]]{1,200}\]\s*)?.*v(\d+\.\d+)/gim`) +// does not itself carry token (b) as this guard defines it (no +// `(?!Phase` lookahead, no marker-emoji pairing) — this exemption +// currently documents intent rather than suppressing a live match. +const FUNCTION_SCOPED_EXEMPTIONS = new Map([ + [path.join('src', 'roadmap-command-router.cts'), new Set(['checkW021'])], + [path.join('src', 'verify.cts'), new Set(['checkMilestonePrefixMismatches'])], +]); + +// Optional `export ` modifier, mirroring `lint-plan-count-drift.cjs`'s +// TOP_LEVEL_FUNCTION_RE — only a column-0 top-level `function` declaration +// updates the current-function tracker; a nested/arrow function does not +// reset it, matching every FUNCTION_SCOPED_EXEMPTIONS entry above (all +// top-level `function` declarations). +const TOP_LEVEL_FUNCTION_RE = /^(?:export\s+)?function\s+([A-Za-z0-9_]+)\s*\(/; + +/** + * Read the quoted or backtick-delimited string/template literal starting at + * `line[start]` (which must be `'`, `"`, or `` ` ``). Returns `{ text, end }` + * — `text` includes both delimiters, `end` is the index one past the literal + * — or null if no matching close quote is found within MAX_REGEX_LITERAL_LEN + * characters. Same single left-to-right, no-backtracking, escape-aware style + * as the shared `readRegexLiteralAt` (`\x` escapes consume both characters, + * so an escaped quote never terminates the literal early) — written locally + * because `lint-plan-count-drift.cjs` has no equivalent need (its literal + * shape is regex-only), so it does not belong in the shared module. + */ +function readStringLiteralAt(line, start) { + const quote = line[start]; + if (quote !== "'" && quote !== '"' && quote !== '`') return null; + const limit = Math.min(line.length, start + MAX_REGEX_LITERAL_LEN); + for (let i = start + 1; i < limit; i++) { + const ch = line[i]; + if (ch === '\\') { + i++; // escape consumes the next character, whatever it is + continue; + } + if (ch === '\r' || ch === '\n') return null; // a literal cannot span lines in this per-line scan + if (ch === quote) return { text: line.slice(start, i + 1), end: i + 1 }; + } + return null; +} + +/** + * The first literal (regex OR quoted/backtick string) on `line` whose text + * contains a HEADING_QUANTIFIER_RE match — the "smoking gun" fragment worth + * reporting, mirroring `findRegexLiteralMdMatch`'s role in the sibling guard. + * Falls back to a bounded, trimmed slice of the raw line when the tokens are + * not both inside one located literal (not currently reachable against this + * repo — see the header comment's per-file audit — but a fail-safe rather + * than a thrown error if a future line splits them). + */ +function extractFragment(line) { + for (let i = 0; i < line.length; i++) { + const ch = line[i]; + let literal = null; + 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; + i = literal.end - 1; // resume scanning just past this literal + } + return line.trim().slice(0, MAX_REGEX_LITERAL_LEN); +} + +/** + * Pure: find every unsanctioned milestone-window re-derivation in `text`. + * `relPath` is the repo-relative path, used both to report file:line and to + * apply the narrow, function-scoped exemptions above. + * Returns [{ line, found }]. + */ +function findMilestoneWindowDrift(text, relPath) { + const out = []; + const lines = text.split('\n'); + const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null; + let currentFunction = null; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const fnMatch = TOP_LEVEL_FUNCTION_RE.exec(line); + if (fnMatch) currentFunction = fnMatch[1]; + + if (!HEADING_QUANTIFIER_RE.test(line)) continue; + const isMilestoneWindowToken = PHASE_LOOKAHEAD_RE.test(line) || (VERSION_TOKEN_RE.test(line) && MARKER_EMOJI_RE.test(line)); + if (!isMilestoneWindowToken) continue; + + if (exemptFunctions && exemptFunctions.has(currentFunction)) continue; + + out.push({ line: i + 1, found: extractFragment(line) }); + } + return out; +} + +/** + * Scan the authored source tree and return every unsanctioned re-derivation, + * each annotated with the repo-relative file path. + */ +function scanRepo(root) { + return scanTree({ + root, + scanDirs: SCAN_DIRS, + scanExt: SCAN_EXT, + onFile(rel, text) { + // `rel` is already the REAL (canonical) path (scanTree resolves + // symlinks before calling onFile), so this comparison — and + // FUNCTION_SCOPED_EXEMPTIONS above, also keyed on `rel` — match + // consistently regardless of which symlink reached the file. + if (rel === OWNER_FILE) return []; + return findMilestoneWindowDrift(text, rel).map((d) => ({ file: rel, ...d })); + }, + }); +} + +function main() { + const root = path.join(__dirname, '..'); + const violations = scanRepo(root); + if (violations.length === 0) { + process.stdout.write('ok milestone-window-drift: no unsanctioned milestone-window re-derivations outside roadmap-parser.cts\n'); + return; + } + process.stderr.write('milestone-window-drift: independent re-derivation(s) of milestone-window bounding found.\n'); + process.stderr.write('Use src/roadmap-parser.cjs `computeMilestoneSectionEnd` / `locateMilestoneHeadings` /\n'); + process.stderr.write('`isMilestoneBoundedInRoadmap` instead of re-deriving the milestone heading/boundary regex:\n'); + for (const d of violations) { + // `d.file` is exactly as attacker-controlled as `d.found`: a repo can + // legally track a filename containing control bytes / bidi overrides, + // and it is a fork-PR-authored value reaching a CI log the same way the + // matched literal does — sanitize it at the same reporting boundary. + process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${sanitizeForReport(d.found)}\n`); + } + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { + findMilestoneWindowDrift, + scanRepo, + HEADING_QUANTIFIER_RE, + PHASE_LOOKAHEAD_RE, + VERSION_TOKEN_RE, + MARKER_EMOJI_RE, + OWNER_FILE, + FUNCTION_SCOPED_EXEMPTIONS, + readStringLiteralAt, + extractFragment, +}; diff --git a/scripts/lint-plan-count-drift.cjs b/scripts/lint-plan-count-drift.cjs index 6314ef04e..10fc5ff10 100644 --- a/scripts/lint-plan-count-drift.cjs +++ b/scripts/lint-plan-count-drift.cjs @@ -47,18 +47,14 @@ * across two DIFFERENT lines with no single line carrying both, is not * caught by this narrow shape. That is left to code review, not this regex. * - * `isInsideRoot`'s root-confinement check (used by the symlink-following - * `walk`, below) is an EXACT string comparison, deliberately not - * case-normalized. On a case-insensitive filesystem (macOS default; not CI, - * which is ubuntu) a symlink whose target is a case-VARIANT of an in-root - * path — a path the OS itself would still resolve to the same file — is - * REJECTED by this exact comparison and silently left unscanned. This is a - * fail-CLOSED miss (a re-derivation goes unreported), never an escape (never - * a wrongly-admitted outside-root read), so it is left as-is: making the - * comparison case-insensitive would WEAKEN confinement (a resolved path - * merely case-differing from a sibling-of-root name, `/repo-Evil` vs - * `/repo-evil`, could then be wrongly admitted) to fix a gap that only ever - * under-reports on a platform this guard is not gated on. + * The tree-walk / root-confinement / regex-literal-tokenizer / sanitizer + * machinery below is SHARED with `scripts/lint-milestone-window-drift.cjs` + * (#3184) via `scripts/lib/drift-scan.cjs` — see that module for the + * `isInsideRoot` case-sensitivity note, the `walk` symlink-confinement + * rationale, and the `readRegexLiteralAt` tokenizer's ReDoS-avoidance + * rationale. It is deliberately NOT duplicated here a second time (ADR-3180 + * Decision 4's own "Rejected" list: "let the new drift guard copy Phase 1's + * tree-walk / root-confinement / sanitizer"). * * A regex literal longer than MAX_REGEX_LITERAL_LEN (400) characters is not * read, and is therefore not caught. That bound is what keeps the scan @@ -74,8 +70,9 @@ * survive, and the reason the detector is now a tokenizer. */ -const fs = require('node:fs'); const path = require('node:path'); +const driftScan = require('./lib/drift-scan.cjs'); +const { readRegexLiteralAt, MAX_REGEX_LITERAL_LEN, isInsideRoot, sanitizeForReport, scanTree } = driftScan; // A `.filter(` call on the line — the shape every current re-derivation uses // to turn a directory listing into a plan-or-summary subset. Kept as its own @@ -96,15 +93,6 @@ const FILENAME_TEST_RE = /\.(?:filter|test|match|exec|endsWith|startsWith|includ // closing quote must match). const PLAN_SUMMARY_LITERAL_RE = /(['"])-?(?:PLAN|SUMMARY)\.md\1/; -// Longest regex literal this scanner will consider, in characters. Real -// plan/summary filename filters are far shorter; the bound is what keeps the -// scan linear. The tokenizer restarts at every `/` on the line (so that a -// literal preceded by a stray unpaired `/` is still found, matching the -// previous regex's "find anywhere" behaviour), which without a per-literal -// bound would be quadratic on a pathological line. With it the whole-line -// cost is O(n * MAX_REGEX_LITERAL_LEN) with no backtracking at all. -const MAX_REGEX_LITERAL_LEN = 400; - // The two tokens that, appearing together INSIDE one regex literal, make it a // plan/summary filename filter. `\.md` is matched as literal source text, not // as a pattern, so there is nothing here to backtrack. @@ -115,17 +103,6 @@ const ESCAPED_MD_TOKEN = '\\.md'; const SCAN_DIRS = ['src']; const SCAN_EXT = new Set(['.cts', '.ts', '.mts']); -// Directory names this scanner never descends into or reports out of — -// `.git` (repo internals, e.g. a persisted CI token in `.git/config`), -// `node_modules` (thousands of third-party files, none of them authored -// source), `dist` (build output). Named once and used at BOTH skip sites -// below: the cheap `entry.name` fast path in `walk`, and the resolved-path -// component check in `isUnderSkippedDir` — a symlink whose OWN name is not -// in this set but whose target resolves through a directory that IS (e.g. -// `src/g -> ../.git`, `src/nm -> ../node_modules`) must still be skipped, or -// the name-only check is a trivial bypass. -const SKIP_DIR_NAMES = new Set(['node_modules', 'dist', '.git']); - // The canonical owner defines the grammar; it is exempt by construction. const OWNER_FILE = path.join('src', 'plan-scan.cts'); @@ -197,50 +174,6 @@ const FUNCTION_SCOPED_EXEMPTIONS = new Map([ // FUNCTION_SCOPED_EXEMPTIONS entry above to take effect. const TOP_LEVEL_FUNCTION_RE = /^(?:export\s+)?function\s+([A-Za-z0-9_]+)\s*\(/; -/** - * Read the JS regex literal starting at `line[start]` (which must be `/`). - * Returns `{ text, end }` — `text` includes the delimiters and any trailing - * flags, `end` is the index one past the literal — or null if no literal - * closes within MAX_REGEX_LITERAL_LEN characters. - * - * Single left-to-right pass, no backtracking. It models the two constructs a - * backtracking pattern gets wrong, which is why this is a tokenizer and not a - * regex: - * - `\x` escapes consume BOTH characters, so an escaped `\/` never - * terminates the literal; - * - inside a `[...]` character class a bare `/` does NOT terminate, so - * `/PLAN[\\/].*\.md$/` is one literal rather than two fragments. The - * previous regex silently MISSED every re-derivation using a - * cross-platform path-separator class for exactly this reason. - */ -function readRegexLiteralAt(line, start) { - if (line[start] !== '/') return null; - const limit = Math.min(line.length, start + MAX_REGEX_LITERAL_LEN); - let inClass = false; - for (let i = start + 1; i < limit; i++) { - const ch = line[i]; - if (ch === '\\') { - i++; // escape consumes the next character, whatever it is - continue; - } - if (ch === '\r' || ch === '\n') return null; // a literal cannot span lines - if (ch === '[') { - inClass = true; - } else if (ch === ']') { - inClass = false; - } else if (ch === '/' && !inClass) { - // Trailing flags are bounded by the SAME `limit` as the literal body - // itself (not `line.length`) — a literal followed by an unbounded run - // of lowercase letters must not make `text` grow past - // MAX_REGEX_LITERAL_LEN either. - let end = i + 1; - while (end < limit && line[end] >= 'a' && line[end] <= 'z') end++; - return { text: line.slice(start, end), end }; - } - } - return null; -} - /** * The regex literal on `line` that mentions PLAN or SUMMARY together with an * escaped `.md` suffix — e.g. `/-PLAN\.md$/`, `/^PLAN-\d+.*\.md$/i`, @@ -265,113 +198,6 @@ function findRegexLiteralMdMatch(line) { return null; } -// Symlinks report `isDirectory()`/`isFile()` as false on the Dirent from -// `readdirSync`, so a symlinked `src/*.cts` (or a symlinked directory -// containing one) was previously invisible to this scanner — an evasion of a -// guard whose stated design principle (ADR-3180 Decision 4a) is whole-repo -// discovery with no allowlist. Resolve each entry with `fs.statSync` (which -// follows symlinks) to classify it, skipping broken links. `ctx.visitedRealDirs` -// guards against a symlink cycle sending `walk` into infinite recursion. -// -// Every sibling drift guard in `scripts/` (`lint-phase-id-drift.cjs`, -// `lint-package-identity-drift.cjs`, `lint-portable-timeout.cjs`, -// `lint-test-file-count.cjs`, `lint-allow-test-rule-refs.cjs`) uses the -// `Dirent` classification straight off `readdirSync` and does NOT follow -// symlinks at all. This guard follows them so a symlinked `src/*.cts` cannot -// evade ADR-3180 Decision 4a's whole-repo discovery — root confinement -// (`isInsideRoot` below) is the price of doing so: without it, a symlink -// planted anywhere under `src/` could walk this scanner out to read and -// report arbitrary files elsewhere on disk. -// -// DIRECTORY vs FILE symlinks are confined to two DIFFERENT roots, tracked as -// `ctx.scanDirRoot` (the realpath of the current top-level SCAN_DIRS entry, -// e.g. `/src`) vs `ctx.realRoot` (the whole repo): -// - a DIRECTORY symlink is descended ONLY if its resolved realpath is -// inside `ctx.scanDirRoot` — NOT merely inside `ctx.realRoot`. Without -// this, `src/up -> ..` (or `-> `) resolves inside the repo -// root and `walk` descends the ENTIRE repo, reporting violations under -// paths like `tests/not-src.cts` or `docs/other.cts` — files the header -// above says the scan is scoped OUT of (`SCAN_DIRS`). This is a -// deliberate, fail-CLOSED trade-off: a directory symlink pointing -// elsewhere INSIDE the repo (but outside the scan directory) is simply -// not followed. The alternative — descending it — is exactly the -// whole-repo sweep this rule exists to prevent, and a fork PR could use -// that sweep to redden `lint:ci` on files this guard was never meant to -// read. The narrower rule is worth more than the missed edge case. -// - a FILE symlink is still scanned if its resolved realpath is inside -// `ctx.realRoot` (the whole repo, not just the scan directory) — this is -// what keeps `src/alias.cts -> vendor/real.cts` covered (test (f)): an -// aliased file genuinely is part of the compiled surface even when its -// real target lives outside `src/`, and it is still reported under its -// canonical (real) path. -// -// A resolved path is inside a root only if it IS that root or begins with -// root + separator — a plain `startsWith(root)` would also accept a sibling -// directory whose name merely starts with the root's name (`/repo-evil`). -function isInsideRoot(realPath, realRoot) { - return realPath === realRoot || realPath.startsWith(realRoot + path.sep); -} - -// True when `realPath` (already confirmed inside `realRoot` by `isInsideRoot`) -// resolves THROUGH a skip-list directory anywhere along its path relative to -// the root — not just when `realPath` itself IS one. This is what closes the -// symlink bypass the `entry.name` fast path alone cannot: `walk` tests -// `entry.name` (the symlink's OWN name in its parent directory), but a -// symlink named something innocuous can still RESOLVE into `.git` / -// `node_modules` / `dist` (`src/g -> ../.git`, `src/leak.cts -> -// ../.git/config`, `src/nm -> ../node_modules`) — `isInsideRoot` alone admits -// all three, because every one of those real paths is still under the root. -function isUnderSkippedDir(realPath, realRoot) { - const rel = path.relative(realRoot, realPath); - return rel.split(path.sep).some((segment) => SKIP_DIR_NAMES.has(segment)); -} - -function walk(dir, acc, ctx) { - let entries; - try { - entries = fs.readdirSync(dir, { withFileTypes: true }); - } catch { - return acc; - } - for (const entry of entries) { - const full = path.join(dir, entry.name); - if (SKIP_DIR_NAMES.has(entry.name)) continue; // cheap fast path - let stat; - try { - stat = entry.isSymbolicLink() ? fs.statSync(full) : entry; - } catch { - continue; // broken symlink target - } - let realPath; - try { - realPath = fs.realpathSync(full); - } catch { - continue; // broken symlink target (race, or a link stat() followed but realpath cannot) - } - if (stat.isDirectory()) { - // Directories (symlinked or real) are confined to the CURRENT scan - // directory root, not merely the repo root — see the comment above - // `isInsideRoot` for why (`src/up -> ..` whole-repo sweep). - if (!isInsideRoot(realPath, ctx.scanDirRoot)) continue; - if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir - if (ctx.visitedRealDirs.has(realPath)) continue; // symlink cycle guard - ctx.visitedRealDirs.add(realPath); - walk(full, acc, ctx); - } else if (stat.isFile() && SCAN_EXT.has(path.extname(entry.name))) { - // Files are confined to the whole repo root — a symlinked FILE whose - // real target lives outside the scan directory but inside the repo - // (e.g. `src/alias.cts -> vendor/real.cts`) is still part of the - // compiled surface and must be scanned. - if (!isInsideRoot(realPath, ctx.realRoot)) continue; - if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir - if (ctx.visitedRealFiles.has(realPath)) continue; // two symlinks, same real file - ctx.visitedRealFiles.add(realPath); - acc.push(realPath); - } - } - return acc; -} - /** * Pure: find every unsanctioned plan/summary-filter re-derivation in `text`. * `relPath` is the repo-relative path, used both to report file:line and to @@ -405,62 +231,19 @@ function findPlanCountDrift(text, relPath) { * each annotated with the repo-relative file path. */ function scanRepo(root) { - const violations = []; - let realRoot; - try { - realRoot = fs.realpathSync(root); - } catch { - return violations; // root itself does not exist / is unreadable - } - for (const dir of SCAN_DIRS) { - const scanDirPath = path.join(root, dir); - let scanDirRoot; - try { - scanDirRoot = fs.realpathSync(scanDirPath); - } catch { - continue; // scan directory itself does not exist / is unreadable - } - const ctx = { realRoot, scanDirRoot, visitedRealDirs: new Set(), visitedRealFiles: new Set() }; - for (const file of walk(scanDirPath, [], ctx)) { - // `file` is already the REAL path (walk pushes realPath, not the - // symlink path), so `rel` is the file's single canonical location - // regardless of which symlink reached it — this is what makes - // FUNCTION_SCOPED_EXEMPTIONS/OWNER_FILE, which are keyed on the - // repo-relative path, match consistently. - const rel = path.relative(realRoot, file); - if (rel === OWNER_FILE) continue; - let text; - try { - text = fs.readFileSync(file, 'utf8'); - } catch { - continue; - } - for (const d of findPlanCountDrift(text, rel)) { - violations.push({ file: rel, ...d }); - } - } - } - return violations; -} - -// Both a `found` fragment AND a reported file path are attacker-controlled -// source text on a fork PR (a repo can legally track a filename containing -// control bytes, so the path is exactly as attacker-controlled as the -// fragment — see the call sites in main() below), and both are written -// straight to a CI log. Replace C0/C1 control bytes (ANSI escapes included) -// with a visible \xNN, AND the non-Latin-1 formatting/bidi/line-separator -// codepoints below with \uNNNN, so a crafted literal or filename cannot -// rewrite the terminal rendering of the report or hide/reorder its own text: -// - U+200B-U+200F: zero-width space/joiners and directional marks -// - U+2028/U+2029: Unicode LINE SEPARATOR / PARAGRAPH SEPARATOR (line -// breaks a `\n`-only log scan would not catch) -// - U+202A-U+202E: bidi embedding/override controls (RLO etc.) -// - U+2066-U+2069: bidi isolate controls -function sanitizeForReport(text) { - return text - // eslint-disable-next-line no-control-regex -- the control range IS the target - .replace(/[\x00-\x1f\x7f-\x9f]/g, (c) => '\\x' + c.charCodeAt(0).toString(16).padStart(2, '0')) - .replace(/[\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069]/g, (c) => '\\u' + c.charCodeAt(0).toString(16).padStart(4, '0')); + return scanTree({ + root, + scanDirs: SCAN_DIRS, + scanExt: SCAN_EXT, + onFile(rel, text) { + // `rel` is already the REAL (canonical) path (scanTree resolves + // symlinks before calling onFile), so this comparison — and + // FUNCTION_SCOPED_EXEMPTIONS above, also keyed on `rel` — match + // consistently regardless of which symlink reached the file. + if (rel === OWNER_FILE) return []; + return findPlanCountDrift(text, rel).map((d) => ({ file: rel, ...d })); + }, + }); } function main() { diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index 358e4a13f..dcef479a6 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -37,6 +37,7 @@ "milestone-helper.test.cjs", "milestone-prefixed-convention.test.cjs", "milestone-summary.test.cjs", + "milestone-window-single-owner.test.cjs", "milestone.test.cjs" ], "issue": "TBD" diff --git a/src/milestone.cts b/src/milestone.cts index 922c17979..df8e5582d 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -29,7 +29,15 @@ import phaseIdMod = require('./phase-id.cjs'); const { escapeRegex, normalizePhaseName, phaseTokenMatches, PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); -const { getMilestonePhaseFilter, extractCurrentMilestone, getMilestoneInfo } = roadmapParserMod; +const { + getMilestonePhaseFilter, + extractCurrentMilestone, + getMilestoneInfo, + sliceMilestoneWindow, +} = 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 coreUtilsMod = require('./core-utils.cjs'); const { extractOneLinerFromBody, countMatchedSummaries } = coreUtilsMod; @@ -520,18 +528,40 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo const today = realClock.localToday(); const milestoneName = options.name || version; - // Ensure archive directory exists (skipped in dry-run — no mutations) - if (!options.dryRun) { - platformEnsureDir(archiveDir); - } - // Scope stats and accomplishments to only the phases belonging to the // current milestone's ROADMAP. Uses the shared filter from roadmap-parser.cjs // (same logic used by cmdPhasesList and other callers). + // #3184 review finding: this scope computation + refusal MUST run BEFORE + // `platformEnsureDir(archiveDir)` below — a refused run (scope not COMPLETE, + // no --force) must be a true no-op on disk, and creating the archive + // directory first left an empty directory behind even on refusal. const isDirInMilestone = getMilestonePhaseFilter(cwd, version); if (isDirInMilestone.missingExplicitVersion) { error(`no phases found for milestone ${version} in ROADMAP.md`); } + // #3184/#3166: `milestone complete` is the ONE-WAY-DOOR consumer of the + // milestone window (ROADMAP/REQUIREMENTS archived, phase directories + // MOVED). #3166 is specifically the TRUNCATED case: the milestone's + // heading IS found but its section closes before the phase region, and the + // phase filter degrades to pass-all (see getMilestonePhaseFilter above) — + // silently archiving every phase directory on disk. UNREADABLE (no + // ROADMAP.md at all) and UNSCOPED (no section for this version) are + // pre-existing, legitimately-handled states — `missingExplicitVersion` + // above already errors where that matters, and a missing ROADMAP.md has + // its own documented graceful path — so only TRUNCATED is refused here. + // The read-path consumers keep the pass-all degrade for every scope + // (ADR-3180 Decision 3's Rejected section: deny-all there would trade one + // silent wrong answer for another); this write path refuses on TRUNCATED + // alone, positioned before `platformEnsureDir` so a refusal stays a no-op + // on disk. + if (isDirInMilestone.scope === SCOPE.TRUNCATED && !options.force) { + error( + `Cannot mark milestone complete: the ROADMAP window for "${version}" is truncated ` + + `(the milestone heading was found but its section ends before reaching any phase ` + + `entries, even though the ROADMAP has phase entries elsewhere), so phase scoping ` + + `cannot be trusted for this destructive operation. Re-run with --force to override.`, + ); + } // Guard: prevent marking complete when ROADMAP still lists phases that have // no directory on disk (disk_status: no_directory). This catches the case @@ -587,7 +617,21 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo } const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); - const scopedContent = extractCurrentMilestone(roadmapContent, cwd); + // #3184/#2946: scope the unstarted-phase guard to the same `version` + // window `getMilestonePhaseFilter` used above, NOT to + // extractCurrentMilestone's own STATE.md-derived window — those two + // can disagree (that disagreement is exactly what the WARNING above + // detects), and scoping this guard to the wrong window under-detects + // unstarted phases on the destructive completion path. Calls the same + // sliceMilestoneWindow owner getMilestonePhaseFilter's versionOverride + // branch calls (a prior pass here re-composed locate+select+section-end + // locally, which review caught as a second, disagreeing derivation of + // the same window — ADR-3180 Decision 4(c)); falls back to + // extractCurrentMilestone's whole-document result only for the + // free-form (no versioned milestones anywhere) shape, where both + // windows converge to the same value regardless of which version drove + // the lookup. + const scopedContent = sliceMilestoneWindow(roadmapContent, version) ?? extractCurrentMilestone(roadmapContent, cwd); // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). const phasePattern = new RegExp(`#{2,4}\\s*Phase\\s+(${PHASE_NUMBER_TOKEN_SOURCE})(?:\\s*\\([^)\\n]{0,200}\\))?\\s*:\\s*([^\\n]+)`, 'gi'); const noDirectoryPhases: string[] = []; @@ -737,6 +781,14 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo return; } + // Ensure archive directory exists. Deliberately placed AFTER the dry-run + // early return and every refusal/guard above (missingExplicitVersion, the + // scope refusal, the unstarted-phase guard) — #3184 review finding: this + // used to run before those checks, so a refused run still left an empty + // archive directory behind. Reaching this point means the run is + // committed to mutating. + platformEnsureDir(archiveDir); + // Archive ROADMAP.md if (fs.existsSync(roadmapPath)) { const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index e77831b85..13e2aa2cb 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -36,8 +36,12 @@ import { platformReadSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import unusableInputMod = require('./unusable-input.cjs'); const { UNUSABLE_REASON, warnUnusableInput } = unusableInputMod; -import { tokenizeHeadings, stripTaggedBlocks, withSection } from './markdown-sectionizer.cjs'; +import { tokenizeHeadings, stripTaggedBlocks, withSection, stripFencedCode } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // ─── Roadmap milestone scoping ─────────────────────────────────────────────── @@ -97,7 +101,194 @@ function isMilestoneShippedInRoadmap(content: string, version: string): boolean } /** - * Extract the current milestone section from ROADMAP.md by positive lookup. + * #3184 (epic #3180 Phase 2): the sole owner of "where does this milestone + * heading's section end". Lifted from `currentMilestoneRawRanges`'s prior + * inline copy — the only one of three byte-identical copies that carried a + * "keep in sync" comment (evidence the risk was known, not controlled). + * `extractCurrentMilestoneScoped`, `currentMilestoneRawRanges`, and + * `getMilestonePhaseFilter`'s versionOverride branch all call this instead of + * re-deriving it. + */ +function computeMilestoneSectionEnd(content: string, headingText: string, headingStart: number): number { + const level = (headingText.match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; + const afterHeading = headingStart + headingText.length; + // Use tokenizeHeadings (fence-aware, offsets into original content) to find + // the next stop boundary without re-implementing fence detection. T4 seam migration. + const headings = tokenizeHeadings(content); + for (const h of headings) { + if (h.offset <= headingStart) continue; + if (h.offset < afterHeading) continue; + if (h.level > level) continue; + // Mirrors old stopPattern: level-bounded, not a Phase heading, milestone marker + if (/^Phase\s+\S/i.test(h.text)) continue; + if (!/v\d+\.\d+|✅|📋|🚧/i.test(h.text)) continue; + return h.offset; + } + return content.length; +} + +/** + * #3184: the sole milestone-heading locator. Boundary-matched on the version + * token with `\b`, NOT the stricter `(?![\w.-])`: this function keeps `\b` + * because a milestone STATE legitimately selects its own sub-milestone + * heading (`v8.0` matching `## v8.0-B …` — `0` is a word char, `-` is not, so + * `\b` matches) — that is deliberate, load-bearing behavior (#730). The + * stricter `(?![\w.-])` boundary answers a DIFFERENT question — "is exactly + * this milestone shipped" (`isMilestoneShippedInRoadmap`) / "which Phase + * Details section belongs to exactly this one's version token" + * (`detailsVersionBoundary`) — and applying it here breaks #730 sub-milestone + * selection. `extractCurrentMilestoneScoped`, `currentMilestoneRawRanges`, + * and `getMilestonePhaseFilter`'s versionOverride branch all consume this + * instead of re-deriving their own heading-location regex. + */ +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', + ); + const matches: RegExpExecArray[] = []; + let m: RegExpExecArray | null; + while ((m = pattern.exec(content)) !== null) { + matches.push(m); + } + return matches; +} + +/** + * #3184: named predicate replacing the two `state.cts` re-derivations + * (`buildStateFrontmatter`, `syncStateFrontmatter`) that each hand-rolled the + * same "is this version bounded to a versioned ROADMAP heading" regex. A + * straight consolidation of the two identical `state.cts` regexes onto the + * shared `locateMilestoneHeadings` owner — no behavior change. + */ +function isMilestoneBoundedInRoadmap(content: string, version: string): boolean { + return locateMilestoneHeadings(content, version).length > 0; +} + +/** + * #3184: does this ROADMAP carry ANY versioned milestone heading (`v1.2`-style + * token on a level 1-3 non-Phase heading), independent of any particular + * version. `extractCurrentMilestoneScoped` (free-form-vs-versioned row 3/4 + * classification) and `getMilestonePhaseFilter` (the deprecation warning + + * the same row 3/4 classification for its versionOverride branch) each + * hand-rolled this identically — the guard does not catch intra-owner-file + * copies by construction, so this was found by review instead. + */ +function hasVersionedMilestones(content: string): boolean { + return /^#{1,3}\s+.*v\d+\.\d+/mi.test(content); +} + +/** + * #3184/#2828/#1761: does this ROADMAP use milestone SECTIONING at all — i.e. + * does it carry any non-Phase heading at level 2-3? Deliberately weaker than + * `hasVersionedMilestones`: this needs to distinguish a FLAT unmilestoned + * roadmap (Phase headings only, where a whole-document phase count is + * correct) from a MILESTONED-but-unbounded one (where that count conflates + * sibling milestones, #1761) — that distinction is load-bearing and must not + * be collapsed into the versioned-milestone check. Owned here so the + * milestone heading vocabulary has one home; routes `state.cts`'s + * `buildStateFrontmatter` #2828 guard instead of a third hand-rolled copy. + */ +function hasMilestoneSectioning(content: string): boolean { + return /^#{2,3}\s+(?!Phase\s+\S)/mi.test(content); +} + +/** + * #3184: the sole "which heading is this milestone's" rule — locate the version's + * headings, prefer the first that is not marked CLOSED/SHIPPED, else fall back to the + * first match. Returns null when the version has no heading at all. + * + * Extracted because three sites had written this same two-line selection + * independently (sliceMilestoneWindow, extractCurrentMilestoneScoped, + * currentMilestoneRawRanges) — the composition-level divergence ADR-3180 + * Decision 4(c) covers: calling the owner's primitives and re-assembling the + * result locally is indistinguishable from re-deriving it. + */ +function selectMilestoneHeading(content: string, version: string): RegExpExecArray | null { + const matches = locateMilestoneHeadings(content, version); + if (matches.length === 0) return null; + return matches.find((m) => !isClosedMilestoneHeading(m[1])) ?? matches[0]; +} + +/** + * #3184: the sole "give me this version's window" composition. Delegates + * heading selection to `selectMilestoneHeading` (the sole selection owner) + * and then to `computeMilestoneSectionEnd` for the slice. Returns null when + * the version has no heading at all, so callers can distinguish "no such + * milestone section" from "empty section". + * + * Review finding (post-merge of this phase's first pass): `getMilestonePhaseFilter`'s + * versionOverride branch and `cmdMilestoneComplete`'s unstarted-phase guard + * had each independently composed `locateMilestoneHeadings` + + * `computeMilestoneSectionEnd` into a window — the SAME derivation written + * twice, and they disagreed (one skipped CLOSED headings, the other did not) + * — exactly the composition-level divergence ADR-3180 Decision 4(c) warns + * about: calling the owner and then re-assembling the result locally is + * indistinguishable from re-deriving it. Both sites now call this instead. + */ +function sliceMilestoneWindow(content: string, version: string): string | null { + const selected = selectMilestoneHeading(content, version); + if (selected === null) return null; + return content.slice(selected.index, computeMilestoneSectionEnd(content, selected[0], selected.index)); +} + +/** + * #3184: counts RAW phase references — a `#{2,4} Phase :` heading + * (fence-aware via `tokenizeHeadings`) or a `#2199` bullet entry — BEFORE any + * sentinel filter. Used for BOTH sides of `classifyMilestoneWindow`'s row-8 + * comparison (does the window contain phase entries; does the document). + * Deliberately does NOT filter `999.x`/Phase 0 sentinels: the question here + * is "did the window reach the phase region", not "how many real phases + * exist" — a window containing only sentinel phases still reached the + * region and must read COMPLETE, not TRUNCATED. + */ +function hasPhaseEntries(markdown: string): boolean { + // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). + const phaseHeadingPattern = /^(?:\[[^\]]{1,200}\]\s*)?Phase\s+([\w][\w.-]*)(?:\s*\([^)\n]{0,200}\))?\s*:/i; + for (const h of tokenizeHeadings(markdown)) { + if (h.level < 2 || h.level > 4) continue; + if (phaseHeadingPattern.test(h.text)) return true; + } + // #3184 review finding: the bullet fallback must be fence-aware too, or a + // FENCED markdown EXAMPLE of the `- [ ] **Phase N — Name**` syntax (e.g. a + // doc showing the convention) counts as a real phase entry. Strip fences + // through the canonical seam before testing, matching tokenizeHeadings' + // fence-awareness above. + return BULLET_PHASE_LINE_PATTERN.test(stripFencedCode(markdown).text); +} + +/** + * #3184: pure decision table (no I/O, no regex construction from caller + * data) implementing the design's Behavior table rows 1-8 (the remaining + * rows 9-17 reduce to one of these six through how the caller constructs its + * input, not additional branches here). Kernighan's Law fired during design: + * `getMilestonePhaseFilter` is already cyclomatic 36, so this discriminator + * is extracted as its own named, separately-testable function rather than + * inlined. + */ +function classifyMilestoneWindow(input: { + readable: boolean; + versionResolved: boolean; + hasVersionedMilestones: boolean; + headingFound: boolean; + windowHasPhaseEntries: boolean; + documentHasPhaseEntries: boolean; +}): Scope { + const { readable, versionResolved, hasVersionedMilestones, headingFound, windowHasPhaseEntries, documentHasPhaseEntries } = input; + return ( + !readable ? SCOPE.UNREADABLE : // row 2 + !versionResolved && !hasVersionedMilestones ? SCOPE.COMPLETE : // row 3: free-form legacy roadmap + !versionResolved && hasVersionedMilestones ? SCOPE.UNSCOPED : // row 4 + versionResolved && !headingFound ? SCOPE.UNSCOPED : // row 5 + headingFound && !windowHasPhaseEntries && documentHasPhaseEntries ? SCOPE.TRUNCATED : // row 8 + SCOPE.COMPLETE // rows 6, 7 + ); +} + +/** + * Extract the current milestone section from ROADMAP.md by positive lookup, + * carrying a `scope` discriminator (ADR-3180 Decision 2) alongside the value. * * @param content - ROADMAP.md content. * @param cwd - Project working directory, used to read the companion STATE.md @@ -106,9 +297,18 @@ function isMilestoneShippedInRoadmap(content: string, version: string): boolean * `.planning/workstreams//` instead of the project root. Omitted (the * default) preserves the prior `planningDir(cwd)` resolution exactly, * including its `GSD_WORKSTREAM` env fallback. + * + * #3184: `extractCurrentMilestone`'s CRITICAL blast radius (200+ affected + * symbols, 20 direct callers) means its signature and return type do not + * change. This is the real owner; `extractCurrentMilestone` becomes a + * one-line wrapper returning `.value` so every existing caller is untouched. */ -function extractCurrentMilestone(content: string, cwd?: string, ws?: string | null): string { - if (!cwd) return stripShippedMilestones(content); +function extractCurrentMilestoneScoped(content: string, cwd?: string, ws?: string | null): { value: string; scope: Scope } { + if (!cwd) { + // Row 1: a deliberate unscoped read (no cwd supplied) is a real answer — + // the caller asked for no scoping, so whole-document is COMPLETE. + return { value: stripShippedMilestones(content), scope: SCOPE.COMPLETE }; + } let version: string | null = null; try { @@ -129,18 +329,33 @@ function extractCurrentMilestone(content: string, cwd?: string, ws?: string | nu } } - if (!version) return stripShippedMilestones(content); + const versionResolved = version !== null; + // #3184: routed through the shared owner (was an inline copy — see the + // twin copy in `getMilestonePhaseFilter`, the intra-owner-file duplicate + // review caught since the drift guard exempts this file by construction). + const versionedMilestonesPresent = hasVersionedMilestones(content); - const escapedVersion = escapeRegex(version); - const sectionPattern = new RegExp( - `(^#{1,3}\\s+(?!Phase\\s+\\S).*${escapedVersion}\\b[^\\n]*)`, - 'gmi' - ); + if (!version) { + const value = stripShippedMilestones(content); + return { + value, + scope: classifyMilestoneWindow({ + readable: true, + versionResolved, + hasVersionedMilestones: versionedMilestonesPresent, + headingFound: false, + windowHasPhaseEntries: hasPhaseEntries(value), + documentHasPhaseEntries: hasPhaseEntries(value), + }), + }; + } + + const documentHasPhaseEntries = hasPhaseEntries(stripShippedMilestones(content)); const summaryPattern = new RegExp( - `]*>([^<]*${escapedVersion}[^<]*)<\\/summary>`, + `]*>([^<]*${escapeRegex(version)}[^<]*)<\\/summary>`, 'i' ); - const headingMatches = [...content.matchAll(sectionPattern)]; + const headingMatches = locateMilestoneHeadings(content, version); if (headingMatches.length === 0) { const summaryMatch = content.match(summaryPattern); @@ -161,39 +376,46 @@ function extractCurrentMilestone(content: string, cwd?: string, ws?: string | nu // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). .replace(/^#{2,4}\s*Phase\s+[\w][\w.-]*(?:\s*\([^)\n]{0,200}\))?\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim, '') .replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, ''); - return preamble + content.slice(detailsOpenIdx, detailsEnd); + const value = preamble + content.slice(detailsOpenIdx, detailsEnd); + return { + value, + scope: classifyMilestoneWindow({ + readable: true, + versionResolved, + hasVersionedMilestones: versionedMilestonesPresent, + headingFound: true, + windowHasPhaseEntries: hasPhaseEntries(value), + documentHasPhaseEntries, + }), + }; } } - return stripShippedMilestones(content); + const value = stripShippedMilestones(content); + return { + value, + scope: classifyMilestoneWindow({ + readable: true, + versionResolved, + hasVersionedMilestones: versionedMilestonesPresent, + headingFound: false, + windowHasPhaseEntries: hasPhaseEntries(value), + documentHasPhaseEntries, + }), + }; } const allMatches = headingMatches; const isClosed = isClosedMilestoneHeading; const firstMatch = allMatches[0]; - const selected = allMatches.find((m) => !isClosed(m[1])) || firstMatch; + // #3184: selection collapses to the sole owner; `allMatches` is still needed + // below (offsets, detailsMatch search), so only the selection itself routes + // through `selectMilestoneHeading` rather than the whole block. + const selected = selectMilestoneHeading(content, version)!; const sectionStart = selected.index; - const computeSectionEnd = (headingText: string, headingStart: number): number => { - const level = (headingText.match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; - const afterHeading = headingStart + headingText.length; - // Use tokenizeHeadings (fence-aware, offsets into original content) to find - // the next stop boundary without re-implementing fence detection. T4 seam migration. - const headings = tokenizeHeadings(content); - for (const h of headings) { - if (h.offset <= headingStart) continue; - if (h.offset < afterHeading) continue; - if (h.level > level) continue; - // Mirrors old stopPattern: level-bounded, not a Phase heading, milestone marker - if (/^Phase\s+\S/i.test(h.text)) continue; - if (!/v\d+\.\d+|✅|📋|🚧/i.test(h.text)) continue; - return h.offset; - } - return content.length; - }; - - const sectionEnd = computeSectionEnd(selected[0], sectionStart); + const sectionEnd = computeMilestoneSectionEnd(content, selected[0], sectionStart); const anyMilestonePattern = /^#{1,3}\s+(?!Phase\s+\S)(?:.*v\d+\.\d+|✅|📋|🚧)/im; const firstMilestoneMatch = content.match(anyMilestonePattern); @@ -231,7 +453,7 @@ function extractCurrentMilestone(content: string, cwd?: string, ws?: string | nu const detailsStart = detailsMatch.index ?? 0; detailsSection = content.slice( detailsStart, - computeSectionEnd(detailsMatch[0], detailsStart), + computeMilestoneSectionEnd(content, detailsMatch[0], detailsStart), ); } @@ -250,9 +472,31 @@ function extractCurrentMilestone(content: string, cwd?: string, ws?: string | nu .replace(currentSectionHasPhaseDetails ? /^#{2,4}\s*Phase\s+[\w][\w.-]*(?:\s*\([^)\n]{0,200}\))?\s*:[^\n]*(?:\n(?!#{1,6}\s)[^\n]*)*\n?/gim : /$/, '') .replace(/^#{1,4}\s*Phase Details\b[^\n]*\n?/gim, ''); - return detailsSection + const value = detailsSection ? preamble + currentSection + '\n' + detailsSection : preamble + currentSection; + + return { + value, + scope: classifyMilestoneWindow({ + readable: true, + versionResolved, + hasVersionedMilestones: versionedMilestonesPresent, + headingFound: true, + windowHasPhaseEntries: hasPhaseEntries(value), + documentHasPhaseEntries, + }), + }; +} + +/** + * #3184: thin wrapper preserving `extractCurrentMilestone`'s exact signature + * and return type — CRITICAL blast radius (20 direct callers), so the type + * stays `string`. `extractCurrentMilestoneScoped` is the real owner; callers + * that need to branch on scope opt in to it directly. + */ +function extractCurrentMilestone(content: string, cwd?: string, ws?: string | null): string { + return extractCurrentMilestoneScoped(content, cwd, ws).value; } /** @@ -603,6 +847,13 @@ type MilestonePhaseFilter = ((dirName: string) => boolean) & { * the current milestone's. */ versionSectionFound: boolean; + /** + * #3184 (ADR-3180 Decision 2): the same window-classification carried by + * `extractCurrentMilestoneScoped`. The filter's FUNCTION behavior is + * UNCHANGED by this field — pass-all still passes all; a destructive + * consumer (`cmdMilestoneComplete`) reads `scope` to refuse instead. + */ + scope: Scope; }; /** @@ -629,13 +880,23 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p let missingExplicitVersion = false; let versionScoped = false; let versionSectionFound = false; + let scope: Scope = SCOPE.UNREADABLE; try { const roadmapPath = path.join(planningDir(cwd, ws), 'ROADMAP.md'); const roadmapContent = platformReadSync(roadmapPath); if (roadmapContent === null) throw new Error('missing'); - let roadmap = extractCurrentMilestone(roadmapContent, cwd, ws); + const scopedResult = extractCurrentMilestoneScoped(roadmapContent, cwd, ws); + let roadmap = scopedResult.value; + // Default: the filter's window IS extractCurrentMilestoneScoped's own + // window (reused verbatim, not re-derived — ADR-3180 Decision 4c). + // Overwritten below when `versionOverride` scopes to a DIFFERENT window. + scope = scopedResult.scope; - const hasVersionedMilestonesGlobal = /^#{1,3}\s+.*v\d+\.\d+/mi.test(roadmapContent); + // #3184: routed through the shared owner (was an inline copy — see the + // twin copy in `extractCurrentMilestoneScoped`, the intra-owner-file + // duplicate review caught since the drift guard exempts this file by + // construction). + const hasVersionedMilestonesGlobal = hasVersionedMilestones(roadmapContent); const hasPhaseHeadings = /#{2,4}\s*(?:\[[^\]]{1,200}\]\s*)?Phase\s+[\w]/i.test(roadmapContent); if (!hasVersionedMilestonesGlobal && hasPhaseHeadings && phaseIdConvention === 'milestone-prefixed') { console.warn( @@ -646,51 +907,48 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p } if (versionOverride) { - const escapedVersion = escapeRegex(versionOverride); - const sectionPattern = new RegExp(`(^#{1,3}\\s+(?!Phase\\s+\\S).*${escapedVersion}[^\\n]*)`, 'mi'); - let sectionMatch = roadmapContent.match(sectionPattern); + // #3184: route the whole "locate headings -> pick the active one -> + // section-end" composition through the single owner (sliceMilestoneWindow) + // instead of assembling it here. This branch used to be a bare `.match()` + // — first hit, no closed-heading skip, no version-token boundary — and a + // review pass caught it independently re-composing the SAME primitives + // `cmdMilestoneComplete`'s guard composed, disagreeing on closed-heading + // skipping. Now both sites call one function. Boundary-matched + // (`(?![\w.-])`) and closed-heading-skipping is a declared Tier-2 change + // affecting every caller that passes `versionOverride`: `roadmap.analyze` + // / `milestone complete` (this module, `cmdMilestoneComplete` in + // milestone.cts), `inspectWorkstream` (workstream-inventory.cts:518, + // via `currentVersion`), and `buildStateFrontmatter` (state.cts:1700, + // via `storedMilestone`). + const sliced = sliceMilestoneWindow(roadmapContent, versionOverride); - if (!sectionMatch) { - const summaryPat = new RegExp(`]*>[^<]*${escapedVersion}[^<]*<\\/summary>`, 'i'); - const summaryHit = roadmapContent.match(summaryPat); - if (summaryHit) { - const beforeSummary = roadmapContent.slice(0, summaryHit.index); - const detailsIdx = beforeSummary.lastIndexOf(']*>[^<]*${escapedVersion}[^<]*<\\/summary>`, 'i').test(roadmapContent); - if (hasVersionedMilestones && !versionInSummary) { + if (hasVersionedMilestonesGlobal && !versionInSummary) { roadmap = ''; missingExplicitVersion = true; } - } else { - versionScoped = true; - versionSectionFound = true; - const sectionStart = sectionMatch.index!; - const headingLevel = (sectionMatch[1].match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; - const afterHeading = sectionStart + sectionMatch[0].length; - // Use tokenizeHeadings (fence-aware, offsets into original content) to find - // the next milestone-boundary heading. T4 seam migration. - const allHeadings = tokenizeHeadings(roadmapContent); - let sectionEnd = roadmapContent.length; - for (const h of allHeadings) { - if (h.offset < afterHeading) continue; - if (h.level > headingLevel) continue; - if (/^Phase\s+\S/i.test(h.text)) continue; - if (!/v\d+\.\d+|✅|📋|🚧/i.test(h.text)) continue; - sectionEnd = h.offset; - break; - } - - const currentSection = roadmapContent.slice(sectionStart, sectionEnd); - roadmap = currentSection; + // else: version appears only inside a ``, or there are no + // versioned milestones anywhere — `roadmap` keeps + // extractCurrentMilestoneScoped's own (STATE-scoped) result, matching + // the pre-existing summary-block / free-form fallback shape. } + + scope = classifyMilestoneWindow({ + readable: true, + versionResolved: true, + hasVersionedMilestones: hasVersionedMilestonesGlobal, + headingFound: sliced !== null, + windowHasPhaseEntries: hasPhaseEntries(roadmap), + documentHasPhaseEntries, + }); } // Use tokenizeHeadings (fence-aware) instead of stripFencedLines + regex. @@ -706,10 +964,15 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p // #2199: also count bullet/checkbox phase entries (`- [ ] **Phase N — name**`) // so a bullet-house-style ROADMAP populates the milestone phase set instead of // collapsing to a zero-count pass-all filter. + // #3184 review finding: this scan must be fence-aware like `hasPhaseEntries` + // above — otherwise a fenced markdown EXAMPLE of the bullet syntax inflates + // milestonePhaseNums / phaseCount. Strip fences through the canonical seam + // first. { let bm: RegExpExecArray | null; const scanner = new RegExp(BULLET_PHASE_LINE_PATTERN.source, 'gim'); - while ((bm = scanner.exec(roadmap)) !== null) { + const roadmapUnfenced = stripFencedCode(roadmap).text; + while ((bm = scanner.exec(roadmapUnfenced)) !== null) { if (!/^999\b/.test(bm[1])) milestonePhaseNums.add(bm[1]); } } @@ -719,7 +982,9 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p * any failure milestonePhaseNums stays empty, which below already * degrades to the same pass-all filter this function returns when a * ROADMAP genuinely has zero recognizable phase headings — a safe, - * non-corrupting (over-inclusive, never under-inclusive) degrade. */ + * non-corrupting (over-inclusive, never under-inclusive) degrade. + * #3184: `scope` was set to SCOPE.UNREADABLE before the try (row 2) and + * is left as-is here — the read/parse fault IS the unreadable case. */ } if (milestonePhaseNums.size === 0) { @@ -731,6 +996,10 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p // `versionScoped` is reset here — this is the only surviving evidence that // the current milestone exists in the ROADMAP and simply has no phases yet. passAll.versionSectionFound = versionSectionFound; + // #3184: the filter's FUNCTION behavior is unchanged — pass-all still + // passes all. `scope` is the decidable signal a destructive consumer + // reads to refuse instead (ADR-3180 Decision 3's two-tier policy). + passAll.scope = scope; return passAll; } @@ -776,6 +1045,7 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p (isDirInMilestone as MilestonePhaseFilter).missingExplicitVersion = missingExplicitVersion; (isDirInMilestone as MilestonePhaseFilter).versionScoped = versionScoped; (isDirInMilestone as MilestonePhaseFilter).versionSectionFound = versionSectionFound; + (isDirInMilestone as MilestonePhaseFilter).scope = scope; return isDirInMilestone as MilestonePhaseFilter; } @@ -785,12 +1055,12 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p * writer) so they cannot touch a backticked prose literal, a Backlog entry, or a * same-numbered phase in a shipped milestone. * - * Mirrors the region selection in `extractCurrentMilestone` (version detection → - * active heading → next milestone boundary → optional Phase Details section). - * Returns null when there is no versioned active milestone; callers then fall - * back to whole-content mutation (the prior behaviour). - * - * NOTE: keep the region logic here in sync with extractCurrentMilestone. + * Mirrors the region selection in `extractCurrentMilestoneScoped` (version + * detection → active heading → next milestone boundary → optional Phase + * Details section) — both consume the same `locateMilestoneHeadings` / + * `computeMilestoneSectionEnd` owner (#3184), so there is no separate copy to + * keep in sync. Returns null when there is no versioned active milestone; + * callers then fall back to whole-content mutation (the prior behaviour). */ function currentMilestoneRawRanges( content: string, @@ -813,33 +1083,16 @@ function currentMilestoneRawRanges( } if (!version) return null; - const escapedVersion = escapeRegex(version); - const sectionPattern = new RegExp( - `(^#{1,3}\\s+(?!Phase\\s+\\S).*${escapedVersion}\\b[^\\n]*)`, - 'gmi', - ); - const headingMatches = [...content.matchAll(sectionPattern)]; + const headingMatches = locateMilestoneHeadings(content, version); if (headingMatches.length === 0) return null; const isClosed = isClosedMilestoneHeading; - const firstMatch = headingMatches[0]; - const selected = headingMatches.find((m) => !isClosed(m[1])) || firstMatch; + // #3184: selection collapses to the sole owner; `headingMatches` is still + // needed below for the detailsMatch search over all headings. + const selected = selectMilestoneHeading(content, version)!; const sectionStart = selected.index ?? 0; - const computeSectionEnd = (headingText: string, headingStart: number): number => { - const level = (headingText.match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; - const afterHeading = headingStart + headingText.length; - for (const h of tokenizeHeadings(content)) { - if (h.offset <= headingStart) continue; - if (h.offset < afterHeading) continue; - if (h.level > level) continue; - if (/^Phase\s+\S/i.test(h.text)) continue; - if (!/v\d+\.\d+|✅|📋|🚧/i.test(h.text)) continue; - return h.offset; - } - return content.length; - }; - const sectionEnd = computeSectionEnd(selected[0], sectionStart); + const sectionEnd = computeMilestoneSectionEnd(content, selected[0], sectionStart); const selectedVersionToken = selected[1].match( /v\d+(?:\.\d+)+(?:[-.][A-Za-z0-9]+)*/i, @@ -857,7 +1110,7 @@ function currentMilestoneRawRanges( let details: { start: number; end: number } | null = null; if (detailsMatch) { const detailsStart = detailsMatch.index ?? 0; - details = { start: detailsStart, end: computeSectionEnd(detailsMatch[0], detailsStart) }; + details = { start: detailsStart, end: computeMilestoneSectionEnd(content, detailsMatch[0], detailsStart) }; } return { primary: { start: sectionStart, end: sectionEnd }, details }; @@ -866,11 +1119,23 @@ function currentMilestoneRawRanges( export = { stripShippedMilestones, extractCurrentMilestone, + extractCurrentMilestoneScoped, isMilestoneShippedInRoadmap, + isMilestoneBoundedInRoadmap, replaceInCurrentMilestone, getRoadmapPhaseInternal, getMilestoneInfo, getMilestonePhaseFilter, currentMilestoneRawRanges, withPhaseSection, + computeMilestoneSectionEnd, + locateMilestoneHeadings, + selectMilestoneHeading, + classifyMilestoneWindow, + // #3184: the sole "give me this version's window" composition — see its + // own doc comment. milestone.cts's destructive-consumer guard consumes + // this instead of composing locate+select+section-end itself. + sliceMilestoneWindow, + hasVersionedMilestones, + hasMilestoneSectioning, }; diff --git a/src/roadmap.cts b/src/roadmap.cts index bcbd1c5e0..263781928 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, replaceInCurrentMilestone } = roadmapParserModule; +const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone } = roadmapParserModule; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import { updateTableCell } from './markdown-table.cjs'; import { platformWriteSync } from './shell-command-projection.cjs'; @@ -309,7 +309,10 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { } const rawContent = fs.readFileSync(roadmapPath, 'utf-8'); - const content = extractCurrentMilestone(rawContent, cwd); + // #3184/#3165: use the scoped variant so a truncated window is a + // distinguishable signal in the output instead of a silent `phase_count: 0` + // indistinguishable from a genuinely empty milestone. + const { value: content, scope } = extractCurrentMilestoneScoped(rawContent, cwd); const phasesDir = planningPaths(cwd).phases; // Extract all phase headings: ## Phase N: Name or ### Phase N: Name @@ -486,6 +489,11 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { current_phase: currentPhase ? currentPhase.number : null, next_phase: nextPhase ? nextPhase.number : null, missing_phase_details: missingDetails.length > 0 ? missingDetails : null, + // #3184/#3165: distinguishes a genuinely empty milestone (`scope: + // "complete"`, `phase_count: 0`) from a window that could not be fully + // resolved (`"truncated"` / `"unscoped"` / `"unreadable"`) — those cases + // were previously output-identical. + scope, }; output(result, raw, undefined); diff --git a/src/state.cts b/src/state.cts index dbc24e6da..634c443b5 100644 --- a/src/state.cts +++ b/src/state.cts @@ -19,7 +19,7 @@ import phaseIdMod = require('./phase-id.cjs'); const { escapeRegex, parsePhaseFromProse, PHASE_NUMBER_TOKEN_SOURCE, phaseKeyFromToken, phaseKeyFromDir } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); -const { getMilestoneInfo, getMilestonePhaseFilter, extractCurrentMilestone } = roadmapParserMod; +const { getMilestoneInfo, getMilestonePhaseFilter, extractCurrentMilestone, isMilestoneBoundedInRoadmap, hasMilestoneSectioning } = roadmapParserMod; import { platformWriteSync, platformReadSync, platformEnsureDir, retryRenameSync, toPosixPath } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); @@ -1775,11 +1775,12 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // downstream (mirrors the sync write-path guard). let milestoneBounded = true; if (milestone && roadmapRaw !== null) { - const versionedHeading = new RegExp( - `^#{1,3}\\s+(?!Phase\\s+\\S).*${escapeRegex(String(milestone).trim())}`, - 'mi', - ); - milestoneBounded = versionedHeading.test(roadmapRaw); + // #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()); } // #2828: distinguish a FLAT unmilestoned roadmap (no milestone sectioning // at all — only Phase headings) from a MILESTONED-but-unbounded one @@ -1787,10 +1788,14 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // On a flat roadmap the whole-doc count is correct (no sibling milestones to // conflate); on a sectioned-but-unbounded one it conflates siblings (#1761), // so fall back to phaseDirs.length. - const hasMilestoneSectioning = roadmapRaw !== null - && /^#{2,3}\s+(?!Phase\s+\S)/mi.test(roadmapRaw); + // #3184: routed through the single owner (roadmap-parser.cjs) — + // deliberately weaker than isMilestoneBoundedInRoadmap above (no + // version-token requirement); see hasMilestoneSectioning's own + // doc comment for why that distinction is load-bearing. + const roadmapHasMilestoneSectioning = roadmapRaw !== null + && hasMilestoneSectioning(roadmapRaw); const safeToUseRoadmapCount = milestoneBounded - || (roadmapPhaseCount > 0 && !hasMilestoneSectioning); + || (roadmapPhaseCount > 0 && !roadmapHasMilestoneSectioning); return { totalPhases: safeToUseRoadmapCount ? Math.max(phaseDirs.length, roadmapPhaseCount) @@ -2986,8 +2991,10 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b const versionStr = typeof fmVersion === 'string' && fmVersion.trim() ? fmVersion.trim() : null; let milestoneBounded = true; if (versionStr !== null && syncRoadmapRaw !== null) { - const versionedHeading = new RegExp(`^#{1,3}\\s+(?!Phase\\s+\\S).*${escapeRegex(versionStr)}`, 'mi'); - milestoneBounded = versionedHeading.test(syncRoadmapRaw); + // #3184: routed through the single owner (roadmap-parser.cjs) instead of + // a hand-rolled, unbounded-substring re-derivation — see the identical + // fix in buildStateFrontmatter above. + milestoneBounded = isMilestoneBoundedInRoadmap(syncRoadmapRaw, versionStr); } let percent: number | null = null; if (!milestoneBounded) { diff --git a/tests/emitted-drift-acks/3184-next-route0-window-scope.json b/tests/emitted-drift-acks/3184-next-route0-window-scope.json new file mode 100644 index 000000000..96b0fb7ed --- /dev/null +++ b/tests/emitted-drift-acks/3184-next-route0-window-scope.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "next.md": "#3184 (epic #3180 Phase 2): Route 0 (resume_incomplete_phase) grows by one branch that reads the new `scope` field `roadmap analyze` emits. Before this, Route 0 treated a well-formed `{phases: []}` as a clean scan and fell through to routing as though the incomplete-phase invariant (#160) had been checked and passed — the silent disarm #3165 reports, where a milestone window truncated by a closed-milestone heading yields an empty phase list with exit 0 and no error. The added `elif [ \"$ROADMAP_SCOPE\" != \"complete\" ]` arm routes that case into the SAME warn-and-fall-through branch the existing `roadmap.analyze failed` arm already uses, so a non-answer is no longer indistinguishable from a genuinely empty scan. The growth is the new arm's three warning lines, its explanatory comment, the `ROADMAP_SCOPE` assignment, and one success-criteria bullet. Adding this to the workflow rather than only to the CLI is deliberate: a spec review of this phase found that emitting `scope` without a consumer left #3165's actual symptom reproducing, so the field would have been a diagnostic nobody reads." + } +} diff --git a/tests/fix-2658-trae-runtime-detection-and-instruction-path.test.cjs b/tests/fix-2658-trae-runtime-detection-and-instruction-path.test.cjs index 0da6ae3c3..ba2692490 100644 --- a/tests/fix-2658-trae-runtime-detection-and-instruction-path.test.cjs +++ b/tests/fix-2658-trae-runtime-detection-and-instruction-path.test.cjs @@ -210,7 +210,17 @@ describe('#2658: end-to-end --trae install never emits the malformed path (accep test('local install: no emitted .md/.js/.cjs file contains the malformed strings; the rules file is concrete', () => { const { configDir, root } = runMinimalInstall({ runtime: 'trae', scope: 'local' }); try { - const files = walk(configDir).filter((f) => /\.(md|js|cjs)$/.test(f)); + const files = walk(configDir) + .filter((f) => /\.(md|js|cjs)$/.test(f)) + // gsd-core/CHANGELOG.md is excluded by exact relative path (not a blanket + // .md skip — the emitted agent/command/workflow markdown this gate exists + // to guard stays fully scanned). CHANGELOG.md legitimately QUOTES the + // malformed `.claude/.trae/rules` / `.trae/.trae/rules` strings while + // documenting the #2658 fix itself (#3006) — that historical-value + // citation is not a regression of the installer's actual output. Verified + // empirically: excluding only this one file drops the hit count to zero + // across all 620 other emitted files. + .filter((f) => f.split(path.sep).join('/').indexOf('gsd-core/CHANGELOG.md') === -1); assert.ok(files.length > 0, 'expected at least one emitted .md/.js/.cjs file'); for (const file of files) { const content = fs.readFileSync(file, 'utf8'); diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index 6296ba546..eec9ca4f0 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -415,6 +415,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index 31052afa9..c47c4b525 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -485,6 +485,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", "skills/gsd-ns-context/skills/extract-learnings/SKILL.md", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index 6c81d7e09..769141d23 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -483,5 +483,6 @@ "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", - "scripts/lib/cli-exit.cjs" + "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs" ] diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index 96a903162..aeec5503b 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -413,6 +413,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/cline.json b/tests/fixtures/install-tree/cline.json index e9b94fd3c..305ac0683 100644 --- a/tests/fixtures/install-tree/cline.json +++ b/tests/fixtures/install-tree/cline.json @@ -385,6 +385,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", "skills/gsd-ns-context/skills/extract-learnings/SKILL.md", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 42c867345..8be621622 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -485,6 +485,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 3ed7a3eee..325bfb8d5 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -492,5 +492,6 @@ "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", - "scripts/lib/cli-exit.cjs" + "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs" ] diff --git a/tests/fixtures/install-tree/copilot.json b/tests/fixtures/install-tree/copilot.json index 768e456be..66c8011ab 100644 --- a/tests/fixtures/install-tree/copilot.json +++ b/tests/fixtures/install-tree/copilot.json @@ -384,6 +384,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index 1766c9f48..ae6ed8c60 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -391,6 +391,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index 238db4d2c..aa429b175 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -414,6 +414,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd/DESCRIPTION.md", "skills/gsd/gsd-ns-context/SKILL.md", "skills/gsd/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 4670f4417..6921ef1ab 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -488,6 +488,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 00ccaa849..8002fc43a 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -414,6 +414,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/kimi.json b/tests/fixtures/install-tree/kimi.json index 84b90b55c..e2bfcb266 100644 --- a/tests/fixtures/install-tree/kimi.json +++ b/tests/fixtures/install-tree/kimi.json @@ -450,6 +450,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 2f7710433..7b183d482 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -488,6 +488,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", "skills/gsd-audit-fix/SKILL.md", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index d5b4c00d1..5e7e17721 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -381,5 +381,6 @@ "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", - "scripts/lib/cli-exit.cjs" + "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs" ] diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 3d97b6418..2caa99098 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -414,6 +414,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", "skills/gsd-ns-context/skills/extract-learnings/SKILL.md", diff --git a/tests/fixtures/install-tree/trae.json b/tests/fixtures/install-tree/trae.json index 995f0ac5c..107b9cdce 100644 --- a/tests/fixtures/install-tree/trae.json +++ b/tests/fixtures/install-tree/trae.json @@ -382,6 +382,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", "skills/gsd-ns-context/skills/extract-learnings/SKILL.md", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index 5093184ec..b6997cc77 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -384,5 +384,6 @@ "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", - "scripts/lib/cli-exit.cjs" + "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs" ] diff --git a/tests/fixtures/install-tree/zcode.json b/tests/fixtures/install-tree/zcode.json index 5dda4b917..df2cefaae 100644 --- a/tests/fixtures/install-tree/zcode.json +++ b/tests/fixtures/install-tree/zcode.json @@ -453,6 +453,7 @@ "scripts/gen-loop-host-contract.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", + "scripts/lib/drift-scan.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", "skills/gsd-ns-context/skills/extract-learnings/SKILL.md", diff --git a/tests/install.test.cjs b/tests/install.test.cjs index df4721bf3..f4da7e67b 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -49,6 +49,8 @@ const { configureKiloPermissions, selectRuntimesFromArgs, normalizeNodePath, + GSD_CHANGESET_FILES, + GSD_SCRIPTS_LIB_FILES, } = require('../bin/install.js'); const { getGlobalConfigDir } = require('../gsd-core/bin/lib/runtime-homes.cjs'); @@ -11107,3 +11109,48 @@ describe('#3026: installer --help documents every accepted runtime flag', () => `--help must document every accepted runtime flag; missing: ${missing.join(', ')}`); }); }); + +// ─── #3184: scripts/lib/ and scripts/changeset/ install/uninstall parity ──── +// +// install() copies both source directories WHOLESALE (every file present — +// see bin/install.js's "and any future lib helpers" comment), but uninstall() +// removes files via an EXPLICIT hardcoded enumeration (GSD_SCRIPTS_LIB_FILES / +// GSD_CHANGESET_FILES). Nothing keeps the two in sync: a file added to either +// source directory ships to every install and then orphans on uninstall (it +// survives removal, keeps the target dir non-empty, and blocks its rmdir). +// This guard fails the moment the real directory outgrows the enumeration. + +/** + * Pure comparison: every file actually present in a source dir must be named + * in the corresponding uninstall enumeration. Decoupled from fs so it can be + * exercised directly with doctored input (see the probe in the PR report). + */ +function findUnenumeratedFiles(actualFiles, enumeratedFiles) { + const enumerated = new Set(enumeratedFiles); + return actualFiles.filter(f => !enumerated.has(f)); +} + +describe('#3184: scripts/lib/ and scripts/changeset/ install/uninstall parity', () => { + const REPO_ROOT = path.join(__dirname, '..'); + + function realFilesIn(relativeDir) { + const dir = path.join(REPO_ROOT, relativeDir); + return fs.readdirSync(dir).filter(entry => fs.statSync(path.join(dir, entry)).isFile()); + } + + test('every file in scripts/lib/ is enumerated in GSD_SCRIPTS_LIB_FILES (bin/install.js)', () => { + const unenumerated = findUnenumeratedFiles(realFilesIn(path.join('scripts', 'lib')), GSD_SCRIPTS_LIB_FILES); + assert.deepEqual(unenumerated, [], + `scripts/lib/${unenumerated.join(', scripts/lib/')} exist on disk but are missing from ` + + `GSD_SCRIPTS_LIB_FILES in bin/install.js — add ${unenumerated.length === 1 ? 'it' : 'them'} to that ` + + 'array so uninstall() removes it (otherwise it ships to every install and orphans on uninstall).'); + }); + + test('every file in scripts/changeset/ is enumerated in GSD_CHANGESET_FILES (bin/install.js)', () => { + const unenumerated = findUnenumeratedFiles(realFilesIn(path.join('scripts', 'changeset')), GSD_CHANGESET_FILES); + assert.deepEqual(unenumerated, [], + `scripts/changeset/${unenumerated.join(', scripts/changeset/')} exist on disk but are missing from ` + + `GSD_CHANGESET_FILES in bin/install.js — add ${unenumerated.length === 1 ? 'it' : 'them'} to that ` + + 'array so uninstall() removes it (otherwise it ships to every install and orphans on uninstall).'); + }); +}); diff --git a/tests/milestone-window-single-owner.test.cjs b/tests/milestone-window-single-owner.test.cjs new file mode 100644 index 000000000..b986cd64c --- /dev/null +++ b/tests/milestone-window-single-owner.test.cjs @@ -0,0 +1,1285 @@ +/** + * Tests for the milestone-window single-owner contract (#3184, epic #3180 + * Phase 2, ADR-3180). Matrix: .gsd/phase/refactor-3184-milestone-window-single-owner/50-test-matrix.md + * + * Covers: + * - src/roadmap-parser.cts `classifyMilestoneWindow` / `extractCurrentMilestoneScoped` + * — the SCOPE discriminator decision table (section A). + * - src/roadmap-parser.cts `computeMilestoneSectionEnd` — the sole section-end + * owner, replacing three former byte-identical copies (section B). + * - Consumer-output identity (ADR-3180 Decision 4c): `roadmap analyze`, + * `milestone complete --dry-run`, `getMilestonePhaseFilter`, `state sync`, + * and `phase complete`'s raw-range write scoping all asserted at the + * CONSUMER's own observable output, never the owner's return value alone + * (section C). + * - Destructive-consumer refusal: `milestone complete` refuses to archive a + * TRUNCATED window without `--force`, with the negative proof that + * nothing on disk moved (section D). + * - `state.cts`'s two `isMilestoneBoundedInRoadmap` call sites (section E). + * - `scripts/lint-milestone-window-drift.cjs` — the whole-repo drift guard + * (section F). + * - fast-check document-shaped property tests (section G, #2371 provenance). + * + * Uses helpers.cjs createTempDir/cleanup per CONTRIBUTING.md — never inline + * mkdtemp. IO failure injection uses mock.method(shellProj, 'platformReadSync', ...) + * restored via t.after(), never fs.chmodSync (root bypasses 000 in Docker/CI). + */ + +'use strict'; + +const { test, mock } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const fc = require('fast-check'); + +const roadmapParser = require('../gsd-core/bin/lib/roadmap-parser.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); +const shellProj = require('../gsd-core/bin/lib/shell-command-projection.cjs'); +const { createTempDir, cleanup, runGsdTools } = require('./helpers.cjs'); +const driftGuard = require('../scripts/lint-milestone-window-drift.cjs'); +const { sanitizeForReport } = require('../scripts/lib/drift-scan.cjs'); + +const { + classifyMilestoneWindow, + extractCurrentMilestoneScoped, + computeMilestoneSectionEnd, + locateMilestoneHeadings, + isMilestoneBoundedInRoadmap, + currentMilestoneRawRanges, + getMilestonePhaseFilter, + stripShippedMilestones, +} = roadmapParser; + +// ─── Fixture helpers ─────────────────────────────────────────────────────── + +function planningDirOf(cwd) { + return path.join(cwd, '.planning'); +} + +function writeRoadmap(cwd, content) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + fs.writeFileSync(path.join(planningDirOf(cwd), 'ROADMAP.md'), content); +} + +function writeState(cwd, fields) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + const lines = ['---']; + for (const [k, v] of Object.entries(fields)) lines.push(`${k}: ${v}`); + lines.push('---', ''); + fs.writeFileSync(path.join(planningDirOf(cwd), 'STATE.md'), lines.join('\n')); +} + +function writeFile(cwd, relPath, content) { + const full = path.join(cwd, relPath); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); +} + +// ═════════════════════════════════════════════════════════════════════════ +// Section A — Scope classification (classifyMilestoneWindow + extractCurrentMilestoneScoped) +// ═════════════════════════════════════════════════════════════════════════ + +test('unscoped read by design reports COMPLETE', () => { + const content = ['# Roadmap', '', '## v1.0 Old ✅ SHIPPED', '', '### Phase 1: Foo'].join('\n'); + const result = extractCurrentMilestoneScoped(content); + assert.strictEqual(result.scope, SCOPE.COMPLETE); + assert.strictEqual(result.value, stripShippedMilestones(content)); +}); + +test('scoped window with phases reports COMPLETE', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('genuinely empty milestone is COMPLETE not TRUNCATED', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = ['# Roadmap', '', '## v1.0 Current 🚧', '', 'Nothing planned yet.'].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('window closed before the phase region reports TRUNCATED', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v3.0' }); + const content = [ + '# Roadmap', + '', + '## v3.0 In Progress 🚧', + '', + 'Some preamble notes. No phase headings here.', + '', + '## v4.0 Next', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.TRUNCATED); +}); + +test('free-form roadmap is COMPLETE not UNSCOPED', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + // No STATE.md milestone field at all -- no version resolvable, and the + // roadmap carries no versioned milestone headings anywhere. + const content = ['# Roadmap', '', '## Overview', '', '### Phase 1: Foo'].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('versioned roadmap with no resolvable milestone is UNSCOPED', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + // No STATE.md milestone field -- no version resolvable -- but the roadmap + // DOES carry versioned milestone headings elsewhere. + const content = ['# Roadmap', '', '## v1.0 Old ✅ SHIPPED', '', '### Phase 1: Foo'].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.UNSCOPED); +}); + +test('absent milestone section is UNSCOPED', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v9.9' }); + const content = ['# Roadmap', '', '## v1.0 Old ✅ SHIPPED', '', '### Phase 1: Foo'].join('\n'); + writeRoadmap(cwd, content); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.UNSCOPED); + + // missingExplicitVersion is a getMilestonePhaseFilter-only field (not on + // ScopedResult) -- confirm the SAME "absent section" disposition is + // preserved there too, for the explicit-version-override argument shape. + const filter = getMilestonePhaseFilter(cwd, 'v9.9'); + assert.strictEqual(filter.missingExplicitVersion, true); + assert.strictEqual(filter.scope, SCOPE.UNSCOPED); +}); + +test('unreadable roadmap reports UNREADABLE', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + writeRoadmap(cwd, ['# Roadmap', '', '## v1.0 Current 🚧', '', '### Phase 1: Foo'].join('\n')); + writeState(cwd, { milestone: 'v1.0' }); + + mock.method(shellProj, 'platformReadSync', () => { + throw new Error('EIO: simulated unreadable ROADMAP.md'); + }); + t.after(() => { + mock.restoreAll(); + cleanup(cwd); + }); + + const filter = getMilestonePhaseFilter(cwd); + assert.strictEqual(filter.scope, SCOPE.UNREADABLE); + // The filter still degrades pass-all -- it is not a destructive consumer. + assert.strictEqual(filter('anything-01'), true); + assert.strictEqual(filter.phaseCount, 0); +}); + +test('empty roadmap is COMPLETE', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + // No STATE.md -- no version resolvable on empty content either. + const result = extractCurrentMilestoneScoped('', cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('bullet-style phase entries count as phases', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + '- [ ] **Phase 1 — Foo**', + '- [ ] **Phase 2 — Bar**', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('bullet-only doc still detects truncation', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + 'No phases yet.', + '', + '## v2.0 Next', + '', + '- [ ] **Phase 1 — Foo**', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.TRUNCATED); +}); + +// #3184 review finding: `hasPhaseEntries`'s bullet fallback was not +// fence-aware (unlike its ATX-heading path, which uses `tokenizeHeadings`). +// A FENCED example of the bullet syntax -- e.g. documentation showing the +// convention inside a non-
-wrapped SHIPPED milestone section -- +// inflated `documentHasPhaseEntries` and misclassified a genuinely-empty +// active milestone TRUNCATED instead of COMPLETE, which then made +// `cmdMilestoneComplete` refuse a legitimate archive without --force. +test('fenced bullet-phase example is not a phase entry', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v0.9 Old ✅ SHIPPED', + '', + 'Example bullet-phase syntax for reference:', + '', + '```markdown', + '- [ ] **Phase 3 — Name**', + '```', + '', + '## v1.0 Current 🚧', + '', + 'Nothing planned yet.', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + // Genuinely empty active milestone: the only bullet-phase-shaped text + // anywhere in the document is fenced, so it must not count as a real + // phase entry on either side of the row-8 comparison -- COMPLETE, not + // TRUNCATED. + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +// Companion to the fenced case above: a real (unfenced) bullet phase entry +// outside the window must still classify TRUNCATED, proving the fence-aware +// fix strips fences rather than disabling bullet detection outright. This is +// the same fixture as 'bullet-only doc still detects truncation' above, +// asserted again here to pin both directions of the fix in one place. +test('unfenced bullet-phase entry outside the window still truncates', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + 'No phases yet.', + '', + '## v2.0 Next', + '', + '- [ ] **Phase 1 — Foo**', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.TRUNCATED); +}); + +test('shipped-details phases do not fake a truncation', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + const content = [ + '# Roadmap', + '', + '
', + '✅ v1.0 SHIPPED', + '', + '### Phase 1: Foo', + '', + '
', + '', + '## v2.0 Current 🚧', + '', + 'No phases yet.', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('sentinel-only window is COMPLETE', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = ['# Roadmap', '', '## v1.0 Current 🚧', '', '### Phase 999.1: Backlog item'].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('phase prose and horizontal rules are not phase entries', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + 'As discussed in Phase 3, we will revisit this.', + '', + '---', + '', + 'More notes.', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + // Neither the prose mention nor the `---` rule is a phase entry, so this + // reduces to the "genuinely empty milestone" shape -- COMPLETE, not TRUNCATED. + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('fenced milestone heading is not a boundary', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + '```markdown', + '## v9.9 milestone', + '```', + '', + '### Phase 1: Foo', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + // The fenced heading must not stop the window early -- Phase 1 stays + // inside it, so the window has phase entries and reads COMPLETE. + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +test('partial truncation is not detected (documented limit)', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '# Roadmap', + '', + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + '## v2.0 Next', + '', + '### Phase 2: Bar', + ].join('\n'); + + const result = extractCurrentMilestoneScoped(content, cwd); + // The window has SOME phases (Phase 1), so this reads COMPLETE even + // though the document has more (Phase 2) outside the window -- design's + // documented "partial truncation is invisible" limit. + assert.strictEqual(result.scope, SCOPE.COMPLETE); +}); + +// ─── A17: CRLF variants classify identically (table-driven) ─────────────── + +const CRLF_TABLE = [ + { + name: 'A2 scoped-window-with-phases', + version: 'v1.0', + lines: ['# Roadmap', '', '## v1.0 Current 🚧', '', '### Phase 1: Foo', '', '### Phase 2: Bar'], + }, + { + name: 'A3 genuinely-empty-milestone', + version: 'v1.0', + lines: ['# Roadmap', '', '## v1.0 Current 🚧', '', 'Nothing planned yet.'], + }, + { + name: 'A4 window-closed-before-phases', + version: 'v3.0', + lines: [ + '# Roadmap', '', '## v3.0 In Progress 🚧', '', 'Some preamble notes. No phase headings here.', + '', '## v4.0 Next', '', '### Phase 1: Foo', '', '### Phase 2: Bar', + ], + }, + { + name: 'A5 free-form-legacy', + version: null, + lines: ['# Roadmap', '', '## Overview', '', '### Phase 1: Foo'], + }, + { + name: 'A6 versioned-unscoped', + version: null, + lines: ['# Roadmap', '', '## v1.0 Old ✅ SHIPPED', '', '### Phase 1: Foo'], + }, + { + name: 'A10 bullet-in-window', + version: 'v1.0', + lines: ['# Roadmap', '', '## v1.0 Current 🚧', '', '- [ ] **Phase 1 — Foo**', '- [ ] **Phase 2 — Bar**'], + }, + { + name: 'A11 bullet-only-truncated', + version: 'v1.0', + lines: [ + '# Roadmap', '', '## v1.0 Current 🚧', '', 'No phases yet.', '', + '## v2.0 Next', '', '- [ ] **Phase 1 — Foo**', + ], + }, + { + name: 'A12 shipped-details-trap', + version: 'v2.0', + lines: [ + '# Roadmap', '', '
', '✅ v1.0 SHIPPED', '', + '### Phase 1: Foo', '', '
', '', '## v2.0 Current 🚧', '', 'No phases yet.', + ], + }, + { + name: 'A13 sentinel-only-window', + version: 'v1.0', + lines: ['# Roadmap', '', '## v1.0 Current 🚧', '', '### Phase 999.1: Backlog item'], + }, + { + name: 'A15 fenced-heading-not-boundary', + version: 'v1.0', + lines: [ + '# Roadmap', '', '## v1.0 Current 🚧', '', '```markdown', '## v9.9 milestone', '```', + '', '### Phase 1: Foo', + ], + }, +]; + +test('CRLF variants classify identically', (t) => { + const tmpDirs = []; + t.after(() => { + for (const dir of tmpDirs) cleanup(dir); + }); + + for (const row of CRLF_TABLE) { + const lfContent = row.lines.join('\n'); + const crlfContent = row.lines.join('\n').replace(/\n/g, '\r\n'); + + const lfCwd = createTempDir('gsd-milestone-window-crlf-lf-'); + const crlfCwd = createTempDir('gsd-milestone-window-crlf-crlf-'); + tmpDirs.push(lfCwd, crlfCwd); + + if (row.version) { + writeState(lfCwd, { milestone: row.version }); + writeState(crlfCwd, { milestone: row.version }); + } + const lfScope = extractCurrentMilestoneScoped(lfContent, lfCwd).scope; + const crlfScope = extractCurrentMilestoneScoped(crlfContent, crlfCwd).scope; + assert.strictEqual(crlfScope, lfScope, `CRLF mismatch for ${row.name}: LF=${lfScope} CRLF=${crlfScope}`); + } +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section B — Section-end owner (computeMilestoneSectionEnd) +// ═════════════════════════════════════════════════════════════════════════ + +test('stops at the next same-level milestone heading', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '## v2.0 Next 📋', 'more'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.indexOf('## v2.0 Next 📋')); +}); + +test('runs to end of document when no boundary follows', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body forever'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.length); +}); + +test('level-2 boundary stops a level-2 section', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '## v2.0 Next 📋'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.indexOf('## v2.0 Next 📋')); + assert.notStrictEqual(end, content.length); +}); + +test('level-3 boundary stops a level-3 section', () => { + const headingLine = '### v1.0 Current 🚧'; + const content = [headingLine, 'body', '### v2.0 Next 📋'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.indexOf('### v2.0 Next 📋')); + assert.notStrictEqual(end, content.length); +}); + +test('level-4 heading is not a milestone boundary', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '#### v2.0 Next 📋', 'tail marker here'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + // #{1,3} is the owner's level ceiling -- a level-4 heading is outside it + // and must never stop the window, regardless of the marker it carries. + assert.strictEqual(end, content.length); +}); + +test('deeper heading is not a boundary', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '### v2.0 Sub 📋', 'tail'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.length); +}); + +test('phase heading is never a boundary', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '## Phase 2: v2.0 Launch 📋', 'tail'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.length); +}); + +test('unmarked heading is not a boundary', () => { + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body', '## Notes', 'tail'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.length); +}); + +test('own heading is not its own boundary', () => { + // A single heading with no other content: the only tokenizeHeadings + // candidate is the heading itself (offset === headingStart), which the + // `h.offset <= headingStart` skip must exclude, forcing a fall-through to + // content.length rather than a zero-length section. + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, 'body'].join('\n'); + const headingStart = content.indexOf(headingLine); + const end = computeMilestoneSectionEnd(content, headingLine, headingStart); + assert.strictEqual(end, content.length); + assert.notStrictEqual(end, headingStart); +}); + +test('offset inside the heading line is not a boundary', () => { + // Defensive-seam test: production callers always pass a headingText whose + // length matches the real heading LINE, so afterHeading never legitimately + // overlaps a DIFFERENT heading's offset. This exercises the seam directly + // by passing an artificially long headingText that extends afterHeading + // past a second, real heading's own offset -- that second heading's + // candidacy must be skipped as "inside the heading span", not picked up + // as a boundary. + const headingLine = '## v1.0 Current 🚧'; + const content = [headingLine, '## v2.0 Next 📋', 'tail'].join('\n'); + const realStart = content.indexOf(headingLine); + const paddedHeadingText = headingLine + '\n## v2.0 Next 📋'; + const end = computeMilestoneSectionEnd(content, paddedHeadingText, realStart); + assert.strictEqual(end, content.length); +}); + +test('three former copies agree via one owner', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + // No preamble before the milestone heading and no "(Phase Details)" + // append, so extractCurrentMilestoneScoped's `.value` is EXACTLY + // content.slice(sectionStart, sectionEnd) -- letting all three former + // call sites be cross-checked against the SAME owner offset. + const content = ['## v1.0 Current 🚧', '', '### Phase 1: Foo', ''].join('\n'); + + const headingMatches = locateMilestoneHeadings(content, 'v1.0'); + assert.strictEqual(headingMatches.length, 1); + const selected = headingMatches[0]; + const ownerEnd = computeMilestoneSectionEnd(content, selected[0], selected.index); + + // Consumer 1: currentMilestoneRawRanges. + const ranges = currentMilestoneRawRanges(content, cwd); + assert.ok(ranges); + assert.strictEqual(ranges.primary.end, ownerEnd); + assert.strictEqual(ranges.primary.start, selected.index); + + // Consumer 2: extractCurrentMilestoneScoped -- with no preamble and no + // details append, `.value` equals the same [start,end) slice exactly. + const scoped = extractCurrentMilestoneScoped(content, cwd); + assert.strictEqual(scoped.value, content.slice(selected.index, ownerEnd)); + + // Consumer 3: getMilestonePhaseFilter's versionOverride branch -- same + // phase set as slicing [start,end) directly would produce. + const filter = getMilestonePhaseFilter(cwd, 'v1.0'); + assert.strictEqual(filter('01-foo'), true); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section C — Consumer-output identity (ADR-3180 Decision 4c) +// ═════════════════════════════════════════════════════════════════════════ + +test('roadmap.analyze phase set matches the owner window', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, [ + '
', + '✅ v1.0 SHIPPED', + '', + '### Phase 1: Foo', + '', + '
', + '', + '## v2.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n')); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '01-foo'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '02-bar'), { recursive: true }); + + const analyzeResult = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(analyzeResult.success, true, analyzeResult.error); + const analyzed = JSON.parse(analyzeResult.output); + const analyzedNumbers = analyzed.phases.map((p) => p.number).sort(); + + // Owner window: getMilestonePhaseFilter membership for the SAME cwd/version. + const filter = getMilestonePhaseFilter(cwd, 'v2.0'); + assert.strictEqual(filter('01-foo'), true); + assert.strictEqual(filter('02-bar'), true); + assert.deepStrictEqual(analyzedNumbers, ['1', '2']); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); +}); + +test('roadmap analyze reports scope truncated on a truncated window', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + // #3165 layout: an ACTIVE milestone heading for STATE.md's version, + // immediately followed by a CLOSED milestone heading at the SAME heading + // level before any `### Phase N:` section -- the phase sections live + // under the CLOSED heading, outside the ACTIVE window. + writeState(cwd, { milestone: 'v3.0' }); + writeRoadmap(cwd, [ + '# Roadmap', + '', + '## v3.0 Current 🚧', + '', + '## v2.0 Old ✅ SHIPPED', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n')); + + const result = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(analyzed.scope, SCOPE.TRUNCATED); + // Deliberately unchanged: the count stays 0 either way -- `scope` is what + // carries the truncation signal, not `phase_count`. + assert.strictEqual(analyzed.phase_count, 0); +}); + +test('roadmap analyze reports scope complete on a genuinely empty milestone', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + // Same shape as the truncated fixture above, but the document carries no + // phase entries anywhere -- the negative proof that the truncated + // assertion above is not just "any zero-phase roadmap reports truncated". + writeState(cwd, { milestone: 'v3.0' }); + writeRoadmap(cwd, [ + '# Roadmap', + '', + '## v3.0 Current 🚧', + '', + '## v2.0 Old ✅ SHIPPED', + '', + 'Nothing here either.', + ].join('\n')); + + const result = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(analyzed.scope, SCOPE.COMPLETE); + assert.strictEqual(analyzed.phase_count, 0); +}); + +test('roadmap analyze emits a scope field on every result', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, ['## v2.0 Current 🚧', '', '### Phase 1: Foo', '', '### Phase 2: Bar'].join('\n')); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '01-foo'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '02-bar'), { recursive: true }); + + const result = runGsdTools(['roadmap', 'analyze', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const analyzed = JSON.parse(result.output); + assert.strictEqual(Object.hasOwn(analyzed, 'scope'), true); + assert.strictEqual(Object.values(SCOPE).includes(analyzed.scope), true); +}); + +test('milestone.complete scoping matches the owner window', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + // #3184 review finding: the shipped v1.0 phase MUST use a phase NUMBER + // that does not also appear in v2.0's own window. getMilestonePhaseFilter + // scopes by matching a directory's NUMERIC phase-id prefix against the + // set of phase numbers found inside the target milestone's own sliced + // window -- it has no notion of "which milestone section a directory + // came from" beyond that number. The original fixture gave both the + // shipped v1.0 phase and the current v2.0 phase the SAME number ("1"), + // so both `01-old-shipped` and `01-foo` matched by numeric-prefix + // coincidence regardless of windowing -- that tested directory-naming + // overlap, not window scoping, and encoded a wrong expectation. + writeRoadmap(cwd, [ + '
', + '✅ v1.0 SHIPPED', + '', + '### Phase 5: Foo', + '', + '
', + '', + '## v2.0 Current 🚧', + '', + '### Phase 1: Foo', + ].join('\n')); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '05-old-shipped'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '01-foo'), { recursive: true }); + + const dryRun = runGsdTools(['milestone', 'complete', 'v2.0', '--dry-run', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(dryRun.success, true, dryRun.error); + const parsed = JSON.parse(dryRun.output); + + const filter = getMilestonePhaseFilter(cwd, 'v2.0'); + const expectedArchived = ['05-old-shipped', '01-foo'].filter((name) => filter(name)).sort(); + assert.deepStrictEqual([...parsed.would_archive.phases].sort(), expectedArchived); + assert.strictEqual(expectedArchived.includes('01-foo'), true); + assert.strictEqual(expectedArchived.includes('05-old-shipped'), false); +}); + +test('filter membership matches the owner window', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = ['## v1.0 Current 🚧', '', '### Phase 1: Foo', '', '### Phase 2: Bar'].join('\n'); + writeRoadmap(cwd, content); + + const filter = getMilestonePhaseFilter(cwd); + assert.strictEqual(filter.phaseCount, 2); + assert.strictEqual(filter('01-foo'), true); + assert.strictEqual(filter('02-bar'), true); + assert.strictEqual(filter('03-baz'), false); + assert.strictEqual(filter.scope, SCOPE.COMPLETE); +}); + +// #3184 review finding: `getMilestonePhaseFilter`'s #2199 bullet scan ran +// against un-stripped window content, so a fenced bullet-phase example +// inflated `milestonePhaseNums` / `phaseCount` the same way it inflated +// `hasPhaseEntries` above. +test('fenced bullet-phase example does not inflate phaseCount', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + const content = [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + 'Example bullet-phase syntax for reference:', + '', + '```markdown', + '- [ ] **Phase 3 — Name**', + '```', + ].join('\n'); + writeRoadmap(cwd, content); + + const filter = getMilestonePhaseFilter(cwd, 'v1.0'); + // Only the real heading (Phase 1) counts -- the fenced bullet example + // (Phase 3) must not. + assert.strictEqual(filter.phaseCount, 1); + assert.strictEqual(filter('01-foo'), true); + assert.strictEqual(filter('03-name'), false); +}); + +test('state bounding matches the owner predicate', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '01-foo'), { recursive: true }); + writeFile(cwd, '.planning/phases/01-foo/01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-SUMMARY.md', '# Summary\n'); + writeState(cwd, { milestone: 'v2.0.1' }); + + // Unbound case: asserted v2.0.1, heading only has v2.0 -- exercises the + // exact #2562-class boundary defect (row 17). + writeRoadmap(cwd, ['## v2.0 Launch', '', '### Phase 1: Foo'].join('\n')); + const roadmapUnbound = fs.readFileSync(path.join(cwd, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.strictEqual(isMilestoneBoundedInRoadmap(roadmapUnbound, 'v2.0.1'), false); + + const jsonUnbound = runGsdTools(['state', 'json', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(jsonUnbound.success, true, jsonUnbound.error); + const parsedUnbound = JSON.parse(jsonUnbound.output); + assert.strictEqual('percent' in (parsedUnbound.progress || {}), false); + + const syncUnbound = runGsdTools(['state', 'sync', '--verify', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(syncUnbound.success, true, syncUnbound.error); + const syncParsedUnbound = JSON.parse(syncUnbound.output); + assert.strictEqual(syncParsedUnbound.changes.length, 1); + + // Bound case: heading matches exactly. + writeRoadmap(cwd, ['## v2.0.1 Launch', '', '### Phase 1: Foo'].join('\n')); + const roadmapBound = fs.readFileSync(path.join(cwd, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.strictEqual(isMilestoneBoundedInRoadmap(roadmapBound, 'v2.0.1'), true); + + const jsonBound = runGsdTools(['state', 'json', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(jsonBound.success, true, jsonBound.error); + const parsedBound = JSON.parse(jsonBound.output); + assert.strictEqual('percent' in (parsedBound.progress || {}), true); + + const syncBound = runGsdTools(['state', 'sync', '--verify', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(syncBound.success, true, syncBound.error); + const syncParsedBound = JSON.parse(syncBound.output); + assert.strictEqual(syncParsedBound.changes.length, 0); +}); + +test('raw ranges match the owner window', (t) => { + // NOTE: 40-design.md's blast-radius table (line ~92) names cmdPhaseInsert + // as currentMilestoneRawRanges's sole dependent. The current source + // (src/phase.cts:2250) shows the actual call site is inside + // cmdPhaseComplete instead -- a real discrepancy between the design doc + // and the code, reported back per the dispatch brief rather than silently + // adjusted around. + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v2.0' }); + writeRoadmap(cwd, [ + '
', + '✅ v1.0 SHIPPED', + '', + '### Phase 1: Foo', + '', + '**Plans**: 0/1 plans complete', + '', + '
', + '', + '## v2.0 Current 🚧', + '', + '### Phase 1: Foo', + '', + '**Plans**: 0/1 plans complete', + ].join('\n')); + writeFile(cwd, '.planning/phases/01-foo/01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-SUMMARY.md', '---\none-liner: did the thing\n---\n# Summary\n'); + writeFile(cwd, '.planning/phases/01-foo/01-VERIFICATION.md', '---\nstatus: passed\n---\n# Verification\n'); + + const before = fs.readFileSync(path.join(cwd, '.planning', 'ROADMAP.md'), 'utf-8'); + const ranges = currentMilestoneRawRanges(before, cwd); + assert.ok(ranges, 'expected a resolvable milestone window'); + assert.ok(ranges.primary.start > 0, 'fixture must have a non-empty preamble to protect'); + + const result = runGsdTools(['phase', 'complete', '1', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + + const after = fs.readFileSync(path.join(cwd, '.planning', 'ROADMAP.md'), 'utf-8'); + // Negative proof: the SHIPPED v1.0 section -- which lies entirely BEFORE + // the owner's computed window start -- is byte-identical after the write. + // A consumer that re-derived its own (potentially wrong) window boundary + // instead of consuming currentMilestoneRawRanges could leak the mutation + // into this identically-shaped sibling "Phase 1" section; this is exactly + // the regression currentMilestoneRawRanges exists to prevent. + assert.strictEqual(after.slice(0, ranges.primary.start), before.slice(0, ranges.primary.start)); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section D — Destructive-consumer refusal (Tier-2) +// ═════════════════════════════════════════════════════════════════════════ + +function buildTruncatedFixture(cwd) { + writeState(cwd, { milestone: 'v3.0' }); + writeRoadmap(cwd, [ + '# Roadmap', + '', + '## v3.0 In Progress 🚧', + '', + 'Some preamble notes. No phase headings here.', + '', + '## v4.0 Next', + '', + '### Phase 1: Foo', + '', + '### Phase 2: Bar', + ].join('\n')); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '1-foo'), { recursive: true }); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '2-bar'), { recursive: true }); +} + +test('milestone complete refuses to archive a truncated window', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + + const result = runGsdTools(['milestone', 'complete', 'v3.0', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, false); + assert.notStrictEqual(result.exitCode, 0); +}); + +test('refusal leaves the phases directory untouched', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + + const before = fs.readdirSync(path.join(cwd, '.planning', 'phases')).sort(); + const result = runGsdTools(['milestone', 'complete', 'v3.0', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, false); + const after = fs.readdirSync(path.join(cwd, '.planning', 'phases')).sort(); + assert.deepStrictEqual(after, before); +}); + +test('--force overrides the truncation refusal', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + buildTruncatedFixture(cwd); + + const result = runGsdTools(['milestone', 'complete', 'v3.0', '--force', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.archived.phases, true); +}); + +test('genuinely empty milestone still completes', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + writeState(cwd, { milestone: 'v1.0' }); + writeRoadmap(cwd, ['# Roadmap', '', '## v1.0 Current 🚧', '', 'Nothing planned yet.'].join('\n')); + + const filter = getMilestonePhaseFilter(cwd, 'v1.0'); + assert.strictEqual(filter.scope, SCOPE.COMPLETE); + + const result = runGsdTools(['milestone', 'complete', 'v1.0', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section E — state.cts boundary defect (design rows 17) +// ═════════════════════════════════════════════════════════════════════════ + +test('version token is boundary-matched not substring-matched', () => { + const content = ['## v2.0 Launch', '', '### Phase 1: Foo'].join('\n'); + assert.strictEqual(isMilestoneBoundedInRoadmap(content, 'v2.0.1'), false); +}); + +test('exact version matches', () => { + const content = ['## v2.0.1 Launch', '', '### Phase 1: Foo'].join('\n'); + assert.strictEqual(isMilestoneBoundedInRoadmap(content, 'v2.0.1'), true); +}); + +test('both state bounding sites agree', (t) => { + const cwd = createTempDir('gsd-milestone-window-'); + t.after(() => cleanup(cwd)); + fs.mkdirSync(path.join(cwd, '.planning', 'phases', '01-foo'), { recursive: true }); + writeFile(cwd, '.planning/phases/01-foo/01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/01-foo/01-SUMMARY.md', '# Summary\n'); + + for (const [heading, expectBound] of [['## v2.0 Launch', false], ['## v2.0.1 Launch', true]]) { + writeState(cwd, { milestone: 'v2.0.1' }); + writeRoadmap(cwd, [heading, '', '### Phase 1: Foo'].join('\n')); + const roadmapRaw = fs.readFileSync(path.join(cwd, '.planning', 'ROADMAP.md'), 'utf-8'); + const ownerVerdict = isMilestoneBoundedInRoadmap(roadmapRaw, 'v2.0.1'); + assert.strictEqual(ownerVerdict, expectBound); + + // Site 1: buildStateFrontmatter, reached via `state json` -- typed via + // progress.percent presence/absence. + const jsonResult = runGsdTools(['state', 'json', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(jsonResult.success, true, jsonResult.error); + const parsedJson = JSON.parse(jsonResult.output); + assert.strictEqual('percent' in (parsedJson.progress || {}), expectBound, `state json site for ${heading}`); + + // Site 2: cmdStateSync's own direct isMilestoneBoundedInRoadmap call -- + // typed via the structural (numeric) changes[] length: an unbound + // milestone unconditionally pushes exactly one "Progress: skipped" + // change; a bound, already-in-sync fixture pushes none. + const syncResult = runGsdTools(['state', 'sync', '--verify', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(syncResult.success, true, syncResult.error); + const parsedSync = JSON.parse(syncResult.output); + assert.strictEqual(parsedSync.changes.length, expectBound ? 0 : 1, `state sync site for ${heading}`); + } +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section F — Drift guard (scripts/lint-milestone-window-drift.cjs) +// ═════════════════════════════════════════════════════════════════════════ + +const REPO_ROOT = path.join(__dirname, '..'); + +// A single line carrying BOTH the heading-quantifier token (a) and the +// phase-lookahead token (b1) -- the narrowest shape findMilestoneWindowDrift +// flags, independent of any version/marker pairing. +function violatingLine() { + return "const RE = /^#{1,3}\\s+(?!Phase\\s+\\S).*/;\n"; +} + +test('zero independent re-derivations remain', () => { + const violations = driftGuard.scanRepo(REPO_ROOT); + assert.deepStrictEqual(violations, []); +}); + +test('a new re-derivation is reported', (t) => { + const root = createTempDir('gsd-milestone-window-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync(path.join(root, 'src', 'fake.cts'), violatingLine()); + + const violations = driftGuard.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].file, path.join('src', 'fake.cts')); + assert.strictEqual(violations[0].line, 1); +}); + +test('function-scoped exemption suppresses only its own function', (t) => { + const root = createTempDir('gsd-milestone-window-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + const content = [ + 'function checkW021() {', + ` ${violatingLine().trim()}`, + '}', + '', + ].join('\n'); + fs.writeFileSync(path.join(root, 'src', 'roadmap-command-router.cts'), content); + + const violations = driftGuard.scanRepo(root); + assert.deepStrictEqual(violations, []); +}); + +test('exemption is function-scoped, not file-scoped', (t) => { + const root = createTempDir('gsd-milestone-window-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + const content = [ + 'function checkW021() {', + ` ${violatingLine().trim()}`, + '}', + '', + 'function someOtherFunction() {', + ` ${violatingLine().trim()}`, + '}', + '', + ].join('\n'); + fs.writeFileSync(path.join(root, 'src', 'roadmap-command-router.cts'), content); + + const violations = driftGuard.scanRepo(root); + // The exempted function's line is suppressed; the SAME shape inside a + // DIFFERENT function in the same exempted FILE is still reported. + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].line, 6); +}); + +test('symlinked source is not an evasion', { skip: process.platform === 'win32' ? 'symlink creation needs privilege on Windows' : false }, (t) => { + const root = createTempDir('gsd-milestone-window-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.mkdirSync(path.join(root, 'vendor'), { recursive: true }); + const realFile = path.join(root, 'vendor', 'real-target.cts'); + fs.writeFileSync(realFile, violatingLine()); + fs.symlinkSync(realFile, path.join(root, 'src', 'linked.cts')); + + const violations = driftGuard.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].file, path.join('vendor', 'real-target.cts')); +}); + +test('root confinement holds', { skip: process.platform === 'win32' ? 'symlink creation needs privilege on Windows' : false }, (t) => { + const root = createTempDir('gsd-milestone-window-drift-root-'); + const outside = createTempDir('gsd-milestone-window-drift-outside-'); + t.after(() => { + cleanup(root); + cleanup(outside); + }); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + const outsideDir = path.join(outside, 'dir'); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, 'evil.cts'), violatingLine()); + fs.symlinkSync(outsideDir, path.join(root, 'src', 'outdir'), 'dir'); + + const violations = driftGuard.scanRepo(root); + assert.strictEqual(violations.length, 0); +}); + +test('report output is sanitized', () => { + // findMilestoneWindowDrift returns the RAW fragment; main() sanitizes both + // `file` and `found` at the reporting boundary via the shared + // sanitizeForReport (scripts/lib/drift-scan.cjs) before writing to stderr. + // Assert that boundary actually escapes the hazardous classes a violating + // line/path could carry: C0/C1 control bytes and bidi override codepoints. + assert.strictEqual(sanitizeForReport(String.fromCharCode(0x1b)), '\\x1b'); + assert.strictEqual(sanitizeForReport('‮'), '\\u202e'); + const text = violatingLine().trim(); + assert.strictEqual(sanitizeForReport(text), text, 'ordinary regex-literal punctuation must pass through unchanged'); +}); + +// ═════════════════════════════════════════════════════════════════════════ +// Section G — Property tests (fast-check, document-shaped, #2371) +// ═════════════════════════════════════════════════════════════════════════ +// +// Generators build documents from a heading/prose/fence/bullet alphabet, +// tracking each block's own offset/level/marker/phase-ness as it is +// assembled -- never by calling tokenizeHeadings, the milestone regexes, or +// any ROADMAP writer. The oracle (expected boundary) is computed from that +// SAME independently-tracked block metadata, not from the parser under +// test, so the property can fail against a real regression. + +const SAFE_WORD = fc.stringMatching(/^[A-Za-z][A-Za-z0-9]{0,8}$/); + +const headingBlockGen = fc.record({ + kind: fc.constant('heading'), + level: fc.integer({ min: 1, max: 6 }), + isPhase: fc.boolean(), + hasMarker: fc.boolean(), + word: SAFE_WORD, +}).map((b) => ({ + ...b, + render() { + const prefix = '#'.repeat(this.level); + const phasePart = this.isPhase ? `Phase 3: ` : ''; + const markerPart = this.hasMarker ? ' v2.0' : ''; + return `${prefix} ${phasePart}${this.word}${markerPart}`; + }, +})); + +const proseBlockGen = SAFE_WORD.map((word) => ({ + kind: 'prose', + render() { return `prose ${word} line`; }, +})); + +const fenceBlockGen = SAFE_WORD.map((word) => ({ + kind: 'fence', + render() { return ['```text', `## fake ${word} v9.9`, '```'].join('\n'); }, +})); + +const bulletBlockGen = SAFE_WORD.map((word) => ({ + kind: 'bullet', + render() { return `- [ ] ${word}`; }, +})); + +const blockGen = fc.oneof(headingBlockGen, proseBlockGen, fenceBlockGen, bulletBlockGen); + +// G1: computeMilestoneSectionEnd always returns content.length or a valid +// same-or-shallower, non-Phase, marker-carrying heading offset, and always +// strictly after headingStart. +test('section end is always a valid boundary or EOF', () => { + const documentGen = fc.record({ + before: fc.array(blockGen, { maxLength: 4 }), + // The target heading itself must be a valid milestone-heading shape + // (level 1-3, mirrors locateMilestoneHeadings's own #{1,3} precondition + // -- computeMilestoneSectionEnd is never called in production with a + // deeper headingText). + target: fc.record({ + kind: fc.constant('heading'), + level: fc.integer({ min: 1, max: 3 }), + isPhase: fc.constant(false), + hasMarker: fc.constant(true), + word: SAFE_WORD, + }).map((b) => ({ ...b, render() { return `${'#'.repeat(this.level)} ${this.word} v1.0`; } })), + after: fc.array(blockGen, { minLength: 1, maxLength: 6 }), + }); + + fc.assert( + fc.property(documentGen, ({ before, target, after }) => { + const blocks = [...before, target, ...after]; + let offset = 0; + const rendered = []; + const withOffsets = blocks.map((b) => { + const text = b.render(); + const o = offset; + rendered.push(text); + offset += text.length + 1; // +1 for the '\n' join separator + return { ...b, text, offset: o }; + }); + const content = rendered.join('\n'); + const targetEntry = withOffsets[before.length]; + + const end = computeMilestoneSectionEnd(content, targetEntry.text, targetEntry.offset); + + assert.ok(end > targetEntry.offset, `end (${end}) must be strictly after headingStart (${targetEntry.offset})`); + + if (end === content.length) return true; + + const expectedBoundary = withOffsets + .slice(before.length + 1) + .find((b) => b.kind === 'heading' && b.level <= targetEntry.level && !b.isPhase && b.hasMarker); + assert.ok(expectedBoundary, `expected a boundary block at end=${end} but none was tracked`); + assert.strictEqual(end, expectedBoundary.offset); + return true; + }), + { seed: 3184, numRuns: 200 }, + ); +}); + +// G2: classifyMilestoneWindow never returns TRUNCATED when the document has +// zero phase entries -- a pure decision-table property, no document text at all. +test('truncation requires phases outside the window', () => { + const inputGen = fc.record({ + readable: fc.boolean(), + versionResolved: fc.boolean(), + hasVersionedMilestones: fc.boolean(), + headingFound: fc.boolean(), + windowHasPhaseEntries: fc.boolean(), + documentHasPhaseEntries: fc.constant(false), + }); + fc.assert( + fc.property(inputGen, (input) => { + const scope = classifyMilestoneWindow(input); + assert.notStrictEqual(scope, SCOPE.TRUNCATED); + return true; + }), + { seed: 3184, numRuns: 200 }, + ); +}); + +// G3: classification is invariant under LF<->CRLF conversion of the same document. +test('classification is newline-invariant', (t) => { + const documentGen = fc.record({ + versioned: fc.boolean(), + blocks: fc.array(blockGen, { minLength: 1, maxLength: 6 }), + }); + + const report = fc.check( + fc.property(documentGen, ({ versioned, blocks }) => { + const headingLine = versioned ? '## v1.0 Current 🚧' : null; + const lines = headingLine ? [headingLine, ...blocks.map((b) => b.render())] : blocks.map((b) => b.render()); + const lfContent = lines.join('\n'); + const crlfContent = lines.join('\n').replace(/\n/g, '\r\n'); + + const lfCwd = createTempDir('gsd-milestone-window-g3-lf-'); + const crlfCwd = createTempDir('gsd-milestone-window-g3-crlf-'); + if (versioned) { + writeState(lfCwd, { milestone: 'v1.0' }); + writeState(crlfCwd, { milestone: 'v1.0' }); + } + const lfScope = extractCurrentMilestoneScoped(lfContent, lfCwd).scope; + const crlfScope = extractCurrentMilestoneScoped(crlfContent, crlfCwd).scope; + cleanup(lfCwd); + cleanup(crlfCwd); + return lfScope === crlfScope; + }), + { seed: 3184, numRuns: 50 }, + ); + if (report.failed) { + t.diagnostic(`G3 counterexample: ${JSON.stringify(report.counterexample)}`); + } + assert.strictEqual(report.failed, false, 'classification must be newline-invariant'); +});