Skip to content

Conversation

@SamMorrowDrums
Copy link
Collaborator

Problem

The remote server's error_categorizer.go uses context to extract GitHub API errors for observability metrics. However, when API calls succeed but return unexpected HTTP status codes (e.g., 404, 422, 500), the errors were returned via utils.NewToolResultError which bypasses the context-based error tracking.

This resulted in 100% tool call success rates in observability even when errors occurred.

Root Cause

There are three categories of errors in the tool handlers:

  1. Parameter validation errors (e.g., RequiredParam failures) - These correctly use utils.NewToolResultError as they are user input errors, not GitHub API errors.

  2. GitHub API client errors (when err != nil after API call) - These mostly use ghErrors.NewGitHubAPIErrorResponse and are tracked correctly.

  3. HTTP status code errors (when API succeeds but returns non-2xx) - These were using utils.NewToolResultError and were NOT being tracked.

Solution

Add a new helper function NewGitHubAPIStatusErrorResponse that:

  1. Creates a synthetic error from the status code and response body
  2. Records the error in context via NewGitHubAPIErrorResponse
  3. Returns the MCP error result to the client

Then systematically updated all tool files to use this new pattern.

Changes

New Function in pkg/errors/error.go

// NewGitHubAPIStatusErrorResponse handles cases where the API call succeeds (err == nil)
// but returns an unexpected HTTP status code. It creates a synthetic error from the
// status code and response body, then records it in context for observability tracking.
func NewGitHubAPIStatusErrorResponse(ctx context.Context, message string, resp *github.Response, body []byte) *mcp.CallToolResult {
    err := fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body))
    return NewGitHubAPIErrorResponse(ctx, message, resp, err)
}

Files Updated

File Fixes
pullrequests.go 12
repositories.go 18
issues.go 10
notifications.go 6
projects.go 5
search.go 3
search_utils.go 1
gists.go 4
code_scanning.go 2
dependabot.go 2
secret_scanning.go 2
security_advisories.go 4
Total ~69

Relation to Existing PRs

This PR addresses the bulk of the error tracking issue that PRs #1566 and #1570 are also addressing:

This PR focuses on the ~69 HTTP status code error paths across all tool files that were not being tracked. After this PR merges:

Testing

  • ✅ All existing tests pass
  • ✅ Added test for NewGitHubAPIStatusErrorResponse
  • ✅ Lint passes with 0 issues

Notes

  • Parameter validation errors continue to use utils.NewToolResultError - they are user input errors, not API errors
  • Internal I/O errors (e.g., io.ReadAll failures) continue to use utils.NewToolResultErrorFromErr - they are not GitHub API errors
  • Only errors where we have a resp (GitHub Response) and ctx are tracked

Add NewGitHubAPIStatusErrorResponse helper function to properly track
GitHub API errors when the API call succeeds but returns an unexpected
HTTP status code (e.g., 404, 422, 500).

Previously, these errors were returned via utils.NewToolResultError which
bypasses the context-based error tracking used by the remote server's
error_categorizer.go for observability metrics. This resulted in 100%
tool call success rates in observability even when errors occurred.

The fix adds a new helper function that:
1. Creates a synthetic error from the status code and response body
2. Records the error in context via NewGitHubAPIErrorResponse
3. Returns the MCP error result to the client

Updated all tool files to use the new pattern for status code errors:
- pullrequests.go: 12 fixes
- repositories.go: 18 fixes
- issues.go: 10 fixes
- notifications.go: 6 fixes
- projects.go: 5 fixes
- search.go: 3 fixes
- search_utils.go: 1 fix
- gists.go: 4 fixes
- code_scanning.go: 2 fixes
- dependabot.go: 2 fixes
- secret_scanning.go: 2 fixes
- security_advisories.go: 4 fixes

Total: ~69 error paths now properly tracked.

Note: Parameter validation errors (RequiredParam failures) and internal
I/O errors (io.ReadAll failures) intentionally continue to use
utils.NewToolResultError as they are not GitHub API errors.
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner December 17, 2025 12:53
Copilot AI review requested due to automatic review settings December 17, 2025 12:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical observability gap where HTTP status code errors (e.g., 404, 422, 500) from successful GitHub API calls were not being tracked in context, resulting in falsely reported 100% tool call success rates. The solution introduces a new helper function NewGitHubAPIStatusErrorResponse that creates synthetic errors from status codes and response bodies, then records them in context for observability metrics.

Key changes:

  • Added NewGitHubAPIStatusErrorResponse helper in pkg/errors/error.go to handle status code errors
  • Systematically updated ~69 error paths across 12 tool files to use the new helper
  • Added comprehensive test coverage for the new function

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/errors/error.go Added NewGitHubAPIStatusErrorResponse helper function to create synthetic errors from status codes and track them in context
pkg/errors/error_test.go Added comprehensive test for the new helper function verifying MCP error result creation and context storage
pkg/github/pullrequests.go Updated 12 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/repositories.go Updated 18 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/issues.go Updated 10 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/notifications.go Updated 6 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/projects.go Updated 5 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/search.go Updated 3 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/search_utils.go Updated 1 HTTP status error path to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/gists.go Updated 4 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/code_scanning.go Updated 2 HTTP status error paths to use NewGitHubAPIStatusErrorResponse; removed unused fmt import
pkg/github/dependabot.go Updated 2 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/secret_scanning.go Updated 2 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking
pkg/github/security_advisories.go Updated 4 HTTP status error paths to use NewGitHubAPIStatusErrorResponse for proper error tracking

@SamMorrowDrums SamMorrowDrums force-pushed the fix/error-tracking-status-codes branch from ed3f9dc to e1bc004 Compare December 17, 2025 15:57
@SamMorrowDrums SamMorrowDrums merged commit bc5d08d into main Dec 17, 2025
39 checks passed
@SamMorrowDrums SamMorrowDrums deleted the fix/error-tracking-status-codes branch December 17, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants