From e7ff7be1a466317b4d9899ae04bea60c5e7fcc9d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 17:57:47 -0400 Subject: [PATCH 1/4] fix(#2125): migrate state.cts prose parsing to canonical parser (drives #2111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of epic #2121. state.cts:parseProsePhaseField now delegates to the anchored phase-id.cts:parsePhaseFromProse (built in Phase 1), removing this module's independent prose phase-id regex. Drives #2111: `milestone complete vX.Y` no longer corrupts current_phase. The body line "Phase: Milestone v0.5 complete" previously had "5" mined from it by the unanchored regex; the anchored parser returns { phase: null }, so syncStateFrontmatter's #905 guard preserves the real current_phase. This also fixes the broader family the review surfaced — every milestone completion (e.g. v1.0 -> "0") was silently corrupting current_phase, not just .5-versions. Regression (tests/milestone.test.cjs, in the milestone-complete suite, #2111): `milestone complete v0.5` on a project with current_phase: "19" now preserves "19". Demonstrated fail-first end-to-end: reverting the migration reproduces current_phase = "5". Closes #2125 Refs #2121 Co-Authored-By: Claude Opus 4.8 --- src/state.cts | 25 ++++++++----------------- tests/milestone.test.cjs | 30 +++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/src/state.cts b/src/state.cts index 0e8199266..37b859091 100644 --- a/src/state.cts +++ b/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 } { diff --git a/tests/milestone.test.cjs b/tests/milestone.test.cjs index 65c0d26b9..dd65626b5 100644 --- a/tests/milestone.test.cjs +++ b/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( From bd5fcfceeb94c6c6925bb173781d4ff7501a620f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 18:11:55 -0400 Subject: [PATCH 2/4] 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 --- 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 = [ '---', From b32ecd1b87a3bbe8e738b6df077d07b79068d0d4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 18:13:03 -0400 Subject: [PATCH 3/4] docs(changeset): Fixed fragment for #2111 (pr:0 to backfill) Co-Authored-By: Claude Opus 4.8 --- .changeset/tidy-bears-swim.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tidy-bears-swim.md diff --git a/.changeset/tidy-bears-swim.md b/.changeset/tidy-bears-swim.md new file mode 100644 index 000000000..cbe01d1f9 --- /dev/null +++ b/.changeset/tidy-bears-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 0 +--- +**`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) From fea63c65e501435f14292beac8198fa8889820ae Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 18:55:58 -0400 Subject: [PATCH 4/4] docs(changeset): backfill pr 2131 for #2111 Co-Authored-By: Claude Opus 4.8 --- .changeset/tidy-bears-swim.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/tidy-bears-swim.md b/.changeset/tidy-bears-swim.md index cbe01d1f9..a20786cd2 100644 --- a/.changeset/tidy-bears-swim.md +++ b/.changeset/tidy-bears-swim.md @@ -1,5 +1,5 @@ --- type: Fixed -pr: 0 +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)