Dismiss stale reviews per rule

policy/approval/approve.go

@@ -133,19 +133,29 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		}
 	}
 	res.PredicateResults = predicateResults
-	approved, msg, err := r.IsApproved(ctx, prctx)
+
+	allowedCandidates, 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)
 	if err != nil {
 		res.Error = errors.Wrap(err, "failed to compute approval status")
 		return
 	}
 
+	res.AllowedCandidates = allowedCandidates
+
 	res.StatusDescription = msg
 	if approved {
 		res.Status = common.StatusApproved
 	} else {
 		res.Status = common.StatusPending
 		res.ReviewRequestRule = r.getReviewRequestRule()
 	}
+
 	return
 }
 
@@ -169,19 +179,14 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule {
 	}
 }
 
-func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string, error) {
+func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, string, error) {
 	log := zerolog.Ctx(ctx)
 
 	if r.Requires.Count <= 0 {
 		log.Debug().Msg("rule requires no approvals")
 		return true, "No approval required", nil
 	}
 
-	candidates, err := r.filteredCandidates(ctx, prctx)
-	if err != nil {
-		return false, "", err
-	}
-
 	log.Debug().Msgf("found %d candidates for approval", len(candidates))
 
 	// collect users "banned" by approval options
@@ -251,7 +256,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
 	return false, msg, nil
 }
 
-func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) {
+func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) {
 	candidates, err := r.Options.GetMethods().Candidates(ctx, prctx)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get approval candidates")
@@ -285,15 +290,12 @@ func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, c
 
 	var allowedCandidates []*common.Candidate
 	for _, candidate := range candidates {
-		if r.Options.IgnoreEditedComments {
-			if candidate.LastEditedAt.IsZero() {
-				allowedCandidates = append(allowedCandidates, candidate)
-			}
+		if candidate.LastEditedAt.IsZero() {
+			allowedCandidates = append(allowedCandidates, candidate)
 		}
 	}
 
-	log.Debug().Msgf("discarded %d candidates with edited comments",
-		len(candidates)-len(allowedCandidates))
+	log.Debug().Msgf("discarded %d candidates with edited comments", len(candidates)-len(allowedCandidates))
 
 	return allowedCandidates, nil
 }

policy/approval/approve_test.go

@@ -135,7 +135,10 @@ func TestIsApproved(t *testing.T) {
 	}
 
 	assertApproved := func(t *testing.T, prctx pull.Context, r *Rule, expected string) {
-		approved, msg, err := r.IsApproved(ctx, prctx)
+		allowedCandidates, err := r.FilteredCandidates(ctx, prctx)
+		require.NoError(t, err)
+
+		approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)
 		require.NoError(t, err)
 
 		if assert.True(t, approved, "pull request was not approved") {
@@ -144,7 +147,10 @@ func TestIsApproved(t *testing.T) {
 	}
 
 	assertPending := func(t *testing.T, prctx pull.Context, r *Rule, expected string) {
-		approved, msg, err := r.IsApproved(ctx, prctx)
+		allowedCandidates, err := r.FilteredCandidates(ctx, prctx)
+		require.NoError(t, err)
+
+		approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)
 		require.NoError(t, err)
 
 		if assert.False(t, approved, "pull request was incorrectly approved") {

policy/common/methods.go

@@ -34,7 +34,16 @@ type Methods struct {
 	GithubReviewState pull.ReviewState `yaml:"-" json:"-"`
 }
 
+type CandidateType string
+
+const (
+	ReviewCandidate  CandidateType = "review"
+	CommentCandidate CandidateType = "comment"
+)
+
 type Candidate struct {
+	Type         CandidateType
+	ReviewID     string
 	User         string
 	CreatedAt    time.Time
 	LastEditedAt time.Time
@@ -64,6 +73,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 		for _, c := range comments {
 			if m.CommentMatches(c.Body) {
 				candidates = append(candidates, &Candidate{
+					Type:         CommentCandidate,
 					User:         c.Author,
 					CreatedAt:    c.CreatedAt,
 					LastEditedAt: c.LastEditedAt,
@@ -83,13 +93,17 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 				if len(m.GithubReviewCommentPatterns) > 0 {
 					if m.githubReviewCommentMatches(r.Body) {
 						candidates = append(candidates, &Candidate{
+							Type:         ReviewCandidate,
+							ReviewID:     r.ID,
 							User:         r.Author,
 							CreatedAt:    r.CreatedAt,
 							LastEditedAt: r.LastEditedAt,
 						})
 					}
 				} else {
 					candidates = append(candidates, &Candidate{
+						Type:         ReviewCandidate,
+						ReviewID:     r.ID,
 						User:         r.Author,
 						CreatedAt:    r.CreatedAt,
 						LastEditedAt: r.LastEditedAt,

policy/common/result.go

@@ -69,6 +69,7 @@ type Result struct {
 	Requires          Actors
 
 	ReviewRequestRule *ReviewRequestRule
+	AllowedCandidates []*Candidate
 
 	Children []*Result
 }

pull/context.go

@@ -198,6 +198,7 @@ const (
 )
 
 type Review struct {
+	ID           string
 	CreatedAt    time.Time
 	LastEditedAt time.Time
 	Author       string

pull/github.go

@@ -978,6 +978,7 @@ func (pi v4PageInfo) UpdateCursor(vars map[string]interface{}, name string) bool
 }
 
 type v4PullRequestReview struct {
+	ID           string
 	Author       v4Actor
 	State        string
 	Body         string
@@ -1004,6 +1005,7 @@ func (r *v4PullRequestReview) ToReview() *Review {
 	}
 
 	return &Review{
+		ID:           r.ID,
 		CreatedAt:    r.SubmittedAt,
 		LastEditedAt: r.LastEditedAt,
 		Author:       r.Author.GetV3Login(),

server/handler/base.go

@@ -31,6 +31,7 @@ import (
 	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog"
+	"github.com/shurcooL/githubv4"
 )
 
 const (
@@ -186,7 +187,17 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo
 		return err
 	}
 
-	return b.RequestReviewsForResult(ctx, prctx, client, trigger, result)
+	err = b.RequestReviewsForResult(ctx, prctx, client, trigger, result)
+	if err != nil {
+		return err
+	}
+
+	err = b.dismissStaleReviewsForResult(ctx, prctx, v4client, result)
+	if err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (b *Base) ValidateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, fc FetchedConfig, trigger common.Trigger) (common.Evaluator, error) {
@@ -391,6 +402,110 @@ func selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest {
 	return req
 }
 
+func (b *Base) findResultsWithAllowedCandidates(result *common.Result) []*common.Result {
+	var results []*common.Result
+	for _, c := range result.Children {
+		results = append(results, b.findResultsWithAllowedCandidates(c)...)
+	}
+
+	if len(result.Children) == 0 && len(result.AllowedCandidates) > 0 && result.Error == nil {
+		results = append(results, result)
+	}
+
+	return results
+}
+
+func (b *Base) reviewIsAllowed(review *pull.Review, allowedCandidates []*common.Candidate) bool {
+	for _, candidate := range allowedCandidates {
+		if review.ID == candidate.ReviewID {
+			return true
+		}
+	}
+
+	return false
+}
+
+// We already know that these are discarded review candidates for one of three 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, and then finally
+// if it was missing a github review comment pattern that was required.
+//
+// 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 (b *Base) reasonForDismissedReview(review *pull.Review) string {
+	if !review.LastEditedAt.IsZero() {
+		return "was edited"
+	}
+
+	if review.CreatedAt.Before(time.Now().Add(-5 * time.Second)) {
+		return "was invalidated by another commit"
+	}
+
+	return "didn't include a valid comment pattern"
+}
+
+func (b *Base) dismissStaleReviewsForResult(ctx context.Context, prctx pull.Context, v4client *githubv4.Client, result common.Result) error {
+	logger := zerolog.Ctx(ctx)
+
+	var allowedCandidates []*common.Candidate
+
+	results := b.findResultsWithAllowedCandidates(&result)
+	for _, res := range results {
+		for _, candidate := range res.AllowedCandidates {
+			if candidate.Type != common.ReviewCandidate {
+				continue
+			}
+			allowedCandidates = append(allowedCandidates, candidate)
+		}
+	}
+
+	reviews, err := prctx.Reviews()
+	if err != nil {
+		return err
+	}
+
+	for _, r := range reviews {
+		if r.State != pull.ReviewApproved {
+			continue
+		}
+
+		if b.reviewIsAllowed(r, allowedCandidates) {
+			continue
+		}
+
+		reason := b.reasonForDismissedReview(r)
+		message := fmt.Sprintf("Dismissed because the approval %s", reason)
+		logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason)
+		err := b.dismissPullRequestReview(ctx, v4client, r.ID, message)
+		if err != nil {
+			logger.Err(err).Msg("Failed to dismiss stale review")
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (b *Base) dismissPullRequestReview(ctx context.Context, v4client *githubv4.Client, reviewID string, message string) error {
+	var m struct {
+		DismissPullRequestReview struct {
+			ClientMutationID *githubv4.String
+		} `graphql:"dismissPullRequestReview(input: $input)"`
+	}
+
+	input := githubv4.DismissPullRequestReviewInput{
+		PullRequestReviewID: githubv4.String(reviewID),
+		Message:             githubv4.String(message),
+	}
+
+	if err := v4client.Mutate(ctx, &m, input, nil); err != nil {
+		return errors.Wrapf(err, "failed to dismiss pull request review %s", reviewID)
+	}
+
+	return nil
+}
+
 func setStringFromEnv(key, prefix string, value *string) bool {
 	if v, ok := os.LookupEnv(prefix + key); ok {
 		*value = v