fix: address review findings for Epic 4.9

- C1: test_tools_registered now asserts all 4 tool names (was no-op)
- M1: mcp_server.py uses lazy proxy — no wasted FastMCP on console script
- L1: register_handlers docstring updated to reference create_mcp_app()

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Brian Krafft

openspace/mcp/tool_handlers.py

@@ -791,7 +791,7 @@ async def upload_skill(
 def register_handlers(mcp) -> None:
     """Wire all tool handlers to the given FastMCP instance.
 
-    Called by ``mcp_server.py`` after creating the FastMCP app.
+    Called by ``create_mcp_app()`` in ``openspace.mcp.server``.
     """
     mcp.tool()(execute_task)
     mcp.tool()(search_skills)

openspace/mcp_server.py

@@ -25,10 +25,27 @@
     run_mcp_server,
 )
 
-# Create the module-level mcp instance for backward compatibility.
-# Some code (e.g., test_mcp_auth.py) imports mcp_server and expects
-# the FastMCP `mcp` instance to exist at module level.
-mcp = create_mcp_app()
+# Lazy module-level mcp instance for backward compat.
+# Some code imports mcp_server.mcp directly. Created on first access
+# to avoid wasting a FastMCP instance when only run_mcp_server() is needed.
+_mcp = None
+
+
+def _get_mcp():
+    global _mcp
+    if _mcp is None:
+        _mcp = create_mcp_app()
+    return _mcp
+
+
+class _LazyMcpProxy:
+    """Proxy that creates the FastMCP instance on first attribute access."""
+
+    def __getattr__(self, name):
+        return getattr(_get_mcp(), name)
+
+
+mcp = _LazyMcpProxy()
 
 if __name__ == "__main__":
-    run_mcp_server(mcp)
+    run_mcp_server(_get_mcp())

tests/test_mcp_server.py

@@ -101,14 +101,15 @@ def test_returns_fastmcp_instance(self):
 
     def test_tools_registered(self):
         app = create_mcp_app()
-        # FastMCP stores tools internally — verify our 4 are present
-        tools = None
-        if hasattr(app, "_tool_manager"):
-            tools = app._tool_manager
-        elif hasattr(app, 'list_tools'):
-            pass
-        # At minimum, the app was created without error
-        assert app is not None
+        # Verify all 4 MCP tools are actually wired
+        if hasattr(app, "_tool_manager") and hasattr(app._tool_manager, "_tools"):
+            tool_names = set(app._tool_manager._tools.keys())
+            assert tool_names == {"execute_task", "search_skills", "fix_skill", "upload_skill"}, (
+                f"Expected 4 tools, got: {tool_names}"
+            )
+        else:
+            # Fallback: at least verify create_mcp_app didn't silently fail
+            assert app is not None
 
     def test_idempotent_calls(self):
         """Multiple calls should each return independent instances."""