Skip to content

feat(mcp): add mcp client and server#1300

Open
paul-nechifor wants to merge 1 commit intodevfrom
paul/feat/mcp-server
Open

feat(mcp): add mcp client and server#1300
paul-nechifor wants to merge 1 commit intodevfrom
paul/feat/mcp-server

Conversation

@paul-nechifor
Copy link
Contributor

No description provided.

@greptile-apps
Copy link

greptile-apps bot commented Feb 19, 2026

Greptile Summary

Refactored MCP implementation from TCP-based stdio bridge to direct HTTP-based FastAPI server/client architecture. The new approach simplifies the integration with Claude Code by removing the intermediate bridge layer and using HTTP JSON-RPC directly.

Major Changes:

  • Moved MCP implementation from dimos/protocol/mcp/ to dimos/agents/mcp/
  • Replaced TCP socket server with FastAPI HTTP server (McpServer) on port 9990
  • Created HTTP-based MCP client (McpClient) using httpx for JSON-RPC communication
  • Removed dependency on mcp>=1.0.0 package, implementing custom JSON-RPC handlers
  • Added comprehensive test coverage (integration and unit tests) with fixture-based testing
  • Updated unitree_go2_agentic_mcp blueprint to use new client/server modules

Issues Found:

  • Critical: test_mcp_client.py:62 returns ValueError instead of raising it, breaking error handling logic
  • Critical: test_mcp_server.py:105 assertion will fail due to mismatched error response format
  • Minor: Grammar error in mcp_client.py:150 ("You will updated")
  • Minor: Typo in README.md:25 ("youse")

Confidence Score: 3/5

  • PR has solid architecture but contains critical test bugs that will cause runtime failures
  • The refactoring from TCP to HTTP is well-designed and the core implementation is sound. However, there are two critical bugs in test code: ValueError being returned instead of raised (line 62 in test_mcp_client.py) and a failing assertion (line 105 in test_mcp_server.py). These will cause test failures and indicate the tests haven't been fully validated. The grammar/typo errors are minor but should be fixed.
  • Pay close attention to dimos/agents/mcp/test_mcp_client.py and dimos/agents/mcp/test_mcp_server.py for the critical logic errors

Important Files Changed

Filename Overview
dimos/agents/mcp/README.md comprehensive documentation for MCP server setup and usage, contains typo on line 25
dimos/agents/mcp/mcp_client.py MCP client implementation with HTTP-based JSON-RPC communication to MCP server, contains grammar error on line 150
dimos/agents/mcp/mcp_server.py FastAPI-based MCP server exposing robot skills as MCP tools, well-structured with proper error handling
dimos/agents/mcp/test_mcp_client.py integration tests for MCP client, contains critical bug on line 62 where ValueError is returned instead of raised
dimos/agents/mcp/test_mcp_server.py unit tests for MCP server request handling, contains assertion issue on line 105
dimos/robot/unitree/go2/blueprints/agentic/unitree_go2_agentic_mcp.py updated to use new MCP client/server architecture with autoconnect

Sequence Diagram

sequenceDiagram
    participant Claude as Claude Code
    participant Client as McpClient
    participant Server as McpServer
    participant Skills as Robot Skills

    Note over Claude,Skills: Initialization Phase
    Claude->>Server: HTTP POST /mcp (initialize)
    Server-->>Claude: Server capabilities & info
    Claude->>Server: HTTP POST /mcp (tools/list)
    Server->>Skills: get_skills()
    Skills-->>Server: List of SkillInfo
    Server-->>Claude: Available tools with schemas

    Note over Claude,Skills: Tool Execution Phase
    Claude->>Client: Human input message
    Client->>Client: Queue message
    Client->>Client: LangChain agent processes
    Client->>Server: HTTP POST /mcp (tools/call)
    Server->>Skills: RPC call to skill
    Skills-->>Server: Result or None
    Server-->>Client: JSON-RPC result
    Client->>Client: Append to history
    Client->>Claude: Publish agent message

    Note over Client,Skills: Image Handling
    Skills-->>Server: Result with agent_encode()
    Server-->>Client: Content with image data
    Client->>Client: Add UUID, append to history
    Client->>Claude: Tool response + image message
Loading

Last reviewed commit: 2fdf0cb

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

27 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile


## MCP Inspector

If you want to inspect the server manually, you can youse MCP Inspector.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "youse" should be "use"

Suggested change
If you want to inspect the server manually, you can youse MCP Inspector.
If you want to inspect the server manually, you can use MCP Inspector.

for item in content:
if item.get("type") != "text":
uuid_ = str(uuid.uuid4())
text += f"Tool call started with UUID: {uuid_}. You will updated with the result soon."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar error: "You will updated" should be "You will be updated"

Suggested change
text += f"Tool call started with UUID: {uuid_}. You will updated with the result soon."
text += f"Tool call started with UUID: {uuid_}. You will be updated with the result soon."

Comment on lines +62 to +64
return ValueError("Names must start with an uppercase letter.")
if not self._use_upper and name[0].isupper():
return ValueError("The names must only use lowercase letters.")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning ValueError instance instead of raising it - the function will return the error object as a string, not raise an exception

Suggested change
return ValueError("Names must start with an uppercase letter.")
if not self._use_upper and name[0].isupper():
return ValueError("The names must only use lowercase letters.")
if self._use_upper and not name[0].isupper():
raise ValueError("Names must start with an uppercase letter.")
if not self._use_upper and name[0].isupper():
raise ValueError("The names must only use lowercase letters.")

rpc_calls,
)
)
assert "not found" in response["result"]["content"][0]["text"].lower()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertion expects "not found" in error response but _handle_tools_call returns "Tool not found: {name}" (capital T), so the assertion will fail after lowercasing. Current implementation returns an error object with code -32602, not a text result.

@spomichter
Copy link
Contributor

@paul-nechifor should we just fork openclaw? Theyve build a lot of robust MCP and hosted gateway infra, runs on all architectures, etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments