From 39dcba6c80d5afbf3681cb46e7da9156ada8773b Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 20 May 2026 10:12:04 +0200 Subject: [PATCH 1/2] refactor: simplify NewServerTool naming - Rename NewServerToolWithRawContextHandler -> NewServerTool. This is the preferred constructor for raw mcp.ToolHandler tools because it avoids creating closures at registration time, which matters for per-request servers that re-register all tools on every request. - Rename deprecated generic NewServerTool[In, Out] -> NewServerToolWithDeps to free up the simpler name and make its closure-based nature explicit. The dynamic tools package is the only legitimate user of this constructor because DynamicToolDependencies differs from the standard ToolDependencies. - Remove deprecated NewServerToolFromHandler. Its only callers can use the new NewServerTool directly via context-injected deps. - Update all call sites in dependencies.go, dynamic_tools.go, and registry_test.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/dependencies.go | 2 +- pkg/github/dynamic_tools.go | 2 +- pkg/inventory/registry_test.go | 24 +++++++++--------------- pkg/inventory/server_tool.go | 22 +++++++--------------- 4 files changed, 18 insertions(+), 32 deletions(-) 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, From 4884d6370b1422de9550928cfeaa9d416c7994ec Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 20 May 2026 10:08:36 +0200 Subject: [PATCH 2/2] fix: return isError for argument validation failures When tool argument unmarshalling fails (wrong types, malformed JSON), return a CallToolResult with IsError: true instead of a Go error. Returning a Go error is converted by the SDK into a JSON-RPC protocol error (-32603), which is invisible to agents and prevents self-correction. Returning IsError: true with the validation message lets agents see the problem and retry with corrected arguments. Affects: - NewServerToolWithDeps (was NewServerTool prior to the rename in #2510) - NewServerToolWithContextHandler Fixes #1952. Re-applies #2488 by @blackwell-systems on top of the NewServerTool rename. Co-authored-by: blackwell-systems <236632453+blackwell-systems@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/inventory/server_tool.go | 15 +++- pkg/inventory/server_tool_test.go | 118 ++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 pkg/inventory/server_tool_test.go diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index c80e9f4a3b..316fffaa91 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -3,6 +3,7 @@ package inventory import ( "context" "encoding/json" + "fmt" "github.com/github/github-mcp-server/pkg/octicons" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -133,7 +134,12 @@ func NewServerToolWithDeps[In any, Out any](tool mcp.Tool, toolset ToolsetMetada return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { var arguments In if err := json.Unmarshal(req.Params.Arguments, &arguments); err != nil { - return nil, err + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("invalid arguments: %s", err)}, + }, + IsError: true, + }, nil } resp, _, err := typedHandler(ctx, req, arguments) return resp, err @@ -157,7 +163,12 @@ func NewServerToolWithContextHandler[In any, Out any](tool mcp.Tool, toolset Too return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { var arguments In if err := json.Unmarshal(req.Params.Arguments, &arguments); err != nil { - return nil, err + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("invalid arguments: %s", err)}, + }, + IsError: true, + }, nil } resp, _, err := handler(ctx, req, arguments) return resp, err diff --git a/pkg/inventory/server_tool_test.go b/pkg/inventory/server_tool_test.go new file mode 100644 index 0000000000..0263857c93 --- /dev/null +++ b/pkg/inventory/server_tool_test.go @@ -0,0 +1,118 @@ +package inventory + +import ( + "context" + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewServerToolWithDeps_InvalidArguments_ReturnsIsError(t *testing.T) { + type expectedArgs struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + } + + tool := NewServerToolWithDeps( + mcp.Tool{Name: "test_tool"}, + testToolsetMetadata("test"), + func(_ any) mcp.ToolHandlerFor[expectedArgs, *mcp.CallToolResult] { + return func(_ context.Context, _ *mcp.CallToolRequest, _ expectedArgs) (*mcp.CallToolResult, *mcp.CallToolResult, error) { + t.Fatal("handler should not be called with invalid arguments") + return nil, nil, nil + } + }, + ) + + handler := tool.HandlerFunc(nil) + + badArgs, _ := json.Marshal(map[string]any{"owner": 12345, "repo": true}) + result, err := handler(context.Background(), &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Name: "test_tool", + Arguments: badArgs, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Len(t, result.Content, 1) + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Contains(t, textContent.Text, "invalid arguments") +} + +func TestNewServerToolWithContextHandler_InvalidArguments_ReturnsIsError(t *testing.T) { + type expectedArgs struct { + Query string `json:"query"` + Limit int `json:"limit"` + } + + tool := NewServerToolWithContextHandler( + mcp.Tool{Name: "test_context_tool"}, + testToolsetMetadata("test"), + func(_ context.Context, _ *mcp.CallToolRequest, _ expectedArgs) (*mcp.CallToolResult, any, error) { + t.Fatal("handler should not be called with invalid arguments") + return nil, nil, nil + }, + ) + + handler := tool.HandlerFunc(nil) + + result, err := handler(context.Background(), &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Name: "test_context_tool", + Arguments: json.RawMessage(`{not valid json`), + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Len(t, result.Content, 1) + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Contains(t, textContent.Text, "invalid arguments") +} + +func TestNewServerToolWithDeps_ValidArguments_Succeeds(t *testing.T) { + type expectedArgs struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + } + + tool := NewServerToolWithDeps( + mcp.Tool{Name: "test_tool"}, + testToolsetMetadata("test"), + func(_ any) mcp.ToolHandlerFor[expectedArgs, *mcp.CallToolResult] { + return func(_ context.Context, _ *mcp.CallToolRequest, args expectedArgs) (*mcp.CallToolResult, *mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "success: " + args.Owner + "/" + args.Repo}, + }, + }, nil, nil + } + }, + ) + + handler := tool.HandlerFunc(nil) + + goodArgs, _ := json.Marshal(map[string]any{"owner": "octocat", "repo": "hello-world"}) + result, err := handler(context.Background(), &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Name: "test_tool", + Arguments: goodArgs, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + assert.False(t, result.IsError) + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Equal(t, "success: octocat/hello-world", textContent.Text) +}