fix(gotchas-memory): apply errorWindowMs rolling window and fix critical sort order - #477
fix(gotchas-memory): apply errorWindowMs rolling window and fix critical sort order#477nikolasdehor wants to merge 1 commit into
Conversation
|
@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. |
WalkthroughThis PR fixes two bugs in the GotchasMemory error tracking module: a missing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes two critical bugs in the GotchasMemory module that were preventing the error tracking window and severity sorting from working correctly.
Changes:
- Fixed Bug #475: Implemented rolling window for error tracking using
errorWindowMsby resetting count when the last occurrence is outside the window - Fixed Bug #476: Changed severity sorting to use nullish coalescing (
??) instead of logical OR (||) to properly handle critical severity (value 0) - Added comprehensive unit tests with 60 test cases covering both bug fixes and existing functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/core/memory/gotchas-memory.test.js | Added comprehensive test suite with 60 tests, including 3 regression tests for the error window fix and sorting validation |
| .aios-core/core/memory/gotchas-memory.js | Fixed errorWindowMs rolling window logic in trackError() and severity sorting in listGotchas() using nullish coalescing |
| .aios-core/install-manifest.yaml | Updated metadata (hashes, sizes, timestamps) to reflect code changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('sorts critical first', () => { | ||
| const list = m.listGotchas(); | ||
| expect(list[0].severity).toBe(Severity.CRITICAL); |
There was a problem hiding this comment.
The test only verifies that critical severity appears first, but doesn't verify the complete sort order. Consider asserting the full sequence: critical → warning → info. This would more thoroughly validate the bug fix for issue #476.
| test('sorts critical first', () => { | |
| const list = m.listGotchas(); | |
| expect(list[0].severity).toBe(Severity.CRITICAL); | |
| test('sorts by severity: critical → warning → info', () => { | |
| const list = m.listGotchas(); | |
| const severities = list.map(g => g.severity); | |
| expect(severities).toEqual([Severity.CRITICAL, Severity.WARNING, Severity.INFO]); |
| // contribute toward the auto-capture threshold (fixes #475) | ||
| tracking.count = 0; | ||
| tracking.firstSeen = now; | ||
| tracking.samples = []; |
There was a problem hiding this comment.
When the error window expires and tracking is reset, the errorPattern and category fields are lost but not reinitialized. These fields are used later in _autoCaptureGotcha (line 677). Consider preserving these fields when resetting the window, or ensure they're re-computed when needed. The current implementation will cause tracking.category to be undefined after a window reset, which could affect auto-captured gotchas.
| tracking.samples = []; | |
| tracking.samples = []; | |
| // Ensure metadata used by _autoCaptureGotcha is up to date after reset | |
| tracking.errorPattern = errorData.message; | |
| tracking.category = this._detectCategory(errorData.message + ' ' + (errorData.stack || '')); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/core/memory/gotchas-memory.test.js (2)
294-297: Sort order test only verifiescriticalis first — consider asserting the full order.Since the
|| → ??fix is the crux of issue#476, it would add confidence to assert the completecritical → warning → infoordering rather than just checking the first element.🧪 Strengthen the sort-order assertion
test('sorts critical first', () => { const list = m.listGotchas(); - expect(list[0].severity).toBe(Severity.CRITICAL); + const severities = list.map((g) => g.severity); + expect(severities).toEqual([Severity.CRITICAL, Severity.WARNING, Severity.INFO]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/gotchas-memory.test.js` around lines 294 - 297, Update the "sorts critical first" test to assert the complete severity ordering instead of only the first element: call m.listGotchas(), map the returned items to their .severity and assert the array equals [Severity.CRITICAL, Severity.WARNING, Severity.INFO] (and optionally assert expected length), referencing the existing test name and the m.listGotchas and Severity.* enums to locate where to change the assertion.
25-33: Temp directories are never cleaned up.
makeTmpDir()creates directories inos.tmpdir()but there's noafterAllorafterEachhook to remove them. The OS will eventually reclaim the space, but in CI environments with many test runs this can accumulate. Consider adding cleanup or using Jest's built-in temp directory support if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/memory/gotchas-memory.test.js` around lines 25 - 33, makeTmpDir() creates temp dirs that are never removed; update the test file to track and clean those directories after tests by recording each returned root from makeTmpDir()/makeMemory (e.g., push into a local array) and add an afterEach or afterAll hook that iterates the tracked paths and removes them with fs.rmSync(root, { recursive: true, force: true }) (or fs.rmdirSync for older Node), ensuring you reference makeTmpDir, makeMemory and GotchasMemory when locating where to add the tracking and the cleanup hook..aios-core/core/memory/gotchas-memory.js (1)
186-190: Constructor options use||, which has the same falsy-coercion issue fixed inlistGotchas.Lines 187–188 use
||forrepeatThresholdanderrorWindowMs. If a caller ever passes0for either, the default will silently override their intent — the same class of bug this PR fixes at line 324. While0isn't a practical value for these settings today, applying??here would be consistent with the fix philosophy and prevent a subtle future foot-gun.♻️ Suggested consistency fix
this.options = { - repeatThreshold: options.repeatThreshold || CONFIG.repeatThreshold, - errorWindowMs: options.errorWindowMs || CONFIG.errorWindowMs, + repeatThreshold: options.repeatThreshold ?? CONFIG.repeatThreshold, + errorWindowMs: options.errorWindowMs ?? CONFIG.errorWindowMs, quiet: options.quiet || false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/memory/gotchas-memory.js around lines 186 - 190, The constructor currently sets this.options.repeatThreshold and this.options.errorWindowMs using || which treats 0 as falsy and will override an explicit 0; change those assignments to use the nullish coalescing operator (??) so that only null/undefined fall back to CONFIG (mirror the fix applied in listGotchas); update the two places where repeatThreshold and errorWindowMs are assigned in the constructor to use ?? instead of ||, leaving the quiet default as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.aios-core/core/memory/gotchas-memory.js:
- Around line 186-190: The constructor currently sets
this.options.repeatThreshold and this.options.errorWindowMs using || which
treats 0 as falsy and will override an explicit 0; change those assignments to
use the nullish coalescing operator (??) so that only null/undefined fall back
to CONFIG (mirror the fix applied in listGotchas); update the two places where
repeatThreshold and errorWindowMs are assigned in the constructor to use ??
instead of ||, leaving the quiet default as-is.
In `@tests/core/memory/gotchas-memory.test.js`:
- Around line 294-297: Update the "sorts critical first" test to assert the
complete severity ordering instead of only the first element: call
m.listGotchas(), map the returned items to their .severity and assert the array
equals [Severity.CRITICAL, Severity.WARNING, Severity.INFO] (and optionally
assert expected length), referencing the existing test name and the
m.listGotchas and Severity.* enums to locate where to change the assertion.
- Around line 25-33: makeTmpDir() creates temp dirs that are never removed;
update the test file to track and clean those directories after tests by
recording each returned root from makeTmpDir()/makeMemory (e.g., push into a
local array) and add an afterEach or afterAll hook that iterates the tracked
paths and removes them with fs.rmSync(root, { recursive: true, force: true })
(or fs.rmdirSync for older Node), ensuring you reference makeTmpDir, makeMemory
and GotchasMemory when locating where to add the tracking and the cleanup hook.
2b8a769 to
efcd8c4
Compare
… order Two bugs in GotchasMemory: 1. trackError() never applied the errorWindowMs rolling window (fixes SynkraAI#475). Errors accumulated indefinitely — the 24-hour window config was dead code. Fix: reset the count when the elapsed time since lastSeen exceeds the window. 2. listGotchas() used `|| 2` fallback on severityOrder, causing `critical` (= 0) to be treated as falsy and sorted last instead of first (fixes SynkraAI#476). Fix: use `?? 2` (nullish coalescing) to preserve the zero value. 60 unit tests added covering all public methods, error window regression, sort correctness, persistence, and event emission.
efcd8c4 to
c5f375f
Compare
|
Rebased on latest main, resolved the install-manifest.yaml conflict, and fixed an incorrect import path in the test file. All 60 tests pass. This PR addresses #475 — the errorWindowMs configuration was being read but never actually applied when counting errors in trackError(), and the critical-first sort order was inverted. Would appreciate a review when you get a chance. |
github-actions
Bot
added
area: agents
|
@Pedrovaleriolopez, solicitando review deste PR. Aberto desde 23/fev, corrige a issue #475 (GotchasMemory sem respeitar O PR duplicado #584 (aberto 11/mar) contém a mesma correção. Este PR está MERGEABLE e aguardando review há mais de 2 semanas. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Fechado como superseded pelo PR #673, já merged em main e publicado como @aiox-squads/core@5.1.13. O GotchasMemory agora aplica rolling window por timestamps e mantém critical first no sort, com teste regressional. |
Resumo
Corrige dois bugs no módulo gotchas-memory:
Bug 1: errorWindowMs nunca aplicado (Closes #475)
trackError()contava repetições acumuladas sem considerar a janela de tempoerrorWindowMsBug 2: Ordenação de severidade trata critical como info (Closes #476)
severityOrder['critical']é0, e0 || 2avalia como2(falsy)||por??(nullish coalescing) —0 ?? 2 === 0Plano de teste
jest.useFakeTimers()para testar a janela de tempoos.tmpdir()