Refactor read-file tool to support XML input format and multiple line ranges

- Updated the `getReadFileDescription` function to reflect new XML structure for file reading requests.
- Modified `readFileTool` to parse XML input, allowing multiple line ranges for each file.
- Removed old parsing logic that handled line ranges as separate parameters.
- Implemented validation for line ranges and ensured proper error handling for file access.
- Adjusted approval messaging to accommodate new line range format.
- Enhanced error handling to provide consistent feedback for file read errors.

Author: samhvw8

src/core/__tests__/read-file-maxReadFileLine.test.ts

@@ -152,13 +152,11 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		;(addLineNumbers as jest.Mock).mockClear()
 
 		// Format args string based on params
-		let argsContent = `:path:${params.path || testFilePath}`
-		if (params.start_line) {
-			argsContent += `\n:start_line:${params.start_line}`
-		}
-		if (params.end_line) {
-			argsContent += `\n:end_line:${params.end_line}`
+		let argsContent = `<file><path>${params.path || testFilePath}</path>`
+		if (params.start_line && params.end_line) {
+			argsContent += `<line_range>${params.start_line}-${params.end_line}</line_range>`
 		}
+		argsContent += `</file>`
 
 		const toolUse: ReadFileToolUse = {
 			type: "tool_use",
@@ -327,7 +325,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 
 			// Verify XML structure
 			expect(result).toBe(
-				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-3">\n1 | Line 1\n2 | Line 2\n3 | Line 3</content>\n<list_code_definition_names>${sourceCodeDef.trim()}</list_code_definition_names>\n<notice>Showing only 3 of 5 total lines. Use start_line and end_line if you need to read more</notice>\n</file>\n</files>`,
+				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-3">\n1 | Line 1\n2 | Line 2\n3 | Line 3</content>\n<list_code_definition_names>${sourceCodeDef.trim()}</list_code_definition_names>\n<notice>Showing only 3 of 5 total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`,
 			)
 		})
 
@@ -436,7 +434,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 
 			// Verify
 			expect(result).toBe(
-				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n1 | Line 1\n2 | Line 2\n3 | Line 3</content>\n<list_code_definition_names>${sourceCodeDef.trim()}</list_code_definition_names>\n<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more</notice>\n</file>\n</files>`,
+				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n1 | Line 1\n2 | Line 2\n3 | Line 3</content>\n<list_code_definition_names>${sourceCodeDef.trim()}</list_code_definition_names>\n<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`,
 			)
 		})
 
@@ -612,7 +610,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			expect(mockedReadLines).toHaveBeenCalled()
 			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
 			expect(result).toBe(
-				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n${numberedContent}\n</content>\n<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more</notice>\n</file>\n</files>`,
+				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n${numberedContent}\n</content>\n<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`,
 			)
 		})
 
@@ -634,7 +632,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 
 			// Should handle long lines without issues
 			expect(result).toBe(
-				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n${numberedLine}\n</content>\n<notice>Showing only ${maxReadFileLine} of 5 total lines. Use start_line and end_line if you need to read more</notice>\n</file>\n</files>`,
+				`<files>\n<file><path>${testFilePath}</path>\n<content lines="1-${maxReadFileLine}">\n${numberedLine}\n</content>\n<notice>Showing only ${maxReadFileLine} of 5 total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`,
 			)
 		})
 	})

src/core/__tests__/read-file-tool.test.ts

@@ -25,6 +25,11 @@ jest.mock("../ignore/RooIgnoreController", () => ({
 	},
 }))
 
+// Mocked functions with correct types
+const mockedCountFileLines = countFileLines as jest.MockedFunction<typeof countFileLines>
+const mockedReadLines = readLines as jest.MockedFunction<typeof readLines>
+const mockedExtractTextFromFile = extractTextFromFile as jest.MockedFunction<typeof extractTextFromFile>
+
 describe("read_file tool functionality", () => {
 	// Mock instances
 	const mockCline = {
@@ -70,7 +75,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:src/app.ts\n:start_line:1\n:end_line:100`,
+					args: `<file><path>src/app.ts</path><line_range>1-100</line_range></file>`,
 				},
 				partial: false,
 			}
@@ -103,7 +108,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:src/app.ts\n:start_line:1\n:end_line:50\n======+++======\n:path:src/utils.ts`,
+					args: `<file><path>src/app.ts</path><line_range>1-50</line_range></file><file><path>src/utils.ts</path></file>`,
 				},
 				partial: false,
 			}
@@ -136,11 +141,15 @@ describe("read_file tool functionality", () => {
 		})
 
 		it("should handle invalid line range parameters", async () => {
+			// Setup
+			mockedReadLines.mockRejectedValue(new Error("Invalid line range: invalid values"))
+			mockedExtractTextFromFile.mockRejectedValue(new Error("Invalid line range: invalid values"))
+
 			const toolUse: ReadFileToolUse = {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:src/app.ts\n:start_line:abc\n:end_line:def`,
+					args: `<file><path>src/app.ts</path><line_range>abc-def</line_range></file>`,
 				},
 				partial: false,
 			}
@@ -159,15 +168,17 @@ describe("read_file tool functionality", () => {
 				(param: string, value: string) => value,
 			)
 
-			expect(result).toBe(`<files><error>Error reading files: Invalid start_line value</error></files>`)
+			expect(result).toBe(
+				`<files>\n<file><path>src/app.ts</path><error>Error reading file: Invalid line range: invalid values</error></file>\n</files>`,
+			)
 		})
 
 		it("should handle empty file entries in multiple file reads", async () => {
 			const toolUse: ReadFileToolUse = {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:src/app.ts\n======+++======\n\n======+++======\n:path:src/utils.ts`,
+					args: `<file><path>src/app.ts</path></file><file><path>src/utils.ts</path></file>`,
 				},
 				partial: false,
 			}
@@ -198,7 +209,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:smallFile.txt`,
+					args: `<file><path>smallFile.txt</path></file>`,
 				},
 				partial: false,
 			}
@@ -231,7 +242,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:largeFile.txt`,
+					args: `<file><path>largeFile.txt</path></file>`,
 				},
 				partial: false,
 			}
@@ -261,7 +272,7 @@ describe("read_file tool functionality", () => {
 			)
 
 			expect(result).toBe(
-				`<files>\n<file><path>largeFile.txt</path>\n<content lines="1-500">\n1 | Test content</content>\n<notice>Showing only 500 of 5000 total lines. Use start_line and end_line if you need to read more</notice>\n</file>\n</files>`,
+				`<files>\n<file><path>largeFile.txt</path>\n<content lines="1-500">\n1 | Test content</content>\n<notice>Showing only 500 of 5000 total lines. Use line_range if you need to read more lines</notice>\n</file>\n</files>`,
 			)
 		})
 
@@ -270,7 +281,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:binary.pdf`,
+					args: `<file><path>binary.pdf</path></file>`,
 				},
 				partial: false,
 			}
@@ -305,7 +316,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:${filePath}\n:start_line:1\n:end_line:100`,
+					args: `<file><path>${filePath}</path><line_range>1-100</line_range></file>`,
 				},
 				partial: false,
 			}
@@ -367,19 +378,22 @@ describe("read_file tool functionality", () => {
 		})
 
 		it("should handle file read errors", async () => {
+			// Setup
+			mockedCountFileLines.mockRejectedValue(new Error("File not found"))
+			mockedReadLines.mockRejectedValue(new Error("File not found"))
+			mockedExtractTextFromFile.mockRejectedValue(new Error("File not found"))
+
 			const toolUse: ReadFileToolUse = {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:nonexistent.txt`,
+					args: `<file><path>nonexistent.txt</path></file>`,
 				},
 				partial: false,
 			}
 
 			const { readFileTool } = require("../tools/readFileTool")
 
-			;(countFileLines as jest.Mock).mockRejectedValue(new Error("File not found"))
-
 			let result: string | undefined
 			await readFileTool(
 				mockCline,
@@ -392,15 +406,17 @@ describe("read_file tool functionality", () => {
 				(param: string, value: string) => value,
 			)
 
-			expect(result).toBe(`<files><error>Error reading files: File not found</error></files>`)
+			expect(result).toBe(
+				`<files>\n<file><path>nonexistent.txt</path><error>Error reading file: File not found</error></file>\n</files>`,
+			)
 		})
 
 		it("should handle line counting errors", async () => {
 			const toolUse: ReadFileToolUse = {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:error.txt`,
+					args: `<file><path>error.txt</path></file>`,
 				},
 				partial: false,
 			}
@@ -431,7 +447,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:src/code.ts`,
+					args: `<file><path>src/code.ts</path></file>`,
 				},
 				partial: false,
 			}
@@ -465,7 +481,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:huge.log`,
+					args: `<file><path>huge.log</path></file>`,
 				},
 				partial: false,
 			}
@@ -497,7 +513,7 @@ describe("read_file tool functionality", () => {
 				type: "tool_use",
 				name: "read_file",
 				params: {
-					args: `:path:longlines.txt`,
+					args: `<file><path>longlines.txt</path></file>`,
 				},
 				partial: false,
 			}