From 3a33cf6c52bd08dabd812d1a90b945df13cbcb9f Mon Sep 17 00:00:00 2001 From: Jpkoech30 Date: Tue, 21 Jul 2026 19:31:47 +0300 Subject: [PATCH] fix: prevent tool call mixing and retry storms --- .../presentAssistantMessage.ts | 14 +++- src/core/task/Task.ts | 83 +++++++++++++++++++ src/core/task/validateToolResultIds.ts | 45 +++++----- src/core/tools/ExecuteCommandTool.ts | 80 ++++++++++++++++-- src/core/tools/ToolRepetitionDetector.ts | 62 +++++++++++++- src/integrations/terminal/TerminalRegistry.ts | 39 +++++++++ 6 files changed, 293 insertions(+), 30 deletions(-) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index f71b5cc1bd..ec8cc87022 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/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({ diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 7bee3c5e14..9981870f1f 100644 --- a/src/core/task/Task.ts +++ b/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 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 = 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 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 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 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) + } } diff --git a/src/core/task/validateToolResultIds.ts b/src/core/task/validateToolResultIds.ts index a966d429ed..3c1cdadb77 100644 --- a/src/core/task/validateToolResultIds.ts +++ b/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() 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 => block !== null) diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index 3d6a44ef18..038ebace2a 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/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((_, 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) diff --git a/src/core/tools/ToolRepetitionDetector.ts b/src/core/tools/ToolRepetitionDetector.ts index 27592c5210..352a0482dc 100644 --- a/src/core/tools/ToolRepetitionDetector.ts +++ b/src/core/tools/ToolRepetitionDetector.ts @@ -11,6 +11,19 @@ export class ToolRepetitionDetector { private consecutiveIdenticalToolCallCount: number = 0 private readonly consecutiveIdenticalToolCallLimit: number + /** + * Sliding window of recent tool call signatures (last 5). + * Tracks the serialized form of each tool call to catch intermittent + * retry patterns where the same tool is called repeatedly with slight + * variations (e.g., different file paths in read_file, different + * regex patterns in search_files). + * + * The window detects patterns like: A, B, A, B, A (interleaved retries) + * which the simple consecutive checker would miss. + */ + private readonly slidingWindow: string[] = [] + private static readonly SLIDING_WINDOW_SIZE = 5 + /** * Creates a new ToolRepetitionDetector * @param limit The maximum number of identical consecutive tool calls allowed @@ -36,7 +49,7 @@ export class ToolRepetitionDetector { // Serialize the block to a canonical JSON string for comparison const currentToolCallJson = this.serializeToolUse(currentToolCallBlock) - // Compare with previous tool call + // Compare with previous tool call (exact consecutive match) if (this.previousToolCallJson === currentToolCallJson) { this.consecutiveIdenticalToolCallCount++ } else { @@ -63,6 +76,53 @@ export class ToolRepetitionDetector { } } + // Sliding window check: detect intermittent retry patterns where the same + // tool (possibly with slight variations) appears multiple times within the + // last N calls. This catches patterns like: A, B, A, B, A (interleaved + // retries) or A, C, A, D, A (same tool name, different params). + this.slidingWindow.push(currentToolCallJson) + if (this.slidingWindow.length > ToolRepetitionDetector.SLIDING_WINDOW_SIZE) { + this.slidingWindow.shift() + } + + // Count how many times this exact tool call appears in the window + const exactMatchCount = this.slidingWindow.filter((entry) => entry === currentToolCallJson).length + + // Count how many times this tool name appears in the window (with any params) + const toolNameCount = this.slidingWindow.filter((entry) => { + try { + const parsed = JSON.parse(entry) + return parsed.name === currentToolCallBlock.name + } catch { + return false + } + }).length + + // If the same exact tool call appears 3+ times in the last 5, or + // the same tool name (with different params) appears 4+ times, it's a retry storm. + const isRetryStorm = + (exactMatchCount >= 3) || + (toolNameCount >= 4 && this.consecutiveIdenticalToolCallLimit > 0) + + if (isRetryStorm) { + console.warn( + `[ToolRepetitionDetector] Retry storm detected for tool "${currentToolCallBlock.name}": ` + + `${exactMatchCount}x exact matches, ${toolNameCount}x name matches in last ` + + `${this.slidingWindow.length} calls.`, + ) + + // Clear the window to prevent cascading detections + this.slidingWindow.length = 0 + + return { + allowExecution: false, + askUser: { + messageKey: "mistake_limit_reached", + messageDetail: t("tools:toolRepetitionLimitReached", { toolName: currentToolCallBlock.name }), + }, + } + } + // Execution is allowed return { allowExecution: true } } diff --git a/src/integrations/terminal/TerminalRegistry.ts b/src/integrations/terminal/TerminalRegistry.ts index da4b3dd16d..136e107247 100644 --- a/src/integrations/terminal/TerminalRegistry.ts +++ b/src/integrations/terminal/TerminalRegistry.ts @@ -207,6 +207,27 @@ export class TerminalRegistry { return newTerminal } + /** + * Creates a new terminal for execute_command, bypassing the reuse logic. + * Each execute_command invocation gets its own dedicated terminal to prevent + * output interleaving when multiple commands run sequentially on the same + * working directory. + * + * @param cwd The working directory path + * @param taskId Optional task ID to associate with the terminal + * @param provider The terminal provider type + * @returns A new RooTerminal instance + */ + public static createCommandTerminal( + cwd: string, + taskId?: string, + provider: RooTerminalProvider = "vscode", + ): RooTerminal { + const terminal = this.createTerminal(cwd, provider) + terminal.taskId = taskId + return terminal + } + /** * Gets an existing terminal or creates a new one for the given working * directory. @@ -269,6 +290,24 @@ export class TerminalRegistry { return terminal } + /** + * Gets or creates a terminal for execute_command — always creates a new + * terminal to prevent output interleaving when multiple commands run + * sequentially on the same working directory. + * + * @param cwd The working directory path + * @param taskId Optional task ID to associate with the terminal + * @param provider The terminal provider type + * @returns A new RooTerminal instance + */ + public static async getOrCreateCommandTerminal( + cwd: string, + taskId?: string, + provider: RooTerminalProvider = "vscode", + ): Promise { + return this.createCommandTerminal(cwd, taskId, provider) + } + /** * Gets unretrieved output from a terminal process. *