mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-23 05:54:19 +02:00
Preserve order in mixed text/image tool returns (#1704)
Co-authored-by: William Easton <strawgate@users.noreply.github.com> Co-authored-by: marvin-context-protocol[bot] <225465937+marvin-context-protocol[bot]@users.noreply.github.com>
This commit is contained in:
parent
66da6d2f6a
commit
d7a74419b2
5 changed files with 183 additions and 74 deletions
|
|
@ -502,26 +502,59 @@ def _convert_to_content(
|
|||
# if the result is a list, then it could either be a list of MCP types,
|
||||
# or a "regular" list that the tool is returning, or a mix of both.
|
||||
#
|
||||
# so we extract all the MCP types / images and convert them as individual content elements,
|
||||
# and aggregate the rest as a single content element
|
||||
# Group adjacent non-MCP types together while preserving order
|
||||
|
||||
mcp_types = []
|
||||
other_content = []
|
||||
content_items = []
|
||||
non_mcp_batch = []
|
||||
|
||||
def flush_non_mcp_batch():
|
||||
"""Convert accumulated non-MCP items to a single TextContent block."""
|
||||
if non_mcp_batch:
|
||||
if len(non_mcp_batch) == 1:
|
||||
# Single item - convert directly to avoid combining when not needed
|
||||
content_items.extend(
|
||||
_convert_to_content(
|
||||
non_mcp_batch[0],
|
||||
serializer=serializer,
|
||||
_process_as_single_item=True,
|
||||
)
|
||||
)
|
||||
else:
|
||||
# Multiple items - combine into a single text block
|
||||
combined_text = ""
|
||||
for item in non_mcp_batch:
|
||||
if isinstance(item, str):
|
||||
combined_text += item
|
||||
else:
|
||||
if serializer is None:
|
||||
combined_text += default_serializer(item)
|
||||
else:
|
||||
try:
|
||||
combined_text += serializer(item)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"Error serializing tool result: %s",
|
||||
e,
|
||||
exc_info=True,
|
||||
)
|
||||
combined_text += default_serializer(item)
|
||||
content_items.append(TextContent(type="text", text=combined_text))
|
||||
non_mcp_batch.clear()
|
||||
|
||||
for item in result:
|
||||
if isinstance(item, ContentBlock | Image | Audio | File):
|
||||
mcp_types.append(_convert_to_content(item)[0])
|
||||
# Flush any accumulated non-MCP items first
|
||||
flush_non_mcp_batch()
|
||||
# Add the MCP item
|
||||
content_items.extend(_convert_to_content(item))
|
||||
else:
|
||||
other_content.append(item)
|
||||
# Accumulate non-MCP items
|
||||
non_mcp_batch.append(item)
|
||||
|
||||
if other_content:
|
||||
other_content = _convert_to_content(
|
||||
other_content,
|
||||
serializer=serializer,
|
||||
_process_as_single_item=True,
|
||||
)
|
||||
# Flush any remaining non-MCP items
|
||||
flush_non_mcp_batch()
|
||||
|
||||
return other_content + mcp_types
|
||||
return content_items
|
||||
|
||||
if not isinstance(result, str):
|
||||
if serializer is None:
|
||||
|
|
|
|||
|
|
@ -185,7 +185,9 @@ class TestTools:
|
|||
async def test_tool_returns_list(self, tool_server: FastMCP):
|
||||
async with Client(tool_server) as client:
|
||||
result = await client.call_tool("list_tool", {})
|
||||
assert result.content[0].text == '["x",2]' # type: ignore[attr-defined]
|
||||
# Adjacent non-MCP list items are combined into single content block
|
||||
assert len(result.content) == 1
|
||||
assert result.content[0].text == "x2" # type: ignore[attr-defined]
|
||||
assert result.data == ["x", 2]
|
||||
|
||||
async def test_file_text_tool(self, tool_server: FastMCP):
|
||||
|
|
@ -426,7 +428,7 @@ class TestToolReturnTypes:
|
|||
self, tool_server: FastMCP, tmp_path: Path
|
||||
):
|
||||
"""Test that lists containing Image objects and other types are handled
|
||||
correctly. Note that the non-MCP content will be grouped together."""
|
||||
correctly. Items now preserve their original order."""
|
||||
# Create a test image
|
||||
image_path = tmp_path / "test.png"
|
||||
image_path.write_bytes(b"test image data")
|
||||
|
|
@ -435,26 +437,30 @@ class TestToolReturnTypes:
|
|||
result = await client.call_tool(
|
||||
"mixed_list_fn", {"image_path": str(image_path)}
|
||||
)
|
||||
assert len(result.content) == 3
|
||||
# Check text conversion
|
||||
assert len(result.content) == 4 # Now each item is separate
|
||||
# Check text message (first item)
|
||||
content1 = result.content[0]
|
||||
assert isinstance(content1, TextContent)
|
||||
assert json.loads(content1.text) == ["text message", {"key": "value"}]
|
||||
# Check image conversion
|
||||
assert content1.text == "text message"
|
||||
# Check image conversion (second item)
|
||||
content2 = result.content[1]
|
||||
assert isinstance(content2, ImageContent)
|
||||
assert content2.mimeType == "image/png"
|
||||
assert base64.b64decode(content2.data) == b"test image data"
|
||||
# Check direct TextContent
|
||||
# Check dict content (third item)
|
||||
content3 = result.content[2]
|
||||
assert isinstance(content3, TextContent)
|
||||
assert content3.text == "direct content"
|
||||
assert json.loads(content3.text) == {"key": "value"}
|
||||
# Check direct TextContent (fourth item)
|
||||
content4 = result.content[3]
|
||||
assert isinstance(content4, TextContent)
|
||||
assert content4.text == "direct content"
|
||||
|
||||
async def test_tool_mixed_list_with_audio(
|
||||
self, tool_server: FastMCP, tmp_path: Path
|
||||
):
|
||||
"""Test that lists containing Audio objects and other types are handled
|
||||
correctly. Note that the non-MCP content will be grouped together."""
|
||||
correctly. Items now preserve their original order."""
|
||||
# Create a test audio file
|
||||
audio_path = tmp_path / "test.wav"
|
||||
audio_path.write_bytes(b"test audio data")
|
||||
|
|
@ -463,26 +469,30 @@ class TestToolReturnTypes:
|
|||
result = await client.call_tool(
|
||||
"mixed_audio_list_fn", {"audio_path": str(audio_path)}
|
||||
)
|
||||
assert len(result.content) == 3
|
||||
# Check text conversion
|
||||
assert len(result.content) == 4 # Now each item is separate
|
||||
# Check text message (first item)
|
||||
content1 = result.content[0]
|
||||
assert isinstance(content1, TextContent)
|
||||
assert json.loads(content1.text) == ["text message", {"key": "value"}]
|
||||
# Check audio conversion
|
||||
assert content1.text == "text message"
|
||||
# Check audio conversion (second item)
|
||||
content2 = result.content[1]
|
||||
assert isinstance(content2, AudioContent)
|
||||
assert content2.mimeType == "audio/wav"
|
||||
assert base64.b64decode(content2.data) == b"test audio data"
|
||||
# Check direct TextContent
|
||||
# Check dict content (third item)
|
||||
content3 = result.content[2]
|
||||
assert isinstance(content3, TextContent)
|
||||
assert content3.text == "direct content"
|
||||
assert json.loads(content3.text) == {"key": "value"}
|
||||
# Check direct TextContent (fourth item)
|
||||
content4 = result.content[3]
|
||||
assert isinstance(content4, TextContent)
|
||||
assert content4.text == "direct content"
|
||||
|
||||
async def test_tool_mixed_list_with_file(
|
||||
self, tool_server: FastMCP, tmp_path: Path
|
||||
):
|
||||
"""Test that lists containing File objects and other types are handled
|
||||
correctly. Note that the non-MCP content will be grouped together."""
|
||||
correctly. Items now preserve their original order."""
|
||||
# Create a test file
|
||||
file_path = tmp_path / "test.bin"
|
||||
file_path.write_bytes(b"test file data")
|
||||
|
|
@ -491,12 +501,12 @@ class TestToolReturnTypes:
|
|||
result = await client.call_tool(
|
||||
"mixed_file_list_fn", {"file_path": str(file_path)}
|
||||
)
|
||||
assert len(result.content) == 3
|
||||
# Check text conversion
|
||||
assert len(result.content) == 4 # Now each item is separate
|
||||
# Check text message (first item)
|
||||
content1 = result.content[0]
|
||||
assert isinstance(content1, TextContent)
|
||||
assert json.loads(content1.text) == ["text message", {"key": "value"}]
|
||||
# Check file conversion
|
||||
assert content1.text == "text message"
|
||||
# Check file conversion (second item)
|
||||
content2 = result.content[1]
|
||||
assert isinstance(content2, EmbeddedResource)
|
||||
assert content2.type == "resource"
|
||||
|
|
@ -505,10 +515,14 @@ class TestToolReturnTypes:
|
|||
assert hasattr(resource, "blob")
|
||||
blob_data = getattr(resource, "blob")
|
||||
assert base64.b64decode(blob_data) == b"test file data"
|
||||
# Check direct TextContent
|
||||
# Check dict content (third item)
|
||||
content3 = result.content[2]
|
||||
assert isinstance(content3, TextContent)
|
||||
assert content3.text == "direct content"
|
||||
assert json.loads(content3.text) == {"key": "value"}
|
||||
# Check direct TextContent (fourth item)
|
||||
content4 = result.content[3]
|
||||
assert isinstance(content4, TextContent)
|
||||
assert content4.text == "direct content"
|
||||
|
||||
|
||||
class TestToolParameters:
|
||||
|
|
|
|||
|
|
@ -24,8 +24,9 @@ async def test_complex_inputs():
|
|||
result = await client.call_tool_mcp(
|
||||
"name_shrimp", {"tank": tank, "extra_names": ["charlie"]}
|
||||
)
|
||||
# Adjacent non-MCP list items are combined into single content
|
||||
assert len(result.content) == 1
|
||||
assert result.content[0].text == '["bob","alice","charlie"]' # type: ignore[attr-defined]
|
||||
assert result.content[0].text == "bobalicecharlie" # type: ignore[attr-defined]
|
||||
|
||||
|
||||
async def test_desktop(monkeypatch):
|
||||
|
|
|
|||
|
|
@ -1056,12 +1056,12 @@ class TestConvertResultToContent:
|
|||
assert result[0].text == '{"a":1,"b":2}'
|
||||
|
||||
def test_list_of_basic_types(self):
|
||||
"""Test that a list of basic types is converted to a single TextContent."""
|
||||
"""Test that a list of basic types is converted to a single combined TextContent item."""
|
||||
result = _convert_to_content([1, "two", {"c": 3}])
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 1
|
||||
assert len(result) == 1 # Adjacent non-MCP types are combined
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == '[1,"two",{"c":3}]'
|
||||
assert result[0].text == '1two{"c":3}' # All adjacent basic types combined
|
||||
|
||||
def test_list_of_mcp_types(self):
|
||||
"""Test that a list of MCP types is returned as a list of those types."""
|
||||
|
|
@ -1091,11 +1091,60 @@ class TestConvertResultToContent:
|
|||
assert text_content_count == 2
|
||||
assert image_content_count == 1
|
||||
|
||||
text_item = next(item for item in result if isinstance(item, TextContent))
|
||||
assert text_item.text == '[{"a":1}]'
|
||||
# Verify order is preserved: text, image, text
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "hello"
|
||||
assert isinstance(result[1], ImageContent)
|
||||
assert result[1].data == "ZmFrZWltYWdlZGF0YQ=="
|
||||
assert isinstance(result[2], TextContent)
|
||||
assert result[2].text == '{"a":1}'
|
||||
|
||||
image_item = next(item for item in result if isinstance(item, ImageContent))
|
||||
assert image_item.data == "ZmFrZWltYWdlZGF0YQ=="
|
||||
def test_mixed_text_and_images_preserve_order(self):
|
||||
"""Test that mixed text and images preserve their original order (GitHub issue #1656)."""
|
||||
img_obj1 = Image(data=b"imagedata1")
|
||||
img_obj2 = Image(data=b"imagedata2")
|
||||
|
||||
# Test the exact pattern from the issue: [text1, img1, text2, img2]
|
||||
result = _convert_to_content(["text1", img_obj1, "text2", img_obj2])
|
||||
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 4
|
||||
|
||||
# Verify exact order is preserved
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "text1"
|
||||
|
||||
assert isinstance(result[1], ImageContent)
|
||||
assert result[1].data == "aW1hZ2VkYXRhMQ==" # base64 of "imagedata1"
|
||||
|
||||
assert isinstance(result[2], TextContent)
|
||||
assert result[2].text == "text2"
|
||||
|
||||
assert isinstance(result[3], ImageContent)
|
||||
assert result[3].data == "aW1hZ2VkYXRhMg==" # base64 of "imagedata2"
|
||||
|
||||
def test_adjacent_non_mcp_types_combined(self):
|
||||
"""Test strawgate's example: image, 'x', 'y', image should be 2 image blocks and 1 content block."""
|
||||
img_obj1 = Image(data=b"imagedata1")
|
||||
img_obj2 = Image(data=b"imagedata2")
|
||||
|
||||
# Test the exact pattern from strawgate's request: [image, 'x', 'y', image]
|
||||
result = _convert_to_content([img_obj1, "x", "y", img_obj2])
|
||||
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 3 # 2 image blocks + 1 combined text block
|
||||
|
||||
# First image
|
||||
assert isinstance(result[0], ImageContent)
|
||||
assert result[0].data == "aW1hZ2VkYXRhMQ==" # base64 of "imagedata1"
|
||||
|
||||
# Combined text content for adjacent 'x' and 'y'
|
||||
assert isinstance(result[1], TextContent)
|
||||
assert result[1].text == "xy" # Adjacent strings combined
|
||||
|
||||
# Second image
|
||||
assert isinstance(result[2], ImageContent)
|
||||
assert result[2].data == "aW1hZ2VkYXRhMg==" # base64 of "imagedata2"
|
||||
|
||||
def test_list_of_mixed_types_list(self):
|
||||
"""Test that a list of mixed types, including a list as one of the elements, is converted correctly."""
|
||||
|
|
@ -1105,19 +1154,23 @@ class TestConvertResultToContent:
|
|||
result = _convert_to_content([content1, image_obj, basic_data])
|
||||
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 3
|
||||
assert (
|
||||
len(result) == 3
|
||||
) # Adjacent non-MCP types are combined: hello (TextContent) + image + basic_data (single TextContent)
|
||||
|
||||
text_content_count = sum(isinstance(item, TextContent) for item in result)
|
||||
image_content_count = sum(isinstance(item, ImageContent) for item in result)
|
||||
|
||||
assert text_content_count == 2
|
||||
assert text_content_count == 2 # hello + serialized basic_data
|
||||
assert image_content_count == 1
|
||||
|
||||
text_item = next(item for item in result if isinstance(item, TextContent))
|
||||
assert text_item.text == '[[{"a":1},{"b":2}]]'
|
||||
|
||||
image_item = next(item for item in result if isinstance(item, ImageContent))
|
||||
assert image_item.data == "ZmFrZWltYWdlZGF0YQ=="
|
||||
# Verify order: hello, image, serialized basic_data
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "hello"
|
||||
assert isinstance(result[1], ImageContent)
|
||||
assert result[1].data == "ZmFrZWltYWdlZGF0YQ=="
|
||||
assert isinstance(result[2], TextContent)
|
||||
assert result[2].text == '[{"a":1},{"b":2}]' # basic_data serialized as JSON
|
||||
|
||||
def test_list_of_mixed_types_with_audio(self):
|
||||
"""Test that a list of mixed types including Audio is converted correctly."""
|
||||
|
|
@ -1135,11 +1188,13 @@ class TestConvertResultToContent:
|
|||
assert text_content_count == 2
|
||||
assert audio_content_count == 1
|
||||
|
||||
text_item = next(item for item in result if isinstance(item, TextContent))
|
||||
assert text_item.text == '[{"a":1}]'
|
||||
|
||||
audio_item = next(item for item in result if isinstance(item, AudioContent))
|
||||
assert audio_item.data == "ZmFrZWF1ZGlvZGF0YQ=="
|
||||
# Verify order is preserved: text, audio, text
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "hello"
|
||||
assert isinstance(result[1], AudioContent)
|
||||
assert result[1].data == "ZmFrZWF1ZGlvZGF0YQ=="
|
||||
assert isinstance(result[2], TextContent)
|
||||
assert result[2].text == '{"a":1}'
|
||||
|
||||
def test_list_of_mixed_types_with_file(self):
|
||||
"""Test that a list of mixed types including File is converted correctly."""
|
||||
|
|
@ -1160,14 +1215,15 @@ class TestConvertResultToContent:
|
|||
assert text_content_count == 2
|
||||
assert embedded_content_count == 1
|
||||
|
||||
text_item = next(item for item in result if isinstance(item, TextContent))
|
||||
assert text_item.text == '[{"a":1}]'
|
||||
# Verify order is preserved: text, file, text
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "hello"
|
||||
assert isinstance(result[1], EmbeddedResource)
|
||||
assert result[1].type == "resource"
|
||||
assert isinstance(result[2], TextContent)
|
||||
assert result[2].text == '{"a":1}'
|
||||
|
||||
embedded_item = next(
|
||||
item
|
||||
for item in result
|
||||
if isinstance(item, EmbeddedResource) and item.type == "resource"
|
||||
)
|
||||
embedded_item = result[1]
|
||||
resource = embedded_item.resource
|
||||
assert resource.mimeType == "application/octet-stream"
|
||||
# Check for blob attribute and its value
|
||||
|
|
@ -1239,28 +1295,28 @@ class TestConvertResultToContent:
|
|||
]
|
||||
|
||||
def test_single_element_list_preserves_structure(self):
|
||||
"""Test that single-element lists preserve their list structure."""
|
||||
"""Test that single-element lists are converted to individual TextContent items."""
|
||||
|
||||
# Test with a single integer
|
||||
# Test with a single integer - now returns the integer as individual content
|
||||
result = _convert_to_content([1])
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 1
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == "[1]" # Should be "[1]", not "1"
|
||||
assert result[0].text == "1" # Individual item, not wrapped in list
|
||||
|
||||
# Test with a single string
|
||||
# Test with a single string - now returns the string as individual content
|
||||
result = _convert_to_content(["hello"])
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 1
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == '["hello"]' # Should be ["hello"], not "hello"
|
||||
assert result[0].text == "hello" # Individual string, not wrapped in list
|
||||
|
||||
# Test with a single dict
|
||||
# Test with a single dict - now returns the dict as individual content
|
||||
result = _convert_to_content([{"a": 1}])
|
||||
assert isinstance(result, list)
|
||||
assert len(result) == 1
|
||||
assert isinstance(result[0], TextContent)
|
||||
assert result[0].text == '[{"a":1}]' # Should be wrapped in a list
|
||||
assert result[0].text == '{"a":1}' # Individual dict, not wrapped in list
|
||||
|
||||
|
||||
class TestAutomaticStructuredContent:
|
||||
|
|
@ -1385,9 +1441,10 @@ class TestAutomaticStructuredContent:
|
|||
|
||||
result = await tool.run({})
|
||||
|
||||
# Should only have content, no structured content
|
||||
# Adjacent non-MCP types should be combined into single content block, no structured content
|
||||
assert len(result.content) == 1
|
||||
assert isinstance(result.content[0], TextContent)
|
||||
assert all(isinstance(item, TextContent) for item in result.content)
|
||||
assert result.content[0].text == "12345" # All numbers combined
|
||||
assert result.structured_content is None
|
||||
|
||||
async def test_int_return_with_schema_creates_structured_content(self):
|
||||
|
|
|
|||
|
|
@ -569,7 +569,9 @@ class TestCallTools:
|
|||
},
|
||||
)
|
||||
|
||||
assert result.content[0].text == '["rex","gertrude"]' # type: ignore[attr-defined]
|
||||
# Adjacent non-MCP list items are combined into single content block
|
||||
assert len(result.content) == 1
|
||||
assert result.content[0].text == "rexgertrude" # type: ignore[attr-defined]
|
||||
assert result.structured_content == {"result": ["rex", "gertrude"]}
|
||||
|
||||
async def test_call_tool_with_custom_serializer(self):
|
||||
|
|
@ -611,9 +613,11 @@ class TestCallTools:
|
|||
]
|
||||
|
||||
result = await manager.call_tool("get_data", {})
|
||||
# Adjacent non-MCP list items get combined with custom serializer applied to each
|
||||
assert len(result.content) == 1
|
||||
assert (
|
||||
result.content[0].text # type: ignore[attr-defined]
|
||||
== 'CUSTOM:[{"key": "value", "number": 123}, {"key": "value2", "number": 456}]' # type: ignore[attr-defined]
|
||||
== '{"key": "value", "number": 123}{"key": "value2", "number": 456}' # Adjacent items combined after individual serialization
|
||||
)
|
||||
assert result.structured_content == {
|
||||
"result": [
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue