From d5286e39d65a1f493c87e26483335484c6c638f8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 16 Jun 2026 20:17:56 +0200 Subject: [PATCH] fix(document-processors): correct cascade escalation metric + review nits Address round-1 review on PR #914: - Attribute the OCR hop in a fast->structured->ocr inline cascade to from_tier="structured" (not a second "fast" escalation), so astrolabe_document_escalation_total per-tier counts stay accurate. - Add test_inline_fast_structured_ocr_cascade pinning that two-hop path and the metric attribution. - Note in classify_from_text that its doc-level control ratio is over full_text (all pages), not the sampled subset classify_pdf uses. - Clarify that corrupt_glyphs never lands in the suppressed-escalation counter. - Dedupe the glyph-corrupt test string into tests/fixtures/glyph_corruption.py. - Use pytest.approx for the control-char-ratio zero checks (SonarCloud S1244). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_processors/classifier.py | 9 ++++-- .../document_processors/registry.py | 12 +++++-- nextcloud_mcp_server/observability/metrics.py | 4 ++- tests/fixtures/glyph_corruption.py | 11 +++++++ tests/unit/test_doc_classifier.py | 15 +++++---- tests/unit/test_registry_tiering.py | 32 ++++++++++++++++--- 6 files changed, 65 insertions(+), 18 deletions(-) create mode 100644 tests/fixtures/glyph_corruption.py diff --git a/nextcloud_mcp_server/document_processors/classifier.py b/nextcloud_mcp_server/document_processors/classifier.py index 54d99414..7ce28fd4 100644 --- a/nextcloud_mcp_server/document_processors/classifier.py +++ b/nextcloud_mcp_server/document_processors/classifier.py @@ -407,9 +407,12 @@ def classify_from_text( # page_count guard also skips escalation; defaulting to 0.0 keeps the # recorded classification metric accurate rather than a misleading "ocr"). ocr_frac = (sum(p.needs_ocr for p in pages) / sampled) if sampled else 0.0 - # Doc-level (char-weighted) control-char ratio -- the glyph-leak signal that - # routes to the structured tier. Computed over full_text so it is robust to - # boundary edge cases. + # Doc-level control-char ratio -- the glyph-leak signal that routes to the + # structured tier. Computed over the WHOLE full_text (all pages), unlike + # classify_pdf which char-weights the up-to-MAX_SAMPLED_PAGES sample; the two + # are therefore not numerically identical for a >24-page doc with corruption + # concentrated outside the sample. full_text is used here because it is exactly + # the text that gets chunked + indexed and is robust to boundary edge cases. control_ratio = _control_char_ratio(full_text) flags, recommended = _route_from_signals( diff --git a/nextcloud_mcp_server/document_processors/registry.py b/nextcloud_mcp_server/document_processors/registry.py index c39fa619..40b9ae1e 100644 --- a/nextcloud_mcp_server/document_processors/registry.py +++ b/nextcloud_mcp_server/document_processors/registry.py @@ -247,6 +247,12 @@ class ProcessorRegistry: result, content, settings, record=True, filename=filename ) + # The tier whose output produced the current ``classification`` -- used as + # ``from_tier`` for a subsequent OCR hop so a fast->structured->ocr cascade + # is attributed correctly (the OCR hop is from ``structured``, not a second + # ``fast`` escalation). + from_tier = "fast" + # Escalate a poor fast extraction up the ladder (fast -> structured -> ocr), # mirroring the external per-tier path so both modes behave identically. A # glyph-corrupt layer (the extractor leaked raw glyph codes -- the @@ -293,6 +299,7 @@ class ProcessorRegistry: ) if structured_result.success: result = structured_result + from_tier = "structured" classification = self._classify_result( result, content, settings, record=False, filename=filename ) @@ -331,10 +338,11 @@ class ProcessorRegistry: if classification.total_chars == 0 else "low_confidence" ) - record_document_escalation("fast", "ocr", reason) + record_document_escalation(from_tier, "ocr", reason) logger.info( - "Escalating %s fast->ocr (reason=%s)", + "Escalating %s %s->ocr (reason=%s)", filename or "", + from_tier, reason, ) ocr_result = await self._run_processor( diff --git a/nextcloud_mcp_server/observability/metrics.py b/nextcloud_mcp_server/observability/metrics.py index c1e64a12..f2ca1060 100644 --- a/nextcloud_mcp_server/observability/metrics.py +++ b/nextcloud_mcp_server/observability/metrics.py @@ -287,7 +287,9 @@ document_escalation_total = Counter( document_escalation_suppressed_total = Counter( "astrolabe_document_escalation_suppressed_total", "Would-be tier escalations suppressed because the target tier is disabled", - # reason: low_confidence | empty_text + # reason: low_confidence | empty_text. (corrupt_glyphs never appears here: it + # targets the structured tier, which has no enabled-gate -- if registered it + # runs, else there is nothing to suppress -- so it only ever hops or returns.) ["from_tier", "to_tier", "reason"], ) diff --git a/tests/fixtures/glyph_corruption.py b/tests/fixtures/glyph_corruption.py new file mode 100644 index 00000000..08e8cb9e --- /dev/null +++ b/tests/fixtures/glyph_corruption.py @@ -0,0 +1,11 @@ +"""Shared test data for the tier-0 glyph-corruption signal. + +A fast-tier text layer that looks like words -- normal spacing and token lengths, +so it scores HIGH on ``_text_quality`` -- but leaks C0 control characters: the +broken-/ToUnicode signature that ``classifier._control_char_ratio`` catches. The +alphabetic tokens decode to a pangram under a -3 (Caesar) shift. + +Kept in one place so the classifier and registry tiering tests can't diverge. +""" + +GLYPH_CORRUPT_TEXT = "WKH \x0f TXLFN \x10 EURZQ \x11 IRA MXPSV \x0f RYHU \x10 GRJ " * 6 diff --git a/tests/unit/test_doc_classifier.py b/tests/unit/test_doc_classifier.py index 84429c3c..2a7d9ee3 100644 --- a/tests/unit/test_doc_classifier.py +++ b/tests/unit/test_doc_classifier.py @@ -14,6 +14,7 @@ import pymupdf import pytest from nextcloud_mcp_server.document_processors import classifier as clf +from tests.fixtures.glyph_corruption import GLYPH_CORRUPT_TEXT pytestmark = pytest.mark.unit @@ -324,17 +325,17 @@ def test_scan_coverage_shorter_than_pages_aligns_without_crash(): # --- glyph-corruption signal (broken /ToUnicode -> structured escalation) ----- -# pypdfium2-style leak: a uniform glyph/Caesar offset turns clean prose into -# alphabetic-but-wrong tokens (normal spacing + token length => HIGH text_quality) -# while digits/punctuation map to C0 control bytes. The control-char ratio is the -# only signal that catches this; _text_quality scores it ~1.0. -_GLYPH_CORRUPT = "WKH \x0f TXLFN \x10 EURZQ \x11 IRA MXPSV \x0f RYHU \x10 GRJ " * 6 +# A uniform glyph/Caesar offset turns clean prose into alphabetic-but-wrong tokens +# (normal spacing + token length => HIGH text_quality) while digits/punctuation map +# to C0 control bytes. The control-char ratio is the only signal that catches this; +# _text_quality scores it ~1.0. Shared with the registry tiering tests. +_GLYPH_CORRUPT = GLYPH_CORRUPT_TEXT def test_control_char_ratio_clean_is_zero(): - assert clf._control_char_ratio("the quick brown fox") == 0.0 + assert clf._control_char_ratio("the quick brown fox") == pytest.approx(0.0) # legitimate whitespace controls (tab/newline/CR/form-feed/vtab) don't count - assert clf._control_char_ratio("a\tb\nc\r\nd\f\ve") == 0.0 + assert clf._control_char_ratio("a\tb\nc\r\nd\f\ve") == pytest.approx(0.0) def test_control_char_ratio_detects_glyph_leak(): diff --git a/tests/unit/test_registry_tiering.py b/tests/unit/test_registry_tiering.py index c654912d..5263f7c3 100644 --- a/tests/unit/test_registry_tiering.py +++ b/tests/unit/test_registry_tiering.py @@ -4,7 +4,7 @@ Covers: default fast-tier routing, the pymupdf rollback toggle, classification recording derived from the extraction, and OCR escalation (on/off). """ -from unittest.mock import MagicMock +from unittest.mock import MagicMock, call import pytest @@ -15,6 +15,7 @@ from nextcloud_mcp_server.document_processors.base import ( ) from nextcloud_mcp_server.document_processors.escalation import EscalationDecision from nextcloud_mcp_server.document_processors.registry import ProcessorRegistry +from tests.fixtures.glyph_corruption import GLYPH_CORRUPT_TEXT pytestmark = pytest.mark.unit @@ -250,10 +251,10 @@ async def test_no_ocr_escalation_when_disabled(monkeypatch): # --- glyph-corruption escalation + full-ladder parity ------------------------ -# A fast-tier text layer that looks like words (normal spacing/token lengths -> -# HIGH text_quality) but leaks C0 control chars: the broken-/ToUnicode signature -# the control-char ratio catches. Decodes to a pangram under a -3 shift. -_GLYPH = "WKH \x0f TXLFN \x10 EURZQ \x11 IRA MXPSV \x0f RYHU \x10 GRJ " * 6 +# A fast-tier text layer that looks like words (HIGH text_quality) but leaks C0 +# control chars -- the broken-/ToUnicode signature the control-char ratio catches. +# Shared with the classifier tests so the two can't diverge. +_GLYPH = GLYPH_CORRUPT_TEXT async def test_glyph_corrupt_escalates_fast_to_structured(monkeypatch): @@ -311,6 +312,27 @@ async def test_inline_empty_skips_structured_straight_to_ocr(monkeypatch): esc.assert_called_once_with("fast", "ocr", "empty_text") +async def test_inline_fast_structured_ocr_cascade(monkeypatch): + # Full cascade: a junk-but-non-empty fast layer hops to structured, the + # structured re-extract is empty (a doc that was ALSO scanned), so it then + # hops to OCR. The second hop must be attributed from_tier="structured", + # NOT a second "fast" escalation. + 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="x" * 40), 20), # quality ~0, non-empty + (_Fake("structured", "structured", text=""), 10), # re-extract empty + (_Fake("ocr", "ocr", text="ocr recovered text"), 5), + ) + res = await r.process(b"%PDF-1.7", "application/pdf") + assert res.processor == "ocr" + assert esc.call_args_list == [ + call("fast", "structured", "low_confidence"), + call("structured", "ocr", "empty_text"), + ] + + def test_evaluate_escalation_glyph_corrupt_goes_structured(monkeypatch): # External path mirrors the inline path: glyph-corrupt -> structured, never OCR. monkeypatch.setattr(reg_mod, "record_document_classification", MagicMock())