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

* 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 <drobison@nvidia.com>
This commit is contained in:
pgondhi987
2026-04-04 04:14:35 +05:30
committed by GitHub
parent 1322aa2ba2
commit e19dce0aed
7 changed files with 82 additions and 15 deletions

View File

@@ -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: {

View File

@@ -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 () => {

View File

@@ -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 };