diff --git a/src/extension/tools/node/abstractReplaceStringTool.tsx b/src/extension/tools/node/abstractReplaceStringTool.tsx index 735832fc5..842a252e8 100644 --- a/src/extension/tools/node/abstractReplaceStringTool.tsx +++ b/src/extension/tools/node/abstractReplaceStringTool.tsx @@ -293,7 +293,8 @@ export abstract class AbstractReplaceStringTool { + return pos >= lineStart && pos <= lineEnd; + }; + + while (positionIndex < positions.length && isPositionInLine(positions[positionIndex])) { + lineNumbers.push(lineIndex + 1); // 1-based line numbers + positionIndex++; + } + + currentPos += lines[lineIndex].length + (isLastLine ? 0 : 1); + } + + return lineNumbers; +} + /** * Calculates the similarity ratio between two strings using Levenshtein distance. * Returns a value between 0 (completely different) and 1 (identical). @@ -140,7 +172,7 @@ type MatchResult = MatchResultCommon & ( | { text: string; type: 'fuzzy' } | { text: string; type: 'whitespace' } | { text: string; type: 'similarity'; suggestion: string; similarity: number } - | { text: string; type: 'multiple'; suggestion: string; matchPositions: number[]; strategy: 'exact' | 'fuzzy' | 'whitespace' } + | { text: string; type: 'multiple'; suggestion: string; matchPositions: number[]; strategy: 'exact' | 'fuzzy' | 'whitespace'; lineNumbers?: number[] } ); /** @@ -150,6 +182,7 @@ type MatchResult = MatchResultCommon & ( * @param text The source text to search in * @param oldStr The string to find and replace * @param newStr The replacement string + * @param replaceAll Whether to replace all occurrences instead of throwing an error for multiple matches * @returns An object with the new text, match type, and additional match information */ export function findAndReplaceOne( @@ -157,21 +190,22 @@ export function findAndReplaceOne( oldStr: string, newStr: string, eol: string, + replaceAll: boolean = false, ): MatchResult { // Strategy 1: Try exact match first (fastest) - const exactResult = tryExactMatch(text, oldStr, newStr); + const exactResult = tryExactMatch(text, oldStr, newStr, replaceAll); if (exactResult.type !== 'none') { return exactResult; } // Strategy 2: Try whitespace-flexible matching - const whitespaceResult = tryWhitespaceFlexibleMatch(text, oldStr, newStr, eol); + const whitespaceResult = tryWhitespaceFlexibleMatch(text, oldStr, newStr, eol, replaceAll); if (whitespaceResult.type !== 'none') { return whitespaceResult; } // Strategy 3: Try line-by-line fuzzy matching - const fuzzyResult = tryFuzzyMatch(text, oldStr, newStr, eol); + const fuzzyResult = tryFuzzyMatch(text, oldStr, newStr, eol, replaceAll); if (fuzzyResult.type !== 'none') { return fuzzyResult; } @@ -194,7 +228,7 @@ export function findAndReplaceOne( /** * Tries to find an exact match of oldStr in text. */ -function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResult { +function tryExactMatch(text: string, oldStr: string, newStr: string, replaceAll: boolean = false): MatchResult { const matchPositions: number[] = []; for (let searchIdx = 0; ;) { const idx = text.indexOf(oldStr, searchIdx); @@ -208,14 +242,30 @@ function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResul // Check for multiple exact occurrences. if (matchPositions.length > 1) { - return { - text, - type: 'multiple', - editPosition: matchPositions.map(idx => [idx, idx + oldStr.length]), - strategy: 'exact', - matchPositions, - suggestion: "Multiple exact matches found. Make your search string more specific." - }; + if (replaceAll) { + // Replace all occurrences from right to left to avoid position shifts + let result = text; + for (let i = matchPositions.length - 1; i >= 0; i--) { + const idx = matchPositions[i]; + result = result.slice(0, idx) + newStr + result.slice(idx + oldStr.length); + } + return { + text: result, + type: 'exact', + editPosition: matchPositions.map(idx => [idx, idx + oldStr.length]), + }; + } else { + const lineNumbers = positionsToLineNumbers(text, matchPositions); + return { + text, + type: 'multiple', + editPosition: matchPositions.map(idx => [idx, idx + oldStr.length]), + strategy: 'exact', + matchPositions, + lineNumbers, + suggestion: `Multiple exact matches found on lines ${lineNumbers.join(', ')}. Make your search string more specific or use replaceAll: true to replace all occurrences.` + }; + } } // Exactly one exact match found. const firstExactIdx = matchPositions[0]; @@ -230,7 +280,7 @@ function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResul /** * Tries to match using flexible whitespace handling. */ -function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string, eol: string): MatchResult { +function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string, eol: string, replaceAll: boolean = false): MatchResult { const haystack = text.split(eol).map(line => line.trim()); const needle = oldStr.trim().split(eol).map(line => line.trim()); needle.push(''); // trailing newline to match until the end of a line @@ -255,14 +305,31 @@ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string const positions = matchedLines.map(match => convert.positionToOffset(new EditorPosition(match + 1, 1))); if (matchedLines.length > 1) { - return { - text, - type: 'multiple', - editPosition: [], - matchPositions: positions, - suggestion: "Multiple matches found with flexible whitespace. Make your search string more unique.", - strategy: 'whitespace', - }; + if (replaceAll) { + // Replace all occurrences from right to left to avoid position shifts + let result = text; + for (let i = matchedLines.length - 1; i >= 0; i--) { + const startIdx = positions[i]; + const endIdx = convert.positionToOffset(new EditorPosition(matchedLines[i] + 1 + needle.length, 1)); + result = result.slice(0, startIdx) + newStr + eol + result.slice(endIdx); + } + return { + text: result, + type: 'whitespace', + editPosition: matchedLines.map((match, i) => [positions[i], convert.positionToOffset(new EditorPosition(match + 1 + needle.length, 1))]), + }; + } else { + const lineNumbers = matchedLines.map(line => line + 1); // Convert to 1-based + return { + text, + type: 'multiple', + editPosition: [], + matchPositions: positions, + lineNumbers, + suggestion: `Multiple matches found with flexible whitespace on lines ${lineNumbers.join(', ')}. Make your search string more unique or use replaceAll: true to replace all occurrences.`, + strategy: 'whitespace', + }; + } } // Exactly one whitespace-flexible match found @@ -279,7 +346,7 @@ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string /** * Tries to match using the traditional fuzzy approach with line-by-line matching. */ -function tryFuzzyMatch(text: string, oldStr: string, newStr: string, eol: string): MatchResult { +function tryFuzzyMatch(text: string, oldStr: string, newStr: string, eol: string, replaceAll: boolean = false): MatchResult { // Handle trailing newlines const hasTrailingLF = oldStr.endsWith(eol); if (hasTrailingLF) { @@ -309,14 +376,33 @@ function tryFuzzyMatch(text: string, oldStr: string, newStr: string, eol: string }; } if (matches.length > 1) { - return { - text, - type: 'multiple', - editPosition: [], - suggestion: "Multiple fuzzy matches found. Try including more context in your search string.", - strategy: 'fuzzy', - matchPositions: matches.map(match => match.index || 0), - }; + if (replaceAll) { + // Replace all occurrences from right to left to avoid position shifts + let result = text; + for (let i = matches.length - 1; i >= 0; i--) { + const match = matches[i]; + const startIdx = match.index || 0; + const endIdx = startIdx + match[0].length; + result = result.slice(0, startIdx) + newStr + result.slice(endIdx); + } + return { + text: result, + type: 'fuzzy', + editPosition: matches.map(match => [match.index || 0, (match.index || 0) + match[0].length]), + }; + } else { + const matchPositions = matches.map(match => match.index || 0); + const lineNumbers = positionsToLineNumbers(text, matchPositions); + return { + text, + type: 'multiple', + editPosition: [], + suggestion: `Multiple fuzzy matches found on lines ${lineNumbers.join(', ')}. Try including more context in your search string or use replaceAll: true to replace all occurrences.`, + strategy: 'fuzzy', + matchPositions, + lineNumbers, + }; + } } // Exactly one fuzzy match found @@ -412,7 +498,8 @@ export async function applyEdit( workspaceService: IWorkspaceService, notebookService: INotebookService, alternativeNotebookContent: IAlternativeNotebookContentService, - languageModel: LanguageModelChat | undefined + languageModel: LanguageModelChat | undefined, + replaceAll: boolean = false ): Promise<{ patch: Hunk[]; updatedFile: string; edits: TextEdit[] }> { let originalFile: string; @@ -443,7 +530,7 @@ export async function applyEdit( // Edit existing file case if (new_string === '') { // For empty new string, handle special deletion case - const result = findAndReplaceOne(originalFile, old_string, new_string, eol); + const result = findAndReplaceOne(originalFile, old_string, new_string, eol, replaceAll); if (result.type === 'none') { // Try with newline appended if the original doesn't end with newline if (!old_string.endsWith(eol) && originalFile.includes(old_string + eol)) { @@ -470,15 +557,15 @@ export async function applyEdit( } else { updatedFile = result.text; - if (result.editPosition.length) { - const [start, end] = result.editPosition[0]; + // Handle multiple edits for replaceAll + for (const [start, end] of result.editPosition) { const range = new Range(document.positionAt(start), document.positionAt(end)); edits.push(TextEdit.delete(range)); } } } else { // Normal replacement case using the enhanced matcher - const result = findAndReplaceOne(originalFile, old_string, new_string, eol); + const result = findAndReplaceOne(originalFile, old_string, new_string, eol, replaceAll); if (result.type === 'none') { const suggestion = result?.suggestion || 'The string to replace must match exactly or be a valid fuzzy match.'; @@ -495,8 +582,8 @@ export async function applyEdit( } else { updatedFile = result.text; - if (result.editPosition.length) { - const [start, end] = result.editPosition[0]; + // Handle multiple edits for replaceAll + for (const [start, end] of result.editPosition) { const range = new Range(document.positionAt(start), document.positionAt(end)); edits.push(TextEdit.replace(range, new_string)); } diff --git a/src/extension/tools/node/replaceStringTool.tsx b/src/extension/tools/node/replaceStringTool.tsx index f8f48a9e7..5ad24b23a 100644 --- a/src/extension/tools/node/replaceStringTool.tsx +++ b/src/extension/tools/node/replaceStringTool.tsx @@ -15,6 +15,7 @@ export interface IReplaceStringToolParams { filePath: string; oldString: string; newString: string; + replaceAll?: boolean; } export class ReplaceStringTool extends AbstractReplaceStringTool { diff --git a/src/extension/tools/node/test/editFileToolUtils.spec.ts b/src/extension/tools/node/test/editFileToolUtils.spec.ts index 61085b806..7cfdad7c7 100644 --- a/src/extension/tools/node/test/editFileToolUtils.spec.ts +++ b/src/extension/tools/node/test/editFileToolUtils.spec.ts @@ -23,8 +23,8 @@ describe('replace_string_in_file - applyEdit', () => { let alternatveContentService: IAlternativeNotebookContentService; let doc: IExtHostDocumentData; - async function doApplyEdit(oldString: string, newString: string, uri = doc.document.uri) { - const r = await applyEdit(uri, oldString, newString, workspaceService, notebookService as INotebookService, alternatveContentService, undefined); + async function doApplyEdit(oldString: string, newString: string, uri = doc.document.uri, replaceAll = false) { + const r = await applyEdit(uri, oldString, newString, workspaceService, notebookService as INotebookService, alternatveContentService, undefined, replaceAll); workspaceEdit.set(uri, r.edits); return r; } @@ -64,6 +64,41 @@ describe('replace_string_in_file - applyEdit', () => { await expect(doApplyEdit('test', 'replacement')).rejects.toThrow(MultipleMatchesError); }); + test('multiple exact matches - replaceAll should replace all occurrences', async () => { + setText('test\ntest\nother'); + const result = await doApplyEdit('test', 'replacement', doc.document.uri, true); + expect(result.updatedFile).toBe('replacement\nreplacement\nother'); + }); + + test('multiple fuzzy matches - replaceAll should replace all occurrences', async () => { + setText('line one\nline two\nline three'); + const result = await doApplyEdit('line', 'item', doc.document.uri, true); + expect(result.updatedFile).toBe('item one\nitem two\nitem three'); + }); + + test('multiple exact matches - error message includes line numbers', async () => { + setText('test\ntest\nother\ntest'); + try { + await doApplyEdit('test', 'replacement'); + expect.fail('Should have thrown MultipleMatchesError'); + } catch (error) { + expect(error).toBeInstanceOf(MultipleMatchesError); + expect(error.message).toContain('lines 1, 2, 4'); + expect(error.message).toContain('replaceAll: true'); + } + }); + + test('replaceAll with single occurrence should work normally', async () => { + setText('single test here'); + const result = await doApplyEdit('test', 'replacement', doc.document.uri, true); + expect(result.updatedFile).toBe('single replacement here'); + }); + + test('replaceAll with no matches should fail normally', async () => { + setText('no match here'); + await expect(doApplyEdit('test', 'replacement', doc.document.uri, true)).rejects.toThrow(NoMatchError); + }); + test('whitespace flexible matching - different indentation', async () => { setText('function test() {\n console.log("hello");\n}'); // Use the exact text from the file for this test diff --git a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx index b16b4177d..ad7e69440 100644 --- a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx +++ b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx @@ -34,7 +34,8 @@ suite('MultiReplaceString', () => { createTextDocumentData(URI.file('/workspace/empty.ts'), '', 'ts'), createTextDocumentData(URI.file('/workspace/whitespace.ts'), ' \t\n', 'ts'), createTextDocumentData(URI.file('/workspace/large.ts'), largeContent, 'ts'), - createTextDocumentData(URI.file('/workspace/multi-sr-bug.ts'), readFileSync(__dirname + '/editFileToolUtilsFixtures/multi-sr-bug-original.txt', 'utf-8'), 'ts') + createTextDocumentData(URI.file('/workspace/multi-sr-bug.ts'), readFileSync(__dirname + '/editFileToolUtilsFixtures/multi-sr-bug-original.txt', 'utf-8'), 'ts'), + createTextDocumentData(URI.file('/workspace/replaceall-test.ts'), 'test\ntest\nother\ntest', 'ts') ]; for (const doc of allDocs) { documents.set(doc.document.uri, doc); @@ -155,4 +156,40 @@ import { IFile } from '../../../../../base/node/zip.js';` const r = await invoke(input); expect(await applyEditsInMap(r.edits)).toMatchFileSnapshot(__dirname + '/editFileToolUtilsFixtures/multi-sr-bug-actual.txt'); }); + + test('replaceAll functionality', async () => { + const input: IMultiReplaceStringToolParams = { + explanation: 'Replace all occurrences of "test" with "replacement"', + replacements: [{ + filePath: '/workspace/replaceall-test.ts', + explanation: 'Replace all occurrences of "test" with "replacement"', + newString: 'replacement', + oldString: 'test', + replaceAll: true + }] + }; + + const r = await invoke(input); + const results = await applyEditsInMap(r.edits); + expect(results['file:///workspace/replaceall-test.ts']).toBe('replacement\nreplacement\nother\nreplacement'); + }); + + test('multiple matches without replaceAll should provide helpful error', async () => { + const input: IMultiReplaceStringToolParams = { + explanation: 'Try to replace "test" without replaceAll flag', + replacements: [{ + filePath: '/workspace/replaceall-test.ts', + explanation: 'Try to replace "test" without replaceAll flag', + newString: 'replacement', + oldString: 'test', + // replaceAll is not set, should default to false + }] + }; + + const r = await invoke(input); + // The result should contain error information + expect(r.result).toBeDefined(); + // Note: We can't easily test the exact error message here due to the tool's structure, + // but the core logic is tested in editFileToolUtils.spec.ts + }); });