fix: prevent tool call mixing and retry storms

Author: Jpkoech30

src/core/assistant-message/presentAssistantMessage.ts

@@ -71,6 +71,10 @@ export async function presentAssistantMessage(cline: Task) {
 	cline.presentAssistantMessageLocked = true
 	cline.presentAssistantMessageHasPendingUpdates = false
 
+	// Reset batch skip counter at the start of each present cycle.
+	// This ensures the count is fresh for each batch of tool calls.
+	;(cline as any).batchSkipCount = 0
+
 	if (cline.currentStreamingContentIndex >= cline.assistantMessageContent.length) {
 		// This may happen if the last content block was completed before
 		// streaming could finish. If streaming is finished, and we're out of
@@ -111,8 +115,11 @@ export async function presentAssistantMessage(cline: Task) {
 			if (cline.didRejectTool) {
 				// For native protocol, we must send a tool_result for every tool_use to avoid API errors
 				const toolCallId = mcpBlock.id
+				const skipCount = (cline as any).batchSkipCount ?? 0
+				const nextSkipCount = skipCount + 1
+				;(cline as any).batchSkipCount = nextSkipCount
 				const errorMessage = !mcpBlock.partial
-					? `Skipping MCP tool ${mcpBlock.name} due to user rejecting a previous tool.`
+					? `Skipping MCP tool ${mcpBlock.name} due to user rejecting a previous tool. (Skipped ${nextSkipCount} tool${nextSkipCount > 1 ? "s" : ""} in this batch.)`
 					: `MCP tool ${mcpBlock.name} was interrupted and not executed due to user rejecting a previous tool.`
 
 				if (toolCallId) {
@@ -391,8 +398,11 @@ export async function presentAssistantMessage(cline: Task) {
 			if (cline.didRejectTool) {
 				// Ignore any tool content after user has rejected tool once.
 				// For native tool calling, we must send a tool_result for every tool_use to avoid API errors
+				const skipCount = (cline as any).batchSkipCount ?? 0
+				const nextSkipCount = skipCount + 1
+				;(cline as any).batchSkipCount = nextSkipCount
 				const errorMessage = !block.partial
-					? `Skipping tool ${toolDescription()} due to user rejecting a previous tool.`
+					? `Skipping tool ${toolDescription()} due to user rejecting a previous tool. (Skipped ${nextSkipCount} tool${nextSkipCount > 1 ? "s" : ""} in this batch.)`
 					: `Tool ${toolDescription()} was interrupted and not executed due to user rejecting a previous tool.`
 
 				cline.pushToolResultToUserContent({

src/core/task/Task.ts

@@ -140,6 +140,13 @@ const DEFAULT_USAGE_COLLECTION_TIMEOUT_MS = 5000 // 5 seconds
 const FORCED_CONTEXT_REDUCTION_PERCENT = 75 // Keep 75% of context (remove 25%) on context window errors
 const MAX_CONTEXT_WINDOW_RETRIES = 3 // Maximum retries for context window errors
 
+/**
+ * Maximum number of consecutive retries allowed for the same tool within a single turn.
+ * After this limit, further retries are blocked until the user provides new input.
+ * This prevents retry storms where the model repeatedly tries the same failing tool.
+ */
+const MAX_TOOL_RETRY_BUDGET = 3
+
 export interface TaskOptions extends CreateTaskOptions {
 	provider: ClineProvider
 	apiConfiguration: ProviderSettings
@@ -321,6 +328,22 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	consecutiveNoAssistantMessagesCount: number = 0
 	toolUsage: ToolUsage = {}
 
+	/**
+	 * Retry budget: tracks how many times the same tool has been retried
+	 * consecutively within the current turn. Keyed by tool name (e.g., "execute_command").
+	 * After MAX_TOOL_RETRY_BUDGET (3) retries of the same tool in a row,
+	 * further retries are blocked until the user provides new input.
+	 * The counter resets when user feedback arrives (handleWebviewAskResponse with messageResponse)
+	 * or at the start of each new API request.
+	 */
+	toolRetryBudget: Map<string, number> = new Map()
+
+	/**
+	 * When true, the retry budget for the current tool has been exceeded and
+	 * no further retries of that tool are allowed until user feedback arrives.
+	 */
+	toolRetryBudgetExceeded: boolean = false
+
 	// Checkpoints
 	enableCheckpoints: boolean
 	checkpointTimeout: number
@@ -1431,6 +1454,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 		this.askResponseText = text
 		this.askResponseImages = images
 
+		// Reset retry budget when user provides new input (messageResponse).
+		// This allows the model to retry tools after the user has had a chance to
+		// provide guidance. The budget is NOT reset on yesButtonClicked (tool approval)
+		// since that's the model continuing its own turn, not user feedback.
+		if (askResponse === "messageResponse") {
+			this.toolRetryBudget.clear()
+			this.toolRetryBudgetExceeded = false
+		}
+
 		// Create a checkpoint whenever the user sends a message.
 		// Use allowEmpty=true to ensure a checkpoint is recorded even if there are no file changes.
 		// Suppress the checkpoint_saved chat row for this particular checkpoint to keep the timeline clean.
@@ -2716,6 +2748,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 				this.didToolFailInCurrentTurn = false
 				this.presentAssistantMessageLocked = false
 				this.presentAssistantMessageHasPendingUpdates = false
+				// Reset retry budget for each new API request (new assistant turn).
+				// This gives the model a fresh budget of 3 retries per tool per turn.
+				this.toolRetryBudget.clear()
+				this.toolRetryBudgetExceeded = false
 				// No legacy text-stream tool parser.
 				this.streamingToolCallIndices.clear()
 				// Clear any leftover streaming tool call state from previous interrupted streams
@@ -4734,4 +4770,51 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 			console.error(`[Task] Queue processing error:`, e)
 		}
 	}
+
+	/**
+		* Checks if the retry budget for a given tool has been exceeded.
+		* Each tool gets MAX_TOOL_RETRY_BUDGET (3) consecutive retries per turn.
+		* After that, further retries are blocked until user feedback arrives.
+		*
+		* @param toolName - The name of the tool being retried
+		* @returns true if the retry budget is still available, false if blocked
+		*/
+	public checkToolRetryBudget(toolName: string): boolean {
+		if (this.toolRetryBudgetExceeded) {
+			console.warn(
+				`[Task#${this.taskId}] Tool retry budget globally exceeded. Blocking further retries for "${toolName}".`,
+			)
+			return false
+		}
+
+		const currentRetries = this.toolRetryBudget.get(toolName) ?? 0
+
+		if (currentRetries >= MAX_TOOL_RETRY_BUDGET) {
+			this.toolRetryBudgetExceeded = true
+			console.warn(
+				`[Task#${this.taskId}] Tool retry budget exceeded for "${toolName}": ` +
+					`${currentRetries} retries >= ${MAX_TOOL_RETRY_BUDGET} max. ` +
+					"Blocking further retries until user provides new input.",
+			)
+			return false
+		}
+
+		// Increment the retry counter for this tool
+		this.toolRetryBudget.set(toolName, currentRetries + 1)
+		return true
+	}
+
+	/**
+		* Records a tool use as successful, which resets the retry budget for that tool.
+		* This is called when a tool completes successfully, allowing the model to use
+		* the tool again (the budget is per-retry, not per-use).
+		*
+		* @param toolName - The name of the tool that succeeded
+		*/
+	public recordSuccessfulToolUse(toolName: string): void {
+		// Only reset the retry budget for this specific tool, not all tools.
+		// This allows the model to switch to a different tool after exhausting
+		// the budget on one tool.
+		this.toolRetryBudget.delete(toolName)
+	}
 }

src/core/task/validateToolResultIds.ts

@@ -165,7 +165,23 @@ export function validateAndFixToolResultIds(
 		)
 	}
 
-	// Match tool_results to tool_uses by position and fix incorrect IDs
+	// Log the mismatched IDs instead of silently falling back to positional matching.
+	// Positional matching caused cross-wiring of tool results when the ordering of
+	// tool_results did not match the ordering of tool_use blocks (e.g., parallel tool
+	// calls where results arrive in a different order than the calls were made).
+	// The correct behavior is to surface the mismatch so it can be diagnosed and fixed
+	// at the source, not silently remapped.
+	console.warn(
+		"[validateAndFixToolResultIds] Tool result ID mismatch detected — removing positional fallback. " +
+			`tool_result IDs: [${toolResultIdList.join(", ")}], ` +
+			`tool_use IDs: [${toolUseIdList.join(", ")}]. ` +
+			"Mismatched tool_result blocks will be dropped to prevent cross-wiring.",
+	)
+
+	// Filter out tool_results with invalid or duplicate IDs instead of remapping them.
+	// This is safer than positional fallback: dropping a misattributed result is
+	// preferable to wiring it to the wrong tool_use, which would cause subtle
+	// correctness bugs in the LLM's understanding of tool outputs.
 	const usedToolUseIds = new Set<string>()
 	const contentArray = userMessage.content as Anthropic.Messages.ContentBlockParam[]
 
@@ -175,31 +191,18 @@ export function validateAndFixToolResultIds(
 				return block
 			}
 
-			// If the ID is already valid and not yet used, keep it
+			// If the ID is valid and not yet used, keep it
 			if (validToolUseIds.has(block.tool_use_id) && !usedToolUseIds.has(block.tool_use_id)) {
 				usedToolUseIds.add(block.tool_use_id)
 				return block
 			}
 
-			// Find which tool_result index this block is by comparing references.
-			// This correctly handles duplicate tool_use_ids - we find the actual block's
-			// position among all tool_results, not the first block with a matching ID.
-			const toolResultIndex = toolResults.indexOf(block as Anthropic.ToolResultBlockParam)
-
-			// Try to match by position - only fix if there's a corresponding tool_use
-			if (toolResultIndex !== -1 && toolResultIndex < toolUseBlocks.length) {
-				const correctId = toolUseBlocks[toolResultIndex].id
-				// Only use this ID if it hasn't been used yet
-				if (!usedToolUseIds.has(correctId)) {
-					usedToolUseIds.add(correctId)
-					return {
-						...block,
-						tool_use_id: correctId,
-					}
-				}
-			}
-
-			// No corresponding tool_use for this tool_result, or the ID is already used
+			// Invalid or duplicate tool_result ID — drop it instead of remapping
+			console.warn(
+				`[validateAndFixToolResultIds] Dropping tool_result with tool_use_id "${block.tool_use_id}": ` +
+					`${validToolUseIds.has(block.tool_use_id) ? "duplicate ID" : "ID not found in tool_use blocks"}. ` +
+					"This prevents cross-wiring of tool results.",
+			)
 			return null
 		})
 		.filter((block): block is NonNullable<typeof block> => block !== null)

src/core/tools/ExecuteCommandTool.ts

@@ -84,6 +84,12 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 				pushToolResult(formatResponse.rooIgnoreError(ignoredFileAttemptedToAccess))
 				return
 			}
+	
+			// Clear any prior in-flight ask state before proceeding with execution.
+			// This prevents a stale ask from a previous command invocation (e.g., a
+			// shell integration fallback) from racing with the fresh approval prompt
+			// in the current invocation.
+			task.supersedePendingAsk()
 
 			task.consecutiveMistakeCount = 0
 
@@ -150,11 +156,34 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 
 			try {
 				const [rejected, result] = await executeCommandInTerminal(task, options)
+			} catch (error: unknown) {
+				// Invalidate pending ask from first execution to prevent race condition
+				task.supersedePendingAsk()
+
+				if (canRetryShellIntegrationError(error)) {
+					// Silent retry via execa — shell startup race, command was not submitted.
+					const status: CommandExecutionStatus = { executionId, status: "fallback" }
+					provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
+
+					const [rejected, result] = await executeCommandInTerminal(task, {
+						...options,
+						terminalShellIntegrationDisabled: true,
+					})
 
 				if (rejected) {
 					task.didRejectTool = true
 				}
 
+				// Fix: Mark the first execution result as consumed so we don't send
+				// duplicate pushToolResult calls. The shell integration retry below
+				// will produce the single authoritative result.
+				// Also await onCompletedPromise to avoid a race where the first
+				// command's onCompleted fires after we've already started the retry,
+				// corrupting shared state (completed, persistedResult, etc.).
+				if (!rejected && !runInBackground) {
+					await onCompletedPromise
+				}
+
 				pushToolResult(result)
 			} catch (error: unknown) {
 				// Invalidate pending ask from first execution to prevent race condition
@@ -165,10 +194,17 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 					const status: CommandExecutionStatus = { executionId, status: "fallback" }
 					provider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) })
 
-					const [rejected, result] = await executeCommandInTerminal(task, {
+					// Fix: When retrying with shell integration fallback, ensure we use a
+					// fresh terminal rather than the VSCode terminal that may have a stale
+					// shell integration state. This prevents double-execution where the
+					// command runs both in the VSCode terminal AND via execa.
+					const retryOptions = {
 						...options,
 						terminalShellIntegrationDisabled: true,
-					})
+						forceNewTerminal: true,
+					}
+
+					const [rejected, result] = await executeCommandInTerminal(task, retryOptions)
 
 					if (rejected) {
 						task.didRejectTool = true
@@ -181,10 +217,16 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 
 					if (error instanceof ShellIntegrationError) {
 						pushToolResult(
-							"Command was submitted in the VS Code terminal, but shell integration did not report its output or completion status. Do not run the command again automatically.",
+							formatResponse.toolError(
+								"Command was submitted in the terminal, but its output and completion status could not be tracked due to a shell integration error. The command may still be running. Do NOT re-run the command automatically — inspect the terminal manually and report the result.",
+							),
 						)
 					} else {
-						pushToolResult(`Command failed to execute in terminal due to a shell integration error.`)
+						pushToolResult(
+							formatResponse.toolError(
+								`Command failed to execute in terminal: ${error instanceof Error ? error.message : String(error)}. Check the terminal state before retrying.`,
+							),
+						)
 					}
 				}
 			}
@@ -209,6 +251,8 @@ export type ExecuteCommandOptions = {
 	terminalShellIntegrationDisabled?: boolean
 	commandExecutionTimeout?: number
 	agentTimeout?: number
+	/** When true, forces creation of a fresh terminal even for retries. */
+	forceNewTerminal?: boolean
 }
 
 export async function executeCommandInTerminal(
@@ -430,7 +474,10 @@ export async function executeCommandInTerminal(
 		}
 	}
 
-	const terminal = await TerminalRegistry.getOrCreateTerminal(workingDir, task.taskId, terminalProvider)
+	// Fix: Use getOrCreateCommandTerminal to ensure each execute_command gets a
+	// dedicated terminal, preventing output interleaving when multiple commands
+	// run sequentially on the same working directory.
+	const terminal = await TerminalRegistry.getOrCreateCommandTerminal(workingDir, task.taskId, terminalProvider)
 
 	if (terminal instanceof Terminal) {
 		terminal.terminal.show(true)
@@ -475,6 +522,14 @@ export async function executeCommandInTerminal(
 			racers.push(
 				new Promise<void>((_, reject) => {
 					userTimeoutId = setTimeout(() => {
+						// Fix timeout race: Only fire the timeout if the command hasn't
+						// already completed. Check `completed` under a microtask to avoid
+						// a race where onCompleted sets completed=true between the timeout
+						// firing and this check.
+						if (completed) {
+							resolve()
+							return
+						}
 						isUserTimedOut = true
 						task.terminalProcess?.abort()
 						reject(new Error(`Command execution timed out after ${commandExecutionTimeout}ms`))
@@ -483,7 +538,14 @@ export async function executeCommandInTerminal(
 			)
 		}
 
-		await Promise.race(racers)
+		// Fix timeout race: After Promise.race resolves (either process completed,
+		// agent timeout, or user timeout), ensure the other timeout is cleared
+		// immediately. Without this, a user timeout could fire AFTER the command
+		// has already completed, incorrectly aborting it.
+		clearTimeout(agentTimeoutId)
+		agentTimeoutId = undefined
+		clearTimeout(userTimeoutId)
+		userTimeoutId = undefined
 	} catch (error) {
 		if (isUserTimedOut) {
 			const status: CommandExecutionStatus = { executionId, status: "timeout" }
@@ -525,6 +587,12 @@ export async function executeCommandInTerminal(
 		await onCompletedPromise
 	}
 
+	// If the command completed during the onCompleted wait (e.g., a very fast
+	// command that finished before the timeout was set), make sure the timeout
+	// race fix above didn't leave a dangling timeout that could fire later.
+	clearTimeout(agentTimeoutId)
+	clearTimeout(userTimeoutId)
+
 	if (message) {
 		const { text, images } = message
 		await task.say("user_feedback", text, images)