From 3e981e647ab5c34b4db315fe46490389a76a343b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:15:39 +0200 Subject: [PATCH] refactor(search): address PR #750 round 7 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7 raised 5 issues; this round addresses all of them and fixes the underlying causes (not just the comments) where applicable so they don't get re-flagged in future passes. Critical: - verified_count description in SemanticSearchResponse said "unique documents" but the value is len(verified_results), a chunk count. Description rewritten to accurately document chunk-level granularity AND explicitly call out the asymmetry with dropped_count (which counts unique (doc_id, doc_type) pairs). - _verify_files false-eviction risk: the round-6 doc-only fix was re-flagged. Address at the source — widen WebDAVClient.get_file_info to raise HTTPStatusError on 404 (matching the rest of the client convention) and reserve None for the genuinely ambiguous malformed-PROPFIND case. _verify_files now keeps the result on None (cannot tell whether the file exists) and evicts only on a definitive HTTPStatusError 404. Tests updated; new test added for the malformed-XML keep-result path. Non-critical: - News verifier semaphore lifetime now explicitly documented: one slot held for one deduplicated fetch per search is the correct backpressure behaviour. - Cross-reference comments in _verify_notes / _verify_deck_cards no longer claim "Mirrors X" pointing at functions defined later in the file; now use direction-neutral "parallel implementation in". - accessible_by_type is mutated by concurrent run_verifier tasks; a comment explains why this is race-free under anyio's cooperative multitasking (distinct keys per task, no await between read and write) so a future reader doesn't add a redundant lock. - Knock-on: tests/integration/test_rag.py wraps get_file_info in a try/except for the new contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/webdav.py | 34 ++++++----- nextcloud_mcp_server/models/semantic.py | 18 ++++-- nextcloud_mcp_server/search/verification.py | 67 +++++++++++++-------- tests/integration/test_rag.py | 15 ++++- tests/unit/client/test_webdav.py | 17 ++++-- tests/unit/search/test_verification.py | 29 ++++++++- 6 files changed, 123 insertions(+), 57 deletions(-) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 2d6356e5..0b314cb5 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -1306,7 +1306,19 @@ class WebDAVClient(BaseNextcloudClient): Returns: File info dictionary with id, name, size, content_type, etc. - Returns None if file not found. + Returns ``None`` ONLY when the server returned a malformed + PROPFIND response (missing ```` / + ```` / ```` elements) — an ambiguous + state where we cannot tell whether the file exists. + + Raises: + HTTPStatusError: For any non-2xx HTTP status, including 404 + ("not found"). Callers that want to treat 404 as + "absent" should catch ``HTTPStatusError`` and check + ``e.response.status_code``. This matches the convention + of the rest of this client and lets verify-on-read + distinguish a definitive absence (HTTP 404) from a + brittle response (None). """ webdav_path = f"{self._get_webdav_base_path()}/{path.lstrip('/')}" @@ -1323,19 +1335,13 @@ class WebDAVClient(BaseNextcloudClient): """ - try: - response = await self._client.request( - "PROPFIND", - webdav_path, - headers={"Depth": "0"}, - content=propfind_body, - ) - response.raise_for_status() - except HTTPStatusError as e: - if e.response.status_code == 404: - logger.debug(f"File not found: {path}") - return None - raise + response = await self._client.request( + "PROPFIND", + webdav_path, + headers={"Depth": "0"}, + content=propfind_body, + ) + response.raise_for_status() # Parse XML response root = ET.fromstring(response.content) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 8c882bdb..b583b098 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -83,17 +83,23 @@ class SemanticSearchResponse(BaseResponse): verified_count: int = Field( default=0, description=( - "Number of unique documents that passed verify-on-read access " - "checks (ADR-019). Equals len(results) before trimming to limit." + "Number of search result chunks that passed verify-on-read " + "access checks (ADR-019). Equals len(verified_results) before " + "trimming to limit. Note: multiple chunks of the same document " + "are counted separately here, whereas dropped_count counts " + "unique (doc_id, doc_type) pairs — the asymmetry is intentional " + "(verified_count is sized in result rows, dropped_count is " + "sized in unique ghost documents)." ), ) dropped_count: int = Field( default=0, description=( - "Number of unique documents dropped as ghost records during " - "verify-on-read (ADR-019). A short result page (len(results) < " - "limit) combined with a non-zero dropped_count indicates ghost " - "density rather than scarcity of relevant content." + "Number of unique (doc_id, doc_type) pairs dropped as ghost " + "records during verify-on-read (ADR-019). A short result page " + "(len(results) < limit) combined with a non-zero dropped_count " + "indicates ghost density rather than scarcity of relevant " + "content." ), ) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index af00bd1c..ced45d43 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -71,8 +71,9 @@ async def _verify_notes( async def check(result: SearchResult) -> None: doc_id = result.id # Parse defensively before the network call so a malformed payload - # produces a specific log line, not a generic "unexpected error" from - # the catch-all ``except Exception`` below. Mirrors ``_verify_deck_cards``. + # produces a specific log line, not a generic "unexpected error" + # from the catch-all ``except Exception`` below. The parallel + # implementation in ``_verify_deck_cards`` follows the same pattern. try: note_id_int = int(doc_id) except (TypeError, ValueError) as e: @@ -139,25 +140,19 @@ async def _verify_files( try: info = await client.webdav.get_file_info(file_path) if info is None: - # Contract: WebDAVClient.get_file_info returns None in two - # cases — (1) HTTP 404, and (2) a malformed PROPFIND XML - # response (missing , , or - # — see client/webdav.py). Both are treated as - # "inaccessible" and trigger eviction. - # - # Trade-off: a malformed response from a brittle backend - # could cause a *false* eviction. We accept that risk in - # exchange for correctness on real 404s — the index - # self-heals via re-indexing on the next scan, and - # malformed responses are exceedingly rare in practice. - # Distinguishing the two cases would require widening - # get_file_info's return contract; deferred to a future - # change if false evictions become observable. - # - # If the contract ever changes (e.g. 404 raises - # HTTPStatusError like other client methods), the - # `except HTTPStatusError` block below already handles - # it via _is_definitive_404_or_403. + # Contract (see WebDAVClient.get_file_info docstring): + # `None` means a malformed PROPFIND response — an + # ambiguous state, not a definitive 404. Treat as + # transient and KEEP the result rather than evicting. + # Real 404s raise HTTPStatusError and land in the + # _is_definitive_404_or_403 branch below. + logger.warning( + "Malformed PROPFIND response verifying file %s (%s); " + "keeping result (ambiguous state, not a definitive 404)", + doc_id, + file_path, + ) + accessible.add(doc_id) return accessible.add(doc_id) except HTTPStatusError as e: @@ -214,8 +209,9 @@ async def _verify_deck_cards( return # Parse defensively before the network call so a malformed payload - # produces a specific log line, not a generic "unexpected error" from - # the catch-all ``except Exception`` below. Mirrors ``_verify_news_items``. + # produces a specific log line, not a generic "unexpected error" + # from the catch-all ``except Exception`` below. The parallel + # implementation in ``_verify_news_items`` follows the same pattern. try: board_id_int = int(board_id) stack_id_int = int(stack_id) @@ -272,11 +268,21 @@ async def _verify_news_items( The Nextcloud News API has no per-item endpoint, so ``news.get_item`` is implemented as a fetch-all + filter — which would be O(N × all_items) if - called per id. Instead we fetch once and intersect. The semaphore is - accepted for signature symmetry but not heavily used (one round-trip total). + called per id. Instead we fetch once and intersect. Only one slot of + the shared semaphore is consumed per search (rather than one per id), + but that slot is held for the full ``get_items`` round-trip; see the + in-body comment for the backpressure rationale. """ doc_ids = [r.id for r in results] + # Semaphore lifetime: this slot is held for the duration of ONE + # deduplicated News fetch (≤1 per search), not per-id. That is the + # correct backpressure behaviour — a single user's news verification + # must not hammer Nextcloud with concurrent fetch-all requests, and + # any other verifiers running in parallel for the same search share + # the same semaphore. Latency of this fetch is proportional to the + # user's full news corpus; see the News caveat in + # docs/configuration.md for production guidance. async with semaphore: try: # TODO(perf): if profiling shows this fetch dominates query latency @@ -426,6 +432,17 @@ async def verify_search_results( # out 50 concurrent get_note calls and exhaust the connection pool. semaphore = anyio.Semaphore(max_concurrent) + # Concurrency note: ``accessible_by_type`` is mutated by multiple + # ``run_verifier`` tasks running under the task group below. This is + # safe without an explicit lock because (a) anyio uses cooperative + # multitasking — a task only yields at ``await`` points, never + # mid-statement; (b) each task is dispatched once per ``doc_type`` + # by the loop ``for doc_type, ... in by_type.items()`` further down, + # so two tasks never write to the same key; and (c) Python dict key + # assignment is not an await point, so two tasks cannot race on the + # same write. Adding a lock would be dead weight; using ``anyio.Lock`` + # here would force serialization on a path that is intentionally + # parallel. accessible_by_type: dict[str, set[int | str]] = {} async def run_verifier(doc_type: str, unique_results: list[SearchResult]) -> None: diff --git a/tests/integration/test_rag.py b/tests/integration/test_rag.py index c9876fd2..7df498cc 100644 --- a/tests/integration/test_rag.py +++ b/tests/integration/test_rag.py @@ -38,6 +38,7 @@ from typing import Any, AsyncGenerator import anyio import pytest +from httpx import HTTPStatusError from mcp import ClientSession from nextcloud_mcp_server.providers.base import Provider @@ -130,10 +131,18 @@ async def indexed_manual_pdf(nc_client, nc_mcp_client): logger.info(f"Setting up indexed manual PDF: {manual_path}") - # Get file info to verify file exists and get file ID - file_info = await nc_client.webdav.get_file_info(manual_path) + # Get file info to verify file exists and get file ID. After the + # round-7 contract widening, get_file_info raises HTTPStatusError on + # 404 instead of returning None — so wrap and skip on a definitive + # not-found. + try: + file_info = await nc_client.webdav.get_file_info(manual_path) + except HTTPStatusError as e: + if e.response.status_code == 404: + pytest.skip(f"Manual PDF not found at '{manual_path}'") + raise if not file_info: - pytest.skip(f"Manual PDF not found at '{manual_path}'") + pytest.skip(f"Manual PDF unreadable at '{manual_path}' (malformed PROPFIND)") file_id = file_info["id"] logger.info(f"Found manual PDF: {manual_path} (file_id={file_id})") diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index cb105cc6..0218d336 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -164,8 +164,14 @@ async def test_get_file_info_returns_file_details(mocker): @pytest.mark.unit -async def test_get_file_info_returns_none_for_missing_file(mocker): - """Test that get_file_info returns None for missing files.""" +async def test_get_file_info_raises_on_404(mocker): + """get_file_info now raises HTTPStatusError on 404 (was: returned None). + + The contract was widened so verify-on-read can distinguish a definitive + 404 from an ambiguous malformed-PROPFIND response. Callers that want + "absent → None" semantics should catch HTTPStatusError and check the + status code themselves. + """ from httpx import HTTPStatusError, Response mock_http_client = AsyncMock() @@ -180,11 +186,10 @@ async def test_get_file_info_returns_none_for_missing_file(mocker): ) ) - # Call get_file_info - result = await client.get_file_info("nonexistent.pdf") + with pytest.raises(HTTPStatusError) as exc_info: + await client.get_file_info("nonexistent.pdf") - # Verify result is None - assert result is None + assert exc_info.value.response.status_code == 404 @pytest.mark.unit diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 45017d8d..46d40ce3 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -317,9 +317,11 @@ async def test_verify_files_uses_path_from_metadata(mocker): @pytest.mark.unit -async def test_verify_files_404_via_get_file_info_drops(mocker): - """get_file_info returns None on 404 — that's a definitive drop.""" - webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) +async def test_verify_files_404_drops(mocker): + """get_file_info raising HTTPStatusError(404) is a definitive drop.""" + webdav_client = SimpleNamespace( + get_file_info=mocker.AsyncMock(side_effect=_http_error(404)) + ) client = SimpleNamespace(webdav=webdav_client, username="alice") result = await _verify_files( @@ -331,6 +333,27 @@ async def test_verify_files_404_via_get_file_info_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_files_malformed_propfind_keeps_result(mocker): + """get_file_info returning None means malformed PROPFIND — keep the result. + + Per the contract change in webdav.py: ``None`` is now reserved for the + ambiguous "malformed XML" case. Real 404s raise HTTPStatusError. The + file verifier must NOT evict on the ambiguous case (we cannot tell + whether the file exists), only log a warning and keep the result. + """ + webdav_client = SimpleNamespace(get_file_info=mocker.AsyncMock(return_value=None)) + client = SimpleNamespace(webdav=webdav_client, username="alice") + + result = await _verify_files( + client, + [_make_result(123, doc_type="file", metadata={"path": "brittle.txt"})], + _sem(), + ) + + assert result == {123}, "ambiguous None must keep result, not evict" + + @pytest.mark.unit async def test_verify_files_403_drops(mocker): """get_file_info raising HTTPStatusError(403) is a definitive drop."""