fix: keep scan cleanup failures from replacing the scan result

[Haiduongcable]

Summary

Fixes #38.

The scan finally block awaited three cleanup steps without guarding any of them:

} finally {
  try {
    await Promise.all([
      knowledgeBase?.cleanup(),
      removeTargetPathsFile(targetPathsFile),
    ]);
  } finally {
    await releaseCredentialHome?.();
  }
}

A rejection in a finally block replaces the pending exception, so a cleanup failure discards
the outcome the try and catch blocks already produced. The caller is handed an unrelated
filesystem error instead of the reason the scan failed.

These are not hypothetical rejections. removeTargetPathsFile rethrows on any non-Windows
platform, and knowledgeBase.cleanup() is rm(path, { recursive: true, force: true }), where
force suppresses only ENOENT — EACCES, EBUSY, and ENOTEMPTY still reject.

The failure path also loses information that was recorded correctly: the catch block writes the
real message to the workbench through fail-scan before the finally block runs, so the
persisted scan history and the error the caller receives disagree about why the scan failed.
ScanCostLimitExceededError and ScanInterruptedError are erased the same way, so a scan
stopped by its own cost ceiling can report a rm failure instead.

Impact

The two kinds of cleanup are treated differently on purpose, because only one of them is
best effort.

Scan outcome	Failing step	Before	After
failed	temporary inputs	cleanup error; real error discarded	real scan error, cleanup warning
failed	credential lock release	cleanup error; real error discarded	real scan error, cleanup warning
completed	temporary inputs	rejects; completed scan discarded	result returned, cleanup warning
completed	credential lock release	rejects	rejects (unchanged, deliberate)

Changes

• Removing the temporary scan inputs is best effort. Those failures are settled and reported
  through the existing onWarning observer instead of deciding the scan result.
• Releasing the credential home lock is not best effort, and is deliberately not reduced to a
  warning on an otherwise successful scan. The release closure only marks itself done once the
  lock directory is gone, so a failed release leaves an owner.json naming a live process;
  recoverStaleCredentialHomeLock then declines to reclaim it because that pid is alive, and
  later scans in the same process wait on a lock nothing frees. Reporting success while leaving
  the client in that state would be worse than failing, so the failure still propagates, and is
  downgraded to a warning only when the scan had already failed and its error is the one worth
  keeping.
• The failed-scan marker is recorded on entry to the catch block. this.#requireOpen() and
  throwIfAborted() raise a different error for the same failed scan, so recording it later
  would let a lock-release failure replace a close or interruption error — the same masking this
  change removes.
• The release keeps its own finally, so it is still attempted even if constructing, settling, or
  reporting the input cleanups throws.
• warnCleanupFailed is total. Reading, coercing, and delivering the reason are all guarded,
  because a throw while reporting a cleanup failure would replace the scan result just as
  effectively as the original bug.
• Promise.allSettled also surfaces both input failures where Promise.all reported only the
  first.

Verification

• The new test fails against main and passes with the change: 67 passed / 1 failed before,
  68 passed / 0 failed after, in tests-ts/api.test.ts.

• The pre-change failure is the bug itself, not a different error:

  Expected substring: "the model refused the scan"
  Received message: "EFAULT: bad address in system call argument, rm '...codex-security-target-paths-....json'"
  

• Verified against real main source rather than a local revert, by restoring
  sdk/typescript/src/api.ts from upstream/main and confirming git diff --stat upstream/main
  reported no difference for that file before running the test.

• Full public SDK suite with the CI-pinned Bun 1.3.14: 471 passed, 6 expected skips, 0 failed
  (main baseline is 470 passed, 6 skipped; this change adds one test).

• pnpm run types, pnpm run format, pnpm run audit:prod, pnpm run build, pnpm pack, and
  pnpm run check:package all clean.

• The existing test that asserts the target-paths file is removed after a scan still passes, so
  cleanup is still performed rather than skipped.

Notes

• The test makes cleanup fail by replacing the target-paths file with a directory, so the
  non-recursive rm rejects on every platform rather than depending on POSIX permission bits.
  It asserts both that the real scan error survives and that exactly one cleanup warning is
  emitted, so a platform where the cleanup unexpectedly succeeded fails the warning assertion
  rather than passing vacuously.
• The credential-lock branch is structural rather than covered by a new test.
  releaseCredentialHome is only assigned for stored_credentials when prepareRuntime is
  undefined, and the test harness has to inject prepareRuntime, so no unit test can reach that
  branch. Flagging this rather than implying coverage it does not have.
• The new warning adds no unredacted disclosure path. It reaches only the onWarning observer and
  is never written to the workbench, and the CLI's handler already passes warnings through
  cliErrorMessage(). This is the distinction relative to Scan failure messages are stored without credential redaction #43, where the fail-scan --message
  path persists an unredacted message.
• Deliberately out of scope: a failed lock release still leaves a lock that
  recoverStaleCredentialHomeLock will not reclaim while the owning pid is alive. That liveness
  defect is in acquireCodexSecurityCredentialHomeLock / recoverStaleCredentialHomeLock in
  src/runtime.ts, not in this block, and needs its own change. This PR makes the failure
  visible rather than silent; it does not claim to repair the lock.
• Also out of scope: complete-scan running before collectResult, which lets a persistence
  error hide an incomplete scan. That is the other error-masking path in this file.

Reporter credit: @mariohercules, who identified the finally block and the replacement behavior
in #38.

[Haiduongcable]

Full CI is green on f7817f5 via a fork run, so this can be reviewed without approving a workflow run here.

Check	Result
ubuntu-latest / node 22, 24, 24.0.0, 26, 26.0.0	pass
macos-latest / node 22	pass
windows-latest / node 22	pass (4m7s)
linux-amd64 container	pass

The local node-ci step sequence also passes in order: audit:prod (no known vulnerabilities), types, test, format, build, pack, and check:package. Full suite with the CI-pinned Bun 1.3.14: 471 passed, 6 expected skips, 0 failed, against a 470-passed baseline on main.

Two notes for review:

• The Windows result is load-bearing rather than incidental. removeTargetPathsFile takes a chmod-and-retry branch that only runs on Windows, and the test asserts both that the real scan error survives and that exactly one cleanup warning was emitted. A platform where the cleanup unexpectedly succeeded would fail the warning assertion instead of passing vacuously.
• fix: make standard scan inventory authoritative #88 edits the same Promise.all this change replaces, adding a third cleanup call to it. The two are compatible and this change would cover that call as well, but it is worth sequencing to avoid a conflict.

[Haiduongcable]

Updated to 62611ac. A review pass found two real defects in the earlier revision, both now fixed, so the branch was squashed and force-pushed rather than amended piecemeal.

What changed since f7817f5:

• Settling the credential home release alongside the temporary input cleanup made a failed release survivable, which it is not. The release only marks itself done once the lock directory is gone, so a failure leaves an owner.json naming a live process that stale-lock recovery declines to reclaim. The earlier version reported success while leaving later scans in that process waiting on a lock nothing frees. A failed release now propagates, and is downgraded to a warning only when the scan had already failed.
• The failed-scan marker was recorded at the end of the catch block, after requireOpen and throwIfAborted. Both raise a different error for the same failed scan, so a client closed mid-scan or an interrupted scan reached cleanup with the marker unset and a failed release replaced that error — the masking this change exists to remove. It is now recorded on entry to the catch block.
• The warning formatter is total, so reading or coercing a hostile reason cannot throw in place of the scan result, and the release keeps its own finally so reporting the earlier failures cannot skip it.

The PR description has been corrected accordingly. Two claims in the first version were wrong: the change does not preserve the original ordering (allSettled waits for both input cleanups where Promise.all rejected early), and a lock-release failure on a completed scan still rejects rather than resolving with a warning.

Full CI is green on 62611ac via a fork run:

Check	Result
ubuntu-latest / node 22, 24, 24.0.0, 26, 26.0.0	pass
macos-latest / node 22	pass
windows-latest / node 22	pass
linux-amd64 container	pass

Local node-ci sequence also clean in order: audit:prod, types, test (471 passed, 6 expected skips, 0 failed, against a 470-passed baseline on main), format, build, pack, check:package.

One scoping note carried into the description: a failed release still leaves a lock that recoverStaleCredentialHomeLock will not reclaim while the owning pid is alive. That liveness defect is in acquireCodexSecurityCredentialHomeLock / recoverStaleCredentialHomeLock in src/runtime.ts and needs its own change. This PR makes the failure visible instead of silent; it does not claim to repair the lock. Happy to open a separate issue for it.

[mldangelo-oai]

Thanks for improving how scan cleanup failures are handled. The behavior and its important regression cases are already on main, so this branch is now redundant. I'm closing it as already addressed, and we'd be happy to see another contribution.