diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index ee3fa148b4..4726a3f946 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -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" @@ -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(/.*?([^<]+)<\/path>/s) - if (match) { - const firstPath = match[1] - // Check if there are multiple files - const fileCount = (block.params.args.match(//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}'` : "" diff --git a/src/core/diff/strategies/multi-file-search-replace.ts b/src/core/diff/strategies/multi-file-search-replace.ts index d875d723a1..373597c2c2 100644 --- a/src/core/diff/strategies/multi-file-search-replace.ts +++ b/src/core/diff/strategies/multi-file-search-replace.ts @@ -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. diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index b90ef4072d..a8af4438e7 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -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. diff --git a/src/core/tools/__tests__/getApplyDiffDescription.spec.ts b/src/core/tools/__tests__/getApplyDiffDescription.spec.ts new file mode 100644 index 0000000000..809ea9f21d --- /dev/null +++ b/src/core/tools/__tests__/getApplyDiffDescription.spec.ts @@ -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: `test.js<<<<<<< SEARCH +function old() { + return 1; +} +======= +function new() { + return 2; +} +>>>>>>> REPLACE`, + } + + 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: `test1.js<<<<<<< SEARCH +old1 +======= +new1 +>>>>>>> REPLACEtest2.js<<<<<<< SEARCH +old2 +======= +new2 +>>>>>>> REPLACE`, + } + + 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: `test.js<<<<<<< SEARCH +old1 +======= +new1 +>>>>>>> REPLACE + +<<<<<<< SEARCH +old2 +======= +new2 +>>>>>>> REPLACE`, + } + + 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: `test.js<<<<<<< SEARCH +old1 +======= +new1 +>>>>>>> REPLACE<<<<<<< SEARCH +old2 +======= +new2 +>>>>>>> REPLACE`, + } + + 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: `xml`, + } + + 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'.") + }) + }) +}) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index ad4bb0590f..843515ed32 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -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, diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index b41d409dbb..6665e08cd5 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -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<{