- Notifications
You must be signed in to change notification settings - Fork3.3k
Add raw client error annotation and annotate GetFileContents#1570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
There was a problem hiding this 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 adds support for annotating raw client errors in the context to enable error-type-based filtering in metrics. The implementation follows the existing patterns for GitHub API and GraphQL errors.
Key changes:
- Introduced
GitHubRawAPIErrortype and associated error handling functions - Added raw error tracking to
GitHubCtxErrorscontext structure - Updated
GetFileContentsto use the new raw error annotation instead of generic error handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/errors/error.go | AddsGitHubRawAPIError type, context tracking, andNewGitHubRawAPIErrorResponse function for annotating raw API errors |
| pkg/github/repositories.go | Updates error handling inGetFileContents to use the new raw API error annotation |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Uh oh!
There was an error while loading.Please reload this page.
| 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) |
CopilotAIDec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test should verify the Error() method output for consistency with the other error type tests. The API error test (line 41) and GraphQL error test (line 63) both verify the Error() method output, but this test doesn't. Add an assertion like:assert.Equal(t, "failed to fetch raw content: raw content not found", rawError.Error())
b4b4eaf to349877fCompareb79d126 intomainUh oh!
There was an error while loading.Please reload this page.
Annotate raw client errors in the context, to allow us to filter tool call failures in our metrics based on the error type.