From a51dd10cc7b67f8381a4ad58d9940709bc2db1ef Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 5 Aug 2026 11:25:28 +0100 Subject: [PATCH] fix(openai): stop telling users their readable PDF could not be processed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _upload_file_to_openai short-circuited on `"openai_file_id" in attachment`. Attachment dicts are built by row_to_dict from `SELECT *`, so that key is always present and is NULL until an upload caches one — every fresh attachment read as a cache hit. The method returned None without uploading and, critically, without raising, so the except branch that inlines the extracted text never ran. The part went out as `{"file_id": None}`, which _resolve_file_part degraded to `[File 'upload.pdf' could not be processed]`. The model then relayed that to the user while thousands of tokens of extracted text sat unused on the attachment row. Reproduced live against the Foundry deployment: the model answers "I couldn't access the attached PDF because it could not be processed; please re-upload it or paste the text". The same bug was fixed in google_ai.py by 8e9f661e ("Truthy check, not membership") which never touched this file; d29a7aaf later turned the resulting hard 400 into this silent wrong answer. Truthy check restored, a falsy file_id now raises so the fallback runs, and the fallback tests `content` truthily — it too was a membership test, so an empty extraction sent a bare "File content:" header that reads as a successfully-read document. With no content at all we emit a note naming the user's own file instead of the `upload.pdf` placeholder. The file part deliberately carries `file_id` only: sending `filename` alongside it is rejected with `400 Unknown parameter: 'messages[0].content[1].file.filename'`. Two consequences of making this path live for the first time are handled here. `attachments.openai_file_id` is a single global column, so ids are now stamped with an endpoint fingerprint — an id minted against one deployment replayed against another answers "No such File object" on every retry, permanently, since the row keeps the bad id. A foreign or legacy unscoped value is treated as a miss and re-uploaded, so it self-heals. And inlined text is bounded against the model's context window, because _enforce_context_window runs before attachments are merged into the messages and nothing downstream would trim a 200-page extraction. --- application/llm/openai.py | 118 ++++++++++++++++++++++++-- tests/llm/test_openai.py | 172 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 278 insertions(+), 12 deletions(-) diff --git a/application/llm/openai.py b/application/llm/openai.py index 0a0a27b2..466ea0b1 100644 --- a/application/llm/openai.py +++ b/application/llm/openai.py @@ -3,6 +3,7 @@ import hashlib import io import json import logging +import os.path from openai import OpenAI @@ -1654,16 +1655,49 @@ class OpenAILLM(BaseLLM): logging.info(f"Attempting to upload PDF to OpenAI: {attachment.get('path', 'unknown')}") try: file_id = self._upload_file_to_openai(attachment) + if not file_id: + # Never emit ``{"file_id": None}``: the part carries no + # document, providers 4xx or silently ignore it, and + # downstream cleaning degrades it to a note claiming the + # file "could not be processed" — while the parsed text + # sits unused on the attachment row. + raise ValueError( + "no file_id returned for PDF upload " + f"{attachment.get('path', 'unknown')!r}" + ) + # ``file_id`` only. Sending ``filename`` alongside it is + # rejected: 400 "Unknown parameter: …file.filename". prepared_messages[user_message_index]["content"].append( {"type": "file", "file": {"file_id": file_id}} ) except Exception as e: logging.error(f"Error uploading PDF to OpenAI: {e}", exc_info=True) - if "content" in attachment: + # Truthy, not membership — ``content`` is always a key on a + # PG-backed attachment and is "" when extraction produced + # nothing, which would otherwise send an empty "File + # content:" block and read as a successfully-read document. + if attachment.get("content"): prepared_messages[user_message_index]["content"].append( { "type": "text", - "text": f"File content:\n\n{attachment['content']}", + "text": ( + "File content:\n\n" + f"{self._fit_attachment_text(attachment['content'])}" + ), + } + ) + else: + # Nothing to fall back on: say so, naming the user's own + # file so the answer can't invent a generic excuse. + filename = ( + attachment.get("filename") + or os.path.basename(attachment.get("path") or "") + or "the attached file" + ) + prepared_messages[user_message_index]["content"].append( + { + "type": "text", + "text": f"[File '{filename}' could not be processed]", } ) else: @@ -1689,6 +1723,75 @@ class OpenAILLM(BaseLLM): except FileNotFoundError: raise FileNotFoundError(f"File not found: {file_path}") + def _fit_attachment_text(self, content: str) -> str: + """Bound inlined attachment text to a share of the model's window. + + ``_enforce_context_window`` runs in ``_llm_gen``, *before* attachments + are merged into the messages here, so nothing downstream trims this. + A 200-page PDF on an endpoint without a Files API would otherwise be + appended whole and get the whole request rejected for length — a worse + outcome than the degrade note it replaces. + """ + from application.core.model_utils import get_token_limit + from application.utils import num_tokens_from_string + + try: + limit = get_token_limit(self.model_id) if self.model_id else 0 + except Exception: + limit = 0 + # Half the window: the prompt, history and the answer share the rest. + budget = int(limit * 0.5) if limit else 24000 + try: + if num_tokens_from_string(content) <= budget: + return content + chars_per_token = len(content) / max(num_tokens_from_string(content), 1) + keep = max(int(budget * chars_per_token * 0.95), 0) + except Exception: + keep = budget * 4 + if keep <= 0 or keep >= len(content): + return content + return ( + f"{content[:keep]}\n\n[Content truncated: the file is larger than " + "this model's context window.]" + ) + + def _endpoint_scope(self) -> str: + """Short fingerprint of ``(provider, base_url, api_key)``. + + A Files-API ``file_id`` is only meaningful to the endpoint and + credential it was uploaded to — the same reasoning the Redis inline + cache already applies in ``_inline_file_id_cache_key``. + """ + creds = "\0".join( + ( + self.provider_name or "", + self._effective_base_url or "", + self.api_key or "", + ) + ) + return hashlib.sha256(creds.encode("utf-8")).hexdigest()[:16] + + def _stamp_file_id(self, file_id: str) -> str: + """Tag a file_id with the endpoint it belongs to before persisting.""" + return f"{self._endpoint_scope()}:{file_id}" + + def _scoped_file_id(self, stored): + """Return a persisted file_id only if this endpoint can resolve it. + + ``attachments.openai_file_id`` is a single global column, so without a + scope an id minted against one deployment would be replayed against + another, which answers ``No such File object`` on every retry — + permanently, since the row keeps the bad id. A miss re-uploads and + overwrites, so this self-heals both cross-endpoint reuse and ids that + aged out of provider retention. Legacy unscoped values are ignored. + """ + if not stored or not isinstance(stored, str): + return None + scope, _, file_id = stored.partition(":") + if not file_id or scope != self._endpoint_scope(): + return None + return file_id + def _upload_file_to_openai(self, attachment): """ Upload a file to OpenAI and return the file_id. @@ -1702,8 +1805,13 @@ class OpenAILLM(BaseLLM): Returns: str: OpenAI file_id for the uploaded file. """ - if "openai_file_id" in attachment: - return attachment["openai_file_id"] + # Truthy check, not membership: attachment dicts are built from + # ``SELECT *``, so ``openai_file_id`` is always a key and is NULL + # until an upload caches one. Testing membership treated every fresh + # attachment as a cache hit and returned None without uploading. + cached = self._scoped_file_id(attachment.get("openai_file_id")) + if cached: + return cached file_path = attachment.get("path") if not self.storage.file_exists(file_path): @@ -1740,7 +1848,7 @@ class OpenAILLM(BaseLLM): AttachmentsRepository(conn).update_any( str(attachment_id), user_id, - {"openai_file_id": file_id}, + {"openai_file_id": self._stamp_file_id(file_id)}, ) except Exception as cache_err: logging.warning( diff --git a/tests/llm/test_openai.py b/tests/llm/test_openai.py index 024ebe04..a58031ef 100644 --- a/tests/llm/test_openai.py +++ b/tests/llm/test_openai.py @@ -89,7 +89,11 @@ class FakeChatCompletions: class FakeFiles: + def __init__(self): + self.created = [] + def create(self, file=None, purpose=None): + self.created.append(purpose) return types.SimpleNamespace(id="file_id_uploaded") @@ -729,6 +733,149 @@ class TestPrepareMessagesWithAttachments: assert isinstance(user_msg["content"], list) +@pytest.mark.unit +class TestPdfAttachmentNullFileIdCache: + """A NULL ``openai_file_id`` column must not read as a cache hit. + + Attachment dicts come from ``SELECT *`` (``row_to_dict``), so every row + carries an ``openai_file_id`` key and it is NULL until an upload caches + one. Testing for key *membership* therefore treated every fresh + attachment as cached, returned ``None`` without uploading, and — because + nothing raised — skipped the fallback that inlines the extracted text. + The user was told their perfectly good PDF "could not be processed". + """ + + @pytest.fixture(autouse=True) + def _real_pdf_on_disk(self, tmp_path): + """The upload path opens the file, so it has to exist.""" + pdf = tmp_path / "contract.pdf" + pdf.write_bytes(b"%PDF-1.4\ntrailer<>\n") + self._pdf_path = str(pdf) + + def _pg_shaped_attachment(self, **overrides): + """What ``_attachment_to_dict`` actually produces for a fresh row.""" + attachment = { + "id": "att-1", + "filename": "contract.pdf", + "path": self._pdf_path, + "upload_path": self._pdf_path, + "mime_type": "application/pdf", + "size": 120_000, + "content": "REAL EXTRACTED TEXT", + "token_count": 6748, + "openai_file_id": None, + "google_file_uri": None, + } + attachment.update(overrides) + return attachment + + def test_null_cached_id_still_uploads(self, llm): + file_id = llm._upload_file_to_openai(self._pg_shaped_attachment()) + assert file_id, "a NULL cached id must not short-circuit the upload" + assert llm.client.files.created, "files.create was never called" + + def test_populated_cached_id_short_circuits(self, llm): + stamped = llm._stamp_file_id("file-cached") + attachment = self._pg_shaped_attachment(openai_file_id=stamped) + assert llm._upload_file_to_openai(attachment) == "file-cached" + assert not llm.client.files.created, "should not re-upload a cached file" + + def test_cached_id_from_another_endpoint_is_ignored(self, llm): + """A file_id only resolves at the endpoint that minted it. + + ``attachments.openai_file_id`` is one global column, so an id cached + against deployment A would otherwise be replayed against deployment B, + which answers "No such File object" on every retry — permanently, + since the row keeps the bad id. + """ + attachment = self._pg_shaped_attachment( + openai_file_id="0123456789abcdef:file-from-elsewhere" + ) + file_id = llm._upload_file_to_openai(attachment) + assert file_id == "file_id_uploaded" + assert llm.client.files.created, "a foreign-scope id must re-upload" + + def test_legacy_unscoped_cached_id_is_ignored(self, llm): + """Rows written before scoping carry a bare id; re-upload and restamp.""" + attachment = self._pg_shaped_attachment(openai_file_id="file-legacy") + assert llm._upload_file_to_openai(attachment) == "file_id_uploaded" + assert llm.client.files.created + + def test_oversized_content_is_truncated_before_inlining(self, llm): + """Attachments merge in *after* the context-window check, so nothing + downstream trims this — a big PDF would blow the whole request.""" + llm._upload_file_to_openai = lambda _attachment: None + llm.model_id = None # exercise the no-registry default budget + huge = "word " * 200_000 + attachment = self._pg_shaped_attachment(content=huge) + + result = llm.prepare_messages_with_attachments( + [{"role": "user", "content": "summarize"}], [attachment] + ) + text = next( + p["text"] + for p in result[-1]["content"] + if p.get("type") == "text" and p["text"].startswith("File content:") + ) + assert len(text) < len(huge) + assert "Content truncated" in text + + def test_extracted_text_reaches_the_model_when_upload_yields_no_id(self, llm): + """The production failure: no id, no exception, no text, wrong answer.""" + llm._upload_file_to_openai = lambda _attachment: None + + msgs = [{"role": "user", "content": "summarize the attached pdf"}] + result = llm.prepare_messages_with_attachments( + msgs, [self._pg_shaped_attachment()] + ) + + user_msg = next(m for m in result if m["role"] == "user") + assert not any( + p.get("type") == "file" and not p.get("file", {}).get("file_id") + for p in user_msg["content"] + ), "must not emit a file part with an empty file_id" + assert any( + "REAL EXTRACTED TEXT" in p.get("text", "") + for p in user_msg["content"] + if p.get("type") == "text" + ), "extracted content must be inlined rather than dropped" + + def test_degrade_note_names_the_real_file(self, llm): + """Fall back to the note only with no text, and never say 'upload.pdf'.""" + llm._upload_file_to_openai = lambda _attachment: None + attachment = self._pg_shaped_attachment(content="") + + msgs = [{"role": "user", "content": "summarize"}] + result = llm.prepare_messages_with_attachments(msgs, [attachment]) + + user_msg = next(m for m in result if m["role"] == "user") + notes = [ + p["text"] + for p in user_msg["content"] + if p.get("type") == "text" and "could not be processed" in p.get("text", "") + ] + assert len(notes) == 1 + assert "contract.pdf" in notes[0] + assert "upload.pdf" not in notes[0] + + def test_file_part_carries_only_file_id(self, llm): + """Never send ``filename`` next to ``file_id``. + + Verified against the Azure Foundry deployment: a file part carrying + both is rejected outright — + ``400 Unknown parameter: 'messages[0].content[1].file.filename'``. + The upload itself and a ``file_id``-only part both return 200 and the + model reads the PDF correctly, so the reference is all we may send. + """ + msgs = [{"role": "user", "content": "summarize"}] + result = llm.prepare_messages_with_attachments( + msgs, [self._pg_shaped_attachment()] + ) + user_msg = next(m for m in result if m["role"] == "user") + file_part = next(p for p in user_msg["content"] if p.get("type") == "file") + assert file_part["file"] == {"file_id": "file_id_uploaded"} + + # _get_base64_image @@ -1048,8 +1195,15 @@ class TestPrepareMessagesWithAttachmentsAdditional: class TestUploadFileToOpenai: def test_cached_file_id_returned(self, llm): - """Cover line 469: cached openai_file_id.""" - result = llm._upload_file_to_openai({"openai_file_id": "cached_id"}) + """A cached openai_file_id short-circuits the upload. + + The stored value is endpoint-scoped: a bare id is a legacy row and is + deliberately treated as a miss (see + ``TestPdfAttachmentNullFileIdCache``). + """ + result = llm._upload_file_to_openai( + {"openai_file_id": llm._stamp_file_id("cached_id")} + ) assert result == "cached_id" def test_file_not_found_raises(self, llm): @@ -1156,8 +1310,10 @@ class TestOpenAILLMConstructor: class TestUploadFileToOpenai2: def test_returns_cached_file_id(self, llm): - """Cover line 491-492: returns cached openai_file_id.""" - result = llm._upload_file_to_openai({"openai_file_id": "file-123"}) + """Returns the cached openai_file_id when it belongs to this endpoint.""" + result = llm._upload_file_to_openai( + {"openai_file_id": llm._stamp_file_id("file-123")} + ) assert result == "file-123" def test_file_not_found_raises(self, llm): @@ -1386,7 +1542,9 @@ class TestUploadFileToOpenaiLine469: """Cover line 469: cached openai_file_id returned early.""" def test_cached_id_returned_immediately(self, llm): - result = llm._upload_file_to_openai({"openai_file_id": "file-cached-123"}) + result = llm._upload_file_to_openai( + {"openai_file_id": llm._stamp_file_id("file-cached-123")} + ) assert result == "file-cached-123" @@ -1529,9 +1687,9 @@ class TestUploadFileToOpenAIError: llm._upload_file_to_openai({"path": "/doc.pdf"}) def test_upload_cached_file_id(self, llm): - """Cover line 491-492: already has openai_file_id.""" + """Already has an endpoint-scoped openai_file_id.""" result = llm._upload_file_to_openai( - {"path": "/doc.pdf", "openai_file_id": "file-cached"} + {"path": "/doc.pdf", "openai_file_id": llm._stamp_file_id("file-cached")} ) assert result == "file-cached"