mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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
This commit is contained in:
parent
7f0b8e75dd
commit
6d7c335ec3
4 changed files with 151 additions and 2 deletions
|
|
@ -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,
|
||||
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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)),
|
||||
|
|
|
|||
|
|
@ -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)) ?? {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue