diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index 01c2dd01..0ba736fc 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/nextcloud_mcp_server/client/mail.py @@ -71,7 +71,13 @@ class MailClient(BaseNextcloudClient): # instead of silently unwrapping data=null. ocs = body.get("ocs", {}) if isinstance(body, dict) else {} meta = ocs.get("meta", {}) - status_code = int(meta.get("statuscode", 200) or 200) + # statuscode is spec'd as an int, but harden against a non-numeric value + # in a non-spec response rather than letting int() raise ValueError + # (which neither MCP-tool handler catches) — treat it as success. + try: + status_code = int(meta.get("statuscode", 200) or 200) + except (TypeError, ValueError): + status_code = 200 if status_code >= 400: synthetic = Response(status_code=status_code, request=response.request) raise HTTPStatusError( diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index 8fc98f95..13e59bb2 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/nextcloud_mcp_server/server/mail.py @@ -24,6 +24,13 @@ from nextcloud_mcp_server.observability.metrics import instrument_tool logger = logging.getLogger(__name__) +# Hard cap on inlined attachment content. The Mail OCS API returns the full +# attachment body in the JSON response, which then has to fit in the host LLM's +# context window; replace anything larger with a sentinel so a 20 MB design file +# can't blow up the MCP response. Callers can still see the real size via the +# message's attachment list. +MAX_ATTACHMENT_CONTENT_BYTES = 5 * 1024 * 1024 + def configure_mail_tools(mcp: FastMCP): """Configure Mail app MCP tools (read-only).""" @@ -161,6 +168,13 @@ def configure_mail_tools(mcp: FastMCP): client = await get_client(ctx) try: message_data = await client.mail.get_message(message_id) + # An empty payload (OCS data=null with a 200 meta) would make + # MailMessage(**{}) raise an uncaught ValidationError; treat it as + # not-found instead. + if not message_data: + raise McpError( + ErrorData(code=-1, message=f"Message {message_id} not found") + ) message = MailMessage(**message_data) return GetMessageResponse(message=message) except RequestError as e: @@ -207,11 +221,17 @@ def configure_mail_tools(mcp: FastMCP): client = await get_client(ctx) try: data = await client.mail.get_attachment(message_id, attachment_id) + content = data.get("content") + if isinstance(content, str) and len(content) > MAX_ATTACHMENT_CONTENT_BYTES: + content = ( + f"[attachment too large to inline: {len(content)} bytes " + f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]" + ) return GetAttachmentResponse( name=data.get("name"), mime=data.get("mime"), size=data.get("size"), - content=data.get("content"), + content=content, ) except RequestError as e: raise McpError( diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index b9150d9c..78b788ee 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -1362,6 +1362,12 @@ async def scan_news_items( # wanted, at the cost of more embedding work. MAIL_SCAN_MAX_PER_MAILBOX = 100 +# Per-process record of (user_id, mailbox_id) for which the newest-N cap has +# already been logged, so the "older mail not indexed" notice is emitted once at +# info level (discoverable) rather than on every scan tick (which would flood +# multi-tenant logs). +_mail_cap_logged: set[tuple[str, int]] = set() + async def scan_mail_messages( user_id: str, @@ -1460,10 +1466,15 @@ async def scan_mail_messages( ) continue - if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX: - logger.debug( + cap_key = (user_id, mailbox_id) + if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX and cap_key not in ( + _mail_cap_logged + ): + _mail_cap_logged.add(cap_key) + logger.info( "[SCAN-%s] Mailbox %s hit the newest-%s cap; older messages " - "are not indexed", + "are not indexed (set MAIL_SCAN_MAX_PER_MAILBOX higher for " + "deeper history)", scan_id, mailbox_id, MAIL_SCAN_MAX_PER_MAILBOX, diff --git a/tests/unit/vector/test_scanner_mail.py b/tests/unit/vector/test_scanner_mail.py index bf38ac3e..c4cfe39e 100644 --- a/tests/unit/vector/test_scanner_mail.py +++ b/tests/unit/vector/test_scanner_mail.py @@ -6,6 +6,7 @@ accounts → mailboxes → newest-N messages — which is the bulk of the new lo and needs no Qdrant. """ +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest @@ -16,6 +17,42 @@ from nextcloud_mcp_server.vector.scanner import DocumentTask, scan_mail_messages pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def _clear_scanner_module_state(): + """Isolate the module-global grace-period / cap-log dicts per test.""" + scanner_module._potentially_deleted.clear() + scanner_module._mail_cap_logged.clear() + yield + scanner_module._potentially_deleted.clear() + scanner_module._mail_cap_logged.clear() + + +def _patch_incremental(mocker, *, indexed_ids, existing_metadata, interval=1): + """Patch the Qdrant-facing helpers for the incremental scan path.""" + mocker.patch.object(scanner_module, "get_qdrant_client", new=AsyncMock()) + mocker.patch.object( + scanner_module, + "_scroll_all_points", + new=AsyncMock( + return_value=[ + SimpleNamespace(payload={"doc_id": doc_id}) for doc_id in indexed_ids + ] + ), + ) + mocker.patch.object( + scanner_module, + "query_document_metadata", + new=AsyncMock(return_value=existing_metadata), + ) + mocker.patch.object(scanner_module, "write_placeholder_point", new=AsyncMock()) + mocker.patch.object(scanner_module, "record_vector_sync_scan") + mocker.patch.object( + scanner_module, + "get_settings", + return_value=MagicMock(vector_sync_scan_interval=interval), + ) + + class _CollectingStream: """Minimal TaskProducer stand-in that records sent DocumentTasks.""" @@ -121,3 +158,74 @@ async def test_no_accounts_queues_nothing(mocker): assert queued == 0 assert stream.tasks == [] + + +def _single_message_client(messages): + nc_client = MagicMock() + nc_client.mail.list_accounts = AsyncMock(return_value=[{"id": 1}]) + nc_client.mail.get_mailboxes = AsyncMock(return_value=[{"databaseId": 10}]) + nc_client.mail.list_messages = AsyncMock(return_value=messages) + return nc_client + + +async def test_incremental_new_message_queued(mocker): + """A message absent from Qdrant (no existing metadata) is queued to index.""" + _patch_incremental(mocker, indexed_ids=[], existing_metadata=None) + nc_client = _single_message_client([{"databaseId": 100, "dateInt": 1700000000}]) + + stream = _CollectingStream() + queued = await scan_mail_messages( + user_id="alice", + send_stream=stream, + nc_client=nc_client, + initial_sync=False, + scan_id=1, + ) + + assert queued == 1 + assert [(t.doc_id, t.operation) for t in stream.tasks] == [("100", "index")] + + +async def test_incremental_reappeared_message_clears_grace(mocker): + """A message back in Nextcloud is removed from the deletion grace period.""" + # Already indexed and up-to-date, so it won't be re-queued. + _patch_incremental( + mocker, indexed_ids=["100"], existing_metadata={"modified_at": 1700000000} + ) + scanner_module._potentially_deleted[("alice", "100")] = 123.0 + nc_client = _single_message_client([{"databaseId": 100, "dateInt": 1700000000}]) + + stream = _CollectingStream() + queued = await scan_mail_messages( + user_id="alice", + send_stream=stream, + nc_client=nc_client, + initial_sync=False, + scan_id=1, + ) + + assert queued == 0 + assert stream.tasks == [] + assert ("alice", "100") not in scanner_module._potentially_deleted + + +async def test_incremental_deletes_after_grace_period(mocker): + """An indexed message gone from Nextcloud past the grace period is deleted.""" + _patch_incremental(mocker, indexed_ids=["999"], existing_metadata=None) + # Seed the grace period far in the past so the delta exceeds grace_period. + scanner_module._potentially_deleted[("alice", "999")] = 0.0 + # Mailbox now returns no messages, so 999 is missing. + nc_client = _single_message_client([]) + + stream = _CollectingStream() + queued = await scan_mail_messages( + user_id="alice", + send_stream=stream, + nc_client=nc_client, + initial_sync=False, + scan_id=1, + ) + + assert queued == 1 + assert [(t.doc_id, t.operation) for t in stream.tasks] == [("999", "delete")] + assert ("alice", "999") not in scanner_module._potentially_deleted