test: add unit tests for recovery-handler module#244
test: add unit tests for recovery-handler module#244nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
Add 59 tests covering RecoveryHandler class: - Recovery strategy and result enums - Error classification: transient, state, config, dependency, fatal - Strategy selection based on error type, attempt count, criticality - Epic failure handling with attempt tracking and events - Strategy execution: retry, rollback, skip, escalate, recovery workflow - Escalation report generation with suggestions - Attempt count tracking, retry limits, reset - Logging with epic filtering and orchestrator forwarding
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit tests for the RecoveryHandler class, which manages automatic error recovery for the orchestration pipeline. The implementation (720 lines) previously had 0% test coverage, and this PR adds 59 tests covering all major functionality including error classification, strategy selection, recovery execution, and logging.
Changes:
- Added 59 unit tests for RecoveryHandler covering exports, constructor, error classification, strategy selection, failure handling, recovery execution, escalation, suggestions, attempt tracking, logging, and state management
- Mocked
fs-extrafor file system operations during escalation report persistence - Tests validate recovery strategy escalation flow: retry → rollback → escalate
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * Tests the RecoveryHandler class that manages automatic error recovery | ||
| * for the orchestration pipeline with multiple strategies. | ||
| */ |
There was a problem hiding this comment.
Consider adding 'use strict'; directive at the top of the file. While not universally present, many test files in the codebase include it (e.g., context-manager-handoff.test.js, brownfield-handler.test.js, terminal-spawner.test.js). This helps catch common JavaScript errors and enforces stricter parsing.
| */ | |
| */ | |
| 'use strict'; |
| describe('strategy selection', () => { | ||
| test('escalates after max retries with autoEscalate', () => { | ||
| handler.attempts[3] = [{}, {}, {}]; // 3 attempts = maxRetries | ||
| const strategy = handler._selectRecoveryStrategy(3, 'error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
||
| test('does not escalate after max retries without autoEscalate', () => { | ||
| handler.autoEscalate = false; | ||
| handler.attempts[3] = [{}, {}, {}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'error', { stuck: false }); | ||
| expect(strategy).not.toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
||
| test('rollback on circular approach detection', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'error', { | ||
| stuck: true, reason: 'circular approach detected', | ||
| }); | ||
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | ||
| }); | ||
|
|
||
| test('retry on transient error', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'Connection timeout', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.RETRY_SAME_APPROACH); | ||
| }); | ||
|
|
||
| test('rollback on state error', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'State corrupted', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | ||
| }); | ||
|
|
||
| test('skip on config error for non-critical epic', () => { | ||
| handler.attempts[6] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(6, 'Config missing', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.SKIP_PHASE); | ||
| }); | ||
|
|
||
| test('escalates on config error for critical epic', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'Config missing', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
||
| test('trigger recovery workflow on dependency error', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'Cannot find module', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.TRIGGER_RECOVERY_WORKFLOW); | ||
| }); | ||
|
|
||
| test('escalates on fatal error', () => { | ||
| handler.attempts[3] = [{}]; | ||
| const strategy = handler._selectRecoveryStrategy(3, 'Fatal error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
||
| test('retry on unknown error first attempt', () => { | ||
| handler.attempts[3] = [{}]; // 1 attempt | ||
| const strategy = handler._selectRecoveryStrategy(3, 'weird error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.RETRY_SAME_APPROACH); | ||
| }); | ||
|
|
||
| test('rollback on unknown error after 2 attempts', () => { | ||
| handler.attempts[3] = [{}, {}]; // 2 attempts | ||
| const strategy = handler._selectRecoveryStrategy(3, 'weird error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for the consecutive failures escalation path. In the implementation (recovery-handler.js lines 258-266), there's a branch that escalates when stuckResult.stuck && stuckResult.context?.consecutiveFailures >= this.maxRetries && this.autoEscalate. Consider adding a test case that verifies this behavior.
| const strategy = handler._selectRecoveryStrategy(3, 'Config missing', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing test coverage for configuration errors on critical epics when autoEscalate is false. The implementation (recovery-handler.js lines 285-288) shows that when autoEscalate is false and there's a config error on a critical epic, it should return ROLLBACK_AND_RETRY instead of ESCALATE_TO_HUMAN. Consider adding a test case for this scenario.
| test('rollbacks on config error for critical epic when autoEscalate is false', () => { | |
| const noAutoEscalateHandler = new RecoveryHandler({ | |
| projectRoot: '/project', | |
| storyId: 'story-1', | |
| maxRetries: 3, | |
| autoEscalate: false, | |
| }); | |
| noAutoEscalateHandler.attempts[3] = [{}]; | |
| const strategy = noAutoEscalateHandler._selectRecoveryStrategy( | |
| 3, | |
| 'Config missing', | |
| { stuck: false }, | |
| ); | |
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | |
| }); |
| const strategy = handler._selectRecoveryStrategy(3, 'Fatal error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ESCALATE_TO_HUMAN); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing test coverage for fatal errors when autoEscalate is false. The implementation (recovery-handler.js lines 296-298) shows that when autoEscalate is false and there's a fatal error, it should return ROLLBACK_AND_RETRY instead of ESCALATE_TO_HUMAN. Consider adding a test case for this scenario.
| test('rollbacks on fatal error when autoEscalate is disabled', () => { | |
| handler.autoEscalate = false; | |
| handler.attempts[3] = [{}]; | |
| const strategy = handler._selectRecoveryStrategy(3, 'Fatal error', { stuck: false }); | |
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | |
| }); |
| handler.attempts[3] = [{}, {}]; // 2 attempts | ||
| const strategy = handler._selectRecoveryStrategy(3, 'weird error', { stuck: false }); | ||
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for unknown errors when attemptCount equals maxRetries and autoEscalate is false. The implementation (recovery-handler.js lines 309-312) shows that in this case, it should return ROLLBACK_AND_RETRY instead of ESCALATE_TO_HUMAN. Consider adding a test case for this scenario.
| }); | |
| }); | |
| test('does not escalate on unknown error at max retries when autoEscalate is false', () => { | |
| handler = new RecoveryHandler({ | |
| projectRoot: '/project', | |
| storyId: 'story-1', | |
| maxRetries: 3, | |
| autoEscalate: false, | |
| }); | |
| handler.attempts[3] = [{}, {}, {}]; // 3 attempts == maxRetries | |
| const strategy = handler._selectRecoveryStrategy(3, 'weird error', { stuck: false }); | |
| expect(strategy).toBe(RecoveryStrategy.ROLLBACK_AND_RETRY); | |
| }); |
|
Consolidated into #426 |
Summary
Closes #243
Add 59 unit tests for the
RecoveryHandlerclass in.aios-core/core/orchestration/recovery-handler.js(720 lines, previously 0% coverage).Test Coverage
Testing Approach
fs-extrafor escalation report persistencehandler.on('recoveryAttempt', ...)All 59 tests passing.