Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for MCP “tool execution” metadata in the Kotlin SDK so tools can advertise task-augmented execution capabilities (issue #543).
Changes:
- Introduces
ToolExecution+TaskSupportand adds optionalexecutiontoTool. - Extends
Server.addTool(...)overload to acceptexecutionand pass it into the registeredTool. - Adds serialization/deserialization unit tests and updates API dumps (plus adds detekt baseline files).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt | Extends addTool overload to accept/pass ToolExecution. |
| kotlin-sdk-server/api/kotlin-sdk-server.api | Updates public API surface for the new addTool signature. |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/types/ToolsTest.kt | Adds unit tests for ToolExecution and Tool.execution JSON round-trips. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tools.kt | Adds Tool.execution, new ToolExecution model, and TaskSupport enum. |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Adjusts deprecated Tool(...) wrapper callsite to use named meta argument. |
| kotlin-sdk-core/detekt-baseline-test.xml | Adds a detekt baseline for tests (currently appears unused/stale). |
| kotlin-sdk-core/detekt-baseline-commonTestSourceSet.xml | Adds a detekt baseline for commonTest source set (currently appears unused). |
| kotlin-sdk-core/api/kotlin-sdk-core.api | Updates public API surface for new types and Tool signature changes. |
💡 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/tools.kt
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types/tools.kt
Show resolved
Hide resolved
kpavlov
left a comment
There was a problem hiding this comment.
Thank you @devcrocod !
Almost perfect, but let's address @copilot findings, especially breaking changes, KDoc discrepancy and baseline cleanup suggestions.
Let's not inflate detekt baseline files: they should only shrink.
Let's maintain PR description accurate and informative.
| <SmellBaseline> | ||
| <ManuallySuppressedIssues/> | ||
| <CurrentIssues> | ||
| <ID>LargeClass:ToolsTest.kt:ToolsTest</ID> |
There was a problem hiding this comment.
-
This is a legitimate finding. Let's split ToolsTest for maintainability instead of suppressing warning.
-
In general, let's not add anything to baselines. Only remove. In exceptional cases let's use
@Suppress annotation in place
There was a problem hiding this comment.
- currently, our approach is to group unit tests by entity for types. Splitting ToolsTest into separate files would increase complexity and make the structure less clear
- ideally, we should avoid adding
@Suppressannotations from third-party tools into the source code. For now, I've added this to baseline, which is also not the best solution. If you see a better approach, I'd appreciate your suggestion
There was a problem hiding this comment.
- This method might not be the best fit when the group gets too large.
Splitting ToolsTest into separate files would increase complexity and make the structure less clear
Actually, it is often the other way around. Smaller files are usually easier to understand and keep up with. This one is over 500 lines and checks out several classes. It could be broken down into smaller pieces, so let’s do that.
There was a problem hiding this comment.
- Ideally, we should avoid situations where
@Suppressis needed.
There is one exception: when we are clearly explaining our design choice to NOT follow best practices. We can add a comment to clarify this. If it is in the code, it is easy to see whether it is a false positive or not. But if it is in the baseline file, it’s separate from the code.
Let’s focus on removing baseline files rather than adding more to them.
I’m afraid I won’t be able to give +1 on this.
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt
Show resolved
Hide resolved
…on and update related tests
29e3d1b to
1cc2a16
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
kpavlov
left a comment
There was a problem hiding this comment.
@devcrocod, ok please check the #582 and consider removing unnecessary baselines
- Updated `detekt.yml` to exclude test folders from `EmptyFunctionBlock`, `LargeClass`, and `LongMethod` checks. - Cleanup detekt-baseline files. - Simplified `.idea/detekt.xml`. ## Motivation and Context To reduce linter noise and reduce tension ## How Has This Been Tested? `./gradlew detekt` ## Breaking Changes No ## Types of changes - [x] Build tool/ci update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context #567
closes #543
How Has This Been Tested?
unit
Breaking Changes
binary compatibility in the Tool class
Types of changes
Checklist