From b6cd7ced18869e141caffd38429547a562fa0cf9 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:58:54 -0500 Subject: [PATCH 01/14] fix(agent): enforce approval gate on allowFinalResponse path and validate predicate args (#54) Two ways the tool-approval gate could be bypassed. The allowFinalResponse path executed pending tool calls with no approval check. When a stopWhen condition halted the loop on a turn that still carried tool calls, the final-response path called executeToolRound directly, skipping the gate the normal loop applies on every round. A tool marked requireApproval would execute unguarded, and since the PermissionRequest hook's deny bookkeeping lives inside handleApprovalCheck, hook-based deny never fired on this path either. Function-based requireApproval also received unvalidated arguments: the predicate got the raw JSON-parsed wire payload while execute receives the values after the tool's Zod inputSchema runs, so any default, coercion, or transform made the two disagree. The predicate now parses with the same schema the executor uses, and fails closed (requires approval) when the arguments don't satisfy it. Co-Authored-By: Claude Fable 5 --- .changeset/approval-gate-fixes.md | 9 + packages/agent/src/lib/conversation-state.ts | 35 +- packages/agent/src/lib/model-result.ts | 20 + .../unit/approval-gate-regressions.test.ts | 436 ++++++++++++++++++ 4 files changed, 490 insertions(+), 10 deletions(-) create mode 100644 .changeset/approval-gate-fixes.md create mode 100644 packages/agent/tests/unit/approval-gate-regressions.test.ts diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md new file mode 100644 index 00000000..d816da02 --- /dev/null +++ b/.changeset/approval-gate-fixes.md @@ -0,0 +1,9 @@ +--- +'@openrouter/agent': patch +--- + +Fix two ways the tool-approval gate could be bypassed. + +**`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. + +**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. If the arguments don't satisfy the schema the gate fails closed and requires approval, rather than judging a value `execute` would never see. diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index d42a4021..a4bbbf77 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -1,4 +1,10 @@ import type * as models from '@openrouter/sdk/models'; +// Same zod entry point the executor validates through (see +// `validateToolInput` in tool-executor.ts) so the approval predicate and +// `execute` agree on parse semantics. Imported directly rather than reusing +// that helper because tool-executor.ts imports this module — sharing it would +// create an import cycle. +import * as z4 from 'zod/v4'; import type { ConversationState, ParsedToolCall, @@ -294,18 +300,27 @@ export async function toolRequiresApproval( const requireApproval = tool.function.requireApproval; // If it's a function, call it with the tool's arguments and context. - // Arguments have already been parsed and validated against the tool's - // Zod inputSchema (a ZodObject), so the runtime shape is always a - // record here. A non-record value signals a real upstream bug — surface - // it rather than substituting an empty object. + // + // `toolCall.arguments` at this point is only the JSON-parsed wire payload + // (see extractToolCallsFromResponse) — it has NOT been validated against + // the tool's Zod inputSchema. The executor validates separately, right + // before calling `execute` (see validateToolInput in tool-executor.ts), so + // handing the raw payload to the predicate would let the two see different + // values whenever the schema applies a default, coercion, or transform + // (e.g. schema `{ dangerous: z.boolean().default(true) }` + model emits + // `{}`: the predicate sees `undefined` and waves the call through, then + // `execute` runs with `dangerous: true`). + // + // Parse with the same schema the executor uses so the predicate decides on + // exactly the values `execute` will receive. Fail CLOSED: if the arguments + // don't satisfy the schema there is no trustworthy value to judge, so + // require approval rather than guessing. if (typeof requireApproval === 'function') { - const rawArgs: unknown = toolCall.arguments; - if (!isRecord(rawArgs)) { - throw new Error( - `toolCall.arguments for "${toolCall.name}" must be an object after Zod validation, got ${rawArgs === null ? 'null' : Array.isArray(rawArgs) ? 'array' : typeof rawArgs}`, - ); + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (!parsed.success || !isRecord(parsed.data)) { + return true; } - return requireApproval(rawArgs, context); + return requireApproval(parsed.data, context); } // Otherwise treat as boolean diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index e8d07757..1b61506b 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -6132,6 +6132,26 @@ export class ModelResult< this.hasExecutableToolCalls(pendingToolCalls) ) { const turnNumber = currentRound + 1; + + // Gate these calls exactly like a normal round would. This path + // executes real tools, so it needs the same approval check as the + // in-loop call sites above — without it, `stopWhen` firing on a turn + // that carries a `requireApproval` call would run that call + // unguarded, and hook-based 'deny' would never fire either (the + // deny bookkeeping lives inside handleApprovalCheck). + // + // On pause, handleApprovalCheck persists `pendingToolCalls` + + // status 'awaiting_approval', executes any auto-approved calls as + // unsent results, and sets `finalResponse` — so returning here is + // safe: nothing executed, so there is no round to record, and we + // must NOT fall through to markStateComplete() or the final + // text-coercion request. `sessionEndReason` stays 'max_turns' — + // accurate (the loop did stop on the stop condition) and consistent + // with the HITL pause return further down this same block. + if (await this.handleApprovalCheck(pendingToolCalls, turnNumber, currentResponse)) { + return; + } + const turnContext: TurnContext = { numberOfTurns: turnNumber, }; diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts new file mode 100644 index 00000000..ec65dddb --- /dev/null +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -0,0 +1,436 @@ +import type { OpenRouterCore } from '@openrouter/sdk/core'; +import type * as models from '@openrouter/sdk/models'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod/v4'; +import { toolRequiresApproval } from '../../src/lib/conversation-state.js'; +import { stepCountIs } from '../../src/lib/stop-conditions.js'; +import { tool } from '../../src/lib/tool.js'; +import type { + ConversationState, + StateAccessor, + Tool, + TurnContext, +} from '../../src/lib/tool-types.js'; + +const mockBetaResponsesSend = vi.hoisted(() => vi.fn()); + +vi.mock('@openrouter/sdk/funcs/betaResponsesSend', () => ({ + betaResponsesSend: mockBetaResponsesSend, +})); + +// Import ModelResult AFTER vi.mock so the transport mock is wired in. +const { ModelResult } = await import('../../src/lib/model-result.js'); + +const context: TurnContext = { + numberOfTurns: 1, +}; + +function makeResponse( + id: string, + output: models.OpenResponsesResult['output'], +): models.OpenResponsesResult { + return { + id, + object: 'response', + createdAt: 0, + model: 'test-model', + status: 'completed', + completedAt: 0, + output, + error: null, + incompleteDetails: null, + temperature: null, + topP: null, + presencePenalty: null, + frequencyPenalty: null, + metadata: null, + instructions: null, + tools: [], + toolChoice: 'auto', + parallelToolCalls: false, + }; +} + +function makeFunctionCallItem( + callId: string, + name: string, + args: string, +): models.OutputFunctionCallItem { + return { + type: 'function_call', + id: `fc_${callId}`, + callId, + name, + arguments: args, + status: 'completed', + }; +} + +function createMemoryAccessor( + initial: ConversationState | null = null, +): { + accessor: StateAccessor; + get: () => ConversationState | null; +} { + let state = initial; + const accessor: StateAccessor = { + load: async () => state, + save: async (next) => { + state = next; + }, + }; + return { + accessor, + get: () => state, + }; +} + +// --------------------------------------------------------------------------- +// Bug 1: the approval predicate must see the SAME arguments `execute` gets — +// i.e. the values after the tool's Zod inputSchema applies defaults and +// coercions. Previously the predicate got the raw JSON.parse'd arguments, so a +// schema default could flip a dangerous flag on *after* the gate had already +// decided no approval was needed. +// --------------------------------------------------------------------------- +describe('approval predicate argument parity with execute (#54)', () => { + it('applies inputSchema defaults before invoking a function-based requireApproval', async () => { + const seenByPredicate: unknown[] = []; + + const conditional = tool({ + name: 'conditional_action', + inputSchema: z.object({ + dangerous: z.boolean().default(true), + }), + requireApproval: (params) => { + seenByPredicate.push(params); + return params.dangerous === true; + }, + execute: async () => ({}), + }); + + // The model emitted `{}` — `dangerous` is absent from the wire payload but + // the schema default makes it `true` by the time `execute` runs. + const emptyArgsCall = { + id: '1', + name: 'conditional_action', + arguments: {}, + }; + + const requires = await toolRequiresApproval( + emptyArgsCall, + [ + conditional, + ], + context, + ); + + expect(seenByPredicate).toEqual([ + { + dangerous: true, + }, + ]); + expect(requires).toBe(true); + }); + + it('applies inputSchema coercions before invoking a function-based requireApproval', async () => { + const seenByPredicate: unknown[] = []; + + const transfer = tool({ + name: 'transfer', + inputSchema: z.object({ + amount: z.coerce.number(), + }), + requireApproval: (params) => { + seenByPredicate.push(params); + return params.amount > 100; + }, + execute: async () => ({}), + }); + + // Model emitted the amount as a string; coercion turns it into a number, + // so the `> 100` comparison must be numeric, not lexicographic. + const stringAmountCall = { + id: '2', + name: 'transfer', + arguments: { + amount: '500', + }, + }; + + const requires = await toolRequiresApproval( + stringAmountCall, + [ + transfer, + ], + context, + ); + + expect(seenByPredicate).toEqual([ + { + amount: 500, + }, + ]); + expect(requires).toBe(true); + }); + + it('fails closed (requires approval) when arguments do not satisfy the inputSchema', async () => { + const predicate = vi.fn(() => false); + + const strict = tool({ + name: 'strict_action', + inputSchema: z.object({ + target: z.string(), + }), + requireApproval: predicate, + execute: async () => ({}), + }); + + // `target` is missing entirely — the schema parse fails. Rather than + // handing the predicate a value `execute` would never see, the gate must + // fail closed and require approval. + const invalidCall = { + id: '3', + name: 'strict_action', + arguments: {}, + }; + + const requires = await toolRequiresApproval( + invalidCall, + [ + strict, + ], + context, + ); + + expect(requires).toBe(true); + expect(predicate).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// Bug 2: when `stopWhen` halts the loop on a turn that still has pending tool +// calls and `allowFinalResponse` is enabled, the pending calls were executed +// with no approval gate at all. +// --------------------------------------------------------------------------- +describe('allowFinalResponse path enforces the approval gate (#54)', () => { + beforeEach(() => { + mockBetaResponsesSend.mockReset(); + }); + + it('does not execute a requireApproval tool when stopWhen halts the loop', async () => { + const safeExecute = vi.fn(async () => ({ + ok: true, + })); + const dangerExecute = vi.fn(async () => ({ + ok: true, + })); + + const safe = tool({ + name: 'safe', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + execute: safeExecute, + }); + + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute: dangerExecute, + }); + + const tools = [ + safe, + danger, + ] as const; + + // Turn 0 calls the ungated tool so one round completes and the stop + // condition (`stepCountIs(1)`) only fires on the NEXT iteration — which + // breaks the loop with `stoppedByStopWhen`, reaching the post-loop + // allowFinalResponse path. The follow-up response carries the gated call. + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_0', [ + makeFunctionCallItem( + 'call_safe', + 'safe', + JSON.stringify({ + target: 'staging', + }), + ), + ]), + }) + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_1', [ + makeFunctionCallItem( + 'call_danger', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor, get } = createMemoryAccessor(); + + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the thing', + tools: [ + { + type: 'function', + name: 'safe', + description: null, + strict: null, + parameters: {}, + }, + { + type: 'function', + name: 'danger', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + state: accessor, + stopWhen: stepCountIs(1), + allowFinalResponse: true, + }); + + await result.getResponse(); + + // The ungated tool ran in the normal loop round. + expect(safeExecute).toHaveBeenCalledTimes(1); + + // The gate must hold on the allowFinalResponse path too: the gated tool + // never runs without approval. + expect(dangerExecute).not.toHaveBeenCalled(); + + // And the run pauses the same way the in-loop gate does. + const saved = get(); + expect(saved?.status).toBe('awaiting_approval'); + expect(saved?.pendingToolCalls).toHaveLength(1); + expect(saved?.pendingToolCalls?.[0]?.id).toBe('call_danger'); + expect(await result.requiresApproval()).toBe(true); + + // No final text-coercion request is made while paused: only the initial + // request and the one follow-up went out. + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(2); + }); + + it('still runs non-gated tools on the allowFinalResponse path', async () => { + const execute = vi.fn(async () => ({ + ok: true, + })); + + const safe = tool({ + name: 'safe', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + execute, + }); + + const tools = [ + safe, + ] as const; + + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_0', [ + makeFunctionCallItem( + 'call_safe', + 'safe', + JSON.stringify({ + target: 'staging', + }), + ), + ]), + }) + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor } = createMemoryAccessor(); + + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the safe thing', + tools: [ + { + type: 'function', + name: 'safe', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + state: accessor, + stopWhen: stepCountIs(0), + allowFinalResponse: true, + }); + + const text = await result.getText(); + + expect(execute).toHaveBeenCalledTimes(1); + expect(text).toBe('Done.'); + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(2); + }); +}); From 3ae034ea89e5f591d502993a08fee76a862e5acd Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:29:53 -0500 Subject: [PATCH 02/14] test(agent): cover hook deny on allowFinalResponse path Adds a regression test for the PermissionRequest hook returning 'deny' on the post-loop allowFinalResponse path: the denied tool must not execute, the run must not pause for a human, and the hook's reason must be recorded in state as a synthesized rejected output for the call. Verified load-bearing: with the approval-gate fix in model-result.ts reverted, the hook handler is never invoked (0 calls) because that path had no approval check at all, which is where hookDeniedCalls is populated. Co-Authored-By: Claude Fable 5 --- .../unit/approval-gate-regressions.test.ts | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index ec65dddb..b1fa3016 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -3,6 +3,7 @@ import type * as models from '@openrouter/sdk/models'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { z } from 'zod/v4'; import { toolRequiresApproval } from '../../src/lib/conversation-state.js'; +import { HooksManager } from '../../src/lib/hooks-manager.js'; import { stepCountIs } from '../../src/lib/stop-conditions.js'; import { tool } from '../../src/lib/tool.js'; import type { @@ -433,4 +434,161 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { expect(text).toBe('Done.'); expect(mockBetaResponsesSend).toHaveBeenCalledTimes(2); }); + + it('honors a PermissionRequest hook returning deny on the allowFinalResponse path', async () => { + const safeExecute = vi.fn(async () => ({ + ok: true, + })); + const dangerExecute = vi.fn(async () => ({ + ok: true, + })); + + const safe = tool({ + name: 'safe', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + execute: safeExecute, + }); + + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute: dangerExecute, + }); + + const tools = [ + safe, + danger, + ] as const; + + // The hook denies the gated call outright, so the run does NOT pause for a + // human: handleApprovalCheck records the denial and returns false, and the + // round synthesizes a rejection instead of executing. + const hooks = new HooksManager(); + const permissionHandler = vi.fn(() => ({ + decision: 'deny' as const, + reason: 'blocked by policy', + })); + hooks.on('PermissionRequest', { + handler: permissionHandler, + }); + + // Same fixture shape as the gate test above: turn 0 calls the ungated tool + // so one round completes and the stop condition fires on the NEXT + // iteration, reaching the post-loop allowFinalResponse path with the gated + // call pending. + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_0', [ + makeFunctionCallItem( + 'call_safe', + 'safe', + JSON.stringify({ + target: 'staging', + }), + ), + ]), + }) + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_1', [ + makeFunctionCallItem( + 'call_danger', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Denied.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor, get } = createMemoryAccessor(); + + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the thing', + tools: [ + { + type: 'function', + name: 'safe', + description: null, + strict: null, + parameters: {}, + }, + { + type: 'function', + name: 'danger', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + stopWhen: stepCountIs(1), + allowFinalResponse: true, + }); + + await result.getResponse(); + + // The hook was consulted for the gated call — before the fix this path had + // no approval check at all, so it never fired here. + expect(permissionHandler).toHaveBeenCalledTimes(1); + + // The denied tool must not execute; the ungated one still runs normally. + expect(dangerExecute).not.toHaveBeenCalled(); + expect(safeExecute).toHaveBeenCalledTimes(1); + + // A hook deny resolves the gate without a human, so the run does not pause + // for approval. + const saved = get(); + expect(saved?.status).not.toBe('awaiting_approval'); + + // The rejection is recorded in state as a synthesized output for the denied + // call, carrying the hook's reason. + const outputs = (saved?.messages ?? []).filter( + (m): m is models.FunctionCallOutputItem => + typeof m === 'object' && + m !== null && + 'type' in m && + m.type === 'function_call_output' && + 'callId' in m && + m.callId === 'call_danger', + ); + expect(outputs).toHaveLength(1); + expect(JSON.stringify(outputs[0]?.output)).toContain('blocked by policy'); + }); }); From 639e7bbc7d7b9af814372f355f2936ac2cf2f750 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Mon, 10 Aug 2026 13:17:16 -0500 Subject: [PATCH 03/14] fix(agent): gate each response once and let schema-invalid calls reach executor validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Devin review on #94: - The new allowFinalResponse gate could re-check calls the pre-loop gate already resolved when a stop condition fired on the first loop iteration (same response object), re-emitting PermissionRequest hooks and re-running requireApproval predicates. handleApprovalCheck now gates each response at most once per run. - Failing closed on schema-invalid arguments converted a recoverable model error into a pause (or a hard throw without a state accessor) for a call that can never execute — the executor validates with the same schema and turns the failure into a tool error output. The gate now lets schema-invalid calls fall through to that validation error; fail-closed is kept only for parses that succeed with a non-record payload, which would break the predicate contract. --- .changeset/approval-gate-fixes.md | 4 +- packages/agent/src/lib/conversation-state.ts | 19 +- packages/agent/src/lib/model-result.ts | 19 ++ .../unit/approval-gate-regressions.test.ts | 264 +++++++++++++++++- 4 files changed, 297 insertions(+), 9 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index d816da02..4d4a4283 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -6,4 +6,6 @@ Fix two ways the tool-approval gate could be bypassed. **`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. -**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. If the arguments don't satisfy the schema the gate fails closed and requires approval, rather than judging a value `execute` would never see. +**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. Arguments that don't satisfy the schema are not gated at all: such a call can never execute (the executor validates with the same schema and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. + +**Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index a4bbbf77..8814782f 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -312,12 +312,23 @@ export async function toolRequiresApproval( // `execute` runs with `dangerous: true`). // // Parse with the same schema the executor uses so the predicate decides on - // exactly the values `execute` will receive. Fail CLOSED: if the arguments - // don't satisfy the schema there is no trustworthy value to judge, so - // require approval rather than guessing. + // exactly the values `execute` will receive. if (typeof requireApproval === 'function') { const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); - if (!parsed.success || !isRecord(parsed.data)) { + if (!parsed.success) { + // Arguments that don't satisfy the schema can never execute: every + // execute path runs the same schema through validateToolInput first + // and converts the failure into a tool error output the model can + // recover from. Gating them would pause the run so a human can approve + // a call that can only fail (or throw outright when no state accessor + // is configured), so let them through the gate to the executor's + // normal validation error. + return false; + } + if (!isRecord(parsed.data)) { + // Valid per the schema but not an object — the predicate contract is + // Record, so there is no trustworthy value to judge. + // Fail closed. return true; } return requireApproval(parsed.data, context); diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 1b61506b..f60b7944 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -645,6 +645,14 @@ export class ModelResult< // normal tool round consults this to synthesize rejected outputs instead of // executing the calls. private readonly hookDeniedCalls = new Map(); + // The response most recently passed through handleApprovalCheck on this + // run. The same response object can reach the gate more than once — the + // pre-loop check plus the first loop iteration, or the pre-loop check plus + // the post-loop allowFinalResponse gate when a stop condition fires before + // any follow-up request. Re-gating would re-emit PermissionRequest hooks + // (duplicate prompts/audit records) and re-run requireApproval predicates + // for calls already resolved, so the gate runs at most once per response. + private lastApprovalGatedResponse: models.OpenResponsesResult | null = null; // Telemetry for the PostModelCall hook: the initial/resume request is // dispatched in initStream but its response is materialized later (stream // consumption), so the dispatch time and turn labeling are parked here @@ -2840,6 +2848,17 @@ export class ModelResult< return false; } + // Each response is gated at most once per run (see the field doc). A + // repeat visit for the same response object means the calls were already + // partitioned, the hooks already fired, and any hook 'deny' results are + // already recorded in hookDeniedCalls — and it did not pause (a pause + // returns out of the run). Skipping is therefore both safe and required + // to avoid duplicate permission prompts. + if (currentResponse === this.lastApprovalGatedResponse) { + return false; + } + this.lastApprovalGatedResponse = currentResponse; + const turnContext: TurnContext = { numberOfTurns: currentRound, // context is handled via contextStore, not on TurnContext diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index b1fa3016..a78c03e6 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -174,7 +174,7 @@ describe('approval predicate argument parity with execute (#54)', () => { expect(requires).toBe(true); }); - it('fails closed (requires approval) when arguments do not satisfy the inputSchema', async () => { + it('does not gate calls whose arguments fail schema validation', async () => { const predicate = vi.fn(() => false); const strict = tool({ @@ -186,9 +186,13 @@ describe('approval predicate argument parity with execute (#54)', () => { execute: async () => ({}), }); - // `target` is missing entirely — the schema parse fails. Rather than - // handing the predicate a value `execute` would never see, the gate must - // fail closed and require approval. + // `target` is missing entirely — the schema parse fails. Such a call can + // never execute (the executor runs the same schema through + // validateToolInput and turns the failure into a tool error output), so + // gating it would pause the run for a human to approve a call that can + // only fail — or throw outright when no state accessor is configured. + // The gate lets it through to the executor's normal validation error and + // never invokes the predicate on a value `execute` would not see. const invalidCall = { id: '3', name: 'strict_action', @@ -203,6 +207,54 @@ describe('approval predicate argument parity with execute (#54)', () => { context, ); + expect(requires).toBe(false); + expect(predicate).not.toHaveBeenCalled(); + }); + + it('still fails closed when the schema parses to a non-object value', async () => { + const predicate = vi.fn(() => false); + + // inputSchema is typed $ZodObject, but nothing enforces that at runtime — + // a schema whose parse succeeds with a non-record payload would break the + // predicate's Record contract, so the gate fails closed. + const weird = tool({ + name: 'weird_action', + inputSchema: z.object({ + target: z.string(), + }), + requireApproval: predicate, + execute: async () => ({}), + }); + ( + weird.function as { + inputSchema: unknown; + } + ).inputSchema = z + .object({ + target: z.string(), + }) + .transform(() => [ + 'not', + 'a', + 'record', + ]); + + const call = { + id: '4', + name: 'weird_action', + arguments: { + target: 'x', + }, + }; + + const requires = await toolRequiresApproval( + call, + [ + weird, + ], + context, + ); + expect(requires).toBe(true); expect(predicate).not.toHaveBeenCalled(); }); @@ -591,4 +643,208 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { expect(outputs).toHaveLength(1); expect(JSON.stringify(outputs[0]?.output)).toContain('blocked by policy'); }); + + it('surfaces schema-invalid arguments as a tool error instead of pausing or throwing', async () => { + const execute = vi.fn(async () => ({ + ok: true, + })); + + const strict = tool({ + name: 'strict', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: (params) => params.target.length > 3, + execute, + }); + + const tools = [ + strict, + ] as const; + + // The model emits `{}` — `target` is missing, so the arguments can never + // satisfy the schema. The gate must let the call through to the executor, + // which turns the validation failure into a tool error output the model + // can recover from. Gating it instead would pause the run for a human to + // approve a call that can only fail — and with no state accessor + // configured (as here), handleApprovalCheck would throw outright. + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_0', [ + makeFunctionCallItem('call_strict', 'strict', '{}'), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Recovered.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor, get } = createMemoryAccessor(); + + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the strict thing', + tools: [ + { + type: 'function', + name: 'strict', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + state: accessor, + }); + + const text = await result.getText(); + + // No throw, no pause: the run completes normally. + expect(text).toBe('Recovered.'); + const saved = get(); + expect(saved?.status).toBe('complete'); + + // The tool body never ran — validation failed first — and the failure was + // recorded as the call's output so the model could see it and recover. + expect(execute).not.toHaveBeenCalled(); + const outputs = (saved?.messages ?? []).filter( + (m): m is models.FunctionCallOutputItem => + typeof m === 'object' && + m !== null && + 'type' in m && + m.type === 'function_call_output' && + 'callId' in m && + m.callId === 'call_strict', + ); + expect(outputs).toHaveLength(1); + expect(JSON.stringify(outputs[0]?.output)).toContain('target'); + + // One round trip for the initial request, one for the follow-up carrying + // the validation error. + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(2); + }); + + it('gates the initial response only once when the stop condition fires on the first iteration', async () => { + const dangerExecute = vi.fn(async () => ({ + ok: true, + })); + + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute: dangerExecute, + }); + + const tools = [ + danger, + ] as const; + + // The hook promotes the gated call past the gate. + const hooks = new HooksManager(); + const permissionHandler = vi.fn(() => ({ + decision: 'allow' as const, + })); + hooks.on('PermissionRequest', { + handler: permissionHandler, + }); + + // The initial response carries the gated call and stepCountIs(0) stops + // the loop on the FIRST iteration, so the post-loop allowFinalResponse + // path re-extracts the exact calls the pre-loop gate already checked. + // Re-gating them would re-emit the PermissionRequest hook — a duplicate + // prompt/audit record for a single tool call. + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_turn_0', [ + makeFunctionCallItem( + 'call_danger', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor } = createMemoryAccessor(); + + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the thing', + tools: [ + { + type: 'function', + name: 'danger', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + stopWhen: stepCountIs(0), + allowFinalResponse: true, + }); + + const text = await result.getText(); + + // Exactly one PermissionRequest emit for the single gated call — not one + // per gate visit. + expect(permissionHandler).toHaveBeenCalledTimes(1); + + // The hook allowed the call, so it executes exactly once on the + // allowFinalResponse path and the final text is still produced. + expect(dangerExecute).toHaveBeenCalledTimes(1); + expect(text).toBe('Done.'); + }); }); From 0418b07adb2642e2a435c97ec7351ea251abe38a Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Mon, 10 Aug 2026 14:15:52 -0500 Subject: [PATCH 04/14] fix(agent): keep fail-closed on invalid args for manual tools Addresses Devin re-review on #94: the schema-invalid fall-through assumed invalid arguments can never execute, which holds only for engine-executed tools (regular/generator/HITL/unified run all validateToolInput first). Manual tools are surfaced via pendingToolCalls and executed by the host application with no engine-side validation, so a malformed call guarded by a function-based requireApproval would bypass the approval pause and the PermissionRequest hook entirely. The fail-open is now gated on isAutoResolvableTool; manual tools fail closed as before. --- .changeset/approval-gate-fixes.md | 2 +- packages/agent/src/lib/conversation-state.ts | 26 ++++++++----- .../unit/approval-gate-regressions.test.ts | 37 +++++++++++++++++++ 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index 4d4a4283..27f1de54 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -6,6 +6,6 @@ Fix two ways the tool-approval gate could be bypassed. **`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. -**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. Arguments that don't satisfy the schema are not gated at all: such a call can never execute (the executor validates with the same schema and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. +**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. Arguments that don't satisfy the schema are not gated for engine-executed tools: such a call can never run (every execute path validates with the same schema first and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. Manual tools — which the host application executes without any engine-side validation — still fail closed on schema-invalid arguments. **Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index 8814782f..4c965cbd 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -12,7 +12,7 @@ import type { TurnContext, UnsentToolResult, } from './tool-types.js'; -import { isClientTool } from './tool-types.js'; +import { isAutoResolvableTool, isClientTool } from './tool-types.js'; import { normalizeInputToArray } from './turn-context.js'; @@ -316,14 +316,22 @@ export async function toolRequiresApproval( if (typeof requireApproval === 'function') { const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); if (!parsed.success) { - // Arguments that don't satisfy the schema can never execute: every - // execute path runs the same schema through validateToolInput first - // and converts the failure into a tool error output the model can - // recover from. Gating them would pause the run so a human can approve - // a call that can only fail (or throw outright when no state accessor - // is configured), so let them through the gate to the executor's - // normal validation error. - return false; + if (isAutoResolvableTool(tool)) { + // Engine-executed tools validate before running: every execute path + // (regular, generator, HITL onToolCalled, unified run) runs the same + // schema through validateToolInput first and converts the failure + // into a tool error output the model can recover from. Arguments + // that don't satisfy the schema can never execute, so gating them + // would pause the run so a human can approve a call that can only + // fail (or throw outright when no state accessor is configured) — + // let them through the gate to the executor's validation error. + return false; + } + // Manual tools (no execute / onToolCalled / run) are surfaced to the + // host application via pendingToolCalls and executed WITHOUT any + // engine-side validation, so the "can never execute" argument does not + // hold — fail closed rather than wave a malformed call past the gate. + return true; } if (!isRecord(parsed.data)) { // Valid per the schema but not an object — the predicate contract is diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index a78c03e6..4ea4be88 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -258,6 +258,43 @@ describe('approval predicate argument parity with execute (#54)', () => { expect(requires).toBe(true); expect(predicate).not.toHaveBeenCalled(); }); + + it('still fails closed on schema-invalid arguments for manual (caller-executed) tools', async () => { + const predicate = vi.fn(() => false); + + // A manual tool: no execute / onToolCalled / run. The engine never + // validates or executes it — the call is surfaced on pendingToolCalls + // for the host application to run as-is — so the "invalid arguments can + // never execute" reasoning does not apply, and the gate must fail closed + // rather than wave a malformed call past the approval check. + const manualTool = { + type: 'function', + function: { + name: 'manual_action', + inputSchema: z.object({ + target: z.string(), + }), + requireApproval: predicate, + }, + } as const; + + const invalidCall = { + id: '5', + name: 'manual_action', + arguments: {}, + }; + + const requires = await toolRequiresApproval( + invalidCall, + [ + manualTool, + ], + context, + ); + + expect(requires).toBe(true); + expect(predicate).not.toHaveBeenCalled(); + }); }); // --------------------------------------------------------------------------- From 631edb2488b457eabf924ae13797da17540c0cac Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Mon, 10 Aug 2026 14:26:32 -0500 Subject: [PATCH 05/14] test(agent): lock in the validate-before-execute invariant behind the gate's fail-open Addresses Devin re-review on #94: the schema-invalid fail-open is sound only while every engine execute path re-validates via validateToolInput before running the tool body. Add one test per execute path (regular, generator, HITL onToolCalled, unified run) asserting an invalid-args call never reaches the tool body and yields an error result. Mutation-checked: stubbing out validateToolInput fails each path's test. --- .../unit/approval-gate-regressions.test.ts | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 4ea4be88..669179cc 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -6,6 +6,12 @@ import { toolRequiresApproval } from '../../src/lib/conversation-state.js'; import { HooksManager } from '../../src/lib/hooks-manager.js'; import { stepCountIs } from '../../src/lib/stop-conditions.js'; import { tool } from '../../src/lib/tool.js'; +import { + executeGeneratorTool, + executeHITLTool, + executeRegularTool, + prepareUnifiedInvocation, +} from '../../src/lib/tool-executor.js'; import type { ConversationState, StateAccessor, @@ -297,6 +303,125 @@ describe('approval predicate argument parity with execute (#54)', () => { }); }); +// --------------------------------------------------------------------------- +// The gate's fail-open for schema-invalid arguments is only sound because +// every engine execute path re-validates with the tool's inputSchema before +// running the tool body. Lock that invariant in: if a future execute path +// (or a refactor of an existing one) ever skips validateToolInput, these +// tests fail — and the approval gate's fail-open must be revisited. +// --------------------------------------------------------------------------- +describe('every engine execute path validates before running the tool body', () => { + const inputSchema = z.object({ + target: z.string(), + }); + // Missing the required `target` — fails inputSchema validation. + const invalidArguments = {}; + + it('regular execute tools', async () => { + const execute = vi.fn(async () => ({})); + const regular = tool({ + name: 'regular', + inputSchema, + execute, + }); + + const result = await executeRegularTool( + regular, + { + id: 'c1', + name: 'regular', + arguments: invalidArguments, + }, + context, + ); + + expect(execute).not.toHaveBeenCalled(); + expect(result.error).toBeDefined(); + }); + + it('generator tools', async () => { + const execute = vi.fn(async function* () { + yield { + progress: 1, + }; + return { + done: true, + }; + }); + const generator = tool({ + name: 'generator', + inputSchema, + eventSchema: z.object({ + progress: z.number(), + }), + outputSchema: z.object({ + done: z.boolean(), + }), + execute, + }); + + const result = await executeGeneratorTool( + generator, + { + id: 'c2', + name: 'generator', + arguments: invalidArguments, + }, + context, + ); + + expect(execute).not.toHaveBeenCalled(); + expect(result.error).toBeDefined(); + }); + + it('HITL onToolCalled tools', async () => { + const onToolCalled = vi.fn(async () => ({})); + const hitl = tool({ + name: 'hitl', + inputSchema, + outputSchema: z.object({ + ok: z.boolean(), + }), + onToolCalled, + }); + + const result = await executeHITLTool( + hitl, + { + id: 'c3', + name: 'hitl', + arguments: invalidArguments, + }, + context, + ); + + expect(onToolCalled).not.toHaveBeenCalled(); + expect(result?.error).toBeDefined(); + }); + + it('unified run tools', async () => { + const run = vi.fn(async () => ({})); + const unified = tool({ + name: 'unified', + inputSchema, + run, + }); + + const result = await prepareUnifiedInvocation( + unified, + { + id: 'c4', + name: 'unified', + arguments: invalidArguments, + }, + context, + ); + + expect(run).not.toHaveBeenCalled(); + expect('error' in result && result.error).toBeDefined(); + }); +}); + // --------------------------------------------------------------------------- // Bug 2: when `stopWhen` halts the loop on a turn that still has pending tool // calls and `allowFinalResponse` is enabled, the pending calls were executed From bc76be8a5a2d76c331b984dce193fcc96a98afd7 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:03:17 -0500 Subject: [PATCH 06/14] fix(agent): normalize call-level approval args --- .changeset/approval-gate-fixes.md | 2 +- packages/agent/src/lib/conversation-state.ts | 29 +++++-- .../unit/approval-gate-regressions.test.ts | 86 +++++++++++++++++++ 3 files changed, 110 insertions(+), 7 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index 27f1de54..75505c40 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -6,6 +6,6 @@ Fix two ways the tool-approval gate could be bypassed. **`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. -**Function-based `requireApproval` received unvalidated arguments.** The predicate was called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made the two disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed the predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. The predicate now sees the parsed value, so it decides on exactly what `execute` will receive. Arguments that don't satisfy the schema are not gated for engine-executed tools: such a call can never run (every execute path validates with the same schema first and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. Manual tools — which the host application executes without any engine-side validation — still fail closed on schema-invalid arguments. +**Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. Arguments that don't satisfy the schema are not gated for engine-executed tools: such a call can never run (every execute path validates with the same schema first and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. Manual tools — which the host application executes without any engine-side validation — still fail closed on schema-invalid arguments. **Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index 4c965cbd..5f42c38b 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -277,12 +277,6 @@ export async function toolRequiresApproval( context: TurnContext, ) => boolean | Promise, ): Promise { - // Call-level check takes precedence - if (callLevelCheck) { - return callLevelCheck(toolCall, context); - } - - // Fall back to tool-level setting (server tools never require approval) const tool = tools.find( ( t, @@ -293,6 +287,29 @@ export async function toolRequiresApproval( } > => isClientTool(t) && t.function.name === toolCall.name, ); + // Call-level checks take precedence. When the call maps to a client tool, + // give the callback a normalized copy while preserving the original wire + // arguments for execution to parse independently. + if (callLevelCheck) { + if (!tool) { + return callLevelCheck(toolCall, context); + } + + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (!parsed.success) { + return !isAutoResolvableTool(tool); + } + + return callLevelCheck( + { + ...toolCall, + arguments: parsed.data, + } as ParsedToolCall, + context, + ); + } + + // Fall back to the tool-level setting (server tools never require approval). if (!tool) { return false; } diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 669179cc..e9cf4b19 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -180,6 +180,92 @@ describe('approval predicate argument parity with execute (#54)', () => { expect(requires).toBe(true); }); + it('applies schema defaults for call-level checks without mutating the executable call', async () => { + const defaulted = tool({ + name: 'call_level_defaulted_action', + inputSchema: z.object({ + destructive: z.boolean().default(true), + }), + execute: async () => ({}), + }); + const toolCall = { + id: 'call-level-default', + name: 'call_level_defaulted_action', + arguments: {}, + }; + const callLevelCheck = vi.fn(() => true); + + const requires = await toolRequiresApproval( + toolCall, + [ + defaulted, + ], + context, + callLevelCheck, + ); + + expect(callLevelCheck).toHaveBeenCalledWith( + { + ...toolCall, + arguments: { + destructive: true, + }, + }, + context, + ); + expect(toolCall.arguments).toEqual({}); + expect(requires).toBe(true); + }); + + it('parses original wire arguments once for the predicate and once for execution', async () => { + let transformCalls = 0; + const predicate = vi.fn(() => false); + const execute = vi.fn(async () => ({})); + const transformed = tool({ + name: 'transformed_action', + inputSchema: z.object({ + value: z.string().transform((value) => `${value}:${++transformCalls}`), + }), + requireApproval: predicate, + execute, + }); + const toolCall = { + id: 'non-idempotent-transform', + name: 'transformed_action', + arguments: { + value: 'wire', + }, + }; + + expect( + await toolRequiresApproval( + toolCall, + [ + transformed, + ], + context, + ), + ).toBe(false); + await executeRegularTool(transformed, toolCall, context); + + expect(predicate).toHaveBeenCalledWith( + { + value: 'wire:1', + }, + context, + ); + expect(execute).toHaveBeenCalledWith( + { + value: 'wire:2', + }, + expect.anything(), + ); + expect(transformCalls).toBe(2); + expect(toolCall.arguments).toEqual({ + value: 'wire', + }); + }); + it('does not gate calls whose arguments fail schema validation', async () => { const predicate = vi.fn(() => false); From 9a415f1ffb99bdafc47a9a3fc9769b9192f7fa7a Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:36:23 -0500 Subject: [PATCH 07/14] fix(agent): preserve call-level approval precedence --- .changeset/approval-gate-fixes.md | 2 +- packages/agent/src/lib/async-params.ts | 2 +- packages/agent/src/lib/conversation-state.ts | 9 ++- packages/agent/src/lib/model-result.ts | 2 +- .../unit/approval-gate-regressions.test.ts | 56 +++++++++++++++++++ 5 files changed, 63 insertions(+), 8 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index 75505c40..f01c77a7 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -6,6 +6,6 @@ Fix two ways the tool-approval gate could be bypassed. **`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. -**Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. Arguments that don't satisfy the schema are not gated for engine-executed tools: such a call can never run (every execute path validates with the same schema first and turns the failure into a tool error output the model can recover from), so pausing for a human to approve it would only stall the run. Manual tools — which the host application executes without any engine-side validation — still fail closed on schema-invalid arguments. +**Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. Call-level checks remain unconditional and receive raw arguments when parsing fails. Tool-level checks do not gate schema-invalid engine-executed tools because every execute path revalidates and returns a tool error; manual tools still fail closed because they have no engine-side validation. **Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. diff --git a/packages/agent/src/lib/async-params.ts b/packages/agent/src/lib/async-params.ts index 9cabd7c1..6b724b25 100644 --- a/packages/agent/src/lib/async-params.ts +++ b/packages/agent/src/lib/async-params.ts @@ -67,7 +67,7 @@ type BaseCallModelInput< context?: ContextInput>; /** * Call-level approval check - overrides tool-level requireApproval setting - * Receives the tool call and turn context, can be sync or async + * Receives normalized arguments when schema parsing succeeds and raw arguments otherwise */ requireApproval?: ( toolCall: ParsedToolCall, diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index 5f42c38b..a22db1d6 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -266,7 +266,7 @@ export function appendToMessages( * @param toolCall - The tool call to check * @param tools - Available tools * @param context - Turn context for the approval check - * @param callLevelCheck - Optional call-level approval function (overrides tool-level), can be async + * @param callLevelCheck - Optional call-level approval function (overrides tool-level), can be async. Receives normalized arguments when schema parsing succeeds and raw arguments otherwise. */ export async function toolRequiresApproval( toolCall: ParsedToolCall, @@ -287,9 +287,8 @@ export async function toolRequiresApproval( } > => isClientTool(t) && t.function.name === toolCall.name, ); - // Call-level checks take precedence. When the call maps to a client tool, - // give the callback a normalized copy while preserving the original wire - // arguments for execution to parse independently. + // Call-level checks always take precedence. Pass a normalized copy when + // parsing succeeds, or the raw call when it does not. if (callLevelCheck) { if (!tool) { return callLevelCheck(toolCall, context); @@ -297,7 +296,7 @@ export async function toolRequiresApproval( const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); if (!parsed.success) { - return !isAutoResolvableTool(tool); + return callLevelCheck(toolCall, context); } return callLevelCheck( diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index f60b7944..8446f33f 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -443,7 +443,7 @@ export interface GetResponseOptions< /** * Call-level approval check - overrides tool-level requireApproval setting - * Receives the tool call and turn context, can be sync or async + * Receives normalized arguments when schema parsing succeeds and raw arguments otherwise */ requireApproval?: ( toolCall: ParsedToolCall, diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index e9cf4b19..7bf07c0f 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -217,6 +217,62 @@ describe('approval predicate argument parity with execute (#54)', () => { expect(requires).toBe(true); }); + it('honors a false call-level check when arguments fail schema validation', async () => { + const strict = tool({ + name: 'call_level_invalid_action', + inputSchema: z.object({ + target: z.string(), + }), + execute: async () => ({}), + }); + const invalidCall = { + id: 'call-level-invalid-false', + name: 'call_level_invalid_action', + arguments: {}, + }; + const callLevelCheck = vi.fn(() => false); + + expect( + await toolRequiresApproval( + invalidCall, + [ + strict, + ], + context, + callLevelCheck, + ), + ).toBe(false); + expect(callLevelCheck).toHaveBeenCalledWith(invalidCall, context); + }); + + it('honors a true call-level check when arguments fail schema validation', async () => { + const strict = tool({ + name: 'call_level_invalid_action', + inputSchema: z.object({ + target: z.string(), + }), + execute: async () => ({}), + }); + const invalidCall = { + id: 'call-level-invalid-true', + name: 'call_level_invalid_action', + arguments: {}, + }; + const callLevelCheck = vi.fn(() => true); + + expect( + await toolRequiresApproval( + invalidCall, + [ + strict, + ], + context, + callLevelCheck, + ), + ).toBe(true); + expect(callLevelCheck).toHaveBeenCalledWith(invalidCall, context); + }); + it('parses original wire arguments once for the predicate and once for execution', async () => { let transformCalls = 0; const predicate = vi.fn(() => false); From 54a5aefeeb19fbe79c1190eb58ba2dae98c2ede1 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:54:37 -0500 Subject: [PATCH 08/14] fix(agent): gate post-hook tool arguments --- .changeset/approval-gate-fixes.md | 2 +- packages/agent/src/index.ts | 1 + packages/agent/src/lib/conversation-state.ts | 20 +- packages/agent/src/lib/model-result.ts | 122 ++++++- packages/agent/src/lib/tool-types.ts | 7 +- .../unit/approval-gate-regressions.test.ts | 328 +++++++++++++++--- 6 files changed, 408 insertions(+), 72 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index f01c77a7..a617e6e7 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -6,6 +6,6 @@ Fix two ways the tool-approval gate could be bypassed. **`allowFinalResponse` executed pending tool calls with no approval check.** When a `stopWhen` condition halted the loop on a turn that still carried tool calls, the final-response path ran those calls directly — skipping the approval gate the normal loop applies on every round. A tool marked `requireApproval: true` (or gated by a predicate) would execute unguarded, and because the `PermissionRequest` hook's deny bookkeeping lives inside the approval check, hook-based `deny` never fired on this path either. That path now runs the same check as the in-loop call sites, so the run pauses with `status: 'awaiting_approval'` and the gated calls on `pendingToolCalls` instead of executing them. -**Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. Call-level checks remain unconditional and receive raw arguments when parsing fails. Tool-level checks do not gate schema-invalid engine-executed tools because every execute path revalidates and returns a tool error; manual tools still fail closed because they have no engine-side validation. +**Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. `PreToolUse` now runs before every auto-resolvable call is partitioned, so approval hooks and persisted pending calls see its effective arguments. Pending calls record an additive marker when preparation ran, preventing a resumed `ModelResult` from applying the hook twice while legacy state without the marker retains its prior behavior. Call-level checks remain unconditional and receive raw arguments when parsing fails; tool-level checks fail closed when schema parsing fails because a hook may later repair the input. **Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. diff --git a/packages/agent/src/index.ts b/packages/agent/src/index.ts index 3b612660..5f47406c 100644 --- a/packages/agent/src/index.ts +++ b/packages/agent/src/index.ts @@ -262,6 +262,7 @@ export type { ParsedToolCall, PartialResponse, PendingAsyncTool, + PendingToolCall, ResponseStreamEvent, ResponseStreamEvent as EnhancedResponseStreamEvent, ServerTool, diff --git a/packages/agent/src/lib/conversation-state.ts b/packages/agent/src/lib/conversation-state.ts index a22db1d6..c02026ca 100644 --- a/packages/agent/src/lib/conversation-state.ts +++ b/packages/agent/src/lib/conversation-state.ts @@ -12,7 +12,7 @@ import type { TurnContext, UnsentToolResult, } from './tool-types.js'; -import { isAutoResolvableTool, isClientTool } from './tool-types.js'; +import { isClientTool } from './tool-types.js'; import { normalizeInputToArray } from './turn-context.js'; @@ -332,21 +332,9 @@ export async function toolRequiresApproval( if (typeof requireApproval === 'function') { const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); if (!parsed.success) { - if (isAutoResolvableTool(tool)) { - // Engine-executed tools validate before running: every execute path - // (regular, generator, HITL onToolCalled, unified run) runs the same - // schema through validateToolInput first and converts the failure - // into a tool error output the model can recover from. Arguments - // that don't satisfy the schema can never execute, so gating them - // would pause the run so a human can approve a call that can only - // fail (or throw outright when no state accessor is configured) — - // let them through the gate to the executor's validation error. - return false; - } - // Manual tools (no execute / onToolCalled / run) are surfaced to the - // host application via pendingToolCalls and executed WITHOUT any - // engine-side validation, so the "can never execute" argument does not - // hold — fail closed rather than wave a malformed call past the gate. + // There is no trustworthy value to pass to the predicate. Fail closed: + // a PreToolUse hook may later replace invalid input with executable + // input, so schema-invalid wire arguments cannot safely bypass approval. return true; } if (!isRecord(parsed.data)) { diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 8446f33f..33f4131b 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -645,6 +645,21 @@ export class ModelResult< // normal tool round consults this to synthesize rejected outputs instead of // executing the calls. private readonly hookDeniedCalls = new Map(); + // PreToolUse must run before approval so predicates inspect the arguments + // that can actually execute. The prepared outcome is consumed by the + // execution path, preventing the hook from running twice. + private readonly preparedToolCalls = new Map< + string, + | { + type: 'ready'; + toolCall: ParsedToolCall; + } + | { + type: 'blocked'; + reason: string; + output: models.FunctionCallOutputItem; + } + >(); // The response most recently passed through handleApprovalCheck on this // run. The same response object can reach the gate more than once — the // pre-loop check plus the first loop iteration, or the pre-loop check plus @@ -1396,6 +1411,7 @@ export class ModelResult< turnContext: TurnContext, onPreliminaryResult?: (toolCallId: string, result: unknown) => void, extras?: ToolExecutionExtras, + runPreToolUse = true, ): Promise< | { type: 'parse_error'; @@ -1484,10 +1500,22 @@ export class ModelResult< }; } - let effectiveToolCall = toolCall; + const prepared = this.preparedToolCalls.get(toolCall.id); + this.preparedToolCalls.delete(toolCall.id); + if (prepared?.type === 'blocked') { + return { + type: 'hook_blocked', + toolCall, + reason: prepared.reason, + output: prepared.output, + }; + } + + let effectiveToolCall = prepared?.type === 'ready' ? prepared.toolCall : toolCall; - // Emit PreToolUse hook -- can block or mutate input. - if (this.hooksManager) { + // Emit PreToolUse here only when the approval gate has not already done + // so (for example, an approved call resumed from older persisted state). + if (this.hooksManager && !prepared && runPreToolUse) { // The hook payload coerces null/undefined arguments to {} for schema // validation, but `effectiveToolCall.arguments` only changes when the // chain reports an actual mutation (`emit.mutated`), so tools that @@ -2612,6 +2640,53 @@ export class ModelResult< }; } + private async prepareToolCallForApproval( + toolCall: ParsedToolCall, + ): Promise> { + if (!this.hooksManager || typeof toolCall.arguments === 'string') { + return toolCall; + } + + const preResult = await this.hooksManager.emit( + 'PreToolUse', + { + toolName: toolCall.name, + toolInput: (toolCall.arguments ?? {}) as Record, + }, + this.hookEmitContext(toolCall.name), + ); + + if (preResult.blocked) { + const block = preResult.results.find((result) => result.block)?.block; + const reason = typeof block === 'string' ? block : 'Blocked by PreToolUse hook'; + this.preparedToolCalls.set(toolCall.id, { + type: 'blocked', + reason, + output: { + type: 'function_call_output', + id: `output_${toolCall.id}`, + callId: toolCall.id, + output: JSON.stringify({ + error: reason, + }), + }, + }); + return toolCall; + } + + const effectiveToolCall = preResult.mutated + ? { + ...toolCall, + arguments: preResult.finalPayload.toolInput, + } + : toolCall; + this.preparedToolCalls.set(toolCall.id, { + type: 'ready', + toolCall: effectiveToolCall, + }); + return effectiveToolCall; + } + /** * Run the UserPromptSubmit hook, supporting both string and structured * inputs. If a handler returns a mutated prompt, the returned object @@ -2864,12 +2939,33 @@ export class ModelResult< // context is handled via contextStore, not on TurnContext }; + const preparedCalls = await Promise.all( + toolCalls.map(async (toolCall) => { + const tool = this.options.tools?.find( + (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, + ); + return tool && isAutoResolvableTool(tool) + ? this.prepareToolCallForApproval(toolCall) + : toolCall; + }), + ); + const blockedIds = new Set( + preparedCalls + .filter((toolCall) => this.preparedToolCalls.get(toolCall.id)?.type === 'blocked') + .map((toolCall) => toolCall.id), + ); const { requiresApproval: needsApproval, autoExecute } = await partitionToolCalls( - toolCalls as ParsedToolCall[], + preparedCalls.filter((toolCall) => !blockedIds.has(toolCall.id)) as ParsedToolCall< + TTools[number] + >[], this.options.tools, turnContext, this.requireApprovalFn ?? undefined, ); + const autoExecuteIncludingBlocked = [ + ...autoExecute, + ...preparedCalls.filter((toolCall) => blockedIds.has(toolCall.id)), + ]; // Nothing needs an approval gate: return immediately WITHOUT executing // anything. The main loop's executeToolRound runs every call exactly @@ -2931,7 +3027,7 @@ export class ModelResult< // so execute the auto-approved calls now and persist their results as // unsent so the resume path can pick them up without re-executing. const unsentResults = await this.executeAutoApproveTools( - autoExecute as ParsedToolCall[], + autoExecuteIncludingBlocked as ParsedToolCall[], turnContext, ); @@ -2947,7 +3043,12 @@ export class ModelResult< // Save state with pending approvals (only reached when stillPending > 0). const stateUpdates: Partial, 'id' | 'createdAt' | 'updatedAt'>> = { - pendingToolCalls: stillPending, + pendingToolCalls: stillPending.map((toolCall) => ({ + ...toolCall, + ...(this.preparedToolCalls.has(toolCall.id) && { + preToolUseApplied: true as const, + }), + })), status: 'awaiting_approval', }; if (combinedResults.length > 0) { @@ -5570,11 +5671,7 @@ export class ModelResult< // Declared from the approved calls only — a pending call the user did not // approve is not part of this round. await this.beginDoomLoopRound( - [ - ...this.approvedToolCalls, - ] - .map((callId) => pendingCalls.find((tc) => tc.id === callId)) - .filter((tc): tc is ParsedToolCall => tc !== undefined), + pendingCalls.filter((toolCall) => this.approvedToolCalls.includes(toolCall.id)), ); // Process approvals - execute the approved tools. Route through @@ -5600,6 +5697,9 @@ export class ModelResult< tool, toolCall as ParsedToolCall, turnContext, + undefined, + undefined, + toolCall.preToolUseApplied !== true, ); if (hookOutcome.type === 'parse_error') { diff --git a/packages/agent/src/lib/tool-types.ts b/packages/agent/src/lib/tool-types.ts index 0ea4140d..88320086 100644 --- a/packages/agent/src/lib/tool-types.ts +++ b/packages/agent/src/lib/tool-types.ts @@ -1149,6 +1149,11 @@ export interface ParsedToolCall { arguments: InferToolInput; // Typed based on tool's inputSchema } +export type PendingToolCall = ParsedToolCall & { + /** PreToolUse already ran and `arguments` contains its effective input. */ + preToolUseApplied?: true; +}; + /** * Result of tool execution. * @@ -1642,7 +1647,7 @@ export interface ConversationState>; + pendingToolCalls?: Array>; /** Tool results executed but not yet sent to the model */ unsentToolResults?: Array>; /** Partial response data captured during interruption */ diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 7bf07c0f..1dae7b59 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -322,7 +322,7 @@ describe('approval predicate argument parity with execute (#54)', () => { }); }); - it('does not gate calls whose arguments fail schema validation', async () => { + it('fails closed when arguments fail schema validation', async () => { const predicate = vi.fn(() => false); const strict = tool({ @@ -334,13 +334,9 @@ describe('approval predicate argument parity with execute (#54)', () => { execute: async () => ({}), }); - // `target` is missing entirely — the schema parse fails. Such a call can - // never execute (the executor runs the same schema through - // validateToolInput and turns the failure into a tool error output), so - // gating it would pause the run for a human to approve a call that can - // only fail — or throw outright when no state accessor is configured. - // The gate lets it through to the executor's normal validation error and - // never invokes the predicate on a value `execute` would not see. + // `target` is missing entirely. A PreToolUse hook could replace this with + // executable input later, so the gate cannot safely waive approval or + // invoke the predicate with a value the tool body would never receive. const invalidCall = { id: '3', name: 'strict_action', @@ -355,7 +351,7 @@ describe('approval predicate argument parity with execute (#54)', () => { context, ); - expect(requires).toBe(false); + expect(requires).toBe(true); expect(predicate).not.toHaveBeenCalled(); }); @@ -445,12 +441,273 @@ describe('approval predicate argument parity with execute (#54)', () => { }); }); +describe('approval uses the post-PreToolUse arguments that execute would receive', () => { + beforeEach(() => { + mockBetaResponsesSend.mockReset(); + }); + + async function runMutationExploit( + wireArguments: Record, + mutatedInput: Record, + ) { + const predicate = vi.fn((params: { dangerous: boolean }) => params.dangerous); + const execute = vi.fn(async () => ({ + ok: true, + })); + const guarded = tool({ + name: 'guarded_action', + inputSchema: z.object({ + dangerous: z.boolean(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: predicate, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: () => ({ + mutatedInput, + }), + }); + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_mutation', [ + makeFunctionCallItem('call_guarded', 'guarded_action', JSON.stringify(wireArguments)), + ]), + }); + const { accessor, get } = createMemoryAccessor(); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'run the guarded action', + tools: [ + { + type: 'function', + name: 'guarded_action', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + }); + + await result.getResponse(); + + expect(predicate).toHaveBeenCalledWith(mutatedInput, { + numberOfTurns: 0, + }); + expect(execute).not.toHaveBeenCalled(); + expect(get()?.status).toBe('awaiting_approval'); + expect(get()?.pendingToolCalls).toEqual([ + { + id: 'call_guarded', + name: 'guarded_action', + arguments: mutatedInput, + preToolUseApplied: true, + }, + ]); + } + + it('does not let a hook rewrite safe arguments into an unapproved dangerous call', async () => { + await runMutationExploit( + { + dangerous: false, + }, + { + dangerous: true, + }, + ); + }); + + it('does not let a hook make schema-invalid arguments executable after the gate', async () => { + await runMutationExploit( + {}, + { + dangerous: true, + }, + ); + }); + + it('persists unconditional approvals with post-hook arguments and does not reapply the hook on resume', async () => { + const preToolUse = vi.fn(({ toolInput }: { toolInput: Record }) => ({ + mutatedInput: { + value: `${String(toolInput['value'])}-prepared`, + }, + })); + const permissionRequest = vi.fn(() => ({ + decision: 'ask_user' as const, + })); + const execute = vi.fn(async (input: { value: string }) => ({ + value: input.value, + })); + const guarded = tool({ + name: 'always_guarded', + inputSchema: z.object({ + value: z.string(), + }), + outputSchema: z.object({ + value: z.string(), + }), + requireApproval: true, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: preToolUse, + }); + hooks.on('PermissionRequest', { + handler: permissionRequest, + }); + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_unconditional', [ + makeFunctionCallItem( + 'call_unconditional', + 'always_guarded', + JSON.stringify({ + value: 'original', + }), + ), + ]), + }); + const { accessor, get } = createMemoryAccessor(); + const request = { + model: 'test-model', + input: 'run the guarded action', + tools: [ + { + type: 'function' as const, + name: 'always_guarded', + description: null, + strict: null, + parameters: {}, + }, + ], + }; + + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + }).getResponse(); + + expect(permissionRequest).toHaveBeenCalledWith( + expect.objectContaining({ + toolInput: { + value: 'original-prepared', + }, + }), + expect.anything(), + ); + expect(get()?.pendingToolCalls).toEqual([ + { + id: 'call_unconditional', + name: 'always_guarded', + arguments: { + value: 'original-prepared', + }, + preToolUseApplied: true, + }, + ]); + const legacyState = structuredClone(get()); + if (legacyState?.pendingToolCalls?.[0]) { + delete legacyState.pendingToolCalls[0].preToolUseApplied; + } + + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_after_approval', [ + { + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + approveToolCalls: [ + 'call_unconditional', + ], + }).getResponse(); + + expect(preToolUse).toHaveBeenCalledTimes(1); + expect(execute).toHaveBeenCalledTimes(1); + expect(execute).toHaveBeenCalledWith( + { + value: 'original-prepared', + }, + expect.anything(), + ); + + const { accessor: legacyAccessor } = createMemoryAccessor(legacyState); + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_after_legacy_approval', [ + { + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + await new ModelResult({ + request, + client: {} as OpenRouterCore, + tools, + hooks, + state: legacyAccessor, + approveToolCalls: [ + 'call_unconditional', + ], + }).getResponse(); + + expect(preToolUse).toHaveBeenCalledTimes(2); + expect(execute).toHaveBeenLastCalledWith( + { + value: 'original-prepared-prepared', + }, + expect.anything(), + ); + }); +}); + // --------------------------------------------------------------------------- -// The gate's fail-open for schema-invalid arguments is only sound because -// every engine execute path re-validates with the tool's inputSchema before -// running the tool body. Lock that invariant in: if a future execute path -// (or a refactor of an existing one) ever skips validateToolInput, these -// tests fail — and the approval gate's fail-open must be revisited. +// Every engine execute path validates before running the tool body. This is +// still required for ordinary validation errors, independent of approval. // --------------------------------------------------------------------------- describe('every engine execute path validates before running the tool body', () => { const inputSchema = z.object({ @@ -948,7 +1205,7 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { expect(JSON.stringify(outputs[0]?.output)).toContain('blocked by policy'); }); - it('surfaces schema-invalid arguments as a tool error instead of pausing or throwing', async () => { + it('fails closed on schema-invalid arguments that no hook repairs', async () => { const execute = vi.fn(async () => ({ ok: true, })); @@ -969,12 +1226,9 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { strict, ] as const; - // The model emits `{}` — `target` is missing, so the arguments can never - // satisfy the schema. The gate must let the call through to the executor, - // which turns the validation failure into a tool error output the model - // can recover from. Gating it instead would pause the run for a human to - // approve a call that can only fail — and with no state accessor - // configured (as here), handleApprovalCheck would throw outright. + // The model emits `{}` — `target` is missing. Because a PreToolUse hook + // could repair it before execution, the predicate has no safe value to + // inspect and the gate fails closed. mockBetaResponsesSend .mockResolvedValueOnce({ ok: true, @@ -1022,31 +1276,19 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { state: accessor, }); - const text = await result.getText(); + await result.getResponse(); - // No throw, no pause: the run completes normally. - expect(text).toBe('Recovered.'); const saved = get(); - expect(saved?.status).toBe('complete'); - - // The tool body never ran — validation failed first — and the failure was - // recorded as the call's output so the model could see it and recover. + expect(saved?.status).toBe('awaiting_approval'); + expect(saved?.pendingToolCalls).toEqual([ + { + id: 'call_strict', + name: 'strict', + arguments: {}, + }, + ]); expect(execute).not.toHaveBeenCalled(); - const outputs = (saved?.messages ?? []).filter( - (m): m is models.FunctionCallOutputItem => - typeof m === 'object' && - m !== null && - 'type' in m && - m.type === 'function_call_output' && - 'callId' in m && - m.callId === 'call_strict', - ); - expect(outputs).toHaveLength(1); - expect(JSON.stringify(outputs[0]?.output)).toContain('target'); - - // One round trip for the initial request, one for the follow-up carrying - // the validation error. - expect(mockBetaResponsesSend).toHaveBeenCalledTimes(2); + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(1); }); it('gates the initial response only once when the stop condition fires on the first iteration', async () => { From f51b2dcf63b62aef6bd18a1bde1e969dab3612d9 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:47:18 -0500 Subject: [PATCH 09/14] fix(agent): split tool approval into two phases --- packages/agent/src/lib/hooks-schemas.ts | 10 +- packages/agent/src/lib/model-result.ts | 390 +++++++++++++----- .../unit/approval-gate-regressions.test.ts | 152 ++++++- .../tests/unit/doom-loop-remediation.test.ts | 36 +- 4 files changed, 428 insertions(+), 160 deletions(-) diff --git a/packages/agent/src/lib/hooks-schemas.ts b/packages/agent/src/lib/hooks-schemas.ts index aa0b2023..8087d04d 100644 --- a/packages/agent/src/lib/hooks-schemas.ts +++ b/packages/agent/src/lib/hooks-schemas.ts @@ -75,13 +75,9 @@ export const PostToolUseFailurePayloadSchema = z4.object({ }); /** - * Fired when a tool EXECUTION throws or returns an error. - * - * Deliberately NOT fired when a tool never ran: a PermissionRequest 'deny', - * a user rejection on approval resume, or a PreToolUse block all synthesize - * a rejected result without execution, so no failure event is emitted. - * Observe those outcomes via the PermissionRequest / PreToolUse hooks - * themselves. + * Fired when an entered tool lifecycle fails after PreToolUse, including a + * mutated-input approval denial/rejection or schema-invalid mutation. + * Initial approval denials and rejections do not enter the lifecycle. */ export type PostToolUseFailurePayload = Readonly>; diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 33f4131b..8c9762c8 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -3,6 +3,7 @@ import { betaResponsesSend } from '@openrouter/sdk/funcs/betaResponsesSend'; import type { EventStream } from '@openrouter/sdk/lib/event-streams'; import type { RequestOptions } from '@openrouter/sdk/lib/sdks'; import type * as models from '@openrouter/sdk/models'; +import * as z4 from 'zod/v4'; import type { $ZodObject, $ZodShape } from 'zod/v4/core'; import type { CallModelInput, ResolvedCallModelInput } from './async-params.js'; import { hasAsyncFunctions, resolveAsyncFunctions } from './async-params.js'; @@ -29,6 +30,7 @@ import { extractTextFromResponse as extractTextFromResponseState, normalizeInputToArray, partitionToolCalls, + toolRequiresApproval, unsentResultsToAPIFormat, updateState, } from './conversation-state.js'; @@ -645,14 +647,13 @@ export class ModelResult< // normal tool round consults this to synthesize rejected outputs instead of // executing the calls. private readonly hookDeniedCalls = new Map(); - // PreToolUse must run before approval so predicates inspect the arguments - // that can actually execute. The prepared outcome is consumed by the - // execution path, preventing the hook from running twice. + // PreToolUse outcomes parked between the approval gates and execution. private readonly preparedToolCalls = new Map< string, | { type: 'ready'; toolCall: ParsedToolCall; + mutated: boolean; } | { type: 'blocked'; @@ -660,14 +661,11 @@ export class ModelResult< output: models.FunctionCallOutputItem; } >(); - // The response most recently passed through handleApprovalCheck on this - // run. The same response object can reach the gate more than once — the - // pre-loop check plus the first loop iteration, or the pre-loop check plus - // the post-loop allowFinalResponse gate when a stop condition fires before - // any follow-up request. Re-gating would re-emit PermissionRequest hooks - // (duplicate prompts/audit records) and re-run requireApproval predicates - // for calls already resolved, so the gate runs at most once per response. - private lastApprovalGatedResponse: models.OpenResponsesResult | null = null; + // Approval is idempotent per response occurrence, call id, argument state, + // and phase. This distinguishes a newly emitted call that reuses an id. + private readonly completedApprovalGates = new Set(); + private readonly approvalResponseOccurrences = new WeakMap(); + private nextApprovalResponseOccurrence = 0; // Telemetry for the PostModelCall hook: the initial/resume request is // dispatched in initStream but its response is materialized later (stream // consumption), so the dispatch time and turn labeling are parked here @@ -1440,23 +1438,6 @@ export class ModelResult< // synthetic error without running the tool or firing hooks. const rawArgs: unknown = toolCall.arguments; if (typeof rawArgs === 'string') { - // Malformed calls are classic doom-loop fuel: a model stuck emitting - // the same invalid JSON re-triggers this parse error forever. Record - // the raw string as the call's identity so the streak trips the - // detector instead of bouncing off the parse error unbounded. The - // parse-error output below already prevents execution, so 'block' - // needs no extra handling; 'steer'/'stop' side effects are applied - // inside the ordered evaluation. - if (this.doomLoopMonitor) { - await this.enqueueDoomLoopEvaluation({ - toolName: String(toolCall.name), - keyMaterial: rawArgs, - fallbackKeyMaterial: null, - allowBlock: true, - detector: 'tool-fingerprint', - toolCall, - }); - } const errorMessage = `Failed to parse tool call arguments for "${toolCall.name}": The model provided invalid JSON. ` + `Raw arguments received: "${rawArgs}". ` + @@ -1476,30 +1457,6 @@ export class ModelResult< }; } - // Doom-loop checkpoint — BEFORE PreToolUse, on the arguments the MODEL - // issued (pre-mutation): the loop evidence is the model repeating - // itself, and a PreToolUse hook that rewrites input each call (e.g. - // injecting a nonce) must not mask that repetition. A 'block' verdict - // returns the same `hook_blocked` shape as a PreToolUse block, so every - // caller handles it identically; PreToolUse deliberately does not fire - // for doom-blocked calls (nothing will execute — mirrors parse_error). - const doomOutcome = await this.checkDoomLoopBeforeExecution(tool, toolCall); - if (doomOutcome.blocked) { - return { - type: 'hook_blocked', - toolCall, - reason: doomOutcome.reason, - output: { - type: 'function_call_output' as const, - id: `output_${toolCall.id}`, - callId: toolCall.id, - output: JSON.stringify({ - error: doomOutcome.reason, - }), - }, - }; - } - const prepared = this.preparedToolCalls.get(toolCall.id); this.preparedToolCalls.delete(toolCall.id); if (prepared?.type === 'blocked') { @@ -2683,10 +2640,147 @@ export class ModelResult< this.preparedToolCalls.set(toolCall.id, { type: 'ready', toolCall: effectiveToolCall, + mutated: preResult.mutated, }); return effectiveToolCall; } + private approvalResponseKey(response: models.OpenResponsesResult): string { + let occurrence = this.approvalResponseOccurrences.get(response); + if (occurrence === undefined) { + occurrence = this.nextApprovalResponseOccurrence++; + this.approvalResponseOccurrences.set(response, occurrence); + } + return `${response.id}:${occurrence}`; + } + + private approvalGateKey( + toolCall: ParsedToolCall, + phase: 'initial' | 'mutated', + responseKey: string, + ): string { + return `${phase}:${responseKey}:${toolCall.id}:${canonicalizeKeyMaterial(toolCall.arguments)}`; + } + + /** Re-check only approval sources whose answer can depend on input. */ + private async mutatedInputRequiresApproval( + toolCall: ParsedToolCall, + context: TurnContext, + ): Promise { + const tools = this.options.tools; + if (!tools) { + return false; + } + const tool = tools.find( + (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, + ); + if (this.requireApprovalFn) { + return toolRequiresApproval( + toolCall as ParsedToolCall, + tools, + context, + this.requireApprovalFn, + ); + } + if (!tool || !isClientTool(tool) || typeof tool.function.requireApproval !== 'function') { + return false; + } + return toolRequiresApproval(toolCall as ParsedToolCall, tools, context); + } + + private async emitPreparedFailure(toolCall: ParsedToolCall, reason: string): Promise { + if (!this.hooksManager) { + return; + } + await this.hooksManager.emit( + 'PostToolUseFailure', + { + toolName: toolCall.name, + toolInput: (toolCall.arguments ?? {}) as Record, + error: new Error(reason), + }, + this.hookEmitContext(toolCall.name), + ); + } + + private async prepareAfterInitialApproval( + toolCall: ParsedToolCall, + context: TurnContext, + responseKey: string, + ): Promise<'ready' | 'blocked' | 'pending'> { + const effective = await this.prepareToolCallForApproval(toolCall); + const prepared = this.preparedToolCalls.get(toolCall.id); + if (prepared?.type !== 'ready' || !prepared.mutated) { + return prepared?.type === 'blocked' ? 'blocked' : 'ready'; + } + + const tool = this.options.tools?.find( + (candidate) => isClientTool(candidate) && candidate.function.name === effective.name, + ); + if (!tool || !isClientTool(tool)) { + return 'ready'; + } + const parsed = z4.safeParse(tool.function.inputSchema, effective.arguments); + if (!parsed.success || !isRecord(parsed.data)) { + const reason = `PreToolUse produced invalid input for "${effective.name}"`; + this.preparedToolCalls.set(effective.id, { + type: 'blocked', + reason, + output: { + type: 'function_call_output', + id: `output_${effective.id}`, + callId: effective.id, + output: JSON.stringify({ + error: reason, + }), + }, + }); + await this.emitPreparedFailure(effective, reason); + return 'blocked'; + } + + const normalized = { + ...effective, + arguments: parsed.data, + } as ParsedToolCall; + this.preparedToolCalls.set(effective.id, { + type: 'ready', + toolCall: normalized, + mutated: true, + }); + const key = this.approvalGateKey(normalized, 'mutated', responseKey); + if (this.completedApprovalGates.has(key)) { + return 'ready'; + } + this.completedApprovalGates.add(key); + if (!(await this.mutatedInputRequiresApproval(normalized, context))) { + return 'ready'; + } + + const { decision, reason } = await this.emitPermissionRequest(normalized); + if (decision === 'allow') { + return 'ready'; + } + if (decision === 'deny') { + const denial = reason ?? 'Denied by PermissionRequest hook'; + this.preparedToolCalls.set(normalized.id, { + type: 'blocked', + reason: denial, + output: { + type: 'function_call_output', + id: `output_${normalized.id}`, + callId: normalized.id, + output: JSON.stringify({ + error: denial, + }), + }, + }); + await this.emitPreparedFailure(normalized, denial); + return 'blocked'; + } + return 'pending'; + } + /** * Run the UserPromptSubmit hook, supporting both string and structured * inputs. If a handler returns a mutated prompt, the returned object @@ -2831,9 +2925,6 @@ export class ModelResult< toolCalls: ParsedToolCall[], turnContext: TurnContext, ): Promise[]> { - // Auto-approved batch = one doom-loop round: identical parallel calls - // count once (see beginDoomLoopRound). - await this.beginDoomLoopRound(toolCalls as ParsedToolCall[]); const toolCallPromises = toolCalls.map(async (tc) => { const tool = this.options.tools?.find((t) => isClientTool(t) && t.function.name === tc.name); if (!tool || !isAutoResolvableTool(tool)) { @@ -2923,56 +3014,79 @@ export class ModelResult< return false; } - // Each response is gated at most once per run (see the field doc). A - // repeat visit for the same response object means the calls were already - // partitioned, the hooks already fired, and any hook 'deny' results are - // already recorded in hookDeniedCalls — and it did not pause (a pause - // returns out of the run). Skipping is therefore both safe and required - // to avoid duplicate permission prompts. - if (currentResponse === this.lastApprovalGatedResponse) { - return false; - } - this.lastApprovalGatedResponse = currentResponse; - const turnContext: TurnContext = { numberOfTurns: currentRound, - // context is handled via contextStore, not on TurnContext }; + const responseKey = this.approvalResponseKey(currentResponse); - const preparedCalls = await Promise.all( - toolCalls.map(async (toolCall) => { - const tool = this.options.tools?.find( - (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, - ); - return tool && isAutoResolvableTool(tool) - ? this.prepareToolCallForApproval(toolCall) - : toolCall; - }), - ); - const blockedIds = new Set( - preparedCalls - .filter((toolCall) => this.preparedToolCalls.get(toolCall.id)?.type === 'blocked') - .map((toolCall) => toolCall.id), + const unseenCalls: ParsedToolCall[] = []; + const blockedCalls: ParsedToolCall[] = []; + const unseenKeys = toolCalls.filter( + (toolCall) => + !this.completedApprovalGates.has(this.approvalGateKey(toolCall, 'initial', responseKey)), ); + await this.beginDoomLoopRound(unseenKeys); + for (const toolCall of toolCalls) { + const key = this.approvalGateKey(toolCall, 'initial', responseKey); + if (this.completedApprovalGates.has(key)) { + continue; + } + this.completedApprovalGates.add(key); + const tool = this.options.tools.find( + (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, + ); + if (tool && isAutoResolvableTool(tool)) { + const rawArgs: unknown = toolCall.arguments; + const doomOutcome = + typeof rawArgs === 'string' && this.doomLoopMonitor + ? await this.enqueueDoomLoopEvaluation({ + toolName: String(toolCall.name), + keyMaterial: rawArgs, + fallbackKeyMaterial: null, + allowBlock: true, + detector: 'tool-fingerprint', + toolCall, + }).then((decision) => + decision.action === 'block' || decision.action === 'stop' + ? { + blocked: true as const, + reason: decision.message ?? 'Blocked by doom loop', + } + : { + blocked: false as const, + }, + ) + : await this.checkDoomLoopBeforeExecution(tool, toolCall); + if (doomOutcome.blocked) { + this.preparedToolCalls.set(toolCall.id, { + type: 'blocked', + reason: doomOutcome.reason, + output: { + type: 'function_call_output', + id: `output_${toolCall.id}`, + callId: toolCall.id, + output: JSON.stringify({ + error: doomOutcome.reason, + }), + }, + }); + blockedCalls.push(toolCall); + continue; + } + } + unseenCalls.push(toolCall); + } + + if (unseenCalls.length === 0 && blockedCalls.length === 0) { + return false; + } + const { requiresApproval: needsApproval, autoExecute } = await partitionToolCalls( - preparedCalls.filter((toolCall) => !blockedIds.has(toolCall.id)) as ParsedToolCall< - TTools[number] - >[], + unseenCalls as ParsedToolCall[], this.options.tools, turnContext, this.requireApprovalFn ?? undefined, ); - const autoExecuteIncludingBlocked = [ - ...autoExecute, - ...preparedCalls.filter((toolCall) => blockedIds.has(toolCall.id)), - ]; - - // Nothing needs an approval gate: return immediately WITHOUT executing - // anything. The main loop's executeToolRound runs every call exactly - // once; pre-executing here would double-run side-effecting tools. - if (needsApproval.length === 0) { - return false; - } // Run the PermissionRequest hook for each tool that needs approval. // This lets hooks short-circuit the approval flow in either direction: @@ -3004,10 +3118,34 @@ export class ModelResult< stillPending.push(...needsApproval); } + const initialSurvivors = [ + ...autoExecute, + ...needsApproval.filter( + (tc) => + !stillPending.some((pending) => pending.id === tc.id) && + !denied.some((d) => d.tc.id === tc.id), + ), + ] as ParsedToolCall[]; + const secondPending: ParsedToolCall[] = []; + for (const toolCall of initialSurvivors) { + const tool = this.options.tools.find( + (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, + ); + if (!tool || !isAutoResolvableTool(tool)) { + continue; + } + if ( + (await this.prepareAfterInitialApproval(toolCall, turnContext, responseKey)) === 'pending' + ) { + const prepared = this.preparedToolCalls.get(toolCall.id); + if (prepared?.type === 'ready') { + secondPending.push(prepared.toolCall); + } + } + } + stillPending.push(...secondPending); + if (stillPending.length === 0) { - // The hook resolved every gated call, so we do not pause. Record denied - // calls so executeToolRound synthesizes rejections instead of running - // them; allowed calls execute once via the normal round. for (const d of denied) { this.hookDeniedCalls.set(d.tc.id, d.reason); } @@ -3026,8 +3164,13 @@ export class ModelResult< // We are pausing: the normal tool round will NOT run for this response, // so execute the auto-approved calls now and persist their results as // unsent so the resume path can pick them up without re-executing. + const pendingIds = new Set(stillPending.map((call) => call.id)); + const executableNow = [ + ...initialSurvivors.filter((call) => !pendingIds.has(call.id)), + ...blockedCalls, + ]; const unsentResults = await this.executeAutoApproveTools( - autoExecuteIncludingBlocked as ParsedToolCall[], + executableNow as ParsedToolCall[], turnContext, ); @@ -3056,6 +3199,8 @@ export class ModelResult< } await this.saveStateSafely(stateUpdates); + this.preparedToolCalls.clear(); + this.hookDeniedCalls.clear(); this.finalResponse = currentResponse; return true; // Pause for approval } @@ -3639,9 +3784,6 @@ export class ModelResult< pausedCalls: ParsedToolCall[]; deferredTasks: PendingAsyncTool[]; }> { - // One executed batch = one doom-loop round: identical parallel calls in - // this batch count as ONE piece of loop evidence and share a decision. - await this.beginDoomLoopRound(toolCalls); const toolCallPromises = toolCalls.map((toolCall) => this.executeSingleToolCall(toolCall, turnContext), ); @@ -5661,19 +5803,13 @@ export class ModelResult< // context is handled via contextStore, not on TurnContext }; + // Calls that pause again after PreToolUse mutation remain pending without + // re-running the hook on the next resume. + const secondGatePausedIds = new Set(); // Track approved HITL calls that paused (onToolCalled returned null) — // these stay in pendingToolCalls so the caller can resume them later. const hitlPausedIds = new Set(); - // The approved batch is one doom-loop round: N approved duplicates of - // the same call count once (the sequential loop below still evaluates - // in order; restored streaks from the persisted state carry forward). - // Declared from the approved calls only — a pending call the user did not - // approve is not part of this round. - await this.beginDoomLoopRound( - pendingCalls.filter((toolCall) => this.approvedToolCalls.includes(toolCall.id)), - ); - // Process approvals - execute the approved tools. Route through // runToolWithHooks so PreToolUse/PostToolUse fire even on this path. for (const callId of this.approvedToolCalls) { @@ -5693,13 +5829,31 @@ export class ModelResult< continue; } + if (toolCall.preToolUseApplied !== true) { + const prepared = await this.prepareAfterInitialApproval( + toolCall as ParsedToolCall, + turnContext, + `persisted:${this.currentState.previousResponseId ?? 'unknown'}`, + ); + if (prepared === 'pending') { + const ready = this.preparedToolCalls.get(callId); + if (ready?.type === 'ready') { + Object.assign(toolCall, ready.toolCall, { + preToolUseApplied: true as const, + }); + } + secondGatePausedIds.add(callId); + continue; + } + } + const hookOutcome = await this.runToolWithHooks( tool, toolCall as ParsedToolCall, turnContext, undefined, undefined, - toolCall.preToolUseApplied !== true, + false, ); if (hookOutcome.type === 'parse_error') { @@ -5752,7 +5906,13 @@ export class ModelResult< continue; } - unsentResults.push(createRejectedResult(callId, String(toolCall.name), 'Rejected by user')); + const reason = 'Rejected by user'; + if (toolCall.preToolUseApplied === true) { + await this.emitPreparedFailure(toolCall as ParsedToolCall, reason); + } + this.preparedToolCalls.delete(callId); + this.hookDeniedCalls.delete(callId); + unsentResults.push(createRejectedResult(callId, String(toolCall.name), reason)); } // Remove processed calls from pending. Approved HITL calls that paused are @@ -5762,7 +5922,7 @@ export class ModelResult< [ ...this.approvedToolCalls, ...this.rejectedToolCalls, - ].filter((id) => !hitlPausedIds.has(id)), + ].filter((id) => !hitlPausedIds.has(id) && !secondGatePausedIds.has(id)), ); const remainingPending = pendingCalls.filter((tc) => !processedIds.has(tc.id)); @@ -5817,6 +5977,8 @@ export class ModelResult< // user message between dangling function_calls and their future // outputs would be invalid history). if (nextStatus !== 'in_progress') { + this.preparedToolCalls.clear(); + this.hookDeniedCalls.clear(); return; } diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 1dae7b59..3bbe2656 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -529,13 +529,147 @@ describe('approval uses the post-PreToolUse arguments that execute would receive ); }); - it('does not let a hook make schema-invalid arguments executable after the gate', async () => { - await runMutationExploit( - {}, - { + it('gates identical post-hook calls independently across responses', async () => { + const predicate = vi.fn((params: { dangerous: boolean }) => params.dangerous); + const execute = vi.fn(async () => ({ + ok: true, + })); + const guarded = tool({ + name: 'guarded_action', + inputSchema: z.object({ + dangerous: z.boolean(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: predicate, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: () => ({ + mutatedInput: { + dangerous: true, + }, + }), + }); + const permissionRequest = vi.fn(() => ({ + decision: 'allow' as const, + })); + hooks.on('PermissionRequest', { + handler: permissionRequest, + }); + + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_mutation_reused', [ + makeFunctionCallItem( + 'call_reused', + 'guarded_action', + JSON.stringify({ + dangerous: false, + }), + ), + ]), + }) + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_mutation_reused', [ + makeFunctionCallItem( + 'call_reused', + 'guarded_action', + JSON.stringify({ + dangerous: false, + }), + ), + ]), + }) + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_done', [ + { + id: 'msg_done', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + await new ModelResult({ + request: { + model: 'test-model', + input: 'run twice', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + hooks, + }).getResponse(); + + expect(predicate.mock.calls.filter(([params]) => params.dangerous === true)).toHaveLength(2); + expect(permissionRequest).toHaveBeenCalledTimes(2); + expect(execute).toHaveBeenCalledTimes(2); + }); + + it('gates schema-invalid model arguments before running PreToolUse', async () => { + const preToolUse = vi.fn(() => ({ + mutatedInput: { dangerous: true, }, - ); + })); + const execute = vi.fn(async () => ({ + ok: true, + })); + const guarded = tool({ + name: 'guarded_action', + inputSchema: z.object({ + dangerous: z.boolean(), + }), + requireApproval: (params) => params.dangerous, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: preToolUse, + }); + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_invalid_original', [ + makeFunctionCallItem('call_invalid_original', 'guarded_action', '{}'), + ]), + }); + const { accessor, get } = createMemoryAccessor(); + + await new ModelResult({ + request: { + model: 'test-model', + input: 'run', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + }).getResponse(); + + expect(preToolUse).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); + expect(get()?.status).toBe('awaiting_approval'); }); it('persists unconditional approvals with post-hook arguments and does not reapply the hook on resume', async () => { @@ -609,7 +743,7 @@ describe('approval uses the post-PreToolUse arguments that execute would receive expect(permissionRequest).toHaveBeenCalledWith( expect.objectContaining({ toolInput: { - value: 'original-prepared', + value: 'original', }, }), expect.anything(), @@ -619,9 +753,8 @@ describe('approval uses the post-PreToolUse arguments that execute would receive id: 'call_unconditional', name: 'always_guarded', arguments: { - value: 'original-prepared', + value: 'original', }, - preToolUseApplied: true, }, ]); const legacyState = structuredClone(get()); @@ -658,6 +791,7 @@ describe('approval uses the post-PreToolUse arguments that execute would receive }).getResponse(); expect(preToolUse).toHaveBeenCalledTimes(1); + expect(permissionRequest).toHaveBeenCalledTimes(1); expect(execute).toHaveBeenCalledTimes(1); expect(execute).toHaveBeenCalledWith( { @@ -698,7 +832,7 @@ describe('approval uses the post-PreToolUse arguments that execute would receive expect(preToolUse).toHaveBeenCalledTimes(2); expect(execute).toHaveBeenLastCalledWith( { - value: 'original-prepared-prepared', + value: 'original-prepared', }, expect.anything(), ); diff --git a/packages/agent/tests/unit/doom-loop-remediation.test.ts b/packages/agent/tests/unit/doom-loop-remediation.test.ts index 6ce08639..941c9002 100644 --- a/packages/agent/tests/unit/doom-loop-remediation.test.ts +++ b/packages/agent/tests/unit/doom-loop-remediation.test.ts @@ -588,37 +588,13 @@ describe('D2/D3: approval-resume doom gating and verdict persistence', () => { pausedCallId as string, ], }); + const requestsBeforeResume = mockBetaResponsesSend.mock.calls.length; await resume1.getResponse().catch(() => undefined); - const pausedAgain = accessor.getLatest(); - expect(pausedAgain?.status).toBe('awaiting_approval'); - const secondCallId = pausedAgain?.pendingToolCalls?.[0]?.id; - expect(secondCallId).toBeDefined(); - const requestsBeforeResume2 = mockBetaResponsesSend.mock.calls.length; - - // Resume 2: approving the second identical call crosses the stop rung - // (restored streak 1 + this execution = 2). NO unsent-results model - // request may fire. - const resume2 = callModel(client, { - model: 'test-model', - input: undefined as unknown as string, - tools: [ - approvalTool, - ] as const, - doomLoop: { - ladder: { - observe: 1, - block: false, - stop: 2, - }, - }, - toolChoice: 'required', - state: accessor, - approveToolCalls: [ - secondCallId as string, - ], - }); - const verdict = await resume2.getDoomLoopVerdict(); + // The initial checkpoint now runs before approval. The repeated call in + // the response after resume therefore crosses the stop rung before a + // second approval pause or PreToolUse lifecycle can begin. + const verdict = await resume1.getDoomLoopVerdict(); expect(verdict).toMatchObject({ action: 'stop', @@ -626,7 +602,7 @@ describe('D2/D3: approval-resume doom gating and verdict persistence', () => { toolName: 'risky', }); // No additional model request after the doom stop. - expect(mockBetaResponsesSend).toHaveBeenCalledTimes(requestsBeforeResume2); + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(requestsBeforeResume + 1); // The stop verdict is persisted for decision-only resumes. expect(accessor.getLatest()?.doomLoop?.stopVerdict).toMatchObject({ From 9bd6d01b03c63a18fffa65c10c0be4c2e9fb014b Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:10:11 -0500 Subject: [PATCH 10/14] refactor(agent): split approval gate phases --- packages/agent/src/lib/model-result.ts | 246 +++++++++++++++---------- 1 file changed, 147 insertions(+), 99 deletions(-) diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 8c9762c8..e53160bc 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -2703,40 +2703,51 @@ export class ModelResult< ); } - private async prepareAfterInitialApproval( + private async blockPreparedToolCall( toolCall: ParsedToolCall, - context: TurnContext, - responseKey: string, - ): Promise<'ready' | 'blocked' | 'pending'> { - const effective = await this.prepareToolCallForApproval(toolCall); - const prepared = this.preparedToolCalls.get(toolCall.id); - if (prepared?.type !== 'ready' || !prepared.mutated) { - return prepared?.type === 'blocked' ? 'blocked' : 'ready'; - } + reason: string, + ): Promise { + this.preparedToolCalls.set(toolCall.id, { + type: 'blocked', + reason, + output: { + type: 'function_call_output', + id: `output_${toolCall.id}`, + callId: toolCall.id, + output: JSON.stringify({ + error: reason, + }), + }, + }); + await this.emitPreparedFailure(toolCall, reason); + } + private async normalizePreparedMutation(effective: ParsedToolCall): Promise< + | { + status: 'ready' | 'blocked'; + } + | { + status: 'normalized'; + toolCall: ParsedToolCall; + } + > { const tool = this.options.tools?.find( (candidate) => isClientTool(candidate) && candidate.function.name === effective.name, ); if (!tool || !isClientTool(tool)) { - return 'ready'; + return { + status: 'ready', + }; } const parsed = z4.safeParse(tool.function.inputSchema, effective.arguments); if (!parsed.success || !isRecord(parsed.data)) { - const reason = `PreToolUse produced invalid input for "${effective.name}"`; - this.preparedToolCalls.set(effective.id, { - type: 'blocked', - reason, - output: { - type: 'function_call_output', - id: `output_${effective.id}`, - callId: effective.id, - output: JSON.stringify({ - error: reason, - }), - }, - }); - await this.emitPreparedFailure(effective, reason); - return 'blocked'; + await this.blockPreparedToolCall( + effective, + `PreToolUse produced invalid input for "${effective.name}"`, + ); + return { + status: 'blocked', + }; } const normalized = { @@ -2748,6 +2759,28 @@ export class ModelResult< toolCall: normalized, mutated: true, }); + return { + status: 'normalized', + toolCall: normalized, + }; + } + + private async prepareAfterInitialApproval( + toolCall: ParsedToolCall, + context: TurnContext, + responseKey: string, + ): Promise<'ready' | 'blocked' | 'pending'> { + const effective = await this.prepareToolCallForApproval(toolCall); + const prepared = this.preparedToolCalls.get(toolCall.id); + if (prepared?.type !== 'ready' || !prepared.mutated) { + return prepared?.type === 'blocked' ? 'blocked' : 'ready'; + } + + const mutation = await this.normalizePreparedMutation(effective); + if (mutation.status !== 'normalized') { + return mutation.status; + } + const normalized = mutation.toolCall; const key = this.approvalGateKey(normalized, 'mutated', responseKey); if (this.completedApprovalGates.has(key)) { return 'ready'; @@ -2762,20 +2795,7 @@ export class ModelResult< return 'ready'; } if (decision === 'deny') { - const denial = reason ?? 'Denied by PermissionRequest hook'; - this.preparedToolCalls.set(normalized.id, { - type: 'blocked', - reason: denial, - output: { - type: 'function_call_output', - id: `output_${normalized.id}`, - callId: normalized.id, - output: JSON.stringify({ - error: denial, - }), - }, - }); - await this.emitPreparedFailure(normalized, denial); + await this.blockPreparedToolCall(normalized, reason ?? 'Denied by PermissionRequest hook'); return 'blocked'; } return 'pending'; @@ -2995,30 +3015,13 @@ export class ModelResult< return results; } - /** - * Check for tools requiring approval and handle accordingly. - * Partitions tool calls into those needing approval and those that can auto-execute. - * - * @param toolCalls - The tool calls to check - * @param currentRound - The current execution round (1-indexed) - * @param currentResponse - The current response to save if pausing - * @returns True if execution should pause for approval, false to continue - * @throws Error if approval is required but no state accessor is configured - */ - private async handleApprovalCheck( + private async classifyInitialApprovalCalls( toolCalls: ParsedToolCall[], - currentRound: number, - currentResponse: models.OpenResponsesResult, - ): Promise { - if (!this.options.tools) { - return false; - } - - const turnContext: TurnContext = { - numberOfTurns: currentRound, - }; - const responseKey = this.approvalResponseKey(currentResponse); - + responseKey: string, + ): Promise<{ + unseenCalls: ParsedToolCall[]; + blockedCalls: ParsedToolCall[]; + }> { const unseenCalls: ParsedToolCall[] = []; const blockedCalls: ParsedToolCall[] = []; const unseenKeys = toolCalls.filter( @@ -3026,13 +3029,14 @@ export class ModelResult< !this.completedApprovalGates.has(this.approvalGateKey(toolCall, 'initial', responseKey)), ); await this.beginDoomLoopRound(unseenKeys); + for (const toolCall of toolCalls) { const key = this.approvalGateKey(toolCall, 'initial', responseKey); if (this.completedApprovalGates.has(key)) { continue; } this.completedApprovalGates.add(key); - const tool = this.options.tools.find( + const tool = this.options.tools?.find( (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, ); if (tool && isAutoResolvableTool(tool)) { @@ -3046,16 +3050,10 @@ export class ModelResult< allowBlock: true, detector: 'tool-fingerprint', toolCall, - }).then((decision) => - decision.action === 'block' || decision.action === 'stop' - ? { - blocked: true as const, - reason: decision.message ?? 'Blocked by doom loop', - } - : { - blocked: false as const, - }, - ) + }).then((decision) => ({ + blocked: decision.action === 'block' || decision.action === 'stop', + reason: decision.message ?? 'Blocked by doom loop', + })) : await this.checkDoomLoopBeforeExecution(tool, toolCall); if (doomOutcome.blocked) { this.preparedToolCalls.set(toolCall.id, { @@ -3076,24 +3074,25 @@ export class ModelResult< } unseenCalls.push(toolCall); } + return { + unseenCalls, + blockedCalls, + }; + } - if (unseenCalls.length === 0 && blockedCalls.length === 0) { - return false; - } - - const { requiresApproval: needsApproval, autoExecute } = await partitionToolCalls( - unseenCalls as ParsedToolCall[], - this.options.tools, - turnContext, - this.requireApprovalFn ?? undefined, - ); - - // Run the PermissionRequest hook for each tool that needs approval. - // This lets hooks short-circuit the approval flow in either direction: - // 'allow' promotes the call past the gate (executed once by the normal - // round), 'deny' synthesizes a rejection (recorded so the round emits a - // rejected output instead of executing), 'ask_user' falls through to the - // human approval flow. + private async resolveApprovalPhases( + needsApproval: ParsedToolCall[], + autoExecute: ParsedToolCall[], + turnContext: TurnContext, + responseKey: string, + ): Promise<{ + denied: { + tc: ParsedToolCall; + reason: string; + }[]; + stillPending: ParsedToolCall[]; + initialSurvivors: ParsedToolCall[]; + }> { const denied: { tc: ParsedToolCall; reason: string; @@ -3103,14 +3102,12 @@ export class ModelResult< if (this.hooksManager) { for (const tc of needsApproval) { const { decision, reason } = await this.emitPermissionRequest(tc as ParsedToolCall); - if (decision === 'allow') { - // Promoted past the gate; the normal tool round executes it once. - } else if (decision === 'deny') { + if (decision === 'deny') { denied.push({ tc, reason: reason ?? 'Denied by PermissionRequest hook', }); - } else { + } else if (decision !== 'allow') { stillPending.push(tc); } } @@ -3123,12 +3120,11 @@ export class ModelResult< ...needsApproval.filter( (tc) => !stillPending.some((pending) => pending.id === tc.id) && - !denied.some((d) => d.tc.id === tc.id), + !denied.some((entry) => entry.tc.id === tc.id), ), ] as ParsedToolCall[]; - const secondPending: ParsedToolCall[] = []; for (const toolCall of initialSurvivors) { - const tool = this.options.tools.find( + const tool = this.options.tools?.find( (candidate) => isClientTool(candidate) && candidate.function.name === toolCall.name, ); if (!tool || !isAutoResolvableTool(tool)) { @@ -3139,11 +3135,63 @@ export class ModelResult< ) { const prepared = this.preparedToolCalls.get(toolCall.id); if (prepared?.type === 'ready') { - secondPending.push(prepared.toolCall); + stillPending.push(prepared.toolCall as ParsedToolCall); } } } - stillPending.push(...secondPending); + return { + denied, + stillPending, + initialSurvivors, + }; + } + + /** + * Check for tools requiring approval and handle accordingly. + * Partitions tool calls into those needing approval and those that can auto-execute. + * + * @param toolCalls - The tool calls to check + * @param currentRound - The current execution round (1-indexed) + * @param currentResponse - The current response to save if pausing + * @returns True if execution should pause for approval, false to continue + * @throws Error if approval is required but no state accessor is configured + */ + private async handleApprovalCheck( + toolCalls: ParsedToolCall[], + currentRound: number, + currentResponse: models.OpenResponsesResult, + ): Promise { + if (!this.options.tools) { + return false; + } + + const turnContext: TurnContext = { + numberOfTurns: currentRound, + }; + const responseKey = this.approvalResponseKey(currentResponse); + + const { unseenCalls, blockedCalls } = await this.classifyInitialApprovalCalls( + toolCalls, + responseKey, + ); + + if (unseenCalls.length === 0 && blockedCalls.length === 0) { + return false; + } + + const { requiresApproval: needsApproval, autoExecute } = await partitionToolCalls( + unseenCalls as ParsedToolCall[], + this.options.tools, + turnContext, + this.requireApprovalFn ?? undefined, + ); + + const { denied, stillPending, initialSurvivors } = await this.resolveApprovalPhases( + needsApproval, + autoExecute, + turnContext, + responseKey, + ); if (stillPending.length === 0) { for (const d of denied) { From 84feaca0132d37bdf6a04e7cffa088904f7513a3 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:22:41 -0500 Subject: [PATCH 11/14] fix(agent): harden mutated approval preparation --- packages/agent/src/lib/model-result.ts | 57 ++-- .../unit/approval-gate-regressions.test.ts | 243 ++++++++++++++++++ 2 files changed, 262 insertions(+), 38 deletions(-) diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index e53160bc..3875036b 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -666,6 +666,7 @@ export class ModelResult< private readonly completedApprovalGates = new Set(); private readonly approvalResponseOccurrences = new WeakMap(); private nextApprovalResponseOccurrence = 0; + private nextApprovalGateFallback = 0; // Telemetry for the PostModelCall hook: the initial/resume request is // dispatched in initStream but its response is materialized later (stream // consumption), so the dispatch time and turn labeling are parked here @@ -2659,7 +2660,12 @@ export class ModelResult< phase: 'initial' | 'mutated', responseKey: string, ): string { - return `${phase}:${responseKey}:${toolCall.id}:${canonicalizeKeyMaterial(toolCall.arguments)}`; + const scope = `${phase}:${responseKey}:${toolCall.id}`; + try { + return `${scope}:${canonicalizeKeyMaterial(toolCall.arguments)}`; + } catch { + return `${scope}:uncanonicalizable:${this.nextApprovalGateFallback++}`; + } } /** Re-check only approval sources whose answer can depend on input. */ @@ -2722,22 +2728,14 @@ export class ModelResult< await this.emitPreparedFailure(toolCall, reason); } - private async normalizePreparedMutation(effective: ParsedToolCall): Promise< - | { - status: 'ready' | 'blocked'; - } - | { - status: 'normalized'; - toolCall: ParsedToolCall; - } - > { + private async validatePreparedMutation( + effective: ParsedToolCall, + ): Promise<'ready' | 'blocked'> { const tool = this.options.tools?.find( (candidate) => isClientTool(candidate) && candidate.function.name === effective.name, ); if (!tool || !isClientTool(tool)) { - return { - status: 'ready', - }; + return 'ready'; } const parsed = z4.safeParse(tool.function.inputSchema, effective.arguments); if (!parsed.success || !isRecord(parsed.data)) { @@ -2745,24 +2743,9 @@ export class ModelResult< effective, `PreToolUse produced invalid input for "${effective.name}"`, ); - return { - status: 'blocked', - }; + return 'blocked'; } - - const normalized = { - ...effective, - arguments: parsed.data, - } as ParsedToolCall; - this.preparedToolCalls.set(effective.id, { - type: 'ready', - toolCall: normalized, - mutated: true, - }); - return { - status: 'normalized', - toolCall: normalized, - }; + return 'ready'; } private async prepareAfterInitialApproval( @@ -2776,26 +2759,24 @@ export class ModelResult< return prepared?.type === 'blocked' ? 'blocked' : 'ready'; } - const mutation = await this.normalizePreparedMutation(effective); - if (mutation.status !== 'normalized') { - return mutation.status; + if ((await this.validatePreparedMutation(effective)) === 'blocked') { + return 'blocked'; } - const normalized = mutation.toolCall; - const key = this.approvalGateKey(normalized, 'mutated', responseKey); + const key = this.approvalGateKey(effective, 'mutated', responseKey); if (this.completedApprovalGates.has(key)) { return 'ready'; } this.completedApprovalGates.add(key); - if (!(await this.mutatedInputRequiresApproval(normalized, context))) { + if (!(await this.mutatedInputRequiresApproval(effective, context))) { return 'ready'; } - const { decision, reason } = await this.emitPermissionRequest(normalized); + const { decision, reason } = await this.emitPermissionRequest(effective); if (decision === 'allow') { return 'ready'; } if (decision === 'deny') { - await this.blockPreparedToolCall(normalized, reason ?? 'Denied by PermissionRequest hook'); + await this.blockPreparedToolCall(effective, reason ?? 'Denied by PermissionRequest hook'); return 'blocked'; } return 'pending'; diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 3bbe2656..47c5797d 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -529,6 +529,111 @@ describe('approval uses the post-PreToolUse arguments that execute would receive ); }); + it('does not abort approval bookkeeping for circular hook mutations', async () => { + const circular: Record = {}; + circular['self'] = circular; + const predicate = vi.fn(() => false); + const execute = vi.fn(async () => ({ + ok: true, + })); + const guarded = tool({ + name: 'circular_mutation', + inputSchema: z.object({ + self: z.unknown(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: predicate, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: () => ({ + mutatedInput: circular, + }), + }); + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_circular_mutation', [ + makeFunctionCallItem('call_circular_mutation', 'circular_mutation', '{}'), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_circular_done', []), + }); + + await expect( + new ModelResult({ + request: { + model: 'test-model', + input: 'run', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + hooks, + }).getResponse(), + ).resolves.toBeDefined(); + expect(predicate).toHaveBeenCalledWith(circular, { + numberOfTurns: 0, + }); + expect(execute).not.toHaveBeenCalled(); + }); + + it('lets normal validation handle deeply nested model input when keying cannot', async () => { + let deep: Record = {}; + for (let index = 0; index < 200; index++) { + deep = { + child: deep, + }; + } + const execute = vi.fn(async () => ({ + ok: true, + })); + const strict = tool({ + name: 'deep_invalid', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: () => false, + execute, + }); + const tools = [ + strict, + ] as const; + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_deep_invalid', [ + makeFunctionCallItem('call_deep_invalid', 'deep_invalid', JSON.stringify(deep)), + ]), + }); + const { accessor, get } = createMemoryAccessor(); + + await expect( + new ModelResult({ + request: { + model: 'test-model', + input: 'run', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + state: accessor, + }).getResponse(), + ).resolves.toBeDefined(); + expect(get()?.status).toBe('awaiting_approval'); + expect(execute).not.toHaveBeenCalled(); + }); + it('gates identical post-hook calls independently across responses', async () => { const predicate = vi.fn((params: { dangerous: boolean }) => params.dangerous); const execute = vi.fn(async () => ({ @@ -623,6 +728,144 @@ describe('approval uses the post-PreToolUse arguments that execute would receive expect(execute).toHaveBeenCalledTimes(2); }); + it('keeps hook-mutated arguments raw across approval and execution transforms', async () => { + const predicate = vi.fn((params: { value: string }) => params.value === 'hook:normalized'); + const execute = vi.fn(async (input: { value: string }) => ({ + value: input.value, + })); + const guarded = tool({ + name: 'transformed_mutation', + inputSchema: z.object({ + value: z.string().transform((value) => `${value}:normalized`), + }), + outputSchema: z.object({ + value: z.string(), + }), + requireApproval: predicate, + execute, + }); + const tools = [ + guarded, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: () => ({ + mutatedInput: { + value: 'hook', + }, + }), + }); + mockBetaResponsesSend.mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_transformed_mutation', [ + makeFunctionCallItem( + 'call_transformed_mutation', + 'transformed_mutation', + JSON.stringify({ + value: 'wire', + }), + ), + ]), + }); + const { accessor, get } = createMemoryAccessor(); + + await new ModelResult({ + request: { + model: 'test-model', + input: 'run', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + }).getResponse(); + + expect(predicate).toHaveBeenLastCalledWith( + { + value: 'hook:normalized', + }, + { + numberOfTurns: 0, + }, + ); + expect(execute).not.toHaveBeenCalled(); + expect(get()?.pendingToolCalls).toEqual([ + { + id: 'call_transformed_mutation', + name: 'transformed_mutation', + arguments: { + value: 'hook', + }, + preToolUseApplied: true, + }, + ]); + }); + + it('executes a type-changing hook mutation from raw input exactly once', async () => { + const execute = vi.fn(async (input: { value: number }) => ({ + value: input.value, + })); + const transformed = tool({ + name: 'type_changing_mutation', + inputSchema: z.object({ + value: z.string().transform((value) => value.length), + }), + outputSchema: z.object({ + value: z.number(), + }), + requireApproval: () => false, + execute, + }); + const tools = [ + transformed, + ] as const; + const hooks = new HooksManager(); + hooks.on('PreToolUse', { + handler: () => ({ + mutatedInput: { + value: 'hook', + }, + }), + }); + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_type_changing_mutation', [ + makeFunctionCallItem( + 'call_type_changing_mutation', + 'type_changing_mutation', + JSON.stringify({ + value: 'wire', + }), + ), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_type_changing_done', []), + }); + + await new ModelResult({ + request: { + model: 'test-model', + input: 'run', + tools: [], + }, + client: {} as OpenRouterCore, + tools, + hooks, + }).getResponse(); + + expect(execute).toHaveBeenCalledOnce(); + expect(execute).toHaveBeenCalledWith( + { + value: 4, + }, + expect.anything(), + ); + }); + it('gates schema-invalid model arguments before running PreToolUse', async () => { const preToolUse = vi.fn(() => ({ mutatedInput: { From 0719be25aefbac203495185177374798d91351e7 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:33:32 -0500 Subject: [PATCH 12/14] fix(agent): prevent approval key collisions --- packages/agent/src/lib/model-result.ts | 19 +++++-- .../unit/approval-gate-regressions.test.ts | 51 +++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 3875036b..7a2622c4 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -2660,11 +2660,24 @@ export class ModelResult< phase: 'initial' | 'mutated', responseKey: string, ): string { - const scope = `${phase}:${responseKey}:${toolCall.id}`; try { - return `${scope}:${canonicalizeKeyMaterial(toolCall.arguments)}`; + return JSON.stringify([ + phase, + responseKey, + toolCall.id, + { + canonical: canonicalizeKeyMaterial(toolCall.arguments), + }, + ]); } catch { - return `${scope}:uncanonicalizable:${this.nextApprovalGateFallback++}`; + return JSON.stringify([ + phase, + responseKey, + toolCall.id, + { + uncanonicalizable: this.nextApprovalGateFallback++, + }, + ]); } } diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 47c5797d..fe31c614 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -634,6 +634,57 @@ describe('approval uses the post-PreToolUse arguments that execute would receive expect(execute).not.toHaveBeenCalled(); }); + it('gates delimiter-colliding canonical and fallback call IDs independently', () => { + let deep: Record = {}; + for (let index = 0; index < 200; index++) { + deep = { + child: deep, + }; + } + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'run both', + }, + client: {} as OpenRouterCore, + }); + const approvalGateKey = ( + result as unknown as { + approvalGateKey: ( + toolCall: { + id: string; + name: string; + arguments: unknown; + }, + phase: 'initial' | 'mutated', + responseKey: string, + ) => string; + } + ).approvalGateKey.bind(result); + const completed = new Set(); + const gate = (id: string, args: unknown) => { + const key = approvalGateKey( + { + id, + name: 'guarded', + arguments: args, + }, + 'initial', + 'response:0', + ); + if (completed.has(key)) { + return false; + } + completed.add(key); + return true; + }; + + expect(gate('a:uncanonicalizable', 0)).toBe(true); + expect(gate('a:uncanonicalizable', 0)).toBe(false); + expect(gate('a', deep)).toBe(true); + expect(gate('a', deep)).toBe(true); + }); + it('gates identical post-hook calls independently across responses', async () => { const predicate = vi.fn((params: { dangerous: boolean }) => params.dangerous); const execute = vi.fn(async () => ({ From 292f9da46a4ba0176c96b60223832ea2b01bb3d2 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:52:30 -0500 Subject: [PATCH 13/14] fix(agent): distinguish duplicate approval calls --- .changeset/approval-gate-fixes.md | 25 ++- packages/agent/src/lib/model-result.ts | 82 ++++++--- .../unit/approval-gate-regressions.test.ts | 160 ++++++++++++++++++ 3 files changed, 247 insertions(+), 20 deletions(-) diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md index a617e6e7..985eca7e 100644 --- a/.changeset/approval-gate-fixes.md +++ b/.changeset/approval-gate-fixes.md @@ -8,4 +8,27 @@ Fix two ways the tool-approval gate could be bypassed. **Function-based `requireApproval` received unvalidated arguments.** Tool-level and call-level predicates were called with the raw JSON-parsed tool arguments, while `execute` receives the arguments *after* the tool's Zod `inputSchema` runs. Any default, coercion, or transform in the schema made them disagree — e.g. with `inputSchema: z.object({ dangerous: z.boolean().default(true) })`, a model emitting `{}` showed a predicate `dangerous: undefined` (no approval required) and then executed with `dangerous: true`. Predicates now see a parsed copy, so they decide on exactly what `execute` will receive without mutating the original executable call or parsing transformed output a second time. `PreToolUse` now runs before every auto-resolvable call is partitioned, so approval hooks and persisted pending calls see its effective arguments. Pending calls record an additive marker when preparation ran, preventing a resumed `ModelResult` from applying the hook twice while legacy state without the marker retains its prior behavior. Call-level checks remain unconditional and receive raw arguments when parsing fails; tool-level checks fail closed when schema parsing fails because a hook may later repair the input. -**Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each response is now gated at most once per run. +**Duplicate approval prompts for the same tool call.** The approval gate could run more than once over the same response — e.g. the pre-loop check plus the post-loop `allowFinalResponse` gate when a stop condition fired on the first iteration — re-emitting the `PermissionRequest` hook and re-running `requireApproval` predicates for calls that were already resolved. Each call occurrence in a response is now gated at most once per run, including responses containing duplicate call IDs and arguments. + +```ts +import { z } from 'zod/v4'; +import { tool, type PendingToolCall } from '@openrouter/agent'; + +const deploy = tool({ + name: 'deploy', + inputSchema: z.object({ + environment: z.enum(['staging', 'production']).default('production'), + }), + requireApproval: ({ environment }) => environment === 'production', + execute: async ({ environment }) => deployEnvironment(environment), +}); + +// `requireApproval` sees the normalized default: { environment: 'production' }. +// Persist this additive marker when PreToolUse already produced effective args. +const pending: PendingToolCall = { + id: 'call_deploy', + name: 'deploy', + arguments: { environment: 'production' }, + preToolUseApplied: true, +}; +``` diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 7a2622c4..2a2ec820 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -661,8 +661,9 @@ export class ModelResult< output: models.FunctionCallOutputItem; } >(); - // Approval is idempotent per response occurrence, call id, argument state, - // and phase. This distinguishes a newly emitted call that reuses an id. + // Approval is idempotent per response occurrence, call identity, arguments, + // duplicate occurrence, and phase. This distinguishes both reused ids and + // identical duplicate calls within one response. private readonly completedApprovalGates = new Set(); private readonly approvalResponseOccurrences = new WeakMap(); private nextApprovalResponseOccurrence = 0; @@ -2655,25 +2656,17 @@ export class ModelResult< return `${response.id}:${occurrence}`; } - private approvalGateKey( - toolCall: ParsedToolCall, - phase: 'initial' | 'mutated', - responseKey: string, - ): string { + private approvalCallIdentity(toolCall: ParsedToolCall): string { try { return JSON.stringify([ - phase, - responseKey, toolCall.id, - { - canonical: canonicalizeKeyMaterial(toolCall.arguments), - }, + toolCall.name, + canonicalizeKeyMaterial(toolCall.arguments), ]); } catch { return JSON.stringify([ - phase, - responseKey, toolCall.id, + toolCall.name, { uncanonicalizable: this.nextApprovalGateFallback++, }, @@ -2681,6 +2674,34 @@ export class ModelResult< } } + private approvalGateKey( + toolCall: ParsedToolCall, + phase: 'initial' | 'mutated', + responseKey: string, + occurrence: number, + ): string { + return JSON.stringify([ + phase, + responseKey, + this.approvalCallIdentity(toolCall), + occurrence, + ]); + } + + private approvalCallOccurrences( + toolCalls: ParsedToolCall[], + ): Map, number> { + const counts = new Map(); + const occurrences = new Map, number>(); + for (const toolCall of toolCalls) { + const identity = this.approvalCallIdentity(toolCall); + const occurrence = counts.get(identity) ?? 0; + counts.set(identity, occurrence + 1); + occurrences.set(toolCall, occurrence); + } + return occurrences; + } + /** Re-check only approval sources whose answer can depend on input. */ private async mutatedInputRequiresApproval( toolCall: ParsedToolCall, @@ -2765,6 +2786,7 @@ export class ModelResult< toolCall: ParsedToolCall, context: TurnContext, responseKey: string, + occurrence: number, ): Promise<'ready' | 'blocked' | 'pending'> { const effective = await this.prepareToolCallForApproval(toolCall); const prepared = this.preparedToolCalls.get(toolCall.id); @@ -2775,7 +2797,7 @@ export class ModelResult< if ((await this.validatePreparedMutation(effective)) === 'blocked') { return 'blocked'; } - const key = this.approvalGateKey(effective, 'mutated', responseKey); + const key = this.approvalGateKey(effective, 'mutated', responseKey, occurrence); if (this.completedApprovalGates.has(key)) { return 'ready'; } @@ -3012,6 +3034,7 @@ export class ModelResult< private async classifyInitialApprovalCalls( toolCalls: ParsedToolCall[], responseKey: string, + occurrences: Map, number>, ): Promise<{ unseenCalls: ParsedToolCall[]; blockedCalls: ParsedToolCall[]; @@ -3020,12 +3043,19 @@ export class ModelResult< const blockedCalls: ParsedToolCall[] = []; const unseenKeys = toolCalls.filter( (toolCall) => - !this.completedApprovalGates.has(this.approvalGateKey(toolCall, 'initial', responseKey)), + !this.completedApprovalGates.has( + this.approvalGateKey(toolCall, 'initial', responseKey, occurrences.get(toolCall) ?? 0), + ), ); await this.beginDoomLoopRound(unseenKeys); for (const toolCall of toolCalls) { - const key = this.approvalGateKey(toolCall, 'initial', responseKey); + const key = this.approvalGateKey( + toolCall, + 'initial', + responseKey, + occurrences.get(toolCall) ?? 0, + ); if (this.completedApprovalGates.has(key)) { continue; } @@ -3079,6 +3109,7 @@ export class ModelResult< autoExecute: ParsedToolCall[], turnContext: TurnContext, responseKey: string, + occurrences: Map, number>, ): Promise<{ denied: { tc: ParsedToolCall; @@ -3125,7 +3156,12 @@ export class ModelResult< continue; } if ( - (await this.prepareAfterInitialApproval(toolCall, turnContext, responseKey)) === 'pending' + (await this.prepareAfterInitialApproval( + toolCall, + turnContext, + responseKey, + occurrences.get(toolCall) ?? 0, + )) === 'pending' ) { const prepared = this.preparedToolCalls.get(toolCall.id); if (prepared?.type === 'ready') { @@ -3151,7 +3187,7 @@ export class ModelResult< * @throws Error if approval is required but no state accessor is configured */ private async handleApprovalCheck( - toolCalls: ParsedToolCall[], + suppliedToolCalls: ParsedToolCall[], currentRound: number, currentResponse: models.OpenResponsesResult, ): Promise { @@ -3163,10 +3199,16 @@ export class ModelResult< numberOfTurns: currentRound, }; const responseKey = this.approvalResponseKey(currentResponse); + // Always enumerate the complete response. A caller-provided subset or a + // later output reorder must not change duplicate occurrence identities. + const responseToolCalls = extractToolCallsFromResponse(currentResponse); + const toolCalls = responseToolCalls.length > 0 ? responseToolCalls : suppliedToolCalls; + const occurrences = this.approvalCallOccurrences(toolCalls); const { unseenCalls, blockedCalls } = await this.classifyInitialApprovalCalls( toolCalls, responseKey, + occurrences, ); if (unseenCalls.length === 0 && blockedCalls.length === 0) { @@ -3185,6 +3227,7 @@ export class ModelResult< autoExecute, turnContext, responseKey, + occurrences, ); if (stillPending.length === 0) { @@ -5876,6 +5919,7 @@ export class ModelResult< toolCall as ParsedToolCall, turnContext, `persisted:${this.currentState.previousResponseId ?? 'unknown'}`, + 0, ); if (prepared === 'pending') { const ready = this.preparedToolCalls.get(callId); diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index fe31c614..6cd42e55 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -1719,6 +1719,166 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { expect(mockBetaResponsesSend).toHaveBeenCalledTimes(1); }); + it('gates duplicate identical calls separately but only once across repeated response visits', async () => { + const dangerExecute = vi.fn(async () => ({ + ok: true, + })); + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + target: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute: dangerExecute, + }); + const tools = [ + danger, + ] as const; + const hooks = new HooksManager(); + const permissionHandler = vi.fn(() => ({ + decision: 'allow' as const, + })); + hooks.on('PermissionRequest', { + handler: permissionHandler, + }); + + mockBetaResponsesSend + .mockResolvedValueOnce({ + ok: true, + value: makeResponse('resp_duplicates', [ + makeFunctionCallItem( + 'call_duplicate', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + makeFunctionCallItem( + 'call_duplicate', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ]), + }) + .mockResolvedValue({ + ok: true, + value: makeResponse('resp_final', [ + { + id: 'msg_final', + type: 'message', + role: 'assistant', + status: 'completed', + content: [ + { + type: 'output_text', + text: 'Done.', + annotations: [], + }, + ], + }, + ]), + }); + + const { accessor } = createMemoryAccessor(); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do both things', + tools: [ + { + type: 'function', + name: 'danger', + description: null, + strict: null, + parameters: {}, + }, + ], + }, + client: {} as OpenRouterCore, + tools, + hooks, + state: accessor, + stopWhen: stepCountIs(0), + allowFinalResponse: true, + }); + + await result.getText(); + + expect(permissionHandler).toHaveBeenCalledTimes(2); + expect(dangerExecute).toHaveBeenCalledTimes(2); + }); + + it('derives duplicate occurrences from the full response when passed a subset', async () => { + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + target: z.string(), + }), + requireApproval: true, + execute: async () => ({}), + }); + const tools = [ + danger, + ] as const; + const hooks = new HooksManager(); + const permissionHandler = vi.fn(() => ({ + decision: 'allow' as const, + })); + hooks.on('PermissionRequest', { + handler: permissionHandler, + }); + const response = makeResponse('resp_grows', [ + makeFunctionCallItem( + 'call_duplicate', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ]); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the thing', + }, + client: {} as OpenRouterCore, + tools, + hooks, + }); + const handleApprovalCheck = ( + result as unknown as { + handleApprovalCheck: ( + calls: Array<{ + id: string; + name: string; + arguments: unknown; + }>, + round: number, + response: models.OpenResponsesResult, + ) => Promise; + } + ).handleApprovalCheck.bind(result); + + await handleApprovalCheck([], 0, response); + response.output.push( + makeFunctionCallItem( + 'call_duplicate', + 'danger', + JSON.stringify({ + target: 'prod', + }), + ), + ); + await handleApprovalCheck([], 0, response); + + expect(permissionHandler).toHaveBeenCalledTimes(2); + }); + it('gates the initial response only once when the stop condition fires on the first iteration', async () => { const dangerExecute = vi.fn(async () => ({ ok: true, From 25ec586cb01f471215aba18ec08dd90ecc258559 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:17:22 -0500 Subject: [PATCH 14/14] fix(agent): stabilize uncanonicalizable approval identities --- packages/agent/src/lib/model-result.ts | 79 +++++------- .../unit/approval-gate-regressions.test.ts | 113 ++++++++++-------- 2 files changed, 91 insertions(+), 101 deletions(-) diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index 2a2ec820..e728b3d5 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -661,13 +661,13 @@ export class ModelResult< output: models.FunctionCallOutputItem; } >(); - // Approval is idempotent per response occurrence, call identity, arguments, - // duplicate occurrence, and phase. This distinguishes both reused ids and - // identical duplicate calls within one response. + // Approval is idempotent per response object, tool-call occurrence, and phase. + // Occurrence tokens avoid content canonicalization and distinguish duplicates. private readonly completedApprovalGates = new Set(); private readonly approvalResponseOccurrences = new WeakMap(); + private readonly approvalCallOccurrences = new WeakMap(); private nextApprovalResponseOccurrence = 0; - private nextApprovalGateFallback = 0; + private nextApprovalCallOccurrence = 0; // Telemetry for the PostModelCall hook: the initial/resume request is // dispatched in initStream but its response is materialized later (stream // consumption), so the dispatch time and turn labeling are parked here @@ -2656,48 +2656,32 @@ export class ModelResult< return `${response.id}:${occurrence}`; } - private approvalCallIdentity(toolCall: ParsedToolCall): string { - try { - return JSON.stringify([ - toolCall.id, - toolCall.name, - canonicalizeKeyMaterial(toolCall.arguments), - ]); - } catch { - return JSON.stringify([ - toolCall.id, - toolCall.name, - { - uncanonicalizable: this.nextApprovalGateFallback++, - }, - ]); - } - } - private approvalGateKey( - toolCall: ParsedToolCall, phase: 'initial' | 'mutated', responseKey: string, - occurrence: number, + occurrence: string, ): string { return JSON.stringify([ phase, responseKey, - this.approvalCallIdentity(toolCall), occurrence, ]); } - private approvalCallOccurrences( + private assignApprovalCallOccurrences( + response: models.OpenResponsesResult, toolCalls: ParsedToolCall[], - ): Map, number> { - const counts = new Map(); - const occurrences = new Map, number>(); - for (const toolCall of toolCalls) { - const identity = this.approvalCallIdentity(toolCall); - const occurrence = counts.get(identity) ?? 0; - counts.set(identity, occurrence + 1); - occurrences.set(toolCall, occurrence); + ): Map, string> { + let identities = this.approvalCallOccurrences.get(response); + if (!identities) { + identities = []; + this.approvalCallOccurrences.set(response, identities); + } + const occurrences = new Map, string>(); + for (const [index, toolCall] of toolCalls.entries()) { + const identity = identities[index] ?? `call:${this.nextApprovalCallOccurrence++}`; + identities[index] = identity; + occurrences.set(toolCall, identity); } return occurrences; } @@ -2786,7 +2770,7 @@ export class ModelResult< toolCall: ParsedToolCall, context: TurnContext, responseKey: string, - occurrence: number, + occurrence: string, ): Promise<'ready' | 'blocked' | 'pending'> { const effective = await this.prepareToolCallForApproval(toolCall); const prepared = this.preparedToolCalls.get(toolCall.id); @@ -2797,7 +2781,7 @@ export class ModelResult< if ((await this.validatePreparedMutation(effective)) === 'blocked') { return 'blocked'; } - const key = this.approvalGateKey(effective, 'mutated', responseKey, occurrence); + const key = this.approvalGateKey('mutated', responseKey, occurrence); if (this.completedApprovalGates.has(key)) { return 'ready'; } @@ -3034,7 +3018,7 @@ export class ModelResult< private async classifyInitialApprovalCalls( toolCalls: ParsedToolCall[], responseKey: string, - occurrences: Map, number>, + occurrences: Map, string>, ): Promise<{ unseenCalls: ParsedToolCall[]; blockedCalls: ParsedToolCall[]; @@ -3044,18 +3028,13 @@ export class ModelResult< const unseenKeys = toolCalls.filter( (toolCall) => !this.completedApprovalGates.has( - this.approvalGateKey(toolCall, 'initial', responseKey, occurrences.get(toolCall) ?? 0), + this.approvalGateKey('initial', responseKey, occurrences.get(toolCall) ?? ''), ), ); await this.beginDoomLoopRound(unseenKeys); for (const toolCall of toolCalls) { - const key = this.approvalGateKey( - toolCall, - 'initial', - responseKey, - occurrences.get(toolCall) ?? 0, - ); + const key = this.approvalGateKey('initial', responseKey, occurrences.get(toolCall) ?? ''); if (this.completedApprovalGates.has(key)) { continue; } @@ -3109,7 +3088,7 @@ export class ModelResult< autoExecute: ParsedToolCall[], turnContext: TurnContext, responseKey: string, - occurrences: Map, number>, + occurrences: Map, string>, ): Promise<{ denied: { tc: ParsedToolCall; @@ -3160,7 +3139,7 @@ export class ModelResult< toolCall, turnContext, responseKey, - occurrences.get(toolCall) ?? 0, + occurrences.get(toolCall) ?? '', )) === 'pending' ) { const prepared = this.preparedToolCalls.get(toolCall.id); @@ -3199,11 +3178,11 @@ export class ModelResult< numberOfTurns: currentRound, }; const responseKey = this.approvalResponseKey(currentResponse); - // Always enumerate the complete response. A caller-provided subset or a - // later output reorder must not change duplicate occurrence identities. + // Always enumerate the complete response so subset visits find the original + // response-local occurrence and appended calls receive fresh identities. const responseToolCalls = extractToolCallsFromResponse(currentResponse); const toolCalls = responseToolCalls.length > 0 ? responseToolCalls : suppliedToolCalls; - const occurrences = this.approvalCallOccurrences(toolCalls); + const occurrences = this.assignApprovalCallOccurrences(currentResponse, toolCalls); const { unseenCalls, blockedCalls } = await this.classifyInitialApprovalCalls( toolCalls, @@ -5919,7 +5898,7 @@ export class ModelResult< toolCall as ParsedToolCall, turnContext, `persisted:${this.currentState.previousResponseId ?? 'unknown'}`, - 0, + callId, ); if (prepared === 'pending') { const ready = this.preparedToolCalls.get(callId); diff --git a/packages/agent/tests/unit/approval-gate-regressions.test.ts b/packages/agent/tests/unit/approval-gate-regressions.test.ts index 6cd42e55..76ae9ca7 100644 --- a/packages/agent/tests/unit/approval-gate-regressions.test.ts +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -634,57 +634,6 @@ describe('approval uses the post-PreToolUse arguments that execute would receive expect(execute).not.toHaveBeenCalled(); }); - it('gates delimiter-colliding canonical and fallback call IDs independently', () => { - let deep: Record = {}; - for (let index = 0; index < 200; index++) { - deep = { - child: deep, - }; - } - const result = new ModelResult({ - request: { - model: 'test-model', - input: 'run both', - }, - client: {} as OpenRouterCore, - }); - const approvalGateKey = ( - result as unknown as { - approvalGateKey: ( - toolCall: { - id: string; - name: string; - arguments: unknown; - }, - phase: 'initial' | 'mutated', - responseKey: string, - ) => string; - } - ).approvalGateKey.bind(result); - const completed = new Set(); - const gate = (id: string, args: unknown) => { - const key = approvalGateKey( - { - id, - name: 'guarded', - arguments: args, - }, - 'initial', - 'response:0', - ); - if (completed.has(key)) { - return false; - } - completed.add(key); - return true; - }; - - expect(gate('a:uncanonicalizable', 0)).toBe(true); - expect(gate('a:uncanonicalizable', 0)).toBe(false); - expect(gate('a', deep)).toBe(true); - expect(gate('a', deep)).toBe(true); - }); - it('gates identical post-hook calls independently across responses', async () => { const predicate = vi.fn((params: { dangerous: boolean }) => params.dangerous); const execute = vi.fn(async () => ({ @@ -1813,6 +1762,68 @@ describe('allowFinalResponse path enforces the approval gate (#54)', () => { expect(dangerExecute).toHaveBeenCalledTimes(2); }); + it('reuses opaque identities for uncanonicalizable response call occurrences', async () => { + let deep: Record = {}; + for (let index = 0; index < 200; index++) { + deep = { + child: deep, + }; + } + const danger = tool({ + name: 'danger', + inputSchema: z.object({ + child: z.unknown(), + }), + requireApproval: true, + execute: async () => ({}), + }); + const tools = [ + danger, + ] as const; + const hooks = new HooksManager(); + const permissionHandler = vi.fn(() => ({ + decision: 'allow' as const, + })); + hooks.on('PermissionRequest', { + handler: permissionHandler, + }); + const firstResponse = makeResponse('resp_deep_duplicates', [ + makeFunctionCallItem('call_deep', 'danger', JSON.stringify(deep)), + makeFunctionCallItem('call_deep', 'danger', JSON.stringify(deep)), + ]); + const laterResponse = makeResponse('resp_deep_later', [ + makeFunctionCallItem('call_deep', 'danger', JSON.stringify(deep)), + ]); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'do the thing', + }, + client: {} as OpenRouterCore, + tools, + hooks, + }); + const handleApprovalCheck = ( + result as unknown as { + handleApprovalCheck: ( + calls: Array<{ + id: string; + name: string; + arguments: unknown; + }>, + round: number, + response: models.OpenResponsesResult, + ) => Promise; + } + ).handleApprovalCheck.bind(result); + + await handleApprovalCheck([], 0, firstResponse); + await handleApprovalCheck([], 0, firstResponse); + await handleApprovalCheck([], 0, laterResponse); + + expect(permissionHandler).toHaveBeenCalledTimes(3); + }); + it('derives duplicate occurrences from the full response when passed a subset', async () => { const danger = tool({ name: 'danger',