fix(bedrock): preserve document content blocks for Converse API (#24641) - #24646
fix(bedrock): preserve document content blocks for Converse API (#24641)#24646sjhddh wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds pass-through handling for The fix touches four code paths in
Several critical bugs raised in earlier review rounds (wrong accumulator lists,
Confidence Score: 4/5Functionally correct on the common path; safe to merge once tests are added to satisfy project policy. The critical bugs from earlier review rounds (NameErrors, wrong list usage, bare litellm/litellm_core_utils/prompt_templates/factory.py — four new document-block branches need test coverage.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/prompt_templates/factory.py | Adds document content-block pass-through for Bedrock Converse API in four places: Anthropic tool-result conversion, Bedrock tool-call-result conversion, and user/assistant message processing in BedrockConverseMessagesProcessor. Previously flagged critical bugs (NameErrors, wrong accumulator lists, raw type key sent to Bedrock) all appear resolved; no tests were added. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming message with document content block] --> B{Destination API}
B -->|Anthropic| C[convert_to_anthropic_tool_result]
B -->|Bedrock - tool result| D[_convert_to_bedrock_tool_call_result]
B -->|Bedrock - user msg| E[BedrockConverseMessagesProcessor user branch]
B -->|Bedrock - assistant msg| F[BedrockConverseMessagesProcessor assistant branch]
C --> C1[Pass content dict as-is, type key retained for Anthropic]
C1 --> C2[anthropic_content_list.append]
D --> D1[doc_content = strip type key]
D1 --> D2[BedrockToolResultContentBlock document=doc_content]
D2 --> D3[tool_result_content_blocks.append]
E --> E1[doc_content = strip type key]
E1 --> E2[BedrockContentBlock document=doc_content]
E2 --> E3[_parts.append]
F --> F1[doc_content = strip type key]
F1 --> F2[BedrockContentBlock document=doc_content]
F2 --> F3[assistants_parts.append]
Reviews (5): Last reviewed commit: "fix(bedrock): resolve Anthropic API type..." | Re-trigger Greptile
| elif content["type"] == "document": | ||
| tool_result_content_blocks.append( | ||
| BedrockToolResultContentBlock(document=content) # Assume it matches Bedrock structure or Anthropic fallback | ||
| ) |
There was a problem hiding this comment.
NameError: tool_result_content_blocks is not defined in this scope
convert_to_anthropic_tool_result is an Anthropic-specific function — it builds an AnthropicMessagesToolResultParam, not a Bedrock structure. There is no tool_result_content_blocks variable defined in this function, so this code will raise NameError: name 'tool_result_content_blocks' is not defined at runtime whenever a tool message contains a document content block.
The document should instead be appended to anthropic_content_list in the Anthropic-compatible format, not routed to a Bedrock type.
| elif content["type"] == "document": | |
| tool_result_content_blocks.append( | |
| BedrockToolResultContentBlock(document=content) # Assume it matches Bedrock structure or Anthropic fallback | |
| ) | |
| elif content["type"] == "document": | |
| anthropic_content_list.append(content) |
| elif element["type"] == "document": | ||
| _part = BedrockContentBlock(document=element) # Pass it through since element already has format, name, source | ||
| _parts.append(_part) |
There was a problem hiding this comment.
Wrong list used in assistant content branch
In this assistant content block, all other content types append to assistants_parts (see text on line 4605, image_url on line 4617). However, this document branch incorrectly appends to _parts, which belongs to the outer user content loop (_parts: List[BedrockContentBlock] defined around line 4422). This silently contaminates the user content list with assistant document blocks and drops them from the assistant message.
| elif element["type"] == "document": | |
| _part = BedrockContentBlock(document=element) # Pass it through since element already has format, name, source | |
| _parts.append(_part) | |
| elif element["type"] == "document": | |
| _part = BedrockContentBlock(document=element) # Pass it through since element already has format, name, source | |
| assistants_parts.append(_part) |
| elif element["type"] == "document": | ||
| _part = BedrockContentBlock(document=element) # Pass it through since element already has format, name, source | ||
| _parts.append(_part) |
There was a problem hiding this comment.
type key passed to Bedrock DocumentBlock may cause API validation errors
The raw OpenAI-format content element (element) is passed directly as document=element. This dict includes a "type": "document" key, but Bedrock's DocumentBlock TypedDict only defines format, source, and name. The extra type field is serialized and sent to the Bedrock Converse API, which may reject the request with a validation error.
The same issue applies to BedrockToolResultContentBlock(document=content) at lines 3978 and 1731.
The type key should be stripped before passing:
# Extract only the fields Bedrock's DocumentBlock expects
_document_block = {
k: v for k, v in element.items() if k != "type"
}
_part = BedrockContentBlock(document=_document_block)
_parts.append(_part)The same fix applies at lines ~3978 and ~1731 where content is passed to BedrockToolResultContentBlock(document=content).
| guardContent={"text": {"text": element["text"]}} | ||
| ) | ||
| _parts.append(_part) | ||
| elif element["type"] == "document": | ||
| _part = BedrockContentBlock(document=element) # Pass it through since element already has format, name, source | ||
| _parts.append(_part) |
There was a problem hiding this comment.
The PR description claims to fix issue #24641 but no unit tests were added to verify the new document block pass-through behaviour in any of the four code paths. Per project policy, fixes should be accompanied by evidence of resolution (e.g. mock unit tests). Relevant test files already exist:
tests/litellm_core_utils/test_bedrock_converse_dedup_factory.pytests/llm_translation/test_bedrock_completion.py
Consider adding at minimum one test for _bedrock_converse_messages_pt and one for _convert_to_bedrock_tool_call_result that asserts document blocks are preserved in the output.
Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
…ding, drop type key
| elif content["type"] == "document": | ||
| # Remove 'type' key from content dict to pass to Anthropic / Bedrock block | ||
| doc_content = {k: v for k, v in content.items() if k != "type"} | ||
| anthropic_content_list.append(doc_content) |
There was a problem hiding this comment.
NameError: anthropic_content_list is not defined in _convert_to_bedrock_tool_call_result
This function only initialises tool_result_content_blocks (line 3964). There is no anthropic_content_list variable anywhere in _convert_to_bedrock_tool_call_result, so this line will raise NameError: name 'anthropic_content_list' is not defined at runtime whenever a tool-message content list contains a document block — the exact scenario this PR intends to fix.
The document block should be wrapped in a BedrockToolResultContentBlock and appended to tool_result_content_blocks, mirroring the text and image branches immediately above:
| anthropic_content_list.append(doc_content) | |
| doc_content = {k: v for k, v in content.items() if k != "type"} | |
| tool_result_content_blocks.append( | |
| BedrockToolResultContentBlock(document=doc_content) | |
| ) |
…t content blocks (BerriAI#24641) Resolves two critical P0/P1 bugs flagged during code review: - Fixed a `NameError` in `_convert_to_bedrock_tool_call_result` where `anthropic_content_list` was used instead of the local `tool_result_content_blocks` list. - Fixed the assistant message branch in `BedrockConverseMessagesProcessor` which mistakenly appended document blocks to the user message list (`_parts`) instead of `assistants_parts`. These fixes ensure Bedrock Converse API reliably parses and passes through 'document' content blocks for both tool results and assistant message history.
🛠️ Code Review Updates appliedThanks for the meticulous catch by the Greptile bot! The two specific P0/P1 issues identified in the code paths have been resolved in this latest commit:
These changes ensure the Converse API safely passes through |
| elif element["type"] == "document": | ||
| doc_content = {k: v for k, v in element.items() if k != "type"} | ||
| assistants_part = BedrockContentBlock(document=doc_content) | ||
| assistants_parts.append(assistants_part) |
There was a problem hiding this comment.
NameError / wrong list in user-message document branch
This document branch sits inside the user message processing loop (while … messages[msg_i]["role"] == "user", lines 4418–4480) where the accumulator for the current message's parts is _parts (line 4424). Every other type in this branch appends to _parts:
text→_parts.append(_part)(line 4429)guarded_text→_parts.append(_part)(line 4435)image_url→_parts.append(_part)(line 4450)file→_parts.append(_part)(line 4455)
But this new document case appends to assistants_parts, which is only defined inside the assistant processing block starting at line 4589. Whenever a conversation begins with a user message that contains a document block (the common case), Python will raise NameError: name 'assistants_parts' is not defined. Even when it happens to be in scope from a prior loop iteration, the document block will be silently injected into the assistant content list rather than the user content list.
The variable name chosen (assistants_part) is also inconsistent with this branch's naming convention (_part).
| elif element["type"] == "document": | |
| doc_content = {k: v for k, v in element.items() if k != "type"} | |
| assistants_part = BedrockContentBlock(document=doc_content) | |
| assistants_parts.append(assistants_part) | |
| elif element["type"] == "document": | |
| doc_content = {k: v for k, v in element.items() if k != "type"} | |
| _part = BedrockContentBlock(document=doc_content) | |
| _parts.append(_part) |
…document blocks (BerriAI#24641) Resolves the remaining Greptile review findings where my previous AST hotfix accidentally applied substitutions to the wrong branches: - `convert_to_anthropic_tool_result` (line 1729) correctly reverts to using `anthropic_content_list` instead of the undefined bedrock list. - `BedrockConverseMessagesProcessor` user branch (line 4436) correctly appends document blocks to `_parts` instead of leaking into `assistants_parts`. The Bedrock tool result and assistant branches remain correctly updated.
🛠️ Final Code Review Updates applied (Greptile P0 fixes)Thank you to the Greptile bot for the follow-up review! The previous hotfix accidentally cross-contaminated the AST replacements between the Anthropic and Bedrock branches, introducing the This has been definitively resolved in the latest commit:
All 4 pathways are now syntactically clean and routing documents to the correct respective accumulators. Ready for the next CI pass! |
… result NameError for document blocks (BerriAI#24641) Addresses the latest automated review findings for the `document` content block pass-through logic: - **Anthropic Tool Result**: Removed the stripping of the `type` field. The Anthropic API explicitly requires `"type": "document"` for document content blocks, so the raw dict is now passed through. - **Bedrock Tool Call Result**: Resolved the persistent `NameError`. Correctly replaced the erroneous `anthropic_content_list.append` with `tool_result_content_blocks.append(BedrockToolResultContentBlock(document=doc_content))` which matches the surrounding block logic.
🛠️ P0 / Logic Fixes Finalized (Review Addressed)The latest automated review hit the nail on the head regarding the dual issues in Summary of what's fixed in the latest commit:
The user/assistant branches for Bedrock converse remain correct (stripping All four content block code paths now correctly handle |
… path
OpenAI Chat Completions `{type: "file", file: {file_data: "data:application/pdf;..."}}`
content blocks inside tool messages were silently dropped when translated to
Bedrock Converse and direct Anthropic. Additionally, PDFs sent via `image_url`
data URIs were either dropped (Bedrock) or wrapped as `type: "image"` and
rejected by the API (Anthropic).
- _convert_to_bedrock_tool_call_result: add `type: "file"` branch; pass through
document blocks produced by BedrockImageProcessor for PDF `image_url` URIs.
Single choke point covers both sync and async converse paths.
- convert_to_anthropic_tool_result: add `type: "file"` branch delegating to
`anthropic_process_openai_file_message`; branch `image_url` on data-URI mime
type so non-image mimes route through the file helper to produce document
blocks.
- AnthropicMessagesToolResultParam.content union extended to accept
`AnthropicMessagesDocumentParam` alongside text and image.
- Add 6 tests (3 Bedrock + 3 Anthropic) covering file-PDF, image_url-PDF, and
image_url-PNG regression.
Fixes BerriAI#24641
Supersedes BerriAI#24646 with an OpenAI-native approach and test coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… path
OpenAI Chat Completions `{type: "file", file: {file_data: "data:application/pdf;..."}}`
content blocks inside tool messages were silently dropped when translated to
Bedrock Converse and direct Anthropic. Additionally, PDFs sent via `image_url`
data URIs were either dropped (Bedrock) or wrapped as `type: "image"` and
rejected by the API (Anthropic).
- _convert_to_bedrock_tool_call_result: add `type: "file"` branch; pass through
document blocks produced by BedrockImageProcessor for PDF `image_url` URIs.
Single choke point covers both sync and async converse paths.
- convert_to_anthropic_tool_result: add `type: "file"` branch delegating to
`anthropic_process_openai_file_message`; branch `image_url` on data-URI mime
type so non-image mimes route through the file helper to produce document
blocks.
- AnthropicMessagesToolResultParam.content union extended to accept
`AnthropicMessagesDocumentParam` alongside text and image.
- Add 6 tests (3 Bedrock + 3 Anthropic) covering file-PDF, image_url-PDF, and
image_url-PNG regression.
Fixes BerriAI#24641
Supersedes BerriAI#24646 with an OpenAI-native approach and test coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
#24641. has been fixed. Closing this |
… path
OpenAI Chat Completions `{type: "file", file: {file_data: "data:application/pdf;..."}}`
content blocks inside tool messages were silently dropped when translated to
Bedrock Converse and direct Anthropic. Additionally, PDFs sent via `image_url`
data URIs were either dropped (Bedrock) or wrapped as `type: "image"` and
rejected by the API (Anthropic).
- _convert_to_bedrock_tool_call_result: add `type: "file"` branch; pass through
document blocks produced by BedrockImageProcessor for PDF `image_url` URIs.
Single choke point covers both sync and async converse paths.
- convert_to_anthropic_tool_result: add `type: "file"` branch delegating to
`anthropic_process_openai_file_message`; branch `image_url` on data-URI mime
type so non-image mimes route through the file helper to produce document
blocks.
- AnthropicMessagesToolResultParam.content union extended to accept
`AnthropicMessagesDocumentParam` alongside text and image.
- Add 6 tests (3 Bedrock + 3 Anthropic) covering file-PDF, image_url-PDF, and
image_url-PNG regression.
Fixes BerriAI#24641
Supersedes BerriAI#24646 with an OpenAI-native approach and test coverage.
Fixes #24641.\n\nLiteLLM currently drops
documentcontent blocks when converting messages to the Bedrock Converse API format because_bedrock_converse_messages_ptand_convert_to_bedrock_tool_call_resultonly processtext,guarded_text, andimage_urltypes. This prevents the model from accessing passed PDF attachments.\n\nThis patch explicitly checks for and passes throughdocumenttype blocks by wrapping them into valid Bedrock Content structures, both for standard user messages and within tool results.