fix(runtime): managed Node/uv resolve first everywhere; require Node 26 - #76459
Conversation
pestoura
left a comment
pestoura
left a comment
There was a problem hiding this comment.
The persisted User PATH migration does not repair the existing affected state. If $HermesHome\node is already present later in the User PATH — which is exactly what previous installer versions created — -notcontains is false, so this block leaves it in place behind a system Node. Fresh installs are fixed, but upgrades keep resolving the wrong executable.
Please remove existing case-insensitive occurrences of $nodeDir, then prepend one canonical entry and persist only when the resulting order changes. A Windows regression should start with something like C:\Program Files\nodejs;...;$HermesHome\node, run the migration, and assert the managed directory becomes the first entry without duplication. Also preserve unrelated entries and empty segments according to the installer's existing PATH policy.
૮ >ﻌ< ა ci reviewran on 8de9c16 ℹ️ InfoCI-sensitive file review · View jobPR touches sensitive files, but the Sensitive files changed:
Desktop E2E visual evidence · View test artifacts · View job3 visual diffs. inline evidence upload failed. Failed to upload diff-1508682a2ae8-boot-ready-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-1508682a2ae8-boot-ready-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
alt-glitch
added
type/bug
ebdac74 to
5cb965c
Compare
Two paths let a pre-existing system Node win over the Hermes-managed one. The desktop backend spawn built its managed-Node PATH entry as `<home>/node/bin` only. That is the POSIX layout install.sh produces; install.ps1 unpacks portable Node straight into `%LOCALAPPDATA%\hermes\node` with node.exe at the root and no `bin\`. On Windows the entry therefore pointed at a directory that does not exist, and the backend fell through to whatever Node was already on PATH. main.ts already had the correct platform-ordered list, behind a "keep this in sync with iter_hermes_node_dirs()" comment on a second copy of the rule. The two copies had drifted. Export the ordering from backend-env.ts and have main.ts consume it so there is one source of truth on the Node side (the Electron main process cannot import hermes_constants.py, so a mirror is unavoidable — but one mirror, not two). install.ps1 appended the node dir to the persisted User PATH instead of prepending it. The session PATH was already prepended correctly, so this only bit later processes: any shell opened after install, and a standalone hermes-setup.exe run that inherits User PATH rather than a curated env, both resolved a system Node ahead of the bundled one. Not a bug, for the record: update.rs's prepend list omits the same Windows root, but it inherits PATH from the desktop, which supplies the correct entries — so it is redundant rather than broken, and no installer rebuild is needed for this fix. Tests: managed dirs lead with the platform-native layout while always offering both shapes, empty without a home, and every managed dir outranks the inherited PATH on darwin and win32. The three existing tests that pinned `entries[1]` by index asserted the old single-dir shape and now assert the relationship instead. install.ps1 has no behavioral test here: CI has no PowerShell host, and AGENTS.md bans source-reading tests (the neighbouring test_install_ps1_node_path_for_npm.py predates that rule).
5cb965c to
3bed7d4
Compare
5bea261 to
cc3d6f4
Compare
ethernet8023
changed the title
Hermes installs runtimes for itself — `uv` at `$HERMES_HOME/bin/uv`, Node
at `$HERMES_HOME/node` — and neither directory is on an arbitrary
process's PATH. Every `shutil.which("node"/"npm"/"npx"/"uv")` in Hermes's
own code therefore 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.
Routed the Hermes-owned call sites through managed-aware resolvers:
- `agent/lsp/install.py`, `hermes_cli/dep_ensure.py`, `hermes_cli/main.py`
(`_make_tui_argv`), `hermes_cli/tools_config.py` (`_run_post_setup`) now
use `find_node_executable()`.
- `hermes_cli/tools_config.py::_pip_install` and `hermes_cli/setup.py`'s
vercel install use `ensure_uv()` (installing uv is in scope during setup,
and the Windows installer's `uv venv` does not seed pip, so the fallback
tier is "No module named pip"). `tools/lazy_deps.py` uses `resolve_uv()`
— a lookup, not a bootstrap, because it runs mid-turn for an optional
dependency and downloading a runtime as a side effect exceeds what the
caller asked for.
- `hermes_cli/gateway.py`: extracted `_append_node_dir_for_service()`,
shared by the systemd unit and launchd plist generators, which appends
the managed dirs before the PATH-resolved one. A service definition is
written once and survives reboots, so resolving a system Node that
happens to lead the installing shell's PATH bakes the wrong interpreter
in permanently. Managed dirs are profile-scoped, so each profile's unit
still names its own Node; the existing symlink-parent rule (don't
`.resolve()`) is preserved verbatim.
- `tools/environments/local.py`: the terminal tool's subshell PATH gains
the managed dirs, appended alongside the sane entries rather than
prepended — a tool the user deliberately put on their own PATH still
wins, and the managed one only fills a gap. This is also what makes the
bare `which("uv")` in `tools/env_probe.py` correct: that probe reports
the environment the *model* sees, and the model can only run what is on
that subshell's PATH.
`scripts/install.ps1`: the persisted User PATH update becomes
`Set-ManagedNodeFirstOnUserPath`, a move-to-front rather than an
add-if-missing. Installs made by an older install.ps1 already have the
managed dir in User PATH — at the tail, behind a system Node — and an
add-if-missing check sees it present and leaves that ordering in place
forever, so the users the bug hurt would never be repaired. Unrelated
entries keep their relative order (empty segments included; a trailing
`;` is legal and the installer's other PATH code preserves them),
duplicates collapse, and it writes only when the string actually changes.
Tests:
- `tests/test_managed_runtime_resolution.py` — AST guard that fails any
new bare `which()` for a managed runtime, with a short justified
allow-list and a companion test that fails when an allow-list entry goes
stale. Reading source is banned by AGENTS.md and this is the documented
exception: the property is "no call site anywhere spells it this way",
which no runtime seam can observe.
- `scripts/ci/test_install_ps1_path_migration.ps1` — behavioral, not a
source regex: it lifts the real `Set-ManagedNodeFirstOnUserPath` out of
install.ps1's AST and rewrites only the two registry calls into an
in-memory store, so the shipped split/dedupe/prepend/change-detection
logic executes for real. Not in the default lane (Linux runners have no
PowerShell host); runs under `pwsh`. 13/13 assertions pass.
…ade paths Hermes now pins its toolchain to Node 26 everywhere. Every path that installs, accepts, heals, or upgrades a Node runtime moves from the old 22-default / `^20.19 || >=22.12` floor to a single rule: Node >=26. Installers: - scripts/install.sh — NODE_VERSION=26; node_satisfies_build() collapses the two-branch Vite floor to `major >= 26`; user-facing messages updated. - scripts/install.ps1 — $NodeVersion=26; Test-NodeVersionOk likewise; winget fallback switches OpenJS.NodeJS.LTS -> OpenJS.NodeJS (26 is Current, not LTS — the LTS manifest would reinstall a too-old Node). - Dockerfile — node_source stage node:22-bookworm-slim -> node:26 (digest pinned, amd64 sha256:9e6f...bf73). - nix/ was already on nodejs_26 (lib.nix, npm-12-0-2.nix); the checks.nix wrapper check ratchets from `>= 20` to `>= 26`. Heal/upgrade paths: - scripts/lib/node-bootstrap.sh — HERMES_NODE_TARGET_MAJOR default 22->26 and HERMES_NODE_MIN_VERSION default 20->26, so heal_managed_node, _nb_install_bundled_node, and the fnm/proto/nvm/brew rungs all target 26 and stop accepting an on-PATH Node below it. Both remain env-overridable. - hermes_constants.py — _HERMES_NODE_TARGET_MAJOR fallback 22->26, which drives the Windows heal path's latest-v26.x download. Version gates: - package.json engines.node >=20 -> >=26; apps/desktop engines `^20.19.0 || >=22.12.0` -> `>=26.0.0`. - CI setup-node: all five workflows 22 -> 26. - Docs describing Hermes's own toolchain updated (windows-native, docker, acp, nix-setup, contributing). Skill docs describing third-party tools' own requirements are untouched. Termux still installs via `pkg install nodejs` best-effort (nodejs.org ships no Android tarballs); that path was never version-gated. Verified: bash -n on both shell scripts, PowerShell AST parse of install.ps1, latest-v26.x index resolves (node-v26.5.1), and the install test suite — 18 tests across the 5 install/runtime test files — passes.
Existing users who only ever launch Hermes (never re-run an installer) kept their managed Node 22 tree forever: the heal path only fired for *broken* trees, and a healthy 22 passes the --version probe. Now "outdated" heals the same way "broken" does, on both sides of the mirror: - hermes_constants.py: find_hermes_node_executable() checks _managed_node_tree_outdated() (managed node major < _HERMES_NODE_TARGET_MAJOR) and routes through the existing once-per-process heal_hermes_managed_node(), which redownloads latest-v26.x. When the heal fails (offline, download error) the outdated-but-runnable tree is still returned — old Node beats no Node. - scripts/lib/node-bootstrap.sh: _nb_managed_node_needs_heal() gains the matching _nb_managed_node_outdated() rung, so heal_managed_node agrees with the Python side. This is the same shape as the managed-uv flow: resolve the managed runtime, notice it can't satisfy the requirement, provision the right one in place, fall back gracefully. Tests (tests/test_hermes_constants.py): outdated tree triggers heal and returns the upgraded binary; failed heal still serves the old tree; an at-target tree never heals (heal stub raises).
27f8b9b to
b13148d
Compare
…tem unit `_append_node_dir_for_service()` had two bugs, both caught by `test_system_unit_uses_target_user_home_not_calling_user`: 1. It crashed. `iter_hermes_node_dirs()` defaults to the *calling* user's Hermes home, so under sudo it stats `/root/.hermes/node/bin` — which raises `PermissionError` for a non-root caller rather than returning False. An unreadable candidate dir means "skip this rung", not "kill the generator", so the probe now swallows OSError. 2. Worse than the crash: had the stat succeeded, a `--system` unit targeting alice would have baked *root's* managed Node into alice's PATH. The generator now skips the managed-Node rung on the system path and re-runs it after `_hermes_home_for_target_user()` resolves, passing that home explicitly. Entries are prepended so the managed Node still outranks the remapped shell-PATH entries, matching the user-unit ordering. The launchd generator is unaffected — it has no target-user remapping, so the default home is already correct there.
Two breaks from moving node_source to node:26, both proven against the real image rather than inferred: 1. `COPY .../node_modules/corepack` failed with "not found". Node unbundled corepack upstream, so node:26 ships only `npm` in /usr/local/lib/node_modules (verified: `ls` in the pinned image lists `npm` alone). Nothing in this repo needs it — no package.json declares a `packageManager` and no build step shells out to yarn or pnpm — so the COPY and its symlink are removed rather than replaced. 2. Hidden behind that failure: node 26's binary links against `libatomic.so.1`, which node 22's did not, and bare debian:13.4 doesn't ship it. Without it every `node` invocation in the image dies with "error while loading shared libraries: libatomic.so.1". Added `libatomic1` to the existing apt layer, which runs well before the node COPY so layer ordering and caching are unchanged. Verified with a minimal probe image (debian:13.4 + the same two COPY lines): node v26.5.1, npm 11.17.0, npx 11.17.0, uv 0.11.6 all execute.
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
The managed-Node ordering work is right, and b13148d3 (heal outdated trees up to the target major) is the piece that actually rescues the installs broken since f88ed6c71. I reproduced both sides on real fixtures:
On current main — temp HERMES_HOME seeded with a managed Node v22.21.1 tree (npm 10.9.4), root manifest, real find_node_executable() + maybe_repair_npm_engine():
resolved node: v22.21.1 / npm 10.9.4
attempt1 rc: 1 (EBADENGINE, npm >=12.0.0)
→ Upgrading Hermes-managed npm to satisfy >=12.0.0…
✗ npm upgrade failed
notsup Required: {"node":"^22.22.2 || ^24.15.0 || >=26.0.0"}
repair -> False
Dead end: #76464's in-place upgrade can't land npm 12 on a Node below 22.22.2, and the healthy-tree probe never re-provisions. Every managed tree older than 22.22.2 is stuck.
On this branch, same fixture:
resolved node: v26.5.1 / npm 11.17.0 (outdated tree healed, 5s)
attempt1 rc: 1 → ✓ npm upgraded to 12.0.2 → attempt2 rc: 0
Two blockers before this can go in.
1. The Dockerfile bump breaks the image build (both arches, currently red)
Node stopped distributing corepack in v25, so node:26-bookworm-slim has no /usr/local/lib/node_modules/corepack, and Dockerfile:161 fails hard:
ERROR: failed to compute cache key: failed to calculate checksum of ref …:
"/usr/local/lib/node_modules/corepack": not found
(amd64 job, same on arm64.) Drop the corepack COPY and its ln -sf at Dockerfile:161,164, or install it explicitly — nothing in the tree invokes corepack. The comment at Dockerfile:154 still says "Node 22 LTS" too.
2. A fresh POSIX install still fails EBADENGINE
engines.npm stays at >=12.0.0 while Node 26 bundles npm 11.17.0, and install.sh's workspace step is a bare npm ci with no engine recovery — scripts/{install.sh,install.ps1,lib/node-bootstrap.sh} never upgrade npm. Provisioned a pristine tree through this branch's own _nb_install_bundled_node:
✓ Node v26.5.1 installed to /tmp/pristine26/node/ (npm 11.17.0)
$ npm ci # what install.sh runs at $INSTALL_DIR
npm error code EBADENGINE
npm error notsup Required: {"node":">=26.0.0","npm":">=12.0.0"}
npm error notsup Actual: {"node":"v26.5.1","npm":"11.17.0"}
The Python update path recovers via npm_engine; the installer path has no such rung, so a new install dies at "Desktop workspace npm install failed."
Cleanest fix is to take the npm floor from #76499 — >=11.17.0 is the first release with min-release-age-exclude (verified against the published tarballs: 11.16.0 ships 0 files mentioning it, 11.17.0 ships 26), so it keeps the 11.10–11.16 band excluded for the reason 3975e9d75 added it while matching what Node 26 actually bundles. node >=26.0.0 + npm >=11.17.0 installs clean with no recovery step at all:
added 208 packages in 658ms
Those two branches touch the same package.json lines, so whichever lands second needs the other's value.
Verified good
tests/{test_managed_runtime_resolution,test_hermes_constants}.py+tests/hermes_cli/test_npm_engine.py— 77 passedapps/desktopelectron/backend-env.test.ts— 10 passed- @pestoura's User PATH note is addressed:
Set-ManagedNodeFirstOnUserPathis now a move-to-front with a case-insensitive-neand collapses duplicates, andscripts/ci/test_install_ps1_path_migration.ps1covers theC:\Program Files\nodejs;…;$HermesHome\nodeupgrade case.
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
Docker's fixed — both arches green, and the libatomic1 catch was a good one; that would have been the next failure behind the corepack COPY. The gateway system-unit fix is right too: probing the calling user's home under sudo was the worse half of that bug.
Two things left, one new.
The Node 26 bump breaks 21 desktop UI tests (check:test:ui, red)
src/store/session.test.ts (12) and src/app/session/hooks/use-model-controls.test.tsx (9), all TypeError: Cannot read properties of undefined (reading 'setItem'). Not flakes and not pre-existing — the same commit passes on Node 22 and fails on Node 26, on the same checkout:
$ node --version && npx vitest run --project ui src/store/session.test.ts
v26.5.1 → Tests 12 failed | 34 passed (46)
v22.22.3 → Tests 46 passed (46)
Node 26 ships a native localStorage global that shadows jsdom's, and it's inert unless the runtime was started with --localstorage-file. Minimal probe under the ui project:
(node:62417) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
× jsdom exposes localStorage
AssertionError: expected undefined to be defined
So window.localStorage is undefined in jsdom on 26 where it was a working Storage on 22. This is a real compatibility break the bump surfaces, not a test-harness nit — worth deciding whether the fix is --localstorage-file in the vitest node options, a setup-file shim, or bumping jsdom (29.1.1 here).
engines.npm: ">=12.0.0" still fails a fresh install
we vendor node 26 and install npm 12
The vendoring works — the tree is Node 26. But nothing installs npm 12 into it. _nb_install_bundled_node unpacks the nodejs.org tarball and stops, and neither install.sh nor install.ps1 has an npm install -g npm@…. Node 26.5.1 bundles npm 11.17.0, so the tarball you vendor is one minor below your own floor. Provisioned a tree with this head's own bootstrap, then ran what install.sh runs:
✓ Node v26.5.1 installed to /tmp/pristine26b/node/
node: v26.5.1 npm: 11.17.0
$ npm ci # install.sh: cd $INSTALL_DIR && npm ci
npm error code EBADENGINE
npm error notsup Required: {"node":">=26.0.0","npm":">=12.0.0"}
npm error notsup Actual: {"node":"v26.5.1","npm":"11.17.0"}
The Python update path recovers through npm_engine; the installer path has no such rung, so a brand-new install dies at "Desktop workspace npm install failed."
Two ways out, either is fine:
- Take #76499's floor —
npm >=11.17.0, the first release carryingmin-release-age-exclude(verified against published tarballs: 11.16.0 has 0 files mentioning it, 11.17.0 has 26), so the 11.10–11.16 band3975e9d75guarded against stays excluded.node >=26.0.0+npm >=11.17.0installs clean with zero recovery:added 208 packages in 658ms. - Actually install npm 12 in the bootstrap — add the
npm install -g npm@">=12.0.0"step to_nb_install_bundled_node(and the ps1 equivalent) so the vendored tree matches the manifest. Then #76499 is genuinely unnecessary.
Right now it's neither, which is why the fresh-install path is red. Everything else I flagged is resolved.
Node 26 defines its own `localStorage` accessor on the global object, which returns `undefined` unless the process was started with `--localstorage-file` (hence the "localStorage is not available because --localstorage-file was not provided" warning now printed by every worker). In the jsdom environment `globalThis` IS the window, so that accessor shadows jsdom's Storage and every `localStorage.getItem(...)` in a test throws "Cannot read properties of undefined". CI caught this on the Node 26 bump: `check:test:ui` failed with 22 errors across session.test.ts, terminals.test.ts, model-settings and onboarding stores — all storage-backed. Reproduced locally against nodejs_26 (12 failures in src/store/session.test.ts alone) before fixing. vitest.setup.ts now installs a real in-memory Storage on both globalThis and window when the global resolves to undefined, before any test module reads it. Guarded on `typeof === 'undefined'` so Node < 26 and any future runtime that provides a working Storage keep jsdom's own implementation. Verified under nodejs_26: the full `--project ui` lane is 378 files / 3268 tests green (was 22 failures).
The bundled-Node bootstrap unpacked the nodejs.org tarball and stopped.
Node 26.5.1 bundles npm 11.17.0, one minor below the root package.json's
own `engines.npm` floor of >=12 — and .npmrc sets `engine-strict=true`,
so that is fatal rather than a warning:
npm error code EBADENGINE
npm error notsup Required: {"node":">=26.0.0","npm":">=12.0.0"}
npm error notsup Actual: {"node":"v26.5.1","npm":"11.17.0"}
A brand-new install died at the first `npm ci` with "Desktop workspace
npm install failed". CI never saw it because the workflows run an
explicit `npm i -g npm@12`; the Python update path recovers through
hermes_cli/npm_engine.py, but the installer path had no such rung.
_nb_ensure_bundled_npm_range() now upgrades the managed tree's npm into
range right after the tarball lands, mirroring upgrade_managed_npm():
- temp cwd, so the checkout's own .npmrc (engine-strict,
min-release-age) does not gate the upgrade meant to satisfy it;
- npm_config_min_release_age=0, which also neutralises a user ~/.npmrc;
- explicit --prefix at the managed tree, because
_nb_configure_npm_prefix writes prefix=~/.local into its etc/npmrc
and a bare `npm i -g` would install a second npm elsewhere while the
managed tree stayed stale.
The range is read out of package.json rather than duplicated, so the two
cannot drift, with HERMES_NPM_TARGET_RANGE as an override and a >=12.0.0
fallback for a stripped install tree. An already-in-range npm skips the
network round-trip. Best-effort: a failed upgrade warns with the manual
command and keeps the working Node, since npm_engine.py still covers the
EBADENGINE that follows.
Verified against a real tree provisioned by this bootstrap: node v26.5.1
/ npm 12.0.2, bin/npm and bin/npx still relative-symlinked into the
upgraded lib/node_modules/npm, the ~/.local/bin links resolving to 12.0.2
through the tree, and no stray second npm under ~/.local/lib.
Follow-up to 6fdc64e, which fixed only the POSIX bootstrap. install.ps1 unpacks the same nodejs.org build, so Windows had the same EBADENGINE: Node 26.5.1 bundles npm 11.17.0, one minor below the root package.json's `engines.npm` floor of >=12, and .npmrc's engine-strict=true makes that fatal at the first `npm ci`. Update-ManagedNpm mirrors _nb_ensure_bundled_npm_range rung for rung — temp cwd so the checkout's .npmrc cannot gate the upgrade meant to satisfy it, npm_config_min_release_age=0, and an explicit --prefix at the managed tree. EAP is relaxed around the npm call for the same reason Install-Uv does it: npm's stderr would otherwise wrap as ErrorRecords and short-circuit before $LASTEXITCODE is read. Env vars and location are restored in a finally. Called from both branches that yield a managed tree: the fresh portable unpack, and the reuse-an-existing-tree path, where an older install still has its original major's npm sitting there. The in-range check makes the second a one-probe no-op on reruns. The range comes from Get-NpmRange, which prefers the checkout's package.json but falls back to a $NpmRange constant — unlike the POSIX side, Test-Node runs before the repo is cloned, so there is usually no manifest on disk yet (and none at all when install.ps1 is piped from the web). The manifest read means a drifted constant self-corrects on any run against an existing checkout. Not executed locally: no pwsh on this machine, and the repo runs no PowerShell in CI.
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
Both blockers are fixed, and the npm-12 route is the better of the two options I offered — the vendored tree now matches the manifest instead of the manifest bending to the tarball. Verified each on real fixtures rather than trusting the commit messages.
Fresh install — pristine tree provisioned by this head's own _nb_install_bundled_node, then the literal cd $INSTALL_DIR && npm ci:
✓ Node v26.5.1 installed to /tmp/pristine26c/node/
→ Upgrading bundled npm to satisfy >=12.0.0...
✓ npm 12.0.2 installed
node: v26.5.1 npm: 12.0.2
$ npm ci
added 208 packages in 545ms
Reading engines.npm out of the manifest with sed instead of duplicating the range is the right call — no drift, and it works before a usable node exists.
jsdom under Node 26 — the two files that were red now pass, and the whole lane is green:
src/store/session.test.ts + use-model-controls.test.tsx → 65 passed
--project ui (full) → 378 files / 3268 tests passed
Guarding on typeof === 'undefined' rather than sniffing the Node version is the durable shape; jsdom keeps its own Storage the moment a runtime provides a working one.
The Docker fix holds up too — both arches green, and libatomic1 was the failure hiding behind the corepack COPY.
One gap left: the POSIX reuse path never upgrades npm
_nb_ensure_bundled_npm_range is called from exactly one place — the tail of _nb_install_bundled_node, i.e. only when a tarball was just unpacked. ensure_node's reuse rung returns before it:
if [ -x "$HERMES_HOME/node/bin/node" ]; then
export PATH="$HERMES_HOME/node/bin:$PATH"
if _nb_have_modern_node; then
_nb_ok "Node $(node --version) found (Hermes-managed)"
HERMES_NODE_AVAILABLE=true
return 0 # ← no npm-range check
fi
fiinstall.ps1 gets this right — Update-ManagedNpm is called from both branches, including the reuse path at line 1231, with the comment "A tree from an older install still has that Node major's bundled npm." The POSIX side is missing that second call site.
It bites whenever an at-target Node 26 tree exists with an out-of-range npm — the upgrade is best-effort || true, so one offline install is enough to strand it permanently. Seeded exactly that and re-ran the installer:
seeded: node v26.5.1, npm 11.17.0
$ ensure_node
✓ Node v26.5.1 found (Hermes-managed)
after ensure_node: npm 11.17.0 ← unchanged
$ npm ci
npm error notsup Required: {"node":">=26.0.0","npm":">=12.0.0"}
npm error notsup Actual: {"node":"v26.5.1","npm":"11.17.0"}
Heal doesn't cover it either — the tree is at the target major and every binary passes --version, so _nb_managed_node_needs_heal correctly says "fine." Re-running the installer, the documented recovery, never repairs it.
Fix is one line: call _nb_ensure_bundled_npm_range || true in the reuse branch too, matching what Update-ManagedNpm already does on Windows. The in-range check makes it a single --version probe on every normal rerun.
With that, and #76499 no longer needed given the tree carries npm 12, this is good to go from me.
_nb_ensure_bundled_npm_range ran only at the tail of _nb_install_bundled_node, so it fired just after a tarball was unpacked. ensure_node's reuse rung returns before reaching it, leaving an existing managed tree on whatever npm its Node major bundled. That strands a real install: the upgrade is best-effort (`|| true`), so one offline run leaves an at-target Node 26 tree carrying npm 11.17.0 — below the root package.json's `engines.npm` floor of >=12, fatal under .npmrc's engine-strict. Heal does not cover it either; the tree is at the target major and every binary passes --version, so _nb_managed_node_needs_heal correctly reports it healthy. Re-running the installer, the documented recovery, never repaired it. install.ps1 already had this right: Update-ManagedNpm is called from both branches that yield a managed tree, including the reuse path. This is the POSIX side of that same call site. Reproduced on a seeded node-26.5.1/npm-11.17.0 tree: before, ensure_node left npm at 11.17.0 and `npm ci` died with EBADENGINE; after, it upgrades to 12.0.2 and `npm ci` installs 208 packages. An already-in-range tree costs one --version probe (~0.13s), and the system-node path is unchanged. Co-authored-by: ethernet8023 <arilotter@gmail.com>
|
Pushed the one-liner myself rather than bouncing it back —
An already-in-range tree costs one That was the last thing on my list — everything else checked out on real fixtures: fresh install lands npm 12.0.2 and |
#76499 landed the npm floor as >=11.17.0 on a Node >=20 baseline. This branch takes the other half of the same constraint: the vendored Node 26 tree now installs npm 12 into itself, so the toolchain satisfies the stricter floor rather than the manifest relaxing to meet the tarball. Resolved package.json + package-lock.json to node >=26.0.0 / npm >=12.0.0 and refreshed npm_engine.py's illustrative range to match. website/'s mirror keeps #76499's >=11.17.0 — it is not a root workspace and builds on its own Node.
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
Everything I raised across the three rounds is closed, each verified on real fixtures rather than commit messages.
Also resolved the collision with #76499, which merged while this was in review — 8de9c16b. It landed the floor as npm >=11.17.0 on a Node >=20 baseline; this branch takes the other half of the same constraint, so package.json + package-lock.json resolve to node >=26.0.0 / npm >=12.0.0 and the vendored tree rises to meet it. website/'s mirror keeps #76499's >=11.17.0 — not a root workspace, builds on its own Node. Re-verified after the merge:
| Check | Result |
|---|---|
Fresh install → npm ci |
✓ npm 12.0.2 installed, added 208 packages |
| Reuse path with stale npm | ✓ upgrades to 12.0.2 (was EBADENGINE) |
| In-range rerun | ✓ one --version probe, ~0.13s |
--project ui under Node 26 |
✓ 378 files / 3268 tests |
| managed-runtime + constants + npm_engine | ✓ 77 passed |
| Docker | ✓ both arches |
Nice work on the libatomic1 catch and on choosing to vendor npm 12 rather than relax the floor — the toolchain matching the manifest is the version of this that stays true.
…-node-path-windows-layout fix(runtime): managed Node/uv resolve first everywhere; require Node 26
…-node-path-windows-layout fix(runtime): managed Node/uv resolve first everywhere; require Node 26
4 participants
What does this PR do?
Makes the Hermes-managed Node/uv toolchain win everywhere, and pins that toolchain to Node 26 across every installer, heal, and upgrade path.
Three stacked changes:
1. Desktop backend spawn missed the Windows portable layout (
3bed7d4ae)backend-env.tsbuilt its managed-Node PATH entry as<home>/node/binonly — the POSIX layout.install.ps1unpacks portable Node straight into%LOCALAPPDATA%\hermes\node(node.exeat the root, nobin\), so on Windows the entry pointed at a directory that doesn't exist andbuildDesktopBackendEnv— the main backend launch path — fell through to whatever Node was already on PATH.main.tshad a second, drifted copy of the ordering rule; nowbackend-env.tsexports it andmain.tsconsumes it (one mirror ofiter_hermes_node_dirs(), not two).install.ps1's persisted User PATH write also appended instead of prepending, so later shells and a standalonehermes-setup.exerun resolved the system Node; now it prepends.2. Managed runtimes resolve before bare PATH, everywhere (
91b7d9ac3)Hermes installs runtimes for itself —
uvat$HERMES_HOME/bin/uv, Node at$HERMES_HOME/node— and neither dir is on an arbitrary process's PATH. Every Hermes-ownedshutil.which("node"/"npm"/"npx"/"uv")either couldn't see the managed runtime ("not installed" on a machine that has exactly what it needs) or let an unowned system copy win. All call sites now route throughfind_node_executable()/resolve_uv()/ensure_uv():agent/lsp/install.py,hermes_cli/dep_ensure.py,hermes_cli/main.py(TUI argv),hermes_cli/tools_config.py(post-setup hooks) →find_node_executable()hermes_cli/tools_config.py::_pip_install+hermes_cli/setup.pyvercel install →ensure_uv();tools/lazy_deps.py→resolve_uv()(lookup only — mid-turn dep installs shouldn't download a runtime as a side effect)hermes_cli/gateway.py: new_append_node_dir_for_service()shared by the systemd unit and launchd plist generators — service definitions survive reboots, so resolving a system Node at generation time baked the wrong interpreter in permanentlytools/environments/local.py: the terminal subshell PATH gains the managed dirs, appended — a tool the user deliberately put on their own PATH still winsscripts/install.ps1:Set-ManagedNodeFirstOnUserPathis a move-to-front, not add-if-missing — installs made by an older appending install.ps1 have the managed dir stranded at the tail, and add-if-missing would never repair them3. Node 26 everywhere (
cc3d6f4b9,91dd92996,27f8b9bf3)Single rule replacing the 22-default /
^20.19 || >=22.12floor: Node >=26.install.shNODE_VERSION=26,install.ps1$NodeVersion=26; both version gates collapse tomajor >= 26node-bootstrap.shHERMES_NODE_TARGET_MAJOR22→26 andHERMES_NODE_MIN_VERSION20→26 (heal + fnm/proto/nvm/brew rungs; still env-overridable);hermes_constants.pytarget major 22→26 (Windows heal download)OpenJS.NodeJS.LTS→OpenJS.NodeJS(26 is Current, not LTS — the LTS manifest would reinstall a too-old Node)node:22-bookworm-slim→ digest-pinnednode:26-bookworm-slim; nix was alreadynodejs_26, its checks.nix ratchet moves>=20→>=26package.jsonengines>=20→>=26(+lockfile sync), desktop engines^20.19.0 || >=22.12.0→>=26.0.0, all five CI workflowssetup-node22 → 26pkg install nodejs(no Android tarballs upstream, was never gated).nvmrcwith26so nvm/fnm users land on the right major in the checkoutExisting users upgrade on next launch, not next reinstall (
27f8b9bf3): the heal path now treats an outdated managed tree (node major < target) like a broken one, on both sides of the mirror —find_hermes_node_executable()checks_managed_node_tree_outdated()and routes through the existing once-per-process heal (redownloads latest-v26.x), andnode-bootstrap.sh's_nb_managed_node_needs_heal()gains the matching_nb_managed_node_outdated()rung. When the heal fails (offline), the outdated-but-runnable tree is still served — old Node beats no Node. Same shape as the managed-uv flow: resolve, notice it can't satisfy, provision the right one in place, degrade gracefully.Related Issue
Fixes the Windows wrong-Node class described in the PR (desktop backend spawning with system Node after #76464 provisions a managed tree).
Type of Change
Changes Made
apps/desktop/electron/backend-env.ts+ test,apps/desktop/electron/main.tsscripts/install.sh,scripts/install.ps1,scripts/lib/node-bootstrap.sh,Dockerfilehermes_constants.py,hermes_cli/{gateway,main,setup,tools_config,dep_ensure}.py,agent/lsp/install.py,tools/{lazy_deps,env_probe}.py,tools/environments/local.pynix/checks.nix,.github/workflows/{js-tests,js-autofix,e2e-desktop,docs-site-checks,deploy-site}.ymlpackage.json(+lock),apps/desktop/package.jsontests/test_managed_runtime_resolution.py(new AST ratchet: barewhich()of a managed runtime fails, with a justified allow-list + stale-entry check)scripts/ci/test_install_ps1_path_migration.ps1(new: executes the realSet-ManagedNodeFirstOnUserPathlifted from install.ps1's AST with registry calls swapped for an in-memory store)How to Test
pwsh -NoProfile -File scripts/ci/test_install_ps1_path_migration.ps1— 13/13 assertionsnix develop -c scripts/run_tests.sh tests/test_hermes_constants.py tests/test_managed_runtime_resolution.py tests/test_install_ps1_node_path_for_npm.py tests/test_install_sh_node_global_prefix.py tests/test_install_ps1_uv_powershell_host.py tests/test_install_sh_root_fhs_uv_python_path.py— green, incl. 3 new heal-on-outdated tests (outdated→heals, offline→keeps old tree, at-target→never heals)cd apps/desktop && npx vitest run electron/backend-env.test.ts— 10 passed (from commit 1)bash scripts/install.shreplaces it with managed latest-v26.x;node-v26.5.1currently resolves fromhttps://nodejs.org/dist/latest-v26.x/Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — install/runtime-focused files green (18 tests); full-suite run pending, CI will confirmDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A