fix: preserve MCP tool metadata across transport#1210
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1210 |
| HEAD checked | 261fd0219a91ecf3beb431df56dacdfa59284922 |
| Request ID | req_retry_exhausted_new_timeout_1779693693_1210 |
| Review record | d917bb7a-38f2-4bd7-8a78-824a27134e77 |
What Improved
- Preserves MCP tool-result metadata across both the FastMCP server boundary and the MCP client adapter parse path.
- Adds focused unit coverage for metadata preservation on server and client adapters.
Issue Requirements
| Requirement | Status |
|---|---|
| No linked issue requirement captured | N/A |
Prior Findings Status
No prior human or inline review concerns were present in the supplied PR comment artifacts.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | src/ouroboros/mcp/tools/subagent.py:198 | Low | Medium | Existing comments still describe FastMCP metadata as dropped. They are now stale after this PR and should be updated in a follow-up to avoid misleading future changes. |
Non-blocking Suggestions
| 1 | src/ouroboros/mcp/tools/evaluation_handlers.py:1469 | Documentation | Existing inline documentation says adapter metadata is dropped; this should be refreshed now that _to_fastmcp_tool_result() forwards _meta. |
Test Coverage Notes
- Reviewed added unit tests in
tests/unit/mcp/client/test_adapter.pyandtests/unit/mcp/server/test_adapter.py. - Could not run pytest because this environment has no
pytestmodule installed. - Ran
python3 -m py_compileon the changed source and test files successfully.
Design Notes
The change is well-scoped to MCP adapter translation boundaries. It keeps handlers thin and preserves internal MCPToolResult.meta without moving business logic into the server wrapper.
Design / Roadmap Gate
Affected boundary is MCP tool-result serialization/deserialization. Server-side MCPToolResult.meta is now mapped to SDK CallToolResult._meta, and client-side SDK meta is restored into internal MCPToolResult.meta. No persistence, replay, auth, or state-contract regressions were found in the changed paths.
Directional Notes
Maintainer memory emphasized keeping MCP handlers as thin unwrap/delegate/wrap layers; this PR follows that shape. I also checked the locked MCP SDK contract: direct CallToolResult returns with _meta are supported by FastMCP.
Test Coverage
- Reviewed added unit tests in
tests/unit/mcp/client/test_adapter.pyandtests/unit/mcp/server/test_adapter.py. - Could not run pytest because this environment has no
pytestmodule installed. - Ran
python3 -m py_compileon the changed source and test files successfully.
Merge Recommendation
Approve. No blocking runtime, API contract, or test-coverage issues were found in the current HEAD snapshot.
Review-Metadata:
verdict: APPROVE
head_sha: 261fd02
request_id: req_retry_exhausted_new_timeout_1779693693_1210
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Summary
Original PR blocker coverage
Addresses the MCP interview reasoning transport blocker reported on #1140.
Verification