fix(commands): ignore empty GEMINI.md to allow re-initialization#25010
fix(commands): ignore empty GEMINI.md to allow re-initialization#25010student-ankitpandit wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
|
/gemini review |
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 improves the robustness of the 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
|
There was a problem hiding this comment.
Code Review
This pull request updates the initCommand to check for existing content in GEMINI.md using fs.promises.readFile instead of just checking for file existence. It also adds robust error handling for file reading and includes comprehensive unit tests to cover various scenarios, such as existing content, empty files, and read errors. I have identified an improvement opportunity regarding the error handling implementation to ensure better type safety and avoid unnecessary lint suppression.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const error = e as NodeJS.ErrnoException; | ||
| if (error.code !== 'ENOENT') { | ||
| return { | ||
| type: 'message', | ||
| messageType: 'error', | ||
| content: `Failed to read GEMINI.md: ${error.message}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The error handling for readFile uses an unsafe type assertion and suppresses a lint warning. This approach is fragile as it assumes e will always be an object with a code property. If e is not an object (which can happen in JavaScript), accessing error.code will throw, causing the command to crash. A more robust approach using an instanceof Error check is preferred to ensure safety and remove the need for lint suppression.
if (e instanceof Error && (e as NodeJS.ErrnoException).code !== 'ENOENT') {
return {
type: 'message',
messageType: 'error',
content: `Failed to read GEMINI.md: ${e.message}`,
};
}There was a problem hiding this comment.
Code Review
This pull request refactors the initCommand to use fs.promises.readFile instead of fs.existsSync for checking the presence and content of GEMINI.md. This change allows for more robust error handling, distinguishing between a non-existent file and an empty file, and handling other file read errors. Corresponding test cases in initCommand.test.ts have been updated and expanded to cover these new scenarios, including tests for existing content, empty files, and various read errors. No feedback is provided as there were no review comments to assess.
Summary
The
/initcommand was blocked from re-running if aGEMINI.mdfile existed but was empty — a common state left behind when a previous/initwas cancelled mid-way. This fix changes the check from file existence to file content, so an emptyGEMINI.mdis treated the same as no file at all.Details
Previously,
/initcreated an emptyGEMINI.mdbefore starting the LLM analysis. If the process was cancelled or failed mid-way, this empty file was left behind — permanently blocking re-initialization since the check only tested for file existence, not content.The fix changes the check from
fs.existsSync()to also read the file content, so/initonly blocks ifGEMINI.mdexists AND has non-whitespace content. An empty file is now treated the same as no file.A new test case was added to cover the empty file scenario.
Related Issues
Fixes #22757
How to Validate
mkdir /tmp/g-cli-test && cd /tmp/g-cli-test/init: touchGEMINI.mdgeminiand type/initinitproceeds with analysis (not blocked)GEMINI.mdstill blocks re-initializationPre-Merge Checklist