fix: improve XML parsing error handling for apply_diff tool #7973
Conversation
- Enhanced error messages for 'StopNode is not closed' errors - Added pre-validation for XML structure when stopNodes are specified - Provided user-friendly guidance for truncated/malformed XML - Added comprehensive tests for error scenarios Fixes #7972
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| if (isStopNodeError) { | ||
| await cline.say( | ||
| "diff_error", | ||
| "The apply_diff XML appears to be incomplete. This often happens when the response is truncated. " + |
There was a problem hiding this comment.
User‐facing error text for the StopNode error is hardcoded ('The apply_diff XML appears to be incomplete...'). Consider using an internationalization (i18n) function to ensure consistency and support for multiple languages.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| const lastTag = tags[tags.length - 1] | ||
|
|
||
| // Check if the tag appears to be opened but not closed | ||
| const openTagPattern = new RegExp(`<${lastTag}[^>]*>`, "g") |
There was a problem hiding this comment.
When constructing regex patterns using the dynamic tag name (e.g. ${lastTag}), ensure that the tag value is safe from unexpected regex special characters. Consider escaping the tag if there's any chance it could contain such characters.
| const openTagPattern = new RegExp(`<${lastTag}[^>]*>`, "g") | |
| const openTagPattern = new RegExp(`<${lastTag.replace(/[.*+?^${}()|[\\]\\]/g, '\\$&')}[^>]*>`, "g") |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const lastOpenTagIndex = xmlString.lastIndexOf(`<${lastTag}`) | ||
|
|
||
| if (lastOpenTagIndex > lastCloseTagIndex) { | ||
| console.warn( |
There was a problem hiding this comment.
The console.warn here could pollute production logs. Would it make sense to use a proper logging framework or make this configurable based on environment? This way we could control when these warnings appear.
| * @param xmlString The XML string to validate | ||
| * @param stopNodes Array of node paths to check | ||
| * @throws Error if validation fails | ||
| */ |
There was a problem hiding this comment.
The JSDoc says this function 'throws Error if validation fails', but it actually only logs warnings. Should we update the documentation to reflect that this performs non-throwing validation?
| const lastTag = tags[tags.length - 1] | ||
|
|
||
| // Check if the tag appears to be opened but not closed | ||
| const openTagPattern = new RegExp(`<${lastTag}[^>]*>`, "g") |
There was a problem hiding this comment.
Is this regex approach intentionally simple? It might incorrectly flag valid XML if these patterns appear in CDATA sections or comments. Would a more robust validation approach be worth considering, or is the current implementation sufficient for our use case?
| }) | ||
|
|
||
| it("should handle enhanced error messages for StopNode errors", () => { | ||
| // Create a mock error to test our enhanced error handling |
There was a problem hiding this comment.
These tests mostly use mocked errors rather than actual XML that would trigger the StopNode error. Since fast-xml-parser may not throw for all malformed XML (as noted in the comments), would it be valuable to add integration tests with real truncated XML that reliably triggers the actual error condition?
|
|
||
| let detailedError: string | ||
| if (isStopNodeError) { | ||
| detailedError = `Failed to parse apply_diff XML: The XML appears to be incomplete or truncated. |
There was a problem hiding this comment.
These error message templates are quite detailed (which is great for users!). Would it make sense to extract them to constants or a separate error messages module for better maintainability? This could make it easier to update messages consistently across the codebase.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing, see #7972 (comment) |
Description
This PR addresses Issue #7972 by improving the error handling for XML parsing in the apply_diff tool, specifically for the 'StopNode is not closed' error that occurs when XML is truncated or malformed.
Problem
Users were encountering the error message:
This error was not providing enough context for users to understand what went wrong or how to fix it.
Solution
Changes Made
src/utils/xml.ts:
validateXmlStructurefunction for pre-validationsrc/core/tools/multiApplyDiffTool.ts:
src/utils/tests/xml.spec.ts:
Testing
Impact
This fix will help users understand and recover from XML parsing errors more easily, particularly when:
Fixes #7972
Important
Improves XML parsing error handling in
apply_difftool by enhancing error messages, adding pre-validation, and providing user-friendly recovery steps.applyDiffTool()inmultiApplyDiffTool.ts.validateXmlStructure()inxml.ts.applyDiffTool().xml.spec.ts.validateXmlStructure()function inxml.tsfor pre-validation.parseXml()inxml.ts.This description was created by
for 1d8aefb. You can customize this summary. It will automatically update as commits are pushed.