diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index caca5ddb39..af0a7563b5 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -66,17 +66,19 @@ const BaseConfigSchema = z.object({ }) // Custom error messages for better user feedback -const typeErrorMessage = "Server type must be 'stdio', 'sse', or 'streamable-http'" +const typeErrorMessage = "Server type must be 'stdio', 'sse', or 'http' (preferred) / 'streamable-http' (deprecated)" const stdioFieldsErrorMessage = "For 'stdio' type servers, you must provide a 'command' field and can optionally include 'args' and 'env'" const sseFieldsErrorMessage = "For 'sse' type servers, you must provide a 'url' field and can optionally include 'headers'" +const httpFieldsErrorMessage = + "For 'http' type servers, you must provide a 'url' field and can optionally include 'headers'" const streamableHttpFieldsErrorMessage = - "For 'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'" + "For 'http'/'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'" const mixedFieldsErrorMessage = - "Cannot mix 'stdio' and ('sse' or 'streamable-http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'streamable-http' use 'url' and 'headers'" + "Cannot mix 'stdio' and ('sse' or 'http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'http' use 'url' and 'headers'" const missingFieldsErrorMessage = - "Server configuration must include either 'command' (for stdio) or 'url' (for sse/streamable-http) and a corresponding 'type' if 'url' is used." + "Server configuration must include either 'command' (for stdio) or 'url' (for sse/http) and a corresponding 'type' if 'url' is used." // Helper function to create a refined schema with better error messages const createServerTypeSchema = () => { @@ -112,7 +114,24 @@ const createServerTypeSchema = () => { type: "sse" as const, })) .refine((data) => data.type === undefined || data.type === "sse", { message: typeErrorMessage }), - // StreamableHTTP config (has url field) + // HTTP config (preferred, has url field) + BaseConfigSchema.extend({ + type: z.enum(["http"]).optional(), + url: z.string().url("URL must be a valid URL format"), + headers: z.record(z.string()).optional(), + // Ensure no stdio fields are present + command: z.undefined().optional(), + args: z.undefined().optional(), + env: z.undefined().optional(), + }) + .transform((data) => ({ + ...data, + type: "http" as const, + })) + .refine((data) => data.type === undefined || data.type === "http", { + message: typeErrorMessage, + }), + // StreamableHTTP config (deprecated but supported for backward compatibility) BaseConfigSchema.extend({ type: z.enum(["streamable-http"]).optional(), url: z.string().url("URL must be a valid URL format"), @@ -124,9 +143,11 @@ const createServerTypeSchema = () => { }) .transform((data) => ({ ...data, - type: "streamable-http" as const, + // Normalize streamable-http to http internally + type: "http" as const, })) - .refine((data) => data.type === undefined || data.type === "streamable-http", { + .refine((data) => true, { + // Always pass since we're transforming to http message: typeErrorMessage, }), ]) @@ -194,7 +215,7 @@ export class McpHub { private validateServerConfig(config: any, serverName?: string): z.infer { // Detect configuration issues before validation const hasStdioFields = config.command !== undefined - const hasUrlFields = config.url !== undefined // Covers sse and streamable-http + const hasUrlFields = config.url !== undefined // Covers sse, http, and streamable-http // Check for mixed fields (stdio vs url-based) if (hasStdioFields && hasUrlFields) { @@ -208,11 +229,13 @@ export class McpHub { // For url-based configs, type must be provided by the user if (hasUrlFields && !config.type) { - throw new Error("Configuration with 'url' must explicitly specify 'type' as 'sse' or 'streamable-http'.") + throw new Error( + "Configuration with 'url' must explicitly specify 'type' as 'sse' or 'http' (preferred) / 'streamable-http' (deprecated).", + ) } // Validate type if provided - if (config.type && !["stdio", "sse", "streamable-http"].includes(config.type)) { + if (config.type && !["stdio", "sse", "http", "streamable-http"].includes(config.type)) { throw new Error(typeErrorMessage) } @@ -223,6 +246,9 @@ export class McpHub { if (config.type === "sse" && !hasUrlFields) { throw new Error(sseFieldsErrorMessage) } + if (config.type === "http" && !hasUrlFields) { + throw new Error(httpFieldsErrorMessage) + } if (config.type === "streamable-http" && !hasUrlFields) { throw new Error(streamableHttpFieldsErrorMessage) } @@ -733,17 +759,17 @@ export class McpHub { } else { console.error(`No stderr stream for ${name}`) } - } else if (configInjected.type === "streamable-http") { - // Streamable HTTP connection + } else if (configInjected.type === "http" || configInjected.type === "streamable-http") { + // HTTP connection (streamable-http is mapped to http for backward compatibility) transport = new StreamableHTTPClientTransport(new URL(configInjected.url), { requestInit: { headers: configInjected.headers, }, }) - // Set up Streamable HTTP specific error handling + // Set up HTTP specific error handling transport.onerror = async (error) => { - console.error(`Transport error for "${name}" (streamable-http):`, error) + console.error(`Transport error for "${name}" (http):`, error) const connection = this.findConnection(name, source) if (connection) { connection.server.status = "disconnected" diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 1db924ed6c..69d5bc92ef 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -2147,4 +2147,217 @@ describe("McpHub", () => { ) }) }) + + describe("HTTP type standardization", () => { + let StdioClientTransport: ReturnType + let Client: ReturnType + let mockTransport: any + let mockClient: any + + beforeEach(async () => { + // Reset mocks + vi.clearAllMocks() + + // Get references to the mocked constructors + const stdioModule = await import("@modelcontextprotocol/sdk/client/stdio.js") + const clientModule = await import("@modelcontextprotocol/sdk/client/index.js") + StdioClientTransport = stdioModule.StdioClientTransport as ReturnType + Client = clientModule.Client as ReturnType + + // Setup mock transport + mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + // Setup mock client + mockClient = { + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + } + + Client.mockImplementation(() => mockClient) + }) + + it("should accept 'http' as the primary type", async () => { + // Mock the config file read with 'http' type + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "http-test-server": { + type: "http", + url: "https://api.example.com/mcp", + headers: { + Authorization: "Bearer token", + }, + }, + }, + }), + ) + + // Create McpHub and let it initialize + const mcpHub = new McpHub(mockProvider as ClineProvider) + await new Promise((resolve) => setTimeout(resolve, 100)) + + // Find the connection + const connection = mcpHub.connections.find((conn) => conn.server.name === "http-test-server") + expect(connection).toBeDefined() + + // Type guard check - should be connected + if (connection && connection.type === "connected") { + expect(connection.client).toBeDefined() + expect(connection.transport).toBeDefined() + expect(connection.server.status).toBe("connected") + } else { + throw new Error("Connection should be of type 'connected'") + } + }) + + it("should accept 'streamable-http' for backward compatibility and map it to 'http'", async () => { + // Mock the config file read with 'streamable-http' type (backward compatibility) + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "legacy-http-server": { + type: "streamable-http", + url: "https://api.example.com/mcp", + headers: { + Authorization: "Bearer token", + }, + }, + }, + }), + ) + + // Create McpHub and let it initialize + const mcpHub = new McpHub(mockProvider as ClineProvider) + await new Promise((resolve) => setTimeout(resolve, 100)) + + // Find the connection + const connection = mcpHub.connections.find((conn) => conn.server.name === "legacy-http-server") + expect(connection).toBeDefined() + + // Type guard check - should be connected + if (connection && connection.type === "connected") { + expect(connection.client).toBeDefined() + expect(connection.transport).toBeDefined() + expect(connection.server.status).toBe("connected") + } else { + throw new Error("Connection should be of type 'connected'") + } + }) + + it("should validate 'http' type configurations", () => { + // Test valid http config + const validHttpConfig = { + type: "http", + url: "https://api.example.com/mcp", + headers: { + Authorization: "Bearer token", + }, + timeout: 60, + } + expect(() => ServerConfigSchema.parse(validHttpConfig)).not.toThrow() + + // Test invalid http config (missing url) + const invalidHttpConfig = { + type: "http", + headers: { + Authorization: "Bearer token", + }, + } + expect(() => ServerConfigSchema.parse(invalidHttpConfig)).toThrow() + }) + + it("should validate 'streamable-http' type configurations for backward compatibility", () => { + // Test valid streamable-http config + const validStreamableHttpConfig = { + type: "streamable-http", + url: "https://api.example.com/mcp", + headers: { + Authorization: "Bearer token", + }, + timeout: 60, + } + expect(() => ServerConfigSchema.parse(validStreamableHttpConfig)).not.toThrow() + + // Test invalid streamable-http config (missing url) + const invalidStreamableHttpConfig = { + type: "streamable-http", + headers: { + Authorization: "Bearer token", + }, + } + expect(() => ServerConfigSchema.parse(invalidStreamableHttpConfig)).toThrow() + }) + + it("should normalize 'streamable-http' to 'http' internally", () => { + const streamableHttpConfig = { + type: "streamable-http", + url: "https://api.example.com/mcp", + } + + // Parse the config through the schema + const parsed = ServerConfigSchema.parse(streamableHttpConfig) + + // Should be normalized to 'http' + expect(parsed.type).toBe("http") + }) + + it("should update error messages to reflect new 'http' type", async () => { + // Create McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Test invalid type error + const invalidConfig = { + type: "invalid-type", + url: "https://api.example.com/mcp", + } + + // Try to validate the config + expect(() => mcpHub["validateServerConfig"](invalidConfig)).toThrow( + "Server type must be 'stdio', 'sse', or 'http' (preferred) / 'streamable-http' (deprecated)", + ) + }) + + it("should require explicit type for url-based configs", async () => { + // Create McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Test config with url but no type + const configWithoutType = { + url: "https://api.example.com/mcp", + } + + // Try to validate the config + expect(() => mcpHub["validateServerConfig"](configWithoutType)).toThrow( + "Configuration with 'url' must explicitly specify 'type' as 'sse' or 'http' (preferred) / 'streamable-http' (deprecated).", + ) + }) + + it("should provide appropriate error for http configs missing url", async () => { + // Create McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Test http config without url + const httpConfigWithoutUrl = { + type: "http", + headers: { + Authorization: "Bearer token", + }, + } + + // Try to validate the config + expect(() => mcpHub["validateServerConfig"](httpConfigWithoutUrl)).toThrow( + "For 'http' type servers, you must provide a 'url' field and can optionally include 'headers'", + ) + }) + }) })