Skip to content

Commit c80f979

Browse files
davidahmanndavidahmann
committed
errors: classify GitHub rate limit failures (#2219)
1 parent b1575ed commit c80f979

File tree

3 files changed

+176
-15
lines changed

3 files changed

+176
-15
lines changed

‎docs/error-handling.md‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@ Used for REST API errors from the GitHub API:
1919

2020
```go
2121
type GitHubAPIError struct {
22-
Message string `json:"message"`
23-
Response *github.Response `json:"-"`
24-
Err error `json:"-"`
22+
Message string `json:"message"`
23+
Code string `json:"code,omitempty"`
24+
RetryAfterSeconds *int `json:"retry_after_seconds,omitempty"`
25+
Response *github.Response `json:"-"`
26+
Err error `json:"-"`
2527
}
2628
```
2729

30+
Current HTTP classifications include authentication/scope failures plus distinct rate-limit codes such as `rate_limited`, `secondary_rate_limited`, and `abuse_rate_limited` when the upstream response provides that signal.
31+
2832
### GitHubGraphQLError
2933

3034
Used for GraphQL API errors from the GitHub API:

‎pkg/errors/error.go‎

Lines changed: 98 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,30 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7+
"strconv"
8+
"strings"
79

810
"github.com/github/github-mcp-server/pkg/utils"
911
"github.com/google/go-github/v82/github"
1012
"github.com/modelcontextprotocol/go-sdk/mcp"
1113
)
1214

1315
type GitHubAPIError struct {
14-
Message string `json:"message"`
15-
Response *github.Response `json:"-"`
16-
Err error `json:"-"`
16+
Message string `json:"message"`
17+
Code string `json:"code,omitempty"`
18+
RetryAfterSeconds *int `json:"retry_after_seconds,omitempty"`
19+
Response *github.Response `json:"-"`
20+
Err error `json:"-"`
1721
}
1822

1923
// NewGitHubAPIError creates a new GitHubAPIError with the provided message, response, and error.
2024
func newGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError {
2125
return &GitHubAPIError{
22-
Message: message,
23-
Response: resp,
24-
Err: err,
26+
Message: message,
27+
Code: classifyHTTPErrorCode(resp.Response, message, err),
28+
RetryAfterSeconds: parseRetryAfterSeconds(resp.Response),
29+
Response: resp,
30+
Err: err,
2531
}
2632
}
2733

@@ -46,23 +52,103 @@ func (e *GitHubGraphQLError) Error() string {
4652
}
4753

4854
type GitHubRawAPIError struct {
49-
Message string `json:"message"`
50-
Response *http.Response `json:"-"`
51-
Err error `json:"-"`
55+
Message string `json:"message"`
56+
Code string `json:"code,omitempty"`
57+
RetryAfterSeconds *int `json:"retry_after_seconds,omitempty"`
58+
Response *http.Response `json:"-"`
59+
Err error `json:"-"`
5260
}
5361

5462
func newGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError {
5563
return &GitHubRawAPIError{
56-
Message: message,
57-
Response: resp,
58-
Err: err,
64+
Message: message,
65+
Code: classifyHTTPErrorCode(resp, message, err),
66+
RetryAfterSeconds: parseRetryAfterSeconds(resp),
67+
Response: resp,
68+
Err: err,
5969
}
6070
}
6171

6272
func (e *GitHubRawAPIError) Error() string {
6373
return fmt.Errorf("%s: %w", e.Message, e.Err).Error()
6474
}
6575

76+
func classifyHTTPErrorCode(resp *http.Response, message string, err error) string {
77+
if resp == nil {
78+
return ""
79+
}
80+
81+
combined := strings.ToLower(strings.TrimSpace(strings.Join([]string{
82+
message,
83+
errorString(err),
84+
resp.Status,
85+
}, " ")))
86+
87+
if code := classifyRateLimitCode(resp, combined); code != "" {
88+
return code
89+
}
90+
91+
switch resp.StatusCode {
92+
case http.StatusUnauthorized:
93+
return "invalid_token"
94+
case http.StatusForbidden:
95+
if strings.Contains(combined, "scope") || strings.Contains(combined, "permission") {
96+
return "insufficient_scope"
97+
}
98+
}
99+
100+
return ""
101+
}
102+
103+
func classifyRateLimitCode(resp *http.Response, combined string) string {
104+
if resp == nil {
105+
return ""
106+
}
107+
108+
if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests {
109+
return ""
110+
}
111+
112+
switch {
113+
case strings.Contains(combined, "secondary rate limit"):
114+
return "secondary_rate_limited"
115+
case strings.Contains(combined, "abuse") || strings.Contains(combined, "abuse detection"):
116+
return "abuse_rate_limited"
117+
case resp.Header.Get("X-RateLimit-Remaining") == "0":
118+
return "rate_limited"
119+
case strings.Contains(combined, "rate limit exceeded"):
120+
return "rate_limited"
121+
default:
122+
return ""
123+
}
124+
}
125+
126+
func parseRetryAfterSeconds(resp *http.Response) *int {
127+
if resp == nil {
128+
return nil
129+
}
130+
131+
retryAfter := strings.TrimSpace(resp.Header.Get("Retry-After"))
132+
if retryAfter == "" {
133+
return nil
134+
}
135+
136+
seconds, err := strconv.Atoi(retryAfter)
137+
if err != nil {
138+
return nil
139+
}
140+
141+
return &seconds
142+
}
143+
144+
func errorString(err error) string {
145+
if err == nil {
146+
return ""
147+
}
148+
149+
return err.Error()
150+
}
151+
66152
type GitHubErrorKey struct{}
67153
type GitHubCtxErrors struct {
68154
api []*GitHubAPIError

‎pkg/errors/error_test.go‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ func TestGitHubErrorContext(t *testing.T) {
3636

3737
apiError := apiErrors[0]
3838
assert.Equal(t, "failed to fetch resource", apiError.Message)
39+
assert.Empty(t, apiError.Code)
40+
assert.Nil(t, apiError.RetryAfterSeconds)
3941
assert.Equal(t, resp, apiError.Response)
4042
assert.Equal(t, originalErr, apiError.Err)
4143
assert.Equal(t, "failed to fetch resource: resource not found", apiError.Error())
@@ -86,6 +88,8 @@ func TestGitHubErrorContext(t *testing.T) {
8688

8789
rawError := rawErrors[0]
8890
assert.Equal(t, "failed to fetch raw content", rawError.Message)
91+
assert.Empty(t, rawError.Code)
92+
assert.Nil(t, rawError.RetryAfterSeconds)
8993
assert.Equal(t, resp, rawError.Response)
9094
assert.Equal(t, originalErr, rawError.Err)
9195
})
@@ -260,6 +264,7 @@ func TestGitHubErrorContext(t *testing.T) {
260264

261265
apiError := apiErrors[0]
262266
assert.Equal(t, "API call failed", apiError.Message)
267+
assert.Empty(t, apiError.Code)
263268
assert.Equal(t, resp, apiError.Response)
264269
assert.Equal(t, originalErr, apiError.Err)
265270
})
@@ -308,6 +313,7 @@ func TestGitHubErrorContext(t *testing.T) {
308313

309314
apiError := apiErrors[0]
310315
assert.Equal(t, "failed to create issue", apiError.Message)
316+
assert.Empty(t, apiError.Code)
311317
assert.Equal(t, resp, apiError.Response)
312318
// The synthetic error should contain the status code and body
313319
assert.Contains(t, apiError.Err.Error(), "unexpected status 422")
@@ -384,6 +390,71 @@ func TestGitHubErrorTypes(t *testing.T) {
384390
var err error = gqlErr
385391
assert.Equal(t, "test message: query failed", err.Error())
386392
})
393+
394+
t.Run("GitHubAPIError classifies rate limit variants and preserves retry metadata", func(t *testing.T) {
395+
t.Run("secondary rate limit", func(t *testing.T) {
396+
resp := &github.Response{
397+
Response: &http.Response{
398+
StatusCode: http.StatusForbidden,
399+
Status: "403 Forbidden",
400+
Header: http.Header{
401+
"Retry-After": []string{"60"},
402+
},
403+
},
404+
}
405+
406+
apiErr := newGitHubAPIError("failed to search", resp, fmt.Errorf("secondary rate limit exceeded"))
407+
408+
require.NotNil(t, apiErr.RetryAfterSeconds)
409+
assert.Equal(t, "secondary_rate_limited", apiErr.Code)
410+
assert.Equal(t, 60, *apiErr.RetryAfterSeconds)
411+
})
412+
413+
t.Run("abuse rate limit", func(t *testing.T) {
414+
resp := &github.Response{
415+
Response: &http.Response{
416+
StatusCode: http.StatusForbidden,
417+
Status: "403 Forbidden",
418+
Header: http.Header{},
419+
},
420+
}
421+
422+
apiErr := newGitHubAPIError("failed to create issue", resp, fmt.Errorf("You have triggered an abuse detection mechanism"))
423+
424+
assert.Equal(t, "abuse_rate_limited", apiErr.Code)
425+
assert.Nil(t, apiErr.RetryAfterSeconds)
426+
})
427+
428+
t.Run("primary rate limit", func(t *testing.T) {
429+
resp := &github.Response{
430+
Response: &http.Response{
431+
StatusCode: http.StatusForbidden,
432+
Status: "403 Forbidden",
433+
Header: http.Header{
434+
"X-RateLimit-Remaining": []string{"0"},
435+
},
436+
},
437+
}
438+
439+
apiErr := newGitHubAPIError("failed to list issues", resp, fmt.Errorf("API rate limit exceeded"))
440+
441+
assert.Equal(t, "rate_limited", apiErr.Code)
442+
assert.Nil(t, apiErr.RetryAfterSeconds)
443+
})
444+
445+
t.Run("invalid token still classified", func(t *testing.T) {
446+
resp := &github.Response{
447+
Response: &http.Response{
448+
StatusCode: http.StatusUnauthorized,
449+
Status: "401 Unauthorized",
450+
},
451+
}
452+
453+
apiErr := newGitHubAPIError("failed to authenticate", resp, fmt.Errorf("bad credentials"))
454+
455+
assert.Equal(t, "invalid_token", apiErr.Code)
456+
})
457+
})
387458
}
388459

389460
// TestMiddlewareScenario demonstrates a realistic middleware scenario

0 commit comments

Comments
 (0)