diff --git a/src/core/tools/SearchAndReplaceTool.ts b/src/core/tools/SearchAndReplaceTool.ts index 49d159f455..7d03a6a22c 100644 --- a/src/core/tools/SearchAndReplaceTool.ts +++ b/src/core/tools/SearchAndReplaceTool.ts @@ -111,6 +111,8 @@ export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> { 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") @@ -125,7 +127,9 @@ export class SearchAndReplaceTool extends BaseTool<"search_and_replace"> { const errors: string[] = [] for (let i = 0; i < operations.length; i++) { - const { search, replace } = operations[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 diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index b0150d04a1..dadb97fde5 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -105,6 +105,8 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { 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_replace") @@ -114,8 +116,12 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { return } + // Normalize line endings in search/replace strings to match file content + const normalizedOldString = old_string.replace(/\r\n/g, "\n") + const normalizedNewString = new_string.replace(/\r\n/g, "\n") + // Check for exact match (literal string, not regex) - const matchCount = fileContent.split(old_string).length - 1 + const matchCount = fileContent.split(normalizedOldString).length - 1 if (matchCount === 0) { task.consecutiveMistakeCount++ @@ -142,7 +148,7 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { } // Apply the single replacement - const newContent = fileContent.replace(old_string, new_string) + const newContent = fileContent.replace(normalizedOldString, normalizedNewString) // Check if any changes were made if (newContent === fileContent) { diff --git a/src/core/tools/__tests__/searchAndReplaceTool.spec.ts b/src/core/tools/__tests__/searchAndReplaceTool.spec.ts new file mode 100644 index 0000000000..c73744ec57 --- /dev/null +++ b/src/core/tools/__tests__/searchAndReplaceTool.spec.ts @@ -0,0 +1,405 @@ +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 { searchAndReplaceTool } from "../SearchAndReplaceTool" + +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 mockRemoveClosingTag: 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) + mockRemoveClosingTag = vi.fn((tag, content) => content) + + 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 toolUse: ToolUse = { + type: "tool_use", + name: "search_and_replace", + params: { + path: testFilePath, + operations: JSON.stringify([{ search: "Line 2", replace: "Modified Line 2" }]), + ...params, + }, + 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, + removeClosingTag: mockRemoveClosingTag, + toolProtocol: "native", + }) + + 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", async () => { + 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" }]), + }, + 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, + removeClosingTag: mockRemoveClosingTag, + toolProtocol: "native", + }) + + 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") + }) + }) +}) diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index b9b1e453d4..984808e971 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -379,4 +379,48 @@ describe("searchReplaceTool", () => { expect(mockCline.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") }) }) + + describe("CRLF normalization", () => { + it("normalizes CRLF to LF when reading file", async () => { + const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3" + + await executeSearchReplaceTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: contentWithCRLF }, + ) + + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(mockAskApproval).toHaveBeenCalled() + }) + + it("normalizes CRLF in old_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 executeSearchReplaceTool( + { old_string: searchWithCRLF, new_string: "Modified Lines" }, + { fileContent: contentWithCRLF }, + ) + + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(mockAskApproval).toHaveBeenCalled() + }) + + it("matches LF old_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 executeSearchReplaceTool( + { old_string: searchWithLF, new_string: "Modified Lines" }, + { fileContent: contentWithCRLF }, + ) + + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(mockAskApproval).toHaveBeenCalled() + }) + }) })