mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-20 13:54:49 +00:00
fix(acp): drop unsupported timeout config option for claude-agent-acp
`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
This commit is contained in:
@@ -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<NonNullable<AcpRuntime["setConfigOption"]>>[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),
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
47
src/acp/control-plane/runtime-options.test.ts
Normal file
47
src/acp/control-plane/runtime-options.test.ts
Normal file
@@ -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"],
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -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<string, string> {
|
||||
|
||||
Reference in New Issue
Block a user