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", () => { 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/__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 c183d51ca53..f578fac3c6f 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, @@ -1046,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 (`). @@ -1073,6 +1094,7 @@ function containsXmlToolMarkup(text: string): boolean { "new_task", "read_command_output", "read_file", + "edit", "search_and_replace", "search_files", "search_replace", @@ -1082,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, ` 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 = 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,