Improvements to push_files tool #1676
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the push_files tool to automatically handle two common failure scenarios: empty repositories and non-existent branches. Instead of failing when encountering these conditions, the tool now initializes empty repositories with an initial commit and creates missing branches from the default branch.
Key changes:
- Added auto-initialization of empty repositories when a 409 Conflict status is detected
- Added auto-creation of branches from the default branch when a branch is not found
- Improved error handling and resource cleanup with nil checks before closing response bodies
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/github/repositories.go | Implements auto-init logic for empty repositories and auto-creation of non-existent branches; adds two helper functions initializeRepository and createReferenceFromDefaultBranch |
| pkg/github/repositories_test.go | Adds comprehensive test coverage for empty repository scenarios, branch auto-creation failures, and updates existing test expectations to match new behavior |
| ghErr, isGhErr := err.(*github.ErrorResponse) | ||
| if isGhErr { | ||
| if ghErr.Response.StatusCode == http.StatusConflict && ghErr.Message == "Git Repository is empty." { | ||
| repositoryIsEmpty = true | ||
| } else if ghErr.Response.StatusCode == http.StatusNotFound { | ||
| branchNotFound = true | ||
| } | ||
| } else { | ||
| return ghErrors.NewGitHubAPIErrorResponse(ctx, | ||
| "failed to get branch reference", | ||
| resp, | ||
| err, | ||
| ), nil, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling logic has critical issues: (1) It doesn't check if ghErr.Response is nil before accessing ghErr.Response.StatusCode, which could cause a nil pointer dereference panic. (2) When the error is a GitHub ErrorResponse but neither a 409 Conflict nor 404 Not Found (e.g., 500 Internal Server Error, 403 Forbidden), the function silently continues execution with both repositoryIsEmpty and branchNotFound as false, then attempts to access ref.Object.SHA on line 1316 where ref is nil, causing a panic.
| if branch != (*defaultRef.Ref)[len("refs/heads/"):] { | ||
| // Create the requested branch from the default branch | ||
| ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) | ||
| if err != nil { | ||
| return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil | ||
| } | ||
| } else { |
There was a problem hiding this comment.
The string slicing operation (*defaultRef.Ref)[len("refs/heads/"):] can panic if defaultRef.Ref is shorter than "refs/heads/" or has an unexpected format. This should validate the ref format before slicing or use a safer method to extract the branch name.
| if branch != (*defaultRef.Ref)[len("refs/heads/"):] { | |
| // Create the requested branch from the default branch | |
| ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) | |
| if err != nil { | |
| return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil | |
| } | |
| } else { | |
| // Safely extract the default branch name from the ref, if it has the expected format. | |
| const headsPrefix = "refs/heads/" | |
| defaultBranchName := "" | |
| if defaultRef != nil && defaultRef.Ref != nil && | |
| strings.HasPrefix(*defaultRef.Ref, headsPrefix) && | |
| len(*defaultRef.Ref) > len(headsPrefix) { | |
| defaultBranchName = (*defaultRef.Ref)[len(headsPrefix):] | |
| } | |
| if defaultBranchName != "" && branch != defaultBranchName { | |
| // Create the requested branch from the default branch | |
| ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) | |
| if err != nil { | |
| return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil | |
| } | |
| } else { | |
| // Either this is the default branch or the ref format was unexpected; use the default ref. |
|
|
||
| // Create the new branch reference | ||
| createdRef, resp, err := client.Git.CreateRef(ctx, owner, repo, github.CreateRef{ | ||
| Ref: *github.Ptr("refs/heads/" + branch), |
There was a problem hiding this comment.
The code unnecessarily wraps the string in github.Ptr and then immediately dereferences it with *. Looking at line 1176 in the same file, the Ref field of CreateRef expects a plain string, not a pointer. This should be: Ref: "refs/heads/" + branch
| Ref: *github.Ptr("refs/heads/" + branch), | |
| Ref: "refs/heads/" + branch, |
| // Create an initial empty commit to create the default branch | ||
| createResp, resp, err := client.Repositories.CreateFile(ctx, owner, repo, "README.md", fileOpts) |
There was a problem hiding this comment.
The function creates an empty README.md file to initialize the repository but doesn't communicate this to the caller. This means if a user is trying to push files including a README.md with specific content, the first file will create an empty README.md, and then the user's README.md content will overwrite it. While this works, it's inefficient and creates an extra commit in the history. Consider either: (1) checking if README.md is in the files being pushed and using that content for initialization, or (2) documenting this behavior clearly.
| // Create an initial empty commit to create the default branch | |
| createResp, resp, err := client.Repositories.CreateFile(ctx, owner, repo, "README.md", fileOpts) | |
| // Create an initial empty commit to create the default branch. | |
| // Use a non-README file so that user-provided README.md content does not | |
| // get an extra empty commit in its history. | |
| createResp, resp, err := client.Repositories.CreateFile(ctx, owner, repo, ".gitkeep", fileOpts) |
| defer func() { _ = resp.Body.Close() }() | ||
| } | ||
|
|
||
| // Update ref to point to the new commit |
There was a problem hiding this comment.
The comment says "Update ref to point to the new commit" but the code is actually just retrieving the ref, not updating it. The ref was already created by the CreateFile operation on line 1451. Consider updating the comment to: "Get the reference that was created by the initial file commit".
| // Update ref to point to the new commit | |
| // Get the reference that was created by the initial file commit |
Summary
This PR aims to significantly reduce failure rate for push_files tool.
Why
push_files tool fails too often due to rigid assumptions that both ref that files are pushed to exists and repo is not empty. However, it is very often not the case and we shouldn't fail on such common cases.
MCP impact
Prompts tested (tool changes only)
Security / limits
Lint & tests
./script/lint./script/testDocs