diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 5ef8a47f..68fd08bd 100644 --- a/nextcloud_mcp_server/app.py +++ b/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: diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 86456174..abfd5a43 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/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) diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 94612eca..eef33735 100644 --- a/tests/unit/search/test_verification.py +++ b/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."""