fix(cli): persist CLI-created agent to disk before responding#1014
Conversation
The `maestro:remoteCreateSession` handler relied on the renderer's 2-second debounced persistence to flush the new session, so a CLI consumer that ran `create-agent` and immediately followed up with `list agents` / `send` / `show agent` hit the disk-backed CLI storage before the flush fired, surfacing as AGENT_NOT_FOUND. Force an immediate `sessions:setMany` write before sending the create_session_result, so the CLI contract "create returned success → the agent is on disk" holds. The subsequent debounced flush is idempotent — it just rewrites the same row. Closes #1013
📝 WalkthroughWalkthroughThe change adds synchronous session persistence to the remote create-session IPC handler. After a new session object is created, the code now calls ChangesRemote Session Creation Persistence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer/hooks/remote/useAppRemoteEventListeners.ts`:
- Around line 1026-1030: The code currently logs setMany failures but still
treats the create path as successful; update the handler in
useAppRemoteEventListeners.ts so that when
window.maestro.sessions.setMany([newSession], []) throws, you call
Sentry.captureException(persistErr, { extra: { newSession } }) (or
captureMessage with context) and then re-throw or return a failed response so
the CLI sees failure instead of a false-positive; replace the current
logger.error(...) only behavior around window.maestro.sessions.setMany with
explicit Sentry capture and propagate the error (throw persistErr) so callers of
the create-agent path receive the failure.
🪄 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: 404fd203-5ca0-4083-9b88-d216cca06d04
📒 Files selected for processing (1)
src/renderer/hooks/remote/useAppRemoteEventListeners.ts
| try { | ||
| await window.maestro.sessions.setMany([newSession], []); | ||
| } catch (persistErr) { | ||
| logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr); | ||
| } |
There was a problem hiding this comment.
Persistence failure is treated as success (false-positive create result).
On Line 1026, a failed setMany is logged but the handler still returns success, so CLI callers can get create-agent success even when disk persistence failed. Please fail the response path (and capture to Sentry) when this write fails.
Proposed fix
try {
await window.maestro.sessions.setMany([newSession], []);
} catch (persistErr) {
+ captureException(persistErr, {
+ extra: {
+ event: 'maestro:remoteCreateSession',
+ sessionId: newId,
+ toolType,
+ cwd,
+ responseChannel,
+ },
+ });
logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr);
+ window.maestro.process.sendRemoteCreateSessionResponse(responseChannel, null);
+ return;
}As per coding guidelines: "Do not silently swallow errors... For unexpected errors, re-throw them... Use Sentry utilities (captureException, captureMessage) for explicit error reporting with context."
🤖 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/renderer/hooks/remote/useAppRemoteEventListeners.ts` around lines 1026 -
1030, The code currently logs setMany failures but still treats the create path
as successful; update the handler in useAppRemoteEventListeners.ts so that when
window.maestro.sessions.setMany([newSession], []) throws, you call
Sentry.captureException(persistErr, { extra: { newSession } }) (or
captureMessage with context) and then re-throw or return a failed response so
the CLI sees failure instead of a false-positive; replace the current
logger.error(...) only behavior around window.maestro.sessions.setMany with
explicit Sentry capture and propagate the error (throw persistErr) so callers of
the create-agent path receive the failure.
Greptile SummaryThis PR fixes a race condition where a CLI consumer calling
Confidence Score: 3/5The happy-path fix is correct, but the error path still sends a success response when the disk write fails, reintroducing the exact bug being fixed. When src/renderer/hooks/remote/useAppRemoteEventListeners.ts — specifically the inner catch block around the new Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as maestro-cli
participant Main as Electron Main
participant Renderer as Renderer (React)
participant Disk as Disk Storage
CLI->>Main: create-agent request
Main->>Renderer: maestro:remoteCreateSession event
Renderer->>Renderer: setSessions([...prev, newSession])
Note over Renderer: NEW: immediate persist before response
Renderer->>Disk: sessions.setMany([newSession], [])
alt setMany succeeds
Disk-->>Renderer: ok
Renderer->>Main: sendRemoteCreateSessionResponse(sessionId)
Main-->>CLI: "{ sessionId }"
CLI->>Main: list agents / send id
Main->>Disk: read sessions
Disk-->>Main: contains newSession
else setMany throws
Renderer->>Renderer: catch logger.error swallowed
Renderer->>Main: sendRemoteCreateSessionResponse(sessionId)
Main-->>CLI: false success sessionId
CLI->>Main: list agents / send id
Main->>Disk: read sessions
Disk-->>Main: AGENT_NOT_FOUND
end
Reviews (1): Last reviewed commit: "fix(cli): persist CLI-created agent to d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| await window.maestro.sessions.setMany([newSession], []); | ||
| } catch (persistErr) { | ||
| logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr); | ||
| } | ||
|
|
||
| window.maestro.process.sendRemoteCreateSessionResponse(responseChannel, { | ||
| sessionId: newId, | ||
| }); |
There was a problem hiding this comment.
When setMany throws, the error is caught and logged but execution falls through to sendRemoteCreateSessionResponse with the valid sessionId. The CLI receives a success signal, immediately queries disk, and hits AGENT_NOT_FOUND — the exact race this PR is trying to eliminate. Re-throwing (or calling sendRemoteCreateSessionResponse(responseChannel, null) in the catch) is needed so the outer catch block returns a failure response that the CLI can act on, rather than a success response backed by a write that never landed.
Context Used: CLAUDE.md (source)
| try { | ||
| await window.maestro.sessions.setMany([newSession], []); | ||
| } catch (persistErr) { | ||
| logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr); | ||
| } |
There was a problem hiding this comment.
The codebase's error-handling guideline (CLAUDE.md §Error Handling & Sentry) says unexpected errors should bubble up rather than be silently logged, so Sentry can capture them. A setMany failure is not a recoverable no-op — it means the contract "create returned success → agent is on disk" is broken. Re-throwing lets the outer catch send a proper failure response and keeps Sentry in the loop.
| try { | |
| await window.maestro.sessions.setMany([newSession], []); | |
| } catch (persistErr) { | |
| logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr); | |
| } | |
| try { | |
| await window.maestro.sessions.setMany([newSession], []); | |
| } catch (persistErr) { | |
| logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr); | |
| throw persistErr; | |
| } |
Context Used: CLAUDE.md (source)
chr1syy
left a comment
chr1syy
left a comment
There was a problem hiding this comment.
LGTM. Verified the fix end-to-end:
setManyon the preload bridge (src/main/preload/settings.ts:51-52) and its IPC handler (src/main/ipc/handlers/persistence.ts:173-290) are genuinely incremental — they merge into the existing on-disksessionsStorerather than overwriting, so the subsequent debounced flush rewrites the same row without churn, as the inline comment claims.- Broadcast suppression (
persistence.ts:226-241) means the debounced re-flush doesn't double-firebroadcastSessionStateChangefor web clients. awaitis correctly placed beforesendRemoteCreateSessionResponse, so the CLI's promise resolves only after disk write completes.
Non-blocking: the sibling remoteDeleteSession / remoteRenameSession handlers retain the same race
The issue reporter explicitly called this out (A unified flush helper in the IPC handler would close the class of bugs). Tracing the wiring:
remoteDeleteSession— the main-process callback insrc/main/web-server/web-server-factory.ts:1368-1381is documented// Fire-and-forget patternand returnstrueimmediately afterwebContents.send('remote:deleteSession', sessionId). The WSdelete_session_result: successreaches the CLI before the renderer has even runsetSessions((prev) => filtered), let alone the debounced disk flush. A CLIremove-agent <id>→list agentswill still see the agent for up to ~2s.remoteRenameSession— has a response channel and waits for the renderer'ssetSessions(...)call, but still doesn't wait for the disk flush. A CLIsession rename→show agentreturns the old name until the debounce fires.
Practical impact is much lower than create (those CLI sequences are rarer than create-agent → send), but the fix shape is the same ~10 lines per handler. I'll open a follow-up issue with the trace so you can scope it. Filing this here only so the link survives in the PR record.
One other small thing worth considering: the catch here logs but still calls sendRemoteCreateSessionResponse(..., {sessionId: newId}) with success. If setMany throws (e.g. ENOSPC) the CLI gets success and moves on. The setMany IPC handler itself returns false on ENOSPC at persistence.ts:274 rather than throwing — so this catch only fires on truly unexpected errors. Probably fine as-is; flagging only because the PR description promises "create returned success → it's on disk" and this is the one path where that contract leaks.
Not blocking. Ship it.
|
Quick correction on my own review above — I underweighted the silent-catch concern. Re-reading after looking at the CodeRabbit and Greptile inline comments, both bots independently flagged it as P1 with reference to CLAUDE.md §Error Handling & Sentry, and they're right:
I'd retract my "Not blocking. Ship it." line on that paragraph. Greptile's suggestion is the cleanest fix (~1 line) — let the unexpected error bubble to the existing outer try {
await window.maestro.sessions.setMany([newSession], []);
} catch (persistErr) {
logger.error('[Remote] Failed to persist new CLI-created session:', undefined, persistErr);
throw persistErr;
}That way: CLI sees failure, Sentry captures the exception via the existing Everything else in my review still stands — the core fix is correct and the delete/rename gap (tracked in #1054) is separate. |
2 participants
Summary
maestro:remoteCreateSessionhandler relied on the renderer's 2-second debounced persistence to flush the new agent. A CLI consumer that ranmaestro-cli create-agentand immediately followed up withlist agents/send/show agenthit the disk-backed CLI storage before the flush fired, surfacing asAGENT_NOT_FOUND.sessions:setManywrite before sending thecreate_session_result, so the CLI contract "create returned success → the agent is on disk" holds. The subsequent debounced flush is idempotent — it just rewrites the same row.Closes #1013
Test plan
maestro-cli create-agent "Test Agent" --cwd <path> --json→ agent shows in Left Barmaestro-cli list agents --json→ returns the new agent (previously returned[])maestro-cli send <id> "test"→ succeeds (previously returnedAGENT_NOT_FOUND)%APPDATA%/Maestro/maestro-sessions.json(or~/Library/Application Support/Maestro/maestro-sessions.json) → contains the agent immediately aftercreate-agentreturnsnpm run lintpassesnpx vitest run src/__tests__/renderer/hooks/utils/useDebouncedPersistence.test.ts src/__tests__/cli/commands/create-agent.test.ts src/__tests__/main/web-server/handlers/messageHandlers.test.tspassesSummary by CodeRabbit
Release Notes