diff --git a/packages/loopover-miner/lib/attempt-cli.js b/packages/loopover-miner/lib/attempt-cli.js index 248d09e37a..1e8096d9f6 100644 --- a/packages/loopover-miner/lib/attempt-cli.js +++ b/packages/loopover-miner/lib/attempt-cli.js @@ -513,23 +513,33 @@ export async function runAttempt(args, options = {}) { claimedIssue = true; const runAttemptPipeline = options.runMinerAttempt ?? runMinerAttempt; - const result = await runAttemptPipeline( - { - loopInput, - issueNumber: parsed.issueNumber, - minerLogin: parsed.minerLogin, - base: parsed.base, - killSwitchScope, - slopThreshold: amsPolicy.spec.slopThreshold, - submissionMode: amsPolicy.spec.submissionMode, - governor, - }, - { - ...deps, - shouldAbort, - resolveKillSwitchScope: () => resolveLiveKillSwitch().scope, - }, - ); + let result; + try { + result = await runAttemptPipeline( + { + loopInput, + issueNumber: parsed.issueNumber, + minerLogin: parsed.minerLogin, + base: parsed.base, + killSwitchScope, + slopThreshold: amsPolicy.spec.slopThreshold, + submissionMode: amsPolicy.spec.submissionMode, + governor, + }, + { + ...deps, + shouldAbort, + resolveKillSwitchScope: () => resolveLiveKillSwitch().scope, + }, + ); + } catch (error) { + // A real attempt that CRASHED is exactly the case that most needs its worktree kept for post-mortem + // inspection, so record the failure explicitly before unwinding. Without this, `attemptOk` stayed + // `undefined` and the finally block's `?? true` default (meant for the earlier blocked paths that never + // ran anything in the worktree) deleted it -- inverting shouldRetainWorktree's documented policy. + worktreeResult.attemptOk = false; + throw error; + } worktreeResult.attemptOk = result.outcome === "submitted"; @@ -670,8 +680,10 @@ export async function runAttempt(args, options = {}) { return reportCliFailure(parsed.json, describeCliError(error)); } finally { // worktreeResult.attemptOk is set to the REAL runMinerAttempt outcome (submitted = true) once that call - // happens; every earlier blocked path (rejection/worktree-prep-failure/infeasible) never sets it, since - // nothing ran in the worktree to postmortem -- those default to `true` (nothing to retain), matching + // happens, and explicitly to `false` when that call THROWS -- a crashed attempt is precisely what needs a + // retained worktree to postmortem, so it must never fall through to the `?? true` default below. Every + // earlier blocked path (rejection/worktree-prep-failure/infeasible) never sets it, since nothing ran in + // the worktree to postmortem -- those are the cases that default to `true` (nothing to retain), matching // cleanupAttemptWorktree's own retention policy (a failed REAL attempt is what gets retained). if (worktreeResult?.ok) { const cleanupWorktree = options.cleanupAttemptWorktree ?? cleanupAttemptWorktree; diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index 10a023863d..31ef886378 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -1541,6 +1541,33 @@ describe("runAttempt: real claim-ledger wiring (#5393)", () => { expect(releaseClaimSpy).toHaveBeenCalledWith("acme/widgets", 7); }); + it("REGRESSION: retains the worktree when runMinerAttempt throws — a crashed attempt is what needs post-mortem (#6759)", async () => { + const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const cleanupAttemptWorktreeSpy = vi.fn().mockResolvedValue({ ok: true, removed: false }); + + const exitCode = await runAttempt(["acme/widgets", "7", "--miner-login", "alice"], { + env: { MINER_CODING_AGENT_PROVIDER: "noop" }, + openWorktreeAllocator: () => allocator, + openClaimLedger: () => claimLedger, + initEventLedger: () => eventLedger, + initAttemptLog: () => attemptLog, + initGovernorLedger: () => governorLedger, + ...readyPipelineOptions({ + cleanupAttemptWorktree: cleanupAttemptWorktreeSpy, + runMinerAttempt: async () => { + throw new Error("boom"); + }, + }), + }); + + expect(exitCode).toBe(2); + // attemptOk MUST be false: shouldRetainWorktree(attemptOk) === !attemptOk, so a crashed attempt's + // worktree is retained for inspection. Before the fix, the throw skipped the `attemptOk` assignment + // entirely and the finally block's `?? true` default deleted exactly the worktree worth keeping. + expect(cleanupAttemptWorktreeSpy).toHaveBeenCalledWith(expect.any(String), expect.any(String), false); + }); + it("never claims when the attempt is blocked before feasibility is even checked (rejection-signaled)", async () => { const { allocator, claimLedger, eventLedger, attemptLog, governorLedger } = tempLedgers(); vi.spyOn(console, "error").mockImplementation(() => undefined);