fix(cue): harden cue.yaml chain validation + topo-sort chain subs in pipeline editor - #981
Merged
Merged
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis pull request enhances Cue pipeline chain validation and resolution for command-triggered agent subscriptions. It adds documentation guidance, a validator test for array shape alignment, implements dependency-aware topological ordering of chain subscriptions, detects missing upstream references with error nodes, and includes regression tests to prevent subscription misrouting in fan-in scenarios. ChangesCommand-Chain Source Subscription Validation & Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/maestro-cue-configuration.md (1)
205-209: ⚡ Quick winAdd explicit fan-in positional mapping rule in the checklist.
Line 205-Line 209 should also state that for fan-in chains,
source_subandsource_sessionarrays must be the same length and aligned by index (andsource_session_idstoo, when present). This is now validator-enforced and is easy to miss without a direct checklist callout.Suggested doc patch
1. Initial trigger subscription uses `action: command` with a valid `command` object. 2. The downstream `agent.completed` subscription includes `source_sub` pointing to that command subscription name. +3. For fan-in, if you use arrays, keep `source_sub` / `source_session` (and `source_session_ids` when present) equal-length and positionally aligned. -3. Keep `pipeline_name` consistent across all subs in the pipeline. -4. Keep per-node identity fields (`target_node_key`, `fan_out_node_keys`) stable once created. +4. Keep `pipeline_name` consistent across all subs in the pipeline. +5. Keep per-node identity fields (`target_node_key`, `fan_out_node_keys`) stable once created.🤖 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 `@docs/maestro-cue-configuration.md` around lines 205 - 209, Add a checklist item stating that in fan-in chains the arrays source_sub and source_session (and source_session_ids when present) must be the same length and positionally aligned; update the checklist lines referencing initial trigger subscription, downstream agent.completed subscription, pipeline_name, and node identity rules to include this rule so authors know the validator enforces index-wise alignment for fan-in mappings.src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts (1)
946-964: 💤 Low valueConsider extracting the missing-source error-node creation.
The branch at lines 946-964 (command-target) and lines 1041-1060 (agent-target) construct the same
missing-sourceerror node — same id pattern (error-source-sub-${sub.name}-${i}), same data shape, same position math. Same is also true of the oldermissing-sourcebranches at lines 973-990 and 1070-1088 that the PR didn't touch.Extracting a small
createMissingSourceErrorNode(sub, i, subRef, targetCol, existingRows)helper would consolidate four near-identical blocks and prevent future drift between the command-target and agent-target paths (e.g. if a new field is added toErrorNodeData).Not a blocker — purely a maintainability improvement and the duplication predates this PR.
Also applies to: 1041-1060
🤖 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/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts` around lines 946 - 964, The code duplicates identical "missing-source" error-node construction in multiple branches of yamlToPipeline.ts; add a helper function (e.g., createMissingSourceErrorNode(sub, i, targetCol, existingRows, LAYOUT)) that builds the error node id (`error-source-sub-${sub.name}-${i}`), composes the ErrorNodeData (reason:'missing-source', subscriptionName: sub.name, message using subRef), computes the position using LAYOUT and (targetCol, existingRows), calls createErrorNode and returns the node (and id if useful); then replace the four duplicated blocks (the spots that currently call createErrorNode and set nodeMap/sourceNode for missing-source — references: variables sub, i, subRef, targetCol, existingRows, nodeMap, sourceNode) to call the new helper and insert the returned node into nodeMap and assign to sourceNode.src/__tests__/main/cue/cue-yaml-loader.test.ts (1)
760-818: ⚡ Quick winConsider adding the symmetric shape-mismatch test.
The validator at lines 381-385 emits two distinct errors:
"source_sub" must be an array when "source_session" is an array(string sub, array session)"source_sub" must be a string when "source_session" is a string(array sub, string session)Only the second path is asserted here (lines 798-818). A test for the first path would lock in the symmetric error message and guard against accidental message drift.
♻️ Proposed additional test
+ it('rejects source_sub string when source_session is an array', () => { + const result = validateCueConfig({ + subscriptions: [ + { + name: 'fan-in-invalid-shape-2', + event: 'agent.completed', + source_session: ['A', 'B'], + source_sub: 'chain-a', + prompt: '{{CUE_SOURCE_OUTPUT}}', + }, + ], + }); + expect(result.valid).toBe(false); + expect(result.errors).toEqual( + expect.arrayContaining([ + expect.stringContaining( + '"source_sub" must be an array when "source_session" is an array' + ), + ]) + ); + });🤖 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 760 - 818, The tests for validateCueConfig are missing the symmetric shape-mismatch case: add a test (in src/__tests__/main/cue/cue-yaml-loader.test.ts) that passes a subscription with source_session as an array (e.g., ['A','B']) and source_sub as a plain string, invoke validateCueConfig, assert result.valid is false, and assert result.errors contains the expected message substring like '"source_sub" must be an array when "source_session" is an array' to mirror the existing test that covers the opposite mismatch; use the same test naming pattern as the other cases so it clearly ties to validateCueConfig and the agent.completed subscription validation.
🤖 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 `@docs/maestro-cue-configuration.md`:
- Around line 205-209: Add a checklist item stating that in fan-in chains the
arrays source_sub and source_session (and source_session_ids when present) must
be the same length and positionally aligned; update the checklist lines
referencing initial trigger subscription, downstream agent.completed
subscription, pipeline_name, and node identity rules to include this rule so
authors know the validator enforces index-wise alignment for fan-in mappings.
In `@src/__tests__/main/cue/cue-yaml-loader.test.ts`:
- Around line 760-818: The tests for validateCueConfig are missing the symmetric
shape-mismatch case: add a test (in
src/__tests__/main/cue/cue-yaml-loader.test.ts) that passes a subscription with
source_session as an array (e.g., ['A','B']) and source_sub as a plain string,
invoke validateCueConfig, assert result.valid is false, and assert result.errors
contains the expected message substring like '"source_sub" must be an array when
"source_session" is an array' to mirror the existing test that covers the
opposite mismatch; use the same test naming pattern as the other cases so it
clearly ties to validateCueConfig and the agent.completed subscription
validation.
In `@src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts`:
- Around line 946-964: The code duplicates identical "missing-source" error-node
construction in multiple branches of yamlToPipeline.ts; add a helper function
(e.g., createMissingSourceErrorNode(sub, i, targetCol, existingRows, LAYOUT))
that builds the error node id (`error-source-sub-${sub.name}-${i}`), composes
the ErrorNodeData (reason:'missing-source', subscriptionName: sub.name, message
using subRef), computes the position using LAYOUT and (targetCol, existingRows),
calls createErrorNode and returns the node (and id if useful); then replace the
four duplicated blocks (the spots that currently call createErrorNode and set
nodeMap/sourceNode for missing-source — references: variables sub, i, subRef,
targetCol, existingRows, nodeMap, sourceNode) to call the new helper and insert
the returned node into nodeMap and assign to sourceNode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05460e78-97a6-4659-b847-916bbe97baa1
📒 Files selected for processing (5)
docs/maestro-cue-configuration.mdsrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.tssrc/main/cue/config/cue-config-validator.tssrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts
Greptile SummaryThis PR fixes a phantom-agent bug in the Pipeline Editor where fan-in chain subs (carrying
Confidence Score: 4/5Safe to merge; the topo-sort fix is well-reasoned and the regression test validates the exact failure mode from the user bug report. The core algorithm change is correct and covered by a purpose-built regression test. Two non-blocking gaps exist: an error-node code path that has no direct test (meaning a future regression there would go unnoticed), and a validator error message that reads 'must be a string when source_session is a string' even when source_session is absent rather than a string. Neither affects current runtime behaviour. The new !knownSubNames.has(subRef) branch in yamlToPipeline.ts (both the command and agent-target paths) lacks a dedicated test; cue-config-validator.ts line 384 has a slightly inaccurate error message for the undefined source_session case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Split subs into initialTriggers / chainSubs] --> B[Sort initialTriggers by tieBreak]
B --> C[Build indegree + dependents maps\nfrom source_sub refs within chainSubNames]
C --> D{ready queue\nnon-empty?}
D -- yes --> E[Pick candidate with lowest tieBreak]
E --> F[Emit to orderedChainSubs\nDecrement dependents' indegree]
F --> G{any dependent\nreached 0?}
G -- yes --> H[Push to ready queue]
H --> D
G -- no --> D
D -- no --> I{consumed all\nchain subs?}
I -- yes --> J[sorted = initialTriggers + orderedChainSubs]
I -- no --> K[Cycle fallback: append\nremainders in tieBreak order]
K --> J
J --> L[Loop over sorted subs]
L --> M{source_sub\nresolved in subNameToNode?}
M -- yes --> N[Use existing node]
M -- no --> O{subRef NOT in\nknownSubNames?}
O -- yes --> P[Emit visible error node]
O -- no --> Q[Fall back to session-name\nresolution]
Reviews (1): Last reviewed commit: "fix(cue-renderer): topologically sort ch..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
Collaborator
|
Thanks for the contribution, @chr1syy! This is a really nice fix — the topological sort + validation hardening directly addresses the Unfortunately the PR currently has merge conflicts against |
chr1syy
added a commit
to chr1syy/Maestro
that referenced
this pull request
May 11, 2026
- Validator: narrow the source_sub shape-mismatch error to the *string* case explicitly so a missing source_session emits only the required-field error instead of the misleading "must be a string when source_session is a string" message (Greptile P2). - Docs: add the fan-in positional-alignment checklist item — when source_sub / source_session / source_session_ids are arrays they must be the same length and aligned by index (CodeRabbit nit). - Tests: add the symmetric shape-mismatch case (array session + string sub) and a regression locking in the "no misleading error when source_session is missing" behaviour (Greptile + CodeRabbit nit). - Tests: add two regression tests for the visible-error-node path — agent-target and command-target — so a future refactor can't silently revert `!knownSubNames.has(subRef)` back to the session-name fallback (Greptile P2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.errorNodes.test.ts (1)
307-341: ⚡ Quick winConsider adding edge verification for consistency.
This test successfully locks in error emission for the command-action branch. However, unlike the agent-target test (lines 298-305), it doesn't verify that downstream nodes wire to the error node. Adding similar edge assertions would strengthen the regression guard and ensure both code paths have equivalent coverage.
♻️ Optional: Add edge verification like the agent-target test
const [pipeline] = subscriptionsToPipelines(subs, sessions); const errs = errorNodes(pipeline.nodes); expect(errs).toHaveLength(1); const data = errs[0].data as ErrorNodeData; expect(data.reason).toBe('missing-source'); expect(data.message).toContain('Stale Ghost Sub'); + // Verify the command node wires to the error node, not a fallback + const cmdNode = pipeline.nodes.find( + (n) => n.type === 'command' && n.data?.subscriptionName === 'pipe-cmd-chain' + ); + expect(cmdNode).toBeDefined(); + const incomingToCmd = pipeline.edges.filter((e) => e.target === cmdNode!.id); + expect(incomingToCmd).toHaveLength(1); + expect(incomingToCmd[0].source).toBe(errs[0].id); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2298189f-0334-4fde-9a0c-1cf85dc05d35
📒 Files selected for processing (6)
docs/maestro-cue-configuration.mdsrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.errorNodes.test.tssrc/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.tssrc/main/cue/config/cue-config-validator.tssrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts
✅ Files skipped from review due to trivial changes (1)
- docs/maestro-cue-configuration.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts
- src/tests/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts
When a fan-in chain sub's `source_sub` referenced another chain with a higher `-chain-N` index, the chain-index sort processed the dependent first. Its source lookup in `subNameToNode` missed, fell back to session-name resolution, and invented a premature agent node — and when the real upstream later created its keyed target via `target_node_key`, the legacy sessionName dedup in `getOrCreateAgentNode` was bypassed, leaving a disconnected phantom agent next to the real one. This was the visible "unresolved upstream" symptom on canvases authored as Trigger → Cmd → Agent → fan-in. Replace the chain-index sort with Kahn's algorithm over `source_sub` dependencies. Chain index becomes a tie-breaker among ready subs and the cycle-fallback order, preserving deterministic layout. Added a regression test that mirrors the user-reported Obsidian Daily Pipe shape (chain-1/chain-2/chain-4 fan-in with `target_node_key` on every sub) — produces 4 agent nodes before the fix, 3 after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Validator: narrow the source_sub shape-mismatch error to the *string* case explicitly so a missing source_session emits only the required-field error instead of the misleading "must be a string when source_session is a string" message (Greptile P2). - Docs: add the fan-in positional-alignment checklist item — when source_sub / source_session / source_session_ids are arrays they must be the same length and aligned by index (CodeRabbit nit). - Tests: add the symmetric shape-mismatch case (array session + string sub) and a regression locking in the "no misleading error when source_session is missing" behaviour (Greptile + CodeRabbit nit). - Tests: add two regression tests for the visible-error-node path — agent-target and command-target — so a future refactor can't silently revert `!knownSubNames.has(subRef)` back to the session-name fallback (Greptile P2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pedramamini
force-pushed
the
fix/cue-renderer
branch
from
May 13, 2026 15:24
6d15e87 to
d2fcfc3
Compare
pedramamini
added a commit
that referenced
this pull request
May 14, 2026
…t keys CLI server discovery (main): - Start CLI server + publish discovery file early in whenReady so a later startup failure can't silently leave maestro-cli unable to connect (the symptom that previously required toggling Live Mode to recover). - Retry ensureCliServer on failure and verify the file is actually present on disk after each write. - Periodic watchdog re-publishes the discovery file if it goes missing or drifts out of sync with the running server. - Surface fatal whenReady errors to Sentry instead of swallowing them. FilePreview: - HTML files get a Globe toggle in the header that swaps source view for a sandboxed iframe (allow-scripts allow-popups allow-forms, no same-origin, no-referrer). State persists per-tab via tabStore. - Move PreviewTierChip into the header button cluster (new iconOnly mode) so the dedicated tier-chip bar is gone — same popover behavior. - HighlightedCodeEditor: whitespace pre + textarea wrap=off so markdown tables and other long lines scroll horizontally instead of fragmenting around `|` delimiters. Cue pipeline layout: - Match saved positions by stable identifiers first (trigger subscriptionName, agent/command nodeKey) and fall back to legacy index-based keys only for older saves. Fixes positions drifting onto the wrong nodes after the chain-sub topo-sort reorder (#981). Misc UX: - SshRemoteSelector: hide the share-to-project-dir toggle when SSH execution is active (it has no audience on the remote). - GroupChatMessages / TerminalOutput: tooltip hints include Shift+ArrowUp for previous-message navigation. Tests: - ensureCliServer retry + watchdog coverage. - semanticNodeKeys priority + YAML-reorder regression cases. - PreviewTierChip iconOnly mode. - FilePreview HTML render mode (toggle visibility, iframe sandbox). - tabStore setFileTabHtmlRenderMode. Claude ID: c9bb9f6b-2568-4b5d-a0db-8b1bb09a510e Maestro ID: 865ec2d1-8898-4a68-9d06-e13562d127e0
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
2 participants
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.
Summary
cue.yamlvalidation around command chains (source_subrequired when anagent.completedsub hasaction: command; fan-insource_sub/source_sessionarray shapes must match positionally).source_subpoints at an unknown subscription, instead of silently falling back to session-name resolution.source_subdependencies inyamlToPipeline.ts. The previous chain-index sort placed a fan-in chain before the chains it depended on; undertarget_node_key-keyed pipelines this produced a duplicate phantom agent node and left the real fan-in target disconnected on canvas. Chain index is retained as a tie-breaker for deterministic layout and as the cycle-fallback order.docs/maestro-cue-configuration.md.Reproduces the user-reported
Obsidian Daily Pipefailure mode (Schedule → `Digest/Git/Health Script` (cmd) → per-agent chain → fan-in to Copilot) where the editor showed unresolved upstream links instead of the configured graph.Test plan
vitest run src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts— 47/47 pass, including new regression testkeeps fan-in wiring intact when chains carry target_node_key and chain-index ordering misranks dependencies(failed before fix with 4 agent nodes; passes after with the expected 3).vitest run src/__tests__/renderer/components/CuePipelineEditor/utils/ src/__tests__/main/cue/cue-yaml-loader.test.ts— 489/489 pass.target_node_key) and confirm no phantom unresolved agent nodes.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests