mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-07 02:51:50 +00:00
fix(tui): tolerate npm's peer-flag drop in lockfile comparison
`_tui_need_npm_install()` compares the canonical `package-lock.json` against
the hidden `node_modules/.package-lock.json` to decide whether `npm install`
needs to re-run. npm 9 drops the `"peer": true` field from the hidden lock
on dev-deps that are *also* declared as peers (the canonical lock preserves
the dual annotation). That made the check flag 16 packages (`@babel/core`,
`@types/node`, `@types/react`, `@typescript-eslint/*`, `react`, `vite`,
`tsx`, `typescript`, …) as mismatched on every launch, triggering a runtime
`npm install`.
Inside the Docker image, that runtime install then fails with EACCES because
`/opt/hermes/ui-tui/node_modules/` is root-owned from build time, so
`docker run … hermes-agent --tui` prints:
Installing TUI dependencies…
npm install failed.
…and exits 1, with no preview. The empty preview is a second bug: the
launcher captured only stderr, but npm 9 writes EACCES to stdout, which
was DEVNULL'd.
Fixes:
- Add `"peer"` to `_NPM_LOCK_RUNTIME_KEYS` so the comparison ignores the
non-deterministic field, alongside the existing `"ideallyInert"`.
- Capture stdout as well as stderr in the install subprocess so future
failures surface a useful preview instead of a bare "failed." line.
Regression tests:
- `test_no_install_when_only_peer_annotation_differs` — the exact scenario
- `test_install_when_version_differs_even_with_peer_drop` — guards against
the peer-drop tolerance masking a real version skew
On-host impact: the same false-positive was firing on every `hermes --tui`
invocation from a normal checkout, silently running a no-op `npm install`
each time (it converged because the host's `node_modules/` is writable).
Startup time on the TUI should drop noticeably.
This commit is contained in:
parent
363cc93674
commit
2f2998bb1b
2 changed files with 51 additions and 4 deletions
|
|
@ -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:
|
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 _tui_need_npm_install(tui_dir):
|
||||||
if not os.environ.get("HERMES_QUIET"):
|
if not os.environ.get("HERMES_QUIET"):
|
||||||
print("Installing TUI dependencies…")
|
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(
|
result = subprocess.run(
|
||||||
[npm, "install", "--silent", "--no-fund", "--no-audit", "--progress=false"],
|
[npm, "install", "--silent", "--no-fund", "--no-audit", "--progress=false"],
|
||||||
cwd=str(tui_dir),
|
cwd=str(tui_dir),
|
||||||
stdout=subprocess.DEVNULL,
|
stdout=subprocess.PIPE,
|
||||||
stderr=subprocess.PIPE,
|
stderr=subprocess.PIPE,
|
||||||
text=True,
|
text=True,
|
||||||
env={**os.environ, "CI": "1"},
|
env={**os.environ, "CI": "1"},
|
||||||
)
|
)
|
||||||
if result.returncode != 0:
|
if result.returncode != 0:
|
||||||
err = (result.stderr or "").strip()
|
combined = f"{result.stdout or ''}\n{result.stderr or ''}".strip()
|
||||||
preview = "\n".join(err.splitlines()[-30:])
|
preview = "\n".join(combined.splitlines()[-30:])
|
||||||
print("npm install failed.")
|
print("npm install failed.")
|
||||||
if preview:
|
if preview:
|
||||||
print(preview)
|
print(preview)
|
||||||
|
|
|
||||||
|
|
@ -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
|
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:
|
def test_no_install_when_lock_older_than_marker(tmp_path: Path, main_mod) -> None:
|
||||||
_touch_ink(tmp_path)
|
_touch_ink(tmp_path)
|
||||||
(tmp_path / "package-lock.json").write_text("{}")
|
(tmp_path / "package-lock.json").write_text("{}")
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue