Repository navigation
fix(bedrock): collapse multimodal parts content in Converse system and tool messages - #7930
Winoren (Winoren) wants to merge 2 commits into
Conversation
…d tool messages _format_messages_for_converse stringified system and tool message content with an f-string or str(). LLMMessage.content is str | list[dict] | None, so a parts list there put the list repr into the system text block (botocore rejects the request before it is sent) or into toolResult.content[0].text. Collapse content with the shared message_content_text helper, matching the treatment user and assistant messages already get (a parts list there is forwarded to Converse unchanged). Fixes crewAIInc#7920
Strix Security ReviewWarning This pull request has 49 commits after the last Strix review ( No security issues found. Review summaryPR #7930 is a narrowly-scoped correctness fix for Updated for Reviewed by Strix |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the Bedrock change: collapsing multimodal parts in Converse system and tool messages. |
| Description check | ✅ Passed | The description identifies issue #7920, explains the problem and solution, and reports tests and quality checks. It covers the template’s required information, although it uses different section headi… |
| Linked Issues check | ✅ Passed | Issue #7920 requires text parts in system and tool messages to be collapsed with message_content_text, including lone and joined system messages. The PR applies the helper to both formatter paths an… |
| Out of Scope Changes check | ✅ Passed | The PR changes only Bedrock Converse formatting and its regression tests. These changes directly support issue #7920. No unrelated changes are reported. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…ock-converse-multimodal-content
|
Hi crewAI team, friendly ping — this PR has been open a while and the CI checks never actually ran because this is a fork PR. The GitHub Actions checks are stuck showing Could someone take a look when you have a moment? Happy to rebase on the latest |
Problem
BedrockCompletion._format_messages_for_conversestringified system and tool message content with an f-string orstr(), butLLMMessage.contentisstr | list[dict[str, Any]] | None— the list form is the multimodal parts one. On the three affected sites a parts list ended up as the list repr:systemtext (system_message += f"\n\n{content}"),system[0].text— botocore rejects the request before it is sent,toolResult.content[0].text.User and assistant messages were not affected (a parts list there is forwarded to Converse unchanged). Same class as #7670, #7674 and #7678, none of which touch the Bedrock provider.
Change
Collapse the content with the shared
message_content_texthelper (crewai.utilities.agent_utils), which returns""forNone, joins thetextparts of a list, and passes strings through — the same treatmentllm.py, memory and token estimation already apply. The"\n\n"join between multiple system messages is unchanged, and plain string content behaves exactly as before.Validation
lib/crewai/tests/llms/bedrock/test_bedrock.py:system_message == "be terse and polite"(failed before, passes after)"A\n\nbe terse and polite"(failed before, passes after)toolResult.content == [{"text": "result payload"}](failed before, passes after)pytest lib/crewai/tests/llms/bedrock/test_bedrock.py→ 42 passed, 1 skippedpytest lib/crewai/tests/llms/→ 851 passed, 20 skippedruff check/ruff format --checkclean on both changed filesFixes #7920
Note
Low Risk
Scoped to Bedrock Converse formatting with a well-used utility; plain-string paths are unchanged and covered by new tests.
Overview
Fixes Bedrock Converse message formatting when system or tool messages use multimodal parts (
contentas a list of dicts) instead of a plain string._format_messages_for_conversenow uses the sharedmessage_content_texthelper for system text (including\n\njoins) andtoolResult.content[0].text, so text parts are extracted instead of Python list reprs or invalid list payloads that botocore rejects.Adds Bedrock formatter tests for parts-based system/tool messages and a regression check that string content is unchanged.
Reviewed by Cursor Bugbot for commit 547db2d. Bugbot is set up for automated code reviews on this repo. Configure here.