mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-08 22:21:23 +00:00
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
This commit is contained in:
parent
df6c57d293
commit
4fcdea18f6
6 changed files with 255 additions and 18 deletions
|
|
@ -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(/<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}'` : ""
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
177
src/core/tools/__tests__/getApplyDiffDescription.spec.ts
Normal file
177
src/core/tools/__tests__/getApplyDiffDescription.spec.ts
Normal file
|
|
@ -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'.")
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<{
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue