-
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
6015f97
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,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. | ||
| """ | ||
|
Check warning on line 30 in src/mcp/server/mcpserver/prompts/base.py
|
||
|
Comment on lines
+26
to
+30
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] Newly public Message/UserMessage/AssistantMessage docstrings omit a Raises: section even though init now performs file I/O: passing Image(path=...)/Audio(path=...) triggers to_image_content()/to_audio_content(), which open() the file and can raise OSError/FileNotFoundError at message construction time. AGENTS.md requires: "When a public API raises exceptions a caller would reasonably catch, document them in a Raises: section." These classes were promoted to public API in this diff (added to src/mcp/server/mcpserver/init.py all), and the Image/Audio acceptance is also new in this diff, so the catchable-exception surface is new. Extended reasoning...Concrete cost (convention, /home/claude/python-sdk/AGENTS.md "Code Quality" rule on Raises: sections): a user building prompt messages with UserMessage(Image(path=...)) has no documented signal that construction reads the file and can raise OSError for a missing/unreadable path — they discover it as an unhandled exception in production instead of a documented, catchable error, and nothing in the class docstring distinguishes this from the pure in-memory data= form. One-line Raises: OSError note on the Message docstring (which UserMessage/AssistantMessage inherit contextually) fixes it. Verification: nit — the claim is factually accurate on all three legs. (1) The classes were promoted to public API in this diff: src/mcp/server/mcpserver/init.py now imports |
||
|
|
||
| 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,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")] | ||
| ) | ||
|
Check warning on line 67 in src/mcp/server/mcpserver/prompts/base.py
|
||
|
Comment on lines
+63
to
+67
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] The left_to_right union + explanatory comment is a bandaid on the real design issue: UserMessage/AssistantMessage keep role: Literal["user", "assistant"] instead of narrowing to their own role, so the AssistantMessage arm of message_validator is permanently dead and a dict {"role": "assistant", ...} materializes as a UserMessage instance whose role field is "assistant". Extended reasoning...Concrete cost: now that Message/UserMessage/AssistantMessage are exported as public API from mcp.server.mcpserver (this PR's item 3), users will naturally write isinstance(msg, AssistantMessage) dispatch over Prompt.render() results or over prompt middleware — and it silently returns False for every assistant message that was supplied in dict form, because message_validator's first arm (UserMessage) accepts role="assistant". The validator also carries a dead second arm plus a three-line comment explaining why it is dead. Narrowing each subclass to Literal["user"] / Literal["assistant"] (or using a discriminated union on role) would make the union self-describing, classify dict messages as the right class, and remove the need for the union_mode override and comment entirely. Wire output is unaffected (role serializes from the field value), so this is nit-level maintainability/API-shape cost, not a correctness bug. Verification: nit — the claim is factually true. src/mcp/server/mcpserver/prompts/base.py:48 and :57 keep |
||
|
|
||
| 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 +103,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 +119,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 +192,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 | ||
| content = pydantic_core.to_json(msg, fallback=str, indent=2).decode() | ||
| messages.append(Message(role="user", content=content)) | ||
|
|
||
| return messages | ||
| except MCPError: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.