docs(cue): multi-root pipelines guide; fix validator misfire on missing source_session - #986
docs(cue): multi-root pipelines guide; fix validator misfire on missing source_session#986chr1syy wants to merge 2 commits into
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR documents multi-root Cue pipeline support and validates ChangesMulti-root Cue Pipelines with source_sub Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
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 |
|
Thanks for the contribution, @chr1syy — the multi-root docs are a clear improvement, and the per-role placement table is exactly the kind of thing future hand-authoring agents will need. A couple of things worth resolving before this lands: PR description vs. actual diffThe description says:
…but the diff also includes:
Could you update the description (and ideally the PR title) so reviewers and the release notes step have an accurate picture? The current framing made me almost approve it as a doc tweak. Backward-incompatible validator change
if (sub.action === 'command' && sub.source_sub === undefined) {
errors.push(`${prefix}: "source_sub" is required for agent.completed subscriptions when action is "command"`);
}Any existing user A few questions:
If 1–3 are answered (intentional, no migration, only legacy hand-authored configs affected), I'm fine with this — just want it acknowledged rather than smuggled in under a docs PR. MergeabilityMergeable against |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/main/cue/cue-yaml-loader.test.ts (1)
108-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate this fixture to satisfy the new
source_subrequirement.
agent.completedsubscriptions withaction: 'command'are now invalid withoutsource_sub, so the loader should skiprelayhere andresult!.subscriptions[0]stops being a happy-path assertion. Add the upstream subscription name to keep this test exercising a valid CLI-command config.Suggested fixture update
subscriptions: - name: relay event: agent.completed source_session: researcher + source_sub: researcher-step action: command command: mode: cli🤖 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/__tests__/main/cue/cue-yaml-loader.test.ts` around lines 108 - 136, The test fixture must be updated so the command-action subscription includes the required source_sub field; in the YAML returned by mockReadFileSync inside the 'parses an action: command cli send subscription' test, add a source_sub: '<upstream-name>' (e.g. source_sub: 'upstream') to the relay subscription so loadCueConfig will treat it as valid and the subsequent assertions against result!.subscriptions[0] and its command (checked by the test) continue to exercise a valid CLI-command config.
🤖 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.
Inline comments:
In `@docs/maestro-cue-configuration.md`:
- Around line 76-99: Add documentation entries for the UUID-keyed fields
`source_session_ids` and `fan_out_ids` to the schema tables referenced as "Full
Schema" and "Optional Fields": describe both as arrays of agent UUID strings
(e.g., type: array of string, format: uuid), explain their semantics (stable
cross-root references used for chaining and fan-out, resilient to agent
renames), and note that they are preferred over the legacy `source_session` and
`fan_out` name-based fields; update any examples or table rows that list
`source_session`/`fan_out` to include the new `_ids` counterparts so readers can
see the expected YAML shape and usage.
In `@src/main/cue/config/cue-config-validator.ts`:
- Around line 369-385: The validator currently rejects mixed-shape source
mappings by erroring when source_session and source_sub differ in type; instead,
update the logic in cue-config-validator (the block handling sub.source_session
and sub.source_sub where sourceSessionIsArray and sourceSubIsArray are computed)
to only validate positional alignment when both sourceSession and sourceSub are
arrays (i.e., keep the length check when Array.isArray(sourceSession) &&
Array.isArray(sourceSub)), and remove the two else-if branches that push errors
for scalar/array combinations so scalar/array mixes are allowed; keep using the
existing prefix and errors array to report only the true positional-mismatch
case.
In `@src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts`:
- Line 535: source_sub resolution is order-dependent because you check
knownSubNames and call subNameToNode.get(subRef) while iterating sorted, so if
the producer appears later you miss it; restructure into two passes: first
iterate sorted to create all producer nodes and populate subNameToNode (and
knownSubNames) without resolving edges, then in a second pass resolve edges
using source_sub/subRef lookups (the logic currently around knownSubNames,
subNameToNode.get(subRef), and the source_sub handling), ensuring deferred edge
resolution also for the other affected blocks around the same pattern (the
sections noted at ~866-885 and ~961-980).
---
Outside diff comments:
In `@src/__tests__/main/cue/cue-yaml-loader.test.ts`:
- Around line 108-136: The test fixture must be updated so the command-action
subscription includes the required source_sub field; in the YAML returned by
mockReadFileSync inside the 'parses an action: command cli send subscription'
test, add a source_sub: '<upstream-name>' (e.g. source_sub: 'upstream') to the
relay subscription so loadCueConfig will treat it as valid and the subsequent
assertions against result!.subscriptions[0] and its command (checked by the
test) continue to exercise a valid CLI-command config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1fb6710-1c2d-47c3-95d0-9daa152562ac
📒 Files selected for processing (7)
docs/maestro-cue-configuration.mddocs/maestro-cue-examples.mddocs/maestro-cue.mdsrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/main/cue/config/cue-config-validator.tssrc/prompts/_maestro-cue.mdsrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts
Greptile SummaryDespite the PR description calling this a "Docs-only diff", the change includes meaningful code additions across the validator, pipeline renderer, and test suite alongside the documentation. The documentation correctly captures the per-agent-cwd discovery model and adds clear role tables for multi-root authoring.
Confidence Score: 3/5The documentation additions are accurate and well-structured, but the new validation rules introduce a breaking change for existing YAML files and a doc/validator mismatch that will cause agents following the new guidance to produce configs that fail validation. The new 'prefer source_session_ids over source_session' prose in both the agent prompt and the user docs directly contradicts the validator's hard requirement for source_session on every agent.completed subscription. Any agent or user that reads the new guidance and omits source_session while supplying only source_session_ids will get a validation error with no clear signal in the docs explaining why. Additionally, the new source_sub required for command rule silently invalidates existing cue.yaml files that lack source_sub on agent.completed + command subscriptions, with no migration note. src/prompts/_maestro-cue.md and docs/maestro-cue-configuration.md need the source_session / source_session_ids guidance clarified; src/main/cue/config/cue-config-validator.ts needs the alignment-check guard tightened and a note about the new source_sub requirement being a breaking change. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[agent.completed subscription\naction: command] -->|new rule| B{source_sub\npresent?}
B -->|No| C[Validation error: source_sub required]
B -->|Yes| D{source_sub in knownSubNames?}
D -->|Found in subNameToNode| E[Resolve to upstream node]
D -->|Not in knownSubNames| F[Render error node: missing-source]
D -->|Known name, resolves later| G[Session-name fallback lookup]
H[source_session array + source_sub array] -->|new rule| I{lengths match?}
I -->|No| J[length mismatch error]
I -->|Yes| E
K[source_session string + source_sub array] -->|new rule| L[must-be-string error - also fires when source_session is undefined]
Reviews (1): Last reviewed commit: "Document per-agent-cwd model for multi-r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
| if (sub.action === 'command' && sub.source_sub === undefined) { | ||
| errors.push( | ||
| `${prefix}: "source_sub" is required for agent.completed subscriptions when action is "command"` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new check makes source_sub required whenever event === 'agent.completed' and action === 'command'. The PR description states "Docs-only diff (+67 / -2). No code, no tests touched." but this introduces a breaking validator rule: any existing cue.yaml with an agent.completed + action: command subscription lacking source_sub will now fail validation. There is no migration note documented. If intentional, the PR description and the configuration reference should call it out as a required field addition for this subscription shape.
chr1syy
changed the title
The engine reads only <projectRoot>/.maestro/cue.yaml for each agent and never walks parents, so a single root cue.yaml cannot manage a fleet of agents living at distinct project roots. Document the correct authoring pattern: each subscription lives in its owning agent's local cue.yaml, cross-agent chains stitch at runtime via source_session_ids / fan_out_ids, and orchestration / fan-in subs live with their target agent (which is the workspace root only when the orchestrator's cwd happens to sit there). - src/prompts/_maestro-cue.md: add Multi-Root Pipelines section above Shared Workspaces with per-role placement table and hand-authoring checklist; update Configuration File and Authoring Guidance to point at it. - docs/maestro-cue-configuration.md: add Note callout under File Location and a Multi-root pipelines section after Sharing a workspace across agents. - docs/maestro-cue.md: clarify Configuration File copy and link to the new section. - docs/maestro-cue-examples.md: add Note on the CI-Style Pipeline example calling out that its three yamls are the multi-root pattern (vs. a same-root setup with agent_id routing in one file). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ture Resolves doc/validator mismatch flagged by Greptile + CodeRabbit, the misfiring shape-check guard, and the now-invalid CLI-send loader fixture. - Validator (cue-config-validator.ts): skip the source_sub/source_session type-shape check when source_session is undefined. The required-field check above already errors; re-emitting "source_sub must be a string when source_session is a string" against an undefined source_session was misleading noise. - Validator test: add regression case asserting only the required-field error fires when source_session is missing (no misleading shape error). - Loader fixture (cue-yaml-loader.test.ts:108-136): add source_sub: researcher-step to the CLI-send fixture so the YAML is semantically valid under the new agent.completed + action: command rule. Assert source_sub round-trips through the loader. - Docs (_maestro-cue.md, maestro-cue-configuration.md): reframe source_session_ids / fan_out_ids as REQUIRED COMPANIONS to source_session / fan_out, not replacements. Validator requires source_session on every agent.completed sub; the _ids fields are additional UUID arrays for rename stability (dispatcher prefers ids at lookup time, falls back to names). - Schema reference (maestro-cue-configuration.md): add source_session_ids, source_sub, and fan_out_ids to both the Full Schema YAML block and the Optional Fields table — they were documented in prose but missing from the schema reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d660fd9 to
e87a8db
Compare
chr1syy
changed the title
|
Thanks for the careful read, @pedramamini — and apologies for the misframing on the original PR description. Walking through what happened and what's now in the PR: Why the diff was bigger than "docs-only"The Looking at I've rebased this branch onto current
The PR now shows 6 files / +139 / -32. No code duplication with #976. Review-feedback fixes in the second commitBoth Greptile and CodeRabbit flagged two things in #976's validator that I went ahead and fixed here since they're directly downstream of #976's
Also patched the loader fixture at Re: your three questions on the
|
|
Closing in favor of #987 — same final tree, fresh branch, clean PR history. This PR accumulated noise from two force-pushes: the branch originally carried two prior commits ( See #987. |
|
✅ Actions performedReviews paused. |
|
Thanks for the heads-up — closing out review activity here. The fixes and docs look solid; will pick up in #987. Tip: You can customize Greptile's behavior for this repo with |
2 participants
Summary
Documents the per-agent-cwd model for multi-root Cue pipelines and fixes the doc/validator mismatch + misfiring shape-check guard flagged by reviewers. Updated from the original framing per @pedramamini's feedback — the validator + tests + renderer pieces that the original PR included are already on
rcvia #976, so this branch was rebased onto currentupstream/rcand now carries only the genuinely new docs and review-fix work.Multi-root pipelines docs (commit 1)
Document that the Cue engine reads only
<projectRoot>/.maestro/cue.yamlper agent (no parent walk), so a single rootcue.yamlis not a valid pattern for fleets spanning multiple project roots. Add per-role placement tables, hand-authoring checklists, and a<Note>callout under File Location.Review-feedback fixes (commit 2)
source_session_ids/fan_out_idsas required companions tosource_session/fan_out, not replacements (Greptile + CodeRabbit caught the doc/validator mismatch — the validator unconditionally requiressource_session, so the prior "prefer ids over names" prose would push agents to write invalid YAML).source_session_ids,source_sub, andfan_out_idsto both the Full Schema YAML block and the Optional Fields table — they were documented in prose but missing from the schema reference.cue-config-validator.ts): skip thesource_sub/source_sessiontype-shape consistency check whensource_sessionis undefined. The required-field check already errors on missingsource_session; re-emitting "must be a string when source_session is a string" against an undefined value was misleading noise (Greptile + CodeRabbit).cue-yaml-loader.test.ts:108-136): addsource_sub: researcher-stepto the CLI-send fixture so the YAML remains semantically valid under the post-Harden Cue YAML command-chain validation and authoring docs #976agent.completed+action: commandrule (CodeRabbit's outside-diff "Quick win"); assertsource_subround-trips through the loader.source_sessionis missing — locks in the misfire fix.Files changed (6)
docs/maestro-cue-configuration.md— Multi-root section + schema reference updates.docs/maestro-cue.md— Configuration File copy now points at the new section.docs/maestro-cue-examples.md— Note on the CI-Style Pipeline example clarifying the multi-root pattern.src/prompts/_maestro-cue.md— Multi-Root Pipelines section +_idscompanion-field framing.src/main/cue/config/cue-config-validator.ts— type-shape guard fix (skip whensource_sessionundefined).src/__tests__/main/cue/cue-yaml-loader.test.ts— fixture update + regression case for the misfire fix.Re: pedram's questions on the now-landed
source_subrule (#976)That rule lives on
rcalready, but for the release-notes loop:Cmd(owner=S) → Agent(S)collapses toAgent(S) → Agent(S)), which is worse than a load-time validation message that points at the missing field.source_subfor Command nodes, so the affected surface is pre-existing hand-authored YAMLs that combineevent: agent.completed+action: commandwithoutsource_sub.source_sub: <upstream-name>before upgrading. Happy to add a CHANGELOG entry if you want.Test plan
npx vitest run src/__tests__/main/cue/cue-yaml-loader.test.ts— 143 pass, 0 fail (includes new validator regression case).npx vitest run src/__tests__/renderer/components/CuePipelineEditor/utils/— 346 pass, 0 fail.lint-and-format+testgreen on the latest push.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
.maestro/cue.yaml(no parent/ancestor fallback)source_session_ids,source_sub, andfan_out_idsBug Fixes