From 3e276d78baa118fcc769cd5202d5285e34c22270 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 17 Jul 2026 18:51:10 -0700 Subject: [PATCH] fix(ssh): probe npm via prepended PATH, not colocated with node (#9165) (#9255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ssh): probe npm via prepended PATH, not colocated with node (#9165) The remote Node/npm toolchain gate invoked npm by its absolute path /npm (POSIX) / npm.cmd (Windows, behind a Test-Path colocation check). But deploy (commandWithNodePath) runs bare `npm` with nodeBinDir merely prepended to PATH, so npm can resolve from anywhere on PATH. A host whose only resolvable node has npm elsewhere on PATH (e.g. node symlinked into a dir without npm) deployed fine on v1.4.144, but after upgrade the candidate is rejected with no fallback → SSH/relay connection fails to establish. Make the probe resolve npm exactly the way deploy does — bare `npm --version` under the same prepended PATH — so it still confirms npm is runnable (the #8450 concern) without requiring colocation. Windows now prepends the backslash-form dir (matching deploy) so bare-command PATH lookup resolves reliably. * test(ssh): cover split Node npm PATH resolution --- .../ssh/ssh-remote-node-resolution.test.ts | 38 ++++++++++++++++++- src/main/ssh/ssh-remote-node-resolution.ts | 5 +-- .../ssh-remote-node-toolchain-probe.test.ts | 16 ++++---- .../ssh/ssh-remote-node-toolchain-probe.ts | 14 ++++--- 4 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/main/ssh/ssh-remote-node-resolution.test.ts b/src/main/ssh/ssh-remote-node-resolution.test.ts index 55a3877ff..a48687284 100644 --- a/src/main/ssh/ssh-remote-node-resolution.test.ts +++ b/src/main/ssh/ssh-remote-node-resolution.test.ts @@ -51,12 +51,46 @@ describe('resolveRemoteNodePath', () => { '/home/u/.nvm/versions/node/v22.22.0/bin/node' ) - expect(execCommandMock.mock.calls[1]![1]).toContain("'/usr/bin/npm' --version") + expect(execCommandMock.mock.calls[1]![1]).toContain("PATH='/usr/bin':$PATH npm --version") expect(execCommandMock.mock.calls[2]![1]).toContain( - "'/home/u/.nvm/versions/node/v22.22.0/bin/npm' --version" + "PATH='/home/u/.nvm/versions/node/v22.22.0/bin':$PATH npm --version" ) }) + it.runIf(process.platform !== 'win32')( + 'accepts npm elsewhere on PATH without probing another Node candidate', + async () => { + const root = mkdtempSync(path.join(os.tmpdir(), 'orca-split-node-npm-')) + try { + const nodePath = path.join(root, 'selected node', 'bin', 'node') + const npmBinDir = path.join(root, 'npm elsewhere', 'bin') + mkdirSync(path.dirname(nodePath), { recursive: true }) + mkdirSync(npmBinDir, { recursive: true }) + writeFileSync(nodePath, '#!/bin/sh\nprintf "v22.22.0\\n"\n') + writeFileSync(path.join(npmBinDir, 'npm'), '#!/bin/sh\nprintf "11.13.0\\n"\n') + chmodSync(nodePath, 0o755) + chmodSync(path.join(npmBinDir, 'npm'), 0o755) + + execCommandMock + .mockResolvedValueOnce(`${nodePath}\n${path.join(root, 'fallback', 'bin', 'node')}\n`) + .mockImplementationOnce((_conn: SshConnection, command: string) => + Promise.resolve( + execFileSync('/bin/sh', ['-c', command], { + encoding: 'utf8', + env: { HOME: root, PATH: npmBinDir } + }) + ) + ) + + await expect(resolveRemoteNodePath(conn)).resolves.toBe(nodePath) + // One inventory exec plus one candidate probe keeps SSH startup work bounded. + expect(execCommandMock).toHaveBeenCalledTimes(2) + } finally { + rmSync(root, { recursive: true, force: true }) + } + } + ) + it('probes mise install directories', async () => { execCommandMock .mockResolvedValueOnce('/home/u/.local/share/mise/installs/node/20/bin/node\n') diff --git a/src/main/ssh/ssh-remote-node-resolution.ts b/src/main/ssh/ssh-remote-node-resolution.ts index 2dd89e4a2..1f2af0570 100644 --- a/src/main/ssh/ssh-remote-node-resolution.ts +++ b/src/main/ssh/ssh-remote-node-resolution.ts @@ -174,9 +174,8 @@ async function tryResolveViaLoginShell( return null } -// Returns true if `nodePath` runs, reports Node >= 18, and has a runnable npm -// beside it. Deployment prepends this same directory before invoking npm, so -// accepting a looser pairing would recreate #8450. +// Validates the same PATH-prepend + bare npm contract used during deployment. +// This rejects missing npm (#8450) without requiring colocation (#9165). // Caches nothing — this runs at most a few times per resolution (one per // candidate), and the exec round-trip dominates. async function nodeToolchainMeetsRequirements( diff --git a/src/main/ssh/ssh-remote-node-toolchain-probe.test.ts b/src/main/ssh/ssh-remote-node-toolchain-probe.test.ts index 4fb3dda56..64f1b82d1 100644 --- a/src/main/ssh/ssh-remote-node-toolchain-probe.test.ts +++ b/src/main/ssh/ssh-remote-node-toolchain-probe.test.ts @@ -6,20 +6,22 @@ import { } from './ssh-remote-node-toolchain-probe' describe('remote Node/npm toolchain probe', () => { - it('probes npm beside the selected POSIX Node with that directory on PATH', () => { + it('probes bare npm with the selected POSIX Node directory prepended to PATH', () => { + // Deploy runs bare `npm` under the same prepended PATH, so accept npm from + // anywhere on PATH rather than requiring it colocated with node (#9165). expect(buildPosixNodeToolchainProbe('/home/u/My Node/bin/node')).toBe( "printf '%s\\n' '__ORCA_NODE_VERSION__' && '/home/u/My Node/bin/node' --version && " + - "printf '%s\\n' '__ORCA_NPM_VERSION__' && PATH='/home/u/My Node/bin':$PATH " + - "'/home/u/My Node/bin/npm' --version" + "printf '%s\\n' '__ORCA_NPM_VERSION__' && PATH='/home/u/My Node/bin':$PATH npm --version" ) }) - it('probes npm.cmd beside the selected Windows Node', () => { + it('probes bare npm with the selected Windows Node directory prepended to PATH', () => { const probe = buildWindowsNodeToolchainProbe('C:/Program Files/nodejs/node.exe') - expect(probe).toContain("Test-Path -LiteralPath 'C:/Program Files/nodejs/npm.cmd'") - expect(probe).toContain("$env:PATH = 'C:/Program Files/nodejs' + ';' + $env:PATH") - expect(probe).toContain("& 'C:/Program Files/nodejs/npm.cmd' --version") + expect(probe).not.toContain('Test-Path') + expect(probe).toContain("$env:PATH = 'C:\\Program Files\\nodejs' + ';' + $env:PATH") + expect(probe).toContain("& 'C:/Program Files/nodejs/node.exe' --version") + expect(probe).toContain('& npm --version') }) it('requires marked, parseable Node and npm versions', () => { diff --git a/src/main/ssh/ssh-remote-node-toolchain-probe.ts b/src/main/ssh/ssh-remote-node-toolchain-probe.ts index addf9b575..b1e0b8d54 100644 --- a/src/main/ssh/ssh-remote-node-toolchain-probe.ts +++ b/src/main/ssh/ssh-remote-node-toolchain-probe.ts @@ -8,26 +8,28 @@ const NPM_VERSION_MARKER = '__ORCA_NPM_VERSION__' export function buildPosixNodeToolchainProbe(nodePath: string): string { const nodeBinDir = posixPath.dirname(nodePath) - const npmPath = posixPath.join(nodeBinDir, 'npm') return [ `printf '%s\\n' '${NODE_VERSION_MARKER}'`, `${shellEscape(nodePath)} --version`, `printf '%s\\n' '${NPM_VERSION_MARKER}'`, - `PATH=${shellEscape(nodeBinDir)}:$PATH ${shellEscape(npmPath)} --version` + // Why: deploy prepends nodeBinDir before running bare npm; requiring a + // colocated executable rejects valid split layouts (#9165). + `PATH=${shellEscape(nodeBinDir)}:$PATH npm --version` ].join(' && ') } export function buildWindowsNodeToolchainProbe(nodePath: string): string { const nodeBinDir = posixPath.dirname(nodePath) - const npmPath = posixPath.join(nodeBinDir, 'npm.cmd') + const windowsNodeBinDir = nodeBinDir.replace(/\//g, '\\') return [ - `if (!(Test-Path -LiteralPath ${powerShellLiteral(npmPath)} -PathType Leaf)) { exit 1 }`, - `$env:PATH = ${powerShellLiteral(nodeBinDir)} + ';' + $env:PATH`, + // Why: mirror deploy's PATH-prepend + bare npm resolution so split + // Node/npm layouts are not rejected solely for lacking npm.cmd (#9165). + `$env:PATH = ${powerShellLiteral(windowsNodeBinDir)} + ';' + $env:PATH`, `Write-Output ${powerShellLiteral(NODE_VERSION_MARKER)}`, `& ${powerShellLiteral(nodePath)} --version`, 'if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }', `Write-Output ${powerShellLiteral(NPM_VERSION_MARKER)}`, - `& ${powerShellLiteral(npmPath)} --version`, + '& npm --version', 'exit $LASTEXITCODE' ].join('; ') }