scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-06 08:18:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: preserve HTML entities in file content and commands

Browse source 
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
This commit is contained in:
Roo Code 2026-01-17 15:58:13 +00:00
parent 6608ed618a
commit 3ec43719ad
7 changed files with 52 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/core/tools/ApplyDiffTool.ts
View file

@ -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<void> {
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) {

10
src/core/tools/ExecuteCommandTool.ts
View file

@ -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<string[]>("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,

19
src/core/tools/MultiApplyDiffTool.ts
View file

@ -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)) ?? {

5
src/core/tools/WriteToFileTool.ts
View file

@ -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)

3
src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts
View file

@ -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",

53
src/core/tools/__tests__/executeCommandTool.spec.ts
View file

@ -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 &lt; to < character", () => {
const input = "echo &lt;test&gt;"
const expected = "echo <test>"
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 &lt;test&gt;"
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",
})
// The command should be passed through unchanged - entities preserved
expect(mockAskApproval).toHaveBeenCalledWith("command", "echo &lt;test&gt;")
})
it("should unescape &gt; to > character", () => {
const input = "echo test &gt; output.txt"
const expected = "echo test > output.txt"
expect(unescapeHtmlEntities(input)).toBe(expected)
})
it("should preserve ampersand entities in commands", async () => {
mockToolUse.params.command = "echo foo &amp;&amp; echo bar"
it("should unescape &amp; to & character", () => {
const input = "echo foo &amp;&amp; echo bar"
const expected = "echo foo && echo bar"
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 handle multiple mixed HTML entities", () => {
const input = "grep -E 'pattern' &lt;file.txt &gt;output.txt 2&gt;&amp;1"
const expected = "grep -E 'pattern' <file.txt >output.txt 2>&1"
expect(unescapeHtmlEntities(input)).toBe(expected)
// Entities should be preserved, not converted to &&
expect(mockAskApproval).toHaveBeenCalledWith("command", "echo foo &amp;&amp; echo bar")
})
})
// Now we can run these tests
describe("Basic functionality", () => {
it("should execute a command normally", async () => {
// Setup

29
src/core/tools/__tests__/writeToFileTool.spec.ts
View file

@ -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<typeof createDirectoriesForFile>
const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction<typeof isPathOutsideWorkspace>
const mockedGetReadablePath = getReadablePath as MockedFunction<typeof getReadablePath>
const mockedUnescapeHtmlEntities = unescapeHtmlEntities as MockedFunction<typeof unescapeHtmlEntities>
const mockedEveryLineHasLineNumbers = everyLineHasLineNumbers as MockedFunction<typeof everyLineHasLineNumbers>
const mockedStripLineNumbers = stripLineNumbers as MockedFunction<typeof stripLineNumbers>
const mockedPathResolve = path.resolve as MockedFunction<typeof path.resolve>
@ -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 &apos;
const contentWithEntities = "&lt;div&gt;John&apos;s &amp; Jane&apos;s &quot;test&quot;&lt;/div&gt;"
await executeWriteFileTool({ content: "&lt;test&gt;" })
await executeWriteFileTool({ content: contentWithEntities })
expect(mockedUnescapeHtmlEntities).toHaveBeenCalledWith("&lt;test&gt;")
// 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 = "&amp;&lt;&gt;&quot;&apos;"
await executeWriteFileTool({ content: "&lt;test&gt;" })
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 () => {