Skip to content
Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
class StreamableHTTPClientTransport {
constructor(url, options = {}) {
this.url = url
this.options = options
this.onerror = null
this.onclose = null
this.connect = jest.fn().mockResolvedValue()
this.close = jest.fn().mockResolvedValue()
this.start = jest.fn().mockResolvedValue()
}
}

module.exports = {
StreamableHTTPClientTransport,
}
24 changes: 23 additions & 1 deletion src/core/tools/__tests__/writeToFileTool.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,25 @@ describe("writeToFileTool", () => {
finalContent: "final content",
}),
scrollToFirstDiff: jest.fn(),
pushToolWriteResult: jest.fn().mockImplementation(async function (
this: any,
task: any,
cwd: string,
isNewFile: boolean,
) {
// Simulate the behavior of pushToolWriteResult
if (this.userEdits) {
await task.say(
"user_feedback_diff",
JSON.stringify({
tool: isNewFile ? "newFileCreated" : "editedExistingFile",
path: "test/path.txt",
diff: this.userEdits,
}),
)
}
return "Tool result message"
}),
}
mockCline.api = {
getModel: jest.fn().mockReturnValue({ id: "claude-3" }),
Expand Down Expand Up @@ -343,11 +362,14 @@ describe("writeToFileTool", () => {
})

it("reports user edits with diff feedback", async () => {
const userEditsValue = "- old line\n+ new line"
mockCline.diffViewProvider.saveChanges.mockResolvedValue({
newProblemsMessage: " with warnings",
userEdits: "- old line\n+ new line",
userEdits: userEditsValue,
finalContent: "modified content",
})
// Manually set the property on the mock instance because the original saveChanges is not called
mockCline.diffViewProvider.userEdits = userEditsValue

await executeWriteFileTool({}, { fileExists: true })

Expand Down
104 changes: 83 additions & 21 deletions src/services/mcp/McpHub.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Client } from "@modelcontextprotocol/sdk/client/index.js"
import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"
import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
import ReconnectingEventSource from "reconnecting-eventsource"
import {
CallToolResultSchema,
Expand Down Expand Up @@ -35,7 +36,7 @@ import { injectEnv } from "../../utils/config"
export type McpConnection = {
server: McpServer
client: Client
transport: StdioClientTransport | SSEClientTransport
transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport
}

// Base configuration schema for common settings
Expand All @@ -47,14 +48,17 @@ const BaseConfigSchema = z.object({
})

// Custom error messages for better user feedback
const typeErrorMessage = "Server type must be either 'stdio' or 'sse'"
const typeErrorMessage = "Server type must be 'stdio', 'sse', or 'streamable-http'"
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 streamableHttpFieldsErrorMessage =
"For 'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'"
const mixedFieldsErrorMessage =
"Cannot mix 'stdio' and 'sse' fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse' use 'url' and 'headers'"
const missingFieldsErrorMessage = "Server configuration must include either 'command' (for stdio) or 'url' (for sse)"
"Cannot mix 'stdio' and ('sse' or 'streamable-http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'streamable-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."

// Helper function to create a refined schema with better error messages
const createServerTypeSchema = () => {
Expand Down Expand Up @@ -90,6 +94,23 @@ const createServerTypeSchema = () => {
type: "sse" as const,
}))
.refine((data) => data.type === undefined || data.type === "sse", { message: typeErrorMessage }),
// StreamableHTTP config (has url field)
BaseConfigSchema.extend({
type: z.enum(["streamable-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: "streamable-http" as const,
}))
.refine((data) => data.type === undefined || data.type === "streamable-http", {
message: typeErrorMessage,
}),
])
}

Expand Down Expand Up @@ -152,33 +173,43 @@ export class McpHub {
private validateServerConfig(config: any, serverName?: string): z.infer<typeof ServerConfigSchema> {
// Detect configuration issues before validation
const hasStdioFields = config.command !== undefined
const hasSseFields = config.url !== undefined
const hasUrlFields = config.url !== undefined // Covers sse and streamable-http

// Check for mixed fields
if (hasStdioFields && hasSseFields) {
// Check for mixed fields (stdio vs url-based)
if (hasStdioFields && hasUrlFields) {
throw new Error(mixedFieldsErrorMessage)
}

// Check if it's a stdio or SSE config and add type if missing
if (!config.type) {
if (hasStdioFields) {
config.type = "stdio"
} else if (hasSseFields) {
config.type = "sse"
} else {
throw new Error(missingFieldsErrorMessage)
}
} else if (config.type !== "stdio" && config.type !== "sse") {
// Infer type for stdio if not provided
if (!config.type && hasStdioFields) {
config.type = "stdio"
}

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

// Validate type if provided
if (config.type && !["stdio", "sse", "streamable-http"].includes(config.type)) {
throw new Error(typeErrorMessage)
}

// Check for type/field mismatch
if (config.type === "stdio" && !hasStdioFields) {
throw new Error(stdioFieldsErrorMessage)
}
if (config.type === "sse" && !hasSseFields) {
if (config.type === "sse" && !hasUrlFields) {
throw new Error(sseFieldsErrorMessage)
}
if (config.type === "streamable-http" && !hasUrlFields) {
throw new Error(streamableHttpFieldsErrorMessage)
}

// If neither command nor url is present (type alone is not enough)
if (!hasStdioFields && !hasUrlFields) {
throw new Error(missingFieldsErrorMessage)
}

// Validate the config against the schema
try {
Expand Down Expand Up @@ -441,7 +472,7 @@ export class McpHub {
},
)

let transport: StdioClientTransport | SSEClientTransport
let transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport

// Inject environment variables to the config
const configInjected = (await injectEnv(config)) as typeof config
Expand Down Expand Up @@ -506,8 +537,33 @@ export class McpHub {
} else {
console.error(`No stderr stream for ${name}`)
}
transport.start = async () => {} // No-op now, .connect() won't fail
} else {
} else if (configInjected.type === "streamable-http") {
// Streamable HTTP connection
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
requestInit: {
headers: configInjected.headers,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this hack in #4148 to make headers work with SSE - do you know if it's also needed for streamable? Hopefully not 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm good call, may not be necessary but wanted it match the existing patterns

Looking at it, EventSource can’t natively send arbitrary request headers from the browser, so #4148 injects them through the polyfill’s custom fetch hook. 

StreamableHTTPClientTransport opens its connection with fetch() under the hood. Because it relies on plain fetch, anything you put in options.requestInit.headers is forwarded automatically; no special shim is required.

The only gotcha is to pass the options object as the second (not third) argument to the transport constructor. Don’t think it poses any usability issues as is, didn’t in my testing but want to make everything clean and tidy can poke at it in the am

},
})

// Set up Streamable HTTP specific error handling
transport.onerror = async (error) => {
console.error(`Transport error for "${name}" (streamable-http):`, error)
const connection = this.findConnection(name, source)
if (connection) {
connection.server.status = "disconnected"
this.appendErrorMessage(connection, error instanceof Error ? error.message : `${error}`)
}
await this.notifyWebviewOfServerChanges()
}

transport.onclose = async () => {
const connection = this.findConnection(name, source)
if (connection) {
connection.server.status = "disconnected"
}
await this.notifyWebviewOfServerChanges()
}
} else if (configInjected.type === "sse") {
// SSE connection
const sseOptions = {
requestInit: {
Expand Down Expand Up @@ -542,7 +598,13 @@ export class McpHub {
}
await this.notifyWebviewOfServerChanges()
}
} else {
// Correctly placed "unsupported type" else block
// Should not happen if validateServerConfig is correct
throw new Error(`Unsupported MCP server type: ${(configInjected as any).type}`)
}
// transport.start assignment moved after all type-specific initializations
transport.start = async () => {} // No-op now, .connect() won't fail

const connection: McpConnection = {
server: {
Expand Down