Skip to content

feat(cli): add dispatch command, deprecate send --live (PR1)#907

Merged
pedramamini merged 3 commits intoRunMaestro:rcfrom
chr1syy:feat/cli-dispatch
Apr 27, 2026
Merged

feat(cli): add dispatch command, deprecate send --live (PR1)#907
pedramamini merged 3 commits intoRunMaestro:rcfrom
chr1syy:feat/cli-dispatch

Conversation

@chr1syy
Copy link
Copy Markdown
Contributor

@chr1syy chr1syy commented Apr 26, 2026

Summary

PR1 of the Maestro CLI surface refactor. Splits the desktop-handoff half of send --live into a dedicated maestro-cli dispatch verb and surfaces tab/session IDs in the send_command and new_ai_tab_with_prompt WebSocket responses, so external consumers (Maestro-Discord, Cue) can chain dispatches without owning a persistent channel.

Today's send --live is two unrelated things — local execution and desktop handoff — wedged into one command. We're splitting them: send stays local-and-synchronous, dispatch becomes the dedicated desktop handoff and returns a session ID so callers can address the same tab again.

No new architecture, no back-channel — pure rename + ID surfacing.

What changed

  • New command: maestro-cli dispatch <agent> <prompt> with --new-tab, --session <tabId>, --force. Output:
    { "success": true, "agentId": "...", "sessionId": "tab-xyz", "tabId": "tab-xyz" }
  • send_command now accepts an optional tabId and echoes the resolved tab id (caller-supplied tabId, or the session's activeTabId from getSessionDetail) in command_result.
  • new_ai_tab_with_prompt returns the freshly-created tabId via the renderer ack — NewAITabWithPromptCallback now returns { success, tabId? }, threaded through preload (sendRemoteNewAITabWithPromptResponse) + the IPC bridge in web-server-factory.
  • Renderer (useRemoteHandlers) honors an explicit tabId from maestro:remoteCommand so a dispatch writes into the requested tab even if the user switches active tabs mid-flight (falls back to active when unset/unknown).
  • send --live stays as a hidden alias that delegates to runDispatch and prints a one-line stderr deprecation notice. Stdout shape (agentName: "live", null sessionId/response/usage) is preserved so existing scripts keep parsing.
  • Cue's CLI executor (cue-cli-executor.ts) now spawns maestro-cli dispatch <target> <message> instead of send <target> <message> --live. The underlying WebSocket message is unchanged, so behavior is identical.

Files touched

  • New: src/cli/commands/dispatch.ts, src/__tests__/cli/commands/dispatch.test.ts
  • CLI: src/cli/commands/send.ts, src/cli/index.ts
  • Web server: handlers/messageHandlers.ts, managers/CallbackRegistry.ts, WebServer.ts, web-server-factory.ts, types.ts
  • Preload + renderer: main/preload/process.ts, renderer/global.d.ts, renderer/hooks/remote/useRemoteIntegration.ts, renderer/hooks/remote/useRemoteHandlers.ts
  • Cue: src/main/cue/cue-cli-executor.ts
  • Updated tests across messageHandlers, CallbackRegistry, web-server-factory, preload/process, useRemoteIntegration, cue-cli-executor, and send test suites

Test plan

  • npm run lint (tsc) clean
  • npm run lint:eslint clean
  • Prettier clean on all touched files
  • 372 in-scope tests pass: dispatch.test.ts (13 new cases), send.test.ts, messageHandlers.test.ts, CallbackRegistry.test.ts, web-server-factory.test.ts, preload/process.test.ts, useRemoteIntegration.test.ts, useRemoteHandlers.test.ts, cue-cli-executor.test.ts
  • Manual: maestro-cli dispatch <agent> "hello" --new-tab → returns tabId
  • Manual: chain dispatch ... --session <tabId> → writes to the correct tab even after a tab switch
  • Manual: maestro-cli send <agent> "hello" --live → still works, prints deprecation notice to stderr
  • Manual: Cue subscription with command.mode: cli → fires through dispatch

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Adds a new maestro-cli "dispatch" command that returns structured sessionId/tabId and supports explicit tab targeting, creating a new tab, and a force option (force gated by settings).
  • Deprecations

    • send --live is deprecated, emits a deprecation notice and preserves legacy response shape for compatibility.
  • Behavior

    • IPC/remote flows now propagate tabId and force; tabId is only included when explicitly provided or created; force can bypass busy guards when allowed.
  • Tests

    • Expanded test coverage for dispatch, send, and remote/IPC behaviors.

Splits the desktop-handoff half of `send --live` into a dedicated
`maestro-cli dispatch` verb and surfaces tab/session IDs in the
`send_command` and `new_ai_tab_with_prompt` WebSocket responses so
external consumers (Maestro-Discord, Cue) can chain dispatches without
owning a persistent channel.

- New `dispatch <agent> <prompt>` command with `--new-tab`,
  `--session <tabId>`, and `--force`. Output:
  `{ success, agentId, sessionId, tabId }` where sessionId === tabId.
- `send_command` now accepts an optional `tabId` and returns the tab
  it wrote to (resolved from caller-supplied tabId or session's
  activeTabId).
- `new_ai_tab_with_prompt` returns the freshly-created `tabId` via
  the renderer ack, threaded through preload + IPC bridge.
- Renderer `useRemoteHandlers` honors an explicit tabId so the dispatch
  writes into the requested tab even if the user switches active tabs
  mid-flight.
- `send --live` is retained as a hidden alias that delegates to
  `runDispatch` and prints a stderr deprecation notice. Stdout shape
  is preserved so existing scripts keep parsing.
- Cue's CLI executor now spawns `maestro-cli dispatch <target>
  <message>` instead of `send <target> <message> --live`.

No new architecture, no back-channel — pure rename + ID surfacing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

📝 Walkthrough

Walkthrough

Adds a new maestro-cli dispatch command and threads optional tabId and force through CLI, WebServer handlers, CallbackRegistry, preload IPC, and renderer code; deprecates send --live and delegates it to the new dispatch flow. Tests updated to cover propagation and error mappings.

Changes

Cohort / File(s) Summary
New Dispatch CLI
src/cli/commands/dispatch.ts, src/cli/index.ts
Adds dispatch <agent-id> <message> with --new-tab, --session, --force; validates options, resolves agent id, sends new_ai_tab_with_prompt or send_command via Maestro, normalizes sessionId/tabId, and maps errors to standardized codes.
Send CLI & Backcompat
src/cli/commands/send.ts
Marks --live deprecated, delegates --live flows to runDispatch, removes prior force gating logic, and preserves legacy SendResponse JSON shape for compatibility.
WebServer core & handlers
src/main/web-server/handlers/messageHandlers.ts, src/main/web-server/types.ts, src/main/web-server/WebServer.ts, src/main/web-server/web-server-factory.ts
Extends executeCommand callback to accept tabId?: string and force?: boolean; changes newAITabWithPrompt to return { success: boolean; tabId?: string }; forwards tabId/force in IPC and handler responses; adds activeTabId?: string support.
CallbackRegistry
src/main/web-server/managers/CallbackRegistry.ts
Updates executeCommand and newAITabWithPrompt signatures to accept/return tab-aware parameters and results; returns { success: false } when unregistered.
Preload / IPC surface
src/main/preload/process.ts, src/renderer/global.d.ts
Extends onRemoteCommand callback signature to include tabId?: string and force?: boolean; sendRemoteNewAITabWithPromptResponse now accepts optional tabId and sends { success, tabId }.
Renderer remote integration & handlers
src/renderer/hooks/remote/useRemoteIntegration.ts, src/renderer/hooks/remote/useRemoteHandlers.ts
Threads tabId and force into dispatched maestro:remoteCommand events; resolves pinned tab targets, allows force to bypass busy guards, forwards logs to target tab, and surfaces created tabId in acknowledgements.
Cue CLI executor & tests
src/main/cue/cue-cli-executor.ts, src/__tests__/main/cue/cue-cli-executor.test.ts
Changes spawned CLI argv and logging to use dispatch instead of send --live; updates tests accordingly.
Tests: CLI, preload, web-server, renderer
src/__tests__/cli/commands/dispatch.test.ts, src/__tests__/cli/commands/send.test.ts, src/__tests__/main/preload/process.test.ts, src/__tests__/main/web-server/handlers/messageHandlers.test.ts, src/__tests__/main/web-server/managers/CallbackRegistry.test.ts, src/__tests__/main/web-server/web-server-factory.test.ts, src/__tests__/renderer/hooks/useRemoteIntegration.test.ts
Adds comprehensive tests for dispatch behavior, option validation, error code mapping, tabId/force propagation, new-tab responses with tabId, --live deprecation, and extended IPC callback compatibility.
Other small test updates
src/__tests__/cli/commands/send.test.ts, src/__tests__/main/preload/process.test.ts
Augmented assertions for --live deprecation notice and backward compatibility for extended IPC callback args.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as "maestro-cli dispatch"
    participant WS as "WebServer / WS Handler"
    participant CB as "CallbackRegistry"
    participant IPC as "Preload / IPC"
    participant Renderer as "Renderer"

    User->>CLI: dispatch <agentId> <message> [--new-tab | --session | --force]
    CLI->>WS: send_command OR new_ai_tab_with_prompt (includes tabId?, force?)
    WS->>CB: executeCommand(sessionId, command, inputMode, tabId?, force?)
    CB->>IPC: remote:executeCommand (sessionId, command, inputMode, tabId?, force?)
    IPC->>Renderer: onRemoteCommand(sessionId, command, inputMode, tabId, force)
    Renderer->>Renderer: resolve target tab or create new tab
    alt new tab created
        Renderer->>IPC: sendRemoteNewAITabWithPromptResponse(channel, true, newTabId)
        IPC->>CB: returns { success: true, tabId: newTabId }
    else command executed / ack
        Renderer->>IPC: acknowledge (boolean) or { success, tabId? }
        IPC->>CB: returns { success: true/false, tabId? }
    end
    CB->>WS: command_result / new_ai_tab_with_prompt_result (include tabId when provided)
    WS->>CLI: DispatchResponse { agentId, sessionId, tabId? } or error code
    CLI->>User: prints JSON success or structured error and exits on failure
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

ready to merge

Poem

🐰 I hopped from code to comfy patch,
Threaded tab IDs into each dispatch,
Renderer, IPC, CLI in sync,
New tabs get IDs in just a blink,
Hooray — the rabbit hums, "Deploy with a nibble!" 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding a new dispatch command and deprecating the --live flag of the send command.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 26, 2026

Greptile Summary

This PR introduces maestro-cli dispatch as a dedicated desktop-handoff verb, splitting it out of send --live, and surfaces tab/session IDs through the WebSocket and IPC layers so callers can chain follow-up dispatches to the same tab. The implementation is well-structured — backward compatibility for send --live is preserved with a stderr deprecation notice, Cue's executor is updated cleanly, and the newAITabWithPrompt IPC protocol handles both new and old renderer formats.

  • P1 — silent wrong-tab delivery: In useRemoteHandlers.ts, when --session <tabId> names a tab that no longer exists, the renderer silently falls back to the active tab without logging a warning or returning an error; the command_result already echoed the requested tabId as authoritative, so callers believing they're chaining to the same conversation are misled.
  • P2 — command_result tabId may not match actual execution tab: For the default (no --session) path, the server echoes sessionDetail.activeTabId in command_result but passes undefined to the renderer, which independently resolves its own active tab; under a concurrent tab switch these can diverge, giving callers a stale tab ID to chain against.

Confidence Score: 3/5

Mergeable with caution — the core rename is sound but the silent wrong-tab fallback in the renderer can mislead consumers relying on the returned tabId for chaining.

One P1 finding (silent fallback to active tab when a requested tab is not found, while command_result already reported the requested tabId) and one P2 (server-resolved activeTabId in command_result can diverge from the renderer's actual target). The P1 directly undermines the core value proposition of dispatch --session <tabId> chaining.

src/renderer/hooks/remote/useRemoteHandlers.ts (silent tab fallback) and src/main/web-server/handlers/messageHandlers.ts (resolvedTabId computation)

Important Files Changed

Filename Overview
src/cli/commands/dispatch.ts New dedicated dispatch verb; well-structured with error mapping and fallback logic, but --new-tab on older desktop yields an ambiguous null tabId
src/cli/commands/send.ts --live cleanly delegates to runDispatch with preserved legacy response shape; deprecation notice correctly written to stderr
src/main/web-server/handlers/messageHandlers.ts Threads tabId through send_command handling; command_result echoes server-resolved activeTabId rather than the tab actually targeted by the renderer, creating a potential divergence for callers chaining dispatches
src/renderer/hooks/remote/useRemoteHandlers.ts Correctly resolves target tab from requestedTabId; silently falls back to active tab when the requested tab no longer exists without surfacing an error to the caller
src/renderer/hooks/remote/useRemoteIntegration.ts Captures createdTabId from flushSync and threads it through both the maestro:remoteCommand event and the IPC ack; backward-compatible failure paths unchanged
src/main/web-server/web-server-factory.ts Good backward-compat handling for newAITabWithPrompt IPC response (object vs bare boolean); tabId forwarded correctly through remote:executeCommand IPC
src/main/preload/process.ts sendRemoteNewAITabWithPromptResponse now sends { success, tabId } object; onRemoteCommand correctly threads tabId through to the callback
src/main/web-server/types.ts Clean type updates: ExecuteCommandCallback gains optional tabId, NewAITabWithPromptCallback returns NewAITabWithPromptResult instead of boolean
src/main/cue/cue-cli-executor.ts Migrated from send <target> <message> --live to dispatch <target> <message>; log messages updated consistently
src/cli/index.ts New dispatch command registered with appropriate options; send --live description updated to note deprecation

Sequence Diagram

sequenceDiagram
    participant CLI as maestro-cli dispatch
    participant WS as WebServer (WebSocketMessageHandler)
    participant CB as CallbackRegistry
    participant IPCMain as web-server-factory (IPC Main)
    participant Preload as preload/process
    participant Renderer as useRemoteIntegration / useRemoteHandlers

    CLI->>WS: send_command { sessionId, command, tabId? }
    WS->>WS: resolvedTabId = requestedTabId ?? sessionDetail.activeTabId
    WS->>CB: executeCommand(sessionId, command, inputMode, requestedTabId)
    CB->>IPCMain: executeCommand callback
    IPCMain->>Renderer: IPC remote:executeCommand(sessionId, command, inputMode, tabId?)
    Renderer->>Renderer: find tab by requestedTabId (fallback: getActiveTab)
    Renderer->>Renderer: spawn agent into targetTab
    WS-->>CLI: command_result { success, tabId: resolvedTabId }

    Note over CLI,WS: --new-tab path
    CLI->>WS: new_ai_tab_with_prompt { sessionId, prompt }
    WS->>CB: newAITabWithPrompt(sessionId, prompt)
    CB->>IPCMain: newAITabWithPrompt callback
    IPCMain->>Renderer: IPC remote:newAITabWithPrompt(sessionId, prompt, responseChannel)
    Renderer->>Renderer: flushSync createTab → createdTabId
    Renderer->>Renderer: dispatchEvent maestro:remoteCommand { tabId: createdTabId }
    Renderer->>Preload: sendRemoteNewAITabWithPromptResponse(channel, true, createdTabId)
    Preload->>IPCMain: IPC responseChannel { success: true, tabId }
    IPCMain-->>WS: resolve { success, tabId }
    WS-->>CLI: new_ai_tab_with_prompt_result { success, tabId }
Loading

Comments Outside Diff (1)

  1. src/cli/commands/dispatch.ts, line 787-794 (link)

    P2 --new-tab returns null tabId on older desktop builds

    When options.newTab is true and an older desktop build returns new_ai_tab_with_prompt_result without a tabId field, result.tabId is undefined, and the fallback chain tabId ?? options.session ?? null yields null (since options.session is mutually exclusive with --new-tab). The caller receives { success: true, tabId: null }, which is indistinguishable from a genuine "no active tab" failure. Adding a distinct code path or error field for this case would give callers a way to detect the degraded mode vs. a true no-session state.

Reviews (1): Last reviewed commit: "feat(cli): add `dispatch` command, depre..." | Re-trigger Greptile

Comment thread src/renderer/hooks/remote/useRemoteHandlers.ts Outdated
Comment thread src/main/web-server/handlers/messageHandlers.ts Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/renderer/hooks/remote/useRemoteHandlers.ts (1)

403-427: ⚠️ Potential issue | 🟠 Major

Abort the spawn when the target tab disappears.

If resolvedWriteTabId is gone, this branch only skips the optimistic log update. The code still continues to window.maestro.process.spawn(...) with a targetSessionId derived from a missing tab, so the agent can start with nowhere valid to route output back to.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/remote/useRemoteHandlers.ts` around lines 403 - 427, The
optimistic-update branch for setSessions detects a missing resolvedWriteTabId
but only skips the log update; you must abort the subsequent spawn so the agent
doesn't start with no valid target. Change the logic around
setSessions/runAICommand so that if !s.aiTabs?.some(t => t.id ===
resolvedWriteTabId) you log the error and immediately stop the operation (return
from the outer async handler or reject/throw) before calling
window.maestro.process.spawn (the spawn call that uses
targetSessionId/targetTabId); ensure no further code runs for that session when
resolvedWriteTabId is not found.
🤖 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/cli/commands/dispatch.ts`:
- Around line 107-139: The catch block in dispatch.ts currently maps
network/WebSocket errors to MAESTRO_NOT_RUNNING but misses
MaestroClient-specific messages; update the error pattern matching inside the
catch (error) block to also check for the exact strings "Maestro desktop app is
not running", "Maestro discovery file is stale (app may have crashed)" and "Not
connected to Maestro" (in addition to the existing substrings) so those errors
return { success: false, error: 'Maestro desktop is not running', code:
'MAESTRO_NOT_RUNNING' } and add unit tests that mock thrown errors with each of
these exact messages to prevent regression; reference the catch handling logic
in dispatch.ts (the try/catch around the dispatch call) and the error branches
that return MAESTRO_NOT_RUNNING / SESSION_NOT_FOUND / COMMAND_FAILED.

In `@src/cli/index.ts`:
- Around line 143-150: The '--live' option is still shown in help; replace the
current .option('-l, --live', 'Send message through Maestro desktop (deprecated:
use `dispatch`)') with a hidden option using commander.Option: import Option
from commander (if not already), then use .addOption(new Option('-l, --live',
'Send message through Maestro desktop (deprecated: use `dispatch`)').hideHelp())
inside the program.command('send'...) chain so the flag remains supported but is
omitted from help output.

In `@src/main/web-server/managers/CallbackRegistry.ts`:
- Around line 231-238: The executeCommand signature and callback contract lack
the new force boolean, so add a force?: boolean parameter to the executeCommand
method and forward it to this.callbacks.executeCommand(sessionId, command,
inputMode, tabId, force); update the callbacks type/interface entry named
executeCommand to accept (sessionId: string, command: string, inputMode?:
'ai'|'terminal', tabId?: string, force?: boolean) => Promise<boolean> (or
boolean) and ensure all implementations/call sites of executeCommand are updated
to accept/forward the force argument so the flag survives the WebSocket
boundary.

In `@src/main/web-server/web-server-factory.ts`:
- Around line 382-384: The current logger.info call in web-server-factory (the
logger.info invocation that builds a message using sessionId, agentSessionId,
inputMode, tabId and command.substring(0, 100)) is leaking raw command text into
info logs; change it to only log metadata (sessionId, agentSessionId, inputMode,
tabId) at info level and remove the raw command text, and if you need a short
preview keep it out of info by emitting a redacted preview or full preview at
debug level (e.g., replace command.substring(0,100) with a fixed "<redacted>" in
the logger.info message and, if desired, send the truncated preview to
logger.debug using the same session/agent metadata).

In `@src/renderer/hooks/remote/useRemoteHandlers.ts`:
- Around line 340-347: Resolve the target tab (compute requestedTab via
requestedTabId and fallback to getActiveTab, and set writeTabId) before
validating slash-commands so error logs are written to the intended tab; update
any call sites in the unknown-slash-command branch that currently call
addLogToActiveTab(sessionId, ...) to instead use the resolved write tab (e.g.,
call addLogToTab(sessionId, writeTabId, ...) or the equivalent function that
writes to a specific tab), and ensure requestedTab/requestedTabId/getActiveTab
are used to determine the correct target prior to emitting the error log.

---

Outside diff comments:
In `@src/renderer/hooks/remote/useRemoteHandlers.ts`:
- Around line 403-427: The optimistic-update branch for setSessions detects a
missing resolvedWriteTabId but only skips the log update; you must abort the
subsequent spawn so the agent doesn't start with no valid target. Change the
logic around setSessions/runAICommand so that if !s.aiTabs?.some(t => t.id ===
resolvedWriteTabId) you log the error and immediately stop the operation (return
from the outer async handler or reject/throw) before calling
window.maestro.process.spawn (the spawn call that uses
targetSessionId/targetTabId); ensure no further code runs for that session when
resolvedWriteTabId is not found.
🪄 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: 43ac2d8b-42f1-4d93-9503-570940a9d574

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc4cf7 and dfdb772.

📒 Files selected for processing (21)
  • src/__tests__/cli/commands/dispatch.test.ts
  • src/__tests__/cli/commands/send.test.ts
  • src/__tests__/main/cue/cue-cli-executor.test.ts
  • src/__tests__/main/preload/process.test.ts
  • src/__tests__/main/web-server/handlers/messageHandlers.test.ts
  • src/__tests__/main/web-server/managers/CallbackRegistry.test.ts
  • src/__tests__/main/web-server/web-server-factory.test.ts
  • src/__tests__/renderer/hooks/useRemoteIntegration.test.ts
  • src/cli/commands/dispatch.ts
  • src/cli/commands/send.ts
  • src/cli/index.ts
  • src/main/cue/cue-cli-executor.ts
  • src/main/preload/process.ts
  • src/main/web-server/WebServer.ts
  • src/main/web-server/handlers/messageHandlers.ts
  • src/main/web-server/managers/CallbackRegistry.ts
  • src/main/web-server/types.ts
  • src/main/web-server/web-server-factory.ts
  • src/renderer/global.d.ts
  • src/renderer/hooks/remote/useRemoteHandlers.ts
  • src/renderer/hooks/remote/useRemoteIntegration.ts

Comment thread src/cli/commands/dispatch.ts
Comment thread src/cli/index.ts Outdated
Comment thread src/main/web-server/managers/CallbackRegistry.ts Outdated
Comment thread src/main/web-server/web-server-factory.ts
Comment thread src/renderer/hooks/remote/useRemoteHandlers.ts Outdated
Review comments resolved:
- P1 (greptile): drop dispatch on unknown --session tabId instead of silently
  re-routing to active tab — chained `dispatch --session <returnedId>` callers
  no longer write into the wrong conversation.
- P2 (greptile): stop echoing the server's `activeTabId` snapshot in
  command_result; only echo caller-supplied authoritative tabIds. Default
  no-tabId path returns no tabId so callers do not chain on stale data.
- Major (coderabbit): map MaestroClient pre-WebSocket throw strings ("not
  running", "discovery file is stale", "not connected") to
  MAESTRO_NOT_RUNNING so the error-code contract holds for downstream
  consumers. Tests cover all three strings.
- Major (coderabbit): thread `force?: boolean` through the executeCommand
  callback chain — types, CallbackRegistry, WebServer, web-server-factory,
  preload, global.d.ts, useRemoteIntegration — so `dispatch --force`
  survives the WebSocket boundary and bypasses the renderer busy guard.
- Major (coderabbit): drop raw command text from the [Web → Renderer] info
  log; metadata + length only at info, redacted preview moves to debug.
- Major (coderabbit, outside-diff): re-check the resolved tab still exists
  after the agent-config await microtask gap and abort the spawn rather
  than starting an agent on a route nothing reads from.
- Minor (coderabbit): resolve target tab before slash-command validation
  so unknown-command error logs land on the dispatched-into tab; switch
  the substituted `activeTabId` template var to the resolved write tab.
- Minor (coderabbit): hide `--live`, `--new-tab`, `--force` from
  `send --help` via Option.hideHelp() — they remain parsable for legacy
  scripts but no longer steer new callers off `dispatch`.

Tests updated/added:
- dispatch.test.ts: it.each over the three MaestroClient error strings.
- messageHandlers.test.ts: command_result no longer includes activeTabId
  on the no-tabId path; force flag passes through.
- CallbackRegistry/web-server-factory/preload/process tests: force=true
  arg propagation; web-server-factory log redaction asserted.
- useRemoteIntegration.test.ts: force=true bypasses the busy guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/renderer/hooks/remote/useRemoteIntegration.ts (1)

109-119: ⚠️ Potential issue | 🟠 Major

Make the pre-dispatch busy check tab-aware.

With an explicit tabId, this guard still gates on targetSession.state, so a valid dispatch --session <tabId> can be dropped before the downstream tab-aware handler resolves the addressed tab. Either remove this duplicate guard or resolve the target tab first and check that tab’s state instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/remote/useRemoteIntegration.ts` around lines 109 - 119,
The busy-check currently inspects targetSession.state before resolving a
tab-specific target, which can drop valid "dispatch --session <tabId>" commands;
update the guard in useRemoteIntegration so that when an explicit tabId is
provided you first resolve the addressed tab/session (e.g., via the existing
tab-resolution helper used later) and then check that resolved session's state,
or simply remove this pre-dispatch guard and rely on the downstream tab-aware
handler; ensure you still respect the force flag and reference targetSession,
force, and the tab-resolution helper/function in your change.
src/renderer/hooks/remote/useRemoteHandlers.ts (1)

525-560: ⚠️ Potential issue | 🟡 Minor

Error-path may leave session stuck in 'busy' if target tab disappeared mid-flight.

When the spawn throws AFTER the success-path setSessions (Lines 447–488) successfully marked the session busy and the target tab as busy, the error path at Lines 545–550 returns s unchanged if the tab no longer exists by then. The session-level state: 'idle' reset on Lines 552–558 is in the other branch and never runs in this race.

Result: a stuck busy session with no log indicating an error. Consider always resetting the session-level state to 'idle' even when the per-tab log can't be written.

♻️ Suggested fix
-					if (!s.aiTabs?.some((t) => t.id === resolvedWriteTabId)) {
-						logger.error(
-							'[runAICommand error] Target tab not found in session — dropping error log'
-						);
-						return s;
-					}
-
-					return {
+					if (!s.aiTabs?.some((t) => t.id === resolvedWriteTabId)) {
+						logger.error(
+							'[runAICommand error] Target tab not found in session — dropping error log'
+						);
+						// Still clear session-level busy so the session isn't stuck.
+						return {
+							...s,
+							state: 'idle' as SessionState,
+							busySource: undefined,
+							thinkingStartTime: undefined,
+						};
+					}
+
+					return {
 						...s,
 						state: 'idle' as SessionState,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/remote/useRemoteHandlers.ts` around lines 525 - 560, The
error-path can leave a session stuck in 'busy' when the target tab disappears
because the code returns the unchanged session when !s.aiTabs?.some(...); change
that branch to still reset the session-level fields (state -> 'idle', busySource
-> undefined, thinkingStartTime -> undefined) and preserve aiTabs unchanged, and
still call logger.error; update the setSessions mapping inside useRemoteHandlers
(referencing setSessions, sessionId, writeTabId, s.aiTabs, logger.error,
SessionState) so the function returns a session object with those session-level
resets even when the target tab is missing.
🧹 Nitpick comments (2)
src/cli/commands/dispatch.ts (1)

152-177: Unknown errors map to COMMAND_FAILED and exit 1; consider Sentry capture for unexpected paths.

As per coding guidelines (src/**/*.{ts,tsx}: For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context.), it's worth considering whether unmatched errors in runDispatch (the COMMAND_FAILED branch) should be reported via captureException with operation context before the JSON error response is emitted. This would help diagnose unexpected failures in the dispatch CLI without relying on user-pasted output.

Optional — CLI tools sometimes opt out of Sentry; if that's the deliberate stance for this binary, ignore this.

As per coding guidelines.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/commands/dispatch.ts` around lines 152 - 177, In dispatch, handle
unexpected failures from runDispatch by reporting them to Sentry before exiting:
import captureException/captureMessage from src/utils/sentry.ts and when
result.success is false and result.code is missing or equals 'COMMAND_FAILED'
(or otherwise an unexpected path), call captureException(result.error ?? new
Error('Unknown dispatch error')) with context (operation: 'dispatch', agentId:
agentIdArg, message) or captureMessage for string-only cases, then call
emitErrorJson(result.error ?? 'Unknown error', result.code ?? 'COMMAND_FAILED')
and process.exit(1); leave existing behavior for known error codes unchanged.
src/main/web-server/handlers/messageHandlers.ts (1)

94-97: activeTabId is intentionally unused in the send_command handler. The interface includes this field for getSessionDetail consumers, but the handler deliberately avoids echoing sessionDetail.activeTabId in responses. The comment at lines 580–583 explains the rationale: echoing the server's snapshot of activeTabId would be incorrect if the user switches tabs between the caller's send and receive, leading CLI tools to chain calls to the wrong tab. Consider adding a brief inline comment (e.g. // activeTabId intentionally not echoed; see rationale above) near where sessionDetail is used, to clarify for future readers why the field is present but not consumed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/web-server/handlers/messageHandlers.ts` around lines 94 - 97, Add a
brief inline comment where the send_command handler references sessionDetail
explaining that sessionDetail.activeTabId is intentionally not echoed; e.g.,
note "activeTabId intentionally not echoed; see rationale above" near the use of
sessionDetail in the send_command handler so future readers understand why the
field exists but is not consumed (reference: activeTabId, send_command handler,
sessionDetail).
🤖 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/cli/commands/dispatch.ts`:
- Around line 75-95: The new-tab branch inside withMaestroClient currently
returns result.tabId without validation; change the logic in the branch that
calls sendCommand with type 'new_ai_tab_with_prompt' to explicitly check that
result.tabId is a non-empty string and, if missing, throw a new error tagged
with a dedicated code (e.g., NEW_TAB_NO_ID). Update the outer catch/response
mapping that currently maps failures to COMMAND_FAILED to detect this
NEW_TAB_NO_ID error code and return the specialized error response so downstream
consumers can fail fast; reference the withMaestroClient call, the sendCommand
invocation for 'new_ai_tab_with_prompt', and the catch mapping that converts
errors to response codes.

In `@src/cli/index.ts`:
- Around line 175-188: The CLI currently advertises the combination dispatch
--new-tab --force but the dispatch handler (dispatch in
src/cli/commands/dispatch.ts) always runs the options.newTab branch before
honoring options.force, so either forbid the combo or propagate force into the
new_tab code path; update the CLI command definition or the dispatch
implementation to make behavior consistent: either validate options in the
dispatch action and throw an error when options.newTab && options.force
(rejecting the combination), or modify the new_ai_tab_with_prompt call path
inside dispatch to accept and forward the options.force flag (and any
allowConcurrentSend checks) so new-tab dispatches respect --force when provided.

In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 81-94: The info log in the onRemoteCommand callback (inside
useRemoteIntegration) currently includes command.substring(0,50) and must be
changed to avoid logging prompt content: update the logger.info call to only
include safe metadata (e.g., sessionId, inputMode, tabId, force, and
commandLength = command?.length) and move the 0-50 preview into a logger.debug
call (e.g., logger.debug('[useRemoteIntegration] command preview', { sessionId,
preview: command?.substring(0,50) })) so previews are only emitted at debug
level; apply the same change to the other logger.info usage around the later
block (the second occurrence referenced in the review).

---

Outside diff comments:
In `@src/renderer/hooks/remote/useRemoteHandlers.ts`:
- Around line 525-560: The error-path can leave a session stuck in 'busy' when
the target tab disappears because the code returns the unchanged session when
!s.aiTabs?.some(...); change that branch to still reset the session-level fields
(state -> 'idle', busySource -> undefined, thinkingStartTime -> undefined) and
preserve aiTabs unchanged, and still call logger.error; update the setSessions
mapping inside useRemoteHandlers (referencing setSessions, sessionId,
writeTabId, s.aiTabs, logger.error, SessionState) so the function returns a
session object with those session-level resets even when the target tab is
missing.

In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 109-119: The busy-check currently inspects targetSession.state
before resolving a tab-specific target, which can drop valid "dispatch --session
<tabId>" commands; update the guard in useRemoteIntegration so that when an
explicit tabId is provided you first resolve the addressed tab/session (e.g.,
via the existing tab-resolution helper used later) and then check that resolved
session's state, or simply remove this pre-dispatch guard and rely on the
downstream tab-aware handler; ensure you still respect the force flag and
reference targetSession, force, and the tab-resolution helper/function in your
change.

---

Nitpick comments:
In `@src/cli/commands/dispatch.ts`:
- Around line 152-177: In dispatch, handle unexpected failures from runDispatch
by reporting them to Sentry before exiting: import
captureException/captureMessage from src/utils/sentry.ts and when result.success
is false and result.code is missing or equals 'COMMAND_FAILED' (or otherwise an
unexpected path), call captureException(result.error ?? new Error('Unknown
dispatch error')) with context (operation: 'dispatch', agentId: agentIdArg,
message) or captureMessage for string-only cases, then call
emitErrorJson(result.error ?? 'Unknown error', result.code ?? 'COMMAND_FAILED')
and process.exit(1); leave existing behavior for known error codes unchanged.

In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 94-97: Add a brief inline comment where the send_command handler
references sessionDetail explaining that sessionDetail.activeTabId is
intentionally not echoed; e.g., note "activeTabId intentionally not echoed; see
rationale above" near the use of sessionDetail in the send_command handler so
future readers understand why the field exists but is not consumed (reference:
activeTabId, send_command handler, sessionDetail).
🪄 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: cd559160-2481-4f84-b84a-436b6712d656

📥 Commits

Reviewing files that changed from the base of the PR and between dfdb772 and 81311fd.

📒 Files selected for processing (17)
  • src/__tests__/cli/commands/dispatch.test.ts
  • src/__tests__/main/preload/process.test.ts
  • src/__tests__/main/web-server/handlers/messageHandlers.test.ts
  • src/__tests__/main/web-server/managers/CallbackRegistry.test.ts
  • src/__tests__/main/web-server/web-server-factory.test.ts
  • src/__tests__/renderer/hooks/useRemoteIntegration.test.ts
  • src/cli/commands/dispatch.ts
  • src/cli/index.ts
  • src/main/preload/process.ts
  • src/main/web-server/WebServer.ts
  • src/main/web-server/handlers/messageHandlers.ts
  • src/main/web-server/managers/CallbackRegistry.ts
  • src/main/web-server/types.ts
  • src/main/web-server/web-server-factory.ts
  • src/renderer/global.d.ts
  • src/renderer/hooks/remote/useRemoteHandlers.ts
  • src/renderer/hooks/remote/useRemoteIntegration.ts
✅ Files skipped from review due to trivial changes (1)
  • src/tests/cli/commands/dispatch.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/tests/renderer/hooks/useRemoteIntegration.test.ts
  • src/tests/main/web-server/managers/CallbackRegistry.test.ts
  • src/main/preload/process.ts
  • src/main/web-server/web-server-factory.ts

Comment thread src/cli/commands/dispatch.ts
Comment thread src/cli/index.ts
Comment thread src/renderer/hooks/remote/useRemoteIntegration.ts
- Major (coderabbit): redact prompt text from useRemoteIntegration info
  logs. Both info-level call sites now log `commandLength` only; the
  truncated preview moves to debug, mirroring the redaction the main
  process applies in web-server-factory.
- Minor (coderabbit): validate that `new_ai_tab_with_prompt` ack contains
  a tabId. Throw on missing id and map to a dedicated `NEW_TAB_NO_ID`
  error code so consumers (Maestro-Discord, Cue) can fail explicitly
  instead of silently receiving `{ success: true, tabId: null }` — that
  silently breaks --new-tab's contract of surfacing a fresh id for
  chaining.
- Minor (coderabbit): reject `--new-tab --force` as `INVALID_OPTIONS`
  with a clear message. A freshly created tab can never be busy, so
  --force has no semantics on this path; the help text now states the
  exclusion explicitly.

Tests:
- dispatch.test.ts: NEW_TAB_NO_ID emitted when desktop omits tabId on
  the --new-tab path; --new-tab --force rejected as INVALID_OPTIONS.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/cli/commands/dispatch.ts (1)

99-101: Optional: replace string-prefix error sentinel with a typed error.

The throw at Line 100 encodes the error code into the message string and the catch at Line 165 demuxes via msg.startsWith('NEW_TAB_NO_ID:'). This works (and the prefix leaks into the user-facing error if anything inadvertently matches), but it's fragile across refactors. A small typed error class makes the code → message mapping unambiguous and keeps the user-facing error text clean.

♻️ Suggested refactor
+class DispatchInternalError extends Error {
+	constructor(public readonly dispatchCode: 'NEW_TAB_NO_ID', message: string) {
+		super(message);
+		this.name = 'DispatchInternalError';
+	}
+}
+
@@
-				if (!result.tabId) {
-					throw new Error('NEW_TAB_NO_ID: new_ai_tab_with_prompt acknowledged without a tabId');
-				}
+				if (!result.tabId) {
+					throw new DispatchInternalError(
+						'NEW_TAB_NO_ID',
+						'new_ai_tab_with_prompt acknowledged without a tabId'
+					);
+				}
@@
-		if (msg.startsWith('NEW_TAB_NO_ID:')) {
+		if (error instanceof DispatchInternalError && error.dispatchCode === 'NEW_TAB_NO_ID') {
 			return {
 				success: false,
 				error:
 					'Maestro desktop acknowledged --new-tab without returning a tab id (cannot chain dispatch)',
 				code: 'NEW_TAB_NO_ID',
 			};
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/commands/dispatch.ts` around lines 99 - 101, Replace the
string-prefixed sentinel error with a dedicated error class: define a
NewTabNoIdError (extends Error) and throw new NewTabNoIdError() where the code
currently does throw new Error('NEW_TAB_NO_ID: ...') in dispatch.ts (the throw
inside the new_ai_tab_with_prompt handling). Update the catch logic that
currently checks msg.startsWith('NEW_TAB_NO_ID:') to use an instanceof
NewTabNoIdError check (or type guard) so the control flow is unambiguous and
user-facing messages remain clean.
🤖 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/cli/commands/dispatch.ts`:
- Around line 99-101: Replace the string-prefixed sentinel error with a
dedicated error class: define a NewTabNoIdError (extends Error) and throw new
NewTabNoIdError() where the code currently does throw new Error('NEW_TAB_NO_ID:
...') in dispatch.ts (the throw inside the new_ai_tab_with_prompt handling).
Update the catch logic that currently checks msg.startsWith('NEW_TAB_NO_ID:') to
use an instanceof NewTabNoIdError check (or type guard) so the control flow is
unambiguous and user-facing messages remain clean.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 54e4ed99-d291-43f2-8008-1911e2f65aa9

📥 Commits

Reviewing files that changed from the base of the PR and between 81311fd and f20f91a.

📒 Files selected for processing (4)
  • src/__tests__/cli/commands/dispatch.test.ts
  • src/cli/commands/dispatch.ts
  • src/cli/index.ts
  • src/renderer/hooks/remote/useRemoteIntegration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tests/cli/commands/dispatch.test.ts

@chr1syy chr1syy added the ready to merge This PR is ready to merge label Apr 27, 2026
@pedramamini
Copy link
Copy Markdown
Collaborator

@chr1syy Hit a merge conflict landing this one — likely from #898 (gist/createGist callback) and/or #896 (web-settings snapshot) which touched overlapping areas in web-server-factory.ts, messageHandlers.ts, CallbackRegistry.ts, and the related tests. Could you rebase on the latest rc and push? Everything else from today's batch (#894, #896, #898, #905, #906, #909) is in. CI was green on this one before the conflict; should be a clean rebase modulo the callback-registry plumbing.

@pedramamini pedramamini merged commit c03f7a8 into RunMaestro:rc Apr 27, 2026
3 checks passed
pedramamini pushed a commit that referenced this pull request Apr 28, 2026
…director-notes

Closes documentation gaps in both the bundled agent-facing CLI reference
(`src/prompts/_maestro-cli.md`) and the public user guide (`docs/cli.md`):

- `dispatch` (PR #907): full section with flag table, JSON response shape,
  and error codes. `send --live` row marked deprecated, pointing to dispatch.
- `gist create` (PR #898): section with `gh` auth prerequisite and error codes.
- `open-browser` (Apr 21): http(s)-only constraint and scheme-less auto-prefix
  behavior documented.
- `open-terminal` (Apr 21): cwd-confinement rule and `zsh` default documented.
- `director-notes`: was only in the bundled prompt; now in the public guide too,
  with the Encore-flag prerequisite called out.

Pure documentation — no source changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants