Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2025
+96 −52
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/tools/ToolRepetitionDetector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix! The change from 1 to 0 is correct - the first occurrence shouldn't count as a repetition. Consider adding a comment here to explain the counting logic for future maintainers, something like: // Start from 0 since this is the first occurrence, not a repetition

this.previousToolCallJson = currentToolCallJson
}

Expand Down
146 changes: 95 additions & 51 deletions src/core/tools/__tests__/ToolRepetitionDetector.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})

Expand Down Expand Up @@ -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)
})
})

Expand All @@ -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)
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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()
Expand All @@ -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)
Expand All @@ -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()
Expand All @@ -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)

Expand All @@ -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", () => {
Expand All @@ -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
Expand All @@ -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)", () => {
Expand All @@ -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", () => {
Expand All @@ -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)

Expand Down
Loading