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 <roomote@roocode.com>
Co-authored-by: daniel-lxs <ricciodaniel98@gmail.com>
This commit is contained in:
roomote[bot] 2025-08-14 17:46:52 -04:00 committed by GitHub
parent 342123d351
commit 6540f2be5c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 166 additions and 4 deletions

View file

@ -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 &amp; 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) {

View file

@ -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 &amp;", () => {
const xml = `
<root>
<content>Team Identity &amp; Project Positioning</content>
</root>
`
const result = parseXmlForDiff(xml) as any
// The &amp; should remain as-is, not be decoded to &
expect(result.root.content).toBe("Team Identity &amp; Project Positioning")
})
it("should preserve & character without encoding", () => {
const xml = `
<root>
<content>Team Identity & Project Positioning</content>
</root>
`
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 = `
<root>
<content>&lt;div&gt; &quot;Hello&quot; &apos;World&apos;</content>
</root>
`
const result = parseXmlForDiff(xml) as any
// All HTML entities should remain as-is
expect(result.root.content).toBe("&lt;div&gt; &quot;Hello&quot; &apos;World&apos;")
})
it("should handle mixed content with entities correctly", () => {
const xml = `
<root>
<code>if (a &lt; b &amp;&amp; c &gt; d) { return &quot;test&quot;; }</code>
</root>
`
const result = parseXmlForDiff(xml) as any
// All entities should remain unchanged
expect(result.root.code).toBe("if (a &lt; b &amp;&amp; c &gt; d) { return &quot;test&quot;; }")
})
})
describe("basic functionality (same as parseXml)", () => {
it("should correctly parse a simple XML string", () => {
const xml = `
<root>
<name>Test Name</name>
<description>Some description</description>
</root>
`
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 = `
<root>
<item id="1" category="test">Item content</item>
</root>
`
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 = `
<root>
<data>
<nestedXml><item>Should not parse this</item></nestedXml>
</data>
</root>
`
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 = `
<args>
<file>
<path>./doc.md</path>
<diff>
<content>Team Identity & Project Positioning</content>
</diff>
</file>
</args>
`
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")
})
})
})

View file

@ -1,13 +1,28 @@
import { XMLParser } from "fast-xml-parser"
/**
* Options for XML parsing
*/
interface ParseXmlOptions {
/**
* Whether to process HTML entities (e.g., &amp; 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 })
}