From e8df6003c5c4fc7f66a7962751b74ae84ad115b8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:02:27 +0200 Subject: [PATCH] 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()