From 104bbd390d7ffee21e867e7342261286384dbdfb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 00:26:06 +0200 Subject: [PATCH] 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."""