Skip to content

fix: handle RangeError in isbinaryfile library #6243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/core/mentions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,11 +267,18 @@ async function getFileOrFolderContent(
const absoluteFilePath = path.resolve(absPath, entry.name)
fileContentPromises.push(
(async () => {
let isBinary = false
try {
isBinary = await isBinaryFile(absoluteFilePath)
} catch (error) {
// If isBinaryFile throws an error (e.g., RangeError), treat as binary
console.warn(`Error checking if file is binary for ${absoluteFilePath}:`, error)
isBinary = true
}
if (isBinary) {
return undefined
}
try {
const isBinary = await isBinaryFile(absoluteFilePath).catch(() => false)
if (isBinary) {
return undefined
}
const content = await extractTextFromFile(absoluteFilePath, maxReadFileLine)
return `<file_content path="${filePath.toPosix()}">\n${content}\n</file_content>`
} catch (error) {
Expand Down
21 changes: 21 additions & 0 deletions src/core/tools/__tests__/readFileTool.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -518,5 +518,26 @@ 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 RangeError from isBinaryFile gracefully", async () => {
// Setup - mock isBinaryFile to throw RangeError
mockedIsBinaryFile.mockRejectedValue(new RangeError("Invalid array length"))
mockedCountFileLines.mockResolvedValue(5)

// Execute - the main goal is to verify the error doesn't crash the application
const result = await executeReadFileTool(
{},
{
totalLines: 5,
},
)

// Verify that the file is processed (the error is handled gracefully)
expect(result).toContain(`<file><path>${testFilePath}</path>`)

// Verify that we get a valid XML response (not an error)
expect(result).toMatch(/<files>.*<\/files>/s)
expect(result).not.toContain("<error>")
})
})
})
15 changes: 14 additions & 1 deletion src/core/tools/readFileTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,20 @@ export async function readFileTool(

// Process approved files
try {
const [totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)])
let totalLines: number
let isBinary: boolean

// First, count the file lines (this should not fail for valid files)
totalLines = await countFileLines(fullPath)

// Then check if it's binary, with error handling specific to isBinaryFile
try {
isBinary = await isBinaryFile(fullPath)
} catch (error) {
// If isBinaryFile throws an error (e.g., RangeError), treat the file as binary
console.warn(`Error checking if file is binary for ${relPath}:`, error)
isBinary = true
}

// Handle binary files (but allow specific file types that extractTextFromFile can handle)
if (isBinary) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ describe("extractTextFromFile - Large File Handling", () => {
// Set default mock behavior
mockedFs.access.mockResolvedValue(undefined)
mockedIsBinaryFile.mockResolvedValue(false)
// Mock console.warn
vi.spyOn(console, "warn").mockImplementation(() => {})
})

it("should truncate files that exceed maxReadFileLine limit", async () => {
Expand Down Expand Up @@ -218,4 +220,20 @@ describe("extractTextFromFile - Large File Handling", () => {
"File not found: /test/nonexistent.ts",
)
})

it("should handle RangeError from isBinaryFile and treat file as binary", async () => {
// Setup - mock isBinaryFile to throw RangeError
mockedIsBinaryFile.mockRejectedValue(new RangeError("Invalid array length"))

// Execute and expect it to throw since file is treated as binary
await expect(extractTextFromFile("/test/problematic-file.bin", 100)).rejects.toThrow(
"Cannot read text for file type: .bin",
)

// Verify that the warning was logged
expect(console.warn).toHaveBeenCalledWith(
"Error checking if file is binary for /test/problematic-file.bin:",
expect.any(RangeError),
)
})
})
9 changes: 8 additions & 1 deletion src/integrations/misc/extract-text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,14 @@ export async function extractTextFromFile(filePath: string, maxReadFileLine?: nu
}

// Handle other files
const isBinary = await isBinaryFile(filePath).catch(() => false)
let isBinary = false
try {
isBinary = await isBinaryFile(filePath)
} catch (error) {
// If isBinaryFile throws an error (e.g., RangeError), treat as binary
console.warn(`Error checking if file is binary for ${filePath}:`, error)
isBinary = true
}

if (!isBinary) {
// Check if we need to apply line limit
Expand Down