[tools] internal/mcp: reorganize the tool api

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam has uploaded the change for review

Jonathan Amsterdam would like Robert Findley and Sam Thanawalla to review this change.

Commit message

internal/mcp: reorganize the tool api

This CL implements the following changes:

- CallToolsParams is no longer generic. Its Arguments field is an any.
  Clients can send whatever type they want, as long as it serializes
  to a JSON object.

- Tools not created via NewTool see the Arguments as a map[string]any.

- NewTool[TReq, TRes] takes an ordinary untyped ToolHandler, but it
  guarantees that the Arguments field of the ToolHandler is set to TReq.

- Schema resolution happens in AddTools. This puts schema-checking
  errors in one place. This is better than having both
  Tool.SetInputSchema and NewTool return errors.

- AddTool panics, but we add AddToolErr for the presumably rare cases
  when tools are created dynamically.

Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Change diff

diff --git a/gopls/internal/mcp/context.go b/gopls/internal/mcp/context.go
index 83c7633..1376a50 100644
--- a/gopls/internal/mcp/context.go
+++ b/gopls/internal/mcp/context.go
@@ -32,8 +32,9 @@
 	Location protocol.Location `json:"location"`
 }
 
-func contextHandler(ctx context.Context, session *cache.Session, params *mcp.CallToolParams[ContextParams]) (*mcp.CallToolResult, error) {
-	fh, snapshot, release, err := session.FileOf(ctx, params.Arguments.Location.URI)
+func contextHandler(ctx context.Context, session *cache.Session, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
+	cp := params.Arguments.(ContextParams)
+	fh, snapshot, release, err := session.FileOf(ctx, cp.Location.URI)
 	if err != nil {
 		return nil, err
 	}
@@ -44,7 +45,7 @@
 		return nil, fmt.Errorf("can't provide context for non-Go file")
 	}
 
-	pkg, pgf, err := golang.NarrowestPackageForFile(ctx, snapshot, params.Arguments.Location.URI)
+	pkg, pgf, err := golang.NarrowestPackageForFile(ctx, snapshot, cp.Location.URI)
 	if err != nil {
 		return nil, err
 	}
diff --git a/gopls/internal/mcp/mcp.go b/gopls/internal/mcp/mcp.go
index 7887eea..7c930fa 100644
--- a/gopls/internal/mcp/mcp.go
+++ b/gopls/internal/mcp/mcp.go
@@ -114,10 +114,10 @@
 	s := mcp.NewServer("golang", "v0.1", nil)
 
 	s.AddTools(
-		mcp.NewTool(
+		mcp.NewTool[ContextParams, struct{}](
 			"context",
 			"Provide context for a region within a Go file",
-			func(ctx context.Context, _ *mcp.ServerSession, request *mcp.CallToolParams[ContextParams]) (*mcp.CallToolResult, error) {
+			func(ctx context.Context, _ *mcp.ServerSession, request *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 				return contextHandler(ctx, session, request)
 			},
 			mcp.Input(
diff --git a/internal/mcp/client.go b/internal/mcp/client.go
index 8f47f6f..c91fa73 100644
--- a/internal/mcp/client.go
+++ b/internal/mcp/client.go
@@ -6,8 +6,6 @@
 
 import (
 	"context"
-	"encoding/json"
-	"fmt"
 	"iter"
 	"slices"
 	"sync"
@@ -289,35 +287,14 @@
 
 // CallTool calls the tool with the given name and arguments.
 // Pass a [CallToolOptions] to provide additional request fields.
-func (cs *ClientSession) CallTool(ctx context.Context, params *CallToolParams[json.RawMessage]) (*CallToolResult, error) {
+func (cs *ClientSession) CallTool(ctx context.Context, params *CallToolParams) (*CallToolResult, error) {
+	if params.Arguments == nil {
+		// Avoid sending nil over the wire.
+		params.Arguments = map[string]any{}
+	}
 	return handleSend[*CallToolResult](ctx, cs, methodCallTool, params)
 }
 
-// CallTool is a helper to call a tool with any argument type. It returns an
-// error if params.Arguments fails to marshal to JSON.
-func CallTool[TArgs any](ctx context.Context, cs *ClientSession, params *CallToolParams[TArgs]) (*CallToolResult, error) {
-	wireParams, err := toWireParams(params)
-	if err != nil {
-		return nil, err
-	}
-	return cs.CallTool(ctx, wireParams)
-}
-
-func toWireParams[TArgs any](params *CallToolParams[TArgs]) (*CallToolParams[json.RawMessage], error) {
-	data, err := json.Marshal(params.Arguments)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal arguments: %v", err)
-	}
-	// The field mapping here must be kept up to date with the CallToolParams.
-	// This is partially enforced by TestToWireParams, which verifies that all
-	// comparable fields are mapped.
-	return &CallToolParams[json.RawMessage]{
-		Meta:      params.Meta,
-		Name:      params.Name,
-		Arguments: data,
-	}, nil
-}
-
 func (cs *ClientSession) SetLevel(ctx context.Context, params *SetLevelParams) error {
 	_, err := handleSend[*emptyResult](ctx, cs, methodSetLevel, params)
 	return err
diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go
index e789e81..3d8b8b2 100644
--- a/internal/mcp/client_test.go
+++ b/internal/mcp/client_test.go
@@ -7,7 +7,6 @@
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -200,28 +199,3 @@
 		})
 	}
 }
-
-func TestToWireParams(t *testing.T) {
-	// This test verifies that toWireParams maps all fields.
-	// The Meta and Arguments fields are not comparable, so can't be checked by
-	// this simple test. However, this test will fail if new fields are added,
-	// and not handled by toWireParams.
-	params := &CallToolParams[struct{}]{
-		Name: "tool",
-	}
-	wireParams, err := toWireParams(params)
-	if err != nil {
-		t.Fatal(err)
-	}
-	v := reflect.ValueOf(wireParams).Elem()
-	for i := range v.Type().NumField() {
-		f := v.Type().Field(i)
-		if f.Name == "Meta" || f.Name == "Arguments" {
-			continue // not comparable
-		}
-		fv := v.Field(i)
-		if fv.Interface() == reflect.Zero(f.Type).Interface() {
-			t.Fatalf("toWireParams: unmapped field %q", f.Name)
-		}
-	}
-}
diff --git a/internal/mcp/cmd_test.go b/internal/mcp/cmd_test.go
index b13f24f..61df4b9 100644
--- a/internal/mcp/cmd_test.go
+++ b/internal/mcp/cmd_test.go
@@ -31,7 +31,7 @@
 	ctx := context.Background()
 
 	server := mcp.NewServer("greeter", "v0.0.1", nil)
-	server.AddTools(mcp.NewTool("greet", "say hi", SayHi))
+	server.AddTools(mcp.NewTool[SayHiParams, struct{}]("greet", "say hi", SayHi))
 
 	if err := server.Run(ctx, mcp.NewStdIOTransport()); err != nil {
 		log.Fatal(err)
@@ -56,7 +56,7 @@
 	if err != nil {
 		log.Fatal(err)
 	}
-	got, err := mcp.CallTool(ctx, session, &mcp.CallToolParams[map[string]any]{
+	got, err := session.CallTool(ctx, &mcp.CallToolParams{
 		Name:      "greet",
 		Arguments: map[string]any{"name": "user"},
 	})
diff --git a/internal/mcp/examples/hello/main.go b/internal/mcp/examples/hello/main.go
index 84e409d..6c8e66f 100644
--- a/internal/mcp/examples/hello/main.go
+++ b/internal/mcp/examples/hello/main.go
@@ -21,7 +21,7 @@
 	Name string `json:"name"`
 }
 
-func SayHi(ctx context.Context, ss *mcp.ServerSession, params *mcp.CallToolParams[HiArgs]) (*mcp.CallToolResult, error) {
+func SayHi(ctx context.Context, ss *mcp.ServerSession, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 	return &mcp.CallToolResult{
 		Content: []*mcp.Content{
 			mcp.NewTextContent("Hi " + params.Name),
@@ -42,7 +42,7 @@
 	flag.Parse()
 
 	server := mcp.NewServer("greeter", "v0.0.1", nil)
-	server.AddTools(mcp.NewTool("greet", "say hi", SayHi, mcp.Input(
+	server.AddTools(mcp.NewTool[HiArgs, struct{}]("greet", "say hi", SayHi, mcp.Input(
 		mcp.Property("name", mcp.Description("the name to say hi to")),
 	)))
 	server.AddPrompts(mcp.NewPrompt("greet", "", PromptHi))
diff --git a/internal/mcp/examples/sse/main.go b/internal/mcp/examples/sse/main.go
index 0447e7f..d8018fa 100644
--- a/internal/mcp/examples/sse/main.go
+++ b/internal/mcp/examples/sse/main.go
@@ -19,7 +19,7 @@
 	Name string `json:"name"`
 }
 
-func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams[SayHiParams]) (*mcp.CallToolResult, error) {
+func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 	return &mcp.CallToolResult{
 		Content: []*mcp.Content{
 			mcp.NewTextContent("Hi " + params.Name),
@@ -35,10 +35,10 @@
 	}
 
 	server1 := mcp.NewServer("greeter1", "v0.0.1", nil)
-	server1.AddTools(mcp.NewTool("greet1", "say hi", SayHi))
+	server1.AddTools(mcp.NewTool[SayHiParams, struct{}]("greet1", "say hi", SayHi))
 
 	server2 := mcp.NewServer("greeter2", "v0.0.1", nil)
-	server2.AddTools(mcp.NewTool("greet2", "say hello", SayHi))
+	server2.AddTools(mcp.NewTool[SayHiParams, struct{}]("greet2", "say hello", SayHi))
 
 	log.Printf("MCP servers serving at %s\n", *httpAddr)
 	handler := mcp.NewSSEHandler(func(request *http.Request) *mcp.Server {
diff --git a/internal/mcp/features_test.go b/internal/mcp/features_test.go
index 327f817..c5543c0 100644
--- a/internal/mcp/features_test.go
+++ b/internal/mcp/features_test.go
@@ -18,7 +18,7 @@
 	Name string `json:"name"`
 }
 
-func SayHi(ctx context.Context, cc *ServerSession, params *CallToolParams[SayHiParams]) (*CallToolResult, error) {
+func SayHi(ctx context.Context, cc *ServerSession, params *CallToolParams) (*CallToolResult, error) {
 	return &CallToolResult{
 		Content: []*Content{
 			NewTextContent("Hi " + params.Name),
@@ -27,9 +27,9 @@
 }
 
 func TestFeatureSetOrder(t *testing.T) {
-	toolA := NewTool("apple", "apple tool", SayHi).Tool
-	toolB := NewTool("banana", "banana tool", SayHi).Tool
-	toolC := NewTool("cherry", "cherry tool", SayHi).Tool
+	toolA := NewTool[SayHiParams, struct{}]("apple", "apple tool", SayHi).Tool
+	toolB := NewTool[SayHiParams, struct{}]("banana", "banana tool", SayHi).Tool
+	toolC := NewTool[SayHiParams, struct{}]("cherry", "cherry tool", SayHi).Tool
 
 	testCases := []struct {
 		tools []*Tool
@@ -52,9 +52,9 @@
 }
 
 func TestFeatureSetAbove(t *testing.T) {
-	toolA := NewTool("apple", "apple tool", SayHi).Tool
-	toolB := NewTool("banana", "banana tool", SayHi).Tool
-	toolC := NewTool("cherry", "cherry tool", SayHi).Tool
+	toolA := NewTool[SayHiParams, struct{}]("apple", "apple tool", SayHi).Tool
+	toolB := NewTool[SayHiParams, struct{}]("banana", "banana tool", SayHi).Tool
+	toolC := NewTool[SayHiParams, struct{}]("cherry", "cherry tool", SayHi).Tool
 
 	testCases := []struct {
 		tools []*Tool
@@ -71,7 +71,6 @@
 		got := slices.Collect(fs.above(tc.above))
 		if diff := cmp.Diff(got, tc.want, cmpopts.IgnoreUnexported(jsonschema.Schema{})); diff != "" {
 			t.Errorf("expected %v, got %v, (-want +got):\n%s", tc.want, got, diff)
-
 		}
 	}
 }
diff --git a/internal/mcp/generate.go b/internal/mcp/generate.go
index d4b63c1..0807b6a 100644
--- a/internal/mcp/generate.go
+++ b/internal/mcp/generate.go
@@ -58,10 +58,9 @@
 		Name: "-",
 		Fields: config{
 			"Params": {
-				Name:       "CallToolParams",
-				TypeParams: [][2]string{{"TArgs", "any"}},
+				Name: "CallToolParams",
 				Fields: config{
-					"Arguments": {Substitute: "TArgs"},
+					"Arguments": {Substitute: "any"},
 				},
 			},
 		},
diff --git a/internal/mcp/internal/readme/client/client.go b/internal/mcp/internal/readme/client/client.go
index 53b36dd..189ba1f 100644
--- a/internal/mcp/internal/readme/client/client.go
+++ b/internal/mcp/internal/readme/client/client.go
@@ -25,11 +25,11 @@
 	}
 	defer session.Close()
 	// Call a tool on the server.
-	params := &mcp.CallToolParams[map[string]any]{
+	params := &mcp.CallToolParams{
 		Name:      "greet",
 		Arguments: map[string]any{"name": "you"},
 	}
-	if res, err := mcp.CallTool(ctx, session, params); err != nil {
+	if res, err := session.CallTool(ctx, params); err != nil {
 		log.Printf("CallTool failed: %v", err)
 	} else {
 		if res.IsError {
diff --git a/internal/mcp/internal/readme/server/server.go b/internal/mcp/internal/readme/server/server.go
index 504c745..f07844b 100644
--- a/internal/mcp/internal/readme/server/server.go
+++ b/internal/mcp/internal/readme/server/server.go
@@ -15,7 +15,7 @@
 	Name string `json:"name"`
 }
 
-func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams[HiParams]) (*mcp.CallToolResult, error) {
+func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 	return &mcp.CallToolResult{
 		Content: []*mcp.Content{mcp.NewTextContent("Hi " + params.Name)},
 	}, nil
@@ -24,7 +24,7 @@
 func main() {
 	// Create a server with a single tool.
 	server := mcp.NewServer("greeter", "v1.0.0", nil)
-	server.AddTools(mcp.NewTool("greet", "say hi", SayHi))
+	server.AddTools(mcp.NewTool[HiParams, struct{}]("greet", "say hi", SayHi))
 	// Run the server over stdin/stdout, until the client disconnects
 	_ = server.Run(context.Background(), mcp.NewStdIOTransport())
 }
diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go
index 2ab5b87..f080bef 100644
--- a/internal/mcp/mcp_test.go
+++ b/internal/mcp/mcp_test.go
@@ -30,11 +30,12 @@
 	Name string
 }
 
-func sayHi(ctx context.Context, ss *ServerSession, params *CallToolParams[hiParams]) (*CallToolResult, error) {
+func sayHi(ctx context.Context, ss *ServerSession, params *CallToolParams) (*CallToolResult, error) {
+	hp := params.Arguments.(hiParams)
 	if err := ss.Ping(ctx, nil); err != nil {
 		return nil, fmt.Errorf("ping failed: %v", err)
 	}
-	return &CallToolResult{Content: []*Content{NewTextContent("hi " + params.Arguments.Name)}}, nil
+	return &CallToolResult{Content: []*Content{NewTextContent("hi " + hp.Name)}}, nil
 }
 
 func TestEndToEnd(t *testing.T) {
@@ -187,7 +188,7 @@
 			t.Fatalf("tools/list mismatch (-want +got):\n%s", diff)
 		}
 
-		gotHi, err := CallTool(ctx, cs, &CallToolParams[map[string]any]{
+		gotHi, err := cs.CallTool(ctx, &CallToolParams{
 			Name:      "greet",
 			Arguments: map[string]any{"name": "user"},
 		})
@@ -201,7 +202,7 @@
 			t.Errorf("tools/call 'greet' mismatch (-want +got):\n%s", diff)
 		}
 
-		gotFail, err := CallTool(ctx, cs, &CallToolParams[map[string]any]{
+		gotFail, err := cs.CallTool(ctx, &CallToolParams{
 			Name:      "fail",
 			Arguments: map[string]any{},
 		})
@@ -218,7 +219,7 @@
 			t.Errorf("tools/call 'fail' mismatch (-want +got):\n%s", diff)
 		}
 
-		s.AddTools(&ServerTool{Tool: &Tool{Name: "T"}})
+		s.AddTools(&ServerTool{Tool: &Tool{Name: "T"}, Handler: sayHi})
 		waitForNotification(t, "tools")
 		s.RemoveTools("T")
 		waitForNotification(t, "tools")
@@ -394,8 +395,8 @@
 	errTestFailure = errors.New("mcp failure")
 
 	tools = map[string]*ServerTool{
-		"greet": NewTool("greet", "say hi", sayHi),
-		"fail": NewTool("fail", "just fail", func(context.Context, *ServerSession, *CallToolParams[struct{}]) (*CallToolResult, error) {
+		"greet": NewTool[hiParams, struct{}]("greet", "say hi", sayHi),
+		"fail": NewTool[struct{}, struct{}]("fail", "just fail", func(context.Context, *ServerSession, *CallToolParams) (*CallToolResult, error) {
 			return nil, errTestFailure
 		}),
 	}
@@ -514,7 +515,7 @@
 }
 
 func TestServerClosing(t *testing.T) {
-	cc, cs := basicConnection(t, NewTool("greet", "say hi", sayHi))
+	cc, cs := basicConnection(t, NewTool[hiParams, struct{}]("greet", "say hi", sayHi))
 	defer cs.Close()
 
 	ctx := context.Background()
@@ -526,7 +527,7 @@
 		}
 		wg.Done()
 	}()
-	if _, err := CallTool(ctx, cs, &CallToolParams[map[string]any]{
+	if _, err := cs.CallTool(ctx, &CallToolParams{
 		Name:      "greet",
 		Arguments: map[string]any{"name": "user"},
 	}); err != nil {
@@ -534,7 +535,7 @@
 	}
 	cc.Close()
 	wg.Wait()
-	if _, err := CallTool(ctx, cs, &CallToolParams[map[string]any]{
+	if _, err := cs.CallTool(ctx, &CallToolParams{
 		Name:      "greet",
 		Arguments: map[string]any{"name": "user"},
 	}); !errors.Is(err, ErrConnectionClosed) {
@@ -586,7 +587,7 @@
 		cancelled = make(chan struct{}, 1) // don't block the request
 	)
 
-	slowRequest := func(ctx context.Context, cc *ServerSession, params *CallToolParams[struct{}]) (*CallToolResult, error) {
+	slowRequest := func(ctx context.Context, cc *ServerSession, params *CallToolParams) (*CallToolResult, error) {
 		start <- struct{}{}
 		select {
 		case <-ctx.Done():
@@ -596,12 +597,20 @@
 		}
 		return nil, nil
 	}
-	_, cs := basicConnection(t, NewTool("slow", "a slow request", slowRequest))
+	st := &ServerTool{
+		Tool:    &Tool{Name: "slow"},
+		Handler: slowRequest,
+	}
+	_, cs := basicConnection(t, st)
 	defer cs.Close()
 
 	ctx, cancel := context.WithCancel(context.Background())
-	go CallTool(ctx, cs, &CallToolParams[struct{}]{Name: "slow"})
-	<-start
+	go cs.CallTool(ctx, &CallToolParams{Name: "slow"})
+	select {
+	case <-start:
+	case <-time.After(2 * time.Second):
+		t.Fatal("timeout waiting for handler to be called")
+	}
 	cancel()
 	select {
 	case <-cancelled:
diff --git a/internal/mcp/protocol.go b/internal/mcp/protocol.go
index 07db7d3..cd9139a 100644
--- a/internal/mcp/protocol.go
+++ b/internal/mcp/protocol.go
@@ -26,15 +26,15 @@
 	Priority float64 `json:"priority,omitempty"`
 }
 
-type CallToolParams[TArgs 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      Meta   `json:"_meta,omitempty"`
-	Arguments TArgs  `json:"arguments,omitempty"`
+	Arguments any    `json:"arguments,omitempty"`
 	Name      string `json:"name"`
 }
 
-func (x *CallToolParams[TArgs]) GetMeta() *Meta { return &x.Meta }
+func (x *CallToolParams) GetMeta() *Meta { return &x.Meta }
 
 // The server's response to a tool call.
 //
diff --git a/internal/mcp/server.go b/internal/mcp/server.go
index fa55c33..dbf4a17 100644
--- a/internal/mcp/server.go
+++ b/internal/mcp/server.go
@@ -9,7 +9,6 @@
 	"context"
 	"encoding/base64"
 	"encoding/gob"
-	"encoding/json"
 	"fmt"
 	"iter"
 	"net/url"
@@ -107,15 +106,56 @@
 
 // AddTools adds the given tools to the server,
 // replacing any with the same names.
+// The arguments must not be modified after this call.
+//
+// AddTools panics if errors are detected.
+// Call [AddToolsErr] to obtain an error instead.
 func (s *Server) AddTools(tools ...*ServerTool) {
+	if err := s.AddToolsErr(tools...); err != nil {
+		panic(err)
+	}
+}
+
+// TODO: don't provide this until someone asks?
+// AddToolsErr is like [AddTools], but returns an error instead of panicking.
+func (s *Server) AddToolsErr(tools ...*ServerTool) error {
 	// Only notify if something could change.
 	if len(tools) == 0 {
-		return
+		return nil
 	}
+	// Wrap the user's Handlers with rawHandlers that take a json.RawMessage.
+	for _, st := range tools {
+		if st.rawHandler == nil {
+			// This ServerTool was not created with NewTool.
+			if st.Handler == nil {
+				return fmt.Errorf("AddTools: tool %q has no handler", st.Tool.Name)
+			}
+			st.rawHandler = newRawHandler[map[string]any](st)
+			// Resolve the schemas, with no base URI. We don't expect tool schemas to
+			// refer outside of themselves.
+			if st.Tool.InputSchema != nil {
+				r, err := st.Tool.InputSchema.Resolve(nil)
+				if err != nil {
+					return err
+				}
+				st.inputResolved = r
+			}
+
+			// if st.Tool.OutputSchema != nil {
+			// 	st.outputResolved, err := st.Tool.OutputSchema.Resolve(nil)
+			// 	if err != nil {
+			// 		return err
+			// 	}
+			// }
+		}
+	}
+
 	// Assume there was a change, since add replaces existing tools.
 	// (It's possible a tool was replaced with an identical one, but not worth checking.)
+	// TODO: surface notify error here?
 	s.changeAndNotify(notificationToolListChanged, &ToolListChangedParams{},
 		func() bool { s.tools.add(tools...); return true })
+	return nil
 }
 
 // RemoveTools removes the tools with the given names.
@@ -230,14 +270,14 @@
 	return res, nil
 }
 
-func (s *Server) callTool(ctx context.Context, cc *ServerSession, params *CallToolParams[json.RawMessage]) (*CallToolResult, error) {
+func (s *Server) callTool(ctx context.Context, cc *ServerSession, params *rawCallToolParams) (*CallToolResult, error) {
 	s.mu.Lock()
 	tool, ok := s.tools.get(params.Name)
 	s.mu.Unlock()
 	if !ok {
 		return nil, fmt.Errorf("%s: unknown tool %q", jsonrpc2.ErrInvalidParams, params.Name)
 	}
-	return tool.Handler(ctx, cc, params)
+	return tool.rawHandler(ctx, cc, params)
 }
 
 func (s *Server) listResources(_ context.Context, _ *ServerSession, params *ListResourcesParams) (*ListResourcesResult, error) {
diff --git a/internal/mcp/server_example_test.go b/internal/mcp/server_example_test.go
index 07d34b9..649180a 100644
--- a/internal/mcp/server_example_test.go
+++ b/internal/mcp/server_example_test.go
@@ -20,10 +20,10 @@
 	Name string `json:"name"`
 }
 
-func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams[SayHiParams]) (*mcp.CallToolResult, error) {
+func SayHi(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 	return &mcp.CallToolResult{
 		Content: []*mcp.Content{
-			mcp.NewTextContent("Hi " + params.Arguments.Name),
+			mcp.NewTextContent("Hi " + params.Arguments.(SayHiParams).Name),
 		},
 	}, nil
 }
@@ -33,7 +33,7 @@
 	clientTransport, serverTransport := mcp.NewInMemoryTransports()
 
 	server := mcp.NewServer("greeter", "v0.0.1", nil)
-	server.AddTools(mcp.NewTool("greet", "say hi", SayHi))
+	server.AddTools(mcp.NewTool[SayHiParams, struct{}]("greet", "say hi", SayHi))
 
 	serverSession, err := server.Connect(ctx, serverTransport)
 	if err != nil {
@@ -46,7 +46,7 @@
 		log.Fatal(err)
 	}
 
-	res, err := mcp.CallTool(ctx, clientSession, &mcp.CallToolParams[map[string]any]{
+	res, err := clientSession.CallTool(ctx, &mcp.CallToolParams{
 		Name:      "greet",
 		Arguments: map[string]any{"name": "user"},
 	})
@@ -78,9 +78,9 @@
 }
 
 func TestListTools(t *testing.T) {
-	toolA := mcp.NewTool("apple", "apple tool", SayHi)
-	toolB := mcp.NewTool("banana", "banana tool", SayHi)
-	toolC := mcp.NewTool("cherry", "cherry tool", SayHi)
+	toolA := mcp.NewTool[SayHiParams, struct{}]("apple", "apple tool", SayHi)
+	toolB := mcp.NewTool[SayHiParams, struct{}]("banana", "banana tool", SayHi)
+	toolC := mcp.NewTool[SayHiParams, struct{}]("cherry", "cherry tool", SayHi)
 	tools := []*mcp.ServerTool{toolA, toolB, toolC}
 	ctx := context.Background()
 	clientSession, serverSession, server := createSessions(ctx)
@@ -147,7 +147,6 @@
 			t.Fatalf("Resources() mismatch (-want +got):\n%s", diff)
 		}
 	})
-
 }
 
 func TestListPrompts(t *testing.T) {
diff --git a/internal/mcp/shared.go b/internal/mcp/shared.go
index 8f0ce48..1dd6046 100644
--- a/internal/mcp/shared.go
+++ b/internal/mcp/shared.go
@@ -271,3 +271,10 @@
 type emptyResult struct{}
 
 func (*emptyResult) GetMeta() *Meta { panic("should never be called") }
+
+// rawCallToolParams is used to unmarshal CallToolParams on the server.
+// It delays unmarshalling of the arguments.
+type rawCallToolParams struct {
+	CallToolParams
+	RawArguments json.RawMessage `json:"arguments,omitempty"`
+}
diff --git a/internal/mcp/shared_test.go b/internal/mcp/shared_test.go
index 7d3d533..4c30a8c 100644
--- a/internal/mcp/shared_test.go
+++ b/internal/mcp/shared_test.go
@@ -5,6 +5,7 @@
 package mcp
 
 import (
+	"context"
 	"encoding/json"
 	"strings"
 	"testing"
@@ -68,3 +69,80 @@
 	}
 	return res
 }
+
+// TODO(jba): this shouldn't be in this file, but tool_test.go doesn't have access to unexported symbols.
+func TestNewToolValidate(t *testing.T) {
+	// Check that the tool returned from NewTool properly validates its input schema.
+
+	type req struct {
+		I int
+		B bool
+		S string `json:",omitempty"`
+		P *int   `json:",omitempty"`
+	}
+
+	dummyHandler := func(context.Context, *ServerSession, *CallToolParams) (*CallToolResult, error) {
+		return nil, nil
+	}
+
+	tool := NewTool[req, struct{}]("test", "test", dummyHandler)
+	// Need to add the tool to a server to get resolved schemas.
+	// s := NewServer("", "", nil)
+
+	for _, tt := range []struct {
+		desc string
+		args map[string]any
+		want string // error should contain this string; empty for success
+	}{
+		{
+			"both required",
+			map[string]any{"I": 1, "B": true},
+			"",
+		},
+		{
+			"optional",
+			map[string]any{"I": 1, "B": true, "S": "foo"},
+			"",
+		},
+		{
+			"wrong type",
+			map[string]any{"I": 1.5, "B": true},
+			"cannot unmarshal",
+		},
+		{
+			"extra property",
+			map[string]any{"I": 1, "B": true, "C": 2},
+			"unknown field",
+		},
+		{
+			"value for pointer",
+			map[string]any{"I": 1, "B": true, "P": 3},
+			"",
+		},
+		{
+			"null for pointer",
+			map[string]any{"I": 1, "B": true, "P": nil},
+			"",
+		},
+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			raw, err := json.Marshal(tt.args)
+			if err != nil {
+				t.Fatal(err)
+			}
+			_, err = tool.rawHandler(context.Background(), nil,
+				&rawCallToolParams{RawArguments: json.RawMessage(raw)})
+			if err == nil && tt.want != "" {
+				t.Error("got success, wanted failure")
+			}
+			if err != nil {
+				if tt.want == "" {
+					t.Fatalf("failed with:\n%s\nwanted success", err)
+				}
+				if !strings.Contains(err.Error(), tt.want) {
+					t.Fatalf("got:\n%s\nwanted to contain %q", err, tt.want)
+				}
+			}
+		})
+	}
+}
diff --git a/internal/mcp/sse_example_test.go b/internal/mcp/sse_example_test.go
index 6ad05b9..603d3de 100644
--- a/internal/mcp/sse_example_test.go
+++ b/internal/mcp/sse_example_test.go
@@ -18,15 +18,16 @@
 	X, Y int
 }
 
-func Add(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams[AddParams]) (*mcp.CallToolResult, error) {
+func Add(ctx context.Context, cc *mcp.ServerSession, params *mcp.CallToolParams) (*mcp.CallToolResult, error) {
+	args := params.Arguments.(AddParams)
 	return &mcp.CallToolResult{
-		Content: []*mcp.Content{mcp.NewTextContent(fmt.Sprintf("%d", params.Arguments.X+params.Arguments.Y))},
+		Content: []*mcp.Content{mcp.NewTextContent(fmt.Sprintf("%d", args.X+args.Y))},
 	}, nil
 }
 
 func ExampleSSEHandler() {
 	server := mcp.NewServer("adder", "v0.0.1", nil)
-	server.AddTools(mcp.NewTool("add", "add two numbers", Add))
+	server.AddTools(mcp.NewTool[AddParams, struct{}]("add", "add two numbers", Add))
 
 	handler := mcp.NewSSEHandler(func(*http.Request) *mcp.Server { return server })
 	httpServer := httptest.NewServer(handler)
@@ -41,7 +42,7 @@
 	}
 	defer cs.Close()
 
-	res, err := mcp.CallTool(ctx, cs, &mcp.CallToolParams[map[string]any]{
+	res, err := cs.CallTool(ctx, &mcp.CallToolParams{
 		Name:      "add",
 		Arguments: map[string]any{"x": 1, "y": 2},
 	})
diff --git a/internal/mcp/sse_test.go b/internal/mcp/sse_test.go
index cef1dbb..a166644 100644
--- a/internal/mcp/sse_test.go
+++ b/internal/mcp/sse_test.go
@@ -19,7 +19,7 @@
 		t.Run(fmt.Sprintf("closeServerFirst=%t", closeServerFirst), func(t *testing.T) {
 			ctx := context.Background()
 			server := NewServer("testServer", "v1.0.0", nil)
-			server.AddTools(NewTool("greet", "say hi", sayHi))
+			server.AddTools(NewTool[hiParams, struct{}]("greet", "say hi", sayHi))
 
 			sseHandler := NewSSEHandler(func(*http.Request) *Server { return server })
 
@@ -44,7 +44,7 @@
 				t.Fatal(err)
 			}
 			ss := <-conns
-			gotHi, err := CallTool(ctx, cs, &CallToolParams[map[string]any]{
+			gotHi, err := cs.CallTool(ctx, &CallToolParams{
 				Name:      "greet",
 				Arguments: map[string]any{"name": "user"},
 			})
diff --git a/internal/mcp/tool.go b/internal/mcp/tool.go
index 6e8b3cf..d012165 100644
--- a/internal/mcp/tool.go
+++ b/internal/mcp/tool.go
@@ -15,48 +15,75 @@
 )
 
 // A ToolHandler handles a call to tools/call.
-type ToolHandler[TArgs any] func(context.Context, *ServerSession, *CallToolParams[TArgs]) (*CallToolResult, error)
+type ToolHandler func(context.Context, *ServerSession, *CallToolParams) (*CallToolResult, error)
+
+// A rawToolHandler is like a ToolHandler, but takes the arguments as as json.RawMessage.
+type rawToolHandler func(context.Context, *ServerSession, *rawCallToolParams) (*CallToolResult, error)
 
 // A Tool is a tool definition that is bound to a tool handler.
 type ServerTool struct {
 	Tool    *Tool
-	Handler ToolHandler[json.RawMessage]
+	Handler ToolHandler
+	// Set in NewTool or AddToolErr.
+	rawHandler rawToolHandler
+	// Resolved tool schemas. Set in AddToolErr.
+	inputResolved, outputResolved *jsonschema.Resolved
 }
 
-// NewTool is a helper to make a tool using reflection on the given handler.
+// NewTool is a helper to make a tool using reflection on the given type parameters.
+// When the tool is called, CallToolParams.Arguments will be of type TReq.
 //
 // If provided, variadic [ToolOption] values may be used to customize the tool.
 //
 // The input schema for the tool is extracted from the request type for the
 // handler, and used to unmmarshal and validate requests to the handler. This
 // schema may be customized using the [Input] option.
-//
-// The handler request type must translate to a valid schema, as documented by
-// [jsonschema.ForType]; otherwise, NewTool panics.
-//
-// TODO: just have the handler return a CallToolResult: returning []Content is
-// going to be inconsistent with other server features.
-func NewTool[TReq any](name, description string, handler ToolHandler[TReq], opts ...ToolOption) *ServerTool {
-	schema, err := jsonschema.For[TReq]()
+func NewTool[TReq, TRes any](name, description string, handler ToolHandler, opts ...ToolOption) *ServerTool {
+	st, err := newToolErr[TReq, TRes](name, description, handler, opts...)
 	if err != nil {
-		panic(err)
+		panic(fmt.Errorf("NewTool(%q): %w", name, err))
 	}
-	// We must resolve the schema after the ToolOptions have had a chance to update it.
-	// But the handler needs access to the resolved schema, and the options may change
-	// the handler too.
-	// The best we can do is use the resolved schema in our own wrapped handler,
-	// and hope that no ToolOption replaces it.
-	// TODO(jba): at a minimum, document this.
-	var resolved *jsonschema.Resolved
-	wrapped := func(ctx context.Context, cc *ServerSession, params *CallToolParams[json.RawMessage]) (*CallToolResult, error) {
-		var params2 CallToolParams[TReq]
-		if params.Arguments != nil {
-			if err := unmarshalSchema(params.Arguments, resolved, &params2.Arguments); err != nil {
-				return nil, err
-			}
+	return st
+}
+
+func newToolErr[TReq, TRes any](name, description string, handler ToolHandler, opts ...ToolOption) (*ServerTool, error) {
+	// TODO: check that TReq is a struct.
+	ischema, err := jsonschema.For[TReq]()
+	if err != nil {
+		return nil, err
+	}
+	// oschema, err := jsonschema.For[TRes]()
+	// if err != nil {
+	// 	return nil, err
+	// }
+
+	t := &ServerTool{
+		Tool: &Tool{
+			Name:        name,
+			Description: description,
+			InputSchema: ischema,
+			// OutputSchema: oschema,
+		},
+		Handler: handler,
+	}
+	for _, opt := range opts {
+		opt.set(t)
+	}
+
+	t.rawHandler = newRawHandler[TReq](t)
+	return t, nil
+}
+
+func newRawHandler[T any](st *ServerTool) rawToolHandler {
+	return func(ctx context.Context, ss *ServerSession, rparams *rawCallToolParams) (*CallToolResult, error) {
+		// Unmarshal the args into what should be a map.
+		var args T
+		if err := unmarshalSchema(rparams.RawArguments, st.inputResolved, &args); err != nil {
+			return nil, err
 		}
-		res, err := handler(ctx, cc, &params2)
-		// TODO: investigate why server errors are embedded in this strange way,
+		rparams.CallToolParams.Arguments = args
+		res, err := st.Handler(ctx, ss, &rparams.CallToolParams)
+		// TODO(rfindley): investigate why server errors are embedded in this strange way,
 		// rather than returned as jsonrpc2 server errors.
 		if err != nil {
 			return &CallToolResult{
@@ -66,26 +93,6 @@
 		}
 		return res, nil
 	}
-	t := &ServerTool{
-		Tool: &Tool{
-			Name:        name,
-			Description: description,
-			InputSchema: schema,
-		},
-		Handler: wrapped,
-	}
-	for _, opt := range opts {
-		opt.set(t)
-	}
-	if schema := t.Tool.InputSchema; schema != nil {
-		// Resolve the schema, with no base URI. We don't expect tool schemas to
-		// refer outside of themselves.
-		resolved, err = schema.Resolve(nil)
-		if err != nil {
-			panic(fmt.Errorf("resolving input schema %s: %w", schemaJSON(schema), err))
-		}
-	}
-	return t
 }
 
 // unmarshalSchema unmarshals data into v and validates the result according to
@@ -103,6 +110,8 @@
 	if err := dec.Decode(v); err != nil {
 		return fmt.Errorf("unmarshaling: %w", err)
 	}
+	// TODO(jba): apply defaults.
+	// TODO: test with nil args.
 	if resolved != nil {
 		if err := resolved.Validate(v); err != nil {
 			return fmt.Errorf("validating\n\t%s\nagainst\n\t %s:\n %w", data, schemaJSON(resolved.Schema()), err)
diff --git a/internal/mcp/tool_test.go b/internal/mcp/tool_test.go
index 077ea39..c1a555e 100644
--- a/internal/mcp/tool_test.go
+++ b/internal/mcp/tool_test.go
@@ -6,8 +6,6 @@
 
 import (
 	"context"
-	"encoding/json"
-	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -17,7 +15,7 @@
 )
 
 // testToolHandler is used for type inference in TestNewTool.
-func testToolHandler[T any](context.Context, *mcp.ServerSession, *mcp.CallToolParams[T]) (*mcp.CallToolResult, error) {
+func testToolHandler(context.Context, *mcp.ServerSession, *mcp.CallToolParams) (*mcp.CallToolResult, error) {
 	panic("not implemented")
 }
 
@@ -27,9 +25,9 @@
 		want *jsonschema.Schema
 	}{
 		{
-			mcp.NewTool("basic", "", testToolHandler[struct {
+			mcp.NewTool[struct {
 				Name string `json:"name"`
-			}]),
+			}, struct{}]("basic", "", testToolHandler),
 			&jsonschema.Schema{
 				Type:     "object",
 				Required: []string{"name"},
@@ -40,7 +38,7 @@
 			},
 		},
 		{
-			mcp.NewTool("enum", "", testToolHandler[struct{ Name string }], mcp.Input(
+			mcp.NewTool[struct{ Name string }, struct{}]("enum", "", testToolHandler, mcp.Input(
 				mcp.Property("Name", mcp.Enum("x", "y", "z")),
 			)),
 			&jsonschema.Schema{
@@ -53,12 +51,12 @@
 			},
 		},
 		{
-			mcp.NewTool("required", "", testToolHandler[struct {
+			mcp.NewTool[struct {
 				Name     string `json:"name"`
 				Language string `json:"language"`
 				X        int    `json:"x,omitempty"`
 				Y        int    `json:"y,omitempty"`
-			}], mcp.Input(
+			}, struct{}]("required", "", testToolHandler, mcp.Input(
 				mcp.Property("x", mcp.Required(true)))),
 			&jsonschema.Schema{
 				Type:     "object",
@@ -73,10 +71,10 @@
 			},
 		},
 		{
-			mcp.NewTool("set_schema", "", testToolHandler[struct {
+			mcp.NewTool[struct {
 				X int `json:"x,omitempty"`
 				Y int `json:"y,omitempty"`
-			}], mcp.Input(
+			}, struct{}]("set_schema", "", testToolHandler, mcp.Input(
 				mcp.Schema(&jsonschema.Schema{Type: "object"})),
 			),
 			&jsonschema.Schema{
@@ -90,76 +88,3 @@
 		}
 	}
 }
-
-func TestNewToolValidate(t *testing.T) {
-	// Check that the tool returned from NewTool properly validates its input schema.
-
-	type req struct {
-		I int
-		B bool
-		S string `json:",omitempty"`
-		P *int   `json:",omitempty"`
-	}
-
-	dummyHandler := func(context.Context, *mcp.ServerSession, *mcp.CallToolParams[req]) (*mcp.CallToolResult, error) {
-		return nil, nil
-	}
-
-	tool := mcp.NewTool("test", "test", dummyHandler)
-	for _, tt := range []struct {
-		desc string
-		args map[string]any
-		want string // error should contain this string; empty for success
-	}{
-		{
-			"both required",
-			map[string]any{"I": 1, "B": true},
-			"",
-		},
-		{
-			"optional",
-			map[string]any{"I": 1, "B": true, "S": "foo"},
-			"",
-		},
-		{
-			"wrong type",
-			map[string]any{"I": 1.5, "B": true},
-			"cannot unmarshal",
-		},
-		{
-			"extra property",
-			map[string]any{"I": 1, "B": true, "C": 2},
-			"unknown field",
-		},
-		{
-			"value for pointer",
-			map[string]any{"I": 1, "B": true, "P": 3},
-			"",
-		},
-		{
-			"null for pointer",
-			map[string]any{"I": 1, "B": true, "P": nil},
-			"",
-		},
-	} {
-		t.Run(tt.desc, func(t *testing.T) {
-			raw, err := json.Marshal(tt.args)
-			if err != nil {
-				t.Fatal(err)
-			}
-			_, err = tool.Handler(context.Background(), nil,
-				&mcp.CallToolParams[json.RawMessage]{Arguments: json.RawMessage(raw)})
-			if err == nil && tt.want != "" {
-				t.Error("got success, wanted failure")
-			}
-			if err != nil {
-				if tt.want == "" {
-					t.Fatalf("failed with:\n%s\nwanted success", err)
-				}
-				if !strings.Contains(err.Error(), tt.want) {
-					t.Fatalf("got:\n%s\nwanted to contain %q", err, tt.want)
-				}
-			}
-		})
-	}
-}

Change information

Files:

• M gopls/internal/mcp/context.go
• M gopls/internal/mcp/mcp.go
• M internal/mcp/client.go
• M internal/mcp/client_test.go
• M internal/mcp/cmd_test.go
• M internal/mcp/examples/hello/main.go
• M internal/mcp/examples/sse/main.go
• M internal/mcp/features_test.go
• M internal/mcp/generate.go
• M internal/mcp/internal/readme/client/client.go
• M internal/mcp/internal/readme/server/server.go
• M internal/mcp/mcp_test.go
• M internal/mcp/protocol.go
• M internal/mcp/server.go
• M internal/mcp/server_example_test.go
• M internal/mcp/shared.go
• M internal/mcp/shared_test.go
• M internal/mcp/sse_example_test.go
• M internal/mcp/sse_test.go
• M internal/mcp/tool.go
• M internal/mcp/tool_test.go

Change size: L

Delta: 21 files changed, 264 insertions(+), 246 deletions(-)

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 1

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

[Jonathan Amsterdam (Gerrit)]

Attention needed from Jonathan Amsterdam, Robert Findley and Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #2 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Jonathan Amsterdam
• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 2

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Jonathan Amsterdam <j...@google.com>

[Jonathan Amsterdam (Gerrit)]

Attention needed from Jonathan Amsterdam, Robert Findley and Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #3 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Jonathan Amsterdam
• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 3

[Robert Findley (Gerrit)]

Attention needed from Jonathan Amsterdam and Sam Thanawalla

Robert Findley added 3 comments

File internal/mcp/protocol.go

Line 33, Patchset 3 (Latest): Arguments any `json:"arguments,omitempty"`

Robert Findley . unresolved

I don't know why this isn't just json.RawMessage, when the spec says it is arbitrary JSON. Using any means that our users aren't able to control their own client-side unmarshalling.

If we want convenience for the client0side, we can keep the CallTool helper (or perhaps better named: CallToolFor).

I think when things are JSON, we should use json.RawMessage in the protocol layer.
Then you don't need rawHandler.

File internal/mcp/tool.go

Line 25, Patchset 3 (Latest): CallToolParams

Robert Findley . unresolved

No need to embed. Let's just generate two types?

Line 60, Patchset 3 (Latest):func NewTool[TReq, TRes any](name, description string, handler ToolHandlerFor[TReq, TRes], opts ...ToolOption) *ServerTool {

Robert Findley . unresolved

Just 'In' and 'Out', similar to genkit? No need for the T prefix
(this is my style, but I think it's wrong).

Open in Gerrit

Related details

Attention is currently required from:

• Jonathan Amsterdam
• Sam Thanawalla

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 3

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Jonathan Amsterdam <j...@google.com>

Gerrit-Comment-Date: Fri, 06 Jun 2025 21:41:51 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam added 3 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Jonathan Amsterdam . resolved

Looks like I confused myself. There was supposed to be another CL with no generics. But if you like this, then it doesn't matter.

File internal/mcp/protocol.go

Line 33, Patchset 3 (Latest): Arguments any `json:"arguments,omitempty"`

Robert Findley . resolved

I don't know why this isn't just json.RawMessage, when the spec says it is arbitrary JSON. Using any means that our users aren't able to control their own client-side unmarshalling.

If we want convenience for the client0side, we can keep the CallTool helper (or perhaps better named: CallToolFor).

I think when things are JSON, we should use json.RawMessage in the protocol layer.
Then you don't need rawHandler.

Jonathan Amsterdam

Done, but I disagree.

Besides user conveinece (we would definitely need a helper somewhere, or people will get annoyed calling json.Marshal and checking the error), there is the double unmarshal on the server (one for validation).

We could export ServerTool.noValidate, but then we're going even further down the path of optimization over convenience.

File internal/mcp/tool.go

Line 60, Patchset 3 (Latest):func NewTool[TReq, TRes any](name, description string, handler ToolHandlerFor[TReq, TRes], opts ...ToolOption) *ServerTool {

Robert Findley . resolved

Just 'In' and 'Out', similar to genkit? No need for the T prefix
(this is my style, but I think it's wrong).

Jonathan Amsterdam

Glad to hear that! Done.

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 3

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Sat, 07 Jun 2025 20:11:38 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Robert Findley <rfin...@google.com>

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam added 1 comment

File internal/mcp/tool.go

Line 25, Patchset 3: CallToolParams

Robert Findley . resolved

No need to embed. Let's just generate two types?

Jonathan Amsterdam

Done

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 4

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Sat, 07 Jun 2025 20:17:18 +0000

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #4 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam added 1 comment

File internal/mcp/protocol.go

Line 33, Patchset 3: Arguments any `json:"arguments,omitempty"`

Robert Findley . resolved

I don't know why this isn't just json.RawMessage, when the spec says it is arbitrary JSON. Using any means that our users aren't able to control their own client-side unmarshalling.

If we want convenience for the client0side, we can keep the CallTool helper (or perhaps better named: CallToolFor).

I think when things are JSON, we should use json.RawMessage in the protocol layer.
Then you don't need rawHandler.

Jonathan Amsterdam

Done, but I disagree.

Besides user conveinece (we would definitely need a helper somewhere, or people will get annoyed calling json.Marshal and checking the error), there is the double unmarshal on the server (one for validation).

We could export ServerTool.noValidate, but then we're going even further down the path of optimization over convenience.

Jonathan Amsterdam

If CallToolParams.Arguments is `any`, then the client can still set it to a json.RawMessage, but they can also use anything else json-Marshalable.

On the server, if a tool has no input schema, then Arguments will contain a json.RawMessage, but if it does, then Arguments will have a map[string]any.
So a tool can control its validation, and there is no double unmarshal.
(If that's too obscure for you, we can add a NoValidate bool to ServerTool.)

(This was the way my lost, no-generic CL worked, with the addition that there was no generic ToolHandlerFor: instead, CallToolParams.Arguments was set to the unmarshaled value of NewTool's `In` type parameter, and tool authors had to downcast.)

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 4

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Sun, 08 Jun 2025 10:56:58 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Robert Findley <rfin...@google.com>

Comment-In-Reply-To: Jonathan Amsterdam <j...@google.com>

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #5 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 5

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam added 1 comment

File internal/mcp/protocol.go

Line 33, Patchset 3: Arguments any `json:"arguments,omitempty"`

Robert Findley . unresolved

I don't know why this isn't just json.RawMessage, when the spec says it is arbitrary JSON. Using any means that our users aren't able to control their own client-side unmarshalling.

If we want convenience for the client0side, we can keep the CallTool helper (or perhaps better named: CallToolFor).

I think when things are JSON, we should use json.RawMessage in the protocol layer.
Then you don't need rawHandler.

Jonathan Amsterdam

Done, but I disagree.

Besides user conveinece (we would definitely need a helper somewhere, or people will get annoyed calling json.Marshal and checking the error), there is the double unmarshal on the server (one for validation).

We could export ServerTool.noValidate, but then we're going even further down the path of optimization over convenience.

Jonathan Amsterdam

If CallToolParams.Arguments is `any`, then the client can still set it to a json.RawMessage, but they can also use anything else json-Marshalable.

On the server, if a tool has no input schema, then Arguments will contain a json.RawMessage, but if it does, then Arguments will have a map[string]any.
So a tool can control its validation, and there is no double unmarshal.
(If that's too obscure for you, we can add a NoValidate bool to ServerTool.)

(This was the way my lost, no-generic CL worked, with the addition that there was no generic ToolHandlerFor: instead, CallToolParams.Arguments was set to the unmarshaled value of NewTool's `In` type parameter, and tool authors had to downcast.)

Jonathan Amsterdam

In bullet points ("Arguments" is `CallToolParams.Arguments`):

1. Arguments is `any`. Client can set to anything, including RawMessage.
2. Server tool with nil InputSchema has its Arguments set to RawMessage. They must downcast, unmarshal and validate.
3. Server tool with non-nil InputSchema has Arguments set to map[string]any,; they need to downcast. Validation done by us.

I considered the alternative where (1) is the same, but on the server side, there is only CallToolParams[In] and ToolHandler[In, Out], and the tool author can pick any type including RawMessage and map[string]any. But I think this is confusing when InputSchema is non-nil and `In` is RawMessage:

• So we should unmarshal, validate, then hand you the original RawMessage?
• Or should we ignore InputSchema (perhaps it's only for doc, when the client displays available tools) and give you the RawMessage unvalidated?

I think selecting behavior based on InputSchema == nil is clearer, at the cost of a downcast.

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 5

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Mon, 09 Jun 2025 11:01:49 +0000

[Robert Findley (Gerrit)]

Attention needed from Jonathan Amsterdam and Sam Thanawalla

Robert Findley added 3 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Robert Findley . resolved

I think my comments may have disrupted this CL even further. Let's discuss over VC.

File internal/mcp/client.go

Line 298, Patchset 5 (Parent):func CallTool[TArgs any](ctx context.Context, cs *ClientSession, params *CallToolParams[TArgs]) (*CallToolResult, error) {

Robert Findley . unresolved

Why not keep CallToolFor?

File internal/mcp/examples/sse/main.go

Line 23, Patchset 5 (Latest): return &mcp.CallToolResultFor[string]{

Robert Findley . unresolved

The "string" here doesn't matter, right? We should make that clear, since it looks like it's returning a string response. Maybe make it 'any'.

Same comment throughout, since this is just a placeholder until output schemas land.

Open in Gerrit

Related details

Attention is currently required from:

• Jonathan Amsterdam
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 5

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Jonathan Amsterdam <j...@google.com>

Gerrit-Comment-Date: Mon, 09 Jun 2025 16:06:34 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #6 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 6

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

[Jonathan Amsterdam (Gerrit)]

Attention needed from Robert Findley and Sam Thanawalla

Jonathan Amsterdam added 3 comments

File internal/mcp/client.go

Line 298, Patchset 5 (Parent):func CallTool[TArgs any](ctx context.Context, cs *ClientSession, params *CallToolParams[TArgs]) (*CallToolResult, error) {

Robert Findley . resolved

Why not keep CallToolFor?

Jonathan Amsterdam

Acknowledged

File internal/mcp/examples/sse/main.go

Line 23, Patchset 5: return &mcp.CallToolResultFor[string]{

Robert Findley . resolved

The "string" here doesn't matter, right? We should make that clear, since it looks like it's returning a string response. Maybe make it 'any'.

Same comment throughout, since this is just a placeholder until output schemas land.

Jonathan Amsterdam

Done

File internal/mcp/protocol.go

Line 33, Patchset 3: Arguments any `json:"arguments,omitempty"`

Robert Findley . resolved

I don't know why this isn't just json.RawMessage, when the spec says it is arbitrary JSON. Using any means that our users aren't able to control their own client-side unmarshalling.

If we want convenience for the client0side, we can keep the CallTool helper (or perhaps better named: CallToolFor).

I think when things are JSON, we should use json.RawMessage in the protocol layer.
Then you don't need rawHandler.

Jonathan Amsterdam

Done, but I disagree.

Besides user conveinece (we would definitely need a helper somewhere, or people will get annoyed calling json.Marshal and checking the error), there is the double unmarshal on the server (one for validation).

We could export ServerTool.noValidate, but then we're going even further down the path of optimization over convenience.

Jonathan Amsterdam

If CallToolParams.Arguments is `any`, then the client can still set it to a json.RawMessage, but they can also use anything else json-Marshalable.

On the server, if a tool has no input schema, then Arguments will contain a json.RawMessage, but if it does, then Arguments will have a map[string]any.
So a tool can control its validation, and there is no double unmarshal.
(If that's too obscure for you, we can add a NoValidate bool to ServerTool.)

(This was the way my lost, no-generic CL worked, with the addition that there was no generic ToolHandlerFor: instead, CallToolParams.Arguments was set to the unmarshaled value of NewTool's `In` type parameter, and tool authors had to downcast.)

Jonathan Amsterdam

In bullet points ("Arguments" is `CallToolParams.Arguments`):

1. Arguments is `any`. Client can set to anything, including RawMessage.
2. Server tool with nil InputSchema has its Arguments set to RawMessage. They must downcast, unmarshal and validate.
3. Server tool with non-nil InputSchema has Arguments set to map[string]any,; they need to downcast. Validation done by us.

I considered the alternative where (1) is the same, but on the server side, there is only CallToolParams[In] and ToolHandler[In, Out], and the tool author can pick any type including RawMessage and map[string]any. But I think this is confusing when InputSchema is non-nil and `In` is RawMessage:

• So we should unmarshal, validate, then hand you the original RawMessage?
• Or should we ignore InputSchema (perhaps it's only for doc, when the client displays available tools) and give you the RawMessage unvalidated?

I think selecting behavior based on InputSchema == nil is clearer, at the cost of a downcast.

Jonathan Amsterdam

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

• Robert Findley
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 6

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Mon, 09 Jun 2025 20:43:39 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Robert Findley (Gerrit)]

Attention needed from Jonathan Amsterdam and Sam Thanawalla

Robert Findley voted and added 3 comments

Votes added by Robert Findley

Code-Review

+2

3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Robert Findley . resolved

Thanks. I think this is the best solution.

File internal/mcp/server.go

Line 144, Patchset 6 (Latest): // if st.Tool.OutputSchema != nil {


// st.outputResolved, err := st.Tool.OutputSchema.Resolve(nil)


// if err != nil {


// return err
// }
// }

Robert Findley . unresolved

If you want to leave this placeholder, it should also have a comment explaining the commented code (the new spec isn't out yet!).

File internal/mcp/tool.go

Line 61, Patchset 6 (Latest): // oschema, err := jsonschema.For[TRes]()


// if err != nil {


// return nil, err
// }

Robert Findley . unresolved

Ditto here: leave a comment explaining the commented code.

Open in Gerrit

Related details

Attention is currently required from:

• Jonathan Amsterdam
• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 6

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Jonathan Amsterdam <j...@google.com>

Gerrit-Comment-Date: Mon, 09 Jun 2025 21:31:33 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Jonathan Amsterdam (Gerrit)]

Attention needed from Sam Thanawalla

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #7 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 7

[Jonathan Amsterdam (Gerrit)]

Attention needed from Sam Thanawalla

Jonathan Amsterdam added 2 comments

File internal/mcp/server.go

Line 144, Patchset 6: // if st.Tool.OutputSchema != nil {


// st.outputResolved, err := st.Tool.OutputSchema.Resolve(nil)
// if err != nil {
// return err
// }
// }

Robert Findley . resolved

If you want to leave this placeholder, it should also have a comment explaining the commented code (the new spec isn't out yet!).

Jonathan Amsterdam

Done

File internal/mcp/tool.go

Line 61, Patchset 6: // oschema, err := jsonschema.For[TRes]()


// if err != nil {
// return nil, err
// }

Robert Findley . resolved

Ditto here: leave a comment explaining the commented code.

Jonathan Amsterdam

Done

Open in Gerrit

Related details

Attention is currently required from:

• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 7

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Tue, 10 Jun 2025 11:14:33 +0000

Gerrit-HasComments: Yes

[Jonathan Amsterdam (Gerrit)]

Attention needed from Sam Thanawalla

Jonathan Amsterdam voted Auto-Submit+1

Auto-Submit

+1

Open in Gerrit

Related details

Attention is currently required from:

• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 7

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Tue, 10 Jun 2025 11:14:54 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Jonathan Amsterdam (Gerrit)]

Attention needed from Sam Thanawalla

Jonathan Amsterdam voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 7

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Tue, 10 Jun 2025 11:22:51 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Jonathan Amsterdam (Gerrit)]

Attention needed from Sam Thanawalla

Jonathan Amsterdam voted and added 1 comment

Votes added by Jonathan Amsterdam

TryBot-Bypass

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Jonathan Amsterdam . resolved

bypassing failing TestFlightRecorder in gopls/internal/test/integration/web.

Open in Gerrit

Related details

Attention is currently required from:

• Sam Thanawalla

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 7

Gerrit-Owner: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Jonathan Amsterdam <j...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Reviewer: Sam Thanawalla <samtha...@google.com>

Gerrit-Attention: Sam Thanawalla <samtha...@google.com>

Gerrit-Comment-Date: Tue, 10 Jun 2025 11:57:44 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Jonathan Amsterdam (Gerrit)]

Jonathan Amsterdam submitted the change with unreviewed changes

Unreviewed changes

6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: internal/mcp/server.go
Insertions: 1, Deletions: 0.

@@ -141,6 +141,7 @@
 				st.inputResolved = r
 			}
 
+			// TODO: uncomment when output schemas drop.

 			// if st.Tool.OutputSchema != nil {
 			// 	st.outputResolved, err := st.Tool.OutputSchema.Resolve(nil)
 			// 	if err != nil {

```
```
The name of the file: internal/mcp/tool.go
Insertions: 1, Deletions: 0.

@@ -58,6 +58,7 @@
 	if err != nil {
 		return nil, err
 	}
+	// TODO: uncomment when output schemas drop.

 	// oschema, err := jsonschema.For[TRes]()
 	// if err != nil {
 	// 	return nil, err

```

Change information

Commit message:

internal/mcp: reorganize the tool API, keeping generics

CallToolParams.Arguments is any.

Clients can send whatever they want.

Tool authors who create their own ToolHandler (bypassing NewTool)
will receive the arguments as a map[string]any which has been
validated against the input schema.

Tool authors who call NewTool can choose the type that the arguments
unmarshal into.

There is no double-unmarshaling and no downcasting.

There is no way for tool authors to avoid the unmarshal, and no way to
avoid the validation unless the omit an input schema, which is probably
bad for LLMs. If tool authors want this optimization, we can provide
it later.

Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Reviewed-on: https://go-review.googlesource.com/c/tools/+/679595

TryBot-Bypass: Jonathan Amsterdam <j...@google.com>

Auto-Submit: Jonathan Amsterdam <j...@google.com>

Reviewed-by: Robert Findley <rfin...@google.com>

Files:

• M gopls/internal/mcp/context.go
• M gopls/internal/mcp/mcp.go
• M gopls/internal/test/marker/marker_test.go
• M internal/mcp/README.md

• M internal/mcp/client.go
• M internal/mcp/client_test.go
• M internal/mcp/cmd_test.go
• M internal/mcp/examples/hello/main.go
• M internal/mcp/examples/sse/main.go
• M internal/mcp/features_test.go
• M internal/mcp/generate.go
• M internal/mcp/internal/readme/client/client.go
• M internal/mcp/internal/readme/server/server.go
• M internal/mcp/mcp_test.go
• M internal/mcp/protocol.go
• M internal/mcp/server.go
• M internal/mcp/server_example_test.go

• M internal/mcp/shared_test.go
• M internal/mcp/sse_example_test.go
• M internal/mcp/sse_test.go
• M internal/mcp/tool.go
• M internal/mcp/tool_test.go

Change size: L

Delta: 22 files changed, 337 insertions(+), 240 deletions(-)

Branch: refs/heads/master

Submit Requirements:

• Code-Review: +2 by Robert Findley
• TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI, TryBot-Bypass+1 by Jonathan Amsterdam

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ieb201e56c7fb8a23bb1a93b27e946630ec9e79ad

Gerrit-Change-Number: 679595

Gerrit-PatchSet: 8