From a146bf03dbf68ca85fb5f04a86db232abf72fe62 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Wed, 13 May 2026 11:25:11 +0530 Subject: [PATCH] refactor(acpx): distill ACP command detection --- extensions/acpx/src/runtime.test.ts | 13 --- extensions/acpx/src/runtime.ts | 108 +++++++++--------- src/acp/control-plane/runtime-options.test.ts | 5 - src/acp/control-plane/runtime-options.ts | 23 ++-- 4 files changed, 58 insertions(+), 91 deletions(-) diff --git a/extensions/acpx/src/runtime.test.ts b/extensions/acpx/src/runtime.test.ts index a236aad69c8..2674b428c57 100644 --- a/extensions/acpx/src/runtime.test.ts +++ b/extensions/acpx/src/runtime.test.ts @@ -476,13 +476,6 @@ describe("AcpxRuntime fresh reset wrapper", () => { }); 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", @@ -554,12 +547,6 @@ describe("AcpxRuntime fresh reset wrapper", () => { 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"`, diff --git a/extensions/acpx/src/runtime.ts b/extensions/acpx/src/runtime.ts index 38e57336f7f..422e64ad6d2 100644 --- a/extensions/acpx/src/runtime.ts +++ b/extensions/acpx/src/runtime.ts @@ -349,6 +349,45 @@ function unwrapEnvCommand(parts: string[]): string[] { return parts.slice(index); } +function matchesExecutableName(value: string, executableName: string): boolean { + const normalized = basename(value).toLowerCase(); + return normalized === executableName || normalized === `${executableName}.exe`; +} + +function matchesPackageSpec(value: string, packageName: string): boolean { + const normalized = value.trim().toLowerCase(); + return normalized === packageName || normalized.startsWith(`${packageName}@`); +} + +function stripModuleExtension(value: string): string { + return value.replace(/\.[cm]?js$/i, "").toLowerCase(); +} + +function isAcpCommand( + command: string | undefined, + params: { packageName: string; executableName: string }, +): boolean { + if (!command) { + return false; + } + const parts = unwrapEnvCommand(splitCommandParts(command.trim())); + if (!parts.length) { + return false; + } + if (parts.some((part) => matchesPackageSpec(part, params.packageName))) { + return true; + } + const commandName = basename(parts[0] ?? ""); + if (matchesExecutableName(commandName, params.executableName)) { + return true; + } + if (!matchesExecutableName(commandName, "node")) { + return false; + } + const scriptName = stripModuleExtension(basename(parts[1] ?? "")); + return scriptName === params.executableName || scriptName === `${params.executableName}-wrapper`; +} + function isOpenClawBridgeCommand(command: string | undefined): boolean { if (!command) { return false; @@ -364,58 +403,18 @@ function isOpenClawBridgeCommand(command: string | undefined): boolean { return /^openclaw(?:\.[cm]?js)?$/i.test(scriptName) && parts[2] === OPENCLAW_BRIDGE_SUBCOMMAND; } -function isCodexAcpPackageSpec(value: string): boolean { - return /^@zed-industries\/codex-acp(?:@.+)?$/i.test(value.trim()); -} - function isCodexAcpCommand(command: string | undefined): boolean { - if (!command) { - return false; - } - const parts = unwrapEnvCommand(splitCommandParts(command.trim())); - if (!parts.length) { - return false; - } - if (parts.some(isCodexAcpPackageSpec)) { - return true; - } - const commandName = basename(parts[0] ?? ""); - if (/^codex-acp(?:\.exe)?$/i.test(commandName)) { - return true; - } - if (commandName !== "node") { - return false; - } - const scriptName = basename(parts[1] ?? ""); - return /^codex-acp(?:-wrapper)?(?:\.[cm]?js)?$/i.test(scriptName); -} - -function isClaudeAcpPackageSpec(value: string): boolean { - return /^@agentclientprotocol\/claude-agent-acp(?:@.+)?$/i.test(value.trim()); + return isAcpCommand(command, { + packageName: "@zed-industries/codex-acp", + executableName: "codex-acp", + }); } 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); + return isAcpCommand(command, { + packageName: "@agentclientprotocol/claude-agent-acp", + executableName: "claude-agent-acp", + }); } function failUnsupportedCodexAcpModel(rawModel: string, detail?: string): never { @@ -431,6 +430,7 @@ function failUnsupportedCodexAcpModel(rawModel: string, detail?: string): never // `SessionResumeRequiredError` on agent restart. Fail fast at this boundary instead. // See openclaw/openclaw#73071. const SUPPORTED_RUNTIME_SESSION_MODES = new Set(["persistent", "oneshot"] as const); +const WIRE_TIMEOUT_CONFIG_KEYS = new Set(["timeout", "timeout_seconds"]); function assertSupportedRuntimeSessionMode( mode: unknown, @@ -917,17 +917,11 @@ 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)) - ) { + const isCodexAcp = isCodexAcpCommand(command); + if (WIRE_TIMEOUT_CONFIG_KEYS.has(key) && (isCodexAcp || isClaudeAcpCommand(command))) { return; } - if (isCodexAcpCommand(command)) { + if (isCodexAcp) { if ( key === "model" || key === "thinking" || diff --git a/src/acp/control-plane/runtime-options.test.ts b/src/acp/control-plane/runtime-options.test.ts index f89025fbebc..37daebe0864 100644 --- a/src/acp/control-plane/runtime-options.test.ts +++ b/src/acp/control-plane/runtime-options.test.ts @@ -2,11 +2,6 @@ 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", diff --git a/src/acp/control-plane/runtime-options.ts b/src/acp/control-plane/runtime-options.ts index a53c28b8d51..13a97316ac9 100644 --- a/src/acp/control-plane/runtime-options.ts +++ b/src/acp/control-plane/runtime-options.ts @@ -342,7 +342,7 @@ export function buildRuntimeConfigOptionPairs( } if ( typeof normalized.timeoutSeconds === "number" && - isTimeoutConfigOptionAdvertised(advertisedConfigOptionKeys) + shouldEmitTimeoutConfigOption(advertisedConfigOptionKeys) ) { pairs.set( resolveRuntimeConfigOptionKey("timeout", advertisedConfigOptionKeys), @@ -358,22 +358,13 @@ 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 { +function shouldEmitTimeoutConfigOption(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)), + return ( + advertisedKeys.size === 0 || + RUNTIME_CONFIG_OPTION_ALIASES.timeoutSeconds.some((alias) => + advertisedKeys.has(normalizeLowercaseStringOrEmpty(alias)), + ) ); }