mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
fix: remove HTML entity unescaping from all tool implementations - Fixes #9563
This commit is contained in:
parent
78dc34498b
commit
1ef31a75be
6 changed files with 7 additions and 85 deletions
|
|
@ -10,7 +10,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 { BaseTool, ToolCallbacks } from "./BaseTool"
|
||||
|
|
@ -35,10 +34,6 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
|
|||
const { askApproval, handleError, pushToolResult, toolProtocol } = 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"
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -11,7 +11,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"
|
||||
|
|
@ -204,7 +203,7 @@ Original error: ${errorMessage}`
|
|||
path: legacyPath,
|
||||
diff: [
|
||||
{
|
||||
content: legacyDiffContent, // Unescaping will be handled later like new diffs
|
||||
content: legacyDiffContent,
|
||||
startLine: legacyStartLineStr ? parseInt(legacyStartLineStr) : undefined,
|
||||
},
|
||||
],
|
||||
|
|
@ -322,12 +321,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)
|
||||
|
|
@ -484,12 +478,8 @@ Original error: ${errorMessage}`
|
|||
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)) ?? {
|
||||
|
|
|
|||
|
|
@ -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 { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
|
||||
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
|
||||
import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats"
|
||||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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", () => ({
|
||||
|
|
@ -105,36 +104,6 @@ describe("executeCommandTool", () => {
|
|||
}
|
||||
})
|
||||
|
||||
/**
|
||||
* 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 () => {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
@ -322,22 +315,6 @@ describe("writeToFileTool", () => {
|
|||
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true)
|
||||
})
|
||||
|
||||
it("unescapes HTML entities for non-Claude models", 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()
|
||||
})
|
||||
|
||||
it("strips line numbers from numbered content", async () => {
|
||||
const contentWithLineNumbers = "1 | line one\n2 | line two"
|
||||
mockedEveryLineHasLineNumbers.mockReturnValue(true)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue