[gh-392] Enable and disable mcp servers#398
Conversation
| for server_name, server_tools in tools: | ||
| self.io.tool_output(f" - {server_name}") | ||
| @property | ||
| def mcp_tools(self): |
There was a problem hiding this comment.
In order to reduce size of PR (and minimize breaking things), i'm creating a "wrapper" that keeps the original mcp_tools intent.
| finally: | ||
| from . import SwitchCoderSignal | ||
|
|
||
| raise SwitchCoderSignal( |
There was a problem hiding this comment.
Looks like raising signals is the only way to make things persistent when changing internal states? Otherwise when i would remove/load an mcp, all my changes disappeared when switching do a different mode
| except StopIteration: | ||
| return None | ||
|
|
||
| async def connect_all(self) -> None: |
There was a problem hiding this comment.
This is replaced by the classmethod, not really needed and wasn't used before either
| session = await server.connect() | ||
| tools_result = await session.list_tools() | ||
| self._server_tools[server.name] = tools_result.tools | ||
| tools = await experimental_mcp_client.load_mcp_tools(session=session, format="openai") |
There was a problem hiding this comment.
i dont have all the context so not sure what the intent is here, but perhaps we can update so we can just use list_tools vs using this library? Would need to be in another PR since that is a bigger change
There was a problem hiding this comment.
https://docs.litellm.ai/docs/mcp#2-list-and-call-mcp-tools
experimental_mcp_client formats the known list of MCP tools into a format that LLM prefers they be in from their own documentation. Based on a lot of your own work with the manager, you'd probably still want to store the name -> tool list mapping from experimental_mcp_client off of the manager instance since we do need to look up the tool names and what server they belong to for agent mode's operations
There was a problem hiding this comment.
you'd probably still want to store the name -> tool list mapping from experimental_mcp_client off of the manager instance since we do need to look up the tool names and what server they belong to for agent mode's operations
I updated self.mcp_tools in the Coder class to point to a backwards compatible version of what was originally there, and since AgentCoder inherits from Coder, it didn't break anything when i ran the agent. (I did do some small changes inside AgentCoder class but they are about handling the Local server)
I thought that would be enough? or maybe I'm misunderstanding 🤔
| for server, did_connect in results: | ||
| if not did_connect and server.name not in ["unnamed-server", "Local"]: | ||
| io.tool_warning( | ||
| f"MCP tool initialization failed after multiple retries: {server.name}" | ||
| ) | ||
|
|
||
| if verbose: | ||
| io.tool_output("MCP servers configured:") | ||
|
|
||
| for server, _ in results: | ||
| io.tool_output(f" - {server.name}") | ||
|
|
||
| for tool in mcp_manager.get_server_tools(server.name): | ||
| tool_name = tool.get("function", {}).get("name", "unknown") | ||
| tool_desc = tool.get("function", {}).get("description", "").split("\n")[0] | ||
| io.tool_output(f" - {tool_name}: {tool_desc}") |
There was a problem hiding this comment.
Don't like that im handling IO operations in a classmethod but it shouold be fine? this is only ever needed at initialization so letting user know details should be okay?
| io.confirm_ask.assert_called_once_with("Edit the files?", allow_tweak=False) | ||
| mock_create.assert_not_called() | ||
|
|
||
| @patch("cecli.coders.base_coder.experimental_mcp_client") |
There was a problem hiding this comment.
Superceded by the tests for the manager class
| | **/read-only** | Add files to the chat that are for reference only, or turn added files to read-only | | ||
| | **/reasoning-effort** | Set the reasoning effort level (values: number or low/medium/high depending on model) | | ||
| | **/report** | Report a problem by opening a GitHub Issue | | ||
| | **/remove-mcp** | Remove a MCP server by name | |
There was a problem hiding this comment.
Running documentation scripts were broken. Needs to be updated in a follow up PR.
2 participants
Two new commands are introcuded:
/load-mcp- Load a disabled mcp/remove-mcp- Remove an active mcpCloses #332 and #392