From 956608d12b0d6381960ddd675196bdfbb1828b8f Mon Sep 17 00:00:00 2001 From: Jahon A Date: Mon, 22 Sep 2025 16:25:11 -0700 Subject: [PATCH] Make sure to dismiss reviews when comments are added --- cmd/pr-bot/main.go | 2 +- github/api.go | 1 + github/github.go | 13 +++ github/mock_api.go | 49 ++++++++++ pullrequest/review/reviewer.go | 51 +++++++++-- pullrequest/review/reviewer_test.go | 135 ++++++++++++++++++++++------ 6 files changed, 214 insertions(+), 37 deletions(-) diff --git a/cmd/pr-bot/main.go b/cmd/pr-bot/main.go index 1d97d9fb..50ea333f 100644 --- a/cmd/pr-bot/main.go +++ b/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) diff --git a/github/api.go b/github/api.go index 1273a784..def76bea 100644 --- a/github/api.go +++ b/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 } diff --git a/github/github.go b/github/github.go index bb5154be..4762281f 100644 --- a/github/github.go +++ b/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 diff --git a/github/mock_api.go b/github/mock_api.go index 067de228..cee73515 100644 --- a/github/mock_api.go +++ b/github/mock_api.go @@ -814,6 +814,55 @@ func (_c *MockAPI_ListReviews_Call) RunAndReturn(run func(context.Context, id.PR return _c } +// DismissReview provides a mock function with given fields: ctx, _a1, reviewID, message +func (_m *MockAPI) DismissReview(ctx context.Context, _a1 id.PR, reviewID int64, message string) error { + ret := _m.Called(ctx, _a1, reviewID, message) + + if len(ret) == 0 { + panic("no return value specified for DismissReview") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, id.PR, int64, string) error); ok { + r0 = rf(ctx, _a1, reviewID, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MockAPI_DismissReview_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DismissReview' +type MockAPI_DismissReview_Call struct { + *mock.Call +} + +// DismissReview is a helper method to define mock.On call +// - ctx context.Context +// - _a1 id.PR +// - reviewID int64 +// - message string +func (_e *MockAPI_Expecter) DismissReview(ctx interface{}, _a1 interface{}, reviewID interface{}, message interface{}) *MockAPI_DismissReview_Call { + return &MockAPI_DismissReview_Call{Call: _e.mock.On("DismissReview", ctx, _a1, reviewID, message)} +} + +func (_c *MockAPI_DismissReview_Call) Run(run func(ctx context.Context, _a1 id.PR, reviewID int64, message string)) *MockAPI_DismissReview_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(id.PR), args[2].(int64), args[3].(string)) + }) + return _c +} + +func (_c *MockAPI_DismissReview_Call) Return(_a0 error) *MockAPI_DismissReview_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockAPI_DismissReview_Call) RunAndReturn(run func(context.Context, id.PR, int64, string) error) *MockAPI_DismissReview_Call { + _c.Call.Return(run) + return _c +} + // NewMockAPI creates a new instance of MockAPI. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewMockAPI(t interface { diff --git a/pullrequest/review/reviewer.go b/pullrequest/review/reviewer.go index b97c1257..b05d4849 100644 --- a/pullrequest/review/reviewer.go +++ b/pullrequest/review/reviewer.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "strings" - "time" "github.com/go-chi/httplog" pe "github.com/marqeta/pr-bot/errors" @@ -32,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. @@ -45,7 +45,7 @@ func (r *reviewer) Approve(ctx context.Context, id id.PR, body string, opts Appr return r.handleAutoMergeError(ctx, id, err) } oplog.Info().Msgf("enabled auto merge on PR") - time.Sleep(5 * time.Second) + err = r.api.AddReview(ctx, id, body, gh.Approve) if err != nil { oplog.Err(err).Msgf("error approving PR") @@ -64,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) @@ -95,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 { diff --git a/pullrequest/review/reviewer_test.go b/pullrequest/review/reviewer_test.go index 6ee76e1f..1a468cc5 100644 --- a/pullrequest/review/reviewer_test.go +++ b/pullrequest/review/reviewer_test.go @@ -4,8 +4,8 @@ import ( "context" "errors" "testing" - "time" + "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" @@ -166,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, @@ -177,31 +177,6 @@ func Test_reviewer_Approve(t *testing.T) { } } -func Test_reviewer_Approve_Sleep(t *testing.T) { - ctx := context.Background() - mockAPI := gh.NewMockAPI(t) - metrics := metrics.NewNoopEmitter() - r := review.NewReviewer(mockAPI, metrics) - - // Set up expectations - mockAPI.EXPECT().EnableAutoMerge(ctx, sampleID(), githubv4.PullRequestMergeMethodMerge).Return(nil) - mockAPI.EXPECT().AddReview(ctx, sampleID(), "LGTM", gh.Approve).Return(nil) - - // Measure the time to verify sleep - start := time.Now() - err := r.Approve(ctx, sampleID(), "LGTM", review.ApproveOptions{ - MergeMethod: githubv4.PullRequestMergeMethodMerge, - }) - duration := time.Since(start) - - // Verify no error and sleep duration is at least 1 second - if err != nil { - t.Errorf("Approve() error = %v, want nil", err) - } - if duration < 5 * time.Second { - t.Errorf("Sleep duration = %v, want at least 1s", duration) - } -} func Test_reviewer_Comment(t *testing.T) { ctx := context.Background() @@ -224,6 +199,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) }, @@ -236,6 +212,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() }, @@ -247,7 +224,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) @@ -300,7 +277,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) @@ -309,6 +286,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{