From 2f2998bb1b0d6a1b1c2c14b66b72982595b91506 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 4 May 2026 14:13:38 +1000 Subject: [PATCH] =?UTF-8?q?fix(tui):=20tolerate=20npm's=20peer-flag=20drop?= =?UTF-8?q?=20in=20lockfile=20comparison=20`=5Ftui=5Fneed=5Fnpm=5Finstall(?= =?UTF-8?q?)`=20compares=20the=20canonical=20`package-lock.json`=20against?= =?UTF-8?q?=20the=20hidden=20`node=5Fmodules/.package-lock.json`=20to=20de?= =?UTF-8?q?cide=20whether=20`npm=20install`=20needs=20to=20re-run.=20npm?= =?UTF-8?q?=209=20drops=20the=20`"peer":=20true`=20field=20from=20the=20hi?= =?UTF-8?q?dden=20lock=20on=20dev-deps=20that=20are=20*also*=20declared=20?= =?UTF-8?q?as=20peers=20(the=20canonical=20lock=20preserves=20the=20dual?= =?UTF-8?q?=20annotation).=20That=20made=20the=20check=20flag=2016=20packa?= =?UTF-8?q?ges=20(`@babel/core`,=20`@types/node`,=20`@types/react`,=20`@ty?= =?UTF-8?q?pescript-eslint/*`,=20`react`,=20`vite`,=20`tsx`,=20`typescript?= =?UTF-8?q?`,=20=E2=80=A6)=20as=20mismatched=20on=20every=20launch,=20trig?= =?UTF-8?q?gering=20a=20runtime=20`npm=20install`.=20Inside=20the=20Docker?= =?UTF-8?q?=20image,=20that=20runtime=20install=20then=20fails=20with=20EA?= =?UTF-8?q?CCES=20because=20`/opt/hermes/ui-tui/node=5Fmodules/`=20is=20ro?= =?UTF-8?q?ot-owned=20from=20build=20time,=20so=20`docker=20run=20?= =?UTF-8?q?=E2=80=A6=20hermes-agent=20--tui`=20prints:=20=20=20=20=20Insta?= =?UTF-8?q?lling=20TUI=20dependencies=E2=80=A6=20=20=20=20=20npm=20install?= =?UTF-8?q?=20failed.=20=E2=80=A6and=20exits=201,=20with=20no=20preview.?= =?UTF-8?q?=20The=20empty=20preview=20is=20a=20second=20bug:=20the=20launc?= =?UTF-8?q?her=20captured=20only=20stderr,=20but=20npm=209=20writes=20EACC?= =?UTF-8?q?ES=20to=20stdout,=20which=20was=20DEVNULL'd.=20Fixes:=20=20-=20?= =?UTF-8?q?Add=20`"peer"`=20to=20`=5FNPM=5FLOCK=5FRUNTIME=5FKEYS`=20so=20t?= =?UTF-8?q?he=20comparison=20ignores=20the=20=20=20=20non-deterministic=20?= =?UTF-8?q?field,=20alongside=20the=20existing=20`"ideallyInert"`.=20=20-?= =?UTF-8?q?=20Capture=20stdout=20as=20well=20as=20stderr=20in=20the=20inst?= =?UTF-8?q?all=20subprocess=20so=20future=20=20=20=20failures=20surface=20?= =?UTF-8?q?a=20useful=20preview=20instead=20of=20a=20bare=20"failed."=20li?= =?UTF-8?q?ne.=20Regression=20tests:=20=20-=20`test=5Fno=5Finstall=5Fwhen?= =?UTF-8?q?=5Fonly=5Fpeer=5Fannotation=5Fdiffers`=20=E2=80=94=20the=20exac?= =?UTF-8?q?t=20scenario=20=20-=20`test=5Finstall=5Fwhen=5Fversion=5Fdiffer?= =?UTF-8?q?s=5Feven=5Fwith=5Fpeer=5Fdrop`=20=E2=80=94=20guards=20against?= =?UTF-8?q?=20=20=20=20the=20peer-drop=20tolerance=20masking=20a=20real=20?= =?UTF-8?q?version=20skew=20On-host=20impact:=20the=20same=20false-positiv?= =?UTF-8?q?e=20was=20firing=20on=20every=20`hermes=20--tui`=20invocation?= =?UTF-8?q?=20from=20a=20normal=20checkout,=20silently=20running=20a=20no-?= =?UTF-8?q?op=20`npm=20install`=20each=20time=20(it=20converged=20because?= =?UTF-8?q?=20the=20host's=20`node=5Fmodules/`=20is=20writable).=20Startup?= =?UTF-8?q?=20time=20on=20the=20TUI=20should=20drop=20noticeably.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- hermes_cli/main.py | 22 +++++++++++++--- tests/hermes_cli/test_tui_npm_install.py | 33 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index d80e31f690..4fe5ff3508 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -837,7 +837,17 @@ def _print_tui_exit_summary(session_id: Optional[str], active_session_file: Opti ) -_NPM_LOCK_RUNTIME_KEYS = frozenset({"ideallyInert"}) +_NPM_LOCK_RUNTIME_KEYS = frozenset({"ideallyInert", "peer"}) +"""Lockfile fields npm writes non-deterministically at install time. + +``ideallyInert`` is npm's runtime annotation for packages it skipped installing +(per-platform opt-outs). ``peer`` is dropped from the hidden ``.package-lock.json`` +on dev-dependencies that are *also* declared as peers — the canonical +``package-lock.json`` records the dual role, but npm 9's actualized tree strips +it. Neither key represents a real skew between what was declared and what was +installed, so we exclude them from the comparison in :func:`_tui_need_npm_install` +to avoid false-positive reinstalls on every launch. +""" def _tui_need_npm_install(root: Path) -> bool: @@ -1042,17 +1052,21 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: if _tui_need_npm_install(tui_dir): if not os.environ.get("HERMES_QUIET"): print("Installing TUI dependencies…") + # Capture stdout as well as stderr — some npm errors (notably EACCES on a + # root-owned node_modules in containers) are emitted on stdout, and a + # bare "npm install failed." with no preview defeats debugging. We keep + # the failure-only print path so a successful install stays silent. result = subprocess.run( [npm, "install", "--silent", "--no-fund", "--no-audit", "--progress=false"], cwd=str(tui_dir), - stdout=subprocess.DEVNULL, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, env={**os.environ, "CI": "1"}, ) if result.returncode != 0: - err = (result.stderr or "").strip() - preview = "\n".join(err.splitlines()[-30:]) + combined = f"{result.stdout or ''}\n{result.stderr or ''}".strip() + preview = "\n".join(combined.splitlines()[-30:]) print("npm install failed.") if preview: print(preview) diff --git a/tests/hermes_cli/test_tui_npm_install.py b/tests/hermes_cli/test_tui_npm_install.py index e56196e07e..1dec625716 100644 --- a/tests/hermes_cli/test_tui_npm_install.py +++ b/tests/hermes_cli/test_tui_npm_install.py @@ -69,6 +69,39 @@ def test_no_install_when_only_optional_peer_package_missing_from_hidden_lock(tmp assert main_mod._tui_need_npm_install(tmp_path) is False +def test_no_install_when_only_peer_annotation_differs(tmp_path: Path, main_mod) -> None: + """npm 9 drops the ``peer`` flag from the hidden lock on dev-deps that are + *also* declared as peers. That's a cosmetic difference — the package is + installed at the requested version — so it must not trigger a reinstall. + Regression for the TUI-in-Docker failure where 16 such mismatches caused + `Installing TUI dependencies…` → EACCES on every launch. + """ + _touch_ink(tmp_path) + (tmp_path / "package-lock.json").write_text( + '{"packages":{' + '"node_modules/foo":{"version":"1.0.0","dev":true,"peer":true,"resolved":"https://x/foo.tgz"}' + '}}' + ) + (tmp_path / "node_modules" / ".package-lock.json").write_text( + '{"packages":{' + '"node_modules/foo":{"version":"1.0.0","dev":true,"resolved":"https://x/foo.tgz"}' + '}}' + ) + assert main_mod._tui_need_npm_install(tmp_path) is False + + +def test_install_when_version_differs_even_with_peer_drop(tmp_path: Path, main_mod) -> None: + """The peer-drop tolerance must not mask a real version skew.""" + _touch_ink(tmp_path) + (tmp_path / "package-lock.json").write_text( + '{"packages":{"node_modules/foo":{"version":"2.0.0","dev":true,"peer":true}}}' + ) + (tmp_path / "node_modules" / ".package-lock.json").write_text( + '{"packages":{"node_modules/foo":{"version":"1.0.0","dev":true}}}' + ) + assert main_mod._tui_need_npm_install(tmp_path) is True + + def test_no_install_when_lock_older_than_marker(tmp_path: Path, main_mod) -> None: _touch_ink(tmp_path) (tmp_path / "package-lock.json").write_text("{}")