fix: prevent tool call mixing and retry storms#974
Conversation
📝 WalkthroughWalkthroughTool execution now limits retry storms, reports skipped rejected tools, rejects unsafe result-ID remapping, and isolates command terminals while tightening fallback and timeout cleanup. ChangesTool execution reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)src/core/tools/ExecuteCommandTool.tsFile contains syntax errors that prevent linting: Line 188: Expected a statement but instead found 'catch (error: unknown)'.; Line 235: Expected a statement but instead found 'catch (error)'.; Line 241: Expected a catch clause but instead found 'override'.; Line 241: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 241: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 241: expected ... [truncated 929 characters] ... of modules.; Line 553: 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
What this PR fixesThis PR addresses two interrelated problems in sequential tool execution: Problem 1: Tool Call Results Get Mixed UpWhen the LLM returns multiple tool calls in one response, three bugs cause outputs to be assigned to the wrong tools:
Problem 2: Retry Storms Waste 3-5x Tokens
ImpactWithout these fixes, a single command failure cascades into 3-5 retry attempts, wasting ~4,000 tokens per failed task. Full analysis |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/tools/ExecuteCommandTool.ts (1)
523-548: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix runtime crash due to missing
resolveparameter and missingPromise.race().There are two critical failures in this code block:
- The
new Promise<void>((_, reject) =>executor does not defineresolvein its parameters, but it callsresolve()synchronously inside thesetTimeout. This will throw aReferenceErrorat runtime.- The code pushes the promises to the
racersarray but completely fails toawait Promise.race(racers). Without thisawait, the block immediately exits, instantly clearing the timeouts and failing to wait for the command to finish.🐛 Proposed fix for the timeout race and missing await
- new Promise<void>((_, reject) => { + new Promise<void>((resolve, 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`)) }, commandExecutionTimeout) }), ) } + 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🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/ExecuteCommandTool.ts` around lines 523 - 548, Update the timeout promise executor in the command execution flow to accept and use the resolve callback when completed is already true, preventing the undefined resolve ReferenceError. Before clearing agentTimeoutId and userTimeoutId, await Promise.race(racers) so the flow waits for command completion or either timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/assistant-message/presentAssistantMessage.ts`:
- Around line 74-77: Update presentAssistantMessage so batchSkipCount is reset
only when processing the first block of the batch, not on recursive calls for
subsequent blocks. Guard the reset using the existing block or batch position
indicator, preserving accumulated counts so nextSkipCount reflects prior tool
evaluations.
In `@src/core/task/Task.ts`:
- Around line 4774-4819: The retry-budget helpers are unused, so retries are
never blocked. Wire checkToolRetryBudget into the tool retry path before
allowing a retry, and invoke recordSuccessfulToolUse after successful tool
completion using the same tool name; otherwise remove both helpers and their
state if the budget is not intended to be enforced.
In `@src/core/tools/ExecuteCommandTool.ts`:
- Around line 157-189: Remove the duplicated malformed catch block within the
execute flow in ExecuteCommandTool, retaining only the valid error-handling path
for executeCommandInTerminal. Delete references that do not belong to this
scope, including onCompletedPromise and the duplicate retry/result logic, and
ensure the surrounding try/catch structure is syntactically valid.
---
Outside diff comments:
In `@src/core/tools/ExecuteCommandTool.ts`:
- Around line 523-548: Update the timeout promise executor in the command
execution flow to accept and use the resolve callback when completed is already
true, preventing the undefined resolve ReferenceError. Before clearing
agentTimeoutId and userTimeoutId, await Promise.race(racers) so the flow waits
for command completion or either timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff2acf02-071b-4543-afc9-df397726ec7e
📒 Files selected for processing (6)
src/core/assistant-message/presentAssistantMessage.tssrc/core/task/Task.tssrc/core/task/validateToolResultIds.tssrc/core/tools/ExecuteCommandTool.tssrc/core/tools/ToolRepetitionDetector.tssrc/integrations/terminal/TerminalRegistry.ts
| // 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 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix batchSkipCount reset to prevent it from resetting on every block.
Because presentAssistantMessage recursively calls itself to process each subsequent block, resetting batchSkipCount here causes it to be 0 for every single tool evaluation. This breaks the skip counting logic, meaning nextSkipCount will always incorrectly evaluate to 1.
Only reset the counter at the start of the entire batch (i.e., when processing the first block).
🐛 Proposed fix for the skip counter
// 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 === 0) {
+ ;(cline as any).batchSkipCount = 0
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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 | |
| // Reset batch skip counter at the start of each present cycle. | |
| // This ensures the count is fresh for each batch of tool calls. | |
| if (cline.currentStreamingContentIndex === 0) { | |
| ;(cline as any).batchSkipCount = 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/assistant-message/presentAssistantMessage.ts` around lines 74 - 77,
Update presentAssistantMessage so batchSkipCount is reset only when processing
the first block of the batch, not on recursive calls for subsequent blocks.
Guard the reset using the existing block or batch position indicator, preserving
accumulated counts so nextSkipCount reflects prior tool evaluations.
| /** | ||
| * 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) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of the retry budget methods across the codebase.
rg -n 'checkToolRetryBudget|recordSuccessfulToolUse'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files mentioning retry budget or tool retries\n'
rg -n -i 'retry budget|toolRetryBudget|MAX_TOOL_RETRY_BUDGET|recordSuccessfulToolUse|checkToolRetryBudget|retry.*tool' src || true
printf '\n## Task.ts context around the methods\n'
FILE='src/core/task/Task.ts'
if [ -f "$FILE" ]; then
line=$(rg -n 'checkToolRetryBudget|recordSuccessfulToolUse' "$FILE" | head -n 1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-40)); [ "$start" -lt 1 ] && start=1
end=$((line+80))
sed -n "${start},${end}p" "$FILE"
fi
fi
printf '\n## presentAssistantMessage.ts references\n'
if [ -f 'src/core/task/presentAssistantMessage.ts' ]; then
rg -n -i 'retry|toolRetryBudget|checkToolRetryBudget|recordSuccessfulToolUse|tool' src/core/task/presentAssistantMessage.ts || true
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 6904
Wire the retry budget into the tool execution path
These helpers are only defined here; nothing in the task flow calls them, so the retry budget never blocks retries. Hook them into the tool retry/success path, or remove the dead code if it is no longer needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/task/Task.ts` around lines 4774 - 4819, The retry-budget helpers are
unused, so retries are never blocked. Wire checkToolRetryBudget into the tool
retry path before allowing a retry, and invoke recordSuccessfulToolUse after
successful tool completion using the same tool name; otherwise remove both
helpers and their state if the budget is not intended to be enforced.
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicated and malformed try-catch block.
There is a severe syntax error here caused by a botched git merge. Lines 159-188 duplicate the catch block and contain invalid references (such as onCompletedPromise, which belongs inside executeCommandInTerminal, not execute). The invalid syntax } catch () { ... } catch () { completely breaks parser execution and surfaces upstream as static analysis errors.
🐛 Proposed fix for the merge conflict
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) {
+
+ if (rejected) {
+ task.didRejectTool = true
+ }
+
+ pushToolResult(result)
+ } catch (error: unknown) {
// Invalidate pending ask from first execution to prevent race condition
task.supersedePendingAsk()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| try { | |
| const [rejected, result] = await executeCommandInTerminal(task, options) | |
| if (rejected) { | |
| task.didRejectTool = true | |
| } | |
| pushToolResult(result) | |
| } catch (error: unknown) { | |
| // Invalidate pending ask from first execution to prevent race condition | |
| task.supersedePendingAsk() |
🧰 Tools
🪛 Biome (2.5.3)
[error] 188-188: Expected a statement but instead found 'catch (error: unknown)'.
(parse)
🪛 GitHub Actions: Code QA Roo Code / 0_compile.txt
[error] 188-188: ESLint parsing error during linting. 'try' expected.
🪛 GitHub Actions: Code QA Roo Code / compile
[error] 188-188: ESLint failed with parsing error: "'try' expected".
🪛 GitHub Actions: E2E Tests (Mocked) / 0_e2e-mock.txt
[error] 188-188: esbuild: ERROR: Unexpected "catch" (Build failed with 1 error).
🪛 GitHub Actions: E2E Tests (Mocked) / e2e-mock
[error] 188-188: esbuild failed: ERROR: Unexpected "catch" (core/tools/ExecuteCommandTool.ts:188:5).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/tools/ExecuteCommandTool.ts` around lines 157 - 189, Remove the
duplicated malformed catch block within the execute flow in ExecuteCommandTool,
retaining only the valid error-handling path for executeCommandInTerminal.
Delete references that do not belong to this scope, including onCompletedPromise
and the duplicate retry/result logic, and ensure the surrounding try/catch
structure is syntactically valid.
1 participant
Summary
Fixes 3 root causes of tool call mixing and retry storms in sequential tool execution:
Fix 1: Tool Call Mixing (3 files)
Fix 2: Retry Storm Prevention (2 files)
Fix 3: Retry Budget (1 file)
Closes #6
Summary by CodeRabbit
Bug Fixes
User Experience