refactor: improve readFileTool XML output format (#2340)

* fix: addLineNumbers handling of empty content

Empty files should not have line numbers, but non-empty files with empty content at a specific line offset should.
- If content is empty, return empty string for empty files
- If content is empty but startLine > 1, return line number for empty content at that offset
This ensures that the model does not think the file contains a single empty line.

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* refactor: improve readFileTool XML output format

- Remove unnecessary XML indentation that could confuse the model
- Separate file content from notices and errors using dedicated tags
- Add line range information to content tags
- Handle empty files properly with self-closing tags
- Add comprehensive test coverage

Fixes #2278

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* fix: always show line numbers in read_file XML output

- Always display line numbers in non-range reads
- Improve XML formatting with consistent newlines for better readability

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* test: update tests to match new XML format with line numbers

- Update test expectations to match the new XML format with newlines
- Update tests to expect line numbers attribute in content tags
- Modify test assertions to check for the correct line range values

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* fix: consistent blank line handling in addLineNumbers

- Add newline to all output
- Handle trailing newlines and empty lines consistently
- Add test cases for blank lines:
  - Multiple blank lines within content
  - Multiple trailing blank lines
  - Only blank lines with offset
  - Trailing newlines

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* test: use actual addLineNumbers in read-file-xml tests

- Modified extract-text mock to preserve actual addLineNumbers implementation
- Removed mock implementation of addLineNumbers
- Updated test data to account for trailing newline
- Removed unnecessary mock verification

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* test: ensure actual addLineNumbers function is called in tests

- Replace direct mocking of addLineNumbers with spy on actual implementation
- Add verification to ensure the real function is called when appropriate
- Add skipAddLineNumbersCheck option for cases where function should not be called
- Update test cases to use appropriate verification options
- Fix numberedFileContent to include trailing newline for consistency

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* fix: modify readLines to process data directly instead of line by line

- Direct data processing provides more accurate results by preserving exact content with carriage returns
- Improved performance through minimal buffering and efficient string operations
- Use string indexes to find newlines while maintaining their original format
- Handle all edge cases correctly with preserved line endings
- Add tests for various edge cases including empty files, single lines, and different line endings

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

* test: remove unused mockInputContent variable

Remove unused variable declaration to appease ellipsis-dev linter requirements.

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>

---------

Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>
Co-authored-by: Eric Wheeler <roo-code@z.ewheeler.org>

Author: KJ7LNW

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

@@ -10,7 +10,22 @@ import { Cline } from "../Cline"
 // Mock dependencies
 jest.mock("../../integrations/misc/line-counter")
 jest.mock("../../integrations/misc/read-lines")
-jest.mock("../../integrations/misc/extract-text")
+jest.mock("../../integrations/misc/extract-text", () => {
+	const actual = jest.requireActual("../../integrations/misc/extract-text")
+	// Create a spy on the actual addLineNumbers function
+	const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers")
+
+	return {
+		...actual,
+		// Expose the spy so tests can access it
+		__addLineNumbersSpy: addLineNumbersSpy,
+		extractTextFromFile: jest.fn(),
+	}
+})
+
+// Get a reference to the spy
+const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy
+
 jest.mock("../../services/tree-sitter")
 jest.mock("isbinaryfile")
 jest.mock("../ignore/RooIgnoreController", () => ({
@@ -46,9 +61,9 @@ describe("read_file tool with maxReadFileLine setting", () => {
 	const testFilePath = "test/file.txt"
 	const absoluteFilePath = "/test/file.txt"
 	const fileContent = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5"
-	const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5"
+	const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5\n"
 	const sourceCodeDef = "\n\n# file.txt\n1--5 | Content"
-	const expectedFullFileXml = `<file>\n  <path>${testFilePath}</path>\n  <content>\n${numberedFileContent}\n  </content>\n</file>`
+	const expectedFullFileXml = `<file><path>${testFilePath}</path>\n<content lines="1-5">\n${numberedFileContent}</content>\n</file>`
 
 	// Mocked functions with correct types
 	const mockedCountFileLines = countFileLines as jest.MockedFunction<typeof countFileLines>
@@ -58,6 +73,10 @@ describe("read_file tool with maxReadFileLine setting", () => {
 	const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction<
 		typeof parseSourceCodeDefinitionsForFile
 	>
+
+	// Variable to control what content is used by the mock - set in beforeEach
+	let mockInputContent = ""
+
 	const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction<typeof isBinaryFile>
 	const mockedPathResolve = path.resolve as jest.MockedFunction<typeof path.resolve>
 
@@ -74,13 +93,19 @@ describe("read_file tool with maxReadFileLine setting", () => {
 
 		// Setup mocks for file operations
 		mockedIsBinaryFile.mockResolvedValue(false)
-		mockedAddLineNumbers.mockImplementation((content: string, startLine = 1) => {
-			return content
-				.split("\n")
-				.map((line, i) => `${i + startLine} | ${line}`)
-				.join("\n")
+
+		// Set the default content for the mock
+		mockInputContent = fileContent
+
+		// Setup the extractTextFromFile mock implementation with the current mockInputContent
+		mockedExtractTextFromFile.mockImplementation((filePath) => {
+			const actual = jest.requireActual("../../integrations/misc/extract-text")
+			return Promise.resolve(actual.addLineNumbers(mockInputContent))
 		})
 
+		// No need to setup the extractTextFromFile mock implementation here
+		// as it's already defined at the module level
+
 		// Setup mock provider
 		mockProvider = {
 			getState: jest.fn(),
@@ -105,16 +130,32 @@ describe("read_file tool with maxReadFileLine setting", () => {
 	/**
 	 * Helper function to execute the read file tool with different maxReadFileLine settings
 	 */
-	async function executeReadFileTool(maxReadFileLine: number, totalLines = 5): Promise<string | undefined> {
+	async function executeReadFileTool(
+		params: Partial<ReadFileToolUse["params"]> = {},
+		options: {
+			maxReadFileLine?: number
+			totalLines?: number
+			skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check
+		} = {},
+	): Promise<string | undefined> {
 		// Configure mocks based on test scenario
+		const maxReadFileLine = options.maxReadFileLine ?? 500
+		const totalLines = options.totalLines ?? 5
+
 		mockProvider.getState.mockResolvedValue({ maxReadFileLine })
 		mockedCountFileLines.mockResolvedValue(totalLines)
 
+		// Reset the spy before each test
+		addLineNumbersSpy.mockClear()
+
 		// Create a tool use object
 		const toolUse: ReadFileToolUse = {
 			type: "tool_use",
 			name: "read_file",
-			params: { path: testFilePath },
+			params: {
+				path: testFilePath,
+				...params,
+			},
 			partial: false,
 		}
 
@@ -133,16 +174,23 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			(param: string, value: string) => value,
 		)
 
+		// Verify addLineNumbers was called appropriately
+		if (!options.skipAddLineNumbersCheck) {
+			expect(addLineNumbersSpy).toHaveBeenCalled()
+		} else {
+			expect(addLineNumbersSpy).not.toHaveBeenCalled()
+		}
+
 		return toolResult
 	}
 
 	describe("when maxReadFileLine is negative", () => {
 		it("should read the entire file using extractTextFromFile", async () => {
-			// Setup
-			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
+			// Setup - use default mockInputContent
+			mockInputContent = fileContent
 
 			// Execute
-			const result = await executeReadFileTool(-1)
+			const result = await executeReadFileTool({}, { maxReadFileLine: -1 })
 
 			// Verify
 			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
@@ -157,8 +205,15 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			// Setup - for maxReadFileLine = 0, the implementation won't call readLines
 			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
 
-			// Execute
-			const result = await executeReadFileTool(0)
+			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
+			const result = await executeReadFileTool(
+				{},
+				{
+					maxReadFileLine: 0,
+					totalLines: 5,
+					skipAddLineNumbersCheck: true,
+				},
+			)
 
 			// Verify
 			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
@@ -167,8 +222,15 @@ describe("read_file tool with maxReadFileLine setting", () => {
 				absoluteFilePath,
 				mockCline.rooIgnoreController,
 			)
-			expect(result).toContain("[Showing only 0 of 5 total lines")
-			expect(result).toContain(sourceCodeDef)
+
+			// Verify XML structure
+			expect(result).toContain(`<file><path>${testFilePath}</path>`)
+			expect(result).toContain("<notice>Showing only 0 of 5 total lines")
+			expect(result).toContain("</notice>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
+			expect(result).toContain("</list_code_definition_names>")
+			expect(result).not.toContain("<content") // No content when maxReadFileLine is 0
 		})
 	})
 
@@ -180,7 +242,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
 			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
 
 			// Execute
-			const result = await executeReadFileTool(3)
+			const result = await executeReadFileTool({}, { maxReadFileLine: 3 })
 
 			// Verify - check behavior but not specific implementation details
 			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
@@ -189,22 +251,32 @@ describe("read_file tool with maxReadFileLine setting", () => {
 				absoluteFilePath,
 				mockCline.rooIgnoreController,
 			)
+
+			// Verify XML structure
+			expect(result).toContain(`<file><path>${testFilePath}</path>`)
+			expect(result).toContain('<content lines="1-3">')
 			expect(result).toContain("1 | Line 1")
 			expect(result).toContain("2 | Line 2")
 			expect(result).toContain("3 | Line 3")
-			expect(result).toContain("[Showing only 3 of 5 total lines")
-			expect(result).toContain(sourceCodeDef)
+			expect(result).toContain("</content>")
+			expect(result).toContain("<notice>Showing only 3 of 5 total lines")
+			expect(result).toContain("</notice>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
+			expect(result).toContain("</list_code_definition_names>")
+			expect(result).toContain("<list_code_definition_names>")
+			expect(result).toContain(sourceCodeDef.trim())
 		})
 	})
 
 	describe("when maxReadFileLine equals or exceeds file length", () => {
 		it("should use extractTextFromFile when maxReadFileLine > totalLines", async () => {
 			// Setup
 			mockedCountFileLines.mockResolvedValue(5) // File shorter than maxReadFileLine
-			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
+			mockInputContent = fileContent
 
 			// Execute
-			const result = await executeReadFileTool(10, 5)
+			const result = await executeReadFileTool({}, { maxReadFileLine: 10, totalLines: 5 })
 
 			// Verify
 			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
@@ -214,69 +286,82 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		it("should read with extractTextFromFile when file has few lines", async () => {
 			// Setup
 			mockedCountFileLines.mockResolvedValue(3) // File shorter than maxReadFileLine
-			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
+			mockInputContent = fileContent
 
 			// Execute
-			const result = await executeReadFileTool(5, 3)
+			const result = await executeReadFileTool({}, { maxReadFileLine: 5, totalLines: 3 })
 
 			// Verify
 			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
 			expect(mockedReadLines).not.toHaveBeenCalled()
-			expect(result).toBe(expectedFullFileXml)
+			// Create a custom expected XML with lines="1-3" since totalLines is 3
+			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
+			expect(result).toBe(expectedXml)
 		})
 	})
 
 	describe("when file is binary", () => {
 		it("should always use extractTextFromFile regardless of maxReadFileLine", async () => {
 			// Setup
 			mockedIsBinaryFile.mockResolvedValue(true)
-			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
+			// For binary files, we're using a maxReadFileLine of 3 and totalLines is assumed to be 3
+			mockedCountFileLines.mockResolvedValue(3)
+
+			// For binary files, we need a special mock implementation that doesn't use addLineNumbers
+			// Save the original mock implementation
+			const originalMockImplementation = mockedExtractTextFromFile.getMockImplementation()
+			// Create a special mock implementation that doesn't call addLineNumbers
+			mockedExtractTextFromFile.mockImplementation(() => {
+				return Promise.resolve(numberedFileContent)
+			})
+
+			// Reset the spy to clear any previous calls
+			addLineNumbersSpy.mockClear()
+
+			// Execute - skip addLineNumbers check as we're directly providing the numbered content
+			const result = await executeReadFileTool(
+				{},
+				{
+					maxReadFileLine: 3,
+					totalLines: 3,
+					skipAddLineNumbersCheck: true,
+				},
+			)
 
-			// Execute
-			const result = await executeReadFileTool(3)
+			// Restore the original mock implementation after the test
+			mockedExtractTextFromFile.mockImplementation(originalMockImplementation)
 
 			// Verify
 			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
 			expect(mockedReadLines).not.toHaveBeenCalled()
-			expect(result).toBe(expectedFullFileXml)
+			// Create a custom expected XML with lines="1-3" for binary files
+			const expectedXml = `<file><path>${testFilePath}</path>\n<content lines="1-3">\n${numberedFileContent}</content>\n</file>`
+			expect(result).toBe(expectedXml)
 		})
 	})
 
 	describe("with range parameters", () => {
 		it("should honor start_line and end_line when provided", async () => {
 			// Setup
-			const rangeToolUse: ReadFileToolUse = {
-				type: "tool_use",
-				name: "read_file",
-				params: {
-					path: testFilePath,
-					start_line: "2",
-					end_line: "4",
-				},
-				partial: false,
-			}
-
 			mockedReadLines.mockResolvedValue("Line 2\nLine 3\nLine 4")
 
-			// Import the tool implementation dynamically
-			const { readFileTool } = require("../tools/readFileTool")
-
-			// Execute the tool
-			let rangeResult: string | undefined
-			await readFileTool(
-				mockCline,
-				rangeToolUse,
-				mockCline.ask,
-				jest.fn(),
-				(result: string) => {
-					rangeResult = result
-				},
-				(param: string, value: string) => value,
-			)
+			// Execute using executeReadFileTool with range parameters
+			const rangeResult = await executeReadFileTool({
+				start_line: "2",
+				end_line: "4",
+			})
 
 			// Verify
 			expect(mockedReadLines).toHaveBeenCalledWith(absoluteFilePath, 3, 1) // end_line - 1, start_line - 1
-			expect(mockedAddLineNumbers).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers
+			expect(addLineNumbersSpy).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers
+
+			// Verify XML structure with lines attribute
+			expect(rangeResult).toContain(`<file><path>${testFilePath}</path>`)
+			expect(rangeResult).toContain(`<content lines="2-4">`)
+			expect(rangeResult).toContain("2 | Line 2")
+			expect(rangeResult).toContain("3 | Line 3")
+			expect(rangeResult).toContain("4 | Line 4")
+			expect(rangeResult).toContain("</content>")
 		})
 	})
 })