diff --git a/.changeset/curious-zebras-roam.md b/.changeset/curious-zebras-roam.md new file mode 100644 index 000000000..15e734d18 --- /dev/null +++ b/.changeset/curious-zebras-roam.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3852 +--- +**`phase complete` no longer advances to an inserted phase that merely has a directory** — the next-phase resolution scanned phase directories first and only consulted ROADMAP.md when the disk turned up nothing, so an inserted decimal phase (whose directory `phase insert` scaffolds immediately) outranked the phases preceding it in roadmap order. The wrong successor was reported and written to STATE.md as the resume pointer. Roadmap order now decides which phase is next; the disk still supplies the on-disk spelling when both agree, and remains the fallback when no roadmap is readable. (#3701) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 192d2a91e..afe1d3feb 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -790,6 +790,16 @@ Interactive command center for managing multiple phases from one terminal. **Phase completion is disk-strict (ADR-3180 §7.4, issue #3186).** A phase's status here — and in `roadmap analyze`, `roadmap update-plan-progress`, and `phase complete` — is decided by one rule: a passing `*-VERIFICATION.md` on disk, checked unconditionally (plan count is never a precondition, so a zero-plan phase with a passing verification reports complete). A ticked `- [x]` checkbox in `ROADMAP.md` is a human annotation only; it carries no machine authority and is never consulted for these commands' completion verdicts. `roadmap update-plan-progress` additionally withholds writing the checkbox/completion date while any plan in the phase has no matching `*-SUMMARY.md`, mirroring `phase complete`'s own coverage gate. +**Which phase comes *next* is a different question, and the roadmap answers it.** Disk-strictness +governs whether a phase is *complete*; it does not decide the successor. `phase complete` resolves +`next_phase` as the **lowest-numbered phase above the completed one that `ROADMAP.md` declares** for the +current milestone, regardless of which phase directories happen to exist. Phase *numbers* decide the +sequence — the order rows happen to appear in the file does not — a phase that has not been planned yet has no directory, and must still +be selected ahead of a later phase that does. When the roadmap and the directories agree, the +directory supplies the spelling (the zero-padded token and its on-disk slug). The directory scan is +the fallback only when no readable roadmap phase list exists (#3701; the same rule #3581 established +for `init.progress`). + **Checkpoint Heartbeats (#2410):** Background `execute-phase` runs emit `[checkpoint]` markers at every wave and plan diff --git a/src/phase.cts b/src/phase.cts index 40785cb7d..5f589df17 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -3036,6 +3036,37 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } + // #3701 — the ROADMAP decides WHICH phase is next; the disk decides only HOW it + // is spelled. Both scans select the numerically lowest phase above N. + // + // Both scans below are unchanged in what they match; what changed is that + // the roadmap is no longer gated behind "the disk found nothing". It used + // to be (`if (isLastPhase && roadmapContent !== null)`), which made a wrong + // disk answer uncorrectable: phase directories are created lazily, but + // `phase insert` scaffolds an inserted phase's directory immediately, so an + // inserted decimal is routinely the ONLY directory above N and outranked + // every phase preceding it in the roadmap. Observed: roadmap `1, 2, 02.1, + // 3` with directories for 01 and 02.1 only reported `next_phase: "02.1"` + // after completing 1 — and PERSISTED it to STATE.md — while + // `roadmap.analyze` correctly said `2`. + // + // #3581 fixed exactly this at `init.progress` and named the rule: "the + // frontier is ROADMAP ORDER, not artifact presence". This call site was not + // in that change's scope. + // + // Why the disk scan survives, rather than being replaced: + // 1. It is the only resolver when there is no ROADMAP.md, or when its + // phase rows do not parse. + // 2. When both agree, it carries the SPELLING the output has always used + // — the zero-padded directory token and the on-disk slug (`02`/`beta`), + // where the roadmap would give `2` and a slugified title. Promoting the + // roadmap without this would silently change the reported value on + // every aligned project, which is the majority case. + let diskNextNum: string | null = null; + let diskNextName: string | null = null; + let roadmapNextNum: string | null = null; + let roadmapNextName: string | null = null; + try { // #3185 (ADR-3180 Decision 1): "which phase directories belong to // the CURRENT milestone" — routed through the canonical owner @@ -3051,11 +3082,15 @@ 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; - if (comparePhaseNum(dm[1], phaseNum) > 0) { - nextPhaseNum = dm[1]; - nextPhaseName = dm[2] || null; - isLastPhase = false; - break; + // 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 + // upstream sort nothing here states. Selecting the minimum explicitly costs + // one comparison and removes the hidden coupling. + if (comparePhaseNum(dm[1], phaseNum) > 0 + && (diskNextNum === null || comparePhaseNum(dm[1], diskNextNum) < 0)) { + diskNextNum = dm[1]; + diskNextName = dm[2] || null; } } } @@ -3069,7 +3104,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { * — not a silent data-loss path. */ } - if (isLastPhase && roadmapContent !== null) { + if (roadmapContent !== null) { try { const roadmapForPhases = extractCurrentMilestone(roadmapContent, cwd); // #1591: match BOTH heading-style phases (`### Phase N:`) AND @@ -3097,15 +3132,28 @@ 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(pm[1])) continue; - if (comparePhaseNum(pm[1], phaseNum) > 0) { - nextPhaseNum = pm[1]; - nextPhaseName = pm[2] + // #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` + // headings, so "first match" is a statement about where a line sits in the + // file — not about which phase comes next. + // + // It mattered only once this scan started deciding the answer. Before, it + // ran solely when the disk scan found nothing; now it outranks the disk, so + // a roadmap listing rows out of numeric sequence (`1, 3, 2`) reported + // `next_phase: 3` and PERSISTED it, skipping Phase 2 — on an input the + // pre-#3701 code got right, because the disk scan is numerically sorted. + // Phase NUMBERS define sequence here, exactly as `comparePhaseNum` does for + // the disk scan and for #2028's lowest-outstanding override; the roadmap + // defines which phases EXIST and which milestone they belong to. + if (comparePhaseNum(pm[1], phaseNum) > 0 + && (roadmapNextNum === null || comparePhaseNum(pm[1], roadmapNextNum) < 0)) { + roadmapNextNum = pm[1]; + roadmapNextName = pm[2] .replace(/\(INSERTED\)/i, '') .trim() .toLowerCase() .replace(/\s+/g, '-'); - isLastPhase = false; - break; } } } catch { @@ -3116,6 +3164,26 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } + + // Resolve. The roadmap wins on identity; the disk wins on spelling when it + // is talking about the same phase. + if (roadmapNextNum !== null) { + // Same comparator both scans already use to order phases, so "the disk + // and the roadmap mean the same phase" cannot drift from "N is above the + // one just completed". `02` and `2` compare equal, which is the whole + // point — they are the same phase spelled two ways. + const diskAgrees = diskNextNum !== null && comparePhaseNum(diskNextNum, roadmapNextNum) === 0; + nextPhaseNum = diskAgrees ? diskNextNum : roadmapNextNum; + nextPhaseName = diskAgrees ? diskNextName : roadmapNextName; + isLastPhase = false; + } else if (diskNextNum !== null) { + // No usable roadmap (absent, unreadable, or no parseable phase rows) — + // the disk is all there is. Unchanged from the pre-#3701 behaviour. + nextPhaseNum = diskNextNum; + nextPhaseName = diskNextName; + isLastPhase = false; + } + // #2028: don't stamp "All phases complete" when a LOWER-numbered phase is // still outstanding. The two blocks above only clear isLastPhase when a // HIGHER-numbered phase exists, so completing the numerically-highest phase diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index c752597a3..0cdb131bb 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -12198,3 +12198,337 @@ describe('bug #3572 controls and clamps', () => { t.after(() => cleanup(tmpDir)); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3701 — next_phase follows ROADMAP ORDER, not artifact presence. +// +// The successor cascade resolved disk-first: the first phase DIRECTORY above N +// won, and the roadmap scan only ran when the disk found nothing. Directories +// are created lazily, but `phase insert` scaffolds an inserted phase's directory +// immediately — so an inserted decimal was routinely the only directory above N +// and outranked every phase preceding it in the roadmap. The wrong value was +// reported AND persisted to STATE.md, silently. +// +// #3581 fixed the identical defect at `init.progress` and named the rule: "the +// frontier is ROADMAP ORDER, not artifact presence". This call site was outside +// that change's scope. +// +// Most of this block is CONTROLS. The fix promotes the roadmap to decide WHICH +// phase is next while the disk still decides HOW it is spelled, so the failure +// mode of a naive fix is a silent spelling change on every aligned project — +// which is the majority case, and which `alignedTreeKeepsDiskSpelling` catches. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3701 phase complete — next_phase follows roadmap order, not disk', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3701-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + const statePath = () => path.join(tmpDir, '.planning', 'STATE.md'); + + function scaffoldPhaseDir(slug) { + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', slug), { recursive: true }); + } + + /** Roadmap rows + matching detail headings, so both scan shapes are present. */ + function writeRoadmap(rows) { + const checklist = rows.map((r) => `- [${r.done ? 'x' : ' '}] **Phase ${r.num}: ${r.name}**${r.inserted ? ' (INSERTED)' : ''} - ${r.name}`); + const details = rows.map((r) => `### Phase ${r.num}: ${r.name}\n\n**Goal:** ${r.name}`); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n## Phases\n\n${checklist.join('\n')}\n\n## Phase Details\n\n${details.join('\n\n')}\n`, + ); + } + + function writeState(currentPhase) { + fs.writeFileSync( + statePath(), + `---\ngsd_state_version: 1.0\ncurrent_phase: ${currentPhase}\nstatus: executing\n---\n\n# Project State\n`, + ); + } + + function complete(phase) { + const result = runVerifiedPhaseComplete(`phase complete ${phase}`, tmpDir); + assert.ok(result.success, `Command failed: ${result.error || result.output}`); + return JSON.parse(result.output); + } + + function frontmatterField(key) { + const m = fs.readFileSync(statePath(), 'utf-8').match(new RegExp(`^${key}:\\s*(.*)$`, 'm')); + return m ? m[1].trim().replace(/^["']|["']$/g, '') : null; + } + + // Roadmap 1, 2, 02.1 (INSERTED), 3 — the shape from the report. + const INSERTED_ROADMAP = [ + { num: '1', name: 'Alpha' }, + { num: '2', name: 'Beta' }, + { num: '02.1', name: 'Inserted Thing', inserted: true }, + { num: '3', name: 'Gamma' }, + ]; + + // ── the defect ──────────────────────────────────────────────────────────── + + test('insertedDecimalDoesNotOutrankTheRoadmapSuccessor', () => { + // Directories for 01 and 02.1 only. Phase 2 is next in the roadmap and has + // no directory — the ordinary state of an unplanned phase. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02.1-inserted-thing'); + writeRoadmap(INSERTED_ROADMAP); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.is_last_phase, false); + assert.strictEqual( + output.next_phase, + '2', + `roadmap order puts Phase 2 next; the inserted decimal has a directory and must not outrank it (got ${output.next_phase})`, + ); + assert.strictEqual(output.next_phase_name, 'beta'); + }); + + test('theWrongSuccessorIsNotPersistedToStateMd', () => { + // Independent of the reported value: the defect's real cost is the resume + // pointer written to STATE.md, which sends the next session to the wrong + // phase for the whole of the following phase. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02.1-inserted-thing'); + writeRoadmap(INSERTED_ROADMAP); + writeState(1); + + complete(1); + + assert.strictEqual( + frontmatterField('current_phase'), + '2', + 'STATE.md must advance to the roadmap successor, not to the inserted decimal', + ); + }); + + // ── controls: what must not move ────────────────────────────────────────── + + test('alignedTreeKeepsDiskSpelling', () => { + // THE control for this fix. When roadmap and disk agree, the directory still + // supplies the spelling: the zero-padded token and the on-disk slug. A fix + // that merely promoted the roadmap would report `2`/`beta` here instead of + // `02`/`beta` — a silent output change on every aligned project. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + scaffoldPhaseDir('03-gamma'); + writeRoadmap([{ num: '1', name: 'Alpha' }, { num: '2', name: 'Beta' }, { num: '3', name: 'Gamma' }]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '02', 'the on-disk zero-padded token is the established spelling'); + assert.strictEqual(output.next_phase_name, 'beta'); + }); + + test('roadmapSpellingWhenTheSuccessorHasNoDirectory', () => { + scaffoldPhaseDir('01-alpha'); + writeRoadmap([{ num: '1', name: 'Alpha' }, { num: '2', name: 'Beta' }, { num: '3', name: 'Gamma' }]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '2', 'no directory exists, so the roadmap supplies the spelling too'); + assert.strictEqual(output.next_phase_name, 'beta'); + }); + + test('aDecimalThatGenuinelyIsNextIsStillSelected', () => { + // The mirror of the defect: an over-correction that refused decimals would + // pass the two tests above and fail here. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + scaffoldPhaseDir('02.1-inserted-thing'); + writeRoadmap([ + { num: '1', name: 'Alpha', done: true }, + { num: '2', name: 'Beta' }, + { num: '02.1', name: 'Inserted Thing', inserted: true }, + { num: '3', name: 'Gamma' }, + ]); + writeState(2); + + const output = complete(2); + assert.strictEqual(output.next_phase, '02.1', 'the inserted phase really does follow 2 in roadmap order'); + assert.strictEqual(output.next_phase_name, 'inserted-thing'); + }); + + test('completingTheDecimalAdvancesToTheNextWholePhase', () => { + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + scaffoldPhaseDir('02.1-inserted-thing'); + writeRoadmap([ + { num: '1', name: 'Alpha', done: true }, + { num: '2', name: 'Beta', done: true }, + { num: '02.1', name: 'Inserted Thing', inserted: true }, + { num: '3', name: 'Gamma' }, + ]); + writeState('02.1'); + + const output = complete('02.1'); + assert.strictEqual(output.next_phase, '3'); + assert.strictEqual(output.next_phase_name, 'gamma'); + }); + + // ── the disk fallback must survive ──────────────────────────────────────── + + test('noRoadmapFallsBackToTheDiskScan', () => { + // Making the roadmap primary must not make it required. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + writeState(1); + // deliberately no ROADMAP.md + + const output = complete(1); + assert.strictEqual(output.next_phase, '02', 'with no roadmap the disk is the only resolver'); + }); + + test('unparseableRoadmapPhaseRowsFallBackToTheDiskScan', () => { + // A roadmap that exists but yields no phase rows is the same situation as no + // roadmap at all, and must degrade the same way. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n\nnothing here parses as a phase row\n'); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '02'); + }); + + test('lastPhaseStillReportsMilestoneEnd', () => { + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + writeRoadmap([{ num: '1', name: 'Alpha', done: true }, { num: '2', name: 'Beta' }]); + writeState(2); + + const output = complete(2); + assert.strictEqual(output.is_last_phase, true); + assert.strictEqual(output.next_phase, null); + }); + + // ── ordering: phase NUMBERS decide sequence, not row position ───────────── + + test('roadmapRowsOutOfNumericOrderStillResolveTheLowestSuccessor', () => { + // Review round 2 (blocker). The roadmap scan walks raw text and one global + // regex sweeps both the checklist and the detail headings, so "first match + // above N" is a statement about file position, not sequence. Once this scan + // started deciding the answer, a roadmap listing rows `1, 3, 2` reported + // next_phase 3 and PERSISTED it — skipping Phase 2 entirely, on an input the + // pre-fix code got right because the disk scan is numerically sorted. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + writeRoadmap([ + { num: '1', name: 'Alpha' }, + { num: '3', name: 'Gamma' }, + { num: '2', name: 'Beta' }, + ]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '02', `Phase 2 is the lowest above 1 regardless of row position (got ${output.next_phase})`); + assert.strictEqual(output.next_phase_name, 'beta'); + }); + + test('detailHeadingOrderDoesNotOverrideNumericOrder', () => { + // The checklist and the `## Phase Details` headings are swept by the same + // regex, so a details section ordered differently from the checklist is a + // second way row position could win. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', '', '## Phases', '', + '- [ ] **Phase 1: Alpha** - Alpha', + '- [ ] **Phase 2: Beta** - Beta', + '- [ ] **Phase 3: Gamma** - Gamma', + '', '## Phase Details', '', + '### Phase 3: Gamma', '', '**Goal:** Gamma', '', + '### Phase 1: Alpha', '', '**Goal:** Alpha', '', + '### Phase 2: Beta', '', '**Goal:** Beta', '', + ].join('\n'), + ); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '02'); + }); + + test('outOfOrderRowsWithNoDirectoryStillResolveNumerically', () => { + // Same rule on the roadmap-only path, where no disk answer exists to mask a + // document-order mistake. + scaffoldPhaseDir('01-alpha'); + writeRoadmap([ + { num: '1', name: 'Alpha' }, + { num: '3', name: 'Gamma' }, + { num: '2', name: 'Beta' }, + ]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '2'); + assert.strictEqual(output.next_phase_name, 'beta'); + }); + + test('anInsertedDecimalListedOutOfOrderDoesNotWin', () => { + // The two hazards together: rows out of sequence AND an inserted decimal + // holding the only directory above N. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02.1-inserted-thing'); + writeRoadmap([ + { num: '1', name: 'Alpha' }, + { num: '3', name: 'Gamma' }, + { num: '02.1', name: 'Inserted Thing', inserted: true }, + { num: '2', name: 'Beta' }, + ]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '2'); + }); + + // ── sentinels and the #2028 stage this change does not touch ────────────── + + test('sentinelBacklogAndDraftPhasesAreNeverSelected', () => { + // Both scans skip sentinels (#2786 / #3185 / #2949). Whichever one is + // primary, that must still hold. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('999.1-backlog-item'); + scaffoldPhaseDir('0.1-draft-item'); + writeRoadmap([ + { num: '1', name: 'Alpha' }, + { num: '2', name: 'Beta' }, + { num: '999.1', name: 'Backlog Item' }, + { num: '0.1', name: 'Draft Item' }, + ]); + writeState(1); + + const output = complete(1); + assert.strictEqual(output.next_phase, '2', 'sentinel phases are not the frontier'); + }); + + test('lowestOutstandingOverrideStillWins', () => { + // Independence: the #2028 stage-3 override answers a different question — is + // a LOWER phase still outstanding — and is untouched by this change. + scaffoldPhaseDir('01-alpha'); + scaffoldPhaseDir('02-beta'); + scaffoldPhaseDir('03-gamma'); + writeRoadmap([ + { num: '1', name: 'Alpha' }, // still unchecked — outstanding + { num: '2', name: 'Beta' }, + { num: '3', name: 'Gamma' }, + ]); + writeState(2); + + const output = complete(2); + assert.strictEqual( + output.next_phase, + '1', + 'a lower outstanding phase still overrides the positional successor (#2028)', + ); + }); +});