Skip to content

feat: implement support for elicitation #271

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LucaButBoring
Copy link

@LucaButBoring LucaButBoring commented May 27, 2025

Implementation for the new elicitation spec (modelcontextprotocol/modelcontextprotocol#382).

Motivation and Context

Supports elicitation, which is now merged into the spec.

How Has This Been Tested?

Thorough unit and integration tests (matched sampling test cases).

Breaking Changes

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

The Python SDK also has a PR that we can compare against (modelcontextprotocol/python-sdk#625).

The SDK data models for schema objects (used in ElicitRequest) are a bit complicated here - similar to Content, schema objects are polymorphic and have a type property, but this doesn't always allow us to directly select a subtype, requiring us to use a custom deserializer for this. The main issue is EnumSchema, which shares the string type with StringSchema, but has an enum field to distinguish it, which can't be modeled by the JsonSubType annotation. Tests have been implement for both serialization and deserialization for this type to make sure we can handle this correctly.

@LucaButBoring LucaButBoring marked this pull request as ready for review May 28, 2025 18:58
@tzolov
Copy link
Contributor

tzolov commented Jun 5, 2025

Thank you @LucaButBoring,

I will try to review this PR

In the future, please consider the contributing guidelines before implementing a major feature.

For non-trivial changes, please clarify with the maintainers in an issue whether you can contribute the change and the desired scope of the change.

@tzolov tzolov self-assigned this Jun 5, 2025
@tzolov tzolov added enhancement New feature or request client labels Jun 5, 2025
Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @LucaButBoring,
There are minor formatting issues with the import to fix.

I'm wondering if we should enforce PrimitiveSchemaDefinition types in the schema or just use Map<String, Object> instead?
We don't use these schemas for argument validation and the schema builders can still be used as external utilities, what do you think?

I guess we can also implement a dynamic schema generation from Java Class or records. But this will be task for another PR

@LucaButBoring
Copy link
Author

Thanks for reviewing, will fix the formatting issues within the next couple of hours once I get a chance.


Regarding the schema data model, the reason I went with a well-typed representation was because unlike tool input and output schemas, which are simply typed as arbitrary JSONSchema data in the specification, the kind of schema used in elicitation is actually a subset of JSONSchema, and is well-defined in the spec.

In particular, the spec's elicitation schema is strictly one level deep, disallowing object or array properties and having no concept of $defs or $ref, among other limitations. Modeling these restrictions in the SDK directly makes sense IMO, as opposed to full JSONSchema objects which are more or less arbitrary. If you think it still makes sense to drop down to Map in spite of that, I'm happy to change it, though.

We could also potentially reuse the Schema hierarchy (or a subset of it if we add object/array) for modeling JSONSchema generally in the future, too.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
client enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants