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