diff --git a/src/main/claude-accounts/service.ts b/src/main/claude-accounts/service.ts index 88e3712fde8..b5c4bc220f5 100644 --- a/src/main/claude-accounts/service.ts +++ b/src/main/claude-accounts/service.ts @@ -13,6 +13,7 @@ import type { import type { Store } from '../persistence' import type { RateLimitService } from '../rate-limits/service' import { resolveClaudeCommand } from '../codex-cli/command' +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' import type { ClaudeRuntimeAuthService } from './runtime-auth-service' import { getClaudeManagedAccountsRoot, @@ -1050,17 +1051,21 @@ export class ClaudeAccountService { configDir.wslDistro === null && args[0] === 'auth' && args[1] === 'login' + // Why lazy: the WSL branch runs `claude` inside the distro, so resolving a + // host binary there would be wasted filesystem probing for a path never used. + let cachedHostClaudeCommand: string | null = null + const hostClaudeCommand = (): string => (cachedHostClaudeCommand ??= resolveClaudeCommand()) const interactiveLogin = isWindowsHostInteractiveLogin - ? buildWindowsHostInteractiveLoginSpawn(resolveClaudeCommand(), args) + ? buildWindowsHostInteractiveLoginSpawn(hostClaudeCommand(), args) : null const spawnConfig = interactiveLogin ? { command: interactiveLogin.command, args: interactiveLogin.args, - env: { + env: withCliRuntimeOnPath(hostClaudeCommand(), { ...process.env, CLAUDE_CONFIG_DIR: configDir.windowsPath - }, + }), shell: false, windowsVerbatimArguments: false } @@ -1081,20 +1086,20 @@ export class ClaudeAccountService { } : process.platform === 'win32' ? { - ...buildWindowsCommandInvocation(resolveClaudeCommand(), args), - env: { + ...buildWindowsCommandInvocation(hostClaudeCommand(), args), + env: withCliRuntimeOnPath(hostClaudeCommand(), { ...process.env, CLAUDE_CONFIG_DIR: configDir.windowsPath - }, + }), shell: false } : { - command: resolveClaudeCommand(), + command: hostClaudeCommand(), args, - env: { + env: withCliRuntimeOnPath(hostClaudeCommand(), { ...process.env, CLAUDE_CONFIG_DIR: configDir.windowsPath - }, + }), shell: false, windowsVerbatimArguments: false } diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index 5fd0a5ef649..56cfd630f2d 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -40,6 +40,7 @@ import { getSystemCodexHomePath } from '../codex/codex-home-paths' import { MANAGED_HOOK_TIMEOUT_SECONDS } from '../agent-hooks/installer-utils' import { readCodexTopLevelModelProvider } from '../codex/codex-model-provider-config' import { resolveCodexCommand } from '../codex-cli/command' +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' import type { Store } from '../persistence' import type { RateLimitService } from '../rate-limits/service' import { parseWslUncPath } from '../../shared/wsl-paths' @@ -1736,10 +1737,10 @@ export class CodexAccountService { return { command: spawnCmd, args: spawnArgs, - env: { + env: withCliRuntimeOnPath(codexCommand, { ...process.env, CODEX_HOME: managedHomePath - }, + }), codexCommand, interactiveLogin } diff --git a/src/main/codex-cli/command.test.ts b/src/main/codex-cli/command.test.ts index 045048f2919..dbe1c358418 100644 --- a/src/main/codex-cli/command.test.ts +++ b/src/main/codex-cli/command.test.ts @@ -4,6 +4,7 @@ import { delimiter, dirname, join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { getVersionManagerBinPaths, + withCliRuntimeOnPath, resolveClaudeCommand, resolveCliCommands, resolveCodexCommand @@ -313,3 +314,79 @@ describe('getVersionManagerBinPaths', () => { expect(paths).toContain(join(root, 'AppData', 'Roaming', 'npm')) }) }) + +describe('withCliRuntimeOnPath', () => { + it('pairs a version-manager CLI with its sibling node (stablyai/orca#10932)', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-pair-')) + const v20 = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + const v22 = join(root, '.nvm', 'versions', 'node', 'v22.9.0', 'bin') + makeExecutable(join(v20, 'node')) + makeExecutable(join(v20, 'codex')) + makeExecutable(join(v22, 'node')) + + // default is v22, but codex only exists under v20 + const env = { PATH: [v22, '/usr/bin'].join(delimiter) } + const codex = resolveCodexCommand({ platform: 'darwin', pathEnv: env.PATH, homePath: root }) + expect(codex).toBe(join(v20, 'codex')) + + const paired = withCliRuntimeOnPath(codex, env, { platform: 'darwin' }) + // the shebang's `node` must now come from v20, not v22 + expect(paired.PATH.split(delimiter)[0]).toBe(v20) + }) + + it('leaves PATH untouched for a CLI whose directory ships no node', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-pair-')) + const brew = join(root, 'opt', 'homebrew', 'bin') + makeExecutable(join(brew, 'codex')) + const env = { PATH: '/usr/bin' } + + expect(withCliRuntimeOnPath(join(brew, 'codex'), env, { platform: 'darwin' })).toBe(env) + }) + + it('leaves PATH untouched for a bare command name', () => { + const env = { PATH: '/usr/bin' } + expect(withCliRuntimeOnPath('codex', env, { platform: 'darwin' })).toBe(env) + }) + + it('is a no-op when the runtime directory already leads PATH', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-pair-')) + const v20 = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + makeExecutable(join(v20, 'node')) + makeExecutable(join(v20, 'codex')) + const env = { PATH: [v20, '/usr/bin'].join(delimiter) } + + expect(withCliRuntimeOnPath(join(v20, 'codex'), env, { platform: 'darwin' })).toBe(env) + }) + + it('reads the Windows path key the child will actually use, whatever its casing', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-pair-')) + const v20 = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + makeExecutable(join(v20, 'node.exe')) + makeExecutable(join(v20, 'codex.cmd')) + // Why: win32 resolves env names case-insensitively, so a block may spell it + // any way. Reading a narrower set than the twin-dedupe deletes would drop + // this entry unread and hand the child a PATH containing only our directory. + const env = { path: 'C:\\Windows;C:\\Windows\\System32', HOME: 'x' } + + const paired = withCliRuntimeOnPath(join(v20, 'codex.cmd'), env, { platform: 'win32' }) + expect(paired.path).toBe([v20, 'C:\\Windows', 'C:\\Windows\\System32'].join(';')) + expect(Object.keys(paired).filter((key) => key.toLowerCase() === 'path')).toEqual(['path']) + }) + + it('writes the Windows Path key without leaving a differently-cased twin', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-pair-')) + const v20 = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + makeExecutable(join(v20, 'node.exe')) + makeExecutable(join(v20, 'codex.cmd')) + // Why both keys: with only `Path` seeded the assertion is vacuous — the + // helper cannot invent a `PATH` key, so the dedupe loop could be deleted + // wholesale and this test would still pass. + const env = { Path: 'C:\\Windows;C:\\Windows\\System32', PATH: 'C:\\Stale' } + + const paired = withCliRuntimeOnPath(join(v20, 'codex.cmd'), env, { platform: 'win32' }) + expect(Object.keys(paired)).toEqual(['Path']) + // Why the whole string: splitting on the host delimiter while joining on ';' + // shredded every drive letter into `C;\\Windows`. + expect(paired.Path).toBe([v20, 'C:\\Windows', 'C:\\Windows\\System32'].join(';')) + }) +}) diff --git a/src/main/codex/codex-app-server-session.ts b/src/main/codex/codex-app-server-session.ts index 04748e6eaa2..2f82242532b 100644 --- a/src/main/codex/codex-app-server-session.ts +++ b/src/main/codex/codex-app-server-session.ts @@ -1,6 +1,7 @@ import { spawn, type ChildProcess, type ChildProcessWithoutNullStreams } from 'node:child_process' import { waitForProcessExitUntil } from './codex-process-exit-deadline' import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-signal' +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' // Why: `codex app-server` is Orca's sanctioned RPC surface into Codex-owned // state (hook trust hashes, the sqlite thread index). This module owns the @@ -10,6 +11,13 @@ import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-s export type CodexAppServerInvocation = { command: string args: string[] + /** + * The resolved CLI path, when `command` is a wrapper such as cmd.exe. Used to + * pair the CLI with the `node` it was installed against; without it a CLI + * resolved out of a version-manager directory runs under whatever node leads + * PATH and dies on a NODE_MODULE_VERSION mismatch (stablyai/orca#10932). + */ + cliPath?: string /** Overlay applied on top of the inherited environment (e.g. CODEX_HOME). */ env?: Record /** Env keys stripped from the inherited environment before spawn (e.g. an @@ -111,8 +119,9 @@ export async function runCodexAppServerSession( for (const key of invocation.envToDelete ?? []) { delete childEnv[key] } + const pairedEnv = withCliRuntimeOnPath(invocation.cliPath ?? invocation.command, childEnv) const child = spawnImpl(invocation.command, invocation.args, { - env: childEnv, + env: pairedEnv, stdio: ['pipe', 'pipe', 'pipe'], windowsHide: true }) as ChildProcessWithoutNullStreams diff --git a/src/main/codex/codex-session-index-heal.ts b/src/main/codex/codex-session-index-heal.ts index 49e575f4f12..baaba13358b 100644 --- a/src/main/codex/codex-session-index-heal.ts +++ b/src/main/codex/codex-session-index-heal.ts @@ -267,6 +267,7 @@ function buildNativeHealInvocation( return { command: spawnCmd, args: spawnArgs, + cliPath: command, // Why: pin the real home explicitly — nested Orca launches can inherit a // managed CODEX_HOME from the daemon environment, which would index the // wrong sqlite DB. diff --git a/src/main/codex/codex-trust-grant-host.ts b/src/main/codex/codex-trust-grant-host.ts index b769445ef0a..ce36909cbd4 100644 --- a/src/main/codex/codex-trust-grant-host.ts +++ b/src/main/codex/codex-trust-grant-host.ts @@ -65,6 +65,7 @@ export function resolveCodexTrustGrantHost(host: CodexTrustGrantHost): ResolvedC invocation: { command: spawnCmd, args: spawnArgs, + cliPath: command, ...(useDefaultCodexHome ? { envToDelete: ['CODEX_HOME'] } : { env: { CODEX_HOME: input.runtimeHomePath } }), diff --git a/src/main/rate-limits/claude-pty.ts b/src/main/rate-limits/claude-pty.ts index 10feb7e90b2..210f4778ac9 100644 --- a/src/main/rate-limits/claude-pty.ts +++ b/src/main/rate-limits/claude-pty.ts @@ -4,6 +4,9 @@ make the lifecycle harder to audit. */ import type { ProviderRateLimits, RateLimitWindow } from '../../shared/rate-limit-types' import { buildConfiguredProxyEnv, type NetworkProxySettings } from '../../shared/network-proxy' import { resolveClaudeCommand } from '../codex-cli/command' +// Why: import from the shared module, not the codex-cli re-export, so a test that +// mocks '../codex-cli/command' does not have to restate this pure helper. +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' import type { ClaudeRuntimeAuthPreparation } from '../claude-accounts/runtime-auth-service' import { applyClaudeEnvPatch } from '../claude-accounts/environment' import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnostic' @@ -292,7 +295,7 @@ export async function fetchViaPty(options?: { // Why: hidden usage PTYs must not inherit the process cwd (e.g. / or a // drive root), which can trigger unbounded file discovery. cwd: resolveHiddenRateLimitPtyCwd(), - env: spawnEnv + env: withCliRuntimeOnPath(claudeCommand, spawnEnv) }) const termDisposables: { dispose: () => void }[] = [registerHiddenRateLimitPty(term)] let enterInterval: ReturnType | null = null diff --git a/src/main/rate-limits/codex-fetcher-runtime-pairing.test.ts b/src/main/rate-limits/codex-fetcher-runtime-pairing.test.ts new file mode 100644 index 00000000000..0353bd4c31d --- /dev/null +++ b/src/main/rate-limits/codex-fetcher-runtime-pairing.test.ts @@ -0,0 +1,133 @@ +import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { EventEmitter } from 'node:events' +import { tmpdir } from 'node:os' +import { delimiter, join } from 'node:path' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { childSpawnMock, readFileMock, resolveCodexCommandMock, ptySpawnMock } = vi.hoisted(() => ({ + childSpawnMock: vi.fn(), + readFileMock: vi.fn(), + resolveCodexCommandMock: vi.fn(), + ptySpawnMock: vi.fn() +})) + +vi.mock('node:child_process', () => ({ spawn: childSpawnMock })) +vi.mock('node:fs/promises', () => ({ readFile: readFileMock })) +vi.mock('../codex-cli/command', () => ({ resolveCodexCommand: resolveCodexCommandMock })) +vi.mock('node-pty', () => ({ spawn: ptySpawnMock })) +vi.mock('../codex/codex-state-db', () => ({ isCodexStateDbBackfillPending: vi.fn(() => false) })) +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn(() => Promise.resolve(null)) +})) +vi.mock('./codex-auth-presence', () => ({ probeCodexAuthPresence: vi.fn(() => 'present') })) + +import { fetchCodexRateLimits } from './codex-fetcher' + +function makeRpcChild() { + const child = new EventEmitter() as EventEmitter & { + stdout: EventEmitter + stderr: EventEmitter + stdin: EventEmitter & { write: ReturnType; end: ReturnType } + kill: ReturnType + exitCode: number | null + } + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + const exitNow = (): void => { + child.exitCode = 0 + child.emit('exit', 0, null) + child.emit('close', 0, null) + } + child.stdin = Object.assign(new EventEmitter(), { write: vi.fn(), end: vi.fn(exitNow) }) + child.exitCode = null + child.kill = vi.fn(() => { + exitNow() + return true + }) + return child +} + +/** A CLI installed under one version manager entry, with its sibling node. */ +function makeVersionManagerCli(cliName = 'codex'): { bin: string; cli: string } { + const root = mkdtempSync(join(tmpdir(), 'orca-fetch-pair-')) + const bin = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + mkdirSync(bin, { recursive: true }) + for (const name of ['node', 'node.exe', cliName]) { + writeFileSync(join(bin, name), '') + chmodSync(join(bin, name), 0o755) + } + return { bin, cli: join(bin, cliName) } +} + +describe('codex rate-limit spawn runtime pairing', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.clearAllMocks() + readFileMock.mockRejectedValue(new Error('no auth fixture')) + vi.stubGlobal('fetch', vi.fn()) + }) + + it("spawns the RPC reader with the resolved CLI's own node ahead of PATH", async () => { + // Why a real fixture: the helper short-circuits on a non-absolute command, + // so a bare 'codex' mock would make this pass vacuously. + const { bin, cli } = makeVersionManagerCli() + resolveCodexCommandMock.mockReturnValue(cli) + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + + const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) + await vi.advanceTimersByTimeAsync(0) + + const spawnEnv = childSpawnMock.mock.calls[0]?.[2]?.env as NodeJS.ProcessEnv + // Guards the argument choice: pairing spawnCmd (cmd.exe on win32) rather than + // the resolved CLI silently reverts the ABI fix (stablyai/orca#10932). + expect(spawnEnv.PATH?.split(delimiter)[0]).toBe(bin) + + rpcChild.emit('close') + await resultPromise + }) + + it('pairs the resolved CLI on win32, where the spawn command is cmd.exe', async () => { + // Why win32 specifically: on posix getSpawnArgsForWindows returns the CLI + // itself, so pairing the spawn command instead of the resolved CLI is + // indistinguishable. Only here does the wrong argument become cmd.exe. + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + const { bin, cli } = makeVersionManagerCli('codex.cmd') + resolveCodexCommandMock.mockReturnValue(cli) + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + + const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) + await vi.advanceTimersByTimeAsync(0) + + const spawnCommand = childSpawnMock.mock.calls[0]?.[0] as string + expect(spawnCommand.toLowerCase()).toContain('cmd.exe') + const spawnEnv = childSpawnMock.mock.calls[0]?.[2]?.env as NodeJS.ProcessEnv + expect((spawnEnv.Path ?? spawnEnv.PATH)?.split(';')[0]).toBe(bin) + + rpcChild.emit('close') + await resultPromise + } finally { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + } + }) + + it('leaves PATH alone when the CLI resolves to a bare command name', async () => { + resolveCodexCommandMock.mockReturnValue('codex') + const rpcChild = makeRpcChild() + childSpawnMock.mockReturnValue(rpcChild) + + const resultPromise = fetchCodexRateLimits({ allowPtyFallback: false }) + await vi.advanceTimersByTimeAsync(0) + + const spawnEnv = childSpawnMock.mock.calls[0]?.[2]?.env as NodeJS.ProcessEnv + expect(spawnEnv.PATH).toBe(process.env.PATH) + + rpcChild.emit('close') + await resultPromise + }) +}) diff --git a/src/main/rate-limits/codex-fetcher.ts b/src/main/rate-limits/codex-fetcher.ts index dc18e715fdd..00c78aaa5f8 100644 --- a/src/main/rate-limits/codex-fetcher.ts +++ b/src/main/rate-limits/codex-fetcher.ts @@ -19,6 +19,9 @@ import { type CodexRateWindowSnapshot } from './codex-rate-limit-window-classification' import { resolveCodexCommand } from '../codex-cli/command' +// Why: import from the shared module, not the codex-cli re-export, so a test that +// mocks '../codex-cli/command' does not have to restate this pure helper. +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' import { CODEX_READ_ONLY_APP_SERVER_ARGS } from '../codex-cli/codex-read-only-app-server-args' import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnostic' import { getCmdExePath, getSpawnArgsForWindows } from '../win32-utils' @@ -648,10 +651,10 @@ async function fetchViaRpc(options?: FetchCodexRateLimitsOptions): Promise | null = null @@ -1005,11 +1008,11 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise void }[] = [registerHiddenRateLimitPty(term)] diff --git a/src/shared/node-cli-command-resolution.ts b/src/shared/node-cli-command-resolution.ts index 44e85c4dd08..aa336e7f190 100644 --- a/src/shared/node-cli-command-resolution.ts +++ b/src/shared/node-cli-command-resolution.ts @@ -1,6 +1,6 @@ import { accessSync, constants, existsSync, readdirSync, statSync } from 'node:fs' import { homedir } from 'node:os' -import { delimiter, dirname, join } from 'node:path' +import { delimiter, dirname, isAbsolute, join } from 'node:path' type ResolveCommandOptions = { pathEnv?: string | null @@ -16,13 +16,16 @@ function getExecutableNames(platform: NodeJS.Platform, commandName: string): str return [commandName] } -function splitPath(pathEnv: string | null | undefined): string[] { +function splitPath( + pathEnv: string | null | undefined, + pathDelimiter: string = delimiter +): string[] { if (!pathEnv) { return [] } return pathEnv - .split(delimiter) + .split(pathDelimiter) .map((entry) => entry.trim()) .filter(Boolean) } @@ -209,6 +212,70 @@ export function resolveClaudeCommand(options: ResolveCommandOptions = {}): strin return resolveCliCommand('claude', options) } +// Why: Win32 resolves env names case-insensitively and object order preserves +// the block order, so the entry the child will actually read is the FIRST +// case-insensitive match — not necessarily `Path` or `PATH`. Reading a narrower +// set than the dedupe below deletes would destroy a third spelling unread. +// Mirrors resolvePathEnvKey in src/main/pty/windows-path-segment-merge.ts, which +// src/shared must not import. +function firstWindowsPathEnvKey(env: NodeJS.ProcessEnv): string { + for (const key of Object.keys(env)) { + if (key.toLowerCase() === 'path' && env[key] !== undefined) { + return key + } + } + return 'Path' +} + +/** + * Put a resolved CLI's own directory ahead of PATH when that directory ships a + * sibling `node`. + * + * Why: `resolveCliCommand` falls back to scanning every version-manager install + * when PATH misses, so it can hand back `~/.nvm/versions/node/v20.x/bin/codex` + * while PATH still leads with v22. The CLI's `#!/usr/bin/env node` shebang then + * loads a v20-built native module under a v22 ABI and the agent dies on first + * require (stablyai/orca#10932). Pair the binary with the runtime it was + * installed against instead. + * + * Only prepends when the sibling `node` really exists, so a CLI resolved from a + * directory that ships no node is left alone. + */ +export function withCliRuntimeOnPath( + commandPath: string, + env: T, + options: Pick = {} +): T { + const platform = options.platform ?? process.platform + if (!isAbsolute(commandPath)) { + return env + } + const commandDirectory = dirname(commandPath) + if (!findFirstExecutable(platform, [commandDirectory], getExecutableNames(platform, 'node'))) { + return env + } + const pathKey = platform === 'win32' ? firstWindowsPathEnvKey(env) : 'PATH' + const pathDelimiter = platform === 'win32' ? ';' : delimiter + const segments = splitPath(env[pathKey], pathDelimiter) + if (segments[0] === commandDirectory) { + return env + } + const next = [commandDirectory, ...segments.filter((entry) => entry !== commandDirectory)].join( + pathDelimiter + ) + const paired = { ...env, [pathKey]: next } + if (platform === 'win32') { + // Why: the spread is case-sensitive while Windows env lookup is not, so a + // differently-cased twin would keep shadowing the value we just wrote. + for (const name of Object.keys(paired)) { + if (name !== pathKey && name.toLowerCase() === pathKey.toLowerCase()) { + delete (paired as NodeJS.ProcessEnv)[name] + } + } + } + return paired as T +} + // Why: Node-script CLIs need their version-manager sibling `node` on PATH. export function getVersionManagerBinPaths(options: ResolveCommandOptions = {}): string[] { const platform = options.platform ?? process.platform