Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: add DeleteFile tool to delete files from GitHub repositories#356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
williammartin merged 7 commits intogithub:mainfromashwin-ant:ashwin/deletefile
May 13, 2025

Conversation

ashwin-ant
Copy link
Contributor

This adds a tool that matches the behavior ofthis API endpoint. This is needed since thecreate_or_update_file tool doesn't allow deletion.

@ashwin-antashwin-ant requested a review froma team as acode ownerApril 28, 2025 18:17
@@ -528,6 +528,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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hey, can you look at adding annotations? You can see examples around the codebase?

For this particular one, as well as the tool title with translation I would add:

ReadOnlyHint: falseDestructiveHint: true

https://modelcontextprotocol.io/docs/concepts/tools#tool-definition-structure

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@SamMorrowDrums done! Thanks for the suggestion.

@ashwin-antashwin-antforce-pushed theashwin/deletefile branch 2 times, most recently fromabe06cd toe6201f4CompareMay 1, 2025 23:18
Copy link
Collaborator

@williammartinwilliammartin left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ashwin-ant thanks for this PR. I'm a bit confused about the implementation though. It seems like you:

  • Get a github ref for the current branch
  • Get the commit for that ref
  • Create a new TreeEntry without the file
  • Create a new commit
  • Update the ref

Why not:

  • Get a github ref for the current branch
  • Get the commit for that ref
  • Get the SHA of the filepath in that commit
  • Callclient.Repositories.DeleteFile which exactly calls the API endpoint you referenced.

Edit: Maybe the TreeEntry approach allows for deletion of a directory where the contents API doesn't?

@williammartin
Copy link
Collaborator

williammartin commentedMay 12, 2025•
edited
Loading

@ashwin-ant I just rebased this and added some e2e tests that we can consider refactoring behind (and perhaps, demonstrating that we can't do directory removal with the contents API). You can run them like:

 GOMAXPROCS=1 GITHUB_MCP_SERVER_E2E_TOKEN=<YOUR TOKEN> go test -v -count=1 -run 'Test.*Deletion' --tags e2e ./e2e

Make sure your token hasdelete_repo scope to avoid debris.

I'll probably collapse them to one test later, but for the moment while we figure this out it makes sense to keep them separate.

@ashwin-ant
Copy link
ContributorAuthor

@williammartin Thanks for reviewing! I actually was going with that approach in myfirst commit here, but ended up switching to this more complicated approach in4d5f719.

The reason for this is that I discovered the REST file deletion endpoint (andclient.Repositories.DeleteFile) don't add commit signing to the deletion commit, unlike how the endpoint backing thecreate_or_update_files tool does. This appears to be a quirk of the API. The approach here will get commit signing when used with either the github-actions user or as an app, both of which suit an LLM well.

@williammartin
Copy link
Collaborator

That's a great explanation and I'd love it if you put that as a comment in the code for all future reviewers!

williammartin
williammartin previously approved these changesMay 13, 2025
Copy link
Collaborator

@williammartinwilliammartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

➜  github-mcp-server git:(ashwin/deletefile) ✗ GOMAXPROCS=1 GITHUB_MCP_SERVER_E2E_TOKEN=$(gh auth token) go test -v -count=1 -run TestFileDeletion --tags e2e ./e2e=== RUN   TestFileDeletion=== PAUSE TestFileDeletion=== CONT  TestFileDeletion    e2e_test.go:50: Building Docker image for e2e tests...    e2e_test.go:124: Starting Stdio MCP client...    e2e_test.go:385: Getting current user...    e2e_test.go:413: Creating repository williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:437: Creating branch in williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:454: Creating commit with new file in williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:478: Getting file contents in williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:506: Deleting file in williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:520: Listing commits in williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...    e2e_test.go:554: Getting commit williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412:76f50ad460c472d7eb9df09bd1c94d5678a99b96...    e2e_test.go:422: Deleting repository williammartin/github-mcp-server-e2e-TestFileDeletion-1747137948412...--- PASS: TestFileDeletion (6.65s)PASSok      github.com/github/github-mcp-server/e2e 6.894s
imageimage

LGTM, thanks.

@williammartin
Copy link
Collaborator

williammartin commentedMay 13, 2025•
edited
Loading

Bypassing the last pusher rule because I fixed minor breaking changes from mcp-go bump onmain around annotations and some small error handling additions.

Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull Request Overview

This PR introduces a newdelete_file tool that leverages the Git Data API to delete files with commit signing, and adds accompanying tests.

  • RegistersDeleteFile in the GitHub toolset initialization.
  • ImplementsDeleteFile inrepositories.go, creating a deletion commit via trees and references.
  • AddsTest_DeleteFile inrepositories_test.go for success and branch-not-found scenarios.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

FileDescription
pkg/github/tools.goRegister the newDeleteFile tool in server toolsets.
pkg/github/repositories.goImplementDeleteFile using Git Data API calls.
pkg/github/repositories_test.goAdd unit tests for theDeleteFile tool.
Comments suppressed due to low confidence (1)

pkg/github/repositories_test.go:1532

  • The test covers success and branch-not-found paths; consider adding cases for failures in tree creation, commit creation, and reference updates to ensure robust error handling across all stages of the deletion flow.
func Test_DeleteFile(t *testing.T) {

@williammartinwilliammartin merged commitda2df71 intogithub:mainMay 13, 2025
9 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@SamMorrowDrumsSamMorrowDrumsSamMorrowDrums left review comments

Copilot code reviewCopilotCopilot left review comments

@williammartinwilliammartinwilliammartin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ashwin-ant@williammartin@SamMorrowDrums
[8]ページ先頭
©2009-2025 Movatter.jp