diff --git a/CLAUDE.md b/CLAUDE.md index c25c175..6528caf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,6 +33,7 @@ text_utils.py <- Shared helpers (extract_spoken_text, ...) tts_synth.py <- Edge TTS helper shared by /api/tts + mobile relay soul_review.py <- Review gate for soul.md overwrites (off/log/confirm) tool_guard.py <- Approval gate for agent tool calls (off/log/confirm) — gates run_command via Arcana confirmation_callback +mcp_bootstrap.py <- Connect external MCP servers at startup; register their tools (gated as external writes) filevault_status.py <- macOS fdesetup probe for the console warning banner tools/ <- Arcana tools (agent's capabilities) @@ -156,6 +157,15 @@ Origin tracking via `tool_guard.current_origin` contextvar (set to `"local"` in Deployment recommendation lives in `SECURITY.md` §7 (matrix keyed on actual exposure). Short version: `log` is the floor for any new install; `confirm` once Telegram/relay/untrusted-`session_watcher` is in the picture; `off` is backwards-compat default, not a recommendation. +### MCP Integration (External Tools) +`mcp_bootstrap.py` connects external [MCP](https://modelcontextprotocol.io) servers and registers their tools as first-class agent tools. Config lives under `mcp_servers:` in `config.yaml` (a list of server dicts → `arcana.contracts.mcp.MCPServerConfig`); omit the block to connect none. Each server's tools are named `.` (e.g. `messageinfra.get_briefing`). + +- **Arcana API:** Arcana 1.0 has NO `Runtime(mcp_servers=...)`/`connect_mcp` — the real primitive is the standalone `arcana.mcp.setup.setup_mcp_tools(configs, registry)`. `mcp_bootstrap.connect_mcp_servers()` re-implements its connect→convert→register loop with **per-server AND per-tool graceful degrade**, so a down/slow/broken/hostile server (or one bad tool spec) is logged and skipped instead of aborting the batch. The handshake is bounded by the server's `timeout_ms` (default 30s), NOT an external `asyncio.wait_for` — cancelling Arcana's `connect()` mid-handshake orphans the spawned subprocess, because `connect()` registers the connection (so `disconnect_all()` can reach it) only after success and has no try/finally cleanup. A server that spawns then fails the handshake can still leak that subprocess (logged so it's observable); the real fix is an upstream try/finally in Arcana. Transports: `stdio` (subprocess) and `streamable_http`; `sse` is unimplemented upstream. +- **Startup:** `server.py`'s startup event parses `mcp_servers` and fires `_bootstrap_mcp()` as a **background task** (so a slow server doesn't delay readiness). It runs after `_get_runtime()` so the native-tool snapshot is taken *before* any MCP tool registers. `shutdown` calls `client.disconnect_all()`. +- **Gating:** MCP tools carry dotted names, so they fall OUTSIDE `tool_guard.set_native_tools()` → an MCP write tool (`side_effect=WRITE`) is **gated as an external write** (CONFIRM-mode modal), per the Phase-0 side-effect-first gate. Caveat: Arcana infers an MCP tool's side-effect from a name/description keyword heuristic (`_infer_side_effect`, default READ), so a write tool whose name dodges the keywords (e.g. messageinfra's `trigger_fetch`) classifies READ and is not gated — an Arcana-level residual. +- **Scope:** wired into the **local server loop only**. stdio transports are bound to the loop that spawned the subprocess, so the relay thread (own loop) and the Telegram process (separate process) don't yet share MCP tools — a documented follow-up. +- **First server:** `messageinfra` (local stdio, no OAuth) — `command: /python, args: [-m, messageinfra.mcp_server], env: {MESSAGEINFRA_URL: …}`. Process-isolated, so its `arcana 0.4.0` dep doesn't collide with Roboot's `.venv` 1.0.0. Gmail/Calendar would be an npx MCP server + OAuth first-run (deferred). See `config.example.yaml` for the shapes. + ### Chat History & Replay Two layers in `memory.py` on top of `chat_store` (SQLite `.chat_history.db`): diff --git a/config.example.yaml b/config.example.yaml index 5c9b6bd..d500448 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -97,6 +97,34 @@ remote_access: # Or deploy your own: see relay/ directory and relay/wrangler.toml # endpoint: "wss://your-own-relay.example.com" +# External tools via MCP (Model Context Protocol). Optional. +# Each connected server's tools become first-class agent tools, named +# `.` (e.g. `messageinfra.get_briefing`). Because they're +# external they are ALWAYS gated as writes by tool_guard when their side-effect +# is a write (so ROBOOT_TOOL_APPROVAL=confirm pops an approval modal before an +# MCP write runs). A server that's down / slow is skipped at startup without +# crashing the daemon. Currently wired into the local server only (not relay / +# Telegram). Omit this block entirely to connect no MCP servers. +# +# mcp_servers: +# # messageinfra — local info-filtering agent (briefing / digest / search). +# # stdio: Roboot spawns the server as a subprocess; it talks JSON-RPC over +# # stdin/stdout, so its own deps stay isolated from Roboot's .venv. Needs the +# # messageinfra service running (default MESSAGEINFRA_URL=http://localhost:9990). +# - name: messageinfra +# transport: stdio +# command: /Users/ty/anaconda3/bin/python +# args: ["-m", "messageinfra.mcp_server"] +# env: +# MESSAGEINFRA_URL: "http://localhost:9990" +# +# # Gmail / Calendar etc. via an npx MCP server (needs Node + a one-time OAuth +# # first-run in the spawned server). Example shape only: +# # - name: gmail +# # transport: stdio +# # command: npx +# # args: ["-y", "@your/gmail-mcp-server"] + # --------------------------------------------------------------------------- # Security hardening (set as environment variables, not config keys) # --------------------------------------------------------------------------- diff --git a/mcp_bootstrap.py b/mcp_bootstrap.py new file mode 100644 index 0000000..0bfbb14 --- /dev/null +++ b/mcp_bootstrap.py @@ -0,0 +1,175 @@ +"""Bootstrap external MCP servers into Roboot's Arcana runtime. + +Reads the `mcp_servers` block of config.yaml, connects each server via Arcana's +MCP client, and registers their tools into the gateway registry. MCP tools +register with dotted `server.tool` names, so they land OUTSIDE tool_guard's +native snapshot (`set_native_tools()`) and gate by default — see the Phase-0 +side-effect-first gate. An MCP write tool like `gmail.send_email` therefore +requires approval in CONFIRM mode rather than auto-executing. + +Arcana 1.0 ships the low-level `arcana.mcp.setup.setup_mcp_tools(configs, +registry)`, but it connects all servers in one pass and a single failure aborts +the batch. We re-implement the same connect→convert→register loop with +PER-SERVER graceful degrade + a connect timeout, so one slow/missing/broken +server can't hang or crash daemon startup. + +Scope: wired into the SERVER (local) event loop only for now. stdio transports +are bound to the loop that spawned the subprocess, so the relay thread (its own +loop) and the Telegram process (separate process) don't yet share these tools — +that's a documented follow-up. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from arcana.contracts.mcp import MCPServerConfig +from arcana.mcp.client import MCPClient +from arcana.mcp.protocol import mcp_tool_to_arcana_spec +from arcana.mcp.tool_provider import MCPToolProvider + +logger = logging.getLogger(__name__) + + +def parse_mcp_configs(config: dict) -> list[MCPServerConfig]: + """Map the `mcp_servers` block of config.yaml to MCPServerConfig objects. + + Each entry is a mapping accepted by `MCPServerConfig`: + `{name, transport?, command?, args?, env?, url?, headers?, timeout_ms?, + capability_prefix?}`. A malformed entry (not a mapping, or missing the + required `name` / an un-coercible field type) is skipped with a warning — + a bad MCP config line must never take the daemon down. Unknown extra fields + are silently ignored by the pydantic model (extra='ignore'), not skipped. + """ + raw = config.get("mcp_servers") + if not raw: + return [] + if not isinstance(raw, list): + logger.warning("config mcp_servers must be a list, ignoring") + return [] + + configs: list[MCPServerConfig] = [] + for i, entry in enumerate(raw): + if not isinstance(entry, dict): + logger.warning("mcp_servers[%d] is not a mapping, skipping", i) + continue + try: + configs.append(MCPServerConfig(**entry)) + except Exception: + logger.warning( + "mcp_servers[%d] (%r) invalid, skipping", + i, + entry.get("name", "?"), + exc_info=True, + ) + return configs + + +async def connect_mcp_servers( + registry: Any, + configs: list[MCPServerConfig], + *, + client: MCPClient | None = None, +) -> tuple[MCPClient | None, list[str]]: + """Connect each MCP server and register its tools into `registry`. + + Per-server graceful degrade: a server that fails to connect (down, + unreachable, handshake error, internal timeout) is logged and skipped; the + others still register. The per-tool registration loop is likewise guarded — + a single bad/hostile tool spec is skipped, never aborting the server or the + batch. Returns `(client, connected_names)`. + + The caller SHOULD pass a pre-created `client` and publish it for shutdown + BEFORE awaiting this — so a shutdown that races an in-flight connect can + still `disconnect_all()` whatever connected so far. One is created if None. + The caller MUST keep the client alive (its subprocess transports die with + it) and `await client.disconnect_all()` on shutdown. + + Connect bound: each server's handshake/list_tools is bounded by its + `MCPServerConfig.timeout_ms` (default 30s); we deliberately do NOT wrap the + connect in an external `asyncio.wait_for`, because cancelling Arcana's + `MCPClient.connect()` mid-handshake orphans the spawned subprocess — + `connect()` registers the connection (so `disconnect_all()` can reach it) + only after a fully successful handshake, and has no try/finally cleanup on + failure. A server that spawns its subprocess then fails the handshake may + therefore still leak that subprocess; we log it so it's observable. The real + fix is upstream in Arcana (wrap `connect()` in try/finally). Roboot can't + reach the half-open transport through the public client API. + """ + if not configs: + return client, [] + if client is None: + client = MCPClient() + + connected: list[str] = [] + for config in configs: + try: + mcp_tools = await client.connect(config) + except Exception: + logger.warning( + "MCP server %r failed to connect; skipping its tools " + "(a spawned subprocess may be orphaned — Arcana connect() has " + "no failure cleanup)", + config.name, + exc_info=True, + ) + # No-op if the connection never registered (the common failure + # case); cleans a fully-registered one if we somehow got here. + try: + await client.disconnect(config.name) + except Exception: + pass + continue + + registered = 0 + for mcp_tool in mcp_tools: + try: + spec = mcp_tool_to_arcana_spec( + mcp_tool, + server_name=config.name, + capability_prefix=config.capability_prefix, + ) + except Exception: + logger.warning( + "MCP tool %r.%r: spec conversion failed, skipping", + config.name, + getattr(mcp_tool, "name", "?"), + exc_info=True, + ) + continue + # Don't silently clobber an existing tool (a duplicate server name, + # or a tool already registered) — a same-named overwrite of a gated + # tool is a quiet failure mode. + if registry.get(spec.name) is not None: + logger.warning( + "MCP tool %r already registered; skipping (name collision)", + spec.name, + ) + continue + try: + registry.register( + MCPToolProvider( + client=client, + server_name=config.name, + mcp_tool_name=mcp_tool.name, + arcana_spec=spec, + ) + ) + registered += 1 + except Exception: + logger.warning( + "MCP tool %r: registration failed, skipping", + spec.name, + exc_info=True, + ) + + connected.append(config.name) + logger.info( + "MCP server %r connected: %d/%d tool(s) registered " + "(external → gated as writes by tool_guard)", + config.name, + registered, + len(mcp_tools), + ) + return client, connected diff --git a/server.py b/server.py index 31b8c50..a36762b 100644 --- a/server.py +++ b/server.py @@ -91,6 +91,8 @@ _runtime: arcana.Runtime | None = None _relay_client = None # Set when relay is enabled +_mcp_client = None # arcana MCPClient, published BEFORE connecting (keep alive) +_mcp_bootstrap_task = None # retained so the loop can't GC it; cancelled on shutdown def _load_config() -> dict: @@ -686,6 +688,44 @@ async def _start_session_watcher(): # Reminder dispatcher for the daemon-owned surfaces. Origin "" covers # reminders created by in-process callers that never set the contextvar. _scheduler.start_dispatcher(["local", "relay", ""], _deliver_reminder) + # Connect configured MCP servers in the BACKGROUND so a slow/missing server + # can't delay readiness. Their tools register as `server.tool` (dotted), + # landing outside tool_guard's native snapshot → gated as external writes. + global _mcp_bootstrap_task + import mcp_bootstrap + + mcp_configs = mcp_bootstrap.parse_mcp_configs(_load_config()) + if mcp_configs: + # Retain the task handle: an unreferenced task can be GC-cancelled on + # 3.11, and shutdown needs something to cancel/await. + _mcp_bootstrap_task = asyncio.create_task(_bootstrap_mcp(mcp_configs)) + + +async def _bootstrap_mcp(configs) -> None: + """Connect MCP servers into the (already-built) runtime registry. + + Runs after `_get_runtime()` so the native-tool snapshot is taken BEFORE any + MCP tool is registered — MCP tools must NOT be captured as native. The + MCPClient is created and published to `_mcp_client` BEFORE connecting, so a + shutdown that races this can still `disconnect_all()` whatever connected. + """ + global _mcp_client + import mcp_bootstrap + from arcana.mcp.client import MCPClient + + rt = _get_runtime() + if rt._tool_gateway is None: + return + _mcp_client = MCPClient() + try: + _, connected = await mcp_bootstrap.connect_mcp_servers( + rt._tool_gateway.registry, configs, client=_mcp_client + ) + except Exception: + logger.warning("MCP bootstrap failed", exc_info=True) + return + if connected: + logger.info("MCP servers connected: %s", ", ".join(connected)) @app.on_event("startup") @@ -704,7 +744,23 @@ async def _start_self_upgrade_loop(): @app.on_event("shutdown") async def shutdown(): - global _runtime + global _runtime, _mcp_client, _mcp_bootstrap_task + # Stop any in-flight MCP bootstrap first so it can't register more after we + # tear down; then disconnect everything that connected so far. Publishing + # _mcp_client before connecting means disconnect_all() reaches partial state. + if _mcp_bootstrap_task is not None: + _mcp_bootstrap_task.cancel() + try: + await _mcp_bootstrap_task + except BaseException: + pass + _mcp_bootstrap_task = None + if _mcp_client is not None: + try: + await _mcp_client.disconnect_all() + except Exception: + logger.warning("MCP disconnect_all failed", exc_info=True) + _mcp_client = None if _runtime: await _runtime.close() _runtime = None diff --git a/tests/test_mcp_bootstrap.py b/tests/test_mcp_bootstrap.py new file mode 100644 index 0000000..9431665 --- /dev/null +++ b/tests/test_mcp_bootstrap.py @@ -0,0 +1,233 @@ +"""Tests for mcp_bootstrap — config parsing + graceful MCP connect/register. + +No real MCP server is spawned: `MCPClient` is monkeypatched with a fake whose +per-server behavior (return tools / raise / hang) is keyed by config name. The +registry, MCPToolProvider, MCPToolSpec, and side-effect inference are the real +Arcana objects, so registration + dotted naming + WRITE classification are +exercised end-to-end. +""" + +from __future__ import annotations + +import asyncio +import logging + +import pytest + +import mcp_bootstrap +from arcana.contracts.mcp import MCPServerConfig, MCPToolSpec +from arcana.tool_gateway.registry import ToolRegistry + + +# ----------------------------------------------------------------------------- +# parse_mcp_configs +# ----------------------------------------------------------------------------- + + +def test_parse_missing_or_empty_returns_empty(): + assert mcp_bootstrap.parse_mcp_configs({}) == [] + assert mcp_bootstrap.parse_mcp_configs({"mcp_servers": None}) == [] + assert mcp_bootstrap.parse_mcp_configs({"mcp_servers": []}) == [] + + +def test_parse_non_list_ignored(caplog): + with caplog.at_level(logging.WARNING, logger=mcp_bootstrap.logger.name): + assert mcp_bootstrap.parse_mcp_configs({"mcp_servers": {"name": "x"}}) == [] + assert any("must be a list" in r.message for r in caplog.records) + + +def test_parse_valid_entry_maps_all_fields(): + cfg = { + "mcp_servers": [ + { + "name": "messageinfra", + "transport": "stdio", + "command": "/anaconda/bin/python", + "args": ["-m", "messageinfra.mcp_server"], + "env": {"MESSAGEINFRA_URL": "http://localhost:9990"}, + } + ] + } + out = mcp_bootstrap.parse_mcp_configs(cfg) + assert len(out) == 1 + c = out[0] + assert isinstance(c, MCPServerConfig) + assert c.name == "messageinfra" + assert c.command == "/anaconda/bin/python" + assert c.args == ["-m", "messageinfra.mcp_server"] + assert c.env["MESSAGEINFRA_URL"] == "http://localhost:9990" + assert c.transport.value == "stdio" + + +def test_parse_skips_malformed_entries(caplog): + cfg = { + "mcp_servers": [ + "not-a-dict", + {"transport": "stdio"}, # missing required 'name' + {"name": "ok", "command": "x"}, # valid + ] + } + with caplog.at_level(logging.WARNING, logger=mcp_bootstrap.logger.name): + out = mcp_bootstrap.parse_mcp_configs(cfg) + assert [c.name for c in out] == ["ok"] # bad entries skipped, not fatal + + +# ----------------------------------------------------------------------------- +# connect_mcp_servers (fake MCPClient) +# ----------------------------------------------------------------------------- + + +class _FakeClient: + """Stand-in for arcana MCPClient. `behavior[name]` is a list[MCPToolSpec] + to return, an Exception to raise, or the string 'hang' to sleep forever.""" + + behavior: dict = {} + last: "_FakeClient | None" = None + + def __init__(self): + self.disconnected: list[str] = [] + self.disconnect_all_called = False + _FakeClient.last = self + + async def connect(self, config): + beh = _FakeClient.behavior.get(config.name, []) + if isinstance(beh, Exception): + raise beh + return beh + + async def disconnect(self, name): + self.disconnected.append(name) + + async def disconnect_all(self): + self.disconnect_all_called = True + + +@pytest.fixture +def fake_client(monkeypatch): + _FakeClient.behavior = {} + _FakeClient.last = None + monkeypatch.setattr(mcp_bootstrap, "MCPClient", _FakeClient) + return _FakeClient + + +def _tool(name: str) -> MCPToolSpec: + return MCPToolSpec(name=name, description="x", input_schema={"type": "object"}) + + +async def test_connect_empty_returns_none(): + client, connected = await mcp_bootstrap.connect_mcp_servers(ToolRegistry(), []) + assert client is None + assert connected == [] + + +async def test_connect_success_registers_dotted_tools(fake_client): + fake_client.behavior = {"mi": [_tool("get_briefing"), _tool("send_alert")]} + reg = ToolRegistry() + client, connected = await mcp_bootstrap.connect_mcp_servers( + reg, [MCPServerConfig(name="mi", command="x")] + ) + assert connected == ["mi"] + names = set(reg.list_tools()) + # Dotted server.tool naming → these land OUTSIDE tool_guard's native set. + assert "mi.get_briefing" in names + assert "mi.send_alert" in names + # Arcana classifies 'send_*' WRITE / 'get_*' READ; the WRITE one gates. + assert reg.get("mi.send_alert").spec.side_effect.value == "write" + assert reg.get("mi.get_briefing").spec.side_effect.value == "read" + + +async def test_connect_graceful_degrade(fake_client, caplog): + fake_client.behavior = { + "down": RuntimeError("connection refused"), + "up": [_tool("get_x")], + } + reg = ToolRegistry() + cfgs = [ + MCPServerConfig(name="down", command="x"), + MCPServerConfig(name="up", command="x"), + ] + with caplog.at_level(logging.WARNING, logger=mcp_bootstrap.logger.name): + client, connected = await mcp_bootstrap.connect_mcp_servers(reg, cfgs) + # The down server is skipped; the up server still registers (no crash). + assert connected == ["up"] + assert "up.get_x" in set(reg.list_tools()) + assert "down" in fake_client.last.disconnected # half-open cleanup attempted + assert any("down" in r.message for r in caplog.records) + + +async def test_connect_failure_skips_server(fake_client): + """A server whose connect raises (down / internal handshake timeout) is + skipped; siblings still register. We deliberately don't use an external + wait_for (cancelling connect orphans the subprocess), so 'timeout' arrives + as an exception from the bounded handshake.""" + fake_client.behavior = { + "slow": asyncio.TimeoutError("handshake timed out"), + "ok": [_tool("get_x")], + } + reg = ToolRegistry() + cfgs = [ + MCPServerConfig(name="slow", command="x"), + MCPServerConfig(name="ok", command="x"), + ] + client, connected = await mcp_bootstrap.connect_mcp_servers(reg, cfgs) + assert connected == ["ok"] + assert "ok.get_x" in set(reg.list_tools()) + + +async def test_connect_skips_colliding_tool(fake_client, caplog): + """A tool whose dotted name already exists (e.g. duplicate server name) is + skipped, not silently overwritten.""" + fake_client.behavior = {"mi": [_tool("get_x")]} + reg = ToolRegistry() + cfgs = [ + MCPServerConfig(name="mi", command="a"), + MCPServerConfig(name="mi", command="b"), # same dotted names + ] + with caplog.at_level(logging.WARNING, logger=mcp_bootstrap.logger.name): + client, connected = await mcp_bootstrap.connect_mcp_servers(reg, cfgs) + assert connected == ["mi", "mi"] + assert sorted(reg.list_tools()) == ["mi.get_x"] # registered once + assert any("collision" in r.message for r in caplog.records) + + +async def test_connect_skips_bad_tool_spec(fake_client, monkeypatch, caplog): + """A tool whose spec conversion raises (hostile/buggy server) is skipped; + sibling tools and later servers are unaffected — no client leak, no abort.""" + fake_client.behavior = {"mi": [_tool("bad"), _tool("good")]} + real = mcp_bootstrap.mcp_tool_to_arcana_spec + + def flaky(mcp_tool, **kw): + if mcp_tool.name == "bad": + raise ValueError("hostile schema") + return real(mcp_tool, **kw) + + monkeypatch.setattr(mcp_bootstrap, "mcp_tool_to_arcana_spec", flaky) + reg = ToolRegistry() + with caplog.at_level(logging.WARNING, logger=mcp_bootstrap.logger.name): + client, connected = await mcp_bootstrap.connect_mcp_servers( + reg, [MCPServerConfig(name="mi", command="x")] + ) + assert connected == ["mi"] + assert set(reg.list_tools()) == {"mi.good"} # bad skipped, good kept + assert any("spec conversion failed" in r.message for r in caplog.records) + + +async def test_connect_uses_and_returns_passed_client(fake_client): + """A pre-created client (published before connect for shutdown safety) is + used and returned, not replaced.""" + fake_client.behavior = {"mi": [_tool("get_x")]} + pre = mcp_bootstrap.MCPClient() # the fake, via fixture monkeypatch + client, connected = await mcp_bootstrap.connect_mcp_servers( + ToolRegistry(), [MCPServerConfig(name="mi", command="x")], client=pre + ) + assert client is pre + + +async def test_connect_returns_live_client_for_shutdown(fake_client): + fake_client.behavior = {"mi": [_tool("get_x")]} + client, connected = await mcp_bootstrap.connect_mcp_servers( + ToolRegistry(), [MCPServerConfig(name="mi", command="x")] + ) + assert client is not None + await client.disconnect_all() + assert client.disconnect_all_called is True