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",

pull/context.go

@@ -44,6 +44,11 @@ type MembershipContext interface {
 type Context interface {
 	MembershipContext
 
+	// EvaluationTimestamp returns the time at the start of the pull request
+	// evaluation, usually the creation time of the context. All calls on the
+	// same context should return the same value.
+	EvaluationTimestamp() time.Time
+
 	// RepositoryOwner returns the owner of the repo that the pull request targets.
 	RepositoryOwner() string
 
@@ -87,6 +92,11 @@ type Context interface {
 	// commit order is implementation dependent.
 	Commits() ([]*Commit, error)
 
+	// PushedAt returns the time at which the commit with sha was pushed. The
+	// returned time may be after the actual push time, but must not be before.
+	// PushedAt returns the zero time if the push time for is unknown.
+	PushedAt(sha string) (time.Time, error)
+
 	// Comments lists all comments on a Pull Request. The comment order is
 	// implementation dependent.
 	Comments() ([]*Comment, error)
@@ -147,10 +157,6 @@ type Commit struct {
 	// committer is not a real user.
 	Committer string
 
-	// PushedAt is the timestamp when the commit was pushed. It is nil if that
-	// information is not available for this commit.
-	PushedAt *time.Time
-
 	// Signature is the signature and details that was extracted from the commit.
 	// It is nil if the commit has no signature
 	Signature *Signature