From 44aa810d6e49c973bb9befeb3f21aedc5ebbe69d Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sat, 25 Apr 2026 13:57:52 -0600 Subject: [PATCH 1/4] errors: improve rate limit error messages for AI agents When the GitHub API returns a rate limit error, replace the raw Go HTTP error string with a clean, actionable message so agents know exactly how long to wait before retrying. Before: search code: GET https://api.github.com/search/code: 403 API rate limit exceeded for user ID 12345. [rate reset in 2m59s] After: search code: GitHub API rate limit exceeded. Retry after 2m59s. create issue: GitHub secondary rate limit exceeded. Retry after 47s. create issue: GitHub secondary rate limit exceeded. Wait before retrying. Edge cases: expired/zero reset time, nil RetryAfter, and errors wrapped with errors.As all produce "Wait before retrying." rather than a negative or confusing duration. The original error is stored in context via addGitHubAPIErrorToContext before the rate-limit check, so middleware is unaffected. Fixes #2385. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error.go | 26 + pkg/errors/error_test.go | 1122 ++++++++++++++++++++++---------------- 2 files changed, 686 insertions(+), 462 deletions(-) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index d757651592..1661b79154 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -2,8 +2,10 @@ package errors import ( "context" + stderrors "errors" "fmt" "net/http" + "time" "github.com/github/github-mcp-server/pkg/utils" "github.com/google/go-github/v82/github" @@ -159,6 +161,30 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github if ctx != nil { _, _ = addGitHubAPIErrorToContext(ctx, apiErr) // Explicitly ignore error for graceful handling } + + var rateLimitErr *github.RateLimitError + if stderrors.As(err, &rateLimitErr) { + resetTime := rateLimitErr.Rate.Reset.Time + if !resetTime.IsZero() && resetTime.After(time.Now()) { + retryIn := time.Until(resetTime).Round(time.Second) + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub API rate limit exceeded. Retry after %v.", message, retryIn)) + } + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub API rate limit exceeded. Wait before retrying.", message)) + } + + var abuseErr *github.AbuseRateLimitError + if stderrors.As(err, &abuseErr) { + if abuseErr.RetryAfter != nil && *abuseErr.RetryAfter > 0 { + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub secondary rate limit exceeded. Retry after %v.", + message, abuseErr.RetryAfter.Round(time.Second))) + } + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub secondary rate limit exceeded. Wait before retrying.", message)) + } + return utils.NewToolResultErrorFromErr(message, err) } diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index e33d5bd39e..56323bba54 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -1,462 +1,660 @@ -package errors - -import ( - "context" - "fmt" - "net/http" - "testing" - - "github.com/google/go-github/v82/github" - "github.com/stretchr/testify/assert" - "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) - - 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()) - }) - - 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) - }) - - 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) - - // Then we should be able to retrieve all errors - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - assert.Len(t, apiErrors, 2) - - gqlErrors, err := GetGitHubGraphQLErrors(ctx) - 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) { - // 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. - - // Given a context with GitHub error tracking enabled - originalCtx := ContextWithGitHubErrors(context.Background()) - - // Simulate a middleware that captures the context early - var middlewareCtx context.Context - - // Middleware function that captures the context - middleware := func(ctx context.Context) { - middlewareCtx = ctx // Middleware saves the context reference - } - - // Call middleware with the original context - middleware(originalCtx) - - // 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}} - - // 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) - - // Add another error - _, err = NewGitHubAPIErrorToCtx(ctx, "second failure", resp, fmt.Errorf("another error")) - require.NoError(t, err) - } - - // Execute business logic - note that we don't propagate the returned context - businessLogic(originalCtx) - - // 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") - - assert.Equal(t, "business logic failed", apiErrors[0].Message) - assert.Equal(t, "second failure", apiErrors[1].Message) - }) - - 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) - }) - - t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { - // Given a context with existing errors - ctx := ContextWithGitHubErrors(context.Background()) - 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") - }) - - 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()) - - resp := &github.Response{Response: &http.Response{StatusCode: 422}} - originalErr := fmt.Errorf("validation failed") - - // When we create an API error response - result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, 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) - - apiError := apiErrors[0] - assert.Equal(t, "API call failed", apiError.Message) - assert.Equal(t, resp, apiError.Response) - assert.Equal(t, originalErr, apiError.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()) - - originalErr := fmt.Errorf("mutation failed") - - // 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) - - gqlError := gqlErrors[0] - assert.Equal(t, "GraphQL call failed", gqlError.Message) - assert.Equal(t, originalErr, gqlError.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()) - - 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) - - // 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) - - apiError := apiErrors[0] - assert.Equal(t, "failed to create issue", apiError.Message) - assert.Equal(t, resp, apiError.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") - }) - - 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") - - // When we try to add an API error to an uninitialized context - updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed operation", 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") - - // 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) - }) - - 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") - }) -} - -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") - - apiErr := newGitHubAPIError("test message", resp, originalErr) - - // Should implement error interface - var err error = apiErr - assert.Equal(t, "test message: not found", err.Error()) - }) - - t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { - originalErr := fmt.Errorf("query failed") - - gqlErr := newGitHubGraphQLError("test message", originalErr) - - // Should implement error interface - var err error = gqlErr - assert.Equal(t, "test message: query failed", err.Error()) - }) -} - -// 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") - }) -} +package errors + +import ( + "context" + "fmt" + "net/http" + "testing" + "time" + + "github.com/google/go-github/v82/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "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) + + 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()) + }) + + 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) + }) + + 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) + + // Then we should be able to retrieve all errors + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + assert.Len(t, apiErrors, 2) + + gqlErrors, err := GetGitHubGraphQLErrors(ctx) + 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) { + // 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. + + // Given a context with GitHub error tracking enabled + originalCtx := ContextWithGitHubErrors(context.Background()) + + // Simulate a middleware that captures the context early + var middlewareCtx context.Context + + // Middleware function that captures the context + middleware := func(ctx context.Context) { + middlewareCtx = ctx // Middleware saves the context reference + } + + // Call middleware with the original context + middleware(originalCtx) + + // 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}} + + // 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) + + // Add another error + _, err = NewGitHubAPIErrorToCtx(ctx, "second failure", resp, fmt.Errorf("another error")) + require.NoError(t, err) + } + + // Execute business logic - note that we don't propagate the returned context + businessLogic(originalCtx) + + // 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") + + assert.Equal(t, "business logic failed", apiErrors[0].Message) + assert.Equal(t, "second failure", apiErrors[1].Message) + }) + + 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) + }) + + t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { + // Given a context with existing errors + ctx := ContextWithGitHubErrors(context.Background()) + 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") + }) + + 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()) + + resp := &github.Response{Response: &http.Response{StatusCode: 422}} + originalErr := fmt.Errorf("validation failed") + + // When we create an API error response + result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, 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) + + apiError := apiErrors[0] + assert.Equal(t, "API call failed", apiError.Message) + assert.Equal(t, resp, apiError.Response) + assert.Equal(t, originalErr, apiError.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()) + + originalErr := fmt.Errorf("mutation failed") + + // 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) + + gqlError := gqlErrors[0] + assert.Equal(t, "GraphQL call failed", gqlError.Message) + assert.Equal(t, originalErr, gqlError.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()) + + 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) + + // 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) + + apiError := apiErrors[0] + assert.Equal(t, "failed to create issue", apiError.Message) + assert.Equal(t, resp, apiError.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") + }) + + 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") + + // When we try to add an API error to an uninitialized context + updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed operation", 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") + + // 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) + }) + + 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") + }) +} + +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") + + apiErr := newGitHubAPIError("test message", resp, originalErr) + + // Should implement error interface + var err error = apiErr + assert.Equal(t, "test message: not found", err.Error()) + }) + + t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { + originalErr := fmt.Errorf("query failed") + + gqlErr := newGitHubGraphQLError("test message", originalErr) + + // Should implement error interface + var err error = gqlErr + assert.Equal(t, "test message: query failed", err.Error()) + }) +} + +// 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") + }) +} + +func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { + t.Run("RateLimitError produces clean message with retry time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(3 * time.Minute) + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + // When we create an API error response for a rate limit error + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + expectedRetryIn := time.Until(resetTime).Round(time.Second) + + // Then it should return an MCP error result + require.NotNil(t, result) + assert.True(t, result.IsError) + + // And the message should be clean and actionable (no raw URLs or status codes) + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) + assert.NotContains(t, textContent.Text, "https://") + assert.NotContains(t, textContent.Text, "403") + + // And the original error should still be stored in context for middleware + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + require.Len(t, apiErrors, 1) + assert.Equal(t, rateLimitErr, apiErrors[0].Err) + }) + + t.Run("AbuseRateLimitError with RetryAfter produces clean message with wait time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + retryAfter := 47 * time.Second + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "You have exceeded a secondary rate limit.", + RetryAfter: &retryAfter, + } + resp := &github.Response{Response: abuseErr.Response} + + // When we create an API error response for a secondary rate limit error + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) + + // Then it should return an MCP error result + require.NotNil(t, result) + assert.True(t, result.IsError) + + // And the message should include the specific retry duration + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Retry after 47s.") + assert.NotContains(t, textContent.Text, "https://") + assert.NotContains(t, textContent.Text, "403") + + // And the original error should still be stored in context for middleware + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + require.Len(t, apiErrors, 1) + assert.Equal(t, abuseErr, apiErrors[0].Err) + }) + + t.Run("AbuseRateLimitError without RetryAfter produces clean message without wait time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "You have exceeded a secondary rate limit.", + RetryAfter: nil, + } + resp := &github.Response{Response: abuseErr.Response} + + // When we create an API error response for a secondary rate limit error without retry info + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) + + // Then it should return an MCP error result + require.NotNil(t, result) + assert.True(t, result.IsError) + + // And the message should be clean and actionable + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Wait before retrying.") + assert.NotContains(t, textContent.Text, "https://") + assert.NotContains(t, textContent.Text, "403") + + // And the original error should still be stored in context for middleware + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + require.Len(t, apiErrors, 1) + assert.Equal(t, abuseErr, apiErrors[0].Err) + }) + + t.Run("RateLimitError with reset time in the past falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(-5 * time.Second) // already passed + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + require.NotNil(t, result) + assert.True(t, result.IsError) + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "GitHub API rate limit exceeded. Wait before retrying.") + }) + + t.Run("RateLimitError with zero reset time falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{}, // zero Reset time + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + require.NotNil(t, result) + assert.True(t, result.IsError) + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "GitHub API rate limit exceeded. Wait before retrying.") + }) + + t.Run("wrapped RateLimitError is handled via errors.As", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(2 * time.Minute) + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + wrappedErr := fmt.Errorf("transport layer: %w", rateLimitErr) + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, wrappedErr) + expectedRetryIn := time.Until(resetTime).Round(time.Second) + + require.NotNil(t, result) + assert.True(t, result.IsError) + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) + assert.NotContains(t, textContent.Text, "https://") + }) + + t.Run("wrapped AbuseRateLimitError is handled via errors.As", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + retryAfter := 30 * time.Second + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "secondary rate limit", + RetryAfter: &retryAfter, + } + wrappedErr := fmt.Errorf("transport layer: %w", abuseErr) + resp := &github.Response{Response: abuseErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, wrappedErr) + + require.NotNil(t, result) + assert.True(t, result.IsError) + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Retry after 30s.") + assert.NotContains(t, textContent.Text, "https://") + }) + + t.Run("non-rate-limit GitHub API error passes through the original error message", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + resp := &github.Response{Response: &http.Response{StatusCode: 422}} + originalErr := fmt.Errorf("validation failed") + + // When we create an API error response for a non-rate-limit error + result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, originalErr) + + // Then the message should contain the original error text unchanged + require.NotNil(t, result) + assert.True(t, result.IsError) + + textContent := result.Content[0].(*mcp.TextContent) + assert.Contains(t, textContent.Text, "validation failed") + }) +} From d480ff73091183797754415731191e43465a08ae Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sat, 25 Apr 2026 14:15:57 -0600 Subject: [PATCH 2/4] errors: fix flaky rate limit tests Compute expectedRetryIn before calling the function under test, and use larger reset time offsets (20-30 min), so a 1s boundary during time.Duration.Round cannot cause spurious mismatches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index 56323bba54..765b852aa8 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -468,7 +468,7 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { // Given a context with GitHub error tracking enabled ctx := ContextWithGitHubErrors(context.Background()) - resetTime := time.Now().Add(3 * time.Minute) + resetTime := time.Now().Add(30 * time.Minute) rateLimitErr := &github.RateLimitError{ Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, Response: &http.Response{StatusCode: 403}, @@ -476,9 +476,11 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { } resp := &github.Response{Response: rateLimitErr.Response} + // Capture expected duration before the call so both use the same time.Until snapshot + expectedRetryIn := time.Until(resetTime).Round(time.Second) + // When we create an API error response for a rate limit error result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) - expectedRetryIn := time.Until(resetTime).Round(time.Second) // Then it should return an MCP error result require.NotNil(t, result) @@ -600,7 +602,7 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { t.Run("wrapped RateLimitError is handled via errors.As", func(t *testing.T) { ctx := ContextWithGitHubErrors(context.Background()) - resetTime := time.Now().Add(2 * time.Minute) + resetTime := time.Now().Add(20 * time.Minute) rateLimitErr := &github.RateLimitError{ Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, Response: &http.Response{StatusCode: 403}, @@ -609,9 +611,11 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { wrappedErr := fmt.Errorf("transport layer: %w", rateLimitErr) resp := &github.Response{Response: rateLimitErr.Response} - result := NewGitHubAPIErrorResponse(ctx, "search code", resp, wrappedErr) + // Capture expected duration before the call so both use the same time.Until snapshot expectedRetryIn := time.Until(resetTime).Round(time.Second) + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, wrappedErr) + require.NotNil(t, result) assert.True(t, result.IsError) textContent := result.Content[0].(*mcp.TextContent) From 52d8383cc2e00fee78ef4ae1cac6a9cbf5e3cc0f Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sat, 25 Apr 2026 14:23:42 -0600 Subject: [PATCH 3/4] errors: extract requireErrorText and assertContextHasError test helpers Reduces repetition in TestNewGitHubAPIErrorResponse_RateLimits subtests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error_test.go | 1319 +++++++++++++++++++------------------- 1 file changed, 655 insertions(+), 664 deletions(-) diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index 765b852aa8..a0c99ceee3 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -1,664 +1,655 @@ -package errors - -import ( - "context" - "fmt" - "net/http" - "testing" - "time" - - "github.com/google/go-github/v82/github" - "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/stretchr/testify/assert" - "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) - - 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()) - }) - - 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) - }) - - 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) - - // Then we should be able to retrieve all errors - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - assert.Len(t, apiErrors, 2) - - gqlErrors, err := GetGitHubGraphQLErrors(ctx) - 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) { - // 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. - - // Given a context with GitHub error tracking enabled - originalCtx := ContextWithGitHubErrors(context.Background()) - - // Simulate a middleware that captures the context early - var middlewareCtx context.Context - - // Middleware function that captures the context - middleware := func(ctx context.Context) { - middlewareCtx = ctx // Middleware saves the context reference - } - - // Call middleware with the original context - middleware(originalCtx) - - // 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}} - - // 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) - - // Add another error - _, err = NewGitHubAPIErrorToCtx(ctx, "second failure", resp, fmt.Errorf("another error")) - require.NoError(t, err) - } - - // Execute business logic - note that we don't propagate the returned context - businessLogic(originalCtx) - - // 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") - - assert.Equal(t, "business logic failed", apiErrors[0].Message) - assert.Equal(t, "second failure", apiErrors[1].Message) - }) - - 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) - }) - - t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { - // Given a context with existing errors - ctx := ContextWithGitHubErrors(context.Background()) - 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") - }) - - 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()) - - resp := &github.Response{Response: &http.Response{StatusCode: 422}} - originalErr := fmt.Errorf("validation failed") - - // When we create an API error response - result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, 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) - - apiError := apiErrors[0] - assert.Equal(t, "API call failed", apiError.Message) - assert.Equal(t, resp, apiError.Response) - assert.Equal(t, originalErr, apiError.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()) - - originalErr := fmt.Errorf("mutation failed") - - // 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) - - gqlError := gqlErrors[0] - assert.Equal(t, "GraphQL call failed", gqlError.Message) - assert.Equal(t, originalErr, gqlError.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()) - - 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) - - // 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) - - apiError := apiErrors[0] - assert.Equal(t, "failed to create issue", apiError.Message) - assert.Equal(t, resp, apiError.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") - }) - - 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") - - // When we try to add an API error to an uninitialized context - updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed operation", 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") - - // 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) - }) - - 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") - }) -} - -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") - - apiErr := newGitHubAPIError("test message", resp, originalErr) - - // Should implement error interface - var err error = apiErr - assert.Equal(t, "test message: not found", err.Error()) - }) - - t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { - originalErr := fmt.Errorf("query failed") - - gqlErr := newGitHubGraphQLError("test message", originalErr) - - // Should implement error interface - var err error = gqlErr - assert.Equal(t, "test message: query failed", err.Error()) - }) -} - -// 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") - }) -} - -func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { - t.Run("RateLimitError produces clean message with retry time", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - - resetTime := time.Now().Add(30 * time.Minute) - rateLimitErr := &github.RateLimitError{ - Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, - Response: &http.Response{StatusCode: 403}, - Message: "API rate limit exceeded", - } - resp := &github.Response{Response: rateLimitErr.Response} - - // Capture expected duration before the call so both use the same time.Until snapshot - expectedRetryIn := time.Until(resetTime).Round(time.Second) - - // When we create an API error response for a rate limit error - result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) - - // Then it should return an MCP error result - require.NotNil(t, result) - assert.True(t, result.IsError) - - // And the message should be clean and actionable (no raw URLs or status codes) - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) - assert.NotContains(t, textContent.Text, "https://") - assert.NotContains(t, textContent.Text, "403") - - // And the original error should still be stored in context for middleware - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) - assert.Equal(t, rateLimitErr, apiErrors[0].Err) - }) - - t.Run("AbuseRateLimitError with RetryAfter produces clean message with wait time", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - - retryAfter := 47 * time.Second - abuseErr := &github.AbuseRateLimitError{ - Response: &http.Response{StatusCode: 403}, - Message: "You have exceeded a secondary rate limit.", - RetryAfter: &retryAfter, - } - resp := &github.Response{Response: abuseErr.Response} - - // When we create an API error response for a secondary rate limit error - result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) - - // Then it should return an MCP error result - require.NotNil(t, result) - assert.True(t, result.IsError) - - // And the message should include the specific retry duration - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Retry after 47s.") - assert.NotContains(t, textContent.Text, "https://") - assert.NotContains(t, textContent.Text, "403") - - // And the original error should still be stored in context for middleware - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) - assert.Equal(t, abuseErr, apiErrors[0].Err) - }) - - t.Run("AbuseRateLimitError without RetryAfter produces clean message without wait time", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - - abuseErr := &github.AbuseRateLimitError{ - Response: &http.Response{StatusCode: 403}, - Message: "You have exceeded a secondary rate limit.", - RetryAfter: nil, - } - resp := &github.Response{Response: abuseErr.Response} - - // When we create an API error response for a secondary rate limit error without retry info - result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) - - // Then it should return an MCP error result - require.NotNil(t, result) - assert.True(t, result.IsError) - - // And the message should be clean and actionable - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Wait before retrying.") - assert.NotContains(t, textContent.Text, "https://") - assert.NotContains(t, textContent.Text, "403") - - // And the original error should still be stored in context for middleware - apiErrors, err := GetGitHubAPIErrors(ctx) - require.NoError(t, err) - require.Len(t, apiErrors, 1) - assert.Equal(t, abuseErr, apiErrors[0].Err) - }) - - t.Run("RateLimitError with reset time in the past falls back to wait message", func(t *testing.T) { - ctx := ContextWithGitHubErrors(context.Background()) - - resetTime := time.Now().Add(-5 * time.Second) // already passed - rateLimitErr := &github.RateLimitError{ - Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, - Response: &http.Response{StatusCode: 403}, - Message: "API rate limit exceeded", - } - resp := &github.Response{Response: rateLimitErr.Response} - - result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) - - require.NotNil(t, result) - assert.True(t, result.IsError) - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "GitHub API rate limit exceeded. Wait before retrying.") - }) - - t.Run("RateLimitError with zero reset time falls back to wait message", func(t *testing.T) { - ctx := ContextWithGitHubErrors(context.Background()) - - rateLimitErr := &github.RateLimitError{ - Rate: github.Rate{}, // zero Reset time - Response: &http.Response{StatusCode: 403}, - Message: "API rate limit exceeded", - } - resp := &github.Response{Response: rateLimitErr.Response} - - result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) - - require.NotNil(t, result) - assert.True(t, result.IsError) - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "GitHub API rate limit exceeded. Wait before retrying.") - }) - - t.Run("wrapped RateLimitError is handled via errors.As", func(t *testing.T) { - ctx := ContextWithGitHubErrors(context.Background()) - - resetTime := time.Now().Add(20 * time.Minute) - rateLimitErr := &github.RateLimitError{ - Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, - Response: &http.Response{StatusCode: 403}, - Message: "API rate limit exceeded", - } - wrappedErr := fmt.Errorf("transport layer: %w", rateLimitErr) - resp := &github.Response{Response: rateLimitErr.Response} - - // Capture expected duration before the call so both use the same time.Until snapshot - expectedRetryIn := time.Until(resetTime).Round(time.Second) - - result := NewGitHubAPIErrorResponse(ctx, "search code", resp, wrappedErr) - - require.NotNil(t, result) - assert.True(t, result.IsError) - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) - assert.NotContains(t, textContent.Text, "https://") - }) - - t.Run("wrapped AbuseRateLimitError is handled via errors.As", func(t *testing.T) { - ctx := ContextWithGitHubErrors(context.Background()) - - retryAfter := 30 * time.Second - abuseErr := &github.AbuseRateLimitError{ - Response: &http.Response{StatusCode: 403}, - Message: "secondary rate limit", - RetryAfter: &retryAfter, - } - wrappedErr := fmt.Errorf("transport layer: %w", abuseErr) - resp := &github.Response{Response: abuseErr.Response} - - result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, wrappedErr) - - require.NotNil(t, result) - assert.True(t, result.IsError) - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "GitHub secondary rate limit exceeded. Retry after 30s.") - assert.NotContains(t, textContent.Text, "https://") - }) - - t.Run("non-rate-limit GitHub API error passes through the original error message", func(t *testing.T) { - // Given a context with GitHub error tracking enabled - ctx := ContextWithGitHubErrors(context.Background()) - - resp := &github.Response{Response: &http.Response{StatusCode: 422}} - originalErr := fmt.Errorf("validation failed") - - // When we create an API error response for a non-rate-limit error - result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, originalErr) - - // Then the message should contain the original error text unchanged - require.NotNil(t, result) - assert.True(t, result.IsError) - - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "validation failed") - }) -} +package errors + +import ( + "context" + "fmt" + "net/http" + "testing" + "time" + + "github.com/google/go-github/v82/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "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) + + 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()) + }) + + 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) + }) + + 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) + + // Then we should be able to retrieve all errors + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + assert.Len(t, apiErrors, 2) + + gqlErrors, err := GetGitHubGraphQLErrors(ctx) + 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) { + // 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. + + // Given a context with GitHub error tracking enabled + originalCtx := ContextWithGitHubErrors(context.Background()) + + // Simulate a middleware that captures the context early + var middlewareCtx context.Context + + // Middleware function that captures the context + middleware := func(ctx context.Context) { + middlewareCtx = ctx // Middleware saves the context reference + } + + // Call middleware with the original context + middleware(originalCtx) + + // 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}} + + // 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) + + // Add another error + _, err = NewGitHubAPIErrorToCtx(ctx, "second failure", resp, fmt.Errorf("another error")) + require.NoError(t, err) + } + + // Execute business logic - note that we don't propagate the returned context + businessLogic(originalCtx) + + // 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") + + assert.Equal(t, "business logic failed", apiErrors[0].Message) + assert.Equal(t, "second failure", apiErrors[1].Message) + }) + + 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) + }) + + t.Run("ContextWithGitHubErrors resets existing errors", func(t *testing.T) { + // Given a context with existing errors + ctx := ContextWithGitHubErrors(context.Background()) + 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") + }) + + 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()) + + resp := &github.Response{Response: &http.Response{StatusCode: 422}} + originalErr := fmt.Errorf("validation failed") + + // When we create an API error response + result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, 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) + + apiError := apiErrors[0] + assert.Equal(t, "API call failed", apiError.Message) + assert.Equal(t, resp, apiError.Response) + assert.Equal(t, originalErr, apiError.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()) + + originalErr := fmt.Errorf("mutation failed") + + // 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) + + gqlError := gqlErrors[0] + assert.Equal(t, "GraphQL call failed", gqlError.Message) + assert.Equal(t, originalErr, gqlError.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()) + + 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) + + // 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) + + apiError := apiErrors[0] + assert.Equal(t, "failed to create issue", apiError.Message) + assert.Equal(t, resp, apiError.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") + }) + + 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") + + // When we try to add an API error to an uninitialized context + updatedCtx, err := NewGitHubAPIErrorToCtx(ctx, "failed operation", 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") + + // 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) + }) + + 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") + }) +} + +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") + + apiErr := newGitHubAPIError("test message", resp, originalErr) + + // Should implement error interface + var err error = apiErr + assert.Equal(t, "test message: not found", err.Error()) + }) + + t.Run("GitHubGraphQLError implements error interface", func(t *testing.T) { + originalErr := fmt.Errorf("query failed") + + gqlErr := newGitHubGraphQLError("test message", originalErr) + + // Should implement error interface + var err error = gqlErr + assert.Equal(t, "test message: query failed", err.Error()) + }) +} + +// 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") + }) +} + +// requireErrorText asserts that result is a non-nil MCP tool error and returns its text content. +func requireErrorText(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + require.NotNil(t, result) + require.True(t, result.IsError) + require.NotEmpty(t, result.Content) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected *mcp.TextContent, got %T", result.Content[0]) + return text.Text +} + +// assertContextHasError asserts that exactly one error is stored in ctx and it matches expectedErr. +// +//nolint:revive // t must be first for test helpers; context-as-argument doesn't apply here +func assertContextHasError(t *testing.T, ctx context.Context, expectedErr error) { + t.Helper() + apiErrors, err := GetGitHubAPIErrors(ctx) + require.NoError(t, err) + require.Len(t, apiErrors, 1) + assert.Equal(t, expectedErr, apiErrors[0].Err) +} + +func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { + t.Run("RateLimitError produces clean message with retry time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(30 * time.Minute) + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + // Capture expected duration before the call so both use the same time.Until snapshot + expectedRetryIn := time.Until(resetTime).Round(time.Second) + + // When we create an API error response for a rate limit error + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + // Then the message should be clean and actionable (no raw URLs or status codes) + text := requireErrorText(t, result) + assert.Contains(t, text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) + assert.NotContains(t, text, "https://") + assert.NotContains(t, text, "403") + + // And the original error should still be stored in context for middleware + assertContextHasError(t, ctx, rateLimitErr) + }) + + t.Run("AbuseRateLimitError with RetryAfter produces clean message with wait time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + retryAfter := 47 * time.Second + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "You have exceeded a secondary rate limit.", + RetryAfter: &retryAfter, + } + resp := &github.Response{Response: abuseErr.Response} + + // When we create an API error response for a secondary rate limit error + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) + + // And the message should include the specific retry duration + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub secondary rate limit exceeded. Retry after 47s.") + assert.NotContains(t, text, "https://") + assert.NotContains(t, text, "403") + + // And the original error should still be stored in context for middleware + assertContextHasError(t, ctx, abuseErr) + }) + + t.Run("AbuseRateLimitError without RetryAfter produces clean message without wait time", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "You have exceeded a secondary rate limit.", + RetryAfter: nil, + } + resp := &github.Response{Response: abuseErr.Response} + + // When we create an API error response for a secondary rate limit error without retry info + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) + + // And the message should be clean and actionable + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub secondary rate limit exceeded. Wait before retrying.") + assert.NotContains(t, text, "https://") + assert.NotContains(t, text, "403") + + // And the original error should still be stored in context for middleware + assertContextHasError(t, ctx, abuseErr) + }) + + t.Run("RateLimitError with reset time in the past falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(-5 * time.Second) // already passed + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.") + }) + + t.Run("RateLimitError with zero reset time falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{}, // zero Reset time + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.") + }) + + t.Run("wrapped RateLimitError is handled via errors.As", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + resetTime := time.Now().Add(20 * time.Minute) + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + wrappedErr := fmt.Errorf("transport layer: %w", rateLimitErr) + resp := &github.Response{Response: rateLimitErr.Response} + + // Capture expected duration before the call so both use the same time.Until snapshot + expectedRetryIn := time.Until(resetTime).Round(time.Second) + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, wrappedErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, fmt.Sprintf("GitHub API rate limit exceeded. Retry after %v.", expectedRetryIn)) + assert.NotContains(t, text, "https://") + }) + + t.Run("wrapped AbuseRateLimitError is handled via errors.As", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + retryAfter := 30 * time.Second + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "secondary rate limit", + RetryAfter: &retryAfter, + } + wrappedErr := fmt.Errorf("transport layer: %w", abuseErr) + resp := &github.Response{Response: abuseErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, wrappedErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub secondary rate limit exceeded. Retry after 30s.") + assert.NotContains(t, text, "https://") + }) + + t.Run("non-rate-limit GitHub API error passes through the original error message", func(t *testing.T) { + // Given a context with GitHub error tracking enabled + ctx := ContextWithGitHubErrors(context.Background()) + + resp := &github.Response{Response: &http.Response{StatusCode: 422}} + originalErr := fmt.Errorf("validation failed") + + // When we create an API error response for a non-rate-limit error + result := NewGitHubAPIErrorResponse(ctx, "API call failed", resp, originalErr) + + // Then the message should contain the original error text unchanged + text := requireErrorText(t, result) + assert.Contains(t, text, "validation failed") + }) +} + From edbbca305f6611337732d7970a42bb8fd9acd391 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sat, 25 Apr 2026 14:51:57 -0600 Subject: [PATCH 4/4] Fix edge cases: sub-second rate limit durations and UTF-8 BOM - Primary rate limit: compute time.Until(resetTime) once and check the rounded result is >0 before showing 'Retry after X'. This avoids a TOCTOU race between the After(time.Now()) guard and the subsequent time.Until call, and prevents showing 'Retry after 0s.' when the reset time is imminent. - Secondary rate limit: round RetryAfter first, then check >0. Previously, a RetryAfter of e.g. 200ms would pass the >0 guard but format as 'Retry after 0s.' after rounding. - Add tests for both sub-second edge cases. - Remove UTF-8 BOM accidentally introduced in error_test.go by .NET WriteAllText with the default UTF8 encoding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/errors/error.go | 19 ++++++++++++------- pkg/errors/error_test.go | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 1661b79154..981f772e8e 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -165,10 +165,12 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github var rateLimitErr *github.RateLimitError if stderrors.As(err, &rateLimitErr) { resetTime := rateLimitErr.Rate.Reset.Time - if !resetTime.IsZero() && resetTime.After(time.Now()) { + if !resetTime.IsZero() { retryIn := time.Until(resetTime).Round(time.Second) - return utils.NewToolResultError(fmt.Sprintf( - "%s: GitHub API rate limit exceeded. Retry after %v.", message, retryIn)) + if retryIn > 0 { + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub API rate limit exceeded. Retry after %v.", message, retryIn)) + } } return utils.NewToolResultError(fmt.Sprintf( "%s: GitHub API rate limit exceeded. Wait before retrying.", message)) @@ -176,10 +178,13 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github var abuseErr *github.AbuseRateLimitError if stderrors.As(err, &abuseErr) { - if abuseErr.RetryAfter != nil && *abuseErr.RetryAfter > 0 { - return utils.NewToolResultError(fmt.Sprintf( - "%s: GitHub secondary rate limit exceeded. Retry after %v.", - message, abuseErr.RetryAfter.Round(time.Second))) + if abuseErr.RetryAfter != nil { + retryAfter := abuseErr.RetryAfter.Round(time.Second) + if retryAfter > 0 { + return utils.NewToolResultError(fmt.Sprintf( + "%s: GitHub secondary rate limit exceeded. Retry after %v.", + message, retryAfter)) + } } return utils.NewToolResultError(fmt.Sprintf( "%s: GitHub secondary rate limit exceeded. Wait before retrying.", message)) diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go index a0c99ceee3..20bfaa9373 100644 --- a/pkg/errors/error_test.go +++ b/pkg/errors/error_test.go @@ -1,4 +1,4 @@ -package errors +package errors import ( "context" @@ -563,6 +563,24 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { assertContextHasError(t, ctx, abuseErr) }) + t.Run("AbuseRateLimitError with sub-second RetryAfter falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + // 200ms rounds to 0s, so should fall back to the generic wait message + retryAfter := 200 * time.Millisecond + abuseErr := &github.AbuseRateLimitError{ + Response: &http.Response{StatusCode: 403}, + Message: "You have exceeded a secondary rate limit.", + RetryAfter: &retryAfter, + } + resp := &github.Response{Response: abuseErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub secondary rate limit exceeded. Wait before retrying.") + }) + t.Run("RateLimitError with reset time in the past falls back to wait message", func(t *testing.T) { ctx := ContextWithGitHubErrors(context.Background()) @@ -580,6 +598,24 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) { assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.") }) + t.Run("RateLimitError with sub-second reset time falls back to wait message", func(t *testing.T) { + ctx := ContextWithGitHubErrors(context.Background()) + + // 250ms in the future: still positive, but rounds to 0s, so should fall back + resetTime := time.Now().Add(250 * time.Millisecond) + rateLimitErr := &github.RateLimitError{ + Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}}, + Response: &http.Response{StatusCode: 403}, + Message: "API rate limit exceeded", + } + resp := &github.Response{Response: rateLimitErr.Response} + + result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr) + + text := requireErrorText(t, result) + assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.") + }) + t.Run("RateLimitError with zero reset time falls back to wait message", func(t *testing.T) { ctx := ContextWithGitHubErrors(context.Background())