Update notification schema #199
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the notification schema to add a params field to all notifications, aligning with the Model Context Protocol specification. This represents a breaking change that restructures how notification data is organized.
Key changes:
- Introduces a
paramsfield to theNotificationinterface and all implementing classes - Changes the progress type from
InttoDoublein progress-related classes - Updates serialization/deserialization logic for notifications to properly handle the new params structure
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Core notification schema changes adding params field and restructuring notification classes |
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt | Updates protocol handling to use params field for progress and cancellation notifications |
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt | Updates server logging to access data through params field |
| api/kotlin-sdk.api | API signature updates reflecting the breaking changes to notification structure |
| Test files | Package name corrections and import cleanups |
| return JSONRPCNotification( | ||
| method.value, | ||
| params = encoded | ||
| method = method.value, | ||
| params = McpJson.encodeToJsonElement(params), |
There was a problem hiding this comment.
The serialization logic for notifications doesn't handle null params correctly. When params is null, encoding it will create a JSON null value, but the protocol may expect the params field to be omitted entirely.
Copilot uses AI. Check for mistakes.
| @@ -408,7 +423,7 @@ public sealed interface ServerResult : RequestResult | |||
| */ | |||
| @Serializable | |||
| public data class UnknownMethodRequestOrNotification( | |||
| override val method: Method, | |||
| override val method: Method, override val params: NotificationParams? = null, | |||
There was a problem hiding this comment.
Making params nullable in UnknownMethodRequestOrNotification but required in other notification types creates inconsistency in the API. Consider whether params should be consistently required or nullable across all notification types.
| override val method: Method, override val params: NotificationParams? = null, | |
| override val method: Method, override val params: NotificationParams = DefaultNotificationParams, |
Copilot uses AI. Check for mistakes.
| @@ -411,8 +411,10 @@ class ClientTest { | |||
| } | |||
| server.sendLoggingMessage( | |||
| LoggingMessageNotification( | |||
There was a problem hiding this comment.
The LoggingMessageNotification constructor call is incomplete - it's missing the closing parenthesis and appears to have syntax errors with the nested params structure.
Copilot uses AI. Check for mistakes.
|
I think so |
Motivation and Context
https://modelcontextprotocol.io/specification/2025-06-18/schema#notifications%2Fcancelled
How Has This Been Tested?
locally
Breaking Changes
yes
Types of changes
Checklist