From c757dac0ec77ec4daeb58cdfcffc452cafac8479 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Sat, 7 Feb 2026 16:03:47 -0700 Subject: [PATCH 1/5] feat: rename search_and_replace tool to edit with new parameter structure - Add new 'edit' tool with file_path, old_string, new_string, replace_all params - Keep search_and_replace as backward-compatible alias via TOOL_ALIASES - Create EditTool execution handler with replace_all and uniqueness checking - Update NativeToolCallParser for both edit and search_and_replace names - Update presentAssistantMessage routing for both tool names - Add alias resolution to isToolAllowedForMode for customTools validation - Reduce SearchAndReplaceTool.ts to re-export wrapper from EditTool - Add 16 new EditTool tests, update searchAndReplaceTool tests - MiniMax includedTools: ['search_and_replace'] continues to work via alias --- packages/types/src/tool.ts | 1 + .../assistant-message/NativeToolCallParser.ts | 26 +- .../presentAssistantMessage.ts | 9 +- src/core/prompts/tools/native-tools/edit.ts | 48 ++ src/core/prompts/tools/native-tools/index.ts | 4 +- src/core/task/Task.ts | 2 +- src/core/tools/EditTool.ts | 279 ++++++++++++ src/core/tools/SearchAndReplaceTool.ts | 305 +------------ src/core/tools/__tests__/editTool.spec.ts | 423 ++++++++++++++++++ .../__tests__/searchAndReplaceTool.spec.ts | 417 +---------------- src/core/tools/validateToolUse.ts | 16 +- src/shared/tools.ts | 8 +- 12 files changed, 808 insertions(+), 730 deletions(-) create mode 100644 src/core/prompts/tools/native-tools/edit.ts create mode 100644 src/core/tools/EditTool.ts create mode 100644 src/core/tools/__tests__/editTool.spec.ts diff --git a/packages/types/src/tool.ts b/packages/types/src/tool.ts index 03144055c9a..a8ea826d11d 100644 --- a/packages/types/src/tool.ts +++ b/packages/types/src/tool.ts @@ -20,6 +20,7 @@ export const toolNames = [ "read_command_output", "write_to_file", "apply_diff", + "edit", "search_and_replace", "search_replace", "edit_file", diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index e7b0067dd92..c8b96e35e31 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -599,11 +599,18 @@ export class NativeToolCallParser { } break + case "edit": case "search_and_replace": - if (partialArgs.path !== undefined || partialArgs.operations !== undefined) { + if ( + partialArgs.file_path !== undefined || + partialArgs.old_string !== undefined || + partialArgs.new_string !== undefined + ) { nativeArgs = { - path: partialArgs.path, - operations: partialArgs.operations, + file_path: partialArgs.file_path, + old_string: partialArgs.old_string, + new_string: partialArgs.new_string, + replace_all: this.coerceOptionalBoolean(partialArgs.replace_all), } } break @@ -806,11 +813,18 @@ export class NativeToolCallParser { } break + case "edit": case "search_and_replace": - if (args.path !== undefined && args.operations !== undefined && Array.isArray(args.operations)) { + if ( + args.file_path !== undefined && + args.old_string !== undefined && + args.new_string !== undefined + ) { nativeArgs = { - path: args.path, - operations: args.operations, + file_path: args.file_path, + old_string: args.old_string, + new_string: args.new_string, + replace_all: this.coerceOptionalBoolean(args.replace_all), } as NativeArgsFor } break diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index c183d51ca53..ce3e44cd2a3 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -18,7 +18,7 @@ import { listFilesTool } from "../tools/ListFilesTool" import { readFileTool } from "../tools/ReadFileTool" import { readCommandOutputTool } from "../tools/ReadCommandOutputTool" import { writeToFileTool } from "../tools/WriteToFileTool" -import { searchAndReplaceTool } from "../tools/SearchAndReplaceTool" +import { editTool } from "../tools/EditTool" import { searchReplaceTool } from "../tools/SearchReplaceTool" import { editFileTool } from "../tools/EditFileTool" import { applyPatchTool } from "../tools/ApplyPatchTool" @@ -357,8 +357,9 @@ export async function presentAssistantMessage(cline: Task) { return `[${block.name} for '${block.params.regex}'${ block.params.file_pattern ? ` in '${block.params.file_pattern}'` : "" }]` + case "edit": case "search_and_replace": - return `[${block.name} for '${block.params.path}']` + return `[${block.name} for '${block.params.file_path}']` case "search_replace": return `[${block.name} for '${block.params.file_path}']` case "edit_file": @@ -739,9 +740,10 @@ export async function presentAssistantMessage(cline: Task) { pushToolResult, }) break + case "edit": case "search_and_replace": await checkpointSaveAndMark(cline) - await searchAndReplaceTool.handle(cline, block as ToolUse<"search_and_replace">, { + await editTool.handle(cline, block as ToolUse<"edit">, { askApproval, handleError, pushToolResult, @@ -1073,6 +1075,7 @@ function containsXmlToolMarkup(text: string): boolean { "new_task", "read_command_output", "read_file", + "edit", "search_and_replace", "search_files", "search_replace", diff --git a/src/core/prompts/tools/native-tools/edit.ts b/src/core/prompts/tools/native-tools/edit.ts new file mode 100644 index 00000000000..e2593b84843 --- /dev/null +++ b/src/core/prompts/tools/native-tools/edit.ts @@ -0,0 +1,48 @@ +import type OpenAI from "openai" + +const EDIT_DESCRIPTION = `Performs exact string replacements in files. + +Usage: +- You must use your \`Read\` tool at least once in the conversation before editing. This tool will error if you attempt an edit without reading the file. +- When editing text from Read tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: spaces + line number + tab. Everything after that tab is the actual file content to match. Never include any part of the line number prefix in the old_string or new_string. +- ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required. +- Only use emojis if the user explicitly requests it. Avoid adding emojis to files unless asked. +- The edit will FAIL if \`old_string\` is not unique in the file. Either provide a larger string with more surrounding context to make it unique or use \`replace_all\` to change every instance of \`old_string\`. +- Use \`replace_all\` for replacing and renaming strings across the file. This parameter is useful if you want to rename a variable for instance.` + +const edit = { + type: "function", + function: { + name: "edit", + description: EDIT_DESCRIPTION, + parameters: { + type: "object", + properties: { + file_path: { + type: "string", + description: "The path of the file to edit (relative to the working directory)", + }, + old_string: { + type: "string", + description: + "The exact text to find in the file. Must match exactly, including all whitespace, indentation, and line endings.", + }, + new_string: { + type: "string", + description: + "The replacement text that will replace old_string. Must include all necessary whitespace and indentation.", + }, + replace_all: { + type: "boolean", + description: + "When true, replaces ALL occurrences of old_string in the file. When false (default), only replaces the first occurrence and errors if multiple matches exist.", + default: false, + }, + }, + required: ["file_path", "old_string", "new_string"], + additionalProperties: false, + }, + }, +} satisfies OpenAI.Chat.ChatCompletionTool + +export default edit diff --git a/src/core/prompts/tools/native-tools/index.ts b/src/core/prompts/tools/native-tools/index.ts index 1c0825233e0..48f1071e1be 100644 --- a/src/core/prompts/tools/native-tools/index.ts +++ b/src/core/prompts/tools/native-tools/index.ts @@ -6,6 +6,7 @@ import askFollowupQuestion from "./ask_followup_question" import attemptCompletion from "./attempt_completion" import browserAction from "./browser_action" import codebaseSearch from "./codebase_search" +import editTool from "./edit" import executeCommand from "./execute_command" import generateImage from "./generate_image" import listFiles from "./list_files" @@ -14,7 +15,6 @@ import readCommandOutput from "./read_command_output" import { createReadFileTool, type ReadFileToolOptions } from "./read_file" import runSlashCommand from "./run_slash_command" import skill from "./skill" -import searchAndReplace from "./search_and_replace" import searchReplace from "./search_replace" import edit_file from "./edit_file" import searchFiles from "./search_files" @@ -63,9 +63,9 @@ export function getNativeTools(options: NativeToolsOptions = {}): OpenAI.Chat.Ch createReadFileTool(readFileOptions), runSlashCommand, skill, - searchAndReplace, searchReplace, edit_file, + editTool, searchFiles, switchMode, updateTodoList, diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index ef6e956dff8..70de2b43804 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3537,7 +3537,7 @@ export class Task extends EventEmitter implements TaskLike { const input = toolUse.nativeArgs || toolUse.params // Use originalName (alias) if present for API history consistency. - // When tool aliases are used (e.g., "edit_file" -> "search_and_replace"), + // When tool aliases are used (e.g., "edit_file" -> "search_and_replace" -> "edit" (current canonical name)), // we want the alias name in the conversation history to match what the model // was told the tool was named, preventing confusion in multi-turn conversations. const toolNameForHistory = toolUse.originalName ?? toolUse.name diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts new file mode 100644 index 00000000000..a0d8b0a6994 --- /dev/null +++ b/src/core/tools/EditTool.ts @@ -0,0 +1,279 @@ +import fs from "fs/promises" +import path from "path" + +import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" + +import { getReadablePath } from "../../utils/path" +import { isPathOutsideWorkspace } from "../../utils/pathUtils" +import { Task } from "../task/Task" +import { formatResponse } from "../prompts/responses" +import { RecordSource } from "../context-tracking/FileContextTrackerTypes" +import { fileExistsAtPath } from "../../utils/fs" +import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" +import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import type { ToolUse } from "../../shared/tools" + +import { BaseTool, ToolCallbacks } from "./BaseTool" + +interface EditParams { + file_path: string + old_string: string + new_string: string + replace_all?: boolean +} + +export class EditTool extends BaseTool<"edit"> { + readonly name = "edit" as const + + async execute(params: EditParams, task: Task, callbacks: ToolCallbacks): Promise { + const { file_path: relPath, old_string: oldString, new_string: newString, replace_all: replaceAll } = params + const { askApproval, handleError, pushToolResult } = callbacks + + try { + // Validate required parameters + if (!relPath) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + pushToolResult(await task.sayAndCreateMissingParamError("edit", "file_path")) + return + } + + if (!oldString) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + pushToolResult(await task.sayAndCreateMissingParamError("edit", "old_string")) + return + } + + if (newString === undefined) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + pushToolResult(await task.sayAndCreateMissingParamError("edit", "new_string")) + return + } + + // Check old_string !== new_string + if (oldString === newString) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + pushToolResult( + formatResponse.toolError( + "'old_string' and 'new_string' are identical. No changes needed. If you want to make a change, ensure 'old_string' and 'new_string' are different.", + ), + ) + return + } + + const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) + + if (!accessAllowed) { + await task.say("rooignore_error", relPath) + pushToolResult(formatResponse.rooIgnoreError(relPath)) + return + } + + // Check if file is write-protected + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false + + const absolutePath = path.resolve(task.cwd, relPath) + + const fileExists = await fileExistsAtPath(absolutePath) + if (!fileExists) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + const errorMessage = `File not found: ${relPath}. Cannot perform edit on a non-existent file.` + await task.say("error", errorMessage) + pushToolResult(formatResponse.toolError(errorMessage)) + return + } + + let fileContent: string + try { + fileContent = await fs.readFile(absolutePath, "utf8") + // Normalize line endings to LF for consistent matching + fileContent = fileContent.replace(/\r\n/g, "\n") + } catch (error) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + const errorMessage = `Failed to read file '${relPath}'. Please verify file permissions and try again.` + await task.say("error", errorMessage) + pushToolResult(formatResponse.toolError(errorMessage)) + return + } + + // Normalize line endings in old_string/new_string to match file content + const normalizedOld = oldString.replace(/\r\n/g, "\n") + const normalizedNew = newString.replace(/\r\n/g, "\n") + + // Count occurrences of old_string in file content + const matchCount = fileContent.split(normalizedOld).length - 1 + + if (matchCount === 0) { + task.consecutiveMistakeCount++ + task.recordToolError("edit", "no_match") + pushToolResult( + formatResponse.toolError( + `No match found for 'old_string' in ${relPath}. Make sure the text to find appears exactly in the file, including whitespace and indentation.`, + ), + ) + return + } + + // Uniqueness check when replace_all is not enabled + if (!replaceAll && matchCount > 1) { + task.consecutiveMistakeCount++ + task.recordToolError("edit") + pushToolResult( + formatResponse.toolError( + `Found ${matchCount} matches of 'old_string' in the file. Use 'replace_all: true' to replace all occurrences, or provide more context in 'old_string' to make it unique.`, + ), + ) + return + } + + // Apply the replacement + let newContent: string + if (replaceAll) { + // Replace all occurrences + const searchPattern = new RegExp(escapeRegExp(normalizedOld), "g") + newContent = fileContent.replace(searchPattern, normalizedNew) + } else { + // Replace single occurrence (already verified uniqueness above) + newContent = fileContent.replace(normalizedOld, normalizedNew) + } + + // Check if any changes were made + if (newContent === fileContent) { + pushToolResult(`No changes needed for '${relPath}'`) + return + } + + task.consecutiveMistakeCount = 0 + + // Initialize diff view + task.diffViewProvider.editType = "modify" + task.diffViewProvider.originalContent = fileContent + + // Generate and validate diff + const diff = formatResponse.createPrettyPatch(relPath, fileContent, newContent) + if (!diff) { + pushToolResult(`No changes needed for '${relPath}'`) + await task.diffViewProvider.reset() + return + } + + // Check if preventFocusDisruption experiment is enabled + const provider = task.providerRef.deref() + const state = await provider?.getState() + const diagnosticsEnabled = state?.diagnosticsEnabled ?? true + const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) + + const sanitizedDiff = sanitizeUnifiedDiff(diff) + const diffStats = computeDiffStats(sanitizedDiff) || undefined + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + + const sharedMessageProps: ClineSayTool = { + tool: "appliedDiff", + path: getReadablePath(task.cwd, relPath), + diff: sanitizedDiff, + isOutsideWorkspace, + } + + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + content: sanitizedDiff, + isProtected: isWriteProtected, + diffStats, + } satisfies ClineSayTool) + + // Show diff view if focus disruption prevention is disabled + if (!isPreventFocusDisruptionEnabled) { + await task.diffViewProvider.open(relPath) + await task.diffViewProvider.update(newContent, true) + task.diffViewProvider.scrollToFirstDiff() + } + + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) + + if (!didApprove) { + // Revert changes if diff view was shown + if (!isPreventFocusDisruptionEnabled) { + await task.diffViewProvider.revertChanges() + } + pushToolResult("Changes were rejected by the user.") + await task.diffViewProvider.reset() + return + } + + // Save the changes + if (isPreventFocusDisruptionEnabled) { + // Direct file write without diff view or opening the file + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + } else { + // Call saveChanges to update the DiffViewProvider properties + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + } + + // Track file edit operation + if (relPath) { + await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) + } + + task.didEditFile = true + + // Get the formatted response message + const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) + pushToolResult(message) + + // Record successful tool usage and cleanup + task.recordToolUsage("edit") + await task.diffViewProvider.reset() + this.resetPartialState() + + // Process any queued messages after file edit completes + task.processQueuedMessages() + } catch (error) { + await handleError("edit", error as Error) + await task.diffViewProvider.reset() + this.resetPartialState() + } + } + + override async handlePartial(task: Task, block: ToolUse<"edit">): Promise { + const relPath: string | undefined = block.params.file_path + + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilized(relPath)) { + return + } + + // relPath is guaranteed non-null after hasPathStabilized + const absolutePath = path.resolve(task.cwd, relPath!) + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + + const sharedMessageProps: ClineSayTool = { + tool: "appliedDiff", + path: getReadablePath(task.cwd, relPath!), + diff: block.params.old_string ? "1 edit operation" : undefined, + isOutsideWorkspace, + } + + await task.ask("tool", JSON.stringify(sharedMessageProps), block.partial).catch(() => {}) + } +} + +/** + * Escapes special regex characters in a string + * @param input String to escape regex characters in + * @returns Escaped string safe for regex pattern matching + */ +function escapeRegExp(input: string): string { + return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") +} + +export const editTool = new EditTool() +export const searchAndReplaceTool = editTool // alias for backward compat diff --git a/src/core/tools/SearchAndReplaceTool.ts b/src/core/tools/SearchAndReplaceTool.ts index 93c3b4533b7..1ce22aa079e 100644 --- a/src/core/tools/SearchAndReplaceTool.ts +++ b/src/core/tools/SearchAndReplaceTool.ts @@ -1,303 +1,2 @@ -import fs from "fs/promises" -import path from "path" - -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" - -import { getReadablePath } from "../../utils/path" -import { isPathOutsideWorkspace } from "../../utils/pathUtils" -import { Task } from "../task/Task" -import { formatResponse } from "../prompts/responses" -import { RecordSource } from "../context-tracking/FileContextTrackerTypes" -import { fileExistsAtPath } from "../../utils/fs" -import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" -import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" -import type { ToolUse } from "../../shared/tools" - -import { BaseTool, ToolCallbacks } from "./BaseTool" - -interface SearchReplaceOperation { - search: string - replace: string -} - -interface SearchAndReplaceParams { - path: string - operations: SearchReplaceOperation[] -} - -export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> { - readonly name = "search_and_replace" as const - - async execute(params: SearchAndReplaceParams, task: Task, callbacks: ToolCallbacks): Promise { - const { path: relPath, operations } = params - const { askApproval, handleError, pushToolResult } = callbacks - - try { - // Validate required parameters - if (!relPath) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - pushToolResult(await task.sayAndCreateMissingParamError("search_and_replace", "path")) - return - } - - if (!operations || !Array.isArray(operations) || operations.length === 0) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - pushToolResult( - formatResponse.toolError( - "Missing or empty 'operations' parameter. At least one search/replace operation is required.", - ), - ) - return - } - - // Validate each operation has search and replace fields - for (let i = 0; i < operations.length; i++) { - const op = operations[i] - if (!op.search) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - pushToolResult(formatResponse.toolError(`Operation ${i + 1} is missing the 'search' field.`)) - return - } - if (op.replace === undefined) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - pushToolResult(formatResponse.toolError(`Operation ${i + 1} is missing the 'replace' field.`)) - return - } - } - - const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) - - if (!accessAllowed) { - await task.say("rooignore_error", relPath) - pushToolResult(formatResponse.rooIgnoreError(relPath)) - return - } - - // Check if file is write-protected - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false - - const absolutePath = path.resolve(task.cwd, relPath) - - const fileExists = await fileExistsAtPath(absolutePath) - if (!fileExists) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - const errorMessage = `File not found: ${relPath}. Cannot perform search and replace on a non-existent file.` - await task.say("error", errorMessage) - pushToolResult(formatResponse.toolError(errorMessage)) - return - } - - let fileContent: string - try { - fileContent = await fs.readFile(absolutePath, "utf8") - // Normalize line endings to LF for consistent matching - fileContent = fileContent.replace(/\r\n/g, "\n") - } catch (error) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace") - const errorMessage = `Failed to read file '${relPath}'. Please verify file permissions and try again.` - await task.say("error", errorMessage) - pushToolResult(formatResponse.toolError(errorMessage)) - return - } - - // Apply all operations sequentially - let newContent = fileContent - const errors: string[] = [] - - for (let i = 0; i < operations.length; i++) { - // Normalize line endings in search/replace strings to match file content - const search = operations[i].search.replace(/\r\n/g, "\n") - const replace = operations[i].replace.replace(/\r\n/g, "\n") - const searchPattern = new RegExp(escapeRegExp(search), "g") - - const matchCount = newContent.match(searchPattern)?.length ?? 0 - if (matchCount === 0) { - errors.push(`Operation ${i + 1}: No match found for search text.`) - continue - } - - if (matchCount > 1) { - errors.push( - `Operation ${i + 1}: Found ${matchCount} matches. Please provide more context to make a unique match.`, - ) - continue - } - - // Apply the replacement - newContent = newContent.replace(searchPattern, replace) - } - - // If all operations failed, return error - if (errors.length === operations.length) { - task.consecutiveMistakeCount++ - task.recordToolError("search_and_replace", "no_match") - pushToolResult(formatResponse.toolError(`All operations failed:\n${errors.join("\n")}`)) - return - } - - // Check if any changes were made - if (newContent === fileContent) { - pushToolResult(`No changes needed for '${relPath}'`) - return - } - - task.consecutiveMistakeCount = 0 - - // Initialize diff view - task.diffViewProvider.editType = "modify" - task.diffViewProvider.originalContent = fileContent - - // Generate and validate diff - const diff = formatResponse.createPrettyPatch(relPath, fileContent, newContent) - if (!diff) { - pushToolResult(`No changes needed for '${relPath}'`) - await task.diffViewProvider.reset() - return - } - - // Check if preventFocusDisruption experiment is enabled - const provider = task.providerRef.deref() - const state = await provider?.getState() - const diagnosticsEnabled = state?.diagnosticsEnabled ?? true - const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) - - const sanitizedDiff = sanitizeUnifiedDiff(diff) - const diffStats = computeDiffStats(sanitizedDiff) || undefined - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) - - const sharedMessageProps: ClineSayTool = { - tool: "appliedDiff", - path: getReadablePath(task.cwd, relPath), - diff: sanitizedDiff, - isOutsideWorkspace, - } - - // Include any partial errors in the message - let resultMessage = "" - if (errors.length > 0) { - resultMessage = `Some operations failed:\n${errors.join("\n")}\n\n` - } - - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - content: sanitizedDiff, - isProtected: isWriteProtected, - diffStats, - } satisfies ClineSayTool) - - // Show diff view if focus disruption prevention is disabled - if (!isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.open(relPath) - await task.diffViewProvider.update(newContent, true) - task.diffViewProvider.scrollToFirstDiff() - } - - const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) - - if (!didApprove) { - // Revert changes if diff view was shown - if (!isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.revertChanges() - } - pushToolResult("Changes were rejected by the user.") - await task.diffViewProvider.reset() - return - } - - // Save the changes - if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) - } else { - // Call saveChanges to update the DiffViewProvider properties - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) - } - - // Track file edit operation - if (relPath) { - await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) - } - - task.didEditFile = true - - // Get the formatted response message - const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) - - // Add error info if some operations failed - if (errors.length > 0) { - pushToolResult(`${resultMessage}${message}`) - } else { - pushToolResult(message) - } - - // Record successful tool usage and cleanup - task.recordToolUsage("search_and_replace") - await task.diffViewProvider.reset() - this.resetPartialState() - - // Process any queued messages after file edit completes - task.processQueuedMessages() - } catch (error) { - await handleError("search and replace", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() - } - } - - override async handlePartial(task: Task, block: ToolUse<"search_and_replace">): Promise { - const relPath: string | undefined = block.params.path - - // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath)) { - return - } - - const operationsStr: string | undefined = block.params.operations - - let operationsPreview: string | undefined - if (operationsStr) { - try { - const ops = JSON.parse(operationsStr) - if (Array.isArray(ops) && ops.length > 0) { - operationsPreview = `${ops.length} operation(s)` - } - } catch { - operationsPreview = "parsing..." - } - } - - // relPath is guaranteed non-null after hasPathStabilized - const absolutePath = path.resolve(task.cwd, relPath!) - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) - - const sharedMessageProps: ClineSayTool = { - tool: "appliedDiff", - path: getReadablePath(task.cwd, relPath!), - diff: operationsPreview, - isOutsideWorkspace, - } - - await task.ask("tool", JSON.stringify(sharedMessageProps), block.partial).catch(() => {}) - } -} - -/** - * Escapes special regex characters in a string - * @param input String to escape regex characters in - * @returns Escaped string safe for regex pattern matching - */ -function escapeRegExp(input: string): string { - return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") -} - -export const searchAndReplaceTool = new SearchAndReplaceTool() +// Deprecated: Use EditTool instead. This file exists only for backward compatibility. +export { EditTool as SearchAndReplaceTool, searchAndReplaceTool } from "./EditTool" diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts new file mode 100644 index 00000000000..9e61fcee231 --- /dev/null +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -0,0 +1,423 @@ +import * as path from "path" +import fs from "fs/promises" + +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import { isPathOutsideWorkspace } from "../../../utils/pathUtils" +import { getReadablePath } from "../../../utils/path" +import { ToolUse, ToolResponse } from "../../../shared/tools" +import { editTool } from "../EditTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue(""), + }, +})) + +vi.mock("path", async () => { + const originalPath = await vi.importActual("path") + return { + ...originalPath, + resolve: vi.fn().mockImplementation((...args) => { + const separator = process.platform === "win32" ? "\\" : "/" + return args.join(separator) + }), + isAbsolute: vi.fn().mockReturnValue(false), + relative: vi.fn().mockImplementation((_from, to) => to), + } +}) + +vi.mock("delay", () => ({ + default: vi.fn(), +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Error: ${msg}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn().mockReturnValue("test/path.txt"), +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +vi.mock("vscode", () => ({ + window: { + showWarningMessage: vi.fn().mockResolvedValue(undefined), + }, + env: { + openExternal: vi.fn(), + }, + Uri: { + parse: vi.fn(), + }, +})) + +describe("editTool", () => { + // Test data + const testFilePath = "test/file.txt" + const absoluteFilePath = process.platform === "win32" ? "C:\\test\\file.txt" : "/test/file.txt" + const testFileContent = "Line 1\nLine 2\nLine 3\nLine 4" + + // Mocked functions + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockedFsReadFile = fs.readFile as unknown as MockedFunction< + (path: string, encoding: string) => Promise + > + const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction + const mockedGetReadablePath = getReadablePath as MockedFunction + const mockedPathResolve = path.resolve as MockedFunction + const mockedPathIsAbsolute = path.isAbsolute as MockedFunction + + const mockTask: any = {} + let mockAskApproval: ReturnType + let mockHandleError: ReturnType + let mockPushToolResult: ReturnType + let toolResult: ToolResponse | undefined + + beforeEach(() => { + vi.clearAllMocks() + + mockedPathResolve.mockReturnValue(absoluteFilePath) + mockedPathIsAbsolute.mockReturnValue(false) + mockedFileExistsAtPath.mockResolvedValue(true) + mockedFsReadFile.mockResolvedValue(testFileContent) + mockedIsPathOutsideWorkspace.mockReturnValue(false) + mockedGetReadablePath.mockReturnValue("test/path.txt") + + mockTask.cwd = "/" + mockTask.consecutiveMistakeCount = 0 + mockTask.didEditFile = false + mockTask.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + }), + }), + } + mockTask.rooIgnoreController = { + validateAccess: vi.fn().mockReturnValue(true), + } + mockTask.rooProtectedController = { + isWriteProtected: vi.fn().mockReturnValue(false), + } + mockTask.diffViewProvider = { + editType: undefined, + isEditing: false, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: null, + finalContent: "final content", + }), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Tool result message"), + } + mockTask.fileContextTracker = { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } + mockTask.say = vi.fn().mockResolvedValue(undefined) + mockTask.ask = vi.fn().mockResolvedValue(undefined) + mockTask.recordToolError = vi.fn() + mockTask.recordToolUsage = vi.fn() + mockTask.processQueuedMessages = vi.fn() + mockTask.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + + toolResult = undefined + }) + + /** + * Helper function to execute the edit tool with different parameters + */ + async function executeEditTool( + params: { + file_path?: string + old_string?: string + new_string?: string + replace_all?: string + } = {}, + options: { + fileExists?: boolean + fileContent?: string + isPartial?: boolean + accessAllowed?: boolean + } = {}, + ): Promise { + const fileExists = options.fileExists ?? true + const fileContent = options.fileContent ?? testFileContent + const isPartial = options.isPartial ?? false + const accessAllowed = options.accessAllowed ?? true + + mockedFileExistsAtPath.mockResolvedValue(fileExists) + mockedFsReadFile.mockResolvedValue(fileContent) + mockTask.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) + + const defaultParams = { + file_path: testFilePath, + old_string: "Line 2", + new_string: "Modified Line 2", + } + const fullParams: Record = { ...defaultParams, ...params } + + // Build nativeArgs from params (only include defined values) + const nativeArgs: Record = {} + if (fullParams.file_path !== undefined) { + nativeArgs.file_path = fullParams.file_path + } + if (fullParams.old_string !== undefined) { + nativeArgs.old_string = fullParams.old_string + } + if (fullParams.new_string !== undefined) { + nativeArgs.new_string = fullParams.new_string + } + if (fullParams.replace_all !== undefined) { + nativeArgs.replace_all = fullParams.replace_all === "true" + } + + const toolUse: ToolUse = { + type: "tool_use", + name: "edit", + params: fullParams as Partial>, + nativeArgs: nativeArgs as ToolUse<"edit">["nativeArgs"], + partial: isPartial, + } + + mockPushToolResult = vi.fn((result: ToolResponse) => { + toolResult = result + }) + + await editTool.handle(mockTask, toolUse as ToolUse<"edit">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + return toolResult + } + + describe("basic replacement", () => { + it("replaces a single unique occurrence of old_string with new_string", async () => { + await executeEditTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockTask.diffViewProvider.editType).toBe("modify") + expect(mockAskApproval).toHaveBeenCalled() + }) + }) + + describe("replace_all", () => { + it("replaces all occurrences when replace_all is true", async () => { + await executeEditTool( + { old_string: "Line", new_string: "Row", replace_all: "true" }, + { fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockTask.diffViewProvider.editType).toBe("modify") + expect(mockAskApproval).toHaveBeenCalled() + }) + }) + + describe("uniqueness check", () => { + it("returns error when old_string appears multiple times without replace_all", async () => { + const result = await executeEditTool( + { old_string: "Line", new_string: "Row" }, + { fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(result).toContain("Error:") + expect(result).toContain("3 matches") + expect(result).toContain("replace_all") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit") + }) + }) + + describe("no match error", () => { + it("returns error when old_string is not found in the file", async () => { + const result = await executeEditTool( + { old_string: "NonExistent", new_string: "New" }, + { fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(result).toContain("Error:") + expect(result).toContain("No match found") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit", "no_match") + }) + }) + + describe("old_string equals new_string", () => { + it("returns error when old_string and new_string are identical", async () => { + const result = await executeEditTool( + { old_string: "Line 2", new_string: "Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3" }, + ) + + expect(result).toContain("Error:") + expect(result).toContain("identical") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit") + }) + }) + + describe("missing required params", () => { + it("returns error when file_path is missing", async () => { + const result = await executeEditTool({ file_path: undefined }) + + expect(result).toBe("Missing param error") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit") + expect(mockTask.sayAndCreateMissingParamError).toHaveBeenCalledWith("edit", "file_path") + }) + + it("returns error when old_string is missing", async () => { + const result = await executeEditTool({ old_string: undefined }) + + expect(result).toBe("Missing param error") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit") + expect(mockTask.sayAndCreateMissingParamError).toHaveBeenCalledWith("edit", "old_string") + }) + + it("returns error when new_string is missing", async () => { + const result = await executeEditTool({ new_string: undefined }) + + expect(result).toBe("Missing param error") + expect(mockTask.consecutiveMistakeCount).toBe(1) + expect(mockTask.recordToolError).toHaveBeenCalledWith("edit") + expect(mockTask.sayAndCreateMissingParamError).toHaveBeenCalledWith("edit", "new_string") + }) + }) + + describe("file access", () => { + it("returns error when file does not exist", async () => { + const result = await executeEditTool({}, { fileExists: false }) + + expect(result).toContain("Error:") + expect(result).toContain("File not found") + expect(mockTask.consecutiveMistakeCount).toBe(1) + }) + + it("returns error when access is denied", async () => { + const result = await executeEditTool({}, { accessAllowed: false }) + + expect(result).toContain("Access denied") + }) + }) + + describe("approval workflow", () => { + it("saves changes when user approves", async () => { + mockAskApproval.mockResolvedValue(true) + + await executeEditTool() + + expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + expect(mockTask.recordToolUsage).toHaveBeenCalledWith("edit") + }) + + it("reverts changes when user rejects", async () => { + mockAskApproval.mockResolvedValue(false) + + const result = await executeEditTool() + + expect(mockTask.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(result).toContain("rejected") + }) + }) + + describe("partial block handling", () => { + it("handles partial block without errors after path stabilizes", async () => { + // Path stabilization requires two consecutive calls with the same path + await executeEditTool({}, { isPartial: true }) + await executeEditTool({}, { isPartial: true }) + + expect(mockTask.ask).toHaveBeenCalled() + }) + }) + + describe("error handling", () => { + it("handles file read errors gracefully", async () => { + mockedFsReadFile.mockRejectedValueOnce(new Error("Read failed")) + + const toolUse: ToolUse = { + type: "tool_use", + name: "edit", + params: { + file_path: testFilePath, + old_string: "Line 2", + new_string: "Modified", + }, + nativeArgs: { + file_path: testFilePath, + old_string: "Line 2", + new_string: "Modified", + } as ToolUse<"edit">["nativeArgs"], + partial: false, + } + + let capturedResult: ToolResponse | undefined + const localPushToolResult = vi.fn((result: ToolResponse) => { + capturedResult = result + }) + + await editTool.handle(mockTask, toolUse as ToolUse<"edit">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: localPushToolResult, + }) + + expect(capturedResult).toContain("Error:") + expect(capturedResult).toContain("Failed to read file") + expect(mockTask.consecutiveMistakeCount).toBe(1) + }) + + it("handles general errors and resets diff view", async () => { + mockTask.diffViewProvider.open.mockRejectedValueOnce(new Error("General error")) + + await executeEditTool() + + expect(mockHandleError).toHaveBeenCalledWith("edit", expect.any(Error)) + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + }) + }) + + describe("file tracking", () => { + it("tracks file context after successful edit", async () => { + await executeEditTool() + + expect(mockTask.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") + }) + }) +}) diff --git a/src/core/tools/__tests__/searchAndReplaceTool.spec.ts b/src/core/tools/__tests__/searchAndReplaceTool.spec.ts index 241d7b67b0d..53d3ee11254 100644 --- a/src/core/tools/__tests__/searchAndReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchAndReplaceTool.spec.ts @@ -1,414 +1,13 @@ -import * as path from "path" -import fs from "fs/promises" +// Deprecated: Tests for the old SearchAndReplaceTool. +// Full edit tool tests are in editTool.spec.ts. +// This file only verifies the backward-compatible re-export. -import type { MockedFunction } from "vitest" - -import { fileExistsAtPath } from "../../../utils/fs" -import { isPathOutsideWorkspace } from "../../../utils/pathUtils" -import { getReadablePath } from "../../../utils/path" -import { ToolUse, ToolResponse } from "../../../shared/tools" import { searchAndReplaceTool } from "../SearchAndReplaceTool" +import { editTool } from "../EditTool" -vi.mock("fs/promises", () => ({ - default: { - readFile: vi.fn().mockResolvedValue(""), - }, -})) - -vi.mock("path", async () => { - const originalPath = await vi.importActual("path") - return { - ...originalPath, - resolve: vi.fn().mockImplementation((...args) => { - const separator = process.platform === "win32" ? "\\" : "/" - return args.join(separator) - }), - isAbsolute: vi.fn().mockReturnValue(false), - relative: vi.fn().mockImplementation((from, to) => to), - } -}) - -vi.mock("delay", () => ({ - default: vi.fn(), -})) - -vi.mock("../../../utils/fs", () => ({ - fileExistsAtPath: vi.fn().mockResolvedValue(true), -})) - -vi.mock("../../prompts/responses", () => ({ - formatResponse: { - toolError: vi.fn((msg) => `Error: ${msg}`), - rooIgnoreError: vi.fn((path) => `Access denied: ${path}`), - createPrettyPatch: vi.fn(() => "mock-diff"), - }, -})) - -vi.mock("../../../utils/pathUtils", () => ({ - isPathOutsideWorkspace: vi.fn().mockReturnValue(false), -})) - -vi.mock("../../../utils/path", () => ({ - getReadablePath: vi.fn().mockReturnValue("test/path.txt"), -})) - -vi.mock("../../diff/stats", () => ({ - sanitizeUnifiedDiff: vi.fn((diff) => diff), - computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), -})) - -vi.mock("vscode", () => ({ - window: { - showWarningMessage: vi.fn().mockResolvedValue(undefined), - }, - env: { - openExternal: vi.fn(), - }, - Uri: { - parse: vi.fn(), - }, -})) - -describe("searchAndReplaceTool", () => { - // Test data - const testFilePath = "test/file.txt" - const absoluteFilePath = process.platform === "win32" ? "C:\\test\\file.txt" : "/test/file.txt" - const testFileContent = "Line 1\nLine 2\nLine 3\nLine 4" - - // Mocked functions - const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction - const mockedFsReadFile = fs.readFile as unknown as MockedFunction< - (path: string, encoding: string) => Promise - > - const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction - const mockedGetReadablePath = getReadablePath as MockedFunction - const mockedPathResolve = path.resolve as MockedFunction - const mockedPathIsAbsolute = path.isAbsolute as MockedFunction - - const mockTask: any = {} - let mockAskApproval: ReturnType - let mockHandleError: ReturnType - let mockPushToolResult: ReturnType - let toolResult: ToolResponse | undefined - - beforeEach(() => { - vi.clearAllMocks() - - mockedPathResolve.mockReturnValue(absoluteFilePath) - mockedPathIsAbsolute.mockReturnValue(false) - mockedFileExistsAtPath.mockResolvedValue(true) - mockedFsReadFile.mockResolvedValue(testFileContent) - mockedIsPathOutsideWorkspace.mockReturnValue(false) - mockedGetReadablePath.mockReturnValue("test/path.txt") - - mockTask.cwd = "/" - mockTask.consecutiveMistakeCount = 0 - mockTask.didEditFile = false - mockTask.providerRef = { - deref: vi.fn().mockReturnValue({ - getState: vi.fn().mockResolvedValue({ - diagnosticsEnabled: true, - writeDelayMs: 1000, - experiments: {}, - }), - }), - } - mockTask.rooIgnoreController = { - validateAccess: vi.fn().mockReturnValue(true), - } - mockTask.rooProtectedController = { - isWriteProtected: vi.fn().mockReturnValue(false), - } - mockTask.diffViewProvider = { - editType: undefined, - isEditing: false, - originalContent: "", - open: vi.fn().mockResolvedValue(undefined), - update: vi.fn().mockResolvedValue(undefined), - reset: vi.fn().mockResolvedValue(undefined), - revertChanges: vi.fn().mockResolvedValue(undefined), - saveChanges: vi.fn().mockResolvedValue({ - newProblemsMessage: "", - userEdits: null, - finalContent: "final content", - }), - saveDirectly: vi.fn().mockResolvedValue(undefined), - scrollToFirstDiff: vi.fn(), - pushToolWriteResult: vi.fn().mockResolvedValue("Tool result message"), - } - mockTask.fileContextTracker = { - trackFileContext: vi.fn().mockResolvedValue(undefined), - } - mockTask.say = vi.fn().mockResolvedValue(undefined) - mockTask.ask = vi.fn().mockResolvedValue(undefined) - mockTask.recordToolError = vi.fn() - mockTask.recordToolUsage = vi.fn() - mockTask.processQueuedMessages = vi.fn() - mockTask.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") - - mockAskApproval = vi.fn().mockResolvedValue(true) - mockHandleError = vi.fn().mockResolvedValue(undefined) - - toolResult = undefined - }) - - /** - * Helper function to execute the search and replace tool with different parameters - */ - async function executeSearchAndReplaceTool( - params: Partial = {}, - options: { - fileExists?: boolean - fileContent?: string - isPartial?: boolean - accessAllowed?: boolean - } = {}, - ): Promise { - const fileExists = options.fileExists ?? true - const fileContent = options.fileContent ?? testFileContent - const isPartial = options.isPartial ?? false - const accessAllowed = options.accessAllowed ?? true - - mockedFileExistsAtPath.mockResolvedValue(fileExists) - mockedFsReadFile.mockResolvedValue(fileContent) - mockTask.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) - - const baseParams: Record = { - path: testFilePath, - operations: JSON.stringify([{ search: "Line 2", replace: "Modified Line 2" }]), - } - const fullParams: Record = { ...baseParams, ...params } - const nativeArgs: Record = { - path: fullParams.path, - operations: - typeof fullParams.operations === "string" ? JSON.parse(fullParams.operations) : fullParams.operations, - } - - const toolUse: ToolUse = { - type: "tool_use", - name: "search_and_replace", - params: fullParams as any, - nativeArgs: nativeArgs as any, - partial: isPartial, - } - - mockPushToolResult = vi.fn((result: ToolResponse) => { - toolResult = result - }) - - await searchAndReplaceTool.handle(mockTask, toolUse as ToolUse<"search_and_replace">, { - askApproval: mockAskApproval, - handleError: mockHandleError, - pushToolResult: mockPushToolResult, - }) - - return toolResult - } - - describe("parameter validation", () => { - it("returns error when path is missing", async () => { - const result = await executeSearchAndReplaceTool({ path: undefined }) - - expect(result).toBe("Missing param error") - expect(mockTask.consecutiveMistakeCount).toBe(1) - expect(mockTask.recordToolError).toHaveBeenCalledWith("search_and_replace") - }) - - it("returns error when operations is missing", async () => { - const result = await executeSearchAndReplaceTool({ operations: undefined }) - - expect(result).toContain("Error:") - expect(result).toContain("Missing or empty 'operations' parameter") - expect(mockTask.consecutiveMistakeCount).toBe(1) - }) - - it("returns error when operations is empty array", async () => { - const result = await executeSearchAndReplaceTool({ operations: JSON.stringify([]) }) - - expect(result).toContain("Error:") - expect(result).toContain("Missing or empty 'operations' parameter") - expect(mockTask.consecutiveMistakeCount).toBe(1) - }) - }) - - describe("file access", () => { - it("returns error when file does not exist", async () => { - const result = await executeSearchAndReplaceTool({}, { fileExists: false }) - - expect(result).toContain("Error:") - expect(result).toContain("File not found") - expect(mockTask.consecutiveMistakeCount).toBe(1) - }) - - it("returns error when access is denied", async () => { - const result = await executeSearchAndReplaceTool({}, { accessAllowed: false }) - - expect(result).toContain("Access denied") - }) - }) - - describe("search and replace logic", () => { - it("returns error when no match is found", async () => { - const result = await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: "NonExistent", replace: "New" }]) }, - { fileContent: "Line 1\nLine 2\nLine 3" }, - ) - - expect(result).toContain("Error:") - expect(result).toContain("No match found") - expect(mockTask.consecutiveMistakeCount).toBe(1) - expect(mockTask.recordToolError).toHaveBeenCalledWith("search_and_replace", "no_match") - }) - - it("returns error when multiple matches are found", async () => { - const result = await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: "Line", replace: "Row" }]) }, - { fileContent: "Line 1\nLine 2\nLine 3" }, - ) - - expect(result).toContain("Error:") - expect(result).toContain("3 matches") - expect(mockTask.consecutiveMistakeCount).toBe(1) - }) - - it("successfully replaces single unique match", async () => { - await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: "Line 2", replace: "Modified Line 2" }]) }, - { fileContent: "Line 1\nLine 2\nLine 3" }, - ) - - expect(mockTask.consecutiveMistakeCount).toBe(0) - expect(mockTask.diffViewProvider.editType).toBe("modify") - expect(mockAskApproval).toHaveBeenCalled() - }) - }) - - describe("CRLF normalization", () => { - it("normalizes CRLF to LF when reading file", async () => { - const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3" - - await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: "Line 2", replace: "Modified Line 2" }]) }, - { fileContent: contentWithCRLF }, - ) - - expect(mockTask.consecutiveMistakeCount).toBe(0) - expect(mockAskApproval).toHaveBeenCalled() - }) - - it("normalizes CRLF in search string to match LF-normalized file content", async () => { - // File has CRLF line endings - const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3" - // Search string also has CRLF (simulating what the model might send) - const searchWithCRLF = "Line 1\r\nLine 2" - - await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: searchWithCRLF, replace: "Modified Lines" }]) }, - { fileContent: contentWithCRLF }, - ) - - expect(mockTask.consecutiveMistakeCount).toBe(0) - expect(mockAskApproval).toHaveBeenCalled() - }) - - it("matches LF search string against CRLF file content after normalization", async () => { - // File has CRLF line endings - const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3" - // Search string has LF (typical model output) - const searchWithLF = "Line 1\nLine 2" - - await executeSearchAndReplaceTool( - { operations: JSON.stringify([{ search: searchWithLF, replace: "Modified Lines" }]) }, - { fileContent: contentWithCRLF }, - ) - - expect(mockTask.consecutiveMistakeCount).toBe(0) - expect(mockAskApproval).toHaveBeenCalled() - }) - }) - - describe("approval workflow", () => { - it("saves changes when user approves", async () => { - mockAskApproval.mockResolvedValue(true) - - await executeSearchAndReplaceTool() - - expect(mockTask.diffViewProvider.saveChanges).toHaveBeenCalled() - expect(mockTask.didEditFile).toBe(true) - expect(mockTask.recordToolUsage).toHaveBeenCalledWith("search_and_replace") - }) - - it("reverts changes when user rejects", async () => { - mockAskApproval.mockResolvedValue(false) - - const result = await executeSearchAndReplaceTool() - - expect(mockTask.diffViewProvider.revertChanges).toHaveBeenCalled() - expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() - expect(result).toContain("rejected") - }) - }) - - describe("partial block handling", () => { - it("handles partial block without errors after path stabilizes", async () => { - // Path stabilization requires two consecutive calls with the same path - // First call sets lastSeenPartialPath, second call sees it has stabilized - await executeSearchAndReplaceTool({}, { isPartial: true }) - await executeSearchAndReplaceTool({}, { isPartial: true }) - - expect(mockTask.ask).toHaveBeenCalled() - }) - }) - - describe("error handling", () => { - it("handles file read errors gracefully", async () => { - mockedFsReadFile.mockRejectedValueOnce(new Error("Read failed")) - - const toolUse: ToolUse = { - type: "tool_use", - name: "search_and_replace", - params: { - path: testFilePath, - operations: JSON.stringify([{ search: "Line 2", replace: "Modified" }]), - }, - nativeArgs: { - path: testFilePath, - operations: [{ search: "Line 2", replace: "Modified" }], - }, - partial: false, - } - - let capturedResult: ToolResponse | undefined - const localPushToolResult = vi.fn((result: ToolResponse) => { - capturedResult = result - }) - - await searchAndReplaceTool.handle(mockTask, toolUse as ToolUse<"search_and_replace">, { - askApproval: mockAskApproval, - handleError: mockHandleError, - pushToolResult: localPushToolResult, - }) - - expect(capturedResult).toContain("Error:") - expect(capturedResult).toContain("Failed to read file") - expect(mockTask.consecutiveMistakeCount).toBe(1) - }) - - it("handles general errors and resets diff view", async () => { - mockTask.diffViewProvider.open.mockRejectedValueOnce(new Error("General error")) - - await executeSearchAndReplaceTool() - - expect(mockHandleError).toHaveBeenCalledWith("search and replace", expect.any(Error)) - expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() - }) - }) - - describe("file tracking", () => { - it("tracks file context after successful edit", async () => { - await executeSearchAndReplaceTool() - - expect(mockTask.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") - }) +describe("SearchAndReplaceTool re-export", () => { + it("exports searchAndReplaceTool as an alias for editTool", () => { + expect(searchAndReplaceTool).toBeDefined() + expect(searchAndReplaceTool).toBe(editTool) }) }) diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index ab261af722e..243a170ed90 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -4,7 +4,7 @@ import { customToolRegistry } from "@roo-code/core" import { type Mode, FileRestrictionError, getModeBySlug, getGroupName } from "../../shared/modes" import { EXPERIMENT_IDS } from "../../shared/experiments" -import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS } from "../../shared/tools" +import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS, TOOL_ALIASES } from "../../shared/tools" /** * Checks if a tool name is a valid, known tool. @@ -126,11 +126,18 @@ export function isToolAllowedForMode( experiments?: Record, includedTools?: string[], // Opt-in tools explicitly included (e.g., from modelInfo) ): boolean { + // Resolve alias to canonical name (e.g., "search_and_replace" → "edit") + const resolvedTool = TOOL_ALIASES[tool] ?? tool + const resolvedIncludedTools = includedTools?.map((t) => TOOL_ALIASES[t] ?? t) + // Check tool requirements first — explicit disabling takes priority over everything, // including ALWAYS_AVAILABLE_TOOLS. This ensures disabledTools works consistently // at both the filtering layer and the execution-time validation layer. if (toolRequirements && typeof toolRequirements === "object") { - if (tool in toolRequirements && !toolRequirements[tool]) { + if ( + (tool in toolRequirements && !toolRequirements[tool]) || + (resolvedTool in toolRequirements && !toolRequirements[resolvedTool]) + ) { return false } } else if (toolRequirements === false) { @@ -179,10 +186,11 @@ export function isToolAllowedForMode( } // Check if the tool is in the group's regular tools - const isRegularTool = groupConfig.tools.includes(tool) + const isRegularTool = groupConfig.tools.includes(resolvedTool) // Check if the tool is a custom tool that has been explicitly included - const isCustomTool = groupConfig.customTools?.includes(tool) && includedTools?.includes(tool) + const isCustomTool = + groupConfig.customTools?.includes(resolvedTool) && resolvedIncludedTools?.includes(resolvedTool) // If the tool isn't in regular tools and isn't an included custom tool, continue to next group if (!isRegularTool && !isCustomTool) { diff --git a/src/shared/tools.ts b/src/shared/tools.ts index 570f55c4f2f..48becaf028a 100644 --- a/src/shared/tools.ts +++ b/src/shared/tools.ts @@ -71,6 +71,7 @@ export const toolParamNames = [ "file_path", // search_replace and edit_file parameter "old_string", // search_replace and edit_file parameter "new_string", // search_replace and edit_file parameter + "replace_all", // edit tool parameter for replacing all occurrences "expected_replacements", // edit_file parameter for multiple occurrences "artifact_id", // read_command_output parameter "search", // read_command_output parameter for grep-like search @@ -101,7 +102,8 @@ export type NativeToolArgs = { attempt_completion: { result: string } execute_command: { command: string; cwd?: string } apply_diff: { path: string; diff: string } - search_and_replace: { path: string; operations: Array<{ search: string; replace: string }> } + edit: { file_path: string; old_string: string; new_string: string; replace_all?: boolean } + search_and_replace: { file_path: string; old_string: string; new_string: string; replace_all?: boolean } search_replace: { file_path: string; old_string: string; new_string: string } edit_file: { file_path: string; old_string: string; new_string: string; expected_replacements?: number } apply_patch: { patch: string } @@ -281,6 +283,7 @@ export const TOOL_DISPLAY_NAMES: Record = { read_command_output: "read command output", write_to_file: "write files", apply_diff: "apply changes", + edit: "edit files", search_and_replace: "apply changes using search and replace", search_replace: "apply single search and replace", edit_file: "edit files using search and replace", @@ -309,7 +312,7 @@ export const TOOL_GROUPS: Record = { }, edit: { tools: ["apply_diff", "write_to_file", "generate_image"], - customTools: ["search_and_replace", "search_replace", "edit_file", "apply_patch"], + customTools: ["edit", "search_replace", "edit_file", "apply_patch"], }, browser: { tools: ["browser_action"], @@ -349,6 +352,7 @@ export const ALWAYS_AVAILABLE_TOOLS: ToolName[] = [ */ export const TOOL_ALIASES: Record = { write_file: "write_to_file", + search_and_replace: "edit", } as const export type DiffResult = From b4923b306c4fbf378b3a58c7c4fa874ed4272f64 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Sat, 7 Feb 2026 17:32:18 -0700 Subject: [PATCH 2/5] chore: delete orphaned search_and_replace.ts prompt schema --- .../tools/native-tools/search_and_replace.ts | 44 ------------------- 1 file changed, 44 deletions(-) delete mode 100644 src/core/prompts/tools/native-tools/search_and_replace.ts diff --git a/src/core/prompts/tools/native-tools/search_and_replace.ts b/src/core/prompts/tools/native-tools/search_and_replace.ts deleted file mode 100644 index ce785b6a165..00000000000 --- a/src/core/prompts/tools/native-tools/search_and_replace.ts +++ /dev/null @@ -1,44 +0,0 @@ -import type OpenAI from "openai" - -const SEARCH_AND_REPLACE_DESCRIPTION = `Apply precise, targeted modifications to an existing file using search and replace operations. This tool is for surgical edits only; provide an array of operations where each operation specifies the exact text to search for and what to replace it with. The search text must exactly match the existing content, including whitespace and indentation.` - -const search_and_replace = { - type: "function", - function: { - name: "search_and_replace", - description: SEARCH_AND_REPLACE_DESCRIPTION, - parameters: { - type: "object", - properties: { - path: { - type: "string", - description: "The path of the file to modify, relative to the current workspace directory.", - }, - operations: { - type: "array", - description: "Array of search and replace operations to perform on the file.", - items: { - type: "object", - properties: { - search: { - type: "string", - description: - "The exact text to find in the file. Must match exactly, including whitespace.", - }, - replace: { - type: "string", - description: "The text to replace the search text with.", - }, - }, - required: ["search", "replace"], - }, - minItems: 1, - }, - }, - required: ["path", "operations"], - additionalProperties: false, - }, - }, -} satisfies OpenAI.Chat.ChatCompletionTool - -export default search_and_replace From fd87a2c88389be048a08dfedb6e19bf45c9686d8 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Sat, 7 Feb 2026 19:57:08 -0700 Subject: [PATCH 3/5] fix: avoid XML false positives for / tags --- ...esentAssistantMessage-unknown-tool.spec.ts | 50 +++++++++++++++++++ .../presentAssistantMessage.ts | 21 +++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts index 15a1e2d8672..0fed09f3ee7 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.ts @@ -242,4 +242,54 @@ describe("presentAssistantMessage - Unknown Tool Handling", () => { expect(toolResult.is_error).toBe(true) expect(toolResult.content).toContain("due to user rejecting a previous tool") }) + + it("should not treat as XML tool markup", async () => { + mockTask.assistantMessageContent = [ + { + type: "text", + content: "Please open the panel and continue.", + partial: false, + }, + ] + + await presentAssistantMessage(mockTask) + + expect(mockTask.say).toHaveBeenCalledWith( + "text", + "Please open the panel and continue.", + undefined, + false, + ) + expect(mockTask.say).not.toHaveBeenCalledWith( + "error", + expect.stringContaining("XML tool calls are no longer supported"), + ) + expect(mockTask.didAlreadyUseTool).toBe(false) + expect(mockTask.consecutiveMistakeCount).toBe(0) + }) + + it("should not treat as XML tool markup", async () => { + mockTask.assistantMessageContent = [ + { + type: "text", + content: "Use an region in the docs example.", + partial: false, + }, + ] + + await presentAssistantMessage(mockTask) + + expect(mockTask.say).toHaveBeenCalledWith( + "text", + "Use an region in the docs example.", + undefined, + false, + ) + expect(mockTask.say).not.toHaveBeenCalledWith( + "error", + expect.stringContaining("XML tool calls are no longer supported"), + ) + expect(mockTask.didAlreadyUseTool).toBe(false) + expect(mockTask.consecutiveMistakeCount).toBe(0) + }) }) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index ce3e44cd2a3..f578fac3c6f 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -1048,6 +1048,25 @@ function containsXmlToolMarkup(text: string): boolean { // Avoid regex so we don't keep legacy XML parsing artifacts around. // Note: This is a best-effort safeguard; tool_use blocks without an id are rejected elsewhere. + const isTagBoundary = (char: string | undefined): boolean => { + if (char === undefined) { + return true + } + return char === ">" || char === "/" || char === " " || char === "\n" || char === "\r" || char === "\t" + } + + const hasTagReference = (haystack: string, prefix: string): boolean => { + let index = haystack.indexOf(prefix) + while (index !== -1) { + const nextChar = haystack[index + prefix.length] + if (isTagBoundary(nextChar)) { + return true + } + index = haystack.indexOf(prefix, index + 1) + } + return false + } + // First, strip out content inside markdown code fences to avoid false positives // when users paste documentation or examples containing tool tag references. // This handles both fenced code blocks (```) and inline code (`). @@ -1085,5 +1104,5 @@ function containsXmlToolMarkup(text: string): boolean { "write_to_file", ] as const - return toolNames.some((name) => lower.includes(`<${name}`) || lower.includes(` hasTagReference(lower, `<${name}`) || hasTagReference(lower, ` Date: Sat, 7 Feb 2026 20:16:01 -0700 Subject: [PATCH 4/5] test: stabilize HistoryPreview ordering in windows CI --- .../history/__tests__/HistoryPreview.spec.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/webview-ui/src/components/history/__tests__/HistoryPreview.spec.tsx b/webview-ui/src/components/history/__tests__/HistoryPreview.spec.tsx index da344970a88..ba12017a46f 100644 --- a/webview-ui/src/components/history/__tests__/HistoryPreview.spec.tsx +++ b/webview-ui/src/components/history/__tests__/HistoryPreview.spec.tsx @@ -27,7 +27,7 @@ const mockTasks: HistoryItem[] = [ id: "task-1", number: 1, task: "First task", - ts: Date.now(), + ts: 600, tokensIn: 100, tokensOut: 50, totalCost: 0.01, @@ -36,7 +36,7 @@ const mockTasks: HistoryItem[] = [ id: "task-2", number: 2, task: "Second task", - ts: Date.now(), + ts: 500, tokensIn: 200, tokensOut: 100, totalCost: 0.02, @@ -45,7 +45,7 @@ const mockTasks: HistoryItem[] = [ id: "task-3", number: 3, task: "Third task", - ts: Date.now(), + ts: 400, tokensIn: 150, tokensOut: 75, totalCost: 0.015, @@ -54,7 +54,7 @@ const mockTasks: HistoryItem[] = [ id: "task-4", number: 4, task: "Fourth task", - ts: Date.now(), + ts: 300, tokensIn: 300, tokensOut: 150, totalCost: 0.03, @@ -63,7 +63,7 @@ const mockTasks: HistoryItem[] = [ id: "task-5", number: 5, task: "Fifth task", - ts: Date.now(), + ts: 200, tokensIn: 250, tokensOut: 125, totalCost: 0.025, @@ -72,7 +72,7 @@ const mockTasks: HistoryItem[] = [ id: "task-6", number: 6, task: "Sixth task", - ts: Date.now(), + ts: 100, tokensIn: 400, tokensOut: 200, totalCost: 0.04, From 285a161dbe2f8ac8b2ad61993ddcc493f5f022ca Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Sat, 7 Feb 2026 20:25:17 -0700 Subject: [PATCH 5/5] test: increase custom tool cache clear timeout for CI --- .../src/custom-tools/__tests__/custom-tool-registry.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/custom-tools/__tests__/custom-tool-registry.spec.ts b/packages/core/src/custom-tools/__tests__/custom-tool-registry.spec.ts index c1838440c24..d1694f2effa 100644 --- a/packages/core/src/custom-tools/__tests__/custom-tool-registry.spec.ts +++ b/packages/core/src/custom-tools/__tests__/custom-tool-registry.spec.ts @@ -281,7 +281,7 @@ describe("CustomToolRegistry", () => { const result = await registry.loadFromDirectory(TEST_FIXTURES_DIR) expect(result.loaded).toContain("cached") - }, 30000) + }, 120_000) }) describe.sequential("loadFromDirectories", () => {