fix: keep scan cleanup failures from replacing the scan result

[Haiduongcable]

Summary

Fixes openai#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 openai/codex-security#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 openai#38.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7817f50c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve failure when the credential lock remains held

When releaseCredentialHome rejects after failing to remove .codex-security-scan.lock (for example, because rm encounters EACCES or EBUSY), this converts the error into an optional warning and allows a successful scan to resolve. recoverStaleCredentialHomeLock in src/runtime.ts considers the owner PID live, so another scan in the same long-lived SDK process will keep polling for a lock that nothing retries releasing; callers without onWarning receive no indication before they hang. Preserve the primary scan error when one exists, but do not report clean success when the credential lock remains held (or explicitly retry/invalidate that lock).

Useful? React with 👍 / 👎.