From 17b08730dc0e294de09d742bf0c2065f9c831102 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 26 Jul 2025 10:17:13 +0000 Subject: [PATCH 1/4] fix: handle RangeError in isbinaryfile library - Add try-catch blocks around all isBinaryFile calls - Treat files as binary when isbinaryfile throws RangeError - Prevents extension from freezing when processing certain files - Fixes #6242 --- src/core/mentions/index.ts | 9 ++++++++- src/core/tools/readFileTool.ts | 12 +++++++++++- src/integrations/misc/extract-text.ts | 9 ++++++++- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/core/mentions/index.ts b/src/core/mentions/index.ts index d0d305d0965..4f9607c4a7a 100644 --- a/src/core/mentions/index.ts +++ b/src/core/mentions/index.ts @@ -268,7 +268,14 @@ async function getFileOrFolderContent( fileContentPromises.push( (async () => { try { - const isBinary = await isBinaryFile(absoluteFilePath).catch(() => false) + 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 } diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 6de8dd56421..5d878c5f810 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -433,7 +433,17 @@ export async function readFileTool( // Process approved files try { - const [totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)]) + let totalLines: number + let isBinary: boolean + + try { + ;[totalLines, isBinary] = await Promise.all([countFileLines(fullPath), 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) + totalLines = await countFileLines(fullPath) + isBinary = true + } // Handle binary files (but allow specific file types that extractTextFromFile can handle) if (isBinary) { diff --git a/src/integrations/misc/extract-text.ts b/src/integrations/misc/extract-text.ts index 8231c609be7..a2a9c722eed 100644 --- a/src/integrations/misc/extract-text.ts +++ b/src/integrations/misc/extract-text.ts @@ -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 From ecb09af00749ee6156c40d019c391e1b5a92abbf Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 29 Jul 2025 22:43:39 +0000 Subject: [PATCH 2/4] fix: address PR review feedback - Remove unusual semicolon prefix in readFileTool.ts and use conventional approach - Add test coverage for RangeError handling in extract-text.ts - Console warning messages are already consistent across files - Error handling patterns are already consistent (single try-catch) --- src/core/tools/readFileTool.ts | 4 +++- .../__tests__/extract-text-large-files.spec.ts | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 5d878c5f810..04b91a2189e 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -437,7 +437,9 @@ export async function readFileTool( let isBinary: boolean try { - ;[totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)]) + const results = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)]) + totalLines = results[0] + isBinary = results[1] } 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) diff --git a/src/integrations/misc/__tests__/extract-text-large-files.spec.ts b/src/integrations/misc/__tests__/extract-text-large-files.spec.ts index fc2f7f54b6b..e164c070ab0 100644 --- a/src/integrations/misc/__tests__/extract-text-large-files.spec.ts +++ b/src/integrations/misc/__tests__/extract-text-large-files.spec.ts @@ -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 () => { @@ -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), + ) + }) }) From 6796e1a2855ee03b0225ca037fd796a3860ba1a7 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Wed, 30 Jul 2025 00:04:26 +0000 Subject: [PATCH 3/4] fix: address review feedback for nested try-catch pattern and add test coverage - Remove nested try-catch pattern in src/core/mentions/index.ts for consistency - Add test coverage for RangeError handling in readFileTool - Ensure graceful error handling when isBinaryFile throws RangeError --- src/core/mentions/index.ts | 22 +++++++++---------- src/core/tools/__tests__/readFileTool.spec.ts | 21 ++++++++++++++++++ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/core/mentions/index.ts b/src/core/mentions/index.ts index 4f9607c4a7a..881c2cc292e 100644 --- a/src/core/mentions/index.ts +++ b/src/core/mentions/index.ts @@ -267,18 +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 { - 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 - } const content = await extractTextFromFile(absoluteFilePath, maxReadFileLine) return `\n${content}\n` } catch (error) { diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 44be1d3b924..ab7e69eee77 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -518,5 +518,26 @@ describe("read_file tool XML output structure", () => { `\n${testFilePath}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.\n`, ) }) + + 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(`${testFilePath}`) + + // Verify that we get a valid XML response (not an error) + expect(result).toMatch(/.*<\/files>/s) + expect(result).not.toContain("") + }) }) }) From 7718bef258df1f9e3ba8ad99047d22d102bb61e5 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Thu, 31 Jul 2025 11:32:07 -0500 Subject: [PATCH 4/4] fix: separate countFileLines and isBinaryFile calls for accurate error handling - Split Promise.all() into sequential calls to isolate error sources - countFileLines is called first and should not fail for valid files - Only isBinaryFile errors are caught and handled by treating file as binary - This ensures RangeError from isBinaryFile doesn't incorrectly attribute to countFileLines - Fixes issue where binary file detection errors were masking actual file reading problems --- src/core/tools/readFileTool.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 04b91a2189e..b8d3f8989cf 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -436,14 +436,15 @@ export async function readFileTool( 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 { - const results = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)]) - totalLines = results[0] - isBinary = results[1] + 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) - totalLines = await countFileLines(fullPath) isBinary = true }