fix: harden tool approval decisions and predicates

src/lib/conversation-state.ts

@@ -8,6 +8,7 @@ import type {
   TurnContext,
   UnsentToolResult,
 } from './tool-types.js';
+import * as z4 from 'zod/v4';
 import { normalizeInputToArray } from './turn-context.js';
 
 /**
@@ -110,26 +111,93 @@ export async function toolRequiresApproval<TTools extends readonly Tool[]>(
   context: TurnContext,
   callLevelCheck?: (toolCall: ParsedToolCall<TTools[number]>, context: TurnContext) => boolean | Promise<boolean>
 ): Promise<boolean> {
+  const tool = tools.find(t => t.function.name === toolCall.name);
+  // Predicates see the arguments the executor will run with (#520)
+  const args = tool ? normalizedArguments(toolCall.arguments, tool) : toolCall.arguments;
+
   // Call-level check takes precedence
-  if (callLevelCheck) {
-    return callLevelCheck(toolCall, context);
+  if (isCallApprovalCheck(callLevelCheck)) {
+    return callLevelCheck({ ...toolCall, arguments: args }, context);
   }
 
   // Fall back to tool-level setting
-  const tool = tools.find(t => t.function.name === toolCall.name);
   if (!tool) return false;
 
   const requireApproval = tool.function.requireApproval;
 
   // If it's a function, call it with the tool's arguments and context
   if (typeof requireApproval === 'function') {
-    return requireApproval(toolCall.arguments, context);
+    const check: unknown = requireApproval;
+    // Always a function here; the guard only widens its parameter type.
+    return isArgumentsApprovalCheck(check) ? check(args, context) : true;
   }
 
   // Otherwise treat as boolean
   return requireApproval ?? false;
 }
 
+/** A call-level approval check, seen with the arguments as parsed values. */
+type CallApprovalCheck = (
+  toolCall: { id: string; name: string; arguments: unknown },
+  context: TurnContext,
+) => boolean | Promise<boolean>;
+
+/** A tool-level approval check, seen with the arguments as parsed values. */
+type ArgumentsApprovalCheck = (params: unknown, context: TurnContext) => boolean | Promise<boolean>;
+
+function isCallApprovalCheck(value: unknown): value is CallApprovalCheck {
+  return typeof value === 'function';
+}
+
+function isArgumentsApprovalCheck(value: unknown): value is ArgumentsApprovalCheck {
+  return typeof value === 'function';
+}
+
+/**
+ * Return the tool-call arguments as the executor will see them: the output of
+ * the tool's inputSchema, with defaults, coercions, and transforms applied.
+ * Arguments that fail validation are returned unchanged; the executor rejects
+ * those calls before `execute` runs.
+ *
+ * The tool call itself is not modified: the executor parses
+ * `toolCall.arguments` again, and parsing already-parsed values would apply
+ * transforms twice.
+ */
+function normalizedArguments(args: unknown, tool: Tool): unknown {
+  const parsed = z4.safeParse(tool.function.inputSchema, args);
+  return parsed.success ? parsed.data : args;
+}
+
+/**
+ * Validate approval decisions before any tool runs (#518).
+ * @throws Error if an ID is listed twice, or is both approved and rejected
+ */
+export function assertValidApprovalDecisions(
+  approveToolCalls: readonly string[],
+  rejectToolCalls: readonly string[],
+): void {
+  const problems = [
+    ...duplicateIds(approveToolCalls).map(id => `"${id}" is listed more than once in approveToolCalls`),
+    ...duplicateIds(rejectToolCalls).map(id => `"${id}" is listed more than once in rejectToolCalls`),
+    ...[...new Set(approveToolCalls)]
+      .filter(id => rejectToolCalls.includes(id))
+      .map(id => `"${id}" is in both approveToolCalls and rejectToolCalls`),
+  ];
+  if (problems.length > 0) {
+    throw new Error(`Invalid approval decisions: ${problems.join('; ')}`);
+  }
+}
+
+function duplicateIds(ids: readonly string[]): string[] {
+  const seen = new Set<string>();
+  const duplicates = new Set<string>();
+  for (const id of ids) {
+    if (seen.has(id)) duplicates.add(id);
+    seen.add(id);
+  }
+  return [...duplicates];
+}
+
 /**
  * Partition tool calls into those requiring approval and those that can auto-execute
  * @param toolCalls - Tool calls to partition

src/lib/model-result.ts

@@ -40,6 +40,7 @@ import {
 } from './async-params.js';
 import {
   appendToMessages,
+  assertValidApprovalDecisions,
   createInitialState,
   createRejectedResult,
   createUnsentResult,
@@ -224,6 +225,8 @@ export class ModelResult<
       );
     }
 
+    assertValidApprovalDecisions(options.approveToolCalls ?? [], options.rejectToolCalls ?? []);
+
     // Initialize state management
     this.stateAccessor = options.state ?? null;
     this.requireApprovalFn = options.requireApproval ?? null;
@@ -1003,9 +1006,10 @@ export class ModelResult<
         if (loadedState) {
           this.currentState = loadedState;
 
-          // Check if we're resuming from awaiting_approval with decisions
-          if (loadedState.status === 'awaiting_approval' &&
-              (this.approvedToolCalls.length > 0 || this.rejectedToolCalls.length > 0)) {
+          // Resuming from awaiting_approval always goes through decision
+          // processing. With no decisions every call stays pending, so the
+          // conversation stays paused instead of advancing (#517).
+          if (loadedState.status === 'awaiting_approval') {
             // Initialize context store before resuming so tools have access
             if (this.options.context !== undefined) {
               const approvalContext: TurnContext = { numberOfTurns: 0 };
@@ -1362,14 +1366,31 @@ export class ModelResult<
     return this.toolExecutionPromise;
   }
 
+  /**
+   * Error for a missing final response. A conversation resumed while still
+   * awaiting approval has no response until a decision is passed.
+   */
+  private responseUnavailableError(): Error {
+    if (this.currentState?.status !== 'awaiting_approval') {
+      return new Error('Response not available');
+    }
+    const pending = (this.currentState.pendingToolCalls ?? [])
+      .map(tc => `${String(tc.name)} (${tc.id})`)
+      .join(', ');
+    return new Error(
+      `Conversation is awaiting approval for tool calls: ${pending}. ` +
+      'Pass approveToolCalls or rejectToolCalls to continue.'
+    );
+  }
+
   /**
    * Internal helper to get the text after tool execution
    */
   private async getTextInternal(): Promise<string> {
     await this.executeToolsIfNeeded();
 
     if (!this.finalResponse) {
-      throw new Error('Response not available');
+      throw this.responseUnavailableError();
     }
 
     return extractTextFromResponse(this.finalResponse);
@@ -1397,7 +1418,7 @@ export class ModelResult<
     await this.executeToolsIfNeeded();
 
     if (!this.finalResponse) {
-      throw new Error('Response not available');
+      throw this.responseUnavailableError();
     }
 
     return this.finalResponse;

tests/unit/approval-hardening.test.ts

@@ -0,0 +1,198 @@
+import type { Fetcher } from '../../src/lib/http.js';
+import type { ConversationState, ParsedToolCall, StateAccessor } from '../../src/lib/tool-types.js';
+
+import { describe, expect, it } from 'vitest';
+import * as z from 'zod/v4';
+import { OpenRouterCore } from '../../src/core.js';
+import {
+  assertValidApprovalDecisions,
+  toolRequiresApproval,
+} from '../../src/lib/conversation-state.js';
+import { HTTPClient } from '../../src/lib/http.js';
+import { ModelResult } from '../../src/lib/model-result.js';
+import { tool } from '../../src/lib/tool.js';
+
+const context = { numberOfTurns: 1 };
+
+// A tool whose schema defaults `destructive` to true, and whose approval
+// predicate gates on that flag.
+const deleteFiles = tool({
+  name: 'delete_files',
+  inputSchema: z.object({
+    path: z.string().default('/tmp'),
+    destructive: z.boolean().default(true),
+  }),
+  requireApproval: (params) => params.destructive === true,
+  execute: async () => ({ deleted: true }),
+});
+
+const tools: readonly [typeof deleteFiles] = [deleteFiles];
+
+function deleteCall(args: Record<string, unknown>): ParsedToolCall<typeof deleteFiles> {
+  return { id: 'call_1', name: 'delete_files', arguments: args };
+}
+
+describe('approval predicates see normalized arguments (#520)', () => {
+  it('applies schema defaults before the tool-level predicate', async () => {
+    // The model omitted `destructive`; the executor would run with the default `true`.
+    expect(await toolRequiresApproval(deleteCall({}), tools, context)).toBe(true);
+  });
+
+  it('applies schema defaults before the call-level predicate', async () => {
+    const seen: unknown[] = [];
+    await toolRequiresApproval(deleteCall({}), tools, context, (call) => {
+      seen.push(call.arguments);
+      return false;
+    });
+
+    expect(seen).toEqual([{ path: '/tmp', destructive: true }]);
+  });
+
+  it('respects an explicit value over the default', async () => {
+    expect(await toolRequiresApproval(deleteCall({ destructive: false }), tools, context)).toBe(false);
+  });
+
+  it('passes type-changing transform output to the predicate', async () => {
+    const forceTool = tool({
+      name: 'force_push',
+      inputSchema: z.object({ force: z.string().transform((value) => value === 'yes') }),
+      requireApproval: (params) => params.force === true,
+      execute: async () => ({ pushed: true }),
+    });
+    const call: ParsedToolCall<typeof forceTool> = {
+      id: 'call_2',
+      name: 'force_push',
+      arguments: { force: 'yes' },
+    };
+
+    // The executor runs with force === true, so approval must be required.
+    expect(await toolRequiresApproval(call, [forceTool], context)).toBe(true);
+  });
+
+  it('does not modify the original tool call', async () => {
+    const call = deleteCall({});
+    await toolRequiresApproval(call, tools, context);
+
+    expect(call.arguments).toEqual({});
+  });
+});
+
+describe('approval decisions are validated (#518)', () => {
+  it('accepts distinct decisions', () => {
+    expect(() => assertValidApprovalDecisions(['call_1', 'call_2'], ['call_3'])).not.toThrow();
+  });
+
+  it('rejects an ID approved twice', () => {
+    expect(() => assertValidApprovalDecisions(['call_1', 'call_1'], [])).toThrow(
+      /"call_1" is listed more than once in approveToolCalls/,
+    );
+  });
+
+  it('rejects an ID rejected twice', () => {
+    expect(() => assertValidApprovalDecisions([], ['call_1', 'call_1'])).toThrow(
+      /"call_1" is listed more than once in rejectToolCalls/,
+    );
+  });
+
+  it('rejects an ID that is both approved and rejected', () => {
+    expect(() => assertValidApprovalDecisions(['call_1'], ['call_1'])).toThrow(
+      /"call_1" is in both approveToolCalls and rejectToolCalls/,
+    );
+  });
+
+  it('throws from ModelResult before any tool runs', () => {
+    const { client } = recordingClient();
+    const { accessor } = memoryState(pausedState());
+
+    expect(
+      () =>
+        new ModelResult({
+          request: { model: 'openai/gpt-5-mini', input: 'hi' },
+          client,
+          tools,
+          state: accessor,
+          approveToolCalls: ['call_1', 'call_1'],
+        }),
+    ).toThrow(/listed more than once/);
+  });
+});
+
+describe('resuming a paused conversation without a decision (#517)', () => {
+  it('stays awaiting_approval and sends no model request', async () => {
+    const { client, requests } = recordingClient();
+    const { accessor, saved } = memoryState(pausedState());
+
+    const result = new ModelResult({
+      request: { model: 'openai/gpt-5-mini', input: 'continue' },
+      client,
+      tools,
+      state: accessor,
+    });
+
+    expect(await result.requiresApproval()).toBe(true);
+    expect(await result.getPendingToolCalls()).toHaveLength(1);
+    expect(requests).toHaveLength(0);
+    expect(saved.every((state) => state.status === 'awaiting_approval')).toBe(true);
+  });
+
+  it('explains that approval is pending when the response is requested', async () => {
+    const { client } = recordingClient();
+    const { accessor } = memoryState(pausedState());
+
+    const result = new ModelResult({
+      request: { model: 'openai/gpt-5-mini', input: 'continue' },
+      client,
+      tools,
+      state: accessor,
+    });
+
+    await expect(result.getResponse()).rejects.toThrow(
+      /awaiting approval for tool calls: delete_files \(call_1\)/,
+    );
+  });
+});
+
+/** A client whose fetcher records requests and answers with a 500. */
+function recordingClient(): { client: OpenRouterCore; requests: Request[] } {
+  const requests: Request[] = [];
+  const fetcher: Fetcher = async (input) => {
+    if (input instanceof Request) requests.push(input);
+    return new Response('{}', { status: 500 });
+  };
+  const client = new OpenRouterCore({
+    apiKey: 'test',
+    httpClient: new HTTPClient({ fetcher }),
+    retryConfig: { strategy: 'none' },
+  });
+  return { client, requests };
+}
+
+/** A conversation paused on one approval-gated call. */
+function pausedState(): ConversationState<typeof tools> {
+  return {
+    id: 'conv_test',
+    messages: [{ role: 'user', content: 'delete the temp files' }],
+    pendingToolCalls: [deleteCall({})],
+    status: 'awaiting_approval',
+    createdAt: 0,
+    updatedAt: 0,
+  };
+}
+
+function memoryState(initial: ConversationState<typeof tools>): {
+  accessor: StateAccessor<typeof tools>;
+  saved: ConversationState<typeof tools>[];
+} {
+  let current = initial;
+  const saved: ConversationState<typeof tools>[] = [];
+  return {
+    accessor: {
+      load: async () => current,
+      save: async (state) => {
+        current = state;
+        saved.push(state);
+      },
+    },
+    saved,
+  };
+}