mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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
This commit is contained in:
parent
ad25634905
commit
9fb232b499
5 changed files with 6 additions and 68 deletions
|
|
@ -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++
|
||||
|
|
|
|||
|
|
@ -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<string[]>("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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <test>"
|
||||
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' <file.txt >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
|
||||
|
|
|
|||
|
|
@ -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>
|
||||
|
|
@ -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 () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue