feat(mcp): promote content-to-tool-result method to public API

[gautamsirdeshmukh]

Description

Renames MCPClient._map_mcp_content_to_tool_result_content to map_mcp_content_to_tool_result_content, making subclass override an officially supported extension pattern for intercepting or transforming MCP content blocks before they reach the model.

(Also maintains the underscored version with backwards compatibility with a DeprecationWarning)

Related Issues

Closes #2251

Type of Change

New feature

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli

• I ran hatch run prepare, added a test

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Assessment: Request Changes

The core idea — promoting the private mapping method to a public extension point — is well-motivated and the implementation is clean for the simple case. However, there's a backwards compatibility gap that needs to be addressed before merge.

Review Details

• Backwards Compatibility (Critical): Existing subclasses that override _map_mcp_content_to_tool_result_content will be silently broken — their override stops being invoked because the internal call site now dispatches through the new public method name. The deprecation wrapper only triggers on explicit calls, not on overrides. This needs a runtime detection mechanism to bridge the transition (see inline comment for a concrete suggestion).

• API Review Label: This PR promotes a private method to a public, documented extension point. Per project guidelines, it should carry the needs-api-review label since it establishes a new public API contract that downstream users will depend on.

The scope and tests are appropriate for the change — just the compat bridge needs to be added.

[agent-of-mkmeral]

TL;DR: Right direction, but the deprecation shim is inverted — existing subclasses that override the old _map_... name will silently stop being invoked (and won't even get a warning). Agreeing with @opieter-aws's CHANGES_REQUESTED. Fix + missing test below. Also worth a needs-api-review label since this promotes a private method to a public contract.

🔴 The deprecation bug (blocking)

The call site now dispatches through the new public name, and the old name is kept as a delegating wrapper:

# call site:
mc := self.map_mcp_content_to_tool_result_content(content)

# old name delegates forward:
def _map_mcp_content_to_tool_result_content(self, content):
    warnings.warn(...)
    return self.map_mcp_content_to_tool_result_content(content)

Today the only extension point is overriding _map_.... After this PR, an existing subclass that overrides _map_... has its override never called — the call site goes through map_.... And because the wrapper itself is what got overridden, the DeprecationWarning never fires either. Silent behavior change, zero signal — the worst kind of break. The warning only triggers on an explicit obj._map_...() call, which nobody does (the method is meant to be overridden, not called).

✅ Suggested fix — runtime override detection

def map_mcp_content_to_tool_result_content(self, content):
    cls = type(self)
    if (
        cls.map_mcp_content_to_tool_result_content is MCPClient.map_mcp_content_to_tool_result_content
        and cls._map_mcp_content_to_tool_result_content is not MCPClient._map_mcp_content_to_tool_result_content
    ):
        warnings.warn(
            "Overriding _map_mcp_content_to_tool_result_content is deprecated. "
            "Override map_mcp_content_to_tool_result_content instead.",
            DeprecationWarning, stacklevel=2,
        )
        return cls._map_mcp_content_to_tool_result_content(self, content)
    # ... real mapping logic ...

This keeps legacy overrides working and emits a migration warning during the deprecation window.

🧪 Missing test coverage

The two new tests don't cover the actual breakage:

• test_map_mcp_content_subclass_override only overrides the new name (trivially works).
• test_deprecated_underscore_method_delegates_with_warning only tests an explicit call.

Neither tests the realistic case: a subclass that overrides the OLD name still gets invoked (with a warning). That's exactly where the bug lives — please add it.

💡 Bigger-picture (non-blocking)

• API review: promoting a private method to a documented public contract is a one-way door — suggest adding needs-api-review rather than landing as size/s.
• Mechanism: subclassing is fine for the feat(mcp): document/promote MCPClient._map_mcp_content_to_tool_result_content as a public extension point #2251 use case but is the least composable option (can't stack transforms). The SDK already has AfterToolCallEvent (writable result) which overlaps at the agent layer — worth documenting when to use which (MCP-client method = per-content-block, pre-mapping; hook = post-mapping, composable). An injectable ContentMapper (Option B in feat(mcp): document/promote MCPClient._map_mcp_content_to_tool_result_content as a public extension point #2251) would be the cleaner v2 if this grows.

[gautamsirdeshmukh]

Still discussing offline about the right direction to go in terms of when to make the breaking change

Discussed offline, team is aligned with the breaking change of making this public without maintaining the deprecated method

[github-actions]

Assessment: Request Changes

Agreeing with the existing feedback from @opieter-aws and @agent-of-mkmeral — the backwards compatibility gap for existing subclass overrides of _map_mcp_content_to_tool_result_content is the blocking issue.

Additional observations on the latest revision

• Deprecation shim removed: The current diff is now a pure rename with no backwards compatibility bridge at all. This simplifies the code but means existing override subclasses will break silently with zero warning. Whatever approach is decided (runtime detection, keeping the shim, or accepting the break with a changelog note), it should be explicit.

• Extra blank line (line 950-951): The diff introduces a double blank line between map_mcp_content_to_tool_result_content and _log_debug_with_thread where the rest of the class uses single blank lines between methods. Minor formatting inconsistency that ruff may flag.

• needs-api-review label: Still applicable — this establishes a new public API contract for subclassing.

The direction and scope are good — just waiting on the backwards compat decision.