diff --git a/operation/sdk_integration_test.go b/operation/sdk_integration_test.go index cc6fed2..b8208e1 100644 --- a/operation/sdk_integration_test.go +++ b/operation/sdk_integration_test.go @@ -233,6 +233,24 @@ func rpcErrorCode(t *testing.T, response rawRPCResponse) int { return wire.Error.Code } +// Regression test for https://gitea.com/gitea/gitea-mcp/issues/229 +func callMissingRequiredArgument(ctx context.Context, t *testing.T, session *mcp.ClientSession) { + t.Helper() + result, err := session.CallTool(ctx, &mcp.CallToolParams{ + Name: "search_issues", + Arguments: map[string]any{"state": "open"}, + }) + if err != nil { + t.Fatalf("CallTool() error = %v, want a tool result", err) + } + if !result.IsError { + t.Errorf("IsError = false, want true for a call without the required query") + } + if got := textContent(t, result); !strings.Contains(got, "query is required") { + t.Errorf("result = %q, want it to name the missing argument", got) + } +} + func TestOfficialSDKInMemory(t *testing.T) { exposeAllTools(t) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -256,6 +274,7 @@ func TestOfficialSDKInMemory(t *testing.T) { } assertToolsOnlyCapabilities(t, session.InitializeResult().Capabilities) listAndCallVersion(ctx, t, session, testServerVersion) + callMissingRequiredArgument(ctx, t, session) if err := session.Close(); err != nil { t.Fatalf("Close() error = %v", err) } diff --git a/pkg/tool/handler_test.go b/pkg/tool/handler_test.go index 70fae03..386ca99 100644 --- a/pkg/tool/handler_test.go +++ b/pkg/tool/handler_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "strings" "testing" "github.com/modelcontextprotocol/go-sdk/jsonrpc" @@ -72,12 +73,11 @@ func TestMCPHandlerErrorClassification(t *testing.T) { for _, test := range []struct { name string handler Handler - wantCode int64 + wantCode int64 // zero when the failure belongs in a tool result rather than a protocol error }{ { - name: "server error", - handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { return nil, errors.New("failed") }, - wantCode: jsonrpc.CodeInternalError, + name: "server error", + handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { return nil, errors.New("failed") }, }, { name: "protocol error", @@ -87,17 +87,28 @@ func TestMCPHandlerErrorClassification(t *testing.T) { wantCode: jsonrpc.CodeInvalidParams, }, { - name: "panic", - handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { panic("failed") }, - wantCode: jsonrpc.CodeInternalError, + name: "panic", + handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { panic("failed") }, }, } { t.Run(test.name, func(t *testing.T) { result, err := callTool(test.handler, nil) - if result != nil { - t.Errorf("result = %#v, want nil", result) + if test.wantCode != 0 { + if result != nil { + t.Errorf("result = %#v, want nil", result) + } + assertProtocolErrorCode(t, err, test.wantCode) + return + } + if err != nil { + t.Fatalf("MCPHandler() error = %v, want nil", err) + } + if !result.IsError { + t.Error("IsError = false, want true") + } + if content, ok := result.Content[0].(*mcp.TextContent); !ok || !strings.Contains(content.Text, "failed") { + t.Errorf("Content[0] = %#v, want text naming the failure", result.Content[0]) } - assertProtocolErrorCode(t, err, test.wantCode) }) } } diff --git a/pkg/tool/tool.go b/pkg/tool/tool.go index 782c52a..2f15fec 100644 --- a/pkg/tool/tool.go +++ b/pkg/tool/tool.go @@ -10,6 +10,7 @@ import ( "gitea.com/gitea/gitea-mcp/pkg/flag" "gitea.com/gitea/gitea-mcp/pkg/log" + "gitea.com/gitea/gitea-mcp/pkg/to" "github.com/modelcontextprotocol/go-sdk/jsonrpc" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -90,9 +91,7 @@ func (s ServerTool) MCPHandler() mcp.ToolHandler { return func(ctx context.Context, req *mcp.CallToolRequest) (result *mcp.CallToolResult, err error) { defer func() { if recovered := recover(); recovered != nil { - panicErr := fmt.Errorf("panic recovered in %s tool handler: %v", s.Tool.Name, recovered) - log.Errorf("%s", panicErr) - err = internalError(panicErr) + result, err = to.ErrorResult(fmt.Errorf("panic recovered in %s tool handler: %v", s.Tool.Name, recovered)) } }() @@ -107,7 +106,7 @@ func (s ServerTool) MCPHandler() mcp.ToolHandler { if errors.As(err, &protocolErr) { return nil, err } - return nil, internalError(err) // Expected failures never reach here, handlers use CallToolResult. + return to.ErrorResult(err) } return result, nil } @@ -129,10 +128,6 @@ func decodeArguments(raw json.RawMessage) (map[string]any, error) { return arguments, nil } -func internalError(err error) error { - return &jsonrpc.Error{Code: jsonrpc.CodeInternalError, Message: err.Error()} -} - // warnUnmatched logs the names present in allowlist but absent from known, // via logUnmatched, so WarnUnmatchedAllowedTools and WarnUnmatchedAllowedScopes // share the same "collect, sort, no-op when empty" logic and can't drift.