diff --git a/src/main/index.ts b/src/main/index.ts index da9c334c7e1..bf14f7292ba 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -699,7 +699,14 @@ if (app.isPackaged && process.platform !== 'win32') { void hydrateShellPath().then((result) => { if (result.ok) { mergePathSegments(result.segments) + return } + // Why: on failure the seeded fallbacks stay in front, which for an nvm user means + // the newest install rather than their `default`. Name the reason so that shows up + // in a log bundle instead of as an unexplained missing CLI. + console.warn( + `[shell-path] login-shell probe failed (${result.failureReason}); using seeded PATH` + ) }) } configureDevUserDataPath(is.dev) diff --git a/src/main/startup/configure-process.test.ts b/src/main/startup/configure-process.test.ts index 230984de851..54ecdc61424 100644 --- a/src/main/startup/configure-process.test.ts +++ b/src/main/startup/configure-process.test.ts @@ -3,10 +3,6 @@ import { homedir, tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' -vi.mock('./hydrate-shell-path', () => ({ - recordLaunchPath: vi.fn() -})) - vi.mock('electron', () => { const paths = new Map([['appData', '/tmp/app-data']]) return { @@ -60,25 +56,6 @@ describe('patchPackagedProcessPath', () => { } }) - it('records the pre-seed PATH so the login-shell probe cannot inherit a seeded nvm version dir', async () => { - const { app } = await import('electron') - const { patchPackagedProcessPath } = await import('./configure-process') - const { recordLaunchPath } = await import('./hydrate-shell-path') - const recorded = vi.mocked(recordLaunchPath) - - setPlatform('darwin') - Object.defineProperty(app, 'isPackaged', { configurable: true, value: true }) - process.env.HOME = '/Users/tester' - process.env.PATH = '/usr/bin:/bin' - recorded.mockClear() - - patchPackagedProcessPath() - - expect(recorded).toHaveBeenCalledWith('/usr/bin:/bin', 'PATH') - // Why: recording must happen before the seeds land, or the probe sees them anyway. - expect(process.env.PATH).not.toBe('/usr/bin:/bin') - }) - it('prepends agent-CLI install dirs (~/.opencode/bin, ~/.vite-plus/bin) for packaged darwin runs', 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 fb470b06398..e39963bc9f2 100644 --- a/src/main/startup/configure-process.ts +++ b/src/main/startup/configure-process.ts @@ -4,7 +4,6 @@ import { homedir } from 'node:os' import { join, resolve } from 'node:path' import { getVersionManagerBinPaths } from '../codex-cli/command' import { getMainE2EConfig } from '../e2e-config' -import { recordLaunchPath } from './hydrate-shell-path' const DEV_PARENT_SHUTDOWN_GRACE_MS = 3000 const HTTP1_COMPATIBILITY_ENV_VAR = 'ORCA_DISABLE_HTTP2' @@ -140,11 +139,6 @@ export function patchPackagedProcessPath(): void { const pathKey = process.platform === 'win32' && process.env.Path !== undefined ? 'Path' : 'PATH' const currentPath = process.env[pathKey] ?? '' - // Why: the login-shell probe must not see the seeds below. nvm's startup - // `use` keeps whatever node is already on PATH instead of the user's - // `default` alias, so a seeded version dir would pin every terminal to the - // newest install and hide global CLIs installed under `default`. - recordLaunchPath(currentPath, pathKey) const pathDelimiter = getProcessPathDelimiter() const existing = new Set(currentPath.split(pathDelimiter)) const missing = extraPaths.filter((path) => !existing.has(path)) diff --git a/src/main/startup/hydrate-shell-path.test.ts b/src/main/startup/hydrate-shell-path.test.ts index a53d78779b9..d70441f6f5b 100644 --- a/src/main/startup/hydrate-shell-path.test.ts +++ b/src/main/startup/hydrate-shell-path.test.ts @@ -6,7 +6,7 @@ import { _resetHydrateShellPathCache, hydrateShellPath, mergePathSegments, - recordLaunchPath, + _setLaunchPathForTests, type HydrationResult } from './hydrate-shell-path' @@ -180,39 +180,44 @@ describe('hydrateShellPath', () => { } }) - it('probes with the recorded launch PATH so seeded fallbacks cannot pin nvm', async () => { - // Why: shellProbeEnv short-circuits on win32, so pin the platform or this - // POSIX-only assertion fails for anyone running the suite on Windows. - vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + async function captureProbeEnv(shell = '/bin/bash'): Promise { const proc = createMockShellProcess() spawnMock.mockReturnValue(proc) - process.env.PATH = `/home/u/.nvm/versions/node/v26.7.0/bin${delimiter}/usr/bin` - recordLaunchPath('/usr/bin') - - const resultPromise = hydrateShellPath({ shellOverride: '/bin/bash', force: true }) + const resultPromise = hydrateShellPath({ shellOverride: shell, force: true }) await vi.waitFor(() => expect(spawnMock).toHaveBeenCalled()) proc.emit('close', 0) await resultPromise + return spawnMock.mock.calls[0][2].env + } - expect(spawnMock.mock.calls[0][2].env.PATH).toBe('/usr/bin') + it('probes with the launch PATH so seeded fallbacks cannot pin nvm', async () => { + process.env.PATH = `/home/u/.nvm/versions/node/v26.7.0/bin${delimiter}/usr/bin` + _setLaunchPathForTests('/usr/bin') + + expect((await captureProbeEnv()).PATH).toBe('/usr/bin') }) - it('overwrites the recorded key in place so Windows never carries both Path and PATH', async () => { - vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') - const proc = createMockShellProcess() - spawnMock.mockReturnValue(proc) + it('uses the PATH captured at module load, not a later mutation of process.env', async () => { + // Why: this is the anti-regression guard. patchPackagedProcessPath mutates + // process.env.PATH after this module is evaluated; the probe must not see it. + const seeded = `/seeded/newest-nvm/bin${delimiter}/usr/bin` + process.env.PATH = seeded + + expect((await captureProbeEnv()).PATH).not.toBe(seeded) + }) + + it('sets a probe marker so rc files can take a fast path', async () => { + expect((await captureProbeEnv()).ORCA_SHELL_PATH_PROBE).toBe('1') + }) + + it('overwrites the captured key in place so Windows never carries both Path and PATH', async () => { const originalWindowsPath = process.env.Path delete process.env.PATH - process.env.Path = `C:\\Users\\u\\.nvm\\versions\\node\\v22.9.0\\bin;C:\\Windows` - recordLaunchPath('C:\\Windows', 'Path') + process.env.Path = 'C:\\Users\\u\\.nvm\\versions\\node\\v22.9.0\\bin;C:\\Windows' + _setLaunchPathForTests('C:\\Windows', 'Path') try { - const resultPromise = hydrateShellPath({ shellOverride: 'C:/Git/bin/bash.exe', force: true }) - await vi.waitFor(() => expect(spawnMock).toHaveBeenCalled()) - proc.emit('close', 0) - await resultPromise - - const env = spawnMock.mock.calls[0][2].env + const env = await captureProbeEnv('C:/Git/bin/bash.exe') expect(env.Path).toBe('C:\\Windows') expect(env.PATH).toBeUndefined() } finally { @@ -223,19 +228,6 @@ describe('hydrateShellPath', () => { } } }) - - it('falls back to process.env when no launch PATH was recorded', async () => { - const proc = createMockShellProcess() - spawnMock.mockReturnValue(proc) - process.env.PATH = '/seeded/bin' - - const resultPromise = hydrateShellPath({ shellOverride: '/bin/bash', force: true }) - await vi.waitFor(() => expect(spawnMock).toHaveBeenCalled()) - proc.emit('close', 0) - await resultPromise - - expect(spawnMock.mock.calls[0][2].env.PATH).toBe('/seeded/bin') - }) }) describe('mergePathSegments', () => { diff --git a/src/main/startup/hydrate-shell-path.ts b/src/main/startup/hydrate-shell-path.ts index 65c5b551499..af1f7fbdb95 100644 --- a/src/main/startup/hydrate-shell-path.ts +++ b/src/main/startup/hydrate-shell-path.ts @@ -33,14 +33,22 @@ export type HydrationResult = let cached: Promise | null = null let probeQueue = Promise.resolve() // Why: patchPackagedProcessPath seeds a version manager's newest install dir -// ahead of PATH before we probe. nvm's startup `use` honors whatever node is -// already on PATH over the user's `default` alias, so a probe that inherits the -// seed comes back pinned to that version and every terminal loses the global -// CLIs installed under `default`. Probe with the launch PATH instead. The key is -// recorded too because Windows keys it `Path`, and a Git Bash user whose nvm uses -// the POSIX ~/.nvm/versions/node layout gets a version-pinned seed there as well. -let launchPath: string | null = null -let launchPathKey = 'PATH' +// ahead of PATH. nvm's startup `use` honors whatever node is already on PATH over +// the user's `default` alias, so a probe inheriting that seed comes back pinned to +// the newest install and every terminal loses the global CLIs installed under +// `default`. Snapshot PATH here instead: module init runs while the import graph is +// evaluated, strictly before any statement in main's body, so it cannot observe the +// seeds — and unlike an explicit hand-off from the seeding site, there is no call +// ordering left for a later refactor to break. Windows keys it `Path`, and a Git Bash +// user whose nvm uses the POSIX ~/.nvm/versions/node layout is seeded there too, so +// capture the key alongside the value. +const LAUNCH_PATH_KEY = + process.platform === 'win32' && process.env.Path !== undefined ? 'Path' : 'PATH' +const LAUNCH_PATH = process.env[LAUNCH_PATH_KEY] ?? null +// Why: rc files that exec into a multiplexer or start a heavy prompt can outrun the +// probe budget. This lets them detect the probe and take a fast path. +const PROBE_MARKER_ENV_VAR = 'ORCA_SHELL_PATH_PROBE' +let launchPathOverride: { key: string; value: string } | null = null let configuredWindowsShell = 'powershell.exe' let configuredWindowsGitBashPath: string | null = null let configuredWindowsFallbackShell: string | null = null @@ -51,8 +59,7 @@ const windowsPathOwnership = new WindowsShellPathOwnership() export function _resetHydrateShellPathCache(): void { cached = null probeQueue = Promise.resolve() - launchPath = null - launchPathKey = 'PATH' + launchPathOverride = null configuredWindowsShell = 'powershell.exe' configuredWindowsGitBashPath = null configuredWindowsFallbackShell = null @@ -105,19 +112,21 @@ function parseCapturedPath(stdout: string, pathDelimiter: string = delimiter): s ] } -/** Records PATH as the process was launched with, before Orca seeds fallbacks onto it. */ -export function recordLaunchPath(pathValue: string, pathKey = 'PATH'): void { - launchPath = pathValue - launchPathKey = pathKey +/** @internal - tests need a launch PATH the runner's own environment cannot supply. */ +export function _setLaunchPathForTests(value: string, key = 'PATH'): void { + launchPathOverride = { key, value } } function shellProbeEnv(): NodeJS.ProcessEnv { - if (launchPath === null) { - return process.env + const key = launchPathOverride?.key ?? LAUNCH_PATH_KEY + const value = launchPathOverride?.value ?? LAUNCH_PATH + const env: NodeJS.ProcessEnv = { ...process.env, [PROBE_MARKER_ENV_VAR]: '1' } + if (value !== null) { + // Why the captured key, not a literal 'PATH': overwrite in place so Windows + // never ends up carrying both `Path` and `PATH` on the spawn env. + env[key] = value } - // Why the recorded key, not a literal 'PATH': overwrite the entry in place so - // Windows never ends up carrying both `Path` and `PATH`. - return { ...process.env, [launchPathKey]: launchPath } + return env } function shellPathProbe(shell: string): { args: string[]; pathDelimiter: string } {