Dismiss stale reviews when reviewing PRs with vulnerable dependencies

When the user accepts mediator's suggestions on PRs that container
vulnerable dependencies, we need to re-review the PR. This patch
dismisses the earlier review and adds a new one, either just commenting
that no vulnerable packages were found or listing the remainign
vulnerabilities.

Also moves the package review code behind an interface that will be used
later to just add comments and optionally set commit status to prevent
merge with vulnerable commits.

Fixes: #914

internal/engine/eval/vulncheck/actions.go

@@ -0,0 +1,42 @@
+// Copyright 2023 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.role/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package vulncheck provides the vulnerability check evaluator
+package vulncheck
+
+import (
+	"context"
+	"fmt"
+
+	pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
+	ghclient "github.com/stacklok/mediator/pkg/providers/github"
+)
+
+type prStatusHandler interface {
+	trackVulnerableDep(
+		ctx context.Context,
+		dep *pb.PrDependencies_ContextualDependency,
+		patch patchFormatter,
+	) error
+	submit(ctx context.Context) error
+}
+
+func newPrStatusHandler(action action, pr *pb.PullRequest, client ghclient.RestAPI) (prStatusHandler, error) {
+	switch action {
+	case actionRejectPr:
+		return newReviewPrHandler(pr, client)
+	default:
+		return nil, fmt.Errorf("unknown action: %s", action)
+	}
+}

internal/engine/eval/vulncheck/review.go

@@ -23,11 +23,27 @@ import (
 	"strings"
 
 	"github.com/google/go-github/v53/github"
+	"github.com/rs/zerolog"
 
 	pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
 	ghclient "github.com/stacklok/mediator/pkg/providers/github"
 )
 
+const (
+	reviewBodyMagicComment              = "<!-- mediator: pr-review-body -->"
+	reviewBodyRequestChangesCommentText = `
+Mediator found vulnerable dependencies in this PR. Either push an updated
+version or accept the proposed changes. Note that accepting the changes will
+include mediator as a co-author of this PR.
+`
+	reviewBodyCommentText = `
+Mediator analyzed this PR and found no vulnerable dependencies.
+`
+	reviewBodyDismissCommentText = `
+Previous mediator review was dismissed because the PR was updated.
+`
+)
+
 type reviewLocation struct {
 	lineToChange      int
 	leadingWhitespace int
@@ -79,38 +95,121 @@ func locateDepInPr(
 	return &loc, nil
 }
 
-func requestChanges(
-	ctx context.Context,
-	client ghclient.RestAPI,
-	fileName string,
+type reviewPrHandler struct {
+	cli ghclient.RestAPI
+	pr  *pb.PullRequest
+
+	mediatorReview *github.PullRequestReview
+
+	comments []*github.DraftReviewComment
+	status   *string
+	text     *string
+}
+
+func newReviewPrHandler(
 	pr *pb.PullRequest,
-	location *reviewLocation,
-	comment string,
+	cli ghclient.RestAPI,
+) (prStatusHandler, error) {
+	if pr == nil {
+		return nil, fmt.Errorf("pr was nil, can't review")
+	}
+
+	return &reviewPrHandler{
+		cli:      cli,
+		pr:       pr,
+		comments: []*github.DraftReviewComment{},
+	}, nil
+}
+
+func (ra *reviewPrHandler) trackVulnerableDep(
+	ctx context.Context,
+	dep *pb.PrDependencies_ContextualDependency,
+	patch patchFormatter,
 ) error {
-	var comments []*github.DraftReviewComment
+	location, err := locateDepInPr(ctx, ra.cli, dep)
+	if err != nil {
+		return fmt.Errorf("could not locate dependency in PR: %w", err)
+	}
 
+	comment := patch.IndentedString(location.leadingWhitespace)
 	body := fmt.Sprintf("```suggestion\n"+"%s\n"+"```\n", comment)
 
 	reviewComment := &github.DraftReviewComment{
-		Path:      github.String(fileName),
+		Path:      github.String(dep.File.Name),
 		Position:  nil,
 		StartLine: github.Int(location.lineToChange),
 		Line:      github.Int(location.lineToChange + 3), // TODO(jakub): Need to count the lines from the patch
 		Body:      github.String(body),
 	}
-	comments = append(comments, reviewComment)
+	ra.comments = append(ra.comments, reviewComment)
+
+	return nil
+}
+
+func (ra *reviewPrHandler) submit(ctx context.Context) error {
+	if err := ra.findPreviousReview(ctx); err != nil {
+		return fmt.Errorf("could not find previous review: %w", err)
+	}
+
+	if ra.mediatorReview != nil {
+		fmt.Println("Dismissing review")
+		err := ra.dismissReview(ctx)
+		if err != nil {
+			zerolog.Ctx(ctx).Error().Err(err).Msg("could not dismiss review")
+		}
+		zerolog.Ctx(ctx).Info().Int32("pull-request", ra.pr.Number).Msg("dismissed review")
+	}
+
+	// either there are changes to request or just send the first review mentioning that everything is ok
+	ra.setStatus()
+	if err := ra.submitReview(ctx); err != nil {
+		return fmt.Errorf("could not submit review: %w", err)
+	}
+	return nil
+}
 
+func (ra *reviewPrHandler) setStatus() {
+	if len(ra.comments) > 0 {
+		// if this pass produced comments, request changes
+		ra.status = github.String("REQUEST_CHANGES")
+		ra.text = github.String(reviewBodyRequestChangesCommentText)
+	} else {
+		// if this pass produced no comments, resolve the mediator review
+		ra.status = github.String("COMMENT")
+		ra.text = github.String(reviewBodyCommentText)
+	}
+}
+
+func (ra *reviewPrHandler) findPreviousReview(ctx context.Context) error {
+	reviews, err := ra.cli.ListReviews(ctx, ra.pr.RepoOwner, ra.pr.RepoName, int(ra.pr.Number), nil)
+	if err != nil {
+		return fmt.Errorf("could not list reviews: %w", err)
+	}
+
+	ra.mediatorReview = nil
+	for _, r := range reviews {
+		if strings.HasPrefix(r.GetBody(), reviewBodyMagicComment) && r.GetState() != "DISMISSED" {
+			ra.mediatorReview = r
+			break
+		}
+	}
+
+	return nil
+}
+
+func (ra *reviewPrHandler) submitReview(ctx context.Context) error {
 	review := &github.PullRequestReviewRequest{
-		CommitID: github.String(pr.CommitSha),
-		Event:    github.String("REQUEST_CHANGES"),
-		Comments: comments,
+		CommitID: github.String(ra.pr.CommitSha),
+		Event:    ra.status,
+		Comments: ra.comments,
+		Body:     github.String(fmt.Sprintf("%s\n\n%s", reviewBodyMagicComment, *ra.text)),
 	}
 
-	_, err := client.CreateReview(
+	_, err := ra.cli.CreateReview(
 		ctx,
-		pr.RepoOwner,
-		pr.RepoName,
-		int(pr.Number),
+		ra.pr.RepoOwner,
+		ra.pr.RepoName,
+		int(ra.pr.Number),
 		review,
 	)
 	if err != nil {
@@ -119,3 +218,25 @@ func requestChanges(
 
 	return nil
 }
+
+func (ra *reviewPrHandler) dismissReview(ctx context.Context) error {
+	if ra.mediatorReview == nil {
+		return nil
+	}
+
+	dismissReview := &github.PullRequestReviewDismissalRequest{
+		Message: github.String(reviewBodyDismissCommentText),
+	}
+
+	_, err := ra.cli.DismissReview(
+		ctx,
+		ra.pr.RepoOwner,
+		ra.pr.RepoName,
+		int(ra.pr.Number),
+		ra.mediatorReview.GetID(),
+		dismissReview)
+	if err != nil {
+		return fmt.Errorf("could not dismiss review: %w", err)
+	}
+	return nil
+}

internal/engine/eval/vulncheck/vulncheck.go

@@ -18,7 +18,6 @@ package vulncheck
 import (
 	"context"
 	"fmt"
-	"log"
 
 	engif "github.com/stacklok/mediator/internal/engine/interfaces"
 	pb "github.com/stacklok/mediator/pkg/generated/protobuf/go/mediator/v1"
@@ -58,6 +57,11 @@ func (e *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Res
 		return fmt.Errorf("failed to parse config: %w", err)
 	}
 
+	prReplyHandler, err := newPrStatusHandler(ruleConfig.Action, prdeps.Pr, e.cli)
+	if err != nil {
+		return fmt.Errorf("failed to create pr action: %w", err)
+	}
+
 	for _, dep := range prdeps.Deps {
 		ecoConfig := ruleConfig.getEcosystemConfig(dep.Dep.Ecosystem)
 		vdb, err := e.getVulnDb(ecoConfig.DbType, ecoConfig.DbEndpoint)
@@ -92,24 +96,15 @@ func (e *Evaluator) Eval(ctx context.Context, pol map[string]any, res *engif.Res
 			return fmt.Errorf("failed to send package request: %w", err)
 		}
 
-		// TODO(jakub): implement comment. Policy only does nothing as the evaluator
-		// takes care of setting the policy to failed later.
-		if ruleConfig.Action == actionRejectPr {
-			reviewLoc, err := locateDepInPr(ctx, e.cli, dep)
-			if err != nil {
-				log.Printf("failed to locate dep in PR: %s", err)
-				continue
-			}
-
-			err = requestChanges(ctx, e.cli, dep.File.Name, prdeps.Pr,
-				reviewLoc, patch.IndentedString(reviewLoc.leadingWhitespace))
-			if err != nil {
-				log.Printf("failed to request changes on PR: %s", err)
-				continue
-			}
+		if err := prReplyHandler.trackVulnerableDep(ctx, dep, patch); err != nil {
+			return fmt.Errorf("failed to add package patch for further processing: %w", err)
 		}
 	}
 
+	if err := prReplyHandler.submit(ctx); err != nil {
+		return fmt.Errorf("failed to submit pr action: %w", err)
+	}
+
 	return evalErr
 }
 

pkg/controlplane/handlers_githubwebhooks.go

@@ -304,7 +304,7 @@ func parseGithubEventForProcessing(
 				context.Background(), ghclient.Github, store, payload, msg)
 		}
 	} else if hook_type == "pull_request" {
-		if payload["action"] == "opened" {
+		if payload["action"] == "opened" || payload["action"] == "synchronize" {
 			return parsePullRequestModEvent(
 				context.Background(), ghclient.Github, store, payload, msg)
 		}

pkg/providers/github/github.go

@@ -67,6 +67,9 @@ type RestAPI interface {
 	GetPackageVersionById(context.Context, bool, string, string, string, int64) (*github.PackageVersion, error)
 	GetPullRequest(context.Context, string, string, int) (*github.PullRequest, error)
 	CreateReview(context.Context, string, string, int, *github.PullRequestReviewRequest) (*github.PullRequestReview, error)
+	ListReviews(context.Context, string, string, int, *github.ListOptions) ([]*github.PullRequestReview, error)
+	DismissReview(context.Context, string, string, int, int64,
+		*github.PullRequestReviewDismissalRequest) (*github.PullRequestReview, error)
 	ListFiles(context.Context, string, string, int, int, int) ([]*github.CommitFile, error)
 	GetToken() string
 	GetOwner() string

pkg/providers/github/github_rest.go

@@ -306,6 +306,35 @@ func (c *RestClient) CreateReview(
 	return review, nil
 }
 
+// ListReviews is a wrapper for the GitHub API to list reviews
+func (c *RestClient) ListReviews(
+	ctx context.Context,
+	owner, repo string,
+	number int,
+	opt *github.ListOptions,
+) ([]*github.PullRequestReview, error) {
+	reviews, _, err := c.client.PullRequests.ListReviews(ctx, owner, repo, number, opt)
+	if err != nil {
+		return nil, fmt.Errorf("error listing reviews for PR %s/%s/%d: %w", owner, repo, number, err)
+	}
+	return reviews, nil
+}
+
+// DismissReview is a wrapper for the GitHub API to dismiss a review
+func (c *RestClient) DismissReview(
+	ctx context.Context,
+	owner, repo string,
+	prId int,
+	reviewId int64,
+	dismissalRequest *github.PullRequestReviewDismissalRequest,
+) (*github.PullRequestReview, error) {
+	review, _, err := c.client.PullRequests.DismissReview(ctx, owner, repo, prId, reviewId, dismissalRequest)
+	if err != nil {
+		return nil, fmt.Errorf("error dismissing review %d for PR %s/%s/%d: %w", reviewId, owner, repo, prId, err)
+	}
+	return review, nil
+}
+
 // GetRepository returns a single repository for the authenticated user
 func (c *RestClient) GetRepository(ctx context.Context, owner string, name string) (*github.Repository, error) {
 	// create a slice to hold the repositories