fix: build-state-manager null guards para checkpoints/failedAttempts/notifications#515
fix: build-state-manager null guards para checkpoints/failedAttempts/notifications#515nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
Conversation
getLastCheckpoint, getCheckpoint e getStatus acessavam .checkpoints, .failedAttempts e .notifications sem verificar se existem. Quando o state é carregado de JSON sem esses campos (campos opcionais na validação), ocorre TypeError. Adiciona 12 testes de regressão cobrindo os cenários de null. Closes SynkraAI#513
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 defensive null/undefined checks to BuildStateManager methods that access nested array properties (checkpoints, notifications) to prevent runtime TypeErrors when these arrays are missing, accompanied by comprehensive test coverage for edge case scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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 adds null guards to prevent TypeErrors when optional arrays (checkpoints, failedAttempts, notifications) are undefined in the build state. The fix addresses issue #513 by adding defensive checks to three methods in BuildStateManager and includes unit tests to verify the guards work correctly.
Changes:
- Added null guards to
getLastCheckpoint()andgetCheckpoint()to check ifcheckpointsarray exists before accessing it - Updated
getStatus()to use the|| []pattern for safely accessingfailedAttempts,checkpoints, andnotificationsarrays - Added 12 unit tests covering null/undefined scenarios for the fixed methods
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .aios-core/core/execution/build-state-manager.js | Added null guards to getLastCheckpoint(), getCheckpoint(), and defensive array access in getStatus() to prevent TypeErrors when arrays are undefined |
| tests/core/execution/build-state-manager-guards.test.js | Added unit tests for null guards in getLastCheckpoint() and getCheckpoint(), plus constructor tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Testes para null guards em BuildStateManager | ||
| * | ||
| * Valida que getLastCheckpoint, getCheckpoint e getStatus | ||
| * não falham com TypeError quando arrays são undefined. | ||
| * | ||
| * Closes #513 | ||
| */ |
There was a problem hiding this comment.
The test file header claims to test getStatus, but there are no actual test cases for the getStatus method in this file. The PR description mentions that getStatus was fixed to handle undefined arrays for failedAttempts, checkpoints, and notifications, but these fixes are not covered by any tests. Consider adding a test suite for getStatus that validates the guards work correctly when these arrays are undefined.
| // ============================================================ | ||
| // constructor | ||
| // ============================================================ | ||
| describe('constructor', () => { | ||
| test('requer storyId', () => { | ||
| expect(() => new BuildStateManager()).toThrow('storyId is required'); | ||
| }); | ||
|
|
||
| test('aceita storyId string', () => { | ||
| const m = new BuildStateManager('story-123'); | ||
| expect(m.storyId).toBe('story-123'); | ||
| }); | ||
|
|
||
| test('inicializa _state como null', () => { | ||
| expect(manager._state).toBeNull(); | ||
| }); | ||
|
|
||
| test('usa rootPath padrão se não fornecido', () => { | ||
| const m = new BuildStateManager('s1'); | ||
| expect(m.rootPath).toBe(process.cwd()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These constructor tests are not related to the null guard fixes described in issue #513 or the PR. They test basic constructor functionality that should be covered by general BuildStateManager tests, not in a file specifically focused on null guard testing. Consider moving these tests to a separate general test file or removing them if they're already covered elsewhere.
| // ============================================================ | |
| // constructor | |
| // ============================================================ | |
| describe('constructor', () => { | |
| test('requer storyId', () => { | |
| expect(() => new BuildStateManager()).toThrow('storyId is required'); | |
| }); | |
| test('aceita storyId string', () => { | |
| const m = new BuildStateManager('story-123'); | |
| expect(m.storyId).toBe('story-123'); | |
| }); | |
| test('inicializa _state como null', () => { | |
| expect(manager._state).toBeNull(); | |
| }); | |
| test('usa rootPath padrão se não fornecido', () => { | |
| const m = new BuildStateManager('s1'); | |
| expect(m.rootPath).toBe(process.cwd()); | |
| }); | |
| }); |
| */ | ||
| getLastCheckpoint() { | ||
| if (!this._state || this._state.checkpoints.length === 0) { | ||
| if (!this._state || !this._state.checkpoints || this._state.checkpoints.length === 0) { |
There was a problem hiding this comment.
There's a discrepancy between the PR description and the actual code. The PR description states that checkpoints is optional in validation, but validateBuildState at line 103 includes 'checkpoints' in the required fields array. In reality, checkpoints is REQUIRED by validation, while failedAttempts and notifications are truly optional (not in the required array). The null guards being added here are still valuable for defensive programming and to handle cases where _state is manually set (as in tests), but the PR description should be corrected to accurately reflect that checkpoints is actually a required field according to the schema validation.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.aios-core/core/execution/build-state-manager.js (2)
896-909:⚠️ Potential issue | 🔴 Critical
getNotificationsandacknowledgeNotificationhave the same missing guard fornotifications.
getNotificationschecks!this._statebut then directly calls.filter()onthis._state.notifications— ifnotificationsis absent from a partial state, this throws a TypeError.
acknowledgeNotificationis worse:!this._state.notifications[index]itself throws a TypeError whennotificationsisundefined, so the guard never short-circuits as intended.🐛 Proposed fix
getNotifications() { if (!this._state) return []; - return this._state.notifications.filter((n) => !n.acknowledged); + return (this._state.notifications || []).filter((n) => !n.acknowledged); }acknowledgeNotification(index) { - if (!this._state || !this._state.notifications[index]) return; + if (!this._state || !this._state.notifications || !this._state.notifications[index]) return; this._state.notifications[index].acknowledged = true; this.saveState(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/execution/build-state-manager.js around lines 896 - 909, Both getNotifications and acknowledgeNotification need a safe guard for this._state.notifications to avoid TypeError when notifications is missing; update getNotifications (method getNotifications) to return [] if !this._state or !Array.isArray(this._state.notifications), and update acknowledgeNotification (method acknowledgeNotification) to first check that this._state and Array.isArray(this._state.notifications) and that index is a valid integer within range (e.g., 0 <= index < this._state.notifications.length) before accessing this._state.notifications[index]; only then set .acknowledged = true and call saveState().
786-795:⚠️ Potential issue | 🔴 Critical
failedAttemptsaccessed without a null guard — same TypeError as issue#513.
this._stateis checked, butthis._state.failedAttemptsis not. A state loaded from older or partial JSON that lacks thefailedAttemptskey will throwTypeError: Cannot read properties of undefined (reading 'filter')at Line 788, and again at thepushon Line 795. This is exactly the category of bug this PR is fixing elsewhere.
_checkIfStuck(called at Line 800) has the identical gap on Line 834:this._state.failedAttempts.filter(...).🐛 Proposed fix
- attempt: - options.attempt || - this._state.failedAttempts.filter((f) => f.subtaskId === subtaskId).length + 1, + attempt: + options.attempt || + (this._state.failedAttempts || []).filter((f) => f.subtaskId === subtaskId).length + 1,- this._state.failedAttempts.push(failure); + (this._state.failedAttempts ??= []).push(failure);And in
_checkIfStuck:- const attempts = this._state.failedAttempts - .filter((f) => f.subtaskId === subtaskId) + const attempts = (this._state.failedAttempts || []) + .filter((f) => f.subtaskId === subtaskId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/execution/build-state-manager.js around lines 786 - 795, The code assumes this._state.failedAttempts exists before using .filter and .push, causing a TypeError when state is missing that key; before constructing and pushing the new failure in the function that creates `failure` and before `_checkIfStuck` uses `this._state.failedAttempts.filter(...)`, ensure `this._state.failedAttempts` is initialized to an array when absent (e.g., set to [] if falsy) so subsequent `.filter` and `.push` calls are safe; update both the failure-recording block (where `failure` is built and `this._state.failedAttempts.push(failure)` is called) and the `_checkIfStuck` method to perform this guard/initialization.
🧹 Nitpick comments (1)
tests/core/execution/build-state-manager-guards.test.js (1)
13-15: AddafterAllto restore the module-level spies.Without restoring these spies, any other test file in the same worker that directly exercises
fs.existsSyncorfs.mkdirSyncmay get the mocked behaviour. While Jest 30's per-file VM isolation makes cross-file leakage unlikely, explicit cleanup is still best practice and documents intent clearly.🛡️ Suggested cleanup
jest.spyOn(fs, 'existsSync').mockReturnValue(false); jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); + +afterAll(() => { + jest.restoreAllMocks(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/execution/build-state-manager-guards.test.js` around lines 13 - 15, Add an afterAll cleanup that restores the module-level spies on fs by calling the mock restore methods for fs.existsSync and fs.mkdirSync; specifically, after the jest.spyOn(fs, 'existsSync') and jest.spyOn(fs, 'mkdirSync') calls in the tests/core/execution/build-state-manager-guards.test.js file, add an afterAll() block that invokes fs.existsSync.mockRestore() and fs.mkdirSync.mockRestore() (or the equivalent jest.restoreAllMocks if preferred) to ensure other tests don't see the mocked behavior.
🤖 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/execution/build-state-manager-guards.test.js`:
- Around line 1-8: Add the missing getStatus null-guard tests in
tests/core/execution/build-state-manager-guards.test.js: create a
describe('getStatus', ...) block that constructs a BuildStateManager (or uses
the existing test setup), calls getStatus with a state object where each of
state.failedAttempts, state.checkpoints, and state.notifications is undefined
(test each scenario separately), and assert that getStatus returns
recentFailures as [], checkpointCount as 0, and notificationCount as 0
respectively; reference the BuildStateManager.getStatus method and the state
fields failedAttempts, checkpoints, notifications as the targets to exercise the
`|| []` guards.
---
Outside diff comments:
In @.aios-core/core/execution/build-state-manager.js:
- Around line 896-909: Both getNotifications and acknowledgeNotification need a
safe guard for this._state.notifications to avoid TypeError when notifications
is missing; update getNotifications (method getNotifications) to return [] if
!this._state or !Array.isArray(this._state.notifications), and update
acknowledgeNotification (method acknowledgeNotification) to first check that
this._state and Array.isArray(this._state.notifications) and that index is a
valid integer within range (e.g., 0 <= index < this._state.notifications.length)
before accessing this._state.notifications[index]; only then set .acknowledged =
true and call saveState().
- Around line 786-795: The code assumes this._state.failedAttempts exists before
using .filter and .push, causing a TypeError when state is missing that key;
before constructing and pushing the new failure in the function that creates
`failure` and before `_checkIfStuck` uses
`this._state.failedAttempts.filter(...)`, ensure `this._state.failedAttempts` is
initialized to an array when absent (e.g., set to [] if falsy) so subsequent
`.filter` and `.push` calls are safe; update both the failure-recording block
(where `failure` is built and `this._state.failedAttempts.push(failure)` is
called) and the `_checkIfStuck` method to perform this guard/initialization.
---
Nitpick comments:
In `@tests/core/execution/build-state-manager-guards.test.js`:
- Around line 13-15: Add an afterAll cleanup that restores the module-level
spies on fs by calling the mock restore methods for fs.existsSync and
fs.mkdirSync; specifically, after the jest.spyOn(fs, 'existsSync') and
jest.spyOn(fs, 'mkdirSync') calls in the
tests/core/execution/build-state-manager-guards.test.js file, add an afterAll()
block that invokes fs.existsSync.mockRestore() and fs.mkdirSync.mockRestore()
(or the equivalent jest.restoreAllMocks if preferred) to ensure other tests
don't see the mocked behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/core/execution/build-state-manager.jstests/core/execution/build-state-manager-guards.test.js
…tion e failedAttempts CodeRabbit encontrou que getNotifications(), acknowledgeNotification() e recordFailure() acessam arrays sem verificar se existem, causando TypeError quando notifications ou failedAttempts são undefined. Correções: - getNotifications: (this._state.notifications || []).filter(...) - acknowledgeNotification: guard !this._state.notifications antes de indexar - recordFailure: this._state.failedAttempts ??= [] antes de .filter/.push - _checkIfStuck: (this._state.failedAttempts || []).filter(...) Adicionados 11 novos testes cobrindo getStatus, getNotifications, acknowledgeNotification e recordFailure com arrays undefined. Total: 23 testes (era 12).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.aios-core/core/execution/build-state-manager.js (2)
365-373:⚠️ Potential issue | 🟠 MajorSame gap in
saveCheckpoint—checkpointsandcompletedSubtaskscan be undefined.
loadState()only validates that the fields exist viavalidateBuildState, butvalidateBuildState(line 105) only checksstate[field] === undefinedforcheckpoints— it does not rejectnull. A state file with"checkpoints": nullpasses validation and reaches this code, causingTypeError: Cannot read properties of null (reading 'push').Apply
??=guards before mutation:Proposed fix
// Add to state + this._state.checkpoints ??= []; this._state.checkpoints.push(checkpoint); this._state.lastCheckpoint = now; // Update completed subtasks + this._state.completedSubtasks ??= []; if (!this._state.completedSubtasks.includes(subtaskId)) {As per coding guidelines, "Core framework modules require thorough review. Verify error handling is comprehensive with proper try/catch and error context."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/execution/build-state-manager.js around lines 365 - 373, In saveCheckpoint, guard against null/undefined collections before mutating them: in the saveCheckpoint method (and similarly any code paths after loadState), ensure this._state.checkpoints ??= [] and this._state.completedSubtasks ??= [] are applied before pushing, and ensure this._state.metrics ??= {} and this._state.metrics.completedSubtasks ??= 0 before incrementing; reference saveCheckpoint, loadState, and validateBuildState to locate the code and add these nullish-coalescing assignments just prior to the existing mutations to prevent TypeError when state fields are null.
881-891:⚠️ Potential issue | 🟠 MajorIncomplete guard coverage —
_addNotificationand_notifyFailurecan crash on missingnotificationsarray.This PR adds guards on read paths but misses several write paths that
.push()onto optional arrays. Ifnotificationsis undefined in a loaded state (permitted by validation schema sincenotificationsis not in required fields), these methods will throwTypeError:
- Line 884:
_addNotificationguards!this._statebut not!this._state.notificationsbefore calling.push()- Line 866:
_notifyFailurehas no guard onthis._statebefore calling.push(notification)Both methods can be reached through state loading paths (resumeBuild, detectAbandoned, completeBuild, failBuild) where notifications array may be undefined.
Apply the
??=pattern already used inrecordFailure(line 785):Proposed fix
_addNotification(type, message, context = {}) { if (!this._state) return; + this._state.notifications ??= []; this._state.notifications.push({_notifyFailure(subtaskId, failure, stuckResult) { const notification = { ... }; + this._state.notifications ??= []; this._state.notifications.push(notification);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/execution/build-state-manager.js around lines 881 - 891, Both _addNotification and _notifyFailure currently assume this._state.notifications exists and can crash; mirror recordFailure's pattern by initializing the array before pushing: ensure in both _addNotification and _notifyFailure you first check this._state (as already done) and then use the nullish-assignment pattern (this._state.notifications ??= []) before calling .push(...). Update the methods _addNotification and _notifyFailure to perform that initialization so pushes are safe when notifications is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.aios-core/core/execution/build-state-manager.js:
- Around line 365-373: In saveCheckpoint, guard against null/undefined
collections before mutating them: in the saveCheckpoint method (and similarly
any code paths after loadState), ensure this._state.checkpoints ??= [] and
this._state.completedSubtasks ??= [] are applied before pushing, and ensure
this._state.metrics ??= {} and this._state.metrics.completedSubtasks ??= 0
before incrementing; reference saveCheckpoint, loadState, and validateBuildState
to locate the code and add these nullish-coalescing assignments just prior to
the existing mutations to prevent TypeError when state fields are null.
- Around line 881-891: Both _addNotification and _notifyFailure currently assume
this._state.notifications exists and can crash; mirror recordFailure's pattern
by initializing the array before pushing: ensure in both _addNotification and
_notifyFailure you first check this._state (as already done) and then use the
nullish-assignment pattern (this._state.notifications ??= []) before calling
.push(...). Update the methods _addNotification and _notifyFailure to perform
that initialization so pushes are safe when notifications is undefined.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/core/execution/build-state-manager.jstests/core/execution/build-state-manager-guards.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/execution/build-state-manager-guards.test.js
|
Fechando como duplicata — substituído pelo PR #523 que aplica os mesmos null guards com base limpa no main. |
Descrição
Corrige 3 potenciais TypeErrors em
build-state-manager.jsquando arrays opcionais (checkpoints,failedAttempts,notifications) sãoundefinedno state carregado de JSON.Problema
Os campos
checkpoints,failedAttemptsenotificationssão opcionais na validação de estado (validateBuildState), mas os métodosgetLastCheckpoint(),getCheckpoint()egetStatus()acessam esses arrays sem verificar se existem:Isso acontece quando um
build-state.jsoné salvo por uma versão mais antiga ou sem esses campos.Correção
getLastCheckpoint() { - if (!this._state || this._state.checkpoints.length === 0) { + if (!this._state || !this._state.checkpoints || this._state.checkpoints.length === 0) { return null; }getCheckpoint(checkpointId) { - if (!this._state) { + if (!this._state || !this._state.checkpoints) { return null; }Testes
12 testes unitários cobrindo todos os cenários de null/undefined:
Closes #513
Summary by CodeRabbit
Bug Fixes
Tests