Skip to content

feat: [CODE-5104]: Change fork-sync API status code for conflicts #101

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
abhinavcode wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: [CODE-5104]: Change fork-sync API status code for conflicts #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33be763
Change fork-sync API status code for conflicts
marko-gacesa Feb 26, 2026
db14735
resolve code comments
marko-gacesa Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions app/api/controller/repo/fork_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ func (in *ForkSyncInput) sanitize() error {
return errors.InvalidArgument("Branch commit SHA must be provided")
}

if in.BranchUpstream == "" {
in.BranchUpstream = in.Branch
}

return nil
}

Expand All @@ -58,19 +62,14 @@ func (c *Controller) ForkSync(
session *auth.Session,
repoRef string,
in *ForkSyncInput,
) (*types.ForkSyncOutput, error) {
) (*types.ForkSyncOutput, *types.ForkSyncConflict, error) {
repoForkCore, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush)
if err != nil {
return nil, err
return nil, nil, err
}

if err := in.sanitize(); err != nil {
return nil, err
}

branchUpstreamName := in.BranchUpstream
if branchUpstreamName == "" {
branchUpstreamName = in.Branch
return nil, nil, err
}

branchForkInfo, err := c.git.GetRef(ctx, git.GetRefParams{
Expand All @@ -79,22 +78,22 @@ func (c *Controller) ForkSync(
Type: gitenum.RefTypeBranch,
})
if err != nil {
return nil, fmt.Errorf("failed to get repo branch: %w", err)
return nil, nil, fmt.Errorf("failed to get repo branch: %w", err)
}

if !branchForkInfo.SHA.Equal(in.BranchCommitSHA) {
return nil, errors.InvalidArgumentf("The commit %s isn't the latest commit on the branch %s",
return nil, nil, errors.InvalidArgumentf("The commit %s isn't the latest commit on the branch %s",
in.BranchCommitSHA, in.Branch)
}

branchUpstreamSHA, repoUpstreamCore, err := c.dotRangeService.FetchUpstreamBranch(
ctx,
session,
repoForkCore,
branchUpstreamName,
in.BranchUpstream,
)
if err != nil {
return nil, fmt.Errorf("failed to fetch upstream branch: %w", err)
return nil, nil, fmt.Errorf("failed to fetch upstream branch: %w", err)
}

ancestorResult, err := c.git.IsAncestor(ctx, git.IsAncestorParams{
Expand All @@ -103,14 +102,14 @@ func (c *Controller) ForkSync(
DescendantCommitSHA: branchForkInfo.SHA,
})
if err != nil {
return nil, fmt.Errorf("failed to check if the upstream commit is ancestor: %w", err)
return nil, nil, fmt.Errorf("failed to check if the upstream commit is ancestor: %w", err)
}

if ancestorResult.Ancestor {
// The branch already contains the latest commit from the upstream repository branch - nothing to do.
return &types.ForkSyncOutput{
AlreadyAncestor: true,
}, nil
}, nil, nil
}

mergeBase, err := c.git.MergeBase(ctx, git.MergeBaseParams{
Expand All @@ -119,7 +118,7 @@ func (c *Controller) ForkSync(
Ref2: branchForkInfo.SHA.String(),
})
if err != nil {
return nil, fmt.Errorf("failed to find merge base: %w", err)
return nil, nil, fmt.Errorf("failed to find merge base: %w", err)
}

var (
Expand All @@ -133,7 +132,7 @@ func (c *Controller) ForkSync(
mergeMethod = gitenum.MergeMethodMerge

message = fmt.Sprintf("Merge upstream branch '%s' of %s",
branchUpstreamName, repoUpstreamCore.Path)
in.BranchUpstream, repoUpstreamCore.Path)
committer = controller.SystemServicePrincipalInfo()
author = controller.IdentityFromPrincipalInfo(*session.Principal.ToPrincipalInfo())
}
Expand All @@ -142,7 +141,7 @@ func (c *Controller) ForkSync(

headBranchRef, err := git.GetRefPath(in.Branch, gitenum.RefTypeBranch)
if err != nil {
return nil, fmt.Errorf("failed to generate ref name: %w", err)
return nil, nil, fmt.Errorf("failed to get ref path: %w", err)
}

refs = append(refs, git.RefUpdate{
Expand All @@ -155,12 +154,11 @@ func (c *Controller) ForkSync(

writeParams, err := controller.CreateRPCSystemReferencesWriteParams(ctx, c.urlProvider, session, repoForkCore)
if err != nil {
return nil, fmt.Errorf("failed to create RPC write params: %w", err)
return nil, nil, fmt.Errorf("failed to create RPC sys ref write params: %w", err)
}

mergeOutput, err := c.git.Merge(ctx, &git.MergeParams{
WriteParams: writeParams,
//HeadRepoUID: repoUpstreamCore.GitUID, // TODO: Remove HeadRepoUID!
WriteParams: writeParams,
BaseSHA: branchForkInfo.SHA,
HeadSHA: branchUpstreamSHA,
Message: message,
Expand All @@ -172,18 +170,17 @@ func (c *Controller) ForkSync(
Method: mergeMethod,
})
if err != nil {
return nil, fmt.Errorf("fork branch sync merge failed: %w", err)
return nil, nil, fmt.Errorf("fork branch sync merge failed: %w", err)
}

if mergeOutput.MergeSHA.IsEmpty() || len(mergeOutput.ConflictFiles) > 0 {
return &types.ForkSyncOutput{
return nil, &types.ForkSyncConflict{
ConflictFiles: mergeOutput.ConflictFiles,
Message: fmt.Sprintf("Branch synchronization blocked by conflicting files: %v",
mergeOutput.ConflictFiles),
Message: "Branch synchronization blocked by conflicting files.",
}, nil
Comment on lines 176 to 180

@coderabbitai coderabbitai Bot Feb 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don’t classify empty MergeSHA as conflict by default.

mergeOutput.MergeSHA.IsEmpty() || len(mergeOutput.ConflictFiles) > 0 can incorrectly return a conflict (HTTP 409) for non-conflict merge anomalies. That masks internal failure modes and can misreport status codes.

Suggested fix 
-	if mergeOutput.MergeSHA.IsEmpty() || len(mergeOutput.ConflictFiles) > 0 {
+	if mergeOutput == nil {
+		return nil, nil, fmt.Errorf("fork branch sync merge returned nil output")
+	}
+
+	if len(mergeOutput.ConflictFiles) > 0 {
 		return nil, &types.ForkSyncConflict{
 			ConflictFiles: mergeOutput.ConflictFiles,
 			Message:       "Branch synchronization blocked by conflicting files.",
 		}, nil
 	}
+
+	if mergeOutput.MergeSHA.IsEmpty() {
+		return nil, nil, fmt.Errorf("fork branch sync merge returned empty merge SHA without conflicts")
+	}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/api/controller/repo/fork_sync.go` around lines 176 - 180, The code
currently treats an empty mergeOutput.MergeSHA as a conflict (HTTP 409) because
the condition uses "||"; instead only classify as a conflict when
mergeOutput.ConflictFiles has entries. Change the logic around mergeOutput (the
variable in fork_sync.go) so: if len(mergeOutput.ConflictFiles) > 0 return the
types.ForkSyncConflict as before; else if mergeOutput.MergeSHA.IsEmpty() return
a non-conflict/internal error (e.g., a standard error or a specific internal
error type) so callers map it to a 5xx rather than a 409; update any
tests/handlers expecting the previous behavior accordingly.

}

return &types.ForkSyncOutput{
NewCommitSHA: mergeOutput.MergeSHA,
}, nil
}, nil, nil
}
9 changes: 7 additions & 2 deletions app/api/handler/repo/fork_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,17 @@ func HandleForkSync(repoCtrl *repo.Controller) http.HandlerFunc {
return
}

result, err := repoCtrl.ForkSync(ctx, session, repoRef, in)
resultMerge, resultConflict, err := repoCtrl.ForkSync(ctx, session, repoRef, in)
if err != nil {
render.TranslatedUserError(ctx, w, err)
return
}

render.JSON(w, http.StatusOK, result)
if resultConflict != nil {
render.JSON(w, http.StatusConflict, resultConflict)
return
}

render.JSON(w, http.StatusOK, resultMerge)
}
}
1 change: 1 addition & 0 deletions app/api/openapi/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,6 +1532,7 @@ func repoOperations(reflector *openapi3.Reflector) {
repo.ForkSyncInput
}{}, http.MethodPost)
_ = reflector.SetJSONResponse(&opForkSyncBranch, new(types.ForkSyncOutput), http.StatusOK)
_ = reflector.SetJSONResponse(&opForkSyncBranch, new(types.ForkSyncConflict), http.StatusConflict)
_ = reflector.SetJSONResponse(&opForkSyncBranch, new(usererror.Error), http.StatusBadRequest)
_ = reflector.SetJSONResponse(&opForkSyncBranch, new(usererror.Error), http.StatusInternalServerError)
_ = reflector.SetJSONResponse(&opForkSyncBranch, new(usererror.Error), http.StatusUnauthorized)
Expand Down
11 changes: 7 additions & 4 deletions types/fork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ package types
import "github.com/harness/gitness/git/sha"

type ForkSyncOutput struct {
AlreadyAncestor bool `json:"already_ancestor,omitempty"`
NewCommitSHA sha.SHA `json:"new_commit_sha,omitzero"`
ConflictFiles []string `json:"conflict_files,omitempty"`
Message string `json:"message,omitempty"`
AlreadyAncestor bool `json:"already_ancestor,omitempty"`
NewCommitSHA sha.SHA `json:"new_commit_sha,omitzero"`
}

type ForkSyncConflict struct {
ConflictFiles []string `json:"conflict_files"`
Message string `json:"message"`
}