From efb866e6038574042bd0964fa481fc71f4aa6763 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 14 Nov 2025 12:11:20 -0800 Subject: [PATCH 1/3] edits: fix conflicting edits in multi-replace-string - Minimize identical content in text edits to avoid potential conflicts in context - Explicitly error any edits that still fail rather than garbling the file Closes https://github.com/microsoft/vscode/issues/277154 --- .../tools/node/abstractReplaceStringTool.tsx | 44 +++- .../tools/node/editFileToolUtils.tsx | 128 +++++++-- .../tools/node/test/editFileToolUtils.spec.ts | 243 +++++++++++++++++- .../node/test/multiReplaceStringTool.spec.tsx | 11 +- 4 files changed, 396 insertions(+), 30 deletions(-) diff --git a/src/extension/tools/node/abstractReplaceStringTool.tsx b/src/extension/tools/node/abstractReplaceStringTool.tsx index 708f19c0c5..888606ac19 100644 --- a/src/extension/tools/node/abstractReplaceStringTool.tsx +++ b/src/extension/tools/node/abstractReplaceStringTool.tsx @@ -25,6 +25,7 @@ import { removeLeadingFilepathComment } from '../../../util/common/markdown'; import { timeout } from '../../../util/vs/base/common/async'; import { Iterable } from '../../../util/vs/base/common/iterator'; import { ResourceMap, ResourceSet } from '../../../util/vs/base/common/map'; +import { extUriBiasedIgnorePathCase } from '../../../util/vs/base/common/resources'; import { isDefined } from '../../../util/vs/base/common/types'; import { URI } from '../../../util/vs/base/common/uri'; import { IInstantiationService } from '../../../util/vs/platform/instantiation/common/instantiation'; @@ -102,13 +103,54 @@ export abstract class AbstractReplaceStringTool this._prepareEditsForFile(options, i, token))), + operation: this._prepareEdits(options, input, token) }; } return this.lastOperation.operation; } + private async _prepareEdits(options: vscode.LanguageModelToolInvocationOptions | vscode.LanguageModelToolInvocationPrepareOptions, input: IAbstractReplaceStringInput[], token: vscode.CancellationToken) { + const results = await Promise.all(input.map(i => this._prepareEditsForFile(options, i, token))); + this._errorConflictingEdits(results); + return results; + } + + private _errorConflictingEdits(results: IPrepareEdit[]) { + for (let i = 1; i < results.length; i++) { + const current = results[i]; + if (!current.generatedEdit.success) { + continue; + } + + for (let k = 0; k < i; k++) { + const other = results[k]; + if (!other.generatedEdit.success || !extUriBiasedIgnorePathCase.isEqual(current.uri, other.uri)) { + continue; + } + + const allEdits = [ + ...current.generatedEdit.textEdits, + ...other.generatedEdit.textEdits, + ].sort((a, b) => a.range.start.compareTo(b.range.start)); + + const hasOverlap = allEdits.some((e2, i) => { + if (i === 0) { return false; } + const e1 = allEdits[i - 1]; + return !e1.range.end.isBeforeOrEqual(e2.range.start); + }); + + if (hasOverlap) { + current.generatedEdit = { + success: false, + errorMessage: `Edit at index ${i} conflicts with another replacement in ${this.promptPathRepresentationService.getFilePath(current.uri)}. You can make another call to try again.` + }; + break; + } + } + } + } + private async _prepareEditsForFile(options: vscode.LanguageModelToolInvocationOptions | vscode.LanguageModelToolInvocationPrepareOptions, input: IAbstractReplaceStringInput, token: vscode.CancellationToken): Promise { const uri = resolveToolInputPath(input.filePath, this.promptPathRepresentationService); try { diff --git a/src/extension/tools/node/editFileToolUtils.tsx b/src/extension/tools/node/editFileToolUtils.tsx index ccf459bc49..d1ac08d3c1 100644 --- a/src/extension/tools/node/editFileToolUtils.tsx +++ b/src/extension/tools/node/editFileToolUtils.tsx @@ -191,10 +191,10 @@ function calculateSimilarity(str1: string, str2: string): number { interface MatchResultCommon { type: string; - /** Replacement text */ + /** Resulting document text */ text: string; /** Array of [startIndex, endIndex] to replace in the file content */ - editPosition: [number, number][]; + editPosition: { start: number; end: number; text: string }[]; /** Model suggestion to correct a fialing match */ suggestion?: string; } @@ -274,12 +274,19 @@ function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResul return { text, editPosition: [], type: 'none' }; } + const identical = getIdenticalChars(oldStr, newStr); + const editPosition = matchPositions.map(idx => ({ + start: idx + identical.leading, + end: idx + oldStr.length - identical.trailing, + text: newStr.slice(identical.leading, newStr.length - identical.trailing) + })); + // Check for multiple exact occurrences. if (matchPositions.length > 1) { return { text, type: 'multiple', - editPosition: matchPositions.map(idx => [idx, idx + oldStr.length]), + editPosition, strategy: 'exact', matchPositions, suggestion: "Multiple exact matches found. Make your search string more specific." @@ -291,7 +298,7 @@ function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResul return { text: replaced, type: 'exact', - editPosition: [[firstExactIdx, firstExactIdx + oldStr.length]], + editPosition, }; } @@ -300,7 +307,8 @@ function tryExactMatch(text: string, oldStr: string, newStr: string): MatchResul */ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string, eol: string): MatchResult { const haystack = text.split(eol).map(line => line.trim()); - const needle = oldStr.trim().split(eol).map(line => line.trim()); + const oldLines = oldStr.trim().split(eol); + const needle = oldLines.map(line => line.trim()); needle.push(''); // trailing newline to match until the end of a line const convert = new OffsetLineColumnConverter(text); @@ -320,7 +328,10 @@ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string }; } - const positions = matchedLines.map(match => convert.positionToOffset(new EditorPosition(match + 1, 1))); + + const newLines = newStr.trim().split(eol); + const identical = getIndenticalLines(oldLines, newLines); + const positions = matchedLines.map(match => convert.positionToOffset(new EditorPosition(match + identical.leading + 1, 1))); if (matchedLines.length > 1) { return { @@ -335,11 +346,12 @@ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string // Exactly one whitespace-flexible match found const startIdx = positions[0]; - const endIdx = convert.positionToOffset(new EditorPosition(matchedLines[0] + 1 + needle.length, 1)); - const replaced = text.slice(0, startIdx) + newStr + eol + text.slice(endIdx); + const endIdx = convert.positionToOffset(new EditorPosition(matchedLines[0] + 1 + needle.length - identical.trailing, 1)); + const minimizedNewStr = newLines.slice(identical.leading, newLines.length - identical.trailing).join(eol); + const replaced = text.slice(0, startIdx) + minimizedNewStr + eol + text.slice(endIdx); return { text: replaced, - editPosition: [[startIdx, endIdx]], + editPosition: [{ start: startIdx, end: endIdx, text: minimizedNewStr }], type: 'whitespace', }; } @@ -356,11 +368,11 @@ function tryFuzzyMatch(text: string, oldStr: string, newStr: string, eol: string // Build a regex pattern where each line is matched exactly // but allows for trailing spaces/tabs and flexible newline formats - const lines = oldStr.split(eol); - const pattern = lines + const oldLines = oldStr.split(eol); + const pattern = oldLines .map((line, i) => { const escaped = escapeRegex(line); - return i < lines.length - 1 || hasTrailingLF + return i < oldLines.length - 1 || hasTrailingLF ? `${escaped}[ \\t]*\\r?\\n` : `${escaped}[ \\t]*`; }) @@ -395,15 +407,23 @@ function tryFuzzyMatch(text: string, oldStr: string, newStr: string, eol: string return { text: replaced, type: 'fuzzy', - editPosition: [[startIdx, endIdx]], + editPosition: [{ start: startIdx, end: endIdx, text: newStr }], }; } +let defaultSimilaryMatchThreshold = 0.95; + +export function setSimilarityMatchThresholdForTests(threshold: number) { + const old = defaultSimilaryMatchThreshold; + defaultSimilaryMatchThreshold = threshold; + return old; +} + /** * Tries to match based on overall string similarity as a last resort. * Only works for relatively small strings to avoid performance issues. */ -function trySimilarityMatch(text: string, oldStr: string, newStr: string, eol: string, threshold: number = 0.95): MatchResult { +function trySimilarityMatch(text: string, oldStr: string, newStr: string, eol: string, threshold: number = defaultSimilaryMatchThreshold): MatchResult { // Skip similarity matching for very large strings or too many lines if (oldStr.length > 1000 || oldStr.split(eol).length > 20) { return { text, editPosition: [], type: 'none' }; @@ -417,6 +437,9 @@ function trySimilarityMatch(text: string, oldStr: string, newStr: string, eol: s return { text, editPosition: [], type: 'none' }; } + const newLines = newStr.split(eol); + const identical = getIndenticalLines(oldLines, newLines); + let bestMatch = { startLine: -1, startOffset: 0, oldLength: 0, similarity: 0 }; let startOffset = 0; @@ -426,15 +449,29 @@ function trySimilarityMatch(text: string, oldStr: string, newStr: string, eol: s let oldLength = 0; // Calculate similarity for each line in the window + let startOffsetIdenticalIncr = 0; + let endOffsetIdenticalIncr = 0; for (let j = 0; j < oldLines.length; j++) { const similarity = calculateSimilarity(oldLines[j], lines[i + j]); totalSimilarity += similarity; oldLength += lines[i + j].length; + + if (j < identical.leading) { + startOffsetIdenticalIncr += lines[i + j].length + eol.length; + } + if (j >= oldLines.length - identical.trailing) { + endOffsetIdenticalIncr += lines[i + j].length + eol.length; + } } const avgSimilarity = totalSimilarity / oldLines.length; if (avgSimilarity > threshold && avgSimilarity > bestMatch.similarity) { - bestMatch = { startLine: i, startOffset, similarity: avgSimilarity, oldLength: oldLength + (oldLines.length - 1) * eol.length }; + bestMatch = { + startLine: i + identical.leading, + startOffset: startOffset + startOffsetIdenticalIncr, + similarity: avgSimilarity, + oldLength: oldLength + (oldLines.length - 1) * eol.length - startOffsetIdenticalIncr - endOffsetIdenticalIncr, + }; } startOffset += lines[i].length + eol.length; @@ -445,16 +482,24 @@ function trySimilarityMatch(text: string, oldStr: string, newStr: string, eol: s } // Replace the matched section - const newLines = [ + const newStrMinimized = newLines.slice(identical.leading, newLines.length - identical.trailing).join(eol); + const matchStart = bestMatch.startLine - identical.leading; + const afterIdx = matchStart + oldLines.length - identical.trailing; + + const newText = [ ...lines.slice(0, bestMatch.startLine), - ...newStr.split(eol), - ...lines.slice(bestMatch.startLine + oldLines.length) - ]; + ...newLines.slice(identical.leading, newLines.length - identical.trailing), + ...lines.slice(afterIdx), + ].join(eol); return { - text: newLines.join(eol), + text: newText, type: 'similarity', - editPosition: [[bestMatch.startOffset, bestMatch.startOffset + bestMatch.oldLength]], + editPosition: [{ + start: bestMatch.startOffset, + end: bestMatch.startOffset + bestMatch.oldLength, + text: newStrMinimized, + }], similarity: bestMatch.similarity, suggestion: `Used similarity matching (${(bestMatch.similarity * 100).toFixed(1)}% similar). Verify the replacement.` }; @@ -472,6 +517,39 @@ function getPatch({ fileContents, oldStr, newStr }: { fileContents: string; oldS }]; } +/** Gets the number of identical leading and trailing lines between two arrays of strings */ +function getIndenticalLines(a: string[], b: string[]) { + let leading = 0; + let trailing = 0; + while (leading < a.length && + leading < b.length && + a[leading] === b[leading]) { + leading++; + } + while (trailing + leading < a.length && + trailing + leading < b.length && + a[a.length - 1 - trailing] === b[b.length - 1 - trailing]) { + trailing++; + } + + return { leading, trailing }; +} + +/** Gets the number of identical leading and trailing characters between two strings */ +function getIdenticalChars(oldString: string, newString: string) { + let leading = 0; + let trailing = 0; + + while (leading < oldString.length && leading < newString.length && oldString[leading] === newString[leading]) { + leading++; + } + while (trailing + leading < oldString.length && trailing + leading < newString.length && + oldString[oldString.length - trailing - 1] === newString[newString.length - trailing - 1]) { + trailing++; + } + return { leading, trailing }; +} + // Apply string edit function export async function applyEdit( uri: URI, @@ -518,7 +596,7 @@ export async function applyEdit( updatedFile = originalFile.replace(old_string + eol, new_string); if (result.editPosition.length) { - const [start, end] = result.editPosition[0]; + const { start, end } = result.editPosition[0]; const range = new Range(document.positionAt(start), document.positionAt(end)); edits.push(TextEdit.delete(range)); } @@ -539,7 +617,7 @@ export async function applyEdit( updatedFile = result.text; if (result.editPosition.length) { - const [start, end] = result.editPosition[0]; + const { start, end } = result.editPosition[0]; const range = new Range(document.positionAt(start), document.positionAt(end)); edits.push(TextEdit.delete(range)); } @@ -564,9 +642,9 @@ export async function applyEdit( updatedFile = result.text; if (result.editPosition.length) { - const [start, end] = result.editPosition[0]; + const { start, end, text } = result.editPosition[0]; const range = new Range(document.positionAt(start), document.positionAt(end)); - edits.push(TextEdit.replace(range, new_string)); + edits.push(TextEdit.replace(range, text)); } // If we used similarity matching, add a warning diff --git a/src/extension/tools/node/test/editFileToolUtils.spec.ts b/src/extension/tools/node/test/editFileToolUtils.spec.ts index 3728f6614b..dfe7c92c24 100644 --- a/src/extension/tools/node/test/editFileToolUtils.spec.ts +++ b/src/extension/tools/node/test/editFileToolUtils.spec.ts @@ -5,7 +5,7 @@ import * as fs from 'fs'; import { homedir } from 'os'; -import { beforeEach, describe, expect, test } from 'vitest'; +import { afterEach, beforeEach, describe, expect, test } from 'vitest'; import { DefaultsOnlyConfigurationService } from '../../../../platform/configuration/common/defaultsOnlyConfigurationService'; import { InMemoryConfigurationService } from '../../../../platform/configuration/test/common/inMemoryConfigurationService'; import type { ICustomInstructionsService } from '../../../../platform/customInstructions/common/customInstructionsService'; @@ -19,7 +19,7 @@ import { isMacintosh } from '../../../../util/vs/base/common/platform'; import { URI } from '../../../../util/vs/base/common/uri'; import { WorkspaceEdit } from '../../../../vscodeTypes'; import { applyEdits as applyTextEdits } from '../../../prompt/node/intents'; -import { applyEdit, assertPathIsSafe, ConfirmationCheckResult, ContentFormatError, makeUriConfirmationChecker, MultipleMatchesError, NoChangeError, NoMatchError } from '../editFileToolUtils'; +import { applyEdit, assertPathIsSafe, ConfirmationCheckResult, ContentFormatError, makeUriConfirmationChecker, MultipleMatchesError, NoChangeError, NoMatchError, setSimilarityMatchThresholdForTests } from '../editFileToolUtils'; describe('replace_string_in_file - applyEdit', () => { let workspaceEdit: WorkspaceEdit; @@ -357,6 +357,245 @@ describe('replace_string_in_file - applyEdit', () => { applyTextEdits(input.join('\r\n'), workspaceEdit.entries()[0][1]) ).toBe(output); }); + + // Whitespace-flexible matching strategy tests + // Note: Whitespace-flexible matching only triggers when: + // 1. No exact match exists + // 2. No fuzzy match exists (fuzzy allows trailing spaces but not leading/different indentation) + // 3. Trimmed lines match exactly AND there's an empty line after the match + describe('whitespace-flexible matching', () => { + test('matches when file has empty line after content', async () => { + // File has content followed by empty line, with varying indentation + setText('function test() {\n \tconsole.log("hello");\n\treturn true;\n\n}'); + // Search for content with trailing newline - the empty line in file will match the empty needle element + const result = await doApplyEdit('console.log("hello");\nreturn true;\n', 'console.log("updated");\nreturn false;\n'); + expect(result.updatedFile).toContain('console.log("updated");'); + expect(result.updatedFile).toContain('return false;'); + }); + + test('matches when indentation varies and empty line follows', async () => { + setText('if (x) {\n\t\t if (y) {\n \t\tcode();\n\t \t}\n\n}'); + // Empty line in file matches empty string in needle + const result = await doApplyEdit('if (y) {\ncode();\n}\n', 'if (y) {\nupdated();\n}\n'); + expect(result.updatedFile).toContain('updated();'); + }); + + test('throws error on multiple matches with empty lines', async () => { + setText('function a() {\n \treturn 1;\n\n}\nfunction b() {\n\t return 1;\n\n}'); + // Both functions have same content when trimmed, followed by empty lines + await expect(doApplyEdit('return 1;\n', 'return 2;\n')).rejects.toThrow(MultipleMatchesError); + }); + + test('matches block with trailing empty line preserving structure', async () => { + setText('class Test {\n\t method() {\n \t\tconst x = 1;\n\t const y = 2;\n\n \t}\n}'); + // Search with trailing newline to match the empty line + const result = await doApplyEdit('const x = 1;\nconst y = 2;\n', 'const z = 3;\n'); + expect(result.updatedFile).toContain('const z = 3;'); + expect(result.updatedFile).toContain('class Test'); + }); + + test('whitespace-flexible match minimizes edits with empty line', async () => { + setText('function test() {\n \tconst a = 1;\n\t const b = 2;\n\n}'); + const result = await doApplyEdit('const a = 1;\nconst b = 2;\n', 'const a = 1;\nconst b = 3;\n'); + // Should preserve identical first line + expect(result.edits.length).toBe(1); + expect(result.updatedFile).toContain('const b = 3;'); + }); + + test('empty line in haystack required for whitespace-flexible match', async () => { + setText('line1\n \tline2\n\n\t line3'); + // Search with trailing newline - empty line in haystack matches empty needle element + const result = await doApplyEdit('line1\nline2\n', 'new1\nnew2\n'); + expect(result.updatedFile).toContain('new1'); + expect(result.updatedFile).toContain('new2'); + expect(result.updatedFile).toContain('line3'); + }); + }); + + // Similarity-based matching strategy tests + describe('similarity matching', () => { + test('matches highly similar content with minor differences', async () => { + setText('function calculate(items) {\n\tlet total = 0;\n\tfor (let i = 0; i < items.length; i++) {\n\t\ttotal += items[i].price;\n\t}\n\treturn total;\n}'); + // Search string has slightly different variable name - 1 char diff in one place + const result = await doApplyEdit( + 'function calculate(items) {\n\tlet total = 0;\n\tfor (let i = 0; i < items.length; i++) {\n\t\ttotal += items[i].pric;\n\t}\n\treturn total;\n}', + 'function calculate(items) {\n\treturn items.reduce((acc, item) => acc + item.price, 0);\n}' + ); + expect(result.updatedFile).toBe('function calculate(items) {\n\treturn items.reduce((acc, item) => acc + item.price, 0);\n}'); + }); + + test('similarity match with small typos in search string', async () => { + setText('const message = "Hello, World!";\nconsole.log(message);'); + // Search has a typo but high similarity (95%+) + const result = await doApplyEdit('const mesage = "Hello, World!";\nconsole.log(message);', 'const greeting = "Hi there!";\nconsole.log(greeting);'); + // Should find a match and replace + expect(result.updatedFile).toContain('greeting'); + }); + + test('similarity match does not trigger for low similarity', async () => { + setText('function test() {\n\treturn true;\n}'); + // Very different content should not match + await expect(doApplyEdit('completely different text here with no similarity at all to the original', 'replacement')).rejects.toThrow(NoMatchError); + }); + + test('similarity match prefers best match among candidates', async () => { + setText('function a() {\n\tconst x = 1;\n\tconst y = 2;\n}\nfunction b() {\n\tconst x = 1;\n\tconst z = 3;\n}'); + // Should match function a (higher similarity with y vs z) + const result = await doApplyEdit('function a() {\nconst x = 1;\nconst y = 2;\n}', 'function a() {\nconst result = 3;\n}'); + expect(result.updatedFile).toContain('const result = 3'); + expect(result.updatedFile).toContain('function b()'); // Second function unchanged + }); + + test('similarity match skips very large strings', async () => { + // Similarity matching should skip strings > 1000 chars or > 20 lines + const largeText = 'line\n'.repeat(50) + 'target line\n' + 'line\n'.repeat(50); + setText(largeText); + // Should fall back to exact/fuzzy matching instead of similarity + const result = await doApplyEdit('target line', 'replaced line'); + expect(result.updatedFile).toContain('replaced line'); + }); + + test('similarity match minimizes edits - preserves identical lines', async () => { + setText('function test() {\n\tconst a = 1;\n\tconst b = 2;\n\tconst c = 3;\n}'); + const result = await doApplyEdit( + 'function test() {\n\tconst a = 1;\n\tconst x = 2;\n\tconst c = 3;\n}', + 'function test() {\n\tconst a = 1;\n\tconst y = 4;\n\tconst c = 3;\n}' + ); + // Should preserve identical first and last lines + expect(result.updatedFile).toContain('const a = 1'); + expect(result.updatedFile).toContain('const y = 4'); + expect(result.updatedFile).toContain('const c = 3'); + }); + + test('similarity match with small content blocks', async () => { + setText('const x = 1;\nconst y = 2;\nconst z = 3;'); + // Small similar block should match via similarity + const result = await doApplyEdit('const x = 1;\nconst w = 2;', 'const a = 10;\nconst b = 20;'); + // Should match first two lines and replace them + expect(result.updatedFile).toContain('const a = 10'); + expect(result.updatedFile).toContain('const b = 20'); + }); + + describe('similarity match - edge cases for slice calculations', () => { + let prev: number; + beforeEach(() => { + prev = setSimilarityMatchThresholdForTests(0.6); + }); + + afterEach(() => { + setSimilarityMatchThresholdForTests(prev); + }); + + test('similarity match preserves lines after replacement when there are identical trailing lines', async () => { + // This test checks for off-by-one errors in the slice calculation + setText('function test() {\n\tconst a = 1;\n\tconst b = 2;\n\tconst c = 3;\n\tconst d = 4;\n}'); + // Search has identical first and last lines, different middle + const result = await doApplyEdit( + 'function test() {\n\tconst a = 1;\n\tconst x = 2;\n\tconst y = 3;\n\tconst d = 4;\n}', + 'function test() {\n\tconst a = 1;\n\tconst newB = 20;\n\tconst newC = 30;\n\tconst d = 4;\n}' + ); + // Should preserve the closing brace + expect(result.updatedFile).toBe('function test() {\n\tconst a = 1;\n\tconst newB = 20;\n\tconst newC = 30;\n\tconst d = 4;\n}'); + }); + + test('similarity match with multiple identical trailing lines', async () => { + // Edge case that tests the slice calculation for multiple trailing lines + // Use similar strings to meet the 60% threshold while ensuring window i=0 has best match + setText('EXACT_START\nchange_me_1\nchange_me_2\nEXACT_END1\nEXACT_END2'); + // Window at i=0: EXACT_START (100%) + change_me_1 vs modify_1 (~70%) + change_me_2 vs modify_2 (~70%) + EXACT_END1 (100%) + EXACT_END2 (100%) = ~88% + const result = await doApplyEdit( + 'EXACT_START\nmodify_1\nmodify_2\nEXACT_END1\nEXACT_END2', + 'EXACT_START\nNEW_1\nNEW_2\nEXACT_END1\nEXACT_END2' + ); + // Should match window at i=0 and replace the middle 2 lines + expect(result.updatedFile).toBe('EXACT_START\nNEW_1\nNEW_2\nEXACT_END1\nEXACT_END2'); + }); test('similarity match boundary: no identical lines', async () => { + setText('aaa\nbbb\nccc'); + // With low similarity, should not match + await expect(doApplyEdit('xxx\nyyy\nzzz', 'new1\nnew2\nnew3')).rejects.toThrow(NoMatchError); + }); + + test('similarity match edge case: all identical lines except middle', async () => { + // This tests the slice calculation with identical leading and trailing lines + // File has 5 lines, search differs in 1 line = 80% similarity, above 60% threshold + setText('Alpha\nBravo\nCharlie\nDelta\nEcho'); + // Search has identical first 2 and last 2, different middle + // identical.leading = 2, identical.trailing = 2 + const result = await doApplyEdit( + 'Alpha\nBravo\nXray\nDelta\nEcho', + 'Alpha\nBravo\nNEW\nDelta\nEcho' + ); + // Should replace only line Charlie with NEW, preserving all other lines + expect(result.updatedFile).toBe('Alpha\nBravo\nNEW\nDelta\nEcho'); + }); + + test('similarity match edge case: only last line differs', async () => { + // Tests the slice calculation when identical.trailing = 0 + // 3/4 lines match = 75% similarity > 60% threshold + setText('start_line\nmiddle_one\nmiddle_two\nold_ending'); + // Search matches first 3 lines, last is different + // identical.leading = 3, identical.trailing = 0 + const result = await doApplyEdit( + 'start_line\nmiddle_one\nmiddle_two\nwrong_ending', + 'start_line\nmiddle_one\nmiddle_two\nnew_ending' + ); + // Should preserve first 3 lines and replace only last line + expect(result.updatedFile).toBe('start_line\nmiddle_one\nmiddle_two\nnew_ending'); + }); + + test('similarity match edge case: only first line differs', async () => { + // Tests the slice calculation when identical.leading = 0 + // 3/4 lines match = 75% similarity > 60% threshold + setText('old_beginning\nmiddle_one\nmiddle_two\nending_line'); + // Search matches last 3 lines, first is different + // identical.leading = 0, identical.trailing = 3 + const result = await doApplyEdit( + 'wrong_beginning\nmiddle_one\nmiddle_two\nending_line', + 'new_beginning\nmiddle_one\nmiddle_two\nending_line' + ); + // Should replace only first line, preserve last 3 + expect(result.updatedFile).toBe('new_beginning\nmiddle_one\nmiddle_two\nending_line'); + }); + }); + }); + + // Edit minimization tests across all strategies + describe('edit minimization', () => { + test('exact match minimizes edits - preserves identical prefix/suffix', async () => { + setText('prefix unchanged middle changed suffix unchanged'); + const result = await doApplyEdit('prefix unchanged middle changed suffix unchanged', 'prefix unchanged middle updated suffix unchanged'); + // Should only edit the "changed" -> "updated" part + expect(result.edits.length).toBe(1); + expect(result.edits[0].newText).toBe('updat'); + }); + + test('fuzzy match only replaces different content', async () => { + setText('line1\nline2\nline3\n'); + const result = await doApplyEdit('line1\nline2\nline3', 'line1\nmodified\nline3'); + // Should edit the content + expect(result.updatedFile).toBe('line1\nmodified\nline3\n'); + }); + + test('edits array contains correct positions', async () => { + setText('start\ntarget line to change\nend'); + const result = await doApplyEdit('target line to change', 'modified line'); + + expect(result.edits.length).toBe(1); + const edit = result.edits[0]; + + // Verify the edit has the right text + expect(edit.newText).toBe('modified lin'); + // Verify it's on the correct line + expect(edit.range.start.line).toBe(1); // 0-indexed, so line 2 + }); + + test('exact match with partial change minimizes edited text', async () => { + setText('const a = 1;\nconst b = 2;\nconst c = 3;'); + const result = await doApplyEdit('const a = 1;\nconst b = 2;\nconst c = 3;', 'const a = 10;\nconst b = 2;\nconst c = 30;'); + // Should have minimized the edits + expect(result.updatedFile).toBe('const a = 10;\nconst b = 2;\nconst c = 30;'); + }); + }); }); diff --git a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx index 7089b8dd42..d405ee74e7 100644 --- a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx +++ b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx @@ -21,6 +21,7 @@ import { ToolName } from '../../common/toolNames'; import { CopilotToolMode } from '../../common/toolsRegistry'; import { IToolsService } from '../../common/toolsService'; import { IMultiReplaceStringToolParams } from '../multiReplaceStringTool'; +import { toolResultToString } from './toolTestUtils'; suite('MultiReplaceString', () => { let accessor: ITestingServicesAccessor; @@ -154,10 +155,10 @@ 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'); + await expect(await applyEditsInMap(r.edits)).toMatchFileSnapshot(__dirname + '/editFileToolUtilsFixtures/multi-sr-bug-actual.txt'); }); - test.skip('The multi_replace_string_in_file trashed my file due to overlapping replacements #277154', async () => { + test.only('The multi_replace_string_in_file trashed my file due to overlapping replacements #277154', async () => { const input: IMultiReplaceStringToolParams = { "explanation": "Adding JSDoc comments to the div and mul functions", @@ -232,6 +233,12 @@ export function sumThreeFloats(a, b, c) {`, const r = await invoke(input); + expect(await toolResultToString(accessor, r.result)).toMatchInlineSnapshot(` + "The following files were successfully edited: + /workspace/math.js + Edit at index 1 conflicts with another replacement in /workspace/math.js. You can make another call to try again." + `); + const edits = Object.values(r.edits).flat(); // edits.sort((a, b) => a.range.start.compareTo(b.range.start)); From 0889b582c3074ea386d648e20cff9d082c7a3a0e Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 14 Nov 2025 12:18:34 -0800 Subject: [PATCH 2/3] rm test.only --- src/extension/tools/node/test/multiReplaceStringTool.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx index d405ee74e7..7e6f544246 100644 --- a/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx +++ b/src/extension/tools/node/test/multiReplaceStringTool.spec.tsx @@ -158,7 +158,7 @@ import { IFile } from '../../../../../base/node/zip.js';` await expect(await applyEditsInMap(r.edits)).toMatchFileSnapshot(__dirname + '/editFileToolUtilsFixtures/multi-sr-bug-actual.txt'); }); - test.only('The multi_replace_string_in_file trashed my file due to overlapping replacements #277154', async () => { + test('The multi_replace_string_in_file trashed my file due to overlapping replacements #277154', async () => { const input: IMultiReplaceStringToolParams = { "explanation": "Adding JSDoc comments to the div and mul functions", From 0a783ba0393b70c6ecab87feed06ef0837497bf5 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 14 Nov 2025 15:29:20 -0800 Subject: [PATCH 3/3] fix whitespaces not being preserved in ws flex match --- src/extension/tools/node/editFileToolUtils.tsx | 17 +++++++++++------ .../replaceString/fixtures/math.js.txt.expected | 2 ++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/extension/tools/node/editFileToolUtils.tsx b/src/extension/tools/node/editFileToolUtils.tsx index d1ac08d3c1..b834d55aaf 100644 --- a/src/extension/tools/node/editFileToolUtils.tsx +++ b/src/extension/tools/node/editFileToolUtils.tsx @@ -331,24 +331,29 @@ function tryWhitespaceFlexibleMatch(text: string, oldStr: string, newStr: string const newLines = newStr.trim().split(eol); const identical = getIndenticalLines(oldLines, newLines); - const positions = matchedLines.map(match => convert.positionToOffset(new EditorPosition(match + identical.leading + 1, 1))); + const positions = matchedLines.map(match => { + const start = new EditorPosition(match + identical.leading + 1, 1); + const end = start.delta(oldLines.length - identical.trailing); + return { start, end }; + }); if (matchedLines.length > 1) { return { text, type: 'multiple', editPosition: [], - matchPositions: positions, + matchPositions: positions.map(p => convert.positionToOffset(p.start)), suggestion: "Multiple matches found with flexible whitespace. Make your search string more unique.", strategy: 'whitespace', }; } - // Exactly one whitespace-flexible match found - const startIdx = positions[0]; - const endIdx = convert.positionToOffset(new EditorPosition(matchedLines[0] + 1 + needle.length - identical.trailing, 1)); + const { start, end } = positions[0]; + const startIdx = convert.positionToOffset(start); + const endIdx = convert.positionToOffset(end) - 1; // -1 to include the last EOL + const minimizedNewStr = newLines.slice(identical.leading, newLines.length - identical.trailing).join(eol); - const replaced = text.slice(0, startIdx) + minimizedNewStr + eol + text.slice(endIdx); + const replaced = text.slice(0, startIdx) + minimizedNewStr + text.slice(endIdx); return { text: replaced, editPosition: [{ start: startIdx, end: endIdx, text: minimizedNewStr }], diff --git a/src/extension/tools/test/node/replaceString/fixtures/math.js.txt.expected b/src/extension/tools/test/node/replaceString/fixtures/math.js.txt.expected index e897e087b1..10af83682d 100644 --- a/src/extension/tools/test/node/replaceString/fixtures/math.js.txt.expected +++ b/src/extension/tools/test/node/replaceString/fixtures/math.js.txt.expected @@ -45,6 +45,8 @@ export function div(A, b) { return A / b; } + + export function mul(a, b) { return a * b; }