From b5c9f108ec0d1a7f840e94a84939bdf67db8ae6d Mon Sep 17 00:00:00 2001 From: 0xMink Date: Tue, 10 Feb 2026 21:35:45 -0500 Subject: [PATCH] fix(data): stop converting HTML entities to literal characters in file writes, diffs, and commands --- src/core/tools/ApplyDiffTool.ts | 7 +-- src/core/tools/ExecuteCommandTool.ts | 11 ++--- src/core/tools/WriteToFileTool.ts | 5 -- .../executeCommandTimeout.integration.spec.ts | 3 -- .../__tests__/executeCommandTool.spec.ts | 47 +++++++++---------- .../tools/__tests__/writeToFileTool.spec.ts | 22 ++------- 6 files changed, 32 insertions(+), 63 deletions(-) diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 5ca7002ff2d..89a4457d939 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" @@ -26,11 +25,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { async execute(params: ApplyDiffParams, task: Task, callbacks: ToolCallbacks): Promise { const { askApproval, handleError, pushToolResult } = 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 ef1370202e1..0d1cf1b8cea 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" @@ -43,9 +42,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { return } - const canonicalCommand = unescapeHtmlEntities(command) - - const ignoredFileAttemptedToAccess = task.rooIgnoreController?.validateCommand(canonicalCommand) + const ignoredFileAttemptedToAccess = task.rooIgnoreController?.validateCommand(command) if (ignoredFileAttemptedToAccess) { await task.say("rooignore_error", ignoredFileAttemptedToAccess) @@ -55,7 +52,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { task.consecutiveMistakeCount = 0 - const didApprove = await askApproval("command", canonicalCommand) + const didApprove = await askApproval("command", command) if (!didApprove) { return @@ -79,7 +76,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { // Check if command matches any prefix in the allowlist const isCommandAllowlisted = commandTimeoutAllowlist.some((prefix) => - canonicalCommand.startsWith(prefix.trim()), + command.startsWith(prefix.trim()), ) // Convert seconds to milliseconds for internal use, but skip timeout if command is allowlisted @@ -87,7 +84,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { const options: ExecuteCommandOptions = { executionId, - command: canonicalCommand, + command, customCwd, terminalShellIntegrationDisabled, commandExecutionTimeout, diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c8455ef3d97..0a9db56cdb7 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" @@ -81,10 +80,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, 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 bd13439ea78..be684b92ed6 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 89b2575288b..7c5ac11dd88 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -6,8 +6,6 @@ import * as vscode from "vscode" import { Task } from "../../task/Task" import { formatResponse } from "../../prompts/responses" import { ToolUse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" -import { unescapeHtmlEntities } from "../../../utils/text-normalization" - // Mock dependencies vitest.mock("execa", () => ({ execa: vitest.fn(), @@ -107,36 +105,37 @@ 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 */ - 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 () => { + mockToolUse.params.command = "echo <test>" + mockToolUse.nativeArgs = { 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, + }) - it("should unescape & to & character", () => { - const input = "echo foo && echo bar" - const expected = "echo foo && echo bar" - expect(unescapeHtmlEntities(input)).toBe(expected) + 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" + mockToolUse.nativeArgs = { 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, + }) + + 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 6c63387ee10..c38e2b5cb7c 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 @@ -116,7 +110,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,13 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true) }) - it("unescapes HTML entities for non-Claude models", async () => { + it("preserves HTML entities in content regardless of model", async () => { mockCline.api.getModel.mockReturnValue({ id: "gpt-4" }) + const contentWithEntities = "<div>John's & Jane's "test"</div>" - await executeWriteFileTool({ content: "<test>" }) - - expect(mockedUnescapeHtmlEntities).toHaveBeenCalledWith("<test>") - }) - - it("skips HTML unescaping for Claude models", async () => { - mockCline.api.getModel.mockReturnValue({ id: "claude-3" }) - - await executeWriteFileTool({ content: "<test>" }) + await executeWriteFileTool({ content: contentWithEntities }) - expect(mockedUnescapeHtmlEntities).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentWithEntities, true) }) it("strips line numbers from numbered content", async () => {