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.
This commit is contained in:
parent
31337b388b
commit
675d41fb25
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue