-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: suppress verbose error for Claude Code 5-hour usage limit #8134
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -85,6 +85,20 @@ export class ClaudeCodeHandler extends BaseProvider implements ApiHandler { | |||||||||||
| throw new Error(content.text) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Check if this is a 5-hour usage limit error (429 status) | ||||||||||||
| // These errors should be handled gracefully without showing verbose details | ||||||||||||
| if ( | ||||||||||||
| content.text.includes("429") && | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this detection logic robust enough? I'm checking for "429" as a string and then looking for keywords, but what if a legitimate non-error response happens to contain these terms? Should we validate the error structure more strictly first (e.g., checking that we're actually in an error object)? |
||||||||||||
| (error.error?.message?.toLowerCase().includes("usage limit") || | ||||||||||||
| error.error?.message?.toLowerCase().includes("rate limit") || | ||||||||||||
| error.error?.message?.toLowerCase().includes("5-hour") || | ||||||||||||
| error.error?.message?.toLowerCase().includes("five hour")) | ||||||||||||
| ) { | ||||||||||||
| // Don't throw the verbose error - let the UI handle it with a concise message | ||||||||||||
| // The UI already shows a grey notice for this case | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future maintainers, it might be worth documenting where this UI handling occurs. Could we add a more specific reference, like: "The UI shows a grey notice via [specific component/file]"? |
||||||||||||
| return | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a debug log here? Silently suppressing the error works for the UI, but it might be helpful for developers to track when rate limits are being hit. Something like:
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (error.error.message.includes("Invalid model name")) { | ||||||||||||
| throw new Error( | ||||||||||||
| content.text + `\n\n${t("common:errors.claudeCode.apiKeyModelPlanMismatch")}`, | ||||||||||||
|
|
||||||||||||
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.
Great test coverage! Consider adding an edge case test where the error message contains "429" and "rate limit" but isn't actually a rate limit error (e.g., if these terms appear in user-generated content). This would help ensure we're not over-suppressing errors.