Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces context-based error middleware support by capturing GitHub API and GraphQL errors in the context, exposing them for later inspection rather than returning raw errors. Key changes include:
- Swapping raw
fmt.Errorfreturns forNewGitHubAPIErrorResponse/NewGitHubGraphQLErrorResponseto store errors in context. - Updating tests to expect
result.IsErrorand extract error text viagetErrorResult. - Initializing and clearing error context in server hooks (
ContextWithGitHubErrors).
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/secret_scanning.go | Replaced raw error returns with NewGitHubAPIErrorResponse. |
| pkg/github/search.go | Same pattern for search endpoints. |
| pkg/github/repositories.go | Wrapped repository errors in context and result responses. |
| pkg/github/pullrequests.go | Applied context error handling to pull request operations. |
| pkg/github/notifications.go | Added context error responses for notifications API calls. |
| pkg/github/context_tools.go | Swapped user error return for API error response in GetMe. |
| pkg/github/actions.go | Adjusted job log handlers to return and store context errors. |
| pkg/github/code_scanning.go | Consistent context error wrapping for code scanning endpoints. |
| pkg/errors/error.go | Introduced GitHubAPIError, GitHubGraphQLError, and helpers. |
| internal/ghmcp/server.go | Added hooks to initialize/clear GitHub error context. |
Various _test.go files |
Updated tests to check result.IsError and inspect error text. |
Comments suppressed due to low confidence (1)
pkg/github/context_tools.go:32
- [nitpick] Consider renaming the variable
restorespfor consistency with other GitHub client calls.
user, res, err := client.Users.Get(ctx, "")
ef16fd8 to
ffd99aa
Compare
JoannaaKL
approved these changes
Jun 24, 2025
omgitsads
approved these changes
Jun 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Primarily for the remote server, this is the minimal approach @omgitsads and I could find to enable us to store API errors on the context, so that we can use the error types and http status codes etc. to derive the nature of failures, so that we can understand if failed tool calls occur due to:
Without this knowledge it's hard for us to validate if changes break things or not.
This is additionally frustrated by the fact mcp-go currently doesn't propagate context through each next step, and so we have concluded that if we create a value pointer on the context, we can edit the values without needing to propagate the context, and we are using this to be able to implement the aforementioned middleware, which can then extract the errors and inspect them without logging them directly (as that would expose PII sometimes), and so it's best to be able to use
Errors.Ischecks. The middleware is not present in this repo (although we could also provide debug logs on the STDIO server using the server log message protocol, which would be a nice addition and likely a follow-up to this work).Developer facing changes
Instead of directly returning mcp tool error responses, tool authors will call functions that will both return the tool error response, but will also add the actual errors to the context for later inspection.
Additional change
Errors that are ones the user should be able to do something about need to be returned as failed tool calls and not as the error value, so I have fixed a few of those in this PR.
I have left things like failures to marhsall response json, as I would consider that a real error and likely something that requires developers to fix.