diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 0f81653a4ed..483d4f00252 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/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: `${relPath}Error reading file: ${errorMsg}`, + 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) { diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 882c7b3ebb3..f178e38026c 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/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: `${dirPath}` }) + + // 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 })