Pull in Roo tool calling fixes (PRs 10466 & 10519)

.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)

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

src/core/task/Task.ts

@@ -1531,6 +1531,10 @@ export class Task extends EventEmitter<TaskEvents> 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

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<Anthropic.ToolResultBlockParam | Anthropic.TextBlockParam>
+
+			// 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", () => {

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<string>()
+	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<string>()
+	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<typeof block> => 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))

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,