From 5f5f8d5b62812db00f6b6d958d6dcdc07893db3c Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Wed, 15 Jul 2026 00:58:37 +0800 Subject: [PATCH] fix(cli): drop agent-browser/@streamdown-math from root npm deps `hermes update` was pruning root-level Node dependencies (agent-browser) because npm ci always wipes and reifies node_modules according to its active filter -- no root-first/workspace-first ordering or flag combination (--workspaces=false, --include-workspace-root, etc.) can reliably keep a root-only package.json dependency from being pruned by a subsequent workspace-scoped npm ci. Confirmed empirically and via npm/cli source (isArboristCmd hardcodes includeWorkspaceRoot=false for ci/install), so no amount of install-order juggling fixes this for good. Instead of chasing install order, remove the root-only dependencies that made the npm step fragile in the first place: - agent-browser is no longer a root package.json dependency. It resolves lazily via `npx agent-browser` (tools/browser_tool.py already had this as a fallback; it's now the primary path). warm_agent_browser_npx_cache() is called fire-and-forget from both `hermes update` and `hermes doctor --fix` to keep npx's cache warm, preserving the "available before any session starts" property agent-browser had as an eager dependency without re-entangling it with the npm workspace graph. - @streamdown/math moves to apps/desktop/package.json, where it's actually imported (markdown-text.tsx, katex-memo.ts) -- it was never used anywhere else and was subject to the same pruning risk. - _update_node_dependencies() collapses to a single `npm ci --workspace ui-tui --workspace web` call now that root has no dependencies of its own to protect, and keeps its original spot ahead of `_build_web_ui()` at both call sites in update_cmd.py -- with no root-only dependencies left to protect, there's no reason for the Node refresh and the web build to run in any particular order relative to each other. - hermes_cli/tools_config.py's post-setup Chromium-install path and hermes_cli/doctor.py's agent-browser check both now resolve through the same PATH -> Homebrew/Hermes-managed-node -> npx cascade (_find_agent_browser / _resolve_npx_bin) instead of hand-rolling their own node_modules/.bin lookups, so they can't diverge from what browser tools actually invoke at runtime. - tests-js/package-json-lazy-deps.test.ts gets a lockfile-level check mirroring the existing camofox one, so a future regression that reintroduces agent-browser into package-lock.json fails this test directly instead of relying on manual review to catch it. Fixes #43564. --- apps/desktop/package.json | 1 + hermes_cli/doctor.py | 62 +++--- hermes_cli/tools_config.py | 115 +++++------ hermes_cli/update_cmd.py | 93 ++++----- package-lock.json | 15 +- package.json | 7 +- tests-js/package-json-lazy-deps.test.ts | 90 +++++++-- tests/hermes_cli/test_cmd_update.py | 219 +++++++++++++++++++- tests/hermes_cli/test_doctor.py | 101 +++++++--- tests/hermes_cli/test_tools_config.py | 256 ++++++++++++++++++++++++ tests/tools/test_browser_npx_warmup.py | 75 +++++++ tools/browser_tool.py | 60 +++++- 12 files changed, 874 insertions(+), 220 deletions(-) create mode 100644 tests/tools/test_browser_npx_warmup.py diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 704a41fb9f..dc9f93392f 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -90,6 +90,7 @@ "@nanostores/react": "1.1.0", "@nous-research/ui": "0.18.2", "@streamdown/code": "1.1.1", + "@streamdown/math": "1.0.2", "@tabler/icons-react": "3.44.0", "@tailwindcss/typography": "0.5.20", "@tailwindcss/vite": "4.3.3", diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 88ce3ecba7..1401827c39 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -2107,49 +2107,43 @@ def run_doctor(args): # Node.js + agent-browser (for browser automation tools) if _safe_which("node"): check_ok("Node.js") - # Check if agent-browser is installed - agent_browser_path = PROJECT_ROOT / "node_modules" / "agent-browser" + # agent-browser is no longer a root package.json dependency (#43564) + # — it resolves lazily via npx (or a global/Hermes-managed install) + # at first use. Mirror tools.browser_tool._find_agent_browser's own + # resolution cascade here so doctor can't diverge from what browser + # tools will actually find; validate=False keeps this a cheap + # existence check with no subprocess spawn or install side effects. agent_browser_ok = False - _which_ab = shutil.which("agent-browser") - # `hermes acp --setup-browser` installs agent-browser into the - # Hermes-managed node prefix, which isn't necessarily on PATH. Mirror - # dep_ensure._has_hermes_agent_browser() so doctor and dep_ensure agree - # on what "installed" means; otherwise doctor false-negatives (#53192). - # Resolve with PATHEXT-aware ``shutil.which`` (not a bare is_file()) - # so Windows picks the executable ``.cmd`` shim — the same class of - # miss fixed for _has_agent_browser() in #73932. - def _which_in(directory) -> str | None: - try: - if not directory.is_dir(): - return None - return shutil.which("agent-browser", path=str(directory)) - except Exception: - return None + try: + from tools.browser_tool import _find_agent_browser + _resolved_ab = _find_agent_browser(validate=False) + except Exception: + _resolved_ab = None - _managed_ab = ( - _which_in(HERMES_HOME / "node" / "bin") - or _which_in(HERMES_HOME / "node") - ) - _legacy_ab = _which_in(HERMES_HOME / "node_modules" / ".bin") - if agent_browser_path.exists(): - check_ok("agent-browser (Node.js)", "(browser automation)") + if _resolved_ab == "npx agent-browser": + check_ok("agent-browser", "(resolves via npx on first use)") agent_browser_ok = True - elif _which_ab and agent_browser_runnable(_which_ab): + if should_fix: + # Doctor can't tell from here whether npx's cache already + # has agent-browser warm — just fire the same warm-up + # `hermes update` does, so a session's first browser call + # 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 + else: + check_info(" Could not warm npx cache (offline or npx unavailable)") + elif _resolved_ab and agent_browser_runnable(_resolved_ab): check_ok("agent-browser", "(browser automation)") agent_browser_ok = True - elif _managed_ab and agent_browser_runnable(_managed_ab): - check_ok("agent-browser", "(browser automation)") - agent_browser_ok = True - elif _legacy_ab and agent_browser_runnable(_legacy_ab): - check_ok("agent-browser", "(browser automation)") - agent_browser_ok = True - elif _which_ab: + elif _resolved_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)", + f"(broken symlink at {_resolved_ab}? run: npx agent-browser --version)", ) elif _is_termux(): check_info("agent-browser is not installed (expected in the tested Termux path)") @@ -2158,7 +2152,7 @@ def run_doctor(args): for step in _termux_browser_setup_steps(node_installed=True): check_info(step) else: - check_warn("agent-browser not installed", "(run: npm install)") + check_warn("agent-browser not installed", "(requires npm/npx on PATH)") # Chromium presence — the browser tools silently fail to register when # agent-browser is found but no Playwright-managed Chromium is on disk diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 8ac29e0f5a..178b8c3ddf 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -1644,69 +1644,50 @@ def _run_cua_driver_installer( def _run_post_setup(post_setup_key: str): """Run post-setup hooks for tools that need extra installation steps.""" - import shutil from hermes_constants import find_node_executable if post_setup_key in {"agent_browser", "browserbase"}: - node_modules = PROJECT_ROOT / "node_modules" / "agent-browser" - # Managed Node first — $HERMES_HOME/node is not on PATH, so a bare - # which() reports "no npm" on installs whose only Node is the one - # Hermes installed for exactly this toolchain. - npm_bin = find_node_executable("npm") - npx_bin = find_node_executable("npx") - # Step 1: install the agent-browser npm package into node_modules/ - if not node_modules.exists() and npm_bin: - _print_info(" Installing Node.js dependencies for browser tools...") - import subprocess - # Use the resolved npm_bin absolute path so subprocess.Popen can - # execute npm.cmd on Windows (CreateProcessW otherwise rejects - # batch shims). On POSIX npm_bin is the plain path — same - # behaviour as before. - result = subprocess.run( - # --workspaces=false restricts the install to the repo root - # only, avoiding the apps/* glob which would pull in - # apps/desktop (Electron + node-pty) unnecessarily. See #38772. - [npm_bin, "install", "--silent", "--workspaces=false"], - capture_output=True, text=True, encoding="utf-8", errors="replace", cwd=str(PROJECT_ROOT), - creationflags=_post_setup_no_window_flags(), - ) - if result.returncode == 0: - _print_success(" Node.js dependencies installed") - else: - from hermes_constants import display_hermes_home - _print_warning(f" npm install failed - run manually: cd {display_hermes_home()}/hermes-agent && npm install --workspaces=false") - if result.stderr: - _print_info(f" {result.stderr.strip()[:200]}") - elif node_modules.exists(): - # Distinct message for the re-run case so the GUI action log tells - # the truth ("nothing to do") instead of implying a fresh install. - _print_success(" agent-browser already installed, nothing to do") - else: - _print_warning(" Node.js not found - browser tools require: npm install (in hermes-agent directory)") - return - - # Step 2: only the local browser provider actually needs Chromium on - # disk. Cloud providers (Browserbase, Browser Use, Firecrawl) host - # their own Chromium and don't need the local install. - if post_setup_key != "agent_browser": - return - - # Step 3: ensure the Chromium / headless-shell build agent-browser - # drives is actually installed. Without it the CLI hangs on first - # use until the command timeout fires. Skip inside Docker — the - # image bakes Chromium in at build time, and runtime users usually - # can't write to PLAYWRIGHT_BROWSERS_PATH anyway. + # agent-browser is no longer a root package.json dependency (#43564) + # — it resolves lazily via npx (or a global/Hermes-managed install) + # instead of a local `npm install`, so there's no node_modules/ + # population step here anymore. try: # Import lazily so the tools_config UI doesn't pull in the full # browser_tool module at import time. from tools.browser_tool import ( _chromium_installed, _running_in_docker, + _find_agent_browser, + _resolve_npx_bin, ) except Exception as exc: # pragma: no cover — defensive _print_warning(f" Could not check Chromium status: {exc}") return + # Reuse the same resolution cascade browser tools use at runtime + # (PATH -> Homebrew/Hermes-managed node -> npx) rather than a bare + # shutil.which — Hermes-managed-Node-only setups resolve agent-browser + # / npx only through the extended fallback path, which a bare + # shutil.which("npx") lookup misses. + try: + browser_cmd = _find_agent_browser(validate=False) + except FileNotFoundError: + _print_warning( + " npx not found - browser tools require Node.js: https://nodejs.org" + ) + return + + # Step 1: only the local browser provider actually needs Chromium on + # disk. Cloud providers (Browserbase, Browser Use, Firecrawl) host + # their own Chromium and don't need the local install. + if post_setup_key != "agent_browser": + return + + # Step 2: ensure the Chromium / headless-shell build agent-browser + # drives is actually installed. Without it the CLI hangs on first + # use until the command timeout fires. Skip inside Docker — the + # image bakes Chromium in at build time, and runtime users usually + # can't write to PLAYWRIGHT_BROWSERS_PATH anyway. if _chromium_installed(): _print_success(" Chromium browser already installed, nothing to do") return @@ -1723,27 +1704,27 @@ def _run_post_setup(post_setup_key: str): ) return - if not npx_bin: - _print_warning( - " npx not found - install Chromium manually: npx agent-browser install --with-deps" - ) - return + # 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": + # 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 + # extended fallback path, and a bare lookup here would silently + # diverge and hand subprocess.run a None argument. + npx_bin = _resolve_npx_bin() + if not npx_bin: + _print_warning( + " npx not found - install Chromium manually: npx agent-browser install --with-deps" + ) + return + install_cmd = [npx_bin, "-y", "agent-browser", "install", "--with-deps"] + else: + install_cmd = [browser_cmd, "install", "--with-deps"] _print_info(" Installing Chromium (~170MB one-time download)...") import subprocess - # Prefer the bundled agent-browser install subcommand so the - # version of Chromium matches the CLI. Fall back to npx shim on - # setups where the local bin stub isn't present. - local_ab = PROJECT_ROOT / "node_modules" / ".bin" / "agent-browser" - if sys.platform == "win32": - local_ab_win = local_ab.with_suffix(".cmd") - if local_ab_win.exists(): - local_ab = local_ab_win - install_cmd = ( - [str(local_ab), "install", "--with-deps"] - if local_ab.exists() - else [npx_bin, "-y", "agent-browser", "install", "--with-deps"] - ) try: result = subprocess.run( install_cmd, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 78eed1d2c6..61cadd7b20 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2023,12 +2023,13 @@ def _npm_manifest_paths() -> tuple[Path, ...]: The workspace list is pulled from the root package.json's `workspaces` globs (npm's own source of truth) rather than hardcoded, so adding a - workspace can never silently escape the skip key. The root install - (step 1, --workspaces=false) still hoists shared deps for EVERY - workspace — desktop included — so all of them belong in the key, not - just the ones step 2 installs. Falls back to hashing just root - manifests if package.json is unreadable (never skips more than main - would have installed). + workspace can never silently escape the skip key. Every workspace + manifest belongs in the key — desktop included, even though the + install only names ui-tui and web — because the single lockfile spans + the whole workspace graph, so any manifest edit can put the lockfile + out of sync and change what the install must do. Falls back to hashing + just root manifests if package.json is unreadable (never skips more + than main would have installed). """ root_pkg = _m().PROJECT_ROOT / "package.json" paths = [_m().PROJECT_ROOT / "package-lock.json", root_pkg] @@ -2102,7 +2103,7 @@ def _record_npm_lockfile_hash(hermes_root: Path) -> None: logger.debug("Could not write npm lockfile hash cache") def _update_node_dependencies() -> list[str]: - """Refresh Node deps in the repo root and update workspaces. + """Refresh Node deps for the ui-tui and web workspaces. Returns the list of labels whose npm install failed (empty on success), so the caller can treat a Node refresh failure as a partial update rather @@ -2124,7 +2125,7 @@ def _update_node_dependencies() -> list[str]: print(" ⚠ Skipped: only a Windows npm is reachable from this WSL shell.") print(" Install Node.js inside the WSL distro (nvm, or your distro's") print(" package manager), then re-run `hermes update`.") - failed = ["repo root"] + failed = [] if any( (_m().PROJECT_ROOT / workspace / "package.json").exists() for workspace in ("ui-tui", "web") @@ -2143,12 +2144,15 @@ def _update_node_dependencies() -> list[str]: logger.info("npm lockfile unchanged, skipping npm install") return [] - # With a single workspace lockfile the root install would cover ALL - # workspaces — but apps/desktop pulls in Electron as a devDependency, - # and its postinstall downloads a ~200MB binary. Most users don't - # need desktop during `hermes update`, so we install root-only first - # then add just the workspaces the CLI/TUI/web build actually requires. - # Desktop deps are installed on demand by the desktop launcher + # Root package.json has no dependencies of its own (agent-browser and + # @streamdown/math were moved out — see #43564): agent-browser resolves + # at runtime via `npx agent-browser` (tools/browser_tool.py), and + # @streamdown/math is a desktop-only import now declared in + # apps/desktop/package.json. That means a plain workspace-scoped install + # can never prune anything root-only, so we only need to name the + # workspaces the CLI/TUI/web build actually requires. apps/desktop pulls + # in Electron as a devDependency with a ~200MB postinstall download, so + # it's deliberately never named here — desktop deps install on demand # (see _desktop_build_needed). print("→ Updating Node.js dependencies...") @@ -2159,53 +2163,50 @@ def _update_node_dependencies() -> list[str]: print(" deps). Fix npm and re-run `hermes update`.") return list(labels) - extra_args = ["--no-fund", "--no-audit", "--prefer-offline", "--progress=false"] + install_args = [ + "--no-fund", "--no-audit", "--prefer-offline", "--progress=false", + "--workspace", "ui-tui", "--workspace", "web", + ] from hermes_constants import with_hermes_node_path nixos_env = with_hermes_node_path(_m()._nixos_build_env()) - # Step 1: root install (no workspace recursion). # NOTE: capture_output=False here is deliberate (#18840) — optional - # postinstall scripts (e.g. @askjo/camofox-browser's browser-binary fetch) - # print download progress, and capturing it makes a long download look - # hung. The chatty npm-deprecation noise during `hermes update` comes from - # the *desktop* build, not this step; that one is captured to update.log. - root_args = [*extra_args, "--workspaces=false"] - root_result = _m()._run_npm_install_deterministic( + # postinstall scripts print download progress, and capturing it makes a + # long download look hung. The chatty npm-deprecation noise during + # `hermes update` comes from the *desktop* build, not this step; that + # one is captured to update.log. + result = _m()._run_npm_install_deterministic( npm, _m().PROJECT_ROOT, - extra_args=tuple(root_args), + extra_args=tuple(install_args), capture_output=False, env=nixos_env, ) - if root_result.returncode != 0: - print(" ⚠ npm install failed in repo root") - stderr = (root_result.stderr or "").strip() if root_result.stderr else "" + if result.returncode == 0: + _record_npm_lockfile_hash(shared_hermes_root) + print(" ✓ ui-tui, web workspaces installed (desktop skipped)") + failures: list[str] = [] + else: + print(" ⚠ npm install failed") + stderr = (result.stderr or "").strip() if result.stderr else "" if stderr: print(f" {stderr.splitlines()[-1]}") - return _partial_update_failure("repo root") + failures = _partial_update_failure("ui-tui, web workspaces") - # Step 2: install only the workspaces update needs (ui-tui, web). - # --workspace selects specific workspaces; the rest (desktop) are skipped. - ws_args = [*extra_args, "--workspace", "ui-tui", "--workspace", "web"] - ws_result = _m()._run_npm_install_deterministic( - npm, - _m().PROJECT_ROOT, - extra_args=tuple(ws_args), - capture_output=False, - env=nixos_env, - ) - if ws_result.returncode == 0: - _record_npm_lockfile_hash(shared_hermes_root) - print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") - return [] + # Fire-and-forget: warm npx's cache for agent-browser so the first + # browser-tool call in a session doesn't pay a registry fetch that used + # to happen here for free back when agent-browser was an eager root + # dependency (see #43564). Independent of the workspace install result + # above — never blocks or fails `hermes update`. + try: + from tools.browser_tool import warm_agent_browser_npx_cache + warm_agent_browser_npx_cache() + except Exception: + pass - print(" ⚠ npm workspace install failed") - stderr = (ws_result.stderr or "").strip() if ws_result.stderr else "" - if stderr: - print(f" {stderr.splitlines()[-1]}") - return _partial_update_failure("ui-tui, web workspaces") + return failures def _log_only_write(text: str) -> None: """Write ``text`` to ``~/.hermes/logs/update.log`` only, never the terminal. diff --git a/package-lock.json b/package-lock.json index a3bbbaacd1..090b2a7bdd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,10 +16,6 @@ "web", "tests-js" ], - "dependencies": { - "@streamdown/math": "1.0.2", - "agent-browser": "0.26.0" - }, "devDependencies": { "@eslint/js": "9.39.5", "eslint-plugin-perfectionist": "5.10.0", @@ -90,6 +86,7 @@ "@nanostores/react": "1.1.0", "@nous-research/ui": "0.18.2", "@streamdown/code": "1.1.1", + "@streamdown/math": "1.0.2", "@tabler/icons-react": "3.44.0", "@tailwindcss/typography": "0.5.20", "@tailwindcss/vite": "4.3.3", @@ -7053,16 +7050,6 @@ "node": ">= 14" } }, - "node_modules/agent-browser": { - "version": "0.26.0", - "resolved": "https://registry.npmjs.org/agent-browser/-/agent-browser-0.26.0.tgz", - "integrity": "sha512-pdqSfjwbFSp+qnwlb2g23e9wXveIOfMi19xpPA9xZUbzEAUp6W4YBZj6Ybj8z4M7WkcbGDDYc+oDIHDt9R3EDQ==", - "hasInstallScript": true, - "license": "Apache-2.0", - "bin": { - "agent-browser": "bin/agent-browser.js" - } - }, "node_modules/aggregate-error": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/aggregate-error/-/aggregate-error-3.1.0.tgz", diff --git a/package.json b/package.json index 32f88de823..0ad8e0cbfd 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "tests-js" ], "scripts": { - "postinstall": "echo '\u2705 Browser tools ready. Run: python run_agent.py --help'", + "postinstall": "echo '\u2705 Node dependencies installed. Run: python run_agent.py --help'", "install:root": "npm install --workspaces=false", "install:web": "npm install --workspace web", "install:tui": "npm install --workspace ui-tui", @@ -34,10 +34,6 @@ "url": "https://github.com/NousResearch/Hermes-Agent/issues" }, "homepage": "https://github.com/NousResearch/Hermes-Agent#readme", - "dependencies": { - "@streamdown/math": "1.0.2", - "agent-browser": "0.26.0" - }, "devDependencies": { "@eslint/js": "9.39.5", "typescript-eslint": "8.64.0", @@ -71,7 +67,6 @@ "esbuild@0.28.1": true, "node-pty@1.1.0": true, "electron-winstaller@5.4.0": true, - "agent-browser@0.26.0": true, "electron@40.10.2": true, "fsevents@2.3.2": true, "fsevents@2.3.3": true, diff --git a/tests-js/package-json-lazy-deps.test.ts b/tests-js/package-json-lazy-deps.test.ts index d00491f6e1..dfba61572f 100644 --- a/tests-js/package-json-lazy-deps.test.ts +++ b/tests-js/package-json-lazy-deps.test.ts @@ -4,14 +4,29 @@ * The root ``package.json`` is installed by ``hermes update`` on every user, * including users who never opted into a given browser backend. Anything * listed in ``dependencies`` therefore runs its npm postinstall script for - * everyone — including binary-fetching backends, on every update. + * everyone, and — per #43564 — is also part of the npm workspace install + * graph, where a workspace-scoped ``npm ci`` (``--workspace ui-tui + * --workspace web``) can silently prune it right back out on the next + * ``hermes update``. * * The contract: * - * - ``agent-browser`` IS eager. It is the default Chromium-driving backend - * used whenever the agent makes a browser call without a cloud provider - * configured, so it must already be installed before any session starts. - * Its postinstall is also small. + * - ``agent-browser`` is NOT a root dependency. It used to be eager (see + * #27055, which reasoned its postinstall was small enough to keep eager + * unlike Camofox's) but #43564 found that keeping ANY dependency in root + * ``package.json`` — however small its postinstall — entangles it with + * the ui-tui/web workspace install and risks it being pruned. It now + * resolves at runtime via ``npx agent-browser`` (see + * ``tools/browser_tool.py::_find_agent_browser``), which sidesteps the + * workspace graph entirely. ``hermes update`` and ``hermes doctor --fix`` + * both fire-and-forget ``warm_agent_browser_npx_cache()`` to keep npx's + * own cache warm, preserving the "available before any session starts" + * property #27055 cared about without re-entangling the dependency. + * + * - ``@streamdown/math`` is NOT a root dependency either. It's imported only + * by desktop's own TS code (``apps/desktop/src/...``), so it belongs in + * ``apps/desktop/package.json`` (alongside its sibling ``@streamdown/code``) + * — not root, where it was subject to the exact same pruning risk. * * - ``@askjo/camofox-browser`` is NOT eager. It is an explicit opt-in * alternative browser backend, selected by the user via @@ -23,10 +38,9 @@ * installed on demand by ``tools_config.py`` ``post_setup_key == * "camofox"`` when the user actually selects Camofox. * - * If a future PR re-adds Camofox (or any other binary-postinstall package) - * to root ``dependencies``, this test fails — read the lazy-install - * guidance in the ``hermes-agent-dev`` skill before changing the - * expectations. + * If a future PR re-adds any of these to root ``dependencies``, this test + * fails — read the lazy-install guidance in the ``hermes-agent-dev`` skill + * before changing the expectations. */ import assert from 'node:assert/strict' @@ -38,6 +52,7 @@ import { test } from 'vitest' const REPO_ROOT = path.resolve(__dirname, '..') const ROOT_PKG = path.join(REPO_ROOT, 'package.json') const ROOT_LOCK = path.join(REPO_ROOT, 'package-lock.json') +const DESKTOP_PKG = path.join(REPO_ROOT, 'apps', 'desktop', 'package.json') function rootPackageJson(): Record { return JSON.parse(fs.readFileSync(ROOT_PKG, 'utf-8')) @@ -55,15 +70,41 @@ test('camofox is not in root dependencies (must stay opt-in)', () => { ) }) -test('agent-browser stays eager (default backend)', () => { +test('agent-browser is not in root dependencies (resolves via npx, #43564)', () => { const deps = (rootPackageJson().dependencies ?? {}) as Record assert.ok( - 'agent-browser' in deps, - 'agent-browser is the default browser-tool backend used by every ' + - 'session that doesn\'t have a cloud browser provider configured. ' + - 'It must stay in root package.json dependencies so it is present ' + - 'after `hermes setup` / `hermes update` without an explicit ' + - 'post_setup step.' + !('agent-browser' in deps), + 'agent-browser must not be a root package.json dependency — it ' + + 'resolves lazily via `npx agent-browser` instead (see ' + + 'tools/browser_tool.py::_find_agent_browser and ' + + 'warm_agent_browser_npx_cache). Putting it back in root ' + + 'dependencies re-entangles it with the ui-tui/web workspace ' + + 'install graph and reintroduces #43564.' + ) +}) + +test('@streamdown/math is not in root dependencies (desktop-only import)', () => { + const deps = (rootPackageJson().dependencies ?? {}) as Record + assert.ok( + !('@streamdown/math' in deps), + '@streamdown/math is only imported by apps/desktop\'s own TS code ' + + '(markdown-text.tsx, katex-memo.ts) — it belongs in ' + + 'apps/desktop/package.json alongside its sibling @streamdown/code, ' + + 'not root, where it\'s subject to the same workspace-pruning risk ' + + 'agent-browser had (#43564).' + ) +}) + +test('@streamdown/math is in desktop dependencies', () => { + const deps = (JSON.parse(fs.readFileSync(DESKTOP_PKG, 'utf-8')).dependencies ?? + {}) as Record + + assert.ok( + '@streamdown/math' in deps, + '@streamdown/math is imported by apps/desktop\'s own TS code ' + + '(markdown-text.tsx, katex-memo.ts) and must be declared in ' + + 'apps/desktop/package.json now that it is no longer a root ' + + 'dependency.' ) }) @@ -87,3 +128,20 @@ test('root lockfile has no camofox entries', () => { '@askjo/camofox-browser). Regenerate the lockfile.' ) }) + +test('root lockfile has no agent-browser entry (#43564)', () => { + if (!fs.existsSync(ROOT_LOCK)) { + // Some CI matrix shards skip lockfile materialization. + return + } + + const text = fs.readFileSync(ROOT_LOCK, 'utf-8') + assert.ok( + !text.includes('"node_modules/agent-browser"'), + 'package-lock.json still has a node_modules/agent-browser entry. ' + + 'It must resolve lazily via `npx agent-browser` instead — ' + + 'regenerate the lockfile after removing the dep: ' + + '`rm package-lock.json && npm install --package-lock-only ' + + '--ignore-scripts --no-fund --no-audit`.' + ) +}) diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 5386e85a47..7f3ae53ef3 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -234,7 +234,6 @@ class TestCmdUpdateBranchFallback: captured = capsys.readouterr() assert "Already up to date!" in captured.out - def test_update_non_interactive_runs_safe_config_migrations(self, mock_args, capsys): """Dashboard/web updates apply non-interactive migrations before restart.""" with patch("shutil.which", return_value=None), patch( @@ -744,13 +743,14 @@ class TestNodeRuntimeNpmResolution: lambda *a, **k: subprocess.CompletedProcess([], 1, stdout="", stderr=""), ) - failed = hm._update_node_dependencies() - assert failed == ["repo root"] + with patch( + "tools.browser_tool.warm_agent_browser_npx_cache", return_value=True + ): + failed = hm._update_node_dependencies() + assert failed == ["ui-tui, web workspaces"] out = capsys.readouterr().out assert "mixed state" in out - - def test_wsl_update_skips_windows_npm_build_paths(self, mock_args, monkeypatch): """A Windows-only npm on WSL must not reach web or desktop builds.""" from hermes_cli import main as hm @@ -790,3 +790,212 @@ class TestNodeRuntimeNpmResolution: not call.args or not call.args[0] or call.args[0][0] != windows_npm for call in mock_run.call_args_list ) + + +class TestUpdateNodeDependencies: + """Unit tests for _update_node_dependencies — issue #43564. + + Root package.json has no dependencies of its own: agent-browser + resolves at runtime via npx (tools/browser_tool.py), and @streamdown/math + moved to apps/desktop/package.json since it's a desktop-only import. + With nothing root-only left to protect, a single workspace-scoped + install (ui-tui, web) is safe — apps/desktop is simply never named, so + its ~200 MB Electron devDependency is never resolved. Skipping is + governed by _npm_lockfile_changed (content hash over the lockfile + + every workspace package.json), tested separately in + TestNpmLockfileChanged. + Uses a tmp_path root so tests never touch real node_modules. + """ + + @pytest.fixture(autouse=True) + def _stub_npx_warmup(self): + """The npx cache warm-up is covered by its own dedicated test below; + stub it out everywhere else so it doesn't add a spurious npm/npx + call to the workspace-install assertions in this class.""" + with patch("tools.browser_tool.warm_agent_browser_npx_cache", return_value=True): + yield + + def _npm_calls(self, mock_run): + return [ + call.args[0] + for call in mock_run.call_args_list + if call.args and "npm" in str(call.args[0][0]) + ] + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_install_names_ui_tui_and_web_workspaces(self, _which, mock_run, tmp_path, monkeypatch): + """Regression for #43564: install ui-tui + web directly. apps/desktop + must never appear, so its Electron postinstall is never triggered. + """ + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: True) + mock_run.return_value = subprocess.CompletedProcess([], 0, stdout="", stderr="") + + hm._update_node_dependencies() + + calls = self._npm_calls(mock_run) + assert len(calls) == 1, f"expected exactly 1 npm call, got: {calls}" + joined = " ".join(str(a) for a in calls[0]) + assert "--workspace ui-tui" in joined and "--workspace web" in joined, ( + f"expected ui-tui + web workspace selectors; actual: {calls[0]}" + ) + assert "desktop" not in joined, ( + f"apps/desktop must not appear (avoids ~200 MB Electron download); actual: {calls[0]}" + ) + assert "--workspaces=false" not in joined, ( + f"no root-only deps remain to protect; --workspaces=false is unnecessary now; actual: {calls[0]}" + ) + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_install_preserves_standard_flags(self, _which, mock_run, tmp_path, monkeypatch): + """--no-fund, --no-audit, --progress=false must survive.""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: True) + mock_run.return_value = subprocess.CompletedProcess([], 0, stdout="", stderr="") + + hm._update_node_dependencies() + + calls = self._npm_calls(mock_run) + assert len(calls) == 1 + joined = " ".join(str(a) for a in calls[0]) + for flag in ("--no-fund", "--no-audit", "--progress=false"): + assert flag in joined, f"{flag} missing from npm call; actual: {calls[0]}" + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_skips_install_when_deps_up_to_date(self, _which, mock_run, tmp_path, monkeypatch): + """When _npm_lockfile_changed reports no change, npm must not be called.""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: False) + + hm._update_node_dependencies() + + assert not self._npm_calls(mock_run), ( + "npm must not run when _npm_lockfile_changed reports no change" + ) + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_runs_install_when_lockfile_changed(self, _which, mock_run, tmp_path, monkeypatch): + """When _npm_lockfile_changed reports a change, npm must run.""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: True) + mock_run.return_value = subprocess.CompletedProcess([], 0, stdout="", stderr="") + + hm._update_node_dependencies() + + calls = self._npm_calls(mock_run) + assert len(calls) == 1, f"expected npm to run when lockfile changed; got: {calls}" + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_records_lockfile_hash_only_on_success(self, _which, mock_run, tmp_path, monkeypatch): + """A failed install must not record the lockfile hash (so the next + run retries instead of wrongly believing deps are up to date).""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: True) + recorded = [] + monkeypatch.setattr(hm, "_record_npm_lockfile_hash", lambda root: recorded.append(root)) + mock_run.return_value = subprocess.CompletedProcess( + [], 1, stdout="", stderr="npm ERR!" + ) + + hm._update_node_dependencies() + + assert not recorded, "lockfile hash must not be recorded when npm install fails" + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_warms_npx_agent_browser_cache_regardless_of_install_result( + self, _which, mock_run, tmp_path, monkeypatch + ): + """The npx warm-up must fire even when the workspace install fails — + it's independent of ui-tui/web dependency state (#43564).""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_npm_lockfile_changed", lambda root: True) + mock_run.return_value = subprocess.CompletedProcess( + [], 1, stdout="", stderr="npm ERR!" + ) + + with patch( + "tools.browser_tool.warm_agent_browser_npx_cache", return_value=True + ) as mock_warm: + hm._update_node_dependencies() + + mock_warm.assert_called_once() + + @patch("subprocess.run") + @patch("shutil.which", return_value=None) + def test_returns_silently_when_npm_not_found(self, _which, mock_run, tmp_path, monkeypatch): + """No npm on PATH → return without calling subprocess.""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + + hm._update_node_dependencies() + + mock_run.assert_not_called() + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_returns_silently_when_package_json_absent(self, _which, mock_run, tmp_path, monkeypatch): + """No package.json → return without calling npm.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + + hm._update_node_dependencies() + + mock_run.assert_not_called() + + @patch("subprocess.run") + @patch("shutil.which", return_value="/usr/bin/npm") + def test_install_runs_from_project_root(self, _which, mock_run, tmp_path, monkeypatch): + """npm install must execute from PROJECT_ROOT, not a workspace subdir.""" + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + (tmp_path / "package-lock.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + + cwd_calls = [] + + def capture(cmd, **kwargs): + if "npm" in str(cmd[0]): + cwd_calls.append(kwargs.get("cwd")) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + mock_run.side_effect = capture + + hm._update_node_dependencies() + + assert cwd_calls, "expected at least one npm call" + for cwd in cwd_calls: + assert cwd == tmp_path, f"npm must run from PROJECT_ROOT; got cwd={cwd}" diff --git a/tests/hermes_cli/test_doctor.py b/tests/hermes_cli/test_doctor.py index c0d7316fd7..3cae143d8b 100644 --- a/tests/hermes_cli/test_doctor.py +++ b/tests/hermes_cli/test_doctor.py @@ -680,41 +680,24 @@ def test_run_doctor_termux_does_not_mark_browser_available_without_agent_browser assert "npm install -g agent-browser && agent-browser install" in out -def _run_doctor_with_managed_agent_browser(monkeypatch, tmp_path, runnable): - """Set up run_doctor with node present, agent-browser only in the - Hermes-managed node bin (~/.hermes/node/bin), not on PATH or in - PROJECT_ROOT/node_modules. Returns the captured stdout.""" +def _doctor_env_for_agent_browser(monkeypatch, tmp_path): + """Shared non-Termux fixture setup for the agent-browser npx-resolution + branch in run_doctor (hermes_cli/doctor.py ~1557-1605).""" home = tmp_path / ".hermes" - (home / "node" / "bin").mkdir(parents=True, exist_ok=True) + home.mkdir(parents=True, exist_ok=True) (home / "config.yaml").write_text("memory: {}\n", encoding="utf-8") - managed_ab = home / "node" / "bin" / "agent-browser" - managed_ab.write_text("#!/bin/sh\n", encoding="utf-8") - managed_ab.chmod(0o755) project = tmp_path / "project" - project.mkdir(exist_ok=True) # no node_modules/agent-browser here + project.mkdir(exist_ok=True) monkeypatch.delenv("TERMUX_VERSION", raising=False) - monkeypatch.delenv("PREFIX", raising=False) + monkeypatch.setenv("PREFIX", "/usr") monkeypatch.setattr(doctor_mod, "HERMES_HOME", home) monkeypatch.setattr(doctor_mod, "PROJECT_ROOT", project) monkeypatch.setattr(doctor_mod, "_DHH", str(home)) - - # node on PATH, agent-browser is NOT on PATH (only in the managed bin). - # The managed-dir rung resolves via shutil.which(..., path=) so - # Windows picks the .cmd shim — mirror that shape here. - def _fake_which(cmd, path=None): - if path is not None: - if cmd == "agent-browser" and str(managed_ab.parent) == str(path): - return str(managed_ab) - return None - return "/usr/bin/node" if cmd in {"node", "npm"} else None - - monkeypatch.setattr(doctor_mod.shutil, "which", _fake_which) - # agent_browser_runnable is imported into doctor's namespace monkeypatch.setattr( - doctor_mod, - "agent_browser_runnable", - lambda path: runnable and str(path) == str(managed_ab), + doctor_mod.shutil, + "which", + lambda cmd: "/usr/bin/node" if cmd in {"node", "npm"} else None, ) fake_model_tools = types.SimpleNamespace( @@ -731,10 +714,74 @@ def _run_doctor_with_managed_agent_browser(monkeypatch, tmp_path, runnable): except Exception: pass + +def test_run_doctor_reports_agent_browser_resolves_via_npx(monkeypatch, tmp_path): + """When agent-browser has no local/global install, _find_agent_browser + falls through to 'npx agent-browser' — doctor must report that as OK + (#43564: agent-browser is no longer a root package.json dependency, so + this is the expected common case now, not a warning).""" + _doctor_env_for_agent_browser(monkeypatch, tmp_path) + + import tools.browser_tool as bt + monkeypatch.setattr(bt, "_find_agent_browser", lambda **_kw: "npx agent-browser") + warm_calls = [] + monkeypatch.setattr( + bt, "warm_agent_browser_npx_cache", lambda *a, **kw: warm_calls.append(1) or True + ) + buf = io.StringIO() with contextlib.redirect_stdout(buf): doctor_mod.run_doctor(Namespace(fix=False)) - return buf.getvalue() + out = buf.getvalue() + + assert "agent-browser" in out + assert "resolves via npx on first use" in out + assert "agent-browser not installed" not in out + # --fix was not requested: the warm-up must not fire on a plain check. + assert not warm_calls + + +def test_run_doctor_fix_warms_npx_cache_when_agent_browser_resolves_via_npx( + monkeypatch, tmp_path +): + """`hermes doctor --fix` must actually call warm_agent_browser_npx_cache() + when agent-browser resolves via npx, and report success.""" + _doctor_env_for_agent_browser(monkeypatch, tmp_path) + + import tools.browser_tool as bt + monkeypatch.setattr(bt, "_find_agent_browser", lambda **_kw: "npx agent-browser") + warm_calls = [] + monkeypatch.setattr( + bt, "warm_agent_browser_npx_cache", lambda *a, **kw: warm_calls.append(1) or True + ) + + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + doctor_mod.run_doctor(Namespace(fix=True)) + out = buf.getvalue() + + assert warm_calls, "warm_agent_browser_npx_cache() must be called under --fix" + assert "Warmed npx cache for agent-browser" in out + assert "Could not warm npx cache" not in out + + +def test_run_doctor_fix_reports_when_npx_warmup_fails(monkeypatch, tmp_path): + """If warm_agent_browser_npx_cache() fails (offline, npx missing from + PATH at call time, etc.), doctor must say so instead of silently + claiming success — and must not count it as a fix.""" + _doctor_env_for_agent_browser(monkeypatch, tmp_path) + + import tools.browser_tool as bt + monkeypatch.setattr(bt, "_find_agent_browser", lambda **_kw: "npx agent-browser") + monkeypatch.setattr(bt, "warm_agent_browser_npx_cache", lambda *a, **kw: False) + + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + doctor_mod.run_doctor(Namespace(fix=True)) + out = buf.getvalue() + + assert "Could not warm npx cache (offline or npx unavailable)" in out + assert "Warmed npx cache for agent-browser" not in out def test_run_doctor_kimi_cn_env_is_detected_and_probe_is_null_safe(monkeypatch, tmp_path): diff --git a/tests/hermes_cli/test_tools_config.py b/tests/hermes_cli/test_tools_config.py index 339e498503..6253c9cbcb 100644 --- a/tests/hermes_cli/test_tools_config.py +++ b/tests/hermes_cli/test_tools_config.py @@ -1,6 +1,7 @@ """Tests for hermes_cli.tools_config platform tool persistence.""" import logging +import subprocess from types import SimpleNamespace from unittest.mock import patch @@ -345,6 +346,261 @@ def test_numeric_mcp_server_name_does_not_crash_sorted(): +class TestAgentBrowserPostSetup: + """_run_post_setup('agent_browser'/'browserbase') — #43564. + + agent-browser is no longer a root package.json dependency (there's no + local `npm install` step anymore); it resolves at runtime via + tools.browser_tool._find_agent_browser (PATH -> Homebrew/Hermes-managed + node -> local .bin -> npx). This class exercises the Chromium-install + branch of _run_post_setup, which now delegates to that same resolution + cascade instead of hand-rolling its own node_modules/.bin/agent-browser + (and Windows .cmd-shim) lookup. + """ + + def test_warns_when_neither_npx_nor_agent_browser_on_path(self): + with patch("shutil.which", return_value=None), patch( + "subprocess.run" + ) as run, patch("hermes_cli.tools_config._print_warning") as warn: + _run_post_setup("agent_browser") + + run.assert_not_called() + warn.assert_called_once() + assert "npx not found" in warn.call_args.args[0] + + def test_browserbase_returns_before_any_chromium_check(self): + """browserbase hosts its own Chromium; it must never reach the + agent-browser-only Chromium-install branch.""" + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed" + ) as chromium_check: + _run_post_setup("browserbase") + + run.assert_not_called() + chromium_check.assert_not_called() + + def test_chromium_already_installed_skips_subprocess(self): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed", return_value=True + ), patch( + "hermes_cli.tools_config._print_success" + ) as success: + _run_post_setup("agent_browser") + + run.assert_not_called() + success.assert_called_once() + assert "already installed" in success.call_args.args[0] + + def test_docker_with_missing_chromium_warns_instead_of_installing(self): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=True + ), patch( + "hermes_cli.tools_config._print_warning" + ) as warn: + _run_post_setup("agent_browser") + + run.assert_not_called() + assert any("Docker" in c.args[0] for c in warn.call_args_list) + + def test_find_agent_browser_not_found_warns_before_any_chromium_check(self): + """_find_agent_browser is resolved up front now (shared with the + browserbase early-return gate), so a FileNotFoundError here must + short-circuit before even checking Chromium/Docker status.""" + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed" + ) as chromium_check, patch( + "tools.browser_tool._running_in_docker" + ) as docker_check, patch( + "tools.browser_tool._find_agent_browser", + side_effect=FileNotFoundError("agent-browser CLI not found"), + ), patch( + "hermes_cli.tools_config._print_warning" + ) as warn: + _run_post_setup("agent_browser") + + run.assert_not_called() + chromium_check.assert_not_called() + docker_check.assert_not_called() + assert any("browser tools require Node.js" in c.args[0] for c in warn.call_args_list) + + def test_installs_chromium_via_npx_when_no_local_binary_resolved(self): + """When _find_agent_browser falls through to npx, the install command + must shell out to npx directly (not the unresolved 'npx agent-browser' + string as a single argv element).""" + with patch( + "shutil.which", + side_effect=lambda name: "/usr/bin/npx" if name == "npx" else None, + ), patch("subprocess.run") as run, patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "hermes_cli.tools_config._print_success" + ): + run.return_value = SimpleNamespace(returncode=0, stdout="", stderr="") + _run_post_setup("agent_browser") + + run.assert_called_once() + assert run.call_args.args[0] == [ + "/usr/bin/npx", "-y", "agent-browser", "install", "--with-deps", + ] + + def test_installs_chromium_via_npx_resolved_only_through_extended_path(self): + """Hermes-managed-Node-only setups: npx resolves via + _find_agent_browser's extended-PATH fallback, not a bare PATH lookup. + The install command must use that same resolved npx, not silently + hand subprocess.run a None argument from a bare shutil.which('npx') + re-derivation (#43564 regression — Copilot review, task #9).""" + hermes_npx = "/home/user/.hermes/node/bin/npx" + with patch("shutil.which", return_value=None), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "tools.browser_tool._resolve_npx_bin", return_value=hermes_npx + ), patch( + "hermes_cli.tools_config._print_success" + ): + run.return_value = SimpleNamespace(returncode=0, stdout="", stderr="") + _run_post_setup("agent_browser") + + run.assert_called_once() + assert run.call_args.args[0] == [ + hermes_npx, "-y", "agent-browser", "install", "--with-deps", + ] + + def test_warns_instead_of_crashing_when_npx_unresolvable_after_all(self): + """Defensive: if _resolve_npx_bin somehow returns None even though + _find_agent_browser resolved "npx agent-browser" (e.g. a race where + npx disappears between the two calls), warn and return instead of + building a command with a None argv element.""" + with patch("shutil.which", return_value=None), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "tools.browser_tool._resolve_npx_bin", return_value=None + ), patch( + "hermes_cli.tools_config._print_warning" + ) as warn: + _run_post_setup("agent_browser") # must not raise + + run.assert_not_called() + assert any("npx not found" in c.args[0] for c in warn.call_args_list) + + def test_installs_chromium_via_resolved_local_binary_path(self): + """When _find_agent_browser resolves a concrete executable (global + install, Homebrew, or the Windows .cmd shim it already knows how to + pick), that path must be invoked directly — not re-wrapped in npx.""" + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run" + ) as run, patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", + return_value="/usr/local/bin/agent-browser", + ), patch( + "hermes_cli.tools_config._print_success" + ): + run.return_value = SimpleNamespace(returncode=0, stdout="", stderr="") + _run_post_setup("agent_browser") + + run.assert_called_once() + assert run.call_args.args[0] == [ + "/usr/local/bin/agent-browser", "install", "--with-deps", + ] + + def test_install_success_invalidates_chromium_cache(self): + import tools.browser_tool as _bt + + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + return_value=SimpleNamespace(returncode=0, stdout="", stderr=""), + ), patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "hermes_cli.tools_config._print_success" + ): + _bt._cached_chromium_installed = True + _run_post_setup("agent_browser") + + assert _bt._cached_chromium_installed is None, ( + "a successful install must invalidate the cached chromium-missing " + "result so the next check_browser_requirements() call re-probes" + ) + + def test_install_failure_prints_stderr_tail_and_does_not_invalidate_cache(self): + import tools.browser_tool as _bt + + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + return_value=SimpleNamespace( + returncode=1, stdout="", stderr="line1\nline2\nfatal: network error" + ), + ), patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "hermes_cli.tools_config._print_warning" + ) as warn, patch( + "hermes_cli.tools_config._print_info" + ) as info: + _bt._cached_chromium_installed = "sentinel" + _run_post_setup("agent_browser") + + assert any("Chromium install failed" in c.args[0] for c in warn.call_args_list) + assert any("fatal: network error" in c.args[0] for c in info.call_args_list) + assert _bt._cached_chromium_installed == "sentinel", ( + "a failed install must not invalidate the chromium cache" + ) + + def test_install_timeout_warns_without_raising(self): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd=["npx"], timeout=600), + ), patch( + "tools.browser_tool._chromium_installed", return_value=False + ), patch( + "tools.browser_tool._running_in_docker", return_value=False + ), patch( + "tools.browser_tool._find_agent_browser", return_value="npx agent-browser" + ), patch( + "hermes_cli.tools_config._print_warning" + ) as warn: + _run_post_setup("agent_browser") # must not raise + + assert any("timed out" in c.args[0] for c in warn.call_args_list) + + class TestImagegenBackendRegistry: """IMAGEGEN_BACKENDS tags drive the model picker flow in tools_config.""" diff --git a/tests/tools/test_browser_npx_warmup.py b/tests/tools/test_browser_npx_warmup.py new file mode 100644 index 0000000000..5f142dbea4 --- /dev/null +++ b/tests/tools/test_browser_npx_warmup.py @@ -0,0 +1,75 @@ +"""Tests for tools.browser_tool.warm_agent_browser_npx_cache (#43564). + +agent-browser was moved out of root package.json dependencies and now +resolves lazily via `npx agent-browser`. warm_agent_browser_npx_cache() is +the fire-and-forget helper `hermes update` / `hermes doctor --fix` call to +pre-fetch it so the first real browser-tool invocation in a session doesn't +pay npx's registry-lookup cost. It must never raise and must accurately +report success/failure via its return value — hermes_cli/doctor.py's +`--fix` path branches on that return value; hermes_cli/update_cmd.py calls +it fire-and-forget inside its own try/except and ignores the result. +""" + +from __future__ import annotations + +import subprocess +from unittest.mock import patch + +from tools.browser_tool import warm_agent_browser_npx_cache + + +def test_returns_false_without_calling_subprocess_when_npx_missing(): + with patch("shutil.which", return_value=None), patch( + "subprocess.run" + ) as mock_run: + assert warm_agent_browser_npx_cache() is False + mock_run.assert_not_called() + + +def test_invokes_npx_with_prefer_offline_version_check(): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + return_value=subprocess.CompletedProcess([], 0, stdout="1.2.3\n", stderr=""), + ) as mock_run: + assert warm_agent_browser_npx_cache() is True + + mock_run.assert_called_once() + args, kwargs = mock_run.call_args + assert args[0] == ["/usr/bin/npx", "--prefer-offline", "-y", "agent-browser", "--version"] + assert kwargs.get("check") is False + + +def test_custom_timeout_is_forwarded_to_subprocess_run(): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + return_value=subprocess.CompletedProcess([], 0, stdout="", stderr=""), + ) as mock_run: + warm_agent_browser_npx_cache(timeout=5.0) + + assert mock_run.call_args.kwargs.get("timeout") == 5.0 + + +def test_returns_false_on_nonzero_exit(): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + return_value=subprocess.CompletedProcess([], 1, stdout="", stderr="registry unreachable"), + ): + assert warm_agent_browser_npx_cache() is False + + +def test_returns_false_instead_of_raising_on_timeout(): + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd=["npx"], timeout=60.0), + ): + assert warm_agent_browser_npx_cache() is False + + +def test_returns_false_instead_of_raising_on_unexpected_exception(): + """Fire-and-forget contract: hermes_cli/doctor.py calls this bare (no + try/except of its own), so any exception inside the subprocess call + must be swallowed here, not propagated.""" + with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "subprocess.run", side_effect=OSError("fork failed") + ): + assert warm_agent_browser_npx_cache() is False diff --git a/tools/browser_tool.py b/tools/browser_tool.py index e0f124231f..b5afd3dba1 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -2350,6 +2350,23 @@ def _agent_browser_candidate_present(path: str | None) -> bool: return os.path.exists(path) and (os.name == "nt" or os.access(path, os.X_OK)) +def _resolve_npx_bin() -> Optional[str]: + """Resolve the npx binary via the same PATH + extended-PATH cascade + _find_agent_browser uses, so callers that need npx's actual path + (rather than the "npx agent-browser" sentinel) can't diverge from what + _find_agent_browser itself would have found. A bare ``shutil.which("npx")`` + misses Hermes-managed-Node-only setups where npx only resolves via the + extended fallback PATH (Homebrew, $HERMES_HOME/node, etc.). + """ + npx_path = shutil.which("npx") + if npx_path: + return npx_path + extended_path = _merge_browser_path("") + if extended_path: + return shutil.which("npx", path=extended_path) + return None + + def _find_agent_browser(*, validate: bool = True) -> str: """ Find the agent-browser CLI executable. @@ -2369,7 +2386,6 @@ def _find_agent_browser(*, validate: bool = True) -> str: raise FileNotFoundError( "agent-browser CLI not found (cached). Install it with: " f"{_browser_install_hint()}\n" - "Or run 'npm install' in the repo root to install locally.\n" "Or ensure npx is available in your PATH." ) return _cached_agent_browser @@ -2434,9 +2450,7 @@ def _find_agent_browser(*, validate: bool = True) -> str: return _cached_agent_browser # Check common npx locations (also search the extended fallback PATH) - npx_path = shutil.which("npx") - if not npx_path and extended_path: - npx_path = shutil.which("npx", path=extended_path) + npx_path = _resolve_npx_bin() if npx_path: if not validate: return "npx agent-browser" @@ -2470,11 +2484,47 @@ def _find_agent_browser(*, validate: bool = True) -> str: raise FileNotFoundError( "agent-browser CLI not found. Install it with: " f"{_browser_install_hint()}\n" - "Or run 'npm install' in the repo root to install locally.\n" "Or ensure npx is available in your PATH." ) +def warm_agent_browser_npx_cache(timeout: float = 60.0) -> bool: + """Best-effort pre-fetch of the agent-browser npm package via npx. + + agent-browser is no longer a root package.json dependency (#43564) — + it resolves lazily via ``npx agent-browser`` instead, which keeps it + out of the npm workspace install graph entirely (nothing to prune it + anymore) but means the first real invocation in a session would + otherwise pay npx's registry-lookup/fetch cost. Calling this during + ``hermes update`` (or ``hermes doctor --fix``) warms npx's own cache + ahead of time, restoring the "available before any session starts" + property agent-browser had while it was an eager root dependency — + without re-entangling it with the workspace graph. + + Fire-and-forget: never raises, always safe to call opportunistically. + Returns True only if npx actually ran successfully (npx unavailable, + a timeout, or a nonzero exit all return False silently). + """ + npx_bin = _resolve_npx_bin() + if not npx_bin: + return False + try: + result = subprocess.run( + # --prefer-offline: once cached, repeat `hermes update`/`doctor + # --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"], + capture_output=True, + text=True, + timeout=timeout, + check=False, + ) + return result.returncode == 0 + except Exception: + return False + + def _extract_screenshot_path_from_text(text: str) -> Optional[str]: """Extract a screenshot file path from agent-browser human-readable output.""" if not text: