Skip to content

Commit fc11476

Browse files
roomoteroomote
committed
fix: Standardize MCP server type to "http" per VS Code specification
- Add support for "http" as the primary type for HTTP-based MCP servers - Maintain backward compatibility with "streamable-http" (deprecated) - Update validation logic to normalize "streamable-http" to "http" internally - Update error messages to reflect the new preferred type - Add comprehensive test coverage for HTTP type standardization Fixes #8847
1 parent f5d7ba1 commit fc11476

File tree

2 files changed

+253
-14
lines changed

2 files changed

+253
-14
lines changed

‎src/services/mcp/McpHub.ts‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,17 +66,19 @@ const BaseConfigSchema = z.object({
6666
})
6767

6868
// Custom error messages for better user feedback
69-
const typeErrorMessage = "Server type must be 'stdio', 'sse', or 'streamable-http'"
69+
const typeErrorMessage = "Server type must be 'stdio', 'sse', or 'http' (preferred) / 'streamable-http' (deprecated)"
7070
const stdioFieldsErrorMessage =
7171
"For 'stdio' type servers, you must provide a 'command' field and can optionally include 'args' and 'env'"
7272
const sseFieldsErrorMessage =
7373
"For 'sse' type servers, you must provide a 'url' field and can optionally include 'headers'"
74+
const httpFieldsErrorMessage =
75+
"For 'http' type servers, you must provide a 'url' field and can optionally include 'headers'"
7476
const streamableHttpFieldsErrorMessage =
75-
"For 'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'"
77+
"For 'http'/'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'"
7678
const mixedFieldsErrorMessage =
77-
"Cannot mix 'stdio' and ('sse' or 'streamable-http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'streamable-http' use 'url' and 'headers'"
79+
"Cannot mix 'stdio' and ('sse' or 'http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'http' use 'url' and 'headers'"
7880
const missingFieldsErrorMessage =
79-
"Server configuration must include either 'command' (for stdio) or 'url' (for sse/streamable-http) and a corresponding 'type' if 'url' is used."
81+
"Server configuration must include either 'command' (for stdio) or 'url' (for sse/http) and a corresponding 'type' if 'url' is used."
8082

8183
// Helper function to create a refined schema with better error messages
8284
const createServerTypeSchema = () => {
@@ -112,7 +114,24 @@ const createServerTypeSchema = () => {
112114
type: "sse" as const,
113115
}))
114116
.refine((data) => data.type === undefined || data.type === "sse", { message: typeErrorMessage }),
115-
// StreamableHTTP config (has url field)
117+
// HTTP config (preferred, has url field)
118+
BaseConfigSchema.extend({
119+
type: z.enum(["http"]).optional(),
120+
url: z.string().url("URL must be a valid URL format"),
121+
headers: z.record(z.string()).optional(),
122+
// Ensure no stdio fields are present
123+
command: z.undefined().optional(),
124+
args: z.undefined().optional(),
125+
env: z.undefined().optional(),
126+
})
127+
.transform((data) => ({
128+
...data,
129+
type: "http" as const,
130+
}))
131+
.refine((data) => data.type === undefined || data.type === "http", {
132+
message: typeErrorMessage,
133+
}),
134+
// StreamableHTTP config (deprecated but supported for backward compatibility)
116135
BaseConfigSchema.extend({
117136
type: z.enum(["streamable-http"]).optional(),
118137
url: z.string().url("URL must be a valid URL format"),
@@ -124,9 +143,11 @@ const createServerTypeSchema = () => {
124143
})
125144
.transform((data) => ({
126145
...data,
127-
type: "streamable-http" as const,
146+
// Normalize streamable-http to http internally
147+
type: "http" as const,
128148
}))
129-
.refine((data) => data.type === undefined || data.type === "streamable-http", {
149+
.refine((data) => true, {
150+
// Always pass since we're transforming to http
130151
message: typeErrorMessage,
131152
}),
132153
])
@@ -194,7 +215,7 @@ export class McpHub {
194215
private validateServerConfig(config: any, serverName?: string): z.infer<typeof ServerConfigSchema> {
195216
// Detect configuration issues before validation
196217
const hasStdioFields = config.command !== undefined
197-
const hasUrlFields = config.url !== undefined // Covers sse and streamable-http
218+
const hasUrlFields = config.url !== undefined // Covers sse, http, and streamable-http
198219

199220
// Check for mixed fields (stdio vs url-based)
200221
if (hasStdioFields && hasUrlFields) {
@@ -208,11 +229,13 @@ export class McpHub {
208229

209230
// For url-based configs, type must be provided by the user
210231
if (hasUrlFields && !config.type) {
211-
throw new Error("Configuration with 'url' must explicitly specify 'type' as 'sse' or 'streamable-http'.")
232+
throw new Error(
233+
"Configuration with 'url' must explicitly specify 'type' as 'sse' or 'http' (preferred) / 'streamable-http' (deprecated).",
234+
)
212235
}
213236

214237
// Validate type if provided
215-
if (config.type && !["stdio", "sse", "streamable-http"].includes(config.type)) {
238+
if (config.type && !["stdio", "sse", "http", "streamable-http"].includes(config.type)) {
216239
throw new Error(typeErrorMessage)
217240
}
218241

@@ -223,6 +246,9 @@ export class McpHub {
223246
if (config.type === "sse" && !hasUrlFields) {
224247
throw new Error(sseFieldsErrorMessage)
225248
}
249+
if (config.type === "http" && !hasUrlFields) {
250+
throw new Error(httpFieldsErrorMessage)
251+
}
226252
if (config.type === "streamable-http" && !hasUrlFields) {
227253
throw new Error(streamableHttpFieldsErrorMessage)
228254
}
@@ -733,17 +759,17 @@ export class McpHub {
733759
} else {
734760
console.error(`No stderr stream for ${name}`)
735761
}
736-
} else if (configInjected.type === "streamable-http") {
737-
// Streamable HTTP connection
762+
} else if (configInjected.type === "http" || configInjected.type === "streamable-http") {
763+
// HTTP connection (streamable-http is mapped to http for backward compatibility)
738764
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
739765
requestInit: {
740766
headers: configInjected.headers,
741767
},
742768
})
743769

744-
// Set up Streamable HTTP specific error handling
770+
// Set up HTTP specific error handling
745771
transport.onerror = async (error) => {
746-
console.error(`Transport error for "${name}" (streamable-http):`, error)
772+
console.error(`Transport error for "${name}" (http):`, error)
747773
const connection = this.findConnection(name, source)
748774
if (connection) {
749775
connection.server.status = "disconnected"

‎src/services/mcp/__tests__/McpHub.spec.ts‎

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2147,4 +2147,217 @@ describe("McpHub", () => {
21472147
)
21482148
})
21492149
})
2150+
2151+
describe("HTTP type standardization", () => {
2152+
let StdioClientTransport: ReturnType<typeof vi.fn>
2153+
let Client: ReturnType<typeof vi.fn>
2154+
let mockTransport: any
2155+
let mockClient: any
2156+
2157+
beforeEach(async () => {
2158+
// Reset mocks
2159+
vi.clearAllMocks()
2160+
2161+
// Get references to the mocked constructors
2162+
const stdioModule = await import("@modelcontextprotocol/sdk/client/stdio.js")
2163+
const clientModule = await import("@modelcontextprotocol/sdk/client/index.js")
2164+
StdioClientTransport = stdioModule.StdioClientTransport as ReturnType<typeof vi.fn>
2165+
Client = clientModule.Client as ReturnType<typeof vi.fn>
2166+
2167+
// Setup mock transport
2168+
mockTransport = {
2169+
start: vi.fn().mockResolvedValue(undefined),
2170+
close: vi.fn().mockResolvedValue(undefined),
2171+
stderr: {
2172+
on: vi.fn(),
2173+
},
2174+
onerror: null,
2175+
onclose: null,
2176+
}
2177+
2178+
// Setup mock client
2179+
mockClient = {
2180+
connect: vi.fn().mockResolvedValue(undefined),
2181+
close: vi.fn().mockResolvedValue(undefined),
2182+
getInstructions: vi.fn().mockReturnValue("test instructions"),
2183+
request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }),
2184+
}
2185+
2186+
Client.mockImplementation(() => mockClient)
2187+
})
2188+
2189+
it("should accept 'http' as the primary type", async () => {
2190+
// Mock the config file read with 'http' type
2191+
vi.mocked(fs.readFile).mockResolvedValue(
2192+
JSON.stringify({
2193+
mcpServers: {
2194+
"http-test-server": {
2195+
type: "http",
2196+
url: "https://api.example.com/mcp",
2197+
headers: {
2198+
Authorization: "Bearer token",
2199+
},
2200+
},
2201+
},
2202+
}),
2203+
)
2204+
2205+
// Create McpHub and let it initialize
2206+
const mcpHub = new McpHub(mockProvider as ClineProvider)
2207+
await new Promise((resolve) => setTimeout(resolve, 100))
2208+
2209+
// Find the connection
2210+
const connection = mcpHub.connections.find((conn) => conn.server.name === "http-test-server")
2211+
expect(connection).toBeDefined()
2212+
2213+
// Type guard check - should be connected
2214+
if (connection && connection.type === "connected") {
2215+
expect(connection.client).toBeDefined()
2216+
expect(connection.transport).toBeDefined()
2217+
expect(connection.server.status).toBe("connected")
2218+
} else {
2219+
throw new Error("Connection should be of type 'connected'")
2220+
}
2221+
})
2222+
2223+
it("should accept 'streamable-http' for backward compatibility and map it to 'http'", async () => {
2224+
// Mock the config file read with 'streamable-http' type (backward compatibility)
2225+
vi.mocked(fs.readFile).mockResolvedValue(
2226+
JSON.stringify({
2227+
mcpServers: {
2228+
"legacy-http-server": {
2229+
type: "streamable-http",
2230+
url: "https://api.example.com/mcp",
2231+
headers: {
2232+
Authorization: "Bearer token",
2233+
},
2234+
},
2235+
},
2236+
}),
2237+
)
2238+
2239+
// Create McpHub and let it initialize
2240+
const mcpHub = new McpHub(mockProvider as ClineProvider)
2241+
await new Promise((resolve) => setTimeout(resolve, 100))
2242+
2243+
// Find the connection
2244+
const connection = mcpHub.connections.find((conn) => conn.server.name === "legacy-http-server")
2245+
expect(connection).toBeDefined()
2246+
2247+
// Type guard check - should be connected
2248+
if (connection && connection.type === "connected") {
2249+
expect(connection.client).toBeDefined()
2250+
expect(connection.transport).toBeDefined()
2251+
expect(connection.server.status).toBe("connected")
2252+
} else {
2253+
throw new Error("Connection should be of type 'connected'")
2254+
}
2255+
})
2256+
2257+
it("should validate 'http' type configurations", () => {
2258+
// Test valid http config
2259+
const validHttpConfig = {
2260+
type: "http",
2261+
url: "https://api.example.com/mcp",
2262+
headers: {
2263+
Authorization: "Bearer token",
2264+
},
2265+
timeout: 60,
2266+
}
2267+
expect(() => ServerConfigSchema.parse(validHttpConfig)).not.toThrow()
2268+
2269+
// Test invalid http config (missing url)
2270+
const invalidHttpConfig = {
2271+
type: "http",
2272+
headers: {
2273+
Authorization: "Bearer token",
2274+
},
2275+
}
2276+
expect(() => ServerConfigSchema.parse(invalidHttpConfig)).toThrow()
2277+
})
2278+
2279+
it("should validate 'streamable-http' type configurations for backward compatibility", () => {
2280+
// Test valid streamable-http config
2281+
const validStreamableHttpConfig = {
2282+
type: "streamable-http",
2283+
url: "https://api.example.com/mcp",
2284+
headers: {
2285+
Authorization: "Bearer token",
2286+
},
2287+
timeout: 60,
2288+
}
2289+
expect(() => ServerConfigSchema.parse(validStreamableHttpConfig)).not.toThrow()
2290+
2291+
// Test invalid streamable-http config (missing url)
2292+
const invalidStreamableHttpConfig = {
2293+
type: "streamable-http",
2294+
headers: {
2295+
Authorization: "Bearer token",
2296+
},
2297+
}
2298+
expect(() => ServerConfigSchema.parse(invalidStreamableHttpConfig)).toThrow()
2299+
})
2300+
2301+
it("should normalize 'streamable-http' to 'http' internally", () => {
2302+
const streamableHttpConfig = {
2303+
type: "streamable-http",
2304+
url: "https://api.example.com/mcp",
2305+
}
2306+
2307+
// Parse the config through the schema
2308+
const parsed = ServerConfigSchema.parse(streamableHttpConfig)
2309+
2310+
// Should be normalized to 'http'
2311+
expect(parsed.type).toBe("http")
2312+
})
2313+
2314+
it("should update error messages to reflect new 'http' type", async () => {
2315+
// Create McpHub instance
2316+
const mcpHub = new McpHub(mockProvider as ClineProvider)
2317+
2318+
// Test invalid type error
2319+
const invalidConfig = {
2320+
type: "invalid-type",
2321+
url: "https://api.example.com/mcp",
2322+
}
2323+
2324+
// Try to validate the config
2325+
expect(() => mcpHub["validateServerConfig"](invalidConfig)).toThrow(
2326+
"Server type must be 'stdio', 'sse', or 'http' (preferred) / 'streamable-http' (deprecated)",
2327+
)
2328+
})
2329+
2330+
it("should require explicit type for url-based configs", async () => {
2331+
// Create McpHub instance
2332+
const mcpHub = new McpHub(mockProvider as ClineProvider)
2333+
2334+
// Test config with url but no type
2335+
const configWithoutType = {
2336+
url: "https://api.example.com/mcp",
2337+
}
2338+
2339+
// Try to validate the config
2340+
expect(() => mcpHub["validateServerConfig"](configWithoutType)).toThrow(
2341+
"Configuration with 'url' must explicitly specify 'type' as 'sse' or 'http' (preferred) / 'streamable-http' (deprecated).",
2342+
)
2343+
})
2344+
2345+
it("should provide appropriate error for http configs missing url", async () => {
2346+
// Create McpHub instance
2347+
const mcpHub = new McpHub(mockProvider as ClineProvider)
2348+
2349+
// Test http config without url
2350+
const httpConfigWithoutUrl = {
2351+
type: "http",
2352+
headers: {
2353+
Authorization: "Bearer token",
2354+
},
2355+
}
2356+
2357+
// Try to validate the config
2358+
expect(() => mcpHub["validateServerConfig"](httpConfigWithoutUrl)).toThrow(
2359+
"For 'http' type servers, you must provide a 'url' field and can optionally include 'headers'",
2360+
)
2361+
})
2362+
})
21502363
})

0 commit comments

Comments
 (0)