Merge pull request #8 from All-Hands-AI/xw/mcp-fork

Remove unnecessary commits and make MCPStdioConfig cleaner

Author: ducphamle2

.gitignore

@@ -233,4 +233,3 @@ containers/runtime/Dockerfile
 containers/runtime/project.tar.gz
 containers/runtime/code
 **/node_modules/
-yes/

config.template.toml

@@ -383,4 +383,4 @@ type = "noop"
 #################################### Eval ####################################
 # Configuration for the evaluation, please refer to the specific evaluation
 # plugin for the available options
-##############################################################################
+##############################################################################

docker-compose.yml

@@ -1,3 +1,4 @@
+
 services:
   openhands:
     build:
@@ -9,7 +10,6 @@ services:
       - SANDBOX_RUNTIME_CONTAINER_IMAGE=${SANDBOX_RUNTIME_CONTAINER_IMAGE:-docker.all-hands.dev/all-hands-ai/runtime:0.31-nikolaik}
       #- SANDBOX_USER_ID=${SANDBOX_USER_ID:-1234} # enable this only if you want a specific non-root sandbox user but you will have to manually adjust permissions of openhands-state for this user
       - WORKSPACE_MOUNT_PATH=${WORKSPACE_BASE:-$PWD/workspace}
-      - LOG_ALL_EVENTS=true
     ports:
       - "3000:3000"
     extra_hosts:

frontend/__tests__/routes/settings.test.tsx

@@ -61,17 +61,6 @@ describe("Settings Screen", () => {
   };
 
   it("should render", async () => {
-    // Mock the config to ensure LLM Settings are visible
-    getConfigSpy.mockResolvedValue({
-      APP_MODE: "oss",
-      GITHUB_CLIENT_ID: "test-github-client-id",
-      POSTHOG_CLIENT_KEY: "test-posthog-client-key",
-      FEATURE_FLAGS: {
-        ENABLE_BILLING: false,
-        HIDE_LLM_SETTINGS: false,
-      },
-    });
-
     renderSettingsScreen();
 
     await waitFor(() => {

frontend/src/i18n/declaration.ts

@@ -301,7 +301,6 @@ export enum I18nKey {
   STATUS$ERROR_LLM_SERVICE_UNAVAILABLE = "STATUS$ERROR_LLM_SERVICE_UNAVAILABLE",
   STATUS$ERROR_LLM_INTERNAL_SERVER_ERROR = "STATUS$ERROR_LLM_INTERNAL_SERVER_ERROR",
   STATUS$ERROR_LLM_OUT_OF_CREDITS = "STATUS$ERROR_LLM_OUT_OF_CREDITS",
-  STATUS$ERROR_LLM_CONTENT_POLICY_VIOLATION = "STATUS$ERROR_LLM_CONTENT_POLICY_VIOLATION",
   STATUS$ERROR_RUNTIME_DISCONNECTED = "STATUS$ERROR_RUNTIME_DISCONNECTED",
   STATUS$LLM_RETRY = "STATUS$LLM_RETRY",
   AGENT_ERROR$BAD_ACTION = "AGENT_ERROR$BAD_ACTION",

openhands/agenthub/codeact_agent/function_calling.py

@@ -42,7 +42,7 @@
 from openhands.events.event import FileEditSource, FileReadSource
 from openhands.events.tool import ToolCallMetadata
 from openhands.llm import LLM
-from openhands.mcp.mcp import BaseTool
+from openhands.mcp import MCPClientTool
 
 
 def combine_thought(action: Action, thought: str) -> Action:
@@ -73,7 +73,7 @@ def response_to_actions(response: ModelResponse) -> list[Action]:
         # Process each tool call to OpenHands action
         for i, tool_call in enumerate(assistant_msg.tool_calls):
             action: Action
-            logger.warning(f'Tool call in function_calling.py: {tool_call}')
+            logger.debug(f'Tool call in function_calling.py: {tool_call}')
             try:
                 arguments = json.loads(tool_call.function.arguments)
             except json.decoder.JSONDecodeError as e:
@@ -199,9 +199,9 @@ def response_to_actions(response: ModelResponse) -> list[Action]:
             # ================================================
             # McpAction (MCP)
             # ================================================
-            elif tool_call.function.name.endswith(BaseTool.postfix()):
+            elif tool_call.function.name.endswith(MCPClientTool.postfix()):
                 action = McpAction(
-                    name=tool_call.function.name.rstrip(BaseTool.postfix()),
+                    name=tool_call.function.name.rstrip(MCPClientTool.postfix()),
                     arguments=tool_call.function.arguments,
                 )
             else:

openhands/core/cli.py

@@ -5,6 +5,7 @@
 
 from termcolor import colored
 
+import openhands.agenthub  # noqa F401 (we import this to get the agents registered)
 from openhands.core.config import (
     AppConfig,
     parse_arguments,
@@ -36,7 +37,7 @@
     FileEditObservation,
 )
 from openhands.io import read_input, read_task
-from openhands.mcp.mcp import fetch_mcp_tools_from_config
+from openhands.mcp import fetch_mcp_tools_from_config
 
 
 def display_message(message: str):

openhands/core/config/mcp_config.py

@@ -31,50 +31,43 @@ def validate_servers(self) -> None:
                 raise ValueError(f'Invalid URL {url}: {str(e)}')
 
 
+class MCPStdioConfigEntry(BaseModel):
+    """Configuration for a single MCP stdio entry.
+
+    Attributes:
+        command: The command to run.
+        args: List of arguments for the command.
+        env: Dictionary of environment variables.
+    """
+
+    command: str
+    args: list[str] = Field(default_factory=list)
+    env: dict[str, str] = Field(default_factory=dict)
+
+    model_config = {'extra': 'forbid'}
+
+
 class MCPStdioConfig(BaseModel):
     """Configuration for MCP stdio settings.
 
     Attributes:
-        commands: List of commands to run.
-        args: List of arguments for each command.
-        envs: List of environment variable tuples for each command.
+        tools: Dictionary of tool configurations, where keys are tool names.
     """
 
-    commands: List[str] = Field(default_factory=list)
-    args: List[List[str]] = Field(default_factory=list)
-    envs: List[List[tuple[str, str]]] = Field(default_factory=list)
+    tools: dict[str, MCPStdioConfigEntry] = Field(default_factory=dict)
 
     model_config = {'extra': 'forbid'}
 
     def validate_stdio(self) -> None:
-        """Validate that commands, args, and envs are properly configured."""
-
-        # Check if number of commands matches number of args lists
-        if len(self.commands) != len(self.args):
-            raise ValueError(
-                f'Number of commands ({len(self.commands)}) does not match '
-                f'number of args lists ({len(self.args)})'
-            )
-
-        # Check if number of commands matches number of envs lists
-        if len(self.commands) != len(self.envs):
-            raise ValueError(
-                f'Number of commands ({len(self.commands)}) does not match '
-                f'number of envs lists ({len(self.envs)})'
-            )
-
-        # Validate each environment variable tuple
-        for i, env_list in enumerate(self.envs):
-            for j, env_tuple in enumerate(env_list):
-                if not isinstance(env_tuple, tuple) or len(env_tuple) != 2:
-                    raise ValueError(
-                        f'Environment variable at index {j} for command {i} must be a tuple of (key, value)'
-                    )
-                key, value = env_tuple
-                if not isinstance(key, str) or not isinstance(value, str):
-                    raise ValueError(
-                        f'Environment variable key and value at index {j} for command {i} must be strings'
-                    )
+        """Validate that tools are properly configured."""
+        # Tool names validation
+        for tool_name in self.tools:
+            if not tool_name.strip():
+                raise ValueError('Tool names cannot be empty')
+            if not tool_name.replace('-', '').isalnum():
+                raise ValueError(
+                    f'Invalid tool name: {tool_name}. Tool names must be alphanumeric (hyphens allowed)'
+                )
 
 
 class MCPConfig(BaseModel):
@@ -105,11 +98,11 @@ def from_toml_section(cls, data: dict) -> dict[str, 'MCPConfig']:
 
         try:
             # Create SSE config if present
-            sse_config = MCPSSEConfig(**data.get('mcp-sse', {}))
+            sse_config = MCPSSEConfig.model_validate(data.get('mcp-sse', {}))
             sse_config.validate_servers()
 
             # Create stdio config if present
-            stdio_config = MCPStdioConfig(**data.get('mcp-stdio', {}))
+            stdio_config = MCPStdioConfig.model_validate(data.get('mcp-stdio', {}))
             stdio_config.validate_stdio()
 
             # Create the main MCP config

openhands/core/main.py

@@ -30,7 +30,7 @@
 from openhands.events.event import Event
 from openhands.events.observation import AgentStateChangedObservation
 from openhands.io import read_input, read_task
-from openhands.mcp.mcp import fetch_mcp_tools_from_config
+from openhands.mcp import fetch_mcp_tools_from_config
 from openhands.memory.memory import Memory
 from openhands.runtime.base import Runtime
 from openhands.utils.async_utils import call_async_from_sync

openhands/llm/async_llm.py

@@ -1,6 +1,6 @@
 import asyncio
 from functools import partial
-from typing import Any
+from typing import Any, Callable
 
 from litellm import acompletion as litellm_acompletion
 
@@ -17,7 +17,7 @@
 class AsyncLLM(LLM):
     """Asynchronous LLM class."""
 
-    def __init__(self, *args, **kwargs):
+    def __init__(self, *args: Any, **kwargs: Any) -> None:
         super().__init__(*args, **kwargs)
 
         self._async_completion = partial(
@@ -46,7 +46,7 @@ def __init__(self, *args, **kwargs):
             retry_max_wait=self.config.retry_max_wait,
             retry_multiplier=self.config.retry_multiplier,
         )
-        async def async_completion_wrapper(*args, **kwargs):
+        async def async_completion_wrapper(*args: Any, **kwargs: Any) -> Any:
             """Wrapper for the litellm acompletion function that adds logging and cost tracking."""
             messages: list[dict[str, Any]] | dict[str, Any] = []
 
@@ -77,7 +77,7 @@ async def async_completion_wrapper(*args, **kwargs):
 
             self.log_prompt(messages)
 
-            async def check_stopped():
+            async def check_stopped() -> None:
                 while should_continue():
                     if (
                         hasattr(self.config, 'on_cancel_requested_fn')
@@ -117,14 +117,14 @@ async def check_stopped():
                 except asyncio.CancelledError:
                     pass
 
-        self._async_completion = async_completion_wrapper  # type: ignore
+        self._async_completion = async_completion_wrapper
 
-    async def _call_acompletion(self, *args, **kwargs):
+    async def _call_acompletion(self, *args: Any, **kwargs: Any) -> Any:
         """Wrapper for the litellm acompletion function."""
         # Used in testing?
         return await litellm_acompletion(*args, **kwargs)
 
     @property
-    def async_completion(self):
+    def async_completion(self) -> Callable:
         """Decorator for the async litellm acompletion function."""
         return self._async_completion

openhands/llm/bedrock.py

@@ -28,5 +28,5 @@ def list_foundation_models(
         return []
 
 
-def remove_error_modelId(model_list):
+def remove_error_modelId(model_list: list[str]) -> list[str]:
     return list(filter(lambda m: not m.startswith('bedrock'), model_list))

openhands/llm/debug_mixin.py

@@ -7,7 +7,7 @@
 
 
 class DebugMixin:
-    def log_prompt(self, messages: list[dict[str, Any]] | dict[str, Any]):
+    def log_prompt(self, messages: list[dict[str, Any]] | dict[str, Any]) -> None:
         if not messages:
             logger.debug('No completion messages!')
             return
@@ -24,30 +24,30 @@ def log_prompt(self, messages: list[dict[str, Any]] | dict[str, Any]):
         else:
             logger.debug('No completion messages!')
 
-    def log_response(self, message_back: str):
+    def log_response(self, message_back: str) -> None:
         if message_back:
             llm_response_logger.debug(message_back)
 
-    def _format_message_content(self, message: dict[str, Any]):
+    def _format_message_content(self, message: dict[str, Any]) -> str:
         content = message['content']
         if isinstance(content, list):
             return '\n'.join(
                 self._format_content_element(element) for element in content
             )
         return str(content)
 
-    def _format_content_element(self, element: dict[str, Any]):
+    def _format_content_element(self, element: dict[str, Any] | Any) -> str:
         if isinstance(element, dict):
             if 'text' in element:
-                return element['text']
+                return str(element['text'])
             if (
                 self.vision_is_active()
                 and 'image_url' in element
                 and 'url' in element['image_url']
             ):
-                return element['image_url']['url']
+                return str(element['image_url']['url'])
         return str(element)
 
     # This method should be implemented in the class that uses DebugMixin
-    def vision_is_active(self):
+    def vision_is_active(self) -> bool:
         raise NotImplementedError

openhands/llm/llm.py

@@ -186,7 +186,7 @@ def __init__(
             retry_multiplier=self.config.retry_multiplier,
             retry_listener=self.retry_listener,
         )
-        def wrapper(*args, **kwargs):
+        def wrapper(*args: Any, **kwargs: Any) -> Any:
             """Wrapper for the litellm completion function. Logs the input and output of the completion function."""
             from openhands.io import json
 
@@ -355,14 +355,14 @@ def wrapper(*args, **kwargs):
         self._completion = wrapper
 
     @property
-    def completion(self):
+    def completion(self) -> Callable:
         """Decorator for the litellm completion function.
 
         Check the complete documentation at https://litellm.vercel.app/docs/completion
         """
         return self._completion
 
-    def init_model_info(self):
+    def init_model_info(self) -> None:
         if self._tried_model_info:
             return
         self._tried_model_info = True
@@ -622,10 +622,12 @@ def get_token_count(self, messages: list[dict] | list[Message]) -> int:
         # try to get the token count with the default litellm tokenizers
         # or the custom tokenizer if set for this LLM configuration
         try:
-            return litellm.token_counter(
-                model=self.config.model,
-                messages=messages,
-                custom_tokenizer=self.tokenizer,
+            return int(
+                litellm.token_counter(
+                    model=self.config.model,
+                    messages=messages,
+                    custom_tokenizer=self.tokenizer,
+                )
             )
         except Exception as e:
             # limit logspam in case token count is not supported
@@ -654,7 +656,7 @@ def _is_local(self) -> bool:
                 return True
         return False
 
-    def _completion_cost(self, response) -> float:
+    def _completion_cost(self, response: Any) -> float:
         """Calculate completion cost and update metrics with running total.
 
         Calculate the cost of a completion response based on the model. Local models are treated as free.
@@ -707,21 +709,21 @@ def _completion_cost(self, response) -> float:
                 logger.debug(
                     f'Using fallback model name {_model_name} to get cost: {cost}'
                 )
-            self.metrics.add_cost(cost)
-            return cost
+            self.metrics.add_cost(float(cost))
+            return float(cost)
         except Exception:
             self.cost_metric_supported = False
             logger.debug('Cost calculation not supported for this model.')
         return 0.0
 
-    def __str__(self):
+    def __str__(self) -> str:
         if self.config.api_version:
             return f'LLM(model={self.config.model}, api_version={self.config.api_version}, base_url={self.config.base_url})'
         elif self.config.base_url:
             return f'LLM(model={self.config.model}, base_url={self.config.base_url})'
         return f'LLM(model={self.config.model})'
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return str(self)
 
     def reset(self) -> None: