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()