From cce7cde9a74a8e92ca6c2260a5074c48f36c913c Mon Sep 17 00:00:00 2001 From: Kamdyn Shaeffer Date: Wed, 13 Aug 2025 12:48:30 -0400 Subject: [PATCH 1/3] issue-217: duplicate tools should not error --- mcp/mcp_test.go | 26 ++++++++++++++++++++++++++ mcp/tool.go | 22 +++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 58b0377e..ede83bcf 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -938,4 +938,30 @@ func TestKeepAliveFailure(t *testing.T) { t.Errorf("expected connection to be closed by keepalive, but it wasn't. Last error: %v", err) } +func TestAddTool_DuplicateNoPanicAndNoDuplicate(t *testing.T) { + // Adding the same tool pointer twice should not panic and should not + // produce duplicates in the server's tool list. + _, cs := basicConnection(t, func(s *Server) { + tool := &Tool{Name: "dup", InputSchema: &jsonschema.Schema{}} + s.AddTool(tool, nopHandler) + s.AddTool(tool, nopHandler) + }) + defer cs.Close() + + ctx := context.Background() + res, err := cs.ListTools(ctx, nil) + if err != nil { + t.Fatal(err) + } + var count int + for _, tt := range res.Tools { + if tt.Name == "dup" { + count++ + } + } + if count != 1 { + t.Fatalf("expected exactly one 'dup' tool, got %d", count) + } +} + var testImpl = &Implementation{Name: "test", Version: "v1.0.0"} diff --git a/mcp/tool.go b/mcp/tool.go index 15f17e11..ec571b20 100644 --- a/mcp/tool.go +++ b/mcp/tool.go @@ -97,11 +97,31 @@ func setSchema[T any](sfield **jsonschema.Schema, rfield **jsonschema.Resolved) var err error if *sfield == nil { *sfield, err = jsonschema.For[T](nil) + if err != nil { + return err + } } + // Resolve operates with internal state and cannot be called twice on the same + // Schema instance. Deep-copy the schema before resolving to avoid mutating the + // original (which may be reused when adding the same tool again). + cloned, err := func(orig *jsonschema.Schema) (*jsonschema.Schema, error) { + if orig == nil { + return nil, nil + } + b, err := json.Marshal(orig) + if err != nil { + return nil, err + } + var cp jsonschema.Schema + if err := json.Unmarshal(b, &cp); err != nil { + return nil, err + } + return &cp, nil + }(*sfield) if err != nil { return err } - *rfield, err = (*sfield).Resolve(&jsonschema.ResolveOptions{ValidateDefaults: true}) + *rfield, err = cloned.Resolve(&jsonschema.ResolveOptions{ValidateDefaults: true}) return err } From 4ee891be251fdfd813028bea21a01d71f60d3ef5 Mon Sep 17 00:00:00 2001 From: Kamdyn Shaeffer Date: Wed, 13 Aug 2025 13:24:24 -0400 Subject: [PATCH 2/3] mcp/mcp_test.go: expanded test case and clarified in comments --- mcp/mcp_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index ede83bcf..4e74dad5 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -942,9 +942,13 @@ func TestAddTool_DuplicateNoPanicAndNoDuplicate(t *testing.T) { // Adding the same tool pointer twice should not panic and should not // produce duplicates in the server's tool list. _, cs := basicConnection(t, func(s *Server) { - tool := &Tool{Name: "dup", InputSchema: &jsonschema.Schema{}} - s.AddTool(tool, nopHandler) - s.AddTool(tool, nopHandler) + // Use two distinct Tool instances with the same name but different + // descriptions to ensure the second replaces the first + // This case was written specifically to reproduce a bug where duplicate tools where causing jsonschema errors + t1 := &Tool{Name: "dup", Description: "first", InputSchema: &jsonschema.Schema{}} + t2 := &Tool{Name: "dup", Description: "second", InputSchema: &jsonschema.Schema{}} + s.AddTool(t1, nopHandler) + s.AddTool(t2, nopHandler) }) defer cs.Close() @@ -954,14 +958,19 @@ func TestAddTool_DuplicateNoPanicAndNoDuplicate(t *testing.T) { t.Fatal(err) } var count int + var gotDesc string for _, tt := range res.Tools { if tt.Name == "dup" { count++ + gotDesc = tt.Description } } if count != 1 { t.Fatalf("expected exactly one 'dup' tool, got %d", count) } + if gotDesc != "second" { + t.Fatalf("expected replaced tool to have description %q, got %q", "second", gotDesc) + } } var testImpl = &Implementation{Name: "test", Version: "v1.0.0"} From ba16a27801accf1e1637f477024a01865cbe70cd Mon Sep 17 00:00:00 2001 From: Kamdyn Shaeffer Date: Thu, 14 Aug 2025 13:00:47 -0400 Subject: [PATCH 3/3] mcp/server.go: changed approach to handling this error --- mcp/server.go | 9 +++++++++ mcp/tool.go | 22 +--------------------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/mcp/server.go b/mcp/server.go index e39372dc..b33b8a22 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -171,6 +171,15 @@ func AddTool[In, Out any](s *Server, t *Tool, h ToolHandlerFor[In, Out]) { func addToolErr[In, Out any](s *Server, t *Tool, h ToolHandlerFor[In, Out]) (err error) { defer util.Wrapf(&err, "adding tool %q", t.Name) + // If the exact same Tool pointer has already been registered under this name, + // avoid rebuilding schemas and re-registering. This prevents duplicate + // registration from causing errors (and unnecessary work). + s.mu.Lock() + if existing, ok := s.tools.get(t.Name); ok && existing.tool == t { + s.mu.Unlock() + return nil + } + s.mu.Unlock() st, err := newServerTool(t, h) if err != nil { return err diff --git a/mcp/tool.go b/mcp/tool.go index ec571b20..15f17e11 100644 --- a/mcp/tool.go +++ b/mcp/tool.go @@ -97,31 +97,11 @@ func setSchema[T any](sfield **jsonschema.Schema, rfield **jsonschema.Resolved) var err error if *sfield == nil { *sfield, err = jsonschema.For[T](nil) - if err != nil { - return err - } } - // Resolve operates with internal state and cannot be called twice on the same - // Schema instance. Deep-copy the schema before resolving to avoid mutating the - // original (which may be reused when adding the same tool again). - cloned, err := func(orig *jsonschema.Schema) (*jsonschema.Schema, error) { - if orig == nil { - return nil, nil - } - b, err := json.Marshal(orig) - if err != nil { - return nil, err - } - var cp jsonschema.Schema - if err := json.Unmarshal(b, &cp); err != nil { - return nil, err - } - return &cp, nil - }(*sfield) if err != nil { return err } - *rfield, err = cloned.Resolve(&jsonschema.ResolveOptions{ValidateDefaults: true}) + *rfield, err = (*sfield).Resolve(&jsonschema.ResolveOptions{ValidateDefaults: true}) return err }