Python: fix(mcp): do not duplicate structuredContent when content present - #8274
Conversation
Fixes microsoft#7866. Prefer content blocks for model-visible tool results; structuredContent remains on the Host payload. Serialize structuredContent only when content is empty.
|
Fork CI for this PR may need a maintainer to Approve workflows before Python tests run (same as other community PRs). Happy to address review feedback once checks appear. |
There was a problem hiding this comment.
🟡 Changes recommended
Existing test expectations still require the old duplicate-output behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes duplicate MCP tool output when both content and structuredContent are present.
Changes:
- Use
structuredContentas model-visible content only when regular content is empty. - Preserve the complete structured payload for host transports.
- Update parser regression coverage.
File summaries
| File | Summary |
|---|---|
python/packages/core/agent_framework/_mcp.py |
Prevents duplicate structured content. Moderate finding (3 votes): remaining test expectations must be updated. |
python/packages/core/tests/core/test_mcp.py |
Updates structured-content parsing expectations; several existing expectations still require the old duplicate behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Address Copilot review on microsoft#8274: update suites that still expected the old duplicated model-visible content shape.
|
Follow-up for Copilot review: aligned the three suites that still expected duplicated |
|
CI is green on this branch. The scoped MCP change (skip dumping Happy to help with any follow-up nits if maintainers want a second look. |
Ricky-7-Yan
left a comment
There was a problem hiding this comment.
I don't think this should hard-code content-first behavior yet.
The issue discussion identified two distinct cases: servers that duplicate the same value in content and structuredContent, and servers where the two fields are complementary. This implementation fixes the former by dropping structuredContent whenever any content block exists, but it also silently removes complementary structured data from the model-visible result.
That is the ambiguity maintainers called out in #7866, where the direction discussed was an explicit selection policy (structured first, content first, content only, structured only, or both), likely with a documented default. Could this expose that policy instead of choosing content-first globally? The tests should cover structured-only, content-only, equivalent-both, complementary-both, and retention of the full payload in HostMessageContent.
Replace hard-coded content-first behavior with structured_first/content_first/content_only/structured_only/both, defaulting to structured_first per microsoft#7866 discussion.
|
Ricky-7-Yan Addressed on the latest commit: replaced the hard-coded content-first behavior with an explicit
This matches the selection options discussed on #7866. Host payload retention is unchanged. Tests cover structured-only, content-only, either-first, complementary both, and the null sentinel. |
|
Ricky-7-Yan Eduard van Valkenburg (@eavanvalkenburg) Updated the PR body to the repository template. The |
|
Ricky-7-Yan LI (@ktz03) Thanks for landing the explicit I checked the branch locally against the #7866 cases. The five modes + complementary-both coverage look good. Two small follow-ups that would fully match the review ask:
I have those two tests ready locally on top of this PR head. Happy to open a tiny follow-up PR into Please re-review when convenient; the selection-policy direction looks right. |
|
Follow-up test coverage for the review ask is ready here: ktz03#1 It adds equivalent-both and Host payload retention assertions for every |
|
Thanks for the contribution. The approval-gated workflows completed, but the current head is not green: |
Add undocumented-param docstring entries on MCP transport constructors and assert tool result text before json.loads.
|
Addressed on
Workflow runs for this head are currently waiting on approval-gated Actions. |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Reviewed the complete diff against main; no blocking issues found. All current checks are green.
Motivation & Context
MCP
CallToolResultmay include bothcontentandstructuredContent. Many servers duplicate the same payload in both fields, which doubles model-visible tokens (#7866). Servers may also use the fields as complementary data. Maintainers asked for an explicit selection policy rather than a hard-coded preference.Fixes #7866.
Description & Review Guide
tool_result_contentonMCPTool/ transport subclasses:structured_first(default),content_first,content_only,structured_only,both. Host payload retention is unchanged. Customparse_tool_resultsstill overrides._parse_tool_result_from_mcpselection modes and the new unit coverage.Related Issue
Fixes #7866
No other open PR targets this issue.
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.