fix implicit OAuth during model discovery - #10929
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: 58d61f0615
ℹ️ 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".
| Err(e) => { | ||
| tracing::warn!("xAI token refresh failed, will re-authenticate: {}", e); | ||
| tracing::warn!("xAI token refresh failed: {}", e); | ||
| self.cache.clear(); | ||
| return Err(ProviderError::Authentication( | ||
| "xAI OAuth credentials expired. Sign in again to continue.".to_string(), | ||
| )); |
There was a problem hiding this comment.
When a cached token enters the refresh window, refresh_access_token can fail because of a temporary network error, a transient server response, or malformed response data, but this branch treats every failure as expired credentials and clears the only persisted refresh token. A brief outage therefore permanently signs a configured user out and forces another browser/device-code login; distinguish invalid-grant/authentication responses from retryable failures, and retain the cache for the latter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 417c9c8835
ℹ️ 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".
| }; | ||
| self.cache.save(&token_data)?; | ||
| Ok(token_data) | ||
| Err(ProviderError::NotConfigured) |
There was a problem hiding this comment.
When a direct xAI chat request has no cached token—or token refresh returns the new Authentication error—inner.stream() obtains the error through AuthProvider::get_auth_header, whose anyhow::Result erases the type; From<anyhow::Error> for ProviderError then converts it to ExecutionError. The explicit preflight added to fetch_supported_models() avoids this, but normal chat requests still lose NotConfigured/Authentication, so credential-specific handling and telemetry do not work. Preflight get_valid_token() in stream() as well, or preserve a nested ProviderError when converting from anyhow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868eb8d256
ℹ️ 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".
| reqwest::StatusCode::BAD_REQUEST | ||
| | reqwest::StatusCode::UNAUTHORIZED | ||
| | reqwest::StatusCode::FORBIDDEN => ProviderError::Authentication(details), |
There was a problem hiding this comment.
When the token endpoint returns 400/401/403 for an error unrelated to the refresh token itself—such as invalid_client, a malformed request, or a proxy/WAF rejection—this blanket status mapping produces Authentication, causing get_valid_token to clear the persisted refresh token. The user is then permanently signed out even though re-authentication cannot fix those conditions; only a response that definitively rejects the grant/token (for example invalid_grant) should discard it. Fresh evidence in this revision is that token_refresh_error still ignores the response body and classifies every one of these statuses as a rejected credential.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa4d765a47
ℹ️ 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".
| Some( | ||
| reqwest::StatusCode::BAD_REQUEST | ||
| | reqwest::StatusCode::UNAUTHORIZED | ||
| | reqwest::StatusCode::FORBIDDEN, | ||
| ) => ProviderError::Authentication(details), |
There was a problem hiding this comment.
When Kimi's refresh endpoint returns 400/401/403 for invalid_client, a malformed request, or a proxy/WAF rejection, this blanket mapping reports an authentication failure even though signing in again cannot fix it; configure_oauth() consequently starts a new device flow for any such error. Preserve the OAuth response error code in refresh_device_flow_token and classify only a definitive refresh-token rejection such as invalid_grant as Authentication, as the xAI path now does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77abb57ff8
ℹ️ 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".
| *guard = Some(usable.clone()); | ||
| return Ok(usable); | ||
| } | ||
| let usable = self.use_or_refresh(token).await?; |
There was a problem hiding this comment.
When two KimiCodeProvider instances cache the same near-expiry credential and Kimi rotates refresh tokens, the first instance can refresh and persist the new token while the second still attempts the old refresh token and receives invalid_grant. This new ? returns immediately instead of falling through to reload the disk cache as the previous implementation did, leaving the second instance to return Authentication on every request despite a valid refreshed credential on disk; reload and prefer the persisted token after an in-memory refresh failure before surfacing the error.
Useful? React with 👍 / 👎.
| let models = match provider.fetch_supported_models().await { | ||
| Ok(models) => models, | ||
| Err(goose_providers::errors::ProviderError::NotConfigured) => { | ||
| return Err(agent_client_protocol::Error::invalid_params() |
There was a problem hiding this comment.
Maybe the AuthRequired variant?
There was a problem hiding this comment.
You mean add Authenitcated error as an arm here? technically in our spreak they are different things, so mapping one on the other doesn't seem precise. I'll add Authenticated though so the client can distinguish
…th-model-fetch # Conflicts: # crates/goose-provider-types/src/errors.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8947e50b6f
ℹ️ 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".
| self.cache.clear(); | ||
| return Err(error); | ||
| } | ||
| Err(error) => return Err(error), |
There was a problem hiding this comment.
When a cached xAI token is inside the 120-second refresh skew but expires_at is still in the future, any transient refresh failure (network error, 5xx, or malformed refresh response) reaches this branch and aborts stream()/model discovery instead of using the still-valid access token. That makes xAI requests fail during brief refresh-endpoint outages even though the bearer token can still be accepted until its actual expiry; fall back to the cached token for non-auth refresh errors while it is unexpired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0235be654
ℹ️ 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".
| for attempt in 0..MAX_ATTEMPTS { | ||
| tracing::trace!("attempt {} to refresh api info", attempt + 1); | ||
| let info = match self.refresh_api_info().await { | ||
| let info = match self.refresh_api_info(&github_token).await { |
There was a problem hiding this comment.
When the saved GITHUB_COPILOT_TOKEN has been revoked or expires, this new model-discovery path calls refresh_api_info(&github_token), but that helper still uses error_for_status()?, which is converted by ProviderError::from(reqwest::Error) into RequestFailed for 401/403 responses rather than Authentication. As a result, on_list_provider_supported_models falls through to internal_error instead of the new auth_required branch, so clients cannot trigger the explicit sign-in flow for stale Copilot credentials; classify 401/403 from the Copilot token endpoint as ProviderError::Authentication before returning it.
Useful? React with 👍 / 👎.
* main: (32 commits) fix: keep turn-context in place for OpenAI Responses-stack models so prompt caching works (#10993) fix(developer): byte-bound the shell truncation preview (#10992) fix(openrouter): stop silently ignoring thinking effort off (#10991) fix: dispatch edited queued messages (#10933) fix: contain recipe template paths (#10930) fix: make shell approval titles faithful (#10986) fix: block MCP app form submissions (#10985) fix: migrate desktop routing to React Router 8.3.0 (#10971) fix: sanitize Bedrock tool errors (#10934) fix implicit OAuth during model discovery (#10929) fix: update React Router to 7.18.2 (#10967) test: early-exit code-exec smoke tests once tool invocation is observed (#10954) fix: keep ACP session naming out of live conversations (#10963) Bind MCP apps to trusted ownership metadata (#10747) tests: add recursion_limit attribute to remaining ACP test files (#10559) Sanitize Unicode tags in MCP resources (#10746) fix(oauth): preserve RFC 9207 iss from MCP OAuth callback (#10678) feat(installer): detect Termux and select musl portable build (#10568) feat: add Celeris provider (#10714) fix: shell ACP providers on desktop (#10907) ...
* main: (101 commits) fix: keep turn-context in place for OpenAI Responses-stack models so prompt caching works (#10993) fix(developer): byte-bound the shell truncation preview (#10992) fix(openrouter): stop silently ignoring thinking effort off (#10991) fix: dispatch edited queued messages (#10933) fix: contain recipe template paths (#10930) fix: make shell approval titles faithful (#10986) fix: block MCP app form submissions (#10985) fix: migrate desktop routing to React Router 8.3.0 (#10971) fix: sanitize Bedrock tool errors (#10934) fix implicit OAuth during model discovery (#10929) fix: update React Router to 7.18.2 (#10967) test: early-exit code-exec smoke tests once tool invocation is observed (#10954) fix: keep ACP session naming out of live conversations (#10963) Bind MCP apps to trusted ownership metadata (#10747) tests: add recursion_limit attribute to remaining ACP test files (#10559) Sanitize Unicode tags in MCP resources (#10746) fix(oauth): preserve RFC 9207 iss from MCP OAuth callback (#10678) feat(installer): detect Termux and select musl portable build (#10568) feat: add Celeris provider (#10714) fix: shell ACP providers on desktop (#10907) ...
2 participants
Fixes #10909
Summary
ProviderError::NotConfiguredNotConfiguredat the ACP boundary without duplicating provider configuration checksRoot cause
OAuth-backed providers could fall through from credential lookup into first-time OAuth during ordinary provider operations, opening a browser or starting device-code polling without an explicit sign-in action.
The provider is now the single authority for credential state. Missing credentials return
NotConfigured; present but invalid configuration still reaches the provider request and returns its underlying error, preserving setup validation.This intentionally removes implicit sign-in from chat as well as model discovery.
goose configureand the client sign-in action remain the explicit setup routes. Expired or rejected refresh tokens therefore require explicit re-authentication. Transient network, rate-limit, and server failures retain the stored refresh token and return retryable provider errors instead.Verification
cargo test -p goose-provider-types retry::testscargo test -p goose missing_token_does_not_start_oauthcargo test -p goose token_refresh_errors_distinguish_rejected_and_transient_requestscargo test -p goose get_api_info_uses_valid_cache_without_github_tokencargo test -p goose fetch_supported_models_does_not_authenticate_when_unconfiguredcargo test -p goose use_or_refresh_preserves_transient_error_for_expired_tokencargo test -p goose kimi_code_inventory_configured_uses_token_cachecargo test -p goose --test acp_custom_requests_test test_custom_provider_supported_modelscargo fmtgit diff --check