|
| 1 | +# MCP Remote Server Support and Duplicate Detection - Design Document |
| 2 | + |
| 3 | +## Problem Statement |
| 4 | + |
| 5 | +### Issue 1: Remote MCP Server Configurations Not Handled |
| 6 | + |
| 7 | +The current MCP implementation only supports stdio-based servers. Remote servers (HTTP/SSE) are: |
| 8 | +1. Read from collection/bundle configuration correctly as strings |
| 9 | +2. Incorrectly typed as `McpStdioServerConfig` instead of `McpRemoteServerConfig` |
| 10 | +3. Processed by `McpConfigService.processServerDefinition()` which only handles stdio properties |
| 11 | +4. Missing URL/headers handling in the config service |
| 12 | + |
| 13 | +### Issue 2: Duplicate MCP Servers Across Bundles |
| 14 | + |
| 15 | +When multiple bundles define the same MCP server (by URL or command signature), they are all installed, leading to: |
| 16 | +1. Duplicate entries in `mcp.json` |
| 17 | +2. Potential conflicts and confusion |
| 18 | +3. No way to identify and disable duplicates |
| 19 | + |
| 20 | +## Design Goals |
| 21 | + |
| 22 | +1. **Proper Type Discrimination**: Fix the type system to properly distinguish stdio vs remote servers |
| 23 | +2. **Remote Server Support**: Handle HTTP/SSE servers in `McpConfigService` and `McpServerManager` |
| 24 | +3. **Duplicate Detection**: Identify semantically equivalent servers across bundles |
| 25 | +4. **Duplicate Handling**: Disable duplicates while preserving their configuration for reference |
| 26 | + |
| 27 | +## Type System Refactoring |
| 28 | + |
| 29 | +### Current Types (Problematic) |
| 30 | + |
| 31 | +```typescript |
| 32 | +// PROBLEM: McpServerDefinition is aliased to stdio-only |
| 33 | +export type McpServerDefinition = McpStdioServerConfig; |
| 34 | +export type McpServersManifest = Record<string, McpServerDefinition>; |
| 35 | +``` |
| 36 | + |
| 37 | +### Proposed Types |
| 38 | + |
| 39 | +```typescript |
| 40 | +/** |
| 41 | + * Type guard to check if a server config is stdio-based |
| 42 | + */ |
| 43 | +export function isStdioServerConfig(config: McpServerConfig): config is McpStdioServerConfig { |
| 44 | + return !('url' in config) || config.type === 'stdio' || config.type === undefined; |
| 45 | +} |
| 46 | + |
| 47 | +/** |
| 48 | + * Type guard to check if a server config is remote (HTTP/SSE) |
| 49 | + */ |
| 50 | +export function isRemoteServerConfig(config: McpServerConfig): config is McpRemoteServerConfig { |
| 51 | + return 'url' in config && (config.type === 'http' || config.type === 'sse'); |
| 52 | +} |
| 53 | + |
| 54 | +/** |
| 55 | + * Union type for server definitions (replaces legacy alias) |
| 56 | + */ |
| 57 | +export type McpServerDefinition = McpServerConfig; |
| 58 | + |
| 59 | +/** |
| 60 | + * Manifest of MCP servers (supports both stdio and remote) |
| 61 | + */ |
| 62 | +export type McpServersManifest = Record<string, McpServerConfig>; |
| 63 | +``` |
| 64 | + |
| 65 | +## McpConfigService Changes |
| 66 | + |
| 67 | +### New Method: `processServerDefinition()` (Refactored) |
| 68 | + |
| 69 | +```typescript |
| 70 | +processServerDefinition( |
| 71 | + serverName: string, |
| 72 | + definition: McpServerConfig, |
| 73 | + bundleId: string, |
| 74 | + bundleVersion: string, |
| 75 | + bundlePath: string |
| 76 | +): McpServerConfig { |
| 77 | + const context: McpVariableContext = { bundlePath, bundleId, bundleVersion, env: process.env }; |
| 78 | + |
| 79 | + if (isRemoteServerConfig(definition)) { |
| 80 | + return this.processRemoteServerDefinition(definition, context); |
| 81 | + } else { |
| 82 | + return this.processStdioServerDefinition(definition, context); |
| 83 | + } |
| 84 | +} |
| 85 | + |
| 86 | +private processRemoteServerDefinition( |
| 87 | + definition: McpRemoteServerConfig, |
| 88 | + context: McpVariableContext |
| 89 | +): McpRemoteServerConfig { |
| 90 | + return { |
| 91 | + type: definition.type, |
| 92 | + url: this.substituteVariables(definition.url, context)!, |
| 93 | + headers: definition.headers ? Object.fromEntries( |
| 94 | + Object.entries(definition.headers).map(([k, v]) => [ |
| 95 | + k, |
| 96 | + this.substituteVariables(v, context)! |
| 97 | + ]) |
| 98 | + ) : undefined, |
| 99 | + disabled: definition.disabled, |
| 100 | + description: definition.description |
| 101 | + }; |
| 102 | +} |
| 103 | + |
| 104 | +private processStdioServerDefinition( |
| 105 | + definition: McpStdioServerConfig, |
| 106 | + context: McpVariableContext |
| 107 | +): McpStdioServerConfig { |
| 108 | + return { |
| 109 | + type: definition.type, |
| 110 | + command: this.substituteVariables(definition.command, context)!, |
| 111 | + args: definition.args?.map(arg => this.substituteVariables(arg, context)!), |
| 112 | + env: definition.env ? Object.fromEntries( |
| 113 | + Object.entries(definition.env).map(([k, v]) => [k, this.substituteVariables(v, context)!]) |
| 114 | + ) : undefined, |
| 115 | + envFile: this.substituteVariables(definition.envFile, context), |
| 116 | + disabled: definition.disabled, |
| 117 | + description: definition.description |
| 118 | + }; |
| 119 | +} |
| 120 | +``` |
| 121 | + |
| 122 | +## Duplicate Detection Strategy |
| 123 | + |
| 124 | +### Server Identity |
| 125 | + |
| 126 | +Two servers are considered duplicates if they have the same "identity": |
| 127 | + |
| 128 | +**For Stdio Servers:** |
| 129 | +- Same `command` AND same `args` (after variable substitution) |
| 130 | + |
| 131 | +**For Remote Servers:** |
| 132 | +- Same `url` (after variable substitution) |
| 133 | + |
| 134 | +### Duplicate Handling |
| 135 | + |
| 136 | +When duplicates are detected: |
| 137 | +1. Keep the **first** installed server enabled |
| 138 | +2. Mark subsequent duplicates as `disabled: true` |
| 139 | +3. Add a `description` noting the duplicate status and which bundle owns the original |
| 140 | +4. Store duplicate relationship in tracking metadata |
| 141 | + |
| 142 | +### New Method: `detectAndDisableDuplicates()` |
| 143 | + |
| 144 | +```typescript |
| 145 | +interface DuplicateInfo { |
| 146 | + serverName: string; |
| 147 | + duplicateOf: string; |
| 148 | + bundleId: string; |
| 149 | + originalBundleId: string; |
| 150 | +} |
| 151 | + |
| 152 | +async detectAndDisableDuplicates( |
| 153 | + scope: 'user' | 'workspace' |
| 154 | +): Promise<{ duplicatesDisabled: DuplicateInfo[]; config: McpConfiguration }> { |
| 155 | + const config = await this.readMcpConfig(scope); |
| 156 | + const tracking = await this.readTrackingMetadata(scope); |
| 157 | + |
| 158 | + const serverIdentities = new Map<string, { serverName: string; bundleId: string }>(); |
| 159 | + const duplicatesDisabled: DuplicateInfo[] = []; |
| 160 | + |
| 161 | + for (const [serverName, serverConfig] of Object.entries(config.servers)) { |
| 162 | + const identity = this.computeServerIdentity(serverConfig); |
| 163 | + const existing = serverIdentities.get(identity); |
| 164 | + |
| 165 | + if (existing && !serverConfig.disabled) { |
| 166 | + // This is a duplicate - disable it |
| 167 | + config.servers[serverName] = { |
| 168 | + ...serverConfig, |
| 169 | + disabled: true, |
| 170 | + description: `Duplicate of ${existing.serverName} (from bundle ${existing.bundleId})` |
| 171 | + }; |
| 172 | + |
| 173 | + const metadata = tracking.managedServers[serverName]; |
| 174 | + duplicatesDisabled.push({ |
| 175 | + serverName, |
| 176 | + duplicateOf: existing.serverName, |
| 177 | + bundleId: metadata?.bundleId || 'unknown', |
| 178 | + originalBundleId: existing.bundleId |
| 179 | + }); |
| 180 | + } else if (!serverConfig.disabled) { |
| 181 | + const metadata = tracking.managedServers[serverName]; |
| 182 | + serverIdentities.set(identity, { |
| 183 | + serverName, |
| 184 | + bundleId: metadata?.bundleId || 'unknown' |
| 185 | + }); |
| 186 | + } |
| 187 | + } |
| 188 | + |
| 189 | + return { duplicatesDisabled, config }; |
| 190 | +} |
| 191 | + |
| 192 | +private computeServerIdentity(config: McpServerConfig): string { |
| 193 | + if (isRemoteServerConfig(config)) { |
| 194 | + return `remote:${config.url}`; |
| 195 | + } else { |
| 196 | + const argsStr = config.args?.join('|') || ''; |
| 197 | + return `stdio:${config.command}:${argsStr}`; |
| 198 | + } |
| 199 | +} |
| 200 | +``` |
| 201 | + |
| 202 | +## Integration Points |
| 203 | + |
| 204 | +### McpServerManager.installServers() |
| 205 | + |
| 206 | +After installing servers, call `detectAndDisableDuplicates()`: |
| 207 | + |
| 208 | +```typescript |
| 209 | +async installServers(...): Promise<McpInstallResult> { |
| 210 | + // ... existing installation logic ... |
| 211 | + |
| 212 | + // After successful installation, detect and disable duplicates |
| 213 | + const { duplicatesDisabled, config } = await this.configService.detectAndDisableDuplicates(options.scope); |
| 214 | + |
| 215 | + if (duplicatesDisabled.length > 0) { |
| 216 | + await this.configService.writeMcpConfig(config, options.scope, false); |
| 217 | + result.warnings?.push( |
| 218 | + `Disabled ${duplicatesDisabled.length} duplicate server(s): ${duplicatesDisabled.map(d => d.serverName).join(', ')}` |
| 219 | + ); |
| 220 | + } |
| 221 | + |
| 222 | + return result; |
| 223 | +} |
| 224 | +``` |
| 225 | + |
| 226 | +## Test Scenarios |
| 227 | + |
| 228 | +### Type Guards |
| 229 | +1. `isStdioServerConfig` returns true for stdio configs |
| 230 | +2. `isStdioServerConfig` returns true for configs without type (backward compat) |
| 231 | +3. `isRemoteServerConfig` returns true for http configs |
| 232 | +4. `isRemoteServerConfig` returns true for sse configs |
| 233 | +5. `isRemoteServerConfig` returns false for stdio configs |
| 234 | + |
| 235 | +### Remote Server Processing |
| 236 | +1. Process HTTP server with URL substitution |
| 237 | +2. Process SSE server with headers substitution |
| 238 | +3. Process remote server with all variable types (bundlePath, env) |
| 239 | +4. Preserve disabled and description fields |
| 240 | + |
| 241 | +### Stdio Server Processing (Existing + Enhanced) |
| 242 | +1. Process stdio server with command/args substitution |
| 243 | +2. Process stdio server with env substitution |
| 244 | +3. Process stdio server with envFile substitution |
| 245 | +4. Handle type field correctly |
| 246 | + |
| 247 | +### Duplicate Detection |
| 248 | +1. Detect duplicate stdio servers (same command + args) |
| 249 | +2. Detect duplicate remote servers (same URL) |
| 250 | +3. First server remains enabled, duplicates disabled |
| 251 | +4. Duplicate description references original |
| 252 | +5. No false positives for different servers |
| 253 | +6. Handle mixed stdio/remote without cross-type duplicates |
| 254 | + |
| 255 | +### Integration |
| 256 | +1. Install bundle with remote servers |
| 257 | +2. Install bundle with mixed stdio/remote servers |
| 258 | +3. Install multiple bundles, duplicates auto-disabled |
| 259 | +4. Uninstall bundle, duplicates re-enabled if applicable |
| 260 | + |
| 261 | +## Migration Notes |
| 262 | + |
| 263 | +- `McpServerDefinition` type alias change is backward compatible (union includes original type) |
| 264 | +- Existing stdio-only manifests continue to work |
| 265 | +- No changes to collection schema required (already supports remote) |
0 commit comments