fix: Resolve critical MCP protocol compliance issues#8
fix: Resolve critical MCP protocol compliance issues#8robotlearning123 merged 2 commits intomainfrom
Conversation
Addresses 4 critical compliance issues identified in comprehensive MCP review: 1. **Protocol Version Misalignment (Critical #1)** - Added MCP_PROTOCOL_VERSION constant set to "2024-11-05" - Updated server documentation to reflect proper protocol version 2. **Server Initialization Non-Compliance (Critical #2)** - Enhanced server initialization with proper capability configuration - Added comprehensive logging for initialization process - Included server instructions with protocol version information - Added error handling for server startup failures 3. **Tool Schema Validation Gaps (Critical #3)** - Updated all tool schemas to JSON Schema Draft 7 compliance - Added required $schema field: "http://json-schema.org/draft-07/schema#" - Added additionalProperties: false for strict validation - Enhanced schema with proper constraints (minimum values, etc.) 4. **Response Format Inconsistencies (Critical #4)** - Ensured consistent use of types.TextContent for all responses - Added debug logging for tool calls - Maintained JSON response format consistency **Testing**: Created comprehensive test suite (test_mcp_compliance_fixes.py) - ✅ All 5 test categories pass (Protocol Version, Tool Schemas, Server Init, Response Format, Schema Validation) - Validates proper JSON Schema Draft 7 compliance - Tests real schema validation with valid/invalid inputs - Confirms server startup and configuration **Compatibility**: Maintains backward compatibility while ensuring MCP 2024-11-05 compliance 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a debug script to probe MCP initialization and tool handling, tightens tool JSON Schemas and startup/capability handling in the MCP server with an explicit protocol version constant, enhances logging around initialization and tool calls, and introduces an integration-style test suite validating protocol version, schemas, initialization, and tool responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor OS as Process
participant Main as mcp_server.main()
participant Server as Server
participant Caps as get_capabilities()
participant Init as InitializationOptions
participant Stdio as stdio server
OS->>Main: start()
Main->>Main: log MCP_PROTOCOL_VERSION
Main->>Caps: server.get_capabilities(NotificationOptions, {})
Caps-->>Main: capabilities
Main->>Init: build InitializationOptions(capabilities, instructions)
Main->>Main: log capabilities, init complete
Main->>Stdio: start stdio server (try/except)
Stdio-->>Main: running
Note over Main,Stdio: Errors logged and re-raised on failure
sequenceDiagram
autonumber
participant Client
participant Server
participant Tools as handle_call_tool
Client->>Server: call_tool(name, arguments)
Server->>Server: log call and args
Server->>Tools: dispatch by name
Tools-->>Client: [TextContent("...")] (MCP-compliant response)
Note over Server,Client: Tool schemas enforce Draft-07 and additionalProperties: false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/mujoco_mcp/mcp_server.py (4)
151-158: Fix capability name: "reset" → "reset_simulation".The advertised capability should match the registered tool name to avoid client confusion.
- "capabilities": ["create_scene", "step_simulation", "get_state", "reset", "close_viewer"] + "capabilities": ["create_scene", "step_simulation", "get_state", "reset_simulation", "close_viewer"]
249-266: Validate required inputs at runtime for clearer errors.Even with schemas, defensive checks help when clients bypass validation. Return a helpful error if model_id is missing.
elif name == "step_simulation": model_id = arguments.get("model_id") steps = arguments.get("steps", 1) + if not model_id: + return [types.TextContent( + type="text", + text="❌ 'model_id' is required." + )] + if not viewer_client or not viewer_client.connected: return [types.TextContent( type="text", text="❌ No active viewer connection. Create a scene first." )]
314-337: Success message shows a failure icon.On success, "close_viewer" returns "❌ Viewer closed". Use a success indicator (or neutral text).
- return [types.TextContent( - type="text", - text="❌ Viewer closed" if response.get("success") - else f"❌ Failed to close: {response.get('error')}" - )] + return [types.TextContent( + type="text", + text="✅ Viewer closed" if response.get("success") + else f"❌ Failed to close: {response.get('error')}" + )]
167-171: Make ViewerClient calls non-blocking and serialize access.Verified: MuJoCoViewerClient.connect() and send_command() are synchronous (socket.connect/recv/send, time.sleep, subprocess) and are called from async code — they will block the asyncio event loop and race on the global viewer_client.
- Run blocking calls in a thread and await them (example diff).
@@ - if not viewer_client.connected: - success = viewer_client.connect() + if not viewer_client.connected: + success = await asyncio.to_thread(viewer_client.connect) @@ - response = viewer_client.send_command({ + response = await asyncio.to_thread(viewer_client.send_command, { "type": "load_model", "model_id": scene_type, "model_xml": scene_models[scene_type] })
- Guard all viewer_client mutations/requests with a module-level asyncio.Lock and use it around connect/send_command/disconnect:
viewer_lock = asyncio.Lock() ... async with viewer_lock: # connect / send_command / disconnect
- Apply the above changes across all call sites that use ViewerClient: src/mujoco_mcp/mcp_server.py (connect/send_command sites around lines 167-171, 231-235, 277-281, 303-307, 323-327), src/mujoco_mcp/visualization_tools.py (send_command/connect sites), src/mujoco_mcp/multi_robot_coordinator.py (send_command usage), src/mujoco_mcp/rl_integration.py (connect/send_command/close sites).
🧹 Nitpick comments (5)
src/mujoco_mcp/mcp_server.py (2)
26-26: Avoid module-level logging.basicConfig in library code.Initialize logging in main() to prevent surprising global logging changes when this module is imported.
-logging.basicConfig(level=logging.INFO) logger = logging.getLogger("mujoco-mcp")Add inside main(), before first log call:
async def main(): """Main entry point for MCP server""" + logging.basicConfig(level=logging.INFO) logger.info(f"Starting MuJoCo MCP Server v{__version__}")Also applies to: 352-356
139-349: Optional: return structured JSON for all tool responses.You standardized content as TextContent; consider JSON bodies for consistency (easier client parsing) beyond get_server_info.
Example pattern:
- return [types.TextContent(type="text", text="⏩ Stepped simulation 5 steps")] + return [types.TextContent( + type="text", + text=json.dumps({"ok": True, "tool": name, "model_id": model_id, "steps": steps}, indent=2) + )]debug_mcp_version.py (1)
24-28: Make debug response JSON for parity with server style.Returning JSON aids quick client-side sanity checks.
-async def handle_call_tool(name: str, arguments: Dict[str, Any]) -> List[types.TextContent]: - """Handle tool calls""" - return [types.TextContent(type="text", text="Test response")] +async def handle_call_tool(name: str, arguments: Dict[str, Any]) -> List[types.TextContent]: + """Handle tool calls""" + payload = {"ok": True, "tool": name, "arguments": arguments} + return [types.TextContent(type="text", text=json.dumps(payload, indent=2))]test_mcp_compliance_fixes.py (2)
33-61: Permit both http and https Draft-07 URLs to future-proof the test.Some validators/sources use https; loosen the check without weakening intent.
- assert schema["$schema"] == "http://json-schema.org/draft-07/schema#", \ - f"Tool '{tool.name}' has incorrect $schema" + assert str(schema["$schema"]).rstrip("#").endswith("/draft-07/schema"), \ + f"Tool '{tool.name}' has incorrect $schema: {schema['$schema']}"
83-105: Nice: response format test exercises real tool call.You might also assert the presence of "description" to match current server_info shape.
- assert "name" in data, "Server info should include name" - assert "version" in data, "Server info should include version" - assert "status" in data, "Server info should include status" + for k in ("name", "version", "status", "description"): + assert k in data, f"Server info should include {k}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
debug_mcp_version.py(1 hunks)src/mujoco_mcp/mcp_server.py(6 hunks)test_mcp_compliance_fixes.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test_mcp_compliance_fixes.py (2)
debug_mcp_version.py (2)
handle_list_tools(20-22)handle_call_tool(25-27)src/mujoco_mcp/mcp_server.py (3)
handle_list_tools(36-137)handle_call_tool(140-350)main(352-386)
src/mujoco_mcp/mcp_server.py (1)
debug_mcp_version.py (1)
handle_call_tool(25-27)
debug_mcp_version.py (1)
src/mujoco_mcp/mcp_server.py (2)
handle_list_tools(36-137)handle_call_tool(140-350)
🔇 Additional comments (4)
src/mujoco_mcp/mcp_server.py (2)
22-24: Protocol version alignment looks correct.Defining MCP_PROTOCOL_VERSION and logging it on startup is clear and matches the stated compliance target.
Also applies to: 355-356
144-146: Good: tool-call observability.The debug log and clarified docstring make tracing calls straightforward.
Also applies to: 141-141
debug_mcp_version.py (1)
29-41: LGTM: capabilities constructed via server.get_capabilities().Matches the main server’s init path and helps validate MCP options in isolation.
test_mcp_compliance_fixes.py (1)
23-31: LGTM: protocol version test is clear and strict.This guards regressions on the spec version.
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "type": "object", | ||
| "properties": {}, | ||
| "required": [] | ||
| "required": [], | ||
| "additionalProperties": False | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema hardening is solid; fix enum/implementation mismatch for "arm".
All tool schemas now declare Draft-07 and disallow additionalProperties—nice. However, "arm" is allowed in create_scene enum (Line 60) but not implemented in scene_models (Lines 176-223). Either implement "arm" or remove it from the enum to avoid user-facing errors.
Apply one of the following diffs.
Option A — implement "arm":
@@
scene_models = {
@@
"cart_pole": """
@@
</mujoco>
"""
+ ,
+ "arm": """
+ <mujoco>
+ <worldbody>
+ <body name="base" pos="0 0 0.1">
+ <geom name="base" type="box" size="0.1 0.1 0.05" rgba="0.6 0.6 0.6 1"/>
+ <body name="link1" pos="0 0 0.1">
+ <joint name="shoulder" type="hinge" axis="0 0 1"/>
+ <geom name="link1" type="capsule" size="0.02 0.3" rgba="0.2 0.6 0.9 1"/>
+ <body name="link2" pos="0 0 0.3">
+ <joint name="elbow" type="hinge" axis="0 1 0"/>
+ <geom name="link2" type="capsule" size="0.02 0.25" rgba="0.9 0.6 0.2 1"/>
+ </body>
+ </body>
+ </body>
+ </worldbody>
+ </mujoco>
+ """
}Option B — remove "arm" from the enum:
- "enum": ["pendulum", "double_pendulum", "cart_pole", "arm"]
+ "enum": ["pendulum", "double_pendulum", "cart_pole"]Also applies to: 54-65, 71-87, 93-103, 109-119, 125-135, 60-60, 176-223
1 participant
Summary
Fixes 4 critical MCP protocol compliance issues identified in comprehensive review against official MCP specification (2024-11-05).
Critical Issues Resolved
🔧 Critical #1: Protocol Version Misalignment
MCP_PROTOCOL_VERSION = "2024-11-05"constant and updated documentation🚀 Critical #2: Server Initialization Non-Compliance
📋 Critical #3: Tool Schema Validation Gaps
$schema: "http://json-schema.org/draft-07/schema#"additionalProperties: falsefor strict validation📤 Critical #4: Response Format Inconsistencies
types.TextContentwith consistent JSON formattingValidation & Testing
✅ Comprehensive Test Suite (
test_mcp_compliance_fixes.py):✅ Server Configuration Check:
Technical Details
JSON Schema Compliance Example:
{ "$schema": "http://json-schema.org/draft-07/schema#", "type": "object", "properties": { "scene_type": { "type": "string", "enum": ["pendulum", "double_pendulum", "cart_pole", "arm"] } }, "required": ["scene_type"], "additionalProperties": false }Enhanced Server Initialization:
Impact
Files Changed
src/mujoco_mcp/mcp_server.py- Main compliance fixestest_mcp_compliance_fixes.py- Comprehensive test suitedebug_mcp_version.py- Debug utilitiesThis resolves the critical protocol compliance gaps and ensures proper MCP client compatibility.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests