Skip to content

Commit 4fcdea1

Browse files
roomoteroomote
committed
feat: add efficiency warnings for single SEARCH/REPLACE blocks in apply_diff
- Add warning message to tool descriptions when only one SEARCH/REPLACE block is used - Update getToolDescription in multi-search-replace.ts and multi-file-search-replace.ts - Create getApplyDiffDescription functions for dynamic warnings in tool usage - Add comprehensive tests for the new warning functionality - Similar to read_file tool efficiency warnings for better LLM context usage Fixes #6054
1 parent df6c57d commit 4fcdea1

File tree

6 files changed

+255
-18
lines changed

6 files changed

+255
-18
lines changed

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { fetchInstructionsTool } from "../tools/fetchInstructionsTool"
1111
import { listFilesTool } from "../tools/listFilesTool"
1212
import { getReadFileToolDescription, readFileTool } from "../tools/readFileTool"
1313
import { writeToFileTool } from "../tools/writeToFileTool"
14+
import { getApplyDiffDescription } from "../tools/multiApplyDiffTool"
1415
import { applyDiffTool } from "../tools/multiApplyDiffTool"
1516
import { insertContentTool } from "../tools/insertContentTool"
1617
import { searchAndReplaceTool } from "../tools/searchAndReplaceTool"
@@ -162,24 +163,7 @@ export async function presentAssistantMessage(cline: Task) {
162163
case "write_to_file":
163164
return `[${block.name} for '${block.params.path}']`
164165
case "apply_diff":
165-
// Handle both legacy format and new multi-file format
166-
if (block.params.path) {
167-
return `[${block.name} for '${block.params.path}']`
168-
} else if (block.params.args) {
169-
// Try to extract first file path from args for display
170-
const match = block.params.args.match(/<file>.*?<path>([^<]+)<\/path>/s)
171-
if (match) {
172-
const firstPath = match[1]
173-
// Check if there are multiple files
174-
const fileCount = (block.params.args.match(/<file>/g) || []).length
175-
if (fileCount > 1) {
176-
return `[${block.name} for '${firstPath}' and ${fileCount - 1} more file${fileCount > 2 ? "s" : ""}]`
177-
} else {
178-
return `[${block.name} for '${firstPath}']`
179-
}
180-
}
181-
}
182-
return `[${block.name}]`
166+
return getApplyDiffDescription(block.name, block.params)
183167
case "search_files":
184168
return `[${block.name} for '${block.params.regex}'${
185169
block.params.file_pattern ? ` in '${block.params.file_pattern}'` : ""

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ Description: Request to apply targeted modifications to one or more files by sea
9999
100100
You can perform multiple distinct search and replace operations within a single \`apply_diff\` call by providing multiple SEARCH/REPLACE blocks in the \`diff\` parameter. This is the preferred way to make several targeted changes efficiently.
101101
102+
**IMPORTANT: Using multiple SEARCH/REPLACE blocks in a single request is more efficient for the LLM. If you have multiple changes to make, include them all in one apply_diff call rather than making separate calls.**
103+
102104
The SEARCH section must exactly match existing content including whitespace and indentation.
103105
If you're not confident in the exact content to search for, use the read_file tool first to get the exact content.
104106
When applying the diffs, be extra careful to remember to change any closing brackets or other syntax that may be affected by the diff farther down in the file.

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ If you're not confident in the exact content to search for, use the read_file to
9999
When applying the diffs, be extra careful to remember to change any closing brackets or other syntax that may be affected by the diff farther down in the file.
100100
ALWAYS make as many changes in a single 'apply_diff' request as possible using multiple SEARCH/REPLACE blocks
101101
102+
**IMPORTANT: Using multiple SEARCH/REPLACE blocks in a single request is more efficient for the LLM. If you have multiple changes to make, include them all in one apply_diff call rather than making separate calls.**
103+
102104
Parameters:
103105
- path: (required) The path of the file to modify (relative to the current workspace directory ${args.cwd})
104106
- diff: (required) The search/replace block defining the changes.

‎src/core/tools/__tests__/getApplyDiffDescription.spec.ts‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
import { describe, it, expect } from "vitest"
2+
import { getApplyDiffDescription as getApplyDiffDescriptionLegacy } from "../applyDiffTool"
3+
import { getApplyDiffDescription as getApplyDiffDescriptionMulti } from "../multiApplyDiffTool"
4+
5+
describe("getApplyDiffDescription", () => {
6+
describe("legacy format (applyDiffTool)", () => {
7+
it("should show efficiency warning when only one SEARCH/REPLACE block is used", () => {
8+
const blockParams = {
9+
path: "test.js",
10+
diff: `<<<<<<< SEARCH
11+
function old() {
12+
return 1;
13+
}
14+
=======
15+
function new() {
16+
return 2;
17+
}
18+
>>>>>>> REPLACE`,
19+
}
20+
21+
const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
22+
expect(result).toContain("Using multiple SEARCH/REPLACE blocks in a single request is more efficient")
23+
expect(result).toContain("[apply_diff for 'test.js'.")
24+
})
25+
26+
it("should not show warning when multiple SEARCH/REPLACE blocks are used", () => {
27+
const blockParams = {
28+
path: "test.js",
29+
diff: `<<<<<<< SEARCH
30+
function old1() {
31+
return 1;
32+
}
33+
=======
34+
function new1() {
35+
return 2;
36+
}
37+
>>>>>>> REPLACE
38+
39+
<<<<<<< SEARCH
40+
function old2() {
41+
return 3;
42+
}
43+
=======
44+
function new2() {
45+
return 4;
46+
}
47+
>>>>>>> REPLACE`,
48+
}
49+
50+
const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
51+
expect(result).toBe("[apply_diff for 'test.js']")
52+
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
53+
})
54+
55+
it("should handle missing path gracefully", () => {
56+
const blockParams = {
57+
diff: `<<<<<<< SEARCH
58+
old content
59+
=======
60+
new content
61+
>>>>>>> REPLACE`,
62+
}
63+
64+
const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
65+
expect(result).toContain("[apply_diff for 'file'.")
66+
expect(result).toContain("Using multiple SEARCH/REPLACE blocks")
67+
})
68+
69+
it("should handle missing diff content", () => {
70+
const blockParams = {
71+
path: "test.js",
72+
}
73+
74+
const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
75+
expect(result).toBe("[apply_diff for 'test.js']")
76+
})
77+
})
78+
79+
describe("multi-file format (multiApplyDiffTool)", () => {
80+
it("should show efficiency warning for single file with single SEARCH/REPLACE block", () => {
81+
const blockParams = {
82+
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
83+
function old() {
84+
return 1;
85+
}
86+
=======
87+
function new() {
88+
return 2;
89+
}
90+
>>>>>>> REPLACE</content></diff></file>`,
91+
}
92+
93+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
94+
expect(result).toContain("Using multiple SEARCH/REPLACE blocks in a single request is more efficient")
95+
expect(result).toContain("[apply_diff for 'test.js'.")
96+
})
97+
98+
it("should not show warning for multiple files", () => {
99+
const blockParams = {
100+
args: `<file><path>test1.js</path><diff><content><<<<<<< SEARCH
101+
old1
102+
=======
103+
new1
104+
>>>>>>> REPLACE</content></diff></file><file><path>test2.js</path><diff><content><<<<<<< SEARCH
105+
old2
106+
=======
107+
new2
108+
>>>>>>> REPLACE</content></diff></file>`,
109+
}
110+
111+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
112+
expect(result).toBe("[apply_diff for 2 files with 2 changes]")
113+
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
114+
})
115+
116+
it("should not show warning for single file with multiple SEARCH/REPLACE blocks", () => {
117+
const blockParams = {
118+
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
119+
old1
120+
=======
121+
new1
122+
>>>>>>> REPLACE
123+
124+
<<<<<<< SEARCH
125+
old2
126+
=======
127+
new2
128+
>>>>>>> REPLACE</content></diff></file>`,
129+
}
130+
131+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
132+
expect(result).toBe("[apply_diff for 1 file with 2 changes]")
133+
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
134+
})
135+
136+
it("should handle multiple diffs per file", () => {
137+
const blockParams = {
138+
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
139+
old1
140+
=======
141+
new1
142+
>>>>>>> REPLACE</content></diff><diff><content><<<<<<< SEARCH
143+
old2
144+
=======
145+
new2
146+
>>>>>>> REPLACE</content></diff></file>`,
147+
}
148+
149+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
150+
expect(result).toBe("[apply_diff for 1 file with 2 changes]")
151+
})
152+
153+
it("should handle invalid XML gracefully", () => {
154+
const blockParams = {
155+
args: `<invalid>xml</content>`,
156+
}
157+
158+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
159+
expect(result).toBe("[apply_diff with unparsable args]")
160+
})
161+
162+
it("should fall back to legacy format when args is not present", () => {
163+
const blockParams = {
164+
path: "test.js",
165+
diff: `<<<<<<< SEARCH
166+
old
167+
=======
168+
new
169+
>>>>>>> REPLACE`,
170+
}
171+
172+
const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
173+
expect(result).toContain("Using multiple SEARCH/REPLACE blocks")
174+
expect(result).toContain("[apply_diff for 'test.js'.")
175+
})
176+
})
177+
})

‎src/core/tools/applyDiffTool.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,22 @@ import { fileExistsAtPath } from "../../utils/fs"
1313
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1414
import { unescapeHtmlEntities } from "../../utils/text-normalization"
1515

16+
export function getApplyDiffDescription(blockName: string, blockParams: any): string {
17+
// Check if diff content has only one SEARCH/REPLACE block
18+
if (blockParams.diff) {
19+
const searchCount = (blockParams.diff.match(/<<<<<<< SEARCH/g) || []).length
20+
if (searchCount === 1) {
21+
return `[${blockName} for '${blockParams.path || "file"}'. Using multiple SEARCH/REPLACE blocks in a single request is more efficient for the LLM. If you have multiple changes to make, include them all in one apply_diff call.]`
22+
}
23+
}
24+
25+
// Default description
26+
if (blockParams.path) {
27+
return `[${blockName} for '${blockParams.path}']`
28+
}
29+
return `[${blockName}]`
30+
}
31+
1632
export async function applyDiffToolLegacy(
1733
cline: Task,
1834
block: ToolUse,

‎src/core/tools/multiApplyDiffTool.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,62 @@ import { parseXml } from "../../utils/xml"
1616
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1717
import { applyDiffToolLegacy } from "./applyDiffTool"
1818

19+
export function getApplyDiffDescription(blockName: string, blockParams: any): string {
20+
// Handle multi-file format
21+
if (blockParams.args) {
22+
try {
23+
const parsed = parseXml(blockParams.args, ["file.diff.content"]) as any
24+
const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean)
25+
26+
// Count total SEARCH/REPLACE blocks across all files
27+
let totalSearchCount = 0
28+
let fileCount = 0
29+
30+
for (const file of files) {
31+
if (!file.path || !file.diff) continue
32+
fileCount++
33+
34+
const diffs = Array.isArray(file.diff) ? file.diff : [file.diff]
35+
for (const diff of diffs) {
36+
if (diff.content) {
37+
const searchCount = (diff.content.match(/<<<<<<< SEARCH/g) || []).length
38+
totalSearchCount += searchCount
39+
}
40+
}
41+
}
42+
43+
if (fileCount === 1 && totalSearchCount === 1) {
44+
const firstPath = files[0]?.path || "file"
45+
return `[${blockName} for '${firstPath}'. Using multiple SEARCH/REPLACE blocks in a single request is more efficient for the LLM. If you have multiple changes to make, include them all in one apply_diff call.]`
46+
} else if (fileCount > 0) {
47+
return `[${blockName} for ${fileCount} file${fileCount > 1 ? "s" : ""} with ${totalSearchCount} change${totalSearchCount > 1 ? "s" : ""}]`
48+
}
49+
50+
// If we get here but no files were found, it's likely invalid XML
51+
if (fileCount === 0) {
52+
return `[${blockName} with unparsable args]`
53+
}
54+
} catch (error) {
55+
console.error("Failed to parse apply_diff args XML for description:", error)
56+
return `[${blockName} with unparsable args]`
57+
}
58+
}
59+
60+
// Handle legacy format
61+
if (blockParams.diff) {
62+
const searchCount = (blockParams.diff.match(/<<<<<<< SEARCH/g) || []).length
63+
if (searchCount === 1) {
64+
return `[${blockName} for '${blockParams.path || "file"}'. Using multiple SEARCH/REPLACE blocks in a single request is more efficient for the LLM. If you have multiple changes to make, include them all in one apply_diff call.]`
65+
}
66+
}
67+
68+
// Default description
69+
if (blockParams.path) {
70+
return `[${blockName} for '${blockParams.path}']`
71+
}
72+
return `[${blockName}]`
73+
}
74+
1975
interface DiffOperation {
2076
path: string
2177
diff: Array<{

0 commit comments

Comments
 (0)