refactor(search): pre-push review fixes for PR #750

Address findings surfaced by `pre-push-review` after the round 8 sweep:

- Add deck verifier symmetry tests (404, transient 5xx, unexpected
  exception, non-numeric metadata) so deck has the same shape as the
  notes/news/files verifiers. Also add unexpected-exception tests for
  the news and file verifiers, which had `except Exception` branches
  no test was reaching. Keeps the registry-style verifier coverage
  uniform.
- Modernize sibling field types in `VectorSyncState`, `AppContext`,
  and `OAuthAppContext` from `Optional[X]` to `X | None`, matching the
  `eviction_task_group: TaskGroup | None` field added in the round 8
  diff (resolves the inconsistency flagged by A6). The lone remaining
  `Optional` import is dropped.
- Reverse cross-reference direction in the verifier docstrings: the
  later-defined `_verify_deck_cards` and `_verify_news_items` now
  point at `_verify_notes` as the canonical hoisted-cast pattern,
  rather than `_verify_notes` forward-referring to verifiers defined
  below it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/app.py

@@ -9,7 +9,7 @@ import traceback
 from collections.abc import AsyncIterator
 from contextlib import AsyncExitStack, asynccontextmanager
 from dataclasses import dataclass
-from typing import Optional, cast
+from typing import cast
 from urllib.parse import urlparse
 
 import anyio
@@ -316,10 +316,10 @@ class VectorSyncState:
     and FastMCP session lifespans (where MCP tools need access to the streams).
     """
 
-    document_send_stream: Optional[MemoryObjectSendStream] = None
-    document_receive_stream: Optional[MemoryObjectReceiveStream] = None
-    shutdown_event: Optional[anyio.Event] = None
-    scanner_wake_event: Optional[anyio.Event] = None
+    document_send_stream: MemoryObjectSendStream | None = None
+    document_receive_stream: MemoryObjectReceiveStream | None = None
+    shutdown_event: anyio.Event | None = None
+    scanner_wake_event: anyio.Event | None = None
     # Long-lived task group used for fire-and-forget background work spawned
     # from the request path (e.g. ADR-019 verify-on-read eviction). Set by the
     # starlette lifespan after entering its task group; cleared on shutdown.
@@ -335,11 +335,11 @@ class AppContext:
     """Application context for BasicAuth mode."""
 
     client: NextcloudClient
-    storage: Optional["RefreshTokenStorage"] = None
-    document_send_stream: Optional[MemoryObjectSendStream] = None
-    document_receive_stream: Optional[MemoryObjectReceiveStream] = None
-    shutdown_event: Optional[anyio.Event] = None
-    scanner_wake_event: Optional[anyio.Event] = None
+    storage: "RefreshTokenStorage | None" = None
+    document_send_stream: MemoryObjectSendStream | None = None
+    document_receive_stream: MemoryObjectReceiveStream | None = None
+    shutdown_event: anyio.Event | None = None
+    scanner_wake_event: anyio.Event | None = None
 
     @property
     def eviction_task_group(self) -> TaskGroup | None:
@@ -357,16 +357,14 @@ class OAuthAppContext:
 
     nextcloud_host: str
     token_verifier: object  # UnifiedTokenVerifier (ADR-005 compliant)
-    refresh_token_storage: Optional["RefreshTokenStorage"] = None
-    oauth_client: Optional[object] = None
+    refresh_token_storage: "RefreshTokenStorage | None" = None
+    oauth_client: object | None = None
     oauth_provider: str = "nextcloud"  # "nextcloud" or "keycloak"
-    server_client_id: Optional[str] = (
-        None  # MCP server's OAuth client ID (static or DCR)
-    )
-    document_send_stream: Optional[MemoryObjectSendStream] = None
-    document_receive_stream: Optional[MemoryObjectReceiveStream] = None
-    shutdown_event: Optional[anyio.Event] = None
-    scanner_wake_event: Optional[anyio.Event] = None
+    server_client_id: str | None = None  # MCP server's OAuth client ID (static or DCR)
+    document_send_stream: MemoryObjectSendStream | None = None
+    document_receive_stream: MemoryObjectReceiveStream | None = None
+    shutdown_event: anyio.Event | None = None
+    scanner_wake_event: anyio.Event | None = None
 
     @property
     def eviction_task_group(self) -> TaskGroup | None:

nextcloud_mcp_server/search/verification.py

@@ -72,8 +72,9 @@ async def _verify_notes(
         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. The parallel
-        # implementation in ``_verify_deck_cards`` follows the same pattern.
+        # from the catch-all ``except Exception`` below. ``_verify_notes``
+        # is the canonical shape; ``_verify_deck_cards`` and
+        # ``_verify_news_items`` mirror this hoisted-cast pattern.
         try:
             note_id_int = int(doc_id)
         except (TypeError, ValueError) as e:
@@ -210,8 +211,8 @@ async def _verify_deck_cards(
 
         # 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. The parallel
-        # implementation in ``_verify_news_items`` follows the same pattern.
+        # from the catch-all ``except Exception`` below. Mirrors the
+        # canonical hoisted-cast pattern in ``_verify_notes``.
         try:
             board_id_int = int(board_id)
             stack_id_int = int(stack_id)

tests/unit/search/test_verification.py

@@ -289,6 +289,31 @@ async def test_verify_news_items_transient_keeps_all(mocker):
     assert result == {1, 2, 3}
 
 
+@pytest.mark.unit
+async def test_verify_news_items_unexpected_exception_keeps_all(mocker):
+    """A non-HTTP exception from get_items must keep all results (fail open).
+
+    Covers the catch-all ``except Exception`` branch that exists so a bug
+    in the News client (or an httpx connection error) cannot silently shrink
+    the result set.
+    """
+    news_client = SimpleNamespace(
+        get_items=mocker.AsyncMock(side_effect=RuntimeError("news client boom"))
+    )
+    client = SimpleNamespace(news=news_client, username="alice")
+
+    result = await _verify_news_items(
+        client,
+        [
+            _make_result(1, doc_type="news_item"),
+            _make_result(2, doc_type="news_item"),
+        ],
+        _sem(),
+    )
+
+    assert result == {1, 2}
+
+
 @pytest.mark.unit
 async def test_verify_news_items_non_numeric_id_keeps_only_bad_item(mocker):
     """A non-numeric doc_id is fail-open per item, not per batch.
@@ -480,6 +505,28 @@ async def test_verify_files_transient_5xx_keeps(mocker):
     assert result == {7}
 
 
+@pytest.mark.unit
+async def test_verify_files_unexpected_exception_keeps(mocker):
+    """A non-HTTP exception from get_file_info must not drop the result.
+
+    The catch-all ``except Exception`` branch in the file verifier exists
+    so a bug in the WebDAV client (or an httpx ConnectError on a flaky
+    network) cannot silently shrink result pages.
+    """
+    webdav_client = SimpleNamespace(
+        get_file_info=mocker.AsyncMock(side_effect=RuntimeError("dav blew up"))
+    )
+    client = SimpleNamespace(webdav=webdav_client, username="alice")
+
+    result = await _verify_files(
+        client,
+        [_make_result(8, doc_type="file", metadata={"path": "y.txt"})],
+        _sem(),
+    )
+
+    assert result == {8}
+
+
 # ---------------------------------------------------------------------------
 # Deck card verifier
 # ---------------------------------------------------------------------------
@@ -530,6 +577,132 @@ async def test_verify_deck_cards_403_drops(mocker):
     assert result == set()
 
 
+@pytest.mark.unit
+async def test_verify_deck_cards_404_drops(mocker):
+    """Card deleted from the board → 404 from get_card → drop."""
+    deck_client = SimpleNamespace(
+        get_card=mocker.AsyncMock(side_effect=_http_error(404))
+    )
+    client = SimpleNamespace(deck=deck_client, username="alice")
+
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                42,
+                doc_type="deck_card",
+                metadata={"board_id": 1, "stack_id": 2},
+            )
+        ],
+        _sem(),
+    )
+
+    assert result == set()
+
+
+@pytest.mark.unit
+async def test_verify_deck_cards_transient_5xx_keeps(mocker):
+    """Transient 5xx from get_card must NOT silently shrink results."""
+    deck_client = SimpleNamespace(
+        get_card=mocker.AsyncMock(side_effect=_http_error(502))
+    )
+    client = SimpleNamespace(deck=deck_client, username="alice")
+
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                42,
+                doc_type="deck_card",
+                metadata={"board_id": 1, "stack_id": 2},
+            )
+        ],
+        _sem(),
+    )
+
+    assert result == {42}
+
+
+@pytest.mark.unit
+async def test_verify_deck_cards_unexpected_exception_keeps(mocker):
+    """Non-HTTP exception from get_card → fail-open, keep result."""
+    deck_client = SimpleNamespace(
+        get_card=mocker.AsyncMock(side_effect=RuntimeError("deck client boom"))
+    )
+    client = SimpleNamespace(deck=deck_client, username="alice")
+
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                42,
+                doc_type="deck_card",
+                metadata={"board_id": 1, "stack_id": 2},
+            )
+        ],
+        _sem(),
+    )
+
+    assert result == {42}
+
+
+@pytest.mark.unit
+async def test_verify_deck_cards_non_numeric_metadata_keeps(mocker):
+    """Non-numeric board_id/stack_id/card_id must fail open before the API call.
+
+    The hoisted ``int()`` casts in ``_verify_deck_cards`` produce a
+    type-specific log line; the catch-all path must never run.
+    """
+    deck_client = SimpleNamespace(
+        get_card=mocker.AsyncMock(side_effect=AssertionError("must not be called"))
+    )
+    client = SimpleNamespace(deck=deck_client, username="alice")
+
+    # Non-numeric board_id
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                42,
+                doc_type="deck_card",
+                metadata={"board_id": "not-a-number", "stack_id": 2},
+            )
+        ],
+        _sem(),
+    )
+    assert result == {42}
+
+    # Non-numeric stack_id
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                43,
+                doc_type="deck_card",
+                metadata={"board_id": 1, "stack_id": "bad"},
+            )
+        ],
+        _sem(),
+    )
+    assert result == {43}
+
+    # Non-numeric card_id (doc_id itself)
+    result = await _verify_deck_cards(
+        client,
+        [
+            _make_result(
+                "card-uuid",
+                doc_type="deck_card",
+                metadata={"board_id": 1, "stack_id": 2},
+            )
+        ],
+        _sem(),
+    )
+    assert result == {"card-uuid"}
+
+    deck_client.get_card.assert_not_awaited()
+
+
 @pytest.mark.unit
 async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker):
     """Legacy data without board_id/stack_id → keep, do NOT iterate or call API."""