fix(#4067): derive advance-plan phase-complete from disk, not the plan counter (#4292)

* test(#4067): pin advance-plan phase-complete guard matrix (RED)

Five-case matrix: decline on unsummarized plans (regression), fire on
fully-summarized phase, fail-open on unresolvable phase dir, idempotent
decline, normal advance untouched.

* fix(#4067): derive advance-plan phase-complete from disk, not the plan counter

The phase-complete branch of state.advance-plan was decided purely by
STATE.md's scalar plan counter (currentPlan >= totalPlans). A stale
counter carried into a newly planned phase, or a counter raced by
wave-parallel executors, let 'Phase complete — ready for verification'
land while sibling plans were still executing.

cmdStateAdvancePlan now re-decides that branch from disk before the
write: every plan in the Current Position phase's directory must have a
SUMMARY.md (scanPhasePlans single owner, the same source
state.update-progress recalculates from). Outstanding plans decline the
entire write byte-identically (idempotent, concurrency-safe, counter
stays display-only); an unavailable disk answer fails open to the
counter-derived decision.

* fix(#4067): review round 1 — route phase-dir lookup through listMilestonePhaseDirs

#3185 drift guard: no hand-rolled phases-dir readdirSync. Windowed
(current-milestone) lookup first so an archived milestone's stale dir
cannot shadow the live one; unscoped retry when the window cannot
answer. Also restore the transform's undefined-data error semantics and
extract scanOutstanding.

* chore(#4067): add changeset fragment

* chore(#4067): backfill PR number in changeset fragment

---------

Co-authored-by: sim <sim@local>

.changeset/graceful-rams-rally.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 4292
+---
+**`state advance-plan` no longer marks a phase complete while sibling plans are still executing** — a stale or wave-raced `Plan: X of Y` counter could write `Phase complete — ready for verification` after 1 of N plans; the decision now comes from disk (every plan summarized) and the call declines with `plans_outstanding` instead. (#4067)

src/state.cts

@@ -26,6 +26,7 @@ import phaseIdMod = require('./phase-id.cjs');
 const {
   parsePhaseFromProse,
   PHASE_NUMBER_TOKEN_SOURCE,
+  matchPhaseDirs,
   phaseKeyFromToken,
   phaseKeyFromDir,
   phaseHeadingPrefixSrcFor,
@@ -62,6 +63,10 @@ function isUnparseableFrontmatter(existingFm: Record<string, unknown>): boolean
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import scanPhasePlans = require('./plan-scan.cjs');
 // eslint-disable-next-line @typescript-eslint/no-require-imports
+import coreUtilsMod = require('./core-utils.cjs');
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+import planDependencyGraphMod = require('./plan-dependency-graph.cjs');
+// eslint-disable-next-line @typescript-eslint/no-require-imports
 import verificationMod = require('./verification.cjs');
 const { isPhaseComplete } = verificationMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
@@ -890,6 +895,84 @@ function stateReplaceFieldWithFallback(content: string, primary: string, fallbac
   return content;
 }
 
+/**
+ * #4067: disk-derived plan-completion answer for advance-plan's phase-complete
+ * guard.
+ *
+ * `advancePlanCore` decides "phase complete" purely from STATE.md's scalar plan
+ * counter (`currentPlan >= totalPlans`). That counter cannot represent
+ * wave-parallel execution — a stale counter carried over from the prior phase
+ * (the reported trigger: `Plan: 7 of 7` surviving into a 10-plan phase) or a
+ * counter raced by N concurrent executors both let the phase-complete branch
+ * fire while sibling plans are mid-flight. This helper answers the completion
+ * question from disk instead, exactly the way `state update-progress`
+ * recalculates it: every plan in the Current Position phase's directory has a
+ * SUMMARY.md.
+ *
+ * Single-derivation discipline: plan/summary counting is owned by
+ * `scanPhasePlans` (src/plan-scan.cts, ADR-3180 §7.5) — this helper consumes
+ * it, never re-derives. It deliberately does NOT consult `isPhaseComplete`
+ * (§7.4): that owner answers the *verification* question (passing
+ * `*-VERIFICATION.md`), a different question from "are all plans executed?".
+ * Blocked summaries (#3345) are filtered from the pairing set with the same
+ * shared predicate `scanPhasePlans` uses, so the named outstanding list can
+ * never disagree with the count-based decision.
+ *
+ * FAIL-OPEN contract: returns `null` when the disk answer is UNAVAILABLE — no
+ * readable phases dir, no directory matching the position phase, or a scan
+ * whose scope is not COMPLETE (the scan may be blind to plans it knows exist).
+ * `null` means "the caller must fall back to the counter-derived decision",
+ * NOT "plans are outstanding"; worlds the seam cannot see (STATE.md with no
+ * Current Position `Phase:` line, milestone-archived layouts) keep today's
+ * behavior rather than being newly refused.
+ *
+ * Returns `{ dir, outstanding }` where `outstanding` is empty when every plan
+ * on disk is summarized (vacuously so for a zero-plan phase — #3168's
+ * zero-plan-phase posture).
+ */
+function scanOutstanding(phasesDir: string, dir: string): { dir: string; outstanding: string[] } | null {
+  const phaseDirPath = path.join(phasesDir, dir);
+  const scan = scanPhasePlans(phaseDirPath);
+  if (scan.scope !== SCOPE.COMPLETE) return null;
+  // Blocked summaries (#3345) are filtered with the same shared predicate
+  // scanPhasePlans uses for its own count, so the named outstanding list can
+  // never disagree with a count-based decision.
+  const countableSummaries = scan.summaryFiles.filter(
+    (f) => !planDependencyGraphMod.isSummaryFileBlocked(path.join(phaseDirPath, f)),
+  );
+  const outstanding = coreUtilsMod.findUnsummarizedPlans(scan.planFiles, countableSummaries);
+  return { dir, outstanding };
+}
+
+function unsummarizedPlansForPositionPhase(
+  cwd: string,
+  positionPhase: string,
+): { dir: string; outstanding: string[] } | null {
+  const phasesDir = planningPaths(cwd).phases;
+  // #3185 (ADR-3180 Decision 1): "which phase directories exist" is owned by
+  // listMilestonePhaseDirs — no hand-rolled readdirSync here. The owner
+  // handles an absent phasesDir as a real empty and refuses sentinels.
+  //
+  // Two passes, narrowest first: the CURRENT-MILESTONE window (so an archived
+  // milestone's stale `01-*` directory cannot shadow the live one), then —
+  // only when the window cannot answer (no bounded ROADMAP, or the position
+  // phase is simply not in it) — an unscoped read, which the owner documents
+  // as a real answer. This is a lookup of ONE phase token STATE.md names, not
+  // a milestone enumeration, so the unscoped retry is in-contract.
+  const convention = resolvePhaseIdConvention(cwd);
+  const windowed = listMilestonePhaseDirs(phasesDir, { cwd, phaseIdConvention: convention });
+  const candidateDirs = windowed.scope === SCOPE.COMPLETE ? windowed.value : [];
+  // Canonical phase-token → directory matching (phase-id owner, #2562): both
+  // sides of the comparison derived by the same function, never a local regex.
+  const { matches } = matchPhaseDirs(candidateDirs, positionPhase, convention);
+  if (matches.length > 0) return scanOutstanding(phasesDir, matches[0]);
+  const unscoped = listMilestonePhaseDirs(phasesDir);
+  if (unscoped.scope !== SCOPE.COMPLETE) return null;
+  const retry = matchPhaseDirs(unscoped.value, positionPhase, convention);
+  if (retry.matches.length === 0) return null;
+  return scanOutstanding(phasesDir, retry.matches[0]);
+}
+
 function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
   const statePath = planningPaths(cwd).state;
   if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw, undefined); return; }
@@ -915,6 +998,11 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
   // STATE.md lock, so the position read and the claim read cannot interleave
   // with another session's Current Position write.
   let milestoneConflict: milestoneLockMod.MilestoneConflict | null = null;
+  // #4067: set when the disk-derived guard declines the phase-complete branch —
+  // named here so the post-lock output path can report it without re-deriving.
+  // Holder (not a bare let) so TypeScript's closure-unaware narrowing cannot
+  // collapse the post-lock read to `never` — the callback assigns it.
+  const outstandingRef: { value: { dir: string; outstanding: string[] } | null } = { value: null };
   const wrote = readModifyWriteStateMd(statePath, (content) => {
     // advance-plan has no phase argument of its own — the phase it advances is
     // whatever ## Current Position names. Compare that against the milestone
@@ -931,11 +1019,58 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
       }
     }
     const result = transitionCore(content, intent, deps);
+    // #4067: the transform's phase-complete branch is decided by STATE.md's
+    // scalar plan counter, which can neither carry a stale value across phases
+    // nor represent wave-parallel execution. Before letting that branch write
+    // "Phase complete — ready for verification", re-decide from disk (the same
+    // source state.update-progress recalculates from): every plan in the
+    // position phase's directory must have a SUMMARY.md. A non-empty
+    // outstanding list declines the ENTIRE write — STATE.md is returned
+    // byte-identical, so the decline is idempotent and safe for any number of
+    // concurrent callers (the disk answer is re-read under the STATE.md lock
+    // each call; the counter stays display-only). `null` (disk answer
+    // unavailable) fails open to the counter-derived decision, so every
+    // world this seam cannot see keeps today's behavior.
+    if (
+      result.data?.['advanced'] === false
+      && result.data?.['reason'] === 'last_plan'
+      && positionPhase !== null
+    ) {
+      const diskAnswer = unsummarizedPlansForPositionPhase(cwd, positionPhase);
+      if (diskAnswer !== null && diskAnswer.outstanding.length > 0) {
+        outstandingRef.value = diskAnswer;
+        resultData = result.data;
+        precomputedUpdated = [];
+        return content;
+      }
+    }
     resultData = result.data;
     precomputedUpdated = result.updated;
     return result.content;
   }, cwd, { divergedFields, preWriteState });
 
+  // #4067 decline path: plans remain unexecuted on disk. Shaped like the
+  // existing `last_plan` decline (advanced:false + machine-readable reason,
+  // exit 0) rather than a hard error — the caller did nothing wrong and
+  // STATE.md needs no repair; the remaining plans' executors will re-run this
+  // command, and the final one finds a fully-summarized phase and completes it.
+  const plansOutstanding = outstandingRef.value;
+  if (plansOutstanding !== null) {
+    declineNoOp(
+      raw,
+      'advanced',
+      'plans_outstanding',
+      `state advance-plan skipped — phase-complete declined: ${plansOutstanding.outstanding.length} plan(s) in .planning/phases/${plansOutstanding.dir} have no SUMMARY.md (${plansOutstanding.outstanding.join(', ')}). STATE.md was left unchanged; re-run once every plan has executed and written its summary.`,
+      {
+        advanced: false,
+        phase_dir: plansOutstanding.dir,
+        outstanding_plans: plansOutstanding.outstanding,
+        milestone_conflict: milestoneConflict,
+      },
+    );
+    return;
+  }
+
   // `!resultData` is a type guard, not a second failure mode: the callback
   // above assigns it unconditionally and only runs once STATE.md is known to
   // exist (the missing-file case returns "STATE.md not found" earlier), and

tests/state.test.cjs

@@ -1986,6 +1986,131 @@ describe('cmdStateAdvancePlan (state advance-plan)', () => {
     const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
     assert.ok(updated.includes('Phase complete'), 'Status should contain Phase complete');
   });
+
+  // #4067: advance-plan's phase-complete decision must be derived from disk
+  // state (every plan in the phase directory has a SUMMARY.md, via the
+  // scanPhasePlans single owner) rather than from STATE.md's scalar plan
+  // counter. A serial counter cannot represent wave-parallel execution — a
+  // stale counter from the prior phase (the reported trigger) or a racing
+  // counter under N concurrent executors both let `X >= Y` fire the
+  // phase-complete branch while sibling plans are mid-flight.
+  describe('cmdStateAdvancePlan #4067 wave-parallel phase-complete guard', () => {
+    const waveFixture = [
+      '# Project State',
+      '',
+      '## Current Position',
+      '',
+      'Phase: 2 — Build out',
+      'Plan: 7 of 7',
+      'Status: Executing',
+      'Last Activity: 2026-09-01',
+      '',
+    ].join('\n');
+
+    const seedPhaseDir = (dir, planCount, summaryCount) => {
+      const phaseDir = path.join(tmpDir, '.planning', 'phases', dir);
+      fs.mkdirSync(phaseDir, { recursive: true });
+      for (let i = 1; i <= planCount; i++) {
+        fs.writeFileSync(path.join(phaseDir, `02-0${i}-PLAN.md`), `# plan ${i}\n`);
+      }
+      for (let i = 1; i <= summaryCount; i++) {
+        fs.writeFileSync(path.join(phaseDir, `02-0${i}-SUMMARY.md`), `# summary ${i}\n`);
+      }
+      return phaseDir;
+    };
+
+    test('declines phase-complete while plans lack summaries (stale counter)', () => {
+      fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), waveFixture);
+      seedPhaseDir('02-second', 3, 1);
+
+      const result = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(result.success, `Command failed: ${result.error}`);
+
+      const out = JSON.parse(result.output);
+      assert.strictEqual(out.advanced, false, 'advanced should be false');
+      assert.strictEqual(out.reason, 'plans_outstanding',
+        `reason should be plans_outstanding; got: ${JSON.stringify(out)}`);
+      assert.ok(Array.isArray(out.outstanding_plans) && out.outstanding_plans.length === 2,
+        `outstanding_plans should name the 2 unsummarized plans; got: ${JSON.stringify(out.outstanding_plans)}`);
+
+      const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+      assert.ok(!updated.includes('Phase complete'),
+        'STATE.md must NOT say Phase complete while plans are unsummarized');
+      assert.ok(updated.includes('Status: Executing'),
+        'STATE.md Status must be left unchanged by the decline');
+    });
+
+    test('fires phase-complete when every plan on disk has a summary', () => {
+      fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), waveFixture);
+      seedPhaseDir('02-second', 3, 3);
+
+      const result = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(result.success, `Command failed: ${result.error}`);
+
+      const out = JSON.parse(result.output);
+      assert.strictEqual(out.advanced, false);
+      assert.strictEqual(out.reason, 'last_plan',
+        `a fully-summarized phase must still take the phase-complete branch; got: ${JSON.stringify(out)}`);
+
+      const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+      assert.ok(updated.includes('Phase complete'), 'Status should contain Phase complete');
+    });
+
+    test('keeps counter-derived phase-complete when the phase directory cannot be determined', () => {
+      // No phase directory matching Current Position's "Phase: 2" exists —
+      // the disk answer is unavailable, so the guard fails open to the
+      // counter-derived decision (existing pinned fixtures exercise the
+      // no-Current-Position spelling; this one pins the no-matching-dir one).
+      fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), waveFixture);
+      seedPhaseDir('09-unrelated', 3, 0);
+
+      const result = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(result.success, `Command failed: ${result.error}`);
+
+      const out = JSON.parse(result.output);
+      assert.strictEqual(out.reason, 'last_plan',
+        `unresolvable phase dir must keep legacy counter behavior; got: ${JSON.stringify(out)}`);
+
+      const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+      assert.ok(updated.includes('Phase complete'), 'Status should contain Phase complete');
+    });
+
+    test('is idempotent when re-run while plans are outstanding', () => {
+      fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), waveFixture);
+      seedPhaseDir('02-second', 3, 1);
+
+      const first = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(first.success, `First call failed: ${first.error}`);
+      const afterFirst = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+
+      const second = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(second.success, `Second call failed: ${second.error}`);
+      const out = JSON.parse(second.output);
+      assert.strictEqual(out.reason, 'plans_outstanding',
+        `re-run must decline identically; got: ${JSON.stringify(out)}`);
+
+      const afterSecond = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+      assert.strictEqual(afterSecond, afterFirst,
+        'a declined advance-plan must leave STATE.md byte-identical (idempotent, race-safe)');
+    });
+
+    test('normal advance is untouched by the disk guard', () => {
+      const midPhase = waveFixture.replace('Plan: 7 of 7', 'Plan: 1 of 3');
+      fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), midPhase);
+      seedPhaseDir('02-second', 3, 0);
+
+      const result = runGsdTools('state advance-plan', tmpDir);
+      assert.ok(result.success, `Command failed: ${result.error}`);
+
+      const out = JSON.parse(result.output);
+      assert.strictEqual(out.advanced, true,
+        `counter below total must still advance (display-only counter); got: ${JSON.stringify(out)}`);
+      assert.strictEqual(out.current_plan, 2);
+
+      const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
+      assert.ok(updated.includes('Plan: 2 of 3'), 'Plan counter should advance to 2 of 3');
+    });
+  });
 });
 
 describe('cmdStateRecordMetric (state record-metric)', () => {