From 12e1d02d4e6649da7386a9895ca1ecdc7131f973 Mon Sep 17 00:00:00 2001 From: Josh Minzner Date: Tue, 28 Apr 2026 16:48:29 -0400 Subject: [PATCH] address Greptile review feedback on tool-result PDF fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tighten _is_anthropic_document_data_uri to match the mimes Anthropic actually accepts as base64 `document` source ({application/pdf, text/plain}). The previous application/* + text/* prefix match would route e.g. data:application/json URIs through the document path, producing blocks the Anthropic API rejects. Unsupported mimes now stay on the existing image code path (same failure mode as before the fix — no regression, just stops introducing a new one). - On the Bedrock tool-result `type: "file"` branch, accept either file_data or file_id and raise BadRequestError on both-None, mirroring the user-message _process_file_message pattern. Previously a file block with only file_id was silently dropped. - Consolidate the six new PDF tool-result tests under tests/test_litellm/ only (the PR template's required location and where the unit-test CI workflow runs with coverage). The duplicate copies under tests/llm_translation/ added drift risk with no additional coverage. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../prompt_templates/factory.py | 52 +++++--- .../test_anthropic_completion.py | 110 ---------------- .../test_bedrock_completion.py | 117 ------------------ ...llm_core_utils_prompt_templates_factory.py | 74 +++++++++++ .../chat/test_converse_transformation.py | 91 ++++++++++++++ 5 files changed, 200 insertions(+), 244 deletions(-) diff --git a/litellm/litellm_core_utils/prompt_templates/factory.py b/litellm/litellm_core_utils/prompt_templates/factory.py index 2d149b3a70d..a9df0895572 100644 --- a/litellm/litellm_core_utils/prompt_templates/factory.py +++ b/litellm/litellm_core_utils/prompt_templates/factory.py @@ -1661,14 +1661,18 @@ def _sanitize_anthropic_tool_use_id(tool_use_id: str) -> str: return sanitized +_ANTHROPIC_DOCUMENT_BASE64_MEDIA_TYPES = {"application/pdf", "text/plain"} + + def _is_anthropic_document_data_uri(url: str) -> bool: - # Anthropic document blocks cover non-image mimes the API accepts via base64 - # source (application/pdf, text/*). Match the mime-type prefix in a data URI. + # Anthropic's base64 document source accepts only application/pdf and + # text/plain (see select_anthropic_content_block_type_for_file). Routing + # other mimes here would produce a document block the API rejects, so we + # leave them on the image code path. match = re.match(r"data:([^;,]+)", url) if not match: return False - mime_type = match.group(1) - return mime_type.startswith("application/") or mime_type.startswith("text/") + return match.group(1) in _ANTHROPIC_DOCUMENT_BASE64_MEDIA_TYPES def convert_to_anthropic_tool_result( @@ -4043,22 +4047,36 @@ def _convert_to_bedrock_tool_call_result( BedrockToolResultContentBlock(document=_block["document"]) ) elif content["type"] == "file": + # Match the user-message path (_process_file_message): accept + # either file_data (base64 data URI) or file_id (server-side + # reference / URL) and hand off to BedrockImageProcessor. Raise + # BadRequestError on both-None rather than silently dropping. file_obj = content.get("file") or {} file_data = file_obj.get("file_data") - if isinstance(file_data, str): - _file_block: BedrockContentBlock = ( - BedrockImageProcessor.process_image_sync(image_url=file_data) + file_id = file_obj.get("file_id") + if file_data is None and file_id is None: + raise litellm.BadRequestError( + message="file_data and file_id cannot both be None. Got={}".format( + content + ), + model="", + llm_provider="bedrock", + ) + file_format = file_obj.get("format") + _file_block: BedrockContentBlock = ( + BedrockImageProcessor.process_image_sync( + image_url=cast(str, file_id or file_data), + format=file_format, + ) + ) + if "document" in _file_block: + tool_result_content_blocks.append( + BedrockToolResultContentBlock(document=_file_block["document"]) + ) + elif "image" in _file_block: + tool_result_content_blocks.append( + BedrockToolResultContentBlock(image=_file_block["image"]) ) - if "document" in _file_block: - tool_result_content_blocks.append( - BedrockToolResultContentBlock( - document=_file_block["document"] - ) - ) - elif "image" in _file_block: - tool_result_content_blocks.append( - BedrockToolResultContentBlock(image=_file_block["image"]) - ) message.get("name", "") id = str(message.get("tool_call_id", str(uuid.uuid4()))) diff --git a/tests/llm_translation/test_anthropic_completion.py b/tests/llm_translation/test_anthropic_completion.py index 75a9c4c39a1..fdf8c24ac9e 100644 --- a/tests/llm_translation/test_anthropic_completion.py +++ b/tests/llm_translation/test_anthropic_completion.py @@ -1885,113 +1885,3 @@ def test_metadata_filter_applies_to_azure_anthropic(): headers={}, ) assert data.get("metadata") == {"user_id": "u2"} - - -def test_anthropic_tool_result_openai_file_pdf_becomes_document(): - """ - OpenAI `{type: "file", file: {file_data: "data:application/pdf;..."}}` inside - a tool-message content list should translate to an Anthropic document block - inside the tool_result content. The existing helper - `anthropic_process_openai_file_message` already does this translation for - user messages; it must be reused on the tool-result path. - """ - from litellm.litellm_core_utils.prompt_templates.factory import ( - convert_to_anthropic_tool_result, - ) - - pdf_b64 = "JVBERi0xLjQKJeLjz9MK" - message = { - "tool_call_id": "toolu_pdf_1", - "role": "tool", - "name": "fetch_document", - "content": [ - { - "type": "file", - "file": { - "file_data": f"data:application/pdf;base64,{pdf_b64}", - "filename": "summary.pdf", - }, - }, - ], - } - - result = convert_to_anthropic_tool_result(message) - - assert result["type"] == "tool_result" - assert result["tool_use_id"] == "toolu_pdf_1" - content = result["content"] - assert isinstance(content, list) and len(content) == 1 - block = content[0] - assert block["type"] == "document" - assert block["source"]["type"] == "base64" - assert block["source"]["media_type"] == "application/pdf" - assert block["source"]["data"] == pdf_b64 - - -def test_anthropic_tool_result_image_url_pdf_data_uri_becomes_document(): - """ - Regression: a PDF sent as an `image_url` data URI on the tool-result path - must translate to an Anthropic document block (not an image block — Anthropic - rejects image blocks whose media_type is a non-image like application/pdf). - """ - from litellm.litellm_core_utils.prompt_templates.factory import ( - convert_to_anthropic_tool_result, - ) - - pdf_b64 = "JVBERi0xLjQKJeLjz9MK" - message = { - "tool_call_id": "toolu_pdf_img_1", - "role": "tool", - "name": "fetch_document", - "content": [ - { - "type": "image_url", - "image_url": { - "url": f"data:application/pdf;base64,{pdf_b64}", - }, - }, - ], - } - - result = convert_to_anthropic_tool_result(message) - - content = result["content"] - assert isinstance(content, list) and len(content) == 1 - block = content[0] - assert block["type"] == "document" - assert block["source"]["media_type"] == "application/pdf" - assert block["source"]["data"] == pdf_b64 - - -def test_anthropic_tool_result_image_url_png_still_becomes_image(): - """ - Regression: image_url with a real image mime type must continue to translate - to an Anthropic image block. Locks in existing behavior after the - data-URI-mime-type branching for PDFs. - """ - from litellm.litellm_core_utils.prompt_templates.factory import ( - convert_to_anthropic_tool_result, - ) - - png_b64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGNgYGBgAAAABQABXvMqOgAAAABJRU5ErkJggg==" - message = { - "tool_call_id": "toolu_png_1", - "role": "tool", - "name": "fetch_image", - "content": [ - { - "type": "image_url", - "image_url": { - "url": f"data:image/png;base64,{png_b64}", - }, - }, - ], - } - - result = convert_to_anthropic_tool_result(message) - - content = result["content"] - assert isinstance(content, list) and len(content) == 1 - block = content[0] - assert block["type"] == "image" - assert block["source"]["media_type"] == "image/png" diff --git a/tests/llm_translation/test_bedrock_completion.py b/tests/llm_translation/test_bedrock_completion.py index c9a886d125e..ddfe383f2a5 100644 --- a/tests/llm_translation/test_bedrock_completion.py +++ b/tests/llm_translation/test_bedrock_completion.py @@ -1282,123 +1282,6 @@ def test_bedrock_converse_translation_tool_message(): ] -def test_bedrock_tool_message_openai_file_pdf_becomes_document(): - """ - OpenAI Chat Completions `{type: "file", file: {file_data: "data:application/pdf;...", filename}}` - inside a tool message content list should translate to a Bedrock - toolResult.content[].document block. This is the documented OpenAI shape for - PDFs on the Chat Completions API and what downstream callers emit. - """ - pdf_b64 = "JVBERi0xLjQKJeLjz9MK" # tiny "%PDF-1.4\n" header - messages = [ - {"role": "user", "content": "Summarize the attached PDF."}, - { - "tool_call_id": "tooluse_pdf_1", - "role": "tool", - "name": "fetch_document", - "content": [ - { - "type": "file", - "file": { - "file_data": f"data:application/pdf;base64,{pdf_b64}", - "filename": "summary.pdf", - }, - }, - ], - }, - ] - - translated_msg = _bedrock_converse_messages_pt( - messages=messages, model="", llm_provider="" - ) - - tool_result = translated_msg[-1]["content"][-1]["toolResult"] - assert tool_result["toolUseId"] == "tooluse_pdf_1" - assert len(tool_result["content"]) == 1 - block = tool_result["content"][0] - assert "document" in block, f"expected document block, got {block}" - assert block["document"]["format"] == "pdf" - assert block["document"]["source"]["bytes"] == pdf_b64 - assert block["document"]["name"].startswith("DocumentPDFmessages_") - assert block["document"]["name"].endswith("_pdf") - - -def test_bedrock_tool_message_image_url_pdf_data_uri_becomes_document(): - """ - Regression for the processor-returns-document-but-wrapper-drops-it bug: - when a caller sends a PDF as an `image_url` data URI on the tool-result path, - BedrockImageProcessor correctly routes it through the document path and - returns a {"document": ...} block, but the tool-result wrapper only - appended the "image" case, silently dropping documents. - """ - pdf_b64 = "JVBERi0xLjQKJeLjz9MK" - messages = [ - {"role": "user", "content": "Summarize the attached PDF."}, - { - "tool_call_id": "tooluse_pdf_img_1", - "role": "tool", - "name": "fetch_document", - "content": [ - { - "type": "image_url", - "image_url": { - "url": f"data:application/pdf;base64,{pdf_b64}", - }, - }, - ], - }, - ] - - translated_msg = _bedrock_converse_messages_pt( - messages=messages, model="", llm_provider="" - ) - - tool_result = translated_msg[-1]["content"][-1]["toolResult"] - assert tool_result["toolUseId"] == "tooluse_pdf_img_1" - assert len(tool_result["content"]) == 1 - block = tool_result["content"][0] - assert "document" in block, f"expected document block, got {block}" - assert block["document"]["format"] == "pdf" - assert block["document"]["source"]["bytes"] == pdf_b64 - - -def test_bedrock_tool_message_image_url_png_still_becomes_image(): - """ - Regression: image_url with an image mime type must continue to translate - to a Bedrock image block (not document). Locks in existing behavior after - the document-passthrough fix. - """ - png_b64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGNgYGBgAAAABQABXvMqOgAAAABJRU5ErkJggg==" - messages = [ - {"role": "user", "content": "Describe the attached image."}, - { - "tool_call_id": "tooluse_png_1", - "role": "tool", - "name": "fetch_image", - "content": [ - { - "type": "image_url", - "image_url": { - "url": f"data:image/png;base64,{png_b64}", - }, - }, - ], - }, - ] - - translated_msg = _bedrock_converse_messages_pt( - messages=messages, model="", llm_provider="" - ) - - tool_result = translated_msg[-1]["content"][-1]["toolResult"] - assert len(tool_result["content"]) == 1 - block = tool_result["content"][0] - assert "image" in block, f"expected image block, got {block}" - assert "document" not in block - assert block["image"]["format"] == "png" - assert block["image"]["source"]["bytes"] == png_b64 - - def test_base_aws_llm_get_credentials(): import time diff --git a/tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py b/tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py index d9b458ad8bd..7d9647c95c3 100644 --- a/tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py +++ b/tests/test_litellm/litellm_core_utils/prompt_templates/test_litellm_core_utils_prompt_templates_factory.py @@ -2553,6 +2553,80 @@ def test_convert_to_anthropic_tool_result_image_url_pdf_data_uri_becomes_documen assert block["source"]["data"] == pdf_b64 +def test_convert_to_anthropic_tool_result_image_url_unsupported_mime_stays_image_path(): + """ + An `image_url` data URI whose mime is neither application/pdf nor text/plain + (e.g. application/json) must NOT be routed through the document path. Anthropic + only accepts application/pdf and text/plain as base64 document media_types — + anything else would produce a document block the API rejects. The old + (pre-fix) behavior was to wrap such data as an image block, which also + fails but stays on the image code path; preserve that failure mode rather + than switching to a document path that is equally broken. + """ + from litellm.litellm_core_utils.prompt_templates.factory import ( + convert_to_anthropic_tool_result, + ) + + message = { + "tool_call_id": "toolu_json_1", + "role": "tool", + "name": "fetch_json", + "content": [ + { + "type": "image_url", + "image_url": { + "url": "data:application/json;base64,eyJrIjoidiJ9", + }, + }, + ], + } + + result = convert_to_anthropic_tool_result(message) + + content = result["content"] + assert isinstance(content, list) and len(content) == 1 + block = content[0] + assert block["type"] == "image", ( + f"unsupported mime {block.get('source', {}).get('media_type')!r} " + f"should not be routed to document path; got {block}" + ) + + +def test_convert_to_anthropic_tool_result_image_url_text_plain_data_uri_becomes_document(): + """ + text/plain is one of the two mimes Anthropic accepts as a base64 document + media_type. Confirm it routes through the document path so tightening the + gate to {application/pdf, text/plain} (not "application/*") covers both. + """ + from litellm.litellm_core_utils.prompt_templates.factory import ( + convert_to_anthropic_tool_result, + ) + + txt_b64 = "aGVsbG8=" # "hello" + message = { + "tool_call_id": "toolu_txt_1", + "role": "tool", + "name": "fetch_text", + "content": [ + { + "type": "image_url", + "image_url": { + "url": f"data:text/plain;base64,{txt_b64}", + }, + }, + ], + } + + result = convert_to_anthropic_tool_result(message) + + content = result["content"] + assert isinstance(content, list) and len(content) == 1 + block = content[0] + assert block["type"] == "document" + assert block["source"]["media_type"] == "text/plain" + assert block["source"]["data"] == txt_b64 + + def test_convert_to_anthropic_tool_result_image_url_png_still_becomes_image(): """ Regression: image_url with a real image mime type must continue to translate diff --git a/tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py b/tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py index 02477c24c42..21e87dfc17c 100644 --- a/tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py +++ b/tests/test_litellm/llms/bedrock/chat/test_converse_transformation.py @@ -4234,6 +4234,97 @@ def test_bedrock_tool_message_image_url_pdf_data_uri_becomes_document(): assert block["document"]["source"]["bytes"] == pdf_b64 +def test_bedrock_tool_message_file_id_http_url_becomes_document(): + """ + OpenAI `file.file_id` is a server-side file reference. The Bedrock + user-message path (_process_file_message at factory.py:4796) accepts either + `file_data` or `file_id` and forwards to BedrockImageProcessor. The + tool-result path must match: when `file_id` is an http(s) PDF URL, it + should resolve to a Bedrock document block, not be silently dropped. + """ + from unittest.mock import patch + + from litellm.litellm_core_utils.prompt_templates.factory import ( + BedrockImageProcessor, + _bedrock_converse_messages_pt, + ) + + pdf_url = "https://example.com/whitepaper.pdf" + fake_document_block = { + "document": { + "format": "pdf", + "name": "fake_doc", + "source": {"bytes": "ZmFrZQ=="}, + } + } + messages = [ + {"role": "user", "content": "Summarize the attached PDF."}, + { + "tool_call_id": "tooluse_fid_1", + "role": "tool", + "name": "fetch_document", + "content": [ + { + "type": "file", + "file": { + "file_id": pdf_url, + "filename": "whitepaper.pdf", + }, + }, + ], + }, + ] + + with patch.object( + BedrockImageProcessor, + "process_image_sync", + return_value=fake_document_block, + ) as mock_proc: + translated_msg = _bedrock_converse_messages_pt( + messages=messages, model="", llm_provider="" + ) + + mock_proc.assert_called_once() + assert mock_proc.call_args.kwargs["image_url"] == pdf_url + + tool_result = translated_msg[-1]["content"][-1]["toolResult"] + assert len(tool_result["content"]) == 1 + block = tool_result["content"][0] + assert "document" in block, f"expected document block, got {block}" + assert block["document"]["source"]["bytes"] == "ZmFrZQ==" + + +def test_bedrock_tool_message_file_without_data_or_id_raises(): + """ + The user-message path raises BadRequestError when a `type: "file"` block + has neither `file_data` nor `file_id` (factory.py:4802-4809). The + tool-result path must match — silently dropping the block makes the model + see an empty tool result and obscures the caller bug. + """ + import litellm + from litellm.litellm_core_utils.prompt_templates.factory import ( + _bedrock_converse_messages_pt, + ) + + messages = [ + {"role": "user", "content": "Summarize."}, + { + "tool_call_id": "tooluse_bad_1", + "role": "tool", + "name": "fetch_document", + "content": [ + { + "type": "file", + "file": {"filename": "nothing.pdf"}, + }, + ], + }, + ] + + with pytest.raises(litellm.BadRequestError): + _bedrock_converse_messages_pt(messages=messages, model="", llm_provider="") + + def test_bedrock_tool_message_image_url_png_still_becomes_image(): """ Regression: image_url with an image mime type must continue to translate