From 6609a39b65cb0d0f37d734037ddd93e28eb3c7ef Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 25 Nov 2025 06:17:11 +0000 Subject: [PATCH] fix: preserve HTML entities in apply_diff to avoid false identical content errors Removed HTML entity unescaping for non-Claude models in ApplyDiffTool This fixes the issue where HTML entities were incorrectly identified as identical to their decoded characters Added comprehensive test coverage for HTML entity preservation Resolves issue #9563 where JSX files with escaped entities failed to update --- src/core/tools/ApplyDiffTool.ts | 7 +- .../applyDiffTool.htmlentities.spec.ts | 206 ++++++++++++++++++ 2 files changed, 207 insertions(+), 6 deletions(-) create mode 100644 src/core/tools/__tests__/applyDiffTool.htmlentities.spec.ts diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index c5ad24bca3..13d56bcbb0 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -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" @@ -33,11 +32,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { async execute(params: ApplyDiffParams, task: Task, callbacks: ToolCallbacks): Promise { 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) { diff --git a/src/core/tools/__tests__/applyDiffTool.htmlentities.spec.ts b/src/core/tools/__tests__/applyDiffTool.htmlentities.spec.ts new file mode 100644 index 0000000000..91aa0969db --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.htmlentities.spec.ts @@ -0,0 +1,206 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import path from "path" +import fs from "fs/promises" +import { ApplyDiffTool } from "../ApplyDiffTool" +import { Task } from "../../task/Task" +import { fileExistsAtPath } from "../../../utils/fs" + +vi.mock("fs/promises") +vi.mock("../../../utils/fs") +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureDiffApplicationError: vi.fn(), + }, + }, +})) + +describe("ApplyDiffTool - HTML Entity Handling", () => { + let applyDiffTool: ApplyDiffTool + let mockTask: any + let mockCallbacks: any + + beforeEach(() => { + applyDiffTool = new ApplyDiffTool() + + // Mock task with all required properties + mockTask = { + cwd: "/test", + api: { + getModel: vi.fn().mockReturnValue({ id: "test-model" }), + }, + diffStrategy: { + applyDiff: vi.fn(), + }, + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + }, + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + say: vi.fn(), + sayAndCreateMissingParamError: vi.fn(), + diffViewProvider: { + editType: "", + originalContent: "", + open: vi.fn(), + update: vi.fn(), + scrollToFirstDiff: vi.fn(), + revertChanges: vi.fn(), + saveChanges: vi.fn(), + saveDirectly: vi.fn(), + pushToolWriteResult: vi.fn(), + reset: vi.fn(), + }, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 100, + experiments: {}, + }), + }), + }, + fileContextTracker: { + trackFileContext: vi.fn(), + }, + didEditFile: false, + processQueuedMessages: vi.fn(), + rooProtectedController: undefined, + } + + mockCallbacks = { + askApproval: vi.fn().mockResolvedValue(true), + handleError: vi.fn(), + pushToolResult: vi.fn(), + toolProtocol: "xml", + } + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue("This doesn't change" as any) + }) + + it("should preserve HTML entities in diff content and not unescape them", async () => { + const params = { + path: "test.jsx", + diff: `<<<<<<< SEARCH +:start_line:1 +------- +This doesn't change +======= +This doesn't change +>>>>>>> REPLACE`, + } + + // Mock successful diff application + mockTask.diffStrategy.applyDiff.mockResolvedValue({ + success: true, + content: "This doesn't change", + }) + + await applyDiffTool.execute(params, mockTask, mockCallbacks) + + // Verify that applyDiff was called with the original diff content (not unescaped) + expect(mockTask.diffStrategy.applyDiff).toHaveBeenCalledWith("This doesn't change", params.diff, 1) + + // Verify the diff content was not modified (no unescaping happened) + const callArgs = mockTask.diffStrategy.applyDiff.mock.calls[0] + // The diff should contain the HTML entity in the REPLACE section + expect(callArgs[1]).toContain("'") + // The entire diff string should match exactly what was passed in + expect(callArgs[1]).toBe(params.diff) + }) + + it("should correctly identify different content when HTML entities are used", async () => { + const params = { + path: "test.jsx", + diff: `<<<<<<< SEARCH +:start_line:1 +------- +This doesn't change +======= +This doesn't change +>>>>>>> REPLACE`, + } + + // The diff strategy should recognize these as different + mockTask.diffStrategy.applyDiff.mockResolvedValue({ + success: true, + content: "This doesn't change", + }) + + await applyDiffTool.execute(params, mockTask, mockCallbacks) + + expect(mockTask.diffStrategy.applyDiff).toHaveBeenCalledTimes(1) + expect(mockCallbacks.pushToolResult).toHaveBeenCalled() + expect(mockTask.consecutiveMistakeCount).toBe(0) // No errors should occur + }) + + it("should handle multiple HTML entities correctly", async () => { + const params = { + path: "test.jsx", + diff: `<<<<<<< SEARCH +:start_line:1 +------- +
It's "quoted" & special
+======= +
It's "quoted" & special
+>>>>>>> REPLACE`, + } + + vi.mocked(fs.readFile).mockResolvedValue(`
It's "quoted" & special
` as any) + + mockTask.diffStrategy.applyDiff.mockResolvedValue({ + success: true, + content: `
It's "quoted" & special
`, + }) + + await applyDiffTool.execute(params, mockTask, mockCallbacks) + + // Verify entities are preserved in the diff + const callArgs = mockTask.diffStrategy.applyDiff.mock.calls[0] + expect(callArgs[1]).toContain("'") + expect(callArgs[1]).toContain(""") + expect(callArgs[1]).toContain("&") + }) + + it("should work for both Claude and non-Claude models", async () => { + const params = { + path: "test.jsx", + diff: `<<<<<<< SEARCH +:start_line:1 +------- +This doesn't change +======= +This doesn't change +>>>>>>> REPLACE`, + } + + // Test with non-Claude model + mockTask.api.getModel.mockReturnValue({ id: "gpt-4" }) + mockTask.diffStrategy.applyDiff.mockResolvedValue({ + success: true, + content: "This doesn't change", + }) + + await applyDiffTool.execute(params, mockTask, mockCallbacks) + + let callArgs = mockTask.diffStrategy.applyDiff.mock.calls[0] + expect(callArgs[1]).toContain("'") + + // Reset and test with Claude model + vi.clearAllMocks() + mockTask.api.getModel.mockReturnValue({ id: "claude-3-opus" }) + mockTask.diffStrategy.applyDiff.mockResolvedValue({ + success: true, + content: "This doesn't change", + }) + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue("This doesn't change" as any) + + await applyDiffTool.execute(params, mockTask, mockCallbacks) + + callArgs = mockTask.diffStrategy.applyDiff.mock.calls[0] + expect(callArgs[1]).toContain("'") + }) +})