From 7ef8760d277f69464cbead83317ca4ab0e8017db Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 9 May 2026 13:29:22 +0200 Subject: [PATCH] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767=20rev?= =?UTF-8?q?iew=20=E2=80=94=20extract=20bbox=20helper,=20fix=20page=5Fnumbe?= =?UTF-8?q?r=20overwrite?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves both 🟡 important issues from the latest review: 1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even when Qdrant's payload lacked the field, clobbering the value resolved from `chunk_context.page_number`. The new helper returns each field independently and both call sites only overwrite via `is not None` guards, matching the existing logic in `visualization.py`. 2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll block was duplicated between `api/visualization.py` and `auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant` in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant` helpers; both routes now share ~12 lines of caller code. New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed and offset paths, the `(bbox, None)` regression case, and graceful degradation on Qdrant strict-mode 400 (which also closes nit #4). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 93 ++-------- nextcloud_mcp_server/auth/viz_routes.py | 93 ++-------- nextcloud_mcp_server/search/context.py | 93 ++++++++++ tests/unit/test_chunk_bbox_helper.py | 215 ++++++++++++++++++++++ 4 files changed, 342 insertions(+), 152 deletions(-) create mode 100644 tests/unit/test_chunk_bbox_helper.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 309ef2de..5c744288 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -14,7 +14,6 @@ import logging from typing import Any import pymupdf -from qdrant_client.models import FieldCondition, Filter, MatchValue from starlette.requests import Request from starlette.responses import JSONResponse @@ -31,13 +30,14 @@ from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, SemanticSearchAlgorithm, ) -from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.search.context import ( + get_chunk_bbox_and_page_from_qdrant, + get_chunk_with_context, +) from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, ) -from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter -from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client from nextcloud_mcp_server.vector.visualization import compute_pca_coordinates logger = logging.getLogger(__name__) @@ -567,82 +567,23 @@ async def get_chunk_context(request: Request) -> JSONResponse: # For PDF files, also fetch the chunk's bounding box from Qdrant if # available so the client can overlay a highlight on top of a - # render-on-demand page image (Deck #76). + # render-on-demand page image (Deck #76). Qdrant's page_number is + # trusted over the context-expansion fallback when present. chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": - try: - settings = get_settings() - qdrant_client = await get_qdrant_client() - - # Prefer chunk_index for the chunk-bbox lookup (always indexed); - # fall back to (chunk_start_offset, chunk_end_offset) when not provided. - if chunk_index is not None: - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_index", - match=MatchValue(value=chunk_index), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - else: - # Legacy fallback for clients that don't send chunk_index - # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset - # aren't indexed in Qdrant Cloud strict mode, so this - # call may fail with HTTP 400 there; the outer except - # logs a warning and the response degrades gracefully - # (no chunk_bbox). - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_start_offset", - match=MatchValue(value=start), - ), - FieldCondition( - key="chunk_end_offset", - match=MatchValue(value=end), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - - if points_response[0]: - payload = points_response[0][0].payload - if payload: - chunk_bbox = payload.get("chunk_bbox") - # Trust Qdrant page number if available (might be more accurate than context expansion logic) - if payload.get("page_number") is not None: - page_number = payload.get("page_number") - - except Exception as e: - logger.warning(f"Failed to fetch chunk bbox: {e}") + qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( + user_id=user_id, + doc_id=doc_id_val, + chunk_index=chunk_index, + chunk_start=start, + chunk_end=end, + ) + if qdrant_bbox is not None: + chunk_bbox = qdrant_bbox + if qdrant_page is not None: + page_number = qdrant_page # Build response response_data = { diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d1a369bb..f6ba5de8 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -18,7 +18,6 @@ from pathlib import Path import anyio import numpy as np from jinja2 import Environment, FileSystemLoader -from qdrant_client.models import FieldCondition, Filter, MatchValue from starlette.authentication import requires from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse @@ -34,13 +33,15 @@ from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, SemanticSearchAlgorithm, ) -from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.search.context import ( + get_chunk_bbox_and_page_from_qdrant, + get_chunk_with_context, +) from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, ) from nextcloud_mcp_server.vector.pca import PCA -from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) @@ -625,82 +626,22 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # For PDF files, also fetch the chunk bbox from Qdrant so the client # can overlay a highlight on top of a render-on-demand page image - # (Deck #76). + # (Deck #76). Qdrant's page_number is trusted over the + # context-expansion fallback when present. chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": - try: - settings = get_settings() - qdrant_client = await get_qdrant_client() - - # Prefer chunk_index for the chunk-bbox lookup (always indexed); - # fall back to (chunk_start_offset, chunk_end_offset) when not provided. - if chunk_index is not None: - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_index", - match=MatchValue(value=chunk_index), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - else: - # Legacy fallback for clients that don't send chunk_index - # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset - # aren't indexed in Qdrant Cloud strict mode, so this - # call may fail with HTTP 400 there; the outer except - # logs a warning and the response degrades gracefully - # (no chunk_bbox). - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_start_offset", - match=MatchValue(value=start), - ), - FieldCondition( - key="chunk_end_offset", - match=MatchValue(value=end), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - - points = points_response[0] - if points and points[0].payload: - chunk_bbox = points[0].payload.get("chunk_bbox") - page_number = points[0].payload.get("page_number") - if chunk_bbox: - logger.info( - f"Found chunk bbox: page={page_number}, " - f"rects={len(chunk_bbox)}" - ) - except Exception as e: - logger.warning(f"Failed to fetch chunk bbox: {e}") + qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( + user_id=user_id, + doc_id=doc_id_int, + chunk_index=chunk_index, + chunk_start=start, + chunk_end=end, + ) + if qdrant_bbox is not None: + chunk_bbox = qdrant_bbox + if qdrant_page is not None: + page_number = qdrant_page # Return response compatible with frontend expectations response_data: dict = { diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 1a5fcaff..3f2205e4 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -12,6 +12,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.vector.html_processor import html_to_markdown +from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) @@ -195,6 +196,98 @@ async def _get_deck_metadata_from_qdrant( return None +async def get_chunk_bbox_and_page_from_qdrant( + user_id: str, + doc_id: int | str, + chunk_index: int | None, + chunk_start: int, + chunk_end: int, +) -> tuple[list | None, int | None]: + """Fetch chunk_bbox and page_number for a chunk from Qdrant payload. + + Prefers chunk_index for the lookup (always indexed); falls back to + (chunk_start_offset, chunk_end_offset) when chunk_index is not provided + — this is the legacy path for clients pre-cbcoutinho/astrolabe#75. The + fallback may 400 in Qdrant Cloud strict mode because those offset fields + aren't indexed there; that's logged as a warning and (None, None) is + returned so callers degrade gracefully. + + Args: + user_id: User ID who owns the document + doc_id: Document ID (int for file/note, str for some doc types) + chunk_index: Zero-based chunk index, or None to use offset fallback + chunk_start: Character offset where chunk starts (used when + chunk_index is None) + chunk_end: Character offset where chunk ends (used when chunk_index + is None) + + Returns: + Tuple of (chunk_bbox, page_number); either field may be None + independently if absent from the payload, or both may be None on + miss/error. + """ + try: + settings = get_settings() + qdrant_client = await get_qdrant_client() + + if chunk_index is not None: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition( + key="chunk_index", match=MatchValue(value=chunk_index) + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["chunk_bbox", "page_number"], + ) + else: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition( + key="chunk_start_offset", + match=MatchValue(value=chunk_start), + ), + FieldCondition( + key="chunk_end_offset", + match=MatchValue(value=chunk_end), + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["chunk_bbox", "page_number"], + ) + + points = points_response[0] + if not points or not points[0].payload: + return None, None + + payload = points[0].payload + chunk_bbox = payload.get("chunk_bbox") + page_number = payload.get("page_number") + if chunk_bbox: + logger.info( + "Found chunk bbox: page=%s, rects=%d", page_number, len(chunk_bbox) + ) + return chunk_bbox, page_number + + except Exception as e: + logger.warning("Failed to fetch chunk bbox: %s", e) + return None, None + + @dataclass class ChunkContext: """Expanded chunk with surrounding context and position markers. diff --git a/tests/unit/test_chunk_bbox_helper.py b/tests/unit/test_chunk_bbox_helper.py new file mode 100644 index 00000000..af542353 --- /dev/null +++ b/tests/unit/test_chunk_bbox_helper.py @@ -0,0 +1,215 @@ +"""Unit tests for +`nextcloud_mcp_server.search.context.get_chunk_bbox_and_page_from_qdrant`. + +Covers the two paths the helper handles: +- Indexed lookup via `chunk_index` (the preferred path post + cbcoutinho/astrolabe#75) +- Legacy offset fallback via `(chunk_start_offset, chunk_end_offset)`, which + may 400 in Qdrant Cloud strict mode + +Plus the regression case from PR #767 review: when the payload has +`chunk_bbox` but no `page_number`, the helper must surface that as +`(bbox, None)` so callers can preserve their context-derived page_number +fallback. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Import via the auth surface first to side-step the known +# `nextcloud_mcp_server.search.__init__` circular-init issue (same workaround +# used in test_chunk_context_offset_gate.py). +import nextcloud_mcp_server.auth.viz_routes # noqa: F401 +from nextcloud_mcp_server.search import context as context_module +from nextcloud_mcp_server.search.context import get_chunk_bbox_and_page_from_qdrant + +pytestmark = pytest.mark.unit + + +def _make_point(payload: dict) -> MagicMock: + point = MagicMock() + point.payload = payload + return point + + +def _patch_qdrant(scroll_return=None, scroll_side_effect=None): + qdrant_client = MagicMock() + if scroll_side_effect is not None: + qdrant_client.scroll = AsyncMock(side_effect=scroll_side_effect) + else: + qdrant_client.scroll = AsyncMock(return_value=scroll_return) + return patch.object( + context_module, + "get_qdrant_client", + new_callable=AsyncMock, + return_value=qdrant_client, + ), qdrant_client + + +class TestIndexedPath: + """When `chunk_index` is supplied, the helper must use the indexed + `chunk_index` filter (not the offset fallback).""" + + async def test_returns_bbox_and_page_when_payload_complete(self): + bbox = [[0, 0, 100, 50]] + point = _make_point({"chunk_bbox": bbox, "page_number": 7}) + ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (bbox, 7) + # One scroll call, and the filter must include chunk_index (not offsets) + qdrant_client.scroll.assert_awaited_once() + scroll_kwargs = qdrant_client.scroll.await_args.kwargs + filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must] + assert "chunk_index" in filter_keys + assert "chunk_start_offset" not in filter_keys + assert "chunk_end_offset" not in filter_keys + + +class TestOffsetFallbackPath: + """When `chunk_index` is None, the helper must use the offset filter.""" + + async def test_returns_bbox_and_page_when_payload_complete(self): + bbox = [[10, 20, 110, 70]] + point = _make_point({"chunk_bbox": bbox, "page_number": 2}) + ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="bob", + doc_id=99, + chunk_index=None, + chunk_start=500, + chunk_end=600, + ) + + assert result == (bbox, 2) + scroll_kwargs = qdrant_client.scroll.await_args.kwargs + filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must] + assert "chunk_start_offset" in filter_keys + assert "chunk_end_offset" in filter_keys + assert "chunk_index" not in filter_keys + + async def test_strict_mode_400_returns_none_pair_and_warns(self, caplog): + """Qdrant Cloud strict mode 400s on unindexed offset filters; the + helper must swallow the exception, log a warning, and degrade + gracefully so the route can still return chunk text.""" + ctx, _ = _patch_qdrant(scroll_side_effect=Exception("strict mode: 400")) + with ctx, caplog.at_level("WARNING"): + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="bob", + doc_id=99, + chunk_index=None, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records) + + +class TestPayloadShape: + """Each payload field can be missing independently — callers rely on + that to decide whether to overwrite their fallback values.""" + + async def test_empty_points_returns_none_pair(self): + ctx, _ = _patch_qdrant(scroll_return=([], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=1, + chunk_index=0, + chunk_start=0, + chunk_end=10, + ) + + assert result == (None, None) + + async def test_missing_page_returns_bbox_only(self): + """Regression for PR #767 review issue #1: when Qdrant returns a + point whose payload lacks `page_number`, the helper must return + `(bbox, None)` so callers preserve their `chunk_context.page_number` + fallback rather than clobbering it to None.""" + bbox = [[0, 0, 100, 50]] + point = _make_point({"chunk_bbox": bbox}) # no page_number + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (bbox, None) + + async def test_missing_bbox_returns_page_only(self): + point = _make_point({"page_number": 5}) # no chunk_bbox + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, 5) + + async def test_empty_payload_returns_none_pair(self): + point = _make_point({}) + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + + async def test_falsy_payload_treated_as_no_point(self): + """`if not points[0].payload` short-circuits when payload is None or + an empty dict, mirroring the original guards in the route handlers.""" + point = MagicMock() + point.payload = None + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + + +class TestExceptionHandling: + """Any error from Qdrant must produce `(None, None)` — never propagate.""" + + async def test_indexed_path_exception_returns_none_pair(self, caplog): + ctx, _ = _patch_qdrant(scroll_side_effect=RuntimeError("qdrant unavailable")) + with ctx, caplog.at_level("WARNING"): + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records)