From d4dbf01b0a54b2c85d3b664c416a813a2fc01cb1 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 2 Jun 2026 21:14:44 +0200 Subject: [PATCH 1/4] fix(search): gate verify-on-read file results on vector-index tag membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-on-read only checked file *accessibility* (file_accessible_by_id), never tag membership, so a file removed from the `vector-index` tag (but still readable) kept surfacing in semantic search, and stale points only got evicted when they happened to rank in a search's top-K. Rework `_verify_files` to gate on current `vector-index` tag membership via a single batch `find_files_by_tag(tag, mime_type_filter="application/pdf")` REPORT per search (plus a one-shot EXCLUDED_TAGS lookup for exclusion-wins parity) — exactly what the scanner indexes. A file is kept iff it is in that set, so untagged / deleted / excluded files drop out immediately and the existing eviction wiring reclaims their Qdrant points. The gate is strict for all file results, own and shared. Mirrors the batch-fetch-and-intersect shape of `_verify_news_items` (one semaphore slot, fail-open on fetch error, malformed-id keep). - Promote the tag name to a `vector_sync_pdf_tag` Settings field (dynaconf env mapping VECTOR_SYNC_PDF_TAG) used by both scanner and verifier; drop the scanner's direct os.getenv. - Expose `find_files_by_tag` on NextcloudClientProtocol. - Rewrite the file-verifier unit tests (tagged/untagged/deleted/excluded/ fail-open/non-numeric); update the ACL + verify-on-read integration tests to seed tagged PDFs. - Amend ADR-019 and the configuration.md verify-on-read latency budget. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-019-verify-on-read-for-semantic-search.md | 16 ++ docs/configuration.md | 36 ++- nextcloud_mcp_server/config.py | 9 + nextcloud_mcp_server/search/algorithms.py | 8 + nextcloud_mcp_server/search/verification.py | 178 +++++++++------ nextcloud_mcp_server/vector/scanner.py | 3 +- tests/integration/test_acl_shared_search.py | 42 +++- tests/integration/test_verify_on_read.py | 72 ++++-- tests/unit/search/test_verification.py | 214 +++++++++--------- 9 files changed, 358 insertions(+), 220 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 926837ed..97ae84ed 100644 --- a/docs/ADR-019-verify-on-read-for-semantic-search.md +++ b/docs/ADR-019-verify-on-read-for-semantic-search.md @@ -4,6 +4,22 @@ **Date**: 2026-05-01 **Depends On**: ADR-007 (Background Vector Sync), ADR-010 (Webhook-Based Vector Sync) +> **Update (2026-06-02) — tag-aware file verification.** The `file` verifier +> described below as a per-id WebDAV check (`PROPFIND`, later +> `file_accessible_by_id`) now gates on current **`vector-index` tag +> membership** instead. It issues a single +> `find_files_by_tag(, mime_type_filter="application/pdf")` +> REPORT per search (plus a one-shot `EXCLUDED_TAGS` lookup) and keeps only +> files in that set — i.e. exactly what the scanner indexes. This is the +> "fetch once and intersect" shape (like `news_item`), not per-id fan-out, and +> it closes a gap the original design missed: a file *removed from the tag* (as +> opposed to deleted/unshared) stayed accessible and so survived the old check, +> lingering in results until the scanner's grace-period sweep. **Decision:** the +> gate is strict for all file results, own and shared — a shared file survives +> only if the owner's (userVisible) tag surfaces in the *searcher's* tag REPORT +> (validated by `tests/integration/test_acl_shared_search.py`). See +> `docs/configuration.md` → "Verify-on-Read Latency Budget" for the cost. + ## 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). diff --git a/docs/configuration.md b/docs/configuration.md index 2640982e..81a719bb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -645,10 +645,12 @@ 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. + in the result set — except `file` and `news_item`, which each batch into a + single call per search regardless of how many results they contribute (see + the Files and News caveats below). 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. Tunable via the `VERIFICATION_CONCURRENCY` env var (settings field `verification_concurrency`, default 20) — lower it if your Nextcloud @@ -664,14 +666,28 @@ aware of: 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. - Eviction failures are logged but never propagated; the next query will - re-verify and re-attempt (self-healing). +- **Files caveat**: `file` results are gated on current **`vector-index` tag + membership**, not bare access — the verifier issues a single + `find_files_by_tag(, mime_type_filter="application/pdf")` REPORT per + search that contains any file result (plus a one-shot `EXCLUDED_TAGS` + lookup), then keeps only files in that set. This matches exactly what the + scanner indexes, so a file removed from the tag (or deleted, or moved under + an excluded folder) drops out of results immediately rather than waiting for + the scanner sweep. The REPORT expands tagged folders via a `Depth: infinity` + SEARCH, so deployments that tag whole directory trees pay that walk once per + search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. **Shared + files**: a file an owner tagged and shared with the searcher only survives + verification if the owner's (userVisible) tag surfaces in the *searcher's* + tag REPORT. +- **Eviction**: when verification finds a definitive miss (a 404 / 403, or — for + files — absence from the tag set), 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. + *definitive* misses (404 / 403, or a file no longer in the tag set) drop them. + If the file tag REPORT itself errors, all file results are kept (fail open). If eviction ever needs to be disabled (debugging, benchmarking), the `evict_on_missing=False` keyword argument on `verify_search_results()` skips diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index d4e60c9e..dc733ef8 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -93,6 +93,10 @@ _DEFAULTS: dict[str, Any] = { # leave work stuck behind the 5x-scan-interval staleness gate. # Escape hatch only — leave on by default. "vector_sync_orphan_sweep_enabled": True, + # System tag that marks files for vector indexing. The scanner indexes + # files carrying this tag; verify-on-read gates results on current + # membership of this tag (ADR-019). + "vector_sync_pdf_tag": "vector-index", # Verify-on-read concurrency cap (ADR-019) "verification_concurrency": 20, # Qdrant @@ -625,6 +629,10 @@ class Settings: vector_sync_queue_max_size: int = 10000 vector_sync_user_poll_interval: int = 60 # seconds - OAuth mode user discovery vector_sync_orphan_sweep_enabled: bool = True # card #101 + # System tag marking files for vector indexing. The scanner indexes files + # carrying this tag and verify-on-read gates results on current membership + # (ADR-019), so an untagged file drops out of search immediately. + vector_sync_pdf_tag: str = "vector-index" # Verify-on-read concurrency (ADR-019). Cap on parallel Nextcloud # round-trips during search-result verification fan-out. Lower this if the @@ -1217,6 +1225,7 @@ def get_settings() -> Settings: "vector_sync_queue_max_size": "VECTOR_SYNC_QUEUE_MAX_SIZE", "vector_sync_user_poll_interval": "VECTOR_SYNC_USER_POLL_INTERVAL", "vector_sync_orphan_sweep_enabled": "VECTOR_SYNC_ORPHAN_SWEEP_ENABLED", + "vector_sync_pdf_tag": "VECTOR_SYNC_PDF_TAG", # Verify-on-read (ADR-019) "verification_concurrency": "VERIFICATION_CONCURRENCY", # Qdrant settings diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index bbd22341..3566133c 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -75,6 +75,14 @@ class NextcloudClientProtocol(Protocol): """News client for accessing news item documents.""" ... + # Top-level client helper (not a sub-client) used by verify-on-read to + # gate file results on current vector-index tag membership. + async def find_files_by_tag( + self, tag_name: str, mime_type_filter: str | None = None + ) -> list[dict]: + """Return files carrying ``tag_name`` (folders expanded by MIME).""" + ... + async def get_indexed_doc_types( user_id: str, accessible_owners: list[str] | None = None diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index ee10d7d9..ba45fdea 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -8,18 +8,21 @@ access (deleted, unshared, etc.) and lazily evicting them from the index. Per-doc_type verifiers are registered in ``_VERIFIERS``. Each takes the 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 +``doc_id`` values that are currently visible to the user. Verifiers read +whatever metadata they need (e.g. 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. +so verification adds zero extra Qdrant round-trips. The file verifier is the +exception: it gates results on current ``vector-index`` tag membership via a +single batch tag REPORT (which also confirms access), so it does not read +per-result metadata. 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: +Failure policy (notes / deck_card / news_item — the per-access verifiers): - Definitive 403/404 from Nextcloud → drop the result and schedule eviction. - Transient errors (5xx, network blips, unexpected exceptions) → keep the @@ -28,6 +31,12 @@ Failure policy: - 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. + +The ``file`` verifier is the exception to the first rule: it gates on current +``vector-index`` tag membership (a single batch tag REPORT), so a file is +dropped+evicted when it is absent from the tag set — untagged, deleted, or +under an ``EXCLUDED_TAGS`` folder — not on a per-file 403/404. A failed tag +fetch still fails open. See ``_verify_files`` for the full contract. """ import logging @@ -132,81 +141,110 @@ async def _verify_files( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[str]: - """Return the doc_ids of file results this user may actually access. + """Return the doc_ids of file results this user may currently see. - Verifies each file by its *global* Nextcloud file id via an ACL-aware - WebDAV SEARCH (``webdav.file_accessible_by_id``), NOT by path. This is the - ACL-aware-search fix: a file an owner shared with the querying user mounts - at a different path under each tree, so the previous path-based check - (``get_file_info``) produced false 404s and dropped legitimate shared-file - hits. Definitive 403/404 → inaccessible (dropped + scheduled for eviction - by the caller); transient/ambiguous errors → kept (fail-open). + A file is included iff it *currently* carries the ``vector-index`` tag (the + same tag the scanner indexes on) AND is not under an ``EXCLUDED_TAGS`` + folder — full parity with the indexing rules. The tag REPORT runs over the + querying user's own files tree (including mounted shares), so membership in + the tagged set already implies the file is *accessible*; this single batch + fetch therefore subsumes the old per-file ``file_accessible_by_id`` check + and replaces N round-trips with one. + + This is the verify-on-read fix for stale tags (ADR-019): a file removed + from the ``vector-index`` tag — or outright deleted — drops out of the + tagged set, so it is dropped from results and scheduled for eviction by the + caller immediately, rather than lingering until the scanner's grace-period + sweep reconciles it. + + Failure policy mirrors ``_verify_news_items``: if the tag fetch itself + fails we keep every file result (fail-open, the next query re-verifies), + and malformed/non-numeric doc_ids are kept (defense-in-depth — the numeric + tag REPORT cannot match them, and producer-side validation is the real + boundary, so false-positive is preferred over false-negative). """ - # safe: cooperative concurrency, no lock needed (see verify_search_results) - accessible: set[str] = set() + # Lazy import to break an import cycle: ``server/__init__`` imports + # ``server.semantic`` which imports this module, so importing + # ``server.tag_exclusion`` at module load time would re-enter a + # partially-initialised ``server`` package depending on import order. + from nextcloud_mcp_server.server.tag_exclusion import ( # noqa: PLC0415 + get_excluded_file_paths, + is_path_excluded, + ) - 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); kept here only for log context. - file_path = (result.metadata or {}).get("path") + tag_name = get_settings().vector_sync_pdf_tag - # Verify by *global* file ID via an ACL-aware WebDAV SEARCH, NOT by - # path. For files the vector ``doc_id`` IS the Nextcloud file ID, and - # file_accessible_by_id searches the user's whole tree (incl. mounted - # shares), so a file an owner shared with this user verifies as - # accessible even though it lives at a different path under the owner's - # root. A path-based check (the old behaviour) would 404 on shared - # files mounted at the recipient's root by basename and silently drop - # legitimate ACL-aware-search results. - # - # Hoisted cast mirrors _verify_notes: a malformed id keeps the result - # (fail open) with a specific log line rather than a generic - # "unexpected error" from the catch-all below. + # One batch fetch per search, holding a single semaphore slot (same + # backpressure rationale as _verify_news_items): the tagged-file REPORT + # plus optional Depth:infinity folder expansion — and the EXCLUDED_TAGS + # lookup — are one round-trip set, not one per result. + # + # TODO(perf): if folder expansion dominates query latency, cache the + # tagged-id set per user with a short TTL (mirroring the + # list_accessible_owners cache in search/access_filter.py). Skipped here so + # an untag is reflected on the very next search rather than after a TTL. + async with semaphore: try: - file_id_int = int(doc_id) - except (TypeError, ValueError) as e: + tagged = await client.find_files_by_tag( + tag_name, mime_type_filter="application/pdf" + ) + except HTTPStatusError as e: logger.warning( - "Non-numeric file id %r (%s): %s; keeping result", - doc_id, - file_path, + "Transient error fetching %r-tagged files for verification: " + "%s %s; keeping all file results", + tag_name, + e.response.status_code, e, ) + return {r.id for r in results} + except Exception as e: + logger.warning( + "Unexpected error fetching %r-tagged files for verification: " + "%s; keeping all file results", + tag_name, + e, + ) + return {r.id for r in results} + + # Exclusion wins: a tagged file under an EXCLUDED_TAGS folder must not + # surface, matching the scanner's defense-in-depth filter. A failure + # here degrades to "no exclusion" rather than dropping legitimate hits. + try: + excluded_paths = await get_excluded_file_paths(client.webdav) + except Exception as e: + logger.warning( + "EXCLUDED_TAGS lookup failed during verification (%s); " + "proceeding without exclusion filter", + e, + ) + excluded_paths = set() + + tagged_ids: set[str] = set() + for f in tagged: + file_id = f.get("id") + if file_id is None: + continue + if excluded_paths and is_path_excluded(f.get("path", ""), excluded_paths): + continue + # Normalise to str — Qdrant doc_id payload is keyword-indexed and the + # scanner stringifies file ids on write, so SearchResult.id is a str. + tagged_ids.add(str(file_id)) + + accessible: set[str] = set() + for r in results: + doc_id = r.id + if doc_id in tagged_ids: accessible.add(doc_id) - return - - async with semaphore: - try: - if await client.webdav.file_accessible_by_id(file_id_int): - accessible.add(doc_id) - # else: definitively inaccessible (not owned, not shared) — - # drop and let the caller schedule eviction. - 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: - # Network blip / unexpected WebDAV error — ambiguous, not a - # definitive denial. Keep the result; the next query re-verifies. - 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 r in results: - tg.start_soon(check, r) - + elif not is_valid_nextcloud_doc_id(doc_id): + logger.warning( + "Malformed file doc_id %r in verifier; keeping to avoid " + "dropping a potentially legitimate result (cannot match " + "against the numeric tag REPORT)", + doc_id, + ) + accessible.add(doc_id) + # else: a valid file id absent from the tagged set is untagged/deleted/ + # excluded — drop it and let the caller schedule eviction. return accessible diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 14e2158c..c690eb4c 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -4,7 +4,6 @@ Periodically scans enabled users' content and queues changed documents for proce """ import logging -import os import random import time from dataclasses import dataclass @@ -396,7 +395,7 @@ async def scan_user_documents( # PDF descendants (Depth: infinity SEARCH), so a tag on a # folder applies to every PDF beneath it. settings = get_settings() - tag_name = os.getenv("VECTOR_SYNC_PDF_TAG", "vector-index") + tag_name = settings.vector_sync_pdf_tag tagged_files = await nc_client.find_files_by_tag( tag_name, mime_type_filter="application/pdf" ) diff --git a/tests/integration/test_acl_shared_search.py b/tests/integration/test_acl_shared_search.py index e689c29d..9d90c851 100644 --- a/tests/integration/test_acl_shared_search.py +++ b/tests/integration/test_acl_shared_search.py @@ -54,6 +54,16 @@ def _reset_owners_cache(): _DOC_TEXT = "Confidential quarterly infrastructure budget and capacity plan" +# Minimal valid PDF. verify-on-read gates file results on the vector-index tag +# via find_files_by_tag(..., mime_type_filter="application/pdf"), so the shared +# file must be a PDF (matching what the scanner actually indexes), not a .txt. +_PDF_BYTES = ( + b"%PDF-1.4\n" + b"1 0 obj<>endobj\n" + b"2 0 obj<>endobj\n" + b"3 0 obj<>endobj\n" + b"trailer<>\n%%EOF\n" +) def _user_client(username: str, password: str) -> NextcloudClient: @@ -81,7 +91,13 @@ async def acl_users(test_users_setup): @pytest.fixture async def shared_file(acl_users): - """alice creates a nested file and shares it with bob (not diana). + """alice creates a nested PDF, tags it ``vector-index``, and shares it with + bob (not diana). + + The vector-index tag is required because verify-on-read now gates file + results on current tag membership (in addition to ACL access). The tag is + created userVisible so the owner's assignment surfaces in the recipient's + systemtag REPORT — this fixture is the live check of that assumption. Yields (file_id, owner_relative_path); cleans up the directory after. """ @@ -89,18 +105,28 @@ async def shared_file(acl_users): suffix = uuid.uuid4().hex[:8] test_dir = f"acl_e2e_{suffix}" nested = f"{test_dir}/reports" - path = f"{nested}/budget.txt" + path = f"{nested}/budget.pdf" await alice.webdav.create_directory(test_dir) await alice.webdav.create_directory(nested) - await alice.webdav.write_file(path, _DOC_TEXT.encode(), "text/plain") + await alice.webdav.write_file(path, _PDF_BYTES, "application/pdf") file_id = (await alice.webdav.get_file_info(path))["id"] + tag = await alice.webdav.get_or_create_tag( + name=get_settings().vector_sync_pdf_tag, + user_visible=True, + user_assignable=True, + ) + await alice.webdav.assign_tag_to_file(file_id, tag["id"]) await alice.sharing.create_share( path=f"/{path}", share_with="bob", share_type=0, permissions=1 ) try: yield file_id, path finally: + try: + await alice.webdav.remove_tag_from_file(file_id, tag["id"]) + except Exception: + pass await alice.webdav.delete_resource(test_dir) @@ -132,7 +158,7 @@ async def seeded_semantic(monkeypatch, shared_file): "user_id": "alice", "is_placeholder": False, "file_path": path, - "title": "budget.txt", + "title": "budget.pdf", "excerpt": _DOC_TEXT, "chunk_index": 0, "total_chunks": 1, @@ -180,7 +206,13 @@ async def _search_as(user_client, file_id_unused) -> list: async def test_recipient_finds_shared_file_without_indexing(acl_users, seeded_semantic): """Bob finds alice's shared file end-to-end: real share lookup expands his accessible owners to include alice, the filter surfaces her point, and - real verification confirms his ACL access — all without bob indexing.""" + real verification confirms both his ACL access AND that the file is still + in the vector-index tag set — all without bob indexing. + + This also exercises the strict tag-gate's key assumption: a vector-index + tag alice assigned (userVisible) surfaces in bob's systemtag REPORT for a + file shared into his tree. If a future Nextcloud version stops surfacing an + owner's tag to a recipient, this assertion is where it fails first.""" file_id = seeded_semantic # Sanity: the live OCS lookup really does expand bob to include alice. owners = await list_accessible_owners(acl_users["bob"].sharing, "bob") diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py index bb405ca4..f4919f2a 100644 --- a/tests/integration/test_verify_on_read.py +++ b/tests/integration/test_verify_on_read.py @@ -5,16 +5,16 @@ 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. +**Coverage**: the ``note`` verifier and the ``file`` verifier (tag-membership +gate, see the shared-recipient tests below) are exercised against real +Nextcloud here. The ``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 (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 @@ -30,6 +30,7 @@ import pytest from httpx import BasicAuth, HTTPStatusError from nextcloud_mcp_server.client import NextcloudClient +from nextcloud_mcp_server.config import get_settings 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 @@ -38,6 +39,17 @@ logger = logging.getLogger(__name__) pytestmark = pytest.mark.integration +# Minimal valid PDF — the file verifier gates on the vector-index tag via +# find_files_by_tag(..., mime_type_filter="application/pdf"), so file fixtures +# must be PDFs (matching what the scanner indexes), not .txt. +_PDF_BYTES = ( + b"%PDF-1.4\n" + b"1 0 obj<>endobj\n" + b"2 0 obj<>endobj\n" + b"3 0 obj<>endobj\n" + b"trailer<>\n%%EOF\n" +) + def _result_for_note(note_id: int) -> SearchResult: return SearchResult( @@ -51,9 +63,10 @@ def _result_for_note(note_id: int) -> SearchResult: def _result_for_file(file_id: int, path: str) -> SearchResult: # Mirrors what the algorithm layer propagates: doc_id IS the global file id, - # ``path`` is carried in metadata (owner-relative) for log context only. + # stringified (SearchResult.id is always str), ``path`` is carried in + # metadata (owner-relative) for log context only. return SearchResult( - id=file_id, + id=str(file_id), doc_type="file", title=path.split("/")[-1], excerpt="...", @@ -222,11 +235,13 @@ async def alice_bob_clients(test_users_setup): async def test_verify_keeps_nested_file_shared_with_recipient( alice_bob_clients, mocker ): - """The PR #813 acceptance check at the verifier layer. + """The PR #813 acceptance check at the verifier layer, under tag-gating. - Alice owns a file in a *subfolder* and shares it with Bob. Verifying the - result as Bob must KEEP it — proving the id-based check sees the share. - A path-based check (the old behaviour) would 404 here and wrongly drop it. + Alice owns a PDF in a *subfolder*, tags it ``vector-index`` (userVisible), + and shares it with Bob. Verifying the result as Bob must KEEP it — proving + the tag REPORT surfaces an owner-assigned tag on a file shared into Bob's + tree. If a future Nextcloud version stops surfacing the owner's tag to a + recipient, this is where strict tag-gating regresses shared search. """ spy_evict = mocker.AsyncMock() mocker.patch.object(verification, "delete_document_points", spy_evict) @@ -235,12 +250,18 @@ async def test_verify_keeps_nested_file_shared_with_recipient( suffix = uuid.uuid4().hex[:8] test_dir = f"acl_verify_{suffix}" nested_dir = f"{test_dir}/reports" - shared_path = f"{nested_dir}/shared.txt" + shared_path = f"{nested_dir}/shared.pdf" await alice.webdav.create_directory(test_dir) await alice.webdav.create_directory(nested_dir) - await alice.webdav.write_file(shared_path, b"alice's shared report", "text/plain") + await alice.webdav.write_file(shared_path, _PDF_BYTES, "application/pdf") file_id = (await alice.webdav.get_file_info(shared_path))["id"] + tag = await alice.webdav.get_or_create_tag( + name=get_settings().vector_sync_pdf_tag, + user_visible=True, + user_assignable=True, + ) + await alice.webdav.assign_tag_to_file(file_id, tag["id"]) await alice.sharing.create_share( path=f"/{shared_path}", share_with="bob", share_type=0, permissions=1 @@ -251,18 +272,23 @@ async def test_verify_keeps_nested_file_shared_with_recipient( bob, [_result_for_file(file_id, shared_path)] ) - assert [r.id for r in kept] == [file_id], ( - "a nested file shared with bob must pass verification for bob" + assert [r.id for r in kept] == [str(file_id)], ( + "a nested tagged PDF shared with bob must pass verification for bob" ) assert dropped_count == 0 spy_evict.assert_not_awaited() finally: + try: + await alice.webdav.remove_tag_from_file(file_id, tag["id"]) + except Exception: + pass await alice.webdav.delete_resource(test_dir) async def test_verify_drops_unshared_file_for_other_user(alice_bob_clients, mocker): - """Negative control: a file Alice did NOT share is inaccessible to Bob and - must be dropped + scheduled for eviction under his identity.""" + """Negative control: a file Alice did NOT share is absent from Bob's + vector-index tag set (and his tree), so it must be dropped + scheduled for + eviction under his identity.""" spy_evict = mocker.AsyncMock() mocker.patch.object(verification, "delete_document_points", spy_evict) @@ -282,6 +308,6 @@ async def test_verify_drops_unshared_file_for_other_user(alice_bob_clients, mock assert kept == [], "an unshared file must not pass verification for bob" assert dropped_count == 1 - spy_evict.assert_awaited_once_with(file_id, "file", bob.username) + spy_evict.assert_awaited_once_with(str(file_id), "file", bob.username) finally: await alice.webdav.delete_resource(test_dir) diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index ca480c3e..bcc6a8af 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -437,141 +437,135 @@ async def test_verify_news_items_malformed_api_response_keeps_all(mocker): # --------------------------------------------------------------------------- +def _patch_excluded(mocker, paths: set[str] | None = None, *, side_effect=None): + """Patch the lazily-imported EXCLUDED_TAGS lookup used by _verify_files.""" + if side_effect is not None: + mock = mocker.AsyncMock(side_effect=side_effect) + else: + mock = mocker.AsyncMock(return_value=paths if paths is not None else set()) + return mocker.patch( + "nextcloud_mcp_server.server.tag_exclusion.get_excluded_file_paths", mock + ) + + +def _file_client(mocker, *, tagged=None, find_side_effect=None, username="alice"): + """Build a client whose find_files_by_tag returns the given tagged files.""" + if find_side_effect is not None: + find = mocker.AsyncMock(side_effect=find_side_effect) + else: + find = mocker.AsyncMock(return_value=tagged if tagged is not None else []) + return SimpleNamespace( + find_files_by_tag=find, + webdav=SimpleNamespace(), + username=username, + ) + + @pytest.mark.unit -async def test_verify_files_accessible_by_global_id_is_kept(mocker): - """File verifier resolves the file by its global ID (the doc_id), ACL-aware. +async def test_verify_files_tagged_is_kept(mocker): + """A file currently carrying the vector-index tag is kept, and the tagged + set is fetched with a single batch call (not one per result).""" + _patch_excluded(mocker) + client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}]) - This is what lets a recipient verify a file an owner shared with them: - file_accessible_by_id searches the user's whole tree (incl. mounted - shares) by global file id, not a path under the caller's own root (which - would 404 on shared files mounted at a different path). - """ - webdav_client = SimpleNamespace( - file_accessible_by_id=mocker.AsyncMock(return_value=True) - ) - client = SimpleNamespace(webdav=webdav_client, username="alice") - - result = await _verify_files( - client, - [_make_result(100, doc_type="file", metadata={"path": "Documents/foo.txt"})], - _sem(), - ) + result = await _verify_files(client, [_make_result(100, doc_type="file")], _sem()) assert result == {"100"} - webdav_client.file_accessible_by_id.assert_awaited_once_with(100) + client.find_files_by_tag.assert_awaited_once_with( + "vector-index", mime_type_filter="application/pdf" + ) @pytest.mark.unit -async def test_verify_files_inaccessible_id_drops(mocker): - """file_accessible_by_id returning False (file not in the user's tree) is a - definitive drop — the file is neither owned by nor shared with the user.""" - webdav_client = SimpleNamespace( - file_accessible_by_id=mocker.AsyncMock(return_value=False) - ) - client = SimpleNamespace(webdav=webdav_client, username="alice") +async def test_verify_files_untagged_drops(mocker): + """A file removed from the vector-index tag (absent from the tagged set) is + dropped even though it may still exist and be readable by the user.""" + _patch_excluded(mocker) + client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}]) result = await _verify_files( client, - [_make_result(123, doc_type="file", metadata={"path": "gone.txt"})], + [_make_result(100, doc_type="file"), _make_result(200, doc_type="file")], _sem(), ) + # 100 is still tagged → kept; 200 was untagged → dropped. + assert result == {"100"} + + +@pytest.mark.unit +async def test_verify_files_deleted_drops(mocker): + """A deleted file is absent from the tagged set → dropped (caller evicts).""" + _patch_excluded(mocker) + client = _file_client(mocker, tagged=[]) + + result = await _verify_files(client, [_make_result(123, doc_type="file")], _sem()) + assert result == set() @pytest.mark.unit -async def test_verify_files_403_404_drops(mocker): - """A 403/404 raised by the SEARCH call is treated as a definitive drop, - consistent with the shared _is_definitive_404_or_403 policy used by every - verifier. (Normal inaccessibility surfaces as an empty result set, not a - status code, and is covered by test_verify_files_inaccessible_id_drops.)""" - for status in (403, 404): - webdav_client = SimpleNamespace( - file_accessible_by_id=mocker.AsyncMock(side_effect=_http_error(status)) - ) - client = SimpleNamespace(webdav=webdav_client, username="alice") +async def test_verify_files_excluded_path_drops(mocker): + """A tagged file under an EXCLUDED_TAGS folder must not surface — exclusion + wins, parity with the scanner's defense-in-depth filter.""" + # get_excluded_file_paths returns slash-stripped (normalised) paths. + _patch_excluded(mocker, {"Secret"}) + client = _file_client( + mocker, + tagged=[ + {"id": 100, "path": "/Documents/foo.pdf"}, + {"id": 200, "path": "/Secret/bar.pdf"}, + ], + ) + result = await _verify_files( + client, + [_make_result(100, doc_type="file"), _make_result(200, doc_type="file")], + _sem(), + ) + + assert result == {"100"} + + +@pytest.mark.unit +async def test_verify_files_tag_fetch_failure_keeps_all(mocker): + """If the tag REPORT itself fails (HTTP or otherwise), keep every file + result (fail-open) — never silently shrink results on a backend blip.""" + _patch_excluded(mocker) + for exc in (_http_error(503), _http_error(429), RuntimeError("dav blew up")): + client = _file_client(mocker, find_side_effect=exc) result = await _verify_files( client, - [_make_result(124, doc_type="file", metadata={"path": "x.txt"})], + [_make_result(7, doc_type="file"), _make_result(8, doc_type="file")], _sem(), ) - - assert result == set(), f"{status} on the SEARCH call must drop" + assert result == {"7", "8"}, f"{exc!r} on the tag fetch must keep all results" @pytest.mark.unit -async def test_verify_files_non_numeric_id_keeps_unverified(mocker): - """Without a numeric file id we cannot verify — fail open, don't drop.""" - webdav_client = SimpleNamespace( - file_accessible_by_id=mocker.AsyncMock( - side_effect=AssertionError("must not be called") - ) - ) - client = SimpleNamespace(webdav=webdav_client, username="alice") +async def test_verify_files_exclusion_lookup_failure_proceeds(mocker): + """If the EXCLUDED_TAGS lookup fails, proceed without the exclusion filter + rather than dropping legitimate tagged hits.""" + _patch_excluded(mocker, side_effect=RuntimeError("ocs down")) + client = _file_client(mocker, tagged=[{"id": 100, "path": "/Documents/foo.pdf"}]) + + result = await _verify_files(client, [_make_result(100, doc_type="file")], _sem()) + + assert result == {"100"} + + +@pytest.mark.unit +async def test_verify_files_non_numeric_id_keeps(mocker): + """A malformed (non-numeric) doc_id cannot be matched against the numeric + tag REPORT, so it is kept (defense-in-depth, false-positive preferred).""" + _patch_excluded(mocker) + client = _file_client(mocker, tagged=[]) result = await _verify_files( - client, - [_make_result("not-a-file-id", doc_type="file", metadata={"path": "x.txt"})], - _sem(), + client, [_make_result("not-a-file-id", doc_type="file")], _sem() ) + assert result == {"not-a-file-id"} - webdav_client.file_accessible_by_id.assert_not_awaited() - - -@pytest.mark.unit -async def test_verify_files_transient_5xx_keeps(mocker): - webdav_client = SimpleNamespace( - file_accessible_by_id=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"} - - -@pytest.mark.unit -async def test_verify_files_429_keeps_as_transient(mocker): - """HTTP 429 from the SEARCH call must NOT silently drop file results.""" - webdav_client = SimpleNamespace( - file_accessible_by_id=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 file_accessible_by_id 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( - file_accessible_by_id=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"} # --------------------------------------------------------------------------- @@ -889,11 +883,11 @@ async def test_verify_evicts_cross_user_file_under_querying_user_id(mocker): """ spy_evict = mocker.AsyncMock() mocker.patch.object(verification, "delete_document_points", spy_evict) + _patch_excluded(mocker) - webdav_client = SimpleNamespace( - file_accessible_by_id=mocker.AsyncMock(return_value=False) - ) - client = SimpleNamespace(webdav=webdav_client, username="bob") + # The shared file is no longer in bob's tagged set (share revoked → absent + # from his vector-index tag REPORT), so the file verifier drops it. + client = _file_client(mocker, tagged=[], username="bob") kept, dropped_count = await verify_search_results( client, From 8cae7d170884c04a3075422f14466c303570b276 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 2 Jun 2026 23:09:46 +0200 Subject: [PATCH 2/4] fix(search): address PR #834 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the latest PR-review comment on the verify-on-read tag-gate work: - tests: stringify note IDs in test_verify_on_read.py so SearchResult.id matches production (scanner stringifies all IDs on write) — helper and the keeps/deleted/mixed/dedupe assertions (blocking). - tests: make the unshared-file negative control a PDF so the drop is unambiguously "unshared", not a mime_type_filter mismatch. - config: add Validator("VECTOR_SYNC_PDF_TAG", len_min=1) — an empty tag name would make find_files_by_tag("") misbehave in the verifier and scanner. - verification: correct the _verify_files comment — two batch fetches (tag REPORT + EXCLUDED_TAGS lookup) are held under one semaphore slot; the pure-Python intersection runs outside it. - tests: de-duplicate the minimal-PDF constant into a shared PDF_BYTES in tests/integration/conftest.py, imported by both integration modules. Verified: ruff/format/ty/unit all green; the two integration modules (10 tests) pass against a local Nextcloud (app-only, no MCP profile needed). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/config.py | 2 ++ nextcloud_mcp_server/search/verification.py | 10 ++++--- tests/integration/conftest.py | 13 +++++++++ tests/integration/test_acl_shared_search.py | 13 ++------- tests/integration/test_verify_on_read.py | 30 +++++++-------------- 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index dc733ef8..c5e95350 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -259,6 +259,8 @@ _dynaconf = Dynaconf( Validator("DOCUMENT_CHUNK_SIZE", gte=1), # Non-negative Validator("DOCUMENT_CHUNK_OVERLAP", gte=0), + # Non-empty strings + Validator("VECTOR_SYNC_PDF_TAG", len_min=1), # Enum constraints Validator("LOG_FORMAT", is_in=["text", "json"]), Validator( diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index ba45fdea..0bd36c32 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -174,10 +174,14 @@ async def _verify_files( tag_name = get_settings().vector_sync_pdf_tag - # One batch fetch per search, holding a single semaphore slot (same + # Two batch fetches per search held under a single semaphore slot (same # backpressure rationale as _verify_news_items): the tagged-file REPORT - # plus optional Depth:infinity folder expansion — and the EXCLUDED_TAGS - # lookup — are one round-trip set, not one per result. + # (plus optional Depth:infinity folder expansion) and the EXCLUDED_TAGS + # lookup (get_excluded_file_paths, itself ~2×N concurrent calls). Both are + # batched once per search, not once per result, and the slot bounds them. + # The pure-Python intersection that builds tagged_ids/accessible runs + # *outside* the slot — it needs no Nextcloud round-trip (mirrors the + # post-fetch present_ids build in _verify_news_items). # # TODO(perf): if folder expansion dominates query latency, cache the # tagged-id set per user with a short TTL (mirroring the diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index ab5b7f8f..9d596e32 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -13,6 +13,19 @@ logger = logging.getLogger(__name__) # Valid provider names VALID_PROVIDERS = ["openai", "ollama", "anthropic", "bedrock"] +# Canonical minimal valid PDF for integration tests. verify-on-read gates file +# results on the vector-index tag via +# find_files_by_tag(..., mime_type_filter="application/pdf"), so file fixtures +# must be PDFs (matching what the scanner indexes), not .txt. Shared here so the +# constant is defined once rather than drifting across test modules. +PDF_BYTES = ( + b"%PDF-1.4\n" + b"1 0 obj<>endobj\n" + b"2 0 obj<>endobj\n" + b"3 0 obj<>endobj\n" + b"trailer<>\n%%EOF\n" +) + def pytest_addoption(parser): """Add --provider command line option for RAG tests.""" diff --git a/tests/integration/test_acl_shared_search.py b/tests/integration/test_acl_shared_search.py index 9d90c851..07ec0135 100644 --- a/tests/integration/test_acl_shared_search.py +++ b/tests/integration/test_acl_shared_search.py @@ -40,6 +40,7 @@ from nextcloud_mcp_server.search.access_filter import ( from nextcloud_mcp_server.search.context import get_chunk_with_context from nextcloud_mcp_server.search.semantic import SemanticSearchAlgorithm from nextcloud_mcp_server.search.verification import verify_search_results +from tests.integration.conftest import PDF_BYTES pytestmark = pytest.mark.integration @@ -54,16 +55,6 @@ def _reset_owners_cache(): _DOC_TEXT = "Confidential quarterly infrastructure budget and capacity plan" -# Minimal valid PDF. verify-on-read gates file results on the vector-index tag -# via find_files_by_tag(..., mime_type_filter="application/pdf"), so the shared -# file must be a PDF (matching what the scanner actually indexes), not a .txt. -_PDF_BYTES = ( - b"%PDF-1.4\n" - b"1 0 obj<>endobj\n" - b"2 0 obj<>endobj\n" - b"3 0 obj<>endobj\n" - b"trailer<>\n%%EOF\n" -) def _user_client(username: str, password: str) -> NextcloudClient: @@ -109,7 +100,7 @@ async def shared_file(acl_users): await alice.webdav.create_directory(test_dir) await alice.webdav.create_directory(nested) - await alice.webdav.write_file(path, _PDF_BYTES, "application/pdf") + await alice.webdav.write_file(path, PDF_BYTES, "application/pdf") file_id = (await alice.webdav.get_file_info(path))["id"] tag = await alice.webdav.get_or_create_tag( name=get_settings().vector_sync_pdf_tag, diff --git a/tests/integration/test_verify_on_read.py b/tests/integration/test_verify_on_read.py index f4919f2a..ccf889cc 100644 --- a/tests/integration/test_verify_on_read.py +++ b/tests/integration/test_verify_on_read.py @@ -34,26 +34,16 @@ from nextcloud_mcp_server.config import get_settings 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 +from tests.integration.conftest import PDF_BYTES logger = logging.getLogger(__name__) pytestmark = pytest.mark.integration -# Minimal valid PDF — the file verifier gates on the vector-index tag via -# find_files_by_tag(..., mime_type_filter="application/pdf"), so file fixtures -# must be PDFs (matching what the scanner indexes), not .txt. -_PDF_BYTES = ( - b"%PDF-1.4\n" - b"1 0 obj<>endobj\n" - b"2 0 obj<>endobj\n" - b"3 0 obj<>endobj\n" - b"trailer<>\n%%EOF\n" -) - def _result_for_note(note_id: int) -> SearchResult: return SearchResult( - id=note_id, + id=str(note_id), doc_type="note", title=f"note_{note_id}", excerpt="...", @@ -96,7 +86,7 @@ async def test_verify_keeps_accessible_note( kept, dropped_count = await verify_search_results(nc_client, results) - assert [r.id for r in kept] == [note_id] + assert [r.id for r in kept] == [str(note_id)] assert dropped_count == 0 spy_evict.assert_not_awaited() @@ -139,7 +129,7 @@ async def test_verify_drops_deleted_note_and_schedules_eviction( assert kept == [], "deleted note must not pass verification" assert dropped_count == 1 - spy_evict.assert_awaited_once_with(note_id, "note", nc_client.username) + spy_evict.assert_awaited_once_with(str(note_id), "note", nc_client.username) async def test_verify_mixed_accessible_and_deleted( @@ -168,9 +158,9 @@ async def test_verify_mixed_accessible_and_deleted( ] kept, dropped_count = await verify_search_results(nc_client, results) - assert [r.id for r in kept] == [accessible_id] + assert [r.id for r in kept] == [str(accessible_id)] assert dropped_count == 1 - spy_evict.assert_awaited_once_with(ghost_id, "note", nc_client.username) + spy_evict.assert_awaited_once_with(str(ghost_id), "note", nc_client.username) async def test_verify_dedupes_chunks_of_same_document( @@ -189,7 +179,7 @@ async def test_verify_dedupes_chunks_of_same_document( # Three chunks of the same note (chunk_index varies) results = [ SearchResult( - id=note_id, + id=str(note_id), doc_type="note", title="note", excerpt=f"chunk {i}", @@ -254,7 +244,7 @@ async def test_verify_keeps_nested_file_shared_with_recipient( await alice.webdav.create_directory(test_dir) await alice.webdav.create_directory(nested_dir) - await alice.webdav.write_file(shared_path, _PDF_BYTES, "application/pdf") + await alice.webdav.write_file(shared_path, PDF_BYTES, "application/pdf") file_id = (await alice.webdav.get_file_info(shared_path))["id"] tag = await alice.webdav.get_or_create_tag( name=get_settings().vector_sync_pdf_tag, @@ -295,10 +285,10 @@ async def test_verify_drops_unshared_file_for_other_user(alice_bob_clients, mock alice, bob = alice_bob_clients suffix = uuid.uuid4().hex[:8] test_dir = f"acl_verify_priv_{suffix}" - private_path = f"{test_dir}/private.txt" + private_path = f"{test_dir}/private.pdf" await alice.webdav.create_directory(test_dir) - await alice.webdav.write_file(private_path, b"alice's private note", "text/plain") + await alice.webdav.write_file(private_path, PDF_BYTES, "application/pdf") file_id = (await alice.webdav.get_file_info(private_path))["id"] try: From 7033c64393939f2b5f59fed3d9f122460cdb0cdb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 2 Jun 2026 23:52:16 +0200 Subject: [PATCH 3/4] fix(search): address PR #834 re-review (403/404 coverage + docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the blocking + important findings from the claude bot's re-review: - test (blocking): pin the file verifier's fail-open contract for definitive 403/404 on the tag REPORT, not just transient 503/429. A disabled systemtags endpoint commonly 403s; unlike the per-access verifiers (where 403/404 = drop), the batch file verifier must keep all results since the whole set hinges on one REPORT. Adds _http_error(403)/_http_error(404) to test_verify_files_tag_fetch_failure_keeps_all and documents the asymmetry. - docs (important): migration caveat — if vector-index was created as user_visible=False (manual occ tag:add, or pre-release), an owner's tag won't surface in a recipient's REPORT and shared-file results are silently dropped after upgrade. Note that the MCP server's get_or_create_tag defaults to user_visible=True, and how to verify/fix an existing tag. - docs (important): note the file verifier's latency scales with both the Depth:infinity folder expansion and the EXCLUDED_TAGS lookup (~2 WebDAV calls per excluded tag, fanned out under one slot); suggest lowering VERIFICATION_CONCURRENCY for large excluded-tag lists / deeply tagged trees. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/configuration.md | 25 +++++++++++++++++++++---- tests/unit/search/test_verification.py | 20 +++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 81a719bb..1b4dd23c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -675,10 +675,27 @@ aware of: an excluded folder) drops out of results immediately rather than waiting for the scanner sweep. The REPORT expands tagged folders via a `Depth: infinity` SEARCH, so deployments that tag whole directory trees pay that walk once per - search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. **Shared - files**: a file an owner tagged and shared with the searcher only survives - verification if the owner's (userVisible) tag surfaces in the *searcher's* - tag REPORT. + search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. The `file` + verifier's latency therefore scales with **both** the `Depth: infinity` folder + expansion **and** the `EXCLUDED_TAGS` lookup: that lookup fans out ~2 WebDAV + calls (1 PROPFIND + 1 REPORT) *per excluded tag*, concurrently, while holding + a single verification slot — so a deployment with a long `EXCLUDED_TAGS` list + and/or deeply tagged trees issues many parallel Nextcloud requests per search. + Operators in that situation may want to **lower `VERIFICATION_CONCURRENCY`** so + the file verifier's internal fan-out does not overwhelm the backend. +- **Shared files**: a file an owner tagged and shared with the searcher only + survives verification if the owner's **`userVisible`** tag surfaces in the + *searcher's* tag REPORT. The MCP server's own tag-creation path + (`WebDAVClient.get_or_create_tag`) defaults to `user_visible=True`, so tags it + creates are fine. **Migration caveat**: if the `vector-index` tag was created + some other way — manually via `occ tag:add … --user-visible=false`, or in a + deployment predating this release — it may be `user_visible=False` (the + Nextcloud default for system-managed tags). In that case an owner's tag will + **not** surface in a recipient's systemtag REPORT, so every shared-file result + is *silently dropped* for recipients after upgrading — no error, just a + narrower result set. Verify the tag's visibility (Administration → *Collaborative + tags*, or `occ tag:list`) and, if it is not user-visible, recreate it as + user-visible so shared search keeps working. - **Eviction**: when verification finds a definitive miss (a 404 / 403, or — for files — absence from the tag set), the corresponding Qdrant points are deleted in the background on a lifespan-owned task group — fire-and-forget, does diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index bcc6a8af..ab198096 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -529,10 +529,24 @@ async def test_verify_files_excluded_path_drops(mocker): @pytest.mark.unit async def test_verify_files_tag_fetch_failure_keeps_all(mocker): - """If the tag REPORT itself fails (HTTP or otherwise), keep every file - result (fail-open) — never silently shrink results on a backend blip.""" + """If the tag REPORT itself fails, keep every file result (fail-open) — + never silently shrink results on a backend blip. + + Unlike the per-access verifiers (notes/deck/news), where a definitive + 403/404 is the DROP signal, the file verifier fails open on *every* HTTP + error — including 403/404. The whole result set hinges on one batch REPORT, + so a disabled systemtags endpoint (commonly 403) must not nuke all file + results; the next query re-verifies. 403 and 404 are pinned here alongside + the transient 503/429 to lock that contract against regression. + """ _patch_excluded(mocker) - for exc in (_http_error(503), _http_error(429), RuntimeError("dav blew up")): + for exc in ( + _http_error(403), + _http_error(404), + _http_error(503), + _http_error(429), + RuntimeError("dav blew up"), + ): client = _file_client(mocker, find_side_effect=exc) result = await _verify_files( client, From b736bf199b236c778bf2f13bb3e59f020bb47813 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 3 Jun 2026 00:23:55 +0200 Subject: [PATCH 4/4] perf(search): skip exclusion lookup on empty tag set; fix semaphore comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the two important findings from the claude bot's latest re-review: - _verify_files: skip get_excluded_file_paths entirely when the tag REPORT returns no files. An empty `tagged` yields an empty `tagged_ids` regardless of exclusions, so the lookup's 2xlen(EXCLUDED_TAGS) WebDAV fan-out is wasted work in the common "this tag matched nothing" case. The per-result loop still runs, so malformed doc_ids are still kept (fail-open) — pinned by a new test (test_verify_files_empty_tag_set_skips_exclusion_lookup), which also asserts the exclusion lookup is never awaited. - Rewrite the semaphore comment: it claimed "the slot bounds them", but the slot only caps concurrent *searches* — get_excluded_file_paths internally spawns a task group issuing 2xlen(EXCLUDED_TAGS) concurrent WebDAV calls, so live Nextcloud connections can exceed VERIFICATION_CONCURRENCY. Comment now says so and points at configuration.md. The third 🟡 (sequential dir expansion in find_files_by_tag) is pre-existing and flagged by the reviewer as a follow-up, not part of this PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/verification.py | 45 ++++++++++++++------- tests/unit/search/test_verification.py | 24 +++++++++++ 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 0bd36c32..1192d5fb 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -174,11 +174,20 @@ async def _verify_files( tag_name = get_settings().vector_sync_pdf_tag - # Two batch fetches per search held under a single semaphore slot (same - # backpressure rationale as _verify_news_items): the tagged-file REPORT - # (plus optional Depth:infinity folder expansion) and the EXCLUDED_TAGS - # lookup (get_excluded_file_paths, itself ~2×N concurrent calls). Both are - # batched once per search, not once per result, and the slot bounds them. + # One semaphore slot is held for both Nextcloud round-trips: the tagged-file + # REPORT (plus optional Depth:infinity folder expansion) and — only when the + # REPORT returned files — the EXCLUDED_TAGS lookup. Both are batched once per + # search, not once per result (same backpressure rationale as + # _verify_news_items). + # + # The slot caps how many *searches* verify files concurrently, but it does + # NOT bound the fan-out *within* one verification: get_excluded_file_paths + # internally spawns a task group issuing 2×len(EXCLUDED_TAGS) concurrent + # WebDAV calls (1 PROPFIND + 1 REPORT per excluded tag), so the live + # Nextcloud connection count can exceed VERIFICATION_CONCURRENCY when + # excluded tags are configured. See configuration.md → "Files caveat" for + # the latency/tuning guidance. + # # The pure-Python intersection that builds tagged_ids/accessible runs # *outside* the slot — it needs no Nextcloud round-trip (mirrors the # post-fetch present_ids build in _verify_news_items). @@ -213,15 +222,23 @@ async def _verify_files( # Exclusion wins: a tagged file under an EXCLUDED_TAGS folder must not # surface, matching the scanner's defense-in-depth filter. A failure # here degrades to "no exclusion" rather than dropping legitimate hits. - try: - excluded_paths = await get_excluded_file_paths(client.webdav) - except Exception as e: - logger.warning( - "EXCLUDED_TAGS lookup failed during verification (%s); " - "proceeding without exclusion filter", - e, - ) - excluded_paths = set() + # + # Skip the lookup entirely when the tag REPORT returned nothing: an empty + # `tagged` yields an empty `tagged_ids` regardless of the exclusion set, + # so the lookup's 2×len(EXCLUDED_TAGS) WebDAV fan-out cannot change the + # outcome — avoid it in the common "this tag matched nothing" case. The + # per-result loop below still runs, so malformed doc_ids are still kept + # (fail-open), exactly as when `tagged` is non-empty. + excluded_paths: set[str] = set() + if tagged: + try: + excluded_paths = await get_excluded_file_paths(client.webdav) + except Exception as e: + logger.warning( + "EXCLUDED_TAGS lookup failed during verification (%s); " + "proceeding without exclusion filter", + e, + ) tagged_ids: set[str] = set() for f in tagged: diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index ab198096..bc79925f 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -504,6 +504,30 @@ async def test_verify_files_deleted_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_files_empty_tag_set_skips_exclusion_lookup(mocker): + """When the tag REPORT returns no files, the EXCLUDED_TAGS lookup is skipped + entirely: an empty tagged set drops every valid-id result regardless of + exclusions, so the lookup's 2xN WebDAV fan-out is wasted work. Malformed + doc_ids are still kept (fail-open), exactly as on the non-empty path.""" + excluded = _patch_excluded(mocker, {"Secret"}) + client = _file_client(mocker, tagged=[]) + + result = await _verify_files( + client, + [ + _make_result(123, doc_type="file"), + _make_result("not-a-file-id", doc_type="file"), + ], + _sem(), + ) + + # Valid id absent from the (empty) tagged set → dropped; malformed id kept. + assert result == {"not-a-file-id"} + # The optimization: no exclusion fan-out when there is nothing to filter. + excluded.assert_not_awaited() + + @pytest.mark.unit async def test_verify_files_excluded_path_drops(mocker): """A tagged file under an EXCLUDED_TAGS folder must not surface — exclusion