From 9affbc55e9adb27daa0c039fae0d136d9ed9aa88 Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Mon, 28 Apr 2025 10:49:15 -0700 Subject: [PATCH 1/7] feat: add DeleteFile tool to delete files from GitHub repositories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- pkg/github/repositories.go | 90 +++++++++++++++++++++ pkg/github/repositories_test.go | 136 ++++++++++++++++++++++++++++++++ pkg/github/tools.go | 1 + 3 files changed, 227 insertions(+) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index fa69de55..ff093147 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -556,6 +556,96 @@ func ForkRepository(getClient GetClientFn, t translations.TranslationHelperFunc) } } +// DeleteFile creates a tool to delete a file in a GitHub repository. +func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { + return mcp.NewTool("delete_file", + mcp.WithDescription(t("TOOL_DELETE_FILE_DESCRIPTION", "Delete a file from a GitHub repository")), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("Repository owner (username or organization)"), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("Repository name"), + ), + mcp.WithString("path", + mcp.Required(), + mcp.Description("Path to the file to delete"), + ), + mcp.WithString("message", + mcp.Required(), + mcp.Description("Commit message"), + ), + mcp.WithString("branch", + mcp.Required(), + mcp.Description("Branch to delete the file from"), + ), + mcp.WithString("sha", + mcp.Required(), + mcp.Description("SHA of the file being deleted"), + ), + ), + func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + owner, err := requiredParam[string](request, "owner") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + repo, err := requiredParam[string](request, "repo") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + path, err := requiredParam[string](request, "path") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + message, err := requiredParam[string](request, "message") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + branch, err := requiredParam[string](request, "branch") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + sha, err := requiredParam[string](request, "sha") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + // Create the file options + opts := &github.RepositoryContentFileOptions{ + Message: github.Ptr(message), + Branch: github.Ptr(branch), + SHA: github.Ptr(sha), + } + + // Delete the file + client, err := getClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + deleteResponse, resp, err := client.Repositories.DeleteFile(ctx, owner, repo, path, opts) + if err != nil { + return nil, fmt.Errorf("failed to delete file: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != 200 && resp.StatusCode != 204 { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to delete file: %s", string(body))), nil + } + + r, err := json.Marshal(deleteResponse) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return mcp.NewToolResultText(string(r)), nil + } +} + // CreateBranch creates a tool to create a new branch. func CreateBranch(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("create_branch", diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 59d19fc4..5dbbaeaa 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1529,6 +1529,142 @@ func Test_ListBranches(t *testing.T) { } } +func Test_DeleteFile(t *testing.T) { + // Verify tool definition once + mockClient := github.NewClient(nil) + tool, _ := DeleteFile(stubGetClientFn(mockClient), translations.NullTranslationHelper) + + assert.Equal(t, "delete_file", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "path") + assert.Contains(t, tool.InputSchema.Properties, "message") + assert.Contains(t, tool.InputSchema.Properties, "branch") + assert.Contains(t, tool.InputSchema.Properties, "sha") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "path", "message", "branch", "sha"}) + + // Setup mock delete response + mockDeleteResponse := &github.RepositoryContentResponse{ + Content: &github.RepositoryContent{ + Name: github.Ptr("example.md"), + Path: github.Ptr("docs/example.md"), + SHA: github.Ptr("abc123def456"), + HTMLURL: github.Ptr("https://github.com/owner/repo/blob/main/docs/example.md"), + }, + Commit: github.Commit{ + SHA: github.Ptr("def456abc789"), + Message: github.Ptr("Delete example file"), + Author: &github.CommitAuthor{ + Name: github.Ptr("Test User"), + Email: github.Ptr("test@example.com"), + Date: &github.Timestamp{Time: time.Now()}, + }, + HTMLURL: github.Ptr("https://github.com/owner/repo/commit/def456abc789"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedResult *github.RepositoryContentResponse + expectedErrMsg string + }{ + { + name: "successful file deletion", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteReposContentsByOwnerByRepoByPath, + expectRequestBody(t, map[string]interface{}{ + "message": "Delete example file", + "branch": "main", + "sha": "abc123def456", + "content": interface{}(nil), + }).andThen( + mockResponse(t, http.StatusOK, mockDeleteResponse), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "path": "docs/example.md", + "message": "Delete example file", + "branch": "main", + "sha": "abc123def456", + }, + expectError: false, + expectedResult: mockDeleteResponse, + }, + { + name: "file deletion fails", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.DeleteReposContentsByOwnerByRepoByPath, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "path": "docs/nonexistent.md", + "message": "Delete nonexistent file", + "branch": "main", + "sha": "abc123def456", + }, + expectError: true, + expectedErrMsg: "failed to delete file", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + _, handler := DeleteFile(stubGetClientFn(client), translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + + // Verify results + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + + require.NoError(t, err) + + // Parse the result and get the text content if no error + textContent := getTextResult(t, result) + + // Unmarshal and verify the result + var returnedContent github.RepositoryContentResponse + err = json.Unmarshal([]byte(textContent.Text), &returnedContent) + require.NoError(t, err) + + // Verify content + if tc.expectedResult.Content != nil { + assert.Equal(t, *tc.expectedResult.Content.Name, *returnedContent.Content.Name) + assert.Equal(t, *tc.expectedResult.Content.Path, *returnedContent.Content.Path) + assert.Equal(t, *tc.expectedResult.Content.SHA, *returnedContent.Content.SHA) + } + + // Verify commit + assert.Equal(t, *tc.expectedResult.Commit.SHA, *returnedContent.Commit.SHA) + assert.Equal(t, *tc.expectedResult.Commit.Message, *returnedContent.Commit.Message) + }) + } +} + func Test_ListTags(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 0d809978..faef86ce 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -36,6 +36,7 @@ func InitToolsets(passedToolsets []string, readOnly bool, getClient GetClientFn, toolsets.NewServerTool(ForkRepository(getClient, t)), toolsets.NewServerTool(CreateBranch(getClient, t)), toolsets.NewServerTool(PushFiles(getClient, t)), + toolsets.NewServerTool(DeleteFile(getClient, t)), ) issues := toolsets.NewToolset("issues", "GitHub Issues related tools"). AddReadTools( From 3d5f6b76dac91ef1b78237d9a3fd77459be36507 Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Tue, 29 Apr 2025 07:06:46 -0700 Subject: [PATCH 2/7] feat: add ReadOnlyHint and DestructiveHint annotations to DeleteFile tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- pkg/github/repositories.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index ff093147..926812cf 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -560,6 +560,11 @@ func ForkRepository(getClient GetClientFn, t translations.TranslationHelperFunc) func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("delete_file", mcp.WithDescription(t("TOOL_DELETE_FILE_DESCRIPTION", "Delete a file from a GitHub repository")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_DELETE_FILE_USER_TITLE", "Delete file"), + ReadOnlyHint: false, + DestructiveHint: true, + }), mcp.WithString("owner", mcp.Required(), mcp.Description("Repository owner (username or organization)"), From 4a5f2903cbf0fa00c5aadd94d1eb33c17d9894fb Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Thu, 1 May 2025 13:00:02 -0700 Subject: [PATCH 3/7] switch to approach that does commit signing --- pkg/github/repositories.go | 74 ++++++++++++----- pkg/github/repositories_test.go | 143 ++++++++++++++++++++------------ 2 files changed, 143 insertions(+), 74 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 926812cf..7f53aee6 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -585,10 +585,6 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to mcp.Required(), mcp.Description("Branch to delete the file from"), ), - mcp.WithString("sha", - mcp.Required(), - mcp.Description("SHA of the file being deleted"), - ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := requiredParam[string](request, "owner") @@ -611,38 +607,70 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to if err != nil { return mcp.NewToolResultError(err.Error()), nil } - sha, err := requiredParam[string](request, "sha") + + client, err := getClient(ctx) if err != nil { - return mcp.NewToolResultError(err.Error()), nil + return nil, fmt.Errorf("failed to get GitHub client: %w", err) } - // Create the file options - opts := &github.RepositoryContentFileOptions{ - Message: github.Ptr(message), - Branch: github.Ptr(branch), - SHA: github.Ptr(sha), + // Get the reference for the branch + ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch) + if err != nil { + return nil, fmt.Errorf("failed to get branch reference: %w", err) } + defer func() { _ = resp.Body.Close() }() - // Delete the file - client, err := getClient(ctx) + // Get the commit object that the branch points to + baseCommit, resp, err := client.Git.GetCommit(ctx, owner, repo, *ref.Object.SHA) if err != nil { - return nil, fmt.Errorf("failed to get GitHub client: %w", err) + return nil, fmt.Errorf("failed to get base commit: %w", err) } - deleteResponse, resp, err := client.Repositories.DeleteFile(ctx, owner, repo, path, opts) + defer func() { _ = resp.Body.Close() }() + + // Create a tree entry for the file deletion by setting SHA to nil + treeEntries := []*github.TreeEntry{ + { + Path: github.Ptr(path), + Mode: github.Ptr("100644"), // Regular file mode + Type: github.Ptr("blob"), + SHA: nil, // Setting SHA to nil deletes the file + }, + } + + // Create a new tree with the deletion + newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, treeEntries) if err != nil { - return nil, fmt.Errorf("failed to delete file: %w", err) + return nil, fmt.Errorf("failed to create tree: %w", err) } defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != 200 && resp.StatusCode != 204 { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) - } - return mcp.NewToolResultError(fmt.Sprintf("failed to delete file: %s", string(body))), nil + // Create a new commit with the new tree + commit := &github.Commit{ + Message: github.Ptr(message), + Tree: newTree, + Parents: []*github.Commit{{SHA: baseCommit.SHA}}, + } + newCommit, resp, err := client.Git.CreateCommit(ctx, owner, repo, commit, nil) + if err != nil { + return nil, fmt.Errorf("failed to create commit: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + // Update the branch reference to point to the new commit + ref.Object.SHA = newCommit.SHA + _, resp, err = client.Git.UpdateRef(ctx, owner, repo, ref, false) + if err != nil { + return nil, fmt.Errorf("failed to update reference: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + // Create a response similar to what the DeleteFile API would return + response := map[string]interface{}{ + "commit": newCommit, + "content": nil, } - r, err := json.Marshal(deleteResponse) + r, err := json.Marshal(response) if err != nil { return nil, fmt.Errorf("failed to marshal response: %w", err) } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 5dbbaeaa..b0a90c0f 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1541,49 +1541,96 @@ func Test_DeleteFile(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "path") assert.Contains(t, tool.InputSchema.Properties, "message") assert.Contains(t, tool.InputSchema.Properties, "branch") - assert.Contains(t, tool.InputSchema.Properties, "sha") - assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "path", "message", "branch", "sha"}) + // SHA is no longer required since we're using Git Data API + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "path", "message", "branch"}) - // Setup mock delete response - mockDeleteResponse := &github.RepositoryContentResponse{ - Content: &github.RepositoryContent{ - Name: github.Ptr("example.md"), - Path: github.Ptr("docs/example.md"), - SHA: github.Ptr("abc123def456"), - HTMLURL: github.Ptr("https://github.com/owner/repo/blob/main/docs/example.md"), + // Setup mock objects for Git Data API + mockRef := &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("abc123"), }, - Commit: github.Commit{ - SHA: github.Ptr("def456abc789"), - Message: github.Ptr("Delete example file"), - Author: &github.CommitAuthor{ - Name: github.Ptr("Test User"), - Email: github.Ptr("test@example.com"), - Date: &github.Timestamp{Time: time.Now()}, - }, - HTMLURL: github.Ptr("https://github.com/owner/repo/commit/def456abc789"), + } + + mockCommit := &github.Commit{ + SHA: github.Ptr("abc123"), + Tree: &github.Tree{ + SHA: github.Ptr("def456"), }, } + mockTree := &github.Tree{ + SHA: github.Ptr("ghi789"), + } + + mockNewCommit := &github.Commit{ + SHA: github.Ptr("jkl012"), + Message: github.Ptr("Delete example file"), + HTMLURL: github.Ptr("https://github.com/owner/repo/commit/jkl012"), + } + tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]interface{} - expectError bool - expectedResult *github.RepositoryContentResponse - expectedErrMsg string + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedCommitSHA string + expectedErrMsg string }{ { - name: "successful file deletion", + name: "successful file deletion using Git Data API", mockedClient: mock.NewMockedHTTPClient( + // Get branch reference + mock.WithRequestMatch( + mock.GetReposGitRefByOwnerByRepoByRef, + mockRef, + ), + // Get commit + mock.WithRequestMatch( + mock.GetReposGitCommitsByOwnerByRepoByCommitSha, + mockCommit, + ), + // Create tree + mock.WithRequestMatchHandler( + mock.PostReposGitTreesByOwnerByRepo, + expectRequestBody(t, map[string]interface{}{ + "base_tree": "def456", + "tree": []interface{}{ + map[string]interface{}{ + "path": "docs/example.md", + "mode": "100644", + "type": "blob", + "sha": nil, + }, + }, + }).andThen( + mockResponse(t, http.StatusCreated, mockTree), + ), + ), + // Create commit mock.WithRequestMatchHandler( - mock.DeleteReposContentsByOwnerByRepoByPath, + mock.PostReposGitCommitsByOwnerByRepo, expectRequestBody(t, map[string]interface{}{ "message": "Delete example file", - "branch": "main", - "sha": "abc123def456", - "content": interface{}(nil), + "tree": "ghi789", + "parents": []interface{}{"abc123"}, + }).andThen( + mockResponse(t, http.StatusCreated, mockNewCommit), + ), + ), + // Update reference + mock.WithRequestMatchHandler( + mock.PatchReposGitRefsByOwnerByRepoByRef, + expectRequestBody(t, map[string]interface{}{ + "sha": "jkl012", + "force": false, }).andThen( - mockResponse(t, http.StatusOK, mockDeleteResponse), + mockResponse(t, http.StatusOK, &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("jkl012"), + }, + }), ), ), ), @@ -1593,19 +1640,18 @@ func Test_DeleteFile(t *testing.T) { "path": "docs/example.md", "message": "Delete example file", "branch": "main", - "sha": "abc123def456", }, - expectError: false, - expectedResult: mockDeleteResponse, + expectError: false, + expectedCommitSHA: "jkl012", }, { - name: "file deletion fails", + name: "file deletion fails - branch not found", mockedClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( - mock.DeleteReposContentsByOwnerByRepoByPath, + mock.GetReposGitRefByOwnerByRepoByRef, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + _, _ = w.Write([]byte(`{"message": "Reference not found"}`)) }), ), ), @@ -1614,11 +1660,10 @@ func Test_DeleteFile(t *testing.T) { "repo": "repo", "path": "docs/nonexistent.md", "message": "Delete nonexistent file", - "branch": "main", - "sha": "abc123def456", + "branch": "nonexistent-branch", }, expectError: true, - expectedErrMsg: "failed to delete file", + expectedErrMsg: "failed to get branch reference", }, } @@ -1647,20 +1692,16 @@ func Test_DeleteFile(t *testing.T) { textContent := getTextResult(t, result) // Unmarshal and verify the result - var returnedContent github.RepositoryContentResponse - err = json.Unmarshal([]byte(textContent.Text), &returnedContent) + var response map[string]interface{} + err = json.Unmarshal([]byte(textContent.Text), &response) require.NoError(t, err) - // Verify content - if tc.expectedResult.Content != nil { - assert.Equal(t, *tc.expectedResult.Content.Name, *returnedContent.Content.Name) - assert.Equal(t, *tc.expectedResult.Content.Path, *returnedContent.Content.Path) - assert.Equal(t, *tc.expectedResult.Content.SHA, *returnedContent.Content.SHA) - } - - // Verify commit - assert.Equal(t, *tc.expectedResult.Commit.SHA, *returnedContent.Commit.SHA) - assert.Equal(t, *tc.expectedResult.Commit.Message, *returnedContent.Commit.Message) + // Verify the response contains the expected commit + commit, ok := response["commit"].(map[string]interface{}) + require.True(t, ok) + commitSHA, ok := commit["sha"].(string) + require.True(t, ok) + assert.Equal(t, tc.expectedCommitSHA, commitSHA) }) } } From 880629f4d9eb74764349a69a43fe7e67cbc6c301 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 12 May 2025 13:58:16 +0200 Subject: [PATCH 4/7] Add e2e tests for file and directory deletion --- e2e/e2e_test.go | 403 ++++++++++++++++++++++++++++++++ pkg/github/repositories.go | 4 +- pkg/github/repositories_test.go | 20 +- 3 files changed, 415 insertions(+), 12 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index b6637191..489681e9 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -4,6 +4,7 @@ package e2e_test import ( "context" + "encoding/base64" "encoding/json" "fmt" "os" @@ -369,3 +370,405 @@ func TestTags(t *testing.T) { require.Equal(t, "v0.0.1", trimmedTag[0].Name, "expected tag name to match") require.Equal(t, *ref.Object.SHA, trimmedTag[0].Commit.SHA, "expected tag SHA to match") } + +func TestFileDeletion(t *testing.T) { + t.Parallel() + + mcpClient := setupMCPClient(t) + + ctx := context.Background() + + // First, who am I + getMeRequest := mcp.CallToolRequest{} + getMeRequest.Params.Name = "get_me" + + t.Log("Getting current user...") + resp, err := mcpClient.CallTool(ctx, getMeRequest) + require.NoError(t, err, "expected to call 'get_me' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + require.False(t, resp.IsError, "expected result not to be an error") + require.Len(t, resp.Content, 1, "expected content to have one item") + + textContent, ok := resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetMeText struct { + Login string `json:"login"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetMeText) + require.NoError(t, err, "expected to unmarshal text content successfully") + + currentOwner := trimmedGetMeText.Login + + // Then create a repository with a README (via autoInit) + repoName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli()) + createRepoRequest := mcp.CallToolRequest{} + createRepoRequest.Params.Name = "create_repository" + createRepoRequest.Params.Arguments = map[string]any{ + "name": repoName, + "private": true, + "autoInit": true, + } + t.Logf("Creating repository %s/%s...", currentOwner, repoName) + _, err = mcpClient.CallTool(ctx, createRepoRequest) + require.NoError(t, err, "expected to call 'get_me' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Cleanup the repository after the test + t.Cleanup(func() { + // MCP Server doesn't support deletions, but we can use the GitHub Client + ghClient := gogithub.NewClient(nil).WithAuthToken(getE2EToken(t)) + t.Logf("Deleting repository %s/%s...", currentOwner, repoName) + _, err := ghClient.Repositories.Delete(context.Background(), currentOwner, repoName) + require.NoError(t, err, "expected to delete repository successfully") + }) + + // Create a branch on which to create a new commit + createBranchRequest := mcp.CallToolRequest{} + createBranchRequest.Params.Name = "create_branch" + createBranchRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "branch": "test-branch", + "from_branch": "main", + } + + t.Logf("Creating branch in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, createBranchRequest) + require.NoError(t, err, "expected to call 'create_branch' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Create a commit with a new file + commitRequest := mcp.CallToolRequest{} + commitRequest.Params.Name = "create_or_update_file" + commitRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-file.txt", + "content": fmt.Sprintf("Created by e2e test %s", t.Name()), + "message": "Add test file", + "branch": "test-branch", + } + + t.Logf("Creating commit with new file in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, commitRequest) + require.NoError(t, err, "expected to call 'create_or_update_file' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedCommitText struct { + SHA string `json:"sha"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedCommitText) + require.NoError(t, err, "expected to unmarshal text content successfully") + + // Check the file exists + getFileContentsRequest := mcp.CallToolRequest{} + getFileContentsRequest.Params.Name = "get_file_contents" + getFileContentsRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-file.txt", + "branch": "test-branch", + } + + t.Logf("Getting file contents in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, getFileContentsRequest) + require.NoError(t, err, "expected to call 'get_file_contents' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetFileText struct { + Content string `json:"content"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetFileText) + require.NoError(t, err, "expected to unmarshal text content successfully") + b, err := base64.StdEncoding.DecodeString(trimmedGetFileText.Content) + require.NoError(t, err, "expected to decode base64 content successfully") + require.Equal(t, fmt.Sprintf("Created by e2e test %s", t.Name()), string(b), "expected file content to match") + + // Delete the file + deleteFileRequest := mcp.CallToolRequest{} + deleteFileRequest.Params.Name = "delete_file" + deleteFileRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-file.txt", + "message": "Delete test file", + "branch": "test-branch", + } + + t.Logf("Deleting file in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, deleteFileRequest) + require.NoError(t, err, "expected to call 'delete_file' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // See that there is a commit that removes the file + listCommitsRequest := mcp.CallToolRequest{} + listCommitsRequest.Params.Name = "list_commits" + listCommitsRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "sha": "test-branch", // can be SHA or branch, which is an unfortunate API design + } + + t.Logf("Listing commits in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, listCommitsRequest) + require.NoError(t, err, "expected to call 'list_commits' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedListCommitsText []struct { + SHA string `json:"sha"` + Commit struct { + Message string `json:"message"` + } + Files []struct { + Filename string `json:"filename"` + Deletions int `json:"deletions"` + } + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedListCommitsText) + require.NoError(t, err, "expected to unmarshal text content successfully") + require.GreaterOrEqual(t, len(trimmedListCommitsText), 1, "expected to find at least one commit") + + deletionCommit := trimmedListCommitsText[0] + require.Equal(t, "Delete test file", deletionCommit.Commit.Message, "expected commit message to match") + + // Now get the commit so we can look at the file changes because list_commits doesn't include them + getCommitRequest := mcp.CallToolRequest{} + getCommitRequest.Params.Name = "get_commit" + getCommitRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "sha": deletionCommit.SHA, + } + + t.Logf("Getting commit %s/%s:%s...", currentOwner, repoName, deletionCommit.SHA) + resp, err = mcpClient.CallTool(ctx, getCommitRequest) + require.NoError(t, err, "expected to call 'get_commit' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetCommitText struct { + Files []struct { + Filename string `json:"filename"` + Deletions int `json:"deletions"` + } + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetCommitText) + require.NoError(t, err, "expected to unmarshal text content successfully") + require.Len(t, trimmedGetCommitText.Files, 1, "expected to find one file change") + require.Equal(t, "test-file.txt", trimmedGetCommitText.Files[0].Filename, "expected filename to match") + require.Equal(t, 1, trimmedGetCommitText.Files[0].Deletions, "expected one deletion") +} + +func TestDirectoryDeletion(t *testing.T) { + t.Parallel() + + mcpClient := setupMCPClient(t) + + ctx := context.Background() + + // First, who am I + getMeRequest := mcp.CallToolRequest{} + getMeRequest.Params.Name = "get_me" + + t.Log("Getting current user...") + resp, err := mcpClient.CallTool(ctx, getMeRequest) + require.NoError(t, err, "expected to call 'get_me' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + require.False(t, resp.IsError, "expected result not to be an error") + require.Len(t, resp.Content, 1, "expected content to have one item") + + textContent, ok := resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetMeText struct { + Login string `json:"login"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetMeText) + require.NoError(t, err, "expected to unmarshal text content successfully") + + currentOwner := trimmedGetMeText.Login + + // Then create a repository with a README (via autoInit) + repoName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli()) + createRepoRequest := mcp.CallToolRequest{} + createRepoRequest.Params.Name = "create_repository" + createRepoRequest.Params.Arguments = map[string]any{ + "name": repoName, + "private": true, + "autoInit": true, + } + t.Logf("Creating repository %s/%s...", currentOwner, repoName) + _, err = mcpClient.CallTool(ctx, createRepoRequest) + require.NoError(t, err, "expected to call 'get_me' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Cleanup the repository after the test + t.Cleanup(func() { + // MCP Server doesn't support deletions, but we can use the GitHub Client + ghClient := gogithub.NewClient(nil).WithAuthToken(getE2EToken(t)) + t.Logf("Deleting repository %s/%s...", currentOwner, repoName) + _, err := ghClient.Repositories.Delete(context.Background(), currentOwner, repoName) + require.NoError(t, err, "expected to delete repository successfully") + }) + + // Create a branch on which to create a new commit + createBranchRequest := mcp.CallToolRequest{} + createBranchRequest.Params.Name = "create_branch" + createBranchRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "branch": "test-branch", + "from_branch": "main", + } + + t.Logf("Creating branch in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, createBranchRequest) + require.NoError(t, err, "expected to call 'create_branch' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Create a commit with a new file + commitRequest := mcp.CallToolRequest{} + commitRequest.Params.Name = "create_or_update_file" + commitRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-dir/test-file.txt", + "content": fmt.Sprintf("Created by e2e test %s", t.Name()), + "message": "Add test file", + "branch": "test-branch", + } + + t.Logf("Creating commit with new file in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, commitRequest) + require.NoError(t, err, "expected to call 'create_or_update_file' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedCommitText struct { + SHA string `json:"sha"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedCommitText) + require.NoError(t, err, "expected to unmarshal text content successfully") + + // Check the file exists + getFileContentsRequest := mcp.CallToolRequest{} + getFileContentsRequest.Params.Name = "get_file_contents" + getFileContentsRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-dir/test-file.txt", + "branch": "test-branch", + } + + t.Logf("Getting file contents in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, getFileContentsRequest) + require.NoError(t, err, "expected to call 'get_file_contents' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetFileText struct { + Content string `json:"content"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetFileText) + require.NoError(t, err, "expected to unmarshal text content successfully") + b, err := base64.StdEncoding.DecodeString(trimmedGetFileText.Content) + require.NoError(t, err, "expected to decode base64 content successfully") + require.Equal(t, fmt.Sprintf("Created by e2e test %s", t.Name()), string(b), "expected file content to match") + + // Delete the directory containing the file + deleteFileRequest := mcp.CallToolRequest{} + deleteFileRequest.Params.Name = "delete_file" + deleteFileRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "path": "test-dir", + "message": "Delete test directory", + "branch": "test-branch", + } + + t.Logf("Deleting directory in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, deleteFileRequest) + require.NoError(t, err, "expected to call 'delete_file' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // See that there is a commit that removes the directory + listCommitsRequest := mcp.CallToolRequest{} + listCommitsRequest.Params.Name = "list_commits" + listCommitsRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "sha": "test-branch", // can be SHA or branch, which is an unfortunate API design + } + + t.Logf("Listing commits in %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, listCommitsRequest) + require.NoError(t, err, "expected to call 'list_commits' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedListCommitsText []struct { + SHA string `json:"sha"` + Commit struct { + Message string `json:"message"` + } + Files []struct { + Filename string `json:"filename"` + Deletions int `json:"deletions"` + } `json:"files"` + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedListCommitsText) + require.NoError(t, err, "expected to unmarshal text content successfully") + require.GreaterOrEqual(t, len(trimmedListCommitsText), 1, "expected to find at least one commit") + + deletionCommit := trimmedListCommitsText[0] + require.Equal(t, "Delete test directory", deletionCommit.Commit.Message, "expected commit message to match") + + // Now get the commit so we can look at the file changes because list_commits doesn't include them + getCommitRequest := mcp.CallToolRequest{} + getCommitRequest.Params.Name = "get_commit" + getCommitRequest.Params.Arguments = map[string]any{ + "owner": currentOwner, + "repo": repoName, + "sha": deletionCommit.SHA, + } + + t.Logf("Getting commit %s/%s:%s...", currentOwner, repoName, deletionCommit.SHA) + resp, err = mcpClient.CallTool(ctx, getCommitRequest) + require.NoError(t, err, "expected to call 'get_commit' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + + var trimmedGetCommitText struct { + Files []struct { + Filename string `json:"filename"` + Deletions int `json:"deletions"` + } + } + err = json.Unmarshal([]byte(textContent.Text), &trimmedGetCommitText) + require.NoError(t, err, "expected to unmarshal text content successfully") + require.Len(t, trimmedGetCommitText.Files, 1, "expected to find one file change") + require.Equal(t, "test-dir/test-file.txt", trimmedGetCommitText.Files[0].Filename, "expected filename to match") + require.Equal(t, 1, trimmedGetCommitText.Files[0].Deletions, "expected one deletion") +} diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 7f53aee6..795c4467 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -287,7 +287,7 @@ func CreateOrUpdateFile(getClient GetClientFn, t translations.TranslationHelperF return mcp.NewToolResultError(err.Error()), nil } - // Convert content to base64 + // json.Marshal encodes byte arrays with base64, which is required for the API. contentBytes := []byte(content) // Create the file options @@ -666,7 +666,7 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to // Create a response similar to what the DeleteFile API would return response := map[string]interface{}{ - "commit": newCommit, + "commit": newCommit, "content": nil, } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index b0a90c0f..6bb97da5 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1570,12 +1570,12 @@ func Test_DeleteFile(t *testing.T) { } tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]interface{} - expectError bool + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool expectedCommitSHA string - expectedErrMsg string + expectedErrMsg string }{ { name: "successful file deletion using Git Data API", @@ -1597,10 +1597,10 @@ func Test_DeleteFile(t *testing.T) { "base_tree": "def456", "tree": []interface{}{ map[string]interface{}{ - "path": "docs/example.md", - "mode": "100644", - "type": "blob", - "sha": nil, + "path": "docs/example.md", + "mode": "100644", + "type": "blob", + "sha": nil, }, }, }).andThen( @@ -1641,7 +1641,7 @@ func Test_DeleteFile(t *testing.T) { "message": "Delete example file", "branch": "main", }, - expectError: false, + expectError: false, expectedCommitSHA: "jkl012", }, { From 36833ff573541e6939802d323dc5fe512fe161f5 Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Mon, 12 May 2025 09:12:00 -0700 Subject: [PATCH 5/7] add comment --- pkg/github/repositories.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 795c4467..d3d51fac 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -557,6 +557,11 @@ func ForkRepository(getClient GetClientFn, t translations.TranslationHelperFunc) } // DeleteFile creates a tool to delete a file in a GitHub repository. +// This tool uses a more roundabout way of deleting a file than just using the client.Repositories.DeleteFile. +// This is because REST file deletion endpoint (and client.Repositories.DeleteFile) don't add commit signing to the deletion commit, +// unlike how the endpoint backing the create_or_update_files tool does. This appears to be a quirk of the API. +// The approach implemented here gets automatic commit signing when used with either the github-actions user or as an app, +// both of which suit an LLM well. func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("delete_file", mcp.WithDescription(t("TOOL_DELETE_FILE_DESCRIPTION", "Delete a file from a GitHub repository")), From 33d04023cc7a63f63b7f3361dabd5de819514bbd Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 13 May 2025 14:05:56 +0200 Subject: [PATCH 6/7] fix mcp-go bump breaking change --- pkg/github/repositories.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index d3d51fac..8740fc82 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -558,17 +558,17 @@ func ForkRepository(getClient GetClientFn, t translations.TranslationHelperFunc) // DeleteFile creates a tool to delete a file in a GitHub repository. // This tool uses a more roundabout way of deleting a file than just using the client.Repositories.DeleteFile. -// This is because REST file deletion endpoint (and client.Repositories.DeleteFile) don't add commit signing to the deletion commit, -// unlike how the endpoint backing the create_or_update_files tool does. This appears to be a quirk of the API. -// The approach implemented here gets automatic commit signing when used with either the github-actions user or as an app, +// This is because REST file deletion endpoint (and client.Repositories.DeleteFile) don't add commit signing to the deletion commit, +// unlike how the endpoint backing the create_or_update_files tool does. This appears to be a quirk of the API. +// The approach implemented here gets automatic commit signing when used with either the github-actions user or as an app, // both of which suit an LLM well. func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("delete_file", mcp.WithDescription(t("TOOL_DELETE_FILE_DESCRIPTION", "Delete a file from a GitHub repository")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_DELETE_FILE_USER_TITLE", "Delete file"), - ReadOnlyHint: false, - DestructiveHint: true, + ReadOnlyHint: toBoolPtr(false), + DestructiveHint: toBoolPtr(true), }), mcp.WithString("owner", mcp.Required(), From 8c5af0e555b0c650da4402e5891026fee339947e Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 13 May 2025 14:14:53 +0200 Subject: [PATCH 7/7] Add additional error handling on delete_file --- pkg/github/repositories.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8740fc82..4403e2a1 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -632,6 +632,14 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to } defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to get commit: %s", string(body))), nil + } + // Create a tree entry for the file deletion by setting SHA to nil treeEntries := []*github.TreeEntry{ { @@ -649,6 +657,14 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to } defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusCreated { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to create tree: %s", string(body))), nil + } + // Create a new commit with the new tree commit := &github.Commit{ Message: github.Ptr(message), @@ -661,6 +677,14 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to } defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusCreated { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to create commit: %s", string(body))), nil + } + // Update the branch reference to point to the new commit ref.Object.SHA = newCommit.SHA _, resp, err = client.Git.UpdateRef(ctx, owner, repo, ref, false) @@ -669,6 +693,14 @@ func DeleteFile(getClient GetClientFn, t translations.TranslationHelperFunc) (to } defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to update reference: %s", string(body))), nil + } + // Create a response similar to what the DeleteFile API would return response := map[string]interface{}{ "commit": newCommit,