From 4f6fda852e57913e2f41f92ce9ef6bf6f423bc05 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 28 Jun 2026 22:41:57 -0400 Subject: [PATCH] fix(#1591): phase.complete recognizes checkbox-list phases in isLastPhase fallback (#1819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1591): phase.complete recognizes checkbox-list phases in the isLastPhase fallback When the active milestone's phase checklist is written as `- [ ] Phase N:` checkbox items inside a
block (the @Azd325 structure) and the next phase has no directory yet, the disk-based next-phase resolver finds nothing and phase.complete falls back to the roadmap-enumeration guard at the isLastPhase site. That guard's phasePattern was heading-only (/#{2,4}\s*Phase…/), so it never matched checklist items → is_last_phase=true and next_phase=null on a mid-milestone phase, and STATE.md was wrongly marked 'Milestone complete' with total_phases decremented. Broaden the marker alternation to match BOTH heading-style (### Phase N:) and checkbox-list items (- [ ] Phase N: / - [x] Phase N:); the number/name captures are unchanged. extractCurrentMilestone already surfaces the
-wrapped checklist correctly, so no parser change is needed. The heading-only sibling patterns elsewhere in phase.cts are left untouched (scope discipline — only the reproduced isLastPhase fallback is changed). Regression: a phase complete 36 on a
-wrapped v2.0 checklist (Phases 36-38, only 36 has a dir) returns is_last_phase=false, next_phase=37, and does NOT flip STATE.md to 'Milestone complete'. * docs(#1591): add changeset fragment for phase.complete checkbox-list fix * test(#1752): add total_phases-preservation regression for the #1591 follow-up #1752 is the scoped follow-up to #1591 — same
-wrapped-checkbox defect, with the additional emphasis on the total_phases decrement cascade. The #1591 fix (is_last_phase=false) already resolves it: with all 8 phase dirs on disk, phase.complete 36 on a v2.0
checklist leaves total_phases at 8 (not decremented to 7) and does not flip STATE.md to 'Milestone complete'. Verified manually before adding the test. Add the #1752 regression case (8 phase dirs, curated total_phases: 8) to the phase complete command block in tests/phase.test.cjs, and update the changeset to reference both issues (#1591, #1752) since this is one user-facing change resolving both. --- ...hase-complete-details-wrapped-checklist.md | 6 + src/phase.cts | 11 +- tests/phase.test.cjs | 181 ++++++++++++++++++ 3 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 .changeset/1591-phase-complete-details-wrapped-checklist.md diff --git a/.changeset/1591-phase-complete-details-wrapped-checklist.md b/.changeset/1591-phase-complete-details-wrapped-checklist.md new file mode 100644 index 000000000..c6814916b --- /dev/null +++ b/.changeset/1591-phase-complete-details-wrapped-checklist.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 1819 +--- +**`phase.complete` no longer reports a false `is_last_phase` on a `
`-wrapped checkbox checklist (#1591, #1752)** — when the active milestone's phase checklist was written as `- [ ] Phase N:` checkbox items inside a `
` block and the next phase had no directory on disk yet (still in planning), `phase.complete`'s `isLastPhase` roadmap-enumeration fallback used a heading-only pattern (`/#{2,4}\s*Phase…/`) that never matched checkbox items. It returned `is_last_phase: true, next_phase: null` on a mid-milestone phase and — via the milestone-complete cascade — wrongly flipped STATE.md to `Milestone complete` and decremented `progress.total_phases` (e.g. 8 → 7). The pattern now matches both heading-style (`### Phase N:`) and checkbox-list phases (`- [ ] Phase N:` / `- [x] Phase N:`); `extractCurrentMilestone` already surfaces the `
`-wrapped checklist correctly, so no parser change was needed. Only the reproduced `phase.complete` fallback is changed; the heading-only sibling patterns elsewhere in `phase.cts` are untouched. + diff --git a/src/phase.cts b/src/phase.cts index 76ef1a74c..8b463f3e5 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1648,7 +1648,16 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { if (isLastPhase && roadmapContent !== null) { try { const roadmapForPhases = extractCurrentMilestone(roadmapContent, cwd); - const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi; + // #1591: match BOTH heading-style phases (`### Phase N:`) AND + // checkbox-list items (`- [ ] Phase N:` / `- [x] Phase N:`). When + // the active milestone's checklist is `- [ ]` items inside a + //
block (and the next phase has no directory yet, so the + // disk-based resolver finds nothing), this roadmap-enumeration + // fallback is the only path that can find the next phase. The prior + // heading-only pattern missed checkbox items → is_last_phase=true on + // a mid-milestone phase. The marker alternation is the only change; + // the number/name captures are unchanged. + const phasePattern = /(?:#{2,4}|-\s*\[[ xX]\])\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi; let pm: RegExpExecArray | null; while ((pm = phasePattern.exec(roadmapForPhases)) !== null) { if (comparePhaseNum(pm[1], phaseNum) > 0) { diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 62c4c90c7..354c07a4c 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2362,6 +2362,187 @@ describe('phase complete command', () => { assert.ok(state.includes('Milestone complete'), 'status should be milestone complete'); }); + // #1591: when the active milestone's phase checklist is wrapped in a + //
block AND phases are written as `- [ ] Phase N:` checkbox list + // items (not `### Phase N:` headings), phase.complete's next-phase enumerator + // saw no further phases → is_last_phase=true, next_phase=null on a mid- + // milestone phase, and STATE.md was wrongly marked "Milestone complete" with + // total_phases decremented. extractCurrentMilestone correctly surfaces the + //
-wrapped checklist; the defect was the heading-only phasePattern + // at the isLastPhase enumerator not recognizing checkbox-list phase items. + test('#1591:
-wrapped checkbox checklist — mid-milestone phase is NOT last', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# ROADMAP', + '', + '## Phases', + '', + '
', + '✅ v1.0 First (Phases 1–3) — SHIPPED', + '', + '- [x] Phase 1: a', + '- [x] Phase 2: b', + '- [x] Phase 3: c', + '', + '
', + '', + '
', + '🚀 v2.0 Second (Phases 36–38) — IN PLANNING', + '', + '- [x] Phase 36: first (completed)', + '- [ ] Phase 37: second', + '- [ ] Phase 38: third', + '', + '
', + '', + '## Backlog', + '', + '### Phase 999.1: future (BACKLOG)', + '', + ].join('\n') + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v2.0', + 'milestone_name: Second', + 'current_phase: "36"', + 'status: executing', + '---', + '', + '# GSD State', + '', + '**Current Phase:** 36', + '**Status:** Executing Phase 36', + '', + ].join('\n') + ); + // Only the COMPLETING phase (36) has a directory. Phases 37/38 exist only + // as `- [ ]` checklist items in the ROADMAP — they are not yet started, so + // they have no phase dirs. This is the @Azd325 scenario: the disk-based + // next-phase resolver finds nothing, and the roadmap-enumeration fallback + // (the heading-only phasePattern) is the only path that can find Phase 37. + const d36 = path.join(tmpDir, '.planning', 'phases', '36-first'); + fs.mkdirSync(d36, { recursive: true }); + fs.writeFileSync(path.join(d36, '36-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(d36, '36-SUMMARY.md'), '# Summary\n'); + + const result = runVerifiedPhaseComplete('phase complete 36', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error}`); + const output = JSON.parse(result.output); + + assert.strictEqual( + output.is_last_phase, + false, + 'Phase 36 of 36–38 must NOT be last — Phases 37/38 are still open `- [ ]` (#1591)', + ); + assert.strictEqual( + output.next_phase, + '37', + 'next_phase must resolve to 37 from the
-wrapped checkbox checklist (#1591)', + ); + + // Cascade check: a wrong is_last_phase=true previously wrote "Milestone + // complete" + decremented total_phases. With the fix, the milestone is + // still in progress. + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + !/Milestone complete/i.test(state), + 'a mid-milestone phase must not flip STATE.md to "Milestone complete" (#1591)', + ); + }); + + // #1752: the #1591 follow-up — when phase.complete wrongly returned + // is_last_phase=true on a
-wrapped mid-milestone checklist, the + // milestone-complete cascade also DECREMENTED progress.total_phases (e.g. + // 8 -> 7) and flipped status. Same root cause, distinct symptom. With the + // #1591 fix (is_last_phase=false), the decrement must not occur: with all 8 + // phase dirs on disk, total_phases stays 8 and status does not flip. + test('#1752:
-wrapped checklist — total_phases is NOT decremented on a mid-milestone phase', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# ROADMAP', + '', + '## Phases', + '', + '
', + '✅ v1.0 First (Phases 1–3) — SHIPPED', + '', + '- [x] Phase 1: a', + '- [x] Phase 2: b', + '- [x] Phase 3: c', + '', + '
', + '', + '
', + '🚀 v2.0 Second (Phases 36–43) — IN PLANNING', + '', + '- [x] Phase 36: first (completed)', + '- [ ] Phase 37: second', + '- [ ] Phase 38: third', + '- [ ] Phase 39: fourth', + '', + '
', + '', + ].join('\n') + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v2.0', + 'milestone_name: Second', + 'current_phase: "36"', + 'status: executing', + 'progress:', + ' total_phases: 8', + ' completed_phases: 5', + ' percent: 62', + '---', + '', + '# GSD State', + '', + '**Current Phase:** 36', + '**Status:** Executing Phase 36', + '', + ].join('\n') + ); + // All 8 phase dirs on disk (Phases 36–43) so the disk count is 8 — the + // reporter's real state. Before the #1591 fix, phase.complete 36 returned + // is_last_phase=true (no Phase 37+ heading match) and the milestone-complete + // path DECREMENTED total_phases 8 -> 7. + const names = ['first', 'second', 'third', 'fourth', 'fifth', 'sixth', 'seventh', 'eighth']; + for (let i = 0; i < 8; i++) { + const num = String(36 + i); + const d = path.join(tmpDir, '.planning', 'phases', `${num}-${names[i]}`); + fs.mkdirSync(d, { recursive: true }); + fs.writeFileSync(path.join(d, `${num}-PLAN.md`), '# Plan\n'); + } + + const result = runVerifiedPhaseComplete('phase complete 36', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual(output.is_last_phase, false, 'is_last_phase must be false (#1752 cascade root)'); + + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + !/Milestone complete/i.test(state), + 'a mid-milestone phase must not flip STATE.md to "Milestone complete" (#1752)', + ); + const tpMatch = state.match(/total_phases:\s*(\d+)/); + assert.ok(tpMatch, 'STATE.md must carry a total_phases value after phase.complete'); + assert.notStrictEqual( + parseInt(tpMatch[1], 10), + 7, + 'total_phases must NOT be decremented to 7 — the #1752 cascade of the false is_last_phase', + ); + }); + test('updates REQUIREMENTS.md traceability when phase completes', () => { fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'),