Repository navigation
Python: fix(core): do not replay MCP tools/call without an idempotency hint - #9243
Yufeng He (he-yufeng) wants to merge 3 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
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.
GitHub Actions
Active
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.
There was a problem hiding this comment.
Fair point, addressed in b5c2244. The framework's own posture is that ToolAnnotations pass through to the host callback without the package making trust decisions from them, so my annotation-only gate was inconsistent with that.
Replay now requires both halves: the server's idempotentHint/readOnlyHint AND a new client-owned trust_server_idempotency_hints=True opt-in on the MCP tool (declared on MCPTool and the three concrete subclasses). Default stays no-replay: an ambiguous lost-response surfaces the unknown-outcome error regardless of what the server claims.
Tests updated accordingly: the reconnect/retry tests now opt in explicitly, and a new case pins that a hint alone (no client trust) still does not replay. test_mcp.py: 415 passed, 2 skipped; pyright clean on _mcp.py.
GitHub Actions
Active
GitHub Actions
Active
…otency hints ToolAnnotations are server-supplied and untrusted by default, so an annotation alone must not authorize replaying an ambiguous tools/call. Replay now additionally requires the client-owned trust_server_idempotency_hints=True opt-in on the MCP tool; without it the lost-response path stays no-replay and surfaces the unknown outcome. Review thread: microsoft#9243 (comment)
GitHub Actions
Active
|
One follow-up worth considering: when the error says the outcome is unknown, the model may simply call the tool again with a new request, which brings back the duplicate this PR removes. Passing a stable per-call key the server can dedupe on (or documenting that pattern for mutating tools) would make that case safe too. |
|
That is the right next question to ask. The failure mode you describe is real: once the client refuses to replay, the model may simply issue a fresh call, which lands the same mutation twice without the dedupe this PR removed. A stable per-call idempotency key would close it, but the MCP tools/call envelope has no standard field for one today, so it would need either a protocol-level addition or a documented per-server convention for mutating tools. I would rather not grow this PR into protocol design; I can open a tracking issue proposing the idempotency-key pattern for tool calls if that sounds right to you. |
|
|
||
| # The query tool is read-only, so a lost response is safe to replay: mark it as such. | ||
| remote_name = (func.additional_properties or {}).get("_mcp_remote_name") or func.name | ||
| tool._tool_annotations_by_name[remote_name] = types.ToolAnnotations(readOnlyHint=True) |
There was a problem hiding this comment.
This test still constructs MCPStreamableHTTPTool with the default trust_server_idempotency_hints=False. Adding the read-only hint here therefore does not make _is_call_replayable() true, so the second invocation raises the unknown-outcome error instead of reconnecting. Please opt this test into trust_server_idempotency_hints=True while retaining the hint, so it exercises the intended replay path.
There was a problem hiding this comment.
Good catch, and the reason it slipped past the unit run is that this one is integration-marked and skips without LOCAL_MCP_URL. Opted the tool into trust_server_idempotency_hints in 56d0dd8, keeping the read-only hint, so the second invocation reaches the reconnect the test is about.
| - **`function_invocation_kwargs` and MCP servers** - That dict is shared across every tool in the run, including every attached `MCPTool`, and any name in it reaches a server that declares a matching `inputSchema` property. `header_provider` does not mitigate this — it reads the kwargs without consuming them. To keep a credential out of tool arguments, source it outside `function_invocation_kwargs`: read a `ContextVar` inside the provider (this still allows a different value per request), configure a custom `http_client`, or use `env` for `MCPStdioTool`. | ||
| - **Sampling guardrails** (`sampling_callback`) - Passing `client=` advertises `SamplingCapability` so the server can send `sampling/createMessage`. Because remote servers are untrusted (confused-deputy risk), the default `sampling_callback` is **deny-by-default** and applies, in order: a per-session rate limit (`sampling_max_requests`, default `_DEFAULT_SAMPLING_MAX_REQUESTS`), an approval gate (`sampling_approval_callback`), and a `maxTokens` cap (`sampling_max_tokens`, default `_DEFAULT_SAMPLING_MAX_TOKENS`). The approval callback (constructor arg on all subclasses; exported type alias `SamplingApprovalCallback`) receives the raw `CreateMessageRequestParams`, may be sync or async, and must return truthy to approve. When it is `None` (the default) every sampling request is denied; pass `lambda params: True` to restore legacy auto-approve as an explicit opt-in. Requests and denials are logged at WARNING (content is not logged). The per-session counter resets in `_reset_session_state`. | ||
| - **MCP Tasks extension** - MCP Python SDK 2.2 does not implement the 2026 `io.modelcontextprotocol/tasks` runtime. Tools that advertise `execution.taskSupport == "required"` are skipped with a warning; optional and task-free tools continue through ordinary `tools/call`. Restoration of the experimental Tasks API and removed sample is tracked in microsoft/agent-framework#8245. | ||
| - **Plain `tools/call` replay gating** - On `ClosedResourceError` / session-terminated `MCPError` the request may already have executed server-side, so `_call_tool_with_retries` only reconnects and replays when the server advertised the tool with `idempotentHint` or `readOnlyHint` (`_is_call_replayable`, backed by the `_tool_annotations_by_name` snapshot from `tools/list`); any other tool 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. Read-only discovery (`tools/list`) and `get_prompt` keep their reconnect-and-retry behavior. |
There was a problem hiding this comment.
This guidance omits the new client-owned trust gate. A server hint alone no longer permits replay; _is_call_replayable() also requires trust_server_idempotency_hints=True. Please document both conditions so this durable guidance matches the implementation and its default security posture.
There was a problem hiding this comment.
This one landed in the rebuild that followed your comment: the AGENTS.md bullet now reads "only replays after reconnect when the server advertised the tool with idempotentHint or readOnlyHint and the client opted in with trust_server_idempotency_hints (_is_call_replayable, backed by the _tool_annotations_by_name snapshot from tools/list)", and the default posture is spelled out on the constructor docs as "Annotations are server-supplied and untrusted by default, so the default False never replays an ambiguous call". If you want the default security posture called out even harder in AGENTS.md, say the word and I will make it explicit in its own sentence.
…y hint Fixes microsoft#9204. 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 microsoft#2884 re-issues the same call unconditionally on ClosedResourceError / session-terminated McpError, 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 a task_id is known for exactly this reason; the plain call path now follows the same policy. _call_tool_with_retries gates the replay on the tool's advertised ToolAnnotations, captured from tools/list into a _tool_annotations_by_name snapshot alongside the existing meta/task-support maps. Replay requires both a server idempotentHint or readOnlyHint and the client-owned trust_server_idempotency_hints opt-in: annotations are server-supplied and untrusted by default, so a misannotated server must not get a non-idempotent call executed twice on its own say-so. Any other tool raises ToolExecutionException stating the connection was lost and the remote execution outcome is unknown, with the connection error as the inner exception. The gate runs after the reconnect attempt, so a caller-owned connection still fails with its existing 'cannot be recreated' error and never reaches the ambiguous-outcome path. tools/list, get_prompt, and the long-running task lifecycle keep their current retry behavior. Tests: test_mcp.py covers a hinted tool replaying once after reconnect, an unhinted tool raising without a replay, a hinted tool without the client opt-in raising without a replay, a lost-response call executing at most once, and the annotations snapshot rolling back when discovery fails after load_tools. The three no-replay cases go red when the gate is disabled. Full test_mcp.py: 467 passed, 2 skipped. poe syntax -P core and poe pyright -P core are clean.
b5c2244 to
254b89b
Compare
GitHub Actions
Active
GitHub Actions
Active
|
Rebuilt onto current main as a single commit and tightened the gate ordering while resolving the conflicts. Two things changed under review of the rebase. The session-fixtures conflict showed that the caller-owned reconnect test pins a "caller-owned and cannot be recreated" error and a no-reconnect, no-replay outcome for exactly the ambiguous call this PR gates. The gate now runs after the reconnect attempt instead of before it, so that contract stays intact: a caller-owned connection still fails with its existing error, and a framework-owned one is back up when the unknown-outcome exception surfaces. Replay still requires both the server hint and the trust_server_idempotency_hints opt-in, so the non-idempotent path never re-issues. Verification on this base: full packages/core/tests/core/test_mcp.py 467 passed 2 skipped, poe syntax -P core clean, poe pyright -P core 0 errors. The three no-replay cases (unhinted tool, hinted tool without client trust, lost-response side effect) go red when the gate is disabled. |
The connection-reset integration test marks the query tool read-only before the simulated drop so the lost response is safe to replay, but the replay also needs the client-owned trust opt-in, which the default posture withholds. Without it the second invocation takes the unknown-outcome path and never reaches the reconnection the test is about. Opt the tool into trust_server_idempotency_hints for that test only, keeping the read-only hint it already sets.
GitHub Actions
Active
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_retriesgates the replay on the tool's advertisedToolAnnotations(captured fromtools/listinto a new_tool_annotations_by_namesnapshot alongside the existing meta/task-support maps). Replay requires both a serveridempotentHintorreadOnlyHintand the client-ownedtrust_server_idempotency_hintsopt-in. Annotations are server-supplied and untrusted by default, so a hinted tool with the default posture still gets no replay. Any other tool raisesToolExecutionExceptionstating that the connection was lost and the remote execution outcome is unknown, with the connection error as the inner exception. The gate runs after the reconnect attempt: a caller-owned connection keeps failing with its existing "cannot be recreated" error before the ambiguous-outcome path, and a framework-owned connection is back up when the exception surfaces, so the caller decides whether to retry explicitly.tools/list,get_prompt, and the long-running task lifecycle keep their current retry behavior._call_tool_with_retriesand whetheridempotentHint/readOnlyHintbehind a client opt-in is 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 on the current base: full
packages/core/tests/core/test_mcp.py(467 passed, 2 skipped),poe syntax -P core(fmt + lint clean),poe pyright -P core(0 errors). The three no-replay cases go red when the gate is disabled.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.