From 0614709960ea6f516db1db3e12db902c4d706874 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 18 Jun 2026 00:56:02 +0200 Subject: [PATCH] fix(ingest): warn when provider=none disables in-cluster; precise model fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address claude-review round 4 on #922: - Important 1: build_ocr_backend now warns when gateway_only + provider=none + DOCUMENT_OCR_INCLUSTER_ENABLED=true — provider=none suppresses the gateway-only in-cluster tier too (it never uses the mistral provider), which surprises an operator who set none just to disable Mistral. Restructured so the gateway_only branch is evaluated before the generic provider=none return. Two tests cover the warn-when-enabled / silent-when-disabled cases. - Nit 3: model fallback uses `model if model is not None else ...` (not `or`), so an empty model string no longer silently falls back to the upstream default and misroutes a per-tier rung. Test added. - Nit 4: the no-surya-literal guard now uses rglob so future document_processors/ subdirs are covered. Deferred (pre-existing / out of scope for this PR): - Important 2 (_GatewayOcrBackend opens a new httpx.AsyncClient per ocr() call): a pre-existing pattern affecting both OCR rungs; a shared pooled client needs careful per-pod lifecycle handling (event-loop binding, aclose) and is better as its own change. Follow-up. - Nit 5 (_MANAGED_QUEUES vs the CLI all-queues list): the two sets differ deliberately (the CLI list includes ingest-maintenance, _MANAGED_QUEUES does not), so a shared constant wouldn't cleanly dedupe them. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_processors/ocr.py | 20 +++++-- tests/unit/test_ocr_processor.py | 52 ++++++++++++++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/document_processors/ocr.py b/nextcloud_mcp_server/document_processors/ocr.py index d05b7aac..cf306f30 100644 --- a/nextcloud_mcp_server/document_processors/ocr.py +++ b/nextcloud_mcp_server/document_processors/ocr.py @@ -239,11 +239,22 @@ def build_ocr_backend( disabled (warn) rather than misrouted to a direct backend that can't serve it. """ provider = settings.document_ocr_provider - model = model or settings.document_ocr_model - if provider == "none": - return None + # `is not None` (not truthiness): an empty model string must NOT silently fall + # back to the upstream default and misroute a per-tier rung. + model = model if model is not None else settings.document_ocr_model if gateway_only: + # The in-cluster tier is gateway-only and never touches the `mistral` + # provider, but provider=none still suppresses it. Warn so an operator who + # set provider=none to disable Mistral doesn't silently lose the GPU tier. + if provider == "none": + if settings.document_ocr_incluster_enabled: + logger.warning( + "DOCUMENT_OCR_PROVIDER=none disables in-cluster OCR even with " + "DOCUMENT_OCR_INCLUSTER_ENABLED=true; set it to 'gateway' or " + "'auto' to keep the in-cluster tier" + ) + return None if settings.embedding_gateway_url: return _GatewayOcrBackend( settings.embedding_gateway_url, @@ -256,6 +267,9 @@ def build_ocr_backend( ) return None + if provider == "none": + return None + if provider in ("gateway", "auto") and settings.embedding_gateway_url: return _GatewayOcrBackend( settings.embedding_gateway_url, diff --git a/tests/unit/test_ocr_processor.py b/tests/unit/test_ocr_processor.py index 469a9250..4aab2d55 100644 --- a/tests/unit/test_ocr_processor.py +++ b/tests/unit/test_ocr_processor.py @@ -18,6 +18,7 @@ def _settings(**kw) -> Any: # a Settings stand-in (only the read fields matter) base = dict( document_ocr_provider="auto", document_ocr_model="mistral/mistral-ocr-latest", + document_ocr_incluster_enabled=False, document_ocr_timeout_seconds=180.0, document_ocr_mode="sync", document_ocr_batch_poll_seconds=120, @@ -124,6 +125,55 @@ def test_build_backend_gateway_only_no_url_disabled(): ) +def test_build_backend_gateway_only_provider_none_warns_when_incluster_enabled(caplog): + """provider=none also suppresses the gateway-only in-cluster tier (it never uses + the mistral provider, but the global `none` gate still applies). When in-cluster + is enabled this is almost certainly an operator mistake -> warn so it's visible.""" + with caplog.at_level( + "WARNING", logger="nextcloud_mcp_server.document_processors.ocr" + ): + b = ocr.build_ocr_backend( + _settings( + document_ocr_provider="none", + embedding_gateway_url="http://gw", + document_ocr_incluster_enabled=True, + ), + gateway_only=True, + ) + assert b is None + assert any( + "DOCUMENT_OCR_PROVIDER=none disables in-cluster" in r.message + for r in caplog.records + ) + + +def test_build_backend_gateway_only_provider_none_silent_when_incluster_disabled( + caplog, +): + """provider=none with in-cluster OFF is a deliberate disable -> no warning noise.""" + with caplog.at_level( + "WARNING", logger="nextcloud_mcp_server.document_processors.ocr" + ): + b = ocr.build_ocr_backend( + _settings(document_ocr_provider="none", embedding_gateway_url="http://gw"), + gateway_only=True, + ) + assert b is None + assert not any("disables in-cluster" in r.message for r in caplog.records) + + +def test_build_backend_empty_model_does_not_fall_back(caplog): + """An empty model string must NOT silently fall back to the upstream default + (an `or` would); the in-cluster rung keeps the empty id it was handed.""" + b = ocr.build_ocr_backend( + _settings(embedding_gateway_url="http://gw"), + model="", + gateway_only=True, + ) + assert isinstance(b, ocr._GatewayOcrBackend) + assert b._model == "" + + def test_build_backend_model_override_is_not_hardcoded(): """The per-tier model is whatever config passes -- surya by default, but fully swappable (e.g. lightonocr) with no code change.""" @@ -186,7 +236,7 @@ def test_no_surya_string_literal_in_document_processors(): pkg = pathlib.Path(ocr.__file__).parent offenders = [ f"{p.name}: {ln.strip()}" - for p in pkg.glob("*.py") + for p in pkg.rglob("*.py") # recurse: future backends/ subdirs too for ln in p.read_text().splitlines() if '"surya' in ln.split("#", 1)[0] or "'surya" in ln.split("#", 1)[0] ]