From 9fb232b499a097ebc0477d6b7a08583ef69da698 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sun, 3 May 2026 18:42:57 +0000 Subject: [PATCH] fix: remove HTML entity unescaping from tool content processing Removes unescapeHtmlEntities() calls from ApplyDiffTool, WriteToFileTool, and ExecuteCommandTool. Since all tool calls now use native JSON parsing, HTML entities in parsed content are intentional (e.g. Go code doing HTML escaping) rather than encoding artifacts. The lossy unescaping was causing apply_diff failures and file content corruption when working with code that legitimately contains HTML entities. Fixes #12264 --- src/core/tools/ApplyDiffTool.ts | 5 --- src/core/tools/ExecuteCommandTool.ts | 13 +++----- src/core/tools/WriteToFileTool.ts | 5 --- .../__tests__/executeCommandTool.spec.ts | 32 ------------------- .../tools/__tests__/writeToFileTool.spec.ts | 19 ++--------- 5 files changed, 6 insertions(+), 68 deletions(-) diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd22..b6b5ea47208 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" @@ -28,10 +27,6 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { const { askApproval, handleError, pushToolResult } = callbacks let { path: relPath, diff: diffContent } = params - if (diffContent && !task.api.getModel().id.includes("claude")) { - diffContent = unescapeHtmlEntities(diffContent) - } - try { if (!relPath) { task.consecutiveMistakeCount++ diff --git a/src/core/tools/ExecuteCommandTool.ts b/src/core/tools/ExecuteCommandTool.ts index 8fcb917b134..b9e1efbc8c6 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" @@ -53,9 +52,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) @@ -65,7 +62,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 @@ -88,9 +85,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { .get("commandTimeoutAllowlist", []) // Check if command matches any prefix in the allowlist - const isCommandAllowlisted = commandTimeoutAllowlist.some((prefix) => - canonicalCommand.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 @@ -100,7 +95,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> { const options: ExecuteCommandOptions = { executionId, - command: canonicalCommand, + command: 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__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index cd31430ab92..0cc9d7ab1a9 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 } from "../../../shared/tools" -import { unescapeHtmlEntities } from "../../../utils/text-normalization" // Mock dependencies vitest.mock("execa", () => ({ @@ -111,37 +110,6 @@ describe("executeCommandTool", () => { process.env.ROO_CLI_RUNTIME = originalCliRuntime }) - /** - * Tests for HTML entity unescaping in commands - * This verifies that HTML entities are properly converted to their actual characters - */ - describe("HTML entity unescaping", () => { - it("should unescape < to < character", () => { - const input = "echo <test>" - const expected = "echo " - expect(unescapeHtmlEntities(input)).toBe(expected) - }) - - it("should unescape > to > character", () => { - const input = "echo test > output.txt" - const expected = "echo test > output.txt" - expect(unescapeHtmlEntities(input)).toBe(expected) - }) - - it("should unescape & to & character", () => { - const input = "echo foo && echo bar" - const expected = "echo foo && echo bar" - expect(unescapeHtmlEntities(input)).toBe(expected) - }) - - 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) - }) - }) - - // 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..451e55c3ab6 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,12 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true) }) - it("unescapes HTML entities for non-Claude models", async () => { + it("preserves HTML entities in content (no unescaping)", async () => { mockCline.api.getModel.mockReturnValue({ id: "gpt-4" }) 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>" }) - - expect(mockedUnescapeHtmlEntities).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("<test>", true) }) it("strips line numbers from numbered content", async () => {