From 679a27498d6be6047df096551ebc5691bbfebb89 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 28 Apr 2026 22:56:23 -0500 Subject: [PATCH] fix(browser): address Copilot round-3 on /browser connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Gate `browser.progress` emit on truthy `session_id`. The TUI prints `messages` from the response when there's no session, so emitting events too would double-render. Now: with a session → events stream live; without one → bundled messages only. * Resolve `system = platform.system()` once in `_browser_connect` and thread it through `try_launch_chrome_debug` and `_failure_messages` → `manual_chrome_debug_command`, so the generated hint is consistent (and tests are deterministic) on any host. * Add `test_browser_manage_connect_no_session_skips_progress_events` to lock in the gating behavior. --- tests/test_tui_gateway_server.py | 45 +++++++++++++++++++++++++++++++- tui_gateway/server.py | 18 ++++++++----- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 49ca6fe0..25e4d410 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -2930,7 +2930,11 @@ def test_browser_manage_connect_default_local_reports_launch_hint(monkeypatch): { "id": "1", "method": "browser.manage", - "params": {"action": "connect", "url": "http://localhost:9222"}, + "params": { + "action": "connect", + "session_id": "sess-1", + "url": "http://localhost:9222", + }, } ) @@ -2952,6 +2956,45 @@ def test_browser_manage_connect_default_local_reports_launch_hint(monkeypatch): assert progress == resp["result"]["messages"] +def test_browser_manage_connect_no_session_skips_progress_events(monkeypatch): + """Without a session_id the TUI prints messages from the response; + emitting ``browser.progress`` events would double-render. Gate the + emit so callers without a session see the bundled list only.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + emitted: list[tuple[str, dict]] = [] + monkeypatch.setattr( + server, + "_emit", + lambda evt, sid, payload=None: emitted.append((evt, payload or {})), + ) + 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=False) + with ( + patch( + "hermes_cli.browser_connect.try_launch_chrome_debug", return_value=False + ), + patch( + "hermes_cli.browser_connect.get_chrome_debug_candidates", + return_value=[], + ), + ): + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://localhost:9222"}, + } + ) + + assert resp["result"]["connected"] is False + assert resp["result"]["messages"] # bundled list still populated + assert [evt for evt, _ in emitted if evt == "browser.progress"] == [] + + def test_browser_manage_connect_default_local_retries_after_launch(monkeypatch): monkeypatch.delenv("BROWSER_CDP_URL", raising=False) monkeypatch.setattr(server.time, "sleep", lambda _seconds: None) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 88e5e3c5..8ee81bdd 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -4800,10 +4800,10 @@ def _normalize_cdp_url(parsed) -> str: return parsed._replace(path="", params="", query="", fragment="").geturl() -def _failure_messages(url: str, port: int) -> list[str]: +def _failure_messages(url: str, port: int, system: str) -> list[str]: from hermes_cli.browser_connect import manual_chrome_debug_command - command = manual_chrome_debug_command(port) + command = manual_chrome_debug_command(port, system) hint = ( ["Start Chrome with remote debugging, then retry /browser connect:", command] if command @@ -4837,17 +4837,24 @@ def _(rid, params: dict) -> dict: def _browser_connect(rid, params: dict) -> dict: + import platform + from hermes_cli.browser_connect import DEFAULT_BROWSER_CDP_URL from tools.browser_tool import cleanup_all_browsers from urllib.parse import urlparse url = params.get("url", DEFAULT_BROWSER_CDP_URL) sid = params.get("session_id") or "" + system = platform.system() messages: list[str] = [] def announce(message: str, *, level: str = "info") -> None: messages.append(message) - _emit("browser.progress", sid, {"message": message, "level": level}) + # Without a session id the TUI prints `messages` from the + # response; emitting an event would double-render. Only stream + # progress when there's a real session to scope it to. + if sid: + _emit("browser.progress", sid, {"message": message, "level": level}) parsed = urlparse(url if "://" in url else f"http://{url}") if parsed.scheme not in {"http", "https", "ws", "wss"}: @@ -4883,12 +4890,11 @@ def _browser_connect(rid, params: dict) -> dict: ok = any(_http_ok(p, timeout=2.0) for p in probes) if not ok and _is_default_local_cdp(parsed): - import platform from hermes_cli.browser_connect import try_launch_chrome_debug announce("Chrome isn't running with remote debugging — attempting to launch...") - if try_launch_chrome_debug(port, platform.system()): + if try_launch_chrome_debug(port, system): for _ in range(20): time.sleep(0.5) if any(_http_ok(p, timeout=1.0) for p in probes): @@ -4898,7 +4904,7 @@ def _browser_connect(rid, params: dict) -> dict: if ok: announce(f"Chrome launched and listening on port {port}") else: - for line in _failure_messages(url, port)[1:]: + for line in _failure_messages(url, port, system)[1:]: announce(line, level="error") return _ok(rid, {"connected": False, "url": url, "messages": messages}) elif not ok: