fix(mcp): close reconciliation gaps left by #4367 - #4369
Conversation
- Wire on_tools_reconciled through MCPToolProvider/create_mcp_tools() instead of attaching it to the client after create_tools() returns. DefaultMCPToolProvider and SettingsBackedMCPToolProvider never forwarded it, so any notifications/tools/list_changed that arrived during the initial connect were silently dropped (reproduced deterministically against a live FastMCP server). - Stop mutating Agent._tools in place in add_runtime_tools / _on_mcp_tools_changed. Agent.model_copy() shares private attrs by reference, so in-place mutation leaked dynamically-added MCP tools into other Agent snapshots; replace the dict instead, matching _on_mcp_tools_reconciled. - Remove an unused tool_names assignment in AgentBase._initialize. - Bound the per-schema MCP validation-model cache (LRU, 512 entries) so a tool whose schema changes repeatedly can't grow it without limit.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
…owups' into fix/mcp-tool-reconciliation-followups
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - the direction is right and the fixes are focused, but two compatibility/concurrency gaps need maintainer attention before I can approve.
[CRITICAL ISSUES]
- See inline comments on the MCP provider compatibility break and unsynchronized LRU cache mutation.
[TESTING GAPS]
- The new regression tests cover the intended MCP reconciliation paths. I did not find a missing targeted test beyond the concurrency/compatibility cases called out inline.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches runtime MCP tool reconciliation and agent tool registration, which are shared agent/tool-calling paths and can affect evaluation behavior. The change follows existing patterns overall, but the public provider protocol change and global cache concurrency issue raise the risk above low. Per repo guidance, this agent-behavior change should be left as COMMENT unless a maintainer has provided eval evidence.
VERDICT:
❌ Needs rework: Preserve compatibility for existing custom MCPToolProvider implementations and make the new bounded action-type cache thread-safe.
KEY INSIGHT:
The reconciliation fix is sound, but the supporting plumbing needs to avoid turning an internal race fix into a provider API break or a new parallel-tool execution race.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
…ead-safe Follow-up to the two issues flagged in code review on this branch: - LocalConversation unconditionally passed on_tools_reconciled to MCPToolProvider.create_tools(), breaking any custom provider written against the pre-existing protocol (only on_tools_changed) with a TypeError on first MCP connection. Check the provider's signature via provider_supports_on_tools_reconciled() and omit the keyword for providers that don't accept it. - _create_mcp_action_type's cache-hit path did .get() then .move_to_end() as two separate steps; a concurrent eviction landing in between raised KeyError. Guard the whole get/move/insert/evict sequence with a lock, since MCP tool calls can validate concurrently through the parallel tool executor.
Adds direct coverage for the explicit-param, **kwargs, and unsupported-signature cases, on top of the indirect coverage from the legacy-provider integration test.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR fixes four issues in the MCP tool-reconciliation code introduced by #4367. I reviewed all 11 changed files against the surrounding codebase and verified each fix's correctness, test coverage, and edge cases.
Assessment
All four changes are correct, minimal, and well-tested. No material issues found.
1. Threading on_tools_reconciled through providers
create_mcp_tools() sets client._tools_reconciled_callback = on_tools_reconciled before client.call_async_from_sync(_connect_and_list_tools, ...), so any notifications/tools/list_changed arriving during the initial connect finds the callback already attached. This closes the race window by construction.
DefaultMCPToolProvider and SettingsBackedMCPToolProvider now forward the parameter instead of silently dropping it. The provider_supports_on_tools_reconciled() helper uses inspect.signature to maintain backward compatibility with custom providers that predate the parameter -- legacy providers get a debug log and skip reconciliation rather than raising TypeError. This is a defensible tradeoff: setting the callback after create_tools() returns would reintroduce the exact race being fixed.
Coverage: test_default_provider_wires_on_tools_reconciled_before_connect (live FastMCP server), test_settings_backed_provider_forwards_on_tools_reconciled, test_legacy_provider_without_on_tools_reconciled_still_works, test_provider_supports_on_tools_reconciled.
2. Replacing _tools instead of mutating in place
add_runtime_tools and the replacements block in _on_mcp_tools_changed now create a new dict via {**self._tools, ...} and assign it with object.__setattr__, matching the existing _on_mcp_tools_reconciled pattern. Since Agent.model_copy() shares private attrs by reference, mutating _tools in place would leak dynamically-registered tools into other snapshots -- the new approach prevents this.
The _tools_lock is an RLock, so the reentrant call to add_runtime_tools from within _on_mcp_tools_changed's lock block is safe. The replacements block reads self._tools after add_runtime_tools has already replaced it, correctly seeing the additions (additions and replacements are disjoint by construction).
Coverage: test_add_runtime_tools_does_not_leak_into_model_copy.
3. Removing unused tool_names in _initialize
The removed tool_names = [tool.name for tool in tools] was inside the if self.filter_tools_regex: block and was never read -- a separate tool_names is recomputed later (line 620) for the duplicate-name check. Genuinely dead code.
4. LRU-bounded _mcp_dynamic_action_type cache
The cache is now an OrderedDict capped at 512 entries, guarded by threading.Lock(). The entire get / move_to_end / create / insert / evict sequence is atomic under the lock, which prevents the race where a cache hit's move_to_end(key) could raise KeyError if another thread evicted that key between .get() and .move_to_end(). Schema.from_mcp_schema runs inside the lock, which serializes model creation -- an acceptable correctness-over-perf tradeoff since Pydantic model construction is fast and this runs at tool-call time, not in a tight loop.
Coverage: test_action_type_cache_is_bounded (eviction at 512+50 entries) and test_action_type_cache_serializes_get_and_evict (concurrency test using a PausingDict that forces the exact interleaving the lock prevents -- without the lock, move_to_end raises KeyError on an evicted key).
Risk Assessment
LOW. No public API changes (on_tools_reconciled was already keyword-only on create_mcp_tools()). Backward compatibility is preserved: legacy custom providers continue to work without reconciliation (which they never had before #4367). The concurrency fix is properly lock-guarded with thorough tests. The _tools isolation fix addresses a real cross-snapshot leak that could occur during load_plugin.
|
Thank you very much for taking the time to follow up on my PR and fix the gaps in #4367. I especially appreciate the callback wiring, copy-on-write tool map updates, bounded cache, and the compatibility/concurrency follow-ups. While validating the current head ( If a real MCP server sends I reproduced this through the full OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY'
import asyncio
import socket
import tempfile
import threading
import time
from pathlib import Path
import mcp.types as mt
from fastmcp import FastMCP
from fastmcp.server.middleware import Middleware, MiddlewareContext
from pydantic import SecretStr
from openhands.sdk import Agent, LLM
from openhands.sdk.conversation.impl.local_conversation import LocalConversation
from openhands.sdk.mcp.config import coerce_mcp_config
class NotifyDuringInitialList(Middleware):
def __init__(self):
self.first = True
async def on_list_tools(self, context: MiddlewareContext, call_next):
tools = await call_next(context)
if self.first:
self.first = False
assert context.fastmcp_context is not None
await context.fastmcp_context.send_notification(
mt.ToolListChangedNotification()
)
# Make the protocol-valid ordering deterministic: let the refresh
# triggered by the notification finish before this first list returns.
await asyncio.sleep(0.5)
return tools
with socket.socket() as sock:
sock.bind(("127.0.0.1", 0))
port = sock.getsockname()[1]
server = FastMCP("startup-race")
server.add_middleware(NotifyDuringInitialList())
@server.tool()
def only_tool(value: int = 1) -> int:
return value
def run_server():
asyncio.run(
server.run_http_async(
host="127.0.0.1",
port=port,
transport="http",
show_banner=False,
path="/mcp",
)
)
threading.Thread(target=run_server, daemon=True).start()
for _ in range(100):
try:
with socket.create_connection(("127.0.0.1", port), timeout=0.1):
break
except OSError:
time.sleep(0.05)
else:
raise RuntimeError("FastMCP server did not start")
with tempfile.TemporaryDirectory() as workspace:
agent = Agent(
llm=LLM(model="test-model", api_key=SecretStr("k")),
tools=[],
include_default_tools=[],
mcp_config=coerce_mcp_config(
{
"race": {
"transport": "http",
"url": f"http://127.0.0.1:{port}/mcp",
}
}
),
)
conversation = LocalConversation(
agent=agent,
workspace=Path(workspace),
visualizer=None,
)
try:
conversation._ensure_agent_ready()
except ValueError as exc:
assert "Duplicate tool names found" in str(exc)
print(type(exc).__name__, str(exc))
else:
conversation.close()
raise AssertionError("Expected duplicate-tool initialization failure")
PYObserved output: The Would you mind judging whether this case should be handled in this PR? My initial thought is that initial snapshot installation and reconciliation may need to be serialized or made idempotent, but please feel free to choose a better approach. Thank you again for improving the original change. |
…ls returns A notifications/tools/list_changed notification arriving while the initial create_tools() call is still in flight can install a tool via on_tools_changed/on_tools_reconciled before the caller's own add_runtime_tools() call runs with the client's returned snapshot, raising a spurious duplicate-tool-names ValueError and crashing agent initialization. Exempt tools already installed by the same MCPClient, matching the same-client check already used in _on_mcp_tools_changed/_on_mcp_tools_reconciled.
|
Hey @Shimada666 thanks for the follow-up. I think you are right. The fix was minimal so I pushed it to this PR. Can you review it? |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
@VascoSch92 Thanks for the quick follow-up and for pushing the fix. I took another look at it with some AI-assisted review and ran the real FastMCP reproduction again. The new change does fix the duplicate-tool exception when the notification and the initial response contain the same tool snapshot. I found one small variation of the startup edge case, though: if the notification represents an actual schema change, the later-arriving initial response can still overwrite the newer snapshot. The sequence I tested was:
Using the same real FastMCP Streamable HTTP setup and full So the crash is gone, but a genuinely newer schema can still be rolled back silently. A removal during the same window may have a similar stale-snapshot issue. The added unit test uses the same This looks like a small startup timing edge case, and I found it while investigating together with AI, so please use your judgment on whether it is worth handling in this PR or in another way. My only suggestion would be to keep any fix scoped to MCP initialization/snapshot ordering rather than broadly changing |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR closes four reconciliation gaps left by #4367. I reviewed all 11 changed files against the surrounding codebase, traced each fix through its call paths, and checked correctness, concurrency, backward compatibility, and test coverage.
Assessment
No material issues found. All four fixes are correct, minimal, and well-tested. The two critical issues raised in the prior bot review (provider compatibility break and unsynchronized LRU cache mutation) are properly resolved in c9fda65, and the newest commit (9f7327a) adds sound startup-race tolerance.
1. Threading on_tools_reconciled through providers
create_mcp_tools() now sets client._tools_reconciled_callback = on_tools_reconciled before call_async_from_sync(_connect_and_list_tools) (utils.py:361-365), so any notifications/tools/list_changed arriving during the initial connect finds the callback attached. provider_supports_on_tools_reconciled() uses inspect.signature to preserve compatibility with legacy custom providers that predate the parameter — they get a debug log and skip reconciliation rather than raising TypeError. This is a defensible tradeoff since legacy providers never had reconciliation before #4367. No public API break: on_tools_reconciled was already keyword-only on create_mcp_tools().
2. Replacing _tools instead of mutating in place
add_runtime_tools and the replacements block in _on_mcp_tools_changed now build a new dict ({**self._tools, ...}) and assign via object.__setattr__, matching the existing _on_mcp_tools_reconciled pattern (base.py:1012). This correctly prevents the cross-snapshot leak via Agent.model_copy(), which shares private attrs by reference.
The new startup-race tolerance in add_runtime_tools is sound: it exempts tools where both the existing and replacement executors are MCPToolExecutor instances sharing the same client (is), matching the same-client exemption already used by _on_mcp_tools_changed/_on_mcp_tools_reconciled. Different MCP clients with the same tool name still raise a conflict (the is check fails), and non-MCP/non-MCP collisions still raise — preserving the original duplicate detection where it matters. The reentrant add_runtime_tools(additions) call from within the _tools_lock block is safe because _tools_lock is an RLock, and additions/replacements are disjoint by construction, so reading self._tools after the additions replacement correctly sees the overlay base for replacements.
3. Removing unused tool_names in _initialize
The removed tool_names = [tool.name for tool in tools] was inside the filter_tools_regex branch and was never read — a separate tool_names is recomputed later (base.py:620) for the duplicate-name check. Genuinely dead code.
4. LRU-bounded, lock-guarded action-type cache
_mcp_dynamic_action_type is now an OrderedDict capped at 512, with the entire get/move_to_end/insert/evict sequence atomic under threading.Lock(). This closes the KeyError race where a concurrent eviction could land between .get() and .move_to_end(). A plain Lock() (not RLock) is correct here since _create_mcp_action_type is not reentrant. Schema.from_mcp_schema runs inside the lock — an acceptable correctness-over-perf tradeoff for a path that runs at tool-call time, not in a hot loop.
Test coverage
Coverage is thorough and targeted:
test_default_provider_wires_on_tools_reconciled_before_connect— live FastMCP server, asserts the callback is attached before connect.test_settings_backed_provider_forwards_on_tools_reconciled— mockscreate_mcp_toolsand asserts the callback reaches it (theMCPSettingsOAuthTokenStore()instantiation is safe sinceget_settings_store()is called lazily inside its methods, not in__init__).test_legacy_provider_without_on_tools_reconciled_still_works/test_provider_supports_on_tools_reconciled— explicit-param,**kwargs, and unsupported-signature cases.test_add_runtime_tools_does_not_leak_into_model_copy— the cross-snapshot leak (failed before the fix).test_add_runtime_tools_tolerates_notification_installed_before_return— the startup-race scenario.test_action_type_cache_is_bounded(eviction at 512+50) andtest_action_type_cache_serializes_get_and_evict(forces the exact interleaving the lock prevents via aPausingDict).
Risk Assessment
LOW. No public API changes. Backward compatibility is preserved for legacy custom providers (they continue to work without reconciliation, which they never had before #4367). The concurrency fix is properly lock-guarded with tests that force the raced interleaving. The _tools isolation fix addresses a real cross-snapshot leak that could occur during load_plugin/_ensure_agent_ready.
4 participants
HUMAN:
Fix some bug introduced in #4367
AGENT:
Why
Follow-up review of #4367 (already merged) found four issues in the new MCP
tool-reconciliation code:
DefaultMCPToolProvider/SettingsBackedMCPToolProvidernever forwardedon_tools_reconciledtocreate_mcp_tools().LocalConversationcompensated by attaching the callback to the client after
create_tools()returned, which is after the client has alreadyconnected and started listening for
notifications/tools/list_changed.Any such notification arriving in that window is silently dropped —
reproduced deterministically against a live FastMCP server (see "How to
Test").
add_runtime_tools/_on_mcp_tools_changedmutateAgent._toolsinplace.
Agent.model_copy()shares private attrs by reference, so adynamically-registered MCP tool can leak into other
Agentsnapshotsthat share the same
_toolsdict (LocalConversation.load_pluginreassigns
self.agentviamodel_copy()right after wiring MCPcallbacks to the pre-copy agent).
AgentBase._initializecomputed an unusedtool_nameslist._mcp_dynamic_action_type) iskeyed by
(name, schema)with no eviction, so a tool whose schema keepschanging grows it without bound.
Summary
on_tools_reconciledthroughMCPToolProvider.create_tools()/create_mcp_tools()instead of setting it on the client after the fact.Agent._toolsinstead of mutating it in place inadd_runtime_tools/_on_mcp_tools_changed, matching_on_mcp_tools_reconciled.tool_namesassignment inAgentBase._initialize._mcp_dynamic_action_typeto an LRU of 512 entries.Issue Number
Follow-up to #4367.
How to Test
Run:
Observed:
1710 passedacross the broadertests/sdk/mcp/,tests/sdk/conversation/,tests/sdk/agent/suites plus the agent-serverOAuth store tests.
Before writing the fix, item 1 was confirmed with a standalone script
(not included in this PR) that ran a real FastMCP HTTP server, forced a
tools/list_changednotification to fire whileDefaultMCPToolProviderwas still inside
create_tools(), and observedon_tools_reconciledneverfiring for the tool added by that notification even though the client's
own
client.toolswas updated correctly.test_default_provider_wires_on_tools_reconciled_before_connect(new) asserts the callback is attached to the client synchronously, before
create_tools()returns — closing that window by construction.Item 2 is covered by the new
test_add_runtime_tools_does_not_leak_into_model_copy, which registers atool on a
model_copy()'dAgentand asserts the original is unaffected(this failed before the fix).
Item 4 is covered by
test_action_type_cache_is_bounded.Also ran, on all changed files:
All passed (pre-commit: ruff format, ruff lint, pycodestyle, pyright,
import dependency rules, tool-subclass registration).
Video/Screenshots
Not applicable; this is SDK runtime behavior covered by unit tests and a
live FastMCP server test.
Type
Notes
No public API changes:
on_tools_reconciledwas already a keyword-onlyparameter on
create_mcp_tools(); this PR just makesMCPToolProvider.create_tools()implementations forward it instead ofsilently dropping it.
_tools_reconciled_callbackis still set as anattribute on the returned
MCPClient, just earlier (insidecreate_mcp_tools(), before connecting) instead of by the callerafterward.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:9f7327a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9f7327a-python) is a multi-arch manifest supporting both amd64 and arm649f7327a-python-amd64) are also available if needed