Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #2545 by enhancing model configuration error messages to display available models when a provider returns a "model not supported" error. When a user configures an unsupported model, the enhanced error message will now include a list of recommended models for the provider, reducing friction during initial setup.
Changes:
- Added an
enhance_model_errorfunction that detects model-related errors and appends a list of available models to the error message - Modified the error handling in
reply_message_streamto callenhance_model_errorbefore returning error streams
| }; | ||
|
|
||
| let msg_lower = msg.to_lowercase(); | ||
| if !msg_lower.contains("404") || !msg_lower.contains("model") { |
There was a problem hiding this comment.
This checks for "404" but the linked issue #2545 describes "400 error" scenarios when models are not supported. Based on the issue description and the fact that unsupported model errors typically return 400 Bad Request (not 404 Not Found), this should check for "400" instead of "404".
| if !msg_lower.contains("404") || !msg_lower.contains("model") { | |
| if !msg_lower.contains("400") || !msg_lower.contains("model") { |
There was a problem hiding this comment.
to copilot's point, is it always a 404?
There was a problem hiding this comment.
no idea, but it's a good point
| async fn enhance_model_error(error: ProviderError, provider: &Arc<dyn Provider>) -> ProviderError { | ||
| let ProviderError::RequestFailed(ref msg) = error else { | ||
| return error; | ||
| }; | ||
|
|
||
| let msg_lower = msg.to_lowercase(); | ||
| if !msg_lower.contains("404") || !msg_lower.contains("model") { | ||
| return error; | ||
| } | ||
|
|
||
| let Ok(Some(models)) = provider.fetch_recommended_models().await else { | ||
| return error; | ||
| }; | ||
|
|
||
| ProviderError::RequestFailed(format!( | ||
| "{}. Available models for this provider: {}", | ||
| msg, | ||
| models.join(", ") | ||
| )) | ||
| } |
There was a problem hiding this comment.
The new enhance_model_error function lacks test coverage. Given that other functions in this file have tests, consider adding tests to verify this function correctly identifies model errors (with both "400" and "model" in the message), successfully fetches recommended models, and properly formats the enhanced error message.
| }; | ||
|
|
||
| let msg_lower = msg.to_lowercase(); | ||
| if !msg_lower.contains("404") || !msg_lower.contains("model") { |
There was a problem hiding this comment.
Potential future enhancement: if we get good enough at identifying the kinds of errors that mean a model change is needed, should we open the model selection modal for the user in addition to showing a message?
There was a problem hiding this comment.
yeah, maybe at some point. we can't really here and as a desktop user you shouldn't get into this - this is mostly for the CLI where specify a model that doesn't exist.
| }; | ||
|
|
||
| let msg_lower = msg.to_lowercase(); | ||
| if !msg_lower.contains("404") || !msg_lower.contains("model") { |
There was a problem hiding this comment.
to copilot's point, is it always a 404?
|
LGTM for a merge |
* 'main' of github.com:block/goose: refactor: move disable_session_naming into AgentConfig (#7062) Add global config switch to disable automatic session naming (#7052) docs: add blog post - 8 Things You Didn't Know About Code Mode (#7059) fix: ensure animated elements are visible when prefers-reduced-motion is enabled (#7047) Show recommended model on failture (#7040) feat(ui): add session content search via API (#7050) docs: fix img url (#7053) Desktop UI for deleting custom providers (#7042) Add blog post: How I Used RPI to Build an OpenClaw Alternative (#7051)
Co-authored-by: Douwe Osinga <douwe@squareup.com>
4 participants
Summary
Fixes: #2545