Merge branch 'main' into copilot/fix-environment-test-failure

edvilme · web-flow · commit d7df91e825b6 · 2026-06-09T16:34:21.000-07:00
diff --git a/src/features/common/activation.ts b/src/features/common/activation.ts
@@ -4,6 +4,7 @@ import {
     getShellActivationCommand,
     getShellCommandAsString,
     getShellDeactivationCommand,
+    wrapDeactivationCommand,
 } from '../terminal/shells/common/shellUtils';
 import { identifyTerminalShell } from './shellDetector';
 
@@ -28,7 +29,7 @@ export function getDeactivationCommand(terminal: Terminal, environment: PythonEn
     const shell = identifyTerminalShell(terminal);
     const command = getShellDeactivationCommand(shell, environment);
     if (command) {
-        return getShellCommandAsString(shell, command);
+        return wrapDeactivationCommand(shell, getShellCommandAsString(shell, command));
     }
     return undefined;
 }
diff --git a/src/features/interpreterSelection.ts b/src/features/interpreterSelection.ts
@@ -44,10 +44,10 @@ export interface PriorityChainResult {
 export interface SettingResolutionError {
     /** The setting that failed */
     setting: 'pythonProjects' | 'defaultEnvManager' | 'defaultInterpreterPath';
+    /** Structured failure kind, used for localization and stale-error re-checks. */
+    kind: 'managerNotRegistered' | 'pathUnresolvedVariables' | 'pathCannotResolve';
     /** The configured value */
     configuredValue: string;
-    /** Reason for failure */
-    reason: string;
 }
 
 /**
@@ -81,8 +81,8 @@ async function resolvePriorityChainCore(
             }
             const error: SettingResolutionError = {
                 setting: 'pythonProjects',
+                kind: 'managerNotRegistered',
                 configuredValue: projectManagerId,
-                reason: `Environment manager '${projectManagerId}' is not registered`,
             };
             errors.push(error);
             traceWarn(`${logPrefix} pythonProjects[] manager '${projectManagerId}' not found, trying next priority`);
@@ -99,8 +99,8 @@ async function resolvePriorityChainCore(
         }
         const error: SettingResolutionError = {
             setting: 'defaultEnvManager',
+            kind: 'managerNotRegistered',
             configuredValue: userConfiguredManager,
-            reason: `Environment manager '${userConfiguredManager}' is not registered`,
         };
         errors.push(error);
         traceWarn(`${logPrefix} defaultEnvManager '${userConfiguredManager}' not found, trying next priority`);
@@ -118,8 +118,8 @@ async function resolvePriorityChainCore(
                 );
                 const error: SettingResolutionError = {
                     setting: 'defaultInterpreterPath',
+                    kind: 'pathUnresolvedVariables',
                     configuredValue: userInterpreterPath,
-                    reason: l10n.t('Path contains unresolved variables'),
                 };
                 errors.push(error);
             } else {
@@ -138,8 +138,8 @@ async function resolvePriorityChainCore(
                 );
                 const error: SettingResolutionError = {
                     setting: 'defaultInterpreterPath',
+                    kind: 'pathUnresolvedVariables',
                     configuredValue: userInterpreterPath,
-                    reason: l10n.t('Path contains unresolved variables'),
                 };
                 errors.push(error);
             } else {
@@ -155,8 +155,8 @@ async function resolvePriorityChainCore(
                 }
                 const error: SettingResolutionError = {
                     setting: 'defaultInterpreterPath',
+                    kind: 'pathCannotResolve',
                     configuredValue: userInterpreterPath,
-                    reason: `Could not resolve interpreter path '${userInterpreterPath}'`,
                 };
                 errors.push(error);
                 traceWarn(
@@ -383,8 +383,9 @@ export async function applyInitialEnvironmentSelection(
         }
         resolveGlobalScope()
             .then(async (globalErrors) => {
-                if (globalErrors.length > 0) {
-                    await notifyUserOfSettingErrors(globalErrors);
+                const liveGlobalErrors = filterLiveSettingErrors(globalErrors, envManagers);
+                if (liveGlobalErrors.length > 0) {
+                    await notifyUserOfSettingErrors(liveGlobalErrors);
                 }
             })
             .catch((err) => traceError(`[interpreterSelection] Background global scope resolution failed: ${err}`));
@@ -397,9 +398,11 @@ export async function applyInitialEnvironmentSelection(
         allErrors.push(...globalErrors);
     }
 
-    // Notify user if any settings could not be applied (workspace + global when awaited)
-    if (allErrors.length > 0) {
-        await notifyUserOfSettingErrors(allErrors);
+    // Drop errors that became stale during the awaits above; see filterLiveSettingErrors.
+    const liveErrors = filterLiveSettingErrors(allErrors, envManagers);
+
+    if (liveErrors.length > 0) {
+        await notifyUserOfSettingErrors(liveErrors);
     }
 
     // Numeric values must go via the measures argument (properties are dropped).
@@ -409,7 +412,7 @@ export async function applyInitialEnvironmentSelection(
             duration: selectionStopWatch.elapsedTime,
             workspaceFolderCount: folders.length,
             resolvedFolderCount,
-            settingErrorCount: allErrors.length,
+            settingErrorCount: liveErrors.length,
         },
         {
             globalScopeDeferred: workspaceFolderResolved,
@@ -430,7 +433,42 @@ export function resetSettingWarnings(): void {
     warnedSettings.clear();
 }
 
+function localizedReasonFor(error: SettingResolutionError): string {
+    switch (error.kind) {
+        case 'managerNotRegistered':
+            return l10n.t("Environment manager '{0}' is not registered", error.configuredValue);
+        case 'pathUnresolvedVariables':
+            return l10n.t('Path contains unresolved variables');
+        case 'pathCannotResolve':
+            return l10n.t("Could not resolve interpreter path '{0}'", error.configuredValue);
+    }
+}
+
+/**
+ * Re-check the live registry to drop `managerNotRegistered` errors that became
+ * stale while later awaits ran — a third-party manager extension may have
+ * registered in the meantime, in which case the warning would be a false positive.
+ */
+function filterLiveSettingErrors(
+    errors: SettingResolutionError[],
+    envManagers: EnvironmentManagers,
+): SettingResolutionError[] {
+    return errors.filter((e) => {
+        if (e.kind === 'managerNotRegistered' && envManagers.getEnvironmentManager(e.configuredValue)) {
+            traceVerbose(
+                `[interpreterSelection] Manager '${e.configuredValue}' is now registered; suppressing stale warning`,
+            );
+            return false;
+        }
+        return true;
+    });
+}
+
 async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Promise<void> {
+    if (errors.length === 0) {
+        return;
+    }
+
     // Group errors by setting type to avoid spamming the user
     const uniqueSettings = [...new Set(errors.map((e) => e.setting))];
 
@@ -442,6 +480,7 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom
 
         const settingErrors = errors.filter((e) => e.setting === setting);
         const firstError = settingErrors[0];
+        const reason = localizedReasonFor(firstError);
 
         let message: string;
         let settingKey: string;
@@ -451,23 +490,23 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom
                 message = l10n.t(
                     "Python project setting for environment manager '{0}' could not be applied: {1}",
                     firstError.configuredValue,
-                    firstError.reason,
+                    reason,
                 );
                 settingKey = 'python-envs.pythonProjects';
                 break;
             case 'defaultEnvManager':
                 message = l10n.t(
                     "Default environment manager '{0}' could not be applied: {1}",
                     firstError.configuredValue,
-                    firstError.reason,
+                    reason,
                 );
                 settingKey = 'python-envs.defaultEnvManager';
                 break;
             case 'defaultInterpreterPath':
                 message = l10n.t(
                     "Default interpreter path '{0}' could not be resolved: {1}",
                     firstError.configuredValue,
-                    firstError.reason,
+                    reason,
                 );
                 settingKey = 'python.defaultInterpreterPath';
                 break;
diff --git a/src/features/terminal/shells/common/shellUtils.ts b/src/features/terminal/shells/common/shellUtils.ts
@@ -50,6 +50,68 @@ export function getShellCommandAsString(shell: string, command: PythonCommandRun
     return commandStr;
 }
 
+// Shells whose bare `deactivate` command is a shell function/alias defined by venv's
+// `activate` script. These can disappear (e.g. on session restore where VIRTUAL_ENV
+// persists but the shell function does not), so we must guard the call with an
+// existence check before sending it to the terminal — otherwise the shell prints
+// `deactivate: command not found`.
+const bareDeactivateGuardByShell = new Map<string, (cmd: string) => string>([
+    // POSIX-family shells: `command -v <name>` is the portable existence check.
+    [ShellConstants.BASH, (cmd) => `command -v deactivate >/dev/null 2>&1 && ${cmd.trimStart()}`],
+    [ShellConstants.SH, (cmd) => `command -v deactivate >/dev/null 2>&1 && ${cmd.trimStart()}`],
+    [ShellConstants.ZSH, (cmd) => `command -v deactivate >/dev/null 2>&1 && ${cmd.trimStart()}`],
+    [ShellConstants.KSH, (cmd) => `command -v deactivate >/dev/null 2>&1 && ${cmd.trimStart()}`],
+    [ShellConstants.GITBASH, (cmd) => `command -v deactivate >/dev/null 2>&1 && ${cmd.trimStart()}`],
+    // fish uses `functions -q` for function existence.
+    [ShellConstants.FISH, (cmd) => `functions -q deactivate; and ${cmd.trimStart()}`],
+    // PowerShell: Get-Command returns silently if not found with -ErrorAction SilentlyContinue.
+    [ShellConstants.PWSH, (cmd) => `if (Get-Command deactivate -ErrorAction SilentlyContinue) { ${cmd.trimStart()} }`],
+]);
+
+/**
+ * Returns the bare `deactivate` token if and only if `command` represents a single,
+ * bare invocation of a shell function/alias literally named `deactivate` — meaning
+ * it is safe and meaningful to gate on the existence of that function in the shell.
+ *
+ * Returns `undefined` for anything else (full paths like `path/to/deactivate.bat`,
+ * multi-token forms like `conda deactivate`, `pyenv shell --unset`, `overlay hide ...`,
+ * etc.), since those have different failure modes that should not be silently swallowed.
+ *
+ * The token is normalized to lowercase so the generated guard is consistent across
+ * shells (notably PowerShell, which is case-insensitive).
+ */
+function bareDeactivateInvocation(command: string): string | undefined {
+    const trimmed = command.trim();
+    return trimmed.toLowerCase() === 'deactivate' ? 'deactivate' : undefined;
+}
+
+/**
+ * Wraps a deactivation command in a shell-specific existence guard so that sending
+ * it to a terminal where the `deactivate` shell function no longer exists does not
+ * print `deactivate: command not found`.
+ *
+ * Only applies when the command is a single bare `deactivate` token and the shell
+ * has a known guard template. All other deactivation forms (cmd's `deactivate.bat`
+ * path, `conda deactivate`, `pyenv shell --unset`, nu's `overlay hide ...`, etc.)
+ * are returned unchanged — their failure modes are legitimate and should surface.
+ */
+export function wrapDeactivationCommand(shell: string, command: string): string {
+    const bare = bareDeactivateInvocation(command);
+    if (!bare) {
+        return command;
+    }
+    const guard = bareDeactivateGuardByShell.get(shell);
+    if (!guard) {
+        return command;
+    }
+    const wrapped = guard(bare);
+    // Preserve the leading-space history-ignore behavior for shells that honor it.
+    if (shellsWithLeadingSpaceHistorySupport.has(shell)) {
+        return ` ${wrapped}`;
+    }
+    return wrapped;
+}
+
 export function normalizeShellPath(filePath: string, shellType?: string): string {
     if (isWindows() && shellType) {
         if (shellType.toLowerCase() === ShellConstants.GITBASH || shellType.toLowerCase() === 'git-bash') {
diff --git a/src/test/features/interpreterSelection.unit.test.ts b/src/test/features/interpreterSelection.unit.test.ts
@@ -968,6 +968,97 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
         const warningMessage = showWarnStub.firstCall.args[0] as string;
         assert.ok(warningMessage.includes('/nonexistent/python'), 'Warning message should include the configured path');
     });
+
+    test('should suppress stale warning when manager registers during await (issue #1517)', async () => {
+        // Simulate the race: defaultEnvManager is set to 'pypa.hatch:hatch', which is not
+        // registered at priority-chain evaluation time, but becomes registered before
+        // notifyUserOfSettingErrors runs. The warning must NOT be shown — neither from the
+        // awaited workspace path nor from the deferred global `.then(...)` path.
+        const hatchManagerId = 'pypa.hatch:hatch';
+
+        sandbox.stub(workspaceApis, 'getWorkspaceFolders').returns([{ uri: testUri, name: 'test', index: 0 }]);
+        sandbox.stub(workspaceApis, 'getConfiguration').returns(createMockConfig([]) as WorkspaceConfiguration);
+        sandbox.stub(helpers, 'getUserConfiguredSetting').callsFake((section: string, key: string) => {
+            if (section === 'python-envs' && key === 'defaultEnvManager') {
+                return hatchManagerId;
+            }
+            return undefined;
+        });
+
+        // Hatch is not registered when the priority chain runs (returns undefined),
+        // but by the time notifyUserOfSettingErrors is called, it is available.
+        const mockHatchManager = {
+            id: hatchManagerId,
+            name: 'hatch',
+            displayName: 'Hatch',
+            get: sandbox.stub().resolves(undefined),
+            set: sandbox.stub().resolves(),
+        } as unknown as sinon.SinonStubbedInstance<InternalEnvironmentManager>;
+
+        let hatchRegistered = false;
+        // Override getEnvironmentManager: returns undefined until hatchRegistered is set, then returns the manager
+        mockEnvManagers.getEnvironmentManager.callsFake((scope: unknown) => {
+            const id = typeof scope === 'string' ? scope : undefined;
+            if (id === hatchManagerId && hatchRegistered) {
+                return mockHatchManager;
+            }
+            if (id === 'ms-python.python:venv') {
+                return mockVenvManager;
+            }
+            if (id === 'ms-python.python:system') {
+                return mockSystemManager;
+            }
+            return undefined;
+        });
+
+        // Simulate Hatch registering during the venv discovery await
+        mockVenvManager.get.callsFake(async () => {
+            hatchRegistered = true; // Hatch "registers" while venv discovery is awaited
+            return mockVenvEnv;
+        });
+
+        // The workspace folder will resolve (venv found), so global scope is deferred to
+        // a background `.then(...)`. Use a deferred promise to deterministically wait for
+        // that background work to complete before asserting on showWarningMessage —
+        // otherwise a regression in the deferred-path filtering would fire AFTER the test
+        // returned and silently slip through.
+        let resolveGlobalDone!: () => void;
+        const globalDone = new Promise<void>((resolve) => {
+            resolveGlobalDone = resolve;
+        });
+        mockEnvManagers.setEnvironments.callsFake(async () => {
+            resolveGlobalDone();
+        });
+
+        const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);
+
+        await applyInitialEnvironmentSelection(
+            mockEnvManagers as unknown as EnvironmentManagers,
+            mockProjectManager as unknown as PythonProjectManager,
+            mockNativeFinder as unknown as NativePythonFinder,
+            mockApi as unknown as PythonEnvironmentApi,
+        );
+
+        // Wait for the background global scope to call setEnvironments, then flush
+        // microtasks so the `.then(...)` handler that would call notifyUserOfSettingErrors
+        // has a chance to run before we assert.
+        await globalDone;
+        await new Promise<void>((resolve) => process.nextTick(resolve));
+
+        // Workspace folder should have resolved (venv found) and global scope was attempted
+        assert.ok(mockEnvManagers.setEnvironment.called, 'setEnvironment should be called for workspace folder');
+        assert.ok(
+            mockEnvManagers.setEnvironments.called,
+            'setEnvironments should be called for deferred global scope',
+        );
+
+        // Warning must NOT be shown because the error was stale on both paths
+        assert.strictEqual(
+            showWarnStub.called,
+            false,
+            'showWarningMessage must NOT be called when manager registered before notification',
+        );
+    });
 });
 
 suite('Interpreter Selection - resolveGlobalEnvironmentByPriority', () => {
diff --git a/src/test/features/terminal/shells/common/shellUtils.unit.test.ts b/src/test/features/terminal/shells/common/shellUtils.unit.test.ts
