tide: changes requested block merge when branch protection requires reviews

Author: Prucek

pkg/tide/github.go

@@ -529,16 +529,18 @@ type mergeChecker struct {
 	ghc    githubClient
 
 	sync.Mutex
-	cache map[config.OrgRepo]map[types.PullRequestMergeType]bool
+	cache                        map[config.OrgRepo]map[types.PullRequestMergeType]bool
+	branchProtectionReviewsCache map[string]bool
 }
 
 // newMergeChecker creates a mergeChecker for GitHub, and should be used only by
 // GitHubProvider.
 func newMergeChecker(cfg config.Getter, ghc githubClient) *mergeChecker {
 	m := &mergeChecker{
-		config: cfg,
-		ghc:    ghc,
-		cache:  map[config.OrgRepo]map[types.PullRequestMergeType]bool{},
+		config:                       cfg,
+		ghc:                          ghc,
+		cache:                        map[config.OrgRepo]map[types.PullRequestMergeType]bool{},
+		branchProtectionReviewsCache: map[string]bool{},
 	}
 
 	go m.clearCache()
@@ -554,6 +556,7 @@ func (m *mergeChecker) clearCache() {
 		<-ticker.C
 		m.Lock()
 		m.cache = make(map[config.OrgRepo]map[types.PullRequestMergeType]bool)
+		m.branchProtectionReviewsCache = map[string]bool{}
 		m.Unlock()
 	}
 }
@@ -588,6 +591,36 @@ func (m *mergeChecker) repoMethods(orgRepo config.OrgRepo) (map[types.PullReques
 	return repoMethods, nil
 }
 
+// branchRequiresReviews checks if branch protection requires pull request reviews.
+func (m *mergeChecker) branchRequiresReviews(org, repo, branch string) (bool, error) {
+	cacheKey := fmt.Sprintf("%s/%s:%s", org, repo, branch)
+	m.Lock()
+	cached, ok := m.branchProtectionReviewsCache[cacheKey]
+	m.Unlock()
+	if ok {
+		return cached, nil
+	}
+
+	bp, err := m.ghc.GetBranchProtection(org, repo, branch)
+	if err != nil {
+		// If branch protection is not configured or we can't access it,
+		// assume reviews are not required (GitHub's default).
+		// Cache the result to avoid repeated API calls for the same branch.
+		m.Lock()
+		m.branchProtectionReviewsCache[cacheKey] = false
+		m.Unlock()
+		return false, nil
+	}
+
+	requiresReviews := bp.RequiredPullRequestReviews != nil &&
+		bp.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0
+
+	m.Lock()
+	m.branchProtectionReviewsCache[cacheKey] = requiresReviews
+	m.Unlock()
+	return requiresReviews, nil
+}
+
 // isAllowed checks if a PR does not have merge conflicts and requests an
 // allowed merge method. If there is no error it returns a string explanation if
 // not allowed or "" if allowed.
@@ -621,6 +654,19 @@ func (m *mergeChecker) isAllowedToMerge(crc *CodeReviewCommon) (string, error) {
 	} else if !allowed {
 		return fmt.Sprintf("Merge type %q disallowed by repo settings", *mergeMethod), nil
 	}
+	// Check if PR has "changes requested" review state and branch protection requires reviews.
+	if pr.ReviewDecision == githubql.PullRequestReviewDecisionChangesRequested {
+		requiresReviews, err := m.branchRequiresReviews(crc.Org, crc.Repo, crc.BaseRefName)
+		if err != nil {
+			logrus.WithError(err).WithFields(logrus.Fields{
+				"org":    crc.Org,
+				"repo":   crc.Repo,
+				"branch": crc.BaseRefName,
+			}).Debug("Error checking if branch requires reviews, allowing GitHub to enforce")
+		} else if requiresReviews {
+			return "PR has changes requested in reviews and branch protection requires reviews", nil
+		}
+	}
 	return "", nil
 }
 

pkg/tide/status.go

@@ -125,7 +125,9 @@ func (sc *statusController) shutdown() {
 // Note: an empty diff can be returned if the reason that the PR does not match
 // the TideQuery is unknown. This can happen if this function's logic
 // does not match GitHub's and does not indicate that the PR matches the query.
-func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker) (string, int) {
+// branchRequiresReviews is an optional function that checks if branch protection
+// requires reviews. If nil, "changes requested" will always be treated as not blocking.
+func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker, branchRequiresReviews func(string, string, string) (bool, error)) (string, int) {
 	const maxLabelChars = 50
 	var desc string
 	var diff int
@@ -254,7 +256,21 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker) (s
 		}
 	}
 
-	if q.ReviewApprovedRequired && pr.ReviewDecision != githubql.PullRequestReviewDecisionApproved {
+	// PullRequestReviewDecisionChangesRequested blocks merging only if branch protection requires reviews.
+	if pr.ReviewDecision == githubql.PullRequestReviewDecisionChangesRequested {
+		shouldBlock := false
+		if branchRequiresReviews != nil {
+			if requiresReviews, err := branchRequiresReviews(string(pr.Repository.Owner.Login), string(pr.Repository.Name), string(pr.BaseRef.Name)); err == nil {
+				shouldBlock = requiresReviews
+			}
+		}
+		if shouldBlock {
+			diff += 50
+			if desc == "" {
+				desc = " PR has changes requested in reviews"
+			}
+		}
+	} else if q.ReviewApprovedRequired && pr.ReviewDecision != githubql.PullRequestReviewDecisionApproved {
 		diff += 50
 		if desc == "" {
 			desc = " PullRequest is missing sufficient approving GitHub review(s)"
@@ -314,8 +330,11 @@ func (sc *statusController) expectedStatus(log *logrus.Entry, queryMap *config.Q
 
 		minDiffCount := -1
 		var minDiff string
+		branchRequiresReviewsFunc := func(org, repo, branch string) (bool, error) {
+			return sc.ghProvider.mergeChecker.branchRequiresReviews(org, repo, branch)
+		}
 		for _, q := range queryMap.ForRepo(repo) {
-			diff, diffCount := requirementDiff(pr, &q, cc)
+			diff, diffCount := requirementDiff(pr, &q, cc, branchRequiresReviewsFunc)
 			if diffCount == 0 {
 				hasFulfilledQuery = true
 				break