From 1ed8362f7818ea24ef3f309ca91cf1a8280b8d73 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 23:00:42 +0200 Subject: [PATCH] refactor(search): address PR #750 round 11 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three minor fixes from the round-11 review on PR #750: - bm25_hybrid.py:209 — Comment said `doc_id` is `int (notes) or str (files)`, which is backwards. Notes, news_items, and deck_cards are stored as `str` (scanner.py:241, 666, 867); files are stored as `int` (scanner.py:425). Updated to point readers at scanner.py as the source of truth. - verification.py:338 — Lowered the News-API 403/404 log line from `info` to `debug`. The News app being uninstalled or disabled is a predictable operational state (matching the other verifiers' debug-on-not-found paths), so this should not generate operator-dashboard noise. Transient errors immediately below stay at `warning` because they're unexpected. - semantic.py:809 — `nc_get_vector_sync_status` was reading `document_receive_stream` via `getattr(..., None)`, but the attribute is guaranteed-defined on both `AppContext` and `OAuthAppContext` (as a field with `None` default). The defensive `getattr` masked typos that the eviction_task_group access at semantic.py:197-199 deliberately surfaces. Switched to direct access; the `if … is None:` value-check below is preserved (the attribute can legitimately be None before sync starts). Items deliberately deferred (with rationale in the plan file): - News verifier semaphore-hold during get_items (reviewer: "not required here, just worth tracking"; ADR already lists follow-ups). - Hardcoded 2× over-fetch / VERIFICATION_OVERFETCH (TODO already in code). - Integration test for the real Qdrant eviction filter (reviewer marked low-priority; type-preservation chain is unit-tested). Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/bm25_hybrid.py | 2 +- nextcloud_mcp_server/search/verification.py | 5 ++++- nextcloud_mcp_server/server/semantic.py | 12 ++++++++---- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index 243f9714..c5848450 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -208,7 +208,7 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): for result in search_response.points: if result.payload is None: continue - # doc_id can be int (notes) or str (files - file paths) + # doc_id can be int (files) or str (notes/news_items/deck_cards) — see scanner.py doc_id = result.payload["doc_id"] doc_type = result.payload.get("doc_type", "note") chunk_start = result.payload.get("chunk_start_offset") diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 073a9f49..ff6bdc02 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -335,7 +335,10 @@ async def _verify_news_items( # If the News API itself is gone (app disabled, user lost access), # treat *all* requested items as inaccessible. Eviction will reclaim. if _is_definitive_404_or_403(e): - logger.info( + # News app commonly disabled/uninstalled — debug-level keeps + # this off operator dashboards; transient errors below stay + # at warning because they're unexpected. + logger.debug( "News API returned %s for user %s; treating all %d news_items as inaccessible", e.response.status_code, client.username, diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 091e3ba1..ca5c9184 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -804,11 +804,15 @@ def configure_semantic_tools(mcp: FastMCP): ) try: - # Get document receive stream from lifespan context + # Get document receive stream from lifespan context. Direct + # attribute access matches the eviction_task_group pattern at + # ``nc_semantic_search`` (see comment there): both AppContext + # and OAuthAppContext define ``document_receive_stream``, so a + # missing attribute is a typo that should fail loudly. The + # value itself can legitimately be ``None`` before sync starts, + # which the check below handles. lifespan_ctx = ctx.request_context.lifespan_context - document_receive_stream = getattr( - lifespan_ctx, "document_receive_stream", None - ) + document_receive_stream = lifespan_ctx.document_receive_stream if document_receive_stream is None: logger.debug(