diff --git a/internal/components/azaks/registry.go b/internal/components/azaks/registry.go index 3bb37e9..30dbe49 100644 --- a/internal/components/azaks/registry.go +++ b/internal/components/azaks/registry.go @@ -77,6 +77,8 @@ func generateToolDescription(accessLevel string) string { // Only show write operation examples if access level allows it if accessLevel == "readwrite" || accessLevel == "admin" { desc += "- Scale cluster: operation=\"scale\", args=\"--name myCluster --resource-group myRG --node-count 5\"\n" + desc += "- Update cluster (enable autoscaler): operation=\"update\", args=\"--name myCluster --resource-group myRG --enable-cluster-autoscaler --min-count 1 --max-count 5\"\n" + desc += "- Update cluster (disable autoscaler): operation=\"update\", args=\"--name myCluster --resource-group myRG --disable-cluster-autoscaler\"\n" } return desc diff --git a/internal/tools/handler.go b/internal/tools/handler.go index e266ee6..c66f216 100644 --- a/internal/tools/handler.go +++ b/internal/tools/handler.go @@ -59,6 +59,10 @@ func CreateToolHandler(executor CommandExecutor, cfg *config.ConfigData) func(ct } if err != nil { + // Include command output (often stderr) in the error for context + if result != "" { + return mcp.NewToolResultError(fmt.Sprintf("%s\n%s", err.Error(), result)), nil + } return mcp.NewToolResultError(err.Error()), nil } @@ -96,6 +100,10 @@ func CreateResourceHandler(handler ResourceHandler, cfg *config.ConfigData) func } if err != nil { + // Include handler output in the error message for better diagnostics + if result != "" { + return mcp.NewToolResultError(fmt.Sprintf("%s\n%s", err.Error(), result)), nil + } return mcp.NewToolResultError(err.Error()), nil } diff --git a/internal/tools/handler_test.go b/internal/tools/handler_test.go new file mode 100644 index 0000000..f629273 --- /dev/null +++ b/internal/tools/handler_test.go @@ -0,0 +1,297 @@ +package tools + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/Azure/aks-mcp/internal/config" + "github.com/Azure/aks-mcp/internal/telemetry" + "github.com/mark3labs/mcp-go/mcp" +) + +// helper to extract first text content from result +func firstText(result *mcp.CallToolResult) string { + for _, c := range result.Content { + if tc, ok := mcp.AsTextContent(c); ok { + return tc.Text + } + } + return "" +} + +func TestCreateToolHandler_ErrorIncludesResultOutput(t *testing.T) { + cfg := config.NewConfig() + + // Fake executor returns stderr-like output with an error + exec := CommandExecutorFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "ERROR: Azure CLI detailed message", errors.New("exit status 1") + }) + + handler := CreateToolHandler(exec, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_tool", + Arguments: map[string]any{"operation": "test"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil { + t.Fatalf("nil result returned") + } + if !res.IsError { + t.Fatalf("expected IsError=true on result") + } + msg := firstText(res) + if !strings.Contains(msg, "exit status 1") || !strings.Contains(msg, "ERROR: Azure CLI detailed message") { + t.Fatalf("expected combined error + output, got: %q", msg) + } +} + +func TestCreateToolHandler_ErrorWithoutOutput(t *testing.T) { + cfg := config.NewConfig() + + exec := CommandExecutorFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "", errors.New("exit status 1") + }) + + handler := CreateToolHandler(exec, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_tool", + Arguments: map[string]any{"operation": "test"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } + msg := firstText(res) + if msg != "exit status 1" { + t.Fatalf("expected only error text, got: %q", msg) + } +} + +func TestCreateResourceHandler_ErrorIncludesResultOutput(t *testing.T) { + cfg := config.NewConfig() + + rh := ResourceHandlerFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "API: detailed failure context", errors.New("bad request") + }) + + handler := CreateResourceHandler(rh, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_resource", + Arguments: map[string]any{"operation": "test"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } + msg := firstText(res) + if !strings.Contains(msg, "bad request") || !strings.Contains(msg, "API: detailed failure context") { + t.Fatalf("expected combined error + output, got: %q", msg) + } +} + +func TestCreateResourceHandler_ErrorWithoutOutput(t *testing.T) { + cfg := config.NewConfig() + + rh := ResourceHandlerFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "", errors.New("bad request") + }) + + handler := CreateResourceHandler(rh, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_resource", + Arguments: map[string]any{"operation": "test"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } + msg := firstText(res) + if msg != "bad request" { + t.Fatalf("expected only error text, got: %q", msg) + } +} + +func TestCreateToolHandler_Success_Verbose_Telemetry_LongResult(t *testing.T) { + cfg := config.NewConfig() + cfg.Verbose = true // exercise logToolCall + logToolResult + // Provide non-nil telemetry to exercise TrackToolInvocation path + cfg.TelemetryService = telemetry.NewService(telemetry.NewConfig("svc", "1.0")) + + long := strings.Repeat("x", 600) + exec := CommandExecutorFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return long, nil + }) + + handler := CreateToolHandler(exec, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_tool", + Arguments: map[string]any{ + "operation": "op", + }, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || res.IsError { + t.Fatalf("expected success result, got: %+v", res) + } + if got := firstText(res); got != long { + t.Fatalf("unexpected result text length=%d", len(got)) + } +} + +func TestCreateToolHandler_InvalidArguments_Verbose_LogsFallback_TracksTelemetry(t *testing.T) { + cfg := config.NewConfig() + cfg.Verbose = true + cfg.TelemetryService = telemetry.NewService(telemetry.NewConfig("svc", "1.0")) + + exec := CommandExecutorFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "should not run", nil + }) + + handler := CreateToolHandler(exec, cfg) + + // Use an argument type that fails json.Marshal to exercise logToolCall fallback branch + ch := make(chan int) + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_tool", + Arguments: ch, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } + msg := firstText(res) + if !strings.Contains(msg, "arguments must be a map[string]interface{}") { + t.Fatalf("unexpected error message: %q", msg) + } +} + +func TestCreateResourceHandler_ShortSuccess_Verbose_Telemetry(t *testing.T) { + cfg := config.NewConfig() + cfg.Verbose = true + cfg.TelemetryService = telemetry.NewService(telemetry.NewConfig("svc", "1.0")) + + rh := ResourceHandlerFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "ok", nil + }) + + handler := CreateResourceHandler(rh, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_resource", + Arguments: map[string]any{"operation": "x"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || res.IsError { + t.Fatalf("expected success result, got: %+v", res) + } + if got := firstText(res); got != "ok" { + t.Fatalf("unexpected text: %q", got) + } +} + +func TestCreateResourceHandler_InvalidArguments_Verbose_LogsFallback_TracksTelemetry(t *testing.T) { + cfg := config.NewConfig() + cfg.Verbose = true + cfg.TelemetryService = telemetry.NewService(telemetry.NewConfig("svc", "1.0")) + + rh := ResourceHandlerFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "should not run", nil + }) + + handler := CreateResourceHandler(rh, cfg) + + // Unmarshalable type to drive logToolCall fallback branch + ch := make(chan int) + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_resource", + Arguments: ch, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } + if msg := firstText(res); !strings.Contains(msg, "arguments must be a map[string]interface{}") { + t.Fatalf("unexpected error message: %q", msg) + } +} + +func TestCreateToolHandler_Error_Verbose_LogErrorBranch(t *testing.T) { + cfg := config.NewConfig() + cfg.Verbose = true + + exec := CommandExecutorFunc(func(params map[string]interface{}, _ *config.ConfigData) (string, error) { + return "", errors.New("boom") + }) + + handler := CreateToolHandler(exec, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "dummy_tool", + Arguments: map[string]any{"operation": "op"}, + }, + } + + res, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if res == nil || !res.IsError { + t.Fatalf("expected error result, got: %+v", res) + } +}