Conversation
c36a8e3 to
a1bbf86
Compare
anp-oai
approved these changes
Jul 7, 2026
| return Ok(build_reqwest_client()); | ||
| } | ||
| if is_sandboxed() { | ||
| // Preserve the sandbox's existing no-proxy policy; sandboxed command egress is routed |
Collaborator
There was a problem hiding this comment.
Don't need to address here but this seems like something we'll need to reconcile before we make respect_system_proxy the default
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
features.respect_system_proxyalready routes authentication traffic through the OS proxy APIs, but it does not affect the primary inference path. That leaves users behind OS-managed proxies unable to send normal Responses API requests even after login succeeds.This PR is the first product-path migration onto the route-aware transport introduced in #31323 and refined in #31331. It also establishes the construction pattern for later migrations: the effective feature state is resolved once into a required HTTP client factory rather than represented by an optional per-call setting.
The scope remains limited to the two HTTP Responses endpoints; WebSockets, model discovery, memories, realtime, and file uploads remain follow-up migrations.
What changed
OutboundProxyPolicy::{ReqwestDefault, RespectSystemProxy}and a requiredHttpClientFactory. The policy has no default, and the lower-level route-aware reqwest builder is now private.Configconstruct the factory from the effective feature state and require everyModelClientconstructor to receive it. There is no optional setter or implicitNonefallback./responsesand/responses/compactwithClientRouteClass::Api, using the complete destination URL so PAC rules can make URL-specific decisions.OutboundProxyPolicy::RespectSystemProxy.Review guide
http-client/src/outbound_proxy.rsdefines the mandatory policy/factory boundary and keeps route resolution private.core/src/config/mod.rs,core/src/session/session.rs, andcore/src/client.rsshow the compile-time invariant: effective config creates the factory, andModelClientcannot be constructed without one.login/src/auth/default_client.rspreserves existing default-client behavior while accepting the required factory for migrated routes.core/src/client.rsswitches only streaming Responses and remote compaction HTTP transports to the API route class.core/src/config/config_tests.rscovers feature-to-factory resolution;http-client/src/outbound_proxy_tests.rscovers routing through an environment-selected proxy.Test plan
cargo check --tests -p codex-http-client -p codex-login -p codex-corejust test -p codex-loginjust test -p codex-core respect_system_proxy_feature_resolves_enabledcompact_uses_bearer_after_agent_identity_session_fallbackcoverage passes with the new transport construction.just bazel-lock-checkStack created with Sapling. Best reviewed with ReviewStack.