diff --git a/hermes_cli/doctor_live.py b/hermes_cli/doctor_live.py index 2371101210..5c990b2af1 100644 --- a/hermes_cli/doctor_live.py +++ b/hermes_cli/doctor_live.py @@ -93,12 +93,21 @@ def _browser_available() -> bool: pass # agent-browser resolves lazily via npx on the default install (#43564), # invisible to the PATH/node_modules probes above. Mirror the rung - # hermes_cli.doctor uses so this probe can't diverge from it. + # hermes_cli.doctor uses so this probe can't diverge from it, including + # the Termux carve-out (bare npx is too fragile to advertise as ready + # there — see check_browser_requirements). try: - from tools.browser_tool import _find_agent_browser, _is_npx_agent_browser_sentinel - return _is_npx_agent_browser_sentinel(_find_agent_browser(validate=False)) + from tools.browser_tool import ( + _find_agent_browser, + _is_npx_agent_browser_sentinel, + _requires_real_termux_browser_install, + ) + browser_cmd = _find_agent_browser(validate=False) except Exception: return False + if not _is_npx_agent_browser_sentinel(browser_cmd): + return False + return not _requires_real_termux_browser_install(browser_cmd) def _launch_browser_probe(timeout: float) -> tuple: diff --git a/tests/hermes_cli/test_dep_ensure.py b/tests/hermes_cli/test_dep_ensure.py index 30e645e331..17b37d1f5d 100644 --- a/tests/hermes_cli/test_dep_ensure.py +++ b/tests/hermes_cli/test_dep_ensure.py @@ -58,6 +58,46 @@ def test_has_npx_agent_browser_false_when_nothing_resolves(): assert _has_npx_agent_browser() is False +def test_find_agent_browser_lazy_install_cycle_terminates(monkeypatch): + """tools.browser_tool._find_agent_browser's "nothing found" branch calls + ensure_dependency("browser"), whose "browser" check now includes + _has_npx_agent_browser() -> _find_agent_browser(validate=False) again. + That nested call must NOT be able to trigger another ensure_dependency + call (only validate=True does that) — verifying the cycle is bounded to + one extra rescan, not unbounded recursion, using the real functions on + both sides rather than mocking the cycle away.""" + import shutil + import tools.browser_tool as bt + from hermes_cli import dep_ensure + + monkeypatch.setattr(bt, "_cached_agent_browser", None) + monkeypatch.setattr(bt, "_agent_browser_resolved", False) + monkeypatch.setattr(shutil, "which", lambda *a, **k: None) + monkeypatch.setattr(bt, "_resolve_npx_bin", lambda: None) + monkeypatch.setattr(dep_ensure, "_has_system_browser", lambda: False) + monkeypatch.setattr(dep_ensure, "_has_hermes_agent_browser", lambda: False) + monkeypatch.setattr(dep_ensure, "_find_install_script", lambda *a, **k: (None, None)) + + real_find_agent_browser = bt._find_agent_browser + validate_calls = [] + + def counting_find_agent_browser(*, validate=True): + validate_calls.append(validate) + return real_find_agent_browser(validate=validate) + + monkeypatch.setattr(bt, "_find_agent_browser", counting_find_agent_browser) + + with pytest.raises(FileNotFoundError): + bt._find_agent_browser(validate=True) + + # One outer validate=True call, plus exactly one bounded nested + # validate=False rescan from _has_npx_agent_browser inside + # ensure_dependency's "browser" check — not unbounded recursion, and not + # a second ensure_dependency("browser") call (which would show up as a + # second `True` in this list). + assert validate_calls == [True, False] + + @pytest.mark.windows_only def test_ensure_dependency_uses_powershell_on_windows(tmp_path): """``windows_only``: the assertion is that we shell out to a real diff --git a/tests/hermes_cli/test_doctor_live.py b/tests/hermes_cli/test_doctor_live.py index 64a1dd9fa7..87b3ba536b 100644 --- a/tests/hermes_cli/test_doctor_live.py +++ b/tests/hermes_cli/test_doctor_live.py @@ -219,6 +219,18 @@ class TestBrowserAvailableNpxRung: assert _real_browser_available() is False + def test_false_on_termux_local_bare_npx(self, monkeypatch, tmp_path): + """On Termux in local mode the bare npx fallback is too fragile to + advertise as ready — must not diverge from dep_ensure/nous_subscription's + same carve-out.""" + self._block_path_and_node_modules_checks(monkeypatch, tmp_path) + import tools.browser_tool as bt + + monkeypatch.setattr(bt, "_find_agent_browser", lambda **_kw: "npx agent-browser") + monkeypatch.setattr(bt, "_requires_real_termux_browser_install", lambda cmd: True) + + assert _real_browser_available() is False + class TestFailureIsolation: def test_one_probe_raising_does_not_stop_others(self, monkeypatch): diff --git a/tests/tools/test_browser_homebrew_paths.py b/tests/tools/test_browser_homebrew_paths.py index 02751433de..ea28150376 100644 --- a/tests/tools/test_browser_homebrew_paths.py +++ b/tests/tools/test_browser_homebrew_paths.py @@ -13,6 +13,8 @@ from tools.browser_tool import ( _discover_homebrew_node_dirs, _find_agent_browser, _run_browser_command, + _run_chrome_fallback_command, + AGENT_BROWSER_NPX_SPEC, _SANE_PATH, check_browser_requirements, ) @@ -349,6 +351,59 @@ class TestRunBrowserCommandPathConstruction: ] + def test_npx_sentinel_resolves_via_resolve_npx_bin_with_pinned_spec(self, tmp_path): + """When _find_agent_browser resolves the npx sentinel, the cmd prefix + must come from _resolve_npx_bin() (not a bare shutil.which("npx"), which + could let a broken system npx shadow a healthy Hermes-managed one) and + use the pinned agent-browser npx spec, not a bare "agent-browser".""" + captured_cmd = None + + mock_proc = MagicMock() + mock_proc.returncode = 0 + mock_proc.wait.return_value = 0 + + def capture_popen(cmd, **kwargs): + nonlocal captured_cmd + captured_cmd = cmd + return mock_proc + + fake_session = { + "session_name": "test-session", + "session_id": "test-id", + "cdp_url": None, + } + fake_json = json.dumps({"success": True}) + hermes_home = str(tmp_path / "hermes-home") + + with patch("tools.browser_tool._find_agent_browser", return_value="npx agent-browser"), \ + patch("tools.browser_tool._resolve_npx_bin", return_value="/opt/hermes/node/bin/npx"), \ + patch("tools.browser_tool._chromium_installed", return_value=True), \ + patch("tools.browser_tool._get_session_info", return_value=fake_session), \ + patch("tools.browser_tool._socket_safe_tmpdir", return_value=str(tmp_path)), \ + patch("tools.browser_tool._discover_homebrew_node_dirs", return_value=[]), \ + patch("hermes_constants.Path.home", return_value=tmp_path), \ + patch("subprocess.Popen", side_effect=capture_popen), \ + patch("os.open", return_value=99), \ + patch("os.close"), \ + patch("tools.interrupt.is_interrupted", return_value=False), \ + patch.dict( + os.environ, + { + "PATH": "/usr/bin:/bin", + "HOME": "/home/test", + "HERMES_HOME": hermes_home, + }, + clear=True, + ): + with patch("builtins.open", mock_open(read_data=fake_json)): + _run_browser_command("test-task", "navigate", ["https://example.com"]) + + assert captured_cmd is not None + assert captured_cmd[:4] == [ + "/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, + ] + assert captured_cmd[4:8] == ["--session", "test-session", "--json", "navigate"] + def test_subprocess_path_includes_termux_fallback_dirs(self, tmp_path): """Termux fallback dirs should survive browser PATH rebuilding.""" captured_env = {} @@ -397,3 +452,41 @@ class TestRunBrowserCommandPathConstruction: result_path = captured_env.get("PATH", "") assert "/data/data/com.termux/files/usr/bin" in result_path assert "/data/data/com.termux/files/usr/sbin" in result_path + + +class TestRunChromeFallbackCommandNpxResolution: + """_run_chrome_fallback_command builds its own npx cmd prefix independently + of _run_browser_command's — it must resolve npx the same way (via + _resolve_npx_bin(), not a bare shutil.which("npx")) and use the pinned + agent-browser npx spec.""" + + def test_npx_sentinel_resolves_via_resolve_npx_bin_with_pinned_spec(self, tmp_path): + captured_cmds = [] + + mock_proc = MagicMock() + mock_proc.returncode = 0 + mock_proc.wait.return_value = 0 + + def capture_popen(cmd, **kwargs): + captured_cmds.append(cmd) + return mock_proc + + url_result = {"success": True, "data": {"result": "https://example.com"}} + + with patch("tools.browser_tool._run_browser_command", return_value=url_result), \ + patch("tools.browser_tool._find_agent_browser", return_value="npx agent-browser"), \ + patch("tools.browser_tool._resolve_npx_bin", return_value="/opt/hermes/node/bin/npx"), \ + patch("tools.browser_tool._chromium_installed", return_value=True), \ + patch("tools.browser_tool._running_in_docker", return_value=False), \ + patch("tools.browser_tool._socket_safe_tmpdir", return_value=str(tmp_path)), \ + patch("subprocess.Popen", side_effect=capture_popen): + _run_chrome_fallback_command("test-task", "navigate", ["https://example.com"], timeout=10) + + assert captured_cmds, "expected at least one Popen call for the chrome-fallback session" + first_cmd = captured_cmds[0] + assert first_cmd[:4] == [ + "/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, + ] + assert first_cmd[4] == "--engine" and first_cmd[5] == "chrome" + assert first_cmd[6] == "--session" and first_cmd[7].startswith("h_cfb_") + assert first_cmd[8] == "--json"