From c1dfdeda229d33ab532367e63a82f6530deee767 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sun, 15 Dec 2024 19:34:51 -0500 Subject: [PATCH] Better error messages for diffs --- src/core/Cline.ts | 15 +- .../__tests__/search-replace.test.ts | 170 +++++++++++++----- src/core/diff/strategies/search-replace.ts | 53 +++++- src/core/diff/strategies/unified.ts | 29 ++- src/core/diff/types.ts | 15 +- 5 files changed, 226 insertions(+), 56 deletions(-) diff --git a/src/core/Cline.ts b/src/core/Cline.ts index cb27fe1..5b190d7 100644 --- a/src/core/Cline.ts +++ b/src/core/Cline.ts @@ -1237,20 +1237,25 @@ export class Cline { const originalContent = await fs.readFile(absolutePath, "utf-8") // Apply the diff to the original content - let newContent = this.diffStrategy?.applyDiff(originalContent, diffContent) ?? false - if (newContent === false) { + const diffResult = this.diffStrategy?.applyDiff(originalContent, diffContent) ?? { + success: false, + error: "No diff strategy available" + } + if (!diffResult.success) { this.consecutiveMistakeCount++ - await this.say("error", `Unable to apply diff to file - contents are out of sync: ${absolutePath}`) - pushToolResult(`Error applying diff to file: ${absolutePath} - contents are out of sync. Try re-reading the relevant lines of the file and applying the diff again.`) + 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}`) break } + const newContent = diffResult.content this.consecutiveMistakeCount = 0 // Show diff view before asking for approval this.diffViewProvider.editType = "modify" await this.diffViewProvider.open(relPath); - await this.diffViewProvider.update(newContent, true); + await this.diffViewProvider.update(diffResult.content, true); await this.diffViewProvider.scrollToFirstDiff(); const completeMessage = JSON.stringify({ diff --git a/src/core/diff/strategies/__tests__/search-replace.test.ts b/src/core/diff/strategies/__tests__/search-replace.test.ts index 9afb9e4..e0b348d 100644 --- a/src/core/diff/strategies/__tests__/search-replace.test.ts +++ b/src/core/diff/strategies/__tests__/search-replace.test.ts @@ -22,7 +22,10 @@ function hello() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe('function hello() {\n console.log("hello world")\n}\n') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('function hello() {\n console.log("hello world")\n}\n') + } }) it('should match content with different surrounding whitespace', () => { @@ -39,7 +42,10 @@ function example() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe('\nfunction example() {\n return 43;\n}\n\n') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('\nfunction example() {\n return 43;\n}\n\n') + } }) it('should match content with different indentation in search block', () => { @@ -56,7 +62,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(' function test() {\n return false;\n }\n') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(' function test() {\n return false;\n }\n') + } }) it('should handle tab-based indentation', () => { @@ -73,7 +82,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe("function test() {\n\treturn false;\n}\n") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe("function test() {\n\treturn false;\n}\n") + } }) it('should preserve mixed tabs and spaces', () => { @@ -94,7 +106,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe("\tclass Example {\n\t constructor() {\n\t\tthis.value = 1;\n\t }\n\t}") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe("\tclass Example {\n\t constructor() {\n\t\tthis.value = 1;\n\t }\n\t}") + } }) it('should handle additional indentation with tabs', () => { @@ -112,7 +127,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe("\tfunction test() {\n\t\t// Add comment\n\t\treturn false;\n\t}") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe("\tfunction test() {\n\t\t// Add comment\n\t\treturn false;\n\t}") + } }) it('should preserve exact indentation characters when adding lines', () => { @@ -131,7 +149,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe("\tfunction test() {\n\t\t// First comment\n\t\t// Second comment\n\t\treturn true;\n\t}") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe("\tfunction test() {\n\t\t// First comment\n\t\t// Second comment\n\t\treturn true;\n\t}") + } }) it('should handle Windows-style CRLF line endings', () => { @@ -148,7 +169,10 @@ function test() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe("function test() {\r\n return false;\r\n}\r\n") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe("function test() {\r\n return false;\r\n}\r\n") + } }) it('should return false if search content does not match', () => { @@ -165,7 +189,7 @@ function hello() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(false) + expect(result.success).toBe(false) }) it('should return false if diff format is invalid', () => { @@ -173,7 +197,7 @@ function hello() { const diffContent = `test.ts\nInvalid diff format` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(false) + expect(result.success).toBe(false) }) it('should handle multiple lines with proper indentation', () => { @@ -192,7 +216,10 @@ function hello() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe('class Example {\n constructor() {\n this.value = 0\n }\n\n getValue() {\n // Add logging\n console.log("Getting value")\n return this.value\n }\n}\n') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('class Example {\n constructor() {\n this.value = 0\n }\n\n getValue() {\n // Add logging\n console.log("Getting value")\n return this.value\n }\n}\n') + } }) it('should preserve whitespace exactly in the output', () => { @@ -209,7 +236,10 @@ function hello() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(" modified\n still indented\n end\n") + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(" modified\n still indented\n end\n") + } }) it('should preserve indentation when adding new lines after existing content', () => { @@ -226,7 +256,10 @@ function hello() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(' onScroll={() => updateHighlights()}\n onDragOver={(e) => {\n e.preventDefault()\n e.stopPropagation()\n }}') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(' onScroll={() => updateHighlights()}\n onDragOver={(e) => {\n e.preventDefault()\n e.stopPropagation()\n }}') + } }) it('should handle varying indentation levels correctly', () => { @@ -264,7 +297,9 @@ class Example { >>>>>>> REPLACE`.trim(); const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(` + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(` class Example { constructor() { this.value = 1; @@ -275,14 +310,15 @@ class Example { } } }`.trim()); - }); + } + }) it('should handle mixed indentation styles in the same file', () => { - const originalContent = `class Example { + const originalContent = `class Example { constructor() { this.value = 0; if (true) { - this.init(); + this.init(); } } }`.trim(); @@ -305,7 +341,9 @@ class Example { >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`class Example { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`class Example { constructor() { this.value = 1; if (true) { @@ -314,7 +352,8 @@ class Example { } } }`); - }); + } + }) it('should handle Python-style significant whitespace', () => { const originalContent = `def example(): @@ -338,17 +377,20 @@ class Example { >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`def example(): + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`def example(): if condition: do_something() while items: item = items.pop() process(item) return True`); + } }); it('should preserve empty lines with indentation', () => { - const originalContent = `function test() { + const originalContent = `function test() { const x = 1; if (x) { @@ -368,7 +410,9 @@ class Example { >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`function test() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function test() { const x = 1; // Check x @@ -376,6 +420,7 @@ class Example { return true; } }`); + } }); it('should handle indentation when replacing entire blocks', () => { @@ -406,7 +451,9 @@ class Example { >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`class Test { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`class Test { method() { try { if (true) { @@ -417,6 +464,7 @@ class Example { } } }`); + } }); it('should handle negative indentation relative to search content', () => { @@ -438,7 +486,9 @@ class Example { >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`class Example { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`class Example { constructor() { if (true) { this.init(); @@ -446,6 +496,7 @@ class Example { } } }`); + } }); it('should handle extreme negative indentation (no indent)', () => { @@ -464,13 +515,16 @@ this.init(); >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`class Example { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`class Example { constructor() { if (true) { this.init(); } } }`); + } }); it('should handle mixed indentation changes in replace block', () => { @@ -495,7 +549,9 @@ this.init(); >>>>>>> REPLACE`; const result = strategy.applyDiff(originalContent, diffContent); - expect(result).toBe(`class Example { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`class Example { constructor() { if (true) { this.init(); @@ -504,6 +560,7 @@ this.init(); } } }`); + } }); }) @@ -530,7 +587,10 @@ function getData() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe('function getData() {\n const data = fetchData();\n return data.filter(Boolean);\n}\n') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('function getData() {\n const data = fetchData();\n return data.filter(Boolean);\n}\n') + } }) it('should not match when content is too different (<90% similar)', () => { @@ -547,7 +607,7 @@ function processData(data) { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe(false) + expect(result.success).toBe(false) }) it('should match content with extra whitespace', () => { @@ -564,7 +624,10 @@ function sum(a, b) { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent) - expect(result).toBe('function sum(a, b) {\n return a + b + 1;\n}') + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('function sum(a, b) {\n return a + b + 1;\n}') + } }) }) @@ -601,7 +664,9 @@ function two() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent, 5, 7) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return 1; } @@ -612,6 +677,7 @@ function two() { function three() { return 3; }`) + } }) it('should find and replace within buffer zone (5 lines before/after)', () => { @@ -642,7 +708,9 @@ function three() { // Even though we specify lines 5-7, it should still find the match at lines 9-11 // because it's within the 5-line buffer zone const result = strategy.applyDiff(originalContent, diffContent, 5, 7) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return 1; } @@ -653,6 +721,7 @@ function two() { function three() { return "three"; }`) + } }) it('should not find matches outside search range and buffer zone', () => { @@ -691,7 +760,7 @@ function five() { // Searching around function two() (lines 5-7) // function five() is more than 5 lines away, so it shouldn't match const result = strategy.applyDiff(originalContent, diffContent, 5, 7) - expect(result).toBe(false) + expect(result.success).toBe(false) }) it('should handle search range at start of file', () => { @@ -716,13 +785,16 @@ function one() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent, 1, 3) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return "one"; } function two() { return 2; }`) + } }) it('should handle search range at end of file', () => { @@ -747,13 +819,16 @@ function two() { >>>>>>> REPLACE` const result = strategy.applyDiff(originalContent, diffContent, 5, 7) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return 1; } function two() { return "two"; }`) + } }) it('should match specific instance of duplicate code using line numbers', () => { @@ -790,7 +865,9 @@ function processData(data) { // Target the second instance of processData const result = strategy.applyDiff(originalContent, diffContent, 10, 12) - expect(result).toBe(`function processData(data) { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function processData(data) { return data.map(x => x * 2); } @@ -808,6 +885,7 @@ function processData(data) { function moreStuff() { console.log("world"); }`) + } }) it('should search from start line to end of file when only start_line is provided', () => { @@ -837,7 +915,9 @@ function three() { // Only provide start_line, should search from there to end of file const result = strategy.applyDiff(originalContent, diffContent, 8) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return 1; } @@ -848,6 +928,7 @@ function two() { function three() { return "three"; }`) + } }) it('should search from start of file to end line when only end_line is provided', () => { @@ -877,7 +958,9 @@ function one() { // Only provide end_line, should search from start of file to there const result = strategy.applyDiff(originalContent, diffContent, undefined, 4) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return "one"; } @@ -888,6 +971,7 @@ function two() { function three() { return 3; }`) + } }) it('should prioritize exact line match over expanded search', () => { @@ -921,7 +1005,9 @@ function process() { // Should match the second instance exactly at lines 10-12 // even though the first instance at 6-8 is within the expanded search range const result = strategy.applyDiff(originalContent, diffContent, 10, 12) - expect(result).toBe(` + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(` function one() { return 1; } @@ -937,7 +1023,8 @@ function process() { function two() { return 2; }`) - }) + } + }) it('should fall back to expanded search only if exact match fails', () => { const originalContent = ` @@ -966,7 +1053,9 @@ function process() { // Specify wrong line numbers (3-5), but content exists at 6-8 // Should still find and replace it since it's within the expanded range const result = strategy.applyDiff(originalContent, diffContent, 3, 5) - expect(result).toBe(`function one() { + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(`function one() { return 1; } @@ -977,6 +1066,7 @@ function process() { function two() { return 2; }`) + } }) }) diff --git a/src/core/diff/strategies/search-replace.ts b/src/core/diff/strategies/search-replace.ts index 021aa12..21309c6 100644 --- a/src/core/diff/strategies/search-replace.ts +++ b/src/core/diff/strategies/search-replace.ts @@ -1,4 +1,4 @@ -import { DiffStrategy } from "../types" +import { DiffStrategy, DiffResult } from "../types" function levenshteinDistance(a: string, b: string): number { const matrix: number[][] = []; @@ -115,11 +115,20 @@ Your search/replace content here ` } - applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): string | false { + applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): DiffResult { // Extract the search and replace blocks const match = diffContent.match(/<<<<<<< SEARCH\n([\s\S]*?)\n=======\n([\s\S]*?)\n>>>>>>> REPLACE/); if (!match) { - return false; + // Log detailed format information + console.log('Invalid Diff Format Debug:', { + expectedFormat: "<<<<<<< SEARCH\\n[search content]\\n=======\\n[replace content]\\n>>>>>>> REPLACE", + tip: "Make sure to include both SEARCH and REPLACE sections with correct markers" + }); + + return { + success: false, + error: "Invalid diff format - missing required SEARCH/REPLACE sections" + }; } const [_, searchContent, replaceContent] = match; @@ -135,12 +144,26 @@ Your search/replace content here // First try exact line range if provided let matchIndex = -1; let bestMatchScore = 0; + let bestMatchContent = ""; if (startLine !== undefined && endLine !== undefined) { // Convert to 0-based index const exactStartIndex = startLine - 1; const exactEndIndex = endLine - 1; + if (exactStartIndex < 0 || exactEndIndex >= originalLines.length) { + // Log detailed debug information + console.log('Invalid Line Range Debug:', { + requestedRange: { start: startLine, end: endLine }, + fileBounds: { start: 1, end: originalLines.length } + }); + + return { + success: false, + error: `Line range ${startLine}-${endLine} is invalid (file has ${originalLines.length} lines)`, + }; + } + // Check exact range first const originalChunk = originalLines.slice(exactStartIndex, exactEndIndex + 1).join('\n'); const searchChunk = searchLines.join('\n'); @@ -149,6 +172,7 @@ Your search/replace content here if (similarity >= this.fuzzyThreshold) { matchIndex = exactStartIndex; bestMatchScore = similarity; + bestMatchContent = originalChunk; } } @@ -177,13 +201,26 @@ Your search/replace content here if (similarity > bestMatchScore) { bestMatchScore = similarity; matchIndex = i; + bestMatchContent = originalChunk; } } } // Require similarity to meet threshold if (matchIndex === -1 || bestMatchScore < this.fuzzyThreshold) { - return false; + const searchChunk = searchLines.join('\n'); + // Log detailed debug information to console + console.log('Search/Replace Debug Info:', { + similarity: bestMatchScore, + threshold: this.fuzzyThreshold, + searchContent: searchChunk, + bestMatch: bestMatchContent || undefined + }); + + return { + success: false, + error: `No sufficiently similar match found${startLine !== undefined ? ` near lines ${startLine}-${endLine}` : ''} (${Math.round(bestMatchScore * 100)}% similar, needs ${Math.round(this.fuzzyThreshold * 100)}%)` + }; } // Get the matched lines from the original content @@ -229,6 +266,10 @@ Your search/replace content here const beforeMatch = originalLines.slice(0, matchIndex); const afterMatch = originalLines.slice(matchIndex + searchLines.length); - return [...beforeMatch, ...indentedReplaceLines, ...afterMatch].join(lineEnding); + const finalContent = [...beforeMatch, ...indentedReplaceLines, ...afterMatch].join(lineEnding); + return { + success: true, + content: finalContent + }; } -} +} \ No newline at end of file diff --git a/src/core/diff/strategies/unified.ts b/src/core/diff/strategies/unified.ts index 9e95e09..2f80a61 100644 --- a/src/core/diff/strategies/unified.ts +++ b/src/core/diff/strategies/unified.ts @@ -1,5 +1,5 @@ import { applyPatch } from "diff" -import { DiffStrategy } from "../types" +import { DiffStrategy, DiffResult } from "../types" export class UnifiedDiffStrategy implements DiffStrategy { getToolDescription(cwd: string): string { @@ -108,7 +108,30 @@ Your diff here ` } - applyDiff(originalContent: string, diffContent: string): string | false { - return applyPatch(originalContent, diffContent) as string | false + applyDiff(originalContent: string, diffContent: string): DiffResult { + try { + const result = applyPatch(originalContent, diffContent) + if (result === false) { + return { + success: false, + error: "Failed to apply unified diff - patch rejected", + details: { + searchContent: diffContent + } + } + } + return { + success: true, + content: result + } + } catch (error) { + return { + success: false, + error: `Error applying unified diff: ${error.message}`, + details: { + searchContent: diffContent + } + } + } } } diff --git a/src/core/diff/types.ts b/src/core/diff/types.ts index e5d4478..3957a1f 100644 --- a/src/core/diff/types.ts +++ b/src/core/diff/types.ts @@ -1,6 +1,17 @@ /** * Interface for implementing different diff strategies */ + +export type DiffResult = + | { success: true; content: string } + | { success: false; error: string; details?: { + similarity?: number; + threshold?: number; + matchedRange?: { start: number; end: number }; + searchContent?: string; + bestMatch?: string; + }}; + export interface DiffStrategy { /** * Get the tool description for this diff strategy @@ -15,7 +26,7 @@ export interface DiffStrategy { * @param diffContent The diff content in the strategy's format * @param startLine Optional line number where the search block starts. If not provided, searches the entire file. * @param endLine Optional line number where the search block ends. If not provided, searches the entire file. - * @returns The new content after applying the diff, or false if the diff could not be applied + * @returns A DiffResult object containing either the successful result or error details */ - applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): string | false + applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): DiffResult }