mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(desktop): force npm --include=dev so self-update rebuild can't be broken by NODE_ENV=production (#38416)
The desktop self-update rebuild (`hermes desktop --build-only`, driven by the macOS in-app updater) runs `_run_npm_install_deterministic`, which used plain `npm ci` / `npm install`. Those honor an inherited `NODE_ENV=production` (or npm `omit=dev`) and silently omit devDependencies. The desktop build toolchain — tsc, vite, electron-builder — are all devDependencies, so under `NODE_ENV=production` the install completes (exit 0) but `tsc` is never placed, and the very next step (`tsc -b && vite build`) dies with `tsc: command not found` (exit 127). The user sees "UPDATE DIDN'T FINISH / Rebuilding the desktop app failed (exit 127)" even though the git/pip update applied cleanly. NODE_ENV=production can leak in from a shell profile, a parent process, or a packaged-app launch context. Force `--include=dev` on both the `npm ci` and `npm install` paths. The only callers are frontend builds (desktop / TUI / web), which always need the dev toolchain, so this is safe across the board. Verified empirically: under NODE_ENV=production, `npm ci` leaves tsc MISSING while `npm ci --include=dev` installs it. Added two regression tests (test_npm_ci_forces_include_dev, test_npm_install_fallback_forces_include_dev) — both confirmed to FAIL when the fix is reverted.
This commit is contained in:
parent
007cd15132
commit
0678f8f019
4 changed files with 108 additions and 5 deletions
|
|
@ -1822,6 +1822,12 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]:
|
|||
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",
|
||||
|
|
@ -4832,6 +4838,25 @@ def _run_npm_install_deterministic(
|
|||
rewrites committed lockfiles (stripping ``"peer": true`` etc.), which leaves
|
||||
the working tree dirty and causes the next ``hermes update`` to stash the
|
||||
lockfile — repeatedly.
|
||||
|
||||
``--include=dev`` is forced on every invocation: the callers are frontend
|
||||
builds (web UI / TUI / desktop workspaces), and those builds need the dev
|
||||
toolchain (``tsc``, ``vite``, ``electron-builder`` — all
|
||||
``devDependencies``). If the caller's environment has
|
||||
``NODE_ENV=production`` (or npm config ``omit=dev``) — which leaks in from
|
||||
a shell profile, a container image, or the bundled TUI launcher that sets
|
||||
``NODE_ENV=production`` on its subprocess env — npm silently omits
|
||||
devDependencies (exit 0, no error), so the build toolchain never installs
|
||||
and the subsequent build dies with ``tsc: command not found`` (exit 127).
|
||||
The flag overrides both the env var and npm config, unlike scrubbing
|
||||
``NODE_ENV`` from the environment which only fixes the env-leak case.
|
||||
|
||||
``--no-save`` on the ``npm install`` fallback keeps it true to this
|
||||
function's contract: never mutate ``package-lock.json``. Without it, an
|
||||
out-of-sync lockfile gets rewritten by the fallback, which drifts the
|
||||
committed lockfile and makes every future ``npm ci`` fail — a
|
||||
self-reinforcing cycle where web devDeps never install and a stale dist
|
||||
is served on every update (PR #65595).
|
||||
"""
|
||||
# unicode-animations' postinstall animates to /dev/tty (bypasses
|
||||
# --silent/capture_output). It no-ops when CI is set — same as the TUI
|
||||
|
|
@ -4840,7 +4865,7 @@ def _run_npm_install_deterministic(
|
|||
|
||||
lockfile = cwd / "package-lock.json"
|
||||
if lockfile.exists():
|
||||
ci_cmd = [npm, "ci", *extra_args]
|
||||
ci_cmd = [npm, "ci", "--include=dev", *extra_args]
|
||||
ci_result = subprocess.run(
|
||||
ci_cmd,
|
||||
cwd=cwd,
|
||||
|
|
@ -4855,7 +4880,7 @@ def _run_npm_install_deterministic(
|
|||
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", *extra_args]
|
||||
install_cmd = [npm, "install", "--no-save", "--include=dev", *extra_args]
|
||||
return subprocess.run(
|
||||
install_cmd,
|
||||
cwd=cwd,
|
||||
|
|
|
|||
|
|
@ -483,6 +483,7 @@ class TestCmdUpdateBranchFallback:
|
|||
root_flags = [
|
||||
"/usr/bin/npm",
|
||||
"ci",
|
||||
"--include=dev",
|
||||
"--no-fund",
|
||||
"--no-audit",
|
||||
"--progress=false",
|
||||
|
|
@ -491,6 +492,7 @@ class TestCmdUpdateBranchFallback:
|
|||
ws_flags = [
|
||||
"/usr/bin/npm",
|
||||
"ci",
|
||||
"--include=dev",
|
||||
"--no-fund",
|
||||
"--no-audit",
|
||||
"--progress=false",
|
||||
|
|
@ -507,7 +509,7 @@ class TestCmdUpdateBranchFallback:
|
|||
# The web/ install runs from the workspace root when the root
|
||||
# lockfile exists (npm workspaces hoist node_modules upward).
|
||||
assert npm_calls[2:] == [
|
||||
(["/usr/bin/npm", "ci", "--workspace", "web", "--silent"], PROJECT_ROOT),
|
||||
(["/usr/bin/npm", "ci", "--include=dev", "--workspace", "web", "--silent"], PROJECT_ROOT),
|
||||
]
|
||||
|
||||
# The web UI build itself went through the streaming helper.
|
||||
|
|
|
|||
|
|
@ -265,6 +265,7 @@ def test_make_tui_argv_keeps_desktop_workspace_install_behaviour(
|
|||
"install",
|
||||
"--workspace",
|
||||
"ui-tui",
|
||||
"--include=dev",
|
||||
"--silent",
|
||||
"--no-fund",
|
||||
"--no-audit",
|
||||
|
|
@ -275,6 +276,39 @@ def test_make_tui_argv_keeps_desktop_workspace_install_behaviour(
|
|||
_assert_utf8_replace_capture(calls[1][1])
|
||||
|
||||
|
||||
def test_make_tui_argv_npm_install_forces_include_dev(
|
||||
tmp_path: Path, main_mod, monkeypatch
|
||||
) -> None:
|
||||
"""The TUI-launch npm install must force --include=dev: ui-tui's build
|
||||
toolchain (esbuild, typescript) lives in devDependencies, and an inherited
|
||||
NODE_ENV=production (container shells; a parent TUI sets it on its own
|
||||
subprocess env) or an npm `omit=dev` config would silently skip them,
|
||||
breaking the TUI build with `tsc`/`esbuild: command not found."""
|
||||
tui_dir = tmp_path / "ui-tui"
|
||||
tui_dir.mkdir()
|
||||
(tui_dir / "package.json").write_text("{}")
|
||||
(tmp_path / "package-lock.json").write_text("{}")
|
||||
|
||||
monkeypatch.delenv("TERMUX_VERSION", raising=False)
|
||||
monkeypatch.setenv("PREFIX", "/usr")
|
||||
monkeypatch.setenv("NODE_ENV", "production")
|
||||
monkeypatch.setattr(main_mod, "_tui_need_npm_install", lambda _root: True)
|
||||
monkeypatch.setattr(main_mod.shutil, "which", lambda name: f"/bin/{name}")
|
||||
calls = []
|
||||
|
||||
def fake_run(*args, **kwargs):
|
||||
calls.append((args, kwargs))
|
||||
return types.SimpleNamespace(returncode=0, stdout="", stderr="")
|
||||
|
||||
monkeypatch.setattr(main_mod.subprocess, "run", fake_run)
|
||||
|
||||
main_mod._make_tui_argv(tui_dir, tui_dev=False)
|
||||
|
||||
install_cmd = calls[0][0][0]
|
||||
assert install_cmd[:2] == ["/bin/npm", "install"]
|
||||
assert "--include=dev" in install_cmd
|
||||
|
||||
|
||||
def test_make_tui_argv_keeps_desktop_always_build_behaviour(
|
||||
tmp_path: Path, main_mod, monkeypatch
|
||||
) -> None:
|
||||
|
|
|
|||
|
|
@ -156,6 +156,47 @@ class TestBuildWebUISkipsWhenFresh:
|
|||
assert kwargs["env"]["CI"] == "1"
|
||||
assert kwargs["env"]["PYTHON"] == "/nix/store/python"
|
||||
|
||||
def test_npm_ci_forces_include_dev(self, tmp_path):
|
||||
"""`npm ci` must pass --include=dev so an inherited NODE_ENV=production
|
||||
(e.g. from a container shell, or the bundled TUI launcher which sets
|
||||
NODE_ENV=production on its subprocess env) or an npm `omit=dev` config
|
||||
can't silently strip the build toolchain (tsc/vite/electron-builder),
|
||||
which otherwise fails the web/desktop build with `tsc: command not
|
||||
found` (exit 127) despite the install exiting 0."""
|
||||
web_dir, _ = _make_web_dir(tmp_path)
|
||||
(web_dir / "package-lock.json").write_text("{}", encoding="utf-8")
|
||||
|
||||
mock_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
with patch("hermes_cli.main.subprocess.run", return_value=mock_cp) as mock_run:
|
||||
_run_npm_install_deterministic("/usr/bin/npm", web_dir)
|
||||
|
||||
args, _ = mock_run.call_args
|
||||
cmd = args[0]
|
||||
assert cmd[:2] == ["/usr/bin/npm", "ci"]
|
||||
assert "--include=dev" in cmd
|
||||
|
||||
def test_npm_install_fallback_forces_include_dev_and_no_save(self, tmp_path):
|
||||
"""When `npm ci` fails (lockfile out of sync) the `npm install`
|
||||
fallback must still force --include=dev (same NODE_ENV rationale as
|
||||
above) and must pass --no-save so the fallback never rewrites the
|
||||
committed lockfile — a drifted lockfile makes every future `npm ci`
|
||||
fail, a self-reinforcing cycle that keeps devDeps from installing."""
|
||||
web_dir, _ = _make_web_dir(tmp_path)
|
||||
(web_dir / "package-lock.json").write_text("{}", encoding="utf-8")
|
||||
|
||||
ci_fail = __import__("subprocess").CompletedProcess([], 1, stdout="", stderr="lockfile out of sync")
|
||||
install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
with patch("hermes_cli.main.subprocess.run", side_effect=[ci_fail, install_ok]) as mock_run:
|
||||
result = _run_npm_install_deterministic("/usr/bin/npm", web_dir)
|
||||
|
||||
assert result.returncode == 0
|
||||
assert mock_run.call_count == 2
|
||||
install_args, _ = mock_run.call_args_list[1]
|
||||
install_cmd = install_args[0]
|
||||
assert install_cmd[:2] == ["/usr/bin/npm", "install"]
|
||||
assert "--include=dev" in install_cmd
|
||||
assert "--no-save" in install_cmd
|
||||
|
||||
def test_npm_install_uses_workspace_web_scope(self, tmp_path):
|
||||
web_dir, _ = _make_web_dir(tmp_path)
|
||||
# Real workspace checkout: the single lockfile lives at the root, so
|
||||
|
|
@ -201,7 +242,7 @@ class TestBuildWebUISkipsWhenFresh:
|
|||
assert result is True
|
||||
args, kwargs = mock_run.call_args
|
||||
assert "--workspace" not in args[0]
|
||||
assert args[0] == ["/usr/bin/npm", "ci", "--silent"]
|
||||
assert args[0] == ["/usr/bin/npm", "ci", "--include=dev", "--silent"]
|
||||
assert kwargs["cwd"] == web_dir
|
||||
|
||||
def test_web_build_uses_idle_timeout_helper(self, tmp_path):
|
||||
|
|
@ -245,6 +286,7 @@ class TestBuildWebUISkipsWhenFresh:
|
|||
assert args[0] == [
|
||||
"/usr/bin/npm",
|
||||
"ci",
|
||||
"--include=dev",
|
||||
"--workspace",
|
||||
"web",
|
||||
"--include-workspace-root=false",
|
||||
|
|
@ -269,7 +311,7 @@ class TestBuildWebUISkipsWhenFresh:
|
|||
|
||||
assert result is True
|
||||
args, kwargs = mock_run.call_args
|
||||
assert args[0] == ["/usr/bin/npm", "ci", "--workspace", "web", "--silent"]
|
||||
assert args[0] == ["/usr/bin/npm", "ci", "--include=dev", "--workspace", "web", "--silent"]
|
||||
assert kwargs["cwd"] == tmp_path
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue