Skip to content

core-plugins: route remote requests through HTTP client factory#31837

Open
bolinfest wants to merge 1 commit into
pr31924from
pr31837
Open

core-plugins: route remote requests through HTTP client factory#31837
bolinfest wants to merge 1 commit into
pr31924from
pr31837

Conversation

@bolinfest

@bolinfest bolinfest commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Why

features.respect_system_proxy only works consistently when every remote-plugin request uses the route-aware service state established from the effective HttpClientFactory. Direct reqwest construction bypassed that policy, while returning a selected transport client would still allow the eventual request URL to diverge from the URL used for PAC/system-proxy routing.

The shared service configuration and curated startup-sync migration now live in #31924, leaving this PR focused on consuming that state for remote plugin APIs and backend-issued upload/download destinations.

What changed

  • Add request-builder helpers to RemotePluginServiceConfig so route selection stays coupled to the eventual request URL.
  • Route catalog reads, featured-plugin discovery, mutations, sharing, signed uploads, and bundle downloads through the shared abstraction.
  • Build query strings before request creation and route backend-issued upload/download destinations independently.
  • Collapse now-impossible client-construction error branches into the request error path.
  • Remove the direct reqwest dependency and temporary deny.toml exception from codex-core-plugins.
  • Add recording-selector coverage for complete query URLs and backend-issued destinations without expanding the public API.

Review guide

  1. Start with the small RemotePluginServiceConfig::{http_request,catalog_request} additions in remote.rs; the selector/config foundation is reviewed in core-plugins: route curated startup sync through HTTP client factory #31924.
  2. Review remote.rs, remote/share.rs, remote_legacy.rs, and remote_bundle.rs by request family.
  3. Review the signed upload and bundle-download paths carefully: both choose a route from the backend-issued destination, not the preceding API URL.
  4. The recording selector in test_support.rs verifies complete query URLs and backend-issued destinations.

Validation

  • The commit builds independently with cargo check -p codex-core-plugins -p codex-core -p codex-app-server --tests.
  • Route-recording, upload/download, and remote catalog tests in codex-core-plugins.

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector

Copy link
Copy Markdown
Contributor

💡 Codex Review

http = { workspace = true }
url = { workspace = true }

P1 Badge Refresh the Bazel cargo lock after dependency changes

This patch changes Rust dependencies in Cargo.toml/Cargo.lock, but the diff does not include an updated MODULE.bazel.lock. The repo requires running just bazel-lock-update for dependency changes, and CI verifies lockfile drift, so this will block the Bazel lockfile check until the generated lock update is committed.

AGENTS.md reference: AGENTS.md:L37-L39


self.client_for_url_with_resolver(request_url, move |request_url| async move {
http_client_factory
.resolve_proxy_route_async(request_url)
.await
})

P2 Badge Re-resolve proxy routes when redirects change hosts

When respect_system_proxy is enabled, this resolves PAC/system proxy routing only for the original request_url and then sends the request with a reqwest client fixed to that Direct/Proxy route. Reqwest follows redirects inside the same client, so redirecting requests such as the GitHub zipball API URL used by curated plugin sync (which redirects to a different host) will not re-run PAC/NO_PROXY selection for the redirected URL; in environments where the redirect target uses a different proxy decision, the request can fail or bypass the required proxy. Consider disabling automatic redirects for route-aware callers and following them after resolving the next URL, or otherwise making redirect routing re-evaluate the final destination.

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

@bolinfest bolinfest force-pushed the pr31837 branch 2 times, most recently from a21a737 to 4fd8c87 Compare July 9, 2026 18:41
@bolinfest bolinfest removed the request for review from a team July 9, 2026 18:43
bolinfest added a commit that referenced this pull request Jul 9, 2026
## 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
bolinfest added a commit that referenced this pull request Jul 9, 2026
## 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
@bolinfest bolinfest changed the base branch from pr31828 to pr31924 July 9, 2026 22:35
@bolinfest bolinfest changed the title core-plugins: route requests through HTTP client factory core-plugins: route remote requests through HTTP client factory Jul 9, 2026
@bolinfest bolinfest force-pushed the pr31924 branch 2 times, most recently from 664cffa to 6698524 Compare July 9, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant