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.
This commit is contained in:
committed by
Sam Morrow
parent
21c5a6f1dd
commit
4e6329eb46
+51
-1
@@ -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
|
||||
|
||||
@@ -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:")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user