-
Notifications
You must be signed in to change notification settings - Fork 116
[RFC 003] [1/n] Implement basic local MCP server & add MCP client #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,329 @@ | |||
| # RFC-003 Implementation Journal | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was written by Claude. Keeping this here while I implement, but I will remove it when the stack lands.
|
|
||
|
|
||
| class EchoEnv(HTTPEnvClient[EchoAction, EchoObservation]): | ||
| class EchoEnv(HTTPEnvClient[CallToolAction, Observation]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I followed the HTTPEnvClient convention, but to me this is the weakest part of OpenEnv as we require that people write boilerplate that we can generate for them.
I will tackle this when we go into supporting ToolCall and CodeAct at the same time, as that is a natural fit for generating this boilerplate on the fly.
| Raises: | ||
| ValueError: If MCP client not configured or action type invalid | ||
| """ | ||
| from .types import CallToolObservation, ListToolsObservation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude this, but me no like. Will fix.
| observation = await loop.run_in_executor( | ||
| self._executor, self.env.step, action | ||
| ) | ||
| # Handle MCP actions asynchronously (don't use thread pool for async operations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pankit-eng this is where I need your eyes the most. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the core MCP (Model Context Protocol) infrastructure for OpenEnv as the first phase of RFC-003. It introduces a new MCPEnvironment base class that allows environments to expose tools via the MCP protocol, and converts the echo_env as a reference implementation. The changes enable both gym-style orchestration through HTTP endpoints and direct MCP JSON-RPC access for production agents.
Key Changes:
- Added MCP action types (
ListToolsAction,CallToolAction) and corresponding observations to the core type system - Implemented
MCPEnvironmentbase class that handles MCP client/server communication boilerplate - Extended HTTP server with
/mcpJSON-RPC endpoint for direct tool access alongside existing/stependpoint - Refactored echo_env to use MCP pattern, replacing custom
EchoAction/EchoObservationwith MCP tools
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/env_server/types.py |
Added MCP action and observation types (ListToolsAction, CallToolAction, ListToolsObservation, CallToolObservation) |
src/core/env_server/mcp_environment.py |
New base class providing MCP infrastructure for environments |
src/core/env_server/http_server.py |
Added /mcp JSON-RPC endpoint and MCP action routing in /step handler |
src/core/env_server/__init__.py |
Exported new MCP types and MCPEnvironment class |
src/core/pyproject.toml |
Added mcp and fastmcp dependencies |
src/envs/echo_env/server/mcp_server.py |
New MCP tool definitions for echo environment |
src/envs/echo_env/server/echo_environment.py |
Refactored to extend MCPEnvironment instead of custom implementation |
src/envs/echo_env/server/app.py |
Updated to use base Action/Observation types for MCP support |
src/envs/echo_env/models.py |
Removed custom EchoAction and EchoObservation (deprecated in favor of MCP) |
src/envs/echo_env/client.py |
Updated to handle MCP actions and parse MCP observations |
src/envs/echo_env/__init__.py |
Removed exports of deprecated custom action/observation types |
tests/core/mcp/test_mcp.py |
New test file for MCP client/server integration |
examples/test_mcp_integration.py |
Integration test script demonstrating MCP usage |
examples/echo_mcp_demo.py |
Demo script showing MCP tool interaction |
rfcs/RFC-003-implementation-journal.md |
Implementation journal documenting decisions and progress |
Comments suppressed due to low confidence (1)
src/core/env_server/http_server.py:17
- Import of 'json' is not used.
import json
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/core/env_server/http_server.py
Outdated
| Returns: | ||
| JSON-RPC 2.0 response | ||
| """ | ||
| if self.env.mcp_client is None: |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /mcp endpoint assumes self.env.mcp_client exists, but not all Environment instances will have this attribute. This will raise an AttributeError for non-MCP environments. Add a hasattr(self.env, 'mcp_client') check or check if the env is an instance of MCPEnvironment first.
| if self.env.mcp_client is None: | |
| if not hasattr(self.env, "mcp_client") or self.env.mcp_client is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pankit-eng this is good catch by copilot. Do you think we should just add that line?
src/envs/echo_env/client.py
Outdated
| >>> tools = client.list_tools() | ||
| >>> print(tools) # [{"name": "echo_message", ...}] | ||
| >>> | ||
| >>> # Call echo_message tool | ||
| >>> result = client.echo_message("Hello!") | ||
| >>> print(result["echoed_message"]) # "Hello!" |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring mentions list_tools() and echo_message() methods that are not implemented in the EchoEnv class. Either implement these convenience methods or update the documentation to reflect the actual API (using step() with ListToolsAction and CallToolAction).
examples/echo_mcp_demo.py
Outdated
|
|
||
| # List available tools | ||
| print("2. Listing available tools...") | ||
| tools = client.list_tools() |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list_tools() method is not implemented in the EchoEnv client class (see client.py). This call will fail with an AttributeError. Either implement the method in the client or use the step() method with a ListToolsAction.
examples/echo_mcp_demo.py
Outdated
|
|
||
| # Call echo_message tool using convenience method | ||
| print("3. Calling echo_message tool...") | ||
| result = client.echo_message("Hello from MCP!") |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The echo_message() method is not implemented in the EchoEnv client class (see client.py). This call will fail with an AttributeError. Either implement the method in the client or use the step() method with a CallToolAction.
src/core/env_server/http_server.py
Outdated
| "id": None, | ||
| } | ||
|
|
||
| body = await request.json() |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /mcp endpoint lacks error handling for malformed JSON. If request.json() fails (e.g., invalid JSON in request body), it will raise an exception. Consider wrapping the await request.json() call in a try-except block and returning a JSON-RPC error with code -32700 ("Parse error") for invalid JSON.
src/core/env_server/http_server.py
Outdated
| return CallToolAction( | ||
| tool_name=action_data["tool_name"], |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing action_data["tool_name"] will raise a KeyError if the tool_name field is missing from the request. Use action_data.get("tool_name") with an appropriate error message if it's None, or validate that the field exists before accessing it.
| return CallToolAction( | |
| tool_name=action_data["tool_name"], | |
| 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=tool_name, |
|
|
||
| try: | ||
| # Reuse MCP client from environment (avoids creating duplicate client) | ||
| async with self.env.mcp_client: |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MCP client is being used within an async with block in the /mcp endpoint (line 168) and also in _handle_mcp_action (mcp_environment.py line 133). This could lead to issues if the client doesn't support being entered multiple times concurrently. Consider whether the client should be managed at the environment level or if each endpoint should manage its own context.
|
|
||
| self.mcp_server = mcp_server | ||
| self.mcp_client = Client(mcp_server) | ||
| super().__init__(mcp_client=self.mcp_client) |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Environment.__init__ base class doesn't accept an mcp_client parameter. This call will fail with a TypeError. The base class __init__ signature is def __init__(self, transform: Transform | None = None) (see interfaces.py line 95).
Either:
- Remove the
mcp_client=self.mcp_clientargument here, or - Update the
Environment.__init__signature to acceptmcp_clientas an optional parameter
| super().__init__(mcp_client=self.mcp_client) | |
| super().__init__() |
| from core.env_server.types import ( | ||
| CallToolAction, | ||
| CallToolObservation, | ||
| ListToolsAction, | ||
| ListToolsObservation, | ||
| Observation, | ||
| State, | ||
| ) |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'ListToolsAction' is not used.
| from openenv_core.env_server.types import ( | ||
| CallToolAction, | ||
| CallToolObservation, | ||
| ListToolsAction, | ||
| ListToolsObservation, | ||
| Observation, | ||
| State, | ||
| ) |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'ListToolsAction' is not used.
|
@copilot I accept all your suggestions. Can you implement these changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if we can rename the file name and not use types.py. It starts conflicting usually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point
|
|
||
| # Reset the environment | ||
| print("1. Resetting environment...") | ||
| result = client.reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is resetting a pre-req to list_tools call? Or in other words, does reset() semantically have any impact on the list_tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that list_tools() should be removed since instead we are just going to allow env.step(ListToolsAction()). Apart from that, hmmm I think that we may still want you to call .reset() first to initialize the env, in case some of your tools require state (e.g. a driving sim)
| Contains the list of available tools with their schemas. | ||
| """ | ||
|
|
||
| tools: List[Dict[str, Any]] = field(default_factory=list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darktex - can we use a strongly types schema(openAPI or similar) like how MCP list/tools does.
Each ToolSpec is defined as:
{
name: string; // Unique identifier for the tool
description?: string; // Human-readable description
inputSchema: { // JSON Schema for the tool's parameters
type: "object",
properties: { ... } // Tool-specific parameters
}
}
And this API returns a list of the above typed object:
[ToolSpec]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout
| metadata: Dict[str, Any] = field(default_factory=dict) | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly recommend that we create typed objects for tools and use that in client as well as the server:
@dataclass
class Tool:
name: str
description: str
input_schema: Dict[str, Any]
output_schema: Dict[str, Any]
| Contains the result of calling a tool, or an error if the call failed. | ||
| """ | ||
|
|
||
| result: Optional[Any] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fields must be required for a CallToolObservation. Lets remove optional
We should be specific about error here: error coming from the tool execution needs to be part of the result object and not the error field. In other words, we should assume that tool will bundle the error in the Result and we will return the same as is in the result field.
For any other errors including but not limited to - invalid args, transport level issue, etc.., we can have an error field as we have here. Error field could also be more expanded:
class Error:
error_type: enum
error_str: description
....
| from fastmcp import Client | ||
|
|
||
| self.mcp_server = mcp_server | ||
| self.mcp_client = Client(mcp_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine that we will need to support multiple mcp servers and hence, multiple mcp clients in one environment. Are you planning on adding the routing support in a separate PR for the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pankit-eng Correct! For now, let's do a single, local MCP server
* Initial plan * Address code review feedback from copilot-pull-request-reviewer Co-authored-by: Darktex <[email protected]> * Address secondary code review comments Co-authored-by: Darktex <[email protected]> * Remove list_tools and echo_message convenience methods per review feedback Co-authored-by: Darktex <[email protected]> * Address code review feedback: add import examples and use isinstance Co-authored-by: Darktex <[email protected]> * Remove unnecessary isinstance check in echo_mcp_demo.py Co-authored-by: Darktex <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Darktex <[email protected]>
|
@copilot Can you send me a PR implementing Pankit's comments? |
As part of implementing RFC 003, I'm adding support for MCP.
At Meta, we typically work with Stacked PRs as a way to stay sane and allow for easier review.
For this reason, this is just the first PR in the stack, but it is already adding the MCP functionality.
Implementation Phases: