diff --git a/.changeset/stop-when-before-approval.md b/.changeset/stop-when-before-approval.md new file mode 100644 index 00000000..d697ea5b --- /dev/null +++ b/.changeset/stop-when-before-approval.md @@ -0,0 +1,5 @@ +--- +"@openrouter/agent": patch +--- + +Security hardening: when `stopWhen` is already met on the first response and `allowFinalResponse` is `false`, pausing for an approval-gated tool call no longer executes its auto-approved sibling calls. They are saved as skipped results instead. diff --git a/packages/agent/src/lib/model-result.ts b/packages/agent/src/lib/model-result.ts index bbc4f047..bd0c1666 100644 --- a/packages/agent/src/lib/model-result.ts +++ b/packages/agent/src/lib/model-result.ts @@ -3240,6 +3240,8 @@ export class ModelResult< * @param toolCalls - The tool calls to check * @param currentRound - The current execution round (1-indexed) * @param currentResponse - The current response to save if pausing + * @param skipAutoExecution - When true, auto-approved calls are not executed; + * they are saved as skipped results so the paused turn stays well-formed * @returns True if execution should pause for approval, false to continue * @throws Error if approval is required but no state accessor is configured */ @@ -3247,6 +3249,7 @@ export class ModelResult< suppliedToolCalls: ParsedToolCall[], currentRound: number, currentResponse: models.OpenResponsesResult, + skipAutoExecution = false, ): Promise { if (!this.options.tools) { return false; @@ -3311,10 +3314,19 @@ export class ModelResult< ...initialSurvivors.filter((call) => !pendingIds.has(call.id)), ...blockedCalls, ]; - const unsentResults = await this.executeAutoApproveTools( - executableNow as ParsedToolCall[], - turnContext, - ); + const unsentResults: UnsentToolResult[] = skipAutoExecution + ? executableNow.map( + (call) => + createRejectedResult( + call.id, + String(call.name), + 'Tool execution skipped: stop condition met', + ) as UnsentToolResult, + ) + : await this.executeAutoApproveTools( + executableNow as ParsedToolCall[], + turnContext, + ); // Combine pre-denied results (from hook "deny") with executed results. const combinedResults: UnsentToolResult[] = [ @@ -6335,8 +6347,16 @@ export class ModelResult< // Extract and check tool calls const toolCalls = extractToolCallsFromResponse(currentResponse); + // With allowFinalResponse: false, a met stopWhen means no tool from + // this response runs (the post-loop stop path skips them). Evaluate it + // before the approval check so pausing for an approval-gated call does + // not execute its auto-approved siblings. With allowFinalResponse on, + // the stop path runs these calls anyway, so the check is not needed. + const skipAutoExecution = + this.options.allowFinalResponse === false && (await this.shouldStopExecution()); + // Check for approval requirements - if (await this.handleApprovalCheck(toolCalls, 0, currentResponse)) { + if (await this.handleApprovalCheck(toolCalls, 0, currentResponse, skipAutoExecution)) { return; // Paused for approval } diff --git a/packages/agent/tests/unit/stop-when-before-approval.test.ts b/packages/agent/tests/unit/stop-when-before-approval.test.ts new file mode 100644 index 00000000..ecf1ad9e --- /dev/null +++ b/packages/agent/tests/unit/stop-when-before-approval.test.ts @@ -0,0 +1,180 @@ +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 { tool } from '../../src/lib/tool.js'; +import type { ConversationState, StateAccessor, Tool } 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'); + +function makeResponse(output: models.OpenResponsesResult['output']): models.OpenResponsesResult { + return { + id: 'resp_1', + 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 functionCall(callId: string, name: string): models.OutputFunctionCallItem { + return { + type: 'function_call', + id: `fc_${callId}`, + callId, + name, + arguments: '{"v":"x"}', + status: 'completed', + }; +} + +function createMemoryAccessor() { + let state: ConversationState | null = null; + const accessor: StateAccessor = { + load: async () => state, + save: async (next) => { + state = next; + }, + }; + return { + accessor, + get: () => state, + }; +} + +function makeTools() { + const autoExecute = vi.fn(async () => ({ + ok: true, + })); + const gatedExecute = vi.fn(async () => ({ + ok: true, + })); + const auto = tool({ + name: 'auto', + inputSchema: z.object({ + v: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + execute: autoExecute, + }); + const gated = tool({ + name: 'gated', + inputSchema: z.object({ + v: z.string(), + }), + outputSchema: z.object({ + ok: z.boolean(), + }), + requireApproval: true, + execute: gatedExecute, + }); + return { + tools: [ + auto, + gated, + ] as const, + autoExecute, + gatedExecute, + }; +} + +describe('stopWhen is evaluated before the approval check on the first response', () => { + beforeEach(() => { + mockBetaResponsesSend.mockReset(); + mockBetaResponsesSend.mockResolvedValue({ + ok: true, + value: makeResponse([ + functionCall('call_auto', 'auto'), + functionCall('call_gated', 'gated'), + ]), + }); + }); + + it('does not run an auto-executable sibling of an approval-gated call when stopWhen is already met', async () => { + const { tools, autoExecute, gatedExecute } = makeTools(); + const memory = createMemoryAccessor(); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'go', + }, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + stopWhen: () => true, + allowFinalResponse: false, + }); + + await result.getResponse(); + + expect(autoExecute).not.toHaveBeenCalled(); + expect(gatedExecute).not.toHaveBeenCalled(); + expect(mockBetaResponsesSend).toHaveBeenCalledTimes(1); + + const saved = memory.get(); + expect(saved?.status).toBe('awaiting_approval'); + expect(saved?.pendingToolCalls?.map((tc) => tc.id)).toEqual([ + 'call_gated', + ]); + // The skipped sibling still gets a matching output for the resume request. + expect(saved?.unsentToolResults).toEqual([ + { + callId: 'call_auto', + name: 'auto', + output: null, + error: 'Tool execution skipped: stop condition met', + }, + ]); + expect(await result.requiresApproval()).toBe(true); + }); + + it('still runs the auto-executable sibling when stopWhen is not met', async () => { + const { tools, autoExecute } = makeTools(); + const memory = createMemoryAccessor(); + const result = new ModelResult({ + request: { + model: 'test-model', + input: 'go', + }, + client: {} as OpenRouterCore, + tools, + state: memory.accessor, + stopWhen: () => false, + allowFinalResponse: false, + }); + + await result.getResponse(); + + expect(autoExecute).toHaveBeenCalledTimes(1); + const saved = memory.get(); + expect(saved?.status).toBe('awaiting_approval'); + expect(saved?.pendingToolCalls?.map((tc) => tc.id)).toEqual([ + 'call_gated', + ]); + expect(saved?.unsentToolResults?.map((r) => r.callId)).toEqual([ + 'call_auto', + ]); + }); +});