Make sure to dismiss reviews when comments are added

cmd/pr-bot/main.go

@@ -171,7 +171,7 @@ func downloadOCIArtifacts(svc *prbot.Service, cfg *prbot.Config) {
 func setupReviewer(svc *prbot.Service, cfg *prbot.Config, api gh.API) review.Reviewer {
 	log.Info().Msg("Setting up reviewer")
 	// mutex -> dedup -> precond -> rate limited -> reviewer
-	base := review.NewReviewer(api, svc.Metrics)
+	base := review.NewReviewer(api, svc.Metrics, cfg.GHE.ServiceAccount)
 	throttler := setupThrottlers(svc, cfg)
 	rateLimited := review.NewRateLimitedReviewer(base, api, throttler)
 	precond := review.NewPreCondValidationReviewer(rateLimited)

github/api.go

@@ -57,4 +57,5 @@ type API interface {
 	GetPullRequest(ctx context.Context, id id.PR) (*github.PullRequest, error)
 	GetRepository(ctx context.Context, id id.PR) (*github.Repository, error)
 	GetOrganization(ctx context.Context, id id.PR) (*github.Organization, error)
+	DismissReview(ctx context.Context, id id.PR, reviewID int64, message string) error
 }

github/github.go

@@ -247,6 +247,19 @@ func (gh *githubDao) GetOrganization(ctx context.Context, id id.PR) (*github.Org
 	return org, nil
 }
 
+// DismissReview implements API.
+func (gh *githubDao) DismissReview(ctx context.Context, id id.PR, reviewID int64, message string) error {
+	_, resp, err := gh.v3.PullRequests.DismissReview(ctx, id.Owner, id.Repo, id.Number, reviewID, 
+		&github.PullRequestReviewDismissalRequest{
+			Message: &message,
+		})
+	if err != nil {
+		return err
+	}
+	gh.emitTokenExpiration(ctx, resp)
+	return nil
+}
+
 func (gh *githubDao) emitTokenExpiration(ctx context.Context, resp *github.Response) {
 	if resp == nil {
 		return

pullrequest/review/reviewer.go

@@ -31,8 +31,9 @@ type Reviewer interface {
 }
 
 type reviewer struct {
-	api     gh.API
-	metrics metrics.Emitter
+	api            gh.API
+	metrics        metrics.Emitter
+	serviceAccount string
 }
 
 // Approve implements Reviewer.
@@ -63,7 +64,15 @@ func (r *reviewer) Approve(ctx context.Context, id id.PR, body string, opts Appr
 // Comment implements Reviewer.
 func (r *reviewer) Comment(ctx context.Context, id id.PR, body string) error {
 	oplog := httplog.LogEntry(ctx)
-	err := r.api.AddReview(ctx, id, body, gh.Comment)
+
+	// Dismiss any existing CHANGES_REQUESTED reviews before posting the comment
+	err := r.dismissChangesRequestedReviews(ctx, id)
+	if err != nil {
+		oplog.Err(err).Msgf("error dismissing CHANGES_REQUESTED reviews for PR %v", id.URL)
+		// Continue with comment even if dismissal fails
+	}
+
+	err = r.api.AddReview(ctx, id, body, gh.Comment)
 	if err != nil {
 		oplog.Err(err).Msgf("error reviewing PR with reviewType:comment %v", id.URL)
 		ae := pe.ServiceFault(ctx, "error reviewing PR with reviewType:comment", err)
@@ -94,8 +103,37 @@ func (r *reviewer) RequestChanges(ctx context.Context, id id.PR, body string) er
 	return nil
 }
 
-func NewReviewer(dao gh.API, metrics metrics.Emitter) Reviewer {
-	return &reviewer{api: dao, metrics: metrics}
+func NewReviewer(dao gh.API, metrics metrics.Emitter, serviceAccount string) Reviewer {
+	return &reviewer{api: dao, metrics: metrics, serviceAccount: serviceAccount}
+}
+
+// dismissChangesRequestedReviews dismisses any existing CHANGES_REQUESTED reviews from the service account on the PR
+func (r *reviewer) dismissChangesRequestedReviews(ctx context.Context, id id.PR) error {
+	oplog := httplog.LogEntry(ctx)
+
+	// Get all reviews for the PR
+	reviews, err := r.api.ListReviews(ctx, id)
+	if err != nil {
+		return fmt.Errorf("error listing reviews: %w", err)
+	}
+
+	// Find and dismiss any CHANGES_REQUESTED reviews from the service account
+	for _, review := range reviews {
+		if review.GetState() == "CHANGES_REQUESTED" && review.User.GetLogin() == r.serviceAccount {
+			oplog.Info().Msgf("dismissing CHANGES_REQUESTED review %d from %s for PR %v", review.GetID(), r.serviceAccount, id.URL)
+
+			dismissMessage := "Review dismissed due to follow-up comment"
+			err := r.api.DismissReview(ctx, id, review.GetID(), dismissMessage)
+			if err != nil {
+				oplog.Err(err).Msgf("error dismissing review %d for PR %v", review.GetID(), id.URL)
+				// Continue with other reviews even if one fails
+			} else {
+				oplog.Info().Msgf("successfully dismissed review %d from %s for PR %v", review.GetID(), r.serviceAccount, id.URL)
+			}
+		}
+	}
+
+	return nil
 }
 
 func (r *reviewer) handleAutoMergeError(ctx context.Context, id id.PR, err error) error {

pullrequest/review/reviewer_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/google/go-github/v50/github"
 	gh "github.com/marqeta/pr-bot/github"
 	"github.com/marqeta/pr-bot/id"
 	"github.com/marqeta/pr-bot/metrics"
@@ -165,7 +166,7 @@ func Test_reviewer_Approve(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			mockAPI := gh.NewMockAPI(t)
 			metrics := metrics.NewNoopEmitter()
-			r := review.NewReviewer(mockAPI, metrics)
+			r := review.NewReviewer(mockAPI, metrics, "test-service-account")
 			tt.args.setExpectations(mockAPI)
 			if err := r.Approve(ctx, tt.args.id, tt.args.body, review.ApproveOptions{
 				MergeMethod: tt.args.mergeMethod,
@@ -197,6 +198,7 @@ func Test_reviewer_Comment(t *testing.T) {
 				id:   sampleID(),
 				body: "random body",
 				setExpectations: func(d *gh.MockAPI) {
+					d.EXPECT().ListReviews(ctx, sampleID()).Return([]*github.PullRequestReview{}, nil)
 					d.EXPECT().AddReview(ctx, sampleID(), "random body", gh.Comment).
 						Return(nil)
 				},
@@ -209,6 +211,7 @@ func Test_reviewer_Comment(t *testing.T) {
 				id:   sampleID(),
 				body: "random body",
 				setExpectations: func(d *gh.MockAPI) {
+					d.EXPECT().ListReviews(ctx, sampleID()).Return([]*github.PullRequestReview{}, nil)
 					d.EXPECT().AddReview(ctx, sampleID(), "random body", gh.Comment).
 						Return(errRandom).Once()
 				},
@@ -220,7 +223,7 @@ func Test_reviewer_Comment(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			mockAPI := gh.NewMockAPI(t)
 			metrics := metrics.NewNoopEmitter()
-			r := review.NewReviewer(mockAPI, metrics)
+			r := review.NewReviewer(mockAPI, metrics, "test-service-account")
 			tt.args.setExpectations(mockAPI)
 			if err := r.Comment(ctx, tt.args.id, tt.args.body); (err != nil) != tt.wantErr {
 				t.Errorf("reviewer.Comment() error = %v, wantErr %v", err, tt.wantErr)
@@ -273,7 +276,7 @@ func Test_reviewer_RequestChanges(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			mockAPI := gh.NewMockAPI(t)
 			metrics := metrics.NewNoopEmitter()
-			r := review.NewReviewer(mockAPI, metrics)
+			r := review.NewReviewer(mockAPI, metrics, "test-service-account")
 			tt.args.setExpectations(mockAPI)
 			if err := r.RequestChanges(ctx, tt.args.id, tt.args.body); (err != nil) != tt.wantErr {
 				t.Errorf("reviewer.RequestChanges() error = %v, wantErr %v", err, tt.wantErr)
@@ -282,6 +285,106 @@ func Test_reviewer_RequestChanges(t *testing.T) {
 	}
 }
 
+func Test_reviewer_Comment_WithDismissChangesRequested(t *testing.T) {
+	ctx := context.Background()
+	//nolint:goerr113
+	errRandom := errors.New("random error")
+
+	// Create a sample review that should be dismissed
+	changesRequestedReview := &github.PullRequestReview{
+		ID:    github.Int64(123),
+		State: github.String("CHANGES_REQUESTED"),
+		User: &github.User{
+			Login: github.String("test-service-account"),
+		},
+	}
+
+	// Create a review from a different user that should not be dismissed
+	otherUserReview := &github.PullRequestReview{
+		ID:    github.Int64(456),
+		State: github.String("CHANGES_REQUESTED"),
+		User: &github.User{
+			Login: github.String("other-user"),
+		},
+	}
+
+	// Create a review from service account but different state
+	approvedReview := &github.PullRequestReview{
+		ID:    github.Int64(789),
+		State: github.String("APPROVED"),
+		User: &github.User{
+			Login: github.String("test-service-account"),
+		},
+	}
+
+	type args struct {
+		id              id.PR
+		body            string
+		setExpectations func(d *gh.MockAPI)
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "Should dismiss CHANGES_REQUESTED reviews from service account before commenting",
+			args: args{
+				id:   sampleID(),
+				body: "random body",
+				setExpectations: func(d *gh.MockAPI) {
+					d.EXPECT().ListReviews(ctx, sampleID()).Return([]*github.PullRequestReview{
+						changesRequestedReview,
+						otherUserReview,
+						approvedReview,
+					}, nil)
+					d.EXPECT().DismissReview(ctx, sampleID(), int64(123), "Review dismissed due to follow-up comment").Return(nil)
+					d.EXPECT().AddReview(ctx, sampleID(), "random body", gh.Comment).Return(nil)
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "Should continue with comment even if dismissal fails",
+			args: args{
+				id:   sampleID(),
+				body: "random body",
+				setExpectations: func(d *gh.MockAPI) {
+					d.EXPECT().ListReviews(ctx, sampleID()).Return([]*github.PullRequestReview{
+						changesRequestedReview,
+					}, nil)
+					d.EXPECT().DismissReview(ctx, sampleID(), int64(123), "Review dismissed due to follow-up comment").Return(errRandom)
+					d.EXPECT().AddReview(ctx, sampleID(), "random body", gh.Comment).Return(nil)
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "Should handle error when ListReviews fails",
+			args: args{
+				id:   sampleID(),
+				body: "random body",
+				setExpectations: func(d *gh.MockAPI) {
+					d.EXPECT().ListReviews(ctx, sampleID()).Return(nil, errRandom)
+					d.EXPECT().AddReview(ctx, sampleID(), "random body", gh.Comment).Return(nil)
+				},
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockAPI := gh.NewMockAPI(t)
+			metrics := metrics.NewNoopEmitter()
+			r := review.NewReviewer(mockAPI, metrics, "test-service-account")
+			tt.args.setExpectations(mockAPI)
+			if err := r.Comment(ctx, tt.args.id, tt.args.body); (err != nil) != tt.wantErr {
+				t.Errorf("reviewer.Comment() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
 func sampleID() id.PR {
 
 	return id.PR{