mcp: minor cleanup of some TODOs (modelcontextprotocol#332)

Do a pass through TODOs to delete or address minor TODOs.

Author: findleyr

mcp/client.go

@@ -226,7 +226,6 @@ func (c *Client) AddRoots(roots ...*Root) {
 // RemoveRoots removes the roots with the given URIs,
 // and notifies any connected servers if the list has changed.
 // It is not an error to remove a nonexistent root.
-// TODO: notification
 func (c *Client) RemoveRoots(uris ...string) {
 	changeAndNotify(c, notificationRootsListChanged, &RootsListChangedParams{},
 		func() bool { return c.roots.remove(uris...) })

mcp/features.go

@@ -17,7 +17,9 @@ import (
 // A featureSet is a collection of features of type T.
 // Every feature has a unique ID, and the spec never mentions
 // an ordering for the List calls, so what it calls a "list" is actually a set.
-// TODO: switch to an ordered map
+//
+// An alternative implementation would use an ordered map, but that's probably
+// not necessary as adds and removes are rare, and usually batched.
 type featureSet[T any] struct {
 	uniqueID   func(T) string
 	features   map[string]T

mcp/server.go

@@ -89,17 +89,18 @@ type ServerOptions struct {
 // The first argument must not be nil.
 //
 // If non-nil, the provided options are used to configure the server.
-func NewServer(impl *Implementation, opts *ServerOptions) *Server {
+func NewServer(impl *Implementation, options *ServerOptions) *Server {
 	if impl == nil {
 		panic("nil Implementation")
 	}
-	if opts == nil {
-		opts = new(ServerOptions)
+	var opts ServerOptions
+	if options != nil {
+		opts = *options
 	}
+	options = nil // prevent reuse
 	if opts.PageSize < 0 {
 		panic(fmt.Errorf("invalid page size %d", opts.PageSize))
 	}
-	// TODO(jba): don't modify opts, modify Server.opts.
 	if opts.PageSize == 0 {
 		opts.PageSize = DefaultPageSize
 	}
@@ -111,7 +112,7 @@ func NewServer(impl *Implementation, opts *ServerOptions) *Server {
 	}
 	return &Server{
 		impl:                    impl,
-		opts:                    *opts,
+		opts:                    opts,
 		prompts:                 newFeatureSet(func(p *serverPrompt) string { return p.prompt.Name }),
 		tools:                   newFeatureSet(func(t *serverTool) string { return t.tool.Name }),
 		resources:               newFeatureSet(func(r *serverResource) string { return r.resource.URI }),
@@ -463,7 +464,7 @@ func fileResourceHandler(dir string) ResourceHandler {
 	return func(ctx context.Context, req *ServerRequest[*ReadResourceParams]) (_ *ReadResourceResult, err error) {
 		defer util.Wrapf(&err, "reading resource %s", req.Params.URI)
 
-		// TODO: use a memoizing API here.
+		// TODO(#25): use a memoizing API here.
 		rootRes, err := req.Session.ListRoots(ctx, nil)
 		if err != nil {
 			return nil, fmt.Errorf("listing roots: %w", err)

mcp/shared.go

@@ -4,8 +4,10 @@
 
 // This file contains code shared between client and server, including
 // method handler and middleware definitions.
-// TODO: much of this is here so that we can factor out commonalities using
-// generics. Perhaps it can be simplified with reflection.
+//
+// Much of this is here so that we can factor out commonalities using
+// generics. If this becomes unwieldy, it can perhaps be simplified with
+// reflection.
 
 package mcp
 

mcp/streamable.go

@@ -1152,7 +1152,6 @@ func (c *streamableClientConn) Write(ctx context.Context, msg jsonrpc.Message) e
 	}
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		// TODO: do a best effort read of the body here, and format it in the error.
 		resp.Body.Close()
 		return fmt.Errorf("broken session: %v", resp.Status)
 	}