diff --git a/agent/lsp/install.py b/agent/lsp/install.py index 2671e7ccd3..fc9bea5930 100644 --- a/agent/lsp/install.py +++ b/agent/lsp/install.py @@ -35,6 +35,7 @@ from pathlib import Path from typing import Any, Dict, Optional from hermes_cli._subprocess_compat import windows_hide_flags +from hermes_constants import find_node_executable logger = logging.getLogger("agent.lsp.install") @@ -249,9 +250,12 @@ def _install_npm( peer deps that npm doesn't auto-pull (typescript-language-server needs ``typescript`` next to it; intelephense ships standalone). """ - npm = shutil.which("npm") + # Managed npm first: $HERMES_HOME/node is not on an arbitrary process's + # PATH, so a bare which() misses the Node that Hermes installed and + # reports "npm not on PATH" on a machine that has a perfectly good one. + npm = find_node_executable("npm") if npm is None: - logger.info("[install] cannot install %s: npm not on PATH", pkg) + logger.info("[install] cannot install %s: no usable npm found", pkg) return None staging = hermes_lsp_bin_dir().parent # /lsp/ install_targets = [pkg] + list(extra_pkgs or []) diff --git a/hermes_cli/dep_ensure.py b/hermes_cli/dep_ensure.py index 3f9cff71cc..1c87f7aa88 100644 --- a/hermes_cli/dep_ensure.py +++ b/hermes_cli/dep_ensure.py @@ -21,13 +21,16 @@ import subprocess import sys from pathlib import Path -from hermes_constants import agent_browser_runnable +from hermes_constants import agent_browser_runnable, find_node_executable from tools.environments.local import hermes_subprocess_env _IS_WINDOWS = platform.system() == "Windows" _DEP_CHECKS = { - "node": lambda: shutil.which("node") is not None, + # find_node_executable() rather than a bare which(): $HERMES_HOME/node is + # not on PATH, so which() would report Node missing on an install that has + # a managed one and trigger a redundant re-install. + "node": lambda: find_node_executable("node") is not None, "browser": lambda: ( agent_browser_runnable(shutil.which("agent-browser")) or _has_system_browser() diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 1d68388fe2..f29c42358a 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -2749,6 +2749,43 @@ def _systemd_watchdog_service_fields( return "notify", f"NotifyAccess=main\nWatchdogSec={seconds}s\n" +def _append_node_dir_for_service(path_entries: list[str]) -> None: + """Add the Node directory a generated service unit should use to *path_entries*. + + The Hermes-managed Node under ``$HERMES_HOME/node`` goes first when it + exists. A bare ``shutil.which("node")`` cannot be trusted on its own here: + a service unit is written once and then survives reboots, so resolving a + system Node that happens to be ahead on the installing shell's PATH bakes + the wrong interpreter in permanently — the exact failure the desktop + backend spawn was fixed for. Managed dirs are profile-scoped + (``get_hermes_home()``), so each profile's unit still names its own Node. + + PATH lookup remains the fallback rung for installs with no managed Node. + """ + from hermes_constants import iter_hermes_node_dirs + + for directory in iter_hermes_node_dirs(): + entry = str(directory) + if directory.is_dir() and entry not in path_entries: + path_entries.append(entry) + + resolved_node = shutil.which("node") + if not resolved_node: + return + + # Use the directory where ``node`` is *found on PATH*, NOT the symlink's + # resolved target. ``~/.local/bin/node`` is often a symlink into a + # specific profile's node install (e.g. profiles/jarvis/node/bin/node); + # calling .resolve() here would chase that symlink and bake one profile's + # node path into *every* profile's service unit. That cross-profile leak + # makes systemd_unit_is_current() perpetually false, so each gateway + # rewrites its unit + daemon-reload on every boot. Using the symlink's own + # parent keeps the generated unit profile-agnostic. + resolved_node_dir = str(Path(resolved_node).parent) + if resolved_node_dir not in path_entries: + path_entries.append(resolved_node_dir) + + def generate_systemd_unit(system: bool = False, run_as_user: str | None = None) -> str: python_path = get_python_path() working_dir = _stable_service_working_dir() @@ -2756,19 +2793,7 @@ def generate_systemd_unit(system: bool = False, run_as_user: str | None = None) venv_dir = str(detected_venv) if detected_venv else str(PROJECT_ROOT / "venv") path_entries = _build_service_path_dirs() - resolved_node = shutil.which("node") - if resolved_node: - # Use the directory where ``node`` is *found on PATH*, NOT the - # symlink's resolved target. ``~/.local/bin/node`` is often a symlink - # into a specific profile's node install (e.g. profiles/jarvis/node/ - # bin/node); calling .resolve() here would chase that symlink and bake - # one profile's node path into *every* profile's service unit. That - # cross-profile leak makes systemd_unit_is_current() perpetually false, - # so each gateway rewrites its unit + daemon-reload on every boot. Using - # the symlink's own parent keeps the generated unit profile-agnostic. - resolved_node_dir = str(Path(resolved_node).parent) - if resolved_node_dir not in path_entries: - path_entries.append(resolved_node_dir) + _append_node_dir_for_service(path_entries) common_bin_paths = [ "/usr/local/sbin", @@ -3961,17 +3986,7 @@ def generate_launchd_plist() -> str: # Resolve the directory containing the node binary (e.g. Homebrew, nvm) # so it's explicitly in PATH even if the user's shell PATH changes later. priority_dirs = _build_service_path_dirs() - resolved_node = shutil.which("node") - if resolved_node: - # Use the directory where ``node`` is *found on PATH*, NOT the symlink's - # resolved target. ``~/.local/bin/node`` is often a symlink into a - # specific profile's node install; calling .resolve() would chase it and - # bake one profile's path into every profile's service definition, - # breaking profile isolation and causing perpetual unit rewrites. See - # the matching fix in generate_systemd_unit(). - resolved_node_dir = str(Path(resolved_node).parent) - if resolved_node_dir not in priority_dirs: - priority_dirs.append(resolved_node_dir) + _append_node_dir_for_service(priority_dirs) sane_path = ":".join( dict.fromkeys( priority_dirs + [p for p in os.environ.get("PATH", "").split(":") if p] diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 9b4611b3ad..e3b8986c67 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -1938,12 +1938,18 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: env_node = os.environ.get("HERMES_NODE") if env_node and os.path.isfile(env_node) and os.access(env_node, os.X_OK): return env_node - path = shutil.which(bin) + # find_node_executable() prefers the managed $HERMES_HOME/node tree, + # which is not on PATH — a bare which() would declare "node not found" + # and exit on an install whose only Node is the one Hermes installed, + # and would pick a system Node over the managed one when both exist. + from hermes_constants import find_node_executable + + path = find_node_executable(bin) if not path and bin == "node": try: from hermes_cli.dep_ensure import ensure_dependency if ensure_dependency("node"): - path = shutil.which("node") + path = find_node_executable("node") except Exception: pass if not path: diff --git a/hermes_cli/setup.py b/hermes_cli/setup.py index 6fe25a3fdb..d18c813f5e 100644 --- a/hermes_cli/setup.py +++ b/hermes_cli/setup.py @@ -1563,7 +1563,14 @@ def setup_terminal_backend(config: dict): print_info("Installing vercel SDK...") import subprocess - uv_bin = shutil.which("uv") + # Managed uv first: $HERMES_HOME/bin is never on PATH, so a bare + # which() misses the uv Hermes installed. Bootstrapping one is + # welcome here — this is the interactive setup wizard, already + # mid-install, and the alternative tier is a pip that a `uv venv` + # venv may not even have. + from hermes_cli.managed_uv import ensure_uv + + uv_bin = ensure_uv() if uv_bin: result = subprocess.run( [uv_bin, "pip", "install", "--python", sys.executable, "vercel"], diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index dab26dbd91..d36a38685f 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -790,7 +790,15 @@ def _pip_install( venv_root = Path(sys.executable).parent.parent uv_env = {**os.environ, "VIRTUAL_ENV": str(venv_root)} - uv_bin = shutil.which("uv") + # Managed uv first: $HERMES_HOME/bin is never on PATH, so a bare which() + # misses the uv Hermes installed and prefers a system one when both exist. + # ensure_uv() rather than a pure lookup because this runs during setup, + # where installing uv is in scope — and tier 2 is a pip that the Windows + # installer's `uv venv` does not seed, so failing to find uv here is the + # difference between a working post-setup hook and "No module named pip". + from hermes_cli.managed_uv import ensure_uv + + uv_bin = ensure_uv() if uv_bin: try: result = subprocess.run( @@ -1608,10 +1616,15 @@ def _run_cua_driver_installer( def _run_post_setup(post_setup_key: str): """Run post-setup hooks for tools that need extra installation steps.""" import shutil + from hermes_constants import find_node_executable + if post_setup_key in {"agent_browser", "browserbase"}: node_modules = PROJECT_ROOT / "node_modules" / "agent-browser" - npm_bin = shutil.which("npm") - npx_bin = shutil.which("npx") + # Managed Node first — $HERMES_HOME/node is not on PATH, so a bare + # which() reports "no npm" on installs whose only Node is the one + # Hermes installed for exactly this toolchain. + npm_bin = find_node_executable("npm") + npx_bin = find_node_executable("npx") # Step 1: install the agent-browser npm package into node_modules/ if not node_modules.exists() and npm_bin: _print_info(" Installing Node.js dependencies for browser tools...") @@ -1729,7 +1742,7 @@ def _run_post_setup(post_setup_key: str): elif post_setup_key == "camofox": camofox_dir = PROJECT_ROOT / "node_modules" / "@askjo" / "camofox-browser" - _npm_bin = shutil.which("npm") + _npm_bin = find_node_executable("npm") if camofox_dir.exists(): _print_success(" Camofox already installed, nothing to do") elif _npm_bin: @@ -1751,7 +1764,7 @@ def _run_post_setup(post_setup_key: str): _print_info(" npx @askjo/camofox-browser") _print_info(" First run downloads the Camoufox engine (~300MB)") _print_info(" Or use Docker: docker run -p 9377:9377 -e CAMOFOX_PORT=9377 jo-inc/camofox-browser") - elif not shutil.which("npm"): + elif not _npm_bin: _print_warning(" Node.js not found. Install Camofox via Docker:") _print_info(" docker run -p 9377:9377 -e CAMOFOX_PORT=9377 jo-inc/camofox-browser") diff --git a/hermes_constants.py b/hermes_constants.py index dc0de54481..69e9d3d13e 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -294,8 +294,9 @@ def iter_hermes_node_dirs(home: Path | None = None) -> list[Path]: dirs = [root / "node"] bin_dir = root / "node" / "bin" # NOTE: keep this ordering in sync with hermesManagedNodePathEntries() in - # apps/desktop/electron/main.cjs — the Electron main process is Node and - # cannot import this module, so the platform-ordering rule is mirrored there. + # apps/desktop/electron/backend-env.ts — the Electron main process is Node + # and cannot import this module, so the platform-ordering rule is mirrored + # there (once; main.ts imports it rather than keeping its own copy). if sys.platform == "win32": return dirs + [bin_dir] return [bin_dir] + dirs diff --git a/scripts/ci/test_install_ps1_path_migration.ps1 b/scripts/ci/test_install_ps1_path_migration.ps1 new file mode 100644 index 0000000000..232d49fb24 --- /dev/null +++ b/scripts/ci/test_install_ps1_path_migration.ps1 @@ -0,0 +1,125 @@ +# Behavioral test for install.ps1's persisted-User-PATH migration. +# +# Run: pwsh -NoProfile -File scripts/ci/test_install_ps1_path_migration.ps1 +# +# Not wired into the default CI lane — the Linux runners have no PowerShell +# host. It runs on any machine with pwsh (including via nixpkgs#powershell), +# and on a Windows runner if one is ever added. +# +# This is NOT a source-regex test. It parses install.ps1, lifts the real +# Set-ManagedNodeFirstOnUserPath body out of the AST, and rewrites *only* the +# two registry calls into an in-memory store so the actual shipped logic — +# split, dedupe, prepend, change-detection — executes for real. Rewriting from +# the AST rather than hand-copying the body means the test cannot silently +# drift away from the function it claims to cover. + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$installPs1 = Join-Path $PSScriptRoot '..' 'install.ps1' | Resolve-Path +$ast = [System.Management.Automation.Language.Parser]::ParseFile( + $installPs1, [ref]$null, [ref]$null) + +$fn = $ast.Find({ + param($n) + $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and + $n.Name -eq 'Set-ManagedNodeFirstOnUserPath' +}, $true) + +if (-not $fn) { + throw "Set-ManagedNodeFirstOnUserPath not found in $installPs1" +} + +# Swap the two registry calls for the in-memory store. Both must match, or the +# function has changed shape and this harness is no longer exercising it. +# Rewrite the whole definition extent (which already carries `function +# { param(...) ... }`) so the shipped param block and body run verbatim. +$definition = $fn.Extent.Text +$reads = ([regex]'\[Environment\]::GetEnvironmentVariable\("Path", "User"\)').Matches($definition).Count +$writes = ([regex]'\[Environment\]::SetEnvironmentVariable\("Path", ([^,]+), "User"\)').Matches($definition).Count +if ($reads -ne 1 -or $writes -ne 1) { + throw "expected exactly one User PATH read and one write in the function body; found $reads read(s), $writes write(s). Update this harness." +} + +$definition = $definition -replace ` + '\[Environment\]::GetEnvironmentVariable\("Path", "User"\)', '$script:FakeUserPath' +$definition = $definition -replace ` + '\[Environment\]::SetEnvironmentVariable\("Path", ([^,]+), "User"\)', '$script:FakeUserPath = $1; $script:FakeWrites++' + +Invoke-Expression $definition + +$NODE = 'C:\Users\me\AppData\Local\hermes\node' +$script:Failures = 0 + +function Invoke-Migration { + param([string]$Start, [string]$NodeDir = $NODE) + $script:FakeUserPath = $Start + $script:FakeWrites = 0 + Set-ManagedNodeFirstOnUserPath $NodeDir +} + +function Assert-Equal { + param($Expected, $Actual, [string]$Name) + if ($Expected -ceq $Actual) { + Write-Host " PASS $Name" + } else { + Write-Host " FAIL $Name" + Write-Host " expected: [$Expected]" + Write-Host " actual: [$Actual]" + $script:Failures++ + } +} + +Write-Host "install.ps1 Set-ManagedNodeFirstOnUserPath" + +# The regression this function exists for: an install made by an older +# install.ps1, which *appended*. A system Node leads and the managed dir is +# stranded at the tail, so every new shell resolves the wrong node.exe. An +# add-if-missing check would see the entry present and leave it there forever. +Invoke-Migration "C:\Program Files\nodejs;C:\Users\me\bin;$NODE" +Assert-Equal "$NODE;C:\Program Files\nodejs;C:\Users\me\bin" $script:FakeUserPath ` + 'upgrade from appending installer: managed dir becomes first entry' +Assert-Equal 1 (@($script:FakeUserPath -split ';' | Where-Object { $_ -eq $NODE }).Count) ` + 'upgrade: managed dir is not duplicated' +Assert-Equal "C:\Program Files\nodejs;C:\Users\me\bin" ` + (($script:FakeUserPath -split ';' | Where-Object { $_ -ne $NODE }) -join ';') ` + 'upgrade: unrelated entries keep their relative order' +Assert-Equal 1 $script:FakeWrites 'upgrade: persists exactly once' + +Invoke-Migration "$NODE;C:\Program Files\nodejs" +Assert-Equal "$NODE;C:\Program Files\nodejs" $script:FakeUserPath 'already correct: unchanged' +Assert-Equal 0 $script:FakeWrites 'already correct: no registry write' + +Invoke-Migration "C:\Program Files\nodejs" +Assert-Equal "$NODE;C:\Program Files\nodejs" $script:FakeUserPath 'fresh install: prepended' + +# Empty segments are legal in a real User PATH (a trailing ';' is common) and +# the installer's other PATH code preserves them. Migration must not quietly +# rewrite parts of PATH it was not asked to touch. +Invoke-Migration "C:\Program Files\nodejs;;C:\Users\me\bin;" +Assert-Equal "$NODE;C:\Program Files\nodejs;;C:\Users\me\bin;" $script:FakeUserPath ` + 'empty segments are preserved' + +# Windows paths are case-insensitive, and -ne on strings is too. +Invoke-Migration "C:\Program Files\nodejs;c:\users\me\appdata\local\HERMES\Node" +Assert-Equal "$NODE;C:\Program Files\nodejs" $script:FakeUserPath ` + 'existing entry in different case is replaced, not duplicated' + +Invoke-Migration "$NODE;C:\Program Files\nodejs;$NODE" +Assert-Equal "$NODE;C:\Program Files\nodejs" $script:FakeUserPath 'duplicates collapse' + +Invoke-Migration "" +Assert-Equal $NODE $script:FakeUserPath 'empty User PATH' + +Invoke-Migration "C:\Program Files\nodejs" "" +Assert-Equal "C:\Program Files\nodejs" $script:FakeUserPath 'empty NodeDir is a no-op' +Assert-Equal 0 $script:FakeWrites 'empty NodeDir does not write' + +if ($script:Failures -gt 0) { + Write-Host "" + Write-Host "$script:Failures assertion(s) failed" + exit 1 +} + +Write-Host "" +Write-Host "all assertions passed" diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 184ae2e348..577cf18e41 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -528,6 +528,41 @@ function Ensure-NodeExeOnPath { return $true } +# Put the Hermes-managed Node dir at the FRONT of the persisted User PATH. +# +# Appending is not enough: it leaves a pre-existing system Node ahead of the +# bundled one in every new shell, so anything launched without a curated +# environment (a standalone hermes-setup.exe run, a user typing `npm`) silently +# resolves the wrong Node. Bundled must win. +# +# Move-to-front rather than add-if-missing, because installs made by an older +# install.ps1 already have this dir in User PATH -- at the tail. An +# add-if-missing check sees it present and leaves the broken ordering in place +# forever, so the very users the ordering bug hurt would never be repaired. +# +# Unrelated entries keep their relative order, including empty segments (a +# trailing ';' is legal and common in a real User PATH; Install-Git's splitting +# preserves them too, so this must not quietly rewrite them). Duplicate +# occurrences of the managed dir collapse into the single leading entry. +# PowerShell's -ne is case-insensitive for strings, which is the right +# comparison on Windows. Persists only when the resulting string differs, so +# an already-correct PATH costs one registry read and no write. +function Set-ManagedNodeFirstOnUserPath { + param([string]$NodeDir) + + if (-not $NodeDir) { return } + + $userPath = [Environment]::GetEnvironmentVariable("Path", "User") + $items = if ($userPath) { @($userPath -split ";") } else { @() } + + $rest = @($items | Where-Object { $_ -ne $NodeDir }) + $updated = (@($NodeDir) + $rest) -join ";" + + if ($updated -ne $userPath) { + [Environment]::SetEnvironmentVariable("Path", $updated, "User") + } +} + # Re-discover uv without re-installing it. Cross-process stage drivers # (the desktop GUI's onboarding wizard, CI step-runners) invoke each stage # in a fresh powershell process, so $script:UvCmd set by Install-Uv in a @@ -1090,6 +1125,7 @@ function Test-Node { if ((Test-Path $managedNode) -and (Test-NodeVersionOk (& $managedNode --version))) { $version = & $managedNode --version $env:Path = "$HermesHome\node;$env:Path" + Set-ManagedNodeFirstOnUserPath "$HermesHome\node" Write-Success "Node.js $version found (Hermes-managed)" $script:HasNode = $true return $true @@ -1132,20 +1168,10 @@ function Test-Node { # Persist to User PATH so fresh shells (and future stages # in cross-process driver mode) see it. Matches the - # pattern Install-Git uses for PortableGit. - # - # PREPEND, don't append. Appending leaves a pre-existing - # system Node ahead of the bundled one in every new shell, - # so anything launched without a curated environment (a - # standalone hermes-setup.exe run, a user typing `npm`) - # silently resolves the wrong Node. Bundled must win. - $nodeDir = "$HermesHome\node" - $userPath = [Environment]::GetEnvironmentVariable("Path", "User") - $userPathItems = if ($userPath) { $userPath -split ";" } else { @() } - if ($userPathItems -notcontains $nodeDir) { - $userPathItems = @($nodeDir) + $userPathItems - [Environment]::SetEnvironmentVariable("Path", ($userPathItems -join ";"), "User") - } + # pattern Install-Git uses for PortableGit. See + # Set-ManagedNodeFirstOnUserPath for why this is a + # move-to-front and not an add-if-missing. + Set-ManagedNodeFirstOnUserPath "$HermesHome\node" $version = & "$HermesHome\node\node.exe" --version Write-Success "Node.js $version installed to $HermesHome\node\ (portable, user-scoped)" diff --git a/tests/test_managed_runtime_resolution.py b/tests/test_managed_runtime_resolution.py new file mode 100644 index 0000000000..2a5d1b5528 --- /dev/null +++ b/tests/test_managed_runtime_resolution.py @@ -0,0 +1,203 @@ +"""Guard: Hermes-owned subprocesses must not resolve managed runtimes by bare PATH. + +Hermes installs runtimes for itself — ``uv`` at ``$HERMES_HOME/bin/uv``, Node at +``$HERMES_HOME/node``. Neither directory is on the ambient PATH of an arbitrary +process, so ``shutil.which("uv")`` / ``shutil.which("node")`` in Hermes's own +code has two failure modes: + +* the managed runtime is invisible, so the caller reports "not installed" or + degrades to a slower tier on a machine that has exactly what it needed; and +* when a system copy also exists, the one Hermes does not own wins — which is + how a generated systemd unit or launchd plist can bake a system Node in and + keep resolving it across reboots. + +The fix per call site is one of ``find_node_executable()``, +``iter_hermes_node_dirs()``, ``resolve_uv()``, or ``ensure_uv()``. This test is +the ratchet that stops a new bare lookup from being added back. + +Reading source is normally banned (see AGENTS.md). It is the right tool here and +only here: the property under test is "no call site anywhere in the tree spells +it this way", which is a statement about the whole codebase rather than about +one function's behavior, and there is no runtime seam that can observe a lookup +that was never written. Every entry in the allow-list below names a call site +whose behavior is separately covered by a real test. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] + +# Runtimes Hermes provisions into HERMES_HOME and must therefore resolve +# through a managed-aware helper rather than PATH. +_MANAGED_COMMANDS = frozenset({"uv", "node", "npm", "npx"}) + +# Directories that are not Hermes-owned subprocess code: plugins ship their own +# resolution policy, tests assert against PATH deliberately, and skills/scripts +# run as standalone user-invoked programs. +_EXEMPT_DIRS = ( + "tests", + "plugins", + "skills", + "optional-skills", + "scripts", + "website", + "node_modules", + ".git", + ".venv", + "venv", + ".worktrees", +) + +# Call sites where a bare PATH lookup is the correct answer. Each entry is +# (path, command) -> why. Keep this list short and justified — the default +# answer for a new call site is a managed-aware helper, not a new exemption. +_ALLOWED: dict[tuple[str, str], str] = { + ("tools/env_probe.py", "uv"): ( + "Reports the environment the MODEL sees in the terminal tool. The model " + "can only run what is on that subshell's PATH, which local.py populates " + "with the managed dirs — so PATH is the correct question to ask here." + ), + ("hermes_cli/update_cmd.py", "uv"): ( + "Termux fallback: a pkg-installed uv lands on PATH but not in the " + "managed bin dir, and it is checked only after resolve_uv() misses." + ), + ("hermes_cli/update_cmd.py", "npm"): ( + "WSL diagnostic: deliberately inspects what PATH resolves so it can " + "warn that the only reachable npm is the Windows one." + ), + ("tools/lazy_deps.py", "uv"): ( + "Fallback after resolve_uv(), plus the except-branch for the " + "hermes_cli import guard." + ), + ("hermes_cli/gateway.py", "node"): ( + "Fallback rung of _append_node_dir_for_service(), after the managed " + "dirs from iter_hermes_node_dirs() are already appended." + ), + ("hermes_cli/main.py", "node"): ( + "_ensure_tui_node()'s idempotence gate: the question really is 'is " + "node already discoverable on PATH', before bootstrapping one." + ), + ("hermes_cli/main.py", "npm"): ( + "Same _ensure_tui_node() gate as node." + ), + ("tools/browser_tool.py", "npx"): ( + "agent-browser runs via `npx`, resolved against the extended browser " + "PATH that _merge_browser_path() already seeds with the managed dirs." + ), +} + + +def _iter_which_calls(tree: ast.AST): + """Yield (command, lineno) for every ``which("")`` call in *tree*. + + AST rather than a regex so prose in docstrings and comments that mentions + ``shutil.which("npm")`` is not mistaken for a call site. + """ + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or not node.args: + continue + func = node.func + name = ( + func.attr + if isinstance(func, ast.Attribute) + else func.id if isinstance(func, ast.Name) else None + ) + if name != "which": + continue + first = node.args[0] + if isinstance(first, ast.Constant) and first.value in _MANAGED_COMMANDS: + yield first.value, node.lineno + + +def _source_files() -> list[Path]: + files: list[Path] = [] + for path in REPO_ROOT.rglob("*.py"): + rel = path.relative_to(REPO_ROOT) + if rel.parts and rel.parts[0] in _EXEMPT_DIRS: + continue + files.append(path) + return files + + +def _findings() -> list[tuple[str, str, int]]: + """Return (relpath, command, lineno) for every bare managed lookup.""" + found: list[tuple[str, str, int]] = [] + for path in _source_files(): + try: + source = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + if "which(" not in source: + continue + try: + tree = ast.parse(source) + except SyntaxError: + continue + rel = path.relative_to(REPO_ROOT).as_posix() + for command, lineno in _iter_which_calls(tree): + found.append((rel, command, lineno)) + return found + + +def test_no_unreviewed_bare_managed_runtime_lookups(): + """Every bare which() for a managed runtime is a reviewed exemption.""" + unexpected = [ + (rel, cmd, lineno) + for rel, cmd, lineno in _findings() + if (rel, cmd) not in _ALLOWED + ] + + assert not unexpected, ( + "Bare PATH lookup for a Hermes-managed runtime.\n\n" + + "\n".join(f" {rel}:{lineno} which({cmd!r})" for rel, cmd, lineno in unexpected) + + "\n\n$HERMES_HOME/bin (uv) and $HERMES_HOME/node are not on an " + "arbitrary process's PATH, so this resolves a system copy — or nothing " + "— on an install that has a managed one.\n" + "Use instead:\n" + " uv -> managed_uv.resolve_uv() (lookup) or ensure_uv() (may install)\n" + " node/npm -> hermes_constants.find_node_executable()\n" + " PATH env -> hermes_constants.iter_hermes_node_dirs()\n" + "If PATH really is the right question, add the site to _ALLOWED with a " + "reason." + ) + + +def test_allowlist_has_no_stale_entries(): + """A fixed call site must be dropped from the allow-list, not left to rot.""" + live = {(rel, cmd) for rel, cmd, _lineno in _findings()} + stale = sorted(set(_ALLOWED) - live) + + assert not stale, ( + "Allow-list entries no longer match any source line — the call site was " + "fixed or moved. Remove them:\n" + + "\n".join(f" {rel} ({cmd})" for rel, cmd in stale) + ) + + +@pytest.mark.parametrize( + "helper", + [ + "find_node_executable", + "find_hermes_node_executable", + "iter_hermes_node_dirs", + "with_hermes_node_path", + ], +) +def test_managed_node_helpers_exist(helper): + """The alternatives this guard points contributors at must be importable.""" + import hermes_constants + + assert callable(getattr(hermes_constants, helper)) + + +def test_managed_uv_helpers_exist(): + from hermes_cli.managed_uv import ensure_uv, managed_uv_path, resolve_uv + + assert callable(resolve_uv) + assert callable(ensure_uv) + assert managed_uv_path().parent.name == "bin" diff --git a/tools/env_probe.py b/tools/env_probe.py index d2031c5c34..f656af3edc 100644 --- a/tools/env_probe.py +++ b/tools/env_probe.py @@ -203,6 +203,12 @@ def _build_probe_line() -> str: py3_has_pip = _has_pip_module("python3") if py3_ver else False pip_bound_to = _pip_python_version() py3_pep668 = _detect_pep668("python3") if py3_ver else False + # Bare which() is correct here, unlike Hermes's own uv call sites: this + # reports the environment *the model will see* in the terminal tool, and + # what the model can type is exactly what is on that subshell's PATH. + # local.py puts the Hermes-managed $HERMES_HOME/bin there, so a managed-only + # install answers yes — without that, claiming uv the model cannot invoke + # would be worse than claiming none. has_uv = shutil.which("uv") is not None # If python3 exists, has pip, has uv (or no PEP 668), and there's no diff --git a/tools/environments/local.py b/tools/environments/local.py index f3ad9d514b..7422e875de 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -1127,6 +1127,34 @@ def _prepend_hermes_bin_dir(existing_path: str) -> str: return sep.join([bin_dir, *entries]) +def _managed_runtime_path_entries() -> list[str]: + """Return existing Hermes-managed runtime dirs for the terminal subshell PATH. + + The terminal tool spawns a subshell whose PATH is the agent process's PATH + plus ``_SANE_PATH``. Neither carries the runtimes Hermes installs for + itself, so on a machine where Hermes provisioned its own toolchain a + command the agent runs resolves a system copy instead — or nothing at all: + + - ``$HERMES_HOME/node`` (+ ``/bin``) — installed to satisfy the desktop and + browser toolchain. ``tools/browser_tool.py`` already does this for its own + subprocesses; the agent's shell deserves the same. + - ``$HERMES_HOME/bin`` — the managed ``uv``. ``install.sh`` writes it there + and nothing has ever put that directory on PATH, so an install whose only + uv is the managed one looks uv-less to both the agent and the model. + + Resolved per call rather than cached in a module constant because + ``get_hermes_home()`` is profile-scoped and a managed tree can appear + mid-process (``heal_hermes_managed_node``, a first browser install). + """ + try: + from hermes_constants import get_hermes_home, iter_hermes_node_dirs + + candidates = [*iter_hermes_node_dirs(), get_hermes_home() / "bin"] + return [str(d) for d in candidates if d.is_dir()] + except Exception: + return [] + + def _append_missing_sane_path_entries(existing_path: str) -> str: """Return a normalised POSIX PATH with missing sane entries appended. @@ -1144,6 +1172,11 @@ def _append_missing_sane_path_entries(existing_path: str) -> str: - **Duplicates are collapsed** (first occurrence wins), so a caller PATH that already contains repeats is not propagated verbatim. + Hermes-managed runtime dirs are appended alongside the sane entries, not + prepended: a tool the user deliberately put on their own PATH still wins, + and the managed one only fills the gap where there would otherwise be + nothing. + For a well-formed PATH (no empties, no duplicates) the leading segment is byte-identical to the input and ordering is preserved; only the missing sane entries are appended. On Windows this is a no-op passthrough (the @@ -1153,6 +1186,9 @@ def _append_missing_sane_path_entries(existing_path: str) -> str: return existing_path sane_entries = [entry for entry in _SANE_PATH.split(":") if entry] + sane_entries.extend( + entry for entry in _managed_runtime_path_entries() if entry not in sane_entries + ) if not existing_path: return ":".join(sane_entries) diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index 8633239104..15992f7006 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -732,7 +732,18 @@ def _venv_pip_install(specs: tuple[str, ...], *, timeout: int = 300) -> _Install uv_env["VIRTUAL_ENV"] = str(venv_root) # Tier 1: uv (preferred — fast, doesn't need pip in the venv) - uv_bin = shutil.which("uv") + # Managed uv first: $HERMES_HOME/bin is never on PATH, so a bare + # which() misses the uv Hermes installed and falls through to the + # slower pip tier. Deliberately a lookup and not ensure_uv(): this runs + # mid-turn to install an optional dependency, and downloading uv + + # migrating the Python runtime as a side effect of that is a far bigger + # action than the caller asked for. Tier 2 pip covers the no-uv case. + try: + from hermes_cli.managed_uv import resolve_uv + + uv_bin = resolve_uv() or shutil.which("uv") + except Exception: + uv_bin = shutil.which("uv") if uv_bin: try: r = subprocess.run(