From 90458b6f08fe69216cf5c9c8e2cd048e0ea2ff43 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 7 May 2026 10:58:48 +0200 Subject: [PATCH 01/12] fix(chunk-context): use indexed chunk_index lookup, fix close-after-use bug Two related bugs surfaced in production while viewing chunks from the Astrolabe frontend on the AWS-hosted MCP server: 1. PyMuPDF document closed: in _fetch_document_text the fallback path referenced pdf_doc.page_count after pdf_doc.close(), raising "document closed" and returning None. The slow PDF re-parse already completed but its result was discarded. Capture page_count into a local before close(). 2. Slow/fragile chunk lookup: get_chunk_with_context filtered Qdrant by (chunk_start_offset, chunk_end_offset). Those fields are not part of the always-indexed payload schema, and with strict_mode enabled they yield 400 errors. Even with manually-added indexes the filter is fragile if a doc is re-chunked. Switch to chunk_index (always indexed) as the primary lookup key, falling back to offset-based lookup when callers don't supply it. Plumb chunk_index/total_chunks through both the management API (api/visualization.py) and the OAuth viz route (auth/viz_routes.py). Apply the same change to the highlighted-image lookup so all four chunk-context Qdrant queries prefer the indexed field. Skip the slow PDF re-parse fallback entirely for files: when both the chunk_index and offset Qdrant lookups miss, re-downloading and re-parsing the source PDF won't find the chunk either, and routinely exceeds 30s on large documents - which is the proxy timeout in Astrolabe. Notes/cards keep the document-fetch fallback (cheap). Removes dead code (_get_file_path_from_qdrant) that was only used by the now-unreachable file fallback path. Companion change in the Astrolabe app passes chunk_index from search results through to the new endpoint params. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 86 +++++-- nextcloud_mcp_server/auth/viz_routes.py | 82 +++++-- nextcloud_mcp_server/search/context.py | 278 +++++++++------------- 3 files changed, 236 insertions(+), 210 deletions(-) diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 242c7c13..5c5951d6 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -479,6 +479,8 @@ async def get_chunk_context(request: Request) -> JSONResponse: doc_id = request.query_params.get("doc_id") start_str = request.query_params.get("start") end_str = request.query_params.get("end") + chunk_index_str = request.query_params.get("chunk_index") + total_chunks_str = request.query_params.get("total_chunks") # Validate required parameters if not all([doc_type, doc_id, start_str, end_str]): @@ -509,6 +511,14 @@ async def get_chunk_context(request: Request) -> JSONResponse: end = _parse_int_param(end_str, 0, 0, 10000000, "end") if end <= start: raise ValueError("end must be greater than start") + chunk_index: int | None = None + if chunk_index_str is not None: + chunk_index = _parse_int_param( + chunk_index_str, 0, 0, 1000000, "chunk_index" + ) + total_chunks = _parse_int_param( + total_chunks_str, 1, 1, 1000000, "total_chunks" + ) except ValueError as e: return JSONResponse({"success": False, "error": str(e)}, status_code=400) # Convert doc_id to int if possible (most IDs are int) @@ -541,6 +551,8 @@ async def get_chunk_context(request: Request) -> JSONResponse: doc_type=doc_type, chunk_start=start, chunk_end=end, + chunk_index=chunk_index, + total_chunks=total_chunks, context_chars=context_chars, ) @@ -563,30 +575,56 @@ async def get_chunk_context(request: Request) -> JSONResponse: settings = get_settings() qdrant_client = await get_qdrant_client() - # Query for this specific chunk's highlighted image - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_start_offset", match=MatchValue(value=start) - ), - FieldCondition( - key="chunk_end_offset", match=MatchValue(value=end) - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["highlighted_page_image", "page_number"], - ) + # 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( + must=[ + get_placeholder_filter(), + FieldCondition( + key="doc_id", match=MatchValue(value=doc_id_val) + ), + FieldCondition( + key="user_id", match=MatchValue(value=user_id) + ), + chunk_filter, + ] + ), + limit=1, + with_vectors=False, + with_payload=["highlighted_page_image", "page_number"], + ) + else: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition( + key="doc_id", match=MatchValue(value=doc_id_val) + ), + FieldCondition( + key="user_id", match=MatchValue(value=user_id) + ), + FieldCondition( + key="chunk_start_offset", + match=MatchValue(value=start), + ), + FieldCondition( + key="chunk_end_offset", + match=MatchValue(value=end), + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["highlighted_page_image", "page_number"], + ) if points_response[0]: payload = points_response[0][0].payload diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d072c373..21c1176a 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -535,6 +535,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: doc_id = request.query_params.get("doc_id") start_str = request.query_params.get("start") end_str = request.query_params.get("end") + chunk_index_str = request.query_params.get("chunk_index") + total_chunks_str = request.query_params.get("total_chunks") context_chars = int(request.query_params.get("context", "500")) # Validate required parameters @@ -555,6 +557,10 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: start = int(start_str) end = int(end_str) + chunk_index: int | None = ( + int(chunk_index_str) if chunk_index_str is not None else None + ) + total_chunks = int(total_chunks_str) if total_chunks_str is not None else 1 # Convert doc_id to int (all document types use int IDs) doc_id_int = int(doc_id) @@ -584,6 +590,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: doc_type=doc_type, chunk_start=start, chunk_end=end, + chunk_index=chunk_index, + total_chunks=total_chunks, context_chars=context_chars, ) @@ -613,30 +621,56 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: qdrant_client = await get_qdrant_client() username = request.user.display_name - # Query for this specific chunk's highlighted image - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) - ), - FieldCondition( - key="user_id", match=MatchValue(value=username) - ), - FieldCondition( - key="chunk_start_offset", match=MatchValue(value=start) - ), - FieldCondition( - key="chunk_end_offset", match=MatchValue(value=end) - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["highlighted_page_image", "page_number"], - ) + # 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: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition( + key="doc_id", match=MatchValue(value=doc_id_int) + ), + FieldCondition( + key="user_id", match=MatchValue(value=username) + ), + FieldCondition( + key="chunk_index", + match=MatchValue(value=chunk_index), + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["highlighted_page_image", "page_number"], + ) + else: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition( + key="doc_id", match=MatchValue(value=doc_id_int) + ), + FieldCondition( + key="user_id", match=MatchValue(value=username) + ), + FieldCondition( + key="chunk_start_offset", + match=MatchValue(value=start), + ), + FieldCondition( + key="chunk_end_offset", + match=MatchValue(value=end), + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["highlighted_page_image", "page_number"], + ) points = points_response[0] if points and points[0].payload: diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index cff7d1cc..94cd7281 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -144,63 +144,6 @@ async def _get_chunk_by_index_from_qdrant( return None -async def _get_file_path_from_qdrant( - user_id: str, file_id: int, chunk_start: int, chunk_end: int -) -> str | None: - """Resolve file_id to file_path by querying Qdrant payload. - - Args: - user_id: User ID who owns the file - file_id: Numeric file ID - chunk_start: Character offset where chunk starts - chunk_end: Character offset where chunk ends - - Returns: - File path string, or None if not found in Qdrant - """ - try: - qdrant_client = await get_qdrant_client() - settings = get_settings() - - # Query for the specific chunk - scroll_result = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - FieldCondition(key="user_id", match=MatchValue(value=user_id)), - FieldCondition(key="doc_id", match=MatchValue(value=file_id)), - FieldCondition(key="doc_type", match=MatchValue(value="file")), - FieldCondition( - key="chunk_start_offset", match=MatchValue(value=chunk_start) - ), - FieldCondition( - key="chunk_end_offset", match=MatchValue(value=chunk_end) - ), - ] - ), - limit=1, - with_payload=["file_path"], - with_vectors=False, - ) - - if scroll_result[0]: - point = scroll_result[0][0] - file_path = point.payload.get("file_path") - if file_path: - logger.debug(f"Resolved file_id {file_id} to file_path {file_path}") - return str(file_path) - - logger.warning( - f"Could not find file_path in Qdrant for file_id {file_id}, " - f"chunk [{chunk_start}:{chunk_end}]" - ) - return None - - except Exception as e: - logger.error(f"Error querying Qdrant for file_path: {e}", exc_info=True) - return None - - async def _get_deck_metadata_from_qdrant( user_id: str, card_id: int ) -> dict[str, int] | None: @@ -293,7 +236,7 @@ async def get_chunk_with_context( chunk_start: int, chunk_end: int, page_number: int | None = None, - chunk_index: int = 0, + chunk_index: int | None = None, total_chunks: int = 1, context_chars: int = 300, ) -> ChunkContext | None: @@ -311,7 +254,9 @@ async def get_chunk_with_context( chunk_start: Character offset where chunk starts chunk_end: Character offset where chunk ends page_number: Optional page number for PDFs - chunk_index: Zero-based chunk index in document + chunk_index: Zero-based chunk index in document. When provided, used as + the primary Qdrant lookup key (uses the always-indexed chunk_index + field). When None, falls back to the (chunk_start, chunk_end) lookup. total_chunks: Total number of chunks in document context_chars: Number of characters to include before/after chunk @@ -326,120 +271,125 @@ async def get_chunk_with_context( else (doc_id if isinstance(doc_id, int) else None) ) - # Try to get chunk from Qdrant first (fast path) + # Try to get chunk from Qdrant (fast path). + # Prefer chunk_index lookup (always-indexed field) when caller supplied it; + # fall back to (chunk_start, chunk_end) lookup otherwise. + chunk_text: str | None = None if doc_id_int is not None: - chunk_text = await _get_chunk_from_qdrant( - user_id, doc_id_int, doc_type, chunk_start, chunk_end + if chunk_index is not None: + chunk_text = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, chunk_index + ) + if chunk_text is None: + 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: + logger.info( + f"Retrieved chunk from Qdrant cache for {doc_type} {doc_id} " + f"(avoids document re-fetch/re-parse)" ) - if chunk_text: - logger.info( - f"Retrieved chunk from Qdrant cache for {doc_type} {doc_id} " - f"(avoids document re-fetch/re-parse)" + + # Fetch adjacent chunks for context expansion + # Get chunk overlap from config to remove duplicate text + settings = get_settings() + chunk_overlap = settings.document_chunk_overlap + + # Effective chunk_index for adjacent lookups and response (default to 0) + effective_chunk_index = chunk_index if chunk_index is not None else 0 + + before_context = "" + after_context = "" + has_before_truncation = False + has_after_truncation = False + + # Fetch previous chunk if not first chunk + if effective_chunk_index > 0: + before_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, effective_chunk_index - 1 ) - - # Fetch adjacent chunks for context expansion - # Get chunk overlap from config to remove duplicate text - settings = get_settings() - chunk_overlap = settings.document_chunk_overlap - - before_context = "" - after_context = "" - has_before_truncation = False - has_after_truncation = False - - # Fetch previous chunk if not first chunk - if chunk_index > 0: - before_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, chunk_index - 1 + if before_chunk: + # Remove overlap: the last chunk_overlap chars of previous chunk + # overlap with the first chunk_overlap chars of current chunk + before_context = ( + before_chunk[:-chunk_overlap] + if len(before_chunk) > chunk_overlap + else "" ) - if before_chunk: - # Remove overlap: the last chunk_overlap chars of previous chunk - # overlap with the first chunk_overlap chars of current chunk - before_context = ( - before_chunk[:-chunk_overlap] - if len(before_chunk) > chunk_overlap - else "" - ) - # Truncate if requested context_chars < remaining length - if before_context and len(before_context) > context_chars: - before_context = before_context[-context_chars:] - has_before_truncation = True - else: - # Could not fetch previous chunk, but we're not at start + # Truncate if requested context_chars < remaining length + if before_context and len(before_context) > context_chars: + before_context = before_context[-context_chars:] has_before_truncation = True + else: + # Could not fetch previous chunk, but we're not at start + has_before_truncation = True - # Fetch next chunk if not last chunk - if chunk_index < total_chunks - 1: - after_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, chunk_index + 1 + # Fetch next chunk if not last chunk + if effective_chunk_index < total_chunks - 1: + after_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, effective_chunk_index + 1 + ) + if after_chunk: + # Remove overlap: the first chunk_overlap chars of next chunk + # overlap with the last chunk_overlap chars of current chunk + after_context = ( + after_chunk[chunk_overlap:] + if len(after_chunk) > chunk_overlap + else "" ) - if after_chunk: - # Remove overlap: the first chunk_overlap chars of next chunk - # overlap with the last chunk_overlap chars of current chunk - after_context = ( - after_chunk[chunk_overlap:] - if len(after_chunk) > chunk_overlap - else "" - ) - # Truncate if requested context_chars < remaining length - if after_context and len(after_context) > context_chars: - after_context = after_context[:context_chars] - has_after_truncation = True - else: - # Could not fetch next chunk, but we're not at end + # Truncate if requested context_chars < remaining length + if after_context and len(after_context) > context_chars: + after_context = after_context[:context_chars] has_after_truncation = True + else: + # Could not fetch next chunk, but we're not at end + has_after_truncation = True - marked_text = _insert_position_markers( - before_context=before_context, - chunk_text=chunk_text, - after_context=after_context, - page_number=page_number, - chunk_index=chunk_index, - total_chunks=total_chunks, - has_before_truncation=has_before_truncation, - has_after_truncation=has_after_truncation, - ) - return ChunkContext( - chunk_text=chunk_text, - before_context=before_context, - after_context=after_context, - chunk_start_offset=chunk_start, - chunk_end_offset=chunk_end, - page_number=page_number, - chunk_index=chunk_index, - total_chunks=total_chunks, - marked_text=marked_text, - has_before_truncation=has_before_truncation, - has_after_truncation=has_after_truncation, - ) + marked_text = _insert_position_markers( + before_context=before_context, + chunk_text=chunk_text, + after_context=after_context, + page_number=page_number, + chunk_index=effective_chunk_index, + total_chunks=total_chunks, + has_before_truncation=has_before_truncation, + has_after_truncation=has_after_truncation, + ) + return ChunkContext( + chunk_text=chunk_text, + before_context=before_context, + after_context=after_context, + chunk_start_offset=chunk_start, + chunk_end_offset=chunk_end, + page_number=page_number, + chunk_index=effective_chunk_index, + total_chunks=total_chunks, + marked_text=marked_text, + has_before_truncation=has_before_truncation, + has_after_truncation=has_after_truncation, + ) + + # Fallback: Fetch full document and extract chunk with context. + # For files this path requires downloading and re-parsing the PDF, which + # routinely exceeds 30s on large documents. Skip it: if the chunk wasn't + # found by chunk_index OR offsets, re-parsing the PDF won't find it either + # (the chunk has been removed or re-indexed with different offsets). + if doc_type == "file": + logger.warning( + f"Chunk not found in Qdrant for file {doc_id} " + f"(chunk_index={chunk_index}, offsets={chunk_start}-{chunk_end}); " + "skipping slow PDF re-parse fallback" + ) + return None - # Fallback: Fetch full document and extract chunk with context - # This path is taken for: - # 1. Legacy data with truncated excerpts in Qdrant - # 2. Failed Qdrant queries logger.info( f"Falling back to document fetch for {doc_type} {doc_id} " f"(Qdrant cache miss, possibly legacy data)" ) - # For files, retrieve file_path from Qdrant payload - resolved_doc_id = doc_id - if doc_type == "file" and isinstance(doc_id, int): - file_path = await _get_file_path_from_qdrant( - user_id, doc_id, chunk_start, chunk_end - ) - if not file_path: - logger.warning( - f"Could not resolve file_id {doc_id} to file_path from Qdrant" - ) - return None - resolved_doc_id = file_path - logger.debug(f"Resolved file_id {doc_id} to file_path {file_path}") - - # Fetch full document text - full_text = await _fetch_document_text( - nc_client, resolved_doc_id, doc_type, user_id - ) + # 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: logger.warning( f"Could not fetch document text for {doc_type} {doc_id}, " @@ -470,13 +420,16 @@ async def get_chunk_with_context( has_before_truncation = context_start > 0 has_after_truncation = context_end < len(full_text) + # Effective chunk_index for response (default to 0 when caller didn't supply) + effective_chunk_index = chunk_index if chunk_index is not None else 0 + # Create marked text with position markers marked_text = _insert_position_markers( before_context=before_context, chunk_text=chunk_text, after_context=after_context, page_number=page_number, - chunk_index=chunk_index, + chunk_index=effective_chunk_index, total_chunks=total_chunks, has_before_truncation=has_before_truncation, has_after_truncation=has_after_truncation, @@ -489,7 +442,7 @@ async def get_chunk_with_context( chunk_start_offset=chunk_start, chunk_end_offset=chunk_end, page_number=page_number, - chunk_index=chunk_index, + chunk_index=effective_chunk_index, total_chunks=total_chunks, marked_text=marked_text, has_before_truncation=has_before_truncation, @@ -538,9 +491,10 @@ async def _fetch_document_text( logger.debug(f"Extracting text from PDF: {file_path}") pdf_doc = pymupdf.open(stream=file_content, filetype="pdf") text_parts = [] + page_count = pdf_doc.page_count # Extract each page as markdown (same as indexing) - for page_num in range(pdf_doc.page_count): + for page_num in range(page_count): page_md = pymupdf4llm.to_markdown( pdf_doc, pages=[page_num], @@ -555,7 +509,7 @@ async def _fetch_document_text( full_text = "".join(text_parts) logger.debug( f"Extracted {len(full_text)} characters from " - f"{pdf_doc.page_count} pages in {file_path}" + f"{page_count} pages in {file_path}" ) return full_text else: From a33a365a699779c6e83094811d4b8f483fbdda35 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 18:35:20 +0200 Subject: [PATCH 02/12] fix(viz_routes): validate chunk_index/total_chunks bounds in OAuth route PR #767 review noted that the OAuth viz route used bare int() parsing for chunk_index and total_chunks while the bearer-token visualization route validates them via _parse_int_param. Mirror the same bounds check so total_chunks=0 and negative chunk_index return 400 instead of silently suppressing adjacent-chunk context. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/viz_routes.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 21c1176a..331173b2 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -23,6 +23,7 @@ from starlette.authentication import requires from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse +from nextcloud_mcp_server.api.management import _parse_int_param from nextcloud_mcp_server.auth.userinfo_routes import ( _get_authenticated_client_for_userinfo, ) @@ -557,10 +558,12 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: start = int(start_str) end = int(end_str) - chunk_index: int | None = ( - int(chunk_index_str) if chunk_index_str is not None else None - ) - total_chunks = int(total_chunks_str) if total_chunks_str is not None else 1 + chunk_index: int | None = None + if chunk_index_str is not None: + chunk_index = _parse_int_param( + chunk_index_str, 0, 0, 1000000, "chunk_index" + ) + total_chunks = _parse_int_param(total_chunks_str, 1, 1, 1000000, "total_chunks") # Convert doc_id to int (all document types use int IDs) doc_id_int = int(doc_id) From 53e6dba5a2e272067851bb8eefff61cde428ae29 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 19:14:43 +0200 Subject: [PATCH 03/12] =?UTF-8?q?fix(viz=5Froutes):=20address=20PR=20#767?= =?UTF-8?q?=20review=20=E2=80=94=20param=20parity=20+=20always-on=20page?= =?UTF-8?q?=5Fnumber?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace bare int() casts for start/end/context_chars in chunk_context_endpoint with _parse_int_param, matching visualization.py bounds (0–10M for offsets, 0–10K for context_chars), and add the missing end > start guard. - Initialize page_number from chunk_context.page_number so non-file doc_types surface it; include page_number, chunk_index, and total_chunks unconditionally in the response. Only highlighted_page_image stays gated on its own truthiness. - Add a chunk_index forwarding regression test that asserts the new kwargs reach get_chunk_with_context and appear in the response payload. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/viz_routes.py | 21 +++++--- .../test_management_chunk_context_endpoint.py | 52 +++++++++++++++++++ 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 331173b2..95256286 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -538,7 +538,6 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: end_str = request.query_params.get("end") chunk_index_str = request.query_params.get("chunk_index") total_chunks_str = request.query_params.get("total_chunks") - context_chars = int(request.query_params.get("context", "500")) # Validate required parameters if not all([doc_type, doc_id, start_str, end_str]): @@ -556,8 +555,17 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: assert start_str is not None assert end_str is not None - start = int(start_str) - end = int(end_str) + context_chars = _parse_int_param( + request.query_params.get("context"), + 500, + 0, + 10000, + "context_chars", + ) + start = _parse_int_param(start_str, 0, 0, 10000000, "start") + end = _parse_int_param(end_str, 0, 0, 10000000, "end") + if end <= start: + raise ValueError("end must be greater than start") chunk_index: int | None = None if chunk_index_str is not None: chunk_index = _parse_int_param( @@ -617,7 +625,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # For PDF files, also fetch the highlighted page image from Qdrant highlighted_page_image = None - page_number = None + page_number = chunk_context.page_number if doc_type == "file": try: settings = get_settings() @@ -697,12 +705,13 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: "after_context": chunk_context.after_context, "has_more_before": chunk_context.has_before_truncation, "has_more_after": chunk_context.has_after_truncation, + "page_number": page_number, + "chunk_index": chunk_context.chunk_index, + "total_chunks": chunk_context.total_chunks, } - # Add image data if available if highlighted_page_image: response_data["highlighted_page_image"] = highlighted_page_image - response_data["page_number"] = page_number return JSONResponse(response_data) diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py index e387a91a..c335fce0 100644 --- a/tests/unit/test_management_chunk_context_endpoint.py +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -259,6 +259,58 @@ class TestChunkContextCredentialPath: assert "failed to fetch chunk context" in data["error"].lower() +class TestChunkContextParameterForwarding: + """Verify new chunk_index / total_chunks query params reach the lookup. + + Regression guard for PR #767: the whole point of the fix is that callers + pass chunk_index, and it must arrive at get_chunk_with_context as the + primary Qdrant lookup key. + """ + + def test_chunk_index_and_total_chunks_forwarded(self): + mock_nc_client = _make_mock_nc_client() + mock_ctx = _make_mock_chunk_context() + mock_ctx.chunk_index = 7 + mock_ctx.total_chunks = 10 + + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.api.visualization.get_chunk_with_context", + new_callable=AsyncMock, + return_value=mock_ctx, + ) as mock_get_chunk, + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=note&doc_id=42" + "&start=0&end=10&chunk_index=7&total_chunks=10", + headers={"Authorization": "Bearer test-token"}, + ) + + assert response.status_code == 200 + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["chunk_index"] == 7 + assert kwargs["total_chunks"] == 10 + + data = response.json() + assert data["chunk_index"] == 7 + assert data["total_chunks"] == 10 + # page_number must be present even when None (frontend may scroll + # by it for non-file doc types) + assert "page_number" in data + + class TestChunkContextConfigErrors: """Tests for configuration failure paths.""" From 8457c427a50cfb51aa92a2bf903c645971b31ba6 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 20:50:27 +0200 Subject: [PATCH 04/12] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767?= =?UTF-8?q?=20review=20=E2=80=94=20doc=5Ftype=20filter=20parity=20+=20test?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `doc_type` FieldCondition to the chunk_index-path highlighted-image Qdrant filter in both `api/visualization.py` and `auth/viz_routes.py`, matching the shape of `_get_chunk_by_index_from_qdrant`. Safe today (the block is guarded by `doc_type == "file"` and Nextcloud file IDs are globally unique) but prevents a latent bug if other doc types start storing highlighted images. - Demote `viz_routes.py` `ValueError` log from `error` to `warning` (lazy %-style) — `_parse_int_param` raises on user-supplied bad input, which is a 400 not a server error and shouldn't pollute error logs. - Hoist `effective_chunk_index` to compute once at the top of `get_chunk_with_context`, removing two duplicate assignments. - Add `test_file_doc_type_qdrant_miss_yields_fast_404` to the management endpoint tests, locking in the proxy-timeout fix contract. - Add `tests/unit/test_viz_routes_chunk_context.py` mirroring management coverage for the OAuth-session route: param forwarding (chunk_index / total_chunks), `doc_type=file` fast 404, and 400 on invalid int params. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 4 + nextcloud_mcp_server/auth/viz_routes.py | 7 +- nextcloud_mcp_server/search/context.py | 11 +- .../test_management_chunk_context_endpoint.py | 43 ++++ tests/unit/test_viz_routes_chunk_context.py | 194 ++++++++++++++++++ 5 files changed, 252 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_viz_routes_chunk_context.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 5c5951d6..949a3977 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -592,6 +592,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), + ), chunk_filter, ] ), diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 95256286..5902c4d0 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -646,6 +646,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_index", match=MatchValue(value=chunk_index), @@ -716,7 +720,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: return JSONResponse(response_data) except ValueError as e: - logger.error(f"Invalid parameter format: {e}") + # User-supplied bad input → 400, not a server error. + logger.warning("Invalid parameter format: %s", e) return JSONResponse( {"success": False, "error": f"Invalid parameter format: {e}"}, status_code=400, diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 94cd7281..b9c1f301 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -271,6 +271,11 @@ async def get_chunk_with_context( else (doc_id if isinstance(doc_id, int) else None) ) + # 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. + effective_chunk_index = chunk_index if chunk_index is not None else 0 + # Try to get chunk from Qdrant (fast path). # Prefer chunk_index lookup (always-indexed field) when caller supplied it; # fall back to (chunk_start, chunk_end) lookup otherwise. @@ -296,9 +301,6 @@ async def get_chunk_with_context( settings = get_settings() chunk_overlap = settings.document_chunk_overlap - # Effective chunk_index for adjacent lookups and response (default to 0) - effective_chunk_index = chunk_index if chunk_index is not None else 0 - before_context = "" after_context = "" has_before_truncation = False @@ -420,9 +422,6 @@ async def get_chunk_with_context( has_before_truncation = context_start > 0 has_after_truncation = context_end < len(full_text) - # Effective chunk_index for response (default to 0 when caller didn't supply) - effective_chunk_index = chunk_index if chunk_index is not None else 0 - # Create marked text with position markers marked_text = _insert_position_markers( before_context=before_context, diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py index c335fce0..7e5d7d6b 100644 --- a/tests/unit/test_management_chunk_context_endpoint.py +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -258,6 +258,49 @@ class TestChunkContextCredentialPath: assert data["success"] is False assert "failed to fetch chunk context" in data["error"].lower() + def test_file_doc_type_qdrant_miss_yields_fast_404(self): + """For doc_type=file, a Qdrant miss must surface as 404 immediately + (no slow PDF re-parse fallback). Locks the proxy-timeout fix in. + + At the unit level we only assert the response shape; the + no-fallback contract itself lives in `search/context.py` and is + exercised by chunk-context tests there. + """ + mock_nc_client = _make_mock_nc_client() + + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.api.visualization.get_chunk_with_context", + new_callable=AsyncMock, + return_value=None, + ) as mock_get_chunk, + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=file&doc_id=12345" + "&start=0&end=10&chunk_index=3&total_chunks=20", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 404 + data = response.json() + assert data["success"] is False + # Confirm the handler called the resolver with doc_type=file + # (not a coerced/normalized value) so the fast-fail path engages. + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["doc_type"] == "file" + assert kwargs["chunk_index"] == 3 + class TestChunkContextParameterForwarding: """Verify new chunk_index / total_chunks query params reach the lookup. diff --git a/tests/unit/test_viz_routes_chunk_context.py b/tests/unit/test_viz_routes_chunk_context.py new file mode 100644 index 00000000..26e550cc --- /dev/null +++ b/tests/unit/test_viz_routes_chunk_context.py @@ -0,0 +1,194 @@ +"""Unit tests for the OAuth-session chunk-context endpoint +(`nextcloud_mcp_server.auth.viz_routes.chunk_context_endpoint`). + +Mirrors the regression coverage of +`tests/unit/test_management_chunk_context_endpoint.py` (which targets the +management API route in `nextcloud_mcp_server.api.visualization`). + +Both routes share the same purpose — fetch chunk text with surrounding +context for the viz pane — but live behind different auth surfaces: + +* Management API: OAuth bearer validated by `validate_token_and_get_user` +* Viz route: Starlette session auth via `@requires("authenticated")` + +Because of the auth-middleware difference, a separate file is cleaner than +mixing both styles into one test module. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from starlette.applications import Starlette +from starlette.authentication import ( + AuthCredentials, + AuthenticationBackend, + SimpleUser, +) +from starlette.middleware import Middleware +from starlette.middleware.authentication import AuthenticationMiddleware +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.auth.viz_routes import chunk_context_endpoint + +pytestmark = pytest.mark.unit + + +class _AlwaysAuthBackend(AuthenticationBackend): + """Stub auth backend: every request is authenticated as `testuser`.""" + + async def authenticate(self, conn): + return AuthCredentials(["authenticated"]), SimpleUser("testuser") + + +def _make_app() -> Starlette: + return Starlette( + routes=[ + Route("/app/chunk-context", chunk_context_endpoint, methods=["GET"]), + ], + middleware=[ + Middleware(AuthenticationMiddleware, backend=_AlwaysAuthBackend()), + ], + ) + + +def _make_mock_chunk_context(chunk_text="chunk", before="before", after="after"): + """Mock a ChunkContext dataclass with enough fields for the handler.""" + ctx = MagicMock() + ctx.chunk_text = chunk_text + ctx.before_context = before + ctx.after_context = after + ctx.has_before_truncation = False + ctx.has_after_truncation = False + ctx.page_number = None + ctx.chunk_index = 0 + ctx.total_chunks = 1 + return ctx + + +def _make_mock_nc_client(): + """Mock NextcloudClient that supports `async with`.""" + mock_client = MagicMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + return mock_client + + +def _make_mock_settings(nextcloud_host: str = "http://localhost:8080") -> MagicMock: + """Mock get_settings() return value with the fields the handler reads.""" + settings = MagicMock() + settings.nextcloud_host = nextcloud_host + settings.get_collection_name.return_value = "test-collection" + return settings + + +class TestVizChunkContextParameterForwarding: + """Regression guard mirroring TestChunkContextParameterForwarding for the + management API: chunk_index / total_chunks must reach get_chunk_with_context. + """ + + def test_chunk_index_and_total_chunks_forwarded(self): + mock_nc_client = _make_mock_nc_client() + mock_ctx = _make_mock_chunk_context() + mock_ctx.chunk_index = 7 + mock_ctx.total_chunks = 10 + + with ( + patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context", + new_callable=AsyncMock, + return_value=mock_ctx, + ) as mock_get_chunk, + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=note&doc_id=42" + "&start=0&end=10&chunk_index=7&total_chunks=10" + ) + + assert response.status_code == 200 + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["chunk_index"] == 7 + assert kwargs["total_chunks"] == 10 + + data = response.json() + assert data["chunk_index"] == 7 + assert data["total_chunks"] == 10 + # page_number must be present even when None — the response shape + # is unconditional so the frontend can rely on the key existing. + assert "page_number" in data + + +class TestVizChunkContextFile404: + """When get_chunk_with_context returns None for doc_type=file, the route + must surface a fast 404 — no slow PDF re-parse fallback. This guards the + proxy-timeout fix from PR #767. + """ + + def test_file_doc_type_qdrant_miss_yields_fast_404(self): + mock_nc_client = _make_mock_nc_client() + + with ( + patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context", + new_callable=AsyncMock, + return_value=None, + ) as mock_get_chunk, + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=file&doc_id=12345" + "&start=0&end=10&chunk_index=3&total_chunks=20" + ) + + assert response.status_code == 404 + data = response.json() + assert data["success"] is False + assert "failed to fetch chunk context" in data["error"].lower() + + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["doc_type"] == "file" + assert kwargs["chunk_index"] == 3 + assert kwargs["total_chunks"] == 20 + + +class TestVizChunkContextValueErrorLogging: + """Verify the route returns 400 for malformed integer params (and does + not crash with a 500). The log-level demotion (logger.warning) is + asserted indirectly via response shape — log-level itself is not a + behaviour the user can observe through HTTP. + """ + + def test_invalid_int_param_returns_400(self): + with patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=note&doc_id=1" + "&start=not-a-number&end=10" + ) + + assert response.status_code == 400 + data = response.json() + assert data["success"] is False + assert "invalid" in data["error"].lower() From 51c1d42ea33a977e9056490f698efce1cd0e1e62 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 21:28:50 +0200 Subject: [PATCH 05/12] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767?= =?UTF-8?q?=20round-2=20review=20=E2=80=94=20gate,=20parity,=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- nextcloud_mcp_server/api/visualization.py | 12 +- nextcloud_mcp_server/auth/viz_routes.py | 4 + nextcloud_mcp_server/search/context.py | 27 +++- tests/unit/test_chunk_context_offset_gate.py | 137 +++++++++++++++++++ 4 files changed, 174 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_chunk_context_offset_gate.py 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() From 45da876cf5e9c19e341f9f1385754b0eae1dfe0e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 22:16:00 +0200 Subject: [PATCH 06/12] fix(chunk-context): propagate chunk_index=None through ChunkContext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the deferred half of PR #767 review issue 2: instead of just documenting the "0/N misreport" with a logger.warning, propagate the caller's None for chunk_index through the dataclass, position markers, and response builders so callers can distinguish "unknown position" from "actually chunk 0". Changes: - ChunkContext.chunk_index: int → int | None - _insert_position_markers: chunk_index parameter is int | None; when None, renders "Chunk ?/N" instead of "Chunk 1 of N" - get_chunk_with_context: drops the effective_chunk_index local entirely. Passes chunk_index (may be None) directly into both ChunkContext and _insert_position_markers, in both the Qdrant fast path and the doc-text fallback. - Fast path: when chunk_index is None and chunk_text was retrieved via the offset lookup (notes/cards), skip the adjacent-chunk fetch. Index arithmetic from a default 0 would query the chunks at positions -1 and 1 even when the actual chunk is, say, 5/20 — silently producing wrong "before"/"after" text. Mark both sides as truncated instead. - Drop the now-redundant logger.warning in the doc-text fallback (the response correctly communicates the unknown state via chunk_index=None). Both existing response builders (`api/visualization.py:657` and `auth/viz_routes.py:717`) already serialise `chunk_context.chunk_index` unconditionally; `None` becomes JSON `null`. No route changes needed. Adds 5 regression tests: - ChunkContext.chunk_index propagates as None in fast path - ChunkContext.chunk_index propagates as None in doc-text fallback - Fast path with chunk_index renders "Chunk N of M" correctly - _insert_position_markers renders "?/N" for None chunk_index - _insert_position_markers renders explicit index when supplied Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/context.py | 126 +++++++------- tests/unit/test_chunk_context_offset_gate.py | 165 +++++++++++++++++++ 2 files changed, 226 insertions(+), 65 deletions(-) diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index ef4f2fbe..5b5bdaa7 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -208,7 +208,10 @@ class ChunkContext: chunk_start_offset: Character position where chunk starts in document chunk_end_offset: Character position where chunk ends in document page_number: Page number for PDFs (None for other doc types) - chunk_index: Zero-based chunk index (N in "chunk N of M") + chunk_index: Zero-based chunk index (N in "chunk N of M"). None when + the caller didn't supply chunk_index and we couldn't determine it + from the lookup path — distinguishes "unknown position" from + "actually chunk 0". total_chunks: Total number of chunks in document marked_text: Full text with position markers around the chunk has_before_truncation: True if before_context was truncated @@ -221,7 +224,7 @@ class ChunkContext: chunk_start_offset: int chunk_end_offset: int page_number: int | None - chunk_index: int + chunk_index: int | None total_chunks: int marked_text: str has_before_truncation: bool @@ -271,16 +274,6 @@ async def get_chunk_with_context( else (doc_id if isinstance(doc_id, int) else None) ) - # 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). # Prefer chunk_index lookup (always-indexed field) when caller supplied it; # fall back to (chunk_start, chunk_end) lookup otherwise. @@ -319,54 +312,63 @@ async def get_chunk_with_context( has_before_truncation = False has_after_truncation = False - # Fetch previous chunk if not first chunk - if effective_chunk_index > 0: - before_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, effective_chunk_index - 1 - ) - if before_chunk: - # Remove overlap: the last chunk_overlap chars of previous chunk - # overlap with the first chunk_overlap chars of current chunk - before_context = ( - before_chunk[:-chunk_overlap] - if len(before_chunk) > chunk_overlap - else "" + if chunk_index is not None: + # Fetch previous chunk if not first chunk + if chunk_index > 0: + before_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, chunk_index - 1 ) - # Truncate if requested context_chars < remaining length - if before_context and len(before_context) > context_chars: - before_context = before_context[-context_chars:] + if before_chunk: + # Remove overlap: the last chunk_overlap chars of previous chunk + # overlap with the first chunk_overlap chars of current chunk + before_context = ( + before_chunk[:-chunk_overlap] + if len(before_chunk) > chunk_overlap + else "" + ) + # Truncate if requested context_chars < remaining length + if before_context and len(before_context) > context_chars: + before_context = before_context[-context_chars:] + has_before_truncation = True + else: + # Could not fetch previous chunk, but we're not at start has_before_truncation = True - else: - # Could not fetch previous chunk, but we're not at start - has_before_truncation = True - # Fetch next chunk if not last chunk - if effective_chunk_index < total_chunks - 1: - after_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, effective_chunk_index + 1 - ) - if after_chunk: - # Remove overlap: the first chunk_overlap chars of next chunk - # overlap with the last chunk_overlap chars of current chunk - after_context = ( - after_chunk[chunk_overlap:] - if len(after_chunk) > chunk_overlap - else "" + # Fetch next chunk if not last chunk + if chunk_index < total_chunks - 1: + after_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, chunk_index + 1 ) - # Truncate if requested context_chars < remaining length - if after_context and len(after_context) > context_chars: - after_context = after_context[:context_chars] + if after_chunk: + # Remove overlap: the first chunk_overlap chars of next chunk + # overlap with the last chunk_overlap chars of current chunk + after_context = ( + after_chunk[chunk_overlap:] + if len(after_chunk) > chunk_overlap + else "" + ) + # Truncate if requested context_chars < remaining length + if after_context and len(after_context) > context_chars: + after_context = after_context[:context_chars] + has_after_truncation = True + else: + # Could not fetch next chunk, but we're not at end has_after_truncation = True - else: - # Could not fetch next chunk, but we're not at end - has_after_truncation = True + else: + # No chunk_index → can't fetch adjacent chunks via index arithmetic + # without risking wrong neighbours (a default of 0 would query the + # chunks at positions -1 and 1 even when the actual chunk is, say, + # 5/20). Mark both sides as truncated so the caller knows context + # wasn't expanded. + has_before_truncation = True + has_after_truncation = True marked_text = _insert_position_markers( before_context=before_context, chunk_text=chunk_text, after_context=after_context, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, has_before_truncation=has_before_truncation, has_after_truncation=has_after_truncation, @@ -378,7 +380,7 @@ async def get_chunk_with_context( chunk_start_offset=chunk_start, chunk_end_offset=chunk_end, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, marked_text=marked_text, has_before_truncation=has_before_truncation, @@ -403,16 +405,6 @@ 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: @@ -451,7 +443,7 @@ async def get_chunk_with_context( chunk_text=chunk_text, after_context=after_context, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, has_before_truncation=has_before_truncation, has_after_truncation=has_after_truncation, @@ -464,7 +456,7 @@ async def get_chunk_with_context( chunk_start_offset=chunk_start, chunk_end_offset=chunk_end, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, marked_text=marked_text, has_before_truncation=has_before_truncation, @@ -644,7 +636,7 @@ def _insert_position_markers( chunk_text: str, after_context: str, page_number: int | None, - chunk_index: int, + chunk_index: int | None, total_chunks: int, has_before_truncation: bool, has_after_truncation: bool, @@ -659,7 +651,8 @@ def _insert_position_markers( chunk_text: The matched chunk after_context: Text after chunk page_number: Optional page number - chunk_index: Zero-based chunk index + chunk_index: Zero-based chunk index, or None when the caller didn't + supply it (rendered as "Chunk ?/N" instead of "Chunk 0/N"). total_chunks: Total chunks in document has_before_truncation: Whether before_context is truncated has_after_truncation: Whether after_context is truncated @@ -671,7 +664,10 @@ def _insert_position_markers( position_parts = [] if page_number is not None: position_parts.append(f"Page {page_number}") - position_parts.append(f"Chunk {chunk_index + 1} of {total_chunks}") + if chunk_index is None: + position_parts.append(f"Chunk ?/{total_chunks}") + else: + position_parts.append(f"Chunk {chunk_index + 1} of {total_chunks}") position_metadata = ", ".join(position_parts) # Build marked text diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py index 3e735289..3f39d303 100644 --- a/tests/unit/test_chunk_context_offset_gate.py +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -135,3 +135,168 @@ class TestOffsetFallbackGate: mock_indexed.assert_not_awaited() mock_offset.assert_awaited_once() + + +class TestNullableChunkIndexPropagation: + """When the caller doesn't supply chunk_index, it must propagate as None + through to ChunkContext and the position markers — distinguishing + "unknown position" from "actually chunk 0". See PR #767 review (🟡 issue 2). + """ + + async def test_fast_path_without_chunk_index_returns_none_in_response( + self, mock_nc_client + ): + """Note retrieved via offset fallback (chunk_index=None) → response + chunk_index is None, markers render '?/N', and adjacent fetch is + skipped (would otherwise produce wrong neighbours from index 0). + """ + 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="matched chunk text", + ), + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=100, + chunk_end=200, + chunk_index=None, + total_chunks=8, + ) + + assert result is not None + assert result.chunk_index is None, ( + "chunk_index must propagate as None, not default to 0" + ) + assert "Chunk ?/8" in result.marked_text + assert "Chunk 1 of 8" not in result.marked_text + # Adjacent fetch must be skipped — index arithmetic from 0 would + # query the wrong neighbours when actual position isn't 0. + mock_indexed.assert_not_awaited() + assert result.has_before_truncation is True + assert result.has_after_truncation is True + + async def test_fast_path_with_chunk_index_renders_position_correctly( + self, mock_nc_client + ): + """Counter-positive: when chunk_index is supplied, response carries + the value and markers render the explicit "Chunk N of M". + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + side_effect=[ + "current chunk text", # primary lookup + "previous chunk text", # adjacent before + "next chunk text", # adjacent after + ], + ), + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + ): + result = 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=5, + total_chunks=20, + ) + + assert result is not None + assert result.chunk_index == 5 + assert "Chunk 6 of 20" in result.marked_text + + async def test_doc_text_fallback_without_chunk_index_returns_none( + self, mock_nc_client + ): + """Doc-text fallback (Qdrant miss → re-fetch document) must also + propagate chunk_index=None into the response so callers can tell + the position is unknown. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + patch.object( + context_module, + "_fetch_document_text", + new_callable=AsyncMock, + return_value="x" * 500, + ), + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=100, + chunk_end=200, + chunk_index=None, + total_chunks=10, + ) + + assert result is not None + assert result.chunk_index is None + assert "Chunk ?/10" in result.marked_text + + +class TestPositionMarkers: + """Direct tests for `_insert_position_markers` rendering when chunk_index + is None vs explicit. + """ + + def test_marker_renders_question_mark_when_chunk_index_is_none(self): + text = context_module._insert_position_markers( + before_context="", + chunk_text="x", + after_context="", + page_number=None, + chunk_index=None, + total_chunks=12, + has_before_truncation=False, + has_after_truncation=False, + ) + assert "Chunk ?/12" in text + + def test_marker_renders_explicit_index_when_supplied(self): + text = context_module._insert_position_markers( + before_context="", + chunk_text="x", + after_context="", + page_number=3, + chunk_index=4, + total_chunks=12, + has_before_truncation=False, + has_after_truncation=False, + ) + assert "Page 3" in text + assert "Chunk 5 of 12" in text From 47b0b737b6526b6513e86444d946b1c4e18f7e4f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 23:21:30 +0200 Subject: [PATCH 07/12] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767?= =?UTF-8?q?=20round-3=20review=20=E2=80=94=20gate=20readability=20+=20lega?= =?UTF-8?q?cy-fallback=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - search/context.py: rename triple-negation gate condition to `skip_offset_lookup` named boolean for readability; convert new logger.warning to lazy %-style per repo convention. - api/visualization.py, auth/viz_routes.py: add comment on the offset-only Qdrant scroll branch noting it is a legacy path for pre-astrolabe#75 clients and degrades gracefully on Qdrant Cloud strict mode. Reviewer item #2 (extracting the duplicated scroll block into a shared helper) deferred to a follow-up issue per the reviewer's "not blocking" framing. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 6 ++++++ nextcloud_mcp_server/auth/viz_routes.py | 6 ++++++ nextcloud_mcp_server/search/context.py | 22 +++++++++++++--------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 95199429..309ef2de 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -601,6 +601,12 @@ async def get_chunk_context(request: Request) -> JSONResponse: with_payload=["chunk_bbox", "page_number"], ) else: + # Legacy fallback for clients that don't send chunk_index + # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset + # aren't indexed in Qdrant Cloud strict mode, so this + # call may fail with HTTP 400 there; the outer except + # logs a warning and the response degrades gracefully + # (no chunk_bbox). points_response = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index b3a33886..8f8d0b72 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -659,6 +659,12 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: with_payload=["chunk_bbox", "page_number"], ) else: + # Legacy fallback for clients that don't send chunk_index + # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset + # aren't indexed in Qdrant Cloud strict mode, so this + # call may fail with HTTP 400 there; the outer except + # logs a warning and the response degrades gracefully + # (no chunk_bbox). points_response = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 5b5bdaa7..be023df8 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -283,12 +283,13 @@ async def get_chunk_with_context( chunk_text = await _get_chunk_by_index_from_qdrant( user_id, doc_id_int, doc_type, chunk_index ) - # 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"): + # Skip the offset fallback for files when the indexed chunk_index + # lookup already ran: chunk_start/end_offset aren't indexed in Qdrant + # Cloud strict mode, so the call returns 400 and surfaces a misleading + # logger.error. The file fast-fail below correctly handles the miss + # without it. + skip_offset_lookup = chunk_index is not None and doc_type == "file" + if chunk_text is None and not skip_offset_lookup: chunk_text = await _get_chunk_from_qdrant( user_id, doc_id_int, doc_type, chunk_start, chunk_end ) @@ -394,9 +395,12 @@ async def get_chunk_with_context( # (the chunk has been removed or re-indexed with different offsets). if doc_type == "file": logger.warning( - f"Chunk not found in Qdrant for file {doc_id} " - f"(chunk_index={chunk_index}, offsets={chunk_start}-{chunk_end}); " - "skipping slow PDF re-parse fallback" + "Chunk not found in Qdrant for file %s (chunk_index=%s, " + "offsets=%s-%s); skipping slow PDF re-parse fallback", + doc_id, + chunk_index, + chunk_start, + chunk_end, ) return None From c780f96d2b0a8dff3d493a9811008a942e44bb6a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 9 May 2026 00:07:51 +0200 Subject: [PATCH 08/12] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767?= =?UTF-8?q?=20review=20=E2=80=94=20drop=20dead=20PDF=20branch,=20redundant?= =?UTF-8?q?=20alias,=20add=20boundary=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unreachable doc_type=="file" branch (and pymupdf/pymupdf4llm imports) from _fetch_document_text in search/context.py — the file path is short-circuited in get_chunk_with_context before reaching it. - Drop the redundant `username = request.user.display_name` alias in auth/viz_routes.py; both Qdrant scroll filters now reference user_id consistently with the rest of the handler. - Add TestAdjacentChunkBoundary in tests/unit/test_chunk_context_offset_gate.py covering chunk_index=0 (before-fetch gate closed) and chunk_index=total_chunks-1 (after-fetch gate closed) — the two off-by-one boundaries previously untested. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/viz_routes.py | 5 +- nextcloud_mcp_server/search/context.py | 59 ++----------- tests/unit/test_chunk_context_offset_gate.py | 89 ++++++++++++++++++++ 3 files changed, 97 insertions(+), 56 deletions(-) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 8f8d0b72..d1a369bb 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -632,7 +632,6 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: try: settings = get_settings() qdrant_client = await get_qdrant_client() - username = request.user.display_name # Prefer chunk_index for the chunk-bbox lookup (always indexed); # fall back to (chunk_start_offset, chunk_end_offset) when not provided. @@ -646,7 +645,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: key="doc_id", match=MatchValue(value=doc_id_int) ), FieldCondition( - key="user_id", match=MatchValue(value=username) + key="user_id", match=MatchValue(value=user_id) ), FieldCondition( key="chunk_index", @@ -674,7 +673,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: key="doc_id", match=MatchValue(value=doc_id_int) ), FieldCondition( - key="user_id", match=MatchValue(value=username) + key="user_id", match=MatchValue(value=user_id) ), FieldCondition( key="chunk_start_offset", diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index be023df8..1a5fcaff 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -7,8 +7,6 @@ position markers for better visualization and understanding of search results. import logging from dataclasses import dataclass -import pymupdf -import pymupdf4llm from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.client import NextcloudClient @@ -473,10 +471,14 @@ async def _fetch_document_text( ) -> str | None: """Fetch full text content of a document. + Note: doc_type=="file" is short-circuited in get_chunk_with_context before + this function is called (re-parsing PDFs is too slow for the request + timeout), so no file branch exists here. + Args: nc_client: Authenticated Nextcloud client - doc_id: Document ID (note ID or file path) - doc_type: Type of document ("note", "file", etc.) + doc_id: Document ID + doc_type: Type of document ("note", "news_item", "deck_card") Returns: Full document text, or None if document cannot be retrieved @@ -490,55 +492,6 @@ async def _fetch_document_text( title = note.get("title", "") content = note.get("content", "") return f"{title}\n\n{content}" - elif doc_type == "file": - # Fetch file content via WebDAV - try: - file_path = str(doc_id) - file_content, content_type = await nc_client.webdav.read_file(file_path) - - # Check if it's a PDF (by content type or file extension) - is_pdf = ( - content_type and "pdf" in content_type.lower() - ) or file_path.lower().endswith(".pdf") - - if is_pdf: - # Extract text from PDF using PyMuPDF - # IMPORTANT: Use pymupdf4llm.to_markdown() to match indexing extraction - # This ensures character offsets align between indexed chunks and retrieval - - logger.debug(f"Extracting text from PDF: {file_path}") - pdf_doc = pymupdf.open(stream=file_content, filetype="pdf") - text_parts = [] - page_count = pdf_doc.page_count - - # Extract each page as markdown (same as indexing) - for page_num in range(page_count): - page_md = pymupdf4llm.to_markdown( - pdf_doc, - pages=[page_num], - write_images=False, # Don't need images for context - page_chunks=False, - ) - text_parts.append(page_md) - - pdf_doc.close() - - # Join pages (no separator - matches indexing) - full_text = "".join(text_parts) - logger.debug( - f"Extracted {len(full_text)} characters from " - f"{page_count} pages in {file_path}" - ) - return full_text - else: - # Assume it's a text file, decode to string - logger.debug(f"Decoding text file: {file_path}") - return file_content.decode("utf-8", errors="replace") - except Exception as e: - logger.error( - f"Error fetching file content for {doc_id}: {e}", exc_info=True - ) - return None elif doc_type == "news_item": # Fetch news item by ID item = await nc_client.news.get_item(int(doc_id)) diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py index 3f39d303..d46dc69d 100644 --- a/tests/unit/test_chunk_context_offset_gate.py +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -269,6 +269,95 @@ class TestNullableChunkIndexPropagation: assert "Chunk ?/10" in result.marked_text +class TestAdjacentChunkBoundary: + """Boundary cases for the `chunk_index > 0` / `chunk_index < total_chunks - 1` + gates that decide whether to fetch the previous / next chunk via Qdrant. + See PR #767 review (🟡 missing boundary tests). + """ + + async def test_first_chunk_skips_before_fetch_only(self, mock_nc_client): + """At chunk_index=0 the before-fetch gate is closed (no previous + chunk exists) but the after-fetch still runs. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + side_effect=[ + "current chunk text", # primary lookup + "next chunk text", # adjacent after only + ], + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + ): + result = 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=0, + total_chunks=10, + ) + + assert result is not None + assert result.chunk_index == 0 + assert result.has_before_truncation is False + assert result.has_after_truncation is False + assert mock_indexed.await_count == 2, ( + "expected primary lookup + after-fetch only (no before-fetch at index 0)" + ) + assert "Chunk 1 of 10" in result.marked_text + + async def test_last_chunk_skips_after_fetch_only(self, mock_nc_client): + """At chunk_index=total_chunks-1 the after-fetch gate is closed (no + next chunk exists) but the before-fetch still runs. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + side_effect=[ + "current chunk text", # primary lookup + "previous chunk text", # adjacent before only + ], + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + ): + result = 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=9, + total_chunks=10, + ) + + assert result is not None + assert result.chunk_index == 9 + assert result.has_before_truncation is False + assert result.has_after_truncation is False + assert mock_indexed.await_count == 2, ( + "expected primary lookup + before-fetch only (no after-fetch at last index)" + ) + assert "Chunk 10 of 10" in result.marked_text + + class TestPositionMarkers: """Direct tests for `_insert_position_markers` rendering when chunk_index is None vs explicit. From 928b973eb84be2ea1f3a084ae31b534a3367b8a0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 9 May 2026 12:57:58 +0200 Subject: [PATCH 09/12] fix(webdav): decode percent-encoded names in PROPFIND/SEARCH responses `` is required by RFC 3986 to be percent-encoded, so non-ASCII filenames (e.g. Chinese, Cyrillic) were leaking through `list_directory` and the SEARCH-based tools (`find_by_name`, `find_by_type`, `list_favorites`, `search_files`) as their URL-encoded form. Decode with the already-imported `urllib.parse.unquote` before exposing to callers. Fixes #776 Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/webdav.py | 12 ++-- tests/unit/client/test_webdav.py | 96 +++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 978ebd1c..f932e1ad 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -257,9 +257,11 @@ class WebDAVClient(BaseNextcloudClient): if href is None: continue - # Extract file/directory name from href + # Extract file/directory name from href. is required by + # RFC 3986 to be percent-encoded, so non-ASCII names arrive + # encoded — decode before exposing to callers (issue #776). href_text = href.text or "" - name = href_text.rstrip("/").split("/")[-1] + name = unquote(href_text.rstrip("/").split("/")[-1]) if not name: continue @@ -767,8 +769,10 @@ class WebDAVClient(BaseNextcloudClient): if href is None: continue - # Extract file/directory path from href - href_text = href.text or "" + # Extract file/directory path from href. is required by + # RFC 3986 to be percent-encoded, so non-ASCII paths arrive + # encoded — decode before exposing to callers (issue #776). + href_text = unquote(href.text or "") # Remove the /remote.php/dav/files/username/ prefix to get relative path path_parts = href_text.split("/files/") if len(path_parts) > 1: diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index 03b0b728..c02144e0 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -414,3 +414,99 @@ async def test_get_files_by_tag_detects_directories(mocker): call_args = mock_http_client.request.call_args assert "" in call_args.kwargs["content"] assert "42" in call_args.kwargs["content"] + + +@pytest.mark.unit +async def test_list_directory_decodes_non_ascii_names(mocker): + """list_directory must percent-decode for non-ASCII filenames (issue #776). + + RFC 3986 requires to be percent-encoded, so a Chinese-named directory + arrives as e.g. "%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1". The MCP response should + expose the decoded "学生邮箱", not the encoded form. + """ + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + # PROPFIND response with one Chinese-named subdirectory and one ASCII file. + # The first is the parent directory and is skipped by list_directory. + xml_content = b""" + + + /remote.php/dav/files/testuser/ + + + + + + + + /remote.php/dav/files/testuser/%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1/ + + + \xe5\xad\xa6\xe7\x94\x9f\xe9\x82\xae\xe7\xae\xb1 + + + + + + /remote.php/dav/files/testuser/notes.txt + + + notes.txt + 10 + text/plain + + + + + """ + + mock_response = AsyncMock() + mock_response.content = xml_content + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + items = await client.list_directory("") + + by_name = {item["name"]: item for item in items} + assert "学生邮箱" in by_name, f"expected decoded Chinese name, got: {list(by_name)}" + assert by_name["学生邮箱"]["is_directory"] is True + assert by_name["学生邮箱"]["path"] == "学生邮箱" + + # ASCII entries must keep working. + assert "notes.txt" in by_name + assert by_name["notes.txt"]["is_directory"] is False + + +@pytest.mark.unit +def test_parse_search_response_decodes_non_ascii_paths(mocker): + """_parse_search_response must percent-decode for non-ASCII paths (issue #776). + + Affects find_by_name, find_by_type, list_favorites, and search_files: the `path` + and `href` fields would otherwise leak percent-encoded URL form to callers. + """ + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + xml_content = b""" + + + /remote.php/dav/files/testuser/%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1/report.pdf + + + report.pdf + application/pdf + 1024 + + + + + """ + + results = client._parse_search_response(xml_content, scope="") + + assert len(results) == 1 + assert results[0]["path"] == "学生邮箱/report.pdf" + assert results[0]["href"] == "/remote.php/dav/files/testuser/学生邮箱/report.pdf" + # name comes from , which is not URL-encoded; sanity-check it. + assert results[0]["name"] == "report.pdf" From 4b6c556f39a1088233f4bee21c3240a85c6c0fa6 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 9 May 2026 11:23:58 +0000 Subject: [PATCH 10/12] =?UTF-8?q?bump:=20version=200.83.0=20=E2=86=92=200.?= =?UTF-8?q?83.1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 6 ++++++ pyproject.toml | 2 +- uv.lock | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9380c671..209c042a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to the Nextcloud MCP Server will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [PEP 440](https://peps.python.org/pep-0440/). +## v0.83.1 (2026-05-09) + +### Fix + +- **webdav**: decode percent-encoded names in PROPFIND/SEARCH responses + ## v0.83.0 (2026-05-08) ### Feat diff --git a/pyproject.toml b/pyproject.toml index 38b0702b..5bf96fbb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "nextcloud-mcp-server" -version = "0.83.0" +version = "0.83.1" description = "Model Context Protocol (MCP) server for Nextcloud integration - enables AI assistants to interact with Nextcloud data" authors = [ {name = "Chris Coutinho", email = "chris@coutinho.io"} diff --git a/uv.lock b/uv.lock index 80de7994..4aa5ca4a 100644 --- a/uv.lock +++ b/uv.lock @@ -2123,7 +2123,7 @@ wheels = [ [[package]] name = "nextcloud-mcp-server" -version = "0.83.0" +version = "0.83.1" source = { editable = "." } dependencies = [ { name = "aiosqlite" }, From 7ef8760d277f69464cbead83317ca4ab0e8017db Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 9 May 2026 13:29:22 +0200 Subject: [PATCH 11/12] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767?= =?UTF-8?q?=20review=20=E2=80=94=20extract=20bbox=20helper,=20fix=20page?= =?UTF-8?q?=5Fnumber=20overwrite?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves both 🟡 important issues from the latest review: 1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even when Qdrant's payload lacked the field, clobbering the value resolved from `chunk_context.page_number`. The new helper returns each field independently and both call sites only overwrite via `is not None` guards, matching the existing logic in `visualization.py`. 2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll block was duplicated between `api/visualization.py` and `auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant` in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant` helpers; both routes now share ~12 lines of caller code. New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed and offset paths, the `(bbox, None)` regression case, and graceful degradation on Qdrant strict-mode 400 (which also closes nit #4). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 93 ++-------- nextcloud_mcp_server/auth/viz_routes.py | 93 ++-------- nextcloud_mcp_server/search/context.py | 93 ++++++++++ tests/unit/test_chunk_bbox_helper.py | 215 ++++++++++++++++++++++ 4 files changed, 342 insertions(+), 152 deletions(-) create mode 100644 tests/unit/test_chunk_bbox_helper.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 309ef2de..5c744288 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -14,7 +14,6 @@ import logging from typing import Any import pymupdf -from qdrant_client.models import FieldCondition, Filter, MatchValue from starlette.requests import Request from starlette.responses import JSONResponse @@ -31,13 +30,14 @@ from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, SemanticSearchAlgorithm, ) -from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.search.context import ( + get_chunk_bbox_and_page_from_qdrant, + get_chunk_with_context, +) from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, ) -from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter -from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client from nextcloud_mcp_server.vector.visualization import compute_pca_coordinates logger = logging.getLogger(__name__) @@ -567,82 +567,23 @@ async def get_chunk_context(request: Request) -> JSONResponse: # For PDF files, also fetch the chunk's bounding box from Qdrant if # available so the client can overlay a highlight on top of a - # render-on-demand page image (Deck #76). + # render-on-demand page image (Deck #76). Qdrant's page_number is + # trusted over the context-expansion fallback when present. chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": - try: - settings = get_settings() - qdrant_client = await get_qdrant_client() - - # Prefer chunk_index for the chunk-bbox lookup (always indexed); - # fall back to (chunk_start_offset, chunk_end_offset) when not provided. - if chunk_index is not None: - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_index", - match=MatchValue(value=chunk_index), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - else: - # Legacy fallback for clients that don't send chunk_index - # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset - # aren't indexed in Qdrant Cloud strict mode, so this - # call may fail with HTTP 400 there; the outer except - # logs a warning and the response degrades gracefully - # (no chunk_bbox). - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_start_offset", - match=MatchValue(value=start), - ), - FieldCondition( - key="chunk_end_offset", - match=MatchValue(value=end), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - - if points_response[0]: - payload = points_response[0][0].payload - if payload: - chunk_bbox = payload.get("chunk_bbox") - # Trust Qdrant page number if available (might be more accurate than context expansion logic) - if payload.get("page_number") is not None: - page_number = payload.get("page_number") - - except Exception as e: - logger.warning(f"Failed to fetch chunk bbox: {e}") + qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( + user_id=user_id, + doc_id=doc_id_val, + chunk_index=chunk_index, + chunk_start=start, + chunk_end=end, + ) + if qdrant_bbox is not None: + chunk_bbox = qdrant_bbox + if qdrant_page is not None: + page_number = qdrant_page # Build response response_data = { diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d1a369bb..f6ba5de8 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -18,7 +18,6 @@ from pathlib import Path import anyio import numpy as np from jinja2 import Environment, FileSystemLoader -from qdrant_client.models import FieldCondition, Filter, MatchValue from starlette.authentication import requires from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse @@ -34,13 +33,15 @@ from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, SemanticSearchAlgorithm, ) -from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.search.context import ( + get_chunk_bbox_and_page_from_qdrant, + get_chunk_with_context, +) from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, ) from nextcloud_mcp_server.vector.pca import PCA -from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) @@ -625,82 +626,22 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # For PDF files, also fetch the chunk bbox from Qdrant so the client # can overlay a highlight on top of a render-on-demand page image - # (Deck #76). + # (Deck #76). Qdrant's page_number is trusted over the + # context-expansion fallback when present. chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": - try: - settings = get_settings() - qdrant_client = await get_qdrant_client() - - # Prefer chunk_index for the chunk-bbox lookup (always indexed); - # fall back to (chunk_start_offset, chunk_end_offset) when not provided. - if chunk_index is not None: - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_index", - match=MatchValue(value=chunk_index), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - else: - # Legacy fallback for clients that don't send chunk_index - # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset - # aren't indexed in Qdrant Cloud strict mode, so this - # call may fail with HTTP 400 there; the outer except - # logs a warning and the response degrades gracefully - # (no chunk_bbox). - points_response = await qdrant_client.scroll( - collection_name=settings.get_collection_name(), - scroll_filter=Filter( - must=[ - get_placeholder_filter(), - FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) - ), - FieldCondition( - key="user_id", match=MatchValue(value=user_id) - ), - FieldCondition( - key="chunk_start_offset", - match=MatchValue(value=start), - ), - FieldCondition( - key="chunk_end_offset", - match=MatchValue(value=end), - ), - ] - ), - limit=1, - with_vectors=False, - with_payload=["chunk_bbox", "page_number"], - ) - - points = points_response[0] - if points and points[0].payload: - chunk_bbox = points[0].payload.get("chunk_bbox") - page_number = points[0].payload.get("page_number") - if chunk_bbox: - logger.info( - f"Found chunk bbox: page={page_number}, " - f"rects={len(chunk_bbox)}" - ) - except Exception as e: - logger.warning(f"Failed to fetch chunk bbox: {e}") + qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( + user_id=user_id, + doc_id=doc_id_int, + chunk_index=chunk_index, + chunk_start=start, + chunk_end=end, + ) + if qdrant_bbox is not None: + chunk_bbox = qdrant_bbox + if qdrant_page is not None: + page_number = qdrant_page # Return response compatible with frontend expectations response_data: dict = { diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 1a5fcaff..3f2205e4 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -12,6 +12,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.vector.html_processor import html_to_markdown +from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) @@ -195,6 +196,98 @@ async def _get_deck_metadata_from_qdrant( return None +async def get_chunk_bbox_and_page_from_qdrant( + user_id: str, + doc_id: int | str, + chunk_index: int | None, + chunk_start: int, + chunk_end: int, +) -> tuple[list | None, int | None]: + """Fetch chunk_bbox and page_number for a chunk from Qdrant payload. + + Prefers chunk_index for the lookup (always indexed); falls back to + (chunk_start_offset, chunk_end_offset) when chunk_index is not provided + — this is the legacy path for clients pre-cbcoutinho/astrolabe#75. The + fallback may 400 in Qdrant Cloud strict mode because those offset fields + aren't indexed there; that's logged as a warning and (None, None) is + returned so callers degrade gracefully. + + Args: + user_id: User ID who owns the document + doc_id: Document ID (int for file/note, str for some doc types) + 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) + chunk_end: Character offset where chunk ends (used when chunk_index + is None) + + Returns: + Tuple of (chunk_bbox, page_number); either field may be None + independently if absent from the payload, or both may be None on + miss/error. + """ + try: + settings = get_settings() + qdrant_client = await get_qdrant_client() + + if chunk_index is not None: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition( + key="chunk_index", match=MatchValue(value=chunk_index) + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["chunk_bbox", "page_number"], + ) + else: + points_response = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + get_placeholder_filter(), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition( + key="chunk_start_offset", + match=MatchValue(value=chunk_start), + ), + FieldCondition( + key="chunk_end_offset", + match=MatchValue(value=chunk_end), + ), + ] + ), + limit=1, + with_vectors=False, + with_payload=["chunk_bbox", "page_number"], + ) + + points = points_response[0] + if not points or not points[0].payload: + return None, None + + payload = points[0].payload + chunk_bbox = payload.get("chunk_bbox") + page_number = payload.get("page_number") + if chunk_bbox: + logger.info( + "Found chunk bbox: page=%s, rects=%d", page_number, len(chunk_bbox) + ) + return chunk_bbox, page_number + + except Exception as e: + logger.warning("Failed to fetch chunk bbox: %s", e) + return None, None + + @dataclass class ChunkContext: """Expanded chunk with surrounding context and position markers. diff --git a/tests/unit/test_chunk_bbox_helper.py b/tests/unit/test_chunk_bbox_helper.py new file mode 100644 index 00000000..af542353 --- /dev/null +++ b/tests/unit/test_chunk_bbox_helper.py @@ -0,0 +1,215 @@ +"""Unit tests for +`nextcloud_mcp_server.search.context.get_chunk_bbox_and_page_from_qdrant`. + +Covers the two paths the helper handles: +- Indexed lookup via `chunk_index` (the preferred path post + cbcoutinho/astrolabe#75) +- Legacy offset fallback via `(chunk_start_offset, chunk_end_offset)`, which + may 400 in Qdrant Cloud strict mode + +Plus the regression case from PR #767 review: when the payload has +`chunk_bbox` but no `page_number`, the helper must surface that as +`(bbox, None)` so callers can preserve their context-derived page_number +fallback. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Import via the auth surface first to side-step the known +# `nextcloud_mcp_server.search.__init__` circular-init issue (same workaround +# used in test_chunk_context_offset_gate.py). +import nextcloud_mcp_server.auth.viz_routes # noqa: F401 +from nextcloud_mcp_server.search import context as context_module +from nextcloud_mcp_server.search.context import get_chunk_bbox_and_page_from_qdrant + +pytestmark = pytest.mark.unit + + +def _make_point(payload: dict) -> MagicMock: + point = MagicMock() + point.payload = payload + return point + + +def _patch_qdrant(scroll_return=None, scroll_side_effect=None): + qdrant_client = MagicMock() + if scroll_side_effect is not None: + qdrant_client.scroll = AsyncMock(side_effect=scroll_side_effect) + else: + qdrant_client.scroll = AsyncMock(return_value=scroll_return) + return patch.object( + context_module, + "get_qdrant_client", + new_callable=AsyncMock, + return_value=qdrant_client, + ), qdrant_client + + +class TestIndexedPath: + """When `chunk_index` is supplied, the helper must use the indexed + `chunk_index` filter (not the offset fallback).""" + + async def test_returns_bbox_and_page_when_payload_complete(self): + bbox = [[0, 0, 100, 50]] + point = _make_point({"chunk_bbox": bbox, "page_number": 7}) + ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (bbox, 7) + # One scroll call, and the filter must include chunk_index (not offsets) + qdrant_client.scroll.assert_awaited_once() + scroll_kwargs = qdrant_client.scroll.await_args.kwargs + filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must] + assert "chunk_index" in filter_keys + assert "chunk_start_offset" not in filter_keys + assert "chunk_end_offset" not in filter_keys + + +class TestOffsetFallbackPath: + """When `chunk_index` is None, the helper must use the offset filter.""" + + async def test_returns_bbox_and_page_when_payload_complete(self): + bbox = [[10, 20, 110, 70]] + point = _make_point({"chunk_bbox": bbox, "page_number": 2}) + ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="bob", + doc_id=99, + chunk_index=None, + chunk_start=500, + chunk_end=600, + ) + + assert result == (bbox, 2) + scroll_kwargs = qdrant_client.scroll.await_args.kwargs + filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must] + assert "chunk_start_offset" in filter_keys + assert "chunk_end_offset" in filter_keys + assert "chunk_index" not in filter_keys + + async def test_strict_mode_400_returns_none_pair_and_warns(self, caplog): + """Qdrant Cloud strict mode 400s on unindexed offset filters; the + helper must swallow the exception, log a warning, and degrade + gracefully so the route can still return chunk text.""" + ctx, _ = _patch_qdrant(scroll_side_effect=Exception("strict mode: 400")) + with ctx, caplog.at_level("WARNING"): + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="bob", + doc_id=99, + chunk_index=None, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records) + + +class TestPayloadShape: + """Each payload field can be missing independently — callers rely on + that to decide whether to overwrite their fallback values.""" + + async def test_empty_points_returns_none_pair(self): + ctx, _ = _patch_qdrant(scroll_return=([], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=1, + chunk_index=0, + chunk_start=0, + chunk_end=10, + ) + + assert result == (None, None) + + async def test_missing_page_returns_bbox_only(self): + """Regression for PR #767 review issue #1: when Qdrant returns a + point whose payload lacks `page_number`, the helper must return + `(bbox, None)` so callers preserve their `chunk_context.page_number` + fallback rather than clobbering it to None.""" + bbox = [[0, 0, 100, 50]] + point = _make_point({"chunk_bbox": bbox}) # no page_number + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (bbox, None) + + async def test_missing_bbox_returns_page_only(self): + point = _make_point({"page_number": 5}) # no chunk_bbox + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, 5) + + async def test_empty_payload_returns_none_pair(self): + point = _make_point({}) + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + + async def test_falsy_payload_treated_as_no_point(self): + """`if not points[0].payload` short-circuits when payload is None or + an empty dict, mirroring the original guards in the route handlers.""" + point = MagicMock() + point.payload = None + ctx, _ = _patch_qdrant(scroll_return=([point], None)) + with ctx: + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + + +class TestExceptionHandling: + """Any error from Qdrant must produce `(None, None)` — never propagate.""" + + async def test_indexed_path_exception_returns_none_pair(self, caplog): + ctx, _ = _patch_qdrant(scroll_side_effect=RuntimeError("qdrant unavailable")) + with ctx, caplog.at_level("WARNING"): + result = await get_chunk_bbox_and_page_from_qdrant( + user_id="alice", + doc_id=42, + chunk_index=3, + chunk_start=0, + chunk_end=100, + ) + + assert result == (None, None) + assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records) From 6b22d1ff7625c2f680d54dc2bf799111bf0d59e3 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 9 May 2026 11:35:32 +0000 Subject: [PATCH 12/12] =?UTF-8?q?bump:=20version=200.83.1=20=E2=86=92=200.?= =?UTF-8?q?83.2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 14 ++++++++++++++ pyproject.toml | 2 +- uv.lock | 2 +- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 209c042a..14e2eab2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ All notable changes to the Nextcloud MCP Server will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [PEP 440](https://peps.python.org/pep-0440/). +## v0.83.2 (2026-05-09) + +### Fix + +- **chunk-context**: address PR #767 review — extract bbox helper, fix page_number overwrite +- **chunk-context**: address PR #767 review — drop dead PDF branch, redundant alias, add boundary tests +- **chunk-context**: address PR #767 round-3 review — gate readability + legacy-fallback comment +- **chunk-context**: propagate chunk_index=None through ChunkContext +- **chunk-context**: address PR #767 round-2 review — gate, parity, doc +- **chunk-context**: address PR #767 review — doc_type filter parity + tests +- **viz_routes**: address PR #767 review — param parity + always-on page_number +- **viz_routes**: validate chunk_index/total_chunks bounds in OAuth route +- **chunk-context**: use indexed chunk_index lookup, fix close-after-use bug + ## v0.83.1 (2026-05-09) ### Fix diff --git a/pyproject.toml b/pyproject.toml index 5bf96fbb..5a8101de 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "nextcloud-mcp-server" -version = "0.83.1" +version = "0.83.2" description = "Model Context Protocol (MCP) server for Nextcloud integration - enables AI assistants to interact with Nextcloud data" authors = [ {name = "Chris Coutinho", email = "chris@coutinho.io"} diff --git a/uv.lock b/uv.lock index 4aa5ca4a..9082741f 100644 --- a/uv.lock +++ b/uv.lock @@ -2123,7 +2123,7 @@ wheels = [ [[package]] name = "nextcloud-mcp-server" -version = "0.83.1" +version = "0.83.2" source = { editable = "." } dependencies = [ { name = "aiosqlite" },