Skip to content

Commit 8d17eee

Browse files
SamMorrowDrumsSamMorrowDrums
committed
move to new approach and update testing
1 parent de0a621 commit 8d17eee

17 files changed

+408
-250
lines changed

‎internal/ghmcp/server.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"strings"
1313
"syscall"
1414

15+
"github.com/github/github-mcp-server/pkg/errors"
1516
"github.com/github/github-mcp-server/pkg/github"
1617
mcplog "github.com/github/github-mcp-server/pkg/log"
1718
"github.com/github/github-mcp-server/pkg/raw"
@@ -90,6 +91,13 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
9091

9192
hooks := &server.Hooks{
9293
OnBeforeInitialize: []server.OnBeforeInitializeFunc{beforeInit},
94+
OnBeforeAny: []server.BeforeAnyHookFunc{
95+
func(ctx context.Context, _ any, _ mcp.MCPMethod, _ any) {
96+
// Ensure the context is cleared of any previous errors
97+
// as context isn't propagated through middleware
98+
errors.ContextWithGitHubErrors(ctx)
99+
},
100+
},
93101
}
94102

95103
ghServer := github.NewServer(cfg.Version, server.WithHooks(hooks))
@@ -222,7 +230,8 @@ func RunStdioServer(cfg StdioServerConfig) error {
222230
loggedIO := mcplog.NewIOLogger(in, out, logrusLogger)
223231
in, out = loggedIO, loggedIO
224232
}
225-
233+
// enable GitHub errors in the context
234+
ctx := errors.ContextWithGitHubErrors(ctx)
226235
errC <- stdioServer.Listen(ctx, in, out)
227236
}()
228237

‎pkg/errors/error.go‎

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package errors
22

33
import (
4+
"context"
45
"fmt"
56

67
"github.com/google/go-github/v72/github"
8+
"github.com/mark3labs/mcp-go/mcp"
79
)
810

911
type GitHubAPIError struct {
@@ -12,7 +14,8 @@ type GitHubAPIError struct {
1214
Err error `json:"-"`
1315
}
1416

15-
func NewGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError {
17+
// NewGitHubAPIError creates a new GitHubAPIError with the provided message, response, and error.
18+
func newGitHubAPIError(message string, resp *github.Response, err error) *GitHubAPIError {
1619
return &GitHubAPIError{
1720
Message: message,
1821
Response: resp,
@@ -29,7 +32,7 @@ type GitHubGraphQLError struct {
2932
Err error `json:"-"`
3033
}
3134

32-
func NewGitHubGraphQLError(message string, err error) *GitHubGraphQLError {
35+
func newGitHubGraphQLError(message string, err error) *GitHubGraphQLError {
3336
return &GitHubGraphQLError{
3437
Message: message,
3538
Err: err,
@@ -39,3 +42,84 @@ func NewGitHubGraphQLError(message string, err error) *GitHubGraphQLError {
3942
func (e *GitHubGraphQLError) Error() string {
4043
return fmt.Errorf("%s: %w", e.Message, e.Err).Error()
4144
}
45+
46+
type GitHubErrorKey struct{}
47+
type GitHubCtxErrors struct {
48+
api []*GitHubAPIError
49+
graphQL []*GitHubGraphQLError
50+
}
51+
52+
// ContextWithGitHubErrors updates or creates a context with a pointer to GitHub error information (to be used by middleware).
53+
func ContextWithGitHubErrors(ctx context.Context) context.Context {
54+
if ctx == nil {
55+
ctx = context.Background()
56+
}
57+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
58+
// If the context already has GitHubCtxErrors, we just empty the slices to start fresh
59+
val.api = []*GitHubAPIError{}
60+
val.graphQL = []*GitHubGraphQLError{}
61+
} else {
62+
// If not, we create a new GitHubCtxErrors and set it in the context
63+
ctx = context.WithValue(ctx, GitHubErrorKey{}, &GitHubCtxErrors{})
64+
}
65+
66+
return ctx
67+
}
68+
69+
// GetGitHubAPIErrors retrieves the slice of GitHubAPIErrors from the context.
70+
func GetGitHubAPIErrors(ctx context.Context) ([]*GitHubAPIError, error) {
71+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
72+
return val.api, nil // return the slice of API errors from the context
73+
}
74+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
75+
}
76+
77+
// GetGitHubGraphQLErrors retrieves the slice of GitHubGraphQLErrors from the context.
78+
func GetGitHubGraphQLErrors(ctx context.Context) ([]*GitHubGraphQLError, error) {
79+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
80+
return val.graphQL, nil // return the slice of GraphQL errors from the context
81+
}
82+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
83+
}
84+
85+
func NewGitHubAPIErrorToCtx(ctx context.Context, message string, resp *github.Response, err error) (context.Context, error) {
86+
apiErr := newGitHubAPIError(message, resp, err)
87+
if ctx != nil {
88+
addGitHubAPIErrorToContext(ctx, apiErr)
89+
}
90+
return ctx, nil
91+
}
92+
93+
func addGitHubAPIErrorToContext(ctx context.Context, err *GitHubAPIError) (context.Context, error) {
94+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
95+
val.api = append(val.api, err) // append the error to the existing slice in the context
96+
return ctx, nil
97+
}
98+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
99+
}
100+
101+
func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError) (context.Context, error) {
102+
if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok {
103+
val.graphQL = append(val.graphQL, err) // append the error to the existing slice in the context
104+
return ctx, nil
105+
}
106+
return nil, fmt.Errorf("context does not contain GitHubCtxErrors")
107+
}
108+
109+
// NewGitHubAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware
110+
func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github.Response, err error) *mcp.CallToolResult {
111+
apiErr := newGitHubAPIError(message, resp, err)
112+
if ctx != nil {
113+
addGitHubAPIErrorToContext(ctx, apiErr)
114+
}
115+
return mcp.NewToolResultErrorFromErr(message, err)
116+
}
117+
118+
// NewGitHubGraphQLErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware
119+
func NewGitHubGraphQLErrorResponse(ctx context.Context, message string, err error) *mcp.CallToolResult {
120+
graphQLErr := newGitHubGraphQLError(message, err)
121+
if ctx != nil {
122+
addGitHubGraphQLErrorToContext(ctx, graphQLErr)
123+
}
124+
return mcp.NewToolResultErrorFromErr(message, err)
125+
}

‎pkg/github/actions.go‎

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"strconv"
1010
"strings"
1111

12+
ghErrors "github.com/github/github-mcp-server/pkg/errors"
1213
"github.com/github/github-mcp-server/pkg/translations"
1314
"github.com/google/go-github/v72/github"
1415
"github.com/mark3labs/mcp-go/mcp"
@@ -644,7 +645,7 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
644645
Filter: "latest",
645646
})
646647
if err != nil {
647-
return mcp.NewToolResultError(fmt.Sprintf("failed to list workflow jobs: %v", err)), nil
648+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list workflow jobs", resp, err), nil
648649
}
649650
defer func() { _ = resp.Body.Close() }()
650651

@@ -670,15 +671,18 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
670671
// Collect logs for all failed jobs
671672
var logResults []map[string]any
672673
for _, job := range failedJobs {
673-
jobResult, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent)
674+
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent)
674675
if err != nil {
675676
// Continue with other jobs even if one fails
676677
jobResult = map[string]any{
677678
"job_id": job.GetID(),
678679
"job_name": job.GetName(),
679680
"error": err.Error(),
680681
}
682+
// Enable reporting of status codes and error causes
683+
ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get job logs", resp, err)
681684
}
685+
682686
logResults = append(logResults, jobResult)
683687
}
684688

@@ -701,9 +705,9 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
701705

702706
// handleSingleJobLogs gets logs for a single job
703707
func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool) (*mcp.CallToolResult, error) {
704-
jobResult, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent)
708+
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent)
705709
if err != nil {
706-
return mcp.NewToolResultError(err.Error()), nil
710+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get job logs", resp, err), nil
707711
}
708712

709713
r, err := json.Marshal(jobResult)
@@ -715,11 +719,11 @@ func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo
715719
}
716720

717721
// getJobLogData retrieves log data for a single job, either as URL or content
718-
func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool) (map[string]any, error) {
722+
func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool) (map[string]any, *github.Response, error) {
719723
// Get the download URL for the job logs
720724
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, owner, repo, jobID, 1)
721725
if err != nil {
722-
return nil, fmt.Errorf("failed to get job logs for job %d: %w", jobID, err)
726+
return nil, resp, fmt.Errorf("failed to get job logs for job %d: %w", jobID, err)
723727
}
724728
defer func() { _ = resp.Body.Close() }()
725729

@@ -732,9 +736,10 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin
732736

733737
if returnContent {
734738
// Download and return the actual log content
735-
content, err := downloadLogContent(url.String())
739+
// TODO we can use a generic http error or an interface instead of github.Response
740+
content, _, err := downloadLogContent(url.String())
736741
if err != nil {
737-
return nil, fmt.Errorf("failed to download log content for job %d: %w", jobID, err)
742+
return nil, nil, fmt.Errorf("failed to download log content for job %d: %w", jobID, err)
738743
}
739744
result["logs_content"] = content
740745
result["message"] = "Job logs content retrieved successfully"
@@ -745,29 +750,29 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin
745750
result["note"] = "The logs_url provides a download link for the individual job logs in plain text format. Use return_content=true to get the actual log content."
746751
}
747752

748-
return result, nil
753+
return result, resp, nil
749754
}
750755

751756
// downloadLogContent downloads the actual log content from a GitHub logs URL
752-
func downloadLogContent(logURL string) (string, error) {
757+
func downloadLogContent(logURL string) (string, *http.Response, error) {
753758
httpResp, err := http.Get(logURL) //nolint:gosec // URLs are provided by GitHub API and are safe
754759
if err != nil {
755-
return "", fmt.Errorf("failed to download logs: %w", err)
760+
return "", httpResp, fmt.Errorf("failed to download logs: %w", err)
756761
}
757762
defer func() { _ = httpResp.Body.Close() }()
758763

759764
if httpResp.StatusCode != http.StatusOK {
760-
return "", fmt.Errorf("failed to download logs: HTTP %d", httpResp.StatusCode)
765+
return "", httpResp, fmt.Errorf("failed to download logs: HTTP %d", httpResp.StatusCode)
761766
}
762767

763768
content, err := io.ReadAll(httpResp.Body)
764769
if err != nil {
765-
return "", fmt.Errorf("failed to read log content: %w", err)
770+
return "", httpResp, fmt.Errorf("failed to read log content: %w", err)
766771
}
767772

768773
// Clean up and format the log content for better readability
769774
logContent := strings.TrimSpace(string(content))
770-
return logContent, nil
775+
return logContent, httpResp, nil
771776
}
772777

773778
// RerunWorkflowRun creates a tool to re-run an entire workflow run
@@ -813,7 +818,7 @@ func RerunWorkflowRun(getClient GetClientFn, t translations.TranslationHelperFun
813818

814819
resp, err := client.Actions.RerunWorkflowByID(ctx, owner, repo, runID)
815820
if err != nil {
816-
return nil, fmt.Errorf("failed to rerun workflow run: %w", err)
821+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to rerun workflow run", resp, err), nil
817822
}
818823
defer func() { _ = resp.Body.Close() }()
819824

@@ -876,7 +881,7 @@ func RerunFailedJobs(getClient GetClientFn, t translations.TranslationHelperFunc
876881

877882
resp, err := client.Actions.RerunFailedJobsByID(ctx, owner, repo, runID)
878883
if err != nil {
879-
return nil, fmt.Errorf("failed to rerun failed jobs: %w", err)
884+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to rerun failed jobs", resp, err), nil
880885
}
881886
defer func() { _ = resp.Body.Close() }()
882887

@@ -939,7 +944,7 @@ func CancelWorkflowRun(getClient GetClientFn, t translations.TranslationHelperFu
939944

940945
resp, err := client.Actions.CancelWorkflowRunByID(ctx, owner, repo, runID)
941946
if err != nil {
942-
return nil, fmt.Errorf("failed to cancel workflow run: %w", err)
947+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to cancel workflow run", resp, err), nil
943948
}
944949
defer func() { _ = resp.Body.Close() }()
945950

@@ -1024,7 +1029,7 @@ func ListWorkflowRunArtifacts(getClient GetClientFn, t translations.TranslationH
10241029

10251030
artifacts, resp, err := client.Actions.ListWorkflowRunArtifacts(ctx, owner, repo, runID, opts)
10261031
if err != nil {
1027-
return nil, fmt.Errorf("failed to list workflow run artifacts: %w", err)
1032+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list workflow run artifacts", resp, err), nil
10281033
}
10291034
defer func() { _ = resp.Body.Close() }()
10301035

@@ -1081,7 +1086,7 @@ func DownloadWorkflowRunArtifact(getClient GetClientFn, t translations.Translati
10811086
// Get the download URL for the artifact
10821087
url, resp, err := client.Actions.DownloadArtifact(ctx, owner, repo, artifactID, 1)
10831088
if err != nil {
1084-
return nil, fmt.Errorf("failed to get artifact download URL: %w", err)
1089+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get artifact download URL", resp, err), nil
10851090
}
10861091
defer func() { _ = resp.Body.Close() }()
10871092

@@ -1146,7 +1151,7 @@ func DeleteWorkflowRunLogs(getClient GetClientFn, t translations.TranslationHelp
11461151

11471152
resp, err := client.Actions.DeleteWorkflowRunLogs(ctx, owner, repo, runID)
11481153
if err != nil {
1149-
return nil, fmt.Errorf("failed to delete workflow run logs: %w", err)
1154+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to delete workflow run logs", resp, err), nil
11501155
}
11511156
defer func() { _ = resp.Body.Close() }()
11521157

@@ -1209,7 +1214,7 @@ func GetWorkflowRunUsage(getClient GetClientFn, t translations.TranslationHelper
12091214

12101215
usage, resp, err := client.Actions.GetWorkflowRunUsageByID(ctx, owner, repo, runID)
12111216
if err != nil {
1212-
return nil, fmt.Errorf("failed to get workflow run usage: %w", err)
1217+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get workflow run usage", resp, err), nil
12131218
}
12141219
defer func() { _ = resp.Body.Close() }()
12151220

‎pkg/github/code_scanning.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ func GetCodeScanningAlert(getClient GetClientFn, t translations.TranslationHelpe
5555

5656
alert, resp, err := client.CodeScanning.GetAlert(ctx, owner, repo, int64(alertNumber))
5757
if err != nil {
58-
return nil, ghErrors.NewGitHubAPIError(
58+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
5959
"failed to get alert",
6060
resp,
6161
err,
62-
)
62+
), nil
6363
}
6464
defer func() { _ = resp.Body.Close() }()
6565

@@ -143,11 +143,11 @@ func ListCodeScanningAlerts(getClient GetClientFn, t translations.TranslationHel
143143
}
144144
alerts, resp, err := client.CodeScanning.ListAlertsForRepo(ctx, owner, repo, &github.AlertListOptions{Ref: ref, State: state, Severity: severity, ToolName: toolName})
145145
if err != nil {
146-
return nil, ghErrors.NewGitHubAPIError(
146+
return ghErrors.NewGitHubAPIErrorResponse(ctx,
147147
"failed to list alerts",
148148
resp,
149149
err,
150-
)
150+
), nil
151151
}
152152
defer func() { _ = resp.Body.Close() }()
153153

‎pkg/github/code_scanning_test.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,12 +94,15 @@ func Test_GetCodeScanningAlert(t *testing.T) {
9494

9595
// Verify results
9696
if tc.expectError {
97-
require.Error(t, err)
98-
assert.Contains(t, err.Error(), tc.expectedErrMsg)
97+
require.NoError(t, err)
98+
require.True(t, result.IsError)
99+
errorContent := getErrorResult(t, result)
100+
assert.Contains(t, errorContent.Text, tc.expectedErrMsg)
99101
return
100102
}
101103

102104
require.NoError(t, err)
105+
require.False(t, result.IsError)
103106

104107
// Parse the result and get the text content if no error
105108
textContent := getTextResult(t, result)
@@ -217,12 +220,15 @@ func Test_ListCodeScanningAlerts(t *testing.T) {
217220

218221
// Verify results
219222
if tc.expectError {
220-
require.Error(t, err)
221-
assert.Contains(t, err.Error(), tc.expectedErrMsg)
223+
require.NoError(t, err)
224+
require.True(t, result.IsError)
225+
errorContent := getErrorResult(t, result)
226+
assert.Contains(t, errorContent.Text, tc.expectedErrMsg)
222227
return
223228
}
224229

225230
require.NoError(t, err)
231+
require.False(t, result.IsError)
226232

227233
// Parse the result and get the text content if no error
228234
textContent := getTextResult(t, result)

0 commit comments

Comments
 (0)