-
Couldn't load subscription status.
- Fork 172
158 support resource links #244
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
base: main
Are you sure you want to change the base?
158 support resource links #244
Conversation
23d2726 to
c3746a4
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 support for ResourceLink ContentBlock type to align with the MCP specification, refactoring the existing PromptMessageContent type hierarchy to use the more structured ContentBlock interface.
- Introduces ResourceLink as a new ContentBlock type for linking to external resources
- Refactors type hierarchy from PromptMessageContent to ContentBlock with specialized content types
- Maintains backwards compatibility by deprecating old interfaces while keeping them functional
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Adds ResourceLink class and ContentBlock interface hierarchy |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.util.kt | Updates polymorphic serializers for new ContentBlock types |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesTest.kt | Adds tests for ResourceLink and updates existing tests |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesUtilTest.kt | Updates serialization tests for ContentBlock types |
| kotlin-sdk-core/api/kotlin-sdk-core.api | API documentation for new types and interfaces |
| CONTRIBUTING.md | Minor update to build instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesTest.kt
Outdated
Show resolved
Hide resolved
6589ccf to
5136548
Compare
…be/mcp-kotlin-sdk into 158-support-resource-links
…links # Conflicts: # kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractPromptIntegrationTest.kt
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.
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Outdated
Show resolved
Hide resolved
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.
I have a few questions regarding this PR.
Could you please take a moment to answer them?
| * Represents the types of a ContentBlock | ||
| */ | ||
| @Serializable(with = ContentBlockPolymorphicSerializer::class) | ||
| public sealed interface ContentBlock : PromptMessageContent |
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.
You’re introducing a new ContentBlock interface, but still extending a deprecated one.
In my opinion, that’s not the best approach
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.
The reason was to keep it backwards compatible. To avoid it immediately breaking ppl's builds when they are upgrading
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.
In the future, according to the deprecation cycle, we’ll eventually have to remove the old interfaces, which will lead to incompatibility issues.
I see two possible approaches here:
- To maintain compatibility, we can create new classes and mark the old ones as deprecated, ensuring they don’t depend on each other.
- Alternatively, we can completely rewrite the old ones, which will cause incompatible, but not binary incompatibility. I believe this option is better since any issues will be detected at compile time
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Show resolved
Hide resolved
So, to be clear, the approach I took was to add the content definitions of the 'new' (2025-06-18) schema while not immediately breaking it for anyone upgrading. Such that I could get the resourcelink in the tool call result as quick as possible and get some work done with it. While the SDK project could then softly phase out the deprecated classes. Alas, #294 is the way of the future, which means I will probably will have to wait a while till an official release with resourcelink will be available... |
Unfortunately, the openapi-generator doesn’t fully meet our needs, see #294 (review) Right now, we’re only using it to compare the diff between our schema implementation and the specification, to identify what we might have missed. At the same time, I still have #185 in my plans. I want our implementation to fully comply with the schema and to improve serialization, since the current custom serialization is redundant |
Support resource links in tool result content #158
Motivation and Context
The new spec introduced a new
ContentBlocktype:ResourceLink. This type however hasn't been implemented yet.Noticed that there was actually no
ContentBlocktype at all in the core as the latest schema reference specified. Instead there is aPromptMessageContent.How Has This Been Tested?
Added unit tests. Ran all tests.
Also used the build in a standalone project:
Breaking Changes
Added the following:
ContentBlockCreateMessageResultContent, andSamplingMessageContentHowever, I kept the two intrefaces
PromptMessageContentandPromptMessageContentMultimodalpurely for backwards compatibility. They are deprecated however.Types of changes
Checklist
Additional context