diff --git a/CHANGELOG.md b/CHANGELOG.md index 9380c671..14e2eab2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,26 @@ 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 + +- **webdav**: decode percent-encoded names in PROPFIND/SEARCH responses + ## v0.83.0 (2026-05-08) ### Feat diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index dae3984b..cec34e92 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__) @@ -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) # doc_id is keyword-indexed in Qdrant as str — pass through verbatim @@ -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, ) @@ -555,49 +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() - - 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=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, + 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 c0ff384b..c189ac0a 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -18,11 +18,11 @@ 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 +from nextcloud_mcp_server.api.management import _parse_int_param from nextcloud_mcp_server.auth.userinfo_routes import ( _get_authenticated_client_for_userinfo, ) @@ -33,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__) @@ -539,7 +541,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") - context_chars = int(request.query_params.get("context", "500")) + 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]): @@ -557,9 +560,25 @@ 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) - # doc_id is keyword-indexed in Qdrant as str — pass through verbatim. + 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( + chunk_index_str, 0, 0, 1000000, "chunk_index" + ) + total_chunks = _parse_int_param(total_chunks_str, 1, 1, 1000000, "total_chunks") + # doc_id is keyword-indexed in Qdrant as str — pass through verbatim + # (no int coercion; producers always stringify on write). user_id = request.user.display_name settings = get_settings() @@ -587,6 +606,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, ) @@ -609,50 +630,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 = None + page_number = chunk_context.page_number if doc_type == "file": - try: - settings = get_settings() - qdrant_client = await get_qdrant_client() - username = request.user.display_name - - 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=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=["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, + 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 = { @@ -662,16 +655,19 @@ 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, } if chunk_bbox: response_data["chunk_bbox"] = chunk_bbox - response_data["page_number"] = page_number 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/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/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 1771c087..a3449b5c 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -7,13 +7,12 @@ 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 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__) @@ -144,64 +143,6 @@ async def _get_chunk_by_index_from_qdrant( return None -async def _get_file_path_from_qdrant( - user_id: str, file_id: str, 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: Stringified file ID (Qdrant payload value, post-doc_id - normalization — see vector/qdrant_client.py) - 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: str ) -> dict[str, int] | None: @@ -255,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. @@ -266,7 +299,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 @@ -279,7 +315,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 @@ -294,7 +330,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: @@ -312,7 +348,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 @@ -320,27 +358,46 @@ async def get_chunk_with_context( ChunkContext with expanded context and markers, or None if document cannot be retrieved """ - # Try to get chunk from Qdrant first (fast path) + # doc_id is keyword-indexed in Qdrant as str — pass through verbatim + # (no int coercion; producers always stringify on write). + + # 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: - chunk_text = await _get_chunk_from_qdrant( - user_id, doc_id, doc_type, chunk_start, chunk_end - ) - if chunk_text: - logger.info( - f"Retrieved chunk from Qdrant cache for {doc_type} {doc_id} " - f"(avoids document re-fetch/re-parse)" + if chunk_index is not None: + chunk_text = await _get_chunk_by_index_from_qdrant( + user_id, doc_id, doc_type, chunk_index + ) + # 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, doc_type, chunk_start, chunk_end ) - # Fetch adjacent chunks for context expansion - # Get chunk overlap from config to remove duplicate text - settings = get_settings() - chunk_overlap = settings.document_chunk_overlap + if chunk_text: + logger.info( + f"Retrieved chunk from Qdrant cache for {doc_type} {doc_id} " + f"(avoids document re-fetch/re-parse)" + ) - before_context = "" - after_context = "" - has_before_truncation = False - has_after_truncation = False + # 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 + + 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( @@ -382,64 +439,62 @@ async def get_chunk_with_context( 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=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=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, + ) + + # 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( + "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 - # 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, doc_id is always the stringified numeric file ID after - # producer normalization — resolve it to a WebDAV path so - # _fetch_document_text can retrieve the binary content. The previous - # `isinstance(doc_id, int)` guard is no longer needed: file producers - # write str(file_id) and the startup backfill rewrites legacy int - # payloads. If lookup fails (e.g. truly malformed legacy data), the - # caller logs and returns None below — a re-index is the recovery path. - resolved_doc_id = doc_id - if doc_type == "file": - 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}, " @@ -502,10 +557,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 @@ -528,54 +587,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 = [] - - # Extract each page as markdown (same as indexing) - for page_num in range(pdf_doc.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"{pdf_doc.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": # News item IDs are integers in the Nextcloud News API; reject # non-numeric doc_ids explicitly so malformed payloads surface @@ -697,7 +708,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, @@ -712,7 +723,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 @@ -724,7 +736,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/pyproject.toml b/pyproject.toml index 38b0702b..5a8101de 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "nextcloud-mcp-server" -version = "0.83.0" +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/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" 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) 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..d46dc69d --- /dev/null +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -0,0 +1,391 @@ +"""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() + + +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 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. + """ + + 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 diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py index e387a91a..7e5d7d6b 100644 --- a/tests/unit/test_management_chunk_context_endpoint.py +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -258,6 +258,101 @@ 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. + + 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.""" 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() diff --git a/uv.lock b/uv.lock index 80de7994..9082741f 100644 --- a/uv.lock +++ b/uv.lock @@ -2123,7 +2123,7 @@ wheels = [ [[package]] name = "nextcloud-mcp-server" -version = "0.83.0" +version = "0.83.2" source = { editable = "." } dependencies = [ { name = "aiosqlite" },