fix(core): skip directories when reading GEMINI.md files

[sontoriyama]

Refactored to use EAFP pattern (catch EISDIR instead of lstat pre-check). Fixes symlink handling and avoids race conditions. Based on suggestions when testing with gemini-code-assist. Closes #16282

[gemini-code-assist]

Summary of Changes

Hello @sontoriyama, I'm Gemini Code Assist¹! 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 resolves an issue in the memoryDiscovery utility where it would attempt to read directories named "GEMINI.md" as files, resulting in unnecessary warnings. By introducing a preliminary check, the system now gracefully skips these directories, logging a debug message instead of a warning, thereby enhancing the stability and clarity of the memory discovery process.

Highlights

• Directory Handling: Implemented a check to identify and silently skip directories that match "GEMINI.md" filenames during memory discovery, preventing warnings.
• Error Prevention: Replaced previous warning messages with debug logs when a directory is encountered where a file was expected, streamlining the memory discovery process.
• Test Coverage: Added a new test case to ensure that directories are correctly skipped and valid "GEMINI.md" files are still processed as expected.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to prevent warnings when GEMINI.md memory discovery encounters a directory with a matching name by adding a check using lstat. However, a high-severity Path Traversal vulnerability has been identified due to the handling of symbolic links, which could allow an attacker to read arbitrary files. Additionally, the current implementation introduces a potential Time-of-check to time-of-use (TOCTOU) race condition; a more robust approach would be to handle the EISDIR error directly in the catch block.

[gemini-code-assist]

This code is vulnerable to a high-severity Path Traversal attack using symbolic links. The fs.lstat(filePath) check does not verify if the path is a symbolic link, allowing fs.readFile to follow malicious symlinks and potentially read arbitrary files. Furthermore, this pre-check introduces a Time-of-check to time-of-use (TOCTOU) race condition. To fix this, you must explicitly check for and reject symbolic links. For handling directories, a more robust approach would be to attempt readFile directly and handle the specific EISDIR error in the catch block.

          const stats = await fs.lstat(filePath);
          if (stats.isSymbolicLink()) {
            if (debugMode) {
              logger.debug(`Skipping symbolic link: ${filePath}`);
            }
            return { filePath, content: null };
          }
          if (stats.isDirectory()) {
            // Skip directories silently - they're valid memory folders
            if (debugMode) {
              logger.debug(
                `Skipping directory ${filePath} (expected file). Using parent directory for memory discovery instead.`
              );
            }
            return { filePath, content: null };
          }

          const content = await fs.readFile(filePath, 'utf-8');

[sontoriyama]

Thank you for the thorough review. I appreciate the feedback on both the test logic and the security concern.

Regarding the path traversal warning: I understand your concern, but I believe this may be a false positive for the following reasons:

1. filePath source: The paths come from setGeminiMdFilename() which uses internal configuration, not external user input. An attacker would need access to the local configuration files.

2. Context: These are memory filenames defined in the tool's own configuration system, not arbitrary paths from user input.

3. Defense in depth: I considered adding path traversal validation as suggested, but I'm concerned about introducing regressions without fully understanding all the edge cases in the memory discovery system. I'd prefer the maintainers to guide on whether additional validation is needed here or if it should be handled at the configuration layer.

I'm happy to implement any changes the maintainers recommend for this. Thank you again for the detailed feedback!

[sontoriyama]

Updated to use EAFP pattern based on code review feedback.

Changes:

• Replaced lstat() pre-check with try/catch for EISDIR error
• This properly handles symlinks (stat follows links, lstat does not)
• Avoids TOCTOU race conditions
• More efficient (no double syscall)
• More idiomatic Node.js pattern

Thanks for the feedback - this is definitely cleaner than the pre-check approach!

[mrcabbage972]

Thanks for the PR! Can you please create an Issue and link the PR to it?

[mrcabbage972]

Thanks for the PR! Can you please fix the failing tests?

[sontoriyama]

Closes #16282