-
Notifications
You must be signed in to change notification settings - Fork 296
feat: rename server property to mcp in MCP-related classes for improved developer experience #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| --- | ||
| "@cloudflare/agents": patch | ||
| --- | ||
|
|
||
| Rename `server` property to `mcp` in MCP-related classes for better developer experience | ||
|
|
||
| ## Changes | ||
|
|
||
| - **BREAKING**: Renamed `server` property to `mcp` in `McpAgent` class and related examples | ||
| - **BREAKING**: Renamed `mcp` property to `mcpClientManager` in `Agent` class to avoid naming conflicts | ||
| - Added backward compatibility support for `server` property in `McpAgent` with deprecation warning | ||
| - Updated all MCP examples to use the new `mcp` property naming convention | ||
| - Improved property naming consistency across the MCP implementation | ||
|
|
||
| ## Migration | ||
|
|
||
| If you're using the `server` property in your `McpAgent` implementations, update your code: | ||
|
|
||
| ```ts | ||
| // Before | ||
| export class MyMcpAgent extends McpAgent { | ||
| server = new McpServer({...}); | ||
| } | ||
|
|
||
| // After | ||
| export class MyMcpAgent extends McpAgent { | ||
| mcp = new McpServer({...}); | ||
| } | ||
| ``` | ||
|
|
||
| The `server` property is still supported for backward compatibility but will be removed in a future version. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -291,7 +291,10 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
| private _ParentClass: typeof Agent<Env, State> = | ||
| Object.getPrototypeOf(this).constructor; | ||
|
|
||
| mcp: MCPClientManager = new MCPClientManager(this._ParentClass.name, "0.0.1"); | ||
| mcpClientManager: MCPClientManager = new MCPClientManager( | ||
| this._ParentClass.name, | ||
| "0.0.1" | ||
| ); | ||
|
||
|
|
||
| /** | ||
| * Initial state for the Agent | ||
|
|
@@ -441,8 +444,8 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
| return agentContext.run( | ||
| { agent: this, connection: undefined, request, email: undefined }, | ||
| async () => { | ||
| if (this.mcp.isCallbackRequest(request)) { | ||
| await this.mcp.handleCallbackRequest(request); | ||
| if (this.mcpClientManager.isCallbackRequest(request)) { | ||
| await this.mcpClientManager.handleCallbackRequest(request); | ||
|
|
||
| // after the MCP connection handshake, we can send updated mcp state | ||
| this.broadcast( | ||
|
|
@@ -1483,7 +1486,7 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
| }; | ||
| } | ||
|
|
||
| const { id, authUrl, clientId } = await this.mcp.connect(url, { | ||
| const { id, authUrl, clientId } = await this.mcpClientManager.connect(url, { | ||
| client: options?.client, | ||
| reconnect, | ||
| transport: { | ||
|
|
@@ -1500,7 +1503,7 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
| } | ||
|
|
||
| async removeMcpServer(id: string) { | ||
| this.mcp.closeConnection(id); | ||
| this.mcpClientManager.closeConnection(id); | ||
| this.sql` | ||
| DELETE FROM cf_agents_mcp_servers WHERE id = ${id}; | ||
| `; | ||
|
|
@@ -1514,10 +1517,10 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
|
|
||
| getMcpServers(): MCPServersState { | ||
| const mcpState: MCPServersState = { | ||
| prompts: this.mcp.listPrompts(), | ||
| resources: this.mcp.listResources(), | ||
| prompts: this.mcpClientManager.listPrompts(), | ||
| resources: this.mcpClientManager.listResources(), | ||
| servers: {}, | ||
| tools: this.mcp.listTools() | ||
| tools: this.mcpClientManager.listTools() | ||
| }; | ||
|
|
||
| const servers = this.sql<MCPServerRow>` | ||
|
|
@@ -1526,7 +1529,7 @@ export class Agent<Env = typeof env, State = unknown> extends Server<Env> { | |
|
|
||
| if (servers && Array.isArray(servers) && servers.length > 0) { | ||
| for (const server of servers) { | ||
| const serverConn = this.mcp.mcpConnections[server.id]; | ||
| const serverConn = this.mcpClientManager.mcpConnections[server.id]; | ||
| mcpState.servers[server.id] = { | ||
| auth_url: server.auth_url, | ||
| capabilities: serverConn?.serverCapabilities ?? null, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,8 +210,24 @@ export abstract class McpAgent< | |
| */ | ||
| private _agent: Agent<Env, State>; | ||
|
|
||
| get mcp() { | ||
| return this._agent.mcp; | ||
| get mcpClientManager() { | ||
| return this._agent.mcpClientManager; | ||
| } | ||
|
Comment on lines
+213
to
+215
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bit is technically breaking for anyone who's using the client manager, but I don't think anyone is and it's not documented.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very happy with this change. I think this is super confusing at the moment. We should keep the |
||
|
|
||
| /** | ||
| * Getter that returns the MCP server implementation. | ||
| * Supports both 'server' and 'mcp' properties for better developer experience. | ||
| * If 'mcp' property is set, use that; otherwise fall back to 'server' property. | ||
| */ | ||
| get mcpServer(): MaybePromise<McpServer | Server> { | ||
| const server = this.mcp ?? this.server; | ||
| if (!server) { | ||
| throw new Error( | ||
| 'Neither the `mcp` nor `server` property is set. Please set "mcp".' | ||
| ); | ||
| } | ||
|
|
||
| return server; | ||
|
Comment on lines
+222
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this adds to the confusion right now. IMO mcpServer should be the new name for |
||
| } | ||
|
|
||
| protected constructor(ctx: DurableObjectState, env: Env) { | ||
|
|
@@ -333,7 +349,7 @@ export abstract class McpAgent< | |
| )) as TransportType; | ||
| await this._init(this.props); | ||
|
|
||
| const server = await this.server; | ||
| const server = await this.mcpServer; | ||
|
|
||
| // Connect to the MCP server | ||
| if (this._transportType === "sse") { | ||
|
|
@@ -350,8 +366,17 @@ export abstract class McpAgent< | |
|
|
||
| /** | ||
| * McpAgent API | ||
| * @deprecated Use the `mcp` property instead for better developer experience. | ||
| */ | ||
| server?: MaybePromise<McpServer | Server>; | ||
| /** | ||
| * This is only set as optional for backward compatibility in case you're | ||
| * using the legacy `server` property. It's recommended to use `mcp`. | ||
| * | ||
| * In the future, we'll make this a required abstract property to implement | ||
| * and remove the `server` property. | ||
| */ | ||
| abstract server: MaybePromise<McpServer | Server>; | ||
| mcp?: MaybePromise<McpServer | Server>; | ||
|
Comment on lines
+369
to
+379
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the primary focus of this PR
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you be open to this changing to mcpServer() ? |
||
| props!: Props; | ||
| initRun = false; | ||
|
|
||
|
|
@@ -430,7 +455,7 @@ export abstract class McpAgent< | |
| // This is not the path that the user requested, but the path that the worker | ||
| // generated. We'll use this path to determine which transport to use. | ||
| const path = url.pathname; | ||
| const server = await this.server; | ||
| const server = await this.mcpServer; | ||
|
|
||
| switch (path) { | ||
| case "/sse": { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
mcpcould be a getter that returnsserver? That way it would be totally backward compatible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would allow me to do this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that myself without having to change anything in
agents. That's actually a reasonable workaround if they decide this is more trouble than it's worth. Thanks for the idea!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually I can't do that because
mcpis already set to themcpClientManager🙁