diff --git a/src/services/adk/runners/runner_utils.py b/src/services/adk/runners/runner_utils.py index 7fe7ae6..402bb31 100644 --- a/src/services/adk/runners/runner_utils.py +++ b/src/services/adk/runners/runner_utils.py @@ -274,6 +274,7 @@ async def process_files( transcribed_texts = [] if files and len(files) > 0: + inlined_bytes = 0 for file_data in files: try: # Check if file is audio @@ -293,6 +294,32 @@ async def process_files( ) ) + # EVO-2181: add the file to the content parts so the model + # actually receives it. This append used to be gated behind + # `if is_audio`, so an image was blobbed and saved to the + # artifact store but never handed to the LLM -> the agent + # replied "No content to process". + # + # It runs before save_artifact on purpose: the bytes are + # already in hand, and a failure while storing them must not + # cost the model its copy of the file and bring this very bug + # back. + skip_reason = self._inline_skip_reason( + file_data.content_type, len(file_bytes), inlined_bytes + ) + if skip_reason: + logger.warning( + f"File {file_data.filename} ({file_data.content_type}) not sent" + f" to the model: {skip_reason}. Still saved as an artifact." + ) + else: + inlined_bytes += len(file_bytes) + file_parts.append(file_part) + logger.info( + f"Added {'audio' if is_audio else 'file'} {file_data.filename}" + f" ({file_data.content_type}) to content parts for LLM processing" + ) + # Always save to artifacts for reference await artifacts_service.save_artifact( app_name=agent_id, @@ -301,12 +328,6 @@ async def process_files( filename=file_data.filename, artifact=file_part, ) - if is_audio: - # Audio file - add to content parts for LLM processing - file_parts.append(file_part) - logger.info( - f"Added audio file {file_data.filename} to content parts for LLM processing" - ) except Exception as e: logger.error( @@ -315,6 +336,55 @@ async def process_files( return file_parts, transcribed_texts + # What google-adk can actually turn into a model content part. Its LiteLlm + # path (`_get_content`) decodes text/* inline, maps image//audio//video/ to + # the matching data-uri part and application/pdf + application/json to a file + # part, then raises ValueError on anything else -- and the runner turns that + # ValueError into a 500, losing the whole turn, the user's text included. + # Mirrored here because it is a private ADK detail; test_media_file_parts + # pins the two together against the installed version. + _MODEL_READABLE_MIME_PREFIXES = ("text/", "image/", "audio/", "video/") + _MODEL_READABLE_MIME_TYPES = frozenset({"application/pdf", "application/json"}) + + # Ceilings on what we inline, mirroring the bounds the bot-runtime already + # applies when downloading the attachment (ai_adapter.go: maxAttachmentBytes + # / maxAttachmentsTotalBytes). Providers reject an oversized inline request, + # and that rejection is a 500 here too. + MAX_INLINE_FILE_BYTES = 15 * 1024 * 1024 + MAX_INLINE_REQUEST_BYTES = 20 * 1024 * 1024 + + def _inline_skip_reason( + self, content_type: str, size_bytes: int, already_inlined: int + ) -> Optional[str]: + """Why this file must not travel to the model as inline data, or None. + + Skipping is the graceful path: the file is still saved as an artifact and + the rest of the message -- text and readable media -- still gets an + answer. Forwarding it instead costs the whole turn. + """ + # "audio/webm;codecs=opus" -> "audio/webm". The Blob keeps the content + # type verbatim; only the check normalizes. + mime_type = (content_type or "").split(";")[0].strip().lower() + if not ( + mime_type.startswith(self._MODEL_READABLE_MIME_PREFIXES) + or mime_type in self._MODEL_READABLE_MIME_TYPES + ): + return "the model layer cannot carry this content type as inline data" + + if size_bytes > self.MAX_INLINE_FILE_BYTES: + return ( + f"{size_bytes} bytes is over the {self.MAX_INLINE_FILE_BYTES} byte" + " per-file inline limit" + ) + + if already_inlined + size_bytes > self.MAX_INLINE_REQUEST_BYTES: + return ( + "it would push this request past the" + f" {self.MAX_INLINE_REQUEST_BYTES} byte inline budget" + ) + + return None + def _is_audio_file(self, content_type: str, filename: str) -> bool: """Check if file is an audio file based on content type and extension.""" if not content_type and not filename: diff --git a/tests/unit/test_media_file_parts.py b/tests/unit/test_media_file_parts.py new file mode 100644 index 0000000..ec979af --- /dev/null +++ b/tests/unit/test_media_file_parts.py @@ -0,0 +1,189 @@ +"""EVO-2181: every incoming file the model can read must reach the model. + +process_files used to append the file part to `file_parts` only `if is_audio`, so +images were blobbed + saved to artifacts but never sent to the LLM, and +create_content("", file_parts) returned None -> "No content to process". + +The other half of the contract: what the model layer *cannot* carry must stay out +of the content parts. google-adk's LiteLlm raises ValueError on a mime type it +does not know, the runner turns that into a 500, and the user loses the whole +turn -- their text included. A plain WhatsApp document (docx/zip) takes exactly +that path, and a caller that omits `mimeType` gets application/octet-stream from +a2a_routes.extract_files_from_message. +""" + +from __future__ import annotations + +import asyncio +import base64 +from unittest.mock import AsyncMock, MagicMock + +import pytest +from google.adk.models.lite_llm import _get_content + +from src.schemas.chat import FileData +from src.services.adk.runners.runner_utils import RunnerUtils + +DOCX = "application/vnd.openxmlformats-officedocument.wordprocessingml.document" + + +def _utils(): + # Bypass __init__ (which builds AgentBuilder(db)); the methods under test use + # neither self.db nor self.agent_builder. + return RunnerUtils.__new__(RunnerUtils) + + +def _artifacts(): + a = MagicMock() + a.save_artifact = AsyncMock() + return a + + +def _file(name, ctype): + return FileData( + filename=name, content_type=ctype, data=base64.b64encode(b"bytes-" + name.encode()).decode() + ) + + +def _run(coro): + return asyncio.run(coro) + + +def test_image_is_appended_to_file_parts(): + parts, transcribed = _run( + _utils().process_files([_file("photo.png", "image/png")], _artifacts(), "agent", "ext", "sess") + ) + assert len(parts) == 1 + assert parts[0].inline_data.mime_type == "image/png" + assert transcribed == [] + + +def test_audio_still_appended(): + parts, _ = _run( + _utils().process_files([_file("voice.ogg", "audio/ogg")], _artifacts(), "a", "e", "s") + ) + assert len(parts) == 1 + assert parts[0].inline_data.mime_type == "audio/ogg" + + +def test_image_and_audio_both_appended(): + parts, _ = _run( + _utils().process_files( + [_file("p.png", "image/png"), _file("v.ogg", "audio/ogg")], _artifacts(), "a", "e", "s" + ) + ) + assert len(parts) == 2 + + +def test_create_content_with_image_only_is_not_none(): + utils = _utils() + parts, _ = _run(utils.process_files([_file("photo.png", "image/png")], _artifacts(), "a", "e", "s")) + content = utils.create_content("", parts) + assert content is not None # regression guard for "No content to process" + assert content.role == "user" + assert len(content.parts) == 2 # empty text part + the image file part + + +def test_create_content_empty_is_none(): + assert _utils().create_content("", []) is None + + +@pytest.mark.parametrize("content_type", [DOCX, "application/zip", "application/octet-stream", ""]) +def test_unreadable_file_stays_out_of_the_content_parts(content_type): + artifacts = _artifacts() + parts, _ = _run( + _utils().process_files([_file("file.bin", content_type)], artifacts, "a", "e", "s") + ) + assert parts == [] + artifacts.save_artifact.assert_awaited_once() # still kept for reference + + +def test_unreadable_file_never_costs_the_text_reply(): + # The regression this guards: forwarding the docx raises ValueError inside + # LiteLlm, the runner answers 500 and the caption goes unanswered. + utils = _utils() + parts, _ = _run(utils.process_files([_file("planilha.docx", DOCX)], _artifacts(), "a", "e", "s")) + content = utils.create_content("segue o documento", parts) + assert content is not None + assert content.parts[0].text == "segue o documento" + assert all(p.inline_data is None for p in content.parts) + + +def test_unreadable_file_does_not_drop_the_image_next_to_it(): + parts, _ = _run( + _utils().process_files( + [_file("a.zip", "application/zip"), _file("p.png", "image/png")], _artifacts(), "a", "e", "s" + ) + ) + assert [p.inline_data.mime_type for p in parts] == ["image/png"] + + +def test_pdf_and_text_still_reach_the_model(): + parts, _ = _run( + _utils().process_files( + [_file("r.pdf", "application/pdf"), _file("n.txt", "text/plain")], _artifacts(), "a", "e", "s" + ) + ) + assert [p.inline_data.mime_type for p in parts] == ["application/pdf", "text/plain"] + + +def test_mime_parameters_do_not_break_the_check(): + parts, _ = _run( + _utils().process_files([_file("v.webm", "audio/webm;codecs=opus")], _artifacts(), "a", "e", "s") + ) + # Normalized for the check, verbatim on the Blob. + assert [p.inline_data.mime_type for p in parts] == ["audio/webm;codecs=opus"] + + +def test_image_survives_an_artifact_store_failure(): + artifacts = MagicMock() + artifacts.save_artifact = AsyncMock(side_effect=RuntimeError("artifact store down")) + parts, _ = _run(_utils().process_files([_file("p.png", "image/png")], artifacts, "a", "e", "s")) + assert [p.inline_data.mime_type for p in parts] == ["image/png"] + + +def test_oversized_file_is_not_inlined(monkeypatch): + monkeypatch.setattr(RunnerUtils, "MAX_INLINE_FILE_BYTES", 4) + artifacts = _artifacts() + parts, _ = _run(_utils().process_files([_file("big.png", "image/png")], artifacts, "a", "e", "s")) + assert parts == [] + artifacts.save_artifact.assert_awaited_once() + + +def test_inline_budget_is_per_request(monkeypatch): + # first.png decodes to 15 bytes, second.png to 16 -> only the first fits. + monkeypatch.setattr(RunnerUtils, "MAX_INLINE_REQUEST_BYTES", 20) + parts, _ = _run( + _utils().process_files( + [_file("first.png", "image/png"), _file("second.png", "image/png")], _artifacts(), "a", "e", "s" + ) + ) + assert len(parts) == 1 + + +def test_every_forwarded_part_is_accepted_by_the_installed_adk(): + """Pins the allowlist to what google-adk actually converts. + + `_get_content` is what raises the ValueError the runner turns into a 500. If + an ADK bump narrows the accepted set, this fails here instead of in front of + a customer. + """ + utils = _utils() + samples = [ + "image/png", + "image/jpeg", + "image/webp", + "audio/ogg", + "audio/mpeg", + "video/mp4", + "text/plain", + "application/pdf", + "application/json", + ] + parts, _ = _run( + utils.process_files( + [_file(f"f{i}.bin", ctype) for i, ctype in enumerate(samples)], _artifacts(), "a", "e", "s" + ) + ) + assert len(parts) == len(samples) + _get_content(utils.create_content("", parts).parts) # must not raise