Skip to content

Add newMCPError helper to eliminate repeated jsonrpc.Error struct literals #18341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Feb 25, 2026
Merged

Add newMCPError helper to eliminate repeated jsonrpc.Error struct literals #18341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b16c10f
Initial plan
Copilot Feb 25, 2026
8f90e08
add newMCPError helper and replace all jsonrpc.Error literals with it
Copilot Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/smoke-test-22395402042.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"smoke_test": "run-22395402042", "note": "Test file for PR push validation"}
{ "smoke_test": "run-22395402042", "note": "Test file for PR push validation" }
60 changes: 27 additions & 33 deletions pkg/cli/mcp_server_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)}
}
Comment on lines +17 to +21
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newMCPError is new behavior (even if simple), but there are existing unit tests in mcp_server_helpers_test.go for related helpers. Consider adding a small test to assert it returns a *jsonrpc.Error with the expected Code, Message, and Data (including that data == nil results in Data == nil) to prevent accidental format regressions.

Copilot uses AI. Check for mistakes.

// 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 {
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
42 changes: 7 additions & 35 deletions pkg/cli/mcp_tools_management.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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{
Expand Down Expand Up @@ -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:
}

Expand All @@ -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{
Expand Down Expand Up @@ -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:
}

Expand All @@ -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{
Expand Down
42 changes: 9 additions & 33 deletions pkg/cli/mcp_tools_privileged.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
}

Expand Down Expand Up @@ -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{
Expand Down
54 changes: 9 additions & 45 deletions pkg/cli/mcp_tools_readonly.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
}

Expand All @@ -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)
Expand Down Expand Up @@ -133,33 +121,21 @@ 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:
}

// Check if any static analysis tools are requested that require Docker images
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:
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
}

Expand All @@ -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{
Expand Down