From 15a1f1af9d0e4628e9e82f81d384d614899770e3 Mon Sep 17 00:00:00 2001 From: TIRUMALASETTI PRANITH Date: Sat, 2 Aug 2025 03:52:17 +0530 Subject: [PATCH] fix(config): Resolve duplicate config loading from home directory (#5090) Co-authored-by: Allen Hutchison Co-authored-by: Allen Hutchison --- packages/cli/src/config/config.ts | 17 ++- packages/cli/src/config/settings.test.ts | 16 ++- packages/cli/src/config/settings.ts | 59 ++++++--- packages/core/src/utils/memoryDiscovery.ts | 135 ++++++++------------- 4 files changed, 117 insertions(+), 110 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index a147bca8..9274b65e 100644 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -4,6 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as fs from 'fs'; +import * as path from 'path'; +import { homedir } from 'node:os'; import yargs from 'yargs/yargs'; import { hideBin } from 'yargs/helpers'; import process from 'node:process'; @@ -244,16 +247,24 @@ export async function loadHierarchicalGeminiMemory( memoryImportFormat: 'flat' | 'tree' = 'tree', fileFilteringOptions?: FileFilteringOptions, ): Promise<{ memoryContent: string; fileCount: number }> { + // FIX: Use real, canonical paths for a reliable comparison to handle symlinks. + const realCwd = fs.realpathSync(path.resolve(currentWorkingDirectory)); + const realHome = fs.realpathSync(path.resolve(homedir())); + const isHomeDirectory = realCwd === realHome; + + // If it is the home directory, pass an empty string to the core memory + // function to signal that it should skip the workspace search. + const effectiveCwd = isHomeDirectory ? '' : currentWorkingDirectory; + if (debugMode) { logger.debug( `CLI: Delegating hierarchical memory load to server for CWD: ${currentWorkingDirectory} (memoryImportFormat: ${memoryImportFormat})`, ); } - // Directly call the server function. - // The server function will use its own homedir() for the global path. + // Directly call the server function with the corrected path. return loadServerHierarchicalMemory( - currentWorkingDirectory, + effectiveCwd, debugMode, fileService, extensionContextFilePaths, diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 5a54e46e..b8ecbb62 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -59,7 +59,21 @@ const MOCK_WORKSPACE_SETTINGS_PATH = pathActual.join( 'settings.json', ); -vi.mock('fs'); +vi.mock('fs', async (importOriginal) => { + // Get all the functions from the real 'fs' module + const actualFs = await importOriginal(); + + return { + ...actualFs, // Keep all the real functions + // Now, just override the ones we need for the test + existsSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + realpathSync: (p: string) => p, + }; +}); + vi.mock('strip-json-comments', () => ({ default: vi.fn((content) => content), })); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 4f701c6d..a875ffc1 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -312,6 +312,22 @@ export function loadSettings(workspaceDir: string): LoadedSettings { let workspaceSettings: Settings = {}; const settingsErrors: SettingsError[] = []; const systemSettingsPath = getSystemSettingsPath(); + + // FIX: Resolve paths to their canonical representation to handle symlinks + const resolvedWorkspaceDir = path.resolve(workspaceDir); + const resolvedHomeDir = path.resolve(homedir()); + + let realWorkspaceDir = resolvedWorkspaceDir; + try { + // fs.realpathSync gets the "true" path, resolving any symlinks + realWorkspaceDir = fs.realpathSync(resolvedWorkspaceDir); + } catch (_e) { + // This is okay. The path might not exist yet, and that's a valid state. + } + + // We expect homedir to always exist and be resolvable. + const realHomeDir = fs.realpathSync(resolvedHomeDir); + // Load system settings try { if (fs.existsSync(systemSettingsPath)) { @@ -356,28 +372,31 @@ export function loadSettings(workspaceDir: string): LoadedSettings { 'settings.json', ); - // Load workspace settings - try { - if (fs.existsSync(workspaceSettingsPath)) { - const projectContent = fs.readFileSync(workspaceSettingsPath, 'utf-8'); - const parsedWorkspaceSettings = JSON.parse( - stripJsonComments(projectContent), - ) as Settings; - workspaceSettings = resolveEnvVarsInObject(parsedWorkspaceSettings); - if (workspaceSettings.theme && workspaceSettings.theme === 'VS') { - workspaceSettings.theme = DefaultLight.name; - } else if ( - workspaceSettings.theme && - workspaceSettings.theme === 'VS2015' - ) { - workspaceSettings.theme = DefaultDark.name; + // This comparison is now much more reliable. + if (realWorkspaceDir !== realHomeDir) { + // Load workspace settings + try { + if (fs.existsSync(workspaceSettingsPath)) { + const projectContent = fs.readFileSync(workspaceSettingsPath, 'utf-8'); + const parsedWorkspaceSettings = JSON.parse( + stripJsonComments(projectContent), + ) as Settings; + workspaceSettings = resolveEnvVarsInObject(parsedWorkspaceSettings); + if (workspaceSettings.theme && workspaceSettings.theme === 'VS') { + workspaceSettings.theme = DefaultLight.name; + } else if ( + workspaceSettings.theme && + workspaceSettings.theme === 'VS2015' + ) { + workspaceSettings.theme = DefaultDark.name; + } } + } catch (error: unknown) { + settingsErrors.push({ + message: getErrorMessage(error), + path: workspaceSettingsPath, + }); } - } catch (error: unknown) { - settingsErrors.push({ - message: getErrorMessage(error), - path: workspaceSettingsPath, - }); } return new LoadedSettings( diff --git a/packages/core/src/utils/memoryDiscovery.ts b/packages/core/src/utils/memoryDiscovery.ts index a673a75e..323b13c5 100644 --- a/packages/core/src/utils/memoryDiscovery.ts +++ b/packages/core/src/utils/memoryDiscovery.ts @@ -94,7 +94,6 @@ async function getGeminiMdFilePathsInternal( const geminiMdFilenames = getAllGeminiMdFilenames(); for (const geminiMdFilename of geminiMdFilenames) { - const resolvedCwd = path.resolve(currentWorkingDirectory); const resolvedHome = path.resolve(userHomePath); const globalMemoryPath = path.join( resolvedHome, @@ -102,12 +101,7 @@ async function getGeminiMdFilePathsInternal( geminiMdFilename, ); - if (debugMode) - logger.debug( - `Searching for ${geminiMdFilename} starting from CWD: ${resolvedCwd}`, - ); - if (debugMode) logger.debug(`User home directory: ${resolvedHome}`); - + // This part that finds the global file always runs. try { await fs.access(globalMemoryPath, fsSync.constants.R_OK); allPaths.add(globalMemoryPath); @@ -116,102 +110,71 @@ async function getGeminiMdFilePathsInternal( `Found readable global ${geminiMdFilename}: ${globalMemoryPath}`, ); } catch { + // It's okay if it's not found. + } + + // FIX: Only perform the workspace search (upward and downward scans) + // if a valid currentWorkingDirectory is provided. + if (currentWorkingDirectory) { + const resolvedCwd = path.resolve(currentWorkingDirectory); if (debugMode) logger.debug( - `Global ${geminiMdFilename} not found or not readable: ${globalMemoryPath}`, + `Searching for ${geminiMdFilename} starting from CWD: ${resolvedCwd}`, ); - } - const projectRoot = await findProjectRoot(resolvedCwd); - if (debugMode) - logger.debug(`Determined project root: ${projectRoot ?? 'None'}`); + const projectRoot = await findProjectRoot(resolvedCwd); + if (debugMode) + logger.debug(`Determined project root: ${projectRoot ?? 'None'}`); - const upwardPaths: string[] = []; - let currentDir = resolvedCwd; - // Determine the directory that signifies the top of the project or user-specific space. - const ultimateStopDir = projectRoot - ? path.dirname(projectRoot) - : path.dirname(resolvedHome); + const upwardPaths: string[] = []; + let currentDir = resolvedCwd; + const ultimateStopDir = projectRoot + ? path.dirname(projectRoot) + : path.dirname(resolvedHome); - while (currentDir && currentDir !== path.dirname(currentDir)) { - // Loop until filesystem root or currentDir is empty - if (debugMode) { - logger.debug( - `Checking for ${geminiMdFilename} in (upward scan): ${currentDir}`, - ); - } - - // Skip the global .gemini directory itself during upward scan from CWD, - // as global is handled separately and explicitly first. - if (currentDir === path.join(resolvedHome, GEMINI_CONFIG_DIR)) { - if (debugMode) { - logger.debug( - `Upward scan reached global config dir path, stopping upward search here: ${currentDir}`, - ); + while (currentDir && currentDir !== path.dirname(currentDir)) { + if (currentDir === path.join(resolvedHome, GEMINI_CONFIG_DIR)) { + break; } - break; - } - const potentialPath = path.join(currentDir, geminiMdFilename); - try { - await fs.access(potentialPath, fsSync.constants.R_OK); - // Add to upwardPaths only if it's not the already added globalMemoryPath - if (potentialPath !== globalMemoryPath) { - upwardPaths.unshift(potentialPath); - if (debugMode) { - logger.debug( - `Found readable upward ${geminiMdFilename}: ${potentialPath}`, - ); + const potentialPath = path.join(currentDir, geminiMdFilename); + try { + await fs.access(potentialPath, fsSync.constants.R_OK); + if (potentialPath !== globalMemoryPath) { + upwardPaths.unshift(potentialPath); } + } catch { + // Not found, continue. } - } catch { - if (debugMode) { - logger.debug( - `Upward ${geminiMdFilename} not found or not readable in: ${currentDir}`, - ); + + if (currentDir === ultimateStopDir) { + break; } + + currentDir = path.dirname(currentDir); } + upwardPaths.forEach((p) => allPaths.add(p)); - // Stop condition: if currentDir is the ultimateStopDir, break after this iteration. - if (currentDir === ultimateStopDir) { - if (debugMode) - logger.debug( - `Reached ultimate stop directory for upward scan: ${currentDir}`, - ); - break; + const mergedOptions = { + ...DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, + ...fileFilteringOptions, + }; + + const downwardPaths = await bfsFileSearch(resolvedCwd, { + fileName: geminiMdFilename, + maxDirs, + debug: debugMode, + fileService, + fileFilteringOptions: mergedOptions, + }); + downwardPaths.sort(); + for (const dPath of downwardPaths) { + allPaths.add(dPath); } - - currentDir = path.dirname(currentDir); - } - upwardPaths.forEach((p) => allPaths.add(p)); - - // Merge options with memory defaults, with options taking precedence - const mergedOptions = { - ...DEFAULT_MEMORY_FILE_FILTERING_OPTIONS, - ...fileFilteringOptions, - }; - - const downwardPaths = await bfsFileSearch(resolvedCwd, { - fileName: geminiMdFilename, - maxDirs, - debug: debugMode, - fileService, - fileFilteringOptions: mergedOptions, // Pass merged options as fileFilter - }); - downwardPaths.sort(); // Sort for consistent ordering, though hierarchy might be more complex - if (debugMode && downwardPaths.length > 0) - logger.debug( - `Found downward ${geminiMdFilename} files (sorted): ${JSON.stringify( - downwardPaths, - )}`, - ); - // Add downward paths only if they haven't been included already (e.g. from upward scan) - for (const dPath of downwardPaths) { - allPaths.add(dPath); } } - // Add extension context file paths + // Add extension context file paths. for (const extensionPath of extensionContextFilePaths) { allPaths.add(extensionPath); }