refactor(cue): harden config boundary, introduce session registry, and isolate trigger sources#779
Merged
reachrazamair merged 5 commits intorcfrom Apr 10, 2026
Merged
refactor(cue): harden config boundary, introduce session registry, and isolate trigger sources#779reachrazamair merged 5 commits intorcfrom
reachrazamair merged 5 commits intorcfrom
Conversation
Closes the Phase 2 loopholes left after the initial config-boundary split:
- Delete the executor's prompt-file resolver. The normalizer already reads
prompt files at config-load time, so the executor's `endsWith('.md')` /
`fs.readFileSync` branch was dead code AND a footgun for inline prompts
ending in a recognized extension. The executor now treats `promptPath` as
inline content unconditionally and fails fast on empty content.
- Drop the dead `?? sub.prompt_file ?? ''` fallback in subscription-setup
and dispatch-service. Post-normalization `sub.prompt` is always a string,
so the fallback never fired and only obscured the contract.
- Remove `prompt_file` and `output_prompt_file` from the shared
CueSubscription contract. These fields were stripped of meaning after
materialization but kept leaking out as orphan metadata. They now live
only on the internal CueSubscriptionDocument used by the normalizer.
- Add `loadCueConfigDetailed()` returning a structured result that
distinguishes missing / parse-error / invalid / valid configs and carries
non-fatal warnings. The session runtime service migrates to this entry
point so the engine logs *why* a config failed (not just "missing") and
surfaces unresolved prompt_file references via the warnings array.
- Move the prompt-file-missing warning out of session-runtime-service and
into the materializer where it belongs, then re-emit through the loader's
warnings array. Single owner, single source.
- Push `cue:readYaml` / `cue:writeYaml` / `cue:deleteYaml` IPC handlers
through cue-config-repository. Add `writeCueConfigFile`,
`deleteCueConfigFile`, and `writeCuePromptFile` so the repository owns
every fs/path/mkdir for `.maestro/cue.yaml` and `.maestro/prompts/`.
- Extract pipeline-layout-store as the single owner of the
`cue-pipeline-layout.json` file under userData. The IPC handler is now
a thin transport with no direct fs/path imports.
Tests:
- 17 new cue-config-repository unit tests covering canonical-vs-legacy
fallback, write/delete/promptFile semantics, and directory creation.
- 8 new loadCueConfigDetailed tests covering parse-error, invalid, and
warning paths.
- Updated 8 cue test files to expose `loadCueConfigDetailed` on their
yaml-loader mock (auto-derived from the existing `loadCueConfig` mock).
- Rewrote cue-ipc-handlers tests to mock the repository module directly,
matching the new thin-transport shape.
- Updated cue-executor tests to assert the executor never touches the
filesystem for prompt resolution.
Cue suite: 584 passing (up from 556). Full repo: 24,699 passing.
Closes the Phase 3 loopholes left after the initial engine decomposition.
The critical fix is L6: session state and dedup keys now have a single owner.
CueSessionRegistry (new)
- Sole owner of `Map<sessionId, SessionState>` plus the `time.scheduled` and
`app.startup` dedup key sets that previously lived on CueEngine and were
mutated by three different files (engine, runtime service, subscription
setup).
- Public surface forces all dedup mutation through typed methods:
markScheduledFired/evictStaleScheduledKeys/clearScheduledForSession and
markStartupFired/clearStartupForSession.
- `clear()` matches the old engine.stop() semantics: drops sessions and
scheduled keys but PRESERVES startup keys, so toggling Cue off/on does
not re-fire app.startup subscriptions. This is the regression target.
CueRecoveryService (new)
- Wraps three previously-scattered concerns: DB init+prune at engine start,
sleep-gap detection, and missed-event reconciliation. Engine no longer
imports cue-db directly.
- cue-heartbeat.ts shrinks to just the 30s writer loop. The
detectSleepAndReconcile logic moved to the recovery service so engine and
bootstrap each call a single method.
CueEngine slimming
- Removes the private scheduledFiredKeys/startupFiredKeys/isBootScan fields.
All session state lives behind the registry.
- start() takes a SessionInitReason instead of an isSystemBoot boolean.
start('system-boot') fires app.startup; start() / start('user-toggle')
does not. The intent now lives in the call signature, not a closure flag.
- All getSessionStates() / getSessionConfigs() callsites read from the
registry directly; CueSessionRuntimeService no longer exposes those.
- Bootstrap is now: recoveryService.init() → registry-driven session loop →
recoveryService.detectSleepAndReconcile() → heartbeat.start().
CueSessionRuntimeService
- initSession takes an explicit `{ reason }` parameter.
- All dedup mutation goes through the registry. The function no longer
receives Set instances by reference.
- refreshSession passes reason='refresh' (so YAML hot-reload doesn't re-fire
app.startup). useCueAutoDiscovery callers will pass 'discovery' next.
cue-subscription-setup.ts
- setupScheduledSubscription now uses registry.markScheduledFired and
registry.evictStaleScheduledKeys instead of mutating a shared Set. This
module is being deprecated entirely in Commit 3 (Phase 4) but compiles
cleanly against the registry interface in the meantime.
L9 — triggerSubscription now reads the registry directly instead of
calling sessionRuntimeService.getSessionStates().
L10 — cue-engine.ts no longer re-exports calculateNextScheduledTime for
test compat. cue-engine.test.ts imports it directly from the real home
in cue-subscription-setup.
Tests
- 15 new CueSessionRegistry unit tests covering register/snapshot,
scheduled dedup (mark/evict/clear), startup dedup, and the clear()
preservation policy that protects toggle-on/off behaviour.
- 10 new CueRecoveryService unit tests covering DB init success/failure,
sleep-gap thresholds, reconciliation dispatch, and shutdown safety.
- 7 new init-reason matrix tests in cue-session-lifecycle.test.ts covering
every entry to initSession (system-boot, user-toggle, refresh, removal).
- Updated existing engine.start(true) callsites to engine.start('system-boot')
across cue-startup.test.ts (19 tests) and cue-concurrency.test.ts.
Cue suite: 616 passing (up from 584). Full repo: 24,731 passing.
Cue refactor: each trigger type becomes a first-class source
object that owns its own underlying mechanism (interval, watcher, poller).
The cue-subscription-setup.ts orchestration hub is deleted.
New module: src/main/cue/triggers/
cue-trigger-source.ts interface + context contract
cue-trigger-filter.ts centralized passesFilter helper (closes L12)
cue-schedule-utils.ts calculateNextScheduledTime (was in setup)
cue-trigger-source-registry.ts factory: event type → source
cue-heartbeat-trigger-source.ts setInterval-backed
cue-scheduled-trigger-source.ts per-minute polling + registry dedup
cue-file-watcher-trigger-source.ts wraps cue-file-watcher.ts
cue-task-scanner-trigger-source.ts wraps cue-task-scanner.ts
cue-github-poller-trigger-source.ts wraps cue-github-poller.ts
Each source exposes start() / stop() / nextTriggerAt() and emits events
through a context-supplied `emit` callback. The runtime never has to know
whether a source owns timers, watchers, or pollers — teardown is just
`source.stop()` for each entry.
L12 — the `if (sub.filter && !matchesFilter(...))` check that previously
appeared 6+ times now lives in `passesFilter`. Every trigger source calls
it exactly once before invoking `emit`.
L13 — the heartbeat fire-immediately path no longer duplicates the on-interval
path; both go through one `fire()` helper inside the heartbeat source.
L14 — `dispatchOrExecuteSubscription` (which duplicated CueDispatchService's
internal fan-out check) is gone. Trigger sources go straight to
`dispatchSubscription` via the runtime's emit callback. The dispatch service
is the single owner of the fan-out vs single-target decision.
cue-session-state.ts: SessionState shape changes
- drop `timers: Timer[]`, `watchers: (() => void)[]`, and the
`nextTriggers: Map<string, number>` projection map
- add `triggerSources: CueTriggerSource[]`
- `getEarliestNextTriggerIso` now iterates trigger sources and calls
their `nextTriggerAt()` instead of reading the projection map
cue-session-runtime-service.ts: thin trigger-wiring loop
- replaces the imperative if/else ladder with a single call to
`createTriggerSource(sub.event, ctx)` per subscription
- app.startup loop now uses `passesFilter` instead of inlined matchesFilter
- drops the now-unused `executeCueRun` callback (everything goes through
`dispatchSubscription`)
- teardown calls `source.stop()` on each trigger source
cue-engine.ts: drops the `executeCueRun` wiring on the runtime service.
cue-subscription-setup.ts: DELETED. The 320 lines of imperative trigger setup
+ the duplicated `dispatchOrExecuteSubscription` helper are gone.
Tests
- 56 new tests across 7 files in src/__tests__/main/cue/triggers/:
- cue-trigger-filter.test.ts (5 tests)
- cue-trigger-source-registry.test.ts (12 tests, every event type)
- cue-heartbeat-trigger-source.test.ts (9 tests)
- cue-scheduled-trigger-source.test.ts (8 tests)
- cue-file-watcher-trigger-source.test.ts (7 tests)
- cue-task-scanner-trigger-source.test.ts (7 tests)
- cue-github-poller-trigger-source.test.ts (8 tests)
- cue-engine.test.ts: updated calculateNextScheduledTime import from the
deleted cue-subscription-setup.ts to triggers/cue-schedule-utils.
Cue suite: 672 passing (up from 616). Full repo: 24,787 passing.
The 17 existing trigger-related integration tests (cue-startup, cue-concurrency,
cue-completion-chains, cue-multi-hop-chains, cue-engine, cue-session-lifecycle)
all pass without any assertion changes — only the architecture moved.
📝 WalkthroughWalkthroughIntroduces a trigger-source architecture and central session registry, moves prompt materialization into config-load ( Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Engine as CueEngine
participant Recovery as CueRecoveryService
participant DB as CueDB
participant Registry as CueSessionRegistry
App->>Engine: start('system-boot')
Engine->>Recovery: init()
Recovery->>DB: initCueDb()
Recovery->>DB: pruneCueEvents()
Recovery-->>Engine: { ok: true }
Engine->>Registry: register(sessionId, state)
Engine->>Engine: initSession(session, { reason: 'system-boot' })
sequenceDiagram
participant Runtime as SessionRuntimeService
participant Factory as TriggerSourceRegistry
participant Source as TriggerSource
participant Registry as CueSessionRegistry
participant Dispatch as DispatchService
Runtime->>Factory: createTriggerSource(eventType, ctx)
Factory->>Source: instantiate
Runtime->>Source: start()
Source->>Source: poll / listen / watch
Source->>Runtime: emit(event)
Runtime->>Registry: markScheduledFired(...)
Runtime->>Dispatch: dispatchSubscription(sessionId, sub, event)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 is a large, well-structured refactor of the Maestro Cue subsystem across three logical commits: (1) a config boundary that strips Confidence Score: 4/5Safe to merge after fixing the JSON.parse crash in loadPipelineLayout; all other findings are P2 One P1 defect: loadPipelineLayout throws on a corrupt layout file instead of returning null, causing every cue:loadPipelineLayout IPC call to reject persistently. The two P2 findings (double YAML parse, missing path-escape guard) are non-blocking. The structural refactor is sound — registry ownership, discriminated union config loading, and trigger source abstraction all look correct. src/main/cue/pipeline-layout-store.ts (P1 JSON.parse); src/main/cue/config/cue-config-repository.ts (P2 path guard); src/main/cue/cue-yaml-loader.ts (P2 double parse) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Config["Config Boundary (Commit 1)"]
YAML["cue.yaml on disk"] --> Repo["cue-config-repository\nreadCueConfigFile / watchCueConfigFile"]
Repo --> Loader["cue-yaml-loader\nloadCueConfigDetailed()"]
Loader --> Normalizer["cue-config-normalizer\nparseCueConfigDocument()\nmaterializeCueConfig()"]
Normalizer -->|"strips prompt_file → inline prompt\nreturns warnings"| Config_RT["CueConfig runtime model\n(no prompt_file field)"]
end
subgraph Registry["Session Registry (Commit 2)"]
CueEngine["CueEngine\nstart(reason: SessionInitReason)"] --> RegObj["CueSessionRegistry\nsessions Map\nscheduledFiredKeys Set\nstartupFiredKeys Set"]
CueEngine --> Recovery["CueRecoveryService\nDB init / prune\nsleep detection / reconcile"]
CueEngine --> Heartbeat["CueHeartbeat\n30s writer loop"]
end
subgraph Triggers["Trigger Sources (Commit 3)"]
RuntimeSvc["CueSessionRuntimeService\ninitSession / refreshSession / removeSession"] --> Factory["createTriggerSource(eventType, ctx)"]
Factory --> HB["HeartbeatTriggerSource\nsetInterval"]
Factory --> Sched["ScheduledTriggerSource\nsetInterval 60s + markScheduledFired"]
Factory --> FW["FileWatcherTriggerSource\nchokidar"]
Factory --> TS["TaskScannerTriggerSource\npoll"]
Factory --> GH["GitHubPollerTriggerSource\npoll"]
Factory -->|"null for runtime-handled"| Direct["app.startup / agent.completed\n(handled directly by runtime)"]
RuntimeSvc --> TrigArr["SessionState.triggerSources[]"]
end
Config_RT --> RuntimeSvc
RegObj --> RuntimeSvc
TrigArr -->|"source.stop() loop on teardown"| Teardown["Clean teardown"]
Reviews (1): Last reviewed commit: "refactor(cue): isolate trigger sources b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+29
to
+35
| export function loadPipelineLayout(): PipelineLayoutState | null { | ||
| const filePath = getLayoutFilePath(); | ||
| if (!fs.existsSync(filePath)) { | ||
| return null; | ||
| } | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| return JSON.parse(content) as PipelineLayoutState; |
There was a problem hiding this comment.
JSON.parse throws on a corrupt layout file
loadPipelineLayout returns null when the file is missing, but throws a SyntaxError when the file exists but is corrupt. withIpcErrorLogging re-throws the exception, so every call to cue:loadPipelineLayout will reject — the renderer gets an error instead of the graceful null path it expects — until the user manually deletes cue-pipeline-layout.json.
Suggested change
| export function loadPipelineLayout(): PipelineLayoutState | null { | |
| const filePath = getLayoutFilePath(); | |
| if (!fs.existsSync(filePath)) { | |
| return null; | |
| } | |
| const content = fs.readFileSync(filePath, 'utf-8'); | |
| return JSON.parse(content) as PipelineLayoutState; | |
| export function loadPipelineLayout(): PipelineLayoutState | null { | |
| const filePath = getLayoutFilePath(); | |
| if (!fs.existsSync(filePath)) { | |
| return null; | |
| } | |
| try { | |
| const content = fs.readFileSync(filePath, 'utf-8'); | |
| return JSON.parse(content) as PipelineLayoutState; | |
| } catch { | |
| return null; | |
| } | |
| } |
Comment on lines
+43
to
+68
|
|
||
| let parsed: unknown; | ||
| try { | ||
| parsed = yaml.load(file.raw); | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| reason: 'parse-error', | ||
| message: err instanceof Error ? err.message : String(err), | ||
| }; | ||
| } | ||
|
|
||
| if (!parsed || typeof parsed !== 'object') { | ||
| return { | ||
| ok: false, | ||
| reason: 'parse-error', | ||
| message: 'Cue config root must be a YAML mapping', | ||
| }; | ||
| } | ||
|
|
||
| const validation = validateCueConfigDocument(parsed); | ||
| if (!validation.valid) { | ||
| return { ok: false, reason: 'invalid', errors: validation.errors }; | ||
| } | ||
|
|
||
| const document = parseCueConfigDocument(file.raw, projectRoot); |
There was a problem hiding this comment.
loadCueConfigDetailed
file.raw is parsed once inline via yaml.load(file.raw) (line 46) for validation, then parseCueConfigDocument(file.raw, projectRoot) (line 68) calls yaml.load a second time internally. For a config file that just passed validation this is harmless, but it's redundant work on every hot-reload. Consider passing the already-parsed object to a variant of parseCueConfigDocument that accepts pre-parsed YAML, or extract the shared parse result.
Comment on lines
+86
to
+101
| export function writeCuePromptFile( | ||
| projectRoot: string, | ||
| relativePath: string, | ||
| content: string | ||
| ): string { | ||
| const promptsDir = path.join(projectRoot, CUE_PROMPTS_DIR); | ||
| if (!fs.existsSync(promptsDir)) { | ||
| fs.mkdirSync(promptsDir, { recursive: true }); | ||
| } | ||
| const absPath = path.join(projectRoot, relativePath); | ||
| const dir = path.dirname(absPath); | ||
| if (!fs.existsSync(dir)) { | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| } | ||
| fs.writeFileSync(absPath, content, 'utf-8'); | ||
| return absPath; |
There was a problem hiding this comment.
relativePath not validated against project root escape
path.join(projectRoot, relativePath) resolves ../ segments, so a caller that sends ../../etc/cron.d/job via the cue:writeYaml IPC message would write outside projectRoot. In practice the renderer is trusted, but a guard makes the boundary explicit:
const absPath = path.join(projectRoot, relativePath);
if (!absPath.startsWith(path.resolve(projectRoot) + path.sep)) {
throw new Error(`writeCuePromptFile: relativePath escapes projectRoot: ${relativePath}`);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
src/main/cue/cue-dispatch-service.ts (1)
62-66: Prefer explicit missing-prompt guard over silent''fallback.Lines 65 and 80 currently mask contract drift by dispatching empty prompts. Consider logging and skipping when prompt is unexpectedly missing.
Suggested refactor
const perTargetPrompt = sub.fan_out_prompts?.[i]; - const prompt = perTargetPrompt ?? sub.prompt ?? ''; + const prompt = perTargetPrompt ?? sub.prompt; + if (prompt === undefined) { + deps.onLog('error', `[CUE] Missing prompt for subscription "${sub.name}"`); + continue; + } @@ deps.executeRun( ownerSessionId, - sub.prompt ?? '', + sub.prompt, event, sub.name, sub.output_prompt, chainDepth );Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-dispatch-service.ts` around lines 62 - 66, The code currently falls back to an empty string for a missing prompt when building `prompt` from `perTargetPrompt ?? sub.prompt ?? ''`; change this to explicitly detect a missing prompt (both `perTargetPrompt` and `sub.prompt` undefined/null/empty) and skip the `deps.executeRun` call, logging a warning that includes identifying info (e.g., `sub` id and target index `i`) so contract drift is visible; apply the same guard where `prompt` is used later (lines around use of `perTargetPrompt`/`sub.prompt` and the other occurrence at 78–81) to avoid dispatching empty prompts.src/__tests__/main/cue/cue-session-lifecycle.test.ts (1)
583-601: Last startup-key test does not verify the “can re-fire” claim yet.The test currently proves “no re-fire on refresh/user-toggle,” but not re-fire on a later
system-bootafter re-adding the session.💡 Suggested assertion extension
engine.refreshSession('session-1', '/projects/test'); expect(deps.onCueRun).toHaveBeenCalledTimes(1); + // After re-add, a real system boot should be eligible to fire startup again. + engine.stop(); + engine.start('system-boot'); + expect(deps.onCueRun).toHaveBeenCalledTimes(2); + engine.stop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-session-lifecycle.test.ts` around lines 583 - 601, Extend the test so it also verifies that after removing the session and refreshing (which shouldn't re-fire), a subsequent real boot does re-trigger startup: after the existing refreshSession('session-1', '/projects/test') assertion, call engine.start('system-boot') again and assert deps.onCueRun has been called one more time (e.g., toHaveBeenCalledTimes(2)); use the same CueEngine instance and existing helpers (makeStartupConfig, createMockDeps, mockLoadCueConfig) and keep the final engine.stop().src/main/cue/triggers/cue-trigger-source-registry.ts (1)
32-50: Add an explicit fallback return path for unsupported event values.Without a fallback, unsupported runtime values can implicitly return
undefinedinstead ofnulland skip observability.As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly."💡 Proposed hardening
switch (eventType) { case 'time.heartbeat': return createCueHeartbeatTriggerSource(ctx); @@ case 'app.startup': // These are not timer/watcher-driven — the runtime handles them // directly via the completion service / startup loop. return null; + default: + ctx.onLog('cue', `[CUE] Unsupported trigger event type: "${String(eventType)}"`); + return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/triggers/cue-trigger-source-registry.ts` around lines 32 - 50, The switch over eventType (which returns various createCue*TriggerSource functions like createCueHeartbeatTriggerSource, createCueScheduledTriggerSource, createCueFileWatcherTriggerSource, createCueTaskScannerTriggerSource and createCueGitHubPollerTriggerSource) currently has no default branch and can implicitly return undefined for unsupported event values; add an explicit default case that returns null and emits an observability signal (e.g. ctx.logger.warn or console.warn) referencing the unknown eventType so unsupported events are handled explicitly rather than silently producing undefined.src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts (1)
111-125: Add a negative case for GitHub subscriptions missingrepo.The suite currently validates positive GitHub paths but doesn’t protect against misconfigured GitHub subscriptions. Adding explicit
repo-missing assertions would prevent regressions in factory guard behavior.Proposed test additions
+ it('returns null for github.pull_request missing repo', () => { + const source = createTriggerSource( + 'github.pull_request', + makeCtx(baseSub('github.pull_request')) + ); + expect(source).toBeNull(); + }); + + it('returns null for github.issue missing repo', () => { + const source = createTriggerSource('github.issue', makeCtx(baseSub('github.issue'))); + expect(source).toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts` around lines 111 - 125, Add negative tests that assert createTriggerSource returns null when GitHub subscriptions lack the required repo field: add two new cases mirroring the existing positives but use makeCtx(baseSub('github.pull_request', {})) and makeCtx(baseSub('github.issue', {})) (or equivalent missing-repo payloads) and expect the resulting source toBeNull(); place these next to the existing tests that use createTriggerSource, makeCtx, and baseSub so the factory guard behavior for github.pull_request and github.issue is covered for missing repo.src/__tests__/main/cue/cue-engine.test.ts (1)
1768-1777: Tighten the invalid-time test expectation.The current assertion accepts both outcomes, so it won’t catch accidental rollover scheduling behavior. Prefer asserting
nullfor invalid time inputs to make this test protective.Proposed test update
it('returns null for invalid time strings', () => { vi.setSystemTime(new Date('2026-03-09T08:00:00')); const result = calculateNextScheduledTime(['25:99']); - // Invalid hours/minutes — parseInt yields 25 and 99, but the resulting - // Date will roll over. The function still produces a candidate because - // Date constructor handles overflow. Check it doesn't crash. - // With hour=25, the date rolls to next day 01:XX — still a valid timestamp. - // This is acceptable behavior (no crash), but let's verify it returns something. - expect(typeof result === 'number' || result === null).toBe(true); + expect(result).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-engine.test.ts` around lines 1768 - 1777, The test for invalid time strings should assert that calculateNextScheduledTime(['25:99']) returns null rather than allowing any number; update the assertion in the test case (the one that calls calculateNextScheduledTime with ['25:99'] and sets system time via vi.setSystemTime) to expect strict null (e.g., expect(result).toBeNull()) so the test fails if the function accidentally accepts rollover times instead of treating them as invalid.src/main/cue/cue-session-registry.ts (1)
22-30: The registry still leaks mutable session state.
get()andsnapshot()hand out liveSessionStateobjects, so callers can still mutatelastTriggered,triggerSources, and similar fields outside the registry. That weakens the “sole owner” boundary this refactor is trying to create. Consider returning readonly views here and adding narrow mutation APIs for the fields that legitimately need to change.Also applies to: 89-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-session-registry.ts` around lines 22 - 30, get() and snapshot() currently leak live mutable SessionState objects; change CueSessionRegistry so get(sessionId) returns a readonly/defensive copy (or an immutable ReadonlySessionState view) and snapshot() returns a Map of readonly/session-state copies to prevent external mutation, and add narrow mutation APIs (e.g., updateLastTriggered(sessionId, newTimestamp), addTriggerSource(sessionId, source), removeTriggerSource(sessionId, source)) to allow controlled updates; update register/unregister implementations to store the canonical mutable state internally while exposing only readonly views via get/snapshot and adjust any callers to use the new mutation methods.src/__tests__/main/cue/cue-yaml-loader.test.ts (1)
349-353: Avoid depending on validator error ordering in this test.
validateCueConfig()returns a list, so checkingresult.errors[0]makes this fail if another validation starts reporting first. Matching the collection keeps the test focused on behavior instead of ordering.Suggested fix
expect(result.ok).toBe(false); if (!result.ok && result.reason === 'invalid') { expect(result.errors.length).toBeGreaterThan(0); - expect(result.errors[0]).toMatch(/interval_minutes/); + expect(result.errors).toEqual( + expect.arrayContaining([expect.stringMatching(/interval_minutes/)]) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-yaml-loader.test.ts` around lines 349 - 353, The test relies on error ordering by asserting result.errors[0]; change the assertion to check the collection contains an error matching /interval_minutes/ instead of indexing into result.errors so it passes regardless of ordering — update the assertion that references result.errors (in the cue-yaml-loader.test around the validateCueConfig call) to use a collection-containing matcher that looks for a string matching /interval_minutes/ (e.g., expect.arrayContaining / expect.stringMatching style) while keeping the existing checks for result.ok and result.reason.
🤖 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-normalizer.ts`:
- Around line 193-201: The warning messages for prompt file resolution
(conditions using sub.promptSpec.file && !sub.promptSpec.inline && !sub.prompt
and sub.outputPromptSpec?.file && !sub.outputPromptSpec.inline &&
sub.output_prompt == null) overstate the failure as "file was not found"; update
the warnings.push text for both instances to reflect that the file either was
not found or resolved to empty/unreadable content (e.g., say "file was not found
or resolved to empty/unreadable content — subscription will fail on trigger" for
the prompt case and a similar wording for the output_prompt case) so the message
matches the actual failure modes detected by the existing condition.
In `@src/main/cue/config/cue-config-repository.ts`:
- Around line 86-101: The writeCuePromptFile function currently trusts
relativePath and can write outside CUE_PROMPTS_DIR; to fix, validate and
constrain the target to the prompts directory: reject absolute paths and
normalize the combined path using path.resolve(promptsDir, relativePath) (or
similar) then verify the resolved path starts with the resolved promptsDir
prefix; if the check fails, throw an error and do not write. After validation,
create parent dirs and write to the resolved path (use the resolved variable
instead of joining projectRoot + relativePath) so all filesystem writes are
confined to the .maestro/prompts boundary.
In `@src/main/cue/cue-executor.ts`:
- Around line 142-143: The current check in cue-executor.ts only tests falsy
promptContent, allowing whitespace-only prompts like ' ' to pass; change the
validation to treat whitespace-only as missing by trimming before checking:
e.g., compute a trimmedPrompt = promptContent?.trim() and then guard with if
(!trimmedPrompt) (or if (!promptContent || promptContent.trim().length === 0)),
and use trimmedPrompt (or keep using promptContent after validating) where you
need non-empty prompt content; reference the existing promptContent and
subscription.name symbols when updating the error message and subsequent logic.
In `@src/main/cue/cue-recovery-service.ts`:
- Around line 103-113: The catch blocks in the sleep-detection recovery code and
in shutdown() currently swallow errors (the catch in the try around sleep
detection and the try/catch inside shutdown() around closeCueDb()), so update
them to report errors to Sentry and/or the logging pipeline before re-throwing
or propagating as appropriate: call Sentry.captureException(error) (or
captureMessage with normalized error text) and deps.onLog('error', ...) with a
normalized message including the function context (e.g., "CUE sleep detection
failed" and "CUE shutdown/closeCueDb failed"), then re-throw or remove the
silent swallow so Sentry can record the failure; ensure you reference the
try/catch that wraps sleep detection and the shutdown() implementation that
calls closeCueDb().
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 119-179: Register the session state with
registry.register(session.id, state) before starting any trigger sources or
firing app.startup so other components (e.g., CueRunManager via
registry.get(sessionId)) see the session settings; move the call to
registry.register up to just after state.yamlWatcher is assigned and before the
for-loop that creates/starts sources and before the app.startup loop
(references: SessionState state, registry.register, createTriggerSource /
source.start, registry.markStartupFired).
In `@src/main/cue/pipeline-layout-store.ts`:
- Around line 29-36: In loadPipelineLayout(), guard the JSON.parse call so
malformed/partial JSON becomes a recoverable load failure: wrap the
fs.readFileSync + JSON.parse logic in a try/catch, return null on parse errors,
and call the Sentry helper(s) (captureException and/or captureMessage from
src/utils/sentry.ts) with context including the filePath and a short message
that the layout JSON is corrupted; keep the existing behavior of returning the
parsed PipelineLayoutState when successful and only treat other unexpected
errors similarly if they indicate corruption.
In `@src/main/cue/triggers/cue-heartbeat-trigger-source.ts`:
- Around line 26-35: The heartbeat handler updates nextFireMs only when an event
is emitted, so when passesFilter(ctx.subscription, event, ctx.onLog) returns
false nextTriggerAt() remains stuck; change fire(...) to advance nextFireMs =
Date.now() + intervalMs before the early return (i.e., set nextFireMs
immediately after logging/creating the event and before the passesFilter return
path), and apply the same fix to the other similar filtered-path (the block
around the second emits at the end of the file) so nextFireMs is always moved
forward even when ctx.emit is skipped.
In `@src/main/cue/triggers/cue-schedule-utils.ts`:
- Around line 41-50: The code parses hour/min from time but doesn't validate
bounds, allowing values outside 0–23 and 0–59 which Date.setHours will normalize
and produce rollover times; in the logic around time.split(':') / parseInt (the
hour, min variables) add explicit checks that hour is an integer between 0 and
23 and min is an integer between 0 and 59 (skip/continue on invalid), and only
then call target.setHours(hour, min, 0, 0) before comparing target.getTime()
with now and pushing to candidates.
In `@src/main/cue/triggers/cue-scheduled-trigger-source.ts`:
- Around line 76-79: The start() function currently waits for the first interval
tick and can miss a scheduled occurrence if created mid-minute; update start()
to perform an immediate check of the current minute before arming the timer: if
timer is falsy, run the same logic that checkAndFire() would use for the current
moment (or call checkAndFire() once) to detect and call markScheduledFired() for
a slot in the same minute, then set timer = setInterval(checkAndFire,
POLL_INTERVAL_MS) and call recomputeNextFire(); preserve the idempotent guard
(if (timer) return) and ensure this immediate check uses the same date/slot
computation as checkAndFire() to avoid duplicate firing.
In `@src/main/ipc/handlers/cue.ts`:
- Around line 221-223: Validate and sanitize options.promptFiles before calling
writeCuePromptFile: for each relativePath from options.promptFiles, reject or
normalize any absolute paths or paths containing ".." and compute const target =
path.resolve(options.projectRoot, ".maestro/prompts", relativePath); then verify
target.startsWith(path.resolve(options.projectRoot,
".maestro/prompts")+path.sep) (or equals the base) and only call
writeCuePromptFile with the original relativePath (or the safe relative from the
base) if the check passes; otherwise log/throw and skip writing. Use functions
path.resolve, path.normalize, and path.sep and reference options.promptFiles,
options.projectRoot and writeCuePromptFile to locate where to apply this check.
- Around line 219-224: The current flow calls writeCueConfigFile(...) before
writing prompt files, which can leave cue.yaml referencing missing prompt files
if a subsequent writeCuePromptFile(...) fails; change the order so you first
iterate over options.promptFiles and call
writeCuePromptFile(options.projectRoot, relativePath, content) for each
(propagating any errors immediately), and only after all prompt files succeed
call writeCueConfigFile(options.projectRoot, options.content) to persist
cue.yaml. Ensure you preserve existing error propagation behavior so failures
abort before writeCueConfigFile is invoked.
---
Nitpick comments:
In `@src/__tests__/main/cue/cue-engine.test.ts`:
- Around line 1768-1777: The test for invalid time strings should assert that
calculateNextScheduledTime(['25:99']) returns null rather than allowing any
number; update the assertion in the test case (the one that calls
calculateNextScheduledTime with ['25:99'] and sets system time via
vi.setSystemTime) to expect strict null (e.g., expect(result).toBeNull()) so the
test fails if the function accidentally accepts rollover times instead of
treating them as invalid.
In `@src/__tests__/main/cue/cue-session-lifecycle.test.ts`:
- Around line 583-601: Extend the test so it also verifies that after removing
the session and refreshing (which shouldn't re-fire), a subsequent real boot
does re-trigger startup: after the existing refreshSession('session-1',
'/projects/test') assertion, call engine.start('system-boot') again and assert
deps.onCueRun has been called one more time (e.g., toHaveBeenCalledTimes(2));
use the same CueEngine instance and existing helpers (makeStartupConfig,
createMockDeps, mockLoadCueConfig) and keep the final engine.stop().
In `@src/__tests__/main/cue/cue-yaml-loader.test.ts`:
- Around line 349-353: The test relies on error ordering by asserting
result.errors[0]; change the assertion to check the collection contains an error
matching /interval_minutes/ instead of indexing into result.errors so it passes
regardless of ordering — update the assertion that references result.errors (in
the cue-yaml-loader.test around the validateCueConfig call) to use a
collection-containing matcher that looks for a string matching
/interval_minutes/ (e.g., expect.arrayContaining / expect.stringMatching style)
while keeping the existing checks for result.ok and result.reason.
In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts`:
- Around line 111-125: Add negative tests that assert createTriggerSource
returns null when GitHub subscriptions lack the required repo field: add two new
cases mirroring the existing positives but use
makeCtx(baseSub('github.pull_request', {})) and makeCtx(baseSub('github.issue',
{})) (or equivalent missing-repo payloads) and expect the resulting source
toBeNull(); place these next to the existing tests that use createTriggerSource,
makeCtx, and baseSub so the factory guard behavior for github.pull_request and
github.issue is covered for missing repo.
In `@src/main/cue/cue-dispatch-service.ts`:
- Around line 62-66: The code currently falls back to an empty string for a
missing prompt when building `prompt` from `perTargetPrompt ?? sub.prompt ??
''`; change this to explicitly detect a missing prompt (both `perTargetPrompt`
and `sub.prompt` undefined/null/empty) and skip the `deps.executeRun` call,
logging a warning that includes identifying info (e.g., `sub` id and target
index `i`) so contract drift is visible; apply the same guard where `prompt` is
used later (lines around use of `perTargetPrompt`/`sub.prompt` and the other
occurrence at 78–81) to avoid dispatching empty prompts.
In `@src/main/cue/cue-session-registry.ts`:
- Around line 22-30: get() and snapshot() currently leak live mutable
SessionState objects; change CueSessionRegistry so get(sessionId) returns a
readonly/defensive copy (or an immutable ReadonlySessionState view) and
snapshot() returns a Map of readonly/session-state copies to prevent external
mutation, and add narrow mutation APIs (e.g., updateLastTriggered(sessionId,
newTimestamp), addTriggerSource(sessionId, source),
removeTriggerSource(sessionId, source)) to allow controlled updates; update
register/unregister implementations to store the canonical mutable state
internally while exposing only readonly views via get/snapshot and adjust any
callers to use the new mutation methods.
In `@src/main/cue/triggers/cue-trigger-source-registry.ts`:
- Around line 32-50: The switch over eventType (which returns various
createCue*TriggerSource functions like createCueHeartbeatTriggerSource,
createCueScheduledTriggerSource, createCueFileWatcherTriggerSource,
createCueTaskScannerTriggerSource and createCueGitHubPollerTriggerSource)
currently has no default branch and can implicitly return undefined for
unsupported event values; add an explicit default case that returns null and
emits an observability signal (e.g. ctx.logger.warn or console.warn) referencing
the unknown eventType so unsupported events are handled explicitly rather than
silently producing undefined.
🪄 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: d66124b6-8546-4bab-9435-617fd36ad44b
📒 Files selected for processing (46)
src/__tests__/main/cue/cue-completion-chains.test.tssrc/__tests__/main/cue/cue-concurrency.test.tssrc/__tests__/main/cue/cue-config-repository.test.tssrc/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/cue/cue-executor.test.tssrc/__tests__/main/cue/cue-ipc-handlers.test.tssrc/__tests__/main/cue/cue-multi-hop-chains.test.tssrc/__tests__/main/cue/cue-recovery-service.test.tssrc/__tests__/main/cue/cue-session-lifecycle.test.tssrc/__tests__/main/cue/cue-session-registry.test.tssrc/__tests__/main/cue/cue-sleep-prevention.test.tssrc/__tests__/main/cue/cue-sleep-wake.test.tssrc/__tests__/main/cue/cue-startup.test.tssrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/main/cue/triggers/cue-file-watcher-trigger-source.test.tssrc/__tests__/main/cue/triggers/cue-github-poller-trigger-source.test.tssrc/__tests__/main/cue/triggers/cue-heartbeat-trigger-source.test.tssrc/__tests__/main/cue/triggers/cue-scheduled-trigger-source.test.tssrc/__tests__/main/cue/triggers/cue-task-scanner-trigger-source.test.tssrc/__tests__/main/cue/triggers/cue-trigger-filter.test.tssrc/__tests__/main/cue/triggers/cue-trigger-source-registry.test.tssrc/main/cue/config/cue-config-normalizer.tssrc/main/cue/config/cue-config-repository.tssrc/main/cue/cue-dispatch-service.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-heartbeat.tssrc/main/cue/cue-recovery-service.tssrc/main/cue/cue-session-registry.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/main/cue/cue-subscription-setup.tssrc/main/cue/cue-yaml-loader.tssrc/main/cue/pipeline-layout-store.tssrc/main/cue/triggers/cue-file-watcher-trigger-source.tssrc/main/cue/triggers/cue-github-poller-trigger-source.tssrc/main/cue/triggers/cue-heartbeat-trigger-source.tssrc/main/cue/triggers/cue-schedule-utils.tssrc/main/cue/triggers/cue-scheduled-trigger-source.tssrc/main/cue/triggers/cue-task-scanner-trigger-source.tssrc/main/cue/triggers/cue-trigger-filter.tssrc/main/cue/triggers/cue-trigger-source-registry.tssrc/main/cue/triggers/cue-trigger-source.tssrc/main/index.tssrc/main/ipc/handlers/cue.tssrc/shared/cue/contracts.ts
💤 Files with no reviewable changes (1)
- src/main/cue/cue-subscription-setup.ts
Inline fixes: - cue-config-normalizer: warning text now says "not found or resolved to empty/unreadable content" instead of just "not found" for both prompt and output_prompt file cases - cue-config-repository: writeCuePromptFile rejects absolute paths and validates the resolved path stays within .maestro/prompts/ - cue-executor: whitespace-only prompt strings treated as missing via trim() - cue-recovery-service: both silent catch blocks now call captureException with context before continuing - cue-session-runtime-service: registry.register() moved before trigger source creation so other components see the session immediately - pipeline-layout-store: JSON.parse wrapped in try/catch; parse errors return null and report to Sentry - cue-heartbeat-trigger-source: nextFireMs advanced before passesFilter so nextTriggerAt() stays current even when the filter rejects - cue-schedule-utils: added hour 0-23 and minute 0-59 bounds validation - cue-scheduled-trigger-source: start() calls checkAndFire() immediately before arming the interval to avoid missing a mid-minute start - cue.ts IPC handler: prompt files validated and written before config YAML; absolute/traversal paths are rejected Nitpick fixes: - cue-engine.test: invalid time test asserts toBeNull(); three GitHub tests get missing repo fields; scheduled no-match test uses a non-matching minute - cue-session-lifecycle.test: startup re-fire test includes stop()/start() cycle; double-count test updated for registry-before-sources behavior - cue-yaml-loader.test: error assertion uses arrayContaining instead of [0] - cue-trigger-source-registry.test: two negative tests for GitHub subs missing repo field - cue-dispatch-service: missing/empty prompts skip executeRun with a warning - cue-trigger-source-registry: explicit default branch with never assertion - cue-github-poller-trigger-source: returns null when repo is missing - cue-config-repository.test: nested-path test expects single mkdirSync call
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts (2)
56-148: Consider parameterizing repetitive event matrix cases.This suite is very repetitive; a small table-driven structure (
it.each) would reduce maintenance overhead as new event types are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts` around lines 56 - 148, The test file repeats many similar assertions for createTriggerSource using makeCtx and baseSub; convert these into table-driven tests using Jest's it.each to reduce duplication. Create an array of cases describing eventName, subscriptionOverrides (e.g., { interval_minutes: 5 } or { watch: '**/*.ts' } or { repo: 'foo/bar' }), and expectedNull boolean, then replace the repeated it blocks with two parameterized suites (one for positive cases where expectedNull=false asserting source not null and function types for start/stop/nextTriggerAt when applicable, and one for negative cases where expectedNull=true asserting source is null) that call createTriggerSource(eventName, makeCtx(baseSub(eventName, subscriptionOverrides))). Ensure references to createTriggerSource, makeCtx, and baseSub are used so test behavior remains identical.
55-138: Assert provider selection explicitly, not only nullability.These tests can still pass if two event mappings are accidentally swapped but both return non-null sources with similar prerequisites. Add assertions on the mocked provider factory calls so each event validates the concrete branch taken.
✅ Example strengthening patch
-import { describe, it, expect, vi } from 'vitest'; +import { beforeEach, describe, it, expect, vi } from 'vitest'; import { createTriggerSource } from '../../../../main/cue/triggers/cue-trigger-source-registry'; import { createCueSessionRegistry } from '../../../../main/cue/cue-session-registry'; import type { CueEventType, CueSubscription } from '../../../../main/cue/cue-types'; import type { CueTriggerSourceContext } from '../../../../main/cue/triggers/cue-trigger-source'; +import { createCueFileWatcher } from '../../../../main/cue/cue-file-watcher'; +import { createCueTaskScanner } from '../../../../main/cue/cue-task-scanner'; +import { createCueGitHubPoller } from '../../../../main/cue/cue-github-poller'; @@ vi.mock('../../../../main/cue/cue-github-poller', () => ({ createCueGitHubPoller: vi.fn(() => vi.fn()), })); + +beforeEach(() => { + vi.clearAllMocks(); +}); @@ it('returns a source for file.changed with watch glob', () => { const source = createTriggerSource( 'file.changed', makeCtx(baseSub('file.changed', { watch: '**/*.ts' })) ); expect(source).not.toBeNull(); + expect(createCueFileWatcher).toHaveBeenCalledTimes(1); + expect(createCueTaskScanner).not.toHaveBeenCalled(); + expect(createCueGitHubPoller).not.toHaveBeenCalled(); }); @@ it('returns a source for task.pending with watch glob', () => { const source = createTriggerSource( 'task.pending', makeCtx(baseSub('task.pending', { watch: '**/*.md' })) ); expect(source).not.toBeNull(); + expect(createCueTaskScanner).toHaveBeenCalledTimes(1); + expect(createCueFileWatcher).not.toHaveBeenCalled(); + expect(createCueGitHubPoller).not.toHaveBeenCalled(); }); @@ it('returns a source for github.pull_request', () => { const source = createTriggerSource( 'github.pull_request', makeCtx(baseSub('github.pull_request', { repo: 'foo/bar' })) ); expect(source).not.toBeNull(); + expect(createCueGitHubPoller).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts` around lines 55 - 138, The tests currently only assert nullability of createTriggerSource results, which won’t catch swapped provider branches; after each createTriggerSource(...) call (using makeCtx and baseSub) add explicit assertions that the mocked provider factory was invoked for the expected provider key/branch so the concrete provider branch is validated (e.g., assert provider factory mock was called with the provider identifier for time.heartbeat, time.scheduled, file.changed, task.pending, github.pull_request, github.issue respectively); reference the existing symbols createTriggerSource, makeCtx, and baseSub when locating where to add the expect(...providerFactoryMock...).toHaveBeenCalledWith(...) assertions and use expect.anything() for irrelevant args if needed.src/__tests__/main/cue/cue-config-repository.test.ts (1)
173-216: Consider adding tests for path validation edge cases.The
writeCuePromptFileimplementation includes important security checks (rejecting absolute paths and paths that escape.maestro/prompts/), but this test suite doesn't cover those branches.💡 Suggested additional test cases
it('rejects absolute paths', () => { expect(() => writeCuePromptFile(PROJECT_ROOT, '/etc/passwd', 'content') ).toThrow('relativePath must be relative'); }); it('rejects paths that escape the prompts directory', () => { expect(() => writeCuePromptFile(PROJECT_ROOT, '.maestro/other/file.md', 'content') ).toThrow('resolves outside the prompts directory'); }); it('rejects traversal attempts', () => { expect(() => writeCuePromptFile(PROJECT_ROOT, '.maestro/prompts/../../etc/passwd', 'content') ).toThrow('resolves outside the prompts directory'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-config-repository.test.ts` around lines 173 - 216, Tests are missing coverage for path validation branches in writeCuePromptFile; add unit tests that assert it throws for absolute paths, for paths that resolve outside the prompts directory (e.g., sibling .maestro/other/file.md), and for traversal attempts (e.g., .maestro/prompts/../../etc/passwd). Locate writeCuePromptFile in the cue-config-repository tests and add three specs that call writeCuePromptFile(PROJECT_ROOT, <badPath>, 'content') and expect a thrown error with messages indicating "relativePath must be relative" or "resolves outside the prompts directory" to exercise those security checks (use the same error text your implementation throws). Ensure mocks for fs functions remain as in other tests so only validation triggers the exceptions.
🤖 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/__tests__/main/cue/cue-config-repository.test.ts`:
- Around line 173-216: Tests are missing coverage for path validation branches
in writeCuePromptFile; add unit tests that assert it throws for absolute paths,
for paths that resolve outside the prompts directory (e.g., sibling
.maestro/other/file.md), and for traversal attempts (e.g.,
.maestro/prompts/../../etc/passwd). Locate writeCuePromptFile in the
cue-config-repository tests and add three specs that call
writeCuePromptFile(PROJECT_ROOT, <badPath>, 'content') and expect a thrown error
with messages indicating "relativePath must be relative" or "resolves outside
the prompts directory" to exercise those security checks (use the same error
text your implementation throws). Ensure mocks for fs functions remain as in
other tests so only validation triggers the exceptions.
In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts`:
- Around line 56-148: The test file repeats many similar assertions for
createTriggerSource using makeCtx and baseSub; convert these into table-driven
tests using Jest's it.each to reduce duplication. Create an array of cases
describing eventName, subscriptionOverrides (e.g., { interval_minutes: 5 } or {
watch: '**/*.ts' } or { repo: 'foo/bar' }), and expectedNull boolean, then
replace the repeated it blocks with two parameterized suites (one for positive
cases where expectedNull=false asserting source not null and function types for
start/stop/nextTriggerAt when applicable, and one for negative cases where
expectedNull=true asserting source is null) that call
createTriggerSource(eventName, makeCtx(baseSub(eventName,
subscriptionOverrides))). Ensure references to createTriggerSource, makeCtx, and
baseSub are used so test behavior remains identical.
- Around line 55-138: The tests currently only assert nullability of
createTriggerSource results, which won’t catch swapped provider branches; after
each createTriggerSource(...) call (using makeCtx and baseSub) add explicit
assertions that the mocked provider factory was invoked for the expected
provider key/branch so the concrete provider branch is validated (e.g., assert
provider factory mock was called with the provider identifier for
time.heartbeat, time.scheduled, file.changed, task.pending, github.pull_request,
github.issue respectively); reference the existing symbols createTriggerSource,
makeCtx, and baseSub when locating where to add the
expect(...providerFactoryMock...).toHaveBeenCalledWith(...) assertions and use
expect.anything() for irrelevant args if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e22dc37-254b-41e7-ba03-46687c7674e6
📒 Files selected for processing (18)
src/__tests__/main/cue/cue-config-repository.test.tssrc/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/cue/cue-session-lifecycle.test.tssrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/main/cue/triggers/cue-trigger-source-registry.test.tssrc/main/cue/config/cue-config-normalizer.tssrc/main/cue/config/cue-config-repository.tssrc/main/cue/cue-dispatch-service.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-recovery-service.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/pipeline-layout-store.tssrc/main/cue/triggers/cue-github-poller-trigger-source.tssrc/main/cue/triggers/cue-heartbeat-trigger-source.tssrc/main/cue/triggers/cue-schedule-utils.tssrc/main/cue/triggers/cue-scheduled-trigger-source.tssrc/main/cue/triggers/cue-trigger-source-registry.tssrc/main/ipc/handlers/cue.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/main/cue/cue-dispatch-service.ts
- src/main/cue/pipeline-layout-store.ts
- src/main/cue/triggers/cue-trigger-source-registry.ts
- src/main/cue/cue-executor.ts
- src/main/cue/triggers/cue-heartbeat-trigger-source.ts
- src/main/cue/triggers/cue-github-poller-trigger-source.ts
- src/main/cue/triggers/cue-schedule-utils.ts
- src/main/cue/triggers/cue-scheduled-trigger-source.ts
- src/tests/main/cue/cue-engine.test.ts
- src/main/cue/cue-session-runtime-service.ts
…ce tests - cue-config-repository.test: add three security specs for writeCuePromptFile covering absolute paths, sibling-directory escapes, and traversal attempts; each asserts the expected error message and that writeFileSync is never called - cue-trigger-source-registry.test: convert repeated it blocks into two parameterized suites using it.each — POSITIVE_CASES (non-null source, correct shape) and NULL_CASES (missing required fields or runtime-handled types); import the three mocked provider factories (createCueFileWatcher, createCueTaskScanner, createCueGitHubPoller) and add factory-invocation assertions inside the positive suite by calling source.start() and checking the correct provider branch was taken; add beforeEach(vi.clearAllMocks) so call counts are isolated between parameterized runs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts (2)
111-118: Also assert non-selected factories are untouched.Current assertions confirm the expected branch was called, but not that only that branch was called. Adding negative assertions will tighten regression detection for routing bugs.
Proposed assertion hardening
const factoryMocks: Record<NonNullable<FactoryKey>, ReturnType<typeof vi.fn>> = { file: vi.mocked(createCueFileWatcher), task: vi.mocked(createCueTaskScanner), github: vi.mocked(createCueGitHubPoller), }; - if (factoryKey !== null) { - expect(factoryMocks[factoryKey]).toHaveBeenCalledOnce(); - } + for (const [key, mock] of Object.entries(factoryMocks) as Array< + [NonNullable<FactoryKey>, ReturnType<typeof vi.fn>] + >) { + if (factoryKey === key) { + expect(mock).toHaveBeenCalledOnce(); + } else { + expect(mock).not.toHaveBeenCalled(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts` around lines 111 - 118, The test currently asserts the selected factory was called but not that the others were untouched; modify the assertion block around factoryMocks/FactoryKey to also assert non-selected factories were not called: when factoryKey !== null, assert factoryMocks[factoryKey] was calledOnce and for each other key in {file, task, github} expect(factoryMocks[other]).not.toHaveBeenCalled(); when factoryKey === null assert all factoryMocks (createCueFileWatcher, createCueTaskScanner, createCueGitHubPoller) were not called. Use the existing factoryMocks and factoryKey identifiers to implement these negative assertions.
99-121: Guaranteestop()runs even when assertions fail.
start()can register timers/watchers; if an assertion fails before cleanup, teardown may be skipped and create cross-test flakiness. Wrap the running/assertion block intry/finally.Proposed fix
it.each(POSITIVE_CASES)('$event', ({ event, overrides, factoryKey }) => { const source = createTriggerSource(event, makeCtx(baseSub(event, overrides))); expect(source).not.toBeNull(); expect(typeof source!.start).toBe('function'); expect(typeof source!.stop).toBe('function'); expect(typeof source!.nextTriggerAt).toBe('function'); - // Start the source so provider factory calls are triggered, then verify - // the correct branch was taken for event types backed by a mocked provider. - source!.start(); - - const factoryMocks: Record<NonNullable<FactoryKey>, ReturnType<typeof vi.fn>> = { - file: vi.mocked(createCueFileWatcher), - task: vi.mocked(createCueTaskScanner), - github: vi.mocked(createCueGitHubPoller), - }; - if (factoryKey !== null) { - expect(factoryMocks[factoryKey]).toHaveBeenCalledOnce(); - } - - source!.stop(); + try { + // Start the source so provider factory calls are triggered, then verify + // the correct branch was taken for event types backed by a mocked provider. + source!.start(); + + const factoryMocks: Record<NonNullable<FactoryKey>, ReturnType<typeof vi.fn>> = { + file: vi.mocked(createCueFileWatcher), + task: vi.mocked(createCueTaskScanner), + github: vi.mocked(createCueGitHubPoller), + }; + if (factoryKey !== null) { + expect(factoryMocks[factoryKey]).toHaveBeenCalledOnce(); + } + } finally { + source!.stop(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts` around lines 99 - 121, The test currently calls source!.start(), runs assertions and then source!.stop(), but if any assertion fails stop() may never run; modify the it.each(POSITIVE_CASES) test body (the block that creates source via createTriggerSource and calls source!.start()) to wrap the execution and assertions in a try/finally so that source!.stop() is always invoked in the finally block; ensure the factoryMocks checks (expect(factoryMocks[factoryKey]).toHaveBeenCalledOnce()) remain inside the try so failures still trigger cleanup and keep references to createTriggerSource, source.start, and source.stop unchanged.
🤖 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/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts`:
- Around line 111-118: The test currently asserts the selected factory was
called but not that the others were untouched; modify the assertion block around
factoryMocks/FactoryKey to also assert non-selected factories were not called:
when factoryKey !== null, assert factoryMocks[factoryKey] was calledOnce and for
each other key in {file, task, github}
expect(factoryMocks[other]).not.toHaveBeenCalled(); when factoryKey === null
assert all factoryMocks (createCueFileWatcher, createCueTaskScanner,
createCueGitHubPoller) were not called. Use the existing factoryMocks and
factoryKey identifiers to implement these negative assertions.
- Around line 99-121: The test currently calls source!.start(), runs assertions
and then source!.stop(), but if any assertion fails stop() may never run; modify
the it.each(POSITIVE_CASES) test body (the block that creates source via
createTriggerSource and calls source!.start()) to wrap the execution and
assertions in a try/finally so that source!.stop() is always invoked in the
finally block; ensure the factoryMocks checks
(expect(factoryMocks[factoryKey]).toHaveBeenCalledOnce()) remain inside the try
so failures still trigger cleanup and keep references to createTriggerSource,
source.start, and source.stop unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdcf9ff1-7a86-4202-bf99-99c135e67d8e
📒 Files selected for processing (2)
src/__tests__/main/cue/cue-config-repository.test.tssrc/__tests__/main/cue/triggers/cue-trigger-source-registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/main/cue/cue-config-repository.test.ts
This was referenced Apr 10, 2026
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 closes out the Maestro Cue refactor across three sequential commits, eliminating the root causes of the recent
fix(cue):cascade and bringing the engine to a structurally sound baseline.What was wrong
The previous architecture had three concrete failure modes:
No config boundary —
prompt_file/output_prompt_fileleaked past normalization into the runtime model; the executor contained a live file-resolver that re-read.md/.txt/.yamlfiles at trigger time instead of using the already-resolved inline content; IPC handlers owned rawfs/pathI/O directly;loadCueConfig()returnednullfor both "file missing" and "file invalid" with no distinction.No session state owner —
scheduledFiredKeysandstartupFiredKeyswere two bareSetinstances shared by reference acrossCueEngine,CueSessionRuntimeService, andcue-subscription-setup.ts. Any of the three could mutate them. TheisBootScanboolean was a closure flag threaded through the enginestart()signature as a boolean, causing the recentfix(cue): harden watcher, fan-out…andfix(cue): tighten validator…cascades.No trigger source abstraction — all trigger types (heartbeat, scheduled, file watcher, task scanner, GitHub poller) were wired by
cue-subscription-setup.tsvia paralleltimers[],watchers[], andnextTriggers Maparrays onSessionState. Filter checks were duplicated across each arm of the if/else chain. Teardown had to know the internal shape of each trigger type.Commit 1 —
refactor(cue): harden config boundary and validationConfig normalizer is now authoritative.
materializeCueConfig()stripsprompt_fileandoutput_prompt_filebefore they reach the runtime model. The executor never reads files; it receives inline content or fails fast with a clear error.loadCueConfigDetailed()returns a discriminated union{ ok: true, config, warnings } | { ok: false, reason: 'missing' | 'parse-error' | 'invalid', … }. The session runtime service surfaces distinct log messages for each failure case.cue-config-repository.tsbecomes the single owner of all.maestro/cue.yamland.maestro/prompts/*.mdfilesystem I/O (writeCueConfigFile,deleteCueConfigFile,writeCuePromptFile). IPC handlers incue.tsare reduced to thin delegates.pipeline-layout-store.ts(new) ownsuserData/cue-pipeline-layout.json;app.getPath('userData')is no longer called inline in the IPC handler.prompt_file?andoutput_prompt_file?removed fromCueSubscriptionincontracts.ts.Files changed:
cue-executor.ts,cue-dispatch-service.ts,cue-yaml-loader.ts,cue-session-runtime-service.ts,config/cue-config-normalizer.ts,config/cue-config-repository.ts,ipc/handlers/cue.ts,pipeline-layout-store.ts(new),shared/cue/contracts.tsTests added/updated:
cue-config-repository.test.ts(17 new),cue-executor.test.ts,cue-yaml-loader.test.ts,cue-ipc-handlers.test.ts, 9 files updated forloadCueConfigDetailedmock shape.Commit 2 —
refactor(cue): introduce session registry and recovery serviceSession state has a single owner.
CueSessionRegistry(new) owns thesessionsmap,scheduledFiredKeys, andstartupFiredKeysbehind typed accessor methods (markScheduledFired,evictStaleScheduledKeys,markStartupFired,clearStartupForSession,clearScheduledForSession). No external code touches the underlyingSets.CueRecoveryService(new) owns DB init/prune, sleep detection, and reconciliation dispatch — previously split betweencue-heartbeat.tsandcue-engine.ts.CueEngine.start()signature changes fromstart(isBootScan: boolean)tostart(reason: SessionInitReason)whereSessionInitReason = 'system-boot' | 'user-toggle' | 'refresh' | 'discovery'.app.startupgating inCueSessionRuntimeService.initSession()checksopts.reason === 'system-boot'instead of a closure flag.cue-heartbeat.tsis trimmed to the 30-second writer loop only.src/main/index.tsupdated:cueEngine.start(true)→cueEngine.start('system-boot').Files changed:
cue-engine.ts,cue-session-runtime-service.ts,cue-heartbeat.ts,cue-session-state.ts,cue-session-registry.ts(new),cue-recovery-service.ts(new),src/main/index.tsTests added/updated:
cue-session-registry.test.ts(15 new),cue-recovery-service.test.ts(10 new),cue-session-lifecycle.test.ts(+7 init-reason matrix cases),cue-startup.test.ts,cue-concurrency.test.ts,cue-engine.test.ts.Commit 3 —
refactor(cue): isolate trigger sources behind a registryEvery trigger type is a first-class object.
src/main/cue/triggers/(new directory) contains:cue-trigger-source.ts—CueTriggerSourceinterface (start,stop,nextTriggerAt) +CueTriggerSourceContextcue-trigger-filter.ts—passesFilter(sub, event, onLog): boolean, the single filter call site; eliminates 6 duplicated filter blockscue-schedule-utils.ts—calculateNextScheduledTime+getDayName(moved fromcue-subscription-setup.ts)cue-trigger-source-registry.ts—createTriggerSource(eventType, ctx)factorycue-heartbeat-trigger-source.ts,cue-scheduled-trigger-source.ts,cue-file-watcher-trigger-source.ts,cue-task-scanner-trigger-source.ts,cue-github-poller-trigger-source.tsSessionState.timers[]+SessionState.watchers[]+SessionState.nextTriggers Mapreplaced bySessionState.triggerSources: CueTriggerSource[]. Teardown is a singlesource.stop()loop.CueSessionRuntimeServicesubscription setup loop replaces the if/else chain withcreateTriggerSource(sub.event, ctx).cue-subscription-setup.tsdeleted.Files changed:
cue-session-runtime-service.ts,cue-session-state.ts,triggers/(9 new files),cue-subscription-setup.ts(deleted)Tests added: 56 new tests across 7 files in
src/__tests__/main/cue/triggers/covering dedup semantics, filter routing, timer lifecycle, registry factory, and per-source start/stop idempotency.Test plan
npm run lint— zero type errorsnpm run lint:eslint— zero lint errorsnpm run test— full suite green (24,787 passing, up from 24,699 baseline)npm run test -- src/__tests__/main/cue/— all Cue-specific tests passSummary by CodeRabbit
New Features
Improvements
Bug Fixes