feat(coworking): expose terminal scrollback to all four agents via opt-in MCP installer#948
Closed
chr1syy wants to merge 25 commits into
Closed
feat(coworking): expose terminal scrollback to all four agents via opt-in MCP installer#948chr1syy wants to merge 25 commits into
chr1syy wants to merge 25 commits into
Conversation
…t-in MCP installer Adds a Settings → Encore Features → Coworking panel that installs a Maestro-managed MCP server entry into each supported agent's user-level config (Claude Code, Codex, OpenCode, Factory Droid). When installed, the agent gains two read-only tools — list_terminals and read_terminal — that surface scrollback from the currently active AI tab's terminal tabs. Each terminal renders a click-to-copy "term:N" pill so users can address them by name in chat. Architecture: - Stable bundled MCP stdio server written to <userData>/coworking-mcp-server.js on every app ready. Spawned by the agent's own MCP client (per protocol); Maestro just keeps a Unix-domain-socket / Windows named-pipe IPC bridge alive. - Per-agent installer strategies under src/main/coworking/installers/ — comment-preserving JSON merge for Claude Code / OpenCode / Factory Droid, sentinel-delimited block append for Codex's TOML. - Renderer pushes active-session terminal records to a main-process registry; the registry feeds list_terminals and resolves "term:N" → tabUuid; main asks renderer for buffers via a request-response IPC channel. - Encore flag `coworking` (off by default) gates the Settings panel and the pill UI. Bridge runs whenever Maestro is up; per-agent activation is opt-in. Tests: 36 new coworking tests + extended terminalTabHelpers tests cover registry filtering, tool truncation, per-agent install/uninstall round-trips with comment preservation, and monotonic non-reusing coworkingId minting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds coworking support across shared contracts, session-scoped registry and tools, bridge/server routing, per-agent installers, IPC/preload wiring, and renderer state and UI for terminal and browser access. ChangesCoworking MCP Terminal and Browser Access
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Greptile SummaryThis PR introduces a Coworking feature that lets each AI agent read its own session's terminal scrollback on demand via a per-agent MCP server. The architecture is well-designed: a bundled stdio MCP server in
Confidence Score: 4/5Safe to merge on Linux; the Codex PID-walk path blocks the Electron main process synchronously on macOS and Windows and should be made async before shipping to those platforms. The session isolation design is solid and the privacy regression from #948 is not reintroduced. The one concrete defect on the changed path is in src/main/coworking/pid-resolution.ts — Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as Agent CLI (e.g. Codex)
participant MCP as coworking-mcp-server.js (stdio)
participant Bridge as Coworking Bridge (main process)
participant Registry as CoworkingRegistry (main)
participant Renderer as Renderer
MCP->>Bridge: connect (Unix socket / named pipe)
MCP->>Bridge: "hello { sessionId | ppid }"
Bridge-->>MCP: "{ ok: true }"
MCP->>Bridge: "listTerminals {}"
Bridge->>Registry: listForSession(sessionId)
Registry-->>Bridge: "[{ id, cwd, title }]"
Bridge-->>MCP: result
MCP->>Bridge: "readTerminal { id, lines? }"
Bridge->>Registry: resolveTabUuidForSession(sessionId, id)
Registry-->>Bridge: tabUuid
Bridge->>Renderer: webContents.send(requestBuffer, tabUuid, sessionId, responseChannel)
Renderer-->>Bridge: ipcRenderer.send(responseChannel, content)
Bridge-->>MCP: "result { content, truncated, totalLines }"
Reviews (3): Last reviewed commit: "Merge branch 'rc' into feat/agentic-cowo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Collaborator
|
Hey @chr1syy — thanks so much for putting this together! The Coworking design (one-shot user-level MCP install + Heads up: the branch currently has merge conflicts against Thanks again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/main/coworking/coworking-types.ts (1)
79-88: ⚡ Quick winAdopt these ID helpers everywhere
term:Nis built/parsing manually.Now that
formatCoworkingId/parseCoworkingIdexist, replace hardcodedterm:string math in coworking registry/renderer call sites to prevent format drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/coworking/coworking-types.ts` around lines 79 - 88, Several call sites build or parse the public coworking id via string math; replace those with the new helpers to avoid format drift by using formatCoworkingId(...) when constructing IDs and parseCoworkingId(...) when reading them. Search for manual "term:" concatenations and Number/id slicing in the coworking registry and renderer code, update construction sites to call formatCoworkingId(coworkingId) and replace parsing sites to call parseCoworkingId(id) and handle the null case appropriately (e.g., treat as invalid input). Ensure all references to the literal "term:" are removed so formatCoworkingId and parseCoworkingId are the single source of truth.
🤖 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/coworking/coworking-registry.ts`:
- Around line 110-116: In notify(), do not leave the catch block empty: when
iterating this.listeners in notify() catch exceptions thrown by fn() and report
them using the Sentry utilities (import captureException and/or captureMessage
from src/utils/sentry.ts) with contextual info (e.g., which listener or a
descriptive message referencing notify/listeners). Preserve isolation by not
letting one listener stop others (continue iteration after reporting), and for
truly unexpected, non-recoverable errors re-throw after capturing so global
error handling (Sentry) can record them.
In `@src/main/coworking/coworking-server-paths.ts`:
- Around line 27-29: The current use of fs.promises.readFile(scriptPath,
'utf8').catch(() => null) swallows all I/O errors; change it to only suppress
ENOENT and rethrow other errors so unexpected failures surface. Locate the read
and comparison around scriptPath and contents and replace the blanket catch with
logic that, on read failure, returns null only if err.code === 'ENOENT' and
otherwise throws the error (either by using an explicit try/catch around the
await or a .catch(err => { if (err.code === 'ENOENT') return null; throw err;
})). Ensure behavior otherwise remains: if existing === contents return
scriptPath.
In `@src/main/coworking/coworking-server-script.ts`:
- Around line 70-115: connectBridge and bridgeCall currently allow concurrent
callers to create multiple sockets that share the module-level bridgeBuffer and
leave pending RPCs unresolved on clean close; fix by serializing connection
attempts and rejecting in-flight RPCs on disconnect. Implement a single shared
"connectingPromise" returned by connectBridge when a connection is in-flight so
concurrent bridgeCall() callers await the same socket; only set bridgeConn after
the socket's 'connect' callback succeeds and initialize a fresh per-connection
bridgeBuffer there; on 'error' and 'close' handlers reject all entries in
pending with a clear Error (e.g., "bridge disconnected") and pending.clear(),
set bridgeConn = null and connectingPromise = null; ensure connectBridge clears
connectingPromise on both success and failure so subsequent attempts can retry.
Reference symbols: connectBridge, bridgeCall, bridgeConn, bridgeBuffer, pending,
connectingPromise.
In `@src/main/coworking/coworking-tools.ts`:
- Around line 45-59: The logic in the tailing/counting block uses
full.split('\n') directly, which treats a trailing newline as an extra empty
line; update the handling in the resolver result path (the code around
resolver(tabUuid), the args.lines branch and the totalLines calculation) to
normalize by trimming a single trailing '\n' before splitting (i.e., if full
endsWith '\n' remove that final newline only), then compute allLines, totalLines
and the slice(-args.lines) from that normalized string so totalLines and
truncated/tail results reflect real lines; apply the same normalization to the
final totalLines calculation (where totalLines = full.length === 0 ? 0 :
full.split('\n').length).
In `@src/main/index.ts`:
- Around line 1034-1040: The try/catch around the coworking bridge bootstrap
(wrapping ensureCoworkingServerScript and startCoworkingBridge) currently only
logs a warning and swallows the error; update the catch to call the Sentry
reporting utility (captureException) with the caught error and contextual
message before logging/continuing so failures are reported to production error
tracking; reference the existing functions ensureCoworkingServerScript and
startCoworkingBridge and use captureException (from the project's sentry
utilities) to capture the exception and include a short context string like
"Failed to start coworking bridge" when calling captureException.
In `@src/main/ipc/handlers/coworking.ts`:
- Around line 133-142: The IPC response handler currently accepts responses from
any renderer via ipcMain.once(responseChannel) which allows spoofing; update the
listener used in the Promise to validate the sender by checking that
_event.sender.id === win.webContents.id (or equivalently comparing _event.sender
=== win.webContents) and if it doesn't match, ignore/reject that specific call
(e.g., call reject or simply return and keep waiting until the timeout) so only
the expected BrowserWindow response resolves; adjust the
ipcMain.once(responseChannel, (_event, content) => { ... }) callback to perform
this sender validation and ensure the timeout/caller cleanup (clearTimeout,
remove listeners) still happens correctly using responseChannel and
BUFFER_REQUEST_TIMEOUT_MS.
In `@src/renderer/components/Settings/CoworkingSetup.tsx`:
- Around line 55-76: The per-agent handlers (e.g., handleInstall) only set
busyAgentId which disables a single row but leaves the bulk action
(installAll/uninstallAll) active; update the UI and handlers so the bulk button
is disabled whenever busyAgentId is non-null and add a guard at the start of
bulk handlers (and at the start of per-agent handlers) to return early if
busyAgentId is set; specifically, use the existing busyAgentId state and
setBusyAgentId to block overlapping operations, update the bulk action
components to check busyAgentId !== null to disable the button, and add an
early-return guard in handleInstall/handleUninstall and the
installAll/uninstallAll handlers to prevent concurrent mutations and races
before calling refresh().
In `@src/renderer/components/TabBar/TerminalTabItem.tsx`:
- Around line 111-118: The click handler handleCoworkingPillClick should await
navigator.clipboard.writeText inside a try/catch instead of calling .then(); on
success call flashCopiedToClipboard(), and on failure call captureException (or
captureMessage) from src/utils/sentry.ts with contextual info (include
coworkingPillId and the operation name) and consider optional user-visible
fallback; update handleCoworkingPillClick to e.stopPropagation(), check
coworkingPillId, then try { await
navigator.clipboard.writeText(coworkingPillId); flashCopiedToClipboard(); }
catch (err) { captureException(err, { extra: { coworkingPillId, action:
'copyCoworkingPill' } }); }.
In `@src/renderer/hooks/coworking/useCoworkingBufferResponder.ts`:
- Around line 15-35: The current onRequestBuffer handler swallows all exceptions
in a broad try/catch causing terminal read failures to disappear; replace the
blanket try/catch around getTerminalBuffer lookups with explicit handling: only
treat missing buffers as recoverable (resulting in empty content), but for
unexpected exceptions call the Sentry helper (captureException or captureMessage
from src/utils/sentry.ts) with context (tabUuid, sessionId, handler id) and
re-throw the error so it surfaces to global error tracking; keep the normal flow
that calls window.maestro.coworking.sendBufferResponse(responseChannel, content)
for successful or legitimately-empty reads, but do not convert unexpected
exceptions into silent empty strings.
In `@src/renderer/hooks/coworking/useCoworkingRegistrySync.ts`:
- Around line 24-61: The hook currently swallows all errors in the three catch
blocks around window.maestro.coworking.setActiveSession and
syncSessionTerminals; change each catch to inspect the thrown error and only
silently ignore known teardown/IPC-not-ready errors (e.g., "main process not
ready", "IPC channel closed" or whatever specific error.message/name your IPC
emits), but for any other/unexpected errors call captureException(error) (or
captureMessage for string errors) and rethrow the error so failures are
observable; apply this change to the setActiveSession calls in useEffect
teardown and the early-return branch and to the try/catch that calls
setActiveSession + syncSessionTerminals (refer to
window.maestro.coworking.setActiveSession and
window.maestro.coworking.syncSessionTerminals and lastPayloadRef) so only
expected teardown IPC failures are suppressed and all other errors are reported
and propagated.
In `@src/renderer/utils/terminalTabHelpers.ts`:
- Around line 122-126: The code currently defaults nextCoworkingId to 1
(nextCoworkingId = session.nextCoworkingId ?? 1) which can collide with existing
terminalTabs for legacy sessions; instead compute the nextCoworkingId when
session.nextCoworkingId is undefined by scanning session.terminalTabs (or the
terminalTabs array in scope) for existing numeric coworkingId suffixes (e.g.,
term:N), take the max and add 1, then assign coworkingId: tab.coworkingId ??
computedNextCoworkingId when building tabWithCoworkingId; apply the same
scan-and-compute fix to the other occurrence around the code that sets
nextCoworkingId at the second location (the block near the other occurrence you
noted).
---
Nitpick comments:
In `@src/main/coworking/coworking-types.ts`:
- Around line 79-88: Several call sites build or parse the public coworking id
via string math; replace those with the new helpers to avoid format drift by
using formatCoworkingId(...) when constructing IDs and parseCoworkingId(...)
when reading them. Search for manual "term:" concatenations and Number/id
slicing in the coworking registry and renderer code, update construction sites
to call formatCoworkingId(coworkingId) and replace parsing sites to call
parseCoworkingId(id) and handle the null case appropriately (e.g., treat as
invalid input). Ensure all references to the literal "term:" are removed so
formatCoworkingId and parseCoworkingId are the single source of truth.
🪄 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: 3d817612-f8fb-49f2-b1c1-65dedc3259e4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
package.jsonsrc/__tests__/main/coworking/coworking-registry.test.tssrc/__tests__/main/coworking/coworking-tools.test.tssrc/__tests__/main/coworking/coworking-types.test.tssrc/__tests__/main/coworking/installers/claude-code.test.tssrc/__tests__/main/coworking/installers/codex.test.tssrc/__tests__/main/coworking/installers/factory-droid.test.tssrc/__tests__/main/coworking/installers/opencode.test.tssrc/__tests__/renderer/utils/terminalTabHelpers.test.tssrc/main/coworking/coworking-bridge.tssrc/main/coworking/coworking-installer.tssrc/main/coworking/coworking-registry.tssrc/main/coworking/coworking-server-paths.tssrc/main/coworking/coworking-server-script.tssrc/main/coworking/coworking-tools.tssrc/main/coworking/coworking-types.tssrc/main/coworking/installers/claude-code.tssrc/main/coworking/installers/codex.tssrc/main/coworking/installers/factory-droid.tssrc/main/coworking/installers/index.tssrc/main/coworking/installers/opencode.tssrc/main/coworking/installers/types.tssrc/main/index.tssrc/main/ipc/handlers/coworking.tssrc/main/ipc/handlers/index.tssrc/main/preload/coworking.tssrc/main/preload/index.tssrc/renderer/components/MainPanel/MainPanel.tsxsrc/renderer/components/Settings/CoworkingSetup.tsxsrc/renderer/components/Settings/searchableSettings.tssrc/renderer/components/Settings/tabs/EncoreTab.tsxsrc/renderer/components/TabBar/TerminalTabItem.tsxsrc/renderer/global.d.tssrc/renderer/hooks/coworking/useCoworkingBufferResponder.tssrc/renderer/hooks/coworking/useCoworkingRegistrySync.tssrc/renderer/types/index.tssrc/renderer/utils/terminalTabHelpers.tssrc/shared/settingsMetadata.ts
- Untrack package-lock.json and add to .gitignore. - Fix display-name index bug in useCoworkingRegistrySync — getTerminalTabDisplayName now receives the original terminalTabs index, not the post-filter index, so pre-feature tabs with no coworkingId no longer skew the agent-visible "Terminal N" fallback names (greptile P1). - Fix coworkingId reuse on legacy sessions: addTerminalTab now falls back to max(existing) + 1 when nextCoworkingId is missing, and bumps strictly past the larger of the chosen id and the bumped counter (coderabbit major). - Trailing-newline off-by-one in readTerminal: a buffer ending with \n no longer adds a synthetic empty line to totalLines/tail truncation (coderabbit major). - Coworking IPC buffer-request handler: read sessionId once at dispatch (drop pointless IIFE; greptile P1), use scoped removeListener instead of removeAllListeners (greptile P2), validate sender id against the main window to reject spoofed responses (coderabbit major). - coworking-server-paths: only suppress ENOENT on the read-existing path (coderabbit major); add resolveNodeCommand() that resolves an absolute node path via which/where at install time, falling back to literal "node" if resolution fails — addresses GUI-launched-agent PATH issues with nvm/fnm/volta (greptile P2). buildMcpServerSpec is now async to thread the resolved node. - coworking-server-script: serialize concurrent connectBridge() calls behind a shared promise, reset the per-connection bridgeBuffer on connect, reject all pending RPCs on both error and clean close, and capture the resolved socket locally in bridgeCall to plug the null-deref + pending-leak hole (greptile P2 + coderabbit major). - Sentry capture for previously-swallowed errors: CoworkingRegistry.notify(), app-startup bridge bootstrap, useCoworkingBufferResponder catch, and the TerminalTabItem clipboard pill click. useCoworkingRegistrySync distinguishes expected teardown IPC errors (silently skipped) from unexpected ones (captured) per project Sentry guidelines (coderabbit major x5). - CoworkingSetup: bulk install/uninstall and per-agent install/uninstall now short-circuit when any other mutation is in flight, and the bulk button disables on anyBusy to prevent concurrent writes to the same user-level config file (coderabbit major). - New regression tests: trailing-newline behavior in readTerminal, empty-buffer case, and legacy-session coworkingId fallback in addTerminalTab. Scoped test count: 92 (was 89). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/renderer/hooks/coworking/useCoworkingRegistrySync.ts (1)
27-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnexpected IPC failures are still swallowed by
reportIfUnexpected().This helper captures and returns, so the
.catch(...)paths below still turn broken coworking sync into a no-op. That leaves the main registry stale until another effect happens. Re-throw after reporting, or only catch the known teardown cases at the call sites. As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."Suggested patch
-function reportIfUnexpected(err: unknown, scope: string): void { +function reportIfUnexpected(err: unknown, scope: string): never | void { if (isExpectedTeardownIpcError(err)) return; void captureException(err instanceof Error ? err : new Error(String(err)), { extra: { scope: `useCoworkingRegistrySync:${scope}` }, }); + throw err instanceof Error ? err : new Error(String(err)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/coworking/useCoworkingRegistrySync.ts` around lines 27 - 32, The helper reportIfUnexpected currently captures unexpected IPC errors then returns, which swallows them; update reportIfUnexpected (and its uses) so that after calling captureException (using captureException(err instanceof Error ? err : new Error(String(err)), { extra: { scope: `useCoworkingRegistrySync:${scope}` } })), the function re-throws the error unless isExpectedTeardownIpcError(err) returned true; this ensures unexpected errors bubble up instead of turning coworking sync into a no-op. Use the same error object (or a new Error(String(err)) when needed) when re-throwing to preserve stack/context.
🤖 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/coworking/coworking-server-script.ts`:
- Around line 188-194: The initialize handler currently hardcodes
protocolVersion to '2025-06-18' in the rpcResult payload; change it to negotiate
using the client's params.protocolVersion (from the incoming params) and your
server-supported versions: if params.protocolVersion is in your supported list
accept and echo it, otherwise select a compatible supported version (e.g.,
highest-preferred supported version or a documented fallback) and use that value
in the rpcResult sent by send(); update the code around the initialize branch
that builds the rpcResult so it computes negotiatedProtocolVersion and assigns
that instead of the literal '2025-06-18' (refer to the initialize handling
logic, rpcResult call, and send invocation).
In `@src/main/index.ts`:
- Around line 852-855: The shutdown call to stopCoworkingBridge() is currently
swallowing all errors; modify the promise catch to report failures to Sentry
instead of silently ignoring them: import and call the Sentry utility
(captureException or captureMessage) from src/utils/sentry.ts inside the catch
handler for stopCoworkingBridge(), include contextual info (e.g., "quit-time
stopCoworkingBridge failure") and still keep it best-effort (do not rethrow), so
shutdown errors are recorded in Sentry while app quit continues.
In `@src/main/ipc/handlers/coworking.ts`:
- Around line 131-145: Wrap the call to
win.webContents.send('coworking:requestBuffer', tabUuid, sessionId,
responseChannel) in a try/catch; if send() throws, immediately remove the
temporary IPC listener (ipcMain.removeListener(responseChannel, handler),
referencing responseChannel and handler), clear the timeout (clearTimeout(timer)
referencing timer), and reject the Promise with the caught error (or a new Error
with context) so the promise does not hang until BUFFER_REQUEST_TIMEOUT_MS
elapses. Ensure the same cleanup path is used as the timeout branch to avoid
leaking the listener.
In `@src/renderer/utils/terminalTabHelpers.ts`:
- Around line 125-144: The code uses session.nextCoworkingId as-is which can be
lower than existing coworkingIds; compute the starting id by clamping against
maxExistingCoworkingId + 1 (e.g. chosenNext = Math.max(session.nextCoworkingId
?? 0, maxExistingCoworkingId + 1)), use that chosenNext when assigning
tabWithCoworkingId.coworkingId (if tab.coworkingId is undefined), and then bump
and store nextCoworkingId as Math.max(chosenNext + 1,
(tabWithCoworkingId.coworkingId ?? 0) + 1) so we never reuse an existing id;
adjust references to nextCoworkingId, maxExistingCoworkingId,
tabWithCoworkingId, and the final nextCoworkingId assignment accordingly.
---
Duplicate comments:
In `@src/renderer/hooks/coworking/useCoworkingRegistrySync.ts`:
- Around line 27-32: The helper reportIfUnexpected currently captures unexpected
IPC errors then returns, which swallows them; update reportIfUnexpected (and its
uses) so that after calling captureException (using captureException(err
instanceof Error ? err : new Error(String(err)), { extra: { scope:
`useCoworkingRegistrySync:${scope}` } })), the function re-throws the error
unless isExpectedTeardownIpcError(err) returned true; this ensures unexpected
errors bubble up instead of turning coworking sync into a no-op. Use the same
error object (or a new Error(String(err)) when needed) when re-throwing to
preserve stack/context.
🪄 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: a6495532-261a-482a-bc6d-229457fd4e84
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignoresrc/__tests__/main/coworking/coworking-tools.test.tssrc/__tests__/renderer/utils/terminalTabHelpers.test.tssrc/main/coworking/coworking-installer.tssrc/main/coworking/coworking-registry.tssrc/main/coworking/coworking-server-paths.tssrc/main/coworking/coworking-server-script.tssrc/main/coworking/coworking-tools.tssrc/main/index.tssrc/main/ipc/handlers/coworking.tssrc/renderer/components/Settings/CoworkingSetup.tsxsrc/renderer/components/TabBar/TerminalTabItem.tsxsrc/renderer/hooks/coworking/useCoworkingBufferResponder.tssrc/renderer/hooks/coworking/useCoworkingRegistrySync.tssrc/renderer/utils/terminalTabHelpers.ts
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- src/main/coworking/coworking-registry.ts
- src/tests/renderer/utils/terminalTabHelpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/coworking/coworking-installer.ts
- src/main/coworking/coworking-tools.ts
- src/tests/main/coworking/coworking-tools.test.ts
…#948 review round 2 Merge resolves a single modify/delete conflict in package-lock.json (kept deleted, now gitignored on this branch). All other conflicts auto-merged cleanly: package.json, src/main/index.ts (new cue-telemetry + cue-stats imports), src/main/preload/index.ts (new createCueStatsApi), src/main/ipc/ handlers/index.ts (new registerCueStatsHandlers), src/renderer/components/ Settings/searchableSettings.ts, src/renderer/global.d.ts, src/renderer/ types/index.ts, and src/shared/settingsMetadata.ts. Round 2 review feedback: - coworking-server-script.ts: implement MCP protocolVersion negotiation in the initialize handler. Echo the client's protocolVersion if it's in our SUPPORTED_PROTOCOL_VERSIONS list (newest-first), otherwise fall back to the newest supported. Previously hardcoded to 2025-06-18, which violates the MCP spec for clients that enforce strict version matching (coderabbit major). - index.ts: capture exceptions on quit-time stopCoworkingBridge() failures via Sentry instead of silently swallowing them. Stale socket / pipe on next launch is now observable in production (coderabbit major). - ipc/handlers/coworking.ts: wrap webContents.send() in try/catch. If send throws (e.g. window destroyed between the guard and the call), reject the promise immediately and remove the temporary IPC listener instead of leaking it for the full 5s timeout window (coderabbit major). - renderer/utils/terminalTabHelpers.ts: clamp nextCoworkingId with Math.max(persisted ?? 1, maxExistingCoworkingId + 1) instead of just falling back when undefined. Protects against persisted-counter-lower-than-existing-id drift (corrupted save / older buggy build), which would otherwise hand out a duplicate term:N (coderabbit major). - New regression test: stale-counter clamp scenario (coworkingId=10 + counter=3 → next add must pick 11, not 3). Scoped test count: 93 (was 92). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/coworking/coworking-server-script.ts`:
- Around line 138-156: bridgeCall() can hang forever if the bridge stops
responding; add a per-RPC timeout so pending entries are cleaned and the
returned promise rejects after a deadline. When creating the pending entry in
bridgeCall (use the same identifiers: bridgeCall, pending, nextRpcId,
connectBridge, conn.write), start a timer (e.g., 30s) that deletes
pending.get(id) and rejects the promise with a timeout Error; store the timer
handle alongside resolve/reject in pending so the response handler can clear it;
when conn.write throws or when resolve/reject is called for a real response,
clear the timer and remove the pending entry to avoid leaks. Ensure the timeout
is rejected with a clear message like "coworking bridge: rpc timeout" so callers
can handle it.
🪄 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: b3a0d43f-cf3a-4d2c-b628-5084ad5144d9
📒 Files selected for processing (12)
package.jsonsrc/__tests__/renderer/utils/terminalTabHelpers.test.tssrc/main/coworking/coworking-server-script.tssrc/main/index.tssrc/main/ipc/handlers/coworking.tssrc/main/ipc/handlers/index.tssrc/main/preload/index.tssrc/renderer/components/Settings/searchableSettings.tssrc/renderer/global.d.tssrc/renderer/types/index.tssrc/renderer/utils/terminalTabHelpers.tssrc/shared/settingsMetadata.ts
✅ Files skipped from review due to trivial changes (2)
- src/shared/settingsMetadata.ts
- src/renderer/components/Settings/searchableSettings.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/preload/index.ts
- src/main/ipc/handlers/coworking.ts
- src/renderer/utils/terminalTabHelpers.ts
- src/tests/renderer/utils/terminalTabHelpers.test.ts
- src/main/ipc/handlers/index.ts
CI failure root cause: actions/setup-node@v6 with cache:npm requires a lockfile, and untracking package-lock.json broke both lint-and-format and test jobs at the npm-cache-setup step. Upstream/rc tracks the lockfile (the merge attempted to restore it). Reverting the .gitignore entry from c3a9892 and re-adding package-lock.json — was the wrong call to untrack it. Round 3 review feedback (coderabbit major): - coworking-server-script.ts: add a per-RPC timeout (BRIDGE_RPC_TIMEOUT_MS = 10s) inside bridgeCall(). Without this, a wedged-but-not-disconnected bridge can stall tools/call indefinitely because pending entries never resolve. The timer expiry deletes the pending entry first (so a late reply is a no-op) and rejects with a clear error; both resolve and reject are wrapped to clearTimeout on the success path; the conn.write try/catch also clears the timer when the write itself throws. 10s comfortably covers the main-process buffer-fetch cap (5s) plus jitter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…namespace CI test job was failing with TypeError: Cannot read properties of undefined (reading 'setActiveSession') across MainPanel tests. Root cause: those tests mock window.maestro without including the new `coworking` namespace, so the hooks bound at MainPanel mount crashed at the first useEffect tick. Defensive guard pattern (matches what existing hooks already do in this codebase): each hook reads `window.maestro?.coworking` once and bails cleanly when undefined. This also covers older preload bundles where the namespace isn't exposed yet (the feature degrades to off rather than crashing). - useCoworkingRegistrySync: early-return when bridge is missing; use the resolved `bridge` reference for all subsequent IPC calls. - useCoworkingBufferResponder: early-return + use resolved `bridge` for onRequestBuffer subscription and sendBufferResponse reply. - CoworkingSetup.refresh(): when bridge is missing, set statuses to [] and finish loading instead of throwing into notifyToast. Local repro fixed: vitest src/__tests__/renderer/components/MainPanel + src/__tests__/main/coworking + src/__tests__/renderer/utils/terminalTabHelpers all pass (281 tests, was failing 131+ MainPanel tests in CI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/coworking/useCoworkingRegistrySync.ts`:
- Around line 22-25: The current isExpectedTeardownIpcError function is too
permissive because its regex includes "cannot be cloned", which hides real
serialization/structured-clone bugs; update isExpectedTeardownIpcError to only
treat true teardown messages ("No handler registered" and "Object has been
destroyed") as expected by removing the "cannot be cloned" branch from the regex
(keep the function name and return behavior otherwise) so serialization errors
are allowed to surface to Sentry for debugging.
- Around line 78-90: The code updates lastPayloadRef.current before the async
IPC completes, so failures in bridge.setActiveSession or
bridge.syncSessionTerminals leave lastPayloadRef stuck and prevent retries;
change the flow so the ref is only updated after the awaited calls succeed (or,
if you must keep the optimistic set, revert lastPayloadRef.current =
null/previousPayload inside the catch that calls reportIfUnexpected) so that
failed syncs allow the next effect run to retry; adjust the async IIFE around
bridge.setActiveSession and bridge.syncSessionTerminals and reference
lastPayloadRef, payload, and reportIfUnexpected accordingly.
🪄 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: 738fc16e-c148-4862-9987-2349a68defc7
📒 Files selected for processing (3)
src/renderer/components/Settings/CoworkingSetup.tsxsrc/renderer/hooks/coworking/useCoworkingBufferResponder.tssrc/renderer/hooks/coworking/useCoworkingRegistrySync.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/Settings/CoworkingSetup.tsx
- src/renderer/hooks/coworking/useCoworkingBufferResponder.ts
- Drop "cannot be cloned" from the expected-teardown regex in isExpectedTeardownIpcError. That message is a structured-clone bug (caller put a non-serializable value in the IPC payload) — a real programming error, not a teardown condition. Letting Sentry see it is the point (coderabbit minor). - Roll back lastPayloadRef.current = '' inside the sync catch so the next effect run retries the failed sync. Previously the ref was set optimistically before the async IIFE resolved, so any IPC failure would leave the main-process registry stale forever — until the user switched sessions or toggled the flag — because the cache-hit short-circuit would block all subsequent attempts (coderabbit major). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Blocker:
|
Closes the privacy blocker reported on PR RunMaestro#948: list_terminals and read_terminal returned whichever Maestro session the user was focused on, so an agent running in the background could read another agent's terminals when focus shifted mid-call. Approach: - Inject MAESTRO_COWORKING_SESSION_ID at agent CLI spawn (ProcessManager) so every MCP subprocess inherits its caller's session id. - MCP server reads the env var on startup; hard-fails if missing rather than degrading to a permissive default. - Bridge requires a `hello` handshake naming the session id before any tool call; per-connection sessionId is stored in a WeakMap and used to scope dispatch. - Registry drops the `activeSessionId` singleton and the corresponding `setActiveSession` / `listForActiveSession` / `resolveTabUuidForActiveSession` helpers; lookups now take an explicit sessionId. - Renderer pushes terminals for every session (not just the focused one) so agents in the background still see their own terminals. - cwd field falls back to session.cwd / projectRoot so the schema's cwd promise is no longer violated with an empty string. - Added handshake/binding tests; updated registry + tools tests for the per-session API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@greptileai re-review please |
|
Tip For best results, initiate chat on the files or code changes.
I also want to highlight that the scoping issue you documented is a significant finding. The core problem — the
[full_review] |
Contributor
Author
Regression —
|
…composite Follow-up to d035932. The privacy fix injected `MAESTRO_COWORKING_SESSION_ID = config.sessionId` at agent spawn, but ProcessManager spawns AI tabs with a composite id of the form `{maestroSessionId}-ai-{tabId}` (and `-fp-{ts}` for forced-parallel). The renderer pushes terminal records keyed by the bare `Session.id`, so the bridge handshake bound the connection to a key that never had records — `list_terminals` came back empty for the caller's own session. Adds `resolveOwningMaestroSessionId(spawnSessionId)` in src/main/coworking/coworking-session-id.ts, which strips the `-ai-{tabId}[-fp-{ts}]` suffix (and the legacy `-ai`) so the env var matches what the renderer pushes under. Non-AI spawn flavors (synopsis-, batch-, group-chat-…) pass through unchanged — they have no terminals to read, so the registry returning [] for them is fine. Tests cover bare-id passthrough, AI-tab unwrap, fp suffix unwrap, UUID-shaped session ids, the legacy `-ai` form, and confirmation that `-ai` inside a hyphenated word does not trigger the parser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ismatch Adds three log lines to triage the in-app "list_terminals returns not-accessible" report on PR RunMaestro#948: - IPC sync: prints which sessionId the renderer is pushing under and how many records it's pushing. - Bridge hello: prints the sessionId the MCP subprocess announces. - Bridge listTerminals: prints the bound sessionId, every sessionId the registry currently knows about, and the size of the result. - MCP server stderr: prints SESSION_ID + SOCKET on startup so we can confirm the env var actually reached the subprocess. Plus a `knownSessionIds()` helper on CoworkingRegistry so the bridge log can read live state without poking private fields. Will be reverted once the self-session list_terminals path is verified end-to-end against a real claude-code agent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The four diagnostic log points added in d9d4727 (bridge hello, bridge listTerminals, IPC syncSessionTerminals, MCP server stderr startup) and the supporting registry.knownSessionIds() helper are removed. They served their purpose for triage; the branch now ships clean for the next production build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…andshake Codex CLI only passes env declared in its config TOML to MCP subprocesses, so MAESTRO_COWORKING_SESSION_ID never reached coworking-mcp-server.js and the server hard-failed before initialize. Adds a parent-PID fallback: hello sends ppid alongside the optional sessionId, the bridge walks the process tree (cap 5 hops) to find a tracked agent-CLI PID via ProcessManager.getSessionIdByPid, and rejects when neither source resolves — preserving the fail-closed privacy property from PR RunMaestro#948. Claude Code/OpenCode keep using the fast env path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves conflicts with upstream additions that landed independently: - package.json: keep both new dependencies (@lezer/highlight from rc + @modelcontextprotocol/sdk from this branch) and take rc's @sentry/electron bump. - package-lock.json: regenerated against the merged package.json. - src/renderer/types/index.ts: TerminalTab gains both coworkingId (this branch) and startupCommand / startupCommandCwd (rc) — orthogonal fields. - src/renderer/components/Settings/tabs/EncoreTab.tsx: keep both imports (CoworkingSetup from this branch + Cue settings/service from rc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
Author
|
@greptileai review |
✅ Actions performedReview triggered.
|
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Comment on lines
+37
to
+65
| if (isMacOS()) { | ||
| const out = execFileSync('ps', ['-o', 'ppid=', '-p', String(pid)], { | ||
| encoding: 'utf8', | ||
| timeout: 1000, | ||
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| }).trim(); | ||
| if (!out) return null; | ||
| const n = Number(out); | ||
| return Number.isInteger(n) && n > 0 ? n : null; | ||
| } | ||
| if (isWindows()) { | ||
| // wmic is deprecated but still present on Windows 10/11. PowerShell would | ||
| // be more future-proof but costs ~200ms cold-start per invocation, which | ||
| // is far too slow for a handshake-critical path. | ||
| const out = execFileSync( | ||
| 'wmic', | ||
| ['process', 'where', `ProcessId=${pid}`, 'get', 'ParentProcessId', '/value'], | ||
| { | ||
| encoding: 'utf8', | ||
| timeout: 2000, | ||
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| windowsHide: true, | ||
| } | ||
| ); | ||
| const m = out.match(/ParentProcessId=(\d+)/); | ||
| if (!m) return null; | ||
| const n = Number(m[1]); | ||
| return Number.isInteger(n) && n > 0 ? n : null; | ||
| } |
There was a problem hiding this comment.
execFileSync blocks the Electron main process event loop
getParentPid uses execFileSync synchronously on macOS and Windows. Since resolveSessionFromPid is invoked inside dispatch() without await, each call holds the main process event loop hostage for the duration of ps (macOS) or wmic (Windows). With MAX_PID_WALK_HOPS = 5, the worst case is 5 consecutive execFileSync forks before failing — that's potentially 250 ms on macOS and well over 1 s on Windows. During that window every IPC channel is stalled.
The fix is to make getParentPid async (using execFile + promisify), make resolveSessionFromPidWalk async, thread await through dispatch, and type CoworkingSessionFromPidResolver as (pid: number) => Promise<string | null>. Linux is already safe since /proc reads complete in < 1 ms.
Surface unexpected IPC failures to global error tracking instead of silently swallowing them (sync path rolls back its optimistic cache first so the next effect run retries). Disable-clear stays best-effort to preserve loop isolation. Addresses PR RunMaestro#948 review feedback.
…read_browser) Mirror per-session browser-tab metadata into the coworking registry (registry- assigned stable browser:N ids) and add three read tools end to end: bridge methods, MCP server-script tools, sessionId-scoped tool fns, and a sender- validated main->renderer browser-op resolver. read_browser drives the live webview via a new BrowserTabView extract()/getMeta() handle, preferring an already-mounted (possibly hidden) tab and activating+restoring as a fallback (decision A'); list/get_url are metadata-only. Browser op contract lives in src/shared/coworkingBrowser.ts as the single source of truth.
Add state-changing browser tools (navigate/back/forward/reload/stop/click/ type/eval/screenshot) routed through a single gated browserInteract bridge method. Permission is a per-agent-type setting (coworkingBrowserInteraction) mirrored per-session into the registry and enforced in the bridge (hand- configured MCP clients cannot bypass the UI toggle). BrowserTabView gains navigate/reload/stop/executeJavaScript/capturePage handle primitives; the responder composes click/type/eval via executeJavaScript with JSON-escaped args and returns screenshots as MCP image content. CoworkingSetup gets a per-agent Interaction toggle (default off).
… coverage 22 new main-process tests (registry browser:N id assignment/scoping/permission, tools format+truncation, bridge dispatch + interaction permission gate + validateInteractionOp) and 8 renderer tests for the responder op->handle mapping (read/navigate/back/forward/reload/stop/click/type/eval/screenshot). Export applyBrowserOp as a test seam. 96 coworking tests pass.
Export resolveAndRun as a test seam and add coverage for the renderer resolution policy (decision A'): active-session guard returns ok:false for a non-focused agent, the mounted-handle fast path avoids stealing focus, and the activate-then-restore fallback mounts an unmounted tab and restores the prior active tab. 100 coworking tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/main/coworking/coworking-browser.test.ts (1)
184-209: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a tool-level cross-session
browserInteracttest.This suite covers cross-session rejection for
readBrowser, butbrowserInteractstill only checks the happy path and an unknown id. Because the bridge suite mocksbrowserInteract, a regression in the real session-scopedtabUuidlookup would slip through here even though this boundary is privacy-sensitive.Possible addition
+ it('browserInteract rejects cross-session access', async () => { + await expect( + browserInteract( + 's2', + { id: 'browser:1', op: { kind: 'reload' } }, + { + registry, + resolver: async (): Promise<BrowserOpResult> => ({ ok: true }), + } + ) + ).rejects.toThrow(/not found in your session/); + });🤖 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/__tests__/main/coworking/coworking-browser.test.ts` around lines 184 - 209, Add a tool-level cross-session browserInteract test to cover the real session-scoped tabUuid lookup path, not just the happy path and unknown id cases. Extend the existing browserInteract tests to call it with a browser id owned by a different session and assert it rejects, using browserInteract and the shared registry/resolver setup to verify the boundary behavior. This should mirror the cross-session coverage already present for readBrowser so a regression in tab resolution is caught even when the bridge suite mocks browserInteract.
🤖 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.
Nitpick comments:
In `@src/__tests__/main/coworking/coworking-browser.test.ts`:
- Around line 184-209: Add a tool-level cross-session browserInteract test to
cover the real session-scoped tabUuid lookup path, not just the happy path and
unknown id cases. Extend the existing browserInteract tests to call it with a
browser id owned by a different session and assert it rejects, using
browserInteract and the shared registry/resolver setup to verify the boundary
behavior. This should mirror the cross-session coverage already present for
readBrowser so a regression in tab resolution is caught even when the bridge
suite mocks browserInteract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02cd0985-e51d-4fd7-9a6b-df28dac95a5e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/__tests__/main/coworking/coworking-browser-bridge.test.tssrc/__tests__/main/coworking/coworking-browser.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
State-changing browser ops now route through a renderer-owned approval queue (coworkingApprovalStore + CoworkingApprovalHost) before execution: the responder awaits requestApproval and returns ok:false on decline. Cancel/close resolves false (cancel-safe, no hang). Per-agent policy coworkingBrowserInteractionConfirm (off | dangerous | all; default dangerous = confirm navigate + eval) configurable in CoworkingSetup. Interaction-op timeouts raised to 5min (bridge + main resolver) so a human has time to approve; reads stay 8s. 110 coworking tests pass.
Hide-from-agent: BrowserTab.hiddenFromAgent (persisted) + a per-tab eye toggle in the browser header; useCoworkingRegistrySync filters hidden tabs so list/read/ interaction never see them. Audit: every browser tool call is recorded (ok | error | denied) with agentType (mirrored per-session into the registry), redacted detail (urls/selectors truncated, eval code/typed text reduced to lengths, never page content), via a sink (system log + JSONL under userData). 92 main coworking tests pass.
…apped) Lets a Coworking agent read/drive its own browser tabs while the user is focused on a DIFFERENT agent, without stealing focus. - coworkingBackgroundBrowserStore: hidden-mount registry + handles, LRU cap (1-10), clear() on opt-out. - CoworkingBackgroundBrowsers host: renders requested tabs off-screen (fixed/aria-hidden/pointer-events:none, isActive=false); onUpdateTab routes to the OWNING session via normalizeBrowserTabUpdates. - resolveAndRun: non-focused requests resolve a background handle; approval uses the REQUESTING agent's policy; blank capturePage on a hidden webview returns ok:false instead of an empty image. - Settings: coworkingBackgroundBrowsers (default off) + ...Limit (default 2, clamped 1-10) across store/defaults/metadata/useSettings; global toggle + limit control in CoworkingSetup. Disabling clears live mounts. - Mounted in MainPanel. Tests: 6 store (dedup/LRU/cap/clear) + cross-session read/interact + blank-screenshot responder tests. 126 coworking tests pass.
The coworking MCP server is hand-rolled (no SDK import anywhere in src). Dropped from package.json and regenerated package-lock.json via `npm install --package-lock-only --ignore-scripts` (no native build), which also pruned its orphaned ajv/json-schema transitive subtree.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/MainPanel/MainPanel.tsx (1)
231-239: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake browser fallback selection session-aware.
Line 232 receives
sessionIdbut Line 235 routes through active-session-scopedonBrowserTabSelect. If the caller’s tab is not already mounted, a non-focused agent can fail to read/drive its own browser because fallback selection targets the focused session. Route this through a session-scoped selection path, or require/ensure the background mount exists before resolving.🤖 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/components/MainPanel/MainPanel.tsx` around lines 231 - 239, Browser fallback selection in selectBrowserTab is still routed through the active-session-scoped onBrowserTabSelect, so non-focused agents can’t recover their own tab when it isn’t mounted. Update MainPanel’s selectBrowserTab/useCoworkingBrowserResponder flow to use the sessionId argument and route selection through a session-scoped browser selection path, or ensure the background mount exists before calling the fallback; keep the fix centered on selectBrowserTab and onBrowserTabSelect.src/renderer/hooks/coworking/useCoworkingRegistrySync.ts (1)
156-159: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoll back
lastSessionIdsRefwhen sync fails.Line 159 commits
currentIdsbefore the IPC batch succeeds. If a removed session’sremoveSessionfails, the catch only clearslastPayloadRef, so the next run no longer includes that session inremovedand the main registry can keep stale tabs. Update the session-id cache only after success, or restore the previous set in the catch.🐛 Proposed fix
const payload = JSON.stringify({ perSession, perSessionBrowsers, removed }); if (payload === lastPayloadRef.current) return; + const previousSessionIds = lastSessionIdsRef.current; lastPayloadRef.current = payload; - lastSessionIdsRef.current = currentIds; (async () => { try { for (const sid of removed) { await bridge.removeSession(sid); @@ for (const { sessionId, inputs, interactionEnabled, agentType } of perSessionBrowsers) { await bridge.syncSessionBrowsers(sessionId, inputs, interactionEnabled, agentType); } + lastSessionIdsRef.current = currentIds; } catch (err) { lastPayloadRef.current = ''; + lastSessionIdsRef.current = previousSessionIds; reportIfUnexpected(err, 'sync'); } })();Also applies to: 172-178
🤖 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/coworking/useCoworkingRegistrySync.ts` around lines 156 - 159, In useCoworkingRegistrySync, the session-id cache is being updated before the IPC sync batch has actually succeeded, so a failed removeSession path can leave stale tabs unaccounted for on the next retry. Move the lastSessionIdsRef.current update so it only happens after the successful batch completion, or save and restore the previous session-id set in the catch block when the sync fails. Keep lastPayloadRef and the session-id cache in sync with the actual outcome of the IPC operations.
🧹 Nitpick comments (2)
src/renderer/hooks/coworking/useCoworkingBrowserResponder.ts (1)
198-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: approval is prompted even when the op can't run (non-focused tab, background browsing off).
For an interaction op targeting a non-focused tab while
coworkingBackgroundBrowsersis disabled, the user is asked to approve, and the op then fails with the "not live" message. Consider resolving liveness/host availability before prompting so users aren't shown an approval dialog for an action that cannot execute.🤖 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/coworking/useCoworkingBrowserResponder.ts` around lines 198 - 263, The approval prompt is happening before checking whether the target browser tab is actually runnable, so non-focused ops can ask for consent and then fail when background browsing is off. Update useCoworkingBrowserResponder to resolve tab liveness/host availability first using the existing isFocused and resolveBackgroundHandle flow, and only call requestApproval for state-changing ops after confirming the op can execute. Keep the early return path for the existing “Browser tab is not live” failure so users are not prompted for impossible actions.src/renderer/components/coworking/CoworkingBackgroundBrowsers.tsx (1)
67-68: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMemoize the per-key ref callback
The responder only reads background handles via async polling in
defaultResolveBackgroundHandle, so the briefnullfrom ref detach/reattach isn’t observable there. The inline callback still triggers twosetHandlewrites and extraMapcopies on every re-render; memoizing the ref callback perm.keyavoids that churn.🤖 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/components/coworking/CoworkingBackgroundBrowsers.tsx` around lines 67 - 68, The inline ref callback on BrowserTabView is recreated on every render, causing extra setHandle writes and Map churn even though defaultResolveBackgroundHandle only polls asynchronously. Memoize the per-key callback used in CoworkingBackgroundBrowsers so the same ref function is reused for each m.key, and keep the existing setHandle behavior unchanged while removing the unnecessary reattachment churn.
🤖 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/main/coworking/coworking-audit.ts`:
- Around line 52-55: The `redactBrowserOpDetail` helper is still logging
`navigate` URLs with query strings and fragments, which can leak tokens/PII into
the audit output and JSONL sink. Update the `navigate` branch to sanitize
`op.url` before truncating it: strip any search and hash components, then keep
the existing 200-character limit. Make the same redaction behavior apply
wherever `BrowserOp` details are serialized for the audit log so
`coworking-browser-audit.jsonl` never receives the full URL.
- Around line 77-79: The audit write in coworking-audit.ts is using a
synchronous main-thread disk call that can block Electron responsiveness. Update
the audit path in the audit entry handling logic to use asynchronous file
writing instead of appendFileSync, ideally by making the audit helper async and
awaiting the write so browser tool events do not stall the main process. Keep
the same file target and JSONL format, but ensure the write path in the audit
function is non-blocking and that any callers of the coworking audit entry flow
handle the async result appropriately.
In `@src/main/coworking/coworking-bridge.ts`:
- Around line 416-424: The audit detail for browser navigation is still exposing
sensitive URL parts, so update redactBrowserOpDetail(op) to fully scrub
browser_navigate payloads before auditedBrowserCall logs them. Ensure the URL is
normalized to origin/path only, or explicitly remove search and hash from the op
detail before it reaches the audit entry. Keep the change localized around
redactBrowserOpDetail and the browser_interact/auditedBrowserCall flow so all
navigation audits use the sanitized form.
In `@src/renderer/components/MainPanel/MainPanel.tsx`:
- Around line 1097-1098: The approval host is currently mounted only inside the
normal session view, so requests can’t surface a modal when MainPanel takes an
early-return branch like Log Viewer, Agent Sessions, Memory Viewer, or the empty
state. Move CoworkingApprovalHost out of the conditional render path in
MainPanel so it mounts in a parent scope that always survives those branches,
while keeping CoworkingBackgroundBrowsers where appropriate.
---
Outside diff comments:
In `@src/renderer/components/MainPanel/MainPanel.tsx`:
- Around line 231-239: Browser fallback selection in selectBrowserTab is still
routed through the active-session-scoped onBrowserTabSelect, so non-focused
agents can’t recover their own tab when it isn’t mounted. Update MainPanel’s
selectBrowserTab/useCoworkingBrowserResponder flow to use the sessionId argument
and route selection through a session-scoped browser selection path, or ensure
the background mount exists before calling the fallback; keep the fix centered
on selectBrowserTab and onBrowserTabSelect.
In `@src/renderer/hooks/coworking/useCoworkingRegistrySync.ts`:
- Around line 156-159: In useCoworkingRegistrySync, the session-id cache is
being updated before the IPC sync batch has actually succeeded, so a failed
removeSession path can leave stale tabs unaccounted for on the next retry. Move
the lastSessionIdsRef.current update so it only happens after the successful
batch completion, or save and restore the previous session-id set in the catch
block when the sync fails. Keep lastPayloadRef and the session-id cache in sync
with the actual outcome of the IPC operations.
---
Nitpick comments:
In `@src/renderer/components/coworking/CoworkingBackgroundBrowsers.tsx`:
- Around line 67-68: The inline ref callback on BrowserTabView is recreated on
every render, causing extra setHandle writes and Map churn even though
defaultResolveBackgroundHandle only polls asynchronously. Memoize the per-key
callback used in CoworkingBackgroundBrowsers so the same ref function is reused
for each m.key, and keep the existing setHandle behavior unchanged while
removing the unnecessary reattachment churn.
In `@src/renderer/hooks/coworking/useCoworkingBrowserResponder.ts`:
- Around line 198-263: The approval prompt is happening before checking whether
the target browser tab is actually runnable, so non-focused ops can ask for
consent and then fail when background browsing is off. Update
useCoworkingBrowserResponder to resolve tab liveness/host availability first
using the existing isFocused and resolveBackgroundHandle flow, and only call
requestApproval for state-changing ops after confirming the op can execute. Keep
the early return path for the existing “Browser tab is not live” failure so
users are not prompted for impossible actions.
🪄 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: 1c4db086-e1e7-420e-a403-e6918c24fbdc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
package.jsonsrc/__tests__/main/coworking/coworking-browser-bridge.test.tssrc/__tests__/renderer/hooks/coworking/applyBrowserOp.test.tssrc/__tests__/renderer/hooks/coworking/buildBrowserInputs.test.tssrc/__tests__/renderer/hooks/coworking/coworkingApproval.test.tssrc/__tests__/renderer/stores/coworkingBackgroundBrowserStore.test.tssrc/main/coworking/coworking-audit.tssrc/main/coworking/coworking-bridge.tssrc/main/coworking/coworking-registry.tssrc/main/coworking/coworking-server-script.tssrc/main/ipc/handlers/coworking.tssrc/main/preload/coworking.tssrc/main/stores/defaults.tssrc/renderer/components/MainPanel/BrowserTabView.tsxsrc/renderer/components/MainPanel/MainPanel.tsxsrc/renderer/components/Settings/CoworkingSetup.tsxsrc/renderer/components/coworking/CoworkingApprovalHost.tsxsrc/renderer/components/coworking/CoworkingBackgroundBrowsers.tsxsrc/renderer/global.d.tssrc/renderer/hooks/coworking/useCoworkingBrowserResponder.tssrc/renderer/hooks/coworking/useCoworkingRegistrySync.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/coworkingApprovalStore.tssrc/renderer/stores/coworkingBackgroundBrowserStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.tssrc/shared/coworkingBrowser.tssrc/shared/settingsMetadata.ts
💤 Files with no reviewable changes (1)
- package.json
Comment on lines
+52
to
+55
| export function redactBrowserOpDetail(op: BrowserOp): string { | ||
| switch (op.kind) { | ||
| case 'navigate': | ||
| return `url=${op.url.slice(0, 200)}`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strip query strings and fragments from audited navigate URLs.
Line 55 currently records op.url verbatim. OAuth callbacks, signed links, and app URLs routinely carry tokens or PII in the query/fragment, so this leaks secrets into both the Log Viewer and coworking-browser-audit.jsonl through Lines 73-79.
🔧 Suggested change
export function redactBrowserOpDetail(op: BrowserOp): string {
switch (op.kind) {
- case 'navigate':
- return `url=${op.url.slice(0, 200)}`;
+ case 'navigate': {
+ try {
+ const url = new URL(op.url);
+ return `url=${`${url.origin}${url.pathname}`.slice(0, 200)}`;
+ } catch {
+ return 'url=[invalid]';
+ }
+ }
case 'click':
return `selector=${op.selector.slice(0, 120)}`;Also applies to: 73-79
🤖 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/main/coworking/coworking-audit.ts` around lines 52 - 55, The
`redactBrowserOpDetail` helper is still logging `navigate` URLs with query
strings and fragments, which can leak tokens/PII into the audit output and JSONL
sink. Update the `navigate` branch to sanitize `op.url` before truncating it:
strip any search and hash components, then keep the existing 200-character
limit. Make the same redaction behavior apply wherever `BrowserOp` details are
serialized for the audit log so `coworking-browser-audit.jsonl` never receives
the full URL.
Source: Linters/SAST tools
Comment on lines
+77
to
+79
| try { | ||
| const file = path.join(app.getPath('userData'), 'coworking-browser-audit.jsonl'); | ||
| fs.appendFileSync(file, JSON.stringify(entry) + '\n'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid synchronous audit writes on the Electron main thread.
Lines 77-79 call appendFileSync for every browser tool audit event. A chatty agent repeatedly hitting browser tools will block the main process on disk I/O and can stall IPC/UI responsiveness.
🤖 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/main/coworking/coworking-audit.ts` around lines 77 - 79, The audit write
in coworking-audit.ts is using a synchronous main-thread disk call that can
block Electron responsiveness. Update the audit path in the audit entry handling
logic to use asynchronous file writing instead of appendFileSync, ideally by
making the audit helper async and awaiting the write so browser tool events do
not stall the main process. Keep the same file target and JSONL format, but
ensure the write path in the audit function is non-blocking and that any callers
of the coworking audit entry flow handle the async result appropriately.
Comment on lines
+416
to
+424
| const result = await auditedBrowserCall( | ||
| { | ||
| sessionId, | ||
| agentType, | ||
| tool: 'browser_interact', | ||
| opKind: op.kind, | ||
| detail: redactBrowserOpDetail(op), | ||
| }, | ||
| () => browserInteract(sessionId, { id, op }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scrub sensitive URL components before audit logging.
browser_navigate audit detail flows through redactBrowserOpDetail(op), which currently truncates the URL but can still persist query strings or fragments containing tokens/PII. Log origin/path or explicitly strip search/hash before writing audit entries.
🤖 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/main/coworking/coworking-bridge.ts` around lines 416 - 424, The audit
detail for browser navigation is still exposing sensitive URL parts, so update
redactBrowserOpDetail(op) to fully scrub browser_navigate payloads before
auditedBrowserCall logs them. Ensure the URL is normalized to origin/path only,
or explicitly remove search and hash from the op detail before it reaches the
audit entry. Keep the change localized around redactBrowserOpDetail and the
browser_interact/auditedBrowserCall flow so all navigation audits use the
sanitized form.
Comment on lines
+1097
to
+1098
| <CoworkingApprovalHost theme={theme} /> | ||
| <CoworkingBackgroundBrowsers theme={theme} /> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mount the approval host outside early-return panel branches.
These hosts are only rendered in the normal session view. If a browser interaction request arrives while Log Viewer, Agent Sessions, Memory Viewer, or the empty state is rendered, the approval queue has no modal host and the tool call waits until timeout. Move CoworkingApprovalHost to a parent that survives those branches.
🤖 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/components/MainPanel/MainPanel.tsx` around lines 1097 - 1098,
The approval host is currently mounted only inside the normal session view, so
requests can’t surface a modal when MainPanel takes an early-return branch like
Log Viewer, Agent Sessions, Memory Viewer, or the empty state. Move
CoworkingApprovalHost out of the conditional render path in MainPanel so it
mounts in a parent scope that always survives those branches, while keeping
CoworkingBackgroundBrowsers where appropriate.
Addresses PR review findings (reprioritized around the real risk surface: the agent pulling untrusted content into a privileged webview). Browser-tool safety: - eval now always requires per-call approval, even under confirm policy 'off'; type is added to the default 'dangerous' confirm set. - navigate is restricted to http/https/about:blank, blocking file:// (local-file read-back), javascript: and data: (a second eval path). - read_browser applies a default char cap (and a hard ceiling on an explicit maxChars); read_terminal clamps requested lines. Full sizes are still reported via totalChars/totalLines + truncated. Correctness: - serialize the registry-sync runs so an older async push can no longer overwrite a newer one and leave the main-process registry stale. - protect a background browser tab from LRU eviction while it has an op in flight, so a concurrent cross-session mount can't unmount its webview mid-run. Hygiene: - audit log records navigate URLs origin+path only (query/fragment, where tokens live, stripped); the JSONL sink is created 0600. - all four MCP installers now write config atomically (temp + rename) via a shared atomicWriteFile, so a crash mid-write can't truncate the user's ~/.claude.json / config.toml / opencode.json / mcp.json. - lock the bridge socket to 0600 on POSIX. - remove the unused upsertTerminal/removeTerminal IPC + preload surface (registry methods kept; they are the test seam). - reuse getWhichCommand(); replace em-dashes in coworking source; add a searchable entry + data-setting-id for background browsing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/coworking/pid-resolution.ts (1)
19-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the Windows
wmicfallback — WMIC is disabled by default on Windows 11 23H2/24H2, so stock installs will hit thecatchpath and reject theppidhandshake. UseGet-CimInstance(or another maintained resolver) as the primary path and keepwmiconly as a legacy fallback.🤖 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/main/coworking/pid-resolution.ts` around lines 19 - 70, The Windows parent-PID lookup in getParentPid currently relies on wmic, which is no longer available on many stock Windows installs and causes the handshake path to fail. Update the Windows branch in src/main/coworking/pid-resolution.ts to use a maintained resolver such as Get-CimInstance as the primary mechanism, and keep wmic only as a legacy fallback for older systems. Preserve the existing validation and null-on-failure behavior in getParentPid while ensuring the Windows path succeeds on current Windows versions.
🧹 Nitpick comments (1)
src/main/utils/atomic-json-store.ts (1)
75-86: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
options.modewon't be applied if a stale.tmpfile already exists.
fs.writeFile'smodeoption only takes effect when the file is newly created; if${filePath}.tmpsurvives from a prior crash/interrupted write (or was previously written withoutmode), this call truncates it in place and keeps its existing permission bits, silently skipping the intended restriction for permission-sensitive payloads.🔧 Proposed fix
const tmp = `${filePath}.tmp`; - await fs.writeFile( - tmp, - contents, - options?.mode !== undefined ? { encoding: 'utf-8', mode: options.mode } : 'utf-8' - ); + await fs.writeFile(tmp, contents, 'utf-8'); + if (options?.mode !== undefined) { + await fs.chmod(tmp, options.mode); + }🤖 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/main/utils/atomic-json-store.ts` around lines 75 - 86, The atomicWriteFile flow is reusing a pre-existing temporary file, so fs.writeFile may ignore options.mode when ${filePath}.tmp already exists. Update atomicWriteFile to ensure the temp path is always created with the intended permissions before writing, either by removing any stale .tmp file or by opening it in a way that forces a new file with the requested mode. Keep the fix localized to atomicWriteFile and preserve the existing rename-based atomic swap behavior.
🤖 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.
Outside diff comments:
In `@src/main/coworking/pid-resolution.ts`:
- Around line 19-70: The Windows parent-PID lookup in getParentPid currently
relies on wmic, which is no longer available on many stock Windows installs and
causes the handshake path to fail. Update the Windows branch in
src/main/coworking/pid-resolution.ts to use a maintained resolver such as
Get-CimInstance as the primary mechanism, and keep wmic only as a legacy
fallback for older systems. Preserve the existing validation and null-on-failure
behavior in getParentPid while ensuring the Windows path succeeds on current
Windows versions.
---
Nitpick comments:
In `@src/main/utils/atomic-json-store.ts`:
- Around line 75-86: The atomicWriteFile flow is reusing a pre-existing
temporary file, so fs.writeFile may ignore options.mode when ${filePath}.tmp
already exists. Update atomicWriteFile to ensure the temp path is always created
with the intended permissions before writing, either by removing any stale .tmp
file or by opening it in a way that forces a new file with the requested mode.
Keep the fix localized to atomicWriteFile and preserve the existing rename-based
atomic swap behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6407048b-a07d-4c0c-9144-966446c179d8
📒 Files selected for processing (24)
src/main/coworking/coworking-audit.tssrc/main/coworking/coworking-bridge.tssrc/main/coworking/coworking-installer.tssrc/main/coworking/coworking-registry.tssrc/main/coworking/coworking-server-paths.tssrc/main/coworking/coworking-server-script.tssrc/main/coworking/coworking-session-id.tssrc/main/coworking/coworking-tools.tssrc/main/coworking/installers/claude-code.tssrc/main/coworking/installers/codex.tssrc/main/coworking/installers/factory-droid.tssrc/main/coworking/installers/opencode.tssrc/main/coworking/pid-resolution.tssrc/main/ipc/handlers/coworking.tssrc/main/preload/coworking.tssrc/main/utils/atomic-json-store.tssrc/renderer/components/Settings/CoworkingSetup.tsxsrc/renderer/components/Settings/searchableSettings.tssrc/renderer/global.d.tssrc/renderer/hooks/coworking/useCoworkingBrowserResponder.tssrc/renderer/hooks/coworking/useCoworkingBufferResponder.tssrc/renderer/hooks/coworking/useCoworkingRegistrySync.tssrc/renderer/stores/coworkingBackgroundBrowserStore.tssrc/shared/coworkingBrowser.ts
💤 Files with no reviewable changes (3)
- src/renderer/global.d.ts
- src/main/ipc/handlers/coworking.ts
- src/main/preload/coworking.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/components/Settings/searchableSettings.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/main/coworking/coworking-server-paths.ts
- src/main/coworking/installers/claude-code.ts
- src/main/coworking/installers/codex.ts
- src/main/coworking/coworking-audit.ts
- src/main/coworking/coworking-installer.ts
- src/main/coworking/installers/opencode.ts
- src/renderer/components/Settings/CoworkingSetup.tsx
- src/renderer/hooks/coworking/useCoworkingRegistrySync.ts
- src/renderer/hooks/coworking/useCoworkingBrowserResponder.ts
- src/main/coworking/installers/factory-droid.ts
- src/main/coworking/coworking-bridge.ts
- src/main/coworking/coworking-registry.ts
- src/main/coworking/coworking-server-script.ts
eval is now force-confirmed regardless of policy (even 'off'), and type is part of the default 'dangerous' confirm set. Aligns the two browserOpNeedsConfirm unit tests with the shipped behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jSydorowicz21
added a commit
to jSydorowicz21/Maestro
that referenced
this pull request
Jul 2, 2026
…ng branch Merges fork-chr1syy/feat/agentic-coworking (74e322f harden + ca5bb60 tests) into our superseding branch. Both branches independently hardened the same coworking review surface; reconciled hunk-by-hunk rather than taking either side wholesale. Brought in from upstream (additive): - eval force-confirm regardless of policy (FORCE_CONFIRM_KINDS); type added to the dangerous confirm set - navigate scheme allowlist (http/https/about:blank only) — ALSO applied to our newTab, closing a file:// local-read exfiltration path - read caps: read_browser default+ceiling char cap, read_terminal line clamp (totalChars/totalLines still report full size) - registry-sync run serialization; background-tab LRU eviction guard - atomic installer writes (temp+rename); 0600 on bridge socket + audit JSONL - searchable-settings entries + data-setting-id for background browsing Reconciled (both touched — kept superior): - audit URL redaction: kept ours (origin+path + queryChars/hashChars + <non-url textChars>); adopted their 0600 sink; deleted their now-dead redactAuditUrl - confirm policy: UNION dangerous set = navigate/eval/type/newTab/closeTab; adopted their FORCE_CONFIRM_KINDS + restructured browserOpNeedsConfirm; kept our BrowserOp additions (waitFor/newTab/closeTab/read.selector) - registry-sync: their serialization + our cache-restore-on-failure + confirmPolicy/hiddenFromAgent push - CoworkingSetup: kept our redesign, grafted their data-setting-id; relabeled confirm 'Never'->'JS only' and fixed descriptions for eval-force-confirm Preserved (ours only): ephemeral tabs, clearSessionData, main-side hidden enforcement, confirm mirror + boundary recheck, wmic->PowerShell-CIM fallback. Verified: typecheck (lint+main) green; coworking/settings slice 545 tests green (Tester reconciled the confirm contract + read caps, both polarities); MCP stdio smoke SMOKE-PASS (17 tools, newTab/waitFor/selector forwarded).
Contributor
Author
|
Closing as superseded by #1160 ( |
jSydorowicz21
added a commit
that referenced
this pull request
Jul 4, 2026
…ols, new agent tools Fixes from the #948 feature-completeness vet: - Ephemeral (incognito) browser tabs: maestro-ephemeral-<sid>-<rand8> in-memory partition, New Incognito Browser entry, tab-strip badge, never persisted/restored; window-manager allowlist extended deliberately - Clear-browsing-data: hardened browser:clearSessionData IPC (full-pattern partition validation + window-sender gate) with two-step confirm toolbar button - hiddenFromAgent now crosses IPC and is enforced in the MAIN registry (list AND resolve; hidden == not-found) plus an execution-boundary re-check in the renderer; browser:N ids stay stable across hide/unhide - Per-agent confirm policy mirrored to main; main computes needsConfirm per op and the renderer ORs it with local policy (stale settings can never weaken the gate) - Audit log: navigate/newTab URLs stripped to origin+path (query/hash -> char counts), non-URL targets length-only; terminal tools now audited too - Windows ppid walk falls back wmic -> PowerShell CIM with backend caching (wmic is absent on newer Win11 builds; validated live on this host) New agent tools (17 total advertised): - browser_wait_for(id, selector, timeoutMs<=30s) - host-side element polling - browser_new_tab(url?, ephemeral?) - session-scoped, URL normalized like navigate, always confirm-gated; browser_close_tab(id) - confirm-gated store close - read_browser gains selector for element-scoped reads Verified: typecheck green (lint+main tsconfigs); 25 test files / 449 tests green (54 new/rewritten by test pass); generated MCP server script node --check clean and driven end-to-end over stdio against a mock bridge (SMOKE-PASS); codex review run, 2 findings fixed, 2 declined with rationale
jSydorowicz21
added a commit
that referenced
this pull request
Jul 5, 2026
…rdened feat(coworking): browser tooling hardening, session-persistence controls, new agent tools (supersedes #948)
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Coworking Encore Feature: agents can read terminal scrollback and inspect/drive in-app browser tabs on demand via a per-agent MCP server. Opt-in via Settings -> Encore Features -> Coworking -> Install for [agent].
maestro-coworkingMCP entry to each agent's user-level config (~/.claude.json,~/.codex/config.toml,~/.config/opencode/opencode.json,~/.factory/mcp.json). Install once, persists. Node is resolved to an absolute path at install time (falls back tonodeon PATH) so GUI-launched agents under nvm/fnm/volta still find a runtime.term:Npill so users can address them by name.list_terminals(),read_terminal(id, lines?).list_browsers(),get_browser_url(id),read_browser(id, format?, maxChars?)whereformatistext | innerText | html.browser_navigate(id, url),browser_back/forward/reload/stop(id),browser_click(id, selector),browser_type(id, selector, text),browser_eval(id, code),browser_screenshot(id).Architecture
<userData>/coworking-mcp-server.json everyapp.ready. Stable absolute path survives Maestro upgrades. Advertises all tools; the agent's MCP client owns the subprocess lifecycle.browser:Nid assigned by the registry (no renderer-side schema change needed).coworking:requestBrowserOpchannel (8s timeout, under the MCP server's 10s per-RPC cap).list_browsers/get_browser_urlare metadata-only and never touch a webview.Browser resolution decision (activation vs background mount)
rc already keeps the active agent's browser tabs mounted (visible or hidden) via
useBrowserTabMounting(thebrowserTabKeepAlivesetting). We chose (A') prefer an already-mounted webview handle; activate the target tab only as a fallback:BrowserTabViewhandle map (lifted fromMainPanelContenttoMainPanel). If the tab is already mounted (even hidden via keep-alive), we use it directly with no focus change.ok:false(metadata tools always work cross-session). See Hardening follow-ups.Hidden multi-webview mounting for non-focused sessions (option B) is now available as an opt-in, capped background host (default off) - see Hardening follow-ups. The focused-agent already-mounted fast path remains the default and the only path when the host is off.
Files (high level)
src/shared/coworkingBrowser.ts- single source of truth for the browser op/result/input/entry contract (shared across main, preload, renderer).src/main/coworking/*- registry (browser records +browser:Nids + interaction permission + agentType), tools (listBrowsers/getBrowserUrl/readBrowser/browserInteract+ resolver), bridge dispatch +validateInteractionOp, server-script (browser tools inTOOLS+tools/call), audit log.src/main/ipc/handlers/coworking.ts,src/main/preload/coworking.ts- browser sync + sender-validated browser-op round-trip.src/renderer/...-useCoworkingRegistrySyncmirrors browser tabs + per-agent interaction flag + hides flagged tabs;useCoworkingBrowserResponderresolves the live (or background) webview and runs ops;BrowserTabViewgainsextract/getMeta/navigate/reload/stop/executeJavaScript/capturePage+ a hide-from-agent toggle;CoworkingSetupgets per-agent Interaction + confirm-policy controls and a global background-browsing toggle.coworkingBrowserInteraction: string[],coworkingBrowserInteractionConfirm: Record<string, policy>,coworkingBackgroundBrowsers+...Limitacross settingsStore/defaults/settingsMetadata/useSettings.Hardening follow-ups (this update)
Layered on top of the read + interaction tools above:
browser_navigate/back/forward/reload/stop/click/type/eval/screenshot, the renderer can require explicit user approval via a cancel-safe confirm dialog. Policy is per-agent (off | dangerous | all, defaultdangerous= confirmnavigate/eval), surfaced in the Coworking Setup panel. Interaction-op timeouts were raised (server bridge 330s / main resolver 300s) so a human has time to respond; reads stay at 8s.hiddenFromAgentflag onBrowserTabwith an eye toggle in the browser toolbar. Hidden tabs are filtered out oflist_browsers/read_browser/ interaction at the registry-sync boundary, so agents can neither see nor address them.ok | error | denied) to the system log + a JSONL underuserData. URLs/selectors are truncated and page content / eval code is never logged (lengths only).coworkingBackgroundBrowsers, default off;coworkingBackgroundBrowsersLimitdefault 2, clamped 1-10) mounts a non-focused agent's tab off-screen - its ownpartitionpreserved,isActive=false,aria-hidden, pointer-events off, no focus steal - so read + interaction work without switching the user's active agent. Disabling the setting releases every hidden webview.capturePageon a hidden webview returns a clearok:falserather than a blank image.@modelcontextprotocol/sdkdependency (see Notes).New files:
coworkingApprovalStore.ts,CoworkingApprovalHost.tsx,CoworkingBackgroundBrowsers.tsx,coworkingBackgroundBrowserStore.ts,coworking-audit.ts.Notes
@modelcontextprotocol/sdkwas declared but unused (the server is hand-rolled); it has been removed frompackage.jsonand the lockfile regenerated (npm install --package-lock-only).Test plan
browser:Nids / scoping / interaction permission + hidden-tab filtering + agentType; tools + read truncation; bridge dispatch + interaction gate + op validation + audit; renderer responder op->handle mapping incl. the cross-session background path; per-call approval store + policy; background-host LRU / cap / clear).initialize/tools/list(all 12 tools) /tools/callforlist_browsers,read_browser, plus an interaction op that proceeds when approved and surfaces a decline as an MCP error.list_browsers/read_browser/ (with Interaction on)browser_navigate+browser_click, confirm the approval dialog, and confirm the page changes.Deferred (out of scope for this PR)
Summary by CodeRabbit