fix(hooks): harden before_tool_call hook runner to fail-closed on error [AI] (#59822)

pgondhi987 · drobison00 · web-flow · commit e19dce0aedbc · 2026-04-03T16:44:35.000-06:00
* fix: address issue

* fix: address PR review feedback

* docs: add changelog entry for PR merge

* docs: normalize changelog entry placement

---------

Co-authored-by: Devin Robison &lt;drobison@nvidia.com&gt;
diff --git 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-<id>`. (#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
@@ -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
@@ -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<typeof tool.execute>[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
@@ -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<string, unknown>();
 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
@@ -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
@@ -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
@@ -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<Record<PluginHookName, HookFailurePolicy>>;
 };
 
 type ModifyingHookPolicy<K extends PluginHookName, TResult> = {
@@ -186,6 +193,10 @@ function getHooksForNameAndPlugin<K extends PluginHookName>(
 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 = <T>(prev: T | undefined, next: T | undefined): T | undefined => prev ?? next;
   const lastDefined = <T>(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 });
