fix(master-orchestrator): previne divisão por zero em cálculo de progresso (#532)#536
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdded guards to progress calculations to return 0 when there are no non-on-demand epics, and updated completed-epics filtering to exclude on-demand epics. Tests were added to cover division-by-zero and on-demand exclusion edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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.
Pull request overview
Este PR corrige um edge case no cálculo de progresso do MasterOrchestrator quando todos os epics em EPIC_CONFIG estão marcados como onDemand, evitando divisão por zero e a propagação de NaN para consumidores (dashboard/logs/status).
Changes:
- Adiciona guard
if (totalEpics === 0) return 0em_calculateProgressFromState()egetProgressPercentage(). - Adiciona teste de regressão cobrindo o caso de todos os epics serem
onDemandparagetProgressPercentage().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.aios-core/core/orchestration/master-orchestrator.js |
Evita divisão por zero no cálculo de progresso. |
tests/core/master-orchestrator.test.js |
Adiciona teste de regressão para impedir retorno NaN em progresso quando totalEpics = 0. |
Comments suppressed due to low confidence (2)
.aios-core/core/orchestration/master-orchestrator.js:1362
completedEpicscounts all epics instate.epics, including on-demand epics, whiletotalEpicsexcludes on-demand epics. If an on-demand epic (e.g., Epic 5) completes, progress can become inaccurate or exceed 100%. FiltercompletedEpicsusing the same!EPIC_CONFIG[num].onDemandcriterion (e.g., count only completed epics whose epic number is non-on-demand).
const totalEpics = Object.keys(EPIC_CONFIG).filter((num) => !EPIC_CONFIG[num].onDemand).length;
if (totalEpics === 0) return 0;
const completedEpics = Object.values(state.epics).filter(
(epic) => epic.status === EpicStatus.COMPLETED,
).length;
.aios-core/core/orchestration/master-orchestrator.js:1419
- Same issue as above:
completedEpicscounts all epics (including on-demand) buttotalEpicsexcludes on-demand, sogetProgressPercentage()can report incorrect values if an on-demand epic completes. Align the numerator with the denominator by filtering completed epics to the non-on-demand set.
const totalEpics = Object.keys(EPIC_CONFIG).filter((num) => !EPIC_CONFIG[num].onDemand).length;
if (totalEpics === 0) return 0;
const completedEpics = Object.values(this.executionState.epics).filter(
(epic) => epic.status === EpicStatus.COMPLETED,
).length;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const originalConfig = {}; | ||
| for (const [num, cfg] of Object.entries(EPIC_CONFIG)) { | ||
| originalConfig[num] = cfg.onDemand; | ||
| cfg.onDemand = true; | ||
| } | ||
|
|
||
| try { | ||
| const progress = orchestrator.getProgressPercentage(); | ||
| expect(progress).toBe(0); | ||
| expect(Number.isNaN(progress)).toBe(false); | ||
| } finally { | ||
| // Restore original config | ||
| for (const [num, val] of Object.entries(originalConfig)) { | ||
| EPIC_CONFIG[num].onDemand = val; | ||
| } |
There was a problem hiding this comment.
This test mutates the shared EPIC_CONFIG object and restores onDemand values by assignment. When the original value was undefined, assigning it back leaves an onDemand property present with value undefined, which can subtly affect any code that enumerates config fields. Restore by deleting onDemand when it wasn’t originally set (or clone/mock EPIC_CONFIG for the test) to avoid altering global shape.
| const totalEpics = Object.keys(EPIC_CONFIG).filter((num) => !EPIC_CONFIG[num].onDemand).length; | ||
| if (totalEpics === 0) return 0; | ||
|
|
There was a problem hiding this comment.
A guard was added for the totalEpics === 0 edge case in _calculateProgressFromState, but there isn't a regression test covering this method. Consider adding a test that exercises _calculateProgressFromState via listSavedStates() (or directly) when all epics are temporarily set to onDemand, asserting progress is 0 (not NaN).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/master-orchestrator.test.js (1)
430-450: Good regression test with proper cleanup.The test correctly exercises the edge case by temporarily making all epics
onDemand: true. Thetry/finallypattern ensures config restoration even if assertions fail.One minor observation: the restoration sets
onDemandtoundefinedfor epics that originally didn't have the property, rather than deleting it. This is functionally equivalent for the filter logic (!EPIC_CONFIG[num].onDemand), but if you want a pristine restoration:♻️ Optional: Cleaner restoration using delete
} finally { // Restore original config for (const [num, val] of Object.entries(originalConfig)) { - EPIC_CONFIG[num].onDemand = val; + if (val === undefined) { + delete EPIC_CONFIG[num].onDemand; + } else { + EPIC_CONFIG[num].onDemand = val; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/master-orchestrator.test.js` around lines 430 - 450, The test's cleanup restores EPIC_CONFIG entries by assigning undefined for epics that originally lacked onDemand, leaving the property present; update the finally block to restore exactly by checking originalConfig[num] — if originalConfig[num] is undefined then delete EPIC_CONFIG[num].onDemand, otherwise set EPIC_CONFIG[num].onDemand = originalConfig[num]; this preserves a pristine EPIC_CONFIG state after the 'should return 0 when all epics are on-demand' test that calls orchestrator.initialize() and uses orchestrator.getProgressPercentage().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/master-orchestrator.test.js`:
- Around line 430-450: The test's cleanup restores EPIC_CONFIG entries by
assigning undefined for epics that originally lacked onDemand, leaving the
property present; update the finally block to restore exactly by checking
originalConfig[num] — if originalConfig[num] is undefined then delete
EPIC_CONFIG[num].onDemand, otherwise set EPIC_CONFIG[num].onDemand =
originalConfig[num]; this preserves a pristine EPIC_CONFIG state after the
'should return 0 when all epics are on-demand' test that calls
orchestrator.initialize() and uses orchestrator.getProgressPercentage().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/core/orchestration/master-orchestrator.jstests/core/master-orchestrator.test.js
Feedback endereçado (2 commits)Obrigado @copilot-pull-request-reviewer e @coderabbitai pelo feedback! Dois commits adicionais: Commit 1:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 947 🏁 Script executed: Length of output: 949 🏁 Script executed: Length of output: 1565 🏁 Script executed: Length of output: 1950
✅ Commit 1: Filtro de completedEpicsA mudança de // Agora numerador e denominador estão alinhados
const completedEpics = Object.entries(state.epics).filter(
([num, epic]) => epic.status === EpicStatus.COMPLETED && !EPIC_CONFIG[num]?.onDemand,
).length;Isso realmente previne que o progresso ultrapasse 100% quando on-demand epics são completados. O Copilot identificou um ponto válido! ✅ Commit 2: Cobertura de testes e cleanup
A PR está robusta e bem testada. Parabéns pela atenção aos detalhes! 🐰 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.aios-core/core/orchestration/master-orchestrator.js (1)
1354-1365: Avoid drift by reusing one progress-calculation path.The same progress logic now exists in two places; this is likely to diverge again in future changes.
♻️ Suggested refactor
getProgressPercentage() { - const totalEpics = Object.keys(EPIC_CONFIG).filter((num) => !EPIC_CONFIG[num].onDemand).length; - if (totalEpics === 0) return 0; - - const completedEpics = Object.entries(this.executionState.epics).filter( - ([num, epic]) => epic.status === EpicStatus.COMPLETED && !EPIC_CONFIG[num]?.onDemand, - ).length; - - return Math.round((completedEpics / totalEpics) * 100); + return this._calculateProgressFromState({ epics: this.executionState.epics }); }Also applies to: 1413-1422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/orchestration/master-orchestrator.js around lines 1354 - 1365, The progress calculation logic is duplicated; extract the common logic into a single helper (e.g., computeProgressFromState or reuse _calculateProgressFromState) and have all callers use it instead of duplicating code. Specifically, centralize the logic that uses EPIC_CONFIG and EpicStatus to compute totalEpics and completedEpics, return the rounded percentage, and replace the duplicate block (the shown _calculateProgressFromState and the other occurrence referenced at the end of the file) with calls to that helper so there is a single authoritative calculation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aios-core/core/orchestration/master-orchestrator.js:
- Around line 1358-1362: CI is failing due to an outdated manifest; run the
manifest generator and commit the change: execute the npm script (npm run
generate:manifest) to regenerate the project manifest, verify the changed
manifest file (the one used by validate:manifest) reflects the current source
(including updates around master-orchestrator.js where totalEpics,
completedEpics and EPIC_CONFIG are defined/used), add and commit the regenerated
manifest, and push the commit to unblock the pipeline.
- Around line 1360-1362: The counting of completed epics in the completedEpics
calculation (and the similar block around lines referencing EPIC_CONFIG at
1417-1419) incorrectly treats unknown epic IDs as valid because
!EPIC_CONFIG[num]?.onDemand is truthy for missing keys; update the filter to
first ensure the epic key exists in EPIC_CONFIG (e.g., check
EPIC_CONFIG.hasOwnProperty(num) or EPIC_CONFIG[num] != null) before evaluating
its onDemand flag, so only epics present in EPIC_CONFIG are considered when
computing completedEpics and the related progress calculation.
---
Nitpick comments:
In @.aios-core/core/orchestration/master-orchestrator.js:
- Around line 1354-1365: The progress calculation logic is duplicated; extract
the common logic into a single helper (e.g., computeProgressFromState or reuse
_calculateProgressFromState) and have all callers use it instead of duplicating
code. Specifically, centralize the logic that uses EPIC_CONFIG and EpicStatus to
compute totalEpics and completedEpics, return the rounded percentage, and
replace the duplicate block (the shown _calculateProgressFromState and the other
occurrence referenced at the end of the file) with calls to that helper so there
is a single authoritative calculation path.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/core/orchestration/master-orchestrator.jstests/core/master-orchestrator.test.js
| if (totalEpics === 0) return 0; | ||
|
|
||
| const completedEpics = Object.values(state.epics).filter( | ||
| (epic) => epic.status === EpicStatus.COMPLETED, | ||
| const completedEpics = Object.entries(state.epics).filter( | ||
| ([num, epic]) => epic.status === EpicStatus.COMPLETED && !EPIC_CONFIG[num]?.onDemand, | ||
| ).length; |
There was a problem hiding this comment.
CI is blocked by outdated manifest hash.
The pipeline reports validate:manifest failure for this file. Please regenerate and commit the manifest update before merge (npm run generate:manifest).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aios-core/core/orchestration/master-orchestrator.js around lines 1358 -
1362, CI is failing due to an outdated manifest; run the manifest generator and
commit the change: execute the npm script (npm run generate:manifest) to
regenerate the project manifest, verify the changed manifest file (the one used
by validate:manifest) reflects the current source (including updates around
master-orchestrator.js where totalEpics, completedEpics and EPIC_CONFIG are
defined/used), add and commit the regenerated manifest, and push the commit to
unblock the pipeline.
…resso Quando todos os epics em EPIC_CONFIG têm onDemand: true, totalEpics é 0 e a divisão retorna NaN. Adiciona guard em _calculateProgressFromState() e getProgressPercentage(), retornando 0 nesse caso. Inclui teste de regressão para o edge case. Closes SynkraAI#532
… fix) Addresses Copilot review feedback: completedEpics was counting ALL epics (including on-demand) while totalEpics excluded on-demand. This could cause progress to be inaccurate or exceed 100% when an on-demand epic completes. Fix: use Object.entries() + filter by !EPIC_CONFIG[num]?.onDemand in both _calculateProgressFromState() and getProgressPercentage().
- Fix test cleanup: use delete for properties that were not originally set instead of assigning undefined (Copilot + CodeRabbit feedback) - Add test: on-demand epics should not count toward progress calculation - Add test: _calculateProgressFromState returns 0 when all epics are on-demand
Unknown/legacy epic IDs persisted in state but absent from EPIC_CONFIG would pass the `!EPIC_CONFIG[num]?.onDemand` filter (since `undefined?.onDemand` is `undefined`, and `!undefined` is `true`), inflating completedEpics count above the totalEpics denominator. Add `EPIC_CONFIG[num] &&` guard in both `getProgressPercentage()` and `_calculateProgressFromState()` so only known epics are counted.
nikolasdehor
force-pushed
the
fix/master-orchestrator-division-by-zero
branch
from
9568e4a to
8363594
Compare
DevOps Review — @devops (Gage)Veredicto: CHANGES REQUESTED — 2 issues pendentes OK
Pendente1. Guard para keys inexistentes (Blocker): .filter(([num, epic]) => EPIC_CONFIG[num] && epic.status === EpicStatus.COMPLETED && !EPIC_CONFIG[num].onDemand)2. Duplicação de lógica (Recomendado): Refatorar 3. Manifest: Após fixes, merge imediato. ⚡ Gage — Repository Guardian |
Resumo
Corrige divisão por zero em
_calculateProgressFromState()egetProgressPercentage()quando todos os epics emEPIC_CONFIGtêmonDemand: true.Problema
Sem o guard,
totalEpics = 0→ divisão por zero →NaNpropaga para dashboard, logs e status.Correção
if (totalEpics === 0) return 0em ambos os métodosPlano de teste
getProgressPercentage()retorna0(nãoNaN) quando todos epics são onDemandCloses #532
Summary by CodeRabbit
Bug Fixes
Tests