🤖 feat: show other agent-browser sessions#3243
Conversation
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Well-designed feature. The allowOtherWorkspaceSession flag travels the full chain without dropping or coercing at any boundary: frontend click, ORPC input, ensureSessionAttachable, token mint, token validate, bridge revalidation, executeControl, getUrl. The negative default (=== true comparison at each boundary) means the strict path is the safe failure mode. The grouped-discovery approach keeps the contract change additive and the backend tests are thorough.
Severity count: 1 P2, 14 P3, 7 Nit.
Two findings converge across 5+ reviewers each: (1) chooseSelectedSession retains a stale persisted session name when sessions = [], and the new picker visibility condition now exposes that stale name as the button label while the viewport says "Choose a browser session"; (2) the hasOtherSessions branch in BrowserViewerState fires when the user has already selected an other session (the pre-first-frame window), showing "Choose a browser session" to someone who just chose one.
The test string mismatch in BrowserTab.test.tsx:148 ("Other running sessions" vs the actual label "Other sessions") means the assertion contributes zero behavioral constraint and passes whether the component is broken or working.
Process observations: the PR description claims dogfooding "with screenshots" but no screenshots are attached. The implementation plan explicitly required screenshot artifacts as deliverables. The setSystemTime-to-Date.now refactor in BrowserBridgeTokenManager.test.ts and the blank line deletion in BrowserBridgeTokenManager.ts are unrelated to the feature.
"The user sees 'alpha' in the picker button but 'Choose a browser session' in the preview area." (Pariston)
src/browser/features/RightSidebar/BrowserTab/BrowserTab.tsx:100
P3 [DEREM-4] chooseSelectedSession returns currentSessionName unchanged when sessions.length === 0. Pre-PR this was harmless because the picker was hidden when no current sessions existed. Post-PR the picker renders whenever discoveredSessions.length + otherDiscoveredSessions.length > 0 (line 279), so when a workspace has no current sessions but has other sessions, the picker button shows a stale persisted name while the viewport shows "Choose a browser session."
The plan states: "if only otherSessions exist, leave the Browser preview unselected." The stale name violates that intent. No unauthorized connection occurs (selectedDiscoveredSession is null), but the UI falsely implies a session is selected.
"The two signals contradict each other: the user may wait for their session to come back instead of explicitly attaching to the available other session." (Hisoka P3, Nami P3, Pariston P3, Razor P3, Kite Note)
Return null in this branch instead of currentSessionName. The "preserve during transient failures" goal is served by the catch path, which does not call setSelectedCurrentSessionName on error.
🤖
src/node/services/browser/AgentBrowserSessionDiscoveryService.ts:617
P3 [DEREM-9] "No sessions discovered" fires when getLookupSessions returns empty, but that happens whenever sessionGroups.sessions is empty regardless of whether sessionGroups.otherSessions is populated. The error message says "no sessions discovered for workspace X" when sessions were discovered but classified into otherSessions.
"A developer who receives this error while
agent-browseris visibly running will check socket files, re-read discovery logic, and find no explanation in the message." (Chopper)
Check sessionGroups.sessions.length + sessionGroups.otherSessions.length === 0 before this message; fall through to the generic "not found" message when total discovery is non-zero.
🤖
src/node/orpc/router.ts:1238
P3 [DEREM-10] Post-control getUrl calls at lines 1240 and 1262 use skipSessionValidation: true but omit allowOtherWorkspaceSession without explaining why the omission is safe. Currently harmless because skipSessionValidation bypasses getSessionConnection entirely. If skipSessionValidation is ever removed, these calls silently start rejecting other-workspace session URL fetches: control commands succeed but page-state updates break.
"'Skip validation' and 'validation not needed' are different reasons." (Hisoka P2, Knov Nit)
Add a comment: // Session already validated by executeControl above; skip redundant lookup.
🤖
src/browser/features/RightSidebar/BrowserTab/BrowserTab.tsx:500-505
P3 [DEREM-12] Error description says "while a discovered browser session is available for this project." When the failing session is explicitly from a different project, the description is backwards.
"The session is not from this project. The user literally just picked it from under 'Other sessions.'" (Luffy)
Consider a scope-neutral message like "Mux will keep retrying while the selected browser session is reachable."
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
17 of 22 findings from R1 addressed (nice work on the BrowserSelectedSession union, the extracted chooseExplicitOtherSession helper, and the bridge success-path test). 1 acknowledged. 4 findings are silent, no author response and no code change. These were posted in the R1 review body (folded from inline comments because their lines were outside the diff hunks), so they may have been missed.
Further panel review is blocked until the author responds to or pushes fixes for the following:
DEREM-4 (P3, 5 reviewers) BrowserTab.tsx:104: chooseSelectedSession still returns currentSessionName when sessions.length === 0. The picker now renders when only other sessions exist, so a stale persisted name appears in the picker button while the viewport says "Choose a browser session." Return null in this branch.
DEREM-9 (P3) AgentBrowserSessionDiscoveryService.ts:616: ensureSessionAttachable still checks sessions.length === 0 where sessions is the current-workspace-only slice. The error "no sessions discovered for workspace X" fires when otherSessions is non-empty. Check total discovery count before this message.
DEREM-10 (P3) router.ts:1243,1265: Post-control getUrl calls still use skipSessionValidation: true without allowOtherWorkspaceSession and no comment explaining why the omission is safe. Add the explanatory comment.
DEREM-12 (P3) BrowserTab.tsx:517: Error description still says "for this project" when the failing session may be an explicitly selected other-workspace session. Use a scope-neutral message.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Addressed the four remaining items from coder-agents-review review 4237216279:
Validation passed locally and the integration CI rerun is now green. I also fetched review threads again; there are no unresolved review threads. |
|
/coder-agents-review |
There was a problem hiding this comment.
All 22 R1 findings resolved. The 4 previously-silent findings (DEREM-4, -9, -10, -12) are correctly fixed in 723a5c0. Panel verified each fix against the root cause.
The allowOtherWorkspaceSession flag was traced end-to-end through 10 layers with no breaks. The bridge server revalidates using the flag stored in the token record rather than a fresh client-supplied value, so the scope boundary is enforced server-side. Test coverage is thorough: both positive and negative paths are exercised at each layer, and the primary coexistence scenario (current + other sessions visible together) is now covered.
1 P3 (re-raised), 1 Nit.
"I tried to build a case against this and could not." (Pariston)
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
2 findings from R3 remain unaddressed. No author response and no new commits since R3 (head SHA unchanged at 76cbefb). Further review is blocked until the author responds or pushes fixes.
DEREM-1 (P3, re-raised R3) BrowserTab.test.tsx:149: queryByText("Other sessions") runs before fireEvent.click opens the picker. The label is inside {isOpen && (, so the assertion returns null regardless. Move it after the click.
DEREM-23 (Nit, 3 reviewers) BrowserTab.tsx:526: "Select an other session" is ungrammatical. Use "another."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
There was a problem hiding this comment.
All 23 prior findings resolved. R5 panel (Bisky, Mafuuu, Pariston, Meruem) verified every fix. Mafuuu traced the allowOtherWorkspaceSession flag end-to-end through 10 layers with no breaks. Pariston tried to build a case against this change and could not. 79 tests pass.
2 new P3 test gaps found by Bisky. No production code issues.
"The flag threads correctly through every layer I traced." (Pariston)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
All 25 findings resolved. R6 panel (Bisky, Mafuuu, Pariston, Chopper) found no new issues. Netero R6 clean across all 8 checks. Both R5 fixes verified.
This PR is ready for human review and merge. 25 findings raised across 6 rounds, all addressed. The allowOtherWorkspaceSession flag propagation, test coverage, error messaging, and UI state machine have been verified by multiple panel configurations.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
All 25 findings resolved. Second consecutive clean panel round (R6 and R7). Netero R7 clean across all 8 checks. CI green. 81 tests pass.
No open findings. This PR is ready for human review and merge.
🤖 This review was automatically generated with Coder Agents.
Summary
Show agent-browser sessions from other working directories in the Browser tab session picker without auto-attaching to them. Current-workspace sessions remain the only auto-selected sessions; selecting an other session explicitly carries an allow flag through bootstrap, bridge validation, and browser controls.
Background
The Browser tab previously hid live agent-browser sessions whose process CWD was outside the current workspace, making it hard to discover and intentionally attach to a known session started elsewhere.
Implementation
Discovery now returns current-workspace sessions and other live sessions as separate groups. The renderer keeps other sessions in the existing picker with their CWD as secondary text, while backend attach/control paths remain workspace-scoped unless the explicit
allowOtherWorkspaceSessionflag is present.Validation
bun test src/node/services/browser/AgentBrowserSessionDiscoveryService.test.ts src/node/services/browser/BrowserBridgeTokenManager.test.ts src/node/services/browser/BrowserBridgeServer.test.ts src/node/services/browser/BrowserControlService.test.ts src/browser/features/RightSidebar/BrowserTab/BrowserTab.test.tsx src/browser/features/RightSidebar/BrowserTab/useBrowserBridgeConnection.test.tsx src/browser/features/RightSidebar/BrowserTab/BrowserToolbar.test.tsxmake typecheckmake static-checkRisks
Moderate Browser-tab risk: the renderer selection state, bridge token scope, and control APIs changed together. The default backend paths remain strict and targeted tests cover strict-vs-explicit behavior.
📋 Implementation Plan
Plan: Show other
agent-browsersessions in the Browser tabGoal
Make the Browser tab less confusing when
agent-browsersessions are running outside the current workspace by showing them in a clearly separated secondary area, while preserving the existing safe default: current-workspace sessions remain primary, auto-selected, and the only sessions used unless the user explicitly chooses otherwise.Evidence from repo investigation
src/node/services/browser/AgentBrowserSessionDiscoveryService.tscurrently discovers global agent-browser sessions, reads each session PID/CWD, and skips sessions whose process CWD is not inside the current workspace candidate paths indiscoverSessions().src/node/services/serviceContainer.tsasworkspaceMetadata.projectPathplusresolveWorkspaceExecutionPath(...).src/node/orpc/router.tsexposesbrowser.listSessions, but currently maps each discovered session down to only{ sessionName, status }, dropping backendcwdmetadata.src/browser/features/RightSidebar/BrowserTab/BrowserTab.tsxpollsapi.browser.listSessions({ workspaceId }), stores onediscoveredSessionsarray, and auto-selects from that list.getBootstrap:BrowserBridgeServerrevalidates the minted token by callinggetSessionConnection(...), so cross-workspace attach would bootstrap and then fail unless the allow/scope flag reaches the bridge validation path.getSessionConnection(...); if we want an explicitly attached other session to remain usable, those calls need the same explicit scope flag.Recommended approach
Hybrid grouped discovery: keep the existing
sessionscontract as “current workspace sessions only”, addotherSessionsto the samebrowser.listSessionsresponse, and require an explicitallowOtherWorkspaceSessionflag for attach/control paths.Net product LoC estimate: +180 to +260 LoC, excluding tests.
Why this approach:
sessionssemantics remain safe and easy to reason about.Rejected alternatives
sourceon every session (+140 to +210 product LoC): simpler schema, but easier to accidentally auto-select/control the wrong session and weakens the existingsessionsmeaning.listOtherSessionsendpoint (+190 to +280 product LoC): clear API separation, but creates duplicate polling and repeated PID/CWD resolution with little benefit for this local UI.Implementation phases
Phase 1 — Backend discovery groups
Files:
src/node/services/browser/AgentBrowserSessionDiscoveryService.tssrc/node/services/browser/AgentBrowserSessionDiscoveryService.test.tsPlan:
Introduce an internal grouped result, e.g.
Refactor the current
discoverSessions(workspaceId)loop so the CWD mismatch branch pushes tootherSessionsinstead of immediatelycontinue-ing.Keep
listSessions(workspaceId)returning only current-workspacesessionsso existing internal callers/tests retain strict behavior.Add a new public method such as
listSessionGroups(workspaceId)for the router/UI discovery response.Add lookup options to strict attach paths:
Then update
getSessionConnection(...)andensureSessionAttachable(...)so they searchotherSessionsonly when that option is true.Preserve existing defensive behavior:
missing_streamwhen the session is otherwise live;Quality gate after Phase 1:
Add/update service tests for:
listSessions()still excludes unmatched sessions by default;listSessionGroups()returns current and other sessions in deterministic order;missing_streamsessions are included inotherSessions;ensureSessionAttachable(..., { allowOtherWorkspaceSession: true })can enable streaming for an other session;Run:
make test -- src/node/services/browser/AgentBrowserSessionDiscoveryService.test.tsPhase 2 — ORPC schema, router, bridge token, and control scope
Files:
src/common/orpc/schemas/api.tssrc/node/orpc/router.tssrc/node/services/browser/BrowserBridgeTokenManager.tssrc/node/services/browser/BrowserBridgeServer.tssrc/node/services/browser/BrowserControlService.tsPlan:
Extend
browser.listSessions.outputto preserve the existing current-workspace field and add secondary sessions:where
BrowserDiscoveredOtherSessionincludes at least:Do not expose
pidorstreamPort.Update the router’s
browser.listSessionshandler to calllistSessionGroups(workspaceId)and map:sessionswithout CWD;otherSessionswith CWD.Add
allowOtherWorkspaceSession: z.boolean().nullish()to browser input schemas that can act on the selected session, at minimum:browser.getBootstrap.input;browser.control.inputif BrowserTab controls use it after attach;browser.getUrl.inputif BrowserTab reads URL for the attached session.Pass
allowOtherWorkspaceSession === trueintoensureSessionAttachable(...)fromgetBootstrap.Store the allow flag in
BrowserBridgeTokenManagertoken records when minting bridge tokens, and return it from validation.In
BrowserBridgeServer, pass the token’s allow flag intogetSessionConnection(...)during WebSocket upgrade revalidation.In
BrowserControlService, thread the allow flag throughgetSessionConnection(...)for explicit other-session control/get-url calls. Default remains strict when the flag is absent.Quality gate after Phase 2:
Add/update tests for:
browser.listSessionsresponse includes bothsessionsandotherSessions;getBootstrappasses the allow flag intoensureSessionAttachable;BrowserBridgeServersucceeds for an other-session token only when the token allows it;Run targeted backend tests, for example:
Phase 3 — BrowserTab state and UI
Files:
src/browser/features/RightSidebar/BrowserTab/BrowserTab.tsxsrc/browser/features/RightSidebar/BrowserTab/useBrowserBridgeConnection.tssrc/browser/features/RightSidebar/BrowserTab/browserBridgeTypes.tssrc/browser/features/RightSidebar/BrowserTab/BrowserTab.test.tsxuseBrowserBridgeConnectionPlan:
Extend frontend types to represent:
sessions;otherSessions, includingcwd.Replace plain
selectedSessionNamewhere necessary with a small selection object so the UI knows whether the selected session requires the explicit allow flag:Update selection logic:
otherSessionsexist, leave the Browser preview unselected and show the secondary section.Update
useBrowserBridgeConnection.connect(...)to accept/passallowOtherWorkspaceSessiontoapi.browser.getBootstrap(...).Pass the same allow flag into browser control/get-url API calls for the selected other session.
Render the UI as two visually distinct areas:
For each other session, show:
Keep the UI minimal: no modal confirmation initially; the secondary section plus “Attach anyway” button is the explicit confirmation.
Quality gate after Phase 3:
Add/update UI tests for:
allowOtherWorkspaceSession: true;Run:
make test -- src/browser/features/RightSidebar/BrowserTab/BrowserTab.test.tsx make typecheckPhase 4 — Full validation and cleanup
Plan:
Run targeted tests from Phases 1–3 again after integration.
Run broader repo validation:
Review the diff for minimality:
as any;.nullish();Acceptance criteria
attachableandmissing_streamother sessions, including stream enablement, bridge token validation, preview streaming, and Browser tab controls.Dogfooding plan
Setup
Start the isolated development server sandbox:
Load the current agent-browser/electron automation guidance before driving the UI:
Open two Mux workspaces for different project/worktree paths.
Start at least two named
agent-browsersessions from different CWDs:Manual scenarios to verify
Current + other sessions visible
No accidental attach
Explicit attach succeeds
Strict default remains safe
allowOtherWorkspaceSession.Dogfooding artifacts
Rollout notes and risks
otherSessionsand do not expose PID/port.sessionNamealone.Generated with
mux• Model:openai:gpt-5.5• Thinking:high• Cost:$61.43