Merge branch 'next' into fix/2056-plan-phase-foreign-prefix

.changeset/tidy-bears-swim.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 2131
+---
+**`milestone complete` no longer corrupts the recorded phase** — closing a milestone (e.g. `v0.5`) previously overwrote `current_phase` in STATE.md with the version's minor digit, and a follow-up `state complete-phase` mined a bogus `0.5` token and rewrote the file; phase resolution is now anchored so the real phase is preserved and a milestone-closure line is rejected. (#2111)

src/state.cts

@@ -16,7 +16,7 @@ import configLoaderMod = require('./config-loader.cjs');
 const { loadConfig } = configLoaderMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import phaseIdMod = require('./phase-id.cjs');
-const { escapeRegex, normalizePhaseName, extractPhaseToken } = phaseIdMod;
+const { escapeRegex, normalizePhaseName, extractPhaseToken, parsePhaseFromProse } = phaseIdMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import roadmapParserMod = require('./roadmap-parser.cjs');
 const { getMilestoneInfo, getMilestonePhaseFilter, extractCurrentMilestone } = roadmapParserMod;
@@ -1116,22 +1116,13 @@ function matchSessionSection(body: string): RegExpMatchArray | null {
 }
 
 function parseProsePhaseField(value: string | null): { phase: string | null; name: string | null } {
-  if (!value) return { phase: null, name: null };
-  const phaseMatch = value.match(/\b(\d+[A-Z]?(?:\.\d+)*)\b/i);
-  // #2124 review: length-bound the name quantifiers so a crafted long
-  // unterminated `(` / `—` run in an untrusted STATE.md field cannot drive
-  // O(n^2) backtracking (CPU DoS). (Phase 2 / #2125 supersedes this function
-  // by delegating to phase-id.cts:parsePhaseFromProse, which is bounded too.)
-  const parenName = value.match(/\(([^)]{1,200})\)/);
-  const dashName = value.match(/—\s*([^(\n]{1,200}?)(?:\s*\(|$)/);
-  const rawName = parenName?.[1] ?? dashName?.[1] ?? null;
-  const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim())
-    ? rawName.trim()
-    : null;
-  return {
-    phase: phaseMatch ? phaseMatch[1] : null,
-    name,
-  };
+  // #2121 Phase 2 (#2125): delegate to the canonical anchored parser so this
+  // module holds no independent prose phase-id regex. Drives #2111 — the
+  // anchored parser returns { phase: null } for a "Milestone vX.Y complete"
+  // body line (the old unanchored regex mined the minor-version digit, e.g.
+  // v0.5 -> "5"), so syncStateFrontmatter's #905 guard preserves the real
+  // current_phase instead of clobbering it.
+  return parsePhaseFromProse(value);
 }
 
 function parseProseLastActivityField(value: string | null): { date: string | null; description: string | null } {
@@ -2722,9 +2713,13 @@ function resolvePhaseIdForCompletePhase(content: string, overridePhase: string |
     stateExtractField(content, 'Phase') ||
     '';
 
-  // Accept canonical phase token only (e.g. 3, 03, 3A, 3.3, 10.2)
-  const phaseMatch = String(candidate).match(/(\d+[A-Z]?(?:\.\d+)*)/i);
-  return phaseMatch ? phaseMatch[1] : null;
+  // #2125: parse via the canonical anchored parser so a narrative `Phase:`
+  // body line (e.g. "Milestone v0.5 complete") does not mine a bogus token —
+  // the old unanchored regex yielded "0.5" and rewrote STATE.md as
+  // "Phase 0.5 complete". A canonical token at the start of the value
+  // (3, 03, 3A, 3.3, 10.2, "3 of 5", "1 — Setup") is preserved; a milestone
+  // closure line yields null, so the caller's "unable to resolve" guard fires.
+  return parsePhaseFromProse(candidate).phase;
 }
 
 function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string): void {
@@ -2751,8 +2746,9 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
   // The handler is now a no-op in that case so re-invocation from downstream
   // workflows cannot regress the project state.
   const existingCurrentPhaseRaw = stateExtractField(content, 'Current Phase') || '';
-  const existingCurrentPhaseMatch = String(existingCurrentPhaseRaw).match(/(\d+[A-Z]?(?:\.\d+)*)/i);
-  const existingCurrentPhase = existingCurrentPhaseMatch ? existingCurrentPhaseMatch[1] : null;
+  // #2125: same canonical parser as resolvePhaseIdForCompletePhase so the two
+  // sites cannot diverge on the token they extract.
+  const existingCurrentPhase = parsePhaseFromProse(existingCurrentPhaseRaw).phase;
   if (existingCurrentPhase && existingCurrentPhase !== resolvedPhase) {
     output(
       { updated: [], phase: resolvedPhase, idempotent: true, note: 'phase already superseded; no-op' },

tests/milestone.test.cjs

@@ -16,7 +16,7 @@ const { test, describe, before, beforeEach, afterEach } = require('node:test');
 const assert = require('node:assert/strict');
 const fs = require('fs');
 const path = require('path');
-const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
+const { runGsdTools, createTempProject, cleanup, parseFrontmatter } = require('./helpers.cjs');
 
 // ─── helpers ─────────────────────────────────────────────────────────────────
 
@@ -54,6 +54,34 @@ describe('milestone complete command', () => {
   beforeEach(() => { tmpDir = createTempProject(); });
   afterEach(() => { cleanup(tmpDir); });
 
+  test('preserves current_phase frontmatter through milestone complete (#2111)', () => {
+    // Seed STATE.md mid-phase-19: the real current_phase lives in frontmatter
+    // and the only body phase source is the `Phase:` prose line (no explicit
+    // `Current Phase:` field — matching what milestoneCompleteCore writes).
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'STATE.md'),
+      `---\ncurrent_phase: "19"\n---\n# State\n\n**Status:** In progress\n` +
+      `**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n\n` +
+      `## Current Position\n\nPhase: 19 — EXECUTING\nPlan: 1 of 1\n` +
+      `Status: Executing\nLast activity: 2025-01-01 — Running phase\n`,
+    );
+    // No ROADMAP.md — mirrors 'handles missing ROADMAP.md gracefully' so the
+    // milestone-phase-filter guard never fires.
+
+    const result = runGsdTools('milestone complete v0.5 --name Test', tmpDir);
+    assert.ok(result.success, `Command failed: ${result.error}`);
+
+    const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+    const fm = parseFrontmatter(state);
+    // Fails pre-migration: the unanchored parser mined "5" from
+    // "Phase: Milestone v0.5 complete" and clobbered current_phase.
+    assert.strictEqual(
+      fm.current_phase, '19',
+      `current_phase must be preserved across milestone complete, not mined from ` +
+      `the version string (#2111); got ${JSON.stringify(fm.current_phase)}`,
+    );
+  });
+
   test('archives roadmap, requirements, creates MILESTONES.md', () => {
     writeRoadmap(tmpDir, `# Roadmap v1.0 MVP\n\n### Phase 1: Foundation\n**Goal:** Setup\n`);
     fs.writeFileSync(

tests/state.test.cjs

@@ -3055,6 +3055,39 @@ describe('state complete-phase: decorated Phase fallback (#2761 nitpick)', () =>
     assert.ok(!after.includes('Status: Phase Phase complete'));
   });
 
+  test('rejects a milestone-closure Phase line, never mines the version token (#2111 / #2125)', () => {
+    // After `milestone complete v0.5`, the only phase signal is the narrative
+    // `Phase: Milestone v0.5 complete`. The old unanchored resolver mined "0.5"
+    // and rewrote Status as "Phase 0.5 complete"; the anchored parser yields no
+    // token, so complete-phase must reject rather than corrupt STATE.md.
+    const stateMd = [
+      '---',
+      'milestone: v0.5',
+      '---',
+      '',
+      '# State',
+      '',
+      '**Status:** Awaiting next milestone',
+      '**Last Activity:** 2024-01-15',
+      '',
+      '## Current Position',
+      '',
+      'Phase: Milestone v0.5 complete',
+      '',
+    ].join('\n');
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+    fs.writeFileSync(statePath, stateMd);
+
+    const result = runGsdTools('state complete-phase', tmpDir);
+    assert.ok(result.success, 'command should return JSON error payload, not crash');
+    const output = JSON.parse(result.output);
+    assert.ok(output.error, 'expected a resolution error, not a phase mined from the version string');
+
+    const after = fs.readFileSync(statePath, 'utf-8');
+    assert.ok(!after.includes('Phase 0.5 complete'), `must not mine "0.5" from the version: ${after}`);
+    assert.ok(!after.includes('Phase: 0.5'), `must not rewrite Current Position to Phase 0.5: ${after}`);
+  });
+
   test('supports explicit phase override for complete-phase disambiguation (#3063)', () => {
     const stateMd = [
       '---',