diff --git a/src/main/startup/configure-process.test.ts b/src/main/startup/configure-process.test.ts index 4747f797cf9..ef7ec6a9b69 100644 --- a/src/main/startup/configure-process.test.ts +++ b/src/main/startup/configure-process.test.ts @@ -137,6 +137,66 @@ describe('patchPackagedProcessPath', () => { expect(segments).toContain('/usr/local/bin') }) + // Why this ordering is load-bearing (#18234): a seed exists so a GUI-launched + // Electron can *find* a tool, not to re-rank tools the user already has. + // `~/.local/bin` is user-writable and can hold a wrapper for any system tool. + // The reporter's `~/.local/bin/gh` wrapped `mise x gh -- gh`; seeded ahead of + // /usr/bin it ran instead of the real gh, and the wrapper's inner bare `gh` + // resolved back to itself. Measured in a container: with the login shell's + // ordering that chain exits in 22ms, with the seeded ordering it never + // terminates and creates ~1,300 processes/second. + it('never lets a seeded user dir overtake a system dir already on PATH', async () => { + const { app } = await import('electron') + const { patchPackagedProcessPath } = await import('./configure-process') + + setPlatform('linux') + Object.defineProperty(app, 'isPackaged', { configurable: true, value: true }) + process.env.HOME = '/home/tester' + process.env.PATH = '/usr/local/bin:/usr/bin:/bin' + + patchPackagedProcessPath() + + const segments = (process.env.PATH ?? '').split(':') + const localBin = segments.indexOf(join('/home/tester', '.local/bin')) + // Still reachable — that is what the seeding is for (#829). + expect(localBin).toBeGreaterThan(-1) + for (const systemDir of ['/usr/bin', '/bin', '/usr/local/bin']) { + expect(segments.indexOf(systemDir)).toBeLessThan(localBin) + } + expect(segments.indexOf(join('/home/tester', 'bin'))).toBeGreaterThan( + segments.indexOf('/usr/bin') + ) + }) + + it('keeps version-manager shims ahead of the inherited PATH', async () => { + const { app } = await import('electron') + const { patchPackagedProcessPath } = await import('./configure-process') + const { getVersionManagerBinPaths } = await import('../../shared/node-cli-command-resolution') + + setPlatform('linux') + Object.defineProperty(app, 'isPackaged', { configurable: true, value: true }) + process.env.HOME = '/home/tester' + process.env.PATH = '/usr/bin:/bin' + + patchPackagedProcessPath() + + const segments = (process.env.PATH ?? '').split(':') + const genericUserBinDirs = [join('/home/tester', 'bin'), join('/home/tester', '.local/bin')] + const seeded = getVersionManagerBinPaths({ platform: 'linux', homePath: '/home/tester' }) + const shimDirs = seeded.filter((dir) => !genericUserBinDirs.includes(dir)) + expect(shimDirs).not.toHaveLength(0) + // Why these keep leading: an nvm/mise/asdf user's runtime must beat a + // system install, which is the reason this seeding is ordered at all. + for (const dir of shimDirs) { + expect(segments.indexOf(dir)).toBeLessThan(segments.indexOf('/usr/bin')) + } + // Why these do not: the same list carries the generic user bin dirs, which + // hold whatever was last installed there rather than a managed toolchain. + for (const dir of genericUserBinDirs) { + expect(segments.indexOf(dir)).toBeGreaterThan(segments.indexOf('/usr/bin')) + } + }) + it('leaves PATH untouched when the app is not packaged', async () => { const { app } = await import('electron') const { patchPackagedProcessPath } = await import('./configure-process') diff --git a/src/main/startup/configure-process.ts b/src/main/startup/configure-process.ts index e49b7e04a03..6b167d7fe14 100644 --- a/src/main/startup/configure-process.ts +++ b/src/main/startup/configure-process.ts @@ -117,20 +117,37 @@ export function patchPackagedProcessPath(): void { } const home = process.env.HOME ?? '' - const extraPaths: string[] = [] + // Why two lists: a seed exists so a GUI-launched Electron can *find* a tool + // its minimal PATH omits. Putting one ahead of the inherited PATH does more + // than that — it re-ranks binaries the user already has, and `~/bin` and + // `~/.local/bin` are arbitrary user-writable directories that can shadow any + // system tool. On the #18234 reporter's box `~/.local/bin/gh` is a wrapper + // around `mise x gh -- gh`; hoisting it over /usr/bin/gh made us run the + // wrapper where their own shell ran the real binary, and the inner bare `gh` + // then resolved back to the wrapper. So: append these, and let a real + // ordering opinion come from the login shell via mergePathSegments. + const isGenericUserBinDir = (path: string): boolean => + process.platform !== 'win32' && + home !== '' && + (path === join(home, 'bin') || path === join(home, '.local/bin')) + const appendPaths: string[] = [] + // Why these still lead: version-manager shims must beat a system install or + // an nvm/mise/asdf user gets the wrong runtime, which is the whole reason + // this seeding is ordered rather than appended (see hydrate-shell-path.ts). + const prependPaths: string[] = [] if (process.platform !== 'win32') { - extraPaths.push('/opt/homebrew/bin', '/opt/homebrew/sbin', '/usr/local/bin', '/usr/local/sbin') + appendPaths.push('/opt/homebrew/bin', '/opt/homebrew/sbin', '/usr/local/bin', '/usr/local/sbin') if (process.platform === 'linux') { // Why: snap and Linuxbrew ship on Linux only, so seeding them elsewhere adds phantom PATH entries every spawn must stat. - extraPaths.push('/snap/bin', '/home/linuxbrew/.linuxbrew/bin') + appendPaths.push('/snap/bin', '/home/linuxbrew/.linuxbrew/bin') } - extraPaths.push('/nix/var/nix/profiles/default/bin') + appendPaths.push('/nix/var/nix/profiles/default/bin') if (home) { - extraPaths.push( + appendPaths.push( join(home, 'bin'), join(home, '.local/bin'), join(home, '.nix-profile/bin'), @@ -142,18 +159,24 @@ export function patchPackagedProcessPath(): void { } // Why: version-manager CLIs use env-node shebangs, so node must be on PATH or spawns fail (also seeds Windows user-local dirs). - extraPaths.push(...getVersionManagerBinPaths()) + // Why the filter: that list carries `~/bin` and `~/.local/bin` too, because + // bun/pnpm/npm --user also install there. Those two are generic user bin + // directories, not a version manager's own shim directory, so they hold + // whatever the user last dropped in them and must not outrank a system dir. + // The specific dirs (.volta/bin, .asdf/shims, mise shims, .bun/bin, …) keep + // leading, which is what the ordering was actually for. + prependPaths.push(...getVersionManagerBinPaths().filter((path) => !isGenericUserBinDir(path))) const pathKey = process.platform === 'win32' && process.env.Path !== undefined ? 'Path' : 'PATH' const currentPath = process.env[pathKey] ?? '' const pathDelimiter = getProcessPathDelimiter() - const existing = new Set(currentPath.split(pathDelimiter)) - const missing = extraPaths.filter((path) => !existing.has(path)) + const currentSegments = currentPath.split(pathDelimiter).filter(Boolean) + const existing = new Set(currentSegments) + const prepend = prependPaths.filter((path) => !existing.has(path)) + const append = appendPaths.filter((path) => !existing.has(path) && !prepend.includes(path)) - if (missing.length > 0) { - process.env[pathKey] = [...missing, ...currentPath.split(pathDelimiter).filter(Boolean)].join( - pathDelimiter - ) + if (prepend.length > 0 || append.length > 0) { + process.env[pathKey] = [...prepend, ...currentSegments, ...append].join(pathDelimiter) } }