diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index da5f1ff3d..93d0ebbf8 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -94,6 +94,15 @@ After evaluating comments: Codex sometimes re-posts old comments that reference code you've already fixed (they appear on the old commit's diff). These are stale — verify the fix is in the latest commit and reply noting the fix is already in place. +## Labels — never apply or invent them + +**Do not apply labels to PRs or issues programmatically, and never create new ones.** Labeling is the maintainer's call (and is often automated). Two hard rules: + +- **Never invent a label.** GitHub's "add labels" API *auto-creates* any label name that doesn't already exist — so a typo or a guessed name silently pollutes the repo's label list with a stray, uncolored duplicate. Adding `breaking` (which does not exist) creates it alongside the real `breaking change` label. +- **Use only labels that already exist.** If you genuinely need to confirm a label, look it up first (`get_label` / the repo's label list) and match the exact name. The canonical names here are specific — e.g. the breaking-change label is **`breaking change`**, not `breaking`; enhancements is **`enhancements`**, features is **`features`**, bugs is **`bugs`**. + +When a change warrants a label (e.g. it's breaking), **say so in the PR body and let the maintainer apply the label** rather than applying it yourself. There is no MCP tool to delete a label, so a mistaken creation can only be cleaned up by hand in repo settings — the cost of guessing is high and one-directional. + ## When a PR is ready A PR is ready for human review when: diff --git a/CLAUDE.md b/CLAUDE.md index 52161a576..a71054b93 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,7 @@ When modifying MCP functionality, changes typically need to be applied across al - Prek hooks are required (run automatically on commits) - Never amend commits to fix prek failures -- Apply PR labels: bugs/breaking/enhancements/features +- Never apply labels manually or invent new ones — the GitHub API auto-creates any unknown label name, polluting the repo's label list. Note the appropriate label in the PR body and let the maintainer/automation apply it. Canonical names: `bugs`, `breaking change`, `enhancements`, `features` (it's `breaking change`, not `breaking`). See the review-pr skill. - Improvements = enhancements (not features) unless specified - **NEVER** force-push on collaborative repos - **ALWAYS** run prek before PRs diff --git a/fastmcp_slim/fastmcp/server/server.py b/fastmcp_slim/fastmcp/server/server.py index 461febb17..b8ac58e1f 100644 --- a/fastmcp_slim/fastmcp/server/server.py +++ b/fastmcp_slim/fastmcp/server/server.py @@ -54,6 +54,7 @@ from fastmcp.exceptions import ( PromptError, ResourceError, ToolError, + ValidationError, ) from fastmcp.mcp_config import MCPConfig from fastmcp.prompts import Prompt @@ -1280,13 +1281,29 @@ class FastMCP( task_meta = replace(task_meta, fn_key=tool.key) try: return await tool._run(arguments or {}, task_meta=task_meta) + except ValidationError as e: + # Argument-validation failure (a bad call). FunctionTool + # converts pydantic's call-validation error into fastmcp's + # ValidationError (see #4128) so it can be filtered as a + # client error. Log the underlying detail without a URL or + # traceback, matching the previous pydantic-error logging. + cause = e.__cause__ + detail = ( + cause.errors(include_url=False) + if isinstance(cause, PydanticValidationError) + else str(e) + ) + logger.warning("Invalid arguments for tool %r: %s", name, detail) + raise except FastMCPError as e: logger.log( e.log_level, f"Error calling tool {name!r}", exc_info=False ) raise except PydanticValidationError as e: - # fastmcp's own ValidationError is a FastMCPError, already handled above. + # A pydantic error that is NOT an argument-validation failure + # (e.g. raised by a non-FunctionTool's own validation). Kept + # for backward compatibility. logger.warning( "Invalid arguments for tool %r: %s", name, diff --git a/fastmcp_slim/fastmcp/tools/function_tool.py b/fastmcp_slim/fastmcp/tools/function_tool.py index a882edc4e..7e965ef9b 100644 --- a/fastmcp_slim/fastmcp/tools/function_tool.py +++ b/fastmcp_slim/fastmcp/tools/function_tool.py @@ -4,9 +4,11 @@ from __future__ import annotations import functools import inspect +import logging import warnings from collections.abc import Callable from dataclasses import dataclass, field +from functools import lru_cache from types import MethodType from typing import ( TYPE_CHECKING, @@ -24,12 +26,13 @@ from typing import ( import anyio from mcp.shared.exceptions import McpError from mcp.types import ErrorData, Icon, ToolAnnotations -from pydantic import Field +from pydantic import Field, TypeAdapter +from pydantic import ValidationError as PydanticValidationError from pydantic.json_schema import SkipJsonSchema import fastmcp from fastmcp.decorators import get_fastmcp_meta, resolve_task_config -from fastmcp.exceptions import FastMCPDeprecationWarning +from fastmcp.exceptions import FastMCPDeprecationWarning, ValidationError from fastmcp.tools.base import ( Tool, ToolResult, @@ -55,6 +58,75 @@ if TYPE_CHECKING: from docket import Docket from docket.execution import Execution + +class _ToolBodyError(Exception): + """Marks a ``pydantic.ValidationError`` raised while executing a tool's body. + + Pydantic validates a tool's arguments *before* invoking the body, so a bare + ``pydantic.ValidationError`` surfacing from the call adapter is unambiguously + an argument-validation failure (a bad call). Errors a tool raises from its + own body — e.g. constructing a model from upstream data — are a different + class of problem (a server-side bug) that must not be reclassified as a bad + call. We wrap the body so those are tagged and can be told apart. See #4128. + """ + + +@lru_cache(maxsize=5000) +def _wrap_body_errors(fn: Callable[..., Any]) -> Callable[..., Any]: + """Wrap ``fn`` so a ``pydantic.ValidationError`` raised by its body is + re-raised as ``_ToolBodyError``. + + The wrapper preserves ``fn``'s signature and annotations so the cached + ``TypeAdapter`` validates arguments identically — only body execution is + affected. Argument validation happens before the wrapper is called, so it + keeps raising a bare ``pydantic.ValidationError``. + """ + if is_coroutine_function(fn): + + async def wrapper(*args: Any, **kwargs: Any) -> Any: + try: + return await fn(*args, **kwargs) + except PydanticValidationError as e: + raise _ToolBodyError from e + else: + + def wrapper(*args: Any, **kwargs: Any) -> Any: + try: + return fn(*args, **kwargs) + except PydanticValidationError as e: + raise _ToolBodyError from e + + # Mirror the original callable so TypeAdapter builds the identical schema and + # binds arguments the same way. Annotations must cover every signature + # parameter or pydantic's call-schema generation raises KeyError — so prefer + # resolved type hints (handles string/forward refs) but fall back to the + # signature's own annotations, which is the only source for callables like + # functools.partial that carry no __annotations__. + try: + resolved_hints = get_type_hints(fn, include_extras=True) + except Exception: + resolved_hints = {} + sig = inspect.signature(fn) + annotations: dict[str, Any] = {} + for param_name, param in sig.parameters.items(): + if param_name in resolved_hints: + annotations[param_name] = resolved_hints[param_name] + elif param.annotation is not inspect.Parameter.empty: + annotations[param_name] = param.annotation + if "return" in resolved_hints: + annotations["return"] = resolved_hints["return"] + elif sig.return_annotation is not inspect.Signature.empty: + annotations["return"] = sig.return_annotation + + wrapper.__signature__ = sig # type: ignore[attr-defined] # ty: ignore[invalid-assignment] + wrapper.__annotations__ = annotations + wrapper.__name__ = getattr(fn, "__name__", "wrapper") + wrapper.__doc__ = getattr(fn, "__doc__", None) + wrapper.__module__ = getattr(fn, "__module__", wrapper.__module__) + wrapper.__qualname__ = getattr(fn, "__qualname__", wrapper.__qualname__) + return wrapper + + F = TypeVar("F", bound=Callable[..., Any]) @@ -317,59 +389,88 @@ class FunctionTool(Tool): wrapper_fn = without_injected_parameters( self.fn, run_in_thread=self.run_in_thread ) - type_adapter = get_cached_typeadapter(wrapper_fn) + # Tag pydantic errors raised by the body so they can be distinguished + # from argument-validation errors (which pydantic raises first). See #4128. + exec_fn = _wrap_body_errors(wrapper_fn) + type_adapter = get_cached_typeadapter(exec_fn) + exec_is_async = is_coroutine_function(wrapper_fn) - # Apply timeout if configured. Combining timeout with - # run_in_thread=False on a sync function is rejected at - # registration (see FunctionTool.from_function), so the timeout - # path here only needs to handle async and threadpool-sync. - if self.timeout is not None: - try: - with anyio.fail_after(self.timeout): - # Thread pool execution for sync functions, direct await for async - if is_coroutine_function(wrapper_fn): - result = await type_adapter.validate_python(arguments) - else: - # Sync function: run in threadpool to avoid blocking - result = await call_sync_fn_in_threadpool( - type_adapter.validate_python, arguments + try: + if self.timeout is not None: + try: + with anyio.fail_after(self.timeout): + result = await self._execute( + type_adapter, exec_is_async, arguments ) - # Handle sync wrappers that return awaitables - if inspect.isawaitable(result): - result = await result - # Materialize generators inside timeout scope so slow - # generators don't run past the configured timeout - result = await self._materialize_generator(result) - except TimeoutError: - logger.warning( - f"Tool '{self.name}' timed out after {self.timeout}s. " - f"Consider using task=True for long-running operations. " - f"See https://gofastmcp.com/servers/tasks" - ) - raise McpError( - ErrorData( - code=-32000, - message=f"Tool '{self.name}' execution timed out after {self.timeout}s", + except TimeoutError: + logger.warning( + f"Tool '{self.name}' timed out after {self.timeout}s. " + f"Consider using task=True for long-running operations. " + f"See https://gofastmcp.com/servers/tasks" ) - ) from None - else: - # No timeout: use existing execution path - if is_coroutine_function(wrapper_fn): - result = await type_adapter.validate_python(arguments) - elif self.run_in_thread: - result = await call_sync_fn_in_threadpool( - type_adapter.validate_python, arguments - ) - if inspect.isawaitable(result): - result = await result + raise McpError( + ErrorData( + code=-32000, + message=f"Tool '{self.name}' execution timed out after {self.timeout}s", + ) + ) from None else: - result = type_adapter.validate_python(arguments) - if inspect.isawaitable(result): - result = await result - result = await self._materialize_generator(result) + result = await self._execute(type_adapter, exec_is_async, arguments) + except PydanticValidationError as e: + # Body errors are re-raised as _ToolBodyError, so a bare pydantic + # ValidationError here is an argument-validation failure (a bad call). + # Convert it to fastmcp's ValidationError so the middleware chain and + # downstream error taxonomy (e.g. Sentry filters) can treat it as a + # client error rather than a server bug. + raise ValidationError(str(e), log_level=logging.WARNING) from e + except _ToolBodyError as e: + # The tool's own body raised a pydantic ValidationError. Surface the + # original so it is treated as a server-side error, hiding the + # internal sentinel while preserving the error's own chained cause. + original = e.__cause__ + assert original is not None + raise original from original.__cause__ return self.convert_result(result) + async def _execute( + self, + type_adapter: TypeAdapter[Any], + exec_is_async: bool, + arguments: dict[str, Any], + ) -> Any: + """Validate arguments and execute the tool body. + + Argument validation runs first and raises a bare + ``pydantic.ValidationError`` on bad input. Body execution (awaiting the + result and materializing generators) is wrapped so any pydantic error it + raises is tagged as ``_ToolBodyError``. + """ + # Combining timeout with run_in_thread=False on a sync function is + # rejected at registration (see FunctionTool.from_function), so this only + # needs to handle async and threadpool-sync under a timeout. + if exec_is_async: + # Argument validation is synchronous; the body runs on await below. + result = type_adapter.validate_python(arguments) + elif self.run_in_thread: + # Sync function: run in threadpool to avoid blocking the event loop. + result = await call_sync_fn_in_threadpool( + type_adapter.validate_python, arguments + ) + else: + result = type_adapter.validate_python(arguments) + + try: + if inspect.isawaitable(result): + result = await result + # Materialize generators (here, so slow generators are still bound by + # any configured timeout scope). + return await self._materialize_generator(result) + except PydanticValidationError as e: + # A pydantic error from awaiting the result or materializing a + # generator is body execution, not argument validation. + raise _ToolBodyError from e + @staticmethod async def _materialize_generator(result: Any) -> Any: """Consume generators/async generators into lists. @@ -450,7 +551,13 @@ class FunctionTool(Tool): if annotation is None: continue adapter = get_cached_typeadapter(annotation) - coerced[name] = adapter.validate_python(value) + try: + coerced[name] = adapter.validate_python(value) + except PydanticValidationError as e: + # Argument coercion failure on the task path is a bad call, just + # like the synchronous path — surface it as fastmcp's + # ValidationError so it is classified consistently (see #4128). + raise ValidationError(str(e), log_level=logging.WARNING) from e return coerced diff --git a/tests/server/providers/local_provider_tools/test_parameters.py b/tests/server/providers/local_provider_tools/test_parameters.py index 92e1af6de..45ecee475 100644 --- a/tests/server/providers/local_provider_tools/test_parameters.py +++ b/tests/server/providers/local_provider_tools/test_parameters.py @@ -109,7 +109,7 @@ class TestToolParameters: assert result.content[0].data == base64.b64encode(b"fake png data").decode() async def test_tool_with_invalid_input(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -149,7 +149,7 @@ class TestToolParameters: assert result.structured_content == {"result": True} async def test_annotated_field_validation(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -164,7 +164,7 @@ class TestToolParameters: await mcp.call_tool("analyze", {"x": 0}) async def test_default_field_validation(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -179,7 +179,7 @@ class TestToolParameters: await mcp.call_tool("analyze", {"x": 0}) async def test_default_field_is_still_required_if_no_default_specified(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -191,7 +191,7 @@ class TestToolParameters: await mcp.call_tool("analyze", {}) async def test_literal_type_validation_error(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -216,7 +216,7 @@ class TestToolParameters: assert result.structured_content == {"result": "a"} async def test_enum_type_validation_error(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -251,7 +251,7 @@ class TestToolParameters: assert result.structured_content == {"result": "red"} async def test_union_type_validation(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -285,7 +285,7 @@ class TestToolParameters: assert result.structured_content == {"result": str(test_path)} async def test_path_type_error(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -310,7 +310,7 @@ class TestToolParameters: assert result.structured_content == {"result": str(test_uuid)} async def test_uuid_type_error(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() @@ -344,7 +344,7 @@ class TestToolParameters: assert result.structured_content == {"result": "2021-01-01T00:00:00"} async def test_datetime_type_error(self): - from pydantic import ValidationError + from fastmcp.exceptions import ValidationError mcp = FastMCP() diff --git a/tests/server/test_dependencies.py b/tests/server/test_dependencies.py index 37e5aa537..e43672376 100644 --- a/tests/server/test_dependencies.py +++ b/tests/server/test_dependencies.py @@ -450,10 +450,11 @@ async def test_argument_validation_with_dependencies(mcp: FastMCP): assert result.structured_content is not None assert result.structured_content["result"] == "age=25" - # Invalid argument type should fail validation - import pydantic + # Invalid argument type should fail validation. Argument-validation errors + # surface as fastmcp's ValidationError (see #4128), not raw pydantic. + from fastmcp.exceptions import ValidationError - with pytest.raises(pydantic.ValidationError): + with pytest.raises(ValidationError): await mcp.call_tool("validated_tool", {"age": "not a number"}) @@ -598,10 +599,12 @@ async def test_external_user_cannot_override_dependency(mcp: FastMCP): assert result.structured_content is not None assert "admin=not_admin" in result.structured_content["result"] - # Try to override dependency - rejected (not in schema) - import pydantic + # Try to override dependency - rejected (not in schema). The rejection is an + # argument-validation error, so it surfaces as fastmcp's ValidationError + # (see #4128). + from fastmcp.exceptions import ValidationError - with pytest.raises(pydantic.ValidationError): + with pytest.raises(ValidationError): await mcp.call_tool("check_permission", {"action": "read", "admin": "hacker"}) diff --git a/tests/tools/tool/test_argument_validation.py b/tests/tools/tool/test_argument_validation.py new file mode 100644 index 000000000..8f41cea5d --- /dev/null +++ b/tests/tools/tool/test_argument_validation.py @@ -0,0 +1,125 @@ +"""Tests for distinguishing argument-validation errors from tool-body errors. + +See https://github.com/PrefectHQ/fastmcp/issues/4128: a bad call (invalid +arguments) should surface as fastmcp's ``ValidationError`` so downstream error +taxonomy (middleware, Sentry filters) can treat it as a client error, while a +``pydantic.ValidationError`` raised by the tool's own body is a server-side bug +and must propagate unchanged. +""" + +from typing import Annotated, Any + +import pytest +from pydantic import BaseModel, Field +from pydantic import ValidationError as PydanticValidationError + +from fastmcp.exceptions import ValidationError +from fastmcp.tools.base import Tool + + +class _Inner(BaseModel): + x: int + + +class TestArgumentValidationErrors: + """Invalid arguments are converted to fastmcp's ValidationError.""" + + async def test_async_constraint_violation(self): + async def tool_fn(n: Annotated[int, Field(le=10)]) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + await tool.run({"n": 20}) + + async def test_sync_constraint_violation(self): + def tool_fn(n: Annotated[int, Field(le=10)]) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + await tool.run({"n": 20}) + + async def test_wrong_type_is_argument_error(self): + async def tool_fn(n: int) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + await tool.run({"n": "not-an-int"}) + + async def test_missing_required_argument(self): + async def tool_fn(n: int) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + await tool.run({}) + + +class TestToolBodyErrors: + """A pydantic error raised by the body is NOT reclassified as a bad call.""" + + async def test_async_body_pydantic_error_propagates(self): + async def tool_fn(data: str) -> int: + bad_value: Any = "not-an-int" + _Inner(x=bad_value) # raises a pydantic ValidationError from the body + return 1 + + tool = Tool.from_function(tool_fn) + with pytest.raises(PydanticValidationError): + await tool.run({"data": "valid"}) + # And it must not be fastmcp's ValidationError. + with pytest.raises(PydanticValidationError) as exc_info: + await tool.run({"data": "valid"}) + assert not isinstance(exc_info.value, ValidationError) + + async def test_sync_body_pydantic_error_propagates(self): + def tool_fn(data: str) -> int: + bad_value: Any = "not-an-int" + _Inner(x=bad_value) # raises a pydantic ValidationError from the body + return 1 + + tool = Tool.from_function(tool_fn) + with pytest.raises(PydanticValidationError): + await tool.run({"data": "valid"}) + + +class TestTaskArgumentValidation: + """The task-execution path (coerce_task_arguments) converts arg errors too.""" + + def test_coerce_task_arguments_wrong_type(self): + def tool_fn(n: int) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + tool.coerce_task_arguments({"n": "not-an-int"}) + + def test_coerce_task_arguments_constraint_violation(self): + def tool_fn(n: Annotated[int, Field(le=10)]) -> int: + return n + + tool = Tool.from_function(tool_fn) + with pytest.raises(ValidationError): + tool.coerce_task_arguments({"n": 20}) + + +class TestValidCallsStillWork: + """Regression: the happy path is unaffected.""" + + async def test_async_valid_call(self): + async def tool_fn(n: Annotated[int, Field(le=10)]) -> int: + return n * 2 + + tool = Tool.from_function(tool_fn) + result = await tool.run({"n": 5}) + assert result.structured_content == {"result": 10} + + async def test_sync_valid_call(self): + def tool_fn(n: int) -> int: + return n + 1 + + tool = Tool.from_function(tool_fn) + result = await tool.run({"n": 5}) + assert result.structured_content == {"result": 6}