diff --git a/cli.py b/cli.py index a017b97e6c..d0bf6e794a 100644 --- a/cli.py +++ b/cli.py @@ -6569,6 +6569,19 @@ class HermesCLI: connect_parts = cmd.strip().split(None, 2) # ["/browser", "connect", "ws://..."] cdp_url = connect_parts[2].strip() if len(connect_parts) > 2 else _DEFAULT_CDP parsed_cdp = urlparse(cdp_url if "://" in cdp_url else f"http://{cdp_url}") + try: + _port = parsed_cdp.port or (443 if parsed_cdp.scheme in {"https", "wss"} else 80) + except ValueError: + print() + print(f" ⚠ Invalid port in browser url: {cdp_url}") + print() + return + if not parsed_cdp.hostname: + print() + print(f" ⚠ Missing host in browser url: {cdp_url}") + print() + return + _host = parsed_cdp.hostname if parsed_cdp.path.startswith("/devtools/browser/"): cdp_url = parsed_cdp.geturl() else: @@ -6588,10 +6601,6 @@ class HermesCLI: print() - # Extract port for connectivity checks - _host = parsed_cdp.hostname or "127.0.0.1" - _port = parsed_cdp.port or (443 if parsed_cdp.scheme in {"https", "wss"} else 80) - # Check if Chrome is already listening on the debug port import socket _already_open = False diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index b69b8bbfb8..89c9d2c652 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -97,7 +97,8 @@ def manual_chrome_debug_command(port: int = DEFAULT_BROWSER_CDP_PORT, system: st candidates = get_chrome_debug_candidates(system) if candidates: - return shlex.join([candidates[0], *_chrome_debug_args(port)]) + argv = [candidates[0], *_chrome_debug_args(port)] + return subprocess.list2cmdline(argv) if system == "Windows" else shlex.join(argv) if system == "Darwin": data_dir = chrome_debug_data_dir() @@ -109,8 +110,18 @@ def manual_chrome_debug_command(port: int = DEFAULT_BROWSER_CDP_PORT, system: st return None +def _detach_kwargs(system: str) -> dict: + if system != "Windows": + return {"start_new_session": True} + flags = getattr(subprocess, "DETACHED_PROCESS", 0) | getattr( + subprocess, "CREATE_NEW_PROCESS_GROUP", 0 + ) + return {"creationflags": flags} if flags else {} + + def try_launch_chrome_debug(port: int = DEFAULT_BROWSER_CDP_PORT, system: str | None = None) -> bool: - candidates = get_chrome_debug_candidates(system or platform.system()) + system = system or platform.system() + candidates = get_chrome_debug_candidates(system) if not candidates: return False @@ -120,7 +131,7 @@ def try_launch_chrome_debug(port: int = DEFAULT_BROWSER_CDP_PORT, system: str | [candidates[0], *_chrome_debug_args(port)], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - start_new_session=True, + **_detach_kwargs(system), ) return True except Exception: diff --git a/tests/cli/test_cli_browser_connect.py b/tests/cli/test_cli_browser_connect.py index 9805297b38..cf9471d584 100644 --- a/tests/cli/test_cli_browser_connect.py +++ b/tests/cli/test_cli_browser_connect.py @@ -1,6 +1,7 @@ """Tests for CLI browser CDP auto-launch helpers.""" import os +import subprocess from unittest.mock import patch from cli import HermesCLI @@ -33,7 +34,13 @@ class TestChromeDebugLaunch: assert HermesCLI._try_launch_chrome_debug(9333, "Windows") is True _assert_chrome_debug_cmd(captured["cmd"], r"C:\Chrome\chrome.exe", 9333) - assert captured["kwargs"]["start_new_session"] is True + # Windows uses creationflags (POSIX-only start_new_session would raise). + assert "start_new_session" not in captured["kwargs"] + flags = captured["kwargs"].get("creationflags", 0) + expected = getattr(subprocess, "DETACHED_PROCESS", 0) | getattr( + subprocess, "CREATE_NEW_PROCESS_GROUP", 0 + ) + assert flags == expected def test_windows_launch_falls_back_to_common_install_dirs(self, monkeypatch): captured = {} @@ -73,8 +80,21 @@ class TestChromeDebugLaunch: command = manual_chrome_debug_command(9222, "Linux") assert command is not None + # Linux/WSL uses POSIX shell quoting (single quotes around paths with spaces). assert command.startswith(f"'{chrome}' --remote-debugging-port=9222") + def test_manual_command_uses_windows_quoting_on_windows(self): + chrome = r"C:\Program Files\Google\Chrome\Application\chrome.exe" + + with patch("hermes_cli.browser_connect.shutil.which", side_effect=lambda name: chrome if name == "chrome.exe" else None), \ + patch("hermes_cli.browser_connect.os.path.isfile", side_effect=lambda path: path == chrome): + command = manual_chrome_debug_command(9222, "Windows") + + assert command is not None + # Windows uses cmd.exe-compatible quoting via subprocess.list2cmdline. + assert command.startswith(f'"{chrome}" --remote-debugging-port=9222') + assert "'" not in command + def test_manual_command_returns_none_when_linux_browser_missing(self): with patch("hermes_cli.browser_connect.shutil.which", return_value=None), \ patch("hermes_cli.browser_connect.os.path.isfile", return_value=False): diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 3be6919087..49ca6fe097 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -3111,6 +3111,69 @@ def test_browser_manage_connect_preserves_devtools_browser_endpoint(monkeypatch) assert os.environ["BROWSER_CDP_URL"] == concrete +def test_browser_manage_connect_local_devtools_ws_preserves_path(monkeypatch): + """Regression: ``ws://127.0.0.1:9222/devtools/browser/`` is a real + connectable endpoint; default-local normalization must not strip the + ``/devtools/browser/...`` path or it breaks valid local CDP connects.""" + 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://127.0.0.1:9222/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}): + 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_rejects_invalid_port(monkeypatch): + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://localhost:abc"}, + } + ) + + assert resp["error"]["code"] == 4015 + assert "invalid port" in resp["error"]["message"] + assert "BROWSER_CDP_URL" not in os.environ + + +def test_browser_manage_connect_rejects_missing_host(monkeypatch): + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": "http://:9222"}, + } + ) + + assert resp["error"]["code"] == 4015 + assert "missing host" in resp["error"]["message"] + assert "BROWSER_CDP_URL" not in os.environ + + 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. diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 6dd0b74ff7..88e5e3c50b 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -140,6 +140,7 @@ _SLASH_WORKER_TIMEOUT_S = max( # response writes are safe. _LONG_HANDLERS = frozenset( { + "browser.manage", "cli.exec", "session.branch", "session.resume", @@ -4753,10 +4754,23 @@ def _resolve_browser_cdp_url() -> str: def _is_default_local_cdp(parsed) -> bool: + """Match the discovery-style local default; never the concrete WS form. + + A user-supplied ``ws://127.0.0.1:9222/devtools/browser/`` is a + real, connectable endpoint — collapsing it to bare ``http://...:9222`` + would strip the path and break the connect. + """ + try: + port = parsed.port or 80 + except ValueError: + return False + + discovery_path = parsed.path in {"", "/", "/json", "/json/version"} return ( parsed.scheme in {"http", "ws"} and parsed.hostname in {"127.0.0.1", "localhost"} - and (parsed.port or 80) == 9222 + and port == 9222 + and discovery_path ) @@ -4838,12 +4852,19 @@ def _browser_connect(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}") + if not parsed.hostname: + return _err(rid, 4015, f"missing host in browser url: {url}") + try: + port = parsed.port or (443 if parsed.scheme in {"https", "wss"} else 80) + except ValueError: + return _err(rid, 4015, f"invalid port in browser url: {url}") # Always normalize default-local to 127.0.0.1:9222 so downstream # comparisons + messaging match what we'll actually persist. if _is_default_local_cdp(parsed): url = DEFAULT_BROWSER_CDP_URL parsed = urlparse(url) + port = parsed.port or 9222 try: # ws[s]://.../devtools/browser/ endpoints (hosted CDP @@ -4852,9 +4873,6 @@ def _browser_connect(rid, params: dict) -> dict: if parsed.scheme in {"ws", "wss"} and parsed.path.startswith("/devtools/browser/"): import socket - if not parsed.hostname: - return _err(rid, 4015, f"missing host in browser url: {url}") - port = parsed.port or (443 if parsed.scheme == "wss" else 80) try: with socket.create_connection((parsed.hostname, port), timeout=2.0): pass @@ -4868,7 +4886,6 @@ def _browser_connect(rid, params: dict) -> dict: import platform from hermes_cli.browser_connect import try_launch_chrome_debug - port = parsed.port or 9222 announce("Chrome isn't running with remote debugging — attempting to launch...") if try_launch_chrome_debug(port, platform.system()): @@ -4887,7 +4904,7 @@ def _browser_connect(rid, params: dict) -> dict: elif not ok: return _err(rid, 5031, f"could not reach browser CDP at {url}") elif _is_default_local_cdp(parsed): - announce(f"Chrome is already listening on port {parsed.port or 9222}") + announce(f"Chrome is already listening on port {port}") normalized = _normalize_cdp_url(parsed) diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index d747dc82a7..db5e37347a 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -218,6 +218,7 @@ describe('createSlashHandler', () => { url: 'http://127.0.0.1:9222' }) ) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) expect(createSlashHandler(ctx)('/browser connect')).toBe(true) diff --git a/ui-tui/src/app/slash/commands/ops.ts b/ui-tui/src/app/slash/commands/ops.ts index 34d19fc25e..21cd52b341 100644 --- a/ui-tui/src/app/slash/commands/ops.ts +++ b/ui-tui/src/app/slash/commands/ops.ts @@ -115,7 +115,9 @@ export const opsCommands: SlashCommand[] = [ ctx.guarded(r => { // Without a session we can't subscribe to streamed // browser.progress events, so flush the bundled list. - if (!sid) r.messages?.forEach(message => ctx.transcript.sys(message)) + if (!sid) { + r.messages?.forEach(message => ctx.transcript.sys(message)) + } if (action === 'status') { return ctx.transcript.sys(