Skip to content

Commit bc5d08d

Browse files
SamMorrowDrumsSamMorrowDrums
authored
fix: track HTTP status code errors in context for observability (#1630)
Add NewGitHubAPIStatusErrorResponse helper function to properly track GitHub API errors when the API call succeeds but returns an unexpected HTTP status code (e.g., 404, 422, 500). Previously, these errors were returned via utils.NewToolResultError which bypasses the context-based error tracking used by the remote server's error_categorizer.go for observability metrics. This resulted in 100% tool call success rates in observability even when errors occurred. The fix adds a new helper function that: 1. Creates a synthetic error from the status code and response body 2. Records the error in context via NewGitHubAPIErrorResponse 3. Returns the MCP error result to the client Updated all tool files to use the new pattern for status code errors: - pullrequests.go: 12 fixes - repositories.go: 18 fixes - issues.go: 10 fixes - notifications.go: 6 fixes - projects.go: 5 fixes - search.go: 3 fixes - search_utils.go: 1 fix - gists.go: 4 fixes - code_scanning.go: 2 fixes - dependabot.go: 2 fixes - secret_scanning.go: 2 fixes - security_advisories.go: 4 fixes Total: ~69 error paths now properly tracked. Note: Parameter validation errors (RequiredParam failures) and internal I/O errors (io.ReadAll failures) intentionally continue to use utils.NewToolResultError as they are not GitHub API errors.
1 parent b820435 commit bc5d08d

File tree

14 files changed

+108
-72
lines changed

14 files changed

+108
-72
lines changed

‎pkg/errors/error.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,3 +124,11 @@ func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err erro
124124
}
125125
return utils.NewToolResultErrorFromErr(message, err)
126126
}
127+
128+
// NewGitHubAPIStatusErrorResponse handles cases where the API call succeeds (err == nil)
129+
// but returns an unexpected HTTP status code. It creates a synthetic error from the
130+
// status code and response body, then records it in context for observability tracking.
131+
func NewGitHubAPIStatusErrorResponse(ctx context.Context, message string, resp *github.Response, body []byte) *mcp.CallToolResult {
132+
err := fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body))
133+
return NewGitHubAPIErrorResponse(ctx, message, resp, err)
134+
}

‎pkg/errors/error_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,33 @@ func TestGitHubErrorContext(t *testing.T) {
231231
assert.Equal(t, originalErr, gqlError.Err)
232232
})
233233

234+
t.Run("NewGitHubAPIStatusErrorResponse creates MCP error result from status code", func(t *testing.T) {
235+
// Given a context with GitHub error tracking enabled
236+
ctx := ContextWithGitHubErrors(context.Background())
237+
238+
resp := &github.Response{Response: &http.Response{StatusCode: 422}}
239+
body := []byte(`{"message": "Validation Failed"}`)
240+
241+
// When we create a status error response
242+
result := NewGitHubAPIStatusErrorResponse(ctx, "failed to create issue", resp, body)
243+
244+
// Then it should return an MCP error result
245+
require.NotNil(t, result)
246+
assert.True(t, result.IsError)
247+
248+
// And the error should be stored in the context
249+
apiErrors, err := GetGitHubAPIErrors(ctx)
250+
require.NoError(t, err)
251+
require.Len(t, apiErrors, 1)
252+
253+
apiError := apiErrors[0]
254+
assert.Equal(t, "failed to create issue", apiError.Message)
255+
assert.Equal(t, resp, apiError.Response)
256+
// The synthetic error should contain the status code and body
257+
assert.Contains(t, apiError.Err.Error(), "unexpected status 422")
258+
assert.Contains(t, apiError.Err.Error(), "Validation Failed")
259+
})
260+
234261
t.Run("NewGitHubAPIErrorToCtx with uninitialized context does not error", func(t *testing.T) {
235262
// Given a regular context without GitHub error tracking initialized
236263
ctx := context.Background()

‎pkg/github/code_scanning.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package github
33
import (
44
"context"
55
"encoding/json"
6-
"fmt"
76
"io"
87
"net/http"
98

@@ -80,7 +79,7 @@ func GetCodeScanningAlert(t translations.TranslationHelperFunc) inventory.Server
8079
if err != nil {
8180
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
8281
}
83-
return utils.NewToolResultError(fmt.Sprintf("failed to get alert: %s", string(body))), nil, nil
82+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get alert", resp, body), nil, nil
8483
}
8584

8685
r, err := json.Marshal(alert)
@@ -184,7 +183,7 @@ func ListCodeScanningAlerts(t translations.TranslationHelperFunc) inventory.Serv
184183
if err != nil {
185184
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
186185
}
187-
return utils.NewToolResultError(fmt.Sprintf("failed to list alerts: %s", string(body))), nil, nil
186+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list alerts", resp, body), nil, nil
188187
}
189188

190189
r, err := json.Marshal(alerts)

‎pkg/github/dependabot.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func GetDependabotAlert(t translations.TranslationHelperFunc) inventory.ServerTo
8080
if err != nil {
8181
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, err
8282
}
83-
return utils.NewToolResultError(fmt.Sprintf("failed to get alert: %s", string(body))), nil, nil
83+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get alert", resp, body), nil, nil
8484
}
8585

8686
r, err := json.Marshal(alert)
@@ -172,7 +172,7 @@ func ListDependabotAlerts(t translations.TranslationHelperFunc) inventory.Server
172172
if err != nil {
173173
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, err
174174
}
175-
return utils.NewToolResultError(fmt.Sprintf("failed to list alerts: %s", string(body))), nil, nil
175+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list alerts", resp, body), nil, nil
176176
}
177177

178178
r, err := json.Marshal(alerts)

‎pkg/github/gists.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func ListGists(t translations.TranslationHelperFunc) inventory.ServerTool {
9090
if err != nil {
9191
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
9292
}
93-
return utils.NewToolResultError(fmt.Sprintf("failed to list gists: %s", string(body))), nil, nil
93+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list gists", resp, body), nil, nil
9494
}
9595

9696
r, err := json.Marshal(gists)
@@ -149,7 +149,7 @@ func GetGist(t translations.TranslationHelperFunc) inventory.ServerTool {
149149
if err != nil {
150150
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
151151
}
152-
return utils.NewToolResultError(fmt.Sprintf("failed to get gist: %s", string(body))), nil, nil
152+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get gist", resp, body), nil, nil
153153
}
154154

155155
r, err := json.Marshal(gist)
@@ -248,7 +248,7 @@ func CreateGist(t translations.TranslationHelperFunc) inventory.ServerTool {
248248
if err != nil {
249249
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
250250
}
251-
return utils.NewToolResultError(fmt.Sprintf("failed to create gist: %s", string(body))), nil, nil
251+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create gist", resp, body), nil, nil
252252
}
253253

254254
minimalResponse := MinimalResponse{
@@ -350,7 +350,7 @@ func UpdateGist(t translations.TranslationHelperFunc) inventory.ServerTool {
350350
if err != nil {
351351
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
352352
}
353-
return utils.NewToolResultError(fmt.Sprintf("failed to update gist: %s", string(body))), nil, nil
353+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update gist", resp, body), nil, nil
354354
}
355355

356356
minimalResponse := MinimalResponse{

‎pkg/github/issues.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func GetIssue(ctx context.Context, client *github.Client, cache *lockdown.RepoAc
340340
if err != nil {
341341
return nil, fmt.Errorf("failed to read response body: %w", err)
342342
}
343-
return utils.NewToolResultError(fmt.Sprintf("failed to get issue: %s", string(body))), nil
343+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get issue", resp, body), nil
344344
}
345345

346346
if flags.LockdownMode {
@@ -396,7 +396,7 @@ func GetIssueComments(ctx context.Context, client *github.Client, cache *lockdow
396396
if err != nil {
397397
return nil, fmt.Errorf("failed to read response body: %w", err)
398398
}
399-
return utils.NewToolResultError(fmt.Sprintf("failed to get issue comments: %s", string(body))), nil
399+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get issue comments", resp, body), nil
400400
}
401401
if flags.LockdownMode {
402402
if cache == nil {
@@ -455,7 +455,7 @@ func GetSubIssues(ctx context.Context, client *github.Client, cache *lockdown.Re
455455
if err != nil {
456456
return nil, fmt.Errorf("failed to read response body: %w", err)
457457
}
458-
return utils.NewToolResultError(fmt.Sprintf("failed to list sub-issues: %s", string(body))), nil
458+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list sub-issues", resp, body), nil
459459
}
460460

461461
if featureFlags.LockdownMode {
@@ -588,7 +588,7 @@ func ListIssueTypes(t translations.TranslationHelperFunc) inventory.ServerTool {
588588
if err != nil {
589589
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
590590
}
591-
return utils.NewToolResultError(fmt.Sprintf("failed to list issue types: %s", string(body))), nil, nil
591+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list issue types", resp, body), nil, nil
592592
}
593593

594594
r, err := json.Marshal(issueTypes)
@@ -673,7 +673,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
673673
if err != nil {
674674
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
675675
}
676-
return utils.NewToolResultError(fmt.Sprintf("failed to create comment: %s", string(body))), nil, nil
676+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create comment", resp, body), nil, nil
677677
}
678678

679679
r, err := json.Marshal(createdComment)
@@ -823,7 +823,7 @@ func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo
823823
if err != nil {
824824
return nil, fmt.Errorf("failed to read response body: %w", err)
825825
}
826-
return utils.NewToolResultError(fmt.Sprintf("failed to add sub-issue: %s", string(body))), nil
826+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", resp, body), nil
827827
}
828828

829829
r, err := json.Marshal(subIssue)
@@ -855,7 +855,7 @@ func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, re
855855
if err != nil {
856856
return nil, fmt.Errorf("failed to read response body: %w", err)
857857
}
858-
return utils.NewToolResultError(fmt.Sprintf("failed to remove sub-issue: %s", string(body))), nil
858+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to remove sub-issue", resp, body), nil
859859
}
860860

861861
r, err := json.Marshal(subIssue)
@@ -904,7 +904,7 @@ func ReprioritizeSubIssue(ctx context.Context, client *github.Client, owner stri
904904
if err != nil {
905905
return nil, fmt.Errorf("failed to read response body: %w", err)
906906
}
907-
return utils.NewToolResultError(fmt.Sprintf("failed to reprioritize sub-issue: %s", string(body))), nil
907+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to reprioritize sub-issue", resp, body), nil
908908
}
909909

910910
r, err := json.Marshal(subIssue)
@@ -1195,7 +1195,7 @@ func CreateIssue(ctx context.Context, client *github.Client, owner string, repo
11951195
if err != nil {
11961196
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil
11971197
}
1198-
return utils.NewToolResultError(fmt.Sprintf("failed to create issue: %s", string(body))), nil
1198+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create issue", resp, body), nil
11991199
}
12001200

12011201
// Return minimal response with just essential information
@@ -1256,7 +1256,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4
12561256
if err != nil {
12571257
return nil, fmt.Errorf("failed to read response body: %w", err)
12581258
}
1259-
return utils.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil
1259+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil
12601260
}
12611261

12621262
// Use GraphQL API for state updates

‎pkg/github/notifications.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ func ListNotifications(t translations.TranslationHelperFunc) inventory.ServerToo
147147
if err != nil {
148148
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
149149
}
150-
return utils.NewToolResultError(fmt.Sprintf("failed to get notifications: %s", string(body))), nil, nil
150+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get notifications", resp, body), nil, nil
151151
}
152152

153153
// Marshal response to JSON
@@ -236,7 +236,7 @@ func DismissNotification(t translations.TranslationHelperFunc) inventory.ServerT
236236
if err != nil {
237237
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
238238
}
239-
return utils.NewToolResultError(fmt.Sprintf("failed to mark notification as %s: %s", state, string(body))), nil, nil
239+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, fmt.Sprintf("failed to mark notification as %s", state), resp, body), nil, nil
240240
}
241241

242242
return utils.NewToolResultText(fmt.Sprintf("Notification marked as %s", state)), nil, nil
@@ -329,7 +329,7 @@ func MarkAllNotificationsRead(t translations.TranslationHelperFunc) inventory.Se
329329
if err != nil {
330330
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
331331
}
332-
return utils.NewToolResultError(fmt.Sprintf("failed to mark all notifications as read: %s", string(body))), nil, nil
332+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to mark all notifications as read", resp, body), nil, nil
333333
}
334334

335335
return utils.NewToolResultText("All notifications marked as read"), nil, nil
@@ -387,7 +387,7 @@ func GetNotificationDetails(t translations.TranslationHelperFunc) inventory.Serv
387387
if err != nil {
388388
return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
389389
}
390-
return utils.NewToolResultError(fmt.Sprintf("failed to get notification details: %s", string(body))), nil, nil
390+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get notification details", resp, body), nil, nil
391391
}
392392

393393
r, err := json.Marshal(thread)
@@ -481,7 +481,7 @@ func ManageNotificationSubscription(t translations.TranslationHelperFunc) invent
481481

482482
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
483483
body, _ := io.ReadAll(resp.Body)
484-
return utils.NewToolResultError(fmt.Sprintf("failed to %s notification subscription: %s", action, string(body))), nil, nil
484+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, fmt.Sprintf("failed to %s notification subscription", action), resp, body), nil, nil
485485
}
486486

487487
if action == NotificationActionDelete {
@@ -589,7 +589,7 @@ func ManageRepositoryNotificationSubscription(t translations.TranslationHelperFu
589589
// Handle non-2xx status codes
590590
if resp != nil && (resp.StatusCode < 200 || resp.StatusCode >= 300) {
591591
body, _ := io.ReadAll(resp.Body)
592-
return utils.NewToolResultError(fmt.Sprintf("failed to %s repository subscription: %s", action, string(body))), nil, nil
592+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, fmt.Sprintf("failed to %s repository subscription", action), resp, body), nil, nil
593593
}
594594

595595
if action == RepositorySubscriptionActionDelete {

‎pkg/github/projects.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func GetProject(t translations.TranslationHelperFunc) inventory.ServerTool {
219219
if err != nil {
220220
return nil, nil, fmt.Errorf("failed to read response body: %w", err)
221221
}
222-
return utils.NewToolResultError(fmt.Sprintf("failed to get project: %s", string(body))), nil, nil
222+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get project", resp, body), nil, nil
223223
}
224224

225225
minimalProject := convertToMinimalProject(project)
@@ -423,7 +423,7 @@ func GetProjectField(t translations.TranslationHelperFunc) inventory.ServerTool
423423
if err != nil {
424424
return nil, nil, fmt.Errorf("failed to read response body: %w", err)
425425
}
426-
return utils.NewToolResultError(fmt.Sprintf("failed to get project field: %s", string(body))), nil, nil
426+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get project field", resp, body), nil, nil
427427
}
428428
r, err := json.Marshal(projectField)
429429
if err != nil {
@@ -782,7 +782,7 @@ func AddProjectItem(t translations.TranslationHelperFunc) inventory.ServerTool {
782782
if err != nil {
783783
return nil, nil, fmt.Errorf("failed to read response body: %w", err)
784784
}
785-
return utils.NewToolResultError(fmt.Sprintf("%s: %s", ProjectAddFailedError, string(body))), nil, nil
785+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, ProjectAddFailedError, resp, body), nil, nil
786786
}
787787
r, err := json.Marshal(addedItem)
788788
if err != nil {
@@ -896,7 +896,7 @@ func UpdateProjectItem(t translations.TranslationHelperFunc) inventory.ServerToo
896896
if err != nil {
897897
return nil, nil, fmt.Errorf("failed to read response body: %w", err)
898898
}
899-
return utils.NewToolResultError(fmt.Sprintf("%s: %s", ProjectUpdateFailedError, string(body))), nil, nil
899+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, ProjectUpdateFailedError, resp, body), nil, nil
900900
}
901901
r, err := json.Marshal(updatedItem)
902902
if err != nil {
@@ -988,7 +988,7 @@ func DeleteProjectItem(t translations.TranslationHelperFunc) inventory.ServerToo
988988
if err != nil {
989989
return nil, nil, fmt.Errorf("failed to read response body: %w", err)
990990
}
991-
return utils.NewToolResultError(fmt.Sprintf("%s: %s", ProjectDeleteFailedError, string(body))), nil, nil
991+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, ProjectDeleteFailedError, resp, body), nil, nil
992992
}
993993
return utils.NewToolResultText("project item successfully deleted"), nil, nil
994994
}

0 commit comments

Comments
 (0)