fix(providers): fall back to predefined model list when /models endpoint fails - #10153
fix(providers): fall back to predefined model list when /models endpoint fails #10153dongjiang1989 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e76835896b
ℹ️ 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".
e768358 to
3872d97
Compare
…int fails Signed-off-by: dongjiang <dongjiang1989@126.com>
3872d97 to
3f47661
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f47661cc5
ℹ️ 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".
itxaiohanglover
left a comment
There was a problem hiding this comment.
Solid fix — falling back to the predefined model list on any error (not just 404) prevents a misconfigured /models endpoint from breaking the provider entirely. The test coverage for the fallback path is good.
|
Thanks for digging into this and for the small, well-scoped PR — the intent is good. The problem is that Falling back to the static list on any error makes that signal disappear. It also swallows The real fix for #10074 is on the base-URL side: guide the user to the correct base URL / fix the Going to close this one for now. A couple of pointers for next time: we ask that AI review comments (the codex P2 here) get addressed or explained before a PR moves forward — see https://github.com/aaif-goose/goose/blob/main/CONTRIBUTING.md#ai-code-reviews. Really do appreciate the contribution, and a follow-up that fixes the base-URL handling would be very welcome. |
Summary
/modelsAPI endpoint returns non-404 errors (invalid JSON, auth errors, server errors, wrong base URL,etc.)
OpenAiProviderandAnthropicProvidernow fall back to their predefined static model list on any API error, not just 404tracing::warnlog is emitted so the failure is observableFixes #10074
Root Cause
When configuring Zhipu AI (or any provider with
dynamic_models: trueand a non-standard base URL), the derived/modelsendpoint URL may be wrong. Forexample, entering
https://open.bigmodel.cninstead ofhttps://open.bigmodel.cn/api/paas/v4produces a models URL ofhttps://open.bigmodel.cn/v1/models, which does not exist.The endpoint returns HTTP 200 with a non-JSON body (e.g. an HTML page), causing
handle_responseto fail withProviderError::RequestFailed("Response body is not valid JSON: ..."). Previously,fetch_supported_modelsonly fell back to the static list onEndpointNotFound(404), so thisRequestFailederror propagated up and blocked the provider configuration entirely.Changes
crates/goose-providers/src/openai.rsOpenAiProvider::fetch_supported_models: replace the catch-allErr(e) => return Err(e)branch with a fallback tocustom_modelsplus awarnlogmake_provider_with_custom_modelstest helper and two#[tokio::test]casescrates/goose-providers/src/anthropic.rsAnthropicProvider::fetch_supported_models: same fallback change (thezaiandminimaxdeclarative providers are also affected)#[cfg(test)]module with matching test casesTesting
cargo test -p goose-providers --lib openai::tests::fetch_supported_models_falls_back_on_non_404_errorcargo test -p goose-providers --lib openai::tests::fetch_supported_models_returns_static_when_dynamic_falsecargo test -p goose-providers --lib anthropic::tests::fetch_supported_models_falls_back_on_non_404_errorcargo test -p goose-providers --lib anthropic::tests::fetch_supported_models_returns_static_when_dynamic_falseRelated Issues
Relates to #10074
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: