Python: Preserve bounded MCP Host payload metadata - #8128
Python: Preserve bounded MCP Host payload metadata#8128Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
Code Coverage OverviewLanguages: Python Python / code-coverage/pythonThe overall line coverage in commit 5aebc2d in the Show a line coverage summary of the most covered files.
|
There was a problem hiding this comment.
🟡 Changes recommended
Oversized errors lose metadata, custom function parsers discard markers, and error capture is not limited to generated calls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds bounded, Host-only MCP result metadata capture while preserving model-facing output.
Changes:
- Adds scoped, size-limited Host payload markers.
- Preserves markers through task and error paths.
- Adds payload, transport, and compatibility tests.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_mcp.py |
Implements MCP Host payload capture and propagation. |
python/packages/core/agent_framework/_tools.py |
Transfers exception metadata into function results. |
python/packages/core/tests/core/test_mcp.py |
Adds MCP payload regression coverage. |
Review details
Suppressed comments (2)
python/packages/core/agent_framework/_mcp.py:2619
- The legacy/fallback error path also captures a complete Host payload for direct
call_tool_as_task()calls even thoughpreserve_host_payloadis false. Apply the generated-call guard here as well so fallback behavior remains scoped consistently.
_mcp_tool_result_host_payload(
fallback_result,
max_size_bytes=self.max_host_payload_size_bytes,
),
python/packages/core/agent_framework/_mcp.py:2836
- Completed-task errors likewise serialize and retain the Host payload when the public direct task API requested no preservation. Gate this capture on
preserve_host_payloadto keep all task paths generated-call-only.
_mcp_tool_result_host_payload(
payload,
max_size_bytes=self.max_host_payload_size_bytes,
),
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if host_payload is not None: | ||
| self._function_result_additional_properties[_MCP_TOOL_RESULT_HOST_PAYLOAD_KEY] = host_payload | ||
| if isinstance(meta := host_payload.get("_meta"), Mapping): | ||
| self._function_result_additional_properties["_meta"] = dict(cast(Mapping[str, Any], meta)) |
| _mcp_tool_result_host_payload( | ||
| result, | ||
| max_size_bytes=self.max_host_payload_size_bytes, | ||
| ), |
| _with_mcp_tool_result_host_payload( | ||
| parsed, | ||
| result, | ||
| max_size_bytes=self.max_host_payload_size_bytes, | ||
| ) |
| with pytest.raises(_MCPToolResultException) as exc_info: | ||
| await _call_generated_mcp_tool(tool, "widget") | ||
|
|
||
| function_result = _function_execution_error_result( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 5aebc2d700f4
Model: gpt-5.6-sol-fast
Overview
The PR cleanly scopes successful Host-payload capture to generated MCP functions, uses early and final serialization-size guards, and covers normal, error, fallback, and completed-task paths with targeted tests. The residual risks are that the per-result cap is not an aggregate bound, raw _meta can bypass it, oversized errors lose metadata, required-task dispatch bypasses public overrides, and an empty custom parser result changes the model-facing projection.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
5 verified findings remained after source verification (5 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_mcp.py
| def __init__(self, message: str, host_payload: dict[str, Any] | None) -> None: | ||
| super().__init__(message) | ||
| self._function_result_additional_properties: dict[str, Any] = {} | ||
| if host_payload is not None: |
There was a problem hiding this comment.
When an isError result exceeds the configured payload limit, _mcp_tool_result_host_payload() returns None, so this branch drops _meta along with the optional Host marker. Oversized generated errors therefore lose server metadata that the success path deliberately preserves for downstream consumers. Please carry a bounded copy of _meta independently of whether the complete Host payload fits.
| updated_item = copy(item) | ||
| updated_item.additional_properties = dict(updated_item.additional_properties) | ||
| updated_item.additional_properties.pop(_MCP_TOOL_RESULT_HOST_PAYLOAD_KEY, None) | ||
| if meta is not None: |
There was a problem hiding this comment.
When _meta itself makes the complete result exceed max_host_payload_size_bytes, the Host marker is omitted but this unbounded server-controlled metadata is still copied onto every parsed item and can be persisted in response history. This lets a large _meta bypass the retention bound even with a bounded custom parser. Please bound _meta independently while continuing to preserve metadata that fits.
| dumped = model_dump(by_alias=True, exclude_none=True) | ||
| if not isinstance(dumped, Mapping): | ||
| return None | ||
| host_payload = cast(dict[str, Any], make_json_safe(dict(cast(Mapping[str, Any], dumped)))) |
There was a problem hiding this comment.
This per-result copy can retain nearly 1 MiB for every generated MCP call, while the function loop launches the entire model-provided batch concurrently and only records max_function_calls after that batch completes. A sufficiently large single turn can therefore multiply the nominal cap into request-level memory exhaustion. Please enforce a pre-execution batch/concurrency bound or charge retained payloads against an aggregate request budget.
| # route through the long-running task lifecycle transparently. | ||
| if self._tool_task_support_by_name.get(tool_name) == "required": | ||
| return await self.call_tool_as_task(tool_name, **kwargs) | ||
| return await self._call_tool_as_task(tool_name, kwargs, preserve_host_payload=preserve_host_payload) |
There was a problem hiding this comment.
For tools advertising required task support, this now bypasses the public call_tool_as_task() method that call_tool() previously dispatched through. Existing MCPTool subclasses that override that lifecycle method for authorization, telemetry, retries, or transport-specific handling will silently stop seeing transparent required-task calls. Please preserve dispatch through the public override while propagating the generated-call capture scope.
| ) -> list[Content]: | ||
| """Attach the complete MCP result once without changing the model projection.""" | ||
| items = [Content.from_text(parsed)] if isinstance(parsed, str) else list(parsed) | ||
| if not items: |
There was a problem hiding this comment.
A custom parse_tool_results callback may intentionally return an empty list[Content]; this converts that valid projection into model-facing text "[]", whereas the function-result layer previously represented it as empty output. Please preserve the parser's empty projection and carry the Host payload through a non-model-visible carrier instead.
| if index == 0 and host_payload is not None: | ||
| updated_item.additional_properties[_MCP_TOOL_RESULT_HOST_PAYLOAD_KEY] = host_payload |
There was a problem hiding this comment.
Could we keep the Host marker outside middleware-rewritable model items, or explicitly transfer it when an item is hidden? SecureMCPToolProxy labels MCP output as untrusted, and the default LabelTrackingFunctionMiddleware(auto_hide_untrusted=True) replaces this first item with a new variable-reference Content; _hide_item() stores only item.text, so _mcp_tool_result_host_payload is gone before Content.from_function_result() runs. As a result, security-enabled MCP Apps still lose structuredContent, isError, and _meta on the Host/history path this contract is meant to preserve.
Motivation & Context
Generated MCP
FunctionToolcalls need to retain the completeCallToolResultfor Host transports even when the model-facing parser intentionally projects only a bounded summary. Without a core-owned capture contract, fields such asstructuredContentare unavailable to downstream transports and persisted history after tool execution.This is layer 1 (the bottom layer) of the split replacement for #7971. It establishes only the core MCP Host-payload capture contract and remains orthogonal to #7897: it does not change which MCP content the model sees, or its ordering, preference, or deduplication.
Description & Review Guide
ContextVar, attach one JSON-safe complete result under a private core marker after built-in or custom parsing, preserve_metaon every model-facing item, and carry the marker through generic function-error conversion. Normal, long-running fallback, completed-task, and MCP error paths share the contract. A configurable per-result size cap rejects oversized payloads before full-copy materialization and retains an exact final serialization guard;Nonedisables the cap. The option is keyword-only onMCPToolafter all existing positional parameters and is forwarded by every concrete transport.call_tool()custom-parser return shapes, model-visible content, transport subclass behavior, or header-provider behavior. Oversized Host payloads are omitted while bounded model projections and_metaremain intact.Related Issue
Part of #7959.
This draft is the core-only first layer replacing the broader open source PR #7971. Draft #7897 addresses model-facing MCP content selection and is intentionally independent from this Host-only metadata contract.
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.