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

* Fix MCP document full-text retrieval crash on cloud storage backends

get_document_text and the document MCP resource read txt_extract_file via
.open("r").read() and forwarded the result straight into json.dumps. On
S3/GCS (django-storages), text-mode reads return bytes, 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.

Add a shared read_field_file_text() helper in utils/files.py that decodes bytes
to str when a backend ignores text mode, and route both MCP read sites
(tools.get_document_text, resources.get_document_resource) through it. Also
consolidate the duplicated isinstance(..., bytes) decode workaround in
tasks/embeddings_task.py and pipeline/parsers/oc_markdown_parser.py onto the
helper, and fix a latent instance of the same bug in
agents/memory.read_memory_content (which decoded bytes only in its exception
fallback, not its primary .open("r").read() branch).

Add a regression test that simulates a bytes-returning backend through
get_document_text and asserts the result is JSON-serializable, plus direct
unit tests for the helper.

* Fix linter (mypy/pyupgrade) and route remaining txt_extract_file reads through helper

- Type read_field_file_text() param as a structural Protocol instead of the
  concrete FieldFile, so the duck-typed _FakeFieldFile test double type-checks
  without # type: ignore (fixes the 4 mypy arg-type errors).
- pyupgrade: modernize .encode("utf-8") -> .encode() in test_files_utils.py.
- Address review: route the 7 remaining production txt_extract_file.open("r")
  call sites through read_field_file_text() (thumbnailer, export_v2,
  extraction_grounding, and the pii/document_indexing/search/annotations LLM
  core tools) so they no longer break on cloud storage.
- Address review: MCP get_document_text / get_document_resource now decode with
  errors="replace" so a few undecodable bytes don't silently yield an empty
  document.

* Fix stale test expectations after MCP cloud-storage read refactor

test_markdown_parser: parser now reads via read_field_file_text (which
calls txt_extract_file.open("r")), not default_storage; the bytes/str
storage simulation now patches FieldFile.open instead of the removed
default_storage import.

test_agent_memory: read_memory_content intentionally collapsed its
two-level open()->.read() fallback; an open() failure now logs a warning
and returns "". Updated the two pre-existing fallback assertions to the
new contract (the bytes-vs-str normalization lives in the helper's
primary path).

* Address review: trim helper docstrings, fresh open handle, route remaining txt reads

- read_field_file_text + Protocol docstrings trimmed to one line (bug
  context belongs in git log).
- MCP regression test patches FieldFile.open with side_effect so each
  open() yields a fresh _BytesHandle instead of a shared, once-consumable
  return_value.
- Route the two text_extracts.py txt_extract_file.read().decode() sites
  through read_field_file_text(errors="replace") so undecodable bytes
  substitute U+FFFD rather than crashing the LLM read path.

* Address review: errors=replace on LLM tool reads, resources bytes test, docstring + test-name fixes

- Route the four agent-facing LLM core tools (annotations, document_indexing,
  pii, search) through read_field_file_text with errors="replace" so a few
  undecodable bytes can't raise UnicodeDecodeError in an agent tool (CLAUDE.md
  fault-tolerance rule); positions stay internally consistent.
- Add get_document_resource bytes-from-cloud regression test mirroring the
  existing get_document_text test (closes the resources.py coverage gap).
- Expand the terse read_field_file_text docstring with the django-storages
  rationale and errors-policy guidance.
- Rename test_both_read_paths_fail_returns_empty ->
  test_open_failure_returns_empty_when_read_would_also_fail to match the
  single-path read after the two-level fallback was removed.

* Address review: dedup _BytesHandle test helper, document errors='replace' rationale

- Extract the duplicated _BytesHandle inner class from the two MCP cloud-storage
  bytes regression tests into a single module-level _BytesFieldFileHandle +
  _patch_fieldfile_open_returns_bytes helper.
- Document that text_extracts.py's errors='replace' is intentional (agent-facing,
  fault-tolerant per CLAUDE.md; pipeline/export paths stay strict).
- Tighten _TextReadableFieldFile.open return type to ContextManager[IO[Any]].
- Explain the FieldFile class-level (not instance) open() patch in test_markdown_parser.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: JSv4

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 = ""