fix(browser): address Copilot round-3 on /browser connect
* 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.
This commit is contained in:
parent
d1ee4915f3
commit
679a27498d
@ -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)
|
||||
|
||||
@ -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:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user