Fix EISDIR warnings and Max Stack Size errors for issue #21527#25444
Fix EISDIR warnings and Max Stack Size errors for issue #21527#25444ProthamD wants to merge 4 commits intogoogle-gemini:mainfrom
Conversation
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and stability of the CLI by addressing two critical failure modes. It improves file system interaction logic to correctly distinguish between files and directories, preventing erroneous processing. Additionally, it introduces a safeguard against excessive recursion in command processing, ensuring the application remains stable even with complex or deeply nested inputs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the .gitignore file and improves file handling logic in the core package by verifying file types during existence and binary checks. It also introduces a depth parameter in the CLI's command processor to mitigate potential recursion issues; however, the current implementation is incomplete as the depth is never incremented or passed to subsequent calls, rendering the guard ineffective.
| if (depth > 2) { | ||
| return { processedQuery: [{ text: query }] }; | ||
| } |
There was a problem hiding this comment.
The depth parameter and the check if (depth > 2) have been added to prevent a RangeError: Maximum call stack size exceeded, but the implementation appears incomplete. There is no recursive call to handleAtCommand within this function, nor is the depth parameter incremented and passed to any other function that might call it back. Consequently, depth will always remain at its default value of 0, and the check will never trigger. If the recursion is intended to happen within this function (e.g., to expand nested @ commands in the content of read files), the recursive call is missing. If the recursion happens in the caller, the caller must be updated to pass an incremented depth value.
References
- A recursive error/reconnect handler is acceptable as long as it includes a mechanism to limit the maximum number of retry attempts to prevent infinite loops.
gemini-cli
bot
added
priority/p1
…gs nested within the actual returned text
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces recursive @ command processing with a depth limit to handle nested file references and improves file system robustness by explicitly checking for file types before processing. I have no feedback to provide.
|
/gemini check if the code is safe |
|
The changes in this pull request are designed to improve the robustness and stability of the CLI by addressing two specific, identified failure modes: EISDIR errors during file processing and stack overflow errors during recursive command expansion. Safety Assessment
ConclusionThe changes are targeted, follow standard defensive programming practices, and directly address the issues described in the PR. The logic is sound and does not introduce obvious security vulnerabilities or performance regressions. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces recursive processing for '@' commands with a depth limit to handle nested file references. It also refines file validation in 'read-many-files.ts' and implements a custom binary file detection logic in 'fileUtils.ts'. A critical issue was identified in the 'isBinaryFile' implementation where an undeclared variable and a missing file handle closure could lead to race conditions and resource leaks.
| fh = await fs.promises.open(filePath, 'r'); | ||
| const stats = await fh.stat(); | ||
| if (stats.isDirectory()) return false; | ||
| const fileSize = stats.size; | ||
| if (fileSize === 0) return false; // empty is not binary | ||
|
|
||
| // Sample up to 4KB from the head (previous behavior) | ||
| const sampleSize = Math.min(4096, fileSize); | ||
| const buf = Buffer.alloc(sampleSize); | ||
| const { bytesRead } = await fh.read(buf, 0, sampleSize, 0); | ||
| if (bytesRead === 0) return false; | ||
|
|
||
| // BOM → text (avoid false positives for UTF‑16/32 with nulls) | ||
| const bom = detectBOM(buf.subarray(0, Math.min(4, bytesRead))); | ||
| if (bom) return false; | ||
|
|
||
| let nonPrintableCount = 0; | ||
| for (let i = 0; i < bytesRead; i++) { | ||
| if (buf[i] === 0) return true; // strong indicator of binary when no BOM | ||
| if (buf[i] < 9 || (buf[i] > 13 && buf[i] < 32)) { | ||
| nonPrintableCount++; | ||
| } | ||
| } | ||
| // If >30% non-printable characters, consider it binary | ||
| return nonPrintableCount / bytesRead > 0.3; |
There was a problem hiding this comment.
The isBinaryFile function contains a critical file descriptor leak and a race condition. The file handle opened with fs.promises.open is not properly closed, which can exhaust file descriptors and lead to a Denial of Service (DoS). Additionally, the variable fh is used without being declared, making it a global variable. In an async function, this creates a race condition where concurrent calls can overwrite fh, leading to unpredictable behavior and potential crashes. To remediate, declare fh as a local variable and ensure it is always closed using a try...finally block. This aligns with repository rules regarding avoiding global state for concurrency and using asynchronous file system operations.
export async function isBinaryFile(filePath: string): Promise<boolean> {
let fh;
try {
fh = await fs.promises.open(filePath, 'r');
const stats = await fh.stat();
if (stats.isDirectory()) return false;
const fileSize = stats.size;
if (fileSize === 0) return false; // empty is not binary
// Sample up to 4KB from the head (previous behavior)
const sampleSize = Math.min(4096, fileSize);
const buf = Buffer.alloc(sampleSize);
const { bytesRead } = await fh.read(buf, 0, sampleSize, 0);
if (bytesRead === 0) return false;
// BOM → text (avoid false positives for UTF‑16/32 with nulls)
const bom = detectBOM(buf.subarray(0, Math.min(4, bytesRead)));
if (bom) return false;
let nonPrintableCount = 0;
for (let i = 0; i < bytesRead; i++) {
if (buf[i] === 0) return true; // strong indicator of binary when no BOM
if (buf[i] < 9 || (buf[i] > 13 && buf[i] < 32)) {
nonPrintableCount++;
}
}
// If >30% non-printable characters, consider it binary
return nonPrintableCount / bytesRead > 0.3;
} catch (error) {
debugLogger.warn(
`Failed to check if file is binary: ${filePath}`,
error instanceof Error ? error.message : String(error),
);
return false;
} finally {
if (fh) {
await fh.close();
}
}
}References
- Avoid module-level global variables for state to prevent race conditions and memory issues in concurrent environments.
- Use asynchronous file system operations (e.g., fs.promises) instead of synchronous ones to avoid blocking the event loop.
ProthamD
force-pushed
the
fix/issue-21527
branch
from
c9dc42a to
96a80eb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces recursive @ command processing within the CLI, limited to a depth of two to prevent infinite recursion. It also replaces the external 'isbinaryfile' dependency with a custom heuristic that analyzes the first 4KB of a file for non-printable characters and UTF-8 validity. Additionally, file existence checks in the core package were updated to specifically verify file types. I have no feedback to provide.
1 participant
Description
Fixes #21527.
This PR addresses two critical failure modes in the CLI related to large inputs and glob configurations:
EISDIR Warning Error (
isBinaryFile)The
read-many-filestool usesglob, which sometimes returned literal dictionary paths if they were inadvertently treated as existant files. By switching fromfsPromises.access()tofsPromises.stat()and correctly using.isFile(), this limits the initial inclusion glob pool to files only. A subsequent checkif (stats.isDirectory()) return false;adds an extra layer of protection aroundisBinaryFile.RangeError: Maximum call stack size exceededhandleAtCommandin theatCommandProcessor.tscould recursively invoke itself on nested path lookups when@commands returned file text that itself included further@symbols, eventually exceeding the stack limit on deep, cyclic paths. Now adepthparameter truncates expansion past a safe threshold (a depth of2).These safeguards make the execution significantly more robust when dealing with generic patterns and large text buffers.
Testing
Tested manually with problematic inclusion paths and deeply nested queries. Unit tests verified.