diff --git a/.changeset/tidy-bears-swim.md b/.changeset/tidy-bears-swim.md new file mode 100644 index 000000000..a20786cd2 --- /dev/null +++ b/.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) diff --git a/src/state.cts b/src/state.cts index 0e8199266..a4778f1c1 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 } { @@ -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' }, 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( 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 = [ '---',