diff --git a/openhands/core/config/mcp_config.py b/openhands/core/config/mcp_config.py index 44a2aaba21..344727efda 100644 --- a/openhands/core/config/mcp_config.py +++ b/openhands/core/config/mcp_config.py @@ -2,6 +2,7 @@ import os from urllib.parse import urlparse from pydantic import BaseModel, Field, ValidationError, model_validator + from openhands.utils.import_utils import get_impl @@ -32,6 +33,22 @@ class MCPStdioServerConfig(BaseModel): args: list[str] = Field(default_factory=list) env: dict[str, str] = Field(default_factory=dict) + def __eq__(self, other): + """Override equality operator to compare server configurations. + + Two server configurations are considered equal if they have the same + name, command, args, and env values. The order of args is important, + but the order of env variables is not. + """ + if not isinstance(other, MCPStdioServerConfig): + return False + return ( + self.name == other.name + and self.command == other.command + and self.args == other.args + and set(self.env.items()) == set(other.env.items()) + ) + class MCPConfig(BaseModel): """Configuration for MCP (Message Control Protocol) settings. @@ -124,10 +141,11 @@ class MCPConfig(BaseModel): return mcp_mapping - class OpenHandsMCPConfig: @staticmethod - def create_default_mcp_server_config(host: str, user_id: str | None = None) -> MCPSSEServerConfig | None: + def create_default_mcp_server_config( + host: str, user_id: str | None = None + ) -> MCPSSEServerConfig | None: """ Create a default MCP server configuration. @@ -141,12 +159,9 @@ class OpenHandsMCPConfig: return MCPSSEServerConfig(url=f'http://{host}/mcp/sse', api_key=None) - openhands_mcp_config_cls = os.environ.get( 'OPENHANDS_MCP_CONFIG_CLS', 'openhands.core.config.mcp_config.OpenHandsMCPConfig', ) -OpenHandsMCPConfigImpl = get_impl( - OpenHandsMCPConfig, openhands_mcp_config_cls -) \ No newline at end of file +OpenHandsMCPConfigImpl = get_impl(OpenHandsMCPConfig, openhands_mcp_config_cls) diff --git a/openhands/runtime/impl/action_execution/action_execution_client.py b/openhands/runtime/impl/action_execution/action_execution_client.py index 7ddcee7d0c..4af70d4623 100644 --- a/openhands/runtime/impl/action_execution/action_execution_client.py +++ b/openhands/runtime/impl/action_execution/action_execution_client.py @@ -10,7 +10,11 @@ import httpx from tenacity import retry, retry_if_exception, stop_after_attempt, wait_exponential from openhands.core.config import AppConfig -from openhands.core.config.mcp_config import MCPConfig, MCPStdioServerConfig, MCPSSEServerConfig +from openhands.core.config.mcp_config import ( + MCPConfig, + MCPSSEServerConfig, + MCPStdioServerConfig, +) from openhands.core.exceptions import ( AgentRuntimeTimeoutError, ) @@ -77,6 +81,7 @@ class ActionExecutionClient(Runtime): self._runtime_initialized: bool = False self._runtime_closed: bool = False self._vscode_token: str | None = None # initial dummy value + self._last_updated_mcp_stdio_servers: list[MCPStdioServerConfig] = [] super().__init__( config, event_stream, @@ -357,43 +362,77 @@ class ActionExecutionClient(Runtime): # Add the runtime as another MCP server updated_mcp_config = self.config.mcp.model_copy() - # Send a request to the action execution server to updated MCP config - stdio_tools = [ - server.model_dump(mode='json') - for server in updated_mcp_config.stdio_servers - ] + # Get current stdio servers + current_stdio_servers: list[MCPStdioServerConfig] = list( + updated_mcp_config.stdio_servers + ) if extra_stdio_servers: - stdio_tools.extend( - [server.model_dump(mode='json') for server in extra_stdio_servers] - ) + current_stdio_servers.extend(extra_stdio_servers) - if len(stdio_tools) > 0: - self.log('debug', f'Updating MCP server to: {stdio_tools}') + # Check if there are any new servers using the __eq__ operator + new_servers = [ + server + for server in current_stdio_servers + if server not in self._last_updated_mcp_stdio_servers + ] + + self.log( + 'debug', + f'adding {len(new_servers)} new stdio servers to MCP config: {new_servers}', + ) + + # Only send update request if there are new servers + if new_servers: + # Use a union of current servers and last updated servers for the update + # This ensures we don't lose any servers that might be missing from either list + combined_servers = current_stdio_servers.copy() + for server in self._last_updated_mcp_stdio_servers: + if server not in combined_servers: + combined_servers.append(server) + + stdio_tools = [ + server.model_dump(mode='json') for server in combined_servers + ] + stdio_tools.sort(key=lambda x: x.get('name', '')) # Sort by server name + + self.log( + 'debug', + f'Updating MCP server with {len(new_servers)} new stdio servers (total: {len(combined_servers)})', + ) response = self._send_action_server_request( 'POST', f'{self.action_execution_server_url}/update_mcp_server', json=stdio_tools, timeout=10, ) + if response.status_code != 200: self.log('warning', f'Failed to update MCP server: {response.text}') - - # No API key by default. Child runtime can override this when appropriate - updated_mcp_config.sse_servers.append( - MCPSSEServerConfig( - url=self.action_execution_server_url.rstrip('/') + '/sse', - api_key=None, + else: + # Update our cached list with combined servers after successful update + self._last_updated_mcp_stdio_servers = combined_servers.copy() + self.log( + 'debug', + f'Successfully updated MCP stdio servers, now tracking {len(combined_servers)} servers', ) - ) self.log( 'info', f'Updated MCP config: {updated_mcp_config.sse_servers}', ) else: - self.log( - 'debug', - 'MCP servers inside runtime is not updated since no stdio servers are provided', + self.log('debug', 'No new stdio servers to update') + + + if len(self._last_updated_mcp_stdio_servers) > 0: + # We should always include the runtime as an MCP server whenever there's > 0 stdio servers + updated_mcp_config.sse_servers.append( + MCPSSEServerConfig( + url=self.action_execution_server_url.rstrip('/') + '/sse', + # No API key by default. Child runtime can override this when appropriate + api_key=None, + ) ) + return updated_mcp_config async def call_tool_mcp(self, action: MCPAction) -> Observation: diff --git a/tests/runtime/test_mcp_action.py b/tests/runtime/test_mcp_action.py index ad4d88657e..da7b1123bc 100644 --- a/tests/runtime/test_mcp_action.py +++ b/tests/runtime/test_mcp_action.py @@ -264,3 +264,86 @@ async def test_both_stdio_and_sse_mcp( if runtime: runtime.close() # SSE Docker container cleanup is handled by the sse_mcp_docker_server fixture + + +@pytest.mark.asyncio +async def test_microagent_and_one_stdio_mcp_in_config( + temp_dir, runtime_cls, run_as_openhands +): + runtime = None + try: + filesystem_config = MCPStdioServerConfig( + name='filesystem', + command='npx', + args=[ + '@modelcontextprotocol/server-filesystem', + '/', + ], + ) + override_mcp_config = MCPConfig(stdio_servers=[filesystem_config]) + runtime, config = _load_runtime( + temp_dir, + runtime_cls, + run_as_openhands, + override_mcp_config=override_mcp_config, + ) + + # NOTE: this simulate the case where the microagent adds a new stdio server to the runtime + # but that stdio server is not in the initial config + # Actual invocation of the microagent involves `add_mcp_tools_to_agent` + # which will call `get_updated_mcp_config` with the stdio server from microagent's config + fetch_config = MCPStdioServerConfig( + name='fetch', command='uvx', args=['mcp-server-fetch'] + ) + updated_config = runtime.get_updated_mcp_config([fetch_config]) + logger.info(f'updated_config: {updated_config}') + + # ======= Test the stdio server in the config ======= + mcp_action_sse = MCPAction(name='list_directory', arguments={'path': '/'}) + obs_sse = await runtime.call_tool_mcp(mcp_action_sse) + logger.info(obs_sse, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs_sse, MCPObservation), ( + 'The observation should be a MCPObservation.' + ) + assert '[FILE] .dockerenv' in obs_sse.content + + # ======= Test the stdio server added by the microagent ======= + # Test browser server + action_cmd_http = CmdRunAction( + command='python3 -m http.server 8000 > server.log 2>&1 &' + ) + logger.info(action_cmd_http, extra={'msg_type': 'ACTION'}) + obs_http = runtime.run_action(action_cmd_http) + logger.info(obs_http, extra={'msg_type': 'OBSERVATION'}) + + assert isinstance(obs_http, CmdOutputObservation) + assert obs_http.exit_code == 0 + assert '[1]' in obs_http.content + + action_cmd_cat = CmdRunAction(command='sleep 3 && cat server.log') + logger.info(action_cmd_cat, extra={'msg_type': 'ACTION'}) + obs_cat = runtime.run_action(action_cmd_cat) + logger.info(obs_cat, extra={'msg_type': 'OBSERVATION'}) + assert obs_cat.exit_code == 0 + + mcp_action_fetch = MCPAction( + name='fetch', arguments={'url': 'http://localhost:8000'} + ) + obs_fetch = await runtime.call_tool_mcp(mcp_action_fetch) + logger.info(obs_fetch, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs_fetch, MCPObservation), ( + 'The observation should be a MCPObservation.' + ) + + result_json = json.loads(obs_fetch.content) + assert not result_json['isError'] + assert len(result_json['content']) == 1 + assert result_json['content'][0]['type'] == 'text' + assert ( + result_json['content'][0]['text'] + == 'Contents of http://localhost:8000/:\n---\n\n* \n\n---' + ) + finally: + if runtime: + runtime.close() + # SSE Docker container cleanup is handled by the sse_mcp_docker_server fixture diff --git a/tests/unit/test_mcp_config.py b/tests/unit/test_mcp_config.py index bac7a2e833..25ae65e333 100644 --- a/tests/unit/test_mcp_config.py +++ b/tests/unit/test_mcp_config.py @@ -192,3 +192,57 @@ def test_mcp_config_extra_fields_forbidden(): # Note: The nested models don't have 'extra': 'forbid' set, so they allow extra fields # We're only testing the main MCPConfig class here + + +def test_stdio_server_equality_with_different_env_order(): + """Test that MCPStdioServerConfig equality works with env in different order but respects arg order.""" + # Test 1: Same args, different env order + server1 = MCPStdioServerConfig( + name='test-server', + command='python', + args=['--verbose', '--debug', '--port=8080'], + env={'DEBUG': 'true', 'PORT': '8080'}, + ) + + server2 = MCPStdioServerConfig( + name='test-server', + command='python', + args=['--verbose', '--debug', '--port=8080'], # Same order + env={'PORT': '8080', 'DEBUG': 'true'}, # Different order + ) + + # Should be equal because env is compared as a set + assert server1 == server2 + + # Test 2: Different args order + server3 = MCPStdioServerConfig( + name='test-server', + command='python', + args=['--debug', '--port=8080', '--verbose'], # Different order + env={'DEBUG': 'true', 'PORT': '8080'}, + ) + + # Should NOT be equal because args order matters + assert server1 != server3 + + # Test 3: Different arg value + server4 = MCPStdioServerConfig( + name='test-server', + command='python', + args=['--verbose', '--debug', '--port=9090'], # Different port + env={'DEBUG': 'true', 'PORT': '8080'}, + ) + + # Should not be equal + assert server1 != server4 + + # Test 4: Different env value + server5 = MCPStdioServerConfig( + name='test-server', + command='python', + args=['--verbose', '--debug', '--port=8080'], + env={'DEBUG': 'true', 'PORT': '9090'}, # Different port + ) + + # Should not be equal + assert server1 != server5