-
Notifications
You must be signed in to change notification settings - Fork 8.7k
fix(cli) : fixed bug #8310 where /memory refresh will create discrepancies with initial memory load ignoring settings/config for trusted folder and file filters #10611
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
Conversation
- Refactored /memory refresh command to make sure it uses loadHierarchicalGeminiMemory to match the initial memory load on gemini.tsx - Made sure that the number of files in the UI was also refreshed as part of this command call. - Used the right call to determine folder trust in this context. - Fixed file filtering options to use the same defaults accross the board comming from DEFAULT_MEMORY_FILE_FILTERING_OPTIONS - Fixed tests and integrations tests across the board
Summary of ChangesHello @sgnagnarella, 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 addresses a critical bug where 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. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly refactors the /memory refresh
command to use the same memory loading logic as the initial startup, fixing inconsistencies with file filtering and folder trust settings. The changes align the refresh behavior with the initial load by using the loadHierarchicalGeminiMemory
wrapper and updating the UI with the new file count. My review includes one high-severity suggestion regarding a change in the @google/gemini-cli-core
package. While the fix for the CLI is correct, changing the default file filtering options in the core Config
class to be memory-specific could introduce subtle bugs in other areas. I've recommended using more generic defaults in the core package and letting the CLI layer handle the specifics, which would improve the overall architecture and prevent potential side effects.
…ages using more generic DEFAULTS for file filtering)
targetDir: tempDir, | ||
debugMode: false, | ||
fileFilteringRespectGitIgnore: undefined, // Should default to true | ||
fileFilteringRespectGitIgnore: undefined, // Should default to DEFAULT_FILE_FILTERING_OPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes good catch!
if (config) { | ||
const { memoryContent, fileCount, filePaths } = | ||
await loadServerHierarchicalMemory( | ||
await loadHierarchicalGeminiMemory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that these are wrappers of each other.
LG.
LGTM for the folder trust related changes. Thanks for fixing! |
…ncies with initial memory load ignoring settings/config for trusted folder and file filters (#10611) Co-authored-by: Bryan Morgan <[email protected]>
… /memory refresh will create discrepancies with initial memory load ignoring settings/config for trusted folder and file filters (google-gemini#10611)
…eate discrepancies with initial memory load ignoring settings/config for trusted folder and file filters (google-gemini#10611) Co-authored-by: Bryan Morgan <[email protected]>
TLDR
Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs