From d90e793d19af8d972df3078143db0974e461b8dc Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 07:45:01 +0200 Subject: [PATCH 01/16] 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() From 7784ec02d7ffae8b976eacc5b3a8d5186810ee47 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 08:22:28 +0200 Subject: [PATCH 02/16] refactor(search): address PR #750 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cap all_results to limit*2 after sort in the per-doc_types branch of nc_semantic_search to bound over-verification (was unbounded N-types). - Switch BatchVerifier from (client, doc_ids, user_id) to (client, results, semaphore). Verifiers now read file paths and deck board/stack ids from SearchResult.metadata instead of doing fresh Qdrant scrolls — eliminates one duplicate round-trip per file/deck-card verification. - Bound per-id verification concurrency with a shared anyio.Semaphore (default 20, matching server/semantic.py context-expansion convention). Prevents httpx pool exhaustion / rate limiting on large search pages. - Propagate stack_id from Qdrant payload to SearchResult.metadata in both bm25_hybrid.py and semantic.py (board_id was already propagated). - Drop now-unused _resolve_file_path / _resolve_deck_metadata helpers. - Drop redundant int(d) in requested predicate from _verify_news_items. - Rewrite eviction comment to be honest about inline (not background) execution and the resulting latency coupling. - ADR-019 status: Proposed -> Accepted. - Add news property to NextcloudClientProtocol. - Widen SearchResult.id and SemanticSearchResult.id to int | str to match BatchVerifier signature and document support for future string-id types. - Flip openWorldHint to True on nc_semantic_search_answer (it calls into Nextcloud via nc_semantic_search). Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 2 +- nextcloud_mcp_server/models/semantic.py | 8 +- nextcloud_mcp_server/search/algorithms.py | 11 +- nextcloud_mcp_server/search/bm25_hybrid.py | 5 + nextcloud_mcp_server/search/semantic.py | 5 + nextcloud_mcp_server/search/verification.py | 406 ++++++++---------- nextcloud_mcp_server/server/semantic.py | 11 +- tests/unit/search/test_verification.py | 202 +++++++-- 8 files changed, 380 insertions(+), 270 deletions(-) diff --git a/docs/ADR-019-verify-on-read-for-semantic-search.md b/docs/ADR-019-verify-on-read-for-semantic-search.md index 9cbd2f12..13abf041 100644 --- a/docs/ADR-019-verify-on-read-for-semantic-search.md +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -1,6 +1,6 @@ # ADR-019: Verify-on-Read for Semantic Search Results -**Status**: Proposed +**Status**: Accepted **Date**: 2026-05-01 **Depends On**: ADR-007 (Background Vector Sync), ADR-010 (Webhook-Based Vector Sync) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 9c5266c4..652f921b 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -10,7 +10,13 @@ from .base import BaseResponse class SemanticSearchResult(BaseModel): """Model for semantic search results with additional metadata.""" - id: int = Field(description="Document ID (int for all document types)") + id: int | str = Field( + description=( + "Document ID. Numeric for all currently indexed types (notes, files, " + "deck cards, news items); typed as int|str to allow future doc types " + "that use string identifiers." + ) + ) doc_type: str = Field( description="Document type (note, calendar_event, deck_card, etc.)" ) diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 0d3bba23..2657bb45 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -67,6 +67,11 @@ class NextcloudClientProtocol(Protocol): """Tables client for accessing table row documents.""" ... + @property + def news(self) -> Any: + """News client for accessing news item documents.""" + ... + async def get_indexed_doc_types(user_id: str) -> set[str]: """Query Qdrant to get actually-indexed document types for a user. @@ -127,7 +132,9 @@ class SearchResult: """A single search result with metadata and score. Attributes: - id: Document ID (int for all document types) + 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). doc_type: Document type (note, file, calendar, contact, etc.) title: Document title excerpt: Content excerpt showing match context @@ -144,7 +151,7 @@ class SearchResult: point_id: Qdrant point ID for batch vector retrieval (None if not from Qdrant) """ - id: int + id: int | str doc_type: str title: str excerpt: str diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 0661e9f8..243f9714 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -233,9 +233,14 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): 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( diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index bfad15a5..c01b0a37 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -164,9 +164,14 @@ class SemanticSearchAlgorithm(SearchAlgorithm): 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( diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index b2799a7d..7d5a8110 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -6,10 +6,18 @@ 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. +authenticated client, the (deduplicated) list of ``SearchResult``s for that +doc_type, and a shared concurrency semaphore. They return the subset of +``doc_id`` values that are currently accessible. Verifiers read whatever +metadata they need (file path, deck card board/stack ids) directly from the +SearchResult — these fields are populated at index-time and propagated by +the algorithm layer (see ``search/bm25_hybrid.py`` and ``search/semantic.py``) +so verification adds zero extra Qdrant round-trips. + +Concurrency is bounded by a shared semaphore (default 20) so a large search +result page (or a multi-doc_type query) cannot exhaust the httpx connection +pool or trigger Nextcloud rate limiting. The 20-slot default matches the +context-expansion convention in ``server/semantic.py``. Failure policy: @@ -28,18 +36,22 @@ 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.""" +# Default cap on concurrent verification round-trips against Nextcloud. Matches +# the convention in ``server/semantic.py`` for context-expansion fan-out. +DEFAULT_VERIFICATION_CONCURRENCY = 20 + + +BatchVerifier = Callable[ + [Any, list[SearchResult], anyio.Semaphore], Awaitable[set[int | str]] +] +"""(client, results, semaphore) -> set of doc_ids accessible to the user.""" # --------------------------------------------------------------------------- @@ -55,185 +67,200 @@ def _is_definitive_404_or_403(exc: BaseException) -> bool: async def _verify_notes( - client: Any, doc_ids: list[int | str], user_id: str + client: Any, results: list[SearchResult], semaphore: anyio.Semaphore ) -> 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 def check(result: SearchResult) -> None: + async with semaphore: + doc_id = result.id + 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) + for r in results: + tg.start_soon(check, r) return accessible async def _verify_files( - client: Any, doc_ids: list[int | str], user_id: str + client: Any, results: list[SearchResult], semaphore: anyio.Semaphore ) -> 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: + async def check(result: SearchResult) -> None: + doc_id = result.id + # file_path is propagated from the Qdrant payload by the algorithm + # layer (bm25_hybrid.py / semantic.py). No extra Qdrant round-trip. + file_path = (result.metadata or {}).get("path") + if not file_path: # Cannot verify without a path; treat as accessible to avoid # silently dropping legitimate results when payload is missing + # (legacy data, or a future doc_type that doesn't propagate path). logger.warning( - "No file_path in Qdrant for file_id %s; keeping result " + "No file path in metadata 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 semaphore: + 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) + for r in results: + tg.start_soon(check, r) return accessible async def _verify_deck_cards( - client: Any, doc_ids: list[int | str], user_id: str + client: Any, results: list[SearchResult], semaphore: anyio.Semaphore ) -> 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: + async def check(result: SearchResult) -> None: + doc_id = result.id + # board_id and stack_id are propagated from the Qdrant payload by the + # algorithm layer. No extra Qdrant round-trip. + meta = result.metadata or {} + board_id = meta.get("board_id") + stack_id = meta.get("stack_id") + if board_id is None or stack_id 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)", + "Incomplete deck metadata for card %s (board_id=%s, stack_id=%s); " + "keeping result (verification skipped, legacy data)", doc_id, + board_id, + stack_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 semaphore: + try: + await client.deck.get_card( + board_id=int(board_id), + stack_id=int(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) + for r in results: + tg.start_soon(check, r) return accessible async def _verify_news_items( - client: Any, doc_ids: list[int | str], user_id: str + client: Any, results: list[SearchResult], semaphore: anyio.Semaphore ) -> 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. + called per id. Instead we fetch once and intersect. The semaphore is + accepted for signature symmetry but not heavily used (one round-trip total). """ - requested = {int(d) for d in doc_ids} + doc_ids = [r.id for r in results] - 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", + async with semaphore: + 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, + client.username, + len(doc_ids), + ) + return set() + logger.warning( + "Transient error fetching news items for verification: %s %s; keeping all results", e.response.status_code, - user_id, - len(requested), + e, ) - 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) + 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) + # Map back to the original doc_id types (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: + if int(d) in present_ids: accessible.add(d) return accessible @@ -255,77 +282,6 @@ def get_supported_doc_types() -> set[str]: 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 # --------------------------------------------------------------------------- @@ -336,13 +292,14 @@ async def verify_search_results( results: list[SearchResult], *, evict_on_missing: bool = True, + max_concurrent: int = DEFAULT_VERIFICATION_CONCURRENCY, ) -> 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). + concurrently per doc_type and concurrently per id within each verifier, + bounded by a shared semaphore (``max_concurrent``). When ``evict_on_missing=True``, points for documents that fail verification are deleted from Qdrant in-line. Eviction failures are @@ -353,6 +310,8 @@ async def verify_search_results( results: SearchResult list from the algorithm layer (may include multiple chunks per document). evict_on_missing: Schedule lazy eviction for inaccessible docs. + max_concurrent: Cap on concurrent verification round-trips against + Nextcloud. Defaults to ``DEFAULT_VERIFICATION_CONCURRENCY``. Returns: Filtered list preserving the original order. @@ -363,28 +322,33 @@ async def verify_search_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]] = {} + # 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]] = {} for r in results: - by_type.setdefault(r.doc_type, set()).add(r.id) + by_type.setdefault(r.doc_type, {}).setdefault(r.id, r) + + # Shared semaphore bounds total Nextcloud round-trips across all + # per-id verifiers. Without it, a 50-result mostly-notes page could fan + # out 50 concurrent get_note calls and exhaust the connection pool. + semaphore = anyio.Semaphore(max_concurrent) - # 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: + async def run_verifier(doc_type: str, unique_results: list[SearchResult]) -> 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), + len(unique_results), ) - accessible_by_type[doc_type] = doc_ids + accessible_by_type[doc_type] = {r.id for r in unique_results} return try: accessible_by_type[doc_type] = await verifier( - client, list(doc_ids), user_id + client, unique_results, semaphore ) except Exception as e: # Verifier itself blew up (not per-id) — fail open. @@ -392,20 +356,20 @@ async def verify_search_results( "Verifier for doc_type=%s raised: %s; keeping all %d result(s) unverified", doc_type, e, - len(doc_ids), + len(unique_results), exc_info=True, ) - accessible_by_type[doc_type] = doc_ids + accessible_by_type[doc_type] = {r.id for r in unique_results} 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) + for doc_type, id_to_result in by_type.items(): + 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() - for doc_type, doc_ids in by_type.items(): - accessible = accessible_by_type.get(doc_type, doc_ids) - for doc_id in doc_ids: + for doc_type, id_to_result in by_type.items(): + accessible = accessible_by_type.get(doc_type, set(id_to_result.keys())) + for doc_id in id_to_result.keys(): if doc_id not in accessible: inaccessible.add((doc_id, doc_type)) @@ -416,11 +380,15 @@ async def verify_search_results( sorted((str(d), t) for d, t in inaccessible), ) - # Filter results in-place-style, preserving order + # Filter results, preserving order. All chunks of an inaccessible document + # are dropped together (dedup happened before verification, but the result + # list still contains all chunks). 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. + # Lazy eviction. Runs inline before returning — slow Qdrant will delay + # the search response. Background eviction would need a task registered + # on the lifespan context; the inline approach is acceptable given + # typical Qdrant delete latency, but callers should be aware. if evict_on_missing and inaccessible: async def evict(doc_id: int | str, doc_type: str) -> None: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index c156db9b..6b804494 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -142,8 +142,13 @@ def configure_semantic_tools(mcp: FastMCP): ) all_results.extend(unverified_results) - # Sort combined results by score + # Sort combined results by score, then cap to `limit * 2` to + # match the cross-app branch's over-fetch budget. Without this + # cap, N requested doc_types × `limit * 2` results would all + # flow into verification, multiplying the Nextcloud round-trip + # cost by N. all_results.sort(key=lambda r: r.score, reverse=True) + all_results = all_results[: limit * 2] # ADR-019: Verify-on-read. The vector index is a recall layer; # Nextcloud is the source of truth for access. Filter out ghost @@ -300,7 +305,7 @@ def configure_semantic_tools(mcp: FastMCP): title="Search with AI-Generated Answer", annotations=ToolAnnotations( readOnlyHint=True, # Search doesn't modify data - openWorldHint=False, # Searches only indexed Nextcloud data + openWorldHint=True, # Calls into Nextcloud via nc_semantic_search ), ) @require_scopes("semantic.read") @@ -432,7 +437,7 @@ def configure_semantic_tools(mcp: FastMCP): async with semaphore: if result.doc_type == "note": try: - note = await client.notes.get_note(result.id) + note = await client.notes.get_note(int(result.id)) content = note.get("content", "") accessible_results[index] = result full_contents[index] = content diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 4cffdfe5..21becc4d 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -2,6 +2,7 @@ from types import SimpleNamespace +import anyio import httpx import pytest from httpx import HTTPStatusError @@ -22,11 +23,16 @@ from nextcloud_mcp_server.search.verification import ( # --------------------------------------------------------------------------- +def _sem(slots: int = 20) -> anyio.Semaphore: + return anyio.Semaphore(slots) + + def _make_result( - doc_id: int, + doc_id: int | str, doc_type: str = "note", chunk_index: int = 0, score: float = 0.9, + metadata: dict | None = None, ) -> SearchResult: return SearchResult( id=doc_id, @@ -35,6 +41,7 @@ def _make_result( excerpt="...", score=score, chunk_index=chunk_index, + metadata=metadata, ) @@ -72,7 +79,9 @@ async def test_verify_notes_200_keeps_all(mocker): ) client = SimpleNamespace(notes=notes_client, username="alice") - result = await _verify_notes(client, [1, 2, 3], "alice") + result = await _verify_notes( + client, [_make_result(1), _make_result(2), _make_result(3)], _sem() + ) assert result == {1, 2, 3} assert notes_client.get_note.await_count == 3 @@ -85,7 +94,7 @@ async def test_verify_notes_404_drops(mocker): ) client = SimpleNamespace(notes=notes_client, username="alice") - result = await _verify_notes(client, [42], "alice") + result = await _verify_notes(client, [_make_result(42)], _sem()) assert result == set() @@ -97,7 +106,7 @@ async def test_verify_notes_403_drops(mocker): ) client = SimpleNamespace(notes=notes_client, username="alice") - result = await _verify_notes(client, [42], "alice") + result = await _verify_notes(client, [_make_result(42)], _sem()) assert result == set() @@ -110,7 +119,7 @@ async def test_verify_notes_transient_5xx_keeps(mocker): ) client = SimpleNamespace(notes=notes_client, username="alice") - result = await _verify_notes(client, [42], "alice") + result = await _verify_notes(client, [_make_result(42)], _sem()) assert result == {42} @@ -122,7 +131,7 @@ async def test_verify_notes_unexpected_exception_keeps(mocker): ) client = SimpleNamespace(notes=notes_client, username="alice") - result = await _verify_notes(client, [7], "alice") + result = await _verify_notes(client, [_make_result(7)], _sem()) assert result == {7} @@ -143,7 +152,9 @@ async def test_verify_notes_mixed_outcomes(mocker): 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") + result = await _verify_notes( + client, [_make_result(1), _make_result(2), _make_result(3)], _sem() + ) assert result == {1, 3} @@ -161,7 +172,15 @@ async def test_verify_news_items_intersects_with_fetched_set(mocker): ) client = SimpleNamespace(news=news_client, username="alice") - result = await _verify_news_items(client, [10, 20, 99], "alice") + result = await _verify_news_items( + client, + [ + _make_result(10, doc_type="news_item"), + _make_result(20, doc_type="news_item"), + _make_result(99, doc_type="news_item"), + ], + _sem(), + ) assert result == {10, 20} assert news_client.get_items.await_count == 1 @@ -174,7 +193,15 @@ async def test_verify_news_items_api_404_drops_all(mocker): ) client = SimpleNamespace(news=news_client, username="alice") - result = await _verify_news_items(client, [1, 2, 3], "alice") + result = await _verify_news_items( + client, + [ + _make_result(1, doc_type="news_item"), + _make_result(2, doc_type="news_item"), + _make_result(3, doc_type="news_item"), + ], + _sem(), + ) assert result == set() @@ -186,7 +213,15 @@ async def test_verify_news_items_transient_keeps_all(mocker): ) client = SimpleNamespace(news=news_client, username="alice") - result = await _verify_news_items(client, [1, 2, 3], "alice") + result = await _verify_news_items( + client, + [ + _make_result(1, doc_type="news_item"), + _make_result(2, doc_type="news_item"), + _make_result(3, doc_type="news_item"), + ], + _sem(), + ) assert result == {1, 2, 3} @@ -197,16 +232,18 @@ async def test_verify_news_items_transient_keeps_all(mocker): @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" - ) +async def test_verify_files_uses_path_from_metadata(mocker): + """File verifier reads path from SearchResult.metadata, no Qdrant round-trip.""" 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") + result = await _verify_files( + client, + [_make_result(100, doc_type="file", metadata={"path": "Documents/foo.txt"})], + _sem(), + ) assert result == {100} webdav_client.get_file_info.assert_awaited_once_with("Documents/foo.txt") @@ -215,27 +252,54 @@ async def test_verify_files_uses_propfind_when_path_resolves(mocker): @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") + result = await _verify_files( + client, + [_make_result(123, doc_type="file", metadata={"path": "gone.txt"})], + _sem(), + ) 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)) +async def test_verify_files_missing_path_metadata_keeps_unverified(mocker): + """Without a path in metadata we cannot verify — fail open, don't drop.""" + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=AssertionError("must not be called")) + ) client = SimpleNamespace(webdav=webdav_client, username="alice") - result = await _verify_files(client, [555], "alice") - + # No metadata at all + result = await _verify_files(client, [_make_result(555, doc_type="file")], _sem()) 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} + webdav_client.get_file_info.assert_not_awaited() + + +@pytest.mark.unit +async def test_verify_files_transient_5xx_keeps(mocker): + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=_http_error(503)) + ) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(7, doc_type="file", metadata={"path": "x.txt"})], + _sem(), + ) + + assert result == {7} + # --------------------------------------------------------------------------- # Deck card verifier @@ -244,15 +308,21 @@ async def test_verify_files_missing_payload_keeps_unverified(mocker): @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 verifier reads board_id+stack_id from metadata, no Qdrant round-trip.""" 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") + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) assert result == {42} deck_client.get_card.assert_awaited_once_with(board_id=1, stack_id=2, card_id=42) @@ -261,33 +331,56 @@ async def test_verify_deck_cards_uses_metadata_fast_path(mocker): @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") + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) 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) +async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): + """Legacy data without board_id/stack_id → keep, do NOT iterate or call API.""" 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") - + # No metadata at all + result = await _verify_deck_cards( + client, [_make_result(42, doc_type="deck_card")], _sem() + ) assert result == {42} + + # Only board_id (stack_id missing) + result = await _verify_deck_cards( + client, + [_make_result(43, doc_type="deck_card", metadata={"board_id": 1})], + _sem(), + ) + assert result == {43} + + # Only stack_id (board_id missing) + result = await _verify_deck_cards( + client, + [_make_result(44, doc_type="deck_card", metadata={"stack_id": 2})], + _sem(), + ) + assert result == {44} + deck_client.get_card.assert_not_awaited() @@ -320,9 +413,12 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker): 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 + # Verifier received exactly one SearchResult (the deduplicated representative) args, _kwargs = spy.call_args - assert args[1] == [1] + assert len(args[1]) == 1 + assert args[1][0].id == 1 + # And a semaphore as the third arg + assert isinstance(args[2], anyio.Semaphore) @pytest.mark.unit @@ -454,8 +550,8 @@ async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker results = [ _make_result(1, doc_type="note"), - _make_result(500, doc_type="file"), - _make_result(999, doc_type="file"), # to be dropped + _make_result(500, doc_type="file", metadata={"path": "a.txt"}), + _make_result(999, doc_type="file", metadata={"path": "b.txt"}), # to be dropped ] client = SimpleNamespace(username="alice") @@ -464,3 +560,21 @@ async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker assert {(r.id, r.doc_type) for r in kept} == {(1, "note"), (500, "file")} note_verifier.assert_awaited_once() file_verifier.assert_awaited_once() + + +@pytest.mark.unit +async def test_verify_search_results_passes_semaphore_to_verifier(mocker): + """The dispatcher must construct a Semaphore and pass it to verifiers.""" + captured: dict[str, anyio.Semaphore] = {} + + async def verifier(client, results, semaphore): + captured["sem"] = semaphore + return {r.id for r in results} + + mocker.patch.dict(verification._VERIFIERS, {"note": verifier}, clear=False) + mocker.patch.object(verification, "delete_document_points", mocker.AsyncMock()) + + client = SimpleNamespace(username="alice") + await verify_search_results(client, [_make_result(1)], max_concurrent=5) + + assert isinstance(captured["sem"], anyio.Semaphore) From 21e5608a39b414d4be2ff5d41f34103bb2ea135c Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 18:53:32 +0200 Subject: [PATCH 03/16] refactor(search): address PR #750 round 2 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements fire-and-forget eviction (ADR-019 §"Lazy eviction"): the search response no longer waits on Qdrant deletes, instead spawning evict() on a long-lived lifespan-owned task group. Falls back to inline eviction in modes without vector sync and in unit tests. Also: harden _verify_news_items against non-numeric ids (fail open instead of crashing the verifier); document the get_file_info None-on-404 contract; add INDEXED_DOC_TYPES single source of truth in vector/scanner.py referenced by the CI-guard test; write a Verify-on-Read Latency Budget section in docs/configuration.md covering the unbounded news.get_items fetch. Closes the two remaining ADR-019 implementation checklist items. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 4 +- docs/configuration.md | 39 ++++++++++ nextcloud_mcp_server/app.py | 25 +++++++ nextcloud_mcp_server/search/verification.py | 75 ++++++++++++++----- nextcloud_mcp_server/server/semantic.py | 13 +++- nextcloud_mcp_server/vector/scanner.py | 10 +++ tests/unit/search/test_verification.py | 55 ++++++++++++-- 7 files changed, 195 insertions(+), 26 deletions(-) diff --git a/docs/ADR-019-verify-on-read-for-semantic-search.md b/docs/ADR-019-verify-on-read-for-semantic-search.md index 13abf041..87848745 100644 --- a/docs/ADR-019-verify-on-read-for-semantic-search.md +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -225,5 +225,5 @@ In `server/semantic.py::nc_semantic_search_answer`, replace the per-type `if res - [ ] 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`. +- [x] CI guard: enumerate indexed doc_types in `vector/scanner.py` and assert each has a registered verifier. (`INDEXED_DOC_TYPES` in `vector/scanner.py`; `tests/unit/search/test_verification.py::test_supported_doc_types_covers_indexed_types`.) +- [x] Document the latency budget and rate-limit posture in `docs/configuration.md`. (See "Verify-on-Read Latency Budget" section.) diff --git a/docs/configuration.md b/docs/configuration.md index 513ef05c..b16443ec 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -474,6 +474,45 @@ DOCUMENT_CHUNK_OVERLAP=100 **Important**: Changing chunk size requires re-embedding all documents. The collection naming strategy (see "Qdrant Collection Naming" above) helps manage this by creating separate collections for different configurations. +### Verify-on-Read Latency Budget + +Every semantic search request runs an access-control verification pass over its +results before returning them, to filter out documents the user can no longer +access (deleted, unshared, permissions changed). See +[ADR-019](ADR-019-verify-on-read-for-semantic-search.md) for the full design. + +This adds Nextcloud round-trips to the search path that operators should be +aware of: + +- **Per-search cost**: one Nextcloud round-trip per *unique* `(doc_id, doc_type)` + in the result set. Chunking means a 10-result page typically references 3-5 + unique documents, so verification adds 3-5 round-trips. With the default + 20-way concurrency this is one parallel batch — usually under 100 ms on a + healthy connection. +- **Concurrency**: all verifications fan out under a shared semaphore + (`DEFAULT_VERIFICATION_CONCURRENCY = 20` in `search/verification.py`). The + limit is not currently exposed as an env var; if production workloads + saturate Nextcloud, consider opening an issue to make it tunable. +- **News API caveat**: the News app has no per-item endpoint, so the news + verifier issues a single `news.get_items(batch_size=-1, get_read=True)` call + per search that contains any news result, then intersects locally. The + payload is **unbounded** — for users with very large feed backlogs this can + dominate verification latency. Disabling News in the indexer or running with + a smaller backlog mitigates this; per-item paginated verification is tracked + as a future improvement. +- **Eviction**: when verification finds a definitive miss (404 / 403), the + corresponding Qdrant points are deleted in the background on a lifespan-owned + task group — fire-and-forget, does **not** block the search response. + Eviction failures are logged but never propagated; the next query will + re-verify and re-attempt (self-healing). +- **Failure modes**: transient errors (5xx, network) keep results visible + (fail open) so a flaky link does not silently shrink result pages; only + *definitive* 404 / 403 drops them. + +If verification ever needs to be disabled (debugging, benchmarking), the +`evict_on_missing=False` flag on `verify_search_results()` skips eviction +without changing what is returned to the caller. + ### Environment Variables Reference | Variable | Required | Default | Description | diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index f5e72660..86a93565 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -15,6 +15,7 @@ from urllib.parse import urlparse import anyio import click import httpx +from anyio.abc import TaskGroup from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream from mcp.server.auth.settings import AuthSettings from mcp.server.fastmcp import Context, FastMCP @@ -319,6 +320,10 @@ class VectorSyncState: document_receive_stream: Optional[MemoryObjectReceiveStream] = None shutdown_event: Optional[anyio.Event] = None scanner_wake_event: Optional[anyio.Event] = None + # Long-lived task group used for fire-and-forget background work spawned + # from the request path (e.g. ADR-019 verify-on-read eviction). Set by the + # starlette lifespan after entering its task group; cleared on shutdown. + eviction_task_group: Optional[TaskGroup] = None # Module-level singleton for vector sync state @@ -335,6 +340,7 @@ class AppContext: document_receive_stream: Optional[MemoryObjectReceiveStream] = None shutdown_event: Optional[anyio.Event] = None scanner_wake_event: Optional[anyio.Event] = None + eviction_task_group: Optional[TaskGroup] = None @dataclass @@ -353,6 +359,7 @@ class OAuthAppContext: document_receive_stream: Optional[MemoryObjectReceiveStream] = None shutdown_event: Optional[anyio.Event] = None scanner_wake_event: Optional[anyio.Event] = None + eviction_task_group: Optional[TaskGroup] = None class BasicAuthMiddleware: @@ -569,6 +576,7 @@ async def app_lifespan_basic(server: FastMCP) -> AsyncIterator[AppContext]: document_receive_stream=_vector_sync_state.document_receive_stream, shutdown_event=_vector_sync_state.shutdown_event, scanner_wake_event=_vector_sync_state.scanner_wake_event, + eviction_task_group=_vector_sync_state.eviction_task_group, ) finally: logger.info("Shutting down BasicAuth session") @@ -1189,6 +1197,7 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = document_receive_stream=_vector_sync_state.document_receive_stream, shutdown_event=_vector_sync_state.shutdown_event, scanner_wake_event=_vector_sync_state.scanner_wake_event, + eviction_task_group=_vector_sync_state.eviction_task_group, ) finally: logger.info("Shutting down MCP server") @@ -1604,6 +1613,12 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = username, ) + # Expose this long-lived task group to request-path code that + # wants to spawn background work (e.g. ADR-019 verify-on-read + # eviction). Eviction coroutines have their own try/except, so + # they cannot panic the parent group. + _vector_sync_state.eviction_task_group = tg + logger.info( f"Background sync tasks started: 1 scanner + " f"{settings.vector_sync_processor_workers} processors" @@ -1617,6 +1632,8 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = # Shutdown signal logger.info("Shutting down background sync tasks") shutdown_event.set() + # Request path must not spawn into a cancelling group. + _vector_sync_state.eviction_task_group = None await client.close() # TaskGroup automatically cancels all tasks on exit @@ -1786,6 +1803,12 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = use_basic_auth, # Pass as positional arg (before task_status) ) + # Expose this long-lived task group to request-path code + # that wants to spawn background work (e.g. ADR-019 + # verify-on-read eviction). Eviction coroutines have their + # own try/except, so they cannot panic the parent group. + _vector_sync_state.eviction_task_group = tg + logger.info( f"Background sync tasks started: 1 user manager + " f"{settings.vector_sync_processor_workers} processors" @@ -1799,6 +1822,8 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = # Shutdown signal logger.info("Shutting down background sync tasks") shutdown_event.set() + # Request path must not spawn into a cancelling group. + _vector_sync_state.eviction_task_group = None # Close token broker HTTP client if token_broker._http_client: await token_broker._http_client.aclose() diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 7d5a8110..f0804751 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -35,6 +35,7 @@ from collections.abc import Awaitable, Callable from typing import Any import anyio +from anyio.abc import TaskGroup from httpx import HTTPStatusError from nextcloud_mcp_server.search.algorithms import SearchResult @@ -128,7 +129,11 @@ async def _verify_files( try: info = await client.webdav.get_file_info(file_path) if info is None: - # get_file_info returns None on definitive 404 + # Contract: WebDAVClient.get_file_info returns None on 404 + # and raises HTTPStatusError on 403/5xx/network. If that + # contract changes (e.g. a future refactor that raises 404 + # like other client methods), the `except HTTPStatusError` + # block below already handles it via _is_definitive_404_or_403. return accessible.add(doc_id) except HTTPStatusError as e: @@ -256,13 +261,27 @@ async def _verify_news_items( ) 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 (caller may pass ints or strs). - accessible: set[int | str] = set() - for d in doc_ids: - if int(d) in present_ids: - accessible.add(d) - return accessible + # Cast safely: a non-numeric id from the API or in our doc_ids would + # otherwise raise ValueError after the semaphore block exits and surface + # as a verifier crash. Treat as transient (fail open) instead. + try: + 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 (caller may pass ints or strs). + accessible: set[int | str] = set() + for d in doc_ids: + if int(d) in present_ids: + accessible.add(d) + return accessible + except (TypeError, ValueError) as e: + logger.warning( + "Non-numeric id while verifying news items (sample=%r, doc_ids=%r): %s; keeping all results", + items[:3] if items else items, + doc_ids, + e, + ) + return set(doc_ids) _VERIFIERS: dict[str, BatchVerifier] = { @@ -293,6 +312,7 @@ async def verify_search_results( *, evict_on_missing: bool = True, max_concurrent: int = DEFAULT_VERIFICATION_CONCURRENCY, + eviction_task_group: TaskGroup | None = None, ) -> list[SearchResult]: """Filter search results to those the user can currently access. @@ -301,9 +321,13 @@ async def verify_search_results( concurrently per doc_type and concurrently per id within each verifier, bounded by a shared semaphore (``max_concurrent``). - When ``evict_on_missing=True``, points for documents that fail - verification are deleted from Qdrant in-line. Eviction failures are - logged but never propagated. + When ``evict_on_missing=True``, points for documents that fail verification + are deleted from Qdrant. If ``eviction_task_group`` is provided (the + lifespan-owned task group from ``app.py::VectorSyncState``), eviction is + fire-and-forget — the search response returns immediately and Qdrant + deletes happen in the background. If no task group is provided (unit + tests, modes without vector sync), eviction falls back to running inline + in a local task group. Eviction failures are logged but never propagated. Args: client: Authenticated NextcloudClient (must expose ``username``). @@ -312,6 +336,10 @@ async def verify_search_results( evict_on_missing: Schedule lazy eviction for inaccessible docs. max_concurrent: Cap on concurrent verification round-trips against Nextcloud. Defaults to ``DEFAULT_VERIFICATION_CONCURRENCY``. + eviction_task_group: Optional long-lived task group on which to + spawn fire-and-forget eviction. Pass + ``ctx.request_context.lifespan_context.eviction_task_group`` + from FastMCP tools. Returns: Filtered list preserving the original order. @@ -385,10 +413,19 @@ async def verify_search_results( # list still contains all chunks). kept = [r for r in results if (r.id, r.doc_type) not in inaccessible] - # Lazy eviction. Runs inline before returning — slow Qdrant will delay - # the search response. Background eviction would need a task registered - # on the lifespan context; the inline approach is acceptable given - # typical Qdrant delete latency, but callers should be aware. + # Lazy eviction. + # + # Preferred path: spawn evict() on the lifespan-owned task group via + # `start_soon`, which returns immediately — the search response is not + # blocked on Qdrant deletes. If the server is shutting down, the task + # group is cleared back to None (see app.py) and we fall through to the + # inline path. Cancellation mid-eviction is fine: the next query will + # re-verify and re-attempt (self-healing per ADR-019). + # + # Fallback path: when no task group is supplied (unit tests, deployment + # modes without vector sync), run eviction inline in a local task group. + # This preserves prior behaviour for tests that rely on eviction being + # 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: @@ -399,8 +436,12 @@ async def verify_search_results( "Failed to evict %s_%s from Qdrant: %s", doc_type, doc_id, e ) - async with anyio.create_task_group() as tg: + if eviction_task_group is not None: for doc_id, doc_type in inaccessible: - tg.start_soon(evict, doc_id, doc_type) + eviction_task_group.start_soon(evict, doc_id, doc_type) + else: + 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 6b804494..7eefbd5d 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -156,8 +156,17 @@ def configure_semantic_tools(mcp: FastMCP): # 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) + # are about to be dropped. Pass the lifespan-owned task group so + # eviction of dropped points is fire-and-forget (does not block + # the response). + eviction_task_group = getattr( + ctx.request_context.lifespan_context, "eviction_task_group", None + ) + verified_results = await verify_search_results( + client, + all_results, + eviction_task_group=eviction_task_group, + ) search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index d57961f2..6973fb9f 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -29,6 +29,16 @@ from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) +# Single source of truth for which doc_types this scanner indexes. The verifier +# registry in `search/verification.py` must cover every type listed here +# (enforced by `tests/unit/search/test_verification.py`). Add a verifier in the +# same PR that adds a new indexed doc_type, or accept ghost-record exposure for +# that type (see ADR-019). +INDEXED_DOC_TYPES: frozenset[str] = frozenset( + {"note", "file", "deck_card", "news_item"} +) + + @dataclass class DocumentTask: """Document task for processing queue.""" diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 21becc4d..25727131 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -17,6 +17,7 @@ from nextcloud_mcp_server.search.verification import ( get_supported_doc_types, verify_search_results, ) +from nextcloud_mcp_server.vector.scanner import INDEXED_DOC_TYPES # --------------------------------------------------------------------------- # Helpers @@ -58,13 +59,13 @@ def _http_error(status_code: int) -> HTTPStatusError: @pytest.mark.unit def test_supported_doc_types_covers_indexed_types(): - """ADR-019 implementation checklist: every indexed doc_type has a verifier. + """ADR-019 CI guard: every doc_type indexed by the scanner has a verifier. - Indexed types are defined in vector/scanner.py and vector/processor.py: - note, file, deck_card, news_item. + `INDEXED_DOC_TYPES` is the single source of truth in `vector/scanner.py`; + this test fails if a new indexed type is added without a registered + verifier in `search/verification.py`. """ - expected = {"note", "file", "deck_card", "news_item"} - assert get_supported_doc_types() >= expected + assert get_supported_doc_types() >= INDEXED_DOC_TYPES # --------------------------------------------------------------------------- @@ -423,6 +424,7 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker): @pytest.mark.unit async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): + """Inline-fallback path (no eviction_task_group): evict completes before return.""" spy_evict = mocker.AsyncMock() mocker.patch.object(verification, "delete_document_points", spy_evict) @@ -442,6 +444,49 @@ async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): spy_evict.assert_awaited_once_with(99, "note", "alice") +@pytest.mark.unit +async def test_verify_search_results_fire_and_forget_eviction(mocker): + """When eviction_task_group is provided, eviction does not block the response. + + Validates the ADR-019 design: spawn evict() on the lifespan-owned task + group via start_soon so the search response returns immediately. The + eviction still runs (verified after the task group exits). + """ + eviction_started = anyio.Event() + eviction_may_complete = anyio.Event() + eviction_completed = anyio.Event() + + async def slow_delete(doc_id, doc_type, user_id): + eviction_started.set() + await eviction_may_complete.wait() + eviction_completed.set() + + mocker.patch.object( + verification, + "delete_document_points", + mocker.AsyncMock(side_effect=slow_delete), + ) + + note_verifier = mocker.AsyncMock(return_value=set()) # both inaccessible + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + results = [_make_result(99, doc_type="note")] + client = SimpleNamespace(username="alice") + + async with anyio.create_task_group() as tg: + kept = await verify_search_results(client, results, eviction_task_group=tg) + # 1. Search response was returned … + assert kept == [] + # 2. … even though eviction has started but not finished. + await eviction_started.wait() + assert not eviction_completed.is_set() + # 3. Now allow eviction to complete; the task group exit awaits it. + eviction_may_complete.set() + + # After the task group exits, the eviction must have run. + assert eviction_completed.is_set() + + @pytest.mark.unit async def test_verify_search_results_no_eviction_when_disabled(mocker): spy_evict = mocker.AsyncMock() From aa4b9498a1799c34e68c61221f384ea6552b8760 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 19:17:35 +0200 Subject: [PATCH 04/16] refactor(search): address PR #750 round 3 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _verify_deck_cards: hoist int(board_id|stack_id|doc_id) out of the generic except Exception into an explicit try/except (TypeError, ValueError) before the network call, mirroring _verify_news_items. Malformed payloads now log a specific warning instead of "unexpected error". - _verify_news_items: add TODO(perf) above the get_items(batch_size=-1) call to mark the known fetch-all cost as a future profiling target. - SemanticSearchResult.id: revert from int|str back to int. The internal SearchResult.id stays int|str for forward-compat; the MCP response model narrows at the boundary. server/semantic.py casts r.id to int when constructing the response so future string-id types fail loudly here instead of silently widening the public API. - nc_semantic_search: replace the terse "extra for access filtering" comment with an ADR-019 NOTE block explaining the 2x over-fetch trade-off and the ghost-density under-delivery case (self-heals via lazy eviction). - tests/integration/test_verify_on_read.py: extend the module docstring to call out that only the note verifier is exercised against real Nextcloud, while file/deck_card/news_item are unit-only — documenting the suite split for future contributors. - ADR-019: rewrite "Module shape", "Verifier registry", example verifier, and "Deduplication" sections to match the shipped BatchVerifier interface (was per-id Verifier in the original draft). Add a "Why batch?" paragraph explaining the design choice. Update implementation checklist — every item is now [x] with corrected verifier names (plural) and the eviction module path (vector/eviction.py). Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 122 ++++++++++++------ nextcloud_mcp_server/models/semantic.py | 8 +- nextcloud_mcp_server/search/verification.py | 30 ++++- nextcloud_mcp_server/server/semantic.py | 26 +++- tests/integration/test_verify_on_read.py | 11 ++ 5 files changed, 150 insertions(+), 47 deletions(-) diff --git a/docs/ADR-019-verify-on-read-for-semantic-search.md b/docs/ADR-019-verify-on-read-for-semantic-search.md index 87848745..61129ffa 100644 --- a/docs/ADR-019-verify-on-read-for-semantic-search.md +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -73,16 +73,24 @@ The vector index becomes a **hint**, not a contract. We never trust it for acces ```python # nextcloud_mcp_server/search/verification.py -from typing import Awaitable, Callable, Protocol +from typing import Awaitable, Callable 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]] +# A batch verifier takes a list of results for a single doc_type and returns +# the set of doc_ids that are currently accessible to the user. The shared +# semaphore caps concurrent Nextcloud round-trips across all verifier types. +# +# - Definitive 403/404 → omit the id from the returned set (drop the result). +# - Transient error (5xx, network, parse) → include the id (fail-open keep). +# - Verifier crash → caught by the dispatcher and treated as transient +# (all results for that type are kept; logged distinctly). +BatchVerifier = Callable[ + ["NextcloudClientProtocol", list[SearchResult], anyio.Semaphore], + Awaitable[set[int | str]], +] async def verify_search_results( @@ -91,56 +99,98 @@ async def verify_search_results( *, max_concurrent: int = 20, evict_on_missing: bool = True, + eviction_task_group: anyio.abc.TaskGroup | None = None, ) -> 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). + from the same document cost a single check. Each verifier owns its own + concurrency under the shared semaphore. Drops results whose verifier + omitted them; keeps results whose verifier raised or whose doc_type has + no registered verifier (transient failure should not silently shrink + results). 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. + Pass ``eviction_task_group`` (typically the lifespan-owned background task + group) to make eviction fire-and-forget; without it we run a local task + group that blocks the response until evictions complete. """ ``` +**Why batch?** A per-id `Verifier` would force one task-group creation per id, multiply the number of small tasks, and prevent the news single-fetch optimization (the News API has no per-item endpoint, so per-id verification would be O(N × all_items)). The batch interface lets each verifier own its own concurrency strategy: notes/files/deck cards parallelize per id under the shared semaphore; news fetches once and intersects. + ### Verifier registry ```python -_VERIFIERS: dict[str, Verifier] = { - "note": _verify_note, - "news_item": _verify_news_item, - "file": _verify_file, - "deck_card": _verify_deck_card, +_VERIFIERS: dict[str, BatchVerifier] = { + "note": _verify_notes, + "news_item": _verify_news_items, + "file": _verify_files, + "deck_card": _verify_deck_cards, } ``` -Each verifier follows the same pattern: +Each verifier follows the same shape — accept a list of results for its type, fan out per-id under the shared semaphore (or fetch once and intersect, for news), and return the set of accessible ids: ```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 +async def _verify_notes( + client, + results: list[SearchResult], + semaphore: anyio.Semaphore, +) -> set[int | str]: + accessible: set[int | str] = set() + + async def check(result: SearchResult) -> None: + async with semaphore: + try: + await client.notes.get_note(int(result.id)) + accessible.add(result.id) + except httpx.HTTPStatusError as e: + if e.response.status_code in (403, 404): + return # definitive — drop + accessible.add(result.id) # transient — keep + + async with anyio.create_task_group() as tg: + for r in results: + tg.start_soon(check, r) + return accessible ``` -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. +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. For `news_item`, batch-fetch the user's items once via `get_items(batch_size=-1)` and intersect, since the News API has no per-item endpoint. ### 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: +A 10-result page typically references 3–4 unique documents because of chunking. Dedupe by `(doc_id, doc_type)` *before* invoking the verifiers, so each batch verifier sees only unique ids. The dispatcher then propagates each id's verdict to every chunk 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)] +unique: list[SearchResult] = [] +seen: set[tuple[int | str, str]] = set() +for r in results: + key = (r.id, r.doc_type) + if key not in seen: + seen.add(key) + unique.append(r) + +# Group unique results by doc_type and run their batch verifiers in parallel. +by_type: dict[str, list[SearchResult]] = group_by_doc_type(unique) +accessible_by_type: dict[str, set[int | str]] = {} +async with anyio.create_task_group() as tg: + for dtype, items in by_type.items(): + verifier = _VERIFIERS.get(dtype) + if verifier is None: + # Soft failure: keep all results for unknown doc_types. + accessible_by_type[dtype] = {r.id for r in items} + continue + tg.start_soon(_run_verifier, verifier, dtype, items, accessible_by_type) + +return [ + r for r in results + if r.id in accessible_by_type.get(r.doc_type, set()) +] ``` -A failed verification (raised exception) maps to "keep" — we do not want a flaky network blip to silently shrink results. +A verifier crash maps to "keep all" for that type — we do not want a flaky network blip to silently shrink results. ### Lazy eviction @@ -217,13 +267,13 @@ In `server/semantic.py::nc_semantic_search_answer`, replace the per-type `if res ## 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. +- [x] Create `nextcloud_mcp_server/search/verification.py` with `verify_search_results()` and the verifier registry. +- [x] Implement `_verify_notes`, `_verify_news_items`, `_verify_files` (PROPFIND), `_verify_deck_cards` (metadata fast-path only). Names plural to reflect the batch-verifier interface (see "Module shape" above). +- [x] Add `delete_document_points()` in `nextcloud_mcp_server/vector/eviction.py` for non-placeholder filter-based deletes. +- [x] Wire into `nc_semantic_search` with `limit * 2` over-fetch, trim to `limit` after verification. +- [x] Wire into `nc_semantic_search_answer`; verification runs upstream in `nc_semantic_search`, and the note-only re-fetch is retained as a sub-second race guard. +- [x] Update existing docstrings in `search/semantic.py` and `search/bm25_hybrid.py` to reflect the new verify-on-read path. +- [x] Unit tests: each verifier handles 200/403/404/transient distinctly; dedup collapses chunks; eviction is scheduled on missing. +- [x] Integration test: index a note, delete via API (no webhook), confirm the next semantic search does not return it. (See `tests/integration/test_verify_on_read.py`. Coverage gap for `file`, `deck_card`, `news_item` integration tests is tracked as a follow-up.) - [x] CI guard: enumerate indexed doc_types in `vector/scanner.py` and assert each has a registered verifier. (`INDEXED_DOC_TYPES` in `vector/scanner.py`; `tests/unit/search/test_verification.py::test_supported_doc_types_covers_indexed_types`.) - [x] Document the latency budget and rate-limit posture in `docs/configuration.md`. (See "Verify-on-Read Latency Budget" section.) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 652f921b..83875ddf 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -10,11 +10,13 @@ from .base import BaseResponse class SemanticSearchResult(BaseModel): """Model for semantic search results with additional metadata.""" - id: int | str = Field( + id: int = Field( description=( "Document ID. Numeric for all currently indexed types (notes, files, " - "deck cards, news items); typed as int|str to allow future doc types " - "that use string identifiers." + "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." ) ) doc_type: str = Field( diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index f0804751..64051f1b 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -189,12 +189,31 @@ async def _verify_deck_cards( accessible.add(doc_id) return + # Parse defensively before the network call so a malformed payload + # produces a specific log line, not a generic "unexpected error" from + # the catch-all ``except Exception`` below. Mirrors ``_verify_news_items``. + try: + board_id_int = int(board_id) + stack_id_int = int(stack_id) + card_id_int = int(doc_id) + except (TypeError, ValueError) as e: + logger.warning( + "Non-numeric deck metadata for card %s " + "(board_id=%r, stack_id=%r): %s; keeping result", + doc_id, + board_id, + stack_id, + e, + ) + accessible.add(doc_id) + return + async with semaphore: try: await client.deck.get_card( - board_id=int(board_id), - stack_id=int(stack_id), - card_id=int(doc_id), + board_id=board_id_int, + stack_id=stack_id_int, + card_id=card_id_int, ) accessible.add(doc_id) except HTTPStatusError as e: @@ -236,6 +255,11 @@ async def _verify_news_items( async with semaphore: try: + # TODO(perf): if profiling shows this fetch dominates query latency + # for news-heavy users, cache the per-request item set or push for + # a per-item News API endpoint. The shared semaphore protects + # against runaway concurrent fetches, but the payload itself can + # be large (News auto-purge cap is in the thousands of items). 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), diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 7eefbd5d..a4023549 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -120,11 +120,20 @@ def configure_semantic_tools(mcp: FastMCP): if doc_types is None: # Cross-app search: search all indexed types - # Get unverified results from Qdrant + # Get unverified results from Qdrant. + # + # NOTE (ADR-019): Over-fetch by 2× to absorb ghost-record drops + # during verify-on-read. When ghost density is high (e.g. a + # large board share was just revoked) this budget can still + # under-deliver against the requested ``limit``; the index + # self-heals via lazy eviction so subsequent searches recover. + # The 2× factor is a deliberate v1 trade-off — raising it + # costs Nextcloud round-trips on every search. Trim to + # ``limit`` happens AFTER verification. unverified_results = await search_algo.search( query=query, user_id=username, - limit=limit * 2, # Get extra for access filtering + limit=limit * 2, doc_type=None, # Signal to search all types score_threshold=score_threshold, ) @@ -132,11 +141,12 @@ def configure_semantic_tools(mcp: FastMCP): else: # Search specific document types # For each requested type, execute search and combine results + # under the same 2× over-fetch budget (see NOTE above). for dtype in doc_types: unverified_results = await search_algo.search( query=query, user_id=username, - limit=limit * 2, # Get extra for combining and filtering + limit=limit * 2, doc_type=dtype, score_threshold=score_threshold, ) @@ -169,12 +179,18 @@ def configure_semantic_tools(mcp: FastMCP): ) search_results = verified_results[:limit] - # Convert SearchResult objects to SemanticSearchResult for response + # 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. results = [] for r in search_results: results.append( SemanticSearchResult( - id=r.id, + id=int(r.id), doc_type=r.doc_type, title=r.title, category=r.metadata.get("category", "") if r.metadata else "", diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py index 3359c7d4..eabea602 100644 --- a/tests/integration/test_verify_on_read.py +++ b/tests/integration/test_verify_on_read.py @@ -5,6 +5,17 @@ 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. +**Coverage**: only the ``note`` verifier is exercised against real Nextcloud +here. The ``file`` (WebDAV PROPFIND), ``deck_card`` (Deck app), and +``news_item`` (News app) verifiers are unit-tested with mocked HTTP +responses in ``tests/unit/search/test_verification.py``. Adding integration +coverage for those types is tracked as a follow-up — it requires fixture +data (tagged PDFs in user files, a Deck board with cards, a News feed) that +is non-trivial to seed from CI. The mocked unit tests are accurate for +status-code semantics but won't catch payload-shape regressions in those +Nextcloud apps; the trade-off is documented here so future readers know +which suite owns which verifier. + 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 From 926722b09d9b37911e7dcd476169501a30c4d5f8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 19:28:55 +0200 Subject: [PATCH 05/16] refactor(search): address PR #750 round 4 review feedback - Guard eviction_task_group.start_soon against shutdown race so a RuntimeError on a closed group never surfaces as a search error. - Correct ADR-019 news_item row: there is no per-item REST endpoint; verification batches via get_items(batch_size=-1) and intersects. - Modernize models/semantic.py typing to PEP 604 / lowercase generics per CLAUDE.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 2 +- nextcloud_mcp_server/models/semantic.py | 28 +++++++++---------- nextcloud_mcp_server/search/verification.py | 10 ++++++- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/docs/ADR-019-verify-on-read-for-semantic-search.md b/docs/ADR-019-verify-on-read-for-semantic-search.md index 61129ffa..926837ed 100644 --- a/docs/ADR-019-verify-on-read-for-semantic-search.md +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -49,7 +49,7 @@ The vector pipeline (`vector/scanner.py`, `vector/processor.py`) currently index | 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. | +| `news_item` | `get_items(batch_size=-1)` once per search + intersect | No per-item REST endpoint (`get_item()` is itself a fetch-all + filter); batching once is cheaper than N fetch-all-and-filter calls. | | `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. | diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 83875ddf..aafa2ab3 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -1,7 +1,5 @@ """Pydantic models for semantic search responses.""" -from typing import List, Optional - from pydantic import BaseModel, Field from .base import BaseResponse @@ -37,36 +35,36 @@ class SemanticSearchResult(BaseModel): ) chunk_index: int = Field(description="Index of matching chunk in document") total_chunks: int = Field(description="Total number of chunks in document") - chunk_start_offset: Optional[int] = Field( + chunk_start_offset: int | None = Field( default=None, description="Character position where chunk starts in document" ) - chunk_end_offset: Optional[int] = Field( + chunk_end_offset: int | None = Field( default=None, description="Character position where chunk ends in document" ) - page_number: Optional[int] = Field( + page_number: int | None = Field( default=None, description="Page number for PDF documents" ) - page_count: Optional[int] = Field( + page_count: int | None = Field( default=None, description="Total number of pages in PDF document" ) # Context expansion fields (optional, populated when include_context=True) has_context_expansion: bool = Field( default=False, description="Whether context expansion was performed" ) - marked_text: Optional[str] = Field( + marked_text: str | None = Field( default=None, description="Full text with position markers around matched chunk", ) - before_context: Optional[str] = Field( + before_context: str | None = Field( default=None, description="Text before the matched chunk" ) - after_context: Optional[str] = Field( + after_context: str | None = Field( default=None, description="Text after the matched chunk" ) - has_before_truncation: Optional[bool] = Field( + has_before_truncation: bool | None = Field( default=None, description="Whether before_context was truncated" ) - has_after_truncation: Optional[bool] = Field( + has_after_truncation: bool | None = Field( default=None, description="Whether after_context was truncated" ) @@ -74,7 +72,7 @@ class SemanticSearchResult(BaseModel): class SemanticSearchResponse(BaseResponse): """Response model for semantic search across all indexed Nextcloud apps.""" - results: List[SemanticSearchResult] = Field( + results: list[SemanticSearchResult] = Field( description="Semantic search results with similarity scores" ) query: str = Field(description="The search query used") @@ -106,7 +104,7 @@ class SamplingSearchResponse(BaseResponse): generated_answer: str = Field( ..., description="LLM-generated answer based on retrieved documents" ) - sources: List[SemanticSearchResult] = Field( + sources: list[SemanticSearchResult] = Field( default_factory=list, description="Source documents with excerpts and relevance scores", ) @@ -114,10 +112,10 @@ class SamplingSearchResponse(BaseResponse): search_method: str = Field( default="semantic_sampling", description="Search method used" ) - model_used: Optional[str] = Field( + model_used: str | None = Field( default=None, description="Model that generated the answer" ) - stop_reason: Optional[str] = Field( + stop_reason: str | None = Field( default=None, description="Reason generation stopped" ) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 64051f1b..fb352222 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -462,7 +462,15 @@ async def verify_search_results( if eviction_task_group is not None: for doc_id, doc_type in inaccessible: - eviction_task_group.start_soon(evict, doc_id, doc_type) + # Guard against the lifespan task group having exited between + # the getattr() capture in server/semantic.py and this call — + # start_soon raises RuntimeError on a closed group, which + # would otherwise surface as a search error. Eviction is + # best-effort: the next query re-verifies and re-attempts. + try: + eviction_task_group.start_soon(evict, doc_id, doc_type) + except Exception: + logger.debug("Eviction task group closed; will retry on next query") else: async with anyio.create_task_group() as tg: for doc_id, doc_type in inaccessible: From ffcca23a7b3db1f8026dded69b96503fa6d8780a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 20:45:56 +0200 Subject: [PATCH 06/16] refactor(search): address PR #750 round 5 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tightens verifier consistency, closes test gaps, hardens the fire-and-forget eviction snapshot, and routes the new concurrency knob through Settings. - Pre-flight ``int()`` guard in ``_verify_notes`` mirrors ``_verify_deck_cards``, so a non-numeric note id produces a type-specific log line instead of falling through to the generic "unexpected error" branch. - Adds explicit 403 tests for the file and news verifiers (symmetry with the existing notes/deck 403 tests) plus a ``non_numeric_id_keeps`` test. - ``AppContext`` and ``OAuthAppContext`` no longer snapshot ``_vector_sync_state.eviction_task_group`` at lifespan-yield time. Both expose it as a ``@property`` that reads the singleton dynamically, removing the order-sensitive race where a future startup-ordering change could silently degrade fire-and-forget eviction to inline forever. - Adds ``verification_concurrency`` (env var ``VERIFICATION_CONCURRENCY``, default 20) to ``Settings`` with a dynaconf validator; ``verify_search_results`` resolves the cap lazily from settings when the caller doesn't override it. - Enriches the news verifier TODO to call out that ``batch_size=-1`` is intentional — a numeric ceiling would silently break correctness because any item beyond the cap would be missing from ``present_ids`` and dropped. - Updates ``Optional[TaskGroup]`` to ``TaskGroup | None`` per project style. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 9 ++-- nextcloud_mcp_server/app.py | 24 +++++++-- nextcloud_mcp_server/config.py | 11 ++++ nextcloud_mcp_server/search/verification.py | 41 +++++++++++---- tests/unit/search/test_verification.py | 56 +++++++++++++++++++++ 5 files changed, 123 insertions(+), 18 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index b16443ec..6438f819 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -489,10 +489,11 @@ aware of: unique documents, so verification adds 3-5 round-trips. With the default 20-way concurrency this is one parallel batch — usually under 100 ms on a healthy connection. -- **Concurrency**: all verifications fan out under a shared semaphore - (`DEFAULT_VERIFICATION_CONCURRENCY = 20` in `search/verification.py`). The - limit is not currently exposed as an env var; if production workloads - saturate Nextcloud, consider opening an issue to make it tunable. +- **Concurrency**: all verifications fan out under a shared semaphore. + Tunable via the `VERIFICATION_CONCURRENCY` env var (settings field + `verification_concurrency`, default 20) — lower it if your Nextcloud + backend struggles with the parallel fan-out, or raise it on a healthy + connection to speed up large result pages. - **News API caveat**: the News app has no per-item endpoint, so the news verifier issues a single `news.get_items(batch_size=-1, get_read=True)` call per search that contains any news result, then intersects locally. The diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 86a93565..5ef8a47f 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -323,7 +323,7 @@ class VectorSyncState: # Long-lived task group used for fire-and-forget background work spawned # from the request path (e.g. ADR-019 verify-on-read eviction). Set by the # starlette lifespan after entering its task group; cleared on shutdown. - eviction_task_group: Optional[TaskGroup] = None + eviction_task_group: TaskGroup | None = None # Module-level singleton for vector sync state @@ -340,7 +340,15 @@ class AppContext: document_receive_stream: Optional[MemoryObjectReceiveStream] = None shutdown_event: Optional[anyio.Event] = None scanner_wake_event: Optional[anyio.Event] = None - eviction_task_group: Optional[TaskGroup] = None + + @property + def eviction_task_group(self) -> TaskGroup | None: + # Read dynamically from the module-level singleton instead of + # snapshotting at lifespan-yield time. Snapshotting is order-sensitive: + # if the FastMCP server lifespan ever runs before the Starlette + # lifespan assigns the task group, every session for the life of the + # process would see ``None`` and fall back to inline eviction. + return _vector_sync_state.eviction_task_group @dataclass @@ -359,7 +367,11 @@ class OAuthAppContext: document_receive_stream: Optional[MemoryObjectReceiveStream] = None shutdown_event: Optional[anyio.Event] = None scanner_wake_event: Optional[anyio.Event] = None - eviction_task_group: Optional[TaskGroup] = None + + @property + def eviction_task_group(self) -> TaskGroup | None: + # See AppContext.eviction_task_group for rationale. + return _vector_sync_state.eviction_task_group class BasicAuthMiddleware: @@ -576,7 +588,8 @@ async def app_lifespan_basic(server: FastMCP) -> AsyncIterator[AppContext]: document_receive_stream=_vector_sync_state.document_receive_stream, shutdown_event=_vector_sync_state.shutdown_event, scanner_wake_event=_vector_sync_state.scanner_wake_event, - eviction_task_group=_vector_sync_state.eviction_task_group, + # eviction_task_group is exposed via @property (reads + # _vector_sync_state at access time, not snapshot). ) finally: logger.info("Shutting down BasicAuth session") @@ -1197,7 +1210,8 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = document_receive_stream=_vector_sync_state.document_receive_stream, shutdown_event=_vector_sync_state.shutdown_event, scanner_wake_event=_vector_sync_state.scanner_wake_event, - eviction_task_group=_vector_sync_state.eviction_task_group, + # eviction_task_group is exposed via @property (reads + # _vector_sync_state at access time, not snapshot). ) finally: logger.info("Shutting down MCP server") diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 13a829fe..eedc254c 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -65,6 +65,8 @@ _DEFAULTS: dict[str, Any] = { "vector_sync_processor_workers": 3, "vector_sync_queue_max_size": 10000, "vector_sync_user_poll_interval": 60, + # Verify-on-read concurrency cap (ADR-019) + "verification_concurrency": 20, # Qdrant "qdrant_url": None, "qdrant_location": None, @@ -170,6 +172,7 @@ _dynaconf = Dynaconf( Validator("VECTOR_SYNC_PROCESSOR_WORKERS", gte=1), Validator("VECTOR_SYNC_QUEUE_MAX_SIZE", gte=1), Validator("VECTOR_SYNC_USER_POLL_INTERVAL", gte=1), + Validator("VERIFICATION_CONCURRENCY", gte=1), Validator("DOCUMENT_CHUNK_SIZE", gte=1), # Non-negative Validator("DOCUMENT_CHUNK_OVERLAP", gte=0), @@ -455,6 +458,12 @@ class Settings: vector_sync_queue_max_size: int = 10000 vector_sync_user_poll_interval: int = 60 # seconds - OAuth mode user discovery + # Verify-on-read concurrency (ADR-019). Cap on parallel Nextcloud + # round-trips during search-result verification fan-out. Lower this if the + # Nextcloud backend struggles with the parallel load; raise it on a + # healthy connection to speed up large result pages. + verification_concurrency: int = 20 + # Qdrant settings (mutually exclusive modes) qdrant_url: str | None = None # Network mode: http://qdrant:6333 qdrant_location: str | None = None # Local mode: :memory: or /path/to/data @@ -793,6 +802,8 @@ def get_settings() -> Settings: "vector_sync_processor_workers": "VECTOR_SYNC_PROCESSOR_WORKERS", "vector_sync_queue_max_size": "VECTOR_SYNC_QUEUE_MAX_SIZE", "vector_sync_user_poll_interval": "VECTOR_SYNC_USER_POLL_INTERVAL", + # Verify-on-read (ADR-019) + "verification_concurrency": "VERIFICATION_CONCURRENCY", # Qdrant settings "qdrant_url": "QDRANT_URL", "qdrant_location": "QDRANT_LOCATION", diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index fb352222..581e8dcf 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -38,17 +38,13 @@ import anyio from anyio.abc import TaskGroup from httpx import HTTPStatusError +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 logger = logging.getLogger(__name__) -# Default cap on concurrent verification round-trips against Nextcloud. Matches -# the convention in ``server/semantic.py`` for context-expansion fan-out. -DEFAULT_VERIFICATION_CONCURRENCY = 20 - - BatchVerifier = Callable[ [Any, list[SearchResult], anyio.Semaphore], Awaitable[set[int | str]] ] @@ -73,10 +69,24 @@ async def _verify_notes( accessible: set[int | str] = set() async def check(result: SearchResult) -> None: + doc_id = result.id + # Parse defensively before the network call so a malformed payload + # produces a specific log line, not a generic "unexpected error" from + # the catch-all ``except Exception`` below. Mirrors ``_verify_deck_cards``. + try: + note_id_int = int(doc_id) + except (TypeError, ValueError) as e: + logger.warning( + "Non-numeric note id %r: %s; keeping result", + doc_id, + e, + ) + accessible.add(doc_id) + return + async with semaphore: - doc_id = result.id try: - await client.notes.get_note(int(doc_id)) + await client.notes.get_note(note_id_int) accessible.add(doc_id) except HTTPStatusError as e: if _is_definitive_404_or_403(e): @@ -260,6 +270,14 @@ async def _verify_news_items( # a per-item News API endpoint. The shared semaphore protects # against runaway concurrent fetches, but the payload itself can # be large (News auto-purge cap is in the thousands of items). + # + # NOTE: ``batch_size`` is intentionally unbounded (-1). A numeric + # ceiling here would silently *break correctness*: any item beyond + # the cap would be missing from ``present_ids`` and incorrectly + # dropped from the result set. The fail-open contract requires + # fetching every item the user has access to. See the news caveat + # in docs/configuration.md (Verify-on-Read) for the latency + # tradeoff and follow-up paths. 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), @@ -335,7 +353,7 @@ async def verify_search_results( results: list[SearchResult], *, evict_on_missing: bool = True, - max_concurrent: int = DEFAULT_VERIFICATION_CONCURRENCY, + max_concurrent: int | None = None, eviction_task_group: TaskGroup | None = None, ) -> list[SearchResult]: """Filter search results to those the user can currently access. @@ -359,7 +377,9 @@ async def verify_search_results( multiple chunks per document). evict_on_missing: Schedule lazy eviction for inaccessible docs. max_concurrent: Cap on concurrent verification round-trips against - Nextcloud. Defaults to ``DEFAULT_VERIFICATION_CONCURRENCY``. + Nextcloud. When ``None`` (the default), resolved from + ``Settings.verification_concurrency`` (env var + ``VERIFICATION_CONCURRENCY``, default 20). eviction_task_group: Optional long-lived task group on which to spawn fire-and-forget eviction. Pass ``ctx.request_context.lifespan_context.eviction_task_group`` @@ -373,6 +393,9 @@ async def verify_search_results( user_id: str = client.username + if max_concurrent is None: + max_concurrent = get_settings().verification_concurrency + # Group unique (doc_id, doc_type) by doc_type so each verifier sees a # deduplicated batch. We pick one SearchResult per (id, doc_type) to carry # metadata (path, board_id/stack_id) into the verifier — chunks of the diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 25727131..916270ca 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -137,6 +137,24 @@ async def test_verify_notes_unexpected_exception_keeps(mocker): assert result == {7} +@pytest.mark.unit +async def test_verify_notes_non_numeric_id_keeps(mocker): + """Non-numeric note id must not surface as a generic 'unexpected error'. + + The defensive int() guard runs before the network call and produces a + type-specific log line; result is kept (fail-open). + """ + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=AssertionError("must not be called")) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [_make_result("not-a-number")], _sem()) + + assert result == {"not-a-number"} + notes_client.get_note.assert_not_awaited() + + @pytest.mark.unit async def test_verify_notes_mixed_outcomes(mocker): """Mix of accessible, deleted, and transient — only deleted is dropped.""" @@ -207,6 +225,27 @@ async def test_verify_news_items_api_404_drops_all(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_news_items_api_403_drops_all(mocker): + """News API 403 (e.g. user lost access to the app) drops all items.""" + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(side_effect=_http_error(403)) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items( + client, + [ + _make_result(1, doc_type="news_item"), + _make_result(2, doc_type="news_item"), + _make_result(3, doc_type="news_item"), + ], + _sem(), + ) + + assert result == set() + + @pytest.mark.unit async def test_verify_news_items_transient_keeps_all(mocker): news_client = SimpleNamespace( @@ -265,6 +304,23 @@ async def test_verify_files_404_via_get_file_info_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_files_403_drops(mocker): + """get_file_info raising HTTPStatusError(403) is a definitive drop.""" + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=_http_error(403)) + ) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(124, doc_type="file", metadata={"path": "forbidden.txt"})], + _sem(), + ) + + assert result == set() + + @pytest.mark.unit async def test_verify_files_missing_path_metadata_keeps_unverified(mocker): """Without a path in metadata we cannot verify — fail open, don't drop.""" From e8df6003c5c4fc7f66a7962751b74ae84ad115b8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:02:27 +0200 Subject: [PATCH 07/16] refactor(search): address PR #750 round 6 review feedback Closes out the remaining nits flagged in the round-6 review. Critical: - _verify_files contract comment now enumerates all None-return cases (404 + malformed PROPFIND XML) and documents the false-eviction trade-off; self-healing via re-indexing recovers - int(r.id) cast at the SemanticSearchResult boundary now raises a TypeError with explicit doc_type/value context instead of bubbling up as an opaque "Search failed: ..." McpError Design observations: - nc_semantic_search_answer docstring documents the per-note round-trip cost from the post-verification race guard - News verification latency hint added to configuration.md - SemanticSearchResponse exposes verified_count + dropped_count so short result pages on high-ghost-density indexes are distinguishable from genuine scarcity. verify_search_results now returns (kept, dropped_count); production caller and tests updated Minor: - Comment clarifies the .get() fallback in verify_search_results is defensive only (run_verifier always populates the entry) - Eviction task-group guard narrowed from except Exception to except RuntimeError (the only documented failure mode of TaskGroup.start_soon on a closed group) - Indexer logs a warning when a deck_card task is missing board_id/stack_id, surfacing data-quality issues at index time rather than at verification time - New unit test covers the news verifier's non-numeric-id fail-open path (one bad doc_id keeps the entire batch) Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 9 ++- nextcloud_mcp_server/models/semantic.py | 16 ++++++ nextcloud_mcp_server/search/verification.py | 40 +++++++++---- nextcloud_mcp_server/server/semantic.py | 30 +++++++++- nextcloud_mcp_server/vector/processor.py | 17 ++++++ tests/integration/test_verify_on_read.py | 14 +++-- tests/unit/search/test_verification.py | 62 +++++++++++++++++---- 7 files changed, 159 insertions(+), 29 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 6438f819..1b4354c4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -498,9 +498,12 @@ aware of: verifier issues a single `news.get_items(batch_size=-1, get_read=True)` call per search that contains any news result, then intersects locally. The payload is **unbounded** — for users with very large feed backlogs this can - dominate verification latency. Disabling News in the indexer or running with - a smaller backlog mitigates this; per-item paginated verification is tracked - as a future improvement. + dominate verification latency. As a rough guide on a healthy LAN connection: + a typical purged backlog (1k–5k items) returns in ~200–500 ms; very large + backlogs (>20k items) can exceed 2 s and become the dominant cost of any + search that surfaces news results. Disabling News in the indexer or running + with a smaller backlog mitigates this; per-item paginated verification is + tracked as a future improvement. - **Eviction**: when verification finds a definitive miss (404 / 403), the corresponding Qdrant points are deleted in the background on a lifespan-owned task group — fire-and-forget, does **not** block the search response. diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index aafa2ab3..8c882bdb 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -80,6 +80,22 @@ class SemanticSearchResponse(BaseResponse): search_method: str = Field( default="semantic", description="Search method used (semantic or hybrid)" ) + verified_count: int = Field( + default=0, + description=( + "Number of unique documents that passed verify-on-read access " + "checks (ADR-019). Equals len(results) before trimming to limit." + ), + ) + dropped_count: int = Field( + default=0, + description=( + "Number of unique documents dropped as ghost records during " + "verify-on-read (ADR-019). A short result page (len(results) < " + "limit) combined with a non-zero dropped_count indicates ghost " + "density rather than scarcity of relevant content." + ), + ) class SamplingSearchResponse(BaseResponse): diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 581e8dcf..af00bd1c 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -139,11 +139,25 @@ async def _verify_files( try: info = await client.webdav.get_file_info(file_path) if info is None: - # Contract: WebDAVClient.get_file_info returns None on 404 - # and raises HTTPStatusError on 403/5xx/network. If that - # contract changes (e.g. a future refactor that raises 404 - # like other client methods), the `except HTTPStatusError` - # block below already handles it via _is_definitive_404_or_403. + # Contract: WebDAVClient.get_file_info returns None in two + # cases — (1) HTTP 404, and (2) a malformed PROPFIND XML + # response (missing , , or + # — see client/webdav.py). Both are treated as + # "inaccessible" and trigger eviction. + # + # Trade-off: a malformed response from a brittle backend + # could cause a *false* eviction. We accept that risk in + # exchange for correctness on real 404s — the index + # self-heals via re-indexing on the next scan, and + # malformed responses are exceedingly rare in practice. + # Distinguishing the two cases would require widening + # get_file_info's return contract; deferred to a future + # change if false evictions become observable. + # + # If the contract ever changes (e.g. 404 raises + # HTTPStatusError like other client methods), the + # `except HTTPStatusError` block below already handles + # it via _is_definitive_404_or_403. return accessible.add(doc_id) except HTTPStatusError as e: @@ -355,7 +369,7 @@ async def verify_search_results( evict_on_missing: bool = True, max_concurrent: int | None = None, eviction_task_group: TaskGroup | None = None, -) -> list[SearchResult]: +) -> tuple[list[SearchResult], int]: """Filter search results to those the user can currently access. Deduplicates by ``(doc_id, doc_type)`` before verifying, so multiple @@ -386,10 +400,13 @@ async def verify_search_results( from FastMCP tools. Returns: - Filtered list preserving the original order. + Tuple of ``(kept_results, dropped_count)`` where ``kept_results`` is + the filtered list preserving the original order and ``dropped_count`` + is the number of unique ``(doc_id, doc_type)`` pairs that failed + verification (ghost records). """ if not results: - return results + return results, 0 user_id: str = client.username @@ -443,6 +460,9 @@ async def verify_search_results( # Compute (doc_id, doc_type) pairs that failed verification inaccessible: set[tuple[int | 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 + # with all ids on verifier crash (fail-open). accessible = accessible_by_type.get(doc_type, set(id_to_result.keys())) for doc_id in id_to_result.keys(): if doc_id not in accessible: @@ -492,11 +512,11 @@ async def verify_search_results( # best-effort: the next query re-verifies and re-attempts. try: eviction_task_group.start_soon(evict, doc_id, doc_type) - except Exception: + except RuntimeError: logger.debug("Eviction task group closed; will retry on next query") else: 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 + return kept, len(inaccessible) diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index a4023549..5ab53042 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -172,11 +172,12 @@ def configure_semantic_tools(mcp: FastMCP): eviction_task_group = getattr( ctx.request_context.lifespan_context, "eviction_task_group", None ) - verified_results = await verify_search_results( + verified_results, dropped_count = await verify_search_results( client, all_results, eviction_task_group=eviction_task_group, ) + verified_count = len(verified_results) search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. @@ -188,9 +189,24 @@ def configure_semantic_tools(mcp: FastMCP): # public API. results = [] for r in search_results: + try: + narrowed_id = int(r.id) + except (TypeError, ValueError) as e: + # Re-raise with explicit context so the outer handler logs + # something operators can act on (the generic "Search + # failed: invalid literal for int()" is opaque). + raise TypeError( + f"SemanticSearchResult.id must be int-convertible, " + f"got {r.id!r} (type={type(r.id).__name__}) for " + f"doc_type={r.doc_type!r}. This indicates a doc_type " + f"with non-numeric ids has been indexed but the " + f"public response model has not been widened. Add " + f"the doc_type to the SemanticSearchResult.id type " + f"or convert at the verifier layer." + ) from e results.append( SemanticSearchResult( - id=int(r.id), + id=narrowed_id, doc_type=r.doc_type, title=r.title, category=r.metadata.get("category", "") if r.metadata else "", @@ -304,6 +320,8 @@ def configure_semantic_tools(mcp: FastMCP): query=query, total_found=len(results), search_method=f"bm25_hybrid_{fusion}", + verified_count=verified_count, + dropped_count=dropped_count, ) except ValueError as e: @@ -382,6 +400,14 @@ def configure_semantic_tools(mcp: FastMCP): Note: Requires MCP client to support sampling. If sampling is unavailable, the tool gracefully degrades to returning documents with an explanation. The client may prompt the user to approve the sampling request. + + Latency profile: For each note in the result page, this tool fetches + the full note body via ``client.notes.get_note`` after upstream + verify-on-read has already round-tripped to the same endpoint as a + race guard (ADR-019). Expect one additional Nextcloud round-trip per + note result; raising ``limit`` above the default of 5 amplifies this + cost roughly linearly. File / news / deck results do not pay this + cost — they reuse the verified excerpt. """ # 1. Retrieve relevant documents via existing semantic search search_response = await nc_semantic_search( diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index a9253448..848b8098 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -658,6 +658,23 @@ async def _index_document( indexed_at = int(time.time()) points = [] + # Surface deck card data quality issues at indexing time rather than + # only at verification time (where _verify_deck_cards falls through to + # legacy-data pass-through when board_id/stack_id are missing). This is + # logged once per document — not per chunk — to avoid log spam. + if doc_task.doc_type == "deck_card": + missing_deck_fields = [ + field for field in ("board_id", "stack_id") if not file_metadata.get(field) + ] + if missing_deck_fields: + logger.warning( + "Indexing deck_card %s for user %s with missing metadata: %s; " + "verification will fall back to legacy-data pass-through", + doc_task.doc_id, + doc_task.user_id, + missing_deck_fields, + ) + for i, (chunk, dense_emb, sparse_emb) in enumerate( zip(chunks, dense_embeddings, sparse_embeddings) ): diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py index eabea602..ef0e27cf 100644 --- a/tests/integration/test_verify_on_read.py +++ b/tests/integration/test_verify_on_read.py @@ -58,9 +58,10 @@ async def test_verify_keeps_accessible_note( note_id = temporary_note["id"] results = [_result_for_note(note_id)] - kept = await verify_search_results(nc_client, results) + kept, dropped_count = await verify_search_results(nc_client, results) assert [r.id for r in kept] == [note_id] + assert dropped_count == 0 spy_evict.assert_not_awaited() @@ -96,9 +97,12 @@ async def test_verify_drops_deleted_note_and_schedules_eviction( 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)]) + kept, dropped_count = await verify_search_results( + nc_client, [_result_for_note(note_id)] + ) assert kept == [], "deleted note must not pass verification" + assert dropped_count == 1 spy_evict.assert_awaited_once_with(note_id, "note", nc_client.username) @@ -126,9 +130,10 @@ async def test_verify_mixed_accessible_and_deleted( _result_for_note(accessible_id), _result_for_note(ghost_id), ] - kept = await verify_search_results(nc_client, results) + kept, dropped_count = await verify_search_results(nc_client, results) assert [r.id for r in kept] == [accessible_id] + assert dropped_count == 1 spy_evict.assert_awaited_once_with(ghost_id, "note", nc_client.username) @@ -158,9 +163,10 @@ async def test_verify_dedupes_chunks_of_same_document( for i in range(3) ] - kept = await verify_search_results(nc_client, results) + kept, dropped_count = await verify_search_results(nc_client, results) # All three chunks kept (they're all from the same accessible note) assert len(kept) == 3 + assert dropped_count == 0 # ...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 index 916270ca..45017d8d 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -266,6 +266,33 @@ async def test_verify_news_items_transient_keeps_all(mocker): assert result == {1, 2, 3} +@pytest.mark.unit +async def test_verify_news_items_non_numeric_id_keeps_all(mocker): + """One non-numeric doc_id triggers fail-open for the WHOLE result set. + + The intersection logic in `_verify_news_items` tries `int(d)` for each + incoming doc_id; a single non-numeric value (e.g. ``"abc"``) raises + ValueError inside the intersection loop. The except block must catch it + and return the full input set (fail open) rather than dropping anything. + This is intentional v1 behaviour — surfacing one bad id should not + drop legitimate adjacent results. + """ + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(return_value=[{"id": 10}, {"id": 20}]) + ) + client = SimpleNamespace(news=news_client, username="alice") + + doc_ids: list[int | str] = [10, 20, "abc"] + result = await _verify_news_items( + client, + [_make_result(d, doc_type="news_item") for d in doc_ids], + _sem(), + ) + + # Fail-open: every requested id is preserved, INCLUDING the bad one. + assert result == {10, 20, "abc"} + + # --------------------------------------------------------------------------- # File verifier # --------------------------------------------------------------------------- @@ -449,7 +476,7 @@ async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): @pytest.mark.unit async def test_verify_search_results_empty_input_passthrough(): client = SimpleNamespace(username="alice") - assert await verify_search_results(client, []) == [] + assert await verify_search_results(client, []) == ([], 0) @pytest.mark.unit @@ -466,9 +493,10 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker): ] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) assert len(kept) == 3 # all kept, all reference the same accessible doc + assert dropped_count == 0 spy.assert_awaited_once() # Verifier received exactly one SearchResult (the deduplicated representative) args, _kwargs = spy.call_args @@ -494,9 +522,10 @@ async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): ] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) assert [r.id for r in kept] == [1] + assert dropped_count == 1 spy_evict.assert_awaited_once_with(99, "note", "alice") @@ -530,9 +559,12 @@ async def test_verify_search_results_fire_and_forget_eviction(mocker): client = SimpleNamespace(username="alice") async with anyio.create_task_group() as tg: - kept = await verify_search_results(client, results, eviction_task_group=tg) + kept, dropped_count = await verify_search_results( + client, results, eviction_task_group=tg + ) # 1. Search response was returned … assert kept == [] + assert dropped_count == 1 # 2. … even though eviction has started but not finished. await eviction_started.wait() assert not eviction_completed.is_set() @@ -554,9 +586,12 @@ async def test_verify_search_results_no_eviction_when_disabled(mocker): results = [_make_result(7, doc_type="note")] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results, evict_on_missing=False) + kept, dropped_count = await verify_search_results( + client, results, evict_on_missing=False + ) assert kept == [] + assert dropped_count == 1 spy_evict.assert_not_awaited() @@ -575,9 +610,10 @@ async def test_verify_search_results_unknown_doc_type_passes_through(mocker, cap results = [_make_result(1, doc_type="calendar")] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) assert len(kept) == 1 + assert dropped_count == 0 spy_evict.assert_not_awaited() @@ -595,9 +631,10 @@ async def test_verify_search_results_verifier_blowup_keeps_all(mocker): ] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) assert [r.id for r in kept] == [1, 2] + assert dropped_count == 0 # fail-open: nothing dropped spy_evict.assert_not_awaited() @@ -615,9 +652,10 @@ async def test_verify_search_results_preserves_order(mocker): ] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) assert [r.id for r in kept] == [1, 3] + assert dropped_count == 1 @pytest.mark.unit @@ -633,8 +671,11 @@ async def test_verify_search_results_eviction_failure_does_not_propagate(mocker) client = SimpleNamespace(username="alice") # Should NOT raise - kept = await verify_search_results(client, [_make_result(1, doc_type="note")]) + kept, dropped_count = await verify_search_results( + client, [_make_result(1, doc_type="note")] + ) assert kept == [] + assert dropped_count == 1 @pytest.mark.unit @@ -656,9 +697,10 @@ async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker ] client = SimpleNamespace(username="alice") - kept = await verify_search_results(client, results) + kept, dropped_count = await verify_search_results(client, results) 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() From 3e981e647ab5c34b4db315fe46490389a76a343b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:15:39 +0200 Subject: [PATCH 08/16] refactor(search): address PR #750 round 7 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7 raised 5 issues; this round addresses all of them and fixes the underlying causes (not just the comments) where applicable so they don't get re-flagged in future passes. Critical: - verified_count description in SemanticSearchResponse said "unique documents" but the value is len(verified_results), a chunk count. Description rewritten to accurately document chunk-level granularity AND explicitly call out the asymmetry with dropped_count (which counts unique (doc_id, doc_type) pairs). - _verify_files false-eviction risk: the round-6 doc-only fix was re-flagged. Address at the source — widen WebDAVClient.get_file_info to raise HTTPStatusError on 404 (matching the rest of the client convention) and reserve None for the genuinely ambiguous malformed-PROPFIND case. _verify_files now keeps the result on None (cannot tell whether the file exists) and evicts only on a definitive HTTPStatusError 404. Tests updated; new test added for the malformed-XML keep-result path. Non-critical: - News verifier semaphore lifetime now explicitly documented: one slot held for one deduplicated fetch per search is the correct backpressure behaviour. - Cross-reference comments in _verify_notes / _verify_deck_cards no longer claim "Mirrors X" pointing at functions defined later in the file; now use direction-neutral "parallel implementation in". - accessible_by_type is mutated by concurrent run_verifier tasks; a comment explains why this is race-free under anyio's cooperative multitasking (distinct keys per task, no await between read and write) so a future reader doesn't add a redundant lock. - Knock-on: tests/integration/test_rag.py wraps get_file_info in a try/except for the new contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/webdav.py | 34 ++++++----- nextcloud_mcp_server/models/semantic.py | 18 ++++-- nextcloud_mcp_server/search/verification.py | 67 +++++++++++++-------- tests/integration/test_rag.py | 15 ++++- tests/unit/client/test_webdav.py | 17 ++++-- tests/unit/search/test_verification.py | 29 ++++++++- 6 files changed, 123 insertions(+), 57 deletions(-) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 2d6356e5..0b314cb5 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -1306,7 +1306,19 @@ class WebDAVClient(BaseNextcloudClient): Returns: File info dictionary with id, name, size, content_type, etc. - Returns None if file not found. + Returns ``None`` ONLY when the server returned a malformed + PROPFIND response (missing ```` / + ```` / ```` elements) — an ambiguous + state where we cannot tell whether the file exists. + + Raises: + HTTPStatusError: For any non-2xx HTTP status, including 404 + ("not found"). Callers that want to treat 404 as + "absent" should catch ``HTTPStatusError`` and check + ``e.response.status_code``. This matches the convention + of the rest of this client and lets verify-on-read + distinguish a definitive absence (HTTP 404) from a + brittle response (None). """ webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" @@ -1323,19 +1335,13 @@ class WebDAVClient(BaseNextcloudClient): """ - try: - response = await self._client.request( - "PROPFIND", - webdav_path, - headers={"Depth": "0"}, - content=propfind_body, - ) - response.raise_for_status() - except HTTPStatusError as e: - if e.response.status_code == 404: - logger.debug(f"File not found: {path}") - return None - raise + response = await self._client.request( + "PROPFIND", + webdav_path, + headers={"Depth": "0"}, + content=propfind_body, + ) + response.raise_for_status() # Parse XML response root = ET.fromstring(response.content) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 8c882bdb..b583b098 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -83,17 +83,23 @@ class SemanticSearchResponse(BaseResponse): verified_count: int = Field( default=0, description=( - "Number of unique documents that passed verify-on-read access " - "checks (ADR-019). Equals len(results) before trimming to limit." + "Number of search result chunks that passed verify-on-read " + "access checks (ADR-019). Equals len(verified_results) before " + "trimming to limit. Note: multiple chunks of the same document " + "are counted separately here, whereas dropped_count counts " + "unique (doc_id, doc_type) pairs — the asymmetry is intentional " + "(verified_count is sized in result rows, dropped_count is " + "sized in unique ghost documents)." ), ) dropped_count: int = Field( default=0, description=( - "Number of unique documents dropped as ghost records during " - "verify-on-read (ADR-019). A short result page (len(results) < " - "limit) combined with a non-zero dropped_count indicates ghost " - "density rather than scarcity of relevant content." + "Number of unique (doc_id, doc_type) pairs dropped as ghost " + "records during verify-on-read (ADR-019). A short result page " + "(len(results) < limit) combined with a non-zero dropped_count " + "indicates ghost density rather than scarcity of relevant " + "content." ), ) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index af00bd1c..ced45d43 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -71,8 +71,9 @@ async def _verify_notes( async def check(result: SearchResult) -> None: doc_id = result.id # Parse defensively before the network call so a malformed payload - # produces a specific log line, not a generic "unexpected error" from - # the catch-all ``except Exception`` below. Mirrors ``_verify_deck_cards``. + # produces a specific log line, not a generic "unexpected error" + # from the catch-all ``except Exception`` below. The parallel + # implementation in ``_verify_deck_cards`` follows the same pattern. try: note_id_int = int(doc_id) except (TypeError, ValueError) as e: @@ -139,25 +140,19 @@ async def _verify_files( try: info = await client.webdav.get_file_info(file_path) if info is None: - # Contract: WebDAVClient.get_file_info returns None in two - # cases — (1) HTTP 404, and (2) a malformed PROPFIND XML - # response (missing , , or - # — see client/webdav.py). Both are treated as - # "inaccessible" and trigger eviction. - # - # Trade-off: a malformed response from a brittle backend - # could cause a *false* eviction. We accept that risk in - # exchange for correctness on real 404s — the index - # self-heals via re-indexing on the next scan, and - # malformed responses are exceedingly rare in practice. - # Distinguishing the two cases would require widening - # get_file_info's return contract; deferred to a future - # change if false evictions become observable. - # - # If the contract ever changes (e.g. 404 raises - # HTTPStatusError like other client methods), the - # `except HTTPStatusError` block below already handles - # it via _is_definitive_404_or_403. + # Contract (see WebDAVClient.get_file_info docstring): + # `None` means a malformed PROPFIND response — an + # ambiguous state, not a definitive 404. Treat as + # transient and KEEP the result rather than evicting. + # Real 404s raise HTTPStatusError and land in the + # _is_definitive_404_or_403 branch below. + logger.warning( + "Malformed PROPFIND response verifying file %s (%s); " + "keeping result (ambiguous state, not a definitive 404)", + doc_id, + file_path, + ) + accessible.add(doc_id) return accessible.add(doc_id) except HTTPStatusError as e: @@ -214,8 +209,9 @@ async def _verify_deck_cards( return # Parse defensively before the network call so a malformed payload - # produces a specific log line, not a generic "unexpected error" from - # the catch-all ``except Exception`` below. Mirrors ``_verify_news_items``. + # produces a specific log line, not a generic "unexpected error" + # from the catch-all ``except Exception`` below. The parallel + # implementation in ``_verify_news_items`` follows the same pattern. try: board_id_int = int(board_id) stack_id_int = int(stack_id) @@ -272,11 +268,21 @@ async def _verify_news_items( 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. The semaphore is - accepted for signature symmetry but not heavily used (one round-trip total). + called per id. Instead we fetch once and intersect. Only one slot of + the shared semaphore is consumed per search (rather than one per id), + but that slot is held for the full ``get_items`` round-trip; see the + in-body comment for the backpressure rationale. """ doc_ids = [r.id for r in results] + # Semaphore lifetime: this slot is held for the duration of ONE + # deduplicated News fetch (≤1 per search), not per-id. That is the + # correct backpressure behaviour — a single user's news verification + # must not hammer Nextcloud with concurrent fetch-all requests, and + # any other verifiers running in parallel for the same search share + # the same semaphore. Latency of this fetch is proportional to the + # user's full news corpus; see the News caveat in + # docs/configuration.md for production guidance. async with semaphore: try: # TODO(perf): if profiling shows this fetch dominates query latency @@ -426,6 +432,17 @@ async def verify_search_results( # out 50 concurrent get_note calls and exhaust the connection pool. semaphore = anyio.Semaphore(max_concurrent) + # Concurrency note: ``accessible_by_type`` is mutated by multiple + # ``run_verifier`` tasks running under the task group below. This is + # safe without an explicit lock because (a) anyio uses cooperative + # multitasking — a task only yields at ``await`` points, never + # mid-statement; (b) each task is dispatched once per ``doc_type`` + # by the loop ``for doc_type, ... in by_type.items()`` further down, + # so two tasks never write to the same key; and (c) Python dict key + # assignment is not an await point, so two tasks cannot race on the + # 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]] = {} async def run_verifier(doc_type: str, unique_results: list[SearchResult]) -> None: diff --git a/tests/integration/test_rag.py b/tests/integration/test_rag.py index c9876fd2..7df498cc 100644 --- a/tests/integration/test_rag.py +++ b/tests/integration/test_rag.py @@ -38,6 +38,7 @@ from typing import Any, AsyncGenerator import anyio import pytest +from httpx import HTTPStatusError from mcp import ClientSession from nextcloud_mcp_server.providers.base import Provider @@ -130,10 +131,18 @@ async def indexed_manual_pdf(nc_client, nc_mcp_client): logger.info(f"Setting up indexed manual PDF: {manual_path}") - # Get file info to verify file exists and get file ID - file_info = await nc_client.webdav.get_file_info(manual_path) + # Get file info to verify file exists and get file ID. After the + # round-7 contract widening, get_file_info raises HTTPStatusError on + # 404 instead of returning None — so wrap and skip on a definitive + # not-found. + try: + file_info = await nc_client.webdav.get_file_info(manual_path) + except HTTPStatusError as e: + if e.response.status_code == 404: + pytest.skip(f"Manual PDF not found at '{manual_path}'") + raise if not file_info: - pytest.skip(f"Manual PDF not found at '{manual_path}'") + pytest.skip(f"Manual PDF unreadable at '{manual_path}' (malformed PROPFIND)") file_id = file_info["id"] logger.info(f"Found manual PDF: {manual_path} (file_id={file_id})") diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index cb105cc6..0218d336 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -164,8 +164,14 @@ async def test_get_file_info_returns_file_details(mocker): @pytest.mark.unit -async def test_get_file_info_returns_none_for_missing_file(mocker): - """Test that get_file_info returns None for missing files.""" +async def test_get_file_info_raises_on_404(mocker): + """get_file_info now raises HTTPStatusError on 404 (was: returned None). + + The contract was widened so verify-on-read can distinguish a definitive + 404 from an ambiguous malformed-PROPFIND response. Callers that want + "absent → None" semantics should catch HTTPStatusError and check the + status code themselves. + """ from httpx import HTTPStatusError, Response mock_http_client = AsyncMock() @@ -180,11 +186,10 @@ async def test_get_file_info_returns_none_for_missing_file(mocker): ) ) - # Call get_file_info - result = await client.get_file_info("nonexistent.pdf") + with pytest.raises(HTTPStatusError) as exc_info: + await client.get_file_info("nonexistent.pdf") - # Verify result is None - assert result is None + assert exc_info.value.response.status_code == 404 @pytest.mark.unit diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 45017d8d..46d40ce3 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -317,9 +317,11 @@ async def test_verify_files_uses_path_from_metadata(mocker): @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.""" - webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) +async def test_verify_files_404_drops(mocker): + """get_file_info raising HTTPStatusError(404) is a definitive drop.""" + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=_http_error(404)) + ) client = SimpleNamespace(webdav=webdav_client, username="alice") result = await _verify_files( @@ -331,6 +333,27 @@ async def test_verify_files_404_via_get_file_info_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_files_malformed_propfind_keeps_result(mocker): + """get_file_info returning None means malformed PROPFIND — keep the result. + + Per the contract change in webdav.py: ``None`` is now reserved for the + ambiguous "malformed XML" case. Real 404s raise HTTPStatusError. The + file verifier must NOT evict on the ambiguous case (we cannot tell + whether the file exists), only log a warning and keep the result. + """ + webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(123, doc_type="file", metadata={"path": "brittle.txt"})], + _sem(), + ) + + assert result == {123}, "ambiguous None must keep result, not evict" + + @pytest.mark.unit async def test_verify_files_403_drops(mocker): """get_file_info raising HTTPStatusError(403) is a definitive drop.""" From 8a2626da6ce48205729d9db58b4180dd174d16e4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:40:30 +0200 Subject: [PATCH 09/16] refactor(search): address PR #750 round 8 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename `verified_count` → `verified_chunk_count` to make the count granularity explicit at the field name (chunks vs unique docs). - News verifier now fails open *per-item* on non-numeric stored doc_ids (matches notes/files/deck shape); a single bad id no longer rescues definitively-missing siblings from eviction. - Update note-verifier integration test to use string doc_ids end-to-end to match production storage (scanner.py:241 stringifies note ids). - Add regression test for the closed-task-group race guard in `verify_search_results` so the RuntimeError swallow is locked in. - Convert remaining f-string logger calls in `server/semantic.py` to lazy %-style formatting (per repo convention). - Document `evict_on_missing` as a developer/test flag (no env var) and flag the `get_file_info` 404→raise contract change in its docstring. - Add a TODO(ADR-019) breadcrumb for the hardcoded 2× over-fetch so future tuning has a clear hook. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 8 +- nextcloud_mcp_server/client/webdav.py | 9 ++ nextcloud_mcp_server/models/semantic.py | 10 +- nextcloud_mcp_server/search/verification.py | 28 ++-- nextcloud_mcp_server/server/semantic.py | 116 ++++++++++----- tests/unit/search/test_verification.py | 157 ++++++++++++++++++-- 6 files changed, 252 insertions(+), 76 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 1b4354c4..d31f40f8 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -513,9 +513,11 @@ aware of: (fail open) so a flaky link does not silently shrink result pages; only *definitive* 404 / 403 drops them. -If verification ever needs to be disabled (debugging, benchmarking), the -`evict_on_missing=False` flag on `verify_search_results()` skips eviction -without changing what is returned to the caller. +If eviction ever needs to be disabled (debugging, benchmarking), the +`evict_on_missing=False` keyword argument on `verify_search_results()` skips +the Qdrant deletes without changing what is returned to the caller. **This +is a developer/test flag, not an operator knob — it has no env-var +equivalent.** Operators who need a runtime toggle should open an issue. ### Environment Variables Reference diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 0b314cb5..585cbc71 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -1301,6 +1301,15 @@ class WebDAVClient(BaseNextcloudClient): async def get_file_info(self, path: str) -> dict[str, Any] | None: """Get file info including file ID via WebDAV PROPFIND. + .. note:: + **Behavior change (ADR-019):** previously this method returned + ``None`` for HTTP 404. It now raises ``HTTPStatusError`` for any + non-2xx status, including 404. ``None`` is reserved for the + ambiguous *malformed PROPFIND* case (server returned 2xx with a + response body missing required XML elements). External callers + updating from the old contract must catch ``HTTPStatusError`` + and inspect ``e.response.status_code`` to handle 404 explicitly. + Args: path: Path to the file (relative to user's files directory) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index b583b098..a2c713b7 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -80,16 +80,14 @@ class SemanticSearchResponse(BaseResponse): search_method: str = Field( default="semantic", description="Search method used (semantic or hybrid)" ) - verified_count: int = Field( + verified_chunk_count: int = Field( default=0, description=( "Number of search result chunks that passed verify-on-read " "access checks (ADR-019). Equals len(verified_results) before " - "trimming to limit. Note: multiple chunks of the same document " - "are counted separately here, whereas dropped_count counts " - "unique (doc_id, doc_type) pairs — the asymmetry is intentional " - "(verified_count is sized in result rows, dropped_count is " - "sized in unique ghost documents)." + "trimming to limit. Sized in chunks (result rows), NOT in " + "unique documents — pair with dropped_count carefully: " + "dropped_count is sized in unique (doc_id, doc_type) pairs." ), ) dropped_count: int = Field( diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index ced45d43..86456174 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -323,28 +323,34 @@ async def _verify_news_items( ) return set(doc_ids) - # Cast safely: a non-numeric id from the API or in our doc_ids would - # otherwise raise ValueError after the semaphore block exits and surface - # as a verifier crash. Treat as transient (fail open) instead. + # Build present_ids from the API response. If the API itself returns + # malformed (non-numeric) ids, the whole batch becomes unverifiable — + # fail open for every requested doc_id (transient). try: 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 (caller may pass ints or strs). - accessible: set[int | str] = set() - for d in doc_ids: - if int(d) in present_ids: - accessible.add(d) - return accessible except (TypeError, ValueError) as e: logger.warning( - "Non-numeric id while verifying news items (sample=%r, doc_ids=%r): %s; keeping all results", + "Non-numeric id in news API response (sample=%r): %s; keeping all results", items[:3] if items else items, - doc_ids, e, ) return set(doc_ids) + # Per-item check: a single non-numeric *stored* doc_id is fail-open + # for THAT item only — not the whole batch. Mirrors the per-item + # shape of the notes/files/deck verifiers. + accessible: set[int | str] = set() + for d in doc_ids: + try: + if int(d) in present_ids: + accessible.add(d) + except (TypeError, ValueError): + logger.debug("Non-numeric news doc_id %r; keeping (cannot verify)", d) + accessible.add(d) + return accessible + _VERIFIERS: dict[str, BatchVerifier] = { "note": _verify_notes, diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 5ab53042..977851cc 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -94,8 +94,13 @@ def configure_semantic_tools(mcp: FastMCP): username = client.username logger.info( - f"BM25 hybrid search: query='{query}', user={username}, " - f"limit={limit}, score_threshold={score_threshold}, fusion={fusion}" + "BM25 hybrid search: query=%r, user=%s, " + "limit=%d, score_threshold=%s, fusion=%s", + query, + username, + limit, + score_threshold, + fusion, ) # Check that vector sync is enabled @@ -130,6 +135,9 @@ def configure_semantic_tools(mcp: FastMCP): # The 2× factor is a deliberate v1 trade-off — raising it # costs Nextcloud round-trips on every search. Trim to # ``limit`` happens AFTER verification. + # TODO(ADR-019): expose VERIFICATION_OVERFETCH so operators + # with persistent high ghost density can tune this without a + # code change. unverified_results = await search_algo.search( query=query, user_id=username, @@ -177,7 +185,7 @@ def configure_semantic_tools(mcp: FastMCP): all_results, eviction_task_group=eviction_task_group, ) - verified_count = len(verified_results) + verified_chunk_count = len(verified_results) search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. @@ -227,8 +235,9 @@ def configure_semantic_tools(mcp: FastMCP): # Expand results with surrounding context if requested if include_context and results: logger.info( - f"Expanding {len(results)} results with context " - f"(context_chars={context_chars})" + "Expanding %d results with context (context_chars=%d)", + len(results), + context_chars, ) # Fetch context for all results in parallel @@ -286,20 +295,27 @@ def configure_semantic_tools(mcp: FastMCP): has_after_truncation=chunk_context.has_after_truncation, ) logger.debug( - f"Expanded context for {result.doc_type} {result.id}" + "Expanded context for %s %s", + result.doc_type, + result.id, ) else: # Context expansion failed, keep original result expanded_results[index] = result logger.debug( - f"Failed to expand context for {result.doc_type} {result.id}, " - "keeping original result" + "Failed to expand context for %s %s, " + "keeping original result", + result.doc_type, + result.id, ) except Exception as e: # Context expansion failed, keep original result expanded_results[index] = result logger.warning( - f"Error expanding context for {result.doc_type} {result.id}: {e}" + "Error expanding context for %s %s: %s", + result.doc_type, + result.id, + e, ) # Run all context fetches in parallel using anyio task group @@ -310,17 +326,18 @@ def configure_semantic_tools(mcp: FastMCP): # Replace results with expanded versions results = [r for r in expanded_results if r is not None] logger.info( - f"Context expansion completed: {len(results)} results with context" + "Context expansion completed: %d results with context", + len(results), ) - logger.info(f"Returning {len(results)} results from BM25 hybrid search") + logger.info("Returning %d results from BM25 hybrid search", len(results)) return SemanticSearchResponse( results=results, query=query, total_found=len(results), search_method=f"bm25_hybrid_{fusion}", - verified_count=verified_count, + verified_chunk_count=verified_chunk_count, dropped_count=dropped_count, ) @@ -341,7 +358,7 @@ def configure_semantic_tools(mcp: FastMCP): ErrorData(code=-1, message=f"Network error during search: {str(e)}") ) except Exception as e: - logger.error(f"Search error: {e}", exc_info=True) + logger.error("Search error: %s", e, exc_info=True) raise McpError(ErrorData(code=-1, message=f"Search failed: {str(e)}")) @mcp.tool( @@ -422,7 +439,7 @@ def configure_semantic_tools(mcp: FastMCP): # 2. Handle no results case - don't waste a sampling call if not search_response.results: - logger.debug(f"No documents found for query: {query}") + logger.debug("No documents found for query: %r", query) return SamplingSearchResponse( query=query, generated_answer="No relevant documents found in your Nextcloud content for this query.", @@ -439,22 +456,25 @@ def configure_semantic_tools(mcp: FastMCP): # Log capability check result for debugging logger.info( - f"Sampling capability check: client_has_sampling={client_has_sampling}, " - f"query='{query}'" + "Sampling capability check: client_has_sampling=%s, query=%r", + client_has_sampling, + query, ) if hasattr(ctx.session, "_client_params") and ctx.session._client_params: client_caps = ctx.session._client_params.capabilities logger.debug( - f"Client advertised capabilities: " - f"roots={client_caps.roots is not None}, " - f"sampling={client_caps.sampling is not None}, " - f"experimental={client_caps.experimental is not None}" + "Client advertised capabilities: " + "roots=%s, sampling=%s, experimental=%s", + client_caps.roots is not None, + client_caps.sampling is not None, + client_caps.experimental is not None, ) if not client_has_sampling: logger.info( - f"Client does not support sampling (query: '{query}'), " - f"returning {len(search_response.results)} documents" + "Client does not support sampling (query: %r), returning %d documents", + query, + len(search_response.results), ) return SamplingSearchResponse( query=query, @@ -493,8 +513,9 @@ def configure_semantic_tools(mcp: FastMCP): accessible_results[index] = result full_contents[index] = content logger.debug( - f"Fetched full content for note {result.id} " - f"(length: {len(content)} chars)" + "Fetched full content for note %s (length: %d chars)", + result.id, + len(content), ) except Exception as e: # Race window after verify_search_results — drop result. @@ -524,7 +545,9 @@ def configure_semantic_tools(mcp: FastMCP): # Check if we filtered out all results if not accessible_results: - logger.warning(f"All search results became inaccessible for query: {query}") + logger.warning( + "All search results became inaccessible for query: %r", query + ) return SamplingSearchResponse( query=query, generated_answer="All matching documents are no longer accessible.", @@ -566,9 +589,12 @@ def configure_semantic_tools(mcp: FastMCP): ) logger.info( - f"Initiating sampling request: query_length={len(query)}, " - f"documents={len(search_response.results)}, " - f"prompt_length={len(prompt)}, max_tokens={max_answer_tokens}" + "Initiating sampling request: query_length=%d, documents=%d, " + "prompt_length=%d, max_tokens=%d", + len(query), + len(search_response.results), + len(prompt), + max_answer_tokens, ) # 6. Request LLM completion via MCP sampling with timeout @@ -601,13 +627,15 @@ def configure_semantic_tools(mcp: FastMCP): # Handle non-text responses (shouldn't happen for text prompts) generated_answer = f"Received non-text response of type: {sampling_result.content.type}" logger.warning( - f"Unexpected content type from sampling: {sampling_result.content.type}" + "Unexpected content type from sampling: %s", + sampling_result.content.type, ) logger.info( - f"Sampling successful: model={sampling_result.model}, " - f"stop_reason={sampling_result.stopReason}, " - f"answer_length={len(generated_answer)}" + "Sampling successful: model=%s, stop_reason=%s, answer_length=%d", + sampling_result.model, + sampling_result.stopReason, + len(generated_answer), ) return SamplingSearchResponse( @@ -623,8 +651,10 @@ def configure_semantic_tools(mcp: FastMCP): except TimeoutError: logger.warning( - f"Sampling request timed out after {sampling_timeout_seconds} seconds for query: '{query}', " - f"returning search results only" + "Sampling request timed out after %d seconds for query: %r, " + "returning search results only", + sampling_timeout_seconds, + query, ) return SamplingSearchResponse( query=query, @@ -646,18 +676,20 @@ def configure_semantic_tools(mcp: FastMCP): if "rejected" in error_msg.lower() or "denied" in error_msg.lower(): # User explicitly declined - this is normal, not an error - logger.info(f"User declined sampling request for query: '{query}'") + logger.info("User declined sampling request for query: %r", query) search_method = "semantic_sampling_user_declined" user_message = "User declined to generate an answer" elif "not supported" in error_msg.lower(): # Client doesn't support sampling - also normal - logger.info(f"Sampling not supported by client for query: '{query}'") + logger.info("Sampling not supported by client for query: %r", query) search_method = "semantic_sampling_unsupported" user_message = "Sampling not supported by this client" else: # Other MCP protocol errors logger.warning( - f"MCP error during sampling for query '{query}': {error_msg}" + "MCP error during sampling for query %r: %s", + query, + error_msg, ) search_method = "semantic_sampling_mcp_error" user_message = f"Sampling unavailable: {error_msg}" @@ -678,8 +710,10 @@ def configure_semantic_tools(mcp: FastMCP): except Exception as e: # Truly unexpected errors - these SHOULD have tracebacks logger.error( - f"Unexpected error during sampling for query '{query}': " - f"{type(e).__name__}: {e}", + "Unexpected error during sampling for query %r: %s: %s", + query, + type(e).__name__, + e, exc_info=True, ) @@ -763,7 +797,7 @@ def configure_semantic_tools(mcp: FastMCP): indexed_count = count_result.count except Exception as e: - logger.warning(f"Failed to query Qdrant for indexed count: {e}") + logger.warning("Failed to query Qdrant for indexed count: %s", e) # Continue with indexed_count = 0 # Determine status @@ -777,7 +811,7 @@ def configure_semantic_tools(mcp: FastMCP): ) except Exception as e: - logger.error(f"Error getting vector sync status: {e}") + logger.error("Error getting vector sync status: %s", e) raise McpError( ErrorData( code=-1, diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 46d40ce3..94612eca 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -178,6 +178,29 @@ async def test_verify_notes_mixed_outcomes(mocker): assert result == {1, 3} +@pytest.mark.unit +async def test_verify_notes_string_doc_id_matches_production(mocker): + """Notes are stored with string doc_ids in production (scanner.py:241). + + The verifier must parse the string to int for the API call but + preserve the original string in the accessible set so eviction + receives the same type that was indexed in Qdrant. Without this + contract, a `MatchValue(value=42)` eviction filter would not match + a payload stored as `"42"`. + """ + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(return_value={"id": 42, "content": "x"}) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [_make_result("42", doc_type="note")], _sem()) + + # Original string id is preserved (not coerced to int 42). + assert result == {"42"} + # The API call still uses the int form internally. + notes_client.get_note.assert_awaited_once_with(42) + + # --------------------------------------------------------------------------- # News batch verifier # --------------------------------------------------------------------------- @@ -267,15 +290,15 @@ async def test_verify_news_items_transient_keeps_all(mocker): @pytest.mark.unit -async def test_verify_news_items_non_numeric_id_keeps_all(mocker): - """One non-numeric doc_id triggers fail-open for the WHOLE result set. +async def test_verify_news_items_non_numeric_id_keeps_only_bad_item(mocker): + """A non-numeric doc_id is fail-open per item, not per batch. The intersection logic in `_verify_news_items` tries `int(d)` for each - incoming doc_id; a single non-numeric value (e.g. ``"abc"``) raises - ValueError inside the intersection loop. The except block must catch it - and return the full input set (fail open) rather than dropping anything. - This is intentional v1 behaviour — surfacing one bad id should not - drop legitimate adjacent results. + incoming doc_id; a single non-numeric value (e.g. ``"abc"``) is now + caught per-item — only that one id is preserved unverified, while + valid numeric ids are still checked against the API response. Mirrors + the per-item shape of the notes/files/deck verifiers (one bad id does + not poison adjacent verifications). """ news_client = SimpleNamespace( get_items=mocker.AsyncMock(return_value=[{"id": 10}, {"id": 20}]) @@ -289,10 +312,59 @@ async def test_verify_news_items_non_numeric_id_keeps_all(mocker): _sem(), ) - # Fail-open: every requested id is preserved, INCLUDING the bad one. + # 10 and 20 are verified present; "abc" is unverifiable so kept fail-open. assert result == {10, 20, "abc"} +@pytest.mark.unit +async def test_verify_news_items_drops_missing_when_other_id_is_non_numeric( + mocker, +): + """A non-numeric doc_id no longer rescues a definitively-missing id. + + Regression for the per-item fail-open: previously a single non-numeric + doc_id triggered batch-wide fail-open, so a definitively-missing id + (20 below) escaped eviction. With per-item handling, only "abc" is + kept; 20 is correctly dropped. + """ + news_client = SimpleNamespace(get_items=mocker.AsyncMock(return_value=[{"id": 10}])) + client = SimpleNamespace(news=news_client, username="alice") + + doc_ids: list[int | str] = [10, 20, "abc"] + result = await _verify_news_items( + client, + [_make_result(d, doc_type="news_item") for d in doc_ids], + _sem(), + ) + + # 10 verified present, 20 verified missing (dropped), "abc" unverifiable. + assert result == {10, "abc"} + + +@pytest.mark.unit +async def test_verify_news_items_malformed_api_response_keeps_all(mocker): + """A malformed API response (non-numeric server id) fails open per batch. + + Distinct from a non-numeric *stored* doc_id: when the News API itself + returns garbage, we cannot build present_ids at all, so every requested + doc_id is preserved (transient — eviction will retry on next query). + """ + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(return_value=[{"id": "not-an-int"}, {"id": 20}]) + ) + client = SimpleNamespace(news=news_client, username="alice") + + doc_ids: list[int | str] = [10, 20] + result = await _verify_news_items( + client, + [_make_result(d, doc_type="news_item") for d in doc_ids], + _sem(), + ) + + # Batch fail-open: API broken, every requested id preserved. + assert result == {10, 20} + + # --------------------------------------------------------------------------- # File verifier # --------------------------------------------------------------------------- @@ -531,25 +603,32 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker): @pytest.mark.unit async def test_verify_search_results_drops_inaccessible_and_evicts(mocker): - """Inline-fallback path (no eviction_task_group): evict completes before return.""" + """Inline-fallback path (no eviction_task_group): evict completes before return. + + Notes are stored with string doc_ids in production (scanner.py:241 + ``doc_id = str(note["id"])``), so this test uses string ids end-to-end + to exercise the actual production type — ``SearchResult.id``, + ``_VERIFIERS["note"]`` return-set members, and + ``delete_document_points`` arguments all stay as ``str``. + """ 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}) + # 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"), + _make_result("1", doc_type="note"), + _make_result("99", doc_type="note"), ] client = SimpleNamespace(username="alice") kept, dropped_count = await verify_search_results(client, results) - assert [r.id for r in kept] == [1] + assert [r.id for r in kept] == ["1"] assert dropped_count == 1 - spy_evict.assert_awaited_once_with(99, "note", "alice") + spy_evict.assert_awaited_once_with("99", "note", "alice") @pytest.mark.unit @@ -598,6 +677,54 @@ async def test_verify_search_results_fire_and_forget_eviction(mocker): assert eviction_completed.is_set() +@pytest.mark.unit +async def test_verify_search_results_eviction_task_group_closed_is_ignored( + mocker, +): + """A closed eviction task group must not surface as a search error. + + Guards the race documented in `verification.py`: the lifespan task + group can exit between the ``getattr()`` capture in + ``server/semantic.py`` and the ``start_soon`` call here. Calling + ``start_soon`` on a closed group raises ``RuntimeError``; the + verifier must catch and log it (eviction is best-effort, the next + query re-verifies). Without this guard the search response would + fail. + """ + spy_evict = mocker.AsyncMock() + mocker.patch.object(verification, "delete_document_points", spy_evict) + + note_verifier = mocker.AsyncMock(return_value=set()) # 99 inaccessible + mocker.patch.dict(verification._VERIFIERS, {"note": note_verifier}, clear=False) + + class ClosedTaskGroup: + """Stand-in for an exited anyio.TaskGroup.""" + + def __init__(self): + self.start_soon_calls = 0 + + def start_soon(self, *_args, **_kwargs): + self.start_soon_calls += 1 + raise RuntimeError("This task group is not active") + + closed_tg = ClosedTaskGroup() + + results = [_make_result(99, doc_type="note")] + client = SimpleNamespace(username="alice") + + # Must NOT raise even though start_soon raises RuntimeError. + kept, dropped_count = await verify_search_results( + client, results, eviction_task_group=closed_tg + ) + + assert kept == [] + assert dropped_count == 1 + assert closed_tg.start_soon_calls == 1 + # Inline fallback must NOT run when a (closed) task group was provided — + # the guard is fire-and-forget, eviction is dropped on the floor. + spy_evict.assert_not_awaited() + + @pytest.mark.unit async def test_verify_search_results_no_eviction_when_disabled(mocker): spy_evict = mocker.AsyncMock() From 7e6ce7d3b5a747b3a6d69e67e16e8e0990702232 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:43:55 +0200 Subject: [PATCH 10/16] build: Update astrolabe commit --- third_party/astrolabe | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/astrolabe b/third_party/astrolabe index 2bf4a705..d9e641b9 160000 --- a/third_party/astrolabe +++ b/third_party/astrolabe @@ -1 +1 @@ -Subproject commit 2bf4a70599c8000a9f6f67f16ee3950767250772 +Subproject commit d9e641b93e6511fba0c27acc06ee0733fcdd5add From 06fe3916e70561abe6a203898144dd411e1c0b8a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:52:22 +0200 Subject: [PATCH 11/16] ci: Update CLAUDE.md and add pre-push-review skill. Remove astrolabe from docker-compose.yml volume mount --- .claude/skills/pre-push-review/SKILL.md | 380 ++++++++++++++++++++++++ CLAUDE.md | 12 +- docker-compose.yml | 2 +- nextcloud_mcp_server/server/semantic.py | 17 +- 4 files changed, 406 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/pre-push-review/SKILL.md diff --git a/.claude/skills/pre-push-review/SKILL.md b/.claude/skills/pre-push-review/SKILL.md new file mode 100644 index 00000000..f2214bf7 --- /dev/null +++ b/.claude/skills/pre-push-review/SKILL.md @@ -0,0 +1,380 @@ +--- +name: pre-push-review +description: | + Self-review the current branch's diff against nextcloud-mcp-server review patterns + before pushing a PR. Runs ruff/ty/tests on changed files and produces a punch list + of likely review-round findings, calibrated to issues that have repeatedly surfaced + 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. +allowed-tools: + - Bash + - Read + - Grep + - Glob +--- + +# Pre-Push Review + +## Purpose + +Catch the issues that have repeatedly surfaced in PR reviews on this repo (#733–#750) +*before* the human reviewer or the automated `claude` PR-review bot sees them. Output is +a labelled punch list. The main loop decides what to fix. + +This skill is calibrated to **this repo's recurring patterns** — not a generic code +review. The point is to short-circuit the review-round-N loop, not to replace human +judgment. + +## When to Use + +**Trigger when:** +- User says "review my work", "ready to push", "check before PR", "pre-push", or + invokes `/pre-push-review`. +- After a substantive change (new module, new tool, refactor, behavior change) and + before `git push`. +- Before opening or updating a PR. + +**Skip when:** +- Tiny diffs (typo fix, README tweak, dependency bump only). +- User has explicitly said "just push it" / "skip the review". +- Branch is `master` or has zero commits ahead of base. + +## Workflow + +### Phase 1 — Establish scope (≤ 30s) + +Determine the base branch and diff range. Default base is `master`. + +```bash +git fetch origin master --quiet +BASE=$(git merge-base HEAD origin/master) +git rev-list --count $BASE..HEAD # commits ahead +git diff --stat $BASE..HEAD # files touched +git log --format="%h %s" $BASE..HEAD # commit list +``` + +If the user names a different base (e.g. `main`, a stacked branch), use that instead. + +**Identify the change shape** — these classifications drive scope-aware checks (Phase 3): + +| Touches | Treat as | +|---|---| +| `nextcloud_mcp_server/auth/`, `*scope*`, `*token*`, `*verifier*` | **security-sensitive** (escalate B, J, K to blocking) | +| `nextcloud_mcp_server/server/*.py` (new `@mcp.tool`) | **MCP surface** (D4, I1 mandatory) | +| `nextcloud_mcp_server/client/*.py` | **client layer** (B1–B4, H1, retry/backoff) | +| `nextcloud_mcp_server/models/*.py` | **schema** (D1–D3, validators for PHP quirks) | +| `nextcloud_mcp_server/search/`, `nextcloud_mcp_server/vector/` | **vector subsystem** (E*, F3, latency budget) | +| Webhooks, `nextcloud_mcp_server/auth/webhook_*` | **webhook handler** (J3, J4, K1) | +| Tests only | **test scope** (skip A–E, run F1–F4) | +| Docs / ADR only | **docs scope** (run G3, G4 only) | + +**Report up front:** branch, base, commits ahead, files changed, change shape. + +### Phase 2 — Automated checks (in parallel, ≤ 30s) + +Run these from the project root with separate `Bash` calls in **one message** so they +run concurrently: + +```bash +uv run ruff check # lint +uv run ruff format --check # formatting +uv run ty check -- nextcloud_mcp_server # types +uv run pytest tests/unit/ -x --no-header -q # unit tests +``` + +If the diff touches `nextcloud_mcp_server/server/` or `tests/server/`, also run: + +```bash +uv run pytest -m smoke -x --no-header -q +``` + +**If any automated check fails: stop and report.** Don't run the checklist on top of a +broken build — fixing the failures may eliminate findings or change the diff. + +### Phase 3 — Read the diff and run the project checklist (2–5min) + +```bash +git diff $BASE..HEAD # full diff +git diff $BASE..HEAD -- '*.py' | head -2000 # python only, capped +``` + +Read the **whole diff** before composing findings. Cross-file patterns (test symmetry, +single-source-of-truth, deduplicated logic) are only visible in aggregate. + +For each violation, capture: +- **Severity label** (🔴 / 🟡 / 🟢 — see Severity Guide below) +- **Category tag** (A1, B2, J3, etc.) +- **File:line** (new-side line number) +- **One-sentence note** — what's wrong, with a concrete fix direction + +### Phase 4 — Output the punch list + +Format follows the template in [§ Output Template](#output-template). Group by +severity. Include a brief **Strengths** section to balance the findings — this +matches the tone of the bot review on PRs #733–#747 and avoids the impression that +the review is purely negative. + +**Hard rules:** +- Do **not** edit any files. +- Do **not** propose patches inline (one-line fix direction is OK; full code blocks are not). +- Do **not** repeat findings the linter or type checker already surfaces. +- If a finding is style-only and CLAUDE.md doesn't mandate it, demote to 🟢. + +--- + +## Severity Guide + +| Label | Meaning | Examples | +|---|---|---| +| 🔴 **blocking** | Must fix before push. Real bug, security issue, or violates an ADR/CLAUDE.md mandate. | Cache-hit bypass on auth gate; raw `List[Dict]` from MCP tool; missing `await` on async call; HMAC compare with strings instead of bytes. | +| 🟡 **important** | Should fix. Not a bug today but a footgun that has caused review-rounds before. | Field description mismatches value; unbounded fan-out without semaphore; `int()` cast inside catch-all `except`; missing `openWorldHint=True`. | +| 🟢 **nit** | Polish. Not blocking, would be nice. | `Optional[X]` when surrounding file uses `X | None`; redundant regex anchors; missing test docstring; pluralization in error messages. | +| 💡 **suggestion** | Alternative approach to consider. No fix expected. | "Could use `frozenset` here for O(1) lookup"; "Could centralise this rewrite in `_make_request`". | +| 🎉 **praise** | Notable good pattern. Worth calling out for visibility. | Cache-hit AND cache-miss paths both enforce the gate; CI-guard test for registry exhaustiveness; explicit `*` keyword-only separator. | + +**Escalation rules** (apply per change shape from Phase 1): +- Security-sensitive scope → all J* and K* findings are at least 🟡; auth bypass risks are 🔴. +- MCP surface → D4 (annotations) and I1 (response wrapping) are 🔴. +- Client layer → H1 (duplicate round-trips) is 🟡 minimum. + +--- + +## Project Checklist + +Each item is tagged for use in the punch list (e.g. `[B1]`). Items are mined from +PRs #733, #734, #735, #736, #737, #741, #745, #746, #747, #750. + +### A. Style & typing (CLAUDE.md) + +- **A1** — PEP 604 unions: `X | None`, never `Optional[X]`. (#737, #745, #735) +- **A2** — Lowercase generics: `dict[str, Any]`, `list[T]`, never `Dict`/`List`. (#736, #745) +- **A3** — `anyio` not `asyncio`: `anyio.create_task_group`, `anyio.Lock`, `anyio.run`. Flag any new `asyncio.gather`/`asyncio.Lock`/`asyncio.run`. +- **A4** — Lazy `%`-style logging: `logger.warning("msg %s", var)`, not f-strings, in any file touched on this branch. (memory: feedback_lazy_logging) +- **A5** — Typed signatures: every new/modified function has parameter and return type hints. (#733) +- **A6** — Stay consistent with surrounding file: if a touched file uses legacy `Dict`/`Optional`, flag the introduced inconsistency, not pre-existing debt. (#736, #735, #745) + +### B. Error handling specificity + +- **B1** — Hoist parsing/casts before network calls: `int()`, `json.loads()`, etc. in their own `try/except (TypeError, ValueError)` *before* the network call. Catch-all `except Exception` over the parse + network produces "unexpected error" instead of a specific log line. (#750-r3, #750-r5) +- **B2** — Narrow exception types: `except RuntimeError`, not bare `except Exception`, when only one failure mode is documented. (#750-r6) +- **B3** — Specific error messages with context: include the problematic value and its surrounding context (e.g. `f"expected int id for {doc_type}, got {type(value).__name__}: {value!r}"`), not opaque re-raises. (#750-r6) +- **B4** — Fail-open vs fail-closed is a deliberate choice: any new fail-open branch (transient error → keep going) needs a log line and a brief comment on *why* fail-open is correct here. (#750-r2) +- **B5** — Except clause order: when catching a subclass and superclass in the same `try`, the subclass must come first. `except TimeoutError` before `except Exception` (TimeoutError is OSError → Exception subtype). (#747) +- **B6** — Defensive parsing envelope: when extracting fields from external payloads, wrap in `try/except (KeyError, TypeError, ValueError)` and return a sentinel. Bubbling `ValueError` from `int("not-a-number")` to the caller is a finding. (#747) + +### C. Comment honesty & documentation + +- **C1** — Comments must match behavior: don't say "background" if the code runs inline; don't say "evicts asynchronously" if it `await`s. (#750-r1) +- **C2** — Document all return-None / sentinel cases: if a function returns `None` for both 404 *and* malformed XML, the docstring must enumerate both. (#750-r6) +- **C3** — Cross-references point at canonical/earlier definition: "Mirrors `_verify_X`" should reference the function defined first. (#750-r1) +- **C4** — TODOs include WHY: `# TODO(perf): get_items(batch_size=-1) fetches all items at query time` ✓; `# TODO: optimize` ✗. (#750-r3) +- **C5** — Defensive code carries its reason: `.get(key, default)` that "shouldn't fire in practice" needs a comment saying so — otherwise readers treat it as load-bearing. (#750-r6) +- **C6** — Surprising-on-first-read code needs a one-liner: e.g. `password=token` for bearer auth is surprising; a `# caldav reuses the password slot for bearer tokens` line saves readers from suspecting copy-paste bugs. (#734) +- **C7** — Mark deliberate omissions: when a code path *intentionally* skips a guard (e.g. DELETE returns empty body, no shape guard), say so explicitly. Otherwise future maintainers will treat it as an oversight. (#736) + +### D. API & type boundaries + +- **D1** — Public response models narrow, internal types may widen: if `SearchResult.id: int | str` for forward-compat, `SemanticSearchResult.id: int` and convert at the boundary with an explicit cast that fails loudly. (#750-r3) +- **D2** — Field descriptions match the value: a field described as "unique documents" must hold a unique-document count, not a chunk count. Watch for `len(results)` assigned to a field whose docstring implies dedup. (#750-r1, #735) +- **D3** — Symmetry between related fields: `verified_count` and `dropped_count` should count at the same granularity. (#750-r6) +- **D4** — MCP tool annotations (ADR-017): tools that hit Nextcloud have `openWorldHint=True`. Read-only ops have `readOnlyHint=True`. Destructive ops have `destructiveHint=True` + `idempotentHint=True`. Create ops have `idempotentHint=False`. Update ops have `idempotentHint=False` (etag changes mean different inputs). (#741) +- **D5** — Keyword-only separators: use `*` in signatures with multiple optional params of the same/related types, e.g. `def __init__(self, *, password=None, token=None)` to prevent positional misuse. (#734) +- **D6** — Defaults match documented constraints: if the docstring says "max 1000 characters", add `Field(max_length=1000)` — don't rely on the server returning 400. (#737) +- **D7** — Create/update tools return `*Response` wrappers, not raw models: any new `@mcp.tool` returning a raw `BaseModel` (not `BaseResponse` subclass) is a finding. (#737, CLAUDE.md MCP Response Patterns) +- **D8** — Pagination metadata: a field called `total` should be the server-side total, not page count. Use `count` for "returned in this page" or add `has_more`. (#737) + +### E. Concurrency, resource bounds & lifespan + +- **E1** — Bound concurrency: any new `asyncio.gather` / task-group fan-out over user data needs an `anyio.Semaphore` cap (default 20 in this repo). (#750-r1) +- **E2** — Bound over-fetching: `limit * K` for filtering must have a fixed `K`. Unbounded N-types or unbounded `K` is a finding. (#750-r1) +- **E3** — Don't snapshot mutable singletons: `eviction_task_group` set during lifespan startup must be read via `@property`/accessor, not snapshotted into another object's `__init__` (order-sensitive race). (#750-r5) +- **E4** — Comment safety of dict/list mutation across tasks: when multiple concurrent tasks write distinct keys to a shared dict, add a one-line comment explaining cooperative-multitasking safety. (#750-r1) +- **E5** — Lifespan context symmetry: when adding fields to one of `AppContext` / `OAuthAppContext`, mirror them in the other to prevent silent regressions in OAuth mode. (#746) +- **E6** — Security gates apply on cache-hit AND cache-miss paths: an allowlist or scope check must run after both branches resolve. A test asserting cache-hit enforcement is mandatory. (#745) + +### F. Tests + +- **F1** — Symmetry across verifiers/handlers: if you added a 403 test for the notes verifier, add one for files/deck/news too. Asymmetric coverage in a registry-style module is a finding. (#750-r5, #741) +- **F2** — Fail-open paths have a test: any verifier or handler with a fail-open branch (non-numeric id, malformed payload) has a unit test that exercises it. (#750-r5, #750-r6) +- **F3** — CI guard tests for registries: registries (e.g. `INDEXED_DOC_TYPES → verifier`) should have a test that the registry is exhaustive. (#750-r2) +- **F4** — New public functions have tests: any new MCP tool, client method, or Pydantic model without a unit test is a 🟡 finding (or 🟢 if integration-only). +- **F5** — `caplog` is logger-scoped: prefer `caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.client.notes")` over plain `caplog.at_level("WARNING")` to avoid false positives from other loggers. (#736) +- **F6** — Wire-through tests at the right layer: pure-unit tests on the leaf class are great, but a test that the parent (`NextcloudClient.from_env`) actually threads the param through is a common gap. (#734) +- **F7** — Production-evidence regressions get a unit test: any bug found via CloudWatch / production logs (not in tests) needs a unit test added in the fix PR. (#746) +- **F8** — Test docstring consistency: if 4 of 5 new tests have docstrings, give the 5th one too. (#735) + +### G. Configuration, docs & ADRs + +- **G1** — New config knobs go through `Settings` + dynaconf validator: any new env var read directly via `os.getenv` in module code (rather than via `Settings`) is a finding. (#750-r5) +- **G2** — Resolve config lazily: `default = settings.X` at function-call time, not at decoration / module-import time, so test overrides work. (#750-r5) +- **G3** — User-facing docs cover non-obvious costs: new code paths with measurable latency (unbounded fetches, per-item round-trips) need a note in `docs/configuration.md` or the relevant ADR. (#750-r2, #750-r6) +- **G4** — ADRs match shipped interface: example code blocks in any modified ADR reflect the actual module's public API. If the ADR shows `Verifier` but the code ships `BatchVerifier`, the ADR is wrong. (#750-r3) +- **G5** — Tool docstrings document hidden round-trips: if an MCP tool now does an extra fetch per result (e.g. post-verification race guard), the docstring says so. (#750-r6) +- **G6** — Migration notes for breaking changes: a PR that silently changes failure modes (e.g. previously-allowed clients now get 401) needs a migration note in the PR description and ideally a startup log line. (#745) +- **G7** — Priority order in env var fallbacks: if a setting reads from `WEBHOOK_INTERNAL_URL` → `NEXTCLOUD_MCP_SERVER_URL` → `/.dockerenv` → localhost, document the order in code comments. (#747) + +### H. Performance hygiene + +- **H1** — No duplicate round-trips: if data needed by a verifier/handler is already in `SearchResult.metadata` or a passed-in object, don't re-scroll/re-fetch it. (#750-r1) +- **H2** — Single source of truth: hardcoded lists of doc types / scopes / app names in multiple files should reference one constant (e.g. `INDEXED_DOC_TYPES`). (#750-r2) +- **H3** — Limit clamping at the right layer: `min(max(1, limit), 200)` at the client layer gives a clear error rather than a confusing server-side mismatch. (#741) +- **H4** — Avoid empty JSON bodies in HTTP requests: `json=body or None` (instead of `json=body`) prevents sending `{}` and a spurious `Content-Type: application/json` header for bodyless calls. (#741) + +### I. MCP response patterns (CLAUDE.md) + +- **I1** — No raw `List[Dict]` from MCP tools: tools return Pydantic models inheriting from `BaseResponse`. FastMCP mangles raw lists into dicts with numeric string keys. **🔴 blocking when violated.** +- **I2** — Co-author trailer in commits: each AI-assisted commit ends with `Co-Authored-By: Claude ...`. Missing trailer on AI-touched commits is a 🟢 note. + +### J. Security & input validation + +- **J1** — Validate untrusted identifiers before HTTP layer: room tokens, paths, IDs that go into URLs must be validated against an allowlist regex (e.g. `r"[A-Za-z0-9]+"` with `fullmatch`) *before* the request is built — not after. Path traversal (`../etc/passwd`) is the canonical example. (#741) +- **J2** — `hmac.compare_digest` on bytes: both sides encoded to `bytes` before comparison. Comparing strings is incorrect. (#747) +- **J3** — HTML-escape user-controlled values in HTML responses: even when the input is gated by an upstream check (e.g. `get_preset(preset_id)` returning `None` for unknown), escape on the success path too. Defense-in-depth, not single-layer. (#747) +- **J4** — Static-config-but-still-rendered fields should be escaped consistently: if dynamic fields are escaped and static fields aren't, the inconsistency itself is the smell. (#747) +- **J5** — Privacy defaults: defaults for polling/read-receipt parameters should minimize side effects (e.g. `no_status_update=True` for chat polling so users don't appear "online" on every poll). (#741) +- **J6** — Server-layer hardening even when client supports it: if an MCP tool calls a client with hardcoded safe values (e.g. `look_into_future=False`), don't expose the unsafe option as a tool parameter. (#741) +- **J7** — Redundant regex anchors with `fullmatch()`: `re.compile(r"^[A-Za-z0-9]+$").fullmatch(...)` — the `^`/`$` are redundant and misleading. Use `re.compile(r"[A-Za-z0-9]+")` with `fullmatch`. (#741) + +### K. Trust boundaries + +- **K1** — Comment where input comes from at trust boundaries: webhook payloads, request body fields, query params — say "user-controlled" or "static config" so future readers know the threat model. (#747) +- **K2** — Differentiate HTTP status codes by retry semantics: 503 for "try again" (queue saturated, dependency down), 500 for "broken" (genuine bug). 401 without `WWW-Authenticate` is correct when the caller has no auth state machine (e.g. Nextcloud webhook delivery). (#747) +- **K3** — Trust-boundary comments at API entrypoints: when a `uri` field in a request body is taken at face value (e.g. Astrolabe provides its own webhook URI), document the trust assumption. (#747) + +### L. Sentinel values & PHP API quirks + +- **L1** — `0` as a valid value vs `None` as "absent": fields like `lastReadMessage` or `expirationTimestamp` may use `0` for "no messages read" or "never expires". Don't truthy-test them. Document semantics in field docstrings. (#741) +- **L2** — PHP empty-array-as-list quirks: spreed and other PHP backends serialize empty objects as `[]`. Pydantic validators must coerce `[] → {}` for object-shaped fields and `[] → None` for nullable single-object fields. (#741) +- **L3** — `isinstance(value, (date, datetime))` is redundant: `datetime` is a subclass of `date`. Either drop `datetime` from the tuple, or distinguish behavior between the two. (#735) +- **L4** — Field description matches serialized form: a field documented as "ISO date format" but accepting `datetime` will serialize as `"2000-01-01T12:30:00"`, not `"2000-01-01"`. Either coerce to date or update the description. (#735) + +### M. Defensive code patterns & symmetry + +- **M1** — `getattr(obj, "field", None)` masks future typos: when an attribute is *expected* to exist post-fix, prefer direct access so a typo (`document_recieve_stream`) fails loudly. The defensive `getattr` is the right call only when the attribute is genuinely optional. (#746) +- **M2** — Both-credentials / multiple-mode case has explicit precedence: when two mutually-exclusive params can both be set, either raise `ValueError` or `logger.warning` — silent precedence is a footgun. (#734) +- **M3** — Defensive dict construction over passing `None`: `kwargs = {"password": password, "auth_type": "basic"} if password else {"auth_type": "bearer"}` — don't pass `None` into constructors that don't accept it. (#734) +- **M4** — Speculative defensive code without observed cause: if you add an unwrap for `[{...}] → {...}` "in case the API returns it", cite the version/route where it's been observed. Otherwise it silently masks contract changes. (#736) +- **M5** — Idempotency of URL/path transforms: `_resolve_url` should be safe to call twice. If it adds `/index.php` only when missing, document and test the idempotency. (#733) + +--- + +## What NOT to Flag + +- **Pre-existing technical debt** outside the diff. Note as 💡 if relevant, but don't list as a blocking finding. +- **Style violations the linter already catches.** ruff and ty are authoritative for what they cover. Manual style checklist is for things they don't cover (PEP 604 unions, lazy logging, `Optional` vs `|`). +- **Code formatting** — `ruff format --check` is the source of truth. +- **Bikeshed-tier preferences** (variable naming taste, line breaks, comment phrasing). +- **Anything you'd phrase as "I would have done it differently"** without a concrete repo-pattern violation. +- **Findings that duplicate the bot's previous review** if there's an open PR with one already (use `gh pr view --comments` to check). + +If a category has zero applicable findings, omit it from the punch list. Don't pad. + +--- + +## Output Template + +``` +# Pre-push review for + +**Base:** · **Commits:** · **Files changed:** +**Change shape:** + +## Automated checks +- ruff: ✅ / ❌ +- format: ✅ / ❌ +- ty: ✅ / ❌ +- unit tests: ✅ / ❌ (X passed, Y failed) +- smoke (if run): ✅ / ❌ + +[If any check failed, stop here and report. Otherwise continue.] + +## Findings + +### 🔴 Blocking () + +1. **[J1]** `nextcloud_mcp_server/client/talk.py:42` — Token interpolated into + URL before validation; path-traversal risk. Move `_validate_token(token)` + above the `httpx.URL(...)` call. + +### 🟡 Important () + +1. **[D2]** `nextcloud_mcp_server/models/semantic.py:87` — `verified_count` + description says "unique documents" but value is `len(verified_results)` + (chunk count). Either reword or dedup the value. + +2. **[F1]** `tests/unit/search/test_verification.py` — 403 tests exist for + notes/deck verifiers but not for files/news. Add for symmetry. + +### 🟢 Nits () + +1. **[A1]** `nextcloud_mcp_server/server/deck.py:118` — `Optional[int]` → + `int | None` per CLAUDE.md. + +### 💡 Suggestions + +1. `nextcloud_mcp_server/auth/client_registry.py` — Pre-existing legacy + `Dict`/`Optional` typing throughout. If touching anyway, opportunity + to modernize. + +## Strengths + +- 🎉 **[E6]** Allowlist enforced on both cache-hit and cache-miss paths in + `verify_token_for_management_api`, with a dedicated test + (`test_cache_hit_also_enforces_allowlist`). Exemplary security gate. +- 🎉 **[F3]** New `test_supported_doc_types_covers_indexed_types` enforces + registry exhaustiveness — will catch any future doc_type added without a + verifier. + +## Notes + +- 5 commits on branch; all have `Co-Authored-By` trailer ✓ +- ADR-019 implementation checklist all marked [x] (G4 ✓) +``` + +### Clean diff (no findings) + +``` +# Pre-push review for fix/notes-etag-handling + +**Base:** master · **Commits:** 2 · **Files changed:** 3 +**Change shape:** client layer + +## Automated checks +- ruff: ✅ +- format: ✅ +- ty: ✅ +- unit tests: ✅ (412 passed) + +## Findings + +No checklist violations found. + +## Strengths + +- 🎉 **[B1]** ETag parsing hoisted into its own try/except before the + network call — error logs will be specific to malformed ETag, not + generic "unexpected error". + +Safe to push. +``` + +--- + +## Notes for the Running Model + +- Read the **whole diff** before composing the report. Cross-file patterns + (F1 symmetry, H2 single-source-of-truth, E5 lifespan symmetry) are only + visible in aggregate. +- Keep findings **specific and short**. One sentence each plus a fix direction. + The point is to feed them back into the main loop for fixing, not to write + a treatise. +- If an automated check fails, **stop and report** — don't run the checklist + on top of a broken build. +- The checklist is calibrated to *this repo's* recurring review feedback. If + the diff is in an unrelated area (CI config, random docs), most items won't + apply — say so rather than padding. +- **Don't fix anything.** This skill reports; the main loop fixes. Even if a + finding is a one-character change, leave it for the user to action. +- Use `gh pr view --comments` (not `gh api`) when checking for prior bot + reviews on an existing PR — see memory feedback_use_gh_pr. diff --git a/CLAUDE.md b/CLAUDE.md index 8b93202e..96fb337c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,11 +23,21 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ``` ### Code Quality -- **Run ruff and ty before committing**: +- **Before committing or pushing, invoke the `pre-push-review` skill**: + - Runs `ruff check`, `ruff format --check`, `ty check`, and unit tests, then audits the + branch diff against this repo's recurring PR-review patterns (mined from PRs #733–#750). + - Output is a labelled punch list (🔴 blocking / 🟡 important / 🟢 nit). The main loop + fixes; the skill reports. + - Skill location: `.claude/skills/pre-push-review/SKILL.md`. Invoke via the `Skill` tool + with `skill="pre-push-review"`, or when the user types `/pre-push-review`. + - Skip only for tiny diffs (typo, README tweak, single-line dependency bump) or when the + user explicitly says "just push it". +- **Manual fallback** (if the skill is unavailable): ```bash uv run ruff check uv run ruff format uv run ty check -- nextcloud_mcp_server + uv run pytest tests/unit/ -x -q ``` - **Ruff configuration** in pyproject.toml (extends select: ["I"] for import sorting) diff --git a/docker-compose.yml b/docker-compose.yml index 8f3bda13..8d993c6d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -36,7 +36,7 @@ services: # Mount OIDC development directory outside /var/www/html to avoid rsync conflicts # The post-installation hook will register /opt/apps as an additional app directory #- ./third_party:/opt/apps:ro - - ./third_party/astrolabe:/opt/apps/astrolabe:ro + #- ./third_party/astrolabe:/opt/apps/astrolabe:ro #- ./third_party/oidc:/opt/apps/oidc:ro environment: - NEXTCLOUD_TRUSTED_DOMAINS=app diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 977851cc..5bdb6fbb 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -177,8 +177,16 @@ def configure_semantic_tools(mcp: FastMCP): # are about to be dropped. Pass the lifespan-owned task group so # eviction of dropped points is fire-and-forget (does not block # the response). - eviction_task_group = getattr( - ctx.request_context.lifespan_context, "eviction_task_group", None + # Direct attribute access — both AppContext and OAuthAppContext + # expose ``eviction_task_group`` as a @property (see app.py), + # reading dynamically from the module-level VectorSyncState + # singleton. A defensive ``getattr(..., None)`` here would mask + # typos; if a future lifespan-context type forgets the property, + # AttributeError surfaces during the first search rather than + # silently degrading to inline eviction for the life of the + # process. + eviction_task_group = ( + ctx.request_context.lifespan_context.eviction_task_group ) verified_results, dropped_count = await verify_search_results( client, @@ -507,8 +515,11 @@ def configure_semantic_tools(mcp: FastMCP): """Fetch full content for a single document (parallel with semaphore).""" async with semaphore: if result.doc_type == "note": + # SemanticSearchResult.id is typed `int` (Pydantic enforces + # at construction); no defensive cast is needed here. The + # catch-all below covers only the verify-then-delete race. try: - note = await client.notes.get_note(int(result.id)) + note = await client.notes.get_note(result.id) content = note.get("content", "") accessible_results[index] = result full_contents[index] = content From 3153c9dac4192cf3d8f706d8e1447c164d9a57ee Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:57:38 +0200 Subject: [PATCH 12/16] refactor(search): pre-push review fixes for PR #750 Address findings surfaced by `pre-push-review` after the round 8 sweep: - Add deck verifier symmetry tests (404, transient 5xx, unexpected exception, non-numeric metadata) so deck has the same shape as the notes/news/files verifiers. Also add unexpected-exception tests for the news and file verifiers, which had `except Exception` branches no test was reaching. Keeps the registry-style verifier coverage uniform. - Modernize sibling field types in `VectorSyncState`, `AppContext`, and `OAuthAppContext` from `Optional[X]` to `X | None`, matching the `eviction_task_group: TaskGroup | None` field added in the round 8 diff (resolves the inconsistency flagged by A6). The lone remaining `Optional` import is dropped. - Reverse cross-reference direction in the verifier docstrings: the later-defined `_verify_deck_cards` and `_verify_news_items` now point at `_verify_notes` as the canonical hoisted-cast pattern, rather than `_verify_notes` forward-referring to verifiers defined below it. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/app.py | 36 ++-- nextcloud_mcp_server/search/verification.py | 9 +- tests/unit/search/test_verification.py | 173 ++++++++++++++++++++ 3 files changed, 195 insertions(+), 23 deletions(-) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 5ef8a47f..68fd08bd 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -9,7 +9,7 @@ import traceback from collections.abc import AsyncIterator from contextlib import AsyncExitStack, asynccontextmanager from dataclasses import dataclass -from typing import Optional, cast +from typing import cast from urllib.parse import urlparse import anyio @@ -316,10 +316,10 @@ class VectorSyncState: and FastMCP session lifespans (where MCP tools need access to the streams). """ - document_send_stream: Optional[MemoryObjectSendStream] = None - document_receive_stream: Optional[MemoryObjectReceiveStream] = None - shutdown_event: Optional[anyio.Event] = None - scanner_wake_event: Optional[anyio.Event] = None + document_send_stream: MemoryObjectSendStream | None = None + document_receive_stream: MemoryObjectReceiveStream | None = None + shutdown_event: anyio.Event | None = None + scanner_wake_event: anyio.Event | None = None # Long-lived task group used for fire-and-forget background work spawned # from the request path (e.g. ADR-019 verify-on-read eviction). Set by the # starlette lifespan after entering its task group; cleared on shutdown. @@ -335,11 +335,11 @@ class AppContext: """Application context for BasicAuth mode.""" client: NextcloudClient - storage: Optional["RefreshTokenStorage"] = None - document_send_stream: Optional[MemoryObjectSendStream] = None - document_receive_stream: Optional[MemoryObjectReceiveStream] = None - shutdown_event: Optional[anyio.Event] = None - scanner_wake_event: Optional[anyio.Event] = None + storage: "RefreshTokenStorage | None" = None + document_send_stream: MemoryObjectSendStream | None = None + document_receive_stream: MemoryObjectReceiveStream | None = None + shutdown_event: anyio.Event | None = None + scanner_wake_event: anyio.Event | None = None @property def eviction_task_group(self) -> TaskGroup | None: @@ -357,16 +357,14 @@ class OAuthAppContext: nextcloud_host: str token_verifier: object # UnifiedTokenVerifier (ADR-005 compliant) - refresh_token_storage: Optional["RefreshTokenStorage"] = None - oauth_client: Optional[object] = None + refresh_token_storage: "RefreshTokenStorage | None" = None + oauth_client: object | None = None oauth_provider: str = "nextcloud" # "nextcloud" or "keycloak" - server_client_id: Optional[str] = ( - None # MCP server's OAuth client ID (static or DCR) - ) - document_send_stream: Optional[MemoryObjectSendStream] = None - document_receive_stream: Optional[MemoryObjectReceiveStream] = None - shutdown_event: Optional[anyio.Event] = None - scanner_wake_event: Optional[anyio.Event] = None + server_client_id: str | None = None # MCP server's OAuth client ID (static or DCR) + document_send_stream: MemoryObjectSendStream | None = None + document_receive_stream: MemoryObjectReceiveStream | None = None + shutdown_event: anyio.Event | None = None + scanner_wake_event: anyio.Event | None = None @property def eviction_task_group(self) -> TaskGroup | None: diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 86456174..abfd5a43 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -72,8 +72,9 @@ async def _verify_notes( doc_id = result.id # Parse defensively before the network call so a malformed payload # produces a specific log line, not a generic "unexpected error" - # from the catch-all ``except Exception`` below. The parallel - # implementation in ``_verify_deck_cards`` follows the same pattern. + # from the catch-all ``except Exception`` below. ``_verify_notes`` + # is the canonical shape; ``_verify_deck_cards`` and + # ``_verify_news_items`` mirror this hoisted-cast pattern. try: note_id_int = int(doc_id) except (TypeError, ValueError) as e: @@ -210,8 +211,8 @@ async def _verify_deck_cards( # Parse defensively before the network call so a malformed payload # produces a specific log line, not a generic "unexpected error" - # from the catch-all ``except Exception`` below. The parallel - # implementation in ``_verify_news_items`` follows the same pattern. + # from the catch-all ``except Exception`` below. Mirrors the + # canonical hoisted-cast pattern in ``_verify_notes``. try: board_id_int = int(board_id) stack_id_int = int(stack_id) diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 94612eca..eef33735 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -289,6 +289,31 @@ async def test_verify_news_items_transient_keeps_all(mocker): assert result == {1, 2, 3} +@pytest.mark.unit +async def test_verify_news_items_unexpected_exception_keeps_all(mocker): + """A non-HTTP exception from get_items must keep all results (fail open). + + Covers the catch-all ``except Exception`` branch that exists so a bug + in the News client (or an httpx connection error) cannot silently shrink + the result set. + """ + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(side_effect=RuntimeError("news client boom")) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items( + client, + [ + _make_result(1, doc_type="news_item"), + _make_result(2, doc_type="news_item"), + ], + _sem(), + ) + + assert result == {1, 2} + + @pytest.mark.unit async def test_verify_news_items_non_numeric_id_keeps_only_bad_item(mocker): """A non-numeric doc_id is fail-open per item, not per batch. @@ -480,6 +505,28 @@ async def test_verify_files_transient_5xx_keeps(mocker): assert result == {7} +@pytest.mark.unit +async def test_verify_files_unexpected_exception_keeps(mocker): + """A non-HTTP exception from get_file_info must not drop the result. + + The catch-all ``except Exception`` branch in the file verifier exists + so a bug in the WebDAV client (or an httpx ConnectError on a flaky + network) cannot silently shrink result pages. + """ + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=RuntimeError("dav blew up")) + ) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(8, doc_type="file", metadata={"path": "y.txt"})], + _sem(), + ) + + assert result == {8} + + # --------------------------------------------------------------------------- # Deck card verifier # --------------------------------------------------------------------------- @@ -530,6 +577,132 @@ async def test_verify_deck_cards_403_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_deck_cards_404_drops(mocker): + """Card deleted from the board → 404 from get_card → drop.""" + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=_http_error(404)) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) + + assert result == set() + + +@pytest.mark.unit +async def test_verify_deck_cards_transient_5xx_keeps(mocker): + """Transient 5xx from get_card must NOT silently shrink results.""" + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=_http_error(502)) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) + + assert result == {42} + + +@pytest.mark.unit +async def test_verify_deck_cards_unexpected_exception_keeps(mocker): + """Non-HTTP exception from get_card → fail-open, keep result.""" + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=RuntimeError("deck client boom")) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) + + assert result == {42} + + +@pytest.mark.unit +async def test_verify_deck_cards_non_numeric_metadata_keeps(mocker): + """Non-numeric board_id/stack_id/card_id must fail open before the API call. + + The hoisted ``int()`` casts in ``_verify_deck_cards`` produce a + type-specific log line; the catch-all path must never run. + """ + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=AssertionError("must not be called")) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + # Non-numeric board_id + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": "not-a-number", "stack_id": 2}, + ) + ], + _sem(), + ) + assert result == {42} + + # Non-numeric stack_id + result = await _verify_deck_cards( + client, + [ + _make_result( + 43, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": "bad"}, + ) + ], + _sem(), + ) + assert result == {43} + + # Non-numeric card_id (doc_id itself) + result = await _verify_deck_cards( + client, + [ + _make_result( + "card-uuid", + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) + assert result == {"card-uuid"} + + deck_client.get_card.assert_not_awaited() + + @pytest.mark.unit async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker): """Legacy data without board_id/stack_id → keep, do NOT iterate or call API.""" From 852ffa3678d50f7693987e97a926fc16cb1aaa30 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 22:18:03 +0200 Subject: [PATCH 13/16] refactor(search): address PR #750 round 9 review feedback - Add concurrency-safety comments to per-verifier accessible sets in _verify_notes/_verify_files/_verify_deck_cards. Same rationale as accessible_by_type in verify_search_results: anyio is cooperative, set.add() is not an await point. - Document 401 exclusion in _is_definitive_404_or_403 (treated as transient because it usually signals expired credentials, not permanent denial). - Note multi-user compounding in the news verifier semaphore comment: N concurrent users hold N slots out of the shared budget. - Log inaccessible doc ids with a type tag (e.g. "int:42" vs "str:42") so ghost-record logs disambiguate id types. - Type the BatchVerifier alias and the four verifier function signatures with NextcloudClientProtocol instead of Any (algorithms.py exposes the right interface; the protocol is runtime_checkable). - Surface verified_chunk_count vs dropped_count semantics in the nc_semantic_search tool docstring Returns block (chunks vs unique documents). - Add comments to the two max_concurrent=20 sites in server/semantic.py noting they are intentionally distinct from settings.verification_concurrency (different request phases). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/verification.py | 53 +++++++++++++++++---- nextcloud_mcp_server/server/semantic.py | 32 +++++++++++-- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index abfd5a43..096c4d92 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -39,14 +39,18 @@ from anyio.abc import TaskGroup from httpx import HTTPStatusError from nextcloud_mcp_server.config import get_settings -from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.search.algorithms import ( + NextcloudClientProtocol, + SearchResult, +) from nextcloud_mcp_server.vector.eviction import delete_document_points logger = logging.getLogger(__name__) BatchVerifier = Callable[ - [Any, list[SearchResult], anyio.Semaphore], Awaitable[set[int | str]] + [NextcloudClientProtocol, list[SearchResult], anyio.Semaphore], + Awaitable[set[int | str]], ] """(client, results, semaphore) -> set of doc_ids accessible to the user.""" @@ -57,15 +61,26 @@ BatchVerifier = Callable[ def _is_definitive_404_or_403(exc: BaseException) -> bool: - """Return True if exc indicates the document is definitively inaccessible.""" + """Return True if exc indicates the document is definitively inaccessible. + + 401 is intentionally excluded — it usually signals expired credentials + rather than permanent denial, so it is treated as transient (keep the + result; the next query will re-verify after the client refreshes). + """ if isinstance(exc, HTTPStatusError): return exc.response.status_code in (403, 404) return False async def _verify_notes( - client: Any, results: list[SearchResult], semaphore: anyio.Semaphore + client: NextcloudClientProtocol, + results: list[SearchResult], + semaphore: anyio.Semaphore, ) -> set[int | str]: + # Mutated by inner check() tasks under the task group below. Safe + # without a lock: anyio is cooperative, .add() is not an await + # point, so two tasks cannot race on the same write. Same rationale + # as accessible_by_type in verify_search_results. accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -116,8 +131,14 @@ async def _verify_notes( async def _verify_files( - client: Any, results: list[SearchResult], semaphore: anyio.Semaphore + client: NextcloudClientProtocol, + results: list[SearchResult], + semaphore: anyio.Semaphore, ) -> set[int | str]: + # Mutated by inner check() tasks under the task group below. Safe + # without a lock: anyio is cooperative, .add() is not an await + # point, so two tasks cannot race on the same write. Same rationale + # as accessible_by_type in verify_search_results. accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -184,8 +205,14 @@ async def _verify_files( async def _verify_deck_cards( - client: Any, results: list[SearchResult], semaphore: anyio.Semaphore + client: NextcloudClientProtocol, + results: list[SearchResult], + semaphore: anyio.Semaphore, ) -> set[int | str]: + # Mutated by inner check() tasks under the task group below. Safe + # without a lock: anyio is cooperative, .add() is not an await + # point, so two tasks cannot race on the same write. Same rationale + # as accessible_by_type in verify_search_results. accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -263,7 +290,9 @@ async def _verify_deck_cards( async def _verify_news_items( - client: Any, results: list[SearchResult], semaphore: anyio.Semaphore + client: NextcloudClientProtocol, + results: list[SearchResult], + semaphore: anyio.Semaphore, ) -> set[int | str]: """Batch-verify news items with a single fetch. @@ -284,6 +313,12 @@ async def _verify_news_items( # the same semaphore. Latency of this fetch is proportional to the # user's full news corpus; see the News caveat in # docs/configuration.md for production guidance. + # + # Multi-user note: the "≤1 per search" bound is per-search, not + # per-process. If N users simultaneously search news content, all N + # hold a slot for the duration of their respective fetches, each + # consuming 1/max_concurrent of the shared verification budget. A + # single news-heavy user can therefore hold their slot for seconds. async with semaphore: try: # TODO(perf): if profiling shows this fetch dominates query latency @@ -493,10 +528,12 @@ 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((str(d), t) for d, t in inaccessible), + sorted((f"{type(d).__name__}:{d}", t) for d, t in inaccessible), ) # Filter results, preserving order. All chunks of an inaccessible document diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 5bdb6fbb..5837e2a5 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -87,7 +87,16 @@ def configure_semantic_tools(mcp: FastMCP): context_chars: Number of characters to include before/after matched chunk (default: 300) Returns: - SemanticSearchResponse with matching documents ranked by fusion scores + SemanticSearchResponse with matching documents ranked by fusion scores. + + Verification fields (ADR-019 verify-on-read): + - verified_chunk_count: chunk rows that passed access checks + (sized in chunks; counted before trimming to ``limit``, so it + can exceed ``len(results)`` when a doc has multiple matching + chunks). + - dropped_count: unique ``(doc_id, doc_type)`` pairs evicted as + ghost records during this search (sized in documents, not + chunks). """ settings = get_settings() client = await get_client(ctx) @@ -248,8 +257,15 @@ def configure_semantic_tools(mcp: FastMCP): context_chars, ) - # Fetch context for all results in parallel - # Limit concurrent requests to prevent connection pool exhaustion + # Fetch context for all results in parallel. + # Limit concurrent requests to prevent connection pool exhaustion. + # + # Intentionally distinct from settings.verification_concurrency: + # that knob bounds Nextcloud round-trips during access + # verification (ADR-019); this one bounds context-expansion + # fetches that run only when ``include_context=True``. Operators + # tuning one rarely want the other in lockstep, so they share + # the default value (20) but not the env var. max_concurrent = 20 semaphore = anyio.Semaphore(max_concurrent) expanded_results = [None] * len(results) @@ -507,7 +523,15 @@ def configure_semantic_tools(mcp: FastMCP): accessible_results = [None] * len(search_response.results) full_contents = [None] * len(search_response.results) - # Limit concurrent requests to prevent connection pool exhaustion + # Limit concurrent requests to prevent connection pool exhaustion. + # + # Intentionally distinct from settings.verification_concurrency: + # that knob bounds Nextcloud round-trips during access + # verification (ADR-019). This one bounds the answer tool's + # full-content fetch — a separate request phase tied to RAG + # answer generation. Operators tuning one rarely want the other + # in lockstep, so they share the default value (20) but not the + # env var. max_concurrent = 20 semaphore = anyio.Semaphore(max_concurrent) From 15ffeca3125a53fb4178eca0a3cfdabcfe9afaef Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 22:44:57 +0200 Subject: [PATCH 14/16] refactor(search): address PR #750 round 10 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tighten verify_search_results signature: client: Any → NextcloudClientProtocol - Collapse 3 copy-pasted lock-justification comments to a single-line pointer - Add logger.debug timing around the verify_search_results call site - Add logger.debug timing around the unbounded news.get_items fetch - Rename SemanticSearchResponse.dropped_count → dropped_document_count to make the chunks-vs-documents unit asymmetry explicit at the API boundary - Drop unreachable duplicate 409 branch in WebDAVClient.move_resource Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/webdav.py | 7 ------ nextcloud_mcp_server/models/semantic.py | 12 +++++------ nextcloud_mcp_server/search/verification.py | 24 +++++++++------------ nextcloud_mcp_server/server/semantic.py | 15 +++++++++---- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 585cbc71..6c267927 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -483,13 +483,6 @@ class WebDAVClient(BaseNextcloudClient): "status_code": 409, "message": "Parent directory of destination doesn't exist", } - logger.debug( - f"Parent directory of destination '{destination_path}' doesn't exist" - ) - return { - "status_code": 409, - "message": "Parent directory of destination doesn't exist", - } else: logger.error( f"HTTP error moving resource from '{source_path}' to '{destination_path}': {e}" diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index a2c713b7..0dcd92e6 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -86,18 +86,18 @@ class SemanticSearchResponse(BaseResponse): "Number of search result chunks that passed verify-on-read " "access checks (ADR-019). Equals len(verified_results) before " "trimming to limit. Sized in chunks (result rows), NOT in " - "unique documents — pair with dropped_count carefully: " - "dropped_count is sized in unique (doc_id, doc_type) pairs." + "unique documents — see dropped_document_count for the " + "per-document counterpart." ), ) - dropped_count: int = Field( + dropped_document_count: int = Field( default=0, description=( "Number of unique (doc_id, doc_type) pairs dropped as ghost " "records during verify-on-read (ADR-019). A short result page " - "(len(results) < limit) combined with a non-zero dropped_count " - "indicates ghost density rather than scarcity of relevant " - "content." + "(len(results) < limit) combined with a non-zero " + "dropped_document_count indicates ghost density rather than " + "scarcity of relevant content." ), ) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 096c4d92..073a9f49 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -32,7 +32,6 @@ Failure policy: import logging from collections.abc import Awaitable, Callable -from typing import Any import anyio from anyio.abc import TaskGroup @@ -77,10 +76,7 @@ async def _verify_notes( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[int | str]: - # Mutated by inner check() tasks under the task group below. Safe - # without a lock: anyio is cooperative, .add() is not an await - # point, so two tasks cannot race on the same write. Same rationale - # as accessible_by_type in verify_search_results. + # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -135,10 +131,7 @@ async def _verify_files( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[int | str]: - # Mutated by inner check() tasks under the task group below. Safe - # without a lock: anyio is cooperative, .add() is not an await - # point, so two tasks cannot race on the same write. Same rationale - # as accessible_by_type in verify_search_results. + # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -209,10 +202,7 @@ async def _verify_deck_cards( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[int | str]: - # Mutated by inner check() tasks under the task group below. Safe - # without a lock: anyio is cooperative, .add() is not an await - # point, so two tasks cannot race on the same write. Same rationale - # as accessible_by_type in verify_search_results. + # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[int | str] = set() async def check(result: SearchResult) -> None: @@ -334,7 +324,13 @@ async def _verify_news_items( # fetching every item the user has access to. See the news caveat # in docs/configuration.md (Verify-on-Read) for the latency # tradeoff and follow-up paths. + news_fetch_start = anyio.current_time() items = await client.news.get_items(batch_size=-1, get_read=True) + logger.debug( + "News fetch for verification took %.2fs (%d item(s) returned)", + anyio.current_time() - news_fetch_start, + len(items), + ) 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. @@ -411,7 +407,7 @@ def get_supported_doc_types() -> set[str]: async def verify_search_results( - client: Any, + client: NextcloudClientProtocol, results: list[SearchResult], *, evict_on_missing: bool = True, diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 5837e2a5..091e3ba1 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -94,9 +94,9 @@ def configure_semantic_tools(mcp: FastMCP): (sized in chunks; counted before trimming to ``limit``, so it can exceed ``len(results)`` when a doc has multiple matching chunks). - - dropped_count: unique ``(doc_id, doc_type)`` pairs evicted as - ghost records during this search (sized in documents, not - chunks). + - dropped_document_count: unique ``(doc_id, doc_type)`` pairs + evicted as ghost records during this search (sized in + documents, not chunks). """ settings = get_settings() client = await get_client(ctx) @@ -197,12 +197,19 @@ def configure_semantic_tools(mcp: FastMCP): eviction_task_group = ( ctx.request_context.lifespan_context.eviction_task_group ) + verification_start = anyio.current_time() verified_results, dropped_count = await verify_search_results( client, all_results, eviction_task_group=eviction_task_group, ) verified_chunk_count = len(verified_results) + logger.debug( + "Verification completed in %.2fs: kept %d chunk(s), dropped %d doc(s)", + anyio.current_time() - verification_start, + verified_chunk_count, + dropped_count, + ) search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. @@ -362,7 +369,7 @@ def configure_semantic_tools(mcp: FastMCP): total_found=len(results), search_method=f"bm25_hybrid_{fusion}", verified_chunk_count=verified_chunk_count, - dropped_count=dropped_count, + dropped_document_count=dropped_count, ) except ValueError as e: From 1ed8362f7818ea24ef3f309ca91cf1a8280b8d73 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 23:00:42 +0200 Subject: [PATCH 15/16] refactor(search): address PR #750 round 11 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three minor fixes from the round-11 review on PR #750: - bm25_hybrid.py:209 — Comment said `doc_id` is `int (notes) or str (files)`, which is backwards. Notes, news_items, and deck_cards are stored as `str` (scanner.py:241, 666, 867); files are stored as `int` (scanner.py:425). Updated to point readers at scanner.py as the source of truth. - verification.py:338 — Lowered the News-API 403/404 log line from `info` to `debug`. The News app being uninstalled or disabled is a predictable operational state (matching the other verifiers' debug-on-not-found paths), so this should not generate operator-dashboard noise. Transient errors immediately below stay at `warning` because they're unexpected. - semantic.py:809 — `nc_get_vector_sync_status` was reading `document_receive_stream` via `getattr(..., None)`, but the attribute is guaranteed-defined on both `AppContext` and `OAuthAppContext` (as a field with `None` default). The defensive `getattr` masked typos that the eviction_task_group access at semantic.py:197-199 deliberately surfaces. Switched to direct access; the `if … is None:` value-check below is preserved (the attribute can legitimately be None before sync starts). Items deliberately deferred (with rationale in the plan file): - News verifier semaphore-hold during get_items (reviewer: "not required here, just worth tracking"; ADR already lists follow-ups). - Hardcoded 2× over-fetch / VERIFICATION_OVERFETCH (TODO already in code). - Integration test for the real Qdrant eviction filter (reviewer marked low-priority; type-preservation chain is unit-tested). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/bm25_hybrid.py | 2 +- nextcloud_mcp_server/search/verification.py | 5 ++++- nextcloud_mcp_server/server/semantic.py | 12 ++++++++---- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 243f9714..c5848450 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -208,7 +208,7 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): for result in search_response.points: if result.payload is None: continue - # doc_id can be int (notes) or str (files - file paths) + # 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") diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 073a9f49..ff6bdc02 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -335,7 +335,10 @@ async def _verify_news_items( # 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 app commonly disabled/uninstalled — debug-level keeps + # this off operator dashboards; transient errors below stay + # at warning because they're unexpected. + logger.debug( "News API returned %s for user %s; treating all %d news_items as inaccessible", e.response.status_code, client.username, diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 091e3ba1..ca5c9184 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -804,11 +804,15 @@ def configure_semantic_tools(mcp: FastMCP): ) try: - # Get document receive stream from lifespan context + # Get document receive stream from lifespan context. Direct + # attribute access matches the eviction_task_group pattern at + # ``nc_semantic_search`` (see comment there): both AppContext + # and OAuthAppContext define ``document_receive_stream``, so a + # missing attribute is a typo that should fail loudly. The + # value itself can legitimately be ``None`` before sync starts, + # which the check below handles. lifespan_ctx = ctx.request_context.lifespan_context - document_receive_stream = getattr( - lifespan_ctx, "document_receive_stream", None - ) + document_receive_stream = lifespan_ctx.document_receive_stream if document_receive_stream is None: logger.debug( From 104bbd390d7ffee21e867e7342261286384dbdfb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 00:26:06 +0200 Subject: [PATCH 16/16] refactor(search): address PR #750 round 12 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review items raised; four required code changes (#3, #4, #5, #6) and two were resolved without code changes (#1 audit-only, #2 informational). * search/verification.py — clarify the granularity asymmetry between the whole-batch fail-open (structural API failure) and the per-item fail-open (single bad stored doc_id). Future readers no longer need to derive why the two paths have different blast radii from the code alone. * models/semantic.py — `dropped_document_count` description now explicitly notes that subtracting it from `verified_chunk_count` is not a meaningful operation, since the two fields count different units (documents vs chunks). Surfaces the unit mismatch where MCP clients actually see it. * server/semantic.py — clarify the per-doc_type over-fetch comment so the N×2 pre-merge Qdrant cost (vs the cross-app branch's 1×2) is explicit rather than implied by "same 2× over-fetch budget". * tests/unit/search/test_verification.py — add four new 429 unit tests (notes/news/files/deck) mirroring the existing 5xx-keeps pattern. Locks in that `_is_definitive_404_or_403` returns False for 429 so a future refactor cannot accidentally treat rate-limit responses as permanent revocations. Audit confirmation for review item #1: all four `WebDAVClient.get_file_info` call sites already handle the new `HTTPStatusError`-on-404 contract (verification.py:156, tests/integration/test_rag.py:139, tests/unit/client/test_webdav.py:153/190). No silent breakage internal to this repo. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/models/semantic.py | 6 +- nextcloud_mcp_server/search/verification.py | 18 +++-- nextcloud_mcp_server/server/semantic.py | 17 ++++- tests/unit/search/test_verification.py | 79 +++++++++++++++++++++ 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 0dcd92e6..4612d26d 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -97,7 +97,11 @@ class SemanticSearchResponse(BaseResponse): "records during verify-on-read (ADR-019). A short result page " "(len(results) < limit) combined with a non-zero " "dropped_document_count indicates ghost density rather than " - "scarcity of relevant content." + "scarcity of relevant content. Note: this counter is sized in " + "unique documents while verified_chunk_count is sized in " + "chunks — a single document can contribute multiple chunks, " + "so subtracting dropped_document_count from " + "verified_chunk_count is NOT a meaningful operation." ), ) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index ff6bdc02..bb3fb678 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -358,9 +358,18 @@ async def _verify_news_items( ) return set(doc_ids) - # Build present_ids from the API response. If the API itself returns - # malformed (non-numeric) ids, the whole batch becomes unverifiable — - # fail open for every requested doc_id (transient). + # Build present_ids from the API response. Granularity is intentionally + # asymmetric with the per-item loop below: + # + # * Here (structural failure): if the API response itself is corrupt + # — even one item with a non-numeric id — we cannot reliably build + # `present_ids`, so every requested doc_id fails open. The batch + # is the only safe blast radius when the source-of-truth payload + # can't be trusted. + # * Below (data failure): a single non-numeric *stored* doc_id is a + # local data issue. Failing the whole batch open would let one bad + # row in Qdrant mask real revocations for every other item, so we + # scope the fail-open to that one id. try: present_ids = { int(item.get("id")) for item in items if item.get("id") is not None @@ -375,7 +384,8 @@ async def _verify_news_items( # Per-item check: a single non-numeric *stored* doc_id is fail-open # for THAT item only — not the whole batch. Mirrors the per-item - # shape of the notes/files/deck verifiers. + # 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() for d in doc_ids: try: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index ca5c9184..c1beb208 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -156,9 +156,20 @@ def configure_semantic_tools(mcp: FastMCP): ) all_results.extend(unverified_results) else: - # Search specific document types - # For each requested type, execute search and combine results - # under the same 2× over-fetch budget (see NOTE above). + # Search specific document types. + # + # Per-Qdrant-query cost: this branch issues ONE query per + # requested doc_type, each capped at `limit * 2`. With N + # types in `doc_types`, the pre-merge result pool is + # therefore N × `limit * 2`, NOT `limit * 2`. That is more + # Qdrant work than the cross-app branch above (which makes a + # single multi-type query returning `limit * 2` total). + # + # The post-merge trim below clamps the pool back down to + # `limit * 2` so verification (and the Nextcloud round-trips + # it triggers) sees the same budget as the cross-app branch. + # The per-type Qdrant cost remains higher; pre-trim cost + # scales linearly with len(doc_types). for dtype in doc_types: unverified_results = await search_algo.search( query=query, diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index eef33735..da5b8065 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -125,6 +125,24 @@ async def test_verify_notes_transient_5xx_keeps(mocker): assert result == {42} +@pytest.mark.unit +async def test_verify_notes_429_keeps_as_transient(mocker): + """HTTP 429 (rate-limit) is transient, NOT a definitive 403/404 drop. + + Locks in that ``_is_definitive_404_or_403`` returns False for 429 so a + future refactor cannot accidentally treat rate-limit responses as + permanent revocations and shrink result pages on every Nextcloud hiccup. + """ + notes_client = SimpleNamespace( + get_note=mocker.AsyncMock(side_effect=_http_error(429)) + ) + client = SimpleNamespace(notes=notes_client, username="alice") + + result = await _verify_notes(client, [_make_result(42)], _sem()) + + assert result == {42} + + @pytest.mark.unit async def test_verify_notes_unexpected_exception_keeps(mocker): notes_client = SimpleNamespace( @@ -289,6 +307,27 @@ async def test_verify_news_items_transient_keeps_all(mocker): assert result == {1, 2, 3} +@pytest.mark.unit +async def test_verify_news_items_429_keeps_as_transient(mocker): + """HTTP 429 from get_items must NOT collapse the batch (transient).""" + news_client = SimpleNamespace( + get_items=mocker.AsyncMock(side_effect=_http_error(429)) + ) + client = SimpleNamespace(news=news_client, username="alice") + + result = await _verify_news_items( + client, + [ + _make_result(1, doc_type="news_item"), + _make_result(2, doc_type="news_item"), + _make_result(3, doc_type="news_item"), + ], + _sem(), + ) + + assert result == {1, 2, 3} + + @pytest.mark.unit async def test_verify_news_items_unexpected_exception_keeps_all(mocker): """A non-HTTP exception from get_items must keep all results (fail open). @@ -505,6 +544,23 @@ async def test_verify_files_transient_5xx_keeps(mocker): assert result == {7} +@pytest.mark.unit +async def test_verify_files_429_keeps_as_transient(mocker): + """HTTP 429 from get_file_info must NOT silently drop file results.""" + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=_http_error(429)) + ) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(7, doc_type="file", metadata={"path": "x.txt"})], + _sem(), + ) + + assert result == {7} + + @pytest.mark.unit async def test_verify_files_unexpected_exception_keeps(mocker): """A non-HTTP exception from get_file_info must not drop the result. @@ -623,6 +679,29 @@ async def test_verify_deck_cards_transient_5xx_keeps(mocker): assert result == {42} +@pytest.mark.unit +async def test_verify_deck_cards_429_keeps_as_transient(mocker): + """HTTP 429 from get_card must NOT silently shrink result pages.""" + deck_client = SimpleNamespace( + get_card=mocker.AsyncMock(side_effect=_http_error(429)) + ) + client = SimpleNamespace(deck=deck_client, username="alice") + + result = await _verify_deck_cards( + client, + [ + _make_result( + 42, + doc_type="deck_card", + metadata={"board_id": 1, "stack_id": 2}, + ) + ], + _sem(), + ) + + assert result == {42} + + @pytest.mark.unit async def test_verify_deck_cards_unexpected_exception_keeps(mocker): """Non-HTTP exception from get_card → fail-open, keep result."""