scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-08-28 05:27:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Improve error message when read_file is used on directory (#10371)

Browse source 
Co-authored-by: Roo Code <roomote@roocode.com>
This commit is contained in:
roomote[bot] 2025-12-28 21:57:41 -08:00 • committed by GitHub
parent add06a2e7b
commit a18c9b7a56
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 84 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
src/core/tools/ReadFileTool.ts
View file

@ -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) {

83
src/core/tools/__tests__/readFileTool.spec.ts
View file

@ -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
})