mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(tui): make /browser connect actually take effect on the live agent (#17120)
* fix(tui): make /browser connect actually take effect on the live agent Reports were that `/browser connect <url>` (and "changes to CDP url don't get picked up") didn't propagate to the live agent in `--tui`, forcing users to fall back to setting `browser.cdp_url` in `config.yaml` and restarting. Tracing the path on current main shows the protocol wiring is already correct — `/browser` is registered in `ui-tui/src/app/slash/commands/ops.ts` and dispatches `browser.manage` through the gateway RPC, NOT the slash worker (covered by the `browser.manage` row in `slashParity.test.ts`). But three real gaps left the experience flaky: 1. `cleanup_all_browsers()` ran AFTER `os.environ["BROWSER_CDP_URL"]` was rewritten. `_ensure_cdp_supervisor(...)` reads the env to resolve its target URL, so a tool call landing in that brief window could re-attach the supervisor to the OLD CDP endpoint just before we reaped sessions, leaving the agent talking to a dead URL. Reorder to clean first, swap env, clean again so the supervisor for the default task is definitively closed. 2. `browser.manage status` reported only the env var, ignoring `browser.cdp_url` from config.yaml. `_get_cdp_override()` (the resolver the agent itself uses) consults both — match it so `/browser status` answers the same question the next `browser_navigate` will see. Closes a stealth bug where users saw "browser not connected" while their CDP URL was perfectly set in config.yaml. 3. `/browser disconnect` only cleared `BROWSER_CDP_URL` and reaped once, leaving the same swap window as connect. Symmetrical double-cleanup here too. Frontend (`ops.ts`): * Echo "next browser tool call will use this CDP endpoint" on success so users see immediate confirmation that the gateway accepted the swap, even before any tool runs. * Mention `browser.cdp_url` in `config.yaml` in the usage hint and the not-connected status line. Persistent config is the correct fix for some terminal-multiplexer / sub-agent flows where env inheritance is unreliable; surfacing it makes that workaround discoverable. Tests (4 new, all hermetic): * `status` returns the resolved URL when only `browser.cdp_url` is set in config.yaml. * `connect` writes env AND cleans before/after, in that order. * `connect` against an unreachable endpoint does NOT mutate env or reap. * `disconnect` removes env and cleans twice. Validation: scripts/run_tests.sh tests/test_tui_gateway_server.py — 94/94 pass. cd ui-tui && npm run type-check — clean; npm test --run — 389/389. * review(copilot): always defer to _get_cdp_override; normalize bare host:port * review(copilot): collapse discovery-style CDP paths so /json/version isn't duplicated * fix(tui): /browser status must not perform CDP discovery I/O Copilot review on PR #17120: previous version routed through `tools.browser_tool._get_cdp_override`, which calls `_resolve_cdp_override` and performs an HTTP probe to /json/version with a multi-second timeout for discovery-style URLs. That blocks the TUI on `/browser status` whenever the configured host is slow or unreachable. Status now reads env-then-config directly with no network I/O. The WS normalization still happens in `browser_navigate` for actual tool calls, so behaviour-on-call is unchanged. * fix(tui): skip /json/version probe for concrete ws://devtools/browser endpoints Round 2 Copilot review on PR #17120: hosted CDP providers (Browserbase, browserless, etc.) return concrete `ws[s]://.../devtools/browser/<id>` URLs which are already directly connectable but don't serve the HTTP discovery path. The previous `/json/version` probe rejected these valid endpoints with 'could not reach browser CDP'. For `ws[s]://...` URLs whose path starts with `/devtools/browser/` we now do a TCP-level reachability check (`socket.create_connection`) instead of the HTTP probe. The actual CDP handshake happens on the next `browser_navigate` call, so we still surface unreachable hosts as 5031 errors — just without the false negatives. Discovery-style URLs (`http://host:port[/json[/version]]`) keep the HTTP probe path unchanged. Updated existing test + added two new ones (TCP-only success, TCP unreachable → 5031).
This commit is contained in:
brooklyn!
GitHub
parent
87d3fa6f1c
commit
15ef11a8b8
3 changed files with 426 additions and 27 deletions
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue