fix: return handler errors and panics as tool results (#230)

Fixes https://gitea.com/gitea/gitea-mcp/issues/229

https://gitea.com/gitea/gitea-mcp/pulls/227 left an unexpected handler error and a recovered panic as JSON-RPC errors, which tell the client the request itself failed and kill the session, as in https://gitea.com/gitea/gitea-mcp/issues/229. Both now return tool results.
Reviewed-on: https://gitea.com/gitea/gitea-mcp/pulls/230
Reviewed-by: bircni <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>

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

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",
+			name:    "server error",
-			handler:  func(context.Context, map[string]any) (*mcp.CallToolResult, error) { return nil, errors.New("failed") },
+			handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { return nil, errors.New("failed") },
-			wantCode: jsonrpc.CodeInternalError,
 		},
 		{
 			name: "protocol error",
@@ -87,17 +87,28 @@ func TestMCPHandlerErrorClassification(t *testing.T) {
 			wantCode: jsonrpc.CodeInvalidParams,
 		},
 		{
-			name:     "panic",
+			name:    "panic",
-			handler:  func(context.Context, map[string]any) (*mcp.CallToolResult, error) { panic("failed") },
+			handler: func(context.Context, map[string]any) (*mcp.CallToolResult, error) { panic("failed") },
-			wantCode: jsonrpc.CodeInternalError,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			result, err := callTool(test.handler, nil)
-			if result != nil {
+			if test.wantCode != 0 {
-				t.Errorf("result = %#v, want nil", result)
+				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)
 		})
 	}
 }

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)
+				result, err = to.ErrorResult(fmt.Errorf("panic recovered in %s tool handler: %v", s.Tool.Name, recovered))
-				log.Errorf("%s", panicErr)
-				err = internalError(panicErr)
 			}
 		}()
 
@@ -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.