diff --git a/docs/warts.md b/docs/warts.md new file mode 100644 index 00000000..572439f6 --- /dev/null +++ b/docs/warts.md @@ -0,0 +1,17 @@ + +# "Warts" - API oversights to reconsider for v2 + +This file collects a list of API oversights or rough edges that we've uncovered +post v1.0, both to address them a potential future v2, and to document their +current workarounds. + +- `EventStore.Open` is unnecessary. This was an artifact of an earlier version + of the SDK where event persistence and delivery were combined. + + **Workaround**: `Open` may be implemented as a no-op. + +- `ClientCapabilities.Roots` should have been a distinguished struct pointer. + This was simply an oversight. + + **Workaround**: the `ClientCapabilities.Roots.Supported` field is managed by + custom marshaling of `ClientCapabilities`. diff --git a/internal/docs/doc.go b/internal/docs/doc.go index 7b23ad63..aa2e9b99 100644 --- a/internal/docs/doc.go +++ b/internal/docs/doc.go @@ -7,6 +7,7 @@ //go:generate weave -o ../../docs/protocol.md ./protocol.src.md //go:generate weave -o ../../docs/client.md ./client.src.md //go:generate weave -o ../../docs/server.md ./server.src.md +//go:generate weave -o ../../docs/warts.md ./warts.src.md //go:generate weave -o ../../docs/troubleshooting.md ./troubleshooting.src.md // The doc package generates the documentation at /doc, via go:generate. diff --git a/internal/docs/warts.src.md b/internal/docs/warts.src.md new file mode 100644 index 00000000..961ccdd4 --- /dev/null +++ b/internal/docs/warts.src.md @@ -0,0 +1,16 @@ +# "Warts" - API oversights to reconsider for v2 + +This file collects a list of API oversights or rough edges that we've uncovered +post v1.0, both to address them a potential future v2, and to document their +current workarounds. + +- `EventStore.Open` is unnecessary. This was an artifact of an earlier version + of the SDK where event persistence and delivery were combined. + + **Workaround**: `Open` may be implemented as a no-op. + +- `ClientCapabilities.Roots` should have been a distinguished struct pointer. + This was simply an oversight. + + **Workaround**: the `ClientCapabilities.Roots.Supported` field is managed by + custom marshaling of `ClientCapabilities`. diff --git a/mcp/client.go b/mcp/client.go index d7e3ae5a..0762bd87 100644 --- a/mcp/client.go +++ b/mcp/client.go @@ -117,6 +117,7 @@ type ClientSessionOptions struct{} func (c *Client) capabilities() *ClientCapabilities { caps := &ClientCapabilities{} + caps.Roots.Supported = true caps.Roots.ListChanged = true if c.opts.CreateMessageHandler != nil { caps.Sampling = &SamplingCapabilities{} diff --git a/mcp/client_test.go b/mcp/client_test.go index eaeedc81..3192a3b9 100644 --- a/mcp/client_test.go +++ b/mcp/client_test.go @@ -202,9 +202,10 @@ func TestClientCapabilities(t *testing.T) { name: "With initial capabilities", configureClient: func(s *Client) {}, wantCapabilities: &ClientCapabilities{ - Roots: struct { - ListChanged bool "json:\"listChanged,omitempty\"" - }{ListChanged: true}, + Roots: rootsCapabilities{ + Supported: true, + ListChanged: true, + }, }, }, { @@ -216,9 +217,10 @@ func TestClientCapabilities(t *testing.T) { }, }, wantCapabilities: &ClientCapabilities{ - Roots: struct { - ListChanged bool "json:\"listChanged,omitempty\"" - }{ListChanged: true}, + Roots: rootsCapabilities{ + Supported: true, + ListChanged: true, + }, Sampling: &SamplingCapabilities{}, }, }, diff --git a/mcp/protocol.go b/mcp/protocol.go index 1312dfbd..efac3d79 100644 --- a/mcp/protocol.go +++ b/mcp/protocol.go @@ -183,17 +183,71 @@ func (x *CancelledParams) SetProgressToken(t any) { setProgressToken(x, t) } type ClientCapabilities struct { // Experimental, non-standard capabilities that the client supports. Experimental map[string]any `json:"experimental,omitempty"` - // Present if the client supports listing roots. + + // Roots reports roots capabilities. + // + // Due to an API oversight (#607), roots is a non-pointer. Check the + // Supported field to see if the roots capability is present Roots struct { + // Suppported reports whether roots/list is supported. It works around an + // API oversight: roots capabilities should have been a distinguished type, + // similar to other capabilities. + Supported bool `json:"-"` + // Whether the client supports notifications for changes to the roots list. ListChanged bool `json:"listChanged,omitempty"` - } `json:"roots,omitempty"` - // Present if the client supports sampling from an LLM. + } `json:"roots"` + + // Sampling is present if the client supports sampling from an LLM. Sampling *SamplingCapabilities `json:"sampling,omitempty"` - // Present if the client supports elicitation from the server. + + // Elicitation is present if the client supports elicitation from the server. Elicitation *ElicitationCapabilities `json:"elicitation,omitempty"` } +// clientCapabilitiesV2 is a version of ClientCapabilities fixes +type clientCapabilitiesV2 struct { + Experimental map[string]any `json:"experimental,omitempty"` + Roots *rootsCapabilities `json:"roots,omitempty"` + Sampling *SamplingCapabilities `json:"sampling,omitempty"` + Elicitation *ElicitationCapabilities `json:"elicitation,omitempty"` +} + +// See #607: rootsCapabilities is for internal use, fixing an API oversight. +type rootsCapabilities struct { + Supported bool `json:"-"` + ListChanged bool `json:"listChanged,omitempty"` +} + +func (c ClientCapabilities) MarshalJSON() ([]byte, error) { + c2 := &clientCapabilitiesV2{ + Experimental: c.Experimental, + Sampling: c.Sampling, + Elicitation: c.Elicitation, + } + if c.Roots.Supported { + c2.Roots = &rootsCapabilities{ + ListChanged: c.Roots.ListChanged, + } + } + return json.Marshal(c2) +} + +func (c *ClientCapabilities) UnmarshalJSON(data []byte) error { + var c2 clientCapabilitiesV2 + if err := json.Unmarshal(data, &c2); err != nil { + return err + } + c.Experimental = c2.Experimental + c.Elicitation = c2.Elicitation + c.Sampling = c2.Sampling + if c2.Roots != nil { + c.Roots = *c2.Roots + c.Roots.Supported = true + } + return nil +} + type CompleteParamsArgument struct { // The name of the argument Name string `json:"name"` @@ -1035,8 +1089,6 @@ type ResourceUpdatedNotificationParams struct { func (*ResourceUpdatedNotificationParams) isParams() {} -// TODO(jba): add CompleteRequest and related types. - // A request from the server to elicit additional information from the user via the client. type ElicitParams struct { // This property is reserved by the protocol to allow clients and servers to diff --git a/mcp/protocol_test.go b/mcp/protocol_test.go index 67d021d1..62acc6f4 100644 --- a/mcp/protocol_test.go +++ b/mcp/protocol_test.go @@ -531,3 +531,30 @@ func TestContentUnmarshal(t *testing.T) { var gotpm PromptMessage roundtrip(pm, &gotpm) } + +func TestClientCapabilitiesJSON(t *testing.T) { + tests := []struct { + capabilities ClientCapabilities + want string + }{ + {ClientCapabilities{}, "{}"}, + {ClientCapabilities{Roots: rootsCapabilities{Supported: true}}, `{"roots":{}}`}, + {ClientCapabilities{Roots: rootsCapabilities{Supported: true, ListChanged: true}}, `{"roots":{"listChanged":true}}`}, + } + for _, test := range tests { + gotBytes, err := json.Marshal(test.capabilities) + if err != nil { + t.Fatalf("json.Marshal(%+v) failed: %v", test.capabilities, err) + } + if got := string(gotBytes); got != test.want { + t.Fatalf("json.Marshal(%+v) = %q, want %q", test.capabilities, got, test.want) + } + var caps2 ClientCapabilities + if err := json.Unmarshal(gotBytes, &caps2); err != nil { + t.Fatalf("json.Unmarshal failed: %v", err) + } + if diff := cmp.Diff(test.capabilities, caps2); diff != "" { + t.Errorf("Unmarshal mismatch (-original +unmarshaled):\n%s", diff) + } + } +}