From 26f677ee948dbc4181a460408b29fe587d73311d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 22 Oct 2026 21:23:04 -0700 Subject: [PATCH] fix(codex): pair structured launches with the CLI runtime (#32061) * fix(codex): pair structured launches with the CLI runtime * test(codex): align launch snapshots with runtime pairing * fix(agent-session): keep this app's CLI ahead of the paired runtime dir Pairing ran after the Orca CLI prepend, so a runtime dir that also holds an `orca` (e.g. /usr/local/bin with node+codex+orca) shadowed this app's launcher for the child. Pair first; the CLI dir stays first on PATH. Drops a PATH-shape test the parity snapshots already cover. Co-Authored-By: Claude * fix(cli-runtime): skip compiled CLIs and keep inherited PATH when pairing a sibling node A native codex/claude beside a node (Homebrew, /usr/bin) never runs on it, so pairing only reordered PATH. An env naming no PATH now pairs over the PATH the child inherits instead of replacing it with the CLI dir alone. Co-Authored-By: Claude * test(cli-runtime): type the PATH-less env fixture Co-Authored-By: Claude --------- Co-authored-by: Claude --- .../claude/claude-structured-child-env.ts | 2 +- ...laude-structured-launch-resolution.test.ts | 6 ++- src/main/codex-cli/command.test.ts | 27 ++++++++++++ ...codex-structured-child-environment.test.ts | 44 ++++++++++++++++++- .../codex-structured-launch-resolution.ts | 3 +- ...tructured-agent-launch-parity.test.ts.snap | 10 ++--- .../runtime/structured-session-child-env.ts | 13 +++--- src/shared/cli-runtime-pairing-inputs.ts | 43 ++++++++++++++++++ src/shared/node-cli-command-resolution.ts | 28 ++++-------- 9 files changed, 141 insertions(+), 35 deletions(-) create mode 100644 src/shared/cli-runtime-pairing-inputs.ts diff --git a/src/main/claude/claude-structured-child-env.ts b/src/main/claude/claude-structured-child-env.ts index 19605b467fc..6056dd39c6f 100644 --- a/src/main/claude/claude-structured-child-env.ts +++ b/src/main/claude/claude-structured-child-env.ts @@ -69,7 +69,7 @@ async function readClaudeChildEnvSources(deps: ClaudeEnvDeps): Promise) => Record = (env) => env, - /** False when the session's seal pairs the CLI runtime itself, after the session's identity. */ + /** False when the session's seal pairs the CLI runtime itself, behind this app's CLI. */ { runtimeOnPath = true }: { runtimeOnPath?: boolean } = {} ): Record { const env = decorateEnv({ diff --git a/src/main/claude/claude-structured-launch-resolution.test.ts b/src/main/claude/claude-structured-launch-resolution.test.ts index 63c6ac8ba12..afc72aa9109 100644 --- a/src/main/claude/claude-structured-launch-resolution.test.ts +++ b/src/main/claude/claude-structured-launch-resolution.test.ts @@ -530,7 +530,11 @@ describe('claude structured launch resolution', () => { }) })({ identity: IDENTITY }) - expect((launch.env?.PATH ?? launch.env?.Path)?.split(delimiter)[0]).toBe(binDir) + // Right behind this app's CLI, so a runtime dir holding another `orca` cannot shadow it. + expect((launch.env?.PATH ?? launch.env?.Path)?.split(delimiter).slice(1)).toEqual([ + binDir, + '/usr/bin' + ]) }) it('refuses other hosts, WSL, providers, and account-home variables', async () => { diff --git a/src/main/codex-cli/command.test.ts b/src/main/codex-cli/command.test.ts index b7e4c6e87f1..5fd69be62b4 100644 --- a/src/main/codex-cli/command.test.ts +++ b/src/main/codex-cli/command.test.ts @@ -349,6 +349,33 @@ describe('withCliRuntimeOnPath', () => { expect(withCliRuntimeOnPath(join(brew, 'codex'), env, { platform: 'darwin' })).toBe(env) }) + it('leaves PATH untouched for a compiled CLI beside a node (Homebrew, /usr/bin)', () => { + const root = makeCommandFixture('orca-pair-') + const brew = join(root, 'opt', 'homebrew', 'bin') + makeExecutable(join(brew, 'node')) + makeExecutable(join(brew, 'codex')) + writeFileSync(join(brew, 'codex'), Buffer.from([0xcf, 0xfa, 0xed, 0xfe, 0x07])) + const env = { PATH: '/usr/bin' } + + expect(withCliRuntimeOnPath(join(brew, 'codex'), env, { platform: 'darwin' })).toBe(env) + }) + + it('pairs over the inherited PATH when the env names none, instead of replacing it', () => { + const root = makeCommandFixture('orca-pair-') + const v20 = join(root, '.nvm', 'versions', 'node', 'v20.11.0', 'bin') + makeExecutable(join(v20, 'node')) + makeExecutable(join(v20, 'codex')) + const inherited = process.env.PATH + onTestFinished(() => { + process.env.PATH = inherited + }) + process.env.PATH = ['/opt/inherited/bin', '/usr/bin'].join(delimiter) + + const noPath: NodeJS.ProcessEnv = {} + const paired = withCliRuntimeOnPath(join(v20, 'codex'), noPath, { platform: 'darwin' }) + expect(paired.PATH).toBe([v20, '/opt/inherited/bin', '/usr/bin'].join(delimiter)) + }) + it('leaves PATH untouched for a bare command name', () => { const env = { PATH: '/usr/bin' } expect(withCliRuntimeOnPath('codex', env, { platform: 'darwin' })).toBe(env) diff --git a/src/main/codex/codex-structured-child-environment.test.ts b/src/main/codex/codex-structured-child-environment.test.ts index acb2bc7330d..67adfb369e4 100644 --- a/src/main/codex/codex-structured-child-environment.test.ts +++ b/src/main/codex/codex-structured-child-environment.test.ts @@ -1,4 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { delimiter, join } from 'node:path' import { openCodexAppServerConnection } from './codex-app-server-connection' import { ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV } from '../../shared/agent-session-caller-env' import { sealStructuredSessionChild } from '../runtime/structured-session-child-env' @@ -20,7 +23,8 @@ const DEV_CLI_LAUNCHER = /^[^:;]*[\\/]cli[\\/]bin[\\/]orca-dev$/ /** The sealed env a Codex launch of `sessionId` spawns with, over a shell env of `env`. */ async function codexChildEnv( env: Record, - sessionId: string + sessionId: string, + command = 'codex' ): Promise> { const record = agentSessionRecordFixture({ sessionId, @@ -30,7 +34,7 @@ async function codexChildEnv( const launch = await createCodexStructuredLaunchResolver({ store: { getRecord: () => record, pinLaunchDirectory: vi.fn() }, resolveEnvironment: async () => env, - resolveCommand: () => 'codex', + resolveCommand: () => command, resolveLaunchArgs: () => [] })({ // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: a launch reads only the session id of its identity here. @@ -151,6 +155,42 @@ describe('the spawned Codex child', () => { vi.unstubAllEnvs() }) + it.skipIf(process.platform === 'win32')( + 'starts the persistent shebang CLI with its sibling Node over a competing PATH runtime', + async () => { + const root = mkdtempSync(join(tmpdir(), 'orca-codex-shebang-')) + try { + const selectedBin = join(root, 'selected', 'bin') + const competingBin = join(root, 'other', 'bin') + mkdirSync(selectedBin, { recursive: true }) + mkdirSync(competingBin, { recursive: true }) + symlinkSync(process.execPath, join(selectedBin, 'node')) + writeFileSync(join(competingBin, 'node'), '#!/bin/sh\nexit 79\n', { mode: 0o755 }) + const command = join(selectedBin, 'codex') + writeFileSync(command, `#!/usr/bin/env node\n${ENV_REPORTING_APP_SERVER}`, { mode: 0o755 }) + const env = await codexChildEnv({ PATH: competingBin }, 'shebang-session', command) + const connection = await openCodexAppServerConnection({ + command, + args: ['app-server'], + cwd: root, + env + }) + try { + await expect(connection.request('test/env')).resolves.toMatchObject({ + sessionId: 'shebang-session', + path: expect.stringMatching(DEV_CLI_BIN_FIRST), + cliCommand: expect.stringMatching(DEV_CLI_LAUNCHER) + }) + expect(env.PATH.split(delimiter).slice(1)).toEqual([selectedBin, competingBin]) + } finally { + await connection.close() + } + } finally { + rmSync(root, { recursive: true, force: true }) + } + } + ) + it("runs with its own session id and this app's CLI, over an id inherited by Orca itself", async () => { // The builder's output is an overlay on process.env, so only the spawned child proves the id // survives the merge — an Orca launched inside another session inherits that session's id. diff --git a/src/main/codex/codex-structured-launch-resolution.ts b/src/main/codex/codex-structured-launch-resolution.ts index 151f3bc7be9..e2361cbda33 100644 --- a/src/main/codex/codex-structured-launch-resolution.ts +++ b/src/main/codex/codex-structured-launch-resolution.ts @@ -119,7 +119,8 @@ export function codexStructuredLaunchPart( inheritedEnvToDelete: [ ...(visuals ? [] : [NATIVE_CHAT_SCRATCH_DIR_ENV]), LEGACY_NATIVE_CHAT_VISUALS_DIR_ENV - ] + ], + cliRuntimeCommand: command } } } diff --git a/src/main/runtime/__snapshots__/structured-agent-launch-parity.test.ts.snap b/src/main/runtime/__snapshots__/structured-agent-launch-parity.test.ts.snap index 819e1b55e84..88fcd2639dd 100644 --- a/src/main/runtime/__snapshots__/structured-agent-launch-parity.test.ts.snap +++ b/src/main/runtime/__snapshots__/structured-agent-launch-parity.test.ts.snap @@ -564,7 +564,7 @@ exports[`native-chat launch parity > claude floating fresh 1`] = ` "ORCA_USER_DATA_PATH": "/user-data", "ORCA_WORKTREE_ID": "parent-worktree", "PARITY_OVERLAY": "claude", - "PATH": "/bin:/orca-cli/bin:/opt/login-shell/bin:/usr/bin:/bin", + "PATH": "/orca-cli/bin:/bin:/opt/login-shell/bin:/usr/bin:/bin", "SHELL_ONLY": "shell", "SHELL_UNLISTED": "shell", }, @@ -652,7 +652,7 @@ exports[`native-chat launch parity > claude floating resumed 1`] = ` "ORCA_USER_DATA_PATH": "/user-data", "ORCA_WORKTREE_ID": "parent-worktree", "PARITY_OVERLAY": "claude", - "PATH": "/bin:/orca-cli/bin:/opt/login-shell/bin:/usr/bin:/bin", + "PATH": "/orca-cli/bin:/bin:/opt/login-shell/bin:/usr/bin:/bin", "SHELL_ONLY": "shell", "SHELL_UNLISTED": "shell", }, @@ -738,7 +738,7 @@ exports[`native-chat launch parity > claude worktree fresh 1`] = ` "ORCA_USER_DATA_PATH": "/user-data", "ORCA_WORKTREE_ID": "parent-worktree", "PARITY_OVERLAY": "claude", - "PATH": "/bin:/orca-cli/bin:/opt/login-shell/bin:/usr/bin:/bin", + "PATH": "/orca-cli/bin:/bin:/opt/login-shell/bin:/usr/bin:/bin", "SHELL_ONLY": "shell", "SHELL_UNLISTED": "shell", }, @@ -904,7 +904,7 @@ exports[`native-chat launch parity > claude worktree fresh worker 1`] = ` "ORCA_USER_DATA_PATH": "/user-data", "ORCA_WORKTREE_ID": "parent-worktree", "PARITY_OVERLAY": "claude", - "PATH": "/bin:/orca-cli/bin:/opt/login-shell/bin:/usr/bin:/bin", + "PATH": "/orca-cli/bin:/bin:/opt/login-shell/bin:/usr/bin:/bin", "SHELL_ONLY": "shell", "SHELL_UNLISTED": "shell", }, @@ -990,7 +990,7 @@ exports[`native-chat launch parity > claude worktree resumed 1`] = ` "ORCA_USER_DATA_PATH": "/user-data", "ORCA_WORKTREE_ID": "parent-worktree", "PARITY_OVERLAY": "claude", - "PATH": "/bin:/orca-cli/bin:/opt/login-shell/bin:/usr/bin:/bin", + "PATH": "/orca-cli/bin:/bin:/opt/login-shell/bin:/usr/bin:/bin", "SHELL_ONLY": "shell", "SHELL_UNLISTED": "shell", }, diff --git a/src/main/runtime/structured-session-child-env.ts b/src/main/runtime/structured-session-child-env.ts index 7ba6ebd5a71..87f652bc666 100644 --- a/src/main/runtime/structured-session-child-env.ts +++ b/src/main/runtime/structured-session-child-env.ts @@ -97,7 +97,7 @@ export type SealedStructuredSessionChild = { * `env` named as this session's child. `inheritedEnvToDelete` is required so every agent states * what it strips, none included; a key the seal itself names the child by is never stripped (a * registered worker keeps its handle). `cliRuntimeCommand`: a Node CLI whose own directory goes - * first on PATH, after the identity's, so it runs on the runtime beside it. + * on PATH right behind this app's CLI, so it runs on the runtime beside it. */ export function sealStructuredSessionChild(input: { sessionId: string @@ -106,13 +106,14 @@ export function sealStructuredSessionChild(input: { inheritedEnvToDelete: readonly string[] cliRuntimeCommand?: string }): SealedStructuredSessionChild { - const identity = childIdentity(input.sessionId, input.env) + // Why: pair before naming the child so this app's CLI dir stays ahead of a runtime dir holding another `orca`. + const paired = input.cliRuntimeCommand + ? withCliRuntimeOnPath(input.cliRuntimeCommand, input.env, { platform: process.platform }) + : input.env + const identity = childIdentity(input.sessionId, paired) const named = new Set([...Object.keys(identity.commandEnv), ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV]) - const env = input.cliRuntimeCommand - ? withCliRuntimeOnPath(input.cliRuntimeCommand, identity.env, { platform: process.platform }) - : identity.env return { - env: { ...env, [ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV]: input.spawnToken }, + env: { ...identity.env, [ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV]: input.spawnToken }, envToDelete: input.inheritedEnvToDelete.filter((key) => !named.has(key)), commandEnv: identity.commandEnv } diff --git a/src/shared/cli-runtime-pairing-inputs.ts b/src/shared/cli-runtime-pairing-inputs.ts new file mode 100644 index 00000000000..ecabad5fae4 --- /dev/null +++ b/src/shared/cli-runtime-pairing-inputs.ts @@ -0,0 +1,43 @@ +import { closeSync, openSync, readSync } from 'node:fs' + +const NATIVE_EXECUTABLE_MAGIC = [ + [0x7f, 0x45, 0x4c, 0x46], // ELF + [0xcf, 0xfa, 0xed, 0xfe], // Mach-O 64-bit + [0xce, 0xfa, 0xed, 0xfe], // Mach-O 32-bit + [0xca, 0xfe, 0xba, 0xbe], // Mach-O universal + [0x4d, 0x5a] // Windows PE +] + +/** A compiled binary runs on no `node`, so pairing would only reorder PATH (Homebrew, /usr/bin). */ +export function isNativeExecutable(commandPath: string): boolean { + let fd: number | null = null + try { + fd = openSync(commandPath, 'r') + const head = Buffer.alloc(4) + const read = readSync(fd, head, 0, head.length, 0) + return NATIVE_EXECUTABLE_MAGIC.some( + (magic) => read >= magic.length && magic.every((byte, index) => head[index] === byte) + ) + } catch { + return false + } finally { + if (fd !== null) { + closeSync(fd) + } + } +} + +// 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 withCliRuntimeOnPath's twin dedupe deletes would destroy a third spelling unread. +// Mirrors resolvePathEnvKey in src/main/pty/windows-path-segment-merge.ts, which +// src/shared must not import. +export function firstWindowsPathEnvKey(env: NodeJS.ProcessEnv): string { + for (const key of Object.keys(env)) { + if (key.toLowerCase() === 'path' && env[key] !== undefined) { + return key + } + } + return 'Path' +} diff --git a/src/shared/node-cli-command-resolution.ts b/src/shared/node-cli-command-resolution.ts index 040894bfae8..a7446c0a1fe 100644 --- a/src/shared/node-cli-command-resolution.ts +++ b/src/shared/node-cli-command-resolution.ts @@ -1,6 +1,7 @@ import { accessSync, constants, existsSync, readFileSync, readdirSync, statSync } from 'node:fs' import { homedir } from 'node:os' import { delimiter, dirname, isAbsolute, join } from 'node:path' +import { firstWindowsPathEnvKey, isNativeExecutable } from './cli-runtime-pairing-inputs' import { getSystemCliInstallDirectories } from './system-cli-install-dirs' type ResolveCommandOptions = { @@ -329,21 +330,6 @@ 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`. @@ -355,8 +341,8 @@ function firstWindowsPathEnvKey(env: NodeJS.ProcessEnv): string { * 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. + * Only prepends when the sibling `node` really exists and the CLI is not a + * compiled binary, so a native CLI or one beside no node is left alone. */ export function withCliRuntimeOnPath( commandPath: string, @@ -368,12 +354,16 @@ export function withCliRuntimeOnPath( return env } const commandDirectory = dirname(commandPath) - if (!findFirstExecutable(platform, [commandDirectory], getExecutableNames(platform, 'node'))) { + if ( + !findFirstExecutable(platform, [commandDirectory], getExecutableNames(platform, 'node')) || + isNativeExecutable(commandPath) + ) { return env } const pathKey = platform === 'win32' ? firstWindowsPathEnvKey(env) : 'PATH' const pathDelimiter = platform === 'win32' ? ';' : delimiter - const segments = splitPath(env[pathKey], pathDelimiter) + // Why: the child inherits Orca's PATH when `env` names none, so pair over that, not over nothing. + const segments = splitPath(env[pathKey] ?? process.env[pathKey], pathDelimiter) if (segments[0] === commandDirectory) { return env }