mcp: add ClientCapabilities.Roots.Supported #608
Conversation
Address an API oversight (modelcontextprotocol#607) by exposing a Supported field on the ClientCapabilities.Roots. Given our promise of compatibility, we unfortunately can't change the Roots field, so this may be the best we can do to work around this oversight.
|
I'm not sure it was an oversight. Our client always supports roots, even if the user doesn't add any. |
|
I assume that servers using the Go SDK should support any client that correctly implements the spec (including clients that lack support of roots) |
|
@jba I'm guessing that the origin of the mistake was that this type was internal only, for use by the client. In that case, it was acceptable to have inlined structs. But when capabilities were exported it became an API oversight, because we need to be able to represent whether the roots capability is present on the client. |
There was a problem hiding this comment.
The inclusion of this file is a great idea- but is warts part of an internal coding style? I'd prefer it called known_issues.md but that's just my 2¢.
WRT the content of the file, I'd suggest creating a PR with the inclusion of the file, and then update the content to add each entry in a different PR, instead of adding the file and the 2 entries here.
There was a problem hiding this comment.
Thanks for the feedback! Yes, I had intended to change the name of this file -- "warts" is a term I've hear before, but isn't nice.
"known issues" is good, but makes me think of "bugs we haven't yet fixed" rather than "API decisions we can't revert". In #609 I went with "rough edges", but let me know what you think.
| // 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:"-"` |
There was a problem hiding this comment.
If this is added, then I prefer that the other capabilities structs have a similar Supported bool field added, to keep the API consistent. (or just remove this Supported workaround field, and break backwards compatibility, because it's a bug)
Right now, my server must assume the client supports Sampling if ClientCapabilities.Sampling != nil, which is fine, but for simplicity I would like all capability fields to behave similarly.
|
How about this option? I'm not totally sure it would work. Add a Roots2 field, and move the json:roots tag to that field. Its type is a pointer. Change our client to populate Roots2. At least this removes the asymmetry between Roots and the other capabilities. Servers have to know to check Roots2, but is this so different from them having to know to check Supported? |
|
Technically speaking any change to struct tags is a breaking API change, no? I realized that I removed omitempty in this CL, and probably can't even do that. |
Excellent point. The spec says (Italics mine):
|
3 participants
Address an API oversight (#607) by exposing a Supported field on the ClientCapabilities.Roots.
Given our promise of compatibility, we unfortunately can't change the Roots field, so this may be the best we can do to work around this oversight.