Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 40 additions & 14 deletions src/services/mcp/McpHub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -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"),
Expand All @@ -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,
}),
])
Expand Down Expand Up @@ -194,7 +215,7 @@ export class McpHub {
private validateServerConfig(config: any, serverName?: string): z.infer<typeof ServerConfigSchema> {
// 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) {
Expand All @@ -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)
}

Expand All @@ -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)
}
Expand Down Expand Up @@ -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") {
// HTTP connection (streamable-http is normalized to http by schema transformation)
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"
Expand Down
213 changes: 213 additions & 0 deletions src/services/mcp/__tests__/McpHub.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2147,4 +2147,217 @@ describe("McpHub", () => {
)
})
})

describe("HTTP type standardization", () => {
let StdioClientTransport: ReturnType<typeof vi.fn>
let Client: ReturnType<typeof vi.fn>
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<typeof vi.fn>
Client = clientModule.Client as ReturnType<typeof vi.fn>

// 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'",
)
})
})
})