mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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
This commit is contained in:
parent
f5d7ba1959
commit
fc11476910
2 changed files with 253 additions and 14 deletions
|
|
@ -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<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) {
|
||||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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'",
|
||||
)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue