Summary
The per-repository worker in runMultiscan awaits checkout removal in a
finally block before it appends the attempt receipt:
} 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(...);
If rm rejects, its filesystem error replaces the outcome already captured by
the worker and execution never reaches appendReceipt. force: true ignores a
missing path, but it does not suppress EACCES, EPERM, or EBUSY.
This is still present on main at
a8fc00984b07f10f5b607a9521c2f05aa57c5107:
|
} 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( |
Environment
- Windows
- Node.js 24.18.0
- pnpm 11.9.0
- Bun 1.3.13
Reproduction
No API request is required. The reproduction creates a one-commit local Git
repository and a one-row multiscan inventory. The injected security client
denies delete access to its checkout with icacls, then throws
ORIGINAL_SCAN_FAILURE.
The complete deterministic harness is available as
repro-multiscan-cleanup.ts in the accompanying review evidence. Its relevant
injection is:
createSecurity: () => ({
async run(repository: string) {
const acl = spawnSync(
"icacls",
[repository, "/deny", `${account}:(D,DC)`],
{ encoding: "utf8", windowsHide: true },
);
if (acl.status !== 0) throw new Error(`icacls failed: ${acl.stderr}`);
throw new Error("ORIGINAL_SCAN_FAILURE");
},
async close() {},
}),
Observed result:
{
"surfaced": "Error: EACCES: permission denied, rm '<checkout>'",
"preservedOriginalFailure": false,
"receiptWritten": false
}
The harness restores the ACL and removes its temporary fixture in a finally
block. It exits successfully only when the cleanup error surfaces, the original
failure is absent, and no receipt contains that failure.
Expected behavior
Checkout cleanup should not replace the scan outcome. Every attempted
repository should receive a durable receipt before a secondary cleanup failure
can terminate or warn.
Actual behavior
The caller receives the checkout rm error, the original scan failure is lost,
and results.jsonl contains no receipt for the attempt. The same ordering also
discards a successful scan result if cleanup fails.
Impact
- The real scan failure is unavailable to the caller.
- A successful result can be changed into a campaign failure.
- The durable campaign ledger no longer records every attempt.
- Resume and operational diagnosis cannot distinguish an unattempted repository
from one whose cleanup failed after scanning.
Duplication check
Searches across open, closed, and merged issue/PR states did not find the same
multiscan behavior.
Suggested direction
Capture the attempt outcome first and treat checkout removal as best-effort
cleanup. Persist the receipt before a cleanup error can terminate the worker,
and report cleanup failure as secondary information. Regression coverage should
include both:
- scan failure plus checkout cleanup failure;
- scan success plus checkout cleanup failure.
Summary
The per-repository worker in
runMultiscanawaits checkout removal in afinallyblock before it appends the attempt receipt:If
rmrejects, its filesystem error replaces the outcome already captured bythe worker and execution never reaches
appendReceipt.force: trueignores amissing path, but it does not suppress
EACCES,EPERM, orEBUSY.This is still present on
mainata8fc00984b07f10f5b607a9521c2f05aa57c5107:codex-security/sdk/typescript/src/multiscan.ts
Lines 192 to 199 in a8fc009
Environment
Reproduction
No API request is required. The reproduction creates a one-commit local Git
repository and a one-row multiscan inventory. The injected security client
denies delete access to its checkout with
icacls, then throwsORIGINAL_SCAN_FAILURE.The complete deterministic harness is available as
repro-multiscan-cleanup.tsin the accompanying review evidence. Its relevantinjection is:
Observed result:
{ "surfaced": "Error: EACCES: permission denied, rm '<checkout>'", "preservedOriginalFailure": false, "receiptWritten": false }The harness restores the ACL and removes its temporary fixture in a
finallyblock. It exits successfully only when the cleanup error surfaces, the original
failure is absent, and no receipt contains that failure.
Expected behavior
Checkout cleanup should not replace the scan outcome. Every attempted
repository should receive a durable receipt before a secondary cleanup failure
can terminate or warn.
Actual behavior
The caller receives the checkout
rmerror, the original scan failure is lost,and
results.jsonlcontains no receipt for the attempt. The same ordering alsodiscards a successful scan result if cleanup fails.
Impact
from one whose cleanup failed after scanning.
Duplication check
api.ts; thefix now on
maindoes not changemultiscan.ts.checkout
finallyor receipt ordering.receipt caused by checkout cleanup.
Searches across open, closed, and merged issue/PR states did not find the same
multiscan behavior.
Suggested direction
Capture the attempt outcome first and treat checkout removal as best-effort
cleanup. Persist the receipt before a cleanup error can terminate the worker,
and report cleanup failure as secondary information. Regression coverage should
include both: