From 4fcdea18f6377d126b014cf1d9b8031f781b4714 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 22 Jul 2025 07:51:55 +0000 Subject: [PATCH] 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 --- .../presentAssistantMessage.ts | 20 +- .../strategies/multi-file-search-replace.ts | 2 + .../diff/strategies/multi-search-replace.ts | 2 + .../__tests__/getApplyDiffDescription.spec.ts | 177 ++++++++++++++++++ src/core/tools/applyDiffTool.ts | 16 ++ src/core/tools/multiApplyDiffTool.ts | 56 ++++++ 6 files changed, 255 insertions(+), 18 deletions(-) create mode 100644 src/core/tools/__tests__/getApplyDiffDescription.spec.ts diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index ee3fa148b41..4726a3f946e 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 d875d723a15..373597c2c29 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 b90ef4072d5..a8af4438e73 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 00000000000..809ea9f21dc --- /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 ad4bb0590f8..843515ed329 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 b41d409dbb7..6665e08cd5e 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<{