diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 9fd241630ae0..c5581045ec5f 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -1848,7 +1848,7 @@ def run_doctor(args): _chromium_installed, _is_camofox_mode, _get_cloud_provider, - _get_cdp_override, + _get_cdp_override_raw, _using_lightpanda_engine, ) except Exception: @@ -1861,7 +1861,7 @@ def run_doctor(args): # Lightpanda all bypass the local Chromium requirement. skip_chromium_check = ( _is_camofox_mode() - or bool(_get_cdp_override()) + or bool(_get_cdp_override_raw()) or _get_cloud_provider() is not None or _using_lightpanda_engine() ) diff --git a/tests/tools/test_browser_cdp_tool.py b/tests/tools/test_browser_cdp_tool.py index 0c0b16e9b198..19ef3c24b730 100644 --- a/tests/tools/test_browser_cdp_tool.py +++ b/tests/tools/test_browser_cdp_tool.py @@ -561,21 +561,36 @@ def test_check_fn_false_when_no_cdp_url(monkeypatch): import tools.browser_tool as bt monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) - monkeypatch.setattr(bt, "_get_cdp_override", lambda: "") + monkeypatch.setattr(bt, "_get_cdp_override_raw", lambda: "") assert browser_cdp_tool._browser_cdp_check() is False def test_check_fn_true_when_cdp_url_set(monkeypatch): - """Gate opens as soon as a CDP URL is resolvable.""" + """Gate opens as soon as a CDP URL is configured (no network resolution).""" import tools.browser_tool as bt monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) monkeypatch.setattr( - bt, "_get_cdp_override", lambda: "ws://localhost:9222/devtools/browser/x" + bt, "_get_cdp_override_raw", lambda: "ws://localhost:9222/devtools/browser/x" ) assert browser_cdp_tool._browser_cdp_check() is True +def test_check_fn_does_not_probe_network(monkeypatch): + """The availability gate must never hit the network: a stale/unreachable + configured endpoint used to cost multiple blocking HTTP probes at every + CLI/Desktop startup (tool-schema assembly), stalling launch by 10+ s.""" + import tools.browser_tool as bt + + def _boom(*a, **k): # pragma: no cover — the assertion is that it's unused + raise AssertionError("check_fn must not perform network I/O") + + monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) + monkeypatch.setattr(bt.requests, "get", _boom) + monkeypatch.setenv("BROWSER_CDP_URL", "http://127.0.0.1:9222") + assert browser_cdp_tool._browser_cdp_check() is True + + def test_check_fn_false_when_browser_requirements_fail(monkeypatch): """Even with a CDP URL, gate closes if the overall browser toolset is unavailable (e.g. agent-browser not installed).""" @@ -583,6 +598,6 @@ def test_check_fn_false_when_browser_requirements_fail(monkeypatch): monkeypatch.setattr(bt, "check_browser_requirements", lambda: False) monkeypatch.setattr( - bt, "_get_cdp_override", lambda: "ws://localhost:9222/devtools/browser/x" + bt, "_get_cdp_override_raw", lambda: "ws://localhost:9222/devtools/browser/x" ) assert browser_cdp_tool._browser_cdp_check() is False diff --git a/tests/tools/test_browser_hybrid_routing.py b/tests/tools/test_browser_hybrid_routing.py index 667ec22a7fbb..9b883ffcd491 100644 --- a/tests/tools/test_browser_hybrid_routing.py +++ b/tests/tools/test_browser_hybrid_routing.py @@ -91,7 +91,7 @@ class TestNavigationSessionKey: def test_cdp_override_stays_on_bare_task_id(self, monkeypatch): """A user-supplied CDP endpoint owns the whole session — no hybrid.""" monkeypatch.setattr(browser_tool, "_get_cloud_provider", lambda: Mock()) - monkeypatch.setattr(browser_tool, "_get_cdp_override", lambda: "ws://localhost:9222") + monkeypatch.setattr(browser_tool, "_get_cdp_override_raw", lambda: "ws://localhost:9222") key = browser_tool._navigation_session_key("default", "http://localhost:3000/") assert key == "default" diff --git a/tests/tools/test_browser_lightpanda.py b/tests/tools/test_browser_lightpanda.py index dabfc5d1bd70..d1660b5da8a9 100644 --- a/tests/tools/test_browser_lightpanda.py +++ b/tests/tools/test_browser_lightpanda.py @@ -127,7 +127,7 @@ class TestShouldInjectEngine: def test_no_inject_with_cdp_override(self): from tools.browser_tool import _should_inject_engine with patch("tools.browser_tool._is_camofox_mode", return_value=False), \ - patch("tools.browser_tool._get_cdp_override", return_value="ws://localhost:9222"): + patch("tools.browser_tool._get_cdp_override_raw", return_value="ws://localhost:9222"): assert _should_inject_engine("lightpanda") is False def test_no_inject_with_cloud_provider(self): diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index 2df9a1660f23..eccd8f8fc1c5 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -653,7 +653,7 @@ def _browser_cdp_check() -> bool: """ try: from tools.browser_tool import ( # type: ignore[import-not-found] - _get_cdp_override, + _get_cdp_override_raw, check_browser_requirements, ) except ImportError as exc: # pragma: no cover — defensive @@ -661,7 +661,10 @@ def _browser_cdp_check() -> bool: return False if not check_browser_requirements(): return False - return bool(_get_cdp_override()) + # Raw (no-I/O) gate: check_fns run during tool-schema assembly at every + # startup; resolving the endpoint over HTTP here would block launch when + # the configured endpoint is stale/unreachable. + return bool(_get_cdp_override_raw()) registry.register( diff --git a/tools/browser_tool.py b/tools/browser_tool.py index c8cf75157008..7a712c875657 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -457,6 +457,45 @@ def _resolve_cdp_override(cdp_url: str) -> str: return raw +def _get_cdp_override_raw() -> str: + """Return the *configured* CDP override without any network I/O. + + Precedence is: + 1. ``BROWSER_CDP_URL`` env var (live override from ``/browser connect``) + 2. ``browser.cdp_url`` in config.yaml (persistent config) + + This is the availability-check variant: callers that only need to know + *whether* a CDP override is configured (tool ``check_fn`` gates, + ``_is_local_mode`` / ``_is_local_backend`` routing decisions, + ``hermes doctor``) MUST use this instead of :func:`_get_cdp_override`. + + Rationale: ``_get_cdp_override`` resolves the endpoint over HTTP + (``/json/version`` discovery, 10s timeout). Tool-schema assembly runs at + every CLI/Desktop startup and probes several browser-family check_fns; + when a *stale* ``browser.cdp_url`` points at a dead endpoint (the debug + Chrome it referenced is long gone), each check blocked on a failing + socket connect and startup stalled for 10+ seconds before the banner — + with no error, just mystery slowness. Same principle as the existing + "do not execute ``agent-browser --version`` here" rule in + ``check_browser_requirements``: no side effects during schema build. + """ + env_override = os.environ.get("BROWSER_CDP_URL", "").strip() + if env_override: + return env_override + + try: + from hermes_cli.config import read_raw_config + + cfg = read_raw_config() + browser_cfg = cfg.get("browser", {}) + if isinstance(browser_cfg, dict): + return str(browser_cfg.get("cdp_url", "") or "").strip() + except Exception as e: + logger.debug("Could not read browser.cdp_url from config: %s", e) + + return "" + + def _get_cdp_override() -> str: """Return a normalized CDP URL override, or empty string. @@ -467,22 +506,16 @@ def _get_cdp_override() -> str: When either is set, we skip both Browserbase and the local headless launcher and connect directly to the supplied Chrome DevTools Protocol endpoint. + + NOTE: resolution may perform an HTTP ``/json/version`` discovery request. + Only call this on paths that are about to *connect* (session creation, + supervisor attach). Pure is-it-configured gates must use + :func:`_get_cdp_override_raw`. """ - env_override = os.environ.get("BROWSER_CDP_URL", "").strip() - if env_override: - return _resolve_cdp_override(env_override) - - try: - from hermes_cli.config import read_raw_config - - cfg = read_raw_config() - browser_cfg = cfg.get("browser", {}) - if isinstance(browser_cfg, dict): - return _resolve_cdp_override(str(browser_cfg.get("cdp_url", "") or "")) - except Exception as e: - logger.debug("Could not read browser.cdp_url from config: %s", e) - - return "" + raw = _get_cdp_override_raw() + if not raw: + return "" + return _resolve_cdp_override(raw) def _get_dialog_policy_config() -> Tuple[str, float]: @@ -789,7 +822,7 @@ def _termux_browser_install_error() -> str: def _is_local_mode() -> bool: """Return True when the browser tool will use a local browser backend.""" - if _get_cdp_override(): + if _get_cdp_override_raw(): return False return _get_cloud_provider() is None @@ -821,7 +854,7 @@ def _is_local_backend() -> bool: # config (both via _get_cdp_override(), and both now suppress camofox in # browser_camofox.py). _is_local_mode() already treats any CDP override as # non-local; keep the two helpers in agreement. - if _get_cdp_override(): + if _get_cdp_override_raw(): return False if _is_camofox_mode(): return True @@ -1313,7 +1346,7 @@ def _navigation_session_key(task_id: str, url: str) -> str: """ if task_id is None: task_id = "default" - if _get_cdp_override(): + if _get_cdp_override_raw(): return task_id if _is_camofox_mode(): return task_id @@ -4740,7 +4773,9 @@ def check_browser_requirements() -> bool: # CDP override mode can connect to an existing remote/local browser endpoint # without requiring the local agent-browser binary on PATH. - if _get_cdp_override(): + # Raw (no-I/O) check: this runs during tool-schema assembly at startup, + # where a stale endpoint must not cost a blocking HTTP probe. + if _get_cdp_override_raw(): return True # The agent-browser CLI is required for local launch and cloud-provider flows.