fix(tools): reuse supervisor sessions for browser_cdp target_id - #32950
fix(tools): reuse supervisor sessions for browser_cdp target_id#32950LeonSGP43 wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling the persistent-CDP-session path. The reported stateless behavior still exists on current main: browser_cdp opens a per-call WebSocket at tools/browser_cdp_tool.py:206 and target attachment is performed on that same connection at :217-260.
Problems
- The new
target_idreturn precedes the shared private-page guard attools/browser_cdp_tool.py:484-490. Commit47764f19f462c0b3a99865255f3b1dfae5098e74fixed the same early-return bypass forframe_id; the target route needs that guard too. - #32685's documented
Target.getTargets→ target-ID workflow is not covered. The new route only runs after a target ID is provided, but the new test reads privatesv._page_target_id.SupervisorSnapshothas no top-level target field (tools/browser_supervisor.py:255-279), andbrowser_snapshotexposes onlysnapshot.to_dict()(tools/browser_tool.py:2998-3007). - The new success payload should redact its result like the stateless payload does at
tools/browser_cdp_tool.py:524-527.
Suggested changes
- Preserve validation and run
_browser_cdp_private_guardbefore target routing; add the corresponding private-page regression test. - Provide and test a public supervisor-backed target-discovery path, then use its returned target ID in the E2E test.
- Update
website/docs/user-guide/features/browser.md:549, which currently says each non-frame_idcall is stateless.
Automated hermes-sweeper review.
| @@ -340,6 +405,16 @@ def browser_cdp( | |||
| params=params, | |||
There was a problem hiding this comment.
This early return is before current main's shared validation and _browser_cdp_private_guard (tools/browser_cdp_tool.py:448-490). A resolved supervisor target can therefore invoke a raw CDP read on a private current page, recreating the bypass fixed for frame_id in 47764f19. Run the shared guard before this route and add an equivalent target-ID private-page regression test.
|
Verification from a production browserless deployment (Fedora 43, rootless podman, hermes v0.19.0, self-hosted browserless v2 / Chrome 149, tokenless One edge case worth handling (or at least documenting) before merge: under browserless, the supervisor's websocket owns a different browser than the agent-browser CLI daemon that A cheap guard would be: if the daemon-driven page's targetId does not appear in the supervisor's |
|
@LeonSGP43 — this fix matters for every Browserless/BaaS deployment (we run one in production, verification above), and it'd be a shame to lose it to the conflict backlog: the branch is now ~2 months behind main ( If you're still on it — great, happy to re-verify a revision against our deployment. If you're unavailable, I'll re-file this rebased onto current main with the review feedback applied as a salvage PR (crediting this one, per house convention) early next week so the mechanism doesn't stall. Just say the word either way. |
|
Salvage PR is up as promised: #71743 — rebased onto current main with the July 13 review feedback applied (guard before target routing + regression test, public |
NousResearch#32950) browser_cdp opens a fresh CDP WebSocket per call. On Browserless-style backends that spawn a private browser per connection, a target id from one call is invalid by the next — multi-step Target.getTargets -> target_id workflows always fail (NousResearch#32685). Route target-scoped calls through the live CDPSupervisor session when the target is already attached (top-level page, OOPIF frame, or auto-attached child), falling back to the stateless attach otherwise. Salvages NousResearch#32950 (credit: @LeonSGP43) rebased onto main with the review feedback applied: - the target route runs after the shared private-page guard, so it cannot become the sibling bypass fixed for frame_id in 47764f1 (regression test included) - public target discovery: SupervisorSnapshot now carries page_target_id (surfaced in browser_snapshot output via to_dict); the E2E test uses it instead of private supervisor attributes - the supervisor-backed payload redacts its result via _redact_cdp_output like the stateless payload (regression test included) - browser.md no longer claims every non-frame_id call is stateless
Summary
browser_cdpis called with atarget_idthat already belongs to the connected supervisorCDPSupervisorand resolve target ids back to live session ids before falling back to the legacy stateless attach flowbrowser_cdp(target_id=...)against the supervisor-backed pathCloses #32685.
Verification
uv run --frozen --extra dev pytest tests/tools/test_browser_cdp_tool.py tests/tools/test_browser_supervisor.py -qsource /Users/leongong/Desktop/LeonProjects/worktrees/hermes-agent/.base/.venv/bin/activate && ruff check tools/browser_cdp_tool.py tools/browser_supervisor.py tests/tools/test_browser_supervisor.pygit diff --check