fix: include previously-resolved variables in completions request #598
Conversation
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
Minor comments:
- 1 typo
- one reference should include prompts and the owner/repo could easily be prompt arguments to, so it's the same problem and the field should be used for both.
I don't want the comment to imply it's only for URIs
connor4312
force-pushed
the
connor4312/597
branch
from
74b6abe to
e5d3133
Compare
Refs modelcontextprotocol/modelcontextprotocol#598 Also fixes a bug I noticed where we sometimes didn't put templates in the quickpick
|
Ah right. Fixed that. Also kept it to just say "resolved variables" rather than URI template expressions since template variables seems like the more-correct thing to fill in (an expression can reference multiple variables which should be completed independently) |
connor4312
force-pushed
the
connor4312/597
branch
from
9507eac to
d412d56
Compare
Refs modelcontextprotocol/modelcontextprotocol#598 Also fixes a bug I noticed where we sometimes didn't put templates in the quickpick
|
We've adopted this in VS Code and the Github MCP server and it's working well for us. Would love to see it ratified 🙂 |
dsp-ant
left a comment
dsp-ant
left a comment
There was a problem hiding this comment.
I am okay with this change. Please update the documentation for the completion specification accordingly and mention how this is intended to be used. I think it would be worthwhile to notice how clients should handle the absence of context if they rely on it (given that it's optional)
5 participants
Motivation and Context
See #597
This PR suggests a way to provide them. I'm not a big fan of the naming so if someone with stronger opinions has a point of view let me know 😛
How Has This Been Tested?
We implemented this as an experimental feature in VS Code's nightly build to resolve GH MCP's above scenario.
Breaking Changes
N/A
Types of changes
Checklist