From 51dfa683d402c88bbc5b94c2b5293d222848cf1c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Jul 2026 13:45:00 -0400 Subject: [PATCH] fix(#2028): phase.complete milestone-end out-of-order + workstream root-fallback guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two code-confirmed defects in `gsd-tools phase complete` (re-verified against next; the three severe corruption paths the issue filed are superseded by the ADR-1769 Transition Module migration + #2012, so this is the confirmed remainder). 1. Milestone-end mislabel (isLastPhase). The milestone-end determination only cleared isLastPhase when a HIGHER-numbered phase existed, so completing the numerically-highest phase out of order (e.g. Phase 10 before Phase 9) stamped STATE.md `Status: Milestone complete` while a lower phase was still outstanding. Added a lower-phase check: after the existing higher-phase scans, if any earlier phase in the current milestone has an unchecked roadmap checkbox (`[ ]`), isLastPhase becomes false AND next_phase/next_phase_name point at the LOWEST outstanding lower phase — so STATE.md advances to the real gap instead of parking on the just-completed phase. A completed phase always has `[x]` (phase.complete sets it), so all-lower-complete still reports milestone-end; heading-only roadmaps (no checkboxes) retain prior behavior. The checkbox regex mirrors the sibling phasePattern's anchoring (whitespace/bold + required `:`) so unrelated checklist lines mentioning "Phase N" don't match. 2. Workstream root-fallback (no guard). cmdPhaseComplete resolves every path via planningDir(cwd); with a `workstreams/` dir present but no active workstream and no --ws, that returns root `.planning`, so phase.complete wrote STATE.md/ ROADMAP.md (and the mislabel) into the shared root other workstreams read. Added the same #1912 fail-safe guard init.progress got: refuse (asking for `--ws`/active workstream) instead of silently writing root. Resolution itself was already wired globally (resolveActiveWorkstream: --ws > GSD_WORKSTREAM > pointer, set in bin/gsd-tools.cjs), so only the refusal guard was missing. The workstream-mode detection (`listAvailableWorkstreams`) is extracted into planning-workspace.cts as the single source of truth and consumed by BOTH init.progress and phase.complete, so the two fail-safe paths cannot drift. Tests (tests/phase.test.cjs, new #2028 describe): out-of-order completion becomes `Ready to plan` with is_last_phase=false, next_phase pointing at the outstanding phase and Current Phase advancing to it (not the completed phase); all-lower- complete still reports milestone-end; workstream-mode-no-active refuses with an `--ws` hint; `--ws` completes in the workstream leaving root untouched; flat mode unaffected. Fail-first verified locally via direct gsd-tools invocation. Co-Authored-By: Claude Opus 4.8 --- ...complete-milestone-end-workstream-guard.md | 5 + src/init.cts | 13 +- src/phase.cts | 61 +++++- src/planning-workspace.cts | 17 ++ tests/phase.test.cjs | 193 ++++++++++++++++++ 5 files changed, 277 insertions(+), 12 deletions(-) create mode 100644 .changeset/2028-phase-complete-milestone-end-workstream-guard.md diff --git a/.changeset/2028-phase-complete-milestone-end-workstream-guard.md b/.changeset/2028-phase-complete-milestone-end-workstream-guard.md new file mode 100644 index 000000000..3f1f184e2 --- /dev/null +++ b/.changeset/2028-phase-complete-milestone-end-workstream-guard.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2066 +--- +**`phase complete` no longer marks a milestone done out of order, nor silently writes root state in workstream mode.** Completing the numerically-highest phase while an earlier phase was still outstanding wrongly flipped STATE.md to `Status: Milestone complete` (the milestone-end check only looked for higher-numbered phases, so an out-of-order completion — e.g. Phase 10 before Phase 9 — read as the end). It now reports milestone-end only when every lower-numbered phase in the milestone is checked complete. Separately, in workstream mode with no active workstream, `phase complete` previously fell back to root `.planning` and wrote STATE.md/ROADMAP.md (and the mislabel) into the shared root other workstreams read; it now fails safe — asking for `--ws ` or an active workstream — mirroring the existing `init progress` guard. (#2066) diff --git a/src/init.cts b/src/init.cts index 19d8e6f1c..8ac092f2c 100644 --- a/src/init.cts +++ b/src/init.cts @@ -80,6 +80,7 @@ const { planningPaths, planningDir, planningRoot, + listAvailableWorkstreams, getActiveWorkstream, findContextMdIn, } = planningWorkspace; @@ -1661,17 +1662,7 @@ function cmdInitProgress(cwd: string, raw: boolean): void { // reporting a stale root milestone. Require an explicit workstream instead. // Mirror planningDir's resolution (GSD_WORKSTREAM env > stored active pointer) so // an explicit --ws (which sets GSD_WORKSTREAM) satisfies the check. - const _wsRoot = path.join(planningRoot(cwd), 'workstreams'); - let _availableWorkstreams: string[] = []; - try { - _availableWorkstreams = fs - .readdirSync(_wsRoot, { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name) - .sort(); - } catch { - /* no workstreams dir → flat mode */ - } + const _availableWorkstreams = listAvailableWorkstreams(cwd); const _resolvedWorkstream = process.env['GSD_WORKSTREAM'] || getActiveWorkstream(cwd); if (_availableWorkstreams.length > 0 && !_resolvedWorkstream) { error( diff --git a/src/phase.cts b/src/phase.cts index f2ca162e4..526f64df3 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -61,7 +61,8 @@ const { evaluateUatPassed } = uatPredicate; import verificationMod = require('./verification.cjs'); const { readVerificationStatus } = verificationMod; -const { planningDir, withPlanningLock } = planningWorkspace; +const { planningDir, withPlanningLock, listAvailableWorkstreams, getActiveWorkstream } = + planningWorkspace; const { extractFrontmatter } = frontmatterMod; const { readModifyWriteStateMd, @@ -1376,6 +1377,22 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { error('phase number required for phase complete'); } + // #2028: fail safe in workstream mode with no active workstream. With no active + // workstream and no --ws, planningDir(cwd) resolves to root .planning, so + // phase.complete would write STATE.md/ROADMAP.md (and mislabel milestone status) + // into the shared root that other workstreams read. Mirror the #1912 guard that + // init.progress got (resolution: GSD_WORKSTREAM env > stored active pointer; an + // explicit --ws sets GSD_WORKSTREAM upstream and satisfies the check). + const availableWorkstreams = listAvailableWorkstreams(cwd); + const resolvedWorkstream = process.env['GSD_WORKSTREAM'] || getActiveWorkstream(cwd); + if (availableWorkstreams.length > 0 && !resolvedWorkstream) { + error( + `phase.complete requires a workstream in workstream mode — no active workstream is set, so root STATE.md/ROADMAP.md (likely stale) would be written. ` + + `Pass --ws or run ${formatGsdSlash('workstream set', resolveRuntime(cwd)) as string} first. ` + + `Available workstreams: ${availableWorkstreams.join(', ')}`, + ); + } + const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); const statePath = path.join(planningDir(cwd), 'STATE.md'); const phasesDir = path.join(planningDir(cwd), 'phases'); @@ -1698,6 +1715,48 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } + // #2028: don't stamp "Milestone 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 + // out of order (e.g. Phase 10 before Phase 9) wrongly read as milestone-end. + // A phase is complete iff its roadmap checkbox is `[x]` (phase.complete sets + // this on completion — including the one just marked above); any earlier + // phase in this milestone whose checkbox is still `[ ]` means the milestone + // is not done, and the LOWEST such phase is the real next actionable item — + // point next_phase at it so STATE.md advances to the gap rather than parking + // on the just-completed phase. Roadmaps without phase checkboxes (heading- + // only) retain the prior behavior — there is nothing to scan. The checkbox + // pattern mirrors the sibling phasePattern's anchoring (only whitespace/bold + // between the box and "Phase", a required `:`) so unrelated checklist lines + // that merely mention "Phase N" don't match. + if (isLastPhase && roadmapContent !== null) { + try { + const milestoneScope = extractCurrentMilestone(roadmapContent, cwd); + const cbPattern = + /-\s*\[(x| )\]\s*(?:\*\*|__)?\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)(?:\s*\([^)\n]*\))?\s*:\s*([^\n*]+)/gi; + let cbm: RegExpExecArray | null; + let lowestOutstanding: { num: string; name: string } | null = null; + while ((cbm = cbPattern.exec(milestoneScope)) !== null) { + const isChecked = cbm[1].toLowerCase() === 'x'; + if (!isChecked && comparePhaseNum(cbm[2], phaseNum) < 0) { + if (lowestOutstanding === null || comparePhaseNum(cbm[2], lowestOutstanding.num) < 0) { + lowestOutstanding = { + num: cbm[2], + name: cbm[3].replace(/\(INSERTED\)/i, '').trim().toLowerCase().replace(/\s+/g, '-'), + }; + } + } + } + if (lowestOutstanding !== null) { + isLastPhase = false; + nextPhaseNum = lowestOutstanding.num; + nextPhaseName = lowestOutstanding.name; + } + } catch { + /* intentionally empty */ + } + } + if (fs.existsSync(statePath)) { const originalStateContent = platformReadSync(statePath) || ''; let stateContent = originalStateContent; diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index d3017d9b3..f94a95f63 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -142,6 +142,22 @@ function planningRoot(cwd: string): string { return path.join(cwd, '.planning'); } +// Sorted list of workstream directory names under `/.planning/workstreams`, +// or `[]` when the project is flat (no workstreams dir). Single source of truth +// for the "workstream mode" detection shared by the #1912/#2028 fail-safe guards +// (init.progress, phase.complete) so the two paths cannot drift. +function listAvailableWorkstreams(cwd: string): string[] { + try { + return fs + .readdirSync(path.join(planningRoot(cwd), 'workstreams'), { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name) + .sort(); + } catch { + return []; + } +} + interface PlanningPaths { planning: string; state: string; @@ -389,6 +405,7 @@ export = { createMemoryPointerAdapter, planningDir, planningRoot, + listAvailableWorkstreams, planningPaths, withPlanningLock, getActiveWorkstream, diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 6681c40cd..1a542c914 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -3749,6 +3749,199 @@ describe('phase complete milestone-scoped next-phase', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #2028 — phase.complete milestone-end inference + workstream root-fallback guard +// ───────────────────────────────────────────────────────────────────────────── + +describe('#2028 — phase complete milestone-end + workstream guard', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // A complement phase numbered AFTER Phase 9 but executed first. Completing the + // numerically-highest phase must not read as milestone-end while a lower phase + // is still outstanding (the isLastPhase blocks only checked for HIGHER phases). + test('does NOT stamp "Milestone complete" when a lower-numbered phase is still outstanding', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [ ] Phase 9: Introspection\n- [ ] Phase 10: Complement\n\n### Phase 9: Introspection\n**Goal:** baseline\n\n### Phase 10: Complement\n**Goal:** complement\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 10\n**Status:** In progress\n**Current Plan:** 10-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n` + ); + const p10 = path.join(tmpDir, '.planning', 'phases', '10-complement'); + fs.mkdirSync(p10, { recursive: true }); + fs.writeFileSync(path.join(p10, '10-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p10, '10-01-SUMMARY.md'), '# Summary'); + + const result = runVerifiedPhaseComplete('phase complete 10', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.is_last_phase, + false, + 'Phase 10 is numerically highest but Phase 9 is outstanding → not milestone-end', + ); + // The outstanding lower phase IS the real next actionable item — STATE.md must + // advance to it (the gap), not park on the just-completed Phase 10. + assert.strictEqual(String(Number(output.next_phase)), '9', 'next_phase should point at the outstanding Phase 9'); + + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + !/Milestone complete/i.test(state), + 'STATE.md must NOT flip to "Milestone complete" while a lower phase is outstanding', + ); + assert.ok(/Ready to plan/i.test(state), 'status should be "Ready to plan"'); + assert.match( + state, + /\*\*Current Phase:\*\*\s*0*9\b/, + 'Current Phase must advance to the outstanding Phase 9, not stay on the completed Phase 10', + ); + assert.doesNotMatch( + state, + /\*\*Current Phase:\*\*\s*10\b/, + 'Current Phase must NOT remain on the just-completed Phase 10', + ); + }); + + // Guard against over-correction: when every earlier phase is [x], completing + // the numerically-highest phase IS still the milestone end. + test('still detects milestone-end when all lower phases are checked complete', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [x] Phase 9: Introspection\n- [ ] Phase 10: Complement\n\n### Phase 9: Introspection\n**Goal:** baseline\n\n### Phase 10: Complement\n**Goal:** complement\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 10\n**Status:** In progress\n**Current Plan:** 10-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n` + ); + const p10 = path.join(tmpDir, '.planning', 'phases', '10-complement'); + fs.mkdirSync(p10, { recursive: true }); + fs.writeFileSync(path.join(p10, '10-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p10, '10-01-SUMMARY.md'), '# Summary'); + + const result = runVerifiedPhaseComplete('phase complete 10', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.is_last_phase, true, 'all lower phases complete → Phase 10 is milestone-end'); + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(/Milestone complete/i.test(state), 'status should be "Milestone complete"'); + }); + + // The lower-phase scan must not treat an unrelated checklist line that merely + // mentions "Phase N" (no `:` after the number) as an outstanding phase — the + // checkbox regex is anchored like the sibling phase scan. + test('does not treat an unrelated checklist line mentioning a phase number as outstanding', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [ ] Add regression coverage for Phase 3 rollback\n- [ ] Phase 5: Final\n\n### Phase 5: Final\n**Goal:** end\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 05\n**Status:** In progress\n**Current Plan:** 05-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n` + ); + const p5 = path.join(tmpDir, '.planning', 'phases', '05-final'); + fs.mkdirSync(p5, { recursive: true }); + fs.writeFileSync(path.join(p5, '05-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p5, '05-01-SUMMARY.md'), '# Summary'); + + const result = runVerifiedPhaseComplete('phase complete 5', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual( + output.is_last_phase, + true, + 'the "Phase 3 rollback" prose line must NOT be read as an outstanding Phase 3', + ); + }); + + // #1912 parity: in workstream mode with no active workstream, planningDir(cwd) + // resolves to root .planning — writing STATE.md/ROADMAP.md into the shared root + // that other workstreams read. Refuse instead of silently writing root. + test('refuses to write root in workstream mode when no workstream is resolved', () => { + fs.mkdirSync(path.join(tmpDir, '.planning', 'workstreams', 'alpha'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'workstreams', 'beta'), { recursive: true }); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + '# State\n\n**Current Phase:** 01\n**Status:** In progress\n', + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n- [ ] Phase 1: A\n\n### Phase 1: A\n**Goal:** x\n', + ); + + const result = runGsdTools('phase complete 1', tmpDir); + assert.equal(result.success, false, 'should refuse rather than silently writing root STATE/ROADMAP'); + assert.match(result.error || '', /workstream|--ws/i, 'error should name the workstream requirement'); + }); + + // An explicit --ws satisfies the guard (it sets GSD_WORKSTREAM upstream) AND + // targets that workstream — the write must land in the workstream's own + // STATE.md/ROADMAP.md, leaving root untouched. + test('--ws satisfies the guard and writes the workstream, not root', () => { + fs.mkdirSync(path.join(tmpDir, '.planning', 'workstreams', 'beta'), { recursive: true }); + const wsDir = path.join(tmpDir, '.planning', 'workstreams', 'alpha'); + fs.mkdirSync(path.join(wsDir, 'phases', '01-only'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), '# Roadmap\n\n### Phase 1: Only\n**Goal:** x\n'); + fs.writeFileSync( + path.join(wsDir, 'STATE.md'), + '# State\n\n**Current Phase:** 01\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** W\n', + ); + fs.writeFileSync(path.join(wsDir, 'phases', '01-only', '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(wsDir, 'phases', '01-only', '01-01-SUMMARY.md'), '# Summary'); + fs.writeFileSync( + path.join(wsDir, 'phases', '01-only', '01-VERIFICATION.md'), + '---\nstatus: passed\n---\n# Verification\n', + ); + + // A distinct root STATE.md that must be left byte-for-byte untouched. + const rootState = '# ROOT State\n\n**Current Phase:** 99\n**Status:** Root sentinel\n'; + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), rootState); + + const result = runGsdTools('phase complete 1 --ws alpha', tmpDir); + assert.ok(result.success, `--ws alpha should complete in the workstream: ${result.error}`); + + // Root STATE.md must be untouched — the write landed in the workstream. + assert.strictEqual( + fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'), + rootState, + 'root STATE.md must NOT be written when --ws targets a workstream', + ); + // The workstream's own STATE.md advanced (single phase → milestone complete). + const wsState = fs.readFileSync(path.join(wsDir, 'STATE.md'), 'utf-8'); + assert.match(wsState, /Milestone complete/i, "the workstream's STATE.md should be the one updated"); + }); + + // The guard only fires in workstream mode — a flat project (no workstreams dir) + // completes normally. + test('flat mode (no workstreams dir) still completes normally', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n### Phase 1: Only\n**Goal:** x\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 01\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n` + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-only'); + 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'); + + const result = runVerifiedPhaseComplete('phase complete 1', tmpDir); + assert.ok(result.success, `flat mode should still complete: ${result.error}`); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // exact token matching (no prefix collisions) // ─────────────────────────────────────────────────────────────────────────────