From b9cf1ea3ce513717416317fa21b87ce98a107ec6 Mon Sep 17 00:00:00 2001 From: HugoMurillo Date: Tue, 19 Aug 2025 13:07:42 -0600 Subject: [PATCH] fix(#5605): .env file loaded after settings are parsed (#6494) --- packages/cli/src/config/settings.test.ts | 162 +++++++---------------- packages/cli/src/config/settings.ts | 107 ++++++++------- 2 files changed, 109 insertions(+), 160 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index d537eeb1..65a556be 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -949,114 +949,37 @@ describe('Settings Loading and Merging', () => { delete process.env['WORKSPACE_ENDPOINT']; }); - it('should prioritize user env variables over workspace env variables if keys clash after resolution', () => { - const userSettingsContent = { configValue: '$SHARED_VAR' }; - const workspaceSettingsContent = { configValue: '$SHARED_VAR' }; + it('should correctly resolve and merge env variables from different scopes', () => { + process.env['SYSTEM_VAR'] = 'system_value'; + process.env['USER_VAR'] = 'user_value'; + process.env['WORKSPACE_VAR'] = 'workspace_value'; + process.env['SHARED_VAR'] = 'final_value'; + + const systemSettingsContent = { + configValue: '$SHARED_VAR', + systemOnly: '$SYSTEM_VAR', + }; + const userSettingsContent = { + configValue: '$SHARED_VAR', + userOnly: '$USER_VAR', + theme: 'dark', + }; + const workspaceSettingsContent = { + configValue: '$SHARED_VAR', + workspaceOnly: '$WORKSPACE_VAR', + theme: 'light', + }; (mockFsExistsSync as Mock).mockReturnValue(true); - const originalSharedVar = process.env['SHARED_VAR']; - // Temporarily delete to ensure a clean slate for the test's specific manipulations - delete process.env['SHARED_VAR']; - - (fs.readFileSync as Mock).mockImplementation( - (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) { - process.env['SHARED_VAR'] = 'user_value_for_user_read'; // Set for user settings read - return JSON.stringify(userSettingsContent); - } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { - process.env['SHARED_VAR'] = 'workspace_value_for_workspace_read'; // Set for workspace settings read - return JSON.stringify(workspaceSettingsContent); - } - return '{}'; - }, - ); - - const settings = loadSettings(MOCK_WORKSPACE_DIR); - - // @ts-expect-error: dynamic property for test - expect(settings.user.settings.configValue).toBe( - 'user_value_for_user_read', - ); - // @ts-expect-error: dynamic property for test - expect(settings.workspace.settings.configValue).toBe( - 'workspace_value_for_workspace_read', - ); - // Merged should take workspace's resolved value - // @ts-expect-error: dynamic property for test - expect(settings.merged.configValue).toBe( - 'workspace_value_for_workspace_read', - ); - - // Restore original environment variable state - if (originalSharedVar !== undefined) { - process.env['SHARED_VAR'] = originalSharedVar; - } else { - delete process.env['SHARED_VAR']; // Ensure it's deleted if it wasn't there before - } - }); - - it('should prioritize workspace env variables over user env variables if keys clash after resolution', () => { - const userSettingsContent = { configValue: '$SHARED_VAR' }; - const workspaceSettingsContent = { configValue: '$SHARED_VAR' }; - - (mockFsExistsSync as Mock).mockReturnValue(true); - const originalSharedVar = process.env['SHARED_VAR']; - // Temporarily delete to ensure a clean slate for the test's specific manipulations - delete process.env['SHARED_VAR']; - - (fs.readFileSync as Mock).mockImplementation( - (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) { - process.env['SHARED_VAR'] = 'user_value_for_user_read'; // Set for user settings read - return JSON.stringify(userSettingsContent); - } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { - process.env['SHARED_VAR'] = 'workspace_value_for_workspace_read'; // Set for workspace settings read - return JSON.stringify(workspaceSettingsContent); - } - return '{}'; - }, - ); - - const settings = loadSettings(MOCK_WORKSPACE_DIR); - - expect(settings.user.settings.configValue).toBe( - 'user_value_for_user_read', - ); - expect(settings.workspace.settings.configValue).toBe( - 'workspace_value_for_workspace_read', - ); - // Merged should take workspace's resolved value - expect(settings.merged.configValue).toBe( - 'workspace_value_for_workspace_read', - ); - - // Restore original environment variable state - if (originalSharedVar !== undefined) { - process.env['SHARED_VAR'] = originalSharedVar; - } else { - delete process.env['SHARED_VAR']; // Ensure it's deleted if it wasn't there before - } - }); - - it('should prioritize system env variables over workspace env variables if keys clash after resolution', () => { - const workspaceSettingsContent = { configValue: '$SHARED_VAR' }; - const systemSettingsContent = { configValue: '$SHARED_VAR' }; - - (mockFsExistsSync as Mock).mockReturnValue(true); - const originalSharedVar = process.env['SHARED_VAR']; - // Temporarily delete to ensure a clean slate for the test's specific manipulations - delete process.env['SHARED_VAR']; - (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { if (p === getSystemSettingsPath()) { - process.env['SHARED_VAR'] = 'system_value_for_system_read'; // Set for system settings read return JSON.stringify(systemSettingsContent); } + if (p === USER_SETTINGS_PATH) { + return JSON.stringify(userSettingsContent); + } if (p === MOCK_WORKSPACE_SETTINGS_PATH) { - process.env['SHARED_VAR'] = 'workspace_value_for_workspace_read'; // Set for workspace settings read return JSON.stringify(workspaceSettingsContent); } return '{}'; @@ -1065,24 +988,35 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); + // Check resolved values in individual scopes // @ts-expect-error: dynamic property for test - expect(settings.system.settings.configValue).toBe( - 'system_value_for_system_read', - ); + expect(settings.system.settings.configValue).toBe('final_value'); // @ts-expect-error: dynamic property for test - expect(settings.workspace.settings.configValue).toBe( - 'workspace_value_for_workspace_read', - ); - // Merged should take system's resolved value + expect(settings.system.settings.systemOnly).toBe('system_value'); // @ts-expect-error: dynamic property for test - expect(settings.merged.configValue).toBe('system_value_for_system_read'); + expect(settings.user.settings.configValue).toBe('final_value'); + // @ts-expect-error: dynamic property for test + expect(settings.user.settings.userOnly).toBe('user_value'); + // @ts-expect-error: dynamic property for test + expect(settings.workspace.settings.configValue).toBe('final_value'); + // @ts-expect-error: dynamic property for test + expect(settings.workspace.settings.workspaceOnly).toBe('workspace_value'); - // Restore original environment variable state - if (originalSharedVar !== undefined) { - process.env['SHARED_VAR'] = originalSharedVar; - } else { - delete process.env['SHARED_VAR']; // Ensure it's deleted if it wasn't there before - } + // Check merged values (system > workspace > user) + // @ts-expect-error: dynamic property for test + expect(settings.merged.configValue).toBe('final_value'); + // @ts-expect-error: dynamic property for test + expect(settings.merged.systemOnly).toBe('system_value'); + // @ts-expect-error: dynamic property for test + expect(settings.merged.userOnly).toBe('user_value'); + // @ts-expect-error: dynamic property for test + expect(settings.merged.workspaceOnly).toBe('workspace_value'); + expect(settings.merged.theme).toBe('light'); // workspace overrides user + + delete process.env['SYSTEM_VAR']; + delete process.env['USER_VAR']; + delete process.env['WORKSPACE_VAR']; + delete process.env['SHARED_VAR']; }); it('should correctly merge dnsResolutionOrder with workspace taking precedence', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 524eb16c..414caf11 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -70,6 +70,43 @@ export interface SettingsFile { settings: Settings; path: string; } + +function mergeSettings( + system: Settings, + user: Settings, + workspace: Settings, +): Settings { + // folderTrust is not supported at workspace level. + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { folderTrust, ...workspaceWithoutFolderTrust } = workspace; + + return { + ...user, + ...workspaceWithoutFolderTrust, + ...system, + customThemes: { + ...(user.customThemes || {}), + ...(workspace.customThemes || {}), + ...(system.customThemes || {}), + }, + mcpServers: { + ...(user.mcpServers || {}), + ...(workspace.mcpServers || {}), + ...(system.mcpServers || {}), + }, + includeDirectories: [ + ...(system.includeDirectories || []), + ...(user.includeDirectories || []), + ...(workspace.includeDirectories || []), + ], + chatCompression: { + ...(system.chatCompression || {}), + ...(user.chatCompression || {}), + ...(workspace.chatCompression || {}), + }, + }; +} + export class LoadedSettings { constructor( system: SettingsFile, @@ -96,39 +133,11 @@ export class LoadedSettings { } private computeMergedSettings(): Settings { - const system = this.system.settings; - const user = this.user.settings; - const workspace = this.workspace.settings; - - // folderTrust is not supported at workspace level. - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { folderTrust, ...workspaceWithoutFolderTrust } = workspace; - - return { - ...user, - ...workspaceWithoutFolderTrust, - ...system, - customThemes: { - ...(user.customThemes || {}), - ...(workspace.customThemes || {}), - ...(system.customThemes || {}), - }, - mcpServers: { - ...(user.mcpServers || {}), - ...(workspace.mcpServers || {}), - ...(system.mcpServers || {}), - }, - includeDirectories: [ - ...(system.includeDirectories || []), - ...(user.includeDirectories || []), - ...(workspace.includeDirectories || []), - ], - chatCompression: { - ...(system.chatCompression || {}), - ...(user.chatCompression || {}), - ...(workspace.chatCompression || {}), - }, - }; + return mergeSettings( + this.system.settings, + this.user.settings, + this.workspace.settings, + ); } forScope(scope: SettingScope): SettingsFile { @@ -339,10 +348,7 @@ export function loadSettings(workspaceDir: string): LoadedSettings { try { if (fs.existsSync(systemSettingsPath)) { const systemContent = fs.readFileSync(systemSettingsPath, 'utf-8'); - const parsedSystemSettings = JSON.parse( - stripJsonComments(systemContent), - ) as Settings; - systemSettings = resolveEnvVarsInObject(parsedSystemSettings); + systemSettings = JSON.parse(stripJsonComments(systemContent)) as Settings; } } catch (error: unknown) { settingsErrors.push({ @@ -355,10 +361,7 @@ export function loadSettings(workspaceDir: string): LoadedSettings { try { if (fs.existsSync(USER_SETTINGS_PATH)) { const userContent = fs.readFileSync(USER_SETTINGS_PATH, 'utf-8'); - const parsedUserSettings = JSON.parse( - stripJsonComments(userContent), - ) as Settings; - userSettings = resolveEnvVarsInObject(parsedUserSettings); + userSettings = JSON.parse(stripJsonComments(userContent)) as Settings; // Support legacy theme names if (userSettings.theme && userSettings.theme === 'VS') { userSettings.theme = DefaultLight.name; @@ -378,10 +381,9 @@ export function loadSettings(workspaceDir: string): LoadedSettings { try { if (fs.existsSync(workspaceSettingsPath)) { const projectContent = fs.readFileSync(workspaceSettingsPath, 'utf-8'); - const parsedWorkspaceSettings = JSON.parse( + workspaceSettings = JSON.parse( stripJsonComments(projectContent), ) as Settings; - workspaceSettings = resolveEnvVarsInObject(parsedWorkspaceSettings); if (workspaceSettings.theme && workspaceSettings.theme === 'VS') { workspaceSettings.theme = DefaultLight.name; } else if ( @@ -399,6 +401,22 @@ export function loadSettings(workspaceDir: string): LoadedSettings { } } + // Create a temporary merged settings object to pass to loadEnvironment. + const tempMergedSettings = mergeSettings( + systemSettings, + userSettings, + workspaceSettings, + ); + + // loadEnviroment depends on settings so we have to create a temp version of + // the settings to avoid a cycle + loadEnvironment(tempMergedSettings); + + // Now that the environment is loaded, resolve variables in the settings. + systemSettings = resolveEnvVarsInObject(systemSettings); + userSettings = resolveEnvVarsInObject(userSettings); + workspaceSettings = resolveEnvVarsInObject(workspaceSettings); + // Create LoadedSettings first const loadedSettings = new LoadedSettings( { @@ -429,9 +447,6 @@ export function loadSettings(workspaceDir: string): LoadedSettings { delete loadedSettings.merged.chatCompression; } - // Load environment with merged settings - loadEnvironment(loadedSettings.merged); - return loadedSettings; }