Implement SEP-986: Tool name validation (#1655)

Author: felixweinberger

src/mcp/server/fastmcp/tools/base.py

@@ -11,6 +11,7 @@
 from mcp.server.fastmcp.exceptions import ToolError
 from mcp.server.fastmcp.utilities.context_injection import find_context_parameter
 from mcp.server.fastmcp.utilities.func_metadata import FuncMetadata, func_metadata
+from mcp.shared.tool_name_validation import validate_and_warn_tool_name
 from mcp.types import Icon, ToolAnnotations
 
 if TYPE_CHECKING:
@@ -56,6 +57,8 @@ def from_function(
         """Create a Tool from a function."""
         func_name = name or fn.__name__
 
+        validate_and_warn_tool_name(func_name)
+
         if func_name == "<lambda>":
             raise ValueError("You must provide a name for lambda functions")
 

src/mcp/server/lowlevel/server.py

@@ -90,6 +90,7 @@ async def main():
 from mcp.shared.exceptions import McpError
 from mcp.shared.message import ServerMessageMetadata, SessionMessage
 from mcp.shared.session import RequestResponder
+from mcp.shared.tool_name_validation import validate_and_warn_tool_name
 
 logger = logging.getLogger(__name__)
 
@@ -422,13 +423,15 @@ async def handler(req: types.ListToolsRequest):
                 if isinstance(result, types.ListToolsResult):  # pragma: no cover
                     # Refresh the tool cache with returned tools
                     for tool in result.tools:
+                        validate_and_warn_tool_name(tool.name)
                         self._tool_cache[tool.name] = tool
                     return types.ServerResult(result)
                 else:
                     # Old style returns list[Tool]
                     # Clear and refresh the entire tool cache
                     self._tool_cache.clear()
                     for tool in result:
+                        validate_and_warn_tool_name(tool.name)
                         self._tool_cache[tool.name] = tool
                     return types.ServerResult(types.ListToolsResult(tools=result))
 

src/mcp/shared/tool_name_validation.py

@@ -0,0 +1,129 @@
+"""Tool name validation utilities according to SEP-986.
+
+Tool names SHOULD be between 1 and 128 characters in length (inclusive).
+Tool names are case-sensitive.
+Allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z),
+digits (0-9), underscore (_), dash (-), and dot (.).
+Tool names SHOULD NOT contain spaces, commas, or other special characters.
+
+See: https://modelcontextprotocol.io/specification/draft/server/tools#tool-names
+"""
+
+from __future__ import annotations
+
+import logging
+import re
+from dataclasses import dataclass, field
+
+logger = logging.getLogger(__name__)
+
+# Regular expression for valid tool names according to SEP-986 specification
+TOOL_NAME_REGEX = re.compile(r"^[A-Za-z0-9._-]{1,128}$")
+
+# SEP reference URL for warning messages
+SEP_986_URL = "https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986"
+
+
+@dataclass
+class ToolNameValidationResult:
+    """Result of tool name validation.
+
+    Attributes:
+        is_valid: Whether the tool name conforms to SEP-986 requirements.
+        warnings: List of warning messages for non-conforming aspects.
+    """
+
+    is_valid: bool
+    warnings: list[str] = field(default_factory=lambda: [])
+
+
+def validate_tool_name(name: str) -> ToolNameValidationResult:
+    """Validate a tool name according to the SEP-986 specification.
+
+    Args:
+        name: The tool name to validate.
+
+    Returns:
+        ToolNameValidationResult containing validation status and any warnings.
+    """
+    warnings: list[str] = []
+
+    # Check for empty name
+    if not name:
+        return ToolNameValidationResult(
+            is_valid=False,
+            warnings=["Tool name cannot be empty"],
+        )
+
+    # Check length
+    if len(name) > 128:
+        return ToolNameValidationResult(
+            is_valid=False,
+            warnings=[f"Tool name exceeds maximum length of 128 characters (current: {len(name)})"],
+        )
+
+    # Check for problematic patterns (warnings, not validation failures)
+    if " " in name:
+        warnings.append("Tool name contains spaces, which may cause parsing issues")
+
+    if "," in name:
+        warnings.append("Tool name contains commas, which may cause parsing issues")
+
+    # Check for potentially confusing leading/trailing characters
+    if name.startswith("-") or name.endswith("-"):
+        warnings.append("Tool name starts or ends with a dash, which may cause parsing issues in some contexts")
+
+    if name.startswith(".") or name.endswith("."):
+        warnings.append("Tool name starts or ends with a dot, which may cause parsing issues in some contexts")
+
+    # Check for invalid characters
+    if not TOOL_NAME_REGEX.match(name):
+        # Find all invalid characters (unique, preserving order)
+        invalid_chars: list[str] = []
+        seen: set[str] = set()
+        for char in name:
+            if not re.match(r"[A-Za-z0-9._-]", char) and char not in seen:
+                invalid_chars.append(char)
+                seen.add(char)
+
+        warnings.append(f"Tool name contains invalid characters: {', '.join(repr(c) for c in invalid_chars)}")
+        warnings.append("Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)")
+
+        return ToolNameValidationResult(is_valid=False, warnings=warnings)
+
+    return ToolNameValidationResult(is_valid=True, warnings=warnings)
+
+
+def issue_tool_name_warning(name: str, warnings: list[str]) -> None:
+    """Log warnings for non-conforming tool names.
+
+    Args:
+        name: The tool name that triggered the warnings.
+        warnings: List of warning messages to log.
+    """
+    if not warnings:
+        return
+
+    logger.warning(f'Tool name validation warning for "{name}":')
+    for warning in warnings:
+        logger.warning(f"  - {warning}")
+    logger.warning("Tool registration will proceed, but this may cause compatibility issues.")
+    logger.warning("Consider updating the tool name to conform to the MCP tool naming standard.")
+    logger.warning(f"See SEP-986 ({SEP_986_URL}) for more details.")
+
+
+def validate_and_warn_tool_name(name: str) -> bool:
+    """Validate a tool name and issue warnings for non-conforming names.
+
+    This is the primary entry point for tool name validation. It validates
+    the name and logs any warnings via the logging module.
+
+    Args:
+        name: The tool name to validate.
+
+    Returns:
+        True if the name is valid, False otherwise.
+    """
+    result = validate_tool_name(name)
+    issue_tool_name_warning(name, result.warnings)
+    return result.is_valid

tests/shared/test_tool_name_validation.py

@@ -0,0 +1,199 @@
+"""Tests for tool name validation utilities (SEP-986)."""
+
+import logging
+
+import pytest
+
+from mcp.shared.tool_name_validation import (
+    issue_tool_name_warning,
+    validate_and_warn_tool_name,
+    validate_tool_name,
+)
+
+
+class TestValidateToolName:
+    """Tests for validate_tool_name function."""
+
+    class TestValidNames:
+        """Test cases for valid tool names."""
+
+        @pytest.mark.parametrize(
+            "tool_name",
+            [
+                "getUser",
+                "get_user_profile",
+                "user-profile-update",
+                "admin.tools.list",
+                "DATA_EXPORT_v2.1",
+                "a",
+                "a" * 128,
+            ],
+            ids=[
+                "simple_alphanumeric",
+                "with_underscores",
+                "with_dashes",
+                "with_dots",
+                "mixed_characters",
+                "single_character",
+                "max_length_128",
+            ],
+        )
+        def test_accepts_valid_names(self, tool_name: str) -> None:
+            """Valid tool names should pass validation with no warnings."""
+            result = validate_tool_name(tool_name)
+            assert result.is_valid is True
+            assert result.warnings == []
+
+    class TestInvalidNames:
+        """Test cases for invalid tool names."""
+
+        def test_rejects_empty_name(self) -> None:
+            """Empty names should be rejected."""
+            result = validate_tool_name("")
+            assert result.is_valid is False
+            assert "Tool name cannot be empty" in result.warnings
+
+        def test_rejects_name_exceeding_max_length(self) -> None:
+            """Names exceeding 128 characters should be rejected."""
+            result = validate_tool_name("a" * 129)
+            assert result.is_valid is False
+            assert any("exceeds maximum length of 128 characters (current: 129)" in w for w in result.warnings)
+
+        @pytest.mark.parametrize(
+            "tool_name,expected_char",
+            [
+                ("get user profile", "' '"),
+                ("get,user,profile", "','"),
+                ("user/profile/update", "'/'"),
+                ("[email protected]", "'@'"),
+            ],
+            ids=[
+                "with_spaces",
+                "with_commas",
+                "with_slashes",
+                "with_at_symbol",
+            ],
+        )
+        def test_rejects_invalid_characters(self, tool_name: str, expected_char: str) -> None:
+            """Names with invalid characters should be rejected."""
+            result = validate_tool_name(tool_name)
+            assert result.is_valid is False
+            assert any("invalid characters" in w and expected_char in w for w in result.warnings)
+
+        def test_rejects_multiple_invalid_chars(self) -> None:
+            """Names with multiple invalid chars should list all of them."""
+            result = validate_tool_name("user name@domain,com")
+            assert result.is_valid is False
+            warning = next(w for w in result.warnings if "invalid characters" in w)
+            assert "' '" in warning
+            assert "'@'" in warning
+            assert "','" in warning
+
+        def test_rejects_unicode_characters(self) -> None:
+            """Names with unicode characters should be rejected."""
+            result = validate_tool_name("user-\u00f1ame")  # n with tilde
+            assert result.is_valid is False
+
+    class TestWarningsForProblematicPatterns:
+        """Test cases for valid names that generate warnings."""
+
+        def test_warns_on_leading_dash(self) -> None:
+            """Names starting with dash should generate warning but be valid."""
+            result = validate_tool_name("-get-user")
+            assert result.is_valid is True
+            assert any("starts or ends with a dash" in w for w in result.warnings)
+
+        def test_warns_on_trailing_dash(self) -> None:
+            """Names ending with dash should generate warning but be valid."""
+            result = validate_tool_name("get-user-")
+            assert result.is_valid is True
+            assert any("starts or ends with a dash" in w for w in result.warnings)
+
+        def test_warns_on_leading_dot(self) -> None:
+            """Names starting with dot should generate warning but be valid."""
+            result = validate_tool_name(".get.user")
+            assert result.is_valid is True
+            assert any("starts or ends with a dot" in w for w in result.warnings)
+
+        def test_warns_on_trailing_dot(self) -> None:
+            """Names ending with dot should generate warning but be valid."""
+            result = validate_tool_name("get.user.")
+            assert result.is_valid is True
+            assert any("starts or ends with a dot" in w for w in result.warnings)
+
+
+class TestIssueToolNameWarning:
+    """Tests for issue_tool_name_warning function."""
+
+    def test_logs_warnings(self, caplog: pytest.LogCaptureFixture) -> None:
+        """Warnings should be logged at WARNING level."""
+        warnings = ["Warning 1", "Warning 2"]
+        with caplog.at_level(logging.WARNING):
+            issue_tool_name_warning("test-tool", warnings)
+
+        assert 'Tool name validation warning for "test-tool"' in caplog.text
+        assert "- Warning 1" in caplog.text
+        assert "- Warning 2" in caplog.text
+        assert "Tool registration will proceed" in caplog.text
+        assert "SEP-986" in caplog.text
+
+    def test_no_logging_for_empty_warnings(self, caplog: pytest.LogCaptureFixture) -> None:
+        """Empty warnings list should not produce any log output."""
+        with caplog.at_level(logging.WARNING):
+            issue_tool_name_warning("test-tool", [])
+
+        assert caplog.text == ""
+
+
+class TestValidateAndWarnToolName:
+    """Tests for validate_and_warn_tool_name function."""
+
+    def test_returns_true_for_valid_name(self) -> None:
+        """Valid names should return True."""
+        assert validate_and_warn_tool_name("valid-tool-name") is True
+
+    def test_returns_false_for_invalid_name(self) -> None:
+        """Invalid names should return False."""
+        assert validate_and_warn_tool_name("") is False
+        assert validate_and_warn_tool_name("a" * 129) is False
+        assert validate_and_warn_tool_name("invalid name") is False
+
+    def test_logs_warnings_for_invalid_name(self, caplog: pytest.LogCaptureFixture) -> None:
+        """Invalid names should trigger warning logs."""
+        with caplog.at_level(logging.WARNING):
+            validate_and_warn_tool_name("invalid name")
+
+        assert "Tool name validation warning" in caplog.text
+
+    def test_no_warnings_for_clean_valid_name(self, caplog: pytest.LogCaptureFixture) -> None:
+        """Clean valid names should not produce any log output."""
+        with caplog.at_level(logging.WARNING):
+            result = validate_and_warn_tool_name("clean-tool-name")
+
+        assert result is True
+        assert caplog.text == ""
+
+
+class TestEdgeCases:
+    """Test edge cases and robustness."""
+
+    @pytest.mark.parametrize(
+        "tool_name,is_valid,expected_warning_fragment",
+        [
+            ("...", True, "starts or ends with a dot"),
+            ("---", True, "starts or ends with a dash"),
+            ("///", False, "invalid characters"),
+            ("user@name123", False, "invalid characters"),
+        ],
+        ids=[
+            "only_dots",
+            "only_dashes",
+            "only_slashes",
+            "mixed_valid_invalid",
+        ],
+    )
+    def test_edge_cases(self, tool_name: str, is_valid: bool, expected_warning_fragment: str) -> None:
+        """Various edge cases should be handled correctly."""
+        result = validate_tool_name(tool_name)
+        assert result.is_valid is is_valid
+        assert any(expected_warning_fragment in w for w in result.warnings)