diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 58b0377e..4e74dad5 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -938,4 +938,39 @@ 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) { + // 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() + + ctx := context.Background() + res, err := cs.ListTools(ctx, nil) + if err != nil { + 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"} 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