Skip to content

Commit b196489

Browse files
authored
Fix overdependence on start/end lines in diff strategy (#2567)
* Add test for issue #2556 * Fix overdependence on start/end lines in diff strategy
1 parent 45fb958 commit b196489

File tree

2 files changed

+99
-135
lines changed

2 files changed

+99
-135
lines changed

src/core/diff/strategies/__tests__/multi-search-replace.test.ts

Lines changed: 93 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,7 +1541,7 @@ function five() {
15411541
})
15421542
})
15431543

1544-
describe("insertion/deletion", () => {
1544+
describe("deletion", () => {
15451545
let strategy: MultiSearchReplaceDiffStrategy
15461546

15471547
beforeEach(() => {
@@ -1646,126 +1646,6 @@ function five() {
16461646
}
16471647
})
16481648
})
1649-
1650-
describe("insertion", () => {
1651-
it("should insert code at specified line when search block is empty", async () => {
1652-
const originalContent = `function test() {
1653-
const x = 1;
1654-
return x;
1655-
}`
1656-
const diffContent = `test.ts
1657-
<<<<<<< SEARCH
1658-
:start_line:2
1659-
:end_line:2
1660-
-------
1661-
=======
1662-
console.log("Adding log");
1663-
>>>>>>> REPLACE`
1664-
1665-
const result = await strategy.applyDiff(originalContent, diffContent, 2, 2)
1666-
expect(result.success).toBe(true)
1667-
if (result.success) {
1668-
expect(result.content).toBe(`function test() {
1669-
console.log("Adding log");
1670-
const x = 1;
1671-
return x;
1672-
}`)
1673-
}
1674-
})
1675-
1676-
it("should preserve indentation when inserting at nested location", async () => {
1677-
const originalContent = `function test() {
1678-
if (true) {
1679-
const x = 1;
1680-
}
1681-
}`
1682-
const diffContent = `test.ts
1683-
<<<<<<< SEARCH
1684-
:start_line:3
1685-
:end_line:3
1686-
-------
1687-
=======
1688-
console.log("Before");
1689-
console.log("After");
1690-
>>>>>>> REPLACE`
1691-
1692-
const result = await strategy.applyDiff(originalContent, diffContent, 3, 3)
1693-
expect(result.success).toBe(true)
1694-
if (result.success) {
1695-
expect(result.content).toBe(`function test() {
1696-
if (true) {
1697-
console.log("Before");
1698-
console.log("After");
1699-
const x = 1;
1700-
}
1701-
}`)
1702-
}
1703-
})
1704-
1705-
it("should handle insertion at start of file", async () => {
1706-
const originalContent = `function test() {
1707-
return true;
1708-
}`
1709-
const diffContent = `test.ts
1710-
<<<<<<< SEARCH
1711-
:start_line:1
1712-
:end_line:1
1713-
-------
1714-
=======
1715-
// Copyright 2024
1716-
// License: MIT
1717-
1718-
>>>>>>> REPLACE`
1719-
1720-
const result = await strategy.applyDiff(originalContent, diffContent, 1, 1)
1721-
expect(result.success).toBe(true)
1722-
if (result.success) {
1723-
expect(result.content).toBe(`// Copyright 2024
1724-
// License: MIT
1725-
1726-
function test() {
1727-
return true;
1728-
}`)
1729-
}
1730-
})
1731-
1732-
it("should handle insertion at end of file", async () => {
1733-
const originalContent = `function test() {
1734-
return true;
1735-
}`
1736-
const diffContent = `test.ts
1737-
<<<<<<< SEARCH
1738-
:start_line:4
1739-
:end_line:4
1740-
-------
1741-
=======
1742-
// End of file
1743-
>>>>>>> REPLACE`
1744-
1745-
const result = await strategy.applyDiff(originalContent, diffContent, 4, 4)
1746-
expect(result.success).toBe(true)
1747-
if (result.success) {
1748-
expect(result.content).toBe(`function test() {
1749-
return true;
1750-
}
1751-
// End of file`)
1752-
}
1753-
})
1754-
1755-
it("should error if no start_line is provided for insertion", async () => {
1756-
const originalContent = `function test() {
1757-
return true;
1758-
}`
1759-
const diffContent = `test.ts
1760-
<<<<<<< SEARCH
1761-
=======
1762-
console.log("test");
1763-
>>>>>>> REPLACE`
1764-
1765-
const result = await strategy.applyDiff(originalContent, diffContent)
1766-
expect(result.success).toBe(false)
1767-
})
1768-
})
17691649
})
17701650

17711651
describe("fuzzy matching", () => {
@@ -1949,6 +1829,98 @@ function three() {
19491829
}
19501830
})
19511831

1832+
it("should work correctly on this example with line numbers that are slightly off", async () => {
1833+
const originalContent = `.game-container {
1834+
display: flex;
1835+
flex-direction: column;
1836+
gap: 1rem;
1837+
}
1838+
1839+
.chess-board-container {
1840+
display: flex;
1841+
gap: 1rem;
1842+
align-items: center;
1843+
}
1844+
1845+
.overlay {
1846+
position: absolute;
1847+
top: 0;
1848+
left: 0;
1849+
width: 100%;
1850+
height: 100%;
1851+
background-color: rgba(0, 0, 0, 0.5);
1852+
z-index: 999; /* Ensure it's above the board but below the promotion dialog */
1853+
}
1854+
1855+
.game-container.promotion-active .chess-board,
1856+
.game-container.promotion-active .game-toolbar,
1857+
.game-container.promotion-active .game-info-container {
1858+
filter: blur(2px);
1859+
pointer-events: none; /* Disable clicks on these elements */
1860+
}
1861+
1862+
.game-container.promotion-active .promotion-dialog {
1863+
z-index: 1000; /* Ensure it's above the overlay */
1864+
pointer-events: auto; /* Enable clicks on the promotion dialog */
1865+
}`
1866+
const diffContent = `test.ts
1867+
<<<<<<< SEARCH
1868+
:start_line:12
1869+
:end_line:13
1870+
-------
1871+
.overlay {
1872+
=======
1873+
.piece {
1874+
will-change: transform;
1875+
}
1876+
1877+
.overlay {
1878+
>>>>>>> REPLACE
1879+
`
1880+
1881+
const result = await strategy.applyDiff(originalContent, diffContent)
1882+
expect(result.success).toBe(true)
1883+
if (result.success) {
1884+
expect(result.content).toBe(`.game-container {
1885+
display: flex;
1886+
flex-direction: column;
1887+
gap: 1rem;
1888+
}
1889+
1890+
.chess-board-container {
1891+
display: flex;
1892+
gap: 1rem;
1893+
align-items: center;
1894+
}
1895+
1896+
.piece {
1897+
will-change: transform;
1898+
}
1899+
1900+
.overlay {
1901+
position: absolute;
1902+
top: 0;
1903+
left: 0;
1904+
width: 100%;
1905+
height: 100%;
1906+
background-color: rgba(0, 0, 0, 0.5);
1907+
z-index: 999; /* Ensure it's above the board but below the promotion dialog */
1908+
}
1909+
1910+
.game-container.promotion-active .chess-board,
1911+
.game-container.promotion-active .game-toolbar,
1912+
.game-container.promotion-active .game-info-container {
1913+
filter: blur(2px);
1914+
pointer-events: none; /* Disable clicks on these elements */
1915+
}
1916+
1917+
.game-container.promotion-active .promotion-dialog {
1918+
z-index: 1000; /* Ensure it's above the overlay */
1919+
pointer-events: auto; /* Enable clicks on the promotion dialog */
1920+
}`)
1921+
}
1922+
})
1923+
19521924
it("should not find matches outside search range and buffer zone", async () => {
19531925
const originalContent = `
19541926
function one() {

src/core/diff/strategies/multi-search-replace.ts

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@ import { ToolUse } from "../../assistant-message"
77
const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches
88

99
function getSimilarity(original: string, search: string): number {
10+
// Empty searches are no longer supported
1011
if (search === "") {
11-
return 1
12+
return 0
1213
}
1314

1415
// Normalize strings by removing extra whitespace but preserve case
@@ -367,7 +368,6 @@ Only use a single line of '=======' between search and replacement content, beca
367368
const replacements = matches
368369
.map((match) => ({
369370
startLine: Number(match[2] ?? 0),
370-
endLine: Number(match[4] ?? resultLines.length),
371371
searchContent: match[6],
372372
replaceContent: match[7],
373373
}))
@@ -376,7 +376,6 @@ Only use a single line of '=======' between search and replacement content, beca
376376
for (const replacement of replacements) {
377377
let { searchContent, replaceContent } = replacement
378378
let startLine = replacement.startLine + (replacement.startLine === 0 ? 0 : delta)
379-
let endLine = replacement.endLine + delta
380379

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

412-
// Validate that empty search requires start line
413-
if (searchLines.length === 0 && !startLine) {
411+
// Validate that search content is not empty
412+
if (searchLines.length === 0) {
414413
diffResults.push({
415414
success: false,
416-
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`,
415+
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`,
417416
})
418417
continue
419418
}
420419

421-
// Validate that empty search requires same start and end line
422-
if (searchLines.length === 0 && startLine && endLine && startLine !== endLine) {
423-
diffResults.push({
424-
success: false,
425-
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`,
426-
})
427-
continue
428-
}
420+
let endLine = replacement.startLine + searchLines.length - 1
429421

430422
// Initialize search variables
431423
let matchIndex = -1

0 commit comments

Comments
 (0)