Downgrade Kotlin to 2.1.21 #1
Conversation
- Downgrade Kotlin 2.2.0->2.1.21 to make broader adoption possible. Update Kotlin compiler configuration - Add [kotlinx-datetime](https://github.com/Kotlin/kotlinx-datetime) dependency. Switch from `kotlin.time.Instant` to `kotlinx.datetime.Instant`. - Downgrade atomicfu 0.29.0->0.28.0 - Downgrade kotlinx-serialization 1.9.0->1.8.1
📝 WalkthroughSummary by CodeRabbit
WalkthroughDocumentation updates, Gradle/KMP configuration changes targeting Kotlin 2.1, introduction of explicit API mode, generated public LIB_VERSION constant, dependency/version catalog adjustments, exposure of kotlinx-datetime as an API dependency, and migration from kotlin.time.Instant to kotlinx.datetime.Instant in core types and tests. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CONTRIBUTING.md(1 hunks)README.md(1 hunks)buildSrc/src/main/kotlin/mcp.multiplatform.gradle.kts(2 hunks)gradle.properties(1 hunks)gradle/libs.versions.toml(2 hunks)kotlin-sdk-core/build.gradle.kts(1 hunks)kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt(14 hunks)kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesTest.kt(1 hunks)
🔇 Additional comments (9)
gradle/libs.versions.toml (2)
35-41: Library aliases look good; confirm API exposure intent.kotlinx-datetime is added and will be API-exposed by kotlin-sdk-core; that matches the Instant type change.
3-19: All checked libraries are compatible with Kotlin 2.1.21.
atomicfu 0.28.0, kotlinx-datetime 0.6.2, kotlinx-serialization-json 1.8.1, kotlinx-coroutines-core 1.10.2, Ktor 3.2.3, and binary-compatibility-validator 0.18.1 have been verified against Kotlin 2.1.21 with no conflicts.gradle.properties (1)
10-10: native.ignoreDisabledTargets: confirm desired behavior in CI.This hides errors for disabled targets; ensure CI still exercises the intended matrix so regressions aren’t masked.
kotlin-sdk-core/build.gradle.kts (1)
37-37: Expose kotlinx-datetime via API — LGTM.Matches the public Instant type usage in commonMain.
buildSrc/src/main/kotlin/mcp.multiplatform.gradle.kts (2)
49-51: Revisit JVM target level.You’re compiling with JDK 21 but emitting Java 8 bytecode. If consumers and dependencies allow, bumping to 11 or 17 can unlock better inlining and APIs. If 1.8 is required, keep as-is.
69-71: Generated LibVersion source wiring — LGTM.Task outputs are correctly added as a sources dir.
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt (3)
4-4: Switch to kotlinx.datetime.Instant — LGTM; confirm JSON module setup.kotlinx.datetime types are @serializable; typically no extra serializersModule is needed. Ensure McpJson doesn’t override it in a way that drops Instant’s serializer.
1495-1501: Enum casing suppression — LGTM.Suppressing EnumEntryName aligns with the wire format.
1743-1744: Action enum suppression — LGTM.Matches lower-case protocol values.
| compilerOptions { | ||
| languageVersion = KOTLIN_2_1 | ||
| apiVersion = KOTLIN_2_1 | ||
| // TODO: allWarningsAsErrors = true | ||
| extraWarnings = true | ||
| freeCompilerArgs = | ||
| listOf( | ||
| "-Xwhen-guards", | ||
| ) | ||
| } | ||
| coreLibrariesVersion = "2.1.21" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Compiler config for Kotlin 2.1 — LGTM; consider enabling Werror.
Settings look correct for 2.1. Consider turning on allWarningsAsErrors once CI is green.
- // TODO: allWarningsAsErrors = true
+ // Enable once the tree is clean
+ // allWarningsAsErrors = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compilerOptions { | |
| languageVersion = KOTLIN_2_1 | |
| apiVersion = KOTLIN_2_1 | |
| // TODO: allWarningsAsErrors = true | |
| extraWarnings = true | |
| freeCompilerArgs = | |
| listOf( | |
| "-Xwhen-guards", | |
| ) | |
| } | |
| coreLibrariesVersion = "2.1.21" | |
| compilerOptions { | |
| languageVersion = KOTLIN_2_1 | |
| apiVersion = KOTLIN_2_1 | |
| // Enable once the tree is clean | |
| // allWarningsAsErrors = true | |
| extraWarnings = true | |
| freeCompilerArgs = | |
| listOf( | |
| "-Xwhen-guards", | |
| ) | |
| } | |
| coreLibrariesVersion = "2.1.21" |
🤖 Prompt for AI Agents
In buildSrc/src/main/kotlin/mcp.multiplatform.gradle.kts around lines 35 to 46,
the TODO for Werror remains; enable allWarningsAsErrors by setting
allWarningsAsErrors = true (or, if you want safer rollout, read a Gradle/project
property like "enableWerror" and set allWarningsAsErrors =
project.findProperty("enableWerror") == "true") so warnings are promoted to
errors once CI is verified; ensure the property default is false in CI until you
flip it.
| js { nodejs() } | ||
| wasmJs { nodejs() } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
JS/Wasm target setup: ensure consistency with module-level kotlin blocks.
This plugin sets js/wasm nodejs; kotlin-sdk-core also configures js { browser(); nodejs() }. Double-configuration is usually fine, but avoid divergent target declarations across plugin and module.
- js { nodejs() }
- wasmJs { nodejs() }
+ // Consider centralizing target declarations either here or per-module to prevent drift.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In buildSrc/src/main/kotlin/mcp.multiplatform.gradle.kts around lines 62-63, the
plugin declares js { nodejs() } and wasmJs { nodejs() } which diverges from
module-level kotlin blocks that use js { browser(); nodejs() }; update the
plugin to match the module-level targets so declarations stay consistent —
either add browser() to the plugin's js target (js { browser(); nodejs() }) or
remove the js nodejs() from the plugin and let modules declare both browser()
and nodejs(); pick one approach and apply it consistently across plugin and
modules, remove duplicate/conflicting declarations, and add a short comment
explaining the chosen centralization.
| * To build for MacOS/iOS targets, Xcode/Command Line Tools should be installed. See Apple Developer [documentation](https://developer.apple.com/documentation/xcode/downloading-and-installing-additional-xcode-components) for details. | ||
| * To build JS/Wasm targets, [Node.js](https://nodejs.org/) must be installed. | ||
| * The project can be opened in IntelliJ IDEA without additional prerequisites. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Specify minimum toolchain versions and quick install hints.
Add minimum versions (e.g., Xcode ≥ 15.x for current Kotlin/Native, Node.js ≥ LTS 18/20) and brief install commands to reduce setup friction.
What minimum versions are you targeting for:
- Kotlin/Native toolchains on macOS/iOS (Xcode/CLT)?
- Node.js for JS/Wasm builds?
🤖 Prompt for AI Agents
CONTRIBUTING.md lines 76-78: add explicit minimum toolchain versions and quick
install hints — target Xcode/Command Line Tools >= 15.x for Kotlin/Native on
macOS/iOS and Node.js LTS (preferably 18.x or 20.x) for JS/Wasm; update the
three bullets to include these minimum versions and a one-line install hint each
(e.g., obtain Xcode from the App Store and install the Command Line Tools, or
install Xcode via Homebrew Cask if preferred; use nvm or your package manager to
install the Node.js LTS version) and include a short note referencing where to
verify installed versions (xcodebuild -version / node -v) so contributors can
confirm they meet the minimums.
| # Build JS targets using npm package manager https://kotlinlang.org/docs/js-project-setup.html#npm-dependencies | ||
| kotlin.js.yarn=false |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Switching Kotlin/JS to npm: document Node/NPM expectations.
With yarn disabled, ensure Node and npm are available in CI and optionally pin Node via a tool (e.g., actions/setup-node) to avoid non-reproducible builds.
🤖 Prompt for AI Agents
In gradle.properties around lines 14-15, you're switching Kotlin/JS to npm
(kotlin.js.yarn=false) but haven't documented or enforced Node/npm in CI; add a
short comment in gradle.properties stating the required Node version and that
npm must be available, and then update CI workflows (e.g., GitHub Actions) to
install and pin Node by adding actions/setup-node with a specific semver (or
exact) version and ensure npm is present before the build; verify the pipeline
runs npm --version and npm ci (or npm install) as part of the job to guarantee
reproducible builds.
| package io.modelcontextprotocol.kotlin.sdk | ||
|
|
||
| import io.modelcontextprotocol.kotlin.sdk.shared.McpJson | ||
| import kotlinx.datetime.Instant |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused ExperimentalTime opt-in.
After migrating to kotlinx.datetime.Instant, the ExperimentalTime annotation/import are unnecessary.
-import kotlin.time.ExperimentalTime
@@
- @OptIn(ExperimentalTime::class)
@Test
fun `should serialize and deserialize annotations correctly`() {Also applies to: 10-10, 241-241
🤖 Prompt for AI Agents
In
kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/TypesTest.kt
around line 4 (also apply same change at lines 10 and 241), remove the
now-unused ExperimentalTime opt-in import/annotation introduced before migration
to kotlinx.datetime.Instant; delete the import and any
@OptIn(ExperimentalTime::class) usages or annotations at those lines so the file
no longer references ExperimentalTime.
| @@ -1,7 +1,8 @@ | |||
| # MCP Kotlin SDK | |||
|
|
|||
| [](http://kotlinlang.org) | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Align Kotlin badge with actual version (2.1.21).
Badge shows "2.1"; suggest "2.1.21" to match libs.versions.toml and avoid drift.
-[](http://kotlinlang.org)
+[](http://kotlinlang.org)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [](http://kotlinlang.org) | |
| [](http://kotlinlang.org) |
🤖 Prompt for AI Agents
In README.md around line 3, the Kotlin version displayed in the shields.io badge
is "2.1" but should match libs.versions.toml ("2.1.21"); update the badge text
and URL segment from "kotlin-2.1-blue.svg" to "kotlin-2.1.21-blue.svg" (and any
other occurrences of 2.1 in the badge markup) so the badge displays 2.1.21 and
stays in sync with project versions.
| [](http://kotlinlang.org) | ||
| [](https://kotlinlang.org/docs/multiplatform.html) | ||
| [-blue)](https://kotlinlang.org/docs/multiplatform.html) | ||
| [](https://kotlinlang.org/docs/multiplatform.html) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify “Native” scope or list targets.
Consider mentioning the concrete KMP native targets (iOS/tvOS/watchOS/macOS, Linux, Windows) to set expectations, or link to a section that enumerates them.
🤖 Prompt for AI Agents
In README.md around line 5, the "Native" badge is ambiguous; update the badge
text or nearby sentence to either list the concrete Kotlin Multiplatform native
targets (iOS, tvOS, watchOS, macOS, Linux, Windows) or add a short parenthetical
or link to a section that enumerates supported native targets so readers know
the exact scope; keep wording concise and consistent with existing badge/link
style.
kotlin.time.Instanttokotlinx.datetime.Instant.