-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: add streaming methods for elicitation and sampling #1210
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?
feat: add streaming methods for elicitation and sampling #1210
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
f8755d8 to
7520307
Compare
7520307 to
4ed61b6
Compare
|
Doing some manual testing on this, converted back to draft temporarily |
|
Validated with new example tool. |
ba50e0a to
771f101
Compare
…ript-sdk into feat/elicitation-sampling-streaming
|
…ript-sdk into LucaButBoring-feat/elicitation-sampling-streaming # Conflicts: # examples/client/src/simpleStreamableHttp.ts
|
@LucaButBoring could you resolve the conflicts on this PR? Hoping to get it merged soon. |
…ript-sdk into feat/elicitation-sampling-streaming
- Replace schema-based setRequestHandler with string methods - Update context API from extra.taskStore to extra.task?.store - Add CreateTaskResult to ResultTypeMap for elicitation/create sampling/createMessage
|
@cliffhall updated 👍 |
| type ServerWithCapabilities = { | ||
| getClientCapabilities(): { elicitation?: { form?: boolean; url?: boolean } } | undefined; | ||
| }; | ||
| const clientCapabilities = (this._server as unknown as ServerWithCapabilities).getClientCapabilities(); |
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.
Hi, could we achieve this without the as unknown? It's something we're actively trying to avoid. Is there something not matching on the ClientCapabilities.elicitation that we need to fix here? (them not being a boolean but an object | undefined)? Would rather fix that than have a proprietary casting here, basically forfeiting the type system fully for it
The checks bellow would pass even if elicitation.form or elicitation.url were objects and not boolean, so is that really needed
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.
Oh, I think that was due to other type inference failures around _server at the time this was originally written which are no longer applicable, I'm doing a scrub for this again.
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.
FWIW the hard casts are needed around getTask and getTaskResult still either way, and that won't be addressed until #1449 is merged.
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.
Alright, updated - this cast and the similar ones in sampling are no longer needed and have been removed.
Following #1041, this adds streaming methods for elicitation and sampling to simplify usage with Tasks.
Motivation and Context
Introduces a consistent way to use Tasks with elicitation and sampling just like we have with tool calls.
How Has This Been Tested?
New unit tests and example tool.
Breaking Changes
N/A
Types of changes
Checklist
Additional context