-
Notifications
You must be signed in to change notification settings - Fork 285
mcp: remove tool genericity #302
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
DO NOT SUBMIT
TESTS DO NOT PASS YET
API changes to remove genericity from the tool call path.
This makes it easier to write code that can deal with tools generally,
like wrappers around a ToolHandler.
Here is the go doc diff:
--- /tmp/old.doc 2025-08-14 09:03:30.772292329 -0400
+++ /tmp/new.doc 2025-08-14 08:58:37.113063370 -0400
@@ -73,7 +73,7 @@
FUNCTIONS
-func AddTool[In, Out any](s *Server, t *Tool, h ToolHandlerFor[In, Out])
+func AddTool[In, Out any](s *Server, t *Tool, h TypedToolHandler[In, Out])
AddTool adds a Tool to the server, or replaces one with the same name.
If the tool's input schema is nil, it is set to the schema inferred from
the In type parameter, using jsonschema.For. If the tool's output schema is
@@ -81,6 +81,10 @@
schema is set to the schema inferred from Out. The Tool argument must not be
modified after this call.
+ The handler should return the result as the second return value. The first
+ return value, a *CallToolResult, may be nil, or its fields other than
+ StructuredContent may be populated.
+
func NewInMemoryTransports() (*InMemoryTransport, *InMemoryTransport)
NewInMemoryTransports returns two [InMemoryTransports] that connect to each
other.
@@ -125,24 +129,28 @@
func (c AudioContent) MarshalJSON() ([]byte, error)
-type CallToolParams = CallToolParamsFor[any]
-
-type CallToolParamsFor[In any] struct {
+type CallToolParams struct {
// This property is reserved by the protocol to allow clients and servers to
// attach additional metadata to their responses.
Meta `json:"_meta,omitempty"`
Name string `json:"name"`
- Arguments In `json:"arguments,omitempty"`
+ Arguments any `json:"arguments,omitempty"`
}
-func (x *CallToolParamsFor[Out]) GetProgressToken() any
+func (x *CallToolParams) GetProgressToken() any
-func (x *CallToolParamsFor[Out]) SetProgressToken(t any)
+func (x *CallToolParams) SetProgressToken(t any)
-type CallToolResult = CallToolResultFor[any]
- The server's response to a tool call.
+func (c *CallToolParams) UnmarshalJSON(data []byte) error
+ When unmarshalling CallToolParams on the server side, we need to delay
+ unmarshaling of the arguments.
-type CallToolResultFor[Out any] struct {
+type CallToolRequest struct {
+ Session *ServerSession
+ Params *CallToolParams
+}
+
+type CallToolResult struct {
// This property is reserved by the protocol to allow clients and servers to
// attach additional metadata to their responses.
Meta `json:"_meta,omitempty"`
@@ -151,7 +159,7 @@
Content []Content `json:"content"`
// An optional JSON object that represents the structured result of the tool
// call.
- StructuredContent Out `json:"structuredContent,omitempty"`
+ StructuredContent any `json:"structuredContent,omitempty"`
// Whether the tool call ended in an error.
//
// If not set, this is assumed to be false (the call was successful).
@@ -166,8 +174,9 @@
// should be reported as an MCP error response.
IsError bool `json:"isError,omitempty"`
}
+ The server's response to a tool call.
-func (x *CallToolResultFor[Out]) UnmarshalJSON(data []byte) error
+func (x *CallToolResult) UnmarshalJSON(data []byte) error
UnmarshalJSON handles the unmarshalling of content into the Content
interface.
@@ -283,7 +292,7 @@
Session *ClientSession
Params P
}
- A ClientRequest is a request to a client.
+ A ClientRequest[P] is a request to a client.
func (r *ClientRequest[P]) GetParams() Params
@@ -1532,9 +1541,7 @@
type ServerSession struct {
// Has unexported fields.
}
- A ServerSession is a logical connection from a single MCP client.
- Its methods can be used to send requests or notifications to the client.
- Create a session by calling Server.Connect.
+ a session by calling Server.Connect.
Call ServerSession.Close to close the connection, or await client
termination with ServerSession.Wait.
@@ -1786,6 +1793,8 @@
// If not provided, Annotations.Title should be used for display if present,
// otherwise Name.
Title string `json:"title,omitempty"`
+
+ // Has unexported fields.
}
Definition for a tool the client can call.
@@ -1826,13 +1835,10 @@
Clients should never make tool use decisions based on ToolAnnotations
received from untrusted servers.
-type ToolHandler = ToolHandlerFor[map[string]any, any]
- A ToolHandler handles a call to tools/call. [CallToolParams.Arguments] will
- contain a map[string]any that has been validated against the input schema.
-
-type ToolHandlerFor[In, Out any] func(context.Context, *ServerRequest[*CallToolParamsFor[In]]) (*CallToolResultFor[Out], error)
- A ToolHandlerFor handles a call to tools/call with typed arguments and
- results.
+type ToolHandler func(ctx context.Context, req *ServerRequest[*CallToolParams], args any) (*CallToolResult, error)
+ A ToolHandler handles a call to tools/call. req.Params.Arguments will
+ contain a json.RawMessage containing the arguments. args will contain a
+ value that has been validated against the input schema.
type ToolListChangedParams struct {
// This property is reserved by the protocol to allow clients and servers to
@@ -1856,6 +1862,10 @@
Transports should be used for at most one call to Server.Connect or
Client.Connect.
+type TypedToolHandler[In, Out any] func(context.Context, *ServerRequest[*CallToolParams], In) (*CallToolResult, Out, error)
+ A TypedToolHandler handles a call to tools/call with typed arguments and
+ results.
+
type UnsubscribeParams struct {
// This property is reserved by the protocol to allow clients and servers to
// attach additional metadata to their responses.
| // The handler should return the result as the second return value. The first return value, | ||
| // a *CallToolResult, may be nil, or its fields other than StructuredContent may be | ||
| // populated. | ||
| func AddTool[In, Out any](s *Server, t *Tool, h TypedToolHandler[In, Out]) { |
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.
This still doesn't solve the problem of easily wrapping all tool handlers.
Based on feedback, we need a way to access the underlying tool handler that is created by AddTool.
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'm not sure what the use case is here.
If you know you want to wrap handlers before you add tools, you would call TypedTool separately.
If you want to do so after adding tools, well, we have no server-side way of enumerating tools anyway, nor has anyone ever asked for one.
Added TypedTool, fixed tests. TODOs for followups: - Rewrite TestToolValidate. - Re-fix the bug from adding a duplicate tool.
| type ToolHandler = ToolHandlerFor[map[string]any, any] | ||
| // req.Params.Arguments will contain a json.RawMessage containing the arguments. | ||
| // args will contain a value that has been validated against the input schema. | ||
| type ToolHandler func(ctx context.Context, req *ServerRequest[*CallToolParams], args any) (*CallToolResult, 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.
Why do we need args in this signature? Won't args be set on the CallToolParams.
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 args in CallToolParams will be a json.RawMessage. The args arg will be unmarshalled and schema-validated.
| // Second arg is *Request[*ServerSession, *CallToolParamsFor[json.RawMessage]], but that creates | ||
| // a cycle. | ||
| type rawToolHandler = func(context.Context, any) (*CallToolResult, error) | ||
| type rawToolHandler func(ctx context.Context, req *ServerRequest[*CallToolParams]) (*CallToolResult, 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.
IMO ToolHandler should just be RawToolHandler.
| func newServerTool[In, Out any](t *Tool, h ToolHandlerFor[In, Out]) (*serverTool, error) { | ||
| st := &serverTool{tool: t} | ||
| // A TypedToolHandler handles a call to tools/call with typed arguments and results. | ||
| type TypedToolHandler[In, Out any] func(context.Context, *ServerRequest[*CallToolParams], In) (*CallToolResult, Out, 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.
My 2p: I'm not sure we shouldn't still have CallToolParamsFor[In] and CallToolResultFor[Out], though we wouldn't want the ordinary CallToolParams to be an alias (because @rsc found that confusing).
The important changes seemed to be:
- That CallToolParams and CallToolResult weren't aliases
- Making it possible to access the underlying ToolHandler
You can achieve both of those in this CL without adding the additional arguments.
I don't like that there are two ways to access the structured input, and two ways to set the structured output.
|
I commented on #318 just now, and I am not sure what the relationship is between that PR and this one. Is that one an alternative to this one? |
|
Yes. |
|
Superseded by #325. |
API changes to remove genericity from the tool call path. This makes it easier to write code that can deal with tools generally, like wrappers around a ToolHandler.
Here is the go doc diff:
--- /tmp/old.doc 2025-08-14 09:03:30.772292329 -0400 +++ /tmp/new.doc 2025-08-14 08:58:37.113063370 -0400 @@ -73,7 +73,7 @@
FUNCTIONS
-func AddTool[In, Out any](s *Server, t *Tool, h ToolHandlerFor[In, Out]) +func AddTool[In, Out any](s *Server, t *Tool, h TypedToolHandler[In, Out])
AddTool adds a Tool to the server, or replaces one with the same name.
If the tool's input schema is nil, it is set to the schema inferred from
the In type parameter, using jsonschema.For. If the tool's output schema is
@@ -81,6 +81,10 @@
schema is set to the schema inferred from Out. The Tool argument must not be
modified after this call.
func NewInMemoryTransports() (*InMemoryTransport, *InMemoryTransport)
NewInMemoryTransports returns two [InMemoryTransports] that connect to each
other.
@@ -125,24 +129,28 @@
func (c AudioContent) MarshalJSON() ([]byte, error)
-type CallToolParams = CallToolParamsFor[any]
-type CallToolParamsFor[In any] struct {
+type CallToolParams struct {
// This property is reserved by the protocol to allow clients and servers to
// attach additional metadata to their responses.
Meta
json:"_meta,omitempty"Name string
json:"name"json:"arguments,omitempty"json:"arguments,omitempty"}
-func (x *CallToolParamsFor[Out]) GetProgressToken() any +func (x *CallToolParams) GetProgressToken() any
-func (x *CallToolParamsFor[Out]) SetProgressToken(t any) +func (x *CallToolParams) SetProgressToken(t any)
-type CallToolResult = CallToolResultFor[any]
+func (c *CallToolParams) UnmarshalJSON(data []byte) error
-type CallToolResultFor[Out any] struct {
+type CallToolRequest struct {
+type CallToolResult struct {
// This property is reserved by the protocol to allow clients and servers to // attach additional metadata to their responses. Meta
json:"_meta,omitempty"@@ -151,7 +159,7 @@Content []Content
json:"content"// An optional JSON object that represents the structured result of the tool // call.json:"structuredContent,omitempty"json:"structuredContent,omitempty"// Whether the tool call ended in an error. // // If not set, this is assumed to be false (the call was successful). @@ -166,8 +174,9 @@// should be reported as an MCP error response. IsError bool
json:"isError,omitempty"}-func (x *CallToolResultFor[Out]) UnmarshalJSON(data []byte) error +func (x *CallToolResult) UnmarshalJSON(data []byte) error
UnmarshalJSON handles the unmarshalling of content into the Content
interface.
@@ -283,7 +292,7 @@
Session *ClientSession
Params P
}
func (r *ClientRequest[P]) GetParams() Params
@@ -1532,9 +1541,7 @@
type ServerSession struct {
// Has unexported fields.
}
a session by calling Server.Connect.
Call ServerSession.Close to close the connection, or await client
termination with ServerSession.Wait.
@@ -1786,6 +1793,8 @@
// If not provided, Annotations.Title should be used for display if present,
// otherwise Name.
Title string
json:"title,omitempty"// Has unexported fields. } Definition for a tool the client can call.
@@ -1826,13 +1835,10 @@
Clients should never make tool use decisions based on ToolAnnotations
received from untrusted servers.
-type ToolHandler = ToolHandlerFor[map[string]any, any]
-type ToolHandlerFor[In, Out any] func(context.Context, *ServerRequest[*CallToolParamsFor[In]]) (*CallToolResultFor[Out], error)
+type ToolHandler func(ctx context.Context, req *ServerRequest[*CallToolParams], args any) (*CallToolResult, error)
type ToolListChangedParams struct {
// This property is reserved by the protocol to allow clients and servers to
@@ -1856,6 +1862,10 @@
Transports should be used for at most one call to Server.Connect or
Client.Connect.
+type TypedToolHandler[In, Out any] func(context.Context, *ServerRequest[*CallToolParams], In) (*CallToolResult, Out, error)
type UnsubscribeParams struct {
// This property is reserved by the protocol to allow clients and servers to
// attach additional metadata to their responses.
PR Guideline
Typically, PRs should consist of a single commit, and so should generally follow
the rules for Go commit messages.
You must follow the form:
Notably, for the subject (the first line of description):
Additionally:
at a high level. Changes that are obvious from the diffs don't need to be mentioned.