-
Notifications
You must be signed in to change notification settings - Fork 18
feat: list and call tools from external MCPs #1057
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
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ed21dda to
0b97202
Compare
91f5cbf to
60aa2a0
Compare
| tools = buildToolListEntries(toolset.Tools) | ||
|
|
||
| // Unfold proxy tools from external MCP servers | ||
| unfoldedTools, err := unfoldExternalMCPTools(ctx, logger, toolset.Tools, payload.oauthTokenInputs) |
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.
So checking my understanding here. It seems like we actually only unfold an external tool in mcp context. So right now these tools wouldn't be visible/useable in the playground. Correct?
60aa2a0 to
af7c803
Compare
0b97202 to
643abc0
Compare
643abc0 to
108dfcc
Compare
server/internal/conv/tools.go
Outdated
| func ToToolListEntry(tool *types.Tool) (name, description string, inputSchema json.RawMessage, meta map[string]any, err error) { | ||
| if tool == nil { | ||
| return "", "", nil, nil | ||
| return "", "", nil, nil, nil |
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 draw the line at five return params lol
Wrap that baby in a struct
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.
yeah that baby belongs in a struct. ✅
| } | ||
|
|
||
| // NewExternalMCPToolCallPlan creates a new Tool wrapping an ExternalMCPTool. | ||
| func NewExternalMCPToolCallPlan(tool *ToolDescriptor, plan *ExternalMCPToolCallPlan) *ToolCallPlan { |
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.
Im surprised the Plan for this isn't the same as HTTP
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 that there are some differences for sure:
for instance, the way you want to map input configuration to source configuration is way less complex for external MCP in that you don't have to worry about all the Header vs Body vs Param configuration options of open API + formatting options Open API supports. Also no notion of something like request method
server/internal/mcp/impl.go
Outdated
| } | ||
|
|
||
| // Pre-resolve external MCP OAuth config for use in switch | ||
| fullToolset, descErr := mv.DescribeToolset(ctx, s.logger, s.db, mv.ProjectID(toolset.ProjectID), mv.ToolsetSlug(toolset.Slug), &s.toolsetCache) |
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 need the FULL toolset? DescribeToolset is somewhat expensive
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.
Does wellKnown get spammed or am i misremembering
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.
well-known gets traffic but not a ton. All of it is from Polar. Even so I refactored to short-circuit before describe toolset in order to ensure that if there is a latency spike that we're going to avoid it
server/internal/mcp/impl.go
Outdated
| } | ||
|
|
||
| // TODO: Consider caching hasExternalMCPOAuth on the toolset record to avoid loading | ||
| // the full toolset on every request just to check OAuth requirements. |
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.
Yes lets definitely do that. Alternatively you can add a query that just retrieves the information needed for oauth instead of loading the entire toolet
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 have a ticket draft for this which I can tag you on for discussion. I don't think it's a matter of just storing it on the toolset. It interfaces with other OAuth concepts in pretty complicated ways, so I was hoping to peel off the actual abstraction work into a separately reviewed PR
| Token: externalSecret.Token, | ||
| }) | ||
| } | ||
| case toolset.McpIsPublic && hasExternalMCPOAuth: |
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 already thought all this oauth stuff needed to be factored out of this method even before this addition. Let's figure out a way to encapsulate the oauth logic elsewhere. At the very least make a ticket with a link to this method saying to refactor
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.
Have a ticket drafted for this. I need to wrap my head around the implications of Walker's recent changes if I'm going to have any hope of making a refactor here a net improvement
| metrics.RecordMCPToolCall(ctx, toolset.OrganizationID, mcpURL, params.Name) | ||
| } | ||
|
|
||
| // Check if this is an external MCP tool call (format: "slug:toolname") |
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.
Is that refactor coming in a later pr
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 version implemented here uses the tool name format. I'll follow up with another PR alongside support for instances.
qstearns
left a comment
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.
Left a few responses. Addressed some of the feedback here but also would prefer to keep some of this work scoped to linear tickets. Stuff like refactoring our oauth logic I think is just gonna require a slightly more holistic view of the system than what I want to bite off as the scope of this PR
server/internal/mcp/impl.go
Outdated
| } | ||
|
|
||
| // Pre-resolve external MCP OAuth config for use in switch | ||
| fullToolset, descErr := mv.DescribeToolset(ctx, s.logger, s.db, mv.ProjectID(toolset.ProjectID), mv.ToolsetSlug(toolset.Slug), &s.toolsetCache) |
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.
well-known gets traffic but not a ton. All of it is from Polar. Even so I refactored to short-circuit before describe toolset in order to ensure that if there is a latency spike that we're going to avoid it
server/internal/mcp/impl.go
Outdated
| } | ||
|
|
||
| // TODO: Consider caching hasExternalMCPOAuth on the toolset record to avoid loading | ||
| // the full toolset on every request just to check OAuth requirements. |
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 have a ticket draft for this which I can tag you on for discussion. I don't think it's a matter of just storing it on the toolset. It interfaces with other OAuth concepts in pretty complicated ways, so I was hoping to peel off the actual abstraction work into a separately reviewed PR
| Token: externalSecret.Token, | ||
| }) | ||
| } | ||
| case toolset.McpIsPublic && hasExternalMCPOAuth: |
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.
Have a ticket drafted for this. I need to wrap my head around the implications of Walker's recent changes if I'm going to have any hope of making a refactor here a net improvement
| metrics.RecordMCPToolCall(ctx, toolset.OrganizationID, mcpURL, params.Name) | ||
| } | ||
|
|
||
| // Check if this is an external MCP tool call (format: "slug:toolname") |
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 version implemented here uses the tool name format. I'll follow up with another PR alongside support for instances.
| } | ||
|
|
||
| // NewExternalMCPToolCallPlan creates a new Tool wrapping an ExternalMCPTool. | ||
| func NewExternalMCPToolCallPlan(tool *ToolDescriptor, plan *ExternalMCPToolCallPlan) *ToolCallPlan { |
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 that there are some differences for sure:
for instance, the way you want to map input configuration to source configuration is way less complex for external MCP in that you don't have to worry about all the Header vs Body vs Param configuration options of open API + formatting options Open API supports. Also no notion of something like request method
Implements runtime tool listing and calling for external MCP servers. Proxy tools (
tools:externalmcp:<slug>:proxy) are "unfolded" at runtime to expose the actual tools from the external MCP server.Key changes:
unfoldExternalMCPTools()in MCP RPC handlers - expands proxy tools to actual tools viaListToolsFromProxy()handleExternalMCPToolCall()- routes tool calls to external MCP servers via session APIdoExternalMCP()in gateway proxy - handles tool execution with OAuth token passthroughGetToolCallPlanByURN()updated to handleToolKindExternalMCPconv.ToBaseTool()returns error for proxy tools (must be unfolded first)Tool naming convention: External MCP tools are named as
<slug>:<toolname>(e.g.,notion:search,github:list_repos).Summary
external-mcp/5/list-and-call: URN kind and design types- Add ToolKindExternalMCPexternal-mcp/5/list-and-call: implementation using generated types- Core unfolding and calling logicexternal-mcp/5/list-and-call: instances, toolsets, and tests- Integration with instances API and test updates🤖 Generated with Claude Code