From b16c10f074acb05103d48b136c0162c24fb2c634 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Feb 2026 12:40:15 +0000 Subject: [PATCH 1/2] Initial plan From 8f90e0829ca61a3576686944e092038cc669c790 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Feb 2026 12:50:23 +0000 Subject: [PATCH 2/2] add newMCPError helper and replace all jsonrpc.Error literals with it Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/smoke-test-22395402042.json | 2 +- pkg/cli/mcp_server_helpers.go | 60 +++++++++++++---------------- pkg/cli/mcp_tools_management.go | 42 ++++---------------- pkg/cli/mcp_tools_privileged.go | 42 +++++--------------- pkg/cli/mcp_tools_readonly.go | 54 +++++--------------------- 5 files changed, 53 insertions(+), 147 deletions(-) diff --git a/.github/smoke-test-22395402042.json b/.github/smoke-test-22395402042.json index 8f941793eb9..eb8cab0afc6 100644 --- a/.github/smoke-test-22395402042.json +++ b/.github/smoke-test-22395402042.json @@ -1 +1 @@ -{"smoke_test": "run-22395402042", "note": "Test file for PR push validation"} +{ "smoke_test": "run-22395402042", "note": "Test file for PR push validation" } diff --git a/pkg/cli/mcp_server_helpers.go b/pkg/cli/mcp_server_helpers.go index 3f0000f70bd..dcf1f39c768 100644 --- a/pkg/cli/mcp_server_helpers.go +++ b/pkg/cli/mcp_server_helpers.go @@ -14,6 +14,12 @@ import ( "github.com/modelcontextprotocol/go-sdk/jsonrpc" ) +// newMCPError creates a jsonrpc.Error with the given code, message, and optional data. +// The data value is marshaled via mcpErrorData. +func newMCPError(code int64, msg string, data any) error { + return &jsonrpc.Error{Code: code, Message: msg, Data: mcpErrorData(data)} +} + // mcpErrorData marshals data to JSON for use in jsonrpc.Error.Data field. // Returns nil if marshaling fails to avoid errors in error handling. func mcpErrorData(v any) json.RawMessage { @@ -176,15 +182,11 @@ func checkActorPermission(ctx context.Context, actor string, validateActor bool, // If validation is enabled but no actor is specified, deny access if actor == "" { mcpLog.Printf("Tool %s: access denied (no actor specified, validation enabled)", toolName) - return &jsonrpc.Error{ - Code: jsonrpc.CodeInvalidRequest, - Message: "permission denied: insufficient role", - Data: mcpErrorData(map[string]any{ - "error": "GITHUB_ACTOR environment variable not set", - "tool": toolName, - "reason": "This tool requires at least write access to the repository. Set GITHUB_ACTOR environment variable to enable access.", - }), - } + return newMCPError(jsonrpc.CodeInvalidRequest, "permission denied: insufficient role", map[string]any{ + "error": "GITHUB_ACTOR environment variable not set", + "tool": toolName, + "reason": "This tool requires at least write access to the repository. Set GITHUB_ACTOR environment variable to enable access.", + }) } // Get repository using cached lookup @@ -208,35 +210,27 @@ func checkActorPermission(ctx context.Context, actor string, validateActor bool, permission, err := queryActorRole(ctx, actor, repo) if err != nil { mcpLog.Printf("Tool %s: failed to query actor role, denying access: %v", toolName, err) - return &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "permission denied: unable to verify repository access", - Data: mcpErrorData(map[string]any{ - "error": err.Error(), - "tool": toolName, - "actor": actor, - "repository": repo, - "reason": "Failed to query actor's repository permissions from GitHub API.", - }), - } + return newMCPError(jsonrpc.CodeInternalError, "permission denied: unable to verify repository access", map[string]any{ + "error": err.Error(), + "tool": toolName, + "actor": actor, + "repository": repo, + "reason": "Failed to query actor's repository permissions from GitHub API.", + }) } // Check if the actor has write+ access if !hasWriteAccess(permission) { mcpLog.Printf("Tool %s: access denied for actor %s (permission: %s, requires: write+)", toolName, actor, permission) - return &jsonrpc.Error{ - Code: jsonrpc.CodeInvalidRequest, - Message: "permission denied: insufficient role", - Data: mcpErrorData(map[string]any{ - "error": "insufficient repository permissions", - "tool": toolName, - "actor": actor, - "repository": repo, - "role": permission, - "required": "write, maintain, or admin", - "reason": fmt.Sprintf("Actor %s has %s access to %s. This tool requires at least write access.", actor, permission, repo), - }), - } + return newMCPError(jsonrpc.CodeInvalidRequest, "permission denied: insufficient role", map[string]any{ + "error": "insufficient repository permissions", + "tool": toolName, + "actor": actor, + "repository": repo, + "role": permission, + "required": "write, maintain, or admin", + "reason": fmt.Sprintf("Actor %s has %s access to %s. This tool requires at least write access.", actor, permission, repo), + }) } mcpLog.Printf("Tool %s: access allowed for actor %s (permission: %s)", toolName, actor, permission) diff --git a/pkg/cli/mcp_tools_management.go b/pkg/cli/mcp_tools_management.go index 0bc37fd75f2..acd4c08b956 100644 --- a/pkg/cli/mcp_tools_management.go +++ b/pkg/cli/mcp_tools_management.go @@ -32,21 +32,13 @@ func registerAddTool(server *mcp.Server, execCmd execCmdFunc) { // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } // Validate required arguments if len(args.Workflows) == 0 { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInvalidParams, - Message: "missing required parameter: at least one workflow specification is required", - Data: nil, - } + return nil, nil, newMCPError(jsonrpc.CodeInvalidParams, "missing required parameter: at least one workflow specification is required", nil) } mcpToolsManagementLog.Printf("add tool invoked: workflows=%d, number=%d, name=%q", len(args.Workflows), args.Number, args.Name) @@ -70,11 +62,7 @@ func registerAddTool(server *mcp.Server, execCmd execCmdFunc) { output, err := cmd.CombinedOutput() if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to add workflows", - Data: mcpErrorData(map[string]any{"error": err.Error(), "output": string(output)}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to add workflows", map[string]any{"error": err.Error(), "output": string(output)}) } return &mcp.CallToolResult{ @@ -121,11 +109,7 @@ Returns formatted text output showing: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -150,11 +134,7 @@ Returns formatted text output showing: output, err := cmd.CombinedOutput() if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to update workflows", - Data: mcpErrorData(map[string]any{"error": err.Error(), "output": string(output)}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to update workflows", map[string]any{"error": err.Error(), "output": string(output)}) } return &mcp.CallToolResult{ @@ -210,11 +190,7 @@ Returns formatted text output showing: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -239,11 +215,7 @@ Returns formatted text output showing: output, err := cmd.CombinedOutput() if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to fix workflows", - Data: mcpErrorData(map[string]any{"error": err.Error(), "output": string(output)}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to fix workflows", map[string]any{"error": err.Error(), "output": string(output)}) } return &mcp.CallToolResult{ diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index bbb7f92ac5d..15ad33624fa 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/pkg/cli/mcp_tools_privileged.go @@ -78,34 +78,22 @@ return a schema description instead of the full output. Adjust the 'max_tokens' // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } // Validate firewall parameters if args.Firewall && args.NoFirewall { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInvalidParams, - Message: "conflicting parameters: cannot specify both 'firewall' and 'no_firewall'", - Data: nil, - } + return nil, nil, newMCPError(jsonrpc.CodeInvalidParams, "conflicting parameters: cannot specify both 'firewall' and 'no_firewall'", nil) } // Validate workflow name before executing command if err := validateWorkflowName(args.WorkflowName); err != nil { mcpLog.Printf("Workflow name validation failed: %v", err) - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInvalidParams, - Message: err.Error(), - Data: mcpErrorData(map[string]any{ - "workflow_name": args.WorkflowName, - "error_type": "workflow_not_found", - }), - } + return nil, nil, newMCPError(jsonrpc.CodeInvalidParams, err.Error(), map[string]any{ + "workflow_name": args.WorkflowName, + "error_type": "workflow_not_found", + }) } // Build command arguments @@ -191,11 +179,7 @@ return a schema description instead of the full output. Adjust the 'max_tokens' "workflow": args.WorkflowName, } - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to download workflow logs: " + err.Error(), - Data: mcpErrorData(errorData), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to download workflow logs: "+err.Error(), errorData) } // Check output size and apply guardrail if needed @@ -270,11 +254,7 @@ Returns JSON with the following structure: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -317,11 +297,7 @@ Returns JSON with the following structure: "run_id_or_url": args.RunIDOrURL, } - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to audit workflow run: " + err.Error(), - Data: mcpErrorData(errorData), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to audit workflow run: "+err.Error(), errorData) } return &mcp.CallToolResult{ diff --git a/pkg/cli/mcp_tools_readonly.go b/pkg/cli/mcp_tools_readonly.go index 9af39e22675..e8868ff1722 100644 --- a/pkg/cli/mcp_tools_readonly.go +++ b/pkg/cli/mcp_tools_readonly.go @@ -39,11 +39,7 @@ Returns a JSON array where each element has the following structure: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -52,21 +48,13 @@ Returns a JSON array where each element has the following structure: // Call GetWorkflowStatuses directly instead of spawning subprocess statuses, err := GetWorkflowStatuses(args.Pattern, "", "", "") if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to get workflow statuses", - Data: mcpErrorData(map[string]any{"error": err.Error()}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to get workflow statuses", map[string]any{"error": err.Error()}) } // Marshal to JSON jsonBytes, err := json.Marshal(statuses) if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to marshal workflow statuses", - Data: mcpErrorData(map[string]any{"error": err.Error()}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to marshal workflow statuses", map[string]any{"error": err.Error()}) } outputStr := string(jsonBytes) @@ -133,11 +121,7 @@ Returns JSON array with validation results for each workflow: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -145,21 +129,13 @@ Returns JSON array with validation results for each workflow: if args.Zizmor || args.Poutine || args.Actionlint { // Check if Docker images are available; if not, start downloading and return retry message if err := CheckAndPrepareDockerImages(ctx, args.Zizmor, args.Poutine, args.Actionlint); err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "docker images not ready", - Data: mcpErrorData(err.Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "docker images not ready", err.Error()) } // Check for cancellation after Docker image preparation select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } } @@ -218,11 +194,7 @@ Returns JSON array with validation results for each workflow: if errors.As(err, &exitErr) { stderr = string(exitErr.Stderr) } - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to compile workflows", - Data: mcpErrorData(map[string]any{"error": err.Error(), "stderr": stderr}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to compile workflows", map[string]any{"error": err.Error(), "stderr": stderr}) } // Otherwise, we have output (likely validation errors in JSON), so continue // and return it to the LLM @@ -279,11 +251,7 @@ Returns formatted text output showing: // Check for cancellation before starting select { case <-ctx.Done(): - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "request cancelled", - Data: mcpErrorData(ctx.Err().Error()), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "request cancelled", ctx.Err().Error()) default: } @@ -310,11 +278,7 @@ Returns formatted text output showing: output, err := cmd.CombinedOutput() if err != nil { - return nil, nil, &jsonrpc.Error{ - Code: jsonrpc.CodeInternalError, - Message: "failed to inspect MCP servers", - Data: mcpErrorData(map[string]any{"error": err.Error(), "output": string(output)}), - } + return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to inspect MCP servers", map[string]any{"error": err.Error(), "output": string(output)}) } return &mcp.CallToolResult{