From 1634e8adc250e1fc585781074afea55506ce923a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 5 Jun 2026 02:13:09 +0200 Subject: [PATCH] fix(review): cache OCR backend, drop asserts, real pipeline_tier, guard zero-page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #858 review: - 🔴 OcrProcessor now resolves its backend once and reuses it. Rebuilding per call created a fresh GatewayTokenProvider each time -- discarding its M2M-token cache, so every OCR'd document fetched a new token -- and a new Mistral client. - 🔴 build_ocr_backend uses explicit ValueError (not assert, which is stripped under `python -O`) for the gateway M2M triple. - PIPELINE_TIER in the Qdrant payload now reflects the tier that actually produced the doc: the registry stamps result.metadata["pipeline_tier"] and the processor reads it (was hardcoded "fast", wrong for OCR/structured). - Escalation now requires classification.page_count > 0, so a zero-page (empty/corrupt) PDF isn't pointlessly sent to OCR; documented that a fast FAILURE (encrypted/unopenable) is a hard failure and is not OCR-escalated. - Documented the OCR page_boundaries separator-attribution choice. - Downgraded the per-document page-boundary / page-assignment INFO logs to debug. New tests: zero-page no-escalation, pipeline_tier stamping. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_processors/ocr.py | 38 ++++++++++++++--- .../document_processors/registry.py | 12 +++++- nextcloud_mcp_server/vector/processor.py | 10 +++-- tests/unit/test_registry_tiering.py | 42 +++++++++++++++---- 4 files changed, 85 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/document_processors/ocr.py b/nextcloud_mcp_server/document_processors/ocr.py index ed76b096..019c4897 100644 --- a/nextcloud_mcp_server/document_processors/ocr.py +++ b/nextcloud_mcp_server/document_processors/ocr.py @@ -38,9 +38,13 @@ def _pages_to_text( ) -> tuple[str, list[dict[str, Any]]]: """Join per-page markdown (ordered by index) into one string + boundaries. - Pages are joined with a blank line; each page owns its leading separator so - ``page_boundaries`` stay contiguous and index exactly into the returned text - (the ``search/pdf_highlighter`` contract). + Pages are joined with a blank line. Boundaries are kept CONTIGUOUS (each + page owns its leading ``\\n\\n`` separator) so they index exactly into the + returned text and ``boundaries[-1]["end_offset"] == len(text)`` -- the + ``search/pdf_highlighter`` contract. Consequence: a page's range starts at + its separator, not its first glyph (the fast pypdfium2 path joins with no + separator, so its ranges are glyph-tight). The 2-char offset is immaterial + to page-level chunk attribution. """ sep = "\n\n" parts: list[str] = [] @@ -137,8 +141,18 @@ def build_ocr_backend(settings: Settings) -> _OcrBackend | None: GatewayTokenProvider, ) - assert settings.embedding_gateway_token_url is not None - assert settings.embedding_gateway_client_secret is not None + # Explicit (not assert -- assert is stripped under `python -O`): the + # M2M triple is all-or-nothing. + if not settings.embedding_gateway_token_url: + raise ValueError( + "EMBEDDING_GATEWAY_TOKEN_URL is required when " + "EMBEDDING_GATEWAY_CLIENT_ID is set" + ) + if not settings.embedding_gateway_client_secret: + raise ValueError( + "EMBEDDING_GATEWAY_CLIENT_SECRET is required when " + "EMBEDDING_GATEWAY_CLIENT_ID is set" + ) token_provider = GatewayTokenProvider( token_url=settings.embedding_gateway_token_url, client_id=settings.embedding_gateway_client_id, @@ -162,6 +176,15 @@ def build_ocr_backend(settings: Settings) -> _OcrBackend | None: class OcrProcessor(DocumentProcessor): """Tier-3 OCR processor (gateway or direct Mistral backend).""" + def __init__(self) -> None: + # Resolve the backend once and reuse it: rebuilding per call would create + # a fresh GatewayTokenProvider each time (discarding its M2M-token cache + # -> a token fetch per document) and a new Mistral SDK client per call. + # A config change needs a pod restart anyway, so caching for the pod's + # lifetime is safe. + self._backend_resolved = False + self._backend: _OcrBackend | None = None + @property def name(self) -> str: return "ocr" @@ -185,7 +208,10 @@ class OcrProcessor(DocumentProcessor): ) = None, ) -> ProcessingResult: settings = get_settings() - backend = build_ocr_backend(settings) + if not self._backend_resolved: + self._backend = build_ocr_backend(settings) + self._backend_resolved = True + backend = self._backend if backend is None: logger.warning( "OCR requested for %s but no backend is configured (provider=%s)", diff --git a/nextcloud_mcp_server/document_processors/registry.py b/nextcloud_mcp_server/document_processors/registry.py index 53b296c2..9a724640 100644 --- a/nextcloud_mcp_server/document_processors/registry.py +++ b/nextcloud_mcp_server/document_processors/registry.py @@ -245,10 +245,16 @@ class ProcessorRegistry: ) # Escalate scanned / no-text-layer PDFs to OCR (tier-3) when enabled and - # a provider is registered. The fast tier is terminal otherwise. + # a provider is registered. The fast tier is terminal otherwise. Note: a + # fast FAILURE (encrypted/corrupt -- result.success False, no + # classification) is NOT escalated; a PDF pypdfium2 can't open is treated + # as a hard failure (OCR reads the same bytes and would usually fail + # too). The page_count guard skips a zero-page (empty/corrupt) PDF, which + # OCR can't help either. if ( classification is not None and classification.recommended_tier == "ocr" + and classification.page_count > 0 and settings.document_ocr_enabled ): ocr = self._pdf_processor_for_tier("ocr") @@ -359,6 +365,10 @@ class ProcessorRegistry: raise duration = time.time() - start_time + # Record the tier that actually produced this result so downstream + # (Qdrant payload pipeline_tier, analytics) reflects escalation + # instead of a hardcoded "fast". + result.metadata.setdefault("pipeline_tier", tier) pages = int(result.metadata.get("page_count", 0) or 0) chars = len(result.text) status = "success" if result.success else "error" diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index e9ed5a5a..e0781895 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -589,7 +589,7 @@ async def _index_document( # Diagnostic: Log page boundary information if available if "page_boundaries" in file_metadata: page_boundaries = file_metadata["page_boundaries"] - logger.info( + logger.debug( "Page boundaries for %s: %s pages, text length: %s", file_path, len(page_boundaries), @@ -652,7 +652,7 @@ async def _index_document( # Diagnostic: Verify page number assignment assigned_count = sum(1 for c in chunks if c.page_number is not None) - logger.info( + logger.debug( "Assigned page numbers to %s/%s chunks for %s", assigned_count, len(chunks), @@ -926,7 +926,11 @@ async def _index_document( # Decomposition payload keys (design §10.2), additive. payload_keys.PROCESSOR_VERSION: "monolith-v1", payload_keys.PARSED_AT: indexed_at, - payload_keys.PIPELINE_TIER: "fast", + # Actual tier that produced this doc (registry stamps it on + # the result metadata); non-PDF doc types stay "fast". + payload_keys.PIPELINE_TIER: file_metadata.get( + "pipeline_tier", "fast" + ), payload_keys.EMBEDDING_IDENTITY: _embedding_identity, payload_keys.ACL_HASH: _acl_hash, # File-specific metadata (PDF, etc.) diff --git a/tests/unit/test_registry_tiering.py b/tests/unit/test_registry_tiering.py index 37dd6177..54d0efd5 100644 --- a/tests/unit/test_registry_tiering.py +++ b/tests/unit/test_registry_tiering.py @@ -20,12 +20,18 @@ pytestmark = pytest.mark.unit class _Fake(DocumentProcessor): def __init__( - self, name: str, tier: str, text: str = "clean text here", success=True + self, + name: str, + tier: str, + text: str = "clean text here", + success=True, + pages: int = 1, ): self._name = name self._tier = tier self._text = text self._success = success + self._pages = pages @property def name(self) -> str: @@ -42,14 +48,14 @@ class _Fake(DocumentProcessor): async def process( self, content, content_type, filename=None, options=None, progress_callback=None ): + boundaries = ( + [{"page": 1, "start_offset": 0, "end_offset": len(self._text)}] + if self._pages + else [] + ) return ProcessingResult( text=self._text, - metadata={ - "page_count": 1, - "page_boundaries": [ - {"page": 1, "start_offset": 0, "end_offset": len(self._text)} - ], - }, + metadata={"page_count": self._pages, "page_boundaries": boundaries}, processor=self._name, success=self._success, ) @@ -117,6 +123,28 @@ async def test_ocr_escalation_on_empty_text(monkeypatch): esc.assert_called_once() +async def test_zero_page_pdf_does_not_escalate(monkeypatch): + # An empty/corrupt PDF (no pages) classifies "ocr" but must NOT escalate -- + # OCR can't help and it would be wasteful. + monkeypatch.setattr(reg_mod, "get_settings", lambda: _Settings(ocr=True)) + esc = MagicMock() + monkeypatch.setattr(reg_mod, "record_document_escalation", esc) + r = _registry( + (_Fake("fast", "fast", text="", pages=0), 20), + (_Fake("ocr", "ocr"), 5), + ) + res = await r.process(b"%PDF-1.7", "application/pdf") + assert res.processor == "fast" + esc.assert_not_called() + + +async def test_pipeline_tier_stamped_on_metadata(monkeypatch): + monkeypatch.setattr(reg_mod, "get_settings", lambda: _Settings()) + r = _registry((_Fake("fast", "fast"), 20)) + res = await r.process(b"%PDF-1.7", "application/pdf") + assert res.metadata["pipeline_tier"] == "fast" + + async def test_ocr_failure_falls_back_to_fast(monkeypatch): # OCR enabled but the backend can't run (no creds / API down) -> keep the # tier-1 result instead of failing the document.