fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950)

[chrisyoung2005]

What / Why

browser_cdp opens a fresh CDP WebSocket per call. On Browserless-style backends that spawn a private browser per connection, a target id returned by one call is invalid by the next — the documented multi-step Target.getTargets → target_id workflow always fails (#32685).

This routes target-scoped calls through the live CDPSupervisor session when the target is already attached (the top-level page, an OOPIF frame, or an auto-attached child target), falling back to the legacy stateless attach otherwise (plain Chrome shares targets across connections, so statelessness keeps working there).

Salvage of #32950 (credit @LeonSGP43) — that branch has been conflicting since May with the review feedback unaddressed. Rebased onto current main with all three review asks applied:

• Private-page guard: the target route runs after the shared _browser_cdp_private_guard, so it cannot become the sibling bypass fixed for frame_id in 47764f1. Regression test included (test_target_id_route_blocked_when_current_page_is_private).
• Public target discovery: SupervisorSnapshot now carries page_target_id (surfaced in browser_snapshot output via to_dict()), and the E2E test discovers the target through it — no private-attribute reads.
• Redaction: the supervisor-backed payload runs _redact_cdp_output like the stateless payload. Regression test included (test_target_id_route_via_supervisor_redacts_secret_result).
• website/docs/user-guide/features/browser.md no longer claims every non-frame_id call is stateless.

Fixes #32685.

How to test

scripts/run_tests.sh tests/tools/test_browser_cdp_tool.py tests/tools/test_browser_supervisor.py

51 passed, 0 failed (the supervisor file needs a local Chrome; its two new E2E tests use only the public snapshot path). New tests fail before the fix (AttributeError on the missing snapshot field / missing route).

Production verification: self-hosted Browserless v2 (Chrome 149), Fedora 43, rootless podman — Target.getTargets → Runtime.evaluate(target_id=...) chains that previously landed in a fresh empty browser now resolve against the supervisor session (details in the #32685 / #32950 comment threads).

Platforms tested

• Linux (Fedora 43), Python 3.11, Chrome-for-Testing 151 headless (E2E) + in-process mock CDP server (unit)
• scripts/check-windows-footguns.py clean on all touched files

[teknium1]

Thanks for carrying forward the private-page guard, public snapshot field, and result-redaction fixes from the earlier review.

Problems

• The reported Target.getTargets → target_id workflow is still stateless. The new route is conditional on an already-present target ID at tools/browser_cdp_tool.py:582; Target.getTargets has none and falls through to a fresh _cdp_call at :593-596. That does not make the two calls share a Browserless private browser.
• page_target_id identifies the supervisor's independently connected browser (tools/browser_supervisor.py:659-690, :770-786), not necessarily the agent-browser daemon's navigated page. The #32685 reproduction documents this split on Browserless, so mixed navigation/CDP usage can silently inspect the wrong page.

Suggested changes

• Route target discovery through the supervisor connection too, then add a per-WebSocket-isolation test for the full discovery/evaluate chain.
• Define and test the mixed daemon/supervisor behavior (verified shared target, fallback, or explicit error) before documenting supervisor state as canonical.

Automated hermes-sweeper review.

[teknium1]

Target.getTargets has no target_id, so it bypasses this branch and still reaches the new per-call WebSocket at :593-596. On Browserless that means its returned target belongs to a different private browser; route discovery through the supervisor as well and cover the full discovery-to-evaluate sequence.

[chrisyoung2005]

Both points addressed in c7869e1f4 (rebased onto current main first).

Discovery now rides the supervisor connection. Correct — Target.getTargets had no target_id, skipped the routing branch, and still opened a fresh per-call WebSocket, so on Browserless the discovery result described a different private browser than the one a follow-up target_id call executed in. _browser_cdp_target_via_supervisor now accepts target_id=None and dispatches browser-level commands (no sessionId) on the supervisor's WebSocket — _cdp already treats session_id as optional, so this is the same connection either way — and the call site routes every post-guard call through it when a live supervisor exists. The requested per-WebSocket-isolation test is a real-Chrome integration test covering the full chain: the stateless _cdp_call is patched to fail the test if anything reaches it, then Target.getTargets (asserted to include the supervisor's own attached page — proof both calls observe one browser) followed by Runtime.evaluate on the discovered target_id, both succeeding over the supervisor connection. Unit tests cover the browser-level dispatch shape (no sessionId on the wire) and the no-supervisor stateless fallback.

Mixed daemon/supervisor behavior — defined and documented. The semantics this PR now commits to, of your three options, are verified-shared-connection with stateless fallback:

• Live supervisor → all browser_cdp traffic (discovery + target-scoped) shares its connection, so results are mutually coherent by construction; responses carry "connection": "supervisor" so callers can see which regime they're in.
• No supervisor, or a target_id the supervisor has no session for → legacy stateless connection, unchanged (the plain-Chrome path, where every connection sees the shared browser).

On the second half — page_target_id describing the supervisor's independently-connected browser rather than the agent-browser daemon's page: that's exact, and it's the supervisor re-attach root cause we split into #74216 (wrong-page dialog bridge + snapshot enrichment; consolidates the #32685 repro, the plain-Chrome multi-tab confirmation on #71744, and the re-attach direction — a contributor is sequencing that work after #71745). Fixing it belongs at the supervisor attach layer, not per-call in browser_cdp; until it lands, browser.md now states explicitly that the supervisor's browser is not necessarily the daemon's page and that within browser_cdp supervisor state is the canonical view, with a pointer to the tracking issue. Happy to re-scope if you'd rather this PR waits on #74216, but the eval-side companion (#71745) took the same boundary per the triage recommendation.

[GottZ]

This was generated by AI during triage.

Summary

Nine PRs span two distinct causes: #32950 and #71743 address #32685 by reusing a persistent CDP supervisor connection, while #4143, #6623, #27304, #30893, #33246, #34131, and #34497 address execute_code terminal exposure, environment leakage, approval gating, or lost thread context. The execute_code cluster is already consolidated in merged #34497; the remaining open issue is the Browserless per-WebSocket lifecycle handled by the two browser PRs.

Related pull requests

• fix(security): remove terminal from sandbox allowed tools in execute_code #4143 [closed] partial — (+338/-222) — n/a: Removes terminal from execute_code, closing one call path but not the confirmed lost approval-context cause; closed after a contributor rejected terminal revocation as the architectural fix, and retained only as security-cluster history.
• fix(tools): revoke terminal access within execute_code sandbox to prevent escape #6623 [closed] partial — (+58/-49) — n/a: Duplicates fix(security): remove terminal from sandbox allowed tools in execute_code #4143's terminal-revocation approach without addressing RPC thread context or browser_cdp persistence; closed and relevant only as the same superseded proposal.
• fix: harden execute_code env and approvals #27304 [closed] fixes — (+102/-10) — n/a: Adds explicit child-environment filtering and an RPC-boundary terminal guard, but not complete ContextVar and callback propagation; superseded by merged fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497, which incorporated its env-scrub direction.
• [codex] Guard execute_code behind gateway approvals #30893 [closed] fixes — (+406/-66) — n/a: Adds the fail-closed whole-script execute_code guard for gateway, ask, and cron-deny contexts, covering direct Python escape paths; superseded by merged fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497, which combined that guard with RPC context restoration.
• fix(tools): reuse supervisor sessions for browser_cdp target_id #32950 best fix — (+121/-0) — best fix: Introduces the core target-ID-to-live-supervisor-session reuse for [Bug]: browser_cdp opens stateless CDP connections, breaking Browserless/BaaS targets between tool calls #32685, but its keep_open review identifies guard-ordering, private discovery, and redaction gaps still present in this diff; fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950) #71743 preserves the mechanism and adds those fixes plus same-connection discovery coverage.
• [codex] Preserve context in execute_code RPC threads #33246 [closed] partial — (+62/-4) — n/a: Propagates ContextVars into both execute_code RPC threads but omits thread-local approval and sudo callbacks; superseded by fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497's complete shared propagation helper.
• fix(security): restore approval/sudo context in execute_code RPC threads + guard entry points #34131 [closed] fixes — (+827/-151) — n/a: Implements the confirmed execute_code root-cause fix—full context/callback propagation, entry gating, and environment hardening—but was closed as redundant after the same implementation landed through fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497.
• fix(security): restore approval/sudo context in execute_code + guard entry points (salvage #34131) #34497 [merged] best fix — (+897/-151) — best fix: Merged reference implementation for Critical: Sandbox code execution bypasses dangerous command approval via terminal tool #4146, execute_code can leak common secret env vars and bypass terminal approval guard #27303, [Bug]: [Security] approvals.mode=manual silently bypassed in gateway mode — dangerous commands execute without approval, including via execute_code sandbox evasion #30882, and fix(cli): dangerous command approval bypass in execute_code RPC threads #33057, combining complete RPC context/callback propagation, fail-closed script gating, and hardened environment scrubbing; it remains relevant as the landed consolidation, not as a browser_cdp fix.
• fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950) #71743 best fix — (+522/-10) — best fix: Salvages fix(tools): reuse supervisor sessions for browser_cdp target_id #32950 and routes both Target.getTargets discovery and target-scoped calls over one supervisor WebSocket, with private-page guarding, public target discovery, redaction, stateless fallback, and a discovery-to-evaluate regression test. The visible keep_open review should remain controlling until contributor re-review confirms these changes and accepts the documented split of mixed daemon/supervisor lifecycle work to [Bug]: CDPSupervisor attaches to an arbitrary page and is never re-pointed — dialog bridge and snapshot enrichment target the wrong page (daemon/supervisor split-brain) #74216.

Duplicates

#4143 and #6623 duplicate the terminal-revocation approach. #27304, #30893, #33246, and #34131 are superseded precursors consolidated in merged #34497; #32950 and #71743 share the browser supervisor-reuse mechanism, with #71743 as the expanded salvage.

Suggested consolidation

Keep #71743 open with a salvage path: preserve its same-supervisor discovery-to-evaluate chain, guard, redaction, public snapshot field, fallback tests, and attribution, then obtain contributor re-review of the still-visible keep_open verdict. Despite the keep_open review and recorded best-fix status on #32950, close #32950 as duplicate of #71743 because its diff retains the three cited blockers while #71743 carries its core fix and addresses them; no further action is needed for the execute_code PRs because merged best-fix #34497 is their consolidated reference implementation.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I32685(["issue #32685 (open)"])
    subgraph Dup32950 ["PRs duplicating each other"]
        P32950["PR #32950 (open)"]
        P71743["PR #71743 (open)"]
    end
    P71743 -->|best fix| I32685
    class I32685 open
    class P32950 open
    class P71743 open
    class P32950 best
    class P71743 best
    class P71743 target
    click I32685 "https://github.com/NousResearch/hermes-agent/issues/32685"
    click P32950 "https://github.com/NousResearch/hermes-agent/pull/32950"
    click P71743 "https://github.com/NousResearch/hermes-agent/pull/71743"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 9 pull requests and 5 issues in this complex. Each diff was read against this issue; Assessment working set: 239 kB of PR diffs, 39 kB of issue/PR text, 23 kB of discussion (25 comments), 38 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(tools): reuse supervisor session for browser_cdp target_id (salvage #32950)

1. _browser_cdp_target_via_supervisor blocks the calling thread on fut.result(timeout=timeout + 2) while the CDP call runs on the supervisor's loop. If the loop is blocked or the command hangs, the tool blocks up to timeout + 2s, and a hung _cdp past its own timeout surfaces as a generic tool_error. Using the same safe_timeout value as the stateless path (and not an arbitrary +2 margin) would make the two paths' latency contracts symmetric.
2. resolve_target_session reads _page_target_id, _frames, and _child_sessions under _state_lock — good. But the routing helper reads supervisor._loop and _cdp without the lock; mid-reconnect reads could see a torn state (caught by the try/except and falling back, so safe, but a lock-guarded read would be deterministic).
3. Discovery shape (target_id=None) now rides the supervisor connection when one is live — a behavior change for browser-level methods on plain Chrome that previously went stateless. The docs update covers it; just confirm no caller relied on stateless isolation (different profile/context) for browser-level calls.
4. _page_target_id is reset in _run on reconnect, and snapshot()/to_dict() expose it — good. One nit: resolve_target_session returns None when _page_target_id matches but _page_session_id is empty (partial attach), which correctly falls back to stateless; consider a brief comment documenting that partial-attach case.

[chrisyoung2005]

Thanks — 1 and 4 documented in 6411c34f5; 2 and 3 answered below. No behavior change; unit suite 17 green (test_browser_supervisor.py remains marker-gated as before).

1 (timeout + 2 vs safe_timeout). The two paths' latency contracts are already symmetric at the value level — both receive the same clamped safe_timeout from the tool entrypoint. The +2 is only the outer future guard: the inner supervisor._cdp(...) enforces timeout itself, so the margin exists to let the inner, more specific CDP timeout error surface instead of a generic fut.result timeout, and it only fires if the loop itself is wedged. Now stated in a comment at both call sites.

2 (unlocked _loop/_cdp reads) — declining the lock. As you note, a torn read is caught and falls back to the stateless path, so the worst case is one call taking the slow path. Acquiring _state_lock on the tool hot path would buy determinism nobody observes at the cost of contention with the supervisor's event thread.

3 (plain-Chrome browser-level behavior change). Confirmed no caller depends on stateless isolation: browser_cdp has no internal callers (only its own registry handler), and on plain Chrome targets are shared across connections, so browser-level discovery returns the same data either way. The only observable difference is on per-connection-browser backends — where riding the supervisor's connection is precisely the fix (#32685). The docs note covers it.

4 (partial attach) — comment added at the resolve_target_session early return: _page_target_id known but _page_session_id not yet ⇒ deliberate None, so callers take the stateless fallback rather than racing a half-attached session.