internal/mcp: avoid JSON nulls on the wire

Add a test that checks for JSON null on the wire, and fix two categories
of bugs leading to null:

1. In many places, we could pass typed nil where we shouldn't have. Add
   an orZero helper to avoid typed nills.
2. When listing roots, we should avoid nil.

Fixes golang/go#74105

Change-Id: I061420487dbfcbf9016cb94b39bd72270e871c0e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/682675
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

mcp/client.go

@@ -167,7 +167,7 @@ func (c *Client) RemoveRoots(uris ...string) {
 		func() bool { return c.roots.remove(uris...) })
 }
 
-// changeAndNotify is called when a feature is added or removed.
+// changeAndNotifyClient is called when a feature is added or removed.
 // It calls change, which should do the work and report whether a change actually occurred.
 // If there was a change, it notifies a snapshot of the sessions.
 func (c *Client) changeAndNotify(notification string, params Params, change func() bool) {
@@ -184,8 +184,12 @@ func (c *Client) changeAndNotify(notification string, params Params, change func
 func (c *Client) listRoots(_ context.Context, _ *ClientSession, _ *ListRootsParams) (*ListRootsResult, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
+	roots := slices.Collect(c.roots.all())
+	if roots == nil {
+		roots = []*Root{} // avoid JSON null
+	}
 	return &ListRootsResult{
-		Roots: slices.Collect(c.roots.all()),
+		Roots: roots,
 	}, nil
 }
 
@@ -272,28 +276,31 @@ func (*ClientSession) ping(context.Context, *PingParams) (*emptyResult, error) {
 
 // Ping makes an MCP "ping" request to the server.
 func (cs *ClientSession) Ping(ctx context.Context, params *PingParams) error {
-	_, err := handleSend[*emptyResult](ctx, cs, methodPing, params)
+	_, err := handleSend[*emptyResult](ctx, cs, methodPing, orZero[Params](params))
 	return err
 }
 
 // ListPrompts lists prompts that are currently available on the server.
 func (cs *ClientSession) ListPrompts(ctx context.Context, params *ListPromptsParams) (*ListPromptsResult, error) {
-	return handleSend[*ListPromptsResult](ctx, cs, methodListPrompts, params)
+	return handleSend[*ListPromptsResult](ctx, cs, methodListPrompts, orZero[Params](params))
 }
 
 // GetPrompt gets a prompt from the server.
 func (cs *ClientSession) GetPrompt(ctx context.Context, params *GetPromptParams) (*GetPromptResult, error) {
-	return handleSend[*GetPromptResult](ctx, cs, methodGetPrompt, params)
+	return handleSend[*GetPromptResult](ctx, cs, methodGetPrompt, orZero[Params](params))
 }
 
 // ListTools lists tools that are currently available on the server.
 func (cs *ClientSession) ListTools(ctx context.Context, params *ListToolsParams) (*ListToolsResult, error) {
-	return handleSend[*ListToolsResult](ctx, cs, methodListTools, params)
+	return handleSend[*ListToolsResult](ctx, cs, methodListTools, orZero[Params](params))
 }
 
 // CallTool calls the tool with the given name and arguments.
 // The arguments can be any value that marshals into a JSON object.
 func (cs *ClientSession) CallTool(ctx context.Context, params *CallToolParams) (*CallToolResult, error) {
+	if params == nil {
+		params = new(CallToolParams)
+	}
 	if params.Arguments == nil {
 		// Avoid sending nil over the wire.
 		params.Arguments = map[string]any{}
@@ -302,23 +309,23 @@ func (cs *ClientSession) CallTool(ctx context.Context, params *CallToolParams) (
 }
 
 func (cs *ClientSession) SetLevel(ctx context.Context, params *SetLevelParams) error {
-	_, err := handleSend[*emptyResult](ctx, cs, methodSetLevel, params)
+	_, err := handleSend[*emptyResult](ctx, cs, methodSetLevel, orZero[Params](params))
 	return err
 }
 
 // ListResources lists the resources that are currently available on the server.
 func (cs *ClientSession) ListResources(ctx context.Context, params *ListResourcesParams) (*ListResourcesResult, error) {
-	return handleSend[*ListResourcesResult](ctx, cs, methodListResources, params)
+	return handleSend[*ListResourcesResult](ctx, cs, methodListResources, orZero[Params](params))
 }
 
 // ListResourceTemplates lists the resource templates that are currently available on the server.
 func (cs *ClientSession) ListResourceTemplates(ctx context.Context, params *ListResourceTemplatesParams) (*ListResourceTemplatesResult, error) {
-	return handleSend[*ListResourceTemplatesResult](ctx, cs, methodListResourceTemplates, params)
+	return handleSend[*ListResourceTemplatesResult](ctx, cs, methodListResourceTemplates, orZero[Params](params))
 }
 
 // ReadResource ask the server to read a resource and return its contents.
 func (cs *ClientSession) ReadResource(ctx context.Context, params *ReadResourceParams) (*ReadResourceResult, error) {
-	return handleSend[*ReadResourceResult](ctx, cs, methodReadResource, params)
+	return handleSend[*ReadResourceResult](ctx, cs, methodReadResource, orZero[Params](params))
 }
 
 func (c *Client) callToolChangedHandler(ctx context.Context, s *ClientSession, params *ToolListChangedParams) (Result, error) {

mcp/client_test.go

@@ -120,7 +120,6 @@ func TestClientPaginateBasic(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			listFunc := func(ctx context.Context, params *ListTestParams) (*ListTestResult, error) {
 				if len(tc.results) == 0 {

mcp/mcp_test.go

@@ -40,8 +40,6 @@ func sayHi(ctx context.Context, ss *ServerSession, params *CallToolParamsFor[hiP
 func TestEndToEnd(t *testing.T) {
 	ctx := context.Background()
 	var ct, st Transport = NewInMemoryTransports()
-	// ct = NewLoggingTransport(ct, os.Stderr)
-	// st = NewLoggingTransport(st, os.Stderr)
 
 	// Channels to check if notification callbacks happened.
 	notificationChans := map[string]chan int{}
@@ -659,6 +657,7 @@ func TestCancellation(t *testing.T) {
 func TestMiddleware(t *testing.T) {
 	ctx := context.Background()
 	ct, st := NewInMemoryTransports()
+
 	s := NewServer("testServer", "v1.0.0", nil)
 	ss, err := s.Connect(ctx, st)
 	if err != nil {
@@ -743,6 +742,69 @@ R1 <roots/list
 	}
 }
 
+type safeBuffer struct {
+	mu  sync.Mutex
+	buf bytes.Buffer
+}
+
+func (b *safeBuffer) Write(data []byte) (int, error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.buf.Write(data)
+}
+
+func (b *safeBuffer) Bytes() []byte {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.buf.Bytes()
+}
+
+func TestNoJSONNull(t *testing.T) {
+	ctx := context.Background()
+	var ct, st Transport = NewInMemoryTransports()
+
+	// Collect logs, to sanity check that we don't write JSON null anywhere.
+	var logbuf safeBuffer
+	ct = NewLoggingTransport(ct, &logbuf)
+
+	s := NewServer("testServer", "v1.0.0", nil)
+	ss, err := s.Connect(ctx, st)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	c := NewClient("testClient", "v1.0.0", nil)
+	cs, err := c.Connect(ctx, ct)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := cs.ListTools(ctx, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := cs.ListPrompts(ctx, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := cs.ListResources(ctx, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := cs.ListResourceTemplates(ctx, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := ss.ListRoots(ctx, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	cs.Close()
+	ss.Wait()
+
+	logs := logbuf.Bytes()
+	if i := bytes.Index(logs, []byte("null")); i >= 0 {
+		start := max(i-20, 0)
+		end := min(i+20, len(logs))
+		t.Errorf("conformance violation: MCP logs contain JSON null: %s", "..."+string(logs[start:end])+"...")
+	}
+}
+
 // traceCalls creates a middleware function that prints the method before and after each call
 // with the given prefix.
 func traceCalls[S Session](w io.Writer, prefix string) Middleware[S] {

mcp/server.go

@@ -501,12 +501,12 @@ func (ss *ServerSession) Ping(ctx context.Context, params *PingParams) error {
 
 // ListRoots lists the client roots.
 func (ss *ServerSession) ListRoots(ctx context.Context, params *ListRootsParams) (*ListRootsResult, error) {
-	return handleSend[*ListRootsResult](ctx, ss, methodListRoots, params)
+	return handleSend[*ListRootsResult](ctx, ss, methodListRoots, orZero[Params](params))
 }
 
 // CreateMessage sends a sampling request to the client.
 func (ss *ServerSession) CreateMessage(ctx context.Context, params *CreateMessageParams) (*CreateMessageResult, error) {
-	return handleSend[*CreateMessageResult](ctx, ss, methodCreateMessage, params)
+	return handleSend[*CreateMessageResult](ctx, ss, methodCreateMessage, orZero[Params](params))
 }
 
 // LoggingMessage sends a logging message to the client.

mcp/shared.go

@@ -25,8 +25,7 @@ import (
 // A MethodHandler handles MCP messages.
 // For methods, exactly one of the return values must be nil.
 // For notifications, both must be nil.
-type MethodHandler[S Session] func(
-	ctx context.Context, _ S, method string, params Params) (result Result, err error)
+type MethodHandler[S Session] func(ctx context.Context, _ S, method string, params Params) (result Result, err error)
 
 // A methodHandler is a MethodHandler[Session] for some session.
 // We need to give up type safety here, or we will end up with a type cycle somewhere
@@ -73,6 +72,15 @@ func defaultSendingMethodHandler[S Session](ctx context.Context, session S, meth
 	return res, nil
 }
 
+// Helper methods to avoid typed nil.
+func orZero[T any, P *U, U any](p P) T {
+	if p == nil {
+		var zero T
+		return zero
+	}
+	return any(p).(T)
+}
+
 func handleNotify[S Session](ctx context.Context, session S, method string, params Params) error {
 	mh := session.sendingMethodHandler().(MethodHandler[S])
 	_, err := mh(ctx, session, method, params)
@@ -141,8 +149,13 @@ type methodInfo struct {
 // A typedMethodHandler is like a MethodHandler, but with type information.
 type typedMethodHandler[S Session, P Params, R Result] func(context.Context, S, P) (R, error)
 
+type paramsPtr[T any] interface {
+	*T
+	Params
+}
+
 // newMethodInfo creates a methodInfo from a typedMethodHandler.
-func newMethodInfo[S Session, P Params, R Result](d typedMethodHandler[S, P, R]) methodInfo {
+func newMethodInfo[S Session, P paramsPtr[T], R Result, T any](d typedMethodHandler[S, P, R]) methodInfo {
 	return methodInfo{
 		unmarshalParams: func(m json.RawMessage) (Params, error) {
 			var p P
@@ -151,9 +164,12 @@ func newMethodInfo[S Session, P Params, R Result](d typedMethodHandler[S, P, R])
 					return nil, fmt.Errorf("unmarshaling %q into a %T: %w", m, p, err)
 				}
 			}
-			return p, nil
+			return orZero[Params](p), nil
 		},
 		handleMethod: MethodHandler[S](func(ctx context.Context, session S, _ string, params Params) (Result, error) {
+			if params == nil {
+				return d(ctx, session, nil)
+			}
 			return d(ctx, session, params.(P))
 		}),
 		// newResult is used on the send side, to construct the value to unmarshal the result into.

mcp/testdata/conformance/server/resources.txtar

@@ -101,8 +101,7 @@ info.txt
 {
 	"jsonrpc": "2.0",
 	"id": 1,
-	"method": "roots/list",
-	"params": null
+	"method": "roots/list"
 }
 {
 	"jsonrpc": "2.0",