From b78c7d5563ad5d3b0ec920c2c117834ce9d4c65f Mon Sep 17 00:00:00 2001 From: Seba Gnagnarella Date: Mon, 6 Oct 2025 14:24:09 -0400 Subject: [PATCH 1/2] fix bug #8310 : - 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 --- .../cli/src/config/config.integration.test.ts | 29 +++++++++---- packages/cli/src/config/config.test.ts | 7 +++- packages/cli/src/config/config.ts | 2 +- .../cli/src/ui/commands/memoryCommand.test.ts | 41 +++++++++++++------ packages/cli/src/ui/commands/memoryCommand.ts | 17 ++++---- packages/core/src/config/config.test.ts | 10 ++++- packages/core/src/config/config.ts | 11 ++++- 7 files changed, 82 insertions(+), 35 deletions(-) diff --git a/packages/cli/src/config/config.integration.test.ts b/packages/cli/src/config/config.integration.test.ts index 78f69bda987..a14e3e2f7cb 100644 --- a/packages/cli/src/config/config.integration.test.ts +++ b/packages/cli/src/config/config.integration.test.ts @@ -12,7 +12,10 @@ import type { ConfigParameters, ContentGeneratorConfig, } from '@google/gemini-cli-core'; -import { Config } from '@google/gemini-cli-core'; +import { + Config, + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, +} from '@google/gemini-cli-core'; import { http, HttpResponse } from 'msw'; import { setupServer } from 'msw/node'; @@ -78,12 +81,14 @@ describe('Configuration Integration Tests', () => { sandbox: false, targetDir: tempDir, debugMode: false, - fileFilteringRespectGitIgnore: undefined, // Should default to true + fileFilteringRespectGitIgnore: undefined, // Should default to DEFAULT_MEMORY_FILE_FILTERING_OPTIONS }; const config = new Config(configParams); - expect(config.getFileFilteringRespectGitIgnore()).toBe(true); + expect(config.getFileFilteringRespectGitIgnore()).toBe( + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + ); }); it('should load custom file filtering settings from configuration', async () => { @@ -112,7 +117,9 @@ describe('Configuration Integration Tests', () => { sandbox: false, targetDir: tempDir, debugMode: false, - fileFilteringRespectGitIgnore: true, + fileFiltering: { + respectGitIgnore: true, + }, }; const config = new Config(configParams); @@ -149,13 +156,15 @@ describe('Configuration Integration Tests', () => { sandbox: false, targetDir: tempDir, debugMode: false, - fileFilteringRespectGitIgnore: undefined, + fileFiltering: {}, }; const config = new Config(configParams); // All settings should use defaults - expect(config.getFileFilteringRespectGitIgnore()).toBe(true); + expect(config.getFileFilteringRespectGitIgnore()).toBe( + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + ); }); it('should handle missing configuration sections gracefully', async () => { @@ -172,7 +181,9 @@ describe('Configuration Integration Tests', () => { const config = new Config(configParams); // All git-aware settings should use defaults - expect(config.getFileFilteringRespectGitIgnore()).toBe(true); + expect(config.getFileFilteringRespectGitIgnore()).toBe( + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + ); }); }); @@ -185,7 +196,9 @@ describe('Configuration Integration Tests', () => { sandbox: false, targetDir: tempDir, debugMode: false, - fileFilteringRespectGitIgnore: true, + fileFiltering: { + respectGitIgnore: true, + }, }; const config = new Config(configParams); diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index e6bb244f591..a334fab2fa7 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -15,7 +15,12 @@ import { DEFAULT_GEMINI_MODEL_AUTO, OutputFormat, } from '@google/gemini-cli-core'; -import { loadCliConfig, parseArguments, type CliArgs } from './config.js'; +import { + loadCliConfig, + loadHierarchicalGeminiMemory, + parseArguments, + type CliArgs, +} from './config.js'; import type { Settings } from './settings.js'; import { ExtensionStorage, type Extension } from './extension.js'; import * as ServerConfig from '@google/gemini-cli-core'; diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 805f2f4e1a2..16f17088161 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -668,7 +668,7 @@ export async function loadCliConfig( }, telemetry: telemetrySettings, usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled ?? true, - fileFiltering: settings.context?.fileFiltering, + fileFiltering, checkpointing: argv.checkpointing || settings.general?.checkpointing?.enabled, proxy: diff --git a/packages/cli/src/ui/commands/memoryCommand.test.ts b/packages/cli/src/ui/commands/memoryCommand.test.ts index 2c285368a4d..3cb89873570 100644 --- a/packages/cli/src/ui/commands/memoryCommand.test.ts +++ b/packages/cli/src/ui/commands/memoryCommand.test.ts @@ -13,10 +13,10 @@ import { MessageType } from '../types.js'; import type { LoadedSettings } from '../../config/settings.js'; import { getErrorMessage, - loadServerHierarchicalMemory, type FileDiscoveryService, } from '@google/gemini-cli-core'; import type { LoadServerHierarchicalMemoryResponse } from '@google/gemini-cli-core/index.js'; +import { loadHierarchicalGeminiMemory } from '../../config/config.js'; vi.mock('@google/gemini-cli-core', async (importOriginal) => { const original = @@ -27,11 +27,19 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { if (error instanceof Error) return error.message; return String(error); }), - loadServerHierarchicalMemory: vi.fn(), }; }); -const mockLoadServerHierarchicalMemory = loadServerHierarchicalMemory as Mock; +vi.mock('../../config/config.js', async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + loadHierarchicalGeminiMemory: vi.fn(), + }; +}); + +const mockLoadHierarchicalGeminiMemory = loadHierarchicalGeminiMemory as Mock; describe('memoryCommand', () => { let mockContext: CommandContext; @@ -177,7 +185,7 @@ describe('memoryCommand', () => { ignore: [], include: [], }), - getFolderTrust: () => false, + isTrustedFolder: () => false, }; mockContext = createMockCommandContext({ @@ -186,11 +194,17 @@ describe('memoryCommand', () => { settings: { merged: { memoryDiscoveryMaxDirs: 1000, + context: { + importFormat: 'tree', + }, }, } as LoadedSettings, }, + ui: { + setGeminiMdFileCount: vi.fn(), + }, }); - mockLoadServerHierarchicalMemory.mockClear(); + mockLoadHierarchicalGeminiMemory.mockClear(); }); it('should display success message when memory is refreshed with content', async () => { @@ -201,7 +215,7 @@ describe('memoryCommand', () => { fileCount: 2, filePaths: ['/path/one/GEMINI.md', '/path/two/GEMINI.md'], }; - mockLoadServerHierarchicalMemory.mockResolvedValue(refreshResult); + mockLoadHierarchicalGeminiMemory.mockResolvedValue(refreshResult); await refreshCommand.action(mockContext, ''); @@ -213,7 +227,7 @@ describe('memoryCommand', () => { expect.any(Number), ); - expect(loadServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockLoadHierarchicalGeminiMemory).toHaveBeenCalledOnce(); expect(mockSetUserMemory).toHaveBeenCalledWith( refreshResult.memoryContent, ); @@ -223,6 +237,9 @@ describe('memoryCommand', () => { expect(mockSetGeminiMdFilePaths).toHaveBeenCalledWith( refreshResult.filePaths, ); + expect(mockContext.ui.setGeminiMdFileCount).toHaveBeenCalledWith( + refreshResult.fileCount, + ); expect(mockContext.ui.addItem).toHaveBeenCalledWith( { @@ -237,11 +254,11 @@ describe('memoryCommand', () => { if (!refreshCommand.action) throw new Error('Command has no action'); const refreshResult = { memoryContent: '', fileCount: 0, filePaths: [] }; - mockLoadServerHierarchicalMemory.mockResolvedValue(refreshResult); + mockLoadHierarchicalGeminiMemory.mockResolvedValue(refreshResult); await refreshCommand.action(mockContext, ''); - expect(loadServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockLoadHierarchicalGeminiMemory).toHaveBeenCalledOnce(); expect(mockSetUserMemory).toHaveBeenCalledWith(''); expect(mockSetGeminiMdFileCount).toHaveBeenCalledWith(0); expect(mockSetGeminiMdFilePaths).toHaveBeenCalledWith([]); @@ -259,11 +276,11 @@ describe('memoryCommand', () => { if (!refreshCommand.action) throw new Error('Command has no action'); const error = new Error('Failed to read memory files.'); - mockLoadServerHierarchicalMemory.mockRejectedValue(error); + mockLoadHierarchicalGeminiMemory.mockRejectedValue(error); await refreshCommand.action(mockContext, ''); - expect(loadServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockLoadHierarchicalGeminiMemory).toHaveBeenCalledOnce(); expect(mockSetUserMemory).not.toHaveBeenCalled(); expect(mockSetGeminiMdFileCount).not.toHaveBeenCalled(); expect(mockSetGeminiMdFilePaths).not.toHaveBeenCalled(); @@ -298,7 +315,7 @@ describe('memoryCommand', () => { expect.any(Number), ); - expect(loadServerHierarchicalMemory).not.toHaveBeenCalled(); + expect(mockLoadHierarchicalGeminiMemory).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/commands/memoryCommand.ts b/packages/cli/src/ui/commands/memoryCommand.ts index b8fb4c03f75..e81c1733ae7 100644 --- a/packages/cli/src/ui/commands/memoryCommand.ts +++ b/packages/cli/src/ui/commands/memoryCommand.ts @@ -4,11 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - getErrorMessage, - loadServerHierarchicalMemory, -} from '@google/gemini-cli-core'; +import { getErrorMessage } from '@google/gemini-cli-core'; import { MessageType } from '../types.js'; +import { loadHierarchicalGeminiMemory } from '../../config/config.js'; import type { SlashCommand, SlashCommandActionReturn } from './types.js'; import { CommandKind } from './types.js'; @@ -82,25 +80,26 @@ export const memoryCommand: SlashCommand = { try { const config = await context.services.config; + const settings = context.services.settings; if (config) { const { memoryContent, fileCount, filePaths } = - await loadServerHierarchicalMemory( + await loadHierarchicalGeminiMemory( config.getWorkingDir(), config.shouldLoadMemoryFromIncludeDirectories() ? config.getWorkspaceContext().getDirectories() : [], config.getDebugMode(), config.getFileService(), + settings.merged, config.getExtensionContextFilePaths(), - config.getFolderTrust(), - context.services.settings.merged.context?.importFormat || - 'tree', // Use setting or default to 'tree' + config.isTrustedFolder(), + settings.merged.context?.importFormat || 'tree', config.getFileFilteringOptions(), - context.services.settings.merged.context?.discoveryMaxDirs, ); config.setUserMemory(memoryContent); config.setGeminiMdFileCount(fileCount); config.setGeminiMdFilePaths(filePaths); + context.ui.setGeminiMdFileCount(fileCount); const successMessage = memoryContent.length > 0 diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 99d43b08aa7..530f5120c15 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -7,7 +7,11 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Mock } from 'vitest'; import type { ConfigParameters, SandboxConfig } from './config.js'; -import { Config, ApprovalMode } from './config.js'; +import { + Config, + ApprovalMode, + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, +} from './config.js'; import * as path from 'node:path'; import { setGeminiMdFilename as mockSetGeminiMdFilename } from '../tools/memoryTool.js'; import { @@ -318,7 +322,9 @@ describe('Server Config (config.ts)', () => { it('should set default file filtering settings when not provided', () => { const config = new Config(baseParams); - expect(config.getFileFilteringRespectGitIgnore()).toBe(true); + expect(config.getFileFilteringRespectGitIgnore()).toBe( + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + ); }); it('should set custom file filtering settings when provided', () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 1765e329a5a..abf31ed92fc 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -393,8 +393,12 @@ export class Config { this.usageStatisticsEnabled = params.usageStatisticsEnabled ?? true; this.fileFiltering = { - respectGitIgnore: params.fileFiltering?.respectGitIgnore ?? true, - respectGeminiIgnore: params.fileFiltering?.respectGeminiIgnore ?? true, + respectGitIgnore: + params.fileFiltering?.respectGitIgnore ?? + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + respectGeminiIgnore: + params.fileFiltering?.respectGeminiIgnore ?? + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGeminiIgnore, enableRecursiveFileSearch: params.fileFiltering?.enableRecursiveFileSearch ?? true, disableFuzzySearch: params.fileFiltering?.disableFuzzySearch ?? false, @@ -872,6 +876,9 @@ export class Config { return this.ideMode; } + /*** + * TODO: Review if this is actually used at it seems it is only used but the FileCommandLoader + */ getFolderTrustFeature(): boolean { return this.folderTrustFeature; } From 419923a60f2198beea504fcc782387392101cdae Mon Sep 17 00:00:00 2001 From: Seba Gnagnarella Date: Mon, 6 Oct 2025 15:17:35 -0400 Subject: [PATCH 2/2] Applying changes suggested by Gemini CLI code review (leave core packages using more generic DEFAULTS for file filtering) --- packages/cli/src/config/config.integration.test.ts | 10 +++++----- packages/core/src/config/config.test.ts | 4 ++-- packages/core/src/config/config.ts | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/config/config.integration.test.ts b/packages/cli/src/config/config.integration.test.ts index a14e3e2f7cb..66f24a48058 100644 --- a/packages/cli/src/config/config.integration.test.ts +++ b/packages/cli/src/config/config.integration.test.ts @@ -14,7 +14,7 @@ import type { } from '@google/gemini-cli-core'; import { Config, - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, + DEFAULT_FILE_FILTERING_OPTIONS, } from '@google/gemini-cli-core'; import { http, HttpResponse } from 'msw'; import { setupServer } from 'msw/node'; @@ -81,13 +81,13 @@ describe('Configuration Integration Tests', () => { sandbox: false, targetDir: tempDir, debugMode: false, - fileFilteringRespectGitIgnore: undefined, // Should default to DEFAULT_MEMORY_FILE_FILTERING_OPTIONS + fileFilteringRespectGitIgnore: undefined, // Should default to DEFAULT_FILE_FILTERING_OPTIONS }; const config = new Config(configParams); expect(config.getFileFilteringRespectGitIgnore()).toBe( - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, ); }); @@ -163,7 +163,7 @@ describe('Configuration Integration Tests', () => { // All settings should use defaults expect(config.getFileFilteringRespectGitIgnore()).toBe( - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, ); }); @@ -182,7 +182,7 @@ describe('Configuration Integration Tests', () => { // All git-aware settings should use defaults expect(config.getFileFilteringRespectGitIgnore()).toBe( - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, ); }); }); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 530f5120c15..df915390b8f 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -10,7 +10,7 @@ import type { ConfigParameters, SandboxConfig } from './config.js'; import { Config, ApprovalMode, - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, + DEFAULT_FILE_FILTERING_OPTIONS, } from './config.js'; import * as path from 'node:path'; import { setGeminiMdFilename as mockSetGeminiMdFilename } from '../tools/memoryTool.js'; @@ -323,7 +323,7 @@ describe('Server Config (config.ts)', () => { it('should set default file filtering settings when not provided', () => { const config = new Config(baseParams); expect(config.getFileFilteringRespectGitIgnore()).toBe( - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, ); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index abf31ed92fc..266fd81cd50 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -134,14 +134,14 @@ export interface ExtensionInstallMetadata { import type { FileFilteringOptions } from './constants.js'; import { - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, DEFAULT_FILE_FILTERING_OPTIONS, + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, } from './constants.js'; export type { FileFilteringOptions }; export { - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, DEFAULT_FILE_FILTERING_OPTIONS, + DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, }; export const DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 4_000_000; @@ -395,10 +395,10 @@ export class Config { this.fileFiltering = { respectGitIgnore: params.fileFiltering?.respectGitIgnore ?? - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGitIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGitIgnore, respectGeminiIgnore: params.fileFiltering?.respectGeminiIgnore ?? - DEFAULT_MEMORY_FILE_FILTERING_OPTIONS.respectGeminiIgnore, + DEFAULT_FILE_FILTERING_OPTIONS.respectGeminiIgnore, enableRecursiveFileSearch: params.fileFiltering?.enableRecursiveFileSearch ?? true, disableFuzzySearch: params.fileFiltering?.disableFuzzySearch ?? false,