From 6b2d2ef1e834c5331ccdd722344c01b9808337b4 Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 10 Jul 2026 14:37:48 +0000 Subject: [PATCH] Handle ACP session-config rejections inside the ACP lane instead of falling back to CLI Review feedback on the previous fix: the point of ACPX is to stop maintaining the CLI adapter, so recovering from a codex-acp set_config_option rejection by switching lanes was the wrong shape. ACP-native handling instead: - The codex model is now applied via a `-c model=...` startup config override on the codex-acp command (mirroring ANTHROPIC_MODEL for claude). The override feeds the same config surface as ~/.codex/config.toml, which accepts manual model ids; verified against codex-acp 0.12.0 that session/new then reports the manual id as currentModelId while set_config_option validation rejects unlisted ids on older versions. - session/set_config_option application is now best-effort: a rejected option (e.g. ACP -32602) logs loudly, surfaces in the run's command notes, and the run continues on the agent default instead of dying with acpx_session_config_failed. - The ACP->CLI session-config fallback and its error-code constant are reverted; auto lane falls back to CLI only for startup failures, as before. Co-Authored-By: Claude Fable 5 --- .../src/acpx-engine/constants.ts | 5 - .../src/acpx-engine/execute.test.ts | 116 ++++++++++++++++- .../adapter-utils/src/acpx-engine/execute.ts | 117 ++++++++---------- packages/adapters/codex-local/src/index.ts | 2 +- .../codex-local/src/server/acp.test.ts | 7 +- .../src/server/execute.acp-fallback.test.ts | 52 ++------ .../codex-local/src/server/execute.ts | 19 +-- 7 files changed, 186 insertions(+), 132 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/constants.ts b/packages/adapter-utils/src/acpx-engine/constants.ts index dc5e65aea3c..86368f5f97b 100644 --- a/packages/adapter-utils/src/acpx-engine/constants.ts +++ b/packages/adapter-utils/src/acpx-engine/constants.ts @@ -5,11 +5,6 @@ export const DEFAULT_ACP_ENGINE_NON_INTERACTIVE_PERMISSIONS = "deny"; export const DEFAULT_ACP_ENGINE_TIMEOUT_SEC = 0; export const DEFAULT_ACP_ENGINE_WARM_HANDLE_IDLE_MS = 0; -// The ACP server rejected a session/set_config_option value (for example a -// manual model id its validation list does not include). Adapters with a -// non-ACP lane can treat this as recoverable and retry there. -export const ACPX_SESSION_CONFIG_FAILED_ERROR_CODE = "acpx_session_config_failed"; - export const ACPX_ADAPTER_AGENT_IDS = { claude_local: "claude", codex_local: "codex", diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index a953021dca5..1176421ab8c 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -75,15 +75,16 @@ async function runExecutor( executionTransport?: Record; authToken?: string; executionTarget?: Record; + runtimeOverrides?: Record; } = {}, ) { const runtimeOptions: Record[] = []; const meta: Record[] = []; const logs: Array<{ stream: string; text: string }> = []; const execute = createAcpxEngineExecutor({ - createRuntime: (options) => { - runtimeOptions.push(options as unknown as Record); - return buildRuntime() as never; + createRuntime: (createOptions) => { + runtimeOptions.push(createOptions as unknown as Record); + return { ...buildRuntime(), ...options.runtimeOverrides } as never; }, }); @@ -1166,3 +1167,112 @@ describe("shared ACP engine execution timeouts", () => { expect(cancelReasons).toContain(expectedMessage); }, 15_000); }); + +describe("ACPX session config application", () => { + it("applies the codex model via a -c startup config override instead of set_config_option", async () => { + const root = await makeTempRoot(); + const stateDir = path.join(root, "state"); + const setConfigOption = [] as Array<{ key: string; value: string }>; + + const { meta } = await runExecutor( + { + agent: "codex", + agentCommand: "node ./fake-codex-acp.js", + model: "gpt-5.5-pro", + stateDir, + env: { CODEX_HOME: path.join(root, "codex-home") }, + }, + { + runtimeOverrides: { + setConfigOption: async (input: { key: string; value: string }) => { + setConfigOption.push({ key: input.key, value: input.value }); + }, + }, + }, + ); + + const wrappers = await fs.readdir(path.join(stateDir, "wrappers")); + const wrapperFile = wrappers.find((name) => name.endsWith(".sh")); + expect(wrapperFile).toBeTruthy(); + const wrapper = await fs.readFile(path.join(stateDir, "wrappers", wrapperFile!), "utf8"); + expect(wrapper).toContain('exec node ./fake-codex-acp.js -c \'model="gpt-5.5-pro"\''); + expect(setConfigOption).toEqual([]); + const commandNotes = (meta[0]?.commandNotes ?? []) as string[]; + expect(commandNotes.join("\n")).toContain("set via -c model=... startup config override"); + }); + + it("still applies codex reasoning effort via set_config_option alongside the model startup override", async () => { + const root = await makeTempRoot(); + const setConfigOption = [] as Array<{ key: string; value: string }>; + + await runExecutor( + { + agent: "codex", + agentCommand: "node ./fake-codex-acp.js", + model: "gpt-5.5-pro", + modelReasoningEffort: "high", + stateDir: path.join(root, "state"), + env: { CODEX_HOME: path.join(root, "codex-home") }, + }, + { + runtimeOverrides: { + setConfigOption: async (input: { key: string; value: string }) => { + setConfigOption.push({ key: input.key, value: input.value }); + }, + }, + }, + ); + + expect(setConfigOption).toEqual([{ key: "reasoning_effort", value: "high" }]); + }); + + it("degrades gracefully when the ACP server rejects a session config option", async () => { + const root = await makeTempRoot(); + + const { logs, meta, result } = await runExecutor( + { + agent: "custom", + agentCommand: "node ./fake-acp.js", + model: "gpt-5.5-pro", + stateDir: path.join(root, "state"), + }, + { + runtimeOverrides: { + setConfigOption: async () => { + throw new Error("Invalid params (ACP -32602)"); + }, + }, + }, + ); + + expect(result.exitCode).toBe(0); + expect(result.errorCode ?? null).toBeNull(); + const stderrText = logs + .filter((entry) => entry.stream === "stderr") + .map((entry) => entry.text) + .join("\n"); + expect(stderrText).toContain('rejected session config model=gpt-5.5-pro'); + expect(stderrText).toContain("continuing with the agent's default model"); + const commandNotes = (meta[0]?.commandNotes ?? []) as string[]; + expect(commandNotes.join("\n")).toContain("continuing with the agent's default model"); + }); + + it("warns and continues when the ACPX runtime lacks session config controls", async () => { + const root = await makeTempRoot(); + + const { logs, result } = await runExecutor({ + agent: "custom", + agentCommand: "node ./fake-acp.js", + thinkingEffort: "high", + stateDir: path.join(root, "state"), + }); + + expect(result.exitCode).toBe(0); + const stderrText = logs + .filter((entry) => entry.stream === "stderr") + .map((entry) => entry.text) + .join("\n"); + expect(stderrText).toContain("does not expose session config controls"); + expect(stderrText).toContain("continuing without effort override(s)"); + }); +}); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 1e97358f19a..41c5ea02e65 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -54,7 +54,6 @@ import { type AcpRuntimeTurnResult, } from "acpx/runtime"; import { - ACPX_SESSION_CONFIG_FAILED_ERROR_CODE, DEFAULT_ACP_ENGINE_AGENT, DEFAULT_ACP_ENGINE_MODE, DEFAULT_ACP_ENGINE_NON_INTERACTIVE_PERMISSIONS, @@ -1076,6 +1075,16 @@ async function buildRuntime(input: { agentCommand = normalized; } } + // For the codex agent, set the model via a `-c model=...` startup config + // override rather than session/set_config_option — the override feeds the + // same config surface as ~/.codex/config.toml, which accepts manual model + // ids, while some codex-acp versions validate set_config_option values + // against their advertised model list and reject unlisted ids (ACP -32602). + // This mirrors the ANTHROPIC_MODEL startup env approach for claude below. + if (requestedModel && acpxAgent === "codex" && agentCommandShell) { + agentCommandShell = `${agentCommandShell} -c ${shellQuote(`model=${JSON.stringify(requestedModel)}`)}`; + agentCommand = agentCommandShell; + } const childStderrDir = path.join(stateDir, "run-stderr"); const childStderrLogPath = agentCommand ? path.join(childStderrDir, `${runId}.log`) : null; const wrapper = agentCommand @@ -1156,9 +1165,10 @@ async function buildRuntime(input: { function sessionConfigOptions(prepared: AcpxPreparedRuntime): Array<{ key: string; value: string }> { const options: Array<{ key: string; value: string }> = []; // Model for the claude agent is pre-set via ANTHROPIC_MODEL env var at - // startup; skip set_config_option to avoid ACP-server model-name validation - // that rejects bare IDs like "claude-opus-4-7" in some runtime versions. - if (prepared.requestedModel && prepared.acpxAgent !== "claude") { + // startup, and for the codex agent via a `-c model=...` startup config + // override; skip set_config_option for both to avoid ACP-server model-name + // validation that rejects bare/manual IDs in some runtime versions. + if (prepared.requestedModel && prepared.acpxAgent !== "claude" && prepared.acpxAgent !== "codex") { options.push({ key: "model", value: prepared.requestedModel }); } if (prepared.requestedThinkingEffort) { @@ -1176,31 +1186,47 @@ function sessionConfigOptions(prepared: AcpxPreparedRuntime): Array<{ key: strin return options; } +// Session config options are preferences, not prerequisites: a value the ACP +// server rejects (e.g. -32602 for a model id its validation list does not +// advertise) must degrade the run, never kill it. Failures are logged loudly +// and returned as notes so they surface in the run's command metadata. async function applySessionConfigOptions(input: { runtime: AcpRuntime; handle: AcpRuntimeHandle; prepared: AcpxPreparedRuntime; onLog: AdapterExecutionContext["onLog"]; -}) { +}): Promise { const options = sessionConfigOptions(input.prepared); - if (options.length === 0) return; + if (options.length === 0) return []; + const notes: string[] = []; if (!input.runtime.setConfigOption) { const message = - "ACPX runtime does not expose session config controls; upgrade ACPX or remove configured model, effort, and fast mode overrides."; + `ACPX runtime does not expose session config controls; continuing without ` + + `${options.map((option) => option.key).join(", ")} override(s). Upgrade ACPX to apply them.`; await input.onLog("stderr", `[paperclip] ${message}\n`); - throw new Error(message); + return [message]; } for (const option of options) { - await input.runtime.setConfigOption({ - handle: input.handle, - key: option.key, - value: option.value, - }); - await input.onLog( - "stdout", - `[paperclip] Applied ACPX ${input.prepared.acpxAgent} config ${option.key}=${option.value}\n`, - ); + try { + await input.runtime.setConfigOption({ + handle: input.handle, + key: option.key, + value: option.value, + }); + await input.onLog( + "stdout", + `[paperclip] Applied ACPX ${input.prepared.acpxAgent} config ${option.key}=${option.value}\n`, + ); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + const message = + `ACPX ${input.prepared.acpxAgent} rejected session config ${option.key}=${option.value} ` + + `(${reason}); continuing with the agent's default ${option.key}.`; + await input.onLog("stderr", `[paperclip] ${message}\n`); + notes.push(message); + } } + return notes; } function renderPaperclipEnvNote(env: Record): string { @@ -1378,7 +1404,7 @@ function resultErrorMessage(result: AcpRuntimeTurnResult): string | null { return result.error.message; } -type AcpxExecutionPhase = "ensure_session" | "configure_session" | "turn"; +type AcpxExecutionPhase = "ensure_session" | "turn"; function describeErrorDiagnostics(err: unknown): { errorName: string; @@ -1443,7 +1469,6 @@ function classifyError( if (acpCode === "ACP_BACKEND_MISSING") return "acpx_backend_missing"; if (acpCode === "ACP_BACKEND_UNAVAILABLE") return "acpx_backend_unavailable"; if (phase === "ensure_session") return "acpx_session_init_failed"; - if (phase === "configure_session") return ACPX_SESSION_CONFIG_FAILED_ERROR_CODE; if (phase === "turn") return "acpx_turn_failed"; return null; })(); @@ -1721,49 +1746,12 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { }; } const sessionHandle = handle; - try { - await applySessionConfigOptions({ - runtime, - handle: sessionHandle, - prepared, - onLog: ctx.onLog, - }); - } catch (err) { - const { classified, message } = await emitAcpxFailure({ - ctx, - prepared, - err, - phase: "configure_session", - }); - await runtime.close({ - handle: sessionHandle, - reason: "paperclip config cleanup", - discardPersistentState: false, - }).catch(() => {}); - const existing = warmHandles.get(prepared.sessionKey); - if (warmHandleMatches(existing, runtime, sessionHandle) && existing) { - clearWarmHandleTimer(existing); - warmHandles.delete(prepared.sessionKey); - } - return { - exitCode: 1, - signal: null, - timedOut: false, - errorMessage: message, - ...classified, - provider: "acpx", - model: prepared.requestedModel || null, - clearSession, - resultJson: { - phase: "configure_session", - agent: prepared.acpxAgent, - requestedModel: prepared.requestedModel || null, - requestedThinkingEffort: prepared.requestedThinkingEffort || null, - fastMode: prepared.fastMode, - }, - summary: message, - }; - } + const sessionConfigNotes = await applySessionConfigOptions({ + runtime, + handle: sessionHandle, + prepared, + onLog: ctx.onLog, + }); const { prompt, promptMetrics, commandNotes } = await buildPrompt(ctx, resumedSession, prepared.env); const runPrompt = joinPromptSections([prepared.skillPromptInstructions, prompt]); await emitAcpxLog(ctx, { @@ -1791,11 +1779,14 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { ? [ prepared.acpxAgent === "claude" ? `Requested ACPX model: ${prepared.requestedModel} (set via ANTHROPIC_MODEL env at startup).` - : `Requested ACPX model: ${prepared.requestedModel}.`, + : prepared.acpxAgent === "codex" + ? `Requested ACPX model: ${prepared.requestedModel} (set via -c model=... startup config override).` + : `Requested ACPX model: ${prepared.requestedModel}.`, ] : []), ...(prepared.requestedThinkingEffort ? [`Requested ACPX thinking effort: ${prepared.requestedThinkingEffort}.`] : []), ...(prepared.fastMode ? ["Requested ACPX Codex fast mode."] : []), + ...sessionConfigNotes, ...(Array.isArray(prepared.skillsIdentity.commandNotes) ? prepared.skillsIdentity.commandNotes.filter((note): note is string => typeof note === "string") : []), diff --git a/packages/adapters/codex-local/src/index.ts b/packages/adapters/codex-local/src/index.ts index 96a0cff1126..cd836564974 100644 --- a/packages/adapters/codex-local/src/index.ts +++ b/packages/adapters/codex-local/src/index.ts @@ -104,5 +104,5 @@ Notes: - Some model/tool combinations reject certain effort levels (for example minimal with web search enabled). - Fast mode is supported on GPT-5.5, GPT-5.4 and manual model IDs. When enabled for those models, Paperclip applies \`service_tier="fast"\` and \`features.fast_mode=true\`. - When Paperclip realizes a workspace/runtime for a run, it injects PAPERCLIP_WORKSPACE_* and PAPERCLIP_RUNTIME_* env vars for agent-side tooling. -- Codex ACP is the preferred auto lane when Node >=22.13.0 and the Codex ACP server are available. It reuses shared ACP prompt/runtime guidance, selected skill materialization into CODEX_HOME/skills, model/reasoning/fast-mode session config, and existing quota-window reporting. Auto selection falls back to CLI when ACP prerequisites are unavailable or the ACP server rejects the session configuration (for example a manual model id its set_config_option validation does not list); explicit engine="acp" fails loudly. +- Codex ACP is the preferred auto lane when Node >=22.13.0 and the Codex ACP server are available. It reuses shared ACP prompt/runtime guidance, selected skill materialization into CODEX_HOME/skills, model/reasoning/fast-mode session config, and existing quota-window reporting. The configured model is applied through a \`-c model=...\` startup config override, which accepts manual model ids; other session config options are best-effort, so a rejected option is logged and the run continues on the agent default instead of failing. Auto selection falls back to CLI only when ACP prerequisites are unavailable; explicit engine="acp" fails loudly. `; diff --git a/packages/adapters/codex-local/src/server/acp.test.ts b/packages/adapters/codex-local/src/server/acp.test.ts index cfb7df55508..d6ca1d0b852 100644 --- a/packages/adapters/codex-local/src/server/acp.test.ts +++ b/packages/adapters/codex-local/src/server/acp.test.ts @@ -368,12 +368,17 @@ describe("codex_local ACP lane", () => { mode: "persistent", cwd: root, }); + // The model is applied via a `-c model=...` startup config override (it + // accepts manual model ids some codex-acp versions reject through + // set_config_option); only effort and fast mode go through session config. expect(runtimes[0]?.setConfigInputs.map((input) => [input.key, input.value])).toEqual([ - ["model", "gpt-5.5"], ["reasoning_effort", "high"], ["service_tier", "fast"], ["features.fast_mode", "true"], ]); + expect(meta[0]?.commandNotes?.join("\n")).toContain( + "Requested ACPX model: gpt-5.5 (set via -c model=... startup config override).", + ); expect(meta[0]?.commandNotes?.join("\n")).toContain("Prepared ACPX Codex skill home"); expect(meta[0]?.env?.CODEX_HOME).toBe(path.join(root, "codex-home")); }); diff --git a/packages/adapters/codex-local/src/server/execute.acp-fallback.test.ts b/packages/adapters/codex-local/src/server/execute.acp-fallback.test.ts index 0c4284ac084..05524dafdff 100644 --- a/packages/adapters/codex-local/src/server/execute.acp-fallback.test.ts +++ b/packages/adapters/codex-local/src/server/execute.acp-fallback.test.ts @@ -159,58 +159,28 @@ describe("codex_local ACP startup fallback", () => { expect(runAdapterExecutionTargetProcess).not.toHaveBeenCalled(); }); - it("falls back to Codex CLI when auto-selected ACP rejects session configuration", async () => { - executeCodexAcp.mockResolvedValueOnce({ - exitCode: 1, + it("returns successful structured ACP results without touching the CLI lane", async () => { + const succeeded = { + exitCode: 0, signal: null, timedOut: false, - errorMessage: - 'Agent rejected session/set_config_option for "model"="gpt-5.5-pro": Invalid params (ACP -32602).', - errorCode: "acpx_session_config_failed", + errorMessage: null, + errorCode: null, provider: "acpx", model: "gpt-5.5-pro", - resultJson: { phase: "configure_session" }, - summary: "config rejected", - } as never); + resultJson: { status: "completed" }, + summary: "acp turn done", + }; + executeCodexAcp.mockResolvedValueOnce(succeeded as never); const ctx = buildContext({ model: "gpt-5.5-pro" }); const result = await execute(ctx as never); - expect(result.exitCode).toBe(0); - expect(result.summary).toBe("hello"); - expect(executeCodexAcp).toHaveBeenCalledTimes(1); - expect(runAdapterExecutionTargetProcess).toHaveBeenCalledTimes(1); - expect(ctx.onLog).toHaveBeenCalledWith( - "stderr", - expect.stringContaining("Codex ACP rejected session configuration"), - ); - expect(ctx.onLog).toHaveBeenCalledWith( - "stderr", - expect.stringContaining("gpt-5.5-pro"), - ); - }); - - it("returns the session-config failure unchanged when engine=acp is explicit", async () => { - const failed = { - exitCode: 1, - signal: null, - timedOut: false, - errorMessage: "Agent rejected session/set_config_option", - errorCode: "acpx_session_config_failed", - provider: "acpx", - resultJson: { phase: "configure_session" }, - summary: "config rejected", - }; - executeCodexAcp.mockResolvedValueOnce(failed as never); - const ctx = buildContext({ engine: "acp" }); - - const result = await execute(ctx as never); - - expect(result).toBe(failed); + expect(result).toBe(succeeded); expect(runAdapterExecutionTargetProcess).not.toHaveBeenCalled(); }); - it("does not fall back for other failed ACP results", async () => { + it("does not fall back to the CLI lane for failed structured ACP results", async () => { const failed = { exitCode: 1, signal: null, diff --git a/packages/adapters/codex-local/src/server/execute.ts b/packages/adapters/codex-local/src/server/execute.ts index 7cabaabd1fd..976b63cf988 100644 --- a/packages/adapters/codex-local/src/server/execute.ts +++ b/packages/adapters/codex-local/src/server/execute.ts @@ -64,7 +64,6 @@ import { formatOutputInactivityMonitorErrorMessage, resolveCodexInactivityTimeout, } from "./output-inactivity-monitor.js"; -import { ACPX_SESSION_CONFIG_FAILED_ERROR_CODE } from "@paperclipai/adapter-utils/acpx-engine/constants"; import { createCodexAcpExecutor, formatCodexAcpFallbackMessage, @@ -331,9 +330,8 @@ export async function ensureCodexSkillsInjected( export async function execute(ctx: AdapterExecutionContext): Promise { const engineSelection = await resolveCodexExecutionEngineForRun(ctx); if (engineSelection.engine === "acp") { - let acpResult: AdapterExecutionResult | null = null; try { - acpResult = await executeCodexAcp(ctx); + return await executeCodexAcp(ctx); } catch (err) { if (engineSelection.explicit) throw err; const reason = err instanceof Error ? err.message : String(err); @@ -342,21 +340,6 @@ export async function execute(ctx: AdapterExecutionContext): Promise