Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 31 additions & 19 deletions packages/loopover-miner/lib/attempt-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;
Expand Down
27 changes: 27 additions & 0 deletions test/unit/miner-attempt-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down