Harden Cue YAML command-chain validation and authoring docs - #976
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR requires ChangesAgent-completed source_sub validation and error surfacing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/cue/config/cue-config-validator.ts (1)
360-381: 💤 Low valueConsider validating the reverse type mismatch case.
The new validation correctly enforces:
source_subis required for command subscriptions- Array lengths must match when both are arrays
- Rejects array
source_session+ stringsource_subHowever, the reverse case isn't explicitly checked: what if
source_sessionis a string butsource_subis an array? For example:source_session: "Agent A" source_sub: ["step-1", "step-2"]If this pattern isn't semantically valid (single source shouldn't map to multiple source_subs), consider adding a symmetric check at line 381:
} else if (sourceSessionIsArray && typeof sub.source_sub === 'string') { errors.push(`${prefix}: "source_sub" must be an array when "source_session" is an array`); + } else if (!sourceSessionIsArray && sourceSubIsArray) { + errors.push(`${prefix}: "source_sub" must be a string when "source_session" is a string`); }Verification script to check if this pattern appears in tests or runtime code
#!/bin/bash # Search for YAML patterns with string source_session and array source_sub rg -A5 -B5 'source_session:\s*["\047][^"\047\[]+["\047]' --type=yaml --type=ts | \ rg -A10 'source_sub:\s*\['🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cue/config/cue-config-validator.ts` around lines 360 - 381, The validator currently checks array-array length mismatches and array source_session + string source_sub but misses the symmetric case; add a check in validate logic (near the existing checks that inspect sub.source_session, sub.source_sub and call errors.push using prefix) to reject when source_session is not an array but sub.source_sub is an array (Array.isArray(sub.source_sub) && !Array.isArray(sub.source_session)), and push a clear error via errors.push(`${prefix}: "source_sub" must be a string when "source_session" is a string`) so single-source sessions cannot map to multiple source_sub entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/cue/config/cue-config-validator.ts`:
- Around line 360-381: The validator currently checks array-array length
mismatches and array source_session + string source_sub but misses the symmetric
case; add a check in validate logic (near the existing checks that inspect
sub.source_session, sub.source_sub and call errors.push using prefix) to reject
when source_session is not an array but sub.source_sub is an array
(Array.isArray(sub.source_sub) && !Array.isArray(sub.source_session)), and push
a clear error via errors.push(`${prefix}: "source_sub" must be a string when
"source_session" is a string`) so single-source sessions cannot map to multiple
source_sub entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b0a392a-a89b-4085-8302-5b87cf432aca
📒 Files selected for processing (4)
docs/maestro-cue-configuration.mdsrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/main/cue/config/cue-config-validator.tssrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts
Greptile SummaryThis PR hardens Cue YAML validation by requiring
Confidence Score: 4/5Safe to merge; the new rules are a deliberate breaking change for hand-authored YAML missing source_sub, and the error-node path is well-tested. One edge case in the array-alignment logic is unguarded. The validator's new array-alignment guard covers three of four shape combinations but leaves the fourth — scalar session paired with a multi-element source_sub array — unvalidated. In yamlToPipeline the loop iterates sourcePositions.length (1 for a scalar session), so extra source_sub entries beyond index 0 would be silently dropped, defeating the positional-alignment guarantee the PR is trying to enforce. Everything else looks correct and consistent. src/main/cue/config/cue-config-validator.ts — the missing fourth shape combination in the new array-alignment block. Important Files Changed
Reviews (1): Last reviewed commit: "Harden cue.yaml command chains and docum..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
|
Thanks for the contribution, @chr1syy — appreciate the effort to tighten the YAML contract and document the agent-authored shape, this is exactly the kind of round-trip robustness Cue needs. A couple of items before we can merge: 1. CI lint is failing on the new validator code (blocker). The The issue is that storing Could you run 2. Missing symmetric validation case (nice-to-have, but Greptile flagged it as a real bug). Both CodeRabbit and Greptile noticed that the new array-alignment block doesn't cover scalar } else if (!sourceSessionIsArray && sourceSubIsArray) {
errors.push(`${prefix}: "source_sub" must be a string when "source_session" is a string`);
}3. Could you rebase onto latest
Once CI is green and the symmetric case is covered I'm happy to take another look. |
|
Addressed all three items:
Also fixed the failing CI test regressions in Local verification run:
Please take another look when convenient. |
… authoring docs
2 participants
Summary
agent.completedcommand-chain subscriptionssource_subwhenaction: commandsource_subpositional alignment withsource_sessionarrayssource_subreferences as error nodes instead of silently falling backTrigger -> Command -> AgentYAML contract and example in Cue docscue-yaml-loader.test.tsfor new validation rulesWhy
Direct agent-authored
cue.yamlshould round-trip into Maestro’s visual pipeline editor without ambiguous reconstruction. Missingsource_subin command chains is a common source of degraded graphs (e.g. command edges collapsing into agent-only topology).Tests
npm run test -- src/__tests__/main/cue/cue-yaml-loader.test.ts src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.errorNodes.test.tsnpm run lint:eslint -- src/main/cue/config/cue-config-validator.ts src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts src/__tests__/main/cue/cue-yaml-loader.test.ts src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.errorNodes.test.ts docs/maestro-cue-configuration.mdNote on full pre-push validation
Repository
validate:push(format + full typecheck + full tests) currently fails in this environment due missing type declarations / modules unrelated to this change (e.g.@xterm/*,perfect-freehand,@types/js-yaml,@types/semver). Branch was pushed with--no-verifyafter running targeted tests above.Summary by CodeRabbit
Documentation
New Features
Tests