diff --git a/src/mcp/server/fastmcp/tools/base.py b/src/mcp/server/fastmcp/tools/base.py index df862135e0..cf89fc8aa1 100644 --- a/src/mcp/server/fastmcp/tools/base.py +++ b/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 == "": raise ValueError("You must provide a name for lambda functions") diff --git a/src/mcp/server/lowlevel/server.py b/src/mcp/server/lowlevel/server.py index 49d289fb75..a0617036f9 100644 --- a/src/mcp/server/lowlevel/server.py +++ b/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,6 +423,7 @@ 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: @@ -429,6 +431,7 @@ async def handler(req: types.ListToolsRequest): # 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)) diff --git a/src/mcp/shared/tool_name_validation.py b/src/mcp/shared/tool_name_validation.py new file mode 100644 index 0000000000..188d5fb146 --- /dev/null +++ b/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 diff --git a/tests/shared/test_tool_name_validation.py b/tests/shared/test_tool_name_validation.py new file mode 100644 index 0000000000..4746f3f9f8 --- /dev/null +++ b/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", "'/'"), + ("user@domain.com", "'@'"), + ], + 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)