fix(mcp): close reconciliation gaps left by #4367

openhands-agent-server/openhands/agent_server/mcp_oauth_store.py

@@ -26,6 +26,7 @@
 )
 from openhands.sdk.mcp.utils import (
     ToolsChangedCallback,
+    ToolsReconciledCallback,
     create_mcp_tools,
 )
 
@@ -337,12 +338,14 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: ToolsChangedCallback | None = None,
+        on_tools_reconciled: ToolsReconciledCallback | None = None,
     ) -> MCPClient:
         return create_mcp_tools(
             mcp_config,
             timeout,
             mcp_oauth_token_storage=MCPSettingsOAuthTokenStore(),
             on_tools_changed=on_tools_changed,
+            on_tools_reconciled=on_tools_reconciled,
         )
 
 

openhands-sdk/openhands/sdk/agent/base.py

@@ -564,7 +564,6 @@ def _initialize(
         if self.filter_tools_regex:
             pattern = re.compile(self.filter_tools_regex)
             tools = [tool for tool in tools if pattern.match(tool.name)]
-            tool_names = [tool.name for tool in tools]
             logger.info("Filtered to %d tools after applying regex filter", len(tools))
 
         # Include default tools from include_default_tools; not subject to regex
@@ -872,13 +871,35 @@ def add_runtime_tools(self, tools: Sequence[ToolDefinition]) -> None:
             }
             raise ValueError(f"Duplicate runtime tool names found: {duplicates}")
         with self._tools_lock:
-            existing = set(self._tools) & set(tool_names)
-            if existing:
-                raise ValueError(f"Duplicate tool names found: {existing}")
-
-            # AgentBase is frozen, so update its mutable tool map in place.
+            # A tools/list_changed notification can race the caller: if it
+            # arrives while the provider's initial create_tools() call is
+            # still in flight, _on_mcp_tools_changed/_on_mcp_tools_reconciled
+            # may already have installed the same tool via this same client
+            # before the caller's own add_runtime_tools() call (using the
+            # client's returned snapshot) gets here. Treat that as a refresh,
+            # not a conflict, matching the same-client exemption already used
+            # by _on_mcp_tools_changed/_on_mcp_tools_reconciled.
+            conflicts: set[str] = set()
             for tool in tools:
-                self._tools[tool.name] = tool
+                existing_tool = self._tools.get(tool.name)
+                if existing_tool is None:
+                    continue
+                existing_executor = existing_tool.executor
+                replacement_executor = tool.executor
+                if (
+                    isinstance(existing_executor, MCPToolExecutor)
+                    and isinstance(replacement_executor, MCPToolExecutor)
+                    and existing_executor.client is replacement_executor.client
+                ):
+                    continue
+                conflicts.add(tool.name)
+            if conflicts:
+                raise ValueError(f"Duplicate tool names found: {conflicts}")
+
+            # AgentBase is frozen; replace the tool map rather than mutating
+            # it in place, so Agent.model_copy() snapshots don't share state.
+            updated = {**self._tools, **{tool.name: tool for tool in tools}}
+            object.__setattr__(self, "_tools", updated)
 
     def _on_mcp_tools_changed(self, tools: Sequence[ToolDefinition]) -> None:
         """Handle dynamically advertised MCP tools.
@@ -930,8 +951,12 @@ def _on_mcp_tools_changed(self, tools: Sequence[ToolDefinition]) -> None:
                 )
 
             self.add_runtime_tools(additions)
-            for tool in replacements:
-                self._tools[tool.name] = tool
+            if replacements:
+                updated = {
+                    **self._tools,
+                    **{tool.name: tool for tool in replacements},
+                }
+                object.__setattr__(self, "_tools", updated)
 
         if additions:
             logger.info(

openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py

@@ -72,6 +72,7 @@
     MCPToolProvider,
     ToolsChangedCallback,
     ToolsReconciledCallback,
+    provider_supports_on_tools_reconciled,
 )
 from openhands.sdk.observability.laminar import OPERATION_METADATA_KEY, observe
 from openhands.sdk.plugin import (
@@ -1288,12 +1289,18 @@ def _runtime_mcp_tools(
         mcp_config = enabled_mcp_servers(mcp_config)
         if not mcp_config:
             return []
+        create_kwargs: dict[str, Any] = {"on_tools_changed": on_tools_changed}
+        if provider_supports_on_tools_reconciled(self._mcp_tool_provider):
+            create_kwargs["on_tools_reconciled"] = on_tools_reconciled
+        elif on_tools_reconciled is not None:
+            logger.debug(
+                "%s does not accept on_tools_reconciled; dynamic MCP tool "
+                "removals/updates won't reach the agent for this provider",
+                type(self._mcp_tool_provider).__name__,
+            )
         client = self._mcp_tool_provider.create_tools(
-            mcp_config,
-            _RUNTIME_MCP_TIMEOUT_SECS,
-            on_tools_changed=on_tools_changed,
+            mcp_config, _RUNTIME_MCP_TIMEOUT_SECS, **create_kwargs
         )
-        client._tools_reconciled_callback = on_tools_reconciled
         return list(client.tools)
 
     def _on_mcp_tools_reconciled(

openhands-sdk/openhands/sdk/mcp/tool.py

@@ -3,8 +3,10 @@
 import copy
 import json
 import re
+import threading
+from collections import OrderedDict
 from collections.abc import Sequence
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, Final
 
 
 if TYPE_CHECKING:
@@ -196,7 +198,12 @@ def close(self) -> None:
         self.client.sync_close()
 
 
-_mcp_dynamic_action_type: dict[tuple[str, str], type[Schema]] = {}
+_MCP_ACTION_TYPE_CACHE_MAX: Final[int] = 512
+# LRU-bounded: keyed by (name, schema), so a tool whose schema keeps changing
+# no longer grows this cache without limit. Guarded by a lock since MCP tool
+# calls can validate concurrently through the parallel tool executor.
+_mcp_dynamic_action_type: OrderedDict[tuple[str, str], type[Schema]] = OrderedDict()
+_mcp_dynamic_action_type_lock = threading.Lock()
 
 
 def _create_mcp_action_type(action_type: mcp.types.Tool) -> type[Schema]:
@@ -218,15 +225,19 @@ def _create_mcp_action_type(action_type: mcp.types.Tool) -> type[Schema]:
         action_type.name,
         json.dumps(action_type.inputSchema, sort_keys=True, separators=(",", ":")),
     )
-    mcp_action_type = _mcp_dynamic_action_type.get(cache_key)
-    if mcp_action_type:
+    with _mcp_dynamic_action_type_lock:
+        mcp_action_type = _mcp_dynamic_action_type.get(cache_key)
+        if mcp_action_type:
+            _mcp_dynamic_action_type.move_to_end(cache_key)
+            return mcp_action_type
+
+        model_name = f"MCP{to_camel_case(action_type.name)}Action"
+        mcp_action_type = Schema.from_mcp_schema(model_name, action_type.inputSchema)
+        _mcp_dynamic_action_type[cache_key] = mcp_action_type
+        if len(_mcp_dynamic_action_type) > _MCP_ACTION_TYPE_CACHE_MAX:
+            _mcp_dynamic_action_type.popitem(last=False)
         return mcp_action_type
 
-    model_name = f"MCP{to_camel_case(action_type.name)}Action"
-    mcp_action_type = Schema.from_mcp_schema(model_name, action_type.inputSchema)
-    _mcp_dynamic_action_type[cache_key] = mcp_action_type
-    return mcp_action_type
-
 
 class MCPToolDefinition(ToolDefinition[MCPToolAction, MCPToolObservation]):
     """MCP Tool that wraps an MCP client and provides tool functionality."""

openhands-sdk/openhands/sdk/mcp/utils.py

@@ -1,6 +1,7 @@
 """Utility functions for MCP integration."""
 
 import asyncio
+import inspect
 import logging
 from collections.abc import Callable, Mapping, Sequence
 from typing import Protocol
@@ -46,6 +47,7 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: ToolsChangedCallback | None = None,
+        on_tools_reconciled: ToolsReconciledCallback | None = None,
     ) -> MCPClient: ...
 
 
@@ -58,8 +60,31 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: ToolsChangedCallback | None = None,
+        on_tools_reconciled: ToolsReconciledCallback | None = None,
     ) -> MCPClient:
-        return create_mcp_tools(mcp_config, timeout, on_tools_changed=on_tools_changed)
+        return create_mcp_tools(
+            mcp_config,
+            timeout,
+            on_tools_changed=on_tools_changed,
+            on_tools_reconciled=on_tools_reconciled,
+        )
+
+
+def provider_supports_on_tools_reconciled(provider: MCPToolProvider) -> bool:
+    """Whether ``provider.create_tools`` accepts ``on_tools_reconciled``.
+
+    Custom ``MCPToolProvider`` implementations written before this parameter
+    existed only accept ``on_tools_changed``; passing the new keyword to
+    them would raise ``TypeError``. Callers should check this first and omit
+    the keyword for providers that don't support it.
+    """
+    try:
+        params = inspect.signature(provider.create_tools).parameters
+    except (TypeError, ValueError):
+        return False
+    return "on_tools_reconciled" in params or any(
+        p.kind is inspect.Parameter.VAR_KEYWORD for p in params.values()
+    )
 
 
 def _oauth_auth_from_authentication_config(

tests/agent_server/test_mcp_oauth_store.py

@@ -6,6 +6,7 @@
 import threading
 import time
 from pathlib import Path
+from unittest.mock import patch
 from urllib.parse import parse_qs, urlparse
 
 import httpx
@@ -20,6 +21,7 @@
 from openhands.agent_server.config import Config
 from openhands.agent_server.mcp_oauth_store import (
     MCPSettingsOAuthTokenStore,
+    SettingsBackedMCPToolProvider,
     create_settings_backed_mcp_tool_provider,
 )
 from openhands.agent_server.persistence import (
@@ -362,3 +364,24 @@ async def test_mcp_oauth_token_storage_does_not_attach_to_non_oauth_server(
         assert "auth" not in server
     finally:
         reset_stores()
+
+
+def test_settings_backed_provider_forwards_on_tools_reconciled():
+    """on_tools_reconciled must reach create_mcp_tools(), not be dropped.
+
+    Previously this provider only accepted on_tools_changed, so callers had
+    to attach on_tools_reconciled to the returned client after the fact --
+    missing any notification that arrived during the initial connect.
+    """
+    provider = SettingsBackedMCPToolProvider()
+    config = coerce_mcp_config({"fake": {"command": "true"}})
+
+    def callback(client, tools):
+        return None
+
+    with patch(
+        "openhands.agent_server.mcp_oauth_store.create_mcp_tools"
+    ) as mock_create:
+        provider.create_tools(config, on_tools_reconciled=callback)
+
+    assert mock_create.call_args.kwargs["on_tools_reconciled"] is callback

tests/sdk/agent/test_filter_tools_regex.py

@@ -8,7 +8,7 @@
 
 import uuid
 from collections.abc import Sequence
-from typing import ClassVar, cast
+from typing import Any, ClassVar, cast
 
 import pytest
 
@@ -252,6 +252,7 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: ToolsChangedCallback | None = None,
+        on_tools_reconciled: Any = None,
     ) -> MCPClient:
         return cast(
             MCPClient,

tests/sdk/conversation/test_local_conversation_mcp.py

@@ -11,6 +11,7 @@
 from openhands.sdk.mcp.client import MCPClient
 from openhands.sdk.mcp.config import MCPServer, coerce_mcp_config
 from openhands.sdk.mcp.tool import MCPToolDefinition
+from openhands.sdk.mcp.utils import MCPToolProvider
 
 
 class EmptyMCPClient:
@@ -35,8 +36,10 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: Any = None,
+        on_tools_reconciled: Any = None,
     ) -> MCPClient:
         self.calls.append(mcp_config)
+        self.client._tools_reconciled_callback = on_tools_reconciled
         return cast(MCPClient, self.client)
 
 
@@ -100,3 +103,63 @@ def test_reconciliation_targets_replaced_agent(tmp_path: Path) -> None:
     assert set(conversation.agent.tools_map) == {"replacement"}
     assert set(old_agent.tools_map) == {"initial"}
     conversation.close()
+
+
+class LegacyMCPToolProvider:
+    """A custom provider written against the pre-reconciliation protocol."""
+
+    def create_tools(
+        self,
+        mcp_config: dict[str, MCPServer],
+        timeout: float = 30.0,
+        *,
+        on_tools_changed: Any = None,
+    ) -> MCPClient:
+        return cast(MCPClient, EmptyMCPClient())
+
+
+def test_legacy_provider_without_on_tools_reconciled_still_works(
+    tmp_path: Path,
+) -> None:
+    """A custom MCPToolProvider that predates on_tools_reconciled must not
+    break; it just won't receive full-snapshot reconciliation."""
+    conversation = LocalConversation(
+        agent=Agent(
+            llm=LLM(model="test-model", api_key=SecretStr("test-key")),
+            tools=[],
+            include_default_tools=[],
+            mcp_config=coerce_mcp_config({"fake": {"command": "true"}}),
+        ),
+        workspace=str(tmp_path),
+        visualizer=None,
+        # Deliberately incompatible with the current MCPToolProvider
+        # protocol shape; that's the scenario under test.
+        mcp_tool_provider=cast(MCPToolProvider, LegacyMCPToolProvider()),
+    )
+
+    conversation._ensure_agent_ready()
+
+    conversation.close()
+
+
+class _KwargsMCPToolProvider:
+    """A provider that accepts arbitrary keywords via **kwargs."""
+
+    def create_tools(
+        self, mcp_config: dict[str, MCPServer], timeout: float = 30.0, **kwargs: Any
+    ) -> MCPClient:
+        return cast(MCPClient, EmptyMCPClient())
+
+
+def test_provider_supports_on_tools_reconciled() -> None:
+    from openhands.sdk.mcp.utils import (
+        DefaultMCPToolProvider,
+        provider_supports_on_tools_reconciled,
+    )
+
+    assert provider_supports_on_tools_reconciled(DefaultMCPToolProvider())
+    assert provider_supports_on_tools_reconciled(RecordingMCPToolProvider())
+    assert provider_supports_on_tools_reconciled(_KwargsMCPToolProvider())
+    assert not provider_supports_on_tools_reconciled(
+        cast(MCPToolProvider, LegacyMCPToolProvider())
+    )

tests/sdk/conversation/test_local_conversation_plugins.py

@@ -31,6 +31,7 @@
 class EmptyMCPClient:
     def __init__(self):
         self.tools = []
+        self._tools_reconciled_callback: Any = None
 
 
 class RecordingMCPToolProvider:
@@ -41,7 +42,7 @@ def __init__(
         state_locked: Callable[[], bool] | None = None,
     ):
         self.created = created
-        self.client = client or EmptyMCPClient()
+        self.client: Any = client or EmptyMCPClient()
         self.state_locked = state_locked
 
     def create_tools(
@@ -50,11 +51,13 @@ def create_tools(
         timeout: float = 30.0,
         *,
         on_tools_changed: Any = None,
+        on_tools_reconciled: Any = None,
     ) -> MCPClient:
         if self.state_locked is None:
             self.created.append(mcp_config)
         else:
             self.created.append((mcp_config, self.state_locked()))
+        self.client._tools_reconciled_callback = on_tools_reconciled
         return cast(MCPClient, self.client)