fix(multiscan): keep scan outcomes when checkout cleanup fails

[rohanpoudel2]

Fixes #211

Problem

The per-repository worker in runCampaign awaited checkout removal in a finally that ran before the attempt receipt was appended:

} catch (error) {
  if (options.signal?.aborted === true) options.signal.throwIfAborted();
  failure = redactedErrorMessage(error);
} finally {
  await rm(checkout, { recursive: true, force: true });
}
const status = failure === undefined ? "completed" : "failed";
await appendReceipt(...);

force: true ignores a checkout that is already gone, but it does not suppress EACCES, EPERM or EBUSY — a still-open handle on Windows, or a parent directory that is no longer writable. When the removal rejected, its filesystem error replaced the outcome the worker had just captured, and execution never reached appendReceipt. Three things went wrong at once: the real scan failure was lost, the campaign surfaced a confusing removal error in its place, and the attempt was never recorded, so resume had no receipt for it.

Change

Capture the removal failure instead of throwing it. The attempt keeps the status its scan earned, always gets a receipt, and reports the removal failure alongside any scan failure, so a leftover checkout is still surfaced rather than silently dropped.

A cleanup failure on a scan that otherwise succeeded therefore produces a completed receipt carrying an error field. That combination is already representable — error is optional on MultiscanReceipt and independent of status — and it is the honest record: the scan did complete, so resume correctly counts it as done rather than re-running billed model work, while the operator still learns that a checkout was left behind. The CLI's existing progress renderer prints it without any change:

codex-security: stubborn completed (attempt 1): Multiscan checkout cleanup failed: EACCES: permission denied, rm '...'

A leftover checkout cannot be mistaken for a fresh one later: every attempt removes the checkout again at the top of the try, so a leftover that outlives this run fails there instead.

Cancellation is unaffected — the signal.throwIfAborted() check stays in the catch, ahead of cleanup.

Verification

Two regression tests in tests-ts/multiscan.test.ts inject a failing removal through mock.module("node:fs/promises", …), the same seam tests-ts/api.test.ts already uses to test cleanup failures. The injection only rejects for the checkout path and only once the scan is over, so the removal at the top of the try still runs normally. That keeps the tests deterministic and cross-platform, rather than depending on icacls or on POSIX permission semantics.

Against the unfixed worker, the injected EACCES escapes runMultiscan entirely, exactly as reported:

EACCES: permission denied, rm '.../results/checkouts/stubborn'
      at runCampaign (src/multiscan.ts:339:29)
      at runMultiscan (src/multiscan.ts:189:15)
(fail) multiscan > keeps a failed scan's outcome when its checkout cannot be removed
(fail) multiscan > keeps a completed scan's outcome when its checkout cannot be removed
 0 pass
 2 fail

The tests assert both halves of the defect: that ORIGINAL_SCAN_FAILURE survives, and that the receipt is written at all. With the fix:

 2 pass
 0 fail

Full suite: 719 pass / 5 skip / 0 fail. pnpm run types and pnpm run format are clean.

Same-class audit

The other finally blocks in multiscan.ts — the supervisor lock release and the client close() path — do not have this shape: they run where a throw is the intended outcome, or already guard their own failures. This was the only cleanup that could silently overwrite a captured result.