Skip to content

fix(desktop): prevent Windows installs from using stale system Node - #82355

Open
fangliquanflq wants to merge 1 commit into
NousResearch:mainfrom
fangliquanflq:fix/desktop-managed-node-path
Open

fangliquanflq wants to merge 1 commit into
NousResearch:mainfrom
fangliquanflq:fix/desktop-managed-node-path

Conversation

@fangliquanflq

Copy link
Copy Markdown
Contributor

What does this PR do?

On Windows, Desktop dependency installation can run npm with Hermes-managed Node while npm lifecycle children resolve an older system Node from PATH. Electron postinstall then fails with ERR_REQUIRE_ESM, blocking Desktop builds and updates even though Hermes has already installed a compatible Node runtime.

This change makes the dependency-install environment move all Hermes-managed Node directories to the front of PATH. It removes normalized duplicate entries first, including Windows case and trailing-separator variants, while preserving the order of unrelated PATH entries.

Related Issue

Closes #82309

Type of Change

  • Bug fix
  • Tests

Changes Made

  • hermes_constants.py - normalize PATH entries with platform path semantics, remove managed-directory duplicates, and prepend canonical managed Node directories.
  • tests/test_hermes_constants.py - cover managed Node reordering and Windows duplicate normalization.
  • tests/hermes_cli/test_gui_command.py - cover Desktop install environments when managed Node is missing from PATH or appears after system Node.

How to Test

  1. On Windows, place a system Node directory first in PATH and existing Hermes-managed Node directories later.
  2. Build the Desktop dependency environment and confirm the managed directories lead PATH, duplicate variants are removed, and bare node resolves to the managed runtime.
  3. Run the focused regression tests:
scripts/run_tests.sh tests/test_hermes_constants.py -k 'managed_node_dirs_move_ahead or windows_managed_node_path_collapses'
scripts/run_tests.sh tests/hermes_cli/test_gui_command.py

Observed results: 2 focused PATH tests passed and all 13 Desktop GUI command tests passed. In the Windows real environment, bare node resolved to managed Node v22.23.1 after the change instead of the older system Node.

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
  • I've run the repository test entry on the relevant tests and all selected tests pass
  • I've added tests for this bug fix
  • I've tested on my platform: Windows 10/11

Documentation & Housekeeping

  • Documentation update is not required for this internal PATH correction
  • Config example update is not applicable because no config keys changed
  • Contributor guide update is not applicable because no architecture or workflow changed
  • I've considered cross-platform impact; POSIX comparison remains case-sensitive and uses POSIX path normalization
  • Tool description and schema updates are not applicable

Screenshots / Logs

Focused managed Node PATH tests: 2 passed
Desktop GUI command tests: 13 passed
Ruff: all checks passed
Windows real-environment verification: managed Node v22.23.1 selected before system Node

@monerostar

Copy link
Copy Markdown
Contributor

Native Win11 verification (monerostar)

Could not leave a formal Approve (external collaborator scope) — review-as-comment with live evidence.

Host: Microsoft Windows 11 Pro · OS Version 10.0.26200 Build 26200 · x64
Python: 3.11.15 (win32)
PR head: ba343709d (fix(desktop): prioritize managed Node in install PATH)
HERMES_HOME used for live probes: C:\Users\Admin\AppData\Local\hermes
Managed Node present: C:\Users\Admin\AppData\Local\hermes\node\node.exev22.22.3

Why this fix is right

with_hermes_node_path() on current main only does exact-string if entry not in parts: parts.insert(0, entry). That fails the real Desktop install cases:

  1. Managed already on PATH but after system Node — main leaves order unchanged, so bare node from npm lifecycle children can still hit stale system Node.
  2. Case / trailing-separator duplicates on WindowsC:\USERS\...\NODE\ is not equal to C:\Users\...\node, so main can prepend and keep the noisy duplicate.

This PR normalizes with ntpath.normcase/normpath on win32, strips managed keys from the remainder, then places canonical managed dirs first.

Live contrast (same inputs)

IN  (system first, managed only as UPPER+trailing sep):
  C:\Program Files\nodejs
  C:\USERS\ADMIN\APPDATA\LOCAL\HERMES\NODE\

MAIN out: [managed, system, UPPER-dup]   # still keeps duplicate
PR   out: [managed, system]              # collapsed

IN2 (system first, managed exact path already present later):
  C:\Program Files\nodejs
  C:\Users\Admin\AppData\Local\hermes\node

MAIN out2: [system, managed]   # DOES NOT REORDER — bug class for ERR_REQUIRE_ESM
PR   out2: [managed, system]   # fixed

Live PR call also returned managed-first with duplicates collapsed; find_node_executable("node") → managed node.exe v22.22.3.

Tests (native Win11, this checkout)

tests/test_hermes_constants.py -k "managed_node_dirs_move_ahead or windows_managed_node_path_collapses"
  2 passed, 50 deselected in 0.72s

tests/hermes_cli/test_gui_command.py
  13 passed in 1.68s
  including:
    test_gui_install_env_prepends_managed_node[missing]
    test_gui_install_env_prepends_managed_node[after-system-node]

Notes

  • Diff is small and scoped to PATH assembly + tests. No new env vars / tools.
  • POSIX path stays on os.path (case-sensitive) — good.
  • I did not run a full Desktop npm ci/electron postinstall on this host tonight; the PATH contract + unit coverage is what this PR claims, and that holds on native Win11.

LGTM from native Windows 11 for landing.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 9, 2026

andrexibiza commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Historical current-main validation receipt, superseded by the later topology decision below:

This comment no longer requests replacement or closure of #82355.

Copy link
Copy Markdown
Contributor

Current-main validation receipt: #91188 rebased this exact three-file managed-Node PATH surface onto current main, preserving @fangliquanflq as Git author. Exact rebased head 150b197363451161308c0621385c7ca490d84108 is green in CI 32433289963, Docker 32433289358, and Nix 32433289336.

GitHub currently reports this original PR mergeable. I am keeping #82355 as the canonical contribution/provenance owner and closing #91188 as a validation duplicate rather than replacing the original contributor's PR.

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

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] Desktop dependency install fails with ERR_REQUIRE_ESM when a system Node precedes the managed Node on PATH

4 participants