fix: normalize OpenAI tool message content - #701
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNormalize tool-message ChangesOpenAI Tool Message Content Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/test_multipart_converter_openai.py (1)
363-394: 💤 Low valueConsider expanding test coverage for additional edge cases.
The current tests validate the primary normalization behaviors well. To increase robustness and coverage of the normalization logic, consider adding tests for:
- Tool message with multiple text blocks: Validates that multiple text blocks are concatenated correctly during normalization.
- Tool message with non-list, non-string content (e.g., a dict or object): Validates the JSON serialization fallback path in
_normalize_tool_message_content(lines 376-378 in context snippet 1).These additions would provide more comprehensive validation of the normalization contract.
🧪 Example test cases for additional coverage
Test for multiple text blocks:
def test_convert_mixed_messages_normalizes_multiple_tool_text_blocks(self): tool_message = { "role": "tool", "tool_call_id": "call_456", "content": [ {"type": "text", "text": "First block"}, {"type": "text", "text": "Second block"}, ], } messages = OpenAIConverter.convert_mixed_messages_to_openai([tool_message]) assert len(messages) == 1 assert messages[0]["role"] == "tool" assert messages[0]["content"] == "First block Second block" assert isinstance(messages[0]["content"], str)Test for non-list content:
def test_convert_mixed_messages_normalizes_tool_dict_content(self): tool_message = { "role": "tool", "tool_call_id": "call_789", "content": {"key": "value", "nested": {"data": 123}}, } messages = OpenAIConverter.convert_mixed_messages_to_openai([tool_message]) assert len(messages) == 1 assert messages[0]["role"] == "tool" assert isinstance(messages[0]["content"], str) # Content should be JSON-serialized import json assert json.loads(messages[0]["content"]) == {"key": "value", "nested": {"data": 123}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_multipart_converter_openai.py` around lines 363 - 394, Add two unit tests for OpenAIConverter.convert_mixed_messages_to_openai: one that supplies a tool message with multiple text blocks to verify _normalize_tool_message_content concatenates them into a single string (e.g., "First block Second block") and another that supplies a tool message whose content is a non-list object/dict to verify the JSON-serialization fallback path returns a JSON string that round-trips via json.loads; reference OpenAIConverter.convert_mixed_messages_to_openai and _normalize_tool_message_content when adding these tests so they validate both concatenation and serialization behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp_agent/workflows/llm/augmented_llm_openai.py`:
- Around line 650-654: The code currently drops the second element returned by
OpenAIConverter.convert_tool_result_to_openai(result, tool_call_id=tool_call_id)
— preserve and propagate any additional_messages returned by that call instead
of discarding them: when converted is a tuple unpack it into (tool_message,
additional_messages) and append or extend additional_messages onto the messages
buffer (same logic where tool_message is used); apply the same fix to the other
occurrence of convert_tool_result_to_openai at the nearby location so non-text
tool outputs keep their follow-up context.
---
Nitpick comments:
In `@tests/utils/test_multipart_converter_openai.py`:
- Around line 363-394: Add two unit tests for
OpenAIConverter.convert_mixed_messages_to_openai: one that supplies a tool
message with multiple text blocks to verify _normalize_tool_message_content
concatenates them into a single string (e.g., "First block Second block") and
another that supplies a tool message whose content is a non-list object/dict to
verify the JSON-serialization fallback path returns a JSON string that
round-trips via json.loads; reference
OpenAIConverter.convert_mixed_messages_to_openai and
_normalize_tool_message_content when adding these tests so they validate both
concatenation and serialization behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3770182b-3ff0-4544-a8cc-06acb1eaf9e6
📒 Files selected for processing (3)
src/mcp_agent/workflows/llm/augmented_llm_openai.pysrc/mcp_agent/workflows/llm/multipart_converter_openai.pytests/utils/test_multipart_converter_openai.py
Fixes #25
Summary:
contentis a string, not a content-part list.role="tool"messages with content blocks when mixed messages are passed through to OpenAI.Validation:
.venv/bin/python -m pytest tests/utils/test_multipart_converter_openai.py -q.venv/bin/python -m py_compile src/mcp_agent/workflows/llm/augmented_llm_openai.py src/mcp_agent/workflows/llm/multipart_converter_openai.py tests/utils/test_multipart_converter_openai.pygit diff --checkNote:
tests/workflows/llm/test_augmented_llm_openai.py, but it currently errors at fixture setup becauseOpenAIAugmentedLLMis still abstract withoutgenerate_stream; this PR leaves that unrelated test setup issue untouched.Summary by CodeRabbit
Bug Fixes
Tests