fix: improve XML parsing error handling for read_file tool

src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts

@@ -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

src/core/tools/__tests__/readFileTool.spec.ts

@@ -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>")
+		})
+
+		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>",
+			)
+		})
 	})
 })
 

src/core/tools/readFileTool.ts

@@ -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))
+					continue // Skip if no path in a file entry
+				}
 
 				const fileEntry: FileEntry = {
-					path: file.path,
+					path: filePath,
 					lineRanges: [],
 				}
 
@@ -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.`
+				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
@@ -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>",
+		)
 		pushToolResult(`<files><error>${errorMsg}</error></files>`)
 		return
 	}