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"