-
Notifications
You must be signed in to change notification settings - Fork 170
Preserve _meta fields in vMCP backend responses #3354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3354 +/- ##
==========================================
+ Coverage 64.84% 65.12% +0.27%
==========================================
Files 375 377 +2
Lines 36603 36715 +112
==========================================
+ Hits 23737 23910 +173
+ Misses 10995 10927 -68
- Partials 1871 1878 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for preserving _meta fields from backend MCP server responses in the vMCP architecture. Previously, protocol-level metadata (progress tokens, trace IDs, custom backend metadata) was being discarded when forwarding responses to clients.
Changes:
- Modified BackendClient interface to return wrapper types (ToolCallResult, ResourceReadResult, PromptGetResult) that include _meta fields
- Updated client implementation to extract and preserve _meta from backend responses
- Added conversion functions to map between vmcp and mcp Meta/Content types
- Updated all mocks and tests to work with new return types
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/types.go | Added new wrapper types (Content, ToolCallResult, ResourceReadResult, PromptGetResult) with Meta fields |
| pkg/vmcp/server/telemetry.go | Updated telemetry wrapper method signatures to use new return types |
| pkg/vmcp/server/adapter/handler_factory.go | Added conversion functions and updated handlers to preserve _meta when forwarding to clients |
| pkg/vmcp/mocks/mock_backend_client.go | Updated mock implementations for new method signatures |
| pkg/vmcp/client/client.go | Modified CallTool, ReadResource, GetPrompt to extract and preserve _meta from backend responses |
| pkg/vmcp/client/meta_integration_test.go | New integration test validating _meta preservation end-to-end |
| pkg/vmcp/composer/workflow_engine.go | Updated to extract output from new ToolCallResult wrapper type |
| pkg/vmcp/server/integration_test.go | Updated mocks to return new wrapper types |
| pkg/vmcp/server/adapter/handler_factory_test.go | Updated all test expectations to use new wrapper types |
| pkg/vmcp/composer/*_test.go | Updated multiple test files to use new ToolCallResult wrapper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
2ad4a00 to
17e9b06
Compare
vMCP was discarding _meta fields from backend MCP server responses, violating the MCP specification requirement to preserve protocol-level metadata. This prevented clients from receiving progress tokens, trace IDs, and custom backend metadata. Fixes #2640
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jhrozek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: Inline Comments
These are non-blocking observations from the code review. The overall implementation is solid - these are documentation suggestions and minor improvement notes for future consideration.
Summary:
- 4 inline comments added
- All non-blocking (INFO/LOW severity)
- Focus on documentation and future improvements
| Expect(err).ToNot(HaveOccurred(), "Tool call should succeed") | ||
| Expect(result).ToNot(BeNil()) | ||
|
|
||
| By("Checking metadata preservation infrastructure") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[QUESTION] E2E Coverage: Should yardstick return _meta?
The test uses yardstick backend which may or may not return _meta. The test gracefully handles both cases, but cannot guarantee the _meta preservation code path is exercised in CI.
Would it be valuable to extend yardstick to always return _meta with known values (e.g., a trace ID)? This would provide deterministic E2E coverage for the metadata preservation feature.
|
thanks for your comments, i will address them. We also discussed the possibility of adding metadata to yardstick test server, so we can properly validate it |
vMCP was discarding _meta fields from backend MCP server responses, violating the MCP specification requirement to preserve protocol-level metadata. This prevented clients from receiving progress tokens, trace IDs, and custom backend metadata.
Large PR Justification
This is an atomic PR with complete functionality and testing. Cannot be split.
Fixes #2640