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.
This commit is contained in:
Zak B. Elep
Teknium
parent
047a45e410
commit
793f0b3ff1
|
|
@ -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"
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue