feat/Issue-13: elicitation support #188
Conversation
krtkvrm
changed the title
krtkvrm
changed the title
|
The PR comment is too wordy. See other PR comments by jba, findleyr and samthanawalla to get the style. The example is useful but it should be an actual example. See the files named |
krtkvrm
force-pushed
the
feat/issue-13-elicitation-support
branch
from
7ce6c7d to
498871c
Compare
mcp/client.go
Outdated
| // TODO: wrap or annotate this error? Pick a standard code? | ||
| return nil, &jsonrpc2.WireError{Code: CodeUnsupportedMethod, Message: "client does not support elicitation"} | ||
| } | ||
| return c.opts.ElicitationHandler(ctx, cs, params) |
There was a problem hiding this comment.
A good place to check the restriction on the schema.
There was a problem hiding this comment.
added, please check if its as expected
There was a problem hiding this comment.
@jba Correct me if I'm wrong but I believe we can also check that the ElicitResult.Content matches the Params.RequestSchema before returning.
It would look like this:
r, err := params.RequestedSchema.Resolve(nil)
res, err := c.opts.ElicitationHandler(ctx, cs, params)
err := r.Validate(res.Content)
if err != nil {
panic(elicitationhandler violated the requestedschema)
}
There was a problem hiding this comment.
Seems about right. Instead of panicking can the RPC return an error?
There was a problem hiding this comment.
This would pass the error to the server instead of the client but I think it should be a client side error since it originated from the ElicitationHandler. What do you think?
There was a problem hiding this comment.
There's no way to surface the error on the client.
I think this is what we do with other handlers.
|
@krtkvrm It would be great to get this in soon if you are able to resolve all the comments, thanks in advance! |
krtkvrm
force-pushed
the
feat/issue-13-elicitation-support
branch
from
ed7c7f2 to
824e0ce
Compare
| } | ||
|
|
||
| // validateElicitSchema validates that the schema only contains top-level properties without nesting. | ||
| func validateElicitSchema(schema *jsonschema.Schema) error { |
There was a problem hiding this comment.
Actually, I just read the spec and there is more to check. Let's check it all.
https://modelcontextprotocol.io/specification/2025-06-18/client/elicitation#supported-schema-types
mcp/elicitation_example_test.go
Outdated
| return &mcp.ElicitResult{ | ||
| Action: "accept", | ||
| Content: map[string]any{ | ||
| "apiKey": "sk-test123", |
There was a problem hiding this comment.
Probably don't want to use this field as an example since it is prohibited in the spec:
https://modelcontextprotocol.io/specification/2025-06-18/client/elicitation#user-interaction-model
|
@krtkvrm can you wrap this up by tomorrow, Thursday Aug 20? We would like it to land before our next release, which will probably be Friday. |
|
@krtkvrm Will you be able to complete it today? We want to release this feature soon so I will take it over unless you say otherwise. (And give you credit) Thank you! |
|
I'll be pushing changes in next few hours, sorry for delays, I am currently travelling |
|
Thanks. We're hoping to tag tomorrow, so if it would help you, we'd still be happy to take over. We will of course add a Co-Authored-By line to give you credit. |
|
LGTM once you resolve the conflicts, thanks! |
krtkvrm
force-pushed
the
feat/issue-13-elicitation-support
branch
from
9e488ed to
87d139d
Compare
|
@samthanawalla @jba @findleyr thanks for helping with reviews on this PR. again sorry for delays, I was caught up with something personal. Please feel free to take over if the deadline is close and things are pending. Looking forward to contribute more :) |
|
Thanks @krtkvrm! Do you think you could look into the staticcheck lint failure? It looks trivial, and is blocking our merging. |
|
pushed the fix for lint |
fixes: #13
Implements elicitation functionality from MCP 2025-06-18 specification.
Changes
Tests