Skip to content

fix(install): skip node/npm/npx symlinks when system versions exist - #45282

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/install-skip-node-symlinks-when-system-exists
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/install-skip-node-symlinks-when-system-exists

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents the POSIX installer from shadowing the user's system-installed Node.js (Homebrew, nvm, etc.) by only creating node/npm/npx symlinks in ~/.local/bin when no existing binary is found on $PATH.

Related Issue

Fixes #45279

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • scripts/install.sh: Wrap the ln -sf calls for node/npm/npx in a command -v guard so symlinks are only created when the command is absent from $PATH. The comment explains the rationale and links to the issue.
  • tests/test_install_sh_node_symlink_shadow.py: Two regression tests verifying (1) unconditional symlinks are removed, and (2) the command -v guard is present.

How to Test

  1. On macOS with Homebrew node installed (brew install node), verify which node resolves to /opt/homebrew/bin/node.
  2. Run the installer: curl -fsSL https://raw.githubusercontent.com/liuhao1024/hermes-agent/fix/install-skip-node-symlinks-when-system-exists/scripts/install.sh | bash
  3. After install, verify which node still resolves to /opt/homebrew/bin/node (not ~/.local/bin/node).
  4. Verify ~/.local/bin/node does NOT exist (symlink was skipped).
  5. Run the regression tests: pytest tests/test_install_sh_node_symlink_shadow.py -v

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: scripts/install.sh::install_node() (symlink creation block, lines 847-855)
  • Blast radius: LOW — only affects fresh install path; existing installs unaffected; Hermes runtime already resolves bundled node via $HERMES_HOME/node/bin prepended to PATH
  • Related patterns: test_uninstall_node_symlinks.py (uninstall path), test_install_sh_symlink_stomp.py (symlink regression)

On macOS user installs, install_node() unconditionally symlinks
node/npm/npx into ~/.local/bin. When that directory precedes
/opt/homebrew/bin (Homebrew) or nvm in $PATH, the symlinks silently
replace the user's real Node toolchain with Hermes' bundled copy.

Guard each symlink with `command -v` so they are only created when no
system-installed version is found. Hermes itself always locates its
bundled node via $HERMES_HOME/node/bin (prepended to PATH at runtime),
so the symlinks are purely for user convenience.

Fixes NousResearch#45279
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists area/config Config system, migrations, profiles labels Jun 13, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the installer shadowing behavior. The current install_node() still creates the unconditional links, so the issue is real, but this implementation needs a layout-aware rework.

Problems

  • The command -v guard on the added loop would also suppress managed links for root-FHS installs. get_command_link_dir() selects /usr/local/bin for ROOT_FHS_LAYOUT (scripts/install.sh:448-455), while check_node() installs a managed runtime when the discovered Node is too old (scripts/install.sh:820-827). The guard still sees that old command and leaves the root recovery path without links.
  • It does not address the non-interactive nvm case described in #45279: when nvm is not on that process's PATH, command -v is false and the user-global shim is still created.
  • scripts/lib/node-bootstrap.sh:216-221 independently creates these links, and _ensure_tui_node() sources that helper (hermes_cli/main.py:1629-1644), so the sibling path remains affected.

Suggested changes

  • Scope the policy by install layout, preserving the intentional FHS-root behavior, and apply it to both installer paths.
  • Add behavioral shell fixtures for root-FHS, user installs, old PATH Node, and non-interactive nvm; the proposed tests only inspect text.

Automated hermes-sweeper review.

Comment thread scripts/install.sh
# where ~/.local/bin precedes /opt/homebrew/bin in PATH (#45279).
local _cmd
for _cmd in node npm npx; do
if ! command -v "$_cmd" >/dev/null 2>&1; then

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command -v is not the required policy boundary. In a root-FHS install, check_node() reaches install_node() precisely when the PATH Node is too old, but this condition still finds that old binary and prevents restoring the managed /usr/local/bin links. Scope this by the install layout so the root-FHS path remains intact.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026

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/config Config system, migrations, profiles area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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