From b0c94866cb5dc10cdb3513c0be1edafcb29c3eba Mon Sep 17 00:00:00 2001 From: Hashim1999164 <64767361+Hashim1999164@users.noreply.github.com> Date: Sun, 16 Aug 2026 17:16:35 +0500 Subject: [PATCH 1/2] Show nested GitHub API validation messages in tool errors create_branch currently forwards the compact 422 dump, which hides ruleset details the GitHub UI already shows. Unwrap ErrorResponse so agents can see each validation message and recover. --- pkg/errors/error.go | 52 ++++++++++++++++++++++++++++++++- pkg/errors/error_test.go | 47 +++++++++++++++++++++++++++++ pkg/github/repositories_test.go | 30 +++++++++++++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index cb4e8b1f0f..205da46ced 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -6,6 +6,7 @@ import ( stderrors "errors" "fmt" "net/http" + "strings" "time" "github.com/github/github-mcp-server/pkg/utils" @@ -191,7 +192,56 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github "%s: GitHub secondary rate limit exceeded. Wait before retrying.", message)) } - return utils.NewToolResultErrorFromErr(message, err) + return utils.NewToolResultErrorFromErr(message, formattedGitHubAPIError(err)) +} + +// formattedGitHubAPIError unwraps a github.ErrorResponse so tool results include +// nested validation messages (for example repository ruleset violations) instead +// of go-github's compact 422 dump. +func formattedGitHubAPIError(err error) error { + var ghErr *github.ErrorResponse + if !stderrors.As(err, &ghErr) { + return err + } + + var parts []string + switch { + case ghErr.Response != nil && ghErr.Response.StatusCode != 0 && ghErr.Message != "": + parts = append(parts, fmt.Sprintf("HTTP %d %s", ghErr.Response.StatusCode, ghErr.Message)) + case ghErr.Response != nil && ghErr.Response.StatusCode != 0: + parts = append(parts, fmt.Sprintf("HTTP %d", ghErr.Response.StatusCode)) + case ghErr.Message != "": + parts = append(parts, ghErr.Message) + } + + for _, item := range ghErr.Errors { + detail := strings.TrimSpace(item.Message) + if detail == "" { + var bits []string + if item.Resource != "" { + bits = append(bits, item.Resource) + } + if item.Field != "" { + bits = append(bits, item.Field) + } + if item.Code != "" { + bits = append(bits, item.Code) + } + detail = strings.Join(bits, " ") + } + if detail != "" { + parts = append(parts, detail) + } + } + + if ghErr.DocumentationURL != "" { + parts = append(parts, "See "+ghErr.DocumentationURL) + } + + if len(parts) == 0 { + return err + } + return stderrors.New(strings.Join(parts, "\n")) } // NewGitHubGraphQLErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index 414b7008f7..c16556ea9a 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -687,3 +687,50 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { assert.Contains(t, text, "validation failed") }) } + +func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) { + t.Run("ruleset ErrorResponse includes nested validation messages", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + originalErr := &github.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusUnprocessableEntity}, + Message: "Validation Failed", + Errors: []github.Error{ + { + Resource: "GitRef", + Field: "ref", + Code: "custom", + Message: "ref name does not match the required pattern 'feature/*'", + }, + }, + DocumentationURL: "https://docs.github.com/rest/git/refs#create-a-reference", + } + + result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "failed to create branch") + assert.Contains(t, text, "HTTP 422 Validation Failed") + assert.Contains(t, text, "ref name does not match the required pattern 'feature/*'") + assert.Contains(t, text, "See https://docs.github.com/rest/git/refs#create-a-reference") + assert.NotContains(t, text, "Resource:") + }) + + t.Run("wrapped ErrorResponse is still unwrapped", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + originalErr := fmt.Errorf("create ref: %w", &github.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusUnprocessableEntity}, + Message: "Validation Failed", + Errors: []github.Error{ + {Message: "Changes must be made through a pull request."}, + }, + }) + + result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "Changes must be made through a pull request.") + assert.NotContains(t, text, "create ref:") + }) +} diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 1d488b9ced..c8b9233148 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1098,6 +1098,36 @@ func Test_CreateBranch(t *testing.T) { expectError: true, expectedErrMsg: "failed to create branch", }, + { + name: "create branch surfaces ruleset validation details", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockSourceRef), + "GET /repos/owner/repo/git/ref/heads/main": mockResponse(t, http.StatusOK, mockSourceRef), + PostReposGitRefsByOwnerByRepo: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{ + "message": "Validation Failed", + "documentation_url": "https://docs.github.com/rest/git/refs#create-a-reference", + "errors": [ + { + "resource": "GitRef", + "field": "ref", + "code": "custom", + "message": "ref name does not match the required pattern 'feature/*'" + } + ] + }`)) + }, + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "branch": "hotfix", + "from_branch": "main", + }, + expectError: true, + expectedErrMsg: "ref name does not match the required pattern 'feature/*'", + }, } for _, tc := range tests { From fed13b5b0b718b09804c8fde26ebd9bf909abb48 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 18 Aug 2026 23:51:31 +0200 Subject: [PATCH 2/2] fix(errors): safely format GitHub validation failures Limit structured formatting to HTTP 422 responses, sanitize allowlisted validation fields, and omit request, response, and documentation metadata while preserving other error contracts. Refs #3080 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error.go | 85 +++++++++++++++++++------------- pkg/errors/error_test.go | 87 ++++++++++++++++++++++++++------- pkg/github/repositories_test.go | 8 ++- 3 files changed, 128 insertions(+), 52 deletions(-) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 205da46ced..13b607b405 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/github/github-mcp-server/pkg/sanitize" "github.com/github/github-mcp-server/pkg/utils" "github.com/google/go-github/v89/github" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -192,58 +193,74 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github "%s: GitHub secondary rate limit exceeded. Wait before retrying.", message)) } - return utils.NewToolResultErrorFromErr(message, formattedGitHubAPIError(err)) + return utils.NewToolResultErrorFromErr(message, formatGitHubValidationError(resp, err)) } -// formattedGitHubAPIError unwraps a github.ErrorResponse so tool results include -// nested validation messages (for example repository ruleset violations) instead -// of go-github's compact 422 dump. -func formattedGitHubAPIError(err error) error { +// formatGitHubValidationError exposes the parsed fields of 422 responses without +// including request or response metadata from the underlying HTTP exchange. +func formatGitHubValidationError(resp *github.Response, err error) error { var ghErr *github.ErrorResponse if !stderrors.As(err, &ghErr) { return err } - var parts []string - switch { - case ghErr.Response != nil && ghErr.Response.StatusCode != 0 && ghErr.Message != "": - parts = append(parts, fmt.Sprintf("HTTP %d %s", ghErr.Response.StatusCode, ghErr.Message)) - case ghErr.Response != nil && ghErr.Response.StatusCode != 0: - parts = append(parts, fmt.Sprintf("HTTP %d", ghErr.Response.StatusCode)) - case ghErr.Message != "": - parts = append(parts, ghErr.Message) + statusCode := 0 + if ghErr.Response != nil { + statusCode = ghErr.Response.StatusCode + } + if statusCode == 0 && resp != nil { + statusCode = resp.StatusCode + } + if statusCode != http.StatusUnprocessableEntity { + return err } - for _, item := range ghErr.Errors { - detail := strings.TrimSpace(item.Message) - if detail == "" { - var bits []string - if item.Resource != "" { - bits = append(bits, item.Resource) - } - if item.Field != "" { - bits = append(bits, item.Field) - } - if item.Code != "" { - bits = append(bits, item.Code) - } - detail = strings.Join(bits, " ") - } - if detail != "" { + parts := make([]string, 0, len(ghErr.Errors)+1) + if summary := sanitizeGitHubValidationText(ghErr.Message); summary != "" { + parts = append(parts, summary) + } + for _, validationErr := range ghErr.Errors { + if detail := formatGitHubValidationDetail(validationErr); detail != "" { parts = append(parts, detail) } } - if ghErr.DocumentationURL != "" { - parts = append(parts, "See "+ghErr.DocumentationURL) - } - if len(parts) == 0 { - return err + return stderrors.New("GitHub API validation failed") } return stderrors.New(strings.Join(parts, "\n")) } +func formatGitHubValidationDetail(validationErr github.Error) string { + resource := sanitizeGitHubValidationText(validationErr.Resource) + field := sanitizeGitHubValidationText(validationErr.Field) + code := sanitizeGitHubValidationText(validationErr.Code) + message := sanitizeGitHubValidationText(validationErr.Message) + + location := strings.Trim(strings.Join([]string{resource, field}, "."), ".") + switch { + case location != "" && code != "": + location += " (" + code + ")" + case location == "": + location = code + } + + switch { + case location != "" && message != "": + return location + ": " + message + case message != "": + return message + default: + return location + } +} + +func sanitizeGitHubValidationText(value string) string { + // Tool errors are plain text; keep quoted branch patterns readable. + sanitized := strings.ReplaceAll(sanitize.Sanitize(value), "'", "'") + return strings.Join(strings.Fields(sanitized), " ") +} + // NewGitHubGraphQLErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err error) *mcp.CallToolResult { graphQLErr := newGitHubGraphQLError(message, err) diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index c16556ea9a..9938c12df9 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -689,48 +689,101 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { } func TestNewGitHubAPIErrorResponse_ValidationMessages(t *testing.T) { - t.Run("ruleset ErrorResponse includes nested validation messages", func(t *testing.T) { + t.Run("ruleset ErrorResponse includes sanitized structured validation messages", func(t *testing.T) { ctx := ContextWithGitHubErrors(context.Background()) + request, err := http.NewRequest(http.MethodPost, "https://api.github.test/repos/owner/repo/git/refs?private=secret-url-token", nil) + require.NoError(t, err) + request.Header.Set("Authorization", "Bearer secret-request-token") + response := &http.Response{ + StatusCode: http.StatusUnprocessableEntity, + Request: request, + Header: http.Header{"X-Secret": []string{"secret-response-header"}}, + } + originalErr := &github.ErrorResponse{ - Response: &http.Response{StatusCode: http.StatusUnprocessableEntity}, - Message: "Validation Failed", + Response: response, + Message: "Validation Failed\u202e", Errors: []github.Error{ { Resource: "GitRef", Field: "ref", Code: "custom", - Message: "ref name does not match the required pattern 'feature/*'", + Message: "ref name does not match the required pattern 'feature/*'\u202e", }, }, - DocumentationURL: "https://docs.github.com/rest/git/refs#create-a-reference", + DocumentationURL: "https://docs.github.test/private?token=secret-doc-token", } - result := NewGitHubAPIErrorResponse(ctx, "failed to create branch", nil, originalErr) + wrappedErr := fmt.Errorf("create ref: %w", originalErr) + result := NewGitHubAPIErrorResponse( + ctx, + "failed to create branch", + &github.Response{Response: response}, + wrappedErr, + ) text := requireErrorText(t, result) - assert.Contains(t, text, "failed to create branch") - assert.Contains(t, text, "HTTP 422 Validation Failed") - assert.Contains(t, text, "ref name does not match the required pattern 'feature/*'") - assert.Contains(t, text, "See https://docs.github.com/rest/git/refs#create-a-reference") - assert.NotContains(t, text, "Resource:") + assert.Equal(t, "failed to create branch: Validation Failed\nGitRef.ref (custom): ref name does not match the required pattern 'feature/*'", text) + assert.NotContains(t, text, "create ref") + assert.NotContains(t, text, "https://") + assert.NotContains(t, text, "secret-") + assert.NotContains(t, text, "Authorization") + assert.NotContains(t, text, "X-Secret") + assert.NotContains(t, text, "