From bb4ef809c2c83c1cd0a595c659157c24b2f3ce86 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 6 Jun 2026 14:02:05 +0200 Subject: [PATCH] test(vector): unit-test page-aware routing; clarify fallback comment Address claude-review round 2 on PR #868: - Extract the use_page_aware branching into a pure `should_use_page_aware` helper and cover the (doc_type, page_boundaries, page_aware_setting) matrix in tests/unit/test_processor_routing.py (file+boundaries+enabled, empty list, None, non-file doc types, disabled setting). - Clarify the PageAwareChunker.chunk_text no-boundaries comment: the processor pre-filters via should_use_page_aware, so that branch is a direct-call safety net, not a production indexing path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../vector/document_chunker.py | 7 ++- nextcloud_mcp_server/vector/processor.py | 30 ++++++--- tests/unit/test_processor_routing.py | 62 +++++++++++++++++++ 3 files changed, 90 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_processor_routing.py diff --git a/nextcloud_mcp_server/vector/document_chunker.py b/nextcloud_mcp_server/vector/document_chunker.py index a71d4b5f..0f45f892 100644 --- a/nextcloud_mcp_server/vector/document_chunker.py +++ b/nextcloud_mcp_server/vector/document_chunker.py @@ -167,8 +167,11 @@ class PageAwareChunker: if not content: return [ChunkWithPosition(text="", start_offset=0, end_offset=0)] - # No page info (e.g. a non-PDF that reached this path) — degrade to the - # char-based behaviour so the caller still gets sensible chunks. + # No page info — degrade to char-based behaviour so the class is safe to + # call directly. The vector-sync processor pre-filters this case + # (``should_use_page_aware`` requires a truthy boundary list), so in + # production this branch is only reached by direct callers/tests, not + # the indexing path. if not page_boundaries: docs = await anyio.to_thread.run_sync( # type: ignore[attr-defined] self.splitter.create_documents, diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 55e998a4..a879e340 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -92,6 +92,25 @@ def assign_page_numbers(chunks, page_boundaries): chunk.page_number = assigned_page +def should_use_page_aware( + *, page_aware_enabled: bool, doc_type: str, page_boundaries: Any +) -> bool: + """Decide whether the page-aware chunker applies to this document. + + Page-aware chunking applies only to paginated files (PDFs) that actually + carry page boundaries. ``page_boundaries`` is tested for truthiness, not + just ``is not None``: an empty list carries no pages, so it routes through + the char-based path rather than the page-aware chunker's no-boundaries + fallback. + + Args: + page_aware_enabled: ``settings.document_chunk_page_aware``. + doc_type: The document type (only ``"file"`` is paginated). + page_boundaries: The extractor's page-boundary list (or ``None``). + """ + return page_aware_enabled and doc_type == "file" and bool(page_boundaries) + + async def processor_task( worker_id: int, receive_stream: MemoryObjectReceiveStream[DocumentTask], @@ -690,13 +709,10 @@ async def _index_document( # which assigns page numbers inline; everything else uses the char-based # chunker followed by post-hoc page assignment. page_boundaries = file_metadata.get("page_boundaries") - use_page_aware = ( - settings.document_chunk_page_aware - and doc_task.doc_type == "file" - # Truthy (not just "is not None"): an empty list carries no pages, so - # route it through the char-based path rather than the page-aware - # chunker's no-boundaries fallback. - and bool(page_boundaries) + use_page_aware = should_use_page_aware( + page_aware_enabled=settings.document_chunk_page_aware, + doc_type=doc_task.doc_type, + page_boundaries=page_boundaries, ) with trace_operation( "vector_sync.chunk_text", diff --git a/tests/unit/test_processor_routing.py b/tests/unit/test_processor_routing.py new file mode 100644 index 00000000..5e581625 --- /dev/null +++ b/tests/unit/test_processor_routing.py @@ -0,0 +1,62 @@ +"""Unit tests for the page-aware chunker routing decision (processor.py).""" + +import pytest + +from nextcloud_mcp_server.vector.processor import should_use_page_aware + +pytestmark = pytest.mark.unit + +_BOUNDARIES = [{"page": 1, "start_offset": 0, "end_offset": 10}] + + +class TestShouldUsePageAware: + """Cover the (doc_type, page_boundaries, page_aware_setting) matrix.""" + + def test_pdf_with_boundaries_and_enabled_uses_page_aware(self): + assert ( + should_use_page_aware( + page_aware_enabled=True, + doc_type="file", + page_boundaries=_BOUNDARIES, + ) + is True + ) + + def test_empty_boundaries_falls_back_to_char_based(self): + """Empty list carries no pages -> char-based path.""" + assert ( + should_use_page_aware( + page_aware_enabled=True, doc_type="file", page_boundaries=[] + ) + is False + ) + + def test_none_boundaries_falls_back_to_char_based(self): + assert ( + should_use_page_aware( + page_aware_enabled=True, doc_type="file", page_boundaries=None + ) + is False + ) + + @pytest.mark.parametrize("doc_type", ["note", "deck_card", "news_item"]) + def test_non_file_doc_types_never_page_aware(self, doc_type): + """Only paginated files are page-aware, even with boundaries present.""" + assert ( + should_use_page_aware( + page_aware_enabled=True, + doc_type=doc_type, + page_boundaries=_BOUNDARIES, + ) + is False + ) + + def test_disabled_setting_forces_char_based(self): + assert ( + should_use_page_aware( + page_aware_enabled=False, + doc_type="file", + page_boundaries=_BOUNDARIES, + ) + is False + )