From 51c1d42ea33a977e9056490f698efce1cd0e1e62 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 21:28:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767=20rou?= =?UTF-8?q?nd-2=20review=20=E2=80=94=20gate,=20parity,=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Latest reviewer comment flagged five items on top of the original PR. This commit addresses every one: 🟡 1. Skip the offset-based Qdrant fallback for `doc_type=file` when `chunk_index` is supplied. Qdrant Cloud's strict mode rejects unindexed filter fields with HTTP 400, which `_get_chunk_from_qdrant` catches and logs at `logger.error` — masking real Qdrant problems in monitoring. Notes/cards keep the offset fallback (cheap, useful for legacy data). 🟡 2. Add a `logger.warning` and clarifying inline comment in the doc-text fallback path when `chunk_index` is None — surfaces the pre-existing "0/N misreport" so callers can detect it. Type-nullability propagation is deferred to a follow-up (out of scope for this hotfix). 🟢 3. Simplify `if chunk_text and doc_id_int is not None:` → `if chunk_text:` with an inner `assert doc_id_int is not None` for `ty` narrowing. The outer second clause was dead. 🟢 4. Add `doc_type` `FieldCondition` to the offset-based image lookup in both `visualization.py` and `viz_routes.py` for parity with the `chunk_index` branches. 🟢 5. Inline the `chunk_filter` local in `visualization.py` directly into the `must=[]` list (matches `viz_routes.py` style). Adds `tests/unit/test_chunk_context_offset_gate.py` with three regression tests covering the gate matrix: (file, with-index → skip offset), (note, with-index → still tries offset), (file, no-index → still tries offset). Lives at top-level rather than `tests/unit/search/` to side-step a pre-existing circular-init issue in `nextcloud_mcp_server.search`. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 12 +- nextcloud_mcp_server/auth/viz_routes.py | 4 + nextcloud_mcp_server/search/context.py | 27 +++- tests/unit/test_chunk_context_offset_gate.py | 137 +++++++++++++++++++ 4 files changed, 174 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_chunk_context_offset_gate.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 949a3977..0027af66 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -578,9 +578,6 @@ async def get_chunk_context(request: Request) -> JSONResponse: # Prefer chunk_index for the highlighted-image lookup (always indexed); # fall back to (chunk_start_offset, chunk_end_offset) when not provided. if chunk_index is not None: - chunk_filter = FieldCondition( - key="chunk_index", match=MatchValue(value=chunk_index) - ) points_response = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( @@ -596,7 +593,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: key="doc_type", match=MatchValue(value=doc_type), ), - chunk_filter, + FieldCondition( + key="chunk_index", + match=MatchValue(value=chunk_index), + ), ] ), limit=1, @@ -615,6 +615,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: FieldCondition( key="user_id", match=MatchValue(value=user_id) ), + FieldCondition( + key="doc_type", + match=MatchValue(value=doc_type), + ), FieldCondition( key="chunk_start_offset", match=MatchValue(value=start), diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 5902c4d0..cb91e4b0 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -672,6 +672,10 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: FieldCondition( key="user_id", match=MatchValue(value=username) ), + FieldCondition( + key="doc_type", + match=MatchValue(value=doc_type), + ), FieldCondition( key="chunk_start_offset", match=MatchValue(value=start), diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index b9c1f301..ef4f2fbe 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -274,6 +274,11 @@ async def get_chunk_with_context( # Effective chunk_index for adjacent lookups, marker insertion, and the # response payload — keep `chunk_index is not None` distinct from this so # the gate at line ~280 still controls *whether* to take the indexed path. + # NOTE: when the caller doesn't supply chunk_index, this defaults to 0, so + # the doc-text fallback path will report the chunk as "0/N" in markers and + # the response payload regardless of its actual position. Callers that + # need accurate position metadata in the fallback path must pass + # chunk_index. See PR #767 review. effective_chunk_index = chunk_index if chunk_index is not None else 0 # Try to get chunk from Qdrant (fast path). @@ -285,12 +290,20 @@ async def get_chunk_with_context( chunk_text = await _get_chunk_by_index_from_qdrant( user_id, doc_id_int, doc_type, chunk_index ) - if chunk_text is None: + # Skip the offset fallback for files when the indexed lookup was + # already attempted: Qdrant Cloud's strict mode requires an index on + # filtered fields, and chunk_start/end_offset aren't indexed there, so + # the call returns 400 and surfaces a misleading logger.error. The + # file fast-fail below correctly handles the miss without it. + if chunk_text is None and not (chunk_index is not None and doc_type == "file"): chunk_text = await _get_chunk_from_qdrant( user_id, doc_id_int, doc_type, chunk_start, chunk_end ) - if chunk_text and doc_id_int is not None: + if chunk_text: + # chunk_text can only be non-None inside the `if doc_id_int is not None:` + # block above, so doc_id_int is guaranteed non-None here. Narrow for ty. + assert doc_id_int is not None logger.info( f"Retrieved chunk from Qdrant cache for {doc_type} {doc_id} " f"(avoids document re-fetch/re-parse)" @@ -390,6 +403,16 @@ async def get_chunk_with_context( f"(Qdrant cache miss, possibly legacy data)" ) + # When chunk_index isn't supplied, the response and markers will report + # this chunk as 0/N regardless of its actual position (effective_chunk_index + # defaulted to 0 above). Surface this so callers can detect the inaccuracy. + if chunk_index is None: + logger.warning( + f"chunk_index not supplied for {doc_type} {doc_id} doc-text " + f"fallback; position metadata in response will default to " + f"0/{total_chunks}" + ) + # Fetch full document text (notes, deck cards, news items, etc.) full_text = await _fetch_document_text(nc_client, doc_id, doc_type, user_id) if full_text is None: diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py new file mode 100644 index 00000000..3e735289 --- /dev/null +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -0,0 +1,137 @@ +"""Unit tests for `nextcloud_mcp_server.search.context.get_chunk_with_context`. + +Focused on the chunk-lookup gate that decides whether to fall back from the +indexed `chunk_index` path to the unindexed `(chunk_start, chunk_end)` path. +The behaviour matters because Qdrant Cloud's strict mode rejects filters on +unindexed fields with HTTP 400 — a fall-through there surfaces a misleading +`logger.error` even when the caller's request would correctly resolve as a +404 via the file fast-fail. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Import via the auth surface first to side-step a known circular-init issue +# in `nextcloud_mcp_server.search.__init__` when `search` is imported as the +# first entry point (also affects pre-existing tests under tests/unit/search/). +import nextcloud_mcp_server.auth.viz_routes # noqa: F401 (init-order fixup) +from nextcloud_mcp_server.search import context as context_module +from nextcloud_mcp_server.search.context import get_chunk_with_context + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def mock_nc_client() -> MagicMock: + return MagicMock() + + +class TestOffsetFallbackGate: + """When chunk_index is provided AND doc_type=='file', the offset fallback + must be skipped — see PR #767 review (🟡 spurious Qdrant error log). + """ + + async def test_file_with_chunk_index_skips_offset_fallback_on_miss( + self, mock_nc_client + ): + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value="should-not-be-returned", + ) as mock_offset, + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=12345, + doc_type="file", + chunk_start=0, + chunk_end=100, + chunk_index=3, + total_chunks=20, + ) + + assert result is None, "file fast-fail must return None on Qdrant miss" + mock_indexed.assert_awaited_once() + mock_offset.assert_not_awaited() + + async def test_note_with_chunk_index_still_uses_offset_fallback( + self, mock_nc_client + ): + """Notes/deck cards keep the offset fallback (cheap, useful for legacy + data): the gate is file-specific. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_offset, + patch.object( + context_module, + "_fetch_document_text", + new_callable=AsyncMock, + return_value=None, + ), + ): + await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=0, + chunk_end=10, + chunk_index=2, + total_chunks=5, + ) + + mock_indexed.assert_awaited_once() + mock_offset.assert_awaited_once() + + async def test_file_without_chunk_index_uses_offset_fallback(self, mock_nc_client): + """Files with no chunk_index supplied still use the offset path — + the gate only kicks in once the indexed lookup has been attempted. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_offset, + ): + await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=12345, + doc_type="file", + chunk_start=0, + chunk_end=100, + chunk_index=None, + total_chunks=20, + ) + + mock_indexed.assert_not_awaited() + mock_offset.assert_awaited_once()