fix: parallel tool calling support in approval flow (#101)

2025-11-17 18:27:20 -08:00
parent 3d4f4dfd3c
commit 6b40e86401
3 changed files with 87 additions and 41 deletions
--- 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) {
--- 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<Record<string, unknown>>(
                  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<Record<string, unknown>>(
-              existingApproval.toolArgs,
-              {},
+            // Analyze approval contexts for ALL pending approvals
+            const contexts = await Promise.all(
+              existingApprovals.map(async (approval) => {
+                const parsedArgs = safeJsonParseOr<Record<string, unknown>>(
+                  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<string>, b: Set<string>) =>
+          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,
--- 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 {