From 106b10244bcd49deff0741d07f2714d522a59921 Mon Sep 17 00:00:00 2001 From: Brooks Cutter Date: Sat, 2 May 2026 02:21:37 -0700 Subject: [PATCH 1/2] Fix Codex arguments --- electron/ipc/agents.ts | 2 +- electron/ipc/pty.ts | 52 +++++++++++++++++++++++++++ package.json | 1 + scripts/fix-node-pty-spawn-helper.mjs | 18 ++++++++++ src/arena/ConfigScreen.tsx | 2 +- src/store/persistence.ts | 3 ++ 6 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 scripts/fix-node-pty-spawn-helper.mjs diff --git a/electron/ipc/agents.ts b/electron/ipc/agents.ts index ea316e16..5ac0e7c8 100644 --- a/electron/ipc/agents.ts +++ b/electron/ipc/agents.ts @@ -31,7 +31,7 @@ const DEFAULT_AGENTS: AgentDef[] = [ command: 'codex', args: [], resume_args: ['resume', '--last'], - skip_permissions_args: ['--full-auto'], + skip_permissions_args: ['--dangerously-bypass-approvals-and-sandbox'], description: "OpenAI's Codex CLI agent", }, { diff --git a/electron/ipc/pty.ts b/electron/ipc/pty.ts index 10194263..4069a700 100644 --- a/electron/ipc/pty.ts +++ b/electron/ipc/pty.ts @@ -64,6 +64,50 @@ const BATCH_INTERVAL = 8; // ms const TAIL_CAP = 8 * 1024; const MAX_LINES = 50; +function redactedSpawnArgs(command: string, args: string[]): string[] { + if ((command === '/bin/sh' || command.endsWith('/sh')) && args[0] === '-c') { + return ['-c', '']; + } + if (command === 'docker') { + return redactDockerArgs(args); + } + return args; +} + +function redactDockerArgs(args: string[]): string[] { + const redacted: string[] = []; + let redactNextEnv = false; + + for (const arg of args) { + if (redactNextEnv) { + redacted.push(redactEnvAssignment(arg)); + redactNextEnv = false; + continue; + } + + if (arg === '-e' || arg === '--env') { + redacted.push(arg); + redactNextEnv = true; + continue; + } + + if (arg.startsWith('--env=')) { + redacted.push(`--env=${redactEnvAssignment(arg.slice('--env='.length))}`); + continue; + } + + redacted.push(arg); + } + + return redacted; +} + +function redactEnvAssignment(value: string): string { + const eqIdx = value.indexOf('='); + if (eqIdx <= 0) return ''; + return `${value.slice(0, eqIdx)}=`; +} + /** Verify that a command exists in PATH. Throws a descriptive error if not found. */ export function validateCommand(command: string): void { if (!command || !command.trim()) { @@ -230,6 +274,14 @@ export function spawnAgent( spawnArgs = args.args; } + logDebug('pty', `spawn command ${args.agentId}`, { + taskId: args.taskId, + command: spawnCommand, + args: redactedSpawnArgs(spawnCommand, spawnArgs), + cwd, + dockerMode: args.dockerMode === true, + }); + const proc = pty.spawn(spawnCommand, spawnArgs, { name: 'xterm-256color', cols: args.cols, diff --git a/package.json b/package.json index 8f385933..4a49d7a7 100644 --- a/package.json +++ b/package.json @@ -24,6 +24,7 @@ "test:coverage": "vitest run --coverage", "check": "npm run typecheck && npm run lint && npm run format:check", "release": "npm run typecheck && npm version patch && git push --follow-tags", + "postinstall": "node scripts/fix-node-pty-spawn-helper.mjs", "prepare": "husky && git config core.hooksPath .husky" }, "license": "MIT", diff --git a/scripts/fix-node-pty-spawn-helper.mjs b/scripts/fix-node-pty-spawn-helper.mjs new file mode 100644 index 00000000..f7c0b08e --- /dev/null +++ b/scripts/fix-node-pty-spawn-helper.mjs @@ -0,0 +1,18 @@ +import { chmodSync, existsSync } from 'fs'; +import { join } from 'path'; +import process from 'process'; + +if (process.platform === 'darwin') { + const helperPath = join( + process.cwd(), + 'node_modules', + 'node-pty', + 'prebuilds', + `darwin-${process.arch}`, + 'spawn-helper', + ); + + if (existsSync(helperPath)) { + chmodSync(helperPath, 0o755); + } +} diff --git a/src/arena/ConfigScreen.tsx b/src/arena/ConfigScreen.tsx index 07b5183a..22097aac 100644 --- a/src/arena/ConfigScreen.tsx +++ b/src/arena/ConfigScreen.tsx @@ -24,7 +24,7 @@ import type { BattleCompetitor } from './types'; /** Built-in tool presets — click to fill the next empty competitor slot */ const TOOL_PRESETS: Array<{ name: string; command: string }> = [ { name: 'Claude', command: 'claude -p "{prompt}" --dangerously-skip-permissions' }, - { name: 'Codex', command: 'codex exec --full-auto "{prompt}"' }, + { name: 'Codex', command: 'codex exec --dangerously-bypass-approvals-and-sandbox "{prompt}"' }, { name: 'Gemini', command: 'gemini -p "{prompt}" --yolo' }, { name: 'Copilot', command: 'copilot -p "{prompt}" --yolo' }, { name: 'Aider', command: 'aider -m "{prompt}" --yes' }, diff --git a/src/store/persistence.ts b/src/store/persistence.ts index 798bfaf2..0d5b744b 100644 --- a/src/store/persistence.ts +++ b/src/store/persistence.ts @@ -28,6 +28,9 @@ function enrichAgentDef(agentDef: AgentDef | null | undefined, availableAgents: if (!agentDef.skip_permissions_args) agentDef.skip_permissions_args = fresh.skip_permissions_args; } + if (agentDef.id === 'codex' && agentDef.skip_permissions_args?.includes('--full-auto')) { + agentDef.skip_permissions_args = ['--dangerously-bypass-approvals-and-sandbox']; + } } export async function saveState(): Promise { From 8a62848af67ab892df4e594b882c046d1c73125c Mon Sep 17 00:00:00 2001 From: Brooks Cutter Date: Sat, 2 May 2026 11:10:50 -0700 Subject: [PATCH 2/2] Add Codex argument regression tests --- electron/ipc/pty.test.ts | 178 +++++++++++++++++++++++----------- src/store/persistence.test.ts | 121 ++++++++++++++++++++++- 2 files changed, 240 insertions(+), 59 deletions(-) diff --git a/electron/ipc/pty.test.ts b/electron/ipc/pty.test.ts index afa4a15b..2a4863b2 100644 --- a/electron/ipc/pty.test.ts +++ b/electron/ipc/pty.test.ts @@ -4,63 +4,66 @@ import path from 'path'; import type { BrowserWindow } from 'electron'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { mockExecFileSync, mockExecFile, mockChildProcessSpawn, mockPtySpawn } = vi.hoisted(() => { - const mockExecFileSync = vi.fn((command: string, args?: string[]) => { - if (command === 'which' && args?.[0] === 'nonexistent-binary-xyz') { - throw new Error('not found'); - } - return ''; - }); - - const mockExecFile = vi.fn(); - const mockChildProcessSpawn = vi.fn(() => ({ - stdout: { on: vi.fn() }, - stderr: { on: vi.fn() }, - on: vi.fn(), - })); - - const mockPtySpawn = vi.fn( - (_command: string, _args: string[], options: { cols: number; rows: number }) => { - let onDataHandler: ((data: string) => void) | undefined; - let onExitHandler: - | ((event: { exitCode: number; signal: number | undefined }) => void) - | undefined; - - const proc = { - cols: options.cols, - rows: options.rows, - write: vi.fn(), - resize: vi.fn((cols: number, rows: number) => { - proc.cols = cols; - proc.rows = rows; - }), - pause: vi.fn(), - resume: vi.fn(), - kill: vi.fn(() => { - onExitHandler?.({ exitCode: 0, signal: 15 }); - }), - onData: vi.fn((handler: (data: string) => void) => { - onDataHandler = handler; - }), - onExit: vi.fn( - (handler: (event: { exitCode: number; signal: number | undefined }) => void) => { - onExitHandler = handler; +const { mockExecFileSync, mockExecFile, mockChildProcessSpawn, mockPtySpawn, mockLogDebug } = + vi.hoisted(() => { + const mockExecFileSync = vi.fn((command: string, args?: string[]) => { + if (command === 'which' && args?.[0] === 'nonexistent-binary-xyz') { + throw new Error('not found'); + } + return ''; + }); + + const mockExecFile = vi.fn(); + const mockChildProcessSpawn = vi.fn(() => ({ + stdout: { on: vi.fn() }, + stderr: { on: vi.fn() }, + on: vi.fn(), + })); + + const mockPtySpawn = vi.fn( + (_command: string, _args: string[], options: { cols: number; rows: number }) => { + let onDataHandler: ((data: string) => void) | undefined; + let onExitHandler: + | ((event: { exitCode: number; signal: number | undefined }) => void) + | undefined; + + const proc = { + cols: options.cols, + rows: options.rows, + write: vi.fn(), + resize: vi.fn((cols: number, rows: number) => { + proc.cols = cols; + proc.rows = rows; + }), + pause: vi.fn(), + resume: vi.fn(), + kill: vi.fn(() => { + onExitHandler?.({ exitCode: 0, signal: 15 }); + }), + onData: vi.fn((handler: (data: string) => void) => { + onDataHandler = handler; + }), + onExit: vi.fn( + (handler: (event: { exitCode: number; signal: number | undefined }) => void) => { + onExitHandler = handler; + }, + ), + emitData(data: string) { + onDataHandler?.(data); }, - ), - emitData(data: string) { - onDataHandler?.(data); - }, - emitExit(event: { exitCode: number; signal: number | undefined }) { - onExitHandler?.(event); - }, - }; + emitExit(event: { exitCode: number; signal: number | undefined }) { + onExitHandler?.(event); + }, + }; - return proc; - }, - ); + return proc; + }, + ); - return { mockExecFileSync, mockExecFile, mockChildProcessSpawn, mockPtySpawn }; -}); + const mockLogDebug = vi.fn(); + + return { mockExecFileSync, mockExecFile, mockChildProcessSpawn, mockPtySpawn, mockLogDebug }; + }); vi.mock('child_process', async () => { const actual = await vi.importActual('child_process'); @@ -76,6 +79,10 @@ vi.mock('node-pty', () => ({ spawn: mockPtySpawn, })); +vi.mock('../log.js', () => ({ + debug: mockLogDebug, +})); + import { buildDockerImage, DOCKER_CONTAINER_HOME, @@ -155,6 +162,14 @@ function getFlagValues(args: string[], flag: string): string[] { return values; } +function getSpawnCommandLogCtx(): { args: string[]; command: string } { + const call = mockLogDebug.mock.calls.find( + ([category, msg]) => category === 'pty' && String(msg).startsWith('spawn command '), + ); + expect(call).toBeTruthy(); + return call?.[2] as { args: string[]; command: string }; +} + function makeTempHome(entries: string[]): string { const home = fs.mkdtempSync(path.join(os.tmpdir(), 'pty-docker-home-')); tempPaths.push(home); @@ -229,6 +244,61 @@ describe('spawnAgent docker mode', () => { expect(envFlags).not.toContain(`HOME=${rendererHome}`); }); + it('redacts docker env values in spawn debug logs', () => { + spawnAgent( + createMockWindow(), + buildSpawnArgs({ + env: { + API_KEY: 'secret-api-key', + NO_VALUE: '', + }, + }), + ); + + const ctx = getSpawnCommandLogCtx(); + const logged = ctx.args.join(' '); + + expect(ctx.command).toBe('docker'); + expect(getFlagValues(ctx.args, '-e')).toContain('API_KEY='); + expect(getFlagValues(ctx.args, '-e')).toContain('NO_VALUE='); + expect(getFlagValues(ctx.args, '-e')).toContain(`HOME=`); + expect(logged).not.toContain('secret-api-key'); + expect(logged).not.toContain(`HOME=${DOCKER_CONTAINER_HOME}`); + expect(logged).toContain('parallel-code-agent:test'); + }); + + it('redacts inline docker env values in spawn debug logs', () => { + spawnAgent( + createMockWindow(), + buildSpawnArgs({ + args: ['--env=INLINE_TOKEN=inline-secret', '--env', 'SPLIT_TOKEN=split-secret'], + }), + ); + + const logged = getSpawnCommandLogCtx().args.join(' '); + + expect(logged).toContain('--env=INLINE_TOKEN='); + expect(logged).toContain('SPLIT_TOKEN='); + expect(logged).not.toContain('inline-secret'); + expect(logged).not.toContain('split-secret'); + }); + + it('redacts shell command strings in spawn debug logs', () => { + spawnAgent( + createMockWindow(), + buildSpawnArgs({ + command: '/bin/sh', + args: ['-c', 'codex exec "prompt containing private context"'], + dockerMode: false, + }), + ); + + const ctx = getSpawnCommandLogCtx(); + + expect(ctx.command).toBe('/bin/sh'); + expect(ctx.args).toEqual(['-c', '']); + }); + it('redirects credential mounts under /tmp inside the container', () => { const home = makeTempHome(['.ssh/', '.gitconfig', '.config/gh/']); vi.stubEnv('HOME', home); diff --git a/src/store/persistence.test.ts b/src/store/persistence.test.ts index 0d4da8e9..3004cd31 100644 --- a/src/store/persistence.test.ts +++ b/src/store/persistence.test.ts @@ -1,5 +1,83 @@ -import { describe, expect, it } from 'vitest'; -import { resolveIncomingPanelUserSize } from './persistence'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { AgentDef } from '../ipc/types'; +import type { PersistedTask } from './types'; + +const { mockInvoke } = vi.hoisted(() => ({ + mockInvoke: vi.fn(), +})); + +vi.mock('../lib/ipc', () => ({ + invoke: mockInvoke, +})); + +import { loadState, resolveIncomingPanelUserSize } from './persistence'; +import { setStore, store } from './core'; + +function agentDef(overrides: Partial = {}): AgentDef { + return { + id: 'codex', + name: 'Codex CLI', + command: 'codex', + args: [], + resume_args: ['resume', '--last'], + skip_permissions_args: [], + description: 'Codex', + ...overrides, + }; +} + +function persistedTask(def: AgentDef): PersistedTask { + return { + id: 'task-1', + name: 'Task', + projectId: 'project-1', + branchName: 'task/task-1', + worktreePath: '/repo/.worktrees/task-1', + notes: '', + lastPrompt: '', + shellCount: 0, + agentDef: def, + gitIsolation: 'worktree', + }; +} + +async function loadPersistedAgent(def: AgentDef): Promise { + mockInvoke.mockResolvedValueOnce( + JSON.stringify({ + projects: [{ id: 'project-1', name: 'Repo', path: '/repo', color: 'hsl(0, 70%, 75%)' }], + lastProjectId: 'project-1', + lastAgentId: null, + taskOrder: ['task-1'], + collapsedTaskOrder: [], + tasks: { + 'task-1': persistedTask(def), + }, + activeTaskId: 'task-1', + sidebarVisible: true, + }), + ); + + await loadState(); + + const agentId = store.tasks['task-1']?.agentIds[0]; + expect(agentId).toBeTruthy(); + return store.agents[agentId as string].def; +} + +beforeEach(() => { + vi.clearAllMocks(); + setStore('projects', []); + setStore('lastProjectId', null); + setStore('lastAgentId', null); + setStore('taskOrder', []); + setStore('collapsedTaskOrder', []); + setStore('tasks', {}); + setStore('agents', {}); + setStore('activeTaskId', null); + setStore('activeAgentId', null); + setStore('availableAgents', []); + setStore('customAgents', []); +}); describe('resolveIncomingPanelUserSize', () => { it('prefers panelUserSize when both new and legacy are present', () => { @@ -27,7 +105,7 @@ describe('resolveIncomingPanelUserSize', () => { 'sidebar:width': 240, }, undefined, - undefined, // no migration flag + undefined, ); expect(result).toEqual({ 'tiling:uuid-1': 520, @@ -54,8 +132,6 @@ describe('resolveIncomingPanelUserSize', () => { }); it('rejects records containing non-finite numbers (NaN / Infinity)', () => { - // NaN and Infinity survive JSON.parse through custom reviver or hand edits - // — tighter validation here prevents them from becoming `flex: 0 0 NaNpx`. const result = resolveIncomingPanelUserSize( { 'tiling:a': Number.NaN, 'tiling:b': 200 }, undefined, @@ -82,3 +158,38 @@ describe('resolveIncomingPanelUserSize', () => { }); }); }); + +describe('loadState agent definition migrations', () => { + it('migrates persisted Codex --full-auto skip-permissions args', async () => { + const restored = await loadPersistedAgent( + agentDef({ + skip_permissions_args: ['--full-auto', '--stale-extra'], + }), + ); + + expect(restored.skip_permissions_args).toEqual(['--dangerously-bypass-approvals-and-sandbox']); + }); + + it('leaves non-Codex --full-auto skip-permissions args unchanged', async () => { + const restored = await loadPersistedAgent( + agentDef({ + id: 'custom-agent', + name: 'Custom Agent', + command: 'custom', + skip_permissions_args: ['--full-auto'], + }), + ); + + expect(restored.skip_permissions_args).toEqual(['--full-auto']); + }); + + it('leaves current Codex skip-permissions args unchanged', async () => { + const restored = await loadPersistedAgent( + agentDef({ + skip_permissions_args: ['--dangerously-bypass-approvals-and-sandbox'], + }), + ); + + expect(restored.skip_permissions_args).toEqual(['--dangerously-bypass-approvals-and-sandbox']); + }); +});