Skip to content

Cleanup failure in the scan finally block replaces the real scan error #38

Description

@mariohercules

Summary

The finally block at the end of #run awaits cleanup with Promise.all. An await that rejects inside finally discards the exception already in flight, so a cleanup failure replaces the real scan error with an unrelated filesystem error.

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

Both operations can reject. knowledgeBase.cleanup() is rm(path, { recursive: true, force: true })force suppresses ENOENT but not EPERM/EBUSY, which are routine on Windows while another handle is open. removeTargetPathsFile deliberately rethrows on non-Windows platforms.

The same file already uses the correct pattern elsewhere — Promise.allSettled at line 869, 879 and 1077 — so this looks like an oversight rather than a deliberate difference.

Affected version and environment

  • Released package: @openai/codex-security@0.1.1
  • Confirmed on current main at f22d4a36f26d16287bcdfd707b369116e02a08c3
  • macOS 26.5.2 (build 25F84)
  • Node.js v24.5.0
  • Bun 1.3.14

Steps to reproduce

No API call is required. The target-paths file is written into dirname(runtime.codexHome), so with an injected runtime the directory can be made read-only mid-scan, standing in for the EPERM/EBUSY that occurs naturally on Windows or on a read-only mount.

createCodex: () => ({
  startThread: () => ({
    id: null,
    async runStreamed() {
      async function* stream() {
        yield { type: "thread.started", thread_id: "t" };
        await chmod(base, 0o500);        // base === dirname(codexHome)
        throw new Error("Authentication failed: the configured credential was rejected.");
      }
      return { events: stream() };
    },
  }),
}),

await client.run(repository, { target: ["src"] });

Observed result:

real scan failure:    Authentication failed: the configured credential was rejected.
surfaced to caller:   EACCES: permission denied, rm '/.../runtime/codex-security-target-paths-cc2f6c70-....json'

=> original failure preserved? false

Expected behavior

Cleanup failures should not mask the scan failure. The original error should reach the caller; a cleanup problem is at most secondary information.

Actual behavior

The caller receives the cleanup error and the real cause is lost entirely — it is not attached as cause, not logged, and not present in JSON output.

Impact

The user is told their scan failed because a temporary file could not be removed, when the actual failure was authentication, a cost limit, an interrupt, or a contract violation. It also interacts badly with the failure classifier: an EACCES ... permission denied message is classified as forbidden, so the CLI then prints credential advice derived from an error that had nothing to do with credentials (see the related report about local errors being reported as connectivity or authorization failures).

Suggested direction

Use Promise.allSettled here, matching the three other cleanup sites in this file, and surface cleanup failures only when no scan error is already pending.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions