feat(get-server-tools): add GetTools method for retrieve MCPServer.tools #437
Conversation
WalkthroughAdds two MCPServer accessors — Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server.go(1 hunks)server/server_test.go(1 hunks)
🔇 Additional comments (2)
server/server_test.go (2)
2026-2380: Excellent comprehensive test coverage.The test suite covers all important scenarios including edge cases, concurrency, and complex schemas. The test structure with subtests is well-organized and the assertions are thorough.
2210-2259: Improve concurrent test to avoid potential race conditions.The current concurrent test has timing issues where multiple goroutines add tools concurrently, but the verification logic doesn't guarantee all tools are added before
GetToolsis called in each goroutine.Consider this improved approach:
t.Run("ConcurrentAccess", func(t *testing.T) { server := NewMCPServer("test-server", "1.0.0") - // Number of goroutines for testing - numGoroutines := 100 - numToolsPerGoroutine := 10 + // Pre-add some tools to test concurrent reads + for i := 0; i < 50; i++ { + server.AddTool(mcp.Tool{ + Name: fmt.Sprintf("pre-tool-%d", i), + Description: fmt.Sprintf("Pre-added tool %d", i), + }, nil) + } - // Channel to collect results - results := make(chan map[string]ServerTool, numGoroutines) + numGoroutines := 100 + results := make(chan map[string]ServerTool, numGoroutines) + var wg sync.WaitGroup - // Start goroutines that concurrently call GetTools + // Test concurrent reads (no race conditions in test logic) for i := 0; i < numGoroutines; i++ { + wg.Add(1) go func(id int) { - // Add some tools specific to this goroutine - for j := 0; j < numToolsPerGoroutine; j++ { - toolName := fmt.Sprintf("tool-%d-%d", id, j) - server.AddTool(mcp.Tool{ - Name: toolName, - Description: fmt.Sprintf("Tool %d from goroutine %d", j, id), - }, nil) - } - - // Get tools + defer wg.Done() tools, err := server.GetTools() assert.NoError(t, err) results <- tools }(i) } + wg.Wait() + close(results) + // Collect all results var allResults []map[string]ServerTool - for i := 0; i < numGoroutines; i++ { - result := <-results + for result := range results { allResults = append(allResults, result) } // Verify that no data races occurred and all results are valid for _, result := range allResults { assert.NotNil(t, result) - // Each result should have at least some tools (may not have all due to timing) - assert.Greater(t, len(result), 0) + assert.Equal(t, 50, len(result), "All concurrent reads should return same number of tools") } - - // Final check - get all tools at the end - finalTools, err := server.GetTools() - assert.NoError(t, err) - assert.NotNil(t, finalTools) - // Should have exactly numGoroutines * numToolsPerGoroutine tools - assert.Equal(t, numGoroutines*numToolsPerGoroutine, len(finalTools)) })Likely an incorrect or invalid review comment.
ValeriiStepanets
force-pushed
the
get-server-tools
branch
from
2cd60ac to
d13b0ed
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/server.go (1)
496-510: Consider returning empty map instead of error for no tools.The current implementation returns an error when no tools are registered, but this may not align with Go conventions. Typically, empty collections return empty maps rather than errors unless the absence of data represents a true error condition.
Consider this approach instead:
-// GetTools retrieves the currently registered tools -func (s *MCPServer) GetTools() (map[string]ServerTool, error) { - s.toolsMu.RLock() - defer s.toolsMu.RUnlock() - // Create a copy to prevent external modification - if len(s.tools) == 0 { - return nil, fmt.Errorf("no tools registered") - } - toolsCopy := make(map[string]ServerTool, len(s.tools)) - for name, tool := range s.tools { - toolsCopy[name] = tool - } - return toolsCopy, nil -} +// GetTools retrieves the currently registered tools +func (s *MCPServer) GetTools() (map[string]ServerTool, error) { + s.toolsMu.RLock() + defer s.toolsMu.RUnlock() + // Create a copy to prevent external modification + toolsCopy := make(map[string]ServerTool, len(s.tools)) + for name, tool := range s.tools { + toolsCopy[name] = tool + } + return toolsCopy, nil +}However, if the current error behavior is intentional for your API design, the implementation is otherwise correct with proper concurrency safety and data encapsulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server.go(1 hunks)server/server_test.go(2 hunks)
🔇 Additional comments (2)
server/server_test.go (2)
11-11: Good addition of sync import for concurrent testing.The
syncimport is appropriately added to support the concurrent access testing in the new test suite.
2027-2419: Excellent comprehensive test suite for GetTools method.This test suite is exceptionally well-designed and thorough, covering:
- Edge cases: Empty server state, single and multiple tools
- State modifications: Tool deletion and replacement via SetTools
- Concurrency safety: 100 goroutines testing thread safety
- Data integrity: Verification that copies are returned, not references
- Complex scenarios: Complex tool schemas with nested properties and annotations
- Consistency: Multiple calls returning identical results
The concurrent test properly uses goroutines with synchronization to collect results and verify no data races occur. The copy verification test effectively demonstrates that external modifications don't affect the server's internal state.
Key strengths:
- Proper use of
sync.WaitGroupfor concurrent testing- Comprehensive assertion coverage with meaningful error messages
- Tests validate both data correctness and behavioral expectations
- Good separation of concerns with focused sub-tests
|
Hello @ezynda3, could you please take a look |
|
@ValeriiStepanets I left a comment, could please update this pr |
ValeriiStepanets
force-pushed
the
get-server-tools
branch
from
d13b0ed to
e32acd2
Compare
|
@dugenkui03, I've split GetTools into 2 methods: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
server/server_test.go (7)
2027-2063: Solid happy-path coverage for GetTool; consider asserting copy semantics.After asserting equality, also verify mutating the returned copy doesn’t affect server state.
@@ - assert.NotNil(t, tool.Handler) + assert.NotNil(t, tool.Handler) + // Mutating returned copy must not affect server state + tool.Tool.Description = "mutated" + tool2 := server.GetTool("test-tool") + assert.Equal(t, "A test tool", tool2.Tool.Description)
2065-2081: Confirm API contract: GetTool returns a non-nil pointer to zero-value for missing tools.This is unusual in Go and easy to misuse. Consider returning (tool *ServerTool, ok bool) or nil when absent; at minimum, document the behavior clearly.
2105-2156: Optional: verify defensive copy for nested schema maps.If you intend callers not to mutate server state via returned objects, add a check that modifying Properties on the returned value doesn’t leak back.
@@ - // Verify annotations + // Verify annotations assert.Equal(t, "Complex Tool", tool.Tool.Annotations.Title) assert.NotNil(t, tool.Tool.Annotations.DestructiveHint) assert.True(t, *tool.Tool.Annotations.DestructiveHint) + + // Optional: ensure nested map is a defensive copy (if this is the intended contract) + tool.Tool.InputSchema.Properties["newProp"] = map[string]any{"type": "string"} + fresh := server.GetTool("complex-tool") + assert.NotContains(t, fresh.Tool.InputSchema.Properties, "newProp", + "Mutating returned schema should not affect server state")
2159-2159: Naming consistency: GetTools vs ListTools.PR title mentions GetTools; tests (and API) use ListTools. Ensure naming is finalized and docs/PR description match.
2160-2166: Confirm nil vs empty map contract for ListTools.Tests assert nil when empty. If consumers iterate without nil checks, returning an empty map may be safer. Please confirm desired API.
2210-2265: Add aliasing guard to catch the “address-of-range-variable” bug.Existing equality checks likely catch it, but an explicit NotSame between pointers strengthens the test.
@@ assert.NotNil(t, retrievedTools) assert.Len(t, retrievedTools, 3) @@ for _, expectedTool := range tools { serverTool, exists := retrievedTools[expectedTool.tool.Name] assert.True(t, exists, "Tool %s should exist", expectedTool.tool.Name) assert.Equal(t, expectedTool.tool, serverTool.Tool) assert.NotNil(t, serverTool.Handler) } + + // Ensure pointers are distinct per entry (no &range var aliasing) + assert.NotSame(t, retrievedTools["tool1"], retrievedTools["tool2"]) + assert.NotSame(t, retrievedTools["tool2"], retrievedTools["tool3"]) + assert.NotSame(t, retrievedTools["tool1"], retrievedTools["tool3"])
2413-2447: Good snapshot-copy test; consider adding deeper immutability checks.You verify map copies; also assert that mutating returned tool data doesn’t affect server state across calls.
@@ // Server should still have the tool tools3 := server.ListTools() assert.NotNil(t, tools3) assert.Len(t, tools3, 1) assert.Contains(t, tools3, "test-tool") + + // Also ensure mutating returned tool does not affect server state + tools2["test-tool"].Tool.Description = "changed" + fresh := server.GetTool("test-tool") + assert.Equal(t, "Test tool", fresh.Tool.Description)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/server.go(1 hunks)server/server_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/server_test.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
PR: mark3labs/mcp-go#495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
Learning: In Go, test files within the same package can access types and functions defined in other test files in that package. When analyzing test code that references undefined types, check other test files in the same package for shared test utilities and mock types.
Applied to files:
server/server_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
server/server_test.go
🧬 Code graph analysis (1)
server/server_test.go (4)
server/server.go (3)
NewMCPServer(292-318)ToolHandlerFunc(41-41)ServerTool(50-53)mcp/tools.go (9)
Tool(472-483)Description(671-675)ToolInputSchema(518-522)Properties(913-917)CallToolRequest(46-49)CallToolResult(36-43)Required(679-683)ToolAnnotation(541-552)Title(687-691)mcp/types.go (4)
Content(827-829)TextContent(833-838)TextContent(840-840)Annotations(805-818)mcp/utils.go (1)
ToBoolPtr(817-819)
🔇 Additional comments (3)
server/server_test.go (3)
11-11: Import looks good.sync is required for the concurrency test; no issues.
2105-2156: Good validation of complex schema and annotations.Covers Required and DestructiveHint; nice.
2340-2379: Concurrency read test is concise and effective.Covers many goroutines and validates consistent length. LGTM.
server/server.go
Outdated
| s.toolsMu.RLock() | ||
| defer s.toolsMu.RUnlock() | ||
| tool := s.tools[toolName] | ||
| return &tool |
There was a problem hiding this comment.
If toolName does not exist, there will be a problem:
func (s *MCPServer) GetTool(toolName string) *ServerTool {
s.toolsMu.RLock()
defer s.toolsMu.RUnlock()
if tool, ok := s.tools[toolName]; ok {
return &tool
}
return nil
}There was a problem hiding this comment.
You are correct!
Fixed, @dugenkui03 please check now
ValeriiStepanets
force-pushed
the
get-server-tools
branch
from
e32acd2 to
5b8ae98
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/server_test.go (2)
2283-2290: Nil vs empty map on ListTools (API ergonomics).Returning an empty map instead of nil simplifies callers (len safe and avoids nil checks). If backward-compat allows, consider returning map[string]*ServerTool{}.
2537-2570: Prefer mutation-based assertions over NotSame for maps; add deep-copy check for values.NotSame on maps is fragile; the stronger proof is that mutating one result doesn’t affect another or server state. Also verify deep-copy for nested schema in returned ServerTool values.
Apply this patch:
- // Add a tool - server.AddTool(mcp.Tool{ - Name: "test-tool", - Description: "Test tool", - }, nil) + // Add a tool with nested schema to test deep-copy + server.AddTool(mcp.Tool{ + Name: "test-tool", + Description: "Test tool", + InputSchema: mcp.ToolInputSchema{ + Type: "object", + Properties: map[string]any{ + "flag": map[string]any{"type": "boolean"}, + }, + }, + }, nil) @@ - // They should NOT be the same reference (different memory addresses) - // This verifies that ListTools returns copies, not shared references - if len(tools1) > 0 && len(tools2) > 0 { - assert.NotSame(t, tools1, tools2, "ListTools should return copies, not shared references") - } - - // Modifying one should not affect the other + // Modifying one should not affect the other delete(tools1, "test-tool") assert.Len(t, tools1, 0, "Modified copy should be empty") assert.Len(t, tools2, 1, "Original copy should be unchanged") assert.Contains(t, tools2, "test-tool", "Original copy should still contain the tool") - // Server should still have the tool + // Also mutate the returned tool value to verify deep-copy of nested fields + tools2["test-tool"].Tool.Description = "Changed" + tools2["test-tool"].Tool.InputSchema.Properties["flag"].(map[string]any)["type"] = "number" + + // Server should still have the original tool unchanged tools3 := server.ListTools() assert.NotNil(t, tools3) assert.Len(t, tools3, 1) assert.Contains(t, tools3, "test-tool") + assert.Equal(t, "Test tool", tools3["test-tool"].Tool.Description) + pflag, ok := tools3["test-tool"].Tool.InputSchema.Properties["flag"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "boolean", pflag["type"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/server.go(1 hunks)server/server_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/server.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/server_test.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
PR: mark3labs/mcp-go#495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
Learning: In Go, test files within the same package can access types and functions defined in other test files in that package. When analyzing test code that references undefined types, check other test files in the same package for shared test utilities and mock types.
Applied to files:
server/server_test.go
🧬 Code graph analysis (1)
server/server_test.go (3)
server/server.go (3)
NewMCPServer(292-318)ServerTool(50-53)ToolHandlerFunc(41-41)mcp/tools.go (9)
Tool(472-483)Description(671-675)ToolInputSchema(518-522)Properties(913-917)CallToolRequest(46-49)CallToolResult(36-43)Required(679-683)ToolAnnotation(541-552)Title(687-691)mcp/types.go (4)
Content(827-829)TextContent(833-838)TextContent(840-840)Annotations(805-818)
🔇 Additional comments (3)
server/server_test.go (3)
11-11: Import sync — OK.Needed for the new concurrency subtests; scoped and used correctly.
2179-2219: ConcurrentAccess subtest — solid.Good use of buffered channel and WaitGroup; exercises read path without races.
2465-2503: ListTools concurrent reads — LGTM.Well-structured fan-out/fan-in; validates stable size under concurrency.
Description
This PR adds the GetTools method for retrieving the MCPServer.tools.
Initially, I was developing the custom
tools managerfor my project, but faced the issue that I couldn't synchronizeMCPServer.toolswith the customTools managerlist of tools, which could possibly lead to issues.Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This PR adds comprehensive tests for the
GetToolsmethod in the MCPServer.Changes
Test Coverage
All tests pass and integrate well with the existing test suite.
Summary by CodeRabbit