molecule-ai/hermes-agent
35
0
Fork 0
Code Issues 2 Pull Requests 4 Actions 14 Packages Projects Releases Wiki Activity

fix(browser): address Copilot review on /browser connect

Browse Source 
Fixes from Copilot's two passes on PR #17238:

* Validate parsed URL once: reject missing host, invalid port, and
  unsupported scheme up front so malformed inputs (e.g. http://:9222
  or http://localhost:abc) don't fall through to a generic 5031.
* Tighten _is_default_local_cdp to require a discovery-style path so
  ws://127.0.0.1:9222/devtools/browser/<id> is not collapsed to bare
  http://127.0.0.1:9222 (which would lose the path and break the
  connect).
* Move browser.manage into _LONG_HANDLERS so the up-to-10s
  launch-and-retry loop runs on the RPC pool instead of blocking the
  main dispatcher.
* try_launch_chrome_debug uses Windows-appropriate detach kwargs
  (creationflags=DETACHED_PROCESS|CREATE_NEW_PROCESS_GROUP) instead
  of POSIX-only start_new_session=True.
* manual_chrome_debug_command uses subprocess.list2cmdline on
  Windows so the printed instruction is cmd.exe-compatible.
* Mirror host/port validation in cli.py /browser connect so the
  classic CLI never persists an invalid BROWSER_CDP_URL.
This commit is contained in:
Brooklyn Nicholson 2026-04-28 22:41:15 -05:00 committed by Teknium
parent 26816d1f77
commit d1ee4915f3
7 changed files with 138 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
cli.py
View File

@ -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

17
hermes_cli/browser_connect.py
View File

@ -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:

22
tests/cli/test_cli_browser_connect.py
View File

@ -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):

63
tests/test_tui_gateway_server.py
View File

@ -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/<id>`` 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.

29
tui_gateway/server.py
View File

@ -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/<id>`` 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/<id> 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)

1
ui-tui/src/__tests__/createSlashHandler.test.ts
View File

@ -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)

4
ui-tui/src/app/slash/commands/ops.ts
View File

@ -115,7 +115,9 @@ export const opsCommands: SlashCommand[] = [
ctx.guarded<BrowserManageResponse>(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(