From bd5fcfceeb94c6c6925bb173781d4ff7501a620f Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Thu, 9 Jul 2026 18:11:55 -0400
Subject: [PATCH] fix(#2125): route complete-phase resolver through canonical
 parser (review)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Orthogonal review surfaced that resolvePhaseIdForCompletePhase (state.cts) and
cmdStateCompletePhase's idempotency check still used an unanchored
/(\d+[A-Z]?(?:\.\d+)*)/i — even more permissive than the parseProsePhaseField
regex this phase fixes. Reachable corruption: after `milestone complete v0.5`,
`state complete-phase` (no --phase) mined "0.5" from the body line
"Phase: Milestone v0.5 complete" and rewrote STATE.md as "Phase 0.5 complete".

Both sites now delegate to phase-id.cts:parsePhaseFromProse (the same anchored
parser), so a milestone-closure line yields no token and the existing
"unable to resolve" guard fires instead of corrupting. Canonical tokens
(3, 03, 3A, 3.3, "3 of 5", "1 — Setup") are preserved unchanged.

Regression (tests/state.test.cjs, complete-phase suite): `state complete-phase`
on a "Milestone v0.5 complete" STATE.md now rejects and does not mine "0.5".
Demonstrated fail-first.

Refs #2125, #2121

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 src/state.cts        | 15 ++++++++++-----
 tests/state.test.cjs | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/src/state.cts b/src/state.cts
index 37b859091..a4778f1c1 100644
--- a/src/state.cts
+++ b/src/state.cts
@@ -2713,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 {
@@ -2742,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' },
diff --git a/tests/state.test.cjs b/tests/state.test.cjs
index 7167579e6..94163ec52 100644
--- a/tests/state.test.cjs
+++ b/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 = [
       '---',