Fix MCP document full-text retrieval crash on cloud storage (bytes not JSON-serializable)

CHANGELOG.md

@@ -98,6 +98,53 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   bug pre-dated the Phase 6 service-layer refactor (it was not a regression).
   Regression test added: `test_smart_label_list_denies_unreadable_corpus`.
 
+### Fixed
+
+- **MCP document full-text retrieval crashed on cloud storage**
+  (`opencontractserver/mcp/tools.py::get_document_text`,
+  `opencontractserver/mcp/resources.py::get_document_resource`) — both read
+  `document.txt_extract_file` via `.open("r").read()` and placed the raw result
+  into the dict that the MCP dispatcher serializes with `json.dumps(...)`
+  (`opencontractserver/mcp/server.py:447` and `:1066`). On AWS/GCP deployments
+  django-storages backends (`S3Boto3Storage`, `GoogleCloudStorage`) return
+  `bytes` from text-mode reads (django-storages #382), so `json.dumps` raised
+  `TypeError: Object of type bytes is not JSON serializable` and downstream MCP
+  clients could not retrieve document full text. LOCAL `FileSystemStorage`
+  returns `str`, which is why dev and the test suite never reproduced it. Both
+  call sites now decode through the new `read_field_file_text()` helper
+  (`opencontractserver/utils/files.py`), which centralizes the
+  `isinstance(..., bytes)` decode workaround previously duplicated in
+  `tasks/embeddings_task.py` and `pipeline/parsers/oc_markdown_parser.py`. The
+  same routing also fixes a latent instance of this bug in the agent memory
+  reader's happy path
+  (`opencontractserver/agents/memory.py::read_memory_content`, which decoded
+  bytes only in its exception fallback, not its primary `.open("r").read()`
+  branch). Regression test:
+  `test_get_document_text_handles_bytes_from_cloud_storage`
+  (`opencontractserver/mcp/tests/test_mcp.py`); helper unit tests:
+  `opencontractserver/tests/test_files_utils.py`.
+- **Routed the remaining seven production `txt_extract_file.open("r")` call
+  sites through `read_field_file_text()`** so they no longer break on cloud
+  storage the same way: `pipeline/base/thumbnailer.py`, `utils/export_v2.py`,
+  `utils/extraction_grounding.py`, and the four LLM core tools
+  (`llms/tools/core_tools/{pii,document_indexing,search,annotations}.py`). The
+  LLM tools are the highest-impact because they feed `doc_text` directly into
+  agent context, where a raw `bytes` value would crash the downstream call or
+  produce garbage. The two MCP read sites and the four LLM core tools now pass
+  `errors="replace"` so a few undecodable bytes substitute `U+FFFD` instead of
+  raising `UnicodeDecodeError`: at the MCP sites it would otherwise be caught
+  by the surrounding `except` and silently return an empty document; in the
+  agent tools it keeps the tool fault-tolerant (per the CLAUDE.md agent
+  tool-fault-tolerance rule) and stays internally consistent because match /
+  annotation positions are computed against the same decoded string. The
+  pipeline/export sites (`thumbnailer`, `export_v2`, `extraction_grounding`)
+  retain strict decoding to fail fast on genuinely corrupt files. Regression
+  test for the resource path:
+  `test_get_document_resource_handles_bytes_from_cloud_storage`
+  (`opencontractserver/mcp/tests/test_mcp.py`). `read_field_file_text()`'s
+  parameter is typed as a structural `Protocol` rather than the concrete
+  `FieldFile`, so duck-typed test doubles type-check without `# type: ignore`.
+
 ### Changed
 
 - **`BaseService.filter_visible_qs` now fails closed** (`opencontractserver/shared/services/base.py`)

opencontractserver/agents/memory.py

@@ -29,6 +29,7 @@
 )
 from opencontractserver.constants.document_processing import MARKDOWN_MIME_TYPE
 from opencontractserver.llms.context_guardrails import estimate_token_count
+from opencontractserver.utils.files import read_field_file_text
 
 if TYPE_CHECKING:
     from opencontractserver.corpuses.models import Corpus
@@ -172,20 +173,13 @@ def _read() -> str:
         if doc is None or not doc.txt_extract_file:
             return ""
         try:
-            with doc.txt_extract_file.open("r") as f:
-                return f.read()
+            return read_field_file_text(doc.txt_extract_file, errors="ignore")
         except Exception:
-            try:
-                content = doc.txt_extract_file.read()
-                if isinstance(content, bytes):
-                    return content.decode("utf-8", errors="ignore")
-                return content
-            except Exception:
-                logger.warning(
-                    "Failed to read memory document for corpus %s",
-                    corpus.id,
-                )
-                return ""
+            logger.warning(
+                "Failed to read memory document for corpus %s",
+                corpus.id,
+            )
+            return ""
 
     return await database_sync_to_async(_read)()
 

opencontractserver/llms/tools/core_tools/annotations.py

@@ -6,6 +6,7 @@
 from typing_extensions import TypedDict
 
 from opencontractserver.utils.compact_pawls import expand_pawls_pages
+from opencontractserver.utils.files import read_field_file_text
 
 from ._helpers import _db_sync_to_async
 
@@ -279,8 +280,11 @@ def _create_annotation(pos: int, end_idx: int, label_obj):
             raise ValueError(
                 f"Text document id={doc_id} lacks txt_extract_file; cannot annotate."
             )
-        with doc.txt_extract_file.open("r") as f:
-            doc_text = f.read()
+        # errors="replace" keeps this agent tool fault-tolerant: a few
+        # undecodable bytes substitute U+FFFD rather than raising
+        # UnicodeDecodeError. Positions are computed against this same string,
+        # so the substitution stays internally consistent.
+        doc_text = read_field_file_text(doc.txt_extract_file, errors="replace")
 
         label_type_const = SPAN_LABEL
 

opencontractserver/llms/tools/core_tools/document_indexing.py

@@ -6,6 +6,7 @@
 from typing_extensions import NotRequired, TypedDict
 
 from opencontractserver.utils.compact_pawls import expand_pawls_pages
+from opencontractserver.utils.files import read_field_file_text
 
 from ._helpers import _db_sync_to_async
 
@@ -151,8 +152,11 @@ def _make_annotation(pos, end_idx, label_obj, title, description):
                 f"Text document id={document_id} lacks txt_extract_file; "
                 "cannot create index."
             )
-        with doc.txt_extract_file.open("r") as f:
-            doc_text = f.read()
+        # errors="replace" keeps this agent tool fault-tolerant: a few
+        # undecodable bytes substitute U+FFFD rather than raising
+        # UnicodeDecodeError. Positions are computed against this same string,
+        # so the substitution stays internally consistent.
+        doc_text = read_field_file_text(doc.txt_extract_file, errors="replace")
 
         label_type_const = SPAN_LABEL
 

opencontractserver/llms/tools/core_tools/pii.py

@@ -31,6 +31,7 @@
 from opencontractserver.tasks.embeddings_task import (
     calculate_embedding_for_annotation_text,
 )
+from opencontractserver.utils.files import read_field_file_text
 
 from ._helpers import _db_sync_to_async
 from ._privacy_filter_client import Detection, adetect_pii
@@ -107,8 +108,10 @@ def _load_doc_text_sync(document_id: int, corpus_id: int) -> _DocTextResult:
     if file_type in TEXT_MIMETYPES:
         if not doc.txt_extract_file:
             raise ValueError(f"Text document id={document_id} lacks txt_extract_file.")
-        with doc.txt_extract_file.open("r") as f:
-            doc_text = f.read()
+        # errors="replace" keeps this agent tool fault-tolerant: a few
+        # undecodable bytes substitute U+FFFD rather than raising
+        # UnicodeDecodeError on read.
+        doc_text = read_field_file_text(doc.txt_extract_file, errors="replace")
         return _DocTextResult(doc, corpus, doc_text, file_type, None)
 
     if file_type == "application/pdf":

opencontractserver/llms/tools/core_tools/search.py

@@ -5,6 +5,7 @@
 
 from opencontractserver.documents.models import Document
 from opencontractserver.utils.compact_pawls import expand_pawls_pages
+from opencontractserver.utils.files import read_field_file_text
 
 from ._helpers import _db_sync_to_async
 
@@ -137,8 +138,11 @@ def search_exact_text_as_sources(
                 f"Text document id={document_id} lacks txt_extract_file; cannot search."
             )
 
-        with doc.txt_extract_file.open("r") as f:
-            doc_text = f.read()
+        # errors="replace" keeps this agent tool fault-tolerant: a few
+        # undecodable bytes substitute U+FFFD rather than raising
+        # UnicodeDecodeError. Match positions are computed against this same
+        # string, so the substitution stays internally consistent.
+        doc_text = read_field_file_text(doc.txt_extract_file, errors="replace")
 
         # Find all matches for each search string
         for search_str in search_strings:

opencontractserver/llms/tools/core_tools/text_extracts.py

@@ -3,6 +3,8 @@
 import logging
 from datetime import datetime  # noqa: F401  (used in type comment below)
 
+from opencontractserver.utils.files import read_field_file_text
+
 logger = logging.getLogger(__name__)
 
 # --------------------------------------------------------------------------- #
@@ -101,9 +103,12 @@ def load_document_txt_extract(
         use_cache = cached_ts == doc.modified
 
     if not use_cache:
-        # (Re)load from storage.
-        content_bytes = doc.txt_extract_file.read()
-        content_str = content_bytes.decode("utf-8")
+        # Cache miss — reload from storage; helper normalizes bytes-returning
+        # backends. ``errors="replace"`` is intentional: this is an
+        # agent-facing read, so per the CLAUDE.md fault-tolerance convention we
+        # substitute U+FFFD for undecodable bytes rather than crash the tool
+        # call (pipeline/export paths use strict mode instead).
+        content_str = read_field_file_text(doc.txt_extract_file, errors="replace")
         _DOC_TXT_CACHE[document_id] = (doc.modified, content_str)
 
         logger.debug(
@@ -161,7 +166,9 @@ async def aload_document_txt_extract(
         use_cache = cached_ts == doc.modified
 
     if not use_cache:
-        content_str = doc.txt_extract_file.read().decode("utf-8")
+        # Agent-facing read: ``errors="replace"`` is intentional (fault-tolerant
+        # per CLAUDE.md) — see the sibling loader above for the full rationale.
+        content_str = read_field_file_text(doc.txt_extract_file, errors="replace")
         _DOC_TXT_CACHE[document_id] = (doc.modified, content_str)
 
         logger.debug(

opencontractserver/mcp/resources.py

@@ -10,6 +10,8 @@
 
 from django.contrib.auth.models import AnonymousUser
 
+from opencontractserver.utils.files import read_field_file_text
+
 if TYPE_CHECKING:
     from opencontractserver.users.types import UserOrAnonymous
 
@@ -87,8 +89,12 @@ def get_document_resource(
     full_text = ""
     if document.txt_extract_file:
         try:
-            with document.txt_extract_file.open("r") as f:
-                full_text = f.read()
+            # errors="replace" so a few undecodable bytes substitute U+FFFD
+            # rather than raising UnicodeDecodeError and silently yielding an
+            # empty document to the client.
+            full_text = read_field_file_text(
+                document.txt_extract_file, errors="replace"
+            )
         except Exception:
             full_text = ""
 

opencontractserver/mcp/tests/test_mcp.py

@@ -2,6 +2,7 @@
 
 import asyncio
 import json
+from unittest import mock
 
 import pytest
 from django.contrib.auth import get_user_model
@@ -13,6 +14,42 @@
 User = get_user_model()
 
 
+class _BytesFieldFileHandle:
+    """Context-manager file handle whose ``read()`` returns ``bytes``.
+
+    Simulates cloud storage backends (S3Boto3Storage / GoogleCloudStorage via
+    django-storages #382) which return ``bytes`` from ``FieldFile.open("r")``
+    even in text mode — the path that ``read_field_file_text`` normalizes and
+    that local ``FileSystemStorage`` never exercises.
+    """
+
+    def __init__(self, payload: str):
+        self._payload = payload.encode("utf-8")
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *exc):
+        return False
+
+    def read(self):
+        return self._payload
+
+
+def _patch_fieldfile_open_returns_bytes(payload: str):
+    """Patch ``FieldFile.open`` so each call yields a fresh bytes-returning handle.
+
+    Patches the *class* (not an instance) because ``FieldFile.open`` is the
+    only reliable interception point; a fresh handle per call keeps each
+    ``open()`` reader unconsumed.
+    """
+    from django.db.models.fields.files import FieldFile
+
+    return mock.patch.object(
+        FieldFile, "open", side_effect=lambda *a, **k: _BytesFieldFileHandle(payload)
+    )
+
+
 class _MCPAsyncRunMixin:
     """Shared helper for running an async coroutine inside a sync test method.
 
@@ -616,6 +653,51 @@ def test_get_document_text_nonexistent(self):
         with self.assertRaises(Document.DoesNotExist):
             get_document_text(self.corpus.slug, "nonexistent-doc")
 
+    def test_get_document_text_handles_bytes_from_cloud_storage(self):
+        """Regression: cloud backends (S3/GCS) return bytes from ``.open("r")``
+        even in text mode (django-storages #382).
+
+        The tool must normalize to ``str`` so the dispatcher's
+        ``json.dumps(result)`` does not raise ``TypeError: Object of type
+        bytes is not JSON serializable`` — the failure reported by downstream
+        MCP clients on cloud-storage deployments. Local FileSystemStorage
+        returns ``str``, so without this simulation the suite never exercises
+        the bytes path.
+        """
+        from opencontractserver.mcp.tools import get_document_text
+
+        payload = "Bytes-backed extracted text ✓"
+
+        with _patch_fieldfile_open_returns_bytes(payload):
+            result = get_document_text(self.corpus.slug, self.doc1.slug)
+
+        # Decoded to str ...
+        self.assertIsInstance(result["text"], str)
+        self.assertEqual(result["text"], payload)
+        # ... and the full payload serializes cleanly (the original crash).
+        json.dumps(result, indent=2)
+
+    def test_get_document_resource_handles_bytes_from_cloud_storage(self):
+        """Regression: the resource path shares the same bytes-from-cloud bug
+        class as ``get_document_text`` (django-storages #382).
+
+        ``get_document_resource`` itself returns a JSON string, so the latent
+        failure is the same ``read_field_file_text`` decode rather than the
+        dispatcher serialization. This pins the resource path independently so
+        the two surfaces can't drift.
+        """
+        from opencontractserver.mcp.resources import get_document_resource
+
+        payload = "Resource bytes-backed extracted text ✓"
+
+        with _patch_fieldfile_open_returns_bytes(payload):
+            result = get_document_resource(self.corpus.slug, self.doc1.slug)
+
+        data = json.loads(result)
+        self.assertIsInstance(data["full_text"], str)
+        self.assertEqual(data["full_text"], payload)
+        self.assertEqual(data["text_preview"], payload[:500])
+
 
 class MCPToolsAnnotationsTest(TestCase):
     """Tests for MCP annotation-related tools."""

opencontractserver/mcp/tools.py

@@ -14,6 +14,7 @@
 from django.db.models import Count, Q
 
 from opencontractserver.constants.mcp import MAX_THREAD_MESSAGE_LENGTH
+from opencontractserver.utils.files import read_field_file_text
 
 from .formatters import (
     format_annotation,
@@ -148,8 +149,12 @@ def get_document_text(
     full_text = ""
     if document.txt_extract_file:
         try:
-            with document.txt_extract_file.open("r") as f:
-                full_text = f.read()
+            # errors="replace" so a few undecodable bytes substitute U+FFFD
+            # rather than raising UnicodeDecodeError and silently yielding an
+            # empty document to the client.
+            full_text = read_field_file_text(
+                document.txt_extract_file, errors="replace"
+            )
         except Exception:
             full_text = ""
 

opencontractserver/pipeline/base/thumbnailer.py

@@ -5,6 +5,7 @@
 from django.core.files.base import File
 
 from opencontractserver.pipeline.base.file_types import FileTypeEnum
+from opencontractserver.utils.files import read_field_file_text
 
 from .base_component import PipelineComponentBase
 
@@ -70,8 +71,7 @@ def generate_thumbnail(self, doc_id: str | int, **kwargs) -> Optional[File]:
 
             # Load the txt file content if available
             if document.txt_extract_file:
-                with document.txt_extract_file.open("r") as txt_file:
-                    txt_content = txt_file.read()
+                txt_content = read_field_file_text(document.txt_extract_file)
 
             # Load the pdf file bytes if available
             if document.pdf_file: