fix(#5605): .env file loaded after settings are parsed (#6494)

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', () => {
+    it('should correctly resolve and merge env variables from different scopes', () => {
-      const userSettingsContent = { configValue: '$SHARED_VAR' };
+      process.env['SYSTEM_VAR'] = 'system_value';
-      const workspaceSettingsContent = { configValue: '$SHARED_VAR' };
+      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(
+      expect(settings.system.settings.configValue).toBe('final_value');
-        'system_value_for_system_read',
-      );
       // @ts-expect-error: dynamic property for test
-      expect(settings.workspace.settings.configValue).toBe(
+      expect(settings.system.settings.systemOnly).toBe('system_value');
-        'workspace_value_for_workspace_read',
-      );
-      // Merged should take system's resolved 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
+      // Check merged values (system > workspace > user)
-      if (originalSharedVar !== undefined) {
+      // @ts-expect-error: dynamic property for test
-        process.env['SHARED_VAR'] = originalSharedVar;
+      expect(settings.merged.configValue).toBe('final_value');
-      } else {
+      // @ts-expect-error: dynamic property for test
-        delete process.env['SHARED_VAR']; // Ensure it's deleted if it wasn't there before
+      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', () => {

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;
+    return mergeSettings(
-    const user = this.user.settings;
+      this.system.settings,
-    const workspace = this.workspace.settings;
+      this.user.settings,
-
+      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 || {}),
-      },
-    };
   }
 
   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(
+      systemSettings = JSON.parse(stripJsonComments(systemContent)) as Settings;
-        stripJsonComments(systemContent),
-      ) as Settings;
-      systemSettings = resolveEnvVarsInObject(parsedSystemSettings);
     }
   } 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(
+      userSettings = JSON.parse(stripJsonComments(userContent)) as Settings;
-        stripJsonComments(userContent),
-      ) as Settings;
-      userSettings = resolveEnvVarsInObject(parsedUserSettings);
       // 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;
 }