Repository navigation
Python: fix(core): do not replay MCP tools/call without an idempotency hint - #9243
Yufeng He (he-yufeng) wants to merge 5 commits into
Conversation
…y hint When the connection drops during an ordinary tools/call, the request may already have executed server-side before the response was lost. The reconnect-and-retry added for microsoft#2884 re-issues the same call blindly, so a non-idempotent operation (charge, write, ticket) can run twice. The long-running task path already refuses to re-issue an augmented call before a task_id is known for exactly this reason. Gate the replay on the tool's advertised ToolAnnotations: tools with idempotentHint or readOnlyHint keep the optimistic reconnect-and-retry, anything else raises ToolExecutionException stating the outcome is unknown, with the connection error as the inner exception, and leaves reconnection to the caller's own retry policy. tools/list, get_prompt, and the task lifecycle keep their current retry behavior. Fixes microsoft#9204
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
🟡 Changes recommended
The annotation snapshot is not restored when connection discovery fails, allowing stale replay guarantees.
1 open finding
What changed in this PR
Prevents unsafe replay of MCP tool calls after connection loss unless the tool advertises read-only or idempotent behavior.
Changes:
- Captures tool annotations and gates reconnect retries.
- Adds regression tests for duplicate side effects.
- Documents the replay policy.
| File | Description |
|---|---|
python/packages/core/agent_framework/_mcp.py |
Implements annotation-based replay gating. |
python/packages/core/tests/core/test_mcp.py |
Tests replayable and non-replayable calls. |
python/packages/core/AGENTS.md |
Documents the new policy. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| self._functions[:] = current_functions | ||
| self._tool_call_meta_by_name = tool_call_meta_by_name | ||
| self._tool_task_support_by_name = tool_task_support_by_name | ||
| self._tool_annotations_by_name = tool_annotations_by_name |
There was a problem hiding this comment.
Good catch, done in 479c00a. The annotations snapshot is now saved with the other maps before discovery and restored in the rollback, so a connect where load_tools succeeds and a later step fails no longer leaves the failed session's hints behind. Covered by test_connect_discovery_failure_rolls_back_annotations_snapshot: it seeds pre-discovery sentinels in all four maps, lets tool loading publish a fresh annotated tool, then fails prompt loading. Against the previous code the annotations map still holds the failed session's entry after the rollback (test fails); with the fix all four maps and the function list come back to their sentinels. Full test_mcp.py green, ruff check and format clean.
Asserting through server.session (ClientSession | None) tripped every typing gate: the optional deref and await_count on a bound method. Keep the AsyncMock on a declared attribute and assert on that instead.
GitHub Actions
Active
…iscovery _connect_on_owner rolled back the call-meta, task-support, and param-name maps when a later discovery step failed, but not the annotations snapshot taken from tools/list. A connect where load_tools succeeded and prompt loading then failed left the failed session's annotations in place, so a later replay decision could trust idempotency hints from a session whose functions had already been rolled back.
GitHub Actions
Active
| the call would duplicate its side effects. | ||
| """ | ||
| annotations = self._tool_annotations_by_name.get(tool_name) | ||
| return bool(annotations and (annotations.idempotentHint or annotations.readOnlyHint)) |
There was a problem hiding this comment.
Yufeng He (@he-yufeng) this still lets the MCP server authorize replay by setting idempotentHint or readOnlyHint. Those annotations are untrusted unless the client has separately trusted the server, and this package’s security layer already treats ToolAnnotations as restriction-only hints. On a lost response, a misannotated or untrusted server can still get the same tools/call executed twice. Please keep the ambiguous-response path as no-replay by default, or gate replay on an explicit client-owned trusted policy in addition to the annotation.
…y-without-idempotency # Conflicts: # python/packages/core/AGENTS.md # python/packages/core/agent_framework/_mcp.py
GitHub Actions
Active
…y-without-idempotency
GitHub Actions
Active
Motivation & Context
When the MCP connection drops during an ordinary
tools/call, the request may already have executed server-side before the response was lost. The reconnect-and-retry added for #2884 re-issues the same call unconditionally onClosedResourceError/ session-terminatedMcpError, so a non-idempotent operation (a charge, a write, a ticket creation) can run twice. The long-running task path already refuses to re-issue an augmented call before atask_idis known for exactly this reason; the plain call path now follows the same policy.Description & Review Guide
_call_tool_with_retriesnow gates the replay on the tool's advertisedToolAnnotations(captured fromtools/listinto a new_tool_annotations_by_namesnapshot alongside the existing meta/task-support maps). Tools withidempotentHintorreadOnlyHintkeep the optimistic reconnect-and-retry from Python: MCP connection not properly invalidated after async generator cleanup #2884. Any other tool raisesToolExecutionExceptionstating that the connection was lost and the remote execution outcome is unknown, with the connection error as the inner exception, and leaves reconnection to the caller's own retry policy.tools/list,get_prompt, and the long-running task lifecycle keep their current retry behavior._call_tool_with_retriesand whetheridempotentHint/readOnlyHintis the right advertised signal; also the updated expectations in the two pre-existing reconnect tests, which now annotate their tools as replayable.Related Issue
Fixes #9204
AI Assistance
AI assistance details: implementation, tests, and this PR description were prepared with AI assistance. Verification actually run: full
packages/core/tests/core/test_mcp.py(396 passed, 2 skipped),poe syntax -P core(fmt + lint),poe pyright -P core(same 151 pre-existing errors as unmodified main, none in the changed files).Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.