fix(browser): validate agent-browser is runnable, not just present (#51740)
After `hermes update`, a globally-installed agent-browser's npm postinstall (fixUnixSymlink) re-points the global symlink (e.g. /opt/homebrew/bin/agent-browser) at our local node_modules binary. The next update wipes node_modules, leaving a dangling symlink that `which` still reports but exec fails on with exit 127 — silently breaking every browser tool (#48521). Root cause is trust-on-presence: shutil.which/Path.exists accept a name that resolves but won't run. Add hermes_constants.agent_browser_runnable() (resolves the path + runs --version) and gate all four resolution sites on it: _find_agent_browser now skips a dead candidate and falls through to the next working one (extended PATH -> local .bin -> npx), self-healing the dangling link. dep_ensure/doctor/nous_subscription validate too; doctor warns on a broken link. Closes #48521.fix/verification-admin-route-recovery
parent
a911bcda18
commit
3c75e11571
|
|
@ -22,12 +22,14 @@ import subprocess
|
||||||
import sys
|
import sys
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
|
from hermes_constants import agent_browser_runnable
|
||||||
|
|
||||||
_IS_WINDOWS = platform.system() == "Windows"
|
_IS_WINDOWS = platform.system() == "Windows"
|
||||||
|
|
||||||
_DEP_CHECKS = {
|
_DEP_CHECKS = {
|
||||||
"node": lambda: shutil.which("node") is not None,
|
"node": lambda: shutil.which("node") is not None,
|
||||||
"browser": lambda: (
|
"browser": lambda: (
|
||||||
shutil.which("agent-browser") is not None
|
agent_browser_runnable(shutil.which("agent-browser"))
|
||||||
or _has_system_browser()
|
or _has_system_browser()
|
||||||
or _has_hermes_agent_browser()
|
or _has_hermes_agent_browser()
|
||||||
),
|
),
|
||||||
|
|
|
||||||
|
|
@ -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.config import get_project_root, get_hermes_home, get_env_path
|
||||||
from hermes_cli.env_loader import load_hermes_dotenv
|
from hermes_cli.env_loader import load_hermes_dotenv
|
||||||
from hermes_constants import display_hermes_home
|
from hermes_constants import display_hermes_home
|
||||||
|
from hermes_constants import agent_browser_runnable
|
||||||
|
|
||||||
PROJECT_ROOT = get_project_root()
|
PROJECT_ROOT = get_project_root()
|
||||||
HERMES_HOME = get_hermes_home()
|
HERMES_HOME = get_hermes_home()
|
||||||
|
|
@ -1483,12 +1484,21 @@ def run_doctor(args):
|
||||||
# Check if agent-browser is installed
|
# Check if agent-browser is installed
|
||||||
agent_browser_path = PROJECT_ROOT / "node_modules" / "agent-browser"
|
agent_browser_path = PROJECT_ROOT / "node_modules" / "agent-browser"
|
||||||
agent_browser_ok = False
|
agent_browser_ok = False
|
||||||
|
_which_ab = shutil.which("agent-browser")
|
||||||
if agent_browser_path.exists():
|
if agent_browser_path.exists():
|
||||||
check_ok("agent-browser (Node.js)", "(browser automation)")
|
check_ok("agent-browser (Node.js)", "(browser automation)")
|
||||||
agent_browser_ok = True
|
agent_browser_ok = True
|
||||||
elif shutil.which("agent-browser"):
|
elif _which_ab and agent_browser_runnable(_which_ab):
|
||||||
check_ok("agent-browser", "(browser automation)")
|
check_ok("agent-browser", "(browser automation)")
|
||||||
agent_browser_ok = True
|
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():
|
elif _is_termux():
|
||||||
check_info("agent-browser is not installed (expected in the tested Termux path)")
|
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")
|
check_info("Install it manually later with: npm install -g agent-browser && agent-browser install")
|
||||||
|
|
|
||||||
|
|
@ -159,11 +159,17 @@ def _toolset_enabled(config: Dict[str, object], toolset_key: str) -> bool:
|
||||||
def _has_agent_browser() -> bool:
|
def _has_agent_browser() -> bool:
|
||||||
import shutil
|
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 = (
|
local_bin = (
|
||||||
Path(__file__).parent.parent / "node_modules" / ".bin" / "agent-browser"
|
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:
|
def _local_browser_runnable() -> bool:
|
||||||
|
|
|
||||||
|
|
@ -340,6 +340,51 @@ def with_hermes_node_path(env: dict[str, str] | None = None) -> dict[str, str]:
|
||||||
return merged
|
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:
|
def display_hermes_home() -> str:
|
||||||
"""Return a user-friendly display string for the current HERMES_HOME.
|
"""Return a user-friendly display string for the current HERMES_HOME.
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -8,6 +8,7 @@ import pytest
|
||||||
import hermes_constants
|
import hermes_constants
|
||||||
from hermes_constants import (
|
from hermes_constants import (
|
||||||
VALID_REASONING_EFFORTS,
|
VALID_REASONING_EFFORTS,
|
||||||
|
agent_browser_runnable,
|
||||||
find_hermes_node_executable,
|
find_hermes_node_executable,
|
||||||
find_node_executable,
|
find_node_executable,
|
||||||
find_node_executable_on_path,
|
find_node_executable_on_path,
|
||||||
|
|
@ -424,3 +425,48 @@ class TestSecureParentDir:
|
||||||
secure_parent_dir(link_target)
|
secure_parent_dir(link_target)
|
||||||
assert len(called_with) == 1
|
assert len(called_with) == 1
|
||||||
assert called_with[0] == (str(real_dir), 0o700)
|
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
|
||||||
|
|
|
||||||
|
|
@ -54,7 +54,8 @@ class TestFindAgentBrowserCache:
|
||||||
|
|
||||||
def test_cached_after_first_call(self):
|
def test_cached_after_first_call(self):
|
||||||
import tools.browser_tool as bt
|
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()
|
result1 = bt._find_agent_browser()
|
||||||
result2 = bt._find_agent_browser()
|
result2 = bt._find_agent_browser()
|
||||||
assert result1 == result2 == "/usr/bin/agent-browser"
|
assert result1 == result2 == "/usr/bin/agent-browser"
|
||||||
|
|
|
||||||
|
|
@ -101,7 +101,8 @@ class TestFindAgentBrowser:
|
||||||
|
|
||||||
def test_finds_in_current_path(self):
|
def test_finds_in_current_path(self):
|
||||||
"""Should return result from shutil.which if available on current PATH."""
|
"""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"
|
assert _find_agent_browser() == "/usr/local/bin/agent-browser"
|
||||||
|
|
||||||
def test_finds_in_homebrew_bin(self):
|
def test_finds_in_homebrew_bin(self):
|
||||||
|
|
@ -112,6 +113,7 @@ class TestFindAgentBrowser:
|
||||||
return None
|
return None
|
||||||
|
|
||||||
with patch("shutil.which", side_effect=mock_which), \
|
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("os.path.isdir", return_value=True), \
|
||||||
patch(
|
patch(
|
||||||
"tools.browser_tool._discover_homebrew_node_dirs",
|
"tools.browser_tool._discover_homebrew_node_dirs",
|
||||||
|
|
|
||||||
|
|
@ -65,7 +65,7 @@ import requests
|
||||||
from typing import Dict, Any, Optional, List, Tuple, Union
|
from typing import Dict, Any, Optional, List, Tuple, Union
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from agent.auxiliary_client import call_llm
|
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 utils import env_int, is_truthy_value
|
||||||
from hermes_cli.config import DEFAULT_CONFIG, cfg_get
|
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
|
# Note: _agent_browser_resolved is set at each return site below
|
||||||
# (not before the search) to prevent a race where a concurrent thread
|
# (not before the search) to prevent a race where a concurrent thread
|
||||||
# sees resolved=True but _cached_agent_browser is still None.
|
# 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)
|
# Check if it's in PATH (global install)
|
||||||
which_result = shutil.which("agent-browser")
|
which_result = shutil.which("agent-browser")
|
||||||
if which_result:
|
if which_result and agent_browser_runnable(which_result):
|
||||||
_cached_agent_browser = which_result
|
_cached_agent_browser = which_result
|
||||||
_agent_browser_resolved = True
|
_agent_browser_resolved = True
|
||||||
return which_result
|
return which_result
|
||||||
|
|
@ -1917,7 +1926,7 @@ def _find_agent_browser() -> str:
|
||||||
extended_path = _merge_browser_path("")
|
extended_path = _merge_browser_path("")
|
||||||
if extended_path:
|
if extended_path:
|
||||||
which_result = shutil.which("agent-browser", path=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
|
_cached_agent_browser = which_result
|
||||||
_agent_browser_resolved = True
|
_agent_browser_resolved = True
|
||||||
return which_result
|
return which_result
|
||||||
|
|
@ -1934,7 +1943,7 @@ def _find_agent_browser() -> str:
|
||||||
local_bin_dir = repo_root / "node_modules" / ".bin"
|
local_bin_dir = repo_root / "node_modules" / ".bin"
|
||||||
if local_bin_dir.is_dir():
|
if local_bin_dir.is_dir():
|
||||||
local_which = shutil.which("agent-browser", path=str(local_bin_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
|
_cached_agent_browser = local_which
|
||||||
_agent_browser_resolved = True
|
_agent_browser_resolved = True
|
||||||
return _cached_agent_browser
|
return _cached_agent_browser
|
||||||
|
|
@ -1952,22 +1961,18 @@ def _find_agent_browser() -> str:
|
||||||
try:
|
try:
|
||||||
from hermes_cli.dep_ensure import ensure_dependency
|
from hermes_cli.dep_ensure import ensure_dependency
|
||||||
if ensure_dependency("browser"):
|
if ensure_dependency("browser"):
|
||||||
recheck = shutil.which("agent-browser")
|
candidates = [
|
||||||
if not recheck and extended_path:
|
shutil.which("agent-browser"),
|
||||||
recheck = shutil.which("agent-browser", path=extended_path)
|
shutil.which("agent-browser", path=extended_path) if extended_path else None,
|
||||||
if not recheck:
|
shutil.which("agent-browser", path=str(get_hermes_home() / "node_modules" / ".bin")),
|
||||||
hermes_nm = str(get_hermes_home() / "node_modules" / ".bin")
|
shutil.which("agent-browser", path=str(get_hermes_home() / "node" / "bin")),
|
||||||
recheck = shutil.which("agent-browser", path=hermes_nm)
|
shutil.which("agent-browser", path=str(get_hermes_home() / "node")),
|
||||||
if not recheck:
|
]
|
||||||
hermes_node_bin = str(get_hermes_home() / "node" / "bin")
|
for recheck in candidates:
|
||||||
recheck = shutil.which("agent-browser", path=hermes_node_bin)
|
if recheck and agent_browser_runnable(recheck):
|
||||||
if not recheck:
|
_cached_agent_browser = recheck
|
||||||
hermes_node_root = str(get_hermes_home() / "node")
|
_agent_browser_resolved = True
|
||||||
recheck = shutil.which("agent-browser", path=hermes_node_root)
|
return recheck
|
||||||
if recheck:
|
|
||||||
_cached_agent_browser = recheck
|
|
||||||
_agent_browser_resolved = True
|
|
||||||
return recheck
|
|
||||||
except Exception:
|
except Exception:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue