From 1983ac7d0ed3805edacacc2dbcd76eafa18185eb Mon Sep 17 00:00:00 2001 From: root Date: Sat, 6 Jun 2026 17:06:51 +0000 Subject: [PATCH 1/3] 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 --- pkg/github/repositories.go | 174 +++++---------------- pkg/github/repositories_helper.go | 208 +++++++++++++++++++++++++ pkg/github/repositories_helper_test.go | 190 ++++++++++++++++++++++ 3 files changed, 436 insertions(+), 136 deletions(-) create mode 100644 pkg/github/repositories_helper_test.go diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 040a968cf9..93f7226b46 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1365,10 +1365,10 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(err.Error()), nil, nil } - // Parse files parameter - this should be an array of objects with path and content - filesObj, ok := args["files"].([]any) - if !ok { - return utils.NewToolResultError("files parameter must be an array of objects with path and content"), nil, nil + // Parse files parameter - accept several encodings from different MCP hosts. + fileEntries, err := parsePushFilesEntries(args["files"]) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetClient(ctx) @@ -1376,149 +1376,51 @@ func PushFiles(t translations.TranslationHelperFunc) inventory.ServerTool { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - // Get the reference for the branch - var repositoryIsEmpty bool - var branchNotFound bool - ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch) + refName, err := ensurePushFilesBranchRef(ctx, client, owner, repo, branch) if err != nil { - 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 - } - } - - if !repositoryIsEmpty && !branchNotFound { + errMsg := err.Error() + switch { + case strings.Contains(errMsg, "initialize repository"): + return utils.NewToolResultError(fmt.Sprintf("failed to initialize repository: %v", err)), nil, nil + case strings.Contains(errMsg, "create branch from default"): + return utils.NewToolResultError(fmt.Sprintf("failed to create branch from default: %v", err)), nil, nil + default: return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get branch reference", - resp, - err, - ), nil, nil - } - } - // Only close resp if it's not nil and not an error case where resp might be nil - if resp != nil && resp.Body != nil { - defer func() { _ = resp.Body.Close() }() - } - - var baseCommit *github.Commit - if !repositoryIsEmpty { - if branchNotFound { - 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 - } - } - - // 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 ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to get base commit", - resp, + nil, err, ), nil, nil } - if resp != nil && resp.Body != nil { - defer func() { _ = resp.Body.Close() }() - } - } else { - var base *github.Commit - // Repository is empty, need to initialize it first - ref, base, err = initializeRepository(ctx, client, owner, repo) - if err != nil { - return utils.NewToolResultError(fmt.Sprintf("failed to initialize repository: %v", err)), nil, nil - } - - defaultBranch := strings.TrimPrefix(*ref.Ref, "refs/heads/") - if branch != defaultBranch { - // 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 - } - } - - baseCommit = base } - // Create tree entries for all files (or remaining files if empty repo) - var entries []*github.TreeEntry - - for _, file := range filesObj { - fileMap, ok := file.(map[string]any) - if !ok { - return utils.NewToolResultError("each file must be an object with path and content"), nil, nil - } - - path, ok := fileMap["path"].(string) - if !ok || path == "" { - return utils.NewToolResultError("each file must have a path"), nil, nil - } - - content, ok := fileMap["content"].(string) - if !ok { - return utils.NewToolResultError("each file must have content"), nil, nil - } - - // Create a tree entry for the file - entries = append(entries, &github.TreeEntry{ - Path: github.Ptr(path), - Mode: github.Ptr("100644"), // Regular file mode - Type: github.Ptr("blob"), - Content: github.Ptr(content), - }) - } - - // Create a new tree with the file entries (baseCommit is now guaranteed to exist) - newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, entries) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to create tree", - resp, - err, - ), nil, nil - } - if resp != nil && resp.Body != nil { - defer func() { _ = resp.Body.Close() }() - } - - // Create a new commit (baseCommit always has a value now) - 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 ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to create commit", - resp, - err, - ), nil, nil - } - if resp != nil && resp.Body != nil { - defer func() { _ = resp.Body.Close() }() - } - - // Update the reference to point to the new commit - ref.Object.SHA = newCommit.SHA - updatedRef, resp, err := client.Git.UpdateRef(ctx, owner, repo, *ref.Ref, github.UpdateRef{ - SHA: *newCommit.SHA, - Force: github.Ptr(false), - }) + pushResult, resp, err := commitEntriesToRef( + ctx, + client, + owner, + repo, + refName, + message, + pushFileEntriesToTreeEntries(fileEntries), + ) if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to update reference", - resp, - err, - ), nil, nil + stage := "failed to push files" + errMsg := err.Error() + switch { + case strings.Contains(errMsg, "get base commit"): + stage = "failed to get base commit" + case strings.Contains(errMsg, "create tree"): + stage = "failed to create tree" + case strings.Contains(errMsg, "create commit"): + stage = "failed to create commit" + case strings.Contains(errMsg, "update reference"): + stage = "failed to update reference" + case strings.Contains(errMsg, "get branch reference"): + stage = "failed to get branch reference" + } + return ghErrors.NewGitHubAPIErrorResponse(ctx, stage, resp, err), nil, nil } - defer func() { _ = resp.Body.Close() }() - r, err := json.Marshal(updatedRef) + r, err := json.Marshal(pushResult.Ref) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) } diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index be377f773e..86ded80951 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -327,3 +327,211 @@ func resolveDefaultBranch(ctx context.Context, githubClient *github.Client, owne return defaultRef, nil } + +const maxGitRefUpdateRetries = 5 + +type pushFileEntry struct { + Path string + Content string +} + +// parsePushFilesEntries normalizes the push_files "files" argument from MCP clients. +// Some hosts send []any, typed slices, or a JSON-encoded string; all are accepted. +func parsePushFilesEntries(raw any) ([]pushFileEntry, error) { + if raw == nil { + return nil, fmt.Errorf("files parameter must be an array of objects with path and content") + } + + var items []any + switch v := raw.(type) { + case []any: + items = v + case string: + if err := json.Unmarshal([]byte(v), &items); err != nil { + return nil, fmt.Errorf("files parameter must be an array of objects with path and content") + } + case json.RawMessage: + if err := json.Unmarshal(v, &items); err != nil { + return nil, fmt.Errorf("files parameter must be an array of objects with path and content") + } + default: + encoded, err := json.Marshal(raw) + if err != nil { + return nil, fmt.Errorf("files parameter must be an array of objects with path and content") + } + if err := json.Unmarshal(encoded, &items); err != nil { + return nil, fmt.Errorf("files parameter must be an array of objects with path and content") + } + } + + entries := make([]pushFileEntry, 0, len(items)) + for _, item := range items { + fileMap, ok := item.(map[string]any) + if !ok { + return nil, fmt.Errorf("each file must be an object with path and content") + } + + path, ok := fileMap["path"].(string) + if !ok || path == "" { + return nil, fmt.Errorf("each file must have a path") + } + + content, ok := fileMap["content"].(string) + if !ok { + return nil, fmt.Errorf("each file must have content") + } + + entries = append(entries, pushFileEntry{Path: path, Content: content}) + } + + return entries, nil +} + +func pushFileEntriesToTreeEntries(entries []pushFileEntry) []*github.TreeEntry { + treeEntries := make([]*github.TreeEntry, 0, len(entries)) + for _, entry := range entries { + treeEntries = append(treeEntries, &github.TreeEntry{ + Path: github.Ptr(entry.Path), + Mode: github.Ptr("100644"), + Type: github.Ptr("blob"), + Content: github.Ptr(entry.Content), + }) + } + return treeEntries +} + +func closeGitHubResponse(resp *github.Response) { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } +} + +func isGitRefUpdateConflict(err error, resp *github.Response) bool { + statusCode := 0 + if resp != nil && resp.Response != nil { + statusCode = resp.StatusCode + } + if ghErr, ok := err.(*github.ErrorResponse); ok { + if ghErr.Response != nil { + statusCode = ghErr.Response.StatusCode + } + msg := strings.ToLower(ghErr.Message) + if strings.Contains(msg, "fast forward") || strings.Contains(msg, "not a fast forward") { + return true + } + } + return statusCode == http.StatusUnprocessableEntity +} + +type gitPushResult struct { + Ref *github.Reference + Commit *github.Commit +} + +// ensurePushFilesBranchRef ensures the target branch exists for push_files. +// Empty repositories are initialized and missing branches are created from the default branch. +func ensurePushFilesBranchRef(ctx context.Context, client *github.Client, owner, repo, branch string) (string, error) { + refName := "refs/heads/" + branch + ref, resp, err := client.Git.GetRef(ctx, owner, repo, refName) + if err == nil { + closeGitHubResponse(resp) + return refName, nil + } + + var repositoryIsEmpty bool + var branchNotFound bool + if ghErr, isGhErr := err.(*github.ErrorResponse); isGhErr { + if ghErr.Response.StatusCode == http.StatusConflict && ghErr.Message == "Git Repository is empty." { + repositoryIsEmpty = true + } else if ghErr.Response.StatusCode == http.StatusNotFound { + branchNotFound = true + } + } + + closeGitHubResponse(resp) + + if !repositoryIsEmpty && !branchNotFound { + return "", fmt.Errorf("failed to get branch reference: %w", err) + } + + if repositoryIsEmpty { + ref, _, err = initializeRepository(ctx, client, owner, repo) + if err != nil { + return "", fmt.Errorf("failed to initialize repository: %w", err) + } + + defaultBranch := strings.TrimPrefix(ref.GetRef(), "refs/heads/") + if branch != defaultBranch { + _, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) + if err != nil { + return "", fmt.Errorf("failed to create branch from default: %w", err) + } + } + return refName, nil + } + + _, err = createReferenceFromDefaultBranch(ctx, client, owner, repo, branch) + if err != nil { + return "", fmt.Errorf("failed to create branch from default: %w", err) + } + + return refName, nil +} + +// commitEntriesToRef creates a commit from tree entries and updates the branch ref. +// When another push updates the branch concurrently, the operation is retried with the latest ref. +func commitEntriesToRef( + ctx context.Context, + client *github.Client, + owner, repo, refName, message string, + entries []*github.TreeEntry, +) (*gitPushResult, *github.Response, error) { + for attempt := 0; attempt < maxGitRefUpdateRetries; attempt++ { + ref, resp, err := client.Git.GetRef(ctx, owner, repo, refName) + if err != nil { + return nil, resp, fmt.Errorf("failed to get branch reference: %w", err) + } + closeGitHubResponse(resp) + + baseCommit, resp, err := client.Git.GetCommit(ctx, owner, repo, ref.GetObject().GetSHA()) + if err != nil { + return nil, resp, fmt.Errorf("failed to get base commit: %w", err) + } + closeGitHubResponse(resp) + + newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, baseCommit.GetTree().GetSHA(), entries) + if err != nil { + return nil, resp, fmt.Errorf("failed to create tree: %w", err) + } + closeGitHubResponse(resp) + + 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, resp, fmt.Errorf("failed to create commit: %w", err) + } + closeGitHubResponse(resp) + + updatedRef, resp, err := client.Git.UpdateRef(ctx, owner, repo, refName, github.UpdateRef{ + SHA: newCommit.GetSHA(), + Force: github.Ptr(false), + }) + if err == nil && (resp == nil || resp.StatusCode == http.StatusOK) { + closeGitHubResponse(resp) + return &gitPushResult{Ref: updatedRef, Commit: newCommit}, nil, nil + } + + if isGitRefUpdateConflict(err, resp) && attempt < maxGitRefUpdateRetries-1 { + closeGitHubResponse(resp) + continue + } + + return nil, resp, fmt.Errorf("failed to update reference: %w", err) + } + + return nil, nil, fmt.Errorf("failed to update reference after %d attempts due to concurrent branch updates", maxGitRefUpdateRetries) +} diff --git a/pkg/github/repositories_helper_test.go b/pkg/github/repositories_helper_test.go new file mode 100644 index 0000000000..31857294e1 --- /dev/null +++ b/pkg/github/repositories_helper_test.go @@ -0,0 +1,190 @@ +package github + +import ( + "encoding/json" + "net/http" + "testing" + + "github.com/google/go-github/v87/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_parsePushFilesEntries(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw any + want []pushFileEntry + wantErr string + }{ + { + name: "array of maps", + raw: []any{ + map[string]any{"path": "README.md", "content": "# Hello"}, + }, + want: []pushFileEntry{{Path: "README.md", Content: "# Hello"}}, + }, + { + name: "typed slice via json round trip", + raw: []map[string]any{ + {"path": "docs/a.md", "content": "alpha"}, + {"path": "docs/b.md", "content": "beta"}, + }, + want: []pushFileEntry{ + {Path: "docs/a.md", Content: "alpha"}, + {Path: "docs/b.md", Content: "beta"}, + }, + }, + { + name: "json string payload", + raw: `[{"path":"README.md","content":"from string"}]`, + want: []pushFileEntry{{Path: "README.md", Content: "from string"}}, + }, + { + name: "invalid string payload", + raw: "not-json", + wantErr: "files parameter must be an array", + }, + { + name: "missing path", + raw: []any{map[string]any{"content": "x"}}, + wantErr: "each file must have a path", + }, + { + name: "missing content", + raw: []any{map[string]any{"path": "a.txt"}}, + wantErr: "each file must have content", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := parsePushFilesEntries(tc.raw) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func Test_isGitRefUpdateConflict(t *testing.T) { + t.Parallel() + + resp422 := &github.Response{Response: &http.Response{StatusCode: http.StatusUnprocessableEntity}} + assert.True(t, isGitRefUpdateConflict(&github.ErrorResponse{ + Response: resp422.Response, + Message: "Update is not a fast forward", + }, resp422)) + + resp200 := &github.Response{Response: &http.Response{StatusCode: http.StatusOK}} + assert.False(t, isGitRefUpdateConflict(nil, resp200)) +} + +func Test_commitEntriesToRef_retriesOnFastForwardConflict(t *testing.T) { + mockRefInitial := &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("abc123"), + }, + } + mockRefUpdated := &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("concurrent999"), + }, + } + mockCommitInitial := &github.Commit{ + SHA: github.Ptr("abc123"), + Tree: &github.Tree{SHA: github.Ptr("def456")}, + } + mockCommitUpdated := &github.Commit{ + SHA: github.Ptr("concurrent999"), + Tree: &github.Tree{SHA: github.Ptr("tree999")}, + } + mockTree := &github.Tree{SHA: github.Ptr("ghi789")} + mockNewCommit := &github.Commit{SHA: github.Ptr("jkl012")} + mockUpdatedRef := &github.Reference{ + Ref: github.Ptr("refs/heads/main"), + Object: &github.GitObject{ + SHA: github.Ptr("jkl012"), + }, + } + + getRefCalls := 0 + updateRefCalls := 0 + + client := mustNewGHClient(t, NewMockedHTTPClient( + WithRequestMatchHandler( + GetReposGitRefByOwnerByRepoByRef, + func(w http.ResponseWriter, _ *http.Request) { + getRefCalls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + ref := mockRefInitial + if getRefCalls >= 3 { + ref = mockRefUpdated + } + require.NoError(t, json.NewEncoder(w).Encode(ref)) + }, + ), + WithRequestMatchHandler( + GetReposGitCommitsByOwnerByRepoByCommitSHA, + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + commit := mockCommitInitial + if r.URL.Path == "/repos/owner/repo/git/commits/concurrent999" { + commit = mockCommitUpdated + } + require.NoError(t, json.NewEncoder(w).Encode(commit)) + }, + ), + WithRequestMatchHandler( + PostReposGitTreesByOwnerByRepo, + mockResponse(t, http.StatusCreated, mockTree), + ), + WithRequestMatchHandler( + PostReposGitCommitsByOwnerByRepo, + mockResponse(t, http.StatusCreated, mockNewCommit), + ), + WithRequestMatchHandler( + PatchReposGitRefsByOwnerByRepoByRef, + func(w http.ResponseWriter, _ *http.Request) { + updateRefCalls++ + w.Header().Set("Content-Type", "application/json") + if updateRefCalls == 1 { + w.WriteHeader(http.StatusUnprocessableEntity) + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "message": "Update is not a fast forward", + })) + return + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(mockUpdatedRef)) + }, + ), + )) + + result, resp, err := commitEntriesToRef( + t.Context(), + client, + "owner", + "repo", + "refs/heads/main", + "Update files", + pushFileEntriesToTreeEntries([]pushFileEntry{{Path: "README.md", Content: "# Hi"}}), + ) + require.NoError(t, err) + require.Nil(t, resp) + require.NotNil(t, result) + assert.Equal(t, "jkl012", result.Ref.GetObject().GetSHA()) + assert.Equal(t, 2, updateRefCalls) + assert.Equal(t, 2, getRefCalls) +} From c15cbe90b232eac84c86961977821f988a1f54ad Mon Sep 17 00:00:00 2001 From: root Date: Sat, 6 Jun 2026 22:08:35 +0000 Subject: [PATCH 2/3] fix: apply ref-update retry to delete_file Reuse commitEntriesToRef for delete_file so concurrent branch updates retry instead of leaving orphaned commits, matching push_files behavior. --- pkg/github/repositories.go | 104 +++++++------------------------------ 1 file changed, 18 insertions(+), 86 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 93f7226b46..0c07e78bab 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1062,31 +1062,8 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - // Get the reference for the branch - ref, resp, err := client.Git.GetRef(ctx, owner, repo, "refs/heads/"+branch) - if err != nil { - return nil, nil, fmt.Errorf("failed to get branch reference: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - // 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 ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to get base commit", - resp, - err, - ), nil, nil - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) - } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get commit", resp, body), nil, nil - } + path = strings.TrimPrefix(path, "/") + refName := "refs/heads/" + branch // Create a tree entry for the file deletion by setting SHA to nil treeEntries := []*github.TreeEntry{ @@ -1098,75 +1075,30 @@ func DeleteFile(t translations.TranslationHelperFunc) inventory.ServerTool { }, } - // Create a new tree with the deletion - newTree, resp, err := client.Git.CreateTree(ctx, owner, repo, *baseCommit.Tree.SHA, treeEntries) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to create tree", - resp, - err, - ), nil, nil - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) - } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create tree", resp, body), nil, 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) + pushResult, resp, err := commitEntriesToRef(ctx, client, owner, repo, refName, message, treeEntries) if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to create commit", - resp, - err, - ), nil, nil - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) + errMsg := err.Error() + if strings.Contains(errMsg, "get branch reference") { + return nil, nil, err } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create commit", resp, body), nil, 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.Ref, github.UpdateRef{ - SHA: *newCommit.SHA, - Force: github.Ptr(false), - }) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to update reference", - resp, - err, - ), nil, nil - } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, nil, fmt.Errorf("failed to read response body: %w", err) + stage := "failed to delete file" + switch { + case strings.Contains(errMsg, "get base commit"): + stage = "failed to get base commit" + case strings.Contains(errMsg, "create tree"): + stage = "failed to create tree" + case strings.Contains(errMsg, "create commit"): + stage = "failed to create commit" + case strings.Contains(errMsg, "update reference"): + stage = "failed to update reference" } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update reference", resp, body), nil, nil + return ghErrors.NewGitHubAPIErrorResponse(ctx, stage, resp, err), nil, nil } // Create a response similar to what the DeleteFile API would return response := map[string]any{ - "commit": newCommit, + "commit": pushResult.Commit, "content": nil, } From 73976729794d04d3472e0155b19fa92be3b93e56 Mon Sep 17 00:00:00 2001 From: root Date: Sat, 6 Jun 2026 22:26:50 +0000 Subject: [PATCH 3/3] fix: only retry ref updates on fast-forward conflicts Avoid retrying unrelated 422 errors during push_files and delete_file ref updates. Match hyphenated fast-forward messages from the GitHub API. --- pkg/github/repositories_helper.go | 13 ++++--------- pkg/github/repositories_helper_test.go | 9 +++++++++ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/pkg/github/repositories_helper.go b/pkg/github/repositories_helper.go index 86ded80951..c9be0e748d 100644 --- a/pkg/github/repositories_helper.go +++ b/pkg/github/repositories_helper.go @@ -407,20 +407,15 @@ func closeGitHubResponse(resp *github.Response) { } func isGitRefUpdateConflict(err error, resp *github.Response) bool { - statusCode := 0 - if resp != nil && resp.Response != nil { - statusCode = resp.StatusCode - } if ghErr, ok := err.(*github.ErrorResponse); ok { - if ghErr.Response != nil { - statusCode = ghErr.Response.StatusCode - } msg := strings.ToLower(ghErr.Message) - if strings.Contains(msg, "fast forward") || strings.Contains(msg, "not a fast forward") { + if strings.Contains(msg, "fast forward") || + strings.Contains(msg, "fast-forward") || + strings.Contains(msg, "not a fast forward") { return true } } - return statusCode == http.StatusUnprocessableEntity + return false } type gitPushResult struct { diff --git a/pkg/github/repositories_helper_test.go b/pkg/github/repositories_helper_test.go index 31857294e1..882dff721b 100644 --- a/pkg/github/repositories_helper_test.go +++ b/pkg/github/repositories_helper_test.go @@ -82,6 +82,15 @@ func Test_isGitRefUpdateConflict(t *testing.T) { Response: resp422.Response, Message: "Update is not a fast forward", }, resp422)) + assert.True(t, isGitRefUpdateConflict(&github.ErrorResponse{ + Response: resp422.Response, + Message: "Cannot fast-forward", + }, resp422)) + + assert.False(t, isGitRefUpdateConflict(&github.ErrorResponse{ + Response: resp422.Response, + Message: "Reference does not exist", + }, resp422)) resp200 := &github.Response{Response: &http.Response{StatusCode: http.StatusOK}} assert.False(t, isGitRefUpdateConflict(nil, resp200))