Address code review feedback for MCP implementation (#226)

* Initial plan

* Address code review feedback from copilot-pull-request-reviewer

Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

* Address secondary code review comments

Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

* Remove list_tools and echo_message convenience methods per review feedback

Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

* Address code review feedback: add import examples and use isinstance

Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

* Remove unnecessary isinstance check in echo_mcp_demo.py

Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Darktex <890615+Darktex@users.noreply.github.com>

Author: Copilot

examples/echo_mcp_demo.py

@@ -4,10 +4,16 @@
 This example demonstrates:
 1. Connecting to echo_env server
 2. Listing available tools via MCP
-3. Calling tools using both step() API and direct tool methods
+3. Calling tools using the step() API
 """
 
 import asyncio
+
+try:
+    from core.env_server.types import CallToolAction, ListToolsAction
+except ImportError:
+    from openenv_core.env_server.types import CallToolAction, ListToolsAction
+
 from envs.echo_env import EchoEnv
 
 
@@ -23,17 +29,19 @@ async def main():
     result = client.reset()
     print(f"   Reset result: {result.observation.metadata}\n")
 
-    # List available tools
+    # List available tools using step API
     print("2. Listing available tools...")
-    tools = client.list_tools()
-    for tool in tools:
+    list_action = ListToolsAction()
+    list_result = client.step(list_action)
+    for tool in list_result.observation.tools:
         print(f"   - {tool['name']}: {tool['description']}")
     print()
 
-    # Call echo_message tool using convenience method
+    # Call echo_message tool using step API
     print("3. Calling echo_message tool...")
-    result = client.echo_message("Hello from MCP!")
-    print(f"   Result: {result}\n")
+    call_action = CallToolAction(tool_name="echo_message", parameters={"message": "Hello from MCP!"})
+    call_result = client.step(call_action)
+    print(f"   Result: {call_result.observation.result}\n")
 
     # Check environment state
     print("4. Checking environment state...")

rfcs/RFC-003-implementation-journal.md

@@ -96,7 +96,7 @@ This journal tracks the implementation of RFC-003: MCP (Model Context Protocol)
 
 #### Echo Env Conversion
 - [x] Deprecate `src/envs/echo_env/models.py` (EchoAction deprecated)
-- [x] Create `src/envs/echo_env/server/mcp_tools.py`
+- [x] Create `src/envs/echo_env/server/mcp_server.py`
 - [x] Define `echo_message` tool using FastMCP
 - [x] Update `src/envs/echo_env/server/echo_environment.py`
 - [x] Update `src/envs/echo_env/server/app.py` to initialize MCP
@@ -146,7 +146,7 @@ This journal tracks the implementation of RFC-003: MCP (Model Context Protocol)
    - Added `mcp_server` parameter to `HTTPEnvServer` and `create_fastapi_app()`
 
 5. **Echo Env Conversion**:
-   - Created `mcp_tools.py` with `echo_message` tool using FastMCP decorators
+   - Created `mcp_server.py` with `echo_message` tool using FastMCP decorators
    - Rewrote `EchoEnvironment` to use MCP client instead of custom actions
    - Updated `app.py` to initialize MCP server, client, and wire them together
    - Deprecated `EchoAction` and `EchoObservation` in `models.py` with warnings

src/core/env_server/http_server.py

@@ -14,7 +14,6 @@
 from __future__ import annotations
 
 import asyncio
-import json
 import os
 from concurrent.futures import ThreadPoolExecutor
 from dataclasses import asdict
@@ -23,6 +22,7 @@
 from fastapi import Body, FastAPI, Request
 
 from .interfaces import Environment
+from .mcp_environment import MCPEnvironment
 from .types import Action, CallToolAction, ListToolsAction, Observation
 
 
@@ -148,17 +148,28 @@ async def mcp_endpoint(request: Request) -> Dict[str, Any]:
             Returns:
                 JSON-RPC 2.0 response
             """
-            if self.env.mcp_client is None:
+            if not hasattr(self.env, "mcp_client") or self.env.mcp_client is None:
                 return {
                     "jsonrpc": "2.0",
                     "error": {
-                        "code": -32600,
+                        "code": -32603,
                         "message": "MCP server not configured for this environment",
                     },
                     "id": None,
                 }
 
-            body = await request.json()
+            try:
+                body = await request.json()
+            except (ValueError, TypeError):
+                return {
+                    "jsonrpc": "2.0",
+                    "error": {
+                        "code": -32700,
+                        "message": "Parse error: Invalid JSON",
+                    },
+                    "id": None,
+                }
+
             method = body.get("method")
             params = body.get("params", {})
             request_id = body.get("id")
@@ -240,8 +251,11 @@ def _deserialize_action(self, action_data: Dict[str, Any]) -> Action:
             return ListToolsAction()
 
         elif action_type == "CallToolAction":
+            tool_name = action_data.get("tool_name")
+            if tool_name is None:
+                raise ValueError("Missing required field 'tool_name' for CallToolAction")
             return CallToolAction(
-                tool_name=action_data["tool_name"],
+                tool_name=tool_name,
                 parameters=action_data.get("parameters", {}),
             )
 

src/core/env_server/mcp_environment.py

@@ -67,7 +67,7 @@ def __init__(self, mcp_server: Any):
 
         self.mcp_server = mcp_server
         self.mcp_client = Client(mcp_server)
-        super().__init__(mcp_client=self.mcp_client)
+        super().__init__()
 
     def reset(self) -> Observation:
         """

src/core/pyproject.toml

@@ -44,7 +44,6 @@ packages = [
     "openenv_core.containers",
     "openenv_core.containers.runtime",
     "openenv_core.env_server",
-    "openenv_core.tools",
-    "openenv_core.mcp"
+    "openenv_core.tools"
 ]
 package-dir = {"openenv_core" = "."}

src/envs/echo_env/client.py

@@ -11,14 +11,13 @@
 over HTTP using MCP actions.
 """
 
-from typing import Any, Dict, List
+from typing import Dict
 
 try:
     from core.client_types import StepResult
     from core.env_server.types import (
         CallToolAction,
         CallToolObservation,
-        ListToolsAction,
         ListToolsObservation,
         Observation,
         State,
@@ -29,7 +28,6 @@
     from openenv_core.env_server.types import (
         CallToolAction,
         CallToolObservation,
-        ListToolsAction,
         ListToolsObservation,
         Observation,
         State,
@@ -45,23 +43,23 @@ class EchoEnv(HTTPEnvClient[CallToolAction, Observation]):
     methods to interact with it using MCP actions.
 
     Example:
+        >>> from core.env_server.types import CallToolAction
         >>> # Connect to a running server
         >>> client = EchoEnv(base_url="http://localhost:8000")
         >>> result = client.reset()
         >>>
-        >>> # List available tools
-        >>> tools = client.list_tools()
-        >>> print(tools)  # [{"name": "echo_message", ...}]
-        >>>
-        >>> # Call echo_message tool
-        >>> result = client.echo_message("Hello!")
-        >>> print(result["echoed_message"])  # "Hello!"
+        >>> # Call echo_message tool using step API
+        >>> action = CallToolAction(tool_name="echo_message", parameters={"message": "Hello!"})
+        >>> result = client.step(action)
+        >>> print(result.observation.result)  # {"echoed_message": "Hello!"}
 
     Example with Docker:
+        >>> from core.env_server.types import CallToolAction
         >>> # Automatically start container and connect
         >>> client = EchoEnv.from_docker_image("echo-env:latest")
         >>> result = client.reset()
-        >>> result = client.echo_message("Test")
+        >>> action = CallToolAction(tool_name="echo_message", parameters={"message": "Test"})
+        >>> result = client.step(action)
     """
 
     def _step_payload(self, action: CallToolAction) -> Dict: