diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index c8024c7500b..133fd58a77b 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -9,7 +9,6 @@ import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" -import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" import type { ToolUse } from "../../shared/tools" @@ -33,11 +32,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { async execute(params: ApplyDiffParams, task: Task, callbacks: ToolCallbacks): Promise { const { askApproval, handleError, pushToolResult, toolProtocol } = callbacks - let { path: relPath, diff: diffContent } = params - - if (diffContent && !task.api.getModel().id.includes("claude")) { - diffContent = unescapeHtmlEntities(diffContent) - } + const { path: relPath, diff: diffContent } = params try { if (!relPath) { diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index 7feb71b0b89..f9278f791e3 100644 --- a/src/core/tools/ExecuteCommandTool.ts +++ b/src/core/tools/ExecuteCommandTool.ts @@ -11,7 +11,6 @@ import { Task } from "../task/Task" import { ToolUse, ToolResponse } from "../../shared/tools" import { formatResponse } from "../prompts/responses" -import { unescapeHtmlEntities } from "../../utils/text-normalization" import { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcess } from "../../integrations/terminal/types" import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry" import { Terminal } from "../../integrations/terminal/Terminal" @@ -58,8 +57,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { task.consecutiveMistakeCount = 0 - const unescapedCommand = unescapeHtmlEntities(command) - const didApprove = await askApproval("command", unescapedCommand) + const didApprove = await askApproval("command", command) if (!didApprove) { return @@ -86,16 +84,14 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { .get("commandTimeoutAllowlist", []) // Check if command matches any prefix in the allowlist - const isCommandAllowlisted = commandTimeoutAllowlist.some((prefix) => - unescapedCommand.startsWith(prefix.trim()), - ) + const isCommandAllowlisted = commandTimeoutAllowlist.some((prefix) => command.startsWith(prefix.trim())) // Convert seconds to milliseconds for internal use, but skip timeout if command is allowlisted const commandExecutionTimeout = isCommandAllowlisted ? 0 : commandExecutionTimeoutSeconds * 1000 const options: ExecuteCommandOptions = { executionId, - command: unescapedCommand, + command, customCwd, terminalShellIntegrationDisabled, terminalOutputLineLimit, diff --git a/src/core/tools/MultiApplyDiffTool.ts b/src/core/tools/MultiApplyDiffTool.ts index af5fefa251c..57de65343a0 100644 --- a/src/core/tools/MultiApplyDiffTool.ts +++ b/src/core/tools/MultiApplyDiffTool.ts @@ -10,7 +10,6 @@ import { ToolUse, RemoveClosingTag, AskApproval, HandleError, PushToolResult } f import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" -import { unescapeHtmlEntities } from "../../utils/text-normalization" import { parseXmlForDiff } from "../../utils/xml" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { applyDiffTool as applyDiffToolClass } from "./ApplyDiffTool" @@ -202,7 +201,7 @@ Original error: ${errorMessage}` path: legacyPath, diff: [ { - content: legacyDiffContent, // Unescaping will be handled later like new diffs + content: legacyDiffContent, startLine: legacyStartLineStr ? parseInt(legacyStartLineStr) : undefined, }, ], @@ -320,12 +319,7 @@ Original error: ${errorMessage}` let unified = "" try { const original = await fs.readFile(opResult.absolutePath!, "utf-8") - const processed = !cline.api.getModel().id.includes("claude") - ? (opResult.diffItems || []).map((item) => ({ - ...item, - content: item.content ? unescapeHtmlEntities(item.content) : item.content, - })) - : opResult.diffItems || [] + const processed = opResult.diffItems || [] const applyRes = (await cline.diffStrategy?.applyDiff(original, processed)) ?? ({ success: false } as any) @@ -481,13 +475,8 @@ Original error: ${errorMessage}` let successCount = 0 let formattedError = "" - // Pre-process all diff items for HTML entity unescaping if needed - const processedDiffItems = !cline.api.getModel().id.includes("claude") - ? diffItems.map((item) => ({ - ...item, - content: item.content ? unescapeHtmlEntities(item.content) : item.content, - })) - : diffItems + // Use diff items directly without HTML entity unescaping + const processedDiffItems = diffItems // Apply all diffs at once with the array-based method const diffResult = (await cline.diffStrategy?.applyDiff(originalContent, processedDiffItems)) ?? { diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 11247ec03d6..6307778e882 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -11,7 +11,6 @@ import { fileExistsAtPath, createDirectoriesForFile } from "../../utils/fs" import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" -import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" import type { ToolUse } from "../../shared/tools" @@ -88,10 +87,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { newContent = newContent.split("\n").slice(0, -1).join("\n") } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } - const fullPath = relPath ? path.resolve(task.cwd, removeClosingTag("path", relPath)) : "" const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) diff --git a/src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts b/src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts index f93a29caaf4..1e13c7cb63f 100644 --- a/src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts +++ b/src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts @@ -23,9 +23,6 @@ vitest.mock("../../prompts/responses", () => ({ rooIgnoreError: vitest.fn((msg) => `RooIgnore Error: ${msg}`), }, })) -vitest.mock("../../../utils/text-normalization", () => ({ - unescapeHtmlEntities: vitest.fn((text) => text), -})) vitest.mock("../../../shared/package", () => ({ Package: { name: "roo-cline", diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 0406a83d2a2..f77276e1f25 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -6,7 +6,6 @@ import * as vscode from "vscode" import { Task } from "../../task/Task" import { formatResponse } from "../../prompts/responses" import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "../../../shared/tools" -import { unescapeHtmlEntities } from "../../../utils/text-normalization" // Mock dependencies vitest.mock("execa", () => ({ @@ -106,36 +105,44 @@ describe("executeCommandTool", () => { }) /** - * Tests for HTML entity unescaping in commands - * This verifies that HTML entities are properly converted to their actual characters + * Tests for HTML entity preservation in commands + * Commands should be passed to the terminal exactly as provided by the model + * HTML entities should NOT be converted to their character equivalents */ - describe("HTML entity unescaping", () => { - it("should unescape < to < character", () => { - const input = "echo <test>" - const expected = "echo " - expect(unescapeHtmlEntities(input)).toBe(expected) - }) + describe("HTML entity preservation", () => { + it("should preserve HTML entities in commands (not convert them)", async () => { + // If a model sends a command with HTML entities, they should be preserved as-is + // This ensures commands are executed exactly as the model intended + mockToolUse.params.command = "echo <test>" - it("should unescape > to > character", () => { - const input = "echo test > output.txt" - const expected = "echo test > output.txt" - expect(unescapeHtmlEntities(input)).toBe(expected) - }) + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + removeClosingTag: mockRemoveClosingTag as unknown as RemoveClosingTag, + toolProtocol: "xml", + }) - it("should unescape & to & character", () => { - const input = "echo foo && echo bar" - const expected = "echo foo && echo bar" - expect(unescapeHtmlEntities(input)).toBe(expected) + // The command should be passed through unchanged - entities preserved + expect(mockAskApproval).toHaveBeenCalledWith("command", "echo <test>") }) - it("should handle multiple mixed HTML entities", () => { - const input = "grep -E 'pattern' <file.txt >output.txt 2>&1" - const expected = "grep -E 'pattern' output.txt 2>&1" - expect(unescapeHtmlEntities(input)).toBe(expected) + it("should preserve ampersand entities in commands", async () => { + mockToolUse.params.command = "echo foo && echo bar" + + await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, { + askApproval: mockAskApproval as unknown as AskApproval, + handleError: mockHandleError as unknown as HandleError, + pushToolResult: mockPushToolResult as unknown as PushToolResult, + removeClosingTag: mockRemoveClosingTag as unknown as RemoveClosingTag, + toolProtocol: "xml", + }) + + // Entities should be preserved, not converted to && + expect(mockAskApproval).toHaveBeenCalledWith("command", "echo foo && echo bar") }) }) - // Now we can run these tests describe("Basic functionality", () => { it("should execute a command normally", async () => { // Setup diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index fd791729b4d..85508785cef 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -5,7 +5,6 @@ import type { MockedFunction } from "vitest" import { fileExistsAtPath, createDirectoriesForFile } from "../../../utils/fs" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" -import { unescapeHtmlEntities } from "../../../utils/text-normalization" import { everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, ToolResponse } from "../../../shared/tools" import { writeToFileTool } from "../WriteToFileTool" @@ -47,10 +46,6 @@ vi.mock("../../../utils/path", () => ({ getReadablePath: vi.fn().mockReturnValue("test/path.txt"), })) -vi.mock("../../../utils/text-normalization", () => ({ - unescapeHtmlEntities: vi.fn().mockImplementation((content) => content), -})) - vi.mock("../../../integrations/misc/extract-text", () => ({ everyLineHasLineNumbers: vi.fn().mockReturnValue(false), stripLineNumbers: vi.fn().mockImplementation((content) => content), @@ -97,7 +92,6 @@ describe("writeToFileTool", () => { const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction const mockedGetReadablePath = getReadablePath as MockedFunction - const mockedUnescapeHtmlEntities = unescapeHtmlEntities as MockedFunction const mockedEveryLineHasLineNumbers = everyLineHasLineNumbers as MockedFunction const mockedStripLineNumbers = stripLineNumbers as MockedFunction const mockedPathResolve = path.resolve as MockedFunction @@ -117,7 +111,6 @@ describe("writeToFileTool", () => { mockedFileExistsAtPath.mockResolvedValue(false) mockedIsPathOutsideWorkspace.mockReturnValue(false) mockedGetReadablePath.mockReturnValue("test/path.txt") - mockedUnescapeHtmlEntities.mockImplementation((content) => content) mockedEveryLineHasLineNumbers.mockReturnValue(false) mockedStripLineNumbers.mockImplementation((content) => content) @@ -327,20 +320,26 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true) }) - it("unescapes HTML entities for non-Claude models", async () => { - mockCline.api.getModel.mockReturnValue({ id: "gpt-4" }) + it("preserves HTML entities in content (does not convert them to characters)", async () => { + // HTML entities should be preserved as-is to allow writing actual HTML entities to files + // This is important for HTML, JSX, TSX files that need to contain entities like ' + const contentWithEntities = "<div>John's & Jane's "test"</div>" - await executeWriteFileTool({ content: "<test>" }) + await executeWriteFileTool({ content: contentWithEntities }) - expect(mockedUnescapeHtmlEntities).toHaveBeenCalledWith("<test>") + // The content should be passed to the diff view unchanged - entities preserved + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentWithEntities, true) }) - it("skips HTML unescaping for Claude models", async () => { - mockCline.api.getModel.mockReturnValue({ id: "claude-3" }) + it("preserves HTML entities regardless of model type", async () => { + // Test with non-Claude model - entities should still be preserved + mockCline.api.getModel.mockReturnValue({ id: "gpt-4" }) + const contentWithEntities = "&<>"'" - await executeWriteFileTool({ content: "<test>" }) + await executeWriteFileTool({ content: contentWithEntities }) - expect(mockedUnescapeHtmlEntities).not.toHaveBeenCalled() + // Entities should be preserved, not converted to & < > " ' + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentWithEntities, true) }) it("strips line numbers from numbered content", async () => {