mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(startup): snapshot the launch PATH at module load, log probe failures
Three loose ends from the review, folded in rather than deferred. The probe's clean PATH was handed over by an explicit recordLaunchPath call from the seeding site, so the invariant lived across three files and a refactor that moved the seed call would silently re-pin nvm. Snapshot PATH during module init instead: that runs while the import graph is evaluated, strictly before any statement in main's body, so it cannot observe the seeds and there is no call ordering left to break. All seven importers are static, so no lazy import can defeat it. A test asserts the probe ignores later process.env mutation, and fails if the live read is reintroduced. A failed startup probe leaves the seeded newest-nvm dir in front and said nothing, so the population whose rc files blow the 5s budget hit the original symptom with no diagnosable trace. Log the failureReason. The probe is an interactive login shell, so rc files that exec into a multiplexer or start a heavy prompt can outrun that budget with no way to opt out. Set ORCA_SHELL_PATH_PROBE=1 so they can take a fast path.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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<string, string>([['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')
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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<NodeJS.ProcessEnv> {
|
||||
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', () => {
|
||||
|
||||
@@ -33,14 +33,22 @@ export type HydrationResult =
|
||||
let cached: Promise<HydrationResult> | 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 } {
|
||||
|
||||
Reference in New Issue
Block a user