From 669bd34b215fe764734c2468121f4d77a0f71efa Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 5 Dec 2025 18:11:49 -0500 Subject: [PATCH] mcp: don't treat pointers differently given an OutputSchema override In the fix for #199, I was overly cautious: I assumed that if the user provided their own schema, we shouldn't fix up typed nils for them. But this was wrong: consider that: 1. If the user provides a schema, it must be an "object" schema. 2. One way that we recommend customizing schemas is by calling jsonschema.For and modifying. It's very confusing that doing this would change the treatment of the zero value. The good news is that any tool that was returning a typed nil is provably wrong, so this is just a bug that we can and should fix. Fixes #691 --- mcp/mcp_test.go | 79 ++++++++++++++++++++++++++++--------------------- mcp/server.go | 17 ++++------- 2 files changed, 50 insertions(+), 46 deletions(-) diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 8c40de08..32f40a10 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -1902,16 +1902,20 @@ func TestPointerArgEquivalence(t *testing.T) { type input struct { In string `json:",omitempty"` } + inputSchema := json.RawMessage(`{"type":"object","properties":{"In":{"type":"string"}},"additionalProperties":false}`) type output struct { Out string } + outputSchema := json.RawMessage(`{"type":"object","required":["Out"],"properties":{"Out":{"type":"string"}},"additionalProperties":false}`) cs, _, cleanup := basicConnection(t, func(s *Server) { - // Add two equivalent tools, one of which operates in the 'pointer' realm, - // the other of which does not. + // Add three equivalent tools: + // - one operates on pointers, with inferred schemas + // - one operates on pointers, with user-provided schemas + // - one operates on non-pointers // // 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 *CallToolRequest, in *input) (*CallToolResult, *output, error) { + handlePointers := func(_ context.Context, req *CallToolRequest, in *input) (*CallToolResult, *output, error) { switch in.In { case "": return nil, nil, fmt.Errorf("must provide input") @@ -1924,7 +1928,13 @@ func TestPointerArgEquivalence(t *testing.T) { default: panic("unreachable") } - }) + } + AddTool(s, &Tool{Name: "pointer-inferred"}, handlePointers) + AddTool(s, &Tool{ + Name: "pointer-provided", + InputSchema: inputSchema, + OutputSchema: outputSchema, + }, handlePointers) AddTool(s, &Tool{Name: "nonpointer"}, func(_ context.Context, req *CallToolRequest, in input) (*CallToolResult, output, error) { switch in.In { case "": @@ -1947,50 +1957,51 @@ func TestPointerArgEquivalence(t *testing.T) { if err != nil { t.Fatal(err) } - if got, want := len(tools.Tools), 2; got != want { + if got, want := len(tools.Tools), 3; 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) + t0 := tools.Tools[0] + for _, t1 := range 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(r0, r1, ctrCmpOpts...); diff != "" { - t.Errorf("CallTool(%v) with no arguments mismatch (-%s +%s):\n%s", args, 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 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}}) + // 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: input{In: in}}) + r1, err := cs.CallTool(ctx, &CallToolParams{Name: t1.Name, Arguments: args}) if err != nil { t.Fatal(err) } if diff := cmp.Diff(r0, r1, ctrCmpOpts...); diff != "" { - t.Errorf("CallTool({\"In\": %q}) mismatch (-%s +%s):\n%s", in, t0.Name, t1.Name, 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, ctrCmpOpts...); diff != "" { + t.Errorf("CallTool({\"In\": %q}) mismatch (-%s +%s):\n%s", in, t0.Name, t1.Name, diff) + } + }) + } } } diff --git a/mcp/server.go b/mcp/server.go index d4317222..6e2be0dd 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -359,21 +359,14 @@ func toolForErr[In, Out any](t *Tool, h ToolHandlerFor[In, Out]) (*Tool, ToolHan // Pointers are treated equivalently to non-pointers when deriving the schema. // If an indirection occurred to derive the schema, a non-nil zero value is // returned to be used in place of the typed nil zero value. -// -// Note that if sfield already holds a schema, zero will be nil even if T is a -// pointer: if the user provided the schema, they may have intentionally -// derived it from the pointer type, and handling of zero values is up to them. -// -// TODO(rfindley): we really shouldn't ever return 'null' results. Maybe we -// should have a jsonschema.Zero(schema) helper? func setSchema[T any](sfield *any, rfield **jsonschema.Resolved) (zero any, err error) { + rt := reflect.TypeFor[T]() + if rt.Kind() == reflect.Pointer { + rt = rt.Elem() + zero = reflect.Zero(rt).Interface() + } var internalSchema *jsonschema.Schema if *sfield == nil { - rt := reflect.TypeFor[T]() - if rt.Kind() == reflect.Pointer { - rt = rt.Elem() - zero = reflect.Zero(rt).Interface() - } // TODO: we should be able to pass nil opts here. internalSchema, err = jsonschema.ForType(rt, &jsonschema.ForOptions{}) if err == nil {