From 6b40e8640194d19182005f81f73313d9f2b1debd Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Mon, 17 Nov 2025 18:27:20 -0800 Subject: [PATCH] fix: parallel tool calling support in approval flow (#101) --- src/agent/check-approval.ts | 26 ++++++------ src/cli/App.tsx | 85 ++++++++++++++++++++++++++++--------- src/cli/helpers/stream.ts | 17 +++++--- 3 files changed, 87 insertions(+), 41 deletions(-) diff --git a/src/agent/check-approval.ts b/src/agent/check-approval.ts index a2a85df..ae86fbc 100644 --- a/src/agent/check-approval.ts +++ b/src/agent/check-approval.ts @@ -121,20 +121,18 @@ export async function getResumeData( : []; // Extract ALL tool calls for parallel approval support - type ValidToolCall = { - tool_call_id: string; - name: string; - arguments: string; - }; - const validToolCalls = toolCalls.filter( - (tc): tc is ValidToolCall => - !!tc && !!tc.tool_call_id && !!tc.name && !!tc.arguments, - ); - pendingApprovals = validToolCalls.map((tc) => ({ - toolCallId: tc.tool_call_id, - toolName: tc.name, - toolArgs: tc.arguments, - })); + // Include ALL tool_call_ids, even those with incomplete name/arguments + // Incomplete entries will be denied at the business logic layer + pendingApprovals = toolCalls + .filter( + (tc): tc is typeof tc & { tool_call_id: string } => + !!tc && !!tc.tool_call_id, + ) + .map((tc) => ({ + toolCallId: tc.tool_call_id, + toolName: tc.name || "", + toolArgs: tc.arguments || "", + })); // Set legacy singular field for backward compatibility (first approval only) if (pendingApprovals.length > 0) { diff --git a/src/cli/App.tsx b/src/cli/App.tsx index 66a53a6..e560910 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -490,6 +490,10 @@ export default function App({ // Case 2: Requires approval if (stopReason === "requires_approval") { + // Clear stale state immediately to prevent ID mismatch bugs + setAutoHandledResults([]); + setAutoDeniedApprovals([]); + // Use new approvals array, fallback to legacy approval for backward compat const approvalsToProcess = approvals && approvals.length > 0 @@ -529,6 +533,19 @@ export default function App({ // Check permissions for all approvals const approvalResults = await Promise.all( approvalsToProcess.map(async (approvalItem) => { + // Check if approval is incomplete (missing name or arguments) + if (!approvalItem.toolName || !approvalItem.toolArgs) { + return { + approval: approvalItem, + permission: { + decision: "deny" as const, + reason: + "Tool call incomplete - missing name or arguments", + }, + context: null, + }; + } + const parsedArgs = safeJsonParseOr>( approvalItem.toolArgs, {}, @@ -590,7 +607,10 @@ export default function App({ // Create denial results for auto-denied tools const autoDeniedResults = autoDenied.map((ac) => ({ approval: ac.approval, - reason: `Permission denied by rule: ${ac.permission.matchedRule || ac.permission.reason}`, + reason: + "matchedRule" in ac.permission && ac.permission.matchedRule + ? `Permission denied by rule: ${ac.permission.matchedRule}` + : `Permission denied: ${ac.permission.reason || "Unknown reason"}`, })); // If all are auto-handled, continue immediately without showing dialog @@ -610,13 +630,10 @@ export default function App({ stderr: ar.result.stderr, })), ...autoDeniedResults.map((ad) => ({ - type: "tool" as const, + type: "approval" as const, tool_call_id: ad.approval.toolCallId, - tool_return: JSON.stringify({ - status: "error", - message: ad.reason, - }), - status: "error" as const, + approve: false, + reason: ad.reason, })), ]; @@ -631,7 +648,11 @@ export default function App({ // Show approval dialog for tools that need user input setPendingApprovals(needsUserInput.map((ac) => ac.approval)); - setApprovalContexts(needsUserInput.map((ac) => ac.context)); + setApprovalContexts( + needsUserInput + .map((ac) => ac.context) + .filter((ctx): ctx is ApprovalContext => ctx !== null), + ); setAutoHandledResults(autoAllowedResults); setAutoDeniedApprovals(autoDeniedResults); setStreaming(false); @@ -1290,28 +1311,29 @@ export default function App({ const client = await getClient(); // Fetch fresh agent state to check for pending approvals with accurate in-context messages const agent = await client.agents.retrieve(agentId); - const { pendingApproval: existingApproval } = await getResumeData( + const { pendingApprovals: existingApprovals } = await getResumeData( client, agent, ); - if (existingApproval) { - // There's a pending approval - show it and DON'T send the message yet + if (existingApprovals && existingApprovals.length > 0) { + // There are pending approvals - show them and DON'T send the message yet // The message will be restored to the input field for the user to decide // Note: The user message is already in the transcript (optimistic update) setStreaming(false); // Stop streaming indicator - setPendingApprovals([existingApproval]); + setPendingApprovals(existingApprovals); - // Analyze approval context - const parsedArgs = safeJsonParseOr>( - existingApproval.toolArgs, - {}, + // Analyze approval contexts for ALL pending approvals + const contexts = await Promise.all( + existingApprovals.map(async (approval) => { + const parsedArgs = safeJsonParseOr>( + approval.toolArgs, + {}, + ); + return await analyzeToolApproval(approval.toolName, parsedArgs); + }), ); - const context = await analyzeToolApproval( - existingApproval.toolName, - parsedArgs, - ); - setApprovalContexts([context]); + setApprovalContexts(contexts); // Return false = message NOT submitted, will be restored to input return { submitted: false }; @@ -1404,6 +1426,26 @@ export default function App({ ...executedResults, ]; + // Dev-only validation: ensure outgoing IDs match expected IDs + if (process.env.NODE_ENV !== "production") { + const expectedIds = new Set(pendingApprovals.map((a) => a.toolCallId)); + const sendingIds = new Set( + allResults.map((r) => r.tool_call_id).filter(Boolean), + ); + + const setsEqual = (a: Set, b: Set) => + a.size === b.size && [...a].every((id) => b.has(id)); + + if (!setsEqual(expectedIds, sendingIds)) { + console.error("[BUG] Approval ID mismatch detected"); + console.error("Expected IDs:", Array.from(expectedIds)); + console.error("Sending IDs:", Array.from(sendingIds)); + throw new Error( + "Approval ID mismatch - refusing to send mismatched IDs", + ); + } + } + // Clear state setPendingApprovals([]); setApprovalContexts([]); @@ -1427,6 +1469,7 @@ export default function App({ approvalResults, autoHandledResults, autoDeniedApprovals, + pendingApprovals, processConversation, refreshDerived, appendError, diff --git a/src/cli/helpers/stream.ts b/src/cli/helpers/stream.ts index 6854a1a..67d4bd7 100644 --- a/src/cli/helpers/stream.ts +++ b/src/cli/helpers/stream.ts @@ -155,20 +155,25 @@ export async function drainStream( let approvals: ApprovalRequest[] = []; if (stopReason === "requires_approval") { - // Convert map to array, filtering out incomplete entries + // Convert map to array, including ALL tool_call_ids (even incomplete ones) + // Incomplete entries will be denied at the business logic layer const allPending = Array.from(pendingApprovals.values()); // console.log( - // "[drainStream] All pending approvals before filter:", + // "[drainStream] All pending approvals before processing:", // JSON.stringify(allPending, null, 2), // ); - approvals = allPending.filter( - (a) => a.toolCallId && a.toolName && a.toolArgs, - ); + // Include ALL tool_call_ids - don't filter out incomplete entries + // Missing name/args will be handled by denial logic in App.tsx + approvals = allPending.map((a) => ({ + toolCallId: a.toolCallId, + toolName: a.toolName || "", + toolArgs: a.toolArgs || "", + })); if (approvals.length === 0) { console.error( - "[drainStream] No valid approvals collected despite requires_approval stop reason", + "[drainStream] No approvals collected despite requires_approval stop reason", ); console.error("[drainStream] Pending approvals map:", allPending); } else {