Add CLI IPC commands for remote Maestro control - #530
Merged
reachrazamair merged 34 commits intoMar 13, 2026
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 CLI↔Electron integration: CLI discovery file and lifecycle, a WebSocket MaestroClient, five CLI commands (open-file, refresh-files, refresh-auto-run, auto-run, status), web-server message types/callbacks for file/auto-run actions, IPC/preload and renderer wiring, and extensive tests across those areas. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant MC as MaestroClient
participant WS as WebSocket
participant Handler as WebServer Handler
participant CB as CallbackRegistry
participant Main as Main (IPC)
participant Renderer as Renderer
CLI->>MC: withMaestroClient(action)
MC->>MC: connect()
MC->>WS: open socket
WS-->>MC: open
CLI->>MC: sendCommand(configure_auto_run)
MC->>WS: send JSON configure_auto_run
WS->>Handler: deliver message
Handler->>CB: configureAutoRun(sessionId, config)
CB->>Main: emit remote:configureAutoRun (responseChannel)
Main->>Renderer: IPC -> configureAutoRun
Renderer-->>Main: sendRemoteConfigureAutoRunResponse(responseChannel, result)
Main-->>CB: return result
CB->>Handler: return success/playbookId
Handler->>WS: send configure_auto_run_result
WS-->>MC: message received
MC-->>CLI: resolve promise
CLI->>MC: disconnect()
MC->>WS: close socket
sequenceDiagram
participant CLI as CLI (open-file)
participant MC as MaestroClient
participant WS as WebSocket
participant Handler as WebServer Handler
participant CB as CallbackRegistry
participant Main as Main (IPC)
participant Renderer as Renderer
CLI->>MC: sendCommand(open_file_tab)
MC->>WS: send JSON open_file_tab
WS->>Handler: handleOpenFileTab
Handler->>CB: openFileTab(sessionId, filePath)
CB->>Main: emit remote:openFileTab(sessionId, filePath)
Main->>Renderer: IPC -> open file tab
Renderer-->>Main: boolean success
Main-->>CB: return success
CB->>Handler: return success
Handler->>WS: send open_file_tab_result
WS-->>MC: receive result
MC-->>CLI: resolve result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
chr1syy
added
the
runmaestro.ai
Greptile SummaryThis PR introduces a full CLI IPC layer that lets external tools (and the Maestro AI agent itself via system prompt instructions) remotely control the Maestro desktop app over a WebSocket connection. The implementation spans CLI commands ( Key changes:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (maestro-client.ts)
participant DS as Discovery File (cli-server.json)
participant WS as WebSocket Server (WebServer.ts)
participant MH as MessageHandlers.ts
participant CB as CallbackRegistry.ts
participant IPC as Electron ipcMain
participant R as Renderer (App.tsx)
Note over CLI,DS: App startup
WS->>DS: writeCliServerInfo(port, token, pid)
Note over CLI,R: CLI command (e.g. maestro auto-run)
CLI->>DS: readCliServerInfo()
DS-->>CLI: { port, token, pid }
CLI->>CLI: isCliServerRunning() (PID check)
CLI->>WS: WebSocket connect ws://localhost:{port}/{token}/ws
WS-->>CLI: open
CLI->>WS: sendCommand({ type: configure_auto_run, requestId, ... })
WS->>MH: handleMessage()
MH->>CB: configureAutoRun(sessionId, config)
CB->>IPC: ipcMain.once(responseChannel, ...)
IPC->>R: webContents.send(remote:configureAutoRun)
R->>R: handle: save playbook / launch / error
R->>IPC: ipcRenderer.send(responseChannel, result)
IPC-->>CB: resolve(result)
CB-->>MH: { success, playbookId? }
MH->>WS: send({ type: configure_auto_run_result, success })
WS-->>CLI: message (type-based match, no requestId echo)
CLI->>CLI: disconnect()
Note over WS,DS: User stops server from UI
IPC->>WS: live:stopServer
WS->>DS: deleteCliServerInfo()
Note over CLI,DS: ⚠ live:startServer does NOT call writeCliServerInfo
|
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/app-lifecycle/quit-handler.ts (1)
182-190:⚠️ Potential issue | 🟡 MinorDiscovery file deleted before server fully stops - minor ordering inconsistency.
The
webServer?.stop()call at lines 184-186 is fire-and-forget, butdeleteCliServerInfo()is called immediately after at line 190. This creates a brief window where the discovery file is gone but the socket is still reachable.In contrast,
web.ts(line 300-310 per context snippet) deletes the file after awaitingwebServer.stop().Since this is the shutdown path and the window is very brief, this is unlikely to cause real issues. However, for consistency, you could move the deletion into the
.then()callback ofstop()or accept this as a known limitation of fire-and-forget shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/app-lifecycle/quit-handler.ts` around lines 182 - 190, The discovery file is deleted before the web server stop completes because webServer?.stop() is fire-and-forget; move the deleteCliServerInfo() call to execute after stop() resolves (e.g., in the promise .then() or after an await) so the CLI file is removed only once getWebServer().stop() has finished; update the shutdown block around getWebServer(), webServer.stop(), logger.info/logger.error and deleteCliServerInfo() to perform deletion after successful stop (or in a finally that runs after awaiting stop()).
🧹 Nitpick comments (2)
src/main/index.ts (1)
405-413: Orphaned comment on line 413.The comment "// via live:startServer IPC call from the renderer" appears to be a leftover from previous code and no longer applies to the context (it's after the
ensureCliServerblock, not related to any IPC call).♻️ Suggested fix
await ensureCliServer({ getWebServer: () => webServer, setWebServer: (server) => { webServer = server; }, createWebServer, }); - // via live:startServer IPC call from the renderer app.on('activate', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 405 - 413, The trailing comment "// via live:startServer IPC call from the renderer" is orphaned and misleading after the ensureCliServer block; remove or relocate it to the correct IPC handler instead. Open the code around ensureCliServer (references: ensureCliServer, getWebServer, setWebServer, createWebServer) and delete that comment or move it next to the actual live:startServer IPC handler so comments align with the related code.src/main/ipc/handlers/web.ts (1)
56-95: Consider adding a guard against concurrent invocations.While
ensureCliServeris currently called only once during app startup, the function lacks protection against concurrent calls. If called simultaneously (e.g., future code paths), both calls could pass the!webServer.isActive()check before either completeswebServer.start(), potentially leading to duplicate server start attempts.A simple mutex or "starting" flag would future-proof this:
♻️ Suggested guard pattern
+let cliServerStarting: Promise<void> | null = null; + export async function ensureCliServer(deps: WebHandlerDependencies): Promise<void> { + // Prevent concurrent startup attempts + if (cliServerStarting) { + return cliServerStarting; + } + const { getWebServer, setWebServer, createWebServer } = deps; + cliServerStarting = (async () => { try { // ... existing logic ... } catch (error: any) { logger.error(`Failed to start CLI server: ${error.message}`, 'CliServer'); + } finally { + cliServerStarting = null; } + })(); + + return cliServerStarting; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/web.ts` around lines 56 - 95, ensureCliServer can race on startup: add a guard (e.g., a module-level "starting" boolean or Promise) to prevent concurrent invocation from attempting to start the same server. Inside ensureCliServer, obtain the current webServer via getWebServer(), then if no server exists set a "starting" flag (or store a Promise) before calling createWebServer()/setWebServer(); if another caller sees the flag, await the existing starting Promise or return early. Also wrap the start sequence (the webServer.start() and writeCliServerInfo calls) with the same guard so only one caller invokes webServer.start(), using webServer.isActive() as a secondary check; clear the flag (or resolve the Promise) on completion or error. Reference symbols: ensureCliServer, getWebServer, setWebServer, createWebServer, webServer.isActive, webServer.start, writeCliServerInfo.
🤖 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__/shared/cli-server-discovery.test.ts`:
- Around line 96-106: Wrap each test's process.env modification and restoration
in a try...finally block so cleanup always runs, and when restoring use
conditional deletion: if the saved original value is undefined use delete
process.env.APPDATA (or the appropriate env name) otherwise reassign the saved
value; apply this pattern around the setup/teardown surrounding
readCliServerInfo() and the other tests that set APPDATA (the same applies to
the other env vars referenced in this file) to avoid converting undefined into
the string "undefined" and to ensure restoration runs even if assertions fail.
In `@src/cli/services/maestro-client.ts`:
- Around line 76-92: The requestId is generated and stored in pendingRequests
but never injected into the outgoing message, and setupMessageHandler currently
matches responses by expectedType only; update the send path (before
this.ws!.send(JSON.stringify(message)) in the Promise that creates requestId) to
attach requestId to the message object, and modify setupMessageHandler to first
try to resolve pendingRequests by response.requestId (matching the stored
requestId key) and only fall back to matching by expectedType when requestId is
absent; keep pendingRequests cleanup (delete and clear timeout) the same and
ensure resolve/reject use the stored handlers.
In `@src/main/preload/process.ts`:
- Around line 452-469: The onRemoteConfigureAutoRun listener currently invokes
the renderer callback directly and if that callback throws or returns a rejected
promise the ipc responseChannel never receives a failure message, causing the
caller to time out; update the handler created in onRemoteConfigureAutoRun so it
wraps the callback call in try/catch and handles promise rejections, and on any
error send a failure payload like { success: false, error: String(err) } via
sendRemoteConfigureAutoRunResponse (or ipcRenderer.send(responseChannel, ...))
before returning; ensure the removeListener logic still unregisters the same
handler.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 732-762: handleConfigureAutoRun currently trusts message.documents
and forwards possibly malformed entries to this.callbacks.configureAutoRun; add
strict per-item validation before building config: iterate over the documents
array and for each entry on the local variable documents ensure it is an object
with a non-empty string filename and, if resetOnCompletion is present, that it
is a boolean (or require resetOnCompletion to be boolean when present),
otherwise call this.sendError with a descriptive message and return; only pass a
sanitized array (e.g., objects with filename:string and
resetOnCompletion?:boolean) into the config used for
this.callbacks.configureAutoRun so downstream code never receives invalid types.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 542-568: The responseChannel currently uses Date.now() which can
collide for concurrent configureAutoRun calls; replace its creation with a
collision-free identifier (e.g., use crypto.randomUUID() or a UUID from the uuid
package, or combine Date.now() + sessionId + Math.random()) so each call builds
a unique channel string (update the responseChannel assignment used by
ipcMain.once and mainWindow.webContents.send). Ensure the new unique ID is used
consistently for both listener registration (ipcMain.once(responseChannel, ...))
and the sent message channel arg so promises cannot resolve with the wrong
response.
In `@src/renderer/App.tsx`:
- Around line 1711-1714: The handler currently ignores the event's sessionId and
always calls handleAutoRunRefresh() for the active UI session; change the
handler to extract the sessionId from the incoming Event (cast to the
CustomEvent or read e.detail.sessionId) and call the refresh routine for that
specific session (either by passing the sessionId into
handleAutoRunRefresh(sessionId) or invoking an existing function like
refreshAutoRunForSession(sessionId)); update the handler implementation in the
same block where handler and handleAutoRunRefresh are defined so the refresh
targets the requested session rather than the locally active one.
- Around line 1782-1785: The configure-only branch currently returns success
without persisting the provided config (config.documents, prompt, and loop
settings); modify the branch that checks saveAsPlaybook/launch so that when both
are false it reuses the same persistence/update path used by the save-as branch:
persist the incoming config.documents, prompt, and loop into the app state or
call the existing save/playbook persistence helper (the same routine used when
saveAsPlaybook is true), then return success via
window.maestro.process.sendRemoteConfigureAutoRunResponse; ensure you update the
same store or file-write helper the saveAsPlaybook path uses so the CLI-visible
desktop state reflects the configure-only change.
- Around line 1670-1689: The handler currently reads files using
window.maestro.fs.readFile/stat without the session's remote context and derives
the tab name by splitting only on '/', which breaks SSH sessions and Windows
paths; inside the handler (the async function handling open-file events), look
up the session's sshRemoteId (from the same sessions store used by
setActiveSessionId or activeSessionId), pass that sshRemoteId into both
window.maestro.fs.readFile and window.maestro.fs.stat calls so the SSH-backed
session uses the remote FS, and compute filename using a cross-platform split
(e.g. split on both '/' and '\\' or use a path basename helper) before calling
handleOpenFileTab (refer to handler, setActiveSessionId,
window.maestro.fs.readFile/stat, handleOpenFileTab, and sshRemoteId).
---
Outside diff comments:
In `@src/main/app-lifecycle/quit-handler.ts`:
- Around line 182-190: The discovery file is deleted before the web server stop
completes because webServer?.stop() is fire-and-forget; move the
deleteCliServerInfo() call to execute after stop() resolves (e.g., in the
promise .then() or after an await) so the CLI file is removed only once
getWebServer().stop() has finished; update the shutdown block around
getWebServer(), webServer.stop(), logger.info/logger.error and
deleteCliServerInfo() to perform deletion after successful stop (or in a finally
that runs after awaiting stop()).
---
Nitpick comments:
In `@src/main/index.ts`:
- Around line 405-413: The trailing comment "// via live:startServer IPC call
from the renderer" is orphaned and misleading after the ensureCliServer block;
remove or relocate it to the correct IPC handler instead. Open the code around
ensureCliServer (references: ensureCliServer, getWebServer, setWebServer,
createWebServer) and delete that comment or move it next to the actual
live:startServer IPC handler so comments align with the related code.
In `@src/main/ipc/handlers/web.ts`:
- Around line 56-95: ensureCliServer can race on startup: add a guard (e.g., a
module-level "starting" boolean or Promise) to prevent concurrent invocation
from attempting to start the same server. Inside ensureCliServer, obtain the
current webServer via getWebServer(), then if no server exists set a "starting"
flag (or store a Promise) before calling createWebServer()/setWebServer(); if
another caller sees the flag, await the existing starting Promise or return
early. Also wrap the start sequence (the webServer.start() and
writeCliServerInfo calls) with the same guard so only one caller invokes
webServer.start(), using webServer.isActive() as a secondary check; clear the
flag (or resolve the Promise) on completion or error. Reference symbols:
ensureCliServer, getWebServer, setWebServer, createWebServer,
webServer.isActive, webServer.start, writeCliServerInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec9ec798-88c8-4f32-8199-86ca39aacc59
📒 Files selected for processing (30)
src/__tests__/cli/commands/auto-run.test.tssrc/__tests__/cli/commands/open-file.test.tssrc/__tests__/cli/services/maestro-client.test.tssrc/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/main/web-server/managers/CallbackRegistry.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/shared/cli-server-discovery.test.tssrc/cli/commands/auto-run.tssrc/cli/commands/open-file.tssrc/cli/commands/refresh-auto-run.tssrc/cli/commands/refresh-files.tssrc/cli/commands/send.tssrc/cli/commands/status.tssrc/cli/index.tssrc/cli/services/maestro-client.tssrc/main/app-lifecycle/quit-handler.tssrc/main/index.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/web.tssrc/main/preload/process.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/prompts/maestro-system-prompt.mdsrc/renderer/App.tsxsrc/renderer/global.d.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/shared/cli-server-discovery.ts
Comment on lines
+1782
to
+1785
| // Case 3: Just configure (no launch, no save) | ||
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | ||
| success: true, | ||
| }); |
There was a problem hiding this comment.
The configure-only branch currently does nothing.
When neither launch nor saveAsPlaybook is set, Lines 1783-1785 return success without persisting config.documents, prompt, or loop settings anywhere. src/cli/index.ts advertises configure-only as the default auto-run path, so the CLI can report success while leaving the desktop state unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/App.tsx` around lines 1782 - 1785, The configure-only branch
currently returns success without persisting the provided config
(config.documents, prompt, and loop settings); modify the branch that checks
saveAsPlaybook/launch so that when both are false it reuses the same
persistence/update path used by the save-as branch: persist the incoming
config.documents, prompt, and loop into the app state or call the existing
save/playbook persistence helper (the same routine used when saveAsPlaybook is
true), then return success via
window.maestro.process.sendRemoteConfigureAutoRunResponse; ensure you update the
same store or file-write helper the saveAsPlaybook path uses so the CLI-visible
desktop state reflects the configure-only change.
chr1syy
marked this pull request as draft
March 7, 2026 21:24
chr1syy
marked this pull request as ready for review
March 8, 2026 14:20
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/renderer/App.tsx (1)
1716-1723:⚠️ Potential issue | 🟠 Major
refresh-auto-runstill targets the wrong session.
setActiveSessionId(sessionId)is asynchronous from this handler’s point of view, so the immediatehandleAutoRunRefresh()still runs against the previously bound active session.refresh-auto-run --session ...can therefore refresh the wrong docs first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 1716 - 1723, The handler currently calls setActiveSessionId(sessionId) then immediately calls handleAutoRunRefresh(), which runs against the old active session; change the flow so the refresh targets the intended session by passing sessionId directly to the refresher (e.g., update handleAutoRunRefresh to accept an optional sessionId parameter and call handleAutoRunRefresh(sessionId) from the handler) or otherwise ensure the refresh runs after the state commit (for example, trigger the same refresh logic from the effect that reacts to activeSessionId). Refer to handler, setActiveSessionId, and handleAutoRunRefresh when making the change.
🧹 Nitpick comments (1)
src/__tests__/renderer/hooks/useRemoteIntegration.test.ts (1)
124-137: Exercise the new remote subscriptions instead of only stubbing them.These mocks never capture or invoke the added callbacks, so the new
openFileTab/refreshFileTree/refreshAutoRunDocs/configureAutoRunwiring inuseRemoteIntegrationis still untested. Please store the handlers like the existing remote mocks and assert the dispatchedCustomEventpayloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useRemoteIntegration.test.ts` around lines 124 - 137, The test currently stubs the remote subscription helpers (onRemoteOpenFileTab, onRemoteRefreshFileTree, onRemoteRefreshAutoRunDocs, onRemoteConfigureAutoRun) with vi.fn().mockImplementation(() => () => {}) which never captures or invokes handlers; change each mock to capture the provided callback (e.g., save it to a local variable) and return an unsubscribe, then in the test invoke the saved handlers to simulate the remote events and assert that useRemoteIntegration dispatches the expected CustomEvent payloads and calls sendRemoteNewTabResponse / sendRemoteConfigureAutoRunResponse appropriately; reference the mock names onRemoteOpenFileTab, onRemoteRefreshFileTree, onRemoteRefreshAutoRunDocs, onRemoteConfigureAutoRun and the response spies sendRemoteNewTabResponse and sendRemoteConfigureAutoRunResponse when adding the assertions.
🤖 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__/shared/cli-server-discovery.test.ts`:
- Around line 329-357: Tests for isCliServerRunning temporarily replace
process.kill but restore it only after assertions, risking leaving it mocked if
an assertion throws; wrap the mock setup and restore in a try/finally in both
test cases so process.kill is always restored: in the 'should return true for
current PID' and 'should return false for non-existent PID' tests, assign
originalKill, set process.kill to the mock, execute the test logic in the try
block, and restore process.kill = originalKill in the finally block to ensure
cleanup even on failures.
In `@src/cli/commands/auto-run.ts`:
- Around line 48-54: The current parsing of options.maxLoops uses parseInt which
accepts strings like "3foo"; update the validation around the
loopEnabled/maxLoops logic to ensure the entire options.maxLoops string is a
valid positive integer before converting: either test the string with a
full-match regex (e.g. /^\d+$/) or convert with Number(options.maxLoops) and
check Number.isInteger(...) and > 0, then assign maxLoops to that integer (or
undefined); apply the check where maxLoops is set and in the subsequent error
branch that currently logs 'Error: --max-loops must be a positive integer'.
In `@src/cli/commands/send.ts`:
- Around line 136-140: The select_session command only sends sessionId (agentId)
which focuses the session but not the specific tab; update the withMaestroClient
callback where client.sendCommand is called (the select_session request) to
include the resolved tab id (e.g., add a tabId property) so the desktop focuses
the correct tab; to get that tabId, resolve the matching tab for the target
agent session before calling client.sendCommand (either by querying the local
data structure that maps agentSessionId/result.agentSessionId to tabId or by
calling the server API to look up agent-session → tab mapping) and pass that
tabId in the select_session payload, or if server-side support is preferred
implement a lookup endpoint that accepts agentSessionId and returns tabId and
call it here before issuing the select_session request.
In `@src/cli/services/maestro-client.ts`:
- Around line 63-68: Add a 'close' handler inside setupMessageHandler() that
mirrors disconnect() behavior: when the WebSocket (ws) emits 'close', iterate
over the pending requests map (pendingRequests) and reject each promise with an
Error indicating the Maestro connection closed, then clear the map and perform
any necessary cleanup; ensure the handler uses the same rejection message/shape
as disconnect() to immediately fail pending commands rather than waiting for the
command timeout.
In `@src/renderer/App.tsx`:
- Around line 1786-1789: The code sends success before the async startBatchRun
completes; change the branch to await startBatchRun(sessionId, batchConfig,
folderPath) and only call
window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, {
success: true }) after it resolves, and add a try/catch around the await so that
on failure you send a failure response (e.g., sendRemoteConfigureAutoRunResponse
with success: false and include the error details) using the same
responseChannel.
- Around line 1682-1694: The file read branch calls window.maestro.fs.readFile
and window.maestro.fs.stat with sshRemoteId but when creating the tab via
handleOpenFileTab it omits sshRemoteId, losing remote context; include
sshRemoteId in the object passed to handleOpenFileTab (alongside path, name,
content, lastModified) so the created tab retains the remote session identifier
used by the read/save/reload flows.
---
Duplicate comments:
In `@src/renderer/App.tsx`:
- Around line 1716-1723: The handler currently calls
setActiveSessionId(sessionId) then immediately calls handleAutoRunRefresh(),
which runs against the old active session; change the flow so the refresh
targets the intended session by passing sessionId directly to the refresher
(e.g., update handleAutoRunRefresh to accept an optional sessionId parameter and
call handleAutoRunRefresh(sessionId) from the handler) or otherwise ensure the
refresh runs after the state commit (for example, trigger the same refresh logic
from the effect that reacts to activeSessionId). Refer to handler,
setActiveSessionId, and handleAutoRunRefresh when making the change.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useRemoteIntegration.test.ts`:
- Around line 124-137: The test currently stubs the remote subscription helpers
(onRemoteOpenFileTab, onRemoteRefreshFileTree, onRemoteRefreshAutoRunDocs,
onRemoteConfigureAutoRun) with vi.fn().mockImplementation(() => () => {}) which
never captures or invokes handlers; change each mock to capture the provided
callback (e.g., save it to a local variable) and return an unsubscribe, then in
the test invoke the saved handlers to simulate the remote events and assert that
useRemoteIntegration dispatches the expected CustomEvent payloads and calls
sendRemoteNewTabResponse / sendRemoteConfigureAutoRunResponse appropriately;
reference the mock names onRemoteOpenFileTab, onRemoteRefreshFileTree,
onRemoteRefreshAutoRunDocs, onRemoteConfigureAutoRun and the response spies
sendRemoteNewTabResponse and sendRemoteConfigureAutoRunResponse when adding the
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02f7a999-9e7c-4900-9128-e54f60bb41eb
📒 Files selected for processing (18)
src/__tests__/cli/commands/auto-run.test.tssrc/__tests__/cli/commands/open-file.test.tssrc/__tests__/cli/services/maestro-client.test.tssrc/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/__tests__/shared/cli-server-discovery.test.tssrc/cli/commands/auto-run.tssrc/cli/commands/send.tssrc/cli/commands/status.tssrc/cli/index.tssrc/cli/services/maestro-client.tssrc/main/preload/process.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/prompts/maestro-system-prompt.mdsrc/renderer/App.tsxsrc/renderer/hooks/remote/useRemoteIntegration.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/cli/services/maestro-client.test.ts
- src/cli/index.ts
- src/prompts/maestro-system-prompt.md
Comment on lines
+136
to
+140
| await withMaestroClient(async (client) => { | ||
| await client.sendCommand( | ||
| { type: 'select_session', sessionId: agentId, focus: true }, | ||
| 'select_session_result' | ||
| ); |
There was a problem hiding this comment.
--tab still focuses the session, not the target tab.
This only sends sessionId, so multi-tab sessions reopen whichever tab is already active in the desktop UI. If send resumed a different result.agentSessionId, the wrong tab stays focused. Please resolve the matching tab ID and include it in the select request, or add a server-side lookup from agent-session ID to tab ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/send.ts` around lines 136 - 140, The select_session command
only sends sessionId (agentId) which focuses the session but not the specific
tab; update the withMaestroClient callback where client.sendCommand is called
(the select_session request) to include the resolved tab id (e.g., add a tabId
property) so the desktop focuses the correct tab; to get that tabId, resolve the
matching tab for the target agent session before calling client.sendCommand
(either by querying the local data structure that maps
agentSessionId/result.agentSessionId to tabId or by calling the server API to
look up agent-session → tab mapping) and pass that tabId in the select_session
payload, or if server-side support is preferred implement a lookup endpoint that
accepts agentSessionId and returns tabId and call it here before issuing the
select_session request.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/cli/services/maestro-client.ts (1)
172-175: User-facing error message should use "agent" terminology.The error message uses "sessions" but coding guidelines specify using "agent" in user-facing language.
📝 Proposed terminology fix
if (sessions.length === 0) { - console.error('Error: No sessions found. Create a session in Maestro first.'); + console.error('Error: No agents found. Create an agent in Maestro first.'); process.exit(1); }As per coding guidelines: "use 'agent' in user-facing language and 'session' for provider-level conversation contexts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/maestro-client.ts` around lines 172 - 175, Replace the user-facing error text that mentions "sessions" with "agent" so it follows terminology guidelines: locate the check that uses the sessions array (the sessions.length === 0 branch in maestro-client.ts) and update the console.error call to a message like "Error: No agents found. Create an agent in Maestro first." while leaving the existing process.exit(1) behavior unchanged.
🤖 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/renderer/App.tsx`:
- Around line 1767-1795: The code currently constructs batchConfig and calls
startBatchRun then reports success even when documents is empty; before awaiting
startBatchRun, validate that (config.documents || []) yields a non-empty
batchConfig.documents and if empty call
window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, {
success: false, error: 'No documents provided for auto-run' }) and return;
adjust the block that builds batchConfig (referencing config.documents,
batchConfig.documents) so you reject empty document lists locally instead of
relying on startBatchRun to throw.
- Around line 1680-1695: The async read can complete after the active session
changes, so ensure the newly opened file tab is created for the originally
requested session by passing that sessionId into the tab-creation routine
instead of relying on global active session; update handleOpenFileTab (or add a
new helper like handleOpenFileTabForSession) to accept a sessionId parameter and
use that sessionId when creating the tab, then call it here with the captured
sessionId variable (while keeping setActiveSessionId(sessionId) as-is); also
update any other call sites of handleOpenFileTab to supply a sessionId or use
the new helper so tabs are always pinned to the intended session.
---
Nitpick comments:
In `@src/cli/services/maestro-client.ts`:
- Around line 172-175: Replace the user-facing error text that mentions
"sessions" with "agent" so it follows terminology guidelines: locate the check
that uses the sessions array (the sessions.length === 0 branch in
maestro-client.ts) and update the console.error call to a message like "Error:
No agents found. Create an agent in Maestro first." while leaving the existing
process.exit(1) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fb7fe65-c18d-4956-9d72-10bc36e02cc2
📒 Files selected for processing (4)
src/__tests__/shared/cli-server-discovery.test.tssrc/cli/commands/auto-run.tssrc/cli/services/maestro-client.tssrc/renderer/App.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/commands/auto-run.ts
- src/tests/shared/cli-server-discovery.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/cli/services/maestro-client.test.ts (1)
173-188: Consider adding a test forrequestId-based response matching.The current test at lines 173-188 verifies type-based matching (the fallback path), but there's no test that verifies the primary
requestId-based matching path. Adding a test where the response includes a matchingrequestIdwould ensure that code path is covered.💡 Suggested test case
it('should resolve on matching requestId', async () => { const client = await createConnectedClient(); const commandPromise = client.sendCommand<{ type: string; data: string }>( { type: 'ping' }, 'pong' ); // Capture the requestId from the sent message const sentMessage = JSON.parse(mockWsInstance.send.mock.calls[0][0]); const requestId = sentMessage.requestId; // Respond with matching requestId mockWsInstance.emit('message', JSON.stringify({ type: 'pong', data: 'ok', requestId })); const result = await commandPromise; expect(result.type).toBe('pong'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/cli/services/maestro-client.test.ts` around lines 173 - 188, Add a unit test to cover the requestId-based resolution path by using createConnectedClient() and calling client.sendCommand(...) to get a pending promise, then read the sent message from mockWsInstance.send.mock.calls[0][0] to extract the requestId, emit a 'message' on mockWsInstance containing the same requestId plus the expected payload (e.g., { type: 'pong', data: 'ok', requestId }), await the command promise and assert the response fields; reference sendCommand, createConnectedClient, and mockWsInstance in the test to locate the correct mocks and behavior.
🤖 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__/cli/services/maestro-client.test.ts`:
- Around line 173-188: Add a unit test to cover the requestId-based resolution
path by using createConnectedClient() and calling client.sendCommand(...) to get
a pending promise, then read the sent message from
mockWsInstance.send.mock.calls[0][0] to extract the requestId, emit a 'message'
on mockWsInstance containing the same requestId plus the expected payload (e.g.,
{ type: 'pong', data: 'ok', requestId }), await the command promise and assert
the response fields; reference sendCommand, createConnectedClient, and
mockWsInstance in the test to locate the correct mocks and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0990adc4-af79-4dc4-8269-15fe72585243
📒 Files selected for processing (5)
src/__tests__/cli/services/maestro-client.test.tssrc/cli/services/maestro-client.tssrc/renderer/App.tsxsrc/renderer/hooks/git/useFileExplorerEffects.tssrc/renderer/hooks/tabs/useTabHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/App.tsx
chr1syy
force-pushed
the
symphony/issue-511-mmgpua7l
branch
from
March 9, 2026 10:41
6a6a143 to
8e15c35
Compare
Contributor
Author
|
@coderabbitai re-review please |
|
✅ Actions performedFull review triggered. |
Contributor
Author
|
@greptileai re-review please |
Comment on lines
+146
to
+154
| // Fall back to matching by response type | ||
| for (const [requestId, pending] of this.pendingRequests) { | ||
| if (pending.expectedType === msgType) { | ||
| clearTimeout(pending.timeout); | ||
| this.pendingRequests.delete(requestId); | ||
| pending.resolve(msg); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Type-based response matching can resolve the wrong pending request
The fallback matching logic resolves the first pending request whose expectedType matches the incoming message type. Since the server handlers in this PR (e.g., handleOpenFileTab, handleRefreshFileTree, handleConfigureAutoRun) never echo back the requestId, every response always falls through to this loop.
If two commands of the same type are in-flight simultaneously — for example, two configure_auto_run commands sent back-to-back before either response arrives — the first response will resolve whichever request happens to be first in the Map, regardless of which command it actually answers. The second request will then be resolved by the second response (which may carry a result from a different operation), or time out if no further response arrives.
While pure CLI usage serialises commands (one at a time), the web interface can have multiple connected clients that may send concurrent requests of the same type. The standard fix is to echo requestId back in the server response for all handlers, then remove the type-fallback path entirely:
// In each handleXxx method, include requestId in the sent response:
this.send(client, {
type: 'open_file_tab_result',
success,
sessionId,
filePath,
requestId: message.requestId, // echo back for reliable matching
});
Contributor
Author
There was a problem hiding this comment.
The review comment is incorrect. All three handlers already echo requestId back. Here's the evidence:
handleOpenFileTab (line ~935):
this.send(client, {
type: 'open_file_tab_result',
success,
sessionId,
filePath,
requestId: message.requestId, // ✅ echoed
});handleRefreshFileTree (line ~750):
this.send(client, {
type: 'refresh_file_tree_result',
success,
sessionId,
requestId: message.requestId, // ✅ echoed
});handleConfigureAutoRun (line ~875):
this.send(client, {
type: 'configure_auto_run_result',
success: result.success,
...
requestId: message.requestId, // ✅ echoed
});No fix needed
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/cli/services/maestro-client.ts (1)
122-129: Consider: Differentiate server-initiated close from client disconnect.The close handler uses "Client disconnected" for all close events, but this is also triggered when the server closes the connection. Consider a more accurate message like "Connection closed" to avoid confusion during debugging.
this.ws.on('close', () => { for (const [, pending] of this.pendingRequests) { clearTimeout(pending.timeout); - pending.reject(new Error('Client disconnected')); + pending.reject(new Error('Maestro connection closed')); } this.pendingRequests.clear(); this.ws = null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/maestro-client.ts` around lines 122 - 129, The close handler currently rejects pending requests with the misleading message 'Client disconnected'; update the ws close handler (this.ws.on('close', ...)) to use a more accurate message like 'Connection closed' and include the close event details (use the close event parameters code and reason) in the rejection/log so you can tell server-initiated closes from client disconnects; ensure you still clear pending.timeouts and this.pendingRequests and set this.ws = null while using pending.reject(new Error(`Connection closed${code ? ` (code=${code})` : ''}${reason ? `: ${reason}` : ''}`)).src/__tests__/main/web-server/web-server-factory.test.ts (1)
39-42: Mock additions look correct, but consider adding registration assertions.The new callback mocks align with the WebServer API surface. However, the
callback registrationstest block (lines 208-262) doesn't include assertions for the new callbacks like it does for existing ones. Consider adding tests to verify these callbacks are registered:it('should register openFileTabCallback', () => { expect(server.setOpenFileTabCallback).toHaveBeenCalled(); }); it('should register refreshFileTreeCallback', () => { expect(server.setRefreshFileTreeCallback).toHaveBeenCalled(); }); // ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/web-server/web-server-factory.test.ts` around lines 39 - 42, The test added mocks for setOpenFileTabCallback, setRefreshFileTreeCallback, setRefreshAutoRunDocsCallback, and setConfigureAutoRunCallback but the "callback registrations" test block doesn't assert they were registered; update the test block that verifies callback registrations (the section around the existing callback assertions) to include expectations that server.setOpenFileTabCallback, server.setRefreshFileTreeCallback, server.setRefreshAutoRunDocsCallback, and server.setConfigureAutoRunCallback have been called (use the same pattern as the existing registration assertions).src/main/index.ts (1)
405-413: Consider removing or updating the orphaned comment.Line 413 contains a comment ("via live:startServer IPC call from the renderer") that appears to be a leftover from before the CLI server auto-start was added. It doesn't connect to the surrounding code and may confuse future readers.
🧹 Proposed cleanup
await ensureCliServer({ getWebServer: () => webServer, setWebServer: (server) => { webServer = server; }, createWebServer, }); - // via live:startServer IPC call from the renderer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 405 - 413, The comment "via live:startServer IPC call from the renderer" is now orphaned after the auto-start of the CLI server; update or remove it to avoid confusion. Edit the block around the ensureCliServer call (references: ensureCliServer, getWebServer, setWebServer, createWebServer, webServer) and either delete that trailing comment or replace it with a brief note explaining that ensureCliServer auto-starts the CLI server and writes the discovery file for CLI IPC.src/renderer/global.d.ts (1)
315-317: Consider typing theconfigparameter more explicitly.The
configparameter is typed asany, which loses type safety. Consider using a more specific type that matchesConfigureAutoRunCallback's config shape fromsrc/main/web-server/types.ts.♻️ Suggested improvement
onRemoteConfigureAutoRun: ( - callback: (sessionId: string, config: any, responseChannel: string) => void + callback: (sessionId: string, config: { + documents: Array<{ filename: string; resetOnCompletion?: boolean }>; + prompt?: string; + loopEnabled?: boolean; + maxLoops?: number; + saveAsPlaybook?: string; + launch?: boolean; + }, responseChannel: string) => void ) => () => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/global.d.ts` around lines 315 - 317, The onRemoteConfigureAutoRun callback currently types its config parameter as any; change it to the concrete type used by the backend (the config shape from ConfigureAutoRunCallback) by importing or re-exporting that type and replacing the `config: any` signature in onRemoteConfigureAutoRun with the specific type (e.g., `config: ConfigureAutoRunConfig` or the actual ConfigureAutoRunCallback's config type), and update the declaration file to include the necessary type import or a matching type alias so the renderer types align with ConfigureAutoRunCallback.src/main/web-server/handlers/messageHandlers.ts (1)
773-780: Consider validating optional config fields at the WebSocket boundary.The
documentsarray is now properly validated (nice fix!), butmaxLoops,loopEnabled,saveAsPlaybook, andlaunchare cast without type checks. A client sending{ maxLoops: "five" }or{ loopEnabled: "true" }would pass through and potentially cause unexpected behavior downstream.♻️ Proposed validation before building config
+ // Validate optional config fields + if (message.maxLoops !== undefined && (typeof message.maxLoops !== 'number' || message.maxLoops < 1)) { + this.sendError(client, 'maxLoops must be a positive number if provided'); + return; + } + if (message.loopEnabled !== undefined && typeof message.loopEnabled !== 'boolean') { + this.sendError(client, 'loopEnabled must be a boolean if provided'); + return; + } + if (message.launch !== undefined && typeof message.launch !== 'boolean') { + this.sendError(client, 'launch must be a boolean if provided'); + return; + } + if (message.saveAsPlaybook !== undefined && typeof message.saveAsPlaybook !== 'string') { + this.sendError(client, 'saveAsPlaybook must be a string if provided'); + return; + } + const config = { documents,🤖 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 773 - 780, The config is casting optional fields from message without type checks (config: documents, prompt, loopEnabled, maxLoops, saveAsPlaybook, launch), so validate and coerce each optional field at the WebSocket boundary before building config: check message.loopEnabled is a boolean (or parse "true"/"false"), ensure message.maxLoops is a finite number (parseInt/Number and clamp/NaN-check), verify saveAsPlaybook is a non-empty string when present, and ensure launch is boolean; if a value is invalid, either reject the message with a clear error or omit the field so downstream code receives correctly typed values.
🤖 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/status.ts`:
- Around line 29-32: Change the user-facing status message to use "agent"
instead of "session": update the console.log that currently references
sessionsResult.sessions and sessionCount (and the pluralization logic using
sessionCount !== 1) so the printed string reads "agent" or "agents" (e.g., `with
${sessionCount} agent${sessionCount !== 1 ? 's' : ''}`) while leaving the
underlying Session-related variables and types unchanged.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 798-827: The handleOpenFileTab handler currently forwards
unchecked filePath to callbacks.openFileTab, enabling path traversal; fix by
resolving the path against the session's allowed root and rejecting any path
that escapes it: import Node's path module, obtain the session root directory
(e.g., via a session-to-root lookup or getSessionRootDir(sessionId)), compute
const resolved = path.resolve(sessionRoot, filePath), then verify
resolved.startsWith(sessionRoot + path.sep) (or path.normalize/sessionRoot
normalization) and call this.sendError(client, 'Invalid file path') if the check
fails; only call this.callbacks.openFileTab(sessionId, resolved) when the
resolved path is contained within the session root.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 351-353: The chat objects are being updated with two different
keys (moderatorSessionId vs moderatorAgentSessionId) causing state divergence;
update the setter inside setGroupChats (the map callback that currently assigns
moderatorSessionId) to use the same property name used by the
onModeratorSessionIdChanged listener (moderatorAgentSessionId) so all code paths
read/write the same field, and search for any other writes/reads to
moderatorSessionId or moderatorAgentSessionId to unify them to the chosen name
across useGroupChatHandlers (including the setGroupChats call and the
onModeratorSessionIdChanged handler).
---
Nitpick comments:
In `@src/__tests__/main/web-server/web-server-factory.test.ts`:
- Around line 39-42: The test added mocks for setOpenFileTabCallback,
setRefreshFileTreeCallback, setRefreshAutoRunDocsCallback, and
setConfigureAutoRunCallback but the "callback registrations" test block doesn't
assert they were registered; update the test block that verifies callback
registrations (the section around the existing callback assertions) to include
expectations that server.setOpenFileTabCallback,
server.setRefreshFileTreeCallback, server.setRefreshAutoRunDocsCallback, and
server.setConfigureAutoRunCallback have been called (use the same pattern as the
existing registration assertions).
In `@src/cli/services/maestro-client.ts`:
- Around line 122-129: The close handler currently rejects pending requests with
the misleading message 'Client disconnected'; update the ws close handler
(this.ws.on('close', ...)) to use a more accurate message like 'Connection
closed' and include the close event details (use the close event parameters code
and reason) in the rejection/log so you can tell server-initiated closes from
client disconnects; ensure you still clear pending.timeouts and
this.pendingRequests and set this.ws = null while using pending.reject(new
Error(`Connection closed${code ? ` (code=${code})` : ''}${reason ? `: ${reason}`
: ''}`)).
In `@src/main/index.ts`:
- Around line 405-413: The comment "via live:startServer IPC call from the
renderer" is now orphaned after the auto-start of the CLI server; update or
remove it to avoid confusion. Edit the block around the ensureCliServer call
(references: ensureCliServer, getWebServer, setWebServer, createWebServer,
webServer) and either delete that trailing comment or replace it with a brief
note explaining that ensureCliServer auto-starts the CLI server and writes the
discovery file for CLI IPC.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 773-780: The config is casting optional fields from message
without type checks (config: documents, prompt, loopEnabled, maxLoops,
saveAsPlaybook, launch), so validate and coerce each optional field at the
WebSocket boundary before building config: check message.loopEnabled is a
boolean (or parse "true"/"false"), ensure message.maxLoops is a finite number
(parseInt/Number and clamp/NaN-check), verify saveAsPlaybook is a non-empty
string when present, and ensure launch is boolean; if a value is invalid, either
reject the message with a clear error or omit the field so downstream code
receives correctly typed values.
In `@src/renderer/global.d.ts`:
- Around line 315-317: The onRemoteConfigureAutoRun callback currently types its
config parameter as any; change it to the concrete type used by the backend (the
config shape from ConfigureAutoRunCallback) by importing or re-exporting that
type and replacing the `config: any` signature in onRemoteConfigureAutoRun with
the specific type (e.g., `config: ConfigureAutoRunConfig` or the actual
ConfigureAutoRunCallback's config type), and update the declaration file to
include the necessary type import or a matching type alias so the renderer types
align with ConfigureAutoRunCallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc526379-b1f1-43ea-96be-d8829a9139f9
📒 Files selected for processing (36)
src/__tests__/cli/commands/auto-run.test.tssrc/__tests__/cli/commands/open-file.test.tssrc/__tests__/cli/services/maestro-client.test.tssrc/__tests__/main/ipc/handlers/system.test.tssrc/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/main/web-server/managers/CallbackRegistry.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/__tests__/shared/cli-server-discovery.test.tssrc/cli/commands/auto-run.tssrc/cli/commands/open-file.tssrc/cli/commands/refresh-auto-run.tssrc/cli/commands/refresh-files.tssrc/cli/commands/send.tssrc/cli/commands/status.tssrc/cli/index.tssrc/cli/services/maestro-client.tssrc/main/app-lifecycle/quit-handler.tssrc/main/index.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/system.tssrc/main/ipc/handlers/web.tssrc/main/preload/process.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/prompts/maestro-system-prompt.mdsrc/renderer/App.tsxsrc/renderer/global.d.tssrc/renderer/hooks/git/useFileExplorerEffects.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/shared/cli-server-discovery.ts
Comment on lines
+351
to
+353
| setGroupChats((prev) => | ||
| prev.map((c) => (c.id === id ? { ...c, moderatorSessionId } : c)) | ||
| ); |
There was a problem hiding this comment.
Critical: Inconsistent property name causes state corruption.
Line 352 sets moderatorSessionId, but line 209 (in the onModeratorSessionIdChanged listener) sets moderatorAgentSessionId for the same conceptual value. This creates two different properties on the chat object, causing state inconsistency where different code paths read/write different properties.
🐛 Proposed fix: use consistent property name
setGroupChats((prev) =>
- prev.map((c) => (c.id === id ? { ...c, moderatorSessionId } : c))
+ prev.map((c) => (c.id === id ? { ...c, moderatorAgentSessionId: moderatorSessionId } : c))
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setGroupChats((prev) => | |
| prev.map((c) => (c.id === id ? { ...c, moderatorSessionId } : c)) | |
| ); | |
| setGroupChats((prev) => | |
| prev.map((c) => (c.id === id ? { ...c, moderatorAgentSessionId: moderatorSessionId } : c)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 351 - 353,
The chat objects are being updated with two different keys (moderatorSessionId
vs moderatorAgentSessionId) causing state divergence; update the setter inside
setGroupChats (the map callback that currently assigns moderatorSessionId) to
use the same property name used by the onModeratorSessionIdChanged listener
(moderatorAgentSessionId) so all code paths read/write the same field, and
search for any other writes/reads to moderatorSessionId or
moderatorAgentSessionId to unify them to the chosen name across
useGroupChatHandlers (including the setGroupChats call and the
onModeratorSessionIdChanged handler).
Contributor
Author
There was a problem hiding this comment.
The GroupChatData type intentionally has two separate fields:
moderatorSessionId (line 165) - the Maestro internal session/agent ID, returned by startModerator
moderatorAgentSessionId (line 166) - the provider's session UUID (e.g., Claude Code's session ID), set asynchronously by the onModeratorSessionIdChanged event
Both are consumed separately in App.tsx (lines 3285-3289) and passed as distinct props to GroupChatRightPanel and GroupChatParticipants, which use them for different purposes (e.g., sessionId vs agentSessionId in the moderator participant object).
There was a problem hiding this comment.
@chr1syy, thanks for the detailed clarification! I misread this as a naming inconsistency — it's clear now that these are two intentionally distinct fields with different lifecycles and consumers. I'll make sure not to flag this in future reviews.
✏️ Learnings added
Learnt from: chr1syy
Repo: RunMaestro/Maestro PR: 530
File: src/renderer/hooks/groupChat/useGroupChatHandlers.ts:351-353
Timestamp: 2026-03-12T06:21:09.239Z
Learning: In `src/renderer/hooks/groupChat/useGroupChatHandlers.ts` (and the `GroupChatData` type), `moderatorSessionId` and `moderatorAgentSessionId` are two intentionally separate fields: `moderatorSessionId` is the Maestro internal session/agent ID returned synchronously by `startModerator`, while `moderatorAgentSessionId` is the provider-level session UUID (e.g., Claude Code's session ID) set asynchronously via the `onModeratorSessionIdChanged` IPC event. Both are consumed separately in App.tsx and passed as distinct props to `GroupChatRightPanel` and `GroupChatParticipants` for different purposes (sessionId vs agentSessionId in the moderator participant object). Do not flag writes to these two fields as a naming inconsistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: RunMaestro/Maestro PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T18:31:11.110Z
Learning: Use 'agent' in user-facing language to refer to entities in the Left Bar backed by a provider (Claude Code, Codex, etc.). Reserve 'session' for provider-level conversation contexts or when documenting the code interface (e.g., provider session_id).
Create src/shared/cli-server-discovery.ts with read/write/delete functions for the CLI server discovery file (cli-server.json). Follows the same pattern as cli-activity.ts with getConfigDir(), PID validation, and atomic writes (write to .tmp then rename). Exports: CliServerInfo interface, writeCliServerInfo, readCliServerInfo, deleteCliServerInfo, isCliServerRunning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add ensureCliServer() to web handlers that creates/starts the web server during app initialization and writes the discovery file (cli-server.json) so the CLI can locate and connect via WebSocket. The existing live:startServer handler continues to work — if the server is already running it returns the existing URL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stop Add deleteCliServerInfo() calls in three places: - quit-handler.ts performCleanup(): after web server stop, before stats DB close - web.ts live:stopServer handler: when server is manually stopped - web.ts live:disableAll handler: when all live sessions are disabled Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests cover writeCliServerInfo, readCliServerInfo, deleteCliServerInfo, and isCliServerRunning with mocked fs/os modules. Includes platform-specific config dir tests and validation edge cases (21 tests). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add new open_file_tab message handler across the web-server module: - messageHandlers.ts: switch case, handler method, callback interface - types.ts: OpenFileTabCallback type - CallbackRegistry.ts: storage, getter, setter - WebServer.ts: callback setter, message handler wiring - web-server-factory.ts: forward to renderer via remote:openFileTab IPC Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements the refresh_auto_run_docs message across the 5-file callback pattern: messageHandlers → types → CallbackRegistry → WebServer → factory. Also fixes pre-existing test mock missing setOpenFileTab/setRefreshFileTree callback methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vents Add onRemoteOpenFileTab, onRemoteRefreshFileTree, and onRemoteRefreshAutoRunDocs listeners in preload/process.ts and corresponding type declarations in global.d.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…file_tree, refresh_auto_run_docs Add three useEffect blocks in useRemoteIntegration.ts that subscribe to preload IPC events and dispatch CustomEvents. Add corresponding event listeners in App.tsx that handle file reading/opening, file tree refresh, and auto-run document refresh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ow foregrounding When focus: true is sent with a select_session message, the desktop window is brought to the foreground via mainWindow.show() and mainWindow.focus() before forwarding the session selection to the renderer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… refresh_file_tree, refresh_auto_run_docs, select_session focus) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds -t/--tab option to the CLI send command that, after a successful send, connects to the Maestro desktop app via WebSocket and focuses the agent's session tab. Gracefully warns if the desktop app is not running without failing the send itself. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds `maestro-cli status` command that checks if the Maestro desktop app is running and reachable via WebSocket. Reports port and session count on success, or clear error messages for common failure modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements the configure_auto_run message handler across the full WebSocket IPC stack: message handler, callback type, registry, WebServer, factory wiring, preload listener, and renderer type declarations. Uses response channel pattern for async results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add onRemoteConfigureAutoRun listener in useRemoteIntegration.ts that dispatches maestro:configureAutoRun CustomEvent, and handler in App.tsx that supports three modes: save as playbook, launch batch run, or configure-only. Uses response channel pattern for WebSocket confirmation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds `maestro-cli auto-run <docs...>` command that sends configure_auto_run messages over WebSocket IPC to the desktop app. Supports --prompt, --loop, --max-loops, --save-as, --launch, and --reset-on-completion options. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents maestro-cli commands (open-file, refresh-files, refresh-auto-run, auto-run, status) in the agent system prompt so AI agents know how to interact with the Maestro desktop app via CLI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bSocket handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix issues identified by Greptile and CodeRabbit reviews: - Send full absolute path instead of basename in auto-run documents - Fix WebSocket timeout race condition with settled flag - Include requestId in outgoing messages for correct response matching - Wrap test env var cleanup in try/finally with proper delete for undefined - Catch callback failures in onRemoteConfigureAutoRun preload handler - Validate document entries in messageHandlers before forwarding - Use randomUUID for collision-free response channels - Pass SSH remote context into file reads for open-file-tab - Fix refresh-auto-run to target the requested session - Return error for configure-only branch instead of silent no-op Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add missing onRemoteOpenFileTab, onRemoteRefreshFileTree, onRemoteRefreshAutoRunDocs, onRemoteConfigureAutoRun, and sendRemoteConfigureAutoRunResponse mocks to useRemoteIntegration test - Run prettier --write on all files flagged by CI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use Promise.resolve() wrapper instead of testing void return for truthiness, which TypeScript disallows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ents - Wrap process.kill mock/restore in try/finally in cli-server-discovery tests - Reject non-integer strings (e.g. "3foo") in --max-loops validation - Add WebSocket close handler to reject pending requests immediately - Await startBatchRun before sending success response, with error handling - Pass sshRemoteId to handleOpenFileTab for remote file context - Fix handleAutoRunRefresh racing with session switch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…and agent terminology - Validate documents array is non-empty before launching auto-run batch, returning an error response instead of proceeding with empty config - Add targetSessionId option to handleOpenFileTab so remote file-open events pin tabs to the originally requested session, avoiding races when the active session changes during async file reads - Update CLI error message from "sessions" to "agents" per terminology guidelines, with matching test update Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use "agent" instead of "session" in CLI status output - Add path traversal protection for open_file_tab handler - Echo requestId in all WebSocket handler responses for reliable matching - Validate configure_auto_run optional config fields at WebSocket boundary - Improve WebSocket close error message with code and reason - Type onRemoteConfigureAutoRun config parameter (replace any) - Add test assertions for file/auto-run callback registrations - Remove orphaned comment in main/index.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update test mock cwd to match test file paths - Fix callback failure test to use path within session root - Add explicit path traversal rejection test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix auto-run --launch timeout: send success response before starting the long-running batch operation instead of awaiting it (both IPC and CLI timeouts are 10s, startBatchRun exceeds that) - Run prettier on list-sessions source and test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chr1syy
force-pushed
the
symphony/issue-511-mmgpua7l
branch
from
March 12, 2026 17:42
a6df4b4 to
2b1ec23
Compare
6 tasks
This was referenced Apr 26, 2026
Merged
This was referenced May 21, 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
2 participants
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
send,status,open-file,refresh-files,refresh-auto-run,auto-run) for controlling Maestro sessions remotely via WebSocket IPCmaestro-client.ts) with server discovery (cli-server-discovery.ts) for CLI-to-Maestro communicationTest plan
maestro statusreports connectivity diagnostics correctlymaestro send --tabfocuses the correct session tabmaestro open-fileopens files in Maestromaestro refresh-filestriggers file tree refreshmaestro refresh-auto-runtriggers Auto Run document refreshmaestro auto-runconfigures auto-run sessionsnpm run testto confirm all new tests pass🤖 Generated with Claude Code
Created from #511
Summary by CodeRabbit
New Features
Tests
Documentation