Repository: palantir/policy-bot
Pull request #463 (2022-08-30 21:06:42)

Dismiss stale reviews per rule
Closes https://github.com/palantir/policy-bot/issues/239

After we migrated over to policy bot, I had a couple people ask why the dismiss feature didn't work the same. If you check the dismiss reviews option in the branch protection rules on the branch it will dismiss all reviews, but what if you have a policy bot policy that only cares to invalidate a push on one rule but not another. Without the dismiss option in github selected, you still get a green check on the approval comment even if policy bot treats it as invalidated so that causes a little bit of confusion.

After reading through the issues I found [this](https://github.com/palantir/policy-bot/issues/239) and wanted to see what it could look like. In theory, I think something like this could work. I ran into something weird while testing it however, Github's dismiss review API is returning a 403 for me even though their [docs](https://docs.github.com/en/rest/overview/permissions-required-for-github-apps) indicate that this is something that a github app should have permission to do. So maybe there's just something else up, I need to write some tests to see what's happening, but before I go too far down that rabbit hole wanted to surface this to see if it would be a feature worth entertaining. I didn't see much discussion on the original issue and that was a while ago.

/cc @bluekeyes 

bluekeyes: Thanks starting an implementation of this feature! I don't have any objections to dismissing stale reviews, so I think it's worthwhile to continue testing and iterating here. This hasn't been a common complaint or request internally at Palantir, so the original issue never made it to the top of our priorities, but it feels like one of those features where everyone will appreciate it once it exists.

For the 403 error, I wonder if by default apps can only dismiss their own reviews and have to be granted some additional permission to dismiss other reviews. The docs for the dismissal API warn:

> Note: To dismiss a pull request review on a [protected branch](https://docs.github.com/rest/reference/repos#branches), you must be a repository administrator or be included in the list of people or teams who can dismiss pull request reviews.
devinburnette: @bluekeyes yea i've tried both removing the branch protection outright and granting admin write to no avail. i get the same error no matter what I do, so maybe its something else i'm doing wrong?

`403 Branch protections do not permit dismissing this review []`

the fact that those `[]` are empty there at the end, that must mean something.
bluekeyes: Not sure if you've tried this yet, but for weird API issues, I usually resort to using the Ruby script from [Authenticating as a GitHub App](https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app) to generated a JWT, exchange it for installation token, and then test requests with `curl`. That way you can see raw errors from GitHub and try things like preview headers quickly.

We don't use any GraphQL mutations in Policy Bot yet, but it looks like there's a [`dismissPullRequestReview`](https://docs.github.com/en/graphql/reference/mutations#dismisspullrequestreview) mutation you could try (probably using the `curl` method.) If it also doesn't it work, it might give a more helpful error.
devinburnette: @bluekeyes yea looks like the REST API just doesn't work, but the GraphQL one does, so I'll try that route and see how things look.
devinburnette: @bluekeyes i think i'm ready for a first glance at this.. this is working well in my testing, before i get too deep at writing tests, i could use your expertise on where you think the bulk of the logic should live?

I went with combining the pull request and pull request review handlers so that these functions could be triggered as a result of an event which invalidates an approval candidate because it didn't feel right putting in inside the filtering logic, so I exported that function and refactored it a little to support the use case.
bluekeyes: Yeah, I agree that implementing dismissal as part of the filtering logic would feel wrong, but I don't think combing the `pull_request` and `pull_request_review` handlers is the best alternative.

There might be a flaw in this I haven't seen yet, but my initial thought is to structure this similar to review requests. Because we're only dismissing reviews that Policy Bot would ignore anyway, it shouldn't matter if we do it before or after evaluating the policy. That means we could add a new field to the `common.Result` struct that contains the dismissal information and have a `handlers.Base` function that acts on that when it is set.

If possible, I'd like to keep the flow `Event -> Evaluation -> Result -> Side Effects`, where the "evaluation" step can be treated like a single function without exposing internal logic like the concept of candidates and filtering.

I see you just marked this as ready for review too, so I'll try to take a closer look this week.
devinburnette: @bluekeyes i've updated my approach to utilize `common.Result` but I'm still finding myself having to pass information through the candidate abstraction, it's probably fine, but wanted to see how you felt about it. 

additionally, I opened a case with Github about the REST API not working as described by the documentation and the findings are that `content: write` and `pull requests: write` are required via REST and only `pull requests: write` is required via GraphQL not sure how they will fix that up, but I'm doubtful they will add the content requirement to GraphQL since that would be a breaking change to whoever is currently using it that way and the documentation for contents seems to have nothing to do with PR review dismissal.
devinburnette: @bluekeyes i've taken a stab at implementing the feedback, and while testing this `common.Result` approach after implementing your suggestions, I think I've found a minor hurdle in review dismissal post evaluation for edited reviews. Basically when a review with an approval comment is edited and it no longer matches the required pattern, then it is no longer considered a candidate so the github dismissal never happens. 

Since the pattern no longer matches, i'm sure it would be very easy for a user to tell why their PR isn't approved, but the green check mark still being there isn't the most ideal. What do you think?

Patch:
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 2d56863..c39fc7a 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -136,12 +136,21 @@ 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
@@ -149,6 +158,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 		res.Status = common.StatusPending
 		res.ReviewRequestRule = r.getReviewRequestRule()
 	}
+
 	return
 }
 
@@ -172,7 +182,7 @@ 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 {
@@ -180,11 +190,6 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
 		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
@@ -254,7 +259,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")
@@ -288,15 +293,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
 }
diff --git a/policy/common/methods.go b/policy/common/methods.go
index d99b70c..8ddd23f 100644
--- a/policy/common/methods.go
+++ b/policy/common/methods.go
@@ -35,7 +35,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
@@ -65,6 +74,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,
@@ -98,6 +108,8 @@ 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,
@@ -105,6 +117,8 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 					}
 				} else {
 					candidates = append(candidates, &Candidate{
+						Type:         ReviewCandidate,
+						ReviewID:     r.ID,
 						User:         r.Author,
 						CreatedAt:    r.CreatedAt,
 						LastEditedAt: r.LastEditedAt,
diff --git a/policy/common/result.go b/policy/common/result.go
index 91c43aa..99ed3d2 100644
--- a/policy/common/result.go
+++ b/policy/common/result.go
@@ -69,6 +69,7 @@ type Result struct {
 	Requires          Actors
 
 	ReviewRequestRule *ReviewRequestRule
+	AllowedCandidates []*Candidate
 
 	Children []*Result
 }
diff --git a/pull/context.go b/pull/context.go
index a351a52..44da64b 100644
--- a/pull/context.go
+++ b/pull/context.go
@@ -201,6 +201,7 @@ const (
 )
 
 type Review struct {
+	ID           string
 	CreatedAt    time.Time
 	LastEditedAt time.Time
 	Author       string
diff --git a/pull/github.go b/pull/github.go
index 43220aa..054dc4e 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -1009,6 +1009,7 @@ func (pi v4PageInfo) UpdateCursor(vars map[string]interface{}, name string) bool
 }
 
 type v4PullRequestReview struct {
+	ID           string
 	Author       v4Actor
 	State        string
 	Body         string
@@ -1035,6 +1036,7 @@ func (r *v4PullRequestReview) ToReview() *Review {
 	}
 
 	return &Review{
+		ID:           r.ID,
 		CreatedAt:    r.SubmittedAt,
 		LastEditedAt: r.LastEditedAt,
 		Author:       r.Author.GetV3Login(),
diff --git a/server/handler/base.go b/server/handler/base.go
index 6eb544f..6ab2491 100644
--- a/server/handler/base.go
+++ b/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


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index 69675f4..31c1822 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -141,7 +141,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") {
@@ -150,7 +153,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") {