fix: improve XML parsing error handling for read_file tool #7802
Conversation
- Add tests to verify XML parser handles whitespace correctly - Add tests for assistant message parser with whitespace in XML - Add tests for readFileTool handling XML args with whitespace - Tests confirm existing implementation already handles whitespace properly - Addresses issue #7664 where Grok/Qwen3-Coder generate XML with spaces
- Add proper trimming of path values to handle whitespace - Provide clearer error messages when path is empty or missing - Track invalid entries for better error reporting - Add tests to confirm line_range is optional - Add tests for malformed XML scenarios - Fixes issue #7664 where Grok/Qwen3-Coder XML with whitespace caused errors
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const errorMsg = await cline.sayAndCreateMissingParamError("read_file", "args (containing valid file paths)") | ||
| const errorMsg = await cline.sayAndCreateMissingParamError( | ||
| "read_file", | ||
| "args with valid file paths. Expected format: <args><file><path>filepath</path></file></args>", |
There was a problem hiding this comment.
Is this intentional? The error message mentions line_range in the documentation hint, but based on the tests, line_range isn't actually a supported parameter for the read_file tool. Could we clarify this in the tool documentation to avoid confusion?
|
|
||
| // If we had invalid entries but no valid ones, provide a helpful error | ||
| if (fileEntries.length === 0 && invalidEntries.length > 0) { | ||
| const errorMessage = `No valid file paths found in read_file args. Invalid entries: ${invalidEntries.join(", ")}. Ensure each <file> element contains a non-empty <path> element.` |
There was a problem hiding this comment.
Consider standardizing error message formats for consistency. We have "No valid file paths found" here and "Failed to parse read_file XML args" below. A consistent format would improve the user experience.
|
|
||
| if (!filePath) { | ||
| // Track this invalid entry | ||
| invalidEntries.push(JSON.stringify(file).substring(0, 100)) |
There was a problem hiding this comment.
Performance consideration: For large malformed XML, collecting up to 100 characters of each invalid entry could accumulate unnecessary data. Consider limiting to the first 3 invalid entries:
| invalidEntries.push(JSON.stringify(file).substring(0, 100)) | |
| if (invalidEntries.length < 3) { | |
| invalidEntries.push(JSON.stringify(file).substring(0, 100)) | |
| } |
| expect(toolResult).toContain('<content lines="1-5">') | ||
| expect(toolResult).not.toContain("<error>") | ||
| }) | ||
|
|
There was a problem hiding this comment.
Great test coverage! Consider adding one more test that combines whitespace issues with multiple files where some have valid paths and others have empty paths after trimming. This would test the mixed scenario comprehensively.
hannesrudolph
added
the
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Fixes #7664
Problem
When AI models like Grok and Qwen3-Coder generate XML with whitespace or empty path elements, the read_file tool was giving confusing "Missing value for required parameter" errors.
Solution
This PR improves the XML parsing and error handling in the read_file tool to:
Changes
src/core/tools/readFileTool.tsto add path trimming and better error handlingTesting
All tests pass (48 tests in readFileTool.spec.ts, 109 total across modified test files).
The existing XML parser already handles whitespace correctly with
trimValues: true, but the readFileTool needed improvements to handle edge cases where paths are empty after trimming.Impact
This fix will improve the experience for users of AI models that generate XML with formatting variations, particularly Grok and Qwen3-Coder.
Important
Improves XML parsing and error handling in
readFileToolto handle whitespace, empty paths, and provide clearer error messages, with extensive testing added.readFileTool.tsto handle whitespace and empty path elements.line_rangeparameter is optional with tests.readFileTool.ts.readFileTool.spec.tsandxml.spec.ts.line_rangeand malformed XML scenarios.parseXmlfunction to trim whitespace in paths.This description was created by
for b8814d3. You can customize this summary. It will automatically update as commits are pushed.