Skip to content

feat: add efficiency warnings for single SEARCH/REPLACE blocks in apply_diff #6056

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 2 additions & 18 deletions src/core/assistant-message/presentAssistantMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { fetchInstructionsTool } from "../tools/fetchInstructionsTool"
import { listFilesTool } from "../tools/listFilesTool"
import { getReadFileToolDescription, readFileTool } from "../tools/readFileTool"
import { writeToFileTool } from "../tools/writeToFileTool"
import { getApplyDiffDescription } from "../tools/multiApplyDiffTool"
import { applyDiffTool } from "../tools/multiApplyDiffTool"
import { insertContentTool } from "../tools/insertContentTool"
import { searchAndReplaceTool } from "../tools/searchAndReplaceTool"
Expand Down Expand Up @@ -162,24 +163,7 @@ export async function presentAssistantMessage(cline: Task) {
case "write_to_file":
return `[${block.name} for '${block.params.path}']`
case "apply_diff":
// Handle both legacy format and new multi-file format
if (block.params.path) {
return `[${block.name} for '${block.params.path}']`
} else if (block.params.args) {
// Try to extract first file path from args for display
const match = block.params.args.match(/<file>.*?<path>([^<]+)<\/path>/s)
if (match) {
const firstPath = match[1]
// Check if there are multiple files
const fileCount = (block.params.args.match(/<file>/g) || []).length
if (fileCount > 1) {
return `[${block.name} for '${firstPath}' and ${fileCount - 1} more file${fileCount > 2 ? "s" : ""}]`
} else {
return `[${block.name} for '${firstPath}']`
}
}
}
return `[${block.name}]`
return getApplyDiffDescription(block.name, block.params)
case "search_files":
return `[${block.name} for '${block.params.regex}'${
block.params.file_pattern ? ` in '${block.params.file_pattern}'` : ""
Expand Down
2 changes: 2 additions & 0 deletions src/core/diff/strategies/multi-file-search-replace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ Description: Request to apply targeted modifications to one or more files by sea

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.

**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.**

The SEARCH section must exactly match existing content including whitespace and indentation.
If you're not confident in the exact content to search for, use the read_file tool first to get the exact content.
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.
Expand Down
2 changes: 2 additions & 0 deletions src/core/diff/strategies/multi-search-replace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ If you're not confident in the exact content to search for, use the read_file to
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.
ALWAYS make as many changes in a single 'apply_diff' request as possible using multiple SEARCH/REPLACE blocks

**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.**

Parameters:
- path: (required) The path of the file to modify (relative to the current workspace directory ${args.cwd})
- diff: (required) The search/replace block defining the changes.
Expand Down
177 changes: 177 additions & 0 deletions src/core/tools/__tests__/getApplyDiffDescription.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
import { describe, it, expect } from "vitest"
import { getApplyDiffDescription as getApplyDiffDescriptionLegacy } from "../applyDiffTool"
import { getApplyDiffDescription as getApplyDiffDescriptionMulti } from "../multiApplyDiffTool"

describe("getApplyDiffDescription", () => {
describe("legacy format (applyDiffTool)", () => {
it("should show efficiency warning when only one SEARCH/REPLACE block is used", () => {
const blockParams = {
path: "test.js",
diff: `<<<<<<< SEARCH
function old() {
return 1;
}
=======
function new() {
return 2;
}
>>>>>>> REPLACE`,
}

const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
expect(result).toContain("Using multiple SEARCH/REPLACE blocks in a single request is more efficient")
expect(result).toContain("[apply_diff for 'test.js'.")
})

it("should not show warning when multiple SEARCH/REPLACE blocks are used", () => {
const blockParams = {
path: "test.js",
diff: `<<<<<<< SEARCH
function old1() {
return 1;
}
=======
function new1() {
return 2;
}
>>>>>>> REPLACE

<<<<<<< SEARCH
function old2() {
return 3;
}
=======
function new2() {
return 4;
}
>>>>>>> REPLACE`,
}

const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
expect(result).toBe("[apply_diff for 'test.js']")
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
})

it("should handle missing path gracefully", () => {
const blockParams = {
diff: `<<<<<<< SEARCH
old content
=======
new content
>>>>>>> REPLACE`,
}

const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
expect(result).toContain("[apply_diff for 'file'.")
expect(result).toContain("Using multiple SEARCH/REPLACE blocks")
})

it("should handle missing diff content", () => {
const blockParams = {
path: "test.js",
}

const result = getApplyDiffDescriptionLegacy("apply_diff", blockParams)
expect(result).toBe("[apply_diff for 'test.js']")
})
})

describe("multi-file format (multiApplyDiffTool)", () => {
it("should show efficiency warning for single file with single SEARCH/REPLACE block", () => {
const blockParams = {
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
function old() {
return 1;
}
=======
function new() {
return 2;
}
>>>>>>> REPLACE</content></diff></file>`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toContain("Using multiple SEARCH/REPLACE blocks in a single request is more efficient")
expect(result).toContain("[apply_diff for 'test.js'.")
})

it("should not show warning for multiple files", () => {
const blockParams = {
args: `<file><path>test1.js</path><diff><content><<<<<<< SEARCH
old1
=======
new1
>>>>>>> REPLACE</content></diff></file><file><path>test2.js</path><diff><content><<<<<<< SEARCH
old2
=======
new2
>>>>>>> REPLACE</content></diff></file>`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toBe("[apply_diff for 2 files with 2 changes]")
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
})

it("should not show warning for single file with multiple SEARCH/REPLACE blocks", () => {
const blockParams = {
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
old1
=======
new1
>>>>>>> REPLACE

<<<<<<< SEARCH
old2
=======
new2
>>>>>>> REPLACE</content></diff></file>`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toBe("[apply_diff for 1 file with 2 changes]")
expect(result).not.toContain("Using multiple SEARCH/REPLACE blocks")
})

it("should handle multiple diffs per file", () => {
const blockParams = {
args: `<file><path>test.js</path><diff><content><<<<<<< SEARCH
old1
=======
new1
>>>>>>> REPLACE</content></diff><diff><content><<<<<<< SEARCH
old2
=======
new2
>>>>>>> REPLACE</content></diff></file>`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toBe("[apply_diff for 1 file with 2 changes]")
})

it("should handle invalid XML gracefully", () => {
const blockParams = {
args: `<invalid>xml</content>`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toBe("[apply_diff with unparsable args]")
})

it("should fall back to legacy format when args is not present", () => {
const blockParams = {
path: "test.js",
diff: `<<<<<<< SEARCH
old
=======
new
>>>>>>> REPLACE`,
}

const result = getApplyDiffDescriptionMulti("apply_diff", blockParams)
expect(result).toContain("Using multiple SEARCH/REPLACE blocks")
expect(result).toContain("[apply_diff for 'test.js'.")
})
})
})
16 changes: 16 additions & 0 deletions src/core/tools/applyDiffTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,22 @@ import { fileExistsAtPath } from "../../utils/fs"
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
import { unescapeHtmlEntities } from "../../utils/text-normalization"

export function getApplyDiffDescription(blockName: string, blockParams: any): string {
// Check if diff content has only one SEARCH/REPLACE block
if (blockParams.diff) {
const searchCount = (blockParams.diff.match(/<<<<<<< SEARCH/g) || []).length
if (searchCount === 1) {
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.]`
}
}

// Default description
if (blockParams.path) {
return `[${blockName} for '${blockParams.path}']`
}
return `[${blockName}]`
}

export async function applyDiffToolLegacy(
cline: Task,
block: ToolUse,
Expand Down
56 changes: 56 additions & 0 deletions src/core/tools/multiApplyDiffTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,62 @@ import { parseXml } from "../../utils/xml"
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
import { applyDiffToolLegacy } from "./applyDiffTool"

export function getApplyDiffDescription(blockName: string, blockParams: any): string {
// Handle multi-file format
if (blockParams.args) {
try {
const parsed = parseXml(blockParams.args, ["file.diff.content"]) as any
const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean)

// Count total SEARCH/REPLACE blocks across all files
let totalSearchCount = 0
let fileCount = 0

for (const file of files) {
if (!file.path || !file.diff) continue
fileCount++

const diffs = Array.isArray(file.diff) ? file.diff : [file.diff]
for (const diff of diffs) {
if (diff.content) {
const searchCount = (diff.content.match(/<<<<<<< SEARCH/g) || []).length
totalSearchCount += searchCount
}
}
}

if (fileCount === 1 && totalSearchCount === 1) {
const firstPath = files[0]?.path || "file"
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.]`
} else if (fileCount > 0) {
return `[${blockName} for ${fileCount} file${fileCount > 1 ? "s" : ""} with ${totalSearchCount} change${totalSearchCount > 1 ? "s" : ""}]`
}

// If we get here but no files were found, it's likely invalid XML
if (fileCount === 0) {
return `[${blockName} with unparsable args]`
}
} catch (error) {
console.error("Failed to parse apply_diff args XML for description:", error)
return `[${blockName} with unparsable args]`
}
}

// Handle legacy format
if (blockParams.diff) {
const searchCount = (blockParams.diff.match(/<<<<<<< SEARCH/g) || []).length
if (searchCount === 1) {
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.]`
}
}

// Default description
if (blockParams.path) {
return `[${blockName} for '${blockParams.path}']`
}
return `[${blockName}]`
}

interface DiffOperation {
path: string
diff: Array<{
Expand Down
Loading