feat: Improve rate limit error handling with user-friendly messages #312
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds explicit rate-limit classification and retry-after extraction, exposes detection utilities, extends error types and user-facing messages, integrates rate-limit handling into the Claude server provider, and adds tests and changelog documenting the behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as User
participant Server as ClaudeProvider
participant Claude as Claude API
participant Utils as Error Utils
Client->>Server: send query
Server->>Claude: execute query
alt success
Claude-->>Server: response
Server-->>Client: forward response
else error / rate-limit
Claude-->>Server: error (e.g., HTTP 429 / "rate_limit")
Server->>Utils: classifyError(error)
activate Utils
Utils-->Utils: detect isRateLimit, extract retryAfter
Utils-->>Server: ErrorInfo(type, isRateLimit, retryAfter, message)
deactivate Utils
Server->>Server: log structured error (type, isRateLimit, retryAfter, original)
Server-->>Client: user-friendly error (rate-limit explanation, retry-after, concurrency tip)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @Shevanio, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience by transforming previously cryptic "exit code 1" errors, which occurred when hitting Claude API rate limits, into clear and actionable messages. It provides users with essential information such as the recommended wait time before retrying and practical advice on adjusting concurrency settings to prevent future rate limit issues, thereby enhancing both usability and debugging capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of improving error handling for API rate limits. The changes are well-structured, with clear separation of concerns between type definitions, utility functions, and the provider implementation. The new user-friendly error messages and comprehensive test coverage are great additions. I have a couple of suggestions to refactor some of the new logic to improve maintainability and reduce code duplication, but overall this is a very solid and valuable contribution.
| } catch (error) { | ||
| console.error('[ClaudeProvider] executeQuery() error during execution:', error); | ||
| throw error; | ||
| // Enhance error with user-friendly message and classification | ||
| const errorInfo = classifyError(error); | ||
| const userMessage = getUserFriendlyErrorMessage(error); | ||
|
|
||
| console.error('[ClaudeProvider] executeQuery() error during execution:', { | ||
| type: errorInfo.type, | ||
| message: errorInfo.message, | ||
| isRateLimit: errorInfo.isRateLimit, | ||
| retryAfter: errorInfo.retryAfter, | ||
| }); | ||
|
|
||
| // For rate limit errors, provide helpful guidance | ||
| if (errorInfo.isRateLimit) { | ||
| const retryAfter = errorInfo.retryAfter || 60; | ||
| const enhancedError = new Error( | ||
| `${userMessage}\n\nTip: If you're running multiple features in auto-mode, consider reducing concurrency (maxConcurrency setting) to avoid hitting rate limits.` | ||
| ); | ||
| // Preserve original error details | ||
| (enhancedError as any).originalError = error; | ||
| (enhancedError as any).type = 'rate_limit'; | ||
| (enhancedError as any).retryAfter = retryAfter; | ||
| throw enhancedError; | ||
| } | ||
|
|
||
| // For other errors, throw with user-friendly message | ||
| const enhancedError = new Error(userMessage); | ||
| (enhancedError as any).originalError = error; | ||
| (enhancedError as any).type = errorInfo.type; | ||
| throw enhancedError; | ||
| } |
There was a problem hiding this comment.
This catch block contains duplicated logic for creating and enhancing error objects. It can be refactored to be more DRY (Don't Repeat Yourself) by using a single path for error creation. This improves readability and makes future modifications easier.
} catch (error) {
// Enhance error with user-friendly message and classification
const errorInfo = classifyError(error);
const userMessage = getUserFriendlyErrorMessage(error);
console.error('[ClaudeProvider] executeQuery() error during execution:', {
type: errorInfo.type,
message: errorInfo.message,
isRateLimit: errorInfo.isRateLimit,
retryAfter: errorInfo.retryAfter,
});
const message = errorInfo.isRateLimit
? `${userMessage}\n\nTip: If you're running multiple features in auto-mode, consider reducing concurrency (maxConcurrency setting) to avoid hitting rate limits.`
: userMessage;
const enhancedError = new Error(message);
(enhancedError as any).originalError = error;
(enhancedError as any).type = errorInfo.type;
if (errorInfo.isRateLimit) {
(enhancedError as any).retryAfter = errorInfo.retryAfter;
}
throw enhancedError;
}
libs/utils/src/error-handler.ts
Outdated
| // Default retry-after for rate limit errors | ||
| if (isRateLimitError(error)) { | ||
| return 60; // Default to 60 seconds for rate limit errors | ||
| } | ||
|
|
There was a problem hiding this comment.
To improve separation of concerns, extractRetryAfter should only extract a value if present, while classifyError should be responsible for providing a default. This change moves the defaulting logic to the caller and avoids a redundant isRateLimitError check. The corresponding test in error-handler.test.ts will also need to be updated.
libs/utils/src/error-handler.ts
Outdated
| const isAuth = isAuthenticationError(message); | ||
| const isCancellation = isCancellationError(message); | ||
| const isRateLimit = isRateLimitError(error); | ||
| const retryAfter = isRateLimit ? extractRetryAfter(error) : undefined; |
There was a problem hiding this comment.
By moving the default value logic here, classifyError becomes the single source of truth for the retryAfter value. Using the nullish coalescing operator (??) provides a concise way to set the default of 60 seconds if extractRetryAfter doesn't find a value.
| const retryAfter = isRateLimit ? extractRetryAfter(error) : undefined; | |
| const retryAfter = isRateLimit ? (extractRetryAfter(error) ?? 60) : undefined; |
| it('should return default 60 for rate limit errors without explicit retry-after', () => { | ||
| const error = new Error('429 rate_limit_error'); | ||
| expect(extractRetryAfter(error)).toBe(60); | ||
| }); |
There was a problem hiding this comment.
This test for extractRetryAfter should be updated to assert that it returns undefined for rate limit errors without an explicit time. The logic for providing a default value is being moved to classifyError, and the tests for that function already cover the default case.
| it('should return default 60 for rate limit errors without explicit retry-after', () => { | |
| const error = new Error('429 rate_limit_error'); | |
| expect(extractRetryAfter(error)).toBe(60); | |
| }); | |
| it('should return undefined for rate limit errors without explicit retry-after', () => { | |
| const error = new Error('429 rate_limit_error'); | |
| expect(extractRetryAfter(error)).toBeUndefined(); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/server/src/providers/claude-provider.ts (1)
93-123: Suggest using a custom error class instead of type assertions.The error handling logic is sound, but using
(enhancedError as any)to attach properties is not type-safe. Consider creating a custom error class to properly type these enhanced errors.🔎 Proposed custom error class
Create a new file
apps/server/src/errors/enhanced-error.ts:export class EnhancedError extends Error { public readonly originalError: unknown; public readonly type: string; public readonly retryAfter?: number; constructor(message: string, originalError: unknown, type: string, retryAfter?: number) { super(message); this.name = 'EnhancedError'; this.originalError = originalError; this.type = type; this.retryAfter = retryAfter; } }Then update the error handling:
// For rate limit errors, provide helpful guidance if (errorInfo.isRateLimit) { const retryAfter = errorInfo.retryAfter || 60; - const enhancedError = new Error( + throw new EnhancedError( `${userMessage}\n\nTip: If you're running multiple features in auto-mode, consider reducing concurrency (maxConcurrency setting) to avoid hitting rate limits.`, + error, + 'rate_limit', + retryAfter ); - // Preserve original error details - (enhancedError as any).originalError = error; - (enhancedError as any).type = 'rate_limit'; - (enhancedError as any).retryAfter = retryAfter; - throw enhancedError; } // For other errors, throw with user-friendly message - const enhancedError = new Error(userMessage); - (enhancedError as any).originalError = error; - (enhancedError as any).type = errorInfo.type; - throw enhancedError; + throw new EnhancedError(userMessage, error, errorInfo.type);CHANGELOG_RATE_LIMIT_HANDLING.md (1)
7-9: Consider adding language specifiers to code blocks.For better markdown rendering, add language specifiers to the fenced code blocks. These can use
textfor plain output examples:🔎 Suggested improvements
Line 7:
-``` +```text Error: Claude Code process exited with code 1Line 63: ```diff -``` +```text [AutoMode] Feature touch-gesture-support failed: Error: Claude Code process exited with code 1Line 69: ```diff -``` +```text [AutoMode] Feature touch-gesture-support failed: Rate limit exceeded (429). Please wait 60 seconds before retrying. Tip: If you're running multiple features in auto-mode, consider reducing concurrency (maxConcurrency setting) to avoid hitting rate limits.</details> Based on static analysis hints. Also applies to: 63-65, 69-73 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 25c9259b502f20795aa841d2184012a14bb5f6ca and cca97a97e669fdbd7a491b45a8581a20af29818e. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `CHANGELOG_RATE_LIMIT_HANDLING.md` * `apps/server/src/providers/claude-provider.ts` * `libs/types/src/error.ts` * `libs/utils/src/error-handler.ts` * `libs/utils/src/index.ts` * `libs/utils/tests/error-handler.test.ts` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>**/*.{ts,tsx,js,jsx}</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Always import from shared packages (@automaker/*), never from old relative paths Files: - `libs/types/src/error.ts` - `libs/utils/src/index.ts` - `libs/utils/tests/error-handler.test.ts` - `apps/server/src/providers/claude-provider.ts` - `libs/utils/src/error-handler.ts` </details> <details> <summary>**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Use `resolveModelString()` from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names Files: - `libs/types/src/error.ts` - `libs/utils/src/index.ts` - `libs/utils/tests/error-handler.test.ts` - `apps/server/src/providers/claude-provider.ts` - `libs/utils/src/error-handler.ts` </details> <details> <summary>apps/server/src/**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket Files: - `apps/server/src/providers/claude-provider.ts` </details> </details><details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>libs/types/src/error.ts (1)</summary><blockquote> <details> <summary>libs/types/src/index.ts (1)</summary> * `ErrorType` (30-30) </details> </blockquote></details> <details> <summary>libs/utils/tests/error-handler.test.ts (2)</summary><blockquote> <details> <summary>libs/utils/src/error-handler.ts (4)</summary> * `isRateLimitError` (60-63) * `extractRetryAfter` (71-92) * `classifyError` (100-133) * `getUserFriendlyErrorMessage` (141-160) </details> <details> <summary>libs/utils/src/index.ts (4)</summary> * `isRateLimitError` (11-11) * `extractRetryAfter` (12-12) * `classifyError` (13-13) * `getUserFriendlyErrorMessage` (14-14) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>CHANGELOG_RATE_LIMIT_HANDLING.md</summary> 7-7: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 63-63: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 69-69: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (13)</summary><blockquote> <details> <summary>libs/types/src/error.ts (2)</summary><blockquote> `4-10`: **LGTM! Clean type extension for rate limiting.** The addition of `'rate_limit'` to the ErrorType union is well-integrated and follows the existing pattern. --- `15-24`: **LGTM! Backward-compatible ErrorInfo extension.** The new fields `isRateLimit` and optional `retryAfter` are well-designed: - Consistent with existing boolean flags (isAbort, isAuth, isCancellation) - Optional `retryAfter` allows flexibility when retry timing isn't available - Clear documentation for the retryAfter field </blockquote></details> <details> <summary>libs/utils/src/index.ts (1)</summary><blockquote> `7-16`: **LGTM! Clean public API extension.** The new exports `isRateLimitError` and `extractRetryAfter` are properly placed in the error handling section and follow the existing export pattern. </blockquote></details> <details> <summary>apps/server/src/providers/claude-provider.ts (1)</summary><blockquote> `10-10`: **LGTM! Correct import following coding guidelines.** The import from `@automaker/utils` follows the coding guidelines to always import from shared packages. </blockquote></details> <details> <summary>libs/utils/tests/error-handler.test.ts (4)</summary><blockquote> `106-130`: **LGTM! Comprehensive rate limit detection tests.** The test cases thoroughly cover: - HTTP 429 status codes - `rate_limit` keyword detection - String-based errors - Edge cases (null/undefined) - Non-rate-limit scenarios --- `132-161`: **LGTM! Thorough retry-after extraction tests.** The tests validate all extraction patterns: - `retry-after` and `retry_after` formats - `wait` format - Default 60-second fallback for rate limit errors - Non-rate-limit errors returning undefined - String error handling --- `177-194`: **LGTM! Well-designed error classification tests.** The tests validate: - Rate limit error classification with default retry-after - Explicit retry-after extraction - Error type prioritization (authentication > rate_limit > abort) - Multiple flag setting when errors match multiple categories The prioritization logic is well-tested and makes sense. Also applies to: 251-267 --- `323-337`: **LGTM! User-friendly message tests are thorough.** The tests validate: - Default 60-second message for rate limit errors - Custom retry-after duration in messages - Message format consistency </blockquote></details> <details> <summary>libs/utils/src/error-handler.ts (4)</summary><blockquote> `54-63`: **LGTM! Simple and effective rate limit detection.** The implementation correctly: - Handles both Error objects and non-Error values - Safely converts null/undefined to empty strings - Checks for both '429' HTTP status and 'rate_limit' keyword --- `65-92`: **LGTM! Comprehensive retry-after extraction.** The implementation handles multiple extraction patterns: 1. Retry-After header format (case-insensitive, flexible delimiters) 2. Wait duration patterns in error messages 3. Sensible 60-second default for rate limit errors The regex patterns are well-designed and the fallback logic is appropriate. --- `100-133`: **LGTM! Well-integrated rate limit classification.** The changes correctly: - Detect rate limit errors and extract retry metadata - Prioritize error types (authentication > rate_limit > abort > cancellation) - Include new fields in the ErrorInfo return value The prioritization logic makes sense—authentication errors should be surfaced before rate limits. --- `141-160`: **LGTM! Clear and actionable rate limit messaging.** The user-friendly message provides: - Clear problem statement ("Rate limit exceeded (429)") - Specific wait time when available - Generic guidance when retry-after is unavailable The conditional messaging ensures users always receive actionable information. </blockquote></details> <details> <summary>CHANGELOG_RATE_LIMIT_HANDLING.md (1)</summary><blockquote> `1-134`: **LGTM! Excellent documentation of the changes.** The changelog provides: - Clear problem statement and root cause analysis - Comprehensive solution overview covering all affected files - Before/after examples showing the user impact - Technical details including API changes - Future improvement suggestions - Testing instructions This documentation will be very helpful for understanding the feature and its implementation. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
✅ Refactoring AppliedThank you @gemini-code-assist for the excellent code review feedback! I've implemented all three suggested improvements: Changes Made:1. Reduced duplication in ClaudeProvider catch block (Medium Priority)
Before: if (errorInfo.isRateLimit) {
const enhancedError = new Error(...);
(enhancedError as any).originalError = error;
(enhancedError as any).type = 'rate_limit';
throw enhancedError;
}
const enhancedError = new Error(...);
(enhancedError as any).originalError = error;
throw enhancedError;After: const message = errorInfo.isRateLimit ? `${userMessage}\n\nTip...` : userMessage;
const enhancedError = new Error(message);
(enhancedError as any).originalError = error;
(enhancedError as any).type = errorInfo.type;
if (errorInfo.isRateLimit) {
(enhancedError as any).retryAfter = errorInfo.retryAfter;
}
throw enhancedError;2. Better separation of concerns (Medium Priority)
Change: // In classifyError()
const retryAfter = isRateLimit ? (extractRetryAfter(error) ?? 60) : undefined;3. Updated test to match new behavior (Medium Priority)
Verification:✅ All 162 tests passing Commits:
Ready for final review! 🚀 |
|
Thank you, @Shevanio, for the detailed update and for incorporating the suggested improvements! I appreciate your thoroughness in addressing the feedback. The refactoring to reduce duplication in the It's great to see that all tests are passing, TypeScript compiles without errors, and the code quality has improved without introducing any functional regressions. This demonstrates a strong commitment to robust and well-structured code. Ready for the final review! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/utils/tests/error-handler.test.ts (1)
177-194: Well-tested rate limit classification and priority handling.The tests correctly verify:
- Rate limit detection and classification
- Default retry-after (60s) when not explicitly provided
- Explicit retry-after extraction from error messages
- Priority chain (authentication > rate_limit > abort > cancellation)
- Multiple classification flags can be true simultaneously
Optional: Consider explicitly testing undefined retryAfter for non-rate-limit errors
While the behavior is implicitly tested, you could add an explicit assertion to the non-rate-limit tests to document this behavior more clearly:
it('should classify execution errors (regular Error)', () => { const error = new Error('Something went wrong'); const result = classifyError(error); expect(result.type).toBe('execution'); expect(result.isAuth).toBe(false); expect(result.isAbort).toBe(false); expect(result.isCancellation).toBe(false); expect(result.retryAfter).toBeUndefined(); // Optional: explicit assertion });Also applies to: 251-267
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/providers/claude-provider.tslibs/utils/src/error-handler.tslibs/utils/tests/error-handler.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/providers/claude-provider.ts
- libs/utils/src/error-handler.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
libs/utils/tests/error-handler.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
libs/utils/tests/error-handler.test.ts
🧬 Code graph analysis (1)
libs/utils/tests/error-handler.test.ts (1)
libs/utils/src/error-handler.ts (4)
isRateLimitError(60-63)extractRetryAfter(71-87)classifyError(95-128)getUserFriendlyErrorMessage(136-155)
🔇 Additional comments (3)
libs/utils/tests/error-handler.test.ts (3)
6-7: LGTM! New utility imports added correctly.The new rate limit detection and retry-after extraction utilities are properly imported for testing.
106-161: Excellent test coverage for rate limit utilities.The test suites for
isRateLimitErrorandextractRetryAfterare comprehensive:
- Cover multiple detection patterns (429, rate_limit)
- Test various retry-after formats (retry-after, retry_after, wait)
- Include edge cases (null, undefined, string errors, missing values)
- Correctly verify that
extractRetryAfterreturnsundefinedwhen no explicit retry-after is present (line 148-151)
323-337: LGTM! User-friendly rate limit messaging is well-tested.The tests verify that rate limit error messages:
- Clearly communicate the rate limit condition
- Include the retry-after duration (both default 60s and custom values)
- Provide actionable guidance to users
- Add rate_limit error type to ErrorInfo classification - Implement isRateLimitError() and extractRetryAfter() utilities - Enhance ClaudeProvider error handling with actionable messages - Add comprehensive test coverage (8 new tests, 162 total passing) **Problem:** When hitting API rate limits, users saw cryptic 'exit code 1' errors with no explanation or guidance on how to resolve the issue. **Solution:** - Detect rate limit errors (429) and extract retry-after duration - Provide clear, user-friendly error messages with: * Explanation of what went wrong * How long to wait before retrying * Actionable tip to reduce concurrency in auto-mode - Preserve original error details for debugging **Changes:** - libs/types: Add 'rate_limit' type and retryAfter field to ErrorInfo - libs/utils: Add rate limit detection and extraction logic - apps/server: Enhance ClaudeProvider with better error messages - tests: Add 8 new test cases covering rate limit scenarios **Benefits:** ✅ Clear communication - users understand the problem ✅ Actionable guidance - users know how to fix it ✅ Better debugging - original errors preserved ✅ Type safety - proper TypeScript typing ✅ Comprehensive testing - all edge cases covered See CHANGELOG_RATE_LIMIT_HANDLING.md for detailed documentation.
Address code review feedback from Gemini Code Assist: 1. Reduce duplication in ClaudeProvider catch block - Consolidate error creation logic into single path - Use conditional message building instead of duplicate blocks - Improves maintainability and follows DRY principle 2. Better separation of concerns in error utilities - Move default retry-after (60s) logic from extractRetryAfter to classifyError - extractRetryAfter now only extracts explicit values - classifyError provides default using nullish coalescing (?? 60) - Clearer single responsibility for each function 3. Update test to match new behavior - extractRetryAfter now returns undefined for rate limits without explicit value - Default value is tested in classifyError tests instead All 162 tests still passing ✅ Builds successfully with no TypeScript errors ✅
Shevanio
force-pushed
the
feat/improve-rate-limit-error-handling
branch
from
25acfff to
19aa86c
Compare
✅ Conflicts ResolvedSuccessfully rebased on latest Changes:
Key merge points:
Verification:✅ All packages build successfully Branch status: Ready for final review and merge 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG_RATE_LIMIT_HANDLING.mdapps/server/src/providers/claude-provider.tslibs/types/src/error.tslibs/utils/src/error-handler.tslibs/utils/src/index.tslibs/utils/tests/error-handler.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/types/src/error.ts
- apps/server/src/providers/claude-provider.ts
- libs/utils/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
libs/utils/src/error-handler.tslibs/utils/tests/error-handler.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
libs/utils/src/error-handler.tslibs/utils/tests/error-handler.test.ts
🧬 Code graph analysis (2)
libs/utils/src/error-handler.ts (3)
libs/utils/src/index.ts (2)
isRateLimitError(11-11)extractRetryAfter(12-12)libs/types/src/error.ts (1)
ErrorType(4-10)libs/types/src/index.ts (1)
ErrorType(34-34)
libs/utils/tests/error-handler.test.ts (1)
libs/utils/src/error-handler.ts (4)
isRateLimitError(60-63)extractRetryAfter(71-87)classifyError(95-128)getUserFriendlyErrorMessage(136-155)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG_RATE_LIMIT_HANDLING.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
libs/utils/src/error-handler.ts (4)
54-63: LGTM! Clean and pragmatic rate limit detection.The string-based detection is simple and effective for the common rate limit patterns (429 status code and rate_limit keywords). The implementation correctly handles both Error objects and non-Error values.
65-87: Excellent separation of concerns.The function correctly extracts explicit retry-after values from multiple formats without imposing defaults. This aligns with the refactoring described in the PR objectives and past review feedback.
100-101: Well-integrated rate limit classification.The implementation cleanly integrates rate limit detection into the error classification flow. The 60-second default via nullish coalescing is concise, and the classification priority (auth > rate_limit > abort) is appropriate for user experience.
Also applies to: 106-107, 124-125
147-152: Clear and actionable user messaging.The rate limit message appropriately balances technical detail (429 status) with actionable guidance (wait duration or reduce concurrency). The conditional message construction handles both explicit and default retry-after scenarios well.
libs/utils/tests/error-handler.test.ts (3)
106-130: Comprehensive test coverage for rate limit detection.The tests thoroughly validate
isRateLimitErroracross Error objects, strings, and edge cases (null/undefined). Good coverage of both positive and negative scenarios.
132-161: Tests correctly validate the refactored separation of concerns.The tests properly verify that
extractRetryAfterreturns undefined when no explicit value is present (line 148-151), and thatclassifyErrorapplies the 60-second default (line 184). This validates the refactoring described in the PR objectives.Also applies to: 177-194
251-267: Excellent priority and integration testing.The priority tests (lines 251-267) ensure correct classification when multiple error conditions are present, and the user message tests (lines 323-337) validate end-to-end functionality with both default and custom retry-after values.
Also applies to: 323-337
| ``` | ||
| Error: Claude Code process exited with code 1 | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
For better readability and syntax highlighting, specify languages for code blocks. Use text or plaintext for error output examples.
🔎 Proposed fix
-```
+```text
Error: Claude Code process exited with code 1
```diff
-```
+```text
[AutoMode] Feature touch-gesture-support failed: Error: Claude Code process exited with code 1
```diff
-```
+```text
[AutoMode] Feature touch-gesture-support failed: Rate limit exceeded (429). Please wait 60 seconds before retrying.
Tip: If you're running multiple features in auto-mode, consider reducing concurrency (maxConcurrency setting) to avoid hitting rate limits.
</details>
Also applies to: 63-65, 69-73
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In CHANGELOG_RATE_LIMIT_HANDLING.md around lines 7-9 (and similarly for blocks
at 63-65 and 69-73), the fenced code blocks containing error/output examples
lack language specifiers; update each triple-backtick fence to include a
language tag (use text or plaintext) so the blocks read text ... (or
plaintext ... ), ensuring all shown error messages and tips are enclosed
by the updated fenced blocks.
</details>
<!-- fingerprinting:phantom:poseidon:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/server/tests/unit/providers/claude-provider.test.ts (2)
233-261: Add test coverage for rate limit error handling.The PR enhances ClaudeProvider to detect rate limits and include retry-after metadata, but this test file only validates the non-rate-limit case. Consider adding a test that:
- Mocks an HTTP 429 error from the SDK
- Verifies
isRateLimit: truein the logged error object- Validates the presence and value of the
retryAfterfield- Confirms the user-friendly error message includes retry-after guidance
Example test case structure
it('should handle rate limit errors (429) with retry-after metadata', async () => { const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const rateLimitError = Object.assign( new Error('Rate limit exceeded'), { status: 429, headers: { 'retry-after': '30' } } ); vi.mocked(sdk.query).mockReturnValue( (async function* () { throw rateLimitError; })() ); const generator = provider.executeQuery({ prompt: 'Test', cwd: '/test', }); await expect(collectAsyncGenerator(generator)).rejects.toThrow(); const errorCall = consoleErrorSpy.mock.calls[0]; expect(errorCall[1]).toMatchObject({ type: 'rate_limit', isRateLimit: true, retryAfter: 30, stack: expect.any(String), }); consoleErrorSpy.mockRestore(); });
254-254: Consider more specific type validation.Using
expect.any(String)for thetypefield doesn't verify correct error classification. For a generic SDK execution error, validate the expected type value (e.g.,'unknown'or'network') to ensureclassifyError()is working as intended.Suggested change
expect(errorCall[1]).toMatchObject({ - type: expect.any(String), + type: 'unknown', // or the appropriate expected type message: 'SDK execution failed', isRateLimit: false, stack: expect.stringContaining('Error: SDK execution failed'), });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/tests/unit/providers/claude-provider.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/tests/unit/providers/claude-provider.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/tests/unit/providers/claude-provider.test.ts
3 participants
Summary
Improves error handling when hitting Claude API rate limits (HTTP 429), providing clear, actionable error messages instead of cryptic "exit code 1" errors.
Problem
When running multiple features concurrently in auto-mode, users would hit API rate limits and see:
This provided no information about:
Solution
Enhanced error handling across the stack:
1. Error Classification (libs/utils)
'rate_limit'error typeisRateLimitError()to detect 429 errorsextractRetryAfter()to parse retry durationclassifyError()to include rate limit metadatagetUserFriendlyErrorMessage()with clear guidance2. Claude Provider (apps/server)
executeQuery()3. Test Coverage
User-Facing Changes
Before
After
Files Changed
libs/types/src/error.ts- Added rate_limit type and retryAfter fieldlibs/utils/src/error-handler.ts- Added detection and extraction logiclibs/utils/src/index.ts- Exported new utility functionslibs/utils/tests/error-handler.test.ts- Added 8 new test casesapps/server/src/providers/claude-provider.ts- Enhanced error handlingCHANGELOG_RATE_LIMIT_HANDLING.md- Detailed documentationTotal: +341 lines added, -3 lines removed
Benefits
✅ Clear communication - Users understand exactly what went wrong
✅ Actionable guidance - Users know how long to wait and how to prevent future errors
✅ Better debugging - Original error details preserved for investigation
✅ Type safety - Proper TypeScript typing for new fields
✅ Comprehensive testing - All edge cases covered with automated tests
Testing
All tests pass:
npm run test:packages # 162 tests passing ✅Documentation
See
CHANGELOG_RATE_LIMIT_HANDLING.mdfor:Checklist
Related Issues
Fixes the cryptic "exit code 1" error when hitting Claude API rate limits.
Future Enhancements
This PR lays the foundation for:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.