Merge pull request microsoft#254873 from microsoft/copilot/fix-254872

Fix terminal suggest providers default enablement logic

Author: Tyriar

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts

@@ -153,11 +153,7 @@ export class TerminalCompletionService extends Disposable implements ITerminalCo
 			return this._collectCompletions(providers, shellType, promptValue, cursorPosition, allowFallbackCompletions, capabilities, token, explicitlyInvoked);
 		}
 
-		const providerConfig: { [key: string]: boolean } = this._configurationService.getValue(TerminalSuggestSettingId.Providers);
-		providers = providers.filter(p => {
-			const providerId = p.id;
-			return providerId && providerId in providerConfig && providerConfig[providerId] !== false;
-		});
+		providers = this._getEnabledProviders(providers);
 
 		if (!providers.length) {
 			return;
@@ -166,6 +162,14 @@ export class TerminalCompletionService extends Disposable implements ITerminalCo
 		return this._collectCompletions(providers, shellType, promptValue, cursorPosition, allowFallbackCompletions, capabilities, token, explicitlyInvoked);
 	}
 
+	protected _getEnabledProviders(providers: ITerminalCompletionProvider[]): ITerminalCompletionProvider[] {
+		const providerConfig: { [key: string]: boolean } = this._configurationService.getValue(TerminalSuggestSettingId.Providers);
+		return providers.filter(p => {
+			const providerId = p.id;
+			return providerId && (!(providerId in providerConfig) || providerConfig[providerId] !== false);
+		});
+	}
+
 	private async _collectCompletions(providers: ITerminalCompletionProvider[], shellType: TerminalShellType | undefined, promptValue: string, cursorPosition: number, allowFallbackCompletions: boolean, capabilities: ITerminalCapabilityStore, token: CancellationToken, explicitlyInvoked?: boolean): Promise<ITerminalCompletion[] | undefined> {
 		const completionPromises = providers.map(async provider => {
 			if (provider.shellTypes && shellType && !provider.shellTypes.includes(shellType)) {

src/vs/workbench/contrib/terminalContrib/suggest/common/terminalSuggestConfiguration.ts

@@ -57,6 +57,7 @@ export interface ITerminalSuggestConfiguration {
 	providers: {
 		'terminal-suggest': boolean;
 		'pwsh-shell-integration': boolean;
+		[key: string]: boolean;
 	};
 	showStatusBar: boolean;
 	cdPath: 'off' | 'relative' | 'absolute';

src/vs/workbench/contrib/terminalContrib/suggest/test/browser/terminalCompletionService.test.ts

@@ -5,7 +5,7 @@
 
 import { URI } from '../../../../../../base/common/uri.js';
 import { IFileService, IFileStatWithMetadata, IResolveMetadataFileOptions } from '../../../../../../platform/files/common/files.js';
-import { TerminalCompletionService, TerminalResourceRequestConfig } from '../../browser/terminalCompletionService.js';
+import { TerminalCompletionService, TerminalResourceRequestConfig, type ITerminalCompletionProvider } from '../../browser/terminalCompletionService.js';
 import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
 import assert, { fail } from 'assert';
 import { isWindows, type IProcessEnvironment } from '../../../../../../base/common/platform.js';
@@ -21,6 +21,7 @@ import { count } from '../../../../../../base/common/strings.js';
 import { WindowsShellType } from '../../../../../../platform/terminal/common/terminal.js';
 import { gitBashToWindowsPath, windowsToGitBashPath } from '../../browser/terminalGitBashHelpers.js';
 import { ILogService, NullLogService } from '../../../../../../platform/log/common/log.js';
+import { TerminalSuggestSettingId } from '../../common/terminalSuggestConfiguration.js';
 
 const pathSeparator = isWindows ? '\\' : '/';
 
@@ -770,4 +771,106 @@ suite('TerminalCompletionService', () => {
 		});
 
 	});
+
+	suite('Provider Configuration', () => {
+		// Test class that extends TerminalCompletionService to access protected methods
+		class TestTerminalCompletionService extends TerminalCompletionService {
+			public getEnabledProviders(providers: ITerminalCompletionProvider[]): ITerminalCompletionProvider[] {
+				return super._getEnabledProviders(providers);
+			}
+		}
+
+		let testTerminalCompletionService: TestTerminalCompletionService;
+
+		setup(() => {
+			testTerminalCompletionService = store.add(instantiationService.createInstance(TestTerminalCompletionService));
+		});
+
+		// Mock provider for testing
+		function createMockProvider(id: string): ITerminalCompletionProvider {
+			return {
+				id,
+				provideCompletions: async () => [{
+					label: `completion-from-${id}`,
+					kind: TerminalCompletionItemKind.Method,
+					replacementIndex: 0,
+					replacementLength: 0,
+					provider: id
+				}]
+			};
+		}
+
+		test('should enable providers by default when no configuration exists', () => {
+			const defaultProvider = createMockProvider('terminal-suggest');
+			const newProvider = createMockProvider('new-extension-provider');
+			const providers = [defaultProvider, newProvider];
+
+			// Set empty configuration (no provider keys)
+			configurationService.setUserConfiguration(TerminalSuggestSettingId.Providers, {});
+
+			const result = testTerminalCompletionService.getEnabledProviders(providers);
+
+			// Both providers should be enabled since they're not explicitly disabled
+			assert.strictEqual(result.length, 2, 'Should enable both providers by default');
+			assert.ok(result.includes(defaultProvider), 'Should include default provider');
+			assert.ok(result.includes(newProvider), 'Should include new provider');
+		});
+
+		test('should disable providers when explicitly set to false', () => {
+			const provider1 = createMockProvider('provider1');
+			const provider2 = createMockProvider('provider2');
+			const providers = [provider1, provider2];
+
+			// Disable provider1, leave provider2 unconfigured
+			configurationService.setUserConfiguration(TerminalSuggestSettingId.Providers, {
+				'provider1': false
+			});
+
+			const result = testTerminalCompletionService.getEnabledProviders(providers);
+
+			// Only provider2 should be enabled
+			assert.strictEqual(result.length, 1, 'Should enable only one provider');
+			assert.ok(result.includes(provider2), 'Should include unconfigured provider');
+			assert.ok(!result.includes(provider1), 'Should not include disabled provider');
+		});
+
+		test('should enable providers when explicitly set to true', () => {
+			const provider1 = createMockProvider('provider1');
+			const provider2 = createMockProvider('provider2');
+			const providers = [provider1, provider2];
+
+			// Explicitly enable provider1, leave provider2 unconfigured
+			configurationService.setUserConfiguration(TerminalSuggestSettingId.Providers, {
+				'provider1': true
+			});
+
+			const result = testTerminalCompletionService.getEnabledProviders(providers);
+
+			// Both providers should be enabled
+			assert.strictEqual(result.length, 2, 'Should enable both providers');
+			assert.ok(result.includes(provider1), 'Should include explicitly enabled provider');
+			assert.ok(result.includes(provider2), 'Should include unconfigured provider');
+		});
+
+		test('should handle mixed configuration correctly', () => {
+			const provider1 = createMockProvider('provider1');
+			const provider2 = createMockProvider('provider2');
+			const provider3 = createMockProvider('provider3');
+			const providers = [provider1, provider2, provider3];
+
+			// Mixed configuration: enable provider1, disable provider2, leave provider3 unconfigured
+			configurationService.setUserConfiguration(TerminalSuggestSettingId.Providers, {
+				'provider1': true,
+				'provider2': false
+			});
+
+			const result = testTerminalCompletionService.getEnabledProviders(providers);
+
+			// provider1 and provider3 should be enabled, provider2 should be disabled
+			assert.strictEqual(result.length, 2, 'Should enable two providers');
+			assert.ok(result.includes(provider1), 'Should include explicitly enabled provider');
+			assert.ok(result.includes(provider3), 'Should include unconfigured provider');
+			assert.ok(!result.includes(provider2), 'Should not include disabled provider');
+		});
+	});
 });