test: add unit tests for RecoveryHandler module#534
test: add unit tests for RecoveryHandler module#534nikolasdehor wants to merge 3 commits intoSynkraAI:mainfrom
Conversation
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@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. |
WalkthroughAdds two comprehensive Jest test suites for RecoveryHandler and ContextManager under tests/core/orchestration; all changes are tests and mocks (fs-extra and orchestration helpers); no production code or API surface changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/orchestration/recovery-handler.test.js (1)
58-1251: Consider splitting this spec into focused filesThis single test file is very large, which makes failures and future edits harder to navigate. Splitting by concern (e.g., enums/constructor, strategy selection, rollback/escalation, logging/history, integrated flow) would keep the same coverage with better maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/recovery-handler.test.js` around lines 58 - 1251, Large single spec; split into focused test files to improve maintainability. Create separate spec files grouping related describes (e.g., enums+constructor, strategy selection for _selectRecoveryStrategy, rollback/escalation for _executeRollback and _escalateToHuman/_saveEscalationReport, logging/history for _log/getLogs/getEpicLogs/getAttemptHistory, and integrated flows for handleEpicFailure/end-to-end), move the matching describe blocks for RecoveryHandler/RecoveryStrategy/RecoveryResult/_selectRecoveryStrategy/_executeRollback/_escalateToHuman/_generateSuggestions/handleEpicFailure/_executeRecoveryStrategy/_triggerRecoveryWorkflow into those files, extract shared setup (criarHandler, jest.clearAllMocks(), mocked fs/path/orchestrator helpers) into a common test helper module and import it in each new spec, ensure each new file still attaches the beforeEach that creates handler and restores mocks and that all referenced symbols (RecoveryHandler, RecoveryStrategy, handler._selectRecoveryStrategy, handler._executeRollback, handler._escalateToHuman, handler.handleEpicFailure, handler._generateSuggestions) remain reachable so tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/orchestration/recovery-handler.test.js`:
- Around line 211-221: The test for handler._getStuckDetector uses a weak
assertion (toBeGreaterThanOrEqual) that can pass even when no new log was
emitted; change the assertion so it strictly verifies no additional log entry
was added on the second call: capture handler.logs.length before the second
call, call handler._getStuckDetector() again, and assert handler.logs.length ===
logCountAfterFirst (or assert the last log entry is unchanged) instead of using
toBeGreaterThanOrEqual; this uses the handler.logs and handler._getStuckDetector
symbols to make the test diagnostic.
---
Nitpick comments:
In `@tests/core/orchestration/recovery-handler.test.js`:
- Around line 58-1251: Large single spec; split into focused test files to
improve maintainability. Create separate spec files grouping related describes
(e.g., enums+constructor, strategy selection for _selectRecoveryStrategy,
rollback/escalation for _executeRollback and
_escalateToHuman/_saveEscalationReport, logging/history for
_log/getLogs/getEpicLogs/getAttemptHistory, and integrated flows for
handleEpicFailure/end-to-end), move the matching describe blocks for
RecoveryHandler/RecoveryStrategy/RecoveryResult/_selectRecoveryStrategy/_executeRollback/_escalateToHuman/_generateSuggestions/handleEpicFailure/_executeRecoveryStrategy/_triggerRecoveryWorkflow
into those files, extract shared setup (criarHandler, jest.clearAllMocks(),
mocked fs/path/orchestrator helpers) into a common test helper module and import
it in each new spec, ensure each new file still attaches the beforeEach that
creates handler and restores mocks and that all referenced symbols
(RecoveryHandler, RecoveryStrategy, handler._selectRecoveryStrategy,
handler._executeRollback, handler._escalateToHuman, handler.handleEpicFailure,
handler._generateSuggestions) remain reachable so tests pass.
| test('_getStuckDetector retorna mesma instância em chamadas repetidas', () => { | ||
| // Primeira chamada retorna null (módulo indisponível) | ||
| handler._getStuckDetector(); | ||
| // Limpa logs para ver se segunda chamada tenta novamente | ||
| const logCountAfterFirst = handler.logs.length; | ||
| // Não deve tentar carregar de novo pois _stuckDetector já foi setado como null | ||
| // Mas o código verifica !this._stuckDetector que é null (falsy), então tenta novamente | ||
| handler._getStuckDetector(); | ||
| // Deve ter logado novamente | ||
| expect(handler.logs.length).toBeGreaterThanOrEqual(logCountAfterFirst); | ||
| }); |
There was a problem hiding this comment.
Weak assertion makes this lazy-load test non-diagnostic
At Line 220, toBeGreaterThanOrEqual(logCountAfterFirst) can pass even when no new warning is emitted, so the test does not reliably validate the repeated-call behavior it describes.
Proposed fix
- expect(handler.logs.length).toBeGreaterThanOrEqual(logCountAfterFirst);
+ expect(handler.logs.length).toBeGreaterThan(logCountAfterFirst);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('_getStuckDetector retorna mesma instância em chamadas repetidas', () => { | |
| // Primeira chamada retorna null (módulo indisponível) | |
| handler._getStuckDetector(); | |
| // Limpa logs para ver se segunda chamada tenta novamente | |
| const logCountAfterFirst = handler.logs.length; | |
| // Não deve tentar carregar de novo pois _stuckDetector já foi setado como null | |
| // Mas o código verifica !this._stuckDetector que é null (falsy), então tenta novamente | |
| handler._getStuckDetector(); | |
| // Deve ter logado novamente | |
| expect(handler.logs.length).toBeGreaterThanOrEqual(logCountAfterFirst); | |
| }); | |
| test('_getStuckDetector retorna mesma instância em chamadas repetidas', () => { | |
| // Primeira chamada retorna null (módulo indisponível) | |
| handler._getStuckDetector(); | |
| // Limpa logs para ver se segunda chamada tenta novamente | |
| const logCountAfterFirst = handler.logs.length; | |
| // Não deve tentar carregar de novo pois _stuckDetector já foi setado como null | |
| // Mas o código verifica !this._stuckDetector que é null (falsy), então tenta novamente | |
| handler._getStuckDetector(); | |
| // Deve ter logado novamente | |
| expect(handler.logs.length).toBeGreaterThan(logCountAfterFirst); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/recovery-handler.test.js` around lines 211 - 221,
The test for handler._getStuckDetector uses a weak assertion
(toBeGreaterThanOrEqual) that can pass even when no new log was emitted; change
the assertion so it strictly verifies no additional log entry was added on the
second call: capture handler.logs.length before the second call, call
handler._getStuckDetector() again, and assert handler.logs.length ===
logCountAfterFirst (or assert the last log entry is unchanged) instead of using
toBeGreaterThanOrEqual; this uses the handler.logs and handler._getStuckDetector
symbols to make the test diagnostic.
There was a problem hiding this comment.
Pull request overview
Adds a comprehensive Jest unit test suite for the RecoveryHandler orchestration module, aiming to validate recovery strategies, escalation behavior, rollback flow, logging, and event emission.
Changes:
- Introduces a new test suite covering
RecoveryHandler,RecoveryStrategy, andRecoveryResult. - Adds mocked filesystem behavior (
fs-extra) and simulated “module unavailable” scenarios for external recovery dependencies. - Exercises key recovery flows end-to-end (retry → rollback → escalation) plus logging/attempt history utilities.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('_getStuckDetector retorna mesma instância em chamadas repetidas', () => { | ||
| // Primeira chamada retorna null (módulo indisponível) | ||
| handler._getStuckDetector(); | ||
| // Limpa logs para ver se segunda chamada tenta novamente | ||
| const logCountAfterFirst = handler.logs.length; | ||
| // Não deve tentar carregar de novo pois _stuckDetector já foi setado como null | ||
| // Mas o código verifica !this._stuckDetector que é null (falsy), então tenta novamente | ||
| handler._getStuckDetector(); | ||
| // Deve ter logado novamente | ||
| expect(handler.logs.length).toBeGreaterThanOrEqual(logCountAfterFirst); |
There was a problem hiding this comment.
Este teste está fraco/inconsistente com o próprio nome e comentários: ele não valida que a segunda chamada realmente gerou uma nova tentativa/log (usa toBeGreaterThanOrEqual, que passa mesmo sem mudança) e também não verifica a “mesma instância”. Ajuste para uma asserção estrita (ex.: > ou +1 no número de logs) e/ou renomeie o teste para refletir o comportamento esperado (re-tentar carregar quando _stuckDetector permanece null).
| test('_getStuckDetector retorna mesma instância em chamadas repetidas', () => { | |
| // Primeira chamada retorna null (módulo indisponível) | |
| handler._getStuckDetector(); | |
| // Limpa logs para ver se segunda chamada tenta novamente | |
| const logCountAfterFirst = handler.logs.length; | |
| // Não deve tentar carregar de novo pois _stuckDetector já foi setado como null | |
| // Mas o código verifica !this._stuckDetector que é null (falsy), então tenta novamente | |
| handler._getStuckDetector(); | |
| // Deve ter logado novamente | |
| expect(handler.logs.length).toBeGreaterThanOrEqual(logCountAfterFirst); | |
| test('_getStuckDetector tenta recarregar módulo quando permanece null em chamadas repetidas', () => { | |
| // Primeira chamada retorna null (módulo indisponível) e deve logar um warn | |
| const logCountBeforeFirst = handler.logs.length; | |
| const firstResult = handler._getStuckDetector(); | |
| const logCountAfterFirst = handler.logs.length; | |
| expect(firstResult).toBeNull(); | |
| expect(logCountAfterFirst).toBeGreaterThan(logCountBeforeFirst); | |
| // Segunda chamada também retorna null, mas deve tentar carregar novamente e logar de novo | |
| const secondResult = handler._getStuckDetector(); | |
| const logCountAfterSecond = handler.logs.length; | |
| expect(secondResult).toBeNull(); | |
| // Deve ter logado novamente (número de logs precisa aumentar estritamente) | |
| expect(logCountAfterSecond).toBeGreaterThan(logCountAfterFirst); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/orchestration/context-manager.test.js (1)
79-131:initializetests still miss filesystem rejection paths.Current cases cover only happy paths. Add explicit tests for
fs.readJsonand initial-statefs.writeJsonrejection to verify propagation/failure behavior.As per coding guidelines, "Verify error handling is comprehensive."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/context-manager.test.js` around lines 79 - 131, Add tests that cover filesystem rejection paths for manager.initialize: one where fs.pathExists resolves true and fs.readJson rejects with an error (mock fs.readJson.mockRejectedValue) and assert that await manager.initialize() rejects with that error and that fs.readJson was called with manager.statePath; and another where fs.pathExists resolves false and fs.writeJson rejects (mock fs.writeJson.mockRejectedValue) and assert initialize rejects with that error and that fs.ensureDir was called for manager.stateDir/manager.handoffDir/manager.confidenceDir before the failing write; use manager.initialize, fs.readJson, fs.writeJson and manager.statePath/manager.stateDir references in the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/orchestration/context-manager.test.js`:
- Around line 149-155: The test for ISO timestamps should assert both timestamps
with toISOString() rather than only checking updatedAt with not.toThrow; update
the test in context-manager.test.js (the test 'startedAt e updatedAt são strings
ISO válidas') to call new Date(state.updatedAt).toISOString() and expect it
toBe(state.updatedAt) in the same way as startedAt, ensuring both
state.startedAt and state.updatedAt produced by manager._createInitialState()
are validated as proper ISO strings.
---
Nitpick comments:
In `@tests/core/orchestration/context-manager.test.js`:
- Around line 79-131: Add tests that cover filesystem rejection paths for
manager.initialize: one where fs.pathExists resolves true and fs.readJson
rejects with an error (mock fs.readJson.mockRejectedValue) and assert that await
manager.initialize() rejects with that error and that fs.readJson was called
with manager.statePath; and another where fs.pathExists resolves false and
fs.writeJson rejects (mock fs.writeJson.mockRejectedValue) and assert initialize
rejects with that error and that fs.ensureDir was called for
manager.stateDir/manager.handoffDir/manager.confidenceDir before the failing
write; use manager.initialize, fs.readJson, fs.writeJson and
manager.statePath/manager.stateDir references in the assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/core/orchestration/context-manager.test.jstests/core/orchestration/recovery-handler.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/orchestration/context-manager.test.js`:
- Around line 1170-1175: The test shows current behavior where an empty string
env var yields 0 because Number('') === 0; change the implementation in
_resolveConfidenceThreshold (the method on manager) to treat empty or
whitespace-only strings as missing and return the fallback (70). Concretely,
read process.env.AIOS_DELIVERY_CONFIDENCE_THRESHOLD into a variable, if it's
null/undefined or raw.trim() === '' return 70, otherwise parse Number(raw) and
return parsed if Number.isFinite(parsed) else 70.
| test('retorna 0 quando env é string vazia (Number("") === 0)', () => { | ||
| process.env.AIOS_DELIVERY_CONFIDENCE_THRESHOLD = ''; | ||
|
|
||
| // Number('') === 0, que é Number.isFinite, então retorna 0 | ||
| expect(manager._resolveConfidenceThreshold()).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of _resolveConfidenceThreshold
ast-grep --pattern $'_resolveConfidenceThreshold() {
$$$
}'Repository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
# Search for the method definition with broader pattern
rg -n "resolveConfidenceThreshold" --type=js -A 15Repository: SynkraAI/aios-core
Length of output: 5091
🏁 Script executed:
# Check for environment variable handling and documentation
rg -n "AIOS_DELIVERY_CONFIDENCE_THRESHOLD" --type=js --type=md -B 2 -A 2Repository: SynkraAI/aios-core
Length of output: 3388
🏁 Script executed:
# Find the implementation file (context-manager source)
fd -t f "context-manager" --extension js | grep -v test | grep -v node_modulesRepository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
# Search for the actual implementation of _resolveConfidenceThreshold in source files
rg -n "_resolveConfidenceThreshold\s*\(" --type=js | grep -v testRepository: SynkraAI/aios-core
Length of output: 44
🏁 Script executed:
# Find the source directory structure
find . -path ./node_modules -prune -o -type d -name "orchestration" -printRepository: SynkraAI/aios-core
Length of output: 120
🏁 Script executed:
# List files in the orchestration directory
ls -la src/core/orchestration/ 2>/dev/null || echo "Directory not found, searching broader..."
git ls-files | grep -i "orchestration" | grep -v test | head -20Repository: SynkraAI/aios-core
Length of output: 1181
🏁 Script executed:
# Search for _resolveConfidenceThreshold in all JS files without test
rg "resolveConfidenceThreshold" --type=js -lRepository: SynkraAI/aios-core
Length of output: 110
🏁 Script executed:
# Read the context-manager.js source file to find the implementation
wc -l .aios-core/core/orchestration/context-manager.jsRepository: SynkraAI/aios-core
Length of output: 114
🏁 Script executed:
# Search for _resolveConfidenceThreshold in the source file
rg -n "_resolveConfidenceThreshold" .aios-core/core/orchestration/context-manager.js -A 20Repository: SynkraAI/aios-core
Length of output: 1291
The test correctly documents current behavior, but the implementation has a semantic inconsistency worth addressing.
The current implementation treats empty strings differently from other invalid inputs: Number('') === 0 passes Number.isFinite(), returning 0 (accept all scores), while invalid strings return NaN and fall back to 70. Consider updating _resolveConfidenceThreshold() to treat empty strings like other invalid inputs:
_resolveConfidenceThreshold() {
const raw = process.env.AIOS_DELIVERY_CONFIDENCE_THRESHOLD;
if (!raw || raw.trim() === '') return 70;
const parsed = Number(raw);
return Number.isFinite(parsed) ? parsed : 70;
}This aligns empty env vars (likely misconfiguration) with the fallback behavior, reducing unintended security gaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/context-manager.test.js` around lines 1170 - 1175,
The test shows current behavior where an empty string env var yields 0 because
Number('') === 0; change the implementation in _resolveConfidenceThreshold (the
method on manager) to treat empty or whitespace-only strings as missing and
return the fallback (70). Concretely, read
process.env.AIOS_DELIVERY_CONFIDENCE_THRESHOLD into a variable, if it's
null/undefined or raw.trim() === '' return 70, otherwise parse Number(raw) and
return parsed if Number.isFinite(parsed) else 70.
Rebase Necessário@nikolasdehor O PR #535 (ContextManager tests) acabou de ser mergeado em main. Como ambos os PRs tocam Ação necessária: Rebase em main e resolver os conflitos no arquivo de testes compartilhado. Os 149 testes de RecoveryHandler estão aprovados — assim que o rebase for feito, mergeamos imediatamente. — Gage (DevOps) |
Troca toBeGreaterThanOrEqual por toBeGreaterThan para validar que segunda chamada realmente gera novo warning. Renomeia teste para refletir comportamento esperado (tentativa de recarga).
nikolasdehor
force-pushed
the
test/recovery-handler-coverage
branch
from
6bd9d45 to
83ff94c
Compare
|
@Pedrovaleriolopez @oalanicolas, esse PR adiciona cobertura de testes pro RecoveryHandler que tava sem nenhum teste unitário. Só testes, sem mudança de código de produção. |
…vazia em _resolveConfidenceThreshold - Corrige imports e asserts do recovery-handler.test.js para .aiox-core - Trata string vazia como ausente em _resolveConfidenceThreshold (retorna fallback 70)
github-actions
bot
added
area: agents
github-actions
bot
added
area: pro
Resumo
Adiciona suite de testes unitários para o módulo RecoveryHandler (
orchestration/recovery-handler.js).Cobertura
Plano de teste
Summary by CodeRabbit