feat(server): add tool name validation per SEP-986#695
Conversation
…EP-986 standards.
devcrocod
requested review from
Copilot and
e5l
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Adds warn-only validation when registering tools on the server, intended to align tool names with the SEP-986 naming guidance while preserving backward compatibility.
Changes:
- Introduces
validateToolName()/warnIfInvalidToolName()utility to emit warnings for non-conforming tool names. - Hooks validation into
Server.addTool(...)andServer.addTools(...)registration paths. - Adds an integration test ensuring non-conpliant tool names are still accepted/registrable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/utils/ToolNameValidation.kt | New warn-only tool-name validation and logging utility. |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt | Calls validation during tool registration. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerToolsTest.kt | Ensures registration/removal still works for non-conforming names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private const val MAX_TOOL_NAME_LENGTH = 128 | ||
|
|
||
| private const val SEP_986_URL = "https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986" | ||
|
|
||
| /** | ||
| * Validates a tool name against the MCP tool naming standard (SEP-986). | ||
| * Returns a list of warnings. An empty list means the name is valid. | ||
| */ | ||
| internal fun validateToolName(name: String): List<String> { | ||
| if (name.isEmpty()) return listOf("Tool name cannot be empty") | ||
|
|
||
| val invalidChars = name.toSet().filterNot { it.isAsciiAllowed() } | ||
| val remainingInvalidChars = invalidChars - setOf(' ', ',') | ||
|
|
||
| return listOfNotNull( | ||
| "Tool name exceeds maximum length of $MAX_TOOL_NAME_LENGTH characters (current: ${name.length})" | ||
| .takeIf { name.length > MAX_TOOL_NAME_LENGTH }, |
There was a problem hiding this comment.
SEP-986 specifies tool names must be 1–64 characters, but this validation uses MAX_TOOL_NAME_LENGTH = 128. This will incorrectly treat names of length 65–128 as conforming. Update the max length constant (and related messages) to 64 to match the spec.
| "Tool name contains invalid characters: ${remainingInvalidChars.joinToString(", ") { "\"$it\"" }}" | ||
| .takeIf { remainingInvalidChars.isNotEmpty() }, | ||
| "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)" | ||
| .takeIf { invalidChars.isNotEmpty() }, | ||
| "Tool name starts or ends with a dash, which may cause parsing issues in some contexts" | ||
| .takeIf { name.startsWith('-') || name.endsWith('-') }, | ||
| "Tool name starts or ends with a dot, which may cause parsing issues in some contexts" | ||
| .takeIf { name.startsWith('.') || name.endsWith('.') }, | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Validates the tool [name] and logs a warning if it does not conform to SEP-986. | ||
| * Does not block registration — tools with non-conforming names are still accepted. | ||
| */ | ||
| internal fun warnIfInvalidToolName(name: String) { | ||
| val warnings = validateToolName(name) | ||
| if (warnings.isEmpty()) return | ||
|
|
||
| val warningLines = warnings.joinToString("\n") { " - $it" } | ||
| logger.warn { | ||
| """ | ||
| |Tool name validation warning for "$name": | ||
| |$warningLines | ||
| |Tool registration will proceed, but this may cause compatibility issues. | ||
| |Consider updating the tool name to conform to the MCP tool naming standard. | ||
| |See SEP-986: $SEP_986_URL | ||
| """.trimMargin() | ||
| } | ||
| } | ||
|
|
||
| private fun Char.isAsciiAllowed(): Boolean = | ||
| this in 'a'..'z' || this in 'A'..'Z' || this in '0'..'9' || this == '_' || this == '-' || this == '.' |
There was a problem hiding this comment.
SEP-986 allows forward slashes (/) and dots (.), but isAsciiAllowed() currently excludes '/'. This causes valid names like "user/profile/update" to be flagged as invalid and also makes the "Allowed characters are..." warning inaccurate. Include '/' in the allowed character set and update the allowed-characters message accordingly.
| @Test | ||
| fun `should return no warnings for max length name`() { | ||
| validateToolName("a".repeat(128)).shouldBeEmpty() | ||
| } |
There was a problem hiding this comment.
This test contradicts SEP-986: max tool name length should be 64, but the test treats 128 as the maximum valid length. Update the test cases to assert 64 is valid and 65 triggers a warning (matching the spec and the validator).
| @ParameterizedTest | ||
| @ValueSource(strings = ["user/profile/update", "a/b"]) | ||
| fun `should warn for names with forward slashes`(name: String) { | ||
| val warnings = validateToolName(name) | ||
| warnings.any { "invalid characters" in it }.shouldBeTrue() | ||
| } |
There was a problem hiding this comment.
SEP-986 explicitly allows forward slashes (/), but this test expects names containing '/' to produce an "invalid characters" warning. Once '/' is allowed in the validator, this test should instead assert no warnings for slash-separated tool names (and optionally add a slash example to the "valid tool names" parameter set).
| @ParameterizedTest | ||
| @ValueSource(strings = ["user@domain.com", "tool#1", "tool\$name"]) | ||
| fun `should warn for names with special characters`(name: String) { | ||
| val warnings = validateToolName(name) | ||
| warnings.any { "invalid characters" in it }.shouldBeTrue() | ||
| warnings shouldContain "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)" | ||
| } |
There was a problem hiding this comment.
The expected "Allowed characters are..." message omits forward slash (/), but SEP-986 allows it. If you update the validator to allow '/', update this assertion to include '/' in the allowed-character list (or adjust the test to match the final wording).
Add warn-only validation for tool names at registration time, following the MCP tool naming standard (SEP-986)
closes #417
How Has This Been Tested?
unit tests
Breaking Changes
none
Types of changes
Checklist