From 8cae7d170884c04a3075422f14466c303570b276 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 2 Jun 2026 23:09:46 +0200 Subject: [PATCH] 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: