Merge pull request #2066 from open-gsd/fix/2028-phase-complete-milestone-end-and-workstream-guard

fix(#2028): phase.complete milestone-end out-of-order + workstream root-fallback guard

.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 <name>` or an active workstream — mirroring the existing `init progress` guard. (#2066)

src/init.cts

@@ -80,6 +80,7 @@ const {
   planningPaths,
   planningDir,
   planningRoot,
+  listAvailableWorkstreams,
   getActiveWorkstream,
   findContextMdIn,
 } = planningWorkspace;
@@ -1664,17 +1665,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(

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,
@@ -1379,6 +1380,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 <name> 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');
@@ -1701,6 +1718,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;

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 `<root>/.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,

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)
 // ─────────────────────────────────────────────────────────────────────────────