diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 814fa7e8f32..24160a85044 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -2023,30 +2023,44 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: tui_dir, include_child_workspaces=True, ) - result = subprocess.run( - [ - npm, - "install", - *npm_workspace_args, - # --include=dev: ui-tui's build toolchain (esbuild, typescript) - # lives in devDependencies. An inherited NODE_ENV=production - # (e.g. from a container shell or a parent TUI launch) or an - # npm `omit=dev` config would silently skip them and the TUI - # build would fail. See _run_npm_install_deterministic. - "--include=dev", - "--silent", - "--no-fund", - "--no-audit", - "--progress=false", - ], - cwd=str(npm_cwd), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - encoding="utf-8", - errors="replace", - env={**os.environ, "CI": "1"}, - ) + npm_install_cmd = [ + npm, + "install", + *npm_workspace_args, + # --include=dev: ui-tui's build toolchain (esbuild, typescript) + # lives in devDependencies. An inherited NODE_ENV=production + # (e.g. from a container shell or a parent TUI launch) or an + # npm `omit=dev` config would silently skip them and the TUI + # build would fail. See _run_npm_install_deterministic. + "--include=dev", + "--silent", + "--no-fund", + "--no-audit", + "--progress=false", + ] + + def _run_tui_install() -> subprocess.CompletedProcess: + return subprocess.run( + npm_install_cmd, + cwd=str(npm_cwd), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + encoding="utf-8", + errors="replace", + env={**os.environ, "CI": "1"}, + ) + + result = _run_tui_install() + if result.returncode != 0: + # An npm outside the root package.json's `engines.npm` range fails + # here before doing any work; upgrade a Hermes-managed npm once and + # retry rather than dumping EBADENGINE at the user. + from hermes_cli.npm_engine import maybe_repair_npm_engine + + combined_output = f"{result.stdout or ''}\n{result.stderr or ''}" + if maybe_repair_npm_engine(npm, combined_output): + result = _run_tui_install() if result.returncode != 0: combined = f"{result.stdout or ''}\n{result.stderr or ''}".strip() preview = "\n".join(combined.splitlines()[-30:]) @@ -5443,34 +5457,84 @@ def _run_npm_install_deterministic( # install path and nix/lib.nix npm ci hooks. run_env = {**os.environ, **(env or {}), "CI": "1"} - lockfile = cwd / "package-lock.json" - if lockfile.exists(): - ci_cmd = [npm, "ci", "--include=dev", *extra_args] - ci_result = subprocess.run( - ci_cmd, + def _run(cmd: list[str]) -> subprocess.CompletedProcess: + return _run_npm_watching_for_engine_failure( + cmd, cwd=cwd, env=run_env, capture_output=capture_output, + ) + + def _attempt() -> subprocess.CompletedProcess: + lockfile = cwd / "package-lock.json" + if lockfile.exists(): + ci_result = _run([npm, "ci", "--include=dev", *extra_args]) + if ci_result.returncode == 0: + return ci_result + # Fall through to `npm install` — lockfile may be out of sync on a + # WIP fork/branch, or `npm ci` may not be available on very old npm. + return _run([npm, "install", "--no-save", "--include=dev", *extra_args]) + + result = _attempt() + if result.returncode == 0: + return result + + # An npm outside the root package.json's `engines.npm` range fails every + # command here identically (the `npm install` fallback included), so the + # failure is worth exactly one upgrade attempt. `maybe_repair_npm_engine` + # returns True only when it actually upgraded a Hermes-managed npm. + from hermes_cli.npm_engine import maybe_repair_npm_engine + + combined = f"{result.stdout or ''}\n{result.stderr or ''}" + if not maybe_repair_npm_engine(npm, combined): + return result + return _attempt() + + +def _run_npm_watching_for_engine_failure( + cmd: list[str], + *, + cwd: Path, + env: dict[str, str], + capture_output: bool, +) -> subprocess.CompletedProcess: + """Run *cmd*, always retaining stderr so ``EBADENGINE`` stays detectable. + + ``capture_output=False`` callers stream npm's progress live and would + otherwise hand back a ``CompletedProcess`` with ``stderr=None``, leaving the + engine-failure recovery nothing to read. Tee stderr instead: each line is + forwarded to this process's stderr as it arrives (so live output is + unchanged) and accumulated for the caller. + """ + if capture_output: + return subprocess.run( + cmd, + cwd=cwd, + env=env, + capture_output=True, text=True, encoding="utf-8", errors="replace", check=False, ) - if ci_result.returncode == 0: - return ci_result - # Fall through to `npm install` — lockfile may be out of sync on a - # WIP fork/branch, or `npm ci` may not be available on very old npm. - install_cmd = [npm, "install", "--no-save", "--include=dev", *extra_args] - return subprocess.run( - install_cmd, + + captured: list[str] = [] + with subprocess.Popen( + cmd, cwd=cwd, - env=run_env, - capture_output=capture_output, + env=env, + stderr=subprocess.PIPE, text=True, encoding="utf-8", errors="replace", - check=False, - ) + ) as proc: + if proc.stderr is not None: + for line in proc.stderr: + captured.append(line) + sys.stderr.write(line) + sys.stderr.flush() + returncode = proc.wait() + return subprocess.CompletedProcess(cmd, returncode, None, "".join(captured)) def _missing_web_build_tool(output: str) -> str | None: diff --git a/hermes_cli/npm_engine.py b/hermes_cli/npm_engine.py new file mode 100644 index 00000000000..335a1a818b9 --- /dev/null +++ b/hermes_cli/npm_engine.py @@ -0,0 +1,276 @@ +"""Recover from npm ``EBADENGINE`` failures by upgrading a managed npm. + +The repo's ``.npmrc`` sets ``engine-strict=true`` and the root ``package.json`` +pins an ``engines.npm`` range, so an npm outside that range aborts every +``npm ci`` / ``npm install`` we run inside the checkout:: + + npm error code EBADENGINE + npm error notsup Required: {"node":">=20.0.0","npm":"<11.10.0 || >=12.0.0"} + npm error notsup Actual: {"npm":"10.9.8","node":"v22.23.1"} + +Rather than predicting the failure (which would mean a semver range matcher and +an ``npm --version`` probe before work that usually succeeds), we react to it: +npm states the required range in the error, so the recovery reads the +constraint straight out of the output it just produced. + +Scope of the repair is deliberately narrow. Hermes only upgrades an npm that +lives inside its **own** managed Node tree (``$HERMES_HOME/node``), installing +in place with ``--prefix`` so ``bin/npm`` keeps resolving to the upgraded +``lib/node_modules/npm``. A system / nvm / brew / Nix npm belongs to the user +and their other projects; for those we print the exact command and let the +original failure stand. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +import tempfile +from pathlib import Path + +from hermes_constants import get_hermes_home, with_hermes_node_path + +__all__ = [ + "is_ebadengine", + "required_npm_range", + "managed_npm_prefix", + "upgrade_managed_npm", + "maybe_repair_npm_engine", +] + +# npm prints `npm error notsup Required: {...}` on npm >= 10 and +# `npm ERR! notsup Required: {...}` on older releases. +_REQUIRED_RE = re.compile(r"Required:\s*(\{.*?\})") +_ACTUAL_RE = re.compile(r"Actual:\s*(\{.*?\})") + +# Wall-clock cap for the self-upgrade. The measured in-place upgrade of a +# managed tree takes ~1s; this only has to cover a slow registry. +_UPGRADE_TIMEOUT = 300 + + +def is_ebadengine(output: str) -> bool: + """Return True when *output* is an npm engine-compatibility failure.""" + if not output: + return False + return "EBADENGINE" in output or "Unsupported engine" in output + + +def _iter_required_blocks(output: str) -> list[dict]: + blocks: list[dict] = [] + for match in _REQUIRED_RE.finditer(output or ""): + try: + parsed = json.loads(match.group(1)) + except ValueError: + continue + if isinstance(parsed, dict): + blocks.append(parsed) + return blocks + + +def required_npm_range(output: str) -> str | None: + """Return the ``engines.npm`` range npm demanded in *output*. + + Returns ``None`` when the output has no engine failure, or when the + failure is about Node rather than npm — upgrading npm cannot fix a Node + version mismatch, so the caller must not try. + + When several packages report conflicting npm ranges the repo's own root + constraint is preferred (it is the one we control); otherwise the first + range wins, since any of them is a strict improvement over an npm that + satisfies none. + """ + if not is_ebadengine(output): + return None + ranges = [ + str(block["npm"]).strip() + for block in _iter_required_blocks(output) + if block.get("npm") + ] + if not ranges: + return None + distinct = list(dict.fromkeys(ranges)) + if len(distinct) > 1: + repo_range = _repo_npm_range() + if repo_range in distinct: + return repo_range + return distinct[0] + + +def actual_npm_version(output: str) -> str | None: + """Return the npm version npm reported as ``Actual`` in *output*.""" + for match in _ACTUAL_RE.finditer(output or ""): + try: + parsed = json.loads(match.group(1)) + except ValueError: + continue + if isinstance(parsed, dict) and parsed.get("npm"): + return str(parsed["npm"]).strip() + return None + + +def _repo_npm_range() -> str | None: + """Return ``engines.npm`` from the checkout's root ``package.json``.""" + package_json = Path(__file__).resolve().parent.parent / "package.json" + try: + data = json.loads(package_json.read_text(encoding="utf-8")) + except (OSError, ValueError): + return None + engines = data.get("engines") + if not isinstance(engines, dict): + return None + value = engines.get("npm") + return str(value).strip() if value else None + + +def managed_npm_prefix(npm: str | os.PathLike[str] | None) -> Path | None: + """Return the Hermes-managed Node root *npm* lives in, else ``None``. + + Symlinks are resolved first: an install links ``~/.local/bin/npm`` at + ``$HERMES_HOME/node/bin/npm``, which itself links into + ``lib/node_modules/npm/bin/npm-cli.js``. Every one of those spellings is + the managed npm and must be recognised as such, or the repair silently + declines to fix the very install it owns. + """ + if not npm: + return None + prefix = get_hermes_home() / "node" + try: + resolved = Path(npm).resolve() + prefix_resolved = prefix.resolve() + except OSError: + return None + if resolved == prefix_resolved or prefix_resolved in resolved.parents: + return prefix + return None + + +def _upgrade_env() -> dict[str, str]: + env = with_hermes_node_path() + # The checkout's .npmrc sets `min-release-age`, which would gate the npm + # release we are trying to install. The upgrade runs from a temp cwd so + # that file is out of scope; this neutralises a user-level ~/.npmrc too. + env["npm_config_min_release_age"] = "0" + # `unicode-animations`-style postinstall animations no-op under CI=1. + env["CI"] = "1" + return env + + +def upgrade_managed_npm( + npm: str, + npm_range: str, + *, + prefix: Path, + quiet: bool = False, +) -> bool: + """Upgrade the managed npm at *npm* in place to satisfy *npm_range*. + + ``--prefix`` targets the managed tree explicitly: a managed install writes + ``prefix=~/.local`` into ``$HERMES_HOME/node/etc/npmrc`` so that global + installs land on PATH, and without the override the "upgrade" would install + a second npm somewhere else while the managed one stayed stale. + """ + if not quiet: + print( + f"→ Upgrading Hermes-managed npm to satisfy {npm_range}…", + flush=True, + ) + try: + # A temp cwd keeps the checkout's .npmrc (engine-strict, min-release-age) + # from applying to the upgrade itself. + with tempfile.TemporaryDirectory(prefix="hermes-npm-upgrade-") as tmp: + result = subprocess.run( + [ + npm, + "install", + "--global", + "--prefix", + str(prefix), + f"npm@{npm_range}", + "--no-fund", + "--no-audit", + "--progress=false", + ], + cwd=tmp, + env=_upgrade_env(), + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=_UPGRADE_TIMEOUT, + check=False, + ) + except (OSError, subprocess.SubprocessError): + if not quiet: + print(" ✗ npm upgrade could not be started", file=sys.stderr) + return False + + if result.returncode != 0: + if not quiet: + detail = (result.stderr or result.stdout or "").strip().splitlines() + print(" ✗ npm upgrade failed", file=sys.stderr) + for line in detail[-10:]: + print(f" {line}", file=sys.stderr) + return False + + if not quiet: + print(f" ✓ npm upgraded to {_probe_version(npm) or npm_range}", flush=True) + return True + + +def _probe_version(npm: str) -> str | None: + try: + result = subprocess.run( + [npm, "--version"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=30, + env=with_hermes_node_path(), + check=False, + ) + except (OSError, subprocess.SubprocessError): + return None + return (result.stdout or "").strip() or None + + +def _print_manual_fix(npm: str, npm_range: str, actual: str | None) -> None: + have = f"npm {actual} " if actual else "This npm " + print( + f"\n✗ {have}does not satisfy the range this project requires: {npm_range}\n" + f" Resolved npm: {npm}\n" + " Hermes only upgrades npm inside its own managed Node install, so this\n" + " one is left alone. Upgrade it yourself with:\n" + f' npm install -g npm@"{npm_range}"', + file=sys.stderr, + ) + + +def maybe_repair_npm_engine( + npm: str | None, + output: str, + *, + quiet: bool = False, +) -> bool: + """Repair an ``EBADENGINE`` failure when Hermes owns the npm involved. + + *output* is the combined stdout/stderr of the npm command that just failed. + Returns ``True`` only when npm was actually upgraded, meaning the caller + should retry its command once. Returns ``False`` for every other case — + not an engine failure, a Node (not npm) mismatch, an npm Hermes does not + own, or a failed upgrade — leaving the original failure to stand. + """ + npm_range = required_npm_range(output) + if not npm_range or not npm: + return False + + prefix = managed_npm_prefix(npm) + if prefix is None: + if not quiet: + _print_manual_fix(npm, npm_range, actual_npm_version(output)) + return False + + return upgrade_managed_npm(npm, npm_range, prefix=prefix, quiet=quiet) diff --git a/tests/hermes_cli/test_npm_engine.py b/tests/hermes_cli/test_npm_engine.py new file mode 100644 index 00000000000..1e48653fe86 --- /dev/null +++ b/tests/hermes_cli/test_npm_engine.py @@ -0,0 +1,229 @@ +"""Tests for npm ``EBADENGINE`` recovery (``hermes_cli/npm_engine.py``). + +The behaviour under test is a contract about *reacting* to npm's own engine +check: npm states the range it wants in the failure, Hermes upgrades only an +npm it owns, and every other case leaves the original failure alone. +""" + +import json +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli.npm_engine import ( + actual_npm_version, + is_ebadengine, + managed_npm_prefix, + maybe_repair_npm_engine, + required_npm_range, +) + + +# Verbatim npm 10 output shape (`npm error`), and the npm 9 shape (`npm ERR!`). +EBADENGINE_OUTPUT = """ +npm error code EBADENGINE +npm error engine Unsupported engine +npm error engine Not compatible with your version of node/npm: hermes-agent@1.0.0 +npm error notsup Not compatible with your version of node/npm: hermes-agent@1.0.0 +npm error notsup Required: {"node":">=20.0.0","npm":"<11.10.0 || >=12.0.0"} +npm error notsup Actual: {"npm":"11.10.0","node":"v22.23.1"} +""" + +LEGACY_EBADENGINE_OUTPUT = """ +npm ERR! code EBADENGINE +npm ERR! engine Unsupported engine +npm ERR! notsup Required: {"node":">=20.0.0","npm":">=12.0.0"} +npm ERR! notsup Actual: {"npm":"9.6.7","node":"v20.1.0"} +""" + +# A lockfile mismatch — the other common `npm ci` failure. Must NOT be treated +# as an engine problem, or every out-of-sync lockfile would trigger an upgrade. +ELOCK_OUTPUT = """ +npm error code EUSAGE +npm error `npm ci` can only install packages when your package.json and +npm error package-lock.json are in sync. +""" + + +class TestDetection: + def test_recognises_modern_and_legacy_engine_failures(self): + assert is_ebadengine(EBADENGINE_OUTPUT) + assert is_ebadengine(LEGACY_EBADENGINE_OUTPUT) + + def test_unrelated_failures_are_not_engine_failures(self): + assert not is_ebadengine(ELOCK_OUTPUT) + assert not is_ebadengine("") + assert not is_ebadengine("npm error code E404") + + def test_range_comes_from_the_error_not_a_hardcoded_list(self): + assert required_npm_range(EBADENGINE_OUTPUT) == "<11.10.0 || >=12.0.0" + assert required_npm_range(LEGACY_EBADENGINE_OUTPUT) == ">=12.0.0" + + def test_actual_version_is_reported_back(self): + assert actual_npm_version(EBADENGINE_OUTPUT) == "11.10.0" + + def test_no_range_for_non_engine_output(self): + assert required_npm_range(ELOCK_OUTPUT) is None + assert required_npm_range("") is None + + def test_node_only_mismatch_yields_no_npm_range(self): + """Upgrading npm cannot fix a Node version mismatch, so don't try.""" + node_only = ( + 'npm error code EBADENGINE\n' + 'npm error notsup Required: {"node":">=20.0.0"}\n' + 'npm error notsup Actual: {"npm":"10.9.8","node":"v18.0.0"}\n' + ) + assert required_npm_range(node_only) is None + + def test_malformed_required_block_is_ignored(self): + broken = ( + "npm error code EBADENGINE\n" + "npm error notsup Required: {not json}\n" + ) + assert required_npm_range(broken) is None + + +class TestManagedDetection: + """The upgrade must fire for every spelling of the managed npm, and for + no other npm — this is the boundary between "Hermes fixes it" and "the + user's own toolchain is left alone".""" + + @pytest.fixture + def managed_tree(self, tmp_path, monkeypatch): + home = tmp_path / ".hermes" + node = home / "node" + (node / "bin").mkdir(parents=True) + (node / "lib" / "node_modules" / "npm" / "bin").mkdir(parents=True) + cli = node / "lib" / "node_modules" / "npm" / "bin" / "npm-cli.js" + cli.write_text("#!/usr/bin/env node\n", encoding="utf-8") + (node / "bin" / "npm").symlink_to(cli) + monkeypatch.setenv("HERMES_HOME", str(home)) + return home + + def test_direct_managed_bin_is_managed(self, managed_tree): + npm = managed_tree / "node" / "bin" / "npm" + assert managed_npm_prefix(npm) == managed_tree / "node" + + def test_symlink_from_local_bin_resolves_to_managed(self, managed_tree, tmp_path): + """An install links ~/.local/bin/npm at the managed tree; that link is + the npm a user's PATH actually resolves, so it must count as managed.""" + local_bin = tmp_path / "local-bin" + local_bin.mkdir() + link = local_bin / "npm" + link.symlink_to(managed_tree / "node" / "bin" / "npm") + assert managed_npm_prefix(link) == managed_tree / "node" + + def test_system_npm_is_not_managed(self, managed_tree, tmp_path): + system_npm = tmp_path / "usr" / "bin" / "npm" + system_npm.parent.mkdir(parents=True) + system_npm.write_text("#!/bin/sh\n", encoding="utf-8") + assert managed_npm_prefix(system_npm) is None + + def test_no_npm_is_not_managed(self, managed_tree): + assert managed_npm_prefix(None) is None + assert managed_npm_prefix("") is None + + +class TestRepairDecision: + """`maybe_repair_npm_engine` returns True only when it actually upgraded, + because its return value is what gates the caller's single retry.""" + + @pytest.fixture + def managed_npm(self, tmp_path, monkeypatch): + home = tmp_path / ".hermes" + bin_dir = home / "node" / "bin" + bin_dir.mkdir(parents=True) + npm = bin_dir / "npm" + npm.write_text("#!/bin/sh\n", encoding="utf-8") + npm.chmod(0o755) + monkeypatch.setenv("HERMES_HOME", str(home)) + return npm + + def test_upgrades_managed_npm_with_the_range_npm_asked_for( + self, managed_npm, monkeypatch + ): + calls = [] + + def fake_run(cmd, **kwargs): + calls.append((cmd, kwargs)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr(subprocess, "run", fake_run) + assert maybe_repair_npm_engine(str(managed_npm), EBADENGINE_OUTPUT, quiet=True) + + upgrade_cmd = calls[0][0] + assert upgrade_cmd[1:3] == ["install", "--global"] + # The range must come from npm's error, and target the managed prefix + # explicitly (the managed etc/npmrc points `prefix` elsewhere). + assert "npm@<11.10.0 || >=12.0.0" in upgrade_cmd + prefix_index = upgrade_cmd.index("--prefix") + assert Path(upgrade_cmd[prefix_index + 1]) == managed_npm.parent.parent + + def test_upgrade_runs_outside_the_checkout(self, managed_npm, monkeypatch): + """The repo .npmrc sets min-release-age, which would gate the very npm + release we need; the upgrade must not run under it.""" + seen = {} + + def fake_run(cmd, **kwargs): + seen.update(kwargs) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr(subprocess, "run", fake_run) + maybe_repair_npm_engine(str(managed_npm), EBADENGINE_OUTPUT, quiet=True) + + cwd = Path(seen["cwd"]) + assert not (cwd / ".npmrc").exists() + assert seen["env"]["npm_config_min_release_age"] == "0" + + def test_failed_upgrade_reports_no_retry(self, managed_npm, monkeypatch): + monkeypatch.setattr( + subprocess, + "run", + lambda cmd, **kw: subprocess.CompletedProcess(cmd, 1, "", "boom"), + ) + assert not maybe_repair_npm_engine( + str(managed_npm), EBADENGINE_OUTPUT, quiet=True + ) + + def test_unmanaged_npm_is_never_touched(self, managed_npm, tmp_path, monkeypatch, capsys): + system_npm = tmp_path / "usr-bin-npm" + system_npm.write_text("#!/bin/sh\n", encoding="utf-8") + + def explode(cmd, **kwargs): # pragma: no cover - must not be reached + raise AssertionError(f"must not run a subprocess for a foreign npm: {cmd}") + + monkeypatch.setattr(subprocess, "run", explode) + assert not maybe_repair_npm_engine(str(system_npm), EBADENGINE_OUTPUT) + + # The user gets the exact command to run, since we refuse to run it. + err = capsys.readouterr().err + assert 'npm install -g npm@"<11.10.0 || >=12.0.0"' in err + + def test_non_engine_failure_never_upgrades(self, managed_npm, monkeypatch): + def explode(cmd, **kwargs): # pragma: no cover - must not be reached + raise AssertionError("a lockfile mismatch must not trigger an upgrade") + + monkeypatch.setattr(subprocess, "run", explode) + assert not maybe_repair_npm_engine(str(managed_npm), ELOCK_OUTPUT, quiet=True) + + +class TestRepoRangeIsSatisfiable: + """Invariant: whatever the root package.json demands, the recovery can + parse and act on it — a malformed range would make the repair a no-op.""" + + def test_root_engines_npm_range_is_a_usable_constraint(self): + repo_root = Path(__file__).resolve().parents[2] + package_json = repo_root / "package.json" + engines = json.loads(package_json.read_text(encoding="utf-8")).get("engines", {}) + npm_range = engines.get("npm") + if not npm_range: + pytest.skip("root package.json does not pin engines.npm") + + synthetic = ( + "npm error code EBADENGINE\n" + 'npm error notsup Required: ' + + json.dumps({"node": ">=20.0.0", "npm": npm_range}) + + "\n" + ) + assert required_npm_range(synthetic) == npm_range