mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-10-12 02:12:31 +00:00
fix(agents): render message content parts as text, not a python repr (#7109)
* fix(agents): render message content parts as text, not a Python repr
A message whose `content` is a multimodal parts list collapsed to
`str(content)` wherever a message had to become a string, so the model
saw `[{'type': 'text', 'text': 'hello'}]` in Current Task, and memory
stored and searched that same repr.
Four sites flattened it that way: the turn promoted into the executor
prompt, the memory recall query, what `_save_kickoff_to_memory` writes,
and `_message_content_text` (token estimation and oversized-message
splitting).
The extraction already existed, inline in `_format_messages_for_summary`
-- text blocks joined, or `[multimodal content]` when a list carries
none. This lifts it to `_content_parts_text` and routes all five callers
through it, so summary, prompt, memory and token counting agree.
`_message_content_text` becomes `message_content_text`: it now has a
caller outside its module, and `agent/core.py` imports only public names
from `agent_utils`. It is not re-exported from any `__init__`, so no
public import path changes.
`test_list_content_uses_str` pinned the repr, so it is intentionally
rewritten to pin the text. Every other existing caller is unchanged:
30 failures on main, 30 on this branch, identical names.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01VwxiogL9mQg9q8cx4qLfYJ
* fix(agents): skip a content part whose text is not a string
`_content_parts_text` joined `block["text"]` straight into a string, and
content blocks are `dict[str, Any]` arriving from a model, so a `text`
key holding an int, dict or None raised `TypeError`. That was contained
to summarization before; routing the prompt, memory and token-estimation
paths through the same helper widened it to `Agent.kickoff`, where the
old `str()` had merely produced an ugly string.
Such a block carries no usable text, so it is skipped. A list left with
nothing usable still falls back to `[multimodal content]`.
Writes the convention down in AGENTS.md rather than leaving it in a
review thread: never `str()` a message's content, use
`message_content_text`. Four sites had independently reached for
`str()`, which is what this whole change is undoing.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01VwxiogL9mQg9q8cx4qLfYJ
---------
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
7c23857aed
commit
6ad3bf9390
@@ -14,6 +14,23 @@ Follow these guidelines when contributing:
|
||||
6. Follow software principles such as DRY and YAGNI.
|
||||
7. Keep diffs as minimal as possible.
|
||||
|
||||
## Message Content
|
||||
|
||||
`LLMMessage.content` is `str | list[dict[str, Any]] | None`; the list form is
|
||||
multimodal content parts. Never `str()` it — that puts a Python repr
|
||||
(`[{'type': 'text', 'text': 'hi'}]`) in front of the model and into memory.
|
||||
Collapse a message to text with the helper instead:
|
||||
|
||||
```python
|
||||
from crewai.utilities.agent_utils import message_content_text
|
||||
|
||||
text = message_content_text(msg) # "" for None; joined text for a parts list
|
||||
```
|
||||
|
||||
Parts arrive from a model and are typed `dict[str, Any]`, so a `text` key that
|
||||
is not a string is possible. `_content_parts_text` skips those blocks rather
|
||||
than raising, and names a list with no usable text `[multimodal content]`.
|
||||
|
||||
## Changing Docs
|
||||
|
||||
1. Edit MDX under `docs/edge/en/*` and reference it from `docs/docs.json` if
|
||||
|
||||
@@ -97,6 +97,7 @@ from crewai.utilities.agent_utils import (
|
||||
get_tool_names,
|
||||
is_inside_event_loop,
|
||||
load_agent_from_repository,
|
||||
message_content_text,
|
||||
parse_tools,
|
||||
render_text_description_and_args,
|
||||
)
|
||||
@@ -1572,7 +1573,7 @@ class Agent(BaseAgent):
|
||||
# assistant's own replies were the user's.
|
||||
request_index = _request_index(carried)
|
||||
request = carried[request_index] if carried else None
|
||||
formatted_messages = str(request.get("content") or "") if request else ""
|
||||
formatted_messages = message_content_text(request) if request else ""
|
||||
# Split, rather than one history list: the promoted request keeps
|
||||
# its position in the conversation. Sending everything before it
|
||||
# would hoist a trailing tool pair above the question it answers,
|
||||
@@ -1581,7 +1582,7 @@ class Agent(BaseAgent):
|
||||
history = carried[:request_index]
|
||||
trailing = carried[request_index + 1 :]
|
||||
recall_text = "\n".join(
|
||||
str(msg.get("content", "")) for msg in carried if msg.get("content")
|
||||
message_content_text(msg) for msg in carried if msg.get("content")
|
||||
)
|
||||
# Only the request's attachments go on the current turn; a history
|
||||
# message keeps its own, so unioning them all would send prior
|
||||
@@ -1804,7 +1805,7 @@ class Agent(BaseAgent):
|
||||
else:
|
||||
input_str = (
|
||||
"\n".join(
|
||||
str(msg.get("content", ""))
|
||||
message_content_text(msg)
|
||||
for msg in messages
|
||||
if msg.get("content")
|
||||
)
|
||||
|
||||
@@ -844,11 +844,40 @@ def _estimate_token_count(text: str) -> int:
|
||||
return len(text) // 4
|
||||
|
||||
|
||||
def _message_content_text(msg: LLMMessage) -> str:
|
||||
"""Return the message content as text for token estimation."""
|
||||
def _content_parts_text(content: list[dict[str, Any]]) -> str:
|
||||
"""Text carried by a multimodal content-part list.
|
||||
|
||||
Non-text blocks (images, audio) have no text to give, so a list with
|
||||
none of them is named rather than rendered -- ``str()`` on the list
|
||||
would put a Python repr in front of the model.
|
||||
|
||||
Blocks are ``dict[str, Any]`` and arrive from a model, so a ``text``
|
||||
key that is not a string is possible; such a block carries no usable
|
||||
text and is skipped rather than joined, which would raise.
|
||||
"""
|
||||
text_parts = [
|
||||
block["text"]
|
||||
for block in content
|
||||
if isinstance(block, dict)
|
||||
and block.get("type") == "text"
|
||||
and isinstance(block.get("text"), str)
|
||||
]
|
||||
return " ".join(text_parts) if text_parts else "[multimodal content]"
|
||||
|
||||
|
||||
def message_content_text(msg: LLMMessage) -> str:
|
||||
"""Return the message content as text.
|
||||
|
||||
Used wherever a message has to collapse to a string -- token
|
||||
estimation, memory, and the one turn promoted into the executor
|
||||
prompt. Content is ``str | list[dict] | None``, and the list form is
|
||||
the multimodal one.
|
||||
"""
|
||||
content = msg.get("content")
|
||||
if content is None:
|
||||
return ""
|
||||
if isinstance(content, list):
|
||||
return _content_parts_text(content)
|
||||
return str(content)
|
||||
|
||||
|
||||
@@ -866,7 +895,7 @@ def _split_text_by_token_limit(text: str, max_tokens: int) -> list[str]:
|
||||
|
||||
def _expand_oversized_message(msg: LLMMessage, max_tokens: int) -> list[LLMMessage]:
|
||||
"""Split a message whose content alone exceeds max_tokens into sub-messages."""
|
||||
msg_text = _message_content_text(msg)
|
||||
msg_text = message_content_text(msg)
|
||||
if _estimate_token_count(msg_text) <= max_tokens:
|
||||
return [msg]
|
||||
|
||||
@@ -932,12 +961,7 @@ def _format_messages_for_summary(messages: list[LLMMessage]) -> str:
|
||||
else:
|
||||
content = ""
|
||||
elif isinstance(content, list):
|
||||
text_parts = [
|
||||
block.get("text", "")
|
||||
for block in content
|
||||
if isinstance(block, dict) and block.get("type") == "text"
|
||||
]
|
||||
content = " ".join(text_parts) if text_parts else "[multimodal content]"
|
||||
content = _content_parts_text(content)
|
||||
|
||||
if role == "assistant":
|
||||
label = "[ASSISTANT]:"
|
||||
@@ -976,7 +1000,7 @@ def _split_messages_into_chunks(
|
||||
current_tokens = 0
|
||||
|
||||
for msg in normalized:
|
||||
msg_tokens = _estimate_token_count(_message_content_text(msg))
|
||||
msg_tokens = _estimate_token_count(message_content_text(msg))
|
||||
|
||||
if current_chunk and (current_tokens + msg_tokens) > max_tokens:
|
||||
chunks.append(current_chunk)
|
||||
|
||||
@@ -0,0 +1,132 @@
|
||||
"""A multimodal content-part list must reach the model as its text."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
from crewai import Agent
|
||||
from crewai.llms.base_llm import BaseLLM
|
||||
|
||||
|
||||
class _Recorder(BaseLLM):
|
||||
"""Records the exact message list handed to the provider."""
|
||||
|
||||
def __init__(self) -> None:
|
||||
super().__init__(model="recorder")
|
||||
object.__setattr__(self, "seen", [])
|
||||
|
||||
def call(self, messages: Any, **kwargs: Any) -> str:
|
||||
self.seen.append(list(messages) if isinstance(messages, list) else messages)
|
||||
return "ok"
|
||||
|
||||
async def acall(self, messages: Any, **kwargs: Any) -> str:
|
||||
return self.call(messages, **kwargs)
|
||||
|
||||
def supports_function_calling(self) -> bool:
|
||||
return False
|
||||
|
||||
def supports_stop_words(self) -> bool:
|
||||
return False
|
||||
|
||||
def get_context_window_size(self) -> int:
|
||||
return 8192
|
||||
|
||||
|
||||
PARTS = [
|
||||
{"type": "text", "text": "what is in this photo"},
|
||||
{"type": "image_url", "image_url": {"url": "https://example/p.png"}},
|
||||
]
|
||||
|
||||
IMAGE_ONLY = [{"type": "image_url", "image_url": {"url": "https://example/p.png"}}]
|
||||
|
||||
|
||||
def _agent(llm: _Recorder, **kwargs: Any) -> Agent:
|
||||
return Agent(role="Support", goal="help", backstory="b", llm=llm, **kwargs)
|
||||
|
||||
|
||||
def _request_text(seen: list[dict[str, Any]]) -> str:
|
||||
return str([m for m in seen if m.get("role") == "user"][-1]["content"])
|
||||
|
||||
|
||||
def test_the_promoted_request_carries_the_text_not_a_repr() -> None:
|
||||
"""`str()` on the parts list put `[{'type': 'text', ...}]` in Current Task."""
|
||||
llm = _Recorder()
|
||||
_agent(llm).kickoff([{"role": "user", "content": PARTS}])
|
||||
|
||||
text = _request_text(llm.seen[0])
|
||||
|
||||
assert "what is in this photo" in text
|
||||
assert "'type'" not in text
|
||||
assert "image_url" not in text
|
||||
|
||||
|
||||
def test_the_async_path_agrees_with_the_sync_one() -> None:
|
||||
"""`kickoff_async` shares `_prepare_kickoff`; assert it, don't assume it.
|
||||
|
||||
Driven with ``asyncio.run`` rather than an async test: ``kickoff`` takes a
|
||||
different path when called from inside a running loop, so the sync half
|
||||
has to run outside one for the comparison to mean anything.
|
||||
"""
|
||||
sync_llm = _Recorder()
|
||||
_agent(sync_llm).kickoff([{"role": "user", "content": PARTS}])
|
||||
|
||||
async_llm = _Recorder()
|
||||
asyncio.run(_agent(async_llm).kickoff_async([{"role": "user", "content": PARTS}]))
|
||||
|
||||
assert _request_text(async_llm.seen[0]) == _request_text(sync_llm.seen[0])
|
||||
assert "what is in this photo" in _request_text(async_llm.seen[0])
|
||||
|
||||
|
||||
def test_an_image_only_request_is_named_not_serialized() -> None:
|
||||
"""No text block to promote, so the placeholder stands in for the repr."""
|
||||
llm = _Recorder()
|
||||
_agent(llm).kickoff([{"role": "user", "content": IMAGE_ONLY}])
|
||||
|
||||
text = _request_text(llm.seen[0])
|
||||
|
||||
assert "[multimodal content]" in text
|
||||
assert "example/p.png" not in text
|
||||
|
||||
|
||||
def test_history_keeps_its_parts_list_untouched() -> None:
|
||||
"""Only the promoted turn collapses to text; the provider still gets parts."""
|
||||
llm = _Recorder()
|
||||
_agent(llm).kickoff(
|
||||
[
|
||||
{"role": "user", "content": PARTS},
|
||||
{"role": "assistant", "content": "a cat"},
|
||||
{"role": "user", "content": "and this one?"},
|
||||
]
|
||||
)
|
||||
history = [m for m in llm.seen[0] if m.get("role") == "user"][0]
|
||||
|
||||
assert history["content"] == PARTS
|
||||
|
||||
|
||||
def test_a_plain_string_request_is_unchanged() -> None:
|
||||
llm = _Recorder()
|
||||
_agent(llm).kickoff([{"role": "user", "content": "just text"}])
|
||||
|
||||
assert "just text" in _request_text(llm.seen[0])
|
||||
|
||||
|
||||
def test_memory_recall_and_save_get_text_not_a_repr() -> None:
|
||||
"""Both memory paths flattened the same way and stored the repr."""
|
||||
llm = _Recorder()
|
||||
memory = MagicMock()
|
||||
memory.recall.return_value = []
|
||||
memory.extract_memories.return_value = ["m"]
|
||||
|
||||
agent = _agent(llm)
|
||||
object.__setattr__(agent, "memory", memory)
|
||||
agent.kickoff([{"role": "user", "content": PARTS}])
|
||||
|
||||
recalled = memory.recall.call_args[0][0]
|
||||
assert "what is in this photo" in recalled
|
||||
assert "'type'" not in recalled
|
||||
|
||||
saved = memory.extract_memories.call_args[0][0]
|
||||
assert "what is in this photo" in saved
|
||||
assert "'type'" not in saved
|
||||
@@ -24,7 +24,7 @@ from crewai.utilities.agent_utils import (
|
||||
_expand_oversized_message,
|
||||
_extract_summary_tags,
|
||||
_format_messages_for_summary,
|
||||
_message_content_text,
|
||||
message_content_text,
|
||||
_normalize_messages_for_chunking,
|
||||
_split_messages_into_chunks,
|
||||
_split_text_by_token_limit,
|
||||
@@ -708,7 +708,7 @@ class TestSplitMessagesIntoChunks:
|
||||
assert len(chunks) > 1
|
||||
for chunk in chunks:
|
||||
chunk_tokens = sum(
|
||||
_estimate_token_count(_message_content_text(msg)) for msg in chunk
|
||||
_estimate_token_count(message_content_text(msg)) for msg in chunk
|
||||
)
|
||||
assert chunk_tokens <= max_tokens
|
||||
|
||||
@@ -723,7 +723,7 @@ class TestSplitMessagesIntoChunks:
|
||||
assert len(chunks) > 1
|
||||
for chunk in chunks:
|
||||
chunk_tokens = sum(
|
||||
_estimate_token_count(_message_content_text(msg)) for msg in chunk
|
||||
_estimate_token_count(message_content_text(msg)) for msg in chunk
|
||||
)
|
||||
assert chunk_tokens <= max_tokens
|
||||
|
||||
@@ -750,22 +750,57 @@ class TestSplitMessagesIntoChunks:
|
||||
|
||||
|
||||
class TestMessageContentText:
|
||||
"""Tests for _message_content_text helper."""
|
||||
"""Tests for message_content_text helper."""
|
||||
|
||||
def test_string_content(self) -> None:
|
||||
msg: dict[str, Any] = {"role": "user", "content": "hello"}
|
||||
assert _message_content_text(msg) == "hello"
|
||||
assert message_content_text(msg) == "hello"
|
||||
|
||||
def test_none_content(self) -> None:
|
||||
msg: dict[str, Any] = {"role": "assistant", "content": None}
|
||||
assert _message_content_text(msg) == ""
|
||||
assert message_content_text(msg) == ""
|
||||
|
||||
def test_list_content_uses_str(self) -> None:
|
||||
def test_list_content_yields_its_text(self) -> None:
|
||||
"""A parts list used to collapse to its Python repr."""
|
||||
msg: dict[str, Any] = {
|
||||
"role": "user",
|
||||
"content": [{"type": "text", "text": "first"}],
|
||||
"content": [{"type": "text", "text": "first"}, {"type": "text", "text": "second"}],
|
||||
}
|
||||
assert _message_content_text(msg) == str(msg["content"])
|
||||
assert message_content_text(msg) == "first second"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad_text", [123, None, {"nested": "x"}, ["a"]], ids=str
|
||||
)
|
||||
def test_a_non_string_text_block_does_not_raise(self, bad_text: Any) -> None:
|
||||
"""Blocks are `dict[str, Any]` from a model, so `text` may be anything."""
|
||||
msg: dict[str, Any] = {
|
||||
"role": "user",
|
||||
"content": [{"type": "text", "text": bad_text}],
|
||||
}
|
||||
|
||||
assert message_content_text(msg) == "[multimodal content]"
|
||||
|
||||
def test_a_usable_text_block_survives_a_malformed_sibling(self) -> None:
|
||||
msg: dict[str, Any] = {
|
||||
"role": "user",
|
||||
"content": [
|
||||
{"type": "text", "text": {"nested": "x"}},
|
||||
{"type": "text", "text": "real text"},
|
||||
],
|
||||
}
|
||||
|
||||
assert message_content_text(msg) == "real text"
|
||||
|
||||
def test_list_content_without_text_is_named_not_repr(self) -> None:
|
||||
msg: dict[str, Any] = {
|
||||
"role": "user",
|
||||
"content": [{"type": "image_url", "image_url": {"url": "https://x/y.png"}}],
|
||||
}
|
||||
|
||||
text = message_content_text(msg)
|
||||
|
||||
assert text == "[multimodal content]"
|
||||
assert "image_url" not in text
|
||||
|
||||
|
||||
class TestSplitTextByTokenLimit:
|
||||
@@ -830,7 +865,7 @@ class TestExpandOversizedMessage:
|
||||
expanded = _expand_oversized_message(msg, max_tokens=max_tokens)
|
||||
assert len(expanded) > 1
|
||||
assert all(
|
||||
_estimate_token_count(_message_content_text(part)) <= max_tokens
|
||||
_estimate_token_count(message_content_text(part)) <= max_tokens
|
||||
for part in expanded
|
||||
)
|
||||
|
||||
@@ -859,7 +894,7 @@ class TestNormalizeMessagesForChunking:
|
||||
assert normalized[-1]["content"] == "Done"
|
||||
assert len(normalized) > 3
|
||||
assert all(
|
||||
_estimate_token_count(_message_content_text(msg)) <= max_tokens
|
||||
_estimate_token_count(message_content_text(msg)) <= max_tokens
|
||||
for msg in normalized
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user