From 6c54972604682699950d3f884f902c9dc0632de0 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:46:57 -0700 Subject: [PATCH 1/4] Prevent writing settings to global scope for environment and package managers --- docs/managing-python-projects.md | 2 + src/features/settings/settingHelpers.ts | 30 ------- .../settings/settingHelpers.unit.test.ts | 84 ++----------------- 3 files changed, 10 insertions(+), 106 deletions(-) diff --git a/docs/managing-python-projects.md b/docs/managing-python-projects.md index 645b3a1a..000715c2 100644 --- a/docs/managing-python-projects.md +++ b/docs/managing-python-projects.md @@ -211,6 +211,8 @@ You can set default managers that apply to all projects without explicit overrid } ``` +> **Important**: The extension never writes settings to the User (global) scope. All extension-managed settings are written at the Workspace or Workspace Folder level only. This prevents the extension from setting values that persist across unrelated projects and cause unexpected interference (see [#1468](https://github.com/microsoft/vscode-python-environments/issues/1468)). If a user want a user-level default, they can set it manually in their User `settings.json`. + ## Working with Multi-Root Workspaces Multi-root workspaces contain multiple top-level folders. The extension handles these seamlessly: diff --git a/src/features/settings/settingHelpers.ts b/src/features/settings/settingHelpers.ts index 4ef9f9ab..84e72944 100644 --- a/src/features/settings/settingHelpers.ts +++ b/src/features/settings/settingHelpers.ts @@ -195,18 +195,6 @@ export async function setAllManagerSettings(edits: EditAllManagerSettings[]): Pr } }); - const config = workspaceApis.getConfiguration('python-envs', undefined); - edits - .filter((e) => !e.project) - .forEach((e) => { - if (config.get('defaultEnvManager') !== e.envManager) { - promises.push(config.update('defaultEnvManager', e.envManager, ConfigurationTarget.Global)); - } - if (config.get('defaultPackageManager') !== e.packageManager) { - promises.push(config.update('defaultPackageManager', e.packageManager, ConfigurationTarget.Global)); - } - }); - await Promise.all(promises); } @@ -269,15 +257,6 @@ export async function setEnvironmentManager(edits: EditEnvManagerSettings[]): Pr } }); - const config = workspaceApis.getConfiguration('python-envs', undefined); - edits - .filter((e) => !e.project) - .forEach((e) => { - if (config.get('defaultEnvManager') !== e.envManager) { - promises.push(config.update('defaultEnvManager', e.envManager, ConfigurationTarget.Global)); - } - }); - await Promise.all(promises); } @@ -343,15 +322,6 @@ export async function setPackageManager(edits: EditPackageManagerSettings[]): Pr } }); - const config = workspaceApis.getConfiguration('python-envs', undefined); - edits - .filter((e) => !e.project) - .forEach((e) => { - if (config.get('defaultPackageManager') !== e.packageManager) { - promises.push(config.update('defaultPackageManager', e.packageManager, ConfigurationTarget.Global)); - } - }); - await Promise.all(promises); } diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index 1cb3a0c1..20b1f317 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -108,7 +108,7 @@ suite('Setting Helpers - Settings Write Behavior', () => { } suite('setAllManagerSettings - Global Settings', () => { - test('should write global defaultEnvManager when value differs from current', async () => { + test('should NOT write global defaultEnvManager even when value differs from current', async () => { const mockConfig = createMockConfig({ currentEnvManager: VENV_MANAGER_ID, }); @@ -125,31 +125,10 @@ suite('Setting Helpers - Settings Write Behavior', () => { const envManagerUpdates = updateCalls.filter( (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, ); - assert.strictEqual(envManagerUpdates.length, 1, 'Should write global defaultEnvManager when value differs'); - assert.strictEqual(envManagerUpdates[0].value, SYSTEM_MANAGER_ID); + assert.strictEqual(envManagerUpdates.length, 0, 'Should never write to user/global settings'); }); - test('should NOT write global defaultEnvManager when value is same as current', async () => { - const mockConfig = createMockConfig({ - currentEnvManager: SYSTEM_MANAGER_ID, - }); - sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig); - - await setAllManagerSettings([ - { - project: undefined, - envManager: SYSTEM_MANAGER_ID, - packageManager: PIP_MANAGER_ID, - }, - ]); - - const envManagerUpdates = updateCalls.filter( - (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, - ); - assert.strictEqual(envManagerUpdates.length, 0, 'Should NOT write when value is same as current'); - }); - - test('should write global defaultPackageManager when value differs from current', async () => { + test('should NOT write global defaultPackageManager even when value differs from current', async () => { const mockConfig = createMockConfig({ currentEnvManager: VENV_MANAGER_ID, currentPkgManager: PIP_MANAGER_ID, @@ -167,17 +146,12 @@ suite('Setting Helpers - Settings Write Behavior', () => { const pkgManagerUpdates = updateCalls.filter( (c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global, ); - assert.strictEqual( - pkgManagerUpdates.length, - 1, - 'Should write global defaultPackageManager when value differs', - ); - assert.strictEqual(pkgManagerUpdates[0].value, CONDA_MANAGER_ID); + assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings'); }); }); suite('setEnvironmentManager - Global Settings', () => { - test('should write when value differs from current', async () => { + test('should NOT write to global even when value differs from current', async () => { const mockConfig = createMockConfig({ currentEnvManager: VENV_MANAGER_ID, }); @@ -193,31 +167,12 @@ suite('Setting Helpers - Settings Write Behavior', () => { const envManagerUpdates = updateCalls.filter( (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, ); - assert.strictEqual(envManagerUpdates.length, 1, 'Should write global defaultEnvManager when value differs'); - }); - - test('should NOT write when value is same as current', async () => { - const mockConfig = createMockConfig({ - currentEnvManager: SYSTEM_MANAGER_ID, - }); - sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig); - - await setEnvironmentManager([ - { - project: undefined, - envManager: SYSTEM_MANAGER_ID, - }, - ]); - - const envManagerUpdates = updateCalls.filter( - (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, - ); - assert.strictEqual(envManagerUpdates.length, 0, 'Should NOT write when value is same'); + assert.strictEqual(envManagerUpdates.length, 0, 'Should never write to user/global settings'); }); }); suite('setPackageManager - Global Settings', () => { - test('should write when value differs from current', async () => { + test('should NOT write to global even when value differs from current', async () => { const mockConfig = createMockConfig({ currentPkgManager: PIP_MANAGER_ID, }); @@ -233,30 +188,7 @@ suite('Setting Helpers - Settings Write Behavior', () => { const pkgManagerUpdates = updateCalls.filter( (c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global, ); - assert.strictEqual( - pkgManagerUpdates.length, - 1, - 'Should write global defaultPackageManager when value differs', - ); - }); - - test('should NOT write when value is same as current', async () => { - const mockConfig = createMockConfig({ - currentPkgManager: PIP_MANAGER_ID, - }); - sinon.stub(workspaceApis, 'getConfiguration').returns(mockConfig); - - await setPackageManager([ - { - project: undefined, - packageManager: PIP_MANAGER_ID, - }, - ]); - - const pkgManagerUpdates = updateCalls.filter( - (c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global, - ); - assert.strictEqual(pkgManagerUpdates.length, 0, 'Should NOT write when value is same'); + assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings'); }); }); }); From 069f60d663ab58f425c774a6d4c11e323929016d Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:59:17 -0700 Subject: [PATCH 2/4] Implement one-time migration to remove system defaultEnvManager from global settings --- src/extension.ts | 4 + src/features/settings/settingHelpers.ts | 34 ++++- .../settings/settingHelpers.unit.test.ts | 119 ++++++++++++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) diff --git a/src/extension.ts b/src/extension.ts index 04bcb3e8..6f72b70c 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -71,6 +71,7 @@ import { import { PythonProjectManagerImpl } from './features/projectManager'; import { getPythonApi, setPythonApi } from './features/pythonApi'; import { registerCompletionProvider } from './features/settings/settingCompletions'; +import { migrateGlobalDefaultEnvManagerSetting } from './features/settings/settingHelpers'; import { setActivateMenuButtonContext } from './features/terminal/activateMenuButton'; import { normalizeShellPath } from './features/terminal/shells/common/shellUtils'; import { @@ -157,6 +158,9 @@ export async function activate(context: ExtensionContext): Promise(section: string, key: string): T | undefi } return undefined; } + +const MIGRATION_KEY = 'globalSettingsMigration.systemEnvManagerRemoved'; + +/** + * One-time migration: removes `defaultEnvManager` from User (global) settings if it was + * set to `system` by the extension. This was an unintentional side effect of a bug where + * the extension wrote to User scope when no workspace was open. Having `system` at the + * User level causes all workspaces to ignore local .venv environments. + * + * See: https://github.com/microsoft/vscode-python-environments/issues/1468 + */ +export async function migrateGlobalDefaultEnvManagerSetting(): Promise { + const state = await getGlobalPersistentState(); + const alreadyMigrated = await state.get(MIGRATION_KEY); + if (alreadyMigrated) { + return; + } + + const config = workspaceApis.getConfiguration('python-envs', undefined); + const inspect = config.inspect('defaultEnvManager'); + + if (inspect?.globalValue === SYSTEM_MANAGER_ID) { + await config.update('defaultEnvManager', undefined, ConfigurationTarget.Global); + traceInfo( + `[migration] Removed 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings ` + + `(was set unintentionally by a previous version). See https://github.com/microsoft/vscode-python-environments/issues/1468`, + ); + } + + await state.set(MIGRATION_KEY, true); +} diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index 20b1f317..27fa96d8 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -3,9 +3,11 @@ import * as assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; import { ConfigurationTarget, Uri, WorkspaceFolder } from 'vscode'; +import * as persistentState from '../../../common/persistentState'; import * as workspaceApis from '../../../common/workspace.apis'; import { addPythonProjectSetting, + migrateGlobalDefaultEnvManagerSetting, setAllManagerSettings, setEnvironmentManager, setPackageManager, @@ -604,3 +606,120 @@ suite('Setting Helpers - Empty Path Migration', () => { }); }); }); + +suite('Setting Helpers - migrateGlobalDefaultEnvManagerSetting', () => { + const SYSTEM_MANAGER_ID = 'ms-python.python:system'; + let sandbox: sinon.SinonSandbox; + + setup(() => { + sandbox = sinon.createSandbox(); + }); + + teardown(() => { + sandbox.restore(); + }); + + function createMockPersistentState(data: Record = {}) { + const store: Record = { ...data }; + return { + get: async (key: string): Promise => store[key] as T | undefined, + set: async (key: string, value: T): Promise => { + store[key] = value; + }, + clear: async (): Promise => { + Object.keys(store).forEach((k) => delete store[k]); + }, + }; + } + + test('should remove system defaultEnvManager from global settings on first run', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; + const mockConfig = { + get: () => undefined, + has: () => false, + inspect: (key: string) => { + if (key === 'defaultEnvManager') { + return { globalValue: SYSTEM_MANAGER_ID }; + } + return undefined; + }, + update: (key: string, value: unknown, target: ConfigurationTarget) => { + updateCalls.push({ key, value, target }); + return Promise.resolve(); + }, + }; + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + const removal = updateCalls.find( + (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, + ); + assert.ok(removal, 'Should remove defaultEnvManager from Global settings'); + assert.strictEqual(removal!.value, undefined, 'Should set to undefined to remove'); + + // Verify migration flag was set + const migrated = await mockState.get('globalSettingsMigration.systemEnvManagerRemoved'); + assert.strictEqual(migrated, true, 'Should set migration flag'); + }); + + test('should not remove if globalValue is not system', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; + const mockConfig = { + get: () => undefined, + has: () => false, + inspect: (key: string) => { + if (key === 'defaultEnvManager') { + return { globalValue: 'ms-python.python:venv' }; + } + return undefined; + }, + update: (key: string, value: unknown, target: ConfigurationTarget) => { + updateCalls.push({ key, value, target }); + return Promise.resolve(); + }, + }; + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + const removal = updateCalls.find( + (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, + ); + assert.strictEqual(removal, undefined, 'Should NOT remove non-system values'); + }); + + test('should not run migration if already migrated', async () => { + const mockState = createMockPersistentState({ + 'globalSettingsMigration.systemEnvManagerRemoved': true, + }); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; + const mockConfig = { + get: () => undefined, + has: () => false, + inspect: (key: string) => { + if (key === 'defaultEnvManager') { + return { globalValue: SYSTEM_MANAGER_ID }; + } + return undefined; + }, + update: (key: string, value: unknown, target: ConfigurationTarget) => { + updateCalls.push({ key, value, target }); + return Promise.resolve(); + }, + }; + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + assert.strictEqual(updateCalls.length, 0, 'Should not write any settings if already migrated'); + }); +}); From b29285a8c47f324b697464897a3e963287ffdddf Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Wed, 13 May 2026 10:12:30 -0700 Subject: [PATCH 3/4] address review: try/catch + telemetry + fire-and-forget --- src/common/telemetry/constants.ts | 22 ++++++++++++++++++ src/extension.ts | 7 ++++-- src/features/settings/settingHelpers.ts | 19 ++++++++++++++- .../settings/settingHelpers.unit.test.ts | 23 +++++++++++++++++++ 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 47e45888..ff50b26e 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -168,6 +168,17 @@ export enum EventNames { * - errorType: string (classified error category, on failure only) */ PET_RESOLVE = 'PET.RESOLVE', + /** + * Telemetry event for the one-time migration that removes a stale + * `python-envs.defaultEnvManager: system` value from User (global) settings. + * Fires only on the activation when the migration actually runs (not on subsequent runs). + * Properties: + * - outcome: 'removed' (was set to system, removed successfully) + * | 'not_set' (no global value of system found, nothing to do) + * | 'failed' (attempted removal threw) + * - errorType: string (only when outcome === 'failed') + */ + MIGRATION_SYSTEM_ENV_MANAGER = 'MIGRATION.SYSTEM_ENV_MANAGER', } // Map all events to their properties @@ -549,4 +560,15 @@ export interface IEventNamePropertyMapping { result: 'success' | 'timeout' | 'error'; errorType?: string; }; + + /* __GDPR__ + "migration.system_env_manager": { + "outcome": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" }, + "errorType": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" } + } + */ + [EventNames.MIGRATION_SYSTEM_ENV_MANAGER]: { + outcome: 'removed' | 'not_set' | 'failed'; + errorType?: string; + }; } diff --git a/src/extension.ts b/src/extension.ts index 6f72b70c..662867b9 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -158,8 +158,11 @@ export async function activate(context: ExtensionContext): Promise + traceError(`[migration] migrateGlobalDefaultEnvManagerSetting threw: ${err}`), + ); const statusBar = new PythonStatusBarImpl(); context.subscriptions.push(statusBar); diff --git a/src/features/settings/settingHelpers.ts b/src/features/settings/settingHelpers.ts index 858a9920..4d0c1b6a 100644 --- a/src/features/settings/settingHelpers.ts +++ b/src/features/settings/settingHelpers.ts @@ -4,6 +4,8 @@ import { PythonProject } from '../../api'; import { DEFAULT_ENV_MANAGER_ID, DEFAULT_PACKAGE_MANAGER_ID, SYSTEM_MANAGER_ID } from '../../common/constants'; import { traceError, traceInfo, traceWarn } from '../../common/logging'; import { getGlobalPersistentState } from '../../common/persistentState'; +import { EventNames } from '../../common/telemetry/constants'; +import { sendTelemetryEvent } from '../../common/telemetry/sender'; import * as workspaceApis from '../../common/workspace.apis'; import { PythonProjectManager, PythonProjectSettings } from '../../internal.api'; @@ -595,11 +597,26 @@ export async function migrateGlobalDefaultEnvManagerSetting(): Promise { const inspect = config.inspect('defaultEnvManager'); if (inspect?.globalValue === SYSTEM_MANAGER_ID) { - await config.update('defaultEnvManager', undefined, ConfigurationTarget.Global); + try { + await config.update('defaultEnvManager', undefined, ConfigurationTarget.Global); + } catch (err) { + // Don't mark migration done; we'll retry on a future activation. + traceWarn( + `[migration] Failed to remove 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings: ${err}`, + ); + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { + outcome: 'failed', + errorType: err instanceof Error ? err.name : typeof err, + }); + return; + } traceInfo( `[migration] Removed 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings ` + `(was set unintentionally by a previous version). See https://github.com/microsoft/vscode-python-environments/issues/1468`, ); + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'removed' }); + } else { + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'not_set' }); } await state.set(MIGRATION_KEY, true); diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index 27fa96d8..f4fb9ea1 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -722,4 +722,27 @@ suite('Setting Helpers - migrateGlobalDefaultEnvManagerSetting', () => { assert.strictEqual(updateCalls.length, 0, 'Should not write any settings if already migrated'); }); + + test('should not set migration flag if update throws (so we retry next activation)', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const mockConfig = { + get: () => undefined, + has: () => false, + inspect: (key: string) => { + if (key === 'defaultEnvManager') { + return { globalValue: SYSTEM_MANAGER_ID }; + } + return undefined; + }, + update: () => Promise.reject(new Error('settings.json read-only')), + }; + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + const migrated = await mockState.get('globalSettingsMigration.systemEnvManagerRemoved'); + assert.notStrictEqual(migrated, true, 'Should NOT set migration flag when removal fails'); + }); }); From 1dd97bb36d847813ebb75a29f055c6df52c2104b Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Wed, 13 May 2026 10:47:31 -0700 Subject: [PATCH 4/4] copilot comment fixes --- src/common/telemetry/constants.ts | 7 +- src/extension.ts | 13 +- src/features/settings/settingHelpers.ts | 82 +++++-- .../settings/settingHelpers.unit.test.ts | 201 ++++++++++++------ 4 files changed, 215 insertions(+), 88 deletions(-) diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 4ecdd8e1..858529e7 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -202,8 +202,9 @@ export enum EventNames { * `python-envs.defaultEnvManager: system` value from User (global) settings. * Fires only on the activation when the migration actually runs (not on subsequent runs). * Properties: - * - outcome: 'removed' (was set to system, removed successfully) - * | 'not_set' (no global value of system found, nothing to do) + * - outcome: 'removed' (was set to system, all user-scope slots cleared) + * | 'partial' (cleared current context's slot but another user-scope slot still has it; will retry) + * | 'not_set' (no user-scope slot of system found, nothing to do) * | 'failed' (attempted removal threw) * - errorType: string (only when outcome === 'failed') */ @@ -653,7 +654,7 @@ export interface IEventNamePropertyMapping { } */ [EventNames.MIGRATION_SYSTEM_ENV_MANAGER]: { - outcome: 'removed' | 'not_set' | 'failed'; + outcome: 'removed' | 'partial' | 'not_set' | 'failed'; errorType?: string; }; } diff --git a/src/extension.ts b/src/extension.ts index cc15d97e..3a28d78f 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -163,11 +163,14 @@ export async function activate(context: ExtensionContext): Promise - traceError(`[migration] migrateGlobalDefaultEnvManagerSetting threw: ${err}`), - ); + // One-time migration: remove `system` defaultEnvManager from User settings if a previous + // version wrote it there (bug #1468). Awaited so the migration deterministically affects + // initial environment selection on the very first activation after upgrade. + try { + await migrateGlobalDefaultEnvManagerSetting(); + } catch (err) { + traceError(`[migration] migrateGlobalDefaultEnvManagerSetting threw: ${err}`); + } const statusBar = new PythonStatusBarImpl(); context.subscriptions.push(statusBar); diff --git a/src/features/settings/settingHelpers.ts b/src/features/settings/settingHelpers.ts index 4d890eb2..2e4857ea 100644 --- a/src/features/settings/settingHelpers.ts +++ b/src/features/settings/settingHelpers.ts @@ -582,12 +582,41 @@ export function getSettingUserScope(section: string, key: string): T | undefi const MIGRATION_KEY = 'globalSettingsMigration.systemEnvManagerRemoved'; +/** + * Returns true if any user-scope slot of the inspection result equals `value`. + * For window-scoped settings VS Code may populate `globalRemoteValue` and/or + * `globalLocalValue` in addition to `globalValue` depending on context. + */ +function userScopeHasValue(inspect: { globalValue?: string } | undefined, value: string): boolean { + if (!inspect) { + return false; + } + const record = inspect as Record; + if (record.globalRemoteValue === value) { + return true; + } + if (record.globalLocalValue === value) { + return true; + } + if (inspect.globalValue === value) { + return true; + } + return false; +} + /** * One-time migration: removes `defaultEnvManager` from User (global) settings if it was * set to `system` by the extension. This was an unintentional side effect of a bug where * the extension wrote to User scope when no workspace was open. Having `system` at the * User level causes all workspaces to ignore local .venv environments. * + * Because `python-envs.defaultEnvManager` is a window-scoped setting, the stale value can + * land in any of `globalValue`, `globalLocalValue`, or `globalRemoteValue` depending on + * which context (local vs remote) hit the bug. We check all three, attempt removal via + * `ConfigurationTarget.Global` (which clears the slot for the current context), then + * re-inspect. If any user-scope slot still holds the stale value we do NOT mark the + * migration complete, so a future activation in the other context can finish the job. + * * See: https://github.com/microsoft/vscode-python-environments/issues/1468 */ export async function migrateGlobalDefaultEnvManagerSetting(): Promise { @@ -600,28 +629,43 @@ export async function migrateGlobalDefaultEnvManagerSetting(): Promise { const config = workspaceApis.getConfiguration('python-envs', undefined); const inspect = config.inspect('defaultEnvManager'); - if (inspect?.globalValue === SYSTEM_MANAGER_ID) { - try { - await config.update('defaultEnvManager', undefined, ConfigurationTarget.Global); - } catch (err) { - // Don't mark migration done; we'll retry on a future activation. - traceWarn( - `[migration] Failed to remove 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings: ${err}`, - ); - sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { - outcome: 'failed', - errorType: err instanceof Error ? err.name : typeof err, - }); - return; - } + if (!userScopeHasValue(inspect, SYSTEM_MANAGER_ID)) { + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'not_set' }); + await state.set(MIGRATION_KEY, true); + return; + } + + try { + await config.update('defaultEnvManager', undefined, ConfigurationTarget.Global); + } catch (err) { + // Don't mark migration done; we'll retry on a future activation. + traceWarn( + `[migration] Failed to remove 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings: ${err}`, + ); + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { + outcome: 'failed', + errorType: err instanceof Error ? err.name : typeof err, + }); + return; + } + + // Re-inspect: `update(Global)` only clears the current context's slot. If another + // context's slot (local vs remote) still holds the stale value, leave the flag unset + // so the next activation in that context can clear it. + const after = config.inspect('defaultEnvManager'); + if (userScopeHasValue(after, SYSTEM_MANAGER_ID)) { traceInfo( - `[migration] Removed 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings ` + - `(was set unintentionally by a previous version). See https://github.com/microsoft/vscode-python-environments/issues/1468`, + `[migration] Partially removed 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings; ` + + `another context still has it set. Will retry on next activation.`, ); - sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'removed' }); - } else { - sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'not_set' }); + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'partial' }); + return; } + traceInfo( + `[migration] Removed 'python-envs.defaultEnvManager: ${SYSTEM_MANAGER_ID}' from User settings ` + + `(was set unintentionally by a previous version). See https://github.com/microsoft/vscode-python-environments/issues/1468`, + ); + sendTelemetryEvent(EventNames.MIGRATION_SYSTEM_ENV_MANAGER, undefined, { outcome: 'removed' }); await state.set(MIGRATION_KEY, true); } diff --git a/src/test/features/settings/settingHelpers.unit.test.ts b/src/test/features/settings/settingHelpers.unit.test.ts index 3a032086..ef195add 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -5,6 +5,7 @@ import * as sinon from 'sinon'; import { ConfigurationTarget, Uri, WorkspaceFolder } from 'vscode'; import * as logging from '../../../common/logging'; import * as persistentState from '../../../common/persistentState'; +import * as sender from '../../../common/telemetry/sender'; import * as workspaceApis from '../../../common/workspace.apis'; import { addPythonProjectSetting, @@ -618,10 +619,16 @@ suite('Setting Helpers - Empty Path Migration', () => { suite('Setting Helpers - migrateGlobalDefaultEnvManagerSetting', () => { const SYSTEM_MANAGER_ID = 'ms-python.python:system'; + const VENV_MANAGER_ID = 'ms-python.python:venv'; + const MIGRATION_FLAG_KEY = 'globalSettingsMigration.systemEnvManagerRemoved'; + const TELEMETRY_EVENT = 'MIGRATION.SYSTEM_ENV_MANAGER'; + let sandbox: sinon.SinonSandbox; + let sendTelemetryEventStub: sinon.SinonStub; setup(() => { sandbox = sinon.createSandbox(); + sendTelemetryEventStub = sandbox.stub(sender, 'sendTelemetryEvent'); }); teardown(() => { @@ -641,25 +648,57 @@ suite('Setting Helpers - migrateGlobalDefaultEnvManagerSetting', () => { }; } - test('should remove system defaultEnvManager from global settings on first run', async () => { - const mockState = createMockPersistentState(); - sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); - + /** + * Builds a mock WorkspaceConfiguration whose `inspect('defaultEnvManager')` returns the + * provided sequence of values (one per call), so a test can simulate a different state + * for the post-update re-inspect. If only one entry is given it is reused for every call. + */ + function createMockConfig(options: { + inspectSequence: Array | undefined>; + updateImpl?: (key: string, value: unknown, target: ConfigurationTarget) => Promise; + }) { const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; + let callIndex = 0; const mockConfig = { get: () => undefined, has: () => false, inspect: (key: string) => { - if (key === 'defaultEnvManager') { - return { globalValue: SYSTEM_MANAGER_ID }; + if (key !== 'defaultEnvManager') { + return undefined; } - return undefined; + const i = Math.min(callIndex, options.inspectSequence.length - 1); + callIndex += 1; + return options.inspectSequence[i]; }, update: (key: string, value: unknown, target: ConfigurationTarget) => { updateCalls.push({ key, value, target }); - return Promise.resolve(); + return options.updateImpl ? options.updateImpl(key, value, target) : Promise.resolve(); }, }; + return { mockConfig, updateCalls }; + } + + function assertTelemetryOutcome(expected: string, extraProps?: Record) { + assert.strictEqual(sendTelemetryEventStub.callCount, 1, 'Should emit exactly one telemetry event'); + const call = sendTelemetryEventStub.firstCall; + assert.strictEqual(call.args[0], TELEMETRY_EVENT, 'Should use the correct event name'); + const props = call.args[2] as Record | undefined; + assert.ok(props, 'Telemetry event should have properties'); + assert.strictEqual(props!.outcome, expected, `outcome should be '${expected}'`); + if (extraProps) { + for (const [k, v] of Object.entries(extraProps)) { + assert.strictEqual(props![k], v, `prop '${k}' should be '${String(v)}'`); + } + } + } + + test('removes system defaultEnvManager from globalValue and marks migrated', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [{ globalValue: SYSTEM_MANAGER_ID }, { globalValue: undefined }], + }); sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); await migrateGlobalDefaultEnvManagerSetting(); @@ -668,90 +707,130 @@ suite('Setting Helpers - migrateGlobalDefaultEnvManagerSetting', () => { (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, ); assert.ok(removal, 'Should remove defaultEnvManager from Global settings'); - assert.strictEqual(removal!.value, undefined, 'Should set to undefined to remove'); + assert.strictEqual(removal!.value, undefined, 'Should pass undefined to clear the setting'); - // Verify migration flag was set - const migrated = await mockState.get('globalSettingsMigration.systemEnvManagerRemoved'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); assert.strictEqual(migrated, true, 'Should set migration flag'); + assertTelemetryOutcome('removed'); }); - test('should not remove if globalValue is not system', async () => { + test('removes when stale value is in globalRemoteValue (remote context)', async () => { const mockState = createMockPersistentState(); sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); - const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; - const mockConfig = { - get: () => undefined, - has: () => false, - inspect: (key: string) => { - if (key === 'defaultEnvManager') { - return { globalValue: 'ms-python.python:venv' }; - } - return undefined; - }, - update: (key: string, value: unknown, target: ConfigurationTarget) => { - updateCalls.push({ key, value, target }); - return Promise.resolve(); - }, - }; + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [ + { globalRemoteValue: SYSTEM_MANAGER_ID, globalValue: undefined }, + { globalRemoteValue: undefined, globalValue: undefined }, + ], + }); sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); await migrateGlobalDefaultEnvManagerSetting(); - const removal = updateCalls.find( - (c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global, - ); - assert.strictEqual(removal, undefined, 'Should NOT remove non-system values'); + assert.strictEqual(updateCalls.length, 1, 'Should call update once'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.strictEqual(migrated, true); + assertTelemetryOutcome('removed'); }); - test('should not run migration if already migrated', async () => { - const mockState = createMockPersistentState({ - 'globalSettingsMigration.systemEnvManagerRemoved': true, + test('removes when stale value is in globalLocalValue', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [ + { globalLocalValue: SYSTEM_MANAGER_ID, globalValue: undefined }, + { globalLocalValue: undefined, globalValue: undefined }, + ], + }); + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + assert.strictEqual(updateCalls.length, 1, 'Should call update once'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.strictEqual(migrated, true); + assertTelemetryOutcome('removed'); + }); + + test('does not mark migrated when another user-scope slot still has the stale value (partial)', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + // Initial inspect: both globalValue and globalLocalValue have the stale value. + // Post-update: only globalValue is cleared (current context); globalLocalValue persists. + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [ + { globalValue: SYSTEM_MANAGER_ID, globalLocalValue: SYSTEM_MANAGER_ID }, + { globalValue: undefined, globalLocalValue: SYSTEM_MANAGER_ID }, + ], }); + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + assert.strictEqual(updateCalls.length, 1, 'Should still attempt removal once'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.notStrictEqual(migrated, true, 'Should NOT set migration flag when another slot still holds the value'); + assertTelemetryOutcome('partial'); + }); + + test('does not remove when no user-scope slot has the stale value (not_set) and marks migrated', async () => { + const mockState = createMockPersistentState(); sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); - const updateCalls: Array<{ key: string; value: unknown; target: ConfigurationTarget }> = []; - const mockConfig = { - get: () => undefined, - has: () => false, - inspect: (key: string) => { - if (key === 'defaultEnvManager') { - return { globalValue: SYSTEM_MANAGER_ID }; - } - return undefined; - }, - update: (key: string, value: unknown, target: ConfigurationTarget) => { - updateCalls.push({ key, value, target }); - return Promise.resolve(); - }, - }; + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [{ globalValue: VENV_MANAGER_ID }], + }); sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); await migrateGlobalDefaultEnvManagerSetting(); + assert.strictEqual(updateCalls.length, 0, 'Should not call update when no stale value present'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.strictEqual(migrated, true, 'Should mark migrated so we never check again'); + assertTelemetryOutcome('not_set'); + }); + + test('does not run migration if already migrated', async () => { + const mockState = createMockPersistentState({ + [MIGRATION_FLAG_KEY]: true, + }); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + const { mockConfig, updateCalls } = createMockConfig({ + inspectSequence: [{ globalValue: SYSTEM_MANAGER_ID }], + }); + const getConfigStub = sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + assert.strictEqual(updateCalls.length, 0, 'Should not write any settings if already migrated'); + assert.strictEqual(getConfigStub.callCount, 0, 'Should short-circuit before reading configuration'); + assert.strictEqual(sendTelemetryEventStub.callCount, 0, 'Should not emit telemetry on no-op runs'); }); - test('should not set migration flag if update throws (so we retry next activation)', async () => { + test('does not set migration flag if update throws and reports failed telemetry', async () => { const mockState = createMockPersistentState(); sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); - const mockConfig = { - get: () => undefined, - has: () => false, - inspect: (key: string) => { - if (key === 'defaultEnvManager') { - return { globalValue: SYSTEM_MANAGER_ID }; - } - return undefined; + const updateError = new Error('settings.json read-only'); + let updateCalled = false; + const { mockConfig } = createMockConfig({ + inspectSequence: [{ globalValue: SYSTEM_MANAGER_ID }], + updateImpl: () => { + updateCalled = true; + return Promise.reject(updateError); }, - update: () => Promise.reject(new Error('settings.json read-only')), - }; + }); sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); await migrateGlobalDefaultEnvManagerSetting(); - const migrated = await mockState.get('globalSettingsMigration.systemEnvManagerRemoved'); + assert.strictEqual(updateCalled, true, 'Failure path must actually attempt the update'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); assert.notStrictEqual(migrated, true, 'Should NOT set migration flag when removal fails'); + assertTelemetryOutcome('failed', { errorType: 'Error' }); }); });