-
Notifications
You must be signed in to change notification settings - Fork 752
feat:add version 2025-11-25 & unit test for version #684
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
WalkthroughUpdates MCP protocol version from 2025-06-18 to 2025-11-25 while maintaining backward compatibility with previous versions. Includes documentation updates in README, version constant changes in mcp/types.go, and comprehensive test coverage for version negotiation scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
server/version_test.go (1)
82-88: Consider usingrequirefor type assertions to prevent cascade failures.If the type assertion on line 82 or 85 fails,
assert.Truewill log the failure but continue execution, causing a panic when accessingresp.ResultorinitResult.ProtocolVersion. Usingrequire.Truewould stop the test immediately on failure.🔎 Proposed fix
+ "github.com/stretchr/testify/require" ... - resp, ok := response.(mcp.JSONRPCResponse) - assert.True(t, ok) + resp, ok := response.(mcp.JSONRPCResponse) + require.True(t, ok, "expected JSONRPCResponse type") - initResult, ok := resp.Result.(mcp.InitializeResult) - assert.True(t, ok) + initResult, ok := resp.Result.(mcp.InitializeResult) + require.True(t, ok, "expected InitializeResult type")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdmcp/types.goserver/version_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/version_test.gomcp/types.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/version_test.go
🧠 Learnings (3)
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
server/version_test.goREADME.md
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
server/version_test.go
📚 Learning: 2025-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Applied to files:
README.md
🧬 Code graph analysis (1)
server/version_test.go (1)
mcp/types.go (10)
Implementation(581-587)ClientCapabilities(509-523)JSONRPCRequest(359-364)JSONRPC_VERSION(146-146)NewRequestId(294-296)Request(203-206)Params(212-212)JSONRPCResponse(373-377)Result(281-285)InitializeResult(484-498)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
mcp/types.go (2)
135-143: LGTM! Protocol version update aligns with PR objectives.The version constant update to
"2025-11-25"and the explicit listing of"2025-06-18"inValidProtocolVersionscorrectly maintains backward compatibility with all four supported versions as documented.
1370-1375: Struct field ordering is consistent.The reordering of
HeaderandParamsfields maintains consistency across task-related request types. SinceHeaderhasjson:"-", there's no serialization impact.server/version_test.go (2)
1-11: LGTM! Good test structure following coding guidelines.The test file follows Go conventions with table-driven tests and uses
testify/assertas specified in the coding guidelines.
44-48: No action needed. The test correctly reflects the server's empty version default behavior. TheprotocolVersion()function inserver/server.go(lines 794-808) explicitly defaults emptyclientVersionto"2025-03-26"per the MCP specification requirement for backward compatibility. The test's inline comment already explains this behavior adequately.README.md (1)
201-207: LGTM! Documentation accurately reflects the version changes.The README correctly documents protocol version
2025-11-25with backward compatibility for2025-06-18,2025-03-26, and2024-11-05, matching theValidProtocolVersionsinmcp/types.go.
Description
Fixes #658
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.