From 7bfc2ce25ae0337021e661e52b2899cf05414d8a Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Fri, 7 Nov 2025 17:31:00 -0800 Subject: [PATCH] fix: improve parallel tool approval UX with sequential review (#79) Co-authored-by: Letta --- src/cli/App.tsx | 207 +++++++++++++--------- src/cli/components/ApprovalDialogRich.tsx | 4 +- 2 files changed, 126 insertions(+), 85 deletions(-) diff --git a/src/cli/App.tsx b/src/cli/App.tsx index 0bba8c5..af33fb9 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -143,16 +143,10 @@ export default function App({ // Sequential approval: track results as user reviews each approval const [approvalResults, setApprovalResults] = useState< - Array<{ - type: "approval" | "tool"; - tool_call_id: string; - approve?: boolean; - reason?: string; - tool_return?: string; - status?: "success" | "error"; - stdout?: string[]; - stderr?: string[]; - }> + Array< + | { type: "approve"; approval: ApprovalRequest } + | { type: "deny"; approval: ApprovalRequest; reason: string } + > >([]); const [isExecutingTool, setIsExecutingTool] = useState(false); @@ -1164,16 +1158,107 @@ export default function App({ // Helper to send all approval results when done const sendAllResults = useCallback( - async (additionalResult?: { - type: "approval" | "tool"; - tool_call_id: string; - approve?: boolean; - reason?: string; - tool_return?: string; - status?: "success" | "error"; - stdout?: string[]; - stderr?: string[]; - }) => { + async ( + additionalDecision?: + | { type: "approve"; approval: ApprovalRequest } + | { type: "deny"; approval: ApprovalRequest; reason: string }, + ) => { + // Combine all decisions + const allDecisions = [ + ...approvalResults, + ...(additionalDecision ? [additionalDecision] : []), + ]; + + // Execute approved tools and format results + const executedResults: Array<{ + type: "tool" | "approval"; + tool_call_id: string; + tool_return?: string; + status?: "success" | "error"; + stdout?: string[]; + stderr?: string[]; + approve?: boolean; + reason?: string; + }> = []; + + for (const decision of allDecisions) { + if (decision.type === "approve") { + // Execute the approved tool + try { + const parsedArgs = safeJsonParseOr>( + decision.approval.toolArgs, + {}, + ); + const toolResult = await executeTool( + decision.approval.toolName, + parsedArgs, + ); + + // Update buffers with tool return for UI + onChunk(buffersRef.current, { + message_type: "tool_return_message", + id: "dummy", + date: new Date().toISOString(), + tool_call_id: decision.approval.toolCallId, + tool_return: toolResult.toolReturn, + status: toolResult.status, + stdout: toolResult.stdout, + stderr: toolResult.stderr, + }); + + executedResults.push({ + type: "tool", + tool_call_id: decision.approval.toolCallId, + tool_return: toolResult.toolReturn, + status: toolResult.status, + stdout: toolResult.stdout, + stderr: toolResult.stderr, + }); + } catch (e) { + appendError(String(e)); + + // Still need to send error result to backend for this tool + const errorMessage = `Error executing tool: ${String(e)}`; + + // Update buffers with error for UI + onChunk(buffersRef.current, { + message_type: "tool_return_message", + id: "dummy", + date: new Date().toISOString(), + tool_call_id: decision.approval.toolCallId, + tool_return: errorMessage, + status: "error", + }); + + executedResults.push({ + type: "tool", + tool_call_id: decision.approval.toolCallId, + tool_return: errorMessage, + status: "error", + }); + } + } else { + // Format denial for backend + // Update buffers with denial for UI + onChunk(buffersRef.current, { + message_type: "tool_return_message", + id: "dummy", + date: new Date().toISOString(), + tool_call_id: decision.approval.toolCallId, + tool_return: `Error: request to call tool denied. User reason: ${decision.reason}`, + status: "error", + }); + + executedResults.push({ + type: "approval", + tool_call_id: decision.approval.toolCallId, + approve: false, + reason: decision.reason, + }); + } + } + + // Combine with auto-handled and auto-denied results const allResults = [ ...autoHandledResults.map((ar) => ({ type: "tool" as const, @@ -1189,8 +1274,7 @@ export default function App({ approve: false, reason: ad.reason, })), - ...approvalResults, - ...(additionalResult ? [additionalResult] : []), + ...executedResults, ]; // Clear state @@ -1218,6 +1302,7 @@ export default function App({ autoDeniedApprovals, processConversation, refreshDerived, + appendError, ], ); @@ -1229,54 +1314,21 @@ export default function App({ if (!currentApproval) return; try { - setIsExecutingTool(true); - - // Execute the approved tool - const parsedArgs = safeJsonParseOr>( - currentApproval.toolArgs, - {}, - ); - const toolResult = await executeTool( - currentApproval.toolName, - parsedArgs, - ); - - // Update buffers with tool return for UI - onChunk(buffersRef.current, { - message_type: "tool_return_message", - id: "dummy", - date: new Date().toISOString(), - tool_call_id: currentApproval.toolCallId, - tool_return: toolResult.toolReturn, - status: toolResult.status, - stdout: toolResult.stdout, - stderr: toolResult.stderr, - }); - - // Store result - const result = { - type: "tool" as const, - tool_call_id: currentApproval.toolCallId, - tool_return: toolResult.toolReturn, - status: toolResult.status, - stdout: toolResult.stdout, - stderr: toolResult.stderr, + // Store approval decision (don't execute yet - batch execute after all approvals) + const decision = { + type: "approve" as const, + approval: currentApproval, }; - setIsExecutingTool(false); - // Check if we're done with all approvals if (currentIndex + 1 >= pendingApprovals.length) { - // All approvals processed, send results to backend - // Pass the new result directly to avoid async state update issue - await sendAllResults(result); + // All approvals collected, execute and send to backend + await sendAllResults(decision); } else { - // Not done yet, store result and show next approval - setApprovalResults((prev) => [...prev, result]); + // Not done yet, store decision and show next approval + setApprovalResults((prev) => [...prev, decision]); } - // Otherwise, next approval will be shown automatically via state update } catch (e) { - setIsExecutingTool(false); appendError(String(e)); setStreaming(false); } @@ -1332,35 +1384,22 @@ export default function App({ if (!currentApproval) return; try { - // Store denial result - const result = { - type: "approval" as const, - tool_call_id: currentApproval.toolCallId, - approve: false, + // Store denial decision + const decision = { + type: "deny" as const, + approval: currentApproval, reason: reason || "User denied the tool execution", }; - // Update buffers with denial for UI (so it shows in the right order) - onChunk(buffersRef.current, { - message_type: "tool_return_message", - id: "dummy", - date: new Date().toISOString(), - tool_call_id: currentApproval.toolCallId, - tool_return: `Error: request to call tool denied. User reason: ${result.reason}`, - status: "error", - }); - // Check if we're done with all approvals if (currentIndex + 1 >= pendingApprovals.length) { - // All approvals processed, send results to backend - // Pass the new result directly to avoid async state update issue + // All approvals collected, execute and send to backend setThinkingMessage(getRandomThinkingMessage()); - await sendAllResults(result); + await sendAllResults(decision); } else { - // Not done yet, store result and show next approval - setApprovalResults((prev) => [...prev, result]); + // Not done yet, store decision and show next approval + setApprovalResults((prev) => [...prev, decision]); } - // Otherwise, next approval will be shown automatically via state update } catch (e) { appendError(String(e)); setStreaming(false); diff --git a/src/cli/components/ApprovalDialogRich.tsx b/src/cli/components/ApprovalDialogRich.tsx index 1c69ea6..9aabb12 100644 --- a/src/cli/components/ApprovalDialogRich.tsx +++ b/src/cli/components/ApprovalDialogRich.tsx @@ -440,7 +440,9 @@ export const ApprovalDialog = memo(function ApprovalDialog({ {progress.total - (progress.current - 1)} remaining) )} - {isExecuting && Executing tool...} + {isExecuting && progress && progress.total > 1 && ( + Executing tool... + )} {/* Dynamic per-tool renderer (indented) */}