From a27f738dbff5e49de55fd50907635ac99081f0c2 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 9 May 2026 14:32:33 +0200 Subject: [PATCH] fix(vector): tighten get_chunk_bbox_and_page_from_qdrant doc_id to str MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔴 Blocking finding from PR #773 latest review: `get_chunk_bbox_and_page_from_qdrant` (`search/context.py:199`) still declared `doc_id: int | str` and passed the raw value into `MatchValue(value=doc_id)` at lines 239 and 256 without `str()` coercion. After this branch's startup backfill normalises every Qdrant `doc_id` payload to a string, an `int` filter would silently match zero points — the function would return `(None, None)` instead of the chunk bbox / page, and PDF highlight overlays would fail in production. Take option 2 from the reviewer's two suggestions (annotation tightening over inline coercion): the producer side of this PR has already narrowed every other `doc_id` annotation to `str`, so this function is the last hold-out. Pushing the contract into the type system means `ty` will catch any future regression at the call site. Production callers in `api/visualization.py` and `auth/viz_routes.py` already pass `doc_id` (str) verbatim after the recent merge with master's chunk_index-first refactor, so no caller-side changes needed. Update the 9 calls in `tests/unit/test_chunk_bbox_helper.py` to use string literals (`"42"` / `"99"` / `"1"`) instead of integers. The mock doesn't validate `MatchValue` value types, so the tests passed with stale int doc_ids today — but they were exercising a path production no longer takes. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/context.py | 8 ++++++-- tests/unit/test_chunk_bbox_helper.py | 18 +++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index a3449b5c..f818e278 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -198,7 +198,7 @@ async def _get_deck_metadata_from_qdrant( async def get_chunk_bbox_and_page_from_qdrant( user_id: str, - doc_id: int | str, + doc_id: str, chunk_index: int | None, chunk_start: int, chunk_end: int, @@ -214,7 +214,11 @@ async def get_chunk_bbox_and_page_from_qdrant( Args: user_id: User ID who owns the document - doc_id: Document ID (int for file/note, str for some doc types) + doc_id: Document ID — always a string. Producers stringify their + native ID before writing to Qdrant so the keyword payload + index on ``doc_id`` matches every point regardless of source + doc_type. An ``int`` filter against the str-indexed payload + would silently match zero points. 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) diff --git a/tests/unit/test_chunk_bbox_helper.py b/tests/unit/test_chunk_bbox_helper.py index af542353..f7abed91 100644 --- a/tests/unit/test_chunk_bbox_helper.py +++ b/tests/unit/test_chunk_bbox_helper.py @@ -58,7 +58,7 @@ class TestIndexedPath: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -84,7 +84,7 @@ class TestOffsetFallbackPath: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="bob", - doc_id=99, + doc_id="99", chunk_index=None, chunk_start=500, chunk_end=600, @@ -105,7 +105,7 @@ class TestOffsetFallbackPath: with ctx, caplog.at_level("WARNING"): result = await get_chunk_bbox_and_page_from_qdrant( user_id="bob", - doc_id=99, + doc_id="99", chunk_index=None, chunk_start=0, chunk_end=100, @@ -124,7 +124,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=1, + doc_id="1", chunk_index=0, chunk_start=0, chunk_end=10, @@ -143,7 +143,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -157,7 +157,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -171,7 +171,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -188,7 +188,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -205,7 +205,7 @@ class TestExceptionHandling: with ctx, caplog.at_level("WARNING"): result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100,