feat(cue): resolve shared-workspace conflicts via owner_agent_id#873
feat(cue): resolve shared-workspace conflicts via owner_agent_id#873pedramamini merged 6 commits intoRunMaestro:rcfrom
Conversation
…Maestro#867) When multiple agents share a projectRoot, unowned subscriptions would fire once per agent. Adds `settings.owner_agent_id` to pin unowned subs to a single agent (by id or display name). When unset, the first agent in the session list wins; non-winners — and every agent in a root whose owner_agent_id matches nobody — are flagged in the Cue dashboard with a red warning icon whose tooltip explains who took the config and how to override. Ownership logic is a pure utility with unit coverage. Closes RunMaestro#867 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a top-level Changes
Sequence Diagram(s)sequenceDiagram
participant AgentA as Agent A\n(Session A)
participant AgentB as Agent B\n(Session B)
participant Config as Config\nLoader
participant Ownership as Ownership\nResolver
participant Runtime as Runtime\n(Subscription Engine)
AgentA->>Config: load .maestro/cue.yaml (projectRoot)
AgentB->>Config: load .maestro/cue.yaml (projectRoot)
Config->>Config: validate settings.owner_agent_id (trim & non-empty)
Config-->>AgentA: config
Config-->>AgentB: config
AgentA->>Ownership: computeOwnershipWarning(session=A, candidates=[A,B], config, configFromAncestor)
AgentB->>Ownership: computeOwnershipWarning(session=B, candidates=[A,B], config, configFromAncestor)
alt owner_agent_id matches A
Ownership-->>AgentA: undefined
Ownership-->>AgentB: "ownershipWarning: not owner — unowned subs suppressed"
else owner_agent_id unset (first-wins)
Ownership-->>AgentA: undefined
Ownership-->>AgentB: "ownershipWarning: implicit non-owner — unowned subs suppressed"
else owner_agent_id matches none
Ownership-->>AgentA: "ownershipWarning: no matching owner (dead config)"
Ownership-->>AgentB: "ownershipWarning: no matching owner (dead config)"
end
AgentA->>Runtime: initSession(owner=true) — wire subscriptions
AgentB->>Runtime: initSession(owner=false) — skip unowned subs
Runtime-->>AgentA: subscriptions wired/executed
Runtime-->>AgentB: unowned subscriptions skipped, log warning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/cue/cue-session-runtime-service.ts (1)
229-283:⚠️ Potential issue | 🟠 MajorUse the ownership-filtered subscription set for downstream runtime state.
Lines 232 and 259 skip unowned subscriptions for non-owners, but Line 276 still calls
hasTimeBasedSubscriptions(config, session.id)on the full config. A non-owner with only skipped unowned timers can still callonPreventSleep, even though it created no timer source.🔧 Proposed fix
+ const runnableSubscriptions = isConfigOwner + ? config.subscriptions + : config.subscriptions.filter((sub) => Boolean(sub.agent_id)); + const runnableConfig = { ...config, subscriptions: runnableSubscriptions }; + // Wire each subscription up to its trigger source. Each source owns its // own timer/watcher/poller and emits events through the `emit` callback, // which centralizes the dispatch path: passesFilter → state.lastTriggered // → dispatchSubscription. Sources never touch session state directly. - for (const sub of config.subscriptions) { + for (const sub of runnableSubscriptions) { if (sub.enabled === false) continue; if (sub.agent_id && sub.agent_id !== session.id) continue; - if (!isConfigOwner && !sub.agent_id) continue; const source: CueTriggerSource | null = createTriggerSource(sub.event, { session, subscription: sub, @@ // app.startup subscriptions fire exactly once per process lifecycle, and // only when the engine is starting because of a real system boot. Toggling // Cue off/on or hot-reloading a YAML must NOT re-fire startup events. if (opts.reason === 'system-boot') { - for (const sub of config.subscriptions) { + for (const sub of runnableSubscriptions) { if (sub.enabled === false) continue; if (sub.agent_id && sub.agent_id !== session.id) continue; - if (!isConfigOwner && !sub.agent_id) continue; if (sub.event !== 'app.startup') continue; @@ - state.sleepPrevented = hasTimeBasedSubscriptions(config, session.id); + state.sleepPrevented = hasTimeBasedSubscriptions(runnableConfig, session.id); if (state.sleepPrevented) { deps.onPreventSleep?.(`cue:schedule:${session.id}`); } @@ 'cue', - `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(config.subscriptions, session.id, session.name)} active subscription(s)` + `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(runnableSubscriptions, session.id, session.name)} active subscription(s)` );🤖 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 229 - 283, The runtime currently checks ownership when creating trigger sources but later calls hasTimeBasedSubscriptions(config, session.id) and countActiveSubscriptions(config.subscriptions, ...) against the full subscription list, causing non-owners to wrongly appear to prevent sleep; compute a filtered subscription list (apply the same predicates: sub.enabled !== false, sub.agent_id match or unset, and if !isConfigOwner skip sub.agent_id-less entries) — use that filtered array in hasTimeBasedSubscriptions(...) and countActiveSubscriptions(...) (and any downstream logic like deps.onPreventSleep or the initialized-log) instead of the raw config.subscriptions so only actually-instantiated subscriptions affect runtime state (refer to createTriggerSource usage and state.sleepPrevented).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/cue/cue-session-state.ts`:
- Around line 130-143: The current logic uses owner_agent_id string matches
against id OR name which can allow multiple owners when names duplicate; change
the boolean ownerExists check to find the list of matchingSessions =
allSessions.filter(s => s.projectRoot === session.projectRoot && (s.id ===
explicitOwner || s.name === explicitOwner)), then handle three cases: if
matchingSessions.length === 0 return the existing "does not match any agent"
message; if matchingSessions.length > 1 return an ambiguity error indicating the
owner_agent_id matches multiple agents (so unowned subscriptions remain
disabled); if matchingSessions.length === 1 compute isOwner by checking
matchingSessions[0].id === session.id (i.e., only treat this session as owner
when the matched unique candidate's id equals this session.id) and preserve the
existing "targets ... run on that agent instead" message when not owner.
In `@src/prompts/_maestro-cue.md`:
- Line 86: Update the prompt in src/prompts/_maestro-cue.md to remove any
suggestion that unowned subscriptions can run on all agents; instead instruct
users to proactively set owner_agent_id in cue.yaml (mention using
{{MAESTRO_CLI_PATH}} list agents to find ids/names) and guide them to specify
explicit agent targets for multi-agent execution rather than implying a
shared-root "all-agent" option; ensure references to "unowned subs" are changed
to state they are pinned or disabled unless an owner_agent_id is provided.
In `@src/renderer/components/CueModal/SessionsTable.tsx`:
- Around line 75-87: The ownership warning is only placed in the title and the
aria-label is a generic "Cue ownership conflict", so screen readers don't get
the actual warning text; update the JSX where s.ownershipWarning is rendered
(the span wrapping AlertTriangle) to include the full warning in the accessible
name — e.g., set aria-label or aria-labelledby to include s.ownershipWarning (or
concatenate it with the existing label) so the AlertTriangle's accessible name
exposes the warning text to assistive tech rather than only a generic label.
In `@src/renderer/hooks/cue/useCueAiChat.ts`:
- Line 31: In useCueAiChat update the prompt text that currently marks
owner_agent_id as "required" so it instead indicates owner_agent_id is
optional/recommended and that the runtime will deterministically pick a fallback
when unset; find the prompt/message string referencing "owner_agent_id" inside
the useCueAiChat hook and change wording to something like "optional —
recommended to choose the owner; if unset, the runtime uses a deterministic
fallback" so it no longer claims the field is required.
---
Outside diff comments:
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 229-283: The runtime currently checks ownership when creating
trigger sources but later calls hasTimeBasedSubscriptions(config, session.id)
and countActiveSubscriptions(config.subscriptions, ...) against the full
subscription list, causing non-owners to wrongly appear to prevent sleep;
compute a filtered subscription list (apply the same predicates: sub.enabled !==
false, sub.agent_id match or unset, and if !isConfigOwner skip sub.agent_id-less
entries) — use that filtered array in hasTimeBasedSubscriptions(...) and
countActiveSubscriptions(...) (and any downstream logic like deps.onPreventSleep
or the initialized-log) instead of the raw config.subscriptions so only
actually-instantiated subscriptions affect runtime state (refer to
createTriggerSource usage and state.sleepPrevented).
🪄 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: 74a48f20-81cd-44bd-b9c1-a369379849bb
📒 Files selected for processing (9)
docs/maestro-cue-configuration.mdsrc/__tests__/main/cue/cue-session-state.test.tssrc/main/cue/config/cue-config-validator.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/prompts/_maestro-cue.mdsrc/renderer/components/CueModal/SessionsTable.tsxsrc/renderer/hooks/cue/useCueAiChat.tssrc/shared/cue/contracts.ts
Greptile SummaryThis PR introduces Confidence Score: 5/5Safe to merge — all P1 issues from prior review rounds are resolved; only P2 style and performance suggestions remain. Both previously raised P1 defects are addressed: config-less agent ownership race is fixed by filtering candidates through No files require special attention for merge; Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[initSession called] --> B{configFromAncestor?}
B -- yes --> C[return undefined\nancestor subs already targeted]
B -- no --> D{owner_agent_id set\nand non-empty?}
D -- yes --> E[filter sameRoot candidates\nby id OR name]
E --> F{matches.length}
F -- 0 --> G[return warning:\n'does not match any agent']
F -- gt 1 --> H[return warning:\n'ambiguous — N agents matched']
F -- 1 --> I{owner.id === session.id?}
I -- yes --> J[return undefined\nthis session is owner]
I -- no --> K[return warning:\n'owner_agent_id targets X']
D -- no --> L{sameRoot.length gt 1?}
L -- no --> M[return undefined\nsingle agent, no contest]
L -- yes --> N{firstForRoot.id === session.id?}
N -- yes --> O[return undefined\nthis session won first-in-list]
N -- no --> P[return warning:\n'X was selected as owner\nset owner_agent_id to override']
Reviews (2): Last reviewed commit: "fix(cue): tighten owner-agent semantics ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…colliding candidates Review follow-up on RunMaestro#867: - P1: computeOwnershipWarning's candidate list is now pre-filtered by the caller to sessions whose projectRoot actually has a cue.yaml. A config-less agent used to be able to win the implicit first-in-list race, making itself the "owner" with nothing to run while the real Cue-configured agent was warned off — silently stopping automation for the whole projectRoot. - P2: When owner_agent_id is set to a display name and multiple agents in the same projectRoot share that name, the first matching candidate now deterministically wins; later matches get a warning telling them to use the full id to disambiguate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for tackling this @scriptease — the matrix in the PR description is a great framing, and consolidating the ownership gate behind a single pure utility is the right call. A few concerns surfaced by the automated reviews (coderabbit / greptile) that I think are worth addressing before merge: 1. Implicit ownership can be won by a non-Cue agent ( 2. Explicit owner lookup can resolve to multiple sessions (
Recommend resolving a single owner candidate first (prefer id match, fall back to unique name match; treat ambiguous name matches as "unresolvable" with a warning), then comparing the current session against that resolved candidate's id. 3. Ownership warning not reachable by keyboard / touch / screen readers ( 4. Minor: prompt wording ( No merge conflicts — base is clean against |
…stro#873 - cue-session-state.ts: display-name collisions on owner_agent_id are now reported as ambiguous and disable unowned subscriptions for every matching agent. Previously the first match silently won, which could route unowned runs to the wrong model. Using a full id still resolves unambiguously. - SessionsTable.tsx: the red warning icon's accessible name now contains the actual ownership-warning text, not just the generic label "Cue ownership conflict". The underlying Lucide icon is marked aria-hidden so the span's role=img + aria-label is the single accessible name. - useCueAiChat.ts: the inline YAML-editor prompt no longer calls owner_agent_id "required" — it's optional-but-recommended, with the deterministic first-in-list fallback noted explicitly. - _maestro-cue.md: rewrote the "Shared Workspaces" section to remove any wording implying unowned subs broadcast to every agent in a root (never true in this implementation) and to steer authors toward explicit per-subscription agent_id for multi-agent targeting. - Tests: updated the display-name collision test to the new "ambiguous → disabled" semantics and added a case confirming that a full UUID disambiguates even when names collide. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/cue/cue-session-runtime-service.ts (1)
213-240:⚠️ Potential issue | 🟠 MajorApply ownership gating to
agent.completeddispatches too.This registers the full config for non-owner sessions and only skips trigger-source creation here.
src/main/cue/cue-completion-service.ts:47-120still iteratesdeps.getSessionConfigs()and dispatchesagent.completedsubscriptions without checkingownershipWarning, so unowned completion subscriptions can still run once per shared-root agent.Please route completion dispatch through an ownership-aware state/config iterator, or skip
state.ownershipWarning && !sub.agent_idthere before dispatching.🤖 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 213 - 240, The completion dispatch path must respect ownership gating: update the code that iterates session configs and dispatches agent.completed (currently in cue-completion-service.ts where deps.getSessionConfigs() is used) to skip subscriptions belonging to shared-root non-owner sessions by checking the session state's ownershipWarning and subscription agent_id (e.g., skip when state.ownershipWarning && !sub.agent_id), or refactor the iterator to use the ownership-aware SessionState/config instance you register (the SessionState with ownershipWarning in cue-session-runtime-service.ts) so only owner-permitted subscriptions are dispatched; ensure the same ownership check used when creating trigger sources (the loop over config.subscriptions that checks sub.agent_id and isConfigOwner) is applied before emitting agent.completed.
🧹 Nitpick comments (1)
src/main/cue/cue-session-runtime-service.ts (1)
284-291: Use the ownership-filtered subscription set for sleep prevention and active counts.For a non-owner, unowned time-based subscriptions are skipped at Lines 240 and 267, but
hasTimeBasedSubscriptions(config, session.id)andcountActiveSubscriptions(...)still see the full config. That can keep sleep prevention enabled and report active subscriptions for work this session will never run.♻️ Suggested refactor
+ const ownedOrExplicitSubscriptions = config.subscriptions.filter( + (sub) => isConfigOwner || sub.agent_id + ); + - state.sleepPrevented = hasTimeBasedSubscriptions(config, session.id); + state.sleepPrevented = hasTimeBasedSubscriptions( + { ...config, subscriptions: ownedOrExplicitSubscriptions }, + session.id + ); if (state.sleepPrevented) { deps.onPreventSleep?.(`cue:schedule:${session.id}`); } deps.onLog( 'cue', - `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(config.subscriptions, session.id, session.name)} active subscription(s)` + `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(ownedOrExplicitSubscriptions, session.id, session.name)} active subscription(s)` );🤖 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 284 - 291, The sleep-prevention and active-subscription count are using the full config instead of the ownership-filtered subscriptions; update the calls so they use the already-created ownership-filtered subscription set (the same filteredSubscriptions used earlier when skipping unowned time-based subscriptions) instead of config.subscriptions: replace hasTimeBasedSubscriptions(config, session.id) with hasTimeBasedSubscriptions(filteredSubscriptions, session.id) (or equivalent) and change countActiveSubscriptions(config.subscriptions, session.id, session.name) to countActiveSubscriptions(filteredSubscriptions, session.id, session.name), keeping state.sleepPrevented and deps.onPreventSleep logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 213-240: The completion dispatch path must respect ownership
gating: update the code that iterates session configs and dispatches
agent.completed (currently in cue-completion-service.ts where
deps.getSessionConfigs() is used) to skip subscriptions belonging to shared-root
non-owner sessions by checking the session state's ownershipWarning and
subscription agent_id (e.g., skip when state.ownershipWarning && !sub.agent_id),
or refactor the iterator to use the ownership-aware SessionState/config instance
you register (the SessionState with ownershipWarning in
cue-session-runtime-service.ts) so only owner-permitted subscriptions are
dispatched; ensure the same ownership check used when creating trigger sources
(the loop over config.subscriptions that checks sub.agent_id and isConfigOwner)
is applied before emitting agent.completed.
---
Nitpick comments:
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 284-291: The sleep-prevention and active-subscription count are
using the full config instead of the ownership-filtered subscriptions; update
the calls so they use the already-created ownership-filtered subscription set
(the same filteredSubscriptions used earlier when skipping unowned time-based
subscriptions) instead of config.subscriptions: replace
hasTimeBasedSubscriptions(config, session.id) with
hasTimeBasedSubscriptions(filteredSubscriptions, session.id) (or equivalent) and
change countActiveSubscriptions(config.subscriptions, session.id, session.name)
to countActiveSubscriptions(filteredSubscriptions, session.id, session.name),
keeping state.sleepPrevented and deps.onPreventSleep logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d000d05-51e6-4d5d-a1d2-e7f3fdc2c4ae
📒 Files selected for processing (6)
src/__tests__/main/cue/cue-session-state.test.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/prompts/_maestro-cue.mdsrc/renderer/components/CueModal/SessionsTable.tsxsrc/renderer/hooks/cue/useCueAiChat.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/renderer/hooks/cue/useCueAiChat.ts
- src/renderer/components/CueModal/SessionsTable.tsx
- src/prompts/_maestro-cue.md
- src/main/cue/cue-session-state.ts
- src/tests/main/cue/cue-session-state.test.ts
|
@greptile plz review |
Review follow-up on RunMaestro#873: - cue-session-runtime-service.ts: the implicit first-in-list candidate list now also excludes toolType === 'terminal'. A terminal agent at a shared projectRoot could previously win implicit ownership and silently block the real Cue agent. - cue-session-state.ts: when owner_agent_id is set, resolution prefers a unique id match first and only falls back to name matching when no id hit exists. A session whose display name happens to equal some other agent's id string can no longer misclaim ownership. Name-only matches remain protected against ambiguity. - SessionsTable.tsx: the ownership warning span is now keyboard-focusable (tabIndex=0) and its aria-label is exactly the warning text so the accessible name carries the actionable detail — not the prior generic "Cue ownership conflict" label. - Tests: updated ambiguity assertion to the refined "by display name" wording and added a case verifying that id match takes precedence even when another session's name equals that id string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/cue/cue-session-runtime-service.ts (1)
239-293:⚠️ Potential issue | 🟠 MajorApply the ownership gate to derived state as well.
Line 242 skips non-owner unowned subscriptions for trigger wiring, but Line 286 still prevents sleep from the full config and Lines 293 / 382-386 still count the full config. A non-owner with only skipped unowned time subscriptions can still show active subscriptions and keep the machine awake without any trigger source running.
🐛 Proposed fix
+ const ownershipEligibleSubscriptions = isConfigOwner + ? config.subscriptions + : config.subscriptions.filter((sub) => sub.agent_id); + for (const sub of config.subscriptions) { + for (const sub of ownershipEligibleSubscriptions) { if (sub.enabled === false) continue; if (sub.agent_id && sub.agent_id !== session.id) continue; - if (!isConfigOwner && !sub.agent_id) continue; const source: CueTriggerSource | null = createTriggerSource(sub.event, { session, @@ if (opts.reason === 'system-boot') { - for (const sub of config.subscriptions) { + for (const sub of ownershipEligibleSubscriptions) { if (sub.enabled === false) continue; if (sub.agent_id && sub.agent_id !== session.id) continue; - if (!isConfigOwner && !sub.agent_id) continue; if (sub.event !== 'app.startup') continue; @@ - state.sleepPrevented = hasTimeBasedSubscriptions(config, session.id); + state.sleepPrevented = hasTimeBasedSubscriptions( + { ...config, subscriptions: ownershipEligibleSubscriptions }, + session.id + ); @@ 'cue', - `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(config.subscriptions, session.id, session.name)} active subscription(s)` + `[CUE] Initialized session "${session.name}" with ${countActiveSubscriptions(ownershipEligibleSubscriptions, session.id, session.name)} active subscription(s)` );For refresh counts, apply the same ownership-aware view:
+ const countableSubscriptions = newState.ownershipWarning + ? newState.config.subscriptions.filter((sub) => sub.agent_id) + : newState.config.subscriptions; const activeCount = countActiveSubscriptions( - newState.config.subscriptions, + countableSubscriptions, sessionId, session.name );Also applies to: 382-386
🤖 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 239 - 293, The ownership gate used when wiring trigger sources (the isConfigOwner checks over config.subscriptions) must also be applied to derived state and counts: ensure hasTimeBasedSubscriptions and countActiveSubscriptions operate over the same ownership-filtered view (only include subscriptions where sub.agent_id === session.id or isConfigOwner is true), and update the code paths that set state.sleepPrevented, the final deps.onLog message, and the other occurrences you noted (the block around lines 382-386) to use that filtered list (or add a helper like getVisibleSubscriptions(session.id, isConfigOwner) and call hasTimeBasedSubscriptions/getVisibleSubscriptions/countActiveSubscriptions with it) so non-owners do not keep the machine awake or see counts for subscriptions they were correctly excluded from when wiring triggers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/cue/cue-session-runtime-service.ts`:
- Around line 239-293: The ownership gate used when wiring trigger sources (the
isConfigOwner checks over config.subscriptions) must also be applied to derived
state and counts: ensure hasTimeBasedSubscriptions and countActiveSubscriptions
operate over the same ownership-filtered view (only include subscriptions where
sub.agent_id === session.id or isConfigOwner is true), and update the code paths
that set state.sleepPrevented, the final deps.onLog message, and the other
occurrences you noted (the block around lines 382-386) to use that filtered list
(or add a helper like getVisibleSubscriptions(session.id, isConfigOwner) and
call hasTimeBasedSubscriptions/getVisibleSubscriptions/countActiveSubscriptions
with it) so non-owners do not keep the machine awake or see counts for
subscriptions they were correctly excluded from when wiring triggers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 882d8734-a8fa-400b-8770-9c1ef95e6e80
📒 Files selected for processing (4)
src/__tests__/main/cue/cue-session-state.test.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-session-state.tssrc/renderer/components/CueModal/SessionsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/CueModal/SessionsTable.tsx
- src/main/cue/cue-session-state.ts
Two follow-ups from CodeRabbit review on RunMaestro#873: - cue-session-runtime-service: filter `config.subscriptions` once into `runnableSubscriptions` (drops unowned subs for non-owners) and reuse that view for trigger wiring, app.startup, `hasTimeBasedSubscriptions`, the init log, and the refresh activeCount. Without the filter a non- owner with only unowned time triggers would still call `onPreventSleep` and report active subscriptions for work it would never run. - cue-completion-service: extend `getSessionConfigs` to expose the per- session `ownershipWarning` and skip unowned `agent.completed` subs on non-owner sessions in both `notifyAgentCompleted` and `hasCompletionSubscribers`. This closes the last duplication path in shared-projectRoot setups — without it, completion chains would still fire once per agent. Adds two regression tests in cue-completion-chains.test.ts covering the duplicate-completion case and the explicit-owner case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/cue/cue-completion-chains.test.ts`:
- Around line 1408-1449: The test currently returns the same sharedConfig for
both owner and non-owner (both at '/vault'), so the owner may also be counted;
change the fixture so only the non-owner session loads the subscription
configuration (e.g., give owner a different projectRoot like '/owner-vault' and
keep mockLoadCueConfig returning sharedConfig only for '/vault'), or
alternatively add an explicit assertion that the non-owner session alone does
not satisfy hasCompletionSubscribers by calling engine.hasCompletionSubscribers
scoped to that session; update the createMockSession/projectRoot values and
mockLoadCueConfig implementation and keep references to createMockSession,
createMockConfig, mockLoadCueConfig, createMockDeps, CueEngine, and
engine.hasCompletionSubscribers when making the change.
🪄 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: d7388c86-b278-4a27-9f0d-526ecdea02e6
📒 Files selected for processing (4)
src/__tests__/main/cue/cue-completion-chains.test.tssrc/main/cue/cue-completion-service.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-session-runtime-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/cue/cue-session-runtime-service.ts
| it('hasCompletionSubscribers returns false when only the non-owner has the unowned sub', () => { | ||
| const sessions = [ | ||
| createMockSession({ id: 'owner', name: 'Owner', projectRoot: '/vault' }), | ||
| createMockSession({ id: 'non-owner', name: 'NonOwner', projectRoot: '/vault' }), | ||
| createMockSession({ id: 'src', name: 'Source', projectRoot: '/projects/src' }), | ||
| ]; | ||
| const sharedConfig = createMockConfig({ | ||
| // Pin owner explicitly so the test doesn't depend on first-wins. | ||
| settings: { | ||
| timeout_minutes: 30, | ||
| timeout_on_fail: 'break', | ||
| max_concurrent: 1, | ||
| queue_size: 10, | ||
| owner_agent_id: 'owner', | ||
| }, | ||
| subscriptions: [ | ||
| { | ||
| name: 'on-source-done', | ||
| event: 'agent.completed', | ||
| enabled: true, | ||
| prompt: 'react', | ||
| source_session: 'Source', | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| mockLoadCueConfig.mockImplementation((projectRoot) => { | ||
| if (projectRoot === '/vault') return sharedConfig; | ||
| return null; | ||
| }); | ||
|
|
||
| const deps = createMockDeps({ getSessions: vi.fn(() => sessions) }); | ||
| const engine = new CueEngine(deps); | ||
| engine.start(); | ||
|
|
||
| // Owner has the sub wired; reporting hasCompletionSubscribers from | ||
| // the perspective of the source should remain true overall, but the | ||
| // non-owner alone must not satisfy the predicate. | ||
| expect(engine.hasCompletionSubscribers('src')).toBe(true); | ||
|
|
||
| engine.stop(); | ||
| }); |
There was a problem hiding this comment.
Strengthen the negative coverage here.
This assertion can still pass if the non-owner session is incorrectly counted, because the owner session loads the same subscription too. Split the fixture so only the non-owner can satisfy the source, or assert the non-owner contribution directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/main/cue/cue-completion-chains.test.ts` around lines 1408 -
1449, The test currently returns the same sharedConfig for both owner and
non-owner (both at '/vault'), so the owner may also be counted; change the
fixture so only the non-owner session loads the subscription configuration
(e.g., give owner a different projectRoot like '/owner-vault' and keep
mockLoadCueConfig returning sharedConfig only for '/vault'), or alternatively
add an explicit assertion that the non-owner session alone does not satisfy
hasCompletionSubscribers by calling engine.hasCompletionSubscribers scoped to
that session; update the createMockSession/projectRoot values and
mockLoadCueConfig implementation and keep references to createMockSession,
createMockConfig, mockLoadCueConfig, createMockDeps, CueEngine, and
engine.hasCompletionSubscribers when making the change.
# Conflicts: # src/__tests__/main/cue/cue-completion-chains.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/__tests__/main/cue/cue-completion-chains.test.ts (1)
1614-1655:⚠️ Potential issue | 🟡 MinorTest name and assertion are inconsistent; coverage is still weak.
The test is named "returns false when only the non-owner has the unowned sub" but the fixture wires the same subscription on the owner as well (both sessions share
/vault, owner pinned to'owner'), and the assertion istoBe(true). As written, this test would pass even if the ownership gate did nothing, because the owner alone satisfieshasCompletionSubscribers('src'). To actually exercise the negative path implied by the name, either:
- give the owner a different
projectRoot(e.g./owner-vaultwith no shared sub) so only the non-owner could satisfy the predicate, and asserttoBe(false); or- keep the current name/intent but additionally introduce a probe that scopes the predicate to the non-owner session and asserts that probe is
false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-completion-chains.test.ts` around lines 1614 - 1655, The test name and assertion mismatch: the fixture currently gives both owner and non-owner the same subscription via sharedConfig so the ownership gate isn't exercised; update the fixture so only the non-owner has the unowned subscription (e.g., change the owner session created by createMockSession to use a different projectRoot like '/owner-vault' or remove owner_agent_id pinning) and then assert expect(engine.hasCompletionSubscribers('src')).toBe(false); reference symbols: hasCompletionSubscribers, createMockSession, sharedConfig (owner_agent_id), createMockDeps, CueEngine.start/stop.
🧹 Nitpick comments (1)
src/__tests__/main/cue/cue-completion-chains.test.ts (1)
1554-1612: Pinowner_agent_idexplicitly to decouple from default-policy ordering.This case relies on the deterministic-default policy picking
'owner'purely from session array order. That makes the test fragile against legitimate changes to the fallback heuristic (e.g. switching from registration order to lexicographic id, or filtering on Cue-capable sessions per the issue's open thread). Settingsettings.owner_agent_id: 'owner'onsharedConfigkeeps this test focused on "non-owner is gated" rather than implicitly re-testing the fallback selection — and you already have separate cases covering the fallback matrix.♻️ Suggested change
const sharedConfig = createMockConfig({ + settings: { + timeout_minutes: 30, + timeout_on_fail: 'break', + max_concurrent: 1, + queue_size: 10, + owner_agent_id: 'owner', + }, subscriptions: [ { name: 'on-source-done', event: 'agent.completed', enabled: true, prompt: 'react', source_session: 'Source', }, ], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-completion-chains.test.ts` around lines 1554 - 1612, The test relies on implicit default-owner selection; explicitly set the owner by adding settings.owner_agent_id: 'owner' to the sharedConfig used in this test (the object created via createMockConfig assigned to sharedConfig) so the test no longer depends on fallback ordering; update the sharedConfig definition in the test that sets subscriptions (the "skips unowned agent.completed subs for non-owner sessions" case) to include a settings object with owner_agent_id 'owner' and keep the rest of the test logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/__tests__/main/cue/cue-completion-chains.test.ts`:
- Around line 1614-1655: The test name and assertion mismatch: the fixture
currently gives both owner and non-owner the same subscription via sharedConfig
so the ownership gate isn't exercised; update the fixture so only the non-owner
has the unowned subscription (e.g., change the owner session created by
createMockSession to use a different projectRoot like '/owner-vault' or remove
owner_agent_id pinning) and then assert
expect(engine.hasCompletionSubscribers('src')).toBe(false); reference symbols:
hasCompletionSubscribers, createMockSession, sharedConfig (owner_agent_id),
createMockDeps, CueEngine.start/stop.
---
Nitpick comments:
In `@src/__tests__/main/cue/cue-completion-chains.test.ts`:
- Around line 1554-1612: The test relies on implicit default-owner selection;
explicitly set the owner by adding settings.owner_agent_id: 'owner' to the
sharedConfig used in this test (the object created via createMockConfig assigned
to sharedConfig) so the test no longer depends on fallback ordering; update the
sharedConfig definition in the test that sets subscriptions (the "skips unowned
agent.completed subs for non-owner sessions" case) to include a settings object
with owner_agent_id 'owner' and keep the rest of the test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d10d965-eb9e-4d56-b944-018426e69d2c
📒 Files selected for processing (1)
src/__tests__/main/cue/cue-completion-chains.test.ts
|
Merged — thank you for this @scriptease! 🙏 The matrix-driven framing made the review trivial to reason about, and consolidating the gate behind a single pure utility ( I tacked on one follow-up commit covering the two outside-diff items CodeRabbit flagged on later passes — extending the ownership gate to Looking forward to your next one. |
…stry dedup RC brought two Cue-related commits since the cue-polish merge point: #849 — resolvePipelinesWriteRoots: seed lastWrittenRootsRef with per-pipeline write roots (common ancestor) rather than each agent's individual projectRoot, so delete+save clears YAML at the correct location. #873 — owner_agent_id: computeOwnershipWarning + runnableSubscriptions filter in cue-session-runtime-service resolves shared-workspace trigger duplication at init time, surfaces a dashboard warning, and respects explicit YAML config. Our cue-polish commit contained a registry-level first-registered-wins approach for the same #867 problem. That approach is now superseded: runnableSubscriptions already filters unowned subs for non-owner sessions before the trigger-source loop runs, so the claimSharedTriggerOwner check was unreachable for non-owners and redundant for the owner. Resolution: - Keep RC's ownershipWarning/runnableSubscriptions (#873) — more complete, user-visible, handles explicit owner_agent_id YAML config - Remove sharedTriggerOwners map + 3 methods from cue-session-registry.ts - Remove claimSharedTriggerOwner + releaseSharedTriggersForSession calls from cue-session-runtime-service.ts - Delete cue-shared-trigger-dedup.test.ts (tests the removed registry approach; cue-session-state.test.ts from #873 covers computeOwnershipWarning instead) - Keep all other cue-polish fixes: #866 dormant visibility, #868 Run Now dedup, #847 error-node pipeline filter, SessionsTable ownershipWarning indicator 26 371 tests passing, 0 failures.
3 participants
Summary
Fixes #867 — when two or more agents share a
projectRoot, every unowned Cue subscription (one without an explicitagent_id) currently fires once per agent. In the issue reporter's setup (an Obsidian vault registered as both an Opus and a Sonnet agent) the 18:00 summary ran twice, thetask.pendingresearch pipeline kicked off two parallel investigations, and their outputs clobbered each other in the sameResearch/<date>-<slug>/folder.This PR resolves the conflict at the engine level rather than asking YAML authors to duplicate configs or pick one agent and remember never to unattended-run the other. It introduces a single optional field,
settings.owner_agent_id, and a deterministic fallback — and surfaces the resulting state in the Cue dashboard so users can see which agent took the config and why.Why this is necessary
agent_idon a subscription is for fan-out targeting within the owner, not for "which agent owns the config" — so it couldn't be used as a workaround.Resolution matrix
The ownership check is a pure utility (⚠️ in the dashboard and use this text as the tooltip";
computeOwnershipWarning) — its return value is the single source of truth: a non-empty string means "non-owner, show the redundefinedmeans "owner, no indicator".owner_agent_idowner_agent_idas the overrideSubscriptions that carry an explicit
agent_idcontinue to fan out regardless — the gate applies only to unowned subs.Files touched
src/shared/cue/contracts.ts—CueSettings.owner_agent_id,CueSessionStatus.ownershipWarningsrc/main/cue/config/cue-config-validator.ts— validates the fieldsrc/main/cue/cue-session-state.ts— extracts the purecomputeOwnershipWarningutility + stores warning onSessionStatesrc/main/cue/cue-session-runtime-service.ts— reduces the ownership gate ininitSessionto a call to the utilitysrc/renderer/components/CueModal/SessionsTable.tsx— redAlertTriangle+ tooltip next to the session namesrc/prompts/_maestro-cue.md,src/renderer/hooks/cue/useCueAiChat.ts— teach the YAML-editor chat agent about the new fielddocs/maestro-cue-configuration.md— new "Sharing a workspace" sectionTest plan
npm run lint— cleannpx vitest run src/__tests__/main/cue/— 984 tests pass (10 new cases incue-session-state.test.tscovering every row of the matrix above, including ancestor bypass and whitespace-onlyowner_agent_id)validate:pushsuite via pre-push hook — 25,687 tests passowner_agent_idto the other agent flips which row fires🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests