mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-08 17:30:42 +00:00
fix(auto-reply): gate inline skill tool dispatch [AI] (#78517)
* fix: enforce tool hooks for inline skill dispatch * addressing claude review * addressing codex review * addressing codex review * fix: complete root-cause handling * docs: add changelog entry for PR merge
This commit is contained in:
committed by
GitHub
parent
79d9b95e67
commit
d5eabbd36c
@@ -141,6 +141,7 @@ Docs: https://docs.openclaw.ai
|
||||
|
||||
### Fixes
|
||||
|
||||
- fix(auto-reply): gate inline skill tool dispatch [AI]. (#78517) Thanks @pgondhi987.
|
||||
- Canvas plugin: keep legacy root `canvasHost` configs valid until `openclaw doctor --fix` migrates them into `plugins.entries.canvas.config.host`, move Canvas/A2UI clients to gateway protocol v4 plugin surfaces, and refresh the generated A2UI bundle hash so normal builds stay clean.
|
||||
- feishu: honor config write policy for dynamic agents [AI]. (#78520) Thanks @pgondhi987.
|
||||
- fix(skill-workshop): honor pending approval for tool suggestions [AI]. (#78516) Thanks @pgondhi987.
|
||||
|
||||
@@ -22,9 +22,15 @@ import {
|
||||
collectPresentOpenClawTools,
|
||||
isUpdatePlanToolEnabledForOpenClawTools,
|
||||
} from "./openclaw-tools.registration.js";
|
||||
import {
|
||||
type HookContext,
|
||||
isToolWrappedWithBeforeToolCallHook,
|
||||
wrapToolWithBeforeToolCallHook,
|
||||
} from "./pi-tools.before-tool-call.js";
|
||||
import type { SandboxFsBridge } from "./sandbox/fs-bridge.js";
|
||||
import type { SpawnedToolContext } from "./spawned-context.js";
|
||||
import type { ToolFsPolicy } from "./tool-fs-policy.js";
|
||||
import { resolveToolLoopDetectionConfig } from "./tool-loop-detection-config.js";
|
||||
import { createAgentsListTool } from "./tools/agents-list-tool.js";
|
||||
import type { AnyAgentTool } from "./tools/common.js";
|
||||
import { createCronTool } from "./tools/cron-tool.js";
|
||||
@@ -117,6 +123,14 @@ export function createOpenClawTools(
|
||||
enableHeartbeatTool?: boolean;
|
||||
/** If true, skip plugin tool resolution and return only shipped core tools. */
|
||||
disablePluginTools?: boolean;
|
||||
/**
|
||||
* Wrap returned tools with the before_tool_call hook at construction time.
|
||||
* Defaults to true; callers that already enforce the hook at a later shared
|
||||
* boundary should opt out explicitly.
|
||||
*/
|
||||
wrapBeforeToolCallHook?: boolean;
|
||||
/** Override or extend the default hook context used by construction-time wrapping. */
|
||||
beforeToolCallHookContext?: HookContext;
|
||||
/** Records hot-path tool-prep stages for reply startup diagnostics. */
|
||||
recordToolPrepStage?: (name: string) => void;
|
||||
/** Trusted sender id from inbound context (not tool args). */
|
||||
@@ -413,23 +427,45 @@ export function createOpenClawTools(
|
||||
...collectPresentOpenClawTools([webSearchTool, webFetchTool, imageTool, pdfTool]),
|
||||
];
|
||||
options?.recordToolPrepStage?.("openclaw-tools:core-tool-list");
|
||||
|
||||
if (options?.disablePluginTools) {
|
||||
return tools;
|
||||
let allTools = tools;
|
||||
if (!options?.disablePluginTools) {
|
||||
const existingToolNames = new Set<string>();
|
||||
for (const tool of tools) {
|
||||
existingToolNames.add(tool.name);
|
||||
}
|
||||
allTools = [
|
||||
...tools,
|
||||
...resolveOpenClawPluginToolsForOptions({
|
||||
options,
|
||||
resolvedConfig,
|
||||
existingToolNames,
|
||||
}),
|
||||
];
|
||||
options?.recordToolPrepStage?.("openclaw-tools:plugin-tools");
|
||||
}
|
||||
|
||||
const existingToolNames = new Set<string>();
|
||||
for (const tool of tools) {
|
||||
existingToolNames.add(tool.name);
|
||||
if (options?.wrapBeforeToolCallHook === false) {
|
||||
return allTools;
|
||||
}
|
||||
const wrappedPluginTools = resolveOpenClawPluginToolsForOptions({
|
||||
options,
|
||||
resolvedConfig,
|
||||
existingToolNames,
|
||||
});
|
||||
options?.recordToolPrepStage?.("openclaw-tools:plugin-tools");
|
||||
|
||||
return [...tools, ...wrappedPluginTools];
|
||||
const hookAgentId = options?.requesterAgentIdOverride ?? sessionAgentId;
|
||||
const defaultHookContext: HookContext = {
|
||||
...(hookAgentId ? { agentId: hookAgentId } : {}),
|
||||
...(resolvedConfig ? { config: resolvedConfig } : {}),
|
||||
...(options?.agentSessionKey ? { sessionKey: options.agentSessionKey } : {}),
|
||||
...(options?.sessionId ? { sessionId: options.sessionId } : {}),
|
||||
...(options?.currentChannelId ? { channelId: options.currentChannelId } : {}),
|
||||
loopDetection: resolveToolLoopDetectionConfig({ cfg: resolvedConfig, agentId: hookAgentId }),
|
||||
};
|
||||
const hookContext = {
|
||||
...defaultHookContext,
|
||||
...options?.beforeToolCallHookContext,
|
||||
};
|
||||
options?.recordToolPrepStage?.("openclaw-tools:tool-hooks");
|
||||
return allTools.map((tool) =>
|
||||
isToolWrappedWithBeforeToolCallHook(tool)
|
||||
? tool
|
||||
: wrapToolWithBeforeToolCallHook(tool, hookContext),
|
||||
);
|
||||
}
|
||||
|
||||
export const __testing = {
|
||||
|
||||
@@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest";
|
||||
import type { OpenClawConfig } from "../config/config.js";
|
||||
import { createOpenClawTools } from "./openclaw-tools.js";
|
||||
import { isUpdatePlanToolEnabledForOpenClawTools } from "./openclaw-tools.registration.js";
|
||||
import { isToolWrappedWithBeforeToolCallHook } from "./pi-tools.before-tool-call.js";
|
||||
import { createUpdatePlanTool } from "./tools/update-plan-tool.js";
|
||||
|
||||
type UpdatePlanGatingParams = Parameters<typeof isUpdatePlanToolEnabledForOpenClawTools>[0];
|
||||
@@ -51,6 +52,27 @@ describe("openclaw-tools update_plan gating", () => {
|
||||
expect(emptyAllowlistTools.some((tool) => tool.name === "update_plan")).toBe(false);
|
||||
});
|
||||
|
||||
it("wraps constructed tools with before-tool-call hooks by default", () => {
|
||||
const tools = createOpenClawTools({
|
||||
config: {} as OpenClawConfig,
|
||||
disablePluginTools: true,
|
||||
});
|
||||
const unwrappedTools = createOpenClawTools({
|
||||
config: {} as OpenClawConfig,
|
||||
disablePluginTools: true,
|
||||
wrapBeforeToolCallHook: false,
|
||||
});
|
||||
|
||||
expect(
|
||||
isToolWrappedWithBeforeToolCallHook(tools.find((tool) => tool.name === "sessions_list")!),
|
||||
).toBe(true);
|
||||
expect(
|
||||
isToolWrappedWithBeforeToolCallHook(
|
||||
unwrappedTools.find((tool) => tool.name === "sessions_list")!,
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("registers update_plan when explicitly enabled", () => {
|
||||
const config = {
|
||||
tools: {
|
||||
|
||||
@@ -746,6 +746,7 @@ export function createOpenClawCodingTools(options?: {
|
||||
disableMessageTool: options?.disableMessageTool,
|
||||
enableHeartbeatTool,
|
||||
disablePluginTools: !includePluginTools,
|
||||
wrapBeforeToolCallHook: false,
|
||||
...(cronSelfRemoveOnlyJobId ? { cronSelfRemoveOnlyJobId } : {}),
|
||||
requesterAgentIdOverride: agentId,
|
||||
requesterSenderId: options?.senderId,
|
||||
|
||||
@@ -687,6 +687,112 @@ describe("handleInlineActions", () => {
|
||||
senderIsOwner: true,
|
||||
}),
|
||||
);
|
||||
expect(toolExecute).toHaveBeenCalled();
|
||||
expect(toolExecute).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^cmd_/),
|
||||
{
|
||||
command: "display name",
|
||||
commandName: "set_profile",
|
||||
skillName: "matrix-profile",
|
||||
},
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("honors construction-time before-tool-call blocks for inline tool dispatch", async () => {
|
||||
const typing = createTypingController();
|
||||
const abortController = new AbortController();
|
||||
const toolExecute = vi.fn(async () => ({
|
||||
content: [{ type: "text", text: "denied by policy" }],
|
||||
details: {
|
||||
status: "blocked",
|
||||
deniedReason: "plugin-before-tool-call",
|
||||
reason: "denied by policy",
|
||||
},
|
||||
}));
|
||||
createOpenClawToolsMock.mockReturnValue([
|
||||
{
|
||||
name: "message",
|
||||
execute: toolExecute,
|
||||
},
|
||||
]);
|
||||
|
||||
const ctx = buildTestCtx({
|
||||
Body: "/set_profile display name",
|
||||
CommandBody: "/set_profile display name",
|
||||
});
|
||||
const skillCommands: SkillCommandSpec[] = [
|
||||
{
|
||||
name: "set_profile",
|
||||
skillName: "matrix-profile",
|
||||
description: "Set Matrix profile",
|
||||
dispatch: {
|
||||
kind: "tool",
|
||||
toolName: "message",
|
||||
argMode: "raw",
|
||||
},
|
||||
sourceFilePath: "/tmp/plugin/commands/set-profile.md",
|
||||
},
|
||||
];
|
||||
|
||||
const result = await handleInlineActions(
|
||||
createHandleInlineActionsInput({
|
||||
ctx,
|
||||
typing,
|
||||
cleanedBody: "/set_profile display name",
|
||||
command: {
|
||||
isAuthorizedSender: true,
|
||||
senderId: "sender-1",
|
||||
senderIsOwner: true,
|
||||
abortKey: "sender-1",
|
||||
rawBodyNormalized: "/set_profile display name",
|
||||
commandBodyNormalized: "/set_profile display name",
|
||||
},
|
||||
overrides: {
|
||||
cfg: {
|
||||
commands: { text: true },
|
||||
tools: {
|
||||
loopDetection: {
|
||||
enabled: true,
|
||||
},
|
||||
},
|
||||
},
|
||||
agentId: "main",
|
||||
allowTextCommands: true,
|
||||
opts: { abortSignal: abortController.signal },
|
||||
skillCommands,
|
||||
sessionEntry: {
|
||||
sessionId: "wrapper-session",
|
||||
updatedAt: 0,
|
||||
},
|
||||
sessionStore: {
|
||||
"s:main": {
|
||||
sessionId: "target-session",
|
||||
updatedAt: 0,
|
||||
},
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
expect(result).toEqual({
|
||||
kind: "reply",
|
||||
reply: { text: "❌ Tool call blocked: denied by policy" },
|
||||
});
|
||||
expect(createOpenClawToolsMock).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
sessionId: "target-session",
|
||||
currentChannelId: "whatsapp",
|
||||
}),
|
||||
);
|
||||
expect(toolExecute).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^cmd_/),
|
||||
{
|
||||
command: "display name",
|
||||
commandName: "set_profile",
|
||||
skillName: "matrix-profile",
|
||||
},
|
||||
abortController.signal,
|
||||
);
|
||||
expect(typing.cleanup).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -144,6 +144,22 @@ function extractTextFromToolResult(result: unknown): string | null {
|
||||
return trimmed ? trimmed : null;
|
||||
}
|
||||
|
||||
function extractBlockedToolReason(result: unknown): string | null {
|
||||
if (!result || typeof result !== "object") {
|
||||
return null;
|
||||
}
|
||||
const details = (result as { details?: unknown }).details;
|
||||
if (!details || typeof details !== "object") {
|
||||
return null;
|
||||
}
|
||||
const status = (details as { status?: unknown }).status;
|
||||
if (status !== "blocked") {
|
||||
return null;
|
||||
}
|
||||
const reason = (details as { reason?: unknown }).reason;
|
||||
return typeof reason === "string" && reason.trim() ? reason.trim() : null;
|
||||
}
|
||||
|
||||
export async function handleInlineActions(params: {
|
||||
ctx: MsgContext;
|
||||
sessionCtx: TemplateContext;
|
||||
@@ -246,6 +262,7 @@ export async function handleInlineActions(params: {
|
||||
skillFilter,
|
||||
})
|
||||
: [];
|
||||
const targetSessionEntry = sessionStore?.[sessionKey] ?? sessionEntry;
|
||||
|
||||
const skillInvocation =
|
||||
allowTextCommands && skillCommands.length > 0
|
||||
@@ -285,6 +302,8 @@ export async function handleInlineActions(params: {
|
||||
config: cfg,
|
||||
allowGatewaySubagentBinding: true,
|
||||
senderIsOwner: command.senderIsOwner,
|
||||
sessionId: targetSessionEntry?.sessionId,
|
||||
currentChannelId: command.channelId,
|
||||
});
|
||||
const authorizedTools = applyOwnerOnlyToolPolicy(tools, command.senderIsOwner);
|
||||
|
||||
@@ -301,7 +320,15 @@ export async function handleInlineActions(params: {
|
||||
commandName: skillInvocation.command.name,
|
||||
skillName: skillInvocation.command.skillName,
|
||||
};
|
||||
const result = await tool.execute(toolCallId, toolArgs);
|
||||
const result = await tool.execute(toolCallId, toolArgs, opts?.abortSignal);
|
||||
const blockedReason = extractBlockedToolReason(result);
|
||||
if (blockedReason) {
|
||||
typing.cleanup();
|
||||
return {
|
||||
kind: "reply",
|
||||
reply: { text: `❌ Tool call blocked: ${blockedReason}` },
|
||||
};
|
||||
}
|
||||
const text = extractTextFromToolResult(result) ?? "✅ Done.";
|
||||
typing.cleanup();
|
||||
return { kind: "reply", reply: { text } };
|
||||
@@ -342,7 +369,6 @@ export async function handleInlineActions(params: {
|
||||
};
|
||||
|
||||
const isStopLikeInbound = isAbortRequestText(command.rawBodyNormalized);
|
||||
const targetSessionEntry = sessionStore?.[sessionKey] ?? sessionEntry;
|
||||
if (!isStopLikeInbound && targetSessionEntry) {
|
||||
const cutoff = readAbortCutoffFromSessionEntry(targetSessionEntry);
|
||||
const incoming = resolveAbortCutoffFromContext(ctx);
|
||||
|
||||
@@ -95,6 +95,7 @@ export function resolveGatewayScopedTools(params: {
|
||||
allowGatewaySubagentBinding: params.allowGatewaySubagentBinding,
|
||||
allowMediaInvokeCommands: params.allowMediaInvokeCommands,
|
||||
disablePluginTools: params.disablePluginTools,
|
||||
wrapBeforeToolCallHook: false,
|
||||
senderIsOwner: params.senderIsOwner,
|
||||
config: params.cfg,
|
||||
workspaceDir,
|
||||
|
||||
Reference in New Issue
Block a user