Add API Error annotations to GitHub issue errors #1566
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR adds error annotation wrappers to errors returned from the GitHub API and GraphQL API in the issues functionality. The changes introduce a new helper function NewGitHubGraphQLErrorToCtx and apply error wrapping patterns to several error handling locations in the issues code.
Key Changes:
- Added
NewGitHubGraphQLErrorToCtxfunction inpkg/errors/error.goto add GraphQL errors to context - Updated error handling in
CreateIssue,ListIssues, andAssignCopilotToIssueto use new error wrapper functions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/errors/error.go | Added NewGitHubGraphQLErrorToCtx function to store GraphQL errors in context, following the pattern of the existing NewGitHubAPIErrorToCtx function |
| pkg/github/issues.go | Updated error handling in multiple functions to use NewGitHubAPIErrorResponse and NewGitHubGraphQLErrorResponse for consistent error annotation |
pkg/github/issues.go
Outdated
| _, _ = ghErrors.NewGitHubGraphQLErrorToCtx(ctx, "failed to get suggested actors", err) | ||
| return nil, nil, err |
There was a problem hiding this comment.
The error at line 1664 is inconsistently handled compared to other GraphQL errors in this file. This code uses NewGitHubGraphQLErrorToCtx which only adds the error to the context, but then returns the raw error without wrapping it in a tool result.
This is inconsistent with other error handling patterns in this function (e.g., line 1711) and throughout the file. The function should return a *mcp.CallToolResult as the first return value when an error occurs, not nil.
Consider changing this to:
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get suggested actors", err), nil, nilThis matches the pattern used at line 1711 and other locations in the file.
| _, _ = ghErrors.NewGitHubGraphQLErrorToCtx(ctx, "failed to get suggested actors", err) | |
| return nil, nil, err | |
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get suggested actors", err), nil, nil |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| return utils.NewToolResultError(err.Error()), nil, nil | ||
| return ghErrors.NewGitHubGraphQLErrorResponse( | ||
| ctx, | ||
| "failed to list issues", |
There was a problem hiding this comment.
Error message capitalization is inconsistent. The message "failed to list issues" uses lowercase "failed", while other nearby error messages in this file use uppercase "Failed" (e.g., "Failed to get issue labels" at line 518, "Failed to find issues" at line 1265). For consistency, this should use "Failed to list issues" with an uppercase F to match the existing pattern in this file.
| "failed to list issues", | |
| "Failed to list issues", |
| err := client.Query(ctx, &query, variables) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get suggested actors", err), nil, nil |
There was a problem hiding this comment.
Error message capitalization is inconsistent. The message "failed to get suggested actors" uses lowercase "failed", while other nearby error messages in this file use uppercase "Failed" (e.g., "Failed to get issue labels" at line 518, "Failed to find issues" at line 1265). For consistency, this should use "Failed to get suggested actors" with an uppercase F to match the existing pattern in this file.
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get suggested actors", err), nil, nil | |
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to get suggested actors", err), nil, nil |
|
|
||
| if err := client.Query(ctx, &getIssueQuery, variables); err != nil { | ||
| return utils.NewToolResultError(fmt.Sprintf("failed to get issue ID: %v", err)), nil, nil | ||
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get issue ID", err), nil, nil |
There was a problem hiding this comment.
Error message capitalization is inconsistent. The message "failed to get issue ID" uses lowercase "failed", while other nearby error messages in this file use uppercase "Failed" (e.g., "Failed to get issue labels" at line 518, "Failed to find issues" at line 1265). For consistency, this should use "Failed to get issue ID" with an uppercase F to match the existing pattern in this file.
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get issue ID", err), nil, nil | |
| return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to get issue ID", err), nil, nil |
Add error wrappers to errors returned from the GitHub API and GitHub GraphQL API