Repository: palantir/policy-bot
Pull request #577 (2023-05-13 00:38:38)

Improve review dismissal behavior
Existing dismissal logic compares all reviews on the PR to the set of reviews that might satisfy any rule. This does not work well when policies combine rules that invalidate on push with rules that do not.

In this situation, reviews are not dismissed when new commits are pushed because they might satisfy the rules that do not invalidate. Instead, a user's review is dismissed when they submit a second review. This is because we only consider the most recent review from a user, which means their previous review doesn't count for any rules and is eligible to be dismissed.

We chose this approach to try to dismiss reviews that did not match required comment patterns, but ended up removing that before merging the feature.

This commit switched back to the original approach, which is to collect specific reviews to dismiss, rather than trying to infer what is safe to dismiss. This should be more reliable and is easier to reason about.

### Testing / Edge Cases

I've tested a few different scenarios with my local dev instance and I think it works correctly. There are probably a few edge cases that will still show up (the space of all policies and review orders is large), but I think this is strictly more correct than the current behavior.

Two quirks I observed so far:

1. If a single approval satisfies rules A and B where A is invalidated by pushes and B is not, pushing a new commit will not dismiss the review (because it still satisfies B). This is the intended behavior, but it could confuse people. I think in practice conflicting rules like this are behind predicates.
2. If you're in the situation in (1), the PR is reapproved, and then you push a change that skips B, only the most recent approval is dismissed. There's still an older approval hanging around (it doesn't affect the Policy Bot status) because of the [deduplicateCandidates](https://github.com/palantir/policy-bot/blob/9aee3c7274e46af6463738ba5e8bd815365da257/policy/common/methods.go#L134) function, which only keeps the most recent candidate from a user. I think this is probably fine for now - changing it would mean auditing the approval logic to make sure it accounts for duplicates instead.

### Future Extensions

I don't remember if Policy Bot has permission to edit comments already, but we could probably make this work for comments as well. It could edit the comment to add a line at the bottom explaining that the comment is now invalid.

bluekeyes: >  I wonder if we should consider enabling/disabling this feature at the server level. If its too difficult/confusing to infer why or what is happening, is it valuable?

I don't think a server-level option is valuable right now. I'm pretty confident that with this version, it will always be clear why Policy Bot dismissed a review (or if it's not, that's a bug we can fix.) The confusing cases I've found are situations where a user thinks a review should be dismissed, but Policy Bot does not dismiss it. I believe these will be rare, and usually indicate that your policy is doing something weird (e.g. for changes to a single file, one group has to re-approve on push, but another group does not.)

Patch:
diff --git a/.palantir/go-version b/.palantir/go-version
index 95a9a4c..22c4499 100644
--- a/.palantir/go-version
+++ b/.palantir/go-version
@@ -1,1 +1,1 @@
-go1.20.3
\ No newline at end of file
+go1.20.4
\ No newline at end of file
diff --git a/godel/config/godel.properties b/godel/config/godel.properties
index 1834841..ff6eb44 100644
--- a/godel/config/godel.properties
+++ b/godel/config/godel.properties
@@ -1,2 +1,2 @@
-distributionURL=https://github.com/palantir/godel/releases/download/v2.82.0/godel-2.82.0.tgz
-distributionSHA256=18123507a81833f187c2eda073e9fa735fcd681a050565f9857004240b2c38ee
+distributionURL=https://github.com/palantir/godel/releases/download/v2.83.0/godel-2.83.0.tgz
+distributionSHA256=59f4360974341d899730aafc5464b3d63046c9a6fe22741ed1d25b045b04bc4f
diff --git a/godelw b/godelw
index 22b49aa..ed32336 100755
--- a/godelw
+++ b/godelw
@@ -3,11 +3,11 @@
 set -euo pipefail
 
 # Version and checksums for godel. Values are populated by the godel "dist" task.
-VERSION=2.82.0
-DARWIN_AMD64_CHECKSUM=d304cf283517c7f7b81ec05ca115ccc8d8bdcf61d838daa0ae67b3383130ee50
-DARWIN_ARM64_CHECKSUM=8c6235be28fe345d59a182c62e0b4f0278dce66e6c9c33835a08f71b6214be70
-LINUX_AMD64_CHECKSUM=54ec259a84c50e42bd114c69f86fd90d886c9bbe29db79ba73ed1a14e93cd227
-LINUX_ARM64_CHECKSUM=13035367062ccd6565b6fd1dd17c87f62ad6a7322c6528fafdfd53d534162c8f
+VERSION=2.83.0
+DARWIN_AMD64_CHECKSUM=6c2782c8d377bf96b522cd5d91d1a4a3d45c7eb0b1dbc281dc8776cd1a9f4a27
+DARWIN_ARM64_CHECKSUM=aff91a1187e1ea7c7c7aa7b500d9a40a2e35c31477bc95de8aacacc5f6e6aaa3
+LINUX_AMD64_CHECKSUM=a3a2358d9966d97aa21fc0fb35d40d64565e126f216d483d8c0bf885ad59af8d
+LINUX_ARM64_CHECKSUM=3ad864171770303b80b32385c73e70488cd5b1e6b6bae9643f609cc9e7f70a6b
 
 # Downloads file at URL to destination path using wget or curl. Prints an error and exits if wget or curl is not present.
 function download {
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 7657509..499725e 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -132,21 +132,22 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 	}
 	res.PredicateResults = predicateResults
 
-	allowedCandidates, err := r.FilteredCandidates(ctx, prctx)
+	candidates, dismissals, err := r.FilteredCandidates(ctx, prctx)
 	if err != nil {
 		res.Error = errors.Wrap(err, "failed to filter candidates")
 		return
 	}
 
-	approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)
+	approved, approvers, err := r.IsApproved(ctx, prctx, candidates)
 	if err != nil {
 		res.Error = errors.Wrap(err, "failed to compute approval status")
 		return
 	}
 
-	res.AllowedCandidates = allowedCandidates
+	res.Approvers = approvers
+	res.Dismissals = dismissals
+	res.StatusDescription = r.statusDescription(approved, approvers, candidates)
 
-	res.StatusDescription = msg
 	if approved {
 		res.Status = common.StatusApproved
 	} else {
@@ -177,12 +178,12 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule {
 	}
 }
 
-func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, string, error) {
+func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, []*common.Candidate, error) {
 	log := zerolog.Ctx(ctx)
 
 	if r.Requires.Count <= 0 {
 		log.Debug().Msg("rule requires no approvals")
-		return true, "No approval required", nil
+		return true, nil, nil
 	}
 
 	log.Debug().Msgf("found %d candidates for approval", len(candidates))
@@ -202,7 +203,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []
 	if !r.Options.AllowContributor && !r.Options.AllowNonAuthorContributor {
 		commits, err := r.filteredCommits(ctx, prctx)
 		if err != nil {
-			return false, "", err
+			return false, nil, err
 		}
 
 		for _, c := range commits {
@@ -215,7 +216,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []
 	}
 
 	// filter real approvers using banned status and required membership
-	var approvers []string
+	var approvers []*common.Candidate
 	for _, c := range candidates {
 		if banned[c.User] {
 			log.Debug().Str("user", c.User).Msg("rejecting approval by banned user")
@@ -224,113 +225,118 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []
 
 		isApprover, err := r.Requires.Actors.IsActor(ctx, prctx, c.User)
 		if err != nil {
-			return false, "", errors.Wrap(err, "failed to check candidate status")
+			return false, nil, errors.Wrap(err, "failed to check candidate status")
 		}
 		if !isApprover {
-			log.Debug().Str("user", c.User).Msg("ignoring approval by non-whitelisted user")
+			log.Debug().Str("user", c.User).Msg("ignoring approval by non-required user")
 			continue
 		}
 
-		approvers = append(approvers, c.User)
+		approvers = append(approvers, c)
 	}
 
 	log.Debug().Msgf("found %d/%d required approvers", len(approvers), r.Requires.Count)
-	remaining := r.Requires.Count - len(approvers)
-
-	if remaining <= 0 {
-		msg := fmt.Sprintf("Approved by %s", strings.Join(approvers, ", "))
-		return true, msg, nil
-	}
-
-	if len(candidates) > 0 && len(approvers) == 0 {
-		msg := fmt.Sprintf("%d/%d approvals required. Ignored %s from disqualified users",
-			len(approvers),
-			r.Requires.Count,
-			numberOfApprovals(len(candidates)))
-		return false, msg, nil
-	}
-
-	msg := fmt.Sprintf("%d/%d approvals required", len(approvers), r.Requires.Count)
-	return false, msg, nil
+	return len(approvers) >= r.Requires.Count, approvers, nil
 }
 
-func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) {
+// FilteredCandidates returns the potential approval candidates and any
+// candidates that should be dimissed due to rule options.
+func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, []*common.Dismissal, error) {
+
 	candidates, err := r.Options.GetMethods().Candidates(ctx, prctx)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to get approval candidates")
+		return nil, nil, errors.Wrap(err, "failed to get approval candidates")
 	}
 
 	sort.Stable(common.CandidatesByCreationTime(candidates))
 
+	var editDismissals []*common.Dismissal
 	if r.Options.IgnoreEditedComments {
-		candidates, err = r.filterEditedCandidates(ctx, prctx, candidates)
+		candidates, editDismissals, err = r.filterEditedCandidates(ctx, prctx, candidates)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 
+	var pushDismissals []*common.Dismissal
 	if r.Options.InvalidateOnPush {
-		candidates, err = r.filterInvalidCandidates(ctx, prctx, candidates)
+		candidates, pushDismissals, err = r.filterInvalidCandidates(ctx, prctx, candidates)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 
-	return candidates, nil
+	var dismissals []*common.Dismissal
+	dismissals = append(dismissals, editDismissals...)
+	dismissals = append(dismissals, pushDismissals...)
+
+	return candidates, dismissals, nil
 }
 
-func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) {
+func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) {
 	log := zerolog.Ctx(ctx)
 
 	if !r.Options.IgnoreEditedComments {
-		return candidates, nil
+		return candidates, nil, nil
 	}
 
-	var allowedCandidates []*common.Candidate
-	for _, candidate := range candidates {
-		if candidate.LastEditedAt.IsZero() {
-			allowedCandidates = append(allowedCandidates, candidate)
+	var allowed []*common.Candidate
+	var dismissed []*common.Dismissal
+	for _, c := range candidates {
+		if c.LastEditedAt.IsZero() {
+			allowed = append(allowed, c)
+		} else {
+			dismissed = append(dismissed, &common.Dismissal{
+				Candidate: c,
+				Reason:    "Comment was edited",
+			})
 		}
 	}
 
-	log.Debug().Msgf("discarded %d candidates with edited comments", len(candidates)-len(allowedCandidates))
+	log.Debug().Msgf("discarded %d candidates with edited comments", len(dismissed))
 
-	return allowedCandidates, nil
+	return allowed, dismissed, nil
 }
 
-func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) {
+func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) {
 	log := zerolog.Ctx(ctx)
 
 	if !r.Options.InvalidateOnPush {
-		return candidates, nil
+		return candidates, nil, nil
 	}
 
 	commits, err := r.filteredCommits(ctx, prctx)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	if len(commits) == 0 {
-		return candidates, nil
+		return candidates, nil, nil
 	}
 
 	last := findLastPushed(commits)
 	if last == nil {
-		return nil, errors.New("no commit contained a push date")
+		return nil, nil, errors.New("no commit contained a push date")
 	}
 
-	var allowedCandidates []*common.Candidate
-	for _, candidate := range candidates {
-		if candidate.CreatedAt.After(*last.PushedAt) {
-			allowedCandidates = append(allowedCandidates, candidate)
+	var allowed []*common.Candidate
+	var dismissed []*common.Dismissal
+	for _, c := range candidates {
+		if c.CreatedAt.After(*last.PushedAt) {
+			allowed = append(allowed, c)
+		} else {
+			dismissed = append(dismissed, &common.Dismissal{
+				Candidate: c,
+				Reason:    fmt.Sprintf("Invalidated by push of %.7s", last.SHA),
+			})
 		}
 	}
 
 	log.Debug().Msgf("discarded %d candidates invalidated by push of %s at %s",
-		len(candidates)-len(allowedCandidates),
+		len(dismissed),
 		last.SHA,
 		last.PushedAt.Format(time.RFC3339))
 
-	return allowedCandidates, nil
+	return allowed, dismissed, nil
 }
 
 func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull.Commit, error) {
@@ -369,6 +375,26 @@ func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull
 	return filtered, nil
 }
 
+func (r *Rule) statusDescription(approved bool, approvers, candidates []*common.Candidate) string {
+	if approved {
+		if len(approvers) == 0 {
+			return "No approval required"
+		}
+
+		var names []string
+		for _, c := range approvers {
+			names = append(names, c.User)
+		}
+		return fmt.Sprintf("Approved by %s", strings.Join(names, ", "))
+	}
+
+	desc := fmt.Sprintf("%d/%d required approvals", len(approvers), r.Requires.Count)
+	if disqualified := len(candidates) - len(approvers); disqualified > 0 {
+		desc += fmt.Sprintf(". Ignored %s from disqualified users", numberOfApprovals(disqualified))
+	}
+	return desc
+}
+
 func isUpdateMerge(commits []*pull.Commit, c *pull.Commit) bool {
 	// must be a simple merge commit (exactly 2 parents)
 	if len(c.Parents) != 2 {
diff --git a/policy/common/result.go b/policy/common/result.go
index fef0d90..3d3ab50 100644
--- a/policy/common/result.go
+++ b/policy/common/result.go
@@ -74,8 +74,19 @@ type Result struct {
 	Requires          Requires
 	Methods           *Methods
 
+	// Approvers contains the candidates that satisfied the rule.
+	Approvers []*Candidate
+
+	// Dismissals contains candidates that should be discarded because they
+	// cannot satisfy any future evaluations.
+	Dismissals []*Dismissal
+
 	ReviewRequestRule *ReviewRequestRule
-	AllowedCandidates []*Candidate
 
 	Children []*Result
 }
+
+type Dismissal struct {
+	Candidate *Candidate
+	Reason    string
+}
diff --git a/server/handler/eval_context_dismissal.go b/server/handler/eval_context_dismissal.go
index 5375510..466b51f 100644
--- a/server/handler/eval_context_dismissal.go
+++ b/server/handler/eval_context_dismissal.go
@@ -16,11 +16,8 @@ package handler
 
 import (
 	"context"
-	"fmt"
-	"time"
 
 	"github.com/palantir/policy-bot/policy/common"
-	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog"
 	"github.com/shurcooL/githubv4"
@@ -29,86 +26,61 @@ import (
 func (ec *EvalContext) dismissStaleReviewsForResult(ctx context.Context, result common.Result) error {
 	logger := zerolog.Ctx(ctx)
 
-	var allowedCandidates []*common.Candidate
+	approvers := findAllApprovers(&result)
 
-	results := findResultsWithAllowedCandidates(&result)
-	for _, res := range results {
-		for _, candidate := range res.AllowedCandidates {
-			if candidate.Type != common.ReviewCandidate {
-				continue
-			}
-			allowedCandidates = append(allowedCandidates, candidate)
-		}
-	}
-
-	reviews, err := ec.PullContext.Reviews()
-	if err != nil {
-		return err
-	}
-
-	for _, r := range reviews {
-		if r.State != pull.ReviewApproved {
+	alreadyDismissed := make(map[string]bool)
+	for _, d := range findAllDismissals(&result) {
+		// Only dismiss stale reviews for now (ignore comments)
+		if d.Candidate.Type != common.ReviewCandidate {
 			continue
 		}
-
-		if reviewIsAllowed(r, allowedCandidates) {
+		// Only dismiss reviews from users who are not currently approvers
+		if approvers[d.Candidate.User] {
 			continue
 		}
-
-		reason := reasonForDismissedReview(r)
-		if reason == "" {
+		// Only dismiss reviews once if they're dismissed by multiple rules
+		if alreadyDismissed[d.Candidate.ReviewID] {
 			continue
 		}
 
-		message := fmt.Sprintf("Dismissed because the approval %s", reason)
-		logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason)
-		if err := dismissPullRequestReview(ctx, ec.V4Client, r.ID, message); err != nil {
+		logger.Info().Str("reason", d.Reason).Msgf("Dismissing stale review %s", d.Candidate.ReviewID)
+		if err := dismissPullRequestReview(ctx, ec.V4Client, d.Candidate.ReviewID, d.Reason); err != nil {
 			return err
 		}
+		alreadyDismissed[d.Candidate.ReviewID] = true
 	}
 
 	return nil
 }
 
-func findResultsWithAllowedCandidates(result *common.Result) []*common.Result {
-	var results []*common.Result
-	for _, c := range result.Children {
-		results = append(results, findResultsWithAllowedCandidates(c)...)
-	}
+func findAllDismissals(result *common.Result) []*common.Dismissal {
+	var dismissals []*common.Dismissal
 
-	if len(result.Children) == 0 && len(result.AllowedCandidates) > 0 && result.Error == nil {
-		results = append(results, result)
+	if len(result.Children) == 0 && result.Error == nil {
+		dismissals = append(dismissals, result.Dismissals...)
+	}
+	for _, c := range result.Children {
+		dismissals = append(dismissals, findAllDismissals(c)...)
 	}
 
-	return results
+	return dismissals
 }
 
-func reviewIsAllowed(review *pull.Review, allowedCandidates []*common.Candidate) bool {
-	for _, candidate := range allowedCandidates {
-		if review.ID == candidate.ReviewID {
-			return true
-		}
-	}
-	return false
-}
+func findAllApprovers(result *common.Result) map[string]bool {
+	approvers := make(map[string]bool)
 
-// We already know that these are discarded review candidates for 1 of 2 reasons
-// so first we check for edited and then we check to see if its a review thats at least
-// 5 seconds old and we know that it was invalidated by a new commit.
-//
-// This is brittle and may need refactoring in future versions because it assumes the bot
-// will take less than 5 seconds to respond, but thought that having a dismissal reason
-// was valuable.
-func reasonForDismissedReview(review *pull.Review) string {
-	if !review.LastEditedAt.IsZero() {
-		return "was edited"
+	if len(result.Children) == 0 && result.Error == nil {
+		for _, a := range result.Approvers {
+			approvers[a.User] = true
+		}
 	}
-
-	if review.CreatedAt.Before(time.Now().Add(-5 * time.Second)) {
-		return "was invalidated by another commit"
+	for _, c := range result.Children {
+		for u := range findAllApprovers(c) {
+			approvers[u] = true
+		}
 	}
 
-	return ""
+	return approvers
 }
 
 func dismissPullRequestReview(ctx context.Context, v4client *githubv4.Client, reviewID string, message string) error {


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index 91e8fb5..b484d1a 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -141,25 +141,27 @@ func TestIsApproved(t *testing.T) {
 	}
 
 	assertApproved := func(t *testing.T, prctx pull.Context, r *Rule, expected string) {
-		allowedCandidates, err := r.FilteredCandidates(ctx, prctx)
+		allowedCandidates, _, err := r.FilteredCandidates(ctx, prctx)
 		require.NoError(t, err)
 
-		approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)
+		approved, approvers, err := r.IsApproved(ctx, prctx, allowedCandidates)
 		require.NoError(t, err)
 
 		if assert.True(t, approved, "pull request was not approved") {
+			msg := r.statusDescription(approved, approvers, allowedCandidates)
 			assert.Equal(t, expected, msg)
 		}
 	}
 
 	assertPending := func(t *testing.T, prctx pull.Context, r *Rule, expected string) {
-		allowedCandidates, err := r.FilteredCandidates(ctx, prctx)
+		allowedCandidates, _, err := r.FilteredCandidates(ctx, prctx)
 		require.NoError(t, err)
 
-		approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)
+		approved, approvers, err := r.IsApproved(ctx, prctx, allowedCandidates)
 		require.NoError(t, err)
 
 		if assert.False(t, approved, "pull request was incorrectly approved") {
+			msg := r.statusDescription(approved, approvers, allowedCandidates)
 			assert.Equal(t, expected, msg)
 		}
 	}
@@ -177,7 +179,7 @@ func TestIsApproved(t *testing.T) {
 				Count: 1,
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("authorCannotApprove", func(t *testing.T) {
@@ -315,7 +317,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("specificOrgApproves", func(t *testing.T) {
@@ -338,7 +340,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("specificOrgsOrUserApproves", func(t *testing.T) {
@@ -377,7 +379,7 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by comment-approver")
 
 		r.Options.InvalidateOnPush = true
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 6 approvals from disqualified users")
 	})
 
 	t.Run("invalidateReviewOnPush", func(t *testing.T) {
@@ -402,7 +404,7 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by review-approver")
 
 		r.Options.InvalidateOnPush = true
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 1 approval from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 1 approval from disqualified users")
 	})
 
 	t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) {
@@ -429,7 +431,7 @@ func TestIsApproved(t *testing.T) {
 				InvalidateOnPush: true,
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 6 approvals from disqualified users")
 
 		r.Options.IgnoreUpdateMerges = true
 		assertApproved(t, prctx, r, "Approved by comment-approver")
@@ -461,7 +463,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 8 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 8 approvals from disqualified users")
 
 		r.Options.IgnoreUpdateMerges = true
 		assertApproved(t, prctx, r, "Approved by merge-committer")
@@ -483,7 +485,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users")
 
 		r.Options.IgnoreCommitsBy = common.Actors{
 			Users: []string{"comment-approver"},
@@ -507,7 +509,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("ignoreCommitsInvalidateOnPush", func(t *testing.T) {
@@ -554,7 +556,7 @@ func TestIsApproved(t *testing.T) {
 
 		r.Options.IgnoreEditedComments = true
 
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users")
 	})
 
 	t.Run("ignoreEditedComments", func(t *testing.T) {
@@ -573,7 +575,7 @@ func TestIsApproved(t *testing.T) {
 
 		r.Options.IgnoreEditedComments = true
 
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users")
 	})
 
 	t.Run("ignoreEditedCommentsWithBodyPattern", func(t *testing.T) {
@@ -600,7 +602,7 @@ func TestIsApproved(t *testing.T) {
 
 		r.Options.IgnoreEditedComments = true
 
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users")
 	})
 }