From b08acd340c70e464abb9340b409459a63482ef2d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:33:43 -0700 Subject: [PATCH] fix(orchestration): harden mutation recovery --- .../handlers/orchestration-gate-cli.test.ts | 10 ++ .../orchestration-mutation-recovery.test.ts | 104 ++++++++++++++++++ src/cli/orchestration-mutation-recovery.ts | 17 ++- src/cli/runtime/client-recovery.test.ts | 2 +- .../orchestration-recovery-command.test.ts | 59 ++++++++-- .../runtime/orchestration-recovery-command.ts | 18 ++- .../ssh-remote-cli-host-passthrough.test.ts | 18 +++ .../ssh/ssh-remote-cli-host-passthrough.ts | 2 + 8 files changed, 207 insertions(+), 23 deletions(-) diff --git a/src/cli/handlers/orchestration-gate-cli.test.ts b/src/cli/handlers/orchestration-gate-cli.test.ts index 0bcdddbc98e..b4be315c155 100644 --- a/src/cli/handlers/orchestration-gate-cli.test.ts +++ b/src/cli/handlers/orchestration-gate-cli.test.ts @@ -206,6 +206,16 @@ describe('orchestration gate commands carry caller identity', () => { 'The Orca runtime closed the connection before responding. Restart Orca and try again. Orchestration mutation request ID: mutation_1.', { orchestrationRequestId: 'mutation_1', + originalCommand: [ + 'orca', + 'orchestration', + 'gate-create', + '--task', + 'task_1', + '--question', + 'ship?', + '--json' + ], failedStage: 'dispatch_input', residualResources: [ { kind: 'worktree', id: 'repo::child' }, diff --git a/src/cli/orchestration-mutation-recovery.test.ts b/src/cli/orchestration-mutation-recovery.test.ts index c1870f5affe..c0f4ba0ac10 100644 --- a/src/cli/orchestration-mutation-recovery.test.ts +++ b/src/cli/orchestration-mutation-recovery.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest' +import { runProcess } from '../shared/child-process/run-process' import { orchestrationMutationRecoveryError, renderCommand @@ -115,6 +116,56 @@ describe('orchestration mutation recovery', () => { ]) }) + it.each([ + [ + 'gate-create', + ['orca', 'orchestration', 'gate-create', '--task', 'task_1', '--question', 'ship?'] + ], + [ + 'worker-retain', + ['orca', 'orchestration', 'worker-retain', '--dispatch', 'dispatch_1', '--json'] + ] + ])('replays exact %s argv with the keyed retry', (_name, originalCommand) => { + const result = orchestrationMutationRecoveryError( + new RuntimeClientError('runtime_timeout', 'request timed out', { + orchestrationRequestId: 'request_exact', + originalCommand + }) + ) as RuntimeClientError + + expect( + (result.data as { recovery?: { retryCommand?: string[] } }).recovery?.retryCommand + ).toEqual([...originalCommand, '--retry-request', 'request_exact']) + }) + + it('reuses the request identity without duplicating an existing retry flag', () => { + const result = orchestrationMutationRecoveryError( + new RuntimeClientError('runtime_timeout', 'request timed out', { + orchestrationRequestId: 'request_reused', + originalCommand: [ + 'orca', + 'orchestration', + 'worker-retain', + '--dispatch', + 'dispatch_1', + '--retry-request=request_reused' + ] + }) + ) as RuntimeClientError + + expect( + (result.data as { recovery?: { retryCommand?: string[] } }).recovery?.retryCommand + ).toEqual([ + 'orca', + 'orchestration', + 'worker-retain', + '--dispatch', + 'dispatch_1', + '--retry-request', + 'request_reused' + ]) + }) + it('renders Windows cmd recovery guidance without quote drift or percent expansion', () => { expect( renderCommand( @@ -137,4 +188,57 @@ describe('orchestration mutation recovery', () => { "orca 'literal $(do-not-run)'" ) }) + + it.runIf(process.platform !== 'win32')( + 'round trips POSIX recovery argv through /bin/sh', + async () => { + const values = ['with spaces', "apostrophe's", 'literal $(do-not-run)', 'line one\nline two'] + const command = renderCommand( + [ + process.execPath, + '-e', + 'process.stdout.write(JSON.stringify(process.argv.slice(1)))', + ...values + ], + 'darwin' + ) + + const result = await runProcess({ program: '/bin/sh', args: ['-c', command] }) + + expect(result).toMatchObject({ code: 0, stderr: '', timedOut: false }) + expect(JSON.parse(result.stdout)).toEqual(values) + } + ) + + it.each([ + [ + 'split', + ['orca', 'orchestration', 'send', '--pairing-code', 'split-secret', '--subject', 'status'], + 'split-secret' + ], + [ + 'equals', + ['orca', 'orchestration', 'send', '--pairing-code=equals-secret', '--subject', 'status'], + 'equals-secret' + ] + ])('blocks recovery and removes %s pairing credentials', (_name, originalCommand, secret) => { + const result = orchestrationMutationRecoveryError( + new RuntimeClientError('runtime_timeout', 'request timed out', { + orchestrationRequestId: 'request_secret', + originalCommand + }) + ) as RuntimeClientError + const output = JSON.stringify({ message: result.message, data: result.data }) + + expect(output).not.toContain(secret) + expect(result.message).toContain('Recovery is blocked') + expect(result.data).toMatchObject({ + recovery: { + orchestrationRequestId: 'request_secret', + recoveryBlocked: true + } + }) + expect(result.data).not.toHaveProperty('originalCommand') + expect((result.data as { recovery: object }).recovery).not.toHaveProperty('retryCommand') + }) }) diff --git a/src/cli/orchestration-mutation-recovery.ts b/src/cli/orchestration-mutation-recovery.ts index 9362d5bb2de..9d245250b59 100644 --- a/src/cli/orchestration-mutation-recovery.ts +++ b/src/cli/orchestration-mutation-recovery.ts @@ -1,5 +1,8 @@ import { RuntimeClientError } from './runtime-client' -import { resolveOrchestrationCliExecutable } from './runtime/orchestration-recovery-command' +import { + recoverableOrchestrationArgs, + resolveOrchestrationCliExecutable +} from './runtime/orchestration-recovery-command' import { quoteWindowsCmdArgument } from '../shared/child-process/windows-command-line' import { quotePowerShellNativeArgument } from '../shared/powershell-native-argument' import { resolveWindowsShellStartupFamily } from '../shared/windows-terminal-shell' @@ -15,7 +18,12 @@ export function orchestrationMutationRecoveryError(error: unknown): unknown { return error } const dispatchId = typeof data?.dispatchId === 'string' ? data.dispatchId : undefined - const originalCommand = commandParts(data?.originalCommand) + const parsedOriginalCommand = commandParts(data?.originalCommand) + const originalCommand = parsedOriginalCommand + ? recoverableOrchestrationArgs(parsedOriginalCommand) + : undefined + const safeData = { ...data } + delete safeData.originalCommand const retryCommand = originalCommand ? [...originalCommand, '--retry-request', requestId] : undefined @@ -48,8 +56,9 @@ export function orchestrationMutationRecoveryError(error: unknown): unknown { : undefined ].filter((line): line is string => line !== undefined) return new RuntimeClientError(error.code, message.join('\n'), { - ...data, + ...safeData, orchestrationRequestId: requestId, + ...(originalCommand ? { originalCommand } : {}), recovery, nextSteps }) @@ -171,7 +180,7 @@ function shellQuote(value: string): string { if (/^[A-Za-z0-9_./:@%+=,-]+$/.test(value)) { return value } - return `'${value.replaceAll("'", "'\\\"'\\\"'")}'` + return `'${value.replaceAll("'", `'"'"'`)}'` } function stripUnsafeRetryAdvice(message: string, requestId: string): string { diff --git a/src/cli/runtime/client-recovery.test.ts b/src/cli/runtime/client-recovery.test.ts index ae77a009b0b..d502df09c2d 100644 --- a/src/cli/runtime/client-recovery.test.ts +++ b/src/cli/runtime/client-recovery.test.ts @@ -69,7 +69,7 @@ describe('RuntimeClient orchestration recovery identity', () => { }) ) - const client = new RuntimeClient(userDataPath, 500) + const client = new RuntimeClient(userDataPath, 500, null, null, 'orca') try { await client.call('orchestration.workerStart', { task: 'task_1' }) throw new Error('expected worker-start failure') diff --git a/src/cli/runtime/orchestration-recovery-command.test.ts b/src/cli/runtime/orchestration-recovery-command.test.ts index 62adde1ebd6..7ffab35ed37 100644 --- a/src/cli/runtime/orchestration-recovery-command.test.ts +++ b/src/cli/runtime/orchestration-recovery-command.test.ts @@ -5,18 +5,15 @@ import { } from './orchestration-recovery-command' describe('orchestration recovery command identity', () => { - it('resolves dev, packaged, and execution-host CLI names without rewriting them', () => { - expect(resolveOrchestrationCliExecutable({ ORCA_CLI_COMMAND: 'orca-dev' }, 'darwin')).toBe( - 'orca-dev' - ) - expect(resolveOrchestrationCliExecutable({ ORCA_CLI_COMMAND: 'orca-ide' }, 'win32')).toBe( - 'orca-ide' - ) - expect(resolveOrchestrationCliExecutable({ ORCA_DEV_REPO_ROOT: '/repo' }, 'darwin')).toBe( - 'orca-dev' - ) - expect(resolveOrchestrationCliExecutable({}, 'linux')).toBe('orca-ide') - expect(resolveOrchestrationCliExecutable({}, 'darwin')).toBe('orca') + it.each([ + ['configured dev', { ORCA_CLI_COMMAND: 'orca-dev' }, 'darwin', 'orca-dev'], + ['configured WSL', { ORCA_CLI_COMMAND: 'orca-ide' }, 'linux', 'orca-ide'], + ['dev checkout', { ORCA_DEV_REPO_ROOT: '/repo' }, 'darwin', 'orca-dev'], + ['packaged Linux', {}, 'linux', 'orca-ide'], + ['local macOS', {}, 'darwin', 'orca'], + ['local Windows', {}, 'win32', 'orca'] + ] as const)('resolves the %s CLI identity', (_name, env, platform, expected) => { + expect(resolveOrchestrationCliExecutable(env, platform)).toBe(expected) }) it('reconstructs the keyed worker-start command from its RPC params', () => { @@ -75,6 +72,30 @@ describe('orchestration recovery command identity', () => { ]) }) + it.each([ + [ + 'gate-create', + 'orchestration.gateCreate', + ['orchestration', 'gate-create', '--task', 'task_1', '--question', 'ship?'] + ], + [ + 'worker-retain', + 'orchestration.workerRetain', + ['orchestration', 'worker-retain', '--dispatch', 'dispatch_1', '--json'] + ] + ] as const)('preserves exact raw argv for %s recovery', (_name, method, args) => { + expect(buildOrchestrationRecoveryCommand(method, {}, 'orca', args)).toEqual(['orca', ...args]) + }) + + it.each([ + ['split', ['orchestration', 'send', '--pairing-code', 'secret', '--subject', 'status']], + ['equals', ['orchestration', 'send', '--pairing-code=secret', '--subject', 'status']] + ])('blocks %s pairing-code argv instead of exposing a credential', (_name, args) => { + expect( + buildOrchestrationRecoveryCommand('orchestration.send', {}, 'orca', args) + ).toBeUndefined() + }) + it('supports the explicit executable-first form', () => { expect( buildOrchestrationRecoveryCommand('orca-ide', 'orchestration.workerStop', { @@ -83,6 +104,20 @@ describe('orchestration recovery command identity', () => { ).toEqual(['orca-ide', 'orchestration', 'worker-stop', '--dispatch', 'dispatch_1']) }) + it('reconstructs worker-retain when raw argv is unavailable', () => { + expect( + buildOrchestrationRecoveryCommand('orchestration.workerRetain', { + dispatch: 'dispatch_1' + }) + ).toEqual([ + resolveOrchestrationCliExecutable(), + 'orchestration', + 'worker-retain', + '--dispatch', + 'dispatch_1' + ]) + }) + it('does not fabricate identity for unsupported mutations or malformed params', () => { expect(buildOrchestrationRecoveryCommand('orchestration.send', {})).toBeUndefined() expect(buildOrchestrationRecoveryCommand('orchestration.workerStart', null)).toBeUndefined() diff --git a/src/cli/runtime/orchestration-recovery-command.ts b/src/cli/runtime/orchestration-recovery-command.ts index 6ddb893de8b..3a93ef817ed 100644 --- a/src/cli/runtime/orchestration-recovery-command.ts +++ b/src/cli/runtime/orchestration-recovery-command.ts @@ -27,7 +27,7 @@ export function buildOrchestrationRecoveryCommand( export function buildOrchestrationRecoveryCommand( methodOrExecutable: string, paramsOrMethod: unknown, - executableOrParams?: string | unknown, + executableOrParams?: unknown, originalArgs?: readonly string[] ): string[] | undefined { const legacyOrder = typeof paramsOrMethod === 'string' && executableOrParams !== undefined @@ -38,6 +38,10 @@ export function buildOrchestrationRecoveryCommand( : typeof executableOrParams === 'string' ? executableOrParams : resolveOrchestrationCliExecutable() + if (originalArgs && originalArgs.length > 0) { + const recoverableArgs = recoverableOrchestrationArgs(originalArgs) + return recoverableArgs ? [executable, ...recoverableArgs] : undefined + } const command = method === 'orchestration.workerStart' ? 'worker-start' @@ -47,7 +51,9 @@ export function buildOrchestrationRecoveryCommand( ? 'worker-abandon' : method === 'orchestration.workerRelease' ? 'worker-release' - : undefined + : method === 'orchestration.workerRetain' + ? 'worker-retain' + : undefined if (!command || params === null || typeof params !== 'object') { return undefined } @@ -56,9 +62,6 @@ export function buildOrchestrationRecoveryCommand( if (typeof record[requiredKey] !== 'string' || record[requiredKey].length === 0) { return undefined } - if (originalArgs && originalArgs.length > 0) { - return [executable, ...withoutRetryRequest(originalArgs)] - } const args = [executable, 'orchestration', command] for (const [key, value] of Object.entries(record)) { if (value === undefined || value === null || value === false) { @@ -74,7 +77,10 @@ export function buildOrchestrationRecoveryCommand( return args } -function withoutRetryRequest(args: readonly string[]): string[] { +export function recoverableOrchestrationArgs(args: readonly string[]): string[] | undefined { + if (args.some((arg) => arg === '--pairing-code' || arg.startsWith('--pairing-code='))) { + return undefined + } const result: string[] = [] for (let index = 0; index < args.length; index += 1) { const arg = args[index] diff --git a/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts b/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts index b4d7ca07fef..e66cbd77507 100644 --- a/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts +++ b/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts @@ -106,11 +106,29 @@ describe('buildHostCliEnv', () => { expect(env.PATH).toBe('/host/bin') expect(env.ORCA_USER_DATA_PATH).toBe('/host/user-data') expect(env.ORCA_CLI_CWD).toBe('/home/alice/wt/sub') + expect(env.ORCA_CLI_COMMAND).toBe('orca') expect(env.ELECTRON_RUN_AS_NODE).toBe('1') expect(env.NODE_OPTIONS).toBeUndefined() expect(env.ORCA_NODE_OPTIONS).toBe('--inspect') }) + it.each([ + ['dev host', { ORCA_DEV_REPO_ROOT: '/repo', ORCA_CLI_COMMAND: 'orca-dev' }], + ['packaged Linux host', { ORCA_CLI_COMMAND: 'orca-ide' }], + ['local host', {}], + ['WSL host', { WSL_DISTRO_NAME: 'Ubuntu', ORCA_CLI_COMMAND: 'orca-ide' }], + ['Windows host', { ComSpec: 'C:\\Windows\\System32\\cmd.exe' }] + ])('pins %s recovery to the remote shim', (_name, hostEnv) => { + const env = buildHostCliEnv({ + hostEnv, + remoteEnv: { ORCA_CLI_COMMAND: 'untrusted-remote-command' }, + userDataPath: '/host/user-data', + remoteCwd: '/srv/repo' + }) + + expect(env.ORCA_CLI_COMMAND).toBe('orca') + }) + it('namespaces identical remote artifact paths by stable SSH target', () => { const artifactInput = { sourceKey: '/srv/repo/report.html', diff --git a/src/main/ssh/ssh-remote-cli-host-passthrough.ts b/src/main/ssh/ssh-remote-cli-host-passthrough.ts index 7749584a61a..05a785c8482 100644 --- a/src/main/ssh/ssh-remote-cli-host-passthrough.ts +++ b/src/main/ssh/ssh-remote-cli-host-passthrough.ts @@ -159,6 +159,8 @@ export function buildHostCliEnv(args: { // subprocess cwd cannot be chdir'd there; ORCA_CLI_CWD carries it for // cwd-based selectors like `--worktree active`. env.ORCA_CLI_CWD = args.remoteCwd + // Why: recovery commands run on the SSH execution host through its relay shim. + env.ORCA_CLI_COMMAND = 'orca' // Why: same node-mode hygiene as the shipped CLI launchers — stash and clear // NODE_OPTIONS so Electron's node bootstrap does not inherit them. env.ORCA_NODE_OPTIONS = args.hostEnv.NODE_OPTIONS ?? ''