[WIP] Symphony: CLI-IPC: WebSocket bridge for provider agents to trigger Maestro UI actions (#511)#1033
[WIP] Symphony: CLI-IPC: WebSocket bridge for provider agents to trigger Maestro UI actions (#511)#1033DRAZY wants to merge 24 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughImplements a CLI↔Electron WebSocket IPC bridge: discovery file persistence, MaestroClient, new CLI commands (auto-run, open-file, refresh-files, refresh-auto-run, status, send --tab), WebSocket message types and handlers, preload/renderer remote handlers, UI wiring, and extensive tests. ChangesCLI‑IPC Bridge
Sequence Diagram(s)sequenceDiagram
participant CLI
participant MaestroClient
participant WebServer
participant CallbackRegistry
participant Renderer
CLI->>MaestroClient: connect() + sendCommand(configure_auto_run/open_file_tab/refresh...)
MaestroClient->>WebServer: WebSocket message (requestId, type, payload)
WebServer->>CallbackRegistry: route to handler (openFileTab/refreshFileTree/configureAutoRun)
CallbackRegistry->>Renderer: invoke callback -> dispatch CustomEvent (maestro:*)
Renderer->>CLI: (optional) sendRemoteConfigureAutoRunResponse via IPC channel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Symphony Contribution SummaryThis pull request was created using Maestro Symphony - connecting AI-powered contributors with open source projects. Contribution Stats
Powered by Maestro • Learn about Symphony |
Greptile SummaryThis PR introduces a WebSocket-based CLI↔desktop IPC bridge, letting provider agents trigger Maestro UI actions — open a file tab, refresh the file tree, refresh Auto Run docs, configure/launch Auto Run, and focus a session — directly from the command line. The web server is now auto-started at app launch and writes a discovery file so the CLI can connect without user interaction.
Confidence Score: 3/5The core IPC plumbing is sound, but the auto-run command's path handling silently substitutes a different file than the one the user specified, which could produce wrong results without any error. The src/cli/commands/auto-run.ts (path validation mismatch) and src/main/ipc/handlers/web.ts / src/main/web-server/web-server-factory.ts (CLI server lifetime tied to Live mode toggle). Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI Process
participant Disc as cli-server.json
participant WS as WebServer (WebSocket)
participant Main as Main Process (ipcMain)
participant Renderer as Renderer (React)
Note over Main: App startup
Main->>WS: ensureCliServer()
WS-->>Disc: writeCliServerInfo(port, token, pid)
CLI->>Disc: readCliServerInfo()
Disc-->>CLI: "{port, token}"
CLI->>WS: "WebSocket connect ws://localhost:{port}/{token}/ws"
CLI->>WS: "{type: configure_auto_run, sessionId, documents}"
WS->>Main: configureAutoRunCallback(sessionId, config)
Main->>Renderer: webContents.send(remote:configureAutoRun, sessionId, config, responseChannel)
Renderer->>Main: ipcRenderer.send(responseChannel, result)
Main-->>WS: resolve(result)
WS-->>CLI: "{type: configure_auto_run_result, success, playbookId}"
Note over Main: App quit or Live mode stop
Main->>Disc: deleteCliServerInfo()
Reviews (1): Last reviewed commit: "MAESTRO: Add auto-run CLI IPC tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const documentPaths = documentPathArgs.map(resolveMarkdownDocument); | ||
| const maxLoops = parseMaxLoops(options.maxLoops); | ||
| const sessionId = resolveSessionId(options); | ||
| const documents = documentPaths.map((documentPath) => ({ | ||
| filename: path.basename(documentPath), | ||
| resetOnCompletion: options.resetOnCompletion || false, | ||
| })); |
There was a problem hiding this comment.
resolveMarkdownDocument resolves and validates the full absolute path passed by the caller, but path.basename() strips everything except the filename before sending it to the desktop. The desktop handler in App.tsx then reads the document from session.autoRunFolderPath + "/" + filename, not from the path the user provided. If the user runs maestro auto-run /some/other/path/spec.md, the CLI validates that /some/other/path/spec.md exists, but the desktop silently reads <autoRunFolderPath>/spec.md — a potentially different file (or one that doesn't exist in the auto-run folder at all). The validated path is entirely unused after validation.
| try { | ||
| logger.info('Stopping web server', 'WebServer'); | ||
| await webServer.stop(); | ||
| deleteCliServerInfo(); |
There was a problem hiding this comment.
ensureCliServer is called at app startup to guarantee CLI IPC is always available. However, both the live:stopServer and live:disableAllSessions handlers call deleteCliServerInfo() and stop the same server instance. If a user disables the Live feature from the UI, the CLI discovery file is deleted and the server is stopped, silently breaking all CLI IPC commands until the app restarts. Since the CLI server is now a first-class always-on service, it should either be started on a separate port/instance or the stop paths should guard against removing the CLI server's discovery info.
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| if (message.type === 'error') { | ||
| this.rejectMatchingOrAll(message); | ||
| return; | ||
| } | ||
|
|
||
| for (const [requestId, pending] of this.pendingRequests) { | ||
| if (message.type !== pending.responseType) continue; | ||
| if (message.requestId && message.requestId !== requestId) continue; | ||
|
|
||
| clearTimeout(pending.timeout); | ||
| this.pendingRequests.delete(requestId); | ||
|
|
||
| if (message.success === false) { | ||
| pending.reject(new Error(this.getErrorMessage(message))); |
There was a problem hiding this comment.
When the server sends a response that omits requestId, the loop matches the first pending entry whose responseType equals message.type, regardless of which request actually triggered it. If two sendCommand calls with the same expected response type are in-flight simultaneously, the earlier one in the map could be resolved by the response intended for the later one. In current usage withMaestroClient creates a fresh client per action, but the MaestroClient class exposes sendCommand publicly, making this a latent bug for callers who issue multiple concurrent commands on one client instance.
| import { useTabStore } from './stores/tabStore'; | ||
| import { useFileExplorerStore } from './stores/fileExplorerStore'; | ||
|
|
||
| type RemoteConfigureAutoRunConfig = { | ||
| documents: Array<{ filename: string; resetOnCompletion?: boolean }>; | ||
| prompt?: string; | ||
| loopEnabled?: boolean; | ||
| maxLoops?: number; | ||
| saveAsPlaybook?: string; | ||
| launch?: boolean; | ||
| }; | ||
|
|
||
| type RemoteConfigureAutoRunResult = { | ||
| success: boolean; | ||
| playbookId?: string; | ||
| error?: string; |
There was a problem hiding this comment.
ConfigureAutoRunConfig/Result types duplicated across four files
Identical type shapes are declared independently in App.tsx, useRemoteIntegration.ts, renderer/global.d.ts, and main/preload/process.ts (with a canonical version already in main/web-server/types.ts). Any field addition requires four separate edits. These could be consolidated to imports from the canonical source.
| return new Promise((resolve) => { | ||
| const responseChannel = `remote:configureAutoRun:response:${Date.now()}`; | ||
| let resolved = false; |
There was a problem hiding this comment.
Date.now() is not guaranteed to be unique — two concurrent configure_auto_run WebSocket messages arriving within the same millisecond would produce identical channel names, causing ipcMain.once listeners to race and cross-resolve each other's responses. randomUUID() (already used elsewhere in this repo) gives collision-free channel names.
| return new Promise((resolve) => { | |
| const responseChannel = `remote:configureAutoRun:response:${Date.now()}`; | |
| let resolved = false; | |
| return new Promise((resolve) => { | |
| const responseChannel = `remote:configureAutoRun:response:${crypto.randomUUID()}`; | |
| let resolved = false; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/cli/commands/send.ts (1)
122-125: 💤 Low valueUnreachable code after
process.exit(1).The
returnstatement on line 124 is unreachable sinceprocess.exit(1)terminates the process immediately.♻️ Remove unreachable return
if (!result.success) { process.exit(1); - return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/send.ts` around lines 122 - 125, The code checks result.success and calls process.exit(1) on failure, but the subsequent return; after process.exit(1) is unreachable — remove the redundant return statement in the failure branch (the block that tests result.success) inside send.ts (where result.success is evaluated) so the function is clean; if you intended to avoid exiting the whole process, replace process.exit(1) with a thrown error or an early return instead of keeping both.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/auto-run.ts`:
- Around line 19-24: The ConfigureAutoRunResult type currently makes success
optional which lets malformed responses be treated as success; update the
interface ConfigureAutoRunResult so success is required (remove the ? from
success) and then change the runtime check in the auto-run command (where it
currently tests success === false around the configure_auto_run response
handling) to a falsy check (e.g., if (!result.success) { ... }) so any missing
or false value is treated as a failure; update any related branches that rely on
optional success to use the required boolean instead.
In `@src/cli/services/maestro-client.ts`:
- Around line 132-158: The handleMessage currently swallows malformed JSON and
can match responses by responseType alone, risking misrouting; change it to (1)
surface JSON parse errors instead of silently returning—catch JSON.parse errors
and re-throw or pass them to the global error telemetry (so Sentry can capture
them) while preserving the function signature, and (2) match incoming messages
to pending requests by requestId first (use message.requestId to look up the
specific pending entry in pendingRequests) and only fall back to strict
responseType matching when no requestId is provided, ensuring you only
clearTimeout, delete, resolve, or reject the exact pending request; reference
handleMessage, pendingRequests, rejectMatchingOrAll, and getErrorMessage when
making the changes.
In `@src/main/ipc/handlers/web.ts`:
- Around line 55-77: The ensureCliServer function always overwrites startedAt
with Date.now() when calling writeCliServerInfo even if the server was already
running; update the logic so startedAt reflects the real server start time by
either moving the writeCliServerInfo call inside the if (!webServer.isActive())
block so it only writes when webServer.start() is executed, or by querying the
WebServer instance for its original start time (e.g., add/use a method like
webServer.getStartTime() or webServer.getStartedAt() and pass that value to
writeCliServerInfo) while keeping other fields (getPort/getSecurityToken/pid)
unchanged.
- Around line 71-76: The discovery writer writeCliServerInfo currently writes a
temp file and renames it to cli-server.json without enforcing permissions;
update writeCliServerInfo to create the temp file with mode 0o600 (use
fs.writeFile with { mode: 0o600 } or fs.open+fs.write), after rename explicitly
chmod the final cli-server.json to 0o600 (fs.chmod) and also chmod the temp file
to 0o600 before renaming to be safe; additionally check the containing config
directory permissions and, if group/world bits are set, chmod the directory to
0o700 so the token file cannot be read by other users.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 771-779: The current check in messageHandlers.ts (around the
configure_auto_run handling) only ensures documents is a non-empty array but
doesn't validate each item's shape, so malformed items (e.g., missing or
non-string filename) can slip through; update the validation in the handler that
sends the 'configure_auto_run_result' response to iterate over documents and
assert each is an object with required properties (at minimum a string
'filename' and any other expected fields), returning a failure response with an
explanatory error if any item is invalid, and apply the same strict per-item
validation to the other configure_auto_run forwarding block referenced around
lines 789-796 so downstream handlers receive well-formed document entries.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 581-582: The responseChannel currently uses Date.now() which can
collide for concurrent configureAutoRun requests; update the creation of
responseChannel (the variable named responseChannel in the configureAutoRun
handling code) to append a collision-safe unique id (e.g., a UUID or
crypto.randomUUID()) instead of Date.now(); ensure you add the necessary import
or use the built-in crypto API and keep the same prefix
`remote:configureAutoRun:response:` so the rest of the handler
(listeners/cleanup logic tied to responseChannel) continues to work unchanged.
In `@src/renderer/App.tsx`:
- Around line 1863-1980: handleRemoteConfigureAutoRun currently can throw and
never send a response; wrap the entire function body in a top-level try/catch so
every code path calls sendResponse and unexpected errors are handled: inside the
catch, call the Sentry utility (captureException) with the caught error,
sendResponse({ success: false, error: error instanceof Error ? error.message :
String(error) }), and then re-throw the error to preserve upstream reporting;
ensure this covers calls like window.maestro.autorun.readDoc,
startBatchRunRef.current(...), and all mapping/access code before returning or
updating state (setSessions, setAutoRunDocumentList, setRemoteBatchRunConfig,
etc.).
- Around line 1839-1841: Replace the console.error-only catch in the remote
file-open flow with explicit Sentry reporting using captureException (and
captureMessage if helpful) from src/utils/sentry.ts, include contextual metadata
(e.g., action: "openRemoteFile", file identifier/path, userId or session if
available), remove the console.error call, and then surface a recoverable result
instead of swallowing (for example set an error state / call the existing UI
notification handler or rethrow the error) so the UI can show a user-facing
failure; reference the catch block in App.tsx and use
captureException/captureMessage to report the error with context.
In `@src/shared/cli-server-discovery.ts`:
- Around line 50-57: The numeric validation for info.port, info.pid (and similar
checks at the other occurrence) is too lax; replace Number.isFinite checks with
stricter integer-and-range checks: ensure info.port is Number.isInteger and
between 1 and 65535, ensure info.pid is Number.isInteger and > 0 (no zero or
negatives), and ensure info.startedAt is a Number.isInteger and non-negative (or
a sensible timestamp lower bound); then only run process liveness checks (e.g.,
process.kill(info.pid, 0)) after these stricter validations so you never call
kill with pid 0 or an invalid value.
- Around line 72-73: The discovery file is written without explicit file
permissions, which can leave the IPC auth token world-readable; when creating
the temp file (tmpPath) before renaming to filePath, ensure it is created with
restrictive permissions (e.g., 0o600) by passing a mode option to
fs.writeFileSync or immediately calling fs.chmodSync(tmpPath, 0o600) after
writing and before fs.renameSync, so the written JSON (info) is only
readable/writable by the owner.
---
Nitpick comments:
In `@src/cli/commands/send.ts`:
- Around line 122-125: The code checks result.success and calls process.exit(1)
on failure, but the subsequent return; after process.exit(1) is unreachable —
remove the redundant return statement in the failure branch (the block that
tests result.success) inside send.ts (where result.success is evaluated) so the
function is clean; if you intended to avoid exiting the whole process, replace
process.exit(1) with a thrown error or an early return instead of keeping both.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b2c7d2e-05dd-42a3-8a70-cafbc8106ece
📒 Files selected for processing (40)
src/__tests__/cli/commands/auto-run.test.tssrc/__tests__/cli/commands/open-file.test.tssrc/__tests__/cli/commands/refresh-auto-run.test.tssrc/__tests__/cli/commands/refresh-files.test.tssrc/__tests__/cli/commands/send.test.tssrc/__tests__/cli/commands/status.test.tssrc/__tests__/cli/services/maestro-client-session.test.tssrc/__tests__/cli/services/maestro-client.test.tssrc/__tests__/main/app-lifecycle/quit-handler.test.tssrc/__tests__/main/ipc/handlers/web.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/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/components/AppModals.tsxsrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/shared/cli-server-discovery.ts
1 participant
Maestro Symphony Contribution
Closes #511
Contributed via Maestro Symphony.
Status: In Progress
Started: 2026-05-22T00:40:48.300Z
This PR will be updated automatically when the Auto Run completes.
Summary by CodeRabbit
New Features
Tests