- Notifications
You must be signed in to change notification settings - Fork1k
feat: add MCP tools for ChatGPT#19102
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ccc3a6a to3e42d7aCompareThere 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.
Looking good, just a few small code organization tweaks.
I'd prefer to add a new Register function to the server struct so that we can handle this one edge case instead of refactoring the whole thing.
For anyone looking at this PR: ChatGPT connectors are 'questionable' (ask for my opinion in Slack 😉) and adding a dedicated endpoint makes sense for the sake of dogfood and sanity.
coderd/mcp/mcp.go Outdated
| // RegisterTools registersall availableMCP tools with the server | ||
| func (s*Server)RegisterTools(client*codersdk.Client)error { | ||
| // RegisterTools registers MCP tools with the server | ||
| func (s*Server)RegisterTools(client*codersdk.Client,tools []toolsdk.GenericTool)error { |
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.
Could we leave the function as it was and add a new function calledRegisterChatGPTTools instead?
This would benefit us by allowing us to add exceptions for ChatGPT more easily on the server structure, and keeping the existing MCP endpoint and function calls as they are/will be.
(Also, we can then write a unit test and assert that a ChatGPT MCP server only has those two tools that we expect)
Uh oh!
There was an error while loading.Please reload this page.
coderd/mcp/mcp_e2e_test.go Outdated
| } | ||
| }() | ||
| ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong) |
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.
| ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong) | |
| ctx,cancel:=context.WithTimeout(t.Context(),testutil.WaitLong) |
coderd/mcp_http.go Outdated
| // mcpHTTPHandler creates the MCP HTTP transport handler | ||
| func (api*API)mcpHTTPHandler() http.Handler { | ||
| func (api*API)mcpHTTPHandler(tools []toolsdk.GenericTool) http.Handler { |
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 we could simplify this a bit by making it a boolean calledisChatGPT bool, then calling the appropriate Register function (either.RegisterTools() or.RegisterChatGPTTools()).
This also makes the handler less complex.
codersdk/toolsdk/chatgpt.go Outdated
| funcgetServerURL(depsDeps)string { | ||
| serverURLCopy:=*deps.coderClient.URL | ||
| serverURLCopy.Path="" | ||
| serverURLCopy.RawQuery="" | ||
| returnserverURLCopy.String() | ||
| } |
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 may want to add a new function toDeps.
Then we could justdeps.ServerURL() to get it.
codersdk/toolsdk/chatgpt.go Outdated
| funcsearchTemplates(ctx context.Context,depsDeps) ([]SearchResultItem,error) { | ||
| serverURL:=getServerURL(deps) | ||
| templates,err:=deps.coderClient.Templates(ctx, codersdk.TemplateFilter{}) |
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.
Can we use the SearchQuery field to parameterize this?
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.
I like the approach with the query param more than adding a new endpoint 👍
We might want to turn it into asource query parameter since that would enable us to composite multiple toolsets into client-specific ones, without "leaking" that abstraction.
| typeMCPToolsetstring | ||
| const ( | ||
| MCPToolsetStandardMCPToolset="standard" |
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 might defineStandard one to be the default empty value, then we don't need to do additional checks later on.
| MCPToolsetStandardMCPToolset="standard" | |
| MCPToolsetStandardMCPToolset="" |
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.
Neat trick, but it feels unintuitive - especially if we add debug logging with toolset names later. I'd be surprised to see this pattern as a reader.
| ) | ||
| // mcpHTTPHandler creates the MCP HTTP transport handler | ||
| // It supports a "toolset" query parameter to select the set of tools to register. |
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.
Do we want to expose toolsets specifically, or do we want to add a simple?source=chatgpt and then internally map to the corresponding toolsets?
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 conceptually prefer toolsets over sources. It feels more appropriate for the client to choose which tools it wants to access, rather than having the server adapt to the client.
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.
Makes sense.
Would it be possible to specify multiple toolsets then?
Say we split the param on, and then composite them into a new one. Then one could have a?toolset=chatgpt,workspaces only to get the chatgpt and workspace-related tools.
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.
That could definitely be a later-stage extension.
2e37318 to5efa6d9Compare79cd80e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Addressescoder/internal#772.
Adds a new
/api/experimental/mcp/chatgptendpoint which exposes newfetchandsearchtools compatible with ChatGPT, as described in theChatGPT docs. These tools are exposed in isolation because in my usage I found that ChatGPT refuses to connect to Coder if it sees additional MCP tools.