mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
feat: improve empty file context detection in read_file tool
- Add clear "This file is empty" message in extractTextFromFile for empty files - Handle empty files consistently across all read_file code paths (range reads, definitions-only, maxReadFileLine scenarios) - Update tests to verify empty file handling works in all scenarios - Add comprehensive test coverage for empty file edge cases Fixes #5789
This commit is contained in:
parent
0f994fcf22
commit
4874a07ecb
4 changed files with 117 additions and 4 deletions
|
|
@ -470,7 +470,7 @@ describe("read_file tool XML output structure", () => {
|
|||
it("should handle empty files correctly", async () => {
|
||||
// Setup
|
||||
mockedCountFileLines.mockResolvedValue(0)
|
||||
mockedExtractTextFromFile.mockResolvedValue("")
|
||||
mockedExtractTextFromFile.mockResolvedValue("This file is empty")
|
||||
mockProvider.getState.mockResolvedValue({ maxReadFileLine: -1 })
|
||||
|
||||
// Execute
|
||||
|
|
@ -518,5 +518,70 @@ describe("read_file tool XML output structure", () => {
|
|||
`<files>\n<file><path>${testFilePath}</path><error>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.</error></file>\n</files>`,
|
||||
)
|
||||
})
|
||||
|
||||
it("should handle empty files with range reads", async () => {
|
||||
// Setup
|
||||
mockedCountFileLines.mockResolvedValue(0)
|
||||
mockedExtractTextFromFile.mockResolvedValue("This file is empty")
|
||||
mockProvider.getState.mockResolvedValue({ maxReadFileLine: -1 })
|
||||
|
||||
// Create args with range manually since the helper doesn't support it
|
||||
const argsContent = `<file><path>${testFilePath}</path><line_range>1-5</line_range></file>`
|
||||
|
||||
// Create a tool use object directly
|
||||
const toolUse: ReadFileToolUse = {
|
||||
type: "tool_use",
|
||||
name: "read_file",
|
||||
params: { args: argsContent },
|
||||
partial: false,
|
||||
}
|
||||
|
||||
// Execute the tool directly
|
||||
await readFileTool(
|
||||
mockCline,
|
||||
toolUse,
|
||||
mockCline.ask,
|
||||
vi.fn(),
|
||||
(result: ToolResponse) => {
|
||||
toolResult = result
|
||||
},
|
||||
(param: ToolParamName, content?: string) => content ?? "",
|
||||
)
|
||||
|
||||
// Verify - should show empty file notice instead of trying to read range
|
||||
expect(toolResult).toBe(
|
||||
`<files>\n<file><path>${testFilePath}</path>\n<content/><notice>File is empty</notice>\n</file>\n</files>`,
|
||||
)
|
||||
})
|
||||
|
||||
it("should handle empty files in definitions-only mode", async () => {
|
||||
// Setup
|
||||
mockedCountFileLines.mockResolvedValue(0)
|
||||
mockedExtractTextFromFile.mockResolvedValue("This file is empty")
|
||||
mockProvider.getState.mockResolvedValue({ maxReadFileLine: 0 })
|
||||
|
||||
// Execute
|
||||
const result = await executeReadFileTool({}, { totalLines: 0, maxReadFileLine: 0 })
|
||||
|
||||
// Verify - should show empty file notice instead of trying to parse definitions
|
||||
expect(result).toBe(
|
||||
`<files>\n<file><path>${testFilePath}</path>\n<content/><notice>File is empty</notice>\n</file>\n</files>`,
|
||||
)
|
||||
})
|
||||
|
||||
it("should handle empty files when maxReadFileLine exceeds file length", async () => {
|
||||
// Setup
|
||||
mockedCountFileLines.mockResolvedValue(0)
|
||||
mockedExtractTextFromFile.mockResolvedValue("This file is empty")
|
||||
mockProvider.getState.mockResolvedValue({ maxReadFileLine: 10 })
|
||||
|
||||
// Execute
|
||||
const result = await executeReadFileTool({}, { totalLines: 0, maxReadFileLine: 10 })
|
||||
|
||||
// Verify - should show empty file notice
|
||||
expect(result).toBe(
|
||||
`<files>\n<file><path>${testFilePath}</path>\n<content/><notice>File is empty</notice>\n</file>\n</files>`,
|
||||
)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -452,6 +452,14 @@ export async function readFileTool(
|
|||
|
||||
// Handle range reads (bypass maxReadFileLine)
|
||||
if (fileResult.lineRanges && fileResult.lineRanges.length > 0) {
|
||||
// Check if file is empty first
|
||||
if (totalLines === 0) {
|
||||
updateFileResult(relPath, {
|
||||
xmlContent: `<file><path>${relPath}</path>\n<content/><notice>File is empty</notice>\n</file>`,
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
const rangeResults: string[] = []
|
||||
for (const range of fileResult.lineRanges) {
|
||||
const content = addLineNumbers(
|
||||
|
|
@ -469,6 +477,14 @@ export async function readFileTool(
|
|||
|
||||
// Handle definitions-only mode
|
||||
if (maxReadFileLine === 0) {
|
||||
// Check if file is empty first
|
||||
if (totalLines === 0) {
|
||||
updateFileResult(relPath, {
|
||||
xmlContent: `<file><path>${relPath}</path>\n<content/><notice>File is empty</notice>\n</file>`,
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
try {
|
||||
const defResult = await parseSourceCodeDefinitionsForFile(fullPath, cline.rooIgnoreController)
|
||||
if (defResult) {
|
||||
|
|
@ -491,6 +507,14 @@ export async function readFileTool(
|
|||
|
||||
// Handle files exceeding line threshold
|
||||
if (maxReadFileLine > 0 && totalLines > maxReadFileLine) {
|
||||
// Check if file is empty first
|
||||
if (totalLines === 0) {
|
||||
updateFileResult(relPath, {
|
||||
xmlContent: `<file><path>${relPath}</path>\n<content/><notice>File is empty</notice>\n</file>`,
|
||||
})
|
||||
continue
|
||||
}
|
||||
|
||||
const content = addLineNumbers(await readLines(fullPath, maxReadFileLine - 1, 0))
|
||||
const lineRangeAttr = ` lines="1-${maxReadFileLine}"`
|
||||
let xmlInfo = `<content${lineRangeAttr}>\n${content}</content>\n`
|
||||
|
|
@ -519,9 +543,13 @@ export async function readFileTool(
|
|||
// Handle normal file read
|
||||
const content = await extractTextFromFile(fullPath)
|
||||
const lineRangeAttr = ` lines="1-${totalLines}"`
|
||||
let xmlInfo = totalLines > 0 ? `<content${lineRangeAttr}>\n${content}</content>\n` : `<content/>`
|
||||
|
||||
if (totalLines === 0) {
|
||||
// Check if file is empty (either totalLines is 0 or content indicates empty file)
|
||||
const isEmptyFile = totalLines === 0 || content === "This file is empty"
|
||||
|
||||
let xmlInfo = isEmptyFile ? `<content/>` : `<content${lineRangeAttr}>\n${content}</content>\n`
|
||||
|
||||
if (isEmptyFile) {
|
||||
xmlInfo += `<notice>File is empty</notice>\n`
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -144,4 +144,19 @@ describe("read_file tool with maxReadFileLine setting", () => {
|
|||
expect(readLines).toHaveBeenCalledWith(filePath, maxReadFileLine - 1, 0)
|
||||
expect(addLineNumbers).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// Test for empty file handling
|
||||
it("should return 'This file is empty' message for empty files", async () => {
|
||||
// Mock extractTextFromFile to simulate empty file behavior
|
||||
;(extractTextFromFile as Mock).mockResolvedValue("This file is empty")
|
||||
|
||||
const filePath = path.resolve("/test", "emptyFile.txt")
|
||||
|
||||
// Test the mocked behavior
|
||||
const result = await extractTextFromFile(filePath)
|
||||
|
||||
// Should return the empty file message
|
||||
expect(result).toBe("This file is empty")
|
||||
expect(extractTextFromFile).toHaveBeenCalledWith(filePath)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -67,7 +67,12 @@ export async function extractTextFromFile(filePath: string): Promise<string> {
|
|||
const isBinary = await isBinaryFile(filePath).catch(() => false)
|
||||
|
||||
if (!isBinary) {
|
||||
return addLineNumbers(await fs.readFile(filePath, "utf8"))
|
||||
const content = await fs.readFile(filePath, "utf8")
|
||||
// Check if file is empty and provide clear feedback
|
||||
if (content === "") {
|
||||
return "This file is empty"
|
||||
}
|
||||
return addLineNumbers(content)
|
||||
} else {
|
||||
throw new Error(`Cannot read text for file type: ${fileExtension}`)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue