Conversation
![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: 2fd8c58fab
ℹ️ 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".
| let method = request.method().clone(); | ||
| let headers = request.headers().clone(); | ||
| let version = request.version(); | ||
| let timeout = request.timeout().copied(); |
There was a problem hiding this comment.
When a request with .timeout(...) follows redirects, copying the same timeout into every redirected Request gives each hop a fresh full budget; for example, a 5s timeout can run for roughly 55s over the allowed 10 redirects before failing. Since this pool replaces reqwest's built-in redirect handling, it should track a single deadline for the whole send() and pass only the remaining time (or stop redirecting once it expires).
Useful? React with 👍 / 👎.
| let cross_origin = previous.host_str() != next.host_str() | ||
| || previous.port_or_known_default() != next.port_or_known_default(); |
There was a problem hiding this comment.
For redirects that change only the scheme while keeping an explicit same port, such as https://host:443 to http://host:443, this check evaluates as same-origin and preserves Authorization/Cookie headers onto the redirected request. Origins include the scheme, so include previous.scheme() != next.scheme() in this decision before carrying sensitive headers forward.
Useful? React with 👍 / 👎.
| let request = url | ||
| .into_url() | ||
| .map(|url| reqwest::Request::new(method, url)) | ||
| .map_err(RouteAwareRequestError::Request); |
There was a problem hiding this comment.
When callers pass credentials in the request URL, e.g. https://user:pass@example.com, this builder creates a raw reqwest::Request and never performs reqwest's usual userinfo extraction, so the request is sent without the expected Authorization: Basic ... header and can fail authentication for endpoints that rely on that supported URL form. Strip the userinfo and populate Basic auth when constructing the request.
Useful? React with 👍 / 👎.
1 participant
Why
PAC and OS proxy decisions are URL-specific. An API that resolves a route and then exposes a reusable transport client lets callers accidentally send a different URL through that client. The shared abstraction should instead couple request creation, route selection, and the URL that is ultimately sent.
What changed
RouteAwareClientPool, whose public API creates requests rather than exposing pooled clients.Review guide
http-client/src/route_aware_client_pool.rscontains the request API, route-keyed cache, and one-shot send path.http-client/src/default_client.rscontains the internal execute path and response/error aliases.Validation
Stack created with Sapling. Best reviewed with ReviewStack.