Browser Control M1 (2/3): Electron main — CDP bridge + command poller#416
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c718ef9bd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if (await probe(base)) return { ok: true }; | ||
|
|
||
| // (2) Resolve + spawn our managed Chrome (once). | ||
| if (!managedChrome) { |
There was a problem hiding this comment.
When the Chrome instance that Cowork spawned is later killed or crashes, the port probe will fail but managedChrome is still non-null because it is only cleared in disposeAllBridges(). This branch therefore skips spawning a replacement and just waits until the 10s timeout, so the reconnect/list-tabs flow stays broken until the whole app restarts. Please clear the handle on child exit or retry the spawn when the existing handle no longer has a live debug port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 439f980 — the spawner clears managedChrome on the child's exit event (identity-guarded so a newer handle is never clobbered), and ensureManagedChrome additionally disposes a stale handle and respawns when the debug port is dead past a 15s startup window. A killed/crashed managed Chrome is now replaced on the next connect instead of blocking until app restart. Tests: respawn-after-death and no-kill-during-slow-startup.
| if (landedHost && landedHost !== target.domain) { | ||
| return { |
There was a problem hiding this comment.
When this landed-host check is reached, the approved same-site URL has redirected to a different registrable host but the bridge remains in connected state with the old approvedTarget. That leaves the tab approved for the old domain while it is actually on the new domain, so a later command can run against the off-domain page instead of requiring reapproval; call handleLost or otherwise detach before returning this navigation_failed result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 439f980 — when the post-navigation read-back lands on a different registrable host, the bridge now calls handleLost (closes the CDP client, drops to lost, requires re-approval) before returning navigation_failed, so no later command can run against the off-domain page under the old grant. Test: redirect → navigation_failed + state lost + subsequent inspect refused with bridge_disconnected.
c718ef9 to
439f980
Compare
b679496 to
a78b11c
Compare
4b5f96a to
2f88a73
Compare
…ommand poller Main-process half of browser control: - cdp-client.ts: minimal Chrome DevTools Protocol WebSocket client. - chrome-discovery.ts: locate a locally running Chrome debug endpoint. - browser-bridge.ts: tab attach/approve lifecycle, domain-scoped grant and link gate, Stop/Take-over state, IPC handlers. - browser-command-poller.ts: long-polls cowork-server /api/v1/browse (hello/session, commands/next, bridge-state, command result posting), with stale-session invalidation when the active conversation changes. - index.ts/preload.ts: wire bridge IPC + expose browserControl* to the renderer via the existing preload surface.
Two codex review fixes in the main-process bridge: 1. Managed-Chrome respawn (P2): if the Chrome that Cowork spawned was later killed or crashed, managedChrome stayed non-null (only cleared in disposeAllBridges), so ensureManagedChrome skipped respawning and the connect/list-tabs flow just waited out the 10s port poll until app restart. Now the default spawner clears the handle on the child 'exit' event, and ensureManagedChrome additionally disposes a stale handle and respawns when the debug port is dead past a 15s startup window (covers hung processes / spawners without exit reporting). A just-spawned Chrome inside the startup window is left alone so a concurrent call cannot kill a Chrome that is still starting. 2. Off-domain redirect revocation (P1): when a same-site link passed the gate but the navigation redirected to a different registrable host, navigateApprovedLink returned navigation_failed while the bridge stayed 'connected' with the old approvedTarget — leaving the tab approved while actually sitting on an unapproved site, so a later command could run against the off-domain page. Now the bridge calls handleLost (drops to 'lost', closes the CDP client, requires re-approval) before returning the navigation_failed result. Adds tests: respawn-after-exit, no-kill-during-startup, and off-domain-redirect → lost + subsequent command refused.
Finding 1 (Stop cannot resume): the local Stop latch was a boolean cleared only by attach()/dispose, but the SERVER deliberately resumes stopped → active on a fresh user turn — so after Stop + new turn the poller refused every handed-out command as bridge_disconnected until the user re-approved a tab. The server is the single stop/resume authority; the local latch exists ONLY to close the hand-out→execute race (a command handed to the wire just before the stop landed). It is now a timestamp: the poller snapshots when its long-poll starts and gates a returned command only when Stop was pressed AFTER that instant (the raced case), then clears the latch either way. A poll started after the Stop can only return a command from a server already resumed by a new turn — it executes normally. Both prior guarantees hold: a raced pre-stop command is still refused, and a Stop with no new turn stays stopped because the server gate hands out nothing. Re-approval is required only after take-over/lost/revoke, never after Stop. Finding 3 (scroll/wait post-read verification): inspect() and navigateApprovedLink() verified the readback URL against the grant, but scroll() and wait() returned observations unchecked — a cross-site navigation racing the readback could leak off-domain content. All four observation-producing primitives now route through one shared verifyObservedOnApprovedDomain() helper (same per-action status mapping: navigate → navigation_failed, others → permission_denied; off-domain drops the bridge to lost) and lastLinks only updates from observations that passed the check.
…atch may clear Blocking 1: verifyObservedOnApprovedDomain treated a hostless observed URL (file:///…, data:…, about:blank — registrableHost '') as approved, returning ok with content and updating lastLinks. Now any PRESENT observed.url whose registrable host differs from the grant — including the empty host — drops the bridge to lost and refuses (same per-action status mapping); only an absent url passes, since extraction gave nothing to verify. navigateApprovedLink's pre-check now uses the shared hostMatchesGrant helper (behavior-identical); the frameNavigated listener and the post-read check document why they don't. Blocking 2: the renderer POSTs /browse/control/stop fire-and-forget, so clearing the latch merely because a poll started after stopRequestedAt could execute a command handed out before the server gate actually set. The latch now carries a main-owned ack: while armed and un-acked the poller itself POSTs the idempotent /browse/control/stop and records ackAt on 2xx (no dependence on the renderer's POST); every handed-out command is gated until a poll starts after the ack, and only such a post-ack poll (command or empty) clears the latch. A fresh Stop resets the ack. Stop <1s is unchanged (requestedAt set immediately on IPC); post-resume commands execute; Stop with no new turn stays stopped. Removed the dead old-latch isStopRequested/isStopRequestedSince API.
2f88a73 to
e7db6c0
Compare
The self-ack was unpinned: the poller POSTed /browse/control/stop with whatever conversation main was bound to AT ACK TIME, and ackStopRequest blindly stamped the current latch. Two failure modes: stopping conversation A then switching to task B before the ack fired would wrongly stop B (and clear A's latch off B's meaningless ack); and an in-flight ack for an older Stop returning after a fresh Stop would satisfy the newer one. The latch now carries identity + generation. requestStop(cid) pins the conversation the Stop TARGETED — passed explicitly over IPC BROWSER_STOP, falling back to main's binding captured at REQUEST time — and bumps a monotonic stopGeneration. The poller's self-ack POSTs the LATCH's stored conversation id (never the current binding) and captures the generation before the POST; ackStopRequest(generation) no-ops unless the generation still matches, so stale acks are rejected. clearStopRequest never resets the generation (stays monotonic across clears). Gating comparisons are unchanged (per-latch requestedAt/ackAt).
…ession
The self-ack POST /browse/control/stop used to SET the server gate rather
than confirm it: stop at t0, fresh-turn resume at t1, poller cycle at t2
saw the latch armed+un-acked and its POST re-stopped the freshly-resumed
session (observed live — the next dispatch was refused 'session stopped'
until a second resume).
The Stop now carries a client-generated stop_id. The latch stores it
alongside conversationId/generation; the poller's ack POST sends
{conversation_id, stop_id}, and the server (sibling change) treats an
already-applied stop_id as a pure acknowledgement — 2xx without touching
the control state — while an unseen stop_id still sets the gate (covers
the renderer's fire-and-forget POST being lost). requestStop mints a
fallback UUID when the IPC carried none so the ack is always tokened.
1 participant
Layer 2 of 3 of Browser Control Milestone 1 (read-only) for the cowork desktop app, stacked on
1-shared. This layer is the Electron main process: Chrome discovery, the read-only CDP bridge, and the long-poll command poller against cowork-server's/api/v1/browseAPI. No renderer/UI changes — that's layer 3 (#412).Stack:
1-shared→2-main(this PR) → #412. Review/merge bottom-up. Merge mechanics: merge bottom-up and delete each layer branch as it merges — GitHub then auto-retargets the next PR's base tostaging; don't merge an upper PR while its base still points at a layer branch.Changes
chrome-discovery.ts(new): finds a Chrome DevTools endpoint (per-platform defaults,process.platform-keyed).cdp-client.ts(new): minimalws-based CDP client (no puppeteer/playwright) — request/response correlation, event subscription, hard close on error.browser-bridge.ts(new, the core): attaches to exactly ONE user-approved tab; executes the four read-only primitives (inspect / navigate-approved-link / scroll / wait); every CDP call passesisReadonlyCdpMethod; off-approved-host links refused asnavigation_failed; ALL four observation-producing primitives route their readback through oneverifyObservedOnApprovedDomainhelper — a present observed URL whose registrable host is empty (file://,data:,about:blank) or differs from the grant fails closed (handleLost+ refusal;lastLinksonly updates after the check passes); tab/Chrome loss →tab_closed/bridge_disconnectedwithrequires_reapprovalsemantics; per-bridge conversation binding cleared on detach/dispose. The main-side Stop latch (set viaBROWSER_STOPIPC) is timestamp+ack based and conversation/generation-bound: it closes only the hand-out→execute race — the SERVER is the sole stop/resume authority (a fresh user turn resumes a stopped session). While armed and un-acked, the poller itself POSTs the idempotent/browse/control/stopfor the latch's pinned conversation and records the ack; a command is gated only until a poll starts after the ack, so post-resume commands execute instead of being refused forever; stale acks from an older Stop generation are ignored. The latch also carries the Stop's client-mintedstop_idtoken: the self-ack POST re-sends that same token, which the server treats as a pure ack when already applied (state untouched), so an ack landing after a fresh-turn resume can never re-stop the resumed session.browser-command-poller.ts(new): hello (conversation_id→ capturessession_id) → long-pollcommands/next→ execute → postresults, all/api/v1-prefixed and schema-exact; sendsobservedonly on ok results with allowlisted digest keys and host-onlyfinal_domain; 404 → session invalidation + re-hello; conversation switch invalidates the cached session; a raced Stop (landing while a long-poll was outstanding) gates the returned command with a reported terminal result, without touching the page, and the latch clears once a post-ack poll confirms the server gate — so a fresh-turn resume flows through cleanly.index.ts/preload.ts: IPC handler registration and thewindow.antontron.browserControl*surface (consumed by layer 3);BROWSER_STOPcarries the Stop's target conversation id and itsstop_idtoken.How to review
browser-bridge.ts— the security core. Check: single-tab attachment, the link gate (registrableHost(href) !== target.domain→ refuse), the landed-host verification after navigation, and that every primitive goes through the read-only method check.browser-command-poller.ts— the server contract. The in-testFakeBrowseServerasserts real request schemas and the/api/v1prefix, so drift fails tests; check the session lifecycle (hello/404/conversation-switch/Stop) state machine.cdp-client.ts/chrome-discovery.ts— small, mechanical.browser-bridge.integration.test.tsruns bridge+poller together against the fake server.Testing
npm run typecheck✓,check-cowork-purity✓,npx vitest run— 360 tests ✓ (on thestagingbase) (incl. tests for the review fixes: managed-Chrome respawn after crash/kill; off-domain redirect dropping the bridge tolostrequiring re-approval; hostless-URL readback (file:///data:/about:blank) failing closed for inspect/scroll/wait; the Stop latch: raced pre-ack command refused, Stop with no new turn stays stopped, post-resume command executes, ack failures keep gating, wrong-conversation and stale-generation acks ignored, and the self-ack POST carrying the latch'sstop_idtoken), all three builds ✓.process.platform(Linux CI/dev machine).