Extract code for read_file from Cline (#2059)

src/core/Cline.ts

@@ -30,6 +30,7 @@ import {
 } from "../integrations/misc/extract-text"
 import { countFileLines } from "../integrations/misc/line-counter"
 import { fetchInstructionsTool } from "./tools/fetchInstructionsTool"
+import { readFileTool } from "./tools/readFileTool"
 import { ExitCodeDetails } from "../integrations/terminal/TerminalProcess"
 import { Terminal } from "../integrations/terminal/Terminal"
 import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry"
@@ -82,9 +83,7 @@ import { insertGroups } from "./diff/insert-groups"
 import { telemetryService } from "../services/telemetry/TelemetryService"
 import { validateToolUse, isToolAllowedForMode, ToolName } from "./mode-validator"
 import { parseXml } from "../utils/xml"
-import { readLines } from "../integrations/misc/read-lines"
 import { getWorkspacePath } from "../utils/path"
-import { isBinaryFile } from "isbinaryfile"
 
 export type ToolResponse = string | Array<Anthropic.TextBlockParam | Anthropic.ImageBlockParam>
 type UserContent = Array<Anthropic.Messages.ContentBlockParam>
@@ -2256,151 +2255,8 @@ export class Cline extends EventEmitter<ClineEvents> {
 					}
 
 					case "read_file": {
-						const relPath: string | undefined = block.params.path
-						const startLineStr: string | undefined = block.params.start_line
-						const endLineStr: string | undefined = block.params.end_line
-
-						// Get the full path and determine if it's outside the workspace
-						const fullPath = relPath ? path.resolve(this.cwd, removeClosingTag("path", relPath)) : ""
-						const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
-
-						const sharedMessageProps: ClineSayTool = {
-							tool: "readFile",
-							path: getReadablePath(this.cwd, removeClosingTag("path", relPath)),
-							isOutsideWorkspace,
-						}
-						try {
-							if (block.partial) {
-								const partialMessage = JSON.stringify({
-									...sharedMessageProps,
-									content: undefined,
-								} satisfies ClineSayTool)
-								await this.ask("tool", partialMessage, block.partial).catch(() => {})
-								break
-							} else {
-								if (!relPath) {
-									this.consecutiveMistakeCount++
-									pushToolResult(await this.sayAndCreateMissingParamError("read_file", "path"))
-									break
-								}
-
-								// Check if we're doing a line range read
-								let isRangeRead = false
-								let startLine: number | undefined = undefined
-								let endLine: number | undefined = undefined
-
-								// Check if we have either range parameter
-								if (startLineStr || endLineStr) {
-									isRangeRead = true
-								}
-
-								// Parse start_line if provided
-								if (startLineStr) {
-									startLine = parseInt(startLineStr)
-									if (isNaN(startLine)) {
-										// Invalid start_line
-										this.consecutiveMistakeCount++
-										await this.say("error", `Failed to parse start_line: ${startLineStr}`)
-										pushToolResult(formatResponse.toolError("Invalid start_line value"))
-										break
-									}
-									startLine -= 1 // Convert to 0-based index
-								}
-
-								// Parse end_line if provided
-								if (endLineStr) {
-									endLine = parseInt(endLineStr)
-
-									if (isNaN(endLine)) {
-										// Invalid end_line
-										this.consecutiveMistakeCount++
-										await this.say("error", `Failed to parse end_line: ${endLineStr}`)
-										pushToolResult(formatResponse.toolError("Invalid end_line value"))
-										break
-									}
-
-									// Convert to 0-based index
-									endLine -= 1
-								}
-
-								const accessAllowed = this.rooIgnoreController?.validateAccess(relPath)
-								if (!accessAllowed) {
-									await this.say("rooignore_error", relPath)
-									pushToolResult(formatResponse.toolError(formatResponse.rooIgnoreError(relPath)))
-
-									break
-								}
-
-								this.consecutiveMistakeCount = 0
-								const absolutePath = path.resolve(this.cwd, relPath)
-								const completeMessage = JSON.stringify({
-									...sharedMessageProps,
-									content: absolutePath,
-								} satisfies ClineSayTool)
-
-								const didApprove = await askApproval("tool", completeMessage)
-								if (!didApprove) {
-									break
-								}
-
-								// Get the maxReadFileLine setting
-								const { maxReadFileLine = 500 } = (await this.providerRef.deref()?.getState()) ?? {}
-
-								// Count total lines in the file
-								let totalLines = 0
-								try {
-									totalLines = await countFileLines(absolutePath)
-								} catch (error) {
-									console.error(`Error counting lines in file ${absolutePath}:`, error)
-								}
-
-								// now execute the tool like normal
-								let content: string
-								let isFileTruncated = false
-								let sourceCodeDef = ""
-
-								const isBinary = await isBinaryFile(absolutePath).catch(() => false)
-
-								if (isRangeRead) {
-									if (startLine === undefined) {
-										content = addLineNumbers(await readLines(absolutePath, endLine, startLine))
-									} else {
-										content = addLineNumbers(
-											await readLines(absolutePath, endLine, startLine),
-											startLine + 1,
-										)
-									}
-								} else if (!isBinary && maxReadFileLine >= 0 && totalLines > maxReadFileLine) {
-									// If file is too large, only read the first maxReadFileLine lines
-									isFileTruncated = true
-
-									const res = await Promise.all([
-										maxReadFileLine > 0 ? readLines(absolutePath, maxReadFileLine - 1, 0) : "",
-										parseSourceCodeDefinitionsForFile(absolutePath, this.rooIgnoreController),
-									])
-
-									content = res[0].length > 0 ? addLineNumbers(res[0]) : ""
-									const result = res[1]
-									if (result) {
-										sourceCodeDef = `\n\n${result}`
-									}
-								} else {
-									// Read entire file
-									content = await extractTextFromFile(absolutePath)
-								}
-
-								// 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}`
-								}
-
-								pushToolResult(content)
-								break
-							}
-						} catch (error) {
-							await handleError("reading file", error)
-							break
-						}
+						readFileTool(this, block, askApproval, handleError, pushToolResult, removeClosingTag)
+						break
 					}
 
 					case "fetch_instructions": {

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

@@ -1,5 +1,3 @@
-const DEBUG = false
-
 import * as path from "path"
 import { countFileLines } from "../../integrations/misc/line-counter"
 import { readLines } from "../../integrations/misc/read-lines"
@@ -8,7 +6,6 @@ import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter"
 import { isBinaryFile } from "isbinaryfile"
 import { ReadFileToolUse } from "../assistant-message"
 import { Cline } from "../Cline"
-import { ClineProvider } from "../webview/ClineProvider"
 
 // Mock dependencies
 jest.mock("../../integrations/misc/line-counter")
@@ -21,7 +18,7 @@ jest.mock("../ignore/RooIgnoreController", () => ({
 		initialize() {
 			return Promise.resolve()
 		}
-		validateAccess(filePath: string) {
+		validateAccess() {
 			return true
 		}
 	},
@@ -45,281 +42,240 @@ jest.mock("path", () => {
 })
 
 describe("read_file tool with maxReadFileLine setting", () => {
-	// Mock original implementation first to use in tests
-	const originalCountFileLines = jest.requireActual("../../integrations/misc/line-counter").countFileLines
-	const originalReadLines = jest.requireActual("../../integrations/misc/read-lines").readLines
-	const originalExtractTextFromFile = jest.requireActual("../../integrations/misc/extract-text").extractTextFromFile
-	const originalAddLineNumbers = jest.requireActual("../../integrations/misc/extract-text").addLineNumbers
-	const originalParseSourceCodeDefinitionsForFile =
-		jest.requireActual("../../services/tree-sitter").parseSourceCodeDefinitionsForFile
-	const originalIsBinaryFile = jest.requireActual("isbinaryfile").isBinaryFile
-
-	let cline: Cline
-	let mockProvider: any
+	// Test data
 	const testFilePath = "test/file.txt"
-	const absoluteFilePath = "/home/ewheeler/src/roo/roo-main/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 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 mockedAddLineNumbers = addLineNumbers as jest.MockedFunction<typeof addLineNumbers>
+	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.resetAllMocks()
-
-		// Reset mocks to simulate original behavior
-		;(countFileLines as jest.Mock).mockImplementation(originalCountFileLines)
-		;(readLines as jest.Mock).mockImplementation(originalReadLines)
-		;(extractTextFromFile as jest.Mock).mockImplementation(originalExtractTextFromFile)
-		;(parseSourceCodeDefinitionsForFile as jest.Mock).mockImplementation(originalParseSourceCodeDefinitionsForFile)
-		;(isBinaryFile as jest.Mock).mockImplementation(originalIsBinaryFile)
-
-		// Default mock implementations
-		;(countFileLines as jest.Mock).mockResolvedValue(5)
-		;(readLines as jest.Mock).mockResolvedValue(fileContent)
-		;(extractTextFromFile as jest.Mock).mockResolvedValue(numberedFileContent)
-		// Use the real addLineNumbers function
-		;(addLineNumbers as jest.Mock).mockImplementation(originalAddLineNumbers)
-		;(parseSourceCodeDefinitionsForFile as jest.Mock).mockResolvedValue(sourceCodeDef)
-		;(isBinaryFile as jest.Mock).mockResolvedValue(false)
-
-		// Add spy to debug the readLines calls
-		const readLinesSpy = jest.spyOn(require("../../integrations/misc/read-lines"), "readLines")
-
-		// Mock path.resolve to return a predictable path
-		;(path.resolve as jest.Mock).mockReturnValue(absoluteFilePath)
-
-		// Create mock provider
+		jest.clearAllMocks()
+
+		// Setup path resolution
+		mockedPathResolve.mockReturnValue(absoluteFilePath)
+
+		// 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")
+		})
+
+		// Setup mock provider
 		mockProvider = {
 			getState: jest.fn(),
 			deref: jest.fn().mockReturnThis(),
 		}
 
-		// Create a Cline instance with the necessary configuration
-		cline = new Cline({
-			provider: mockProvider,
-			apiConfiguration: { apiProvider: "anthropic" } as any,
-			task: "Test read_file tool", // Required to satisfy constructor check
-			startTask: false, // Prevent actual task initialization
-		})
+		// 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()
 
-		// Set up the read_file tool use
-		const readFileToolUse: ReadFileToolUse = {
+		// Reset tool result
+		toolResult = undefined
+	})
+
+	/**
+	 * Helper function to execute the read file tool with different maxReadFileLine settings
+	 */
+	async function executeReadFileTool(maxReadFileLine: number, totalLines = 5): Promise<string | undefined> {
+		// Configure mocks based on test scenario
+		mockProvider.getState.mockResolvedValue({ maxReadFileLine })
+		mockedCountFileLines.mockResolvedValue(totalLines)
+
+		// Create a tool use object
+		const toolUse: ReadFileToolUse = {
 			type: "tool_use",
 			name: "read_file",
-			params: {
-				path: testFilePath,
-			},
+			params: { path: testFilePath },
 			partial: false,
 		}
 
-		// Set up the Cline instance for testing
-		const clineAny = cline as any
-
-		// Set up the required properties for the test
-		clineAny.assistantMessageContent = [readFileToolUse]
-		clineAny.currentStreamingContentIndex = 0
-		clineAny.userMessageContent = []
-		clineAny.presentAssistantMessageLocked = false
-		clineAny.didCompleteReadingStream = true
-		clineAny.didRejectTool = false
-		clineAny.didAlreadyUseTool = false
-
-		// Mock methods that would be called during presentAssistantMessage
-		clineAny.say = jest.fn().mockResolvedValue(undefined)
-		clineAny.ask = jest.fn().mockImplementation((type, message) => {
-			return Promise.resolve({ response: "yesButtonClicked" })
-		})
-	})
+		// Import the tool implementation dynamically to avoid hoisting issues
+		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,
+		)
 
-	// Helper function to get user message content
-	const getUserMessageContent = (clineInstance: Cline) => {
-		const clineAny = clineInstance as any
-		return clineAny.userMessageContent
+		return toolResult
 	}
 
-	// Helper function to validate response lines
-	const validateResponseLines = (
-		responseLines: string[],
-		options: {
-			expectedLineCount: number
-			shouldContainLines?: number[]
-			shouldNotContainLines?: number[]
-		},
-	) => {
-		if (options.shouldContainLines) {
-			const contentLines = responseLines.filter((line) => line.includes("Line "))
-			expect(contentLines.length).toBe(options.expectedLineCount)
-			options.shouldContainLines.forEach((lineNum) => {
-				expect(contentLines[lineNum - 1]).toContain(`Line ${lineNum}`)
-			})
-		}
+	describe("when maxReadFileLine is negative", () => {
+		it("should read the entire file using extractTextFromFile", async () => {
+			// Setup
+			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
 
-		if (options.shouldNotContainLines) {
-			options.shouldNotContainLines.forEach((lineNum) => {
-				expect(responseLines.some((line) => line.includes(`Line ${lineNum}`))).toBe(false)
-			})
-		}
-	}
+			// Execute
+			const result = await executeReadFileTool(-1)
 
-	interface TestExpectations {
-		extractTextCalled: boolean
-		readLinesCalled: boolean
-		sourceCodeDefCalled: boolean
-		readLinesParams?: [string, number, number]
-		responseValidation: {
-			expectedLineCount: number
-			shouldContainLines?: number[]
-			shouldNotContainLines?: number[]
-		}
-		expectedContent?: string
-		truncationMessage?: string
-		includeSourceCodeDef?: boolean
-	}
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			expect(mockedParseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
+			expect(result).toBe(numberedFileContent)
+		})
+	})
 
-	interface TestCase {
-		name: string
-		maxReadFileLine: number
-		setup?: () => void
-		expectations: TestExpectations
-	}
+	describe("when maxReadFileLine is 0", () => {
+		it("should return an empty content with source code definitions", async () => {
+			// Setup - for maxReadFileLine = 0, the implementation won't call readLines
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute
+			const result = await executeReadFileTool(0)
+
+			// Verify
+			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
+			expect(mockedReadLines).not.toHaveBeenCalled() // Per implementation line 141
+			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
+				absoluteFilePath,
+				mockCline.rooIgnoreController,
+			)
+			expect(result).toContain("[Showing only 0 of 5 total lines")
+			expect(result).toContain(sourceCodeDef)
+		})
+	})
 
-	// Test cases
-	const testCases: TestCase[] = [
-		{
-			name: "read entire file when maxReadFileLine is -1",
-			maxReadFileLine: -1,
-			expectations: {
-				extractTextCalled: true,
-				readLinesCalled: false,
-				sourceCodeDefCalled: false,
-				responseValidation: {
-					expectedLineCount: 5,
-					shouldContainLines: [1, 2, 3, 4, 5],
-				},
-				expectedContent: numberedFileContent,
-			},
-		},
-		{
-			name: "read entire file when maxReadFileLine >= file length",
-			maxReadFileLine: 10,
-			expectations: {
-				extractTextCalled: true,
-				readLinesCalled: false,
-				sourceCodeDefCalled: false,
-				responseValidation: {
-					expectedLineCount: 5,
-					shouldContainLines: [1, 2, 3, 4, 5],
-				},
-				expectedContent: numberedFileContent,
-			},
-		},
-		{
-			name: "read zero lines and only provide line declaration definitions when maxReadFileLine is 0",
-			maxReadFileLine: 0,
-			expectations: {
-				extractTextCalled: false,
-				readLinesCalled: false,
-				sourceCodeDefCalled: true,
-				responseValidation: {
-					expectedLineCount: 0,
-				},
-				truncationMessage: `[Showing only 0 of 5 total lines. Use start_line and end_line if you need to read more]`,
-				includeSourceCodeDef: true,
-			},
-		},
-		{
-			name: "read maxReadFileLine lines and provide line declaration definitions when maxReadFileLine < file length",
-			maxReadFileLine: 3,
-			setup: () => {
-				jest.clearAllMocks()
-				;(countFileLines as jest.Mock).mockResolvedValue(5)
-				;(readLines as jest.Mock).mockImplementation((path, endLine, startLine = 0) => {
-					const lines = fileContent.split("\n")
-					const actualEndLine = endLine !== undefined ? Math.min(endLine, lines.length - 1) : lines.length - 1
-					const actualStartLine = startLine !== undefined ? Math.min(startLine, lines.length - 1) : 0
-					const requestedLines = lines.slice(actualStartLine, actualEndLine + 1)
-					return Promise.resolve(requestedLines.join("\n"))
-				})
-			},
-			expectations: {
-				extractTextCalled: false,
-				readLinesCalled: true,
-				sourceCodeDefCalled: true,
-				readLinesParams: [absoluteFilePath, 2, 0],
-				responseValidation: {
-					expectedLineCount: 3,
-					shouldContainLines: [1, 2, 3],
-					shouldNotContainLines: [4, 5],
-				},
-				truncationMessage: `[Showing only 3 of 5 total lines. Use start_line and end_line if you need to read more]`,
-				includeSourceCodeDef: true,
-			},
-		},
-	]
+	describe("when maxReadFileLine is less than file length", () => {
+		it("should read only maxReadFileLine lines and add source code definitions", async () => {
+			// Setup
+			const content = "Line 1\nLine 2\nLine 3"
+			mockedReadLines.mockResolvedValue(content)
+			mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef)
+
+			// Execute
+			const result = await executeReadFileTool(3)
+
+			// Verify - check behavior but not specific implementation details
+			expect(mockedExtractTextFromFile).not.toHaveBeenCalled()
+			expect(mockedReadLines).toHaveBeenCalled()
+			expect(mockedParseSourceCodeDefinitionsForFile).toHaveBeenCalledWith(
+				absoluteFilePath,
+				mockCline.rooIgnoreController,
+			)
+			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)
+		})
+	})
 
-	test.each(testCases)("should $name", async (testCase) => {
-		// Setup
-		if (testCase.setup) {
-			testCase.setup()
-		}
-		mockProvider.getState.mockResolvedValue({ maxReadFileLine: testCase.maxReadFileLine })
+	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)
 
-		// Execute
-		await cline.presentAssistantMessage()
+			// Execute
+			const result = await executeReadFileTool(10, 5)
 
-		// Verify mock calls
-		if (testCase.expectations.extractTextCalled) {
-			expect(extractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
-		} else {
-			expect(extractTextFromFile).not.toHaveBeenCalled()
-		}
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(result).toBe(numberedFileContent)
+		})
 
-		if (testCase.expectations.readLinesCalled) {
-			const params = testCase.expectations.readLinesParams
-			if (!params) {
-				throw new Error("readLinesParams must be defined when readLinesCalled is true")
-			}
-			expect(readLines).toHaveBeenCalledWith(...params)
-		} else {
-			expect(readLines).not.toHaveBeenCalled()
-		}
+		it("should read with extractTextFromFile when file has few lines", async () => {
+			// Setup
+			mockedCountFileLines.mockResolvedValue(3) // File shorter than maxReadFileLine
+			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
 
-		if (testCase.expectations.sourceCodeDefCalled) {
-			expect(parseSourceCodeDefinitionsForFile).toHaveBeenCalled()
-		} else {
-			expect(parseSourceCodeDefinitionsForFile).not.toHaveBeenCalled()
-		}
+			// Execute
+			const result = await executeReadFileTool(5, 3)
+
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			expect(result).toBe(numberedFileContent)
+		})
+	})
 
-		// Verify response content
-		const userMessageContent = getUserMessageContent(cline)
+	describe("when file is binary", () => {
+		it("should always use extractTextFromFile regardless of maxReadFileLine", async () => {
+			// Setup
+			mockedIsBinaryFile.mockResolvedValue(true)
+			mockedExtractTextFromFile.mockResolvedValue(numberedFileContent)
 
-		if (DEBUG) {
-			console.log(`\n=== Test: ${testCase.name} ===`)
-			console.log(`maxReadFileLine: ${testCase.maxReadFileLine}`)
-			console.log("Response content:", JSON.stringify(userMessageContent, null, 2))
-		}
-		const responseLines = userMessageContent[1].text.split("\n")
+			// Execute
+			const result = await executeReadFileTool(3)
 
-		if (DEBUG) {
-			console.log(`Number of lines in response: ${responseLines.length}`)
-		}
+			// Verify
+			expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath)
+			expect(mockedReadLines).not.toHaveBeenCalled()
+			expect(result).toBe(numberedFileContent)
+		})
+	})
 
-		expect(userMessageContent.length).toBe(2)
-		expect(userMessageContent[0].text).toBe(`[read_file for '${testFilePath}'] Result:`)
+	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,
+			}
 
-		if (testCase.expectations.expectedContent) {
-			expect(userMessageContent[1].text).toBe(testCase.expectations.expectedContent)
-		}
+			mockedReadLines.mockResolvedValue("Line 2\nLine 3\nLine 4")
 
-		if (testCase.expectations.responseValidation) {
-			validateResponseLines(responseLines, testCase.expectations.responseValidation)
-		}
+			// Import the tool implementation dynamically
+			const { readFileTool } = require("../tools/readFileTool")
 
-		if (testCase.expectations.truncationMessage) {
-			expect(userMessageContent[1].text).toContain(testCase.expectations.truncationMessage)
-		}
+			// Execute the tool
+			let rangeResult: string | undefined
+			await readFileTool(
+				mockCline,
+				rangeToolUse,
+				mockCline.ask,
+				jest.fn(),
+				(result: string) => {
+					rangeResult = result
+				},
+				(param: string, value: string) => value,
+			)
 
-		if (testCase.expectations.includeSourceCodeDef) {
-			expect(userMessageContent[1].text).toContain(sourceCodeDef)
-		}
+			// 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
+		})
 	})
 })

src/core/tools/readFileTool.ts

@@ -0,0 +1,168 @@
+import path from "path"
+import { Cline } from "../Cline"
+import { ClineSayTool } from "../../shared/ExtensionMessage"
+import { ToolUse } from "../assistant-message"
+import { formatResponse } from "../prompts/responses"
+import { AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "./types"
+import { isPathOutsideWorkspace } from "../../utils/pathUtils"
+import { getReadablePath } from "../../utils/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"
+
+export async function readFileTool(
+	cline: Cline,
+	block: ToolUse,
+	askApproval: AskApproval,
+	handleError: HandleError,
+	pushToolResult: PushToolResult,
+	removeClosingTag: RemoveClosingTag,
+) {
+	switch (true) {
+		default:
+			const relPath: string | undefined = block.params.path
+			const startLineStr: string | undefined = block.params.start_line
+			const endLineStr: string | undefined = block.params.end_line
+
+			// Get the full path and determine if it's outside the workspace
+			const fullPath = relPath ? path.resolve(cline.cwd, removeClosingTag("path", relPath)) : ""
+			const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
+
+			const sharedMessageProps: ClineSayTool = {
+				tool: "readFile",
+				path: getReadablePath(cline.cwd, removeClosingTag("path", relPath)),
+				isOutsideWorkspace,
+			}
+			try {
+				if (block.partial) {
+					const partialMessage = JSON.stringify({
+						...sharedMessageProps,
+						content: undefined,
+					} satisfies ClineSayTool)
+					await cline.ask("tool", partialMessage, block.partial).catch(() => {})
+					break
+				} else {
+					if (!relPath) {
+						cline.consecutiveMistakeCount++
+						pushToolResult(await cline.sayAndCreateMissingParamError("read_file", "path"))
+						break
+					}
+
+					// Check if we're doing a line range read
+					let isRangeRead = false
+					let startLine: number | undefined = undefined
+					let endLine: number | undefined = undefined
+
+					// Check if we have either range parameter
+					if (startLineStr || endLineStr) {
+						isRangeRead = true
+					}
+
+					// Parse start_line if provided
+					if (startLineStr) {
+						startLine = parseInt(startLineStr)
+						if (isNaN(startLine)) {
+							// Invalid start_line
+							cline.consecutiveMistakeCount++
+							await cline.say("error", `Failed to parse start_line: ${startLineStr}`)
+							pushToolResult(formatResponse.toolError("Invalid start_line value"))
+							break
+						}
+						startLine -= 1 // Convert to 0-based index
+					}
+
+					// Parse end_line if provided
+					if (endLineStr) {
+						endLine = parseInt(endLineStr)
+
+						if (isNaN(endLine)) {
+							// Invalid end_line
+							cline.consecutiveMistakeCount++
+							await cline.say("error", `Failed to parse end_line: ${endLineStr}`)
+							pushToolResult(formatResponse.toolError("Invalid end_line value"))
+							break
+						}
+
+						// Convert to 0-based index
+						endLine -= 1
+					}
+
+					const accessAllowed = cline.rooIgnoreController?.validateAccess(relPath)
+					if (!accessAllowed) {
+						await cline.say("rooignore_error", relPath)
+						pushToolResult(formatResponse.toolError(formatResponse.rooIgnoreError(relPath)))
+
+						break
+					}
+
+					cline.consecutiveMistakeCount = 0
+					const absolutePath = path.resolve(cline.cwd, relPath)
+					const completeMessage = JSON.stringify({
+						...sharedMessageProps,
+						content: absolutePath,
+					} satisfies ClineSayTool)
+
+					const didApprove = await askApproval("tool", completeMessage)
+					if (!didApprove) {
+						break
+					}
+
+					// Get the maxReadFileLine setting
+					const { maxReadFileLine = 500 } = (await cline.providerRef.deref()?.getState()) ?? {}
+
+					// Count total lines in the file
+					let totalLines = 0
+					try {
+						totalLines = await countFileLines(absolutePath)
+					} catch (error) {
+						console.error(`Error counting lines in file ${absolutePath}:`, error)
+					}
+
+					// now execute the tool like normal
+					let content: string
+					let isFileTruncated = false
+					let sourceCodeDef = ""
+
+					const isBinary = await isBinaryFile(absolutePath).catch(() => false)
+
+					if (isRangeRead) {
+						if (startLine === undefined) {
+							content = addLineNumbers(await readLines(absolutePath, endLine, startLine))
+						} else {
+							content = addLineNumbers(await readLines(absolutePath, endLine, startLine), startLine + 1)
+						}
+					} else if (!isBinary && maxReadFileLine >= 0 && totalLines > maxReadFileLine) {
+						// If file is too large, only read the first maxReadFileLine lines
+						isFileTruncated = true
+
+						const res = await Promise.all([
+							maxReadFileLine > 0 ? readLines(absolutePath, maxReadFileLine - 1, 0) : "",
+							parseSourceCodeDefinitionsForFile(absolutePath, cline.rooIgnoreController),
+						])
+
+						content = res[0].length > 0 ? addLineNumbers(res[0]) : ""
+						const result = res[1]
+						if (result) {
+							sourceCodeDef = `\n\n${result}`
+						}
+					} else {
+						// Read entire file
+						content = await extractTextFromFile(absolutePath)
+					}
+
+					// 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}`
+					}
+
+					pushToolResult(content)
+					break
+				}
+			} catch (error) {
+				await handleError("reading file", error)
+				break
+			}
+	}
+}

src/core/tools/types.ts

@@ -1,4 +1,5 @@
 import { ClineAsk, ToolProgressStatus } from "../../schemas"
+import { ToolParamName } from "../assistant-message"
 import { ToolResponse } from "../Cline"
 
 export type AskApproval = (
@@ -10,3 +11,5 @@ export type AskApproval = (
 export type HandleError = (action: string, error: Error) => void
 
 export type PushToolResult = (content: ToolResponse) => void
+
+export type RemoveClosingTag = (tag: ToolParamName, content?: string) => string