From bac36c1a933c133bb8b6d11032d43d5972f48a15 Mon Sep 17 00:00:00 2001 From: yonaka15 Date: Sun, 13 Jul 2025 00:51:38 +0900 Subject: [PATCH 1/7] fix: Add SHA to get_file_contents while preserving MCP behavior (#595) Enhance get_file_contents to include SHA information without changing the existing MCP server response format. Changes: - Add Contents API call to retrieve SHA before fetching raw content - Include SHA in resourceURI (repo://owner/repo/sha/{SHA}/contents/path) - Add SHA to success messages - Update tests to verify SHA inclusion - Maintain original behavior: text files return raw text, binaries return base64 This preserves backward compatibility while providing SHA information for better file versioning support. Closes #595 --- pkg/github/repositories.go | 31 +++++++++++++++++++++++++++---- pkg/github/repositories_test.go | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 186bd2321..dc7c98dc1 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -507,6 +507,14 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t // If the path is (most likely) not to be a directory, we will // first try to get the raw content from the GitHub raw content API. if path != "" && !strings.HasSuffix(path, "/") { + // First, get file info from Contents API to retrieve SHA + var fileSHA string + opts := &github.RepositoryContentGetOptions{Ref: ref} + fileContent, _, respContents, errContents := client.Repositories.GetContents(ctx, owner, repo, path, opts) + if errContents == nil && respContents.StatusCode == http.StatusOK && fileContent != nil && fileContent.SHA != nil { + fileSHA = *fileContent.SHA + defer func() { _ = respContents.Body.Close() }() + } rawClient, err := getRawClient(ctx) if err != nil { @@ -530,6 +538,11 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t var resourceURI string switch { + case fileSHA != "": + resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", fileSHA, "contents", path) + if err != nil { + return nil, fmt.Errorf("failed to create resource URI: %w", err) + } case sha != "": resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path) if err != nil { @@ -548,18 +561,28 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t } if strings.HasPrefix(contentType, "application") || strings.HasPrefix(contentType, "text") { - return mcp.NewToolResultResource("successfully downloaded text file", mcp.TextResourceContents{ + result := mcp.TextResourceContents{ URI: resourceURI, Text: string(body), MIMEType: contentType, - }), nil + } + // Include SHA in the result metadata + if fileSHA != "" { + return mcp.NewToolResultResource(fmt.Sprintf("successfully downloaded text file (SHA: %s)", fileSHA), result), nil + } + return mcp.NewToolResultResource("successfully downloaded text file", result), nil } - return mcp.NewToolResultResource("successfully downloaded binary file", mcp.BlobResourceContents{ + result := mcp.BlobResourceContents{ URI: resourceURI, Blob: base64.StdEncoding.EncodeToString(body), MIMEType: contentType, - }), nil + } + // Include SHA in the result metadata + if fileSHA != "" { + return mcp.NewToolResultResource(fmt.Sprintf("successfully downloaded binary file (SHA: %s)", fileSHA), result), nil + } + return mcp.NewToolResultResource("successfully downloaded binary file", result), nil } } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 4977bb0a9..ccf617a96 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -76,6 +76,20 @@ func Test_GetFileContents(t *testing.T) { _, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": ""}}`)) }), ), + mock.WithRequestMatchHandler( + mock.GetReposContentsByOwnerByRepoByPath, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("README.md"), + Path: github.Ptr("README.md"), + SHA: github.Ptr("abc123"), + Type: github.Ptr("file"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }), + ), mock.WithRequestMatchHandler( raw.GetRawReposContentsByOwnerByRepoByBranchByPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -92,7 +106,7 @@ func Test_GetFileContents(t *testing.T) { }, expectError: false, expectedResult: mcp.TextResourceContents{ - URI: "repo://owner/repo/refs/heads/main/contents/README.md", + URI: "repo://owner/repo/sha/abc123/contents/README.md", Text: "# Test Repository\n\nThis is a test repository.", MIMEType: "text/markdown", }, @@ -107,6 +121,20 @@ func Test_GetFileContents(t *testing.T) { _, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": ""}}`)) }), ), + mock.WithRequestMatchHandler( + mock.GetReposContentsByOwnerByRepoByPath, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr("test.png"), + Path: github.Ptr("test.png"), + SHA: github.Ptr("def456"), + Type: github.Ptr("file"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }), + ), mock.WithRequestMatchHandler( raw.GetRawReposContentsByOwnerByRepoByBranchByPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -123,7 +151,7 @@ func Test_GetFileContents(t *testing.T) { }, expectError: false, expectedResult: mcp.BlobResourceContents{ - URI: "repo://owner/repo/refs/heads/main/contents/test.png", + URI: "repo://owner/repo/sha/def456/contents/test.png", Blob: base64.StdEncoding.EncodeToString(mockRawContent), MIMEType: "image/png", }, From 5acb47cd1cb2d2e9878e9cb1208de388e9be87f0 Mon Sep 17 00:00:00 2001 From: yonaka15 Date: Sun, 13 Jul 2025 05:45:31 +0900 Subject: [PATCH 2/7] fix: Improve error handling for Contents API response Ensure response body is properly closed even when an error occurs by moving the defer statement before the error check. This prevents potential resource leaks when the Contents API returns an error with a non-nil response. Changes: - Move defer respContents.Body.Close() before error checking - Rename errContents to err for consistency - Add nil check for respContents before attempting to close body This follows Go best practices for handling HTTP responses and prevents potential goroutine/memory leaks. --- pkg/github/repositories.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index dc7c98dc1..45620268b 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -510,11 +510,13 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t // First, get file info from Contents API to retrieve SHA var fileSHA string opts := &github.RepositoryContentGetOptions{Ref: ref} - fileContent, _, respContents, errContents := client.Repositories.GetContents(ctx, owner, repo, path, opts) - if errContents == nil && respContents.StatusCode == http.StatusOK && fileContent != nil && fileContent.SHA != nil { - fileSHA = *fileContent.SHA + fileContent, _, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts) + if respContents != nil { defer func() { _ = respContents.Body.Close() }() } + if err == nil && respContents.StatusCode == http.StatusOK && fileContent != nil && fileContent.SHA != nil { + fileSHA = *fileContent.SHA + } rawClient, err := getRawClient(ctx) if err != nil { From e650bb0abe2e477e13804d174d98ca08be03d7cf Mon Sep 17 00:00:00 2001 From: LuluBeatson Date: Tue, 15 Jul 2025 11:23:53 +0100 Subject: [PATCH 3/7] revert changes to resource URI --- pkg/github/repositories.go | 5 ----- pkg/github/repositories_test.go | 4 ++-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 45620268b..c2f95cdbc 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -540,11 +540,6 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t var resourceURI string switch { - case fileSHA != "": - resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", fileSHA, "contents", path) - if err != nil { - return nil, fmt.Errorf("failed to create resource URI: %w", err) - } case sha != "": resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path) if err != nil { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index ccf617a96..77835e66d 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -106,7 +106,7 @@ func Test_GetFileContents(t *testing.T) { }, expectError: false, expectedResult: mcp.TextResourceContents{ - URI: "repo://owner/repo/sha/abc123/contents/README.md", + URI: "repo://owner/repo/refs/heads/main/contents/README.md", Text: "# Test Repository\n\nThis is a test repository.", MIMEType: "text/markdown", }, @@ -151,7 +151,7 @@ func Test_GetFileContents(t *testing.T) { }, expectError: false, expectedResult: mcp.BlobResourceContents{ - URI: "repo://owner/repo/sha/def456/contents/test.png", + URI: "repo://owner/repo/refs/heads/main/contents/test.png", Blob: base64.StdEncoding.EncodeToString(mockRawContent), MIMEType: "image/png", }, From f1f225d571cd8c5faba244956613693a8440e80f Mon Sep 17 00:00:00 2001 From: LuluBeatson Date: Tue, 15 Jul 2025 13:30:41 +0100 Subject: [PATCH 4/7] use GraphQL API to get file SHA --- pkg/github/repositories.go | 52 +++++++++++++++++++++++++++------ pkg/github/repositories_test.go | 29 ++++++++++++++++-- pkg/github/tools.go | 2 +- 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index c2f95cdbc..1364b2b80 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -16,8 +16,13 @@ import ( "github.com/google/go-github/v72/github" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" + "github.com/shurcooL/githubv4" ) +// getFileSHAFunc is a package-level variable that holds the getFileSHA function +// This allows tests to mock the behavior +var getFileSHAFunc = getFileSHA + func GetCommit(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_commit", mcp.WithDescription(t("TOOL_GET_COMMITS_DESCRIPTION", "Get details for a commit from a GitHub repository")), @@ -446,7 +451,7 @@ func CreateRepository(getClient GetClientFn, t translations.TranslationHelperFun } // GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository. -func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_file_contents", mcp.WithDescription(t("TOOL_GET_FILE_CONTENTS_DESCRIPTION", "Get the contents of a file or directory from a GitHub repository")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -507,15 +512,13 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t // If the path is (most likely) not to be a directory, we will // first try to get the raw content from the GitHub raw content API. if path != "" && !strings.HasSuffix(path, "/") { - // First, get file info from Contents API to retrieve SHA - var fileSHA string - opts := &github.RepositoryContentGetOptions{Ref: ref} - fileContent, _, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts) - if respContents != nil { - defer func() { _ = respContents.Body.Close() }() + gqlClient, err := getGQLClient(ctx) + if err != nil { + return mcp.NewToolResultError("failed to get GitHub GraphQL client"), nil } - if err == nil && respContents.StatusCode == http.StatusOK && fileContent != nil && fileContent.SHA != nil { - fileSHA = *fileContent.SHA + fileSHA, err := getFileSHAFunc(ctx, gqlClient, owner, repo, path, rawOpts) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to get file SHA: %s", err)), nil } rawClient, err := getRawClient(ctx) @@ -1383,3 +1386,34 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner // Use provided ref, or it will be empty which defaults to the default branch return &raw.ContentOpts{Ref: ref, SHA: sha}, nil } + +// getFileSHA retrieves the Blob SHA of a file. +func getFileSHA(ctx context.Context, client *githubv4.Client, owner, repo, path string, opts *raw.ContentOpts) (string, error) { + var query struct { + Repository struct { + Object struct { + Blob struct { + OID string + } `graphql:"... on Blob"` + } `graphql:"object(expression: $expression)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + // Prepare the expression based on the provided options + expression := githubv4.String(path) + if opts != nil && opts.SHA != "" { + expression = githubv4.String(fmt.Sprintf("%s:%s", opts.SHA, path)) + } else if opts != nil && opts.Ref != "" { + expression = githubv4.String(fmt.Sprintf("%s:%s", opts.Ref, path)) + } + + variables := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "expression": expression, + } + if err := client.Query(ctx, &query, variables); err != nil { + return "", fmt.Errorf("failed to query file SHA: %w", err) + } + return query.Repository.Object.Blob.OID, nil +} diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 77835e66d..ff1c88a7c 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -15,15 +15,24 @@ import ( "github.com/google/go-github/v72/github" "github.com/mark3labs/mcp-go/mcp" "github.com/migueleliasweb/go-github-mock/src/mock" + "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// mockGetFileSHA is a test helper that mocks the getFileSHA function so that we don't need to mock the GraphQL client in Test_GetFileContents. +func mockGetFileSHA(expectedSHA string) func(context.Context, *githubv4.Client, string, string, string, *raw.ContentOpts) (string, error) { + return func(_ context.Context, _ *githubv4.Client, _, _, _ string, _ *raw.ContentOpts) (string, error) { + return expectedSHA, nil + } +} + func Test_GetFileContents(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) mockRawClient := raw.NewClient(mockClient, &url.URL{Scheme: "https", Host: "raw.githubusercontent.com", Path: "/"}) - tool, _ := GetFileContents(stubGetClientFn(mockClient), stubGetRawClientFn(mockRawClient), translations.NullTranslationHelper) + mockGQLClient := githubv4.NewClient(nil) + tool, _ := GetFileContents(stubGetClientFn(mockClient), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "get_file_contents", tool.Name) @@ -56,10 +65,10 @@ func Test_GetFileContents(t *testing.T) { HTMLURL: github.Ptr("https://github.com/owner/repo/tree/main/src"), }, } - tests := []struct { name string mockedClient *http.Client + mockFileSHA string requestArgs map[string]interface{} expectError bool expectedResult interface{} @@ -98,6 +107,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), + mockFileSHA: "abc123", requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -143,6 +153,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), + mockFileSHA: "def456", requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -188,6 +199,7 @@ func Test_GetFileContents(t *testing.T) { ), ), ), + mockFileSHA: "", // Directory content doesn't need SHA requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -221,6 +233,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), + mockFileSHA: "", // Error case doesn't need SHA requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -234,10 +247,20 @@ func Test_GetFileContents(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { + // Mock the getFileSHA function if mockFileSHA is provided + if tc.mockFileSHA != "" { + originalGetFileSHA := getFileSHAFunc + getFileSHAFunc = mockGetFileSHA(tc.mockFileSHA) + defer func() { + getFileSHAFunc = originalGetFileSHA + }() + } + // Setup client with mock client := github.NewClient(tc.mockedClient) mockRawClient := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"}) - _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), translations.NullTranslationHelper) + mockGQLClient := githubv4.NewClient(tc.mockedClient) + _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index a469b7678..0683bc52b 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -24,7 +24,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG repos := toolsets.NewToolset("repos", "GitHub Repository related tools"). AddReadTools( toolsets.NewServerTool(SearchRepositories(getClient, t)), - toolsets.NewServerTool(GetFileContents(getClient, getRawClient, t)), + toolsets.NewServerTool(GetFileContents(getClient, getRawClient, getGQLClient, t)), toolsets.NewServerTool(ListCommits(getClient, t)), toolsets.NewServerTool(SearchCode(getClient, t)), toolsets.NewServerTool(GetCommit(getClient, t)), From 505f361fc010b4357d91e5104a4236c1082c06c2 Mon Sep 17 00:00:00 2001 From: LuluBeatson Date: Tue, 15 Jul 2025 14:34:12 +0100 Subject: [PATCH 5/7] refactor: mock GQL client instead of getFileSHA function to follow conventions --- pkg/github/repositories.go | 6 +--- pkg/github/repositories_test.go | 59 ++++++++++++++++++++++----------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 1364b2b80..c09659c2c 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -19,10 +19,6 @@ import ( "github.com/shurcooL/githubv4" ) -// getFileSHAFunc is a package-level variable that holds the getFileSHA function -// This allows tests to mock the behavior -var getFileSHAFunc = getFileSHA - func GetCommit(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_commit", mcp.WithDescription(t("TOOL_GET_COMMITS_DESCRIPTION", "Get details for a commit from a GitHub repository")), @@ -516,7 +512,7 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, get if err != nil { return mcp.NewToolResultError("failed to get GitHub GraphQL client"), nil } - fileSHA, err := getFileSHAFunc(ctx, gqlClient, owner, repo, path, rawOpts) + fileSHA, err := getFileSHA(ctx, gqlClient, owner, repo, path, rawOpts) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get file SHA: %s", err)), nil } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index ff1c88a7c..77b0e8e5b 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/translations" @@ -20,11 +21,39 @@ import ( "github.com/stretchr/testify/require" ) -// mockGetFileSHA is a test helper that mocks the getFileSHA function so that we don't need to mock the GraphQL client in Test_GetFileContents. -func mockGetFileSHA(expectedSHA string) func(context.Context, *githubv4.Client, string, string, string, *raw.ContentOpts) (string, error) { - return func(_ context.Context, _ *githubv4.Client, _, _, _ string, _ *raw.ContentOpts) (string, error) { - return expectedSHA, nil +func mockGQLClientFileSHA(t *testing.T, owner, repo, expression, expectedSHA string) *githubv4.Client { + // Create the query structure that matches getFileSHA function + query := struct { + Repository struct { + Object struct { + Blob struct { + OID string + } `graphql:"... on Blob"` + } `graphql:"object(expression: $expression)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{} + + // Match any variables, we don't care about the exact values in this test + variables := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "expression": githubv4.String(expression), } + + // Create the mock response with the expected SHA + mockResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "object": map[string]any{ + "oid": expectedSHA, + }, + }, + }) + + // Create the matcher and mock client + matcher := githubv4mock.NewQueryMatcher(query, variables, mockResponse) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + + return githubv4.NewClient(httpClient) } func Test_GetFileContents(t *testing.T) { @@ -68,7 +97,7 @@ func Test_GetFileContents(t *testing.T) { tests := []struct { name string mockedClient *http.Client - mockFileSHA string + mockGQLClient *githubv4.Client requestArgs map[string]interface{} expectError bool expectedResult interface{} @@ -107,7 +136,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockFileSHA: "abc123", + mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:README.md", "abc123"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -153,7 +182,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockFileSHA: "def456", + mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:test.png", "def456"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -199,7 +228,7 @@ func Test_GetFileContents(t *testing.T) { ), ), ), - mockFileSHA: "", // Directory content doesn't need SHA + mockGQLClient: nil, requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -233,7 +262,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockFileSHA: "", // Error case doesn't need SHA + mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:nonexistent.md", ""), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -247,20 +276,10 @@ func Test_GetFileContents(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Mock the getFileSHA function if mockFileSHA is provided - if tc.mockFileSHA != "" { - originalGetFileSHA := getFileSHAFunc - getFileSHAFunc = mockGetFileSHA(tc.mockFileSHA) - defer func() { - getFileSHAFunc = originalGetFileSHA - }() - } - // Setup client with mock client := github.NewClient(tc.mockedClient) mockRawClient := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"}) - mockGQLClient := githubv4.NewClient(tc.mockedClient) - _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) + _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(tc.mockGQLClient), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) From af760f23aef328d1018a221fc6b5dcc82d39d8c1 Mon Sep 17 00:00:00 2001 From: LuluBeatson Date: Tue, 15 Jul 2025 16:35:24 +0100 Subject: [PATCH 6/7] lint --- pkg/github/repositories_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 77b0e8e5b..3403e0c74 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -21,7 +21,7 @@ import ( "github.com/stretchr/testify/require" ) -func mockGQLClientFileSHA(t *testing.T, owner, repo, expression, expectedSHA string) *githubv4.Client { +func mockGQLClientFileSHA(owner, repo, expression, expectedSHA string) *githubv4.Client { // Create the query structure that matches getFileSHA function query := struct { Repository struct { @@ -136,7 +136,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:README.md", "abc123"), + mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:README.md", "abc123"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -182,7 +182,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:test.png", "def456"), + mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:test.png", "def456"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -262,7 +262,7 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA(t, "owner", "repo", "refs/heads/main:nonexistent.md", ""), + mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:nonexistent.md", ""), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", From d1b6c3239d1127240e09b0a714cde4f639280258 Mon Sep 17 00:00:00 2001 From: LuluBeatson Date: Wed, 16 Jul 2025 16:13:46 +0100 Subject: [PATCH 7/7] revert GraphQL --- pkg/github/repositories.go | 54 ++++++++++----------------------- pkg/github/repositories_test.go | 48 ++--------------------------- pkg/github/tools.go | 2 +- 3 files changed, 20 insertions(+), 84 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 036b15701..2e56c8644 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -16,7 +16,6 @@ import ( "github.com/google/go-github/v73/github" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" - "github.com/shurcooL/githubv4" ) func GetCommit(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { @@ -447,7 +446,7 @@ func CreateRepository(getClient GetClientFn, t translations.TranslationHelperFun } // GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository. -func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_file_contents", mcp.WithDescription(t("TOOL_GET_FILE_CONTENTS_DESCRIPTION", "Get the contents of a file or directory from a GitHub repository")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -508,14 +507,24 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, get // If the path is (most likely) not to be a directory, we will // first try to get the raw content from the GitHub raw content API. if path != "" && !strings.HasSuffix(path, "/") { - gqlClient, err := getGQLClient(ctx) - if err != nil { - return mcp.NewToolResultError("failed to get GitHub GraphQL client"), nil + // First, get file info from Contents API to retrieve SHA + var fileSHA string + opts := &github.RepositoryContentGetOptions{Ref: ref} + fileContent, _, respContents, err := client.Repositories.GetContents(ctx, owner, repo, path, opts) + if respContents != nil { + defer func() { _ = respContents.Body.Close() }() } - fileSHA, err := getFileSHA(ctx, gqlClient, owner, repo, path, rawOpts) if err != nil { - return mcp.NewToolResultError(fmt.Sprintf("failed to get file SHA: %s", err)), nil + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to get file SHA", + respContents, + err, + ), nil + } + if fileContent == nil || fileContent.SHA == nil { + return mcp.NewToolResultError("file content SHA is nil"), nil } + fileSHA = *fileContent.SHA rawClient, err := getRawClient(ctx) if err != nil { @@ -1382,34 +1391,3 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner // Use provided ref, or it will be empty which defaults to the default branch return &raw.ContentOpts{Ref: ref, SHA: sha}, nil } - -// getFileSHA retrieves the Blob SHA of a file. -func getFileSHA(ctx context.Context, client *githubv4.Client, owner, repo, path string, opts *raw.ContentOpts) (string, error) { - var query struct { - Repository struct { - Object struct { - Blob struct { - OID string - } `graphql:"... on Blob"` - } `graphql:"object(expression: $expression)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - // Prepare the expression based on the provided options - expression := githubv4.String(path) - if opts != nil && opts.SHA != "" { - expression = githubv4.String(fmt.Sprintf("%s:%s", opts.SHA, path)) - } else if opts != nil && opts.Ref != "" { - expression = githubv4.String(fmt.Sprintf("%s:%s", opts.Ref, path)) - } - - variables := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "expression": expression, - } - if err := client.Query(ctx, &query, variables); err != nil { - return "", fmt.Errorf("failed to query file SHA: %w", err) - } - return query.Repository.Object.Blob.OID, nil -} diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 9ee3ff0f8..1572a12f4 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -9,59 +9,21 @@ import ( "testing" "time" - "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/raw" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v73/github" "github.com/mark3labs/mcp-go/mcp" "github.com/migueleliasweb/go-github-mock/src/mock" - "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func mockGQLClientFileSHA(owner, repo, expression, expectedSHA string) *githubv4.Client { - // Create the query structure that matches getFileSHA function - query := struct { - Repository struct { - Object struct { - Blob struct { - OID string - } `graphql:"... on Blob"` - } `graphql:"object(expression: $expression)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{} - - // Match any variables, we don't care about the exact values in this test - variables := map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "expression": githubv4.String(expression), - } - - // Create the mock response with the expected SHA - mockResponse := githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "object": map[string]any{ - "oid": expectedSHA, - }, - }, - }) - - // Create the matcher and mock client - matcher := githubv4mock.NewQueryMatcher(query, variables, mockResponse) - httpClient := githubv4mock.NewMockedHTTPClient(matcher) - - return githubv4.NewClient(httpClient) -} - func Test_GetFileContents(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) mockRawClient := raw.NewClient(mockClient, &url.URL{Scheme: "https", Host: "raw.githubusercontent.com", Path: "/"}) - mockGQLClient := githubv4.NewClient(nil) - tool, _ := GetFileContents(stubGetClientFn(mockClient), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) + tool, _ := GetFileContents(stubGetClientFn(mockClient), stubGetRawClientFn(mockRawClient), translations.NullTranslationHelper) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "get_file_contents", tool.Name) @@ -94,10 +56,10 @@ func Test_GetFileContents(t *testing.T) { HTMLURL: github.Ptr("https://github.com/owner/repo/tree/main/src"), }, } + tests := []struct { name string mockedClient *http.Client - mockGQLClient *githubv4.Client requestArgs map[string]interface{} expectError bool expectedResult interface{} @@ -136,7 +98,6 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:README.md", "abc123"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -182,7 +143,6 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:test.png", "def456"), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -228,7 +188,6 @@ func Test_GetFileContents(t *testing.T) { ), ), ), - mockGQLClient: nil, requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -262,7 +221,6 @@ func Test_GetFileContents(t *testing.T) { }), ), ), - mockGQLClient: mockGQLClientFileSHA("owner", "repo", "refs/heads/main:nonexistent.md", ""), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", @@ -279,7 +237,7 @@ func Test_GetFileContents(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) mockRawClient := raw.NewClient(client, &url.URL{Scheme: "https", Host: "raw.example.com", Path: "/"}) - _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), stubGetGQLClientFn(tc.mockGQLClient), translations.NullTranslationHelper) + _, handler := GetFileContents(stubGetClientFn(client), stubGetRawClientFn(mockRawClient), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index bf270d8f6..77a1ccd3b 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -24,7 +24,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG repos := toolsets.NewToolset("repos", "GitHub Repository related tools"). AddReadTools( toolsets.NewServerTool(SearchRepositories(getClient, t)), - toolsets.NewServerTool(GetFileContents(getClient, getRawClient, getGQLClient, t)), + toolsets.NewServerTool(GetFileContents(getClient, getRawClient, t)), toolsets.NewServerTool(ListCommits(getClient, t)), toolsets.NewServerTool(SearchCode(getClient, t)), toolsets.NewServerTool(GetCommit(getClient, t)),