Skip to content

Commit 27178da

Browse files
author
root
committed
fix: retry push_files on concurrent branch updates
push_files used a read-modify-write sequence without retrying when UpdateRef failed with a non-fast-forward conflict. During rapid multi-file pushes this left orphaned commits and intermittent failures. Retry with the latest ref and accept flexible files argument encodings from MCP hosts. Closes #2481
1 parent 33849e9 commit 27178da

3 files changed

Lines changed: 436 additions & 136 deletions

File tree

pkg/github/repositories.go

Lines changed: 38 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -1365,160 +1365,62 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool {
13651365
return utils.NewToolResultError(err.Error()), nil, nil
13661366
}
13671367

1368-
// Parse files parameter - this should be an array of objects with path and content
1369-
filesObj, ok := args["files"].([]any)
1370-
if !ok {
1371-
return utils.NewToolResultError("files parameter must be an array of objects with path and content"), nil, nil
1368+
// Parse files parameter - accept several encodings from different MCP hosts.
1369+
fileEntries, err := parsePushFilesEntries(args["files"])
1370+
if err != nil {
1371+
return utils.NewToolResultError(err.Error()), nil, nil
13721372
}
13731373

13741374
client, err := deps.GetClient(ctx)
13751375
if err != nil {
13761376
return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
13771377
}
13781378

1379-
// Get the reference for the branch
1380-
var repositoryIsEmpty bool
1381-
var branchNotFound bool
1382-
ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch)
1379+
refName, err := ensurePushFilesBranchRef(ctx, client, owner, repo, branch)
13831380
if err != nil {
1384-
ghErr, isGhErr := err.(*github.ErrorResponse)
1385-
if isGhErr {
1386-
if ghErr.Response.StatusCode == http.StatusConflict && ghErr.Message == "Git Repository is empty." {
1387-
repositoryIsEmpty = true
1388-
} else if ghErr.Response.StatusCode == http.StatusNotFound {
1389-
branchNotFound = true
1390-
}
1391-
}
1392-
1393-
if !repositoryIsEmpty && !branchNotFound {
1381+
errMsg := err.Error()
1382+
switch {
1383+
case strings.Contains(errMsg, "initialize repository"):
1384+
return utils.NewToolResultError(fmt.Sprintf("failed to initialize repository: %v", err)), nil, nil
1385+
case strings.Contains(errMsg, "create branch from default"):
1386+
return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil
1387+
default:
13941388
return ghErrors.NewGitHubAPIErrorResponse(ctx,
13951389
"failed to get branch reference",
1396-
resp,
1397-
err,
1398-
), nil, nil
1399-
}
1400-
}
1401-
// Only close resp if it's not nil and not an error case where resp might be nil
1402-
if resp != nil && resp.Body != nil {
1403-
defer func() { _ = resp.Body.Close() }()
1404-
}
1405-
1406-
var baseCommit *github.Commit
1407-
if !repositoryIsEmpty {
1408-
if branchNotFound {
1409-
ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch)
1410-
if err != nil {
1411-
return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil
1412-
}
1413-
}
1414-
1415-
// Get the commit object that the branch points to
1416-
baseCommit, resp, err = client.Git.GetCommit(ctx, owner, repo, *ref.Object.SHA)
1417-
if err != nil {
1418-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1419-
"failed to get base commit",
1420-
resp,
1390+
nil,
14211391
err,
14221392
), nil, nil
14231393
}
1424-
if resp != nil && resp.Body != nil {
1425-
defer func() { _ = resp.Body.Close() }()
1426-
}
1427-
} else {
1428-
var base *github.Commit
1429-
// Repository is empty, need to initialize it first
1430-
ref, base, err = initializeRepository(ctx, client, owner, repo)
1431-
if err != nil {
1432-
return utils.NewToolResultError(fmt.Sprintf("failed to initialize repository: %v", err)), nil, nil
1433-
}
1434-
1435-
defaultBranch := strings.TrimPrefix(*ref.Ref, "refs/heads/")
1436-
if branch != defaultBranch {
1437-
// Create the requested branch from the default branch
1438-
ref, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch)
1439-
if err != nil {
1440-
return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil
1441-
}
1442-
}
1443-
1444-
baseCommit = base
14451394
}
14461395

1447-
// Create tree entries for all files (or remaining files if empty repo)
1448-
var entries []*github.TreeEntry
1449-
1450-
for _, file := range filesObj {
1451-
fileMap, ok := file.(map[string]any)
1452-
if !ok {
1453-
return utils.NewToolResultError("each file must be an object with path and content"), nil, nil
1454-
}
1455-
1456-
path, ok := fileMap["path"].(string)
1457-
if !ok || path == "" {
1458-
return utils.NewToolResultError("each file must have a path"), nil, nil
1459-
}
1460-
1461-
content, ok := fileMap["content"].(string)
1462-
if !ok {
1463-
return utils.NewToolResultError("each file must have content"), nil, nil
1464-
}
1465-
1466-
// Create a tree entry for the file
1467-
entries = append(entries, &github.TreeEntry{
1468-
Path: github.Ptr(path),
1469-
Mode: github.Ptr("100644"), // Regular file mode
1470-
Type: github.Ptr("blob"),
1471-
Content: github.Ptr(content),
1472-
})
1473-
}
1474-
1475-
// Create a new tree with the file entries (baseCommit is now guaranteed to exist)
1476-
newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries)
1477-
if err != nil {
1478-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1479-
"failed to create tree",
1480-
resp,
1481-
err,
1482-
), nil, nil
1483-
}
1484-
if resp != nil && resp.Body != nil {
1485-
defer func() { _ = resp.Body.Close() }()
1486-
}
1487-
1488-
// Create a new commit (baseCommit always has a value now)
1489-
commit := github.Commit{
1490-
Message: github.Ptr(message),
1491-
Tree: newTree,
1492-
Parents: []*github.Commit{{SHA: baseCommit.SHA}},
1493-
}
1494-
newCommit, resp, err := client.Git.CreateCommit(ctx, owner, repo, commit, nil)
1495-
if err != nil {
1496-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1497-
"failed to create commit",
1498-
resp,
1499-
err,
1500-
), nil, nil
1501-
}
1502-
if resp != nil && resp.Body != nil {
1503-
defer func() { _ = resp.Body.Close() }()
1504-
}
1505-
1506-
// Update the reference to point to the new commit
1507-
ref.Object.SHA = newCommit.SHA
1508-
updatedRef, resp, err := client.Git.UpdateRef(ctx, owner, repo, *ref.Ref, github.UpdateRef{
1509-
SHA: *newCommit.SHA,
1510-
Force: github.Ptr(false),
1511-
})
1396+
pushResult, resp, err := commitEntriesToRef(
1397+
ctx,
1398+
client,
1399+
owner,
1400+
repo,
1401+
refName,
1402+
message,
1403+
pushFileEntriesToTreeEntries(fileEntries),
1404+
)
15121405
if err != nil {
1513-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
1514-
"failed to update reference",
1515-
resp,
1516-
err,
1517-
), nil, nil
1406+
stage := "failed to push files"
1407+
errMsg := err.Error()
1408+
switch {
1409+
case strings.Contains(errMsg, "get base commit"):
1410+
stage = "failed to get base commit"
1411+
case strings.Contains(errMsg, "create tree"):
1412+
stage = "failed to create tree"
1413+
case strings.Contains(errMsg, "create commit"):
1414+
stage = "failed to create commit"
1415+
case strings.Contains(errMsg, "update reference"):
1416+
stage = "failed to update reference"
1417+
case strings.Contains(errMsg, "get branch reference"):
1418+
stage = "failed to get branch reference"
1419+
}
1420+
return ghErrors.NewGitHubAPIErrorResponse(ctx, stage, resp, err), nil, nil
15181421
}
1519-
defer func() { _ = resp.Body.Close() }()
15201422

1521-
r, err := json.Marshal(updatedRef)
1423+
r, err := json.Marshal(pushResult.Ref)
15221424
if err != nil {
15231425
return nil, nil, fmt.Errorf("failed to marshal response: %w", err)
15241426
}

0 commit comments

Comments
 (0)