Explorar el Código

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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* test: remove unused mockInputContent variable

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

Signed-off-by: Eric Wheeler <[email protected]>

---------

Signed-off-by: Eric Wheeler <[email protected]>
Co-authored-by: Eric Wheeler <[email protected]>
KJ7LNW hace 10 meses
padre
commit
270fd88cc8

+ 141 - 56
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,11 +251,21 @@ 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())
 		})
 	})
 
@@ -201,10 +273,10 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		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,15 +286,17 @@ 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)
 		})
 	})
 
@@ -230,53 +304,64 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		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>")
 		})
 	})
 })

+ 614 - 0
src/core/__tests__/read-file-xml.test.ts

@@ -0,0 +1,614 @@
+import * as path from "path"
+import { countFileLines } from "../../integrations/misc/line-counter"
+import { readLines } from "../../integrations/misc/read-lines"
+import { extractTextFromFile, addLineNumbers } from "../../integrations/misc/extract-text"
+import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter"
+import { isBinaryFile } from "isbinaryfile"
+import { ReadFileToolUse } from "../assistant-message"
+import { Cline } from "../Cline"
+
+// Mock dependencies
+jest.mock("../../integrations/misc/line-counter")
+jest.mock("../../integrations/misc/read-lines")
+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().mockImplementation((filePath) => {
+			// Use the actual addLineNumbers function
+			const content = mockInputContent
+			return Promise.resolve(actual.addLineNumbers(content))
+		}),
+	}
+})
+
+// Get a reference to the spy
+const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy
+
+// Variable to control what content is used by the mock
+let mockInputContent = ""
+jest.mock("../../services/tree-sitter")
+jest.mock("isbinaryfile")
+jest.mock("../ignore/RooIgnoreController", () => ({
+	RooIgnoreController: class {
+		initialize() {
+			return Promise.resolve()
+		}
+		validateAccess() {
+			return true
+		}
+	},
+}))
+jest.mock("fs/promises", () => ({
+	mkdir: jest.fn().mockResolvedValue(undefined),
+	writeFile: jest.fn().mockResolvedValue(undefined),
+	readFile: jest.fn().mockResolvedValue("{}"),
+}))
+jest.mock("../../utils/fs", () => ({
+	fileExistsAtPath: jest.fn().mockReturnValue(true),
+}))
+
+// Mock path
+jest.mock("path", () => {
+	const originalPath = jest.requireActual("path")
+	return {
+		...originalPath,
+		resolve: jest.fn().mockImplementation((...args) => args.join("/")),
+	}
+})
+
+describe("read_file tool XML output structure", () => {
+	// Test data
+	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\n"
+	const sourceCodeDef = "\n\n# file.txt\n1--5 | Content"
+
+	// 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>
+	const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction<
+		typeof parseSourceCodeDefinitionsForFile
+	>
+	const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction<typeof isBinaryFile>
+	const mockedPathResolve = path.resolve as jest.MockedFunction<typeof path.resolve>
+
+	// Mock instances
+	const mockCline: any = {}
+	let mockProvider: any
+	let toolResult: string | undefined
+
+	beforeEach(() => {
+		jest.clearAllMocks()
+
+		// Setup path resolution
+		mockedPathResolve.mockReturnValue(absoluteFilePath)
+
+		// Setup mocks for file operations
+		mockedIsBinaryFile.mockResolvedValue(false)
+
+		// Set the default content for the mock
+		mockInputContent = fileContent
+
+		// Setup mock provider
+		mockProvider = {
+			getState: jest.fn().mockResolvedValue({ maxReadFileLine: 500 }),
+			deref: jest.fn().mockReturnThis(),
+		}
+
+		// Setup Cline instance with mock methods
+		mockCline.cwd = "/"
+		mockCline.task = "Test"
+		mockCline.providerRef = mockProvider
+		mockCline.rooIgnoreController = {
+			validateAccess: jest.fn().mockReturnValue(true),
+		}
+		mockCline.say = jest.fn().mockResolvedValue(undefined)
+		mockCline.ask = jest.fn().mockResolvedValue(true)
+		mockCline.presentAssistantMessage = jest.fn()
+		mockCline.sayAndCreateMissingParamError = jest.fn().mockResolvedValue("Missing required parameter")
+
+		// Reset tool result
+		toolResult = undefined
+	})
+
+	/**
+	 * Helper function to execute the read file tool with custom parameters
+	 */
+	async function executeReadFileTool(
+		params: Partial<ReadFileToolUse["params"]> = {},
+		options: {
+			totalLines?: number
+			maxReadFileLine?: number
+			isBinary?: boolean
+			validateAccess?: boolean
+			skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check
+		} = {},
+	): Promise<string | undefined> {
+		// Configure mocks based on test scenario
+		const totalLines = options.totalLines ?? 5
+		const maxReadFileLine = options.maxReadFileLine ?? 500
+		const isBinary = options.isBinary ?? false
+		const validateAccess = options.validateAccess ?? true
+
+		mockProvider.getState.mockResolvedValue({ maxReadFileLine })
+		mockedCountFileLines.mockResolvedValue(totalLines)
+		mockedIsBinaryFile.mockResolvedValue(isBinary)
+		mockCline.rooIgnoreController.validateAccess = jest.fn().mockReturnValue(validateAccess)
+
+		// Create a tool use object
+		const toolUse: ReadFileToolUse = {
+			type: "tool_use",
+			name: "read_file",
+			params: {
+				path: testFilePath,
+				...params,
+			},
+			partial: false,
+		}
+
+		// Import the tool implementation dynamically to avoid hoisting issues
+		const { readFileTool } = require("../tools/readFileTool")
+
+		// Reset the spy's call history before each test
+		addLineNumbersSpy.mockClear()
+
+		// Execute the tool
+		await readFileTool(
+			mockCline,
+			toolUse,
+			mockCline.ask,
+			jest.fn(),
+			(result: string) => {
+				toolResult = result
+			},
+			(param: string, value: string) => value,
+		)
+		// Verify addLineNumbers was called (unless explicitly skipped)
+		if (!options.skipAddLineNumbersCheck) {
+			expect(addLineNumbersSpy).toHaveBeenCalled()
+		} else {
+			// For cases where we expect addLineNumbers NOT to be called
+			expect(addLineNumbersSpy).not.toHaveBeenCalled()
+		}
+
+		return toolResult
+	}
+
+	describe("Basic XML Structure Tests", () => {
+		it("should produce XML output with no unnecessary indentation", async () => {
+			// Setup - use default mockInputContent (fileContent)
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool()
+
+			// Verify
+			expect(result).toBe(
+				`<file><path>${testFilePath}</path>\n<content lines="1-5">\n${numberedFileContent}</content>\n</file>`,
+			)
+		})
+
+		it("should follow the correct XML structure format", async () => {
+			// Setup - use default mockInputContent (fileContent)
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool()
+
+			// Verify using regex to check structure
+			const xmlStructureRegex = new RegExp(
+				`^<file><path>${testFilePath}</path>\\n<content lines="1-5">\\n.*</content>\\n</file>$`,
+				"s",
+			)
+			expect(result).toMatch(xmlStructureRegex)
+		})
+	})
+
+	describe("Line Range Tests", () => {
+		it("should include lines attribute when start_line is specified", async () => {
+			// Setup
+			const startLine = 2
+			mockedReadLines.mockResolvedValue(
+				fileContent
+					.split("\n")
+					.slice(startLine - 1)
+					.join("\n"),
+			)
+
+			// Execute
+			const result = await executeReadFileTool({ start_line: startLine.toString() })
+
+			// Verify
+			expect(result).toContain(`<content lines="${startLine}-5">`)
+		})
+
+		it("should include lines attribute when end_line is specified", async () => {
+			// Setup
+			const endLine = 3
+			mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, endLine).join("\n"))
+
+			// Execute
+			const result = await executeReadFileTool({ end_line: endLine.toString() })
+
+			// Verify
+			expect(result).toContain(`<content lines="1-${endLine}">`)
+		})
+
+		it("should include lines attribute when both start_line and end_line are specified", async () => {
+			// Setup
+			const startLine = 2
+			const endLine = 4
+			mockedReadLines.mockResolvedValue(
+				fileContent
+					.split("\n")
+					.slice(startLine - 1, endLine)
+					.join("\n"),
+			)
+
+			// Execute
+			const result = await executeReadFileTool({
+				start_line: startLine.toString(),
+				end_line: endLine.toString(),
+			})
+
+			// Verify
+			expect(result).toContain(`<content lines="${startLine}-${endLine}">`)
+		})
+
+		it("should include lines attribute even when no range is specified", async () => {
+			// Setup - use default mockInputContent (fileContent)
+			mockInputContent = fileContent
+
+			// Execute
+			const result = await executeReadFileTool()
+
+			// Verify
+			expect(result).toContain(`<content lines="1-5">\n`)
+		})
+
+		it("should include content when maxReadFileLine=0 and range is specified", async () => {
+			// Setup
+			const maxReadFileLine = 0
+			const startLine = 2
+			const endLine = 4
+			const totalLines = 10
+
+			mockedReadLines.mockResolvedValue(
+				fileContent
+					.split("\n")
+					.slice(startLine - 1, endLine)
+					.join("\n"),
+			)
+
+			// Execute
+			const result = await executeReadFileTool(
+				{
+					start_line: startLine.toString(),
+					end_line: endLine.toString(),
+				},
+				{ maxReadFileLine, totalLines },
+			)
+
+			// Verify
+			// Should include content tag with line range
+			expect(result).toContain(`<content lines="${startLine}-${endLine}">`)
+
+			// Should NOT include definitions (range reads never show definitions)
+			expect(result).not.toContain("<list_code_definition_names>")
+
+			// Should NOT include truncation notice
+			expect(result).not.toContain(`<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines`)
+		})
+
+		it("should include content when maxReadFileLine=0 and only start_line is specified", async () => {
+			// Setup
+			const maxReadFileLine = 0
+			const startLine = 3
+			const totalLines = 10
+
+			mockedReadLines.mockResolvedValue(
+				fileContent
+					.split("\n")
+					.slice(startLine - 1)
+					.join("\n"),
+			)
+
+			// Execute
+			const result = await executeReadFileTool(
+				{
+					start_line: startLine.toString(),
+				},
+				{ maxReadFileLine, totalLines },
+			)
+
+			// Verify
+			// Should include content tag with line range
+			expect(result).toContain(`<content lines="${startLine}-${totalLines}">`)
+
+			// Should NOT include definitions (range reads never show definitions)
+			expect(result).not.toContain("<list_code_definition_names>")
+
+			// Should NOT include truncation notice
+			expect(result).not.toContain(`<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines`)
+		})
+
+		it("should include content when maxReadFileLine=0 and only end_line is specified", async () => {
+			// Setup
+			const maxReadFileLine = 0
+			const endLine = 3
+			const totalLines = 10
+
+			mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, endLine).join("\n"))
+
+			// Execute
+			const result = await executeReadFileTool(
+				{
+					end_line: endLine.toString(),
+				},
+				{ maxReadFileLine, totalLines },
+			)
+
+			// Verify
+			// Should include content tag with line range
+			expect(result).toContain(`<content lines="1-${endLine}">`)
+
+			// Should NOT include definitions (range reads never show definitions)
+			expect(result).not.toContain("<list_code_definition_names>")
+
+			// Should NOT include truncation notice
+			expect(result).not.toContain(`<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines`)
+		})
+
+		it("should include full range content when maxReadFileLine=5 and content has more than 5 lines", async () => {
+			// Setup
+			const maxReadFileLine = 5
+			const startLine = 2
+			const endLine = 8
+			const totalLines = 10
+
+			// Create mock content with 7 lines (more than maxReadFileLine)
+			const rangeContent = Array(endLine - startLine + 1)
+				.fill("Range line content")
+				.join("\n")
+
+			mockedReadLines.mockResolvedValue(rangeContent)
+
+			// Execute
+			const result = await executeReadFileTool(
+				{
+					start_line: startLine.toString(),
+					end_line: endLine.toString(),
+				},
+				{ maxReadFileLine, totalLines },
+			)
+
+			// Verify
+			// Should include content tag with the full requested range (not limited by maxReadFileLine)
+			expect(result).toContain(`<content lines="${startLine}-${endLine}">`)
+
+			// Should NOT include definitions (range reads never show definitions)
+			expect(result).not.toContain("<list_code_definition_names>")
+
+			// Should NOT include truncation notice
+			expect(result).not.toContain(`<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines`)
+
+			// Should contain all the requested lines, not just maxReadFileLine lines
+			expect(result).toBeDefined()
+			if (result) {
+				expect(result.split("\n").length).toBeGreaterThan(maxReadFileLine)
+			}
+		})
+	})
+
+	describe("Notice and Definition Tags Tests", () => {
+		it("should include notice tag for truncated files", async () => {
+			// Setup
+			const maxReadFileLine = 3
+			const totalLines = 10
+			mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, maxReadFileLine).join("\n"))
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines })
+
+			// Verify
+			expect(result).toContain(`<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines`)
+		})
+
+		it("should include list_code_definition_names tag when source code definitions are available", async () => {
+			// Setup
+			const maxReadFileLine = 3
+			const totalLines = 10
+			mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, maxReadFileLine).join("\n"))
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines })
+
+			// Verify
+			// Use regex to match the tag content regardless of whitespace
+			expect(result).toMatch(
+				new RegExp(
+					`<list_code_definition_names>[\\s\\S]*${sourceCodeDef.trim()}[\\s\\S]*</list_code_definition_names>`,
+				),
+			)
+		})
+
+		it("should only have definitions, no content when maxReadFileLine=0", async () => {
+			// Setup
+			const maxReadFileLine = 0
+			const totalLines = 10
+			// Mock content with exactly 10 lines to match totalLines
+			const rawContent = Array(10).fill("Line content").join("\n")
+			mockInputContent = rawContent
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines, skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toContain(`<notice>Showing only 0 of ${totalLines} total lines`)
+			// Use regex to match the tag content regardless of whitespace
+			expect(result).toMatch(
+				new RegExp(
+					`<list_code_definition_names>[\\s\\S]*${sourceCodeDef.trim()}[\\s\\S]*</list_code_definition_names>`,
+				),
+			)
+			expect(result).not.toContain(`<content`)
+		})
+
+		it("should handle maxReadFileLine=0 with no source code definitions", async () => {
+			// Setup
+			const maxReadFileLine = 0
+			const totalLines = 10
+			// Mock that no source code definitions are available
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue("")
+			// Mock content with exactly 10 lines to match totalLines
+			const rawContent = Array(10).fill("Line content").join("\n")
+			mockInputContent = rawContent
+
+			// Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines, skipAddLineNumbersCheck: true })
+
+			// Verify
+			// Should include notice
+			expect(result).toContain(
+				`<file><path>${testFilePath}</path>\n<notice>Showing only 0 of ${totalLines} total lines. Use start_line and end_line if you need to read more</notice>\n</file>`,
+			)
+			// Should not include list_code_definition_names tag since there are no definitions
+			expect(result).not.toContain("<list_code_definition_names>")
+			// Should not include content tag for non-empty files with maxReadFileLine=0
+			expect(result).not.toContain("<content")
+		})
+	})
+
+	describe("Error Handling Tests", () => {
+		it("should include error tag for invalid path", async () => {
+			// Setup - missing path parameter
+			const toolUse: ReadFileToolUse = {
+				type: "tool_use",
+				name: "read_file",
+				params: {},
+				partial: false,
+			}
+
+			// Import the tool implementation dynamically
+			const { readFileTool } = require("../tools/readFileTool")
+
+			// Execute the tool
+			await readFileTool(
+				mockCline,
+				toolUse,
+				mockCline.ask,
+				jest.fn(),
+				(result: string) => {
+					toolResult = result
+				},
+				(param: string, value: string) => value,
+			)
+
+			// Verify
+			expect(toolResult).toContain(`<file><path></path><error>`)
+			expect(toolResult).not.toContain(`<content`)
+		})
+
+		it("should include error tag for invalid start_line", async () => {
+			// Execute - skip addLineNumbers check as it returns early with an error
+			const result = await executeReadFileTool({ start_line: "invalid" }, { skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toContain(`<file><path>${testFilePath}</path><error>Invalid start_line value</error></file>`)
+			expect(result).not.toContain(`<content`)
+		})
+
+		it("should include error tag for invalid end_line", async () => {
+			// Execute - skip addLineNumbers check as it returns early with an error
+			const result = await executeReadFileTool({ end_line: "invalid" }, { skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toContain(`<file><path>${testFilePath}</path><error>Invalid end_line value</error></file>`)
+			expect(result).not.toContain(`<content`)
+		})
+
+		it("should include error tag for RooIgnore error", async () => {
+			// Execute - skip addLineNumbers check as it returns early with an error
+			const result = await executeReadFileTool({}, { validateAccess: false, skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toContain(`<file><path>${testFilePath}</path><error>`)
+			expect(result).not.toContain(`<content`)
+		})
+	})
+
+	describe("Edge Cases Tests", () => {
+		it("should handle empty files correctly with maxReadFileLine=-1", async () => {
+			// Setup - use empty string
+			mockInputContent = ""
+			const maxReadFileLine = -1
+			const totalLines = 0
+			mockedCountFileLines.mockResolvedValue(totalLines)
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines })
+
+			// Verify
+			// Empty files should include a content tag and notice
+			expect(result).toBe(`<file><path>${testFilePath}</path>\n<content/><notice>File is empty</notice>\n</file>`)
+			// And make sure there's no error
+			expect(result).not.toContain(`<error>`)
+		})
+
+		it("should handle empty files correctly with maxReadFileLine=0", async () => {
+			// Setup - use empty string
+			mockInputContent = ""
+			const maxReadFileLine = 0
+			const totalLines = 0
+			mockedCountFileLines.mockResolvedValue(totalLines)
+
+			// Execute
+			const result = await executeReadFileTool({}, { maxReadFileLine, totalLines })
+
+			// Verify
+			// Empty files should include a content tag and notice even with maxReadFileLine=0
+			expect(result).toBe(`<file><path>${testFilePath}</path>\n<content/><notice>File is empty</notice>\n</file>`)
+		})
+
+		it("should handle binary files correctly", async () => {
+			// Setup
+			// For binary content, we need to override the mock since we don't use addLineNumbers
+			mockedExtractTextFromFile.mockResolvedValue("Binary content")
+
+			// Execute - skip addLineNumbers check as we're directly mocking extractTextFromFile
+			const result = await executeReadFileTool({}, { isBinary: true, skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toBe(
+				`<file><path>${testFilePath}</path>\n<content lines="1-5">\nBinary content</content>\n</file>`,
+			)
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+		})
+
+		it("should handle file read errors correctly", async () => {
+			// Setup
+			const errorMessage = "File not found"
+			// For error cases, we need to override the mock to simulate a failure
+			mockedExtractTextFromFile.mockRejectedValue(new Error(errorMessage))
+
+			// Execute - skip addLineNumbers check as it throws an error
+			const result = await executeReadFileTool({}, { skipAddLineNumbersCheck: true })
+
+			// Verify
+			expect(result).toContain(
+				`<file><path>${testFilePath}</path><error>Error reading file: ${errorMessage}</error></file>`,
+			)
+			expect(result).not.toContain(`<content`)
+		})
+	})
+})

+ 55 - 8
src/core/tools/readFileTool.ts

@@ -45,7 +45,8 @@ export async function readFileTool(
 		} else {
 			if (!relPath) {
 				cline.consecutiveMistakeCount++
-				pushToolResult(await cline.sayAndCreateMissingParamError("read_file", "path"))
+				const errorMsg = await cline.sayAndCreateMissingParamError("read_file", "path")
+				pushToolResult(`<file><path></path><error>${errorMsg}</error></file>`)
 				return
 			}
 
@@ -66,7 +67,7 @@ export async function readFileTool(
 					// Invalid start_line
 					cline.consecutiveMistakeCount++
 					await cline.say("error", `Failed to parse start_line: ${startLineStr}`)
-					pushToolResult(formatResponse.toolError("Invalid start_line value"))
+					pushToolResult(`<file><path>${relPath}</path><error>Invalid start_line value</error></file>`)
 					return
 				}
 				startLine -= 1 // Convert to 0-based index
@@ -80,7 +81,7 @@ export async function readFileTool(
 					// Invalid end_line
 					cline.consecutiveMistakeCount++
 					await cline.say("error", `Failed to parse end_line: ${endLineStr}`)
-					pushToolResult(formatResponse.toolError("Invalid end_line value"))
+					pushToolResult(`<file><path>${relPath}</path><error>Invalid end_line value</error></file>`)
 					return
 				}
 
@@ -91,8 +92,8 @@ export async function readFileTool(
 			const accessAllowed = cline.rooIgnoreController?.validateAccess(relPath)
 			if (!accessAllowed) {
 				await cline.say("rooignore_error", relPath)
-				pushToolResult(formatResponse.toolError(formatResponse.rooIgnoreError(relPath)))
-
+				const errorMsg = formatResponse.rooIgnoreError(relPath)
+				pushToolResult(`<file><path>${relPath}</path><error>${errorMsg}</error></file>`)
 				return
 			}
 
@@ -159,23 +160,69 @@ export async function readFileTool(
 				content = res[0].length > 0 ? addLineNumbers(res[0]) : ""
 				const result = res[1]
 				if (result) {
-					sourceCodeDef = `\n\n${result}`
+					sourceCodeDef = `${result}`
 				}
 			} else {
 				// Read entire file
 				content = await extractTextFromFile(absolutePath)
 			}
 
+			// Create variables to store XML components
+			let xmlInfo = ""
+			let contentTag = ""
+
 			// Add truncation notice if applicable
 			if (isFileTruncated) {
-				content += `\n\n[Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more]${sourceCodeDef}`
+				xmlInfo += `<notice>Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more</notice>\n`
+
+				// Add source code definitions if available
+				if (sourceCodeDef) {
+					xmlInfo += `<list_code_definition_names>${sourceCodeDef}</list_code_definition_names>\n`
+				}
+			}
+
+			// Empty files (zero lines)
+			if (content === "" && totalLines === 0) {
+				// Always add self-closing content tag and notice for empty files
+				contentTag = `<content/>`
+				xmlInfo += `<notice>File is empty</notice>\n`
+			}
+			// Range reads should always show content regardless of maxReadFileLine
+			else if (isRangeRead) {
+				// Create content tag with line range information
+				let lineRangeAttr = ""
+				const displayStartLine = startLine !== undefined ? startLine + 1 : 1
+				const displayEndLine = endLine !== undefined ? endLine + 1 : totalLines
+				lineRangeAttr = ` lines="${displayStartLine}-${displayEndLine}"`
+
+				// Maintain exact format expected by tests
+				contentTag = `<content${lineRangeAttr}>\n${content}</content>\n`
+			}
+			// maxReadFileLine=0 for non-range reads
+			else if (maxReadFileLine === 0) {
+				// Skip content tag for maxReadFileLine=0 (definitions only mode)
+				contentTag = ""
+			}
+			// Normal case: non-empty files with content (non-range reads)
+			else {
+				// For non-range reads, always show line range
+				let lines = totalLines
+				if (maxReadFileLine >= 0 && totalLines > maxReadFileLine) {
+					lines = maxReadFileLine
+				}
+				const lineRangeAttr = ` lines="1-${lines}"`
+
+				// Maintain exact format expected by tests
+				contentTag = `<content${lineRangeAttr}>\n${content}</content>\n`
 			}
 
 			// Format the result into the required XML structure
-			const xmlResult = `<file>\n  <path>${relPath}</path>\n  <content>\n${content}\n  </content>\n</file>`
+			const xmlResult = `<file><path>${relPath}</path>\n${contentTag}${xmlInfo}</file>`
 			pushToolResult(xmlResult)
 		}
 	} catch (error) {
+		const errorMsg = error instanceof Error ? error.message : String(error)
+		pushToolResult(`<file><path>${relPath || ""}</path><error>Error reading file: ${errorMsg}</error></file>`)
 		await handleError("reading file", error)
 	}
 }

+ 38 - 7
src/integrations/misc/__tests__/extract-text.test.ts

@@ -9,32 +9,63 @@ import {
 describe("addLineNumbers", () => {
 	it("should add line numbers starting from 1 by default", () => {
 		const input = "line 1\nline 2\nline 3"
-		const expected = "1 | line 1\n2 | line 2\n3 | line 3"
+		const expected = "1 | line 1\n2 | line 2\n3 | line 3\n"
 		expect(addLineNumbers(input)).toBe(expected)
 	})
 
 	it("should add line numbers starting from specified line number", () => {
 		const input = "line 1\nline 2\nline 3"
-		const expected = "10 | line 1\n11 | line 2\n12 | line 3"
+		const expected = "10 | line 1\n11 | line 2\n12 | line 3\n"
 		expect(addLineNumbers(input, 10)).toBe(expected)
 	})
 
 	it("should handle empty content", () => {
-		expect(addLineNumbers("")).toBe("1 | ")
-		expect(addLineNumbers("", 5)).toBe("5 | ")
+		expect(addLineNumbers("")).toBe("")
+		expect(addLineNumbers("", 5)).toBe("5 | \n")
 	})
 
 	it("should handle single line content", () => {
-		expect(addLineNumbers("single line")).toBe("1 | single line")
-		expect(addLineNumbers("single line", 42)).toBe("42 | single line")
+		expect(addLineNumbers("single line")).toBe("1 | single line\n")
+		expect(addLineNumbers("single line", 42)).toBe("42 | single line\n")
 	})
 
 	it("should pad line numbers based on the highest line number", () => {
 		const input = "line 1\nline 2"
 		// When starting from 99, highest line will be 100, so needs 3 spaces padding
-		const expected = " 99 | line 1\n100 | line 2"
+		const expected = " 99 | line 1\n100 | line 2\n"
 		expect(addLineNumbers(input, 99)).toBe(expected)
 	})
+
+	it("should preserve trailing newline without adding extra line numbers", () => {
+		const input = "line 1\nline 2\n"
+		const expected = "1 | line 1\n2 | line 2\n"
+		expect(addLineNumbers(input)).toBe(expected)
+	})
+
+	it("should handle multiple blank lines correctly", () => {
+		const input = "line 1\n\n\n\nline 2"
+		const expected = "1 | line 1\n2 | \n3 | \n4 | \n5 | line 2\n"
+		expect(addLineNumbers(input)).toBe(expected)
+	})
+
+	it("should handle multiple trailing newlines correctly", () => {
+		const input = "line 1\nline 2\n\n\n"
+		const expected = "1 | line 1\n2 | line 2\n3 | \n4 | \n"
+		expect(addLineNumbers(input)).toBe(expected)
+	})
+
+	it("should handle numbered trailing newline correctly", () => {
+		const input = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n\n"
+		const expected =
+			" 1 | Line 1\n 2 | Line 2\n 3 | Line 3\n 4 | Line 4\n 5 | Line 5\n 6 | Line 6\n 7 | Line 7\n 8 | Line 8\n 9 | Line 9\n10 | Line 10\n11 | \n"
+		expect(addLineNumbers(input)).toBe(expected)
+	})
+
+	it("should handle only blank lines with offset correctly", () => {
+		const input = "\n\n\n"
+		const expected = "10 | \n11 | \n12 | \n"
+		expect(addLineNumbers(input, 10)).toBe(expected)
+	})
 })
 
 describe("everyLineHasLineNumbers", () => {

+ 70 - 6
src/integrations/misc/__tests__/read-lines.test.ts

@@ -18,17 +18,23 @@ describe("nthline", () => {
 	describe("readLines function", () => {
 		it("should read lines from start when from_line is not provided", async () => {
 			const lines = await readLines(testFile, 2)
-			expect(lines).toEqual(["Line 1", "Line 2", "Line 3"].join("\n"))
+			// Expect lines with trailing newline because it exists in the file at that point
+			const expected = ["Line 1", "Line 2", "Line 3"].join("\n") + "\n"
+			expect(lines).toEqual(expected)
 		})
 
 		it("should read a range of lines from a file", async () => {
 			const lines = await readLines(testFile, 3, 1)
-			expect(lines).toEqual(["Line 2", "Line 3", "Line 4"].join("\n"))
+			// Expect lines with trailing newline because it exists in the file at that point
+			const expected = ["Line 2", "Line 3", "Line 4"].join("\n") + "\n"
+			expect(lines).toEqual(expected)
 		})
 
 		it("should read lines when to_line equals from_line", async () => {
 			const lines = await readLines(testFile, 2, 2)
-			expect(lines).toEqual("Line 3")
+			// Expect line with trailing newline because it exists in the file at that point
+			const expected = "Line 3\n"
+			expect(lines).toEqual(expected)
 		})
 
 		it("should throw error for negative to_line", async () => {
@@ -39,15 +45,15 @@ describe("nthline", () => {
 
 		it("should handle negative from_line by clamping to 0", async () => {
 			const lines = await readLines(testFile, 3, -1)
-			expect(lines).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n"))
+			expect(lines).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n") + "\n")
 		})
 
 		it("should floor non-integer line numbers", async () => {
 			const linesWithNonIntegerStart = await readLines(testFile, 3, 1.5)
-			expect(linesWithNonIntegerStart).toEqual(["Line 2", "Line 3", "Line 4"].join("\n"))
+			expect(linesWithNonIntegerStart).toEqual(["Line 2", "Line 3", "Line 4"].join("\n") + "\n")
 
 			const linesWithNonIntegerEnd = await readLines(testFile, 3.5)
-			expect(linesWithNonIntegerEnd).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n"))
+			expect(linesWithNonIntegerEnd).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n") + "\n")
 		})
 
 		it("should throw error when from_line > to_line", async () => {
@@ -64,5 +70,63 @@ describe("nthline", () => {
 		it("should throw error if from_line is beyond file length", async () => {
 			await expect(readLines(testFile, 20, 15)).rejects.toThrow("does not exist")
 		})
+
+		// Helper function to create a temporary file, run a test, and clean up
+		async function withTempFile(filename: string, content: string, testFn: (filepath: string) => Promise<void>) {
+			const filepath = path.join(__dirname, filename)
+			await fs.writeFile(filepath, content)
+			try {
+				await testFn(filepath)
+			} finally {
+				await fs.unlink(filepath)
+			}
+		}
+
+		it("should handle empty files", async () => {
+			await withTempFile("empty.txt", "", async (filepath) => {
+				await expect(readLines(filepath, 0, 0)).rejects.toThrow("does not exist")
+			})
+		})
+
+		it("should handle files with only one line without carriage return", async () => {
+			await withTempFile("single-line-no-cr.txt", "Single line", async (filepath) => {
+				const lines = await readLines(filepath, 0, 0)
+				expect(lines).toEqual("Single line")
+			})
+		})
+
+		it("should handle files with only one line with carriage return", async () => {
+			await withTempFile("single-line-with-cr.txt", "Single line\n", async (filepath) => {
+				const lines = await readLines(filepath, 0, 0)
+				expect(lines).toEqual("Single line\n")
+			})
+		})
+
+		it("should read the entire file when no startLine or endLine is specified", async () => {
+			const content = await readLines(testFile)
+			expect(content).toEqual(Array.from({ length: 10 }, (_, i) => `Line ${i + 1}`).join("\n"))
+		})
+
+		it("should handle files with different line endings", async () => {
+			await withTempFile("mixed-endings.txt", "Line 1\rLine 2\r\nLine 3\n", async (filepath) => {
+				const lines = await readLines(filepath, 2)
+				expect(lines).toEqual("Line 1\rLine 2\r\nLine 3\n")
+			})
+		})
+
+		it("should handle files with Unicode characters", async () => {
+			await withTempFile("unicode.txt", "Line 1 😀\nLine 2 你好\nLine 3 こんにちは\n", async (filepath) => {
+				const lines = await readLines(filepath, 1)
+				expect(lines).toEqual("Line 1 😀\nLine 2 你好\n")
+			})
+		})
+
+		it("should handle files containing only carriage returns", async () => {
+			await withTempFile("cr-only.txt", "\n\n\n\n\n", async (filepath) => {
+				// Read lines 1-3 (second, third, and fourth lines)
+				const lines = await readLines(filepath, 3, 1)
+				expect(lines).toEqual("\n\n\n")
+			})
+		})
 	})
 })

+ 16 - 1
src/integrations/misc/extract-text.ts

@@ -55,14 +55,29 @@ async function extractTextFromIPYNB(filePath: string): Promise<string> {
 }
 
 export function addLineNumbers(content: string, startLine: number = 1): string {
+	// If content is empty, return empty string - empty files should not have line numbers
+	// If content is empty but startLine > 1, return "startLine | " because we know the file is not empty
+	// but the content is empty at that line offset
+	if (content === "") {
+		return startLine === 1 ? "" : `${startLine} | \n`
+	}
+
+	// Split into lines and handle trailing newlines
 	const lines = content.split("\n")
+	const lastLineEmpty = lines[lines.length - 1] === ""
+	if (lastLineEmpty) {
+		lines.pop()
+	}
+
 	const maxLineNumberWidth = String(startLine + lines.length - 1).length
-	return lines
+	const numberedContent = lines
 		.map((line, index) => {
 			const lineNumber = String(startLine + index).padStart(maxLineNumberWidth, " ")
 			return `${lineNumber} | ${line}`
 		})
 		.join("\n")
+
+	return numberedContent + "\n"
 }
 // Checks if every line in the content has line numbers prefixed (e.g., "1 | content" or "123 | content")
 // Line numbers must be followed by a single pipe character (not double pipes)

+ 49 - 20
src/integrations/misc/read-lines.ts

@@ -53,35 +53,64 @@ export function readLines(filepath: string, endLine?: number, startLine?: number
 			)
 		}
 
-		let cursor = 0
-		const lines: string[] = []
+		// Set up stream
 		const input = createReadStream(filepath)
-		const rl = createInterface({ input })
+		let buffer = ""
+		let lineCount = 0
+		let result = ""
 
-		rl.on("line", (line) => {
-			// Only collect lines within the specified range
-			if (cursor >= effectiveStartLine && (endLine === undefined || cursor <= endLine)) {
-				lines.push(line)
-			}
+		// Handle errors
+		input.on("error", reject)
+
+		// Process data chunks directly
+		input.on("data", (chunk) => {
+			// Add chunk to buffer
+			buffer += chunk.toString()
+
+			let pos = 0
+			let nextNewline = buffer.indexOf("\n", pos)
+
+			// Process complete lines in the buffer
+			while (nextNewline !== -1) {
+				// If we're in the target range, add this line to the result
+				if (lineCount >= effectiveStartLine && (endLine === undefined || lineCount <= endLine)) {
+					result += buffer.substring(pos, nextNewline + 1) // Include the newline
+				}
+
+				// Move position and increment line counter
+				pos = nextNewline + 1
+				lineCount++
+
+				// If we've reached the end line, we can stop
+				if (endLine !== undefined && lineCount > endLine) {
+					input.destroy()
+					resolve(result)
+					return
+				}
 
-			// Close stream after reaching to_line (if specified)
-			if (endLine !== undefined && cursor === endLine) {
-				rl.close()
-				input.close()
-				resolve(lines.join("\n"))
+				// Find next newline
+				nextNewline = buffer.indexOf("\n", pos)
 			}
 
-			cursor++
+			// Trim buffer - keep only the incomplete line
+			buffer = buffer.substring(pos)
 		})
 
-		rl.on("error", reject)
-
+		// Handle end of file
 		input.on("end", () => {
-			// If we collected some lines but didn't reach to_line, return what we have
-			if (lines.length > 0) {
-				resolve(lines.join("\n"))
-			} else {
+			// Process any remaining data in buffer (last line without newline)
+			if (buffer.length > 0) {
+				if (lineCount >= effectiveStartLine && (endLine === undefined || lineCount <= endLine)) {
+					result += buffer
+				}
+				lineCount++
+			}
+
+			// Check if we found any lines in the requested range
+			if (lineCount <= effectiveStartLine) {
 				reject(outOfRangeError(filepath, effectiveStartLine))
+			} else {
+				resolve(result)
 			}
 		})
 	})