admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(chunk-context): address PR #767 round-2 review — gate, parity, doc

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-08 21:28:50 +02:00
co-authored by Claude Opus 4.7
parent 8457c427a5
commit 51c1d42ea3
4 changed files with 174 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/api/visualization.py
+8 -4
View File
@@ -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),
nextcloud_mcp_server/auth/viz_routes.py
+4
View File
@@ -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),
nextcloud_mcp_server/search/context.py
+25 -2
View File
@@ -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:
tests/unit/test_chunk_context_offset_gate.py
+137
View File
@@ -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()