From 94136efccaf13a1ae4ce19eea85a539a90410313 Mon Sep 17 00:00:00 2001 From: konsisumer Date: Wed, 15 Jul 2026 19:24:45 +0200 Subject: [PATCH] fix(cli): don't run Windows npm on WSL update and stop reporting success on Node refresh failure --- hermes_cli/main.py | 172 ++++++++++++++++++++++---- tests/hermes_cli/test_cmd_update.py | 180 ++++++++++++++++++++++++++++ 2 files changed, 327 insertions(+), 25 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 45ce0a7a8..1712d7888 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -4896,9 +4896,9 @@ def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: encoding = getattr(sys.stdout, "encoding", None) or "ascii" print(text.encode(encoding, errors="replace").decode(encoding, errors="replace")) - from hermes_constants import find_node_executable, with_hermes_node_path + from hermes_constants import with_hermes_node_path - npm = find_node_executable("npm") + npm = _resolve_node_runtime_npm() if not npm: if fatal: _say("Web UI frontend not built and npm is not available.") @@ -5636,7 +5636,7 @@ def cmd_gui(args: argparse.Namespace): except Exception: pass - from hermes_constants import find_node_executable, with_hermes_node_path + from hermes_constants import with_hermes_node_path # with_hermes_node_path() copies os.environ when called with no arg. env = with_hermes_node_path() @@ -5666,7 +5666,7 @@ def cmd_gui(args: argparse.Namespace): packaged_executable = _desktop_packaged_executable(desktop_dir) if source_mode or not skip_build: - npm = find_node_executable("npm") + npm = _resolve_node_runtime_npm() if not npm: print("Desktop GUI requires Node.js/npm, but npm was not found on PATH.") print("Install Node.js, then run: hermes gui") @@ -6400,7 +6400,7 @@ def _update_via_zip(args): ) _install_python_dependencies_with_optional_fallback(pip_cmd) - _update_node_dependencies() + node_failures = _update_node_dependencies() _build_web_ui(PROJECT_ROOT / "web") # Sync skills @@ -6439,7 +6439,15 @@ def _update_via_zip(args): logger.debug("Model catalog seed during zip update failed: %s", e) print() - print("✓ Update complete!") + if node_failures: + print( + "⚠ Update partially complete — Node.js dependencies for " + f"{', '.join(node_failures)} did not refresh." + ) + print(" Code and Python deps are updated, but the dashboard/TUI may") + print(" be in a mixed state until the Node deps are rebuilt.") + else: + print("✓ Update complete!") try: _print_curator_first_run_notice() except Exception as e: @@ -6448,7 +6456,14 @@ def _update_via_zip(args): _print_curator_recent_run_notice() except Exception as e: logger.debug("Curator recent-run notice failed: %s", e) - _kill_stale_dashboard_processes() + # Don't stop a working dashboard when the Node refresh failed — see the + # git-update path for rationale (#30271). + if node_failures: + print() + print(" ℹ Leaving running dashboard process(es) untouched because the") + print(" Node.js dependency refresh did not complete.") + else: + _kill_stale_dashboard_processes() def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[str]: @@ -8210,15 +8225,94 @@ def _record_npm_lockfile_hash(hermes_root: Path) -> None: logger.debug("Could not write npm lockfile hash cache") -def _update_node_dependencies() -> None: - from hermes_constants import find_node_executable, with_hermes_node_path +def _is_windows_npm_path(npm_path: str) -> bool: + """Return True if ``npm_path`` points at a Windows npm shim. + + On WSL the Windows install dir is exposed through the ``/mnt/c`` drive + mount and PATH interop, so ``shutil.which("npm")`` can hand back + ``/mnt/c/Program Files/nodejs/npm`` (or the ``npm.cmd`` / ``npm.exe`` + shim). Those are detected here by their ``.exe``/``.cmd``/``.bat`` + suffix, a ``/mnt/`` drive-mount prefix, or an embedded backslash (a UNC + path). Callers use this only on a POSIX host — on native Windows an + ``npm.cmd`` shim is the correct executable. + """ + low = npm_path.lower() + return ( + low.endswith((".exe", ".cmd", ".bat")) + or low.startswith("/mnt/") + or "\\" in npm_path + ) + + +def _resolve_node_runtime_npm() -> str | None: + """Resolve an npm executable that belongs to the host's Node runtime. + + On WSL/Linux ``shutil.which("npm")`` may resolve a Windows npm exposed + through PATH interop. Running that Windows npm against the Linux checkout + operates over ``\\wsl.localhost\\...`` UNC paths and fails with EISDIR / + symlink errors in symlink-heavy trees like ``ui-tui`` (#30271). Refuse a + Windows npm on a POSIX host and re-scan PATH (skipping ``/mnt/*`` interop + entries) for a Linux-native npm. Returns the npm path, or ``None`` when + no suitable npm is reachable. + """ + from hermes_constants import find_node_executable npm = find_node_executable("npm") - if not npm: - return + # On native Windows the platform npm (``npm.cmd``) is exactly what we + # want — only reject Windows shims when we're a POSIX/WSL process. + if _is_windows(): + return npm + + if not npm: + return None + + if not _is_windows_npm_path(npm): + return npm + + # The first resolution was a Windows npm. Re-scan PATH skipping the + # ``/mnt/*`` Windows drive mounts WSL injects, so a Linux-native npm that + # came later on PATH is still found. + for directory in os.environ.get("PATH", "").split(os.pathsep): + if not directory or directory.lower().startswith("/mnt/"): + continue + candidate = shutil.which("npm", path=directory) + if candidate and not _is_windows_npm_path(candidate): + return candidate + return None + + +def _update_node_dependencies() -> list[str]: + """Refresh Node deps in the repo root and update workspaces. + + Returns the list of labels whose npm install failed (empty on success), + so the caller can treat a Node refresh failure as a partial update rather + than silently reporting ``Update complete!`` (#30271). + """ if not (PROJECT_ROOT / "package.json").exists(): - return + return [] + + npm = _resolve_node_runtime_npm() + if not npm: + # If the only npm reachable inside this WSL shell is the Windows one, + # flag it loudly: silently skipping leaves ui-tui deps stale while the + # rest of the update proceeds, and running it would corrupt the tree. + from hermes_constants import is_wsl + + path_npm = shutil.which("npm") + if is_wsl() and path_npm and _is_windows_npm_path(path_npm): + print("→ Updating Node.js dependencies...") + print(" ⚠ Skipped: only a Windows npm is reachable from this WSL shell.") + print(" Install Node.js inside the WSL distro (nvm, or your distro's") + print(" package manager), then re-run `hermes update`.") + failed = ["repo root"] + if any( + (PROJECT_ROOT / workspace / "package.json").exists() + for workspace in ("ui-tui", "web") + ): + failed.append("ui-tui, web workspaces") + return failed + return [] from hermes_constants import get_default_hermes_root @@ -8228,7 +8322,7 @@ def _update_node_dependencies() -> None: shared_hermes_root = get_default_hermes_root() if not _npm_lockfile_changed(shared_hermes_root): logger.info("npm lockfile unchanged, skipping npm install") - return + return [] # With a single workspace lockfile the root install would cover ALL # workspaces — but apps/desktop pulls in Electron as a devDependency, @@ -8238,8 +8332,18 @@ def _update_node_dependencies() -> None: # Desktop deps are installed on demand by the desktop launcher # (see _desktop_build_needed). print("→ Updating Node.js dependencies...") + + def _partial_update_failure(*labels: str) -> list[str]: + print() + print(" ⚠ Node.js dependency refresh did not complete cleanly; the") + print(" installation may be in a mixed state (updated code, stale Node") + print(" deps). Fix npm and re-run `hermes update`.") + return list(labels) + extra_args = ["--no-fund", "--no-audit", "--progress=false"] + from hermes_constants import with_hermes_node_path + nixos_env = with_hermes_node_path(_nixos_build_env()) # Step 1: root install (no workspace recursion). @@ -8261,7 +8365,7 @@ def _update_node_dependencies() -> None: stderr = (root_result.stderr or "").strip() if root_result.stderr else "" if stderr: print(f" {stderr.splitlines()[-1]}") - return + return _partial_update_failure("repo root") # Step 2: install only the workspaces update needs (ui-tui, web). # --workspace selects specific workspaces; the rest (desktop) are skipped. @@ -8276,11 +8380,13 @@ def _update_node_dependencies() -> None: if ws_result.returncode == 0: _record_npm_lockfile_hash(shared_hermes_root) print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") - else: - print(" ⚠ npm workspace install failed") - stderr = (ws_result.stderr or "").strip() if ws_result.stderr else "" - if stderr: - print(f" {stderr.splitlines()[-1]}") + return [] + + print(" ⚠ npm workspace install failed") + stderr = (ws_result.stderr or "").strip() if ws_result.stderr else "" + if stderr: + print(f" {stderr.splitlines()[-1]}") + return _partial_update_failure("ui-tui, web workspaces") class _UpdateOutputStream: @@ -10074,7 +10180,7 @@ def _cmd_update_impl(args, gateway_mode: bool): _refresh_active_lazy_features() - _update_node_dependencies() + node_failures = _update_node_dependencies() _build_web_ui(PROJECT_ROOT / "web") # Rebuild the desktop app if the source tree changed since the last @@ -10086,9 +10192,7 @@ def _cmd_update_impl(args, gateway_mode: bool): # Electron build by ``hermes update``. desktop_dir = PROJECT_ROOT / "apps" / "desktop" has_desktop_app = _desktop_packaged_executable(desktop_dir) is not None or _desktop_dist_exists(desktop_dir) - from hermes_constants import find_node_executable - - if (desktop_dir / "package.json").exists() and find_node_executable("npm") and has_desktop_app: + if (desktop_dir / "package.json").exists() and _resolve_node_runtime_npm() and has_desktop_app: print("→ Checking if desktop app needs rebuilding...") _desktop_build_cmd = [sys.executable, "-m", "hermes_cli.main", "desktop", "--build-only"] # Capture the (very loud) Electron/vite build output into @@ -10376,7 +10480,15 @@ def _cmd_update_impl(args, gateway_mode: bool): logger.debug("Cron jobs auto-restore check failed: %s", exc) print() - print("✓ Update complete!") + if node_failures: + print( + "⚠ Update partially complete — Node.js dependencies for " + f"{', '.join(node_failures)} did not refresh." + ) + print(" Code and Python deps are updated, but the dashboard/TUI may") + print(" be in a mixed state until the Node deps are rebuilt.") + else: + print("✓ Update complete!") # Curator first-run heads-up. Only prints when curator is enabled AND # has never run — i.e. the window where the ticker would otherwise @@ -11134,7 +11246,17 @@ def _cmd_update_impl(args, gateway_mode: bool): # silent frontend/backend mismatch. We can't auto-restart it # (no saved launch args) but we can stop it, and a hint is # printed for the user to re-launch. - _kill_stale_dashboard_processes() + # + # Exception: if the Node dependency refresh failed, the rebuilt + # frontend the new backend expects may not exist, so stopping a + # working dashboard would leave the user with nothing running + # rather than a usable (if mixed) state (#30271). Leave it alone. + if node_failures: + print() + print(" ℹ Leaving running dashboard process(es) untouched because the") + print(" Node.js dependency refresh did not complete.") + else: + _kill_stale_dashboard_processes() print() print("Tip: You can now select a provider and model:") diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index b1458cbd3..dd3d2130a 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -1060,3 +1060,183 @@ termux = ["rich>=14"] assert hm._load_installable_optional_extras(group="all") == ["mcp"] assert hm._load_installable_optional_extras(group="termux-all") == ["termux", "mcp"] + + +class TestNodeRuntimeNpmResolution: + """Regression tests for #30271 — WSL must not run Windows npm against the + Linux checkout, and a failed Node refresh must not report success.""" + + @pytest.mark.parametrize( + "path", + [ + "/mnt/c/Program Files/nodejs/npm", + "/mnt/c/Program Files/nodejs/npm.cmd", + "C:\\Program Files\\nodejs\\npm.exe", + "/usr/local/bin/npm.bat", + ], + ) + def test_windows_npm_paths_detected(self, path): + from hermes_cli import main as hm + + assert hm._is_windows_npm_path(path) is True + + @pytest.mark.parametrize( + "path", + [ + "/usr/bin/npm", + "/root/.local/bin/npm", + "/home/u/.nvm/versions/node/v22/bin/npm", + ], + ) + def test_linux_npm_paths_not_flagged(self, path): + from hermes_cli import main as hm + + assert hm._is_windows_npm_path(path) is False + + def test_resolve_rejects_windows_npm_and_rescans_path(self, monkeypatch): + """On WSL/Linux, a Windows npm is refused and PATH is re-scanned + (skipping /mnt mounts) for a Linux-native npm.""" + from hermes_cli import main as hm + import hermes_constants + + monkeypatch.setattr(hm, "_is_windows", lambda: False) + monkeypatch.setenv( + "PATH", "/mnt/c/Program Files/nodejs:/root/.local/bin:/usr/bin" + ) + + def fake_which(cmd, path=None): + if path is None: + # Mirrors WSL: interop puts the Windows shim first on PATH. + return "/mnt/c/Program Files/nodejs/npm" + if path == "/root/.local/bin": + return "/root/.local/bin/npm" + return None + + monkeypatch.setattr( + hermes_constants, + "find_node_executable", + lambda _command: "/mnt/c/Program Files/nodejs/npm", + ) + monkeypatch.setattr(hm.shutil, "which", fake_which) + assert hm._resolve_node_runtime_npm() == "/root/.local/bin/npm" + + def test_resolve_returns_none_when_only_windows_npm(self, monkeypatch): + from hermes_cli import main as hm + import hermes_constants + + monkeypatch.setattr(hm, "_is_windows", lambda: False) + monkeypatch.setenv("PATH", "/mnt/c/Program Files/nodejs:/usr/bin") + + def fake_which(cmd, path=None): + if path is None: + return "/mnt/c/Program Files/nodejs/npm" + return None + + monkeypatch.setattr( + hermes_constants, + "find_node_executable", + lambda _command: "/mnt/c/Program Files/nodejs/npm", + ) + monkeypatch.setattr(hm.shutil, "which", fake_which) + assert hm._resolve_node_runtime_npm() is None + + def test_resolve_keeps_platform_npm_on_windows(self, monkeypatch): + from hermes_cli import main as hm + import hermes_constants + + monkeypatch.setattr(hm, "_is_windows", lambda: True) + monkeypatch.setattr( + hermes_constants, + "find_node_executable", + lambda _command: "C:\\nodejs\\npm.cmd", + ) + assert hm._resolve_node_runtime_npm() == "C:\\nodejs\\npm.cmd" + + def test_node_failure_returns_failed_labels_and_warns( + self, tmp_path, monkeypatch, capsys + ): + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_resolve_node_runtime_npm", lambda: "/usr/bin/npm") + monkeypatch.setattr( + hm, + "_run_npm_install_deterministic", + lambda *a, **k: subprocess.CompletedProcess([], 1, stdout="", stderr=""), + ) + + failed = hm._update_node_dependencies() + assert failed == ["repo root"] + out = capsys.readouterr().out + assert "mixed state" in out + + def test_node_success_returns_empty(self, tmp_path, monkeypatch): + from hermes_cli import main as hm + + (tmp_path / "package.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_resolve_node_runtime_npm", lambda: "/usr/bin/npm") + monkeypatch.setattr( + hm, + "_run_npm_install_deterministic", + lambda *a, **k: subprocess.CompletedProcess([], 0, stdout="", stderr=""), + ) + + assert hm._update_node_dependencies() == [] + + def test_wsl_windows_only_npm_flags_skip(self, tmp_path, monkeypatch, capsys): + from hermes_cli import main as hm + import hermes_constants + + (tmp_path / "package.json").write_text("{}") + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + monkeypatch.setattr(hm, "_resolve_node_runtime_npm", lambda: None) + monkeypatch.setattr(hermes_constants, "is_wsl", lambda: True) + monkeypatch.setattr( + hm.shutil, "which", lambda cmd, path=None: "/mnt/c/nodejs/npm" + ) + + failed = hm._update_node_dependencies() + assert failed == ["repo root"] + assert "Windows npm" in capsys.readouterr().out + + def test_wsl_update_skips_windows_npm_build_paths(self, mock_args, monkeypatch): + """A Windows-only npm on WSL must not reach web or desktop builds.""" + from hermes_cli import main as hm + import hermes_constants + + windows_npm = "/mnt/c/Program Files/nodejs/npm" + monkeypatch.setattr(hm, "_is_windows", lambda: False) + monkeypatch.setattr(hermes_constants, "is_wsl", lambda: True) + monkeypatch.setattr( + hermes_constants, + "find_node_executable", + lambda command: windows_npm if command == "npm" else None, + ) + monkeypatch.setattr( + hm.shutil, + "which", + lambda command, path=None: windows_npm if command == "npm" else "/usr/bin/uv", + ) + monkeypatch.setenv("PATH", "/mnt/c/Program Files/nodejs") + + with patch("subprocess.run") as mock_run, \ + patch.object(hm, "_web_ui_build_needed", return_value=True), \ + patch.object(hm, "_desktop_packaged_executable", return_value=None), \ + patch.object(hm, "_desktop_dist_exists", return_value=True), \ + patch.object(hm, "_run_npm_install_deterministic") as mock_npm_install, \ + patch.object(hm, "_run_with_idle_timeout") as mock_idle_build, \ + patch.object(hm, "_run_logged_subprocess") as mock_desktop_build: + mock_run.side_effect = _make_run_side_effect( + branch="main", verify_ok=True, commit_count="1" + ) + cmd_update(mock_args) + + mock_npm_install.assert_not_called() + mock_idle_build.assert_not_called() + mock_desktop_build.assert_not_called() + assert all( + not call.args or not call.args[0] or call.args[0][0] != windows_npm + for call in mock_run.call_args_list + )