feat(wip): added mcp client tool

[ErikBjare]

Attempting to make a MCP client tool, as a first step to proper MCP integration/support.

Having lots of trouble with the async interface of the MCP SDK.

TODO:

• Wrap the async MCP SDK interface to work well from non-async code
• Make the MCP interface itself less of a tool in gptme and more of a "tool loader" which creates ToolSpec's from MCP servers. So that we can load tools from MCP, instead of loading a MCP tool.
• When done, make PR to get added to the MCP docs of supported clients

Related:

• Investigate Anthropic's MCP (Model Context Protocol) #286
• feat: add Model Context Protocol (MCP) support #287 (gptme as MCP server)

---

Important

Introduces MCP client tool for server interaction with command handling and testing support.

• New Feature:
  • Added MCPTool and MCPCommandHandler in gptme/tools/mcp.py for interacting with MCP servers.
  • Supports commands: connect, disconnect, tools, resources, prompts, call, read, prompt.
• Configuration:
  • Updated .pre-commit-config.yaml to use a local mypy hook.
  • Updated Makefile to exclude scripts/Kokoro-82M from source files.
  • Updated gptme.toml to include pyproject.toml and docs/index.rst.
  • Added mcp dependency in pyproject.toml.
• Testing:
  • Added test_mcp_server.py to create a test MCP server with a simple hello tool.
  • Added test_mcp_tool.py to test MCP tool commands and server interactions.

^{This description was created by for 76c62d2. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to e2e72e0 in 1 minute and 29 seconds

More details

• Looked at 691 lines of code in 5 files
• Skipped 1 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. gptme/tools/mcp.py:508

• Draft comment:
  Use await receive_stream.receive() instead of result_queue.get_nowait() to receive messages from an anyio stream.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment appears to be technically correct - when using anyio streams, it's generally better to use the async receive() method rather than get_nowait(). The code is mixing async and sync styles by using get_nowait(). However, this section of code is inside a synchronous generator function (execute()) and is running after the async work is complete, so get_nowait() is actually appropriate here since we can't use await.
  The comment correctly identifies a potential issue with mixing async/sync styles, but fails to recognize that this code is in a synchronous context where await cannot be used.
  While using receive() would be better in an async context, get_nowait() is actually the correct choice here since we're in a synchronous generator function after all async work is complete.
  The comment should be deleted because it suggests an incorrect change - we cannot use await receive() in this synchronous context, and get_nowait() is actually the appropriate choice here.

2. gptme/tools/mcp.py:496

• Draft comment:
  Avoid creating a new event loop with asyncio.new_event_loop(). Use the existing event loop with asyncio.get_event_loop() instead.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Creating new event loops is actually the correct approach here since this is a synchronous function (execute()) that needs to run async code. Using get_event_loop() could cause issues if there's already an event loop running. The code properly creates, uses, and closes the loops.
  The comment might be thinking about the general best practice of reusing event loops, but doesn't consider the specific context where new loops are needed.
  While reusing event loops is generally good practice, this is a special case where new loops are required to bridge sync and async code safely.
  The comment should be deleted as it suggests a change that would make the code less reliable. Creating new event loops is the correct approach in this context.

3. gptme/tools/mcp.py:519

• Draft comment:
  Avoid creating a new event loop for cleanup. Use the existing event loop with asyncio.get_event_loop() instead.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment points out a real issue - creating new event loops can be problematic, especially when there might be an existing one. However, this code is in a synchronous context (the execute() method is not async) and is handling async cleanup. In this case, creating a new event loop is actually a valid approach since we need to run async code from a sync context.
  The comment might be right that reusing event loops is generally better practice, but it doesn't account for the sync-to-async context transition happening here.
  Given the synchronous context of execute(), creating a new event loop is a reasonable approach to handle the async cleanup. Using get_event_loop() could be problematic if there isn't an active event loop.
  The comment should be deleted because the current approach of creating a new event loop is appropriate for this specific sync-to-async context.

4. gptme/tools/mcp.py:34

• Draft comment:
  Using ast.literal_eval can be a security risk if the input is not trusted. Consider using a safer alternative for parsing.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The input is coming from MCP command arguments, which are presumably from a trusted source (the user running the commands). ast.literal_eval() is actually considered a safe alternative to eval() as it only parses Python literals, not arbitrary code. The function already tries JSON first, which is even safer. The comment seems to misunderstand the security implications.
  I could be wrong about the trust level of MCP command inputs. There could be injection vectors I haven't considered.
  ast.literal_eval() is specifically designed as the safe way to parse Python literals, and this is a fallback after JSON. The input scope is limited to command arguments from the user running the tool.
  The comment should be deleted as it raises a security concern about ast.literal_eval() that is not valid - this is already the safe approach for parsing Python literals.

Workflow ID: wflow_3JskRwAlKDBllSof

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Use await send_stream.send instead of result_queue.put for sending messages in an anyio stream. This issue is also present at lines 366, 389, 399, 425, 439, 456, and 477.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 17.02128% with 234 lines in your changes missing coverage. Please review.

Project coverage is 65.21%. Comparing base (8c536f1) to head (76c62d2).
Report is 2 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
gptme/tools/mcp.py	17.02%	234 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   67.42%   65.21%   -2.22%     
==========================================
  Files          70       71       +1     
  Lines        6131     6413     +282     
==========================================
+ Hits         4134     4182      +48     
- Misses       1997     2231     +234     

Flag	Coverage Δ
anthropic/claude-3-haiku-20240307	63.99% <17.02%> (-2.18%)	⬇️
openai/gpt-4o-mini	63.49% <17.02%> (-2.14%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 76c62d2 in 56 seconds

More details

• Looked at 517 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. gptme/tools/mcp.py:133

• Draft comment:
  Using AnyHttpUrl directly for URL conversion is incorrect. Consider using a standard library method for URL validation or conversion.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  AnyHttpUrl is a valid Pydantic type specifically designed for URL validation. It's part of Pydantic's validation system and is commonly used. The code is using it correctly to validate URLs before passing them to the MCP client. The comment seems incorrect - using AnyHttpUrl is a perfectly valid approach.
  I could be wrong about the MCP client's requirements - maybe it has specific URL format needs that AnyHttpUrl doesn't handle correctly.
  Looking at the code, the MCP client itself uses AnyUrl type (imported from mcp.types), and AnyHttpUrl is compatible with this as it's a more specific version of URL validation. The code converts to AnyUrl if needed on line 428.
  The comment should be deleted. Using AnyHttpUrl is a valid approach for URL validation, and the code handles conversion to the MCP client's AnyUrl type correctly.

2. gptme/tools/mcp.py:179

• Draft comment:
  The lambda functions in the handlers dictionary for tools, resources, and prompts are not awaited. Consider using async def instead of lambda for these handlers.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment appears to be incorrect. While the lambda functions do wrap async functions, they are properly awaited when used. On line 188, the handler function (including the lambda) is awaited with 'await handlerscmd'. The lambda just serves as a way to partially apply the first argument to handle_list() - it's not causing any async/await issues.
  Could there be subtle timing or error handling issues introduced by wrapping async functions in lambdas? Could this pattern make debugging more difficult?
  While using lambdas to wrap async functions isn't always ideal for debugging, in this case it's a simple and valid pattern for partial application. The code correctly awaits the functions and handles errors appropriately.
  The comment should be deleted as it incorrectly identifies an issue. The async functions are properly awaited and the lambda pattern used is valid.

Workflow ID: wflow_SnDHe6gVe8erlLhQ

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.