feat(cli): add gist create to publish session transcripts to GitHub#898
feat(cli): add gist create to publish session transcripts to GitHub#898pedramamini merged 4 commits intoRunMaestro:rcfrom
gist create to publish session transcripts to GitHub#898Conversation
|
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 end-to-end gist creation: new CLI command, WebSocket message and handler, callback registration, preload IPC handlers, web-server-factory wiring to proxy to renderer, renderer logic to format logs and create gists, and associated tests. Changes
Sequence DiagramsequenceDiagram
participant CLI
participant Maestro as Maestro App
participant WebSocket as WebSocket Handler
participant Factory as Web Server Factory
participant Renderer
participant Git as Git Service
CLI->>Maestro: send create_gist(sessionId, description, isPublic)
Maestro->>WebSocket: WebSocket message type: create_gist
WebSocket->>Factory: invoke registered createGist(sessionId, description, isPublic)
Factory->>Renderer: ipc: remote:createGist(sessionId, description, isPublic, responseChannel)
Renderer->>Renderer: gather session, format logs to markdown, build filename
Renderer->>Git: createGist(filename, content, description, isPublic)
Git-->>Renderer: { success, gistUrl?, error? }
Renderer->>Factory: ipc: sendRemoteCreateGistResponse(responseChannel, result)
Factory-->>WebSocket: callback result { success, gistUrl?, error? }
WebSocket-->>Maestro: create_gist_result (requestId, result)
Maestro-->>CLI: deliver result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Adds a `maestro-cli gist create <agent-id>` subcommand that routes through the running Maestro desktop app over the existing WebSocket bridge and publishes every AI tab's transcript for the agent as a single GitHub gist. Reuses the existing `git:createGist` IPC handler (shells out to `gh gist create`), so the same GitHub CLI auth requirements apply as in the desktop "Publish Gist" flow. Supports `--description` and `--public`; defaults to private. Plumbing mirrors the summarize/transfer/merge-context remote pattern: `create_gist` WS message -> main-process callback -> `remote:createGist` IPC into the renderer, where `formatLogsForClipboard()` builds the markdown payload from the session's in-memory `aiTabs`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds The plumbing follows the existing Confidence Score: 5/5Safe to merge — all findings are P2 style/observability issues with no impact on correctness or the primary user path. The new gist feature is correctly wired end-to-end, follows established IPC patterns, includes tests for the new callback and preload hooks, and handles the main error cases (agent not found, no transcript, desktop not running, timeout). The three P2 comments cover a fragile string-match, a missing captureException call, and a minor error.message cast — none are blocking. No files require special attention before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as maestro-cli(gist.ts)
participant WS as WebSocketMessageHandler
participant CB as CallbackRegistry
participant Factory as web-server-factory.ts
participant IPC as ipcMain
participant Renderer as useRemoteIntegration
participant GH as gh gist create
CLI->>WS: WS: type=create_gist, sessionId, description, isPublic
WS->>CB: handleCreateGist -> callbacks.createGist(...)
CB->>Factory: createGist callback
Factory->>IPC: ipcMain.once(responseChannel)
Factory->>Renderer: webContents.send(remote:createGist, ...)
Renderer->>Renderer: find session, format aiTabs via formatLogsForClipboard()
Renderer->>GH: window.maestro.git.createGist(filename, content, desc, isPublic)
GH-->>Renderer: { success, gistUrl }
Renderer->>IPC: ipcRenderer.send(responseChannel, result)
IPC-->>Factory: handleResponse fires, clearTimeout
Factory-->>CB: resolve { success, gistUrl }
CB-->>WS: Promise resolves
WS-->>CLI: WS: type=create_gist_result, success, gistUrl
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| window.maestro.process.sendRemoteCreateGistResponse(responseChannel, { | ||
| success: false, | ||
| error: message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The catch block responds to the caller but doesn't call captureException for unexpected errors (e.g. window.maestro.git.createGist throws for an unforeseen reason). The cue subscription handler immediately above in this file (remoteTriggerCueSubscription) calls captureException(error, { extra: { context: '...' } }) for the same pattern. Aligning here ensures observability parity across all remote-action handlers.
|
|
||
| this.callbacks | ||
| .createGist(sessionId, description, isPublic) |
There was a problem hiding this comment.
error.message access without instanceof guard
If the createGist callback rejects with a non-Error (a string, a plain object, etc.), error.message evaluates to undefined and the client receives "Failed to create gist: undefined". Guarding with error instanceof Error ? error.message : String(error) is the convention used throughout this codebase (see gist.ts line 675 and web-server-factory.ts error paths) and avoids the unhelpful message.
| this.callbacks | |
| .createGist(sessionId, description, isPublic) | |
| .catch((error: unknown) => { | |
| const msg = error instanceof Error ? error.message : String(error); | |
| this.sendError(client, `Failed to create gist: ${msg}`); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/__tests__/main/web-server/handlers/messageHandlers.test.ts (1)
122-122: Consider adding handler-level tests forcreate_gist.The mock callback is added to
createMockCallbacks, but this suite doesn't yet exercise thecreate_gistmessage path (happy path, missingagentId,createGistcallback unconfigured, callback rejection →create_gist_resultwith error). Given the structured error contract promised in the PR description (AGENT_NOT_FOUND,GIST_CREATE_FAILED, timeouts), a small describe block here would lock that contract down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/web-server/handlers/messageHandlers.test.ts` at line 122, Add a new describe block in the existing message handlers test suite that exercises the create_gist message path using the createMockCallbacks mock: write tests for the happy path (mock createGist to resolve and assert a create_gist_result with success and gistUrl), for missing agentId (send message without agentId and assert create_gist_result returns AGENT_NOT_FOUND), for an unconfigured callback (ensure createGist is undefined in createMockCallbacks and assert create_gist_result returns GIST_CREATE_FAILED or appropriate error), and for callback rejection (mock createGist to reject and assert create_gist_result contains GIST_CREATE_FAILED and error details). Ensure each test sends the create_gist message and inspects the returned create_gist_result payload.src/main/preload/process.ts (1)
1113-1130: Consider the ack-on-sync-throw pattern for consistency with sibling handlers.Unlike
onRemoteOpenBrowserTab,onRemoteOpenTerminalTab,onRemoteNewAITabWithPrompt, andonRemoteConfigureAutoRun(which wrap the callback invocation intry/catchand send a failure ack on synchronous throw),onRemoteCreateGistforwards directly. If the renderer's callback ever throws synchronously (e.g., programming error before thetryinuseRemoteIntegration.tsruns), the CLI will stall until the 60sweb-server-factorytimeout instead of failing fast.In practice the current renderer implementation is async-safe, but matching the surrounding pattern is cheap defense.
♻️ Align with sibling handlers
onRemoteCreateGist: ( callback: ( sessionId: string, description: string, isPublic: boolean, responseChannel: string ) => void ): (() => void) => { const handler = ( _: unknown, sessionId: string, description: string, isPublic: boolean, responseChannel: string - ) => callback(sessionId, description, isPublic, responseChannel); + ) => { + try { + callback(sessionId, description, isPublic, responseChannel); + } catch (error) { + ipcRenderer.send(responseChannel, { + success: false, + error: error instanceof Error ? error.message : String(error), + }); + throw error; + } + }; ipcRenderer.on('remote:createGist', handler); return () => ipcRenderer.removeListener('remote:createGist', handler); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/process.ts` around lines 1113 - 1130, The onRemoteCreateGist handler currently forwards the renderer callback directly and can let a synchronous throw bubble up; update the handler in onRemoteCreateGist to mirror the ack-on-sync-throw pattern used by onRemoteOpenBrowserTab/onRemoteOpenTerminalTab/onRemoteNewAITabWithPrompt/onRemoteConfigureAutoRun by wrapping the callback invocation in try/catch and, on a caught synchronous error, send a failure acknowledgment on the provided responseChannel (e.g., ipcRenderer.send(responseChannel, { success: false, error: error?.message || String(error) })) before returning; keep the same handler signature and the existing ipcRenderer.on/removeListener lifecycle.src/__tests__/cli/services/agent-spawner.test.ts (1)
1637-2105: LGTM on the new config-override + SSH test coverage.The new
driveSpawnToCompletionharness that raceswaitForSpawnCall()against the promise is a nice touch — it lets the SSH hard-fail tests resolve via the returned error without hanging on a spawn that never happens. Precedence (session vs agent-level), env layering (shell-wins vs user-wins), and SSH wrapper invocation/failure paths are all covered.One soft nit (non-blocking): in
forwards agent binaryName (not local path) to the SSH wrapper(Lines 1967-1982),mockGetAgentCustomPath.mockReturnValue('/opt/local/claude')has no effect ongetAgentCommand('claude-code')here becausecachedPathsis empty anddetectAgent()isn't invoked in this test — sowrapConfig.commandis'claude'too, and the test only provesagentBinaryName === 'claude'. If you want to actually exercise the "local custom path vs remote binary name" distinction, prime the cache viadetectAgent(or export a test seam) and additionally assertwrapConfig.commandpoints at the local path whileagentBinaryNamestays'claude'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/cli/services/agent-spawner.test.ts` around lines 1637 - 2105, The test "forwards agent binaryName (not local path) to the SSH wrapper" is not exercising the local-vs-remote distinction because cachedPaths isn't populated; prime the agent detection cache before calling spawnAgent so wrapConfig.command reflects the local path while wrapConfig.agentBinaryName remains the binary name. Fix by invoking or mocking detectAgent/getAgentCommand to populate cachedPaths (e.g., await detectAgent('claude-code') or mock detectAgent to return '/opt/local/claude') prior to spawnAgent, then assert wrapConfig.command is the local path and wrapConfig.agentBinaryName is 'claude'.src/cli/services/agent-spawner.ts (1)
90-109: Optional: batch-mode env defaults can't shadow agent defaults in the local path, but they do on SSH.In
applyEnvLayers, bothagentDefaultsandbatchDefaultsare written with anif (!env[k])guard. BecauseagentDefaultsruns first, any key that exists in bothdef.defaultEnvVarsanddef.batchModeEnvVarswill resolve to the agent-default value (the batch-default write is blocked by the guard once the agent default has populatedenv[k]). MeanwhilebuildSshEnvForRemote(Lines 471-482) uses sequentialObject.assignso the batch value wins on the remote side. The "batch mode is more specific than the interactive default" intent is reversed in the local path.No current overlap between
defaultEnvVarsandbatchModeEnvVarsis visible in the provided context, so this is latent today — flagging for when batch-mode-only overrides are added for an agent that also ships defaults for the same key.♻️ Proposed fix (let batch defaults override agent defaults, still shell-wins against both)
function applyEnvLayers( env: NodeJS.ProcessEnv, agentDefaults: Record<string, string> | undefined, batchDefaults: Record<string, string> | undefined, userEnvVars: Record<string, string> | undefined, readOnlyOverrides: Record<string, string> | undefined ): void { - if (agentDefaults) { - for (const [k, v] of Object.entries(agentDefaults)) { - if (!env[k]) env[k] = v; - } - } - if (batchDefaults) { - for (const [k, v] of Object.entries(batchDefaults)) { - if (!env[k]) env[k] = v; - } - } + // Merge agent < batch so batch can override agent; then layer with shell-wins + const defaults: Record<string, string> = { + ...(agentDefaults || {}), + ...(batchDefaults || {}), + }; + for (const [k, v] of Object.entries(defaults)) { + if (!env[k]) env[k] = v; + } if (userEnvVars) Object.assign(env, userEnvVars); if (readOnlyOverrides) Object.assign(env, readOnlyOverrides); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 90 - 109, applyEnvLayers currently lets agentDefaults win over batchDefaults locally because agentDefaults are set first with an "if (!env[k])" guard; change the logic so agentDefaults only apply when the key is not already set in env AND not present in batchDefaults (i.e., treat batchDefaults as more specific), e.g. when iterating agentDefaults skip keys that exist in batchDefaults; keep userEnvVars and readOnlyOverrides application order intact so shell/user overrides still win; refer to applyEnvLayers and buildSshEnvForRemote to ensure local behavior now matches the remote ssh path semantics.
🤖 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/gist.ts`:
- Around line 57-67: The code treats a truthy result.success as success even
when result.gistUrl is missing, producing an unusable GistCreateResponse; update
the success check to treat a missing or empty result.gistUrl as failure: after
verifying result.success, validate result.gistUrl (e.g., non-empty string) and
if absent call emitErrorJson('Failed to create gist', 'GIST_CREATE_FAILED') and
exit, otherwise build the GistCreateResponse (using agentId and result.gistUrl)
and print it; reference the existing symbols result.success, result.gistUrl,
emitErrorJson, GistCreateResponse, and agentId to locate and change the logic.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 2592-2616: The handler currently calls this.sendError for missing
sessionId and when createGist is not configured and also on the createGist
.catch path, which causes clients waiting for "create_gist_result" to timeout;
update these failure paths to always send a "create_gist_result" via this.send
with success: false and an error message (include message.requestId and
timestamp) instead of this.sendError. Specifically modify the checks that use
this.sendError (the sessionId null branch and the callbacks.createGist not
configured branch), and change the .catch handler on
this.callbacks.createGist(...) to send a "create_gist_result" payload containing
success: false, error: error.message, gistUrl: null (or omitted), requestId:
message.requestId, and timestamp: Date.now().
- Around line 2589-2591: Replace the loose truthy coercion for isPublic with an
explicit check against boolean true and the literal string "true": instead of
const isPublic = Boolean(message.isPublic), use something like const isPublic =
message.isPublic === true || message.isPublic === 'true' to ensure values like
"false" are not treated as true; update the code near where description and
isPublic are initialized (variables description and isPublic, using
message.isPublic) to use this strict validation.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 1096-1102: The catch currently swallows unexpected errors and only
sends a CLI response; change it to capture the error with Sentry (e.g.,
Sentry.captureException(error) or Sentry.captureMessage) following the same
"capture + ack" pattern used in onRemoteTriggerCueSubscription, then still call
window.maestro.process.sendRemoteCreateGistResponse(responseChannel, { success:
false, error: message }); ensure the Sentry payload uses captureException
without attaching tab content/description/filename/transcript or any PII (do not
include those fields in the Sentry scope or extra), and keep the existing error
string assignment (const message = error instanceof Error ? error.message :
String(error)) for the IPC response.
---
Nitpick comments:
In `@src/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 1637-2105: The test "forwards agent binaryName (not local path) to
the SSH wrapper" is not exercising the local-vs-remote distinction because
cachedPaths isn't populated; prime the agent detection cache before calling
spawnAgent so wrapConfig.command reflects the local path while
wrapConfig.agentBinaryName remains the binary name. Fix by invoking or mocking
detectAgent/getAgentCommand to populate cachedPaths (e.g., await
detectAgent('claude-code') or mock detectAgent to return '/opt/local/claude')
prior to spawnAgent, then assert wrapConfig.command is the local path and
wrapConfig.agentBinaryName is 'claude'.
In `@src/__tests__/main/web-server/handlers/messageHandlers.test.ts`:
- Line 122: Add a new describe block in the existing message handlers test suite
that exercises the create_gist message path using the createMockCallbacks mock:
write tests for the happy path (mock createGist to resolve and assert a
create_gist_result with success and gistUrl), for missing agentId (send message
without agentId and assert create_gist_result returns AGENT_NOT_FOUND), for an
unconfigured callback (ensure createGist is undefined in createMockCallbacks and
assert create_gist_result returns GIST_CREATE_FAILED or appropriate error), and
for callback rejection (mock createGist to reject and assert create_gist_result
contains GIST_CREATE_FAILED and error details). Ensure each test sends the
create_gist message and inspects the returned create_gist_result payload.
In `@src/cli/services/agent-spawner.ts`:
- Around line 90-109: applyEnvLayers currently lets agentDefaults win over
batchDefaults locally because agentDefaults are set first with an "if (!env[k])"
guard; change the logic so agentDefaults only apply when the key is not already
set in env AND not present in batchDefaults (i.e., treat batchDefaults as more
specific), e.g. when iterating agentDefaults skip keys that exist in
batchDefaults; keep userEnvVars and readOnlyOverrides application order intact
so shell/user overrides still win; refer to applyEnvLayers and
buildSshEnvForRemote to ensure local behavior now matches the remote ssh path
semantics.
In `@src/main/preload/process.ts`:
- Around line 1113-1130: The onRemoteCreateGist handler currently forwards the
renderer callback directly and can let a synchronous throw bubble up; update the
handler in onRemoteCreateGist to mirror the ack-on-sync-throw pattern used by
onRemoteOpenBrowserTab/onRemoteOpenTerminalTab/onRemoteNewAITabWithPrompt/onRemoteConfigureAutoRun
by wrapping the callback invocation in try/catch and, on a caught synchronous
error, send a failure acknowledgment on the provided responseChannel (e.g.,
ipcRenderer.send(responseChannel, { success: false, error: error?.message ||
String(error) })) before returning; keep the same handler signature and the
existing ipcRenderer.on/removeListener lifecycle.
🪄 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: c4dbc3d0-0c00-41e5-8c46-26bf786f467b
📒 Files selected for processing (20)
CLAUDE.mddocs/agent-guides/CLI-PLAYBOOKS.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/cli/commands/gist.tssrc/cli/commands/send.tssrc/cli/index.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/main/preload/process.tssrc/main/utils/agent-args.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/renderer/global.d.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/shared/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/renderer/hooks/remote/useRemoteIntegration.ts (1)
1096-1102:⚠️ Potential issue | 🟡 MinorReport unexpected errors to Sentry instead of silently wrapping them into the CLI response.
Known recoverable failures (session missing, empty history,
ghnot installed/authenticated) are already returned as explicit{ success: false, ... }payloads upstream, so anything arriving here is an unexpected failure (IPC crash, null deref, etc.). Follow theonRemoteTriggerCueSubscriptionpattern at lines 1023-1039 — capture withcaptureException(already imported on line 5), then send the failure ack. Keep the transcript/description/filename out of the Sentry payload since they can carry PII or secrets.🛡️ Capture + ack
} catch (error) { const message = error instanceof Error ? error.message : String(error); + captureException(error, { + extra: { + context: 'remoteCreateGist', + sessionId, + isPublic, + descriptionProvided: Boolean(description), + }, + }); window.maestro.process.sendRemoteCreateGistResponse(responseChannel, { success: false, error: message, }); }As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/remote/useRemoteIntegration.ts` around lines 1096 - 1102, The catch block that currently converts the unexpected error to a string and sends a CLI response should first report the raw exception to Sentry using captureException(error) (captureException is already imported), then continue to send the failure ack via window.maestro.process.sendRemoteCreateGistResponse(responseChannel, { success: false, error: message }); ensure you do not include transcript/description/filename or other PII in the capture payload; reference the existing responseChannel and sendRemoteCreateGistResponse call so the reporting happens before the existing send.src/cli/commands/gist.ts (1)
57-67:⚠️ Potential issue | 🟡 MinorTreat a missing
gistUrlon asuccess: trueresponse as a failure.The renderer-side contract (
window.maestro.git.createGist→{ success, gistUrl?, error? }) allowssuccess: truewithout a URL in theory, and other structured failures at the main/renderer boundary also resolve tosuccess: truepayloads with no URL (e.g., empty-response fallbacks atweb-server-factory.ts:1737). Without agistUrlthe CLI prints a "successful but unusable" JSON response.Proposed fix
- if (!result.success) { - emitErrorJson(result.error ?? 'Failed to create gist', 'GIST_CREATE_FAILED'); - process.exit(1); - } + if (!result.success || !result.gistUrl) { + emitErrorJson(result.error ?? 'Failed to create gist', 'GIST_CREATE_FAILED'); + process.exit(1); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/gist.ts` around lines 57 - 67, When handling the renderer result in gist.ts, treat a success:true that lacks a gistUrl as a failure: after checking result.success, also verify result.gistUrl is a non-empty string and if not call emitErrorJson with a descriptive error (e.g., 'Missing gistUrl in successful response') and exit via process.exit(1); only construct the GistCreateResponse (using agentId and result.gistUrl) and console.log when result.success === true and result.gistUrl is present.
🤖 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/main/web-server/web-server-factory.ts`:
- Around line 1715-1764: The tests break because the new server API method
setCreateGistCallback is not stubbed in
src/__tests__/main/web-server/web-server-factory.test.ts; update the test setup
to add a mock/no-op for server.setCreateGistCallback (or a jest.fn()) so the
factory can call it without throwing, and add an assertion that
setCreateGistCallback was called (or that the callback was registered) to
validate the change; reference the server mock used in that test and the
exported setCreateGistCallback method when adding the stub and assertion.
---
Duplicate comments:
In `@src/cli/commands/gist.ts`:
- Around line 57-67: When handling the renderer result in gist.ts, treat a
success:true that lacks a gistUrl as a failure: after checking result.success,
also verify result.gistUrl is a non-empty string and if not call emitErrorJson
with a descriptive error (e.g., 'Missing gistUrl in successful response') and
exit via process.exit(1); only construct the GistCreateResponse (using agentId
and result.gistUrl) and console.log when result.success === true and
result.gistUrl is present.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 1096-1102: The catch block that currently converts the unexpected
error to a string and sends a CLI response should first report the raw exception
to Sentry using captureException(error) (captureException is already imported),
then continue to send the failure ack via
window.maestro.process.sendRemoteCreateGistResponse(responseChannel, { success:
false, error: message }); ensure you do not include
transcript/description/filename or other PII in the capture payload; reference
the existing responseChannel and sendRemoteCreateGistResponse call so the
reporting happens before the existing send.
🪄 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: d85236c7-38e0-4c46-be8c-979a0f789be3
📒 Files selected for processing (12)
src/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/cli/commands/gist.tssrc/cli/index.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/renderer/global.d.tssrc/renderer/hooks/remote/useRemoteIntegration.ts
✅ Files skipped from review due to trivial changes (3)
- src/tests/main/web-server/handlers/messageHandlers.test.ts
- src/main/preload/process.ts
- src/renderer/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tests/renderer/hooks/useRemoteIntegration.test.ts
- src/main/web-server/WebServer.ts
- src/main/web-server/types.ts
- src/main/web-server/handlers/messageHandlers.ts
| // Create gist — uses IPC request-response pattern. The renderer holds | ||
| // the AI-tab transcripts in memory, so we forward to it and let it | ||
| // build the payload + call the existing `git:createGist` handler. | ||
| server.setCreateGistCallback( | ||
| async (sessionId: string, description: string, isPublic: boolean) => { | ||
| const mainWindow = getMainWindow(); | ||
| if (!mainWindow) { | ||
| logger.warn('mainWindow is null for createGist', 'WebServer'); | ||
| return { success: false, error: 'Desktop app window is not available' }; | ||
| } | ||
|
|
||
| return new Promise<{ success: boolean; gistUrl?: string; error?: string }>((resolve) => { | ||
| const responseChannel = `remote:createGist:response:${randomUUID()}`; | ||
| let resolved = false; | ||
|
|
||
| const handleResponse = ( | ||
| _event: Electron.IpcMainEvent, | ||
| result: { success: boolean; gistUrl?: string; error?: string } | undefined | ||
| ) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| clearTimeout(timeoutId); | ||
| resolve(result ?? { success: false, error: 'Empty response' }); | ||
| }; | ||
|
|
||
| ipcMain.once(responseChannel, handleResponse); | ||
| if (!isWebContentsAvailable(mainWindow)) { | ||
| logger.warn('webContents is not available for createGist', 'WebServer'); | ||
| ipcMain.removeListener(responseChannel, handleResponse); | ||
| resolve({ success: false, error: 'Desktop webContents not available' }); | ||
| return; | ||
| } | ||
| mainWindow.webContents.send( | ||
| 'remote:createGist', | ||
| sessionId, | ||
| description, | ||
| isPublic, | ||
| responseChannel | ||
| ); | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
| ipcMain.removeListener(responseChannel, handleResponse); | ||
| logger.warn(`createGist callback timed out for session ${sessionId}`, 'WebServer'); | ||
| resolve({ success: false, error: 'Timed out waiting for gist creation' }); | ||
| }, 60000); | ||
| }); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Create-gist callback registration LGTM; 60s timeout matches the end-to-end cost of gh gist create.
The request-response wiring follows the established pattern (per-call responseChannel via randomUUID(), guarded ipcMain.once, timeout cleanup, structured failure payloads). One gap the CI is already flagging: src/__tests__/main/web-server/web-server-factory.test.ts doesn't stub the new setCreateGistCallback, so every factory test in that file throws TypeError: server.setCreateGistCallback is not a function. That needs a mock update (and ideally a positive assertion that the callback was registered) in that test file before this PR can merge green.
🧰 Tools
🪛 GitHub Check: test
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > callback registrations > should register getSessionDetailCallback
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:349:13
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > callback registrations > should register getSessionsCallback
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:349:13
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should generate and store new token when persistentWebLink is true and no token exists
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:325:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should reject invalid stored token and generate a new UUID
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:300:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should use stored token when persistentWebLink is true and token is a valid UUID
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:287:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should not pass security token when persistentWebLink is false
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:273:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should use custom port when enabled
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:260:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should use random port (0) when custom port is disabled
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:246:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should register a bionify reading mode callback sourced from settings
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:230:19
[failure] 1718-1718: src/tests/main/web-server/web-server-factory.test.ts > web-server/web-server-factory > createWebServerFactory > should create a WebServer when called
TypeError: server.setCreateGistCallback is not a function
❯ createWebServer src/main/web-server/web-server-factory.ts:1718:10
❯ src/tests/main/web-server/web-server-factory.test.ts:217:19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/web-server/web-server-factory.ts` around lines 1715 - 1764, The
tests break because the new server API method setCreateGistCallback is not
stubbed in src/__tests__/main/web-server/web-server-factory.test.ts; update the
test setup to add a mock/no-op for server.setCreateGistCallback (or a jest.fn())
so the factory can call it without throwing, and add an assertion that
setCreateGistCallback was called (or that the callback was registered) to
validate the change; reference the server mock used in that test and the
exported setCreateGistCallback method when adding the stub and assertion.
|
Thanks for putting this together, @chr1syy — nice clean reuse of the existing remote-request pattern, and the BlockingCI Worth addressing (bot feedback I agree with)
Non-blocking, but worth a quick look. Once CI is green and #1–#3 are addressed, we should be good to land. No merge conflicts currently — just a stacking dependency on #888 as you noted. |
- Stub the new `setCreateGistCallback` in the factory test's WebServer mock so every `createWebServer()` test stops throwing TypeError. - `handleCreateGist` now always replies with `create_gist_result` on every failure path (missing sessionId, missing callback, strict-validation refusal, rejected promise). Previously sendError was emitted, which left clients hanging until their WebSocket request timeout. - Strictly validate `isPublic` and `description` types; reject coerced truthy values like the string "false" to prevent accidental private→public gist publication. - Guard the rejected-promise `catch` with `instanceof Error`. - Renderer gist handler now reports unexpected errors via `captureException` (omitting transcript/description/filename, which can carry PII). - CLI treats a `success: true` response with no `gistUrl` as a failure. - Added handler tests pinning the new contract (success path, defaults, missing sessionId, non-boolean isPublic, rejected callback). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR RunMaestro#898 added the gist callback to CallbackRegistry and the factory's setCreateGistCallback, but `setupMessageHandlerCallbacks` in WebServer.ts was missing the createGist mapping. As a result, `messageHandlers.handleCreateGist` always saw `this.callbacks.createGist === undefined` and replied with "Gist creation not configured", so `maestro-cli gist create` returned a hard failure for every input even though the rest of the pipeline was correct. Mirror the pattern used for summarizeContext: forward the call through to `this.callbackRegistry.createGist`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Conflicts: - src/cli/index.ts: keep both gist and notify command groups - src/renderer/hooks/remote/useRemoteIntegration.ts: keep both formatLogsForClipboard and notify/session-store imports Review fixes: - preload/process.ts onRemoteCreateGist: ack a structured failure on synchronous callback throw before rethrowing, mirroring the sibling onRemoteOpenBrowserTab/onRemoteOpenTerminalTab/onRemoteNewAITabWithPrompt pattern so the CLI doesn't wait on the 60s response timeout. - Add messageHandlers test for the unconfigured-callback path (createGist === undefined → create_gist_result with "not configured"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/web-server/web-server-factory.ts (1)
1746-1746:⚠️ Potential issue | 🔴 CriticalWebServer factory tests must mock the new
setCreateGistCallbackAPI.Adding this setter changes the expected WebServer mock shape; factory tests will fail until the test double includes
setCreateGistCallbackand validates registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/web-server/web-server-factory.ts` at line 1746, Update the web-server factory tests to include and assert the new setter on the WebServer test double: add a mock implementation for setCreateGistCallback on the WebServer stub used by the factory tests, ensure the factory calls server.setCreateGistCallback(...) during setup, and validate that the provided callback was registered (e.g., called or stored) so the mock shape matches the updated WebServer interface; look for references to setCreateGistCallback and the WebServer stub in the web-server-factory tests and add the necessary mock + assertion.
🧹 Nitpick comments (1)
src/__tests__/main/web-server/handlers/messageHandlers.test.ts (1)
1733-1749: Add a non-Errorrejection test forcreate_gist.Current coverage only asserts
Errorrejections. Add a case for string/object rejection to lock in theinstanceof Errorguard behavior and prevent"undefined"error payload regressions.🧪 Suggested test addition
describe('Create Gist', () => { @@ it('surfaces rejected callback errors as create_gist_result', async () => { (callbacks.createGist as any).mockRejectedValue(new Error('boom')); @@ expect(response.error).toContain('boom'); }); + + it('surfaces non-Error rejections as create_gist_result', async () => { + (callbacks.createGist as any).mockRejectedValue('boom-string'); + + handler.handleMessage(client, { + type: 'create_gist', + sessionId: 'session-1', + }); + + await vi.waitFor(() => { + expect(client.socket.send).toHaveBeenCalled(); + }); + + const calls = (client.socket.send as any).mock.calls; + const response = JSON.parse(calls[calls.length - 1][0]); + expect(response.type).toBe('create_gist_result'); + expect(response.success).toBe(false); + expect(response.error).toContain('boom-string'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/web-server/handlers/messageHandlers.test.ts` around lines 1733 - 1749, Add a new test that mirrors the existing "surfaces rejected callback errors as create_gist_result" but mocks callbacks.createGist to reject with a non-Error value (e.g., a string or plain object) and then calls handler.handleMessage(client, { type: 'create_gist', sessionId: 'session-1' }); wait for client.socket.send and assert the JSON-parsed response still has type 'create_gist_result', success false, and that response.error contains a sensible message (for string rejects assert the string appears; for object rejects assert a fallback like JSON.stringify or a default message is used). This ensures the instanceof Error guard around callbacks.createGist rejection handling produces a stable error payload when callbacks.createGist is rejected with non-Error values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/web-server/web-server-factory.ts`:
- Line 1746: Update the web-server factory tests to include and assert the new
setter on the WebServer test double: add a mock implementation for
setCreateGistCallback on the WebServer stub used by the factory tests, ensure
the factory calls server.setCreateGistCallback(...) during setup, and validate
that the provided callback was registered (e.g., called or stored) so the mock
shape matches the updated WebServer interface; look for references to
setCreateGistCallback and the WebServer stub in the web-server-factory tests and
add the necessary mock + assertion.
---
Nitpick comments:
In `@src/__tests__/main/web-server/handlers/messageHandlers.test.ts`:
- Around line 1733-1749: Add a new test that mirrors the existing "surfaces
rejected callback errors as create_gist_result" but mocks callbacks.createGist
to reject with a non-Error value (e.g., a string or plain object) and then calls
handler.handleMessage(client, { type: 'create_gist', sessionId: 'session-1' });
wait for client.socket.send and assert the JSON-parsed response still has type
'create_gist_result', success false, and that response.error contains a sensible
message (for string rejects assert the string appears; for object rejects assert
a fallback like JSON.stringify or a default message is used). This ensures the
instanceof Error guard around callbacks.createGist rejection handling produces a
stable error payload when callbacks.createGist is rejected with non-Error
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31febc86-068d-4f3f-b3c1-ba1494b31a66
📒 Files selected for processing (12)
src/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/cli/index.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/renderer/global.d.tssrc/renderer/hooks/remote/useRemoteIntegration.ts
✅ Files skipped from review due to trivial changes (3)
- src/tests/main/web-server/web-server-factory.test.ts
- src/tests/renderer/hooks/useRemoteIntegration.test.ts
- src/main/web-server/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/index.ts
…director-notes Closes documentation gaps in both the bundled agent-facing CLI reference (`src/prompts/_maestro-cli.md`) and the public user guide (`docs/cli.md`): - `dispatch` (PR #907): full section with flag table, JSON response shape, and error codes. `send --live` row marked deprecated, pointing to dispatch. - `gist create` (PR #898): section with `gh` auth prerequisite and error codes. - `open-browser` (Apr 21): http(s)-only constraint and scheme-less auto-prefix behavior documented. - `open-terminal` (Apr 21): cwd-confinement rule and `zsh` default documented. - `director-notes`: was only in the bundled prompt; now in the public guide too, with the Encore-flag prerequisite called out. Pure documentation — no source changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
maestro-cli gist create <agent-id> [-d <desc>] [-p], which asks the running Maestro desktop app to publish an agent's full session transcript (every AI tab) as a single GitHub gist.git:createGistIPC handler, so the sameghCLI install + auth requirements as the desktop "Publish Gist" flow apply.--publicopts into a public gist.--descriptionis optional.How it works
Plumbing mirrors the summarize/transfer/merge-context remote pattern already in the codebase:
withMaestroClientand sends a newcreate_gistmessage.handleCreateGistinmessageHandlers.tsinvokes acreateGistcallback.web-server-factory.tsforwards to the renderer overremote:createGist(request-response with a timeout).formatLogsForClipboard(), concatenates them under## Tab:headers, and callswindow.maestro.git.createGist(...).gh gist createand the result propagates back through the WebSocket ascreate_gist_result.Errors (agent not found, empty conversation,
ghnot installed/authenticated, desktop app not running, timeout) are mapped to structured JSON with codesAGENT_NOT_FOUND,MAESTRO_NOT_RUNNING,GIST_CREATE_FAILED.Stacking note
This PR is stacked on top of #888 (
feat/cli-ssh-remote-agents). Until that merges, the GitHub diff here will show those commits too; they drop off automatically once #888 lands.Test plan
tsc -p tsconfig.lint.json,tsconfig.main.json,tsconfig.cli.json— clean on touched fileseslinton touched files — cleanprettier --check— formattedvitest run src/__tests__/renderer/hooks/useRemoteIntegration.test.ts src/__tests__/main/web-server/handlers/messageHandlers.test.ts— 125 tests pass (tests updated to mock the new callback/preload hooks)maestro-cli gist create <agent-id>— expect ahttps://gist.github.com/...URL in the JSON response--publicand--description "..."MAESTRO_NOT_RUNNINGGIST_CREATE_FAILEDwith a clear message🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests