-
Notifications
You must be signed in to change notification settings - Fork 670
Pass request _meta to request handlers extra param #328
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
Conversation
This is helpful, can this be merged @cliffhall ? |
+1 for this. Looks like there's a conflict that needs to fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ProgressToken gets its own schema, why not the request metadata? Certainly helps with handling that request meta object on its own, since it gets its own inferred type. LGTM! 👍
Does the progressToken make sense, actually? There's already a relatedRequestId passed through by sendNotification—maybe that should be used instead of a separate progress token?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you
@iamarcel please can you fix the build and then we can merge? |
Yes! Rebased it on the latest version—the merge before messed up. |
Motivation and Context
The MCP client sends a progress token when calling tools, but there's currently no way for the server (in this SDK) to retrieve this progress token. While the client will receive the progress notifications, it won't associate them with the tool calls and thus it can't properly report on progress.
How Has This Been Tested?
In the inspector, of course, and my chat MCP client (SSE server).
Here's the setup, roughly:
Breaking Changes
None.
Types of changes
Checklist
Additional context
This is the smallest possible way to implement the functionality in the SDK, but I'm not convinced on the ergonomics as a user.
I'm open to hear your ideas:
sendProgress(progress: number, total?:number)
in theextra
params instead? The Python SDK has areport_progress
that it passes to the request context.progressToken
make sense, actually? There's already arelatedRequestId
passed through bysendNotification
—maybe that should be used instead of a separate progress token?