diff --git a/.claude/skills/pre-push-review/SKILL.md b/.claude/skills/pre-push-review/SKILL.md index efc906b0..26883d49 100644 --- a/.claude/skills/pre-push-review/SKILL.md +++ b/.claude/skills/pre-push-review/SKILL.md @@ -7,7 +7,6 @@ description: | in this repo's automated PR reviews. Use when the user is about to push, says "ready to push", "review my work", "check before PR", or invokes /pre-push-review. Report-only — does not modify code. -model: sonnet allowed-tools: - Bash - Read diff --git a/docker-compose.yml b/docker-compose.yml index 208ce05e..46984db5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -293,7 +293,7 @@ services: # the same configure_astrolabe_for_mcp_server fixture (which creates the # static `nextcloudMcpServerUIPublicClient` OIDC client) works for tests # against this profile too. - - ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient + - ALLOWED_MGMT_CLIENT=astrolabeMcpClientOAuth00000000000 volumes: - login-flow-data:/app/data - login-flow-oauth-storage:/app/.oauth diff --git a/docs/configuration.md b/docs/configuration.md index dd71e300..a1879596 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -329,6 +329,50 @@ OLLAMA_EMBEDDING_MODEL=all-minilm - **Switching models requires re-embedding** all documents (may take time for large note collections) - **Old collection remains** in Qdrant and can be deleted manually if no longer needed +#### Startup migrations on existing collections + +On the first call to `get_qdrant_client()` against an existing collection, the +server runs two idempotent migrations: + +1. **Payload-index creation** — adds `KEYWORD` payload indexes for `doc_id`, + `user_id`, and `doc_type`. Required by Qdrant for any `FieldCondition` + filter. Cheap; runs even on healthy collections. +2. **`doc_id` backfill** — scans the collection once and rewrites any + legacy integer `doc_id` payloads to strings so they match the keyword + index. Idempotent: on a clean collection (all `doc_id` values already + `str`), the scroll runs but emits zero writes. On the first start after + the upgrade, expect a delay proportional to total point count for the + scroll itself, plus an additional delay proportional to any `int`-typed + `doc_id` points found while their payloads are rewritten. + +Both steps emit INFO-level log lines so operators can track progress. + +> **Operator note:** if the server logs `TypeError: SemanticSearchResult.id +> must be int-convertible` after upgrading, this indicates a `doc_type` +> with non-numeric ids has been indexed but the public response model +> (`SemanticSearchResult.id: int`) has not been widened to accept strings. +> Semantic search itself is not broken — the boundary cast in +> `server/semantic.py` is failing loudly on purpose so the discrepancy is +> caught early. Either widen the public model's `id` field or convert the +> id at the verifier layer. + +> **Degraded-migration signals:** both startup steps swallow non-fatal +> failures so the server still starts, but each leaves a distinct ERROR +> log line that operators should treat as a "restart needed" signal: +> +> - `Unexpected error creating payload index on '' (status 5xx)` — +> the index was not created. Searches filtering on that field will keep +> returning HTTP 400 (`Index required but not found`) until a subsequent +> restart succeeds in creating it. +> - `doc_id backfill scroll failed on ''; will retry on next restart` — +> the migration sentinel was not written. Legacy integer `doc_id` +> payloads remain invisible to the keyword index in the meantime; the +> scroll re-runs from scratch on the next process start. +> +> Neither prevents the server from accepting requests, but both indicate +> that vector search is operating in a degraded state on the affected +> collection until the next clean restart. + #### Explicit Override Set `QDRANT_COLLECTION` to use a specific collection name: diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 5c744288..c3a9b790 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -34,6 +34,7 @@ from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, @@ -498,6 +499,30 @@ async def get_chunk_context(request: Request) -> JSONResponse: assert doc_id is not None assert doc_type is not None + # Validate doc_id at the handler boundary: a malformed doc_id would + # otherwise pass through to get_chunk_with_context and bottom out as a + # 404 from deep inside, not a clear 400. Nextcloud IDs are unsigned + # ints from MySQL auto_increment; doc_id stays a str downstream + # (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id + # rejects "0", leading zeros, and Unicode digits that pass isdigit(). + # + # Canonical TODO (referenced by ``auth/viz_routes.py`` and + # ``vector/scanner.py:get_last_indexed_timestamp``): when chunk- + # context support extends to non-numeric doc_types (calendar VEVENT + # UIDs, CardDAV hrefs, …), relax this gate or make it doc_type- + # aware. Today every indexed doc_type is numeric. The follow-up + # tracker also covers the O(N) → O(1) migration of + # ``get_last_indexed_timestamp`` (currently re-scans every + # ``indexed_at`` on each tick). + if not is_valid_nextcloud_doc_id(doc_id): + return JSONResponse( + { + "success": False, + "error": f"doc_id must be numeric, got {doc_id!r}", + }, + status_code=400, + ) + # Parse and validate integer parameters with bounds checking try: context_chars = _parse_int_param( @@ -521,8 +546,8 @@ async def get_chunk_context(request: Request) -> JSONResponse: ) 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) - doc_id_val: str | int = int(doc_id) if doc_id.isdigit() else doc_id + # doc_id is keyword-indexed in Qdrant as str — pass through verbatim + # (no int coercion; producers always stringify on write). # Get Nextcloud host from OAuth context oauth_ctx = request.app.state.oauth_context @@ -547,7 +572,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, - doc_id=doc_id_val, + doc_id=doc_id, doc_type=doc_type, chunk_start=start, chunk_end=end, @@ -575,7 +600,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: if doc_type == "file": qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( user_id=user_id, - doc_id=doc_id_val, + doc_id=doc_id, chunk_index=chunk_index, chunk_start=start, chunk_end=end, diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index f6ba5de8..1c6ad627 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -37,6 +37,7 @@ from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, @@ -287,7 +288,11 @@ async def vector_visualization_search(request: Request) -> JSONResponse: vector = point.vector if vector is not None and point.payload: - doc_id = point.payload.get("doc_id") + # SearchResult.id is str; coerce payload doc_id to match + # so the tuple lookup below succeeds even on legacy + # int-typed payloads written before normalization. + raw_doc_id = point.payload.get("doc_id") + doc_id = None if raw_doc_id is None else str(raw_doc_id) chunk_start = point.payload.get("chunk_start_offset") chunk_end = point.payload.get("chunk_end_offset") chunk_key = (doc_id, chunk_start, chunk_end) @@ -556,6 +561,20 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: assert start_str is not None assert end_str is not None + # Same numeric-doc_id gate as ``api/visualization.py`` — see the + # canonical TODO and rationale there. Kept in sync so both + # OAuth-protected and direct-access handlers reject malformed + # IDs at the boundary instead of bottoming out as a 404 from + # deep inside ``get_chunk_with_context``. + if not is_valid_nextcloud_doc_id(doc_id): + return JSONResponse( + { + "success": False, + "error": f"doc_id must be numeric, got {doc_id!r}", + }, + status_code=400, + ) + context_chars = _parse_int_param( request.query_params.get("context"), 500, @@ -573,8 +592,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: 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) + # 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() @@ -598,7 +617,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, - doc_id=doc_id_int, + doc_id=doc_id, doc_type=doc_type, chunk_start=start, chunk_end=end, @@ -633,7 +652,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: if doc_type == "file": qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant( user_id=user_id, - doc_id=doc_id_int, + doc_id=doc_id, chunk_index=chunk_index, chunk_start=start, chunk_end=end, diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 4612d26d..bb0091dc 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -11,10 +11,12 @@ class SemanticSearchResult(BaseModel): id: int = Field( description=( "Document ID. Numeric for all currently indexed types (notes, files, " - "deck cards, news items). The internal SearchResult.id is typed as " - "int|str to leave room for future doc types with string identifiers; " - "the MCP response narrows to int and a future widening here would be " - "a deliberate, breaking-by-design API change." + "deck cards, news items). The internal SearchResult.id is stringified " + "for Qdrant's keyword-indexed doc_id payload; the MCP response narrows " + "back to int via int(r.id). A future doc_type with non-numeric ids " + "would surface here as a TypeError at the narrowing boundary, " + "forcing a deliberate widening of this field rather than a silent " + "API change." ) ) doc_type: str = Field( diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 2657bb45..f89a8933 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -5,12 +5,14 @@ from abc import ABC, abstractmethod from dataclasses import dataclass from typing import Any, Protocol, runtime_checkable -from qdrant_client.models import FieldCondition, Filter, MatchValue +from qdrant_client.models import FieldCondition, Filter, MatchValue, ScoredPoint from nextcloud_mcp_server.config import get_settings 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__) + @runtime_checkable class NextcloudClientProtocol(Protocol): @@ -91,7 +93,6 @@ async def get_indexed_doc_types(user_id: str) -> set[str]: ... # Search notes """ - logger = logging.getLogger(__name__) settings = get_settings() qdrant_client = await get_qdrant_client() @@ -132,9 +133,12 @@ class SearchResult: """A single search result with metadata and score. Attributes: - id: Document ID. Numeric for indexed types today (notes, files, - deck cards, news items), but typed as ``int | str`` to allow - future doc types that use string identifiers (e.g., file paths). + id: Document ID — always a string. Producers stringify their native + ID before writing to Qdrant so the keyword payload index on + ``doc_id`` matches every point regardless of source doc_type. + Public response models (e.g. ``SemanticSearchResult``) re-narrow + this back to ``int`` at the MCP boundary via ``int(r.id)`` — + see ``server/semantic.py`` for the narrowing site. doc_type: Document type (note, file, calendar, contact, etc.) title: Document title excerpt: Content excerpt showing match context @@ -151,7 +155,7 @@ class SearchResult: point_id: Qdrant point ID for batch vector retrieval (None if not from Qdrant) """ - id: int | str + id: str doc_type: str title: str excerpt: str @@ -178,6 +182,74 @@ class SearchResult: raise ValueError(f"Score must be non-negative, got {self.score}") +def build_search_result_from_point( + point: ScoredPoint, + *, + metadata_extras: dict[str, Any] | None = None, +) -> SearchResult | None: + """Construct a SearchResult from a Qdrant ScoredPoint payload. + + Returns ``None`` when the payload is missing — callers should skip the + point. The defensive ``str()`` coercion on ``doc_id`` covers legacy int + payloads until the startup backfill has run everywhere (see + ``vector/qdrant_client.py:_backfill_doc_id_to_string``). + + Args: + point: A Qdrant ``ScoredPoint`` from a search response. + metadata_extras: Algorithm-specific metadata merged into the result's + ``metadata`` dict (e.g., ``{"search_method": "bm25_hybrid_rrf"}``). + + Returns: + A populated ``SearchResult``, or ``None`` if ``point.payload`` is + missing. + """ + if point.payload is None: + return None + + raw_doc_id = point.payload.get("doc_id") + if raw_doc_id is None: + logger.warning("Skipping point %s: missing doc_id in payload", point.id) + return None + doc_id = str(raw_doc_id) + doc_type = point.payload.get("doc_type", "note") + + # Caller-supplied metadata is merged first; payload-derived common fields + # (chunk_index, total_chunks) win in case of key collisions so they always + # reflect the actual point. + metadata: dict[str, Any] = dict(metadata_extras) if metadata_extras else {} + metadata["chunk_index"] = point.payload.get("chunk_index") + metadata["total_chunks"] = point.payload.get("total_chunks") + + # File-specific metadata for PDF viewer + if doc_type == "file" and (path := point.payload.get("file_path")): + metadata["path"] = path + + # Deck-card metadata for frontend URL construction and verify-on-read + # (ADR-019) — both board_id and stack_id are required to call + # deck.get_card without an O(boards × stacks) iteration fallback. + if doc_type == "deck_card": + if board_id := point.payload.get("board_id"): + metadata["board_id"] = board_id + if stack_id := point.payload.get("stack_id"): + metadata["stack_id"] = stack_id + + return SearchResult( + id=doc_id, + doc_type=doc_type, + title=point.payload.get("title", "Untitled"), + excerpt=point.payload.get("excerpt", ""), + score=point.score, + metadata=metadata, + chunk_start_offset=point.payload.get("chunk_start_offset"), + chunk_end_offset=point.payload.get("chunk_end_offset"), + page_number=point.payload.get("page_number"), + page_count=point.payload.get("page_count"), + chunk_index=point.payload.get("chunk_index", 0), + total_chunks=point.payload.get("total_chunks", 1), + point_id=str(point.id), + ) + + class SearchAlgorithm(ABC): """Abstract base class for search algorithms. diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index c5848450..a400b957 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -10,7 +10,11 @@ from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_bm25_service, get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation from nextcloud_mcp_server.observability.tracing import trace_operation -from nextcloud_mcp_server.search.algorithms import SearchAlgorithm, SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchAlgorithm, + SearchResult, + build_search_result_from_point, +) from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -202,65 +206,30 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): "search.deduplicate", attributes={"dedupe.num_points": len(search_response.points)}, ): - seen_chunks = set() - results = [] + seen_chunks: set[tuple[str, str, Any, Any]] = set() + results: list[SearchResult] = [] + metadata_extras = { + "search_method": f"bm25_hybrid_{self.fusion_name}", + } - for result in search_response.points: - if result.payload is None: + for point in search_response.points: + sr = build_search_result_from_point( + point, metadata_extras=metadata_extras + ) + if sr is None: continue - # doc_id can be int (files) or str (notes/news_items/deck_cards) — see scanner.py - doc_id = result.payload["doc_id"] - doc_type = result.payload.get("doc_type", "note") - chunk_start = result.payload.get("chunk_start_offset") - chunk_end = result.payload.get("chunk_end_offset") - chunk_key = (doc_id, doc_type, chunk_start, chunk_end) - # Skip if we've already seen this exact chunk + chunk_key = ( + sr.id, + sr.doc_type, + sr.chunk_start_offset, + sr.chunk_end_offset, + ) if chunk_key in seen_chunks: continue - seen_chunks.add(chunk_key) - # Build metadata dict with common fields - metadata = { - "chunk_index": result.payload.get("chunk_index"), - "total_chunks": result.payload.get("total_chunks"), - "search_method": f"bm25_hybrid_{self.fusion_name}", - } - - # Add file-specific metadata for PDF viewer - if doc_type == "file" and (path := result.payload.get("file_path")): - metadata["path"] = path - - # Add deck_card-specific metadata for frontend URL construction - # and verify-on-read (ADR-019) — both board_id and stack_id are - # required to call deck.get_card without an O(boards × stacks) - # iteration fallback. - if doc_type == "deck_card": - if board_id := result.payload.get("board_id"): - metadata["board_id"] = board_id - if stack_id := result.payload.get("stack_id"): - metadata["stack_id"] = stack_id - - # Return unverified results (verification happens at output stage) - results.append( - SearchResult( - id=doc_id, - doc_type=doc_type, - title=result.payload.get("title", "Untitled"), - excerpt=result.payload.get("excerpt", ""), - score=result.score, # Fusion score (RRF or DBSF) - metadata=metadata, - chunk_start_offset=result.payload.get("chunk_start_offset"), - chunk_end_offset=result.payload.get("chunk_end_offset"), - page_number=result.payload.get("page_number"), - page_count=result.payload.get("page_count"), - chunk_index=result.payload.get("chunk_index", 0), - total_chunks=result.payload.get("total_chunks", 1), - point_id=str(result.id), # Qdrant point ID for batch retrieval - ) - ) - + results.append(sr) if len(results) >= limit: break diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 3f2205e4..11ef544c 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -11,6 +11,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.utils.validation import is_valid_nextcloud_doc_id 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 @@ -19,7 +20,7 @@ logger = logging.getLogger(__name__) async def _get_chunk_from_qdrant( - user_id: str, doc_id: int, doc_type: str, chunk_start: int, chunk_end: int + user_id: str, doc_id: str, doc_type: str, chunk_start: int, chunk_end: int ) -> str | None: """Retrieve full chunk text from Qdrant payload. @@ -86,7 +87,7 @@ async def _get_chunk_from_qdrant( async def _get_chunk_by_index_from_qdrant( - user_id: str, doc_id: int, doc_type: str, chunk_index: int + user_id: str, doc_id: str, doc_type: str, chunk_index: int ) -> str | None: """Retrieve chunk text by chunk_index from Qdrant payload. @@ -144,7 +145,7 @@ async def _get_chunk_by_index_from_qdrant( async def _get_deck_metadata_from_qdrant( - user_id: str, card_id: int + user_id: str, card_id: str ) -> dict[str, int] | None: """Retrieve board_id and stack_id for a deck card from Qdrant payload. @@ -198,7 +199,7 @@ async def _get_deck_metadata_from_qdrant( async def get_chunk_bbox_and_page_from_qdrant( user_id: str, - doc_id: int | str, + doc_id: str, chunk_index: int | None, chunk_start: int, chunk_end: int, @@ -214,7 +215,11 @@ async def get_chunk_bbox_and_page_from_qdrant( Args: user_id: User ID who owns the document - doc_id: Document ID (int for file/note, str for some doc types) + doc_id: Document ID — always a string. Producers stringify their + native ID before writing to Qdrant so the keyword payload + index on ``doc_id`` matches every point regardless of source + doc_type. An ``int`` filter against the str-indexed payload + would silently match zero points. chunk_index: Zero-based chunk index, or None to use offset fallback chunk_start: Character offset where chunk starts (used when chunk_index is None) @@ -325,7 +330,7 @@ class ChunkContext: async def get_chunk_with_context( nc_client: NextcloudClient, user_id: str, - doc_id: str | int, + doc_id: str, doc_type: str, chunk_start: int, chunk_end: int, @@ -343,7 +348,7 @@ async def get_chunk_with_context( Args: nc_client: Authenticated Nextcloud client user_id: User ID who owns the document - doc_id: Document ID (int for notes/files) + doc_id: Document ID (str — keyword-indexed in Qdrant payload) doc_type: Type of document ("note", "file", etc.) chunk_start: Character offset where chunk starts chunk_end: Character offset where chunk ends @@ -358,37 +363,31 @@ async def get_chunk_with_context( ChunkContext with expanded context and markers, or None if document cannot be retrieved """ - # Convert doc_id to int for Qdrant query - doc_id_int = ( - int(doc_id) - if isinstance(doc_id, str) and doc_id.isdigit() - else (doc_id if isinstance(doc_id, int) else None) - ) + # 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_int is not None: - if chunk_index is not None: - 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 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 - ) + if chunk_index is not None: + chunk_text = await _get_chunk_by_index_from_qdrant( + user_id, doc_id, doc_type, chunk_index + ) + # When chunk_index is supplied, the indexed lookup is canonical: both the + # index path and the offset path query the same Qdrant collection, so an + # indexed miss means the chunk is genuinely absent. Skipping the offset + # filter avoids a redundant Qdrant round-trip. Legacy data without + # chunk_index (pre-cbcoutinho/astrolabe#75) still hits the offset path + # and degrades to a None chunk with a WARNING; that's the same behavior + # get_chunk_bbox_and_page_from_qdrant already documents. + skip_offset_lookup = chunk_index is not None + if chunk_text is None and not skip_offset_lookup: + chunk_text = await _get_chunk_from_qdrant( + user_id, doc_id, doc_type, chunk_start, chunk_end + ) 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)" @@ -408,7 +407,7 @@ async def get_chunk_with_context( # 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 + user_id, doc_id, doc_type, chunk_index - 1 ) if before_chunk: # Remove overlap: the last chunk_overlap chars of previous chunk @@ -429,7 +428,7 @@ async def get_chunk_with_context( # 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 + user_id, doc_id, doc_type, chunk_index + 1 ) if after_chunk: # Remove overlap: the first chunk_overlap chars of next chunk @@ -560,7 +559,7 @@ async def get_chunk_with_context( async def _fetch_document_text( - nc_client: NextcloudClient, doc_id: str | int, doc_type: str, user_id: str + nc_client: NextcloudClient, doc_id: str, doc_type: str, user_id: str ) -> str | None: """Fetch full text content of a document. @@ -578,6 +577,16 @@ async def _fetch_document_text( """ try: if doc_type == "note": + # Note IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); a malformed payload surfaces in + # logs rather than getting silently swallowed by `except Exception`. + if not is_valid_nextcloud_doc_id(doc_id): + logger.warning( + "Expected numeric note doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch note by ID note = await nc_client.notes.get_note(note_id=int(doc_id)) # Reconstruct full content as indexed: title + "\n\n" + content @@ -586,6 +595,16 @@ async def _fetch_document_text( content = note.get("content", "") return f"{title}\n\n{content}" elif doc_type == "news_item": + # News item IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); malformed payloads surface in + # logs rather than getting swallowed by the broad except below. + if not is_valid_nextcloud_doc_id(doc_id): + logger.warning( + "Expected numeric news_item doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch news item by ID item = await nc_client.news.get_item(int(doc_id)) # Reconstruct full content as indexed: title + source + URL + body @@ -604,11 +623,23 @@ async def _fetch_document_text( content_parts.append(body_markdown) return "\n".join(content_parts) elif doc_type == "deck_card": + # Deck card IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); malformed payloads surface in + # logs rather than getting swallowed by the broad except below. + # The numeric check covers both the metadata-fast-path and the + # iteration fallback below. + if not is_valid_nextcloud_doc_id(doc_id): + logger.warning( + "Expected numeric deck_card doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch card from Deck API # Try to get board_id/stack_id from Qdrant metadata (O(1) lookup) # Otherwise fall back to iteration (legacy data) card = None - deck_metadata = await _get_deck_metadata_from_qdrant(user_id, int(doc_id)) + deck_metadata = await _get_deck_metadata_from_qdrant(user_id, doc_id) if deck_metadata: # Fast path: Direct lookup with known board_id/stack_id diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index c01b0a37..cb15eade 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -8,7 +8,11 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation -from nextcloud_mcp_server.search.algorithms import SearchAlgorithm, SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchAlgorithm, + SearchResult, + build_search_result_from_point, +) from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -134,64 +138,20 @@ class SemanticSearchAlgorithm(SearchAlgorithm): # Deduplicate by (doc_id, doc_type, chunk_start, chunk_end) # This allows multiple chunks from same doc, but removes duplicate chunks - seen_chunks = set() - results = [] + seen_chunks: set[tuple[str, str, Any, Any]] = set() + results: list[SearchResult] = [] - for result in search_response.points: - if result.payload is None: + for point in search_response.points: + sr = build_search_result_from_point(point) + if sr is None: continue - # doc_id can be int (notes) or str (files - file paths) - doc_id = result.payload["doc_id"] - doc_type = result.payload.get("doc_type", "note") - chunk_start = result.payload.get("chunk_start_offset") - chunk_end = result.payload.get("chunk_end_offset") - chunk_key = (doc_id, doc_type, chunk_start, chunk_end) - # Skip if we've already seen this exact chunk + chunk_key = (sr.id, sr.doc_type, sr.chunk_start_offset, sr.chunk_end_offset) if chunk_key in seen_chunks: continue - seen_chunks.add(chunk_key) - # Build metadata dict with common fields - metadata = { - "chunk_index": result.payload.get("chunk_index"), - "total_chunks": result.payload.get("total_chunks"), - } - - # Add file-specific metadata for PDF viewer - if doc_type == "file" and (path := result.payload.get("file_path")): - metadata["path"] = path - - # Add deck_card-specific metadata for frontend URL construction - # and verify-on-read (ADR-019) — both board_id and stack_id are - # required to call deck.get_card without an O(boards × stacks) - # iteration fallback. - if doc_type == "deck_card": - if board_id := result.payload.get("board_id"): - metadata["board_id"] = board_id - if stack_id := result.payload.get("stack_id"): - metadata["stack_id"] = stack_id - - # Return unverified results (verification happens at output stage) - results.append( - SearchResult( - id=doc_id, - doc_type=doc_type, - title=result.payload.get("title", "Untitled"), - excerpt=result.payload.get("excerpt", ""), - score=result.score, - metadata=metadata, - chunk_start_offset=result.payload.get("chunk_start_offset"), - chunk_end_offset=result.payload.get("chunk_end_offset"), - page_number=result.payload.get("page_number"), - page_count=result.payload.get("page_count"), - chunk_index=result.payload.get("chunk_index", 0), - total_chunks=result.payload.get("total_chunks", 1), - point_id=str(result.id), # Qdrant point ID for batch retrieval - ) - ) - + results.append(sr) if len(results) >= limit: break diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index bb3fb678..480f3515 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -42,6 +42,7 @@ from nextcloud_mcp_server.search.algorithms import ( NextcloudClientProtocol, SearchResult, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.eviction import delete_document_points logger = logging.getLogger(__name__) @@ -49,7 +50,7 @@ logger = logging.getLogger(__name__) BatchVerifier = Callable[ [NextcloudClientProtocol, list[SearchResult], anyio.Semaphore], - Awaitable[set[int | str]], + Awaitable[set[str]], ] """(client, results, semaphore) -> set of doc_ids accessible to the user.""" @@ -75,9 +76,9 @@ async def _verify_notes( client: NextcloudClientProtocol, results: list[SearchResult], semaphore: anyio.Semaphore, -) -> set[int | str]: +) -> set[str]: # safe: cooperative concurrency, no lock needed (see verify_search_results) - accessible: set[int | str] = set() + accessible: set[str] = set() async def check(result: SearchResult) -> None: doc_id = result.id @@ -130,9 +131,9 @@ async def _verify_files( client: NextcloudClientProtocol, results: list[SearchResult], semaphore: anyio.Semaphore, -) -> set[int | str]: +) -> set[str]: # safe: cooperative concurrency, no lock needed (see verify_search_results) - accessible: set[int | str] = set() + accessible: set[str] = set() async def check(result: SearchResult) -> None: doc_id = result.id @@ -201,9 +202,9 @@ async def _verify_deck_cards( client: NextcloudClientProtocol, results: list[SearchResult], semaphore: anyio.Semaphore, -) -> set[int | str]: +) -> set[str]: # safe: cooperative concurrency, no lock needed (see verify_search_results) - accessible: set[int | str] = set() + accessible: set[str] = set() async def check(result: SearchResult) -> None: doc_id = result.id @@ -283,7 +284,7 @@ async def _verify_news_items( client: NextcloudClientProtocol, results: list[SearchResult], semaphore: anyio.Semaphore, -) -> set[int | str]: +) -> set[str]: """Batch-verify news items with a single fetch. The Nextcloud News API has no per-item endpoint, so ``news.get_item`` is @@ -386,8 +387,27 @@ async def _verify_news_items( # for THAT item only — not the whole batch. Mirrors the per-item # shape of the notes/files/deck verifiers. See the granularity note # above for why this is narrower than the API-response failure path. - accessible: set[int | str] = set() + accessible: set[str] = set() for d in doc_ids: + # SearchResult.id is always str (Qdrant payload doc_id is keyword- + # indexed; producers stringify on write). Pass through verbatim. + if not is_valid_nextcloud_doc_id(d): + # The news API has no per-item endpoint, so a malformed doc_id + # cannot be verified against the source of truth. Err toward + # false-positive (keep in results) over false-negative (drop a + # potentially legitimate result) — matches the same conservative + # posture _verify_notes and _verify_deck_cards take for + # non-numeric IDs. The producer-side validation is the real + # security boundary; the verifier is defence-in-depth. + logger.warning( + "Malformed news_item doc_id %r in verifier; keeping to " + "avoid dropping a potentially legitimate result (news API " + "has no per-item endpoint, so cannot verify against source " + "of truth — false-positive preferred over false-negative)", + d, + ) + accessible.add(d) + continue try: if int(d) in present_ids: accessible.add(d) @@ -474,7 +494,7 @@ async def verify_search_results( # deduplicated batch. We pick one SearchResult per (id, doc_type) to carry # metadata (path, board_id/stack_id) into the verifier — chunks of the # same document share these fields, so any chunk works. - by_type: dict[str, dict[int | str, SearchResult]] = {} + by_type: dict[str, dict[str, SearchResult]] = {} for r in results: by_type.setdefault(r.doc_type, {}).setdefault(r.id, r) @@ -494,7 +514,7 @@ async def verify_search_results( # same write. Adding a lock would be dead weight; using ``anyio.Lock`` # here would force serialization on a path that is intentionally # parallel. - accessible_by_type: dict[str, set[int | str]] = {} + accessible_by_type: dict[str, set[str]] = {} async def run_verifier(doc_type: str, unique_results: list[SearchResult]) -> None: verifier = _VERIFIERS.get(doc_type) @@ -526,7 +546,7 @@ async def verify_search_results( tg.start_soon(run_verifier, doc_type, list(id_to_result.values())) # Compute (doc_id, doc_type) pairs that failed verification - inaccessible: set[tuple[int | str, str]] = set() + inaccessible: set[tuple[str, str]] = set() for doc_type, id_to_result in by_type.items(): # The .get() default is defensive only — run_verifier always populates # accessible_by_type[doc_type], either with the verifier's result or @@ -537,12 +557,10 @@ async def verify_search_results( inaccessible.add((doc_id, doc_type)) if inaccessible: - # Tag ids with their type (int vs str) so ghost-record logs are - # unambiguous: int 42 and str "42" both render as "42" otherwise. logger.info( "Verification dropped %d inaccessible document(s): %s", len(inaccessible), - sorted((f"{type(d).__name__}:{d}", t) for d, t in inaccessible), + sorted(inaccessible), ) # Filter results, preserving order. All chunks of an inaccessible document @@ -565,7 +583,7 @@ async def verify_search_results( # complete by the time `verify_search_results` returns. if evict_on_missing and inaccessible: - async def evict(doc_id: int | str, doc_type: str) -> None: + async def evict(doc_id: str, doc_type: str) -> None: try: await delete_document_points(doc_id, doc_type, user_id) except Exception as e: diff --git a/nextcloud_mcp_server/server/contacts.py b/nextcloud_mcp_server/server/contacts.py index f495f6fd..ffa35bcf 100644 --- a/nextcloud_mcp_server/server/contacts.py +++ b/nextcloud_mcp_server/server/contacts.py @@ -197,7 +197,11 @@ def configure_contacts_tools(mcp: FastMCP): hay_parts: list[str] = [] if contact.fn: hay_parts.append(contact.fn.lower()) - nickname = contact.custom_fields.get("nickname") if contact.custom_fields else None + nickname = ( + contact.custom_fields.get("nickname") + if contact.custom_fields + else None + ) if nickname: hay_parts.append(str(nickname).lower()) for e in contact.emails: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index c1beb208..e8d1a396 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -224,12 +224,11 @@ def configure_semantic_tools(mcp: FastMCP): search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. - # SearchResult.id is typed `int | str` for forward-compat with future - # doc_types, but every currently indexed type uses numeric ids and - # the MCP response model narrows to `int`. Casting here makes the - # narrowing explicit and surfaces any future string-id type as a - # loud failure at the boundary instead of silently widening the - # public API. + # SearchResult.id is `str` (Qdrant keyword-indexed payload), but + # every currently indexed type uses numeric ids and the MCP response + # model narrows to `int`. Casting here makes the narrowing explicit + # and surfaces any future non-numeric-id type as a loud failure at + # the boundary instead of silently widening the public API. results = [] for r in search_results: try: @@ -304,7 +303,10 @@ def configure_semantic_tools(mcp: FastMCP): chunk_context = await get_chunk_with_context( nc_client=client, user_id=username, - doc_id=result.id, + # SemanticSearchResult.id is the int-narrowed + # public form; get_chunk_with_context queries + # Qdrant where doc_id is keyword-indexed as str. + doc_id=str(result.id), doc_type=result.doc_type, chunk_start=result.chunk_start_offset, chunk_end=result.chunk_end_offset, diff --git a/nextcloud_mcp_server/utils/validation.py b/nextcloud_mcp_server/utils/validation.py new file mode 100644 index 00000000..2f9d0287 --- /dev/null +++ b/nextcloud_mcp_server/utils/validation.py @@ -0,0 +1,15 @@ +"""Shared validators for primitive types crossing system boundaries.""" + +import re + +# Nextcloud object IDs are unsigned ints from MySQL AUTO_INCREMENT, which +# starts at 1. Restrict to ASCII positive integers to exclude Unicode digit +# classes (e.g. superscripts, Arabic-Indic numerals) that pass str.isdigit() +# / str.isdecimal() but would never be valid Nextcloud IDs, and to reject "0" +# and leading zeros. +_NEXTCLOUD_DOC_ID_RE = re.compile(r"^[1-9][0-9]*$") + + +def is_valid_nextcloud_doc_id(value: str) -> bool: + """True iff `value` is the str form of a positive ASCII integer (>= 1).""" + return bool(_NEXTCLOUD_DOC_ID_RE.fullmatch(value)) diff --git a/nextcloud_mcp_server/vector/eviction.py b/nextcloud_mcp_server/vector/eviction.py index 44874e08..8c7555a7 100644 --- a/nextcloud_mcp_server/vector/eviction.py +++ b/nextcloud_mcp_server/vector/eviction.py @@ -18,7 +18,7 @@ logger = logging.getLogger(__name__) async def delete_document_points( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> None: @@ -29,7 +29,7 @@ async def delete_document_points( not present — Qdrant returns successfully with zero points affected. Args: - doc_id: Document ID (int for notes/files/cards/news, str otherwise) + doc_id: Document ID (str — keyword-indexed in Qdrant payload) doc_type: Document type (note, file, deck_card, news_item) user_id: Owner of the points being evicted diff --git a/nextcloud_mcp_server/vector/placeholder.py b/nextcloud_mcp_server/vector/placeholder.py index fcd12bca..4950c2fc 100644 --- a/nextcloud_mcp_server/vector/placeholder.py +++ b/nextcloud_mcp_server/vector/placeholder.py @@ -31,7 +31,7 @@ from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) -def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str: +def _generate_placeholder_id(doc_type: str, doc_id: str) -> str: """Generate deterministic UUID for placeholder point. Args: @@ -46,7 +46,7 @@ def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str: async def write_placeholder_point( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, modified_at: int, @@ -60,7 +60,7 @@ async def write_placeholder_point( processing completes. Args: - doc_id: Document ID (int for notes/files) + doc_id: Document ID (always str — see DocumentTask) doc_type: Document type (note, file, etc.) user_id: User ID who owns the document modified_at: Document modification timestamp @@ -135,7 +135,7 @@ async def write_placeholder_point( async def query_document_metadata( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> dict | None: @@ -185,7 +185,7 @@ async def query_document_metadata( async def delete_placeholder_point( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> None: @@ -230,7 +230,7 @@ async def delete_placeholder_point( async def update_placeholder_status( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, status: str, diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index efd9e0a8..e1e745af 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -1,10 +1,17 @@ """Qdrant client wrapper.""" import logging +from typing import Any +import anyio from qdrant_client import AsyncQdrantClient, models from qdrant_client.http.exceptions import UnexpectedResponse -from qdrant_client.models import Distance, VectorParams +from qdrant_client.models import ( + Distance, + PayloadSchemaType, + PointStruct, + VectorParams, +) from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_embedding_service @@ -12,8 +19,462 @@ from nextcloud_mcp_server.embedding import get_embedding_service logger = logging.getLogger(__name__) -# Singleton instance +# Payload fields filtered by exact-match in scanner/processor/placeholder/eviction +# and the chunk-context lookup path. Qdrant requires a payload index for any +# field used in a FieldCondition; without one, queries fail with HTTP 400 +# ("Index required but not found") on instances that enforce strict-mode +# index-required filtering (Qdrant Cloud, network mode with strict settings). +# The three string fields (doc_id, user_id, doc_type) carry str values after +# producer normalization, so KEYWORD is the correct schema. is_placeholder is +# the bool used by ``get_placeholder_filter`` and ``delete_placeholder_point`` +# (see vector/placeholder.py), so it gets BOOL. chunk_index is the int used by +# ``_get_chunk_by_index_from_qdrant`` and ``get_chunk_bbox_and_page_from_qdrant`` +# (see search/context.py) — the always-indexed fast path that the offset-based +# fallback exists to avoid; it has to actually be indexed for that promise to +# hold on Qdrant Cloud strict mode. chunk_start_offset / chunk_end_offset are +# the ints used by the legacy offset fallback in the same module — pre-#75 +# clients have no chunk_index payload, so the offset path still has to work +# (or 400 silently and return None on Qdrant Cloud strict mode). +_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = { + "doc_id": PayloadSchemaType.KEYWORD, + "user_id": PayloadSchemaType.KEYWORD, + "doc_type": PayloadSchemaType.KEYWORD, + "is_placeholder": PayloadSchemaType.BOOL, + "chunk_index": PayloadSchemaType.INTEGER, + "chunk_start_offset": PayloadSchemaType.INTEGER, + "chunk_end_offset": PayloadSchemaType.INTEGER, +} + +# Sentinel point that records "this collection has been backfilled to str +# doc_id". Written after a successful pass of _backfill_doc_id_to_string so +# subsequent restarts can short-circuit the O(N) scroll. Carries no +# user_id/doc_id/doc_type, so production search filters (which always +# require user_id) never see it. In :memory: mode the sentinel does not +# survive a restart — the scroll runs every start, but is a no-op against +# an empty in-memory collection. +_DOC_ID_BACKFILL_SENTINEL_ID: str = "00000000-0000-0000-0000-d0c1d0d1d0c1" +_DOC_ID_BACKFILL_SENTINEL_PAYLOAD: dict[str, str] = {"_migration_marker": "doc_id_v1"} + +# Singleton instance + init lock. The lock serialises concurrent first +# callers so the idempotent-but-expensive startup migration +# (``_backfill_doc_id_to_string`` + ``_ensure_payload_indexes``) only runs +# once per process. Steady-state callers hit the fast path above the lock +# and never acquire it. The lock is lazy-initialised inside +# ``get_qdrant_client`` rather than constructed at module import time: +# anyio's docs are explicit that synchronization primitives should be +# instantiated within an async context, and ``anyio_mode = "auto"`` in +# pyproject.toml means tests can run under trio where eager construction +# would fail. Construction is safe under cooperative multitasking — there +# is no ``await`` between the None-check and the assignment, so two +# coroutines cannot both create a lock. _qdrant_client: AsyncQdrantClient | None = None +_qdrant_init_lock: anyio.Lock | None = None + + +async def _create_one_payload_index( + client: AsyncQdrantClient, + collection_name: str, + field: str, + schema_type: PayloadSchemaType, +) -> bool: + """Create one payload index with per-field error containment. + + Returns True on success or benign 400 schema-conflict (caller treats as + indexed). Returns False if the field should be added to the caller's + failed-fields list. Never re-raises: the singleton in + ``get_qdrant_client`` is already assigned by the time this runs, so + propagating a network blip would leave the process holding a usable + client with the migration silently incomplete. + """ + try: + await client.create_payload_index( + collection_name=collection_name, + field_name=field, + field_schema=schema_type, + wait=True, + ) + logger.info("Created %s payload index on '%s'", schema_type.name, field) + return True + except UnexpectedResponse as e: + body = getattr(e, "content", b"") or b"" + body_text = body.decode("utf-8", errors="replace") + # 400 is the expected schema-conflict path (index already exists + # with a different type). Verified for Qdrant OSS, where an + # idempotent re-create against a matching schema returns 200; if + # Qdrant Cloud diverges and returns 400 for benign re-creates, + # the WARNING below will fire on every restart against an + # already-indexed collection — read the response body before + # treating that as a real schema conflict. 5xx is unexpected — + # keep the loop going so the remaining fields still get + # attempted, but log at error so operators see it. + if e.status_code == 400: + logger.warning( + "Schema conflict on payload index '%s': %s", field, body_text + ) + # Treat schema conflict the same as a wrong-type index + # discovered via `existing_schema` in `_ensure_payload_indexes` + # (lines 195-206) — both are "the index present is not the + # one we'd build", so the consolidated `Payload index + # creation incomplete` summary should fire in both cases. + # Without this, tenants whose payload_schema is hidden from + # their JWT (Qdrant Cloud collection-scoped tokens) would + # only see the per-field WARNING and miss the summary. + return False + logger.error( + "Unexpected error creating payload index on '%s' (status %s): %s", + field, + e.status_code, + body_text, + ) + return False + except Exception: + # Raw network / timeout failures (httpx.ConnectError, + # asyncio.TimeoutError, etc.) reach here — outside the HTTP-status + # taxonomy that UnexpectedResponse covers. Same containment + # rationale as above: one transient failure on one field must not + # skip the rest, and the singleton in get_qdrant_client is already + # assigned by this point so re-raising would leave the process + # holding a usable client with the migration silently incomplete. + logger.error( + "Network error creating payload index on '%s'; " + "field will remain unindexed until next successful restart", + field, + exc_info=True, + ) + return False + + +async def _ensure_payload_indexes( + client: AsyncQdrantClient, + collection_name: str, + existing_schema: dict[str, Any] | None = None, +) -> None: + """Create payload indexes for fields used in exact-match filters. + + Each entry in ``_PAYLOAD_INDEX_FIELDS`` is created with its declared + schema type (KEYWORD for string fields, BOOL for ``is_placeholder``, + INTEGER for ``chunk_index``). Skips fields that are already in + ``existing_schema`` so routine restarts make no Qdrant write round-trips + and emit no INFO log lines. Per-field error handling (schema conflicts, + network errors) lives in ``_create_one_payload_index``; this loop is + flat so a single transient failure on one field does not skip the rest. + + Args: + client: Qdrant client instance. + collection_name: Target collection. + existing_schema: The collection's current ``payload_schema``. If + ``None``, this function fetches it via ``get_collection``; + callers that have already fetched the collection info (e.g. + ``get_qdrant_client``'s dimension-validation step) should pass + it through to avoid a duplicate round-trip. + """ + # Mirror the broad swallow in `_backfill_doc_id_to_string`: the singleton + # in `get_qdrant_client` is already assigned by the time this function + # runs, so a transient `get_collection` failure (timeout, DNS blip) + # propagating out would leave the process holding a usable client with + # the migration silently skipped on every subsequent call. Log ERROR + # with exc_info and return; the next process restart retries from scratch. + if existing_schema is None: + try: + collection_info = await client.get_collection(collection_name) + except Exception: + logger.error( + "Failed to fetch collection info for '%s'; payload indexes not " + "created. Will retry on next restart.", + collection_name, + exc_info=True, + ) + return + existing_schema = collection_info.payload_schema or {} + + failed_fields: list[str] = [] + for field, schema_type in _PAYLOAD_INDEX_FIELDS.items(): + if field in existing_schema: + # Index already present. Confirm the existing schema type matches + # what we'd create — a pre-existing collection with `doc_id` + # indexed as INTEGER (the bug this PR fixes) would otherwise + # silently survive here, and searches using + # MatchValue(value="123") would keep failing with HTTP 400 on + # Qdrant Cloud strict mode. Compare via PayloadSchemaType + # equality; PayloadIndexInfo.data_type is the same enum + # we wrote with. + existing_info = existing_schema[field] + existing_type = getattr(existing_info, "data_type", None) + if existing_type is not None and existing_type != schema_type: + logger.warning( + "Payload index on '%s' has wrong schema type " + "(got %s, expected %s); searches filtering on this " + "field will fail with HTTP 400 until the index is " + "dropped and recreated. See docs/configuration.md " + "for the recovery procedure.", + field, + getattr(existing_type, "name", existing_type), + schema_type.name, + ) + failed_fields.append(field) + # Either way, skip the create call: a matching index needs no + # work, and a mismatch must not be auto-repaired (operator + # intervention only — see docs/configuration.md). + continue + if not await _create_one_payload_index( + client, collection_name, field, schema_type + ): + failed_fields.append(field) + + # A single per-field ERROR line is easy to miss in startup noise. Surface + # the partial-failure summary at WARNING so operators auditing the log + # for the post-startup state see a single line listing every missing + # index. See docs/configuration.md for the recovery procedure. + if failed_fields: + logger.warning( + "Payload index creation incomplete on '%s' — fields without indexes: %s. " + "Searches filtering on these fields will fail with HTTP 400 " + "(`Index required but not found`) until the next successful restart.", + collection_name, + ", ".join(failed_fields), + ) + + +def _group_int_doc_ids(points: list[Any]) -> tuple[dict[str, list[Any]], int]: + """Group point IDs whose payload carries an int doc_id, keyed by str(doc_id). + + Returns ``(by_value, scanned)`` where ``scanned`` is the total number of + points inspected (str / missing payloads count toward scanned but are not + grouped). Pulled out of ``_backfill_doc_id_to_string`` to keep that + function's cognitive complexity within the project's limit. + + Point IDs widen to ``Any`` to satisfy the qdrant client's + ``PointsSelector`` signature (UUID / int / str unions) without re-spelling + the full type union here. + """ + by_value: dict[str, list[Any]] = {} + scanned = 0 + for point in points: + scanned += 1 + # Qdrant client typing allows None payload even when with_payload was + # requested; defensive default so the type checker is happy. + payload = point.payload or {} + value = payload.get("doc_id") + if value is None or isinstance(value, str): + continue + # Strict type check: bool is a subclass of int in Python, so an + # `isinstance(value, int)` guard would let `True`/`False` slip + # through and be stringified to `"True"`/`"False"` — which the + # keyword index would never match and the verification side + # would later reject. Producers only ever write int or str; + # anything else (bool, float, etc.) is a producer bug. Skip + # and log loudly instead. + if type(value) is not int: + logger.warning( + "Unexpected doc_id type %s on point %s; skipping rewrite", + type(value).__name__, + point.id, + ) + continue + by_value.setdefault(str(value), []).append(point.id) + return by_value, scanned + + +async def _apply_backfill_writes( + client: AsyncQdrantClient, + collection_name: str, + by_value: dict[str, list[Any]], +) -> int: + """Apply one ``set_payload`` per stringified doc_id; return rewritten count. + + ``wait=True`` is load-bearing for two reasons: + + 1. It ensures each batch commits before the scroll loop advances to + the next page (and before the sentinel is written by the caller + after ``_backfill_doc_id_to_string`` returns). A crash mid-scroll + leaves no sentinel, so the next restart re-scrolls — and that + re-scroll only sees a deterministic, committed partial state when + each batch was committed synchronously. Fire-and-forget writes + would race the next scroll page against still-in-flight rewrites. + 2. ``_ensure_payload_indexes`` runs after this backfill returns and + can only index already-committed payload values. Without + ``wait=True``, the keyword index could be built over points whose + payloads are still int values in flight to disk, leaving them + silently invisible to ``FieldCondition`` filters. + """ + rewritten = 0 + for str_val, point_ids in by_value.items(): + await client.set_payload( + collection_name=collection_name, + payload={"doc_id": str_val}, + points=point_ids, + wait=True, + ) + rewritten += len(point_ids) + return rewritten + + +async def _backfill_doc_id_to_string( + client: AsyncQdrantClient, collection_name: str, dimension: int +) -> None: + """Rewrite legacy integer doc_id payloads to strings. + + Producers now uniformly write str(doc_id), but historical points may carry + int values from before normalization. A KEYWORD index does not match int + payloads, so any leftover int doc_ids would be silently invisible to + filters. Scrolls all points once, converts in-place, and writes a + sentinel point on success; subsequent restarts retrieve the sentinel + and skip the scroll entirely. Idempotent in both directions (a second + pass on a migrated collection short-circuits via the sentinel; a + second pass with the sentinel manually deleted is the same zero-write + scroll the first pass would do on an already-clean collection). + + Within each scroll batch, points sharing the same int doc_id are batched + into a single ``set_payload`` call to minimize Qdrant round-trips. + + Only called for **existing** collections (see the + ``if collection_name in collection_names`` branch in + ``get_qdrant_client``); brand-new collections skip the backfill since + there can be no legacy int payloads in a freshly created collection. + + Args: + client: Qdrant client instance. + collection_name: Target collection. + dimension: Dense-vector dimension for the sentinel point's vector, + forwarded by ``get_qdrant_client`` from the embedding model. + Required because the sentinel is upserted into an existing + collection and must match the collection's vector schema. + """ + # Sentinel guard: if the migration ran successfully against this + # collection on a previous start, retrieve() returns the marker point + # and we skip the scroll. Cheap single-key lookup vs. an O(N) scroll. + sentinel = await client.retrieve( + collection_name=collection_name, + ids=[_DOC_ID_BACKFILL_SENTINEL_ID], + with_payload=False, + with_vectors=False, + ) + if sentinel: + logger.debug( + "doc_id backfill sentinel found on '%s'; skipping scroll", + collection_name, + ) + return + + logger.info( + "Running doc_id backfill on '%s' (one-time migration on first " + "start after upgrade; subsequent restarts skip via sentinel)", + collection_name, + ) + + rewritten = 0 + scanned = 0 + batch_num = 0 + # Qdrant scroll returns next_offset as PointId | None — keep it untyped here + # so the qdrant client's full union (UUID/int/str/PointId) flows through. + next_offset = None + # Smaller than ``_DELETION_TRACKING_PAGE_SIZE = 1024`` in + # ``vector/scanner.py`` because this is a read-write path: every batch + # is followed by a ``set_payload`` upsert, and 256-point upserts are + # the working size where Qdrant comfortably accepts writes without + # timing out under load. The scanner-side scroll has no per-page write + # round-trip, so it can use a larger page. + batch_size = 256 + # Log progress every N batches so a long-running migration on a large + # collection (≥ 50k points) doesn't look like a startup hang. At batch + # size 256, every 20 batches ≈ 5 120 points scanned. + progress_log_every = 20 + + # A transient Qdrant failure mid-scroll (network blip, timeout) must not + # crash startup. The singleton in get_qdrant_client is already assigned + # by the time this runs, so re-raising here would leave the process in + # a half-initialized state where the next call returns the cached + # client and skips this migration entirely. Catch broadly, log with + # exc_info, and return without writing the sentinel — the next process + # restart will retry from scratch. The sentinel write is NOT covered by + # this try/except: a failure there means the data migration succeeded + # and only the short-circuit marker is missing, which is a different + # (and milder) condition than a scroll failure. + try: + while True: + points, next_offset = await client.scroll( + collection_name=collection_name, + limit=batch_size, + offset=next_offset, + with_payload=["doc_id"], + with_vectors=False, + ) + if not points: + break + + batch_num += 1 + by_value, batch_scanned = _group_int_doc_ids(points) + scanned += batch_scanned + rewritten += await _apply_backfill_writes(client, collection_name, by_value) + + if batch_num % progress_log_every == 0: + logger.info( + "doc_id backfill progress on '%s': scanned %d points, " + "rewrote %d so far", + collection_name, + scanned, + rewritten, + ) + + if next_offset is None: + break + except Exception: + logger.error( + "doc_id backfill scroll failed on '%s'; will retry on next restart", + collection_name, + exc_info=True, + ) + return + + # Data backfill succeeded — write the sentinel so a future restart can + # short-circuit. Empty sparse vector mirrors the placeholder.py + # convention (vector/placeholder.py). The dense vector uses a single + # non-zero element instead of all zeros: cosine distance is undefined + # for the zero vector and Qdrant Cloud's strict mode rejects zero-vector + # upserts. The sentinel still never participates in a search (no + # user_id / doc_id / doc_type payload to match), so the exact value + # doesn't matter — it just has to be normalisable. + # A failure here is non-fatal: the data is correct; only the short-circuit + # marker is missing, so the next restart will re-scroll an already-clean + # collection (idempotent zero-write) before retrying the upsert. + sentinel_dense = [1e-9] + [0.0] * (dimension - 1) + sentinel_point = PointStruct( + id=_DOC_ID_BACKFILL_SENTINEL_ID, + vector={ + "dense": sentinel_dense, + "sparse": models.SparseVector(indices=[], values=[]), + }, + payload=dict(_DOC_ID_BACKFILL_SENTINEL_PAYLOAD), + ) + try: + await client.upsert( + collection_name=collection_name, + points=[sentinel_point], + wait=True, + ) + except Exception: + logger.warning( + "doc_id backfill data succeeded on '%s' but sentinel write failed; " + "next restart will re-scroll (idempotent zero-write on clean collection)", + collection_name, + exc_info=True, + ) + return + + if rewritten: + logger.info( + "doc_id backfill complete on '%s': rewrote %d/%d int payloads to str", + collection_name, + rewritten, + scanned, + ) + else: + logger.info( + "doc_id backfill complete on '%s': %d points scanned, none required " + "rewriting (collection already in str form)", + collection_name, + scanned, + ) async def get_qdrant_client() -> AsyncQdrantClient: @@ -33,130 +494,219 @@ async def get_qdrant_client() -> AsyncQdrantClient: Raises: Exception: If Qdrant connection fails or collection creation fails """ - global _qdrant_client + global _qdrant_client, _qdrant_init_lock - if _qdrant_client is None: - settings = get_settings() + # Fast path: already initialized — skip lock acquisition for the + # steady-state hot path (every MCP tool call after first start). + if _qdrant_client is not None: + return _qdrant_client - # Detect mode and initialize client accordingly - if settings.qdrant_url: - # Network mode - logger.info(f"Using Qdrant network mode: {settings.qdrant_url}") - _qdrant_client = AsyncQdrantClient( - url=settings.qdrant_url, - api_key=settings.qdrant_api_key, - timeout=30, - ) - elif settings.qdrant_location: - # Local mode (either :memory: or persistent path) - if settings.qdrant_location == ":memory:": - logger.info("Using Qdrant in-memory mode: :memory:") - _qdrant_client = AsyncQdrantClient(":memory:") + # Lazy-create the init lock on first cold-start. Safe under cooperative + # multitasking: there is no ``await`` between the None-check and the + # assignment, so two coroutines cannot both reach the construction. + # See the rationale on _qdrant_init_lock for why eager construction + # would break under the trio backend. + if _qdrant_init_lock is None: + _qdrant_init_lock = anyio.Lock() + + # Slow path: serialise concurrent first-callers so the idempotent-but- + # expensive startup migration (``_backfill_doc_id_to_string`` + + # ``_ensure_payload_indexes``) runs exactly once. Without this lock, + # parallel cold-start callers would all enter the init block, run the + # migration N times, and emit duplicate "skip-because-exists" warnings + # from the index helper — annoying log noise but not data corruption. + async with _qdrant_init_lock: + # Double-checked: another waiter may have initialized while we + # blocked on the lock. + if _qdrant_client is None: + settings = get_settings() + + # Build the client into a local ``provisional`` and only publish + # it to the global ``_qdrant_client`` after the migration awaits + # below have all completed. The fast-path check at the top of + # this function reads ``_qdrant_client`` without the lock, so + # publishing the constructed-but-unmigrated client would let a + # concurrent caller short-circuit the lock and fire a filtered + # search before ``_ensure_payload_indexes`` runs — that search + # would 400 with "Index required but not found". + provisional: AsyncQdrantClient + + # Detect mode and initialize client accordingly + if settings.qdrant_url: + # Network mode + logger.info(f"Using Qdrant network mode: {settings.qdrant_url}") + provisional = AsyncQdrantClient( + url=settings.qdrant_url, + api_key=settings.qdrant_api_key, + timeout=30, + ) + elif settings.qdrant_location: + # Local mode (either :memory: or persistent path) + if settings.qdrant_location == ":memory:": + logger.info("Using Qdrant in-memory mode: :memory:") + provisional = AsyncQdrantClient(":memory:") + else: + # Persistent local mode - use path parameter + logger.info( + f"Using Qdrant persistent mode: {settings.qdrant_location}" + ) + provisional = AsyncQdrantClient(path=settings.qdrant_location) else: - # Persistent local mode - use path parameter - logger.info(f"Using Qdrant persistent mode: {settings.qdrant_location}") - _qdrant_client = AsyncQdrantClient(path=settings.qdrant_location) - else: - # Should not happen due to __post_init__ validation, but handle gracefully - logger.warning("No Qdrant mode configured, defaulting to :memory:") - _qdrant_client = AsyncQdrantClient(":memory:") + # Should not happen due to __post_init__ validation, but handle gracefully + logger.warning("No Qdrant mode configured, defaulting to :memory:") + provisional = AsyncQdrantClient(":memory:") - # Get collection name (auto-generated from deployment ID + model) - collection_name = settings.get_collection_name() + # Get collection name (auto-generated from deployment ID + model) + collection_name = settings.get_collection_name() - embedding_service = get_embedding_service() + embedding_service = get_embedding_service() - # Detect dimension dynamically (for OllamaEmbeddingProvider) - if hasattr(embedding_service.provider, "_detect_dimension"): - await embedding_service.provider._detect_dimension() # type: ignore[call-non-callable] + # Detect dimension dynamically (for OllamaEmbeddingProvider) + if hasattr(embedding_service.provider, "_detect_dimension"): + await embedding_service.provider._detect_dimension() # type: ignore[call-non-callable] - expected_dimension = embedding_service.get_dimension() + expected_dimension = embedding_service.get_dimension() - # Existence check folded into the get_collection() call. - # - # In managed multi-tenant Qdrant Cloud setups, per-tenant JWTs are - # scoped to a single collection (`access: [{"collection": "...", - # "access": "rw"}]`) and Qdrant denies the cluster-level meta - # endpoints `GET /collections` (used by `get_collections()`) and - # `GET /collections/{name}/exists` (used by `collection_exists()`) - # with 403 Forbidden — by design, since listing or probing - # collections cluster-wide is a tenant-isolation boundary. - # `GET /collections/{name}` (the underlying call for - # `get_collection()`) is the only existence-probe permitted on a - # collection-scoped JWT — it returns 200 with the collection - # detail on hit and 404 on miss. - logger.debug(f"Fetching collection '{collection_name}' details...") - collection_info = None - try: - collection_info = await _qdrant_client.get_collection(collection_name) - except UnexpectedResponse as exc: - if exc.status_code != 404: - raise - logger.debug(f"Collection '{collection_name}' not found (404).") + # Existence check folded into the get_collection() call. + # + # In managed multi-tenant Qdrant Cloud setups, per-tenant JWTs are + # scoped to a single collection (`access: [{"collection": "...", + # "access": "rw"}]`) and Qdrant denies the cluster-level meta + # endpoints `GET /collections` (used by `get_collections()`) and + # `GET /collections/{name}/exists` (used by `collection_exists()`) + # with 403 Forbidden — by design, since listing or probing + # collections cluster-wide is a tenant-isolation boundary. + # `GET /collections/{name}` (the underlying call for + # `get_collection()`) is the only existence-probe permitted on a + # collection-scoped JWT — it returns 200 with the collection + # detail on hit and 404 on miss. + logger.debug(f"Fetching collection '{collection_name}' details...") + collection_info = None + try: + collection_info = await provisional.get_collection(collection_name) + except UnexpectedResponse as exc: + if exc.status_code != 404: + raise + logger.debug(f"Collection '{collection_name}' not found (404).") + except ValueError as exc: + # Local/in-memory qdrant_client raises ValueError(f"Collection + # {name} not found") instead of UnexpectedResponse — see + # qdrant_client/local/async_qdrant_local.py. Match on the + # message rather than catching every ValueError so genuine + # programming bugs (bad collection_name validation, etc.) + # still propagate. PR #779 introduced this regression by + # switching the existence probe from `collection_exists()` + # (which returned a bool in both modes) to `get_collection` + # without accounting for the local-mode signalling + # convention; the failing single-user / login-flow / + # multi-user-basic CI jobs all exercise this path. + if "not found" not in str(exc): + raise + logger.debug(f"Collection '{collection_name}' not found (local mode).") - if collection_info is not None: - # Collection exists - validate dimensions - logger.debug( - f"Collection '{collection_name}' found, validating dimensions..." - ) - # Handle both named vectors (dict) and legacy single vector - vectors = collection_info.config.params.vectors - if isinstance(vectors, dict): - actual_dimension = vectors["dense"].size - else: - # Type narrowing: vectors must be VectorParams if not dict - assert isinstance(vectors, VectorParams) - actual_dimension = vectors.size + if collection_info is not None: + # Collection exists - validate dimensions + logger.debug( + f"Collection '{collection_name}' found, validating dimensions..." + ) + # Handle both named vectors (dict) and legacy single vector + vectors = collection_info.config.params.vectors + if isinstance(vectors, dict): + actual_dimension = vectors["dense"].size + else: + # Type narrowing: vectors must be VectorParams if not dict + assert isinstance(vectors, VectorParams) + actual_dimension = vectors.size - # Validate dimension matches - if actual_dimension != expected_dimension: - embedding_model = settings.get_embedding_model_name() - raise ValueError( - f"Dimension mismatch for collection '{collection_name}':\n" - f" Expected: {expected_dimension} (from embedding model '{embedding_model}')\n" - f" Found: {actual_dimension}\n" - f"This usually means you changed the embedding model.\n" - f"Solutions:\n" - f" 1. Delete the old collection: Collection will be recreated with new dimensions\n" - f" 2. Set QDRANT_COLLECTION to use a different collection name\n" - f" 3. Revert to the original embedding model" + # Validate dimension matches + if actual_dimension != expected_dimension: + embedding_model = settings.get_embedding_model_name() + raise ValueError( + f"Dimension mismatch for collection '{collection_name}':\n" + f" Expected: {expected_dimension} (from embedding model '{embedding_model}')\n" + f" Found: {actual_dimension}\n" + f"This usually means you changed the embedding model.\n" + f"Solutions:\n" + f" 1. Delete the old collection: Collection will be recreated with new dimensions\n" + f" 2. Set QDRANT_COLLECTION to use a different collection name\n" + f" 3. Revert to the original embedding model" + ) + + logger.info( + f"Using existing Qdrant collection: {collection_name} " + f"(dimension={actual_dimension}, model={settings.get_embedding_model_name()})" ) - logger.info( - f"Using existing Qdrant collection: {collection_name} " - f"(dimension={actual_dimension}, model={settings.get_embedding_model_name()})" - ) + # Existing collections may pre-date the doc_id normalization / + # payload-index work. Backfill before creating the index so the + # index covers every point. Pass the already-fetched + # collection_info.payload_schema through to avoid a redundant + # get_collection round-trip on every restart — safe because + # _backfill_doc_id_to_string only rewrites payload *values*, + # never schema or indexes, so the snapshot remains accurate + # across the backfill call. + await _backfill_doc_id_to_string( + provisional, collection_name, expected_dimension + ) + await _ensure_payload_indexes( + provisional, + collection_name, + existing_schema=collection_info.payload_schema or {}, + ) - else: - # Collection doesn't exist - create it - embedding_model = settings.get_embedding_model_name() - logger.info( - f"Collection '{collection_name}' not found, creating with " - f"dimension={expected_dimension}, model={embedding_model}..." - ) - await _qdrant_client.create_collection( - collection_name=collection_name, - vectors_config={ - "dense": VectorParams( - size=expected_dimension, - distance=Distance.COSINE, - ), - }, - sparse_vectors_config={ - "sparse": models.SparseVectorParams( - index=models.SparseIndexParams( - on_disk=False, - ) - ), - }, - ) - logger.info( - f"Created Qdrant collection: {collection_name}\n" - f" Dense vector dimension: {expected_dimension}\n" - f" Dense embedding model: {embedding_model}\n" - f" Sparse vectors: BM25 (for hybrid search)\n" - f" Distance: COSINE\n" - f"Background sync will index all documents with dense + sparse vectors." - ) + else: + # Collection doesn't exist - create it + embedding_model = settings.get_embedding_model_name() + logger.info( + f"Collection '{collection_name}' not found, creating with " + f"dimension={expected_dimension}, model={embedding_model}..." + ) + await provisional.create_collection( + collection_name=collection_name, + vectors_config={ + "dense": VectorParams( + size=expected_dimension, + distance=Distance.COSINE, + ), + }, + sparse_vectors_config={ + "sparse": models.SparseVectorParams( + index=models.SparseIndexParams( + on_disk=False, + ) + ), + }, + ) + logger.info( + f"Created Qdrant collection: {collection_name}\n" + f" Dense vector dimension: {expected_dimension}\n" + f" Dense embedding model: {embedding_model}\n" + f" Sparse vectors: BM25 (for hybrid search)\n" + f" Distance: COSINE\n" + f"Background sync will index all documents with dense + sparse vectors." + ) + # Freshly created collection has no payload schema yet; pass + # {} explicitly to skip the otherwise-redundant + # get_collection call. Every field in _PAYLOAD_INDEX_FIELDS + # then goes through create_payload_index; on a brand-new + # collection none of them exist yet, so the WARNING in the + # 400-handler should *never* fire on this path. If it does + # on Qdrant Cloud first-start, that points at a + # deployment-level issue (race with a concurrent creator, + # implicit auto-indexes, etc.) worth investigating before + # suppressing. + await _ensure_payload_indexes( + provisional, collection_name, existing_schema={} + ) + # Publish only after the migration awaits completed. From this + # point on, fast-path callers may short-circuit the lock and + # use the client; every payload index they could filter on now + # exists. + _qdrant_client = provisional + + # Lock released. ``_qdrant_client`` is guaranteed non-None here: + # either the fast path returned earlier, the lock-protected branch + # set it, or a sibling waiter set it before we got the lock. + assert _qdrant_client is not None return _qdrant_client diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 4f4edf3f..c4a2e6a2 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -9,11 +9,13 @@ import random import time from dataclasses import dataclass from email.utils import parsedate_to_datetime +from typing import cast import anyio from anyio.abc import TaskStatus from anyio.streams.memory import MemoryObjectSendStream -from qdrant_client.models import FieldCondition, Filter, MatchValue +from qdrant_client import AsyncQdrantClient +from qdrant_client.models import FieldCondition, Filter, MatchValue, Record from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.client.news import NewsItemType @@ -43,12 +45,62 @@ INDEXED_DOC_TYPES: frozenset[str] = frozenset( ) +# Page size for paginated deletion-tracking scrolls. Chosen to keep per-page +# memory bounded while making the round-trip count manageable in the typical +# < 100 k point per (user_id, doc_type) case. The previous single-page +# ``limit=10_000`` silently truncated deletion sets for any user past the +# cap, so anything indexed beyond the first 10 k was never reconciled. +# +# Intentionally larger than the ``batch_size = 256`` used by +# ``_backfill_doc_id_to_string`` in ``vector/qdrant_client.py``: this is a +# read-only scroll that just collects payloads (no write round-trip per +# point), so the per-page memory budget is the only relevant constraint. +# The 256 there is sized for read-write upsert batches where Qdrant +# accepts ~256-point chunks comfortably without timing out under load. +_DELETION_TRACKING_PAGE_SIZE: int = 1024 + + +async def _scroll_all_points( + qdrant_client: AsyncQdrantClient, + *, + collection_name: str, + scroll_filter: Filter, + payload_fields: list[str], + page_size: int = _DELETION_TRACKING_PAGE_SIZE, +) -> list[Record]: + """Scroll every point matching the filter, paginating until exhausted. + + Replaces the prior single-page ``limit=10_000`` calls that silently + dropped points beyond the first page. Pagination follows Qdrant's + documented contract: ``scroll`` returns ``(points, next_page_offset)`` + and ``next_page_offset`` is ``None`` once the cursor reaches the end. + Errors propagate to the caller — the scanner's outer ``try`` already + handles them by skipping the deletion-tracking pass for this scan + (worse: extra-scan latency; never: bad data). + """ + all_points: list[Record] = [] + offset = None + while True: + points, offset = await qdrant_client.scroll( + collection_name=collection_name, + scroll_filter=scroll_filter, + with_payload=payload_fields, + with_vectors=False, + limit=page_size, + offset=offset, + ) + all_points.extend(points) + if offset is None: + break + return all_points + + @dataclass class DocumentTask: """Document task for processing queue.""" user_id: str - doc_id: int | str # int for files/notes, str for legacy + doc_id: str # Always str — see vector/qdrant_client.py keyword index doc_type: str # "note", "file", "calendar" operation: str # "index" or "delete" modified_at: int @@ -76,11 +128,22 @@ async def get_last_indexed_timestamp(user_id: str) -> int | None: Returns: Unix timestamp of most recently indexed note, or None if no notes indexed yet """ + # TODO: This is O(N) over a user's indexed notes on every incremental + # sync tick. Was accidentally bounded at 10 k before this PR (single- + # page scroll silently truncated); paginating fixed correctness but + # made the unbounded cost visible. Track the max ``indexed_at`` as + # collection metadata or a dedicated sentinel point so this becomes + # O(1). Out of scope for the current PR — see the chunk-context / + # vector-sync follow-up tracker (referenced by the canonical TODO at + # ``api/visualization.py``). try: qdrant_client = await get_qdrant_client() - # Query for user's notes, ordered by indexed_at descending, limit 1 - scroll_result = await qdrant_client.scroll( + # Scroll across every indexed note for this user — paginated so users + # with > 10 k indexed notes still produce a correct max (the prior + # single-page ``limit=10_000`` would have silently undercounted). + points = await _scroll_all_points( + qdrant_client, collection_name=get_settings().get_collection_name(), scroll_filter=Filter( must=[ @@ -88,19 +151,16 @@ async def get_last_indexed_timestamp(user_id: str) -> int | None: FieldCondition(key="doc_type", match=MatchValue(value="note")), ] ), - with_payload=["indexed_at"], - with_vectors=False, - limit=10000, # Get all to find max + payload_fields=["indexed_at"], ) - # Find max indexed_at across all results - num_points = len(scroll_result[0]) if scroll_result[0] else 0 + num_points = len(points) logger.info(f"Found {num_points} indexed notes in Qdrant for user {user_id}") - if scroll_result[0]: + if points: timestamps = [ point.payload.get("indexed_at", 0) - for point in scroll_result[0] + for point in points if point.payload is not None ] max_timestamp = max(timestamps) if timestamps else 0 @@ -210,11 +270,23 @@ async def scan_user_documents( ) # For deletion tracking, get all doc_ids in Qdrant (for incremental sync) - # Note: We no longer bulk-query indexed_at, instead check per-document + # Note: We no longer bulk-query indexed_at, instead check per-document. + # Hoisted to function scope so the file-scroll block below doesn't + # depend on a name bound inside the notes-scroll block; future + # refactors that add an early return between the two blocks would + # otherwise hit an UnboundLocalError. get_qdrant_client is a + # singleton call, so the cost is identical. + qdrant_client = await get_qdrant_client() if not initial_sync else None indexed_doc_ids = set() if not initial_sync: - qdrant_client = await get_qdrant_client() - scroll_result = await qdrant_client.scroll( + # ``assert ... is not None`` would also narrow but raises an + # opaque AssertionError under ``-O`` and at runtime — ``cast`` + # is the conventional zero-cost narrower for branches the type + # checker can't infer from the surrounding ``if not + # initial_sync`` (the ternary above ties the two together). + qdrant_client = cast(AsyncQdrantClient, qdrant_client) + points = await _scroll_all_points( + qdrant_client, collection_name=get_settings().get_collection_name(), scroll_filter=Filter( must=[ @@ -222,15 +294,13 @@ async def scan_user_documents( FieldCondition(key="doc_type", match=MatchValue(value="note")), ] ), - with_payload=["doc_id"], - with_vectors=False, - limit=10000, + payload_fields=["doc_id"], ) indexed_doc_ids = { - point.payload["doc_id"] - for point in (scroll_result[0] or []) - if point.payload is not None + str(point.payload["doc_id"]) + for point in points + if point.payload is not None and "doc_id" in point.payload } logger.debug(f"Found {len(indexed_doc_ids)} indexed documents in Qdrant") @@ -387,7 +457,9 @@ async def scan_user_documents( # Get indexed file IDs from Qdrant (for deletion tracking) indexed_file_ids = set() if not initial_sync: - file_scroll_result = await qdrant_client.scroll( + assert qdrant_client is not None # narrow for the type checker + points = await _scroll_all_points( + qdrant_client, collection_name=settings.get_collection_name(), scroll_filter=Filter( must=[ @@ -395,15 +467,13 @@ async def scan_user_documents( FieldCondition(key="doc_type", match=MatchValue(value="file")), ] ), - limit=10000, # Reasonable limit for file count - with_payload=["doc_id"], - with_vectors=False, + payload_fields=["doc_id"], ) indexed_file_ids = { - point.payload["doc_id"] - for point in (file_scroll_result[0] or []) - if point.payload is not None + str(point.payload["doc_id"]) + for point in points + if point.payload is not None and "doc_id" in point.payload } logger.debug(f"Found {len(indexed_file_ids)} indexed files in Qdrant") @@ -456,7 +526,9 @@ async def scan_user_documents( for file_info in tagged_files: # Files are already filtered by MIME type in find_files_by_tag() file_count += 1 - file_id = file_info["id"] # Use numeric file ID, not path + # Normalize file ID to str — Qdrant doc_id payload is keyword-indexed + # and producers across doc_types must agree on a single type. + file_id = str(file_info["id"]) file_path = file_info["path"] # Keep path for logging nextcloud_file_ids.add(file_id) @@ -482,11 +554,11 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="index", modified_at=modified_at, - file_path=file_path, # Pass file path for content retrieval + file_path=file_path, ) ) file_queued += 1 @@ -545,11 +617,11 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="index", modified_at=modified_at, - file_path=file_path, # Pass file path for content retrieval + file_path=file_path, ) ) file_queued += 1 @@ -579,7 +651,7 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="delete", modified_at=0, @@ -666,7 +738,8 @@ async def scan_news_items( indexed_item_ids: set[str] = set() if not initial_sync: qdrant_client = await get_qdrant_client() - scroll_result = await qdrant_client.scroll( + points = await _scroll_all_points( + qdrant_client, collection_name=settings.get_collection_name(), scroll_filter=Filter( must=[ @@ -674,14 +747,12 @@ async def scan_news_items( FieldCondition(key="doc_type", match=MatchValue(value="news_item")), ] ), - with_payload=["doc_id"], - with_vectors=False, - limit=10000, + payload_fields=["doc_id"], ) indexed_item_ids = { - point.payload["doc_id"] - for point in (scroll_result[0] or []) - if point.payload is not None + str(point.payload["doc_id"]) + for point in points + if point.payload is not None and "doc_id" in point.payload } logger.debug(f"Found {len(indexed_item_ids)} indexed news items in Qdrant") @@ -845,7 +916,8 @@ async def scan_deck_cards( indexed_card_ids: set[str] = set() if not initial_sync: qdrant_client = await get_qdrant_client() - scroll_result = await qdrant_client.scroll( + points = await _scroll_all_points( + qdrant_client, collection_name=settings.get_collection_name(), scroll_filter=Filter( must=[ @@ -853,14 +925,12 @@ async def scan_deck_cards( FieldCondition(key="doc_type", match=MatchValue(value="deck_card")), ] ), - with_payload=["doc_id"], - with_vectors=False, - limit=10000, + payload_fields=["doc_id"], ) indexed_card_ids = { - point.payload["doc_id"] - for point in (scroll_result[0] or []) - if point.payload is not None + str(point.payload["doc_id"]) + for point in points + if point.payload is not None and "doc_id" in point.payload } logger.debug(f"Found {len(indexed_card_ids)} indexed deck cards in Qdrant") diff --git a/nextcloud_mcp_server/vector/visualization.py b/nextcloud_mcp_server/vector/visualization.py index 5ffb4bd1..3da8d2e6 100644 --- a/nextcloud_mcp_server/vector/visualization.py +++ b/nextcloud_mcp_server/vector/visualization.py @@ -70,7 +70,10 @@ async def compute_pca_coordinates( vector = point.vector if vector is not None and point.payload: - doc_id = point.payload.get("doc_id") + # SearchResult.id is str; coerce payload doc_id to match so the + # tuple lookup below succeeds even on legacy int-typed payloads. + raw_doc_id = point.payload.get("doc_id") + doc_id = None if raw_doc_id is None else str(raw_doc_id) chunk_start = point.payload.get("chunk_start_offset") chunk_end = point.payload.get("chunk_end_offset") chunk_key = (doc_id, chunk_start, chunk_end) diff --git a/tests/integration/test_astrolabe_login_flow_provisioning.py b/tests/integration/test_astrolabe_login_flow_provisioning.py new file mode 100644 index 00000000..15d2abbb --- /dev/null +++ b/tests/integration/test_astrolabe_login_flow_provisioning.py @@ -0,0 +1,139 @@ +"""Integration test for Astrolabe's "Enable Semantic Search" OAuth flow on +the `mcp-login-flow` profile. + +Cross-system interface test. Brings together Astrolabe (Nextcloud PHP app +installed at container start by ``app-hooks/post-installation``) with the +``mcp-login-flow`` MCP server over OAuth + the management API. Mirrors +the production-shaped flow that PR #773's recent +`ALLOWED_MGMT_CLIENT` ↔ `astrolabeMcpClientOAuth00000000000` drift was +masking — every management API call from Astrolabe (e.g. +``/api/v1/users/admin/session``) was returning 401 because the +real-deployment client id was not in the test-fixture allowlist, so the +Astrolabe settings page never updated to reflect a successful +authorization. + +This test is **regression coverage** for that class of drift. If the +Astrolabe client id ever falls out of `mcp-login-flow`'s +``ALLOWED_MGMT_CLIENT`` again, the post-redirect assertions here will +fail because the page state stays on ``oauth-required.php``. + +Requires the login-flow stack to be running: + + MCP_SERVER_URL=http://mcp-login-flow:8004 \\ + docker compose --profile login-flow up -d app db mcp-login-flow + +The ``app-hooks/before-starting/26-configure-astrolabe-oauth.sh`` hook +creates the OAuth client with the production-shaped id +``astrolabeMcpClientOAuth00000000000`` automatically when +``MCP_SERVER_URL`` is set, so no fixture-level OIDC client creation is +needed here. +""" + +import logging +import os +import re + +import pytest +from playwright.async_api import Page + +# Reuse helpers from the multi-user-basic Astrolabe test for login + nav. +from tests.integration.test_astrolabe_multi_user_background_sync import ( + login_to_nextcloud, + navigate_to_astrolabe_settings, +) + +logger = logging.getLogger(__name__) + +pytestmark = [pytest.mark.integration, pytest.mark.login_flow] + +NEXTCLOUD_URL = "http://localhost:8080" +ASTROLABE_SETTINGS_URL = f"{NEXTCLOUD_URL}/settings/user/astrolabe" + + +async def _click_enable_semantic_search(page: Page) -> None: + """Click the "Enable Semantic Search" OAuth link on the + ``oauth-required.php`` template that login-flow mode renders to a + not-yet-authorized user. + + Astrolabe's own e2e helper (``third_party/astrolabe/tests/e2e/helpers/ + authorize.ts``) targets the same link by accessible name. + """ + enable_link = page.get_by_role("link", name="Enable Semantic Search") + await enable_link.wait_for(state="visible", timeout=10_000) + logger.info("Clicking 'Enable Semantic Search' OAuth link") + await enable_link.click() + + +async def _grant_oidc_consent(page: Page) -> None: + """Click "Allow" on the Nextcloud OIDC consent screen, if shown. + + Nextcloud may auto-redirect for already-trusted clients, in which + case the consent button never appears — that's not an error. + """ + allow_button = page.get_by_role("button", name=re.compile(r"^allow$", re.I)) + try: + await allow_button.wait_for(state="visible", timeout=10_000) + logger.info("Clicking 'Allow' on OIDC consent") + await allow_button.click(force=True) + except Exception: + logger.info( + "OIDC consent screen not visible — assuming auto-grant for " + "already-trusted client" + ) + + +@pytest.mark.timeout(180) +async def test_enable_semantic_search_completes_oauth_for_login_flow(browser): + """Click the "Enable Semantic Search" link, grant consent, and assert + the post-redirect page reflects a completed authorization. + + The success criterion is intentionally negative: after the OAuth + flow, the original "Enable Semantic Search" link must be gone. If + Astrolabe's management API call is rejected by the MCP server (HTTP + 401, the original bug), the page falls back to the same + ``oauth-required.php`` template and the link reappears — making this + test the canary for the drift class. + """ + admin_password = os.getenv("NEXTCLOUD_PASSWORD") + if admin_password is None: + raise RuntimeError("NEXTCLOUD_PASSWORD must be set") + page = await browser.new_page() + try: + await login_to_nextcloud(page, "admin", admin_password) + await navigate_to_astrolabe_settings(page) + + # Sanity-check we're on the not-yet-authorized template. + enable_link = page.get_by_role("link", name="Enable Semantic Search") + if await enable_link.count() == 0: + pytest.skip( + "Astrolabe is already authorized for admin (oauth-required.php " + "not rendered). Reset by clearing the user's OAuth tokens " + "before re-running this test." + ) + + await _click_enable_semantic_search(page) + await _grant_oidc_consent(page) + + # OAuth callback returns to /apps/astrolabe/oauth/callback then the + # controller redirects to /settings/user/astrolabe. + await page.wait_for_url(re.compile(r"/settings/user/astrolabe"), timeout=30_000) + await page.wait_for_load_state("networkidle", timeout=15_000) + + # Regression assertion for the ALLOWED_MGMT_CLIENT drift bug: + # the page must have moved past oauth-required.php. If + # Astrolabe's management API call to /api/v1/users/{id}/session + # is rejected (401), the session lookup falls back to "no token", + # and the same oauth-required.php template re-renders with the + # link still present. + post_auth_count = await page.get_by_role( + "link", name="Enable Semantic Search" + ).count() + assert post_auth_count == 0, ( + "'Enable Semantic Search' link still visible after completing " + "OAuth flow — Astrolabe could not read the user's session from " + "the MCP server. Most likely cause: " + "`astrolabeMcpClientOAuth00000000000` missing from " + "`ALLOWED_MGMT_CLIENT` on `mcp-login-flow`." + ) + finally: + await page.close() diff --git a/tests/integration/test_deck_vector_search.py b/tests/integration/test_deck_vector_search.py index 3f6433bc..7524816f 100644 --- a/tests/integration/test_deck_vector_search.py +++ b/tests/integration/test_deck_vector_search.py @@ -207,10 +207,13 @@ async def test_deck_card_chunk_context(nc_client): # Fetch chunk context (simulates viz UI request) # The chunk spans the title, so start=0 and end=len(card_title) + # doc_id is str — keyword-indexed in Qdrant payload; the real + # callers (viz_routes.py from URL path; server/semantic.py from + # str(result.id)) all stringify before reaching this entry point. context = await get_chunk_with_context( nc_client=nc_client, user_id=nc_client.username, - doc_id=card.id, + doc_id=str(card.id), doc_type="deck_card", chunk_start=0, chunk_end=len(card_title), diff --git a/tests/server/login_flow/conftest.py b/tests/server/login_flow/conftest.py index 900f02fc..a7714a87 100644 --- a/tests/server/login_flow/conftest.py +++ b/tests/server/login_flow/conftest.py @@ -936,21 +936,26 @@ async def diana_login_flow_mcp_client( # Static OIDC client used by the management API integration tests. -# Matches the value `mcp-login-flow` and `mcp-multi-user-basic` allowlist -# (`ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient`) so tokens issued -# to it pass the management API allowlist check. -STATIC_MGMT_CLIENT_ID = "nextcloudMcpServerUIPublicClient" +# Matches the `mcp-login-flow` allowlist +# (`ALLOWED_MGMT_CLIENT=astrolabeMcpClientOAuth00000000000`) — i.e. the same +# id `app-hooks/before-starting/26-configure-astrolabe-oauth.sh` provisions +# in real deployments — so tokens issued to it pass the management API +# allowlist check on `mcp-login-flow` and exercise the production-shaped +# code path. +STATIC_MGMT_CLIENT_ID = "astrolabeMcpClientOAuth00000000000" @pytest.fixture(scope="session") async def login_flow_static_client_credentials(anyio_backend, oauth_callback_server): - """Pre-create the static OIDC client `nextcloudMcpServerUIPublicClient` + """Pre-create the static OIDC client `astrolabeMcpClientOAuth00000000000` via `occ oidc:create` with the test's OAuth callback URL. - The static client_id is allowlisted on `mcp-login-flow` (and - `mcp-multi-user-basic`) via `ALLOWED_MGMT_CLIENT`, so tokens it issues - pass the management API allowlist check. Uses a confidential JWT-token - client to match production Astrolabe configuration. + The static client_id matches the id provisioned by + `app-hooks/before-starting/26-configure-astrolabe-oauth.sh` in real + deployments, and is allowlisted on `mcp-login-flow` via + `ALLOWED_MGMT_CLIENT`, so tokens it issues pass the management API + allowlist check. Uses a confidential JWT-token client to match + production Astrolabe configuration. Yields: (client_id, client_secret, callback_url, token_endpoint, authorization_endpoint) """ diff --git a/tests/server/login_flow/test_login_flow_integration.py b/tests/server/login_flow/test_login_flow_integration.py index f2db50ba..262be92d 100644 --- a/tests/server/login_flow/test_login_flow_integration.py +++ b/tests/server/login_flow/test_login_flow_integration.py @@ -43,7 +43,13 @@ class TestLoginFlowAuthTools: data = json.loads(result.content[0].text) assert data["status"] == "provisioned" assert data["username"] is not None - assert data["scopes"] is not None + # ``scopes`` may legitimately be ``None`` — per ProvisionStatusResponse + # in models/auth.py, ``None`` is the documented sentinel for "all + # scopes granted" and is what the web provisioning path + # (``provision_routes.py``, used by Astrolabe's "Enable Semantic + # Search" flow) stores. So accept either a non-empty list or None; + # the field's *presence* in the payload is what we care about here. + assert data["scopes"] is None or len(data["scopes"]) > 0 logger.info(f"Provisioned as: {data['username']}, scopes: {data['scopes']}") async def test_provision_access_already_provisioned( diff --git a/tests/server/login_flow/test_management_api.py b/tests/server/login_flow/test_management_api.py index 397066dd..9830c145 100644 --- a/tests/server/login_flow/test_management_api.py +++ b/tests/server/login_flow/test_management_api.py @@ -1,9 +1,11 @@ """Integration tests for the management API on the login-flow MCP server. These tests drive a real OAuth flow against Nextcloud's `oidc` app using the -static `nextcloudMcpServerUIPublicClient` client (which is allowlisted on the -`mcp-login-flow` container via `ALLOWED_MGMT_CLIENT`), then hit the -management API endpoints with the resulting bearer token. +static `astrolabeMcpClientOAuth00000000000` client (which is allowlisted on +the `mcp-login-flow` container via `ALLOWED_MGMT_CLIENT` and matches the id +provisioned by `app-hooks/before-starting/26-configure-astrolabe-oauth.sh` +in real deployments), then hit the management API endpoints with the +resulting bearer token. Regression coverage for the bug where /api/v1/apps proxied to OCS v1 /cloud/apps and always 401'd. The handler now uses /ocs/v2.php/cloud/capabilities, diff --git a/tests/unit/search/test_search_result.py b/tests/unit/search/test_search_result.py index c9dbf0b1..95eece1e 100644 --- a/tests/unit/search/test_search_result.py +++ b/tests/unit/search/test_search_result.py @@ -1,15 +1,29 @@ """Unit tests for SearchResult validation.""" +from types import SimpleNamespace + import pytest -from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchResult, + build_search_result_from_point, +) + + +def _make_point(point_id, payload, score=0.5): + """Stand-in for qdrant_client.models.ScoredPoint. + + The helper only reads ``id``, ``payload``, and ``score`` — full Pydantic + validation isn't required for unit tests. + """ + return SimpleNamespace(id=point_id, payload=payload, score=score) @pytest.mark.unit def test_search_result_rrf_score_in_range(): """Test SearchResult accepts RRF scores in [0.0, 1.0] range.""" result = SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="Test excerpt", @@ -23,7 +37,7 @@ def test_search_result_rrf_score_in_range(): def test_search_result_rrf_score_at_lower_bound(): """Test SearchResult accepts RRF score at lower bound (0.0).""" result = SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="Test excerpt", @@ -37,7 +51,7 @@ def test_search_result_rrf_score_at_lower_bound(): def test_search_result_rrf_score_at_upper_bound(): """Test SearchResult accepts RRF score at upper bound (1.0).""" result = SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="Test excerpt", @@ -57,7 +71,7 @@ def test_search_result_dbsf_score_above_one(): """ # Typical DBSF score when both systems agree result = SearchResult( - id=1, + id="1", doc_type="note", title="Highly Relevant Note", excerpt="Contains keywords and is semantically similar", @@ -74,7 +88,7 @@ def test_search_result_dbsf_score_edge_case(): Maximum DBSF score with 2 systems: 1.0 (dense) + 1.0 (sparse) = 2.0 """ result = SearchResult( - id=1, + id="1", doc_type="note", title="Perfect Match", excerpt="Perfect semantic and keyword match", @@ -89,7 +103,7 @@ def test_search_result_negative_score_raises_error(): """Test SearchResult rejects negative scores.""" with pytest.raises(ValueError) as exc_info: SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="Test excerpt", @@ -104,7 +118,7 @@ def test_search_result_negative_score_raises_error(): def test_search_result_with_metadata(): """Test SearchResult with optional metadata field.""" result = SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="Test excerpt", @@ -122,7 +136,7 @@ def test_search_result_with_metadata(): def test_search_result_with_chunk_offsets(): """Test SearchResult with chunk offset information.""" result = SearchResult( - id=1, + id="1", doc_type="note", title="Test Note", excerpt="matching chunk text", @@ -133,3 +147,166 @@ def test_search_result_with_chunk_offsets(): assert result.chunk_start_offset == 100 assert result.chunk_end_offset == 500 + + +# --------------------------------------------------------------------------- +# build_search_result_from_point +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_build_search_result_from_point_returns_none_when_payload_missing(): + """Helper signals the caller to skip the point by returning None.""" + point = _make_point(point_id="p1", payload=None) + + assert build_search_result_from_point(point) is None + + +@pytest.mark.unit +def test_build_search_result_from_point_returns_none_when_doc_id_missing(): + """A payload without a doc_id key is skipped instead of raising KeyError.""" + point = _make_point(point_id="p-bad", payload={"doc_type": "note"}) + + assert build_search_result_from_point(point) is None + + +@pytest.mark.unit +def test_build_search_result_from_point_note_payload(): + """Note-type payload populates the SearchResult fields without metadata extras.""" + point = _make_point( + point_id="p-1", + payload={ + "doc_id": "42", + "doc_type": "note", + "title": "Hello", + "excerpt": "world", + "chunk_start_offset": 0, + "chunk_end_offset": 100, + "chunk_index": 0, + "total_chunks": 2, + }, + score=0.91, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.id == "42" + assert sr.doc_type == "note" + assert sr.title == "Hello" + assert sr.excerpt == "world" + assert sr.score == pytest.approx(0.91) + assert sr.chunk_start_offset == 0 + assert sr.chunk_end_offset == 100 + assert sr.chunk_index == 0 + assert sr.total_chunks == 2 + assert sr.point_id == "p-1" + assert sr.metadata == {"chunk_index": 0, "total_chunks": 2} + + +@pytest.mark.unit +def test_build_search_result_from_point_coerces_int_doc_id_to_str(): + """Legacy int doc_id payloads are stringified defensively.""" + point = _make_point( + point_id=1, + payload={"doc_id": 7, "doc_type": "note"}, + score=0.5, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.id == "7" + + +@pytest.mark.unit +def test_build_search_result_from_point_file_metadata_includes_path(): + """File-type payloads with a file_path attach it under metadata['path'].""" + point = _make_point( + point_id="p-2", + payload={ + "doc_id": "100", + "doc_type": "file", + "file_path": "/Documents/report.pdf", + "page_number": 3, + "page_count": 12, + }, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.doc_type == "file" + assert sr.metadata["path"] == "/Documents/report.pdf" + assert sr.page_number == 3 + assert sr.page_count == 12 + + +@pytest.mark.unit +def test_build_search_result_from_point_deck_card_metadata(): + """Deck-card payloads carry board_id/stack_id forward for verify-on-read.""" + point = _make_point( + point_id="p-3", + payload={ + "doc_id": "55", + "doc_type": "deck_card", + "board_id": 7, + "stack_id": 12, + "title": "Card", + }, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.metadata["board_id"] == 7 + assert sr.metadata["stack_id"] == 12 + + +@pytest.mark.unit +def test_build_search_result_from_point_merges_metadata_extras(): + """metadata_extras augment the helper's computed metadata dict. + + Common fields (chunk_index, total_chunks) win over caller-supplied + extras to keep them tied to the actual point. + """ + point = _make_point( + point_id="p-4", + payload={ + "doc_id": "1", + "doc_type": "note", + "chunk_index": 3, + "total_chunks": 9, + }, + ) + + sr = build_search_result_from_point( + point, + metadata_extras={ + "search_method": "bm25_hybrid_rrf", + # Caller tries to override a common field — should be ignored. + "chunk_index": "should-be-overwritten", + }, + ) + + assert sr is not None + assert sr.metadata["search_method"] == "bm25_hybrid_rrf" + assert sr.metadata["chunk_index"] == 3 + assert sr.metadata["total_chunks"] == 9 + + +@pytest.mark.unit +def test_build_search_result_from_point_defaults_when_optional_fields_missing(): + """Missing optional payload keys fall back to documented defaults.""" + point = _make_point(point_id="p-5", payload={"doc_id": "1"}) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.doc_type == "note" # default doc_type + assert sr.title == "Untitled" + assert sr.excerpt == "" + assert sr.chunk_index == 0 + assert sr.total_chunks == 1 + assert sr.chunk_start_offset is None + assert sr.chunk_end_offset is None diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index da5b8065..3cee6981 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -35,8 +35,11 @@ def _make_result( score: float = 0.9, metadata: dict | None = None, ) -> SearchResult: + # Mirror the producer-side stringification (scanner writes str(note["id"]) + # etc. into Qdrant payloads). Tests pass int literals for readability; + # the SearchResult contract is ``id: str``. return SearchResult( - id=doc_id, + id=str(doc_id), doc_type=doc_type, title=f"{doc_type}_{doc_id}", excerpt="...", @@ -84,7 +87,7 @@ async def test_verify_notes_200_keeps_all(mocker): client, [_make_result(1), _make_result(2), _make_result(3)], _sem() ) - assert result == {1, 2, 3} + assert result == {"1", "2", "3"} assert notes_client.get_note.await_count == 3 @@ -122,7 +125,7 @@ async def test_verify_notes_transient_5xx_keeps(mocker): result = await _verify_notes(client, [_make_result(42)], _sem()) - assert result == {42} + assert result == {"42"} @pytest.mark.unit @@ -140,7 +143,7 @@ async def test_verify_notes_429_keeps_as_transient(mocker): result = await _verify_notes(client, [_make_result(42)], _sem()) - assert result == {42} + assert result == {"42"} @pytest.mark.unit @@ -152,7 +155,7 @@ async def test_verify_notes_unexpected_exception_keeps(mocker): result = await _verify_notes(client, [_make_result(7)], _sem()) - assert result == {7} + assert result == {"7"} @pytest.mark.unit @@ -193,7 +196,7 @@ async def test_verify_notes_mixed_outcomes(mocker): client, [_make_result(1), _make_result(2), _make_result(3)], _sem() ) - assert result == {1, 3} + assert result == {"1", "3"} @pytest.mark.unit @@ -242,7 +245,7 @@ async def test_verify_news_items_intersects_with_fetched_set(mocker): _sem(), ) - assert result == {10, 20} + assert result == {"10", "20"} assert news_client.get_items.await_count == 1 @@ -304,7 +307,7 @@ async def test_verify_news_items_transient_keeps_all(mocker): _sem(), ) - assert result == {1, 2, 3} + assert result == {"1", "2", "3"} @pytest.mark.unit @@ -325,7 +328,7 @@ async def test_verify_news_items_429_keeps_as_transient(mocker): _sem(), ) - assert result == {1, 2, 3} + assert result == {"1", "2", "3"} @pytest.mark.unit @@ -350,7 +353,7 @@ async def test_verify_news_items_unexpected_exception_keeps_all(mocker): _sem(), ) - assert result == {1, 2} + assert result == {"1", "2"} @pytest.mark.unit @@ -377,7 +380,7 @@ async def test_verify_news_items_non_numeric_id_keeps_only_bad_item(mocker): ) # 10 and 20 are verified present; "abc" is unverifiable so kept fail-open. - assert result == {10, 20, "abc"} + assert result == {"10", "20", "abc"} @pytest.mark.unit @@ -402,7 +405,7 @@ async def test_verify_news_items_drops_missing_when_other_id_is_non_numeric( ) # 10 verified present, 20 verified missing (dropped), "abc" unverifiable. - assert result == {10, "abc"} + assert result == {"10", "abc"} @pytest.mark.unit @@ -426,7 +429,7 @@ async def test_verify_news_items_malformed_api_response_keeps_all(mocker): ) # Batch fail-open: API broken, every requested id preserved. - assert result == {10, 20} + assert result == {"10", "20"} # --------------------------------------------------------------------------- @@ -448,7 +451,7 @@ async def test_verify_files_uses_path_from_metadata(mocker): _sem(), ) - assert result == {100} + assert result == {"100"} webdav_client.get_file_info.assert_awaited_once_with("Documents/foo.txt") @@ -487,7 +490,7 @@ async def test_verify_files_malformed_propfind_keeps_result(mocker): _sem(), ) - assert result == {123}, "ambiguous None must keep result, not evict" + assert result == {"123"}, "ambiguous None must keep result, not evict" @pytest.mark.unit @@ -517,14 +520,14 @@ async def test_verify_files_missing_path_metadata_keeps_unverified(mocker): # No metadata at all result = await _verify_files(client, [_make_result(555, doc_type="file")], _sem()) - assert result == {555} + assert result == {"555"} webdav_client.get_file_info.assert_not_awaited() # Metadata present but no "path" key result = await _verify_files( client, [_make_result(556, doc_type="file", metadata={})], _sem() ) - assert result == {556} + assert result == {"556"} webdav_client.get_file_info.assert_not_awaited() @@ -541,7 +544,7 @@ async def test_verify_files_transient_5xx_keeps(mocker): _sem(), ) - assert result == {7} + assert result == {"7"} @pytest.mark.unit @@ -558,7 +561,7 @@ async def test_verify_files_429_keeps_as_transient(mocker): _sem(), ) - assert result == {7} + assert result == {"7"} @pytest.mark.unit @@ -580,7 +583,7 @@ async def test_verify_files_unexpected_exception_keeps(mocker): _sem(), ) - assert result == {8} + assert result == {"8"} # --------------------------------------------------------------------------- @@ -606,7 +609,7 @@ async def test_verify_deck_cards_uses_metadata_fast_path(mocker): _sem(), ) - assert result == {42} + assert result == {"42"} deck_client.get_card.assert_awaited_once_with(board_id=1, stack_id=2, card_id=42) @@ -676,7 +679,7 @@ async def test_verify_deck_cards_transient_5xx_keeps(mocker): _sem(), ) - assert result == {42} + assert result == {"42"} @pytest.mark.unit @@ -699,7 +702,7 @@ async def test_verify_deck_cards_429_keeps_as_transient(mocker): _sem(), ) - assert result == {42} + assert result == {"42"} @pytest.mark.unit @@ -722,7 +725,7 @@ async def test_verify_deck_cards_unexpected_exception_keeps(mocker): _sem(), ) - assert result == {42} + assert result == {"42"} @pytest.mark.unit @@ -749,7 +752,7 @@ async def test_verify_deck_cards_non_numeric_metadata_keeps(mocker): ], _sem(), ) - assert result == {42} + assert result == {"42"} # Non-numeric stack_id result = await _verify_deck_cards( @@ -763,7 +766,7 @@ async def test_verify_deck_cards_non_numeric_metadata_keeps(mocker): ], _sem(), ) - assert result == {43} + assert result == {"43"} # Non-numeric card_id (doc_id itself) result = await _verify_deck_cards( @@ -794,7 +797,7 @@ async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): result = await _verify_deck_cards( client, [_make_result(42, doc_type="deck_card")], _sem() ) - assert result == {42} + assert result == {"42"} # Only board_id (stack_id missing) result = await _verify_deck_cards( @@ -802,7 +805,7 @@ async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): [_make_result(43, doc_type="deck_card", metadata={"board_id": 1})], _sem(), ) - assert result == {43} + assert result == {"43"} # Only stack_id (board_id missing) result = await _verify_deck_cards( @@ -810,7 +813,7 @@ async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): [_make_result(44, doc_type="deck_card", metadata={"stack_id": 2})], _sem(), ) - assert result == {44} + assert result == {"44"} deck_client.get_card.assert_not_awaited() @@ -829,7 +832,7 @@ async def test_verify_search_results_empty_input_passthrough(): @pytest.mark.unit async def test_verify_search_results_dedupes_chunks_per_document(mocker): """Two chunks of the same note → ONE call to the underlying verifier.""" - spy = mocker.AsyncMock(return_value={1}) + spy = mocker.AsyncMock(return_value={"1"}) mocker.patch.dict(verification._VERIFIERS, {"note": spy}, clear=False) mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) @@ -848,7 +851,7 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker): # Verifier received exactly one SearchResult (the deduplicated representative) args, _kwargs = spy.call_args assert len(args[1]) == 1 - assert args[1][0].id == 1 + assert args[1][0].id == "1" # And a semaphore as the third arg assert isinstance(args[2], anyio.Semaphore) @@ -1035,7 +1038,7 @@ async def test_verify_search_results_verifier_blowup_keeps_all(mocker): kept, dropped_count = await verify_search_results(client, results) - assert [r.id for r in kept] == [1, 2] + assert [r.id for r in kept] == ["1", "2"] assert dropped_count == 0 # fail-open: nothing dropped spy_evict.assert_not_awaited() @@ -1043,7 +1046,7 @@ async def test_verify_search_results_verifier_blowup_keeps_all(mocker): @pytest.mark.unit async def test_verify_search_results_preserves_order(mocker): """Order of original results must be preserved after filtering.""" - note_verifier = mocker.AsyncMock(return_value={1, 3}) + note_verifier = mocker.AsyncMock(return_value={"1", "3"}) mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) @@ -1056,7 +1059,7 @@ async def test_verify_search_results_preserves_order(mocker): kept, dropped_count = await verify_search_results(client, results) - assert [r.id for r in kept] == [1, 3] + assert [r.id for r in kept] == ["1", "3"] assert dropped_count == 1 @@ -1083,8 +1086,8 @@ async def test_verify_search_results_eviction_failure_does_not_propagate(mocker) @pytest.mark.unit async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker): """Mixed doc_types must be routed to their respective verifiers.""" - note_verifier = mocker.AsyncMock(return_value={1}) - file_verifier = mocker.AsyncMock(return_value={500}) + note_verifier = mocker.AsyncMock(return_value={"1"}) + file_verifier = mocker.AsyncMock(return_value={"500"}) mocker.patch.dict( verification._VERIFIERS, {"note": note_verifier, "file": file_verifier}, @@ -1101,7 +1104,7 @@ async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker kept, dropped_count = await verify_search_results(client, results) - assert {(r.id, r.doc_type) for r in kept} == {(1, "note"), (500, "file")} + assert {(r.id, r.doc_type) for r in kept} == {("1", "note"), ("500", "file")} assert dropped_count == 1 note_verifier.assert_awaited_once() file_verifier.assert_awaited_once() diff --git a/tests/unit/test_chunk_bbox_helper.py b/tests/unit/test_chunk_bbox_helper.py index af542353..f7abed91 100644 --- a/tests/unit/test_chunk_bbox_helper.py +++ b/tests/unit/test_chunk_bbox_helper.py @@ -58,7 +58,7 @@ class TestIndexedPath: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -84,7 +84,7 @@ class TestOffsetFallbackPath: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="bob", - doc_id=99, + doc_id="99", chunk_index=None, chunk_start=500, chunk_end=600, @@ -105,7 +105,7 @@ class TestOffsetFallbackPath: with ctx, caplog.at_level("WARNING"): result = await get_chunk_bbox_and_page_from_qdrant( user_id="bob", - doc_id=99, + doc_id="99", chunk_index=None, chunk_start=0, chunk_end=100, @@ -124,7 +124,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=1, + doc_id="1", chunk_index=0, chunk_start=0, chunk_end=10, @@ -143,7 +143,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -157,7 +157,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -171,7 +171,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -188,7 +188,7 @@ class TestPayloadShape: with ctx: result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, @@ -205,7 +205,7 @@ class TestExceptionHandling: with ctx, caplog.at_level("WARNING"): result = await get_chunk_bbox_and_page_from_qdrant( user_id="alice", - doc_id=42, + doc_id="42", chunk_index=3, chunk_start=0, chunk_end=100, diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py index d46dc69d..c8e6dedd 100644 --- a/tests/unit/test_chunk_context_offset_gate.py +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -28,8 +28,12 @@ def mock_nc_client() -> 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). + """When chunk_index is provided, the offset fallback must be skipped for + every doc_type. The original gate was file-only (PR #767 review, 🟡 + spurious Qdrant error log); PR #773 round 11 broadened it to all + doc_types because chunk_start/end_offset aren't in + ``_PAYLOAD_INDEX_FIELDS`` and 400 in Qdrant Cloud strict mode for + notes / deck cards / news items the same way they do for files. """ async def test_file_with_chunk_index_skips_offset_fallback_on_miss( @@ -52,7 +56,7 @@ class TestOffsetFallbackGate: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=12345, + doc_id="12345", doc_type="file", chunk_start=0, chunk_end=100, @@ -64,11 +68,15 @@ class TestOffsetFallbackGate: mock_indexed.assert_awaited_once() mock_offset.assert_not_awaited() - async def test_note_with_chunk_index_still_uses_offset_fallback( + async def test_note_with_chunk_index_skips_offset_fallback_on_miss( self, mock_nc_client ): - """Notes/deck cards keep the offset fallback (cheap, useful for legacy - data): the gate is file-specific. + """Notes (and other non-file doc_types) trust the indexed + chunk_index lookup as canonical too. A miss means absent — don't + hit the unindexed offset path that 400s in Qdrant Cloud strict + mode. Legacy data without chunk_index still uses the offset path + via the chunk_index=None branch (see + test_file_without_chunk_index_uses_offset_fallback). """ with ( patch.object( @@ -81,7 +89,7 @@ class TestOffsetFallbackGate: context_module, "_get_chunk_from_qdrant", new_callable=AsyncMock, - return_value=None, + return_value="should-not-be-returned", ) as mock_offset, patch.object( context_module, @@ -93,7 +101,7 @@ class TestOffsetFallbackGate: await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=0, chunk_end=10, @@ -102,7 +110,7 @@ class TestOffsetFallbackGate: ) mock_indexed.assert_awaited_once() - mock_offset.assert_awaited_once() + mock_offset.assert_not_awaited() 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 — @@ -125,7 +133,7 @@ class TestOffsetFallbackGate: await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=12345, + doc_id="12345", doc_type="file", chunk_start=0, chunk_end=100, @@ -167,7 +175,7 @@ class TestNullableChunkIndexPropagation: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=100, chunk_end=200, @@ -214,7 +222,7 @@ class TestNullableChunkIndexPropagation: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=0, chunk_end=10, @@ -256,7 +264,7 @@ class TestNullableChunkIndexPropagation: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=100, chunk_end=200, @@ -299,7 +307,7 @@ class TestAdjacentChunkBoundary: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=0, chunk_end=10, @@ -340,7 +348,7 @@ class TestAdjacentChunkBoundary: result = await get_chunk_with_context( nc_client=mock_nc_client, user_id="alice", - doc_id=42, + doc_id="42", doc_type="note", chunk_start=0, chunk_end=10, diff --git a/tests/unit/utils/__init__.py b/tests/unit/utils/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/utils/test_validation.py b/tests/unit/utils/test_validation.py new file mode 100644 index 00000000..8e1f38ed --- /dev/null +++ b/tests/unit/utils/test_validation.py @@ -0,0 +1,54 @@ +"""Unit tests for shared boundary validators.""" + +import pytest + +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id + + +@pytest.mark.unit +@pytest.mark.parametrize( + "value", + [ + "1", + "42", + "1234567890", + "9999999999999999999", + ], +) +def test_accepts_positive_ascii_integers(value): + """Any positive ASCII integer (no leading zero) is a valid doc_id.""" + assert is_valid_nextcloud_doc_id(value) is True + + +@pytest.mark.unit +@pytest.mark.parametrize( + "value,reason", + [ + ("", "empty string"), + ("0", "MySQL AUTO_INCREMENT starts at 1"), + ("01", "leading zero"), + ("00", "leading zeros"), + ("-1", "negative"), + ("+1", "explicit sign"), + ("1.0", "float-like"), + (" 1", "leading whitespace"), + ("1 ", "trailing whitespace"), + ("1\n", "trailing newline"), + ("abc", "alphabetic"), + ("1a", "trailing letter"), + ("a1", "leading letter"), + # Unicode digit classes that pass str.isdigit() but are not ASCII. + # `²` (U+00B2) is a superscript and would slip past the old guard. + ("²", "Unicode superscript-2"), + # `٢` (U+0662) Arabic-Indic digit two — passes both isdigit() and + # isdecimal(), so only an explicit ASCII regex catches it. + ("٢", "Arabic-Indic digit two"), + # `१` (U+0967) Devanagari digit one — same story. + ("१", "Devanagari digit one"), + # Mixed ASCII + Unicode digits. + ("1٢", "mixed ASCII + Arabic-Indic"), + ], +) +def test_rejects_invalid_doc_ids(value, reason): + """Reject empty/zero/leading-zero/non-ASCII/non-digit inputs.""" + assert is_valid_nextcloud_doc_id(value) is False, f"should reject: {reason}" diff --git a/tests/unit/vector/__init__.py b/tests/unit/vector/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py new file mode 100644 index 00000000..cb0f5236 --- /dev/null +++ b/tests/unit/vector/test_qdrant_client.py @@ -0,0 +1,990 @@ +"""Unit tests for Qdrant payload-index helpers and doc_id backfill. + +These cover the startup-time migrations added to ``vector/qdrant_client.py`` +after production HTTP 400 errors revealed that: + +1. The collection had no payload index for ``doc_id``, so any + ``FieldCondition(key="doc_id", ...)`` filter failed at the Qdrant layer. +2. Producers wrote a mix of ``int`` and ``str`` values for ``doc_id``, so a + single keyword index could not have covered both kinds even if it had + existed. + +The fix has three coordinated parts; this module covers the two helpers that +run at startup. Producer-side normalization is exercised by the existing +scanner tests. +""" + +from types import SimpleNamespace +from unittest.mock import call + +import anyio +import httpx +import pytest +from qdrant_client.http.exceptions import UnexpectedResponse +from qdrant_client.models import PayloadSchemaType + +from nextcloud_mcp_server.vector import qdrant_client as qdrant_module +from nextcloud_mcp_server.vector.qdrant_client import ( + _DOC_ID_BACKFILL_SENTINEL_ID, + _PAYLOAD_INDEX_FIELDS, + _backfill_doc_id_to_string, + _ensure_payload_indexes, + _group_int_doc_ids, + get_qdrant_client, +) + + +def _empty_collection_info() -> SimpleNamespace: + """Stand-in for a CollectionInfo with no payload indexes yet. + + Tests for _ensure_payload_indexes only read ``payload_schema`` + off the result. None / empty dict both signal "no indexes" — use {} + here to match the production-code default. + """ + return SimpleNamespace(payload_schema={}) + + +def _backfill_dimension() -> int: + """Vector dimension for sentinel writes in backfill tests. + + Any positive int is fine — the sentinel point is never read by the + test bodies, only the upsert call site is asserted. + """ + return 4 + + +def _make_unexpected(status_code: int, body: bytes) -> UnexpectedResponse: + """Build a real UnexpectedResponse for raise_for_status-style branches.""" + return UnexpectedResponse( + status_code=status_code, + reason_phrase="Bad Request", + content=body, + headers=httpx.Headers(), + ) + + +def _record(point_id: int | str, doc_id: int | str | None) -> SimpleNamespace: + """Stand-in for qdrant_client.http.models.Record. + + Tests don't need full Pydantic validation — only ``id`` and ``payload`` + are read by the helpers under test. + """ + payload: dict | None = {"doc_id": doc_id} if doc_id is not None else None + return SimpleNamespace(id=point_id, payload=payload) + + +# --------------------------------------------------------------------------- +# _ensure_payload_indexes +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_ensure_payload_indexes_creates_each_field(mocker): + """Happy path: every field in _PAYLOAD_INDEX_FIELDS gets its declared schema. + + The dict-of-(field, schema_type) registry pairs string fields with + KEYWORD and the boolean ``is_placeholder`` with BOOL — both are + required because Qdrant's strict-mode index-required filtering + enforces a payload index for any ``FieldCondition`` regardless of + value type. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = _empty_collection_info() + + await _ensure_payload_indexes(client, "test-collection") + + assert client.create_payload_index.await_count == len(_PAYLOAD_INDEX_FIELDS) + expected_calls = [ + call( + collection_name="test-collection", + field_name=field, + field_schema=schema_type, + wait=True, + ) + for field, schema_type in _PAYLOAD_INDEX_FIELDS.items() + ] + client.create_payload_index.assert_has_awaits(expected_calls, any_order=False) + + +@pytest.mark.unit +async def test_ensure_payload_indexes_includes_is_placeholder_as_bool(mocker): + """is_placeholder must be created with BOOL schema, not KEYWORD. + + ``get_placeholder_filter`` and ``delete_placeholder_point`` filter on + ``is_placeholder`` (a bool); creating it with KEYWORD would still + fail strict-mode index-required filtering on Qdrant Cloud because + the index type wouldn't match the value type. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = _empty_collection_info() + + await _ensure_payload_indexes(client, "test-collection") + + bool_calls = [ + c + for c in client.create_payload_index.await_args_list + if c.kwargs.get("field_name") == "is_placeholder" + ] + assert len(bool_calls) == 1, "is_placeholder must be created exactly once" + assert bool_calls[0].kwargs["field_schema"] is PayloadSchemaType.BOOL + + +@pytest.mark.unit +async def test_ensure_payload_indexes_skips_fields_already_indexed(mocker, caplog): + """Routine restart path: existing payload indexes are silently skipped. + + Without the pre-fetch, every restart logs `Created payload + index on ''` for every field — noise that hides genuinely + interesting first-time-creation lines. With the pre-fetch, no log + fires and no Qdrant write round-trip happens for already-indexed + fields. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = SimpleNamespace( + payload_schema={"doc_id": object()} + ) + + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + # Only the missing fields are created — every entry in the registry + # other than the one already in the schema. + expected_missing = set(_PAYLOAD_INDEX_FIELDS) - {"doc_id"} + assert client.create_payload_index.await_count == len(expected_missing) + created_fields = { + c.kwargs["field_name"] for c in client.create_payload_index.await_args_list + } + assert created_fields == expected_missing + # No INFO log fires for the already-indexed field. + info_messages = [r.getMessage() for r in caplog.records if r.levelname == "INFO"] + assert not any("doc_id" in m for m in info_messages), info_messages + + +@pytest.mark.unit +async def test_ensure_payload_indexes_warns_on_wrong_schema_type(mocker, caplog): + """Pre-existing index with wrong schema type surfaces as a WARNING. + + The bug this PR fixes: a collection migrated from the int-doc_id era + can have ``doc_id`` indexed as INTEGER, which silently survives the + "field already in schema → skip" branch and lets ``MatchValue(value="123")`` + keep failing with HTTP 400 on Qdrant Cloud strict mode. Confirm the + type-aware check fires a WARNING, marks the field as failed (so the + consolidated end-of-function summary picks it up), and does NOT + attempt to recreate the index — operator intervention is the only + safe path. + """ + client = mocker.AsyncMock() + # PayloadIndexInfo-like stand-in: only ``data_type`` is read. + wrong = SimpleNamespace(data_type=PayloadSchemaType.INTEGER) + client.get_collection.return_value = SimpleNamespace( + payload_schema={"doc_id": wrong} + ) + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + # No create attempt for the mismatched field. + created_fields = { + c.kwargs["field_name"] for c in client.create_payload_index.await_args_list + } + assert "doc_id" not in created_fields + + warning_messages = [ + r.getMessage() for r in caplog.records if r.levelname == "WARNING" + ] + # Per-field warning describes both observed and expected types. + assert any( + "doc_id" in m and "INTEGER" in m and "KEYWORD" in m for m in warning_messages + ), warning_messages + # Consolidated summary at end of function includes the field too. + assert any( + "Payload index creation incomplete" in m and "doc_id" in m + for m in warning_messages + ), warning_messages + + +@pytest.mark.unit +async def test_ensure_payload_indexes_logs_400_as_warning(mocker, caplog): + """A 400 from create_payload_index logs WARNING *and* fires the summary. + + Real Qdrant returns 200 when the index already exists with a matching + schema, so 400s indicate a genuine problem (e.g., schema conflict on a + pre-existing index). The loop continues past the failure so the + remaining fields still get indexed, *and* the field accumulates into + ``failed_fields`` so the consolidated `Payload index creation + incomplete` summary fires — without this, tenants whose + ``payload_schema`` is hidden from their JWT (Qdrant Cloud + collection-scoped tokens) would only see the per-field warning and + miss the operator-level summary that `wrong_schema_type` paths emit. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = _empty_collection_info() + # First field fails with 400; remaining fields succeed. One side_effect + # entry per item in _PAYLOAD_INDEX_FIELDS so the iteration is exhaustive. + client.create_payload_index.side_effect = [ + _make_unexpected( + 400, + b'{"status":{"error":"field \\"doc_id\\" indexed with different schema"}}', + ), + *([None] * (len(_PAYLOAD_INDEX_FIELDS) - 1)), + ] + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + # Loop continued past the failing field; every field was attempted. + assert client.create_payload_index.await_count == len(_PAYLOAD_INDEX_FIELDS) + warning_messages = [ + r.getMessage() for r in caplog.records if r.levelname == "WARNING" + ] + # Per-field warning describes the schema conflict. + assert any( + m.startswith("Schema conflict on payload index") and "different schema" in m + for m in warning_messages + ), warning_messages + # Consolidated summary names the failed field too — see the docstring + # for why this matters in tenant-scoped Qdrant Cloud setups. + first_field = next(iter(_PAYLOAD_INDEX_FIELDS)) + assert any( + "Payload index creation incomplete" in m and first_field in m + for m in warning_messages + ), warning_messages + + +@pytest.mark.unit +async def test_ensure_payload_indexes_logs_non_400_as_error(mocker, caplog): + """A non-400 status from create_payload_index escalates to ERROR. + + A 5xx response (e.g., Qdrant temporarily unavailable) should not be + silently downgraded to a warning the way a 400 schema-conflict is. + The loop still continues so the remaining fields get attempted. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = _empty_collection_info() + # First field fails with 500; remaining fields succeed. + client.create_payload_index.side_effect = [ + _make_unexpected(500, b'{"status":{"error":"internal server error"}}'), + *([None] * (len(_PAYLOAD_INDEX_FIELDS) - 1)), + ] + + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + assert client.create_payload_index.await_count == len(_PAYLOAD_INDEX_FIELDS) + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + msg = errors[0].getMessage() + assert "500" in msg + assert "internal server error" in msg + + +@pytest.mark.unit +async def test_ensure_payload_indexes_continues_past_raw_network_error(mocker, caplog): + """A raw network error (e.g. ConnectError, TimeoutError) must not skip the rest. + + UnexpectedResponse covers HTTP-shaped failures, but transport-level + failures (httpx.ConnectError, asyncio.TimeoutError) reach the loop as + bare Exceptions. Without a broad catch, the first network blip + propagates, leaves _qdrant_client assigned, and silently skips every + remaining field. The fix is per-field containment matching the 5xx + behaviour: log at ERROR with exc_info, append to failed_fields, and + continue. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = _empty_collection_info() + # First field hits a connection failure; remaining fields succeed. + client.create_payload_index.side_effect = [ + ConnectionError("Connection refused"), + *([None] * (len(_PAYLOAD_INDEX_FIELDS) - 1)), + ] + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + # Loop continued past the failing field; every field was attempted. + assert client.create_payload_index.await_count == len(_PAYLOAD_INDEX_FIELDS) + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + assert "Network error creating payload index" in errors[0].getMessage() + # exc_info is preserved so operators can see the underlying cause. + assert errors[0].exc_info is not None + assert errors[0].exc_info[0] is ConnectionError + # The partial-failure summary surfaces the field as missing. + summary_warnings = [ + r + for r in caplog.records + if r.levelname == "WARNING" + and "Payload index creation incomplete" in r.getMessage() + ] + assert len(summary_warnings) == 1 + # The first field in _PAYLOAD_INDEX_FIELDS is the one that raised. + failing_field = next(iter(_PAYLOAD_INDEX_FIELDS)) + assert failing_field in summary_warnings[0].getMessage() + + +@pytest.mark.unit +async def test_ensure_payload_indexes_logs_and_returns_when_get_collection_raises( + mocker, caplog +): + """A get_collection failure is logged and swallowed; no indexes are attempted. + + Mirrors the broad swallow in `_backfill_doc_id_to_string`. The + qdrant_client singleton is already assigned by the time this + function runs, so re-raising would leave the process holding a + usable client with the migration silently skipped on every + subsequent call. Catching, logging, and returning preserves the + retry-on-next-restart behavior. + """ + client = mocker.AsyncMock() + + async def _get_collection_raises(*args, **kwargs): + # See _scroll_raises in the backfill section for why this is async. + await anyio.lowlevel.checkpoint() + raise RuntimeError("connection refused") + + client.get_collection.side_effect = _get_collection_raises + + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + # No index creation was attempted — the function returned early. + client.create_payload_index.assert_not_awaited() + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + msg = errors[0].getMessage() + assert "Failed to fetch collection info for 'test-collection'" in msg + assert "Will retry on next restart" in msg + assert errors[0].exc_info is not None + assert errors[0].exc_info[0] is RuntimeError + + +# --------------------------------------------------------------------------- +# _backfill_doc_id_to_string +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_backfill_clean_collection_makes_no_writes(mocker, caplog): + """A collection with only str doc_ids triggers zero set_payload calls. + + Verifies the no-write path: scroll runs, no payloads need rewriting, + and a sentinel is written so subsequent restarts can short-circuit. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] # No sentinel — backfill must run + client.scroll.return_value = ( + [_record(1, "abc"), _record(2, "def")], + None, + ) + + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + client.set_payload.assert_not_awaited() + completion_logs = [ + r.getMessage() for r in caplog.records if "backfill complete" in r.getMessage() + ] + assert completion_logs, "expected an INFO log line for backfill completion" + # rewritten=0 → human-readable wording instead of the misleading + # "rewrote 0/N from int to str" formula. + assert "2 points scanned" in completion_logs[0] + assert "none required rewriting" in completion_logs[0] + + +@pytest.mark.unit +async def test_backfill_skips_when_sentinel_present(mocker, caplog): + """If the sentinel exists, retrieve() returns it and the scroll is skipped. + + This is the routine-restart fast path: the migration already ran on a + previous start, so we avoid the O(N) scroll entirely. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [SimpleNamespace(id=_DOC_ID_BACKFILL_SENTINEL_ID)] + + with caplog.at_level("DEBUG", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + client.scroll.assert_not_awaited() + client.set_payload.assert_not_awaited() + client.upsert.assert_not_awaited() + debug_msgs = [r.getMessage() for r in caplog.records if r.levelname == "DEBUG"] + assert any("sentinel" in m and "skipping" in m for m in debug_msgs), debug_msgs + + +@pytest.mark.unit +async def test_backfill_writes_sentinel_after_successful_scroll(mocker): + """Successful backfill writes a sentinel point so future restarts skip.""" + client = mocker.AsyncMock() + client.retrieve.return_value = [] # No sentinel — backfill must run + client.scroll.return_value = ([_record(1, "abc")], None) + + await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension()) + + # Single upsert with the sentinel UUID + migration marker payload. + assert client.upsert.await_count == 1 + upsert_kwargs = client.upsert.await_args.kwargs + assert upsert_kwargs["collection_name"] == "test-collection" + assert upsert_kwargs["wait"] is True + points = upsert_kwargs["points"] + assert len(points) == 1 + assert points[0].id == _DOC_ID_BACKFILL_SENTINEL_ID + assert points[0].payload == {"_migration_marker": "doc_id_v1"} + + +@pytest.mark.unit +async def test_backfill_rewrites_int_doc_ids_to_str(mocker): + """Mixed int/str payload across two scroll pages: only ints get rewritten. + + Includes ``doc_id=0`` to guard against a future "early-exit on + falsy" refactor — the helper must rewrite zero alongside other + ints, not skip it. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] + # Two scroll calls: batch 1 mixes int (incl. 0) + str and reports a + # next_offset; batch 2 mixes int + str with next_offset=None to terminate. + client.scroll.side_effect = [ + ( + [_record(1, 100), _record(2, "abc"), _record(5, 0)], + "next-offset-123", + ), + ([_record(3, 200), _record(4, "def")], None), + ] + + await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension()) + + # One set_payload per *unique* int value across all batches: 100, 0, 200. + assert client.set_payload.await_count == 3 + client.set_payload.assert_any_await( + collection_name="test-collection", + payload={"doc_id": "100"}, + points=[1], + wait=True, + ) + client.set_payload.assert_any_await( + collection_name="test-collection", + payload={"doc_id": "0"}, + points=[5], + wait=True, + ) + client.set_payload.assert_any_await( + collection_name="test-collection", + payload={"doc_id": "200"}, + points=[3], + wait=True, + ) + + +@pytest.mark.unit +async def test_backfill_batches_points_with_same_doc_id(mocker): + """Multiple points sharing the same int doc_id collapse to one set_payload. + + A single document indexed as multiple chunks all share its doc_id; the + backfill should issue one set_payload call covering the chunk batch. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] + client.scroll.side_effect = [ + ( + [ + _record(10, 42), + _record(11, 42), + _record(12, 42), + _record(13, "already-str"), + ], + None, + ), + ] + + await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension()) + + # All three int-payload points share doc_id=42, so a single call covers them. + assert client.set_payload.await_count == 1 + client.set_payload.assert_awaited_with( + collection_name="test-collection", + payload={"doc_id": "42"}, + points=[10, 11, 12], + wait=True, + ) + + +@pytest.mark.unit +async def test_backfill_emits_completion_log(mocker, caplog): + """Backfill logs final rewritten/scanned counts at INFO.""" + client = mocker.AsyncMock() + client.retrieve.return_value = [] + client.scroll.side_effect = [ + ([_record(1, 7), _record(2, "x")], None), + ] + + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + completion_logs = [ + r.getMessage() for r in caplog.records if "backfill complete" in r.getMessage() + ] + assert completion_logs, "expected an INFO log line for backfill completion" + msg = completion_logs[0] + assert "1/2" in msg, f"expected '1/2' rewritten/scanned in {msg!r}" + + +@pytest.mark.unit +async def test_backfill_handles_none_payload(mocker): + """A point with payload=None is skipped without crashing.""" + client = mocker.AsyncMock() + client.retrieve.return_value = [] + client.scroll.side_effect = [ + ([_record(1, None), _record(2, 99)], None), + ] + + await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension()) + + # Only the int doc_id at point 2 was rewritten; the None-payload point was skipped. + assert client.set_payload.await_count == 1 + client.set_payload.assert_awaited_with( + collection_name="test-collection", + payload={"doc_id": "99"}, + points=[2], + wait=True, + ) + + +@pytest.mark.unit +async def test_backfill_handles_payload_with_explicit_none_doc_id(mocker): + """A payload of {doc_id: None, ...} is skipped just like payload=None.""" + client = mocker.AsyncMock() + client.retrieve.return_value = [] + # Build the record manually to distinguish payload=None from payload={"doc_id": None}. + point_with_explicit_none = SimpleNamespace( + id=1, payload={"doc_id": None, "doc_type": "file"} + ) + client.scroll.side_effect = [ + ([point_with_explicit_none, _record(2, 99)], None), + ] + + await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension()) + + # Only the int doc_id at point 2 was rewritten; the explicit-None payload was skipped. + assert client.set_payload.await_count == 1 + client.set_payload.assert_awaited_with( + collection_name="test-collection", + payload={"doc_id": "99"}, + points=[2], + wait=True, + ) + + +@pytest.mark.unit +async def test_backfill_logs_and_returns_when_scroll_raises(mocker, caplog): + """A scroll-time exception is logged and swallowed; sentinel is not written. + + The singleton client in get_qdrant_client is already assigned by the + time _backfill_doc_id_to_string runs, so re-raising here would leave + the process holding a usable client with the migration silently + skipped on every subsequent call. Catching, logging, and returning + without writing the sentinel preserves retry-on-next-restart behavior. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] # No sentinel — backfill must run + + # An async-callable side_effect lets AsyncMock await the coroutine + # before the exception propagates; assigning a bare exception class + # leaks an un-awaited coroutine and trips RuntimeWarning at gc time. + # The `await anyio.lowlevel.checkpoint()` is a no-op event-loop yield that + # satisfies static analysis ("async function uses no async features") + # without changing observable behavior. + async def _scroll_raises(*args, **kwargs): + await anyio.lowlevel.checkpoint() + raise RuntimeError("boom") + + client.scroll.side_effect = _scroll_raises + + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + # No sentinel written — next process restart will retry from scratch. + client.upsert.assert_not_awaited() + client.set_payload.assert_not_awaited() + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + assert "doc_id backfill scroll failed" in errors[0].getMessage() + assert "test-collection" in errors[0].getMessage() + # exc_info=True attaches the original exception to the log record. + assert errors[0].exc_info is not None + assert errors[0].exc_info[0] is RuntimeError + + +@pytest.mark.unit +async def test_backfill_logs_warning_when_sentinel_upsert_fails(mocker, caplog): + """Sentinel-write failure after a successful scroll logs WARNING, not ERROR. + + A failure here means the data migration succeeded but the + short-circuit marker is missing. The data is correct; only the + marker is absent, so the next restart will re-scroll an + already-clean collection (idempotent zero-write) and retry the + upsert. Differentiating this from a genuine scroll failure prevents + an "ERROR — backfill failed" log line that contradicts the + successful data state. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] # No sentinel — backfill must run + client.scroll.return_value = ([], None) # Empty scroll — clean collection + + async def _upsert_raises(*args, **kwargs): + # See _scroll_raises above for why this is async + sleep(0). + await anyio.lowlevel.checkpoint() + raise RuntimeError("sentinel write blip") + + client.upsert.side_effect = _upsert_raises + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + assert "sentinel write failed" in warnings[0].getMessage() + assert "test-collection" in warnings[0].getMessage() + assert warnings[0].exc_info is not None + assert warnings[0].exc_info[0] is RuntimeError + # No ERROR — data state is correct, not a backfill failure. + assert not [r for r in caplog.records if r.levelname == "ERROR"] + + +@pytest.mark.unit +async def test_backfill_emits_progress_log_every_20_batches(mocker, caplog): + """Long scrolls emit a progress INFO line every 20 batches. + + Operators auditing a 50k+ point collection's startup migration need + proof the server isn't hung; a single start/end pair leaves a + minutes-long silence in the log. The progress line carries the + collection name, scanned count, and rewritten count so the same + log message also acts as a heartbeat. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] + + # Return 21 non-empty batches followed by an empty one to terminate + # the loop; every batch contains points already in str form so no + # set_payload calls happen — the test focuses on the progress log + # cadence, not the rewrite path. + str_point = SimpleNamespace(id=1, payload={"doc_id": "abc"}) + # Real Qdrant returns next_offset as a UUID string (or None to terminate). + # Match that shape so the stub remains accurate if scroll's return type is + # ever tightened — and aligns with test_backfill_rewrites_int_doc_ids_to_str. + batches: list[tuple[list[SimpleNamespace], str | None]] = [ + ([str_point], "next-1") for _ in range(21) + ] + [([], None)] + client.scroll.side_effect = batches + + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + progress_messages = [ + r.getMessage() + for r in caplog.records + if "doc_id backfill progress on" in r.getMessage() + ] + # 21 batches → exactly one progress line at batch 20. + assert len(progress_messages) == 1 + assert "scanned 20 points" in progress_messages[0] + assert "test-collection" in progress_messages[0] + + +# --------------------------------------------------------------------------- +# _group_int_doc_ids +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_group_int_doc_ids_skips_float_and_warns(caplog): + """A float doc_id is not stringified; it logs WARNING and is skipped. + + Producers always write int or str. A float would round-trip to e.g. + ``"3.0"``, which the keyword index and verification path + (``int(doc_id)``) would never match. Skipping with a loud warning is + the only safe choice. + """ + float_point = SimpleNamespace(id=99, payload={"doc_id": 3.0}) + int_point = SimpleNamespace(id=42, payload={"doc_id": 7}) + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + by_value, scanned = _group_int_doc_ids([float_point, int_point]) + + # Only the int point made it into by_value; float was dropped. + assert by_value == {"7": [42]} + # Both points still count toward the scanned total — the warning + # should not hide them from progress logs. + assert scanned == 2 + + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + msg = warnings[0].getMessage() + assert "float" in msg + assert "99" in msg + + +@pytest.mark.unit +def test_group_int_doc_ids_skips_bool_and_warns(caplog): + """A bool doc_id is not stringified to "True"/"False"; it logs and skips. + + ``isinstance(True, int)`` is ``True`` because ``bool`` is a subclass of + ``int`` in Python, so a naive ``isinstance(value, int)`` guard would let + a boolean payload through and write ``str(True)`` → ``"True"`` into + Qdrant. Producers never write bools, but the strict ``type(value) is + int`` guard ensures any future producer bug surfaces as a WARNING and is + not silently stringified. + """ + bool_point = SimpleNamespace(id=33, payload={"doc_id": True}) + int_point = SimpleNamespace(id=42, payload={"doc_id": 7}) + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + by_value, scanned = _group_int_doc_ids([bool_point, int_point]) + + # Only the int point made it into by_value — "True" is *not* a key. + assert by_value == {"7": [42]} + assert "True" not in by_value + assert "False" not in by_value + assert scanned == 2 + + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + msg = warnings[0].getMessage() + assert "bool" in msg + assert "33" in msg + + +@pytest.mark.unit +def test_group_int_doc_ids_handles_str_and_missing_silently(caplog): + """str / missing doc_id payloads are skipped without warning. + + These are the steady-state paths — already-migrated str values and + sentinel-style points without a doc_id key. Neither should noise up + the log on every restart. + """ + str_point = SimpleNamespace(id=1, payload={"doc_id": "abc"}) + none_payload_point = SimpleNamespace(id=2, payload=None) + missing_key_point = SimpleNamespace(id=3, payload={"other": "value"}) + explicit_none_point = SimpleNamespace(id=4, payload={"doc_id": None}) + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + by_value, scanned = _group_int_doc_ids( + [str_point, none_payload_point, missing_key_point, explicit_none_point] + ) + + assert by_value == {} + assert scanned == 4 + # No warnings — these paths are expected and silent. + assert not [r for r in caplog.records if r.levelname == "WARNING"] + + +@pytest.mark.unit +def test_group_int_doc_ids_groups_ints_by_str_value(): + """Multiple int-doc_id points sharing a value collapse into one entry. + + Pins the chunk-batching contract: all chunks of one document share its + doc_id, so the helper hands ``_apply_backfill_writes`` a single key + with all chunk point-ids attached. + """ + by_value, scanned = _group_int_doc_ids( + [ + SimpleNamespace(id=10, payload={"doc_id": 42}), + SimpleNamespace(id=11, payload={"doc_id": 42}), + SimpleNamespace(id=12, payload={"doc_id": 7}), + ] + ) + + assert by_value == {"42": [10, 11], "7": [12]} + assert scanned == 3 + + +@pytest.mark.unit +async def test_ensure_payload_indexes_summarises_failed_fields(mocker, caplog): + """A non-400 failure surfaces both as ERROR and a WARNING summary. + + Per-field ERROR lines are easy to miss in startup noise; the + WARNING summary at the end of the loop names every field that + didn't get an index, so operators auditing the log can spot the + degraded state at a glance. + """ + client = mocker.AsyncMock() + client.get_collection.return_value = SimpleNamespace(payload_schema={}) + # _PAYLOAD_INDEX_FIELDS preserves insertion order; user_id is the second + # entry, so call #2 is the success case and every other field fails. Don't + # hard-code the full field list here — it grows as new fields move into + # the index dict, and the assertions below are what enforce coverage. + call_count = {"n": 0} + + async def _create_index(*args, **kwargs): + # See _scroll_raises above for why this is async + sleep(0). + await anyio.lowlevel.checkpoint() + call_count["n"] += 1 + if call_count["n"] != 2: + raise _make_unexpected(500, b'{"status":{"error":"boom"}}') + return None + + client.create_payload_index.side_effect = _create_index + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_payload_indexes(client, "test-collection") + + summary = [ + r.getMessage() + for r in caplog.records + if "Payload index creation incomplete" in r.getMessage() + ] + assert len(summary) == 1 + # Every field that failed must appear in the summary — operators rely on + # this single log line to spot the degraded state, so any missing entry + # is a silent gap. + assert "doc_id" in summary[0] + assert "doc_type" in summary[0] + assert "is_placeholder" in summary[0] + assert "chunk_index" in summary[0] + assert "chunk_start_offset" in summary[0] + assert "chunk_end_offset" in summary[0] + assert "user_id" not in summary[0] # The one that succeeded. + assert "test-collection" in summary[0] + + +# --------------------------------------------------------------------------- +# get_qdrant_client — collection-existence probe across modes +# --------------------------------------------------------------------------- + + +@pytest.fixture +def reset_qdrant_singleton(): + """Reset the module-level singleton + init lock around each test. + + ``get_qdrant_client`` short-circuits on a non-None ``_qdrant_client`` + via the unsynchronized fast path, so any prior test that initialised + the singleton would mask the cold-start logic these tests exercise. + Restore the original after the test so a leak doesn't bleed into + later tests in the same process. + """ + original_client = qdrant_module._qdrant_client + original_lock = qdrant_module._qdrant_init_lock + qdrant_module._qdrant_client = None + qdrant_module._qdrant_init_lock = None + yield + qdrant_module._qdrant_client = original_client + qdrant_module._qdrant_init_lock = original_lock + + +def _stub_provisional(mocker, get_collection_side_effect): + """Build a fake AsyncQdrantClient suitable for cold-start get_qdrant_client. + + ``get_collection`` is wired up to ``get_collection_side_effect``; + every other awaited method returns an AsyncMock so the migration + helpers (``_ensure_payload_indexes`` etc.) don't blow up on the + create-collection path. Returns the mock so tests can assert against + the awaited methods. + """ + provisional = mocker.AsyncMock() + provisional.get_collection.side_effect = get_collection_side_effect + # _ensure_payload_indexes pulls payload_schema off the freshly-created + # collection's get_collection result; on the create path it's passed + # an explicit {} so this branch isn't exercised, but make it safe + # anyway in case the order of init shifts. + provisional.create_payload_index.return_value = None + return provisional + + +def _stub_settings_and_embedding(mocker, monkeypatch): + """Replace get_settings and the embedding service with deterministic stubs.""" + from nextcloud_mcp_server.config import Settings + + settings = Settings( + qdrant_location=":memory:", + ollama_embedding_model="nomic-embed-text", + vector_sync_enabled=False, + ) + monkeypatch.setattr( + "nextcloud_mcp_server.vector.qdrant_client.get_settings", lambda: settings + ) + + embedding_service = mocker.Mock() + # No _detect_dimension attribute → the dynamic-detection branch is + # skipped. Real Ollama provider has it, but tests don't need to. + embedding_service.provider = mocker.Mock(spec_set=[]) + embedding_service.get_dimension = lambda: 4 + monkeypatch.setattr( + "nextcloud_mcp_server.embedding.get_embedding_service", + lambda: embedding_service, + ) + return settings + + +@pytest.mark.unit +async def test_get_qdrant_client_creates_collection_on_local_mode_value_error( + mocker, monkeypatch, reset_qdrant_singleton +): + """Local-mode `ValueError("Collection X not found")` must trigger create. + + The local/in-memory ``AsyncQdrantClient`` raises ``ValueError`` (see + ``qdrant_client/local/async_qdrant_local.py``) where the HTTP-mode + client would raise ``UnexpectedResponse(status_code=404)``. Both must + be treated as "the collection doesn't exist yet — create it." + Without this dual-path catch, the ``mcp`` container fails on first + start with `Failed to initialize Qdrant collection: Collection X not + found` and the ``app.py`` lifespan re-raises as ``RuntimeError``, + crashing every single-user / login-flow / multi-user-basic CI job. + """ + settings = _stub_settings_and_embedding(mocker, monkeypatch) + collection_name = settings.get_collection_name() + + provisional = _stub_provisional( + mocker, ValueError(f"Collection {collection_name} not found") + ) + monkeypatch.setattr( + "nextcloud_mcp_server.vector.qdrant_client.AsyncQdrantClient", + lambda *a, **kw: provisional, + ) + + client = await get_qdrant_client() + + assert client is provisional + provisional.create_collection.assert_awaited_once() + # The created collection should be the auto-generated name from + # settings — guards against accidental collection-name drift. + assert ( + provisional.create_collection.await_args.kwargs["collection_name"] + == collection_name + ) + + +@pytest.mark.unit +async def test_get_qdrant_client_propagates_unrelated_value_error( + mocker, monkeypatch, reset_qdrant_singleton +): + """A ValueError that is *not* a missing-collection signal must propagate. + + The ``except ValueError`` clause in ``get_qdrant_client`` matches on + the ``"not found"`` substring rather than catching every + ``ValueError`` so genuine programming bugs (bad ``collection_name`` + validation, dimension assertions, etc.) still surface to the caller. + Loosening the guard to a bare ``except ValueError`` would silently + treat any of those as "create the collection" and mask the bug. + """ + _stub_settings_and_embedding(mocker, monkeypatch) + provisional = _stub_provisional(mocker, ValueError("Bad collection_name")) + monkeypatch.setattr( + "nextcloud_mcp_server.vector.qdrant_client.AsyncQdrantClient", + lambda *a, **kw: provisional, + ) + + with pytest.raises(ValueError, match="Bad collection_name"): + await get_qdrant_client() + + provisional.create_collection.assert_not_awaited() diff --git a/third_party/astrolabe b/third_party/astrolabe index d9e641b9..7e08b498 160000 --- a/third_party/astrolabe +++ b/third_party/astrolabe @@ -1 +1 @@ -Subproject commit d9e641b93e6511fba0c27acc06ee0733fcdd5add +Subproject commit 7e08b4983ae03675a81039d85c7056eb7a3bd70e