-
Notifications
You must be signed in to change notification settings - Fork 259
feat(js/plugins/mcp): Refactor to a client manager structure, with dynamic tool calling. #2884
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: launch/mcp-redux
Are you sure you want to change the base?
Conversation
ssbushi
commented
May 7, 2025
•
edited
Loading
edited
- Modified the current McpClient class to a Client manager class
- GenkitMcpClient is now a dedicated single client-server connection.
- Tools are now fetched dynamically to support adding/removing/disabling clients in the manager.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
One more high-level piece of food for thought: in the client manager context, I think we probably want to be very robust to misconfiguration errors. It's possible people will use this for dynamic, user-configured tools and when that happens we should recover gracefully as best we can.
So maybe in the client manager there should be an {error?: {message, status, details}}
and if there's an error trying to connect to the server it is put in a disabled state, an error-level log is emitted, and the error is also saved with the server.
Oh also I think we should put this work into a launch branch - I created |
I'll work on this in a new commit, updating the PR to reflect the changes so far. |
js/plugins/mcp/README.md
Outdated
|
||
All MCP actions are namespaced under the name you supply, so a client called `filesystem` will register tools such as `filesystem/read_file`. | ||
<!-- TODO: Note about MCP prompts. --> |
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.
How should we approach prompts with the newer dynamic API? Tools and resources can be wrapped as dynamic tools, but prompts should be treated differently, I suppose....
We don't have dynamic prompt support today and I'm wondering how to expose them in this new iteration of the plugin.
Should we also provide the user to install the MCP client as a plugin like we did for mcpClient({..})
and provide prompts there?
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.
Let's think short-term / long-term.
Short-term, maybe we should essentially follow the API of ExecutablePrompt
but not actually register them? So basically:
const mcpPrompt = mcpManager.prompt('some-prompt');
await mcpPrompt(input, options);
await mcpPrompt.stream(input, options);
await mcpPrompt.render(input, options);
and under the hood that's just rendering the prompt to MessageData[]
and then either executing it, streaming it, or returning the rendered options?
Long-term I think we should have an MCP manager able to be used as a plugin with full dynamic registry integration.
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 like the idea of bespoke prompt references. One thing I'd suggest for the short term is to use a serverName qualifer to narrow it down:
const mcpPrompt = mcpManager.prompt({serverName: 'my-mcp-server', prompt: 'some-prompt'});
We can add support for dynamic look-ups as we go forward and handle server resolution when there is not conflicts.
Long-term I think we should have an MCP manager able to be used as a plugin with full dynamic registry integration.
Agreed. We need to discuss more on how to support dynamic registrations via plugins.
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 mcpManager.prompt('server-name', 'some-prompt')
?
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.
Added prompt support. I had to leave the following as unimplemented:
ExecutablePrompt.render
-- no way to render the prompt in MCP prompts (AFAIK)ExecutablePrompt.asTool
-- @pavelgj said this is unimplemented in Genkit today.
Not sure if my implementation is accurate. LMK if I made a blunder here.
Co-authored-by: Pavel Jbanov <pavelj@google.com>
Co-authored-by: Pavel Jbanov <pavelj@google.com>
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.
This is mostly looking really good, thanks for all the work! I think probably the most important thing after this will be adding a good suite of tests to give us more confidence that it's robust and ready for an upgrade.
// Each key (e.g., 'fs', 'git') becomes a namespace for the server's tools. | ||
fs: { | ||
command: 'npx', | ||
args: ['-y', '@modelcontextprotocol/server-everything', ...ALLOWED_DIRS], |
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 think you probably meant something other than server-everything
here.
- **`url`**: (string) The URL of a remote server to connect to using the SSE MCP transport. | ||
- **`serverWebsocketUrl`**: (string) The URL of a remote server to connect to using the WebSocket MCP transport. | ||
- **`transport`**: An existing MCP transport object for connecting to the server. | ||
|
||
Most MCP servers are built to run as spawned processes on the same machine using the `stdio` transport. When you supply the `serverProcess` option, you are specifying the command, arguments, and environment variables for spawning the server as a subprocess. |
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.
No serverProcess
option anymore.
### [Deprecated] `mcpClient()` | ||
|
||
Note: This method is deprecated, please use `createMcpClient` or `createMcpManager` instead. | ||
|
||
For simpler scenarios involving a single MCP server, or for backward compatibility, the legacy `mcpClient()` function is still available: | ||
|
||
### Using MCP Actions | ||
```ts | ||
import { mcpClient } from 'genkitx-mcp/legacy'; // Note the import path | ||
|
||
const legacyFilesystemClient = mcpClient({ | ||
name: 'filesystemLegacy', | ||
serverProcess: { /* ... */ }, | ||
}); | ||
``` | ||
This function takes similar options to a single server configuration within `createMcpManager` (e.g., `name`, `version`, `serverProcess`, `rawToolResponses`) and directly returns a Genkit plugin for that single client. It is recommended to use `createMcpManager` for new projects. |
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.
We can leave this out - keep the deprecated method around but it doesn't need to be in the README.
|
||
The legacy `mcpServer()` function is deprecated; please use `createMcpServer()` instead. |
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.
The legacy `mcpServer()` function is deprecated; please use `createMcpServer()` instead. |
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.
Thinking out loud: it would be great to build up some tests for this library, and maybe the easiest way we could do that is creating a custom transport that would let us have a "mock" MCP server.
) { | ||
this._clientStates[serverName] = { error }; | ||
logger.error( | ||
`An error has occured while managing your MCP client '${serverName}'. The client may be disabled to avoid further issues. Please resolve the issue and reenable the client '${serverName}' to continue using its resources.` |
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.
Probably a warn
level log here.
} | ||
|
||
/** | ||
* Lookup all tools available in the server and register each as a Genkit tool. |
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.
* Lookup all tools available in the server and register each as a Genkit tool. | |
* Lookup all prompts available in the server and register each as a Genkit prompt. |
|
||
function listResourcesTool( | ||
ai: Genkit, | ||
client: any, // Use 'any' or let TS infer; removing specific type import |
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.
Why any? If it's a circular dependency issue, you can do import type
without triggering circ dependencies.
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 think resources need to be handled a bit differently in the Manager. I don't want there to be a tool per client to list and read resources - there should be a single tool at the manager level (perhaps with optional list of server names to include) for listing / reading across all connected servers.
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.
Let's go ahead and change the package name to @genkit-ai/mcp
so we don't forget to do it later. Should be safe since we're merging to a launch branch.