fix(tools): gate browser_console eval fast-path on supervisor watching the daemon's page by chrisyoung2005 · Pull Request #71745 · NousResearch/hermes-agent

chrisyoung2005 · 2026-07-26T04:09:50Z

Fixes #71744. Sibling of #71743 / #32685 (same root cause, different tool path; diffs are independent).

Summary

Symptom: browser_console(expression=…)'s supervisor fast path evaluates in whatever page the supervisor's own CDP connection attached — not necessarily the page the agent-browser daemon is driving. On Browserless-style backends (private browser per CDP websocket) it never is: evals "succeed" against the supervisor's about:blank and return ""/null, and the always-correct subprocess fallback never runs because the fast path reported ok. On plain Chrome the same split occurs when the daemon drives a different tab than the one the supervisor attached, or after a click opens a new tab.
Change: tools/browser_tool.py records, per session key, the final (post-redirect) URL of each successful browser_navigate (refreshed by browser_back's reported landing URL) and, at that moment, binds the supervisor's top-frame id iff the supervisor is provably showing that same URL. _eval_supervisor_fast_path now runs only when the supervisor's live top frame is still that bound frame and its URL matches the recorded one (fragment-insensitive). Click / Enter / Space mark the session "possibly diverged" (a click can open a new tab whose URL still matches the stale record) until the next authoritative URL. cleanup_browser drops the bookkeeping. Zero extra subprocess calls — both sides of every comparison are already in memory.
Behavior: matching page → fast path at full speed, as today. Mismatch, no binding, diverged mark, or unreadable supervisor state → the agent-browser subprocess path, which always evaluates in the daemon's session. Nothing else about eval, redaction, or the SSRF guards changes.

Evidence

scripts/run_tests.sh tests/tools/test_browser_eval_supervisor_path.py   # 26 passed

TestSupervisorDaemonPageSplit + TestEvalFastPathDivergence: split-brain fall-through, matching page keeps fast path, fragment-only difference keeps it, same-URL-different-target falls back (reviewer scenario, verbatim), matching URL without bound identity falls back, no recorded navigation falls back, unreadable supervisor state falls through, click/Enter/Space stand the fast path down, navigate/back re-arm it, cleanup clears state. 11/24 fail before the fix (the split-brain case returns the wrong-browser result). Also green: test_browser_console.py, test_browser_console_ssrf.py, test_browser_eval_ssrf.py.

Production verification (self-hosted Browserless v2, Fedora 43): browser_navigate → idle → browser_console(expression="document.title") returns the real title where the ungated fast path returned "" for ten days.

Residual (deliberately out of scope, tracked in #74216): the supervisor's dialog bridge and snapshot frame-tree enrichment have the same wrong-page problem; this PR only fixes the eval path.

Branch state

On current main — re-homed onto the #102117 module split on 2026-09-04 (the facade still owns navigate/click/back/press/eval; cleanup_browser hook lives in browser_tool_lifecycle.py; tests patch browser_tool_session._run_browser_command / browser_tool_eval_policy._eval_ssrf_guard_active). Two source files, one test file. Happy for this to be cherry-picked / salvaged if that's the faster path.

Platforms tested

Linux (Fedora 43), Python 3.11; unit tests are mock-based (no browser needed). scripts/check-windows-footguns.py clean on touched files.

drowchaeshew · 2026-07-27T07:56:15Z

@@ -2972,6 +2978,7 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str:
        # Failed opens and blocked redirects must not retarget follow-up clicks
        # or snapshots to a newly-created but irrelevant session.
        _last_active_session_key[effective_task_id] = nav_session_key
+        _last_navigated_urls[nav_session_key] = str(final_url or "")


tl;dr: Daemon url changes not only happens on browser_navigate().

Consider this scenario (cdp, browserless):
0. There's only on tab with url https://www.example.com; both cdp & daemon bind to this tab;

With browser_click, a new tab is opened - daemon binds to this newly created tab, while cdp don't.

invoke browser_console(document.title) result in a https:///www.example.com.

Suggestion?

quick fix: add _last_navigated_urls[nav_session_key] = str(final_url or "") to browser_click() also (though I'm not sure if a url can be obtained in that scenario...)

Good catch — confirmed, and fixed in 07be57692. Your scenario is exactly the blind spot: after a click opens a new tab, the daemon rebinds while the supervisor keeps showing the old page, whose URL still matches the stale record, so the URL comparison alone keeps trusting the fast path.

On the suggested quick fix: browser_click's result carries no URL, so recording one there would mean probing the daemon — a subprocess call per click, and clicks are far more frequent than evals. So the commit inverts it:

browser_click and browser_press("Enter") mark the session as possibly diverged (zero extra calls). Marked sessions skip the fast path and take the always-correct agent-browser subprocess path — worst case is the subprocess spawn we'd have paid anyway pre-fast-path, never a wrong-tab result.

The mark clears on the next authoritative URL: browser_navigate, or browser_back — whose result does report the landing URL, so it now re-records _last_navigated_urls too.

Other keys and failed clicks keep the fast path.

7 new tests in test_browser_eval_supervisor_path.py::TestPageDivergenceAfterInteractions, including your new-tab scenario verbatim (test_click_then_eval_falls_through_to_subprocess); 5 fail pre-fix. Full browser suite green (555 passed).

drowchaeshew · 2026-07-27T09:02:47Z

Here, a small problem: consider this scenario:

only a new tab in your browser;

browser_nagivate('https://www.example.com')

browser_snapshot(): likley _supervisor (3121) is not None, which will bring the wrong frame tree info (e.g. chrome-untrusted:new-tab-page/one-google-bar) to response.

Solution?

Quick-fix: Use _supervisor_page_matches_daemon() & set _supervisor to None if not matches. (but this will permanently disable the merging logic, since nobody fix the wrong supervisor in SUPERVISOR_REGISTRY, the _supervisor_page_matches_daemon() will never returns True once split-brain happens).

Fix permanently: ... frankly speaking I have no idea.

remove supervisor once and for all: too aggresive!

find solution to synchonize the url between supervisor & daemon (really hard work...)

Agreed this is real — it's the snapshot-enrichment half of the same supervisor split-brain — but I'd argue it belongs with the supervisor re-attach work rather than this PR, which is scoped to the eval fast-path (where a wrong answer is returned as the result; the snapshot case pollutes auxiliary frame info merged onto an otherwise-correct daemon snapshot).

Your analysis of the quick fix matches why I didn't gate the merge here: nothing ever re-attaches the supervisor to the right page today, so if not _supervisor_page_matches_daemon(...): _supervisor = None degrades "sometimes wrong frame tree" into "permanently no dialog/frame merging after the first divergence" — dialogs are the supervisor's most load-bearing duty, and a pending-dialog report is correct browser-wide even when the frame tree is stale.

The durable fix is synchronizing the supervisor with the daemon's target, which is the #32685 / #32950 / #71743 family — on #32950 I proposed a split-brain guard (daemon targetId ∉ supervisor's Target.getTargets → warn/re-attach) that would fix eval, snapshot, and dialogs at the root. If maintainers prefer, I'm happy to file a follow-up issue for the snapshot-merge case specifically so it's tracked — this PR does leave _supervisor_page_matches_daemon + the new divergence mark importable by browser_snapshot, so a scoped follow-up (e.g. merge dialogs but drop frame_tree on mismatch) would be small.

teknium1 · 2026-07-30T12:15:45Z

Thanks for tracing the split-brain failure and adding focused coverage. The current-main premise is real: tools/browser_tool.py:3799-3802 evaluates through any registered supervisor, while tools/browser_supervisor.py:741-756 attaches that supervisor to the first page target it sees.

Problems

The proposed URL gate does not establish page identity. Two different CDP page targets can have the same URL; in that case the helper accepts the fast path even though evaluate_runtime() still runs in the supervisor's stored session (tools/browser_supervisor.py:533-553). This leaves the stated different-tab case unresolved.
The related browser_snapshot() path still merges frame_tree from any active supervisor (tools/browser_tool.py:3195-3200) into a daemon snapshot without a target check. The existing review discussion correctly identifies this remaining split-brain output path.

Suggested changes

Carry and compare a stable daemon target/session identity, or fall back whenever that identity cannot be established; add a same-URL/different-target regression test.
Track or address the snapshot frame-tree merge separately while preserving its dialog behavior.

Automated hermes-sweeper review.

chrisyoung2005 · 2026-07-30T16:13:20Z

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

Page identity, not URL equality. Agreed — a URL match can't establish identity when two page targets show the same URL. The constraint worth stating plainly: the daemon side cannot participate in an identity handshake, because agent-browser is an external CLI that never exposes its target id in any tool result. So the commit takes the strongest identity available without daemon cooperation, plus your suggested fallback for everything else:

_record_daemon_url (navigate, and back-with-reported-URL) now also captures the supervisor's top-frame id — but only when the supervisor's top-frame URL equals the fresh post-navigation URL, the one moment the two provably coincide. Any doubt (no supervisor, URL mismatch, unreadable state) clears the binding. On Browserless the supervisor's private browser sits on about:blank, so no binding is ever made and every eval takes the subprocess path — the [Bug]: browser_cdp opens stateless CDP connections, breaking Browserless/BaaS targets between tool calls #32685-correct behavior.
The gate now requires bound identity + stable frame id + URL match. No binding → never trust the fast path, which also removes the previous no-recorded-navigation legacy trust (/browser connect-then-eval now pays the subprocess spawn instead of risking a wrong-page result) — that's the "fall back whenever identity cannot be established" arm, taken literally.
Regression tests include your scenario verbatim: supervisor top-frame URL matches the recorded URL but its frame id differs from the one bound at navigation → subprocess path, evaluate_runtime never called. Plus: matching URL with no binding, navigate-binds/doesn't-bind, and the inverted no-recorded-navigation case. 11 of the file's 24 tests fail with the source half of the commit reverted; full browser suite green.

Residual case, disclosed rather than hidden: if a different target was already showing the same URL at the moment of navigation and the supervisor was attached to it, the binding is wrong and indistinguishable without daemon cooperation. It's narrower than the pre-commit exposure (which trusted any same-URL state at eval time, plus all unproven states) and self-heals on the next navigation; closing it fully needs the daemon to expose its target id, which is out of this repo's reach.

Snapshot frame-tree merge. Already split out and tracked: #74216 consolidates that path (wrong-page frame_tree/pending_dialogs enrichment and the must_respond dialog bridge) with the supervisor re-attach direction — it has a contributor sequencing their work after this PR precisely because re-attach wants to import this gate's helper. Kept out of this diff deliberately, per the earlier review discussion: nulling the supervisor on mismatch here would permanently kill dialog merging.

Enough1122 · 2026-08-15T18:44:50Z

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

fix(tools): gate browser_console eval fast-path on supervisor watching the daemon's page

Key-namespace consistency: browser_navigate calls _record_daemon_url(nav_session_key, ...) while browser_click/browser_back/browser_press and the gate in _browser_eval use effective_task_id (_last_session_key(task_id)). _last_active_session_key's own comment notes task_id → session_key can differ (sidecar sessions). If they differ for a task, the URL is recorded under nav_session_key but the gate reads _last_navigated_urls.get(effective_task_id) → always empty → the fast path is silently dead for those sessions (safe, but slower). Verify the two keys coincide or normalize them to one namespace.
_bind_supervisor_page_identity calls supervisor.snapshot() synchronously inside browser_navigate on every successful navigation — a CDP round-trip on the navigation hot path. If the supervisor connection is slow/stuck, navigate latency grows (exceptions are swallowed to debug, but the wait is not bounded). Consider a timeout around the snapshot.
browser_press only marks divergence for Enter/NumpadEnter — Space on a focused button/link also activates it in browsers and can navigate. Marking Space too (or checking the focused element) would be cheap conservatism.
Ordering nit: _record_daemon_url clears the divergence mark before _bind_supervisor_page_identity runs; if the bind fails (no supervisor / URL mismatch), the mark is cleared but the frame binding stays stale — the gate then still falls back via the missing binding, so behavior is correct, but the mark-clear-then-bind ordering is worth a comment so future readers don't "fix" it into clearing after.

chrisyoung2005 · 2026-08-18T16:06:46Z

Thanks — 3 and 4 acted on in 34eb0b0c6; 1 and 2 don't hold against the code, walk below.

1 (key-namespace mismatch). The two namespaces meet by construction: browser_navigate records the URL under nav_session_key and sets _last_active_session_key[task_id] = nav_session_key in the same success block (browser_tool.py:3546-3547). Readers resolve effective_task_id = _last_session_key(task_id), which returns exactly that recorded nav_session_key while the binding is live — same key, fast path available. When the binding is dropped (cleanup or ownership mismatch), _last_session_key falls back to the bare task_id, the gate then reads an empty recorded URL, and the fast path stands down — fail-closed to the always-correct subprocess path, which is the gate's intended posture for any doubt.

2 (snapshot on the navigate hot path). supervisor.snapshot() (browser_supervisor.py:427) is a pure in-memory read under _state_lock — tuples of already-collected state, no CDP round-trip, no unbounded wait. Worst case is brief lock contention with the event thread's state updates.

3 (Space) — fixed. Space activates a focused button/link, so browser_press("Space") can navigate exactly like Enter: both "Space" and " " now mark the session possibly-diverged. 2 new tests mirroring the Enter case, both fail pre-fix; suite runs 26 green.

4 (ordering) — documented. _record_daemon_url now states why the divergence mark is cleared before the identity bind: a failed bind pops the frame binding, and a missing binding alone stands the fast path down, so clear-first cannot re-enable a stale fast path.

chrisyoung2005 · 2026-08-26T18:16:58Z

Status: the triage recommendation (merge, via #71744) stands; all reviewer scenarios are regression tests (frame-identity gate 70015f3a3, divergence marks incl. the Space nit in 34eb0b0c6), residual supervisor split-brain tracked in #74216. Rebased onto upstream main daily; re-verified today against the v0.20.5 release tag (v2026.8.19) — cherry-picks clean, test_browser_eval_supervisor_path.py green. Ready for review or salvage-merge whenever convenient.

…g the daemon's page The CDPSupervisor opens its own CDP websocket and attaches to the first page target that connection sees. On Browserless-style backends every websocket gets a private browser, so the supervisor's connection can never see the page the agent-browser daemon navigated — the _browser_eval fast path 'succeeds' against the supervisor's own about:blank and returns empty/wrong results, while the subprocess fallback that would answer correctly never runs. On plain Chrome the same split occurs when the daemon drives a different tab than the one the supervisor attached. Production symptom (self-hosted Browserless v2): after browser_navigate loaded a product page, browser_console(expression="document.title") returned "" and price selectors returned null while browser_snapshot showed the fully-loaded page — misdiagnosed as target-site bot detection for ten days. Fix: record the final (post-redirect) URL of each successful browser_navigate per session key, and gate the supervisor fast path on the supervisor's live top-frame URL matching it (fragment-insensitive). Zero extra subprocess calls: both sides of the comparison are already in memory. No recorded navigation preserves legacy fast-path behaviour for /browser connect-then-eval flows; any mismatch or unreadable supervisor state falls through to the agent-browser subprocess path, which always evaluates in the daemon's session. Regression tests cover the split-brain fall-through, matching-page and fragment-only fast paths, the no-navigation legacy path, and snapshot failure; all fail before the fix.

Review feedback on the URL-match gate: a click can open a new tab — the daemon rebinds to it while the supervisor keeps showing the old page, whose URL still matches the recorded last-navigated URL, so the URL comparison alone wrongly keeps the fast path and evaluates in the old tab. Click results carry no URL, and probing the daemon would cost a subprocess call per click, so instead: - browser_click and browser_press Enter mark the session as possibly-diverged; marked sessions skip the fast path and take the always-correct agent-browser subprocess path - browser_navigate and browser_back (whose result reports the landing URL) record the daemon's authoritative URL and clear the mark - other keys and failed clicks keep the fast path

… fast-path Review follow-up: a URL match cannot establish page identity — two CDP page targets can show the same URL, and the supervisor would still evaluate in its own stored session. The gate now requires the supervisor's top frame to be the exact frame bound at the last authoritative navigation: * _record_daemon_url captures the supervisor's top-frame id at the one moment daemon and supervisor provably coincide — when the supervisor's top-frame URL equals the fresh post-navigation URL. Any doubt (no supervisor, URL mismatch as with a Browserless private browser stuck on about:blank, unreadable state) clears the binding. * _supervisor_page_matches_daemon requires bound identity + stable frame id + URL match; no binding means the fast path is never trusted — including the former no-recorded-navigation legacy trust, which is exactly the "fall back whenever identity cannot be established" case. The daemon side cannot participate directly: agent-browser is an external CLI that does not expose its target id, so identity is inferred at navigation time and required stable thereafter. New tests: same-URL/different-target (review scenario), matching URL without bound identity, navigate binds/doesn't-bind identity, plus the inverted no-recorded-navigation case. 11 fail with the source reverted.

…rged Review follow-up: Space activates a focused button/link in browsers, so a browser_press("Space") can navigate exactly like Enter — mark the session possibly-diverged for it too (both "Space" and " " spellings). Also documents why _record_daemon_url clears the divergence mark before the identity bind (a failed bind pops the frame binding, which alone stands the fast path down). 2 new tests, both fail pre-fix; suite 26 green.

chrisyoung2005 · 2026-09-04T19:02:03Z

Re-homed onto current main after #102117 (the whole-codebase simplification) landed this morning and left this branch conflicting — head 786e363715, mergeable again. Port notes: the facade still owns navigate/click/back/press/eval, so the gate sits in _eval_supervisor_fast_path, the click/press hooks ride a small on_success param on _guarded_action, and the cleanup pops moved to browser_tool_lifecycle.py; tests repointed to the defining siblings (browser_tool_session._run_browser_command, browser_tool_eval_policy._eval_ssrf_guard_active), same as upstream's own 09-03 repoint. 26 passed. git diff main...HEAD audited to contain only this PR's files; no behavior change vs. the pre-refactor version.

PR body rewritten in the Symptom → change → behavior shape. If cherry-picking/salvaging is the faster path for this one, please go ahead — no objection to authorship going either way.

alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/browser Browser automation (CDP, Playwright) labels Jul 26, 2026

drowchaeshew reviewed Jul 27, 2026

View reviewed changes

GottZ mentioned this pull request Jul 27, 2026

[Bug]: browser_console eval fast-path evaluates in the wrong browser/tab (supervisor attached to a different page than the agent-browser daemon) #71744

Open

chrisyoung2005 mentioned this pull request Jul 29, 2026

[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

Open

teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch from 07be576 to 70015f3 Compare July 30, 2026 16:12

chrisyoung2005 mentioned this pull request Jul 30, 2026

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

Open

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 8 times, most recently from f4a09ba to 006cd9d Compare August 9, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 5 times, most recently from 3747d48 to d2e29b5 Compare August 15, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 3 times, most recently from 29086f3 to 430cd9b Compare August 18, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 7 times, most recently from 07eb3b3 to e18ba01 Compare August 25, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch from e18ba01 to 30e0ca2 Compare August 26, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 8 times, most recently from 9073141 to ed3b50d Compare September 2, 2026 10:26

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch 2 times, most recently from 9b2f075 to 21c08a5 Compare September 4, 2026 10:26

chrisyoung2005 added 4 commits September 4, 2026 11:09

chrisyoung2005 force-pushed the fix/browser-eval-supervisor-page-mismatch branch from 21c08a5 to 786e363 Compare September 4, 2026 18:57

Navigation Menu

Uh oh!

fix(tools): gate browser_console eval fast-path on supervisor watching the daemon's page - #71745

fix(tools): gate browser_console eval fast-path on supervisor watching the daemon's page#71745
chrisyoung2005 wants to merge 4 commits into
NousResearch:mainfrom
chrisyoung2005:fix/browser-eval-supervisor-page-mismatch

chrisyoung2005 commented Jul 26, 2026 •

edited

Loading

Uh oh!

drowchaeshew Jul 27, 2026

Uh oh!

chrisyoung2005 Jul 28, 2026

Uh oh!

drowchaeshew Jul 27, 2026 •

edited

Loading

Uh oh!

chrisyoung2005 Jul 28, 2026

Uh oh!

teknium1 commented Jul 30, 2026

Uh oh!

chrisyoung2005 commented Jul 30, 2026

Uh oh!

Enough1122 commented Aug 15, 2026

Uh oh!

chrisyoung2005 commented Aug 18, 2026

Uh oh!

chrisyoung2005 commented Aug 26, 2026

Uh oh!

chrisyoung2005 commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

chrisyoung2005 commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Evidence

Branch state

Platforms tested

Uh oh!

drowchaeshew Jul 27, 2026

Choose a reason for hiding this comment

tl;dr: Daemon url changes not only happens on browser_navigate().

Suggestion?

Uh oh!

chrisyoung2005 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

drowchaeshew Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Solution?

Uh oh!

chrisyoung2005 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

teknium1 commented Jul 30, 2026

Problems

Suggested changes

Uh oh!

chrisyoung2005 commented Jul 30, 2026

Uh oh!

Enough1122 commented Aug 15, 2026

Uh oh!

chrisyoung2005 commented Aug 18, 2026

Uh oh!

chrisyoung2005 commented Aug 26, 2026

Uh oh!

chrisyoung2005 commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chrisyoung2005 commented Jul 26, 2026 •

edited

Loading

tl;dr: Daemon url changes not only happens on `browser_navigate()`.

drowchaeshew Jul 27, 2026 •

edited

Loading