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 == []