fix(mcp): preserve embedded resource text in tool results (#30601)

[Tranquil-Flow]

What

MCP tool results containing EmbeddedResource (resource-type) content blocks are silently dropped because the handler only checks block.text and image cache. For EmbeddedResource, the model-facing text lives at block.resource.text, which was never unwrapped — producing empty results for any MCP server that returns resource blocks (e.g., qmd's get tool).

Fix

Extracted _extract_mcp_text_block() — a duck-typed helper that extracts text from both:

• Plain TextContent blocks (block.text)
• EmbeddedResource blocks (block.resource.text)

Used in both the error path and the success path of _make_tool_handler, replacing the old inline hasattr(block, "text") checks.

Test

• 2 new regression tests in tests/tools/test_mcp_structured_content.py:
  • test_embedded_resource_text_result — normal resource block surfaces text
  • test_error_embedded_resource_text_result — error resource block surfaces error text
• Full MCP test suite: 202/202 passing (195 existing + 7 structured content)
• Fail-without-fix confirmed: reverting the production change causes both new tests to fail with empty results

Files changed

• tools/mcp_tool.py — +20 lines helper, −4/+5 lines at call sites (net +21)
• tests/tools/test_mcp_structured_content.py — +29 lines (2 tests + mock)

Verification

python3 -m pytest tests/tools/test_mcp_tool.py tests/tools/test_mcp_structured_content.py -v -o addopts= --tb=short
# 202 passed in 15.22s

Closes #30601

[alt-glitch]

Duplicate of #13736 — same fix for MCP EmbeddedResource text extraction in mcp_tool.py. Prior duplicates: #29962, #18118, #3655. The canonical open PR is #13736 (which also fixes #30601).

[Bartok9]

I rechecked this against current origin/main (b207dc28b). The gap is still present: _make_tool_handler (lines 2339 and 2360) only reads block.text via hasattr, which is False for EmbeddedResource blocks where the text lives at block.resource.text.

The _extract_mcp_text_block helper in this PR correctly duck-types both shapes, and the two regression tests (test_embedded_resource_text_result, test_error_embedded_resource_text_result) reproduce the drop. Fix looks clean and the approach is consistent with the existing image-block handling pattern added in #17915.

[Tranquil-Flow]

Thanks for the review @Bartok9 and thanks for flagging the duplicate @alt-glitch.

I compared #31356 against #13736 (jpterry, the canonical PR). #13736 is CONFLICTING with current main and only fixes the success path — the error path (isError=True) still silently drops EmbeddedResource text. Our PR fixes both paths.

Revision (force-push): Added blob/binary resource handling to _extract_mcp_text_block — when a resource block carries .blob instead of .text, we now return a human-readable [binary resource: {uri}] placeholder. This matches jpterry's blob coverage without the inline complexity:

# Before (text only)
resource_text = getattr(resource, "text", None)
if resource_text:
    return resource_text
return ""

# After (text + blob)
resource_text = getattr(resource, "text", None)
if resource_text:
    return resource_text
resource_blob = getattr(resource, "blob", None)
if resource_blob:
    uri = getattr(resource, "uri", "unknown")
    return f"[binary resource: {uri}]"
return ""

Summary of advantages over #13736:

	#13736	#31356
Error path	❌	✅
Approach	Inline elif hasattr	Extracted helper (reusable)
Test file	test_mcp_tool.py	test_mcp_structured_content.py (correct)
Merge conflicts	CONFLICTING	MERGEABLE

203/203 MCP tests passing. Ready for maintainer review.

[Morad37]

Nice catch. The _extract_mcp_text_block helper is a clean abstraction -- TextContent at block.text and EmbeddedResource at block.resource.text handled uniformly, with a sensible fallback for binary blobs. I use MCP tools daily and this kind of silent content drop (resource-backed results returning empty) is exactly the sort of bug that's hard to notice but causes real confusion downstream.

The duck-typed approach (getattr instead of isinstance) is practical too -- avoids coupling to specific MCP SDK class imports.

[alt-glitch]

This was generated by AI during triage.

Not a duplicate. The cited canonical #13736 is CLOSED and unmerged, and the gap it targeted is still present on current main (b207dc28b) — confirmed by @Bartok9 and the PR author. This PR (#31356) is the mergeable canonical fix: it covers both the success and error paths (via the extracted _extract_mcp_text_block helper) whereas #13736 was conflicting and success-path only. Keeping open as the cluster anchor; related_to #13736 (closed predecessor), #38463 / #50999 / #29962 (open siblings), and issue #30601.

[Bartok9]

Thanks for the triage confirmation @alt-glitch — agreed on all points. To recap the state for anyone landing here:

• Predecessor fix(mcp): extract text from EmbeddedResource content blocks in tool results #13736 is closed/unmerged and was success-path only + conflicting with main.
• fix(mcp): preserve embedded resource text in tool results (#30601) #31356 remains MERGEABLE against b207dc28b and covers both the success and error (isError=True) paths via the extracted _extract_mcp_text_block helper, plus blob/binary resource placeholders.
• 203/203 MCP tests passing.

Happy to rebase if main moves before merge. Ready for maintainer review as the cluster anchor.

[teknium1]

Thanks for the focused MCP regression fix. Current main still has the reported omission: tools/mcp_tool.py:3947-3956 reads only block.text for error results, and tools/mcp_tool.py:3970-3977 does the same for successful results before image handling. The PR's shared extractor covers both paths and the added cases in tests/tools/test_mcp_structured_content.py cover normal resource text, error resource text, and blob fallback.

Automated hermes-sweeper review.

[Bartok9]

Thanks @teknium1 — confirmed. The shared _extract_mcp_text_block helper replaces the block.text-only reads at both tools/mcp_tool.py:3947-3956 (error path) and 3970-3977 (success path), and the three new cases in tests/tools/test_mcp_structured_content.py (normal resource text, error resource text, blob fallback) lock the regression. Still MERGEABLE against b207dc28b, 203/203 MCP tests green. Ready to merge — happy to rebase if main moves first.

[teknium1]

This is now fixed on main via PR #64556 (salvaged from #64061, which materializes binary blobs to the document cache and covers ResourceLink/Audio in addition to embedded resource text). Your fix addressed the same underlying bug — thanks for the contribution, and sorry we couldn't take this one; five PRs ended up targeting the same dropped-resource-block issue.

[Bartok9]

No worries at all @teknium1 — glad the underlying dropped-resource-block bug is fixed on main via #64556. Thanks for the thorough triage and for covering the broader ResourceLink/Audio/blob cases. Happy the cluster landed. 🙌