fix(execution): null guard em getLastCheckpoint/getCheckpoint (#513)#523
fix(execution): null guard em getLastCheckpoint/getCheckpoint (#513)#523nikolasdehor wants to merge 2 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. |
WalkthroughBug fix adding null/undefined guards to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 addresses a defensive programming issue in the BuildStateManager where accessing this._state.checkpoints without first verifying it exists could cause a TypeError. The fix adds null guards to getLastCheckpoint() and getCheckpoint() methods to prevent crashes when the state's checkpoints property is undefined.
Changes:
- Added null guard for
this._state.checkpointsingetLastCheckpoint()method - Added null guard for
this._state.checkpointsingetCheckpoint()method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.aios-core/core/execution/build-state-manager.js (1)
365-366: Consider adding a similar null guard here for consistency.While
saveCheckpointalready validates!this._state, the same corrupted-state scenario (state exists butcheckpointsis undefined) could cause a TypeError at line 366. For defense-in-depth, consider initializingcheckpointsif missing:🛡️ Optional defensive initialization
// Add to state + if (!this._state.checkpoints) { + this._state.checkpoints = []; + } this._state.checkpoints.push(checkpoint);Alternatively, throw a more descriptive error if state integrity is compromised. This is optional and can be addressed separately if needed.
🤖 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 - 366, The push to this._state.checkpoints can throw if this._state exists but checkpoints is undefined; in the saveCheckpoint method (or wherever this._state.checkpoints.push(checkpoint) is called) add a defensive guard that ensures this._state.checkpoints is an array before pushing (e.g. if (!Array.isArray(this._state.checkpoints)) this._state.checkpoints = [];), or alternatively throw a clear error indicating corrupted state; update the code around the this._state.checkpoints.push(checkpoint) call to perform that check/initialization to avoid a TypeError.
🤖 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/execution/build-state-manager.js:
- Around line 365-366: The push to this._state.checkpoints can throw if
this._state exists but checkpoints is undefined; in the saveCheckpoint method
(or wherever this._state.checkpoints.push(checkpoint) is called) add a defensive
guard that ensures this._state.checkpoints is an array before pushing (e.g. if
(!Array.isArray(this._state.checkpoints)) this._state.checkpoints = [];), or
alternatively throw a clear error indicating corrupted state; update the code
around the this._state.checkpoints.push(checkpoint) call to perform that
check/initialization to avoid a TypeError.
getLastCheckpoint() acessava this._state.checkpoints.length sem verificar se checkpoints existe. State carregado de JSON incompleto (sem a propriedade checkpoints) causa TypeError em runtime. Adiciona guard !this._state.checkpoints em ambos os métodos. Closes SynkraAI#513
|
@Pedrovaleriolopez @oalanicolas, null guard em getLastCheckpoint/getCheckpoint no build-state-manager — previne crash quando não há checkpoints salvos. Fix da issue #513. |
nikolasdehor
force-pushed
the
fix/build-state-manager-checkpoint-guard
branch
from
52824bf to
7295587
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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)
.aiox-core/core/execution/build-state-manager.js (2)
399-416:⚠️ Potential issue | 🟠 MajorThe getters are now defensive, but legacy/incomplete state files still fail before they can help.
This only masks the problem at the accessor layer.
loadState()still validatescheckpointsas required and throws on older/corruptedbuild-state.json, so the backwards-compatibility scenario described in the PR remains broken. The same invalid shape can also still tripsaveCheckpoint()/getStatus()later if_stateis injected directly.A safer fix is to normalize
state.checkpointsduring load/migration instead of only guarding reads here.Suggested direction
loadState() { if (!fs.existsSync(this.stateFilePath)) { return null; } try { const content = fs.readFileSync(this.stateFilePath, 'utf-8'); const state = JSON.parse(content); + // Backwards-compatible migration for older/incomplete state files + if (state.checkpoints == null) { + state.checkpoints = []; + } + // Validate const validation = validateBuildState(state); if (!validation.valid) { throw new Error(`Invalid state file: ${validation.errors.join(', ')}`); }As per coding guidelines, "Ensure backwards compatibility — core modules are consumed by all agents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/execution/build-state-manager.js around lines 399 - 416, loadState() currently enforces that state.checkpoints exists and throws on legacy/corrupt build-state.json, so defensive getters (getLastCheckpoint, getCheckpoint) only mask the symptom; normalize and sanitize checkpoints during load/migration instead: in loadState() ensure this._state is created if missing, coerce this._state.checkpoints to an array (e.g., [] when missing or not an array), remove any non-object entries and ensure each checkpoint has an id (or drop/repair entries), and update saveCheckpoint()/getStatus() to rely on the normalized this._state.checkpoints so callers no longer receive an invalid shape.
399-416:⚠️ Potential issue | 🟡 MinorAdd test coverage for
getCheckpoint()method.The
getLastCheckpoint()method is covered by the "should return null when no checkpoints" test in the Checkpoint Management suite. However,getCheckpoint(checkpointId)has no test coverage and should have explicit tests covering:
- Return of null when state is missing
- Return of null when checkpoints array is undefined/null
- Successful lookup by checkpoint ID
- Return of null when checkpoint ID is not found
Per coding guidelines, test coverage is required for new/modified functions in core modules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/execution/build-state-manager.js around lines 399 - 416, Add unit tests for the BuildStateManager.getCheckpoint(checkpointId) method: create tests that (1) assert getCheckpoint returns null when this._state is undefined/null, (2) assert it returns null when this._state.checkpoints is undefined or null, (3) assert it returns the correct checkpoint object when a checkpoint with the given id exists in this._state.checkpoints, and (4) assert it returns null when no checkpoint with the given id is found. Use the existing Checkpoint Management test suite pattern (the test that covers getLastCheckpoint) to instantiate BuildStateManager, set up or omit the _state/_state.checkpoints values, and call getCheckpoint(checkpointId) with appropriate ids to verify the expected outcomes.
🤖 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 @.aiox-core/core/execution/build-state-manager.js:
- Around line 399-416: loadState() currently enforces that state.checkpoints
exists and throws on legacy/corrupt build-state.json, so defensive getters
(getLastCheckpoint, getCheckpoint) only mask the symptom; normalize and sanitize
checkpoints during load/migration instead: in loadState() ensure this._state is
created if missing, coerce this._state.checkpoints to an array (e.g., [] when
missing or not an array), remove any non-object entries and ensure each
checkpoint has an id (or drop/repair entries), and update
saveCheckpoint()/getStatus() to rely on the normalized this._state.checkpoints
so callers no longer receive an invalid shape.
- Around line 399-416: Add unit tests for the
BuildStateManager.getCheckpoint(checkpointId) method: create tests that (1)
assert getCheckpoint returns null when this._state is undefined/null, (2) assert
it returns null when this._state.checkpoints is undefined or null, (3) assert it
returns the correct checkpoint object when a checkpoint with the given id exists
in this._state.checkpoints, and (4) assert it returns null when no checkpoint
with the given id is found. Use the existing Checkpoint Management test suite
pattern (the test that covers getLastCheckpoint) to instantiate
BuildStateManager, set up or omit the _state/_state.checkpoints values, and call
getCheckpoint(checkpointId) with appropriate ids to verify the expected
outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8130659-5d4e-4b3a-a975-35e43209b628
📒 Files selected for processing (1)
.aiox-core/core/execution/build-state-manager.js
Problema
Em
build-state-manager.js:400,getLastCheckpoint()acessathis._state.checkpoints.lengthdiretamente. Sethis._stateexiste mascheckpointséundefined(state carregado de JSON incompleto), ocorre TypeError: Cannot read property 'length' of undefined.Mesmo bug em
getCheckpoint()(linha 416): acessa.checkpoints.find()sem guard.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; }Validação
Closes #513
Summary by CodeRabbit