From 3b2f6b29dc7da0c01b172fb016cdcdb8c09c8707 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:49:25 -0700 Subject: [PATCH] fix(computer): supervise desktop providers from main --- ...omputer-provider-supervisor-client.test.ts | 71 +++- .../computer-provider-supervisor-client.ts | 29 +- .../computer-provider-supervisor-host.test.ts | 85 ++++- .../computer-provider-supervisor-host.ts | 21 +- ...puter-provider-supervisor-protocol.test.ts | 152 +++++++++ .../computer-provider-supervisor-protocol.ts | 56 ++- .../desktop-script-provider-bridge.ts | 115 +------ .../desktop-script-provider-client.ts | 61 +--- .../desktop-script-provider-errors.test.ts | 41 +-- ...ktop-script-provider-process-invocation.ts | 110 ++++++ ...ktop-script-provider-request-validation.ts | 277 +++++++++++++++ ...desktop-script-provider-supervisor.test.ts | 317 +++++++++++++++++ .../desktop-script-provider-supervisor.ts | 320 ++++++++++++++++++ .../desktop-script-provider-test-harness.ts | 78 ++--- 14 files changed, 1490 insertions(+), 243 deletions(-) create mode 100644 src/main/computer/computer-provider-supervisor-protocol.test.ts create mode 100644 src/main/computer/desktop-script-provider-process-invocation.ts create mode 100644 src/main/computer/desktop-script-provider-request-validation.ts create mode 100644 src/main/computer/desktop-script-provider-supervisor.test.ts create mode 100644 src/main/computer/desktop-script-provider-supervisor.ts diff --git a/src/main/computer/computer-provider-supervisor-client.test.ts b/src/main/computer/computer-provider-supervisor-client.test.ts index 131c33ca6fc..e32b73451d5 100644 --- a/src/main/computer/computer-provider-supervisor-client.test.ts +++ b/src/main/computer/computer-provider-supervisor-client.test.ts @@ -1,5 +1,8 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -import { ComputerProviderSupervisorClient } from './computer-provider-supervisor-client' +import { + ComputerProviderSupervisorClient, + DESKTOP_PROVIDER_SUPERVISOR_REQUEST_TIMEOUT_MS +} from './computer-provider-supervisor-client' import { COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, type ComputerProviderSupervisorRequest @@ -88,6 +91,52 @@ describe('ComputerProviderSupervisorClient', () => { ]) }) + it('executes only a fixed desktop request and validates its result', async () => { + const sent: ComputerProviderSupervisorRequest[] = [] + const client = new ComputerProviderSupervisorClient((message, callback) => { + sent.push(message) + callback(null) + }) + + const execution = client.executeDesktopProvider({ tool: 'list_apps' }) + expect(sent).toEqual([ + { + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 1, + method: 'desktop.execute', + params: { request: { tool: 'list_apps' } } + } + ]) + client.handleMessage({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'response', + id: 1, + ok: true, + result: { stdout: '{"ok":true}', stderr: '', error: null } + }) + + await expect(execution).resolves.toEqual({ + stdout: '{"ok":true}', + stderr: '', + error: null + }) + }) + + it('rejects malformed desktop results from the parent boundary', async () => { + const client = new ComputerProviderSupervisorClient(() => {}) + const execution = client.executeDesktopProvider({ tool: 'list_apps' }) + client.handleMessage({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'response', + id: 1, + ok: true, + result: { stdout: Buffer.from('unbounded binary'), stderr: '', error: null } + }) + + await expect(execution).rejects.toThrow('invalid desktop provider result') + }) + it('rejects immediately when supervisor IPC delivery fails', async () => { const client = new ComputerProviderSupervisorClient((_message, callback) => { callback(new Error('ipc channel closed')) @@ -189,4 +238,24 @@ describe('ComputerProviderSupervisorClient', () => { await rejection }) + + it('allows the desktop supervisor deadline and reap grace to finish', async () => { + vi.useFakeTimers() + const client = new ComputerProviderSupervisorClient(() => {}) + + const execution = client.executeDesktopProvider({ tool: 'list_apps' }) + const rejection = expect(execution).rejects.toThrow( + 'computer provider supervisor desktop.execute timed out' + ) + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_SUPERVISOR_REQUEST_TIMEOUT_MS - 1) + let settled = false + void execution.catch(() => { + settled = true + }) + await Promise.resolve() + expect(settled).toBe(false) + + await vi.advanceTimersByTimeAsync(1) + await rejection + }) }) diff --git a/src/main/computer/computer-provider-supervisor-client.ts b/src/main/computer/computer-provider-supervisor-client.ts index 1c2fa257ccd..60e6b86605f 100644 --- a/src/main/computer/computer-provider-supervisor-client.ts +++ b/src/main/computer/computer-provider-supervisor-client.ts @@ -2,12 +2,16 @@ import { COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, isComputerProviderSupervisorMessage, isStartedSupervisedMacOSProvider, + isSupervisedDesktopProviderResult, type ComputerProviderSupervisorRequest, - type StartedSupervisedMacOSProvider + type StartedSupervisedMacOSProvider, + type SupervisedDesktopProviderResult } from './computer-provider-supervisor-protocol' +import type { BridgeRequest } from './desktop-script-provider-types' import { RuntimeClientError } from './runtime-client-error' const SUPERVISOR_REQUEST_TIMEOUT_MS = 10_000 +export const DESKTOP_PROVIDER_SUPERVISOR_REQUEST_TIMEOUT_MS = 40_000 const MAX_EARLY_MACOS_TERMINATIONS = 32 type PendingSupervisorRequest = { @@ -69,6 +73,17 @@ export class ComputerProviderSupervisorClient { } } + async executeDesktopProvider(request: BridgeRequest): Promise { + const result = await this.request('desktop.execute', { request }) + if (!isSupervisedDesktopProviderResult(result)) { + throw new RuntimeClientError( + 'accessibility_error', + 'computer provider supervisor returned an invalid desktop provider result' + ) + } + return result + } + handleMessage(message: unknown): boolean { if (!isComputerProviderSupervisorMessage(message)) { return false @@ -138,6 +153,10 @@ export class ComputerProviderSupervisorClient { } as ComputerProviderSupervisorRequest return new Promise((resolve, reject) => { + const timeoutMs = + method === 'desktop.execute' + ? DESKTOP_PROVIDER_SUPERVISOR_REQUEST_TIMEOUT_MS + : SUPERVISOR_REQUEST_TIMEOUT_MS const timer = setTimeout(() => { this.pending.delete(id) reject( @@ -146,7 +165,7 @@ export class ComputerProviderSupervisorClient { `computer provider supervisor ${method} timed out` ) ) - }, SUPERVISOR_REQUEST_TIMEOUT_MS) + }, timeoutMs) this.pending.set(id, { resolve, reject, timer }) try { this.send(request, (error) => { @@ -204,6 +223,12 @@ export function releaseSupervisedMacOSProvider(sessionId: string): Promise return client.releaseMacOSProvider(sessionId) } +export function executeSupervisedDesktopProvider( + request: BridgeRequest +): Promise { + return client.executeDesktopProvider(request) +} + export function handleComputerProviderSupervisorMessage(message: unknown): boolean { return client.handleMessage(message) } diff --git a/src/main/computer/computer-provider-supervisor-host.test.ts b/src/main/computer/computer-provider-supervisor-host.test.ts index 7e347b5cffc..bd185294587 100644 --- a/src/main/computer/computer-provider-supervisor-host.test.ts +++ b/src/main/computer/computer-provider-supervisor-host.test.ts @@ -15,6 +15,13 @@ function macOSSupervisor() { } } +function desktopSupervisor() { + return { + execute: vi.fn(async () => ({ stdout: '{"ok":true}', stderr: '', error: null })), + shutdown: vi.fn() + } +} + describe('ComputerProviderSupervisorHost', () => { it('routes only fixed domain operations and returns their bounded result', async () => { const macOS = macOSSupervisor() @@ -49,6 +56,80 @@ describe('ComputerProviderSupervisorHost', () => { ]) }) + it('routes validated desktop requests without accepting invocation fields', async () => { + const macOS = macOSSupervisor() + const desktop = desktopSupervisor() + const host = new ComputerProviderSupervisorHost(macOS as never, desktop as never) + const sent: unknown[] = [] + host.attach((message) => sent.push(message)) + + expect( + host.handle({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 2, + method: 'desktop.execute', + params: { request: { tool: 'list_apps' } } + }) + ).toBe(true) + await vi.waitFor(() => expect(sent).toHaveLength(1)) + + expect(desktop.execute).toHaveBeenCalledWith({ tool: 'list_apps' }) + expect(sent).toEqual([ + expect.objectContaining({ + id: 2, + ok: true, + result: { stdout: '{"ok":true}', stderr: '', error: null } + }) + ]) + + expect( + host.handle({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 3, + method: 'desktop.execute', + params: { + request: { tool: 'list_apps' }, + command: '/tmp/untrusted', + args: ['--anything'] + } + }) + ).toBe(false) + expect(desktop.execute).toHaveBeenCalledTimes(1) + }) + + it('does not deliver a stale operation response to a replacement owner', async () => { + type DesktopResult = { stdout: string; stderr: string; error: null } + let resolveExecution = (_result: DesktopResult): void => {} + const desktop = desktopSupervisor() + desktop.execute.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveExecution = resolve + }) + ) + const host = new ComputerProviderSupervisorHost(macOSSupervisor() as never, desktop as never) + const originalSend = vi.fn() + const replacementSend = vi.fn() + host.attach(originalSend) + host.handle({ + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 4, + method: 'desktop.execute', + params: { request: { tool: 'list_apps' } } + }) + + host.shutdown() + host.attach(replacementSend) + resolveExecution({ stdout: '{"ok":true}', stderr: '', error: null }) + await Promise.resolve() + + expect(originalSend).not.toHaveBeenCalled() + expect(replacementSend).not.toHaveBeenCalled() + }) + it('rejects arbitrary executable and argument fields at the protocol boundary', () => { const macOS = macOSSupervisor() const host = new ComputerProviderSupervisorHost(macOS as never) @@ -67,13 +148,15 @@ describe('ComputerProviderSupervisorHost', () => { it('kills all sessions and detaches delivery when the owner shuts down', () => { const macOS = macOSSupervisor() - const host = new ComputerProviderSupervisorHost(macOS as never) + const desktop = desktopSupervisor() + const host = new ComputerProviderSupervisorHost(macOS as never, desktop as never) const send = vi.fn() host.attach(send) host.shutdown() expect(macOS.shutdown).toHaveBeenCalledTimes(1) + expect(desktop.shutdown).toHaveBeenCalledTimes(1) expect( host.handle({ channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, diff --git a/src/main/computer/computer-provider-supervisor-host.ts b/src/main/computer/computer-provider-supervisor-host.ts index 417fe0c89b3..b91e1a1789a 100644 --- a/src/main/computer/computer-provider-supervisor-host.ts +++ b/src/main/computer/computer-provider-supervisor-host.ts @@ -5,6 +5,7 @@ import { type ComputerProviderSupervisorRequest, type ComputerProviderSupervisorResponse } from './computer-provider-supervisor-protocol' +import { DesktopScriptProviderSupervisor } from './desktop-script-provider-supervisor' import { MacOSNativeProviderSupervisor } from './macos-native-provider-supervisor' type SupervisorMessageSender = (message: ComputerProviderSupervisorMessage) => void @@ -14,13 +15,17 @@ type SupervisorResponsePayload = export class ComputerProviderSupervisorHost { private sender: SupervisorMessageSender | null = null + private ownerGeneration = 0 private readonly macOS: MacOSNativeProviderSupervisor + private readonly desktop: DesktopScriptProviderSupervisor - constructor(macOS?: MacOSNativeProviderSupervisor) { + constructor(macOS?: MacOSNativeProviderSupervisor, desktop?: DesktopScriptProviderSupervisor) { this.macOS = macOS ?? new MacOSNativeProviderSupervisor((event) => this.sender?.(event)) + this.desktop = desktop ?? new DesktopScriptProviderSupervisor() } attach(sender: SupervisorMessageSender): void { + this.ownerGeneration++ this.sender = sender } @@ -28,10 +33,11 @@ export class ComputerProviderSupervisorHost { if (!this.sender || !isComputerProviderSupervisorRequest(message)) { return false } + const ownerGeneration = this.ownerGeneration void this.dispatch(message).then( - (result) => this.send({ id: message.id, ok: true, result }), + (result) => this.send(ownerGeneration, { id: message.id, ok: true, result }), (error) => - this.send({ + this.send(ownerGeneration, { id: message.id, ok: false, error: errorPayload(error) @@ -41,8 +47,10 @@ export class ComputerProviderSupervisorHost { } shutdown(): void { + this.ownerGeneration++ this.sender = null this.macOS.shutdown() + this.desktop.shutdown() } private async dispatch(request: ComputerProviderSupervisorRequest): Promise { @@ -55,10 +63,15 @@ export class ComputerProviderSupervisorHost { case 'macos.release': this.macOS.release(request.params.sessionId) return { released: true } + case 'desktop.execute': + return this.desktop.execute(request.params.request) } } - private send(response: SupervisorResponsePayload): void { + private send(ownerGeneration: number, response: SupervisorResponsePayload): void { + if (ownerGeneration !== this.ownerGeneration) { + return + } this.sender?.({ channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, kind: 'response', diff --git a/src/main/computer/computer-provider-supervisor-protocol.test.ts b/src/main/computer/computer-provider-supervisor-protocol.test.ts new file mode 100644 index 00000000000..cb1f9f262e6 --- /dev/null +++ b/src/main/computer/computer-provider-supervisor-protocol.test.ts @@ -0,0 +1,152 @@ +import { describe, expect, it } from 'vitest' +import { CLIPBOARD_TEXT_WRITE_MAX_BYTES } from '../../shared/clipboard-text' +import { + COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + isComputerProviderSupervisorRequest, + isSupervisedDesktopProviderResult +} from './computer-provider-supervisor-protocol' +import { DESKTOP_PROVIDER_REQUEST_MAX_BYTES } from './desktop-script-provider-request-validation' + +function desktopRequest(request: Record): Record { + return { + channel: COMPUTER_PROVIDER_SUPERVISOR_CHANNEL, + kind: 'request', + id: 1, + method: 'desktop.execute', + params: { request } + } +} + +describe('computer provider supervisor protocol', () => { + it('accepts exact desktop tool requests with validated nested element state', () => { + expect( + isComputerProviderSupervisorRequest( + desktopRequest({ + tool: 'click', + app: 'Text Editor', + element: { + index: 0, + runtimeId: [42, 7], + name: 'Save', + frame: { x: 1, y: 2, width: 30, height: 10 }, + actions: ['invoke'] + }, + click_count: 1, + mouse_button: 'left', + modifiers: 'CmdOrCtrl+Shift', + windowBounds: null + }) + ) + ).toBe(true) + }) + + it('rejects command, path, argument, environment, and envelope injection', () => { + for (const injected of [ + { command: 'sh' }, + { executablePath: '/tmp/provider' }, + { scriptPath: '/tmp/runtime.py' }, + { args: ['--arbitrary'] }, + { env: { PATH: '/tmp' } }, + { timeout: 1 } + ]) { + expect( + isComputerProviderSupervisorRequest(desktopRequest({ tool: 'list_apps', ...injected })) + ).toBe(false) + } + expect( + isComputerProviderSupervisorRequest({ + ...desktopRequest({ tool: 'list_apps' }), + command: 'sh' + }) + ).toBe(false) + }) + + it('rejects malformed and incomplete action requests', () => { + const rejected = [ + { tool: 'unknown' }, + { tool: 'click', app: 'Text Editor' }, + { tool: 'click', app: 'Text Editor', x: 1 }, + { tool: 'click', app: 'Text Editor', x: Number.NaN, y: 2 }, + { tool: 'click', app: 'Text Editor', x: 1, y: 2, modifiers: 'A' }, + { tool: 'scroll', app: 'Text Editor', x: 1, y: 2, direction: 'diagonal' }, + { + tool: 'drag', + app: 'Text Editor', + from_x: 1, + from_y: 2, + to_x: 3 + }, + { + tool: 'set_value', + app: 'Text Editor', + element: { index: -1 }, + value: 'draft' + }, + { + tool: 'click', + app: 'Text Editor', + element: { index: 0, runtimeId: [1, 'untrusted'] } + }, + { + tool: 'get_app_state', + app: 'Text Editor', + windowId: 1, + windowIndex: 2 + } + ] + for (const request of rejected) { + expect(isComputerProviderSupervisorRequest(desktopRequest(request))).toBe(false) + } + }) + + it('enforces the existing paste limit independently of the envelope cap', () => { + const text = 'x'.repeat(CLIPBOARD_TEXT_WRITE_MAX_BYTES + 1) + + expect( + isComputerProviderSupervisorRequest( + desktopRequest({ tool: 'paste_text', app: 'Text Editor', text }) + ) + ).toBe(false) + }) + + it('rejects oversized and non-serializable requests', () => { + expect( + isComputerProviderSupervisorRequest( + desktopRequest({ + tool: 'type_text', + app: 'Text Editor', + text: 'x'.repeat(DESKTOP_PROVIDER_REQUEST_MAX_BYTES) + }) + ) + ).toBe(false) + + const cyclic = { tool: 'get_app_state' } as Record + cyclic.app = cyclic + expect(isComputerProviderSupervisorRequest(desktopRequest(cyclic))).toBe(false) + }) + + it('validates exact desktop result shapes', () => { + expect( + isSupervisedDesktopProviderResult({ + stdout: '{"ok":true}', + stderr: '', + error: null + }) + ).toBe(true) + expect( + isSupervisedDesktopProviderResult({ + stdout: '', + stderr: 'failed', + error: { message: 'failed', killed: false } + }) + ).toBe(true) + expect( + isSupervisedDesktopProviderResult({ + stdout: '', + stderr: '', + error: null, + command: 'sh' + }) + ).toBe(false) + }) +}) diff --git a/src/main/computer/computer-provider-supervisor-protocol.ts b/src/main/computer/computer-provider-supervisor-protocol.ts index 82895df9cc2..b8dac0a1f88 100644 --- a/src/main/computer/computer-provider-supervisor-protocol.ts +++ b/src/main/computer/computer-provider-supervisor-protocol.ts @@ -1,3 +1,6 @@ +import { isDesktopScriptProviderRequest } from './desktop-script-provider-request-validation' +import type { BridgeRequest } from './desktop-script-provider-types' + export const COMPUTER_PROVIDER_SUPERVISOR_CHANNEL = 'orca:computer-provider-supervisor' export type ComputerProviderSupervisorRequest = @@ -15,6 +18,13 @@ export type ComputerProviderSupervisorRequest = method: 'macos.claim' | 'macos.release' params: { sessionId: string } } + | { + channel: typeof COMPUTER_PROVIDER_SUPERVISOR_CHANNEL + kind: 'request' + id: number + method: 'desktop.execute' + params: { request: BridgeRequest } + } export type ComputerProviderSupervisorResponse = | { @@ -50,6 +60,14 @@ export type StartedSupervisedMacOSProvider = { socketToken: string } +export type SupervisedDesktopProviderResult = { + stdout: string + stderr: string + error: { message: string; killed: boolean } | null +} + +const REQUEST_KEYS = new Set(['channel', 'kind', 'id', 'method', 'params']) + export function isComputerProviderSupervisorRequest( value: unknown ): value is ComputerProviderSupervisorRequest { @@ -59,7 +77,8 @@ export function isComputerProviderSupervisorRequest( record.channel !== COMPUTER_PROVIDER_SUPERVISOR_CHANNEL || record.kind !== 'request' || !Number.isSafeInteger(record.id) || - typeof record.method !== 'string' + typeof record.method !== 'string' || + !hasOnlyKeys(record, REQUEST_KEYS) ) { return false } @@ -77,6 +96,9 @@ export function isComputerProviderSupervisorRequest( Object.keys(params).length === 1 ) } + if (record.method === 'desktop.execute') { + return Object.keys(params).length === 1 && isDesktopScriptProviderRequest(params.request) + } return false } @@ -115,11 +137,41 @@ export function isStartedSupervisedMacOSProvider( ) } +export function isSupervisedDesktopProviderResult( + value: unknown +): value is SupervisedDesktopProviderResult { + const record = messageRecord(value) + if ( + !record || + !hasOnlyKeys(record, new Set(['stdout', 'stderr', 'error'])) || + typeof record.stdout !== 'string' || + typeof record.stderr !== 'string' + ) { + return false + } + if (record.error === null) { + return true + } + const error = messageRecord(record.error) + return ( + !!error && + hasOnlyKeys(error, new Set(['message', 'killed'])) && + typeof error.message === 'string' && + typeof error.killed === 'boolean' + ) +} + function isErrorPayload(value: unknown): value is { code: string; message: string } { const record = messageRecord(value) return !!record && typeof record.code === 'string' && typeof record.message === 'string' } function messageRecord(value: unknown): Record | null { - return value && typeof value === 'object' ? (value as Record) : null + return value && typeof value === 'object' && !Array.isArray(value) + ? (value as Record) + : null +} + +function hasOnlyKeys(value: Record, allowed: Set): boolean { + return Object.keys(value).every((key) => allowed.has(key)) } diff --git a/src/main/computer/desktop-script-provider-bridge.ts b/src/main/computer/desktop-script-provider-bridge.ts index c3c21496d29..63ff560ed2f 100644 --- a/src/main/computer/desktop-script-provider-bridge.ts +++ b/src/main/computer/desktop-script-provider-bridge.ts @@ -1,106 +1,23 @@ -import { execFile } from 'node:child_process' +import { executeSupervisedDesktopProvider } from './computer-provider-supervisor-client' import { RuntimeClientError } from './runtime-client-error' -import type { DesktopScriptPlatform } from './desktop-script-provider-paths' +import type { BridgeRequest } from './desktop-script-provider-types' -const REQUEST_TIMEOUT_MS = 30_000 -const FORCE_KILL_GRACE_MS = 1_000 - -export function execBridge( - platform: DesktopScriptPlatform, - scriptPath: string, - operationPath: string +export async function execBridge( + request: BridgeRequest ): Promise<{ stdout: string; stderr: string }> { - const command = platform === 'windows' ? 'powershell.exe' : 'python3' - const args = - platform === 'windows' - ? [ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-File', - scriptPath, - operationPath - ] - : [scriptPath, operationPath] - return new Promise((resolve, reject) => { - let child: ReturnType | null = null - let settled = false - let forceKillTimer: ReturnType | null = null + const result = await executeSupervisedDesktopProvider(withoutUndefinedValues(request)) + if (result.error) { + throw result.error.killed + ? new RuntimeClientError('action_timeout', result.error.message) + : mapBridgeError(result.error.message) + } + return { stdout: result.stdout, stderr: result.stderr } +} - const clearForceKillTimer = (): void => { - if (forceKillTimer) { - clearTimeout(forceKillTimer) - forceKillTimer = null - } - } - - const finish = ( - error: Error | null, - result?: { stdout: string; stderr: string }, - options: { keepForceKillTimer?: boolean } = {} - ): void => { - if (settled) { - return - } - settled = true - clearTimeout(timeout) - if (!options.keepForceKillTimer) { - clearForceKillTimer() - } - if (error) { - reject(error) - return - } - resolve(result ?? { stdout: '', stderr: '' }) - } - - // Why: native automation can hang inside platform APIs; reject promptly, - // then escalate cleanup if the bridge ignores the graceful termination. - const timeout = setTimeout(() => { - child?.kill('SIGTERM') - forceKillTimer = setTimeout(() => { - child?.kill('SIGKILL') - forceKillTimer = null - }, FORCE_KILL_GRACE_MS) - finish( - new RuntimeClientError( - 'action_timeout', - `desktop provider timed out after ${REQUEST_TIMEOUT_MS}ms` - ), - undefined, - { keepForceKillTimer: true } - ) - }, REQUEST_TIMEOUT_MS) - - try { - child = execFile( - command, - args, - { - env: process.env, - maxBuffer: 20 * 1024 * 1024, - timeout: REQUEST_TIMEOUT_MS, - windowsHide: true - }, - (error, stdout, stderr) => { - if (error) { - const message = stderr.trim() || stdout.trim() || error.message - finish( - error.killed - ? new RuntimeClientError('action_timeout', message) - : mapBridgeError(message) - ) - return - } - finish(null, { stdout, stderr }) - } - ) - child?.once('exit', clearForceKillTimer) - } catch (error) { - finish(mapBridgeError(error instanceof Error ? error.message : String(error))) - } - }) +function withoutUndefinedValues(request: BridgeRequest): BridgeRequest { + return Object.fromEntries( + Object.entries(request).filter(([, value]) => value !== undefined) + ) as BridgeRequest } export function mapBridgeError(message: string): RuntimeClientError { diff --git a/src/main/computer/desktop-script-provider-client.ts b/src/main/computer/desktop-script-provider-client.ts index 5e8e01e0d44..85bc83d1230 100644 --- a/src/main/computer/desktop-script-provider-client.ts +++ b/src/main/computer/desktop-script-provider-client.ts @@ -1,6 +1,3 @@ -import { mkdtemp, rm, writeFile } from 'node:fs/promises' -import { tmpdir } from 'node:os' -import { join } from 'node:path' import type { ComputerActionResult, ComputerListAppsResult, @@ -27,8 +24,7 @@ import { } from './desktop-script-provider-params' import { desktopScriptPlatform, - resolveDesktopScriptProviderPath, - type DesktopScriptPlatform + resolveDesktopScriptProviderPath } from './desktop-script-provider-paths' import type { BridgeRequest, @@ -49,11 +45,6 @@ export class DesktopScriptProviderClient { readonly snapshots = this.snapshotStore.snapshots private providerCapabilities: ComputerProviderCapabilities | null = null - constructor( - private readonly platform: DesktopScriptPlatform = requiredPlatform(), - private readonly scriptPath: string = requiredScriptPath() - ) {} - shutdown(): void { this.snapshotStore.clear() this.providerCapabilities = null @@ -203,27 +194,20 @@ export class DesktopScriptProviderClient { } private async callBridge(request: BridgeRequest): Promise { - const operationDirectory = await mkdtemp(join(tmpdir(), 'orca-computer-use-')) - const operationPath = join(operationDirectory, 'operation.json') + const { stdout, stderr } = await execBridge(request) + let response: BridgeResponse try { - await writeFile(operationPath, JSON.stringify(request), { encoding: 'utf8', mode: 0o600 }) - const { stdout, stderr } = await execBridge(this.platform, this.scriptPath, operationPath) - let response: BridgeResponse - try { - response = JSON.parse(stdout) as BridgeResponse - } catch (error) { - throw new RuntimeClientError( - 'accessibility_error', - `desktop provider returned invalid JSON: ${error instanceof Error ? error.message : String(error)}` - ) - } - if (!response.ok) { - throw mapBridgeError(response.error ?? stderr) - } - return response - } finally { - await rm(operationDirectory, { force: true, recursive: true }) + response = JSON.parse(stdout) as BridgeResponse + } catch (error) { + throw new RuntimeClientError( + 'accessibility_error', + `desktop provider returned invalid JSON: ${error instanceof Error ? error.message : String(error)}` + ) } + if (!response.ok) { + throw mapBridgeError(response.error ?? stderr) + } + return response } private async readCapabilities(): Promise { @@ -254,22 +238,3 @@ export class DesktopScriptProviderClient { return renderSnapshot(response.snapshot, noScreenshot) } } - -function requiredPlatform(): DesktopScriptPlatform { - const platform = desktopScriptPlatform() - if (!platform) { - throw new RuntimeClientError('accessibility_error', 'desktop script provider is not available') - } - return platform -} - -function requiredScriptPath(): string { - const scriptPath = resolveDesktopScriptProviderPath() - if (!scriptPath) { - throw new RuntimeClientError( - 'accessibility_error', - 'desktop script provider script was not found' - ) - } - return scriptPath -} diff --git a/src/main/computer/desktop-script-provider-errors.test.ts b/src/main/computer/desktop-script-provider-errors.test.ts index c03449529fd..aa508b2d60c 100644 --- a/src/main/computer/desktop-script-provider-errors.test.ts +++ b/src/main/computer/desktop-script-provider-errors.test.ts @@ -1,8 +1,7 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import { createDesktopScriptProviderClient, - expectDesktopProviderSubprocessStarted, - mockDesktopProviderSubprocessThatIgnoresTimeout, + mockBridgeExecutionError, mockBridgeResponse, resetDesktopScriptProviderTestHarness, sampleBridgeSnapshot, @@ -117,43 +116,19 @@ describe('DesktopScriptProviderClient errors and capabilities', () => { }) }) - it('rejects when the desktop provider subprocess ignores the exec timeout', async () => { - vi.useFakeTimers() - const kill = vi.fn() - const once = vi.fn() - mockDesktopProviderSubprocessThatIgnoresTimeout({ kill, once }) + it('maps supervised provider process failures', async () => { + mockBridgeExecutionError('appNotFound("Missing")') const client = await createDesktopScriptProviderClient('linux', '/tmp/runtime.py') - const promise = client.listApps() - let settled = false - void promise.catch(() => { - settled = true - }) - await vi.waitFor(expectDesktopProviderSubprocessStarted, { timeout: 1_000 }) - await vi.advanceTimersByTimeAsync(30_001) - await vi.runOnlyPendingTimersAsync() - - expect(settled).toBe(true) - expect(kill).toHaveBeenCalledWith('SIGTERM') - await expect(promise).rejects.toMatchObject({ code: 'action_timeout' }) + await expect(client.listApps()).rejects.toMatchObject({ code: 'app_not_found' }) }) - it('force-kills a timed-out desktop provider subprocess that has not exited', async () => { - vi.useFakeTimers() - const kill = vi.fn() - const once = vi.fn() - mockDesktopProviderSubprocessThatIgnoresTimeout({ kill, once }) + it('preserves killed provider failures as timeouts', async () => { + mockBridgeExecutionError('provider exceeded its deadline', true) const client = await createDesktopScriptProviderClient('linux', '/tmp/runtime.py') - const promise = client.listApps().catch(() => undefined) - await vi.waitFor(expectDesktopProviderSubprocessStarted, { timeout: 1_000 }) - await vi.advanceTimersByTimeAsync(30_001) - expect(kill).toHaveBeenCalledWith('SIGTERM') - - await vi.advanceTimersByTimeAsync(1_000) - expect(kill).toHaveBeenCalledWith('SIGKILL') - await promise + await expect(client.listApps()).rejects.toMatchObject({ code: 'action_timeout' }) }) }) diff --git a/src/main/computer/desktop-script-provider-process-invocation.ts b/src/main/computer/desktop-script-provider-process-invocation.ts new file mode 100644 index 00000000000..3c369d3bc34 --- /dev/null +++ b/src/main/computer/desktop-script-provider-process-invocation.ts @@ -0,0 +1,110 @@ +import { execFile, type ExecFileException } from 'node:child_process' +import { randomUUID } from 'node:crypto' +import { rmSync } from 'node:fs' +import { chmod, mkdtemp, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import type { SupervisedDesktopProviderResult } from './computer-provider-supervisor-protocol' +import { + desktopScriptPlatform, + resolveDesktopScriptProviderPath, + type DesktopScriptPlatform +} from './desktop-script-provider-paths' +import { RuntimeClientError } from './runtime-client-error' + +export const DESKTOP_PROVIDER_MAX_BUFFER_BYTES = 20 * 1024 * 1024 + +export type DesktopProviderExecutionPlan = { + platform: DesktopScriptPlatform + command: string + scriptPath: string + env: NodeJS.ProcessEnv +} + +export type DesktopScriptProviderSupervisorDeps = { + platform: () => DesktopScriptPlatform | null + resolveScriptPath: (platform: DesktopScriptPlatform) => string | null + execFile: typeof execFile + randomUUID: () => string + temporaryDirectory: () => string + mkdtemp: typeof mkdtemp + chmod: typeof chmod + writeFile: typeof writeFile + rmSync: typeof rmSync + setTimer: (callback: () => void, timeoutMs: number) => NodeJS.Timeout + clearTimer: (timer: NodeJS.Timeout) => void +} + +export function desktopProviderExecutionPlan( + deps: DesktopScriptProviderSupervisorDeps +): DesktopProviderExecutionPlan { + const platform = deps.platform() + if (!platform) { + throw new RuntimeClientError( + 'accessibility_error', + 'desktop script provider is not available on this platform' + ) + } + const scriptPath = deps.resolveScriptPath(platform) + if (!scriptPath) { + throw new RuntimeClientError( + 'accessibility_error', + 'desktop script provider script was not found' + ) + } + return { + platform, + command: platform === 'windows' ? 'powershell.exe' : 'python3', + scriptPath, + env: { ...process.env } + } +} + +export function desktopProviderCommandArgs( + plan: DesktopProviderExecutionPlan, + operationPath: string +): string[] { + return plan.platform === 'windows' + ? [ + '-NoProfile', + '-NonInteractive', + '-ExecutionPolicy', + 'Bypass', + '-File', + plan.scriptPath, + operationPath + ] + : [plan.scriptPath, operationPath] +} + +export function desktopProviderExecutionResult( + error: ExecFileException | null, + stdout: string, + stderr: string +): SupervisedDesktopProviderResult { + return { + stdout, + stderr, + error: error + ? { + message: stderr.trim() || stdout.trim() || error.message, + killed: error.killed === true + } + : null + } +} + +export function createDesktopScriptProviderSupervisorDeps(): DesktopScriptProviderSupervisorDeps { + return { + platform: desktopScriptPlatform, + resolveScriptPath: resolveDesktopScriptProviderPath, + execFile, + randomUUID, + temporaryDirectory: tmpdir, + mkdtemp, + chmod, + writeFile, + rmSync, + setTimer: setTimeout, + clearTimer: clearTimeout + } +} diff --git a/src/main/computer/desktop-script-provider-request-validation.ts b/src/main/computer/desktop-script-provider-request-validation.ts new file mode 100644 index 00000000000..2dd5e62dda8 --- /dev/null +++ b/src/main/computer/desktop-script-provider-request-validation.ts @@ -0,0 +1,277 @@ +import { CLIPBOARD_TEXT_WRITE_MAX_BYTES } from '../../shared/clipboard-text' +import { + computerUseClickModifiersValidationMessage, + computerUseHotkeyValidationMessage, + computerUsePressKeyValidationMessage +} from '../../shared/computer-use-key-spec' +import type { BridgeElement, BridgeFrame, BridgeRequest } from './desktop-script-provider-types' + +export const DESKTOP_PROVIDER_REQUEST_MAX_BYTES = 17 * 1024 * 1024 +const SERIALIZED_REQUESTS = new WeakMap() + +const APP_TARGET_KEYS = ['tool', 'app', 'windowId', 'windowIndex', 'noScreenshot', 'restoreWindow'] +const ACTION_BASE_KEYS = [...APP_TARGET_KEYS, 'windowBounds'] +const TOOL_KEYS = new Map>([ + ['handshake', keys('tool')], + ['list_apps', keys('tool')], + ['list_windows', keys('tool', 'app')], + ['get_app_state', keys(...APP_TARGET_KEYS)], + [ + 'click', + keys(...ACTION_BASE_KEYS, 'element', 'x', 'y', 'click_count', 'mouse_button', 'modifiers') + ], + ['perform_secondary_action', keys(...ACTION_BASE_KEYS, 'element', 'action')], + ['scroll', keys(...ACTION_BASE_KEYS, 'element', 'x', 'y', 'direction', 'pages')], + [ + 'drag', + keys(...ACTION_BASE_KEYS, 'fromElement', 'toElement', 'from_x', 'from_y', 'to_x', 'to_y') + ], + ['type_text', keys(...ACTION_BASE_KEYS, 'text')], + ['press_key', keys(...ACTION_BASE_KEYS, 'key')], + ['hotkey', keys(...ACTION_BASE_KEYS, 'key')], + ['paste_text', keys(...ACTION_BASE_KEYS, 'text')], + ['set_value', keys(...ACTION_BASE_KEYS, 'element', 'value')] +]) +const ELEMENT_KEYS = keys( + 'index', + 'runtimeId', + 'automationId', + 'name', + 'controlType', + 'localizedControlType', + 'className', + 'value', + 'isSelected', + 'nativeWindowHandle', + 'frame', + 'actions' +) +const FRAME_KEYS = keys('x', 'y', 'width', 'height') + +export function isDesktopScriptProviderRequest(value: unknown): value is BridgeRequest { + const request = record(value) + if (!request || typeof request.tool !== 'string') { + return false + } + const allowedKeys = TOOL_KEYS.get(request.tool) + if (!allowedKeys || !hasOnlyKeys(request, allowedKeys) || !isWithinRequestLimit(request)) { + return false + } + if (request.tool === 'handshake' || request.tool === 'list_apps') { + return true + } + if (!isNonEmptyString(request.app)) { + return false + } + if (request.tool === 'list_windows') { + return true + } + if (!hasValidAppTarget(request)) { + return false + } + if (request.tool === 'get_app_state') { + return true + } + if (!isOptionalFrameOrNull(request.windowBounds)) { + return false + } + return hasValidAction(request) +} + +export function serializeDesktopScriptProviderRequest(request: BridgeRequest): string { + const cached = SERIALIZED_REQUESTS.get(request) + if (cached !== undefined) { + SERIALIZED_REQUESTS.delete(request) + return cached + } + return JSON.stringify(request) +} + +function hasValidAppTarget(request: Record): boolean { + return ( + isOptionalFiniteNumber(request.windowId) && + isOptionalFiniteNumber(request.windowIndex) && + !(request.windowId !== undefined && request.windowIndex !== undefined) && + isOptionalBoolean(request.noScreenshot) && + isOptionalBoolean(request.restoreWindow) + ) +} + +function hasValidAction(request: Record): boolean { + switch (request.tool) { + case 'click': + return ( + isElementOrCoordinateTarget(request, 'element', 'x', 'y') && + isOptionalPositiveInteger(request.click_count) && + isOptionalEnum(request.mouse_button, ['left', 'right', 'middle']) && + isValidClickModifiers(request.modifiers) + ) + case 'perform_secondary_action': + return isElement(request.element) && isNonEmptyString(request.action) + case 'scroll': + return ( + isElementOrCoordinateTarget(request, 'element', 'x', 'y') && + isOptionalEnum(request.direction, ['up', 'down', 'left', 'right'], true) && + isOptionalPositiveNumber(request.pages) + ) + case 'drag': + return isDragTarget(request) + case 'type_text': + return isNonEmptyString(request.text) + case 'press_key': + return ( + isNonEmptyString(request.key) && computerUsePressKeyValidationMessage(request.key) === null + ) + case 'hotkey': + return ( + isNonEmptyString(request.key) && computerUseHotkeyValidationMessage(request.key) === null + ) + case 'paste_text': + return ( + isNonEmptyString(request.text) && + Buffer.byteLength(request.text, 'utf8') <= CLIPBOARD_TEXT_WRITE_MAX_BYTES + ) + case 'set_value': + return isElement(request.element) && typeof request.value === 'string' + default: + return false + } +} + +function isValidClickModifiers(value: unknown): value is string | undefined { + return ( + value === undefined || + (isNonEmptyString(value) && computerUseClickModifiersValidationMessage(value) === null) + ) +} + +function isElementOrCoordinateTarget( + request: Record, + elementKey: string, + xKey: string, + yKey: string +): boolean { + const hasElement = request[elementKey] !== undefined + const hasX = request[xKey] !== undefined + const hasY = request[yKey] !== undefined + return ( + hasElement !== (hasX && hasY) && + hasX === hasY && + (!hasElement || isElement(request[elementKey])) && + isOptionalFiniteNumber(request[xKey]) && + isOptionalFiniteNumber(request[yKey]) + ) +} + +function isDragTarget(request: Record): boolean { + const hasElements = request.fromElement !== undefined || request.toElement !== undefined + const coordinates = [request.from_x, request.from_y, request.to_x, request.to_y] + const hasCoordinates = coordinates.some((value) => value !== undefined) + if (hasElements === hasCoordinates) { + return false + } + return hasElements + ? isElement(request.fromElement) && isElement(request.toElement) + : coordinates.every((value) => typeof value === 'number' && Number.isFinite(value)) +} + +function isElement(value: unknown): value is BridgeElement { + const element = record(value) + return ( + !!element && + hasOnlyKeys(element, ELEMENT_KEYS) && + Number.isSafeInteger(element.index) && + (element.index as number) >= 0 && + (element.runtimeId === undefined || + (Array.isArray(element.runtimeId) && + element.runtimeId.every((part) => Number.isSafeInteger(part)))) && + isOptionalString(element.automationId) && + isOptionalString(element.name) && + isOptionalString(element.controlType) && + isOptionalString(element.localizedControlType) && + isOptionalString(element.className) && + isOptionalString(element.value) && + isOptionalBoolean(element.isSelected) && + isOptionalFiniteNumber(element.nativeWindowHandle) && + isOptionalFrameOrNull(element.frame) && + (element.actions === undefined || + (Array.isArray(element.actions) && + element.actions.every((action) => typeof action === 'string'))) + ) +} + +function isOptionalFrameOrNull(value: unknown): value is BridgeFrame | null | undefined { + if (value === undefined || value === null) { + return true + } + const frame = record(value) + return ( + !!frame && + hasOnlyKeys(frame, FRAME_KEYS) && + Number.isFinite(frame.x) && + Number.isFinite(frame.y) && + Number.isFinite(frame.width) && + Number.isFinite(frame.height) + ) +} + +function isWithinRequestLimit(request: Record): boolean { + try { + const serialized = JSON.stringify(request) + const withinLimit = + typeof serialized === 'string' && + Buffer.byteLength(serialized, 'utf8') <= DESKTOP_PROVIDER_REQUEST_MAX_BYTES + if (withinLimit) { + SERIALIZED_REQUESTS.set(request, serialized) + } + return withinLimit + } catch { + return false + } +} + +function isOptionalEnum( + value: unknown, + allowed: readonly string[], + required = false +): value is string | undefined { + return value === undefined ? !required : typeof value === 'string' && allowed.includes(value) +} + +function isOptionalPositiveInteger(value: unknown): value is number | undefined { + return value === undefined || (Number.isSafeInteger(value) && (value as number) > 0) +} + +function isOptionalPositiveNumber(value: unknown): value is number | undefined { + return value === undefined || (typeof value === 'number' && Number.isFinite(value) && value > 0) +} + +function hasOnlyKeys(value: Record, allowed: Set): boolean { + return Object.keys(value).every((key) => allowed.has(key)) +} + +function keys(...values: string[]): Set { + return new Set(values) +} + +function isNonEmptyString(value: unknown): value is string { + return typeof value === 'string' && value.length > 0 +} + +function isOptionalString(value: unknown): value is string | undefined { + return value === undefined || typeof value === 'string' +} + +function isOptionalFiniteNumber(value: unknown): value is number | undefined { + return value === undefined || (typeof value === 'number' && Number.isFinite(value)) +} + +function isOptionalBoolean(value: unknown): value is boolean | undefined { + return value === undefined || typeof value === 'boolean' +} + +function record(value: unknown): Record | null { + return value !== null && typeof value === 'object' && !Array.isArray(value) + ? (value as Record) + : null +} diff --git a/src/main/computer/desktop-script-provider-supervisor.test.ts b/src/main/computer/desktop-script-provider-supervisor.test.ts new file mode 100644 index 00000000000..dce22ab2184 --- /dev/null +++ b/src/main/computer/desktop-script-provider-supervisor.test.ts @@ -0,0 +1,317 @@ +import type { ExecFileException } from 'node:child_process' +import { EventEmitter } from 'node:events' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + DESKTOP_PROVIDER_MAX_BUFFER_BYTES, + type DesktopScriptProviderSupervisorDeps +} from './desktop-script-provider-process-invocation' +import { + DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS, + DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS, + DesktopScriptProviderSupervisor +} from './desktop-script-provider-supervisor' + +type ExecCallback = (error: ExecFileException | null, stdout: string, stderr: string) => void + +class FakeChild extends EventEmitter { + pid: number | undefined = 4321 + kill = vi.fn(() => true) +} + +function deferred() { + let resolve = (_value: T): void => {} + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +async function flushPreparation(): Promise { + for (let turn = 0; turn < 8; turn++) { + await Promise.resolve() + } +} + +describe('DesktopScriptProviderSupervisor', () => { + let callback: ExecCallback | null + let child: FakeChild + let deps: DesktopScriptProviderSupervisorDeps + + beforeEach(() => { + vi.useFakeTimers() + callback = null + child = new FakeChild() + deps = { + platform: vi.fn(() => 'linux'), + resolveScriptPath: vi.fn(() => '/fixed/runtime.py'), + execFile: vi.fn((_command, _args, _options, execCallback) => { + callback = execCallback as ExecCallback + return child as never + }) as never, + randomUUID: vi.fn(() => 'operation-1'), + temporaryDirectory: vi.fn(() => '/private/tmp'), + mkdtemp: vi.fn( + async () => '/private/tmp/orca-computer-use-operation' + ) as unknown as DesktopScriptProviderSupervisorDeps['mkdtemp'], + chmod: vi.fn(async () => undefined), + writeFile: vi.fn(async () => undefined), + rmSync: vi.fn(), + setTimer: (handler, timeoutMs) => setTimeout(handler, timeoutMs), + clearTimer: (timer) => clearTimeout(timer) + } + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('constructs the fixed Linux invocation and waits for callback plus exit', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + await flushPreparation() + + const operationPath = join('/private/tmp/orca-computer-use-operation', 'operation.json') + expect(deps.mkdtemp).toHaveBeenCalledWith(join('/private/tmp', 'orca-computer-use-')) + expect(deps.chmod).toHaveBeenCalledWith('/private/tmp/orca-computer-use-operation', 0o700) + expect(deps.writeFile).toHaveBeenCalledWith(operationPath, '{"tool":"list_apps"}', { + encoding: 'utf8', + mode: 0o600 + }) + expect(deps.execFile).toHaveBeenCalledWith( + 'python3', + ['/fixed/runtime.py', operationPath], + expect.objectContaining({ + encoding: 'utf8', + maxBuffer: DESKTOP_PROVIDER_MAX_BUFFER_BYTES, + windowsHide: true + }), + expect.any(Function) + ) + + callback!(null, '{"ok":true}', '') + let settled = false + void result.finally(() => { + settled = true + }) + await Promise.resolve() + expect(settled).toBe(false) + expect(deps.rmSync).not.toHaveBeenCalled() + + child.emit('exit', 0, null) + await expect(result).resolves.toEqual({ + stdout: '{"ok":true}', + stderr: '', + error: null + }) + expect(deps.rmSync).toHaveBeenCalledWith('/private/tmp/orca-computer-use-operation', { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 100 + }) + }) + + it('waits for the exec callback when exit arrives first', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + await flushPreparation() + + child.emit('exit', 0, null) + let settled = false + void result.finally(() => { + settled = true + }) + await Promise.resolve() + expect(settled).toBe(false) + expect(deps.rmSync).not.toHaveBeenCalled() + + callback!(null, '{"ok":true}', '') + await expect(result).resolves.toMatchObject({ error: null }) + expect(deps.rmSync).toHaveBeenCalledTimes(1) + }) + + it('reuses the main-selected platform, script, and environment plan', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const first = supervisor.execute({ tool: 'list_apps' }) + await flushPreparation() + callback!(null, '{"ok":true}', '') + child.emit('exit', 0, null) + await first + + const second = supervisor.execute({ tool: 'list_apps' }) + await flushPreparation() + callback!(null, '{"ok":true}', '') + child.emit('exit', 0, null) + await second + + expect(deps.platform).toHaveBeenCalledTimes(1) + expect(deps.resolveScriptPath).toHaveBeenCalledTimes(1) + expect(deps.execFile).toHaveBeenCalledTimes(2) + }) + + it('escalates a Linux deadline while retaining state until confirmed exit', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toMatchObject({ code: 'action_timeout' }) + await flushPreparation() + + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS) + await rejection + expect(child.kill).toHaveBeenCalledWith('SIGTERM') + expect(deps.rmSync).not.toHaveBeenCalled() + expect(child.listenerCount('exit')).toBe(1) + + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS) + expect(child.kill).toHaveBeenLastCalledWith('SIGKILL') + expect(deps.rmSync).not.toHaveBeenCalled() + + child.emit('exit', null, 'SIGKILL') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + expect(child.listenerCount('exit')).toBe(0) + }) + + it('cancels Linux force escalation after the timed-out child exits', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toMatchObject({ code: 'action_timeout' }) + await flushPreparation() + + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS) + await rejection + child.emit('exit', null, 'SIGTERM') + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS) + + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.kill).toHaveBeenCalledWith('SIGTERM') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + }) + + it('uses forceful Windows termination without POSIX signal escalation', async () => { + vi.mocked(deps.platform).mockReturnValue('windows') + vi.mocked(deps.resolveScriptPath).mockReturnValue('/fixed/runtime.ps1') + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toMatchObject({ code: 'action_timeout' }) + await flushPreparation() + + expect(deps.execFile).toHaveBeenCalledWith( + 'powershell.exe', + [ + '-NoProfile', + '-NonInteractive', + '-ExecutionPolicy', + 'Bypass', + '-File', + '/fixed/runtime.ps1', + join('/private/tmp/orca-computer-use-operation', 'operation.json') + ], + expect.any(Object), + expect.any(Function) + ) + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS) + await rejection + expect(child.kill).toHaveBeenCalledTimes(1) + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + + await vi.advanceTimersByTimeAsync(DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS) + expect(child.kill).toHaveBeenCalledTimes(1) + child.emit('exit', null, 'SIGTERM') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + }) + + it('cancels an operation before directory creation completes', async () => { + const directory = deferred() + vi.mocked(deps.mkdtemp).mockReturnValueOnce(directory.promise) + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toThrow('supervisor shut down') + + supervisor.shutdown() + await rejection + expect(deps.execFile).not.toHaveBeenCalled() + + directory.resolve('/private/tmp/orca-computer-use-late') + await flushPreparation() + expect(deps.rmSync).toHaveBeenCalledWith('/private/tmp/orca-computer-use-late', { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 100 + }) + expect(deps.execFile).not.toHaveBeenCalled() + }) + + it('cancels after directory creation without spawning after an async setup resumes', async () => { + const chmod = deferred() + vi.mocked(deps.chmod).mockReturnValueOnce(chmod.promise) + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toThrow('supervisor shut down') + await flushPreparation() + expect(deps.chmod).toHaveBeenCalled() + + supervisor.shutdown() + await rejection + expect(deps.rmSync).toHaveBeenCalledTimes(1) + chmod.resolve() + await flushPreparation() + expect(deps.writeFile).not.toHaveBeenCalled() + expect(deps.execFile).not.toHaveBeenCalled() + }) + + it('retains a spawned operation and its file until shutdown confirms exit', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toThrow('supervisor shut down') + await flushPreparation() + + supervisor.shutdown() + await rejection + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + expect(deps.rmSync).not.toHaveBeenCalled() + expect(child.listenerCount('exit')).toBe(1) + + child.emit('exit', null, 'SIGKILL') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + expect(child.listenerCount('exit')).toBe(0) + }) + + it('handles a child without a pid and retains an asynchronous error listener', async () => { + child.pid = undefined + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + await flushPreparation() + + await expect(result).rejects.toThrow('did not report a pid') + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + expect(() => child.emit('error', new Error('spawn failed asynchronously'))).not.toThrow() + }) + + it('rejects a registered child error but cleans up only after exit', async () => { + const supervisor = new DesktopScriptProviderSupervisor(deps) + const result = supervisor.execute({ tool: 'list_apps' }) + const rejection = expect(result).rejects.toThrow('provider failed: handle failed') + await flushPreparation() + + child.emit('error', new Error('handle failed')) + await rejection + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + expect(deps.rmSync).not.toHaveBeenCalled() + + child.emit('exit', null, 'SIGKILL') + expect(deps.rmSync).toHaveBeenCalledTimes(1) + }) + + it('removes the private directory when setup fails before spawn', async () => { + vi.mocked(deps.writeFile).mockRejectedValueOnce(new Error('operation write failed')) + const supervisor = new DesktopScriptProviderSupervisor(deps) + + await expect(supervisor.execute({ tool: 'list_apps' })).rejects.toThrow( + 'operation write failed' + ) + expect(deps.execFile).not.toHaveBeenCalled() + expect(deps.rmSync).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/computer/desktop-script-provider-supervisor.ts b/src/main/computer/desktop-script-provider-supervisor.ts new file mode 100644 index 00000000000..508480fd08f --- /dev/null +++ b/src/main/computer/desktop-script-provider-supervisor.ts @@ -0,0 +1,320 @@ +import type { ChildProcess } from 'node:child_process' +import { join } from 'node:path' +import type { SupervisedDesktopProviderResult } from './computer-provider-supervisor-protocol' +import { + createDesktopScriptProviderSupervisorDeps, + desktopProviderCommandArgs, + desktopProviderExecutionPlan, + desktopProviderExecutionResult, + DESKTOP_PROVIDER_MAX_BUFFER_BYTES, + type DesktopProviderExecutionPlan, + type DesktopScriptProviderSupervisorDeps +} from './desktop-script-provider-process-invocation' +import { serializeDesktopScriptProviderRequest } from './desktop-script-provider-request-validation' +import type { BridgeRequest } from './desktop-script-provider-types' +import { RuntimeClientError } from './runtime-client-error' + +export const DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS = 30_000 +export const DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS = 1_000 + +type DesktopProviderOperation = { + id: string + platform: DesktopProviderExecutionPlan['platform'] + child: ChildProcess | null + directory: string | null + settled: boolean + terminating: boolean + exitConfirmed: boolean + callbackResult: SupervisedDesktopProviderResult | null + deadlineTimer: NodeJS.Timeout | null + forceKillTimer: NodeJS.Timeout | null + resolve: (result: SupervisedDesktopProviderResult) => void + reject: (error: Error) => void + cleanupListeners: () => void +} + +function ignoreUnownedChildError(): void {} + +export class DesktopScriptProviderSupervisor { + private readonly operations = new Map() + private executionPlan: DesktopProviderExecutionPlan | null = null + + constructor( + private readonly deps: DesktopScriptProviderSupervisorDeps = createDesktopScriptProviderSupervisorDeps() + ) {} + + execute(request: BridgeRequest): Promise { + const plan = this.executionPlan ?? desktopProviderExecutionPlan(this.deps) + this.executionPlan = plan + let resolve = (_result: SupervisedDesktopProviderResult): void => {} + let reject = (_error: Error): void => {} + const result = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise + reject = rejectPromise + }) + const operation: DesktopProviderOperation = { + id: this.deps.randomUUID(), + platform: plan.platform, + child: null, + directory: null, + settled: false, + terminating: false, + exitConfirmed: false, + callbackResult: null, + deadlineTimer: null, + forceKillTimer: null, + resolve, + reject, + cleanupListeners: () => {} + } + this.operations.set(operation.id, operation) + void this.prepareAndSpawn(operation, plan, request) + return result + } + + shutdown(): void { + for (const operation of this.operations.values()) { + operation.terminating = true + this.clearOperationTimers(operation) + this.rejectRequest( + operation, + new RuntimeClientError('accessibility_error', 'desktop provider supervisor shut down') + ) + if (!operation.child || operation.exitConfirmed) { + this.finishOperation(operation) + continue + } + try { + operation.child.kill('SIGKILL') + } catch {} + } + } + + private async prepareAndSpawn( + operation: DesktopProviderOperation, + plan: DesktopProviderExecutionPlan, + request: BridgeRequest + ): Promise { + try { + const directory = await this.deps.mkdtemp( + join(this.deps.temporaryDirectory(), 'orca-computer-use-') + ) + operation.directory = directory + if (!this.isRegistered(operation) || operation.terminating) { + this.removeOperationDirectory(operation) + return + } + await this.deps.chmod(directory, 0o700) + if (!this.isRegistered(operation) || operation.terminating) { + this.removeOperationDirectory(operation) + return + } + const operationPath = join(directory, 'operation.json') + await this.deps.writeFile(operationPath, serializeDesktopScriptProviderRequest(request), { + encoding: 'utf8', + mode: 0o600 + }) + if (!this.isRegistered(operation) || operation.terminating) { + this.removeOperationDirectory(operation) + return + } + this.spawnOperation(operation, plan, operationPath) + } catch (error) { + if (this.isRegistered(operation)) { + this.failBeforeSpawn(operation, error) + } else { + this.removeOperationDirectory(operation) + } + } + } + + private spawnOperation( + operation: DesktopProviderOperation, + plan: DesktopProviderExecutionPlan, + operationPath: string + ): void { + const args = desktopProviderCommandArgs(plan, operationPath) + let child: ChildProcess | null = null + try { + child = this.deps.execFile( + plan.command, + args, + { + env: plan.env, + encoding: 'utf8', + maxBuffer: DESKTOP_PROVIDER_MAX_BUFFER_BYTES, + windowsHide: true + }, + (error, stdout, stderr) => { + operation.callbackResult = desktopProviderExecutionResult(error, stdout, stderr) + this.completeAfterCallback(operation) + } + ) + child.on('error', ignoreUnownedChildError) + if (typeof child.pid !== 'number' || child.pid <= 0) { + throw new Error('desktop provider process did not report a pid') + } + } catch (error) { + try { + child?.kill('SIGKILL') + } catch {} + this.failBeforeSpawn(operation, error) + return + } + + operation.child = child + const onError = (error: Error): void => this.handleChildError(operation, error) + const onExit = (): void => this.handleChildExit(operation) + child.on('error', onError) + child.once('exit', onExit) + child.off('error', ignoreUnownedChildError) + operation.cleanupListeners = () => { + child.off('error', onError) + child.off('exit', onExit) + child.off('error', ignoreUnownedChildError) + child.on('error', ignoreUnownedChildError) + } + operation.deadlineTimer = this.deps.setTimer( + () => this.handleDeadline(operation), + DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS + ) + } + + private completeAfterCallback(operation: DesktopProviderOperation): void { + if ( + !this.isRegistered(operation) || + operation.settled || + !operation.exitConfirmed || + !operation.callbackResult + ) { + return + } + const result = operation.callbackResult + operation.settled = true + this.finishOperation(operation) + operation.resolve(result) + } + + private handleChildError(operation: DesktopProviderOperation, error: Error): void { + if (!this.isRegistered(operation) || operation.terminating) { + return + } + operation.terminating = true + this.clearOperationTimers(operation) + this.rejectRequest( + operation, + new RuntimeClientError('accessibility_error', `desktop provider failed: ${error.message}`) + ) + try { + operation.child?.kill('SIGKILL') + } catch {} + } + + private handleChildExit(operation: DesktopProviderOperation): void { + if (!this.isRegistered(operation)) { + return + } + operation.exitConfirmed = true + if (operation.settled) { + this.finishOperation(operation) + return + } + this.completeAfterCallback(operation) + } + + private handleDeadline(operation: DesktopProviderOperation): void { + if (!this.isRegistered(operation) || operation.terminating) { + return + } + operation.terminating = true + operation.deadlineTimer = null + this.rejectRequest( + operation, + new RuntimeClientError( + 'action_timeout', + `desktop provider timed out after ${DESKTOP_PROVIDER_REQUEST_TIMEOUT_MS}ms` + ) + ) + if (operation.exitConfirmed) { + this.finishOperation(operation) + return + } + try { + operation.child?.kill(operation.platform === 'windows' ? 'SIGKILL' : 'SIGTERM') + } catch {} + if (operation.platform === 'windows') { + return + } + operation.forceKillTimer = this.deps.setTimer(() => { + operation.forceKillTimer = null + try { + operation.child?.kill('SIGKILL') + } catch {} + }, DESKTOP_PROVIDER_FORCE_KILL_GRACE_MS) + } + + private failBeforeSpawn(operation: DesktopProviderOperation, error: unknown): void { + if (!this.isRegistered(operation)) { + this.removeOperationDirectory(operation) + return + } + this.operations.delete(operation.id) + this.removeOperationDirectory(operation) + this.rejectRequest( + operation, + new RuntimeClientError( + 'accessibility_error', + error instanceof Error ? error.message : String(error) + ) + ) + } + + private finishOperation(operation: DesktopProviderOperation): void { + if (!this.isRegistered(operation)) { + return + } + this.operations.delete(operation.id) + this.clearOperationTimers(operation) + operation.cleanupListeners() + this.removeOperationDirectory(operation) + } + + private rejectRequest(operation: DesktopProviderOperation, error: Error): void { + if (operation.settled) { + return + } + operation.settled = true + operation.reject(error) + } + + private clearOperationTimers(operation: DesktopProviderOperation): void { + if (operation.deadlineTimer) { + this.deps.clearTimer(operation.deadlineTimer) + operation.deadlineTimer = null + } + if (operation.forceKillTimer) { + this.deps.clearTimer(operation.forceKillTimer) + operation.forceKillTimer = null + } + } + + private removeOperationDirectory(operation: DesktopProviderOperation): void { + if (!operation.directory) { + return + } + const directory = operation.directory + operation.directory = null + try { + this.deps.rmSync(directory, { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 100 + }) + } catch {} + } + + private isRegistered(operation: DesktopProviderOperation): boolean { + return this.operations.get(operation.id) === operation + } +} diff --git a/src/main/computer/desktop-script-provider-test-harness.ts b/src/main/computer/desktop-script-provider-test-harness.ts index 2cbd1e9a776..b715db459d6 100644 --- a/src/main/computer/desktop-script-provider-test-harness.ts +++ b/src/main/computer/desktop-script-provider-test-harness.ts @@ -1,80 +1,52 @@ import { expect, vi } from 'vitest' -const { execFileMock, operationFiles, mkdtempMock, rmMock, writeFileMock } = vi.hoisted(() => { - const files = new Map() - return { - execFileMock: vi.fn(), - operationFiles: files, - mkdtempMock: vi.fn(async (prefix: string) => `${prefix}${files.size}`), - rmMock: vi.fn(async () => undefined), - writeFileMock: vi.fn(async (filePath: string, data: string | Buffer) => { - files.set(filePath, Buffer.isBuffer(data) ? data.toString('utf8') : data) - }) - } -}) - -vi.mock('child_process', () => ({ - execFile: execFileMock +const { executeSupervisedDesktopProviderMock } = vi.hoisted(() => ({ + executeSupervisedDesktopProviderMock: vi.fn() })) -vi.mock('fs/promises', () => ({ - mkdtemp: mkdtempMock, - rm: rmMock, - writeFile: writeFileMock +vi.mock('./computer-provider-supervisor-client', () => ({ + executeSupervisedDesktopProvider: executeSupervisedDesktopProviderMock })) export async function createDesktopScriptProviderClient( - platform: 'linux' | 'windows', - executablePath: string + _platform: 'linux' | 'windows', + _executablePath: string ) { const { DesktopScriptProviderClient } = await import('./desktop-script-provider-client') - return new DesktopScriptProviderClient(platform, executablePath) + return new DesktopScriptProviderClient() } export function resetDesktopScriptProviderTestHarness(): void { vi.useRealTimers() - execFileMock.mockReset() - operationFiles.clear() - mkdtempMock.mockClear() - rmMock.mockClear() - writeFileMock.mockClear() -} - -export function mockDesktopProviderSubprocessThatIgnoresTimeout({ - kill, - once -}: { - kill: (signal: string) => void - once: () => void -}): void { - execFileMock.mockImplementationOnce(() => ({ kill, once }) as never) -} - -export function expectDesktopProviderSubprocessStarted(): void { - expect(execFileMock).toHaveBeenCalled() + executeSupervisedDesktopProviderMock.mockReset() } export function expectDesktopProviderSubprocessStartCount(count: number): void { - expect(execFileMock).toHaveBeenCalledTimes(count) + expect(executeSupervisedDesktopProviderMock).toHaveBeenCalledTimes(count) +} + +export function mockBridgeExecutionError(message: string, killed = false): void { + executeSupervisedDesktopProviderMock.mockResolvedValueOnce({ + stdout: '', + stderr: message, + error: { message, killed } + }) } export function mockBridgeResponse( response: unknown, inspectOperation?: (operation: Record) => void ): void { - execFileMock.mockImplementationOnce((_command, _args, _options, callback) => { - const operationPath = _args?.at(-1) - if (inspectOperation && typeof operationPath === 'string') { - const operation = operationFiles.get(operationPath) - if (!operation) { - throw new Error(`Missing mocked operation file: ${operationPath}`) + executeSupervisedDesktopProviderMock.mockImplementationOnce( + async (operation: Record) => { + inspectOperation?.(operation) + return { + stdout: JSON.stringify(response), + stderr: '', + error: null } - inspectOperation(JSON.parse(operation) as Record) } - const done = callback as (error: Error | null, stdout: string, stderr: string) => void - done(null, JSON.stringify(response), '') - return null as never - }) + ) } export function sampleBridgeSnapshot(name: string, value: string) {