Implement invalidate_on_push without pushedDate field

README.md

@@ -31,7 +31,6 @@ UI to view the detailed approval status of any pull request.
     - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates)
     - [Cross-organization Membership Tests](#cross-organization-membership-tests)
     - [Update Merges](#update-merges)
-    - [Private Repositories](#private-repositories)
     - [Automatically Requesting Reviewers](#automatically-requesting-reviewers)
 * [Security](#security)
 * [Deployment](#deployment)
@@ -725,23 +724,6 @@ issue by using the `ignore_commits_by` option in combination with the
 [requirement on a protected branch]: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#require-status-checks-before-merging
 [commit-current-user-check]: https://github.com/github/platform-samples/blob/master/pre-receive-hooks/commit-current-user-check.sh
 
-### Invalidate On Push
-
-`policy-bot` can be configured to invalidate approvals when a commit is pushed using
-the `invalidate_on_push` option. This option is disabled by default.
-
-However, in recent versions of GitHub after [2023-07-01 breaking changes][], policy-bot
-is unable to load pushedDate for commits. This means that it is unable to determine
-whether a commit was pushed after the approval was granted.
-
-A server option (`do_not_load_commit_pushed_date`) is provided to toggle loading the push
-date for commits. If enabled, policy-bot will not be able to determine whether a commit
-was pushed after an approval was granted.
-
-Can also be configured by using the following env variable `POLICYBOT_OPTIONS_DO_NOT_LOAD_COMMIT_PUSHED_DATE`.
-
-[2023-07-01 breaking changes]: https://docs.github.com/en/graphql/overview/breaking-changes#changes-scheduled-for-2023-07-01
-
 ## Deployment
 
 `policy-bot` is easy to deploy in your own environment as it has no dependencies

policy/approval/approve.go

@@ -301,10 +301,6 @@ func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, c
 func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) {
 	log := zerolog.Ctx(ctx)
 
-	if !r.Options.InvalidateOnPush {
-		return candidates, nil, nil
-	}
-
 	commits, err := r.filteredCommits(ctx, prctx)
 	if err != nil {
 		return nil, nil, err
@@ -313,37 +309,61 @@ func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context,
 		return candidates, nil, nil
 	}
 
-	last := findLastPushed(commits)
-	if last == nil {
-		return nil, nil, errors.New("no commit contained a push date")
+	sha := commits[0].SHA
+	lastPushedAt, err := prctx.PushedAt(sha)
+	if err != nil {
+		return nil, nil, errors.Wrap(err, "failed to get last push timestamp")
+	}
+
+	// The most recent filtered commit did not have a status. This can happen
+	// in two situations:
+	//
+	// The commit was pushed in a batch where the head commit is ignored...
+	if lastPushedAt.IsZero() && sha != prctx.HeadSHA() {
+		sha = prctx.HeadSHA()
+		lastPushedAt, err = prctx.PushedAt(sha)
+		if err != nil {
+			return nil, nil, errors.Wrap(err, "failed to get last push timestamp")
+		}
+	}
+	// ... or the commit (or the head commit) hasn't been evaluated yet
+	//
+	// In this case, we're the first evaluation for this commit, so set the
+	// push time to the curent time, which is guaranteed to be after the actual
+	// push due to webhook delivery and processing time.
+	if lastPushedAt.IsZero() {
+		lastPushedAt = prctx.EvaluationTimestamp()
 	}
 
 	var allowed []*common.Candidate
 	var dismissed []*common.Dismissal
 	for _, c := range candidates {
-		if c.CreatedAt.After(*last.PushedAt) {
+		if c.CreatedAt.After(lastPushedAt) {
 			allowed = append(allowed, c)
 		} else {
 			dismissed = append(dismissed, &common.Dismissal{
 				Candidate: c,
-				Reason:    fmt.Sprintf("Invalidated by push of %.7s", last.SHA),
+				Reason:    fmt.Sprintf("Invalidated by push of %.7s", sha),
 			})
 		}
 	}
 
-	log.Debug().Msgf("discarded %d candidates invalidated by push of %s at %s",
-		len(dismissed),
-		last.SHA,
-		last.PushedAt.Format(time.RFC3339))
+	log.Debug().Msgf(
+		"discarded %d candidates invalidated by push of %s on or before %s",
+		len(dismissed), sha, lastPushedAt.Format(time.RFC3339),
+	)
 
 	return allowed, dismissed, nil
 }
 
+// filteredCommits returns the relevant commits for the evaluation ordered in
+// history order, from most to least recent.
 func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull.Commit, error) {
 	commits, err := prctx.Commits()
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to list commits")
 	}
+	commits = sortCommits(commits, prctx.HeadSHA())
 
 	ignoreUpdates := r.Options.IgnoreUpdateMerges
 	ignoreCommits := !r.Options.IgnoreCommitsBy.IsEmpty()
@@ -430,19 +450,32 @@ func isIgnoredCommit(ctx context.Context, prctx pull.Context, actors *common.Act
 	return len(c.Users()) > 0, nil
 }
 
-func findLastPushed(commits []*pull.Commit) *pull.Commit {
-	var last *pull.Commit
-	for _, c := range commits {
-		if c.PushedAt != nil && (last == nil || c.PushedAt.After(*last.PushedAt)) {
-			last = c
-		}
-	}
-	return last
-}
-
 func numberOfApprovals(count int) string {
 	if count == 1 {
 		return "1 approval"
 	}
 	return fmt.Sprintf("%d approvals", count)
 }
+
+// sortCommits orders commits in history order starting from head. It must be
+// called on the unfiltered set of commits.
+func sortCommits(commits []*pull.Commit, head string) []*pull.Commit {
+	commitsBySHA := make(map[string]*pull.Commit)
+	for _, c := range commits {
+		commitsBySHA[c.SHA] = c
+	}
+
+	ordered := make([]*pull.Commit, 0, len(commits))
+	for {
+		c, ok := commitsBySHA[head]
+		if !ok {
+			break
+		}
+		ordered = append(ordered, c)
+		if len(c.Parents) == 0 {
+			break
+		}
+		head = c.Parents[0]
+	}
+	return ordered
+}

policy/approval/approve_test.go

@@ -36,7 +36,8 @@ func TestIsApproved(t *testing.T) {
 	now := time.Now()
 	basePullContext := func() *pulltest.Context {
 		return &pulltest.Context{
-			AuthorValue: "mhaypenny",
+			EvaluationTimestampValue: now,
+			AuthorValue:              "mhaypenny",
 			BodyValue: &pull.Body{
 				Body:         "/no-platform",
 				CreatedAt:    now.Add(10 * time.Second),
@@ -110,24 +111,24 @@ func TestIsApproved(t *testing.T) {
 					Body:         "I LIKE THIS",
 				},
 			},
+			HeadSHAValue: "97d5ea26da319a987d80f6db0b7ef759f2f2e441",
 			CommitsValue: []*pull.Commit{
 				{
-					PushedAt:  newTime(now.Add(5 * time.Second)),
 					SHA:       "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
 					Author:    "mhaypenny",
 					Committer: "mhaypenny",
 				},
 				{
-					PushedAt:  newTime(now.Add(15 * time.Second)),
 					SHA:       "674832587eaaf416371b30f5bc5a47e377f534ec",
 					Author:    "contributor-author",
 					Committer: "mhaypenny",
+					Parents:   []string{"c6ade256ecfc755d8bc877ef22cc9e01745d46bb"},
 				},
 				{
-					PushedAt:  newTime(now.Add(45 * time.Second)),
 					SHA:       "97d5ea26da319a987d80f6db0b7ef759f2f2e441",
 					Author:    "mhaypenny",
 					Committer: "contributor-committer",
+					Parents:   []string{"674832587eaaf416371b30f5bc5a47e377f534ec"},
 				},
 			},
 			OrgMemberships: map[string][]string{
@@ -359,9 +360,38 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("invalidateCommentOnPush", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.PushedAtValue = map[string]time.Time{
+			"c6ade256ecfc755d8bc877ef22cc9e01745d46bb": now.Add(25 * time.Second),
+		}
+		prctx.HeadSHAValue = "c6ade256ecfc755d8bc877ef22cc9e01745d46bb"
+		prctx.CommitsValue = []*pull.Commit{
+			{
+				SHA:       "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
+				Author:    "mhaypenny",
+				Committer: "mhaypenny",
+			},
+		}
+
+		r := &Rule{
+			Requires: common.Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Users: []string{"comment-approver"},
+				},
+			},
+		}
+		assertApproved(t, prctx, r, "Approved by comment-approver")
+
+		r.Options.InvalidateOnPush = true
+		assertPending(t, prctx, r, "0/1 required approvals. Ignored 6 approvals from disqualified users")
+	})
+
+	t.Run("invalidateCommentOnPushFirstEvaluation", func(t *testing.T) {
+		prctx := basePullContext()
+		prctx.EvaluationTimestampValue = now.Add(25 * time.Second)
+		prctx.HeadSHAValue = "c6ade256ecfc755d8bc877ef22cc9e01745d46bb"
 		prctx.CommitsValue = []*pull.Commit{
 			{
-				PushedAt:  newTime(now.Add(25 * time.Second)),
 				SHA:       "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
 				Author:    "mhaypenny",
 				Committer: "mhaypenny",
@@ -384,9 +414,12 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("invalidateReviewOnPush", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.PushedAtValue = map[string]time.Time{
+			"c6ade256ecfc755d8bc877ef22cc9e01745d46bb": now.Add(85 * time.Second),
+		}
+		prctx.HeadSHAValue = "c6ade256ecfc755d8bc877ef22cc9e01745d46bb"
 		prctx.CommitsValue = []*pull.Commit{
 			{
-				PushedAt:  newTime(now.Add(85 * time.Second)),
 				SHA:       "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
 				Author:    "mhaypenny",
 				Committer: "mhaypenny",
@@ -409,8 +442,12 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.EvaluationTimestampValue = now.Add(25 * time.Second)
+		prctx.PushedAtValue = map[string]time.Time{
+			"c6ade256ecfc755d8bc877ef22cc9e01745d46bb": now,
+		}
+		prctx.HeadSHAValue = "647c5078288f0ea9de27b5c280f25edaf2089045"
 		prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{
-			PushedAt:        newTime(now.Add(25 * time.Second)),
 			SHA:             "647c5078288f0ea9de27b5c280f25edaf2089045",
 			CommittedViaWeb: true,
 			Parents: []string{
@@ -439,8 +476,11 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("ignoreUpdateMergeContributor", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.PushedAtValue = map[string]time.Time{
+			"c6ade256ecfc755d8bc877ef22cc9e01745d46bb": now.Add(25 * time.Second),
+		}
+		prctx.HeadSHAValue = "647c5078288f0ea9de27b5c280f25edaf2089045"
 		prctx.CommitsValue = append(prctx.CommitsValue[:1], &pull.Commit{
-			PushedAt:        newTime(now.Add(25 * time.Second)),
 			SHA:             "647c5078288f0ea9de27b5c280f25edaf2089045",
 			CommittedViaWeb: true,
 			Parents: []string{
@@ -471,6 +511,7 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("ignoreCommits", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.HeadSHAValue = "ea9be5fcd016dc41d70dc457dfee2e64a8f951c1"
 		prctx.CommitsValue = append(prctx.CommitsValue, &pull.Commit{
 			SHA:       "ea9be5fcd016dc41d70dc457dfee2e64a8f951c1",
 			Author:    "comment-approver",
@@ -514,9 +555,12 @@ func TestIsApproved(t *testing.T) {
 
 	t.Run("ignoreCommitsInvalidateOnPush", func(t *testing.T) {
 		prctx := basePullContext()
+		prctx.PushedAtValue = map[string]time.Time{
+			"c6ade256ecfc755d8bc877ef22cc9e01745d46bb": now.Add(25 * time.Second),
+		}
+		prctx.HeadSHAValue = "c6ade256ecfc755d8bc877ef22cc9e01745d46bb"
 		prctx.CommitsValue = []*pull.Commit{
 			{
-				PushedAt:  newTime(now.Add(25 * time.Second)),
 				SHA:       "c6ade256ecfc755d8bc877ef22cc9e01745d46bb",
 				Author:    "mhaypenny",
 				Committer: "mhaypenny",
@@ -703,6 +747,91 @@ func TestTrigger(t *testing.T) {
 	})
 }
 
+func TestSortCommits(t *testing.T) {
+	tests := map[string]struct {
+		Commits       []*pull.Commit
+		Head          string
+		ExpectedOrder []string
+	}{
+		"sorted": {
+			Commits: []*pull.Commit{
+				{SHA: "1", Parents: []string{"2"}},
+				{SHA: "2", Parents: []string{"3"}},
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "5"},
+			},
+			Head:          "1",
+			ExpectedOrder: []string{"1", "2", "3", "4", "5"},
+		},
+		"reverseSorted": {
+			Commits: []*pull.Commit{
+				{SHA: "5"},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "2", Parents: []string{"3"}},
+				{SHA: "1", Parents: []string{"2"}},
+			},
+			Head:          "1",
+			ExpectedOrder: []string{"1", "2", "3", "4", "5"},
+		},
+		"unsorted": {
+			Commits: []*pull.Commit{
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "1", Parents: []string{"2"}},
+				{SHA: "5"},
+				{SHA: "2", Parents: []string{"3"}},
+			},
+			Head:          "1",
+			ExpectedOrder: []string{"1", "2", "3", "4", "5"},
+		},
+		"partialOrder": {
+			Commits: []*pull.Commit{
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "1", Parents: []string{"2"}},
+				{SHA: "5"},
+				{SHA: "2", Parents: []string{"3"}},
+			},
+			Head:          "3",
+			ExpectedOrder: []string{"3", "4", "5"},
+		},
+		"independentHistory": {
+			Commits: []*pull.Commit{
+				{SHA: "1", Parents: []string{"2"}},
+				{SHA: "2"},
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "5"},
+			},
+			Head:          "1",
+			ExpectedOrder: []string{"1", "2"},
+		},
+		"missingHead": {
+			Commits: []*pull.Commit{
+				{SHA: "1", Parents: []string{"2"}},
+				{SHA: "2", Parents: []string{"3"}},
+				{SHA: "3", Parents: []string{"4"}},
+				{SHA: "4", Parents: []string{"5"}},
+				{SHA: "5"},
+			},
+			Head:          "42",
+			ExpectedOrder: nil,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			var actual []string
+			for _, c := range sortCommits(test.Commits, test.Head) {
+				actual = append(actual, c.SHA)
+			}
+			assert.Equal(t, test.ExpectedOrder, actual, "incorrect commit order")
+		})
+	}
+}
+
 func newTime(t time.Time) *time.Time {
 	return &t
 }