From 6540f2be5cf921eafafb329e080e7288563a8541 Mon Sep 17 00:00:00 2001 From: "roomote[bot]" <219738659+roomote[bot]@users.noreply.github.com> Date: Thu, 14 Aug 2025 17:46:52 -0400 Subject: [PATCH] fix: prevent XML entity decoding in diff tools (#7107) (#7108) * fix: prevent XML entity decoding in diff tools - Add parseXmlForDiff function with processEntities: false to preserve exact content - Update multiApplyDiffTool to use parseXmlForDiff instead of parseXml - Add comprehensive tests for entity handling in parseXmlForDiff This fixes the issue where fast-xml-parser was decoding HTML entities like & causing mismatches in diff tools when comparing against original file content. Fixes #7107 * refactor: eliminate code duplication between parseXml and parseXmlForDiff - Refactored parseXml to accept optional ParseXmlOptions parameter - parseXmlForDiff now delegates to parseXml with processEntities: false - Added explanatory comment in multiApplyDiffTool.ts about why parseXmlForDiff is used - Improved JSDoc documentation with specific use cases for parseXmlForDiff This maintains backward compatibility while eliminating code duplication. parseXml continues to be used for general XML parsing (file reads, follow-up questions), while parseXmlForDiff is specifically for diff operations where entity processing must be disabled. --------- Co-authored-by: Roo Code Co-authored-by: daniel-lxs --- src/core/tools/multiApplyDiffTool.ts | 7 +- src/utils/__tests__/xml.spec.ts | 125 ++++++++++++++++++++++++++- src/utils/xml.ts | 38 +++++++- 3 files changed, 166 insertions(+), 4 deletions(-) diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index db514d2b64..4cce2d1777 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -12,7 +12,7 @@ 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 { parseXmlForDiff } from "../../utils/xml" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { applyDiffToolLegacy } from "./applyDiffTool" @@ -108,7 +108,10 @@ export async function applyDiffTool( if (argsXmlTag) { // Parse file entries from XML (new way) try { - const parsed = parseXml(argsXmlTag, ["file.diff.content"]) as ParsedXmlResult + // IMPORTANT: We use parseXmlForDiff here instead of parseXml to prevent HTML entity decoding + // This ensures exact character matching when comparing parsed content against original file content + // Without this, special characters like & would be decoded to & causing diff mismatches + const parsed = parseXmlForDiff(argsXmlTag, ["file.diff.content"]) as ParsedXmlResult const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean) for (const file of files) { diff --git a/src/utils/__tests__/xml.spec.ts b/src/utils/__tests__/xml.spec.ts index 0e43cf04a1..f7a282b0c0 100644 --- a/src/utils/__tests__/xml.spec.ts +++ b/src/utils/__tests__/xml.spec.ts @@ -1,4 +1,4 @@ -import { parseXml } from "../xml" +import { parseXml, parseXmlForDiff } from "../xml" describe("parseXml", () => { describe("type conversion", () => { @@ -115,3 +115,126 @@ describe("parseXml", () => { }) }) }) + +describe("parseXmlForDiff", () => { + describe("HTML entity handling", () => { + it("should NOT decode HTML entities like &", () => { + const xml = ` + + Team Identity & Project Positioning + + ` + + const result = parseXmlForDiff(xml) as any + + // The & should remain as-is, not be decoded to & + expect(result.root.content).toBe("Team Identity & Project Positioning") + }) + + it("should preserve & character without encoding", () => { + const xml = ` + + Team Identity & Project Positioning + + ` + + const result = parseXmlForDiff(xml) as any + + // The & should remain as-is + expect(result.root.content).toBe("Team Identity & Project Positioning") + }) + + it("should NOT decode other HTML entities", () => { + const xml = ` + + <div> "Hello" 'World' + + ` + + const result = parseXmlForDiff(xml) as any + + // All HTML entities should remain as-is + expect(result.root.content).toBe("<div> "Hello" 'World'") + }) + + it("should handle mixed content with entities correctly", () => { + const xml = ` + + if (a < b && c > d) { return "test"; } + + ` + + const result = parseXmlForDiff(xml) as any + + // All entities should remain unchanged + expect(result.root.code).toBe("if (a < b && c > d) { return "test"; }") + }) + }) + + describe("basic functionality (same as parseXml)", () => { + it("should correctly parse a simple XML string", () => { + const xml = ` + + Test Name + Some description + + ` + + const result = parseXmlForDiff(xml) as any + + expect(result).toHaveProperty("root") + expect(result.root).toHaveProperty("name", "Test Name") + expect(result.root).toHaveProperty("description", "Some description") + }) + + it("should handle attributes correctly", () => { + const xml = ` + + Item content + + ` + + const result = parseXmlForDiff(xml) as any + + expect(result.root.item).toHaveProperty("@_id", "1") + expect(result.root.item).toHaveProperty("@_category", "test") + expect(result.root.item).toHaveProperty("#text", "Item content") + }) + + it("should support stopNodes parameter", () => { + const xml = ` + + + Should not parse this + + + ` + + const result = parseXmlForDiff(xml, ["nestedXml"]) as any + + expect(result.root.data.nestedXml).toBeTruthy() + expect(result.root.data.nestedXml).toHaveProperty("item", "Should not parse this") + }) + }) + + describe("diff-specific use case", () => { + it("should preserve exact content for diff matching", () => { + // This simulates the actual use case from the issue + const xml = ` + + + ./doc.md + + Team Identity & Project Positioning + + + + ` + + const result = parseXmlForDiff(xml, ["file.diff.content"]) as any + + // The & should remain as-is for exact matching with file content + expect(result.args.file.diff.content).toBe("Team Identity & Project Positioning") + }) + }) +}) diff --git a/src/utils/xml.ts b/src/utils/xml.ts index 0fd6ef574c..f183309d49 100644 --- a/src/utils/xml.ts +++ b/src/utils/xml.ts @@ -1,13 +1,28 @@ import { XMLParser } from "fast-xml-parser" +/** + * Options for XML parsing + */ +interface ParseXmlOptions { + /** + * Whether to process HTML entities (e.g., & to &). + * Default: true for general parsing, false for diff operations + */ + processEntities?: boolean +} + /** * Parses an XML string into a JavaScript object * @param xmlString The XML string to parse + * @param stopNodes Optional array of node names to stop parsing at + * @param options Optional parsing options * @returns Parsed JavaScript object representation of the XML * @throws Error if the XML is invalid or parsing fails */ -export function parseXml(xmlString: string, stopNodes?: string[]): unknown { +export function parseXml(xmlString: string, stopNodes?: string[], options?: ParseXmlOptions): unknown { const _stopNodes = stopNodes ?? [] + const processEntities = options?.processEntities ?? true + try { const parser = new XMLParser({ ignoreAttributes: false, @@ -15,6 +30,7 @@ export function parseXml(xmlString: string, stopNodes?: string[]): unknown { parseAttributeValue: false, parseTagValue: false, trimValues: true, + processEntities, stopNodes: _stopNodes, }) @@ -25,3 +41,23 @@ export function parseXml(xmlString: string, stopNodes?: string[]): unknown { throw new Error(`Failed to parse XML: ${errorMessage}`) } } + +/** + * Parses an XML string for diffing purposes, ensuring no HTML entities are decoded. + * This is a specialized version of parseXml to be used exclusively by diffing tools + * to prevent mismatches caused by entity processing. + * + * Use this instead of parseXml when: + * - Comparing parsed content against original file content + * - Performing diff operations where exact character matching is required + * - Processing XML that will be used in search/replace operations + * + * @param xmlString The XML string to parse + * @param stopNodes Optional array of node names to stop parsing at + * @returns Parsed JavaScript object representation of the XML + * @throws Error if the XML is invalid or parsing fails + */ +export function parseXmlForDiff(xmlString: string, stopNodes?: string[]): unknown { + // Delegate to parseXml with processEntities disabled + return parseXml(xmlString, stopNodes, { processEntities: false }) +}