From ae23bbe8b8558e7abdddfcdb3f457d3284e2926d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 10 May 2026 10:27:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(vector):=20address=20PR=20review=20round=20?= =?UTF-8?q?13=20=E2=80=94=20index=20offset=20fields=20+=20tighten=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so the legacy offset-based fallback in search/context.py works on Qdrant Cloud strict mode (pre-#75 clients have no chunk_index payload). - Cover chunk_index / chunk_start_offset / chunk_end_offset in the payload-index summary test; refresh the stale field-list comment. - Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler sites with a TODO for future non-numeric doc_types. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 3 +++ nextcloud_mcp_server/auth/viz_routes.py | 3 +++ nextcloud_mcp_server/vector/qdrant_client.py | 7 ++++++- tests/unit/vector/test_qdrant_client.py | 14 ++++++++++---- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 8203ef9a..370d4ef4 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -505,6 +505,9 @@ async def get_chunk_context(request: Request) -> JSONResponse: # ints from MySQL auto_increment; doc_id stays a str downstream # (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id # rejects "0", leading zeros, and Unicode digits that pass isdigit(). + # TODO: when chunk-context support extends to non-numeric doc_types + # (calendar VEVENT UIDs, CardDAV hrefs, …), relax this gate or make + # it doc_type-aware. Today every indexed doc_type is numeric. if not is_valid_nextcloud_doc_id(doc_id): return JSONResponse( { diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 688d03e9..f73717b9 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -567,6 +567,9 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # ints from MySQL auto_increment; doc_id stays a str downstream # (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id # rejects "0", leading zeros, and Unicode digits that pass isdigit(). + # TODO: when chunk-context support extends to non-numeric doc_types + # (calendar VEVENT UIDs, CardDAV hrefs, …), relax this gate or make + # it doc_type-aware. Today every indexed doc_type is numeric. if not is_valid_nextcloud_doc_id(doc_id): return JSONResponse( { diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index b51b74bf..7217966a 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -31,13 +31,18 @@ logger = logging.getLogger(__name__) # ``_get_chunk_by_index_from_qdrant`` and ``get_chunk_bbox_and_page_from_qdrant`` # (see search/context.py) — the always-indexed fast path that the offset-based # fallback exists to avoid; it has to actually be indexed for that promise to -# hold on Qdrant Cloud strict mode. +# hold on Qdrant Cloud strict mode. chunk_start_offset / chunk_end_offset are +# the ints used by the legacy offset fallback in the same module — pre-#75 +# clients have no chunk_index payload, so the offset path still has to work +# (or 400 silently and return None on Qdrant Cloud strict mode). _PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = { "doc_id": PayloadSchemaType.KEYWORD, "user_id": PayloadSchemaType.KEYWORD, "doc_type": PayloadSchemaType.KEYWORD, "is_placeholder": PayloadSchemaType.BOOL, "chunk_index": PayloadSchemaType.INTEGER, + "chunk_start_offset": PayloadSchemaType.INTEGER, + "chunk_end_offset": PayloadSchemaType.INTEGER, } # Sentinel point that records "this collection has been backfilled to str diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py index 6611aedc..30b71338 100644 --- a/tests/unit/vector/test_qdrant_client.py +++ b/tests/unit/vector/test_qdrant_client.py @@ -763,9 +763,10 @@ async def test_ensure_payload_indexes_summarises_failed_fields(mocker, caplog): """ client = mocker.AsyncMock() client.get_collection.return_value = SimpleNamespace(payload_schema={}) - # All but the second field fail with 5xx. _PAYLOAD_INDEX_FIELDS has - # insertion-ordered keys (doc_id, user_id, doc_type, is_placeholder), - # so call #2 (user_id) is the success case. + # _PAYLOAD_INDEX_FIELDS preserves insertion order; user_id is the second + # entry, so call #2 is the success case and every other field fails. Don't + # hard-code the full field list here — it grows as new fields move into + # the index dict, and the assertions below are what enforce coverage. call_count = {"n": 0} async def _create_index(*args, **kwargs): @@ -787,9 +788,14 @@ async def test_ensure_payload_indexes_summarises_failed_fields(mocker, caplog): if "Payload index creation incomplete" in r.getMessage() ] assert len(summary) == 1 - # All fields except user_id should appear in the summary. + # Every field that failed must appear in the summary — operators rely on + # this single log line to spot the degraded state, so any missing entry + # is a silent gap. assert "doc_id" in summary[0] assert "doc_type" in summary[0] assert "is_placeholder" in summary[0] + assert "chunk_index" in summary[0] + assert "chunk_start_offset" in summary[0] + assert "chunk_end_offset" in summary[0] assert "user_id" not in summary[0] # The one that succeeded. assert "test-collection" in summary[0]