diff --git a/.changeset/daring-finches-zip.md b/.changeset/daring-finches-zip.md new file mode 100644 index 000000000..145182e65 --- /dev/null +++ b/.changeset/daring-finches-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4820 +--- +**phase complete no longer names an already-complete phase as next** — completing a reopened phase out of order (later phases already [x]) picked the numerically-next phase even when its checkbox was already ticked, persisting it to STATE.md as current_phase. next_phase now skips phases whose roadmap checkbox is already [x], agreeing with roadmap.analyze and init.progress. (#4699) diff --git a/src/phase.cts b/src/phase.cts index ada98269c..0325fadc0 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -4237,6 +4237,37 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { let roadmapNextNum: string | null = null; let roadmapNextName: string | null = null; + // #4699: a phase whose roadmap checkbox is `[x]` is already complete and + // must never be selected as next_phase — out-of-order completion (a + // reopened phase finished after later phases shipped) otherwise persists + // the already-done phase as STATE.md current_phase. Collected from the + // same milestone-scoped text the roadmap scan walks; membership is + // comparePhaseNum-based so `02` and `2` dedupe. With no ROADMAP.md (or no + // parseable rows) the set is empty and the scans behave exactly as + // before. + const roadmapCompleteNums: string[] = []; + if (roadmapContent !== null) { + try { + const milestoneForComplete = extractCurrentMilestone(roadmapContent, cwd); + const completePattern = new RegExp( + `-\\s*\\[[xX]\\]\\s*(?:\\*\\*|__)?\\s*Phase\\s+(${PHASE_NUMBER_TOKEN_SOURCE})`, + 'gi' + ); + let cm: RegExpExecArray | null; + while ((cm = completePattern.exec(milestoneForComplete)) !== null) { + if (isSentinelPhaseId(cm[1])) continue; + if (!roadmapCompleteNums.some((n) => comparePhaseNum(cm![1], n) === 0)) { + roadmapCompleteNums.push(cm[1]); + } + } + } catch { + /* best-effort: an unreadable milestone section leaves the complete + * set empty — the scans then behave exactly as they did pre-#4699. */ + } + } + const isCompletePhaseNum = (num: string): boolean => + roadmapCompleteNums.some((n) => comparePhaseNum(num, n) === 0); + try { // #3185 (ADR-3180 Decision 1): "which phase directories belong to // the CURRENT milestone" — routed through the canonical owner @@ -4252,6 +4283,9 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { if (dm) { // #3185: canonical sentinel predicate (SENTINEL_RANGES [0,999]) — this was a local 999-only literal that admitted Phase 0. if (isSentinelPhaseId(dm[1])) continue; + // #4699: an already-complete phase (roadmap checkbox [x]) is never + // a next_phase candidate — out-of-order completion must skip it. + if (roadmapContent !== null && isCompletePhaseNum(dm[1])) continue; // Numeric MINIMUM above N, not "first encountered". `listMilestonePhaseDirs` // does sort by `comparePhaseNum`, so a `break` on the first hit happens to be // correct today — but that makes this scan's correctness depend on an @@ -4321,6 +4355,10 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // already skips sentinel dirs on disk via isSentinelPhaseId (#3185); // stage 2's heading scan must not advance into backlog headings either. if (isSentinelPhaseId(pmNum)) continue; + // #4699: skip complete phases — a `[x]` checkbox row and the + // `## Phase Details` heading of an already-done phase both name a + // phase that must never be next_phase. + if (roadmapContent !== null && isCompletePhaseNum(pmNum)) continue; // #3701 review: the numeric MINIMUM above N, not the first row above N in // DOCUMENT order. This scan walks raw roadmap text, and one global regex // sweeps both the `## Phases` checklist and the `## Phase Details` diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 9469560f8..ad1014c16 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -15812,3 +15812,140 @@ describe('bug #3982: archived details leak into lowest-outstanding scan', () => `STATE.md current_phase must not jump backwards into the archived range; got: ${state}`); }); }); + +// ── #4699 — next_phase must skip phases whose roadmap checkbox is [x] ──────── +// Out-of-order completion (a reopened phase finished after later phases +// shipped) used to persist the already-complete phase as next_phase / +// STATE.md current_phase: both next-phase scans select the numerically lowest +// phase above N without consulting completion state. Roadmap checkbox state +// is the completion rule (#2028) — an [x] phase is never "next". + +describe('phase complete skips already-complete phases as next_phase (#4699)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-4699-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function writeRoadmap({ thirdBox = '[x]', fourthBox = '[ ]' } = {}) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap + +## Phases + +- [x] **Phase 1: One** - Goal one +- [ ] **Phase 2: Two** - Goal two +- ${thirdBox} **Phase 3: Three** - Goal three +- ${fourthBox} **Phase 4: Four** - Goal four + +### Phase 1: One +**Goal**: Goal one + +### Phase 2: Two +**Goal**: Goal two + +### Phase 3: Three +**Goal**: Goal three + +### Phase 4: Four +**Goal**: Goal four +`, + ); + } + + function scaffoldPhaseDir(n, slug) { + const padded = String(n).padStart(2, '0'); + const dir = path.join(tmpDir, '.planning', 'phases', `${padded}-${slug}`); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, `${padded}-01-PLAN.md`), '# Plan'); + fs.writeFileSync(path.join(dir, `${padded}-01-SUMMARY.md`), '# Summary'); + fs.writeFileSync( + path.join(dir, `${padded}-VERIFICATION.md`), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + } + + function writeMinimalState() { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + '# State\n\n**Current Phase:** 2\n**Status:** In progress\n', + ); + } + + test('completing phase 2 out of order skips the already-complete phase 3 (#4699)', () => { + writeRoadmap(); + writeMinimalState(); + scaffoldPhaseDir(1, 'one'); + scaffoldPhaseDir(2, 'two'); + scaffoldPhaseDir(3, 'three'); + + const result = runGsdTools('phase complete 2', tmpDir); + const output = JSON.parse(result.output); + assert.equal(output.next_phase, '4', + 'next_phase must skip the already-[x] phase 3 and select the outstanding phase 4'); + assert.equal(output.is_last_phase, false); + // #4699's actual harm was persistence: STATE.md used to carry the + // already-complete phase as current_phase. + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.doesNotMatch(state, /current_phase:\s*3(\s|$)/m, + 'STATE.md must not carry the already-complete phase as current_phase'); + }); + + test('all later phases already [x] completes the milestone tail (#4699 corner)', () => { + writeRoadmap({ fourthBox: '[x]' }); + writeMinimalState(); + scaffoldPhaseDir(1, 'one'); + scaffoldPhaseDir(2, 'two'); + scaffoldPhaseDir(3, 'three'); + + const result = runGsdTools('phase complete 2', tmpDir); + const output = JSON.parse(result.output); + assert.equal(output.is_last_phase, true, + 'when every phase above N is already [x], completing N is the milestone tail'); + assert.equal(output.next_phase, null); + }); + + test('uppercase [X] checkboxes are recognized as complete (#4699)', () => { + writeRoadmap({ thirdBox: '[X]' }); + writeMinimalState(); + scaffoldPhaseDir(1, 'one'); + scaffoldPhaseDir(2, 'two'); + scaffoldPhaseDir(3, 'three'); + + const result = runGsdTools('phase complete 2', tmpDir); + const output = JSON.parse(result.output); + assert.equal(output.next_phase, '4', '[X] is a complete checkbox, case-insensitively'); + }); + + test('checkbox completion matches phase numbers across zero-padding (#4699)', () => { + // Roadmap spells the phase without padding; the directory carries the + // zero-padded token — comparePhaseNum must dedupe them in the complete set. + writeRoadmap({ thirdBox: '[x]' }); + writeMinimalState(); + scaffoldPhaseDir(1, 'one'); + scaffoldPhaseDir(2, 'two'); + scaffoldPhaseDir(3, 'three'); + + const result = runGsdTools('phase complete 2', tmpDir); + const output = JSON.parse(result.output); + assert.equal(output.next_phase, '4'); + }); + + test('an outstanding phase 3 (unchecked) is still selected — negative control (#4699)', () => { + writeRoadmap({ thirdBox: '[ ]' }); + writeMinimalState(); + scaffoldPhaseDir(1, 'one'); + scaffoldPhaseDir(2, 'two'); + scaffoldPhaseDir(3, 'three'); + + const result = runGsdTools('phase complete 2', tmpDir); + const output = JSON.parse(result.output); + assert.equal(output.next_phase, '03', + 'without the fix scope change: an unchecked phase 3 stays a valid candidate (disk spelling wins)'); + }); +});