refactor(cue): decompose executor into SRP modules and fix run manager race conditions - #790
Conversation
…s, −67%) Extract three single-responsibility modules from the monolithic executor: - cue-template-context-builder: enricher registry for event→template mapping - cue-spawn-builder: agent definition lookup, arg building, SSH wrapping - cue-process-lifecycle: process spawning, stdio capture, timeout enforcement Executor becomes a thin orchestrator composing the three modules. All existing 47 executor tests pass unchanged. 52 new tests across the three modules.
…chine
Add explicit RunPhase ('running' | 'stopping' | 'finished') to ActiveRun
with a validated transitionRun() helper. stopRun() is now fully self-contained
and the finally block only handles natural completions via activeRuns.has()
guard. Fixes spurious onRunCompleted after engine reset and prevents double-stop.
New test file: cue-run-manager.test.ts (37 tests) covering phase transitions,
race conditions, concurrency slot management, and output prompt lifecycle.
|
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 (1)
📝 WalkthroughWalkthroughExtracts Cue process lifecycle, spawn-spec construction, and template-context enrichment into dedicated modules; refactors executor and run-manager to delegate to those modules; and adds comprehensive Vitest suites covering lifecycle, spawn building, run-manager state, and template-context behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/cue/cue-executor.ts (1)
162-164: Consider stronger typing for the return value.The return type
Map<string, any>is loose. IfgetActiveProcessMap()has a more specific type incue-process-lifecycle, propagating it here would improve type safety for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-executor.ts` around lines 162 - 164, The exported function getActiveProcesses currently returns Map<string, any>; change its signature to a stronger type by using the actual type from cue-process-lifecycle (either import the concrete Map type or use ReturnType<typeof getActiveProcessMap>) so callers get correct typings; update the return type of getActiveProcesses to that specific type and ensure any imports (e.g., the type alias or getActiveProcessMap) are added so the function remains a thin wrapper around getActiveProcessMap().
🤖 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-executor.ts`:
- Around line 93-102: The code validates trimmedPrompt but then calls
substituteTemplateVariables with the original promptPath, so whitespace can
sneak back in; change the call to substituteTemplateVariables to use
trimmedPrompt (after setting templateContext.cue via buildCueTemplateContext)
and ensure any downstream logic uses trimmedPrompt instead of promptPath (refer
to trimmedPrompt, promptPath, substituteTemplateVariables,
buildCueTemplateContext, and substitutedPrompt) so substitutions operate on the
trimmed value.
In `@src/main/cue/cue-process-lifecycle.ts`:
- Around line 11-16: The code currently swallows child-process startup failures
into stderr; update the spawn logic in cue-process-lifecycle.ts to report both
synchronous spawn exceptions and ChildProcess 'error' events to Sentry (use the
same captureException call pattern as in ChildProcessSpawner.ts) and ensure such
errors convert the CueRunStatus result to a failed state. Specifically, wrap the
call to spawn(...) in a try/catch and call captureException(err) then
return/emit a failed CueRunStatus when spawn throws synchronously, and attach an
'error' listener on the returned ChildProcess that calls captureException(error)
and transitions the run to failed (same error-to-failed handling as for non-zero
exit). Ensure these changes touch the areas around the spawn invocation and the
existing listeners for the ChildProcess so both synchronous and async spawn
failures are reported and produce a failed result.
In `@src/main/cue/cue-run-manager.ts`:
- Line 181: The handler is stopping the parent runId instead of the actual
executing phase run (outputRunId), so update the stop logic to target the
phase-specific id: when you start a subprocess under a new outputRunId, store
its abortController and phase in activeRuns using outputRunId (see
activeRuns.set(...)), and call onStopCueRun(outputRunId) (not
onStopCueRun(runId)) when cancelling that subprocess; make the same change for
the other occurrence in the 374-382 region so all stop signals reference the
exact run id that owns the abortController/phase.
- Around line 399-404: The catch currently swallowing failures from
updateCueEventStatus(runId, 'stopped') must report the error to Sentry and
include runId context; update the try/catch around updateCueEventStatus to
import and call the Sentry helper (captureException or captureMessage) with the
caught error and an extra/context payload containing runId and the attempted
status ('stopped'), and also optionally log a short message via the existing
logger; do not silently ignore the exception so it surfaces for monitoring.
In `@src/main/cue/cue-spawn-builder.ts`:
- Around line 116-151: The code currently skips appending the prompt whenever
sshRemoteConfig?.enabled is true, even if wrapSpawnWithSsh didn't run because
sshStore was missing; change the logic to append the prompt unless the SSH
wrapper was actually used. Concretely, ensure the sshRemoteUsed flag (set from
sshResult.sshRemoteUsed after wrapSpawnWithSsh) is initialized false and
updated, then replace the local-only condition if (!sshRemoteConfig?.enabled)
with if (!sshRemoteUsed) so the prompt (via agentDef.promptArgs,
agentDef.noPromptSeparator, substitutedPrompt) is appended whenever the process
was not actually wrapped by wrapSpawnWithSsh.
---
Nitpick comments:
In `@src/main/cue/cue-executor.ts`:
- Around line 162-164: The exported function getActiveProcesses currently
returns Map<string, any>; change its signature to a stronger type by using the
actual type from cue-process-lifecycle (either import the concrete Map type or
use ReturnType<typeof getActiveProcessMap>) so callers get correct typings;
update the return type of getActiveProcesses to that specific type and ensure
any imports (e.g., the type alias or getActiveProcessMap) are added so the
function remains a thin wrapper around getActiveProcessMap().
🪄 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: 61d893b6-84b5-46b0-b710-3b95c1ed7063
📒 Files selected for processing (9)
src/__tests__/main/cue/cue-process-lifecycle.test.tssrc/__tests__/main/cue/cue-run-manager.test.tssrc/__tests__/main/cue/cue-spawn-builder.test.tssrc/__tests__/main/cue/cue-template-context-builder.test.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-process-lifecycle.tssrc/main/cue/cue-run-manager.tssrc/main/cue/cue-spawn-builder.tssrc/main/cue/cue-template-context-builder.ts
Greptile SummaryThis PR decomposes a 521-line Confidence Score: 5/5Safe to merge — no P0/P1 issues; all findings are P2 style suggestions that do not affect correctness on the changed path. The RunPhase state machine correctly eliminates both documented race conditions. The module decomposition is clean, backwards compatibility is preserved via re-exports, and the 89-test suite covers phase transitions, race conditions, concurrency, DB recording, and SSH paths. All three inline comments are non-blocking improvements. src/main/cue/cue-run-manager.ts — three P2 concerns: this.stopRun binding in stopAll(), reset() not signaling processes, and output-prompt process not cancellable via stopRun. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CueEngine triggers event] --> B[CueRunManager.execute]
B --> C{Concurrency slot available?}
C -- No --> D[Queue event]
C -- Yes --> E[doExecuteCueRun phase: running]
E --> F[CueTemplateContextBuilder]
F --> G[substituteTemplateVariables]
G --> H[CueSpawnBuilder buildSpawnSpec]
H --> I{SSH enabled?}
I -- Yes --> J[wrapSpawnWithSsh]
I -- No --> K[Append prompt to args]
J --> L[CueProcessLifecycle runProcess]
K --> L
L --> M[spawn child process]
M --> N{Outcome}
N -- Natural exit --> O[finally block activeRuns.has check]
N -- Timeout SIGTERM/SIGKILL --> O
O -- run still tracked --> P[transitionRun finished / onRunCompleted]
O -- removed by stopRun/reset --> Q[Skip no spurious callbacks]
R[stopRun called] --> S[transitionRun stopping]
S --> T[delete from activeRuns / release slot]
T --> U[onStopCueRun SIGTERM process]
T --> V[onRunStopped DB update]
W[reset called] --> X[onAllowSleep each run / clear maps]
X --> Q
|
…anager, and spawn builder - Use trimmedPrompt in substituteTemplateVariables instead of raw promptPath - Add Sentry reporting for both sync and async spawn failures in process lifecycle - Stop output prompt subprocess with correct runId via new processRunId field - Report DB update failures to Sentry with context instead of silently swallowing - Use sshRemoteUsed flag for prompt append guard so missing sshStore falls back correctly - Tighten getActiveProcesses return type from Map<string, any> to actual type 7 new tests covering Sentry reporting, output prompt stop targeting, DB error handling, and SSH fallback prompt appending.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-spawn-builder.test.ts`:
- Around line 220-227: The test currently asserts process.env.PATH which is
platform-fragile; modify the test that calls buildSpawnSpec(createConfig(),
'prompt') to instead seed a unique temporary environment variable on process.env
(e.g., TEMP_TEST_ENV = 'some-value') before invoking buildSpawnSpec, then assert
that result.ok is true and that result.spec.env[TEMP_TEST_ENV] equals the seeded
value; finally, remove or restore the temporary env var after the assertion. Use
the existing symbols buildSpawnSpec, createConfig and result.spec.env to locate
where to inject the setup/teardown and assertion.
In `@src/main/cue/cue-executor.ts`:
- Around line 124-130: The runProcess call is passing sshRemoteEnabled based on
sshRemoteConfig?.enabled which can differ from the actual decision made by
buildSpawnSpec(); update the runProcess invocation to use spec.sshRemoteUsed
(and only pass sshStdinScript/stdinPrompt when spec.sshRemoteUsed is true) so
runProcess sees whether SSH wrapping is actually used and won't duplicate stdin
delivery. Locate the runProcess call and replace sshRemoteEnabled:
sshRemoteConfig?.enabled with sshRemoteEnabled: spec.sshRemoteUsed, and
conditionally include sshStdinScript/stdinPrompt based on spec.sshRemoteUsed to
avoid sending the prompt for local fallbacks.
🪄 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: 7faee5a2-54a7-416f-b240-728a413bd2a9
📒 Files selected for processing (7)
src/__tests__/main/cue/cue-process-lifecycle.test.tssrc/__tests__/main/cue/cue-run-manager.test.tssrc/__tests__/main/cue/cue-spawn-builder.test.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-process-lifecycle.tssrc/main/cue/cue-run-manager.tssrc/main/cue/cue-spawn-builder.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/cue/cue-process-lifecycle.ts
- src/tests/main/cue/cue-process-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/cue/cue-run-manager.ts
- src/main/cue/cue-spawn-builder.ts
…tform-fragile env test - cue-executor: derive sshActuallyUsed from spec.sshRemoteUsed (the resolved SSH config) rather than sshRemoteConfig?.enabled (the requested config). This prevents stdin misdelivery when SSH is configured but sshStore is missing and the wrapper was never called. Also gates sshStdinScript and stdinPrompt on sshActuallyUsed so they are not passed through in local mode. - cue-spawn-builder.test: replace PATH assertion (undefined on some CI environments) with a seeded __CUE_SPAWN_TEST__ env var in a try/finally block so the test is deterministic on all platforms.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/cue/cue-executor.ts (1)
36-50: Consider moving inline import types to the import section.The inline import types for
CueEventandSshRemoteSettingsStorework correctly but deviate from the standard pattern of top-level imports used elsewhere in the file. Moving them to the import section would improve consistency.♻️ Suggested refactor
import type { CueRunResult, CueSubscription } from './cue-types'; +import type { CueEvent } from './cue-types'; import type { HistoryEntry, SessionInfo } from '../../shared/types'; import { substituteTemplateVariables, type TemplateContext } from '../../shared/templateVariables'; import { buildCueTemplateContext } from './cue-template-context-builder'; import { buildSpawnSpec } from './cue-spawn-builder'; +import type { SshRemoteSettingsStore } from '../utils/ssh-remote-resolver'; ... export interface CueExecutionConfig { ... - event: import('./cue-types').CueEvent; + event: CueEvent; ... - sshStore?: import('../utils/ssh-remote-resolver').SshRemoteSettingsStore; + sshStore?: SshRemoteSettingsStore;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-executor.ts` around lines 36 - 50, Move the inline type imports to the top import section: add "import type { CueEvent } from './cue-types';" and "import type { SshRemoteSettingsStore } from '../utils/ssh-remote-resolver';" (use "import type" to avoid runtime imports), then replace the inline type annotations in the parameter/interface (the event property and sshStore?: ...) so they reference CueEvent and SshRemoteSettingsStore directly (e.g., event: CueEvent; sshStore?: SshRemoteSettingsStore;). Ensure any references to the previous inline import syntax are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/cue/cue-executor.ts`:
- Around line 36-50: Move the inline type imports to the top import section: add
"import type { CueEvent } from './cue-types';" and "import type {
SshRemoteSettingsStore } from '../utils/ssh-remote-resolver';" (use "import
type" to avoid runtime imports), then replace the inline type annotations in the
parameter/interface (the event property and sshStore?: ...) so they reference
CueEvent and SshRemoteSettingsStore directly (e.g., event: CueEvent; sshStore?:
SshRemoteSettingsStore;). Ensure any references to the previous inline import
syntax are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 871bb869-e52b-45dd-ab41-e83f04b6ec45
📒 Files selected for processing (2)
src/__tests__/main/cue/cue-spawn-builder.test.tssrc/main/cue/cue-executor.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/main/cue/cue-spawn-builder.test.ts
…ports
Replace import('./cue-types').CueEvent and
import('../utils/ssh-remote-resolver').SshRemoteSettingsStore inline
annotations in CueExecutionConfig with proper top-level import type
declarations, keeping runtime-free type-only imports.
1 participant
Summary
CueExecutor Decomposition (521 → 170 lines, −67%)
The
cue-executor.tsmodule mixed four concerns: template variable population, agent spawn preparation, process lifecycle management, and history recording. This PR decomposes it into three focused modules with the executor as a thin orchestrator.cue-template-context-builder.ts(105 lines) — BuildstemplateContext.cuefrom event payloads using an enricher registry pattern (Map<CueEventType | '*', EnricherFn>). The'*'enricher produces 15 common fields (eventType, triggerName, runId, file/source fields). Dedicated enrichers handletask.pending(6 task fields),github.pull_request, andgithub.issue(13 GitHub fields via shared helper). Adding a new event type requires one enricher entry — no changes to executor or engine.cue-spawn-builder.ts(155 lines) — Pure async transformation:CueExecutionConfig+ substituted prompt →SpawnSpec. Handles agent definition lookup,buildAgentArgs,applyAgentConfigOverrides, SSH wrapping viawrapSpawnWithSsh, and prompt appending (supporting--separator,promptArgs, andnoPromptSeparatormodes). Returns a discriminated union ({ ok: true, spec }|{ ok: false, message }) for type-safe error handling.cue-process-lifecycle.ts(237 lines) — Spawns child processes, manages stdio capture, enforces timeouts with SIGTERM → SIGKILL escalation (5s grace period), and tracks active processes for the Process Monitor. Owns the module-levelactiveProcessesMap, the settled guard pattern (prevents duplicate close/error resolution), and output parser integration.Refactored
cue-executor.ts(170 lines) — Thin orchestrator: validate prompt → build template context → substitute variables → build spawn spec → run process → assembleCueRunResult. Re-exportsCueProcessInfoandSpawnSpecfor backwards compatibility. All delegation wrappers (stopCueRun,getActiveProcesses,getCueProcessList) preserved — zero changes tosrc/main/index.tsor any other consumer.Run Manager State Machine Fix
Problem:
stopRun()and thefinallyblock ofdoExecuteCueRunboth independently managed the "run is done" transition via amanuallyStoppedRunsSet. This caused two bugs:Spurious chain propagation after engine shutdown —
reset()(called byengine.stop()) clearedactiveRunsandmanuallyStoppedRuns, but thefinallyblock of still-running promises would execute afterward, callingonRunCompletedand triggering downstream agent.completed chains even though the engine was shut down.No double-stop protection — Calling
stopRun()twice on the same run would add it tomanuallyStoppedRunsagain and attempt to signal/delete/decrement a second time.Fix: Replaced
manuallyStoppedRunsSet with an explicitRunPhasetype ('running' | 'stopping' | 'finished') onActiveRun, with atransitionRun(runId, toPhase)helper that validates transitions and returns the previous phase (ornullfor invalid transitions).stopRun()callstransitionRun(runId, 'stopping')— returnsfalseif the run doesn't exist or is already stopping/finished. It is now fully self-contained: transition → signal kill → set result → delete from activeRuns → release sleep → free concurrency slot → drain queue → DB update → notify → log.finallyblock usesactiveRuns.has(runId)as its guard: if the run was already removed (bystopRunorreset), it skips all cleanup entirely. Only natural completions flow through the finally path.stopRunnow also recordsupdateCueEventStatus(runId, 'stopped')directly, rather than deferring to the finally block. This makes DB status consistent regardless of when the promise resolves.Test plan
file.changed,task.pending,github.pull_request,github.issue,time.heartbeat,time.scheduled,agent.completed), empty payload defaults, numeric/boolean coercion, unknown event type graceful handling--separator,promptArgs,noPromptSeparator), yoloMode flag, config overrides passthrough, process.env inclusion, SSH execution (wrapSpawnWithSsh call, no double-append, sshStdinScript, stdinPrompt passthrough)npm run lint— 0 type errors across all 3 tsconfig filesnpm run lint:eslint— 0 errors on all changed filesSummary by CodeRabbit
Tests
Refactor
New Features