diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 16be84efb4..eb856e0bd6 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -253,7 +253,7 @@ func NewToolFromHandler( requiredScopes []scopes.Scope, handler func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest) (*mcp.CallToolResult, error), ) inventory.ServerTool { - st := inventory.NewServerToolWithRawContextHandler(tool, toolset, func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + st := inventory.NewServerTool(tool, toolset, func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { deps := MustDepsFromContext(ctx) return handler(ctx, deps, req) }) diff --git a/pkg/github/dynamic_tools.go b/pkg/github/dynamic_tools.go index 5c7d31d4ea..1106616fa0 100644 --- a/pkg/github/dynamic_tools.go +++ b/pkg/github/dynamic_tools.go @@ -31,7 +31,7 @@ type DynamicToolDependencies struct { // tools (ToolDependencies), so they intentionally use the closure pattern. func NewDynamicTool(toolset inventory.ToolsetMetadata, tool mcp.Tool, handler func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any]) inventory.ServerTool { //nolint:staticcheck // SA1019: Dynamic tools use a different deps structure, closure pattern is intentional - return inventory.NewServerTool(tool, toolset, func(d any) mcp.ToolHandlerFor[map[string]any, any] { + return inventory.NewServerToolWithDeps(tool, toolset, func(d any) mcp.ToolHandlerFor[map[string]any, any] { return handler(d.(DynamicToolDependencies)) }) } diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 77c3bb57e5..e6aedc620c 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -38,7 +38,7 @@ func testToolsetMetadataWithDefault(id string, isDefault bool) ToolsetMetadata { // mockToolWithDefault creates a mock tool with a default toolset flag func mockToolWithDefault(name string, toolsetID string, readOnly bool, isDefault bool) ServerTool { - return NewServerToolFromHandler( + return NewServerTool( mcp.Tool{ Name: name, Annotations: &mcp.ToolAnnotations{ @@ -47,17 +47,15 @@ func mockToolWithDefault(name string, toolsetID string, readOnly bool, isDefault InputSchema: json.RawMessage(`{"type":"object","properties":{}}`), }, testToolsetMetadataWithDefault(toolsetID, isDefault), - func(_ any) mcp.ToolHandler { - return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - return nil, nil - } + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, nil }, ) } // mockTool creates a minimal ServerTool for testing func mockTool(name string, toolsetID string, readOnly bool) ServerTool { - return NewServerToolFromHandler( + return NewServerTool( mcp.Tool{ Name: name, Annotations: &mcp.ToolAnnotations{ @@ -66,10 +64,8 @@ func mockTool(name string, toolsetID string, readOnly bool) ServerTool { InputSchema: json.RawMessage(`{"type":"object","properties":{}}`), }, testToolsetMetadata(toolsetID), - func(_ any) mcp.ToolHandler { - return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - return nil, nil - } + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, nil }, ) } @@ -1839,7 +1835,7 @@ func TestWithTools_DeprecatedAliasAndFeatureFlag(t *testing.T) { // mockToolWithMeta creates a ServerTool with Meta for testing insiders mode func mockToolWithMeta(name string, toolsetID string, meta map[string]any) ServerTool { - return NewServerToolFromHandler( + return NewServerTool( mcp.Tool{ Name: name, Annotations: &mcp.ToolAnnotations{ @@ -1849,10 +1845,8 @@ func mockToolWithMeta(name string, toolsetID string, meta map[string]any) Server Meta: meta, }, testToolsetMetadata(toolsetID), - func(_ any) mcp.ToolHandler { - return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - return nil, nil - } + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, nil }, ) } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 752a4c2bd0..c80e9f4a3b 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -118,13 +118,13 @@ func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) { s.AddTool(&toolCopy, handler) } -// NewServerTool creates a ServerTool from a tool definition, toolset metadata, and a typed handler function. +// NewServerToolWithDeps creates a ServerTool from a tool definition, toolset metadata, and a typed handler function. // The handler function takes dependencies (as any) and returns a typed handler. // Callers should type-assert deps to their typed dependencies struct. // // Deprecated: This creates closures at registration time. For better performance in // per-request server scenarios, use NewServerToolWithContextHandler instead. -func NewServerTool[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandlerFor[In, Out]) ServerTool { +func NewServerToolWithDeps[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandlerFor[In, Out]) ServerTool { return ServerTool{ Tool: tool, Toolset: toolset, @@ -166,22 +166,14 @@ func NewServerToolWithContextHandler[In any, Out any](tool mcp.Tool, toolset Too } } -// NewServerToolFromHandler creates a ServerTool from a tool definition, toolset metadata, and a raw handler function. -// Use this when you have a handler that already conforms to mcp.ToolHandler. -// -// Deprecated: This creates closures at registration time. For better performance in -// per-request server scenarios, use NewServerToolWithRawContextHandler instead. -func NewServerToolFromHandler(tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandler) ServerTool { - return ServerTool{Tool: tool, Toolset: toolset, HandlerFunc: handlerFn} -} - -// NewServerToolWithRawContextHandler creates a ServerTool with a raw handler that receives deps via context. -// This is the preferred approach for tools that use mcp.ToolHandler directly because it doesn't -// create closures at registration time. +// NewServerTool creates a ServerTool with a raw handler that receives deps via context. +// This is the preferred constructor for tools that use mcp.ToolHandler directly because +// it doesn't create closures at registration time, which is critical for performance in +// servers that create a new instance per request. // // The handler function is stored directly without wrapping in a deps closure. // Dependencies should be injected into context before calling tool handlers. -func NewServerToolWithRawContextHandler(tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandler) ServerTool { +func NewServerTool(tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandler) ServerTool { return ServerTool{ Tool: tool, Toolset: toolset,