Conversation
WalkthroughAdds server-side clientInfo support: a new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-04-21T21:26:32.945ZApplied to files:
📚 Learning: 2025-06-30T07:13:17.052ZApplied to files:
🧬 Code graph analysis (1)mcptest/mcptest.go (1)
🔇 Additional comments (4)
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.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @mcptest/mcptest_test.go:
- Line 374: The call to srv.Start(context.TODO()) ignores its returned error and
uses context.TODO() inconsistently; change it to use the existing ctx variable
and check the error returned from srv.Start by calling err := srv.Start(ctx) and
handling it (e.g., t.Fatalf or require.NoError) similar to how
TestServerWithPrompt checks Start's error so the test fails on start failures.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcptest/mcptest.gomcptest/mcptest_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:
mcptest/mcptest.gomcptest/mcptest_test.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:
mcptest/mcptest_test.go
🧠 Learnings (2)
📚 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:
mcptest/mcptest.go
📚 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:
mcptest/mcptest_test.go
🧬 Code graph analysis (2)
mcptest/mcptest.go (1)
mcp/types.go (2)
Implementation(581-587)Params(212-212)
mcptest/mcptest_test.go (5)
mcptest/mcptest.go (1)
NewUnstartedServer(59-70)mcp/tools.go (3)
NewTool(726-748)CallToolRequest(54-58)CallToolResult(40-51)server/session.go (2)
ClientSessionFromContext(112-117)SessionWithClientInfo(66-76)mcp/utils.go (1)
NewToolResultText(271-280)mcp/types.go (2)
Implementation(581-587)Params(212-212)
🔇 Additional comments (5)
mcptest/mcptest.go (3)
27-27: LGTM!The new
clientInfofield follows the existing struct pattern and is appropriately placed with other configuration fields.
124-127: LGTM!The setter method follows Go conventions and the GoDoc comment correctly starts with the identifier name as per coding guidelines.
166-166: LGTM!The client info is correctly propagated to the initialization request, enabling the test client to identify itself to the server during the MCP handshake.
mcptest/mcptest_test.go (2)
361-370: LGTM!The tool handler correctly demonstrates the client info retrieval pattern using
ClientSessionFromContextand the type assertion toSessionWithClientInfo. The nil checks are properly handled.
371-373: LGTM!The test correctly sets client info before starting the server and verifies the expected greeting is returned. The assertion pattern is consistent with other tests in the file.
Also applies to: 376-394
|
@urisimchoni could you fix the merge conflict? I will merge after. |
This allows testing of mcp server behavior that depends on the type of the mcp client.
f379598 to
4742c41
Compare
@ezynda3 Sure, done. Sorry for not noticing that this cannot be cleanly merged. |
Description
Allow tests using mcptest to simulate client info
Type of Change
Checklist
Additional Information
Sometimes the MCP server behavior depends slightly on the type of the MCP client. The server may use the "clientInfo" object that is sent as part of the "initialize" message. This PR adds the testing capability to mcptest, to set the clientInfo field sent by the test client to the tested server.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.