fix(install): keep Hermes-managed Node private instead of hijacking the user's node - #47897
raysonmeng wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Solid infrastructure improvement. This PR keeps Hermes-managed Node private under $HERMES_HOME/node instead of placing symlinks in user PATH directories (/.local/bin, /usr/local/bin), preventing Hermes from accidentally shadowing the user's own Node installations. Also cleans up legacy symlinks and prioritizes user version managers (fnm, Volta, nvm) before falling back to Hermes's private Node.
Changes
- hermes_cli/dep_ensure.py: Path management helpers, legacy symlink cleanup
- hermes_cli/main.py: TUI node bootstrap cleanup
- hermes_cli/uninstall.py: Updated docstring for clarity
- scripts/install.sh: Node installation, symlink cleanup, version manager preference
- scripts/lib/node-bootstrap.sh: POSIX node bootstrap
Security
- No hardcoded secrets or credentials
- Path manipulation is careful and scoped to Hermes directories only
Quality
- Well-commented throughout with rationale for each change
- Migration-safe: all new columns are nullable for backward compatibility
- Idempotent operations throughout
Testing
- No explicit tests added, but this is an infrastructure change that would require integration testing to fully verify
Reviewed by Hermes Agent
…he user's node The POSIX installer symlinked Hermes's bundled node/npm/npx into ~/.local/bin (and set the managed npm global prefix to the link dir's parent), which shadowed a user's nvm/fnm/Volta node on PATH and recurred on every install/update. This keeps the managed Node private to Hermes subprocesses and migrates away the legacy shims. - install.sh / node-bootstrap.sh: stop symlinking node/npm/npx into command dirs; set the managed npm global prefix to $HERMES_HOME/node (global bins land in $HERMES_HOME/node/bin -- already on Hermes's runtime PATH and detected by _has_hermes_agent_browser, matching install.sh's explicit --prefix install). - Migration: install / bootstrap / update / lazy-ensure entrypoints remove legacy node/npm/npx shims, but only links that resolve into THIS $HERMES_HOME/node -- user-repointed links (nvm/fnm/Volta) are left untouched. - Prefer an existing fnm/Volta/nvm node before falling back to Hermes's private copy, so a one-time fallback never permanently wins over a version manager. - dep_ensure / TUI / update path resolve $HERMES_HOME/node/bin without a PATH symlink, and never prepend the private node when the user already has node on PATH. - hermes update cleans up legacy shims even on the "already up to date" early return, so updates don't reintroduce the hijack. - Per-platform managed-Node layout: POSIX node/bin, Windows portable zip node/. - Resolve managed Node through the shared hermes_constants helpers (find_node_executable / with_hermes_node_path / iter_hermes_node_dirs) instead of hand-rolled per-platform path logic -- rebased onto current main, where those helpers (kept in sync with apps/desktop/electron/main.cjs) now exist. - hermes setup: the post-setup browser/camofox hook resolves npm/npx via find_node_executable and runs them with with_hermes_node_path(), so browser tooling still installs on a Hermes-private-node box (no node/npm on PATH) once the legacy shims are gone. 中文:POSIX 安装器把 Hermes 自带的 node/npm/npx symlink 进 ~/.local/bin(并把 managed npm 全局 prefix 设到 link 目录的父目录),会在 PATH 上 shadow 用户的 nvm/fnm/Volta node,且每次 install/update 复发。本改动让 managed Node 仅供 Hermes 子进程私用,并迁移清理 legacy shim:不再 symlink 进命令目录;npm 全局 prefix 改为 $HERMES_HOME/node;migration 只删解析进本机 $HERMES_HOME/node 的链接(用户改指 nvm/fnm/Volta 的保留);复用私有 node 前先优先用户版本管理器;update 的 already-up-to-date 早返回也清理;按平台区分 node 布局(POSIX node/bin,Windows node/)。 Node 解析统一改走 hermes_constants 的官方 helper(rebase 到最新 main 后这些 helper 已 存在),并顺带修好 hermes setup 的 post-setup 浏览器安装在私有 node 机器上的回归。 Tests: touched suites pass -- test_dep_ensure, test_cmd_update, test_tools_config, test_uninstall_node_symlinks, test_tui_resume_flow, test_install_sh_node_global_prefix, test_hermes_constants.
77976a1 to
fc86e2f
Compare
|
@OutThisLife — gentle ping for a review when you get a chance 🙏 Status: this is rebased onto latest Heads-up on overlap: #49426 targets the same managed-Node symlink bug. This PR (#47897) is rebased onto current |
|
Closing — the premise no longer holds on current The managed-Node symlink hijack this targets is already solved: #46085 added The shared helpers this revision says it "adopts from main" ( Thanks for the thorough work — the isolation goal is achieved on main via the prefix-local npmrc rather than by dropping the symlinks. |
Update — rebased onto latest
main(+ adopted shared helpers, +hermes setupfix)This branch was ~580 commits behind
main; it has been rebased onto currentmainand the overlap reconciled:hermes_cli/main.py_update_node_dependencies). Since this branch was cut,mainindependently introducedhermes_constants.{find_node_executable, with_hermes_node_path, iter_hermes_node_dirs}for managed-Node resolution. The Python side now uses those shared helpers instead of the hand-rolled per-platform path logic this PR originally added —dep_ensure.pyno longer re-derives$HERMES_HOME/node[/bin]; it defers toiter_hermes_node_dirs/find_hermes_node_executable(which are kept in sync withapps/desktop/electron/main.cjs). The install-side fix and the legacy-shim migration are unchanged — the symlink hijack still exists on currentmain, so this fix is still needed.hermes setupregression fix (new in this revision). Removing the PATH symlinks exposed a pre-existing bareshutil.which("npm"/"npx")inhermes_cli/tools_config.py::_run_post_setup: on a machine whose only Node was the Hermes symlink, the post-setup browser/camofox install was silently skipped. It now resolves viafind_node_executableand runs npm/npx withenv=with_hermes_node_path()(mirroring the existing WhatsApp / web-build call sites inmain.py). Added a regression test (test_post_setup_browser_uses_managed_node_when_path_has_none) that fails on the old code.test_dep_ensure,test_cmd_update,test_tools_config,test_uninstall_node_symlinks,test_tui_resume_flow,test_install_sh_node_global_prefix,test_hermes_constants.Deliberately out of scope (pre-existing, not introduced here)
Removing the PATH symlinks also exposes other pre-existing bare
shutil.which/command -v nodecall sites that the symlinks were silently backstopping. These are not on the critical install / first-run / update path (which self-heals via_ensure_tui_node/ensure_dependency/ the desktop PATH builder), so they are left for a separate follow-up:apps/desktop/electron/backend-env.cjs— Windows: omits thenode/root from the spawned backend's PATH (pre-dates this PR).hermes_cli/main.py_launch_tui— copiesos.environbefore_ensure_tui_node()prependsnode/bin, so the TUI child env can lack the managed dir.hermes_cli/doctor.py— diagnostic-only false "node not found" on a private-node box.agent/lsp/install.py,plugins/platforms/photon/*— opt-in flows.try_existing_*node probes pick the lexicographically-first version ≥ build floor rather than newest /nvm default(nosort -V).What does this PR do?
On POSIX, the installer makes Hermes's bundled Node hijack the user's own Node.
scripts/install.sh(and the lazyscripts/lib/node-bootstrap.sh) symlink the managednode/npm/npxinto~/.local/bin(or/usr/local/bin/$PREFIX/bin) and point the managed npm global prefix at the link dir's parent. Because~/.local/bintypically sits ahead of a version manager's shims on PATH, this silently shadows the user'snvm/fnm/VoltaNode — and it recurs on every install/update/lazy-ensure, so removing the symlinks by hand doesn't stick.This makes the managed Node private to Hermes subprocesses and migrates away the legacy shims, so Hermes stops competing for the user's
nodeon PATH.Related Issue
No tracking issue — happy to open one if you'd like it linked.
Type of Change
Changes Made
scripts/install.sh,scripts/lib/node-bootstrap.sh— stop symlinkingnode/npm/npxinto command dirs on install. Set the managed npm global prefix to$HERMES_HOME/node(sonpm install -gbins land in$HERMES_HOME/node/bin, which is already on Hermes's runtime PATH and matches the explicit--prefix "$HERMES_HOME/node"the installer already uses for agent-browser — rather than an off-PATH$HERMES_HOME/bin).check_node/ensure_node/hermes update/ lazyensure_dependencyremove legacynode/npm/npxshims, but only links that resolve into this$HERMES_HOME/node. Links the user repointed elsewhere (nvm/fnm/Volta) are left untouched (reusesuninstall.remove_node_symlinks).check_node/ensure_nodeprobe existingfnm/Volta/nvmNode before falling back to the private copy, so a one-time fallback never permanently wins over a version manager.hermes_cli/dep_ensure.py,hermes_cli/main.py— resolve$HERMES_HOME/node/binfor Hermes subprocesses without relying on a PATH symlink, and never prepend the private Node when the user already hasnodeon PATH.hermes updateidempotency — legacy shims are cleaned up even on the "Already up to date" early-return, so updates don't reintroduce the hijack.node/binon POSIX andnode/for the Windows portable zip; the newdep_ensure/update helpers honor both.tests/for every behavior above.How to Test
Reproduction (POSIX):
nvm/fnm/Voltaprovidingnodeon PATH, run the installer (orbash scripts/install.sh --ensure node).~/.local/bin/{node,npm,npx}get symlinked to the bundled Node and shadow the version manager (readlink $(command -v node)points into$HERMES_HOME/node); the symlinks come back after eachhermes update.$HERMES_HOME/nodeare removed; user-repointed links are preserved.Automated:
scripts/run_tests.sh \ tests/test_install_sh_node_global_prefix.py \ tests/hermes_cli/test_dep_ensure.py \ tests/hermes_cli/test_cmd_update.py \ tests/hermes_cli/test_uninstall_node_symlinks.py \ tests/hermes_cli/test_tui_resume_flow.py # 107 passedAlso verified with two temp-
HOMEsandboxes:install.sh --ensure nodeandnode-bootstrap ensure_nodeboth remove legacy~/.local/binshims, prefer an existing nvm Node, and writeprefix=$HERMES_HOME/node.Cross-platform note (install.sh ↔ install.ps1)
Per CONTRIBUTING rule #13 I checked
scripts/install.ps1: the POSIX bugs do not exist on Windows, so no equivalent ps1 change is needed for this fix. install.ps1 appends$HERMES_HOME\nodeto the user PATH (it does not shadow a user's existing Node, which sits earlier on PATH) and already installs agent-browser with an explicit--prefix "$HERMES_HOME\node"(on PATH). The new Python helpers are made layout-correct for both platforms (Windows portable Node lives innode\, POSIX innode/bin/). Fully isolating the managed Node from the persistent Windows User PATH would be a separate, larger, Windows-tested change and is intentionally out of scope here.One known non-blocking nit:
main.py:_ensure_tui_nodestill appendsnode/binliterally, but that path is POSIX-only by construction (it's driven bybash -c 'source node-bootstrap.sh') and guarded byif extra.is_dir(), so it is a harmless no-op on Windows. Happy to platformize it for consistency if preferred.Checklist
Code
fix(install): ...)Documentation & Housekeeping
cli-config.yaml.example— N/A (no config keys changed)CONTRIBUTING.md/AGENTS.md— N/A (no workflow change)