Prevent writing settings to global scope for environment and package managers#1489
Prevent writing settings to global scope for environment and package managers#1489eleanorjboyd wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents the extension from persisting environment/package manager defaults into User (global) settings, reducing cross-project interference (addresses issue #1468).
Changes:
- Removed
ConfigurationTarget.Globalwrites fordefaultEnvManageranddefaultPackageManagerin settings helpers. - Updated unit tests to assert no global-scope writes occur for these settings.
- Documented that the extension avoids writing settings to User scope (with a note referencing #1468).
Show a summary per file
| File | Description |
|---|---|
| src/features/settings/settingHelpers.ts | Removes global-scope persistence for default manager settings. |
| src/test/features/settings/settingHelpers.unit.test.ts | Updates tests to ensure default manager settings aren’t written to global scope. |
| docs/managing-python-projects.md | Adds guidance about settings scope behavior and rationale. |
Copilot's findings
Comments suppressed due to low confidence (5)
src/test/features/settings/settingHelpers.unit.test.ts:171
- Same issue as above: this test can miss user/global writes if the boolean form (
true) is used instead ofConfigurationTarget.Global. Consider asserting that there are zero updates fordefaultEnvManagerregardless of the third argument shape.
const envManagerUpdates = updateCalls.filter(
(c) => c.key === 'defaultEnvManager' && c.target === ConfigurationTarget.Global,
);
assert.strictEqual(envManagerUpdates.length, 0, 'Should never write to user/global settings');
});
src/test/features/settings/settingHelpers.unit.test.ts:192
- Same issue as above: restricting the check to
ConfigurationTarget.Globalcan miss global writes using the boolean form. Consider asserting thatdefaultPackageManageris never updated when invoked withproject: undefined, regardless of target representation.
const pkgManagerUpdates = updateCalls.filter(
(c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global,
);
assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings');
});
src/features/settings/settingHelpers.ts:261
- Same as
setAllManagerSettings:editswithprojectunset are filtered out and now become a no-op. To avoid callers accidentally passing “global” edits and thinking they were saved, consider either rejecting/logging those edits or updating the type/docs to remove the “undefined means global” expectation.
}
});
await Promise.all(promises);
}
src/features/settings/settingHelpers.ts:326
- Same concern here:
project: undefinededits are filtered out, so callers trying to persist a global package manager selection will get a silent no-op. Consider surfacing that (trace/log/return) or tightening the type contract so it’s clear only workspace(-folder) scoped edits are supported.
}
});
await Promise.all(promises);
}
src/test/features/settings/settingHelpers.unit.test.ts:150
- Same issue as above: checking only for
ConfigurationTarget.Globalcan miss user/global writes if the boolean form (true) is used for the update target. Track the raw target and assert nodefaultPackageManagerupdate is attempted in any global form.
const pkgManagerUpdates = updateCalls.filter(
(c) => c.key === 'defaultPackageManager' && c.target === ConfigurationTarget.Global,
);
assert.strictEqual(pkgManagerUpdates.length, 0, 'Should never write to user/global settings');
});
- Files reviewed: 3/3 changed files
- Comments generated: 4
| } | ||
| ``` | ||
|
|
||
| > **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`. |
There was a problem hiding this comment.
Grammar: "If a user want" → "If a user wants".
| > **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`. | |
| > **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 wants a user-level default, they can set it manually in their User `settings.json`. |
| 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'); | ||
| }); |
There was a problem hiding this comment.
This assertion only checks ConfigurationTarget.Global, but WorkspaceConfiguration.update() also supports passing true for the user/global target. Because the mock records configurationTarget via a cast, a global write using the boolean form (or an omitted target, depending on VS Code behavior) would not be detected by this test. Consider recording the raw third argument and asserting that no update for defaultEnvManager occurs regardless of whether the target is Global or true.
This issue also appears in the following locations of the same file:
- line 146
- line 167
- line 188
| } | ||
| ``` | ||
|
|
||
| > **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`. |
There was a problem hiding this comment.
This note says the extension “never writes settings to the User (global) scope”, but the codebase does update global settings in at least some cases (e.g., clearing python-envs.defaultEnvManager via ConfigurationTarget.Global during cleanup/migration). Consider rewording to clarify that the extension does not set/persist user/global defaults, while it may still remove/clean up global values when necessary.
| > **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`. | |
| > **Important**: The extension does not set or persist extension-managed defaults in the User (global) scope. Normal extension-managed settings are written at the Workspace or Workspace Folder level only, which prevents values from persisting across unrelated projects and causing unexpected interference (see [#1468](https://github.com/microsoft/vscode-python-environments/issues/1468)). In some cleanup or migration scenarios, the extension may still remove or clear existing User/global values when necessary. If a user wants a user-level default, they can set it manually in their User `settings.json`. |
| } | ||
| }); | ||
|
|
||
| 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); | ||
| } |
There was a problem hiding this comment.
With the global update path removed, any edits that come in with project unset are now silently ignored (the function filters to !!e.project). Consider making this behavior explicit (e.g., validate and traceVerbose when an edit has no project, or update the Edit*Settings contract/docs to no longer imply support for a “global” edit) so callers don’t assume settings were persisted.
This issue also appears in the following locations of the same file:
- line 257
- line 322
prevents #1468. Now we no longer save env manager or package manage default values to user settings. If a user wants to save these they can but the extension should not as it could very easily cause disruption across a users projects