From d90e793d19af8d972df3078143db0974e461b8dc Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 07:45:01 +0200 Subject: [PATCH] feat(search): verify-on-read for semantic search results (ADR-019) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vector index lags Nextcloud (5-min webhook cron + scanner interval), producing ghost records for deleted/unshared documents until the next reconciliation. Verify each unique document against Nextcloud at query time, drop inaccessible results, and lazily evict the corresponding Qdrant points. Per-doc_type batch verifiers: notes/files/deck cards run concurrently per id; news items use a single fetch + intersect to avoid the per-item fetch-all amplification. Transient errors fail open (keep result, log warning) — only definitive 4xx drops. Multiple chunks of the same doc collapse to one verification call. Wired into nc_semantic_search before the limit trim and before context expansion. nc_semantic_search_answer's per-note re-fetch retained as a sub-second race guard since verification now happens upstream. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 229 +++++++++ nextcloud_mcp_server/search/bm25_hybrid.py | 6 +- nextcloud_mcp_server/search/semantic.py | 6 +- nextcloud_mcp_server/search/verification.py | 438 ++++++++++++++++ nextcloud_mcp_server/server/semantic.py | 42 +- nextcloud_mcp_server/vector/eviction.py | 61 +++ tests/integration/test_verify_on_read.py | 155 ++++++ tests/unit/search/test_verification.py | 466 ++++++++++++++++++ 8 files changed, 1379 insertions(+), 24 deletions(-) create mode 100644 docs/ADR-019-verify-on-read-for-semantic-search.md create mode 100644 nextcloud_mcp_server/search/verification.py create mode 100644 nextcloud_mcp_server/vector/eviction.py create mode 100644 tests/integration/test_verify_on_read.py create mode 100644 tests/unit/search/test_verification.py diff --git a/docs/ADR-019-verify-on-read-for-semantic-search.md b/docs/ADR-019-verify-on-read-for-semantic-search.md new file mode 100644 index 00000000..9cbd2f12 --- /dev/null +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -0,0 +1,229 @@ +# ADR-019: Verify-on-Read for Semantic Search Results + +**Status**: Proposed +**Date**: 2026-05-01 +**Depends On**: ADR-007 (Background Vector Sync), ADR-010 (Webhook-Based Vector Sync) + +## Context + +The vector index in Qdrant is a *recall layer*, not the source of truth. Authoritative state for every indexed document — whether a note exists, whether a file is still shared with the user, whether a deck card is on a board the user can read — lives in Nextcloud, not in our index. Whenever those two views drift, semantic search returns **ghost records**: results that point to documents the user can no longer access (or that no longer exist at all). + +### How drift happens + +Two mechanisms keep Qdrant in sync with Nextcloud, and both have non-zero latency: + +1. **Webhook delivery (ADR-010)**. Nextcloud's `webhook_listeners` app dispatches change notifications via background jobs. The default `cron` job runs every 5 minutes, so even a healthy webhook pipeline opens a 0–5 minute window where deletions/unshares are not yet reflected in the index. Operators with dedicated webhook workers can shrink this, but most production deployments stay on the default cadence. + +2. **Periodic scanner (ADR-007)**. The fallback reconciliation scan runs on `vector_sync_scan_interval`. The dev default is 60 seconds, but ADR-010 explicitly recommends raising this to 1 hour or more in production once webhooks are in place, since the scanner exists primarily to recover from missed events. Large deployments may run it once per day. + +Beyond cadence, several failure modes cause webhooks to be missed entirely: + +- The MCP server is down or unreachable when the webhook fires (Nextcloud does not durably retry). +- Sharing changes (revoking a share, leaving a group) do not always emit file events that match what we registered for. +- Application-level deletions in apps without rich event support (older Deck versions, custom Tables flows) bypass the file-event hooks. + +In all of these cases, the document remains in Qdrant until the next periodic scan reconciles it — which may be hours away. Until then, `nc_semantic_search` happily returns the stale entry. + +### Why this matters more for semantic search than keyword search + +A keyword search via the Notes API is naturally bounded by what the API returns: deleted notes are not in the result set. The vector index is a separate store with its own lifecycle. The further we extend semantic search across apps (notes, files, deck cards, news items today; calendar, contacts, tables, cookbook tomorrow), the more divergent surfaces we expose to drift. Every new doc_type is another path where a webhook can be missed and another type of "ghost" can leak into results. + +The risk is not just a confusing UX. For RAG flows like `nc_semantic_search_answer` (ADR-008), a stale result means the LLM is asked to synthesize an answer over content the user no longer has access to — a privacy boundary violation, not just a relevance bug. + +### Current state of verification + +Verification today is ad-hoc and inconsistent: + +| Surface | Verifies? | Mechanism | +|---|---|---| +| `nc_semantic_search` | No | Returns raw Qdrant results. The docstring at `search/semantic.py:52` and `search/bm25_hybrid.py:75` references a `verify_search_results()` helper that was never implemented. | +| `nc_semantic_search_answer` | Partially | `server/semantic.py:431-448` fetches `notes.get_note(id)` and drops on exception — but only for `doc_type == "note"`. Files, news items, and deck cards fall through to the `else` branch (`server/semantic.py:449`) and are returned with their excerpt unverified. | +| `get_chunk_with_context` (when `include_context=True`) | Implicitly, all types | `search/context.py::_fetch_document_text` re-fetches the document; on failure the *context expansion* is skipped but the original (unverified) result is still returned (`server/semantic.py:246-251`). | + +There is no single point where the system asks: "is this document still accessible to this user, right now?" + +### The four indexed doc types + +The vector pipeline (`vector/scanner.py`, `vector/processor.py`) currently indexes four types, each with its own access-check shape: + +| doc_type | Cheapest authoritative check | Notes | +|---|---|---| +| `note` | `notes.get_note(id)` — single REST call, 404 on deletion | Per-user store; access is binary (yours or not). | +| `news_item` | `news.get_item(id)` — single REST call | Per-user feeds; clean 404 semantics. | +| `file` | WebDAV `PROPFIND` with `Depth: 0` on `file_path` (already stored in Qdrant payload, see `server/semantic.py:161`) | `read_file()` works but downloads the body — too heavy for a verification check. PROPFIND is the WebDAV equivalent of HEAD. Catches both deletes and unshares. | +| `deck_card` | `deck.get_card(board_id, stack_id, card_id)` using metadata cached in Qdrant (`search/context.py::_get_deck_metadata_from_qdrant`) | Fallback iteration through all boards/stacks (used by context expansion) is O(boards × stacks) and far too expensive to run on every query. | + +All four are query-time-cheap **if** we (a) deduplicate per-document before checking and (b) run checks concurrently. + +## Decision + +Implement **verify-on-read** as the authoritative access gate for semantic search. The vector index decides *what might be relevant*; Nextcloud decides *what the user can see*. We will: + +1. Introduce a single `nextcloud_mcp_server/search/verification.py` module exposing `verify_search_results(client, results) -> list[SearchResult]`. +2. Wire it into both `nc_semantic_search` and `nc_semantic_search_answer` as the final step before results leave the server, replacing the ad-hoc note-only verification in the answer tool. +3. Dispatch per `doc_type` to a registry of verifiers using the cheapest authoritative check for each type. +4. Lazily evict from Qdrant when verification reveals a definitively-gone document, so the next query for the same content does not re-pay the verification cost. + +The vector index becomes a **hint**, not a contract. We never trust it for access decisions. + +## Implementation + +### Module shape + +```python +# nextcloud_mcp_server/search/verification.py + +from typing import Awaitable, Callable, Protocol +import anyio +import httpx + +from nextcloud_mcp_server.search.algorithms import SearchResult + +# A verifier returns True if the document is currently accessible to the user. +# It MUST distinguish definitive 404/403 (return False) from transient errors +# (raise — caller will keep the result and log a warning). +Verifier = Callable[["NextcloudClientProtocol", int | str], Awaitable[bool]] + + +async def verify_search_results( + client: "NextcloudClientProtocol", + results: list[SearchResult], + *, + max_concurrent: int = 20, + evict_on_missing: bool = True, +) -> list[SearchResult]: + """Filter search results to those the user can currently access. + + Deduplicates by (doc_id, doc_type) before verifying, so multiple chunks + from the same document cost a single check. Verifies concurrently under + a semaphore. Drops results whose verifier returned False; keeps results + whose verifier raised (transient failure should not produce silent gaps). + + When evict_on_missing=True, schedules async deletion of the Qdrant points + for the missing document(s) so subsequent queries don't re-pay the cost. + """ +``` + +### Verifier registry + +```python +_VERIFIERS: dict[str, Verifier] = { + "note": _verify_note, + "news_item": _verify_news_item, + "file": _verify_file, + "deck_card": _verify_deck_card, +} +``` + +Each verifier follows the same pattern: + +```python +async def _verify_note(client, doc_id: int) -> bool: + try: + await client.notes.get_note(int(doc_id)) + return True + except httpx.HTTPStatusError as e: + if e.response.status_code in (403, 404): + return False + raise # transient — caller keeps the result +``` + +For `file`, use `webdav` PROPFIND (`Depth: 0`) on the `file_path` from the Qdrant payload, not `read_file()`. For `deck_card`, use the cached `(board_id, stack_id)` from `_get_deck_metadata_from_qdrant`; if metadata is absent, treat the result as accessible and log — we will not run the iteration fallback in the hot path. + +### Deduplication + +A 10-result page typically references 3–4 unique documents because of chunking. Verify each unique `(doc_id, doc_type)` once, then propagate the verdict to all chunks of that document: + +```python +unique_keys = {(r.id, r.doc_type) for r in results} +verdicts = {key: await _verify(client, key) for key in unique_keys} # via task group +return [r for r in results if verdicts.get((r.id, r.doc_type), True)] +``` + +A failed verification (raised exception) maps to "keep" — we do not want a flaky network blip to silently shrink results. + +### Lazy eviction + +When a verdict is `False`, queue a Qdrant delete for all points matching `(user_id, doc_id, doc_type)`. The plumbing already exists in `vector/placeholder.py::delete_placeholder_point` (which uses a filter-based delete); we need a sibling `delete_document_points` that omits the `is_placeholder` filter, so it removes real chunks too. + +Eviction is fire-and-forget from the verification path — wrap it in a background task group on the lifespan context to avoid blocking the response. If eviction fails, the next query will simply re-verify and re-attempt; this is self-healing. + +### Wiring + +In `server/semantic.py::nc_semantic_search`, after the existing dedup and `[:limit]` slice, but **before** context expansion (which is expensive and pointless on inaccessible results): + +```python +search_results = all_results[:limit * 2] # fetch extra to absorb evictions +search_results = await verify_search_results(client, search_results) +search_results = search_results[:limit] +``` + +Note the over-fetch: verification can shrink the page, so we ask for `limit * 2` candidates and trim *after* verification. This preserves the user's requested page size when ghosts are present without paying for full re-search. + +In `server/semantic.py::nc_semantic_search_answer`, replace the per-type `if result.doc_type == "note"` branch (lines 428-453) with a call to `verify_search_results` followed by the existing full-content fetch (which can stay note-specific, since only notes use full content; the rest still use excerpts). + +### What we deliberately do NOT do + +- **No verification cache.** The whole point of verify-on-read is that the answer can change between calls. A short-TTL cache (say, 30s) is plausible if benchmarks show verification dominating latency, but it is not in the v1 scope. +- **No verifier for unsupported doc_types.** If a future doc_type lands in Qdrant without a registered verifier, log a warning and pass the result through. Verification is opt-in per type; missing a verifier is a soft failure. +- **No deck-card iteration fallback.** The fallback in `_fetch_document_text` exists for context expansion, where O(boards × stacks) is acceptable for a single result. In verification we may run the check on every chunk in every search; the fallback would amplify search latency unacceptably. + +## Consequences + +### Positive + +- **Correctness**: Deletes/unshares are reflected in search results within one query, regardless of webhook delivery delays or scanner intervals. Operators can safely raise `vector_sync_scan_interval` to its production-recommended value without leaking ghost records. +- **Privacy**: RAG flows (`nc_semantic_search_answer`) can no longer synthesize answers over content the user has lost access to. +- **Self-healing index**: Lazy eviction means the index converges toward correctness as users query, without needing the scanner to find every drifted record. +- **Single source of truth**: Removes the docstring/code mismatch where `verify_search_results()` was promised but never delivered. + +### Negative + +- **Latency tax on every search**: Each unique `(doc_id, doc_type)` adds one Nextcloud round-trip. With 3–4 unique docs and 20-way concurrency, this is one parallel batch — likely under 100ms on a healthy connection, but it *is* on the critical path. +- **API load on Nextcloud**: A query that previously hit only Qdrant now hits Nextcloud once per unique result. For high-QPS deployments this is non-trivial and may need rate limiting (already present in `BaseNextcloudClient` retry logic). +- **More moving parts in the search path**: Errors in verification can mask errors in search. Verifier exceptions must be logged distinctly so debugging stays tractable. +- **Doc_type coverage is now a correctness contract**: When we add a new indexable doc_type, we must add a verifier in the same PR, or accept that ghost records are possible for that type. CI should fail if a doc_type is indexed without a registered verifier. + +### Neutral + +- The `verify_search_results()` function name in existing docstrings becomes accurate. No public API breakage. +- Webhooks remain valuable — they keep the index *recall* fresh (so semantically-relevant new docs appear in results quickly). Verification only handles the *precision* side (filtering inaccessible ones out). + +## Alternatives Considered + +**1. Tighten webhook delivery cadence.** Reduce Nextcloud's webhook cron interval from 5 minutes to 1 minute, or run a dedicated webhook worker. *Rejected as a complete solution*: addresses average-case latency but does nothing for missed webhooks, server-down windows, or app surfaces that lack rich events. We still recommend operators do this — it improves recall freshness — but it cannot replace verification. + +**2. Synchronous webhook acknowledgement.** Have the MCP server delete from Qdrant inside the webhook handler before returning 2xx. *Rejected*: still doesn't help missed webhooks, and adds a hard dependency from the webhook critical path to Qdrant being reachable. Already partially implemented; verify-on-read complements it rather than replacing it. + +**3. Bloom filter / negative cache of recently-deleted IDs.** Maintain an in-memory set of "known deleted" IDs populated by webhook handlers, consulted before returning search results. *Rejected*: cannot answer for unshares (which are user-relative, not global), grows unbounded, and is essentially a worse verifier — verifying against Nextcloud is authoritative and not much slower for the page sizes we deal with. + +**4. Verify only in `nc_semantic_search_answer`, not `nc_semantic_search`.** Argue that raw search is "advisory" and verification only matters when the LLM consumes content. *Rejected*: ghost records in raw search results are still misleading to users and to other tools that compose on top of search. The bar for a search tool is "results are accessible," not "results are accessible if you happen to feed them into a sampling tool." + +**5. Pre-verification at index time only (no query-time check).** *Already what we have*, and the problem statement. + +## Related Decisions + +- ADR-007: Background Vector Sync — establishes the polling architecture that produces drift. +- ADR-008: MCP Sampling for Semantic Search — defines the RAG flow that most acutely needs verified results. +- ADR-010: Webhook-Based Vector Sync — reduces but does not eliminate drift; verify-on-read closes the residual gap. +- ADR-013: RAG Evaluation — verification policy should be exercised in eval suites (with both fresh and stale fixtures). + +## References + +- `nextcloud_mcp_server/search/semantic.py:52` and `search/bm25_hybrid.py:75` — orphaned `verify_search_results()` references. +- `nextcloud_mcp_server/server/semantic.py:428-453` — current note-only verification in `nc_semantic_search_answer`. +- `nextcloud_mcp_server/search/context.py::_fetch_document_text` — per-doc-type fetch logic that informs the verifier registry. +- `nextcloud_mcp_server/vector/placeholder.py::delete_placeholder_point` — filter-based Qdrant delete pattern to extend for full-document eviction. + +## Implementation Checklist + +- [ ] Create `nextcloud_mcp_server/search/verification.py` with `verify_search_results()` and the verifier registry. +- [ ] Implement `_verify_note`, `_verify_news_item`, `_verify_file` (PROPFIND), `_verify_deck_card` (metadata fast-path only). +- [ ] Add `delete_document_points()` in `vector/placeholder.py` (or a new `vector/eviction.py`) for non-placeholder filter-based deletes. +- [ ] Wire into `nc_semantic_search` with `limit * 2` over-fetch, trim to `limit` after verification. +- [ ] Wire into `nc_semantic_search_answer`, replacing the per-type note branch. +- [ ] Update existing docstrings in `search/semantic.py:52` and `search/bm25_hybrid.py:75` to point at the new helper. +- [ ] Unit tests: each verifier handles 200/403/404/transient distinctly; dedup collapses chunks; eviction is scheduled on `False`. +- [ ] Integration test: index a note, delete via API (no webhook), confirm the next semantic search does not return it. +- [ ] CI guard: enumerate indexed doc_types in `vector/scanner.py` and assert each has a registered verifier. +- [ ] Document the latency budget and rate-limit posture in `docs/configuration.md`. diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 2dc7609c..0661e9f8 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -71,8 +71,10 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): """ Execute hybrid search using dense + sparse vectors with native RRF fusion. - Returns unverified results from Qdrant. Access verification should be - performed separately at the final output stage using verify_search_results(). + Returns unverified results from Qdrant. Access verification is + performed separately at the server tool layer via + ``nextcloud_mcp_server.search.verification.verify_search_results`` + (see ADR-019). Deduplicates by (doc_id, doc_type, chunk_start_offset, chunk_end_offset) to show multiple chunks from the same document while avoiding duplicate chunks. diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index c1903d39..bfad15a5 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -48,8 +48,10 @@ class SemanticSearchAlgorithm(SearchAlgorithm): ) -> list[SearchResult]: """Execute semantic search using vector similarity. - Returns unverified results from Qdrant. Access verification should be - performed separately at the final output stage using verify_search_results(). + Returns unverified results from Qdrant. Access verification is + performed separately at the server tool layer via + ``nextcloud_mcp_server.search.verification.verify_search_results`` + (see ADR-019). Deduplicates by (doc_id, doc_type, chunk_start_offset, chunk_end_offset) to show multiple chunks from the same document while avoiding duplicate chunks. diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py new file mode 100644 index 00000000..b2799a7d --- /dev/null +++ b/nextcloud_mcp_server/search/verification.py @@ -0,0 +1,438 @@ +"""Verify-on-read access checks for semantic search results (ADR-019). + +The vector index is a recall layer; Nextcloud is the source of truth for +access. This module filters search results by checking each unique document +against Nextcloud at query time, dropping any that the user can no longer +access (deleted, unshared, etc.) and lazily evicting them from the index. + +Per-doc_type verifiers are registered in ``_VERIFIERS``. Each takes the +authenticated client, a list of doc_ids, and the user_id, and returns the +subset of doc_ids that are currently accessible. The dispatch deliberately +groups by doc_type so doc-types with cheap batch endpoints (news_item) can +do a single fetch rather than one round-trip per result. + +Failure policy: + +- Definitive 403/404 from Nextcloud → drop the result and schedule eviction. +- Transient errors (5xx, network blips, unexpected exceptions) → keep the + result and log a warning. We never silently shrink result sets due to + flakes; the next query will re-verify. +- Unsupported doc_type (no registered verifier) → keep the result and log a + warning. Verification is opt-in per type; a missing verifier is a soft + failure, not a search failure. +""" + +import logging +from collections.abc import Awaitable, Callable +from typing import Any + +import anyio +from httpx import HTTPStatusError +from qdrant_client.models import FieldCondition, Filter, MatchValue + +from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.vector.eviction import delete_document_points +from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client + +logger = logging.getLogger(__name__) + + +BatchVerifier = Callable[[Any, list[int | str], str], Awaitable[set[int | str]]] +"""(client, doc_ids, user_id) -> set of accessible doc_ids.""" + + +# --------------------------------------------------------------------------- +# Per-doc-type verifiers +# --------------------------------------------------------------------------- + + +def _is_definitive_404_or_403(exc: BaseException) -> bool: + """Return True if exc indicates the document is definitively inaccessible.""" + if isinstance(exc, HTTPStatusError): + return exc.response.status_code in (403, 404) + return False + + +async def _verify_notes( + client: Any, doc_ids: list[int | str], user_id: str +) -> set[int | str]: + accessible: set[int | str] = set() + + async def check(doc_id: int | str) -> None: + try: + await client.notes.get_note(int(doc_id)) + accessible.add(doc_id) + except HTTPStatusError as e: + if _is_definitive_404_or_403(e): + return + logger.warning( + "Transient error verifying note %s: %s %s; keeping result", + doc_id, + e.response.status_code, + e, + ) + accessible.add(doc_id) + except Exception as e: + logger.warning( + "Unexpected error verifying note %s: %s; keeping result", + doc_id, + e, + ) + accessible.add(doc_id) + + async with anyio.create_task_group() as tg: + for doc_id in doc_ids: + tg.start_soon(check, doc_id) + + return accessible + + +async def _verify_files( + client: Any, doc_ids: list[int | str], user_id: str +) -> set[int | str]: + accessible: set[int | str] = set() + + async def check(doc_id: int | str) -> None: + # Resolve file_id → file_path from Qdrant payload + file_path = await _resolve_file_path(user_id, doc_id) + if file_path is None: + # Cannot verify without a path; treat as accessible to avoid + # silently dropping legitimate results when payload is missing + logger.warning( + "No file_path in Qdrant for file_id %s; keeping result " + "(verification skipped)", + doc_id, + ) + accessible.add(doc_id) + return + + try: + info = await client.webdav.get_file_info(file_path) + if info is None: + # get_file_info returns None on definitive 404 + return + accessible.add(doc_id) + except HTTPStatusError as e: + if _is_definitive_404_or_403(e): + return + logger.warning( + "Transient error verifying file %s (%s): %s %s; keeping result", + doc_id, + file_path, + e.response.status_code, + e, + ) + accessible.add(doc_id) + except Exception as e: + logger.warning( + "Unexpected error verifying file %s (%s): %s; keeping result", + doc_id, + file_path, + e, + ) + accessible.add(doc_id) + + async with anyio.create_task_group() as tg: + for doc_id in doc_ids: + tg.start_soon(check, doc_id) + + return accessible + + +async def _verify_deck_cards( + client: Any, doc_ids: list[int | str], user_id: str +) -> set[int | str]: + accessible: set[int | str] = set() + + async def check(doc_id: int | str) -> None: + # Resolve card_id → (board_id, stack_id) from Qdrant payload + meta = await _resolve_deck_metadata(user_id, int(doc_id)) + if meta is None: + # Without metadata we cannot run the cheap fast-path. Per ADR-019 + # we deliberately do NOT fall back to O(boards × stacks) iteration + # in the search hot path; treat as accessible. + logger.warning( + "No deck metadata in Qdrant for card %s; keeping result " + "(verification skipped, legacy data without board_id/stack_id)", + doc_id, + ) + accessible.add(doc_id) + return + + try: + await client.deck.get_card( + board_id=meta["board_id"], + stack_id=meta["stack_id"], + card_id=int(doc_id), + ) + accessible.add(doc_id) + except HTTPStatusError as e: + if _is_definitive_404_or_403(e): + return + logger.warning( + "Transient error verifying deck card %s: %s %s; keeping result", + doc_id, + e.response.status_code, + e, + ) + accessible.add(doc_id) + except Exception as e: + logger.warning( + "Unexpected error verifying deck card %s: %s; keeping result", + doc_id, + e, + ) + accessible.add(doc_id) + + async with anyio.create_task_group() as tg: + for doc_id in doc_ids: + tg.start_soon(check, doc_id) + + return accessible + + +async def _verify_news_items( + client: Any, doc_ids: list[int | str], user_id: str +) -> set[int | str]: + """Batch-verify news items with a single fetch. + + The Nextcloud News API has no per-item endpoint, so ``news.get_item`` is + implemented as a fetch-all + filter — which would be O(N × all_items) if + called per id. Instead we fetch once and intersect. + """ + requested = {int(d) for d in doc_ids} + + try: + items = await client.news.get_items(batch_size=-1, get_read=True) + except HTTPStatusError as e: + # If the News API itself is gone (app disabled, user lost access), + # treat *all* requested items as inaccessible. Eviction will reclaim. + if _is_definitive_404_or_403(e): + logger.info( + "News API returned %s for user %s; treating all %d news_items as inaccessible", + e.response.status_code, + user_id, + len(requested), + ) + return set() + logger.warning( + "Transient error fetching news items for verification: %s %s; keeping all results", + e.response.status_code, + e, + ) + return set(doc_ids) + except Exception as e: + logger.warning( + "Unexpected error fetching news items for verification: %s; keeping all results", + e, + ) + return set(doc_ids) + + present_ids = {int(item.get("id")) for item in items if item.get("id") is not None} + # Map back to the original doc_id types (the caller may pass ints or strs) + accessible: set[int | str] = set() + for d in doc_ids: + if int(d) in present_ids and int(d) in requested: + accessible.add(d) + return accessible + + +_VERIFIERS: dict[str, BatchVerifier] = { + "note": _verify_notes, + "file": _verify_files, + "deck_card": _verify_deck_cards, + "news_item": _verify_news_items, +} + + +def get_supported_doc_types() -> set[str]: + """Return the set of doc_types that have registered verifiers. + + Used by CI guards and tests to ensure every indexed doc_type has a + verifier (see ADR-019 implementation checklist). + """ + return set(_VERIFIERS.keys()) + + +# --------------------------------------------------------------------------- +# Qdrant payload lookup helpers +# --------------------------------------------------------------------------- + + +async def _resolve_file_path(user_id: str, doc_id: int | str) -> str | None: + """Look up file_path for a file_id from any chunk's Qdrant payload.""" + try: + qdrant_client = await get_qdrant_client() + settings = get_settings() + + scroll_result = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="doc_type", match=MatchValue(value="file")), + ] + ), + limit=1, + with_payload=["file_path"], + with_vectors=False, + ) + + if scroll_result[0]: + point = scroll_result[0][0] + file_path = point.payload.get("file_path") if point.payload else None + if file_path: + return str(file_path) + return None + + except Exception as e: + logger.debug("Error resolving file_path for file_id %s: %s", doc_id, e) + return None + + +async def _resolve_deck_metadata(user_id: str, card_id: int) -> dict[str, int] | None: + """Look up (board_id, stack_id) for a deck card from any chunk's payload.""" + try: + qdrant_client = await get_qdrant_client() + settings = get_settings() + + scroll_result = await qdrant_client.scroll( + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition(key="doc_id", match=MatchValue(value=card_id)), + FieldCondition(key="doc_type", match=MatchValue(value="deck_card")), + ] + ), + limit=1, + with_payload=["board_id", "stack_id"], + with_vectors=False, + ) + + if scroll_result[0]: + point = scroll_result[0][0] + payload = point.payload or {} + board_id = payload.get("board_id") + stack_id = payload.get("stack_id") + if board_id is not None and stack_id is not None: + return {"board_id": int(board_id), "stack_id": int(stack_id)} + return None + + except Exception as e: + logger.debug("Error resolving deck metadata for card %s: %s", card_id, e) + return None + + +# --------------------------------------------------------------------------- +# Public entry point +# --------------------------------------------------------------------------- + + +async def verify_search_results( + client: Any, + results: list[SearchResult], + *, + evict_on_missing: bool = True, +) -> list[SearchResult]: + """Filter search results to those the user can currently access. + + Deduplicates by ``(doc_id, doc_type)`` before verifying, so multiple + chunks from the same document cost a single check. Verifiers run + concurrently per doc_type (and within each doc_type, per id where that + is cheaper than batching). + + When ``evict_on_missing=True``, points for documents that fail + verification are deleted from Qdrant in-line. Eviction failures are + logged but never propagated. + + Args: + client: Authenticated NextcloudClient (must expose ``username``). + results: SearchResult list from the algorithm layer (may include + multiple chunks per document). + evict_on_missing: Schedule lazy eviction for inaccessible docs. + + Returns: + Filtered list preserving the original order. + """ + if not results: + return results + + user_id: str = client.username + + # Group unique (doc_id, doc_type) by doc_type so each verifier sees a + # deduplicated batch. + by_type: dict[str, set[int | str]] = {} + for r in results: + by_type.setdefault(r.doc_type, set()).add(r.id) + + # Run all type verifiers concurrently. Per-id failures are absorbed + # inside each verifier; this outer task group only fans out per type. + accessible_by_type: dict[str, set[int | str]] = {} + + async def run_verifier(doc_type: str, doc_ids: set[int | str]) -> None: + verifier = _VERIFIERS.get(doc_type) + if verifier is None: + logger.warning( + "No verifier registered for doc_type=%r; keeping %d result(s) unverified", + doc_type, + len(doc_ids), + ) + accessible_by_type[doc_type] = doc_ids + return + try: + accessible_by_type[doc_type] = await verifier( + client, list(doc_ids), user_id + ) + except Exception as e: + # Verifier itself blew up (not per-id) — fail open. + logger.error( + "Verifier for doc_type=%s raised: %s; keeping all %d result(s) unverified", + doc_type, + e, + len(doc_ids), + exc_info=True, + ) + accessible_by_type[doc_type] = doc_ids + + async with anyio.create_task_group() as tg: + for doc_type, doc_ids in by_type.items(): + tg.start_soon(run_verifier, doc_type, doc_ids) + + # Compute (doc_id, doc_type) pairs that failed verification + inaccessible: set[tuple[int | str, str]] = set() + for doc_type, doc_ids in by_type.items(): + accessible = accessible_by_type.get(doc_type, doc_ids) + for doc_id in doc_ids: + if doc_id not in accessible: + inaccessible.add((doc_id, doc_type)) + + if inaccessible: + logger.info( + "Verification dropped %d inaccessible document(s): %s", + len(inaccessible), + sorted((str(d), t) for d, t in inaccessible), + ) + + # Filter results in-place-style, preserving order + kept = [r for r in results if (r.id, r.doc_type) not in inaccessible] + + # Lazy eviction — fire and forget, but bounded inline so we don't lose + # the user_id binding by escaping the task group. + if evict_on_missing and inaccessible: + + async def evict(doc_id: int | str, doc_type: str) -> None: + try: + await delete_document_points(doc_id, doc_type, user_id) + except Exception as e: + logger.warning( + "Failed to evict %s_%s from Qdrant: %s", doc_type, doc_id, e + ) + + async with anyio.create_task_group() as tg: + for doc_id, doc_type in inaccessible: + tg.start_soon(evict, doc_id, doc_type) + + return kept diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index b8e28ef8..c156db9b 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -32,6 +32,7 @@ from nextcloud_mcp_server.observability.metrics import ( ) from nextcloud_mcp_server.search.bm25_hybrid import BM25HybridSearchAlgorithm from nextcloud_mcp_server.search.context import get_chunk_with_context +from nextcloud_mcp_server.search.verification import verify_search_results from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -144,16 +145,15 @@ def configure_semantic_tools(mcp: FastMCP): # Sort combined results by score all_results.sort(key=lambda r: r.score, reverse=True) - # Note: BM25HybridSearchAlgorithm already deduplicates at chunk level - # (doc_id, doc_type, chunk_start, chunk_end), which allows multiple - # chunks from the same document while preventing duplicate chunks. - # No additional deduplication needed here - multiple chunks per document - # are valuable for RAG contexts. - # Qdrant already filters by user_id for multi-tenant isolation. - # Sampling tool will verify access when fetching full content. - search_results = all_results[ - :limit - ] # Final limit after chunk-level dedup in algorithm + # ADR-019: Verify-on-read. The vector index is a recall layer; + # Nextcloud is the source of truth for access. Filter out ghost + # records (deleted/unshared docs not yet reconciled by webhooks) + # BEFORE trimming to `limit`, so we don't lose accessible results + # to the limit slot that ghosts would otherwise occupy. We also + # run this BEFORE context expansion to avoid re-fetching docs that + # are about to be dropped. + verified_results = await verify_search_results(client, all_results) + search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response results = [] @@ -414,9 +414,11 @@ def configure_semantic_tools(mcp: FastMCP): success=True, ) - # 4. Fetch full content for notes in parallel (also verifies access) - # Use anyio task group for concurrent fetching with semaphore to prevent - # connection pool exhaustion + # 4. Fetch full content for notes in parallel. + # Access verification has already happened upstream in + # nc_semantic_search via verify_search_results (ADR-019), so any + # exception here is a sub-second race (doc deleted between + # verification and this fetch) — drop the result in that case. client = await get_client(ctx) accessible_results = [None] * len(search_response.results) full_contents = [None] * len(search_response.results) @@ -431,7 +433,6 @@ def configure_semantic_tools(mcp: FastMCP): if result.doc_type == "note": try: note = await client.notes.get_note(result.id) - # Note is accessible, store result and full content content = note.get("content", "") accessible_results[index] = result full_contents[index] = content @@ -440,15 +441,16 @@ def configure_semantic_tools(mcp: FastMCP): f"(length: {len(content)} chars)" ) except Exception as e: - # Note might have been deleted or permissions changed - # Leave as None to filter out later + # Race window after verify_search_results — drop result. logger.debug( - f"Note {result.id} not accessible: {e}. " - f"Excluding from results." + "Note %s disappeared between verification and " + "content fetch: %s. Excluding from results.", + result.id, + e, ) else: - # Non-note document types (future: calendar, deck, files) - # For now, keep them with excerpts + # Non-note types (file, news_item, deck_card) keep the + # excerpt — already access-verified upstream. accessible_results[index] = result # full_contents[index] remains None (will use excerpt) diff --git a/nextcloud_mcp_server/vector/eviction.py b/nextcloud_mcp_server/vector/eviction.py new file mode 100644 index 00000000..44874e08 --- /dev/null +++ b/nextcloud_mcp_server/vector/eviction.py @@ -0,0 +1,61 @@ +"""Lazy eviction of stale documents from the vector index. + +Used by the verify-on-read path (ADR-019) to remove points for documents that +have been deleted or unshared in Nextcloud but not yet reconciled by the +webhook/scanner sync loop. Eviction is fire-and-forget from the search hot +path; failures are logged but never propagated, since the next query will +simply re-verify and re-attempt. +""" + +import logging + +from qdrant_client.models import FieldCondition, Filter, MatchValue + +from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client + +logger = logging.getLogger(__name__) + + +async def delete_document_points( + doc_id: str | int, + doc_type: str, + user_id: str, +) -> None: + """Remove all Qdrant points for a single document. + + Deletes both real chunk points and any leftover placeholder points for the + given (user_id, doc_id, doc_type) tuple. Safe to call when the document is + not present — Qdrant returns successfully with zero points affected. + + Args: + doc_id: Document ID (int for notes/files/cards/news, str otherwise) + doc_type: Document type (note, file, deck_card, news_item) + user_id: Owner of the points being evicted + + Raises: + Exception: If the underlying Qdrant client raises. Callers in the + search hot path should catch and log; eviction failures must not + block search responses. + """ + qdrant_client = await get_qdrant_client() + settings = get_settings() + + await qdrant_client.delete( + collection_name=settings.get_collection_name(), + points_selector=Filter( + must=[ + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), + FieldCondition(key="doc_type", match=MatchValue(value=doc_type)), + ] + ), + ) + + logger.info( + "Evicted Qdrant points for %s_%s (user=%s); " + "document was inaccessible at verification time", + doc_type, + doc_id, + user_id, + ) diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py new file mode 100644 index 00000000..3359c7d4 --- /dev/null +++ b/tests/integration/test_verify_on_read.py @@ -0,0 +1,155 @@ +"""Integration tests for verify-on-read access checks (ADR-019). + +These tests exercise ``verify_search_results`` against a real Nextcloud +instance — the verification path's whole purpose is to consult Nextcloud as +the source of truth, so unit-level mocks don't catch protocol or status-code +mismatches between our verifier and the real API. + +Qdrant is mocked out (``delete_document_points`` and the payload-resolution +helpers) so these tests don't require a running vector database. The unit +suite in ``tests/unit/search/test_verification.py`` covers the Qdrant-side +behaviour separately. +""" + +import logging +import uuid + +import pytest +from httpx import HTTPStatusError + +from nextcloud_mcp_server.client import NextcloudClient +from nextcloud_mcp_server.search import verification +from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.search.verification import verify_search_results + +logger = logging.getLogger(__name__) + +pytestmark = pytest.mark.integration + + +def _result_for_note(note_id: int) -> SearchResult: + return SearchResult( + id=note_id, + doc_type="note", + title=f"note_{note_id}", + excerpt="...", + score=0.9, + ) + + +async def test_verify_keeps_accessible_note( + nc_client: NextcloudClient, temporary_note: dict, mocker +): + """A note that exists in Nextcloud must be kept by verification.""" + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + note_id = temporary_note["id"] + results = [_result_for_note(note_id)] + + kept = await verify_search_results(nc_client, results) + + assert [r.id for r in kept] == [note_id] + spy_evict.assert_not_awaited() + + +async def test_verify_drops_deleted_note_and_schedules_eviction( + nc_client: NextcloudClient, mocker +): + """The core ghost-record scenario. + + Create a note, delete it via the API (no webhook delivery), then run + verification with a SearchResult still pointing at the gone-but-indexed + document. verify-on-read must drop it and schedule eviction. + """ + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + # Create a note we'll delete to simulate a ghost record + unique_suffix = uuid.uuid4().hex[:8] + created = await nc_client.notes.create_note( + title=f"verify-on-read ghost {unique_suffix}", + content="This note will be deleted before verification runs.", + category="VerifyOnReadTest", + ) + note_id = created["id"] + + # Delete via API directly. In production a webhook *should* fire and + # evict from Qdrant — but the whole point of ADR-019 is that we cannot + # rely on this. Verification must catch the drift independently. + await nc_client.notes.delete_note(note_id=note_id) + + # Confirm the note is really gone before running verification, so the + # test fails fast if the API behaves unexpectedly. + with pytest.raises(HTTPStatusError) as exc_info: + await nc_client.notes.get_note(note_id) + assert exc_info.value.response.status_code == 404 + + kept = await verify_search_results(nc_client, [_result_for_note(note_id)]) + + assert kept == [], "deleted note must not pass verification" + spy_evict.assert_awaited_once_with(note_id, "note", nc_client.username) + + +async def test_verify_mixed_accessible_and_deleted( + nc_client: NextcloudClient, temporary_note: dict, mocker +): + """Verification must drop only the inaccessible result, keep the rest.""" + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + # temporary_note stays alive for the duration of the test. + accessible_id = temporary_note["id"] + + # Make a second note and immediately delete it to create a ghost id. + unique_suffix = uuid.uuid4().hex[:8] + ghost = await nc_client.notes.create_note( + title=f"verify-on-read ghost mix {unique_suffix}", + content="ghost", + category="VerifyOnReadTest", + ) + ghost_id = ghost["id"] + await nc_client.notes.delete_note(note_id=ghost_id) + + results = [ + _result_for_note(accessible_id), + _result_for_note(ghost_id), + ] + kept = await verify_search_results(nc_client, results) + + assert [r.id for r in kept] == [accessible_id] + spy_evict.assert_awaited_once_with(ghost_id, "note", nc_client.username) + + +async def test_verify_dedupes_chunks_of_same_document( + nc_client: NextcloudClient, temporary_note: dict, mocker +): + """Multiple chunks of the same note must produce ONE Nextcloud round-trip.""" + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + # Spy through to the real notes client to count round-trips + real_get_note = nc_client.notes.get_note + spy_get_note = mocker.AsyncMock(side_effect=real_get_note) + mocker.patch.object(nc_client.notes, "get_note", spy_get_note) + + note_id = temporary_note["id"] + # Three chunks of the same note (chunk_index varies) + results = [ + SearchResult( + id=note_id, + doc_type="note", + title="note", + excerpt=f"chunk {i}", + score=0.9 - i * 0.1, + chunk_index=i, + ) + for i in range(3) + ] + + kept = await verify_search_results(nc_client, results) + + # All three chunks kept (they're all from the same accessible note) + assert len(kept) == 3 + # ...but verification only fetched the note ONCE + assert spy_get_note.await_count == 1 diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py new file mode 100644 index 00000000..4cffdfe5 --- /dev/null +++ b/tests/unit/search/test_verification.py @@ -0,0 +1,466 @@ +"""Unit tests for verify-on-read (ADR-019).""" + +from types import SimpleNamespace + +import httpx +import pytest +from httpx import HTTPStatusError + +from nextcloud_mcp_server.search import verification +from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.search.verification import ( + _verify_deck_cards, + _verify_files, + _verify_news_items, + _verify_notes, + get_supported_doc_types, + verify_search_results, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_result( + doc_id: int, + doc_type: str = "note", + chunk_index: int = 0, + score: float = 0.9, +) -> SearchResult: + return SearchResult( + id=doc_id, + doc_type=doc_type, + title=f"{doc_type}_{doc_id}", + excerpt="...", + score=score, + chunk_index=chunk_index, + ) + + +def _http_error(status_code: int) -> HTTPStatusError: + request = httpx.Request("GET", "http://test.local/x") + response = httpx.Response(status_code=status_code, request=request) + return HTTPStatusError(f"{status_code}", request=request, response=response) + + +# --------------------------------------------------------------------------- +# Registry shape +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_supported_doc_types_covers_indexed_types(): + """ADR-019 implementation checklist: every indexed doc_type has a verifier. + + Indexed types are defined in vector/scanner.py and vector/processor.py: + note, file, deck_card, news_item. + """ + expected = {"note", "file", "deck_card", "news_item"} + assert get_supported_doc_types() >= expected + + +# --------------------------------------------------------------------------- +# Note verifier +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_notes_200_keeps_all(mocker): + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(return_value={"id": 1, "content": "x"}) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [1, 2, 3], "alice") + + assert result == {1, 2, 3} + assert notes_client.get_note.await_count == 3 + + +@pytest.mark.unit +async def test_verify_notes_404_drops(mocker): + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=_http_error(404)) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [42], "alice") + + assert result == set() + + +@pytest.mark.unit +async def test_verify_notes_403_drops(mocker): + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=_http_error(403)) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [42], "alice") + + assert result == set() + + +@pytest.mark.unit +async def test_verify_notes_transient_5xx_keeps(mocker): + """Transient errors must NOT silently shrink results.""" + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=_http_error(503)) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [42], "alice") + + assert result == {42} + + +@pytest.mark.unit +async def test_verify_notes_unexpected_exception_keeps(mocker): + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=RuntimeError("boom")) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [7], "alice") + + assert result == {7} + + +@pytest.mark.unit +async def test_verify_notes_mixed_outcomes(mocker): + """Mix of accessible, deleted, and transient — only deleted is dropped.""" + + async def side_effect(note_id): + if note_id == 1: + return {"id": 1} + if note_id == 2: + raise _http_error(404) # deleted + if note_id == 3: + raise _http_error(500) # transient → keep + raise AssertionError(f"unexpected id {note_id}") + + notes_client = SimpleNamespace(get_note=mocker.AsyncMock(side_effect=side_effect)) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [1, 2, 3], "alice") + + assert result == {1, 3} + + +# --------------------------------------------------------------------------- +# News batch verifier +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_news_items_intersects_with_fetched_set(mocker): + """News verifier does ONE fetch and intersects, regardless of how many ids.""" + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(return_value=[{"id": 10}, {"id": 20}, {"id": 30}]) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items(client, [10, 20, 99], "alice") + + assert result == {10, 20} + assert news_client.get_items.await_count == 1 + + +@pytest.mark.unit +async def test_verify_news_items_api_404_drops_all(mocker): + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(side_effect=_http_error(404)) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items(client, [1, 2, 3], "alice") + + assert result == set() + + +@pytest.mark.unit +async def test_verify_news_items_transient_keeps_all(mocker): + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(side_effect=_http_error(502)) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items(client, [1, 2, 3], "alice") + + assert result == {1, 2, 3} + + +# --------------------------------------------------------------------------- +# File verifier +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_files_uses_propfind_when_path_resolves(mocker): + mocker.patch.object( + verification, "_resolve_file_path", return_value="Documents/foo.txt" + ) + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(return_value={"id": 100}) + ) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files(client, [100], "alice") + + assert result == {100} + webdav_client.get_file_info.assert_awaited_once_with("Documents/foo.txt") + + +@pytest.mark.unit +async def test_verify_files_404_via_get_file_info_drops(mocker): + """get_file_info returns None on 404 — that's a definitive drop.""" + mocker.patch.object(verification, "_resolve_file_path", return_value="gone.txt") + webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files(client, [123], "alice") + + assert result == set() + + +@pytest.mark.unit +async def test_verify_files_missing_payload_keeps_unverified(mocker): + """Without a file_path we cannot verify — fail open, don't drop.""" + mocker.patch.object(verification, "_resolve_file_path", return_value=None) + webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files(client, [555], "alice") + + assert result == {555} + webdav_client.get_file_info.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# Deck card verifier +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_deck_cards_uses_metadata_fast_path(mocker): + mocker.patch.object( + verification, + "_resolve_deck_metadata", + return_value={"board_id": 1, "stack_id": 2}, + ) + deck_client = SimpleNamespace(get_card=mocker.AsyncMock(return_value=object())) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards(client, [42], "alice") + + assert result == {42} + deck_client.get_card.assert_awaited_once_with(board_id=1, stack_id=2, card_id=42) + + +@pytest.mark.unit +async def test_verify_deck_cards_403_drops(mocker): + """Board unshared with user → 403 from get_card → drop.""" + mocker.patch.object( + verification, + "_resolve_deck_metadata", + return_value={"board_id": 1, "stack_id": 2}, + ) + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=_http_error(403)) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards(client, [42], "alice") + + assert result == set() + + +@pytest.mark.unit +async def test_verify_deck_cards_no_metadata_skips_verification(mocker): + """Legacy data without board_id/stack_id payload → keep, do NOT iterate.""" + mocker.patch.object(verification, "_resolve_deck_metadata", return_value=None) + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=AssertionError("must not be called")) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards(client, [42], "alice") + + assert result == {42} + deck_client.get_card.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# Top-level verify_search_results +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_search_results_empty_input_passthrough(): + client = SimpleNamespace(username="alice") + assert await verify_search_results(client, []) == [] + + +@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}) + mocker.patch.dict(verification._VERIFIERS, {"note": spy}, clear=False) + mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) + + results = [ + _make_result(1, doc_type="note", chunk_index=0), + _make_result(1, doc_type="note", chunk_index=1), + _make_result(1, doc_type="note", chunk_index=2), + ] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert len(kept) == 3 # all kept, all reference the same accessible doc + spy.assert_awaited_once() + # Verifier received the single deduplicated id, not three copies + args, _kwargs = spy.call_args + assert args[1] == [1] + + +@pytest.mark.unit +async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + # Verifier reports note 1 accessible, note 99 not + note_verifier = mocker.AsyncMock(return_value={1}) + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + results = [ + _make_result(1, doc_type="note"), + _make_result(99, doc_type="note"), + ] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert [r.id for r in kept] == [1] + spy_evict.assert_awaited_once_with(99, "note", "alice") + + +@pytest.mark.unit +async def test_verify_search_results_no_eviction_when_disabled(mocker): + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + note_verifier = mocker.AsyncMock(return_value=set()) # all inaccessible + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + results = [_make_result(7, doc_type="note")] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results, evict_on_missing=False) + + assert kept == [] + spy_evict.assert_not_awaited() + + +@pytest.mark.unit +async def test_verify_search_results_unknown_doc_type_passes_through(mocker, caplog): + """No verifier registered for doc_type → keep, log a warning.""" + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + # Ensure no verifier for "calendar" + mocker.patch.dict( + verification._VERIFIERS, + {k: v for k, v in verification._VERIFIERS.items() if k != "calendar"}, + clear=True, + ) + + results = [_make_result(1, doc_type="calendar")] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert len(kept) == 1 + spy_evict.assert_not_awaited() + + +@pytest.mark.unit +async def test_verify_search_results_verifier_blowup_keeps_all(mocker): + """A verifier raising an unexpected exception must not silently drop results.""" + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + note_verifier = mocker.AsyncMock(side_effect=RuntimeError("qdrant down")) + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + results = [ + _make_result(1, doc_type="note"), + _make_result(2, doc_type="note"), + ] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert [r.id for r in kept] == [1, 2] + spy_evict.assert_not_awaited() + + +@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}) + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) + + results = [ + _make_result(1, doc_type="note", score=0.9), + _make_result(2, doc_type="note", score=0.8), + _make_result(3, doc_type="note", score=0.7), + ] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert [r.id for r in kept] == [1, 3] + + +@pytest.mark.unit +async def test_verify_search_results_eviction_failure_does_not_propagate(mocker): + """Eviction failures are logged, never raised — must not break search.""" + mocker.patch.object( + verification, + "delete_document_points", + mocker.AsyncMock(side_effect=RuntimeError("qdrant down")), + ) + note_verifier = mocker.AsyncMock(return_value=set()) + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + client = SimpleNamespace(username="alice") + # Should NOT raise + kept = await verify_search_results(client, [_make_result(1, doc_type="note")]) + assert kept == [] + + +@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}) + mocker.patch.dict( + verification._VERIFIERS, + {"note": note_verifier, "file": file_verifier}, + clear=False, + ) + mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) + + results = [ + _make_result(1, doc_type="note"), + _make_result(500, doc_type="file"), + _make_result(999, doc_type="file"), # to be dropped + ] + client = SimpleNamespace(username="alice") + + kept = await verify_search_results(client, results) + + assert {(r.id, r.doc_type) for r in kept} == {(1, "note"), (500, "file")} + note_verifier.assert_awaited_once() + file_verifier.assert_awaited_once()