refactor(search): address PR #750 round 7 review feedback
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
e8df6003c5
commit
3e981e647a
@@ -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})")
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user