Add in-app browser host and surface management#146
Conversation
- Wire desktop, web, and server RPC plumbing for hosted browser sessions - Add navigation, input, storage, cookie, and profile management
📝 WalkthroughWalkthroughThis PR adds browser control-plane contracts and helpers, server authentication and RPC wiring, desktop host/surface plumbing, web browser-panel routing and UI, provider browser runtime tools, and supporting config, tests, and docs. ChangesBuilt-in in-app browser
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
github-actions
Bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
apps/desktop/src/browser/BrowserKernel.ts-31-34 (1)
31-34: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClean up temporary profile storage when closing hosted sessions.
BrowserProfiles.cleanupTemporary()is never called becauseHostedSessiondrops theBrowserProfile; non-persistent partitions remain cached insessionsByProfilewith cookies/storage and can be reused for the sameprofileId.Proposed fix
interface HostedSession { session: BrowserSessionSnapshot; + profile: BrowserProfile; view: WebContentsView; }- const hosted: HostedSession = { session: { ...session, status: "ready" }, view }; + const hosted: HostedSession = { session: { ...session, status: "ready" }, profile, view };this.sessions.delete(sessionId); - hosted.view.webContents.close({ waitForBeforeUnload: false }); + try { + hosted.view.webContents.close({ waitForBeforeUnload: false }); + } finally { + await this.profiles.cleanupTemporary(hosted.profile); + }Also applies to: 223-234
🤖 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 `@apps/desktop/src/browser/BrowserKernel.ts` around lines 31 - 34, The hosted-session lifecycle is losing the BrowserProfile reference, so temporary profiles never get cleaned up and remain cached in sessionsByProfile. Update HostedSession and the BrowserKernel session management flow to retain the BrowserProfile alongside the BrowserSessionSnapshot/WebContentsView, then call BrowserProfiles.cleanupTemporary() when a non-persistent hosted session is closed or removed. Make sure the cleanup path uses the stored profile identity so temporary storage is released consistently.apps/desktop/src/browser/BrowserKernel.ts-226-227 (1)
226-227: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not report
open_sessionsuccess after the initial navigation fails.The current
.catch(() => undefined)hidesloadURLfailures, so the backend receivesok: trueeven when the requested initial page was never loaded.Proposed fix
- await view.webContents.loadURL(initialUrl ?? "about:blank").catch(() => undefined); + await view.webContents.loadURL(initialUrl ?? "about:blank"); return this.updateSnapshotFromWebContents(hosted);🤖 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 `@apps/desktop/src/browser/BrowserKernel.ts` around lines 226 - 227, The initial navigation in BrowserKernel’s session startup is swallowing loadURL failures, which allows open_session to be reported as successful even when the page never loaded. Update the logic around BrowserKernel’s loadURL/updateSnapshotFromWebContents flow to stop hiding the error, and ensure the caller only returns success after the initial navigation completes successfully; if loadURL fails, propagate or convert that failure so open_session is not marked ok.apps/desktop/src/browser/BrowserKernel.ts-563-585 (1)
563-585: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch event-sink failures from WebContents listeners.
void this.emit(...)drops rejected promises from the async event sink; a transient RPC/WS publishing failure can become an unhandled rejection in the Electron main process.Proposed fix
- void this.emit({ + void this.emit({ type: "session.updated", session: this.updateSnapshotFromWebContents(hosted), createdAt: new Date().toISOString(), - }); + }).catch(() => undefined);- void this.emit({ + void this.emit({ type: "tab.crashed", sessionId: hosted.session.sessionId, tabId: tab.tabId, reason: details.reason, createdAt: new Date().toISOString(), - }); + }).catch(() => undefined);🤖 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 `@apps/desktop/src/browser/BrowserKernel.ts` around lines 563 - 585, The WebContents listeners in installWebContentsListeners are ignoring failures from the async event sink because void this.emit(...) discards rejected promises. Update the update callback and the render-process-gone handler to catch and handle emit failures explicitly, using this.emit and a .catch(...) path (or equivalent) so transient publishing errors don’t become unhandled rejections in BrowserKernel.apps/desktop/src/browser/BrowserHostConnection.ts-68-101 (1)
68-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTear down partial startup state on failed connect/register.
If client creation on Line 77 or
registeron Lines 79-94 rejects,this.runtimeandthis.clientScopestay live and the socket/scope never get disposed because onlystop()performs cleanup. Wrap the startup sequence intry/catch, callawait this.stop(), then rethrow.Suggested fix
async start(input: { readonly wsBaseUrl: string; readonly token: string }): Promise<void> { await this.stop(); this.commandStreamAbort = false; - const runtime = ManagedRuntime.make( - browserHostProtocolLayer(browserHostWsUrl(input.wsBaseUrl), input.token), - ); - this.runtime = runtime; - const clientScope = runtime.runSync(Scope.make()); - this.clientScope = clientScope; - const client = await runtime.runPromise(Scope.provide(clientScope)(makeBrowserHostRpcClient)); - this.client = client; - await runtime.runPromise( - client[BROWSER_HOST_METHODS.register]({ - hostId: this.hostId, - runId: this.runId, - capabilities: { - surface: true, - persistentProfiles: true, - temporaryProfiles: true, - screenshots: false, - domSnapshot: true, - input: true, - downloads: false, - devtools: false, - }, - }), - ); - this.startCommandStream(client); - this.heartbeatTimer = setInterval(() => { - void this.heartbeat().catch((error) => { - console.warn("[desktop-browser-host] heartbeat failed", error); - }); - }, 5_000); - this.heartbeatTimer.unref(); + try { + const runtime = ManagedRuntime.make( + browserHostProtocolLayer(browserHostWsUrl(input.wsBaseUrl), input.token), + ); + this.runtime = runtime; + const clientScope = runtime.runSync(Scope.make()); + this.clientScope = clientScope; + const client = await runtime.runPromise(Scope.provide(clientScope)(makeBrowserHostRpcClient)); + this.client = client; + await runtime.runPromise( + client[BROWSER_HOST_METHODS.register]({ + hostId: this.hostId, + runId: this.runId, + capabilities: { + surface: true, + persistentProfiles: true, + temporaryProfiles: true, + screenshots: false, + domSnapshot: true, + input: true, + downloads: false, + devtools: false, + }, + }), + ); + this.startCommandStream(client); + this.heartbeatTimer = setInterval(() => { + void this.heartbeat().catch((error) => { + console.warn("[desktop-browser-host] heartbeat failed", error); + }); + }, 5_000); + this.heartbeatTimer.unref(); + } catch (error) { + await this.stop(); + throw error; + } }🤖 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 `@apps/desktop/src/browser/BrowserHostConnection.ts` around lines 68 - 101, The BrowserHostConnection.start flow leaves partial state behind if makeBrowserHostRpcClient or the BROWSER_HOST_METHODS.register call fails, because runtime and clientScope are assigned before the full startup completes. Update start to wrap the client creation/register sequence in a try/catch, and on any failure call await this.stop() to dispose the runtime, scope, and socket before rethrowing. Keep the fix centered in BrowserHostConnection.start and use the existing stop(), runtime, clientScope, and startCommandStream setup as the cleanup boundary.apps/desktop/src/browser/BrowserHostConnection.ts-151-175 (1)
151-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep one failed command from killing the subscription.
A rejection from
this.handleCommand(envelope)on Line 161 orcommandResult(...)on Lines 162-168 escapesStream.runForEach, so the catch on Lines 172-175 logs once and the host stops consuming every later command. Catch failures inside the per-envelope handler and keep the stream alive.Suggested fix
Stream.runForEach( client[BROWSER_HOST_METHODS.subscribeCommands]({ hostId: this.hostId, runId: this.runId, }), (envelope) => Effect.promise(async () => { if (this.commandStreamAbort) return; - const result = await this.handleCommand(envelope); - await runtime.runPromise( - client[BROWSER_HOST_METHODS.commandResult]({ - hostId: this.hostId, - runId: this.runId, - result, - }), - ); + try { + const result = await this.handleCommand(envelope); + if (this.commandStreamAbort) return; + await runtime.runPromise( + client[BROWSER_HOST_METHODS.commandResult]({ + hostId: this.hostId, + runId: this.runId, + result, + }), + ); + } catch (error) { + console.warn("[desktop-browser-host] command failed", error); + } }), ), )🤖 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 `@apps/desktop/src/browser/BrowserHostConnection.ts` around lines 151 - 175, The per-envelope handler in BrowserHostConnection’s subscribeCommands stream is letting errors from handleCommand(envelope) or client[BROWSER_HOST_METHODS.commandResult](...) escape Stream.runForEach, which stops the subscription after one failure. Wrap the logic inside the Effect.promise handler with its own try/catch (or equivalent failure handling) so each envelope logs or reports the failure and then returns without rejecting the outer stream, keeping the command subscription alive for later messages.apps/web/src/components/BrowserPanel.tsx-421-423 (1)
421-423: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep transient command errors out of
status.
statusalso controls native surface attachment. A bad URL or a failed back/reload/open-external call flips the panel to"error", which tears down the surface; later successful control actions usually do not restore"ready", so the browser view can stay blank until remount. Track command errors separately from availability/session lifecycle.Also applies to: 523-525, 597-639, 726-732, 869-873
🤖 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 `@apps/web/src/components/BrowserPanel.tsx` around lines 421 - 423, Transient command failures are incorrectly being stored in BrowserPanelStatus and that can tear down the native surface; keep command errors separate from the lifecycle state. Update BrowserPanel in the status/message/session state flow so commands like back, reload, and open-external no longer set status to "error" unless the panel is truly unavailable, and instead store those failures in a separate error field. Make sure the lifecycle-driven status transitions in the BrowserPanel controls preserve "ready" once the session is healthy, using the existing BrowserPanelStatus, setStatus, setMessage, and BrowserSessionSnapshot handling.apps/web/src/components/BrowserPanel.tsx-455-521 (1)
455-521: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the BrowserHost session in the effect cleanup.
openSession()runs every time the route target changes, but the cleanup only removes the event subscription. Closing the panel or switching threads leaves the previous browser session alive, which can accumulate orphaned tabs/processes and stale project-scoped sessions.Suggested fix
useEffect(() => { let cancelled = false; let unsubscribe: (() => void) | undefined; + let sessionApi: typeof browserApi | undefined; + let openedSessionId: string | null = null; const openSession = async () => { @@ const currentBrowserApi = readEnvironmentApi(environmentId)?.browser; + sessionApi = currentBrowserApi; @@ const snapshot = await currentBrowserApi.openSession({ threadId, ...(projectId ? { projectId } : {}), profileMode: "project", }); if (cancelled) return; + openedSessionId = snapshot.sessionId; setSession(snapshot); setStatus("ready"); setMessage(null); @@ return () => { cancelled = true; unsubscribe?.(); + if (sessionApi && openedSessionId) { + void sessionApi.closeSession({ sessionId: openedSessionId }).catch(() => undefined); + } }; }, [environmentId, projectId, threadId]);🤖 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 `@apps/web/src/components/BrowserPanel.tsx` around lines 455 - 521, The BrowserPanel effect only unsubscribes from events and never closes the active BrowserHost session, so sessions can leak when environmentId/projectId/threadId changes or the component unmounts. Update the cleanup in the BrowserPanel useEffect/openSession flow to call the browser API’s session-close/teardown method for the current session (alongside unsubscribe), and make sure it runs safely even when the async openSession is still in flight. Use the existing openSession, currentBrowserApi, and cleanup return function as the place to wire this in.packages/shared/src/browser.ts-46-53 (1)
46-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't rewrite explicit non-browser schemes into
https://….
mailto:user@example.comcurrently falls through this branch and becomes a synthetic HTTPS URL, sonormalizeBrowserNavigationUrlreturns{ ok: true }instead of rejecting it. The same branch also misses mixed-case explicit schemes likeABOUT:blank. Please preserve genuine explicit schemes here and only auto-prefix bare host / host:port inputs.🤖 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 `@packages/shared/src/browser.ts` around lines 46 - 53, The URL normalization logic in normalizeBrowserNavigationUrl is over-prompting bare defaults and accidentally rewriting explicit non-browser schemes like mailto: and mixed-case schemes like ABOUT:blank. Update the explicit scheme detection and the candidate selection in browser.ts so any genuine scheme is preserved as-is, and only bare host or host:port inputs get an auto-prefixed default scheme; ensure the explicitScheme check handles case-insensitive matches before deciding whether to synthesize https://.apps/server/src/provider/tools/BrowserRuntimeTool.ts-56-66 (1)
56-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the
"browser_open"argument name with what the executor actually reads.The tool definition advertises
initialUrl, butBrowserRuntimeToolCallInputand the executor useurl. Calls that follow the advertised schema will open the session without navigating.Suggested fix
{ name: "browser_open", description: "Open or focus the isolated Ryco browser session for the current thread.", input: { type: "object", properties: { - initialUrl: { type: "string" }, + url: { type: "string" }, }, additionalProperties: false, }, },Also applies to: 159-170
🤖 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 `@apps/server/src/provider/tools/BrowserRuntimeTool.ts` around lines 56 - 66, The browser_open tool schema is out of sync with the executor input shape: BROWSER_RUNTIME_TOOL_DEFINITIONS advertises initialUrl while BrowserRuntimeToolCallInput and the browser_open execution path read url. Update the tool definition and any related input typing/default handling so the same argument name is used end-to-end, and make sure the browser_open executor continues to navigate using that shared field.packages/contracts/src/browser.ts-475-484 (1)
475-484: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNarrow
datato a JSON-safe shape before it crosses RPC.
BrowserCommandResultis sent over the browser-host JSON websocket, butSchema.Unknownaccepts values the transport cannot encode reliably. A host can pass schema validation here and still fail at serialization time.🤖 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 `@packages/contracts/src/browser.ts` around lines 475 - 484, The BrowserCommandResultPayload schema currently allows data to be Schema.Unknown, which can pass validation but break when BrowserCommandResult is serialized over the browser-host JSON websocket. Update the BrowserCommandResultPayload definition to use a JSON-safe schema for data instead of Schema.Unknown, and keep the change localized to the BrowserCommandResultPayload/BrowserCommandResult path so the RPC boundary only accepts encodable values.packages/contracts/src/browser.ts-209-215 (1)
209-215: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReuse the bounded URL schema for all initial-navigation paths.
browser.navigatecapsurlat 8,192 chars, butBrowserOpenSessionInput.initialUrland the"open_session"host command still accept an unbounded string. That lets callers bypass the only URL-size guard just by putting the first navigation intoopenSession.Suggested contract fix
+const BrowserUrl = Schema.String.check(Schema.isMaxLength(8_192)); + export const BrowserOpenSessionInput = Schema.Struct({ threadId: ThreadId, projectId: Schema.optional(ProjectId), profileMode: Schema.optional(BrowserProfileMode), profileName: Schema.optional(TrimmedNonEmptyStringSchema), - initialUrl: Schema.optional(Schema.String), + initialUrl: Schema.optional(BrowserUrl), }); @@ export const BrowserNavigateInput = Schema.Struct({ sessionId: BrowserSessionId, tabId: Schema.optional(BrowserTabId), - url: Schema.String.check(Schema.isMaxLength(8_192)), + url: BrowserUrl, }); @@ Schema.Struct({ kind: Schema.Literal("open_session"), session: BrowserSessionSnapshot, profile: BrowserProfile, - initialUrl: Schema.optional(Schema.String), + initialUrl: Schema.optional(BrowserUrl), }),Also applies to: 229-233, 393-398
🤖 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 `@packages/contracts/src/browser.ts` around lines 209 - 215, Reuse the bounded URL schema for every initial-navigation entry point so callers cannot bypass the URL length guard through session startup. Update BrowserOpenSessionInput.initialUrl and the open_session host command schema to use the same capped URL schema that browser.navigate uses, and make sure any session-opening flow passes through that shared validator. Locate the affected definitions around BrowserOpenSessionInput and the open_session command contract, plus any related initial-navigation schema uses.packages/contracts/src/browserHostRpc.ts-57-70 (1)
57-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister
BrowserHostEventsRpcin the routed RPC group.
BrowserHostEventsRpcis declared, butBrowserHostRpcGroupomits it. Since/browser-host/wsserves onlyBrowserHostRpcGroup,"browserHost.events"is currently unreachable.Suggested fix
export const BrowserHostRpcGroup = RpcGroup.make( BrowserHostRegisterRpc, BrowserHostHeartbeatRpc, BrowserHostSubscribeCommandsRpc, BrowserHostCommandResultRpc, BrowserHostEventRpc, + BrowserHostEventsRpc, );🤖 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 `@packages/contracts/src/browserHostRpc.ts` around lines 57 - 70, `BrowserHostEventsRpc` is defined but not included in `BrowserHostRpcGroup`, so the `browserHost.events` route is never exposed. Update the `BrowserHostRpcGroup.make(...)` registration in `browserHostRpc.ts` to add `BrowserHostEventsRpc` alongside the existing browser host RPCs, so the `/browser-host/ws` server can route this event stream correctly.apps/server/src/browser/BrowserHostRegistry.ts-130-136 (1)
130-136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace, don’t merge, session state during host re-registration/snapshot heartbeats.
register()preservesprevious.sessions, andheartbeat()only upsertsinput.sessions. If the desktop host restarts or reconnects with a smaller snapshot, sessions that disappeared on the host remain in registry state indefinitely. That leavessnapshot()serving stale sessions/tabs and can route later commands against sessions the host no longer owns.Also applies to: 151-160
🤖 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 `@apps/server/src/browser/BrowserHostRegistry.ts` around lines 130 - 136, The host re-registration and heartbeat flow in BrowserHostRegistry is merging session state instead of replacing it, which leaves stale sessions behind when the host reconnects with a smaller snapshot. Update register() and heartbeat() so the registry replaces the stored sessions with the host’s current snapshot rather than preserving previous.sessions or only upserting input.sessions; keep failPending(previous.pending, staleHostError()) and state reset behavior, but ensure snapshot() reflects only the sessions currently reported by the active host.apps/server/src/browser/BrowserHostRegistry.ts-97-98 (1)
97-98: 🩺 Stability & Availability | 🟠 MajorStale
commandStreaminstances steal and drop commands from the shared queue.The
commandQueue(line 97) is a single shared bounded queue. Each call tocommandStream()(e.g., lines 291-295) creates a newStream.fromQueue(commandQueue)consumer. Because theStream.filtervalidatingrunIdexecutes after the item is dequeued, a stale subscriber from a previous host session can pull a command intended for the current run and discard it, causing the active subscriber to miss the command and the caller to time out.Fix by ensuring commands are routed to specific listeners before queue consumption, such as using per-run queues or a
PubSubmechanism that broadcasts to matching filters.🤖 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 `@apps/server/src/browser/BrowserHostRegistry.ts` around lines 97 - 98, The shared commandQueue is being consumed by multiple commandStream() subscribers, so a stale BrowserHostRegistry stream can dequeue and drop commands meant for the active run. Update BrowserHostRegistry so commands are routed to the correct listener before consumption, using per-run queues or a PubSub-based fanout instead of Stream.fromQueue(commandQueue) with a post-dequeue runId filter. Make sure the commandStream() path only receives commands for its own runId and cannot steal items from another session.apps/server/src/ws.ts-92-108 (1)
92-108: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftClosed browser-host sockets never clear the registered host.
This route authenticates and hands off the RPC socket, but unlike the main
/wsroute it has no release/finalizer path.BrowserHostRegistry.disconnect(...)is the only supplied mechanism that flips the host to disconnected and fails pending commands, so an unexpected WebSocket drop afterregisterleaves the last host looking healthy until commands time out.You’ll need a connection-scoped finalizer that remembers the registered
hostId/runIdand callsdisconnecton close.🤖 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 `@apps/server/src/ws.ts` around lines 92 - 108, The browser-host WebSocket handler in ws.ts registers a host but never clears it when the socket closes, so add a connection-scoped finalizer around the Effect.gen flow that remembers the registered hostId and runId and invokes BrowserHostRegistry.disconnect on teardown. Update the WebSocket upgrade path used with authenticateWebSocketUpgrade and RpcServer.toHttpEffectWebsocket so the cleanup runs on unexpected disconnects as well as normal completion, mirroring the main /ws route behavior.apps/server/src/browser/BrowserPolicy.ts-37-39 (1)
37-39: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't auto-allow
file:navigation.Folding
value.kind === "file"into the allow path lets provider-driven browser flows openfile:///...without any approval step, which turns this policy into silent local-file access from the desktop host. Treatfileasask(or deny it) instead of bundling it withabout/loopback.Suggested change
- const decision = - value.kind === "loopback" || value.kind === "about" || value.kind === "file" - ? "allow" - : "ask"; + const decision = + value.kind === "loopback" || value.kind === "about" + ? "allow" + : "ask";🤖 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 `@apps/server/src/browser/BrowserPolicy.ts` around lines 37 - 39, The navigation policy in BrowserPolicy should not auto-allow file URLs; the current check in the logic that maps value.kind to allow/ask treats value.kind === "file" the same as about and loopback. Update that branch so BrowserPolicy returns ask (or deny) for file and only keeps loopback/about on the allow path, using the existing value.kind handling in BrowserPolicy to locate the change.apps/server/src/browser/BrowserArtifactStore.ts-46-72 (1)
46-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
retentionMsonly changes metadata right now.
putstampsexpiresAt, but nothing ever evicts expired entries andgetstill returns them after expiry. That means browser artifacts can accumulate indefinitely in memory and outlive the retention policy you expose in the returned contract.Suggested direction
+ const pruneExpired = ( + current: ReadonlyMap<string, BrowserArtifactRecord>, + now = Date.now(), + ) => { + const next = new Map(current); + for (const [artifactId, record] of next) { + if (Date.parse(record.expiresAt) <= now) { + next.delete(artifactId); + } + } + return next; + }; put: (input) => Effect.gen(function* () { const now = new Date(); const expiresAt = new Date(now.getTime() + (input.retentionMs ?? 24 * 60 * 60 * 1000)); @@ yield* Ref.update(records, (current) => { - const next = new Map(current); + const next = pruneExpired(current, now.getTime()); next.set(ref.artifactId, record); return next; }); return ref; }), get: (artifactId) => - Ref.get(records).pipe(Effect.map((current) => current.get(artifactId) ?? null)), + Ref.modify(records, (current) => { + const next = pruneExpired(current); + return [next.get(artifactId) ?? null, next] as const; + }),🤖 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 `@apps/server/src/browser/BrowserArtifactStore.ts` around lines 46 - 72, The BrowserArtifactStore put/get flow currently records expiresAt but never enforces it, so expired artifacts remain in the in-memory records map and are still returned. Update the BrowserArtifactStore implementation around put and get to actively respect retentionMs by filtering out expired entries, either by evicting stale records before or during reads and writes or by having get return null once expiresAt has passed. Use the existing artifacts map update path and the BrowserArtifactRef/BrowseArtifactRecord symbols to keep the retention behavior consistent with the stored metadata.apps/server/src/browser/BrowserService.ts-418-423 (1)
418-423: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the profile-lock check atomic.
The
existinglookup and the laterRef.update()are separate operations. Two concurrentopenSession()calls for the same derivedprofileIdcan both observe "no existing session" and create distinct sessions before either update lands, which defeats theprofile_lockedguard.Also applies to: 452-458
🤖 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 `@apps/server/src/browser/BrowserService.ts` around lines 418 - 423, Make the profile-lock check atomic in BrowserService.openSession: the current lookup for an existing non-closed session and the later Ref.update() happen separately, so concurrent calls can both pass the check and create duplicate sessions. Refactor the session creation path so the “find existing session by profileId” check and the state mutation occur as one atomic Ref.modify/Ref.update operation, and apply the same fix to the related block in the later openSession path as well.apps/server/src/browser/BrowserService.ts-452-472 (1)
452-472: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't persist the session before
open_sessionsucceeds.This writes the new profile/session into
statebeforeexecuteSessionCommand()runs. If the host is unavailable, times out, disconnects, or returns an error, the call fails but the in-memory session remains visible togetStatus()and can block futureopenSession()calls for the same profile.Suggested fix
- yield* Ref.update(state, (latest) => { - const profiles = new Map(latest.profiles); - profiles.set(profile.profileId, profile); - const sessions = new Map(latest.sessions); - sessions.set(session.sessionId, session); - return { profiles, sessions }; - }); - - return yield* executeSessionCommand({ + const opened = yield* executeSessionCommand({ command: { kind: "open_session", session, profile, ...(initialNavigation ? { initialUrl: initialNavigation.url } : {}), }, fallbackSession: { ...session, status: "ready", updatedAt: nowIso(), }, }); + yield* Ref.update(state, (latest) => { + const profiles = new Map(latest.profiles); + profiles.set(profile.profileId, profile); + return { ...latest, profiles }; + }); + return opened;🤖 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 `@apps/server/src/browser/BrowserService.ts` around lines 452 - 472, The BrowserService open-session flow is persisting the new profile/session into state before `executeSessionCommand()` succeeds, which leaves failed sessions visible and can block later `openSession()` calls. Update the `openSession` logic in `BrowserService` so the `Ref.update` that adds `profile` and `session` happens only after `executeSessionCommand` returns successfully, or roll back any optimistic state on failure. Use the `openSession`, `executeSessionCommand`, and `Ref.update` blocks to locate the change and keep the in-memory state consistent with the host result.apps/server/src/browser/BrowserHostRegistry.test.ts-92-93 (1)
92-93: 🎯 Functional Correctness | 🟠 MajorUse Effect's public failure helpers here.
The current
findFailurehelper accessesexit.cause.reasonsdirectly. While this exposes internal structure,Exit.causeitself is a public API property defined ineffect/Cause. The private/public distinction here lies in the internal array structure ofreasons, but the pattern is widely used in the ecosystem.However,
Cause.failureOption(exit.cause)correctly usesCause.failureOptionwhich is a stable public utility that safely extracts the error type from a Cause. The suggested refactor usingExit.isFailureandOption.getOrUndefinedis the more idiomatic, type-safe approach that decouples the test from the internal representation of multiple reasons or parallel failures, assuming the test intent is to find the failure value.Since the current test asserts
expect(failure?.error), andfindFailurecurrently returnsreason(wherereasonmatches{ _tag: "Fail", error: ... }), the current code actually works but relies on the specific shape. The refactored code changes the return value to justerror. To ensure type safety and stability, useCause.failureOption.- exit._tag === "Failure" ? exit.cause.reasons.find((reason) => reason._tag === "Fail") : undefined; + Exit.isFailure(exit) ? Option.getOrUndefined(Cause.failureOption(exit.cause)) : undefined;This should be used at lines 92-93.
🤖 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 `@apps/server/src/browser/BrowserHostRegistry.test.ts` around lines 92 - 93, The findFailure helper is reaching into Exit.cause.reasons directly, which ties the test to Cause’s internal structure; refactor it to use the public failure helpers instead. Update findFailure in BrowserHostRegistry.test.ts to rely on Exit.isFailure and Cause.failureOption (with Option.getOrUndefined if needed) so it returns the failure value in a type-safe way without inspecting reasons.apps/server/src/auth/Layers/BrowserHostAuth.ts-20-26 (1)
20-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the peer address, not just the
Hostheader.Line 45 only checks the client-controlled
Hostheader, andrequestHost ?? undefinedis accepted as loopback becauseisLoopbackHost(undefined)returns true. Require a parsed Host and the request source IP to be loopback before accepting the browser-host token.Suggested fix
import { ServerConfig } from "../../config.ts"; import { isLoopbackHost } from "../../startupAccess.ts"; +import { deriveAuthClientMetadata } from "../utils.ts"; import { BrowserHostAuth, type BrowserHostAuthShape } from "../Services/BrowserHostAuth.ts"; import { AuthError } from "../Services/ServerAuth.ts"; @@ try { return new URL(`http://${value}`).hostname.toLowerCase(); } catch { - return value.split(":")[0]?.toLowerCase() ?? null; + const fallbackHost = value.split(":")[0]?.toLowerCase().trim(); + return fallbackHost && fallbackHost.length > 0 ? fallbackHost : null; } } @@ - const requestHost = hostnameFromHostHeader(request.headers.host); - if (!isLoopbackHost(requestHost ?? undefined)) { + const requestHost = hostnameFromHostHeader(request.headers.host); + const clientIpAddress = deriveAuthClientMetadata({ request }).ipAddress; + if ( + requestHost === null || + !isLoopbackHost(requestHost) || + clientIpAddress === undefined || + !isLoopbackHost(clientIpAddress) + ) { return yield* new AuthError({ message: "BrowserHost route requires loopback desktop access.", status: 403,Also applies to: 45-46
🤖 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 `@apps/server/src/auth/Layers/BrowserHostAuth.ts` around lines 20 - 26, The browser-host auth check in BrowserHostAuth should not trust only the client-controlled Host header; update the validation around hostnameFromHostHeader and the request-host check so a parsed Host is required and the peer/source IP must also be loopback before accepting the browser-host token. Make the logic in the auth flow that currently treats requestHost ?? undefined as loopback use the actual request origin address (or equivalent peer address) alongside the parsed host, and only allow success when both are loopback-safe.
🟡 Minor comments (3)
apps/desktop/src/browser/BrowserStorageHelpers.ts-86-90 (1)
86-90: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClamp non-finite storage sizes before returning contract metadata.
typeof candidate.valueBytes === "number"still acceptsNaNandInfinity, so renderer-controlled storage inspection data can produce invalidBrowserStorageEntryMetadata.Proposed fix
- const size = typeof candidate.valueBytes === "number" ? candidate.valueBytes : 0; + const size = + typeof candidate.valueBytes === "number" && Number.isFinite(candidate.valueBytes) + ? candidate.valueBytes + : 0;🤖 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 `@apps/desktop/src/browser/BrowserStorageHelpers.ts` around lines 86 - 90, Clamp non-finite storage sizes before returning BrowserStorageEntryMetadata in BrowserStorageHelpers by tightening the valueBytes normalization in the metadata builder, since typeof candidate.valueBytes alone still allows NaN and Infinity. Update the logic around the candidate.valueBytes handling in the helper that returns BrowserStorageEntryMetadata so it only passes through finite numeric values, otherwise falls back to 0, while keeping the key slicing and existing floor/max behavior intact.packages/shared/src/browser.ts-114-139 (1)
114-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBracketed IPv6 locals are misclassified as public hosts.
[fd00::1]and[fe80::1]currently miss all three hostname classifiers, so bare local IPv6 addresses normalize tohttps://…and report the wrongkind. Users can work around it by typinghttp://explicitly, but the helper should strip[/]once and handle local IPv6 ranges directly.Also applies to: 159-169
🤖 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 `@packages/shared/src/browser.ts` around lines 114 - 139, The hostname classifiers in browser.ts are treating bracketed IPv6 literals like [fd00::1] and [fe80::1] as public because isPrivateNetworkHostname and isLinkLocalHostname only handle IPv4-style parsing and raw fe80: prefixes. Update the shared hostname normalization used by isPrivateNetworkHostname, isLinkLocalHostname, and any related classifier logic so it strips surrounding [ ] once, then recognizes local IPv6 ranges (at least fd00::/8 for private and fe80::/10 for link-local) before falling back to the existing IPv4 checks and public-host behavior.docs/superpowers/specs/2026-06-22-in-app-browser-design.md-145-146 (1)
145-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
BrowserHostRunIdin the core ID list.The design later relies on
BrowserHostRunIdfor reconnect fencing and stale-command rejection at Lines 237, 376, and 657. Leaving it out of the schema checklist makes the contracts section internally inconsistent.🤖 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 `@docs/superpowers/specs/2026-06-22-in-app-browser-design.md` around lines 145 - 146, The core ID list is missing BrowserHostRunId, which makes the schema checklist inconsistent with the later reconnect and stale-command flow. Update the IDs section in the design doc to include BrowserHostRunId alongside the existing BrowserHostId, BrowserProfileId, BrowserSessionId, BrowserTabId, BrowserCommandId, and BrowserPermissionRequestId entries so the contracts section matches the BrowserHostRunId usage elsewhere.
🧹 Nitpick comments (2)
apps/desktop/src/preload.ts (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the browser IPC channel strings.
These same literals are also declared in
apps/desktop/src/main.ts:139-142. A future edit can desynchronize preload and main, and surface attach/focus calls will start failing at runtime with no type-level protection. Move them into a shared desktop IPC module and import from both sides.Suggested refactor
-const BROWSER_ATTACH_SURFACE_CHANNEL = "desktop:browser:attach-surface"; -const BROWSER_UPDATE_SURFACE_BOUNDS_CHANNEL = "desktop:browser:update-surface-bounds"; -const BROWSER_DETACH_SURFACE_CHANNEL = "desktop:browser:detach-surface"; -const BROWSER_FOCUS_SURFACE_CHANNEL = "desktop:browser:focus-surface"; +import { + BROWSER_ATTACH_SURFACE_CHANNEL, + BROWSER_UPDATE_SURFACE_BOUNDS_CHANNEL, + BROWSER_DETACH_SURFACE_CHANNEL, + BROWSER_FOCUS_SURFACE_CHANNEL, +} from "./browser/browserIpcChannels.ts";🤖 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 `@apps/desktop/src/preload.ts` around lines 44 - 47, The browser IPC channel literals are duplicated between the preload and main sides, so move the shared channel constants out of the preload-specific code into a common desktop IPC module and import them from both places. Update the references in the preload-side symbols that use BROWSER_ATTACH_SURFACE_CHANNEL, BROWSER_UPDATE_SURFACE_BOUNDS_CHANNEL, BROWSER_DETACH_SURFACE_CHANNEL, and BROWSER_FOCUS_SURFACE_CHANNEL, and make the main-side handlers consume the same shared constants to keep the channel names synchronized.apps/server/src/test/serverConfigFixtures.ts (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the shared test fixture aligned with runtime defaults.
Lines 25-27 make
makeTestServerConfigenable browser-host auth by default, whileapps/server/src/config.tsLine 170 andapps/server/src/cli.tsLines 347 and 427 keepdesktopBrowserHostTokenunset unless bootstrap data provides it. That makes this generic helper silently exercise a privileged path that production does not enable by default.♻️ Proposed change
- desktopBrowserHostToken: "secret-token", + desktopBrowserHostToken: undefined,// BrowserHostAuth.test.ts makeTestServerConfig({ desktopBrowserHostToken: "secret-token" });🤖 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 `@apps/server/src/test/serverConfigFixtures.ts` around lines 25 - 27, The shared test fixture is enabling browser-host auth by default instead of matching the runtime default. Update makeTestServerConfig in serverConfigFixtures to leave desktopBrowserHostToken unset unless a test explicitly overrides it, and move any browser-host auth setup into the specific tests that need it (for example by passing a token from BrowserHostAuth.test). This keeps the fixture aligned with config.ts and cli.ts defaults.
🤖 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.
Major comments:
In `@apps/desktop/src/browser/BrowserHostConnection.ts`:
- Around line 68-101: The BrowserHostConnection.start flow leaves partial state
behind if makeBrowserHostRpcClient or the BROWSER_HOST_METHODS.register call
fails, because runtime and clientScope are assigned before the full startup
completes. Update start to wrap the client creation/register sequence in a
try/catch, and on any failure call await this.stop() to dispose the runtime,
scope, and socket before rethrowing. Keep the fix centered in
BrowserHostConnection.start and use the existing stop(), runtime, clientScope,
and startCommandStream setup as the cleanup boundary.
- Around line 151-175: The per-envelope handler in BrowserHostConnection’s
subscribeCommands stream is letting errors from handleCommand(envelope) or
client[BROWSER_HOST_METHODS.commandResult](...) escape Stream.runForEach, which
stops the subscription after one failure. Wrap the logic inside the
Effect.promise handler with its own try/catch (or equivalent failure handling)
so each envelope logs or reports the failure and then returns without rejecting
the outer stream, keeping the command subscription alive for later messages.
In `@apps/desktop/src/browser/BrowserKernel.ts`:
- Around line 31-34: The hosted-session lifecycle is losing the BrowserProfile
reference, so temporary profiles never get cleaned up and remain cached in
sessionsByProfile. Update HostedSession and the BrowserKernel session management
flow to retain the BrowserProfile alongside the
BrowserSessionSnapshot/WebContentsView, then call
BrowserProfiles.cleanupTemporary() when a non-persistent hosted session is
closed or removed. Make sure the cleanup path uses the stored profile identity
so temporary storage is released consistently.
- Around line 226-227: The initial navigation in BrowserKernel’s session startup
is swallowing loadURL failures, which allows open_session to be reported as
successful even when the page never loaded. Update the logic around
BrowserKernel’s loadURL/updateSnapshotFromWebContents flow to stop hiding the
error, and ensure the caller only returns success after the initial navigation
completes successfully; if loadURL fails, propagate or convert that failure so
open_session is not marked ok.
- Around line 563-585: The WebContents listeners in installWebContentsListeners
are ignoring failures from the async event sink because void this.emit(...)
discards rejected promises. Update the update callback and the
render-process-gone handler to catch and handle emit failures explicitly, using
this.emit and a .catch(...) path (or equivalent) so transient publishing errors
don’t become unhandled rejections in BrowserKernel.
In `@apps/server/src/auth/Layers/BrowserHostAuth.ts`:
- Around line 20-26: The browser-host auth check in BrowserHostAuth should not
trust only the client-controlled Host header; update the validation around
hostnameFromHostHeader and the request-host check so a parsed Host is required
and the peer/source IP must also be loopback before accepting the browser-host
token. Make the logic in the auth flow that currently treats requestHost ??
undefined as loopback use the actual request origin address (or equivalent peer
address) alongside the parsed host, and only allow success when both are
loopback-safe.
In `@apps/server/src/browser/BrowserArtifactStore.ts`:
- Around line 46-72: The BrowserArtifactStore put/get flow currently records
expiresAt but never enforces it, so expired artifacts remain in the in-memory
records map and are still returned. Update the BrowserArtifactStore
implementation around put and get to actively respect retentionMs by filtering
out expired entries, either by evicting stale records before or during reads and
writes or by having get return null once expiresAt has passed. Use the existing
artifacts map update path and the BrowserArtifactRef/BrowseArtifactRecord
symbols to keep the retention behavior consistent with the stored metadata.
In `@apps/server/src/browser/BrowserHostRegistry.test.ts`:
- Around line 92-93: The findFailure helper is reaching into Exit.cause.reasons
directly, which ties the test to Cause’s internal structure; refactor it to use
the public failure helpers instead. Update findFailure in
BrowserHostRegistry.test.ts to rely on Exit.isFailure and Cause.failureOption
(with Option.getOrUndefined if needed) so it returns the failure value in a
type-safe way without inspecting reasons.
In `@apps/server/src/browser/BrowserHostRegistry.ts`:
- Around line 130-136: The host re-registration and heartbeat flow in
BrowserHostRegistry is merging session state instead of replacing it, which
leaves stale sessions behind when the host reconnects with a smaller snapshot.
Update register() and heartbeat() so the registry replaces the stored sessions
with the host’s current snapshot rather than preserving previous.sessions or
only upserting input.sessions; keep failPending(previous.pending,
staleHostError()) and state reset behavior, but ensure snapshot() reflects only
the sessions currently reported by the active host.
- Around line 97-98: The shared commandQueue is being consumed by multiple
commandStream() subscribers, so a stale BrowserHostRegistry stream can dequeue
and drop commands meant for the active run. Update BrowserHostRegistry so
commands are routed to the correct listener before consumption, using per-run
queues or a PubSub-based fanout instead of Stream.fromQueue(commandQueue) with a
post-dequeue runId filter. Make sure the commandStream() path only receives
commands for its own runId and cannot steal items from another session.
In `@apps/server/src/browser/BrowserPolicy.ts`:
- Around line 37-39: The navigation policy in BrowserPolicy should not
auto-allow file URLs; the current check in the logic that maps value.kind to
allow/ask treats value.kind === "file" the same as about and loopback. Update
that branch so BrowserPolicy returns ask (or deny) for file and only keeps
loopback/about on the allow path, using the existing value.kind handling in
BrowserPolicy to locate the change.
In `@apps/server/src/browser/BrowserService.ts`:
- Around line 418-423: Make the profile-lock check atomic in
BrowserService.openSession: the current lookup for an existing non-closed
session and the later Ref.update() happen separately, so concurrent calls can
both pass the check and create duplicate sessions. Refactor the session creation
path so the “find existing session by profileId” check and the state mutation
occur as one atomic Ref.modify/Ref.update operation, and apply the same fix to
the related block in the later openSession path as well.
- Around line 452-472: The BrowserService open-session flow is persisting the
new profile/session into state before `executeSessionCommand()` succeeds, which
leaves failed sessions visible and can block later `openSession()` calls. Update
the `openSession` logic in `BrowserService` so the `Ref.update` that adds
`profile` and `session` happens only after `executeSessionCommand` returns
successfully, or roll back any optimistic state on failure. Use the
`openSession`, `executeSessionCommand`, and `Ref.update` blocks to locate the
change and keep the in-memory state consistent with the host result.
In `@apps/server/src/provider/tools/BrowserRuntimeTool.ts`:
- Around line 56-66: The browser_open tool schema is out of sync with the
executor input shape: BROWSER_RUNTIME_TOOL_DEFINITIONS advertises initialUrl
while BrowserRuntimeToolCallInput and the browser_open execution path read url.
Update the tool definition and any related input typing/default handling so the
same argument name is used end-to-end, and make sure the browser_open executor
continues to navigate using that shared field.
In `@apps/server/src/ws.ts`:
- Around line 92-108: The browser-host WebSocket handler in ws.ts registers a
host but never clears it when the socket closes, so add a connection-scoped
finalizer around the Effect.gen flow that remembers the registered hostId and
runId and invokes BrowserHostRegistry.disconnect on teardown. Update the
WebSocket upgrade path used with authenticateWebSocketUpgrade and
RpcServer.toHttpEffectWebsocket so the cleanup runs on unexpected disconnects as
well as normal completion, mirroring the main /ws route behavior.
In `@apps/web/src/components/BrowserPanel.tsx`:
- Around line 421-423: Transient command failures are incorrectly being stored
in BrowserPanelStatus and that can tear down the native surface; keep command
errors separate from the lifecycle state. Update BrowserPanel in the
status/message/session state flow so commands like back, reload, and
open-external no longer set status to "error" unless the panel is truly
unavailable, and instead store those failures in a separate error field. Make
sure the lifecycle-driven status transitions in the BrowserPanel controls
preserve "ready" once the session is healthy, using the existing
BrowserPanelStatus, setStatus, setMessage, and BrowserSessionSnapshot handling.
- Around line 455-521: The BrowserPanel effect only unsubscribes from events and
never closes the active BrowserHost session, so sessions can leak when
environmentId/projectId/threadId changes or the component unmounts. Update the
cleanup in the BrowserPanel useEffect/openSession flow to call the browser API’s
session-close/teardown method for the current session (alongside unsubscribe),
and make sure it runs safely even when the async openSession is still in flight.
Use the existing openSession, currentBrowserApi, and cleanup return function as
the place to wire this in.
In `@packages/contracts/src/browser.ts`:
- Around line 475-484: The BrowserCommandResultPayload schema currently allows
data to be Schema.Unknown, which can pass validation but break when
BrowserCommandResult is serialized over the browser-host JSON websocket. Update
the BrowserCommandResultPayload definition to use a JSON-safe schema for data
instead of Schema.Unknown, and keep the change localized to the
BrowserCommandResultPayload/BrowserCommandResult path so the RPC boundary only
accepts encodable values.
- Around line 209-215: Reuse the bounded URL schema for every initial-navigation
entry point so callers cannot bypass the URL length guard through session
startup. Update BrowserOpenSessionInput.initialUrl and the open_session host
command schema to use the same capped URL schema that browser.navigate uses, and
make sure any session-opening flow passes through that shared validator. Locate
the affected definitions around BrowserOpenSessionInput and the open_session
command contract, plus any related initial-navigation schema uses.
In `@packages/contracts/src/browserHostRpc.ts`:
- Around line 57-70: `BrowserHostEventsRpc` is defined but not included in
`BrowserHostRpcGroup`, so the `browserHost.events` route is never exposed.
Update the `BrowserHostRpcGroup.make(...)` registration in `browserHostRpc.ts`
to add `BrowserHostEventsRpc` alongside the existing browser host RPCs, so the
`/browser-host/ws` server can route this event stream correctly.
In `@packages/shared/src/browser.ts`:
- Around line 46-53: The URL normalization logic in
normalizeBrowserNavigationUrl is over-prompting bare defaults and accidentally
rewriting explicit non-browser schemes like mailto: and mixed-case schemes like
ABOUT:blank. Update the explicit scheme detection and the candidate selection in
browser.ts so any genuine scheme is preserved as-is, and only bare host or
host:port inputs get an auto-prefixed default scheme; ensure the explicitScheme
check handles case-insensitive matches before deciding whether to synthesize
https://.
---
Minor comments:
In `@apps/desktop/src/browser/BrowserStorageHelpers.ts`:
- Around line 86-90: Clamp non-finite storage sizes before returning
BrowserStorageEntryMetadata in BrowserStorageHelpers by tightening the
valueBytes normalization in the metadata builder, since typeof
candidate.valueBytes alone still allows NaN and Infinity. Update the logic
around the candidate.valueBytes handling in the helper that returns
BrowserStorageEntryMetadata so it only passes through finite numeric values,
otherwise falls back to 0, while keeping the key slicing and existing floor/max
behavior intact.
In `@docs/superpowers/specs/2026-06-22-in-app-browser-design.md`:
- Around line 145-146: The core ID list is missing BrowserHostRunId, which makes
the schema checklist inconsistent with the later reconnect and stale-command
flow. Update the IDs section in the design doc to include BrowserHostRunId
alongside the existing BrowserHostId, BrowserProfileId, BrowserSessionId,
BrowserTabId, BrowserCommandId, and BrowserPermissionRequestId entries so the
contracts section matches the BrowserHostRunId usage elsewhere.
In `@packages/shared/src/browser.ts`:
- Around line 114-139: The hostname classifiers in browser.ts are treating
bracketed IPv6 literals like [fd00::1] and [fe80::1] as public because
isPrivateNetworkHostname and isLinkLocalHostname only handle IPv4-style parsing
and raw fe80: prefixes. Update the shared hostname normalization used by
isPrivateNetworkHostname, isLinkLocalHostname, and any related classifier logic
so it strips surrounding [ ] once, then recognizes local IPv6 ranges (at least
fd00::/8 for private and fe80::/10 for link-local) before falling back to the
existing IPv4 checks and public-host behavior.
---
Nitpick comments:
In `@apps/desktop/src/preload.ts`:
- Around line 44-47: The browser IPC channel literals are duplicated between the
preload and main sides, so move the shared channel constants out of the
preload-specific code into a common desktop IPC module and import them from both
places. Update the references in the preload-side symbols that use
BROWSER_ATTACH_SURFACE_CHANNEL, BROWSER_UPDATE_SURFACE_BOUNDS_CHANNEL,
BROWSER_DETACH_SURFACE_CHANNEL, and BROWSER_FOCUS_SURFACE_CHANNEL, and make the
main-side handlers consume the same shared constants to keep the channel names
synchronized.
In `@apps/server/src/test/serverConfigFixtures.ts`:
- Around line 25-27: The shared test fixture is enabling browser-host auth by
default instead of matching the runtime default. Update makeTestServerConfig in
serverConfigFixtures to leave desktopBrowserHostToken unset unless a test
explicitly overrides it, and move any browser-host auth setup into the specific
tests that need it (for example by passing a token from BrowserHostAuth.test).
This keeps the fixture aligned with config.ts and cli.ts defaults.
- Desktop host: screenshots, console/network capture, DOM snapshot with ref-based clicking - Server: BrowserService, artifact storage, provider MCP bridge integration - Tools: browser_open, navigate, input, snapshot, screenshot, console, network, waitFor - Providers: Claude (MCP), Copilot, Cursor, OpenCode; Codex schema pending
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/browser/BrowserArtifactStore.ts (1)
59-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd artifact expiry cleanup
expiresAtis stored but never used here, so expired artifacts stay inrecordsand on disk for the server lifetime. Add a periodic reaper or remove expired entries/files on access.🤖 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 `@apps/server/src/browser/BrowserArtifactStore.ts` around lines 59 - 124, The BrowserArtifactStoreLive implementation computes and stores expiresAt in BrowserArtifactRecord, but nothing ever removes expired artifacts from records or disk. Add expiry handling in the BrowserArtifactStoreShape methods by either introducing a periodic cleanup/reaper in BrowserArtifactStoreLive or checking expiry on get/readData/put and deleting stale entries/files before returning. Make sure the cleanup updates the records Ref and removes the corresponding filePath for each expired BrowserArtifactRecord.apps/server/src/browser/BrowserService.ts (1)
537-599: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftClaim the profile atomically.
openSessionchecks for an existing session and later writes the new one as separate steps, so two concurrent calls for the same profile can both pass the lock check and create active sessions. Move the reservation/claim into a single atomic update path before proceeding.🤖 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 `@apps/server/src/browser/BrowserService.ts` around lines 537 - 599, openSession currently performs the “is this profile already attached?” check and the session insert as separate steps, which allows two concurrent callers to race and both create active sessions. Update BrowserService.openSession so the profile claim happens atomically inside the state update path: check the current sessions and reserve the profile/session in one Ref.update/modify-style operation before continuing, and only proceed with makeSession/executeSessionCommand after the reservation succeeds. Use the existing openSession, profileFor, state, and sessionOwners/session maps to locate the logic.
🧹 Nitpick comments (10)
apps/server/src/provider/tools/BrowserRuntimeTool.ts (2)
243-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Schema.is(BrowserLoadState)instead of hardcoded literals.This duplicates the literal set already defined by the
BrowserLoadStatecontract schema. If the contract's allowed values change, this helper silently falls out of sync (returningundefinedfor values that should be valid).browser_input's action validation already usesSchema.is(BrowserInputAction)— apply the same pattern here.♻️ Proposed fix
-function readBrowserToolLoadState(value: unknown): BrowserLoadState | undefined { - if (value === "idle" || value === "loading" || value === "loaded" || value === "failed") { - return value; - } - return undefined; -} +function readBrowserToolLoadState(value: unknown): BrowserLoadState | undefined { + return Schema.is(BrowserLoadState)(value) ? value : undefined; +}🤖 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 `@apps/server/src/provider/tools/BrowserRuntimeTool.ts` around lines 243 - 248, The readBrowserToolLoadState helper is duplicating the BrowserLoadState allowed values with hardcoded literals, which can drift from the contract schema. Update readBrowserToolLoadState in BrowserRuntimeTool to validate using Schema.is(BrowserLoadState), matching the pattern already used for browser_input with Schema.is(BrowserInputAction), so the helper always stays aligned with the schema definition.
368-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated
missing_sessionchecks aroundrequireSession.
requireSessionis only used for snapshot/screenshot/console/network/wait_for;browser_navigate, the back/forward/reload/stop group, andbrowser_inputeach re-implement the sameif (!input.sessionId) { fail(missing_session) }block inline. Routing all cases throughrequireSessionwould remove ~5 duplicated blocks and keep the error message/code consistent if it ever needs to change.♻️ Example for the navigate case
case "browser_navigate": - if (!input.sessionId) { - return Effect.fail( - new BrowserRuntimeToolError({ - code: "missing_session", - message: "browser_navigate requires a browser session id.", - }), - ); - } if (!input.url) { return Effect.fail( new BrowserRuntimeToolError({ code: "missing_url", message: "browser_navigate requires a URL.", }), ); } - return browser.navigate({ - sessionId: input.sessionId, - ...(input.tabId ? { tabId: input.tabId } : {}), - url: input.url, - ...(input.source ? { source: input.source } : {}), - }); + return Effect.gen(function* () { + const sessionId = yield* requireSession("browser_navigate"); + return yield* browser.navigate({ + sessionId, + ...(input.tabId ? { tabId: input.tabId } : {}), + url: input.url as string, + ...(input.source ? { source: input.source } : {}), + }); + });🤖 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 `@apps/server/src/provider/tools/BrowserRuntimeTool.ts` around lines 368 - 451, The `missing_session` validation is duplicated in `browser_navigate`, the `browser_back`/`browser_forward`/`browser_reload`/`browser_stop` branch, and `browser_input` instead of reusing `requireSession`. Update `BrowserRuntimeTool` so those cases call `requireSession(input.name)` (or an equivalent shared helper flow) before invoking `browser.navigate`, `browser[command]`, or `browser.input`, and keep the existing `BrowserRuntimeToolError` behavior centralized there for consistent session-id handling.apps/server/src/browser/BrowserMcpBridge.ts (1)
95-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
BrowserMcpBridgeLivehas no finalizer to close active servers.
bridgesRefaccumulates listeningnet.Serverinstances and socket files, but the layer never releases them on teardown. Cleanup only happens via per-threadstop. If the layer is torn down while sessions are still registered (or astartsucceeds but the caller never reachesstop), the server sockets and.sockfiles leak. Consider adding anEffect.addFinalizerthat stops all remaining bridges.🤖 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 `@apps/server/src/browser/BrowserMcpBridge.ts` around lines 95 - 98, BrowserMcpBridgeLive currently creates and stores ActiveBridge instances in bridgesRef but never cleans them up when the layer shuts down. Add an Effect.addFinalizer inside the BrowserMcpBridgeLive Effect.gen setup that iterates over the remaining bridges in bridgesRef and stops each one, ensuring any net.Server listeners and .sock files are released even if stop was never called per thread. Use the existing BrowserMcpBridgeLive, bridgesRef, and ActiveBridge stop/cleanup logic to wire this teardown into the layer lifecycle.apps/server/src/provider/Layers/CopilotAdapter.ts (1)
258-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
runPromisedrops the adapter runtime context here.Unlike the Cursor and Claude adapters — which capture
Effect.context<never>()and useEffect.runPromiseWith(runtimeContext)— this executes browser tool effects on Effect's default runtime. That loses the fiber context (logger/tracing span correlation, fiber refs, and any context-provided services) for browser tool calls. Consider capturing the runtime context once inmakeCopilotAdapterand threadingEffect.runPromiseWith(runtimeContext)throughbuildSessionConfigfor consistency.🤖 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 `@apps/server/src/provider/Layers/CopilotAdapter.ts` at line 258, The browser tool executor in CopilotAdapter is running effects with the default runtime, which drops the adapter’s fiber/runtime context. Update makeCopilotAdapter to capture the Effect context once, then thread that runtimeContext into buildSessionConfig so the runPromise implementation uses Effect.runPromiseWith(runtimeContext) instead of Effect.runPromise; keep the behavior aligned with the Cursor and Claude adapter implementations.apps/server/src/provider/tools/BrowserToolLifecycleEvents.ts (1)
22-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate event-building logic between started/completed builders.
makeBrowserToolStartedEventandmakeBrowserToolCompletedEventshare nearly identical scaffolding (eventId generation, optionalproviderInstanceId/turnIdspreads,threadId,createdAt,raw.source). Consider extracting a shared base-event builder to reduce duplication and the risk of the two diverging when fields are added later.♻️ Proposed extraction
+const makeEventBase = (input: { + readonly call: BrowserRuntimeToolCallInput; + readonly context: BrowserToolLifecycleContext; + readonly itemId: RuntimeItemId; + readonly createdAt?: string; +}) => + Effect.gen(function* () { + const eventId = EventId.make(yield* Random.nextUUIDv4); + const createdAt = input.createdAt ?? new Date().toISOString(); + return { + eventId, + provider: input.context.provider, + ...(input.context.providerInstanceId + ? { providerInstanceId: input.context.providerInstanceId } + : {}), + threadId: input.call.threadId, + createdAt, + ...(input.context.turnId ? { turnId: input.context.turnId } : {}), + itemId: input.itemId, + }; + });🤖 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 `@apps/server/src/provider/tools/BrowserToolLifecycleEvents.ts` around lines 22 - 101, Both BrowserToolLifecycleEvents builders duplicate the same event scaffolding, so extract the shared base event creation from makeBrowserToolStartedEvent and makeBrowserToolCompletedEvent into a helper. Move the common pieces like EventId.make, provider/providerInstanceId/turnId spreads, threadId, createdAt, and raw.source assembly into a shared function, then let each builder add only its specific type and payload fields.apps/server/src/provider/tools/BrowserRuntimeToolHelpers.ts (1)
39-55: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLoose validation in
readScreenshotArtifact.Only
artifactIdandmimeTypeare checked before casting toBrowserArtifactRef; ifenrichBrowserRuntimeToolCallToolResultor downstream consumers ever rely on other required fields of that contract (e.g.byteSize,url), the shallow guard won't catch a malformed structured payload. Given only these two fields are actually consumed here, this is acceptable as-is but worth a comment noting the guard is intentionally partial.🤖 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 `@apps/server/src/provider/tools/BrowserRuntimeToolHelpers.ts` around lines 39 - 55, The validation in readScreenshotArtifact is intentionally shallow, but it currently looks like a full contract check even though only artifactId and mimeType are used. Update the guard in readScreenshotArtifact to clearly document that it is a partial validation for BrowserArtifactRef, and note that enrichBrowserRuntimeToolCallToolResult and downstream consumers only depend on those fields. Keep the existing checks, but add an explicit comment near the artifact cast explaining the intentional scope of validation.apps/server/src/browser/BrowserObservationHelpers.ts (1)
12-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
Schema.decodeUnknownOptionover hand-rolled shape checks.All four decoders duplicate validation logic that already exists as exported
effectSchemas in@ryco/contracts(BrowserHostScreenshotData,BrowserHostDomSnapshotData,BrowserHostConsoleData,BrowserHostNetworkData). The manual checks are also shallower than schema validation — e.g.decodeBrowserHostDomSnapshotDatanever validates nestedtreenode shapes, anddecodeBrowserHostConsoleData/decodeBrowserHostNetworkDataonly checkArray.isArraybefore unsafely castingentries, so malformed entries pass through untyped.♻️ Suggested approach
-import type { +import { BrowserHostConsoleData, BrowserHostDomSnapshotData, BrowserHostNetworkData, BrowserHostScreenshotData, } from "`@ryco/contracts`"; +import { Schema } from "effect"; export function decodeBrowserHostScreenshotData(value: unknown): BrowserHostScreenshotData | null { - if (!isRecord(value) || value.kind !== "screenshot" || typeof value.base64 !== "string") { - return null; - } - return { kind: "screenshot", base64: value.base64 }; + return Schema.decodeUnknownOption(BrowserHostScreenshotData)(value).pipe( + Option.getOrNull, + ); }Apply the same pattern to the other three decoders.
🤖 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 `@apps/server/src/browser/BrowserObservationHelpers.ts` around lines 12 - 55, Replace the hand-rolled validation in decodeBrowserHostScreenshotData, decodeBrowserHostDomSnapshotData, decodeBrowserHostConsoleData, and decodeBrowserHostNetworkData with the exported effect schemas from `@ryco/contracts` using Schema.decodeUnknownOption. The current checks only validate shallow fields and unsafe-cast nested data, so wire each decoder to its matching schema to get full nested validation (including dom_snapshot tree and entries shapes) and return null on decode failure.apps/server/src/provider/Layers/CopilotAdapter.mapEvent.ts (1)
216-219: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefix-based browser tool detection can misclassify identically-prefixed MCP tools.
toolName.startsWith("browser_")is checked beforeisMcpTool, so any MCP-hosted tool whose name happens to start withbrowser_(e.g., a user's custom MCP server tool) would be reported asbrowser_tool_callinstead ofmcp_tool_call. Consider using the canonicalisBrowserRuntimeToolNamecheck (already defined inapps/server/src/provider/tools/BrowserRuntimeTool.ts) instead of a raw string-prefix heuristic, to avoid ambiguity and duplicated matching logic across adapters.Also applies to: 245-250
🤖 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 `@apps/server/src/provider/Layers/CopilotAdapter.mapEvent.ts` around lines 216 - 219, The tool classification in CopilotAdapter.mapEvent is using a raw browser_ prefix check that can mislabel MCP tools as browser tools. Update the tool execution mapping logic to use the canonical isBrowserRuntimeToolName helper from BrowserRuntimeTool instead of startsWith("browser_"), and keep the existing isMcpTool branch so MCP-hosted tools like tool.execution_start and the related mapping block are classified consistently.apps/server/src/browser/BrowserService.ts (1)
949-988: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
waitFortext-polling triggers a full DOM snapshot + session save + event publish every 250ms.When
text/textGoneis set, each poll iteration callsdispatchObservationCommand({ kind: "snapshot_dom" }), which (viaexecuteHostDataCommand) also callssaveSessionand publishes asession.updatedevent on every tick — even when nothing changed. With the default 30s timeout that's up to ~120 host round trips and ~120 broadcast events for a singlewaitForcall, which could add meaningful load on the desktop host and spam subscribers with redundant updates.Consider only publishing/saving when the session/tab snapshot actually differs from the last observed one, or backing off the poll interval over time.
🤖 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 `@apps/server/src/browser/BrowserService.ts` around lines 949 - 988, The waitFor polling in BrowserService is repeatedly triggering snapshot_dom through dispatchObservationCommand, which causes saveSession and session.updated broadcasts on every 250ms tick even when nothing changes. Update the waitFor flow to avoid redundant host work by caching the last observed session/tab snapshot (or visible text) and only calling the snapshot/save/publish path when the observed state actually differs, or otherwise reduce the polling frequency over time. Focus the change around waitFor, dispatchObservationCommand, and executeHostDataCommand so text/textGone waits stop spamming unchanged updates.apps/server/src/browser/BrowserService.test.ts (1)
419-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the oversized DOM snapshot → artifact path.
Only the untruncated
snapshotDompath is tested here. ThesnapshotJson.length > 65_536branch inBrowserService.ts(stores snapshot as an artifact, emptiestree, setstruncated: true) isn't covered by any test in this file.🤖 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 `@apps/server/src/browser/BrowserService.test.ts` around lines 419 - 460, The BrowserService.snapshotDom test only covers the normal in-memory response path; add a case for the oversized snapshot branch where snapshotJson.length exceeds 65_536 so BrowserService stores the snapshot as an artifact, returns an empty tree, and marks truncated as true. Update or add a test alongside the existing BrowserService.openSession/snapshotDom coverage to assert the artifact-backed response shape and that the browser host command flow still includes snapshot_dom.
🤖 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 @.tmp-agent-handoff.md:
- Around line 121-135: The fenced handoff/architecture blocks in the referenced
markdown need language identifiers added to satisfy markdownlint MD040. Update
the affected fenced sections in the handoff doc so both opening and closing
fences use text as the language tag, keeping the existing content unchanged;
this applies to the blocks around the listed browser/server file references and
the later related block mentioned in the comment.
In `@apps/desktop/src/browser/BrowserKernel.ts`:
- Around line 43-49: Move the webRequest network hooks off each HostedSession
and attach them once to the shared Electron Session returned by
BrowserProfiles.resolve(), since installWebContentsListeners() currently
re-registers handlers per hosted session and can overwrite capture for the same
profile. Also add cleanup/invalidation for domRefCache in the browser lifecycle
so navigation, reload, back, and forward reset cached element bounds and
ref-based clicks don’t use stale data. Use the existing BrowserKernel and
installWebContentsListeners() flow as the main place to centralize both fixes.
In `@apps/server/src/browser/BrowserMcpBridge.ts`:
- Around line 230-261: The browserMcpBridgeRequest promise can stay pending if
the socket connects but never yields a response, so harden the request path in
BrowserMcpBridge.ts. Add a connection/response timeout around the
createConnection(socketPath) flow, and make sure the Promise settles on socket
close/end without a parsed response by rejecting with a clear error. Update the
handlers in browserMcpBridgeRequest to track whether a response was already
received so that connect/data/error/close/end all resolve or reject exactly
once.
- Around line 129-134: The BrowserMcpBridge socket setup can exceed the UNIX
socket path limit and the socket directory is too permissive. In
BrowserMcpBridge, shorten the generated socket name in the socketPath creation
logic (the input.threadId plus randomUUID path) so it stays safely under
sun_path limits, and ensure SOCKET_DIR is created with 0700 permissions before
use. Keep the stale-socket cleanup via Effect.tryPromise/rm, but update the
surrounding socket initialization to enforce the shorter path and locked-down
directory.
In `@apps/server/src/browser/BrowserRuntimeMcpConfig.ts`:
- Around line 10-19: The fallback in resolveBrowserMcpStdioCommand assumes the
bun executable is available on PATH, which can break when apps/server is started
via node or npx. Update the command resolution to first verify Bun is actually
available (or otherwise detect a valid Bun runtime) before returning the MCP
bridge spawn command, and if it is not available, throw or surface a clear error
instead of defaulting to command "bun". Keep the fix localized to
resolveBrowserMcpStdioCommand and any helper you add for Bun availability
checks.
In `@apps/server/src/provider/Layers/CursorAdapter.ts`:
- Around line 517-557: The browser bridge startup in CursorAdapter leaks a
registered bridge when session setup fails before sessions.set(...) runs. Add a
session-scoped finalizer around the Effect.gen block that calls
stopBrowserBridgeForThread(input.threadId) so any bridge started via
bridge.start is cleaned up even if makeCursorAcpRuntime or a later step throws.
Keep the cleanup adjacent to the existing session lifecycle logic in
CursorAdapter and preserve the current bridge start/error handling.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 990-992: The browser turn-cleanup is implemented directly inside
OpenCodeAdapter’s turn-completion/error handlers, duplicating provider-specific
cleanup logic across adapters. Move the cleanup responsibility out of
OpenCodeAdapter (and the similar CopilotAdapter path) into the orchestration
layer, specifically the turn-completion handling in CheckpointReactor, so
runtime events are projected centrally through the existing
ProviderRuntimeIngestion/ProviderCommandReactor/CheckpointReactor flow. Keep
OpenCodeAdapter limited to emitting provider events and remove the direct
cleanup calls around cleanupBrowserThreadAfterProviderTurn, including the
repeated branch near the second referenced occurrence.
- Around line 1216-1222: `startSession` in `OpenCodeAdapter` should not fail
session startup if `connectOpenCodeBrowserMcp` errors after the session is
already registered and the event pump has started. Wrap the desktop-only
`connectOpenCodeBrowserMcp(context, directory)` call in `Effect.ignore`,
matching the `sendTurn` call site behavior, so bridge/MCP setup failures are
swallowed and the live session entry in `sessions` is not left orphaned.
---
Outside diff comments:
In `@apps/server/src/browser/BrowserArtifactStore.ts`:
- Around line 59-124: The BrowserArtifactStoreLive implementation computes and
stores expiresAt in BrowserArtifactRecord, but nothing ever removes expired
artifacts from records or disk. Add expiry handling in the
BrowserArtifactStoreShape methods by either introducing a periodic
cleanup/reaper in BrowserArtifactStoreLive or checking expiry on
get/readData/put and deleting stale entries/files before returning. Make sure
the cleanup updates the records Ref and removes the corresponding filePath for
each expired BrowserArtifactRecord.
In `@apps/server/src/browser/BrowserService.ts`:
- Around line 537-599: openSession currently performs the “is this profile
already attached?” check and the session insert as separate steps, which allows
two concurrent callers to race and both create active sessions. Update
BrowserService.openSession so the profile claim happens atomically inside the
state update path: check the current sessions and reserve the profile/session in
one Ref.update/modify-style operation before continuing, and only proceed with
makeSession/executeSessionCommand after the reservation succeeds. Use the
existing openSession, profileFor, state, and sessionOwners/session maps to
locate the logic.
---
Nitpick comments:
In `@apps/server/src/browser/BrowserMcpBridge.ts`:
- Around line 95-98: BrowserMcpBridgeLive currently creates and stores
ActiveBridge instances in bridgesRef but never cleans them up when the layer
shuts down. Add an Effect.addFinalizer inside the BrowserMcpBridgeLive
Effect.gen setup that iterates over the remaining bridges in bridgesRef and
stops each one, ensuring any net.Server listeners and .sock files are released
even if stop was never called per thread. Use the existing BrowserMcpBridgeLive,
bridgesRef, and ActiveBridge stop/cleanup logic to wire this teardown into the
layer lifecycle.
In `@apps/server/src/browser/BrowserObservationHelpers.ts`:
- Around line 12-55: Replace the hand-rolled validation in
decodeBrowserHostScreenshotData, decodeBrowserHostDomSnapshotData,
decodeBrowserHostConsoleData, and decodeBrowserHostNetworkData with the exported
effect schemas from `@ryco/contracts` using Schema.decodeUnknownOption. The
current checks only validate shallow fields and unsafe-cast nested data, so wire
each decoder to its matching schema to get full nested validation (including
dom_snapshot tree and entries shapes) and return null on decode failure.
In `@apps/server/src/browser/BrowserService.test.ts`:
- Around line 419-460: The BrowserService.snapshotDom test only covers the
normal in-memory response path; add a case for the oversized snapshot branch
where snapshotJson.length exceeds 65_536 so BrowserService stores the snapshot
as an artifact, returns an empty tree, and marks truncated as true. Update or
add a test alongside the existing BrowserService.openSession/snapshotDom
coverage to assert the artifact-backed response shape and that the browser host
command flow still includes snapshot_dom.
In `@apps/server/src/browser/BrowserService.ts`:
- Around line 949-988: The waitFor polling in BrowserService is repeatedly
triggering snapshot_dom through dispatchObservationCommand, which causes
saveSession and session.updated broadcasts on every 250ms tick even when nothing
changes. Update the waitFor flow to avoid redundant host work by caching the
last observed session/tab snapshot (or visible text) and only calling the
snapshot/save/publish path when the observed state actually differs, or
otherwise reduce the polling frequency over time. Focus the change around
waitFor, dispatchObservationCommand, and executeHostDataCommand so text/textGone
waits stop spamming unchanged updates.
In `@apps/server/src/provider/Layers/CopilotAdapter.mapEvent.ts`:
- Around line 216-219: The tool classification in CopilotAdapter.mapEvent is
using a raw browser_ prefix check that can mislabel MCP tools as browser tools.
Update the tool execution mapping logic to use the canonical
isBrowserRuntimeToolName helper from BrowserRuntimeTool instead of
startsWith("browser_"), and keep the existing isMcpTool branch so MCP-hosted
tools like tool.execution_start and the related mapping block are classified
consistently.
In `@apps/server/src/provider/Layers/CopilotAdapter.ts`:
- Line 258: The browser tool executor in CopilotAdapter is running effects with
the default runtime, which drops the adapter’s fiber/runtime context. Update
makeCopilotAdapter to capture the Effect context once, then thread that
runtimeContext into buildSessionConfig so the runPromise implementation uses
Effect.runPromiseWith(runtimeContext) instead of Effect.runPromise; keep the
behavior aligned with the Cursor and Claude adapter implementations.
In `@apps/server/src/provider/tools/BrowserRuntimeTool.ts`:
- Around line 243-248: The readBrowserToolLoadState helper is duplicating the
BrowserLoadState allowed values with hardcoded literals, which can drift from
the contract schema. Update readBrowserToolLoadState in BrowserRuntimeTool to
validate using Schema.is(BrowserLoadState), matching the pattern already used
for browser_input with Schema.is(BrowserInputAction), so the helper always stays
aligned with the schema definition.
- Around line 368-451: The `missing_session` validation is duplicated in
`browser_navigate`, the
`browser_back`/`browser_forward`/`browser_reload`/`browser_stop` branch, and
`browser_input` instead of reusing `requireSession`. Update `BrowserRuntimeTool`
so those cases call `requireSession(input.name)` (or an equivalent shared helper
flow) before invoking `browser.navigate`, `browser[command]`, or
`browser.input`, and keep the existing `BrowserRuntimeToolError` behavior
centralized there for consistent session-id handling.
In `@apps/server/src/provider/tools/BrowserRuntimeToolHelpers.ts`:
- Around line 39-55: The validation in readScreenshotArtifact is intentionally
shallow, but it currently looks like a full contract check even though only
artifactId and mimeType are used. Update the guard in readScreenshotArtifact to
clearly document that it is a partial validation for BrowserArtifactRef, and
note that enrichBrowserRuntimeToolCallToolResult and downstream consumers only
depend on those fields. Keep the existing checks, but add an explicit comment
near the artifact cast explaining the intentional scope of validation.
In `@apps/server/src/provider/tools/BrowserToolLifecycleEvents.ts`:
- Around line 22-101: Both BrowserToolLifecycleEvents builders duplicate the
same event scaffolding, so extract the shared base event creation from
makeBrowserToolStartedEvent and makeBrowserToolCompletedEvent into a helper.
Move the common pieces like EventId.make, provider/providerInstanceId/turnId
spreads, threadId, createdAt, and raw.source assembly into a shared function,
then let each builder add only its specific type and payload fields.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9843c7a9-9003-400b-9b44-3515c514eb8e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
.tmp-agent-handoff.mdapps/desktop/src/browser/BrowserHostConnection.tsapps/desktop/src/browser/BrowserKernel.tsapps/desktop/src/browser/BrowserObservationHelpers.test.tsapps/desktop/src/browser/BrowserObservationHelpers.tsapps/desktop/src/browser/BrowserSurfaceManager.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/providerService.integration.test.tsapps/server/package.jsonapps/server/src/browser/BrowserArtifactStore.test.tsapps/server/src/browser/BrowserArtifactStore.tsapps/server/src/browser/BrowserMcpBridge.tsapps/server/src/browser/BrowserObservationHelpers.tsapps/server/src/browser/BrowserRuntimeMcpConfig.tsapps/server/src/browser/BrowserService.test.tsapps/server/src/browser/BrowserService.tsapps/server/src/browser/BrowserThreadCleanup.tsapps/server/src/browser/browser-mcp-stdio.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CodexSessionRuntime.tsapps/server/src/provider/Layers/CopilotAdapter.mapEvent.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/acp/AcpSessionRuntime.tsapps/server/src/provider/tools/BrowserRuntimeTool.test.tsapps/server/src/provider/tools/BrowserRuntimeTool.tsapps/server/src/provider/tools/BrowserRuntimeToolHelpers.test.tsapps/server/src/provider/tools/BrowserRuntimeToolHelpers.tsapps/server/src/provider/tools/BrowserRuntimeToolTestLayers.tsapps/server/src/provider/tools/BrowserToolLifecycleEvents.tsapps/server/src/provider/tools/ClaudeBrowserRuntimeTools.test.tsapps/server/src/provider/tools/ClaudeBrowserRuntimeTools.tsapps/server/src/provider/tools/CopilotBrowserRuntimeTools.tsapps/server/src/provider/tools/ProviderRuntimeEventHub.tsapps/server/src/provider/tools/ProviderRuntimeToolRegistry.tsapps/server/src/server.test.tsapps/server/src/server.tsapps/server/src/ws/browserRpc.tsapps/web/src/components/BrowserPanel.logic.test.tsapps/web/src/components/BrowserPanel.logic.tsapps/web/src/components/BrowserPanel.tsxapps/web/src/localApi.test.tsapps/web/src/rpc/protocol.tspackages/contracts/src/browser.tspackages/contracts/src/rpc.tspackages/shared/src/browser.test.tspackages/shared/src/browser.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/server/src/ws/browserRpc.ts
- packages/shared/src/browser.test.ts
- apps/desktop/src/browser/BrowserSurfaceManager.ts
- apps/desktop/src/browser/BrowserHostConnection.ts
- apps/web/src/components/BrowserPanel.tsx
- apps/server/src/server.test.ts
- apps/server/src/server.ts
| ``` | ||
| apps/server/src/server.ts # layer wiring | ||
| apps/server/src/browser/BrowserService.ts # observation + policy | ||
| apps/server/src/provider/tools/BrowserRuntimeTool.ts | ||
| apps/server/src/provider/tools/ProviderRuntimeToolRegistry.ts | ||
| apps/server/src/provider/tools/ClaudeBrowserRuntimeTools.ts | ||
| apps/server/src/provider/tools/CopilotBrowserRuntimeTools.ts | ||
| apps/server/src/provider/Layers/CodexSessionRuntime.ts | ||
| apps/server/src/browser/BrowserMcpBridge.ts | ||
| apps/desktop/src/browser/BrowserKernel.ts | ||
| apps/web/src/components/BrowserPanel.tsx | ||
| apps/server/src/server.test.ts # fix test layer + typecheck (~line 610 browserLayer, ~line 419 buildAppUnderTest) | ||
| docs/superpowers/specs/2026-06-22-in-app-browser-design.md | ||
| docs/superpowers/plans/2026-06-22-in-app-browser.md | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the fenced blocks.
Both fences are missing a language identifier, which triggers markdownlint MD040. Use text for these handoff/architecture blocks.
Suggested edit
-```
+```text
...
-```
+```textAlso applies to: 141-149
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.tmp-agent-handoff.md around lines 121 - 135, The fenced
handoff/architecture blocks in the referenced markdown need language identifiers
added to satisfy markdownlint MD040. Update the affected fenced sections in the
handoff doc so both opening and closing fences use text as the language tag,
keeping the existing content unchanged; this applies to the blocks around the
listed browser/server file references and the later related block mentioned in
the comment.
Source: Linters/SAST tools
| interface HostedSession { | ||
| session: BrowserSessionSnapshot; | ||
| view: WebContentsView; | ||
| consoleEntries: BufferedConsoleEntry[]; | ||
| networkEntries: BufferedNetworkEntry[]; | ||
| networkRequestStartedAt: Map<string, string>; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether BrowserProfiles resolves persistent profile modes to a shared Electron session partition.
fd -a BrowserProfiles apps/desktop/src/browser
rg -n "fromPartition|persist:" apps/desktop/src/browser -A3 -B3Repository: sak0a/ryco
Length of output: 843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- BrowserProfiles.ts ---'
wc -l apps/desktop/src/browser/BrowserProfiles.ts
cat -n apps/desktop/src/browser/BrowserProfiles.ts | sed -n '1,240p'
echo
echo '--- BrowserKernel.ts (relevant ranges) ---'
wc -l apps/desktop/src/browser/BrowserKernel.ts
cat -n apps/desktop/src/browser/BrowserKernel.ts | sed -n '1,260p'Repository: sak0a/ryco
Length of output: 12451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- installWebContentsListeners / session lifecycle ---'
rg -n "installWebContentsListeners|closeSession|navigationAction|snapshotDomPayload|domRefCache|webRequest|onBeforeRequest|onCompleted|onErrorOccurred" apps/desktop/src/browser/BrowserKernel.ts -A8 -B8
echo
echo '--- BrowserKernel.ts around the relevant methods ---'
sed -n '260,760p' apps/desktop/src/browser/BrowserKernel.ts | cat -nRepository: sak0a/ryco
Length of output: 30013
Attach the network hooks to the shared Electron Session, not each hosted session
BrowserProfiles.resolve()returns the same persistentSessionfor a profileId, sobrowserSession.webRequest.onBeforeRequest/onCompleted/onErrorOccurredininstallWebContentsListeners()will overwrite the previous hosted session’s network capture on that profile.domRefCacheis also never cleared on navigation/reload/back/forward, so ref-based clicks can use stale bounds after the page changes.
🤖 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 `@apps/desktop/src/browser/BrowserKernel.ts` around lines 43 - 49, Move the
webRequest network hooks off each HostedSession and attach them once to the
shared Electron Session returned by BrowserProfiles.resolve(), since
installWebContentsListeners() currently re-registers handlers per hosted session
and can overwrite capture for the same profile. Also add cleanup/invalidation
for domRefCache in the browser lifecycle so navigation, reload, back, and
forward reset cached element bounds and ref-based clicks don’t use stale data.
Use the existing BrowserKernel and installWebContentsListeners() flow as the
main place to centralize both fixes.
| const socketId = crypto.randomUUID(); | ||
| const socketPath = nodePath.join(SOCKET_DIR, `${input.threadId}-${socketId}.sock`); | ||
| yield* Effect.tryPromise({ | ||
| try: () => rm(socketPath, { force: true }), | ||
| catch: () => new Error("Failed to remove stale browser MCP bridge socket."), | ||
| }).pipe(Effect.ignore); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby socket setup code.
git ls-files 'apps/server/src/browser/BrowserMcpBridge.ts' 'apps/server/src/browser/*' | sed -n '1,20p'
echo '---'
wc -l apps/server/src/browser/BrowserMcpBridge.ts
echo '---'
sed -n '1,220p' apps/server/src/browser/BrowserMcpBridge.ts
echo '---'
rg -n "SOCKET_DIR|mkdtemp|mkdir|chmod|socket|sun_path|listen|net\.createServer|unix" apps/server/src/browser -SRepository: sak0a/ryco
Length of output: 14180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for other socket directory creation / permission handling in the server codebase.
rg -n "SOCKET_DIR|mkdtemp|chmod\\(|mode:\\s*0o700|0o700|createServer\\(|listen\\(" apps/server/src -SRepository: sak0a/ryco
Length of output: 12565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '[BrowserMcpBridge.ts]'
wc -l apps/server/src/browser/BrowserMcpBridge.ts
echo '---'
sed -n '1,220p' apps/server/src/browser/BrowserMcpBridge.ts
echo '---[socket dir handling search]'
rg -n "SOCKET_DIR|mkdtemp|mkdir\\(|chmod\\(|0o700|createServer\\(|listen\\(" apps/server/src -S
echo '---[path-length probe]'
python3 - <<'PY'
from pathlib import Path
import re
p = Path('apps/server/src/browser/BrowserMcpBridge.ts').read_text()
for name in ['SOCKET_DIR','threadId','randomUUID','socketPath']:
m = re.search(rf'.*{name}.*', p)
if m:
print(m.group(0))
PYRepository: sak0a/ryco
Length of output: 22172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ThreadId definition and its typical shape/usages.
rg -n "export type ThreadId|type ThreadId|interface ThreadId|ThreadId" apps packages -SRepository: sak0a/ryco
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the actual ThreadId definition and a few call sites.
files=$(rg -l "ThreadId" . | head -n 20)
printf '%s\n' "$files"
echo '---'
for f in $files; do
case "$f" in
*node_modules*) continue;;
esac
if [ -f "$f" ]; then
echo "### $f"
rg -n "ThreadId|threadId" "$f" -S | sed -n '1,20p'
fi
doneRepository: sak0a/ryco
Length of output: 15176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg 'ThreadId|contracts' -nRepository: sak0a/ryco
Length of output: 2139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Estimate socket path lengths under realistic TMPDIR values.
python3 - <<'PY'
from pathlib import Path
samples = [
"/tmp",
"/var/folders/zz/zyxvpxvq6csfxvn_n00000sm00006d/T",
"/var/folders/ab/cdefghijklmnopqrstuvwx/T",
]
thread_ids = ["1", "thread-1234567890", "0123456789abcdef0123456789abcdef"]
uuid = "123e4567-e89b-12d3-a456-426614174000"
for tmp in samples:
base = Path(tmp) / "ryco-browser-mcp"
for tid in thread_ids:
path = base / f"{tid}-{uuid}.sock"
print(len(str(path).encode()), str(path))
print("---")
PYRepository: sak0a/ryco
Length of output: 1168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show Node's unix socket path limit behavior from docs isn't directly available here,
# but inspect the current code for any permission hardening around the socket dir.
sed -n '1,260p' apps/server/src/browser/BrowserMcpBridge.ts | nl -ba | sed -n '120,150p'Repository: sak0a/ryco
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '^.*contracts.*\.(ts|tsx|js|jsx)$|^.*ThreadId.*$' -nRepository: sak0a/ryco
Length of output: 2064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "export (type|interface) ThreadId|type ThreadId|interface ThreadId" . -SRepository: sak0a/ryco
Length of output: 2171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use the repository's file list to locate the contracts package and inspect any ThreadId type.
git ls-files | rg 'contracts|ThreadId' | sed -n '1,120p'Repository: sak0a/ryco
Length of output: 1904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "threadId" apps/server/src apps/client/src packages src -S | sed -n '1,200p'Repository: sak0a/ryco
Length of output: 20081
Shorten the socket path and lock down the socket dir A long TMPDIR can push this UNIX socket path over the macOS/Linux sun_path limit, and the directory should be created with 0700 so other local users can’t connect.
🤖 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 `@apps/server/src/browser/BrowserMcpBridge.ts` around lines 129 - 134, The
BrowserMcpBridge socket setup can exceed the UNIX socket path limit and the
socket directory is too permissive. In BrowserMcpBridge, shorten the generated
socket name in the socketPath creation logic (the input.threadId plus randomUUID
path) so it stays safely under sun_path limits, and ensure SOCKET_DIR is created
with 0700 permissions before use. Keep the stale-socket cleanup via
Effect.tryPromise/rm, but update the surrounding socket initialization to
enforce the shorter path and locked-down directory.
| return new Promise((resolve, reject) => { | ||
| const id = crypto.randomUUID(); | ||
| const socket = createConnection(input.socketPath); | ||
| let buffer = ""; | ||
| socket.on("connect", () => { | ||
| socket.write( | ||
| `${JSON.stringify({ | ||
| id, | ||
| toolName: input.toolName, | ||
| arguments: input.arguments, | ||
| })}\n`, | ||
| ); | ||
| }); | ||
| socket.on("data", (chunk) => { | ||
| buffer += chunk.toString("utf8"); | ||
| const newlineIndex = buffer.indexOf("\n"); | ||
| if (newlineIndex === -1) return; | ||
| const line = buffer.slice(0, newlineIndex).trim(); | ||
| socket.end(); | ||
| try { | ||
| const response = JSON.parse(line) as BrowserMcpBridgeResponse; | ||
| if (!response.ok) { | ||
| reject(new Error(response.message ?? "Browser MCP bridge request failed.")); | ||
| return; | ||
| } | ||
| resolve(response.result); | ||
| } catch (cause) { | ||
| reject(cause); | ||
| } | ||
| }); | ||
| socket.on("error", reject); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
browserMcpBridgeRequest can hang indefinitely.
There is no connection/response timeout and no close/end handler. If the bridge server accepts the connection but closes without writing a response (e.g., server crash mid-request, or socket.end() before write), neither data nor error fires with a settled result, so the returned Promise never resolves or rejects. Since this is awaited from MCP tool handlers, a stuck request blocks the tool call.
Suggested hardening
return new Promise((resolve, reject) => {
const id = crypto.randomUUID();
const socket = createConnection(input.socketPath);
let buffer = "";
+ let settled = false;
+ const finish = (fn: () => void) => {
+ if (settled) return;
+ settled = true;
+ socket.end();
+ fn();
+ };
+ socket.setTimeout(30_000, () =>
+ finish(() => reject(new Error("Browser MCP bridge request timed out."))),
+ );
socket.on("connect", () => {
socket.write(
`${JSON.stringify({ id, toolName: input.toolName, arguments: input.arguments })}\n`,
);
});
socket.on("data", (chunk) => {
buffer += chunk.toString("utf8");
const newlineIndex = buffer.indexOf("\n");
if (newlineIndex === -1) return;
const line = buffer.slice(0, newlineIndex).trim();
- socket.end();
try {
const response = JSON.parse(line) as BrowserMcpBridgeResponse;
- if (!response.ok) {
- reject(new Error(response.message ?? "Browser MCP bridge request failed."));
- return;
- }
- resolve(response.result);
+ finish(() =>
+ response.ok
+ ? resolve(response.result)
+ : reject(new Error(response.message ?? "Browser MCP bridge request failed.")),
+ );
} catch (cause) {
- reject(cause);
+ finish(() => reject(cause));
}
});
- socket.on("error", reject);
+ socket.on("error", (cause) => finish(() => reject(cause)));
+ socket.on("close", () =>
+ finish(() => reject(new Error("Browser MCP bridge connection closed before response."))),
+ );
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise((resolve, reject) => { | |
| const id = crypto.randomUUID(); | |
| const socket = createConnection(input.socketPath); | |
| let buffer = ""; | |
| socket.on("connect", () => { | |
| socket.write( | |
| `${JSON.stringify({ | |
| id, | |
| toolName: input.toolName, | |
| arguments: input.arguments, | |
| })}\n`, | |
| ); | |
| }); | |
| socket.on("data", (chunk) => { | |
| buffer += chunk.toString("utf8"); | |
| const newlineIndex = buffer.indexOf("\n"); | |
| if (newlineIndex === -1) return; | |
| const line = buffer.slice(0, newlineIndex).trim(); | |
| socket.end(); | |
| try { | |
| const response = JSON.parse(line) as BrowserMcpBridgeResponse; | |
| if (!response.ok) { | |
| reject(new Error(response.message ?? "Browser MCP bridge request failed.")); | |
| return; | |
| } | |
| resolve(response.result); | |
| } catch (cause) { | |
| reject(cause); | |
| } | |
| }); | |
| socket.on("error", reject); | |
| }); | |
| return new Promise((resolve, reject) => { | |
| const id = crypto.randomUUID(); | |
| const socket = createConnection(input.socketPath); | |
| let buffer = ""; | |
| let settled = false; | |
| const finish = (fn: () => void) => { | |
| if (settled) return; | |
| settled = true; | |
| socket.end(); | |
| fn(); | |
| }; | |
| socket.setTimeout(30_000, () => | |
| finish(() => reject(new Error("Browser MCP bridge request timed out."))), | |
| ); | |
| socket.on("connect", () => { | |
| socket.write( | |
| `${JSON.stringify({ | |
| id, | |
| toolName: input.toolName, | |
| arguments: input.arguments, | |
| })}\n`, | |
| ); | |
| }); | |
| socket.on("data", (chunk) => { | |
| buffer += chunk.toString("utf8"); | |
| const newlineIndex = buffer.indexOf("\n"); | |
| if (newlineIndex === -1) return; | |
| const line = buffer.slice(0, newlineIndex).trim(); | |
| try { | |
| const response = JSON.parse(line) as BrowserMcpBridgeResponse; | |
| finish(() => | |
| response.ok | |
| ? resolve(response.result) | |
| : reject(new Error(response.message ?? "Browser MCP bridge request failed.")), | |
| ); | |
| } catch (cause) { | |
| finish(() => reject(cause)); | |
| } | |
| }); | |
| socket.on("error", (cause) => finish(() => reject(cause))); | |
| socket.on("close", () => | |
| finish(() => reject(new Error("Browser MCP bridge connection closed before response."))), | |
| ); | |
| }); |
🤖 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 `@apps/server/src/browser/BrowserMcpBridge.ts` around lines 230 - 261, The
browserMcpBridgeRequest promise can stay pending if the socket connects but
never yields a response, so harden the request path in BrowserMcpBridge.ts. Add
a connection/response timeout around the createConnection(socketPath) flow, and
make sure the Promise settles on socket close/end without a parsed response by
rejecting with a clear error. Update the handlers in browserMcpBridgeRequest to
track whether a response was already received so that
connect/data/error/close/end all resolve or reject exactly once.
| export function resolveBrowserMcpStdioCommand(): { | ||
| readonly command: string; | ||
| readonly args: ReadonlyArray<string>; | ||
| } { | ||
| const runtime = process.execPath; | ||
| const usesBun = nodePath.basename(runtime).includes("bun"); | ||
| return usesBun | ||
| ? { command: runtime, args: [browserMcpStdioEntry] } | ||
| : { command: "bun", args: [browserMcpStdioEntry] }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is bun expected to always be present in PATH for a Node.js-hosted server that spawns bun as a child process?
💡 Result:
No, Bun is not expected to always be present in the system PATH for a Node.js-hosted server to successfully spawn a Bun child process. However, whether the process succeeds depends on how you resolve the executable and configure the environment. When spawning a child process (using either the native node:child_process module or Bun's own spawn APIs), you have two primary ways to avoid reliance on the system PATH: 1. Use an Absolute Path: When you provide the full, absolute file path to the Bun executable (e.g., /usr/local/bin/bun or a custom installation directory) as the command argument, the operating system does not need to look up the location in the PATH variable [1][2]. 2. Explicit Environment Configuration: If you need to spawn a process using a bare command name (like bun), you can explicitly provide the environment variables via the env option in your spawn configuration [1][2]. By passing an env object that includes the necessary PATH, you ensure the spawned process can locate the executable without relying on the parent process's environment or the system-wide global PATH [1][2]. Important Considerations: - Execution Context: If you do not provide an explicit environment, Bun generally defaults to using the process.env snapshot from when the parent process was launched [1][2]. Recent updates have improved how Bun handles runtime mutations to process.env.PATH, particularly for synchronous spawn variants, to ensure they more closely mirror Node.js behavior [3]. - Troubleshooting: If you encounter errors like Executable not found in $PATH, it typically indicates that the environment the child process is inheriting does not include the directory where Bun is installed [4][5]. This is common in containerized environments or restricted shell sessions. You can resolve this by either using an absolute path or by explicitly injecting the correct PATH into the child process's environment [1][2]. In summary, while using the system PATH is the default and most convenient method, it is not a strict requirement if you manage the executable's location or environment configuration explicitly in your code [1][2].
Citations:
- 1: https://bun.sh/reference/bun/spawnSync
- 2: https://bun.sh/reference/bun/spawn
- 3: child_process: honor runtime process.env.PATH in sync spawn variants oven-sh/bun#29239
- 4: bun create react-app fails unless Node is installed oven-sh/bun#5995
- 5: process.env.PATH issues oven-sh/bun#11182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target file =="
ast-grep outline apps/server/src/browser/BrowserRuntimeMcpConfig.ts --view expanded || true
echo
echo "== File contents =="
cat -n apps/server/src/browser/BrowserRuntimeMcpConfig.ts
echo
echo "== References =="
rg -n "resolveBrowserMcpStdioCommand|browserMcpStdioEntry|BrowserRuntimeMcpConfig|McpStdio" apps/server -S
echo
echo "== Nearby runtime / spawn logic =="
rg -n "spawn\\(|command: \"bun\"|process.execPath|process.env.PATH|PATH" apps/server/src -SRepository: sak0a/ryco
Length of output: 21269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Package scripts and runtime hints =="
for f in package.json apps/server/package.json bun.lockb bun.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f"
echo
fi
done
echo "== Bun / Node mentions in docs and config =="
rg -n "bun|node\.|process\.execPath|PATH|runtime" README* docs apps/server -g '!**/node_modules/**' -SRepository: sak0a/ryco
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json scripts =="
python3 - <<'PY'
import json, pathlib
for path in ["package.json", "apps/server/package.json"]:
p = pathlib.Path(path)
if p.exists():
data = json.loads(p.read_text())
print(f"--- {path} ---")
scripts = data.get("scripts", {})
for k in sorted(scripts):
print(f"{k}: {scripts[k]}")
print()
PY
echo "== apps/server/README.md (selected lines) =="
sed -n '1,120p' apps/server/README.md
echo
echo "== bun mentions in root README and server README =="
rg -n "bun|Node|node" README.md apps/server/README.md -SRepository: sak0a/ryco
Length of output: 6659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Browser runtime MCP related references =="
rg -n "BrowserRuntimeMcpConfig|browser-mcp-stdio|resolveBrowserMcpStdioCommand|makeOpenCodeBrowserMcpConfig|makeCopilotBrowserMcpServerConfig|makeAcpBrowserMcpServer" apps/server -S
echo
echo "== MCP stdio bridge file =="
cat -n apps/server/src/browser/browser-mcp-stdio.tsRepository: sak0a/ryco
Length of output: 3411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json scripts =="
python3 - <<'PY'
import json, pathlib
for path in ["package.json", "apps/server/package.json"]:
p = pathlib.Path(path)
if p.exists():
data = json.loads(p.read_text())
print(f"--- {path} ---")
for k, v in sorted(data.get("scripts", {}).items()):
print(f"{k}: {v}")
print()
PY
echo "== apps/server/README.md =="
sed -n '1,120p' apps/server/README.md
echo
echo "== Root README.md bun/node mentions =="
sed -n '1,120p' README.md 2>/dev/null || true
rg -n "bun|node" README.md apps/server/README.md -SRepository: sak0a/ryco
Length of output: 13226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/server/README.md (top) =="
sed -n '1,120p' apps/server/README.md
echo
echo "== package.json scripts only =="
python3 - <<'PY'
import json
from pathlib import Path
for path in [Path("package.json"), Path("apps/server/package.json")]:
if path.exists():
data = json.loads(path.read_text())
print(f"--- {path} ---")
for k, v in sorted(data.get("scripts", {}).items()):
print(f"{k}: {v}")
print()
PY
echo
echo "== top-level docs mentions of bun/node =="
rg -n "bun|Node|node" README.md apps/server/README.md docs/observability.md docs/effect-fn-checklist.md -SRepository: sak0a/ryco
Length of output: 7086
Don't assume bun is on PATH here. apps/server is launched via node/npx, so this fallback can fail when Bun isn’t installed; add an availability check or surface a clearer error before spawning the MCP bridge.
🤖 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 `@apps/server/src/browser/BrowserRuntimeMcpConfig.ts` around lines 10 - 19, The
fallback in resolveBrowserMcpStdioCommand assumes the bun executable is
available on PATH, which can break when apps/server is started via node or npx.
Update the command resolution to first verify Bun is actually available (or
otherwise detect a valid Bun runtime) before returning the MCP bridge spawn
command, and if it is not available, throw or surface a clear error instead of
defaulting to command "bun". Keep the fix localized to
resolveBrowserMcpStdioCommand and any helper you add for Bun availability
checks.
| if (serverConfig.mode === "desktop") { | ||
| yield* Option.match(browserMcpBridge, { | ||
| onNone: () => Effect.void, | ||
| onSome: (bridge) => | ||
| Option.match(providerRuntimeToolRegistry, { | ||
| onNone: () => Effect.void, | ||
| onSome: (registry) => | ||
| Effect.gen(function* () { | ||
| const bridgeHandle = yield* bridge | ||
| .start({ | ||
| threadId: input.threadId, | ||
| runPromise, | ||
| executeBrowserTool: (toolInput) => | ||
| registry.executeBrowserTool(toolInput, { | ||
| lifecycle: { | ||
| provider: PROVIDER, | ||
| providerInstanceId: boundInstanceId, | ||
| ...(ctxHolder.current?.activeTurnId | ||
| ? { turnId: ctxHolder.current.activeTurnId } | ||
| : {}), | ||
| }, | ||
| }), | ||
| }) | ||
| .pipe( | ||
| Effect.mapError( | ||
| (cause) => | ||
| new ProviderAdapterProcessError({ | ||
| provider: PROVIDER, | ||
| threadId: input.threadId, | ||
| detail: cause.message, | ||
| cause, | ||
| }), | ||
| ), | ||
| ); | ||
| browserMcpServers.push( | ||
| makeAcpBrowserMcpServer({ socketPath: bridgeHandle.socketPath }), | ||
| ); | ||
| }), | ||
| }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant ranges.
ast-grep outline apps/server/src/provider/Layers/CursorAdapter.ts --view expanded || true
printf '\n--- around bridge.start ---\n'
sed -n '470,580p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- around session registration/cleanup ---\n'
sed -n '840,930p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- around turn cleanup ---\n'
sed -n '980,1045p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- browser bridge implementation references ---\n'
rg -n "BrowserMcpBridgeLive|browserMcpBridge|start\\(|stopSessionInternal|sessions|socketPath" apps/server/src/provider -SRepository: sak0a/ryco
Length of output: 33463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- stopSessionInternal and helpers ---\n'
sed -n '420,470p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- cleanupBrowserThreadAfterProviderTurn ---\n'
sed -n '1070,1125p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- BrowserMcpBridge interface/provider ---\n'
rg -n "export interface BrowserMcpBridge|class .*BrowserMcpBridge|Layer\\.succeed\\(BrowserMcpBridge|start: \\(" apps/server/src/provider -S
printf '\n--- Browser runtime tool layer ---\n'
sed -n '1,120p' apps/server/src/provider/tools/BrowserRuntimeToolTestLayers.ts
printf '\n--- cursor provider session startup for comparison ---\n'
sed -n '460,560p' apps/server/src/provider/Layers/CursorProvider.tsRepository: sak0a/ryco
Length of output: 9621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- stopBrowserBridgeForThread and related state ---\n'
sed -n '340,430p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- BrowserMcpBridge type/implementation ---\n'
fd -a 'BrowserMcpBridge.ts' apps/server/src/provider apps/server/src -t f
printf '\n'
rg -n "export const BrowserMcpBridge|export interface BrowserMcpBridge|start: \\(|stop: \\(" apps/server/src -S
printf '\n--- cursor adapter bridge cleanup references ---\n'
rg -n "stopBrowserBridgeForThread|browserMcpServers|bridgeHandle|socketPath" apps/server/src/provider/Layers/CursorAdapter.ts -n -SRepository: sak0a/ryco
Length of output: 6551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- BrowserMcpBridge.ts ---\n'
sed -n '1,220p' apps/server/src/browser/BrowserMcpBridge.ts
printf '\n--- CursorAdapter startSession continuation after acp.start ---\n'
sed -n '560,760p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- CursorAdapter finalizers and shutdown path ---\n'
sed -n '1088,1125p' apps/server/src/provider/Layers/CursorAdapter.ts
printf '\n--- any startup-failure cleanup helpers ---\n'
rg -n "stopBrowserBridgeForThread|cleanupBrowserThreadAfterProviderTurn|sessionScopeTransferred|Effect.addFinalizer|Scope.close\\(sessionScope" apps/server/src/provider/Layers/CursorAdapter.ts -SRepository: sak0a/ryco
Length of output: 18465
Clean up the browser bridge on failed session startup. If makeCursorAcpRuntime or a later step throws before sessions.set(...), the bridge started here stays registered in BrowserMcpBridgeLive with its socket open until a later same-thread restart or process shutdown. Add a finalizer for stopBrowserBridgeForThread(input.threadId) next to the session-scope cleanup.
🤖 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 `@apps/server/src/provider/Layers/CursorAdapter.ts` around lines 517 - 557, The
browser bridge startup in CursorAdapter leaks a registered bridge when session
setup fails before sessions.set(...) runs. Add a session-scoped finalizer around
the Effect.gen block that calls stopBrowserBridgeForThread(input.threadId) so
any bridge started via bridge.start is cleaned up even if makeCursorAcpRuntime
or a later step throws. Keep the cleanup adjacent to the existing session
lifecycle logic in CursorAdapter and preserve the current bridge start/error
handling.
| yield* cleanupBrowserThreadAfterProviderTurn(context.session.threadId).pipe( | ||
| Effect.ignore, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Browser turn-cleanup logic duplicated per adapter instead of centralized.
cleanupBrowserThreadAfterProviderTurn is invoked directly inside OpenCodeAdapter's event-handling code on turn completion/error. Graph evidence shows the same pattern independently reimplemented in CopilotAdapter.ts (with stopBridge: false) and referenced via CursorAdapter.ts/CodexAdapter.ts's BrowserMcpBridge wiring. This duplicates a cross-cutting concern across every provider adapter rather than reacting centrally to the turn.completed/session.error runtime events already emitted by these adapters.
As per coding guidelines, "Provider runtime events must be projected into orchestration through ProviderRuntimeIngestion.ts, ProviderCommandReactor.ts, CheckpointReactor.ts, and OrchestrationEngine.ts" — moving this cleanup into CheckpointReactor.ts (reacting to the turn-completion event) would avoid the per-adapter duplication and drift risk (e.g., the stopBridge option already differs between OpenCode and Copilot).
Also applies to: 1022-1024
🤖 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 `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 990 - 992,
The browser turn-cleanup is implemented directly inside OpenCodeAdapter’s
turn-completion/error handlers, duplicating provider-specific cleanup logic
across adapters. Move the cleanup responsibility out of OpenCodeAdapter (and the
similar CopilotAdapter path) into the orchestration layer, specifically the
turn-completion handling in CheckpointReactor, so runtime events are projected
centrally through the existing
ProviderRuntimeIngestion/ProviderCommandReactor/CheckpointReactor flow. Keep
OpenCodeAdapter limited to emitting provider events and remove the direct
cleanup calls around cleanupBrowserThreadAfterProviderTurn, including the
repeated branch near the second referenced occurrence.
Source: Coding guidelines
| sessions.set(input.threadId, context); | ||
| yield* startEventPump(context); | ||
|
|
||
| if (serverConfig.mode === "desktop") { | ||
| yield* connectOpenCodeBrowserMcp(context, directory); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline apps/server/src/provider/Layers/OpenCodeAdapter.ts --view expandedRepository: sak0a/ryco
Length of output: 3154
🏁 Script executed:
rg -n "connectOpenCodeBrowserMcp|sendTurn|startSession|stopSession|ProviderAdapterProcessError|ProviderAdapterRequestError" apps/server/src/provider/Layers/OpenCodeAdapter.tsRepository: sak0a/ryco
Length of output: 1618
🏁 Script executed:
sed -n '470,540p;1110,1245p;1420,1485p' apps/server/src/provider/Layers/OpenCodeAdapter.tsRepository: sak0a/ryco
Length of output: 11164
🏁 Script executed:
rg -n "startEventPump|sessionScope|sessions.delete\\(|ensuring\\(|onExit\\(|tapError\\(|catchAll\\(" apps/server/src/provider/Layers/OpenCodeAdapter.tsRepository: sak0a/ryco
Length of output: 1666
🏁 Script executed:
sed -n '560,660p;1040,1110p' apps/server/src/provider/Layers/OpenCodeAdapter.tsRepository: sak0a/ryco
Length of output: 6650
Guard connectOpenCodeBrowserMcp during startSession
startSession has already registered the session and started the event pump by this point. If the desktop bridge/MCP setup fails here, startup aborts after the session is live, leaving an orphaned entry/scope until a later cleanup path happens. Wrap this in Effect.ignore like the sendTurn call site so a bridge hiccup doesn’t fail session startup.
🤖 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 `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 1216 - 1222,
`startSession` in `OpenCodeAdapter` should not fail session startup if
`connectOpenCodeBrowserMcp` errors after the session is already registered and
the event pump has started. Wrap the desktop-only
`connectOpenCodeBrowserMcp(context, directory)` call in `Effect.ignore`,
matching the `sendTurn` call site behavior, so bridge/MCP setup failures are
swallowed and the live session entry in `sessions` is not left orphaned.
1 participant
Summary
Testing
bun fmt.bun lint.bun typecheck.Summary by CodeRabbit