Skip to content

fix: improve XML parsing error handling for read_file tool #7802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fix: improve XML parsing error handling for read_file tool #7802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
593292c
test: add comprehensive tests for XML parsing with whitespace
roomote Sep 9, 2025
b8814d3
fix: improve XML parsing error handling for read_file tool
roomote Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,48 @@ const isEmptyTextContent = (block: AssistantMessageContent) =>
expect(toolUse.partial).toBe(false)
})

it("should handle whitespace and newlines between XML tags (Grok/Qwen format)", () => {
// This is the format that Grok and Qwen3-Coder generate that was causing issues
const message = `<read_file>
<args>
<file>
<path>src/shared/infrastructure/supabase/factory.py</path>
</file>
</args>
</read_file>`
const result = parser(message).filter((block) => !isEmptyTextContent(block))

expect(result).toHaveLength(1)
const toolUse = result[0] as ToolUse
expect(toolUse.type).toBe("tool_use")
expect(toolUse.name).toBe("read_file")
// The args should be captured as a parameter
expect(toolUse.params.args).toBeDefined()
expect(toolUse.params.args).toContain("<file>")
expect(toolUse.params.args).toContain("<path>src/shared/infrastructure/supabase/factory.py</path>")
expect(toolUse.partial).toBe(false)
})

it("should handle whitespace-only path values", () => {
const message = `<read_file>
<args>
<file>
<path> </path>
</file>
</args>
</read_file>`
const result = parser(message).filter((block) => !isEmptyTextContent(block))

expect(result).toHaveLength(1)
const toolUse = result[0] as ToolUse
expect(toolUse.type).toBe("tool_use")
expect(toolUse.name).toBe("read_file")
expect(toolUse.params.args).toBeDefined()
// The whitespace-only path should be preserved in the args
expect(toolUse.params.args).toContain("<path> </path>")
expect(toolUse.partial).toBe(false)
})

it("should handle multi-line parameters", () => {
const message = `<write_to_file><path>file.ts</path><content>
line 1
Expand Down
244 changes: 244 additions & 0 deletions src/core/tools/__tests__/readFileTool.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,250 @@ describe("read_file tool XML output structure", () => {
`<files>\n<file><path>${testFilePath}</path><error>Access to ${testFilePath} is blocked by the .rooignore file settings. You must try to continue in the task without using this file, or ask the user to update the .rooignore file.</error></file>\n</files>`,
)
})

it("should handle XML args with whitespace and newlines (Grok/Qwen format)", async () => {
// This test reproduces the exact issue reported with Grok and Qwen3-Coder
// where XML has newlines and spaces between tags
const argsWithWhitespace = `
<file>
<path> test/file.txt </path>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: argsWithWhitespace },
partial: false,
}

// Setup mocks
mockedCountFileLines.mockResolvedValue(5)
mockedExtractTextFromFile.mockResolvedValue("1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5")
mockProvider.getState.mockResolvedValue({
maxReadFileLine: -1,
maxImageFileSize: 20,
maxTotalImageSize: 20,
})

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
vi.fn(),
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// Verify the file was read successfully (path was trimmed)
expect(toolResult).toContain("<file><path>test/file.txt</path>")
expect(toolResult).toContain('<content lines="1-5">')
expect(toolResult).not.toContain("<error>")
})

Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

it("should handle multiple files with whitespace in XML", async () => {
// Test multiple files with varying whitespace
const argsWithMultipleFiles = `
<file>
<path>
test/file1.txt
</path>
</file>
<file>
<path> test/file2.txt </path>
</file>
<file>
<path>test/file3.txt</path>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: argsWithMultipleFiles },
partial: false,
}

// Setup mocks
mockedCountFileLines.mockResolvedValue(3)
mockedExtractTextFromFile.mockResolvedValue("1 | Content")
mockProvider.getState.mockResolvedValue({
maxReadFileLine: -1,
maxImageFileSize: 20,
maxTotalImageSize: 20,
})

// Mock path.resolve for each file
mockedPathResolve.mockImplementation((cwd, relPath) => `/${relPath.trim()}`)

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
vi.fn(),
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// Verify all files were processed
expect(toolResult).toContain("<file><path>test/file1.txt</path>")
expect(toolResult).toContain("<file><path>test/file2.txt</path>")
expect(toolResult).toContain("<file><path>test/file3.txt</path>")
expect(toolResult).not.toContain("<error>")
})

it("should handle empty path after trimming whitespace", async () => {
// Test case where path is only whitespace
const argsWithEmptyPath = `
<file>
<path> </path>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: argsWithEmptyPath },
partial: false,
}

// Create a spy for handleError
const handleErrorSpy = vi.fn()

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
handleErrorSpy,
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// Verify error is returned for empty path
expect(toolResult).toContain("<error>")
expect(toolResult).toContain("No valid file paths found")
expect(toolResult).toContain("Ensure each <file> element contains a non-empty <path> element")
expect(handleErrorSpy).toHaveBeenCalled()
})

it("should work without line_range parameter (line_range is optional)", async () => {
// Test that line_range is truly optional
const argsWithoutLineRange = `
<file>
<path>test/file.txt</path>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: argsWithoutLineRange },
partial: false,
}

// Setup mocks
mockedCountFileLines.mockResolvedValue(5)
mockedExtractTextFromFile.mockResolvedValue("1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5")
mockProvider.getState.mockResolvedValue({
maxReadFileLine: -1,
maxImageFileSize: 20,
maxTotalImageSize: 20,
})

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
vi.fn(),
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// Verify the file was read successfully without line_range
expect(toolResult).toContain("<file><path>test/file.txt</path>")
expect(toolResult).toContain('<content lines="1-5">')
expect(toolResult).not.toContain("<error>")
expect(toolResult).not.toContain("line_range")
})

it("should provide helpful error for malformed XML missing path element", async () => {
// Test case simulating what Grok might send - file element with missing path
const malformedArgs = `
<file>
<path></path>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: malformedArgs },
partial: false,
}

// Create a spy for handleError
const handleErrorSpy = vi.fn()

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
handleErrorSpy,
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// Verify helpful error is returned
expect(toolResult).toContain("<error>")
expect(toolResult).toContain("No valid file paths found")
expect(toolResult).toContain("Ensure each <file> element contains a non-empty <path> element")
expect(handleErrorSpy).toHaveBeenCalled()
})

it("should provide error when file element has no path child at all", async () => {
// Test case where file element exists but has no path child element
const malformedArgs = `
<file>
</file>
`
const toolUse: ReadFileToolUse = {
type: "tool_use",
name: "read_file",
params: { args: malformedArgs },
partial: false,
}

// Execute
await readFileTool(
mockCline,
toolUse,
mockCline.ask,
vi.fn(),
(result: ToolResponse) => {
toolResult = result
},
(param: ToolParamName, content?: string) => content ?? "",
)

// When file element has no path child, it falls through to the general error
expect(toolResult).toContain("<error>")
expect(toolResult).toContain("Missing required parameter")
// This is handled by sayAndCreateMissingParamError
expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith(
"read_file",
"args with valid file paths. Expected format: <args><file><path>filepath</path></file></args>",
)
})
})
})

Expand Down
29 changes: 25 additions & 4 deletions src/core/tools/readFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,21 @@ export async function readFileTool(
const parsed = parseXml(argsXmlTag) as any
const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean)

// Track invalid entries for better error reporting
const invalidEntries: string[] = []

for (const file of files) {
if (!file.path) continue // Skip if no path in a file entry
// Check if path exists and is not empty after trimming
const filePath = typeof file.path === "string" ? file.path.trim() : file.path

if (!filePath) {
// Track this invalid entry
invalidEntries.push(JSON.stringify(file).substring(0, 100))
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
invalidEntries.push(JSON.stringify(file).substring(0, 100))
if (invalidEntries.length < 3) {
invalidEntries.push(JSON.stringify(file).substring(0, 100))
}

continue // Skip if no path in a file entry
}

const fileEntry: FileEntry = {
path: file.path,
path: filePath,
lineRanges: [],
}

Expand All @@ -153,8 +163,16 @@ export async function readFileTool(
}
fileEntries.push(fileEntry)
}

// 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.`
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await handleError("parsing read_file args", new Error(errorMessage))
pushToolResult(`<files><error>${errorMessage}</error></files>`)
return
}
} catch (error) {
const errorMessage = `Failed to parse read_file XML args: ${error instanceof Error ? error.message : String(error)}`
const errorMessage = `Failed to parse read_file XML args: ${error instanceof Error ? error.message : String(error)}. Expected format: <file><path>filepath</path></file>`
await handleError("parsing read_file args", new Error(errorMessage))
pushToolResult(`<files><error>${errorMessage}</error></files>`)
return
Expand Down Expand Up @@ -186,7 +204,10 @@ export async function readFileTool(
if (fileEntries.length === 0) {
cline.consecutiveMistakeCount++
cline.recordToolError("read_file")
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>",
Copy link
Contributor Author

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

)
pushToolResult(`<files><error>${errorMsg}</error></files>`)
return
}
Expand Down
Loading
Loading