Merge pull request #145 from RooVetGit/more_diff_iterations

More diff iterations

Author: mrubens

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Roo Cline Changelog
 
+## [2.2.14]
+
+-   Make diff editing more robust to transient errors
+
 ## [2.2.13]
 
 -   Fixes to sound playing and applying diffs

package-lock.json

@@ -3,7 +3,7 @@
   "displayName": "Roo Cline",
   "description": "A fork of Cline, an autonomous coding agent, with some added experimental configuration and automation features.",
   "publisher": "RooVeterinaryInc",
-  "version": "2.2.13",
+  "version": "2.2.14",
   "icon": "assets/icons/rocket.png",
   "galleryBanner": {
     "color": "#617A91",

package.json

@@ -75,6 +75,7 @@ export class Cline {
 	private askResponseImages?: string[]
 	private lastMessageTs?: number
 	private consecutiveMistakeCount: number = 0
+	private consecutiveMistakeCountForApplyDiff: Map<string, number> = new Map()
 	private providerRef: WeakRef<ClineProvider>
 	private abort: boolean = false
 	didFinishAborting = false
@@ -97,7 +98,6 @@ export class Cline {
 		apiConfiguration: ApiConfiguration,
 		customInstructions?: string,
 		diffEnabled?: boolean,
-		debugDiffEnabled?: boolean,
 		task?: string,
 		images?: string[],
 		historyItem?: HistoryItem,
@@ -110,7 +110,7 @@ export class Cline {
 		this.diffViewProvider = new DiffViewProvider(cwd)
 		this.customInstructions = customInstructions
 		if (diffEnabled && this.api.getModel().id) {
-			this.diffStrategy = getDiffStrategy(this.api.getModel().id, debugDiffEnabled)
+			this.diffStrategy = getDiffStrategy(this.api.getModel().id)
 		}
 		if (historyItem) {
 			this.taskId = historyItem.id
@@ -1230,8 +1230,9 @@ export class Cline {
 
 								if (!fileExists) {
 									this.consecutiveMistakeCount++
-									await this.say("error", `File does not exist at path: ${absolutePath}`)
-									pushToolResult(`Error: File does not exist at path: ${absolutePath}`)
+									const formattedError = `File does not exist at path: ${absolutePath}\n\n<error_details>\nThe specified file could not be found. Please verify the file path and try again.\n</error_details>`
+									await this.say("error", formattedError)
+									pushToolResult(formattedError)
 									break
 								}
 
@@ -1249,15 +1250,19 @@ export class Cline {
 								}
 								if (!diffResult.success) {
 									this.consecutiveMistakeCount++
-									const errorDetails = diffResult.details ? `\n\nDetails:\n${JSON.stringify(diffResult.details, null, 2)}` : ''
-									await this.say("error", `Unable to apply diff to file: ${absolutePath}\n${diffResult.error}${errorDetails}`)
-									pushToolResult(`Error applying diff to file: ${absolutePath}\n${diffResult.error}${errorDetails}`)
+									const currentCount = (this.consecutiveMistakeCountForApplyDiff.get(relPath) || 0) + 1
+									this.consecutiveMistakeCountForApplyDiff.set(relPath, currentCount)
+									const errorDetails = diffResult.details ? JSON.stringify(diffResult.details, null, 2) : ''
+									const formattedError = `Unable to apply diff to file: ${absolutePath}\n\n<error_details>\n${diffResult.error}${errorDetails ? `\n\nDetails:\n${errorDetails}` : ''}\n</error_details>`
+									if (currentCount >= 2) {
+										await this.say("error", formattedError)
+									}
+									pushToolResult(formattedError)
 									break
 								}
-								const newContent = diffResult.content
 
 								this.consecutiveMistakeCount = 0
-
+								this.consecutiveMistakeCountForApplyDiff.delete(relPath)
 								// Show diff view before asking for approval
 								this.diffViewProvider.editType = "modify"
 								await this.diffViewProvider.open(relPath);

src/core/Cline.ts

@@ -279,8 +279,9 @@ describe('Cline', () => {
                 mockApiConfig,
                 'custom instructions',
                 false, // diffEnabled
-                false, // debugDiffEnabled
-                'test task'
+                'test task', // task
+                undefined, // images
+                undefined  // historyItem
             );
 
             expect(cline.customInstructions).toBe('custom instructions');

src/core/__tests__/Cline.test.ts

@@ -6,10 +6,10 @@ import { SearchReplaceDiffStrategy } from './strategies/search-replace'
  * @param model The name of the model being used (e.g., 'gpt-4', 'claude-3-opus')
  * @returns The appropriate diff strategy for the model
  */
-export function getDiffStrategy(model: string, debugEnabled?: boolean): DiffStrategy {
+export function getDiffStrategy(model: string): DiffStrategy {
     // For now, return SearchReplaceDiffStrategy for all models (with a fuzzy threshold of 0.9)
     // This architecture allows for future optimizations based on model capabilities
-    return new SearchReplaceDiffStrategy(0.9, debugEnabled)
+    return new SearchReplaceDiffStrategy(0.9)
 }
 
 export type { DiffStrategy }

src/core/diff/DiffStrategy.ts

@@ -5,7 +5,7 @@ describe('SearchReplaceDiffStrategy', () => {
         let strategy: SearchReplaceDiffStrategy
 
         beforeEach(() => {
-            strategy = new SearchReplaceDiffStrategy() // Default 1.0 threshold for exact matching
+            strategy = new SearchReplaceDiffStrategy(1.0, 5) // Default 1.0 threshold for exact matching, 5 line buffer for tests
         })
 
         it('should replace matching content', () => {
@@ -562,6 +562,63 @@ this.init();
 }`);
             }
         });
+
+        it('should find matches from middle out', () => {
+            const originalContent = `
+function one() {
+    return "target";
+}
+
+function two() {
+    return "target";
+}
+
+function three() {
+    return "target";
+}
+
+function four() {
+    return "target";
+}
+
+function five() {
+    return "target";
+}`.trim()
+        
+            const diffContent = `test.ts
+<<<<<<< SEARCH
+    return "target";
+=======
+    return "updated";
+>>>>>>> REPLACE`
+        
+            // Search around the middle (function three)
+            // Even though all functions contain the target text,
+            // it should match the one closest to line 9 first
+            const result = strategy.applyDiff(originalContent, diffContent, 9, 9)
+            expect(result.success).toBe(true)
+            if (result.success) {
+                expect(result.content).toBe(`function one() {
+    return "target";
+}
+
+function two() {
+    return "target";
+}
+
+function three() {
+    return "updated";
+}
+
+function four() {
+    return "target";
+}
+
+function five() {
+    return "target";
+}`)
+            }
+        })
     })
 
     describe('line number stripping', () => {
@@ -915,7 +972,7 @@ function test() {
                 }
             })
 
-            it('should insert at the start of the file if no start_line is provided for insertion', () => {
+            it('should error if no start_line is provided for insertion', () => {
                 const originalContent = `function test() {
     return true;
 }`
@@ -926,22 +983,15 @@ console.log("test");
 >>>>>>> REPLACE`
 
                 const result = strategy.applyDiff(originalContent, diffContent)
-                expect(result.success).toBe(true)
-                if (result.success) {
-                    expect(result.content).toBe(`console.log("test");
-function test() {
-    return true;
-}`)
-                }
+                expect(result.success).toBe(false)
             })
         })
     })
 
     describe('fuzzy matching', () => {
         let strategy: SearchReplaceDiffStrategy
-
         beforeEach(() => {
-            strategy = new SearchReplaceDiffStrategy(0.9) // 90% similarity threshold
+            strategy = new SearchReplaceDiffStrategy(0.9, 5) // 90% similarity threshold, 5 line buffer for tests
         })
 
         it('should match content with small differences (>90% similar)', () => {
@@ -959,6 +1009,8 @@ function getData() {
 }
 >>>>>>> REPLACE`
 
+            strategy = new SearchReplaceDiffStrategy(0.9, 5) // Use 5 line buffer for tests
+
             const result = strategy.applyDiff(originalContent, diffContent)
             expect(result.success).toBe(true)
             if (result.success) {
@@ -1008,7 +1060,7 @@ function sum(a, b) {
         let strategy: SearchReplaceDiffStrategy
 
         beforeEach(() => {
-            strategy = new SearchReplaceDiffStrategy()
+            strategy = new SearchReplaceDiffStrategy(0.9, 5)
         })
 
         it('should find and replace within specified line range', () => {
@@ -1467,8 +1519,8 @@ function two() {
 
         it('should document start_line and end_line parameters', () => {
             const description = strategy.getToolDescription('/test')
-            expect(description).toContain('start_line: (required) The line number where the search block starts (inclusive).')
-            expect(description).toContain('end_line: (required) The line number where the search block ends (inclusive).')
+            expect(description).toContain('start_line: (required) The line number where the search block starts.')
+            expect(description).toContain('end_line: (required) The line number where the search block ends.')
         })
     })
 })