fix(#3262): guard phase writes against milestone-scope headings (#3446)

* fix(#3262): guard phase writes against milestone-scope headings * fix(#3262): fill changeset pr with 3446 --------- Co-authored-by: sim <sim@local>
2026-08-14 01:44:54 -04:00
parent 8757080676
commit fd4715f80f
12 changed files with 553 additions and 40 deletions
--- a/src/command-aliases.cts
+++ b/src/command-aliases.cts
@@ -628,6 +628,14 @@ export const ROADMAP_COMMAND_ALIASES: CommandAlias[] = [
    "subcommand": "analyze",
    "mutation": false
  },
+  {
+    "canonical": "roadmap.milestone-scope",
+    "aliases": [
+      "roadmap milestone-scope"
+    ],
+    "subcommand": "milestone-scope",
+    "mutation": false
+  },
  {
    "canonical": "roadmap.get-phase",
    "aliases": [
--- a/src/phase.cts
+++ b/src/phase.cts
@@ -53,7 +53,7 @@ import phaseLocatorMod = require('./phase-locator.cjs');
 const { findPhaseInternal, getArchivedPhaseDirs, listMilestonePhaseDirs } = phaseLocatorMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module
 import roadmapParserMod = require('./roadmap-parser.cjs');
-const { stripShippedMilestones, extractCurrentMilestone, currentMilestoneRawRanges, withPhaseSection } = roadmapParserMod;
+const { stripShippedMilestones, extractCurrentMilestone, currentMilestoneRawRanges, withPhaseSection, findMilestoneScopeHeadingLines } = roadmapParserMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module
 import planningWorkspace = require('./planning-workspace.cjs');
 // eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module
@@ -1030,10 +1030,36 @@ function phaseEntryInsertOffset(rawContent: string, cwd: string): number {
  return lastSeparator > 0 ? ranges.primary.start + lastSeparator : ranges.primary.end;
 }

+/**
+ * #3262 (write-time milestone-scope guard): the phase-creation and
+ * phase-insertion entry templates interpolate the caller's `description`
+ * verbatim into `### Phase N: ${description}`. A description embedding a
+ * level 1-3 heading that carries a milestone marker (version token,
+ * ✅/📋/🚧/🔄, or the word "Milestone") would splice a heading that TERMINATES
+ * the current milestone window (`computeMilestoneSectionEnd`) and silently
+ * drops every later phase out of the derived milestone phase set. Reject
+ * before any write or phase-directory creation — the fail-loud sibling of
+ * the edit-phase workflow's depends_on gate. The predicate itself
+ * (`findMilestoneScopeHeadingLines`) is fence-aware and Phase-heading-exempt,
+ * so ordinary descriptions and the phase's own numbered heading never trip it.
+ */
+function assertDescriptionPreservesMilestoneScope(description: string, command: string): void {
+  const offending = findMilestoneScopeHeadingLines(description);
+  if (offending.length === 0) return;
+  error(
+    `${command}: description contains a milestone-scoping heading line — writing it to ROADMAP.md would terminate ` +
+      `the current milestone window and silently drop later phases out of the milestone scope. ` +
+      `Offending line(s): ${offending.map((line) => JSON.stringify(line)).join(', ')}. ` +
+      `Rewrite the line so it is not a level 1-3 "#" heading carrying a milestone marker ` +
+      `(a vN.N version token, a ✅/📋/🚧/🔄 marker, or the word "Milestone").`
+  );
+}
+
 function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: string): void {
  if (!description) {
    error('description required for phase add');
  }
+  assertDescriptionPreservesMilestoneScope(description, 'phase add');

  const config = loadConfig(cwd);
  const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
@@ -1149,6 +1175,12 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo
  if (!Array.isArray(descriptions) || descriptions.length === 0) {
    error('descriptions array required for phase add-batch');
  }
+  // #3262: validate every description BEFORE the lock — the batch is
+  // all-or-nothing, so one offending description must reject the whole batch
+  // with no ROADMAP write and no phase directories created.
+  for (const description of descriptions) {
+    assertDescriptionPreservesMilestoneScope(description, 'phase add-batch');
+  }
  const config = loadConfig(cwd);
  const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
  if (!fs.existsSync(roadmapPath)) {
@@ -1230,6 +1262,7 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra
  if (!afterPhase || !description) {
    error('after-phase and description required for phase insert');
  }
+  assertDescriptionPreservesMilestoneScope(description, 'phase insert');

  const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
  if (!fs.existsSync(roadmapPath)) {
--- a/src/roadmap-command-router.cts
+++ b/src/roadmap-command-router.cts
@@ -36,6 +36,7 @@ const { SCOPE } = planningScopeMod;
 interface RoadmapModule {
  cmdRoadmapGetPhase(cwd: string, phase: string | undefined, raw: boolean): void;
  cmdRoadmapAnalyze(cwd: string, raw: boolean): void;
+  cmdRoadmapMilestoneScope(cwd: string, raw: boolean): void;
  cmdRoadmapUpdatePlanProgress(cwd: string, phase: string | undefined, raw: boolean): void;
  cmdRoadmapAnnotateDependencies(cwd: string, phase: string | undefined, raw: boolean): void;
 }
@@ -148,6 +149,9 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom
    handlers: {
      'get-phase': () => roadmap.cmdRoadmapGetPhase(cwd, args[2], raw),
      analyze: () => roadmap.cmdRoadmapAnalyze(cwd, raw),
+      // #3262: read-only milestone-window identity probe — the capture/compare
+      // signal for the edit-phase workflow's write-time milestone-scope guard.
+      'milestone-scope': () => roadmap.cmdRoadmapMilestoneScope(cwd, raw),
      'update-plan-progress': () => roadmap.cmdRoadmapUpdatePlanProgress(cwd, args[2], raw),
      'annotate-dependencies': () => roadmap.cmdRoadmapAnnotateDependencies(cwd, args[2], raw),
      'validate': () => {
--- a/src/roadmap-parser.cts
+++ b/src/roadmap-parser.cts
@@ -427,6 +427,89 @@ function hasPhaseEntries(markdown: string): boolean {
  return BULLET_PHASE_LINE_PATTERN.test(stripFencedCode(markdown).text);
 }

+/**
+ * #3262: the sole owner of "which phase ids does THIS milestone window
+ * declare". Extracted verbatim from `getMilestonePhaseFilter`'s former inline
+ * heading scan + bullet scan so the new `roadmap milestone-scope` probe (the
+ * write-time milestone-scope guard's capture/compare signal) reads the SAME
+ * derivation the phase filter builds its membership set from — never a second
+ * copy of either scan.
+ *
+ * Fence-aware on both scans (tokenizeHeadings + stripFencedCode), matching
+ * `hasPhaseEntries` above: a fenced markdown EXAMPLE of either syntax is not
+ * a declared phase.
+ *
+ * #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
+ * leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
+ * phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
+ * narrower question — "which phase ids does this window declare" — where only
+ * the 999 icebox range is excluded.
+ */
+function scanMilestonePhaseIds(window: string): Set<string> {
+  const ids = new Set<string>();
+  // Use tokenizeHeadings (fence-aware) instead of stripFencedLines + regex.
+  // T4 seam migration: phase headings inside fences are excluded automatically.
+  // #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(window)) {
+    if (h.level < 2 || h.level > 4) continue;
+    const pm = phaseHeadingPattern.exec(h.text);
+    if (pm && !/^999\b/.test(pm[1])) ids.add(pm[1]);
+  }
+  // #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.
+  let bm: RegExpExecArray | null;
+  const scanner = new RegExp(BULLET_PHASE_LINE_PATTERN.source, 'gim');
+  const unfenced = stripFencedCode(window).text;
+  while ((bm = scanner.exec(unfenced)) !== null) {
+    if (!/^999\b/.test(bm[1])) ids.add(bm[1]);
+  }
+  return ids;
+}
+
+/**
+ * #3262 (write-time milestone-scope guard): does this free-text value contain
+ * a heading line that would TERMINATE the current milestone window if spliced
+ * into ROADMAP.md? Returns the offending heading texts (empty array = safe).
+ *
+ * Mirrors the parser's own terminator vocabulary (`computeMilestoneSectionEnd`):
+ * a heading terminates the window when it is level 1-3, is NOT a Phase heading
+ * (`/^Phase\s+\S/i` — the phase's OWN numbered heading is existing, correct,
+ * load-bearing behavior and is never a violation), and carries a milestone
+ * signal. The signal test is the union of `MILESTONE_HEADING_SIGNAL_PATTERN`
+ * (this module's "is this heading a milestone heading" vocabulary) and `🔄`
+ * (which terminates in `extractCurrentMilestoneScoped`'s own preamble pattern)
+ * — deliberately the CONSERVATIVE union: a field value whose line is a level
+ * 1-3 heading naming a version, a status marker, or the word "Milestone" is
+ * exactly the shape that silently narrows the window, so the guard rejects on
+ * any of them rather than re-deriving which specific marker a given roadmap's
+ * terminator would fire on.
+ *
+ * Two deliberate conservatisms, both one-directional (reject more, never less):
+ *  - `computeMilestoneSectionEnd` also bounds by the milestone heading's own
+ *    level (a `###` marker only terminates a `###`-level milestone heading);
+ *    this predicate flags every level 1-3 marker regardless, because which
+ *    level the active milestone heading uses is a property of the document at
+ *    write time, not of the text being validated.
+ *  - level 4+ headings never terminate any window and are not flagged.
+ *
+ * Fence-aware via `tokenizeHeadings`: a FENCED example of a milestone heading
+ * inside a field value does not terminate the real window, so it must not be
+ * a violation either — the parser and this guard must agree on fences.
+ */
+function findMilestoneScopeHeadingLines(text: string): string[] {
+  const out: string[] = [];
+  for (const h of tokenizeHeadings(text)) {
+    if (h.level > 3) continue;
+    if (/^Phase\s+\S/i.test(h.text)) continue;
+    if (MILESTONE_HEADING_SIGNAL_PATTERN.test(h.text) || /🔄/.test(h.text)) {
+      out.push(h.text.trim());
+    }
+  }
+  return out;
+}
+
 /**
 * #3184: pure decision table (no I/O, no regex construction from caller
 * data) implementing the design's Behavior table rows 1-8 (the remaining
@@ -1294,39 +1377,11 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p
      });
    }

-    // Use tokenizeHeadings (fence-aware) instead of stripFencedLines + regex.
-    // T4 seam migration: phase headings inside fences are excluded automatically.
-    // #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(roadmap)) {
-      if (h.level < 2 || h.level > 4) continue;
-      const pm = phaseHeadingPattern.exec(h.text);
-      // #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
-      // leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
-      // phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
-      // narrower question -- "which phase ids does this milestone's window
-      // declare" -- where only the 999 icebox range is excluded.
-      if (pm && !/^999\b/.test(pm[1])) milestonePhaseNums.add(pm[1]);
-    }
-    // #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');
-      const roadmapUnfenced = stripFencedCode(roadmap).text;
-      while ((bm = scanner.exec(roadmapUnfenced)) !== null) {
-        // #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
-        // leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
-        // phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
-        // narrower question -- "which phase ids does this milestone's window
-        // declare" -- where only the 999 icebox range is excluded.
-        if (!/^999\b/.test(bm[1])) milestonePhaseNums.add(bm[1]);
-      }
+    // #3262: the set-building scan now lives in its own named owner
+    // (`scanMilestonePhaseIds`) so the new `roadmap milestone-scope` probe
+    // reads the SAME derivation this filter does — never a second copy.
+    for (const id of scanMilestonePhaseIds(roadmap)) {
+      milestonePhaseNums.add(id);
    }
  } catch {
    /* best-effort (#2245 audit): the real throw source is platformReadSync
@@ -1531,4 +1586,10 @@ export = {
  // #1956: sole owner of the #2012 decoy-avoidance scope for the
  // `drift-guard phase-status` CLI seam.
  findRoadmapProgressTable,
+  // #3262 (write-time milestone-scope guard): the window phase-id scan owner
+  // (consumed by getMilestonePhaseFilter above and the roadmap milestone-scope
+  // CLI probe) and the free-text predicate the phase add/add-batch/insert
+  // guards and the edit-phase workflow's pre/post capture are built on.
+  scanMilestonePhaseIds,
+  findMilestoneScopeHeadingLines,
 };
--- a/src/roadmap.cts
+++ b/src/roadmap.cts
@@ -26,7 +26,7 @@ const { SCOPE } = planningScopeMod;
 type Scope = planningScopeMod.Scope;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import roadmapParserModule = require('./roadmap-parser.cjs');
-const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings } = roadmapParserModule;
+const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings, scanMilestonePhaseIds } = roadmapParserModule;
 import { tokenizeHeadings } from './markdown-sectionizer.cjs';
 import { updateTableCell } from './markdown-table.cjs';
 import { clampPercent } from './phase-lifecycle.cjs';
@@ -622,6 +622,40 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void {
  output(result, raw, undefined);
 }

+// ─── cmdRoadmapMilestoneScope ────────────────────────────────────────────────
+
+/**
+ * #3262 (write-time milestone-scope guard): read-only probe emitting the
+ * current milestone window's IDENTITY — its scope classification and the
+ * phase ids it declares — so the edit-phase workflow can capture it before
+ * its in-place section write, re-derive it after, and roll back on any
+ * change. This is the milestone-scope sibling of the workflow's existing
+ * `depends_on` gate, expressed as a command because the workflow's write is
+ * assistant-driven free-text surgery, not a code path.
+ *
+ * Deliberately NOT `cmdRoadmapAnalyze`: analyze's #3165 recovery re-populates
+ * `phases` from the shipped-milestone-stripped document when the scoped
+ * window is suspect, which is right for a human-facing progress report and
+ * wrong for a before/after equality probe — the refill would mask exactly
+ * the narrowing this guard exists to detect. This probe reports the RAW
+ * window (`extractCurrentMilestoneScoped` + `scanMilestonePhaseIds`), no
+ * fallback, so a narrowed window is always visible as a changed phase set.
+ */
+function cmdRoadmapMilestoneScope(cwd: string, raw: boolean): void {
+  const roadmapPath = planningPaths(cwd).roadmap;
+
+  if (!fs.existsSync(roadmapPath)) {
+    output({ error: 'ROADMAP.md not found', scope: SCOPE.UNREADABLE, phases: [], phase_count: 0 }, raw, undefined);
+    return;
+  }
+
+  const rawContent = fs.readFileSync(roadmapPath, 'utf-8');
+  const { value: window, scope } = extractCurrentMilestoneScoped(rawContent, cwd);
+  // Document order (Set insertion order) — deterministic for a given document.
+  const phases = [...scanMilestonePhaseIds(window)];
+  output({ scope, phases, phase_count: phases.length }, raw, undefined);
+}
+
 // ─── cmdRoadmapUpdatePlanProgress ─────────────────────────────────────────────

 /**
@@ -1153,6 +1187,7 @@ export = {
  cmdRoadmapGetPhase,
  getRoadmapPhaseWithFallback,
  cmdRoadmapAnalyze,
+  cmdRoadmapMilestoneScope,
  cmdRoadmapUpdatePlanProgress,
  cmdRoadmapAnnotateDependencies,
  buildPhaseHeadingRegex,