mcp: treat pointers equivalently to non-pointers when deriving schema

As reported in #199 and #200, the fact that we return a possibly "null"
schema for pointer types breaks various clients, which expect schemas to
be of type "object". This is an unfortunate footgun. For now, assume
that the user wants us to treat pointers equivalently to non-pointers.
If we want to change this behavior in the future, we can do so behind an
option.

+ a test

Also fix the handling of nil results in the case where the output schema
is non-nil: we must provide structured content in this case. (This was
causing the test to fail).

Fixes #199
Fixes #200

Author: findleyr

mcp/mcp_test.go

@@ -1084,3 +1084,104 @@ func TestNoDistributedDeadlock(t *testing.T) {
 }
 
 var testImpl = &Implementation{Name: "test", Version: "v1.0.0"}
+
+// This test checks that when we use pointer types for tools, we get the same
+// schema as when using the non-pointer types. It is too much of a footgun for
+// there to be a difference (see #199 and #200).
+//
+// If anyone asks, we can add an option that controls how pointers are treated.
+func TestPointerArgEquivalence(t *testing.T) {
+	type input struct {
+		In string
+	}
+	type output struct {
+		Out string
+	}
+	cs, _ := basicConnection(t, func(s *Server) {
+		// Add two equivalent tools, one of which operates in the 'pointer' realm,
+		// the other of which does not.
+		//
+		// We handle a few different types of results, to assert they behave the
+		// same in all cases.
+		AddTool(s, &Tool{Name: "pointer"}, func(_ context.Context, req *ServerRequest[*CallToolParamsFor[*input]]) (*CallToolResultFor[*output], error) {
+			switch req.Params.Arguments.In {
+			case "":
+				return nil, fmt.Errorf("must provide input")
+			case "nil":
+				return nil, nil
+			case "empty":
+				return &CallToolResultFor[*output]{}, nil
+			case "ok":
+				return &CallToolResultFor[*output]{StructuredContent: &output{Out: "foo"}}, nil
+			default:
+				panic("unreachable")
+			}
+		})
+		AddTool(s, &Tool{Name: "nonpointer"}, func(_ context.Context, req *ServerRequest[*CallToolParamsFor[input]]) (*CallToolResultFor[output], error) {
+			switch req.Params.Arguments.In {
+			case "":
+				return nil, fmt.Errorf("must provide input")
+			case "nil":
+				return nil, nil
+			case "empty":
+				return &CallToolResultFor[output]{}, nil
+			case "ok":
+				return &CallToolResultFor[output]{StructuredContent: output{Out: "foo"}}, nil
+			default:
+				panic("unreachable")
+			}
+		})
+	})
+	defer cs.Close()
+
+	ctx := context.Background()
+	tools, err := cs.ListTools(ctx, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, want := len(tools.Tools), 2; got != want {
+		t.Fatalf("got %d tools, want %d", got, want)
+	}
+	t0 := tools.Tools[0]
+	t1 := tools.Tools[1]
+
+	// First, check that the tool schemas don't differ.
+	if diff := cmp.Diff(t0.InputSchema, t1.InputSchema); diff != "" {
+		t.Errorf("input schemas do not match (-%s +%s):\n%s", t0.Name, t1.Name, diff)
+	}
+	if diff := cmp.Diff(t0.OutputSchema, t1.OutputSchema); diff != "" {
+		t.Errorf("output schemas do not match (-%s +%s):\n%s", t0.Name, t1.Name, diff)
+	}
+
+	// Then, check that we handle empty input equivalently.
+	for _, args := range []any{nil, struct{}{}} {
+		r0, err := cs.CallTool(ctx, &CallToolParams{Name: t0.Name, Arguments: args})
+		if err != nil {
+			t.Fatal(err)
+		}
+		r1, err := cs.CallTool(ctx, &CallToolParams{Name: t1.Name, Arguments: args})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if diff := cmp.Diff(r0, r1); diff != "" {
+			t.Errorf("CallTool(%v) with no arguments mismatch (-%s +%s):\n%s", args, t0.Name, t1.Name, diff)
+		}
+	}
+
+	// Then, check that we handle different types of output equivalently.
+	for _, in := range []string{"nil", "empty", "ok"} {
+		t.Run(in, func(t *testing.T) {
+			r0, err := cs.CallTool(ctx, &CallToolParams{Name: t0.Name, Arguments: input{In: in}})
+			if err != nil {
+				t.Fatal(err)
+			}
+			r1, err := cs.CallTool(ctx, &CallToolParams{Name: t1.Name, Arguments: input{In: in}})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if diff := cmp.Diff(r0, r1); diff != "" {
+				t.Errorf("CallTool({\"In\": %q}) mismatch (-%s +%s):\n%s", in, t0.Name, t1.Name, diff)
+			}
+		})
+	}
+}

mcp/tool.go

@@ -42,11 +42,23 @@ type serverTool struct {
 func newServerTool[In, Out any](t *Tool, h ToolHandlerFor[In, Out]) (*serverTool, error) {
 	st := &serverTool{tool: t}
 
-	if err := setSchema[In](&t.InputSchema, &st.inputResolved); err != nil {
+	if _, _, err := setSchema[In](&t.InputSchema, &st.inputResolved); err != nil {
 		return nil, err
 	}
+	// Handling for zero values:
+	//
+	// In some cases (for example, if the response is nil yet we've specified an
+	// output schema), we must use a synthetic zero value.
+	//
+	// If pointer is set, an indirection occured when deriving the schema.
+	var (
+		pointer bool
+		zero    any
+	)
 	if reflect.TypeFor[Out]() != reflect.TypeFor[any]() {
-		if err := setSchema[Out](&t.OutputSchema, &st.outputResolved); err != nil {
+		var err error
+		pointer, zero, err = setSchema[Out](&t.OutputSchema, &st.outputResolved)
+		if err != nil {
 			return nil, err
 		}
 	}
@@ -88,30 +100,56 @@ func newServerTool[In, Out any](t *Tool, h ToolHandlerFor[In, Out]) (*serverTool
 		}
 		var ctr CallToolResult
 		// TODO(jba): What if res == nil? Is that valid?
-		// TODO(jba): if t.OutputSchema != nil, check that StructuredContent is present and validates.
+		// TODO(jba): if t.OutputSchema != nil, check that StructuredContent validates.
 		if res != nil {
 			// TODO(jba): future-proof this copy.
 			ctr.Meta = res.Meta
 			ctr.Content = res.Content
 			ctr.IsError = res.IsError
 			ctr.StructuredContent = res.StructuredContent
+			if pointer {
+				// Avoid typed nil, which will serialize as JSON null.
+				var z Out
+				if any(res.StructuredContent) == any(z) { // bypass comparable check: pointers are comparable
+					ctr.StructuredContent = zero
+				}
+			}
+		} else if t.OutputSchema != nil {
+			// StructuredContent must be present.
+			ctr.StructuredContent = zero
 		}
 		return &ctr, nil
 	}
 
 	return st, nil
 }
 
-func setSchema[T any](sfield **jsonschema.Schema, rfield **jsonschema.Resolved) error {
-	var err error
+// setSchema sets the schema and resolved schema corresponding to the type T.
+//
+// If sfield is nil, the schema is derived from T.
+//
+// Pointers are treated equivalently to non-pointers when deriving the schema.
+// The pointer result whether an indirection occurs to derive the schema.
+//
+// Note that if sfield is non-nil, pointer is false even if T is a pointer: if
+// the user provided the schema, they may have intentionally derived it from
+// the pointer type.
+func setSchema[T any](sfield **jsonschema.Schema, rfield **jsonschema.Resolved) (pointer bool, zero any, err error) {
+	rt := reflect.TypeFor[T]()
 	if *sfield == nil {
-		*sfield, err = jsonschema.For[T](nil)
+		if rt.Kind() == reflect.Pointer {
+			pointer = true
+			rt = rt.Elem()
+		}
+		// TODO: we should be able to pass nil opts here.
+		*sfield, err = jsonschema.ForType(rt, &jsonschema.ForOptions{})
 	}
+	zero = reflect.Zero(rt).Interface()
 	if err != nil {
-		return err
+		return pointer, zero, err
 	}
 	*rfield, err = (*sfield).Resolve(&jsonschema.ResolveOptions{ValidateDefaults: true})
-	return err
+	return pointer, zero, err
 }
 
 // unmarshalSchema unmarshals data into v and validates the result according to