From fad946fb2c77755f287a513965efefd8bcca4d37 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:08:36 +0000 Subject: [PATCH 1/2] Initial plan From df19fc28ea048cfcf7a111240484c4beee38547e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:22:37 +0000 Subject: [PATCH 2/2] Replace context-based error handling with CallToolResult.SetError Migrate from storing GitHub API errors in context via middleware to embedding typed errors directly in CallToolResult using the Go SDK v1.3.0 SetError/GetError API. Key changes: - NewGitHubAPIErrorResponse/NewGitHubGraphQLErrorResponse/ NewGitHubRawAPIErrorResponse now use result.SetError() with typed errors instead of storing in context - Add Unwrap() to all error types for errors.As/errors.Is support - Export error constructors (NewGitHubAPIError, etc.) for direct use - Remove context-based error infrastructure (ContextWithGitHubErrors, GetGitHubAPIErrors, GitHubCtxErrors, etc.) - Remove addGitHubAPIErrorToContext middleware from server.go - Remove NewGitHubAPIErrorToCtx/NewGitHubGraphQLErrorToCtx and their callers in repositories_helper.go and actions.go - Update tests to verify SetError/GetError and errors.As extraction - Update error-handling.md documentation Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com> --- docs/error-handling.md | 94 ++++-- internal/ghmcp/server.go | 3 - pkg/errors/error.go | 157 +++------ pkg/errors/error_test.go | 516 ++++++++---------------------- pkg/github/actions.go | 5 +- pkg/github/repositories_helper.go | 18 +- pkg/github/server.go | 15 +- 7 files changed, 250 insertions(+), 558 deletions(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index 9bb27e0fa..07a6adf66 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -1,15 +1,15 @@ # Error Handling -This document describes the error handling patterns used in the GitHub MCP Server, specifically how we handle GitHub API errors and avoid direct use of mcp-go error types. +This document describes the error handling patterns used in the GitHub MCP Server, specifically how we handle GitHub API errors using the MCP SDK's `SetError`/`GetError` mechanism. ## Overview -The GitHub MCP Server implements a custom error handling approach that serves two primary purposes: +The GitHub MCP Server uses the Go SDK's `CallToolResult.SetError()` to embed typed GitHub API errors directly in tool results. This approach enables: 1. **Tool Response Generation**: Return appropriate MCP tool error responses to clients -2. **Middleware Inspection**: Store detailed error information in the request context for middleware analysis +2. **Error Type Inspection**: Consumers can use `result.GetError()` with `errors.As` to extract typed errors for analysis -This dual approach enables better observability and debugging capabilities, particularly for remote server deployments where understanding the nature of failures (rate limiting, authentication, 404s, 500s, etc.) is crucial for validation and monitoring. +This is powered by the Go SDK v1.3.0+ `SetError`/`GetError` methods on `CallToolResult`, which embed a Go `error` in the result alongside the error text content. ## Error Types @@ -36,11 +36,23 @@ type GitHubGraphQLError struct { } ``` +### GitHubRawAPIError + +Used for raw HTTP API errors from the GitHub API: + +```go +type GitHubRawAPIError struct { + Message string `json:"message"` + Response *http.Response `json:"-"` + Err error `json:"-"` +} +``` + ## Usage Patterns ### For GitHub REST API Errors -Instead of directly returning `mcp.NewToolResultError()`, use: +When a GitHub REST API call fails, use: ```go return ghErrors.NewGitHubAPIErrorResponse(ctx, message, response, err), nil @@ -48,8 +60,8 @@ return ghErrors.NewGitHubAPIErrorResponse(ctx, message, response, err), nil This function: - Creates a `GitHubAPIError` with the provided message, response, and error -- Stores the error in the context for middleware inspection -- Returns an appropriate MCP tool error response +- Calls `result.SetError()` to embed the typed error in the tool result +- Returns a `CallToolResult` with `IsError: true` and error text content ### For GitHub GraphQL API Errors @@ -57,17 +69,28 @@ This function: return ghErrors.NewGitHubGraphQLErrorResponse(ctx, message, err), nil ``` -### Context Management - -The error handling system uses context to store errors for later inspection: +### For Raw HTTP API Errors ```go -// Initialize context with error tracking -ctx = errors.ContextWithGitHubErrors(ctx) +return ghErrors.NewGitHubRawAPIErrorResponse(ctx, message, response, err), nil +``` + +### Extracting Errors from Results -// Retrieve errors for inspection (typically in middleware) -apiErrors, err := errors.GetGitHubAPIErrors(ctx) -graphqlErrors, err := errors.GetGitHubGraphQLErrors(ctx) +Consumers (such as middleware or the remote server) can extract typed errors from results: + +```go +if err := result.GetError(); err != nil { + var apiErr *errors.GitHubAPIError + if errors.As(err, &apiErr) { + // Access apiErr.Response.StatusCode, apiErr.Message, etc. + } + + var gqlErr *errors.GitHubGraphQLError + if errors.As(err, &gqlErr) { + // Access gqlErr.Message, gqlErr.Err, etc. + } +} ``` ## Design Principles @@ -77,20 +100,20 @@ graphqlErrors, err := errors.GetGitHubGraphQLErrors(ctx) - **User-actionable errors** (authentication failures, rate limits, 404s) should be returned as failed tool calls using the error response functions - **Developer errors** (JSON marshaling failures, internal logic errors) should be returned as actual Go errors that bubble up through the MCP framework -### Context Limitations - -This approach was designed to work around current limitations in mcp-go where context is not propagated through each step of request processing. By storing errors in context values, middleware can inspect them without requiring context propagation. - -### Graceful Error Handling +### Type Safety with SetError/GetError -Error storage operations in context are designed to fail gracefully - if context storage fails, the tool will still return an appropriate error response to the client. +All GitHub API error types implement the `error` interface with `Unwrap()` support, enabling: +- `errors.As()` to extract the specific error type (e.g., `*GitHubAPIError`) +- `errors.Is()` to check for the underlying cause +- Standard Go error handling patterns ## Benefits -1. **Observability**: Middleware can inspect the specific types of GitHub API errors occurring -2. **Debugging**: Detailed error information is preserved without exposing potentially sensitive data in logs -3. **Validation**: Remote servers can use error types and HTTP status codes to validate that changes don't break functionality -4. **Privacy**: Error inspection can be done programmatically using `errors.Is` checks without logging PII +1. **Type Safety**: Errors are embedded in the result as typed Go errors, not just strings +2. **Observability**: Middleware can inspect the specific types of GitHub API errors using `errors.As` +3. **Simplicity**: No context-based error storage or middleware setup required +4. **Debugging**: Detailed error information (HTTP status codes, response objects) is preserved +5. **Privacy**: Error inspection can be done programmatically using `errors.Is`/`errors.As` checks ## Example Implementation @@ -102,12 +125,12 @@ func GetIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (tool if err != nil { return mcp.NewToolResultError(err.Error()), nil } - + client, err := getClient(ctx) if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) } - + issue, resp, err := client.Issues.Get(ctx, owner, repo, issueNumber) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -116,10 +139,23 @@ func GetIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (tool err, ), nil } - + return MarshalledTextResult(issue), nil } } ``` -This approach ensures that both the client receives an appropriate error response and any middleware can inspect the underlying GitHub API error for monitoring and debugging purposes. +The error can then be inspected by consumers: + +```go +result, err := handler(ctx, request) +if err == nil && result.IsError { + if apiErr := result.GetError(); apiErr != nil { + var ghErr *errors.GitHubAPIError + if errors.As(apiErr, &ghErr) { + log.Printf("GitHub API error: status=%d message=%s", + ghErr.Response.StatusCode, ghErr.Message) + } + } +} +``` diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 6f5ba4e45..f141ca564 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -12,7 +12,6 @@ import ( "syscall" "time" - "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/transport" "github.com/github/github-mcp-server/pkg/inventory" @@ -298,8 +297,6 @@ func RunStdioServer(cfg StdioServerConfig) error { in, out = loggedIO, loggedIO } - // enable GitHub errors in the context - ctx := errors.ContextWithGitHubErrors(ctx) errC <- ghServer.Run(ctx, &mcp.IOTransport{Reader: in, Writer: out}) }() diff --git a/pkg/errors/error.go b/pkg/errors/error.go index d75765159..f6cecda89 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -5,11 +5,12 @@ import ( "fmt" "net/http" - "github.com/github/github-mcp-server/pkg/utils" "github.com/google/go-github/v82/github" "github.com/modelcontextprotocol/go-sdk/mcp" ) +// GitHubAPIError represents an error from the GitHub REST API. +// Use errors.As to extract this from a CallToolResult.GetError(). type GitHubAPIError struct { Message string `json:"message"` Response *github.Response `json:"-"` @@ -17,7 +18,7 @@ type GitHubAPIError struct { } // NewGitHubAPIError creates a new GitHubAPIError with the provided message, response, and error. -func newGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError { +func NewGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError { return &GitHubAPIError{ Message: message, Response: resp, @@ -29,12 +30,19 @@ func (e *GitHubAPIError) Error() string { return fmt.Errorf("%s: %w", e.Message, e.Err).Error() } +func (e *GitHubAPIError) Unwrap() error { + return e.Err +} + +// GitHubGraphQLError represents an error from the GitHub GraphQL API. +// Use errors.As to extract this from a CallToolResult.GetError(). type GitHubGraphQLError struct { Message string `json:"message"` Err error `json:"-"` } -func newGitHubGraphQLError(message string, err error) *GitHubGraphQLError { +// NewGitHubGraphQLError creates a new GitHubGraphQLError with the provided message and error. +func NewGitHubGraphQLError(message string, err error) *GitHubGraphQLError { return &GitHubGraphQLError{ Message: message, Err: err, @@ -45,13 +53,20 @@ func (e *GitHubGraphQLError) Error() string { return fmt.Errorf("%s: %w", e.Message, e.Err).Error() } +func (e *GitHubGraphQLError) Unwrap() error { + return e.Err +} + +// GitHubRawAPIError represents an error from a raw HTTP GitHub API call. +// Use errors.As to extract this from a CallToolResult.GetError(). type GitHubRawAPIError struct { Message string `json:"message"` Response *http.Response `json:"-"` Err error `json:"-"` } -func newGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError { +// NewGitHubRawAPIError creates a new GitHubRawAPIError with the provided message, response, and error. +func NewGitHubRawAPIError(message string, resp *http.Response, err error) *GitHubRawAPIError { return &GitHubRawAPIError{ Message: message, Response: resp, @@ -63,126 +78,40 @@ func (e *GitHubRawAPIError) Error() string { return fmt.Errorf("%s: %w", e.Message, e.Err).Error() } -type GitHubErrorKey struct{} -type GitHubCtxErrors struct { - api []*GitHubAPIError - graphQL []*GitHubGraphQLError - raw []*GitHubRawAPIError +func (e *GitHubRawAPIError) Unwrap() error { + return e.Err } -// ContextWithGitHubErrors updates or creates a context with a pointer to GitHub error information (to be used by middleware). -func ContextWithGitHubErrors(ctx context.Context) context.Context { - if ctx == nil { - ctx = context.Background() - } - if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { - // 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{}) - } - - return ctx +// NewGitHubAPIErrorResponse returns a CallToolResult with the error set via SetError, +// embedding a typed GitHubAPIError accessible via result.GetError() and errors.As. +func NewGitHubAPIErrorResponse(_ context.Context, message string, resp *github.Response, err error) *mcp.CallToolResult { + apiErr := NewGitHubAPIError(message, resp, err) + var result mcp.CallToolResult + result.SetError(apiErr) + return &result } -// GetGitHubAPIErrors retrieves the slice of GitHubAPIErrors from the context. -func GetGitHubAPIErrors(ctx context.Context) ([]*GitHubAPIError, error) { - if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { - return val.api, nil // return the slice of API errors from the context - } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") +// NewGitHubGraphQLErrorResponse returns a CallToolResult with the error set via SetError, +// embedding a typed GitHubGraphQLError accessible via result.GetError() and errors.As. +func NewGitHubGraphQLErrorResponse(_ context.Context, message string, err error) *mcp.CallToolResult { + graphQLErr := NewGitHubGraphQLError(message, err) + var result mcp.CallToolResult + result.SetError(graphQLErr) + return &result } -// GetGitHubGraphQLErrors retrieves the slice of GitHubGraphQLErrors from the context. -func GetGitHubGraphQLErrors(ctx context.Context) ([]*GitHubGraphQLError, error) { - if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { - return val.graphQL, nil // return the slice of GraphQL errors from the context - } - 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 { - _, _ = addGitHubAPIErrorToContext(ctx, apiErr) // Explicitly ignore error for graceful handling - } - return ctx, nil -} - -func NewGitHubGraphQLErrorToCtx(ctx context.Context, message string, err error) (context.Context, error) { - graphQLErr := newGitHubGraphQLError(message, err) - if ctx != nil { - _, _ = addGitHubGraphQLErrorToContext(ctx, graphQLErr) // Explicitly ignore error for graceful handling - } - return ctx, nil -} - -func addGitHubAPIErrorToContext(ctx context.Context, err *GitHubAPIError) (context.Context, error) { - if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { - val.api = append(val.api, err) // append the error to the existing slice in the context - return ctx, nil - } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") -} - -func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError) (context.Context, error) { - if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { - val.graphQL = append(val.graphQL, err) // append the error to the existing slice in the context - return ctx, nil - } - 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 - } - - 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) - if ctx != nil { - _, _ = addGitHubAPIErrorToContext(ctx, apiErr) // Explicitly ignore error for graceful handling - } - return utils.NewToolResultErrorFromErr(message, err) -} - -// NewGitHubGraphQLErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware -func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err error) *mcp.CallToolResult { - graphQLErr := newGitHubGraphQLError(message, err) - if ctx != nil { - _, _ = addGitHubGraphQLErrorToContext(ctx, graphQLErr) // Explicitly ignore error for graceful handling - } - 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) +// NewGitHubRawAPIErrorResponse returns a CallToolResult with the error set via SetError, +// embedding a typed GitHubRawAPIError accessible via result.GetError() and errors.As. +func NewGitHubRawAPIErrorResponse(_ context.Context, message string, resp *http.Response, err error) *mcp.CallToolResult { + rawErr := NewGitHubRawAPIError(message, resp, err) + var result mcp.CallToolResult + result.SetError(rawErr) + return &result } // 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. +// status code and response body, then sets it on the result via SetError. func NewGitHubAPIStatusErrorResponse(ctx context.Context, message string, resp *github.Response, body []byte) *mcp.CallToolResult { err := fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body)) return NewGitHubAPIErrorResponse(ctx, message, resp, err) diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index e33d5bd39..73a3276cf 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -2,6 +2,7 @@ package errors import ( "context" + "errors" "fmt" "net/http" "testing" @@ -11,452 +12,205 @@ import ( "github.com/stretchr/testify/require" ) -func TestGitHubErrorContext(t *testing.T) { - t.Run("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 GitHub response - resp := &github.Response{ - Response: &http.Response{ - StatusCode: 404, - Status: "404 Not Found", - }, - } - originalErr := fmt.Errorf("resource not found") - - // When we add an API error to the context - updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed to fetch resource", resp, originalErr) - require.NoError(t, err) - - // Then we should be able to retrieve the error from the updated context - apiErrors, err := GetGitHubAPIErrors(updatedCtx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) - - apiError := apiErrors[0] - assert.Equal(t, "failed to fetch resource", apiError.Message) - assert.Equal(t, resp, apiError.Response) - assert.Equal(t, originalErr, apiError.Err) - assert.Equal(t, "failed to fetch resource: resource not found", apiError.Error()) - }) - - t.Run("GraphQL errors can be added to context and retrieved", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - - originalErr := fmt.Errorf("GraphQL query failed") - - // When we add a GraphQL error to the context - graphQLErr := newGitHubGraphQLError("failed to execute mutation", originalErr) - updatedCtx, err := addGitHubGraphQLErrorToContext(ctx, graphQLErr) - require.NoError(t, err) - - // Then we should be able to retrieve the error from the updated context - gqlErrors, err := GetGitHubGraphQLErrors(updatedCtx) - require.NoError(t, err) - require.Len(t, gqlErrors, 1) +func TestGitHubErrorTypes(t *testing.T) { + t.Run("GitHubAPIError implements error interface", func(t *testing.T) { + resp := &github.Response{Response: &http.Response{StatusCode: 404}} + originalErr := fmt.Errorf("not found") - gqlError := gqlErrors[0] - assert.Equal(t, "failed to execute mutation", gqlError.Message) - assert.Equal(t, originalErr, gqlError.Err) - assert.Equal(t, "failed to execute mutation: GraphQL query failed", gqlError.Error()) - }) + apiErr := NewGitHubAPIError("test message", resp, originalErr) - 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) + // Should implement error interface + var err error = apiErr + assert.Equal(t, "test message: not found", err.Error()) }) - 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()) - - // When we add multiple API errors - resp1 := &github.Response{Response: &http.Response{StatusCode: 404}} - resp2 := &github.Response{Response: &http.Response{StatusCode: 403}} - - ctx, err := NewGitHubAPIErrorToCtx(ctx, "first error", resp1, fmt.Errorf("not found")) - require.NoError(t, err) - - ctx, err = NewGitHubAPIErrorToCtx(ctx, "second error", resp2, fmt.Errorf("forbidden")) - require.NoError(t, err) - - // And add a GraphQL error - gqlErr := newGitHubGraphQLError("graphql error", fmt.Errorf("query failed")) - 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) + t.Run("GitHubAPIError supports Unwrap", func(t *testing.T) { + originalErr := fmt.Errorf("not found") + apiErr := NewGitHubAPIError("test message", nil, originalErr) - // Then we should be able to retrieve all errors - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - assert.Len(t, apiErrors, 2) + assert.True(t, errors.Is(apiErr, originalErr)) + }) - gqlErrors, err := GetGitHubGraphQLErrors(ctx) - require.NoError(t, err) - assert.Len(t, gqlErrors, 1) + t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { + originalErr := fmt.Errorf("query failed") - rawErrors, err := GetGitHubRawAPIErrors(ctx) - require.NoError(t, err) - assert.Len(t, rawErrors, 1) + gqlErr := NewGitHubGraphQLError("test message", originalErr) - // 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) + // Should implement error interface + var err error = gqlErr + assert.Equal(t, "test message: query failed", err.Error()) }) - t.Run("context pointer sharing allows middleware to inspect errors without context propagation", func(t *testing.T) { - // This test demonstrates the key behavior: even when the context itself - // isn't propagated through function calls, the pointer to the error slice - // is shared, allowing middleware to inspect errors that were added later. + t.Run("GitHubGraphQLError supports Unwrap", func(t *testing.T) { + originalErr := fmt.Errorf("query failed") + gqlErr := NewGitHubGraphQLError("test message", originalErr) - // Given a context with GitHub error tracking enabled - originalCtx := ContextWithGitHubErrors(context.Background()) + assert.True(t, errors.Is(gqlErr, originalErr)) + }) - // Simulate a middleware that captures the context early - var middlewareCtx context.Context + t.Run("GitHubRawAPIError implements error interface", func(t *testing.T) { + resp := &http.Response{StatusCode: 500} + originalErr := fmt.Errorf("server error") - // Middleware function that captures the context - middleware := func(ctx context.Context) { - middlewareCtx = ctx // Middleware saves the context reference - } + rawErr := NewGitHubRawAPIError("test message", resp, originalErr) - // Call middleware with the original context - middleware(originalCtx) + var err error = rawErr + assert.Equal(t, "test message: server error", err.Error()) + }) - // Simulate some business logic that adds errors to the context - // but doesn't propagate the updated context back to middleware - businessLogic := func(ctx context.Context) { - resp := &github.Response{Response: &http.Response{StatusCode: 500}} + t.Run("GitHubRawAPIError supports Unwrap", func(t *testing.T) { + originalErr := fmt.Errorf("server error") + rawErr := NewGitHubRawAPIError("test message", nil, originalErr) - // Add an error to the context (this modifies the shared pointer) - _, err := NewGitHubAPIErrorToCtx(ctx, "business logic failed", resp, fmt.Errorf("internal error")) - require.NoError(t, err) + assert.True(t, errors.Is(rawErr, originalErr)) + }) +} - // Add another error - _, err = NewGitHubAPIErrorToCtx(ctx, "second failure", resp, fmt.Errorf("another error")) - require.NoError(t, err) - } +func TestNewGitHubAPIErrorResponse(t *testing.T) { + t.Run("creates CallToolResult with error set via SetError", func(t *testing.T) { + resp := &github.Response{Response: &http.Response{StatusCode: 422}} + originalErr := fmt.Errorf("validation failed") - // Execute business logic - note that we don't propagate the returned context - businessLogic(originalCtx) + result := NewGitHubAPIErrorResponse(context.Background(), "API call failed", resp, originalErr) - // Then the middleware should be able to see the errors that were added - // even though it only has a reference to the original context - apiErrors, err := GetGitHubAPIErrors(middlewareCtx) - require.NoError(t, err) - assert.Len(t, apiErrors, 2, "Middleware should see errors added after it captured the context") + require.NotNil(t, result) + assert.True(t, result.IsError) - assert.Equal(t, "business logic failed", apiErrors[0].Message) - assert.Equal(t, "second failure", apiErrors[1].Message) - }) + // The typed error should be accessible via GetError + gotErr := result.GetError() + require.NotNil(t, gotErr) - t.Run("context without GitHub errors returns error", func(t *testing.T) { - // Given a regular context without GitHub error tracking - ctx := context.Background() - - // When we try to retrieve errors - apiErrors, err := GetGitHubAPIErrors(ctx) - - // Then it should return an error - assert.Error(t, err) - assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors") - assert.Nil(t, apiErrors) - - // Same for GraphQL errors - gqlErrors, err := GetGitHubGraphQLErrors(ctx) - 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) + var apiErr *GitHubAPIError + require.True(t, errors.As(gotErr, &apiErr)) + assert.Equal(t, "API call failed", apiErr.Message) + assert.Equal(t, resp, apiErr.Response) + assert.Equal(t, originalErr, apiErr.Err) }) - t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { - // Given a context with existing errors - ctx := ContextWithGitHubErrors(context.Background()) + t.Run("error text is set in content", func(t *testing.T) { resp := &github.Response{Response: &http.Response{StatusCode: 404}} - ctx, err := NewGitHubAPIErrorToCtx(ctx, "existing error", resp, fmt.Errorf("error")) - require.NoError(t, err) - - // 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 all errors should be cleared - apiErrors, err = GetGitHubAPIErrors(resetCtx) - require.NoError(t, err) - 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") - }) + originalErr := fmt.Errorf("not found") - t.Run("NewGitHubAPIErrorResponse creates MCP error result and stores context error", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) + result := NewGitHubAPIErrorResponse(context.Background(), "failed to get issue", resp, originalErr) - resp := &github.Response{Response: &http.Response{StatusCode: 422}} - originalErr := fmt.Errorf("validation failed") + require.NotNil(t, result) + assert.True(t, result.IsError) + // SetError sets content to err.Error() + assert.Equal(t, "failed to get issue: not found", result.GetError().Error()) + }) +} + +func TestNewGitHubGraphQLErrorResponse(t *testing.T) { + t.Run("creates CallToolResult with error set via SetError", func(t *testing.T) { + originalErr := fmt.Errorf("mutation failed") - // When we create an API error response - result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, originalErr) + result := NewGitHubGraphQLErrorResponse(context.Background(), "GraphQL call failed", originalErr) - // Then it should return an MCP error result require.NotNil(t, result) assert.True(t, result.IsError) - // And the error should be stored in the context - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) + // The typed error should be accessible via GetError + gotErr := result.GetError() + require.NotNil(t, gotErr) - apiError := apiErrors[0] - assert.Equal(t, "API call failed", apiError.Message) - assert.Equal(t, resp, apiError.Response) - assert.Equal(t, originalErr, apiError.Err) + var gqlErr *GitHubGraphQLError + require.True(t, errors.As(gotErr, &gqlErr)) + assert.Equal(t, "GraphQL call failed", gqlErr.Message) + assert.Equal(t, originalErr, gqlErr.Err) }) +} - t.Run("NewGitHubGraphQLErrorResponse creates MCP error result and stores context error", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) +func TestNewGitHubRawAPIErrorResponse(t *testing.T) { + t.Run("creates CallToolResult with error set via SetError", func(t *testing.T) { + resp := &http.Response{StatusCode: 500} + originalErr := fmt.Errorf("server error") - originalErr := fmt.Errorf("mutation failed") + result := NewGitHubRawAPIErrorResponse(context.Background(), "raw API failed", resp, originalErr) - // When we create a GraphQL error response - result := NewGitHubGraphQLErrorResponse(ctx, "GraphQL call failed", originalErr) - - // Then it should return an MCP error result require.NotNil(t, result) assert.True(t, result.IsError) - // And the error should be stored in the context - gqlErrors, err := GetGitHubGraphQLErrors(ctx) - require.NoError(t, err) - require.Len(t, gqlErrors, 1) + // The typed error should be accessible via GetError + gotErr := result.GetError() + require.NotNil(t, gotErr) - gqlError := gqlErrors[0] - assert.Equal(t, "GraphQL call failed", gqlError.Message) - assert.Equal(t, originalErr, gqlError.Err) + var rawErr *GitHubRawAPIError + require.True(t, errors.As(gotErr, &rawErr)) + assert.Equal(t, "raw API failed", rawErr.Message) + assert.Equal(t, resp, rawErr.Response) + assert.Equal(t, originalErr, rawErr.Err) }) +} - t.Run("NewGitHubAPIStatusErrorResponse creates MCP error result from status code", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - +func TestNewGitHubAPIStatusErrorResponse(t *testing.T) { + t.Run("creates MCP error result from status code", func(t *testing.T) { resp := &github.Response{Response: &http.Response{StatusCode: 422}} body := []byte(`{"message": "Validation Failed"}`) - // When we create a status error response - result := NewGitHubAPIStatusErrorResponse(ctx, "failed to create issue", resp, body) + result := NewGitHubAPIStatusErrorResponse(context.Background(), "failed to create issue", resp, body) - // Then it should return an MCP error result require.NotNil(t, result) assert.True(t, result.IsError) - // And the error should be stored in the context - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) + // The typed error should be accessible via GetError + gotErr := result.GetError() + require.NotNil(t, gotErr) - apiError := apiErrors[0] - assert.Equal(t, "failed to create issue", apiError.Message) - assert.Equal(t, resp, apiError.Response) + var apiErr *GitHubAPIError + require.True(t, errors.As(gotErr, &apiErr)) + assert.Equal(t, "failed to create issue", apiErr.Message) + assert.Equal(t, resp, apiErr.Response) // The synthetic error should contain the status code and body - assert.Contains(t, apiError.Err.Error(), "unexpected status 422") - assert.Contains(t, apiError.Err.Error(), "Validation Failed") + assert.Contains(t, apiErr.Err.Error(), "unexpected status 422") + assert.Contains(t, apiErr.Err.Error(), "Validation Failed") }) +} - t.Run("NewGitHubAPIErrorToCtx with uninitialized context does not error", func(t *testing.T) { - // Given a regular context without GitHub error tracking initialized - ctx := context.Background() - - // Create a mock GitHub response - resp := &github.Response{ - Response: &http.Response{ - StatusCode: 500, - Status: "500 Internal Server Error", - }, - } - originalErr := fmt.Errorf("internal server error") +func TestErrorTypesCanBeExtractedWithErrorsAs(t *testing.T) { + t.Run("GitHubAPIError can be extracted from CallToolResult", func(t *testing.T) { + resp := &github.Response{Response: &http.Response{StatusCode: 403}} + originalErr := fmt.Errorf("forbidden") - // When we try to add an API error to an uninitialized context - updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed operation", resp, originalErr) + result := NewGitHubAPIErrorResponse(context.Background(), "access denied", resp, originalErr) - // Then it should not return an error (graceful handling) - assert.NoError(t, err, "NewGitHubAPIErrorToCtx should handle uninitialized context gracefully") - assert.Equal(t, ctx, updatedCtx, "Context should be returned unchanged when not initialized") + gotErr := result.GetError() + require.NotNil(t, gotErr) - // And attempting to retrieve errors should still return an error since context wasn't initialized - apiErrors, err := GetGitHubAPIErrors(updatedCtx) - assert.Error(t, err) - assert.Contains(t, err.Error(), "context does not contain GitHubCtxErrors") - assert.Nil(t, apiErrors) - }) + // Can extract typed error + var apiErr *GitHubAPIError + require.True(t, errors.As(gotErr, &apiErr)) + assert.Equal(t, 403, apiErr.Response.StatusCode) - t.Run("NewGitHubAPIErrorToCtx with nil context does not error", func(t *testing.T) { - // Given a nil context - var ctx context.Context - - // Create a mock GitHub response - resp := &github.Response{ - Response: &http.Response{ - StatusCode: 400, - Status: "400 Bad Request", - }, - } - originalErr := fmt.Errorf("bad request") - - // When we try to add an API error to a nil context - updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed with nil context", resp, originalErr) - - // Then it should not return an error (graceful handling) - assert.NoError(t, err, "NewGitHubAPIErrorToCtx should handle nil context gracefully") - assert.Nil(t, updatedCtx, "Context should remain nil when passed as nil") + // Can also unwrap to original error + assert.True(t, errors.Is(gotErr, originalErr)) }) -} -func TestGitHubErrorTypes(t *testing.T) { - t.Run("GitHubAPIError implements error interface", func(t *testing.T) { - resp := &github.Response{Response: &http.Response{StatusCode: 404}} - originalErr := fmt.Errorf("not found") + t.Run("GitHubGraphQLError can be extracted from CallToolResult", func(t *testing.T) { + originalErr := fmt.Errorf("invalid query") - apiErr := newGitHubAPIError("test message", resp, originalErr) + result := NewGitHubGraphQLErrorResponse(context.Background(), "query failed", originalErr) - // Should implement error interface - var err error = apiErr - assert.Equal(t, "test message: not found", err.Error()) + gotErr := result.GetError() + require.NotNil(t, gotErr) + + var gqlErr *GitHubGraphQLError + require.True(t, errors.As(gotErr, &gqlErr)) + assert.Equal(t, "query failed", gqlErr.Message) + assert.True(t, errors.Is(gotErr, originalErr)) }) - t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { - originalErr := fmt.Errorf("query failed") + t.Run("GitHubRawAPIError can be extracted from CallToolResult", func(t *testing.T) { + resp := &http.Response{StatusCode: 500} + originalErr := fmt.Errorf("internal server error") - gqlErr := newGitHubGraphQLError("test message", originalErr) + result := NewGitHubRawAPIErrorResponse(context.Background(), "raw call failed", resp, originalErr) - // Should implement error interface - var err error = gqlErr - assert.Equal(t, "test message: query failed", err.Error()) - }) -} + gotErr := result.GetError() + require.NotNil(t, gotErr) -// TestMiddlewareScenario demonstrates a realistic middleware scenario -func TestMiddlewareScenario(t *testing.T) { - t.Run("realistic middleware error collection scenario", func(t *testing.T) { - // Simulate a realistic HTTP middleware scenario - - // 1. Request comes in, middleware sets up error tracking - ctx := ContextWithGitHubErrors(context.Background()) - - // 2. Middleware stores reference to context for later inspection - var middlewareCtx context.Context - setupMiddleware := func(ctx context.Context) context.Context { - middlewareCtx = ctx - return ctx - } - - // 3. Setup middleware - ctx = setupMiddleware(ctx) - - // 4. Simulate multiple service calls that add errors - simulateServiceCall1 := func(ctx context.Context) { - resp := &github.Response{Response: &http.Response{StatusCode: 403}} - _, err := NewGitHubAPIErrorToCtx(ctx, "insufficient permissions", resp, fmt.Errorf("forbidden")) - require.NoError(t, err) - } - - simulateServiceCall2 := func(ctx context.Context) { - resp := &github.Response{Response: &http.Response{StatusCode: 404}} - _, err := NewGitHubAPIErrorToCtx(ctx, "resource not found", resp, fmt.Errorf("not found")) - require.NoError(t, err) - } - - simulateGraphQLCall := func(ctx context.Context) { - gqlErr := newGitHubGraphQLError("mutation failed", fmt.Errorf("invalid input")) - _, err := addGitHubGraphQLErrorToContext(ctx, gqlErr) - require.NoError(t, err) - } - - // 5. Execute service calls (without context propagation) - simulateServiceCall1(ctx) - simulateServiceCall2(ctx) - simulateGraphQLCall(ctx) - - // 6. Middleware inspects errors at the end of request processing - finalizeMiddleware := func(ctx context.Context) ([]string, []string) { - var apiErrorMessages []string - var gqlErrorMessages []string - - if apiErrors, err := GetGitHubAPIErrors(ctx); err == nil { - for _, apiErr := range apiErrors { - apiErrorMessages = append(apiErrorMessages, apiErr.Message) - } - } - - if gqlErrors, err := GetGitHubGraphQLErrors(ctx); err == nil { - for _, gqlErr := range gqlErrors { - gqlErrorMessages = append(gqlErrorMessages, gqlErr.Message) - } - } - - return apiErrorMessages, gqlErrorMessages - } - - // 7. Middleware can see all errors that were added during request processing - apiMessages, gqlMessages := finalizeMiddleware(middlewareCtx) - - // Verify all errors were captured - assert.Len(t, apiMessages, 2) - assert.Contains(t, apiMessages, "insufficient permissions") - assert.Contains(t, apiMessages, "resource not found") - - assert.Len(t, gqlMessages, 1) - assert.Contains(t, gqlMessages, "mutation failed") + var rawErr *GitHubRawAPIError + require.True(t, errors.As(gotErr, &rawErr)) + assert.Equal(t, 500, rawErr.Response.StatusCode) + assert.True(t, errors.Is(gotErr, originalErr)) }) } diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 516c7fe37..cdb78f5dd 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -78,7 +78,7 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo // Collect logs for all failed jobs var logResults []map[string]any for _, job := range failedJobs { - jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contentWindowSize) + jobResult, _, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contentWindowSize) if err != nil { // Continue with other jobs even if one fails jobResult = map[string]any{ @@ -86,8 +86,7 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo "job_name": job.GetName(), "error": err.Error(), } - // Enable reporting of status codes and error causes - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get job logs", resp, err) // Explicitly ignore error for graceful handling + } logResults = append(logResults, jobResult) diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index a347ebdd6..49867937a 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -19,7 +19,6 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep // First, we need to check what the default branch in this empty repo should be: repository, resp, err := client.Repositories.Get(ctx, owner, repo) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository", resp, err) return nil, nil, fmt.Errorf("failed to get repository: %w", err) } if resp != nil && resp.Body != nil { @@ -37,7 +36,6 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep // Create an initial empty commit to create the default branch createResp, resp, err := client.Repositories.CreateFile(ctx, owner, repo, "README.md", fileOpts) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to create initial file", resp, err) return nil, nil, fmt.Errorf("failed to create initial file: %w", err) } if resp != nil && resp.Body != nil { @@ -47,7 +45,6 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep // Get the commit that was just created to use as base for remaining files baseCommit, resp, err = client.Git.GetCommit(ctx, owner, repo, *createResp.Commit.SHA) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get initial commit", resp, err) return nil, nil, fmt.Errorf("failed to get initial commit: %w", err) } if resp != nil && resp.Body != nil { @@ -56,7 +53,6 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep ref, resp, err = client.Git.GetRef(ctx, owner, repo, "refs/heads/"+defaultBranch) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get final reference", resp, err) return nil, nil, fmt.Errorf("failed to get branch reference after initial commit: %w", err) } if resp != nil && resp.Body != nil { @@ -70,7 +66,6 @@ func initializeRepository(ctx context.Context, client *github.Client, owner, rep func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client, owner, repo, branch string) (*github.Reference, error) { defaultRef, err := resolveDefaultBranch(ctx, client, owner, repo) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to resolve default branch", nil, err) return nil, fmt.Errorf("failed to resolve default branch: %w", err) } @@ -80,7 +75,6 @@ func createReferenceFromDefaultBranch(ctx context.Context, client *github.Client SHA: *defaultRef.Object.SHA, }) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to create new branch reference", resp, err) return nil, fmt.Errorf("failed to create new branch reference: %w", err) } if resp != nil && resp.Body != nil { @@ -219,7 +213,6 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner // 2) If no SHA is provided, we try to resolve the ref into a fully-qualified format. var reference *github.Reference - var resp *github.Response var err error var fallbackUsed bool @@ -239,7 +232,7 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner default: // 2d) It's a short name, so we try to resolve it to either a branch or a tag. branchRef := "refs/heads/" + originalRef - reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef) + reference, _, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef) if err == nil { ref = branchRef // It's a branch. @@ -248,7 +241,7 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner ghErr, isGhErr := err.(*github.ErrorResponse) if isGhErr && ghErr.Response.StatusCode == http.StatusNotFound { tagRef := "refs/tags/" + originalRef - reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef) + reference, _, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef) if err == nil { ref = tagRef // It's a tag. } else { @@ -269,19 +262,17 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner } // The tag lookup failed for a different reason. - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (tag)", resp, err) return nil, false, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err) } } else { // The branch lookup failed for a different reason. - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (branch)", resp, err) return nil, false, fmt.Errorf("failed to get reference for branch '%s': %w", originalRef, err) } } } if reference == nil { - reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, ref) + reference, _, err = githubClient.Git.GetRef(ctx, owner, repo, ref) if err != nil { if ref == "refs/heads/main" { reference, err = resolveDefaultBranch(ctx, githubClient, owner, repo) @@ -292,7 +283,6 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner ref = reference.GetRef() fallbackUsed = true } else { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get final reference", resp, err) return nil, false, fmt.Errorf("failed to get final reference for %q: %w", ref, err) } } @@ -305,7 +295,6 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner func resolveDefaultBranch(ctx context.Context, githubClient *github.Client, owner, repo string) (*github.Reference, error) { repoInfo, resp, err := githubClient.Repositories.Get(ctx, owner, repo) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository info", resp, err) return nil, fmt.Errorf("failed to get repository info: %w", err) } @@ -317,7 +306,6 @@ func resolveDefaultBranch(ctx context.Context, githubClient *github.Client, owne defaultRef, resp, err := githubClient.Git.GetRef(ctx, owner, repo, "heads/"+defaultBranch) if err != nil { - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get default branch reference", resp, err) return nil, fmt.Errorf("failed to get default branch reference: %w", err) } diff --git a/pkg/github/server.go b/pkg/github/server.go index 14741939d..212b4c072 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -8,7 +8,6 @@ import ( "strings" "time" - gherrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/octicons" "github.com/github/github-mcp-server/pkg/translations" @@ -98,11 +97,10 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci ghServer := NewServer(cfg.Version, serverOpts) - // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, - // and any middleware that needs to read or modify the context should be before it. + // Add middlewares. The deps middleware must be applied so that it runs before + // tool handlers to inject dependencies into the context. ghServer.AddReceivingMiddleware(middleware...) ghServer.AddReceivingMiddleware(InjectDepsMiddleware(deps)) - ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext) if unrecognized := inv.UnrecognizedToolsets(); len(unrecognized) > 0 { cfg.Logger.Warn("Warning: unrecognized toolsets ignored", "toolsets", strings.Join(unrecognized, ", ")) @@ -162,15 +160,6 @@ func ResolvedEnabledToolsets(dynamicToolsets bool, enabledToolsets []string, ena return nil } -func addGitHubAPIErrorToContext(next mcp.MethodHandler) mcp.MethodHandler { - return func(ctx context.Context, method string, req mcp.Request) (result mcp.Result, err error) { - // Ensure the context is cleared of any previous errors - // as context isn't propagated through middleware - ctx = gherrors.ContextWithGitHubErrors(ctx) - return next(ctx, method, req) - } -} - // NewServer creates a new GitHub MCP server with the specified GH client and logger. func NewServer(version string, opts *mcp.ServerOptions) *mcp.Server { if opts == nil {