chore: introduces VSCodeMCPConnectionManager to talk to contributed MCP server MCP-131 #1111
Conversation
fbb8e1e to
ca40afc
Compare
| LogPayload, | ||
| UserConfig, | ||
| ConnectionManagerFactoryFn, | ||
| } from '@himanshusinghs/mongodb-mcp-server'; |
There was a problem hiding this comment.
This is temporary until we release the mongodb-mcp-server with injectable connection manager work.
| try { | ||
| const serviceProvider = await NodeDriverServiceProvider.connect( | ||
| connectParams.connectionString, | ||
| connectParams.connectOptions, |
There was a problem hiding this comment.
There's one thing that is bothering me here about connectOptions and that's the appName attribute. In MCP server, we go a stretch to ensure the appName but we don't do that here. That means the appName will be how VSCode extension sets it, in here.
Should we instead set the appName to something like vscode-mcp-xxx?
There was a problem hiding this comment.
This is relevant for analytics
There was a problem hiding this comment.
I went through the implementation and it seems mostly okay - I have mostly design suggestions.
One thing that might be curious to test out in the real world is how the LLM behaves when we switch connections - I've seen sometimes LLMs will pull certain results out of the context instead of invoking the tool again. For example, if you ask "what are my databases", then switch the connection, then ask again, the model might decide to just regurgitate the previous response until you actively ask it to fetch them again. We should see if that's a real problem and if it is, explore if exposing a resource for the current connection, that we update and send update notifications for, might solve it.
Until we send out another release containing the work from injectable connection manager, we will be using the privately scoped mongodb-mcp-server.
e2fcdf9 to
c3f72cb
Compare
Yea I have seen this as well! I don't think having resources are going to help in this case because resources are application controlled. Until user decide to put resource in context, LLM won't know if something is happening with the resources. The resource notifications are limited to the MCP client and wouldn't expect MCP client to pass the updated resource (even notification) to LLM without explicit user action. |
|
I would like to correct myself. From the docs on Resources, it seems applications are allowed to decide if they want to automatically include the resource in the context. In that case, we should certainly try if having resource changes the situation by any means. I will set up a follow up for this, if that sounds alright? |
| super(); | ||
| } | ||
|
|
||
| connect(): Promise<AnyConnectionState> { |
There was a problem hiding this comment.
nit: I personally prefer to use throw. Also, if possible, don't use an array to just join it, this is something that can be solved by using backticks.
There was a problem hiding this comment.
We shouldn't use throw here because the throw will happen synchronously and might break the call site depending on how the method is called. For example someone could be doing.
const connectPromise = manger.connect();
Promise.race([connectPromise, timeout])
Here Promise.race will never happen because connectPromise would have already thrown.
About array.join, I am using to make the text more readable and not appear in single line.
There was a problem hiding this comment.
In general, if a function returns a promise, it should be marked as async, and then throwing will have the same effect than returning a rejected promise. I don't see why we need to make this function sync but return a promise, I feel we are just not being consistent with the typical conventions.
About using array.join, I think it's less clear than backticks, but if you feel strong about it I'm fine keeping it.
There was a problem hiding this comment.
But there is no point in marking a function async when it has no await expression, not to forget that this is also something the eslint rule in our repo discourages. If I were to mark the function as async and then disable the eslint rule then that would be diverging from the conventions in the repo.
About array.join, I don't think its fun to scroll the single line horizontally in the editor (which is what we'll get with the entire sentence being a single string) to read the entire sentence. My intention here was to make the error message glanceable in the first view which I think is served better when the sentence is broken apart.
| ); | ||
| } | ||
|
|
||
| async connectToVSCodeConnection( |
There was a problem hiding this comment.
That's one of the things I was warning about yesterday during our meeting. We are just inheriting here to share a bit of logic, but most of it is literally a clone of what we have in the original ConnectionManager.
This is not a good design:
- We have an abstract class that is used to share logic, but not contract.
- Logic is duplicated, so if we add a new state, we need to update both implementations.
There was a problem hiding this comment.
I don't see how this implementation is a clone of the original ConnectionManager, we are not doing anything that the original ConnectionManager does. For connect, we literally take connectionOptions and simply use them to create a service provider instance, there are no connection preparation, there are no user flows involved and no explicit oidc handling which is what original connection manager does.
There was a problem hiding this comment.
The implementation uses the Node.js Provider the same way we do in the MCPConnectionManager but it forgets about some events and introduces a few bugs:
- connection-requested is not emitted.
- It discards any proxy support, which we still need to maintain. This is not guaranteed by the vscode configuration right now.
When you add connection-requested, it will literally do the same without our OIDC custom options, that are fine to skip in this scenario.
And, exposing a new function that would do the same as connect, it's literally breaking the contract of the ConnectionManager itself because we just want to reuse a few methods (basically, just this.changeState). This is far from being advisable in OOP, because inheritance is not a mixin, which is what you kind of want here and how you are treating the base ConnectionManager class.
There was a problem hiding this comment.
The implementation uses the Node.js Provider the same way we do in the MCPConnectionManager but it forgets about some events and introduces a few bugs:
That's not true. The implementation uses the Node.js provider without preparing the connect options, like we do in original implementation. And that's because we're using an already connected connection's connect options to establish a connection without re-invoking any user flows again. This is exactly how language server, playground workers establish connection across different processes without re-invoking the user flows again. If there are any bugs then that should be true for both language server and playground workers as well but that's not the case. And I have tested this setup for OIDC connection and it works as expected. I plan to add e2e for this but that would require some extra time and that's why will happen in a follow up.
connection-requested is not emitted.
connection-requested is not emitted because this state never comes through for MCP server. MCP server is notified of active connection changed, only when the extension has gone through the user flows and actually connected to the connection or disconnected entirely. Once notified of the active connection changed, MCP server literally takes the connection options of the already connected connection and use them to connect, thereby avoiding all the user flows that it would otherwise have to go through. For this reason, connection-requested state is not even relevant for this implementation.
It discards any proxy support, which we still need to maintain. This is not guaranteed by the vscode configuration right now.
The implementation is literally the same as used by playground worker and language server to establish the connection to a connection selected in VSCode extension's sidepanel. We support all the connection types provided by the connection form in VSCode which is basically the same as Compass. So we are not discarding any proxy support.
And, exposing a new function that would do the same as connect, it's literally breaking the contract of the ConnectionManager itself because we just want to reuse a few methods (basically, just this.changeState)
This was not at all the intention of having the new function. The intention instead was that this new function would only be known to VSCode world and not to the MCP world and that's how we are defining clear boundaries between an internal connection manager where MCP server sees and can invoke connect and external connection manager where MCP server can only see connect (which throws) and not the other connect method. About the connect throwing in external connection manager, that's also intentional because we want MCP server to be notified that the invoke was not expected and instead that the connection management is handled by an external entity (in this case by VSCode with perfect guidelines on how to connect)
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes