Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 4.8k
Add raw client error annotation and annotate GetFileContents#1570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8fc11ae234565bdd7f537831b908c12abc43727f682b7d7818dad5c284b1f0c349877f0a6760d16a1445fe976d4a1b6d7b9d3173fFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -3,6 +3,7 @@ package errors | ||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "github.com/github/github-mcp-server/pkg/utils" | ||
| "github.com/google/go-github/v79/github" | ||
| @@ -44,10 +45,29 @@ func (e *GitHubGraphQLError) Error() string { | ||
| return fmt.Errorf("%s: %w", e.Message, e.Err).Error() | ||
| } | ||
| type GitHubRawAPIError struct { | ||
| Message string `json:"message"` | ||
| Response *http.Response `json:"-"` | ||
| Err error `json:"-"` | ||
| } | ||
| func newGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError { | ||
| return &GitHubRawAPIError{ | ||
| Message: message, | ||
| Response: resp, | ||
| Err: err, | ||
mattdholloway marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| } | ||
mattdholloway marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| func (e *GitHubRawAPIError) Error() string { | ||
| return fmt.Errorf("%s: %w", e.Message, e.Err).Error() | ||
| } | ||
mattdholloway marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| type GitHubErrorKey struct{} | ||
| type GitHubCtxErrors struct { | ||
| api []*GitHubAPIError | ||
| graphQL []*GitHubGraphQLError | ||
| raw []*GitHubRawAPIError | ||
| } | ||
| // 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 { | ||
| // If the context already has GitHubCtxErrors, we just empty the slices to start fresh | ||
| val.api = []*GitHubAPIError{} | ||
| val.graphQL = []*GitHubGraphQLError{} | ||
| val.raw = []*GitHubRawAPIError{} | ||
| } else { | ||
| // If not, we create a new GitHubCtxErrors and set it in the context | ||
| ctx = context.WithValue(ctx, GitHubErrorKey{}, &GitHubCtxErrors{}) | ||
| @@ -83,6 +104,14 @@ func GetGitHubGraphQLErrors(ctx context.Context) ([]*GitHubGraphQLError, error) | ||
| return nil, fmt.Errorf("context does not contain GitHubCtxErrors") | ||
| } | ||
| // GetGitHubRawAPIErrors retrieves the slice of GitHubRawAPIErrors from the context. | ||
| func GetGitHubRawAPIErrors(ctx context.Context) ([]*GitHubRawAPIError, error) { | ||
| if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { | ||
| return val.raw, nil // return the slice of raw API errors from the context | ||
| } | ||
| return nil, fmt.Errorf("context does not contain GitHubCtxErrors") | ||
| } | ||
| func NewGitHubAPIErrorToCtx(ctx context.Context, message string, resp *github.Response, err error) (context.Context, error) { | ||
| apiErr := newGitHubAPIError(message, resp, err) | ||
| if ctx != nil { | ||
| @@ -107,6 +136,15 @@ func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError | ||
| return nil, fmt.Errorf("context does not contain GitHubCtxErrors") | ||
| } | ||
| func addRawAPIErrorToContext(ctx context.Context, err *GitHubRawAPIError) (context.Context, error) { | ||
| if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { | ||
| val.raw = append(val.raw, err) | ||
| return ctx, nil | ||
| } | ||
mattdholloway marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return nil, fmt.Errorf("context does not contain GitHubCtxErrors") | ||
| } | ||
| // NewGitHubAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware | ||
| func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github.Response, err error) *mcp.CallToolResult { | ||
| apiErr := newGitHubAPIError(message, resp, err) | ||
| @@ -125,6 +163,15 @@ func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err erro | ||
| return utils.NewToolResultErrorFromErr(message, err) | ||
| } | ||
| // NewGitHubRawAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware | ||
| func NewGitHubRawAPIErrorResponse(ctx context.Context, message string, resp *http.Response, err error) *mcp.CallToolResult { | ||
| rawErr := newGitHubRawAPIError(message, resp, err) | ||
| if ctx != nil { | ||
| _, _ = addRawAPIErrorToContext(ctx, rawErr) // Explicitly ignore error for graceful handling | ||
| } | ||
| return utils.NewToolResultErrorFromErr(message, err) | ||
| } | ||
| // NewGitHubAPIStatusErrorResponse handles cases where the API call succeeds (err == nil) | ||
| // but returns an unexpected HTTP status code. It creates a synthetic error from the | ||
| // status code and response body, then records it in context for observability tracking. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -63,6 +63,33 @@ func TestGitHubErrorContext(t *testing.T) { | ||
| assert.Equal(t, "failed to execute mutation: GraphQL query failed", gqlError.Error()) | ||
| }) | ||
| t.Run("Raw API errors can be added to context and retrieved", func(t *testing.T) { | ||
| // Given a context with GitHub error tracking enabled | ||
| ctx := ContextWithGitHubErrors(context.Background()) | ||
| // Create a mock HTTP response | ||
| resp := &http.Response{ | ||
| StatusCode: 404, | ||
| Status: "404 Not Found", | ||
| } | ||
| originalErr := fmt.Errorf("raw content not found") | ||
| // When we add a raw API error to the context | ||
| rawAPIErr := newGitHubRawAPIError("failed to fetch raw content", resp, originalErr) | ||
| updatedCtx, err := addRawAPIErrorToContext(ctx, rawAPIErr) | ||
| require.NoError(t, err) | ||
| // Then we should be able to retrieve the error from the updated context | ||
| rawErrors, err := GetGitHubRawAPIErrors(updatedCtx) | ||
| require.NoError(t, err) | ||
| require.Len(t, rawErrors, 1) | ||
| rawError := rawErrors[0] | ||
| assert.Equal(t, "failed to fetch raw content", rawError.Message) | ||
| assert.Equal(t, resp, rawError.Response) | ||
| assert.Equal(t, originalErr, rawError.Err) | ||
Comment on lines
+87
to
+90
CopilotAI | ||
| }) | ||
| t.Run("multiple errors can be accumulated in context", func(t *testing.T) { | ||
| // Given a context with GitHub error tracking enabled | ||
| ctx := ContextWithGitHubErrors(context.Background()) | ||
| @@ -82,6 +109,11 @@ func TestGitHubErrorContext(t *testing.T) { | ||
| ctx, err = addGitHubGraphQLErrorToContext(ctx, gqlErr) | ||
| require.NoError(t, err) | ||
| // And add a raw API error | ||
| rawErr := newGitHubRawAPIError("raw error", &http.Response{StatusCode: 404}, fmt.Errorf("not found")) | ||
| ctx, err = addRawAPIErrorToContext(ctx, rawErr) | ||
| require.NoError(t, err) | ||
| // Then we should be able to retrieve all errors | ||
| apiErrors, err := GetGitHubAPIErrors(ctx) | ||
| require.NoError(t, err) | ||
| @@ -91,10 +123,15 @@ func TestGitHubErrorContext(t *testing.T) { | ||
| require.NoError(t, err) | ||
| assert.Len(t, gqlErrors, 1) | ||
| rawErrors, err := GetGitHubRawAPIErrors(ctx) | ||
| require.NoError(t, err) | ||
| assert.Len(t, rawErrors, 1) | ||
| // Verify error details | ||
| assert.Equal(t, "first error", apiErrors[0].Message) | ||
| assert.Equal(t, "second error", apiErrors[1].Message) | ||
| assert.Equal(t, "graphql error", gqlErrors[0].Message) | ||
| assert.Equal(t, "raw error", rawErrors[0].Message) | ||
| }) | ||
| 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) { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors") | ||
| assert.Nil(t, gqlErrors) | ||
| // Same for raw API errors | ||
| rawErrors, err := GetGitHubRawAPIErrors(ctx) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors") | ||
| assert.Nil(t, rawErrors) | ||
| }) | ||
| t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { | ||
| @@ -169,18 +212,31 @@ func TestGitHubErrorContext(t *testing.T) { | ||
| ctx, err := NewGitHubAPIErrorToCtx(ctx, "existing error", resp, fmt.Errorf("error")) | ||
| require.NoError(t, err) | ||
| // Verify error exists | ||
| // Add a raw API error too | ||
| rawErr := newGitHubRawAPIError("existing raw error", &http.Response{StatusCode: 404}, fmt.Errorf("error")) | ||
| ctx, err = addRawAPIErrorToContext(ctx, rawErr) | ||
| require.NoError(t, err) | ||
| // Verify errors exist | ||
| apiErrors, err := GetGitHubAPIErrors(ctx) | ||
| require.NoError(t, err) | ||
| assert.Len(t, apiErrors, 1) | ||
| rawErrors, err := GetGitHubRawAPIErrors(ctx) | ||
| require.NoError(t, err) | ||
| assert.Len(t, rawErrors, 1) | ||
| // When we call ContextWithGitHubErrors again | ||
| resetCtx := ContextWithGitHubErrors(ctx) | ||
| // Then the errors should be cleared | ||
| // Then all errors should be cleared | ||
| apiErrors, err = GetGitHubAPIErrors(resetCtx) | ||
| require.NoError(t, err) | ||
| assert.Len(t, apiErrors, 0, "Errors should be reset") | ||
| assert.Len(t, apiErrors, 0, "API errors should be reset") | ||
| rawErrors, err = GetGitHubRawAPIErrors(resetCtx) | ||
| require.NoError(t, err) | ||
| assert.Len(t, rawErrors, 0, "Raw API errors should be reset") | ||
| }) | ||
| t.Run("NewGitHubAPIErrorResponse creates MCP error result and stores context error", func(t *testing.T) { | ||
Uh oh!
There was an error while loading. Please reload this page.