fix: yaml-validator getMaxDepth trata null como nível extra de profundidade#514
fix: yaml-validator getMaxDepth trata null como nível extra de profundidade#514nikolasdehor wants to merge 3 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
WalkthroughFixes YAML validator's getMaxDepth to avoid recursing into null values by requiring Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the YAML validator where null values were incorrectly counted as additional nesting levels. The root cause is JavaScript's quirk where typeof null === 'object', causing the getMaxDepth() method to recurse on null values. This could trigger false warnings about deep nesting in valid YAML configurations.
Changes:
- Added null check to
getMaxDepth()method in yaml-validator.js (line 264) - Added comprehensive test suite with 16 test cases covering the fix and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.aios-core/core/utils/yaml-validator.js |
Added && value !== null check before recursing on object values in getMaxDepth() method |
tests/core/utils/yaml-validator-depth.test.js |
New test file with 16 tests covering empty objects, nested objects, null handling, arrays, and deep nesting scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let maxDepth = currentDepth; | ||
| for (const value of Object.values(obj)) { | ||
| if (typeof value === 'object') { | ||
| if (typeof value === 'object' && value !== null) { |
There was a problem hiding this comment.
The fix correctly addresses the bug where null values were incorrectly counted as nested objects. However, there are duplicate copies of this file in .aios-core/development/scripts/yaml-validator.js (line 261) and .aios-core/infrastructure/scripts/yaml-validator.js (line 261) that have the same getMaxDepth method with the same bug, but are not fixed in this PR. These duplicate files should also receive the same fix to maintain consistency across the codebase.
|
|
||
| test('profundidade correta com array contendo objetos', () => { | ||
| const obj = { list: [{ a: 1 }, { b: { c: 2 } }] }; | ||
| // list = 1, list[1].b = 2 + list = 3 |
There was a problem hiding this comment.
The comment explaining the depth calculation could be clearer. The current comment "list = 1, list[1].b = 2 + list = 3" is somewhat confusing. A clearer explanation would be: "list array is 1 level deep, objects in array add 1 level (total 2), nested object b.c adds 1 more level (total 3)".
| // list = 1, list[1].b = 2 + list = 3 | |
| // list array is 1 level deep, objects in array add 1 level (total 2), nested object b.c adds 1 more level (total 3) |
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/infrastructure/scripts/yaml-validator.js (1)
1-1:⚠️ Potential issue | 🔴 CriticalCI blocker: regenerate the manifest before merging.
The pipeline is failing with:
Manifest is OUTDATED. Modified file with hash mismatch.Run
npm run generate:manifestlocally, commit the updated manifest, and re-push. This affects both modified files and must be resolved before the PR can be merged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/infrastructure/scripts/yaml-validator.js at line 1, The CI is failing due to an outdated manifest; run the npm script "generate:manifest" locally to regenerate the manifest artifact, verify the updated manifest reflects the latest changes (resolving "Manifest is OUTDATED. Modified file with hash mismatch."), add and commit the regenerated manifest file to the branch, and push the commit so the pipeline can pass..aios-core/development/scripts/yaml-validator.js (1)
1-397: 🛠️ Refactor suggestion | 🟠 MajorConsolidate three diverging copies of
YAMLValidatorinto a single re-export from the canonical core utility.The three copies do exist in
.aios-core/core/utils/yaml-validator.js,.aios-core/development/scripts/yaml-validator.js, and.aios-core/infrastructure/scripts/yaml-validator.js. They are already diverging in meaningful ways:
- Infrastructure copy: Uses undeclared
const keyat line 330 (unused variable without prefix)- Development copy: Uses
const _keyat the same location (prefixed to signal intentional non-use)- Core version: More modernized implementation with
Object.hasOwn()instead ofhasOwnProperty(), proper JSDoc documentation, and different code patternsThe infrastructure copy represents a maintenance risk—it has an undeclared variable that should either be removed or properly prefixed. Rather than patching each copy separately, replace both the development and infrastructure variants with a simple re-export:
Suggested consolidation
-/** - * YAML Validator for AIOS Developer Meta-Agent - * ... - */ -const yaml = require('js-yaml'); -const fs = require('fs-extra'); - -class YAMLValidator { ... } - -module.exports = YAMLValidator; +module.exports = require('../../core/utils/yaml-validator');(Adjust the relative path to match each script's location.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/development/scripts/yaml-validator.js around lines 1 - 397, The file contains a divergent local implementation of the YAMLValidator class (symbols: class YAMLValidator, methods validate, validateFile, validateStructure, validateFieldStructure, validateFieldTypes, getMaxDepth, fixIndentation, fixQuotes) and must be consolidated to re-export the single canonical implementation from the core utility; replace the local class and its code with a simple re-export of the canonical YAMLValidator module (i.e. export/require the core YAMLValidator) so both development and infrastructure copies no longer diverge—if you must keep any local code, remove the undeclared/unused variable (the stray const key) and convert any hasOwnProperty uses to Object.hasOwn() to match the core implementation.
♻️ Duplicate comments (1)
.aios-core/development/scripts/yaml-validator.js (1)
254-268: Same correct fix as the infrastructure copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/development/scripts/yaml-validator.js around lines 254 - 268, Update getMaxDepth to match the infrastructure copy: treat any non-null object (including arrays) as a level by returning currentDepth + 1 for object nodes and recurse into all children while properly incrementing depth; specifically adjust the logic in getMaxDepth (and its recursive call this.getMaxDepth) so the root object counts as depth 1 and nested objects increment depth correctly instead of returning the currentDepth prematurely for object nodes.
🤖 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/development/scripts/yaml-validator.js:
- Around line 1-397: The file contains a divergent local implementation of the
YAMLValidator class (symbols: class YAMLValidator, methods validate,
validateFile, validateStructure, validateFieldStructure, validateFieldTypes,
getMaxDepth, fixIndentation, fixQuotes) and must be consolidated to re-export
the single canonical implementation from the core utility; replace the local
class and its code with a simple re-export of the canonical YAMLValidator module
(i.e. export/require the core YAMLValidator) so both development and
infrastructure copies no longer diverge—if you must keep any local code, remove
the undeclared/unused variable (the stray const key) and convert any
hasOwnProperty uses to Object.hasOwn() to match the core implementation.
In @.aios-core/infrastructure/scripts/yaml-validator.js:
- Line 1: The CI is failing due to an outdated manifest; run the npm script
"generate:manifest" locally to regenerate the manifest artifact, verify the
updated manifest reflects the latest changes (resolving "Manifest is OUTDATED.
Modified file with hash mismatch."), add and commit the regenerated manifest
file to the branch, and push the commit so the pipeline can pass.
---
Duplicate comments:
In @.aios-core/development/scripts/yaml-validator.js:
- Around line 254-268: Update getMaxDepth to match the infrastructure copy:
treat any non-null object (including arrays) as a level by returning
currentDepth + 1 for object nodes and recurse into all children while properly
incrementing depth; specifically adjust the logic in getMaxDepth (and its
recursive call this.getMaxDepth) so the root object counts as depth 1 and nested
objects increment depth correctly instead of returning the currentDepth
prematurely for object nodes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.aios-core/development/scripts/yaml-validator.js.aios-core/infrastructure/scripts/yaml-validator.js
typeof null === 'object' é um quirk do JavaScript. O loop em getMaxDepth() usava apenas typeof value === 'object' para decidir recursão, fazendo null inflar a profundidade calculada em +1. Adiciona guard value !== null para tratar null como folha. Closes SynkraAI#512
nikolasdehor
force-pushed
the
fix/yaml-validator-null-depth
branch
from
7d33bc4 to
347fac7
Compare
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)
.aios-core/core/utils/yaml-validator.js (1)
257-271:⚠️ Potential issue | 🟠 MajorCode fix is correct, but test file was not found in repository.
The addition of
&& value !== nullat line 264 correctly preventsnullvalues from being recursed into during depth calculation, mirroring the guard at line 258. This resolves the false positivedeep_nestingwarnings.However, despite the PR claim of 16 added tests, no test file exists in the repository for the
getMaxDepth()method. Per coding guidelines for CRITICAL PATH core modules, test coverage is required for new/modified functions. Please add tests covering:
- Null values mixed with nested objects
- Arrays containing null
- Pure null input
- Deep nested structures with null at various levels
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/utils/yaml-validator.js around lines 257 - 271, The PR added a null-guard to getMaxDepth but no tests were committed; add a new test file that exercises the getMaxDepth function with the required cases: (1) objects containing nulls mixed with nested objects, (2) arrays containing null values, (3) a pure null input, and (4) deeply nested objects with nulls at various levels; for each case assert the expected depth returned by getMaxDepth and include descriptive test names so CI verifies the deep_nesting fix.
🤖 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/utils/yaml-validator.js:
- Around line 257-271: The PR added a null-guard to getMaxDepth but no tests
were committed; add a new test file that exercises the getMaxDepth function with
the required cases: (1) objects containing nulls mixed with nested objects, (2)
arrays containing null values, (3) a pure null input, and (4) deeply nested
objects with nulls at various levels; for each case assert the expected depth
returned by getMaxDepth and include descriptive test names so CI verifies the
deep_nesting fix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.aios-core/install-manifest.yaml (1)
1-3: Auto-generated manifest — ensure it was regenerated via the documented command.The header states this file should be regenerated with
npm run generate:manifest. Confirm this command was executed after modifyingyaml-validator.jsto ensure all hashes are accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/install-manifest.yaml around lines 1 - 3, The install manifest header says it must be regenerated; after changing yaml-validator.js you need to run the manifest generator and commit the new file: run the documented command `npm run generate:manifest` (which executes scripts/generate-install-manifest.js), verify .aios-core/install-manifest.yaml was updated with new hashes, and include that regenerated file in your PR so hashes match the modified yaml-validator.js; re-run tests/CI to confirm no drift.
🤖 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/install-manifest.yaml:
- Around line 1-3: The install manifest header says it must be regenerated;
after changing yaml-validator.js you need to run the manifest generator and
commit the new file: run the documented command `npm run generate:manifest`
(which executes scripts/generate-install-manifest.js), verify
.aios-core/install-manifest.yaml was updated with new hashes, and include that
regenerated file in your PR so hashes match the modified yaml-validator.js;
re-run tests/CI to confirm no drift.
Copilot detectou que development/scripts/ e infrastructure/scripts/ contêm cópias do yaml-validator.js com o mesmo bug typeof null. Aplica a mesma correção && value \!== null em ambas.
Rebase + Manifest Necessários@nikolasdehor Este PR tem conflitos de merge (provavelmente Ações necessárias:
O fix do null depth está aprovado — merge imediato após essas ações. — Gage (DevOps) |
Descrição
Corrige bug no cálculo de profundidade de objetos YAML onde valores
nulleram contados como nível adicional de aninhamento.Em JavaScript,
typeof null === 'object'— isso fazia ogetMaxDepth()tratarnullcomo um objeto e fazer recursão desnecessária, inflando o contador de profundidade. Configs YAML com valoresnull(muito comum) podiam disparar falsos warnings dedeep_nesting.Mudança
for (const value of Object.values(obj)) { - if (typeof value === 'object') { + if (typeof value === 'object' && value !== null) { const depth = this.getMaxDepth(value, currentDepth + 1);Testes
16 testes unitários para
getMaxDepth()cobrindo:Closes #512
Summary by CodeRabbit