From bf8f5d991ce024e71de5ebd6e86fbcb86a8db0ad Mon Sep 17 00:00:00 2001 From: sxxtony <166789813+sxxtony@users.noreply.github.com> Date: Tue, 12 May 2026 21:31:43 +0000 Subject: [PATCH] fix(acp): drop unsupported timeout config option for claude-agent-acp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runtime-options.buildRuntimeConfigOptionPairs` translated `AcpSessionRuntimeOptions.timeoutSeconds` into a `session/set_config_option(configId: "timeout")` pair on every turn. Both the control plane (`AcpSessionManager.applyManagerRuntimeControls`) and the ACPX wrapper (`AcpxRuntime.setConfigOption`) sit between that pair and the backend: - The control plane validates pairs against the backend's advertised config-option keys and throws `ACP_BACKEND_UNSUPPORTED_CONTROL` for any pair the backend did not advertise. claude-agent-acp does not advertise a `timeout` alias. - The wrapper then forwards remaining pairs to the delegate. The Codex ACP command was already short-circuited there; every other command, including claude-agent-acp, fell through. Net effect on the reporter's scenario: `sessions_spawn({ runtime:"acp", agentId:"claude", timeoutSeconds: 60 })` failed at the control-plane validation with `ACP_BACKEND_UNSUPPORTED_CONTROL` (and, had it reached the wire, claude-agent-acp would have answered `-32603 Internal error / Unknown config option: timeout`, surfacing as `ACP_TURN_FAILED: Internal error`). Fix two layers: 1. Control plane (`src/acp/control-plane/runtime-options.ts`): add `isTimeoutConfigOptionAdvertised(advertisedConfigOptionKeys)` and gate the timeout pair on it. When advertised keys are unknown (`undefined` or empty), keep emitting the pair — this preserves current behavior for backends that have not produced a capability list yet. When advertised keys are present but exclude every alias in `RUNTIME_CONFIG_OPTION_ALIASES.timeoutSeconds`, skip the pair. The per-turn timeout is still enforced in-process via `AcpSessionManager.resolveTurnTimeoutMs` in `manager.core.ts`. 2. ACPX wrapper (`extensions/acpx/src/runtime.ts`): hoist the Codex `timeout` / `timeout_seconds` suppression so it also applies to claude-agent-acp commands. Add `isClaudeAcpCommand` mirroring `isCodexAcpCommand` (package spec, binary, generated wrapper script). This layer is defense in depth — relevant when callers reach the wrapper without going through `applyManagerRuntimeControls`, or when advertised keys are not yet known. Coverage: - `src/acp/control-plane/runtime-options.test.ts` (new) asserts: - the timeout pair is omitted when advertised keys exclude every alias, - the pair is kept when `timeout` or `timeout_seconds` is advertised, - the pair is kept when advertised keys are unknown, - model/thinking emission is unaffected. - `extensions/acpx/src/runtime.test.ts` flips the previous `forwards timeout config controls for non-Codex ACP agents` test, which codified the buggy behavior, into a suppression assertion. Adds a positive `still forwards non-timeout config controls for claude-agent-acp` test and an `isClaudeAcpCommand` detector test. Closes #81127 --- extensions/acpx/src/runtime.test.ts | 74 ++++++++++++++++++- extensions/acpx/src/runtime.ts | 42 ++++++++++- src/acp/control-plane/runtime-options.test.ts | 47 ++++++++++++ src/acp/control-plane/runtime-options.ts | 24 +++++- 4 files changed, 180 insertions(+), 7 deletions(-) create mode 100644 src/acp/control-plane/runtime-options.test.ts diff --git a/extensions/acpx/src/runtime.test.ts b/extensions/acpx/src/runtime.test.ts index 5d410573ded..a236aad69c8 100644 --- a/extensions/acpx/src/runtime.test.ts +++ b/extensions/acpx/src/runtime.test.ts @@ -475,7 +475,14 @@ describe("AcpxRuntime fresh reset wrapper", () => { expect(setConfigOption).not.toHaveBeenCalled(); }); - it("forwards timeout config controls for non-Codex ACP agents", async () => { + it("ignores unsupported claude-agent-acp timeout config controls", async () => { + // Regression for openclaw/openclaw#81127: claude-agent-acp rejects any + // `session/set_config_option` whose configId it does not advertise + // (`Unknown config option: timeout`), which previously surfaced to callers + // as ACP_TURN_FAILED whenever `sessions_spawn` was invoked with + // `timeoutSeconds`. OpenClaw still enforces the per-turn timeout + // in-process via runTimeoutSeconds; the wire option just must not leave + // the wrapper. const baseStore: TestSessionStore = { load: vi.fn(async () => ({ acpxRecordId: "agent:claude:acp:test", @@ -497,15 +504,76 @@ describe("AcpxRuntime fresh reset wrapper", () => { key: "timeout", value: "60", }); + await runtime.setConfigOption({ + handle, + key: "Timeout_Seconds", + value: "60", + }); + + expect(setConfigOption).not.toHaveBeenCalled(); + }); + + it("still forwards non-timeout config controls for claude-agent-acp", async () => { + const baseStore: TestSessionStore = { + load: vi.fn(async () => ({ + acpxRecordId: "agent:claude:acp:test", + agentCommand: "npx @agentclientprotocol/claude-agent-acp", + })), + save: vi.fn(async () => {}), + }; + const { runtime, delegate } = makeRuntime(baseStore); + const setConfigOption = vi.spyOn(delegate, "setConfigOption").mockResolvedValue(undefined); + const handle: Parameters>[0]["handle"] = { + sessionKey: "agent:claude:acp:test", + backend: "acpx", + runtimeSessionName: "agent:claude:acp:test", + acpxRecordId: "agent:claude:acp:test", + }; + + await runtime.setConfigOption({ + handle, + key: "model", + value: "claude-sonnet-4.6", + }); expect(setConfigOption).toHaveBeenCalledOnce(); expect(setConfigOption).toHaveBeenCalledWith({ handle, - key: "timeout", - value: "60", + key: "model", + value: "claude-sonnet-4.6", }); }); + it("recognizes claude-agent-acp commands", () => { + expect(__testing.isClaudeAcpCommand("npx @agentclientprotocol/claude-agent-acp")).toBe(true); + expect( + __testing.isClaudeAcpCommand("npx -y @agentclientprotocol/claude-agent-acp@0.33.1"), + ).toBe(true); + expect(__testing.isClaudeAcpCommand("claude-agent-acp")).toBe(true); + expect(__testing.isClaudeAcpCommand("claude-agent-acp.exe")).toBe(true); + expect( + __testing.isClaudeAcpCommand(`node "/tmp/openclaw/acpx/claude-agent-acp-wrapper.mjs"`), + ).toBe(true); + // Generated ACP wrapper commands are built from `process.execPath`, which + // is `node.exe` on Windows. The detector must accept that launcher so the + // bundled Claude ACP wrapper path does not slip past the timeout + // suppression on Windows hosts. Generated commands use forward-slash + // paths (Node accepts those on Windows) — matching the same pattern as + // the existing CODEX_ACP_WRAPPER_COMMAND fixture. + expect( + __testing.isClaudeAcpCommand( + `node.exe "C:/Users/runner/AppData/Local/Temp/openclaw/acpx/claude-agent-acp-wrapper.mjs"`, + ), + ).toBe(true); + expect( + __testing.isClaudeAcpCommand( + `Node.EXE "C:/Users/runner/AppData/Local/Temp/openclaw/acpx/claude-agent-acp-wrapper.mjs"`, + ), + ).toBe(true); + expect(__testing.isClaudeAcpCommand("openclaw acp")).toBe(false); + expect(__testing.isClaudeAcpCommand("npx @zed-industries/codex-acp")).toBe(false); + }); + it("keeps stale persistent loads hidden until a fresh record is saved", async () => { const baseStore: TestSessionStore = { load: vi.fn(async () => ({ acpxRecordId: "stale" }) as never), diff --git a/extensions/acpx/src/runtime.ts b/extensions/acpx/src/runtime.ts index 369f331d031..38e57336f7f 100644 --- a/extensions/acpx/src/runtime.ts +++ b/extensions/acpx/src/runtime.ts @@ -390,6 +390,34 @@ function isCodexAcpCommand(command: string | undefined): boolean { return /^codex-acp(?:-wrapper)?(?:\.[cm]?js)?$/i.test(scriptName); } +function isClaudeAcpPackageSpec(value: string): boolean { + return /^@agentclientprotocol\/claude-agent-acp(?:@.+)?$/i.test(value.trim()); +} + +function isClaudeAcpCommand(command: string | undefined): boolean { + if (!command) { + return false; + } + const parts = unwrapEnvCommand(splitCommandParts(command.trim())); + if (!parts.length) { + return false; + } + if (parts.some(isClaudeAcpPackageSpec)) { + return true; + } + const commandName = basename(parts[0] ?? ""); + if (/^claude-agent-acp(?:\.exe)?$/i.test(commandName)) { + return true; + } + // Generated wrappers are launched via `process.execPath`, which is + // `node.exe` on Windows. + if (!/^node(?:\.exe)?$/i.test(commandName)) { + return false; + } + const scriptName = basename(parts[1] ?? ""); + return /^claude-agent-acp(?:-wrapper)?(?:\.[cm]?js)?$/i.test(scriptName); +} + function failUnsupportedCodexAcpModel(rawModel: string, detail?: string): never { throw new AcpRuntimeError( "ACP_INVALID_RUNTIME_OPTION", @@ -889,10 +917,17 @@ export class AcpxRuntime implements AcpRuntime { const delegate = await this.resolveDelegateForHandle(input.handle); const command = await this.resolveCommandForHandle(input.handle); const key = input.key.trim().toLowerCase(); + // Neither @zed-industries/codex-acp nor @agentclientprotocol/claude-agent-acp + // advertises a `timeout` config option; forwarding one triggers a JSON-RPC + // rejection that surfaces as ACP_TURN_FAILED. OpenClaw still enforces the + // per-turn timeout in-process via runTimeoutSeconds. + if ( + (key === "timeout" || key === "timeout_seconds") && + (isCodexAcpCommand(command) || isClaudeAcpCommand(command)) + ) { + return; + } if (isCodexAcpCommand(command)) { - if (key === "timeout" || key === "timeout_seconds") { - return; - } if ( key === "model" || key === "thinking" || @@ -974,6 +1009,7 @@ export const __testing = { appendCodexAcpConfigOverrides, assertSupportedRuntimeSessionMode, codexAcpSessionModelId, + isClaudeAcpCommand, isCodexAcpCommand, normalizeCodexAcpModelOverride, }; diff --git a/src/acp/control-plane/runtime-options.test.ts b/src/acp/control-plane/runtime-options.test.ts new file mode 100644 index 00000000000..f89025fbebc --- /dev/null +++ b/src/acp/control-plane/runtime-options.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import { buildRuntimeConfigOptionPairs } from "./runtime-options.js"; + +describe("buildRuntimeConfigOptionPairs timeout advertisement", () => { + // Regression for openclaw/openclaw#81127: when a backend (e.g. claude-agent-acp) + // does not advertise a `timeout` config option, applyManagerRuntimeControls + // throws ACP_BACKEND_UNSUPPORTED_CONTROL before the runtime wrapper sees the + // call. The pair must not be emitted in that case. OpenClaw's per-turn + // timeout is still enforced in-process via manager.core.resolveTurnTimeoutMs. + it("omits the timeout pair when advertised keys exclude every timeout alias", () => { + const pairs = buildRuntimeConfigOptionPairs({ timeoutSeconds: 60 }, [ + "model", + "thinking", + "approval_policy", + ]); + expect(pairs).toEqual([]); + }); + + it("keeps the timeout pair when advertised keys include `timeout`", () => { + const pairs = buildRuntimeConfigOptionPairs({ timeoutSeconds: 60 }, ["model", "timeout"]); + expect(pairs).toEqual([["timeout", "60"]]); + }); + + it("keeps the timeout pair using the advertised `timeout_seconds` alias", () => { + const pairs = buildRuntimeConfigOptionPairs({ timeoutSeconds: 60 }, [ + "model", + "timeout_seconds", + ]); + expect(pairs).toEqual([["timeout_seconds", "60"]]); + }); + + it("keeps the timeout pair when advertised keys are unknown (empty or undefined)", () => { + expect(buildRuntimeConfigOptionPairs({ timeoutSeconds: 60 })).toEqual([["timeout", "60"]]); + expect(buildRuntimeConfigOptionPairs({ timeoutSeconds: 60 }, [])).toEqual([["timeout", "60"]]); + }); + + it("does not affect model or thinking emission when only timeout is unadvertised", () => { + const pairs = buildRuntimeConfigOptionPairs( + { model: "claude-sonnet-4.6", thinking: "high", timeoutSeconds: 60 }, + ["model", "thinking"], + ); + expect(pairs).toEqual([ + ["model", "claude-sonnet-4.6"], + ["thinking", "high"], + ]); + }); +}); diff --git a/src/acp/control-plane/runtime-options.ts b/src/acp/control-plane/runtime-options.ts index 1cfd74a0d3c..a53c28b8d51 100644 --- a/src/acp/control-plane/runtime-options.ts +++ b/src/acp/control-plane/runtime-options.ts @@ -340,7 +340,10 @@ export function buildRuntimeConfigOptionPairs( normalized.permissionProfile, ); } - if (typeof normalized.timeoutSeconds === "number") { + if ( + typeof normalized.timeoutSeconds === "number" && + isTimeoutConfigOptionAdvertised(advertisedConfigOptionKeys) + ) { pairs.set( resolveRuntimeConfigOptionKey("timeout", advertisedConfigOptionKeys), String(normalized.timeoutSeconds), @@ -355,6 +358,25 @@ export function buildRuntimeConfigOptionPairs( return [...pairs.entries()]; } +/** + * Returns true when the backend's advertised config keys either include a + * timeout alias or are unknown to us (`undefined` / empty array). Returns + * false only when advertisement info is present and lists no timeout alias + * — in which case `buildRuntimeConfigOptionPairs` must NOT emit the pair, + * because `applyManagerRuntimeControls` would otherwise reject the pre-turn + * apply with `ACP_BACKEND_UNSUPPORTED_CONTROL`. OpenClaw's per-turn timeout + * is still enforced in-process via `resolveTurnTimeoutMs` in `manager.core`. + */ +function isTimeoutConfigOptionAdvertised(advertisedConfigOptionKeys?: readonly string[]): boolean { + const advertisedKeys = buildAdvertisedConfigOptionKeyMap(advertisedConfigOptionKeys); + if (advertisedKeys.size === 0) { + return true; + } + return RUNTIME_CONFIG_OPTION_ALIASES.timeoutSeconds.some((alias) => + advertisedKeys.has(normalizeLowercaseStringOrEmpty(alias)), + ); +} + function buildAdvertisedConfigOptionKeyMap( advertisedConfigOptionKeys?: readonly string[], ): Map {