diff --git a/.changeset/approval-feedback-fix.md b/.changeset/approval-feedback-fix.md new file mode 100644 index 00000000000..8f83d14e453 --- /dev/null +++ b/.changeset/approval-feedback-fix.md @@ -0,0 +1,10 @@ +--- +"kilo-code": patch +--- + +Fix duplicate tool_result blocks when users approve tool execution with feedback text + +Cherry-picked from upstream Roo-Code: + +- [#10466](https://github.com/RooCodeInc/Roo-Code/pull/10466) - Add explicit deduplication (thanks @daniel-lxs) +- [#10519](https://github.com/RooCodeInc/Roo-Code/pull/10519) - Merge approval feedback into tool result (thanks @daniel-lxs) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index db144df124f..40beeb88563 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -154,7 +154,10 @@ export async function presentAssistantMessage(cline: Task) { const toolCallId = mcpBlock.id const toolProtocol = TOOL_PROTOCOL.NATIVE // MCP tools in native mode always use native protocol - const pushToolResult = (content: ToolResponse) => { + // Store approval feedback to merge into tool result (GitHub #10465) + let approvalFeedback: { text: string; images?: string[] } | undefined + + const pushToolResult = (content: ToolResponse, feedbackImages?: string[]) => { if (hasToolResult) { console.warn( `[presentAssistantMessage] Skipping duplicate tool_result for mcp_tool_use: ${toolCallId}`, @@ -175,6 +178,18 @@ export async function presentAssistantMessage(cline: Task) { "(tool did not return anything)" } + // Merge approval feedback into tool result (GitHub #10465) + if (approvalFeedback) { + const feedbackText = formatResponse.toolApprovedWithFeedback(approvalFeedback.text, toolProtocol) + resultContent = `${feedbackText}\n\n${resultContent}` + + // Add feedback images to the image blocks + if (approvalFeedback.images) { + const feedbackImageBlocks = formatResponse.imageBlocks(approvalFeedback.images) + imageBlocks = [...feedbackImageBlocks, ...imageBlocks] + } + } + if (toolCallId) { cline.pushToolResultToUserContent({ type: "tool_result", @@ -223,11 +238,12 @@ export async function presentAssistantMessage(cline: Task) { return false } + // Store approval feedback to be merged into tool result (GitHub #10465) + // Don't push it as a separate tool_result here - that would create duplicates. + // The tool will call pushToolResult, which will merge the feedback into the actual result. if (text) { await cline.say("user_feedback", text, images) - pushToolResult( - formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text, toolProtocol), images), - ) + approvalFeedback = { text, images } } return true @@ -548,6 +564,9 @@ export async function presentAssistantMessage(cline: Task) { // Previously resolved from experiments.isEnabled(..., EXPERIMENT_IDS.MULTIPLE_NATIVE_TOOL_CALLS) const isMultipleNativeToolCallsEnabled = false + // Store approval feedback to merge into tool result (GitHub #10465) + let approvalFeedback: { text: string; images?: string[] } | undefined + const pushToolResult = (content: ToolResponse) => { if (toolProtocol === TOOL_PROTOCOL.NATIVE) { // For native protocol, only allow ONE tool_result per tool call @@ -576,6 +595,21 @@ export async function presentAssistantMessage(cline: Task) { "(tool did not return anything)" } + // Merge approval feedback into tool result (GitHub #10465) + if (approvalFeedback) { + const feedbackText = formatResponse.toolApprovedWithFeedback( + approvalFeedback.text, + toolProtocol, + ) + resultContent = `${feedbackText}\n\n${resultContent}` + + // Add feedback images to the image blocks + if (approvalFeedback.images) { + const feedbackImageBlocks = formatResponse.imageBlocks(approvalFeedback.images) + imageBlocks = [...feedbackImageBlocks, ...imageBlocks] + } + } + // Add tool_result with text content only cline.pushToolResultToUserContent({ type: "tool_result", @@ -591,15 +625,44 @@ export async function presentAssistantMessage(cline: Task) { hasToolResult = true } else { // For XML protocol, add as text blocks (legacy behavior) + let resultContent: string + + if (typeof content === "string") { + resultContent = content || "(tool did not return anything)" + } else { + const textBlocks = content.filter((item) => item.type === "text") + resultContent = + textBlocks.map((item) => (item as Anthropic.TextBlockParam).text).join("\n") || + "(tool did not return anything)" + } + + // Merge approval feedback into tool result (GitHub #10465) + if (approvalFeedback) { + const feedbackText = formatResponse.toolApprovedWithFeedback( + approvalFeedback.text, + toolProtocol, + ) + resultContent = `${feedbackText}\n\n${resultContent}` + } + cline.userMessageContent.push({ type: "text", text: `${toolDescription()} Result:` }) if (typeof content === "string") { cline.userMessageContent.push({ type: "text", - text: content || "(tool did not return anything)", + text: resultContent, }) } else { - cline.userMessageContent.push(...content) + // Add text content with merged feedback + cline.userMessageContent.push({ + type: "text", + text: resultContent, + }) + // Add any images from the tool result + const imageBlocks = content.filter((item) => item.type === "image") + if (imageBlocks.length > 0) { + cline.userMessageContent.push(...imageBlocks) + } } } @@ -668,12 +731,12 @@ export async function presentAssistantMessage(cline: Task) { return false } - // Handle yesButtonClicked with text. + // Store approval feedback to be merged into tool result (GitHub #10465) + // Don't push it as a separate tool_result here - that would create duplicates. + // The tool will call pushToolResult, which will merge the feedback into the actual result. if (text) { await cline.say("user_feedback", text, images) - pushToolResult( - formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text, toolProtocol), images), - ) + approvalFeedback = { text, images } } captureAskApproval(block.name, true) // kilocode_change diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index fbc55205e55..92af0fca97b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1531,6 +1531,10 @@ export class Task extends EventEmitter implements TaskLike { this.handleWebviewAskResponse("noButtonClicked", text, images) } + public supersedePendingAsk(): void { + this.lastMessageTs = Date.now() + } + /** * Updates the API configuration but preserves the locked tool protocol. * The task's tool protocol is locked at creation time and should NOT change diff --git a/src/core/task/__tests__/validateToolResultIds.spec.ts b/src/core/task/__tests__/validateToolResultIds.spec.ts index 28491aedd73..0926e899aad 100644 --- a/src/core/task/__tests__/validateToolResultIds.spec.ts +++ b/src/core/task/__tests__/validateToolResultIds.spec.ts @@ -482,6 +482,96 @@ describe("validateAndFixToolResultIds", () => { expect(resultContent[1].type).toBe("text") expect((resultContent[1] as Anthropic.TextBlockParam).text).toBe("Some additional context") }) + + // Verifies fix for GitHub #10465: Terminal fallback race condition can generate + // duplicate tool_results with the same valid tool_use_id, causing API protocol violations. + it("should filter out duplicate tool_results with identical valid tool_use_ids (terminal fallback scenario)", () => { + const assistantMessage: Anthropic.MessageParam = { + role: "assistant", + content: [ + { + type: "tool_use", + id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g", + name: "execute_command", + input: { command: "ps aux | grep test", cwd: "/path/to/project" }, + }, + ], + } + + // Two tool_results with the SAME valid tool_use_id from terminal fallback race condition + const userMessage: Anthropic.MessageParam = { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g", // First result from command execution + content: "No test processes found", + }, + { + type: "tool_result", + tool_use_id: "tooluse_QZ-pU8v2QKO8L8fHoJRI2g", // Duplicate from user approval during fallback + content: '{"status":"approved","message":"The user approved this operation"}', + }, + ], + } + + const result = validateAndFixToolResultIds(userMessage, [assistantMessage]) + + expect(Array.isArray(result.content)).toBe(true) + const resultContent = result.content as Anthropic.ToolResultBlockParam[] + + // Only ONE tool_result should remain to prevent API protocol violation + expect(resultContent.length).toBe(1) + expect(resultContent[0].tool_use_id).toBe("tooluse_QZ-pU8v2QKO8L8fHoJRI2g") + expect(resultContent[0].content).toBe("No test processes found") + }) + + it("should preserve text blocks while deduplicating tool_results with same valid ID", () => { + const assistantMessage: Anthropic.MessageParam = { + role: "assistant", + content: [ + { + type: "tool_use", + id: "tool-123", + name: "read_file", + input: { path: "test.txt" }, + }, + ], + } + + const userMessage: Anthropic.MessageParam = { + role: "user", + content: [ + { + type: "tool_result", + tool_use_id: "tool-123", + content: "First result", + }, + { + type: "text", + text: "Environment details here", + }, + { + type: "tool_result", + tool_use_id: "tool-123", // Duplicate with same valid ID + content: "Duplicate result from fallback", + }, + ], + } + + const result = validateAndFixToolResultIds(userMessage, [assistantMessage]) + + expect(Array.isArray(result.content)).toBe(true) + const resultContent = result.content as Array + + // Should have: 1 tool_result + 1 text block (duplicate filtered out) + expect(resultContent.length).toBe(2) + expect(resultContent[0].type).toBe("tool_result") + expect((resultContent[0] as Anthropic.ToolResultBlockParam).tool_use_id).toBe("tool-123") + expect((resultContent[0] as Anthropic.ToolResultBlockParam).content).toBe("First result") + expect(resultContent[1].type).toBe("text") + expect((resultContent[1] as Anthropic.TextBlockParam).text).toBe("Environment details here") + }) }) describe("when there are more tool_uses than tool_results", () => { diff --git a/src/core/task/validateToolResultIds.ts b/src/core/task/validateToolResultIds.ts index ce89a4e167b..a966d429ed5 100644 --- a/src/core/task/validateToolResultIds.ts +++ b/src/core/task/validateToolResultIds.ts @@ -78,7 +78,33 @@ export function validateAndFixToolResultIds( } // Find tool_result blocks in the user message - const toolResults = userMessage.content.filter( + let toolResults = userMessage.content.filter( + (block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result", + ) + + // Deduplicate tool_result blocks to prevent API protocol violations (GitHub #10465) + // This serves as a safety net for any potential race conditions that could generate + // duplicate tool_results with the same tool_use_id. The root cause (approval feedback + // creating duplicate results) has been fixed in presentAssistantMessage.ts, but this + // deduplication remains as a defensive measure for unknown edge cases. + const seenToolResultIds = new Set() + const deduplicatedContent = userMessage.content.filter((block) => { + if (block.type !== "tool_result") { + return true + } + if (seenToolResultIds.has(block.tool_use_id)) { + return false // Duplicate - filter out + } + seenToolResultIds.add(block.tool_use_id) + return true + }) + + userMessage = { + ...userMessage, + content: deduplicatedContent, + } + + toolResults = deduplicatedContent.filter( (block): block is Anthropic.ToolResultBlockParam => block.type === "tool_result", ) @@ -139,15 +165,12 @@ export function validateAndFixToolResultIds( ) } - // Create a mapping of tool_result IDs to corrected IDs - // Strategy: Match by position (first tool_result -> first tool_use, etc.) - // This handles most cases where the mismatch is due to ID confusion - // - // Track which tool_use IDs have been used to prevent duplicates + // Match tool_results to tool_uses by position and fix incorrect IDs const usedToolUseIds = new Set() + const contentArray = userMessage.content as Anthropic.Messages.ContentBlockParam[] - const correctedContent = userMessage.content - .map((block) => { + const correctedContent = contentArray + .map((block: Anthropic.Messages.ContentBlockParam) => { if (block.type !== "tool_result") { return block } @@ -177,17 +200,18 @@ export function validateAndFixToolResultIds( } // No corresponding tool_use for this tool_result, or the ID is already used - // Filter out this orphaned tool_result by returning null return null }) .filter((block): block is NonNullable => block !== null) // Add missing tool_result blocks for any tool_use that doesn't have one - // After the ID correction above, recalculate which tool_use IDs are now covered const coveredToolUseIds = new Set( correctedContent - .filter((b): b is Anthropic.ToolResultBlockParam => b.type === "tool_result") - .map((r) => r.tool_use_id), + .filter( + (b: Anthropic.Messages.ContentBlockParam): b is Anthropic.ToolResultBlockParam => + b.type === "tool_result", + ) + .map((r: Anthropic.ToolResultBlockParam) => r.tool_use_id), ) const stillMissingToolUseIds = toolUseBlocks.filter((toolUse) => !coveredToolUseIds.has(toolUse.id)) diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index f7271bffe9c..7feb71b0b89 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -116,6 +116,9 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) await task.say("shell_integration_warning") + // Invalidate pending ask from first execution to prevent race condition + task.supersedePendingAsk() + if (error instanceof ShellIntegrationError) { const [rejected, result] = await executeCommandInTerminal(task, { ...options,