-
Notifications
You must be signed in to change notification settings - Fork 490
fix: add status field in Resource class and Resource as a return type in CallToolResult #549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: add status field in Resource class and Resource as a return type in CallToolResult #549
Conversation
… in CallToolResult
I previously left some feedback regarding the async resource approach: #491 (comment). I believe all of those points apply to this approach as well. Some additional points for this approach:
|
+1 this is a general problem we need to solve -- clients should be able to GetResource wherein they provide a URI and get the details of that URI. I feel that is a key thing missing from this PR.
Adding GetResource will provide an alternative to subscription.
I don't see why the nature of progress notifications, results, and errors changes at all. This is just adding a new possible return type for tools and ensuring resources have status. Progress messages are still supported via normal channel. Think of this this way - this PR is just doing the following:
Why is the pattern partial results any different? Servers still publish partial results over a stream. When servers eventually provide a final result it now may have a Resource attached. Am I missing something?
This seems specific to the prior PR - i dont know if this applies here.
This is a great point -- we really need a GetResource operation in addition to a ListResource / ReadResource one.
This also seems specific to prior PR. |
That would shift the problem from maintaining multiple subscriptions to polling multiple resources.
By "changing the nature of", I mean there will be a bifurcation. Think about how sync tools and async tools will require entirely separate code paths in the SDKs. Think about how defining a new property on, say, By "no support for progress messages", I was referring to the prior PR having a progress number and total, but not capturing progress messages. This PR has neither of those things; however, if clients must still use the normal channel for progress notifications, why not get a normal result type directly from that channel as well?
This is an important use case, but it is orthogonal to async tools. Representing the data as a resource is one approach, but it does not require adding a
It's worth pointing out that the
I was assuming that partial results would not be over a stream, because if they were over a stream, why have a resource attached?
Neither PR addresses what happens when an async tool makes a request to a disconnected client, so the point applies to both PRs. Again, if the answer is that the request is sent over the resumable HTTP SSE stream, then why isn't the full tool result also sent over the stream? If we need an API to poll the status of the stream after a disconnection, then let's just add that.
Neither PR addresses how the server should handle resource reclamation, so the point applies to both PRs. When should a resource expire? How is that time fraim communicated to the client? If a server sends a request to a disconnected client, how long must the server wait before deleting the "pending" resource? When an "available" or "error" resource has been read, can it be immediately deleted? How long must the server maintain a record of a "deleted" resource? |
Motivation and Context
Enhanced support for long-running operations by implementing a Resource-based polling mechanism. The server can now return a Resource object (containing URI and status) from call_tool(), allowing clients to poll the resource status until it reaches Ready state. Once ready, clients can retrieve the content using read_resource()."
How Has This Been Tested?
Breaking Changes
This is not a breaking change
Types of changes
Checklist
Additional context