fix(api): check HTTP status in Copilot OAuth responses#145
Conversation
request_device_code, poll_for_access_token, and exchange_for_copilot_token called response.json() directly, so a non-2xx response (invalid/expired token, rate limit, GitHub outage) was parsed as if it were a success payload, producing a confusing serde deserialization error instead of a clear message with the status code and body. Add ensure_success(), mirroring the pattern already used in openai.rs/responses.rs, and cover it with regression tests against a local stub server.
|
@codex review |
![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: d9593f7bc6
ℹ️ 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".
| .send() | ||
| .await | ||
| .map_err(ApiError::Http)?; | ||
| let response = ensure_success(response).await?; |
There was a problem hiding this comment.
In the normal Copilot device-code flow, the token endpoint can return OAuth error JSON such as authorization_pending or slow_down with HTTP 400; this function already has match arms to keep polling for those codes. Because ensure_success now runs before deserializing the body, the first pending/slow-down response is returned as ApiError::Api and run_copilot_device_code_auth aborts instead of waiting for the user. Parse the token response, or special-case OAuth error JSON, before applying the generic non-2xx rejection on this polling endpoint.
Useful? React with 👍 / 👎.
| "{\"message\":\"Bad creds\"}" | ||
| ); | ||
| let (base_url, _rx) = spawn_test_server(response); | ||
| let client = reqwest::Client::new(); |
There was a problem hiding this comment.
These new stub-server tests use reqwest::Client::new(), which honors HTTP_PROXY/HTTPS_PROXY from the environment; in proxy-configured CI/sandboxes where the active NO_PROXY does not cover 127.0.0.1, the request never reaches spawn_test_server and the assertions see the proxy's 403 instead of the stubbed 401/500 (observed with cargo test -p api copilot). Build the test clients with proxying disabled, and do the same in the test below, so the regression is deterministic.
Useful? React with 👍 / 👎.
…lot OAuth flow In poll_for_access_token, read response body first and try to parse as AccessTokenResponse before checking HTTP status. This allows 'authorization_pending' and 'slow_down' errors (HTTP 400) to flow through to the existing poll-loop handling instead of being rejected by ensure_success as ApiError::Api. Also disable proxy for localhost regression tests — use reqwest::Client::builder().no_proxy() to prevent HTTP_PROXY env vars from intercepting test requests to the local stub server.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
copilot.rs's three GitHub OAuth HTTP calls (request_device_code,poll_for_access_token,exchange_for_copilot_token) parsed the response body withresponse.json()without first checkingresponse.status().is_success().serde/reqwest decoding error instead of a clear message with the HTTP status and body — mirroring how sibling clients (openai.rs,responses.rs) already handle this.ensure_success()helper (sameApiError::Api { status, body, .. }shape used elsewhere in the crate) and applied it before every JSON parse in this file.Test plan
exchange_for_copilot_token_surfaces_non_success_statusandrequest_device_code_surfaces_non_success_statusregression tests against a local stub HTTP server, proving a 401/500 response now surfacesApiError::Apiwith the real status + body instead of a confusing decode error.cargo fmt --package apicargo test -p api(192 passed)cargo clippy -p api --all-targets -- -D warnings(clean)