diff --git a/.changeset/2067-phase-complete-checkbox-regex.md b/.changeset/2067-phase-complete-checkbox-regex.md new file mode 100644 index 000000000..892a3763a --- /dev/null +++ b/.changeset/2067-phase-complete-checkbox-regex.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2079 +--- +**`phase complete` no longer ticks the wrong phase's ROADMAP checkbox** — completing a phase whose number also appears in a later phase's description (e.g. an idempotent re-run of an already-complete phase) used to mark the *wrong* phase done, because the checkbox-matching regex greedily spanned from `]` to any later "Phase N" mention instead of only the immediately-following phase title. (#2067) diff --git a/src/phase.cts b/src/phase.cts index 51d18284b..deb41fe4b 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1467,8 +1467,14 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { roadmapContent = originalRoadmapContent; const phaseEscaped = phaseMarkdownRegexSource(phaseNum); + // #2067: the gap between `]` and `Phase N` must allow only whitespace / + // markdown bold emphasis — NOT greedy `.*`. A greedy gap matched a later + // phase whose description merely mentioned the completed phase number, + // so completing an already-checked phase (idempotent re-run) checked the + // wrong phase's box. Mirrors the tight pattern used by phase-insert + // (`]\\s*(?:\\*\\*)?Phase`). const checkboxPattern = new RegExp( - `(-\\s*\\[)[ ](\\]\\s*.*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`, + `(-\\s*\\[)[ ](\\]\\s*(?:\\*\\*)?\\s*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`, 'i', ); roadmapContent = roadmapContent.replace( diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 1a542c914..82e1cdf45 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2530,6 +2530,95 @@ describe('phase complete command', () => { assert.ok(roadmap.includes('completed'), 'completion date should be added'); }); + // #2067: the checkbox regex in cmdPhaseComplete used a greedy `.*` between + // `]` and `Phase N`, so completing Phase 1 (already checked → idempotent + // re-run) matched a LATER phase whose description merely mentioned "Phase 1". + // Non-global replace then checked the wrong phase's box. The gap between `]` + // and `Phase` must allow only whitespace / markdown emphasis. + test('#2067 — completing a phase must not check a later phase whose description mentions it (idempotent re-run)', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap + +- [x] **Phase 1: Core Feed & Posting** - first slice (completed 2025-01-01) +- [ ] **Phase 2: Polish & Edge Cases** - hardening (only as needed after Phase 1 verification) + +### Phase 1: Core Feed & Posting +**Goal:** Ship feed +**Plans:** 1 plans + +### Phase 2: Polish & Edge Cases +**Goal:** Harden +` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 01\n**Current Phase Name:** Core Feed & Posting\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working on phase 1\n` + ); + + const p1 = path.join(tmpDir, '.planning', 'phases', '01-core-feed-posting'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary'); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-polish-edge-cases'), { recursive: true }); + + const result = runVerifiedPhaseComplete('phase complete 1', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + // Phase 2's checkbox MUST remain unchecked — its description mentions + // "Phase 1" but it is NOT the phase being completed. + const phase2Line = roadmap.match(/^- \[([ x])\] \*\*Phase 2:.*$/m); + assert.ok(phase2Line, 'Phase 2 line must still be present in ROADMAP'); + assert.strictEqual(phase2Line[1], ' ', 'Phase 2 checkbox must remain unchecked (#2067)'); + // Phase 1 must remain checked. + const phase1Line = roadmap.match(/^- \[([ x])\] \*\*Phase 1:.*$/m); + assert.ok(phase1Line, 'Phase 1 line must still be present in ROADMAP'); + assert.strictEqual(phase1Line[1], 'x', 'Phase 1 checkbox must remain checked (#2067)'); + }); + + // #2067 companion: normal completion (Phase 1 unchecked) must still check + // Phase 1 — and must NOT also check a later phase whose description mentions + // Phase 1. Guards the tightened regex against over-restricting the happy path. + test('#2067 — normal completion checks only the target phase when a later phase mentions it', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap + +- [ ] **Phase 1: Core Feed & Posting** - first slice +- [ ] **Phase 2: Polish & Edge Cases** - hardening (only as needed after Phase 1 verification) + +### Phase 1: Core Feed & Posting +**Goal:** Ship feed +**Plans:** 1 plans + +### Phase 2: Polish & Edge Cases +**Goal:** Harden +` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 01\n**Current Phase Name:** Core Feed & Posting\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working on phase 1\n` + ); + + const p1 = path.join(tmpDir, '.planning', 'phases', '01-core-feed-posting'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary'); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-polish-edge-cases'), { recursive: true }); + + const result = runVerifiedPhaseComplete('phase complete 1', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase1Line = roadmap.match(/^- \[([ x])\] \*\*Phase 1:.*$/m); + assert.ok(phase1Line, 'Phase 1 line must still be present in ROADMAP'); + assert.strictEqual(phase1Line[1], 'x', 'Phase 1 checkbox must be checked'); + const phase2Line = roadmap.match(/^- \[([ x])\] \*\*Phase 2:.*$/m); + assert.ok(phase2Line, 'Phase 2 line must still be present in ROADMAP'); + assert.strictEqual(phase2Line[1], ' ', 'Phase 2 checkbox must remain unchecked (#2067)'); + }); + test('#2012 — Progress row updated even when an earlier phase-numbered table precedes ## Progress', () => { fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'),