fix: return isError for argument validation failures (#2511)

* 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>

* 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>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: blackwell-systems <236632453+blackwell-systems@users.noreply.github.com>

Author: 3 people

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

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)
+}