From 675d41fb25012ae039a81aff7491e12cf214ba00 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Mon, 10 Aug 2026 17:45:43 +0800 Subject: [PATCH] fix(browser): pin npx agent-browser resolution and share a sentinel constant Git-clone installs resolving agent-browser via bare npx floated latest with no integrity check, while install.sh/install.ps1 installs stayed pinned to ^0.26.0. Pin the npx spec to match. Also extract the "npx agent-browser" sentinel comparison (6 call sites across two packages) into a named constant/predicate, fix a PATH-priority inversion where a broken system npx could shadow a healthy Hermes-managed one at the two real npx launch sites, and stop `hermes doctor --fix` from counting a bonus npx cache warm as a fixed issue on an otherwise-healthy run. --- hermes_cli/doctor.py | 7 +-- hermes_cli/tools_config.py | 6 +- tests/hermes_cli/test_tools_config.py | 5 +- .../test_browser_chromium_autoinstall.py | 2 +- tests/tools/test_browser_npx_warmup.py | 4 +- tools/browser_tool.py | 58 +++++++++++++------ 6 files changed, 53 insertions(+), 29 deletions(-) diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 1401827c39cfa..e16de6060c9ef 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -2115,12 +2115,12 @@ def run_doctor(args): # existence check with no subprocess spawn or install side effects. agent_browser_ok = False try: - from tools.browser_tool import _find_agent_browser + from tools.browser_tool import _find_agent_browser, _is_npx_agent_browser_sentinel _resolved_ab = _find_agent_browser(validate=False) except Exception: _resolved_ab = None - if _resolved_ab == "npx agent-browser": + if _resolved_ab and _is_npx_agent_browser_sentinel(_resolved_ab): check_ok("agent-browser", "(resolves via npx on first use)") agent_browser_ok = True if should_fix: @@ -2130,8 +2130,7 @@ def run_doctor(args): # doesn't pay the registry fetch either way. from tools.browser_tool import warm_agent_browser_npx_cache if warm_agent_browser_npx_cache(): - check_ok(" Warmed npx cache for agent-browser") - fixed_count += 1 + check_info(" Warmed npx cache for agent-browser") else: check_info(" Could not warm npx cache (offline or npx unavailable)") elif _resolved_ab and agent_browser_runnable(_resolved_ab): diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 178b8c3ddf077..07d944f77fae6 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -1659,6 +1659,8 @@ def _run_post_setup(post_setup_key: str): _running_in_docker, _find_agent_browser, _resolve_npx_bin, + _is_npx_agent_browser_sentinel, + AGENT_BROWSER_NPX_SPEC, ) except Exception as exc: # pragma: no cover — defensive _print_warning(f" Could not check Chromium status: {exc}") @@ -1707,7 +1709,7 @@ def _run_post_setup(post_setup_key: str): # browser_cmd was already resolved above (same PATH -> Homebrew -> # Hermes-managed-node -> npx cascade _find_agent_browser uses at # runtime), so this can't diverge from what actually gets invoked. - if browser_cmd == "npx agent-browser": + if _is_npx_agent_browser_sentinel(browser_cmd): # Re-resolve via the same PATH + extended-PATH cascade # _find_agent_browser used, rather than a bare shutil.which("npx") # — Hermes-managed-Node-only setups resolve npx only through the @@ -1719,7 +1721,7 @@ def _run_post_setup(post_setup_key: str): " npx not found - install Chromium manually: npx agent-browser install --with-deps" ) return - install_cmd = [npx_bin, "-y", "agent-browser", "install", "--with-deps"] + install_cmd = [npx_bin, "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps"] else: install_cmd = [browser_cmd, "install", "--with-deps"] diff --git a/tests/hermes_cli/test_tools_config.py b/tests/hermes_cli/test_tools_config.py index 6253c9cbcb26a..17d4c3ad06dcd 100644 --- a/tests/hermes_cli/test_tools_config.py +++ b/tests/hermes_cli/test_tools_config.py @@ -7,6 +7,7 @@ from unittest.mock import patch import pytest +from tools.browser_tool import AGENT_BROWSER_NPX_SPEC from hermes_cli.nous_account import NousPortalAccountInfo, NousToolAccessInfo from hermes_cli.nous_subscription import NousSubscriptionFeatures from hermes_cli.tools_config import ( @@ -454,7 +455,7 @@ class TestAgentBrowserPostSetup: run.assert_called_once() assert run.call_args.args[0] == [ - "/usr/bin/npx", "-y", "agent-browser", "install", "--with-deps", + "/usr/bin/npx", "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", ] def test_installs_chromium_via_npx_resolved_only_through_extended_path(self): @@ -482,7 +483,7 @@ class TestAgentBrowserPostSetup: run.assert_called_once() assert run.call_args.args[0] == [ - hermes_npx, "-y", "agent-browser", "install", "--with-deps", + hermes_npx, "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", ] def test_warns_instead_of_crashing_when_npx_unresolvable_after_all(self): diff --git a/tests/tools/test_browser_chromium_autoinstall.py b/tests/tools/test_browser_chromium_autoinstall.py index 549e2ae9b2346..6e0cfa4de4690 100644 --- a/tests/tools/test_browser_chromium_autoinstall.py +++ b/tests/tools/test_browser_chromium_autoinstall.py @@ -72,7 +72,7 @@ class TestInstall: ) assert bt._maybe_autoinstall_chromium() is True - assert captured["cmd"] == ["/usr/bin/npx", "-y", "agent-browser", "install"] + assert captured["cmd"] == ["/usr/bin/npx", "-y", bt.AGENT_BROWSER_NPX_SPEC, "install"] assert "--with-deps" not in captured["cmd"] def test_nonzero_exit_returns_false(self, monkeypatch): diff --git a/tests/tools/test_browser_npx_warmup.py b/tests/tools/test_browser_npx_warmup.py index 5f142dbea421b..10d1e08dd7e27 100644 --- a/tests/tools/test_browser_npx_warmup.py +++ b/tests/tools/test_browser_npx_warmup.py @@ -15,7 +15,7 @@ from __future__ import annotations import subprocess from unittest.mock import patch -from tools.browser_tool import warm_agent_browser_npx_cache +from tools.browser_tool import AGENT_BROWSER_NPX_SPEC, warm_agent_browser_npx_cache def test_returns_false_without_calling_subprocess_when_npx_missing(): @@ -35,7 +35,7 @@ def test_invokes_npx_with_prefer_offline_version_check(): mock_run.assert_called_once() args, kwargs = mock_run.call_args - assert args[0] == ["/usr/bin/npx", "--prefer-offline", "-y", "agent-browser", "--version"] + assert args[0] == ["/usr/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, "--version"] assert kwargs.get("check") is False diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 90c92ab2197f8..8a8e3f49b6290 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -905,8 +905,26 @@ def _browser_install_hint() -> str: return "npm install -g agent-browser && agent-browser install --with-deps" +# Sentinel _find_agent_browser returns/caches to mean "resolve via npx" rather +# than a concrete executable path. A named constant + predicate keep the six +# comparison sites (four here, plus hermes_cli/tools_config.py and +# hermes_cli/doctor.py) from drifting if the sentinel's exact spelling ever +# changes. +NPX_AGENT_BROWSER_SENTINEL = "npx agent-browser" + +# Pinned to match scripts/install.sh / scripts/install.ps1's +# "agent-browser@^0.26.0" managed install so a git-clone install resolving +# agent-browser via bare npx gets the same version as a managed install, +# instead of floating latest with no integrity check. Update both together. +AGENT_BROWSER_NPX_SPEC = "agent-browser@^0.26.0" + + +def _is_npx_agent_browser_sentinel(browser_cmd: str) -> bool: + return browser_cmd.strip() == NPX_AGENT_BROWSER_SENTINEL + + def _requires_real_termux_browser_install(browser_cmd: str) -> bool: - return _is_termux_environment() and _is_local_mode() and browser_cmd.strip() == "npx agent-browser" + return _is_termux_environment() and _is_local_mode() and _is_npx_agent_browser_sentinel(browser_cmd) def _termux_browser_install_error() -> str: @@ -1210,14 +1228,16 @@ def _run_chrome_fallback_command( ) return {"success": False, "error": hint} - # On Windows npx is npx.cmd — use shutil.which so CreateProcessW can - # execute the batch shim. shutil.which honours PATHEXT on Windows and - # returns the plain executable on POSIX. If npx isn't on PATH (Termux, - # bare container), fall back to the bare name and let Popen raise with - # a readable "FileNotFoundError: 'npx'" rather than WinError 193. - if browser_cmd == "npx agent-browser": - _npx_bin = shutil.which("npx") or "npx" - cmd_prefix = [_npx_bin, "agent-browser"] + # Resolve npx via the same PATH + extended-PATH cascade _find_agent_browser + # uses, not a bare shutil.which("npx") — Hermes-managed-Node-only setups + # resolve npx only through the extended fallback path, and a bare lookup + # would let a broken system npx shadow a healthy managed one. If npx isn't + # found at all (Termux, bare container), fall back to the bare name and + # let Popen raise with a readable "FileNotFoundError: 'npx'" rather than + # WinError 193. + if _is_npx_agent_browser_sentinel(browser_cmd): + _npx_bin = _resolve_npx_bin() or "npx" + cmd_prefix = [_npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] else: cmd_prefix = [browser_cmd] base_args = cmd_prefix + ["--engine", "chrome", "--session", tmp_session, "--json"] @@ -2453,8 +2473,8 @@ def _find_agent_browser(*, validate: bool = True) -> str: npx_path = _resolve_npx_bin() if npx_path: if not validate: - return "npx agent-browser" - _cached_agent_browser = "npx agent-browser" + return NPX_AGENT_BROWSER_SENTINEL + _cached_agent_browser = NPX_AGENT_BROWSER_SENTINEL _agent_browser_resolved = True return _cached_agent_browser @@ -2514,7 +2534,7 @@ def warm_agent_browser_npx_cache(timeout: float = 60.0) -> bool: # --fix` runs shouldn't hit the registry just to re-confirm # "latest" is still latest — that would defeat the point of # warming the cache in the first place. - [npx_bin, "--prefer-offline", "-y", "agent-browser", "--version"], + [npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, "--version"], capture_output=True, text=True, timeout=timeout, @@ -2646,10 +2666,12 @@ def _run_browser_command( # Keep concrete executable paths intact, even when they contain spaces. # Only the synthetic npx fallback needs to expand into multiple argv items. - # shutil.which resolves npx → npx.cmd on Windows; bare "npx" stays on POSIX. - if browser_cmd == "npx agent-browser": - _npx_bin = shutil.which("npx") or "npx" - cmd_prefix = [_npx_bin, "agent-browser"] + # Resolve via the same PATH + extended-PATH cascade _find_agent_browser + # uses (see the chrome-fallback call site above for why a bare + # shutil.which("npx") is wrong here). + if _is_npx_agent_browser_sentinel(browser_cmd): + _npx_bin = _resolve_npx_bin() or "npx" + cmd_prefix = [_npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] else: cmd_prefix = [browser_cmd] @@ -4966,8 +4988,8 @@ def _maybe_autoinstall_chromium() -> bool: except FileNotFoundError: return False - if browser_cmd == "npx agent-browser": - install_cmd = [shutil.which("npx") or "npx", "-y", "agent-browser", "install"] + if _is_npx_agent_browser_sentinel(browser_cmd): + install_cmd = [_resolve_npx_bin() or "npx", "-y", AGENT_BROWSER_NPX_SPEC, "install"] else: install_cmd = [browser_cmd, "install"]