feat: [CODE-5104]: Change fork-sync API status code for conflicts#101
Open
abhinavcode wants to merge 2 commits into
Open
feat: [CODE-5104]: Change fork-sync API status code for conflicts#101abhinavcode wants to merge 2 commits into
abhinavcode wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughController ForkSync now returns (output, conflict, error); conflict details moved to a new ForkSyncConflict type. Handlers map conflict results to HTTP 409. ForkSyncInput.sanitize defaults BranchUpstream to Branch and upstream usage is standardized throughout the flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Handler
participant Controller as Controller
Note over Handler,Controller: types: ForkSyncInput, ForkSyncOutput, ForkSyncConflict
Client->>Handler: POST /repo/{repo}/fork/sync (ForkSyncInput)
Handler->>Controller: ForkSync(ctx, session, repoRef, in)
alt No conflict
Controller-->>Handler: (*ForkSyncOutput, nil, nil)
Handler-->>Client: 200 OK (ForkSyncOutput)
else Conflict detected
Controller-->>Handler: (nil, *ForkSyncConflict, nil)
Handler-->>Client: 409 Conflict (ForkSyncConflict)
else Error
Controller-->>Handler: (nil, nil, error)
Handler-->>Client: 5xx / error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/controller/repo/fork_sync.go`:
- Around line 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.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/api/controller/repo/fork_sync.gotypes/fork.go
🚧 Files skipped from review as they are similar to previous changes (1)
- types/fork.go
Comment on lines
176
to
180
| 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 |
There was a problem hiding this comment.
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.
abhinavcode
force-pushed
the
mg/fork/sync-branch-conflict-status-code
branch
from
2458e82 to
db14735
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/api/controller/repo/fork_sync.go (1)
176-180:⚠️ Potential issue | 🟠 MajorAvoid treating empty merge SHA as conflict (409).
Line 176 still maps
MergeSHA.IsEmpty()to conflict. That can mask internal merge anomalies as HTTP 409 instead of surfacing them as server errors.Suggested fix
- if mergeOutput.MergeSHA.IsEmpty() || len(mergeOutput.ConflictFiles) > 0 { + 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 current check treats mergeOutput.MergeSHA.IsEmpty() as a conflict (types.ForkSyncConflict) which masks internal merge failures as HTTP 409; update the logic in fork_sync.go (the block handling mergeOutput) so only len(mergeOutput.ConflictFiles) > 0 produces and returns a types.ForkSyncConflict (409). If mergeOutput.MergeSHA.IsEmpty() is true, return an internal/server error instead (e.g., a non-conflict error or wrapped error) so it surfaces as a 5xx; adjust the conditional and return paths around mergeOutput, MergeSHA.IsEmpty(), and types.ForkSyncConflict accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/api/controller/repo/fork_sync.go`:
- Around line 176-180: The current check treats mergeOutput.MergeSHA.IsEmpty()
as a conflict (types.ForkSyncConflict) which masks internal merge failures as
HTTP 409; update the logic in fork_sync.go (the block handling mergeOutput) so
only len(mergeOutput.ConflictFiles) > 0 produces and returns a
types.ForkSyncConflict (409). If mergeOutput.MergeSHA.IsEmpty() is true, return
an internal/server error instead (e.g., a non-conflict error or wrapped error)
so it surfaces as a 5xx; adjust the conditional and return paths around
mergeOutput, MergeSHA.IsEmpty(), and types.ForkSyncConflict accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Breaking Changes