Preserve content-level annotations through vMCP relay#4336
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
==========================================
- Coverage 69.08% 69.02% -0.06%
==========================================
Files 478 478
Lines 48432 48552 +120
==========================================
+ Hits 33457 33513 +56
- Misses 12314 12319 +5
- Partials 2661 2720 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yrobla
left a comment
There was a problem hiding this comment.
Nice fix overall — the ACL pattern is correctly applied and all five content type branches are updated. A couple of things worth addressing before merge.
Important: Two related issues in the annotation converters (pointer aliasing + asymmetric nil-elision). Test gaps: blob embedded resource path is untested, and TestToMCPAnnotations is thin.
|
Review priority summary (following up on the inline comments above): Fix in this PR:
Fine as follow-ups:
|
MCP content items (text, image, audio, embedded resource, resource link) can carry per-content annotations (audience, priority, lastModified) that inform how clients display results. The vMCP relay layer was silently dropping these annotations during the mcp->vmcp->mcp conversion round trip. Add ContentAnnotations domain type following the existing Anti-Corruption Layer pattern, and update ConvertMCPContent/ToMCPContent to preserve annotations for all five content types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Deep-copy Priority pointer in ConvertMCPAnnotations and ToMCPAnnotations to prevent pointer aliasing across the ACL boundary - Add zero-value guard in ToMCPAnnotations for symmetry with ConvertMCPAnnotations (empty non-nil input now returns nil) - Document Priority range constraint [0.0, 1.0] and valid Audience values per MCP spec - Add test case for empty non-nil ContentAnnotations - Add blob embedded resource test case for round-trip verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2646a7b to
0fa964a
Compare
Break long lines exceeding 130-char limit in ConvertMCPContent and fix gci formatting alignment in content_test.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jerm-dro
left a comment
There was a problem hiding this comment.
The implementation is correct and well-tested. Round-trip coverage for all content types, priority-zero edge case handled, nil coalescing is sound.
question: Stepping back — this is the third consecutive PR fixing silent data loss in the vmcp conversion layer (#4334 prompts, #4335 resources, #4336 annotations). Each time mcp-go gains a field, the relay silently drops it until someone adds the conversion + round-trip tests in 4 places (vmcp type, ConvertMCP*, ToMCP*, tests).
The vmcp types (Content, ContentAnnotations, ResourceContent, PromptMessage) are 1:1 mirrors of their mcp-go counterparts, and 38 files in pkg/vmcp/ already import mcp-go/mcp directly — so the ACL boundary the conversion layer is meant to provide doesn't really exist in practice.
Would it be simpler to use mcp-go types directly for protocol pass-through data (content, annotations, resource contents, prompt messages) and reserve vmcp-specific types for things that genuinely have no mcp-go equivalent (BackendTarget, RoutingTable, etc.)? That would eliminate the conversion code for these types and — more importantly — make silent data loss structurally impossible for pass-through fields.
Not blocking this PR, but curious what @JAORMX and @yrobla think about whether the conversion layer is pulling its weight vs. producing bugs.
Generated with Claude Code
Summary
The vMCP relay was silently dropping per-content
Annotations(audience, priority, lastModified) during the mcp→vmcp→mcp conversion. The MCP spec defines these annotations on every content type (text, image, audio, embedded resource, resource link), but the conversion layer was not carrying them through.This adds
ContentAnnotationsto the vmcp domain model and threads annotation preservation through all content type conversions, ensuring annotations survive the full relay round-trip.ContentAnnotationstype (Audience, Priority, LastModified) tovmcp.typesAnnotations *ContentAnnotationsfield tovmcp.ContentConvertMCPAnnotations/ToMCPAnnotationsconverters for content-level annotationsConvertMCPContentandToMCPContentto preserve annotations on all content types (text, image, audio, embedded resource, resource link)Type of change
Test plan
task test)task lint-fix)Does this introduce a user-facing change?
Content annotations (audience hints, priority, timestamps) from backend MCP servers are now preserved through virtual MCP server responses, enabling AI clients to respect content targeting and ordering hints.
Special notes for reviewers
This is one of three related PRs fixing vMCP relay fidelity:
The
ContentAnnotationstype follows the existing Anti-Corruption Layer pattern — vmcp types use[]stringfor audience rather than[]mcp.Role, keeping the domain model decoupled from mcp-go.Generated with Claude Code