Conversation
ab5927c to
10c283c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080d07d94f
ℹ️ 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".
| http = { workspace = true } | ||
| url = { workspace = true } |
There was a problem hiding this comment.
Regenerate the Bazel lockfile after dependency changes
When Bazel or CI runs after these Cargo.toml/Cargo.lock dependency edits, MODULE.bazel.lock remains stale because this change does not include the output of just bazel-lock-update. The repo requires refreshing and committing that lockfile whenever Rust dependencies change, and CI verifies drift, so Bazel lock checks will fail until MODULE.bazel.lock is updated.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
| route: &OutboundProxyRoute, | ||
| ) -> Result<reqwest::Client, BuildRouteAwareHttpClientError> { | ||
| let builder = match route { | ||
| OutboundProxyRoute::TransportDefault => builder, |
There was a problem hiding this comment.
Preserve direct fallback for unresolved system routes
When respect_system_proxy is enabled and the platform resolver returns Unavailable for a backend URL (for example a PAC/WPAD failure), the route-aware pool passes TransportDefault into this method. Leaving the builder unchanged here skips the configure_env_proxy_handling(...).no_proxy() fallback used by the synchronous route builder, so in the no-env-proxy case these backend requests can still use reqwest's default proxy discovery instead of the intended direct fallback.
Useful? React with 👍 / 👎.
## Why The `codex-http-client` migration now has a shared implementation and several migrated request paths, but nothing prevents a new crate from adding another direct `reqwest` dependency while the remaining call sites are being converted. The dependency graph should both enforce the direction of travel and make the remaining scope visible. This PR adds that ratchet on top of #31363. It does not claim the migration is complete: the allowlist deliberately records all 18 first-party crates that still depend on `reqwest` directly. ## What changed - Ban `reqwest` with cargo-deny unless its immediate parent is an explicitly listed wrapper. - Identify `codex-http-client` as the intended owner. - Record the 18 current first-party direct dependents as temporary migration exceptions. - Separately allow six third-party integrations that own their `reqwest` dependency: `oauth2`, `opentelemetry-http`, `opentelemetry-otlp`, `rmcp`, `sentry`, and `webrtc-sys-build`. - Cover both `reqwest` 0.12 and 0.13 with the same package-level rule. ## Migration rule A new first-party crate cannot add `reqwest`. When a listed crate finishes migrating, its direct Cargo dependency and its wrapper entry should be removed in the same PR, so the first-party list can only shrink. ## Review guide The entire change is the new `reqwest` entry in `codex-rs/deny.toml`: 1. `codex-http-client` is the permanent intended wrapper. 2. The next 18 entries are the first-party migration backlog. 3. The final six entries are separately documented third-party parents required by cargo-deny graph semantics. ## Validation - `cargo deny check bans --hide-inclusion-graph` (`bans ok`; existing duplicate-version warnings remain warnings) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31431). * #31837 * #31828 * #31825 * #31821 * #31637 * __->__ #31431
a1ef581 to
fc0a88a
Compare
## Why Login already honors `respect_system_proxy`, but several login-owned auth flows still construct and pass around raw `reqwest::Client` values. That keeps those request paths coupled to the underlying transport and leaves `codex-login` on the temporary direct-`reqwest` allowlist introduced by #31431. Auth endpoints also have a stricter logging boundary than ordinary API requests: custom issuer URLs and response headers may contain credentials. Moving these requests behind the shared HTTP abstraction must preserve that boundary while retaining route-aware proxy and custom-CA behavior. This is a bounded login migration. The separate Agent Identity and shared default-client compatibility migrations remain follow-up work. ## What changed - Add `HttpClientFactory::build_client` to construct the shared `HttpClient` abstraction for a resolved destination and route class. - Add a route-aware construction path that suppresses request URL, response-header, and transport-error diagnostics for sensitive auth endpoints. - Route device-code user-code/polling requests, OAuth authorization-code exchange, and API-key token exchange through `HttpClient`. - Build the revoke timeout test client through the same factory API. - Use the transport-neutral `http::StatusCode` in the migrated device-code flow. - Add an end-to-end log-capture regression test covering successful responses and transport failures after `RequestBuilder` transformations. ## Review guidance The request behavior is intended to be unchanged: each issuer/token endpoint selects the same auth route, including the existing system/PAC proxy and custom-CA handling, and raw auth clients still omit Codex default headers. The intentional logging change is limited to raw auth requests, whose URL userinfo, query credentials, response headers, and transport errors must not cross the auth redaction boundary. This PR deliberately does **not** remove `codex-login` from #31431's allowlist. The remaining direct `reqwest` surface belongs primarily to: - Agent Identity APIs that still accept `reqwest::Client`. - Exported default-client compatibility helpers used by other workspace crates. - A small number of tests and concrete error/header types. ## Testing - `cargo check -p codex-http-client -p codex-login --tests` - `just test -p codex-http-client` (41 tests) - `just test -p codex-login` (155 tests) - `just bazel-lock-check` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31637). * #31837 * #31828 * #31825 * #31821 * __->__ #31637
Why
codex-backend-clientowns ChatGPT backend traffic used by account state, rate limits, cloud tasks, cloud config, and memories. Those requests must honorfeatures.respect_system_proxy; leaving the crate on a directly constructed reqwest client creates a large gap in the migration started by #31431 and #31637.What changed
HttpClientFactorythrough cloud config, cloud tasks, app-server account operations, and memories.deny.tomlexception fromcodex-backend-client.Review guide
backend-client/src/client.rsfor the transport migration and request-level invariant.Validation
cargo checkcovered backend-client and its migrated consumers.Stack created with Sapling. Best reviewed with ReviewStack.