Add CLI IPC commands for remote Maestro control#530
Open
chr1syy wants to merge 28 commits intoRunMaestro:mainfrom
Open
Add CLI IPC commands for remote Maestro control#530chr1syy wants to merge 28 commits intoRunMaestro:mainfrom
chr1syy wants to merge 28 commits intoRunMaestro:mainfrom
Conversation
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>
Implements MaestroClient class and withMaestroClient helper that connects to the running Maestro desktop app via the discovery file from Phase 01. Supports connect/disconnect lifecycle, typed command/response matching, and configurable timeouts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds `maestro-cli open-file <file-path> [--session <id>]` command that opens a file as a preview tab in the running Maestro desktop app via WebSocket IPC. Also extracts resolveSessionId() into maestro-client.ts as a shared helper for commands that need session resolution with fallback to first available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
📝 WalkthroughWalkthroughAdds a CLI↔Electron bridge: CLI server discovery, a WebSocket MaestroClient, five CLI commands (open-file, refresh-files, refresh-auto-run, auto-run, status), new WebSocket message types and callbacks, IPC/renderer handlers for remote operations, and extensive tests across CLI, WebSocket, web-server, renderer, and discovery code. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant MC as MaestroClient
participant WS as WebSocket
participant Handler as WebSocket 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 event
CLI->>MC: sendCommand(configure_auto_run)
MC->>WS: send JSON message
WS->>Handler: receive message
Handler->>CB: call configureAutoRun(sessionId, config)
CB->>Main: emit remote:configureAutoRun (responseChannel)
Main->>Renderer: IPC -> handle 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 with result
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 WebSocket Handler
participant CB as CallbackRegistry
participant Main as Main (IPC)
participant Renderer as Renderer
CLI->>MC: sendCommand(open_file_tab)
MC->>WS: send JSON message
WS->>Handler: handleOpenFileTab
Handler->>CB: openFileTab(sessionId, filePath)
CB->>Main: emit remote:openFileTab(sessionId, filePath)
Main->>Renderer: IPC -> open file tab (reads file, opens tab)
Renderer-->>Main: success boolean
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 issues
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)
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 for remotely controlling Maestro sessions via WebSocket — adding six new CLI commands ( The architecture is sound: the discovery file approach cleanly decouples the CLI from the running app, the atomic write+rename in Key issues to address before merging:
Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant CLI as maestro-cli
participant Disc as cli-server.json
participant WS as WebSocket (MaestroClient)
participant Main as Electron Main
participant IPC as ipcMain
participant Renderer as Renderer (App.tsx)
Note over Main,Disc: On app start (ensureCliServer)
Main->>WS: webServer.start()
Main->>Disc: writeCliServerInfo({port, token, pid})
Note over CLI,Renderer: CLI command (e.g. auto-run --launch)
CLI->>Disc: readCliServerInfo()
Disc-->>CLI: {port, token, pid}
CLI->>CLI: isCliServerRunning() (kill -0 pid)
CLI->>WS: connect ws://localhost:{port}/{token}/ws
WS-->>CLI: open event
CLI->>WS: sendCommand({type, sessionId, ...docs, requestId})
WS->>Main: WebSocket message
Main->>Main: messageHandlers.handleMessage()
Main->>IPC: webContents.send('remote:configureAutoRun', sessionId, config, responseChannel)
IPC->>Renderer: IPC event
Renderer->>Renderer: dispatchEvent('maestro:configureAutoRun')
Renderer->>Renderer: startBatchRun / playbooks.create
Renderer->>IPC: ipcRenderer.send(responseChannel, result)
IPC->>Main: ipcMain.once(responseChannel)
Main->>WS: send({type:'configure_auto_run_result', success, ...})
WS-->>CLI: message event → resolve sendCommand
CLI->>CLI: print result / process.exit
Note over Main,Disc: On app quit
Main->>Disc: deleteCliServerInfo()
Last reviewed commit: 6c1a30f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.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.
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>
Comment on lines
+43
to
+46
| const documents = resolvedPaths.map((d) => ({ | ||
| filename: d, | ||
| resetOnCompletion: options.resetOnCompletion || false, | ||
| })); |
There was a problem hiding this comment.
Full absolute path sent as filename, but renderer expects bare name
filename: d stores the full absolute path (e.g. /Users/alice/project/docs/my-doc.md). In App.tsx the renderer's launch branch processes it as:
filename: doc.filename.replace(/\.md$/, '')
// → "/Users/alice/project/docs/my-doc" (still a full path)startBatchRun then resolves this against session.autoRunFolderPath, producing a path like /auto-run-folder//Users/alice/project/docs/my-doc.md which will never be found. All other callers of startBatchRun (e.g. useWizardHandlers.ts:1210, useSymphonyContribution.ts:241) pass a bare name without extension.
The fix is to send only the basename without extension (matching what the renderer expects), rather than the full path:
Suggested change
| const documents = resolvedPaths.map((d) => ({ | |
| filename: d, | |
| resetOnCompletion: options.resetOnCompletion || false, | |
| })); | |
| const documents = resolvedPaths.map((d) => ({ | |
| filename: path.basename(d, '.md'), | |
| resetOnCompletion: options.resetOnCompletion || false, | |
| })); |
If the CLI must also communicate the full path for the renderer to locate the file, a separate filePath field would need to be added to both the protocol and the renderer's handler.
Comment on lines
+1762
to
+1770
| if (config.launch) { | ||
| const folderPath = session.autoRunFolderPath; | ||
| if (!folderPath) { | ||
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | ||
| success: false, | ||
| error: 'No Auto Run folder configured for this session', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
"Configure only" path is unreachable — auto-run without --launch or --save-as always errors
Case 3 deliberately returns success: false, but auto-run.ts has a success branch for the "just configure" case (the else block that prints "Auto-run configured with N documents"). That branch is dead code because the renderer will always reject a bare configure_auto_run without launch or saveAsPlaybook.
Running maestro auto-run doc.md (no --launch, no --save-as) will always exit with code 1 and print the error message "Use --launch to start auto-run immediately, or --save-as to save as a playbook", contradicting the command's description ("Configure and optionally launch").
If "configure only" is not a supported mode, the CLI command should validate up-front and require --launch or --save-as, and the else branch in auto-run.ts should be removed. If it is intended to be supported, the renderer needs to implement a way to persist the configuration.
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
+329
to
+357
| describe('isCliServerRunning', () => { | ||
| it('should return true for current PID', () => { | ||
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | ||
|
|
||
| const originalKill = process.kill; | ||
| process.kill = vi.fn().mockReturnValue(true) as unknown as typeof process.kill; | ||
|
|
||
| const result = isCliServerRunning(); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(process.kill).toHaveBeenCalledWith(12345, 0); | ||
|
|
||
| process.kill = originalKill; | ||
| }); | ||
|
|
||
| it('should return false for non-existent PID', () => { | ||
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | ||
|
|
||
| const originalKill = process.kill; | ||
| process.kill = vi.fn().mockImplementation(() => { | ||
| throw new Error('ESRCH: No such process'); | ||
| }) as unknown as typeof process.kill; | ||
|
|
||
| const result = isCliServerRunning(); | ||
|
|
||
| expect(result).toBe(false); | ||
|
|
||
| process.kill = originalKill; | ||
| }); |
There was a problem hiding this comment.
Wrap process.kill restoration in finally block.
The same cleanup issue previously flagged for env vars applies here. If an assertion fails before line 341 or 356, process.kill is left mocked, potentially corrupting subsequent tests or the test runner.
🛡️ Proposed fix using try/finally
it('should return true for current PID', () => {
mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo));
const originalKill = process.kill;
process.kill = vi.fn().mockReturnValue(true) as unknown as typeof process.kill;
- const result = isCliServerRunning();
-
- expect(result).toBe(true);
- expect(process.kill).toHaveBeenCalledWith(12345, 0);
-
- process.kill = originalKill;
+ try {
+ const result = isCliServerRunning();
+
+ expect(result).toBe(true);
+ expect(process.kill).toHaveBeenCalledWith(12345, 0);
+ } finally {
+ process.kill = originalKill;
+ }
});
it('should return false for non-existent PID', () => {
mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo));
const originalKill = process.kill;
process.kill = vi.fn().mockImplementation(() => {
throw new Error('ESRCH: No such process');
}) as unknown as typeof process.kill;
- const result = isCliServerRunning();
-
- expect(result).toBe(false);
-
- process.kill = originalKill;
+ try {
+ const result = isCliServerRunning();
+
+ expect(result).toBe(false);
+ } finally {
+ process.kill = originalKill;
+ }
});📝 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
| describe('isCliServerRunning', () => { | |
| it('should return true for current PID', () => { | |
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | |
| const originalKill = process.kill; | |
| process.kill = vi.fn().mockReturnValue(true) as unknown as typeof process.kill; | |
| const result = isCliServerRunning(); | |
| expect(result).toBe(true); | |
| expect(process.kill).toHaveBeenCalledWith(12345, 0); | |
| process.kill = originalKill; | |
| }); | |
| it('should return false for non-existent PID', () => { | |
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | |
| const originalKill = process.kill; | |
| process.kill = vi.fn().mockImplementation(() => { | |
| throw new Error('ESRCH: No such process'); | |
| }) as unknown as typeof process.kill; | |
| const result = isCliServerRunning(); | |
| expect(result).toBe(false); | |
| process.kill = originalKill; | |
| }); | |
| describe('isCliServerRunning', () => { | |
| it('should return true for current PID', () => { | |
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | |
| const originalKill = process.kill; | |
| process.kill = vi.fn().mockReturnValue(true) as unknown as typeof process.kill; | |
| try { | |
| const result = isCliServerRunning(); | |
| expect(result).toBe(true); | |
| expect(process.kill).toHaveBeenCalledWith(12345, 0); | |
| } finally { | |
| process.kill = originalKill; | |
| } | |
| }); | |
| it('should return false for non-existent PID', () => { | |
| mockFs.readFileSync.mockReturnValue(JSON.stringify(sampleInfo)); | |
| const originalKill = process.kill; | |
| process.kill = vi.fn().mockImplementation(() => { | |
| throw new Error('ESRCH: No such process'); | |
| }) as unknown as typeof process.kill; | |
| try { | |
| const result = isCliServerRunning(); | |
| expect(result).toBe(false); | |
| } finally { | |
| process.kill = originalKill; | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/shared/cli-server-discovery.test.ts` around lines 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.
Comment on lines
+48
to
+54
| const loopEnabled = options.loop || options.maxLoops !== undefined; | ||
| const maxLoops = options.maxLoops !== undefined ? parseInt(options.maxLoops, 10) : undefined; | ||
|
|
||
| if (maxLoops !== undefined && (isNaN(maxLoops) || maxLoops < 1)) { | ||
| console.error('Error: --max-loops must be a positive integer'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Reject partially numeric --max-loops values.
parseInt('3foo', 10) returns 3, so malformed input currently passes validation. Validate the entire string before converting, or switch to Number() plus Number.isInteger().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/auto-run.ts` around lines 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'.
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.
Comment on lines
+63
to
+68
| ws.on('error', (err) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| reject(new Error(`Failed to connect to Maestro: ${err.message}`)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that MaestroClient handles open/error but not post-connect close, and that
# pending requests are only rejected on explicit disconnect().
rg -n "ws\\.on\\('open'|ws\\.on\\('error'|ws\\.on\\('close'|pendingRequests|Client disconnected" src/cli/services/maestro-client.tsRepository: RunMaestro/Maestro
Length of output: 736
🏁 Script executed:
cat -n src/cli/services/maestro-client.ts | head -160Repository: RunMaestro/Maestro
Length of output: 5353
Add a close handler in setupMessageHandler() to fail pending requests immediately when the socket closes.
After a successful connection, setupMessageHandler() only registers a message handler. If Maestro closes the connection mid-command, pending requests hang until the 10-second command timeout instead of failing immediately. Add a close handler that rejects all pending requests (similar to how disconnect() does at lines 107–111).
🤖 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 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.
Comment on lines
+1682
to
+1694
| const [content, stat] = await Promise.all([ | ||
| window.maestro.fs.readFile(filePath, sshRemoteId), | ||
| window.maestro.fs.stat(filePath, sshRemoteId).catch(() => null), | ||
| ]); | ||
| if (content !== null) { | ||
| const filename = filePath.split(/[\\/]/).pop() || filePath; | ||
| const lastModified = stat?.modifiedAt ? new Date(stat.modifiedAt).getTime() : undefined; | ||
| handleOpenFileTab({ | ||
| path: filePath, | ||
| name: filename, | ||
| content, | ||
| lastModified, | ||
| }); |
There was a problem hiding this comment.
Carry sshRemoteId into the created file tab.
This read path uses the remote FS, but the tab payload drops sshRemoteId. On SSH-backed sessions, later reload/save flows will lose the remote context even though the existing DocumentGraph open path in this file preserves it.
🔧 Proposed fix
handleOpenFileTab({
path: filePath,
name: filename,
content,
+ sshRemoteId,
lastModified,
});📝 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
| const [content, stat] = await Promise.all([ | |
| window.maestro.fs.readFile(filePath, sshRemoteId), | |
| window.maestro.fs.stat(filePath, sshRemoteId).catch(() => null), | |
| ]); | |
| if (content !== null) { | |
| const filename = filePath.split(/[\\/]/).pop() || filePath; | |
| const lastModified = stat?.modifiedAt ? new Date(stat.modifiedAt).getTime() : undefined; | |
| handleOpenFileTab({ | |
| path: filePath, | |
| name: filename, | |
| content, | |
| lastModified, | |
| }); | |
| const [content, stat] = await Promise.all([ | |
| window.maestro.fs.readFile(filePath, sshRemoteId), | |
| window.maestro.fs.stat(filePath, sshRemoteId).catch(() => null), | |
| ]); | |
| if (content !== null) { | |
| const filename = filePath.split(/[\\/]/).pop() || filePath; | |
| const lastModified = stat?.modifiedAt ? new Date(stat.modifiedAt).getTime() : undefined; | |
| handleOpenFileTab({ | |
| path: filePath, | |
| name: filename, | |
| content, | |
| sshRemoteId, | |
| lastModified, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/App.tsx` around lines 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.
Comment on lines
+1786
to
+1789
| startBatchRun(sessionId, batchConfig, folderPath); | ||
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | ||
| success: true, | ||
| }); |
There was a problem hiding this comment.
Wait for batch startup before returning success.
startBatchRun is async, but this branch sends { success: true } before startup finishes. If launch fails, the CLI gets a false positive and the catch path never reports the real error.
🔧 Proposed fix
- startBatchRun(sessionId, batchConfig, folderPath);
+ await startBatchRun(sessionId, batchConfig, folderPath);
window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, {
success: true,
});📝 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
| startBatchRun(sessionId, batchConfig, folderPath); | |
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | |
| success: true, | |
| }); | |
| startBatchRun(sessionId, batchConfig, folderPath); | |
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | |
| success: true, | |
| }); |
Suggested change
| startBatchRun(sessionId, batchConfig, folderPath); | |
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | |
| success: true, | |
| }); | |
| await startBatchRun(sessionId, batchConfig, folderPath); | |
| window.maestro.process.sendRemoteConfigureAutoRunResponse(responseChannel, { | |
| success: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/App.tsx` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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
Release Notes
New Features
Tests