-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve error handling for OpenAI Compatible API providers #7487
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
- Add detailed error messages with context-specific recovery suggestions - Handle various error types (401, 404, 429, connection, timeout, etc.) - Provide actionable guidance for users to resolve issues - Add comprehensive tests for error handling scenarios - Update existing provider tests to match new error format Fixes #7486
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| let suggestions: string[] = [] | ||
|
|
||
| // Check for common error patterns | ||
| if (error?.status === 401 || errorMessage.includes("401") || errorMessage.includes("Unauthorized")) { |
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.
The error detection logic here could be more robust. Currently, it relies on string matching (e.g., errorMessage.includes("401")) which could lead to false positives. For example, if an error message contains "401k retirement plan", it would incorrectly trigger authentication error suggestions.
Consider checking the status code first before falling back to string matching:
| if (error?.status === 401 || errorMessage.includes("401") || errorMessage.includes("Unauthorized")) { | |
| if (error?.status === 401 || (!error?.status && (errorMessage.includes("401") || errorMessage.includes("Unauthorized")))) { |
| suggestions.push("• You've hit the rate limit for this API") | ||
| suggestions.push("• Wait a moment before retrying") | ||
| suggestions.push("• Consider upgrading your API plan for higher limits") | ||
| } else if (error?.status === 500 || error?.status === 502 || error?.status === 503) { |
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.
Consider handling additional common error scenarios:
- 400 Bad Request (malformed request, invalid parameters)
- 403 Forbidden (different from 401 - has permission but action is forbidden)
- 422 Unprocessable Entity (common in OpenAI-compatible APIs for invalid model parameters)
These are frequently encountered and would benefit from specific guidance.
| suggestions.push("• Try using a different model") | ||
| } | ||
|
|
||
| // Add general suggestions if no specific ones were added |
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.
Is this intentional? If an error matches multiple conditions (e.g., has status 401 AND message includes "Unauthorized"), the suggestions could be duplicated since we're using if-else. This actually prevents duplicates, but it means some errors might miss additional relevant suggestions. Consider collecting all applicable suggestions using a Set to allow multiple matches while avoiding duplicates.
| expect(enhancedError.message).toContain("Check your provider's status page") | ||
| } | ||
| }) | ||
| }) |
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.
Consider adding test cases for edge scenarios:
- Errors with both status codes and matching message strings (to verify no duplicate suggestions)
- Null/undefined error objects
- Errors with circular references in properties
These edge cases could help ensure the error handler is robust against unexpected inputs.
| } | ||
| } | ||
|
|
||
| /** |
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.
The method could benefit from JSDoc documentation with examples:
| /** | |
| /** | |
| * Enhances error messages with helpful guidance for OpenAI Compatible API issues | |
| * @example | |
| * // Input: Error { message: "401 Unauthorized", status: 401 } | |
| * // Output: Error { message: "OpenAI Compatible API Error...\n\nSuggestions..." } | |
| */ |
|
Closing, see #7486 (comment) |
Fixes #7486
Problem
Users encountering errors with OpenAI Compatible API providers (like qwen-coder and chatGLM4.5) were receiving vague "Roo encountered a problem" messages without actionable guidance for recovery.
Solution
Enhanced the error handling in
BaseOpenAiCompatibleProviderto provide detailed, context-specific error messages with recovery suggestions.Changes
enhanceErrorMessagemethod to analyze errors and provide helpful suggestionsTesting
Example Error Messages
Before
After
This provides users with clear, actionable steps to resolve their issues instead of leaving them confused about what went wrong.
Important
Enhances error handling for OpenAI Compatible API providers by adding detailed error messages and recovery suggestions in
BaseOpenAiCompatibleProvider.enhanceErrorMessagemethod inBaseOpenAiCompatibleProviderto provide detailed error messages and recovery suggestions.base-openai-compatible-error-handling.spec.tsfor error handling scenarios.chutes.spec.ts,featherless.spec.ts,fireworks.spec.ts,groq.spec.ts,io-intelligence.spec.ts,roo.spec.ts,sambanova.spec.ts, andzai.spec.tsto match new error format.completePromptmethod inbase-openai-compatible-provider.ts.This description was created by
for f577ee2. You can customize this summary. It will automatically update as commits are pushed.