diff --git a/.changeset/graceful-rams-rally.md b/.changeset/graceful-rams-rally.md new file mode 100644 index 000000000..6946cb73e --- /dev/null +++ b/.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) diff --git a/src/state.cts b/src/state.cts index 699970199..1b8ac74ce 100644 --- a/src/state.cts +++ b/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): 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 diff --git a/tests/state.test.cjs b/tests/state.test.cjs index fc085258c..ae796c657 100644 --- a/tests/state.test.cjs +++ b/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)', () => {