From 6d7c335ec3441edbd2bd9db8a60da280b1f26a27 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Fri, 18 Jul 2025 22:34:58 +0000 Subject: [PATCH] feat: add VSCode setting for HTML entity unescaping in diff tools - Add unescapeHtmlEntitiesInDiffs setting to global-settings schema - Modify applyDiffTool.ts to use setting instead of hardcoded behavior - Modify multiApplyDiffTool.ts to use setting instead of hardcoded behavior - Add comprehensive tests for setting-based behavior - Default setting to false to preserve current behavior - Only applies to non-Claude models when enabled Addresses feedback from @daniel-lxs in PR #5904 --- packages/types/src/global-settings.ts | 2 + .../__tests__/applyDiffHtmlEntity.spec.ts | 126 ++++++++++++++++++ src/core/tools/applyDiffTool.ts | 11 ++ src/core/tools/multiApplyDiffTool.ts | 14 +- 4 files changed, 151 insertions(+), 2 deletions(-) diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 514b15d783..44b6995d00 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -88,6 +88,7 @@ export const globalSettingsSchema = z.object({ rateLimitSeconds: z.number().optional(), diffEnabled: z.boolean().optional(), fuzzyMatchThreshold: z.number().optional(), + unescapeHtmlEntitiesInDiffs: z.boolean().optional(), experiments: experimentsSchema.optional(), codebaseIndexModels: codebaseIndexModelsSchema.optional(), @@ -224,6 +225,7 @@ export const EVALS_SETTINGS: RooCodeSettings = { diffEnabled: true, fuzzyMatchThreshold: 1, + unescapeHtmlEntitiesInDiffs: false, enableCheckpoints: false, diff --git a/src/core/tools/__tests__/applyDiffHtmlEntity.spec.ts b/src/core/tools/__tests__/applyDiffHtmlEntity.spec.ts index 7cc665cc6f..70d6005ed7 100644 --- a/src/core/tools/__tests__/applyDiffHtmlEntity.spec.ts +++ b/src/core/tools/__tests__/applyDiffHtmlEntity.spec.ts @@ -32,6 +32,13 @@ describe("HTML Entity Handling in apply_diff Tools", () => { api: { getModel: vi.fn().mockReturnValue({ id: "gpt-4" }), // Non-Claude model }, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + unescapeHtmlEntitiesInDiffs: false, // Default to false + }), + }), + }, diffStrategy: { applyDiff: vi.fn().mockResolvedValue({ success: true, @@ -256,4 +263,123 @@ describe("HTML Entity Handling in apply_diff Tools", () => { ) }) }) + + describe("Setting-based HTML entity unescaping", () => { + it("should unescape HTML entities when setting is enabled for non-Claude models", async () => { + // Enable the setting + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + unescapeHtmlEntitiesInDiffs: true, + }), + }) + + const diffContent = `\<<<<<<< SEARCH +------- +// Comment with & entity +======= +// Comment with & 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 unescaped content + const actualDiffContent = mockCline.diffStrategy.applyDiff.mock.calls[0][1] + expect(actualDiffContent).toContain("// Comment with & entity") + expect(actualDiffContent).toContain("// Comment with & entity updated") + }) + + it("should not unescape HTML entities when setting is disabled for non-Claude models", async () => { + // Disable the setting (default behavior) + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + unescapeHtmlEntitiesInDiffs: false, + }), + }) + + const diffContent = `\<<<<<<< SEARCH +------- +// Comment with & entity +======= +// Comment with & 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 original content (not unescaped) + const actualDiffContent = mockCline.diffStrategy.applyDiff.mock.calls[0][1] + expect(actualDiffContent).toContain("// Comment with & entity") + expect(actualDiffContent).toContain("// Comment with & entity updated") + }) + + it("should never unescape HTML entities for Claude models regardless of setting", async () => { + // Enable the setting + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + unescapeHtmlEntitiesInDiffs: true, + }), + }) + + // Set up Claude model + mockCline.api.getModel.mockReturnValue({ id: "claude-3-sonnet" }) + + const diffContent = `\<<<<<<< SEARCH +------- +// Comment with & entity +======= +// Comment with & 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 original content (not unescaped) + const actualDiffContent = mockCline.diffStrategy.applyDiff.mock.calls[0][1] + expect(actualDiffContent).toContain("// Comment with & entity") + expect(actualDiffContent).toContain("// Comment with & entity updated") + }) + }) }) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index f259be0c52..83aa20bcfc 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -10,6 +10,7 @@ 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, @@ -22,6 +23,16 @@ export async function applyDiffToolLegacy( const relPath: string | undefined = block.params.path let diffContent: string | undefined = block.params.diff + // Apply HTML entity unescaping based on user setting + if (diffContent) { + const state = (await cline.providerRef.deref()?.getState()) ?? {} + const unescapeHtmlEntitiesInDiffs = (state as any).unescapeHtmlEntitiesInDiffs ?? false + + if (unescapeHtmlEntitiesInDiffs && !cline.api.getModel().id.includes("claude")) { + diffContent = unescapeHtmlEntities(diffContent) + } + } + const sharedMessageProps: ClineSayTool = { tool: "appliedDiff", path: getReadablePath(cline.cwd, removeClosingTag("path", relPath)), diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index 31e4587d23..7584767cf5 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -10,6 +10,7 @@ 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" @@ -409,8 +410,17 @@ Original error: ${errorMessage}` let successCount = 0 let formattedError = "" - // Use diff items as-is without HTML entity unescaping to prevent search mismatches - const processedDiffItems = diffItems + // Apply HTML entity unescaping based on user setting + let processedDiffItems = diffItems + const state = (await cline.providerRef.deref()?.getState()) ?? {} + const unescapeHtmlEntitiesInDiffs = (state as any).unescapeHtmlEntitiesInDiffs ?? false + + if (unescapeHtmlEntitiesInDiffs && !cline.api.getModel().id.includes("claude")) { + processedDiffItems = diffItems.map((item) => ({ + ...item, + content: item.content ? unescapeHtmlEntities(item.content) : item.content, + })) + } // Apply all diffs at once with the array-based method const diffResult = (await cline.diffStrategy?.applyDiff(originalContent, processedDiffItems)) ?? {