diff --git a/.changeset/approval-gate-fixes.md b/.changeset/approval-gate-fixes.md new file mode 100644 index 0000000..985eca7 --- /dev/null +++ b/.changeset/approval-gate-fixes.md @@ -0,0 +1,34 @@ +--- +'@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.** 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 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/index.ts b/packages/agent/src/index.ts index 3b61266..5f47406 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/async-params.ts b/packages/agent/src/lib/async-params.ts index 9cabd7c..6b724b2 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 d42a402..c02026c 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, @@ -260,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, @@ -271,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, @@ -287,6 +287,28 @@ export async function toolRequiresApproval( } > => isClientTool(t) && t.function.name === toolCall.name, ); + // 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); + } + + const parsed = z4.safeParse(tool.function.inputSchema, toolCall.arguments); + if (!parsed.success) { + return callLevelCheck(toolCall, context); + } + + 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; } @@ -294,18 +316,34 @@ 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. 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) { + // 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)) { + // 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(rawArgs, context); + return requireApproval(parsed.data, context); } // Otherwise treat as boolean diff --git a/packages/agent/src/lib/hooks-schemas.ts b/packages/agent/src/lib/hooks-schemas.ts index aa0b202..8087d04 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 e8d0775..e728b3d 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'; @@ -443,7 +445,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, @@ -645,6 +647,27 @@ export class ModelResult< // normal tool round consults this to synthesize rejected outputs instead of // executing the calls. private readonly hookDeniedCalls = new Map(); + // PreToolUse outcomes parked between the approval gates and execution. + private readonly preparedToolCalls = new Map< + string, + | { + type: 'ready'; + toolCall: ParsedToolCall; + mutated: boolean; + } + | { + type: 'blocked'; + reason: string; + output: models.FunctionCallOutputItem; + } + >(); + // 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 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 @@ -1388,6 +1411,7 @@ export class ModelResult< turnContext: TurnContext, onPreliminaryResult?: (toolCallId: string, result: unknown) => void, extras?: ToolExecutionExtras, + runPreToolUse = true, ): Promise< | { type: 'parse_error'; @@ -1416,23 +1440,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}". ` + @@ -1452,34 +1459,22 @@ 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) { + const prepared = this.preparedToolCalls.get(toolCall.id); + this.preparedToolCalls.delete(toolCall.id); + if (prepared?.type === '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, - }), - }, + reason: prepared.reason, + output: prepared.output, }; } - let effectiveToolCall = toolCall; + 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 @@ -2604,6 +2599,208 @@ 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, + 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( + phase: 'initial' | 'mutated', + responseKey: string, + occurrence: string, + ): string { + return JSON.stringify([ + phase, + responseKey, + occurrence, + ]); + } + + private assignApprovalCallOccurrences( + response: models.OpenResponsesResult, + toolCalls: ParsedToolCall[], + ): 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; + } + + /** 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 blockPreparedToolCall( + toolCall: ParsedToolCall, + 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 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 'ready'; + } + const parsed = z4.safeParse(tool.function.inputSchema, effective.arguments); + if (!parsed.success || !isRecord(parsed.data)) { + await this.blockPreparedToolCall( + effective, + `PreToolUse produced invalid input for "${effective.name}"`, + ); + return 'blocked'; + } + return 'ready'; + } + + private async prepareAfterInitialApproval( + toolCall: ParsedToolCall, + context: TurnContext, + responseKey: string, + occurrence: 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'; + } + + if ((await this.validatePreparedMutation(effective)) === 'blocked') { + return 'blocked'; + } + const key = this.approvalGateKey('mutated', responseKey, occurrence); + if (this.completedApprovalGates.has(key)) { + return 'ready'; + } + this.completedApprovalGates.add(key); + if (!(await this.mutatedInputRequiresApproval(effective, context))) { + return 'ready'; + } + + const { decision, reason } = await this.emitPermissionRequest(effective); + if (decision === 'allow') { + return 'ready'; + } + if (decision === 'deny') { + await this.blockPreparedToolCall(effective, reason ?? 'Denied by PermissionRequest hook'); + return 'blocked'; + } + return 'pending'; + } + /** * Run the UserPromptSubmit hook, supporting both string and structured * inputs. If a handler returns a mutated prompt, the returned object @@ -2748,9 +2945,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)) { @@ -2821,6 +3015,146 @@ export class ModelResult< return results; } + private async classifyInitialApprovalCalls( + toolCalls: ParsedToolCall[], + responseKey: string, + occurrences: Map, string>, + ): Promise<{ + unseenCalls: ParsedToolCall[]; + blockedCalls: ParsedToolCall[]; + }> { + const unseenCalls: ParsedToolCall[] = []; + const blockedCalls: ParsedToolCall[] = []; + const unseenKeys = toolCalls.filter( + (toolCall) => + !this.completedApprovalGates.has( + this.approvalGateKey('initial', responseKey, occurrences.get(toolCall) ?? ''), + ), + ); + await this.beginDoomLoopRound(unseenKeys); + + for (const toolCall of toolCalls) { + const key = this.approvalGateKey('initial', responseKey, occurrences.get(toolCall) ?? ''); + 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) => ({ + 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, { + 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); + } + return { + unseenCalls, + blockedCalls, + }; + } + + private async resolveApprovalPhases( + needsApproval: ParsedToolCall[], + autoExecute: ParsedToolCall[], + turnContext: TurnContext, + responseKey: string, + occurrences: Map, string>, + ): Promise<{ + denied: { + tc: ParsedToolCall; + reason: string; + }[]; + stillPending: ParsedToolCall[]; + initialSurvivors: ParsedToolCall[]; + }> { + const denied: { + tc: ParsedToolCall; + reason: string; + }[] = []; + const stillPending: ParsedToolCall[] = []; + + if (this.hooksManager) { + for (const tc of needsApproval) { + const { decision, reason } = await this.emitPermissionRequest(tc as ParsedToolCall); + if (decision === 'deny') { + denied.push({ + tc, + reason: reason ?? 'Denied by PermissionRequest hook', + }); + } else if (decision !== 'allow') { + stillPending.push(tc); + } + } + } else { + stillPending.push(...needsApproval); + } + + const initialSurvivors = [ + ...autoExecute, + ...needsApproval.filter( + (tc) => + !stillPending.some((pending) => pending.id === tc.id) && + !denied.some((entry) => entry.tc.id === tc.id), + ), + ] as 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, + occurrences.get(toolCall) ?? '', + )) === 'pending' + ) { + const prepared = this.preparedToolCalls.get(toolCall.id); + if (prepared?.type === 'ready') { + stillPending.push(prepared.toolCall as ParsedToolCall); + } + } + } + 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. @@ -2832,7 +3166,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 { @@ -2842,57 +3176,40 @@ export class ModelResult< const turnContext: TurnContext = { numberOfTurns: currentRound, - // context is handled via contextStore, not on TurnContext }; + const responseKey = this.approvalResponseKey(currentResponse); + // 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.assignApprovalCallOccurrences(currentResponse, toolCalls); + + const { unseenCalls, blockedCalls } = await this.classifyInitialApprovalCalls( + toolCalls, + responseKey, + occurrences, + ); + + if (unseenCalls.length === 0 && blockedCalls.length === 0) { + return false; + } const { requiresApproval: needsApproval, autoExecute } = await partitionToolCalls( - toolCalls as ParsedToolCall[], + unseenCalls as ParsedToolCall[], this.options.tools, turnContext, this.requireApprovalFn ?? undefined, ); - // 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: - // '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. - const denied: { - tc: ParsedToolCall; - reason: string; - }[] = []; - const stillPending: ParsedToolCall[] = []; - - 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') { - denied.push({ - tc, - reason: reason ?? 'Denied by PermissionRequest hook', - }); - } else { - stillPending.push(tc); - } - } - } else { - stillPending.push(...needsApproval); - } + const { denied, stillPending, initialSurvivors } = await this.resolveApprovalPhases( + needsApproval, + autoExecute, + turnContext, + responseKey, + occurrences, + ); 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); } @@ -2911,8 +3228,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( - autoExecute as ParsedToolCall[], + executableNow as ParsedToolCall[], turnContext, ); @@ -2928,7 +3250,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) { @@ -2936,6 +3263,8 @@ export class ModelResult< } await this.saveStateSafely(stateUpdates); + this.preparedToolCalls.clear(); + this.hookDeniedCalls.clear(); this.finalResponse = currentResponse; return true; // Pause for approval } @@ -3519,9 +3848,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), ); @@ -5541,23 +5867,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( - [ - ...this.approvedToolCalls, - ] - .map((callId) => pendingCalls.find((tc) => tc.id === callId)) - .filter((tc): tc is ParsedToolCall => tc !== undefined), - ); - // Process approvals - execute the approved tools. Route through // runToolWithHooks so PreToolUse/PostToolUse fire even on this path. for (const callId of this.approvedToolCalls) { @@ -5577,10 +5893,32 @@ export class ModelResult< continue; } + if (toolCall.preToolUseApplied !== true) { + const prepared = await this.prepareAfterInitialApproval( + toolCall as ParsedToolCall, + turnContext, + `persisted:${this.currentState.previousResponseId ?? 'unknown'}`, + callId, + ); + 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, + false, ); if (hookOutcome.type === 'parse_error') { @@ -5633,7 +5971,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 @@ -5643,7 +5987,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)); @@ -5698,6 +6042,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; } @@ -6132,6 +6478,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/src/lib/tool-types.ts b/packages/agent/src/lib/tool-types.ts index 0ea4140..8832008 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 new file mode 100644 index 0000000..76ae9ca --- /dev/null +++ b/packages/agent/tests/unit/approval-gate-regressions.test.ts @@ -0,0 +1,1995 @@ +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 { 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, + 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('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('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); + 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('fails closed when arguments fail schema validation', 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. 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', + arguments: {}, + }; + + const requires = await toolRequiresApproval( + invalidCall, + [ + strict, + ], + context, + ); + + expect(requires).toBe(true); + 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(); + }); + + 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(); + }); +}); + +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 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 () => ({ + 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('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: { + 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 () => { + 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', + }, + }), + expect.anything(), + ); + expect(get()?.pendingToolCalls).toEqual([ + { + id: 'call_unconditional', + name: 'always_guarded', + arguments: { + value: 'original', + }, + }, + ]); + 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(permissionRequest).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', + }, + expect.anything(), + ); + }); +}); + +// --------------------------------------------------------------------------- +// 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({ + 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 +// 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); + }); + + 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'); + }); + + it('fails closed on schema-invalid arguments that no hook repairs', 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. 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, + 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, + }); + + await result.getResponse(); + + const saved = get(); + expect(saved?.status).toBe('awaiting_approval'); + expect(saved?.pendingToolCalls).toEqual([ + { + id: 'call_strict', + name: 'strict', + arguments: {}, + }, + ]); + expect(execute).not.toHaveBeenCalled(); + 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('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', + 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, + })); + + 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.'); + }); +}); diff --git a/packages/agent/tests/unit/doom-loop-remediation.test.ts b/packages/agent/tests/unit/doom-loop-remediation.test.ts index 6ce0863..941c900 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({