From 8990a943213b1da0dcd2684128f2cb61588dd113 Mon Sep 17 00:00:00 2001 From: Michael Freno Date: Tue, 20 Oct 2026 23:29:01 -0400 Subject: [PATCH] fix: a finished all-run is not a run to resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/pygienium-all` loaded the previous run-state and skipped every check whose entry was terminal, so a second invocation dispatched nothing at all. The one check that did run was whichever an interrupted run had left non-terminal — registry order made that `comments`, which read as "comments ran, everything after it was skipped". Resumability belongs to unfinished work: continue only when the stored run is not complete (`in_progress`, `partial`, `failed`), so completed checks are carried forward cheaply and the interrupted one picks up where it stopped. A completed run means the next invocation is a new scan, which is what the command name promises; `--fresh` and `/pygienium-resume` keep their explicit jobs. Settle a terminal check's phases on the way past too: an interrupted run can leave a phase `in_progress` behind a check that reads `complete`, which made the state claim both finished and still running. Tests: reverted src/ fails "a completed run is not resumed", "settles a phase an interrupted run left in_progress", and the integration test "an unfinished run-state is continued, not restarted"; all pass with the fix. Suite 164 pass, 1 pre-existing worktree-only failure (MISC-46). bun run typecheck clean; port-to-omp regenerates and typechecks. MISC-47 Co-Authored-By: nw-flash-flex --- src/modes/all.ts | 22 ++++++-- src/run-state.ts | 30 ++++++++++ tests/all-integration.test.ts | 27 ++++++--- tests/all.test.ts | 102 +++++++++++++++++++++++++++------- 4 files changed, 148 insertions(+), 33 deletions(-) diff --git a/src/modes/all.ts b/src/modes/all.ts index 5718d7f..b0834b9 100644 --- a/src/modes/all.ts +++ b/src/modes/all.ts @@ -43,6 +43,8 @@ import { reconcileRunStatus, resetCheckEntry, saveRunState, + settleTerminalPhases, + shouldResumeRun, shouldRunOnResume, stateDir, type RunState, @@ -282,12 +284,17 @@ export async function runAllChecks( } // --- Init / resume the single shared run-state -------------------------- + // A cleanly completed run is finished work, not a run to continue: a new + // invocation is a new scan. Without this, re-running `/pygienium-all` + // dispatched nothing but whatever an interrupted run had left non-terminal. + const stored = await loadRunState(cwd); let state = - (await loadRunState(cwd)) ?? - initRunState( - cwd, - selected.map((c) => ({ name: c.name, label: c.label, fix })), - ); + stored && shouldResumeRun(stored) + ? stored + : initRunState( + cwd, + selected.map((c) => ({ name: c.name, label: c.label, fix })), + ); // Ensure every selected check has an entry (adds any missing on resume). for (const check of selected) { @@ -363,6 +370,11 @@ export async function runAllChecks( // Resumability: skip terminal checks unless --fresh. if (entry && !shouldRunOnResume(entry, fresh)) { skipped.push(check.name); + // The entry may still carry a phase an interrupted run left + // `in_progress`; settle it so the state never reads as both finished + // and still running. + settleTerminalPhases(entry); + await saveRunState(state); footerSettle( check.name, entry.status === "complete" ? "skipped" : "skipped", diff --git a/src/run-state.ts b/src/run-state.ts index 124dd02..f518ee5 100644 --- a/src/run-state.ts +++ b/src/run-state.ts @@ -269,6 +269,36 @@ export function shouldRunOnResume(check: CheckRun, fresh: boolean): boolean { return fresh || !isCheckTerminal(check); } +/** + * Whether a stored run should be continued rather than started over. + * + * Only a cleanly completed run is finished work: asking for another run then + * means a new scan, so its completed entries must not be skipped (doing that + * made the second `/pygienium-all` dispatch nothing at all). Anything else — + * interrupted, partial, failed — is continued, which is what carries completed + * checks forward cheaply while an unfinished one picks up where it stopped. + */ +export function shouldResumeRun(state: RunState): boolean { + return state.status !== "complete"; +} + +/** + * Settle the phases of a terminal check that an interrupted run left + * `in_progress`, so the stored state never reads as finished and still running + * at once. A completed check's phases become `complete`; a skipped check's + * become `skipped`. + */ +export function settleTerminalPhases(check: CheckRun): void { + if (!isCheckTerminal(check)) return; + const settled: PhaseStatus = check.status === "complete" ? "complete" : "skipped"; + for (const phase of check.phases) { + if (phase.status === "in_progress") { + phase.status = settled; + phase.finishedAt ??= check.finishedAt ?? Date.now(); + } + } +} + /** * Reset a single check entry back to `pending` with fresh phases. Used by * `/pygienium-resume --fresh` so that previously-complete checks are diff --git a/tests/all-integration.test.ts b/tests/all-integration.test.ts index e54999d..b4d345c 100644 --- a/tests/all-integration.test.ts +++ b/tests/all-integration.test.ts @@ -22,7 +22,7 @@ import { type AgentRunner, } from "../src/agent-runner.js"; import { handleAllCommand, type PygieniumCtx } from "../src/commands.js"; -import { loadRunState } from "../src/run-state.js"; +import { loadRunState, markCheckStatus, markRunStatus, saveRunState } from "../src/run-state.js"; import { canonicalChecksRoot } from "../src/export.js"; /** Stub check whose fake-runner task writes artifacts + echoes a line. */ @@ -147,16 +147,27 @@ describe("/pygienium-all end-to-end (task 14)", () => { expect(out.join("\n")).toContain("no checks registered"); }); - it("is resumable: a second call reuses the existing run-state", async () => { + it("is resumable: an unfinished run-state is continued, not restarted", async () => { registerCheck(fakeCheck("alpha")); + // A completed run is finished work: the next call starts a new run-state. await captureStdout(() => handleAllCommand("--fix", stubCtx(cwd))); - const first = await loadRunState(cwd); - const firstStarted = first!.startedAt; + const completed = await loadRunState(cwd); + expect(completed!.status).toBe("complete"); + await captureStdout(() => handleAllCommand("--fix", stubCtx(cwd))); + const fresh = await loadRunState(cwd); + expect(fresh!.startedAt).toBeGreaterThan(completed!.startedAt); + + // An unfinished one is continued: same run, same startedAt, and the + // completed check is not dispatched again. + const interrupted = await loadRunState(cwd); + markRunStatus(interrupted!, "in_progress"); + markCheckStatus(interrupted!, "alpha", "in_progress"); + await saveRunState(interrupted!); + const before = (await loadRunState(cwd))!.startedAt; - // Second run reloads the existing run-state (same startedAt). await captureStdout(() => handleAllCommand("--fix", stubCtx(cwd))); - const second = await loadRunState(cwd); - expect(second!.startedAt).toBe(firstStarted); - expect(second!.status).toBe("complete"); + const resumed = await loadRunState(cwd); + expect(resumed!.startedAt).toBe(before); + expect(resumed!.status).toBe("complete"); }); }); diff --git a/tests/all.test.ts b/tests/all.test.ts index 8965fdb..5f8052d 100644 --- a/tests/all.test.ts +++ b/tests/all.test.ts @@ -39,6 +39,9 @@ import { loadRunState, markCheckStatus, applyPhaseStatus, + initRunState, + markRunStatus, + saveRunState, PHASE_RECON, PHASE_ANALYSIS, PHASE_FIX, @@ -229,41 +232,100 @@ describe("/pygienium-all orchestrator (task 12)", () => { expect(summary).not.toContain("## beta"); }); - it("skips terminal checks on re-run; --fresh re-runs them", async () => { + it("a completed run is not resumed: the next invocation runs every check again", async () => { registerCheck(fakeCheck("alpha")); registerCheck(fakeCheck("beta")); // First run: both complete. await captureStdout(() => handleAllCommand("--fix", stubCtx(cwd))); - expect(track.dispatched.length).toBe(4); // 2 checks × 2 phases - const firstAlphaFix = await readFile( - join(cwd, ".pygienium", "checks", "alpha", "changes.md"), - "utf8", - ); + expect(track.dispatched).toEqual(["alpha", "alpha", "beta", "beta"]); - // Second run without --fresh: both already terminal → skipped. + // Second run, no flags: the stored run completed, so this is a new scan. + // Every check dispatches again — the regression this pins dispatched + // nothing but whichever check an interrupted run happened to leave open. track.dispatched.length = 0; const out = await captureStdout(() => handleAllCommand("--fix", stubCtx(cwd)), ); - expect(track.dispatched).toHaveLength(0); - expect(out.join("\n")).toContain("skipping"); - const state2 = await loadRunState(cwd); - expect(state2?.status).toBe("complete"); + expect(track.dispatched).toEqual(["alpha", "alpha", "beta", "beta"]); + expect(out.join("\n")).not.toContain("skipping"); + const state = await loadRunState(cwd); + expect(state?.status).toBe("complete"); + expect(state?.checks.alpha?.status).toBe("complete"); + expect(state?.checks.beta?.status).toBe("complete"); - // Third run with --fresh: both re-dispatched from scratch. + // --fresh remains the explicit reset and dispatches the same way. track.dispatched.length = 0; await captureStdout(() => handleAllCommand("--fix --fresh", stubCtx(cwd))); expect(track.dispatched).toEqual(["alpha", "alpha", "beta", "beta"]); - const state3 = await loadRunState(cwd); - expect(state3?.status).toBe("complete"); - // The fresh re-run overwrote alpha's changes.md (still valid content). - const alphaFix2 = await readFile( - join(cwd, ".pygienium", "checks", "alpha", "changes.md"), - "utf8", + }); + + it("resumes an unfinished run: terminal checks skip, the rest re-dispatch", async () => { + registerCheck(fakeCheck("alpha")); + registerCheck(fakeCheck("beta")); + + // A run that stopped with alpha done and beta failed is unfinished work, + // so it continues instead of starting over. + const state = initRunState(cwd, [ + { name: "alpha", label: "alpha", fix: true }, + { name: "beta", label: "beta", fix: true }, + ]); + state.recon = { + complete: true, + path: join(cwd, ".pygienium", "recon.json"), + finishedAt: Date.now(), + }; + markComplete(state, "alpha"); + markCheckStatus(state, "beta", "failed", "provider reset mid-scan"); + markRunStatus(state, "partial"); + await saveRunState(state); + + const out = await captureStdout(() => + handleAllCommand("--fix", stubCtx(cwd)), ); - expect(alphaFix2).toContain("alpha changes"); - void firstAlphaFix; + expect(track.dispatched).toEqual(["beta", "beta"]); + expect(out.join("\n")).toContain("skipping"); + }); + + it("settles a phase an interrupted run left in_progress on a skipped check", async () => { + registerCheck(fakeCheck("alpha")); + registerCheck(fakeCheck("beta")); + + // alpha finished, but a later interrupted run left its analysis phase open. + const state = initRunState(cwd, [ + { name: "alpha", label: "alpha", fix: false }, + { name: "beta", label: "beta", fix: false }, + ]); + state.recon = { + complete: true, + path: join(cwd, ".pygienium", "recon.json"), + finishedAt: Date.now(), + }; + markComplete(state, "alpha"); + // Directly, because applyPhaseStatus would also move the check itself back + // to in_progress — the corruption here is a phase left open behind a + // check that still reads complete. + const staleAnalysis = state.checks.alpha?.phases.find( + (p) => p.id === PHASE_ANALYSIS, + ); + if (!staleAnalysis) throw new Error("alpha has no analysis phase"); + staleAnalysis.status = "in_progress"; + applyPhaseStatus(state, "beta", PHASE_ANALYSIS, "in_progress"); + await saveRunState(state); + + await captureStdout(() => handleAllCommand("", stubCtx(cwd))); + + const after = await loadRunState(cwd); + // alpha was skipped as already complete: its stale phase settles to + // complete rather than leaving the check finished-but-running. + const alphaPhases = after?.checks.alpha?.phases ?? []; + expect(alphaPhases.every((p) => p.status !== "in_progress")).toBe(true); + expect( + alphaPhases.find((p) => p.id === PHASE_ANALYSIS)?.status, + ).toBe("complete"); + // beta never finished, so the run resumes it and its scan runs again + // (scan-only: this run passes no --fix). + expect(track.dispatched).toEqual(["beta"]); }); it("resumes an interrupted run (re-dispatches non-terminal checks only)", async () => {