Improve error message when read_file is used on directory

src/core/tools/ReadFileTool.ts

@@ -1,4 +1,5 @@
 import path from "path"
+import * as fs from "fs/promises"
 import { isBinaryFile } from "isbinaryfile"
 import type { FileEntry, LineRange } from "@roo-code/types"
 import { isNativeProtocol, ANTHROPIC_DEFAULT_MAX_TOKENS } from "@roo-code/types"
@@ -350,6 +351,20 @@ export class ReadFileTool extends BaseTool<"read_file"> {
 				const fullPath = path.resolve(task.cwd, relPath)
 
 				try {
+					// Check if the path is a directory before attempting to read it
+					const stats = await fs.stat(fullPath)
+					if (stats.isDirectory()) {
+						const errorMsg = `Cannot read '${relPath}' because it is a directory. To view the contents of a directory, use the list_files tool instead.`
+						updateFileResult(relPath, {
+							status: "error",
+							error: errorMsg,
+							xmlContent: `<file><path>${relPath}</path><error>Error reading file: ${errorMsg}</error></file>`,
+							nativeContent: `File: ${relPath}\nError: Error reading file: ${errorMsg}`,
+						})
+						await task.say("error", `Error reading file ${relPath}: ${errorMsg}`)
+						continue
+					}
+
 					const [totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)])
 
 					if (isBinary) {

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

@@ -305,6 +305,13 @@ describe("read_file tool with maxReadFileLine setting", () => {
 		mockedPathResolve.mockReturnValue(absoluteFilePath)
 		mockedIsBinaryFile.mockResolvedValue(false)
 
+		// Mock fsPromises.stat to return a file (not directory) by default
+		fsPromises.stat.mockResolvedValue({
+			isDirectory: () => false,
+			isFile: () => true,
+			isSymbolicLink: () => false,
+		} as any)
+
 		mockInputContent = fileContent
 
 		// Setup the extractTextFromFile mock implementation with the current mockInputContent
@@ -612,7 +619,12 @@ describe("read_file tool output structure", () => {
 
 		// CRITICAL: Reset fsPromises mocks to prevent cross-test contamination
 		fsPromises.stat.mockClear()
-		fsPromises.stat.mockResolvedValue({ size: 1024 })
+		fsPromises.stat.mockResolvedValue({
+			size: 1024,
+			isDirectory: () => false,
+			isFile: () => true,
+			isSymbolicLink: () => false,
+		} as any)
 		fsPromises.readFile.mockClear()
 
 		// Use shared mock setup function
@@ -852,7 +864,7 @@ describe("read_file tool output structure", () => {
 				fsPromises.stat = vi.fn().mockImplementation((filePath) => {
 					const normalizedFilePath = path.normalize(filePath.toString())
 					const image = smallImages.find((img) => normalizedFilePath.includes(path.normalize(img.path)))
-					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024 })
+					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024, isDirectory: () => false })
 				})
 
 				// Mock path.resolve for each image
@@ -928,7 +940,7 @@ describe("read_file tool output structure", () => {
 				fsPromises.stat = vi.fn().mockImplementation((filePath) => {
 					const normalizedFilePath = path.normalize(filePath.toString())
 					const image = largeImages.find((img) => normalizedFilePath.includes(path.normalize(img.path)))
-					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024 })
+					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024, isDirectory: () => false })
 				})
 
 				// Mock path.resolve for each image
@@ -1012,9 +1024,9 @@ describe("read_file tool output structure", () => {
 					const normalizedFilePath = path.normalize(filePath.toString())
 					const image = exactLimitImages.find((img) => normalizedFilePath.includes(path.normalize(img.path)))
 					if (image) {
-						return Promise.resolve({ size: image.sizeKB * 1024 })
+						return Promise.resolve({ size: image.sizeKB * 1024, isDirectory: () => false })
 					}
-					return Promise.resolve({ size: 1024 * 1024 }) // Default 1MB
+					return Promise.resolve({ size: 1024 * 1024, isDirectory: () => false }) // Default 1MB
 				})
 
 				// Mock path.resolve
@@ -1085,7 +1097,7 @@ describe("read_file tool output structure", () => {
 					const fileName = path.basename(filePath)
 					const baseName = path.parse(fileName).name
 					const image = mixedImages.find((img) => img.path.includes(baseName))
-					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024 })
+					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024, isDirectory: () => false })
 				})
 
 				// Mock provider state with 5MB individual limit
@@ -1139,9 +1151,9 @@ describe("read_file tool output structure", () => {
 					const normalizedFilePath = path.normalize(filePath.toString())
 					const file = testImages.find((f) => normalizedFilePath.includes(path.normalize(f.path)))
 					if (file) {
-						return { size: file.sizeMB * 1024 * 1024 }
+						return { size: file.sizeMB * 1024 * 1024, isDirectory: () => false }
 					}
-					return { size: 1024 * 1024 } // Default 1MB
+					return { size: 1024 * 1024, isDirectory: () => false } // Default 1MB
 				})
 
 				const imagePaths = testImages.map((img) => img.path)
@@ -1201,7 +1213,7 @@ describe("read_file tool output structure", () => {
 				// Setup - first call with images that use memory
 				const firstBatch = [{ path: "test/first.png", sizeKB: 10240 }] // 10MB
 
-				fsPromises.stat = vi.fn().mockResolvedValue({ size: 10240 * 1024 })
+				fsPromises.stat = vi.fn().mockResolvedValue({ size: 10240 * 1024, isDirectory: () => false })
 				mockedPathResolve.mockImplementation((cwd, relPath) => `/${relPath}`)
 
 				// Execute first batch
@@ -1254,7 +1266,7 @@ describe("read_file tool output structure", () => {
 				mockedCountFileLines.mockClear()
 
 				// Reset mocks for second batch
-				fsPromises.stat = vi.fn().mockResolvedValue({ size: 15360 * 1024 })
+				fsPromises.stat = vi.fn().mockResolvedValue({ size: 15360 * 1024, isDirectory: () => false })
 				fsPromises.readFile.mockResolvedValue(
 					Buffer.from(
 						"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==",
@@ -1303,7 +1315,7 @@ describe("read_file tool output structure", () => {
 				fsPromises.stat = vi.fn().mockImplementation((filePath) => {
 					const normalizedFilePath = path.normalize(filePath.toString())
 					const image = manyImages.find((img) => normalizedFilePath.includes(path.normalize(img.path)))
-					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024 })
+					return Promise.resolve({ size: (image?.sizeKB || 1024) * 1024, isDirectory: () => false })
 				})
 
 				// Mock path.resolve
@@ -1350,7 +1362,7 @@ describe("read_file tool output structure", () => {
 				// First invocation - use 15MB of memory
 				const firstBatch = [{ path: "test/large1.png", sizeKB: 15360 }] // 15MB
 
-				fsPromises.stat = vi.fn().mockResolvedValue({ size: 15360 * 1024 })
+				fsPromises.stat = vi.fn().mockResolvedValue({ size: 15360 * 1024, isDirectory: () => false })
 				mockedPathResolve.mockImplementation((cwd, relPath) => `/${relPath}`)
 
 				// Execute first batch
@@ -1371,7 +1383,7 @@ describe("read_file tool output structure", () => {
 				fsPromises.readFile.mockClear()
 				mockedPathResolve.mockClear()
 
-				fsPromises.stat = vi.fn().mockResolvedValue({ size: 18432 * 1024 })
+				fsPromises.stat = vi.fn().mockResolvedValue({ size: 18432 * 1024, isDirectory: () => false })
 				fsPromises.readFile.mockResolvedValue(imageBuffer)
 				mockedPathResolve.mockImplementation((cwd, relPath) => `/${relPath}`)
 
@@ -1429,6 +1441,37 @@ describe("read_file tool output structure", () => {
 				`File: ${testFilePath}\nError: Access to ${testFilePath} is blocked by the .rooignore file settings. You must try to continue in the task without using this file, or ask the user to update the .rooignore file.`,
 			)
 		})
+
+		it("should provide helpful error when trying to read a directory", async () => {
+			// Setup - mock fsPromises.stat to indicate the path is a directory
+			const dirPath = "test/my-directory"
+			const absoluteDirPath = "/test/my-directory"
+
+			mockedPathResolve.mockReturnValue(absoluteDirPath)
+
+			// Mock fs/promises stat to return directory
+			fsPromises.stat.mockResolvedValue({
+				isDirectory: () => true,
+				isFile: () => false,
+				isSymbolicLink: () => false,
+			} as any)
+
+			// Mock isBinaryFile won't be called since we check directory first
+			mockedIsBinaryFile.mockResolvedValue(false)
+
+			// Execute
+			const result = await executeReadFileTool({ args: `<file><path>${dirPath}</path></file>` })
+
+			// Verify - native format for error
+			expect(result).toContain(`File: ${dirPath}`)
+			expect(result).toContain(`Error: Error reading file: Cannot read '${dirPath}' because it is a directory`)
+			expect(result).toContain("use the list_files tool instead")
+
+			// Verify that task.say was called with the error
+			expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("Cannot read"))
+			expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("is a directory"))
+			expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("list_files tool"))
+		})
 	})
 })
 
@@ -1460,7 +1503,12 @@ describe("read_file tool with image support", () => {
 
 		// CRITICAL: Reset fsPromises.stat to prevent cross-test contamination
 		fsPromises.stat.mockClear()
-		fsPromises.stat.mockResolvedValue({ size: 1024 })
+		fsPromises.stat.mockResolvedValue({
+			size: 1024,
+			isDirectory: () => false,
+			isFile: () => true,
+			isSymbolicLink: () => false,
+		} as any)
 
 		// Use shared mock setup function with local variables
 		const mocks = createMockCline()
@@ -1801,6 +1849,13 @@ describe("read_file tool concurrent file reads limit", () => {
 		mockedIsBinaryFile.mockResolvedValue(false)
 		mockedCountFileLines.mockResolvedValue(10)
 
+		// Mock fsPromises.stat to return a file (not directory) by default
+		fsPromises.stat.mockResolvedValue({
+			isDirectory: () => false,
+			isFile: () => true,
+			isSymbolicLink: () => false,
+		} as any)
+
 		toolResult = undefined
 	})