Skip to content

Fix overdependence on start/end lines in diff strategy #2567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrubens merged 2 commits into from
Apr 14, 2025
Merged

Fix overdependence on start/end lines in diff strategy #2567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08d803f
Add test for issue #2556
mrubens Apr 13, 2025
842bb2b
Fix overdependence on start/end lines in diff strategy
mrubens Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 93 additions & 121 deletions src/core/diff/strategies/__tests__/multi-search-replace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,7 @@ function five() {
})
})

describe("insertion/deletion", () => {
describe("deletion", () => {
let strategy: MultiSearchReplaceDiffStrategy

beforeEach(() => {
Expand Down Expand Up @@ -1646,126 +1646,6 @@ function five() {
}
})
})

describe("insertion", () => {
it("should insert code at specified line when search block is empty", async () => {
const originalContent = `function test() {
const x = 1;
return x;
}`
const diffContent = `test.ts
<<<<<<< SEARCH
:start_line:2
:end_line:2
-------
=======
console.log("Adding log");
>>>>>>> REPLACE`

const result = await strategy.applyDiff(originalContent, diffContent, 2, 2)
expect(result.success).toBe(true)
if (result.success) {
expect(result.content).toBe(`function test() {
console.log("Adding log");
const x = 1;
return x;
}`)
}
})

it("should preserve indentation when inserting at nested location", async () => {
const originalContent = `function test() {
if (true) {
const x = 1;
}
}`
const diffContent = `test.ts
<<<<<<< SEARCH
:start_line:3
:end_line:3
-------
=======
console.log("Before");
console.log("After");
>>>>>>> REPLACE`

const result = await strategy.applyDiff(originalContent, diffContent, 3, 3)
expect(result.success).toBe(true)
if (result.success) {
expect(result.content).toBe(`function test() {
if (true) {
console.log("Before");
console.log("After");
const x = 1;
}
}`)
}
})

it("should handle insertion at start of file", async () => {
const originalContent = `function test() {
return true;
}`
const diffContent = `test.ts
<<<<<<< SEARCH
:start_line:1
:end_line:1
-------
=======
// Copyright 2024
// License: MIT

>>>>>>> REPLACE`

const result = await strategy.applyDiff(originalContent, diffContent, 1, 1)
expect(result.success).toBe(true)
if (result.success) {
expect(result.content).toBe(`// Copyright 2024
// License: MIT

function test() {
return true;
}`)
}
})

it("should handle insertion at end of file", async () => {
const originalContent = `function test() {
return true;
}`
const diffContent = `test.ts
<<<<<<< SEARCH
:start_line:4
:end_line:4
-------
=======
// End of file
>>>>>>> REPLACE`

const result = await strategy.applyDiff(originalContent, diffContent, 4, 4)
expect(result.success).toBe(true)
if (result.success) {
expect(result.content).toBe(`function test() {
return true;
}
// End of file`)
}
})

it("should error if no start_line is provided for insertion", async () => {
const originalContent = `function test() {
return true;
}`
const diffContent = `test.ts
<<<<<<< SEARCH
=======
console.log("test");
>>>>>>> REPLACE`

const result = await strategy.applyDiff(originalContent, diffContent)
expect(result.success).toBe(false)
})
})
})

describe("fuzzy matching", () => {
Expand Down Expand Up @@ -1949,6 +1829,98 @@ function three() {
}
})

it("should work correctly on this example with line numbers that are slightly off", async () => {
const originalContent = `.game-container {
display: flex;
flex-direction: column;
gap: 1rem;
}

.chess-board-container {
display: flex;
gap: 1rem;
align-items: center;
}

.overlay {
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
background-color: rgba(0, 0, 0, 0.5);
z-index: 999; /* Ensure it's above the board but below the promotion dialog */
}

.game-container.promotion-active .chess-board,
.game-container.promotion-active .game-toolbar,
.game-container.promotion-active .game-info-container {
filter: blur(2px);
pointer-events: none; /* Disable clicks on these elements */
}

.game-container.promotion-active .promotion-dialog {
z-index: 1000; /* Ensure it's above the overlay */
pointer-events: auto; /* Enable clicks on the promotion dialog */
}`
const diffContent = `test.ts
<<<<<<< SEARCH
:start_line:12
:end_line:13
-------
.overlay {
=======
.piece {
will-change: transform;
}

.overlay {
>>>>>>> REPLACE
`

const result = await strategy.applyDiff(originalContent, diffContent)
expect(result.success).toBe(true)
if (result.success) {
expect(result.content).toBe(`.game-container {
display: flex;
flex-direction: column;
gap: 1rem;
}

.chess-board-container {
display: flex;
gap: 1rem;
align-items: center;
}

.piece {
will-change: transform;
}

.overlay {
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
background-color: rgba(0, 0, 0, 0.5);
z-index: 999; /* Ensure it's above the board but below the promotion dialog */
}

.game-container.promotion-active .chess-board,
.game-container.promotion-active .game-toolbar,
.game-container.promotion-active .game-info-container {
filter: blur(2px);
pointer-events: none; /* Disable clicks on these elements */
}

.game-container.promotion-active .promotion-dialog {
z-index: 1000; /* Ensure it's above the overlay */
pointer-events: auto; /* Enable clicks on the promotion dialog */
}`)
}
})

it("should not find matches outside search range and buffer zone", async () => {
const originalContent = `
function one() {
Expand Down
20 changes: 6 additions & 14 deletions src/core/diff/strategies/multi-search-replace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import { ToolUse } from "../../assistant-message"
const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches

function getSimilarity(original: string, search: string): number {
// Empty searches are no longer supported
if (search === "") {
return 1
return 0
}

// Normalize strings by removing extra whitespace but preserve case
Expand Down Expand Up @@ -367,7 +368,6 @@ Only use a single line of '=======' between search and replacement content, beca
const replacements = matches
.map((match) => ({
startLine: Number(match[2] ?? 0),
endLine: Number(match[4] ?? resultLines.length),
Copy link
Collaborator Author

@mrubens mrubens Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not using the passed in endLine anymore (just taking the start line plus the length of the search block), but I figure we can clean that up as a follow-up if this doesn't cause any unforeseen problems.

Copy link
Collaborator Author

@mrubens mrubens Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2615

searchContent: match[6],
replaceContent: match[7],
}))
Expand All @@ -376,7 +376,6 @@ Only use a single line of '=======' between search and replacement content, beca
for (const replacement of replacements) {
let { searchContent, replaceContent } = replacement
let startLine = replacement.startLine + (replacement.startLine === 0 ? 0 : delta)
let endLine = replacement.endLine + delta

// First unescape any escaped markers in the content
searchContent = this.unescapeMarkers(searchContent)
Expand Down Expand Up @@ -409,23 +408,16 @@ Only use a single line of '=======' between search and replacement content, beca
let searchLines = searchContent === "" ? [] : searchContent.split(/\r?\n/)
let replaceLines = replaceContent === "" ? [] : replaceContent.split(/\r?\n/)

// Validate that empty search requires start line
if (searchLines.length === 0 && !startLine) {
// Validate that search content is not empty
if (searchLines.length === 0) {
diffResults.push({
success: false,
error: `Empty search content requires start_line to be specified\n\nDebug Info:\n- Empty search content is only valid for insertions at a specific line\n- For insertions, specify the line number where content should be inserted`,
error: `Empty search content is not allowed\n\nDebug Info:\n- Search content cannot be empty\n- For insertions, provide a specific line using :start_line: and include content to search for\n- For example, match a single line to insert before/after it`,
})
continue
}

// Validate that empty search requires same start and end line
if (searchLines.length === 0 && startLine && endLine && startLine !== endLine) {
diffResults.push({
success: false,
error: `Empty search content requires start_line and end_line to be the same (got ${startLine}-${endLine})\n\nDebug Info:\n- Empty search content is only valid for insertions at a specific line\n- For insertions, use the same line number for both start_line and end_line`,
})
continue
}
let endLine = replacement.startLine + searchLines.length - 1

// Initialize search variables
let matchIndex = -1
Expand Down