From e7ff7be1a466317b4d9899ae04bea60c5e7fcc9d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 9 Jul 2026 17:57:47 -0400 Subject: [PATCH] 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(