From 9ef4f80f85ade12128d4f07b311e93eaf3c244e4 Mon Sep 17 00:00:00 2001 From: Pavel Gorgulov Date: Tue, 21 Jul 2026 18:37:21 +0200 Subject: [PATCH] fix(server): reject duplicate feature names on registration (#883) Reject duplicate feature registration instead of silently overwriting the existing entry. ## Motivation and Context `FeatureRegistry.add`/`addAll` silently replaced a feature registered under the same key and emitted a spurious `list_changed` notification. 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`/`addResourceTemplate` now throw `IllegalArgumentException` on a duplicate name/URI; the existing registration stays intact and no notification is emitted. - `addTools`/`addPrompts`/`addResources` are all-or-nothing: batches with intra-batch duplicates or conflicts with existing keys are rejected without registering anything. - To replace a feature, remove it first (`removeTool` + `addTool`), matching the TypeScript SDK's remove-then-re-register path. No new public API. ## How Has This Been Tested? New `FeatureRegistryTest` unit 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 duplicate `addResource` in `AbstractResourceIntegrationTest`, fixed here. `jvmTest` (all modules), `apiCheck`, `ktlintCheck`, and `detekt` pass locally. ## Breaking Changes No public API signature changes (`apiCheck` passes against the existing dump). Behavioral: registering a duplicate name/URI now throws `IllegalArgumentException` instead of silently overwriting; remove the feature first to replace it. ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [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 - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed --- .../kotlin/AbstractResourceIntegrationTest.kt | 17 --- .../sdk/server/ServerBulkFeaturesTest.kt | 74 ++++++++++++ .../kotlin/sdk/server/ServerPromptsTest.kt | 23 ++++ .../sdk/server/ServerResourceTemplateTest.kt | 19 ++++ .../kotlin/sdk/server/ServerResourcesTest.kt | 21 ++++ .../sdk/server/ServerToolsNotificationTest.kt | 22 ++++ .../kotlin/sdk/server/ServerToolsTest.kt | 39 +++++++ .../kotlin/sdk/server/FeatureRegistry.kt | 40 +++++-- .../kotlin/sdk/server/FeatureRegistryTest.kt | 106 ++++++++++++++++++ 9 files changed, 333 insertions(+), 28 deletions(-) create mode 100644 kotlin-sdk-server/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistryTest.kt diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt index da7d85b..ab2a854 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt @@ -77,23 +77,6 @@ abstract class AbstractResourceIntegrationTest : KotlinTestBase() { ) } - server.addResource( - uri = testResourceUri, - name = testResourceName, - description = testResourceDescription, - mimeType = "text/plain", - ) { request -> - ReadResourceResult( - contents = listOf( - TextResourceContents( - text = testResourceContent, - uri = request.params.uri, - mimeType = "text/plain", - ), - ), - ) - } - server.addResource( uri = binaryResourceUri, name = binaryResourceName, diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerBulkFeaturesTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerBulkFeaturesTest.kt index 1eb828f..52f6908 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerBulkFeaturesTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerBulkFeaturesTest.kt @@ -1,5 +1,6 @@ package io.modelcontextprotocol.kotlin.sdk.server +import io.kotest.assertions.throwables.shouldThrow import io.kotest.matchers.collections.shouldBeEmpty import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder import io.kotest.matchers.collections.shouldHaveSize @@ -57,6 +58,39 @@ class ServerBulkFeaturesTest : AbstractServerFeaturesTest() { tools.map { it.name } shouldContainExactlyInAnyOrder listOf("existing", "new-a", "new-b") } + @Test + fun `addTools should throw and register nothing when a tool is already registered`() = runTest { + server.addTool("existing", "Existing") { CallToolResult(emptyList()) } + + shouldThrow { + server.addTools( + listOf( + RegisteredTool(Tool("new-a", ToolSchema())) { CallToolResult(emptyList()) }, + RegisteredTool(Tool("existing", ToolSchema())) { CallToolResult(emptyList()) }, + ), + ) + } + + val tools = client.listTools().tools + + tools shouldHaveSize 1 + tools.map { it.name } shouldContainExactlyInAnyOrder listOf("existing") + } + + @Test + fun `addTools should throw when batch contains duplicate names`() = runTest { + shouldThrow { + server.addTools( + listOf( + RegisteredTool(Tool("dup", ToolSchema())) { CallToolResult(emptyList()) }, + RegisteredTool(Tool("dup", ToolSchema())) { CallToolResult(emptyList()) }, + ), + ) + } + + client.listTools().tools.shouldBeEmpty() + } + // ── addPrompts ───────────────────────────────────────────────────────────── @Test @@ -89,6 +123,25 @@ class ServerBulkFeaturesTest : AbstractServerFeaturesTest() { prompts.map { it.name } shouldContainExactlyInAnyOrder listOf("existing-prompt", "new-p") } + @Test + fun `addPrompts should throw and register nothing when a prompt is already registered`() = runTest { + server.addPrompt("existing-prompt", "Existing") { GetPromptResult(messages = emptyList()) } + + shouldThrow { + server.addPrompts( + listOf( + RegisteredPrompt(Prompt("new-p")) { GetPromptResult(messages = emptyList()) }, + RegisteredPrompt(Prompt("existing-prompt")) { GetPromptResult(messages = emptyList()) }, + ), + ) + } + + val prompts = client.listPrompts().prompts + + prompts shouldHaveSize 1 + prompts.map { it.name } shouldContainExactlyInAnyOrder listOf("existing-prompt") + } + // ── addResources ─────────────────────────────────────────────────────────── @Test @@ -123,6 +176,27 @@ class ServerBulkFeaturesTest : AbstractServerFeaturesTest() { resources.map { it.uri } shouldContainExactlyInAnyOrder listOf("test://existing-res", "test://new-res") } + @Test + fun `addResources should throw and register nothing when a resource is already registered`() = runTest { + server.addResource("test://existing-res", "Existing", "Existing resource") { + ReadResourceResult(emptyList()) + } + + shouldThrow { + server.addResources( + listOf( + RegisteredResource(Resource("test://new-res", "New")) { ReadResourceResult(emptyList()) }, + RegisteredResource(Resource("test://existing-res", "Dup")) { ReadResourceResult(emptyList()) }, + ), + ) + } + + val resources = client.listResources().resources + + resources shouldHaveSize 1 + resources.map { it.uri } shouldContainExactlyInAnyOrder listOf("test://existing-res") + } + // ── Partial-batch remove ─────────────────────────────────────────────────── @Test diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerPromptsTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerPromptsTest.kt index d7ae46d..44b1e42 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerPromptsTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerPromptsTest.kt @@ -1,5 +1,8 @@ package io.modelcontextprotocol.kotlin.sdk.server +import io.kotest.matchers.string.shouldContain +import io.modelcontextprotocol.kotlin.sdk.types.GetPromptRequest +import io.modelcontextprotocol.kotlin.sdk.types.GetPromptRequestParams import io.modelcontextprotocol.kotlin.sdk.types.GetPromptResult import io.modelcontextprotocol.kotlin.sdk.types.Implementation import io.modelcontextprotocol.kotlin.sdk.types.Method @@ -110,6 +113,26 @@ class ServerPromptsTest : AbstractServerFeaturesTest() { assertFalse(promptListChangedNotificationReceived, "No notification should be sent when prompt doesn't exist") } + @Test + fun `addPrompt should throw when prompt with same name is already registered`() = runTest { + server.addPrompt(Prompt("test-prompt", "Original Prompt", null)) { + GetPromptResult(description = "original", messages = emptyList()) + } + + val exception = assertThrows { + server.addPrompt(Prompt("test-prompt", "Duplicate Prompt", null)) { + GetPromptResult(description = "duplicate", messages = emptyList()) + } + } + exception.message.orEmpty() shouldContain "Prompt \"test-prompt\" is already registered" + + // The original registration is intact + val prompts = client.listPrompts().prompts + assertEquals(1, prompts.size, "Only the original prompt should be registered") + val result = client.getPrompt(GetPromptRequest(GetPromptRequestParams("test-prompt"))) + assertEquals("original", result.description) + } + @Test fun `removePrompt should throw when prompts capability is not supported`() = runTest { var promptListChangedNotificationReceived = false diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourceTemplateTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourceTemplateTest.kt index c8ace19..8405469 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourceTemplateTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourceTemplateTest.kt @@ -5,6 +5,7 @@ import io.kotest.matchers.collections.shouldHaveSize import io.kotest.matchers.maps.shouldContainKey import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain import io.modelcontextprotocol.kotlin.sdk.types.Implementation import io.modelcontextprotocol.kotlin.sdk.types.ListResourceTemplatesRequest import io.modelcontextprotocol.kotlin.sdk.types.McpException @@ -162,6 +163,24 @@ class ServerResourceTemplateTest : AbstractServerFeaturesTest() { removed shouldBe false } + @Test + fun `addResourceTemplate should throw when template with same uriTemplate is already registered`() { + server.addResourceTemplate("test://items/{id}", "Item") { _, _ -> + ReadResourceResult(emptyList()) + } + + val exception = assertThrows { + server.addResourceTemplate("test://items/{id}", "Duplicate Item") { _, _ -> + ReadResourceResult(emptyList()) + } + } + exception.message.orEmpty() shouldContain "ResourceTemplate \"test://items/{id}\" is already registered" + + // The original registration is intact + server.resourceTemplates shouldHaveSize 1 + server.resourceTemplates[0].name shouldBe "Item" + } + @Test fun `addResourceTemplate should throw when resources capability is not supported`() { val noResourcesServer = Server( diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourcesTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourcesTest.kt index fafc3c8..a830788 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourcesTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerResourcesTest.kt @@ -1,7 +1,10 @@ package io.modelcontextprotocol.kotlin.sdk.server +import io.kotest.matchers.string.shouldContain import io.modelcontextprotocol.kotlin.sdk.types.Implementation import io.modelcontextprotocol.kotlin.sdk.types.Method +import io.modelcontextprotocol.kotlin.sdk.types.ReadResourceRequest +import io.modelcontextprotocol.kotlin.sdk.types.ReadResourceRequestParams import io.modelcontextprotocol.kotlin.sdk.types.ReadResourceResult import io.modelcontextprotocol.kotlin.sdk.types.ResourceListChangedNotification import io.modelcontextprotocol.kotlin.sdk.types.ServerCapabilities @@ -115,6 +118,24 @@ class ServerResourcesTest : AbstractServerFeaturesTest() { ) } + @Test + fun `addResource should throw when resource with same uri is already registered`() = runTest { + server.addResource("test://resource", "Original", "Original resource") { + ReadResourceResult(listOf(TextResourceContents(text = "original", uri = "test://resource"))) + } + + val exception = assertThrows { + server.addResource("test://resource", "Duplicate", "Duplicate resource") { + ReadResourceResult(listOf(TextResourceContents(text = "duplicate", uri = "test://resource"))) + } + } + exception.message.orEmpty() shouldContain "Resource \"test://resource\" is already registered" + + // The original registration is intact + val result = client.readResource(ReadResourceRequest(ReadResourceRequestParams("test://resource"))) + assertEquals("original", (result.contents.single() as TextResourceContents).text) + } + @Test fun `removeResource should throw when resources capability is not supported`() = runTest { // Create server without resources capability diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsNotificationTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsNotificationTest.kt index 3572082..9ac744b 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsNotificationTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsNotificationTest.kt @@ -11,6 +11,7 @@ import kotlinx.coroutines.test.runTest import org.awaitility.kotlin.await import org.awaitility.kotlin.untilAsserted import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -81,6 +82,27 @@ class ServerToolsNotificationTest : AbstractServerFeaturesTest() { } } + @Test + fun `addTool should not send notification when duplicate is rejected`() = runTest { + // Track notifications + val notifications = mutableListOf() + client.setNotificationHandler( + Method.Defined.NotificationsToolsListChanged, + ) { notification -> + notifications.add(notification) + CompletableDeferred(Unit) + } + + server.addTool("test-tool", "Test Tool") { CallToolResult(emptyList()) } + assertThrows { + 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") + } + @Test fun `notification should not be send when removed tool does not exists`() = runTest { // Track notifications diff --git a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt index 54c998e..1b3df91 100644 --- a/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt +++ b/integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt @@ -1,5 +1,8 @@ package io.modelcontextprotocol.kotlin.sdk.server +import io.kotest.matchers.string.shouldContain +import io.modelcontextprotocol.kotlin.sdk.types.CallToolRequest +import io.modelcontextprotocol.kotlin.sdk.types.CallToolRequestParams import io.modelcontextprotocol.kotlin.sdk.types.CallToolResult import io.modelcontextprotocol.kotlin.sdk.types.Implementation import io.modelcontextprotocol.kotlin.sdk.types.Method @@ -108,6 +111,42 @@ class ServerToolsTest : AbstractServerFeaturesTest() { assertTrue(result, "Tool with non-conforming name should have been registered and removable") } + @Test + fun `addTool should throw when tool with same name is already registered`() = runTest { + server.addTool("test-tool", "Original Tool") { + CallToolResult(listOf(TextContent("Original result"))) + } + + val exception = assertThrows { + server.addTool("test-tool", "Duplicate Tool") { + CallToolResult(listOf(TextContent("Duplicate result"))) + } + } + exception.message.orEmpty() shouldContain "Tool \"test-tool\" is already registered" + + // The original registration is intact + val tools = client.listTools().tools + assertEquals(1, tools.size, "Only the original tool should be registered") + val result = client.callTool(CallToolRequest(CallToolRequestParams("test-tool"))) + assertEquals("Original result", (result.content.single() as TextContent).text) + } + + @Test + fun `addTool should succeed after removing existing tool`() = runTest { + server.addTool("test-tool", "Original Tool") { + CallToolResult(listOf(TextContent("Original result"))) + } + + server.removeTool("test-tool") + server.addTool("test-tool", "Replacement Tool") { + CallToolResult(listOf(TextContent("Replacement result"))) + } + + assertEquals(1, client.listTools().tools.size, "Re-adding after removal should not duplicate the tool") + val result = client.callTool(CallToolRequest(CallToolRequestParams("test-tool"))) + assertEquals("Replacement result", (result.content.single() as TextContent).text) + } + @Test fun `removeTool should throw when tools capability is not supported`() = runTest { var toolListChangedNotificationReceived = false diff --git a/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistry.kt b/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistry.kt index 0ed6558..142d59c 100644 --- a/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistry.kt +++ b/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistry.kt @@ -20,9 +20,12 @@ internal interface FeatureListener { * A generic registry for managing features of a specified type. This class provides thread-safe * operations for adding, removing, and retrieving features from the registry. * + * Feature keys are unique: [add] and [addAll] reject keys that are already registered. + * To replace a feature, [remove] it first and add the new one. + * * @param T The type of the feature, constrained to implement the [Feature] interface. * @param featureType A string description of the type of feature being managed. - * Used primarily for logging purposes. + * Used for logging and error messages. */ internal class FeatureRegistry(private val featureType: String) { @@ -44,11 +47,11 @@ internal class FeatureRegistry(private val featureType: String) { private val listeners = atomic(persistentListOf()) internal fun addListener(listener: FeatureListener) { - listeners.update { it.add(listener) } + listeners.update { it.adding(listener) } } internal fun removeListener(listener: FeatureListener) { - listeners.update { it.remove(listener) } + listeners.update { it.removing(listener) } } /** @@ -58,27 +61,42 @@ internal class FeatureRegistry(private val featureType: String) { */ internal fun add(feature: T) { logger.info { "Adding $featureType: \"${feature.key}\"" } - val oldMap = registry.getAndUpdate { current -> current.put(feature.key, feature) } - val oldFeature = oldMap[feature.key] + registry.update { current -> + require(!current.containsKey(feature.key)) { + "$featureType \"${feature.key}\" is already registered. Remove it first to replace it." + } + current.putting(feature.key, feature) + } logger.info { "Added $featureType: \"${feature.key}\"" } - notifyFeatureUpdated(oldFeature, feature) + notifyFeatureUpdated(null, feature) } /** * Adds the given list of features to the registry. Each feature is mapped by its key - * and added to the current registry. + * and added to the current registry. The operation is all-or-nothing: if any key is + * rejected, no features are added. * * @param features The list of features to add to the registry. */ internal fun addAll(features: List) { logger.info { "Adding ${featureType}s: ${features.size}" } - val oldMap = registry.getAndUpdate { current -> current.putAll(features.associateBy { it.key }) } + val newEntries = features.associateBy { it.key } + require(newEntries.size == features.size) { + val duplicated = features.groupingBy { it.key }.eachCount().filterValues { it > 1 }.keys + "Duplicate $featureType keys in batch: $duplicated" + } + registry.update { current -> + val conflicting = newEntries.keys.filter { current.containsKey(it) } + require(conflicting.isEmpty()) { + "${featureType}s already registered: $conflicting. Remove them first to replace them." + } + current.puttingAll(newEntries) + } logger.info { "Added ${featureType}s: ${features.size}" } for (feature in features) { - val oldFeature = oldMap[feature.key] - notifyFeatureUpdated(oldFeature, feature) + notifyFeatureUpdated(null, feature) } } @@ -90,7 +108,7 @@ internal class FeatureRegistry(private val featureType: String) { */ internal fun remove(key: FeatureKey): Boolean { logger.info { "Removing $featureType: \"$key\"" } - val oldMap = registry.getAndUpdate { current -> current.remove(key) } + val oldMap = registry.getAndUpdate { current -> current.removing(key) } val removedFeature = oldMap[key] val removed = removedFeature != null diff --git a/kotlin-sdk-server/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistryTest.kt b/kotlin-sdk-server/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistryTest.kt new file mode 100644 index 0000000..c6bf72b --- /dev/null +++ b/kotlin-sdk-server/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/FeatureRegistryTest.kt @@ -0,0 +1,106 @@ +package io.modelcontextprotocol.kotlin.sdk.server + +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() + + override fun onFeatureUpdated(featureKey: String) { + updatedKeys.add(featureKey) + } +} + +class FeatureRegistryTest { + + private val registry = FeatureRegistry("Tool") + private val listener = RecordingListener() + + init { + registry.addListener(listener) + } + + @Test + fun `add should register feature and notify listener`() { + val feature = TestFeature("a") + + registry.add(feature) + + registry.values shouldBe mapOf("a" to feature) + listener.updatedKeys shouldBe listOf("a") + } + + @Test + fun `add should throw when key is already registered`() { + registry.add(TestFeature("a")) + + val exception = assertFailsWith { + registry.add(TestFeature("a", payload = "override")) + } + + exception.message shouldBe "Tool \"a\" is already registered. Remove it first to replace it." + } + + @Test + fun `add should keep original feature and not notify when duplicate is rejected`() { + val original = TestFeature("a", payload = "original") + registry.add(original) + + assertFailsWith { + registry.add(TestFeature("a", payload = "override")) + } + + registry.values shouldBe mapOf("a" to original) + listener.updatedKeys shouldBe listOf("a") + } + + @Test + fun `addAll should register all features and notify per feature`() { + val a = TestFeature("a") + val b = TestFeature("b") + + registry.addAll(listOf(a, b)) + + registry.values shouldBe mapOf("a" to a, "b" to b) + listener.updatedKeys shouldBe listOf("a", "b") + } + + @Test + fun `addAll should throw on duplicate keys within batch and register nothing`() { + val exception = assertFailsWith { + registry.addAll(listOf(TestFeature("a"), TestFeature("b"), TestFeature("a"))) + } + + exception.message shouldBe "Duplicate Tool keys in batch: [a]" + registry.values shouldBe emptyMap() + listener.updatedKeys shouldBe emptyList() + } + + @Test + fun `addAll should throw when any key is already registered and register nothing`() { + val existing = TestFeature("existing") + registry.add(existing) + + val exception = assertFailsWith { + registry.addAll(listOf(TestFeature("new"), TestFeature("existing", payload = "override"))) + } + + exception.message shouldBe "Tools already registered: [existing]. Remove them first to replace them." + registry.values shouldBe mapOf("existing" to existing) + listener.updatedKeys shouldBe listOf("existing") + } + + @Test + fun `add should succeed after the existing feature is removed`() { + registry.add(TestFeature("a", payload = "original")) + registry.remove("a") + + registry.add(TestFeature("a", payload = "replacement")) + + registry.values shouldBe mapOf("a" to TestFeature("a", payload = "replacement")) + listener.updatedKeys shouldBe listOf("a", "a", "a") + } +}