-
Notifications
You must be signed in to change notification settings - Fork 346
Support tools varied by user #160
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
Comments
Would need to think about this. I might have some time to look at implementing this during the weekend. |
@SamMorrowDrums would this work for your use-case? #179 |
I will look into details shortly, but that looks really perfect from my cursory glance. Definitely the right direction of travel. Thanks so much for looking into this and thinking about the problem thoroughly! Can't wait to try it. |
One thing I will be looking at for our purposes is if we can't guarantee a user hitting the same server more than once what we do instead. If that's the case we'll probably use an external session store and then rehydrate the session in mcp-go for each request (I believe there are two requests required even for streamable http). To handle deployment, even if sessions are sticky to a host we still could end up with a user hitting two servers during a deploy so we are still planning on keeping the servers themselves stateless as far as is possible. Long running tasks with server events could get dropped with a server switch, but things like tools should survive. Just sharing this for a bit more around our thinking. |
FWIW it looks like that would all work with the code you shared. We will have to think about registering and unregistering sessions lifecycle, but I believe we should be able to work that out well. Only thing I'd be worried about is carefully avoiding memory leak, so the cleanup is definitely required. I'm off until Wednesday, but I want to give this a try as soon as I can. |
Ok great. If you would try it out and let me know if it works in general, I can add it to the next release I think. |
@SamMorrowDrums wondering if you've had a chance to try this out yet? |
Sorry not quite, tomorrow I will hopefully get a chance to build a proof of concept with it. |
I am experimenting with dynamically changing the tool offering, and for a local server with one user it works really well but for a remote server with many users we ideally support this.
One option is all tools are enabled, and we filter them with middleware before returning the tools list response. Say for example because they cannot use some due to oauth capabilities of their token.
Tool updates also should have a way to target individual sessions and not be global, otherwise this doesn't work either (we don't want one user changing the global tool state for all)
Another option is a user session so we can set information on the session stored client side so that we can be stateless on the server side, but support dynamic capabilities. This would require spec updates of course.
One way or another I think we need this at GitHub as we have so many tools.
Some more context here: github/github-mcp-server#275
Thanks for taking a look at this, as you know I will be willing to contribute to help if I can be useful. Just wanted to document this desire.
The text was updated successfully, but these errors were encountered: