refactor(search): address PR #750 round 3 review feedback
- _verify_deck_cards: hoist int(board_id|stack_id|doc_id) out of the generic except Exception into an explicit try/except (TypeError, ValueError) before the network call, mirroring _verify_news_items. Malformed payloads now log a specific warning instead of "unexpected error". - _verify_news_items: add TODO(perf) above the get_items(batch_size=-1) call to mark the known fetch-all cost as a future profiling target. - SemanticSearchResult.id: revert from int|str back to int. The internal SearchResult.id stays int|str for forward-compat; the MCP response model narrows at the boundary. server/semantic.py casts r.id to int when constructing the response so future string-id types fail loudly here instead of silently widening the public API. - nc_semantic_search: replace the terse "extra for access filtering" comment with an ADR-019 NOTE block explaining the 2x over-fetch trade-off and the ghost-density under-delivery case (self-heals via lazy eviction). - tests/integration/test_verify_on_read.py: extend the module docstring to call out that only the note verifier is exercised against real Nextcloud, while file/deck_card/news_item are unit-only — documenting the suite split for future contributors. - ADR-019: rewrite "Module shape", "Verifier registry", example verifier, and "Deduplication" sections to match the shipped BatchVerifier interface (was per-id Verifier in the original draft). Add a "Why batch?" paragraph explaining the design choice. Update implementation checklist — every item is now [x] with corrected verifier names (plural) and the eviction module path (vector/eviction.py). 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
21e5608a39
commit
aa4b9498a1
@@ -10,11 +10,13 @@ from .base import BaseResponse
|
||||
class SemanticSearchResult(BaseModel):
|
||||
"""Model for semantic search results with additional metadata."""
|
||||
|
||||
id: int | str = Field(
|
||||
id: int = Field(
|
||||
description=(
|
||||
"Document ID. Numeric for all currently indexed types (notes, files, "
|
||||
"deck cards, news items); typed as int|str to allow future doc types "
|
||||
"that use string identifiers."
|
||||
"deck cards, news items). The internal SearchResult.id is typed as "
|
||||
"int|str to leave room for future doc types with string identifiers; "
|
||||
"the MCP response narrows to int and a future widening here would be "
|
||||
"a deliberate, breaking-by-design API change."
|
||||
)
|
||||
)
|
||||
doc_type: str = Field(
|
||||
|
||||
@@ -189,12 +189,31 @@ async def _verify_deck_cards(
|
||||
accessible.add(doc_id)
|
||||
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``.
|
||||
try:
|
||||
board_id_int = int(board_id)
|
||||
stack_id_int = int(stack_id)
|
||||
card_id_int = int(doc_id)
|
||||
except (TypeError, ValueError) as e:
|
||||
logger.warning(
|
||||
"Non-numeric deck metadata for card %s "
|
||||
"(board_id=%r, stack_id=%r): %s; keeping result",
|
||||
doc_id,
|
||||
board_id,
|
||||
stack_id,
|
||||
e,
|
||||
)
|
||||
accessible.add(doc_id)
|
||||
return
|
||||
|
||||
async with semaphore:
|
||||
try:
|
||||
await client.deck.get_card(
|
||||
board_id=int(board_id),
|
||||
stack_id=int(stack_id),
|
||||
card_id=int(doc_id),
|
||||
board_id=board_id_int,
|
||||
stack_id=stack_id_int,
|
||||
card_id=card_id_int,
|
||||
)
|
||||
accessible.add(doc_id)
|
||||
except HTTPStatusError as e:
|
||||
@@ -236,6 +255,11 @@ async def _verify_news_items(
|
||||
|
||||
async with semaphore:
|
||||
try:
|
||||
# TODO(perf): if profiling shows this fetch dominates query latency
|
||||
# for news-heavy users, cache the per-request item set or push for
|
||||
# a per-item News API endpoint. The shared semaphore protects
|
||||
# against runaway concurrent fetches, but the payload itself can
|
||||
# be large (News auto-purge cap is in the thousands of items).
|
||||
items = await client.news.get_items(batch_size=-1, get_read=True)
|
||||
except HTTPStatusError as e:
|
||||
# If the News API itself is gone (app disabled, user lost access),
|
||||
|
||||
@@ -120,11 +120,20 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
|
||||
if doc_types is None:
|
||||
# Cross-app search: search all indexed types
|
||||
# Get unverified results from Qdrant
|
||||
# Get unverified results from Qdrant.
|
||||
#
|
||||
# NOTE (ADR-019): Over-fetch by 2× to absorb ghost-record drops
|
||||
# during verify-on-read. When ghost density is high (e.g. a
|
||||
# large board share was just revoked) this budget can still
|
||||
# under-deliver against the requested ``limit``; the index
|
||||
# self-heals via lazy eviction so subsequent searches recover.
|
||||
# The 2× factor is a deliberate v1 trade-off — raising it
|
||||
# costs Nextcloud round-trips on every search. Trim to
|
||||
# ``limit`` happens AFTER verification.
|
||||
unverified_results = await search_algo.search(
|
||||
query=query,
|
||||
user_id=username,
|
||||
limit=limit * 2, # Get extra for access filtering
|
||||
limit=limit * 2,
|
||||
doc_type=None, # Signal to search all types
|
||||
score_threshold=score_threshold,
|
||||
)
|
||||
@@ -132,11 +141,12 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
else:
|
||||
# Search specific document types
|
||||
# For each requested type, execute search and combine results
|
||||
# under the same 2× over-fetch budget (see NOTE above).
|
||||
for dtype in doc_types:
|
||||
unverified_results = await search_algo.search(
|
||||
query=query,
|
||||
user_id=username,
|
||||
limit=limit * 2, # Get extra for combining and filtering
|
||||
limit=limit * 2,
|
||||
doc_type=dtype,
|
||||
score_threshold=score_threshold,
|
||||
)
|
||||
@@ -169,12 +179,18 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
)
|
||||
search_results = verified_results[:limit]
|
||||
|
||||
# Convert SearchResult objects to SemanticSearchResult for response
|
||||
# Convert SearchResult objects to SemanticSearchResult for response.
|
||||
# SearchResult.id is typed `int | str` for forward-compat with future
|
||||
# doc_types, but every currently indexed type uses numeric ids and
|
||||
# the MCP response model narrows to `int`. Casting here makes the
|
||||
# narrowing explicit and surfaces any future string-id type as a
|
||||
# loud failure at the boundary instead of silently widening the
|
||||
# public API.
|
||||
results = []
|
||||
for r in search_results:
|
||||
results.append(
|
||||
SemanticSearchResult(
|
||||
id=r.id,
|
||||
id=int(r.id),
|
||||
doc_type=r.doc_type,
|
||||
title=r.title,
|
||||
category=r.metadata.get("category", "") if r.metadata else "",
|
||||
|
||||
Reference in New Issue
Block a user