From 3074622455f0214aae779fd0a6cbf3128e4bbf96 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 11:53:47 +0200 Subject: [PATCH 1/8] feat(mail): read and index Nextcloud Mail via the Mail OCS API Add read-only support for the Nextcloud Mail app, plus semantic indexing of mail messages. The MCP server never speaks IMAP/POP3 itself: it calls the Mail app's CSRF-free OCS API (/ocs/v2.php/apps/mail/api/...) with the existing Basic-Auth app-password flow and an OCS-APIRequest header, and the Mail app handles IMAP server-side. - client/mail.py: MailClient (accounts, mailboxes, messages, message, attachment), OCS-envelope aware. - models/mail.py: Pydantic models with the API's camelCase aliases. - server/mail.py: 5 read-only MCP tools (mail.read scope), registered in AVAILABLE_APPS. - Vector pipeline: new "mail_message" doc_type wired into scanner (scan_mail_messages, newest-N per mailbox), processor (body -> markdown embedding), per-id verifier, and context expansion. - Tests: client API, model round-trips, verifier behavior; consent-backstop test now derives its allowed set from INDEXED_DOC_TYPES. - README + semantic-search docstrings updated. Requires Mail 5.x / Nextcloud 32+ and a mail account configured in the Mail app. Follow-up: astrolabe must advertise "mail_message" in its enabled_doc_types capability for search under admin doc_type restriction. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 5 +- nextcloud_mcp_server/client/__init__.py | 2 + nextcloud_mcp_server/client/mail.py | 155 ++++++++++ nextcloud_mcp_server/models/auth.py | 1 + nextcloud_mcp_server/models/mail.py | 183 ++++++++++++ nextcloud_mcp_server/search/algorithms.py | 5 + nextcloud_mcp_server/search/context.py | 42 +++ nextcloud_mcp_server/search/verification.py | 57 ++++ nextcloud_mcp_server/server/__init__.py | 3 + nextcloud_mcp_server/server/mail.py | 224 ++++++++++++++ nextcloud_mcp_server/server/semantic.py | 4 +- nextcloud_mcp_server/vector/processor.py | 54 ++++ nextcloud_mcp_server/vector/scanner.py | 274 +++++++++++++++++- tests/client/mail/__init__.py | 0 tests/client/mail/test_mail_api.py | 206 +++++++++++++ tests/unit/search/test_verification.py | 92 ++++++ tests/unit/test_mail_models.py | 109 +++++++ .../vector/test_scanner_consent_backstop.py | 4 +- 18 files changed, 1413 insertions(+), 7 deletions(-) create mode 100644 nextcloud_mcp_server/client/mail.py create mode 100644 nextcloud_mcp_server/models/mail.py create mode 100644 nextcloud_mcp_server/server/mail.py create mode 100644 tests/client/mail/__init__.py create mode 100644 tests/client/mail/test_mail_api.py create mode 100644 tests/unit/test_mail_models.py diff --git a/README.md b/README.md index 8a99a60b..3b05e326 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,7 @@ For Kubernetes, see [cbcoutinho/helm-charts](https://github.com/cbcoutinho/helm- - **110+ MCP Tools** - Comprehensive API coverage across 10 Nextcloud apps - **MCP Resources** - Structured data URIs for browsing Nextcloud data -- **Semantic Search (Experimental)** - Optional vector-powered search for Notes, Files, News items, and Deck cards (requires Qdrant + Ollama) +- **Semantic Search (Experimental)** - Optional vector-powered search for Notes, Files, News items, Deck cards, and Mail messages (requires Qdrant + Ollama) - **Document Processing** - OCR and text extraction from PDFs, DOCX, images with progress notifications - **Flexible Deployment** - Docker, Kubernetes ([Helm chart](https://github.com/cbcoutinho/helm-charts)), VM, or local installation - **Production-Ready Auth** - Basic Auth with app passwords; multi-user via Login Flow v2 — MCP clients authenticate via OAuth, the server handles Nextcloud app passwords transparently @@ -90,9 +90,10 @@ For Kubernetes, see [cbcoutinho/helm-charts](https://github.com/cbcoutinho/helm- | **Tables** | 5 | Row operations on Nextcloud Tables | | **Sharing** | 10+ | Create and manage shares | | **News** | 8 | Feeds, folders, items, feed health monitoring | +| **Mail** | 5 | Read-only: accounts, mailboxes, messages, attachments (via Mail app's OCS API) | | **Collectives** | 16 | Full CRUD on collectives, pages, and tags | | **Talk (spreed)** | 6 | List conversations, read/post messages, mark as read, list participants | -| **Semantic Search** | 2+ | Vector search for Notes, Files, News items, and Deck cards (experimental, opt-in, requires infrastructure) | +| **Semantic Search** | 2+ | Vector search for Notes, Files, News items, Deck cards, and Mail messages (experimental, opt-in, requires infrastructure) | Want to see another Nextcloud app supported? [Open an issue](https://github.com/cbcoutinho/nextcloud-mcp-server/issues) or contribute a pull request! diff --git a/nextcloud_mcp_server/client/__init__.py b/nextcloud_mcp_server/client/__init__.py index 6891e47d..f8386fa3 100644 --- a/nextcloud_mcp_server/client/__init__.py +++ b/nextcloud_mcp_server/client/__init__.py @@ -20,6 +20,7 @@ from .contacts import ContactsClient from .cookbook import CookbookClient from .deck import DeckClient from .groups import GroupsClient +from .mail import MailClient from .news import NewsClient from .notes import NotesClient from .sharing import SharingClient @@ -144,6 +145,7 @@ class NextcloudClient: self.collectives = CollectivesClient(self._client, username) self.deck = DeckClient(self._client, username) self.news = NewsClient(self._client, username) + self.mail = MailClient(self._client, username) self.talk = TalkClient(self._client, username) self.users = UsersClient(self._client, username) self.groups = GroupsClient(self._client, username) diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py new file mode 100644 index 00000000..75e8a331 --- /dev/null +++ b/nextcloud_mcp_server/client/mail.py @@ -0,0 +1,155 @@ +"""Client for Nextcloud Mail app operations (read-only). + +Talks to the Mail app's OCS API under ``/ocs/v2.php/apps/mail/api/...``. The +Mail app's *server* handles the IMAP connection on the user's behalf, so this +client only ever speaks HTTP to Nextcloud — it never connects to IMAP/POP3 +itself. The read endpoints are ``#[NoCSRFRequired]`` + ``#[NoAdminRequired]``, +so they are reachable with the same Basic-Auth app-password flow the other app +clients use, provided the ``OCS-APIRequest`` header is sent. + +Prerequisites: the mail account must already be configured inside the Nextcloud +Mail app (so the server has IMAP credentials), and the Mail app must expose the +OCS API controllers (Mail 5.x / Nextcloud 32+). +""" + +import logging +from typing import Any + +from .base import BaseNextcloudClient + +logger = logging.getLogger(__name__) + + +class MailClient(BaseNextcloudClient): + """Read-only client for Nextcloud Mail app operations.""" + + app_name = "mail" + API_BASE = "/ocs/v2.php/apps/mail/api" + + # OCS endpoints require this header; without it Nextcloud rejects the + # request (or redirects to a login page). ``format=json`` forces a JSON + # envelope rather than XML. + _OCS_HEADERS = {"OCS-APIRequest": "true", "Accept": "application/json"} + + async def _ocs_get(self, path: str, *, params: dict[str, Any] | None = None) -> Any: + """GET an OCS endpoint and unwrap the ``ocs.data`` payload. + + Args: + path: Path under ``API_BASE`` (e.g. ``/account/list``) + params: Optional query params (``format=json`` is added automatically) + + Returns: + The ``data`` payload (a list, dict, or string depending on endpoint) + """ + query: dict[str, Any] = {"format": "json"} + if params: + query.update(params) + response = await self._make_request( + "GET", + f"{self.API_BASE}{path}", + params=query, + headers=self._OCS_HEADERS, + ) + body = response.json() + # Standard OCS envelope: {"ocs": {"meta": {...}, "data": }} + return body.get("ocs", {}).get("data") + + # --- Accounts --- + + async def list_accounts(self) -> list[dict[str, Any]]: + """List the user's configured mail accounts. + + Returns: + List of account objects (keys: id, email, isDelegated, aliases) + """ + data = await self._ocs_get("/account/list") + return data or [] + + # --- Mailboxes --- + + async def get_mailboxes(self, account_id: int) -> list[dict[str, Any]]: + """List the mailboxes (folders) of an account. + + Args: + account_id: Account ID (the ``id`` from :meth:`list_accounts`) + + Returns: + List of mailbox objects. Note ``databaseId`` is the numeric mailbox + id needed by :meth:`list_messages` (``id`` is a base64 string). + """ + data = await self._ocs_get("/mailboxes", params={"accountId": account_id}) + return data or [] + + # --- Messages --- + + async def list_messages( + self, + mailbox_id: int, + *, + cursor: int | None = None, + filter: str | None = None, + limit: int = 20, + view: str | None = None, + ) -> list[dict[str, Any]]: + """List message envelopes in a mailbox (newest first). + + Reads DB-cached envelope metadata, so this is fast and does not hit + IMAP per request. + + Args: + mailbox_id: Numeric mailbox id (``databaseId`` from get_mailboxes) + cursor: Pagination cursor (timestamp/id from a prior page) + filter: Optional search/filter query + 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) + + Returns: + List of message summary objects (keys include databaseId, subject, + from, to, dateInt, flags, previewText, mailboxId). + """ + params: dict[str, Any] = {"limit": limit} + if cursor is not None: + params["cursor"] = cursor + if filter is not None: + params["filter"] = filter + if view is not None: + params["view"] = view + data = await self._ocs_get(f"/mailboxes/{mailbox_id}/messages", params=params) + return data or [] + + async def get_message(self, message_id: int) -> dict[str, Any]: + """Get a single message with its full body. + + The Mail app fetches the body from IMAP server-side and returns it as a + single ``body`` field (sanitized HTML when ``hasHtmlBody`` is true, + otherwise plain text). ``body`` may be absent on partial (206) responses + when S/MIME decryption fails. + + Args: + message_id: Numeric message id (``databaseId`` from list_messages) + + Returns: + Full message object. + """ + data = await self._ocs_get(f"/message/{message_id}") + return data or {} + + async def get_attachment( + self, message_id: int, attachment_id: str + ) -> dict[str, Any]: + """Get a single attachment's metadata and content. + + The Mail OCS API returns the attachment as a JSON object (not a binary + download): keys ``name``, ``mime``, ``size``, ``content``. + + Args: + message_id: Numeric message id + attachment_id: Attachment id (a string; from the message's + ``attachments`` array) + + Returns: + Attachment object with name, mime, size, content. + """ + data = await self._ocs_get(f"/message/{message_id}/attachment/{attachment_id}") + return data or {} diff --git a/nextcloud_mcp_server/models/auth.py b/nextcloud_mcp_server/models/auth.py index 45a27f68..3b93ffce 100644 --- a/nextcloud_mcp_server/models/auth.py +++ b/nextcloud_mcp_server/models/auth.py @@ -74,6 +74,7 @@ ALL_SUPPORTED_SCOPES: frozenset[str] = frozenset( "sharing.write", "news.read", "news.write", + "mail.read", "collectives.read", "collectives.write", } diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py new file mode 100644 index 00000000..b084cbdf --- /dev/null +++ b/nextcloud_mcp_server/models/mail.py @@ -0,0 +1,183 @@ +"""Pydantic models for Nextcloud Mail app responses (read-only).""" + +from typing import List + +from pydantic import BaseModel, ConfigDict, Field + +from .base import BaseResponse + + +class MailAddress(BaseModel): + """An email address with an optional display label.""" + + model_config = ConfigDict(populate_by_name=True) + + label: str | None = Field(None, description="Display name") + email: str | None = Field(None, description="Email address") + + +class MailAccount(BaseModel): + """A configured mail account (from the account/list endpoint).""" + + model_config = ConfigDict(populate_by_name=True) + + id: int = Field(description="Account ID") + email: str = Field(description="Account email address") + is_delegated: bool = Field( + False, alias="isDelegated", description="Whether this is a delegated account" + ) + + +class MailMailbox(BaseModel): + """A mailbox (folder) within an account.""" + + model_config = ConfigDict(populate_by_name=True) + + # ``databaseId`` is the numeric id used by list_messages; ``id`` is a + # base64-encoded mailbox name (a string), so we expose the numeric one. + database_id: int = Field(alias="databaseId", description="Numeric mailbox ID") + name: str = Field(description="IMAP mailbox name (e.g. INBOX, Sent)") + display_name: str | None = Field( + None, alias="displayName", description="Human-readable mailbox name" + ) + account_id: int = Field(alias="accountId", description="Parent account ID") + special_use: List[str] = Field( + default_factory=list, + alias="specialUse", + description="Special-use roles (e.g. inbox, sent, trash)", + ) + unread: int = Field(0, description="Number of unread messages") + + +class MailMessageFlags(BaseModel): + """IMAP flags on a message.""" + + model_config = ConfigDict(populate_by_name=True) + + seen: bool = False + flagged: bool = False + answered: bool = False + deleted: bool = False + draft: bool = False + forwarded: bool = False + has_attachments: bool = Field(False, alias="hasAttachments") + important: bool = False + + +class MailAttachment(BaseModel): + """Metadata for a message attachment.""" + + model_config = ConfigDict(populate_by_name=True) + + # Attachment id is a string and may be null for inline/body-part messages. + id: str | None = Field(None, description="Attachment ID") + file_name: str | None = Field( + None, alias="fileName", description="Attachment file name" + ) + mime: str | None = Field(None, description="MIME type") + size: int | None = Field(None, description="Size in bytes") + cid: str | None = Field(None, description="Content-ID (for inline attachments)") + disposition: str | None = Field( + None, description="Content disposition (attachment/inline)" + ) + + +class MailMessageSummary(BaseModel): + """Lightweight message envelope for mailbox listings.""" + + model_config = ConfigDict(populate_by_name=True) + + # ``databaseId`` is the numeric id passed to get_message. + database_id: int = Field(alias="databaseId", description="Numeric message ID") + uid: int | None = Field(None, description="IMAP UID") + subject: str | None = Field(None, description="Message subject") + date_int: int | None = Field( + None, alias="dateInt", description="Sent date as a Unix timestamp (seconds)" + ) + from_: List[MailAddress] = Field( + default_factory=list, alias="from", description="Sender addresses" + ) + to: List[MailAddress] = Field( + default_factory=list, description="Recipient addresses" + ) + mailbox_id: int | None = Field( + None, alias="mailboxId", description="Parent mailbox ID" + ) + preview_text: str | None = Field( + None, alias="previewText", description="Short preview snippet" + ) + flags: MailMessageFlags | None = Field(None, description="IMAP flags") + + +class MailMessage(BaseModel): + """Full message with body (from the message/{id} endpoint).""" + + model_config = ConfigDict(populate_by_name=True) + + id: int = Field(description="Numeric message ID") + uid: int | None = Field(None, description="IMAP UID") + message_id: str | None = Field( + None, alias="messageId", description="RFC Message-ID header" + ) + subject: str | None = Field(None, description="Message subject") + date_int: int | None = Field( + None, alias="dateInt", description="Sent date as a Unix timestamp (seconds)" + ) + from_: List[MailAddress] = Field( + default_factory=list, alias="from", description="Sender addresses" + ) + 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") + has_html_body: bool = Field( + False, alias="hasHtmlBody", description="Whether the body is HTML" + ) + body: str | None = Field( + None, + description="Rendered body (sanitized HTML if hasHtmlBody, else plain text)", + ) + attachments: List[MailAttachment] = Field( + default_factory=list, description="Message attachments" + ) + + +# --- Response Models --- + + +class ListAccountsResponse(BaseResponse): + """Response model for listing 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") + 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") + total_count: int = Field(description="Number of messages returned") + has_more: bool = Field(False, description="Whether more messages may exist") + + +class GetMessageResponse(BaseResponse): + """Response model for getting a single full message.""" + + message: MailMessage = Field(description="Full message details") + + +class GetAttachmentResponse(BaseResponse): + """Response model for getting a single attachment.""" + + name: str | None = Field(None, description="Attachment file name") + mime: str | None = Field(None, description="MIME type") + size: int | None = Field(None, description="Size in bytes") + content: str | None = Field(None, description="Attachment content") diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index b4990b0c..9a228f01 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -76,6 +76,11 @@ class NextcloudClientProtocol(Protocol): """News client for accessing news item documents.""" ... + @property + def mail(self) -> Any: + """Mail client for accessing mail message documents.""" + ... + # Top-level client helper (not a sub-client) used by verify-on-read to # gate file results on current vector-index tag membership. async def find_files_by_tag( diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index d8d898fd..8c484044 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -822,6 +822,48 @@ async def _fetch_document_text( if card.description: content_parts.append(card.description) return "\n\n".join(content_parts) + elif doc_type == "mail_message": + # Mail message IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + if not is_valid_nextcloud_doc_id(doc_id): + logger.warning( + "Expected numeric mail_message doc_id, got %r — skipping document fetch", + 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. + 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) else: logger.warning("Unsupported doc_type for context expansion: %s", doc_type) return None diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 1192d5fb..9d319021 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -488,11 +488,68 @@ async def _verify_news_items( return accessible +async def _verify_mail_messages( + client: NextcloudClientProtocol, + results: list[SearchResult], + semaphore: anyio.Semaphore, +) -> set[str]: + """Verify mail messages per-id via the Mail OCS ``get_message`` endpoint. + + Mirrors ``_verify_notes``: a definitive 403/404 (message deleted, account + removed, or Mail app disabled) drops the result and schedules eviction; + transient errors and non-numeric ids fail open (keep the result). + """ + # safe: cooperative concurrency, no lock needed (see verify_search_results) + accessible: set[str] = set() + + async def check(result: SearchResult) -> None: + doc_id = result.id + try: + message_id_int = int(doc_id) + except (TypeError, ValueError) as e: + logger.warning( + "Non-numeric mail message id %r: %s; keeping result", + doc_id, + e, + ) + accessible.add(doc_id) + return + + async with semaphore: + try: + await client.mail.get_message(message_id_int) + accessible.add(doc_id) + except HTTPStatusError as e: + if _is_definitive_404_or_403(e): + return + logger.warning( + "Transient error verifying mail message %s: %s %s; keeping result", + doc_id, + e.response.status_code, + e, + ) + accessible.add(doc_id) + except Exception as e: + logger.warning( + "Unexpected error verifying mail message %s: %s; keeping result", + doc_id, + e, + ) + accessible.add(doc_id) + + async with anyio.create_task_group() as tg: + for r in results: + tg.start_soon(check, r) + + return accessible + + _VERIFIERS: dict[str, BatchVerifier] = { "note": _verify_notes, "file": _verify_files, "deck_card": _verify_deck_cards, "news_item": _verify_news_items, + "mail_message": _verify_mail_messages, } diff --git a/nextcloud_mcp_server/server/__init__.py b/nextcloud_mcp_server/server/__init__.py index e2f4616b..42428a65 100644 --- a/nextcloud_mcp_server/server/__init__.py +++ b/nextcloud_mcp_server/server/__init__.py @@ -7,6 +7,7 @@ from .collectives import configure_collectives_tools from .contacts import configure_contacts_tools from .cookbook import configure_cookbook_tools from .deck import configure_deck_tools +from .mail import configure_mail_tools from .news import configure_news_tools from .notes import configure_notes_tools from .semantic import configure_semantic_tools @@ -30,6 +31,7 @@ AVAILABLE_APPS: dict[str, Callable[[FastMCP], None]] = { "cookbook": configure_cookbook_tools, "deck": configure_deck_tools, "news": configure_news_tools, + "mail": configure_mail_tools, "talk": configure_talk_tools, } @@ -40,6 +42,7 @@ __all__ = [ "configure_contacts_tools", "configure_cookbook_tools", "configure_deck_tools", + "configure_mail_tools", "configure_news_tools", "configure_notes_tools", "configure_semantic_tools", diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py new file mode 100644 index 00000000..cdda67ca --- /dev/null +++ b/nextcloud_mcp_server/server/mail.py @@ -0,0 +1,224 @@ +"""MCP tools for Nextcloud Mail app (read-only).""" + +import logging + +from httpx import HTTPStatusError, RequestError +from mcp.server.fastmcp import Context, FastMCP +from mcp.shared.exceptions import McpError +from mcp.types import ErrorData, ToolAnnotations + +from nextcloud_mcp_server.auth import require_scopes +from nextcloud_mcp_server.context import get_client +from nextcloud_mcp_server.models.mail import ( + GetAttachmentResponse, + GetMessageResponse, + ListAccountsResponse, + ListMailboxesResponse, + ListMessagesResponse, + MailAccount, + MailMailbox, + MailMessage, + MailMessageSummary, +) +from nextcloud_mcp_server.observability.metrics import instrument_tool + +logger = logging.getLogger(__name__) + + +def configure_mail_tools(mcp: FastMCP): + """Configure Mail app MCP tools (read-only).""" + + @mcp.tool( + title="List Mail Accounts", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("mail.read") + @instrument_tool + async def nc_mail_list_accounts(ctx: Context) -> ListAccountsResponse: + """List the user's configured mail accounts (requires mail.read scope).""" + client = await get_client(ctx) + try: + accounts_data = await client.mail.list_accounts() + accounts = [MailAccount(**a) for a in accounts_data] + return ListAccountsResponse(results=accounts, total_count=len(accounts)) + except RequestError as e: + raise McpError( + ErrorData(code=-1, message=f"Network error listing accounts: {str(e)}") + ) + except HTTPStatusError as e: + raise McpError( + ErrorData( + code=-1, + message=f"Failed to list accounts: {e.response.status_code}", + ) + ) + + @mcp.tool( + title="List Mail Mailboxes", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("mail.read") + @instrument_tool + async def nc_mail_list_mailboxes( + account_id: int, ctx: Context + ) -> ListMailboxesResponse: + """List the mailboxes (folders) of a mail account (requires mail.read scope). + + Args: + account_id: Account ID (from nc_mail_list_accounts) + + Returns: + ListMailboxesResponse with mailboxes. Use a mailbox's ``database_id`` + with nc_mail_list_messages. + """ + client = await get_client(ctx) + try: + mailboxes_data = await client.mail.get_mailboxes(account_id) + mailboxes = [MailMailbox(**m) for m in mailboxes_data] + return ListMailboxesResponse(results=mailboxes, total_count=len(mailboxes)) + except RequestError as e: + raise McpError( + ErrorData(code=-1, message=f"Network error listing mailboxes: {str(e)}") + ) + except HTTPStatusError as e: + raise McpError( + ErrorData( + code=-1, + message=f"Failed to list mailboxes: {e.response.status_code}", + ) + ) + + @mcp.tool( + title="List Mail Messages", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("mail.read") + @instrument_tool + async def nc_mail_list_messages( + mailbox_id: int, + ctx: Context, + cursor: int | None = None, + filter: str | None = None, + limit: int = 20, + ) -> ListMessagesResponse: + """List message envelopes in a mailbox, newest first (requires mail.read scope). + + Reads cached envelope metadata (fast); does not fetch bodies. Use + nc_mail_get_message to fetch a full body. + + 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 + limit: Max messages to return (1-100, default 20) + + Returns: + ListMessagesResponse with message summaries. + """ + client = await get_client(ctx) + try: + messages_data = await client.mail.list_messages( + mailbox_id, cursor=cursor, filter=filter, limit=limit + ) + messages = [MailMessageSummary(**m) for m in messages_data] + return ListMessagesResponse( + results=messages, + total_count=len(messages), + has_more=len(messages) == limit and limit > 0, + ) + except RequestError as e: + raise McpError( + ErrorData(code=-1, message=f"Network error listing messages: {str(e)}") + ) + except HTTPStatusError as e: + raise McpError( + ErrorData( + code=-1, + message=f"Failed to list messages: {e.response.status_code}", + ) + ) + + @mcp.tool( + title="Get Mail Message", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("mail.read") + @instrument_tool + async def nc_mail_get_message(message_id: int, ctx: Context) -> GetMessageResponse: + """Get a single mail message with its full body (requires mail.read scope). + + The Mail app fetches the body from IMAP server-side. + + Args: + message_id: Numeric message id (``database_id`` from nc_mail_list_messages) + + Returns: + GetMessageResponse with the full message including body and attachments. + """ + client = await get_client(ctx) + try: + message_data = await client.mail.get_message(message_id) + message = MailMessage(**message_data) + return GetMessageResponse(message=message) + except RequestError as e: + raise McpError( + ErrorData( + code=-1, + message=f"Network error getting message {message_id}: {str(e)}", + ) + ) + except HTTPStatusError as e: + if e.response.status_code == 404: + raise McpError( + ErrorData(code=-1, message=f"Message {message_id} not found") + ) + raise McpError( + ErrorData( + code=-1, + message=f"Failed to get message {message_id}: " + f"{e.response.status_code}", + ) + ) + + @mcp.tool( + title="Get Mail Attachment", + annotations=ToolAnnotations(readOnlyHint=True, openWorldHint=True), + ) + @require_scopes("mail.read") + @instrument_tool + async def nc_mail_get_attachment( + message_id: int, attachment_id: str, ctx: Context + ) -> GetAttachmentResponse: + """Get a single mail attachment's metadata and content (requires mail.read scope). + + Args: + message_id: Numeric message id + attachment_id: Attachment id (a string, from the message's attachments) + + Returns: + GetAttachmentResponse with name, mime, size, and content. + """ + client = await get_client(ctx) + try: + data = await client.mail.get_attachment(message_id, attachment_id) + return GetAttachmentResponse( + name=data.get("name"), + mime=data.get("mime"), + size=data.get("size"), + content=data.get("content"), + ) + except RequestError as e: + raise McpError( + ErrorData( + code=-1, message=f"Network error getting attachment: {str(e)}" + ) + ) + except HTTPStatusError as e: + if e.response.status_code == 404: + raise McpError(ErrorData(code=-1, message="Attachment not found")) + raise McpError( + ErrorData( + code=-1, + message=f"Failed to get attachment: {e.response.status_code}", + ) + ) diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 27f84102..20daa203 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -214,12 +214,12 @@ def configure_semantic_tools(mcp: FastMCP): understanding and keyword precision. Requires VECTOR_SYNC_ENABLED=true. Supports indexing of notes, files, - news items, and deck cards. + news items, deck cards, and mail messages. Args: query: Natural language or keyword search query limit: Maximum number of results to return (default: 10) - doc_types: Document types to search (e.g., ["note", "file", "deck_card", "news_item"]). None = search all indexed types (default) + doc_types: Document types to search (e.g., ["note", "file", "deck_card", "news_item", "mail_message"]). None = search all indexed types (default) score_threshold: Minimum fusion score (0-1, default: 0.0) fusion: Fusion algorithm: "rrf" (Reciprocal Rank Fusion, default) or "dbsf" (Distribution-Based Score Fusion) RRF: Good general-purpose fusion using reciprocal ranks diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index cc421b7e..c63462b0 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -840,6 +840,60 @@ async def _index_document( file_path = None content_bytes = None 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. + 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) + + 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")), + "date_int": message.get("dateInt"), + "has_attachments": bool(message.get("attachments")), + "account_id": (doc_task.metadata or {}).get("account_id"), + "mailbox_id": (doc_task.metadata or {}).get("mailbox_id"), + } + file_path = None + content_bytes = None + content_type = None elif doc_task.doc_type == "deck_card": # Fetch card from Deck API # Use metadata from scanner if available (O(1) lookup) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index fdf381e7..b9150d9c 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -48,7 +48,7 @@ logger = logging.getLogger(__name__) # same PR that adds a new indexed doc_type, or accept ghost-record exposure for # that type (see ADR-019). INDEXED_DOC_TYPES: frozenset[str] = frozenset( - {"note", "file", "deck_card", "news_item"} + {"note", "file", "deck_card", "news_item", "mail_message"} ) @@ -956,13 +956,36 @@ async def scan_user_documents( user_id, ) + # Scan Mail messages (newest per mailbox) + mail_queued = 0 + if _should_scan("mail", "mail_message", enabled_apps, allowed): + try: + mail_queued = await scan_mail_messages( + user_id=user_id, + send_stream=send_stream, + nc_client=nc_client, + initial_sync=initial_sync, + scan_id=scan_id, + ) + queued += mail_queued + except Exception as e: + logger.warning("Failed to scan mail messages for %s: %s", user_id, e) + else: + logger.debug( + "[SCAN-%s] Mail app not enabled for %s; skipping mail messages", + scan_id, + user_id, + ) + if queued > 0: logger.info( - "Sent %s documents (%s files, %s news items, %s deck cards) for incremental sync: %s", + "Sent %s documents (%s files, %s news items, %s deck cards, " + "%s mail messages) for incremental sync: %s", queued, file_queued, news_queued, deck_queued, + mail_queued, user_id, ) else: @@ -1332,6 +1355,253 @@ async def scan_news_items( return queued +# Newest-N messages indexed per mailbox. The Mail app paginates by recency, so +# this bounds the index to recent mail; older messages age out of the index as +# newer ones arrive (and the deletion-tracking pass below evicts them, the same +# way it handles actually-deleted messages). Raise this if deeper history is +# wanted, at the cost of more embedding work. +MAIL_SCAN_MAX_PER_MAILBOX = 100 + + +async def scan_mail_messages( + user_id: str, + send_stream: TaskProducer, + nc_client: NextcloudClient, + initial_sync: bool, + scan_id: int, +) -> int: + """ + Scan a user's Mail messages and queue changed messages for indexing. + + Enumerates accounts → mailboxes → newest ``MAIL_SCAN_MAX_PER_MAILBOX`` + messages per mailbox. Email is immutable, so a message's ``dateInt`` (sent + timestamp) is used as the change-detection ``modified_at`` — a message is + indexed once and not re-sent. Messages that drop out of the newest-N window + (or are deleted) are evicted via the deletion-tracking pass, keeping the + index bounded to recent mail. + + The MCP server never speaks IMAP: listing reads the Mail app's DB-cached + envelopes, and the body fetch (in the processor) goes through the Mail app's + OCS API, which handles IMAP server-side. + + Args: + user_id: User to scan + send_stream: Stream to send changed documents to processors + nc_client: Authenticated Nextcloud client + initial_sync: If True, send all documents (first-time sync) + scan_id: Scan identifier for logging + + Returns: + Number of messages queued for processing + """ + settings = get_settings() + queued = 0 + + # Get indexed mail message IDs from Qdrant (for deletion tracking) + indexed_message_ids: set[str] = set() + if not initial_sync: + qdrant_client = await get_qdrant_client() + points = await _scroll_all_points( + qdrant_client, + collection_name=settings.get_collection_name(), + scroll_filter=Filter( + must=[ + FieldCondition(key="user_id", match=MatchValue(value=user_id)), + FieldCondition( + key="doc_type", match=MatchValue(value="mail_message") + ), + ] + ), + payload_fields=["doc_id"], + ) + indexed_message_ids = { + str(point.payload["doc_id"]) + for point in points + if point.payload is not None and "doc_id" in point.payload + } + logger.debug( + "Found %s indexed mail messages in Qdrant", len(indexed_message_ids) + ) + + # Enumerate accounts → mailboxes → newest-N messages. + accounts = await nc_client.mail.list_accounts() + nextcloud_message_ids: set[str] = set() + message_count = 0 + + for account in accounts: + account_id = account.get("id") + if account_id is None: + continue + try: + mailboxes = await nc_client.mail.get_mailboxes(account_id) + except Exception as e: + logger.warning( + "[SCAN-%s] Failed to list mailboxes for account %s: %s", + scan_id, + account_id, + e, + ) + continue + + for mailbox in mailboxes: + mailbox_id = mailbox.get("databaseId") + if mailbox_id is None: + continue + try: + messages = await nc_client.mail.list_messages( + mailbox_id, limit=MAIL_SCAN_MAX_PER_MAILBOX + ) + except Exception as e: + logger.warning( + "[SCAN-%s] Failed to list messages for mailbox %s: %s", + scan_id, + mailbox_id, + e, + ) + continue + + if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX: + logger.debug( + "[SCAN-%s] Mailbox %s hit the newest-%s cap; older messages " + "are not indexed", + scan_id, + mailbox_id, + MAIL_SCAN_MAX_PER_MAILBOX, + ) + + for message in messages: + msg_db_id = message.get("databaseId") + if msg_db_id is None: + continue + doc_id = str(msg_db_id) + nextcloud_message_ids.add(doc_id) + message_count += 1 + + modified_at = message.get("dateInt", 0) or 0 + task_metadata: dict[str, int | str] = { + "account_id": account_id, + "mailbox_id": mailbox_id, + } + + if initial_sync: + await write_placeholder_point( + doc_id=doc_id, + doc_type="mail_message", + user_id=user_id, + modified_at=modified_at, + ) + await send_stream.send( + DocumentTask( + user_id=user_id, + doc_id=doc_id, + doc_type="mail_message", + operation="index", + modified_at=modified_at, + metadata=task_metadata, + ) + ) + queued += 1 + else: + doc_key = (user_id, doc_id) + if doc_key in _potentially_deleted: + logger.debug( + "Mail message %s reappeared, removing from deletion " + "grace period", + doc_id, + ) + del _potentially_deleted[doc_key] + + existing_metadata = await query_document_metadata( + doc_id=doc_id, doc_type="mail_message", user_id=user_id + ) + + needs_indexing = False + if existing_metadata is None: + needs_indexing = True + elif existing_metadata.get("modified_at", 0) < modified_at: + needs_indexing = True + elif existing_metadata.get("is_placeholder", False): + queued_at = existing_metadata.get("queued_at", 0) + placeholder_age = time.time() - queued_at + stale_threshold = settings.vector_sync_scan_interval * 5 + if placeholder_age > stale_threshold: + logger.debug( + "Found stale placeholder for mail message %s " + "(age=%ss), requeuing", + doc_id, + format(placeholder_age, ".1f"), + ) + needs_indexing = True + + if needs_indexing: + await write_placeholder_point( + doc_id=doc_id, + doc_type="mail_message", + user_id=user_id, + modified_at=modified_at, + ) + await send_stream.send( + DocumentTask( + user_id=user_id, + doc_id=doc_id, + doc_type="mail_message", + operation="index", + modified_at=modified_at, + metadata=task_metadata, + ) + ) + queued += 1 + + logger.info( + "[SCAN-%s] Found %s mail messages for %s", + scan_id, + message_count, + user_id, + ) + record_vector_sync_scan(message_count) + + # Check for deleted / aged-out messages (not initial sync) + if not initial_sync: + grace_period = settings.vector_sync_scan_interval * 1.5 + current_time = time.time() + + for doc_id in indexed_message_ids: + if doc_id not in nextcloud_message_ids: + doc_key = (user_id, doc_id) + + if doc_key in _potentially_deleted: + first_missing_time = _potentially_deleted[doc_key] + time_missing = current_time - first_missing_time + + if time_missing >= grace_period: + logger.info( + "Mail message %s missing for %ss (>%ss grace period), " + "sending deletion", + doc_id, + format(time_missing, ".1f"), + format(grace_period, ".1f"), + ) + await send_stream.send( + DocumentTask( + user_id=user_id, + doc_id=doc_id, + doc_type="mail_message", + operation="delete", + modified_at=0, + ) + ) + queued += 1 + del _potentially_deleted[doc_key] + else: + logger.debug( + "Mail message %s missing for first time, starting grace period", + doc_id, + ) + _potentially_deleted[doc_key] = current_time + + return queued + + async def scan_deck_cards( user_id: str, send_stream: TaskProducer, diff --git a/tests/client/mail/__init__.py b/tests/client/mail/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/client/mail/test_mail_api.py b/tests/client/mail/test_mail_api.py new file mode 100644 index 00000000..dec7c929 --- /dev/null +++ b/tests/client/mail/test_mail_api.py @@ -0,0 +1,206 @@ +"""Unit tests for MailClient API methods.""" + +import logging +from typing import Any + +import httpx +import pytest + +from nextcloud_mcp_server.client.mail import MailClient +from tests.client.conftest import create_mock_response + +logger = logging.getLogger(__name__) + +# Mark all tests in this module as unit tests +pytestmark = pytest.mark.unit + + +def _ocs_response(data: Any, status_code: int = 200) -> httpx.Response: + """Wrap a payload in the standard OCS envelope.""" + return create_mock_response( + status_code=status_code, + json_data={ + "ocs": { + "meta": {"status": "ok", "statuscode": status_code, "message": "OK"}, + "data": data, + } + }, + ) + + +async def test_list_accounts_unwraps_ocs_envelope(mocker): + """list_accounts returns the ocs.data payload.""" + mock_response = _ocs_response( + [ + {"id": 1, "email": "alice@example.com", "isDelegated": False}, + {"id": 2, "email": "bob@example.com", "isDelegated": False}, + ] + ) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + accounts = await client.list_accounts() + + assert len(accounts) == 2 + assert accounts[0]["id"] == 1 + assert accounts[0]["email"] == "alice@example.com" + + # Correct URL, OCS header, and format=json param. + args, kwargs = mock_make_request.call_args + assert args == ("GET", "/ocs/v2.php/apps/mail/api/account/list") + assert kwargs["headers"]["OCS-APIRequest"] == "true" + assert kwargs["params"]["format"] == "json" + + +async def test_get_mailboxes_passes_account_id(mocker): + """get_mailboxes sends accountId and unwraps the list.""" + mock_response = _ocs_response( + [ + { + "databaseId": 10, + "id": "SU5CT1g=", + "name": "INBOX", + "displayName": "INBOX", + "accountId": 1, + "specialUse": ["inbox"], + "unread": 3, + } + ] + ) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + mailboxes = await client.get_mailboxes(account_id=1) + + assert len(mailboxes) == 1 + assert mailboxes[0]["databaseId"] == 10 + assert mailboxes[0]["specialUse"] == ["inbox"] + + args, kwargs = mock_make_request.call_args + assert args == ("GET", "/ocs/v2.php/apps/mail/api/mailboxes") + assert kwargs["params"]["accountId"] == 1 + + +async def test_list_messages_builds_params(mocker): + """list_messages forwards limit/cursor/filter/view query params.""" + mock_response = _ocs_response( + [ + { + "databaseId": 100, + "subject": "Hello", + "dateInt": 1700000000, + "from": [{"label": "Alice", "email": "alice@example.com"}], + "to": [{"label": "Bob", "email": "bob@example.com"}], + "mailboxId": 10, + } + ] + ) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + messages = await client.list_messages( + 10, cursor=42, filter="hello", limit=50, view="threaded" + ) + + assert len(messages) == 1 + assert messages[0]["databaseId"] == 100 + + args, kwargs = mock_make_request.call_args + assert args == ("GET", "/ocs/v2.php/apps/mail/api/mailboxes/10/messages") + assert kwargs["params"]["limit"] == 50 + assert kwargs["params"]["cursor"] == 42 + assert kwargs["params"]["filter"] == "hello" + assert kwargs["params"]["view"] == "threaded" + + +async def test_list_messages_omits_optional_params(mocker): + """Optional params are omitted when not supplied; limit always present.""" + mock_response = _ocs_response([]) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + await client.list_messages(10) + + _, kwargs = mock_make_request.call_args + params = kwargs["params"] + assert params["limit"] == 20 # default + assert "cursor" not in params + assert "filter" not in params + assert "view" not in params + + +async def test_get_message_unwraps_full_message(mocker): + """get_message returns the full message dict.""" + mock_response = _ocs_response( + { + "id": 100, + "subject": "Hello", + "hasHtmlBody": True, + "body": "

Hi there

", + "from": [{"label": "Alice", "email": "alice@example.com"}], + "attachments": [ + { + "id": "1.2", + "fileName": "doc.pdf", + "mime": "application/pdf", + "size": 1024, + } + ], + } + ) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + message = await client.get_message(100) + + assert message["id"] == 100 + assert message["hasHtmlBody"] is True + assert message["attachments"][0]["fileName"] == "doc.pdf" + + args, _ = mock_make_request.call_args + assert args == ("GET", "/ocs/v2.php/apps/mail/api/message/100") + + +async def test_get_attachment_unwraps_json(mocker): + """get_attachment returns the JSON attachment object (not a binary download).""" + mock_response = _ocs_response( + {"name": "doc.pdf", "mime": "application/pdf", "size": 1024, "content": "abc"} + ) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + attachment = await client.get_attachment(100, "1.2") + + assert attachment["name"] == "doc.pdf" + assert attachment["content"] == "abc" + + args, _ = mock_make_request.call_args + assert args == ("GET", "/ocs/v2.php/apps/mail/api/message/100/attachment/1.2") + + +async def test_empty_data_returns_empty_list(mocker): + """A null ocs.data payload degrades to an empty list for list endpoints.""" + mock_response = _ocs_response(None) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mocker.patch.object(MailClient, "_make_request", return_value=mock_response) + + client = MailClient(mock_client, "testuser") + assert await client.list_accounts() == [] diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index bc79925f..f14f5999 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -12,6 +12,7 @@ from nextcloud_mcp_server.search.algorithms import SearchResult from nextcloud_mcp_server.search.verification import ( _verify_deck_cards, _verify_files, + _verify_mail_messages, _verify_news_items, _verify_notes, get_supported_doc_types, @@ -222,6 +223,97 @@ async def test_verify_notes_string_doc_id_matches_production(mocker): notes_client.get_note.assert_awaited_once_with(42) +# --------------------------------------------------------------------------- +# Mail verifier (per-id, mirrors the note verifier) +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_verify_mail_200_keeps_all(mocker): + mail_client = SimpleNamespace(get_message=mocker.AsyncMock(return_value={"id": 42})) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, [_make_result(42, doc_type="mail_message")], _sem() + ) + assert result == {"42"} + mail_client.get_message.assert_awaited_once_with(42) + + +@pytest.mark.unit +async def test_verify_mail_404_drops(mocker): + mail_client = SimpleNamespace( + get_message=mocker.AsyncMock(side_effect=_http_error(404)) + ) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, [_make_result(42, doc_type="mail_message")], _sem() + ) + assert result == set() + + +@pytest.mark.unit +async def test_verify_mail_403_drops(mocker): + mail_client = SimpleNamespace( + get_message=mocker.AsyncMock(side_effect=_http_error(403)) + ) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, [_make_result(42, doc_type="mail_message")], _sem() + ) + assert result == set() + + +@pytest.mark.unit +async def test_verify_mail_transient_5xx_keeps(mocker): + mail_client = SimpleNamespace( + get_message=mocker.AsyncMock(side_effect=_http_error(503)) + ) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, [_make_result(42, doc_type="mail_message")], _sem() + ) + assert result == {"42"} + + +@pytest.mark.unit +async def test_verify_mail_non_numeric_id_keeps(mocker): + mail_client = SimpleNamespace(get_message=mocker.AsyncMock()) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, [_make_result("not-a-number", doc_type="mail_message")], _sem() + ) + assert result == {"not-a-number"} + # Malformed id is kept without any network call. + mail_client.get_message.assert_not_awaited() + + +@pytest.mark.unit +async def test_verify_mail_mixed_outcomes(mocker): + async def fake_get(message_id: int): + if message_id == 20: + raise _http_error(404) # deleted + return {"id": message_id} + + mail_client = SimpleNamespace(get_message=mocker.AsyncMock(side_effect=fake_get)) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, + [ + _make_result(10, doc_type="mail_message"), + _make_result(20, doc_type="mail_message"), + _make_result(30, doc_type="mail_message"), + ], + _sem(), + ) + assert result == {"10", "30"} + + # --------------------------------------------------------------------------- # News batch verifier # --------------------------------------------------------------------------- diff --git a/tests/unit/test_mail_models.py b/tests/unit/test_mail_models.py new file mode 100644 index 00000000..8ff6a3e3 --- /dev/null +++ b/tests/unit/test_mail_models.py @@ -0,0 +1,109 @@ +"""Unit tests for Mail Pydantic models (alias mapping from the OCS API).""" + +import pytest + +from nextcloud_mcp_server.models.mail import ( + GetMessageResponse, + ListAccountsResponse, + MailAccount, + MailMailbox, + MailMessage, + MailMessageSummary, +) + +pytestmark = pytest.mark.unit + + +def test_account_maps_is_delegated_alias(): + account = MailAccount(**{"id": 1, "email": "a@example.com", "isDelegated": True}) + assert account.id == 1 + assert account.is_delegated is True + + +def test_mailbox_maps_camelcase_aliases(): + mailbox = MailMailbox( + **{ + "databaseId": 10, + "id": "SU5CT1g=", + "name": "INBOX", + "displayName": "Inbox", + "accountId": 1, + "specialUse": ["inbox"], + "unread": 5, + } + ) + assert mailbox.database_id == 10 + assert mailbox.account_id == 1 + assert mailbox.display_name == "Inbox" + assert mailbox.special_use == ["inbox"] + assert mailbox.unread == 5 + + +def test_message_summary_maps_from_and_dateint(): + summary = MailMessageSummary( + **{ + "databaseId": 100, + "subject": "Hello", + "dateInt": 1700000000, + "from": [{"label": "Alice", "email": "alice@example.com"}], + "to": [{"email": "bob@example.com"}], + "mailboxId": 10, + "previewText": "snippet", + "flags": {"seen": True, "hasAttachments": True}, + } + ) + assert summary.database_id == 100 + assert summary.date_int == 1700000000 + assert summary.from_[0].label == "Alice" + assert summary.to[0].email == "bob@example.com" + assert summary.mailbox_id == 10 + assert summary.preview_text == "snippet" + assert summary.flags is not None + assert summary.flags.seen is True + assert summary.flags.has_attachments is True + + +def test_full_message_maps_body_and_attachments(): + message = MailMessage( + **{ + "id": 100, + "subject": "Hello", + "hasHtmlBody": True, + "body": "

Hi

", + "from": [{"label": "Alice", "email": "alice@example.com"}], + "attachments": [ + { + "id": "1.2", + "fileName": "doc.pdf", + "mime": "application/pdf", + "size": 1024, + } + ], + } + ) + assert message.id == 100 + assert message.has_html_body is True + assert message.body == "

Hi

" + assert message.attachments[0].file_name == "doc.pdf" + assert message.attachments[0].id == "1.2" + + +def test_message_tolerates_missing_optional_fields(): + """A 206 partial response may omit the body.""" + message = MailMessage(**{"id": 100}) + assert message.id == 100 + assert message.body is None + assert message.has_html_body is False + assert message.attachments == [] + + +def test_response_models_wrap_results(): + resp = ListAccountsResponse( + results=[MailAccount(id=1, email="a@example.com")], total_count=1 + ) + assert resp.success is True + assert resp.total_count == 1 + assert resp.results[0].email == "a@example.com" + + msg_resp = GetMessageResponse(message=MailMessage(id=5, subject="Hi")) + assert msg_resp.message.id == 5 diff --git a/tests/unit/vector/test_scanner_consent_backstop.py b/tests/unit/vector/test_scanner_consent_backstop.py index e87ebed9..5205e3d2 100644 --- a/tests/unit/vector/test_scanner_consent_backstop.py +++ b/tests/unit/vector/test_scanner_consent_backstop.py @@ -104,7 +104,9 @@ async def test_noop_when_allowed_is_none(monkeypatch): async def test_noop_when_all_text_types_allowed(monkeypatch): send = AsyncMock() - allowed = frozenset({"note", "news_item", "deck_card", "file"}) + # Derive from INDEXED_DOC_TYPES so a newly-indexed text type doesn't make + # this "all allowed" set silently incomplete (and trip the backstop). + allowed = frozenset(scanner_module.INDEXED_DOC_TYPES) queued = await _enqueue_deletes_for_disabled_types( "alice", _producer(send), allowed, 1 ) From 62ee3e9f3211a6aa79c72804268fa3424c263f0b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 12:30:51 +0200 Subject: [PATCH 2/8] 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) --- nextcloud_mcp_server/client/mail.py | 42 +++++-- nextcloud_mcp_server/models/mail.py | 24 ++-- nextcloud_mcp_server/search/context.py | 37 +----- nextcloud_mcp_server/server/mail.py | 16 ++- nextcloud_mcp_server/vector/mail_content.py | 56 +++++++++ nextcloud_mcp_server/vector/processor.py | 60 ++++------ tests/client/mail/test_mail_api.py | 39 ++++++- tests/unit/vector/test_scanner_mail.py | 123 ++++++++++++++++++++ 8 files changed, 303 insertions(+), 94 deletions(-) create mode 100644 nextcloud_mcp_server/vector/mail_content.py create mode 100644 tests/unit/vector/test_scanner_mail.py diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index 75e8a331..01c2dd01 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/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": }} - 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": }}. + # 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) diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py index b084cbdf..0e4eb439 100644 --- a/nextcloud_mcp_server/models/mail.py +++ b/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") diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 8c484044..e57c6602 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/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 diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index cdda67ca..8fc98f95 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/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: diff --git a/nextcloud_mcp_server/vector/mail_content.py b/nextcloud_mcp_server/vector/mail_content.py new file mode 100644 index 00000000..1ee49fc1 --- /dev/null +++ b/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): + + From: + To: + + + + 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) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index c63462b0..1a1a1603 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/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 diff --git a/tests/client/mail/test_mail_api.py b/tests/client/mail/test_mail_api.py index dec7c929..4ae1068e 100644 --- a/tests/client/mail/test_mail_api.py +++ b/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"not found" + ) + 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() diff --git a/tests/unit/vector/test_scanner_mail.py b/tests/unit/vector/test_scanner_mail.py new file mode 100644 index 00000000..bf38ac3e --- /dev/null +++ b/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 == [] From c62ccf3d0da07f40f557cf67d81c185d6115e9ef Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 12:45:47 +0200 Subject: [PATCH 3/8] fix(mail): address PR #935 round-2 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- nextcloud_mcp_server/client/mail.py | 8 +- nextcloud_mcp_server/server/mail.py | 22 ++++- nextcloud_mcp_server/vector/scanner.py | 17 +++- tests/unit/vector/test_scanner_mail.py | 108 +++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) 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 From 891d07db12419ca68c302f0d6f17107028d5b233 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:01:21 +0200 Subject: [PATCH 4/8] perf(mail): batch verify-on-read; test build_mail_content; addr-recall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #935 round-3 review: - search/verification.py: rewrite _verify_mail_messages to batch by mailbox. get_message triggers a server-side IMAP body fetch, so per-result verify issued one IMAP FETCH per hit; now it calls the DB-cached list_messages once per mailbox (mailbox_id comes from the Qdrant payload via result.metadata) and intersects — O(unique mailboxes) light calls instead of O(results) IMAP. - vector/mail_content.py: include Cc/Bcc in the indexed text so recipient queries match; move MAIL_SCAN_MAX_PER_MAILBOX here (shared by scanner index window + verifier presence window) with a note that it equals the Mail OCS per-request max (100), so it's a fixed constant not a config knob. - client/mail.py: clamp list_messages limit to 1..100 at the client layer. - tests: add test_mail_content.py (exact-layout contract for build_mail_content); rewrite the mail verifier tests for the batch-per-mailbox shape. Left as-is: ValidationError isn't caught in the list-endpoint tools — consistent with nc_notes_*/nc_deck_* and not a regression. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/mail.py | 5 +- nextcloud_mcp_server/search/verification.py | 102 +++++++++++++++----- nextcloud_mcp_server/vector/mail_content.py | 20 +++- nextcloud_mcp_server/vector/scanner.py | 15 +-- tests/unit/search/test_verification.py | 92 +++++++++++------- tests/unit/vector/test_mail_content.py | 79 +++++++++++++++ 6 files changed, 244 insertions(+), 69 deletions(-) create mode 100644 tests/unit/vector/test_mail_content.py diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index 0ba736fc..a17e7493 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/nextcloud_mcp_server/client/mail.py @@ -142,7 +142,10 @@ class MailClient(BaseNextcloudClient): List of message summary objects (keys include databaseId, subject, from, to, dateInt, flags, previewText, mailboxId). """ - params: dict[str, Any] = {"limit": limit} + # The Mail OCS API clamps limit to 1..100 server-side; do it here too so + # the contract is enforced at the client layer with a predictable value + # (a limit of 0 would otherwise collapse to 1 server-side). + params: dict[str, Any] = {"limit": min(max(1, limit), 100)} if cursor is not None: params["cursor"] = cursor if search_filter is not None: diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 9d319021..79c1f5e3 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -53,6 +53,7 @@ from nextcloud_mcp_server.search.algorithms import ( ) from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.eviction import delete_document_points +from nextcloud_mcp_server.vector.mail_content import MAIL_SCAN_MAX_PER_MAILBOX logger = logging.getLogger(__name__) @@ -493,53 +494,106 @@ async def _verify_mail_messages( results: list[SearchResult], semaphore: anyio.Semaphore, ) -> set[str]: - """Verify mail messages per-id via the Mail OCS ``get_message`` endpoint. + """Verify mail messages with one DB-cached list per mailbox, then intersect. - Mirrors ``_verify_notes``: a definitive 403/404 (message deleted, account - removed, or Mail app disabled) drops the result and schedules eviction; - transient errors and non-numeric ids fail open (keep the result). + ``mail.get_message`` triggers a server-side IMAP body fetch, so a per-result + verify would issue one IMAP FETCH per hit — multiple seconds for an active + inbox. Instead we batch by ``mailbox_id`` (propagated into ``result.metadata`` + from the Qdrant payload) and call ``mail.list_messages`` once per mailbox, + which reads the Mail app's DB cache (no IMAP). A message is accessible iff it + is present in its mailbox's newest-N listing — the same window the scanner + indexes, so anything aged out of the window is being evicted anyway. + + Failure policy mirrors ``_verify_news_items``: a definitive 403/404 for a + mailbox drops all its results (eviction reclaims); transient errors keep + them (fail-open). Results with no/!numeric ``mailbox_id`` or a non-numeric + ``doc_id`` are kept (fail-open; verification can't batch them). """ # safe: cooperative concurrency, no lock needed (see verify_search_results) accessible: set[str] = set() - async def check(result: SearchResult) -> None: - doc_id = result.id - try: - message_id_int = int(doc_id) - except (TypeError, ValueError) as e: + # Partition results by mailbox so each mailbox is listed exactly once. + by_mailbox: dict[int, list[SearchResult]] = {} + for r in results: + mailbox_id = (r.metadata or {}).get("mailbox_id") + # No usable mailbox_id (legacy payload) — can't batch via the DB cache + # without an IMAP-triggering per-id fetch, so keep it (fail-open). + if mailbox_id is None: logger.warning( - "Non-numeric mail message id %r: %s; keeping result", - doc_id, - e, + "Mail result %s has no mailbox_id; keeping (batch verification " + "skipped)", + r.id, ) - accessible.add(doc_id) - return + accessible.add(r.id) + continue + try: + mailbox_int = int(mailbox_id) + except (TypeError, ValueError): + logger.warning( + "Mail result %s has non-numeric mailbox_id %r; keeping " + "(batch verification skipped)", + r.id, + mailbox_id, + ) + accessible.add(r.id) + continue + by_mailbox.setdefault(mailbox_int, []).append(r) + async def check_mailbox(mailbox_id: int, mb_results: list[SearchResult]) -> None: async with semaphore: try: - await client.mail.get_message(message_id_int) - accessible.add(doc_id) + messages = await client.mail.list_messages( + mailbox_id, limit=MAIL_SCAN_MAX_PER_MAILBOX + ) except HTTPStatusError as e: if _is_definitive_404_or_403(e): + # Mailbox/account gone — all its results are inaccessible. return logger.warning( - "Transient error verifying mail message %s: %s %s; keeping result", - doc_id, + "Transient error listing mailbox %s for verification: %s %s; " + "keeping its %d result(s)", + mailbox_id, e.response.status_code, e, + len(mb_results), ) - accessible.add(doc_id) + for r in mb_results: + accessible.add(r.id) + return except Exception as e: logger.warning( - "Unexpected error verifying mail message %s: %s; keeping result", - doc_id, + "Unexpected error listing mailbox %s for verification: %s; " + "keeping its %d result(s)", + mailbox_id, e, + len(mb_results), ) - accessible.add(doc_id) + for r in mb_results: + accessible.add(r.id) + return + + present_ids = { + str(m.get("databaseId")) + for m in messages + if m.get("databaseId") is not None + } + for r in mb_results: + if r.id in present_ids: + accessible.add(r.id) + elif not is_valid_nextcloud_doc_id(r.id): + # Malformed stored id can't match the numeric listing; keep it + # (fail-open) rather than drop a possibly-legitimate result — + # mirrors the notes/news posture. + logger.warning( + "Malformed mail_message doc_id %r in verifier; keeping", + r.id, + ) + accessible.add(r.id) + # else: genuinely absent (deleted or aged out) -> drop + evict. async with anyio.create_task_group() as tg: - for r in results: - tg.start_soon(check, r) + for mailbox_id, mb_results in by_mailbox.items(): + tg.start_soon(check_mailbox, mailbox_id, mb_results) return accessible diff --git a/nextcloud_mcp_server/vector/mail_content.py b/nextcloud_mcp_server/vector/mail_content.py index 1ee49fc1..61f97f79 100644 --- a/nextcloud_mcp_server/vector/mail_content.py +++ b/nextcloud_mcp_server/vector/mail_content.py @@ -10,6 +10,13 @@ from typing import Any from nextcloud_mcp_server.vector.html_processor import html_to_markdown +# Newest-N messages indexed (and verified) per mailbox. This equals the Mail +# OCS API's per-request maximum (it clamps ``limit`` to 1..100), so it cannot be +# raised without adding cursor pagination — hence a documented constant rather +# than a config knob that would silently cap at 100. Shared by the scanner +# (index window) and the verifier (presence window) so they stay consistent. +MAIL_SCAN_MAX_PER_MAILBOX = 100 + def format_mail_addresses(addrs: list[dict[str, Any]] | None) -> str: """Render a list of {label, email} address objects as a display string.""" @@ -33,16 +40,21 @@ def build_mail_content(message: dict[str, Any]) -> str: From: To: + Cc: # only when non-empty + Bcc: # only when non-empty - The body is the Mail OCS ``body`` field — sanitized HTML when - ``hasHtmlBody`` is set (converted to Markdown for embedding), otherwise + Cc/Bcc are included so recipient-oriented queries ("emails where alice was + cc'd") can match. 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")) + cc_str = format_mail_addresses(message.get("cc")) + bcc_str = format_mail_addresses(message.get("bcc")) raw_body = message.get("body") or "" body_text = html_to_markdown(raw_body) if message.get("hasHtmlBody") else raw_body @@ -51,6 +63,10 @@ def build_mail_content(message: dict[str, Any]) -> str: content_parts.append(f"From: {from_str}") if to_str: content_parts.append(f"To: {to_str}") + if cc_str: + content_parts.append(f"Cc: {cc_str}") + if bcc_str: + content_parts.append(f"Bcc: {bcc_str}") content_parts.append("") # Blank line content_parts.append(body_text) return "\n".join(content_parts) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 78b788ee..16023504 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -28,6 +28,7 @@ from nextcloud_mcp_server.server.tag_exclusion import ( is_path_excluded, ) from nextcloud_mcp_server.vector.dead_letter import is_dead_lettered +from nextcloud_mcp_server.vector.mail_content import MAIL_SCAN_MAX_PER_MAILBOX from nextcloud_mcp_server.vector.placeholder import ( query_document_metadata, write_placeholder_point, @@ -1355,13 +1356,13 @@ async def scan_news_items( return queued -# Newest-N messages indexed per mailbox. The Mail app paginates by recency, so -# this bounds the index to recent mail; older messages age out of the index as -# newer ones arrive (and the deletion-tracking pass below evicts them, the same -# way it handles actually-deleted messages). Raise this if deeper history is -# wanted, at the cost of more embedding work. -MAIL_SCAN_MAX_PER_MAILBOX = 100 - +# Newest-N messages indexed per mailbox (= the Mail OCS per-request maximum; +# imported from mail_content so the scanner index window and the search-time +# verifier presence window stay identical). Older messages age out of the index +# as newer ones arrive — the deletion-tracking pass below evicts them, the same +# way it handles actually-deleted messages. Going beyond 100 would require +# cursor pagination, so the value is fixed rather than configurable. +# # 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 diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index f14f5999..9edbea24 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -18,6 +18,7 @@ from nextcloud_mcp_server.search.verification import ( get_supported_doc_types, verify_search_results, ) +from nextcloud_mcp_server.vector.mail_content import MAIL_SCAN_MAX_PER_MAILBOX from nextcloud_mcp_server.vector.scanner import INDEXED_DOC_TYPES # --------------------------------------------------------------------------- @@ -228,90 +229,111 @@ async def test_verify_notes_string_doc_id_matches_production(mocker): # --------------------------------------------------------------------------- +def _mail_result(doc_id, mailbox_id=10): + """A mail_message SearchResult carrying mailbox_id in its metadata.""" + return _make_result( + doc_id, doc_type="mail_message", metadata={"mailbox_id": mailbox_id} + ) + + @pytest.mark.unit -async def test_verify_mail_200_keeps_all(mocker): - mail_client = SimpleNamespace(get_message=mocker.AsyncMock(return_value={"id": 42})) +async def test_verify_mail_batches_one_list_per_mailbox(mocker): + """The verifier lists each mailbox once (DB cache, not per-message IMAP).""" + list_messages = mocker.AsyncMock( + return_value=[{"databaseId": 10}, {"databaseId": 30}] + ) + mail_client = SimpleNamespace(list_messages=list_messages) client = SimpleNamespace(mail=mail_client, username="alice") result = await _verify_mail_messages( - client, [_make_result(42, doc_type="mail_message")], _sem() + client, + [_mail_result(10), _mail_result(20), _mail_result(30)], # all mailbox 10 + _sem(), ) - assert result == {"42"} - mail_client.get_message.assert_awaited_once_with(42) + # 10 and 30 present; 20 absent (deleted/aged out) -> dropped. + assert result == {"10", "30"} + # One DB-cached list call for the single mailbox, not one per result. + list_messages.assert_awaited_once_with(10, limit=MAIL_SCAN_MAX_PER_MAILBOX) @pytest.mark.unit -async def test_verify_mail_404_drops(mocker): +async def test_verify_mail_404_drops_mailbox(mocker): mail_client = SimpleNamespace( - get_message=mocker.AsyncMock(side_effect=_http_error(404)) + list_messages=mocker.AsyncMock(side_effect=_http_error(404)) ) client = SimpleNamespace(mail=mail_client, username="alice") - result = await _verify_mail_messages( - client, [_make_result(42, doc_type="mail_message")], _sem() - ) + result = await _verify_mail_messages(client, [_mail_result(42)], _sem()) assert result == set() @pytest.mark.unit -async def test_verify_mail_403_drops(mocker): +async def test_verify_mail_403_drops_mailbox(mocker): mail_client = SimpleNamespace( - get_message=mocker.AsyncMock(side_effect=_http_error(403)) + list_messages=mocker.AsyncMock(side_effect=_http_error(403)) ) client = SimpleNamespace(mail=mail_client, username="alice") - result = await _verify_mail_messages( - client, [_make_result(42, doc_type="mail_message")], _sem() - ) + result = await _verify_mail_messages(client, [_mail_result(42)], _sem()) assert result == set() @pytest.mark.unit async def test_verify_mail_transient_5xx_keeps(mocker): mail_client = SimpleNamespace( - get_message=mocker.AsyncMock(side_effect=_http_error(503)) + list_messages=mocker.AsyncMock(side_effect=_http_error(503)) ) client = SimpleNamespace(mail=mail_client, username="alice") + result = await _verify_mail_messages(client, [_mail_result(42)], _sem()) + assert result == {"42"} + + +@pytest.mark.unit +async def test_verify_mail_missing_mailbox_id_keeps(mocker): + """A result without a usable mailbox_id is kept without any network call.""" + list_messages = mocker.AsyncMock() + mail_client = SimpleNamespace(list_messages=list_messages) + client = SimpleNamespace(mail=mail_client, username="alice") + result = await _verify_mail_messages( client, [_make_result(42, doc_type="mail_message")], _sem() ) assert result == {"42"} + list_messages.assert_not_awaited() @pytest.mark.unit -async def test_verify_mail_non_numeric_id_keeps(mocker): - mail_client = SimpleNamespace(get_message=mocker.AsyncMock()) +async def test_verify_mail_non_numeric_id_kept_when_mailbox_listed(mocker): + """A malformed doc_id can't match the numeric listing, so it's kept.""" + mail_client = SimpleNamespace( + list_messages=mocker.AsyncMock(return_value=[{"databaseId": 99}]) + ) client = SimpleNamespace(mail=mail_client, username="alice") - result = await _verify_mail_messages( - client, [_make_result("not-a-number", doc_type="mail_message")], _sem() - ) + result = await _verify_mail_messages(client, [_mail_result("not-a-number")], _sem()) assert result == {"not-a-number"} - # Malformed id is kept without any network call. - mail_client.get_message.assert_not_awaited() @pytest.mark.unit -async def test_verify_mail_mixed_outcomes(mocker): - async def fake_get(message_id: int): - if message_id == 20: - raise _http_error(404) # deleted - return {"id": message_id} +async def test_verify_mail_partitions_distinct_mailboxes(mocker): + """Results in different mailboxes each get their own list call.""" - mail_client = SimpleNamespace(get_message=mocker.AsyncMock(side_effect=fake_get)) + async def list_messages(mailbox_id, *, limit): + return {10: [{"databaseId": 1}], 20: [{"databaseId": 2}]}[mailbox_id] + + mail_client = SimpleNamespace( + list_messages=mocker.AsyncMock(side_effect=list_messages) + ) client = SimpleNamespace(mail=mail_client, username="alice") result = await _verify_mail_messages( client, - [ - _make_result(10, doc_type="mail_message"), - _make_result(20, doc_type="mail_message"), - _make_result(30, doc_type="mail_message"), - ], + [_mail_result(1, mailbox_id=10), _mail_result(2, mailbox_id=20)], _sem(), ) - assert result == {"10", "30"} + assert result == {"1", "2"} + assert mail_client.list_messages.await_count == 2 # --------------------------------------------------------------------------- diff --git a/tests/unit/vector/test_mail_content.py b/tests/unit/vector/test_mail_content.py new file mode 100644 index 00000000..bb07dedb --- /dev/null +++ b/tests/unit/vector/test_mail_content.py @@ -0,0 +1,79 @@ +"""Unit tests for the shared mail content reconstruction. + +``build_mail_content`` is the single source of truth for index-time and +query-time chunk offsets; these tests pin the exact layout so a change to the +separators or header order can't silently misalign every indexed message. +""" + +import pytest + +from nextcloud_mcp_server.vector.mail_content import ( + build_mail_content, + format_mail_addresses, +) + +pytestmark = pytest.mark.unit + + +def test_format_addresses_variants(): + assert ( + format_mail_addresses([{"label": "Alice", "email": "alice@example.com"}]) + == "Alice " + ) + # Email only, label only, label==email, and multiple joined by ", ". + assert format_mail_addresses([{"email": "bob@example.com"}]) == "bob@example.com" + assert format_mail_addresses([{"label": "Ops"}]) == "Ops" + assert format_mail_addresses([{"label": "x@y.z", "email": "x@y.z"}]) == "x@y.z" + assert format_mail_addresses(None) == "" + assert ( + format_mail_addresses([{"email": "a@x.io"}, {"label": "B", "email": "b@x.io"}]) + == "a@x.io, B " + ) + + +def test_build_mail_content_plain_text_layout(): + message = { + "subject": "Hello", + "from": [{"label": "Alice", "email": "alice@example.com"}], + "to": [{"email": "bob@example.com"}], + "hasHtmlBody": False, + "body": "Hi there.", + } + assert build_mail_content(message) == ( + "Hello\nFrom: Alice \nTo: bob@example.com\n\nHi there." + ) + + +def test_build_mail_content_includes_cc_and_bcc_when_present(): + message = { + "subject": "Sync", + "from": [{"email": "a@x.io"}], + "to": [{"email": "b@x.io"}], + "cc": [{"email": "c@x.io"}], + "bcc": [{"email": "d@x.io"}], + "hasHtmlBody": False, + "body": "body", + } + assert build_mail_content(message) == ( + "Sync\nFrom: a@x.io\nTo: b@x.io\nCc: c@x.io\nBcc: d@x.io\n\nbody" + ) + + +def test_build_mail_content_converts_html_body(): + message = { + "subject": "HTML", + "from": [{"email": "a@x.io"}], + "hasHtmlBody": True, + "body": "

Hello world

", + } + result = build_mail_content(message) + # Header preserved; body converted to markdown (no raw tags). + assert result.startswith("HTML\nFrom: a@x.io\n\n") + assert "

" not in result + assert "world" in result + + +def test_build_mail_content_tolerates_empty_fields(): + # No subject/addresses/body (e.g. a 206 partial) -> just the blank-line + + # empty body, with no spurious header lines. + assert build_mail_content({}) == "\n\n" From 0856d5995697be7e2c5b49ad684d744ecb5fafbe Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:12:15 +0200 Subject: [PATCH 5/8] harden(mail): address PR #935 round-4 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No blockers raised; hardening + clarity: - client/mail.py: URL-encode the caller-supplied attachment_id (quote(..., safe="")) — defense-in-depth against path traversal. - server/mail.py: measure attachment content in UTF-8 bytes (not characters) for the size cap and the sentinel message. - scanner.py: bound _mail_cap_logged (insertion-ordered dict + oldest-first eviction at 50k, mirroring _consent_backstop_done) so the cap-log dedup set can't leak in a long-running multi-tenant process; reword the cap log to not imply MAIL_SCAN_MAX_PER_MAILBOX is operator-tunable (it's the Mail OCS max). - models/mail.py: comment why GetAttachmentResponse doesn't nest MailAttachment (different OCS endpoint shape). - mail_content.py: document format_mail_addresses' empty-entry skip contract. _potentially_deleted doc_type-in-key remains tracked as Deck #376 (pre-existing cross-cutting; reviewer confirmed deferral). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/mail.py | 8 ++++- nextcloud_mcp_server/models/mail.py | 8 ++++- nextcloud_mcp_server/server/mail.py | 12 ++++--- nextcloud_mcp_server/vector/mail_content.py | 8 ++++- nextcloud_mcp_server/vector/scanner.py | 35 +++++++++++++++------ 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index a17e7493..22334bf2 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/nextcloud_mcp_server/client/mail.py @@ -14,6 +14,7 @@ OCS API controllers (Mail 5.x / Nextcloud 32+). import logging from typing import Any +from urllib.parse import quote from httpx import HTTPStatusError, RequestError, Response @@ -188,5 +189,10 @@ class MailClient(BaseNextcloudClient): Returns: Attachment object with name, mime, size, content. """ - data = await self._ocs_get(f"/message/{message_id}/attachment/{attachment_id}") + # URL-encode the caller-supplied attachment id (defense-in-depth: keeps + # a value like "../.." from being normalised into a different path). + safe_attachment_id = quote(attachment_id, safe="") + data = await self._ocs_get( + f"/message/{message_id}/attachment/{safe_attachment_id}" + ) return data or {} diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py index 0e4eb439..b6d449c9 100644 --- a/nextcloud_mcp_server/models/mail.py +++ b/nextcloud_mcp_server/models/mail.py @@ -173,7 +173,13 @@ class GetMessageResponse(BaseResponse): class GetAttachmentResponse(BaseResponse): - """Response model for getting a single attachment.""" + """Response model for getting a single attachment. + + Intentionally does NOT nest ``MailAttachment``: the Mail OCS *get-attachment* + endpoint returns a different shape (``name``/``mime``/``size``/``content``) + than the attachment entries on a message listing, which ``MailAttachment`` + models (``id``/``fileName``/``cid``/``disposition``, and no ``content``). + """ name: str | None = Field(None, description="Attachment file name") mime: str | None = Field(None, description="MIME type") diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index 13e59bb2..8c029092 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/nextcloud_mcp_server/server/mail.py @@ -222,11 +222,13 @@ def configure_mail_tools(mcp: FastMCP): 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})]" - ) + if isinstance(content, str): + content_bytes = len(content.encode("utf-8")) + if content_bytes > MAX_ATTACHMENT_CONTENT_BYTES: + content = ( + f"[attachment too large to inline: {content_bytes} bytes " + f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]" + ) return GetAttachmentResponse( name=data.get("name"), mime=data.get("mime"), diff --git a/nextcloud_mcp_server/vector/mail_content.py b/nextcloud_mcp_server/vector/mail_content.py index 61f97f79..744ccf36 100644 --- a/nextcloud_mcp_server/vector/mail_content.py +++ b/nextcloud_mcp_server/vector/mail_content.py @@ -19,7 +19,13 @@ MAIL_SCAN_MAX_PER_MAILBOX = 100 def format_mail_addresses(addrs: list[dict[str, Any]] | None) -> str: - """Render a list of {label, email} address objects as a display string.""" + """Render a list of {label, email} address objects as a display string. + + ``None`` and address objects with neither ``label`` nor ``email`` are + skipped (yielding ``""`` for an all-empty list) — IMAP envelope addresses + effectively always carry at least an email, so this only drops malformed + entries rather than losing real recipients. + """ parts: list[str] = [] for addr in addrs or []: label = addr.get("label") diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 16023504..b4b0eea9 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -1366,8 +1366,26 @@ async def scan_news_items( # 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() +# multi-tenant logs). Insertion-ordered dict + bounded eviction (mirrors +# _consent_backstop_done) so a long-running multi-tenant process can't leak it. +_mail_cap_logged: dict[tuple[str, int], None] = {} +_MAIL_CAP_LOGGED_MAX = 50_000 + + +def _mark_mail_cap_logged(key: tuple[str, int]) -> bool: + """Record a one-shot cap-log marker; return True if this is the first time. + + Evicts oldest-first to half capacity on overflow (a re-log after eviction is + a harmless info line), so the dedup set stays bounded. + """ + if key in _mail_cap_logged: + return False + if len(_mail_cap_logged) >= _MAIL_CAP_LOGGED_MAX: + overage = len(_mail_cap_logged) - _MAIL_CAP_LOGGED_MAX // 2 + for stale_key in list(_mail_cap_logged)[:overage]: + del _mail_cap_logged[stale_key] + _mail_cap_logged[key] = None + return True async def scan_mail_messages( @@ -1467,18 +1485,17 @@ async def scan_mail_messages( ) continue - cap_key = (user_id, mailbox_id) - if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX and cap_key not in ( - _mail_cap_logged + if len(messages) >= MAIL_SCAN_MAX_PER_MAILBOX and _mark_mail_cap_logged( + (user_id, mailbox_id) ): - _mail_cap_logged.add(cap_key) logger.info( - "[SCAN-%s] Mailbox %s hit the newest-%s cap; older messages " - "are not indexed (set MAIL_SCAN_MAX_PER_MAILBOX higher for " - "deeper history)", + "[SCAN-%s] Mailbox %s contains more than %s messages; only " + "the newest %s are indexed (cursor pagination not yet " + "implemented)", scan_id, mailbox_id, MAIL_SCAN_MAX_PER_MAILBOX, + MAIL_SCAN_MAX_PER_MAILBOX, ) for message in messages: From d0061454446e607f836099f095cf301ba1cb424e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:24:24 +0200 Subject: [PATCH 6/8] harden(mail): address PR #935 round-5 review No blockers raised; residual cleanup: - processor.py: guard int(doc_task.doc_id) with is_valid_nextcloud_doc_id in the mail_message branch (consistent with search/context.py + the verifier). - mail metadata symmetry: store `bcc` in file_metadata and the Qdrant payload alongside cc (build_mail_content already emits a Bcc: line). - server/mail.py: extract _cap_attachment_content helper (byte-accurate cap) and unit-test it (small/None/oversized/multibyte). - client/mail.py: give the synthetic OCS-error Response an explicit empty body; add a test that a traversal-style attachment_id is percent-encoded. - models/mail.py: clarify ListMessagesResponse.total_count is the page count, not the mailbox total. Deferred (Deck #376): _potentially_deleted doc_type-in-key. It's pre-existing and spans ~30 sites across the notes/news/deck/file/mail scanners (whose deletion paths have no unit coverage), so it belongs in its own focused PR rather than expanding this mail PR's blast radius into other doc types. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/mail.py | 4 ++- nextcloud_mcp_server/models/mail.py | 6 +++- nextcloud_mcp_server/server/mail.py | 27 +++++++++++------ nextcloud_mcp_server/vector/processor.py | 8 +++++ tests/client/mail/test_mail_api.py | 19 ++++++++++++ tests/unit/test_mail_server_helpers.py | 38 ++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 tests/unit/test_mail_server_helpers.py diff --git a/nextcloud_mcp_server/client/mail.py b/nextcloud_mcp_server/client/mail.py index 22334bf2..fc006210 100644 --- a/nextcloud_mcp_server/client/mail.py +++ b/nextcloud_mcp_server/client/mail.py @@ -80,7 +80,9 @@ class MailClient(BaseNextcloudClient): except (TypeError, ValueError): status_code = 200 if status_code >= 400: - synthetic = Response(status_code=status_code, request=response.request) + synthetic = Response( + status_code=status_code, request=response.request, content=b"" + ) raise HTTPStatusError( f"Mail OCS error {status_code} for {path}: {meta.get('message')}", request=response.request, diff --git a/nextcloud_mcp_server/models/mail.py b/nextcloud_mcp_server/models/mail.py index b6d449c9..2887c9f7 100644 --- a/nextcloud_mcp_server/models/mail.py +++ b/nextcloud_mcp_server/models/mail.py @@ -162,7 +162,11 @@ class ListMessagesResponse(BaseResponse): """Response model for listing message envelopes.""" results: list[MailMessageSummary] = Field(description="List of message summaries") - total_count: int = Field(description="Number of messages returned") + total_count: int = Field( + description="Number of messages returned in this page (NOT the mailbox " + "total, which isn't known without a full scan); page with cursor and " + "stop on an empty result" + ) has_more: bool = Field(False, description="Whether more messages may exist") diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index 8c029092..b9ca5f9b 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/nextcloud_mcp_server/server/mail.py @@ -32,6 +32,23 @@ logger = logging.getLogger(__name__) MAX_ATTACHMENT_CONTENT_BYTES = 5 * 1024 * 1024 +def _cap_attachment_content(content: str | None) -> str | None: + """Replace oversized attachment content with a size sentinel. + + Measures UTF-8 byte length (what actually lands in the MCP response/LLM + context), not character count. Non-string content is returned unchanged. + """ + if not isinstance(content, str): + return content + content_bytes = len(content.encode("utf-8")) + if content_bytes > MAX_ATTACHMENT_CONTENT_BYTES: + return ( + f"[attachment too large to inline: {content_bytes} bytes " + f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]" + ) + return content + + def configure_mail_tools(mcp: FastMCP): """Configure Mail app MCP tools (read-only).""" @@ -221,19 +238,11 @@ 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): - content_bytes = len(content.encode("utf-8")) - if content_bytes > MAX_ATTACHMENT_CONTENT_BYTES: - content = ( - f"[attachment too large to inline: {content_bytes} bytes " - f"(> {MAX_ATTACHMENT_CONTENT_BYTES})]" - ) return GetAttachmentResponse( name=data.get("name"), mime=data.get("mime"), size=data.get("size"), - content=content, + content=_cap_attachment_content(data.get("content")), ) except RequestError as e: raise McpError( diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 1a1a1603..5b2ccf2c 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -42,6 +42,7 @@ from nextcloud_mcp_server.observability.metrics import ( from nextcloud_mcp_server.observability.tracing import trace_operation from nextcloud_mcp_server.search.pdf_highlighter import PDFHighlighter from nextcloud_mcp_server.usage import UsageEventStore +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector import payload_keys from nextcloud_mcp_server.vector._errors import format_exception_group from nextcloud_mcp_server.vector.dead_letter import ( @@ -848,6 +849,11 @@ async def _index_document( # Fetch the full message via the Mail OCS API. The Mail app handles # IMAP server-side; we only ever speak HTTP. build_mail_content is # shared with search/context.py so index- and query-time text match. + # Guard the cast before the network call (consistent with the same + # doc_type in search/context.py) so a malformed queue record produces + # a specific error rather than a bare ValueError. + if not is_valid_nextcloud_doc_id(doc_task.doc_id): + raise ValueError(f"Invalid mail_message doc_id: {doc_task.doc_id!r}") message = await nc_client.mail.get_message(int(doc_task.doc_id)) content = build_mail_content(message) @@ -861,6 +867,7 @@ async def _index_document( "from": format_mail_addresses(message.get("from")), "to": format_mail_addresses(message.get("to")), "cc": format_mail_addresses(message.get("cc")), + "bcc": format_mail_addresses(message.get("bcc")), "date_int": message.get("dateInt"), "has_attachments": bool(message.get("attachments")), "account_id": (doc_task.metadata or {}).get("account_id"), @@ -1640,6 +1647,7 @@ async def _index_document( "from": file_metadata.get("from"), "to": file_metadata.get("to"), "cc": file_metadata.get("cc"), + "bcc": file_metadata.get("bcc"), "date_int": file_metadata.get("date_int"), "has_attachments": file_metadata.get("has_attachments"), "account_id": file_metadata.get("account_id"), diff --git a/tests/client/mail/test_mail_api.py b/tests/client/mail/test_mail_api.py index 4ae1068e..07c9ee2a 100644 --- a/tests/client/mail/test_mail_api.py +++ b/tests/client/mail/test_mail_api.py @@ -196,6 +196,25 @@ async def test_get_attachment_unwraps_json(mocker): assert args == ("GET", "/ocs/v2.php/apps/mail/api/message/100/attachment/1.2") +async def test_get_attachment_url_encodes_attachment_id(mocker): + """A traversal-style attachment_id is percent-encoded in the URL path.""" + mock_response = _ocs_response({"name": "x", "content": "y"}) + mock_client = mocker.AsyncMock(spec=httpx.AsyncClient) + mock_make_request = mocker.patch.object( + MailClient, "_make_request", return_value=mock_response + ) + + client = MailClient(mock_client, "testuser") + await client.get_attachment(100, "../../evil") + + args, _ = mock_make_request.call_args + # The "/" and ".." are encoded, so they can't escape the attachment path. + assert args == ( + "GET", + "/ocs/v2.php/apps/mail/api/message/100/attachment/..%2F..%2Fevil", + ) + + async def test_empty_data_returns_empty_list(mocker): """A null ocs.data payload degrades to an empty list for list endpoints.""" mock_response = _ocs_response(None) diff --git a/tests/unit/test_mail_server_helpers.py b/tests/unit/test_mail_server_helpers.py new file mode 100644 index 00000000..7d3027d3 --- /dev/null +++ b/tests/unit/test_mail_server_helpers.py @@ -0,0 +1,38 @@ +"""Unit tests for server/mail.py helper logic.""" + +import pytest + +from nextcloud_mcp_server.server.mail import ( + MAX_ATTACHMENT_CONTENT_BYTES, + _cap_attachment_content, +) + +pytestmark = pytest.mark.unit + + +def test_small_content_passes_through(): + assert _cap_attachment_content("hello") == "hello" + + +def test_none_content_passes_through(): + assert _cap_attachment_content(None) is None + + +def test_oversized_content_replaced_with_byte_sentinel(): + oversized = "a" * (MAX_ATTACHMENT_CONTENT_BYTES + 1) + result = _cap_attachment_content(oversized) + assert result != oversized + assert "too large to inline" in result + # Reports the actual UTF-8 byte count, not character count. + assert f"{MAX_ATTACHMENT_CONTENT_BYTES + 1} bytes" in result + + +def test_multibyte_counted_in_bytes_not_chars(): + # Each "€" is 3 UTF-8 bytes; a string just under the byte cap in characters + # can still exceed it in bytes. + char_count = (MAX_ATTACHMENT_CONTENT_BYTES // 3) + 1 + content = "€" * char_count + # Under the cap by character count, over it by byte count -> capped. + assert len(content) <= MAX_ATTACHMENT_CONTENT_BYTES + assert len(content.encode("utf-8")) > MAX_ATTACHMENT_CONTENT_BYTES + assert "too large to inline" in _cap_attachment_content(content) From 10e768e4140576b0895120148d41528384acf61d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:35:49 +0200 Subject: [PATCH 7/8] fix(mail): guard empty message payload in processor (PR #935 round-6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - processor.py: raise on an empty mail_message payload (OCS data=null with a <400 meta) so the task dead-letters instead of indexing a near-empty placeholder — mirrors the nc_mail_get_message tool guard. (the round-6 approve-gating item) - server/mail.py: clamp limit once in nc_mail_list_messages and base has_more on the effective (post-clamp) limit, so a caller passing limit<=0 doesn't get a misleading count. - server/mail.py: note in nc_mail_get_message that attachments with id=null are inline body parts and can't be fetched via nc_mail_get_attachment. - tests: add the first-time-missing incremental scanner case (enters the grace period, nothing queued/deleted). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/server/mail.py | 13 +++++++++++-- nextcloud_mcp_server/vector/processor.py | 7 +++++++ tests/unit/vector/test_scanner_mail.py | 21 +++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/server/mail.py b/nextcloud_mcp_server/server/mail.py index b9ca5f9b..f0abdb3f 100644 --- a/nextcloud_mcp_server/server/mail.py +++ b/nextcloud_mcp_server/server/mail.py @@ -143,15 +143,22 @@ def configure_mail_tools(mcp: FastMCP): messages; page with ``cursor`` and stop on an empty result. """ client = await get_client(ctx) + # Clamp to the same window the client/OCS API enforce so the has_more + # heuristic compares against the limit actually applied (a caller passing + # limit<=0 otherwise gets a misleading count). + effective_limit = min(max(1, limit), 100) try: messages_data = await client.mail.list_messages( - mailbox_id, cursor=cursor, search_filter=search_filter, limit=limit + mailbox_id, + cursor=cursor, + search_filter=search_filter, + limit=effective_limit, ) messages = [MailMessageSummary(**m) for m in messages_data] return ListMessagesResponse( results=messages, total_count=len(messages), - has_more=len(messages) == limit and limit > 0, + has_more=len(messages) == effective_limit, ) except RequestError as e: raise McpError( @@ -181,6 +188,8 @@ def configure_mail_tools(mcp: FastMCP): Returns: GetMessageResponse with the full message including body and attachments. + Attachments with ``id: null`` are inline body parts and cannot be + fetched via nc_mail_get_attachment (which requires a string id). """ client = await get_client(ctx) try: diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 5b2ccf2c..d0486b6f 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -855,6 +855,13 @@ async def _index_document( if not is_valid_nextcloud_doc_id(doc_task.doc_id): raise ValueError(f"Invalid mail_message doc_id: {doc_task.doc_id!r}") message = await nc_client.mail.get_message(int(doc_task.doc_id)) + # An empty payload (OCS data=null with a <400 meta) would otherwise + # index a useless near-empty placeholder; fail loudly so the task + # dead-letters instead of corrupting the index. + if not message: + raise ValueError( + f"mail_message {doc_task.doc_id!r} returned an empty payload" + ) content = build_mail_content(message) subject = message.get("subject") or "" diff --git a/tests/unit/vector/test_scanner_mail.py b/tests/unit/vector/test_scanner_mail.py index c4cfe39e..df1c1031 100644 --- a/tests/unit/vector/test_scanner_mail.py +++ b/tests/unit/vector/test_scanner_mail.py @@ -229,3 +229,24 @@ async def test_incremental_deletes_after_grace_period(mocker): 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 + + +async def test_incremental_first_missing_starts_grace(mocker): + """A newly-missing indexed message enters the grace period (no delete yet).""" + _patch_incremental(mocker, indexed_ids=["999"], existing_metadata=None) + # Not previously seen as missing, and the mailbox now returns no messages. + 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, + ) + + # First miss only starts the grace period — nothing queued, nothing deleted. + assert queued == 0 + assert stream.tasks == [] + assert ("alice", "999") in scanner_module._potentially_deleted From 15042c1b1bd382751fb391e2e36b19325bd729cd Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 20 Jun 2026 13:44:28 +0200 Subject: [PATCH 8/8] polish(mail): consistency guards (PR #935 round-7) Non-blocking consistency fixes: - scanner.py: skip re-queuing a mail_message whose placeholder status is "failed" (mirrors the file scanner's permanent-failure guard); the modified_at branch still retries once the message changes. - search/context.py: return None on an empty get_message payload during context expansion, mirroring the processor's index-time empty-payload guard. - tests: cover the non-numeric mailbox_id fail-open branch in the verifier. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/context.py | 4 ++++ nextcloud_mcp_server/vector/scanner.py | 10 ++++++++++ tests/unit/search/test_verification.py | 16 ++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index e57c6602..d4219eea 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -834,6 +834,10 @@ async def _fetch_document_text( # 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)) + # Empty payload (OCS data=null with a <400 meta) -> skip context + # expansion, mirroring the processor's index-time guard. + if not message: + return None return build_mail_content(message) else: logger.warning("Unsupported doc_type for context expansion: %s", doc_type) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index b4b0eea9..0468dc98 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -1549,6 +1549,16 @@ async def scan_mail_messages( needs_indexing = True elif existing_metadata.get("modified_at", 0) < modified_at: needs_indexing = True + elif existing_metadata.get("status") == "failed": + # A permanent processing failure — don't re-queue an + # unchanged message that will just fail again; the + # modified_at branch above retries once it changes + # (mirrors the file scanner's failed-placeholder guard). + logger.debug( + "Skipping mail message %s: previous processing " + "failed permanently", + doc_id, + ) elif existing_metadata.get("is_placeholder", False): queued_at = existing_metadata.get("queued_at", 0) placeholder_age = time.time() - queued_at diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index 9edbea24..5fbb4338 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -303,6 +303,22 @@ async def test_verify_mail_missing_mailbox_id_keeps(mocker): list_messages.assert_not_awaited() +@pytest.mark.unit +async def test_verify_mail_non_numeric_mailbox_id_keeps(mocker): + """A non-numeric mailbox_id in metadata is kept without a list call.""" + list_messages = mocker.AsyncMock() + mail_client = SimpleNamespace(list_messages=list_messages) + client = SimpleNamespace(mail=mail_client, username="alice") + + result = await _verify_mail_messages( + client, + [_make_result(42, doc_type="mail_message", metadata={"mailbox_id": "bad"})], + _sem(), + ) + assert result == {"42"} + list_messages.assert_not_awaited() + + @pytest.mark.unit async def test_verify_mail_non_numeric_id_kept_when_mailbox_listed(mocker): """A malformed doc_id can't match the numeric listing, so it's kept."""