Client sampling and roots capabilities set to None if not implemented #802
Conversation
|
@ihrpr I think this is an important PR to make sure the SDK adheres to the protocol. |
src/mcp/client/session.py
Outdated
| roots = None | ||
| else: | ||
| roots = types.RootsCapability( | ||
| # TODO: Should this be based on whether we |
There was a problem hiding this comment.
The spec says:
When roots change, clients that support listChanged MUST send a notification:
So I guess it would be the former.
There was a problem hiding this comment.
Totally agree!
This TODO comment was already in the code base, I just kept it...
I only added the if-else structure around it.
|
@ihrpr I've addressed your comments, but since there's a new commit, we need your approval again. Btw, I want to implement a larger feature, but before actually implementing it and opening a PR, I'd love to chat with you just to make sure it makes sense. Is there a way I can reach out to you @ihrpr ? |
| else None | ||
| ) | ||
| roots = ( | ||
| types.RootsCapability(listChanged=True) |
There was a problem hiding this comment.
i'm curious - does this entail that any client that supports roots, or list_roots, supports roots.listChanged?
There was a problem hiding this comment.
Maybe worth putting the comment back, sorry.
The client can send list changes, as the code is there to trigger that notification, so it's supported.
But the spec says "listChanged indicates whether the client will emit notifications when the list of roots changes."
We do need to address this, but a separate PR would be probably be better
There was a problem hiding this comment.
I don't know if I follow...
So I read the comment as asking - should the listChanged=True be included if and only if the client WILL send a notification when the roots list is changed, or that it MAY (i.e., it "supports it"). And I think I even said roughly "this is answered in the spec":
listChanged indicates whether the client will emit notifications when the list of roots changes.
So if that's the meaning of the question/comment, I think it's answered by the spec. My question was - if a client SUPPORTS ROOTS (e.g. they support "roots/list"), does it follow that they support "roots/list_changed"?
That's MY question, and it seems to me that the spec implies NO, otherwise (a) why would this be need to be announced in the capabilities; (b) why would the spec talk about "whether the client will emit notifications" if its non-optional.
Cuz the client could (a) not support roots; (b) support roots, and therefore support "roots/list", but not support "notifications/roots/list_changed" (c) support roots, "roots/list" and listChanged.
In the spec it sort of visually implies that SUPPORTS ROOTS -> SUPPORTS ROOTS/LIST -> SUPPORTS NOTIFICATIONS/ROOTS/LIST_CHANGED. But I don't believe this is actually stated anywhere.
So my case is - a client may or may not support roots full stop. If they support roots, then they must support "roots/list" - but that's not something that's declared in the capabilities - but I don't see any language that says that any client that SUPPORTS ROOTS also supports roots/list_changed.
So my reading of: "Clients that support roots MUST declare the roots capability during initialization" is that the existence of the "roots" key indicates support for roots (and therefore list-roots), and the value for roots.listChanged indicates support for list_CHANGED. So the capabilities object could have:
(1) no roots support:
"capabilities": {
"sampling": {}
},
(2) supports roots (and "roots/list") but not listChanged:
"capabilities": {
"roots": {
"listChanged": false
},
"sampling": {}
},
(3) supports roots, "roots/list" and listChanged:
"capabilities": {
"roots": {
"listChanged": true
},
"sampling": {}
},
I think this is consistent with the schema.ts:
export interface ClientCapabilities {
/**
* Experimental, non-standard capabilities that the client supports.
*/
experimental?: { [key: string]: object };
/**
* Present if the client supports listing roots.
*/
roots?: {
/**
* Whether the client supports notifications for changes to the roots list.
*/
listChanged?: boolean;
};
/**
* Present if the client supports sampling from an LLM.
*/
sampling?: object;
}
capabilities.roots is optional and is present if the client supports listing roots. capabilities.roots.listChanged is optional and is true if the client support "roots/list_changed" and false if the client supports "roots/list" but not "roots/list_changed".
FWIW, I think this is slightly non-optimal and that listChanged?: boolean should be listChanged: boolean, the "meaning" overal being: if you support roots at all, then you must support "roots/list" (hence, this does not need to be announced), but it's still yes/no if you support "roots/list_changed" (hence that must be announced).
The current structure seems to allow:
...
roots: {}
...
which I guess is equivalent to:
...
roots: {
listChanged: false
}
...
SO - on my reading, there are three states - (a) no roots (omit the key), (b) "roots/list" support but not "roots/list_changed" (include key and object value and either omit roots.listChanged or set it to false, and (c) supports "roots/list_changed" (include roots.listChanged: true) - in case (c), the client MUST send "notifications/roots/list_changed" when list changes. This also comports with this:
Category | Capability | Description
-------- -------- -------
Client | roots | Ability to provide filesystem roots
Meaning the presence of the roots key is optional and indicates, if present that the client (at least) supports "roots/list".
THEN, there is the further announcement of whether they ALSO supports "roots/list_changed", as in:
Capability objects can describe sub-capabilities like:
* `listChanged`: Support for list change notifications...
Pulling it together, the comment is asking: if the client SUPPORTS "roots/list_changed", does it follow that they MAY or that they MUST actually send the notifications?
My question was: if a client SUPPORTS roots, does it follow that they support listChanged - (a) does it follow given the logic in this PR? (b) is that what the spec declares? For (b) I say "no", so if the PR entails "yes", then I believe it is against the spec.
|
@ihrpr I want to implement a pretty large feature, but before actually implementing it and opening a PR, I'd love to chat with you. Is there a way I can reach out to you? |
oh, sorry, missed this. I think the best option would be to open an issue and we can discuss there |
|
Sounds good! @ihpr Btw I am working on a fix for the |
thanks @lorenzocesconetto ! hope I haven't added to the confusion, please see my last comment/essay that I posted for a thorough recap of my position |
|
@hesreallyhim imo your understanding is right. The client capabilities declaration was obviously not following the protocol specs, that's why I opened this PR in the first place. This PR addressed only part of the issue, as you've noted the hardcoded piece |
Right now clients always claims to support sampling and roots capabilities, which is inaccurate.
Having this properly set allows servers to fallback to alternative implementations.
Motivation and Context
Servers might rely on the correctness of client capabilities, so clients should inform them accurately.
How Has This Been Tested?
Unit tests
Breaking Changes
No
Types of changes
Checklist
Additional context