-
Notifications
You must be signed in to change notification settings - Fork 3.8k
MCPServer: content-block returns are unstructured, prompt messages take Image/Audio #3320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
02a0c5e
5234189
0be83f5
d26de07
988fbbf
b1f7a29
9cc83c9
2251112
6f95028
6015f97
4fadfdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
|
|
||
| import functools | ||
| from collections.abc import Awaitable, Callable, Sequence | ||
| from typing import TYPE_CHECKING, Any, Literal | ||
| from typing import TYPE_CHECKING, Annotated, Any, Literal | ||
|
|
||
| import anyio.to_thread | ||
| import pydantic_core | ||
|
|
@@ -13,6 +13,7 @@ | |
|
|
||
| from mcp.server.mcpserver.utilities.context_injection import find_context_parameter, inject_context | ||
| from mcp.server.mcpserver.utilities.func_metadata import func_metadata | ||
| from mcp.server.mcpserver.utilities.types import Audio, Image | ||
| from mcp.shared._callable_inspection import is_async_callable | ||
| from mcp.shared.exceptions import MCPError | ||
|
|
||
|
|
@@ -22,14 +23,26 @@ | |
|
|
||
|
|
||
| class Message(BaseModel): | ||
| """Base class for all prompt messages.""" | ||
| """Base class for all prompt messages. | ||
|
|
||
| `content` may be a plain string (wrapped in `TextContent`), an `Image` or `Audio` | ||
| helper (converted to `ImageContent` / `AudioContent`, reading the file for path-backed | ||
| helpers), or any ready-made content block. | ||
|
|
||
| Raises: | ||
| OSError: If a path-backed `Image` or `Audio` cannot be read. | ||
| """ | ||
|
maxisbey marked this conversation as resolved.
|
||
|
|
||
| role: Literal["user", "assistant"] | ||
| content: ContentBlock | ||
|
|
||
| def __init__(self, content: str | ContentBlock, **kwargs: Any): | ||
| def __init__(self, content: str | ContentBlock | Image | Audio, **kwargs: Any): | ||
|
maxisbey marked this conversation as resolved.
|
||
| if isinstance(content, str): | ||
| content = TextContent(type="text", text=content) | ||
| elif isinstance(content, Image): | ||
| content = content.to_image_content() | ||
| elif isinstance(content, Audio): | ||
| content = content.to_audio_content() | ||
|
maxisbey marked this conversation as resolved.
|
||
| super().__init__(content=content, **kwargs) | ||
|
|
||
|
|
||
|
|
@@ -38,7 +51,7 @@ | |
|
|
||
| role: Literal["user", "assistant"] = "user" | ||
|
|
||
| def __init__(self, content: str | ContentBlock, **kwargs: Any): | ||
| def __init__(self, content: str | ContentBlock | Image | Audio, **kwargs: Any): | ||
| super().__init__(content=content, **kwargs) | ||
|
|
||
|
|
||
|
|
@@ -47,13 +60,18 @@ | |
|
|
||
| role: Literal["user", "assistant"] = "assistant" | ||
|
|
||
| def __init__(self, content: str | ContentBlock, **kwargs: Any): | ||
| def __init__(self, content: str | ContentBlock | Image | Audio, **kwargs: Any): | ||
| super().__init__(content=content, **kwargs) | ||
|
|
||
|
|
||
| message_validator = TypeAdapter[UserMessage | AssistantMessage](UserMessage | AssistantMessage) | ||
| # Both classes accept either role, so the first arm always matches: validate left to right rather than | ||
| # trying both (which converted - and for path-backed Image/Audio, read - the content twice). | ||
| message_validator: TypeAdapter[UserMessage | AssistantMessage] = TypeAdapter( | ||
| Annotated[UserMessage | AssistantMessage, Field(union_mode="left_to_right")] | ||
| ) | ||
|
maxisbey marked this conversation as resolved.
|
||
|
|
||
| SyncPromptResult = str | Message | dict[str, Any] | InputRequiredResult | Sequence[str | Message | dict[str, Any]] | ||
| _PromptResultItem = str | ContentBlock | Image | Audio | Message | dict[str, Any] | ||
| SyncPromptResult = _PromptResultItem | InputRequiredResult | Sequence[_PromptResultItem] | ||
| PromptResult = SyncPromptResult | Awaitable[SyncPromptResult] | ||
|
|
||
|
|
||
|
|
@@ -89,7 +107,7 @@ | |
| """Create a Prompt from a function. | ||
|
|
||
| The function can return: | ||
| - A string (converted to a message) | ||
| - A string, content block, `Image` or `Audio` (each becomes a user message) | ||
|
maxisbey marked this conversation as resolved.
|
||
| - A Message object | ||
| - A dict (converted to a message) | ||
| - A sequence of any of the above | ||
|
|
@@ -105,10 +123,9 @@ | |
| if context_kwarg is None: # pragma: no branch | ||
| context_kwarg = find_context_parameter(fn) | ||
|
|
||
| # Get schema from func_metadata, excluding context parameter | ||
| # Only the argument model is needed; a prompt has no output schema to derive | ||
| func_arg_metadata = func_metadata( | ||
| fn, | ||
| skip_names=[context_kwarg] if context_kwarg is not None else [], | ||
| fn, skip_names=[context_kwarg] if context_kwarg is not None else [], structured_output=False | ||
| ) | ||
| parameters = func_arg_metadata.arg_model.model_json_schema() | ||
|
|
||
|
|
@@ -179,19 +196,15 @@ | |
| # Convert result to messages | ||
| messages: list[Message] = [] | ||
| for msg in result: # type: ignore[reportUnknownVariableType] | ||
| try: | ||
| if isinstance(msg, Message): | ||
| messages.append(msg) | ||
| elif isinstance(msg, dict): | ||
| messages.append(message_validator.validate_python(msg)) | ||
| elif isinstance(msg, str): | ||
| content = TextContent(type="text", text=msg) | ||
| messages.append(UserMessage(content=content)) | ||
| else: # pragma: no cover | ||
| content = pydantic_core.to_json(msg, fallback=str, indent=2).decode() | ||
| messages.append(Message(role="user", content=content)) | ||
| except Exception: # pragma: no cover | ||
| raise ValueError(f"Could not convert prompt result to message: {msg}") | ||
| 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 | ||
|
Check warning on line 205 in src/mcp/server/mcpserver/prompts/base.py
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [quality] Rewritten render() conversion loop re-adds Extended reasoning...Concrete cost: a documented library-code behavior (a prompt returning a non-content value such as an int or a BaseModel is JSON-dumped via pydantic_core.to_json into a user text message, src/mcp/server/mcpserver/prompts/base.py:205-207) stays permanently excluded from the repo's 100%-coverage gate. CLAUDE.md -> AGENTS.md states 'Avoid adding new Verification: nit — src/mcp/server/mcpserver/prompts/base.py:205 in the rewritten loop reads |
||
| content = pydantic_core.to_json(msg, fallback=str, indent=2).decode() | ||
| messages.append(Message(role="user", content=content)) | ||
|
Check notice on line 207 in src/mcp/server/mcpserver/prompts/base.py
|
||
|
Comment on lines
+205
to
+207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing, surfaced by this rewrite: a prompt function returning the wire type mcp_types.PromptMessage (the type GetPromptResult actually carries, and what lowlevel-server prompt handlers return) falls through the rewritten conversion loop to the JSON-dump fallback: it is not a mcpserver Message, not a dict, and not in the new Extended reasoning...A user migrating a lowlevel-server prompt handler (or following the spec's vocabulary) writes Verification: pre-existing — behavior predates this PR (the old loop also JSON-dumped anything that wasn't str/Message/dict), but the PR rewrites this exact conversion loop and widens the accepted types, so the gap is squarely in reviewed code. The claim checks out line by line in /home/claude/python-sdk/src/mcp/server/mcpserver/prompts/base.py: the loop at lines 198-207 handles |
||
|
|
||
| return messages | ||
| except MCPError: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,25 @@ | |
| return isinstance(obj, type) and issubclass(obj, InputRequiredResult) | ||
|
|
||
|
|
||
| _CONTENT_TYPES = (*get_args(ContentBlock), Image, Audio) | ||
| # `_convert_to_content` unrolls list/tuple values; a `Sequence[...]` annotation is one of those at runtime. | ||
| _CONTENT_SEQUENCE_ORIGINS = (list, tuple, Sequence) | ||
|
|
||
|
|
||
| def _returns_content(annotation: Any) -> bool: | ||
| """Whether a return annotation declares content blocks or the `Image`/`Audio` helpers, bare or as | ||
| the items of a list/tuple or the arms of a union: the values `_convert_to_content` renders as blocks | ||
| rather than dumping as data. Keep the two in sync.""" | ||
| origin = get_origin(annotation) | ||
| if origin is None: | ||
| return isinstance(annotation, type) and issubclass(annotation, _CONTENT_TYPES) | ||
| if origin is Annotated: | ||
| return _returns_content(get_args(annotation)[0]) | ||
| if is_union_origin(origin) or origin in _CONTENT_SEQUENCE_ORIGINS: | ||
| return any(_returns_content(arg) for arg in get_args(annotation)) | ||
| return False | ||
|
|
||
|
|
||
| class StrictJsonSchema(GenerateJsonSchema): | ||
| """A JSON schema generator that raises exceptions instead of emitting warnings. | ||
|
|
||
|
|
@@ -222,6 +241,9 @@ | |
| - TypedDict - converted to a Pydantic model with same fields | ||
| - Dataclasses and other annotated classes - converted to Pydantic models | ||
| - Generic types (list, dict, Union, etc.) - wrapped in a model with a 'result' field | ||
| - Content blocks (TextContent, EmbeddedResource, ...), Image and Audio, bare or inside a | ||
| list, tuple or union - unstructured when auto-detecting; structured_output=True bypasses | ||
| this rule (a content block then publishes its own schema; Image/Audio have none and raise) | ||
|
Check warning on line 246 in src/mcp/server/mcpserver/utilities/func_metadata.py
|
||
|
Comment on lines
+244
to
+246
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [quality] nit: in-code docs for the new content rule omit Extended reasoning...Concrete cost: the func_metadata docstring is the reference for the public Verification: nit — the claim is factually accurate. In /home/claude/python-sdk/src/mcp/server/mcpserver/utilities/func_metadata.py, line 38 defines |
||
|
|
||
| Returns: | ||
| A FuncMetadata object containing: | ||
|
|
@@ -345,6 +367,13 @@ | |
| else: | ||
| original_annotation = effective_annotation | ||
|
|
||
| if structured_output is None and _returns_content(return_type_expr): | ||
| # Content blocks and the Image/Audio helpers are what the model reads, not data for the | ||
| # application: a derived schema would advertise the block's own model as output_schema (and, | ||
| # unless the tool builds its own CallToolResult, echo every block into structured_content). | ||
| # structured_output=True still forces one. | ||
| return FuncMetadata(arg_model=arguments_model) | ||
|
|
||
| output_model, output_schema, wrap_output = _try_create_model_and_schema( | ||
|
maxisbey marked this conversation as resolved.
|
||
| original_annotation, return_type_expr, func.__name__ | ||
| ) | ||
|
|
@@ -546,7 +575,7 @@ | |
| Note: This conversion logic comes from previous versions of MCPServer and is being | ||
| retained for purposes of backwards compatibility. It produces different unstructured | ||
| output than the lowlevel server tool call handler, which just serializes structured | ||
| content verbatim. | ||
| content verbatim. `_returns_content` is the annotation-level mirror of these branches. | ||
| """ | ||
| if result is None: # pragma: no cover | ||
| return [] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 [quality] nit: stale claim "There is one way to end up unstructured without asking for it" now contradicts the new "Content blocks and media" section added two paragraphs above on the same page, which introduces a second default opt-out path (content-block/Image/Audio return annotations derive no schema). The PR updated the Recap bullet at line 247 to list both opt-outs ("Content blocks,
ImageandAudioopt out by default; a class without type hints opts out silently") but left the section opener asserting the annotation-less class is the only such path.Extended reasoning...
Concrete cost: the published structured-output page contradicts itself. A reader who lands on the "A class without type hints" section (or skims from its heading) is told the only way to get an unstructured tool without passing structured_output=False is an annotation-less class, and will not suspect that their
-> EmbeddedResource/-> list[TextContent]tool also silently stopped advertising outputSchema after this release — exactly the behaviour change the PR calls out for release notes. Fix is one sentence: reword the opener (e.g. "Besides content blocks, there is one more way...") so the two sections on the same page agree.Verification: nit — the factual basis checks out. docs/servers/structured-output.md line 217 still reads "There is one way to end up unstructured without asking for it: return a class that has no annotations on its body." — pre-existing text the PR did not touch — while the new "Content blocks and media" section added two paragraphs above (lines 211–213) introduces a second default path to an unstructur