From 6a86bbee4d67f62ef7684c57afb4efa528931b1b Mon Sep 17 00:00:00 2001 From: Goodnessmbakara Date: Sat, 25 Oct 2025 20:32:49 +0100 Subject: [PATCH] feat: implement MCP server deduplication with comprehensive key generation - Add deduplication logic to prevent duplicate server definitions - Use McpServerLaunch.hash() for comprehensive key generation including: - Variable resolution through existing infrastructure - Environment variables in hash calculation - HTTP headers for HTTP servers - Implement priority-based deduplication (user > workspace > extensions) - Add proper telemetry tracking with typed data structures - Add comprehensive test suite following VS Code patterns - Addresses reviewer feedback on variable resolution and environment variables Fixes: Server deduplication now properly handles all launch configuration properties --- .../contrib/mcp/common/mcpService.ts | 139 +++++++++- .../mcp/test/common/mcpDeduplication.test.ts | 250 ++++++++++++++++++ 2 files changed, 382 insertions(+), 7 deletions(-) create mode 100644 src/vs/workbench/contrib/mcp/test/common/mcpDeduplication.test.ts diff --git a/src/vs/workbench/contrib/mcp/common/mcpService.ts b/src/vs/workbench/contrib/mcp/common/mcpService.ts index 4b3d6cb4bd7e8..06c296ca908bc 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpService.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpService.ts @@ -12,9 +12,26 @@ import { IInstantiationService } from '../../../../platform/instantiation/common import { ILogService } from '../../../../platform/log/common/log.js'; import { mcpAutoStartConfig, McpAutoStartValue } from '../../../../platform/mcp/common/mcpManagement.js'; import { StorageScope } from '../../../../platform/storage/common/storage.js'; +import { ITelemetryService } from '../../../../platform/telemetry/common/telemetry.js'; import { IMcpRegistry } from './mcpRegistryTypes.js'; import { McpServer, McpServerMetadataCache } from './mcpServer.js'; -import { IAutostartResult, IMcpServer, IMcpService, McpCollectionDefinition, McpConnectionState, McpDefinitionReference, McpServerCacheState, McpServerDefinition, McpStartServerInteraction, McpToolName, UserInteractionRequiredError } from './mcpTypes.js'; +import { IAutostartResult, IMcpServer, IMcpService, McpCollectionDefinition, McpConnectionState, McpDefinitionReference, McpServerCacheState, McpServerDefinition, McpServerLaunch, McpStartServerInteraction, McpToolName, UserInteractionRequiredError } from './mcpTypes.js'; + +type ServerDeduplicationData = { + serverId: string; + collectionId: string; + scope: StorageScope; + replaced: boolean; +}; + +type ServerDeduplicationClassification = { + owner: 'microsoft'; + comment: 'MCP server deduplication event tracking'; + serverId: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'The ID of the server being deduplicated' }; + collectionId: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'The ID of the collection containing the server' }; + scope: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; comment: 'The scope of the collection (user, workspace, etc.)' }; + replaced: { classification: 'SystemMetaData'; purpose: 'FeatureInsight'; isMeasurement: true; comment: 'Whether the server was replaced by a higher priority one' }; +}; import { startServerAndWaitForLiveTools } from './mcpTypesUtils.js'; type IMcpServerRec = { object: IMcpServer; toolPrefix: string }; @@ -36,7 +53,8 @@ export class McpService extends Disposable implements IMcpService { @IInstantiationService private readonly _instantiationService: IInstantiationService, @IMcpRegistry private readonly _mcpRegistry: IMcpRegistry, @ILogService private readonly _logService: ILogService, - @IConfigurationService private readonly configurationService: IConfigurationService + @IConfigurationService private readonly configurationService: IConfigurationService, + @ITelemetryService private readonly _telemetryService: ITelemetryService ) { super(); @@ -151,11 +169,11 @@ export class McpService extends Disposable implements IMcpService { private async _activateCollections() { const collections = await this._mcpRegistry.discoverCollections(); - this.updateCollectedServers(); + await this.updateCollectedServers(); return new Set(collections.map(c => c.id)); } - public updateCollectedServers() { + public async updateCollectedServers(): Promise { const prefixGenerator = new McpPrefixGenerator(); const definitions = this._mcpRegistry.collections.get().flatMap(collectionDefinition => collectionDefinition.serverDefinitions.get().map(serverDefinition => { @@ -164,10 +182,12 @@ export class McpService extends Disposable implements IMcpService { }) ); - const nextDefinitions = new Set(definitions); + // Deduplicate server definitions based on server ID and complete configuration + const deduplicatedDefinitions = await this.deduplicateServerDefinitions(definitions); + const nextDefinitions = new Set(deduplicatedDefinitions); const currentServers = this._servers.get(); const nextServers: IMcpServerRec[] = []; - const pushMatch = (match: (typeof definitions)[0], rec: IMcpServerRec) => { + const pushMatch = (match: (typeof deduplicatedDefinitions)[0], rec: IMcpServerRec) => { nextDefinitions.delete(match); nextServers.push(rec); const connection = rec.object.connection.get(); @@ -180,7 +200,7 @@ export class McpService extends Disposable implements IMcpService { // Transfer over any servers that are still valid. for (const server of currentServers) { - const match = definitions.find(d => defsEqual(server.object, d) && server.toolPrefix === d.toolPrefix); + const match = deduplicatedDefinitions.find(d => defsEqual(server.object, d) && server.toolPrefix === d.toolPrefix); if (match) { pushMatch(match, server); } else { @@ -208,6 +228,111 @@ export class McpService extends Disposable implements IMcpService { }); } + /** + * Deduplicates server definitions with priority-based logic. + * Keeps the highest priority server definition when duplicates are found. + * Uses comprehensive key generation that includes all launch configuration properties. + * + * @param definitions Array of server definitions to deduplicate + * @returns Deduplicated array of server definitions + * @internal + */ + private async deduplicateServerDefinitions(definitions: Array<{ serverDefinition: McpServerDefinition; collectionDefinition: McpCollectionDefinition; toolPrefix: string }>): Promise> { + const seen = new Map(); + const deduplicated: Array<{ serverDefinition: McpServerDefinition; collectionDefinition: McpCollectionDefinition; toolPrefix: string }> = []; + + // Generate keys for all definitions + const keyPromises = definitions.map(async (def) => ({ + def, + key: await this.getServerDefinitionKey(def.serverDefinition) + })); + const keyedDefinitions = await Promise.all(keyPromises); + + for (const { def, key } of keyedDefinitions) { + const existing = seen.get(key); + + if (existing) { + // Found a duplicate - check priority + const existingPriority = this.getCollectionPriority(existing.collectionDefinition); + const currentPriority = this.getCollectionPriority(def.collectionDefinition); + + if (currentPriority > existingPriority) { + // Current definition has higher priority, replace existing + seen.set(key, def); + const existingIndex = deduplicated.findIndex(d => d === existing); + if (existingIndex !== -1) { + deduplicated[existingIndex] = def; + } + + // Track deduplication telemetry + this._telemetryService.publicLog2('mcp/serverDeduplication', { + serverId: def.serverDefinition.id, + collectionId: def.collectionDefinition.id, + scope: def.collectionDefinition.scope, + replaced: true + }); + + this._logService.debug(`MCP deduplication: Replaced server ${def.serverDefinition.id} from collection ${existing.collectionDefinition.id} with higher priority from ${def.collectionDefinition.id}`); + } else { + // Existing definition has higher or equal priority, skip current + this._telemetryService.publicLog2('mcp/serverDeduplication', { + serverId: def.serverDefinition.id, + collectionId: def.collectionDefinition.id, + scope: def.collectionDefinition.scope, + replaced: false + }); + + this._logService.debug(`MCP deduplication: Skipping duplicate server ${def.serverDefinition.id} from collection ${def.collectionDefinition.id} (lower priority than ${existing.collectionDefinition.id})`); + } + } else { + seen.set(key, def); + deduplicated.push(def); + } + } + + return deduplicated; + } + + /** + * Gets a unique key for a server definition to identify duplicates. + * Uses the existing McpServerLaunch.hash() method which includes all relevant properties: + * - For stdio: command, args, cwd, env, envFile + * - For HTTP: uri, headers + * - Handles variable resolution through the existing hash mechanism + * + * @param serverDefinition The server definition to generate a key for + * @returns A unique string key based on server ID and complete launch configuration + * @internal + */ + private async getServerDefinitionKey(serverDefinition: McpServerDefinition): Promise { + // Use the existing McpServerLaunch.hash() method which already handles all properties + // including environment variables, cwd, headers, and variable resolution + const launchHash = await McpServerLaunch.hash(serverDefinition.launch); + return `${serverDefinition.id}:${launchHash}`; + } + + /** + * Gets the priority of a collection for deduplication. + * Higher numbers indicate higher priority. + * + * @param collection The collection to get priority for + * @returns Priority number (higher = more important) + * @internal + */ + private getCollectionPriority(collection: McpCollectionDefinition): number { + // Priority order: user settings > workspace > extensions + switch (collection.scope) { + case StorageScope.PROFILE: + return 3; // User settings - highest priority + case StorageScope.WORKSPACE: + return 2; // Workspace settings - medium priority + case StorageScope.APPLICATION: + return 1; // Extensions - lowest priority + default: + return 0; + } + } + public override dispose(): void { this._servers.get().forEach(s => s.object.dispose()); super.dispose(); diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpDeduplication.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpDeduplication.test.ts new file mode 100644 index 0000000000000..daa7e551b7af6 --- /dev/null +++ b/src/vs/workbench/contrib/mcp/test/common/mcpDeduplication.test.ts @@ -0,0 +1,250 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as assert from 'assert'; +import { observableValue } from '../../../../../base/common/observable.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; +import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; +import { ILogService, NullLogService } from '../../../../../platform/log/common/log.js'; +import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; +import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js'; +import { ITelemetryService, TelemetryLevel } from '../../../../../platform/telemetry/common/telemetry.js'; +import { StorageScope } from '../../../../../platform/storage/common/storage.js'; +import { McpService } from '../../common/mcpService.js'; +import { McpCollectionDefinition, McpServerDefinition, McpServerTransportType, McpServerTrust } from '../../common/mcpTypes.js'; +import { IMcpRegistry } from '../../common/mcpRegistryTypes.js'; +import { TestMcpRegistry } from './mcpRegistryTypes.js'; + +class TestTelemetryService implements ITelemetryService { + declare readonly _serviceBrand: undefined; + public events: Array<{ eventName: string; data: any }> = []; + + publicLog2(eventName: string, data: any): void { + this.events.push({ eventName, data }); + } + + publicLog(eventName: string, data: any): void { + this.events.push({ eventName, data }); + } + + publicLogError(eventName: string, data: any): void { + this.events.push({ eventName, data }); + } + + publicLogError2(eventName: string, data: any): void { + this.events.push({ eventName, data }); + } + + setEnabled(value: boolean): void { + // No-op + } + + isOptedIn(): boolean { + return true; + } + + get telemetryLevel() { return TelemetryLevel.USAGE; } + get sessionId() { return 'test-session'; } + get machineId() { return 'test-machine'; } + get sqmId() { return 'test-sqm'; } + get firstSessionDate() { return '2023-01-01'; } + get msftInternal() { return false; } + get isOptedInProductImprovement() { return false; } + get devDeviceId() { return 'test-device'; } + sendErrorTelemetry = false; + setExperimentProperty(name: string, value: string): void { /* no-op */ } +} + +suite('Workbench - MCP - Deduplication', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + let instantiationService: TestInstantiationService; + let mcpService: McpService; + let telemetryService: TestTelemetryService; + let mockRegistry: TestMcpRegistry; + + setup(() => { + telemetryService = new TestTelemetryService(); + mockRegistry = new TestMcpRegistry(store.add(new TestInstantiationService())); + + const services = new ServiceCollection( + [ILogService, store.add(new NullLogService())], + [IConfigurationService, new TestConfigurationService()], + [ITelemetryService, telemetryService], + [IMcpRegistry, mockRegistry] + ); + + instantiationService = store.add(new TestInstantiationService(services)); + mcpService = store.add(instantiationService.createInstance(McpService)); + }); + + test('should deduplicate identical server definitions', async () => { + // Create two identical server definitions from different collections + const serverDef: McpServerDefinition = { + id: 'test-server', + label: 'Test Server', + cacheNonce: 'a', + launch: { + type: McpServerTransportType.Stdio, + command: 'test-command', + args: ['--arg1', '--arg2'], + env: { TEST_VAR: 'value1' }, + envFile: undefined, + cwd: '/test' + } + }; + + const collection1: McpCollectionDefinition = { + id: 'collection-1', + label: 'Collection 1', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.APPLICATION, + configTarget: 1 // ConfigurationTarget.USER + }; + + const collection2: McpCollectionDefinition = { + id: 'collection-2', + label: 'Collection 2', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.APPLICATION, + configTarget: 1 // ConfigurationTarget.USER + }; + + // Update the mock registry + mockRegistry.collections.set([collection1, collection2], undefined); + + // Test the deduplication logic + await mcpService.updateCollectedServers(); + + // Verify that only one server instance was created (deduplication worked) + const servers = mcpService.servers.get(); + assert.strictEqual(servers.length, 1, 'Should have only one server after deduplication'); + + // Verify telemetry was called for deduplication + const telemetryEvents = telemetryService.events; + const deduplicationEvents = telemetryEvents.filter(e => e.eventName === 'mcp/serverDeduplication'); + assert.strictEqual(deduplicationEvents.length, 1, 'Should have one deduplication telemetry event'); + assert.strictEqual(deduplicationEvents[0].data.serverId, 'test-server', 'Telemetry should track correct server ID'); + }); + + test('should keep servers with different configurations as separate', async () => { + // Create two servers with different configurations + const serverDef1: McpServerDefinition = { + id: 'test-server', + label: 'Test Server', + cacheNonce: 'a', + launch: { + type: McpServerTransportType.Stdio, + command: 'test-command', + args: ['--arg1'], + env: { TEST_VAR: 'value1' }, + envFile: undefined, + cwd: '/test' + } + }; + + const serverDef2: McpServerDefinition = { + id: 'test-server', + label: 'Test Server', + cacheNonce: 'a', + launch: { + type: McpServerTransportType.Stdio, + command: 'test-command', + args: ['--arg2'], // Different args + env: { TEST_VAR: 'value2' }, // Different env + envFile: undefined, + cwd: '/different' // Different cwd + } + }; + + const collection1: McpCollectionDefinition = { + id: 'collection-1', + label: 'Collection 1', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef1]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.APPLICATION, + configTarget: 1 // ConfigurationTarget.USER + }; + + const collection2: McpCollectionDefinition = { + id: 'collection-2', + label: 'Collection 2', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef2]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.APPLICATION, + configTarget: 1 // ConfigurationTarget.USER + }; + + // Update the mock registry + mockRegistry.collections.set([collection1, collection2], undefined); + + // Test the deduplication logic + await mcpService.updateCollectedServers(); + + // Should have two servers since they have different configurations + const servers = mcpService.servers.get(); + assert.strictEqual(servers.length, 2, 'Should have two servers with different configurations'); + }); + + test('should prioritize user settings over workspace settings for metadata', async () => { + // Create identical server definitions in different scopes + const serverDef: McpServerDefinition = { + id: 'test-server', + label: 'Test Server', + cacheNonce: 'a', + launch: { + type: McpServerTransportType.Stdio, + command: 'test-command', + args: ['--arg1'], + env: { TEST_VAR: 'value1' }, + envFile: undefined, + cwd: '/test' + } + }; + + const userCollection: McpCollectionDefinition = { + id: 'user-collection', + label: 'User Collection', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.PROFILE, // User scope - higher priority + configTarget: 1 // ConfigurationTarget.USER + }; + + const workspaceCollection: McpCollectionDefinition = { + id: 'workspace-collection', + label: 'Workspace Collection', + remoteAuthority: null, + serverDefinitions: observableValue('servers', [serverDef]), + trustBehavior: McpServerTrust.Kind.Trusted, + scope: StorageScope.WORKSPACE, // Workspace scope - lower priority + configTarget: 2 // ConfigurationTarget.WORKSPACE + }; + + // Update the mock registry + mockRegistry.collections.set([userCollection, workspaceCollection], undefined); + + // Test the deduplication logic + await mcpService.updateCollectedServers(); + + // Should have only one server since they are identical after deduplication + // The user collection should take priority for metadata + const servers = mcpService.servers.get(); + assert.strictEqual(servers.length, 1, 'Should deduplicate identical servers with user settings taking priority'); + + // Verify telemetry was called for deduplication + const telemetryEvents = telemetryService.events; + const deduplicationEvents = telemetryEvents.filter(e => e.eventName === 'mcp/serverDeduplication'); + assert.strictEqual(deduplicationEvents.length, 1, 'Should have one deduplication telemetry event'); + }); +});