-
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
base: main
Are you sure you want to change the base?
Changes from all commits
02a0c5e
5234189
0be83f5
d26de07
988fbbf
b1f7a29
9cc83c9
2251112
6f95028
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 |
|---|---|---|
|
|
@@ -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,22 @@ | |
|
|
||
|
|
||
| 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`), or any ready-made content block. | ||
| """ | ||
|
|
||
| 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 +47,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 +56,14 @@ | |
|
|
||
| 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) | ||
|
|
||
| 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 +99,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 +115,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() | ||
|
|
||
|
|
@@ -184,9 +193,8 @@ | |
| 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)) | ||
| elif isinstance(msg, str | ContentBlock | Image | Audio): # bare content is one user message | ||
| messages.append(UserMessage(msg)) | ||
|
Check warning on line 197 in src/mcp/server/mcpserver/prompts/base.py
|
||
|
Comment on lines
193
to
+197
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: Prompt.render's new message-conversion branch performs synchronous file I/O on the event loop: Extended reasoning...Concrete cost: a prompt that returns Verification: nit — the factual claim checks out. In |
||
| else: # pragma: no cover | ||
| content = pydantic_core.to_json(msg, fallback=str, indent=2).decode() | ||
| messages.append(Message(role="user", content=content)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,25 @@ def _is_input_required_type(obj: Any) -> bool: | |
| 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 @@ def func_metadata( | |
| - 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) | ||
|
|
||
| Returns: | ||
| A FuncMetadata object containing: | ||
|
|
@@ -345,6 +367,13 @@ def func_metadata( | |
| 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( | ||
|
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: with structured_output=True, a return annotation like list[Image] or tuple[Audio, ...] makes _try_create_model_and_schema raise a raw PydanticSchemaGenerationError (wrapper model built outside the try) instead of the intended InvalidSignature; the new content-type rule only shields the structured_output=None path, and the new docstring at line 237 says such annotations "raise" without noting it is an unhandled pydantic internal error. Extended reasoning...A user who reads the new docs/docstring and passes structured_output=True to force a schema on Verification: pre-existing — the defective code (wrapper model built outside the try) predates this PR, but the diff interacts with it directly: the new guard at src/mcp/server/mcpserver/utilities/func_metadata.py:361 (
Contributor
Author
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. Agreed and pre-existing: the wrapper model is built outside the |
||
| original_annotation, return_type_expr, func.__name__ | ||
| ) | ||
|
|
@@ -546,7 +575,7 @@ def _convert_to_content(result: Any) -> list[ContentBlock]: | |
| 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: new "Content blocks and media" section says the opt-out applies "on their own or as the items of a
listortupleor the arms of a union", but_CONTENT_SEQUENCE_ORIGINSin src/mcp/server/mcpserver/utilities/func_metadata.py:38 also matchesSequence[...]annotations, which the docs omit.Extended reasoning...
Concrete cost: the published docs page is inaccurate about the rule it documents — a user annotating a tool
-> Sequence[TextContent]reads the page and expects an output schema and structured_content, but auto-detection silently opts the tool out; one word ("orSequence") in the sentence fixes it.Verification: nit — the claim is factually accurate. docs/servers/structured-output.md:213 (new "Content blocks and media" section) says the opt-out applies to content types "on their own or as the items of a
listortupleor the arms of a union", but src/mcp/server/mcpserver/utilities/func_metadata.py:38 defines_CONTENT_SEQUENCE_ORIGINS = (list, tuple, Sequence)and line 50 (`if is_union_origin(orig