Fix env variable caching and add testing arch for environment variable functionality

Author: eleanorjboyd

.github/instructions/testing-workflow.instructions.md

@@ -574,3 +574,10 @@ envConfig.inspect
 -   Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable)
 
 ## 🧠 Agent Learnings
+
+-   VS Code file watchers only monitor workspace folders, not external temp directories (1)
+-   Use fixture-based testing with real files instead of mocking fs-extra, which has non-configurable property descriptors that prevent stubbing (1)
+-   Extension tests (.test.ts) should use real filesystem operations; unit tests (.unit.test.ts) should mock dependencies (1)
+-   Use `as unknown as TargetType` for type casting instead of `as any` to maintain type safety and avoid 'any' violations
+-   If tests frequently need private access consider that maybe methods should be protected, or public test utilities should exist for testing (1)
+-   When making systematic changes across many similar locations, fix one instance completely first to validate the approach before applying the pattern everywhere (1)

src/features/terminal/terminalEnvVarInjector.ts

@@ -5,8 +5,10 @@ import * as fse from 'fs-extra';
 import * as path from 'path';
 import {
     Disposable,
+    EnvironmentVariableCollection,
     EnvironmentVariableScope,
     GlobalEnvironmentVariableCollection,
+    Uri,
     window,
     workspace,
     WorkspaceFolder,
@@ -23,6 +25,13 @@ import { EnvVarManager } from '../execution/envVariableManager';
 export class TerminalEnvVarInjector implements Disposable {
     private disposables: Disposable[] = [];
 
+    /**
+     * Track which environment variables we've set per workspace to properly handle
+     * variables that are removed/commented out in .env files.
+     * Key: workspace fsPath, Value: Set of env var keys we've set for that workspace
+     */
+    private readonly trackedEnvVars: Map<string, Set<string>> = new Map();
+
     constructor(
         private readonly envVarCollection: GlobalEnvironmentVariableCollection,
         private readonly envVarManager: EnvVarManager,
@@ -134,48 +143,22 @@ export class TerminalEnvVarInjector implements Disposable {
      */
     private async injectEnvironmentVariablesForWorkspace(workspaceFolder: WorkspaceFolder): Promise<void> {
         const workspaceUri = workspaceFolder.uri;
-        try {
-            const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri);
+        const workspaceKey = workspaceUri.fsPath;
 
-            // use scoped environment variable collection
+        try {
             const envVarScope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder });
 
-            // Check if env file injection is enabled
-            const config = getConfiguration('python', workspaceUri);
-            const useEnvFile = config.get<boolean>('terminal.useEnvFile', false);
-            const envFilePath = config.get<string>('envFile');
-
-            if (!useEnvFile) {
-                traceVerbose(
-                    `TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`,
-                );
+            // Check if we should inject and get the env file path
+            const shouldInject = await this.shouldInjectEnvVars(workspaceUri, envVarScope, workspaceKey);
+            if (!shouldInject) {
                 return;
             }
 
-            // Track which .env file is being used for logging
-            const resolvedEnvFilePath: string | undefined = envFilePath
-                ? path.resolve(resolveVariables(envFilePath, workspaceUri))
-                : undefined;
-            const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env');
-
-            let activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath;
-            if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) {
-                traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`);
-            } else {
-                traceVerbose(
-                    `TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`,
-                );
-                return; // No .env file to inject
-            }
+            // Get environment variables from the .env file
+            const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri);
 
-            for (const [key, value] of Object.entries(envVars)) {
-                if (value === undefined) {
-                    // Remove the environment variable if the value is undefined
-                    envVarScope.delete(key);
-                } else {
-                    envVarScope.replace(key, value);
-                }
-            }
+            // Apply the environment variable changes
+            this.applyEnvVarChanges(envVarScope, envVars, workspaceKey);
         } catch (error) {
             traceError(
                 `TerminalEnvVarInjector: Error injecting environment variables for workspace ${workspaceUri.fsPath}:`,
@@ -184,6 +167,91 @@ export class TerminalEnvVarInjector implements Disposable {
         }
     }
 
+    /**
+     * Check if environment variables should be injected for the workspace.
+     * Returns true if injection should proceed, false otherwise.
+     */
+    private async shouldInjectEnvVars(
+        workspaceUri: Uri,
+        envVarScope: EnvironmentVariableCollection,
+        workspaceKey: string,
+    ): Promise<boolean> {
+        const config = getConfiguration('python', workspaceUri);
+        const useEnvFile = config.get<boolean>('terminal.useEnvFile', false);
+        const envFilePath = config.get<string>('envFile');
+
+        if (!useEnvFile) {
+            traceVerbose(`TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`);
+            this.cleanupTrackedVars(envVarScope, workspaceKey);
+            return false;
+        }
+
+        // Determine which .env file to use
+        const resolvedEnvFilePath: string | undefined = envFilePath
+            ? path.resolve(resolveVariables(envFilePath, workspaceUri))
+            : undefined;
+        const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env');
+        const activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath;
+
+        if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) {
+            traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`);
+            return true;
+        } else {
+            traceVerbose(
+                `TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`,
+            );
+            this.cleanupTrackedVars(envVarScope, workspaceKey);
+            return false;
+        }
+    }
+
+    /**
+     * Apply environment variable changes by comparing current and previous state.
+     */
+    private applyEnvVarChanges(
+        envVarScope: EnvironmentVariableCollection,
+        envVars: { [key: string]: string | undefined },
+        workspaceKey: string,
+    ): void {
+        const previousKeys = this.trackedEnvVars.get(workspaceKey) || new Set<string>();
+        const currentKeys = new Set<string>();
+
+        // Update/add current env vars from .env file
+        for (const [key, value] of Object.entries(envVars)) {
+            if (value === undefined || value === '') {
+                // Variable explicitly unset in .env (e.g., VAR=)
+                envVarScope.delete(key);
+            } else {
+                envVarScope.replace(key, value);
+                currentKeys.add(key);
+            }
+        }
+
+        // Delete keys that were previously set but are now gone from .env
+        for (const oldKey of previousKeys) {
+            if (!currentKeys.has(oldKey)) {
+                traceVerbose(
+                    `TerminalEnvVarInjector: Removing previously set env var '${oldKey}' from workspace ${workspaceKey}`,
+                );
+                envVarScope.delete(oldKey);
+            }
+        }
+
+        // Update our tracking for this workspace
+        this.trackedEnvVars.set(workspaceKey, currentKeys);
+    }
+
+    /**
+     * Clean up previously tracked environment variables for a workspace.
+     */
+    private cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void {
+        const previousKeys = this.trackedEnvVars.get(workspaceKey);
+        if (previousKeys) {
+            previousKeys.forEach((key) => envVarScope.delete(key));
+            this.trackedEnvVars.delete(workspaceKey);
+        }
+    }
+
     /**
      * Dispose of the injector and clean up resources.
      */
@@ -192,8 +260,20 @@ export class TerminalEnvVarInjector implements Disposable {
         this.disposables.forEach((disposable) => disposable.dispose());
         this.disposables = [];
 
-        // Clear all environment variables from the collection
-        this.envVarCollection.clear();
+        // Clear only the environment variables that we've set, preserving others (e.g., BASH_ENV)
+        for (const [workspaceKey, trackedKeys] of this.trackedEnvVars.entries()) {
+            try {
+                // Try to find the workspace folder for proper scoping
+                const workspaceFolder = workspace.workspaceFolders?.find((wf) => wf.uri.fsPath === workspaceKey);
+                if (workspaceFolder) {
+                    const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder });
+                    trackedKeys.forEach((key) => scope.delete(key));
+                }
+            } catch (error) {
+                traceError(`Failed to clean up environment variables for workspace ${workspaceKey}:`, error);
+            }
+        }
+        this.trackedEnvVars.clear();
     }
 
     private getEnvironmentVariableCollectionScoped(scope: EnvironmentVariableScope = {}) {
@@ -205,9 +285,16 @@ export class TerminalEnvVarInjector implements Disposable {
      * Clear all environment variables for a workspace.
      */
     private clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void {
+        const workspaceKey = workspaceFolder.uri.fsPath;
         try {
             const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder });
-            scope.clear();
+
+            // Only delete env vars that we've set, not ones set by other managers (e.g., BASH_ENV)
+            const trackedKeys = this.trackedEnvVars.get(workspaceKey);
+            if (trackedKeys) {
+                trackedKeys.forEach((key) => scope.delete(key));
+                this.trackedEnvVars.delete(workspaceKey);
+            }
         } catch (error) {
             traceError(`Failed to clear environment variables for workspace ${workspaceFolder.uri.fsPath}:`, error);
         }