Skip to content

fix: handle deferred network proxy denials#19184

Merged
viyatb-oai merged 14 commits intomainfrom
codex/viyatb/fix-network-proxy-denials
Apr 29, 2026
Merged

fix: handle deferred network proxy denials#19184
viyatb-oai merged 14 commits intomainfrom
codex/viyatb/fix-network-proxy-denials

Conversation

@viyatb-oai
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai commented Apr 23, 2026

Why

This bug is exposed by Guardian/auto-review approvals. With the managed network proxy enabled, a blocked network request can be reported back through the network approval service as an approval denial after the command has already started. Before this change, the shell and unified exec runtimes registered those network approval calls, but did not have a way to observe an async proxy denial as a cancellation/failure signal for the running process.

The result was confusing: Guardian/auto-review could correctly deny network access, but the command path could keep running or unregister the approval without surfacing the denial as the command failure.

What Changed

  • NetworkApprovalService now attaches a cancellation token to active and deferred network approvals.
  • Proxy-denial outcomes are recorded only for active registrations, cancel the owning token, and are consumed when the approval is finalized.
  • The shell runtime combines the normal command timeout with the network-denial cancellation token.
  • Unified exec stores the deferred network approval object, terminates tracked processes when the proxy denial arrives, and returns the denial as a process failure while polling or completing the process.
  • Tool orchestration passes the active network approval cancellation token into the sandbox attempt and preserves deferred approval errors instead of silently unregistering them.
  • App-server command/exec now handles the combined timeout-or-cancellation expiration variant used by the runtime.

Verification

  • cargo test -p codex-core network_approval --lib
  • cargo clippy -p codex-app-server --all-targets -- -D warnings
  • cargo clippy -p codex-core --all-targets -- -D warnings

Teach the model how to interpret managed network proxy allow/deny context and wire proxy denials into running shell and unified exec processes so blocked requests cancel promptly and surface as command failures.

Co-authored-by: Codex noreply@openai.com
@viyatb-oai viyatb-oai requested a review from a team as a code owner April 23, 2026 19:11
Keep the prompt-template updates out of the managed network proxy denial plumbing PR so they can land separately.

Co-authored-by: Codex noreply@openai.com
@viyatb-oai viyatb-oai changed the title fix: handle managed network proxy denials fix: handle deferrede network proxy denials Apr 23, 2026
Cover the network-denial cancellation plus timeout expiration variant in command/exec so app-server builds remain exhaustive.

Co-authored-by: Codex noreply@openai.com
@viyatb-oai viyatb-oai changed the title fix: handle deferrede network proxy denials fix: handle deferred network proxy denials Apr 23, 2026
Co-authored-by: Codex noreply@openai.com
Copy link
Copy Markdown
Collaborator

@dylan-hurd-oai dylan-hurd-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good, but i'd like @jif-oai to look at unified exec. would be great to have slightly more detail in the denial messages

Copy link
Copy Markdown
Collaborator

@dylan-hurd-oai dylan-hurd-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're okay to proceed for now, but would like the denial messages to be a bit clearer about where the network access denial is coming from. I think lack of specificity will lead to model confusion here

Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
Co-authored-by: Codex noreply@openai.com
Co-authored-by: Codex noreply@openai.com
Co-authored-by: Codex noreply@openai.com
Copy link
Copy Markdown
Collaborator

@jif-oai jif-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unified exec still builds ExecOptions { expiration: ExecExpiration::DefaultTimeout, ... } without wiring attempt.network_denial_cancellation_token into the spawned request. The manager-side watcher covers the normal process after spawn, but anything inside the spawn/ preparation path, especially zsh-fork escalation setup, is not cancelled through the same expiration path as shell commands. That makes unified exec less robust than the shell path added in this PR.

Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
Co-authored-by: Codex noreply@openai.com
@viyatb-oai viyatb-oai requested a review from jif-oai April 25, 2026 21:21
@viyatb-oai
Copy link
Copy Markdown
Collaborator Author

@codex review

Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

_ = &mut expiration, if !timed_out => {
timed_out = true;
session.request_terminate();
}
exit = &mut exit_rx => {
if timed_out {
break EXEC_TIMEOUT_EXIT_CODE;
} else {

P2 Badge Distinguish cancellation from timeout in command/exec

TimeoutOrCancellation now feeds this expiration arm, but the branch always sets timed_out = true; later exit handling maps any such termination to EXEC_TIMEOUT_EXIT_CODE. Cancellation-triggered stops (including network-denial cancellations) are therefore mislabeled as timeouts in responses.

ℹ️ 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".

Comment thread codex-rs/core/src/unified_exec/process_manager.rs
Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
Comment thread codex-rs/core/src/exec.rs
Ensure deferred network denials emit failed unified-exec end events even when the process exits before it is stored, preserve short-lived denial output, and distinguish cancellation from timeouts across shell/app-server/Windows sandbox paths.

Co-authored-by: Codex noreply@openai.com
viyatb-oai and others added 2 commits April 27, 2026 13:59
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Comment thread codex-rs/core/src/unified_exec/process_manager.rs Outdated
if active_calls.len() == 1 {
return active_calls.values().next().cloned();
let calls = self.calls.lock().await;
if calls.active_calls.len() == 1 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes denial cancellation work only when exactly one network-backed command is active. Can't we have to live execs? (or more ofc)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Multiple live network-backed commands are possible. right now the proxy observer gives us an unattributed BlockedRequest, so this path only cancels when exactly one active call exists; with multiple active calls we cannot safely know which registration to fail, and intentionally avoid canceling the wrong one.

I've tried to fix this before but it ends up being a messy fix in the proxy code with registration/call ids in the headers and its brittle (an id via proxy credentials in the proxy URL).
I'll call this out explicitly in the code.

Comment thread codex-rs/core/src/exec.rs
Comment thread codex-rs/core/src/unified_exec/process_manager.rs
viyatb-oai and others added 3 commits April 28, 2026 13:11
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
@viyatb-oai viyatb-oai enabled auto-merge (squash) April 29, 2026 18:46
@viyatb-oai viyatb-oai merged commit 07c8b8c into main Apr 29, 2026
25 checks passed
@viyatb-oai viyatb-oai deleted the codex/viyatb/fix-network-proxy-denials branch April 29, 2026 19:14
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants