From 793f0b3ff172e157e441be262963e958f6fe30e9 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Tue, 11 Aug 2026 17:34:28 +0800 Subject: [PATCH] fix(install): stop npm-installing agent-browser eagerly in install.sh/install.ps1 ensure_browser() (install.sh) and Install-AgentBrowser (install.ps1) are reached only via the explicit --ensure browser / -Ensure browser on-demand mode, itself only triggered by an actual browser-tool call's lazy-install fallback or `hermes acp --setup-browser`. agent-browser already resolves via npx in that same fallback before ever reaching these scripts, so eagerly npm-installing a second, separately version-pinned copy here was redundant and an extra credential/ supply-chain surface for a path npx already covers. Chromium acquisition for this on-demand path is now deferred entirely to _maybe_autoinstall_chromium's existing lazy fallback. camofox's install and system-browser detection/configuration are unaffected. install.ps1 also drops the now-dead -SkipChromium switch, confirmed unused at its one call site. --- scripts/install.ps1 | 41 +++++-------- scripts/install.sh | 37 ++++-------- tests/test_install_ps1_browser_install.py | 72 +++++++++++++++++++++++ tests/test_install_sh_browser_install.py | 47 +++++++++++++++ 4 files changed, 143 insertions(+), 54 deletions(-) create mode 100644 tests/test_install_ps1_browser_install.py diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 6ca4de190a307..ddcbb027518e7 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -653,14 +653,21 @@ function Write-BrowserEnv { } function Install-AgentBrowser { - param([switch]$SkipChromium) $npm = Resolve-NpmCmd if (-not $npm) { Write-Err "npm not found -- install Node.js first" throw "npm not found" } - Write-Info "Installing agent-browser via npm -g --prefix..." + # agent-browser itself is intentionally NOT installed here (#43564 / + # PR #44772 review): it resolves lazily via `npx agent-browser` instead, + # which every consumer (tools/browser_tool.py, `hermes update`'s npx + # cache warm) already goes through. Eagerly npm-installing a second, + # separately version-pinned copy here -- only reachable via this + # explicit -Ensure browser fallback in the first place -- was redundant + # complexity and an extra credential/supply-chain surface for a path + # npx already covers. + Write-Info "Installing camofox browser server..." $prefixDir = Join-Path $HermesHome "node" if (-not (Test-Path $prefixDir)) { New-Item -ItemType Directory -Path $prefixDir -Force | Out-Null @@ -668,7 +675,7 @@ function Install-AgentBrowser { $npmLog = [System.IO.Path]::GetTempFileName() $prevEAP = $ErrorActionPreference $ErrorActionPreference = "Continue" - & $npm install -g --prefix $prefixDir --silent --ignore-scripts "agent-browser@^0.26.0" "@askjo/camofox-browser@^1.5.2" 2>&1 | Tee-Object -FilePath $npmLog | Out-Null + & $npm install -g --prefix $prefixDir --silent --ignore-scripts "@askjo/camofox-browser@^1.5.2" 2>&1 | Tee-Object -FilePath $npmLog | Out-Null $npmExit = $LASTEXITCODE $ErrorActionPreference = $prevEAP if ($npmExit -ne 0) { @@ -683,30 +690,10 @@ function Install-AgentBrowser { } Remove-Item $npmLog -Force -ErrorAction SilentlyContinue - if (-not $SkipChromium) { - $sysBrowser = Find-SystemBrowser - if ($sysBrowser) { - Write-BrowserEnv -BrowserPath $sysBrowser - Write-Info "Explicit browser override set -- skipping bundled Chromium download" - } else { - $abExe = Join-Path $prefixDir "agent-browser.cmd" - if (Test-Path $abExe) { - Write-Info "Installing Chromium via agent-browser install..." - $abLog = [System.IO.Path]::GetTempFileName() - $prevEAP = $ErrorActionPreference - $ErrorActionPreference = "Continue" - & $abExe install 2>&1 | Tee-Object -FilePath $abLog | Out-Null - $abExit = $LASTEXITCODE - $ErrorActionPreference = $prevEAP - if ($abExit -ne 0) { - $abDetail = Get-Content $abLog -Raw -ErrorAction SilentlyContinue - Write-Warn "Chromium install failed (exit $abExit): $abDetail" - } - Remove-Item $abLog -Force -ErrorAction SilentlyContinue - } else { - Write-Warn "agent-browser.cmd not found at $abExe" - } - } + $sysBrowser = Find-SystemBrowser + if ($sysBrowser) { + Write-BrowserEnv -BrowserPath $sysBrowser + Write-Info "Explicit browser override set -- Chromium download will be skipped when agent-browser installs on demand" } Write-Success "Agent-browser ready" } diff --git a/scripts/install.sh b/scripts/install.sh index a1c6d4d9ddf3a..4ddb94fbe6222 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -2706,13 +2706,20 @@ ensure_browser() { return 1 fi - log_info "Installing agent-browser..." + # agent-browser itself is intentionally NOT installed here (#43564 / + # PR #44772 review): it resolves lazily via `npx agent-browser` instead, + # which every consumer (tools/browser_tool.py, `hermes update`'s npx + # cache warm) already goes through. Eagerly npm-installing a second, + # separately version-pinned copy here -- only reachable via this + # explicit --ensure browser fallback in the first place -- was redundant + # complexity and an extra credential/supply-chain surface for a path + # npx already covers. + log_info "Installing camofox browser server..." local log_file log_file="$(mktemp)" # Time-boxed (#39219): a stalled npm registry fetch here would otherwise # hang the installer with no progress, same class as the desktop build. if ! run_with_timeout "$NODE_DEPS_TIMEOUT" "$npm_bin" install -g --prefix "$HERMES_HOME/node" --silent --ignore-scripts \ - "agent-browser@^0.26.0" \ "@askjo/camofox-browser@^1.5.2" \ >"$log_file" 2>&1; then log_error "npm install failed or timed out:" @@ -2728,31 +2735,7 @@ ensure_browser() { sys_browser="$(find_system_browser 2>/dev/null || true)" if [ -n "$sys_browser" ]; then configure_browser_env_from_system_browser "$sys_browser" - log_info "Explicit browser override set -- skipping bundled Chromium download" - return 0 - fi - - log_info "Installing Chromium via agent-browser install..." - local ab_bin="$HERMES_HOME/node/bin/agent-browser" - if [ -x "$ab_bin" ]; then - "$ab_bin" install 2>/dev/null || { - log_warn "Chromium install failed. Browser tools may not work without a system browser." - - # OS-specific hints (detect_os sets $DISTRO) - case "${DISTRO:-unknown}" in - ubuntu|debian) - log_info "Try: sudo apt-get install -y chromium-browser" - ;; - arch) - log_info "Try: sudo pacman -S chromium" - ;; - fedora|rhel|centos) - log_info "Try: sudo dnf install -y chromium" - ;; - esac - } - else - log_warn "agent-browser not found at $ab_bin" + log_info "Explicit browser override set -- Chromium download will be skipped when agent-browser installs on demand" fi return 0 diff --git a/tests/test_install_ps1_browser_install.py b/tests/test_install_ps1_browser_install.py new file mode 100644 index 0000000000000..0a6b9b556a14a --- /dev/null +++ b/tests/test_install_ps1_browser_install.py @@ -0,0 +1,72 @@ +"""Regression test for install.ps1 browser setup (PR #44772 review). + +agent-browser resolves lazily via npx everywhere else in the system +(tools/browser_tool.py::_find_agent_browser); Install-AgentBrowser was the +last place that still eagerly npm-installed a second, separately +version-pinned copy of it. Removed: agent-browser acquisition now happens +only via `hermes update`'s npx cache warm or an actual browser-tool call's +lazy npx resolution. + +Linux CI cannot execute the PowerShell installer, so verification here is +source-text-level only, matching tests/test_install_sh_browser_install.py +and tests/test_install_ps1_ascii_only.py. +""" + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +INSTALL_PS1 = REPO_ROOT / "scripts" / "install.ps1" + + +def _extract_function_body(source: str, name: str) -> str: + m = re.search( + rf"^function {re.escape(name)} \{{.*?^\}}", source, re.MULTILINE | re.DOTALL + ) + assert m, f"could not extract function {name} from install.ps1" + return m.group(0) + + +def test_install_agent_browser_no_longer_npm_installs_agent_browser() -> None: + body = _extract_function_body(INSTALL_PS1.read_text(), "Install-AgentBrowser") + + assert "agent-browser@" not in body + assert "Installing Chromium via agent-browser install" not in body + # camofox is unrelated to this change and must still be installed here. + assert "@askjo/camofox-browser@^1.5.2" in body + # System-browser detection is still cheap/valuable without agent-browser. + assert "Find-SystemBrowser" in body + assert "Write-BrowserEnv" in body + + +def test_install_agent_browser_drops_unused_skip_chromium_param() -> None: + """$SkipChromium only existed to gate the now-removed Chromium-download + branch; grep confirms it's never passed at the one real call site, so a + lingering param would be dead code implying a control path that no + longer exists.""" + body = _extract_function_body(INSTALL_PS1.read_text(), "Install-AgentBrowser") + + assert "SkipChromium" not in body + + # And the one real call site must not pass it either. + text = INSTALL_PS1.read_text() + call_site = re.search(r"^\s*Install-AgentBrowser.*$", text, re.MULTILINE) + assert call_site, "could not find Install-AgentBrowser call site" + assert "SkipChromium" not in call_site.group(0) + + +def test_install_agent_browser_still_ignore_scripts_hardened() -> None: + """The removal of agent-browser must not have also dropped the + supply-chain hardening that still applies to the remaining camofox + install.""" + body = _extract_function_body(INSTALL_PS1.read_text(), "Install-AgentBrowser") + + assert "--ignore-scripts" in body + + +def test_install_agent_browser_no_longer_references_agent_browser_cmd_shim() -> None: + """No dangling reference to the agent-browser.cmd shim should remain + now that this function never installs it.""" + body = _extract_function_body(INSTALL_PS1.read_text(), "Install-AgentBrowser") + + assert "agent-browser.cmd" not in body diff --git a/tests/test_install_sh_browser_install.py b/tests/test_install_sh_browser_install.py index 17b4de00c42fd..a080bcdde22b6 100644 --- a/tests/test_install_sh_browser_install.py +++ b/tests/test_install_sh_browser_install.py @@ -197,6 +197,53 @@ def test_no_retry_when_native_succeeds_on_ubuntu_26() -> None: assert r["final_rc"] == 0 +import re + + +def _extract_function_body(source: str, name: str) -> str: + m = re.search(rf"^{re.escape(name)}\(\) \{{.*?^\}}", source, re.MULTILINE | re.DOTALL) + assert m, f"could not extract {name}() from install.sh" + return m.group(0) + + +def test_ensure_browser_no_longer_npm_installs_agent_browser() -> None: + """agent-browser resolves lazily via npx everywhere else in the system + (tools/browser_tool.py::_find_agent_browser); this was the last place + that still eagerly npm-installed a second, separately version-pinned + copy of it. Removed: agent-browser acquisition now happens only via + `hermes update`'s npx cache warm or an actual browser-tool call's lazy + npx resolution (PR #44772 review).""" + body = _extract_function_body(INSTALL_SH.read_text(), "ensure_browser") + + assert "agent-browser@" not in body + assert "Installing Chromium via agent-browser install" not in body + # camofox is unrelated to this change and must still be installed here. + assert "@askjo/camofox-browser@^1.5.2" in body + # System-browser detection is still cheap/valuable without agent-browser. + assert "find_system_browser" in body + assert "configure_browser_env_from_system_browser" in body + + +def test_ensure_browser_still_ignore_scripts_and_timeout_guarded() -> None: + """The removal of agent-browser must not have also dropped the + supply-chain and hang-protection hardening that still applies to the + remaining camofox install.""" + body = _extract_function_body(INSTALL_SH.read_text(), "ensure_browser") + + assert "--ignore-scripts" in body + assert "run_with_timeout" in body + + +def test_ensure_browser_no_longer_references_agent_browser_binary_path() -> None: + """No dangling reference to a local agent-browser binary path should + remain now that this function never installs it — a leftover reference + would be dead code pointing at a binary that no longer gets placed + there by this function.""" + body = _extract_function_body(INSTALL_SH.read_text(), "ensure_browser") + + assert "$HERMES_HOME/node/bin/agent-browser" not in body + +