diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 35f28217..858529e7 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -197,6 +197,18 @@ 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, 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') + */ + MIGRATION_SYSTEM_ENV_MANAGER = 'MIGRATION.SYSTEM_ENV_MANAGER', } // Map all events to their properties @@ -634,4 +646,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' | 'partial' | 'not_set' | 'failed'; + errorType?: string; + }; } diff --git a/src/extension.ts b/src/extension.ts index 64ffa07f..3a28d78f 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 { @@ -162,6 +163,15 @@ export async function activate(context: ExtensionContext): Promise(section: string, key: string): T | undefi } return undefined; } + +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 { + 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 (!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] 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: '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 40233435..ef195add 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -4,9 +4,12 @@ import * as path from 'path'; 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, + migrateGlobalDefaultEnvManagerSetting, setAllManagerSettings, setEnvironmentManager, setPackageManager, @@ -150,9 +153,6 @@ suite('Setting Helpers - Settings Write Behavior', () => { 'Should never write defaultPackageManager for global edits', ); }); - }); - - suite('setEnvironmentManager - Global Settings', () => { test('should NOT write to global even when value differs from current', async () => { const mockConfig = createMockConfig({ currentEnvManager: VENV_MANAGER_ID, @@ -616,3 +616,221 @@ 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(() => { + 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]); + }, + }; + } + + /** + * 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 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 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(); + + 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 pass undefined to clear the setting'); + + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.strictEqual(migrated, true, 'Should set migration flag'); + assertTelemetryOutcome('removed'); + }); + + test('removes when stale value is in globalRemoteValue (remote context)', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + 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(); + + assert.strictEqual(updateCalls.length, 1, 'Should call update once'); + const migrated = await mockState.get(MIGRATION_FLAG_KEY); + assert.strictEqual(migrated, true); + assertTelemetryOutcome('removed'); + }); + + 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 { 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('does not set migration flag if update throws and reports failed telemetry', async () => { + const mockState = createMockPersistentState(); + sandbox.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + 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); + }, + }); + sandbox.stub(workspaceApis, 'getConfiguration').returns(mockConfig as any); + + await migrateGlobalDefaultEnvManagerSetting(); + + 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' }); + }); +});