feat: MCP server exposing mailrag email-RAG query - #67
Conversation
Add src/mcp_server, a thin Model Context Protocol (stdio) server that wraps the existing hybrid searcher (build_hybrid_searcher/search_threads) and grounded answer path (answer_from_threads) as two tools: - search_email(query, top_k) -> ranked, attributed email threads (no LLM) - answer_question(query, k) -> grounded RAG answer + sources (one LLM call) Config mirrors 'mailrag ask': collection resolves from --collection / MAILRAG_COLLECTION / latest onboarding manifest; Qdrant URL from --qdrant-url / MAILRAG_QDRANT_URL / QDRANT_URL / localhost. Missing corpus yields a clear error rather than a crash. Launch via 'mailrag mcp' (new CLI verb) or 'python -m src.mcp_server'. Adds the official 'mcp' SDK to pyproject + lock + requirements. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover config resolution (collection/qdrant-url precedence + clear error when no corpus), searcher build+cache via injectable factory, tool result mapping and top_k/k truncation, invalid input, empty results, FastMCP tool registration (names + input schema), a call_tool dispatch through the server, and the 'mailrag mcp' CLI wiring. No live Qdrant or LLM required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add docs/MCP_SERVER.md (tool surface, launch, config precedence table, MCP-client registration example) and wire it into the README quickstart, project layout, documentation index, and roadmap (the #32 query path is now live). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| """ | ||
| if not query or not query.strip(): | ||
| raise ValueError("query must be a non-empty string") | ||
| if top_k < 1: |
There was a problem hiding this comment.
No upper bound on top_k — a misbehaving or adversarial MCP client can pass top_k=1_000_000, which makes search_threads return everything it can and then serialises the full result set. Same issue with k at line 138.
Consider capping at a reasonable maximum (e.g. 50) and raising a ValueError rather than silently accepting huge values:
| if top_k < 1: | |
| if top_k < 1 or top_k > 50: | |
| raise ValueError("top_k must be between 1 and 50") |
(Choose a limit that matches what the searcher can meaningfully return; 50 is just a placeholder.)
| if not query or not query.strip(): | ||
| raise ValueError("query must be a non-empty string") | ||
| if k < 1: | ||
| raise ValueError("k must be >= 1") |
There was a problem hiding this comment.
Same missing upper-bound issue as top_k above — k=10000 would pass through to answer_from_threads with no guard. If you add a cap to search_email, add one here too.
| # FastMCP (this SDK version) returns (content_blocks, structured_result). | ||
| content_blocks, structured = result |
There was a problem hiding this comment.
This unpacks FastMCP.call_tool's return value as (content_blocks, structured_result), which is an SDK-internal implementation detail of mcp 1.28.1 — not part of the public API. The comment acknowledges it, but the test will fail silently (wrong assertion) or loudly (unpack error) if the SDK changes its return format on upgrade.
Two options:
- Test the public effect instead: mock the module-level
search_emailfunction and assert it was called with the right args — that verifies dispatch without depending on FastMCP internals. - Wrap the unpack in a try/except with a clear message so a future breakage points at the SDK change rather than a mystery index error.
Either way, a # mcp==1.28.1 call_tool return format version pin comment would make the assumption auditable.
| set(by_name["answer_question"].inputSchema["properties"]), | ||
| {"query", "k"}) | ||
|
|
||
| def test_call_tool_dispatches_into_search_email(self): |
There was a problem hiding this comment.
answer_question has no call_tool dispatch test — only search_email is exercised through the FastMCP layer here. The unit tests in TestAnswerQuestion cover the function logic well, but they don't catch a broken tool registration (wrong name, missing kwarg, wrong return type in the MCP wrapper).
A minimal twin of this test for answer_question (mocking both get_searcher and answer_from_threads) would close that gap.
Bring the MCP server PR up to date with main's CI hardening (ruff format, ruff/mypy/pip-audit gates, 85% coverage floor, CodeQL) and deps bump (cryptography/pypdf/tornado). Conflict resolution: - src/cli.py: kept main's ruff-expanded _add_verb style AND #67's `mcp` verb (run the stdio MCP server), inserted after `ask`. - pyproject.toml: kept BOTH main's [tool.ruff]/[tool.mypy]/pytest config AND #67's `mcp = "^1.28.1"` dependency. - poetry.lock: regenerated with `poetry lock` (not hand-merged); `poetry check --lock` passes. - requirements.txt / README.md: kept both sides' intent. src/mcp_server/ type-checks cleanly under the existing lenient mypy config (ignore_missing_imports covers the mcp SDK), so no per-module override was needed. ruff check/format clean; full suite 913 passed, coverage 87.9%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| manifest. Raises ``ValueError`` with an actionable message when none is | ||
| available (so the MCP client sees a clear error rather than a crash). | ||
| """ | ||
| coll = collection or os.environ.get("MAILRAG_COLLECTION") or latest_manifest_collection() |
There was a problem hiding this comment.
Bug: whitespace collection name passes through silently.
resolve_qdrant_url strips the resolved value and falls back to the default when blank. resolve_collection has no equivalent guard. If MAILRAG_COLLECTION (or the manifest .get("collection")) holds an all-whitespace string, the or-chain returns it as truthy, if not coll stays False, and a garbage collection name is forwarded to Qdrant — producing a confusing Collection not found rather than the intended clear ValueError.
| coll = collection or os.environ.get("MAILRAG_COLLECTION") or latest_manifest_collection() | |
| coll = (collection or os.environ.get("MAILRAG_COLLECTION") or latest_manifest_collection() or "").strip() |
| or os.environ.get("MAILRAG_QDRANT_URL") | ||
| or os.environ.get("QDRANT_URL") | ||
| or DEFAULT_QDRANT_URL | ||
| ).strip() or DEFAULT_QDRANT_URL |
There was a problem hiding this comment.
The .strip() or DEFAULT_QDRANT_URL at the end acts on the entire or-chain result, not just DEFAULT_QDRANT_URL — so a whitespace env-var value correctly falls back. This is good, but it reads as if the strip only applies to the last element. A comment prevents an accidental "simplification" that drops the fallback:
| ).strip() or DEFAULT_QDRANT_URL | |
| ).strip() or DEFAULT_QDRANT_URL # strip covers all branches; blank env var falls back to default |
| """Construct the ``FastMCP`` server with the mailrag tools registered. | ||
|
|
||
| Imported lazily so importing this module (e.g. for unit tests of the pure | ||
| query functions) does not require the ``mcp`` SDK at import time. |
There was a problem hiding this comment.
The docstring says "importing this module … does not require the mcp SDK at import time," but the module already imports answer_from_threads, latest_manifest_collection, and build_hybrid_searcher at the top level. What's actually lazy is just the mcp.server.fastmcp import inside this function — tighten the claim so readers (and future refactors) aren't misled:
Defers the
mcp.server.fastmcpimport so test modules that only exercise the pure query helpers don't require themcpSDK installed.
| def test_call_tool_dispatches_into_search_email(self): | ||
| srv = server.build_server() | ||
| searcher = _FakeSearcher(_threads()) | ||
| with mock.patch("src.mcp_server.server.get_searcher", return_value=searcher): | ||
| result = asyncio.run(srv.call_tool("search_email", {"query": "invoices", "top_k": 1})) | ||
| # FastMCP (this SDK version) returns (content_blocks, structured_result). | ||
| content_blocks, structured = result | ||
| rows = structured["result"] | ||
| self.assertEqual(rows[0]["thread_id"], "t1") | ||
| # The text content mirrors the same payload. | ||
| self.assertIn("t1", content_blocks[0].text) |
There was a problem hiding this comment.
This test unpacks the call_tool return as (content_blocks, structured) and accesses structured["result"] — coupling to an internal FastMCP return shape that is not part of the public MCP protocol contract. If the SDK bumps from 1.28.x to a 1.29.x (within the ^1.28.1 pin) and changes this tuple layout, the test will fail with an unhelpful ValueError: not enough values to unpack rather than a meaningful assertion message.
Consider asserting only on the observable MCP-protocol output (e.g. the content blocks' text), or at minimum add a comment that this shape is version-specific so a future breakage is immediately diagnosable.
| def test_mcp_verb_invokes_serve_and_sets_env(self): | ||
| from src import cli | ||
|
|
||
| with ( | ||
| mock.patch("src.mcp_server.server.serve") as serve, | ||
| mock.patch.dict("os.environ", {}, clear=True), | ||
| ): | ||
| rc = cli.main( | ||
| ["mcp", "--collection", "work-rag", "--qdrant-url", "http://localhost:6333"] | ||
| ) | ||
| self.assertEqual(rc, 0) | ||
| serve.assert_called_once() | ||
| import os | ||
|
|
||
| self.assertEqual(os.environ["MAILRAG_COLLECTION"], "work-rag") | ||
| self.assertEqual(os.environ["MAILRAG_QDRANT_URL"], "http://localhost:6333") |
There was a problem hiding this comment.
Missing test: mailrag mcp with no flags (the most common invocation).
This test only covers the path where both --collection and --qdrant-url are supplied. There's no coverage for running without flags, where collection resolution falls through to $MAILRAG_COLLECTION → manifest. A minimal addition:
def test_mcp_verb_no_flags_still_calls_serve(self):
from src import cli
with (
mock.patch("src.mcp_server.server.serve") as serve,
mock.patch.dict("os.environ", {"MAILRAG_COLLECTION": "my-col"}, clear=False),
):
rc = cli.main(["mcp"])
self.assertEqual(rc, 0)
serve.assert_called_once()
# env var must not be overwritten when flag was absent
self.assertEqual(os.environ["MAILRAG_COLLECTION"], "my-col")
Review summaryClean, well-scoped addition. The thin-wrapper design is correct — no retrieval logic is reimplemented, the injectable-factory pattern makes tests fast, config precedence mirrors Correctness
Tests
Dependency footprint
Nits (no action required)
|
What
A new stdio MCP (Model Context Protocol) server (
src/mcp_server/) that exposes mailrag's existing email-RAG query pipeline to MCP clients (Claude Desktop, Claude Code, or any MCP-capable agent). It is a thin wrapper — retrieval and answering reuse the same code paths asmailrag ask; nothing about ranking, fusion, or answering is reimplemented.Addresses the query half of the MCP-server roadmap item (#32).
Tool surface
search_emailsearch_email(query, top_k=5)HybridSearcher.search_threads. Returns up totop_krows{thread_id, subject, num_emails, text}. No LLM call.answer_questionanswer_question(query, k=3)kwithanswer_from_threads. Returns{answer, sources:[{thread_id, subject}]}.Launch
Blocks on stdio until the client disconnects. Registration example for Claude clients is in
docs/MCP_SERVER.md.Config (mirrors
mailrag ask)--collection→$MAILRAG_COLLECTION→ latest onboarding manifest. Missing corpus → clear error, not a crash.--qdrant-url→$MAILRAG_QDRANT_URL→$QDRANT_URL→http://localhost:6333. The dedicatedMAILRAG_QDRANT_URLoverride avoids inheriting the container-orientedQDRANT_URLon the host (the query:mailrag queryfails on the host — QDRANT_URL in .env points to host.docker.internal #29 gotcha).Settings.llmstack (usualRAG_*env vars).Packaging
mcpSDK topyproject.toml, regeneratedpoetry.lock, andrequirements.txt.mailrag mcpverb wired intosrc/cli.py.Tests
New
tests/test_mcp_server.py(18 cases, retrieval layer fully mocked — no live Qdrant/LLM): config resolution + precedence + clear-error-on-missing-corpus, searcher build/cache via injectable factory, result mapping +top_k/ktruncation, invalid input, empty results, FastMCP tool registration (names + input schema), acall_tooldispatch, and themailrag mcpCLI wiring.Full suite: 913 passed, 1 skipped in the
mailrag-testconda env.Open questions / notes
(collection, qdrant_url)for the server's lifetime; there is no live re-indexing, matching current mailrag behaviour.🤖 Generated with Claude Code