feat(sdk): preserve structured MCP result data

[georgeglarson]

HUMAN:

Human verified. See screenshot.

---

AGENT:

Why

MCP tool results can carry structuredContent and result _meta alongside their content blocks. MCPToolObservation.from_call_tool_result currently keeps only text/image blocks, is_error, and tool_name, so both fields are silently discarded at the conversion boundary. Once dropped there, no downstream consumer (event persistence, websocket delivery, protocol-aware clients such as Canvas) can recover them.

Reproduced on main (9d143aac3): converting a real mcp.types.CallToolResult that carries structuredContent and _meta yields an observation with neither field.

Summary

• MCPToolObservation gains two optional fields, structured_content and result_meta, copied from the CallToolResult at conversion. Both default to None, so events serialized before this change still load; a fixture serialized by SDK 1.44.1 covers that path.
• The fields serialize with the observation for protocol-aware clients but stay out of to_llm_content, so they do not reach the model.
• The existing secret-masking step now walks both fields recursively: registered secrets are masked in nested keys and values, including secrets echoed as JSON numbers. A field that cannot be masked safely (two keys colliding after masking, for example) is dropped rather than returned unmasked, and one field's masking failure no longer drops its sibling.

Issue Number

Closes #4776

How to Test

uv run pytest tests/sdk/mcp: 142 passed on this branch, covering conversion, recursive masking, masking failure paths, key-collision handling, numeric secrets, and old-event loading.

Live run against a real MCP server (FastMCP, HTTP transport): a tool returning a dict payload with two registered secrets, one nested in a value, one in key position. After the executor's masking step:

STRUCTURED {'customer_id': '<secret-hidden>', 'credentials': {'api_key': '<secret-hidden>'}, 'raw-<secret-hidden>': 'key-position secret', 'open_invoices': 2}
RAW_SECRET_IN_SERIALIZED False

The payload's text rendering still reaches the model through the normal content path, masked there as before.

A full agent-loop run with a real LLM (glm-5.3 over an OpenAI-compatible endpoint): real LocalConversation, the secret registered through conversation.update_secrets, and a real MCP server over stdio. The agent called the tool once and answered correctly from the masked text; the persisted event stream carries the structured data masked, and the model's message stream never contained the raw secret:

STRUCTURED {'customer_id': 'C-100', 'credentials': {'api_key': '<secret-hidden>'}, 'open_invoices': 2}
RAW_SECRET_IN_EVENTS False
RAW_SECRET_IN_LLM_MESSAGES False

Video/Screenshots

Design Doc

Not included; the change is four files and covered above.

Type

• Bug fix

Notes

Companion docs PR: OpenHands/docs#771 (sdk/arch/mcp.mdx).

Heads-up for hook consumers: PostToolUse payloads built from event.observation.model_dump() now include the two new fields.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 2960e16e2ca9065c98c1049cdb8db35d9d6bda7c
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/8562170b-1134-437a-b772-eca4d44edbac

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

🟢 Good taste — clean, additive change that preserves previously-discarded MCP protocol fields without altering the model-visible path.

The PR adds structured_content and result_meta to MCPToolObservation, both defaulting to None so old events still load (verified by the hardcoded SDK 1.44.1 fixture). The fields serialize for protocol-aware clients but stay out of to_llm_content, so the model sees exactly the same content as before. Secret masking is extended recursively into both new fields with a fail-closed policy (drop the field on any masking error), while text content retains its existing fail-open behavior. Key-collision detection, scalar/numeric secret masking, and per-field isolation (one field's failure doesn't drop its sibling) are all well-tested.

Findings

No material issues found on the changed lines. The backward-compatibility path is correct (additive fields with defaults on an extra="forbid" model need no deprecation handler), the security approach is sound, and test coverage is thorough — conversion, recursive masking, failure paths, key collisions, numeric secrets, and legacy event loading are all covered.

Eval / Benchmark Risk

This PR modifies tool execution code (_mask_observation, from_call_tool_result), which falls in the eval-risk category per the review guide. However, the model-visible path (to_llm_content) is unchanged — the agent reasons over the same text content as before, and the text masking success path is identical to main. The new fields are client-visible only. I assess the eval risk as low, but since no eval monitor evidence is linked in the PR description, I'm leaving a COMMENT rather than APPROVE and flagging for a human maintainer to confirm with a lightweight eval if deemed necessary.

All CI checks pass on the current head SHA, including sdk-tests, tools-tests, integration-test, and coverage-report.

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW — additive, backward-compatible, model-visible path unchanged, well-tested, all CI green.

VERDICT: Worth merging. Flagging for human maintainer eval confirmation per the tool-execution review policy.

KEY INSIGHT: The model-visible observation path is untouched; only protocol-aware clients gain the two new fields, and secret masking is extended to cover them fail-closed.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it is merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.