-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: improve server error messages to include HTTP status code #5532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes #5528 Previously, when a server error (5xx) occurred without a response body, the error message would display 'Server error: None', which was not helpful for debugging. This change improves error messages by: - Including the HTTP status code (e.g., 'HTTP 500') - Providing a clear message when no response body is received - Maintaining detailed error info when a response body is present Example error messages: - Before: 'Server error: None' - After: 'HTTP 500: No response body received from server' This affects all OpenAI-compatible providers (OpenAI, Claude, Google, Ollama, Azure, Databricks, GitHub Copilot, etc.) and Google-compatible endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves error messages for server errors by including HTTP status codes and providing clearer messages when no response body is received. The changes standardize error formatting across HTTP server error handling.
Key changes:
- Enhanced server error messages to include HTTP status codes
- Added explicit handling for cases when no response body is received from the server
- Updated test expectations to match the new error message format
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…error: None' When providers return 500 errors with null response bodies, the error formatting was displaying 'Server error: null' which appeared as 'Server error: None' to users. This was confusing and unhelpful. This fix treats Value::Null the same as None, displaying the more informative message 'HTTP 500: No response body received from server' instead. Fixes #5528
crates/goose/src/providers/utils.rs
Outdated
| .any(|phrase| text_lower.contains(phrase)) | ||
| } | ||
|
|
||
| fn format_server_error_message(status_code: u16, payload: Option<&Value>) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't status_code by of ype StatusCode here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, then I need to add .as_u16() inside the function. but guess that's negligible. will fix...
Use the format_server_error_message helper function in test cases instead of hardcoding error message strings. This improves maintainability by ensuring error message format changes only need to be updated in one place. Addresses code review feedback from DOsinga.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tead of u16 - Changed parameter type from u16 to StatusCode for better type safety - Updated all call sites to pass StatusCode directly - Updated test expectations to use the helper function instead of hardcoded strings - Improves maintainability by keeping error format in one place Addresses code review feedback from DOsinga on PR #5532.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…est-and-fix * 'main' of github.com:block/goose: improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) feat: add --output-format json flag to goose run command (#5525)
* main: (85 commits) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) feat: add --output-format json flag to goose run command (#5525) Sessions required (#5548) feat: add grouped extension loading notification (#5529) we should run this on main and also test open models at least via ope… (#5556) info: print location of sessions.db via goose info (#5557) chore: remove yarn usage from documentation (#5555) cli: adjust default theme to address #1905 (#5552) Manual compaction counting fix + cli cleanup (#5480) chore(deps): bump prismjs and react-syntax-highlighter in /ui/desktop (#5549) fix: remove qwen3-coder from provider/mcp smoke tests (#5551) fix: do not build unsigned desktop app bundles on every PR in ci. add manual option. (#5550) fix: update Husky prepare script to v9 format (#5522) ...
* main: (54 commits) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) feat: add --output-format json flag to goose run command (#5525) Sessions required (#5548) feat: add grouped extension loading notification (#5529) we should run this on main and also test open models at least via ope… (#5556) info: print location of sessions.db via goose info (#5557) chore: remove yarn usage from documentation (#5555) cli: adjust default theme to address #1905 (#5552) Manual compaction counting fix + cli cleanup (#5480) chore(deps): bump prismjs and react-syntax-highlighter in /ui/desktop (#5549) fix: remove qwen3-coder from provider/mcp smoke tests (#5551) fix: do not build unsigned desktop app bundles on every PR in ci. add manual option. (#5550) ...
* main: (41 commits) Add pending extension indicator to extension panel (#5493) Add environment subsition for auth blocks (#5439) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) ...
* main: (53 commits) acp: ToolCallLocations and working cancellation (#5588) feat(providers): add Mistral AI provider (#5009) Listen for ctrl-c during provider request (#5585) Also accept null as description, not just missing (#5589) Document missing recipe param types (#5584) docs: description required for "Add Extension" in cli (#5573) fix: Add schema-aware numeric coercion for MCP tool arguments (#5478) Add uv for uvx in Justfile (#5581) Keep llm logs in place (#5577) bump to 1.12.0 (#5580) automate more of the release process (#5409) add clippy warning for string_slice (#5422) improve linux tray icon support (#5425) feat: log rotation (#5561) use app.isPackaged instead of checking for node env development (#5465) disable RPM build-ID generation to prevent package conflicts (#5563) Add Diagnostics Info to Q&A and Bug Report Templates (#5565) fix: improve server error messages to include HTTP status code (#5532) improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300) fix: unblock acp via databricks (#5562) ...
…#5532) Co-authored-by: Copilot <[email protected]> Signed-off-by: fbalicchia <[email protected]>
…#5532) Co-authored-by: Copilot <[email protected]> Signed-off-by: Blair Allan <[email protected]>
Summary
Previously, when a server error (5xx) occurred without a response body, the error message would display 'Server error: None', which was not helpful for debugging.
This change improves error messages by:
Example error messages:
This affects all OpenAI-compatible providers (OpenAI, Claude, Google, Ollama, Azure, Databricks, GitHub Copilot, etc.) and Google-compatible endpoints.
Type of Change
Testing
ran all the tests in the utils module, and updated them to match new response
Related Issues
related to #5528. this gives a better message but doesn't solve the problem of frequently running into 500 errors.