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

[JSv4]

Problem

A downstream MCP client reported being unable to retrieve document full text, with an error about bytes not being convertible.

get_document_text (opencontractserver/mcp/tools.py) and the document MCP resource (opencontractserver/mcp/resources.py) read document.txt_extract_file via .open("r").read() and place the result straight 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 even though "r" was requested (django-storages #382). json.dumps then raises TypeError: Object of type bytes is not JSON serializable, the tool/resource call fails, and the client cannot retrieve text.

LOCAL FileSystemStorage (dev + the test suite) returns str, which is why this was never reproduced locally. The codebase already worked around this exact issue elsewhere (e.g. tasks/embeddings_task.py, citing the same django-storages issue) — the MCP module simply lacked the guard. The try/except around the read did not help: .read() returns the bytes without raising, so the TypeError fires later in server.py, outside the guard.

On the "annotation text" half of the report: list_annotations / format_annotation and the annotation resource never read a FileField (they return raw_text, a TextField, and the already-JSON-native json field), so they cannot emit bytes. The bytes failure is specific to the document full-text path; a client seeing it for "annotations" was most likely pulling text through the document path.

Fix

• New shared helper read_field_file_text() in opencontractserver/utils/files.py — reads a FieldFile as text and decodes bytes → str when a backend ignores text mode (single source of truth for the workaround).
• Route both MCP read sites through it (the actual bug fix).
• DRY consolidation: the previously duplicated isinstance(..., bytes) decode in tasks/embeddings_task.py and pipeline/parsers/oc_markdown_parser.py now uses the helper (the latter's now-unused default_storage import is removed).
• Latent same-class bug fixed in agents/memory.read_memory_content, whose primary .open("r").read() branch returned bytes unchanged (it only decoded in the exception fallback).

Per-site decode semantics are preserved: strict for embeddings/markdown, errors="ignore" for memory. The MCP sites' existing try/except now degrades an undecodable file to "" instead of crashing.

Testing

• opencontractserver/tests/test_files_utils.py — unit tests for the helper (bytes→str, str passthrough, errors policy, strict-raises).
• test_get_document_text_handles_bytes_from_cloud_storage in opencontractserver/mcp/tests/test_mcp.py — patches FieldFile.open to return bytes (simulating S3/GCS) and asserts the tool output is str and json.dumps-able. Local FileSystemStorage returns str, so this simulation is required to exercise the bytes path.

Verified locally: python -m py_compile on all touched files, black (clean), flake8 (clean), and a standalone run of the helper's decode logic. The Dockerized suite could not be run in this ephemeral container (no Docker daemon / app deps absent); please run the targeted tests in CI:

docker compose -f test.yml run --rm django pytest \
  opencontractserver/tests/test_files_utils.py \
  opencontractserver/mcp/tests/test_mcp.py -k "document_text or ReadFieldFileText" -q

---

Generated by Claude Code

[claude]

Code Review

Overview

This PR fixes a real production crash: json.dumps raising TypeError: Object of type bytes is not JSON serializable on cloud storage deployments (S3/GCS), because django-storages backends return bytes from text-mode open("r") calls despite the mode argument (django-storages #382). The root cause analysis is accurate and well-documented. The core fix — a shared read_field_file_text() helper in opencontractserver/utils/files.py — is exactly the right architectural move.

---

What's Good

• Correct and minimal fix. The helper normalises bytes → str in one place, and the five call sites that already used the inline workaround (embeddings, markdown parser) or had the latent bug (MCP tools/resources, agent memory) are now routed through it. DRY win.
• Helper design is solid. encoding and errors as keyword-only args with sensible defaults ("utf-8", "strict") give callers explicit control without exposing footguns. The errors="ignore" choice for agent memory (where partial text is better than nothing) vs. strict at MCP sites (caller try/except degrades to "") reflects thoughtful per-site semantics.
• Test coverage is appropriate. ReadFieldFileTextTests covers the four meaningful code paths (bytes-passthrough, str-passthrough, errors="ignore" policy, strict-raises). The _BytesHandle / _FakeFieldFile fakes are clean and scoped to the test module. The MCP regression test correctly simulates the cloud-storage bytes path by patching FieldFile.open at the class level.
• CHANGELOG entry is detailed — includes file paths, line numbers, downstream impact, and links to the upstream issue. That level of detail is exactly what this project asks for.
• default_storage import removal from oc_markdown_parser.py is a clean-up that was overdue.

---

Issues

1. Incomplete audit — 7 other call sites have the identical latent bug (high priority)

The same txt_extract_file.open("r") … f.read() pattern survives in:

File	Line
opencontractserver/utils/extraction_grounding.py	153–154
opencontractserver/pipeline/base/thumbnailer.py	73–74
opencontractserver/utils/export_v2.py	81–82
opencontractserver/llms/tools/core_tools/document_indexing.py	154–155
opencontractserver/llms/tools/core_tools/annotations.py	282–283
opencontractserver/llms/tools/core_tools/search.py	140–141
opencontractserver/llms/tools/core_tools/pii.py	110–111

All seven will fail the same way on S3/GCS deployments. The LLM core tools (document_indexing, annotations, search, pii) are particularly high-impact because they feed doc_text directly into LLM context — a bytes object there will either crash the downstream call or silently produce garbage. These should be routed through read_field_file_text() in this same PR; it's a one-liner replacement at each site and the helper is already available.

A quick way to guard against future regressions once these are fixed:

grep -rn 'txt_extract_file\.open("r")' opencontractserver/ --include="*.py" | grep -v read_field_file_text

This should produce zero output after a complete fix.

2. Semantic regression in memory.py — two-level fallback collapsed into one

Before:

try:
    with doc.txt_extract_file.open("r") as f:
        return f.read()          # primary path (FileSystemStorage)
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(...)
        return ""

After:

try:
    return read_field_file_text(doc.txt_extract_file, errors="ignore")
except Exception:
    logger.warning(...)
    return ""

The new code is cleaner and the primary path (the with block in the helper) correctly handles bytes. However, the outer except in the original caught failures from open() itself (e.g. missing file, network error) and then retried with .read() — the retry may have rescued transient failures. With the new code, any exception from open() or read() goes straight to the warning/return-"". This is almost certainly fine for memory content (empty is safe), but the PR description doesn't call out that the retry safety net was intentionally removed. Worth a comment in the PR or a note that the retry was vestigial.

3. Strict default errors="strict" at MCP sites will silently degrade to ""

get_document_text and get_document_resource use read_field_file_text(document.txt_extract_file) — no errors= argument, so errors="strict". On S3/GCS, if the file contains any non-UTF-8 bytes (Latin-1 content, a corrupted file, etc.), read_field_file_text will raise UnicodeDecodeError, which the outer except Exception catches, and full_text becomes "". The client gets an empty document with no indication of why.

errors="replace" would preserve the rest of the text, substituting U+FFFD for undecodable bytes — arguably safer than silent empty string. This is a judgement call, but worth a conscious decision (and a comment explaining it) rather than an accidental consequence of the default.

---

Minor Points

• _BytesHandle.__exit__: return False is correct for a context manager that doesn't suppress exceptions. No issue, just confirming.
• _FakeFieldFile doesn't inherit FieldFile: the function's type annotation says FieldFile but the implementation relies on duck typing. The tests work because of duck typing, but a typing.Protocol or typing.Union might be more accurate. Not a bug, purely cosmetic.
• PR description note: "The Dockerized suite could not be run in this ephemeral container" — CI will catch this, but it means the regression test hasn't been run against real Django/ORM yet. Keep an eye on the CI run.

---

Summary

The fix is correct, well-motivated, and a genuine improvement. The two blockers before merging are:

1. Route the 7 remaining txt_extract_file.open("r") call sites through read_field_file_text() — they have the same bug and the helper is already there.
2. Consider errors="replace" (or explicit errors="ignore") at the MCP sites to avoid silently returning an empty document for files with a few bad bytes.

Everything else is polish.

[claude]

Code Review

Overall: This is a clean, well-scoped bug fix. The root cause analysis is correct, the helper design is solid, and the DRY consolidation is the right move. A few items worth addressing before merge.

---

What this does

Introduces read_field_file_text() in opencontractserver/utils/files.py as a single canonical function for reading a FieldFile as text, normalizing the bytes-vs-str disparity across django-storages backends. Routes all 11 affected call sites through it, eliminating the duplicated isinstance(raw, bytes) decode guards that had accumulated across the codebase.

---

Issues

1. Docstring length in read_field_file_text violates project convention

CLAUDE.md is explicit: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The new helper has a 12-line docstring block. The bug context (django-storages #382) belongs in the git log / PR description, not in every reader's working memory via a doc comment. Trim to one line, e.g.:

def read_field_file_text(field_file, *, encoding="utf-8", errors="strict") -> str:
    """Read a FieldFile as str, decoding bytes returned by cloud-storage backends."""

2. Default errors="strict" on LLM tool call sites is a latent regression

annotations.py, document_indexing.py, pii.py, search.py, thumbnailer.py, export_v2.py, and extraction_grounding.py all call read_field_file_text(doc.txt_extract_file) with the default strict policy.

Before this PR, those sites called with file.open("r") as f: f.read(). On a cloud backend that returns bytes, the raw bytes object would propagate (wrong, but silent — the caller would crash later or produce garbage). After this PR, the same scenario raises UnicodeDecodeError immediately inside the helper — which is arguably more correct, but it changes the exception type and call site of the failure.

The concern: if any document on a production cloud deployment has a file with a stray invalid byte (e.g. a Latin-1 byte in what was meant to be UTF-8), these call sites will now raise where previously they would silently continue. For the LLM tool paths (annotations.py, pii.py, etc.) the surrounding caller typically has a broad except that converts errors to a string returned to the LLM — so the user impact is bounded. Still worth confirming the callers' exception handling covers UnicodeDecodeError or, if the intent is to be lenient, consider errors="replace" for the LLM tool paths (consistent with the MCP choice).

3. _BytesHandle in the regression test creates one shared instance

with mock.patch.object(FieldFile, "open", return_value=_BytesHandle()):

return_value passes a single _BytesHandle() instance that is reused for every call to open(). This works here since the test makes one call, but if get_document_text ever opens the file more than once the context-manager state would be shared. Prefer side_effect to create a fresh handle each call:

with mock.patch.object(FieldFile, "open", side_effect=lambda *a, **k: _BytesHandle()):

---

Minor / stylistic

• The inline comments added in tools.py and resources.py explaining errors="replace" / U+FFFD are justified (the why is non-obvious), so they satisfy the project rule. No change needed there.
• The removal of the now-unused default_storage import in oc_markdown_parser.py is correct and welcome.
• The Protocol _TextReadableFieldFile is a nice touch — lets test doubles satisfy the type without # type: ignore. The mode: str = ... ellipsis-default syntax is valid for Protocol stubs.
• The memory.py simplification (collapsing the nested try/except) is correct: the helper handles the bytes case, so the fallback chain is no longer needed.

---

Test coverage

The unit tests in test_files_utils.py cover the four key scenarios (bytes decode, str passthrough, errors="ignore", strict raise) — good. The regression test in test_mcp.py exercises the specific crash path. The seven other routing changes (LLM tools, export, etc.) aren't directly tested but are all mechanically identical to the MCP paths, and the helper unit tests cover the shared behavior. Acceptable.

---

Summary

Fix the docstring length and decide on error policy for the LLM tool sites (document that the strict behaviour is intentional, or switch to errors="replace"). The side_effect mock cleanup is low priority but clean. Otherwise this is a solid, mergeable fix.

[claude]

Code Review

Overview

This PR fixes a real production bug: django-storages cloud backends (S3Boto3Storage, GoogleCloudStorage) return bytes even from text-mode open("r") reads, causing json.dumps to raise TypeError: Object of type bytes is not JSON serializable in MCP tool/resource responses. The fix centralizes the decode workaround in a single helper and applies it consistently across 13 call sites that all had the same latent or active bug.

The root-cause diagnosis is accurate, the PR description is thorough, and the CHANGELOG is well-written. Most of the review comments below are minor, but there are a couple of points worth discussing before merge.

---

Strengths

• Correct fix, applied systematically. The helper pattern is the right approach. The pre-PR codebase had at least three independent copies of isinstance(content, bytes) decode logic, all citing the same django-storages issue number. One canonical location prevents future drift.
• Protocol-based typing is clean. _TextReadableFieldFile lets test doubles satisfy the type checker without # type: ignore. The underscore prefix correctly signals it is private to the module.
• Regression test is accurate. Patching FieldFile.open to return a bytes-yielding context manager correctly simulates the cloud backend failure the suite could not exercise before, and json.dumps(result) is asserted to not raise -- testing the original crash directly.
• Agent memory simplification is a genuine improvement. The two-level fallback (open -> read) was overcomplicated; the primary path not decoding bytes was a latent bug. The simplified form is correct and easier to reason about.

---

Issues and Concerns

1. Behavioral change in aload_document_txt_extract (minor, intentional but worth confirming)

In llms/tools/core_tools/text_extracts.py, the async path changed from:

content_str = doc.txt_extract_file.read().decode("utf-8")

to:

content_str = read_field_file_text(doc.txt_extract_file, errors="replace")

The error policy softened from strict (bytes.decode("utf-8") raises on invalid bytes) to errors="replace" (substitutes U+FFFD). The sync counterpart load_document_txt_extract also changed to errors="replace". Worth confirming both callers intentionally want the same lenient policy since the old code was strict in both.

2. Sync file I/O inside an async function (pre-existing, but worth flagging)

aload_document_txt_extract is async def, but read_field_file_text does synchronous blocking I/O (the old .read().decode() did the same). On a busy event loop this can block the thread. A follow-up wrapping the read in asyncio.to_thread or database_sync_to_async would be the right fix -- out of scope here, but a good candidate for a near-term issue.

3. Strict errors in LLM tool call sites may surface new UnicodeDecodeErrors

The following call sites use the helper's default strict mode:

• pipeline/base/thumbnailer.py
• utils/export_v2.py
• utils/extraction_grounding.py
• llms/tools/core_tools/{annotations,document_indexing,pii,search}.py

With the old open("r") pattern, a cloud backend returning bytes would silently put a bytes object into doc_text. With the new strict helper, the same file raises UnicodeDecodeError at read time.

Fail-fast is arguably better, but check that callers handle it. In annotations.py:

doc_text = read_field_file_text(doc.txt_extract_file)  # can now raise UnicodeDecodeError

If the surrounding code does not catch this, the LLM tool propagates UnicodeDecodeError to the agent framework rather than returning a structured error string. Per the CLAUDE.md agent tool fault-tolerance note, operational exceptions should be caught and returned as error strings to the LLM. Consider errors="replace" for the LLM tool call sites, or confirm the agent framework's outer handler covers UnicodeDecodeError.

4. Test name is now misleading (nit)

In test_agent_memory.py, test_both_read_paths_fail_returns_empty still refers to "both read paths" but there is now only one path since the two-level fallback was removed. The test logic is still valid but the name could confuse future readers.

5. test_markdown_parser.py patches the class globally (minor parallel-test risk)

patch.object(type(doc.txt_extract_file), "open") patches FieldFile.open on the class itself. Inside a with block this reverts cleanly, but if pytest-xdist runs this test concurrently with any test that relies on real FieldFile.open behavior there is a transient window where the class method is swapped. Probably fine in practice, but a short comment explaining why the class is patched rather than an instance would help future readers.

---

Test Coverage Assessment

Path	Covered
utils/files.py::read_field_file_text (bytes/str/ignore/strict)	Yes -- test_files_utils.py
mcp/tools.py::get_document_text bytes path	Yes -- test_mcp.py regression
mcp/resources.py::get_document_resource bytes path	No new test
agents/memory.py simplification	Yes -- test_agent_memory.py updated
pipeline/parsers/oc_markdown_parser.py	Yes -- test_markdown_parser.py updated
LLM core tool sites	No new tests (inherits helper coverage)
tasks/embeddings_task.py DRY consolidation	No new test (existing behavior)

The notable gap is mcp/resources.py -- the resource path has the same bug class as the tool path but no direct bytes-path regression test. A test mirroring test_get_document_text_handles_bytes_from_cloud_storage for get_document_resource would round this out.

---

Verdict

Approve with the LLM tool UnicodeDecodeError concern addressed. Everything else is minor or pre-existing. The core fix is correct, well-scoped, and a genuine improvement to the codebase's consistency. If the LLM tool call sites are confirmed to be covered by the agent framework's outer operational-exception handler, no code change is needed there -- just confirmation.

[claude]

Code Review

Overview

This PR fixes a real production bug: txt_extract_file.open("r").read() on cloud storage backends (S3/GCS via django-storages #382) silently returns bytes instead of str, causing json.dumps to raise TypeError: Object of type bytes is not JSON serializable in MCP tool/resource handlers. The fix introduces a shared read_field_file_text() helper in utils/files.py and routes all 11 affected call sites through it.

The root cause analysis is accurate, the fix is comprehensive, and the approach (DRY consolidation + Protocol-typed helper) matches the project conventions well.

---

Strengths

• Correct diagnosis. The bug only manifests on cloud storage and never locally, which explains why it wasn't caught earlier. The PR description accurately traces the failure path through json.dumps outside the try/except.
• Single source of truth. read_field_file_text() is the right abstraction. The duplicated isinstance(..., bytes) decode in embeddings_task.py and oc_markdown_parser.py should have been here from the start.
• Protocol typing. Using a structural Protocol for the parameter type instead of the concrete FieldFile lets test doubles type-check cleanly — this is the right call.
• Differentiated error policies. Strict decoding for pipeline/export paths (fail fast on corrupt files), errors="replace" for agent/MCP paths (fault-tolerant per CLAUDE.md convention). The reasoning is sound.
• Test coverage. Unit tests for the helper cover all four cases (bytes→str, str passthrough, errors policy, strict raises). The regression tests in test_mcp.py correctly simulate the cloud-storage bytes path that local FileSystemStorage can't exercise.

---

Issues and Suggestions

1. _BytesHandle is duplicated across two test methods (minor DRY violation)

test_get_document_text_handles_bytes_from_cloud_storage and test_get_document_resource_handles_bytes_from_cloud_storage each define an identical _BytesHandle inner class. Given that _FakeOpenedFile in test_files_utils.py already does the same job, this is an easy extraction:

# At class level inside MCPToolsDocumentTest (or extracted to a module helper)
class _BytesHandle:
    def __enter__(self): return self
    def __exit__(self, *exc): return False
    def read(self): return b""  # overridden per test

Or the two test methods could share a _make_bytes_handle(payload) factory. Not a blocker, but it's the kind of duplication CLAUDE.md flags.

2. text_extracts.py silently changes error-handling semantics

load_document_txt_extract and aload_document_txt_extract previously used strict UTF-8 decoding (.decode("utf-8") with no errors= argument → raises on bad bytes). They now pass errors="replace", silently substituting U+FFFD for undecodable bytes. The cached string written to _DOC_TXT_CACHE is now potentially different from what strict decoding would produce.

This is probably fine in practice (the data is already in cloud storage, so if it's there it should decode cleanly), but it's a semantic change that isn't called out in the PR description's "per-site decode semantics are preserved" claim. Worth either:

• Confirming this is intentional and adding a brief comment to that effect, or
• Keeping text_extracts.py at strict mode (errors="strict") to match its prior behavior.

3. test_parse_handles_bytes_from_storage mock patches the class — worth a comment

with patch.object(type(doc.txt_extract_file), "open") as mock_open:

This patches open on the FieldFile class, not the instance, so it affects every FieldFile instance alive during the test. That's intentional (it's the only reliable way to intercept FieldFile.open) but it's slightly surprising. A one-line comment explaining why patch.object(doc.txt_extract_file, ...) would fail would make this clearer for future readers.

4. _TextReadableFieldFile.open return type could be more precise

def open(self, mode: str = ...) -> typing.ContextManager[typing.Any]: ...

ContextManager[Any] is accurate but loses the type of f inside the with block, which is where f.read() is called. A tighter bound like ContextManager[IO[Any]] (from typing) or even just leaving it at Any would make the intent clearer to type-checkers. Not a blocking issue, but worth considering since the Protocol is the public contract for this helper.

---

Minor Nits

• The # (Re)load from storage. comment removed from text_extracts.py was useful — the replacement # helper normalizes bytes-returning backends. is accurate but loses the "re-load on cache miss" context. Consider # Cache miss — reload from storage; helper normalizes bytes-returning backends.
• The four identical comments in llms/tools/core_tools/ about errors="replace" and U+FFFD are fine but could be a single per-module comment rather than repeating in each function. Not a DRY issue worth changing now, just noting.

---

Summary

This is a solid fix for a real production issue. The abstraction is correct, the test coverage is meaningful, and the rollout is comprehensive. The two items worth addressing before merge are:

1. The text_extracts.py error-handling semantics change — confirm intentional or revert to strict.
2. _BytesHandle duplication in the two new MCP test methods — straightforward extraction.

Everything else is optional polish.

[claude]

Code Review

Summary

This PR fixes a production crash on cloud storage deployments (AWS S3, GCP): json.dumps raised TypeError: Object of type bytes is not JSON serializable because django-storages backends return bytes from FieldFile.open("r").read() even in text mode, while local FileSystemStorage returns str. The fix introduces a centralized read_field_file_text() helper and routes all txt_extract_file.open("r") call sites through it.

The root cause analysis is accurate and the approach is well-chosen. A few issues follow.

---

Required

PR body credits Claude Code — violates project rules

The PR description ends with _Generated by [Claude Code](https://claude.ai/code/session_...)_. The project's CLAUDE.md states explicitly: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please edit the PR body to remove that line.

---

Issues

Behavioral regression in agents/memory.py — silent failure on legitimate open() errors

opencontractserver/agents/memory.py:77-94

The old code had a two-level fallback: if open("r") raised, it fell back to .read() (which some backends support without an explicit open() call) and decoded the result. The new code removes that fallback entirely — an open() failure now immediately returns "" with a warning log.

This is documented in the PR and the tests are updated to match. But the removed fallback did handle at least one real scenario: backends that work with direct .read() but not with open("r"). The behavioral change silently degrades memory reads to empty string on those backends with no way for the caller to distinguish "file is empty" from "file could not be opened". If there's a reason to believe no production backend hits this path, a brief comment to that effect would clarify intent for future readers.

No test for the async path in text_extracts.py

opencontractserver/llms/tools/core_tools/text_extracts.py:233-237

The aload_document_txt_extract change swaps doc.txt_extract_file.read().decode("utf-8") for read_field_file_text(...). The sync variant has coverage through the existing cache tests, but the async path's bytes-returning-backend scenario has no dedicated test. The MCP regression tests cover the MCP tools, but this agent-facing path is a separate call stack. Worth adding a short test alongside the existing cache tests to pin it.

FieldFile.open() class-level patch in test_markdown_parser.py is fragile

opencontractserver/tests/test_markdown_parser.py:639

with patch.object(type(doc.txt_extract_file), "open") as mock_open:

Patching the FieldFile class (not the instance) means every FieldFile.open() call within the with block — including any incidental calls in middleware, signals, or setup — uses the mock. The _patch_fieldfile_open_returns_bytes helper in test_mcp.py has the same footprint. Using mock.patch.object(doc.txt_extract_file, "open", ...) would scope the patch to the specific instance, though you'd need to verify Django's descriptor resolution allows instance-level patching here (it may not, which is exactly why you went class-level). If instance-level patching doesn't work, a comment explaining why would prevent future readers from "fixing" it.

---

Observations

Protocol type annotation is a good call

opencontractserver/utils/files.py:714-718

Using _TextReadableFieldFile as a structural Protocol rather than typing against the concrete FieldFile keeps the helper testable with simple duck-typed fakes without # type: ignore. The fake objects in test_files_utils.py satisfy it cleanly. Good pattern.

errors parameter typed as str rather than Literal

Minor: errors: str = "strict" accepts any string. typing.Literal["strict", "ignore", "replace", "xmlcharrefreplace", "backslashreplace"] would surface invalid values at type-check time. Not blocking, but an easy improvement.

Comment verbosity in LLM tool files

opencontractserver/llms/tools/core_tools/annotations.py:283-287 (and the three sibling files)

The three-line comment block explaining errors="replace" is the same across all four tool files. The WHY is non-obvious (fault-tolerance-by-design), so a comment is warranted per CLAUDE.md. But the CHANGELOG entry already contains the full rationale. A single-line comment — e.g., # errors="replace": agent tool must stay fault-tolerant (see CLAUDE.md) — would be sufficient and avoid the duplication.

export_v2.py and extraction_grounding.py use strict mode correctly

Both pipeline/export call sites call read_field_file_text(...) with the default errors="strict", which is the right choice for pipeline code that should fail fast on corrupt files. Good differentiation.

Test coverage for the helper is complete

opencontractserver/tests/test_files_utils.py covers bytes→str decode, str passthrough, errors="ignore" policy, and strict-raises. The MCP regression tests verify the full tool call path including json.dumps serializability. Solid.

---

Overall

The fix is correct and the DRY consolidation across twelve call sites is well-executed. Addressing the CLAUDE.md attribution line, the memory.py behavioral regression discussion, and the missing async-path test would make this ready to merge.