fix: remove HTML entity unescaping from apply_diff tools (#5633)

- Remove HTML entity unescaping logic from applyDiffTool.ts for non-Claude models
- Remove HTML entity unescaping logic from multiApplyDiffTool.ts for non-Claude models
- Remove unused imports of unescapeHtmlEntities from both files
- Add comprehensive test coverage for HTML entity handling scenarios
- Preserve HTML entities in search content to ensure exact matching with file content
- Fix search failures and unintended content modifications caused by entity unescaping

Fixes #5633

Author: roomote

src/core/tools/__tests__/applyDiffHtmlEntity.spec.ts

@@ -0,0 +1,259 @@
+import { describe, it, expect, vi, beforeEach } from "vitest"
+import { applyDiffToolLegacy } from "../applyDiffTool"
+import fs from "fs/promises"
+
+// Mock dependencies
+vi.mock("fs/promises")
+vi.mock("../../../utils/fs", () => ({
+	fileExistsAtPath: vi.fn().mockResolvedValue(true),
+}))
+
+vi.mock("../../../utils/path", () => ({
+	getReadablePath: vi.fn((cwd, relPath) => relPath),
+}))
+
+describe("HTML Entity Handling in apply_diff Tools", () => {
+	let mockCline: any
+	let mockBlock: any
+	let mockAskApproval: any
+	let mockHandleError: any
+	let mockPushToolResult: any
+	let mockRemoveClosingTag: any
+
+	beforeEach(() => {
+		vi.clearAllMocks()
+
+		// Mock file system
+		const mockReadFile = vi.mocked(fs.readFile)
+		mockReadFile.mockResolvedValue("// Comment with & entity\nconst value = 'test';")
+
+		mockCline = {
+			cwd: "/test",
+			api: {
+				getModel: vi.fn().mockReturnValue({ id: "gpt-4" }), // Non-Claude model
+			},
+			diffStrategy: {
+				applyDiff: vi.fn().mockResolvedValue({
+					success: true,
+					content: "// Comment with & entity\nconst value = 'updated';",
+				}),
+				getProgressStatus: vi.fn(),
+			},
+			diffViewProvider: {
+				editType: "",
+				open: vi.fn(),
+				update: vi.fn(),
+				scrollToFirstDiff: vi.fn(),
+				saveChanges: vi.fn(),
+				pushToolWriteResult: vi.fn().mockResolvedValue("File updated successfully"),
+				reset: vi.fn(),
+				revertChanges: vi.fn(),
+			},
+			fileContextTracker: {
+				trackFileContext: vi.fn(),
+			},
+			rooIgnoreController: {
+				validateAccess: vi.fn().mockReturnValue(true),
+			},
+			rooProtectedController: {
+				isWriteProtected: vi.fn().mockReturnValue(false),
+			},
+			consecutiveMistakeCount: 0,
+			consecutiveMistakeCountForApplyDiff: new Map(),
+			didEditFile: false,
+			ask: vi.fn(),
+			say: vi.fn(),
+			recordToolError: vi.fn(),
+			sayAndCreateMissingParamError: vi.fn(),
+		}
+
+		mockAskApproval = vi.fn().mockResolvedValue(true)
+		mockHandleError = vi.fn()
+		mockPushToolResult = vi.fn()
+		mockRemoveClosingTag = vi.fn((tag, value) => value)
+	})
+
+	describe("Legacy apply_diff tool", () => {
+		it("should not unescape HTML entities in diff content for non-Claude models", async () => {
+			const diffContent = `<<<<<<< SEARCH
+:start_line:1
+-------
+// Comment with &amp; entity
+=======
+// Comment with &amp; entity updated
+>>>>>>> REPLACE`
+
+			mockBlock = {
+				params: {
+					path: "test.js",
+					diff: diffContent,
+				},
+				partial: false,
+			}
+
+			await applyDiffToolLegacy(
+				mockCline,
+				mockBlock,
+				mockAskApproval,
+				mockHandleError,
+				mockPushToolResult,
+				mockRemoveClosingTag,
+			)
+
+			// Verify that diffStrategy.applyDiff was called with the original diff content (not unescaped)
+			expect(mockCline.diffStrategy.applyDiff).toHaveBeenCalledWith(
+				"// Comment with &amp; entity\nconst value = 'test';",
+				diffContent,
+				NaN, // parseInt of undefined start_line
+			)
+		})
+
+		it("should handle files containing various HTML entities without unescaping search content", async () => {
+			const fileContent = `<div>Hello &amp; welcome to &lt;our&gt; site!</div>
+<p>Don&apos;t forget to check &quot;special offers&quot;</p>`
+
+			const mockReadFile = vi.mocked(fs.readFile)
+			mockReadFile.mockResolvedValue(fileContent)
+
+			const diffContent = `<<<<<<< SEARCH
+:start_line:1
+-------
+<div>Hello &amp; welcome to &lt;our&gt; site!</div>
+=======
+<div>Hello &amp; welcome to &lt;our updated&gt; site!</div>
+>>>>>>> REPLACE`
+
+			mockBlock = {
+				params: {
+					path: "test.html",
+					diff: diffContent,
+				},
+				partial: false,
+			}
+
+			await applyDiffToolLegacy(
+				mockCline,
+				mockBlock,
+				mockAskApproval,
+				mockHandleError,
+				mockPushToolResult,
+				mockRemoveClosingTag,
+			)
+
+			// Verify the search content was not unescaped
+			expect(mockCline.diffStrategy.applyDiff).toHaveBeenCalledWith(
+				fileContent,
+				expect.stringContaining("&amp; welcome to &lt;our&gt;"),
+				NaN,
+			)
+		})
+
+		it("should preserve HTML entities in both search and replace content", async () => {
+			const diffContent = `<<<<<<< SEARCH
+:start_line:1
+-------
+// Step 5 &amp; 6: Find and validate
+=======
+// Step 5 &amp; 6: Find, validate &amp; process
+>>>>>>> REPLACE`
+
+			mockBlock = {
+				params: {
+					path: "test.js",
+					diff: diffContent,
+				},
+				partial: false,
+			}
+
+			await applyDiffToolLegacy(
+				mockCline,
+				mockBlock,
+				mockAskApproval,
+				mockHandleError,
+				mockPushToolResult,
+				mockRemoveClosingTag,
+			)
+
+			const actualDiffContent = mockCline.diffStrategy.applyDiff.mock.calls[0][1]
+			expect(actualDiffContent).toContain("Step 5 &amp; 6: Find and validate")
+			expect(actualDiffContent).toContain("Step 5 &amp; 6: Find, validate &amp; process")
+		})
+
+		it("should handle apostrophe entities correctly", async () => {
+			const fileContent = "// Don&apos;t modify this comment"
+			const mockReadFile = vi.mocked(fs.readFile)
+			mockReadFile.mockResolvedValue(fileContent)
+
+			const diffContent = `<<<<<<< SEARCH
+:start_line:1
+-------
+// Don&apos;t modify this comment
+=======
+// Don&apos;t modify this updated comment
+>>>>>>> REPLACE`
+
+			mockBlock = {
+				params: {
+					path: "test.js",
+					diff: diffContent,
+				},
+				partial: false,
+			}
+
+			await applyDiffToolLegacy(
+				mockCline,
+				mockBlock,
+				mockAskApproval,
+				mockHandleError,
+				mockPushToolResult,
+				mockRemoveClosingTag,
+			)
+
+			// Verify apostrophe entities are preserved
+			const actualDiffContent = mockCline.diffStrategy.applyDiff.mock.calls[0][1]
+			expect(actualDiffContent).toContain("Don&apos;t modify this comment")
+			expect(actualDiffContent).toContain("Don&apos;t modify this updated comment")
+		})
+	})
+
+	describe("Claude model behavior", () => {
+		beforeEach(() => {
+			// Set up Claude model
+			mockCline.api.getModel.mockReturnValue({ id: "claude-3-sonnet" })
+		})
+
+		it("should not unescape HTML entities for Claude models (no change in behavior)", async () => {
+			const diffContent = `<<<<<<< SEARCH
+:start_line:1
+-------
+// Comment with &amp; entity
+=======
+// Comment with &amp; entity updated
+>>>>>>> REPLACE`
+
+			mockBlock = {
+				params: {
+					path: "test.js",
+					diff: diffContent,
+				},
+				partial: false,
+			}
+
+			await applyDiffToolLegacy(
+				mockCline,
+				mockBlock,
+				mockAskApproval,
+				mockHandleError,
+				mockPushToolResult,
+				mockRemoveClosingTag,
+			)
+
+			// Verify that diffStrategy.applyDiff was called with the original diff content
+			expect(mockCline.diffStrategy.applyDiff).toHaveBeenCalledWith(
+				"// Comment with &amp; entity\nconst value = 'test';",
+				diffContent,
+				NaN,
+			)
+		})
+	})
+})

src/core/tools/applyDiffTool.ts

@@ -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"
 
 export async function applyDiffToolLegacy(
 	cline: Task,
@@ -23,10 +22,6 @@ export async function applyDiffToolLegacy(
 	const relPath: string | undefined = block.params.path
 	let diffContent: string | undefined = block.params.diff
 
-	if (diffContent && !cline.api.getModel().id.includes("claude")) {
-		diffContent = unescapeHtmlEntities(diffContent)
-	}
-
 	const sharedMessageProps: ClineSayTool = {
 		tool: "appliedDiff",
 		path: getReadablePath(cline.cwd, removeClosingTag("path", relPath)),

src/core/tools/multiApplyDiffTool.ts

@@ -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 { parseXml } from "../../utils/xml"
 import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
 import { applyDiffToolLegacy } from "./applyDiffTool"
@@ -410,13 +409,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 as-is without HTML entity unescaping to prevent search mismatches
+				const processedDiffItems = diffItems
 
 				// Apply all diffs at once with the array-based method
 				const diffResult = (await cline.diffStrategy?.applyDiff(originalContent, processedDiffItems)) ?? {