-
Notifications
You must be signed in to change notification settings - Fork 10
deps: update modelcontextprotocol/go-sdk to v1.2.0 #75
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
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
WalkthroughThe go.mod dependency for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modpkg/mcpproxy/record.go
🔇 Additional comments (5)
pkg/mcpproxy/record.go (5)
4-5: LGTM: Imports are appropriate.The new imports support the custom JSON marshaling and safe request wrapping functionality.
Also applies to: 9-9
34-52: LGTM: Conversion logic is sound.The function correctly handles nil input and selectively copies serializable fields from RequestExtra, which addresses the SDK's non-serializable field issue.
54-57: LGTM: SafeRequestExtra structure is appropriate.The struct correctly extracts only the serializable fields (TokenInfo and Header) from the SDK's RequestExtra.
67-77: LGTM: Custom marshaling is correctly implemented.The alias pattern correctly avoids infinite recursion, and SafeServerRequestFromUnsafe properly converts the Request field to a serializable form.
Also applies to: 87-97
28-32: Generic constraint correctly usesmcp.Paramsfrom the SDK.The
SafeServerRequest[P mcp.Params]generic constraint is properly defined. Themcp.Paramsinterface exists in the SDK and is the correct constraint for parameter types in MCP requests.
matzew
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.
LGTM
This PR updates the MCP SDK to v1.2.0 and fixes the functional test failures we were seeing on the dependabot PR.
The root cause is that the SDK introduced a non serializable parameter into the
ServerRequest.RequestExtrastruct, leading to our serialization of output to fail. From discussion in modelcontextprotocol/go-sdk#743 these types were not meant to be fully serializable. As we are serializing this info to help devs debug MCP evaluation scenarios, we still need to serialize this. So, this PR introduces a bit of custom serialization logic.Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.