fix(server): reject duplicate feature names on registration#883
Open
devcrocod wants to merge 1 commit into
Open
fix(server): reject duplicate feature names on registration#883devcrocod wants to merge 1 commit into
devcrocod wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes the server-side feature registration behavior to reject duplicate tool/prompt/resource/resource-template keys instead of silently overwriting existing registrations, aligning behavior with MCP uniqueness expectations and the TypeScript SDK.
Changes:
FeatureRegistry.add/addAllnow throw on duplicate keys, andaddAllis atomic (register-all-or-register-none).- Server registration KDocs updated to document the new
IllegalArgumentExceptionbehavior and remove-then-replace guidance. - New unit + integration tests added for duplicate rejection, batch atomicity, and “no notification on rejection”; one real-world duplicate resource registration in an integration test setup was removed.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistryTest.kt | New unit tests for duplicate rejection and batch atomicity in FeatureRegistry. |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt | KDoc updates to document new duplicate-registration exceptions and replacement workflow. |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistry.kt | Core behavior change: reject duplicates in add/addAll, ensure atomic batch registration. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt | Integration tests for duplicate tool registration rejection and re-add-after-remove behavior. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsNotificationTest.kt | Integration test ensuring no list-changed notification is emitted for rejected duplicates. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourceTemplateTest.kt | Integration test for duplicate resource-template registration rejection. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourcesTest.kt | Integration test for duplicate resource registration rejection. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerPromptsTest.kt | Integration test for duplicate prompt registration rejection. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerBulkFeaturesTest.kt | Integration tests for bulk registration atomicity when conflicts/duplicates exist. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt | Removes a duplicate addResource that would now correctly fail. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+24
| import io.kotest.matchers.shouldBe | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertFailsWith | ||
|
|
||
| private data class TestFeature(override val key: FeatureKey, val payload: String = "") : Feature | ||
|
|
||
| private class RecordingListener : FeatureListener { | ||
| val updatedKeys = mutableListOf<String>() | ||
|
|
||
| override fun onFeatureUpdated(featureKey: String) { | ||
| updatedKeys.add(featureKey) | ||
| } | ||
| } | ||
|
|
||
| class FeatureRegistryTest { | ||
|
|
||
| private val registry = FeatureRegistry<TestFeature>("Tool") | ||
| private val listener = RecordingListener() | ||
|
|
||
| init { | ||
| registry.addListener(listener) | ||
| } |
Comment on lines
+96
to
+104
| server.addTool("test-tool", "Test Tool") { CallToolResult(emptyList()) } | ||
| assertThrows<IllegalArgumentException> { | ||
| server.addTool("test-tool", "Duplicate Tool") { CallToolResult(emptyList()) } | ||
| } | ||
| // Close the server to stop processing further events and flush notifications | ||
| server.close() | ||
|
|
||
| assertEquals(1, notifications.size, "Only the first registration should send a notification") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reject duplicate feature registration instead of silently overwriting the existing entry.
Motivation and Context
FeatureRegistry.add/addAllsilently replaced a feature registered under the same key and emitted a spuriouslist_changednotification. Tool/prompt names and resource URIs are unique identifiers in MCP, and the TypeScript SDK already rejects duplicates at registration time (Tool X is already registered).addTool/addPrompt/addResource/addResourceTemplatenow throwIllegalArgumentExceptionon a duplicate name/URI; the existing registration stays intact and no notification is emitted.addTools/addPrompts/addResourcesare all-or-nothing: batches with intra-batch duplicates or conflicts with existing keys are rejected without registering anything.removeTool+addTool), matching the TypeScript SDK's remove-then-re-register path. No new public API.How Has This Been Tested?
New
FeatureRegistryTestunit suite (duplicate rejection, batch atomicity, no notification on rejection, re-add after remove) plus integration tests for tools, prompts, resources, and resource templates. The check also surfaced a real duplicateaddResourceinAbstractResourceIntegrationTest, fixed here.jvmTest(all modules),apiCheck,ktlintCheck, anddetektpass locally.Breaking Changes
No public API signature changes (
apiCheckpasses against the existing dump). Behavioral: registering a duplicate name/URI now throwsIllegalArgumentExceptioninstead of silently overwriting; remove the feature first to replace it.Types of changes
Checklist