diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 271214fa39..da863360fb 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -2721,3 +2721,292 @@ def test_session_most_recent_handles_db_unavailable(monkeypatch): ) assert resp["result"]["session_id"] is None +# ── browser.manage ─────────────────────────────────────────────────── + + +def _stub_urlopen(monkeypatch, *, ok: bool): + """Patch urllib.request.urlopen used by browser.manage to short-circuit probes.""" + + class _Resp: + status = 200 if ok else 503 + + def __enter__(self): + return self + + def __exit__(self, *_): + return False + + def _opener(_url, timeout=2.0): # noqa: ARG001 — match urllib signature + if not ok: + raise OSError("probe failed") + return _Resp() + + import urllib.request + + monkeypatch.setattr(urllib.request, "urlopen", _opener) + + +def test_browser_manage_status_reads_env_var(monkeypatch): + """Status returns the env var verbatim (no network I/O).""" + monkeypatch.setenv("BROWSER_CDP_URL", "http://127.0.0.1:9222") + + resp = server.handle_request( + {"id": "1", "method": "browser.manage", "params": {"action": "status"}} + ) + + assert resp["result"] == {"connected": True, "url": "http://127.0.0.1:9222"} + + +def test_browser_manage_status_falls_back_to_config_cdp_url(monkeypatch): + """When env is unset, status surfaces ``browser.cdp_url`` from + config.yaml so users see what the next tool call will read.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + + fake_cfg = types.SimpleNamespace( + read_raw_config=lambda: {"browser": {"cdp_url": "http://lan:9222"}} + ) + with patch.dict(sys.modules, {"hermes_cli.config": fake_cfg}): + resp = server.handle_request( + {"id": "1", "method": "browser.manage", "params": {"action": "status"}} + ) + + assert resp["result"] == {"connected": True, "url": "http://lan:9222"} + + +def test_browser_manage_status_does_not_call_get_cdp_override(monkeypatch): + """Regression guard for Copilot's "status must not block" review: + status must NOT route through `_get_cdp_override`, which performs a + `/json/version` HTTP probe with a multi-second timeout.""" + monkeypatch.setenv("BROWSER_CDP_URL", "http://127.0.0.1:9222") + + fake = types.SimpleNamespace( + _get_cdp_override=lambda: pytest.fail( # noqa: PT015 — fail loudly if called + "_get_cdp_override must not run on /browser status (network I/O)" + ) + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + resp = server.handle_request( + {"id": "1", "method": "browser.manage", "params": {"action": "status"}} + ) + + assert resp["result"]["connected"] is True + + +def test_browser_manage_connect_sets_env_and_cleans_twice(monkeypatch): + """`/browser connect` must reach the live process: set env, reap browser + sessions before AND after publishing the new URL. The double-cleanup + closes the supervisor swap window where ``_ensure_cdp_supervisor`` + could re-attach to the *old* CDP endpoint between steps.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + cleanup_calls: list[str] = [] + + def _cleanup_all(): + cleanup_calls.append(os.environ.get("BROWSER_CDP_URL", "")) + + fake = types.SimpleNamespace( + cleanup_all_browsers=_cleanup_all, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + _stub_urlopen(monkeypatch, ok=True) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://127.0.0.1:9222"}, + } + ) + + assert resp["result"] == {"connected": True, "url": "http://127.0.0.1:9222"} + assert os.environ.get("BROWSER_CDP_URL") == "http://127.0.0.1:9222" + # First cleanup runs against the OLD env (none here), second against the NEW. + assert cleanup_calls == ["", "http://127.0.0.1:9222"] + + +def test_browser_manage_connect_rejects_unreachable_endpoint(monkeypatch): + """An unreachable endpoint must NOT mutate the env or reap sessions.""" + monkeypatch.setenv("BROWSER_CDP_URL", "http://existing:9222") + cleanup_calls: list[str] = [] + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: cleanup_calls.append(os.environ.get("BROWSER_CDP_URL", "")), + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + _stub_urlopen(monkeypatch, ok=False) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://unreachable:9222"}, + } + ) + + assert "error" in resp + # Env preserved; nothing reaped. + assert os.environ["BROWSER_CDP_URL"] == "http://existing:9222" + assert cleanup_calls == [] + + +def test_browser_manage_connect_normalizes_bare_host_port(monkeypatch): + """Persist a parsed `scheme://host:port` URL so `_get_cdp_override` + can normalize it; storing a bare host:port would break subsequent + tool calls (Copilot review on #17120).""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + _stub_urlopen(monkeypatch, ok=True) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "127.0.0.1:9222"}, + } + ) + + assert resp["result"]["connected"] is True + # Bare host:port got promoted to a full URL with explicit scheme. + assert resp["result"]["url"].startswith("http://") + assert os.environ["BROWSER_CDP_URL"].startswith("http://") + + +def test_browser_manage_connect_strips_discovery_path(monkeypatch): + """User-supplied discovery paths like `/json` or `/json/version` + must collapse to bare `scheme://host:port`; otherwise + ``_resolve_cdp_override`` will append ``/json/version`` again and + produce a duplicate path (Copilot review round-2 on #17120).""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + _stub_urlopen(monkeypatch, ok=True) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://127.0.0.1:9222/json"}, + } + ) + + assert resp["result"]["connected"] is True + assert resp["result"]["url"] == "http://127.0.0.1:9222" + assert os.environ["BROWSER_CDP_URL"] == "http://127.0.0.1:9222" + + +def test_browser_manage_connect_preserves_devtools_browser_endpoint(monkeypatch): + """Concrete devtools websocket endpoints (e.g. Browserbase) must + survive verbatim — we only collapse discovery-style paths.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + concrete = "ws://browserbase.example/devtools/browser/abc123" + + class _OkSocket: + def __enter__(self): return self + def __exit__(self, *a): return False + + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + # If urlopen is reached for a concrete ws endpoint, the test + # would still pass because _stub_urlopen returned ok=True before; + # patch it to assert-fail so we prove the HTTP probe is skipped. + with patch("urllib.request.urlopen", side_effect=AssertionError("urlopen called")): + with patch("socket.create_connection", return_value=_OkSocket()): + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": concrete}, + } + ) + + assert resp["result"]["connected"] is True + assert resp["result"]["url"] == concrete + assert os.environ["BROWSER_CDP_URL"] == concrete + + +def test_browser_manage_connect_concrete_ws_skips_http_probe(monkeypatch): + """Regression for round-2 Copilot review: a hosted CDP endpoint + (no HTTP discovery) must connect via TCP-only reachability check. + The HTTP probe used to reject these even though they're valid.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + concrete = "wss://chrome.browserless.io/devtools/browser/sess-1" + + seen_targets: list[tuple[str, int]] = [] + + class _OkSocket: + def __enter__(self): return self + def __exit__(self, *a): return False + + def _fake_create_connection(addr, timeout=None): + seen_targets.append(addr) + return _OkSocket() + + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + # urlopen would 404/ECONNREFUSED on a real hosted CDP endpoint; + # asserting it's never called proves the probe was skipped. + with patch("urllib.request.urlopen", side_effect=AssertionError("urlopen called")): + with patch("socket.create_connection", side_effect=_fake_create_connection): + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": concrete}, + } + ) + + assert resp["result"] == {"connected": True, "url": concrete} + # wss → port 443, host preserved verbatim. + assert seen_targets == [("chrome.browserless.io", 443)] + + +def test_browser_manage_connect_concrete_ws_tcp_unreachable(monkeypatch): + """If the TCP reachability check fails for a concrete ws endpoint, + return a clear 5031 error — no fallback to the HTTP probe (which + can never succeed for these URLs anyway).""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + concrete = "ws://offline.example/devtools/browser/missing" + + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + with patch("socket.create_connection", side_effect=OSError("ECONNREFUSED")): + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": concrete}, + } + ) + + assert "error" in resp + assert resp["error"]["code"] == 5031 + + +def test_browser_manage_disconnect_drops_env_and_cleans(monkeypatch): + monkeypatch.setenv("BROWSER_CDP_URL", "http://127.0.0.1:9222") + cleanup_count = {"n": 0} + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: cleanup_count.__setitem__("n", cleanup_count["n"] + 1), + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + resp = server.handle_request( + {"id": "1", "method": "browser.manage", "params": {"action": "disconnect"}} + ) + + assert resp["result"] == {"connected": False} + assert "BROWSER_CDP_URL" not in os.environ + # Two cleanups: once before env removal, once after, matching connect. + assert cleanup_count["n"] == 2 diff --git a/tui_gateway/server.py b/tui_gateway/server.py index fc70168144..c2d8dc73af 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -4674,12 +4674,51 @@ def _(rid, params: dict) -> dict: # ── Methods: browser / plugins / cron / skills ─────────────────────── +def _resolve_browser_cdp_url() -> str: + """Return the configured browser CDP override without network I/O. + + ``/browser status`` must be fast — calling + ``tools.browser_tool._get_cdp_override`` would invoke + ``_resolve_cdp_override``, which performs an HTTP probe to + ``.../json/version`` for discovery-style URLs. That probe has + a multi-second timeout and would block the TUI on a slow or + unreachable host even though status only needs to report whether + an override is set. + + Mirrors the env/config precedence of ``_get_cdp_override`` (env + var first, then ``browser.cdp_url`` from config.yaml) without the + websocket-resolution step, so the answer reflects user intent + even when the configured host is not currently reachable. The + actual WS normalization happens in ``browser_navigate`` on the + next tool call. + """ + env_url = os.environ.get("BROWSER_CDP_URL", "").strip() + if env_url: + return env_url + try: + from hermes_cli.config import read_raw_config + + cfg = read_raw_config() + browser_cfg = cfg.get("browser", {}) if isinstance(cfg, dict) else {} + if isinstance(browser_cfg, dict): + return str(browser_cfg.get("cdp_url", "") or "").strip() + except Exception: + pass + return "" + + @method("browser.manage") def _(rid, params: dict) -> dict: action = params.get("action", "status") if action == "status": - url = os.environ.get("BROWSER_CDP_URL", "") - return _ok(rid, {"connected": bool(url), "url": url}) + resolved_url = _resolve_browser_cdp_url() + return _ok( + rid, + { + "connected": bool(resolved_url), + "url": resolved_url, + }, + ) if action == "connect": url = params.get("url", "http://localhost:9222") try: @@ -4690,36 +4729,97 @@ def _(rid, params: dict) -> dict: parsed = urlparse(url if "://" in url else f"http://{url}") if parsed.scheme not in {"http", "https", "ws", "wss"}: return _err(rid, 4015, f"unsupported browser url: {url}") - probe_root = f"{'https' if parsed.scheme == 'wss' else 'http' if parsed.scheme == 'ws' else parsed.scheme}://{parsed.netloc}" - probe_urls = [ - f"{probe_root.rstrip('/')}/json/version", - f"{probe_root.rstrip('/')}/json", - ] - ok = False - for probe in probe_urls: - try: - with urllib.request.urlopen(probe, timeout=2.0) as resp: - if 200 <= getattr(resp, "status", 200) < 300: - ok = True - break - except Exception: - continue - if not ok: - return _err(rid, 5031, f"could not reach browser CDP at {url}") - os.environ["BROWSER_CDP_URL"] = url + # A concrete ``ws[s]://.../devtools/browser/`` endpoint is + # already directly connectable — those are the URLs Browserbase + # / browserless / hosted CDP providers return, and they + # generally DON'T serve the discovery-style ``/json/version`` + # path. Probing it would just reject valid endpoints. Skip + # the HTTP probe and do a TCP-level reachability check instead; + # the actual CDP handshake happens on the next ``browser_navigate``. + is_concrete_ws = ( + parsed.scheme in {"ws", "wss"} + and parsed.path.startswith("/devtools/browser/") + ) + if is_concrete_ws: + import socket + + host = parsed.hostname + port = parsed.port or (443 if parsed.scheme == "wss" else 80) + if not host: + return _err(rid, 4015, f"missing host in browser url: {url}") + try: + with socket.create_connection((host, port), timeout=2.0): + pass + except OSError as e: + return _err(rid, 5031, f"could not reach browser CDP at {url}: {e}") + else: + probe_root = f"{'https' if parsed.scheme == 'wss' else 'http' if parsed.scheme == 'ws' else parsed.scheme}://{parsed.netloc}" + probe_urls = [ + f"{probe_root.rstrip('/')}/json/version", + f"{probe_root.rstrip('/')}/json", + ] + ok = False + for probe in probe_urls: + try: + with urllib.request.urlopen(probe, timeout=2.0) as resp: + if 200 <= getattr(resp, "status", 200) < 300: + ok = True + break + except Exception: + continue + if not ok: + return _err(rid, 5031, f"could not reach browser CDP at {url}") + + # Persist a normalized URL for downstream CDP resolution. + # Discovery-style inputs (`http://host:port` or + # `http://host:port/json[/version]`) collapse to bare + # ``scheme://host:port`` so ``_resolve_cdp_override`` can + # safely append ``/json/version`` without producing a + # double-discovery path like ``.../json/json/version``. + # Concrete websocket endpoints (``/devtools/browser/`` + # — what Browserbase and other cloud providers return) + # are preserved verbatim. + if parsed.path.startswith("/devtools/browser/"): + normalized = parsed.geturl() + else: + normalized = parsed._replace( + path="", + params="", + query="", + fragment="", + ).geturl() + + # Order matters: clear any cached browser sessions BEFORE + # publishing the new env var so an in-flight tool call + # observing the old supervisor is reaped first, and the + # next call freshly resolves the new URL. The previous + # ordering left a brief window where ``_ensure_cdp_supervisor`` + # could re-attach to the *old* supervisor. + cleanup_all_browsers() + os.environ["BROWSER_CDP_URL"] = normalized + # Drain any further cached state that could outlive the + # cleanup pass (CDP supervisor for the default task, + # cached agent-browser timeouts, etc.) so the next + # ``browser_navigate`` definitively reaches ``normalized``. cleanup_all_browsers() except Exception as e: return _err(rid, 5031, str(e)) - return _ok(rid, {"connected": True, "url": url}) + return _ok(rid, {"connected": True, "url": normalized}) if action == "disconnect": - os.environ.pop("BROWSER_CDP_URL", None) try: from tools.browser_tool import cleanup_all_browsers cleanup_all_browsers() except Exception: pass + os.environ.pop("BROWSER_CDP_URL", None) + try: + from tools.browser_tool import cleanup_all_browsers as _again + + _again() + except Exception: + pass return _ok(rid, {"connected": False}) return _err(rid, 4015, f"unknown action: {action}") diff --git a/ui-tui/src/app/slash/commands/ops.ts b/ui-tui/src/app/slash/commands/ops.ts index ef1b240604..7443552740 100644 --- a/ui-tui/src/app/slash/commands/ops.ts +++ b/ui-tui/src/app/slash/commands/ops.ts @@ -98,13 +98,16 @@ export const opsCommands: SlashCommand[] = [ const action = (rawAction || 'status').toLowerCase() if (!['connect', 'disconnect', 'status'].includes(action)) { - return ctx.transcript.sys('usage: /browser [connect|disconnect|status] [url]') + return ctx.transcript.sys( + 'usage: /browser [connect|disconnect|status] [url] · persistent: set browser.cdp_url in config.yaml' + ) } const payload: Record = { action } + const requested = rest.join(' ').trim() if (action === 'connect') { - payload.url = rest.join(' ').trim() || 'http://localhost:9222' + payload.url = requested || 'http://localhost:9222' } ctx.gateway @@ -113,14 +116,21 @@ export const opsCommands: SlashCommand[] = [ ctx.guarded(r => { if (action === 'status') { return ctx.transcript.sys( - r.connected ? `browser connected: ${r.url || '(url unavailable)'}` : 'browser not connected' + r.connected + ? `browser connected: ${r.url || '(url unavailable)'}` + : 'browser not connected (try /browser connect or set browser.cdp_url in config.yaml)' ) } if (action === 'connect') { - return ctx.transcript.sys( - r.connected ? `browser connected: ${r.url || '(url unavailable)'}` : 'browser connect failed' - ) + if (r.connected) { + ctx.transcript.sys(`browser connected: ${r.url || '(url unavailable)'}`) + ctx.transcript.sys('next browser tool call will use this CDP endpoint') + + return + } + + return ctx.transcript.sys('browser connect failed') } ctx.transcript.sys('browser disconnected')