diff --git a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts index 23900fc142..207b88a265 100644 --- a/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts +++ b/src/core/diff/strategies/__tests__/multi-search-replace.spec.ts @@ -1181,5 +1181,122 @@ function sum(a, b) { expect(result.error).toContain("INCORRECT FORMAT:") expect(result.error).toContain(":start_line:5 <-- Invalid location") }) + describe("Issue #2325 - Closing bracket duplication", () => { + let strategy: MultiSearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new MultiSearchReplaceDiffStrategy() + }) + + it("should not duplicate closing brackets when appending at end of file", async () => { + // This is the exact scenario from the issue + const originalContent = `function example() { + console.log("Hello"); +}` + + // The problematic diff that causes duplication + // Model searches for the line before the closing bracket + // and includes the bracket in the replacement + const diffContent = `test.ts +<<<<<<< SEARCH +:start_line:2 +------- + console.log("Hello"); +======= + console.log("Hello"); + console.log("World"); +} +>>>>>>> REPLACE` + + // Apply the diff + const result = await strategy.applyDiff(originalContent, diffContent) + + // The issue would cause this to become: + // function example() { + // console.log("Hello"); + // console.log("World"); + // }} // <-- Double closing bracket + + if (result.success) { + // Count closing brackets + const closingBrackets = (result.content.match(/}/g) || []).length + + // Should only have one closing bracket + expect(closingBrackets).toBe(1) + expect(result.content).toBe(`function example() { + console.log("Hello"); + console.log("World"); +}`) + } else { + // If the test passes (no duplication), this branch should execute + // because the current implementation should fail to find exact match + expect(result.success).toBe(false) + } + }) + + it("should handle appending before closing bracket correctly", async () => { + const originalContent = `class MyClass { + method1() { + return true; + } +}` + + // Correct way: search includes the closing bracket + const diffContent = `test.ts +<<<<<<< SEARCH +:start_line:4 +------- + } +} +======= + } + + method2() { + return false; + } +} +>>>>>>> REPLACE` + + const result = await strategy.applyDiff(originalContent, diffContent) + + if (result.success) { + // Count closing brackets - should be 3 (one for method1, one for method2, one for class) + const closingBrackets = (result.content.match(/}/g) || []).length + expect(closingBrackets).toBe(3) + } + }) + + it("should not duplicate when model incorrectly includes bracket in replacement", async () => { + const originalContent = `{ + "name": "test", + "version": "1.0.0" +}` + + // Problematic pattern: searching for content before bracket + // but including bracket in replacement + const diffContent = `test.ts +<<<<<<< SEARCH +:start_line:3 +------- + "version": "1.0.0" +======= + "version": "1.0.0", + "description": "test package" +} +>>>>>>> REPLACE` + + const result = await strategy.applyDiff(originalContent, diffContent) + + if (result.success) { + // Should not have duplicate closing brackets + const closingBrackets = (result.content.match(/}/g) || []).length + expect(closingBrackets).toBe(1) + } else { + // If the test passes (no duplication), this branch should execute + // because the current implementation should fail to find exact match + expect(result.success).toBe(false) + } + }) + }) }) }) diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index b90ef4072d..31eed247aa 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -331,6 +331,29 @@ Only use a single line of '=======' between search and replacement content, beca } } + /** + * Helper function to check if a line contains only a closing bracket (with optional whitespace) + */ + private isClosingBracketLine(line: string): string | null { + const trimmed = line.trim() + if (trimmed === "}" || trimmed === "]" || trimmed === ")" || trimmed === ">") { + return trimmed + } + return null + } + + /** + * Helper function to get the last non-empty line from content + */ + private getLastNonEmptyLine(lines: string[]): { line: string; index: number } | null { + for (let i = lines.length - 1; i >= 0; i--) { + if (lines[i].trim()) { + return { line: lines[i], index: i } + } + } + return null + } + async applyDiff( originalContent: string, diffContent: string, @@ -551,6 +574,46 @@ Only use a single line of '=======' between search and replacement content, beca // Get the matched lines from the original content const matchedLines = resultLines.slice(matchIndex, matchIndex + searchLines.length) + // Check for potential bracket duplication + let additionalLinesToReplace = 0 + + // Check if replacement ends with a closing bracket that's not in the search + const lastReplaceLine = this.getLastNonEmptyLine(replaceLines) + const lastSearchLine = this.getLastNonEmptyLine(searchLines) + + if (lastReplaceLine && lastSearchLine) { + const replaceBracket = this.isClosingBracketLine(lastReplaceLine.line) + const searchBracket = this.isClosingBracketLine(lastSearchLine.line) + + // If replacement has a closing bracket but search doesn't + if (replaceBracket && !searchBracket) { + // Check if the next line in original content is the same bracket + const nextLineIndex = matchIndex + searchLines.length + if (nextLineIndex < resultLines.length) { + const nextLineBracket = this.isClosingBracketLine(resultLines[nextLineIndex]) + if (nextLineBracket === replaceBracket) { + // Extend the replacement to include the bracket line + additionalLinesToReplace = 1 + + // Also check for multiple consecutive bracket lines + let checkIndex = nextLineIndex + 1 + while (checkIndex < resultLines.length) { + const bracket = this.isClosingBracketLine(resultLines[checkIndex]) + if ( + bracket && + replaceLines.some((line) => this.isClosingBracketLine(line) === bracket) + ) { + additionalLinesToReplace++ + checkIndex++ + } else { + break + } + } + } + } + } + } + // Get the exact indentation (preserving tabs/spaces) of each line const originalIndents = matchedLines.map((line) => { const match = line.match(/^[\t ]*/) @@ -590,9 +653,9 @@ Only use a single line of '=======' between search and replacement content, beca // Construct the final content const beforeMatch = resultLines.slice(0, matchIndex) - const afterMatch = resultLines.slice(matchIndex + searchLines.length) + const afterMatch = resultLines.slice(matchIndex + searchLines.length + additionalLinesToReplace) resultLines = [...beforeMatch, ...indentedReplaceLines, ...afterMatch] - delta = delta - matchedLines.length + replaceLines.length + delta = delta - (matchedLines.length + additionalLinesToReplace) + replaceLines.length appliedCount++ } const finalContent = resultLines.join(lineEnding)