From 2292372e427f2044a558eaf0d9bf7cf68679235a Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sat, 14 Dec 2024 20:29:52 -0500 Subject: [PATCH] Improvements to search/replace diff --- CHANGELOG.md | 4 + package-lock.json | 4 +- package.json | 2 +- .../__tests__/search-replace.test.ts | 329 ++++++++++++++++++ src/core/diff/strategies/search-replace.ts | 94 +++-- src/core/diff/types.ts | 4 +- 6 files changed, 383 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23111f9..76fdaf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Roo Cline Changelog +## [2.2.7] + +- More fixes to search/replace diffs + ## [2.2.6] - Add a fuzzy match tolerance when applying diffs diff --git a/package-lock.json b/package-lock.json index 62aa553..945e8e7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "roo-cline", - "version": "2.2.6", + "version": "2.2.7", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "roo-cline", - "version": "2.2.6", + "version": "2.2.7", "dependencies": { "@anthropic-ai/bedrock-sdk": "^0.10.2", "@anthropic-ai/sdk": "^0.26.0", diff --git a/package.json b/package.json index fde1305..9fda9aa 100644 --- a/package.json +++ b/package.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.6", + "version": "2.2.7", "icon": "assets/icons/rocket.png", "galleryBanner": { "color": "#617A91", diff --git a/src/core/diff/strategies/__tests__/search-replace.test.ts b/src/core/diff/strategies/__tests__/search-replace.test.ts index d90ccd0..ee2174a 100644 --- a/src/core/diff/strategies/__tests__/search-replace.test.ts +++ b/src/core/diff/strategies/__tests__/search-replace.test.ts @@ -291,6 +291,329 @@ function sum(a, b) { }) }) + describe('line-constrained search', () => { + let strategy: SearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new SearchReplaceDiffStrategy() + }) + + it('should find and replace within specified line range', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return 3; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function two() { + return 2; +} +======= +function two() { + return "two"; +} +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent, 5, 7) + expect(result).toBe(`function one() { + return 1; +} + +function two() { + return "two"; +} + +function three() { + return 3; +}`) + }) + + it('should find and replace within buffer zone (5 lines before/after)', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return 3; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function three() { + return 3; +} +======= +function three() { + return "three"; +} +>>>>>>> REPLACE` + + // 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() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return "three"; +}`) + }) + + it('should not find matches outside search range and buffer zone', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return 3; +} + +function four() { + return 4; +} + +function five() { + return 5; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function five() { + return 5; +} +======= +function five() { + return "five"; +} +>>>>>>> REPLACE` + + // 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) + }) + + it('should handle search range at start of file', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function one() { + return 1; +} +======= +function one() { + return "one"; +} +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent, 1, 3) + expect(result).toBe(`function one() { + return "one"; +} + +function two() { + return 2; +}`) + }) + + it('should handle search range at end of file', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function two() { + return 2; +} +======= +function two() { + return "two"; +} +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent, 5, 7) + expect(result).toBe(`function one() { + return 1; +} + +function two() { + return "two"; +}`) + }) + + it('should match specific instance of duplicate code using line numbers', () => { + const originalContent = ` +function processData(data) { + return data.map(x => x * 2); +} + +function unrelatedStuff() { + console.log("hello"); +} + +// Another data processor +function processData(data) { + return data.map(x => x * 2); +} + +function moreStuff() { + console.log("world"); +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function processData(data) { + return data.map(x => x * 2); +} +======= +function processData(data) { + // Add logging + console.log("Processing data..."); + return data.map(x => x * 2); +} +>>>>>>> REPLACE` + + // Target the second instance of processData + const result = strategy.applyDiff(originalContent, diffContent, 10, 12) + expect(result).toBe(`function processData(data) { + return data.map(x => x * 2); +} + +function unrelatedStuff() { + console.log("hello"); +} + +// Another data processor +function processData(data) { + // Add logging + console.log("Processing data..."); + return data.map(x => x * 2); +} + +function moreStuff() { + console.log("world"); +}`) + }) + + it('should search from start line to end of file when only start_line is provided', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return 3; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function three() { + return 3; +} +======= +function three() { + return "three"; +} +>>>>>>> REPLACE` + + // Only provide start_line, should search from there to end of file + const result = strategy.applyDiff(originalContent, diffContent, 8) + expect(result).toBe(`function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return "three"; +}`) + }) + + it('should search from start of file to end line when only end_line is provided', () => { + const originalContent = ` +function one() { + return 1; +} + +function two() { + return 2; +} + +function three() { + return 3; +} +`.trim() + const diffContent = `test.ts +<<<<<<< SEARCH +function one() { + return 1; +} +======= +function one() { + return "one"; +} +>>>>>>> REPLACE` + + // 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() { + return "one"; +} + +function two() { + return 2; +} + +function three() { + return 3; +}`) + }) + }) + describe('getToolDescription', () => { let strategy: SearchReplaceDiffStrategy @@ -312,5 +635,11 @@ function sum(a, b) { expect(description).toContain('') expect(description).toContain('') }) + + 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.') + expect(description).toContain('end_line: (required) The line number where the search block ends.') + }) }) }) diff --git a/src/core/diff/strategies/search-replace.ts b/src/core/diff/strategies/search-replace.ts index 1e51c34..ee0272e 100644 --- a/src/core/diff/strategies/search-replace.ts +++ b/src/core/diff/strategies/search-replace.ts @@ -56,7 +56,7 @@ export class SearchReplaceDiffStrategy implements DiffStrategy { getToolDescription(cwd: string): string { return `## apply_diff -Description: Request to replace existing code using search and replace blocks. +Description: Request to replace existing code using a search and replace block. This tool allows for precise, surgical replaces to files by specifying exactly what content to search for and what to replace it with. The tool will maintain proper indentation and formatting while making changes. Only a single operation is allowed per tool use. @@ -65,33 +65,32 @@ If you're not confident in the exact content to search for, use the read_file to Parameters: - path: (required) The path of the file to modify (relative to the current working directory ${cwd}) -- diff: (required) The search/replace blocks defining the changes. +- diff: (required) The search/replace block defining the changes. +- start_line: (required) The line number where the search block starts. +- end_line: (required) The line number where the search block ends. -Format: -1. First line must be the file path -2. Followed by search/replace blocks: - \`\`\` - <<<<<<< SEARCH - [exact content to find including whitespace] - ======= - [new content to replace with] - >>>>>>> REPLACE - \`\`\` +Diff format: +\`\`\` +<<<<<<< SEARCH +[exact content to find including whitespace] +======= +[new content to replace with] +>>>>>>> REPLACE +\`\`\` Example: Original file: \`\`\` -def calculate_total(items): - total = 0 - for item in items: - total += item - return total +1 | def calculate_total(items): +2 | total = 0 +3 | for item in items: +4 | total += item +5 | return total \`\`\` Search/Replace content: \`\`\` -main.py <<<<<<< SEARCH def calculate_total(items): total = 0 @@ -111,10 +110,12 @@ Usage: Your search/replace content here +1 +5 ` } - applyDiff(originalContent: string, diffContent: string): string | false { + applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): string | false { // Extract the search and replace blocks const match = diffContent.match(/<<<<<<< SEARCH\n([\s\S]*?)\n=======\n([\s\S]*?)\n>>>>>>> REPLACE/); if (!match) { @@ -131,11 +132,25 @@ Your search/replace content here const replaceLines = replaceContent.split(/\r?\n/); const originalLines = originalContent.split(/\r?\n/); + // Determine search range based on provided line numbers + let searchStartIndex = 0; + let searchEndIndex = originalLines.length; + + if (startLine !== undefined || endLine !== undefined) { + // Convert to 0-based index and add buffer + if (startLine !== undefined) { + searchStartIndex = Math.max(0, startLine - 6); + } + if (endLine !== undefined) { + searchEndIndex = Math.min(originalLines.length, endLine + 5); + } + } + // Find the search content in the original using fuzzy matching let matchIndex = -1; let bestMatchScore = 0; - for (let i = 0; i <= originalLines.length - searchLines.length; i++) { + for (let i = searchStartIndex; i <= searchEndIndex - searchLines.length; i++) { // Join the lines and calculate overall similarity const originalChunk = originalLines.slice(i, i + searchLines.length).join('\n'); const searchChunk = searchLines.join('\n'); @@ -169,40 +184,19 @@ Your search/replace content here // Apply the replacement while preserving exact indentation const indentedReplaceLines = replaceLines.map((line, i) => { - // Get the corresponding original and search indentations - const originalIndent = originalIndents[Math.min(i, originalIndents.length - 1)]; - const searchIndent = searchIndents[Math.min(i, searchIndents.length - 1)]; + // Get the matched line's exact indentation + const matchedIndent = originalIndents[0]; - // Get the current line's indentation + // Get the current line's indentation relative to the search content const currentIndentMatch = line.match(/^[\t ]*/); const currentIndent = currentIndentMatch ? currentIndentMatch[0] : ''; + const searchBaseIndent = searchIndents[0] || ''; - // Get the corresponding search line's indentation - const searchLineIndex = Math.min(i, searchLines.length - 1); - const searchLineIndent = searchIndents[searchLineIndex]; - - // Get the corresponding original line's indentation - const originalLineIndex = Math.min(i, originalIndents.length - 1); - const originalLineIndent = originalIndents[originalLineIndex]; - - // If this line has the same indentation as its corresponding search line, - // use the original indentation - if (currentIndent === searchLineIndent) { - return originalLineIndent + line.trim(); - } - - // Otherwise, preserve the original indentation structure - const indentChar = originalLineIndent.charAt(0) || '\t'; - const indentLevel = Math.floor(originalLineIndent.length / indentChar.length); - - // Calculate the relative indentation from the search line - const searchLevel = Math.floor(searchLineIndent.length / indentChar.length); - const currentLevel = Math.floor(currentIndent.length / indentChar.length); - const relativeLevel = currentLevel - searchLevel; - - // Apply the relative indentation to the original level - const targetLevel = Math.max(0, indentLevel + relativeLevel); - return indentChar.repeat(targetLevel) + line.trim(); + // Calculate the relative indentation from the search content + const relativeIndent = currentIndent.slice(searchBaseIndent.length); + + // Apply the matched indentation plus any relative indentation + return matchedIndent + relativeIndent + line.trim(); }); // Construct the final content diff --git a/src/core/diff/types.ts b/src/core/diff/types.ts index f4cdf17..e5d4478 100644 --- a/src/core/diff/types.ts +++ b/src/core/diff/types.ts @@ -13,7 +13,9 @@ export interface DiffStrategy { * Apply a diff to the original content * @param originalContent The original file content * @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 */ - applyDiff(originalContent: string, diffContent: string): string | false + applyDiff(originalContent: string, diffContent: string, startLine?: number, endLine?: number): string | false }