From 4af7c7104b4047b5cb27ffb16d22f25e340c214b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 16 Jun 2026 20:43:25 +0200 Subject: [PATCH] fix(document-processors): make glyph-corruption ratio of 0 disable the signal Address round-4 review on PR #914: - glyph_corruption_ratio <= 0 now disables the signal (previously `control_ratio > 0` fired on any single C0 control byte), matching the "0 disables" convention used elsewhere (document_max_pdf_size_mb) and the config comment. Add a zero-disables test. - Correct the document_escalation_suppressed_total comment: corrupt_glyphs CAN appear there in the narrow case where structured is unregistered and OCR is registered-but-disabled (evaluate_escalation follows minimum="structured" past the missing rung to a gated-off OCR). Add a test for that suppressed decision. - Add a test for the double-corruption edge: a structured re-extract that is also glyph-corrupt escalates structured->ocr with reason corrupt_glyphs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_processors/classifier.py | 12 +++-- nextcloud_mcp_server/observability/metrics.py | 7 +-- tests/unit/test_doc_classifier.py | 9 ++++ tests/unit/test_registry_tiering.py | 45 +++++++++++++++++++ 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/document_processors/classifier.py b/nextcloud_mcp_server/document_processors/classifier.py index 26dcb548..ecf76877 100644 --- a/nextcloud_mcp_server/document_processors/classifier.py +++ b/nextcloud_mcp_server/document_processors/classifier.py @@ -203,9 +203,15 @@ def _route_from_signals( Flags are diagnostic and independent of the verdict (e.g. ``image_heavy`` fires on ANY image-heavy page; the OCR route needs a page FRACTION). """ - # total_chars > 0 also guarantees this never overlaps the scanned branch - # (total_chars == 0), so a doc is never both glyph-corrupt and "scanned". - glyph_corrupt = total_chars > 0 and control_ratio > glyph_corruption_ratio + # glyph_corruption_ratio <= 0 disables the signal (a ratio of 0 would otherwise + # fire on any single C0 control byte). total_chars > 0 also guarantees this + # never overlaps the scanned branch (total_chars == 0), so a doc is never both + # glyph-corrupt and "scanned". + glyph_corrupt = ( + glyph_corruption_ratio > 0 + and total_chars > 0 + and control_ratio > glyph_corruption_ratio + ) flags: set[str] = set() if ocr_frac >= page_fraction and total_chars == 0: diff --git a/nextcloud_mcp_server/observability/metrics.py b/nextcloud_mcp_server/observability/metrics.py index 26e20421..139b6c87 100644 --- a/nextcloud_mcp_server/observability/metrics.py +++ b/nextcloud_mcp_server/observability/metrics.py @@ -287,9 +287,10 @@ 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. (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.) + # reason: low_confidence | empty_text | corrupt_glyphs. (corrupt_glyphs lands + # here only in the narrow case where the structured tier is unregistered AND + # OCR is registered-but-disabled: evaluate_escalation follows minimum="structured" + # past the missing rung to OCR, which is gated off -> suppressed{to_tier="ocr"}.) ["from_tier", "to_tier", "reason"], ) diff --git a/tests/unit/test_doc_classifier.py b/tests/unit/test_doc_classifier.py index 5d843f6f..29400159 100644 --- a/tests/unit/test_doc_classifier.py +++ b/tests/unit/test_doc_classifier.py @@ -387,6 +387,15 @@ def test_glyph_corruption_ratio_override_disables_trigger(): assert "corrupt_glyphs" not in c.flags +def test_glyph_corruption_ratio_zero_disables_trigger(): + full = _GLYPH_CORRUPT + bounds = [{"page": 1, "start_offset": 0, "end_offset": len(full)}] + # 0 disables the signal (rather than firing on any single control byte). + c = clf.classify_from_text(full, bounds, glyph_corruption_ratio=0.0) + assert c.recommended_tier == "fast" + assert "corrupt_glyphs" not in c.flags + + def test_empty_doc_routes_ocr_not_structured(): # Precedence: a scanned/empty doc (no text layer) has no control chars to leak, # so it must stay an OCR case, never structured. diff --git a/tests/unit/test_registry_tiering.py b/tests/unit/test_registry_tiering.py index 6b00a092..d2c9d6df 100644 --- a/tests/unit/test_registry_tiering.py +++ b/tests/unit/test_registry_tiering.py @@ -349,6 +349,26 @@ async def test_inline_fast_structured_ocr_cascade(monkeypatch): ] +async def test_inline_structured_still_corrupt_escalates_to_ocr(monkeypatch): + # Edge: the structured re-extract is ALSO glyph-corrupt (pymupdf also failed to + # decode). Re-classification stays "structured", so the OCR gate fires -- + # attributed from_tier="structured" with reason corrupt_glyphs. + 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=_GLYPH), 20), + (_Fake("structured", "structured", text=_GLYPH), 10), # still corrupt + (_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", "corrupt_glyphs"), + call("structured", "ocr", "corrupt_glyphs"), + ] + + 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()) @@ -395,6 +415,31 @@ def test_evaluate_escalation_glyph_corrupt_no_structured_falls_through_to_ocr( assert decision == EscalationDecision("hop", "ocr", "corrupt_glyphs") +def test_evaluate_escalation_glyph_corrupt_no_structured_ocr_disabled_suppressed( + monkeypatch, +): + # Structured unregistered AND OCR registered-but-disabled: the would-be OCR + # fallthrough is suppressed, and it carries the corrupt_glyphs reason (so the + # "what-if OCR" counter can show latent glyph-corruption demand). + monkeypatch.setattr(reg_mod, "record_document_classification", MagicMock()) + r = _registry( + (_Fake("fast", "fast"), 20), + (_Fake("ocr", "ocr"), 5), + ) # structured not registered; ocr registered but disabled below + res = ProcessingResult( + text=_GLYPH, + metadata={ + "page_count": 1, + "page_boundaries": [ + {"page": 1, "start_offset": 0, "end_offset": len(_GLYPH)} + ], + }, + processor="fast", + ) + decision = r.evaluate_escalation(res, b"%PDF", "fast", _Settings(ocr=False)) + assert decision == EscalationDecision("suppressed", "ocr", "corrupt_glyphs") + + # --- Per-tier external path (Deck #323) -------------------------------------