Просмотр исходного кода

fix: Include nativeArgs in tool repetition detection (#9377)

* fix: Include nativeArgs in tool repetition detection

Fixes false positive 'stuck in a loop' error for native protocol tools
like read_file that store parameters in nativeArgs instead of params.

Previously, the ToolRepetitionDetector only compared the params object,
which was empty for native protocol tools. This caused all read_file
calls to appear identical, triggering false loop detection even when
reading different files.

Changes:
- Updated serializeToolUse() to include nativeArgs in comparison
- Added comprehensive tests for native protocol scenarios
- Maintains backward compatibility with XML protocol tools

Closes: Issue reported in Discord about read_file loop detection

* Try to use safe-stable-stringify in the tool repetition detector

---------

Co-authored-by: Matt Rubens <[email protected]>
Daniel 4 месяцев назад
Родитель
Сommit
89068c4ff9

+ 10 - 1
pnpm-lock.yaml

@@ -773,6 +773,9 @@ importers:
       reconnecting-eventsource:
       reconnecting-eventsource:
         specifier: ^1.6.4
         specifier: ^1.6.4
         version: 1.6.4
         version: 1.6.4
+      safe-stable-stringify:
+        specifier: ^2.5.0
+        version: 2.5.0
       sanitize-filename:
       sanitize-filename:
         specifier: ^1.6.3
         specifier: ^1.6.3
         version: 1.6.3
         version: 1.6.3
@@ -8831,6 +8834,10 @@ packages:
     resolution: {integrity: sha512-x/+Cz4YrimQxQccJf5mKEbIa1NzeCRNI5Ecl/ekmlYaampdNLPalVyIcCZNNH3MvmqBugV5TMYZXv0ljslUlaw==}
     resolution: {integrity: sha512-x/+Cz4YrimQxQccJf5mKEbIa1NzeCRNI5Ecl/ekmlYaampdNLPalVyIcCZNNH3MvmqBugV5TMYZXv0ljslUlaw==}
     engines: {node: '>= 0.4'}
     engines: {node: '>= 0.4'}
 
 
+  [email protected]:
+    resolution: {integrity: sha512-b3rppTKm9T+PsVCBEOUR46GWI7fdOs00VKZ1+9c1EWDaDMvjQc6tUwuFyIprgGgTcWoVHSKrU8H31ZHA2e0RHA==}
+    engines: {node: '>=10'}
+
   [email protected]:
   [email protected]:
     resolution: {integrity: sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg==}
     resolution: {integrity: sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg==}
 
 
@@ -14066,7 +14073,7 @@ snapshots:
       sirv: 3.0.1
       sirv: 3.0.1
       tinyglobby: 0.2.14
       tinyglobby: 0.2.14
       tinyrainbow: 2.0.0
       tinyrainbow: 2.0.0
-      vitest: 3.2.4(@types/[email protected])(@types/node@24.2.1)(@vitest/[email protected])([email protected])([email protected])([email protected])([email protected])([email protected])
+      vitest: 3.2.4(@types/[email protected])(@types/node@20.17.50)(@vitest/[email protected])([email protected])([email protected])([email protected])([email protected])([email protected])
 
 
   '@vitest/[email protected]':
   '@vitest/[email protected]':
     dependencies:
     dependencies:
@@ -19361,6 +19368,8 @@ snapshots:
       es-errors: 1.3.0
       es-errors: 1.3.0
       is-regex: 1.2.1
       is-regex: 1.2.1
 
 
+  [email protected]: {}
+
   [email protected]: {}
   [email protected]: {}
 
 
   [email protected]:
   [email protected]:

+ 8 - 17
src/core/tools/ToolRepetitionDetector.ts

@@ -1,3 +1,4 @@
+import stringify from "safe-stable-stringify"
 import { ToolUse } from "../../shared/tools"
 import { ToolUse } from "../../shared/tools"
 import { t } from "../../i18n"
 import { t } from "../../i18n"
 
 
@@ -95,26 +96,16 @@ export class ToolRepetitionDetector {
 	 * @returns JSON string representation of the tool use with sorted parameter keys
 	 * @returns JSON string representation of the tool use with sorted parameter keys
 	 */
 	 */
 	private serializeToolUse(toolUse: ToolUse): string {
 	private serializeToolUse(toolUse: ToolUse): string {
-		// Create a new parameters object with alphabetically sorted keys
-		const sortedParams: Record<string, unknown> = {}
-
-		// Get parameter keys and sort them alphabetically
-		const sortedKeys = Object.keys(toolUse.params).sort()
-
-		// Populate the sorted parameters object in a type-safe way
-		for (const key of sortedKeys) {
-			if (Object.prototype.hasOwnProperty.call(toolUse.params, key)) {
-				sortedParams[key] = toolUse.params[key as keyof typeof toolUse.params]
-			}
+		const toolObject: Record<string, any> = {
+			name: toolUse.name,
+			params: toolUse.params,
 		}
 		}
 
 
-		// Create the object with the tool name and sorted parameters
-		const toolObject = {
-			name: toolUse.name,
-			parameters: sortedParams,
+		// Only include nativeArgs if it has content
+		if (toolUse.nativeArgs && Object.keys(toolUse.nativeArgs).length > 0) {
+			toolObject.nativeArgs = toolUse.nativeArgs
 		}
 		}
 
 
-		// Convert to a canonical JSON string
-		return JSON.stringify(toolObject)
+		return stringify(toolObject)
 	}
 	}
 }
 }

+ 135 - 0
src/core/tools/__tests__/ToolRepetitionDetector.spec.ts

@@ -562,4 +562,139 @@ describe("ToolRepetitionDetector", () => {
 			expect(result.askUser).toBeDefined()
 			expect(result.askUser).toBeDefined()
 		})
 		})
 	})
 	})
+
+	// ===== Native Protocol (nativeArgs) tests =====
+	describe("native protocol with nativeArgs", () => {
+		it("should differentiate read_file calls with different files in nativeArgs", () => {
+			const detector = new ToolRepetitionDetector(2)
+
+			// Create read_file tool use with nativeArgs (like native protocol does)
+			const readFile1: ToolUse = {
+				type: "tool_use",
+				name: "read_file" as ToolName,
+				params: {}, // Empty for native protocol
+				partial: false,
+				nativeArgs: {
+					files: [{ path: "file1.ts" }],
+				},
+			}
+
+			const readFile2: ToolUse = {
+				type: "tool_use",
+				name: "read_file" as ToolName,
+				params: {}, // Empty for native protocol
+				partial: false,
+				nativeArgs: {
+					files: [{ path: "file2.ts" }],
+				},
+			}
+
+			// First call with file1
+			expect(detector.check(readFile1).allowExecution).toBe(true)
+
+			// Second call with file2 - should be treated as different
+			expect(detector.check(readFile2).allowExecution).toBe(true)
+
+			// Third call with file1 again - should reset counter
+			expect(detector.check(readFile1).allowExecution).toBe(true)
+		})
+
+		it("should detect repetition when same files are read multiple times with nativeArgs", () => {
+			const detector = new ToolRepetitionDetector(2)
+
+			// Create identical read_file tool uses
+			const readFile: ToolUse = {
+				type: "tool_use",
+				name: "read_file" as ToolName,
+				params: {}, // Empty for native protocol
+				partial: false,
+				nativeArgs: {
+					files: [{ path: "same-file.ts" }],
+				},
+			}
+
+			// First call allowed
+			expect(detector.check(readFile).allowExecution).toBe(true)
+
+			// Second call allowed
+			expect(detector.check(readFile).allowExecution).toBe(true)
+
+			// Third identical call should be blocked (limit is 2)
+			const result = detector.check(readFile)
+			expect(result.allowExecution).toBe(false)
+			expect(result.askUser).toBeDefined()
+		})
+
+		it("should differentiate read_file calls with multiple files in different orders", () => {
+			const detector = new ToolRepetitionDetector(2)
+
+			const readFile1: ToolUse = {
+				type: "tool_use",
+				name: "read_file" as ToolName,
+				params: {},
+				partial: false,
+				nativeArgs: {
+					files: [{ path: "a.ts" }, { path: "b.ts" }],
+				},
+			}
+
+			const readFile2: ToolUse = {
+				type: "tool_use",
+				name: "read_file" as ToolName,
+				params: {},
+				partial: false,
+				nativeArgs: {
+					files: [{ path: "b.ts" }, { path: "a.ts" }],
+				},
+			}
+
+			// Different order should be treated as different calls
+			expect(detector.check(readFile1).allowExecution).toBe(true)
+			expect(detector.check(readFile2).allowExecution).toBe(true)
+		})
+
+		it("should handle tools with both params and nativeArgs", () => {
+			const detector = new ToolRepetitionDetector(2)
+
+			const tool1: ToolUse = {
+				type: "tool_use",
+				name: "execute_command" as ToolName,
+				params: { command: "ls" },
+				partial: false,
+				nativeArgs: {
+					command: "ls",
+					cwd: "/home/user",
+				},
+			}
+
+			const tool2: ToolUse = {
+				type: "tool_use",
+				name: "execute_command" as ToolName,
+				params: { command: "ls" },
+				partial: false,
+				nativeArgs: {
+					command: "ls",
+					cwd: "/home/admin",
+				},
+			}
+
+			// Different cwd in nativeArgs should make these different
+			expect(detector.check(tool1).allowExecution).toBe(true)
+			expect(detector.check(tool2).allowExecution).toBe(true)
+		})
+
+		it("should handle tools with only params (no nativeArgs)", () => {
+			const detector = new ToolRepetitionDetector(2)
+
+			const legacyTool = createToolUse("read_file", "read_file", { path: "test.txt" })
+
+			// Should work the same as before
+			expect(detector.check(legacyTool).allowExecution).toBe(true)
+			expect(detector.check(legacyTool).allowExecution).toBe(true)
+
+			const result = detector.check(legacyTool)
+			expect(result.allowExecution).toBe(false)
+			expect(result.askUser).toBeDefined()
+		})
+	})
 })
 })

+ 1 - 0
src/package.json

@@ -509,6 +509,7 @@
 		"puppeteer-chromium-resolver": "^24.0.0",
 		"puppeteer-chromium-resolver": "^24.0.0",
 		"puppeteer-core": "^23.4.0",
 		"puppeteer-core": "^23.4.0",
 		"reconnecting-eventsource": "^1.6.4",
 		"reconnecting-eventsource": "^1.6.4",
+		"safe-stable-stringify": "^2.5.0",
 		"sanitize-filename": "^1.6.3",
 		"sanitize-filename": "^1.6.3",
 		"say": "^0.16.0",
 		"say": "^0.16.0",
 		"serialize-error": "^12.0.0",
 		"serialize-error": "^12.0.0",