fix(cue): harden engine safety, fill test coverage gaps, and design group chat integration#578
fix(cue): harden engine safety, fill test coverage gaps, and design group chat integration#578
Conversation
…mprehensive test coverage - Add circular chain detection (MAX_CHAIN_DEPTH=10) to prevent infinite A→B→A loops - Clean event queue and scheduledFiredKeys on session teardown/refresh - Generate separate runId for output prompt phase with independent DB tracking - Add GitHub poller seed marker on first-poll failure to prevent event flooding - Validate watch glob patterns via picomatch in YAML loader - Warn at setup time when prompt_file references a missing file - Add 42 new tests: multi-hop chains, session lifecycle, template variables, config hotloading, output prompt edge cases, filter/GitHub/concurrency stress
📝 WalkthroughWalkthroughAdds chain-depth tracking (MAX_CHAIN_DEPTH) to Cue completion flows and threads chainDepth through dispatch/execute/fan-in/out and output-prompt flows; seeds GitHub on first poll failure to avoid event floods; adds picomatch-based glob validation for watch patterns; large set of new/additive Cue unit tests and two .gitignore entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as CueEngine
participant DB as CueDB
participant Output as OutputPromptRun
participant Down as DownstreamSubscription
Engine->>Engine: Main run completes (notifyAgentCompleted)
Engine->>Engine: Read/compute chainDepth
alt chainDepth <= MAX_CHAIN_DEPTH
Engine->>Output: Spawn output-prompt run (distinct runId, outputPromptPhase: true)
Output->>DB: Persist output-run status & stdout
Output->>Engine: Return output stdout
Engine->>DB: Record dispatch event with chainDepth+1 and sourceOutput
Engine->>Down: Dispatch downstream events (chainDepth+1)
Down->>Down: Process downstream run(s)
else chainDepth exceeded
Engine->>Engine: Abort downstream dispatch (drop chain)
end
sequenceDiagram
participant Poller as GitHubPoller
participant DB as CueDB
participant Later as SubsequentPolls
Poller->>Poller: First poll attempt (firstPollAttempted=false)
Poller->>Poller: Poll fails
Poller->>DB: Try markGitHubItemSeen("__seed_marker__") (non-fatal)
DB-->>Poller: Ack or error (ignored)
Poller->>Poller: Set firstPollAttempted=true (finally)
Later->>Poller: Subsequent poll succeeds and compares items against seed to avoid flood
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR is a broad hardening pass on the Maestro Cue engine: 7 code fixes and 42 new tests across 11 files. The majority of fixes (queue cleanup on teardown, scheduledFiredKeys pruning, GitHub first-run resilience, glob validation, prompt-file warnings, and the separate output-prompt Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant E as CueEngine
participant A as doExecuteCueRun(A)
participant B as doExecuteCueRun(B)
participant CD as chainDepth (shared state)
Note over E,CD: Fix 1 — chainDepth guard (async circular A→B→A)
E->>CD: chainDepth++ (=1)
E->>A: executeCueRun → doExecuteCueRun(A) [no await]
A-->>A: await onCueRun(A) — yields to event loop
E->>CD: chainDepth-- (=0, before A completes!)
Note over CD: chainDepth resets to 0 BEFORE async work runs
A-->>A: onCueRun(A) resolves (microtask)
A->>CD: chainDepth++ (=1)
A->>B: executeCueRun → doExecuteCueRun(B) [no await]
B-->>B: await onCueRun(B) — yields
A->>CD: chainDepth-- (=0)
B-->>B: onCueRun(B) resolves (microtask)
B->>CD: chainDepth++ (=1)
B->>A: executeCueRun → doExecuteCueRun(A) [no await]
Note over CD: MAX_CHAIN_DEPTH=10 never reached — loop continues
Note over E,CD: Fix 5 — outputRunId tracking gap
E->>A: doExecuteCueRun (runId added to activeRuns)
A-->>A: await onCueRun(runId) resolves
A->>A: generate outputRunId (NOT added to activeRuns)
A-->>A: await onCueRun(outputRunId)
Note over A: stopRun(outputRunId) → activeRuns.get() = undefined → returns false
Note over A: manuallyStoppedRuns.has(outputRunId) always false (dead code)
Last reviewed commit: a01c39b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/main/cue/cue-engine.ts
Outdated
| this.chainDepth++; | ||
| if (this.chainDepth > MAX_CHAIN_DEPTH) { | ||
| this.deps.onLog( | ||
| 'error', | ||
| `[CUE] Max chain depth (${MAX_CHAIN_DEPTH}) exceeded — aborting to prevent infinite loop` | ||
| ); | ||
| this.chainDepth--; | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| this.notifyAgentCompletedInner(sessionId, completionData); | ||
| } finally { | ||
| this.chainDepth--; | ||
| } |
There was a problem hiding this comment.
Chain depth guard only protects synchronous recursion, not async loops
The chainDepth counter is incremented and then decremented back to 0 before any async work can complete. Since doExecuteCueRun is async and yields at await this.deps.onCueRun(...), by the time that downstream run finishes and calls notifyAgentCompleted again (in its own finally block), the chain depth is already back to 0.
For a circular A→B→A pipeline:
notifyAgentCompleted('A')—chainDepth= 0→1dispatchSubscription→executeCueRun→doExecuteCueRun(B)launched as a floating async call, yields at firstawaitchainDepth--→ back to 0 before B even runs
The guard therefore never triggers for async circular chains. The maximum depth ever observed is 1, well below MAX_CHAIN_DEPTH = 10. The new test for this case confirms the issue: it explicitly calls engine.stop() inside the mock to manually break the loop, with a comment that reads:
"With async onCueRun each hop runs in a separate microtask, so we cap the mock to stop the engine after a bounded number of calls"
This means circular A→B→A pipelines still run indefinitely in production (consuming concurrency slots and resources), just via microtasks rather than causing a synchronous stack overflow.
A more effective guard would track which sessions are part of the current chain (e.g., a Set<string> of session IDs added when a completion triggers a downstream run, cleared when the downstream run finishes) or use a per-subscription cooldown/run-rate limiter.
src/main/cue/cue-engine.ts
Outdated
| if (this.manuallyStoppedRuns.has(runId) || this.manuallyStoppedRuns.has(outputRunId)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
outputRunId is never in activeRuns — manual-stop check is dead code
outputRunId is generated and used for the DB record and onCueRun call, but it is never inserted into this.activeRuns. Therefore:
stopRun(outputRunId)will always returnfalse(becauseactiveRuns.get(outputRunId)isundefined).this.manuallyStoppedRuns.has(outputRunId)can never betrue— nothing ever putsoutputRunIdintomanuallyStoppedRuns, because that only happens insidestopRun()which first requires the ID to be inactiveRuns.
The check on this line is therefore unreachable and provides no protection. During the output-prompt phase, the only way to stop execution is via the main runId (which IS in activeRuns), meaning the original runId check on the same line already covers the stop scenario.
To make independent stopping of the output phase work as described in the PR ("making it impossible to stop/track the output phase independently"), outputRunId needs to be tracked in activeRuns (with its own AbortController) before the output-phase onCueRun call.
| if (this.manuallyStoppedRuns.has(runId) || this.manuallyStoppedRuns.has(outputRunId)) { | |
| return; | |
| } | |
| if (this.manuallyStoppedRuns.has(runId)) { |
| onLog( | ||
| 'info', | ||
| `[CUE] First poll for "${triggerName}" failed — seed marker set to prevent event flooding on recovery` | ||
| ); |
There was a problem hiding this comment.
Log message describes the opposite of what the seed marker does
The message reads "seed marker set to prevent event flooding on recovery", but the seed marker's actual effect is the opposite:
- Without the seed marker: the next successful poll sees
hasAnyGitHubSeen() === false→isFirstRun = true→ ALL existing items are silently seeded as "already seen" without firing events → items created during the outage are silently swallowed. - With the seed marker:
hasAnyGitHubSeen() === true→isFirstRun = false→ ALL unseen items fire as events on recovery (including items created before the outage). This is "event flooding", not the prevention of it.
A clearer message would be something like:
| onLog( | |
| 'info', | |
| `[CUE] First poll for "${triggerName}" failed — seed marker set to prevent event flooding on recovery` | |
| ); | |
| `[CUE] First poll for "${triggerName}" failed — seed marker set to prevent silent event loss on recovery` |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/__tests__/renderer/components/CueModal.test.tsx (1)
566-610: Good edge-case coverage; remove low-signal assertions and unusedasync.Line 566 and Line 590 mark tests as
asyncwithout awaiting anything, and Line 587 / Line 609expect(container).toBeTruthy()does not add real verification.♻️ Suggested cleanup
- it('renders without crash when status has many sessions', async () => { + it('renders without crash when status has many sessions', () => { @@ - const { container } = render(<CueModal theme={mockTheme} onClose={mockOnClose} />); + render(<CueModal theme={mockTheme} onClose={mockOnClose} />); @@ - expect(container).toBeTruthy(); }); @@ - it('renders activity log entries with long names', async () => { + it('renders activity log entries with long names', () => { @@ - const { container } = render(<CueModal theme={mockTheme} onClose={mockOnClose} />); + render(<CueModal theme={mockTheme} onClose={mockOnClose} />); @@ - expect(container).toBeTruthy(); expect(screen.getByText(/completed in 5s/)).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/CueModal.test.tsx` around lines 566 - 610, Remove the low-signal assertions and unnecessary async from the two tests in CueModal.test.tsx: in the "renders without crash when status has many sessions" and "renders activity log entries with long names" tests, delete the expect(container).toBeTruthy() lines and remove the unused async keyword from the test declarations; keep the remaining meaningful assertions (session name checks and the completed-in-5s check) and ensure mockUseCueReturn setup (manySessions and activityLog) and render(<CueModal ... />) remain unchanged.src/__tests__/main/cue/cue-template-variables.test.ts (1)
90-123: Consider addingCUE_GH_MERGED_ATfor completeness.The test covers 12 of 13 GitHub variables but omits
CUE_GH_MERGED_AT. Adding it would ensure full coverage of the GitHub variable set.📝 Suggested addition
const ctx = makeContext({ ghType: 'pull_request', ghNumber: '42', ghTitle: 'Add feature X', ghAuthor: 'alice', ghUrl: 'https://github.com/owner/repo/pull/42', ghBody: 'This PR adds feature X', ghLabels: 'enhancement,priority', ghState: 'open', ghRepo: 'owner/repo', ghBranch: 'feature-x', ghBaseBranch: 'main', ghAssignees: 'bob,charlie', + ghMergedAt: '2026-03-15T12:00:00Z', }); const template = [ 'type={{CUE_GH_TYPE}}', 'num={{CUE_GH_NUMBER}}', 'title={{CUE_GH_TITLE}}', 'author={{CUE_GH_AUTHOR}}', 'url={{CUE_GH_URL}}', 'body={{CUE_GH_BODY}}', 'labels={{CUE_GH_LABELS}}', 'state={{CUE_GH_STATE}}', 'repo={{CUE_GH_REPO}}', 'branch={{CUE_GH_BRANCH}}', 'base={{CUE_GH_BASE_BRANCH}}', 'assignees={{CUE_GH_ASSIGNEES}}', + 'merged={{CUE_GH_MERGED_AT}}', ].join(' ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-template-variables.test.ts` around lines 90 - 123, Add the missing GitHub variable CUE_GH_MERGED_AT into the test by supplying ghMergedAt in the makeContext call, adding a 'merged={{CUE_GH_MERGED_AT}}' entry to the template array, and updating the expected string in the substituteTemplateVariables assertion to include the merged=<value> fragment (use a concrete timestamp value like an ISO string). Refer to makeContext, substituteTemplateVariables, and the test case name 'substitutes all github variables' to locate where to add ghMergedAt, the template token, and the updated expected result.src/__tests__/main/cue/cue-filter.test.ts (1)
275-278: Duplicate test case.This test duplicates the existing test at lines 258-262 in the "empty filter" describe block. Consider removing the duplicate.
♻️ Remove duplicate test
describe('combined filter conditions', () => { it('combined numeric + glob in same filter object', () => { expect( matchesFilter({ size: 1500, path: 'src/app.ts' }, { size: '>1000', path: 'src/**/*.ts' }) ).toBe(true); expect( matchesFilter({ size: 500, path: 'src/app.ts' }, { size: '>1000', path: 'src/**/*.ts' }) ).toBe(false); }); - - it('empty filter object matches everything', () => { - expect(matchesFilter({ any: 'value' }, {})).toBe(true); - expect(matchesFilter({}, {})).toBe(true); - }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-filter.test.ts` around lines 275 - 278, The test case titled 'empty filter object matches everything' is duplicated; locate the duplicate test invoking matchesFilter({ any: 'value' }, {}) and matchesFilter({}, {}) within the cue-filter tests and remove the redundant occurrence so only the original test in the "empty filter" describe block (which asserts that an empty filter matches everything) remains; ensure the remaining test still covers both expectations for non-empty and empty inputs referencing matchesFilter.src/__tests__/main/cue/cue-multi-hop-chains.test.ts (2)
676-771: Fan-in/fan-out test is synchronous but asserts async behavior.Line 676 declares the test without
async, but the test callsnotifyAgentCompletedwhich may involve async operations. The assertions at lines 742-768 checkdeps.onCueRuncalls immediately afternotifyAgentCompleted.If
notifyAgentCompletedtriggers async work (e.g., viaonCueRun), the assertions might pass only because the mock returns synchronously. This could make the test fragile if the implementation changes.♻️ Make test async for consistency
- it('fan-in -> fan-out combination dispatches to all targets after all sources complete', () => { + it('fan-in -> fan-out combination dispatches to all targets after all sources complete', async () => { // ... setup ... // Second source completes — fan-in should fire, then fan-out dispatches engine.notifyAgentCompleted('source-b', { sessionName: 'SourceB', stdout: 'output-b' }); + await vi.advanceTimersByTimeAsync(0); // Fan-out should dispatch to both TargetX and TargetY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-multi-hop-chains.test.ts` around lines 676 - 771, The test "fan-in -> fan-out combination dispatches to all targets after all sources complete" is synchronous but relies on potentially async behavior from CueEngine.notifyAgentCompleted and deps.onCueRun; convert the test to async and await the async flows (e.g., await engine.notifyAgentCompleted calls or await a Promise that resolves when deps.onCueRun has been invoked) to ensure assertions run after async dispatches complete; specifically update the test declaration to async, use await around notifyAgentCompleted (or await a helper like waitFor/deps.onCueRun mock promise) and only then assert on deps.onCueRun calls, keeping references to notifyAgentCompleted, engine.start/stop, and deps.onCueRun unchanged.
477-583: Circular and self-referencing chain tests don't verify the MAX_CHAIN_DEPTH guard.The codebase implements a
MAX_CHAIN_DEPTH=10guard innotifyAgentCompleted()that logs an error and stops processing when chain depth exceeds the limit. However, both the circular chain test (lines 477–583) and self-referencing test (lines 585–674) manually stop the engine aftermaxCalls=10rather than letting the guard trigger. This means neither test validates that the guard actually prevents infinite loops.Add a test that removes the manual
engine.stop()cap, allows the chain to run until the guard blocks further depth, and verifies the error is logged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-multi-hop-chains.test.ts` around lines 477 - 583, The test currently prevents the MAX_CHAIN_DEPTH guard from firing by calling engine.stop() inside the onCueRun mock; update the circular/self-referencing test to remove that manual stop so the engine runs until notifyAgentCompleted's MAX_CHAIN_DEPTH guard triggers, and assert that CueEngine logs the depth-exceeded error via the onLog mock. Specifically, in the test referencing CueEngine and the onCueRun mock, delete the engine.stop() call inside the mock (and any maxCalls/count short-circuit), let the timers advance until the guard runs, and then verify onLog was called with an error message indicating MAX_CHAIN_DEPTH or "chain depth" exceeded (i.e., assert onLog.mock.calls includes the expected error), ensuring the test targets notifyAgentCompleted and MAX_CHAIN_DEPTH behavior.src/main/cue/cue-yaml-loader.ts (1)
306-314: Duplicated glob validation logic.The glob validation logic at lines 306-314 and 328-336 is identical. Consider extracting to a helper function.
♻️ Suggested refactor
+function validateGlobPattern(pattern: string, prefix: string, errors: string[]): void { + try { + picomatch(pattern); + } catch (e) { + errors.push( + `${prefix}: "watch" value "${pattern}" is not a valid glob pattern: ${e instanceof Error ? e.message : String(e)}` + ); + } +} // In file.changed branch: } else { - try { - picomatch(sub.watch as string); - } catch (e) { - errors.push( - `${prefix}: "watch" value "${sub.watch}" is not a valid glob pattern: ${e instanceof Error ? e.message : String(e)}` - ); - } + validateGlobPattern(sub.watch as string, prefix, errors); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-yaml-loader.ts` around lines 306 - 314, The duplicated glob validation for sub.watch is repeated; extract the logic into a single helper (e.g., validateGlob or validateGlobPattern) and replace both occurrences with calls to it; the helper should accept the pattern string (sub.watch), context/prefix (prefix), field name if needed, and the errors array, run picomatch(pattern) inside a try/catch, and push the same error message (`${prefix}: "watch" value "${sub.watch}" is not a valid glob pattern: ...`) when validation fails so both sites (the current blocks around sub.watch) share the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/cue/cue-template-variables.test.ts`:
- Around line 157-164: The test name is misleading—rename the test case string
to reflect that substituteTemplateVariables preserves (does not truncate) a
5000-char CUE_SOURCE_OUTPUT; update the it(...) description from 'handles
truncated 5000-char sourceOutput' to something like 'preserves 5000-char
sourceOutput without truncation' in the test that calls
substituteTemplateVariables with CUE_SOURCE_OUTPUT (the
longOutput/ctx/template/result assertions remain unchanged).
---
Nitpick comments:
In `@src/__tests__/main/cue/cue-filter.test.ts`:
- Around line 275-278: The test case titled 'empty filter object matches
everything' is duplicated; locate the duplicate test invoking matchesFilter({
any: 'value' }, {}) and matchesFilter({}, {}) within the cue-filter tests and
remove the redundant occurrence so only the original test in the "empty filter"
describe block (which asserts that an empty filter matches everything) remains;
ensure the remaining test still covers both expectations for non-empty and empty
inputs referencing matchesFilter.
In `@src/__tests__/main/cue/cue-multi-hop-chains.test.ts`:
- Around line 676-771: The test "fan-in -> fan-out combination dispatches to all
targets after all sources complete" is synchronous but relies on potentially
async behavior from CueEngine.notifyAgentCompleted and deps.onCueRun; convert
the test to async and await the async flows (e.g., await
engine.notifyAgentCompleted calls or await a Promise that resolves when
deps.onCueRun has been invoked) to ensure assertions run after async dispatches
complete; specifically update the test declaration to async, use await around
notifyAgentCompleted (or await a helper like waitFor/deps.onCueRun mock promise)
and only then assert on deps.onCueRun calls, keeping references to
notifyAgentCompleted, engine.start/stop, and deps.onCueRun unchanged.
- Around line 477-583: The test currently prevents the MAX_CHAIN_DEPTH guard
from firing by calling engine.stop() inside the onCueRun mock; update the
circular/self-referencing test to remove that manual stop so the engine runs
until notifyAgentCompleted's MAX_CHAIN_DEPTH guard triggers, and assert that
CueEngine logs the depth-exceeded error via the onLog mock. Specifically, in the
test referencing CueEngine and the onCueRun mock, delete the engine.stop() call
inside the mock (and any maxCalls/count short-circuit), let the timers advance
until the guard runs, and then verify onLog was called with an error message
indicating MAX_CHAIN_DEPTH or "chain depth" exceeded (i.e., assert
onLog.mock.calls includes the expected error), ensuring the test targets
notifyAgentCompleted and MAX_CHAIN_DEPTH behavior.
In `@src/__tests__/main/cue/cue-template-variables.test.ts`:
- Around line 90-123: Add the missing GitHub variable CUE_GH_MERGED_AT into the
test by supplying ghMergedAt in the makeContext call, adding a
'merged={{CUE_GH_MERGED_AT}}' entry to the template array, and updating the
expected string in the substituteTemplateVariables assertion to include the
merged=<value> fragment (use a concrete timestamp value like an ISO string).
Refer to makeContext, substituteTemplateVariables, and the test case name
'substitutes all github variables' to locate where to add ghMergedAt, the
template token, and the updated expected result.
In `@src/__tests__/renderer/components/CueModal.test.tsx`:
- Around line 566-610: Remove the low-signal assertions and unnecessary async
from the two tests in CueModal.test.tsx: in the "renders without crash when
status has many sessions" and "renders activity log entries with long names"
tests, delete the expect(container).toBeTruthy() lines and remove the unused
async keyword from the test declarations; keep the remaining meaningful
assertions (session name checks and the completed-in-5s check) and ensure
mockUseCueReturn setup (manySessions and activityLog) and render(<CueModal ...
/>) remain unchanged.
In `@src/main/cue/cue-yaml-loader.ts`:
- Around line 306-314: The duplicated glob validation for sub.watch is repeated;
extract the logic into a single helper (e.g., validateGlob or
validateGlobPattern) and replace both occurrences with calls to it; the helper
should accept the pattern string (sub.watch), context/prefix (prefix), field
name if needed, and the errors array, run picomatch(pattern) inside a try/catch,
and push the same error message (`${prefix}: "watch" value "${sub.watch}" is not
a valid glob pattern: ...`) when validation fails so both sites (the current
blocks around sub.watch) share the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9ec3ddc-b135-43d0-aa99-f852e5b79693
📒 Files selected for processing (14)
.gitignoresrc/__tests__/main/cue/cue-concurrency.test.tssrc/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/cue/cue-filter.test.tssrc/__tests__/main/cue/cue-github-poller.test.tssrc/__tests__/main/cue/cue-ipc-handlers.test.tssrc/__tests__/main/cue/cue-multi-hop-chains.test.tssrc/__tests__/main/cue/cue-session-lifecycle.test.tssrc/__tests__/main/cue/cue-template-variables.test.tssrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/renderer/components/CueModal.test.tsxsrc/main/cue/cue-engine.tssrc/main/cue/cue-github-poller.tssrc/main/cue/cue-yaml-loader.ts
…elper - Rewrite chain depth guard to propagate via AgentCompletionData.chainDepth across async hops instead of a synchronous instance counter that resets between microtasks; thread chainDepth through dispatchSubscription, executeCueRun, doExecuteCueRun, QueuedEvent, and drainQueue - Extract validateGlobPattern helper in cue-yaml-loader to deduplicate the file.changed and task.pending glob validation blocks - Remove duplicate empty-filter test, merge assertion into original - Add missing CUE_GH_MERGED_AT to github template variable test - Rename misleading test to 'preserves 5000-char sourceOutput without truncation' - Make fan-in/fan-out test async with proper await before assertions - Rewrite circular chain and self-referencing tests to rely on the MAX_CHAIN_DEPTH guard instead of manually stopping the engine - Remove low-signal expect(container).toBeTruthy() and unnecessary async from CueModal edge case tests
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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)
src/main/cue/cue-engine.ts (1)
732-737:⚠️ Potential issue | 🟠 MajorMissing
chainDepthpropagation in fan-in completion path.The
dispatchSubscriptioncall here does not passchainDepth. Similarly, line 790 inhandleFanInTimeoutomits it. This means chains involving fan-in will reset their depth tracking, potentially allowing circular chains to bypass theMAX_CHAIN_DEPTHguard.Consider tracking the maximum
chainDepthfrom incoming source completions and propagating it through the fan-in dispatch:Proposed fix for fan-in chainDepth tracking
private handleFanIn( ownerSessionId: string, state: SessionState, sub: CueSubscription, sources: string[], completedSessionId: string, completedSessionName: string, - completionData?: AgentCompletionData + completionData?: AgentCompletionData, + chainDepth?: number ): void { const key = `${ownerSessionId}:${sub.name}`; if (!this.fanInTrackers.has(key)) { - this.fanInTrackers.set(key, new Map()); + this.fanInTrackers.set(key, new Map()); + // Could also track maxChainDepth per fan-in key }You would need to track the maximum
chainDepthacross all sources in the fan-in tracker and pass it todispatchSubscriptionwhen all sources complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-engine.ts` around lines 732 - 737, The fan-in path is not propagating chainDepth to dispatchSubscription (seen at the dispatchSubscription(...) call and in handleFanInTimeout), so compute and pass the maximum chainDepth from incoming completions: when collecting completions (e.g., the completions array/objects returned to the fan-in tracker) extract each completion's chainDepth, take Math.max (or equivalent) to get maxChainDepth, update the fan-in tracker to store/merge this max as sources arrive, and then pass that maxChainDepth as the chainDepth argument to dispatchSubscription in both the "all sources complete" branch and the timeout branch (methods referenced: dispatchSubscription and handleFanInTimeout; update fan-in tracker logic where completions are aggregated).
🤖 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 `@src/main/cue/cue-engine.ts`:
- Around line 732-737: The fan-in path is not propagating chainDepth to
dispatchSubscription (seen at the dispatchSubscription(...) call and in
handleFanInTimeout), so compute and pass the maximum chainDepth from incoming
completions: when collecting completions (e.g., the completions array/objects
returned to the fan-in tracker) extract each completion's chainDepth, take
Math.max (or equivalent) to get maxChainDepth, update the fan-in tracker to
store/merge this max as sources arrive, and then pass that maxChainDepth as the
chainDepth argument to dispatchSubscription in both the "all sources complete"
branch and the timeout branch (methods referenced: dispatchSubscription and
handleFanInTimeout; update fan-in tracker logic where completions are
aggregated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4fa792f-62ec-49fe-a811-01481a868fd3
📒 Files selected for processing (7)
src/__tests__/main/cue/cue-filter.test.tssrc/__tests__/main/cue/cue-multi-hop-chains.test.tssrc/__tests__/main/cue/cue-template-variables.test.tssrc/__tests__/renderer/components/CueModal.test.tsxsrc/main/cue/cue-engine.tssrc/main/cue/cue-types.tssrc/main/cue/cue-yaml-loader.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/main/cue/cue-template-variables.test.ts
- src/tests/renderer/components/CueModal.test.tsx
…og message - Remove unreachable manuallyStoppedRuns.has(outputRunId) check — outputRunId is never added to activeRuns so stopRun() can never populate it - Fix misleading log: seed marker prevents silent event loss (items swallowed by seeding path), not event flooding
Store chainDepth in FanInSourceCompletion and pass the max across completed sources to dispatchSubscription in both the all-complete and timeout-continue branches, closing a gap where circular pipelines routed through fan-in nodes would bypass the depth guard.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/cue/cue-engine.ts`:
- Around line 517-526: The current guard defaults completionData?.chainDepth to
0 which lets callers omit the field to bypass the loop-protection; change the
logic in cue-engine.ts so that if completionData is missing or
completionData.chainDepth is undefined/null you treat it as an invalid/missing
provenance and abort (log an error via this.deps.onLog and return) rather than
silently resetting to 0, otherwise read the numeric chainDepth and compare to
MAX_CHAIN_DEPTH as before (references: completionData, chainDepth,
AgentCompletionData, MAX_CHAIN_DEPTH).
- Around line 1496-1503: The cleanup loop for scheduledFiredKeys currently
matches only on sub.name (in the block iterating state.config.subscriptions and
this.scheduledFiredKeys) which can delete markers for other sessions; update the
matching to be session-scoped by including/validating the session identifier
when comparing keys (e.g., require the key to contain both sub.name and the
current session id or parse the key and compare both parts). Modify the logic
surrounding scheduledFiredKeys deletion in cue-engine.ts to only delete keys
that match sub.name AND the current session's id (use the existing session
identifier available in this instance or state) so teardown only affects that
session's fired markers.
- Around line 1324-1336: The code records an output event as 'running' before
awaiting onCueRun and then swallows DB/errors, which can leave events stuck and
hides exceptions; change the flow so you still record the 'running' state
(recordCueEvent) but wrap the await onCueRun(...) in a try/catch that on error
calls updateCueEventStatus(outputRunId, 'failed', error message) and then uses
captureException/captureMessage (import from src/main/utils/sentry.ts) to report
the error before rethrowing, and in the success path call
updateCueEventStatus(outputRunId, 'succeeded'); do the same fix for the other
block using the same pattern and symbols (recordCueEvent, onCueRun,
updateCueEventStatus, captureException, captureMessage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2695a8bf-267b-44c5-86ae-cf4782e6562e
📒 Files selected for processing (2)
src/main/cue/cue-engine.tssrc/main/cue/cue-github-poller.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/cue/cue-github-poller.ts
| // Guard against infinite chain loops (A triggers B triggers A). | ||
| // chainDepth is propagated through AgentCompletionData so it persists across async hops. | ||
| const chainDepth = completionData?.chainDepth ?? 0; | ||
| if (chainDepth >= MAX_CHAIN_DEPTH) { | ||
| this.deps.onLog( | ||
| 'error', | ||
| `[CUE] Max chain depth (${MAX_CHAIN_DEPTH}) exceeded — aborting to prevent infinite loop` | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
chainDepth guard is bypassable when callers omit the field.
Defaulting chainDepth to 0 here means any caller that forgets to pass it silently resets loop depth, which weakens the infinite-chain protection.
💡 Suggested hardening
- const chainDepth = completionData?.chainDepth ?? 0;
+ const isRootCompletion = completionData?.triggeredBy == null;
+ if (!isRootCompletion && completionData?.chainDepth == null) {
+ this.deps.onLog(
+ 'error',
+ '[CUE] Missing chainDepth for chained completion; aborting to prevent guard bypass'
+ );
+ return;
+ }
+ const chainDepth = completionData?.chainDepth ?? 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-engine.ts` around lines 517 - 526, The current guard
defaults completionData?.chainDepth to 0 which lets callers omit the field to
bypass the loop-protection; change the logic in cue-engine.ts so that if
completionData is missing or completionData.chainDepth is undefined/null you
treat it as an invalid/missing provenance and abort (log an error via
this.deps.onLog and return) rather than silently resetting to 0, otherwise read
the numeric chainDepth and compare to MAX_CHAIN_DEPTH as before (references:
completionData, chainDepth, AgentCompletionData, MAX_CHAIN_DEPTH).
| try { | ||
| recordCueEvent({ | ||
| id: outputRunId, | ||
| type: event.type, | ||
| triggerName: event.triggerName, | ||
| sessionId, | ||
| subscriptionName: `${subscriptionName}:output`, | ||
| status: 'running', | ||
| payload: JSON.stringify(outputEvent.payload), | ||
| }); | ||
| } catch { | ||
| // Non-fatal if DB is unavailable | ||
| } |
There was a problem hiding this comment.
Output-phase event can be left stuck in running and DB errors are silently swallowed.
recordCueEvent(outputRunId, running) is written before await onCueRun(...); if that await throws, updateCueEventStatus(outputRunId, ...) is skipped, leaving stale state. Also, these new catches swallow all exceptions without Sentry reporting.
🛠️ Suggested fix
+ import { captureException } from 'src/main/utils/sentry.ts';
if (outputPrompt && result.status === 'completed') {
...
- try {
+ try {
recordCueEvent({
...
});
- } catch {
- // Non-fatal if DB is unavailable
+ } catch (error) {
+ captureException(error, { context: 'CueEngine.output.recordCueEvent', sessionId, subscriptionName });
}
...
- const outputResult = await this.deps.onCueRun({
- runId: outputRunId,
- ...
- });
-
- try {
- updateCueEventStatus(outputRunId, outputResult.status);
- } catch {
- // Non-fatal if DB is unavailable
- }
+ let outputStatus: CueRunResult['status'] = 'failed';
+ try {
+ const outputResult = await this.deps.onCueRun({
+ runId: outputRunId,
+ ...
+ });
+ outputStatus = outputResult.status;
+ ...
+ } finally {
+ try {
+ updateCueEventStatus(outputRunId, outputStatus);
+ } catch (error) {
+ captureException(error, { context: 'CueEngine.output.updateCueEventStatus', outputRunId });
+ }
+ }
}As per coding guidelines, "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production." and "Use Sentry utilities for explicit reporting: import captureException and captureMessage from src/main/utils/sentry.ts or src/renderer/utils/sentry.ts to report exceptions with context and notable events."
Also applies to: 1348-1352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-engine.ts` around lines 1324 - 1336, The code records an
output event as 'running' before awaiting onCueRun and then swallows DB/errors,
which can leave events stuck and hides exceptions; change the flow so you still
record the 'running' state (recordCueEvent) but wrap the await onCueRun(...) in
a try/catch that on error calls updateCueEventStatus(outputRunId, 'failed',
error message) and then uses captureException/captureMessage (import from
src/main/utils/sentry.ts) to report the error before rethrowing, and in the
success path call updateCueEventStatus(outputRunId, 'succeeded'); do the same
fix for the other block using the same pattern and symbols (recordCueEvent,
onCueRun, updateCueEventStatus, captureException, captureMessage).
…ion collision Keys now use format sessionId:subName:time instead of subName:time, so teardown of one session cannot delete another session's fired markers when both have identically-named subscriptions.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/cue/cue-engine.ts (1)
1324-1336:⚠️ Potential issue | 🟠 MajorOutput-phase event can stay stuck in
runningand DB failures are silently swallowed.If
onCueRunthrows at Line 1339,updateCueEventStatus(outputRunId, ...)at Line 1348 is skipped, leaving stale state. Also, the catches at Line 1324 and Line 1348 swallow DB errors without Sentry reporting.As per coding guidelines, "Do not silently swallow errors with try-catch blocks that only log." and "Use Sentry utilities for explicit reporting: import `captureException` and `captureMessage`..."💡 Suggested hardening
+import { captureException } from '../utils/sentry'; ... const contextPrompt = `${outputPrompt}\n\n---\n\nContext from completed task:\n${result.stdout.substring(0, SOURCE_OUTPUT_MAX_CHARS)}`; - const outputResult = await this.deps.onCueRun({ - runId: outputRunId, - sessionId, - prompt: contextPrompt, - subscriptionName: `${subscriptionName}:output`, - event: outputEvent, - timeoutMs, - }); - - try { - updateCueEventStatus(outputRunId, outputResult.status); - } catch { - // Non-fatal if DB is unavailable - } + let outputStatus: CueRunResult['status'] = 'failed'; + try { + const outputResult = await this.deps.onCueRun({ + runId: outputRunId, + sessionId, + prompt: contextPrompt, + subscriptionName: `${subscriptionName}:output`, + event: outputEvent, + timeoutMs, + }); + outputStatus = outputResult.status; + + if (outputResult.status === 'completed') { + result.stdout = outputResult.stdout; + } else { + this.deps.onLog( + 'cue', + `[CUE] "${subscriptionName}" output prompt failed (${outputResult.status}), using main task output` + ); + } + } finally { + try { + updateCueEventStatus(outputRunId, outputStatus); + } catch (error) { + captureException(error, { + context: 'CueEngine.output.updateCueEventStatus', + outputRunId, + sessionId, + subscriptionName, + }); + } + }Also applies to: 1339-1352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-engine.ts` around lines 1324 - 1336, The recordCueEvent(...) try-catch swallows DB failures and if onCueRun(...) throws the output event can remain stuck with status 'running'; change the flow so that updateCueEventStatus(outputRunId, ...) is always attempted (use a finally or ensure-post-run block) to set final status ('completed' or 'failed'), and report any DB or runtime errors to Sentry using captureException/captureMessage; specifically wrap calls to recordCueEvent, onCueRun, and updateCueEventStatus (referencing recordCueEvent, onCueRun, updateCueEventStatus, and outputRunId) so DB errors are not silently ignored but sent to Sentry and the event status is updated even when onCueRun throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/cue/cue-engine.ts`:
- Around line 1324-1336: The recordCueEvent(...) try-catch swallows DB failures
and if onCueRun(...) throws the output event can remain stuck with status
'running'; change the flow so that updateCueEventStatus(outputRunId, ...) is
always attempted (use a finally or ensure-post-run block) to set final status
('completed' or 'failed'), and report any DB or runtime errors to Sentry using
captureException/captureMessage; specifically wrap calls to recordCueEvent,
onCueRun, and updateCueEventStatus (referencing recordCueEvent, onCueRun,
updateCueEventStatus, and outputRunId) so DB errors are not silently ignored but
sent to Sentry and the event status is updated even when onCueRun throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a93d16e-9f29-437c-a759-d3a261d0238f
📒 Files selected for processing (1)
src/main/cue/cue-engine.ts
1 participant
Summary
Comprehensive audit and hardening of the Maestro Cue event-driven automation system. Addresses 7 code-level gaps that could cause infinite loops, orphaned state, or silent failures in production. Adds 42 new tests across 3 new files and 8 extended files, bringing the Cue test count from ~428 to 470 in the core engine alone (~571 total including renderer/integration tests).
Code Fixes
Fix 1: Circular Chain Detection (
cue-engine.ts)Added a
chainDepthcounter withMAX_CHAIN_DEPTH = 10guard innotifyAgentCompleted(). Prevents infinite recursion when subscription A triggers B and B triggers A — previously this would freeze the main process.Fix 2: Queue Cleanup on Session Teardown (
cue-engine.ts)teardownSession()now callsclearQueue(sessionId)to remove stale queued events during config reloads viarefreshSession(). Previously, queued events survived YAML hot-reloads and could fire against an outdated subscription config.Fix 3: GitHub First-Run Error Resilience (
cue-github-poller.ts)When the first
ghCLI poll fails, a__seed_marker__is now placed in the seen-items DB. This prevents the next successful poll from treating ALL existing items as a first-run seed — which would silently swallow any items created during the outage.Fix 4: scheduledFiredKeys Cleanup (
cue-engine.ts)teardownSession()now purgesscheduledFiredKeysentries for the torn-down session's subscriptions. Previously, keys from deleted/renamed subscriptions accumulated unboundedly over the engine's lifetime.Fix 5: Output Prompt Separate runId (
cue-engine.ts)Output prompts now get their own
outputRunIdwith independent DB tracking (subscription name suffixed with:output). Previously, the output prompt reused the main task'srunId, overwriting its DB record and making it impossible to stop/track the output phase independently.Fix 6: Watch Glob Validation (
cue-yaml-loader.ts)Added
picomatch-based validation forwatchglob patterns infile.changedandtask.pendingsubscriptions during config validation. Invalid patterns that would silently produce no events are now caught before the engine starts.Fix 7: Prompt File Existence Warning (
cue-engine.ts)initSession()now checks each subscription at setup time: ifprompt_fileis set but the file wasn't found by the loader (empty prompt), a warning is logged immediately — not just when the subscription fires and does nothing.New Tests (42 total)
Tier 1 — Production Confidence (21 tests)
cue-multi-hop-chains.test.ts(new)cue-session-lifecycle.test.ts(new)cue-engine.test.ts(extended)Tier 2 — Edge Cases (13 tests)
cue-template-variables.test.ts(new)cue-filter.test.ts(extended)cue-github-poller.test.ts(extended)cue-yaml-loader.test.ts(extended)Tier 3 — Defense in Depth (8 tests)
cue-concurrency.test.ts(extended)CueModal.test.tsx(extended)cue-ipc-handlers.test.ts(extended)Test plan
npx tsc --noEmit— only pre-existing errors in unrelated filenpm run test— 23,417 tests passed, 0 failures (580 test files)npm run lint:eslint— cleansrc/__tests__/main/cue/)Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests