From e42f0989e5b269e3fb4ab11cce19ca2b936763b2 Mon Sep 17 00:00:00 2001 From: PowerCreek Date: Mon, 25 May 2026 00:40:13 +0000 Subject: [PATCH] fix(mcp): defensive registrar wrap + boot-line log + contract test (closes #88) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One failing devagentic-side registrar (canvas / docs / mutations / github / lane-h) no longer kills peer registrars in create_mcp_server — each call is wrapped in try/except with a WARN log naming the family. Boot-line INFO log on server creation: "MCP server boot: registered N tools: [...]" — deployed-container stderr now exposes exactly which tools are present in that wheel, instead of the operator having to truncate-enumerate (which is what triggered the original #88 false alarm). Contract test asserts 14 named G1/G2/G3/G4 + canvas + docs tools are present after create_mcp_server returns. A future refactor that drops a registrar surfaces as a CI failure with a precise "Missing: {...}" diff. Also defensive tests for the wrap+WARN path and the boot-line emission, including when a registrar fails. Fixed a separate latent regression in TestToolRegistration::test_all_tools_registered — the exact-equality assertion against 10 messaging tool names had been failing silently since the G1-G4 cascade landed (now 32 tools registered). Switched to subset semantics; the new contract test in test_mcp_defensive_registrars.py covers the rest. --- mcp_serve.py | 39 +++++- tests/test_mcp_defensive_registrars.py | 183 +++++++++++++++++++++++++ tests/test_mcp_serve.py | 3 +- 3 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 tests/test_mcp_defensive_registrars.py diff --git a/mcp_serve.py b/mcp_serve.py index 16fc0cb4b5d56..d7e74db14af22 100644 --- a/mcp_serve.py +++ b/mcp_serve.py @@ -872,11 +872,40 @@ def permissions_respond( # strings — no MCP tool ever raises. # --------------------------------------------------------------------- - _register_canvas_tools(mcp) - _register_docs_tools(mcp) - _register_devagentic_mutation_tools(mcp) - _register_github_tools(mcp) - _register_lane_h_tools(mcp) + # Each registrar is wrapped defensively: if one fails (e.g., a + # plugin module is malformed in a deployed wheel), the others + # still register and the failure surfaces in stderr instead of + # crashing the whole server. Per issue #88. + _devagentic_registrars = ( + ("canvas", _register_canvas_tools), + ("docs", _register_docs_tools), + ("devagentic-mutations", _register_devagentic_mutation_tools), + ("github", _register_github_tools), + ("lane-h", _register_lane_h_tools), + ) + for _label, _registrar in _devagentic_registrars: + try: + _registrar(mcp) + except Exception as exc: + logger.warning( + "MCP server: registrar %r failed: %s — peers still register", + _label, exc, + ) + + # Boot-line summary so deployed-container subprocess stderr exposes + # exactly which tools are present in the loaded mcp_serve build. + # Without this, operators have to truncate-enumerate (see #88 false + # alarm). Per issue #88. + try: + _tool_names = sorted( + getattr(t, "name", "") for t in mcp._tool_manager.list_tools() + ) + logger.info( + "MCP server boot: registered %d tools: %s", + len(_tool_names), _tool_names, + ) + except Exception as exc: + logger.debug("MCP server boot: tool enumeration failed: %s", exc) return mcp diff --git a/tests/test_mcp_defensive_registrars.py b/tests/test_mcp_defensive_registrars.py new file mode 100644 index 0000000000000..2ac93c3559b18 --- /dev/null +++ b/tests/test_mcp_defensive_registrars.py @@ -0,0 +1,183 @@ +"""Defensive-hardening + contract tests for ``create_mcp_server`` +(issue #88). + +The original #88 triage uncovered three asks once the false-alarm was +ruled out: + +1. One failing registrar should not crash peer registrars — wrap each + call in try/except + WARN log. +2. Boot-line log of registered tools so deployed-container stderr + shows EXACTLY which tools are present in that wheel (without the + operator having to truncate-enumerate). +3. Contract test asserting the G2/G3/G4 tool families are present, so + a registrar being dropped or its decorator path breaking surfaces + as a CI failure instead of in production. +""" +from __future__ import annotations + +import logging + +import pytest + + +pytest.importorskip("mcp", reason="MCP SDK not installed") + + +# --------------------------------------------------------------------------- +# (3) contract — G1/G2/G3/G4 tool families +# --------------------------------------------------------------------------- + +# Canvas (G2 pre-cascade), docs (canvas sibling), devagentic-mutations +# (G2), github (G3), lane-h + grafted-context (G4) tool families. Pulled +# out by name so a dropped registrar surfaces as a precise diff. +_REQUIRED_DEVAGENTIC_TOOLS = { + # G2 — devagentic-mutations + "silo_query", + "confer_run", + # G3 — hermes-github + "file_issue", + # G4 — devagentic-lane-h + "lane_h_list", + "lane_h_fetch", + "grafted_context_fetch", + # canvas family (pre-G2 cascade, devagentic#203 G2 sibling) + "canvas_list", + "canvas_open", + "canvas_add_node", + "canvas_search", + # docs family (canvas sibling) + "doc_search", + "doc_write", + "doc_show", + "fork_open", +} + + +def test_devagentic_tools_all_registered(): + """All G1–G4 + canvas + docs tools surface on a fresh server. + + If a registrar gets dropped (e.g., a future refactor removes the + call from create_mcp_server) or its decorator path breaks + (e.g., the @mcp.tool() decorator import path changes), this test + fails with a precise "Missing: {...}" diff naming the dropped + tools. + """ + import mcp_serve + + server = mcp_serve.create_mcp_server() + tools = server._tool_manager.list_tools() + tool_names = {t.name for t in tools} + + missing = _REQUIRED_DEVAGENTIC_TOOLS - tool_names + assert not missing, ( + f"Missing devagentic tools: {missing}. " + f"Registered: {sorted(tool_names)}" + ) + + +# --------------------------------------------------------------------------- +# (1) defensive — one failing registrar does not kill peers +# --------------------------------------------------------------------------- + +def test_failing_registrar_does_not_crash_server(monkeypatch, caplog): + """If one registrar raises during create_mcp_server, the others + still register and the failure surfaces as a WARN log line.""" + import mcp_serve + + def _broken_registrar(_mcp): + raise RuntimeError("synthetic registrar failure (test-only)") + + # Replace _register_lane_h_tools with the broken stand-in. + monkeypatch.setattr(mcp_serve, "_register_lane_h_tools", _broken_registrar) + + with caplog.at_level(logging.WARNING, logger="hermes.mcp_serve"): + server = mcp_serve.create_mcp_server() + + # Peer registrars still ran — canvas + docs + mutations + github + # should be present even though lane-h blew up. + tools = server._tool_manager.list_tools() + tool_names = {t.name for t in tools} + assert "canvas_list" in tool_names + assert "doc_search" in tool_names + assert "silo_query" in tool_names + assert "file_issue" in tool_names + + # And lane-h tools are NOT there (the broken registrar didn't run). + assert "lane_h_list" not in tool_names + + # WARN log surfaces the failure. + warn_msgs = [r.getMessage() for r in caplog.records + if r.levelno == logging.WARNING] + assert any("lane-h" in m and "synthetic registrar failure" in m + for m in warn_msgs), warn_msgs + + +def test_failing_registrar_warn_names_the_label(monkeypatch, caplog): + """Each registrar carries a human label; the WARN line uses it so + operators can identify which family failed without grepping + function names.""" + import mcp_serve + + def _broken(_mcp): + raise ValueError("kaboom") + + monkeypatch.setattr(mcp_serve, "_register_github_tools", _broken) + + with caplog.at_level(logging.WARNING, logger="hermes.mcp_serve"): + mcp_serve.create_mcp_server() + + warn_msgs = [r.getMessage() for r in caplog.records + if r.levelno == logging.WARNING] + # Label appears verbatim in the WARN line. + assert any("github" in m for m in warn_msgs), warn_msgs + + +# --------------------------------------------------------------------------- +# (2) boot-line log surfaces the registered tool inventory +# --------------------------------------------------------------------------- + +def test_boot_line_lists_registered_tools(caplog): + """A single INFO-level log line at create_mcp_server completion + shows the count + sorted tool names. Operators rely on this to + confirm what is present in a deployed wheel.""" + import mcp_serve + + with caplog.at_level(logging.INFO, logger="hermes.mcp_serve"): + mcp_serve.create_mcp_server() + + info_msgs = [r.getMessage() for r in caplog.records + if r.levelno == logging.INFO] + boot_lines = [m for m in info_msgs if "MCP server boot:" in m] + assert boot_lines, f"No boot-line log emitted. INFO log: {info_msgs}" + + line = boot_lines[-1] + # Tool count present. + assert "registered " in line and " tools:" in line + # Sample of expected names visible in the line (sorted, so each + # appears once in alpha order). + for name in ("canvas_list", "doc_search", "lane_h_list", + "silo_query", "file_issue"): + assert name in line, f"Tool {name!r} missing from boot line: {line}" + + +def test_boot_line_runs_even_when_a_registrar_fails(monkeypatch, caplog): + """The boot line still emits when one registrar fails — operator + sees both the WARN (which family failed) and the INFO (what's + actually present) on the same boot.""" + import mcp_serve + + def _broken(_mcp): + raise RuntimeError("test failure") + monkeypatch.setattr(mcp_serve, "_register_canvas_tools", _broken) + + with caplog.at_level(logging.INFO, logger="hermes.mcp_serve"): + mcp_serve.create_mcp_server() + + info_msgs = [r.getMessage() for r in caplog.records + if r.levelno == logging.INFO] + boot_lines = [m for m in info_msgs if "MCP server boot:" in m] + assert boot_lines, "Boot line must emit even when a registrar fails" + # Canvas tools are NOT in the inventory (broken registrar). + assert "canvas_list" not in boot_lines[-1] + # Peer family tools ARE in the inventory. + assert "lane_h_list" in boot_lines[-1] diff --git a/tests/test_mcp_serve.py b/tests/test_mcp_serve.py index 86e3ae0bd3838..400ee0209f11e 100644 --- a/tests/test_mcp_serve.py +++ b/tests/test_mcp_serve.py @@ -936,7 +936,8 @@ def test_all_tools_registered(self, mcp_server_e2e, _event_loop): "messages_send", "channels_list", "permissions_list_open", "permissions_respond", } - assert expected == tool_names, f"Missing: {expected - tool_names}, Extra: {tool_names - expected}" + missing = expected - tool_names + assert not missing, f"Missing messaging tools: {missing}" def test_tools_have_descriptions(self, mcp_server_e2e, _event_loop): server, _ = mcp_server_e2e