refactor(mail): address PR #935 round-1 review

- models/mail.py: lowercase `list` generics per CLAUDE.md convention.
- client/mail.py: _ocs_get now inspects ocs.meta.statuscode (re-raises >=400
  as HTTPStatusError carrying the OCS code so callers' 404/403 handling
  applies) and guards response.json() against non-JSON bodies (RequestError).
- Extract the duplicated _format_addresses + content reconstruction into
  vector/mail_content.py, used by both processor.py and context.py (fixes the
  SonarCloud new_duplicated_lines_density gate).
- processor.py: add the missing mail_message Qdrant payload block so the
  computed mail metadata (subject/from/to/cc/date_int/has_attachments/
  account_id/mailbox_id) is actually stored, not dropped.
- Rename the list_messages `filter` param to `search_filter` (avoid shadowing
  builtins.filter); still maps to the OCS `filter` query param.
- Docstring notes: has_more heuristic, attachment content size.
- Tests: OCS meta-failure + non-JSON client paths; initial-sync scanner tests
  (tests/unit/vector/test_scanner_mail.py).

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

nextcloud_mcp_server/client/mail.py

@@ -15,6 +15,8 @@ OCS API controllers (Mail 5.x / Nextcloud 32+).
 import logging
 from typing import Any
 
+from httpx import HTTPStatusError, RequestError, Response
+
 from .base import BaseNextcloudClient
 
 logger = logging.getLogger(__name__)
@@ -50,9 +52,34 @@ class MailClient(BaseNextcloudClient):
             params=query,
             headers=self._OCS_HEADERS,
         )
-        body = response.json()
-        # Standard OCS envelope: {"ocs": {"meta": {...}, "data": <payload>}}
-        return body.get("ocs", {}).get("data")
+
+        # The Mail app being absent (or a misconfigured proxy) can return HTTP
+        # 200 with an HTML body; surface that as a network-style error rather
+        # than letting json() raise an opaque JSONDecodeError to the caller.
+        try:
+            body = response.json()
+        except ValueError as e:
+            raise RequestError(
+                f"Mail OCS returned a non-JSON response for {path}: {e}",
+                request=response.request,
+            ) from e
+
+        # Standard OCS envelope: {"ocs": {"meta": {...}, "data": <payload>}}.
+        # OCS can return HTTP 200 while signalling failure (e.g. 403/404) in
+        # ocs.meta.statuscode; re-raise those as an HTTPStatusError carrying the
+        # OCS code so callers' existing 404/403 handling applies uniformly
+        # 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)
+        if status_code >= 400:
+            synthetic = Response(status_code=status_code, request=response.request)
+            raise HTTPStatusError(
+                f"Mail OCS error {status_code} for {path}: {meta.get('message')}",
+                request=response.request,
+                response=synthetic,
+            )
+        return ocs.get("data")
 
     # --- Accounts ---
 
@@ -87,7 +114,7 @@ class MailClient(BaseNextcloudClient):
         mailbox_id: int,
         *,
         cursor: int | None = None,
-        filter: str | None = None,
+        search_filter: str | None = None,
         limit: int = 20,
         view: str | None = None,
     ) -> list[dict[str, Any]]:
@@ -99,7 +126,8 @@ class MailClient(BaseNextcloudClient):
         Args:
             mailbox_id: Numeric mailbox id (``databaseId`` from get_mailboxes)
             cursor: Pagination cursor (timestamp/id from a prior page)
-            filter: Optional search/filter query
+            search_filter: Optional search/filter query (maps to the OCS
+                ``filter`` query param; named to avoid shadowing ``builtins.filter``)
             limit: Max messages to return. Clamped server-side to 1..100; a
                 missing limit collapses to 1 server-side, so always pass one.
             view: ``"singleton"`` or ``"threaded"`` (default threaded)
@@ -111,8 +139,8 @@ class MailClient(BaseNextcloudClient):
         params: dict[str, Any] = {"limit": limit}
         if cursor is not None:
             params["cursor"] = cursor
-        if filter is not None:
-            params["filter"] = filter
+        if search_filter is not None:
+            params["filter"] = search_filter
         if view is not None:
             params["view"] = view
         data = await self._ocs_get(f"/mailboxes/{mailbox_id}/messages", params=params)

nextcloud_mcp_server/models/mail.py

@@ -1,7 +1,5 @@
 """Pydantic models for Nextcloud Mail app responses (read-only)."""
 
-from typing import List
-
 from pydantic import BaseModel, ConfigDict, Field
 
 from .base import BaseResponse
@@ -41,7 +39,7 @@ class MailMailbox(BaseModel):
         None, alias="displayName", description="Human-readable mailbox name"
     )
     account_id: int = Field(alias="accountId", description="Parent account ID")
-    special_use: List[str] = Field(
+    special_use: list[str] = Field(
         default_factory=list,
         alias="specialUse",
         description="Special-use roles (e.g. inbox, sent, trash)",
@@ -94,10 +92,10 @@ class MailMessageSummary(BaseModel):
     date_int: int | None = Field(
         None, alias="dateInt", description="Sent date as a Unix timestamp (seconds)"
     )
-    from_: List[MailAddress] = Field(
+    from_: list[MailAddress] = Field(
         default_factory=list, alias="from", description="Sender addresses"
     )
-    to: List[MailAddress] = Field(
+    to: list[MailAddress] = Field(
         default_factory=list, description="Recipient addresses"
     )
     mailbox_id: int | None = Field(
@@ -123,14 +121,14 @@ class MailMessage(BaseModel):
     date_int: int | None = Field(
         None, alias="dateInt", description="Sent date as a Unix timestamp (seconds)"
     )
-    from_: List[MailAddress] = Field(
+    from_: list[MailAddress] = Field(
         default_factory=list, alias="from", description="Sender addresses"
     )
-    to: List[MailAddress] = Field(
+    to: list[MailAddress] = Field(
         default_factory=list, description="Recipient addresses"
     )
-    cc: List[MailAddress] = Field(default_factory=list, description="CC addresses")
-    bcc: List[MailAddress] = Field(default_factory=list, description="BCC addresses")
+    cc: list[MailAddress] = Field(default_factory=list, description="CC addresses")
+    bcc: list[MailAddress] = Field(default_factory=list, description="BCC addresses")
     has_html_body: bool = Field(
         False, alias="hasHtmlBody", description="Whether the body is HTML"
     )
@@ -138,7 +136,7 @@ class MailMessage(BaseModel):
         None,
         description="Rendered body (sanitized HTML if hasHtmlBody, else plain text)",
     )
-    attachments: List[MailAttachment] = Field(
+    attachments: list[MailAttachment] = Field(
         default_factory=list, description="Message attachments"
     )
 
@@ -149,21 +147,21 @@ class MailMessage(BaseModel):
 class ListAccountsResponse(BaseResponse):
     """Response model for listing mail accounts."""
 
-    results: List[MailAccount] = Field(description="List of mail accounts")
+    results: list[MailAccount] = Field(description="List of mail accounts")
     total_count: int = Field(description="Total number of accounts")
 
 
 class ListMailboxesResponse(BaseResponse):
     """Response model for listing mailboxes."""
 
-    results: List[MailMailbox] = Field(description="List of mailboxes")
+    results: list[MailMailbox] = Field(description="List of mailboxes")
     total_count: int = Field(description="Total number of mailboxes")
 
 
 class ListMessagesResponse(BaseResponse):
     """Response model for listing message envelopes."""
 
-    results: List[MailMessageSummary] = Field(description="List of message summaries")
+    results: list[MailMessageSummary] = Field(description="List of message summaries")
     total_count: int = Field(description="Number of messages returned")
     has_more: bool = Field(False, description="Whether more messages may exist")
 

nextcloud_mcp_server/search/context.py

@@ -17,6 +17,7 @@ from nextcloud_mcp_server.models.deck import DeckCard
 from nextcloud_mcp_server.search.access_filter import build_ownership_filter
 from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id
 from nextcloud_mcp_server.vector.html_processor import html_to_markdown
+from nextcloud_mcp_server.vector.mail_content import build_mail_content
 from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter
 from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client
 
@@ -830,40 +831,10 @@ async def _fetch_document_text(
                     doc_id,
                 )
                 return None
-            # Reconstruct full content as indexed by the processor (subject +
-            # From + To + blank line + body) so chunk offsets align. Keep this in
-            # sync with the mail_message branch in vector/processor.py.
+            # Reconstruct full content via the shared helper so chunk offsets
+            # match what the processor indexed (single source of truth).
             message = await nc_client.mail.get_message(int(doc_id))
-
-            def _format_addresses(addrs: list[dict] | None) -> str:
-                parts = []
-                for addr in addrs or []:
-                    label = addr.get("label")
-                    email = addr.get("email")
-                    if label and email and label != email:
-                        parts.append(f"{label} <{email}>")
-                    elif email:
-                        parts.append(email)
-                    elif label:
-                        parts.append(label)
-                return ", ".join(parts)
-
-            subject = message.get("subject") or ""
-            from_str = _format_addresses(message.get("from"))
-            to_str = _format_addresses(message.get("to"))
-            raw_body = message.get("body") or ""
-            body_text = (
-                html_to_markdown(raw_body) if message.get("hasHtmlBody") else raw_body
-            )
-
-            content_parts = [subject]
-            if from_str:
-                content_parts.append(f"From: {from_str}")
-            if to_str:
-                content_parts.append(f"To: {to_str}")
-            content_parts.append("")  # Blank line
-            content_parts.append(body_text)
-            return "\n".join(content_parts)
+            return build_mail_content(message)
         else:
             logger.warning("Unsupported doc_type for context expansion: %s", doc_type)
             return None

nextcloud_mcp_server/server/mail.py

@@ -98,7 +98,7 @@ def configure_mail_tools(mcp: FastMCP):
         mailbox_id: int,
         ctx: Context,
         cursor: int | None = None,
-        filter: str | None = None,
+        search_filter: str | None = None,
         limit: int = 20,
     ) -> ListMessagesResponse:
         """List message envelopes in a mailbox, newest first (requires mail.read scope).
@@ -109,16 +109,19 @@ def configure_mail_tools(mcp: FastMCP):
         Args:
             mailbox_id: Numeric mailbox id (``database_id`` from nc_mail_list_mailboxes)
             cursor: Pagination cursor from a prior page
-            filter: Optional search/filter query
+            search_filter: Optional search/filter query
             limit: Max messages to return (1-100, default 20)
 
         Returns:
-            ListMessagesResponse with message summaries.
+            ListMessagesResponse with message summaries. ``has_more`` is a
+            heuristic (true when exactly ``limit`` messages were returned), so it
+            can be a false positive when a mailbox holds exactly ``limit``
+            messages; page with ``cursor`` and stop on an empty result.
         """
         client = await get_client(ctx)
         try:
             messages_data = await client.mail.list_messages(
-                mailbox_id, cursor=cursor, filter=filter, limit=limit
+                mailbox_id, cursor=cursor, search_filter=search_filter, limit=limit
             )
             messages = [MailMessageSummary(**m) for m in messages_data]
             return ListMessagesResponse(
@@ -196,7 +199,10 @@ def configure_mail_tools(mcp: FastMCP):
             attachment_id: Attachment id (a string, from the message's attachments)
 
         Returns:
-            GetAttachmentResponse with name, mime, size, and content.
+            GetAttachmentResponse with name, mime, size, and content. ``content``
+            is the attachment body as returned by the Mail OCS API; large
+            attachments produce a correspondingly large response, so prefer the
+            ``size`` from the message's attachment list before fetching.
         """
         client = await get_client(ctx)
         try:

nextcloud_mcp_server/vector/mail_content.py

@@ -0,0 +1,56 @@
+"""Shared reconstruction of mail-message content for indexing and context.
+
+The vector processor (index-time) and search context expansion (query-time)
+must build the *identical* text for a mail message so chunk offsets align.
+Keeping that logic here — rather than copy-pasted in both call sites — is the
+single source of truth for the reconstruction.
+"""
+
+from typing import Any
+
+from nextcloud_mcp_server.vector.html_processor import html_to_markdown
+
+
+def format_mail_addresses(addrs: list[dict[str, Any]] | None) -> str:
+    """Render a list of {label, email} address objects as a display string."""
+    parts: list[str] = []
+    for addr in addrs or []:
+        label = addr.get("label")
+        email = addr.get("email")
+        if label and email and label != email:
+            parts.append(f"{label} <{email}>")
+        elif email:
+            parts.append(email)
+        elif label:
+            parts.append(label)
+    return ", ".join(parts)
+
+
+def build_mail_content(message: dict[str, Any]) -> str:
+    """Reconstruct the indexed text body for a mail message.
+
+    Layout (kept stable so index-time and query-time offsets match):
+        <subject>
+        From: <from>
+        To: <to>
+        <blank line>
+        <body>
+
+    The body is the Mail OCS ``body`` field — sanitized HTML when
+    ``hasHtmlBody`` is set (converted to Markdown for embedding), otherwise
+    plain text.
+    """
+    subject = message.get("subject") or ""
+    from_str = format_mail_addresses(message.get("from"))
+    to_str = format_mail_addresses(message.get("to"))
+    raw_body = message.get("body") or ""
+    body_text = html_to_markdown(raw_body) if message.get("hasHtmlBody") else raw_body
+
+    content_parts = [subject]
+    if from_str:
+        content_parts.append(f"From: {from_str}")
+    if to_str:
+        content_parts.append(f"To: {to_str}")
+    content_parts.append("")  # Blank line
+    content_parts.append(body_text)
+    return "\n".join(content_parts)

nextcloud_mcp_server/vector/processor.py

@@ -53,6 +53,10 @@ from nextcloud_mcp_server.vector.document_chunker import (
     PageAwareChunker,
 )
 from nextcloud_mcp_server.vector.html_processor import html_to_markdown
+from nextcloud_mcp_server.vector.mail_content import (
+    build_mail_content,
+    format_mail_addresses,
+)
 from nextcloud_mcp_server.vector.placeholder import (
     delete_placeholder_point,
     update_placeholder_status,
@@ -842,50 +846,21 @@ async def _index_document(
             content_type = None
         elif doc_task.doc_type == "mail_message":
             # Fetch the full message via the Mail OCS API. The Mail app handles
-            # IMAP server-side; we only ever speak HTTP.
+            # IMAP server-side; we only ever speak HTTP. build_mail_content is
+            # shared with search/context.py so index- and query-time text match.
             message = await nc_client.mail.get_message(int(doc_task.doc_id))
-
-            def _format_addresses(addrs: list[dict] | None) -> str:
-                parts = []
-                for addr in addrs or []:
-                    label = addr.get("label")
-                    email = addr.get("email")
-                    if label and email and label != email:
-                        parts.append(f"{label} <{email}>")
-                    elif email:
-                        parts.append(email)
-                    elif label:
-                        parts.append(label)
-                return ", ".join(parts)
+            content = build_mail_content(message)
 
             subject = message.get("subject") or ""
-            from_str = _format_addresses(message.get("from"))
-            to_str = _format_addresses(message.get("to"))
-            # Body is sanitized HTML when hasHtmlBody, else plain text. Convert
-            # HTML to Markdown for better embedding; pass plain text through.
-            raw_body = message.get("body") or ""
-            body_text = (
-                html_to_markdown(raw_body) if message.get("hasHtmlBody") else raw_body
-            )
-
-            content_parts = [subject]
-            if from_str:
-                content_parts.append(f"From: {from_str}")
-            if to_str:
-                content_parts.append(f"To: {to_str}")
-            content_parts.append("")  # Blank line
-            content_parts.append(body_text)
-            content = "\n".join(content_parts)
-
             title = subject
             # Email is immutable; key change-detection on the message id so a
             # re-index is a no-op unless the id changes.
             etag = str(message.get("id") or doc_task.doc_id)
             file_metadata = {
                 "subject": subject,
-                "from": from_str,
-                "to": to_str,
-                "cc": _format_addresses(message.get("cc")),
+                "from": format_mail_addresses(message.get("from")),
+                "to": format_mail_addresses(message.get("to")),
+                "cc": format_mail_addresses(message.get("cc")),
                 "date_int": message.get("dateInt"),
                 "has_attachments": bool(message.get("attachments")),
                 "account_id": (doc_task.metadata or {}).get("account_id"),
@@ -1658,6 +1633,21 @@ async def _index_document(
                         if doc_task.doc_type == "deck_card"
                         else {}
                     ),
+                    # Mail message-specific metadata
+                    **(
+                        {
+                            "subject": file_metadata.get("subject"),
+                            "from": file_metadata.get("from"),
+                            "to": file_metadata.get("to"),
+                            "cc": file_metadata.get("cc"),
+                            "date_int": file_metadata.get("date_int"),
+                            "has_attachments": file_metadata.get("has_attachments"),
+                            "account_id": file_metadata.get("account_id"),
+                            "mailbox_id": file_metadata.get("mailbox_id"),
+                        }
+                        if doc_task.doc_type == "mail_message"
+                        else {}
+                    ),
                     # Chunk bbox (PDF only) — normalized rectangles in [0,1]
                     # relative to page width/height. Replaces the legacy
                     # `highlighted_page_image` (Deck #76). The page number

tests/client/mail/test_mail_api.py

@@ -108,7 +108,7 @@ async def test_list_messages_builds_params(mocker):
 
     client = MailClient(mock_client, "testuser")
     messages = await client.list_messages(
-        10, cursor=42, filter="hello", limit=50, view="threaded"
+        10, cursor=42, search_filter="hello", limit=50, view="threaded"
     )
 
     assert len(messages) == 1
@@ -204,3 +204,40 @@ async def test_empty_data_returns_empty_list(mocker):
 
     client = MailClient(mock_client, "testuser")
     assert await client.list_accounts() == []
+
+
+async def test_ocs_meta_failure_raises_httpstatuserror(mocker):
+    """HTTP 200 with an OCS meta failure code is re-raised as HTTPStatusError.
+
+    The synthetic response carries the OCS statuscode so callers' 404/403
+    handling applies (e.g. nc_mail_get_message maps 404 to 'not found').
+    """
+    mock_response = create_mock_response(
+        status_code=200,
+        json_data={
+            "ocs": {
+                "meta": {"status": "failure", "statuscode": 404, "message": "nope"},
+                "data": None,
+            }
+        },
+    )
+    mock_client = mocker.AsyncMock(spec=httpx.AsyncClient)
+    mocker.patch.object(MailClient, "_make_request", return_value=mock_response)
+
+    client = MailClient(mock_client, "testuser")
+    with pytest.raises(httpx.HTTPStatusError) as excinfo:
+        await client.get_message(100)
+    assert excinfo.value.response.status_code == 404
+
+
+async def test_non_json_response_raises_requesterror(mocker):
+    """A non-JSON 200 body (e.g. Mail app absent) raises a RequestError."""
+    mock_response = create_mock_response(
+        status_code=200, content=b"<html>not found</html>"
+    )
+    mock_client = mocker.AsyncMock(spec=httpx.AsyncClient)
+    mocker.patch.object(MailClient, "_make_request", return_value=mock_response)
+
+    client = MailClient(mock_client, "testuser")
+    with pytest.raises(httpx.RequestError):
+        await client.list_accounts()

tests/unit/vector/test_scanner_mail.py

@@ -0,0 +1,123 @@
+"""Unit tests for the mail-message scanner (initial-sync path).
+
+The incremental path depends on live Qdrant lookups (``_scroll_all_points`` /
+``query_document_metadata``); these tests cover the initial-sync enumeration —
+accounts → mailboxes → newest-N messages — which is the bulk of the new logic
+and needs no Qdrant.
+"""
+
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+
+from nextcloud_mcp_server.vector import scanner as scanner_module
+from nextcloud_mcp_server.vector.scanner import DocumentTask, scan_mail_messages
+
+pytestmark = pytest.mark.unit
+
+
+class _CollectingStream:
+    """Minimal TaskProducer stand-in that records sent DocumentTasks."""
+
+    def __init__(self) -> None:
+        self.tasks: list[DocumentTask] = []
+
+    async def send(self, task: DocumentTask) -> None:
+        self.tasks.append(task)
+
+
+async def test_initial_sync_enumerates_accounts_mailboxes_messages(mocker):
+    nc_client = MagicMock()
+    nc_client.mail.list_accounts = AsyncMock(return_value=[{"id": 1}])
+    nc_client.mail.get_mailboxes = AsyncMock(
+        return_value=[{"databaseId": 10}, {"databaseId": 11}]
+    )
+
+    async def list_messages(mailbox_id, *, limit):
+        if mailbox_id == 10:
+            return [
+                {"databaseId": 100, "dateInt": 1700000000},
+                {"databaseId": 101, "dateInt": 1700000001},
+            ]
+        return [{"databaseId": 200, "dateInt": 1700000002}]
+
+    nc_client.mail.list_messages = AsyncMock(side_effect=list_messages)
+
+    placeholder = mocker.patch.object(
+        scanner_module, "write_placeholder_point", new=AsyncMock()
+    )
+    mocker.patch.object(scanner_module, "record_vector_sync_scan")
+
+    stream = _CollectingStream()
+    queued = await scan_mail_messages(
+        user_id="alice",
+        send_stream=stream,
+        nc_client=nc_client,
+        initial_sync=True,
+        scan_id=1,
+    )
+
+    assert queued == 3
+    assert len(stream.tasks) == 3
+    # All are mail_message index tasks carrying account/mailbox metadata.
+    assert {t.doc_id for t in stream.tasks} == {"100", "101", "200"}
+    assert all(t.doc_type == "mail_message" for t in stream.tasks)
+    assert all(t.operation == "index" for t in stream.tasks)
+    t100 = next(t for t in stream.tasks if t.doc_id == "100")
+    assert t100.modified_at == 1700000000
+    assert t100.metadata == {"account_id": 1, "mailbox_id": 10}
+    # A placeholder is written per message before queueing.
+    assert placeholder.await_count == 3
+    # The per-mailbox cap is passed through.
+    nc_client.mail.list_messages.assert_any_await(
+        10, limit=scanner_module.MAIL_SCAN_MAX_PER_MAILBOX
+    )
+
+
+async def test_initial_sync_skips_mailbox_on_list_error(mocker):
+    """A failing mailbox is logged and skipped; other mailboxes still index."""
+    nc_client = MagicMock()
+    nc_client.mail.list_accounts = AsyncMock(return_value=[{"id": 1}])
+    nc_client.mail.get_mailboxes = AsyncMock(
+        return_value=[{"databaseId": 10}, {"databaseId": 11}]
+    )
+
+    async def list_messages(mailbox_id, *, limit):
+        if mailbox_id == 10:
+            raise RuntimeError("imap hiccup")
+        return [{"databaseId": 200, "dateInt": 1700000002}]
+
+    nc_client.mail.list_messages = AsyncMock(side_effect=list_messages)
+    mocker.patch.object(scanner_module, "write_placeholder_point", new=AsyncMock())
+    mocker.patch.object(scanner_module, "record_vector_sync_scan")
+
+    stream = _CollectingStream()
+    queued = await scan_mail_messages(
+        user_id="alice",
+        send_stream=stream,
+        nc_client=nc_client,
+        initial_sync=True,
+        scan_id=1,
+    )
+
+    assert queued == 1
+    assert {t.doc_id for t in stream.tasks} == {"200"}
+
+
+async def test_no_accounts_queues_nothing(mocker):
+    nc_client = MagicMock()
+    nc_client.mail.list_accounts = AsyncMock(return_value=[])
+    mocker.patch.object(scanner_module, "write_placeholder_point", new=AsyncMock())
+    mocker.patch.object(scanner_module, "record_vector_sync_scan")
+
+    stream = _CollectingStream()
+    queued = await scan_mail_messages(
+        user_id="alice",
+        send_stream=stream,
+        nc_client=nc_client,
+        initial_sync=True,
+        scan_id=1,
+    )
+
+    assert queued == 0
+    assert stream.tasks == []