Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial Kotlin SDK type support for MCP “tasks” (SEP-1686) by introducing task models, task-related request/response/notification types, and wiring them into the existing JSON (de)serialization layer.
Changes:
- Introduces core task types (
Task,TaskStatus,TaskMetadata,RelatedTaskMetadata) and task RPC types (tasks/get,tasks/result,tasks/list,tasks/cancel) plusnotifications/tasks/status. - Registers new methods and hooks task requests/notifications/results into the polymorphic serializers.
- Adds/extends unit tests for task serialization/deserialization and updates detekt baselines / public API snapshot.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt | New task models + task RPC request/result types. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/notification.kt | Adds TaskStatusNotification and params. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/methods.kt | Registers new task methods + notification method. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/serializers.kt | Wires task request/notification deserialization and result shape matching. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/request.kt | Adds RequestMeta.relatedTask accessor. |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/types/TasksTest.kt | New comprehensive tests for task types and RPCs. |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/types/NotificationTest.kt | Adds tests for notifications/tasks/status. |
| kotlin-sdk-core/detekt-baseline-*.xml | Baseline updates/suppressions for new code/tests. |
| kotlin-sdk-core/api/kotlin-sdk-core.api | Updates public API snapshot for new types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/request.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/notification.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/types/TasksTest.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/serializers.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tasks.kt
Outdated
Show resolved
Hide resolved
kpavlov
left a comment
There was a problem hiding this comment.
Thank you, @devcrocod
There are copilot findings that look legitimate.
Let's split big classes and don't inflate detekt baselines.
| <SmellBaseline> | ||
| <ManuallySuppressedIssues/> | ||
| <CurrentIssues> | ||
| <ID>LargeClass:TasksTest.kt:TasksTest</ID> |
There was a problem hiding this comment.
Let's not inflate detekt baselines.
In this case the test class should be splitted
| */ | ||
| public val relatedTask: RelatedTaskMetadata? | ||
| get() = json[RELATED_TASK_META_KEY]?.let { element -> | ||
| McpJson.decodeFromJsonElement(element) |
There was a problem hiding this comment.
Could we cache it or handle during deserialization in the serializer? Currently, it will run json parsing every time
There was a problem hiding this comment.
json is not parsed every time, It’s not an expensive operation
I can do it that way, but then we’ll have to modify the class since it’s value class
There was a problem hiding this comment.
handle during deserialization in the serializer
And that would require a custom serializer, which I’d prefer to avoid
There was a problem hiding this comment.
json is not parsed every time, It’s not an expensive operation
I see the getter is not cached, so every property call will result in json parsing. In general, it should be avoided. The application would have to cache it.
A value class may not be the best solution here, as it introduces unnecessary restrictions.
Additionally, RequestMeta risks becoming a God Object as more metadata (e.g., RelatedTaskMetadata) is added over time. This pattern hinders extensibility and violates clean design principles.
An alternative solution is to use an extension function:
public fun RequestMeta.relatedTaskMetadata(): RelatedTaskMetadata? = this.json[RELATED_TASK_META_KEY]?.let {
McpJson.decodeFromJsonElement(it)
}This approach is more extensible and avoids the pitfalls of the current solution, even if it doesn't address the lack of caching.
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/notification.kt
Show resolved
Hide resolved
| ServerResult, | ||
| TaskFields | ||
|
|
||
| // ============================================================================ |
There was a problem hiding this comment.
This file is getting huge. Let's split it. There are clear boundaries: models, requests, responses
| <ManuallySuppressedIssues/> | ||
| <CurrentIssues> | ||
| <ID>CyclomaticComplexMethod:jsonUtils.kt:@OptIn(ExperimentalUnsignedTypes::class, ExperimentalSerializationApi::class) private fun convertToJsonElement: JsonElement</ID> | ||
| <ID>CyclomaticComplexMethod:serializers.kt:private fun selectServerResultDeserializer: DeserializationStrategy<ServerResult>?</ID> |
There was a problem hiding this comment.
Can we address it in a proper way?
f43c48b to
cb0c75b
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| assertNotNull(result.meta) | ||
| assertEquals("task-42", result.meta?.get("origin")?.jsonPrimitive?.content) | ||
| assertNotNull(result["content"]) | ||
| assertNotNull(result["isError"]) |
There was a problem hiding this comment.
Why not give Kotest a try for our new tests?
|
|
||
| assertEquals("task-1", cancelResult.taskId) | ||
| assertEquals(TaskStatus.Cancelled, cancelResult.status) | ||
| } |
There was a problem hiding this comment.
Wow, that’s almost 800 lines!
How about we break things down a bit? We could split tasks.kt and TaskTest.kt into different files, like GetTask.kt, ListTasks.kt and CancelTask.kt, based on what each one does.
| @SerialName("_meta") | ||
| override val meta: JsonObject? = null, | ||
| ) : ServerResult | ||
| public value class GetTaskPayloadResult(public val json: JsonObject) : |
There was a problem hiding this comment.
I am not sure I understand the advantages of moving to the value class here. It seems like it might not be possible to extend it by the user.
@e5l, do you think we are safe here?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Metadata for augmenting a request with task execution. | ||
| * Include this in the `task` field of the request parameters. | ||
| * | ||
| * @property ttl Requested duration in milliseconds to retain task from creation. | ||
| */ | ||
| @Serializable | ||
| public data class TaskMetadata(val ttl: Double? = null) | ||
|
|
There was a problem hiding this comment.
PR description says this change “closes #421 / #406”, but this PR appears to only add task-related model/types, serializers, and tests. Key items called out in those issues (e.g., updating LATEST_PROTOCOL_VERSION to 2025-11-25 and adding task: TaskMetadata? to RequestParams/request params) are not included here, so merging this as-is likely shouldn’t auto-close those tracking issues. Consider updating the PR description/linked issues to reflect the actual scope (e.g., “partial” / “follow-ups”).
|
@devcrocod, please consider defining a |
|
|
||
| @Test | ||
| fun `RELATED_TASK_META_KEY should have correct value`() { | ||
| assertEquals("io.modelcontextprotocol/related-task", RELATED_TASK_META_KEY) |
There was a problem hiding this comment.
This test is not valuable. Let's verify RequestMeta deserialization with RelatedTaskMetadata instead.
Would it be possible to get some test data from the spec? https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/tasks, e.g. https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/tasks#retrieving-task-results response is pretty good.
| { | ||
| "method": "tasks/get", | ||
| "params": { | ||
| "taskId": "task-11" |
There was a problem hiding this comment.
Are we missing the "io.modelcontextprotocol/related-task" here?
5.7.1.
Associating Task-Related Messages
All requests, notifications, and responses related to a task MUST include the io.modelcontextprotocol/related-task key in their _meta field, with the value set to an object with a taskId matching the associated task ID.
Motivation and Context
closes #421
#406
How Has This Been Tested?
Unit tests
Breaking Changes
NaN
Types of changes
Checklist