Fix tool calls #38
Fix tool calls #38
Conversation
WalkthroughThe changes update the server’s approach to handling capabilities in Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🔭 Outside diff range comments (3)
server/server.go (3)
524-566:⚠️ Potential issueFix nil pointer dereference in handleInitialize
This section contains a critical error. Since capabilities are now initialized as nil, the handleInitialize method will cause a panic when trying to access fields of nil capability structures.
The pipeline failure confirms this issue: "Panic: runtime error: invalid memory address or nil pointer dereference in MCPServer.handleInitialize."
Here's how to fix it:
func (s *MCPServer) handleInitialize( ctx context.Context, id interface{}, request mcp.InitializeRequest, ) mcp.JSONRPCMessage { capabilities := mcp.ServerCapabilities{} - capabilities.Resources = &struct { - Subscribe bool `json:"subscribe,omitempty"` - ListChanged bool `json:"listChanged,omitempty"` - }{ - Subscribe: s.capabilities.resources.subscribe, - ListChanged: s.capabilities.resources.listChanged, - } - - capabilities.Prompts = &struct { - ListChanged bool `json:"listChanged,omitempty"` - }{ - ListChanged: s.capabilities.prompts.listChanged, - } - - capabilities.Tools = &struct { - ListChanged bool `json:"listChanged,omitempty"` - }{ - ListChanged: s.capabilities.tools.listChanged, - } + if s.capabilities.resources != nil { + capabilities.Resources = &struct { + Subscribe bool `json:"subscribe,omitempty"` + ListChanged bool `json:"listChanged,omitempty"` + }{ + Subscribe: s.capabilities.resources.subscribe, + ListChanged: s.capabilities.resources.listChanged, + } + } + + if s.capabilities.prompts != nil { + capabilities.Prompts = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{ + ListChanged: s.capabilities.prompts.listChanged, + } + } + + if s.capabilities.tools != nil { + capabilities.Tools = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{ + ListChanged: s.capabilities.tools.listChanged, + } + } if s.capabilities.logging { capabilities.Logging = &struct{}{} } result := mcp.InitializeResult{ ProtocolVersion: mcp.LATEST_PROTOCOL_VERSION, ServerInfo: mcp.Implementation{ Name: s.name, Version: s.version, }, Capabilities: capabilities, } s.initialized.Store(true) return createResponse(id, result) }🧰 Tools
🪛 GitHub Actions: go
[error] 535-535: Panic: runtime error: invalid memory address or nil pointer dereference in MCPServer.handleInitialize.
379-385: 🛠️ Refactor suggestionInconsistent tools check in handleMessage
In the "tools/list" case, you're checking
len(s.tools) == 0while other capabilities check for nil. This is inconsistent with other methods like "tools/call" which check ifs.capabilities.tools == nil.For consistency with other capability checks, modify this section:
case "tools/list": - if len(s.tools) == 0 { + if s.capabilities.tools == nil { return createErrorResponse( baseMessage.ID, mcp.METHOD_NOT_FOUND, "Tools not supported", ) }
489-495: 🛠️ Refactor suggestionMissing capability initialization in SetTools
The SetTools method resets the tools map but doesn't initialize the tools capability if it's nil.
Add capability initialization:
// SetTools replaces all existing tools with the provided list func (s *MCPServer) SetTools(tools ...ServerTool) { + if s.capabilities.tools == nil { + s.capabilities.tools = &toolCapabilities{} + } s.mu.Lock() s.tools = make(map[string]ServerTool) s.mu.Unlock() s.AddTools(tools...) }
🧹 Nitpick comments (2)
server/server.go (2)
465-467: Consider initializing tools capability in AddToolThe AddTool method delegates to AddTools but doesn't initialize the capabilities itself, which is inconsistent with other similar methods.
For consistency with other capability initialization methods:
// AddTool registers a new tool and its handler func (s *MCPServer) AddTool(tool mcp.Tool, handler ToolHandlerFunc) { + if s.capabilities.tools == nil { + s.capabilities.tools = &toolCapabilities{} + } s.AddTools(ServerTool{Tool: tool, Handler: handler}) }
497-512: Consider capability check in DeleteToolsThe DeleteTools method doesn't check if the tools capability is initialized.
Add a protection against nil capability:
// DeleteTools removes a tool from the server func (s *MCPServer) DeleteTools(names ...string) { + if s.capabilities.tools == nil { + return // Nothing to delete if tools capability isn't enabled + } s.mu.Lock() for _, name := range names { delete(s.tools, name) } initialized := s.initialized.Load() s.mu.Unlock() // Send notification if server is already initialized if initialized { if err := s.SendNotificationToClient("notifications/tools/list_changed", nil); err != nil { // We can't return the error, but in a future version we could log it } } }🧰 Tools
🪛 golangci-lint (1.62.2)
508-508: SA9003: empty branch
(staticcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server.go(8 hunks)
🧰 Additional context used
🪛 GitHub Actions: go
server/server.go
[error] 535-535: Panic: runtime error: invalid memory address or nil pointer dereference in MCPServer.handleInitialize.
🔇 Additional comments (4)
server/server.go (4)
214-216: Initialize capabilities to nil by defaultThe change from initializing capabilities with empty structs to nil values makes sense as it aligns with the updated approach of lazy initialization throughout the server code.
328-334: Check for nil capabilities across request handlersThe changes to check for nil capabilities pointers rather than checking fields is consistent and aligned with the new initialization approach.
Also applies to: 345-351, 362-368, 396-402
425-428: Dynamic initialization of capabilitiesGood improvement! The code now dynamically initializes capabilities when they're needed instead of panicking, which improves robustness.
Also applies to: 442-444, 455-457
471-473: Initialize tools capabilities on demandThe change to initialize the tools capability structure on demand is consistent with the other capability initializations.
Summary by CodeRabbit