Block PR approval when merge conflicts are present #637
Conversation
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
Prevent PR approval when merge conflicts are detected in the database, even if GitHub returns Unknown or Clean/Mergeable state. This ensures that merge conflict information is preserved and blocks approval appropriately. Changes: - Add check in check_pr_approval_validity to block approval when DB has HasConflicts - Preserve HasConflicts state in set_pr_mergeability_based_on_user_action when GitHub returns Unknown or Clean/Mergeable - Reload PR from DB after mergeability update to ensure latest state is used - Add tests for merge conflict blocking scenarios
Kobzol
reviewed
Jan 14, 2026
Member
There was a problem hiding this comment.
Thank you, looks great! Mergeability checking is very tricky, and I just finished a big reworking of it, so some of the invariants might not be documented properly yet, but this PR shouldn't need to change how it works - it should be enough to simply block approvals if the PR is currently unmergeable on GitHub.
3 tasks
- Block approval only based on GitHub mergeable state (HasConflicts), do not use DB mergeability for approval checks. - Restore original behavior of set_pr_mergeability_based_on_user_action and avoid extra DB reload in handle_comment. - Update tests so DB-only conflict scenarios expect approval, matching the desired invariants.
Kobzol
reviewed
Jan 14, 2026
Member
Kobzol
left a comment
Kobzol
left a comment
There was a problem hiding this comment.
Yeah I think that you did a bit too much there! 😅 I think that the approve_pr_with_merge_conflict_dirty test is enough, otherwise LGTM.
Keep only approve_pr_with_merge_conflict_dirty test as requested by reviewer. Remove tests that modify DB directly (approve_pr_with_merge_conflict_blocked, approve_pr_with_merge_conflict_in_db, approve_pr_with_merge_conflict_clean_but_db_has_conflicts) to avoid 'cheating' and maintain end-to-end test approach.
Contributor
Author
|
no problem! it was nice to work together! ping me if there's some difficulty that i could help. of course i will discover some issues by myself, tho i'm near my pc all the time :3 |
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.
added new func + test
fixes #452
Prevents
r+command from approving PRs with merge conflicts by adding conflict checks incheck_pr_approval_validity(). The implementation usesMergeableState::from()to convert GitHub mergeable state and checks both GitHub and database states to catch conflicts even when GitHub state hasn't been refreshed yet. Returnsapprove_merge_conflict_comment()error message instead of approving the PR.added tests for all conflict scenarios: dirty, blocked, unknown state with DB conflicts, and clean state with DB conflicts.