mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-10 22:41:14 +00:00
fix: improve loop detection to avoid false positives for MCP tools
- Add response tracking to detect actual progress in tool execution - Implement tool-specific repetition limits (MCP tools: 50, default: 3) - Add progress detection that resets counter when responses differ - Support configuration for excluded tools and custom limits - Update tests to cover new progress detection functionality This addresses the issue where MCP tools that legitimately need multiple consecutive calls (e.g., reading streaming data) were incorrectly flagged as stuck in a loop.
This commit is contained in:
parent
2ca9eac9e0
commit
d88d96a194
3 changed files with 343 additions and 8 deletions
|
|
@ -304,6 +304,7 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
|
||||
// Track if we've already pushed a tool result for this tool call (native protocol only)
|
||||
let hasToolResult = false
|
||||
let lastToolResponse: string | undefined
|
||||
|
||||
// Determine protocol by checking if this tool call has an ID.
|
||||
// Native protocol tool calls ALWAYS have an ID (set when parsed from tool_call chunks).
|
||||
|
|
@ -339,6 +340,11 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
"(tool did not return anything)"
|
||||
}
|
||||
|
||||
// Track the response for progress detection
|
||||
lastToolResponse = resultContent
|
||||
// Update the detector with this response for future checks
|
||||
cline.toolRepetitionDetector.updateLastResponse(resultContent)
|
||||
|
||||
// Add tool_result with text content only
|
||||
cline.userMessageContent.push({
|
||||
type: "tool_result",
|
||||
|
|
@ -357,11 +363,26 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
cline.userMessageContent.push({ type: "text", text: `${toolDescription()} Result:` })
|
||||
|
||||
if (typeof content === "string") {
|
||||
// Track the response for progress detection
|
||||
lastToolResponse = content || "(tool did not return anything)"
|
||||
// Update the detector with this response for future checks
|
||||
cline.toolRepetitionDetector.updateLastResponse(lastToolResponse)
|
||||
|
||||
cline.userMessageContent.push({
|
||||
type: "text",
|
||||
text: content || "(tool did not return anything)",
|
||||
})
|
||||
} else {
|
||||
// For array responses, track text content
|
||||
const textContent = content
|
||||
.filter((item) => item.type === "text")
|
||||
.map((item) => (item as Anthropic.TextBlockParam).text)
|
||||
.join("\n")
|
||||
if (textContent) {
|
||||
lastToolResponse = textContent
|
||||
// Update the detector with this response for future checks
|
||||
cline.toolRepetitionDetector.updateLastResponse(textContent)
|
||||
}
|
||||
cline.userMessageContent.push(...content)
|
||||
}
|
||||
}
|
||||
|
|
@ -514,7 +535,8 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
// Check for identical consecutive tool calls.
|
||||
if (!block.partial) {
|
||||
// Use the detector to check for repetition, passing the ToolUse
|
||||
// block directly.
|
||||
// block directly and the last tool response for progress detection.
|
||||
// The lastToolResponse from previous execution is already tracked in the detector
|
||||
const repetitionCheck = cline.toolRepetitionDetector.check(block)
|
||||
|
||||
// If execution is not allowed, notify user and break.
|
||||
|
|
|
|||
|
|
@ -2,21 +2,50 @@ import stringify from "safe-stable-stringify"
|
|||
import { ToolUse } from "../../shared/tools"
|
||||
import { t } from "../../i18n"
|
||||
|
||||
/**
|
||||
* Configuration for tool-specific repetition limits
|
||||
*/
|
||||
interface ToolSpecificConfig {
|
||||
/** Tool names that should be excluded from repetition detection */
|
||||
excludedTools?: string[]
|
||||
/** Custom limits for specific tools (key is tool name, value is limit) */
|
||||
toolLimits?: Record<string, number>
|
||||
}
|
||||
|
||||
/**
|
||||
* Class for detecting consecutive identical tool calls
|
||||
* to prevent the AI from getting stuck in a loop.
|
||||
*
|
||||
* Enhanced to detect actual progress by tracking tool responses
|
||||
* and providing special handling for tools that legitimately need
|
||||
* multiple consecutive calls (e.g., MCP tools reading streaming data).
|
||||
*/
|
||||
export class ToolRepetitionDetector {
|
||||
private previousToolCallJson: string | null = null
|
||||
private consecutiveIdenticalToolCallCount: number = 0
|
||||
private readonly consecutiveIdenticalToolCallLimit: number
|
||||
private lastToolResponse: string | null = null
|
||||
private responseHistory: string[] = []
|
||||
private readonly maxResponseHistorySize = 10
|
||||
private readonly toolSpecificConfig: ToolSpecificConfig
|
||||
|
||||
/**
|
||||
* Creates a new ToolRepetitionDetector
|
||||
* @param limit The maximum number of identical consecutive tool calls allowed
|
||||
* @param toolSpecificConfig Configuration for tool-specific behavior
|
||||
*/
|
||||
constructor(limit: number = 3) {
|
||||
constructor(limit: number = 3, toolSpecificConfig: ToolSpecificConfig = {}) {
|
||||
this.consecutiveIdenticalToolCallLimit = limit
|
||||
this.toolSpecificConfig = {
|
||||
excludedTools: toolSpecificConfig.excludedTools || [],
|
||||
toolLimits: {
|
||||
// MCP tools often need more consecutive calls for legitimate streaming/chunked data
|
||||
use_mcp_tool: 50,
|
||||
access_mcp_resource: 50,
|
||||
// Override with any user-provided limits
|
||||
...(toolSpecificConfig.toolLimits || {}),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -24,15 +53,24 @@ export class ToolRepetitionDetector {
|
|||
* and determines if execution should be allowed
|
||||
*
|
||||
* @param currentToolCallBlock ToolUse object representing the current tool call
|
||||
* @param toolResponse Optional tool response from the last execution to track progress
|
||||
* @returns Object indicating if execution is allowed and a message to show if not
|
||||
*/
|
||||
public check(currentToolCallBlock: ToolUse): {
|
||||
public check(
|
||||
currentToolCallBlock: ToolUse,
|
||||
toolResponse?: string,
|
||||
): {
|
||||
allowExecution: boolean
|
||||
askUser?: {
|
||||
messageKey: string
|
||||
messageDetail: string
|
||||
}
|
||||
} {
|
||||
// Check if this tool is excluded from repetition detection
|
||||
if (this.toolSpecificConfig.excludedTools?.includes(currentToolCallBlock.name)) {
|
||||
return { allowExecution: true }
|
||||
}
|
||||
|
||||
// Browser scroll actions should not be subject to repetition detection
|
||||
// as they are frequently needed for navigating through web pages
|
||||
if (this.isBrowserScrollAction(currentToolCallBlock)) {
|
||||
|
|
@ -40,6 +78,16 @@ export class ToolRepetitionDetector {
|
|||
return { allowExecution: true }
|
||||
}
|
||||
|
||||
// Special handling for MCP tools that may be reading streaming data
|
||||
if (this.isMcpStreamingTool(currentToolCallBlock)) {
|
||||
// Check if we're making progress by comparing responses
|
||||
if (this.isShowingProgress(toolResponse)) {
|
||||
// Reset the counter since we're making progress
|
||||
this.consecutiveIdenticalToolCallCount = 0
|
||||
return { allowExecution: true }
|
||||
}
|
||||
}
|
||||
|
||||
// Serialize the block to a canonical JSON string for comparison
|
||||
const currentToolCallJson = this.serializeToolUse(currentToolCallBlock)
|
||||
|
||||
|
|
@ -49,23 +97,45 @@ export class ToolRepetitionDetector {
|
|||
} else {
|
||||
this.consecutiveIdenticalToolCallCount = 0 // Reset to 0 for a new tool
|
||||
this.previousToolCallJson = currentToolCallJson
|
||||
// Clear response history when switching to a different tool
|
||||
this.responseHistory = []
|
||||
this.lastToolResponse = null
|
||||
}
|
||||
|
||||
// Update response tracking
|
||||
if (toolResponse) {
|
||||
this.responseHistory.push(toolResponse)
|
||||
if (this.responseHistory.length > this.maxResponseHistorySize) {
|
||||
this.responseHistory.shift()
|
||||
}
|
||||
this.lastToolResponse = toolResponse
|
||||
}
|
||||
|
||||
// Get the appropriate limit for this tool
|
||||
const toolLimit = this.getToolLimit(currentToolCallBlock.name)
|
||||
|
||||
// Check if limit is reached (0 means unlimited)
|
||||
if (
|
||||
this.consecutiveIdenticalToolCallLimit > 0 &&
|
||||
this.consecutiveIdenticalToolCallCount >= this.consecutiveIdenticalToolCallLimit
|
||||
) {
|
||||
if (toolLimit > 0 && this.consecutiveIdenticalToolCallCount >= toolLimit) {
|
||||
// For MCP tools, provide a more helpful message
|
||||
const isMcpTool = this.isMcpStreamingTool(currentToolCallBlock)
|
||||
const messageDetail = isMcpTool
|
||||
? t("tools:toolRepetitionLimitReached", { toolName: currentToolCallBlock.name }) +
|
||||
" This may be a false positive if the tool is legitimately reading streaming data. " +
|
||||
"Consider increasing the repetition limit for MCP tools in settings."
|
||||
: t("tools:toolRepetitionLimitReached", { toolName: currentToolCallBlock.name })
|
||||
|
||||
// Reset counters to allow recovery if user guides the AI past this point
|
||||
this.consecutiveIdenticalToolCallCount = 0
|
||||
this.previousToolCallJson = null
|
||||
this.responseHistory = []
|
||||
this.lastToolResponse = null
|
||||
|
||||
// Return result indicating execution should not be allowed
|
||||
return {
|
||||
allowExecution: false,
|
||||
askUser: {
|
||||
messageKey: "mistake_limit_reached",
|
||||
messageDetail: t("tools:toolRepetitionLimitReached", { toolName: currentToolCallBlock.name }),
|
||||
messageDetail,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
|
@ -74,6 +144,67 @@ export class ToolRepetitionDetector {
|
|||
return { allowExecution: true }
|
||||
}
|
||||
|
||||
/**
|
||||
* Updates the last tool response for progress tracking
|
||||
* @param response The response from the last tool execution
|
||||
*/
|
||||
public updateLastResponse(response: string): void {
|
||||
this.lastToolResponse = response
|
||||
this.responseHistory.push(response)
|
||||
if (this.responseHistory.length > this.maxResponseHistorySize) {
|
||||
this.responseHistory.shift()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if the tool responses show progress is being made
|
||||
* @param currentResponse The current tool response to check
|
||||
* @returns true if progress is detected
|
||||
*/
|
||||
private isShowingProgress(currentResponse?: string): boolean {
|
||||
if (!currentResponse || !this.lastToolResponse) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Check if the response is different from the last one
|
||||
if (currentResponse !== this.lastToolResponse) {
|
||||
return true
|
||||
}
|
||||
|
||||
// Check if we have varying responses in history (not all the same)
|
||||
if (this.responseHistory.length > 1) {
|
||||
const uniqueResponses = new Set(this.responseHistory)
|
||||
// If we have more than one unique response, we're likely making progress
|
||||
return uniqueResponses.size > 1
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if a tool is an MCP streaming tool that may need many consecutive calls
|
||||
* @param toolUse The tool use to check
|
||||
* @returns true if it's an MCP tool that might stream data
|
||||
*/
|
||||
private isMcpStreamingTool(toolUse: ToolUse): boolean {
|
||||
// MCP tools that commonly need to read streaming/chunked data
|
||||
return toolUse.name === "use_mcp_tool" || toolUse.name === "access_mcp_resource"
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets the repetition limit for a specific tool
|
||||
* @param toolName The name of the tool
|
||||
* @returns The repetition limit for the tool
|
||||
*/
|
||||
private getToolLimit(toolName: string): number {
|
||||
// Check for tool-specific limit
|
||||
if (this.toolSpecificConfig.toolLimits && toolName in this.toolSpecificConfig.toolLimits) {
|
||||
return this.toolSpecificConfig.toolLimits[toolName]
|
||||
}
|
||||
// Fall back to default limit
|
||||
return this.consecutiveIdenticalToolCallLimit
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if a tool use is a browser scroll action
|
||||
*
|
||||
|
|
|
|||
|
|
@ -697,4 +697,186 @@ describe("ToolRepetitionDetector", () => {
|
|||
expect(result.askUser).toBeDefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe("Progress Detection for MCP Tools", () => {
|
||||
it("should reset counter when MCP tool shows progress through different responses", () => {
|
||||
const detector = new ToolRepetitionDetector(3)
|
||||
|
||||
const mcpTool = {
|
||||
type: "tool_use",
|
||||
name: "use_mcp_tool",
|
||||
params: {
|
||||
server_name: "test-server",
|
||||
tool_name: "read_output",
|
||||
},
|
||||
} as ToolUse
|
||||
|
||||
// First call
|
||||
expect(detector.check(mcpTool).allowExecution).toBe(true)
|
||||
detector.updateLastResponse("Output chunk 1: data...")
|
||||
|
||||
// Second call with different response - should reset counter
|
||||
detector.updateLastResponse("Output chunk 2: more data...")
|
||||
expect(detector.check(mcpTool).allowExecution).toBe(true)
|
||||
|
||||
// Third call with another different response - should still allow
|
||||
detector.updateLastResponse("Output chunk 3: even more data...")
|
||||
expect(detector.check(mcpTool).allowExecution).toBe(true)
|
||||
|
||||
// Fourth call with yet another different response - should still allow
|
||||
detector.updateLastResponse("Output chunk 4: final data...")
|
||||
expect(detector.check(mcpTool).allowExecution).toBe(true)
|
||||
})
|
||||
|
||||
it("should allow higher limits for MCP tools by default", () => {
|
||||
const detector = new ToolRepetitionDetector(3) // Default limit is 3
|
||||
|
||||
const mcpTool = {
|
||||
type: "tool_use",
|
||||
name: "use_mcp_tool",
|
||||
params: {
|
||||
server_name: "test-server",
|
||||
tool_name: "read_output",
|
||||
},
|
||||
} as ToolUse
|
||||
|
||||
// MCP tools should have a much higher limit (50 by default)
|
||||
// Let's test up to 10 calls to verify it's not using the default 3
|
||||
for (let i = 1; i <= 10; i++) {
|
||||
const result = detector.check(mcpTool)
|
||||
expect(result.allowExecution).toBe(true) // Should allow all 10 calls
|
||||
}
|
||||
})
|
||||
|
||||
it("should block MCP tools when reaching their higher limit without progress", () => {
|
||||
const detector = new ToolRepetitionDetector(3, {
|
||||
toolLimits: {
|
||||
use_mcp_tool: 5, // Override default to a lower number for testing
|
||||
},
|
||||
})
|
||||
|
||||
const mcpTool = {
|
||||
type: "tool_use",
|
||||
name: "use_mcp_tool",
|
||||
params: {
|
||||
server_name: "test-server",
|
||||
tool_name: "read_output",
|
||||
},
|
||||
} as ToolUse
|
||||
|
||||
// First 5 calls should be allowed
|
||||
for (let i = 1; i <= 5; i++) {
|
||||
expect(detector.check(mcpTool).allowExecution).toBe(true)
|
||||
}
|
||||
|
||||
// 6th call should be blocked
|
||||
const result = detector.check(mcpTool)
|
||||
expect(result.allowExecution).toBe(false)
|
||||
expect(result.askUser?.messageDetail).toContain("use_mcp_tool")
|
||||
expect(result.askUser?.messageDetail).toContain("false positive")
|
||||
})
|
||||
|
||||
it("should exclude tools from repetition detection when configured", () => {
|
||||
const detector = new ToolRepetitionDetector(2, {
|
||||
excludedTools: ["execute_command"],
|
||||
})
|
||||
|
||||
const excludedTool = {
|
||||
type: "tool_use",
|
||||
name: "execute_command",
|
||||
params: {
|
||||
command: "ls -la",
|
||||
},
|
||||
} as ToolUse
|
||||
|
||||
// Should always allow execution for excluded tools
|
||||
for (let i = 1; i <= 10; i++) {
|
||||
expect(detector.check(excludedTool).allowExecution).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it("should track response history and detect progress patterns", () => {
|
||||
const detector = new ToolRepetitionDetector(3)
|
||||
|
||||
const tool = {
|
||||
type: "tool_use",
|
||||
name: "use_mcp_tool",
|
||||
params: {
|
||||
server_name: "test-server",
|
||||
},
|
||||
} as ToolUse
|
||||
|
||||
// Same response multiple times - no progress
|
||||
detector.updateLastResponse("Same output")
|
||||
expect(detector.check(tool).allowExecution).toBe(true)
|
||||
|
||||
detector.updateLastResponse("Same output")
|
||||
expect(detector.check(tool).allowExecution).toBe(true)
|
||||
|
||||
detector.updateLastResponse("Same output")
|
||||
expect(detector.check(tool).allowExecution).toBe(true)
|
||||
|
||||
// Now a different response - shows progress
|
||||
detector.updateLastResponse("Different output")
|
||||
expect(detector.check(tool).allowExecution).toBe(true) // Counter should reset
|
||||
})
|
||||
|
||||
it("should use custom tool limits when provided", () => {
|
||||
const detector = new ToolRepetitionDetector(3, {
|
||||
toolLimits: {
|
||||
read_file: 10,
|
||||
write_to_file: 1,
|
||||
},
|
||||
})
|
||||
|
||||
const readTool = {
|
||||
type: "tool_use",
|
||||
name: "read_file",
|
||||
params: { path: "test.txt" },
|
||||
} as ToolUse
|
||||
|
||||
const writeTool = {
|
||||
type: "tool_use",
|
||||
name: "write_to_file",
|
||||
params: { path: "test.txt", content: "data" },
|
||||
} as ToolUse
|
||||
|
||||
// read_file should allow up to 10 calls
|
||||
for (let i = 1; i <= 10; i++) {
|
||||
expect(detector.check(readTool).allowExecution).toBe(true)
|
||||
}
|
||||
expect(detector.check(readTool).allowExecution).toBe(false) // 11th call blocked
|
||||
|
||||
// write_to_file should allow only 1 call
|
||||
expect(detector.check(writeTool).allowExecution).toBe(true)
|
||||
expect(detector.check(writeTool).allowExecution).toBe(false) // 2nd call blocked
|
||||
})
|
||||
|
||||
it("should clear response history when switching tools", () => {
|
||||
const detector = new ToolRepetitionDetector(3)
|
||||
|
||||
const tool1 = {
|
||||
type: "tool_use",
|
||||
name: "use_mcp_tool",
|
||||
params: { server_name: "server1" },
|
||||
} as ToolUse
|
||||
|
||||
const tool2 = {
|
||||
type: "tool_use",
|
||||
name: "list_files",
|
||||
params: { path: "/" },
|
||||
} as ToolUse
|
||||
|
||||
// Set up response history for tool1
|
||||
detector.updateLastResponse("Response 1")
|
||||
detector.updateLastResponse("Response 2")
|
||||
expect(detector.check(tool1).allowExecution).toBe(true)
|
||||
|
||||
// Switch to tool2 - should clear response history
|
||||
expect(detector.check(tool2).allowExecution).toBe(true)
|
||||
|
||||
// Back to tool1 - should start fresh without previous response history
|
||||
expect(detector.check(tool1).allowExecution).toBe(true)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue