feat: add MCP tools for ChatGPT #19102
Conversation
ccc3a6a to
3e42d7a
Compare
There was a problem hiding this comment.
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 registers all available MCP 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.
Could we leave the function as it was and add a new function called RegisterChatGPTTools 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)
coderd/mcp/mcp_e2e_test.go
Outdated
| } | ||
| }() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) |
There was a problem hiding this comment.
| 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.
I think we could simplify this a bit by making it a boolean called isChatGPT bool, then calling the appropriate Register function (either .RegisterTools() or .RegisterChatGPTTools()).
This also makes the handler less complex.
codersdk/toolsdk/chatgpt.go
Outdated
| func getServerURL(deps Deps) string { | ||
| serverURLCopy := *deps.coderClient.URL | ||
| serverURLCopy.Path = "" | ||
| serverURLCopy.RawQuery = "" | ||
| return serverURLCopy.String() | ||
| } |
There was a problem hiding this comment.
We may want to add a new function to Deps.
Then we could just deps.ServerURL() to get it.
codersdk/toolsdk/chatgpt.go
Outdated
|
|
||
| func searchTemplates(ctx context.Context, deps Deps) ([]SearchResultItem, error) { | ||
| serverURL := getServerURL(deps) | ||
| templates, err := deps.coderClient.Templates(ctx, codersdk.TemplateFilter{}) |
There was a problem hiding this comment.
Can we use the SearchQuery field to parameterize this?
| type MCPToolset string | ||
|
|
||
| const ( | ||
| MCPToolsetStandard MCPToolset = "standard" |
There was a problem hiding this comment.
We might define Standard one to be the default empty value, then we don't need to do additional checks later on.
| MCPToolsetStandard MCPToolset = "standard" | |
| MCPToolsetStandard MCPToolset = "" |
There was a problem hiding this comment.
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.
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.
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.
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.
That could definitely be a later-stage extension.
2e37318 to
5efa6d9
Compare
Addresses coder/internal#772.
Adds a new
/api/experimental/mcp/chatgptendpoint which exposes newfetchandsearchtools compatible with ChatGPT, as described in the ChatGPT 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.