diff --git a/hermes_cli/dep_ensure.py b/hermes_cli/dep_ensure.py index 848e402396c..6f0bc950664 100644 --- a/hermes_cli/dep_ensure.py +++ b/hermes_cli/dep_ensure.py @@ -22,12 +22,14 @@ import subprocess import sys from pathlib import Path +from hermes_constants import agent_browser_runnable + _IS_WINDOWS = platform.system() == "Windows" _DEP_CHECKS = { "node": lambda: shutil.which("node") is not None, "browser": lambda: ( - shutil.which("agent-browser") is not None + agent_browser_runnable(shutil.which("agent-browser")) or _has_system_browser() or _has_hermes_agent_browser() ), diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 7aadc58f5f2..7377e2959a2 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -13,6 +13,7 @@ from pathlib import Path from hermes_cli.config import get_project_root, get_hermes_home, get_env_path from hermes_cli.env_loader import load_hermes_dotenv from hermes_constants import display_hermes_home +from hermes_constants import agent_browser_runnable PROJECT_ROOT = get_project_root() HERMES_HOME = get_hermes_home() @@ -1483,12 +1484,21 @@ def run_doctor(args): # Check if agent-browser is installed agent_browser_path = PROJECT_ROOT / "node_modules" / "agent-browser" agent_browser_ok = False + _which_ab = shutil.which("agent-browser") if agent_browser_path.exists(): check_ok("agent-browser (Node.js)", "(browser automation)") agent_browser_ok = True - elif shutil.which("agent-browser"): + elif _which_ab and agent_browser_runnable(_which_ab): check_ok("agent-browser", "(browser automation)") agent_browser_ok = True + elif _which_ab: + # Found on PATH but won't run — almost always a dangling global + # symlink left behind by agent-browser's npm postinstall after a + # `hermes update` wiped node_modules (issue #48521). + check_warn( + "agent-browser found but not runnable", + f"(broken symlink at {_which_ab}? run: npm install)", + ) elif _is_termux(): check_info("agent-browser is not installed (expected in the tested Termux path)") check_info("Install it manually later with: npm install -g agent-browser && agent-browser install") diff --git a/hermes_cli/nous_subscription.py b/hermes_cli/nous_subscription.py index 50dfb97ba2a..f2775e3bbe8 100644 --- a/hermes_cli/nous_subscription.py +++ b/hermes_cli/nous_subscription.py @@ -159,11 +159,17 @@ def _toolset_enabled(config: Dict[str, object], toolset_key: str) -> bool: def _has_agent_browser() -> bool: import shutil - agent_browser_bin = shutil.which("agent-browser") + from hermes_constants import agent_browser_runnable + + # Validate the resolved binary actually runs — a dangling global symlink + # (issue #48521) is reported by ``which`` but fails at exec. Fall through to + # the local node_modules copy, which the validator also checks. + if agent_browser_runnable(shutil.which("agent-browser")): + return True local_bin = ( Path(__file__).parent.parent / "node_modules" / ".bin" / "agent-browser" ) - return bool(agent_browser_bin or local_bin.exists()) + return agent_browser_runnable(str(local_bin)) if local_bin.exists() else False def _local_browser_runnable() -> bool: diff --git a/hermes_constants.py b/hermes_constants.py index 9f131f30489..3b37e671e7d 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -340,6 +340,51 @@ def with_hermes_node_path(env: dict[str, str] | None = None) -> dict[str, str]: return merged +def agent_browser_runnable(path: str | None) -> bool: + """Return True only when *path* is an agent-browser CLI that actually runs. + + A bare presence check (``shutil.which`` / ``Path.exists``) is not enough: + agent-browser's npm ``postinstall`` re-points a *global* install symlink + (e.g. ``/opt/homebrew/bin/agent-browser``) at our local + ``node_modules/agent-browser/bin/...`` binary, which then disappears on the + next ``hermes update`` — leaving a **dangling symlink** that ``which`` still + reports but exec fails on with exit 127 (issue #48521). Callers that trust + such a path silently break every browser tool. + + This validates the candidate by resolving it to a real, executable file and + running ``--version`` with a short timeout. Returns True only on a clean + (exit 0) run, so a dead/wrong-arch/hung binary is rejected and the caller + can fall through to the next resolution candidate. + + Special cases: + * ``None`` / empty → False. + * The ``"npx agent-browser"`` fallback form (contains a space, not a real + file) → True; npx resolves and validates the package at run time, so + there is nothing to stat here. + """ + if not path: + return False + # The npx fallback is a two-token command string, not a filesystem path. + if " " in path and path.split()[0].endswith("npx"): + return True + # exists() follows symlinks — a dangling link returns False here, so we + # never even spawn a subprocess for the broken-link case. + if not os.path.exists(path) or not os.access(path, os.X_OK): + return False + import subprocess + + try: + result = subprocess.run( + [path, "--version"], + capture_output=True, + timeout=10, + env=with_hermes_node_path(), + ) + except (OSError, subprocess.TimeoutExpired, ValueError): + return False + return result.returncode == 0 + + def display_hermes_home() -> str: """Return a user-friendly display string for the current HERMES_HOME. diff --git a/tests/test_hermes_constants.py b/tests/test_hermes_constants.py index d6b67cd3348..3d8c95e6085 100644 --- a/tests/test_hermes_constants.py +++ b/tests/test_hermes_constants.py @@ -8,6 +8,7 @@ import pytest import hermes_constants from hermes_constants import ( VALID_REASONING_EFFORTS, + agent_browser_runnable, find_hermes_node_executable, find_node_executable, find_node_executable_on_path, @@ -424,3 +425,48 @@ class TestSecureParentDir: secure_parent_dir(link_target) assert len(called_with) == 1 assert called_with[0] == (str(real_dir), 0o700) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX shell stubs; Windows uses .cmd shims") +class TestAgentBrowserRunnable: + """agent_browser_runnable() validates the resolved CLI actually runs. + + Regression coverage for issue #48521: a dangling global symlink left by + agent-browser's npm postinstall is reported by ``which`` but fails at exec + with exit 127, silently breaking every browser tool. The validator must + reject it (and other non-runnable candidates) so callers fall through. + """ + + def _stub(self, tmp_path, name, body, mode=0o755): + p = tmp_path / name + p.write_text(body) + p.chmod(mode) + return p + + def test_none_and_empty_rejected(self): + assert agent_browser_runnable(None) is False + assert agent_browser_runnable("") is False + + def test_dangling_symlink_rejected(self, tmp_path): + link = tmp_path / "agent-browser" + link.symlink_to(tmp_path / "does-not-exist") + # exists() follows the link → False, so it's rejected without exec. + assert agent_browser_runnable(str(link)) is False + + def test_runnable_binary_accepted(self, tmp_path): + good = self._stub(tmp_path, "agent-browser", "#!/bin/sh\necho 'agent-browser 0.27.1'\nexit 0\n") + assert agent_browser_runnable(str(good)) is True + + def test_nonzero_exit_rejected(self, tmp_path): + bad = self._stub(tmp_path, "agent-browser", "#!/bin/sh\nexit 127\n") + assert agent_browser_runnable(str(bad)) is False + + def test_not_executable_rejected(self, tmp_path): + noexec = self._stub(tmp_path, "agent-browser", "#!/bin/sh\necho hi\n", mode=0o644) + assert agent_browser_runnable(str(noexec)) is False + + def test_npx_fallback_form_accepted(self): + # The "npx agent-browser" command form is not a real file; npx resolves + # the package at run time, so the validator trusts it without stat. + assert agent_browser_runnable("npx agent-browser") is True + assert agent_browser_runnable("/usr/local/bin/npx agent-browser") is True diff --git a/tests/tools/test_browser_hardening.py b/tests/tools/test_browser_hardening.py index cf1197eae63..d004fd84316 100644 --- a/tests/tools/test_browser_hardening.py +++ b/tests/tools/test_browser_hardening.py @@ -54,7 +54,8 @@ class TestFindAgentBrowserCache: def test_cached_after_first_call(self): import tools.browser_tool as bt - with patch("shutil.which", return_value="/usr/bin/agent-browser"): + with patch("shutil.which", return_value="/usr/bin/agent-browser"), \ + patch("tools.browser_tool.agent_browser_runnable", return_value=True): result1 = bt._find_agent_browser() result2 = bt._find_agent_browser() assert result1 == result2 == "/usr/bin/agent-browser" diff --git a/tests/tools/test_browser_homebrew_paths.py b/tests/tools/test_browser_homebrew_paths.py index 16b7f5607ba..fbb1749d4d6 100644 --- a/tests/tools/test_browser_homebrew_paths.py +++ b/tests/tools/test_browser_homebrew_paths.py @@ -101,7 +101,8 @@ class TestFindAgentBrowser: def test_finds_in_current_path(self): """Should return result from shutil.which if available on current PATH.""" - with patch("shutil.which", return_value="/usr/local/bin/agent-browser"): + with patch("shutil.which", return_value="/usr/local/bin/agent-browser"), \ + patch("tools.browser_tool.agent_browser_runnable", return_value=True): assert _find_agent_browser() == "/usr/local/bin/agent-browser" def test_finds_in_homebrew_bin(self): @@ -112,6 +113,7 @@ class TestFindAgentBrowser: return None with patch("shutil.which", side_effect=mock_which), \ + patch("tools.browser_tool.agent_browser_runnable", return_value=True), \ patch("os.path.isdir", return_value=True), \ patch( "tools.browser_tool._discover_homebrew_node_dirs", diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 3332d3a740d..9770bcd459f 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -65,7 +65,7 @@ import requests from typing import Dict, Any, Optional, List, Tuple, Union from pathlib import Path from agent.auxiliary_client import call_llm -from hermes_constants import get_hermes_home +from hermes_constants import agent_browser_runnable, get_hermes_home from utils import env_int, is_truthy_value from hermes_cli.config import DEFAULT_CONFIG, cfg_get @@ -1904,10 +1904,19 @@ def _find_agent_browser() -> str: # Note: _agent_browser_resolved is set at each return site below # (not before the search) to prevent a race where a concurrent thread # sees resolved=True but _cached_agent_browser is still None. + # + # Every candidate below is validated with ``agent_browser_runnable`` before + # it is cached. A bare ``shutil.which`` hit is NOT trusted: agent-browser's + # npm postinstall re-points a global install symlink at our local + # node_modules binary, which disappears on the next ``hermes update`` and + # leaves a dangling link that ``which`` still reports but exec fails on with + # exit 127 (issue #48521). Validating lets a dead candidate fall through to + # the next working resolution (extended PATH → local .bin → npx) instead of + # caching the broken one and silently killing every browser tool. # Check if it's in PATH (global install) which_result = shutil.which("agent-browser") - if which_result: + if which_result and agent_browser_runnable(which_result): _cached_agent_browser = which_result _agent_browser_resolved = True return which_result @@ -1917,7 +1926,7 @@ def _find_agent_browser() -> str: extended_path = _merge_browser_path("") if extended_path: which_result = shutil.which("agent-browser", path=extended_path) - if which_result: + if which_result and agent_browser_runnable(which_result): _cached_agent_browser = which_result _agent_browser_resolved = True return which_result @@ -1934,7 +1943,7 @@ def _find_agent_browser() -> str: local_bin_dir = repo_root / "node_modules" / ".bin" if local_bin_dir.is_dir(): local_which = shutil.which("agent-browser", path=str(local_bin_dir)) - if local_which: + if local_which and agent_browser_runnable(local_which): _cached_agent_browser = local_which _agent_browser_resolved = True return _cached_agent_browser @@ -1952,22 +1961,18 @@ def _find_agent_browser() -> str: try: from hermes_cli.dep_ensure import ensure_dependency if ensure_dependency("browser"): - recheck = shutil.which("agent-browser") - if not recheck and extended_path: - recheck = shutil.which("agent-browser", path=extended_path) - if not recheck: - hermes_nm = str(get_hermes_home() / "node_modules" / ".bin") - recheck = shutil.which("agent-browser", path=hermes_nm) - if not recheck: - hermes_node_bin = str(get_hermes_home() / "node" / "bin") - recheck = shutil.which("agent-browser", path=hermes_node_bin) - if not recheck: - hermes_node_root = str(get_hermes_home() / "node") - recheck = shutil.which("agent-browser", path=hermes_node_root) - if recheck: - _cached_agent_browser = recheck - _agent_browser_resolved = True - return recheck + candidates = [ + shutil.which("agent-browser"), + shutil.which("agent-browser", path=extended_path) if extended_path else None, + shutil.which("agent-browser", path=str(get_hermes_home() / "node_modules" / ".bin")), + shutil.which("agent-browser", path=str(get_hermes_home() / "node" / "bin")), + shutil.which("agent-browser", path=str(get_hermes_home() / "node")), + ] + for recheck in candidates: + if recheck and agent_browser_runnable(recheck): + _cached_agent_browser = recheck + _agent_browser_resolved = True + return recheck except Exception: pass