From 3ec43719ad093c76df05b515259d100990e1dd06 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 17 Jan 2026 15:58:13 +0000 Subject: [PATCH] fix: preserve HTML entities in file content and commands Removes unescapeHtmlEntities function calls from all tool files. This function was converting HTML entities to their character equivalents, breaking files that needed actual HTML entities. - Remove unescapeHtmlEntities from WriteToFileTool.ts - Remove unescapeHtmlEntities from ApplyDiffTool.ts - Remove unescapeHtmlEntities from ExecuteCommandTool.ts - Remove unescapeHtmlEntities from MultiApplyDiffTool.ts - Add tests verifying HTML entities are preserved - Remove obsolete text-normalization mocks Fixes #10804 --- src/core/tools/ApplyDiffTool.ts | 7 +-- src/core/tools/ExecuteCommandTool.ts | 10 ++-- src/core/tools/MultiApplyDiffTool.ts | 19 ++----- src/core/tools/WriteToFileTool.ts | 5 -- .../executeCommandTimeout.integration.spec.ts | 3 -- .../__tests__/executeCommandTool.spec.ts | 53 +++++++++++-------- .../tools/__tests__/writeToFileTool.spec.ts | 29 +++++----- 7 files changed, 52 insertions(+), 74 deletions(-) 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 () => {