From 6a198f8a12e1dcfba79238c1185987b552395df0 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 13 Aug 2026 13:39:54 -0500 Subject: [PATCH] fix(install): a failed Node dependency install now fails the install instead of printing success (#85537) * fix(install): fail when Node dependencies cannot install (#85297) The POSIX installer converted root and TUI npm failures into warnings, then printed a dependency-success message and reached the installation-complete banner with a zero exit status. This left consumers with no usable node_modules while reporting success. Treat both required npm installs as fatal: log an error, restore tracked lockfile churn, return status 1, and propagate the failure from the monolithic and node-deps stage callers. Successful installs, Termux and missing-Node skips, missing-manifest skips, and optional Playwright/Browser Use/Computer Use best-effort behavior remain unchanged. The fix is limited to the POSIX installer; the PowerShell installer is outside this issue's scope. Focused and adjacent installer tests passed (32), with bash syntax, py_compile, and diff checks clean. The broader installer family had 90 passes, one unrelated pre-existing failure, and two skips; the full suite was environment-limited by missing dependencies. CodeRabbit, iterative deep security/compatibility reviews, and final confidence security/compatibility reviews were clean against the final diff. Fixes #85297 * fix(install): require npm alongside node in check_node (#77003) A stray `node` symlink without a sibling `npm` (leftover from a node version manager) made check_node report "Node.js found"; every later npm install then failed and the desktop build died with an opaque "Node.js / npm unavailable". Node now only counts as found when npm resolves on the same PATH, with an explicit "stray node symlink?" branch that falls through to the Hermes-managed Node (which bundles npm). The overlapping success-log honesty half of the original PR is subsumed by the previous commit, which makes a failed npm install fatal rather than conditionally-logged; the behavioral tests there cover it, so this commit keeps only the check_node PATH-gate assertions. Fixes #77003. Co-authored-by: criptogus --------- Co-authored-by: Eugeniusz Gilewski Co-authored-by: CriptoGus <128640021+criptogus@users.noreply.github.com> Co-authored-by: criptogus --- scripts/install.sh | 44 +++++-- tests/test_install_sh_node_deps_failure.py | 145 +++++++++++++++++++++ tests/test_install_sh_node_npm_check.py | 42 ++++++ 3 files changed, 219 insertions(+), 12 deletions(-) create mode 100644 tests/test_install_sh_node_deps_failure.py create mode 100644 tests/test_install_sh_node_npm_check.py diff --git a/scripts/install.sh b/scripts/install.sh index ac217619aea69..2aff3abe91a68 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -835,8 +835,16 @@ check_node() { # enough for the desktop build AND an npm that can read our .npmrc. A # bad-band npm (see npm_supports_npmrc) fails `npm ci` outright, and the # managed Node we install instead bundles one that works. - if command -v node &> /dev/null && node_satisfies_build "$(node --version)"; then - if ! command -v npm &> /dev/null || npm_supports_npmrc "$(npm --version 2>/dev/null)"; then + # + # npm must actually be reachable, not just node: a stray `node` symlink + # without a sibling npm (leftover from a node version manager) makes + # `command -v node` succeed while every later `npm install` silently + # fails and the desktop build dies with an opaque "Node.js / npm + # unavailable" (#77003). Node only counts as found when npm resolves on + # the same PATH. + if command -v node &> /dev/null && command -v npm &> /dev/null \ + && node_satisfies_build "$(node --version)"; then + if npm_supports_npmrc "$(npm --version 2>/dev/null)"; then log_success "Node.js $(node --version) found" HAS_NODE=true return 0 @@ -848,14 +856,17 @@ check_node() { fi # Prefer a Hermes-managed Node from a previous run over a too-old system one. - if [ -x "$HERMES_HOME/node/bin/node" ] && node_satisfies_build "$("$HERMES_HOME/node/bin/node" --version)"; then + if [ -x "$HERMES_HOME/node/bin/node" ] && [ -x "$HERMES_HOME/node/bin/npm" ] \ + && node_satisfies_build "$("$HERMES_HOME/node/bin/node" --version)"; then export PATH="$HERMES_HOME/node/bin:$PATH" log_success "Node.js $("$HERMES_HOME/node/bin/node" --version) found (Hermes-managed)" HAS_NODE=true return 0 fi - if command -v node &> /dev/null; then + if command -v node &> /dev/null && ! command -v npm &> /dev/null; then + log_warn "node found but npm is not on PATH (stray node symlink?) — installing Hermes-managed Node $NODE_VERSION LTS..." + elif command -v node &> /dev/null; then log_warn "Node.js $(node --version) is too old (Hermes requires Node >=26) — installing Hermes-managed Node $NODE_VERSION..." elif [ "$DISTRO" = "termux" ]; then log_info "Node.js not found — installing Node.js via pkg..." @@ -2294,9 +2305,14 @@ install_node_deps() { cd "$INSTALL_DIR" # Time-boxed: a stalled registry fetch would otherwise hang here with no # progress (same #39219 stall class as the desktop build below). - run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent || { - log_warn "npm install failed or timed out (browser tools may not work)" - } + # A failed npm install used to still print "✓ Node.js dependencies + # installed", hiding the degradation from the user (#77003). Now it + # fails the install outright instead of burying the warning (#85297). + if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent; then + log_error "npm install failed or timed out; Node.js dependencies were not installed" + restore_dirty_lockfiles "$INSTALL_DIR" + return 1 + fi log_success "Node.js dependencies installed" # Install Playwright browser + system dependencies. @@ -2396,9 +2412,13 @@ install_node_deps() { log_info "Installing TUI dependencies..." cd "$INSTALL_DIR/ui-tui" # Time-boxed: a stalled registry fetch would otherwise hang here (#39219). - run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent || { - log_warn "TUI npm install failed or timed out (hermes --tui may not work)" - } + # Report success only on actual success, same as node-deps above + # (#77003) — and fail the install outright (#85297). + if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent; then + log_error "TUI npm install failed or timed out; TUI dependencies were not installed" + restore_dirty_lockfiles "$INSTALL_DIR" + return 1 + fi log_success "TUI dependencies installed" fi @@ -3292,7 +3312,7 @@ run_stage_body() { resolve_install_layout require_install_dir check_node - install_node_deps + install_node_deps || return install_uv install_browser_use_cli install_computer_use_driver @@ -3410,7 +3430,7 @@ main() { clone_repo setup_venv install_deps - install_node_deps + install_node_deps || return install_browser_use_cli install_computer_use_driver setup_path diff --git a/tests/test_install_sh_node_deps_failure.py b/tests/test_install_sh_node_deps_failure.py new file mode 100644 index 0000000000000..c42e0cb32ca41 --- /dev/null +++ b/tests/test_install_sh_node_deps_failure.py @@ -0,0 +1,145 @@ +"""Behavioral coverage for required Node dependency installation.""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parent.parent +INSTALL_SH = REPO_ROOT / "scripts" / "install.sh" + + +def _write_executable(path: Path, body: str) -> None: + path.write_text(body, encoding="utf-8") + path.chmod(0o755) + + +def _run_node_deps_stage( + tmp_path: Path, + *, + fail_directory: str | None, +) -> tuple[subprocess.CompletedProcess[str], Path, list[str]]: + install_dir = tmp_path / "install" + tui_dir = install_dir / "ui-tui" + bin_dir = tmp_path / "bin" + hermes_home = tmp_path / "home" + managed_bin = hermes_home / "bin" + npm_calls = tmp_path / "npm-calls" + + tui_dir.mkdir(parents=True) + bin_dir.mkdir() + managed_bin.mkdir(parents=True) + (install_dir / "package.json").write_text( + '{"name":"installer-regression-probe","private":true}\n', + encoding="utf-8", + ) + (tui_dir / "package.json").write_text( + '{"name":"tui-regression-probe","private":true}\n', + encoding="utf-8", + ) + _write_executable(bin_dir / "node", "#!/bin/sh\necho v26.0.0\n") + _write_executable( + bin_dir / "npm", + """#!/bin/sh +if [ "${1:-}" = "--version" ]; then + echo 12.0.0 + exit 0 +fi +printf '%s\\n' "$PWD" >> "$NPM_CALLS" +if [ -n "${NPM_FAIL_DIRECTORY:-}" ] && [ "$PWD" = "$NPM_FAIL_DIRECTORY" ]; then + echo "simulated npm lifecycle failure" >&2 + exit 37 +fi +exit 0 +""", + ) + _write_executable(managed_bin / "uv", "#!/bin/sh\necho 'uv probe'\n") + + env = os.environ.copy() + env.update( + { + "HERMES_HOME": str(hermes_home), + "HERMES_INSTALL_DIR": str(install_dir), + "NPM_CALLS": str(npm_calls), + "NPM_FAIL_DIRECTORY": fail_directory or "", + "PATH": f"{bin_dir}:{env['PATH']}", + } + ) + proc = subprocess.run( + [ + "bash", + str(INSTALL_SH), + "--stage", + "node-deps", + "--json", + "--skip-browser", + "--skip-computer-use", + ], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + check=False, + ) + calls = npm_calls.read_text(encoding="utf-8").splitlines() + return proc, install_dir, calls + + +def _stage_result(proc: subprocess.CompletedProcess[str]) -> dict[str, object]: + return json.loads(proc.stdout.splitlines()[-1]) + + +def test_root_node_dependency_failure_is_fatal(tmp_path: Path) -> None: + install_dir = tmp_path / "install" + proc, actual_install_dir, calls = _run_node_deps_stage( + tmp_path, + fail_directory=str(install_dir), + ) + + assert actual_install_dir == install_dir + assert proc.returncode != 0 + assert _stage_result(proc) == { + "ok": False, + "stage": "node-deps", + "skipped": False, + "reason": "exit code 1", + } + assert calls == [str(install_dir)] + assert "Node.js dependencies installed" not in proc.stdout + assert "TUI dependencies installed" not in proc.stdout + assert not (install_dir / "node_modules").exists() + + +def test_tui_node_dependency_failure_is_fatal(tmp_path: Path) -> None: + install_dir = tmp_path / "install" + tui_dir = install_dir / "ui-tui" + proc, _, calls = _run_node_deps_stage( + tmp_path, + fail_directory=str(tui_dir), + ) + + assert proc.returncode != 0 + assert _stage_result(proc)["ok"] is False + assert calls == [str(install_dir), str(tui_dir)] + assert "Node.js dependencies installed" in proc.stdout + assert "TUI dependencies installed" not in proc.stdout + + +def test_node_dependency_success_remains_successful(tmp_path: Path) -> None: + proc, install_dir, calls = _run_node_deps_stage( + tmp_path, + fail_directory=None, + ) + + assert proc.returncode == 0, proc.stderr + assert _stage_result(proc) == { + "ok": True, + "stage": "node-deps", + "skipped": False, + } + assert calls == [str(install_dir), str(install_dir / "ui-tui")] + assert "Node.js dependencies installed" in proc.stdout + assert "TUI dependencies installed" in proc.stdout diff --git a/tests/test_install_sh_node_npm_check.py b/tests/test_install_sh_node_npm_check.py new file mode 100644 index 0000000000000..6fa33e9bae7ab --- /dev/null +++ b/tests/test_install_sh_node_npm_check.py @@ -0,0 +1,42 @@ +"""Regression tests for install.sh Node/npm checks (#77003). + +A stray `node` symlink without a sibling `npm` (leftover from a node +version manager) made the installer report "✓ Node.js found" and then fail +opaquely at the desktop stage. Node must only count as found when npm +resolves on the same PATH, and npm install stages must not report success +when the install actually failed. +""" + +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +INSTALL_SH = REPO_ROOT / "scripts" / "install.sh" + + +def test_check_node_requires_npm_alongside_node() -> None: + """check_node must not report success when only `node` resolves. + + Before the fix, `command -v node` succeeding was enough — a stray node + symlink (no sibling npm) passed the check, every later `npm install` + failed silently, and the desktop build died with an opaque + "Node.js / npm unavailable" (#77003). + """ + text = INSTALL_SH.read_text() + + # The system-toolchain branch now gates on BOTH node and npm. + assert ( + "if command -v node &> /dev/null && command -v npm &> /dev/null \\" in text + ) + # The "node found but npm missing" case has its own explicit branch that + # falls through to installing the Hermes-managed Node (which bundles npm). + assert "node found but npm is not on PATH (stray node symlink?)" in text + + +def test_check_node_managed_requires_npm() -> None: + """The Hermes-managed Node fallback also requires its npm to exist.""" + text = INSTALL_SH.read_text() + assert ( + '[ -x "$HERMES_HOME/node/bin/node" ] && [ -x "$HERMES_HOME/node/bin/npm" ] \\' + in text + ) +