fix(bedrock): handle document content blocks with citations and context in Converse API - #26948
Conversation
|
|
Greptile SummaryThis PR fixes silently-dropped
Confidence Score: 2/5The production fix logic is sound, but the test suite cannot run at all due to a broken import, leaving the fix unverified. A P0 ImportError breaks every test added in this PR, and a separate P1 wrong-exception assertion would also fail independently. The test_bedrock_document_blocks.py (ImportError + wrong exception type) and litellm/types/llms/bedrock.py (DocumentBlock required-field regression)
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/prompt_templates/factory.py | Adds _process_document_message static method and three elif "document" branches (async user path, sync user path, tool results path); MIME map is recreated per call (P2 style issue). |
| litellm/types/llms/bedrock.py | Adds CitationsConfig TypedDict and extends DocumentBlock — but changing DocumentBlock to total=False makes the three required Bedrock API fields (format, source, name) silently optional, undermining type safety. |
| tests/test_litellm/litellm_core_utils/prompt_templates/test_bedrock_document_blocks.py | New test suite for document block handling — imports non-existent _process_bedrock_document_block (causes ImportError, breaking all 9 tests) and asserts ValueError where BadRequestError is actually raised. |
| tests/test_litellm/litellm_core_utils/prompt_templates/init.py | Empty __init__.py added to make the directory a proper Python package for test discovery — no issues. |
Reviews (1): Last reviewed commit: "fix(bedrock): handle document content bl..." | Re-trigger Greptile
| from litellm.litellm_core_utils.prompt_templates.factory import ( | ||
| _bedrock_converse_messages_pt, | ||
| _convert_to_bedrock_tool_call_result, | ||
| _process_bedrock_document_block, |
There was a problem hiding this comment.
Import error —
_process_bedrock_document_block does not exist
The test file imports _process_bedrock_document_block from factory.py, but that name is never defined or exported there. The implementation lives as BedrockConverseMessagesProcessor._process_document_message. This causes an ImportError that prevents every test in this file from running.
| def test_non_base64_source_raises(): | ||
| """Non-base64 source type raises a clear ValueError.""" | ||
| with pytest.raises(ValueError, match="base64"): | ||
| _process_bedrock_document_block( | ||
| { | ||
| "type": "document", | ||
| "source": {"type": "url", "url": "https://example.com/doc.pdf"}, |
There was a problem hiding this comment.
| class DocumentBlock(TypedDict, total=False): | ||
| format: Union[BedrockDocumentTypes, str] | ||
| source: SourceBlock | ||
| name: str | ||
| citations: CitationsConfig | ||
| context: str |
There was a problem hiding this comment.
Required fields silently dropped from
DocumentBlock
Changing DocumentBlock to total=False makes format, source, and name — which are all required by the Bedrock API — optional at the type level. Any caller that constructs a DocumentBlock without those fields will now pass type-checking but produce a runtime API error. Since only citations and context are genuinely optional, the required fields should use Required[...] annotations:
| class DocumentBlock(TypedDict, total=False): | |
| format: Union[BedrockDocumentTypes, str] | |
| source: SourceBlock | |
| name: str | |
| citations: CitationsConfig | |
| context: str | |
| class DocumentBlock(TypedDict, total=False): | |
| format: Required[Union[BedrockDocumentTypes, str]] | |
| source: Required[SourceBlock] | |
| name: Required[str] | |
| citations: CitationsConfig | |
| context: str |
| _mime_to_format: dict = { | ||
| "application/pdf": "pdf", | ||
| "text/csv": "csv", | ||
| "application/msword": "doc", | ||
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document": "docx", | ||
| "application/vnd.ms-excel": "xls", | ||
| "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet": "xlsx", | ||
| "text/html": "html", | ||
| "text/plain": "txt", | ||
| "text/markdown": "md", | ||
| } | ||
| doc_format = _mime_to_format.get(media_type, media_type.split("/")[-1]) |
There was a problem hiding this comment.
| if "document" in _doc_block: | ||
| tool_result_content_blocks.append( | ||
| BedrockToolResultContentBlock(document=_doc_block["document"]) | ||
| ) | ||
|
|
||
| message.get("name", "") |
…xt in Converse API - Add `_process_document_message()` static method on `BedrockConverseMessagesProcessor` that converts Anthropic-style document blocks to Bedrock DocumentBlock format: - Validates source.type == "base64" - Maps media_type → Bedrock format string - Generates deterministic SHA-256-based document name - Forwards `citations.enabled` → CitationsConfig on the DocumentBlock - Forwards `context` field to DocumentBlock - Add `elif element["type"] == "document"` branch in async user-message path - Add `elif element["type"] == "document"` branch in sync user-message path - Add `elif content["type"] == "document"` branch in _convert_to_bedrock_tool_call_result() - Add CitationsConfig TypedDict and extend DocumentBlock with optional citations/context fields Fixes BerriAI#26937
4d65e7a to
d038aae
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…and DocumentBlock required fields
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Relevant issues
Fixes #26937
Pre-Submission checklist
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitType
🐛 Bug Fix / ✨ Enhancement
Changes
documentcontent blocks (PDF, CSV, etc. via Anthropic's API format) were silently dropped during Bedrock Converse API message conversion. Additionally, the Bedrock Converse API supportscitationsandcontextfields onDocumentBlockwhich were never forwarded.Root cause
for element in message_block["content"]loop in both the sync and async Bedrock Converse message paths had noelif element["type"] == "document"branch — blocks were skipped silently._convert_to_bedrock_tool_call_result()only handledtextandimage_url, droppingdocumentblocks in tool results.DocumentBlockinlitellm/types/llms/bedrock.pywas missingcitationsandcontextfields.Fix
_process_document_message()static method onBedrockConverseMessagesProcessor:source.type == "base64"media_type→ Bedrock format string (pdf, csv, docx, xlsx, html, txt, md, etc.)citations: {enabled: true}→CitationsConfig(enabled=True)(Bedrock Converse API docs)contextfieldelif element["type"] == "document"branch in the async user-message pathelif element["type"] == "document"branch in the sync user-message path (_bedrock_converse_messages_pt)elif content["type"] == "document"branch in_convert_to_bedrock_tool_call_result()(tool results)CitationsConfigTypedDict and extendedDocumentBlockwith optionalcitationsandcontextfieldsBefore / After
Tests added (
tests/test_litellm/litellm_core_utils/prompt_templates/test_bedrock_document_blocks.py)test_document_block_basictest_document_block_not_dropped_with_texttest_document_block_citations_forwardedcitations.enabled=Trueforwarded toCitationsConfigtest_document_block_citations_not_set_when_disabledcitationsabsent when disabledtest_document_block_context_forwardedcontextfield forwarded to DocumentBlocktest_document_block_deterministic_nametest_document_format_mappingtest_document_in_tool_resulttest_non_base64_source_raises🤖 Generated with Claude Code