-
Notifications
You must be signed in to change notification settings - Fork 176
Add missing title, size and annotation attributes on Resource/ResourceTemplate and other types #219
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
Add missing title, size and annotation attributes on Resource/ResourceTemplate and other types #219
Conversation
faeebc5 to
4cd3f6e
Compare
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.
Pull Request Overview
This PR adds missing attributes and the Annotations type to align the Kotlin SDK with the latest MCP specification. The changes include adding missing attributes to several resource and content types, implementing the new Annotations data class, and ensuring proper serialization support.
- Adds the missing
Annotationstype with validation for priority values - Extends
Resource,ResourceTemplate, and various content types with missing attributes - Includes serialization/deserialization tests for the new
Annotationstype
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Implements the Annotations data class and adds missing attributes to resource and content types |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesTest.kt | Adds test coverage for Annotations serialization/deserialization |
| kotlin-sdk-core/api/kotlin-sdk-core.api | Updates API signatures to reflect the new attributes and Annotations type |
| val priority: Double?, | ||
| ) { | ||
| init { | ||
| require(priority == null || priority in 0.0..1.0) { "Priority must be between 0.0 and 1.0" } |
Copilot
AI
Aug 11, 2025
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.
[nitpick] The validation logic could be more explicit about inclusive bounds. Consider using a more descriptive error message that clarifies both bounds are inclusive: "Priority must be between 0.0 and 1.0 (inclusive)"
| require(priority == null || priority in 0.0..1.0) { "Priority must be between 0.0 and 1.0" } | |
| require(priority == null || priority in 0.0..1.0) { "Priority must be between 0.0 and 1.0 (inclusive)" } |
devcrocod
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.
Thank you!
lgtm
I have just one small question
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Show resolved
Hide resolved
4cd3f6e to
c7c5b5a
Compare
This PR adds:
AnnotationstypeResource.title,Resource.sizeandResource.annotationsResourceTemplate.titleandResourceTemplate.annotationsTextContent.annotationsImageContent.annotationsAudioContent.annotationsEmeddedResource.annotationsFixes #159.
(PS: the solution I described in the issue was wrong. I checked with the specification's schema to be sure of the changes.)
Motivation and Context
Adherence to the latest specification version, cf. #159.
How Has This Been Tested?
I only added a serialization/deserialization test for
Annotationsto ensure that the serializer forkotlin.time.Instantwas indeed present. Cf. below in "Additional Context".Breaking Changes
N/A
Types of changes
Checklist
Additional context
I took two liberties:
Annotations.lastModified, as this must be encoded as an ISO 8601 date.Annotations.priorityto verify that it is between 0 and 1.