fix(execution): corrige falsy-safe defaults no ResultAggregator#611
Open
nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
Open
fix(execution): corrige falsy-safe defaults no ResultAggregator#611nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
Conversation
…falsy-safe waveIndex 0 era tratado como undefined por causa de || (OR lógico que trata 0 como falsy). Substitui todas as ocorrências de || para defaults por ?? (nullish coalescing) que só substitui null/undefined. Afeta: waveIndex, startedAt, rootPath, reportDir, maxHistory, agentId, filesModified, duration, output.
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. |
github-actions
bot
added
area: agents
WalkthroughReplaced many logical OR ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
📝 Coding Plan
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 |
…nager Mesma classe de bug do ResultAggregator: operadores || tratam 0 como falsy, causando defaults incorretos quando o valor real é 0 (ex: duration, totalSubtasks, waveIndex). Corrige 11 ocorrências em 2 módulos.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
.aiox-core/core/execution/result-aggregator.js (1)
369-369:⚠️ Potential issue | 🟠 MajorIncomplete fix:
waveIndex || 'all'still treats0as falsy.The PR correctly preserves
waveIndex: 0in the aggregation logic, but this line still uses||, causing wave 0 reports to be namedwave-results-all.jsoninstead ofwave-results-0.json. This undermines the fix.🐛 Proposed fix
- const reportName = filename || `wave-results-${aggregation.waveIndex || 'all'}.json`; + const reportName = filename ?? `wave-results-${aggregation.waveIndex ?? 'all'}.json`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/execution/result-aggregator.js at line 369, The reportName assignment uses `||` which treats 0 as falsy, causing waveIndex 0 to become 'all'; update the expression that builds `reportName` (the `reportName` variable using `filename` and `aggregation.waveIndex`) to use a nullish check so 0 is preserved (e.g., use `aggregation.waveIndex ?? 'all'` or an explicit check for undefined/null) so waveIndex 0 yields `wave-results-0.json`.
🧹 Nitpick comments (1)
.aiox-core/core/execution/result-aggregator.js (1)
313-313: Minor inconsistency:filesModified || []still uses||.For consistency with the other changes in this file, consider updating to
??. While functionally equivalent here (empty arrays are truthy), aligning the pattern makes the codebase more predictable.♻️ Optional consistency fix
- const allFiles = tasks.flatMap((t) => t.filesModified || []); + const allFiles = tasks.flatMap((t) => t.filesModified ?? []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/execution/result-aggregator.js at line 313, Update the nullish-coalescing usage for consistency: in the expression that builds allFiles (the const allFiles = tasks.flatMap((t) => t.filesModified || []);), replace the fallback operator `||` with `??` so it reads use of `filesModified ?? []` inside the tasks.flatMap callback; keep the rest of the logic unchanged.
🤖 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/result-aggregator.js:
- Line 369: The reportName assignment uses `||` which treats 0 as falsy, causing
waveIndex 0 to become 'all'; update the expression that builds `reportName` (the
`reportName` variable using `filename` and `aggregation.waveIndex`) to use a
nullish check so 0 is preserved (e.g., use `aggregation.waveIndex ?? 'all'` or
an explicit check for undefined/null) so waveIndex 0 yields
`wave-results-0.json`.
---
Nitpick comments:
In @.aiox-core/core/execution/result-aggregator.js:
- Line 313: Update the nullish-coalescing usage for consistency: in the
expression that builds allFiles (the const allFiles = tasks.flatMap((t) =>
t.filesModified || []);), replace the fallback operator `||` with `??` so it
reads use of `filesModified ?? []` inside the tasks.flatMap callback; keep the
rest of the logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5533a5c7-bbb1-47c0-b3f8-63eb16f145a1
📒 Files selected for processing (2)
.aiox-core/core/execution/result-aggregator.js.aiox-core/install-manifest.yaml
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 @.aiox-core/core/execution/wave-executor.js:
- Line 67: Summary: The object property assignments use "tasks: context.tasks ??
[]" which lets non-array falsy values (e.g., '', false) pass through and later
break code like wave.tasks.map; normalize to arrays. Fix: replace uses of
"tasks: context.tasks ?? []" (and the similar occurrence around the second
mention) with a normalization that ensures an array, e.g. set tasks to
Array.isArray(context.tasks) ? context.tasks : [] (or wrap non-array single
values in [context.tasks] if intended), and update any construction sites that
create "wave" so wave.tasks is always an array before being iterated (reference
the property assignment where tasks is set and the downstream wave.tasks.map
usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74a87338-175c-44c3-b11a-319990df05d5
📒 Files selected for processing (3)
.aiox-core/core/execution/build-state-manager.js.aiox-core/core/execution/wave-executor.js.aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .aiox-core/install-manifest.yaml
| { | ||
| index: 1, | ||
| tasks: context.tasks || [], | ||
| tasks: context.tasks ?? [], |
There was a problem hiding this comment.
Normalize tasks to arrays before use to avoid runtime failures.
With ??, values like ''/false now pass through instead of falling back to []. Downstream, this can break at Line 94 (wave.tasks.map(...)) and during wave execution.
🛠️ Proposed fix
@@
- analysis = {
+ analysis = {
waves: [
{
index: 1,
- tasks: context.tasks ?? [],
+ tasks: Array.isArray(context.tasks) ? context.tasks : [],
},
],
};
@@
- this.emit('wave_started', {
+ const waveTasks = Array.isArray(wave.tasks) ? wave.tasks : [];
+ this.emit('wave_started', {
waveIndex: wave.index,
- tasks: wave.tasks.map((t) => ({ id: t.id, description: t.description })),
- totalTasks: wave.tasks.length,
+ tasks: waveTasks.map((t) => ({ id: t.id, description: t.description })),
+ totalTasks: waveTasks.length,
});
@@
- const tasks = wave.tasks ?? [];
+ const tasks = Array.isArray(wave.tasks) ? wave.tasks : [];As per coding guidelines, "Check for proper input validation on public API methods."
Also applies to: 151-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/execution/wave-executor.js at line 67, Summary: The object
property assignments use "tasks: context.tasks ?? []" which lets non-array falsy
values (e.g., '', false) pass through and later break code like wave.tasks.map;
normalize to arrays. Fix: replace uses of "tasks: context.tasks ?? []" (and the
similar occurrence around the second mention) with a normalization that ensures
an array, e.g. set tasks to Array.isArray(context.tasks) ? context.tasks : []
(or wrap non-array single values in [context.tasks] if intended), and update any
construction sites that create "wave" so wave.tasks is always an array before
being iterated (reference the property assignment where tasks is set and the
downstream wave.tasks.map usage).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumo
Substitui
||por??(nullish coalescing) em todos os defaults doResultAggregator.Problema
waveIndex: waveResults.waveIndex || waveResults.wavetrata0como falsy, causandowaveIndex: undefinedquando o valor real é0(primeira wave).Mesmo problema em:
rootPath,reportDir,maxHistory,agentId,filesModified,duration,output,startedAt.Fix
Substituição direta de
||por??em 15 ocorrências. O operador??só substitui quando o valor énullouundefined, preservando0,'', efalsecomo valores válidos.Arquivos
.aiox-core/core/execution/result-aggregator.jsPlano de teste
Summary by CodeRabbit