fix(mail): address PR #935 round-2 review

- server/mail.py: guard nc_mail_get_message against an empty OCS payload so
  MailMessage(**{}) can't raise an uncaught ValidationError (returns a clean
  'not found' instead).
- server/mail.py: cap inlined attachment content at MAX_ATTACHMENT_CONTENT_BYTES
  (5 MiB), replacing oversized bodies with a sentinel so a large attachment
  can't blow up the MCP response.
- client/mail.py: harden the OCS meta statuscode parse against a non-numeric
  value (treat as success) instead of letting int() raise an uncaught
  ValueError.
- scanner.py: log the newest-N cap hit once per (user, mailbox) at info level
  (discoverable without flooding multi-tenant logs on every scan tick).
- tests: add incremental-sync scanner cases (new message queued, reappeared
  message clears grace, deletion after grace expiry).

Deferred (tracked, card #376): include doc_type in the _potentially_deleted
grace-period key — a pre-existing cross-cutting collision the reviewer flagged
as a follow-up, not a blocker.

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

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(

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(

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,

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