Add shared browser session daemon bridge#83
Merged
glittercowboy merged 2 commits intomainfrom Apr 28, 2026
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces a comprehensive browser session management system spanning daemon, service, and extension layers. Implements a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Daemon
participant Manager
participant Service
participant BrowserDaemon as Browser<br/>Daemon
Client->>Daemon: BrowserSessionOpen
Daemon->>Manager: Open(msg)
Manager->>Service: Open(OpenRequest)
Service->>BrowserDaemon: Start daemon process
Service->>BrowserDaemon: RPC: cloud_session_status
BrowserDaemon-->>Service: URL, Title
Service-->>Manager: OpenResult
Manager->>Manager: Store session state, start frameLoop
Manager->>Client: emit BrowserSessionOpened
Client->>Daemon: BrowserControlClaim
Daemon->>Manager: Claim(msg)
Manager->>Manager: Update control owner (OwnerLex)
Client->>Daemon: BrowserToolCall
Daemon->>Manager: Tool(msg)
Manager->>Service: Tool(browserID, method, params)
Service->>BrowserDaemon: RPC: cloud_tool
BrowserDaemon-->>Service: result JSON
Service-->>Manager: ToolResult
Manager->>Client: emit BrowserToolResult
Client->>Daemon: BrowserControlRelease
Daemon->>Manager: Release(msg)
Manager->>Manager: Reset control owner (OwnerAgent)
Client->>Daemon: BrowserSessionClose
Daemon->>Manager: Close(msg)
Manager->>Manager: Remove session state
Manager->>Service: Close(browserID)
Service->>BrowserDaemon: Terminate daemon
Manager->>Client: emit BrowserSessionClosed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/loop/daemon.go (1)
621-640:⚠️ Potential issue | 🔴 CriticalTask-scoped browser grants are only applied on first actor spawn.
Grant lookup is currently inside
if actor == nil, so subsequent tasks on the same session actor reuse staleBrowserGrantID/BrowserID. That can authorize browser access for tasks that were never granted.🔧 Proposed fix direction
+browserGrantID := "" +browserID := "" +if d.browserManager != nil { + if browserGrant, ok := d.browserManager.GrantForTask(msg.TaskID); ok { + browserGrantID = browserGrant.GrantID + browserID = browserGrant.BrowserID + } +} + actor := d.manager.Get(msg.SessionID) if actor == nil { // spawn with current task grant } else { + // update/clear actor browser context per incoming task before SendTask + // (add Actor.SetBrowserContext(grantID, browserID) in internal/session/actor.go) + actor.SetBrowserContext(browserGrantID, browserID) }Also ensure the actor clears browser context (
"","") when no grant exists for a task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/loop/daemon.go` around lines 621 - 640, The browser grant lookup must run for every task (not only when actor == nil) so each Spawn/Resume call gets the current grant; move the GrantForTask call (d.browserManager.GrantForTask) out of the actor==nil guard and always compute browserGrantID/browserID before calling d.manager.Spawn, passing the values into the Session options (BrowserGrantID, BrowserID). Also ensure that when no grant exists you explicitly set BrowserGrantID and BrowserID to "" to clear any prior browser context on a reused actor.
🧹 Nitpick comments (1)
internal/pi/extension/browser-tool.test.mjs (1)
5-15: Cover the no-grant branch as well.This only proves the positive path. A regression that always registers
gsd_browserwould still pass. Please add a companion assertion thatbuildClaudeCliToolsForTest({})does not surface the browser tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pi/extension/browser-tool.test.mjs` around lines 5 - 15, Add a negative-test case to assert the no-grant branch: call buildClaudeCliToolsForTest({}) and assert that none of the returned tools has name "gsd_browser" (e.g., assert.equal(tools.some(tool => tool.name === "gsd_browser"), false)). Update or add an it() in browser-tool.test.mjs alongside the existing positive test to cover the missing browserGrant path so regressions that always register gsd_browser fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager.go`:
- Around line 126-132: sendFrame currently returns early on expiry and leaves
session data and the frameLoop ticker running; change the expiry path in
Manager.sendFrame to perform a full teardown: while holding m.mu remove the
session from m.byID and m.byTask, stop/disable its frameLoop/ticker and invoke
the session’s shutdown (e.g.,
state.close()/state.browser.Close()/state.cancel()—use the existing session
shutdown method) and only then unlock; ensure no lingering references remain so
expired sessions and their frameLoop/ticker are fully cleaned up.
- Around line 209-221: UserInput currently skips checking state.expiresAt so
expired grants still accept events; update Manager.UserInput to mirror
Tool/GrantForTask by checking state.expiresAt (e.g., if
time.Now().After(state.expiresAt) return an error like "browser grant expired")
before calling m.service.UserInput, while keeping the existing owner check and
proper mutex use around reading state from m.byID and releasing m.mu before
forwarding to m.service.UserInput (refer to Manager.UserInput, state.expiresAt,
OwnerLex, m.byID, m.mu, and m.service.UserInput).
In `@internal/browser/service.go`:
- Around line 185-192: writeFrame currently writes the header and payload with
single Write calls which can result in short writes and truncated JSON-RPC
frames; update writeFrame (the function named writeFrame and the header buffer
logic) to loop until the full header and then the full payload are written
(checking the returned n and advancing the slice on each iteration) and return
any write error immediately, or alternatively construct a bytes.Reader for
header+payload and use io.Copy to ensure the entire framed payload is sent;
ensure you propagate errors from partial writes correctly.
- Around line 154-165: After establishing the unix socket (using
net.Dialer.DialContext) ensure I/O honors ctx by applying deadlines or closing
the connection when ctx is done: before calling writeFrame/readFrame set conn
deadlines using conn.SetDeadline/SetWriteDeadline/SetReadDeadline derived from
ctx.Deadline() (or a reasonable fallback), and also start a goroutine that does
<-ctx.Done() then conn.Close() to unblock blocking I/O; update the code around
the DialContext/conn usage (referencing conn, ctx, writeFrame, readFrame, and
s.socketPath(grantID)) so all subsequent reads/writes are interrupted when ctx
is canceled.
In `@internal/loop/daemon.go`:
- Around line 242-247: The code builds browserStateDir by joining homeDir and
".gsd-browser" but when os.UserHomeDir() fails (homeErr != nil) homeDir is set
to "" which makes filepath.Join produce a relative path; change the fallback so
that when homeErr != nil or homeDir == "" you set homeDir to a safe absolute
fallback (e.g., os.TempDir()) before computing browserStateDir; update the logic
around homeDir/homeErr and the browserStateDir assignment (variables: homeDir,
homeErr, browserStateDir) so the result is always an absolute path.
In `@internal/pi/executor.go`:
- Around line 168-174: The child process currently inherits the full parent
environment when cmd.Env is not set, which can leak stale GSD_BROWSER_*
variables; update the logic around cmd.Env in executor.go (the exec.Cmd variable
`cmd` and the executor struct `e` with `e.opts.BrowserGrantID`,
`e.opts.BrowserID`, `e.opts.BrowserSessionID`) to first build cmd.Env from
os.Environ() filtered to remove any existing keys starting with "GSD_BROWSER_"
and then, only if all three options are non-empty, append the three sanitized
"GSD_BROWSER_GRANT_ID", "GSD_BROWSER_ID", and "GSD_BROWSER_SESSION_ID" entries;
ensure cmd.Env is always assigned the filtered slice so no stale GSD_BROWSER_*
variables are inherited when the condition is false.
In `@internal/pi/extension/index.ts`:
- Around line 55-58: The schema makes BrowserToolParams.params optional but
elsewhere the tool's parameters schema (look for the `parameters` object and its
`input_schema.required` array) still lists "params" as required, causing
validation mismatches; fix by removing "params" from `input_schema.required` (or
conditionally include it only when params are required) so
`input_schema.required` aligns with BrowserToolParams where `params` is
optional, ensuring `parameters` and `BrowserToolParams` agree; update the
corresponding duplicate block around the `parameters` definition at the other
location (lines ~66-74) as well.
- Around line 90-119: The browserRpc function can hang and leak sockets and the
input schema is inconsistent; update browserRpc to accept an AbortSignal
(forward the executor's _signal from execute), set a connection/read timeout
(e.g., setTimeout or socket.setTimeout) that destroys the socket and rejects the
promise, listen and handle 'end' and 'close' events to parse/cleanup or reject
on premature close, call socket.destroy() on error/timeout/abort, and remove the
unused 'params' entry from the input_schema.required array so it matches the
TypeBox Optional params; reference browserRpc (add an AbortSignal parameter and
signal handler), the execute call site (forward _signal to browserRpc), and
input_schema.required for the schema fix.
---
Outside diff comments:
In `@internal/loop/daemon.go`:
- Around line 621-640: The browser grant lookup must run for every task (not
only when actor == nil) so each Spawn/Resume call gets the current grant; move
the GrantForTask call (d.browserManager.GrantForTask) out of the actor==nil
guard and always compute browserGrantID/browserID before calling
d.manager.Spawn, passing the values into the Session options (BrowserGrantID,
BrowserID). Also ensure that when no grant exists you explicitly set
BrowserGrantID and BrowserID to "" to clear any prior browser context on a
reused actor.
---
Nitpick comments:
In `@internal/pi/extension/browser-tool.test.mjs`:
- Around line 5-15: Add a negative-test case to assert the no-grant branch: call
buildClaudeCliToolsForTest({}) and assert that none of the returned tools has
name "gsd_browser" (e.g., assert.equal(tools.some(tool => tool.name ===
"gsd_browser"), false)). Update or add an it() in browser-tool.test.mjs
alongside the existing positive test to cover the missing browserGrant path so
regressions that always register gsd_browser fail.
🪄 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 Plus
Run ID: b0c9e851-c55a-426e-a301-ad1e1c8a4225
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.suminternal/pi/extension/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
go.modinternal/browser/manager.gointernal/browser/manager_test.gointernal/browser/service.gointernal/browser/types.gointernal/loop/daemon.gointernal/pi/executor.gointernal/pi/extension/browser-tool.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/package.jsoninternal/relay/conn.gointernal/session/actor.gointernal/session/browser_tool_test.go
Comment on lines
+81
to
+106
| m.mu.Lock() | ||
| state := &sessionState{ | ||
| openRequest: req, | ||
| browserID: result.BrowserID, | ||
| owner: OwnerAgent, | ||
| expiresAt: expiresAt, | ||
| frameCancel: frameCancel, | ||
| } | ||
| m.byID[result.BrowserID] = state | ||
| if msg.TaskID != "" { | ||
| m.byTask[msg.TaskID] = state | ||
| } | ||
| m.mu.Unlock() | ||
| if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{ | ||
| Type: protocol.MsgTypeBrowserSessionOpened, | ||
| RequestID: msg.RequestID, | ||
| BrowserID: result.BrowserID, | ||
| GrantID: msg.GrantID, | ||
| SessionID: msg.SessionID, | ||
| ChannelID: msg.ChannelID, | ||
| URL: result.URL, | ||
| Title: result.Title, | ||
| OpenedAt: time.Now().UTC().Format(time.RFC3339Nano), | ||
| }); err != nil { | ||
| frameCancel() | ||
| return err |
There was a problem hiding this comment.
Roll back manager state if BrowserSessionOpened cannot be sent.
The session is inserted into m.byID/m.byTask before line 94, but the error path only cancels the frame loop. If Send fails, Open returns an error while leaving a live session registered and never closing the underlying browser service. Remove the map entries and best-effort Close the browser before returning.
Suggested fix
m.mu.Unlock()
if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{
Type: protocol.MsgTypeBrowserSessionOpened,
RequestID: msg.RequestID,
BrowserID: result.BrowserID,
GrantID: msg.GrantID,
SessionID: msg.SessionID,
ChannelID: msg.ChannelID,
URL: result.URL,
Title: result.Title,
OpenedAt: time.Now().UTC().Format(time.RFC3339Nano),
}); err != nil {
+ m.mu.Lock()
+ delete(m.byID, result.BrowserID)
+ if msg.TaskID != "" {
+ delete(m.byTask, msg.TaskID)
+ }
+ m.mu.Unlock()
frameCancel()
+ _ = m.service.Close(ctx, result.BrowserID)
return err
}📝 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.
Suggested change
| m.mu.Lock() | |
| state := &sessionState{ | |
| openRequest: req, | |
| browserID: result.BrowserID, | |
| owner: OwnerAgent, | |
| expiresAt: expiresAt, | |
| frameCancel: frameCancel, | |
| } | |
| m.byID[result.BrowserID] = state | |
| if msg.TaskID != "" { | |
| m.byTask[msg.TaskID] = state | |
| } | |
| m.mu.Unlock() | |
| if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{ | |
| Type: protocol.MsgTypeBrowserSessionOpened, | |
| RequestID: msg.RequestID, | |
| BrowserID: result.BrowserID, | |
| GrantID: msg.GrantID, | |
| SessionID: msg.SessionID, | |
| ChannelID: msg.ChannelID, | |
| URL: result.URL, | |
| Title: result.Title, | |
| OpenedAt: time.Now().UTC().Format(time.RFC3339Nano), | |
| }); err != nil { | |
| frameCancel() | |
| return err | |
| m.mu.Lock() | |
| state := &sessionState{ | |
| openRequest: req, | |
| browserID: result.BrowserID, | |
| owner: OwnerAgent, | |
| expiresAt: expiresAt, | |
| frameCancel: frameCancel, | |
| } | |
| m.byID[result.BrowserID] = state | |
| if msg.TaskID != "" { | |
| m.byTask[msg.TaskID] = state | |
| } | |
| m.mu.Unlock() | |
| if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{ | |
| Type: protocol.MsgTypeBrowserSessionOpened, | |
| RequestID: msg.RequestID, | |
| BrowserID: result.BrowserID, | |
| GrantID: msg.GrantID, | |
| SessionID: msg.SessionID, | |
| ChannelID: msg.ChannelID, | |
| URL: result.URL, | |
| Title: result.Title, | |
| OpenedAt: time.Now().UTC().Format(time.RFC3339Nano), | |
| }); err != nil { | |
| m.mu.Lock() | |
| delete(m.byID, result.BrowserID) | |
| if msg.TaskID != "" { | |
| delete(m.byTask, msg.TaskID) | |
| } | |
| m.mu.Unlock() | |
| frameCancel() | |
| _ = m.service.Close(ctx, result.BrowserID) | |
| return err | |
| } |
glittercowboy
force-pushed
the
codex/browser-session-daemon
branch
from
4d87d70 to
f22b8ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/actor.go (1)
487-494:⚠️ Potential issue | 🟠 MajorSnapshot browser context while holding
taskMu.
SetBrowserContextcan modifya.opts.BrowserGrantID/a.opts.BrowserIDundertaskMu, butexecuteTaskreads these fields after releasing the lock (lines 526-527). This creates a race condition where concurrentSetBrowserContextcalls during task execution can cause the wrong browser grant/ID to be attached to the task.Suggested fix
@@ a.taskMu.Lock() a.taskCancel = cancel a.pendingTaskID = "" a.taskID = task.TaskID + browserGrantID := a.opts.BrowserGrantID + browserID := a.opts.BrowserID now := time.Now() a.taskStartedAt = &now a.idleSince = nil a.taskMu.Unlock() @@ CustomInstructions: task.CustomInstructions, - BrowserGrantID: a.opts.BrowserGrantID, - BrowserID: a.opts.BrowserID, + BrowserGrantID: browserGrantID, + BrowserID: browserID, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` around lines 487 - 494, executeTask releases a.taskMu before reading browser fields, allowing SetBrowserContext to race and change a.opts.BrowserGrantID / a.opts.BrowserID; fix by snapshotting the browser context while still holding a.taskMu (e.g., read and store a.opts.BrowserGrantID and a.opts.BrowserID into local variables or into the task object inside the critical section) before unlocking, so when executeTask later uses those values they reflect the state at task start; ensure the snapshot uses the same lock (a.taskMu) and update references in executeTask to use the local snapshot variables instead of reading a.opts directly after unlock.
♻️ Duplicate comments (1)
internal/browser/manager.go (1)
81-107:⚠️ Potential issue | 🟠 MajorRoll back manager state when
BrowserSessionOpenedsend fails.State is inserted before send (Line 89-92), but failure path (Line 105-106) only cancels the frame context. This leaves stale entries in
m.byID/m.byTaskand leaks the underlying browser session.Suggested fix
@@ if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{ @@ }); err != nil { + m.mu.Lock() + delete(m.byID, result.BrowserID) + if msg.TaskID != "" { + delete(m.byTask, msg.TaskID) + } + m.mu.Unlock() frameCancel() + _ = m.service.Close(ctx, result.BrowserID) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager.go` around lines 81 - 107, The code stores the new sessionState into m.byID and m.byTask before sending the BrowserSessionOpened message but never removes those entries if m.sender.Send fails, leaking state and the browser session; update the error path after calling m.sender.Send so that on error you acquire m.mu.Lock(), delete m.byID[result.BrowserID] and if msg.TaskID != "" delete m.byTask[msg.TaskID], m.mu.Unlock(), then call frameCancel() and return the error (keep using the existing sessionState, frameCancel, and m.sender.Send symbols to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager.go`:
- Around line 160-181: The frame loop currently calls m.sender.Send with the
unbounded ctx from frameLoop which can block and stall the loop; wrap each
m.sender.Send call (the BrowserFrame and BrowserNavigation sends) with a short
cancellable context using context.WithTimeout (e.g., 500ms–1s), defer cancel()
and use that child context for the Send, and also propagate/check the returned
error instead of assigning to blank identifier so blocked sends are bounded and
errors are logged/handled; update the calls around m.sender.Send in frameLoop to
use the new timeout context and handle the error value.
---
Outside diff comments:
In `@internal/session/actor.go`:
- Around line 487-494: executeTask releases a.taskMu before reading browser
fields, allowing SetBrowserContext to race and change a.opts.BrowserGrantID /
a.opts.BrowserID; fix by snapshotting the browser context while still holding
a.taskMu (e.g., read and store a.opts.BrowserGrantID and a.opts.BrowserID into
local variables or into the task object inside the critical section) before
unlocking, so when executeTask later uses those values they reflect the state at
task start; ensure the snapshot uses the same lock (a.taskMu) and update
references in executeTask to use the local snapshot variables instead of reading
a.opts directly after unlock.
---
Duplicate comments:
In `@internal/browser/manager.go`:
- Around line 81-107: The code stores the new sessionState into m.byID and
m.byTask before sending the BrowserSessionOpened message but never removes those
entries if m.sender.Send fails, leaking state and the browser session; update
the error path after calling m.sender.Send so that on error you acquire
m.mu.Lock(), delete m.byID[result.BrowserID] and if msg.TaskID != "" delete
m.byTask[msg.TaskID], m.mu.Unlock(), then call frameCancel() and return the
error (keep using the existing sessionState, frameCancel, and m.sender.Send
symbols to locate the changes).
🪄 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 Plus
Run ID: 79a7fcc1-50f1-466c-a6b8-1e168067c723
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.suminternal/pi/extension/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
go.modinternal/browser/manager.gointernal/browser/manager_test.gointernal/browser/service.gointernal/browser/types.gointernal/loop/daemon.gointernal/pi/executor.gointernal/pi/extension/browser-tool.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/package.jsoninternal/relay/conn.gointernal/session/actor.gointernal/session/browser_tool_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/pi/extension/package.json
- go.mod
- internal/browser/types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/session/browser_tool_test.go
- internal/pi/extension/browser-tool.test.mjs
- internal/browser/manager_test.go
- internal/relay/conn.go
- internal/pi/executor.go
- internal/loop/daemon.go
- internal/pi/extension/index.ts
Comment on lines
+160
to
+181
| _ = m.sender.Send(ctx, &protocol.BrowserFrame{ | ||
| Type: protocol.MsgTypeBrowserFrame, | ||
| BrowserID: browserID, | ||
| SessionID: req.SessionID, | ||
| ChannelID: req.ChannelID, | ||
| Seq: frame.Sequence, | ||
| ContentType: frame.ContentType, | ||
| DataBase64: frame.DataBase64, | ||
| Width: frame.Width, | ||
| Height: frame.Height, | ||
| CapturedAt: frame.CapturedAt, | ||
| }) | ||
| if frame.URL != "" { | ||
| _ = m.sender.Send(ctx, &protocol.BrowserNavigation{ | ||
| Type: protocol.MsgTypeBrowserNavigation, | ||
| BrowserID: browserID, | ||
| SessionID: req.SessionID, | ||
| ChannelID: req.ChannelID, | ||
| URL: frame.URL, | ||
| Title: frame.Title, | ||
| EndedAt: time.Now().UTC().Format(time.RFC3339Nano), | ||
| }) |
There was a problem hiding this comment.
Bound frame event sends with a timeout.
m.sender.Send uses ctx from frameLoop (no deadline). If sender blocks, one call can stall the frame loop indefinitely.
Suggested fix
@@
- _ = m.sender.Send(ctx, &protocol.BrowserFrame{
+ sendCtx, sendCancel := context.WithTimeout(ctx, 5*time.Second)
+ _ = m.sender.Send(sendCtx, &protocol.BrowserFrame{
@@
- })
+ })
+ sendCancel()
if frame.URL != "" {
- _ = m.sender.Send(ctx, &protocol.BrowserNavigation{
+ navCtx, navCancel := context.WithTimeout(ctx, 5*time.Second)
+ _ = m.sender.Send(navCtx, &protocol.BrowserNavigation{
@@
- })
+ })
+ navCancel()
}📝 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.
Suggested change
| _ = m.sender.Send(ctx, &protocol.BrowserFrame{ | |
| Type: protocol.MsgTypeBrowserFrame, | |
| BrowserID: browserID, | |
| SessionID: req.SessionID, | |
| ChannelID: req.ChannelID, | |
| Seq: frame.Sequence, | |
| ContentType: frame.ContentType, | |
| DataBase64: frame.DataBase64, | |
| Width: frame.Width, | |
| Height: frame.Height, | |
| CapturedAt: frame.CapturedAt, | |
| }) | |
| if frame.URL != "" { | |
| _ = m.sender.Send(ctx, &protocol.BrowserNavigation{ | |
| Type: protocol.MsgTypeBrowserNavigation, | |
| BrowserID: browserID, | |
| SessionID: req.SessionID, | |
| ChannelID: req.ChannelID, | |
| URL: frame.URL, | |
| Title: frame.Title, | |
| EndedAt: time.Now().UTC().Format(time.RFC3339Nano), | |
| }) | |
| sendCtx, sendCancel := context.WithTimeout(ctx, 5*time.Second) | |
| _ = m.sender.Send(sendCtx, &protocol.BrowserFrame{ | |
| Type: protocol.MsgTypeBrowserFrame, | |
| BrowserID: browserID, | |
| SessionID: req.SessionID, | |
| ChannelID: req.ChannelID, | |
| Seq: frame.Sequence, | |
| ContentType: frame.ContentType, | |
| DataBase64: frame.DataBase64, | |
| Width: frame.Width, | |
| Height: frame.Height, | |
| CapturedAt: frame.CapturedAt, | |
| }) | |
| sendCancel() | |
| if frame.URL != "" { | |
| navCtx, navCancel := context.WithTimeout(ctx, 5*time.Second) | |
| _ = m.sender.Send(navCtx, &protocol.BrowserNavigation{ | |
| Type: protocol.MsgTypeBrowserNavigation, | |
| BrowserID: browserID, | |
| SessionID: req.SessionID, | |
| ChannelID: req.ChannelID, | |
| URL: frame.URL, | |
| Title: frame.Title, | |
| EndedAt: time.Now().UTC().Format(time.RFC3339Nano), | |
| }) | |
| navCancel() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 160 - 181, The frame loop currently
calls m.sender.Send with the unbounded ctx from frameLoop which can block and
stall the loop; wrap each m.sender.Send call (the BrowserFrame and
BrowserNavigation sends) with a short cancellable context using
context.WithTimeout (e.g., 500ms–1s), defer cancel() and use that child context
for the Send, and also propagate/check the returned error instead of assigning
to blank identifier so blocked sends are bounded and errors are logged/handled;
update the calls around m.sender.Send in frameLoop to use the new timeout
context and handle the error value.
glittercowboy
force-pushed
the
codex/browser-session-daemon
branch
from
40c0e2a to
bf55277
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/actor.go (1)
487-528:⚠️ Potential issue | 🟠 MajorSnapshot the browser context before releasing
taskMu.
SetBrowserContextupdatesa.opts.BrowserGrantID/a.opts.BrowserIDundertaskMu, but Lines 526-527 read them after the lock was dropped. That leavesexecuteTaskracy and can attach the wrong browser grant to a task while it is starting.Proposed fix
a.taskMu.Lock() a.taskCancel = cancel a.pendingTaskID = "" a.taskID = task.TaskID now := time.Now() a.taskStartedAt = &now a.idleSince = nil + browserGrantID := a.opts.BrowserGrantID + browserID := a.opts.BrowserID a.taskMu.Unlock() @@ tc := &taskContext{ TaskID: task.TaskID, ChannelID: task.ChannelID, ActorContext: ctx, @@ - BrowserGrantID: a.opts.BrowserGrantID, - BrowserID: a.opts.BrowserID, + BrowserGrantID: browserGrantID, + BrowserID: browserID, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` around lines 487 - 528, The code reads a.opts.BrowserGrantID and a.opts.BrowserID into the taskContext (tc) after releasing a.taskMu, which creates a race if SetBrowserContext is called under the lock; fix by snapshotting the browser grant and browser id into local variables while holding a.taskMu (use the same critical section where a.taskCancel, a.taskID, a.taskStartedAt and idle fields are set) and then populate tc.BrowserGrantID and tc.BrowserID from those local variables before releasing the lock so executeTask uses the consistent browser context.
♻️ Duplicate comments (2)
internal/browser/manager.go (2)
154-181:⚠️ Potential issue | 🟠 MajorBound the frame and navigation sends.
frameLoopruns on a background context, so Lines 160 and 173 can block indefinitely insidem.sender.Send. One stuck relay send will freeze frame streaming and stop later expiry checks from running on that loop.Proposed fix
- _ = m.sender.Send(ctx, &protocol.BrowserFrame{ + sendCtx, sendCancel := context.WithTimeout(ctx, 5*time.Second) + err = m.sender.Send(sendCtx, &protocol.BrowserFrame{ Type: protocol.MsgTypeBrowserFrame, BrowserID: browserID, SessionID: req.SessionID, @@ Height: frame.Height, CapturedAt: frame.CapturedAt, }) + sendCancel() + if err != nil { + return + } if frame.URL != "" { - _ = m.sender.Send(ctx, &protocol.BrowserNavigation{ + navCtx, navCancel := context.WithTimeout(ctx, 5*time.Second) + _ = m.sender.Send(navCtx, &protocol.BrowserNavigation{ Type: protocol.MsgTypeBrowserNavigation, BrowserID: browserID, SessionID: req.SessionID, @@ Title: frame.Title, EndedAt: time.Now().UTC().Format(time.RFC3339Nano), }) + navCancel() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager.go` around lines 154 - 181, The frameLoop uses the background ctx and currently calls m.sender.Send(...) for protocol.BrowserFrame and protocol.BrowserNavigation which can block indefinitely; wrap each send in a short bounded context (e.g., context.WithTimeout with a small timeout like 1–2s) so a stuck relay won't stall the loop, then cancel the timeout context and ignore/send error handling as appropriate; update the calls around m.sender.Send in the frameLoop (the BrowserFrame and BrowserNavigation sends) to use this timeout context when invoking m.sender.Send.
81-106:⚠️ Potential issue | 🟠 MajorRoll back the opened session if
BrowserSessionOpenedfails.By Line 93 the session is already registered in
m.byID/m.byTask. If Line 94 fails,Openreturns an error but leaves the state behind and the browser process alive;frameCancel()only stops future frame sends.Proposed fix
m.mu.Unlock() if err := m.sender.Send(ctx, &protocol.BrowserSessionOpened{ Type: protocol.MsgTypeBrowserSessionOpened, RequestID: msg.RequestID, @@ Title: result.Title, OpenedAt: time.Now().UTC().Format(time.RFC3339Nano), }); err != nil { + m.mu.Lock() + m.removeStateLocked(state) + m.mu.Unlock() frameCancel() + closeCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _ = m.service.Close(closeCtx, result.BrowserID) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager.go` around lines 81 - 106, The session is inserted into m.byID / m.byTask before sending the BrowserSessionOpened message, so if m.sender.Send(...) fails the registered state and live browser remain; modify Open to undo registration and cancel the session when sender.Send returns an error: after detecting the send error call frameCancel(), remove the session from m.byID and, if msg.TaskID != "" remove from m.byTask, and clear any other state (e.g., ensure no lingering references to sessionState) before returning the error so the browser/process and in-memory maps are rolled back; look for the insertion sites using sessionState, m.byID, m.byTask and the send call to implement the rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager_test.go`:
- Around line 11-54: The test doubles fakeService and recordingSender are not
concurrency-safe as Manager.Open starts frameLoop which mutates
fakeService.calls and recordingSender.msgs from background goroutines; make them
safe by adding a sync.Mutex (or RWMutex) to fakeService and recordingSender,
lock around all mutations in methods Open/Close/Frame/Tool/UserInput and Send,
and add thread-safe snapshot accessor methods (e.g., fakeService.Calls() and
recordingSender.Msgs()) that return a copy under the lock so tests can read
stable snapshots when asserting after Manager.Open/frameLoop.
---
Outside diff comments:
In `@internal/session/actor.go`:
- Around line 487-528: The code reads a.opts.BrowserGrantID and a.opts.BrowserID
into the taskContext (tc) after releasing a.taskMu, which creates a race if
SetBrowserContext is called under the lock; fix by snapshotting the browser
grant and browser id into local variables while holding a.taskMu (use the same
critical section where a.taskCancel, a.taskID, a.taskStartedAt and idle fields
are set) and then populate tc.BrowserGrantID and tc.BrowserID from those local
variables before releasing the lock so executeTask uses the consistent browser
context.
---
Duplicate comments:
In `@internal/browser/manager.go`:
- Around line 154-181: The frameLoop uses the background ctx and currently calls
m.sender.Send(...) for protocol.BrowserFrame and protocol.BrowserNavigation
which can block indefinitely; wrap each send in a short bounded context (e.g.,
context.WithTimeout with a small timeout like 1–2s) so a stuck relay won't stall
the loop, then cancel the timeout context and ignore/send error handling as
appropriate; update the calls around m.sender.Send in the frameLoop (the
BrowserFrame and BrowserNavigation sends) to use this timeout context when
invoking m.sender.Send.
- Around line 81-106: The session is inserted into m.byID / m.byTask before
sending the BrowserSessionOpened message, so if m.sender.Send(...) fails the
registered state and live browser remain; modify Open to undo registration and
cancel the session when sender.Send returns an error: after detecting the send
error call frameCancel(), remove the session from m.byID and, if msg.TaskID !=
"" remove from m.byTask, and clear any other state (e.g., ensure no lingering
references to sessionState) before returning the error so the browser/process
and in-memory maps are rolled back; look for the insertion sites using
sessionState, m.byID, m.byTask and the send call to implement the rollback.
🪄 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 Plus
Run ID: 657e93fc-7624-4450-903b-a7893c8ad234
⛔ Files ignored due to path filters (1)
internal/pi/extension/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
internal/browser/manager.gointernal/browser/manager_test.gointernal/browser/service.gointernal/browser/types.gointernal/loop/daemon.gointernal/pi/executor.gointernal/pi/extension/browser-tool.test.mjsinternal/pi/extension/index.tsinternal/pi/extension/package.jsoninternal/relay/conn.gointernal/session/actor.gointernal/session/browser_tool_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/pi/extension/package.json
- internal/pi/extension/browser-tool.test.mjs
- internal/browser/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/pi/extension/index.ts
- internal/loop/daemon.go
Comment on lines
+11
to
+54
| type fakeService struct { | ||
| calls []string | ||
| } | ||
|
|
||
| func (f *fakeService) Open(ctx context.Context, req OpenRequest) (OpenResult, error) { | ||
| f.calls = append(f.calls, "open:"+req.GrantID) | ||
| return OpenResult{BrowserID: "browser_1", URL: "about:blank", Title: "Blank"}, nil | ||
| } | ||
|
|
||
| func (f *fakeService) Close(ctx context.Context, browserID string) error { | ||
| f.calls = append(f.calls, "close:"+browserID) | ||
| return nil | ||
| } | ||
|
|
||
| func (f *fakeService) Frame(ctx context.Context, browserID string) (Frame, error) { | ||
| f.calls = append(f.calls, "frame:"+browserID) | ||
| return Frame{ | ||
| Sequence: 1, | ||
| ContentType: "image/jpeg", | ||
| DataBase64: "aGVsbG8=", | ||
| Width: 1280, | ||
| Height: 720, | ||
| CapturedAt: time.Now().UTC().Format(time.RFC3339Nano), | ||
| }, nil | ||
| } | ||
|
|
||
| func (f *fakeService) Tool(ctx context.Context, browserID string, method string, params []byte) (ToolResult, error) { | ||
| f.calls = append(f.calls, "tool:"+method) | ||
| return ToolResult{OK: true, ResultJSON: []byte(`{"ok":true}`)}, nil | ||
| } | ||
|
|
||
| func (f *fakeService) UserInput(ctx context.Context, browserID string, input *protocol.BrowserUserInput) error { | ||
| f.calls = append(f.calls, "input:"+input.Kind) | ||
| return nil | ||
| } | ||
|
|
||
| type recordingSender struct { | ||
| msgs []any | ||
| } | ||
|
|
||
| func (r *recordingSender) Send(ctx context.Context, msg any) error { | ||
| r.msgs = append(r.msgs, msg) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Make the test doubles concurrency-safe.
Manager.Open starts frameLoop immediately, so fakeService.calls and recordingSender.msgs are mutated from background code while the tests inspect them on the main goroutine. The raw slices here will race under go test -race and can make the assertions flaky.
Proposed fix
type fakeService struct {
+ mu sync.Mutex
calls []string
}
func (f *fakeService) Open(ctx context.Context, req OpenRequest) (OpenResult, error) {
+ f.mu.Lock()
f.calls = append(f.calls, "open:"+req.GrantID)
+ f.mu.Unlock()
return OpenResult{BrowserID: "browser_1", URL: "about:blank", Title: "Blank"}, nil
}
@@
type recordingSender struct {
+ mu sync.Mutex
msgs []any
}
func (r *recordingSender) Send(ctx context.Context, msg any) error {
+ r.mu.Lock()
r.msgs = append(r.msgs, msg)
+ r.mu.Unlock()
return nil
}You’ll also want to read these slices through a locked snapshot helper in the assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager_test.go` around lines 11 - 54, The test doubles
fakeService and recordingSender are not concurrency-safe as Manager.Open starts
frameLoop which mutates fakeService.calls and recordingSender.msgs from
background goroutines; make them safe by adding a sync.Mutex (or RWMutex) to
fakeService and recordingSender, lock around all mutations in methods
Open/Close/Frame/Tool/UserInput and Send, and add thread-safe snapshot accessor
methods (e.g., fakeService.Calls() and recordingSender.Msgs()) that return a
copy under the lock so tests can read stable snapshots when asserting after
Manager.Open/frameLoop.
This was referenced Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gsd_browserPi extension tool for browser-granted tasks while keeping the claude-cli provider path.Verification
npm testininternal/pi/extensiongo test ./...go build -o gsd-cloud . && rm -f gsd-cloudDependencies and merge order
gsd-build/protocol-gov0.24.0.gsd-browsershared browser service PR and release.Summary by CodeRabbit