fix(sdk): preserve canceled scan state - #735
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53a8c9f052
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67afadc3ba
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
canceled is inferred from the signal's current state, not from the exception that ended the scan. If a real scan failure occurs and abort/close races before this bookkeeping block, the failure is recorded as canceled and its message/cost are dropped. Use cancel-scan only for a cancellation-derived failure, and add a fail-then-abort race test.
|
Addressed in commit |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ca814c11
ℹ️ 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".
| } | ||
| return ( | ||
| current === signal.reason || | ||
| (isRecord(current) && current["name"] === "AbortError") |
There was a problem hiding this comment.
When maxCostUsd tracking aborts the combined signal first, the Codex stream can surface a nested AbortError; if the caller signal or client close then aborts before this catch classifies the failure, the external-signal check passes and this name-only branch returns true even though signal.reason remains the original tracking error. The scan is consequently persisted with cancel-scan, losing the tracking failure message and cost instead of using fail-scan. Fresh evidence beyond the prior tracking-error fix is this nested AbortError path, which bypasses the first-abort origin retained by AbortSignal.any.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L25
Useful? React with 👍 / 👎.
…scan-cancel # Conflicts: # sdk/typescript/src/api.ts
|
@codex review |
|
Merged latest origin/main in commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb5e4dbc8
ℹ️ 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".
| const canceled = | ||
| signal.aborted && | ||
| (options.signal?.aborted === true || | ||
| this.#abortController.signal.aborted) && | ||
| isCancellationDerivedFailure(failure, signal); |
There was a problem hiding this comment.
When close() aborts an already registered scan just as an awaited operation completes normally, the subsequent checkOpen() throws CodexSecurity is closed. before throwIfAborted() can produce a cancellation-derived error. That ordinary error fails this predicate, so the scan is still persisted through fail-scan even though client closure caused it to stop. Classify this closed-client path as cancellation while continuing to preserve any earlier internal failure.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L25
Useful? React with 👍 / 👎.
|
Addressed the client-close race review in commit Validation: 5 focused API tests passed, TypeScript lint, Prettier, generated-model check, and plugin build passed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad93cd531d
ℹ️ 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".
| try { | ||
| await workbench({ ...activeScan.options, signal: undefined }, [ | ||
| "fail-scan", | ||
| canceled ? "cancel-scan" : "fail-scan", |
There was a problem hiding this comment.
When run() is called with { mock: true }, the dispatch at the start of #run bypasses this lifecycle selection, and #runMock still unconditionally invokes fail-scan in its catch block. A caller abort after onScanStarted therefore leaves the registered mock scan without canceled_at, while the equivalent real scan now uses cancel-scan; apply the same cancellation classification to the mock path so persisted scan state remains consistent.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 647d7266. Mock scans now use the same cancellation classification as real scans: cancellation-derived failures call cancel-scan without a failure message; ordinary failures still call fail-scan with the sanitized message. Updated the existing abort regression to assert the canceled terminal state. Focused API/mock tests, lint, format, model-generation, plugin build, and diff checks pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 647d72662f
ℹ️ 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".
| signal: AbortSignal, | ||
| ): boolean { | ||
| let current = failure; | ||
| for (let depth = 0; depth < 8; depth += 1) { |
There was a problem hiding this comment.
When a caller aborts a registered scan with an AbortSignal.reason containing at least seven nested ScanInterruptedError causes, throwIfAborted adds another wrapper and this loop exits before reaching the matching reason or AbortError. The scan is then persisted with fail-scan even though caller cancellation caused it; traverse until a non-ScanInterruptedError cause, using cycle detection rather than a fixed depth.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 3854a75f. Cancellation classification now traverses nested ScanInterruptedError causes until it reaches the underlying reason, with cycle detection and no arbitrary depth cutoff. The existing ordinary-failure and cost-limit guards remain fail-closed. Added a regression with ten nested interruption causes; focused API/mock tests, lint, format, model-generation, plugin build, and diff checks pass.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
Fixes #44. When a caller cancels a registered SDK scan, the scan history is
currently finalized with
fail-scan, so an intentional cancellation is storedas a failure instead of a cancellation.
Changes
ScanInterruptedErrorvalues throughcancel-scan.fail-scanfor ordinary failures, internal tracking failures, andScanCostLimitExceededErrorso those errors retain their message and costmetadata.
arguments for a canceled scan.
Testing
bun test --timeout 30000 tests-ts/api.test.ts— 139 passed, 2 skipped, 0 failed.bun test --timeout 30000 tests-ts/scan-recovery.test.ts— 40 passed, 0 failed.pnpm --ignore-workspace run lint— passed.pnpm --ignore-workspace --dir ../../plugins/codex-security/mcp-app run typecheck— passed.pnpm --ignore-workspace run generate:models:check— passed.pnpm --ignore-workspace run format— passed.git diff --check— passed.Risk and rollout
This changes only the persisted lifecycle command for explicit caller/client
cancellation. Ordinary failures and cost-limit recovery retain the existing
failure path and metadata. No public API or scan input contract changes.
Public disclosure review