From 3f37e6d7a85239261cb0a8d9f326da9c7fac4dfd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 27 Jun 2026 23:09:15 -0700 Subject: [PATCH 1/3] fix: resolve SSH commit/PR agents through login+interactive shell PATH OpenCode and other agents installed via shell startup files (nvm/fnm/ Homebrew) could not be found when generating commit/PR messages over SSH. The relay runs on the remote host via a non-interactive SSH command whose PATH lacks those install dirs, so the agent binary was not found and generation failed. Opt the SSH commit/PR-generation spawn into login+interactive shell (bash/zsh -ilc) PATH resolution, matching the established pattern in hydrate-shell-path.ts and preflight-handler.ts. The interactive flag is required so .bashrc/.zshrc PATH hooks get sourced. Windows and non-bash/zsh shells are unchanged. Fixes #4720 Supersedes #5197. Co-authored-by: Andy Murray Co-authored-by: Orca --- src/main/providers/ssh-git-provider.test.ts | 9 ++- src/main/providers/ssh-git-provider.ts | 6 +- src/relay/agent-exec-handler.test.ts | 63 ++++++++++++++++++++- src/relay/agent-exec-handler.ts | 32 ++++++++++- 4 files changed, 103 insertions(+), 7 deletions(-) diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 01736fb2defb..59d148634229 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -382,7 +382,8 @@ describe('SshGitProvider', () => { cwd: '/home/user/repo', stdin: null, timeoutMs: 60_000, - operation: 'commit-message' + operation: 'commit-message', + shell: true }) expect(result).toEqual(execResult) }) @@ -426,7 +427,8 @@ describe('SshGitProvider', () => { cwd: '/home/user/repo', stdin: null, timeoutMs: 60_000, - operation: 'commit-message' + operation: 'commit-message', + shell: true }) expect(mux.request).toHaveBeenNthCalledWith(2, 'agent.execNonInteractive', { binary: 'codex', @@ -434,7 +436,8 @@ describe('SshGitProvider', () => { cwd: '/home/user/repo', stdin: null, timeoutMs: 60_000, - operation: 'pull-request-fields' + operation: 'pull-request-fields', + shell: true }) await provider.cancelGenerateCommitMessage('/home/user/repo') diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 751ebc1ef7cd..5cea9d2d1bd4 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -192,7 +192,10 @@ export class SshGitProvider implements IGitProvider { cwd, stdin: plan.stdinPayload, timeoutMs, - operation + operation, + // Why: commit/PR agents on SSH hosts are often installed by shell + // startup files, so this path opts into login-shell PATH resolution. + shell: true }, undefined, operation @@ -265,6 +268,7 @@ export class SshGitProvider implements IGitProvider { timeoutMs: number env?: Record operation?: string + shell?: boolean }, signal?: AbortSignal, operation?: string diff --git a/src/relay/agent-exec-handler.test.ts b/src/relay/agent-exec-handler.test.ts index f36c99105cce..d89e8739dc03 100644 --- a/src/relay/agent-exec-handler.test.ts +++ b/src/relay/agent-exec-handler.test.ts @@ -1,7 +1,12 @@ import { exec, spawn } from 'child_process' import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as ChildProcess from 'child_process' -import { createFakeChild, createHandlers, requestContext } from './agent-exec-handler-test-harness' +import { + createFakeChild, + createHandlers, + requestContext, + withPlatform +} from './agent-exec-handler-test-harness' vi.mock('child_process', async (importOriginal) => { const actual = await importOriginal() @@ -97,6 +102,62 @@ describe('AgentExecHandler', () => { }) }) + it('uses the default POSIX shell when requested so SSH agent commands inherit shell PATH', async () => { + const originalShell = process.env.SHELL + process.env.SHELL = '/bin/bash' + try { + await withPlatform('linux', async () => { + const child = createFakeChild() + spawnMock.mockReturnValue(child as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run', '--model', 'opencode/deepseek-v4-flash-free'], + cwd: '/repo', + stdin: 'PROMPT', + timeoutMs: 5_000, + shell: true + }, + requestContext() + ) + + child.emit('close', 0) + + await expect(pending).resolves.toMatchObject({ + exitCode: 0, + timedOut: false + }) + expect(spawnMock).toHaveBeenCalledWith( + '/bin/bash', + [ + '-ilc', + 'exec "$@"', + '_', + 'opencode', + 'run', + '--model', + 'opencode/deepseek-v4-flash-free' + ], + { + cwd: '/repo', + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + } + ) + expect(child.stdin.end).toHaveBeenCalledWith('PROMPT') + }) + } finally { + if (originalShell === undefined) { + delete process.env.SHELL + } else { + process.env.SHELL = originalShell + } + } + }) + it('cancels the in-flight command for the requested cwd', async () => { const child = createFakeChild() spawnMock.mockReturnValue(child as never) diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index 3aab57967e1f..baeee3936c1f 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -1,7 +1,8 @@ import { exec, spawn, type ChildProcess } from 'child_process' import { existsSync } from 'fs' -import { delimiter, join } from 'path' +import { basename, delimiter, join } from 'path' import type { RelayDispatcher, RequestContext } from './dispatcher' +import { resolveDefaultShell } from './pty-shell-utils' const DEFAULT_TIMEOUT_MS = 60_000 const MAX_TIMEOUT_MS = 5 * 60 * 1000 @@ -64,6 +65,31 @@ function getWindowsSafeSpawn( return { spawnCmd: getCmdExePath(), spawnArgs: ['/d', '/s', '/c', commandLine] } } +function getSpawnPlan( + binary: string, + args: string[], + env: NodeJS.ProcessEnv, + useShell: boolean +): { spawnCmd: string; spawnArgs: string[] } { + if (process.platform === 'win32' || !useShell) { + return getWindowsSafeSpawn(binary, args, env) + } + const shell = resolveDefaultShell() + const shellName = basename(shell).toLowerCase() + if (shellName !== 'bash' && shellName !== 'zsh') { + return { spawnCmd: binary, spawnArgs: args } + } + // Why: SSH relay processes are launched by a non-interactive SSH command, + // whose PATH often lacks nvm/fnm/Homebrew agent installs. The interactive + // flag (`-i`) is required so .bashrc/.zshrc — where those PATH hooks live — + // get sourced; a plain login shell (`-lc`) would skip them. This resolves + // remote agent commands like the user's Orca terminal shell does. + return { + spawnCmd: shell, + spawnArgs: ['-ilc', 'exec "$@"', '_', binary, ...args] + } +} + // Why: mirrors src/main/text-generation/commit-message-text-generation.ts. On // Windows, npm-installed CLIs like `claude`/`codex` are usually `.cmd` shims. // We route those through cmd.exe so Node can launch them, and taskkill is @@ -95,6 +121,7 @@ type ExecParams = { timeoutMs: unknown env: unknown operation: unknown + shell: unknown } type CancelParams = { @@ -169,11 +196,12 @@ export class AgentExecHandler { ? (params.env as Record) : null const spawnEnv = extraEnv ? { ...process.env, ...extraEnv } : process.env + const useShell = params.shell === true return new Promise((resolve) => { let child try { - const { spawnCmd, spawnArgs } = getWindowsSafeSpawn(binary, args, spawnEnv) + const { spawnCmd, spawnArgs } = getSpawnPlan(binary, args, spawnEnv, useShell) child = spawn(spawnCmd, spawnArgs, { cwd, env: spawnEnv, From 53ed94dde9e8d7f585248f9d5dc800057aa67ba1 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 29 Jun 2026 17:38:02 -0700 Subject: [PATCH 2/3] fix: narrow SSH agent shell PATH fallback --- src/main/providers/ssh-git-provider.ts | 2 +- src/relay/agent-exec-handler.test.ts | 193 +++++++++++++++++++++++-- src/relay/agent-exec-handler.ts | 156 +++++++------------- src/relay/agent-exec-spawn-plan.ts | 134 +++++++++++++++++ 4 files changed, 368 insertions(+), 117 deletions(-) create mode 100644 src/relay/agent-exec-spawn-plan.ts diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 5cea9d2d1bd4..612115b28e1a 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -194,7 +194,7 @@ export class SshGitProvider implements IGitProvider { timeoutMs, operation, // Why: commit/PR agents on SSH hosts are often installed by shell - // startup files, so this path opts into login-shell PATH resolution. + // startup files, so this path allows relay-side shell PATH fallback. shell: true }, undefined, diff --git a/src/relay/agent-exec-handler.test.ts b/src/relay/agent-exec-handler.test.ts index d89e8739dc03..b369c630ab9c 100644 --- a/src/relay/agent-exec-handler.test.ts +++ b/src/relay/agent-exec-handler.test.ts @@ -1,6 +1,20 @@ import { exec, spawn } from 'child_process' import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as ChildProcess from 'child_process' +import type * as Os from 'os' + +const { userInfoMock } = vi.hoisted(() => ({ + userInfoMock: vi.fn<() => { shell: string | null }>(() => ({ shell: null })) +})) + +vi.mock('os', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + userInfo: userInfoMock + } +}) + import { createFakeChild, createHandlers, @@ -22,10 +36,30 @@ const execMock = vi.mocked(exec) type AgentExecResult = { exitCode: number | null; timedOut: boolean } +async function withShell(shell: string | undefined, fn: () => Promise): Promise { + const originalShell = process.env.SHELL + if (shell === undefined) { + delete process.env.SHELL + } else { + process.env.SHELL = shell + } + try { + return await fn() + } finally { + if (originalShell === undefined) { + delete process.env.SHELL + } else { + process.env.SHELL = originalShell + } + } +} + describe('AgentExecHandler', () => { beforeEach(() => { spawnMock.mockReset() execMock.mockReset() + userInfoMock.mockReset() + userInfoMock.mockReturnValue({ shell: null }) }) it('executes a non-interactive command with captured output and stdin', async () => { @@ -102,10 +136,8 @@ describe('AgentExecHandler', () => { }) }) - it('uses the default POSIX shell when requested so SSH agent commands inherit shell PATH', async () => { - const originalShell = process.env.SHELL - process.env.SHELL = '/bin/bash' - try { + it('tries the inherited PATH before shell fallback when shell PATH resolution is requested', async () => { + await withShell('/bin/bash', async () => { await withPlatform('linux', async () => { const child = createFakeChild() spawnMock.mockReturnValue(child as never) @@ -114,7 +146,7 @@ describe('AgentExecHandler', () => { const pending = handlers.get('agent.execNonInteractive')!( { binary: 'opencode', - args: ['run', '--model', 'opencode/deepseek-v4-flash-free'], + args: ['run'], cwd: '/repo', stdin: 'PROMPT', timeoutMs: 5_000, @@ -129,7 +161,61 @@ describe('AgentExecHandler', () => { exitCode: 0, timedOut: false }) - expect(spawnMock).toHaveBeenCalledWith( + expect(spawnMock).toHaveBeenCalledWith('opencode', ['run'], { + cwd: '/repo', + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + }) + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(child.stdin.end).toHaveBeenCalledWith('PROMPT') + }) + }) + }) + + it('falls back to the explicit POSIX shell when the inherited PATH misses the agent', async () => { + await withShell('/bin/bash', async () => { + await withPlatform('linux', async () => { + const directChild = createFakeChild() + const shellChild = createFakeChild() + spawnMock.mockReturnValueOnce(directChild as never).mockReturnValueOnce(shellChild as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run', '--model', 'opencode/deepseek-v4-flash-free'], + cwd: '/repo', + stdin: 'PROMPT', + timeoutMs: 5_000, + shell: true + }, + requestContext() + ) + + directChild.emit( + 'error', + Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) + ) + shellChild.emit('close', 0) + + await expect(pending).resolves.toMatchObject({ + exitCode: 0, + timedOut: false + }) + expect(spawnMock).toHaveBeenNthCalledWith( + 1, + 'opencode', + ['run', '--model', 'opencode/deepseek-v4-flash-free'], + { + cwd: '/repo', + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + } + ) + expect(spawnMock).toHaveBeenNthCalledWith( + 2, '/bin/bash', [ '-ilc', @@ -147,14 +233,95 @@ describe('AgentExecHandler', () => { windowsHide: true } ) - expect(child.stdin.end).toHaveBeenCalledWith('PROMPT') + expect(directChild.stdin.end).toHaveBeenCalledWith('PROMPT') + expect(shellChild.stdin.end).toHaveBeenCalledWith('PROMPT') }) - } finally { - if (originalShell === undefined) { - delete process.env.SHELL - } else { - process.env.SHELL = originalShell - } + }) + }) + + it('falls back to the account login shell when SHELL is unset', async () => { + userInfoMock.mockReturnValue({ shell: '/bin/zsh' }) + await withShell(undefined, async () => { + await withPlatform('linux', async () => { + const directChild = createFakeChild() + const shellChild = createFakeChild() + spawnMock.mockReturnValueOnce(directChild as never).mockReturnValueOnce(shellChild as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run'], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + shell: true + }, + requestContext() + ) + + directChild.emit( + 'error', + Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) + ) + shellChild.emit('close', 0) + + await expect(pending).resolves.toMatchObject({ + exitCode: 0, + timedOut: false + }) + expect(spawnMock).toHaveBeenNthCalledWith( + 2, + '/bin/zsh', + ['-ilc', 'exec "$@"', '_', 'opencode', 'run'], + { + cwd: '/repo', + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + } + ) + }) + }) + }) + + it('does not run shell fallback when the configured shell is missing or unsupported', async () => { + for (const shell of [undefined, '/usr/bin/fish']) { + await withShell(shell, async () => { + await withPlatform('linux', async () => { + const child = createFakeChild() + spawnMock.mockReturnValue(child as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run'], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + shell: true + }, + requestContext() + ) + + child.emit('error', Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' })) + + await expect(pending).resolves.toMatchObject({ + exitCode: null, + timedOut: false, + spawnError: 'spawn opencode ENOENT' + }) + expect(spawnMock).toHaveBeenCalledWith('opencode', ['run'], { + cwd: '/repo', + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + }) + expect(spawnMock).toHaveBeenCalledTimes(1) + }) + }) + spawnMock.mockReset() } }) diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index baeee3936c1f..f06d8dd620ef 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -1,94 +1,10 @@ import { exec, spawn, type ChildProcess } from 'child_process' -import { existsSync } from 'fs' -import { basename, delimiter, join } from 'path' import type { RelayDispatcher, RequestContext } from './dispatcher' -import { resolveDefaultShell } from './pty-shell-utils' +import { getSpawnPlan, isSpawnEnoent, type SpawnCommand } from './agent-exec-spawn-plan' const DEFAULT_TIMEOUT_MS = 60_000 const MAX_TIMEOUT_MS = 5 * 60 * 1000 const MAX_OUTPUT_BYTES = 4 * 1024 * 1024 -const WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR = 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' - -function getCmdExePath(): string { - return process.env.ComSpec || `${process.env.SystemRoot ?? 'C:\\Windows'}\\System32\\cmd.exe` -} - -function isWindowsBatchScript(commandPath: string): boolean { - return process.platform === 'win32' && /\.(cmd|bat)$/i.test(commandPath) -} - -function hasUnsafeWindowsBatchSyntax(value: string): boolean { - return /[&|<>^"%!\r\n]/.test(value) -} - -function quoteWindowsBatchToken(value: string): string { - if (hasUnsafeWindowsBatchSyntax(value)) { - throw new Error(WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR) - } - return `"${value}"` -} - -function resolveWindowsCommand(binary: string, env: NodeJS.ProcessEnv): string { - if (process.platform !== 'win32') { - return binary - } - if (/[\\/]/.test(binary) || /\.[a-z0-9]+$/i.test(binary)) { - return binary - } - - const pathEnv = env.PATH ?? env.Path - if (!pathEnv) { - return binary - } - const names = [`${binary}.cmd`, `${binary}.exe`, `${binary}.bat`, binary] - for (const directory of pathEnv.split(delimiter).filter(Boolean)) { - for (const name of names) { - const candidate = join(directory, name) - if (existsSync(candidate)) { - return candidate - } - } - } - return binary -} - -function getWindowsSafeSpawn( - binary: string, - args: string[], - env: NodeJS.ProcessEnv -): { spawnCmd: string; spawnArgs: string[] } { - const resolvedBinary = resolveWindowsCommand(binary, env) - if (!isWindowsBatchScript(resolvedBinary)) { - return { spawnCmd: resolvedBinary, spawnArgs: args } - } - const commandLine = [resolvedBinary, ...args].map(quoteWindowsBatchToken).join(' ') - return { spawnCmd: getCmdExePath(), spawnArgs: ['/d', '/s', '/c', commandLine] } -} - -function getSpawnPlan( - binary: string, - args: string[], - env: NodeJS.ProcessEnv, - useShell: boolean -): { spawnCmd: string; spawnArgs: string[] } { - if (process.platform === 'win32' || !useShell) { - return getWindowsSafeSpawn(binary, args, env) - } - const shell = resolveDefaultShell() - const shellName = basename(shell).toLowerCase() - if (shellName !== 'bash' && shellName !== 'zsh') { - return { spawnCmd: binary, spawnArgs: args } - } - // Why: SSH relay processes are launched by a non-interactive SSH command, - // whose PATH often lacks nvm/fnm/Homebrew agent installs. The interactive - // flag (`-i`) is required so .bashrc/.zshrc — where those PATH hooks live — - // get sourced; a plain login shell (`-lc`) would skip them. This resolves - // remote agent commands like the user's Orca terminal shell does. - return { - spawnCmd: shell, - spawnArgs: ['-ilc', 'exec "$@"', '_', binary, ...args] - } -} // Why: mirrors src/main/text-generation/commit-message-text-generation.ts. On // Windows, npm-installed CLIs like `claude`/`codex` are usually `.cmd` shims. @@ -199,15 +115,20 @@ export class AgentExecHandler { const useShell = params.shell === true return new Promise((resolve) => { - let child - try { - const { spawnCmd, spawnArgs } = getSpawnPlan(binary, args, spawnEnv, useShell) - child = spawn(spawnCmd, spawnArgs, { + const spawnPlannedChild = (plan: SpawnCommand): ChildProcess => + spawn(plan.spawnCmd, plan.spawnArgs, { cwd, env: spawnEnv, stdio: ['pipe', 'pipe', 'pipe'], windowsHide: true }) + + let child: ChildProcess + let shellFallback: SpawnCommand | undefined + try { + const spawnPlan = getSpawnPlan(binary, args, spawnEnv, useShell) + shellFallback = spawnPlan.shellFallback + child = spawnPlannedChild(spawnPlan) } catch (error) { resolve({ stdout: '', @@ -226,6 +147,7 @@ export class AgentExecHandler { let timedOut = false let canceled = false let settled = false + let shellFallbackUsed = false const laneKey = typeof cwd === 'string' ? this.laneKey(cwd, params.operation) : '' let entry: InFlightExec | null = null let timer: ReturnType | null = null @@ -247,6 +169,13 @@ export class AgentExecHandler { } resolve(result) } + const sendStdin = (): void => { + if (stdinPayload !== null) { + child.stdin?.end(stdinPayload) + } else { + child.stdin?.end() + } + } const cancelCurrent = (): void => { canceled = true killProcessTree(child) @@ -290,6 +219,28 @@ export class AgentExecHandler { stderr += chunk.toString('utf-8') } const onError = (error: Error): void => { + if (!settled && !canceled && !shellFallbackUsed && shellFallback && isSpawnEnoent(error)) { + shellFallbackUsed = true + detachChildListeners() + try { + child = spawnPlannedChild(shellFallback) + if (entry) { + entry.child = child + } + attachChildListeners() + sendStdin() + } catch (fallbackError) { + finish({ + stdout, + stderr, + exitCode: null, + timedOut, + spawnError: + fallbackError instanceof Error ? fallbackError.message : String(fallbackError) + }) + } + return + } finish({ stdout, stderr, @@ -301,16 +252,19 @@ export class AgentExecHandler { const onClose = (code: number | null): void => { finish({ stdout, stderr, exitCode: code, timedOut, canceled }) } - child.stdout?.on('data', onStdoutData) - child.stderr?.on('data', onStderrData) - child.on('error', onError) - child.on('close', onClose) - detachChildListeners = () => { - child.stdout?.off('data', onStdoutData) - child.stderr?.off('data', onStderrData) - child.off('error', onError) - child.off('close', onClose) + function attachChildListeners(): void { + child.stdout?.on('data', onStdoutData) + child.stderr?.on('data', onStderrData) + child.on('error', onError) + child.on('close', onClose) + detachChildListeners = () => { + child.stdout?.off('data', onStdoutData) + child.stderr?.off('data', onStderrData) + child.off('error', onError) + child.off('close', onClose) + } } + attachChildListeners() if (context?.signal) { if (context.signal.aborted) { @@ -323,11 +277,7 @@ export class AgentExecHandler { } } - if (stdinPayload !== null) { - child.stdin?.end(stdinPayload) - } else { - child.stdin?.end() - } + sendStdin() }) } } diff --git a/src/relay/agent-exec-spawn-plan.ts b/src/relay/agent-exec-spawn-plan.ts new file mode 100644 index 000000000000..531cd2071c60 --- /dev/null +++ b/src/relay/agent-exec-spawn-plan.ts @@ -0,0 +1,134 @@ +import { existsSync } from 'fs' +import { userInfo } from 'os' +import { delimiter, join, posix } from 'path' + +export const WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR = 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' + +export type SpawnCommand = { spawnCmd: string; spawnArgs: string[] } + +type SpawnPlan = SpawnCommand & { shellFallback?: SpawnCommand } + +function getCmdExePath(): string { + return process.env.ComSpec || `${process.env.SystemRoot ?? 'C:\\Windows'}\\System32\\cmd.exe` +} + +function isWindowsBatchScript(commandPath: string): boolean { + return process.platform === 'win32' && /\.(cmd|bat)$/i.test(commandPath) +} + +function hasUnsafeWindowsBatchSyntax(value: string): boolean { + return /[&|<>^"%!\r\n]/.test(value) +} + +function quoteWindowsBatchToken(value: string): string { + if (hasUnsafeWindowsBatchSyntax(value)) { + throw new Error(WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR) + } + return `"${value}"` +} + +function resolveWindowsCommand(binary: string, env: NodeJS.ProcessEnv): string { + if (process.platform !== 'win32') { + return binary + } + if (/[\\/]/.test(binary) || /\.[a-z0-9]+$/i.test(binary)) { + return binary + } + + const pathEnv = env.PATH ?? env.Path + if (!pathEnv) { + return binary + } + const names = [`${binary}.cmd`, `${binary}.exe`, `${binary}.bat`, binary] + for (const directory of pathEnv.split(delimiter).filter(Boolean)) { + for (const name of names) { + const candidate = join(directory, name) + if (existsSync(candidate)) { + return candidate + } + } + } + return binary +} + +function getWindowsSafeSpawn(binary: string, args: string[], env: NodeJS.ProcessEnv): SpawnCommand { + const resolvedBinary = resolveWindowsCommand(binary, env) + if (!isWindowsBatchScript(resolvedBinary)) { + return { spawnCmd: resolvedBinary, spawnArgs: args } + } + const commandLine = [resolvedBinary, ...args].map(quoteWindowsBatchToken).join(' ') + return { spawnCmd: getCmdExePath(), spawnArgs: ['/d', '/s', '/c', commandLine] } +} + +function normalizeInteractiveShell(shell: unknown): string | null { + if (typeof shell !== 'string') { + return null + } + if (!shell || !posix.isAbsolute(shell)) { + return null + } + const shellName = posix.basename(shell).toLowerCase() + return shellName === 'bash' || shellName === 'zsh' ? shell : null +} + +function getAccountLoginShell(): string | null { + try { + return normalizeInteractiveShell(userInfo().shell) + } catch { + return null + } +} + +function resolveRequestedInteractiveShell(env: NodeJS.ProcessEnv): string | null { + const envShell = typeof env.SHELL === 'string' ? env.SHELL : '' + if (envShell) { + return normalizeInteractiveShell(envShell) + } + return getAccountLoginShell() +} + +function buildInteractiveShellFallback( + binary: string, + args: string[], + env: NodeJS.ProcessEnv +): SpawnCommand | null { + if (binary.includes('/')) { + return null + } + const shell = resolveRequestedInteractiveShell(env) + if (!shell) { + return null + } + // Why: SSH relay processes are launched by a non-interactive SSH command, + // whose PATH often lacks nvm/fnm/Homebrew agent installs. Use the user's + // explicit bash/zsh only as an ENOENT fallback so normal PATH hits stay fast. + return { + spawnCmd: shell, + spawnArgs: ['-ilc', 'exec "$@"', '_', binary, ...args] + } +} + +export function getSpawnPlan( + binary: string, + args: string[], + env: NodeJS.ProcessEnv, + useShell: boolean +): SpawnPlan { + if (process.platform === 'win32' || !useShell) { + return getWindowsSafeSpawn(binary, args, env) + } + const shellFallback = buildInteractiveShellFallback(binary, args, env) + if (!shellFallback) { + return { spawnCmd: binary, spawnArgs: args } + } + return { + spawnCmd: binary, + spawnArgs: args, + shellFallback + } +} + +export function isSpawnEnoent(error: Error): boolean { + const code = (error as NodeJS.ErrnoException).code + return code === 'ENOENT' || /\bENOENT\b/i.test(error.message) +} From 93ac20babe20d5eac6a95233607b69994aaccc59 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 29 Jun 2026 18:08:14 -0700 Subject: [PATCH 3/3] fix: resolve SSH agent paths before fallback spawn --- src/relay/agent-exec-handler-test-harness.ts | 25 +- src/relay/agent-exec-handler.test.ts | 219 +++++++++++++--- src/relay/agent-exec-handler.ts | 142 +++++----- src/relay/agent-exec-process-tree.ts | 24 ++ src/relay/agent-exec-rpc-types.ts | 30 +++ src/relay/agent-exec-spawn-plan.ts | 61 +---- src/relay/preflight-handler.test.ts | 82 +++++- src/relay/preflight-handler.ts | 187 +------------ src/relay/relay-command-path-lookup.ts | 261 +++++++++++++++++++ 9 files changed, 700 insertions(+), 331 deletions(-) create mode 100644 src/relay/agent-exec-process-tree.ts create mode 100644 src/relay/agent-exec-rpc-types.ts create mode 100644 src/relay/relay-command-path-lookup.ts diff --git a/src/relay/agent-exec-handler-test-harness.ts b/src/relay/agent-exec-handler-test-harness.ts index 7fe94d6c8370..84a1ab95fdd2 100644 --- a/src/relay/agent-exec-handler-test-harness.ts +++ b/src/relay/agent-exec-handler-test-harness.ts @@ -3,13 +3,34 @@ import { vi } from 'vitest' import type { MethodHandler, RequestContext } from './dispatcher' import { AgentExecHandler } from './agent-exec-handler' +function hasFinally(value: unknown): value is Promise { + return ( + typeof value === 'object' && + value !== null && + 'finally' in value && + typeof value.finally === 'function' + ) +} + export function withPlatform(platform: NodeJS.Platform, fn: () => T): T { const original = process.platform Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + let restoreImmediately = true + const restore = (): void => { + Object.defineProperty(process, 'platform', { configurable: true, value: original }) + } try { - return fn() + const result = fn() + if (hasFinally(result)) { + restoreImmediately = false + return result.finally(restore) as T + } + restore() + return result } finally { - Object.defineProperty(process, 'platform', { configurable: true, value: original }) + if (restoreImmediately && process.platform === platform) { + restore() + } } } diff --git a/src/relay/agent-exec-handler.test.ts b/src/relay/agent-exec-handler.test.ts index b369c630ab9c..fc070ab9834d 100644 --- a/src/relay/agent-exec-handler.test.ts +++ b/src/relay/agent-exec-handler.test.ts @@ -3,7 +3,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as ChildProcess from 'child_process' import type * as Os from 'os' -const { userInfoMock } = vi.hoisted(() => ({ +const { execFileAsyncMock, userInfoMock } = vi.hoisted(() => ({ + execFileAsyncMock: vi.fn(), userInfoMock: vi.fn<() => { shell: string | null }>(() => ({ shell: null })) })) @@ -21,12 +22,20 @@ import { requestContext, withPlatform } from './agent-exec-handler-test-harness' +import { + _resetRelayCommandPathCacheForTests, + resolveCommandLaunchForRelay +} from './relay-command-path-lookup' vi.mock('child_process', async (importOriginal) => { const actual = await importOriginal() + const execFileWithPromisify = Object.assign(vi.fn(), { + [Symbol.for('nodejs.util.promisify.custom')]: execFileAsyncMock + }) return { ...actual, exec: vi.fn(), + execFile: execFileWithPromisify, spawn: vi.fn() } }) @@ -58,6 +67,8 @@ describe('AgentExecHandler', () => { beforeEach(() => { spawnMock.mockReset() execMock.mockReset() + execFileAsyncMock.mockReset() + _resetRelayCommandPathCacheForTests() userInfoMock.mockReset() userInfoMock.mockReturnValue({ shell: null }) }) @@ -173,12 +184,19 @@ describe('AgentExecHandler', () => { }) }) - it('falls back to the explicit POSIX shell when the inherited PATH misses the agent', async () => { + it('resolves through the explicit POSIX shell when inherited PATH misses the agent', async () => { await withShell('/bin/bash', async () => { await withPlatform('linux', async () => { const directChild = createFakeChild() - const shellChild = createFakeChild() - spawnMock.mockReturnValueOnce(directChild as never).mockReturnValueOnce(shellChild as never) + const resolvedChild = createFakeChild() + spawnMock + .mockReturnValueOnce(directChild as never) + .mockReturnValueOnce(resolvedChild as never) + execFileAsyncMock.mockResolvedValueOnce({ + stdout: + '__ORCA_AGENT_PATH__/home/test/.nvm/versions/node/v22/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/home/test/.nvm/versions/node/v22/bin:/usr/bin\n' + }) const handlers = createHandlers() const pending = handlers.get('agent.execNonInteractive')!( @@ -197,7 +215,8 @@ describe('AgentExecHandler', () => { 'error', Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) ) - shellChild.emit('close', 0) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) + resolvedChild.emit('close', 0) await expect(pending).resolves.toMatchObject({ exitCode: 0, @@ -216,36 +235,55 @@ describe('AgentExecHandler', () => { ) expect(spawnMock).toHaveBeenNthCalledWith( 2, + '/home/test/.nvm/versions/node/v22/bin/opencode', + ['run', '--model', 'opencode/deepseek-v4-flash-free'], + { + cwd: '/repo', + env: expect.objectContaining({ + ...process.env, + PATH: '/home/test/.nvm/versions/node/v22/bin:/usr/bin' + }), + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + } + ) + expect(execFileAsyncMock).toHaveBeenCalledWith( '/bin/bash', [ '-ilc', - 'exec "$@"', - '_', - 'opencode', - 'run', - '--model', - 'opencode/deepseek-v4-flash-free' + [ + "if resolved=$(command -v 'opencode' 2>/dev/null); then", + 'printf \'__ORCA_AGENT_PATH__%s\\n\' "$resolved"', + 'printf \'__ORCA_AGENT_ENV_PATH__%s\\n\' "$PATH"', + 'fi' + ].join('\n') ], { - cwd: '/repo', - env: process.env, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true + encoding: 'utf-8', + env: expect.objectContaining({ SHELL: '/bin/bash' }), + timeout: 5000 } ) expect(directChild.stdin.end).toHaveBeenCalledWith('PROMPT') - expect(shellChild.stdin.end).toHaveBeenCalledWith('PROMPT') + expect(resolvedChild.stdin.end).toHaveBeenCalledWith('PROMPT') }) }) }) - it('falls back to the account login shell when SHELL is unset', async () => { + it('resolves through the account login shell when SHELL is unset', async () => { userInfoMock.mockReturnValue({ shell: '/bin/zsh' }) await withShell(undefined, async () => { await withPlatform('linux', async () => { const directChild = createFakeChild() - const shellChild = createFakeChild() - spawnMock.mockReturnValueOnce(directChild as never).mockReturnValueOnce(shellChild as never) + const resolvedChild = createFakeChild() + spawnMock + .mockReturnValueOnce(directChild as never) + .mockReturnValueOnce(resolvedChild as never) + execFileAsyncMock.mockResolvedValueOnce({ + stdout: + '__ORCA_AGENT_PATH__/Users/test/.local/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/Users/test/.local/bin:/usr/bin\n' + }) const handlers = createHandlers() const pending = handlers.get('agent.execNonInteractive')!( @@ -264,23 +302,30 @@ describe('AgentExecHandler', () => { 'error', Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) ) - shellChild.emit('close', 0) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) + resolvedChild.emit('close', 0) await expect(pending).resolves.toMatchObject({ exitCode: 0, timedOut: false }) - expect(spawnMock).toHaveBeenNthCalledWith( - 2, + expect(execFileAsyncMock).toHaveBeenCalledWith( '/bin/zsh', - ['-ilc', 'exec "$@"', '_', 'opencode', 'run'], - { - cwd: '/repo', - env: process.env, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true - } + expect.arrayContaining(['-ilc']), + expect.objectContaining({ + encoding: 'utf-8', + timeout: 5000 + }) ) + expect(spawnMock).toHaveBeenNthCalledWith(2, '/Users/test/.local/bin/opencode', ['run'], { + cwd: '/repo', + env: expect.objectContaining({ + ...process.env, + PATH: '/Users/test/.local/bin:/usr/bin' + }), + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + }) }) }) }) @@ -319,12 +364,128 @@ describe('AgentExecHandler', () => { windowsHide: true }) expect(spawnMock).toHaveBeenCalledTimes(1) + expect(execFileAsyncMock).not.toHaveBeenCalled() }) }) spawnMock.mockReset() + execFileAsyncMock.mockReset() } }) + it('uses a preflight-cached path without a generation-time shell lookup', async () => { + await withShell('/usr/bin/fish', async () => { + await withPlatform('linux', async () => { + const env = { SHELL: '/usr/bin/fish', PATH: '/usr/bin' } + execFileAsyncMock.mockResolvedValueOnce({ + stdout: + '__ORCA_AGENT_PATH__/home/test/.local/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/home/test/.local/bin:/usr/bin\n' + }) + + await resolveCommandLaunchForRelay('opencode', { + platform: 'linux', + env, + accountLoginShell: null + }) + execFileAsyncMock.mockClear() + + const directChild = createFakeChild() + const resolvedChild = createFakeChild() + spawnMock + .mockReturnValueOnce(directChild as never) + .mockReturnValueOnce(resolvedChild as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run'], + cwd: '/repo', + stdin: 'PROMPT', + timeoutMs: 5_000, + env, + shell: true + }, + requestContext() + ) + + directChild.emit( + 'error', + Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) + ) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) + resolvedChild.emit('close', 0) + + await expect(pending).resolves.toMatchObject({ + exitCode: 0, + timedOut: false + }) + expect(execFileAsyncMock).not.toHaveBeenCalled() + expect(spawnMock).toHaveBeenNthCalledWith(2, '/home/test/.local/bin/opencode', ['run'], { + cwd: '/repo', + env: expect.objectContaining({ + ...process.env, + PATH: '/home/test/.local/bin:/usr/bin', + SHELL: '/usr/bin/fish' + }), + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + }) + }) + }) + }) + + it('cancels while resolving the fallback path without spawning the resolved agent', async () => { + await withShell('/bin/bash', async () => { + await withPlatform('linux', async () => { + let resolveLookup!: (value: { stdout: string }) => void + execFileAsyncMock.mockReturnValueOnce( + new Promise((resolve) => { + resolveLookup = resolve + }) + ) + const directChild = createFakeChild() + spawnMock.mockReturnValueOnce(directChild as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'opencode', + args: ['run'], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + shell: true + }, + requestContext() + ) + + directChild.emit( + 'error', + Object.assign(new Error('spawn opencode ENOENT'), { code: 'ENOENT' }) + ) + await vi.waitFor(() => expect(execFileAsyncMock).toHaveBeenCalledTimes(1)) + + await expect( + handlers.get('agent.cancelExec')!({ cwd: '/repo' }, requestContext()) + ).resolves.toEqual({ canceled: true }) + await expect(pending).resolves.toMatchObject({ + exitCode: null, + timedOut: false, + canceled: true + }) + + resolveLookup({ + stdout: + '__ORCA_AGENT_PATH__/home/test/.local/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/home/test/.local/bin:/usr/bin\n' + }) + await Promise.resolve() + expect(spawnMock).toHaveBeenCalledTimes(1) + }) + }) + }) + it('cancels the in-flight command for the requested cwd', async () => { const child = createFakeChild() spawnMock.mockReturnValue(child as never) diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index f06d8dd620ef..5bc5814841e2 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -1,68 +1,19 @@ -import { exec, spawn, type ChildProcess } from 'child_process' +import { spawn, type ChildProcess } from 'child_process' import type { RelayDispatcher, RequestContext } from './dispatcher' import { getSpawnPlan, isSpawnEnoent, type SpawnCommand } from './agent-exec-spawn-plan' +import { resolveCommandLaunchForRelay } from './relay-command-path-lookup' +import { killProcessTree } from './agent-exec-process-tree' +import type { CancelParams, ExecParams, ExecResult, InFlightExec } from './agent-exec-rpc-types' const DEFAULT_TIMEOUT_MS = 60_000 const MAX_TIMEOUT_MS = 5 * 60 * 1000 const MAX_OUTPUT_BYTES = 4 * 1024 * 1024 -// Why: mirrors src/main/text-generation/commit-message-text-generation.ts. On -// Windows, npm-installed CLIs like `claude`/`codex` are usually `.cmd` shims. -// We route those through cmd.exe so Node can launch them, and taskkill is -// needed to terminate the whole wrapper + node.exe process tree. Kept -// duplicated rather than imported because the relay ships to remote hosts. -function killProcessTree(child: ChildProcess): void { - const pid = child.pid - if (!pid) { - return - } - if (process.platform === 'win32') { - exec(`taskkill /pid ${pid} /T /F`, () => { - // Best-effort; the spawn's `close` listener fires once the tree exits. - }) - return - } - try { - child.kill('SIGKILL') - } catch { - // Child may already have exited between the kill request and now. - } -} - -type ExecParams = { - binary: unknown - args: unknown - cwd: unknown - stdin: unknown - timeoutMs: unknown - env: unknown - operation: unknown - shell: unknown -} - -type CancelParams = { - cwd: unknown - operation: unknown -} - function laneKeyFor(cwd: string, operation: unknown): string { const op = typeof operation === 'string' && operation ? operation : 'default' return JSON.stringify([op, cwd]) } -type InFlightExec = { child: ChildProcess; cancel: () => void } - -type ExecResult = { - stdout: string - stderr: string - exitCode: number | null - timedOut: boolean - /** Set when the user canceled the exec via `agent.cancelExec`. */ - canceled?: boolean - /** Set when the binary could not be spawned (e.g. ENOENT). */ - spawnError?: string -} - /** * Non-interactive subprocess exec on the remote host. Used by the AI commit * message generator to spawn agent CLIs (claude, codex, …) with the staged @@ -115,19 +66,22 @@ export class AgentExecHandler { const useShell = params.shell === true return new Promise((resolve) => { - const spawnPlannedChild = (plan: SpawnCommand): ChildProcess => + const spawnPlannedChild = ( + plan: SpawnCommand, + env: NodeJS.ProcessEnv = spawnEnv + ): ChildProcess => spawn(plan.spawnCmd, plan.spawnArgs, { cwd, - env: spawnEnv, + env, stdio: ['pipe', 'pipe', 'pipe'], windowsHide: true }) let child: ChildProcess - let shellFallback: SpawnCommand | undefined + let pathFallbackCommand: string | undefined try { const spawnPlan = getSpawnPlan(binary, args, spawnEnv, useShell) - shellFallback = spawnPlan.shellFallback + pathFallbackCommand = spawnPlan.pathFallbackCommand child = spawnPlannedChild(spawnPlan) } catch (error) { resolve({ @@ -147,7 +101,8 @@ export class AgentExecHandler { let timedOut = false let canceled = false let settled = false - let shellFallbackUsed = false + let pathFallbackUsed = false + let pathFallbackPending = false const laneKey = typeof cwd === 'string' ? this.laneKey(cwd, params.operation) : '' let entry: InFlightExec | null = null let timer: ReturnType | null = null @@ -178,6 +133,10 @@ export class AgentExecHandler { } const cancelCurrent = (): void => { canceled = true + if (pathFallbackPending) { + finish({ stdout, stderr, exitCode: null, timedOut, canceled }) + return + } killProcessTree(child) } if (laneKey) { @@ -219,17 +178,64 @@ export class AgentExecHandler { stderr += chunk.toString('utf-8') } const onError = (error: Error): void => { - if (!settled && !canceled && !shellFallbackUsed && shellFallback && isSpawnEnoent(error)) { - shellFallbackUsed = true + if ( + !settled && + !canceled && + !pathFallbackUsed && + pathFallbackCommand && + isSpawnEnoent(error) + ) { + pathFallbackUsed = true detachChildListeners() - try { - child = spawnPlannedChild(shellFallback) - if (entry) { - entry.child = child + pathFallbackPending = true + void (async () => { + const resolved = await resolveCommandLaunchForRelay(pathFallbackCommand, { + env: spawnEnv, + allowedShellNames: ['bash', 'zsh'], + includeInheritedPathFallback: false + }) + pathFallbackPending = false + if (settled) { + return + } + if (canceled) { + finish({ stdout, stderr, exitCode: null, timedOut, canceled }) + return + } + if (!resolved) { + finish({ + stdout, + stderr, + exitCode: null, + timedOut, + spawnError: error.message + }) + return } - attachChildListeners() - sendStdin() - } catch (fallbackError) { + try { + const fallbackEnv = + resolved.pathEnv === undefined ? spawnEnv : { ...spawnEnv, PATH: resolved.pathEnv } + child = spawnPlannedChild( + { spawnCmd: resolved.commandPath, spawnArgs: args }, + fallbackEnv + ) + if (entry) { + entry.child = child + } + attachChildListeners() + sendStdin() + } catch (fallbackError) { + finish({ + stdout, + stderr, + exitCode: null, + timedOut, + spawnError: + fallbackError instanceof Error ? fallbackError.message : String(fallbackError) + }) + } + })().catch((fallbackError: unknown) => { + pathFallbackPending = false finish({ stdout, stderr, @@ -238,7 +244,7 @@ export class AgentExecHandler { spawnError: fallbackError instanceof Error ? fallbackError.message : String(fallbackError) }) - } + }) return } finish({ diff --git a/src/relay/agent-exec-process-tree.ts b/src/relay/agent-exec-process-tree.ts new file mode 100644 index 000000000000..25f3d46ed5b1 --- /dev/null +++ b/src/relay/agent-exec-process-tree.ts @@ -0,0 +1,24 @@ +import { exec, type ChildProcess } from 'child_process' + +// Why: mirrors src/main/text-generation/commit-message-text-generation.ts. On +// Windows, npm-installed CLIs like `claude`/`codex` are usually `.cmd` shims. +// We route those through cmd.exe so Node can launch them, and taskkill is +// needed to terminate the whole wrapper + node.exe process tree. Kept +// duplicated rather than imported because the relay ships to remote hosts. +export function killProcessTree(child: ChildProcess): void { + const pid = child.pid + if (!pid) { + return + } + if (process.platform === 'win32') { + exec(`taskkill /pid ${pid} /T /F`, () => { + // Best-effort; the spawn's `close` listener fires once the tree exits. + }) + return + } + try { + child.kill('SIGKILL') + } catch { + // Child may already have exited between the kill request and now. + } +} diff --git a/src/relay/agent-exec-rpc-types.ts b/src/relay/agent-exec-rpc-types.ts new file mode 100644 index 000000000000..fd04143139ad --- /dev/null +++ b/src/relay/agent-exec-rpc-types.ts @@ -0,0 +1,30 @@ +import type { ChildProcess } from 'child_process' + +export type ExecParams = { + binary: unknown + args: unknown + cwd: unknown + stdin: unknown + timeoutMs: unknown + env: unknown + operation: unknown + shell: unknown +} + +export type CancelParams = { + cwd: unknown + operation: unknown +} + +export type InFlightExec = { child: ChildProcess; cancel: () => void } + +export type ExecResult = { + stdout: string + stderr: string + exitCode: number | null + timedOut: boolean + /** Set when the user canceled the exec via `agent.cancelExec`. */ + canceled?: boolean + /** Set when the binary could not be spawned (e.g. ENOENT). */ + spawnError?: string +} diff --git a/src/relay/agent-exec-spawn-plan.ts b/src/relay/agent-exec-spawn-plan.ts index 531cd2071c60..58efcbb27ae0 100644 --- a/src/relay/agent-exec-spawn-plan.ts +++ b/src/relay/agent-exec-spawn-plan.ts @@ -1,12 +1,11 @@ import { existsSync } from 'fs' -import { userInfo } from 'os' -import { delimiter, join, posix } from 'path' +import { delimiter, join } from 'path' export const WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR = 'UNSAFE_WINDOWS_BATCH_ARGUMENTS' export type SpawnCommand = { spawnCmd: string; spawnArgs: string[] } -type SpawnPlan = SpawnCommand & { shellFallback?: SpawnCommand } +type SpawnPlan = SpawnCommand & { pathFallbackCommand?: string } function getCmdExePath(): string { return process.env.ComSpec || `${process.env.SystemRoot ?? 'C:\\Windows'}\\System32\\cmd.exe` @@ -60,54 +59,6 @@ function getWindowsSafeSpawn(binary: string, args: string[], env: NodeJS.Process return { spawnCmd: getCmdExePath(), spawnArgs: ['/d', '/s', '/c', commandLine] } } -function normalizeInteractiveShell(shell: unknown): string | null { - if (typeof shell !== 'string') { - return null - } - if (!shell || !posix.isAbsolute(shell)) { - return null - } - const shellName = posix.basename(shell).toLowerCase() - return shellName === 'bash' || shellName === 'zsh' ? shell : null -} - -function getAccountLoginShell(): string | null { - try { - return normalizeInteractiveShell(userInfo().shell) - } catch { - return null - } -} - -function resolveRequestedInteractiveShell(env: NodeJS.ProcessEnv): string | null { - const envShell = typeof env.SHELL === 'string' ? env.SHELL : '' - if (envShell) { - return normalizeInteractiveShell(envShell) - } - return getAccountLoginShell() -} - -function buildInteractiveShellFallback( - binary: string, - args: string[], - env: NodeJS.ProcessEnv -): SpawnCommand | null { - if (binary.includes('/')) { - return null - } - const shell = resolveRequestedInteractiveShell(env) - if (!shell) { - return null - } - // Why: SSH relay processes are launched by a non-interactive SSH command, - // whose PATH often lacks nvm/fnm/Homebrew agent installs. Use the user's - // explicit bash/zsh only as an ENOENT fallback so normal PATH hits stay fast. - return { - spawnCmd: shell, - spawnArgs: ['-ilc', 'exec "$@"', '_', binary, ...args] - } -} - export function getSpawnPlan( binary: string, args: string[], @@ -117,14 +68,16 @@ export function getSpawnPlan( if (process.platform === 'win32' || !useShell) { return getWindowsSafeSpawn(binary, args, env) } - const shellFallback = buildInteractiveShellFallback(binary, args, env) - if (!shellFallback) { + if (binary.includes('/')) { return { spawnCmd: binary, spawnArgs: args } } return { spawnCmd: binary, spawnArgs: args, - shellFallback + // Why: SSH commit/PR agents may be installed only after shell PATH + // initialization. Resolve that path only after direct ENOENT, then spawn + // the resolved binary directly instead of running the agent inside a shell. + pathFallbackCommand: binary } } diff --git a/src/relay/preflight-handler.test.ts b/src/relay/preflight-handler.test.ts index 6d2f9339b9e6..f51c7e000cf6 100644 --- a/src/relay/preflight-handler.test.ts +++ b/src/relay/preflight-handler.test.ts @@ -12,11 +12,13 @@ vi.mock('child_process', () => { }) import { + _resetRelayCommandPathCacheForTests, buildCommandLookupSpec, buildCommandLookupSpecs, hasAbsoluteCommandPath, - isCommandOnPathForRelay -} from './preflight-handler' + isCommandOnPathForRelay, + resolveCommandLaunchForRelay +} from './relay-command-path-lookup' function lookupArgs(command: string, mode: '-lc' | '-ilc' = '-lc'): string[] { return [ @@ -24,6 +26,7 @@ function lookupArgs(command: string, mode: '-lc' | '-ilc' = '-lc'): string[] { [ `if resolved=$(command -v ${command} 2>/dev/null); then`, 'printf \'__ORCA_AGENT_PATH__%s\\n\' "$resolved"', + 'printf \'__ORCA_AGENT_ENV_PATH__%s\\n\' "$PATH"', 'fi' ].join('\n') ] @@ -36,6 +39,7 @@ function fishLookupArgs(command: string): string[] { `set -l resolved (command -v ${command} 2>/dev/null)`, 'if test -n "$resolved"', 'printf \'__ORCA_AGENT_PATH__%s\\n\' "$resolved"', + 'printf \'__ORCA_AGENT_ENV_PATH__%s\\n\' "$PATH"', 'end' ].join('\n') ] @@ -43,6 +47,7 @@ function fishLookupArgs(command: string): string[] { beforeEach(() => { execFileAsyncMock.mockReset() + _resetRelayCommandPathCacheForTests() }) describe('buildCommandLookupSpec', () => { @@ -107,6 +112,22 @@ describe('buildCommandLookupSpecs', () => { }) }) + it('uses the account login shell when SHELL is unset', () => { + expect(buildCommandLookupSpecs('codex', 'linux', {}, '/bin/zsh')[0]).toEqual({ + file: '/bin/zsh', + args: lookupArgs("'codex'", '-ilc') + }) + }) + + it('can disable inherited PATH lookup after a direct spawn ENOENT', () => { + expect( + buildCommandLookupSpecs('codex', 'linux', { SHELL: '/bin/zsh' }, '/bin/zsh', { + allowedShellNames: ['bash', 'zsh'], + includeInheritedPathFallback: false + }) + ).toEqual([{ file: '/bin/zsh', args: lookupArgs("'codex'", '-ilc') }]) + }) + it('uses fish syntax for trusted fish shells', () => { expect(buildCommandLookupSpecs('codex', 'linux', { SHELL: '/usr/bin/fish' }, null)[0]).toEqual({ file: '/usr/bin/fish', @@ -127,6 +148,63 @@ describe('buildCommandLookupSpecs', () => { }) }) +describe('resolveCommandLaunchForRelay', () => { + it('returns the resolved absolute command path and shell-initialized PATH', async () => { + execFileAsyncMock.mockResolvedValueOnce({ + stdout: + '__ORCA_AGENT_PATH__/home/test/.nvm/versions/node/v22/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/home/test/.nvm/versions/node/v22/bin:/usr/bin\n' + }) + + await expect( + resolveCommandLaunchForRelay('opencode', { + platform: 'linux', + env: { SHELL: '/bin/bash', PATH: '/usr/bin' }, + accountLoginShell: '/bin/bash', + includeInheritedPathFallback: false + }) + ).resolves.toEqual({ + commandPath: '/home/test/.nvm/versions/node/v22/bin/opencode', + pathEnv: '/home/test/.nvm/versions/node/v22/bin:/usr/bin' + }) + }) + + it('reuses a preflighted path across stricter generation lookup options', async () => { + execFileAsyncMock.mockResolvedValueOnce({ + stdout: + '__ORCA_AGENT_PATH__/home/test/.local/bin/opencode\n' + + '__ORCA_AGENT_ENV_PATH__/home/test/.local/bin:/usr/bin\n' + }) + const env = { SHELL: '/usr/bin/fish', PATH: '/usr/bin' } + + await expect( + resolveCommandLaunchForRelay('opencode', { + platform: 'linux', + env, + accountLoginShell: null + }) + ).resolves.toEqual({ + commandPath: '/home/test/.local/bin/opencode', + pathEnv: '/home/test/.local/bin:/usr/bin' + }) + execFileAsyncMock.mockClear() + + await expect( + resolveCommandLaunchForRelay('opencode', { + platform: 'linux', + env, + accountLoginShell: null, + allowedShellNames: ['bash', 'zsh'], + includeInheritedPathFallback: false + }) + ).resolves.toEqual({ + commandPath: '/home/test/.local/bin/opencode', + pathEnv: '/home/test/.local/bin:/usr/bin' + }) + expect(execFileAsyncMock).not.toHaveBeenCalled() + }) +}) + describe('isCommandOnPathForRelay', () => { it('falls back to inherited PATH when shell startup returns no absolute command path', async () => { execFileAsyncMock diff --git a/src/relay/preflight-handler.ts b/src/relay/preflight-handler.ts index a0382d772752..9b91b50c6952 100644 --- a/src/relay/preflight-handler.ts +++ b/src/relay/preflight-handler.ts @@ -1,27 +1,15 @@ -import { execFile } from 'child_process' -import { userInfo } from 'os' -import { promisify } from 'util' -import path, { win32 } from 'path' import type { RelayDispatcher } from './dispatcher' -import { buildRelayCommandEnv } from './relay-command-env' - -const execFileAsync = promisify(execFile) - -type CommandLookupSpec = { - file: string - args: string[] - windowsHide?: true -} - -type RelayCommandLookupOptions = { - platform?: NodeJS.Platform - env?: NodeJS.ProcessEnv - accountLoginShell?: string | null -} - -const SUPPORTED_POSIX_SHELLS = new Set(['sh', 'dash', 'bash', 'zsh', 'fish']) -const CONSERVATIVE_SYSTEM_SHELL_DIRS = new Set(['/bin', '/usr/bin']) -const AGENT_PATH_PREFIX = '__ORCA_AGENT_PATH__' +import { isCommandOnPathForRelay } from './relay-command-path-lookup' + +export { + _resetRelayCommandPathCacheForTests, + buildCommandLookupSpec, + buildCommandLookupSpecs, + hasAbsoluteCommandPath, + isCommandOnPathForRelay, + resolveCommandLaunchForRelay, + resolveCommandPathForRelay +} from './relay-command-path-lookup' export class PreflightHandler { private dispatcher: RelayDispatcher @@ -62,156 +50,3 @@ export class PreflightHandler { return isCommandOnPathForRelay(command) } } - -export function buildCommandLookupSpec( - command: string, - platform: NodeJS.Platform, - env: NodeJS.ProcessEnv = process.env, - accountLoginShell?: string | null -): CommandLookupSpec { - const [spec] = buildCommandLookupSpecs(command, platform, env, accountLoginShell) - return spec ?? buildPosixCommandLookupSpec(command, '/bin/sh') -} - -export function buildCommandLookupSpecs( - command: string, - platform: NodeJS.Platform, - env: NodeJS.ProcessEnv = process.env, - accountLoginShell?: string | null -): CommandLookupSpec[] { - if (platform === 'win32') { - return [{ file: 'where.exe', args: [command], windowsHide: true }] - } - const trustedShell = pickTrustedPosixShell( - env, - resolveAccountLoginShell(platform, accountLoginShell) - ) - const specs: CommandLookupSpec[] = [] - - if (trustedShell) { - specs.push(buildPosixCommandLookupSpec(command, trustedShell)) - } - - const inheritedPathSpec = buildPosixCommandLookupSpec(command, '/bin/sh') - if (!trustedShell || trustedShell !== inheritedPathSpec.file) { - specs.push(inheritedPathSpec) - } - - return specs -} - -export async function isCommandOnPathForRelay( - command: string, - options: RelayCommandLookupOptions = {} -): Promise { - const platform = options.platform ?? process.platform - const env = options.env ?? process.env - const specs = buildCommandLookupSpecs(command, platform, env, options.accountLoginShell) - - for (const spec of specs) { - try { - const { stdout } = await execFileAsync(spec.file, spec.args, { - encoding: 'utf-8', - env: buildRelayCommandEnv(env, platform), - timeout: 5000, - ...(spec.windowsHide ? { windowsHide: true } : {}) - }) - if (hasAbsoluteCommandPath(stdout, platform)) { - return true - } - } catch { - // Try the inherited-PATH fallback before reporting the agent missing. - } - } - - return false -} - -export function hasAbsoluteCommandPath(output: string, platform: NodeJS.Platform): boolean { - const pathOps = platform === 'win32' ? win32 : path - return output - .split(/\r?\n/) - .map((line) => line.trim()) - .some((line) => { - const resolvedPath = - platform === 'win32' - ? line - : line.startsWith(AGENT_PATH_PREFIX) - ? line.slice(AGENT_PATH_PREFIX.length) - : '' - return pathOps.isAbsolute(resolvedPath) - }) -} - -function buildPosixCommandLookupSpec(command: string, shell: string): CommandLookupSpec { - const shellName = path.posix.basename(shell).toLowerCase() - if (shellName === 'fish') { - return { file: shell, args: ['-ilc', buildFishCommandLookupScript(command)] } - } - return { file: shell, args: [getShellCommandMode(shell), buildShCommandLookupScript(command)] } -} - -function buildShCommandLookupScript(command: string): string { - const quotedCommand = shellQuote(command) - return [ - `if resolved=$(command -v ${quotedCommand} 2>/dev/null); then`, - `printf '${AGENT_PATH_PREFIX}%s\\n' "$resolved"`, - 'fi' - ].join('\n') -} - -function buildFishCommandLookupScript(command: string): string { - const quotedCommand = shellQuote(command) - return [ - `set -l resolved (command -v ${quotedCommand} 2>/dev/null)`, - 'if test -n "$resolved"', - `printf '${AGENT_PATH_PREFIX}%s\\n' "$resolved"`, - 'end' - ].join('\n') -} - -function resolveAccountLoginShell( - platform: NodeJS.Platform, - accountLoginShell?: string | null -): string | null { - if (accountLoginShell !== undefined) { - return accountLoginShell - } - if (platform === 'win32') { - return null - } - try { - return userInfo().shell ?? null - } catch { - return null - } -} - -function pickTrustedPosixShell( - env: NodeJS.ProcessEnv, - accountLoginShell: string | null -): string | null { - const shell = env.SHELL - if (!shell || !path.posix.isAbsolute(shell)) { - return null - } - const shellName = path.posix.basename(shell).toLowerCase() - if (!SUPPORTED_POSIX_SHELLS.has(shellName)) { - return null - } - if (accountLoginShell) { - return shell === accountLoginShell ? shell : null - } - return CONSERVATIVE_SYSTEM_SHELL_DIRS.has(path.posix.dirname(shell)) ? shell : null -} - -function getShellCommandMode(shell: string): '-lc' | '-ilc' { - const shellName = path.posix.basename(shell).toLowerCase() - // Why: bash/zsh/fish users commonly add package-manager bins from interactive - // startup files. POSIX sh/dash may not support interactive login flags. - return shellName === 'sh' || shellName === 'dash' ? '-lc' : '-ilc' -} - -function shellQuote(value: string): string { - return `'${value.replace(/'/g, "'\\''")}'` -} diff --git a/src/relay/relay-command-path-lookup.ts b/src/relay/relay-command-path-lookup.ts new file mode 100644 index 000000000000..08b2af3db156 --- /dev/null +++ b/src/relay/relay-command-path-lookup.ts @@ -0,0 +1,261 @@ +import { execFile } from 'child_process' +import { userInfo } from 'os' +import { promisify } from 'util' +import path, { win32 } from 'path' +import { buildRelayCommandEnv } from './relay-command-env' + +const execFileAsync = promisify(execFile) + +export type CommandLookupSpec = { + file: string + args: string[] + windowsHide?: true +} + +export type RelayCommandLookupOptions = { + platform?: NodeJS.Platform + env?: NodeJS.ProcessEnv + accountLoginShell?: string | null + allowedShellNames?: readonly string[] + includeInheritedPathFallback?: boolean +} + +export type RelayCommandLaunch = { + commandPath: string + pathEnv?: string +} + +const SUPPORTED_POSIX_SHELLS = new Set(['sh', 'dash', 'bash', 'zsh', 'fish']) +const CONSERVATIVE_SYSTEM_SHELL_DIRS = new Set(['/bin', '/usr/bin']) +const AGENT_PATH_PREFIX = '__ORCA_AGENT_PATH__' +const AGENT_ENV_PATH_PREFIX = '__ORCA_AGENT_ENV_PATH__' +const commandLaunchCache = new Map() + +export function _resetRelayCommandPathCacheForTests(): void { + commandLaunchCache.clear() +} + +function cacheKeyFor(args: { + command: string + platform: NodeJS.Platform + env: NodeJS.ProcessEnv + accountLoginShell: string | null +}): string { + return JSON.stringify([ + args.command, + args.platform, + args.env.PATH ?? '', + args.env.Path ?? '', + args.env.SHELL ?? '', + args.accountLoginShell ?? '' + ]) +} + +export function buildCommandLookupSpec( + command: string, + platform: NodeJS.Platform, + env: NodeJS.ProcessEnv = process.env, + accountLoginShell?: string | null +): CommandLookupSpec { + const [spec] = buildCommandLookupSpecs(command, platform, env, accountLoginShell) + return spec ?? buildPosixCommandLookupSpec(command, '/bin/sh') +} + +export function buildCommandLookupSpecs( + command: string, + platform: NodeJS.Platform, + env: NodeJS.ProcessEnv = process.env, + accountLoginShell?: string | null, + options: Pick< + RelayCommandLookupOptions, + 'allowedShellNames' | 'includeInheritedPathFallback' + > = {} +): CommandLookupSpec[] { + if (platform === 'win32') { + return [{ file: 'where.exe', args: [command], windowsHide: true }] + } + const trustedShell = pickTrustedPosixShell( + env, + resolveAccountLoginShell(platform, accountLoginShell), + options.allowedShellNames + ) + const includeInheritedPathFallback = options.includeInheritedPathFallback !== false + const specs: CommandLookupSpec[] = [] + + if (trustedShell) { + specs.push(buildPosixCommandLookupSpec(command, trustedShell)) + } + + if (includeInheritedPathFallback) { + const inheritedPathSpec = buildPosixCommandLookupSpec(command, '/bin/sh') + if (!trustedShell || trustedShell !== inheritedPathSpec.file) { + specs.push(inheritedPathSpec) + } + } + + return specs +} + +export async function resolveCommandLaunchForRelay( + command: string, + options: RelayCommandLookupOptions = {} +): Promise { + const platform = options.platform ?? process.platform + const env = options.env ?? process.env + const accountLoginShell = resolveAccountLoginShell(platform, options.accountLoginShell) + const allowedShellNames = options.allowedShellNames ?? [...SUPPORTED_POSIX_SHELLS] + const cacheKey = cacheKeyFor({ command, platform, env, accountLoginShell }) + const cached = commandLaunchCache.get(cacheKey) + if (cached) { + return cached + } + const specs = buildCommandLookupSpecs(command, platform, env, accountLoginShell, { + allowedShellNames, + includeInheritedPathFallback: options.includeInheritedPathFallback + }) + + for (const spec of specs) { + try { + const { stdout } = await execFileAsync(spec.file, spec.args, { + encoding: 'utf-8', + env: buildRelayCommandEnv(env, platform), + timeout: 5000, + ...(spec.windowsHide ? { windowsHide: true } : {}) + }) + const resolved = parseCommandLaunch(stdout, platform) + if (resolved) { + commandLaunchCache.set(cacheKey, resolved) + return resolved + } + } catch { + // Try the next trusted lookup source before reporting the agent missing. + } + } + + return null +} + +export async function resolveCommandPathForRelay( + command: string, + options: RelayCommandLookupOptions = {} +): Promise { + return (await resolveCommandLaunchForRelay(command, options))?.commandPath ?? null +} + +export async function isCommandOnPathForRelay( + command: string, + options: RelayCommandLookupOptions = {} +): Promise { + return (await resolveCommandLaunchForRelay(command, options)) !== null +} + +export function hasAbsoluteCommandPath(output: string, platform: NodeJS.Platform): boolean { + return parseCommandLaunch(output, platform) !== null +} + +function parseCommandLaunch(output: string, platform: NodeJS.Platform): RelayCommandLaunch | null { + const pathOps = platform === 'win32' ? win32 : path + let commandPath: string | null = null + let pathEnv: string | undefined + + for (const line of output.split(/\r?\n/)) { + const trimmed = line.trim() + if (platform === 'win32') { + if (pathOps.isAbsolute(trimmed)) { + return { commandPath: trimmed } + } + continue + } + if (trimmed.startsWith(AGENT_PATH_PREFIX)) { + const resolvedPath = trimmed.slice(AGENT_PATH_PREFIX.length) + if (pathOps.isAbsolute(resolvedPath)) { + commandPath = resolvedPath + } + continue + } + if (trimmed.startsWith(AGENT_ENV_PATH_PREFIX)) { + pathEnv = trimmed.slice(AGENT_ENV_PATH_PREFIX.length) + } + } + + if (!commandPath) { + return null + } + return pathEnv === undefined ? { commandPath } : { commandPath, pathEnv } +} + +function buildPosixCommandLookupSpec(command: string, shell: string): CommandLookupSpec { + const shellName = path.posix.basename(shell).toLowerCase() + if (shellName === 'fish') { + return { file: shell, args: ['-ilc', buildFishCommandLookupScript(command)] } + } + return { file: shell, args: [getShellCommandMode(shell), buildShCommandLookupScript(command)] } +} + +function buildShCommandLookupScript(command: string): string { + const quotedCommand = shellQuote(command) + return [ + `if resolved=$(command -v ${quotedCommand} 2>/dev/null); then`, + `printf '${AGENT_PATH_PREFIX}%s\\n' "$resolved"`, + `printf '${AGENT_ENV_PATH_PREFIX}%s\\n' "$PATH"`, + 'fi' + ].join('\n') +} + +function buildFishCommandLookupScript(command: string): string { + const quotedCommand = shellQuote(command) + return [ + `set -l resolved (command -v ${quotedCommand} 2>/dev/null)`, + 'if test -n "$resolved"', + `printf '${AGENT_PATH_PREFIX}%s\\n' "$resolved"`, + `printf '${AGENT_ENV_PATH_PREFIX}%s\\n' "$PATH"`, + 'end' + ].join('\n') +} + +function resolveAccountLoginShell( + platform: NodeJS.Platform, + accountLoginShell?: string | null +): string | null { + if (accountLoginShell !== undefined) { + return accountLoginShell + } + if (platform === 'win32') { + return null + } + try { + return userInfo().shell ?? null + } catch { + return null + } +} + +function pickTrustedPosixShell( + env: NodeJS.ProcessEnv, + accountLoginShell: string | null, + allowedShellNames: readonly string[] = [...SUPPORTED_POSIX_SHELLS] +): string | null { + const shell = env.SHELL || accountLoginShell || '' + if (!shell || !path.posix.isAbsolute(shell)) { + return null + } + const shellName = path.posix.basename(shell).toLowerCase() + if (!SUPPORTED_POSIX_SHELLS.has(shellName) || !allowedShellNames.includes(shellName)) { + return null + } + if (accountLoginShell) { + return shell === accountLoginShell ? shell : null + } + return CONSERVATIVE_SYSTEM_SHELL_DIRS.has(path.posix.dirname(shell)) ? shell : null +} + +function getShellCommandMode(shell: string): '-lc' | '-ilc' { + const shellName = path.posix.basename(shell).toLowerCase() + // Why: bash/zsh/fish users commonly add package-manager bins from interactive + // startup files. POSIX sh/dash may not support interactive login flags. + return shellName === 'sh' || shellName === 'dash' ? '-lc' : '-ilc' +} + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, "'\\''")}'` +}