Conversation
Co-authored-by: Copilot <copilot@github.com>
|
Insiders build for testing: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=436352&view=results |
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple regressions around chat.disableAIFeatures where AI features could be re-enabled (or fail to stay enabled) without an explicit user opt-in, and where remote scenarios could throw unhandled enablement errors. It does so by centralizing the “what scope should we write?” decision into a dedicated helper and updating the relevant actions/contributions to use explicit ConfigurationTargets instead of relying on implicit target derivation.
Changes:
- Added
chatAIDisabledHelpers.tsto compute explicit, scope-targeted updates for global opt-in, workspace opt-in, and extension-enablement reconciliation. - Updated Extensions UI actions and Chat setup trigger/teardown to use these helpers (avoiding implicit scope-walking writes).
- Added a unit test suite covering the reported issue scenarios and helper behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/services/chat/test/common/chatAIDisabledHelpers.test.ts | New unit tests validating helper logic for the reported issues and edge cases. |
| src/vs/workbench/services/chat/common/chatAIDisabledHelpers.ts | New helper module centralizing scope-aware chat.disableAIFeatures update decisions and enablement-changeability checks. |
| src/vs/workbench/contrib/extensions/browser/extensionsActions.ts | Uses helpers to clear/override chat.disableAIFeatures with explicit targets when opting in globally or per-workspace. |
| src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupContributions.ts | Uses helpers for explicit opt-in writes and adds a guard to avoid enablement changes in non-changeable states. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/services/chat/common/chatAIDisabledHelpers.ts:103
isExtensionEnablementChangeableonly models the states handled inthrowErrorIfEnablementStateCannotBeChanged, butsetEnablementcan also throw for other non-changeable scenarios (e.g. extensions enabled in the environment /EnablementState.EnabledByEnvironment, language packs, etc. viathrowErrorIfCannotChangeEnablement). As a result, callers may still hit the same unhandled-error class this helper is meant to prevent. Consider usingIWorkbenchExtensionEnablementService.canChangeEnablement(..)/canChangeWorkspaceEnablement(..)at the call site, or expanding this helper to cover all cases that makesetEnablementthrow.
// `setEnablement` throws (in `throwErrorIfEnablementStateCannotBeChanged`) for these states.
// `DisabledByTrustRequirement` is excluded - `setEnablement` handles it via a trust request.
// `DisabledByExtensionDependency` is excluded - it is only conditionally non-changeable.
const NON_CHANGEABLE_ENABLEMENT_STATES: readonly EnablementState[] = [
EnablementState.DisabledByEnvironment,
EnablementState.DisabledByMalicious,
EnablementState.DisabledByVirtualWorkspace,
EnablementState.DisabledByExtensionKind,
EnablementState.DisabledByAllowlist,
EnablementState.DisabledByInvalidExtension,
];
/**
* Returns true if the extension's current enablement state can be changed via `setEnablement`.
* Callers driving enablement from external state must skip the call when this returns false to
* avoid unhandled errors (https://github.com/microsoft/vscode/issues/312381).
*/
export function isExtensionEnablementChangeable(state: EnablementState): boolean {
return !NON_CHANGEABLE_ENABLEMENT_STATES.includes(state);
}
src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupContributions.ts:806
- Before calling
setEnablement, the code only checksisExtensionEnablementChangeable(defaultChatExtension.enablementState), butsetEnablementcan throw for reasons beyond the enablement-state switch (e.g. extensions enabled in the environment). Since this path is explicitly trying to avoid unhandled errors in remote/locked scenarios, it would be safer to gate withthis.extensionEnablementService.canChangeEnablement(...)/canChangeWorkspaceEnablement(...)(depending onstate), or to wrapsetEnablementin a try/catch and log.
// The chat extension's enablement may be locked by the environment (extension kind, virtual workspace,
// allow-list, etc.). Calling `setEnablement` in those states throws an unhandled error
// (https://github.com/microsoft/vscode/issues/312381).
if (!isExtensionEnablementChangeable(defaultChatExtension.enablementState)) {
return;
}
await this.extensionsWorkbenchService.setEnablement([defaultChatExtension], state);
await this.extensionsWorkbenchService.updateRunningExtensions(state === EnablementState.EnabledGlobally || state === EnablementState.EnabledWorkspace ? localize('restartExtensionHost.reason.enable', "Enabling AI features") : localize('restartExtensionHost.reason.disable', "Disabling AI features"));
- Files reviewed: 4/4 changed files
- Comments generated: 2
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupContributions.ts:812
maybeEnableOrDisableExtensionisasync, but all current call sites invoke it withoutawait(e.g. inhandleChatDisabled). IfsetEnablementorupdateRunningExtensionsrejects for any reason other than thecanChange*precheck, it will still surface as an unhandled promise rejection. Consider wrapping the body in atry/catch(and report viaonUnexpectedError) or have the callers attach a.catch(...)when invoking this method.
private async maybeEnableOrDisableExtension(state: EnablementState.EnabledGlobally | EnablementState.EnabledWorkspace | EnablementState.DisabledGlobally | EnablementState.DisabledWorkspace): Promise<void> {
const defaultChatExtension = this.extensionsWorkbenchService.local.find(value => ExtensionIdentifier.equals(value.identifier.id, defaultChat.chatExtensionId));
if (!defaultChatExtension?.local) {
return;
}
// The chat extension's enablement may be locked by the environment (extension kind, virtual workspace,
// allow-list, language pack, settings-sync auth provider, etc.). Calling `setEnablement` in those
// cases throws an unhandled error (https://github.com/microsoft/vscode/issues/312381).
const workspace = state === EnablementState.EnabledWorkspace || state === EnablementState.DisabledWorkspace;
const canChange = workspace
? this.extensionEnablementService.canChangeWorkspaceEnablement(defaultChatExtension.local)
: this.extensionEnablementService.canChangeEnablement(defaultChatExtension.local);
if (!canChange) {
return;
}
await this.extensionsWorkbenchService.setEnablement([defaultChatExtension], state);
await this.extensionsWorkbenchService.updateRunningExtensions(state === EnablementState.EnabledGlobally || state === EnablementState.EnabledWorkspace ? localize('restartExtensionHost.reason.enable', "Enabling AI features") : localize('restartExtensionHost.reason.disable', "Disabling AI features"));
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
|
@dmitrivMS Thanks for the PR and especially for the repro steps 👏 I have extracted Repro 1 and Repro 3 into separate issues #314733 and #314734 respectively and came up with fixes for each. Regarding Repro 2 - why do you think it is a bug? Since user has ai features disabled in both user and workspace and triggering using the AI features in the workspace could be meant only for workspace. Enabling it everywhere could surprise user. Hence closing this PR |
|
Chiming in on @sandy081's comment since I originally raised #309947: My expectation, as a user, in reproduction 2 would be that triggering AI features in a workspace with AI disabled both at the user level and workspace level would produce a prompt asking what to do. Something like: "You have AI disabled in your workspace and user settings but have started an AI command. Would you like to turn it on in your workspace settings, user settings, or both? [Enable for Workspace] [Enable Everywhere] [Both]" I think this approach would provide the most user agency and make it obvious to the user what is about to happen. |
|
My two cents is that that command/dialogue shouldn't even be available if AI features were disabled in the user profile |
I don't disagree, honestly. I think, if a user has opt'd to disable all AI-related features, there should be no way to explicitly trigger them without resetting that flag. That said, and understanding that's a much more major change, I think the prompt is a good compromise between not making major changes that would take forever to implement and still giving users good agency. |
Fixes for a number of issues where
disableAIFeaturessetting is incorrected turned off without user taking explicit action to enable AI.Fixes #309947 – chat.disableAIFeatures removed from user/profile settings without consent
Fixes #311898 – per-workspace AI opt-in does not persist after reload
Fixes #312381 – unhandled "Cannot change enablement of GitHub Copilot Chat extension because of its extension kind" error in remote scenarios
Fixes #312998 – likely covered (same root cause as #309947)
Note: Since these issues involve race conditions, I am creating stable repro steps that force the bugs to occur (such as using workbench.settings.applyToAllProfiles etc). However, based on reports the bugs also happen with simple reload of VS Code if race conditions occur.
Repro 1
1., Clean install of VS Code (no profiles, no settings, no sign in)
2. Open User Settings (JSON) and keep it open
3. Go to Profiles and create a new profile, name it 'A'
4. Replace settings.json with this:
Actual result
chat.disableAIFeaturesis changed tofalseinsettings.json.Expected result
settings.jsonshould not be touched.Verified as fixed by the chagne.
Repro 2
Use AI Features with Copilot for free…command and immediately cancel the setup dialogActual result
The 'disableAIFeatures' is removed from user
settings.json.Workspace
settings.jsonstill hasdisableAIFeaturesset totrue.Expected result
Both need to be modified so that overall result is
false.Verified as fixed by the change.
Repro 3
GitHub Copilot ChatDisable AI FeaturesEnable AI Features (Workspace)Actual result
AI is not enabled, workspace setting is not updated.
Expected result
AI to be enabled, workspace setting to be updated.
Verified as fixed by the change.