diff --git a/src/mcp/server/mcpserver/prompts/__init__.py b/src/mcp/server/mcpserver/prompts/__init__.py index 763726964c..5ea52ce2fc 100644 --- a/src/mcp/server/mcpserver/prompts/__init__.py +++ b/src/mcp/server/mcpserver/prompts/__init__.py @@ -1,4 +1,4 @@ -from .base import Prompt +from .base import Prompt, PromptValidationError from .manager import PromptManager -__all__ = ["Prompt", "PromptManager"] +__all__ = ["Prompt", "PromptManager", "PromptValidationError"] diff --git a/src/mcp/server/mcpserver/prompts/base.py b/src/mcp/server/mcpserver/prompts/base.py index d30c0b3c60..b0f4d07ec8 100644 --- a/src/mcp/server/mcpserver/prompts/base.py +++ b/src/mcp/server/mcpserver/prompts/base.py @@ -22,6 +22,18 @@ from mcp.server.mcpserver.context import Context +class PromptValidationError(ValueError): + """Raised when prompt arguments fail validation (e.g. a required argument is missing). + + A `ValueError` subclass so existing callers that catch `ValueError` are + unaffected. It exists to distinguish this expected, user-input validation + failure from the generic `ValueError` `Prompt.render` also raises when the + prompt function itself raises an unexpected exception - callers that log + the two differently (see `MCPServer.get_prompt`) can `except` this + specifically without downgrading a genuine crash to a quiet warning. + """ + + class Message(BaseModel): """Base class for all prompt messages. @@ -166,7 +178,8 @@ async def render( through unchanged so the multi-round-trip flow reaches the client. Raises: - ValueError: If required arguments are missing, or if rendering fails. + PromptValidationError: If required arguments are missing. + ValueError: If the prompt function itself raises an unexpected exception. """ # Validate required arguments if self.arguments: @@ -174,7 +187,7 @@ async def render( provided = set(arguments or {}) missing = required - provided if missing: - raise ValueError(f"Missing required arguments: {missing}") + raise PromptValidationError(f"Missing required arguments: {missing}") try: # Add context to arguments if needed @@ -183,6 +196,7 @@ async def render( fn = self.fn if is_async_callable(fn): result = await fn(**call_args) + else: result = await anyio.to_thread.run_sync(functools.partial(self.fn, **call_args)) @@ -198,16 +212,21 @@ async def render( for msg in result: # type: ignore[reportUnknownVariableType] if isinstance(msg, Message): messages.append(msg) + elif isinstance(msg, dict): messages.append(message_validator.validate_python(msg)) + elif isinstance(msg, str | ContentBlock | Image | Audio): # bare content is one user message messages.append(UserMessage(msg)) + else: # pragma: no cover content = pydantic_core.to_json(msg, fallback=str, indent=2).decode() messages.append(Message(role="user", content=content)) return messages + except MCPError: raise + except Exception as e: raise ValueError(f"Error rendering prompt {self.name}: {e}") diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index b3a3cb3bcd..a3536b1da9 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -72,7 +72,7 @@ from mcp.server.lowlevel.server import lifespan as default_lifespan from mcp.server.mcpserver.context import Context from mcp.server.mcpserver.exceptions import ResourceError, ResourceNotFoundError -from mcp.server.mcpserver.prompts import Prompt, PromptManager +from mcp.server.mcpserver.prompts import Prompt, PromptManager, PromptValidationError from mcp.server.mcpserver.resources import ( DEFAULT_RESOURCE_SECURITY, FunctionResource, @@ -1283,6 +1283,7 @@ async def get_prompt( """ if context is None: context = Context(mcp_server=self, subscriptions=self._subscriptions) + try: prompt = self._prompt_manager.get_prompt(name) if not prompt: @@ -1296,8 +1297,22 @@ async def get_prompt( description=prompt.description, messages=pydantic_core.to_jsonable_python(rendered), ) + except MCPError: raise + + except PromptValidationError as e: + # Expected user-input validation failures, like missing required + # arguments, don't need a full traceback. Log a concise warning + # instead of the exc_info dump reserved for unexpected errors. + + # `Prompt.render` also raises a plain `ValueError` when the prompt + # function itself throws, so this narrower type is caught here + # instead of `ValueError` to avoid swallowing that traceback too. + + logger.warning(f"Error getting prompt {name}: {e}") + raise ValueError(str(e)) from e + except Exception as e: logger.exception(f"Error getting prompt {name}") raise ValueError(str(e)) from e diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index f9055a4ed4..1e6121e045 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -1,4 +1,5 @@ import base64 +import logging from pathlib import Path from types import SimpleNamespace from typing import Any @@ -1527,6 +1528,67 @@ def prompt_fn(name: str) -> str: ... # pragma: no branch with pytest.raises(MCPError, match="Missing required arguments"): await client.get_prompt("prompt_fn") + async def test_get_prompt_missing_args_logs_warning_without_traceback( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Regression for issue #3342: missing-argument ValueErrors are an expected + validation failure, so `MCPServer.get_prompt`'s own logger should emit + a plain warning without exc_info, instead of a full traceback. + + Note: `jsonrpc_dispatcher` has a separate, intentional catch-all that + logs a traceback for any handler exception it doesn't recognize as + `MCPError`/`ValidationError`. However, that generic safety net is out of + scope here and unaffected by this fix PR #3347. + """ + mcp = MCPServer() + + @mcp.prompt() + def prompt_fn(name: str) -> str: ... # pragma: no branch. + + # In Python 3.14, coverage.py undercounts a branch when `caplog.at_level` + # wraps `async with Client(...): with pytest.raises(...): await ...` as + # a 4th nesting level around a single `await` statement (3 levels of + # nesting is OK; 4 is not). So `caplog.set_level` avoids extra `with` layer. + caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server") + async with Client(mcp, mode="legacy") as client: + with pytest.raises(MCPError, match="Missing required arguments"): + await client.get_prompt("prompt_fn") + + server_records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert len(server_records) == 1 + + # Missing-argument PromptValidationError, which is a ValueError subclass, + # logs as a plain warning, with no exc_info/traceback at all. + assert server_records[0].levelno == logging.WARNING + assert not server_records[0].exc_info + + async def test_get_prompt_unexpected_error_still_logs_traceback(self, caplog: pytest.LogCaptureFixture) -> None: + """A prompt function raising an unexpected (non-validation) exception must + still be logged with a full traceback, even though `Prompt.render` also + wraps it as a plain `ValueError` — same wire type as the missing-argument + case, but not a `PromptValidationError`, so it must not be downgraded.""" + mcp = MCPServer() + + @mcp.prompt() + def prompt_fn() -> str: + raise KeyError("boom") + + # In Python 3.14, coverage.py undercounts a branch when `caplog.at_level` + # wraps `async with Client(...): with pytest.raises(...): await ...` as + # a 4th nesting level around a single `await` statement (3 levels of + # nesting is OK; 4 is not). So `caplog.set_level` avoids extra `with` layer. + caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server") + async with Client(mcp, mode="legacy") as client: + with pytest.raises(MCPError): + await client.get_prompt("prompt_fn") + + server_records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"] + assert len(server_records) == 1 + + # Unexpected errors should have error log with exc_info. + assert server_records[0].levelno == logging.ERROR + assert server_records[0].exc_info is not None + async def test_resource_decorator_rfc6570_reserved_expansion(): # Regression: old regex-based param extraction couldn't see `path`