Skip to content

Commit b79d126

Browse files
omgitsadsmattdhollowayCopilot
authored
Add raw client error annotation and annotate GetFileContents (#1570)
* Add raw client error annotation and annotate GetFileContents * Track response. * add raw errors to context * add raw api error test * Update pkg/errors/error.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add blank line after Error() method for readability * add NewGitHubRawAPIErrorResponse back --------- Co-authored-by: Matt Holloway <mattdholloway@pm.me> Co-authored-by: Matt Holloway <mattdholloway@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 63c7db0 commit b79d126

File tree

3 files changed

+107
-4
lines changed

3 files changed

+107
-4
lines changed

pkg/errors/error.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package errors
33
import (
44
"context"
55
"fmt"
6+
"net/http"
67

78
"github.com/github/github-mcp-server/pkg/utils"
89
"github.com/google/go-github/v79/github"
@@ -44,10 +45,29 @@ func (e *GitHubGraphQLError) Error() string {
4445
return fmt.Errorf("%s: %w", e.Message, e.Err).Error()
4546
}
4647

48+
type GitHubRawAPIError struct {
49+
Message string `json:"message"`
50+
Response *http.Response `json:"-"`
51+
Err error `json:"-"`
52+
}
53+
54+
func newGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError {
55+
return &GitHubRawAPIError{
56+
Message: message,
57+
Response: resp,
58+
Err: err,
59+
}
60+
}
61+
62+
func (e *GitHubRawAPIError) Error() string {
63+
return fmt.Errorf("%s: %w", e.Message, e.Err).Error()
64+
}
65+
4766
type GitHubErrorKey struct{}
4867
type GitHubCtxErrors struct {
4968
api []*GitHubAPIError
5069
graphQL []*GitHubGraphQLError
70+
raw []*GitHubRawAPIError
5171
}
5272

5373
// ContextWithGitHubErrors updates or creates a context with a pointer to GitHub error information (to be used by middleware).
@@ -59,6 +79,7 @@ func ContextWithGitHubErrors(ctx context.Context) context.Context {
5979
// If the context already has GitHubCtxErrors, we just empty the slices to start fresh
6080
val.api = []*GitHubAPIError{}
6181
val.graphQL = []*GitHubGraphQLError{}
82+
val.raw = []*GitHubRawAPIError{}
6283
} else {
6384
// If not, we create a new GitHubCtxErrors and set it in the context
6485
ctx = context.WithValue(ctx, GitHubErrorKey{}, &GitHubCtxErrors{})
@@ -83,6 +104,14 @@ func GetGitHubGraphQLErrors(ctx context.Context) ([]*GitHubGraphQLError, error)
83104
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
84105
}
85106

107+
// GetGitHubRawAPIErrors retrieves the slice of GitHubRawAPIErrors from the context.
108+
func GetGitHubRawAPIErrors(ctx context.Context) ([]*GitHubRawAPIError, error) {
109+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
110+
return val.raw, nil // return the slice of raw API errors from the context
111+
}
112+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
113+
}
114+
86115
func NewGitHubAPIErrorToCtx(ctx context.Context, message string, resp *github.Response, err error) (context.Context, error) {
87116
apiErr := newGitHubAPIError(message, resp, err)
88117
if ctx != nil {
@@ -107,6 +136,15 @@ func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError
107136
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
108137
}
109138

139+
func addRawAPIErrorToContext(ctx context.Context, err *GitHubRawAPIError) (context.Context, error) {
140+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
141+
val.raw = append(val.raw, err)
142+
return ctx, nil
143+
}
144+
145+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
146+
}
147+
110148
// NewGitHubAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware
111149
func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github.Response, err error) *mcp.CallToolResult {
112150
apiErr := newGitHubAPIError(message, resp, err)
@@ -125,6 +163,15 @@ func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err erro
125163
return utils.NewToolResultErrorFromErr(message, err)
126164
}
127165

166+
// NewGitHubRawAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware
167+
func NewGitHubRawAPIErrorResponse(ctx context.Context, message string, resp *http.Response, err error) *mcp.CallToolResult {
168+
rawErr := newGitHubRawAPIError(message, resp, err)
169+
if ctx != nil {
170+
_, _ = addRawAPIErrorToContext(ctx, rawErr) // Explicitly ignore error for graceful handling
171+
}
172+
return utils.NewToolResultErrorFromErr(message, err)
173+
}
174+
128175
// NewGitHubAPIStatusErrorResponse handles cases where the API call succeeds (err == nil)
129176
// but returns an unexpected HTTP status code. It creates a synthetic error from the
130177
// status code and response body, then records it in context for observability tracking.

pkg/errors/error_test.go

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,33 @@ func TestGitHubErrorContext(t *testing.T) {
6363
assert.Equal(t, "failed to execute mutation: GraphQL query failed", gqlError.Error())
6464
})
6565

66+
t.Run("Raw API errors can be added to context and retrieved", func(t *testing.T) {
67+
// Given a context with GitHub error tracking enabled
68+
ctx := ContextWithGitHubErrors(context.Background())
69+
70+
// Create a mock HTTP response
71+
resp := &http.Response{
72+
StatusCode: 404,
73+
Status: "404 Not Found",
74+
}
75+
originalErr := fmt.Errorf("raw content not found")
76+
77+
// When we add a raw API error to the context
78+
rawAPIErr := newGitHubRawAPIError("failed to fetch raw content", resp, originalErr)
79+
updatedCtx, err := addRawAPIErrorToContext(ctx, rawAPIErr)
80+
require.NoError(t, err)
81+
82+
// Then we should be able to retrieve the error from the updated context
83+
rawErrors, err := GetGitHubRawAPIErrors(updatedCtx)
84+
require.NoError(t, err)
85+
require.Len(t, rawErrors, 1)
86+
87+
rawError := rawErrors[0]
88+
assert.Equal(t, "failed to fetch raw content", rawError.Message)
89+
assert.Equal(t, resp, rawError.Response)
90+
assert.Equal(t, originalErr, rawError.Err)
91+
})
92+
6693
t.Run("multiple errors can be accumulated in context", func(t *testing.T) {
6794
// Given a context with GitHub error tracking enabled
6895
ctx := ContextWithGitHubErrors(context.Background())
@@ -82,6 +109,11 @@ func TestGitHubErrorContext(t *testing.T) {
82109
ctx, err = addGitHubGraphQLErrorToContext(ctx, gqlErr)
83110
require.NoError(t, err)
84111

112+
// And add a raw API error
113+
rawErr := newGitHubRawAPIError("raw error", &http.Response{StatusCode: 404}, fmt.Errorf("not found"))
114+
ctx, err = addRawAPIErrorToContext(ctx, rawErr)
115+
require.NoError(t, err)
116+
85117
// Then we should be able to retrieve all errors
86118
apiErrors, err := GetGitHubAPIErrors(ctx)
87119
require.NoError(t, err)
@@ -91,10 +123,15 @@ func TestGitHubErrorContext(t *testing.T) {
91123
require.NoError(t, err)
92124
assert.Len(t, gqlErrors, 1)
93125

126+
rawErrors, err := GetGitHubRawAPIErrors(ctx)
127+
require.NoError(t, err)
128+
assert.Len(t, rawErrors, 1)
129+
94130
// Verify error details
95131
assert.Equal(t, "first error", apiErrors[0].Message)
96132
assert.Equal(t, "second error", apiErrors[1].Message)
97133
assert.Equal(t, "graphql error", gqlErrors[0].Message)
134+
assert.Equal(t, "raw error", rawErrors[0].Message)
98135
})
99136

100137
t.Run("context pointer sharing allows middleware to inspect errors without context propagation", func(t *testing.T) {
@@ -160,6 +197,12 @@ func TestGitHubErrorContext(t *testing.T) {
160197
assert.Error(t, err)
161198
assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors")
162199
assert.Nil(t, gqlErrors)
200+
201+
// Same for raw API errors
202+
rawErrors, err := GetGitHubRawAPIErrors(ctx)
203+
assert.Error(t, err)
204+
assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors")
205+
assert.Nil(t, rawErrors)
163206
})
164207

165208
t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) {
@@ -169,18 +212,31 @@ func TestGitHubErrorContext(t *testing.T) {
169212
ctx, err := NewGitHubAPIErrorToCtx(ctx, "existing error", resp, fmt.Errorf("error"))
170213
require.NoError(t, err)
171214

172-
// Verify error exists
215+
// Add a raw API error too
216+
rawErr := newGitHubRawAPIError("existing raw error", &http.Response{StatusCode: 404}, fmt.Errorf("error"))
217+
ctx, err = addRawAPIErrorToContext(ctx, rawErr)
218+
require.NoError(t, err)
219+
220+
// Verify errors exist
173221
apiErrors, err := GetGitHubAPIErrors(ctx)
174222
require.NoError(t, err)
175223
assert.Len(t, apiErrors, 1)
176224

225+
rawErrors, err := GetGitHubRawAPIErrors(ctx)
226+
require.NoError(t, err)
227+
assert.Len(t, rawErrors, 1)
228+
177229
// When we call ContextWithGitHubErrors again
178230
resetCtx := ContextWithGitHubErrors(ctx)
179231

180-
// Then the errors should be cleared
232+
// Then all errors should be cleared
181233
apiErrors, err = GetGitHubAPIErrors(resetCtx)
182234
require.NoError(t, err)
183-
assert.Len(t, apiErrors, 0, "Errors should be reset")
235+
assert.Len(t, apiErrors, 0, "API errors should be reset")
236+
237+
rawErrors, err = GetGitHubRawAPIErrors(resetCtx)
238+
require.NoError(t, err)
239+
assert.Len(t, rawErrors, 0, "Raw API errors should be reset")
184240
})
185241

186242
t.Run("NewGitHubAPIErrorResponse creates MCP error result and stores context error", func(t *testing.T) {

pkg/github/repositories.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool
724724
// If the raw content is found, return it directly
725725
body, err := io.ReadAll(resp.Body)
726726
if err != nil {
727-
return utils.NewToolResultError("failed to read response body"), nil, nil
727+
return ghErrors.NewGitHubRawAPIErrorResponse(ctx, "failed to get raw repository content", resp, err), nil, nil
728728
}
729729
contentType := resp.Header.Get("Content-Type")
730730

0 commit comments

Comments
 (0)