From 60cbed8f4e4c3bb45b7c9536ce8d6392292d262f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Jun 2026 02:45:53 +0000 Subject: [PATCH] Revert PR #1527: remove one-time defaultEnvManager User-settings migration --- src/common/telemetry/constants.ts | 25 +- src/extension.ts | 10 - src/features/settings/settingHelpers.ts | 95 +------- .../settings/settingHelpers.unit.test.ts | 224 +----------------- 4 files changed, 5 insertions(+), 349 deletions(-) diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 77e10d07..4175c90e 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -198,19 +198,7 @@ export enum EventNames { */ 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', - /** - * Telemetry event fired once per session, per URI, the first time a `.py` + * Telemetry event fired once per session, per URI, the first time a `.py` * file with a valid PEP 723 `# /// script` block is observed by the lazy * detector. Used to size the population of users who actually see inline * script files — the denominator for the "view vs edit" question. @@ -695,17 +683,6 @@ export interface IEventNamePropertyMapping { petCommitSha?: 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; - }; - /* __GDPR__ "inlineScript.detected": { "trigger": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "StellaHuang95" }, diff --git a/src/extension.ts b/src/extension.ts index e574eeab..eb429bdc 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -73,7 +73,6 @@ 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 { @@ -166,15 +165,6 @@ 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 ef195add..40233435 100644 --- a/src/test/features/settings/settingHelpers.unit.test.ts +++ b/src/test/features/settings/settingHelpers.unit.test.ts @@ -4,12 +4,9 @@ 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, @@ -153,6 +150,9 @@ 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,221 +616,3 @@ 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' }); - }); -});