fix: correct tool repetition detector to not block first tool call when limit is 1 (#6836)

Co-authored-by: Roo Code <[email protected]>

Author: roomote[bot]

src/core/tools/ToolRepetitionDetector.ts

@@ -39,7 +39,7 @@ export class ToolRepetitionDetector {
 		if (this.previousToolCallJson === currentToolCallJson) {
 			this.consecutiveIdenticalToolCallCount++
 		} else {
-			this.consecutiveIdenticalToolCallCount = 1 // Start with 1 for the first occurrence
+			this.consecutiveIdenticalToolCallCount = 0 // Reset to 0 for a new tool
 			this.previousToolCallJson = currentToolCallJson
 		}
 

src/core/tools/__tests__/ToolRepetitionDetector.spec.ts

@@ -32,30 +32,38 @@ describe("ToolRepetitionDetector", () => {
 			const detector = new ToolRepetitionDetector()
 			// We'll verify this through behavior in subsequent tests
 
-			// First call (counter = 1)
+			// First call (counter = 0)
 			const result1 = detector.check(createToolUse("test", "test-tool"))
 			expect(result1.allowExecution).toBe(true)
 
-			// Second identical call (counter = 2)
+			// Second identical call (counter = 1)
 			const result2 = detector.check(createToolUse("test", "test-tool"))
 			expect(result2.allowExecution).toBe(true)
 
-			// Third identical call (counter = 3) reaches the default limit
+			// Third identical call (counter = 2)
 			const result3 = detector.check(createToolUse("test", "test-tool"))
-			expect(result3.allowExecution).toBe(false)
+			expect(result3.allowExecution).toBe(true)
+
+			// Fourth identical call (counter = 3) reaches the default limit
+			const result4 = detector.check(createToolUse("test", "test-tool"))
+			expect(result4.allowExecution).toBe(false)
 		})
 
 		it("should use the custom limit when provided", () => {
 			const customLimit = 2
 			const detector = new ToolRepetitionDetector(customLimit)
 
-			// First call (counter = 1)
+			// First call (counter = 0)
 			const result1 = detector.check(createToolUse("test", "test-tool"))
 			expect(result1.allowExecution).toBe(true)
 
-			// Second identical call (counter = 2) reaches the custom limit
+			// Second identical call (counter = 1)
 			const result2 = detector.check(createToolUse("test", "test-tool"))
-			expect(result2.allowExecution).toBe(false)
+			expect(result2.allowExecution).toBe(true)
+
+			// Third identical call (counter = 2) reaches the custom limit
+			const result3 = detector.check(createToolUse("test", "test-tool"))
+			expect(result3.allowExecution).toBe(false)
 		})
 	})
 
@@ -97,17 +105,21 @@ describe("ToolRepetitionDetector", () => {
 		it("should allow execution when repetition is below limit and block when limit reached", () => {
 			const detector = new ToolRepetitionDetector(3)
 
-			// First call (counter = 1)
+			// First call (counter = 0)
 			const result1 = detector.check(createToolUse("repeat", "repeat-tool"))
 			expect(result1.allowExecution).toBe(true)
 
-			// Second identical call (counter = 2)
+			// Second identical call (counter = 1)
 			const result2 = detector.check(createToolUse("repeat", "repeat-tool"))
 			expect(result2.allowExecution).toBe(true)
 
-			// Third identical call (counter = 3) reaches limit
+			// Third identical call (counter = 2)
 			const result3 = detector.check(createToolUse("repeat", "repeat-tool"))
-			expect(result3.allowExecution).toBe(false)
+			expect(result3.allowExecution).toBe(true)
+
+			// Fourth identical call (counter = 3) reaches limit
+			const result4 = detector.check(createToolUse("repeat", "repeat-tool"))
+			expect(result4.allowExecution).toBe(false)
 		})
 	})
 
@@ -116,13 +128,16 @@ describe("ToolRepetitionDetector", () => {
 		it("should block execution when repetition reaches the limit", () => {
 			const detector = new ToolRepetitionDetector(3)
 
-			// First call (counter = 1)
+			// First call (counter = 0)
+			detector.check(createToolUse("repeat", "repeat-tool"))
+
+			// Second identical call (counter = 1)
 			detector.check(createToolUse("repeat", "repeat-tool"))
 
-			// Second identical call (counter = 2)
+			// Third identical call (counter = 2)
 			detector.check(createToolUse("repeat", "repeat-tool"))
 
-			// Third identical call (counter = 3) - should reach limit
+			// Fourth identical call (counter = 3) - should reach limit
 			const result = detector.check(createToolUse("repeat", "repeat-tool"))
 
 			expect(result.allowExecution).toBe(false)
@@ -136,6 +151,7 @@ describe("ToolRepetitionDetector", () => {
 
 			// Reach the limit
 			detector.check(createToolUse("repeat", "repeat-tool"))
+			detector.check(createToolUse("repeat", "repeat-tool"))
 			const limitResult = detector.check(createToolUse("repeat", "repeat-tool")) // This reaches limit
 			expect(limitResult.allowExecution).toBe(false)
 
@@ -152,6 +168,7 @@ describe("ToolRepetitionDetector", () => {
 
 			// Reach the limit with a specific tool
 			detector.check(createToolUse("problem", "problem-tool"))
+			detector.check(createToolUse("problem", "problem-tool"))
 			const limitResult = detector.check(createToolUse("problem", "problem-tool")) // This reaches limit
 			expect(limitResult.allowExecution).toBe(false)
 
@@ -165,13 +182,17 @@ describe("ToolRepetitionDetector", () => {
 
 			// Reach the limit
 			detector.check(createToolUse("repeat", "repeat-tool"))
+			detector.check(createToolUse("repeat", "repeat-tool"))
 			const limitResult = detector.check(createToolUse("repeat", "repeat-tool")) // This reaches limit
 			expect(limitResult.allowExecution).toBe(false)
 
 			// First call after reset
 			detector.check(createToolUse("repeat", "repeat-tool"))
 
-			// Second identical call (counter = 2) should reach limit again
+			// Second call after reset
+			detector.check(createToolUse("repeat", "repeat-tool"))
+
+			// Third identical call (counter = 2) should reach limit again
 			const result = detector.check(createToolUse("repeat", "repeat-tool"))
 			expect(result.allowExecution).toBe(false)
 			expect(result.askUser).toBeDefined()
@@ -186,6 +207,7 @@ describe("ToolRepetitionDetector", () => {
 
 			// Reach the limit
 			detector.check(createToolUse("test", toolName))
+			detector.check(createToolUse("test", toolName))
 			const result = detector.check(createToolUse("test", toolName))
 
 			expect(result.allowExecution).toBe(false)
@@ -201,7 +223,8 @@ describe("ToolRepetitionDetector", () => {
 			// Create an empty tool call - a tool with no parameters
 			// Use the empty tool directly in the check calls
 			detector.check(createToolUse("empty-tool", "empty-tool"))
-			const result = detector.check(createToolUse("empty-tool"))
+			detector.check(createToolUse("empty-tool", "empty-tool"))
+			const result = detector.check(createToolUse("empty-tool", "empty-tool"))
 
 			expect(result.allowExecution).toBe(false)
 			expect(result.askUser).toBeDefined()
@@ -210,7 +233,7 @@ describe("ToolRepetitionDetector", () => {
 		it("should handle different tool names with identical serialized JSON", () => {
 			const detector = new ToolRepetitionDetector(2)
 
-			// First, call with tool-name-1 twice to set up the counter
+			// First, call with tool-name-1 to set up the counter
 			const toolUse1 = createToolUse("tool-name-1", "tool-name-1", { param: "value" })
 			detector.check(toolUse1)
 
@@ -223,21 +246,25 @@ describe("ToolRepetitionDetector", () => {
 			;(detector as any).serializeToolUse = (tool: ToolUse) => {
 				// Use string comparison for the name since it's technically an enum
 				if (String(tool.name) === "tool-name-2") {
-					return (detector as any).serializeToolUse(toolUse1) // Return the same JSON as toolUse1
+					return originalSerialize.call(detector, toolUse1) // Return the same JSON as toolUse1
 				}
-				return originalSerialize(tool)
+				return originalSerialize.call(detector, tool)
 			}
 
-			// This should detect as a repetition now
-			const result = detector.check(toolUse2)
+			// Second call - this should be considered identical due to our mock
+			const result2 = detector.check(toolUse2)
+			expect(result2.allowExecution).toBe(true) // Still allowed (counter = 1)
+
+			// Third call - should be blocked (limit is 2)
+			const result3 = detector.check(toolUse2)
 
 			// Restore the original method
 			;(detector as any).serializeToolUse = originalSerialize
 
 			// Since we're directly manipulating the internal state for testing,
-			// we still expect it to consider this a repetition
-			expect(result.allowExecution).toBe(false)
-			expect(result.askUser).toBeDefined()
+			// we expect it to consider this a repetition
+			expect(result3.allowExecution).toBe(false)
+			expect(result3.askUser).toBeDefined()
 		})
 
 		it("should treat tools with same parameters in different order as identical", () => {
@@ -247,11 +274,13 @@ describe("ToolRepetitionDetector", () => {
 			const toolUse1 = createToolUse("same-tool", "same-tool", { a: "1", b: "2", c: "3" })
 			detector.check(toolUse1)
 
-			// Create tool with same parameters but in different order
+			// Second call with same parameters but in different order
 			const toolUse2 = createToolUse("same-tool", "same-tool", { c: "3", a: "1", b: "2" })
+			detector.check(toolUse2)
 
-			// This should still detect as a repetition due to canonical JSON with sorted keys
-			const result = detector.check(toolUse2)
+			// Third call - should be blocked (limit is 2)
+			const toolUse3 = createToolUse("same-tool", "same-tool", { b: "2", c: "3", a: "1" })
+			const result = detector.check(toolUse3)
 
 			// Since parameters are sorted alphabetically in the serialized JSON,
 			// these should be considered identical
@@ -262,44 +291,56 @@ describe("ToolRepetitionDetector", () => {
 
 	// ===== Explicit Nth Call Blocking tests =====
 	describe("explicit Nth call blocking behavior", () => {
-		it("should block on the 1st call for limit 1", () => {
+		it("should allow the 1st call but block on the 2nd call for limit 1", () => {
 			const detector = new ToolRepetitionDetector(1)
 
-			// First call (counter = 1) should be blocked
-			const result = detector.check(createToolUse("tool", "tool-name"))
+			// First call (counter = 0) should be allowed
+			const result1 = detector.check(createToolUse("tool", "tool-name"))
+			expect(result1.allowExecution).toBe(true)
+			expect(result1.askUser).toBeUndefined()
 
-			expect(result.allowExecution).toBe(false)
-			expect(result.askUser).toBeDefined()
+			// Second identical call (counter = 1) should be blocked
+			const result2 = detector.check(createToolUse("tool", "tool-name"))
+			expect(result2.allowExecution).toBe(false)
+			expect(result2.askUser).toBeDefined()
 		})
 
-		it("should block on the 2nd call for limit 2", () => {
+		it("should allow first 2 calls but block on the 3rd call for limit 2", () => {
 			const detector = new ToolRepetitionDetector(2)
 
-			// First call (counter = 1)
+			// First call (counter = 0)
 			const result1 = detector.check(createToolUse("tool", "tool-name"))
 			expect(result1.allowExecution).toBe(true)
 
-			// Second call (counter = 2) should be blocked
+			// Second identical call (counter = 1)
 			const result2 = detector.check(createToolUse("tool", "tool-name"))
-			expect(result2.allowExecution).toBe(false)
-			expect(result2.askUser).toBeDefined()
+			expect(result2.allowExecution).toBe(true)
+
+			// Third identical call (counter = 2) should be blocked
+			const result3 = detector.check(createToolUse("tool", "tool-name"))
+			expect(result3.allowExecution).toBe(false)
+			expect(result3.askUser).toBeDefined()
 		})
 
-		it("should block on the 3rd call for limit 3 (default)", () => {
+		it("should allow first 3 calls but block on the 4th call for limit 3 (default)", () => {
 			const detector = new ToolRepetitionDetector(3)
 
-			// First call (counter = 1)
+			// First call (counter = 0)
 			const result1 = detector.check(createToolUse("tool", "tool-name"))
 			expect(result1.allowExecution).toBe(true)
 
-			// Second call (counter = 2)
+			// Second identical call (counter = 1)
 			const result2 = detector.check(createToolUse("tool", "tool-name"))
 			expect(result2.allowExecution).toBe(true)
 
-			// Third call (counter = 3) should be blocked
+			// Third identical call (counter = 2)
 			const result3 = detector.check(createToolUse("tool", "tool-name"))
-			expect(result3.allowExecution).toBe(false)
-			expect(result3.askUser).toBeDefined()
+			expect(result3.allowExecution).toBe(true)
+
+			// Fourth identical call (counter = 3) should be blocked
+			const result4 = detector.check(createToolUse("tool", "tool-name"))
+			expect(result4.allowExecution).toBe(false)
+			expect(result4.askUser).toBeDefined()
 		})
 
 		it("should never block when limit is 0 (unlimited)", () => {
@@ -318,18 +359,18 @@ describe("ToolRepetitionDetector", () => {
 			const detector5 = new ToolRepetitionDetector(5)
 			const tool = createToolUse("tool", "tool-name")
 
-			// First 4 calls should be allowed
-			for (let i = 0; i < 4; i++) {
+			// First 5 calls should be allowed
+			for (let i = 0; i < 5; i++) {
 				const result = detector5.check(tool)
 				expect(result.allowExecution).toBe(true)
 				expect(result.askUser).toBeUndefined()
 			}
 
-			// 5th call should be blocked
-			const result5 = detector5.check(tool)
-			expect(result5.allowExecution).toBe(false)
-			expect(result5.askUser).toBeDefined()
-			expect(result5.askUser?.messageKey).toBe("mistake_limit_reached")
+			// 6th call should be blocked
+			const result6 = detector5.check(tool)
+			expect(result6.allowExecution).toBe(false)
+			expect(result6.askUser).toBeDefined()
+			expect(result6.askUser?.messageKey).toBe("mistake_limit_reached")
 		})
 
 		it("should reset counter after blocking and allow new attempts", () => {
@@ -339,7 +380,10 @@ describe("ToolRepetitionDetector", () => {
 			// First call allowed
 			expect(detector.check(tool).allowExecution).toBe(true)
 
-			// Second call should block (limit is 2)
+			// Second call allowed
+			expect(detector.check(tool).allowExecution).toBe(true)
+
+			// Third call should block (limit is 2)
 			const blocked = detector.check(tool)
 			expect(blocked.allowExecution).toBe(false)