From 96b55821bc2c9c3d28c8ff6725a24b8cd7e908d0 Mon Sep 17 00:00:00 2001 From: Devin Robison Date: Thu, 2 Apr 2026 10:45:41 -0600 Subject: [PATCH] fix: share ACP owner-only approval classes (#201) (#59255) Co-authored-by: OpenClaw Dummy Agent --- CHANGELOG.md | 1 + src/acp/approval-classifier.test.ts | 46 +++++++++++++++++++++-------- src/acp/approval-classifier.ts | 18 ++++------- src/acp/client.test.ts | 45 ++++++++++++++++++++++++++++ src/agents/tool-policy.test.ts | 24 +++++++++++++++ src/agents/tool-policy.ts | 20 +++++++++---- 6 files changed, 124 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c7293f1955..df034415137 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -226,6 +226,7 @@ Docs: https://docs.openclaw.ai - Slack: stop retry-driven duplicate replies when draft-finalization edits fail ambiguously, and log configured allowlisted users/channels by readable name instead of raw IDs. - Agents/OpenAI Responses: normalize raw bundled MCP tool schemas on the WebSocket/Responses path so bare-object, object-ish, and top-level union MCP tools no longer get rejected by OpenAI during tool registration. (#58299) Thanks @yelog. - ACP/security: replace ACP's dangerous-tool name override with semantic approval classes, so only narrow readonly reads/searches can auto-approve while indirect exec-capable and control-plane tools always require explicit prompt approval. Thanks @vincentkoc. +- ACP: derive owner-only approval classes from the shared tool-policy fallback map so `cron`, `nodes`, and `whatsapp_login` cannot drift out of prompt-required coverage. - ACP/sessions_spawn: register ACP child runs for completion tracking and lifecycle cleanup, and make registration-failure cleanup explicitly best-effort so callers do not assume an already-started ACP turn was fully aborted. (#40885) Thanks @xaeon2026 and @vincentkoc. - ACP/tasks: mark cleanly exited ACP runs as blocked when they end on deterministic write or authorization blockers, and wake the parent session with a follow-up instead of falsely reporting success. - ACPX/runtime: derive the bundled ACPX expected version from the extension package metadata instead of hardcoding a separate literal, so plugin-local ACPX installs stop drifting out of health-check parity after version bumps. (#49089) Thanks @jiejiesks and @vincentkoc. diff --git a/src/acp/approval-classifier.test.ts b/src/acp/approval-classifier.test.ts index a6a3fe2b05a..5359db66c54 100644 --- a/src/acp/approval-classifier.test.ts +++ b/src/acp/approval-classifier.test.ts @@ -70,18 +70,40 @@ describe("classifyAcpToolApproval", () => { }); }); - it("classifies nodes as exec-capable even for list actions", () => { - expect( - classify({ - title: "nodes: list", - rawInput: { name: "nodes", action: "list" }, - }), - ).toEqual({ - toolName: "nodes", - approvalClass: "exec_capable", - autoApprove: false, - }); - }); + it.each([ + { + title: "cron: status", + rawInput: { name: "cron", action: "status" }, + expectedToolName: "cron", + expectedClass: "control_plane", + }, + { + title: "nodes: list", + rawInput: { name: "nodes", action: "list" }, + expectedToolName: "nodes", + expectedClass: "exec_capable", + }, + { + title: "whatsapp_login: start", + rawInput: { name: "whatsapp_login" }, + expectedToolName: "whatsapp_login", + expectedClass: "interactive", + }, + ] as const)( + "classifies shared owner-only ACP backstops for $expectedToolName", + ({ title, rawInput, expectedToolName, expectedClass }) => { + expect( + classify({ + title, + rawInput, + }), + ).toEqual({ + toolName: expectedToolName, + approvalClass: expectedClass, + autoApprove: false, + }); + }, + ); it("classifies gateway as control-plane", () => { expect( diff --git a/src/acp/approval-classifier.ts b/src/acp/approval-classifier.ts index f28d116a83a..2bcb2f0b059 100644 --- a/src/acp/approval-classifier.ts +++ b/src/acp/approval-classifier.ts @@ -2,6 +2,7 @@ import { homedir } from "node:os"; import path from "node:path"; import { isKnownCoreToolId } from "../agents/tool-catalog.js"; import { isMutatingToolCall } from "../agents/tool-mutation.js"; +import { resolveOwnerOnlyToolApprovalClass } from "../agents/tool-policy.js"; const SAFE_SEARCH_TOOL_IDS = new Set(["search", "web_search", "memory_search"]); const TRUSTED_SAFE_TOOL_ALIASES = new Set(["search"]); @@ -11,17 +12,9 @@ const EXEC_CAPABLE_TOOL_IDS = new Set([ "shell", "bash", "process", - "nodes", "code_execution", ]); -const CONTROL_PLANE_TOOL_IDS = new Set([ - "gateway", - "cron", - "sessions_spawn", - "sessions_send", - "session_status", -]); -const INTERACTIVE_TOOL_IDS = new Set(["whatsapp_login"]); +const CONTROL_PLANE_TOOL_IDS = new Set(["sessions_spawn", "sessions_send", "session_status"]); export type AcpApprovalClass = | "readonly_scoped" @@ -218,15 +211,16 @@ export function classifyAcpToolApproval(params: { if (SAFE_SEARCH_TOOL_IDS.has(toolName) && isTrustedToolId) { return { toolName, approvalClass: "readonly_search", autoApprove: true }; } + const ownerOnlyApprovalClass = resolveOwnerOnlyToolApprovalClass(toolName); + if (ownerOnlyApprovalClass) { + return { toolName, approvalClass: ownerOnlyApprovalClass, autoApprove: false }; + } if (EXEC_CAPABLE_TOOL_IDS.has(toolName)) { return { toolName, approvalClass: "exec_capable", autoApprove: false }; } if (CONTROL_PLANE_TOOL_IDS.has(toolName)) { return { toolName, approvalClass: "control_plane", autoApprove: false }; } - if (INTERACTIVE_TOOL_IDS.has(toolName)) { - return { toolName, approvalClass: "interactive", autoApprove: false }; - } if (isMutatingToolCall(toolName, params.toolCall?.rawInput)) { return { toolName, approvalClass: "mutating", autoApprove: false }; } diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts index c1acd16f05c..c30a5493979 100644 --- a/src/acp/client.test.ts +++ b/src/acp/client.test.ts @@ -400,6 +400,51 @@ describe("resolvePermissionRequest", () => { expect(res).toEqual({ outcome: { outcome: "selected", optionId: "allow" } }); }); + it.each([ + { + toolName: "cron", + title: "cron: status", + rawInput: { + name: "cron", + action: "status", + }, + }, + { + toolName: "nodes", + title: "nodes: list", + rawInput: { + name: "nodes", + action: "list", + }, + }, + { + toolName: "whatsapp_login", + title: "whatsapp_login: start", + rawInput: { + name: "whatsapp_login", + }, + }, + ] as const)( + "prompts for shared owner-only backstop tools: $toolName", + async ({ toolName, title, rawInput }) => { + const prompt = vi.fn(async () => true); + const res = await resolvePermissionRequest( + makePermissionRequest({ + toolCall: { + toolCallId: `tool-${toolName}`, + title, + status: "pending", + rawInput, + }, + }), + { prompt, log: () => {} }, + ); + expect(prompt).toHaveBeenCalledTimes(1); + expect(prompt).toHaveBeenCalledWith(toolName, title); + expect(res).toEqual({ outcome: { outcome: "selected", optionId: "allow" } }); + }, + ); + it("auto-approves search without prompting", async () => { const prompt = vi.fn(async () => true); const res = await resolvePermissionRequest( diff --git a/src/agents/tool-policy.test.ts b/src/agents/tool-policy.test.ts index 963c703a409..686ed50f81b 100644 --- a/src/agents/tool-policy.test.ts +++ b/src/agents/tool-policy.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; +import { DEFAULT_GATEWAY_HTTP_TOOL_DENY } from "../security/dangerous-tools.js"; import { isToolAllowed, resolveSandboxToolPolicyForAgent } from "./sandbox/tool-policy.js"; import type { SandboxToolPolicy } from "./sandbox/types.js"; import { TOOL_POLICY_CONFORMANCE } from "./tool-policy.conformance.js"; @@ -8,6 +9,7 @@ import { expandToolGroups, isOwnerOnlyToolName, normalizeToolName, + resolveOwnerOnlyToolApprovalClass, resolveToolProfilePolicy, TOOL_GROUPS, } from "./tool-policy.js"; @@ -84,6 +86,28 @@ describe("tool-policy", () => { expect(isOwnerOnlyToolName("read")).toBe(false); }); + it("exposes stable approval classes for shared owner-only fallbacks", () => { + expect(resolveOwnerOnlyToolApprovalClass("whatsapp_login")).toBe("interactive"); + expect(resolveOwnerOnlyToolApprovalClass("cron")).toBe("control_plane"); + expect(resolveOwnerOnlyToolApprovalClass("gateway")).toBe("control_plane"); + expect(resolveOwnerOnlyToolApprovalClass("nodes")).toBe("exec_capable"); + expect(resolveOwnerOnlyToolApprovalClass("read")).toBeUndefined(); + }); + + it("keeps ACP owner-only backstops aligned with the HTTP deny list", () => { + const sharedBackstops = DEFAULT_GATEWAY_HTTP_TOOL_DENY.flatMap((name) => { + const approvalClass = resolveOwnerOnlyToolApprovalClass(name); + return approvalClass ? ([[name, approvalClass]] as const) : []; + }); + + expect(Object.fromEntries(sharedBackstops)).toEqual({ + cron: "control_plane", + gateway: "control_plane", + nodes: "exec_capable", + whatsapp_login: "interactive", + }); + }); + it("strips owner-only tools for non-owner senders", async () => { const tools = createOwnerPolicyTools(); const filtered = applyOwnerOnlyToolPolicy(tools, false); diff --git a/src/agents/tool-policy.ts b/src/agents/tool-policy.ts index 5538fb765ce..affcdc6279a 100644 --- a/src/agents/tool-policy.ts +++ b/src/agents/tool-policy.ts @@ -15,6 +15,8 @@ export { } from "./tool-policy-shared.js"; export type { ToolProfileId } from "./tool-policy-shared.js"; +export type OwnerOnlyToolApprovalClass = "control_plane" | "exec_capable" | "interactive"; + // Keep tool-policy browser-safe: do not import tools/common at runtime. function wrapOwnerOnlyToolExecution(tool: AnyAgentTool, senderIsOwner: boolean): AnyAgentTool { if (tool.ownerOnly !== true || senderIsOwner || !tool.execute) { @@ -28,15 +30,21 @@ function wrapOwnerOnlyToolExecution(tool: AnyAgentTool, senderIsOwner: boolean): }; } -const OWNER_ONLY_TOOL_NAME_FALLBACKS = new Set([ - "whatsapp_login", - "cron", - "gateway", - "nodes", +const OWNER_ONLY_TOOL_APPROVAL_CLASS_FALLBACKS = new Map([ + ["whatsapp_login", "interactive"], + ["cron", "control_plane"], + ["gateway", "control_plane"], + ["nodes", "exec_capable"], ]); +export function resolveOwnerOnlyToolApprovalClass( + name: string, +): OwnerOnlyToolApprovalClass | undefined { + return OWNER_ONLY_TOOL_APPROVAL_CLASS_FALLBACKS.get(normalizeToolName(name)); +} + export function isOwnerOnlyToolName(name: string) { - return OWNER_ONLY_TOOL_NAME_FALLBACKS.has(normalizeToolName(name)); + return resolveOwnerOnlyToolApprovalClass(name) !== undefined; } function isOwnerOnlyTool(tool: AnyAgentTool) {