fall back to static model list only for non-models payloads - #10189
Conversation
|
cc @DOsinga PTAL, thanks |
![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: a2157bc794
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9030200712
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9030200712
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9030200712
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e5fe22900
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…models fetch Previously api_get() collapsed all body-read failures (timeouts, connection resets, invalid JSON) into payload: None, which then got classified as EndpointNotFound and triggered a silent fallback to the static model list. Now uses response_get() directly so network errors propagate as NetworkError while only genuine JSON decode failures become EndpointNotFound — matching the OpenAI engine behavior. Addresses Codex P2 review comment on PR aaif-goose#10189. Signed-off-by: dongjiang <dongjiang1989@126.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7be39526b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… is absent
A valid models payload like {"data":[...], "message":"ok"} was being
rejected as an auth error because the top-level message check ran
before the data-array check. Now the message check only fires inside
the missing-data branch, so payloads with a `data` array are always
accepted regardless of extra informational fields.
Addresses Codex P2 review comments on PR aaif-goose#10189.
Signed-off-by: dongjiang <dongjiang1989@126.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 623ca144d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
response.json() conflates transport/encoding errors (e.g. bad gzip from a proxy) with JSON parse failures — both appear as is_decode() in reqwest. Now reads the body as text first (NetworkError on transport failure) and parses JSON separately (EndpointNotFound only on actual parse failure), matching the Anthropic path. Addresses Codex P2 review comment on PR aaif-goose#10189. Signed-off-by: dongjiang <dongjiang1989@126.com>
Port audited changes from aaif-goose/goose#10189. Upstream commits: a2157bc79, 903020071, 2e5fe2290, 7be39526b, 623ca144d, 07077411f. Local audit: patch applied cleanly after adding the #10041 canonical_provider_id field to the new OpenAI test helper; invalid/non-model payloads map to EndpointNotFound for static fallback while auth/error payloads still propagate. Gate: source bin/activate-hermit && cargo fmt && cargo test -p goose-providers fetch_models_treats && cargo test -p goose-providers fetch_supported_models
…s only Fixes aaif-goose#10074. When a provider's base_url is misconfigured, the /models endpoint can return an HTML error page or a non-models JSON response. Previously this surfaced as a raw JSON-decode error that aborted provider configuration entirely, even though a static model list was available as fallback. Changes to fetch_models_from_api in both OpenAI and Anthropic engines: - HTTP status errors (401/403/429/etc.) propagate unchanged so real misconfiguration stays visible - Body-read failures (timeouts, bad gzip, connection resets) propagate as NetworkError - JSON decode failures (HTML pages, non-JSON responses) are reclassified as EndpointNotFound, triggering the existing static-list fallback - 200 responses with error payloads ({"error":{"message":"..."} or top-level {"message":"..."} without a "data" array) propagate as Authentication errors - Valid payloads with a "data" array are accepted even if they carry extra informational fields like {"message":"ok"} Added 14 wiremock regression tests (7 per engine) covering all paths. Signed-off-by: dongjiang <dongjiang1989@126.com>
0707741 to
d938bef
Compare
|
Thanks for the follow-up here. This is definitely closer to the shape we discussed on the earlier PR, in particular preserving non-2xx errors and not falling back on obvious auth-style error payloads. One thing I am still worried about is that the fallback condition may still be too broad. In particular, a 200 response with valid JSON but no I think we should be careful to avoid making setup look successful in cases where the provider is broken. Falling back on an actual 404 makes sense, and perhaps falling back on invalid JSON/HTML does too if we explicitly want to support providers without a models endpoint, but I am less convinced arbitrary valid JSON missing |
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for iterating on this. This is much closer than the previous version: checking HTTP status before parsing, separating body-read failures from JSON parse failures, and preserving explicit error payloads are all the right direction.
I still think the fallback is too broad though. In particular, a 200 response with valid JSON but no data array should not automatically be treated as EndpointNotFound. That can hide real configuration/provider errors from proxies or compatible APIs, e.g. {"status":"ok"}, {"success":false}, {"detail":"invalid api key"}, {"errors":[...]}, etc. Falling back to the static model list in those cases makes the configuration look healthy until a later request fails in a less obvious place.
I would narrow the fallback to cases that are strong evidence that /models is unavailable or not a models endpoint, such as the explicit 404 path and possibly invalid JSON/HTML from an otherwise successful response. For valid JSON missing data, I think we should return a real provider/configuration error instead of EndpointNotFound, unless there is a specific known provider shape we want to support.
A couple of cleanup points while you're there:
- Please remove the redundant explanatory comments in the implementation and tests. Most of them restate the branch/test name or the literal mock payload, and they make this harder to maintain.
- The
{"status":"ok"}test currently locks in the broad fallback behavior above. I think that should be changed to assert that valid JSON without a modelsdataarray does not fall back silently. - Be careful with mapping top-level
messagetoAuthentication; a top-level message can represent many provider/proxy errors, not only auth.
I'm going to snooze this briefly to give you time to respond/update.
- Missing `data` array now returns RequestFailed instead of EndpointNotFound, so valid JSON without `data` propagates to the user instead of silently falling back to the static list - Remove top-level `message` -> Authentication mapping (semantically wrong, not specific to auth errors) - Remove redundant comments from implementation and tests - Rename tests to match new error classifications - Add test asserting missing-data does not fall back through fetch_supported_models
|
Thanks for the review @DOsinga. I've pushed fixes addressing your feedback:
|
|
@codex Please review the latest commits (d938bef and 0e4a4a4). Key changes since your last review:
|
|
@DOsinga Please re-check it, thanks |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
# Conflicts: # crates/goose-providers/src/openai.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b88a3a8d0
ℹ️ 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 body = response.text().await.map_err(|e| { | ||
| ProviderError::NetworkError(format!("Failed to read response body: {}", e)) | ||
| })?; | ||
| let json: serde_json::Value = serde_json::from_str(&body).map_err(|e| { | ||
| ProviderError::EndpointNotFound(format!("Response body is not valid JSON: {}", e)) |
There was a problem hiding this comment.
When an OpenAI-compatible endpoint or proxy returns malformed UTF-8 inside otherwise JSON-shaped bytes, Response::text() replaces the invalid bytes with U+FFFD instead of failing. serde_json::from_str can then accept the modified payload and return a corrupted model ID rather than classifying the response as invalid and using the configured static models. Read the body with bytes() and parse it with serde_json::from_slice so invalid encoding remains a parse failure.
Useful? React with 👍 / 👎.
| let body = response.text().await.map_err(|e| { | ||
| ProviderError::NetworkError(format!("Failed to read response body: {}", e)) | ||
| })?; | ||
| let json: Value = serde_json::from_str(&body).map_err(|e| { | ||
| ProviderError::EndpointNotFound(format!("Response body is not valid JSON: {}", e)) |
There was a problem hiding this comment.
When an Anthropic-compatible endpoint or proxy returns malformed UTF-8 inside otherwise JSON-shaped bytes, Response::text() performs lossy replacement and allows serde_json::from_str to produce corrupted model identifiers. This bypasses the intended invalid-payload fallback and can expose an unusable dynamic model name; parse response.bytes() with serde_json::from_slice instead so invalid encoding is rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0367642100
ℹ️ 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".
| ProviderError::EndpointNotFound(format!("Response body is not valid JSON: {}", e)) | ||
| })?; | ||
|
|
||
| if let Some(err_obj) = json.get("error") { |
There was a problem hiding this comment.
For an Anthropic-compatible endpoint returning a successful envelope such as {"data":[{"id":"model-a"}],"error":null}, json.get("error") still returns Some(Value::Null), so this branch reports Authentication("unknown error") and aborts configuration instead of accepting the dynamic model list. Only treat a non-null error payload as an error, allowing successful responses that explicitly include error: null.
Useful? React with 👍 / 👎.
# Conflicts: # crates/goose-providers/src/openai.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24c9b7ef3b
ℹ️ 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".
| if let Some(err_obj) = json.get("error").filter(|error| !error.is_null()) { | ||
| let msg = err_obj | ||
| .get("message") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("unknown error"); | ||
| return Err(ProviderError::Authentication(msg.to_string())); |
There was a problem hiding this comment.
When an Anthropic-compatible endpoint returns HTTP 200 with an error envelope for a non-authentication failure, such as {"error":{"type":"rate_limit_error","message":"quota exceeded"}}, this branch reports Authentication regardless of the supplied error type. Configuration still aborts, but the user is incorrectly told to fix credentials rather than seeing a rate-limit, credits, or server error; inspect the envelope's type and preserve the corresponding ProviderError category, using a generic request failure for unknown types.
Useful? React with 👍 / 👎.
* origin/main: (42 commits) chore (codex-acp): migrate to @agentclientprotocol/codex-acp (#10923) style: add dark mode inline code styling for message bubbles (#10861) docs: clarify macOS sandbox feature was removed in post-v1.25.0 cleanup (#10900) fall back to static model list only for non-models payloads (#10189) Remove CLI project support (#10838) fix(anthropic): add claude-sonnet-5 and claude-fable-5 to known models list (#10865) fix: stdio extensions silently skipped when name missing or env: used in config (#10773) feat(desktop): show session metadata on sidebar chat hover (#10663) fix(serve): start scheduler at startup instead of first ACP connection (#10766) fix: respect disabled builtin extensions (developer) at session start (#10223) fix: parse PyPI requirements for OSV checks (#10510) fix(telegram): contain voice file extensions (#10456) Revert "feat(security): chunk command-classifier input with overlapping windows" (#10416) (#10870) docs: update Discord invite (#10863) fix(security): preserve denied tool request precedence (#10612) fix(hints): contain subdirectory hint discovery (#10545) chore(deps): bump pem from 3.0.6 to 4.0.0 (#10853) chore(deps): bump base64 from 0.22.1 to 0.23.0 (#10851) chore(deps): bump jsonwebtoken from 10.4.0 to 11.0.0 (#10850) chore(deps): bump astral-sh/setup-uv from 8.3.2 to 9.0.0 (#10847) ...
* origin/main: (58 commits) Handle MCP tool list change notifications fix: restore final_output_tool when agent is recreated after LRU eviction (#10793) feat(ui): bring back make sidebar resizable with drag handle and persisted width (#10795) docs: document auto-injected GOOSE_SHELL flags (#10759) feat: surface output-token-limit info (#10831) fix(desktop): avoid O(n²) cloning during session load (#10665) feat: add interactive menu for single select elicitations (#10327) fix(acp): prefix child stderr log target so it passes the default goose=info filter (#10901) chore (codex-acp): migrate to @agentclientprotocol/codex-acp (#10923) style: add dark mode inline code styling for message bubbles (#10861) docs: clarify macOS sandbox feature was removed in post-v1.25.0 cleanup (#10900) fall back to static model list only for non-models payloads (#10189) Remove CLI project support (#10838) fix(anthropic): add claude-sonnet-5 and claude-fable-5 to known models list (#10865) fix: stdio extensions silently skipped when name missing or env: used in config (#10773) feat(desktop): show session metadata on sidebar chat hover (#10663) fix(serve): start scheduler at startup instead of first ACP connection (#10766) fix: respect disabled builtin extensions (developer) at session start (#10223) fix: parse PyPI requirements for OSV checks (#10510) fix(telegram): contain voice file extensions (#10456) ... # Conflicts: # Cargo.lock # crates/goose/src/agents/extension_manager.rs # crates/goose/tests/mcp_replays/npx-y@modelcontextprotocol_server-everything@2026.1.14
2 participants
Summary
Originally posted by @DOsinga in #10153 (comment)
Fixes #10074.
When a provider's
base_urlis misconfigured, the/modelsendpoint can return an HTML error page (or any non-models JSON). Previously this surfaced as a raw JSON-decode error that aborted provider configuration entirely — even though a perfectly good static model list was available as a fallback.The fix narrows
fetch_models_from_apiin both the OpenAI and Anthropic engines so that only non-models payloads (invalid JSON or a response missing thedataarray) are reclassified asEndpointNotFound, which lets the existingfetch_supported_modelsfallback return the predefined static list. Auth, credit, rate-limit and network errors are unaffected — they continue to propagate so the user sees real misconfiguration.Testing
cargo test -p goose-providers --lib openai::tests(4 new tests)cargo test -p goose-providers --lib anthropic::tests(4 new tests)cargo clippy --all-targets -- -D warningscargo fmt --checkRelated Issues
Relates to #10153
Discussion: #10153 (comment)_
Screenshots/Demos (for UX changes)
Before:
After: