From ef90b905a3444ddc7b629a0431ef7b63b60f1b46 Mon Sep 17 00:00:00 2001 From: Jason Kneen Date: Tue, 16 Jun 2026 02:06:53 +0100 Subject: [PATCH] fix(chat): place codex exec OPTIONS before `resume` subcommand on multi-turn resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's CLI grammar is `codex exec [OPTIONS] resume [PROMPT]` (confirmed via `codex exec --help`, codex-cli 0.139.0): `resume` is a subcommand of `exec`, so every exec-level OPTION (--json, --model, sandbox/approval flags, --ignore-user-config, --skip-git-repo-check, -C ) must precede it; only SESSION_ID and PROMPT follow. Both codex argv builders emitted `resume ` immediately after `exec`, BEFORE the option flags. On a resume turn codex parsed `resume` first and then rejected the trailing exec options against the resume subparser with `error: unexpected argument '-C' found`, breaking multi-turn resume for codex chats. - src/main/chat/providers/agent-mode-payloads.ts (buildCodexSpawnArgs): the runtime path chatCodex spawns. Push all option flags first, THEN `resume `, THEN the prompt. - packages/codesurf-daemon/bin/chat-jobs.mjs (buildCodexExecArgs): the daemon path. Same reorder. (This builder introduced the defect in 64980c8 "Fix daemon multi-turn chat continuity".) Non-resume (first-turn) argv is functionally unchanged: just `exec `. Audit of every harness resume invocation in the daemon — no other site repeats the subcommand-before-options defect: - src/main/relay/provider-executor.ts runCodexTurn — no resume path (clean) - src/main/session-sources.ts:1384, packages/.../session-index.mjs:923 — `resumeBin`/`resumeArgs` launch metadata for the interactive `codex resume ` TUI (not `codex exec`); top-level command, order- independent (clean) - hermes (--resume ), claude (--resume / SDK options.resume), opencode (--session) — all flag-form, order-independent (clean) Claude resume (claude.ts:725 options.resume) was investigated and verified CORRECT — not the same bug class. `resume` is a named SDK option, immune to positional ordering. A live 2-turn SDK spike (haiku, claude-opus-4-7, and with the named-agent options chatClaude sets) resumes correctly every time; the renderer round-trip is intact (useChatStreamHandler captures the `session` event → useChatTileMessaging sends `sessionId` on the next turn). The desktop "claude" provider dispatches to chatClaude (chat.ts:1025), the verified path. No provider-level Claude fix is warranted. Tests: - packages/codesurf-daemon/test/codex-exec-args.test.mjs (new): assert buildCodexExecArgs puts all options before `resume`, `-C` never follows `resume`, and fresh turns omit `resume`. - test/daemon/chat-jobs-agent-mode.test.mjs: corrected the prior test that asserted the buggy `resume`-immediately-after-`exec` order; added direct coverage for buildCodexSpawnArgs (BUG 1's actual function). --- packages/codesurf-daemon/bin/chat-jobs.mjs | 12 ++- .../test/codex-exec-args.test.mjs | 73 +++++++++++++++++++ .../chat/providers/agent-mode-payloads.ts | 9 ++- test/daemon/chat-jobs-agent-mode.test.mjs | 63 +++++++++++++--- 4 files changed, 143 insertions(+), 14 deletions(-) create mode 100644 packages/codesurf-daemon/test/codex-exec-args.test.mjs diff --git a/packages/codesurf-daemon/bin/chat-jobs.mjs b/packages/codesurf-daemon/bin/chat-jobs.mjs index f77dcfda..6ef90f2d 100644 --- a/packages/codesurf-daemon/bin/chat-jobs.mjs +++ b/packages/codesurf-daemon/bin/chat-jobs.mjs @@ -700,20 +700,24 @@ export function buildCodexExecArgs(request, workspaceDir, instructionPrompt = '' // Multi-turn continuity: when the request carries the Codex thread id from a // prior turn (emitted as a `thread.started` session event and echoed back by // the client as request.sessionId), resume that thread so the model keeps the - // full conversation — `codex exec resume ...`. This mirrors the - // runtime builder (src/main/chat/providers/agent-mode-payloads.ts - // buildCodexSpawnArgs), which places `resume ` immediately after `exec`. + // full conversation — `codex exec [OPTIONS] resume [PROMPT]`. + // Codex's CLI grammar requires every exec-level OPTION (--json, --model, + // --skip-git-repo-check, -C , sandbox/approval flags) to precede the + // `resume` subcommand; only SESSION_ID and PROMPT follow it. Placing `resume` + // before the options makes codex reject the trailing flags (e.g. + // `error: unexpected argument '-C' found`). Mirrors the runtime builder + // (src/main/chat/providers/agent-mode-payloads.ts buildCodexSpawnArgs). // First turn (no sessionId) starts a fresh thread (unchanged behavior). const resumeArgs = request.sessionId ? ['resume', request.sessionId] : [] const codexArgs = [ 'exec', - ...resumeArgs, '--json', '--model', request.model, '--skip-git-repo-check', ...(workspaceDir ? ['-C', workspaceDir] : []), ...sandboxApprovalFlags, + ...resumeArgs, ] codexArgs.push(buildCodexPrompt( lastUserMsg?.content ?? '', diff --git a/packages/codesurf-daemon/test/codex-exec-args.test.mjs b/packages/codesurf-daemon/test/codex-exec-args.test.mjs new file mode 100644 index 00000000..2d938a6c --- /dev/null +++ b/packages/codesurf-daemon/test/codex-exec-args.test.mjs @@ -0,0 +1,73 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { buildCodexExecArgs } from '../bin/chat-jobs.mjs' + +// Codex's CLI grammar is `codex exec [OPTIONS] resume [PROMPT]`. +// Every exec-level OPTION (--json, --model, sandbox/approval flags, +// --skip-git-repo-check, -C ) must precede the `resume` subcommand; only +// SESSION_ID and PROMPT follow it. The original builder pushed `resume ` +// immediately after `exec`, so codex rejected the trailing `-C` with +// `error: unexpected argument '-C' found`, breaking multi-turn resume. + +test('buildCodexExecArgs places exec-level options before resume', () => { + const args = buildCodexExecArgs({ + provider: 'codex', + model: 'o3', + mode: 'default', + sessionId: 'session-1', + messages: [{ role: 'user', content: 'continue' }], + }, '/tmp/workspace') + + assert.deepEqual(args.slice(0, 12), [ + 'exec', + '--json', + '--model', + 'o3', + '--skip-git-repo-check', + '-C', + '/tmp/workspace', + '-s', + 'workspace-write', + '-c', + 'approval_policy=on-request', + 'resume', + ]) + assert.equal(args[12], 'session-1') + assert.match(args.at(-1), /continue/) +}) + +test('buildCodexExecArgs never emits -C (or any flag) after resume', () => { + const args = buildCodexExecArgs({ + provider: 'codex', + model: 'o3', + mode: 'default', + sessionId: 'session-1', + messages: [{ role: 'user', content: 'continue' }], + }, '/tmp/workspace') + + const resumeIdx = args.indexOf('resume') + assert.ok(resumeIdx > -1, 'resume subcommand must be present on a continuation turn') + // `-C` and every other exec-level flag must live BEFORE `resume`. + assert.ok(args.lastIndexOf('-C') < resumeIdx, '`-C` must never follow `resume`') + for (const flag of ['--json', '--model', '--skip-git-repo-check', '-s', '-c']) { + assert.ok(args.indexOf(flag) > -1 && args.indexOf(flag) < resumeIdx, `${flag} must precede resume`) + } + // Only the session id and prompt follow `resume`. + assert.equal(resumeIdx, args.length - 3, '`resume` must be third-from-last: resume, , ') + assert.equal(args[resumeIdx + 1], 'session-1', 'the session id immediately follows `resume`') +}) + +test('buildCodexExecArgs keeps fresh turns on codex exec without resume', () => { + const args = buildCodexExecArgs({ + provider: 'codex', + model: 'o3', + mode: 'default', + messages: [{ role: 'user', content: 'start' }], + }, '/tmp/workspace') + + assert.equal(args[0], 'exec') + assert.equal(args[1], '--json', 'fresh turns keep the flag order immediately after exec') + assert.equal(args.includes('resume'), false) + assert.equal(args.at(-1).includes('start'), true) +}) diff --git a/src/main/chat/providers/agent-mode-payloads.ts b/src/main/chat/providers/agent-mode-payloads.ts index 52ee8f77..45f8069b 100644 --- a/src/main/chat/providers/agent-mode-payloads.ts +++ b/src/main/chat/providers/agent-mode-payloads.ts @@ -130,13 +130,20 @@ export function buildCodexSpawnArgs(input: CodexSpawnInput): string[] { agentPersona, ) + // Codex CLI grammar is `codex exec [OPTIONS] resume [PROMPT]`: + // every exec-level OPTION (--json, --model, sandbox/approval flags, + // --ignore-user-config, --skip-git-repo-check, -C ) MUST precede the + // `resume` subcommand — only SESSION_ID and PROMPT follow it. Emitting + // `resume` before the options makes codex reject the trailing flags with + // `error: unexpected argument '-C' found`, breaking multi-turn resume. So push + // all options first, THEN (when resuming) `resume `, THEN the prompt. const args: string[] = ['exec'] - if (input.resumeThreadId) args.push('resume', input.resumeThreadId) args.push('--json', '--model', input.model) args.push(...sandboxApprovalFlags) args.push('--ignore-user-config') if (input.workspaceDir) args.push('--skip-git-repo-check', '-C', input.workspaceDir) else args.push('--skip-git-repo-check') + if (input.resumeThreadId) args.push('resume', input.resumeThreadId) args.push(promptText) return args } diff --git a/test/daemon/chat-jobs-agent-mode.test.mjs b/test/daemon/chat-jobs-agent-mode.test.mjs index 503f1d1e..d67dea60 100644 --- a/test/daemon/chat-jobs-agent-mode.test.mjs +++ b/test/daemon/chat-jobs-agent-mode.test.mjs @@ -500,24 +500,37 @@ test('daemon Codex: AgentMode.systemPrompt is injected into the prompt arg', () // ─── daemon Codex: multi-turn continuity (resume) ──────────────────────────── -test('daemon Codex: a request with sessionId resumes the thread (`exec resume `), first turn does NOT', () => { +test('daemon Codex: a request with sessionId resumes the thread (`exec [OPTIONS] resume `), first turn does NOT', () => { const base = { provider: 'codex', model: 'gpt-test', mode: 'default', messages: [{ role: 'user', content: 'do it' }] } // Continuation turn: the Codex thread id from a prior turn is echoed back as - // request.sessionId. The argv must resume that thread so the model keeps the - // full conversation — `codex exec resume ...`, with `resume ` - // immediately after `exec` (mirrors the runtime buildCodexSpawnArgs shape). + // request.sessionId. Codex's CLI grammar is `codex exec [OPTIONS] resume + // [PROMPT]` — ALL exec-level options (--json, --model, + // --skip-git-repo-check, -C , sandbox/approval flags) MUST precede the + // `resume` subcommand; only the SESSION_ID and PROMPT follow it. Emitting + // `resume` before the options makes codex reject the trailing flags + // (`error: unexpected argument '-C' found`), which broke multi-turn resume. const resumed = buildCodexExecArgs({ ...base, sessionId: 'thread-1' }, '/ws') const execIdx = resumed.indexOf('exec') - assert.equal(resumed[execIdx + 1], 'resume', '`resume` must immediately follow `exec`') - assert.equal(resumed[execIdx + 2], 'thread-1', 'the thread id must follow `resume`') - // (a plain includes('resume thread-1') would be false — they are two argv elements.) - assert.ok(resumed.includes('resume') && resumed.includes('thread-1')) + const resumeIdx = resumed.indexOf('resume') + // `resume` must NOT sit immediately after `exec` — it comes AFTER the options. + assert.notEqual(resumed[execIdx + 1], 'resume', '`resume` must NOT immediately follow `exec` (options come first)') + assert.equal(resumed[execIdx + 1], '--json', 'exec-level options lead the argv') + assert.equal(resumed[resumeIdx + 1], 'thread-1', 'the thread id must immediately follow `resume`') + // The thread id and the prompt are the only args after `resume`. + assert.equal(resumeIdx, resumed.length - 3, '`resume` must be third-from-last (resume, , )') + assert.match(resumed.at(-1), /do it/, 'the prompt is the final positional arg') + // Every exec-level option/flag must appear BEFORE `resume`. + for (const flag of ['--json', '--model', 'gpt-test', '--skip-git-repo-check', '-C', '/ws', '-s', 'workspace-write', '-c', 'approval_policy=on-request']) { + assert.ok(resumed.indexOf(flag) > -1 && resumed.indexOf(flag) < resumeIdx, `${flag} must precede resume`) + } + // `-C ` must never appear after `resume` (the exact bug codex rejected). + assert.ok(resumed.lastIndexOf('-C') < resumeIdx, '`-C` must never follow `resume`') // First turn (no sessionId): a fresh thread, NO resume subcommand. const fresh = buildCodexExecArgs({ ...base }, '/ws') assert.ok(!fresh.includes('resume'), 'first turn (no sessionId) must NOT resume') - assert.equal(fresh[fresh.indexOf('exec') + 1], '--json', 'first turn keeps the unchanged flag order after exec') + assert.equal(fresh[fresh.indexOf('exec') + 1], '--json', 'first turn keeps the flag order after exec') // Empty-string sessionId is falsy → treated as a first turn (no resume). assert.ok(!buildCodexExecArgs({ ...base, sessionId: '' }, '/ws').includes('resume')) @@ -527,6 +540,38 @@ test('daemon Codex: a request with sessionId resumes the thread (`exec resume `, first turn does NOT resume', () => { + const base = { agentMode: null, mode: 'default', model: 'gpt-test', userContent: 'do it', workspaceDir: '/ws' } + + // Same Codex CLI grammar invariant as the daemon builder: `codex exec + // [OPTIONS] resume [PROMPT]`. buildCodexSpawnArgs is the function + // chatCodex actually spawns (the runtime path); BUG 1 was that it pushed + // `resume ` before the option flags, so codex rejected `-C` on resume. + const resumed = buildCodexSpawnArgs({ ...base, resumeThreadId: 'thread-9' }) + const execIdx = resumed.indexOf('exec') + const resumeIdx = resumed.indexOf('resume') + assert.equal(resumed[execIdx + 1], '--json', 'exec-level options must lead the argv (not `resume`)') + assert.notEqual(resumed[execIdx + 1], 'resume', '`resume` must NOT immediately follow `exec`') + assert.equal(resumed[resumeIdx + 1], 'thread-9', 'the thread id must immediately follow `resume`') + assert.equal(resumeIdx, resumed.length - 3, '`resume` must be third-from-last (resume, , )') + // Prompt is the final positional; `resume ` are the two args before it. + assert.equal(resumed[resumed.length - 2], 'thread-9', ' precedes the prompt') + assert.match(resumed.at(-1), /do it/, 'the prompt is the final positional arg') + // Every exec-level option/flag must precede `resume`, and `-C` must not follow it. + for (const flag of ['--json', '--model', 'gpt-test', '--ignore-user-config', '--skip-git-repo-check', '-C', '/ws', '-s', 'workspace-write', '-c', 'approval_policy=on-request']) { + assert.ok(resumed.indexOf(flag) > -1 && resumed.indexOf(flag) < resumeIdx, `${flag} must precede resume`) + } + assert.ok(resumed.lastIndexOf('-C') < resumeIdx, '`-C` must never follow `resume`') + + // First turn (no resumeThreadId): no resume subcommand, prompt stays last. + const fresh = buildCodexSpawnArgs({ ...base }) + assert.ok(!fresh.includes('resume'), 'first turn must NOT resume') + assert.equal(fresh[fresh.indexOf('exec') + 1], '--json', 'first turn keeps the flag order after exec') + assert.match(fresh.at(-1), /do it/, 'prompt is last on a fresh turn too') +}) + // ─── daemon Codex SDK provider: opt-in mapping + resume ────────────────────── test('daemon Codex SDK: each UI mode maps to SDK sandboxMode + approvalPolicy', () => {