From 3bed7d4ae7bc9139dd9e26cd54fc70e8bc699226 Mon Sep 17 00:00:00 2001 From: ethernet Date: Sat, 1 Aug 2026 19:03:46 -0400 Subject: [PATCH] fix(desktop,install): keep bundled Node ahead of system Node on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/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). --- apps/desktop/electron/backend-env.test.ts | 71 +++++++++++++++++++++-- apps/desktop/electron/backend-env.ts | 33 ++++++++++- apps/desktop/electron/main.ts | 17 ++---- scripts/install.ps1 | 8 ++- 4 files changed, 109 insertions(+), 20 deletions(-) diff --git a/apps/desktop/electron/backend-env.test.ts b/apps/desktop/electron/backend-env.test.ts index a92ce6e062..6fb598cdfe 100644 --- a/apps/desktop/electron/backend-env.test.ts +++ b/apps/desktop/electron/backend-env.test.ts @@ -7,6 +7,7 @@ import { appendUniquePathEntries, buildDesktopBackendEnv, buildDesktopBackendPath, + hermesManagedNodePathEntries, normalizeHermesHomeRoot, pathEnvKey, POSIX_SANE_PATH_ENTRIES @@ -22,8 +23,12 @@ test('desktop backend PATH adds Hermes-managed bins and missing POSIX sane entri }) const entries = result.split(':') - assert.equal(entries[0], '/Users/test/.hermes/node/bin') - assert.equal(entries[1], '/Users/test/.hermes/hermes-agent/venv/bin') + // Both managed-Node layouts lead, POSIX-native shape first, then the venv. + assert.deepEqual(entries.slice(0, 3), [ + '/Users/test/.hermes/node/bin', + '/Users/test/.hermes/node', + '/Users/test/.hermes/hermes-agent/venv/bin' + ]) assert.ok(entries.includes('/opt/homebrew/bin'), 'Apple Silicon Homebrew bin is added') assert.ok(entries.includes('/opt/homebrew/sbin'), 'Apple Silicon Homebrew sbin is added') assert.ok(entries.includes('/usr/local/sbin'), 'missing standard sbin is added') @@ -33,6 +38,56 @@ test('desktop backend PATH adds Hermes-managed bins and missing POSIX sane entri } }) +test('managed Node dirs lead with the platform-native layout but always offer both', () => { + const posix = hermesManagedNodePathEntries('/Users/test/.hermes', { + platform: 'darwin', + pathModule: path.posix + }) + + const windows = hermesManagedNodePathEntries('C:\\Users\\test\\AppData\\Local\\hermes', { + platform: 'win32', + pathModule: path.win32 + }) + + // install.sh uses node/bin; install.ps1 unpacks node.exe into node\ itself. + // Both shapes are always emitted so migrated installs keep resolving. + assert.deepEqual(posix, ['/Users/test/.hermes/node/bin', '/Users/test/.hermes/node']) + assert.deepEqual(windows, [ + 'C:\\Users\\test\\AppData\\Local\\hermes\\node', + 'C:\\Users\\test\\AppData\\Local\\hermes\\node\\bin' + ]) +}) + +test('managed Node dirs are empty without a Hermes home', () => { + assert.deepEqual(hermesManagedNodePathEntries(undefined, { platform: 'darwin', pathModule: path.posix }), []) + assert.deepEqual(hermesManagedNodePathEntries('', { platform: 'win32', pathModule: path.win32 }), []) +}) + +test('every managed Node dir outranks the inherited PATH on both platforms', () => { + for (const [platform, pathModule, home, inherited, delimiter] of [ + ['darwin', path.posix, '/Users/test/.hermes', '/usr/local/bin:/usr/bin', ':'], + ['win32', path.win32, 'C:\\hermes', 'C:\\Program Files\\nodejs;C:\\Windows\\System32', ';'] + ] as const) { + const entries = buildDesktopBackendPath({ + hermesHome: home, + venvRoot: null, + currentPath: inherited, + platform, + pathModule + }).split(delimiter) + + const managed = hermesManagedNodePathEntries(home, { platform, pathModule }) + const firstInherited = Math.min(...inherited.split(delimiter).map(entry => entries.indexOf(entry))) + + for (const dir of managed) { + assert.ok( + entries.indexOf(dir) >= 0 && entries.indexOf(dir) < firstInherited, + `${dir} must precede the inherited PATH on ${platform}` + ) + } + } +}) + test('desktop backend PATH preserves first occurrence and avoids duplicates', () => { const result = buildDesktopBackendPath({ hermesHome: '/Users/test/.hermes', @@ -64,7 +119,9 @@ test('buildDesktopBackendEnv extends PYTHONPATH and backend PATH together', () = }) assert.equal(env.PYTHONPATH, '/repo/hermes-agent:/existing/pythonpath') - assert.ok(env.PATH.startsWith('/Users/test/.hermes/node/bin:/Users/test/.hermes/hermes-agent/venv/bin:')) + assert.ok( + env.PATH.startsWith('/Users/test/.hermes/node/bin:/Users/test/.hermes/node:/Users/test/.hermes/hermes-agent/venv/bin:') + ) assert.ok(env.PATH.includes('/opt/homebrew/bin')) }) @@ -115,7 +172,13 @@ test('Windows PATH casing and delimiter are preserved without POSIX sane entries assert.equal(pathEnvKey({ Path: 'x' }, 'win32'), 'Path') assert.equal(env.PATH, undefined) - assert.ok(env.Path.startsWith('C:\\Users\\test\\AppData\\Local\\hermes\\node\\bin;')) + // Windows leads with the portable layout (install.ps1 unpacks node.exe + // straight into node\, no bin\), then the POSIX shape for migrated installs. + assert.ok( + env.Path.startsWith( + 'C:\\Users\\test\\AppData\\Local\\hermes\\node;C:\\Users\\test\\AppData\\Local\\hermes\\node\\bin;' + ) + ) assert.ok(env.Path.includes('\\venv\\Scripts;')) assert.ok(env.Path.includes(';C:\\Windows\\System32;C:\\Windows')) assert.equal(env.Path.includes('/opt/homebrew/bin'), false) diff --git a/apps/desktop/electron/backend-env.ts b/apps/desktop/electron/backend-env.ts index 3db4a19d03..24d6928bad 100644 --- a/apps/desktop/electron/backend-env.ts +++ b/apps/desktop/electron/backend-env.ts @@ -60,6 +60,34 @@ function appendUniquePathEntries(entries, { delimiter = path.delimiter } = {}) { return ordered.join(delimiter) } +/** + * Hermes-managed Node.js directories, in preferred lookup order. + * + * There are two on-disk layouts. `scripts/install.ps1` unpacks portable Node + * straight into `%LOCALAPPDATA%\hermes\node` (node.exe at the root, no `bin\`); + * `scripts/install.sh` and the node-bootstrap helper use the POSIX + * `$HERMES_HOME/node/bin`. Emit BOTH on every platform so mixed and migrated + * installs resolve, leading with the layout native to the current platform. + * + * This is the single source of truth for the ordering rule on the Node side — + * `main.ts` imports it rather than keeping its own copy. Mirrors + * `iter_hermes_node_dirs()` in hermes_constants.py, which the Electron main + * process cannot import. + */ +function hermesManagedNodePathEntries( + hermesHome, + { platform = process.platform, pathModule = pathModuleForPlatform(platform) }: any = {} +) { + if (!hermesHome) { + return [] + } + + const root = pathModule.join(hermesHome, 'node') + const bin = pathModule.join(root, 'bin') + + return platform === 'win32' ? [root, bin] : [bin, root] +} + function buildDesktopBackendPath({ hermesHome, venvRoot, @@ -68,11 +96,11 @@ function buildDesktopBackendPath({ pathModule = pathModuleForPlatform(platform) }: any = {}) { const delimiter = delimiterForPlatform(platform) - const hermesNodeBin = hermesHome ? pathModule.join(hermesHome, 'node', 'bin') : null + const hermesNodeDirs = hermesManagedNodePathEntries(hermesHome, { platform, pathModule }) const venvBin = venvRoot ? pathModule.join(venvRoot, platform === 'win32' ? 'Scripts' : 'bin') : null const saneEntries = platform === 'win32' ? [] : POSIX_SANE_PATH_ENTRIES - return appendUniquePathEntries([hermesNodeBin, venvBin, currentPath, saneEntries], { delimiter }) + return appendUniquePathEntries([hermesNodeDirs, venvBin, currentPath, saneEntries], { delimiter }) } function normalizeHermesHomeRoot(hermesHome, { pathModule = pathModuleForPlatform(process.platform) }: any = {}) { @@ -126,6 +154,7 @@ export { buildDesktopBackendEnv, buildDesktopBackendPath, delimiterForPlatform, + hermesManagedNodePathEntries, normalizeHermesHomeRoot, pathEnvKey, POSIX_SANE_PATH_ENTRIES diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 785a1dc96e..72e6960c96 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -35,7 +35,7 @@ import { classifyActiveRuntime } from './active-runtime-state' import { stopBackendChild as stopBackendChildImpl } from './backend-child' import { dashboardFallbackArgs, sourceDeclaresServe } from './backend-command' import { createBackendConnectionState } from './backend-connection-state' -import { buildDesktopBackendEnv, normalizeHermesHomeRoot } from './backend-env' +import { buildDesktopBackendEnv, hermesManagedNodePathEntries, normalizeHermesHomeRoot } from './backend-env' import { isReauthRequiredError, waitForHermesReady } from './backend-health' import { canImportHermesCli, @@ -574,19 +574,10 @@ function resolveHermesHome() { const HERMES_HOME = resolveHermesHome() -function hermesManagedNodePathEntries() { - // NOTE: keep this ordering in sync with iter_hermes_node_dirs() in - // hermes_constants.py — this Node main process cannot import the Python - // module, so the platform-ordering rule is mirrored here. - const root = path.join(HERMES_HOME, 'node') - const bin = path.join(root, 'bin') - const entries = IS_WINDOWS ? [root, bin] : [bin, root] - - return entries.filter(directoryExists) -} - function pathWithHermesManagedNode(...entries) { - return [...hermesManagedNodePathEntries(), ...entries, process.env.PATH].filter(Boolean).join(path.delimiter) + const managed = hermesManagedNodePathEntries(HERMES_HOME).filter(directoryExists) + + return [...managed, ...entries, process.env.PATH].filter(Boolean).join(path.delimiter) } // ACTIVE_HERMES_ROOT — the canonical mutable Hermes install. Same path diff --git a/scripts/install.ps1 b/scripts/install.ps1 index b39464f42c..184ae2e348 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -1133,11 +1133,17 @@ function Test-Node { # Persist to User PATH so fresh shells (and future stages # in cross-process driver mode) see it. Matches the # pattern Install-Git uses for PortableGit. + # + # PREPEND, don't append. Appending leaves a pre-existing + # system Node ahead of the bundled one in every new shell, + # so anything launched without a curated environment (a + # standalone hermes-setup.exe run, a user typing `npm`) + # silently resolves the wrong Node. Bundled must win. $nodeDir = "$HermesHome\node" $userPath = [Environment]::GetEnvironmentVariable("Path", "User") $userPathItems = if ($userPath) { $userPath -split ";" } else { @() } if ($userPathItems -notcontains $nodeDir) { - $userPathItems += $nodeDir + $userPathItems = @($nodeDir) + $userPathItems [Environment]::SetEnvironmentVariable("Path", ($userPathItems -join ";"), "User") }