diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b32e0f5cf0..4c6bc0b7e4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ Docs: https://docs.openclaw.ai - Agents/workspace: respect `agents.defaults.workspace` for non-default agents by resolving them under the configured base path instead of falling back to `workspace-`. (#59858) Thanks @joelnishanth. - Config/All Settings: keep the raw config view intact when sensitive fields are blank instead of corrupting or dropping the snapshot during redaction. (#28214) thanks @solodmd. - Plugins/runtime: honor explicit capability allowlists during fallback speech, media-understanding, and image-generation provider loading so bundled capability plugins do not bypass restrictive `plugins.allow` config. (#52262) Thanks @PerfectPan. +- Hooks/tool policy: block tool calls when a `before_tool_call` hook crashes so hook failures fail closed instead of silently allowing execution. (#59822) Thanks @pgondhi987. ## 2026.4.2 diff --git a/src/agents/pi-tools.before-tool-call.e2e.test.ts b/src/agents/pi-tools.before-tool-call.e2e.test.ts index b06b4f22c84..1e922ee02be 100644 --- a/src/agents/pi-tools.before-tool-call.e2e.test.ts +++ b/src/agents/pi-tools.before-tool-call.e2e.test.ts @@ -390,6 +390,22 @@ describe("before_tool_call requireApproval handling", () => { expect(mockCallGateway).not.toHaveBeenCalled(); }); + it("blocks when before_tool_call hook execution throws", async () => { + hookRunner.runBeforeToolCall.mockRejectedValueOnce(new Error("hook crashed")); + + const result = await runBeforeToolCallHook({ + toolName: "bash", + params: { command: "ls" }, + ctx: { agentId: "main", sessionKey: "main" }, + }); + + expect(result.blocked).toBe(true); + expect(result).toHaveProperty( + "reason", + "Tool call blocked because before_tool_call hook failed", + ); + }); + it("calls gateway RPC and unblocks on allow-once", async () => { hookRunner.runBeforeToolCall.mockResolvedValue({ requireApproval: { diff --git a/src/agents/pi-tools.before-tool-call.integration.e2e.test.ts b/src/agents/pi-tools.before-tool-call.integration.e2e.test.ts index e3b10844c70..1430031d7e2 100644 --- a/src/agents/pi-tools.before-tool-call.integration.e2e.test.ts +++ b/src/agents/pi-tools.before-tool-call.integration.e2e.test.ts @@ -167,7 +167,7 @@ describe("before_tool_call hook integration", () => { expect(execute).not.toHaveBeenCalled(); }); - it("continues execution when hook throws", async () => { + it("blocks tool execution when hook throws", async () => { beforeToolCallHook = installBeforeToolCallHook({ runBeforeToolCallImpl: async () => { throw new Error("boom"); @@ -178,14 +178,10 @@ describe("before_tool_call hook integration", () => { const tool = wrapToolWithBeforeToolCallHook({ name: "read", execute } as any); const extensionContext = {} as Parameters[3]; - await tool.execute("call-4", { path: "/tmp/file" }, undefined, extensionContext); - - expect(execute).toHaveBeenCalledWith( - "call-4", - { path: "/tmp/file" }, - undefined, - extensionContext, - ); + await expect( + tool.execute("call-4", { path: "/tmp/file" }, undefined, extensionContext), + ).rejects.toThrow("Tool call blocked because before_tool_call hook failed"); + expect(execute).not.toHaveBeenCalled(); }); it("normalizes non-object params for hook contract", async () => { diff --git a/src/agents/pi-tools.before-tool-call.ts b/src/agents/pi-tools.before-tool-call.ts index 17b5cf414d4..641fca01709 100644 --- a/src/agents/pi-tools.before-tool-call.ts +++ b/src/agents/pi-tools.before-tool-call.ts @@ -24,6 +24,8 @@ type HookOutcome = { blocked: true; reason: string } | { blocked: false; params: const log = createSubsystemLogger("agents/tools"); const BEFORE_TOOL_CALL_WRAPPED = Symbol("beforeToolCallWrapped"); +const BEFORE_TOOL_CALL_HOOK_FAILURE_REASON = + "Tool call blocked because before_tool_call hook failed"; const adjustedParamsByToolCallId = new Map(); const MAX_TRACKED_ADJUSTED_PARAMS = 1024; const LOOP_WARNING_BUCKET_SIZE = 10; @@ -67,6 +69,13 @@ function isAbortSignalCancellation(err: unknown, signal?: AbortSignal): boolean return false; } +function unwrapErrorCause(err: unknown): unknown { + if (err instanceof Error && err.cause !== undefined) { + return err.cause; + } + return err; +} + function shouldEmitLoopWarning(state: SessionState, warningKey: string, count: number): boolean { if (!state.toolLoopWarningBuckets) { state.toolLoopWarningBuckets = new Map(); @@ -357,7 +366,12 @@ export async function runBeforeToolCallHook(args: { } } catch (err) { const toolCallId = args.toolCallId ? ` toolCallId=${args.toolCallId}` : ""; - log.warn(`before_tool_call hook failed: tool=${toolName}${toolCallId} error=${String(err)}`); + const cause = unwrapErrorCause(err); + log.error(`before_tool_call hook failed: tool=${toolName}${toolCallId} error=${String(cause)}`); + return { + blocked: true, + reason: BEFORE_TOOL_CALL_HOOK_FAILURE_REASON, + }; } return { blocked: false, params }; diff --git a/src/plugins/hook-runner-global.ts b/src/plugins/hook-runner-global.ts index cef104b34e5..99f1c8f7f2c 100644 --- a/src/plugins/hook-runner-global.ts +++ b/src/plugins/hook-runner-global.ts @@ -40,6 +40,9 @@ export function initializeGlobalHookRunner(registry: PluginRegistry): void { error: (msg) => log.error(msg), }, catchErrors: true, + failurePolicyByHook: { + before_tool_call: "fail-closed", + }, }); const hookCount = registry.hooks.length; diff --git a/src/plugins/hooks.security.test.ts b/src/plugins/hooks.security.test.ts index a923a01a340..878be98e736 100644 --- a/src/plugins/hooks.security.test.ts +++ b/src/plugins/hooks.security.test.ts @@ -163,6 +163,32 @@ describe("before_tool_call terminal block semantics", () => { expect(result?.block).toBe(true); expect(low).not.toHaveBeenCalled(); }); + + it("throws for before_tool_call when configured as fail-closed", async () => { + addStaticTestHooks(registry, { + hookName: "before_tool_call", + hooks: [ + { + pluginId: "failing", + result: {}, + priority: 100, + handler: () => { + throw new Error("boom"); + }, + }, + ], + }); + const runner = createHookRunner(registry, { + catchErrors: true, + failurePolicyByHook: { + before_tool_call: "fail-closed", + }, + }); + + await expect(runner.runBeforeToolCall(toolEvent, toolCtx)).rejects.toThrow( + "before_tool_call handler from failing failed: Error: boom", + ); + }); }); describe("message_sending terminal cancel semantics", () => { diff --git a/src/plugins/hooks.ts b/src/plugins/hooks.ts index b3b5e37461d..befed67b8ec 100644 --- a/src/plugins/hooks.ts +++ b/src/plugins/hooks.ts @@ -124,10 +124,17 @@ export type HookRunnerLogger = { error: (message: string) => void; }; +export type HookFailurePolicy = "fail-open" | "fail-closed"; + export type HookRunnerOptions = { logger?: HookRunnerLogger; /** If true, errors in hooks will be caught and logged instead of thrown */ catchErrors?: boolean; + /** + * Optional per-hook failure policy. + * Defaults to fail-open unless explicitly overridden for a hook name. + */ + failurePolicyByHook?: Partial>; }; type ModifyingHookPolicy = { @@ -186,6 +193,10 @@ function getHooksForNameAndPlugin( export function createHookRunner(registry: PluginRegistry, options: HookRunnerOptions = {}) { const logger = options.logger; const catchErrors = options.catchErrors ?? true; + const failurePolicyByHook = options.failurePolicyByHook ?? {}; + + const shouldCatchHookErrors = (hookName: PluginHookName): boolean => + catchErrors && (failurePolicyByHook[hookName] ?? "fail-open") === "fail-open"; const firstDefined = (prev: T | undefined, next: T | undefined): T | undefined => prev ?? next; const lastDefined = (prev: T | undefined, next: T | undefined): T | undefined => next ?? prev; @@ -255,7 +266,7 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp const msg = `[hooks] ${params.hookName} handler from ${params.pluginId} failed: ${String( params.error, )}`; - if (catchErrors) { + if (shouldCatchHookErrors(params.hookName)) { logger?.error(msg); return; } @@ -797,7 +808,7 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp const msg = `[hooks] tool_result_persist handler from ${hook.pluginId} returned a Promise; ` + `this hook is synchronous and the result was ignored.`; - if (catchErrors) { + if (shouldCatchHookErrors("tool_result_persist")) { logger?.warn?.(msg); continue; } @@ -810,7 +821,7 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp } } catch (err) { const msg = `[hooks] tool_result_persist handler from ${hook.pluginId} failed: ${String(err)}`; - if (catchErrors) { + if (shouldCatchHookErrors("tool_result_persist")) { logger?.error(msg); } else { throw new Error(msg, { cause: err }); @@ -862,7 +873,7 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp const msg = `[hooks] before_message_write handler from ${hook.pluginId} returned a Promise; ` + `this hook is synchronous and the result was ignored.`; - if (catchErrors) { + if (shouldCatchHookErrors("before_message_write")) { logger?.warn?.(msg); continue; } @@ -882,7 +893,7 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp } } catch (err) { const msg = `[hooks] before_message_write handler from ${hook.pluginId} failed: ${String(err)}`; - if (catchErrors) { + if (shouldCatchHookErrors("before_message_write")) { logger?.error(msg); } else { throw new Error(msg, { cause: err });