Skip to content

fix(installer): skip node/npm/npx symlinks for user installs - #111921

Open
Lee-Si-Yoon wants to merge 1 commit into
NousResearch:mainfrom
Lee-Si-Yoon:fix/install-skip-user-node-symlinks
Open

Lee-Si-Yoon wants to merge 1 commit into
NousResearch:mainfrom
Lee-Si-Yoon:fix/install-skip-user-node-symlinks

Conversation

@Lee-Si-Yoon

@Lee-Si-Yoon Lee-Si-Yoon commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What

install_node_line() symlinks node/npm/npx into ~/.local/bin for user installs. rc files prepend that dir for uv, pipx, etc., so the links override the user's own Node in every interactive shell. which node resolves to Hermes' bundled Node, not nvm/Homebrew/volta.

Fixes #45279.

Fix

Gate the symlinks on install layout:

  • root FHS (/usr/local/bin): keep. Stock root shell has no other PATH entry for node.
  • Termux ($PREFIX/bin): keep. Same reason.
  • user (~/.local/bin): skip. Hermes finds its own Node by absolute path through find_hermes_node_executable(), not via PATH, so skipping is safe.

Why prior PRs stalled

This PR keys the decision on $ROOT_FHS_LAYOUT, the same variable get_command_link_dir() uses, so root FHS keeps its links and only the user case changes.

check_node() only reaches install_node_line() when the system Node was too old or had a bad npm band. The managed Node is what the install needs internally; this PR just stops dropping it into the user's interactive PATH.

Repro

Before (user install with nvm v24.21.0):

$ which node
/home/siyoon/.local/bin/node  ->  ~/.hermes/node/bin/node (Hermes v26.8.1)
$ node -v
v26.8.1

After:

$ which node
/home/siyoon/.nvm/versions/node/v24.21.0/bin/node
$ node -v
v24.21.0

Hermes still resolves its own Node: ~/.hermes/node/bin/node --version reports v26.8.1.

install_node_line() symlinks node/npm/npx into ~/.local/bin for user
installs. rc files prepend that dir for uv, pipx, etc., so the links
override the user's Node (nvm, Homebrew) in every interactive shell
(NousResearch#45279). Hermes finds its own Node by absolute path and skipping the
links is safe.

Keep the links for root FHS and Termux, where the link dir is the only
PATH entry the shell searches. Same policy as HERMES_NODE_SKIP_LINKS=1
in bootstrap_hermes_managed_node().

Fixes NousResearch#45279
@Lee-Si-Yoon
Lee-Si-Yoon force-pushed the fix/install-skip-user-node-symlinks branch from efa9f4f to 65e4640 Compare September 15, 2026 13:39
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor labels Sep 15, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: #45282 (open) fixes the same #45279 shadowing with a different gate (skip symlinks only when system node/npm already exist) and #45384 was an earlier closed-stale attempt with the layout gate used here. Two open approaches for a maintainer to pick between.

@kyssta-exe

Copy link
Copy Markdown
Contributor

Review: fix(installer): skip node/npm/npx symlinks for user installs

Summary
Stops user-layout installs from symlinking the managed Node into ~/.local/bin, where rc-file PATH prepending let it shadow the user's own toolchain (nvm/Homebrew/volta) in every interactive shell. Root FHS and Termux layouts keep the links. Fixes #45279.

What changed

  • scripts/install.sh (install_node_line) — symlink block gated on [ "$ROOT_FHS_LAYOUT" = true ] || is_termux, with a thorough comment explaining the gate and why skipping is safe (find_hermes_node_executable resolves by absolute path; with_hermes_node_path scopes PATH to Hermes-owned subprocesses).

Strengths

Findings

  • Edge: if is_termux is a function, || is_termux as a bare command in the [ ... ] || chain is correct; just confirming it's a predicate function and not a variable (unquoted $is_termux would behave differently) — the surrounding style suggests function, fine.
  • Non-blocking: the else branch log_info fires on every user install that reaches this path. Appropriate visibility, no change needed.

Verdict
Looks good to merge.

Reviewed using Hermes-Agent

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR #38889 still creates node/npm/npx shims in ~/.local/bin for user macOS installs, shadowing Homebrew/nvm

3 participants