-
Notifications
You must be signed in to change notification settings - Fork 51
Add service extension for sending a sampling request to the MCP server #325
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
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
jakemac53
left a comment
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 but see my suggestion about simplifying this a bunch
| await dtd.registerService( | ||
| McpServiceConstants.serviceName, | ||
| McpServiceConstants.samplingRequest, | ||
| _handleSamplingRequest, | ||
| ); |
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 just also realized this should probably be guarded behind a check that the client actually supports sampling, something like:
| await dtd.registerService( | |
| McpServiceConstants.serviceName, | |
| McpServiceConstants.samplingRequest, | |
| _handleSamplingRequest, | |
| ); | |
| if (clientCapabilities.sampling != null) { | |
| await dtd.registerService( | |
| McpServiceConstants.serviceName, | |
| McpServiceConstants.samplingRequest, | |
| _handleSamplingRequest, | |
| ); | |
| } |
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.
Also, do we need to make sure this service isn't already registered on DTD by some other client?
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.
Oof good point! Done.
Re checking if another client has already registered it - can there be multiple Dart MCP servers connected to the same DTD connection? I didn't think there could. I don't know why another client would have registered it on DTD.
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.
There can be yes - if multiple chat clients connect to the DTD instance started by an IDE.
For instance if somebody has two instances of Gemini CLI running at once for some reason, or two different chat bots.
|
Also just to clarify I am fine with landing this as is and following up with something to handle multiple clients trying to register the same method, if that is easier |
Good because that was my plan 😄 |
Work towards #326