refactor(cue): split shared contracts, config boundaries, and runtime orchestration#750
Merged
reachrazamair merged 5 commits intorcfrom Apr 8, 2026
Merged
refactor(cue): split shared contracts, config boundaries, and runtime orchestration#750reachrazamair merged 5 commits intorcfrom
reachrazamair merged 5 commits intorcfrom
Conversation
📝 WalkthroughWalkthroughModularizes the CUE subsystem: adds shared Cue contracts, config repo/normalizer/validator, session runtime/state, dispatch/completion/query services, refactors cue-engine/yaml loader to delegate to new services, and consolidates Cue types across preload/renderer. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (session)
participant Completion as CueCompletionService
participant FanIn as FanInTracker
participant Dispatch as CueDispatchService
participant RunMgr as RunManager
Agent->>Completion: notifyAgentCompleted(sessionId, completionData)
Completion->>Completion: deps.enabled() & chainDepth check
alt multiple matching sources
Completion->>FanIn: handleCompletion(subscription, matchingSources, completionData)
FanIn->>Dispatch: decide target dispatches
Dispatch->>RunMgr: executeRun(...) per resolved target
else single matching source
Completion->>Dispatch: dispatchSubscription(ownerSessionId, sub, event, sourceSessionName, chainDepth)
Dispatch->>RunMgr: executeRun(...) (fan-out expanded if configured)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
Greptile SummaryThis PR refactors the Cue subsystem in two passes: (1) centralizing shared DTOs under The refactor is behavior-preserving — IPC channel names, preload API shape, YAML semantics, and runtime execution logic are unchanged. Confidence Score: 5/5Safe to merge — all changes are structural refactoring with no behavioral regressions found. All four P2 findings are non-blocking: a documentation gap in getSettings(), an || vs ?? inconsistency in fan-out dispatch, an O(n×m) loop that can be simplified, and a silent filter-discard behaviour in the normalizer. None affect correctness. The extraction of services is clean, initialization order in the CueEngine constructor is correct, startupFiredKeys persistence semantics are preserved, and all public entrypoints remain compatible. No files require special attention for merge safety.
|
| Filename | Overview |
|---|---|
| src/shared/cue/contracts.ts | New shared contract layer centralizing all Cue DTOs — runtime-agnostic, free of Node/Electron deps, and re-exported cleanly via index.ts. |
| src/main/cue/cue-types.ts | Converted to a compatibility facade re-exporting all shared types plus main-process-only helpers (createCueEvent, AgentCompletionData, legacy filename constants). |
| src/main/cue/config/cue-config-repository.ts | Cleanly extracted file-discovery and chokidar watching from the old yaml-loader; debounce cleanup on watcher close is correct. |
| src/main/cue/config/cue-config-normalizer.ts | YAML parsing + normalization split out correctly; minor concern that normalizeFilter silently discards the whole filter object on the first invalid value rather than dropping just the bad key. |
| src/main/cue/config/cue-config-validator.ts | Validation logic extracted unchanged; picomatch glob validation, event-specific field checks, and settings bounds all intact. |
| src/main/cue/cue-dispatch-service.ts | Fan-out and single-target dispatch correctly extracted; minor inconsistency using |
| src/main/cue/cue-completion-service.ts | agent.completed chain handling extracted correctly; fan-in delegation to fanInTracker, chain-depth guard, and filter checks all preserved. |
| src/main/cue/cue-query-service.ts | Status/graph/settings projection correctly extracted; getSettings() undocumented first-session tie-breaking is a minor style concern. |
| src/main/cue/cue-session-runtime-service.ts | Session lifecycle (init, refresh, remove, teardown, clearAll) correctly extracted with pendingYamlWatchers; teardownSession has an O(n×m) scheduled-key cleanup loop that could be simplified. |
| src/main/cue/cue-session-state.ts | Session state helpers (countActiveSubscriptions, getEarliestNextTriggerIso, toSessionStatus) extracted cleanly with no logic changes. |
| src/main/cue/cue-engine.ts | CueEngine is now a lean coordinator/facade; constructor wires all services, startupFiredKeys persistence across stop/start is preserved, and service initialization order is correct. |
| src/main/cue/cue-yaml-loader.ts | Now a stable public facade delegating to the three config modules; all original entrypoints preserved. |
| src/main/preload/cue.ts | Updated to import types from shared/cue instead of duplicating them; re-exports the correct shared subset for the renderer. |
| src/renderer/global.d.ts | Cue API types in MaestroAPI now reference shared/cue contracts directly — consistent with preload and hook changes. |
| src/renderer/hooks/useCue.ts | Hook imports migrated to shared/cue with no behavioral changes to polling, activity update subscription, or state management. |
| src/shared/cue-pipeline-types.ts | CueGraphSession is now a type alias for the shared contract rather than a local definition; CueEventType imported from shared/cue. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph shared["src/shared/cue/"]
contracts["contracts.ts\n(CueConfig, CueSubscription,\nCueEvent, CueRunResult, ...)"]
index["index.ts\n(barrel re-export)"]
contracts --> index
end
subgraph config["src/main/cue/config/"]
repo["cue-config-repository.ts\nfile discovery + chokidar watching"]
norm["cue-config-normalizer.ts\nYAML parse + normalize"]
val["cue-config-validator.ts\nvalidation rules"]
end
subgraph yaml_loader["cue-yaml-loader.ts (facade)"]
loadCueConfig["loadCueConfig()"]
watchCueYaml["watchCueYaml()"]
validateCueConfig["validateCueConfig()"]
end
subgraph services["Runtime Services"]
state["cue-session-state.ts\nstate helpers"]
dispatch["cue-dispatch-service.ts\nfan-out / single dispatch"]
completion["cue-completion-service.ts\nagent.completed chains"]
query["cue-query-service.ts\nstatus / graph / settings"]
runtime["cue-session-runtime-service.ts\nsession lifecycle"]
end
subgraph engine["CueEngine (facade)"]
cueengine["CueEngine\nstart/stop/refresh\ngetStatus/getGraphData\nhasCompletionSubscribers"]
end
index --> yaml_loader
index --> services
repo --> yaml_loader
norm --> yaml_loader
val --> yaml_loader
yaml_loader --> runtime
state --> runtime
state --> query
dispatch --> cueengine
completion --> cueengine
query --> cueengine
runtime --> cueengine
subgraph consumers["Consumers"]
preload["preload/cue.ts"]
renderer["renderer/hooks/useCue.ts\nglobal.d.ts\ncue-pipeline-types.ts"]
end
index --> preload
index --> renderer
cueengine -->|IPC| preload
Comments Outside Diff (1)
-
src/main/cue/cue-config-normalizer.ts, line 50-56 (link)normalizeFiltersilently drops the entire filter on the first invalid valueIf any single filter key has a non-primitive value (e.g. an array), the whole
filterobject is discarded andundefinedis returned. In contrast, the validator (cue-config-validator.ts) reports a per-key error and keeps validating other keys. A user whose config passes validation would still see a silently-dropped filter at load time.Consider emitting a warning for the dropped key (consistent with
initSession's prompt-file warning style) or adopting the same defensive approach as the validator — skip the bad key rather than discarding the whole object.
Reviews (1): Last reviewed commit: "refactor(cue): extract runtime orchestra..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+114
to
+119
| getSettings(): CueSettings { | ||
| for (const [, state] of deps.getSessionStates()) { | ||
| return { ...state.config.settings }; | ||
| } | ||
| return { ...DEFAULT_CUE_SETTINGS }; | ||
| }, |
There was a problem hiding this comment.
getSettings() silently returns the first session's config
When multiple sessions have active Cue configs with different settings blocks, only the first entry in the Map is returned. The method has no documentation about this tie-breaking policy, so callers can't reason about what they'll get.
Consider adding a comment that documents this intentional behaviour (e.g., "returns the first active session's settings as a global defaults view") so future readers don't assume it merges or is deterministic across sessions.
Suggested change
| getSettings(): CueSettings { | |
| for (const [, state] of deps.getSessionStates()) { | |
| return { ...state.config.settings }; | |
| } | |
| return { ...DEFAULT_CUE_SETTINGS }; | |
| }, | |
| getSettings(): CueSettings { | |
| // Returns settings from the first active session as a representative global view. | |
| // If sessions have different settings, there is no merge — the first wins. | |
| for (const [, state] of deps.getSessionStates()) { | |
| return { ...state.config.settings }; | |
| } | |
| return { ...DEFAULT_CUE_SETTINGS }; | |
| }, |
Comment on lines
+62
to
+64
| const perTargetPrompt = sub.fan_out_prompts?.[i]; | ||
| const prompt = perTargetPrompt || sub.prompt_file || sub.prompt; | ||
| deps.executeRun( |
There was a problem hiding this comment.
The fan-out path uses || (falsy check) while the non-fan-out path on line 79 uses ?? (nullish check). For this data shape both behave identically (prompt_file is either a non-empty string or undefined), but the inconsistency is a readability trap for future edits.
Suggested change
| const perTargetPrompt = sub.fan_out_prompts?.[i]; | |
| const prompt = perTargetPrompt || sub.prompt_file || sub.prompt; | |
| deps.executeRun( | |
| const perTargetPrompt = sub.fan_out_prompts?.[i]; | |
| const prompt = perTargetPrompt ?? sub.prompt_file ?? sub.prompt; |
Comment on lines
+189
to
+196
| for (const sub of state.config.subscriptions) { | ||
| for (const key of deps.scheduledFiredKeys) { | ||
| if (key.startsWith(`${sessionId}:${sub.name}:`)) { | ||
| deps.scheduledFiredKeys.delete(key); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The outer loop iterates over each subscription, and for each subscription the full scheduledFiredKeys Set is re-scanned. The Set can be iterated once per teardown instead of once per subscription:
const prefix = `${sessionId}:`;
for (const key of deps.scheduledFiredKeys) {
if (key.startsWith(prefix)) {
deps.scheduledFiredKeys.delete(key);
}
}This also cleans up orphaned keys from subscriptions renamed between hot-reloads (the per-subscription prefix check misses those).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/cue/cue-session-runtime-service.ts (1)
265-267: Consider returning a shallow copy of the sessions Map.
getSessionStates()returns the internalsessionsMap directly, allowing callers to mutate internal state. This could lead to subtle bugs if a consumer accidentally modifies it.♻️ Defensive copy suggestion
getSessionStates(): Map<string, SessionState> { - return sessions; + return new Map(sessions); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-session-runtime-service.ts` around lines 265 - 267, getSessionStates currently returns the internal sessions Map directly, allowing external code to mutate internal state; change getSessionStates to return a shallow copy (e.g., construct and return a new Map from the existing sessions) so callers receive a snapshot while leaving the internal sessions Map unmodified; ensure the method signature still returns Map<string, SessionState> and references the same sessions variable.
🤖 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/config/cue-config-validator.ts`:
- Around line 132-147: The current checks for sub.poll_minutes only test typeof
and < 1, which allow non-finite numbers like NaN and Infinity; update the
validation in the validator that handles sub.poll_minutes (the branch
referencing sub.poll_minutes, event, and pushing to errors) to also require
Number.isFinite(sub.poll_minutes) (or equivalent) so non-finite values are
rejected and an error is pushed to errors (same message as existing) for both
the task.pending and github.pull_request/github.issue branches; ensure you apply
this change where sub.poll_minutes is validated so broken intervals are not
accepted.
- Around line 116-123: In the agent.completed validation branch inside
cue-config-validator.ts (the block checking event === 'agent.completed'),
tighten the checks on sub.source_session so that a string value must be
non-empty and an array value must be non-empty and contain only non-empty
strings; update the logic around the existing checks (the conditional that
currently does Array.isArray(sub.source_session)) to push errors for
source_session: [] and for any array element that is not a string or is an empty
string, and also push an error when sub.source_session is a string but is empty,
using the same prefix/error message pattern as the surrounding validator code.
In `@src/main/cue/cue-dispatch-service.ts`:
- Around line 62-63: The prompt resolution is inverted and may send a file path
because materializeCueConfig puts the resolved file contents into sub.prompt
while sub.prompt_file remains the path; update occurrences that pick a prompt so
they prefer sub.prompt over sub.prompt_file: change the per-target selection
(currently perTargetPrompt || sub.prompt_file || sub.prompt) to perTargetPrompt
|| sub.prompt || sub.prompt_file, and change the other fallback (currently
sub.prompt_file ?? sub.prompt) to sub.prompt ?? sub.prompt_file; locate these in
cue-dispatch-service (look for perTargetPrompt, sub.fan_out_prompts,
sub.prompt_file, sub.prompt) and make the two swaps so the resolved content is
used first.
---
Nitpick comments:
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 265-267: getSessionStates currently returns the internal sessions
Map directly, allowing external code to mutate internal state; change
getSessionStates to return a shallow copy (e.g., construct and return a new Map
from the existing sessions) so callers receive a snapshot while leaving the
internal sessions Map unmodified; ensure the method signature still returns
Map<string, SessionState> and references the same sessions variable.
🪄 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: 84d0e030-6ae5-4391-b174-4ae87ad81fdf
📒 Files selected for processing (17)
src/main/cue/config/cue-config-normalizer.tssrc/main/cue/config/cue-config-repository.tssrc/main/cue/config/cue-config-validator.tssrc/main/cue/cue-completion-service.tssrc/main/cue/cue-dispatch-service.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-query-service.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/main/cue/cue-types.tssrc/main/cue/cue-yaml-loader.tssrc/main/preload/cue.tssrc/renderer/global.d.tssrc/renderer/hooks/useCue.tssrc/shared/cue-pipeline-types.tssrc/shared/cue/contracts.tssrc/shared/cue/index.ts
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/config/cue-config-validator.ts`:
- Around line 224-226: The validator currently treats arrays as valid for
cfg.settings because typeof [] === 'object'; update the validation in the
cue-config-validator (the block checking cfg.settings) to explicitly reject
arrays by adding an Array.isArray check (i.e., ensure cfg.settings is an object,
not null, and not an array) and push the same '"settings" must be an object'
error when an array is provided.
In `@src/main/cue/cue-dispatch-service.ts`:
- Around line 62-63: The code uses logical OR when resolving the final prompt
which treats an empty string as missing; update the resolution in
cue-dispatch-service.ts to use nullish coalescing so empty prompt content is
preserved — specifically change the expression that sets prompt (currently using
perTargetPrompt || sub.prompt || sub.prompt_file || '') to use ?? between
perTargetPrompt, sub.prompt, and sub.prompt_file (so perTargetPrompt ??
sub.prompt ?? sub.prompt_file ?? '') thus keeping empty strings from sub.prompt
or a resolved prompt file rather than falling back to the file path.
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 75-79: initSession currently returns early when
loadCueConfig(session.projectRoot) is falsy, which prevents watchCueYaml and the
fallback watcher from being registered for sessions that start without cue.yaml;
change initSession so it does not return when config is missing and instead
always invoke the watcher registration logic (call watchCueYaml with the
session/projectRoot or register the fallback watcher path) so the code will
detect a newly created cue.yaml; keep using loadCueConfig to set up
watchers/config when present but ensure the fallback watcher path (the existing
watcher-registration code that runs after a prior config) is executed
unconditionally so sessions that boot without config still get hot-reload
discovery.
- Around line 160-162: The code currently calls deps.onPreventSleep only when
hasTimeBasedSubscriptions(config, session.id) is true but always calls
deps.onAllowSleep later, which can release a sleep block that was never
acquired; change the teardown logic so that deps.onAllowSleep is invoked only if
the corresponding onPreventSleep was previously called for this session (e.g.,
compute a local boolean like preventedSleep = hasTimeBasedSubscriptions(config,
session.id) before calling deps.onPreventSleep and then wrap the subsequent
deps.onAllowSleep(`cue:schedule:${session.id}`) in a conditional that checks
that same boolean), or alternatively track acquisition per-session and match
onAllowSleep to onPreventSleep using the same key `cue:schedule:${session.id}`
so you only release what you acquired.
- Around line 110-115: SubscriptionSetupDeps currently omits
dispatchSubscription causing heartbeat/scheduled/file/task/GitHub subscription
setups to call executeCueRun directly and bypass fan-out logic; add
dispatchSubscription to the SubscriptionSetupDeps type, pass the existing
dispatchSubscription implementation into setupDeps, and update the subscription
setup helpers for heartbeat/scheduled/file/task/GitHub to invoke
dispatchSubscription (which checks sub.fan_out and fan_out_prompts) when a
subscription has fan_out set, falling back to executeCueRun only when no fan-out
is present.
🪄 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: 7f149792-b417-4997-a470-e547126745c9
📒 Files selected for processing (3)
src/main/cue/config/cue-config-validator.tssrc/main/cue/cue-dispatch-service.tssrc/main/cue/cue-session-runtime-service.ts
Comment on lines
+75
to
+79
| function initSession(session: SessionInfo): void { | ||
| if (!deps.enabled()) return; | ||
|
|
||
| const config = loadCueConfig(session.projectRoot); | ||
| if (!config) return; |
There was a problem hiding this comment.
Sessions that start without cue.yaml never get a hot-reload watcher.
Returning at Line 79 skips watchCueYaml(), and the fallback watcher path in Lines 231-236 only runs after a session previously had config. A session that boots without Cue config will not auto-discover a newly created cue.yaml until some external full refresh happens.
💡 Suggested change
function initSession(session: SessionInfo): void {
if (!deps.enabled()) return;
+ const pendingWatcher = pendingYamlWatchers.get(session.id);
+ if (pendingWatcher) {
+ pendingWatcher();
+ pendingYamlWatchers.delete(session.id);
+ }
+
const config = loadCueConfig(session.projectRoot);
- if (!config) return;
+ if (!config) {
+ const yamlWatcher = watchCueYaml(session.projectRoot, () => {
+ deps.onRefreshRequested(session.id, session.projectRoot);
+ });
+ pendingYamlWatchers.set(session.id, yamlWatcher);
+ return;
+ }Also applies to: 231-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-session-runtime-service.ts` around lines 75 - 79,
initSession currently returns early when loadCueConfig(session.projectRoot) is
falsy, which prevents watchCueYaml and the fallback watcher from being
registered for sessions that start without cue.yaml; change initSession so it
does not return when config is missing and instead always invoke the watcher
registration logic (call watchCueYaml with the session/projectRoot or register
the fallback watcher path) so the code will detect a newly created cue.yaml;
keep using loadCueConfig to set up watchers/config when present but ensure the
fallback watcher path (the existing watcher-registration code that runs after a
prior config) is executed unconditionally so sessions that boot without config
still get hot-reload discovery.
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/config/cue-config-validator.ts`:
- Around line 55-58: The validator currently only requires at least one of
sub.prompt or sub.prompt_file to be a string, allowing an invalid sibling to
slip through; update the logic in the function that sets hasPrompt/hasPromptFile
(the validation block using variables hasPrompt, hasPromptFile, sub.prompt,
sub.prompt_file and errors.push with prefix) to also error when a sibling field
exists but is the wrong type: if sub.prompt is present and typeof sub.prompt !==
'string' push a descriptive error, and if sub.prompt_file is present and typeof
sub.prompt_file !== 'string' push a descriptive error; keep the existing check
that at least one of them must be provided and only treat a field as valid when
it is a string.
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 175-178: The log is counting all config.subscriptions instead of
only those wired to this session; replace the filter logic used in the
deps.onLog calls (around the initialization and the later block at lines
~233-240) with the same predicate used by countActiveSubscriptions(...) in
cue-session-state.ts so the log and activeCount reflect subscriptions actually
targeted to this session (use countActiveSubscriptions(session, config) or
replicate its predicate when computing the number to log), and update both
occurrences to use that function/predicate.
In `@src/main/cue/cue-subscription-setup.ts`:
- Around line 85-97: The non-fan-out direct-execute path in
dispatchOrExecuteSubscription is using sub.prompt_file ?? sub.prompt, which
flips the intended precedence and passes file paths into executeCueRun instead
of hydrated prompt contents; change the argument order to sub.prompt ??
sub.prompt_file so executeCueRun receives the hydrated prompt when present
(mirroring the precedence used by cue-dispatch-service.ts), leaving
dispatchSubscription behavior unchanged.
🪄 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: d339739a-e53b-46a0-8b61-ed09a3dc0a68
📒 Files selected for processing (5)
src/main/cue/config/cue-config-validator.tssrc/main/cue/cue-dispatch-service.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/main/cue/cue-subscription-setup.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/cue/cue-dispatch-service.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/cue/cue-session-runtime-service.ts (1)
305-308: Consider renaming internal function to clarify intent.The public
removeSessionmethod correctly delegates to the internalremoveSessionfunction (scope resolution finds the outer function at line 263), but the identical naming could confuse future maintainers reading this code.♻️ Rename internal function for clarity
- function removeSession(sessionId: string): void { + function removeSessionInternal(sessionId: string): void { teardownSession(sessionId); sessions.delete(sessionId); deps.clearQueue(sessionId); @@ removeSession(sessionId: string): void { - removeSession(sessionId); + removeSessionInternal(sessionId); deps.onLog('cue', `[CUE] Session removed: ${sessionId}`); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-session-runtime-service.ts` around lines 305 - 308, The public method removeSession currently calls an identically named inner function removeSession (shadowing the outer method) which can be confusing; rename the internal function (the one declared around the earlier scope, e.g., the function currently referenced by removeSession at line ~263) to a clearer name such as removeSessionInternal or destroySession, update this public method to call that new name, and update any other call sites that reference the old internal function name to the new identifier to avoid breaking references.src/main/cue/config/cue-config-validator.ts (2)
197-207: Theapp.startupbranch is dead code.Since
'app.startup'is included inCUE_EVENT_TYPES(persrc/shared/cue/contracts.ts:34), the unknown-event check at lines 199-207 would never fire for it anyway. The explicit branch is harmless but redundant.♻️ Simplify by removing the no-op branch
- } else if (event === 'app.startup') { - // No additional required fields for the startup trigger. - } else if ( + } else if ( sub.event && typeof sub.event === 'string' && !CUE_EVENT_TYPES.includes(event as any)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/config/cue-config-validator.ts` around lines 197 - 207, Remove the redundant no-op branch that checks for event === 'app.startup' in the validation chain: delete the else if (event === 'app.startup') { /* No additional required fields for the startup trigger. */ } block so the subsequent validation that uses sub.event, typeof sub.event, and CUE_EVENT_TYPES.includes(event) runs directly; ensure the remaining conditions referencing event, sub.event, CUE_EVENT_TYPES, prefix and errors remain intact and the if/else chain still compiles.
62-66: Empty stringprompt/prompt_filevalues are accepted.The type validation at lines 55-60 correctly rejects non-string types, but
prompt: ""andprompt_file: ""will satisfyhasPrompt/hasPromptFileand pass validation despite being unusable.🔧 Proposed tightening
- const hasPrompt = typeof sub.prompt === 'string'; - const hasPromptFile = typeof sub.prompt_file === 'string'; + const hasPrompt = typeof sub.prompt === 'string' && sub.prompt.trim().length > 0; + const hasPromptFile = typeof sub.prompt_file === 'string' && sub.prompt_file.trim().length > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/config/cue-config-validator.ts` around lines 62 - 66, The validator currently treats empty strings as valid because hasPrompt/hasPromptFile only check typeof string; change those checks in cue-config-validator (the hasPrompt and hasPromptFile checks) to require non-empty trimmed strings (e.g., typeof sub.prompt === 'string' && sub.prompt.trim().length > 0 and similarly for sub.prompt_file), then rely on the existing branch that calls errors.push(`${prefix}: "prompt" or "prompt_file" is required`) so empty values are rejected as unusable.
🤖 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/config/cue-config-validator.ts`:
- Around line 197-207: Remove the redundant no-op branch that checks for event
=== 'app.startup' in the validation chain: delete the else if (event ===
'app.startup') { /* No additional required fields for the startup trigger. */ }
block so the subsequent validation that uses sub.event, typeof sub.event, and
CUE_EVENT_TYPES.includes(event) runs directly; ensure the remaining conditions
referencing event, sub.event, CUE_EVENT_TYPES, prefix and errors remain intact
and the if/else chain still compiles.
- Around line 62-66: The validator currently treats empty strings as valid
because hasPrompt/hasPromptFile only check typeof string; change those checks in
cue-config-validator (the hasPrompt and hasPromptFile checks) to require
non-empty trimmed strings (e.g., typeof sub.prompt === 'string' &&
sub.prompt.trim().length > 0 and similarly for sub.prompt_file), then rely on
the existing branch that calls errors.push(`${prefix}: "prompt" or "prompt_file"
is required`) so empty values are rejected as unusable.
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 305-308: The public method removeSession currently calls an
identically named inner function removeSession (shadowing the outer method)
which can be confusing; rename the internal function (the one declared around
the earlier scope, e.g., the function currently referenced by removeSession at
line ~263) to a clearer name such as removeSessionInternal or destroySession,
update this public method to call that new name, and update any other call sites
that reference the old internal function name to the new identifier to avoid
breaking references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4434cee7-26c5-4b98-bd39-5e6855bc99b3
📒 Files selected for processing (4)
src/__tests__/main/cue/cue-engine.test.tssrc/main/cue/config/cue-config-validator.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-subscription-setup.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/main/cue/cue-engine.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/cue/cue-subscription-setup.ts
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
1 participant
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
This PR continues the Cue refactor with a strict behavior-preserving goal.
It reorganizes Cue in three architectural steps:
CueEngineruntime orchestration into focused services while keepingCueEngineas the public facadeThe intent is to reduce responsibility overlap in Cue without changing:
Commit Stack
1.
40029853refactor(cue): extract shared contracts and config boundariesIntroduces a shared Cue contract layer and moves Cue config responsibilities behind stable facades.
Included changes:
src/shared/cue/src/main/cue/cue-types.tsinto a compatibility facade over shared typescue-yaml-loader.tsresponsibilities into:loadCueConfig,watchCueYaml,validateCueConfig, andresolveCueConfigPathentrypoints2.
1fdd50e6refactor(cue): extract runtime orchestration servicesDecomposes the core runtime logic that had accumulated inside
CueEngine.Included changes:
cue-session-state.tscue-dispatch-service.tscue-completion-service.tscue-query-service.tscue-session-runtime-service.tsCueEngineas the compatibility facade for callers while delegating internally to the new servicesWhat Changed
Shared contracts
Cue shared DTOs now live in one place and are consumed consistently by:
This removes duplicated Cue type declarations and makes the boundary between shared and runtime-only types explicit.
Config boundaries
Cue config handling is now split by responsibility:
The old
cue-yaml-loader.tsmodule remains the stable public facade, so runtime callers and tests do not need a contract change.Runtime orchestration
CueEngineno longer owns all of the following directly in one file:Those concerns now live in dedicated services, while
CueEngineremains the integration surface used by the rest of the app.Behavior Preservation
This PR is intended to preserve all existing Cue behavior.
Specifically unchanged:
.maestro/cue.yamland legacy config supportagent.completedchaining semanticsWhy This Refactor
Cue had become difficult to reason about because the architecture mixed:
This PR reduces that coupling so later refactor phases can proceed with less risk and smaller diffs.
Testing
Verified with:
npm run lintnpm run test -- src/__tests__/main/cue/cue-yaml-loader.test.ts src/__tests__/main/cue/cue-ipc-handlers.test.ts src/__tests__/main/cue/cue-engine.test.ts src/__tests__/main/cue/cue-concurrency.test.ts src/ __tests__/main/cue/cue-completion-chains.test.ts src/__tests__/main/cue/cue-startup.test.ts src/__tests__/main/cue/cue-executor.test.ts src/__tests__/main/cue/cue-sleep-wake.test.ts src/__tests__/main/cue/ cue-sleep-prevention.test.ts src/__tests__/main/cue/cue-session-lifecycle.test.ts src/__tests__/main/cue/cue-multi-hop-chains.test.tsManual Regression Checklist
.maestro/cue.yamland confirm Cue is discovered.maestro-cue.yamland confirm fallback still works.Config removedand tears down the session.file.changedsubscription and confirm exactly one run fires.time.heartbeatsubscription and confirm interval behavior still works.time.scheduledsubscription and confirm no same-minute double-fire regression.app.startupstill fires once per boot only.agent.completedchaining still passes{{CUE_SOURCE_OUTPUT}}.prompt_fileandoutput_prompt_filestill resolve correctly.Summary by CodeRabbit
New Features
Refactor