fix(codex): preserve native confirmation approval replies

plugins/opengui/src/confirmation.ts

@@ -1,18 +1,22 @@
-import { execFile } from 'node:child_process'
+import { execFile, type ExecFileException } from 'node:child_process'
 import type { ExternalSideEffect } from './codex/service.ts'
 
 export type ConfirmAction = (effect: ExternalSideEffect, args: Record<string, unknown>, signal: AbortSignal) => Promise<boolean>
 
 /** Fixed AppleScript; untrusted action descriptions are passed as argv, never code. */
-const SCRIPT = [
+export const CONFIRMATION_SCRIPT = [
   'on run argv',
   'set messageText to item 1 of argv',
   'try',
-  'display dialog messageText with title "OpenGUI — Confirm Android action" buttons {"Cancel", "Allow once"} default button "Cancel" cancel button "Cancel" giving up after 60',
-  'if gave up of result then return "cancel"',
-  'return button returned of result',
-  'on error',
-  'return "cancel"',
+  'activate',
+  'set dialogReply to display dialog messageText with title "OpenGUI — Confirm Android action" buttons {"Cancel", "Allow once"} default button "Cancel" cancel button "Cancel" giving up after 60',
+  // AppleScript control statements can clear implicit `result`; retain the reply explicitly.
+  'if gave up of dialogReply then return "timeout"',
+  'return button returned of dialogReply',
+  'on error errorMessage number errorNumber',
+  'if errorNumber is -128 then return "cancel"',
+  'if errorNumber is -1712 then return "timeout"',
+  'return "unavailable"',
   'end try',
   'end run',
 ].join('\n')
@@ -23,14 +27,28 @@ export const confirmAction: ConfirmAction = async (effect, args, signal) => {
 }
 
 export async function confirmLocalSetup(description: string, signal: AbortSignal): Promise<boolean> {
-  if (process.platform !== 'darwin') return false
   signal.throwIfAborted()
-  return new Promise<boolean>((resolve, reject) => {
-    execFile('/usr/bin/osascript', ['-e', SCRIPT, description], {
-      shell: false, signal, timeout: 65_000, maxBuffer: 8192,
-    }, (error, stdout) => {
-      if (signal.aborted) reject(signal.reason)
-      else resolve(error === null && stdout.trim() === 'Allow once')
+  if (process.platform !== 'darwin') throw new Error('opengui: confirmation UI unavailable')
+  let output: string
+  try {
+    output = await new Promise<string>((resolve, reject) => {
+      execFile('/usr/bin/osascript', ['-e', CONFIRMATION_SCRIPT, description], {
+        shell: false, signal, timeout: 65_000, maxBuffer: 8192,
+      }, (error, stdout) => error ? reject(error) : resolve(stdout))
     })
-  })
+  } catch (error) {
+    signal.throwIfAborted()
+    const failure = error as ExecFileException
+    if (failure?.killed && failure.signal === 'SIGTERM') throw new Error('opengui: confirmation timed out')
+    // Process errors may contain the full command and private action arguments.
+    throw new Error('opengui: confirmation UI unavailable')
+  }
+  signal.throwIfAborted()
+  switch (output.trim()) {
+    case 'Allow once': return true
+    case 'cancel':
+    case 'Cancel': return false
+    case 'timeout': throw new Error('opengui: confirmation timed out')
+    default: throw new Error('opengui: confirmation UI unavailable')
+  }
 }

plugins/opengui/tests/confirmation-applescript.spec.ts

@@ -0,0 +1,27 @@
+import { execFileSync } from 'node:child_process'
+import { describe, expect, it } from 'vitest'
+import { CONFIRMATION_SCRIPT } from '../src/confirmation.ts'
+
+// Run actual AppleScript control flow with a synthetic dialog result. No UI or device is opened.
+function runSyntheticDialog(body: string): string {
+  const dialogLine = CONFIRMATION_SCRIPT.split('\n').find(line => line.includes('display dialog messageText'))!
+  const replacement = dialogLine.startsWith('set dialogReply to ') ? 'set dialogReply to my syntheticDialog()' : 'my syntheticDialog()'
+  const script = `on syntheticDialog()\n${body}\nend syntheticDialog\n`
+    + CONFIRMATION_SCRIPT.replace('\nactivate\n', '\n').replace(dialogLine, replacement)
+  return execFileSync('/usr/bin/osascript', ['-e', script, 'synthetic "quoted" argument'], {
+    encoding: 'utf8', timeout: 5000, maxBuffer: 8192,
+  }).trim()
+}
+
+describe.runIf(process.platform === 'darwin')('actual AppleScript response handling', () => {
+  it('preserves Allow once after checking the timeout flag', () => {
+    expect(runSyntheticDialog('return {button returned:"Allow once", gave up:false}')).toBe('Allow once')
+  })
+  it('handles timeout without reading a missing button field', () => {
+    expect(runSyntheticDialog('return {gave up:true}')).toBe('timeout')
+  })
+  it('keeps explicit cancellation separate from a UI error', () => {
+    expect(runSyntheticDialog('error number -128')).toBe('cancel')
+    expect(runSyntheticDialog('error "synthetic private detail" number -2753')).toBe('unavailable')
+  })
+})

plugins/opengui/tests/confirmation.spec.ts

@@ -0,0 +1,109 @@
+import type { ExecFileException, ExecFileOptions } from 'node:child_process'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { confirmAction, confirmLocalSetup } from '../src/confirmation.ts'
+
+const { run } = vi.hoisted(() => ({
+  run: vi.fn<(file: string, args: string[], options: ExecFileOptions,
+    callback: (error: ExecFileException | null, stdout: string, stderr: string) => void) => void>(),
+}))
+vi.mock('node:child_process', () => ({ execFile: run }))
+
+const platform = Object.getOwnPropertyDescriptor(process, 'platform')!
+const signal = () => new AbortController().signal
+function complete(output: string, error: ExecFileException | null = null) {
+  run.mockImplementation((_file, _args, _options, callback) => callback(error, output, 'private stderr'))
+}
+
+beforeEach(() => {
+  run.mockReset()
+  Object.defineProperty(process, 'platform', { ...platform, value: 'darwin' })
+})
+afterEach(() => Object.defineProperty(process, 'platform', platform))
+
+describe('native confirmation', () => {
+  it('foregrounds the dialog and passes untrusted descriptions only as argv', async () => {
+    complete('Allow once\n')
+    const description = 'Private " text\n$(touch injected) `whoami` ; display dialog "injected"'
+    const requestSignal = signal()
+    await expect(confirmLocalSetup(description, requestSignal)).resolves.toBe(true)
+    expect(run).toHaveBeenCalledOnce()
+    const [file, args, options] = run.mock.calls[0]!
+    expect(file).toBe('/usr/bin/osascript')
+    expect(args).toHaveLength(3)
+    expect(args[0]).toBe('-e')
+    expect(args[2]).toBe(description)
+    const script = args[1]!
+    expect(script).not.toContain(description)
+    expect(script).toContain('activate\nset dialogReply to display dialog')
+    expect(script).toContain('default button "Cancel" cancel button "Cancel"')
+    expect(script).toContain('if errorNumber is -128 then return "cancel"')
+    expect(script).toContain('if gave up of dialogReply then return "timeout"')
+    expect(script).toContain('if errorNumber is -1712 then return "timeout"')
+    expect(options).toMatchObject({ shell: false, signal: requestSignal, timeout: 65_000, maxBuffer: 8192 })
+  })
+
+  it('preserves the action context in the confirmation request', async () => {
+    complete('Allow once')
+    await expect(confirmAction('send', { text: 'Hello' }, signal())).resolves.toBe(true)
+    const args = run.mock.calls[0]![1]
+    expect(args[1]).toContain('Confirm Android action')
+    expect(args[2]).toContain('Allow one send action on the selected Android phone?')
+    expect(args[2]).toContain('"text": "Hello"')
+  })
+
+  it.each(['cancel', 'Cancel\n'])('returns false for explicit cancellation %j', async output => {
+    complete(output)
+    await expect(confirmLocalSetup('request', signal())).resolves.toBe(false)
+  })
+
+  it('reports a dialog timeout separately from user cancellation', async () => {
+    complete('timeout\n')
+    await expect(confirmLocalSetup('request', signal())).rejects.toThrow('opengui: confirmation timed out')
+    expect(run).toHaveBeenCalledOnce()
+  })
+
+  it('reports the child process timeout without exposing command arguments', async () => {
+    complete('', Object.assign(new Error('private command arguments'), { killed: true, signal: 'SIGTERM' as const }))
+    await expect(confirmLocalSetup('private request', signal())).rejects.toThrow(/^opengui: confirmation timed out$/)
+  })
+
+  it.each(['unavailable', '', 'Allow', 'true', 'Allow once\nprivate output'])('rejects unexpected or unavailable UI output %j', async output => {
+    complete(output)
+    await expect(confirmLocalSetup('private request', signal())).rejects.toThrow(/^opengui: confirmation UI unavailable$/)
+  })
+
+  it('redacts subprocess failures even when stdout claims approval', async () => {
+    complete('Allow once', Object.assign(new Error('private command arguments'), { code: 1 }))
+    await expect(confirmLocalSetup('private request', signal())).rejects.toThrow(/^opengui: confirmation UI unavailable$/)
+    expect(run).toHaveBeenCalledOnce()
+  })
+
+  it('redacts synchronous subprocess launch failures', async () => {
+    run.mockImplementation(() => { throw new Error('private invalid arguments') })
+    await expect(confirmLocalSetup('private request', signal())).rejects.toThrow(/^opengui: confirmation UI unavailable$/)
+  })
+
+  it('preserves an existing abort reason without opening a dialog', async () => {
+    const controller = new AbortController()
+    const reason = new Error('caller cancelled')
+    controller.abort(reason)
+    await expect(confirmLocalSetup('request', controller.signal)).rejects.toBe(reason)
+    expect(run).not.toHaveBeenCalled()
+  })
+
+  it.each([null, new Error('process aborted')])('preserves an in-flight abort over a process result %j', async error => {
+    const controller = new AbortController()
+    const reason = new Error('caller cancelled')
+    run.mockImplementation((_file, _args, _options, callback) => {
+      controller.abort(reason)
+      callback(error, 'Allow once', '')
+    })
+    await expect(confirmLocalSetup('request', controller.signal)).rejects.toBe(reason)
+  })
+
+  it('reports unsupported UI hosts without presenting them as cancellation', async () => {
+    Object.defineProperty(process, 'platform', { ...platform, value: 'linux' })
+    await expect(confirmLocalSetup('request', signal())).rejects.toThrow(/^opengui: confirmation UI unavailable$/)
+    expect(run).not.toHaveBeenCalled()
+  })
+})