From c5020d9629d96a844efc166cf505964380af09a5 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 10 May 2026 11:50:46 +0200 Subject: [PATCH] =?UTF-8?q?fix(vector):=20address=20PR=20review=20round=20?= =?UTF-8?q?14=20=E2=80=94=20accurate=20offset-skip=20comment=20+=20news=5F?= =?UTF-8?q?item=20doc=5Fid=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-14 review surfaced one blocking and one important issue. context.py: the comment justifying `skip_offset_lookup` claimed chunk_start/end_offset weren't in _PAYLOAD_INDEX_FIELDS — round 13 indexed both as INTEGER, so the comment now actively misleads. Replace with the real reason: an indexed chunk_index miss is canonical (both paths hit the same Qdrant collection), and skipping the offset filter avoids a redundant round-trip. verification.py: hoist an is_valid_nextcloud_doc_id guard before the `int(d)` cast in _verify_news_items, mirroring the boundary-validation pattern already in _fetch_document_text. Coerce via `str(d)` because SearchResult.id is `int | str` (D1 forward-compat widening). Malformed ids now surface as a logger.warning rather than a generic debug line; fail-open semantics are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/context.py | 15 +++++++-------- nextcloud_mcp_server/search/verification.py | 10 ++++++++++ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index c233007f..11ef544c 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -374,14 +374,13 @@ async def get_chunk_with_context( chunk_text = await _get_chunk_by_index_from_qdrant( user_id, doc_id, doc_type, chunk_index ) - # When chunk_index is available, treat the indexed lookup as canonical - # for every doc_type. A miss means the chunk is genuinely absent, not - # "fall back to the unindexed slow path". chunk_start/end_offset aren't - # in _PAYLOAD_INDEX_FIELDS, so the offset filter 400s in Qdrant Cloud - # strict mode and surfaces a misleading logger.error. Legacy data - # without chunk_index (pre-cbcoutinho/astrolabe#75) still hits the - # offset path and degrades to a None chunk with a WARNING; that's the - # same behavior get_chunk_bbox_and_page_from_qdrant already documents. + # When chunk_index is supplied, the indexed lookup is canonical: both the + # index path and the offset path query the same Qdrant collection, so an + # indexed miss means the chunk is genuinely absent. Skipping the offset + # filter avoids a redundant Qdrant round-trip. Legacy data without + # chunk_index (pre-cbcoutinho/astrolabe#75) still hits the offset path + # and degrades to a None chunk with a WARNING; that's the same behavior + # get_chunk_bbox_and_page_from_qdrant already documents. skip_offset_lookup = chunk_index is not None if chunk_text is None and not skip_offset_lookup: chunk_text = await _get_chunk_from_qdrant( diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index a4b0a0d6..291b2cae 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -42,6 +42,7 @@ from nextcloud_mcp_server.search.algorithms import ( NextcloudClientProtocol, SearchResult, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.eviction import delete_document_points logger = logging.getLogger(__name__) @@ -388,6 +389,15 @@ async def _verify_news_items( # above for why this is narrower than the API-response failure path. accessible: set[str] = set() for d in doc_ids: + # SearchResult.id is `int | str` (D1: forward-compat widening). Coerce + # to str so the validator's regex applies consistently to both shapes. + if not is_valid_nextcloud_doc_id(str(d)): + logger.warning( + "Malformed news_item doc_id %r in verifier; keeping (cannot verify)", + d, + ) + accessible.add(d) + continue try: if int(d) in present_ids: accessible.add(d)