diff --git a/.github/workflows/format-vote-gate.yml b/.github/workflows/format-vote-gate.yml index d120057ac18..f771948636b 100644 --- a/.github/workflows/format-vote-gate.yml +++ b/.github/workflows/format-vote-gate.yml @@ -6,9 +6,19 @@ name: Format spec vote gate # the persisted format protos or `docs/src/format/**`; execution-only wire # schemas are excluded. This gate reads that label and blocks merging until the # PR has 3 binding +1 votes from PMC members (PR approvals, excluding the -# author), has no outstanding veto (a PMC "Request changes" review), and the -# 72-hour voting period has elapsed. That period starts once the PR is labeled -# and ready for review, and pauses over weekends. +# author) with at least one of them on the head commit, has no outstanding veto +# (a PMC "Request changes" review), and the 72-hour voting period has elapsed. +# That period starts once the PR is labeled and ready for review, and pauses +# over weekends. Approvals carry over across pushes; the member who approves the +# head commit is vouching that nothing substantive changed since the others +# voted. +# +# That carry-over depends on a branch-protection setting the gate cannot read +# with its token: turning on "Dismiss stale pull request approvals when new +# commits are pushed" makes GitHub report the earlier approvals as DISMISSED, +# which this gate does not count. The gate then quietly reverts to requiring all +# 3 approvals on the head commit — it fails closed, but anyone wondering why +# should look there first. # # The gate publishes its verdict as the `format-spec-vote` commit status on the # PR head. To make it a merge blocker, an org admin must add `format-spec-vote` diff --git a/ci/format_vote_gate.py b/ci/format_vote_gate.py index f4ed1a972d0..b80f20c0b35 100644 --- a/ci/format_vote_gate.py +++ b/ci/format_vote_gate.py @@ -5,8 +5,10 @@ applied by the path labeler (`.github/labeler-area.yml`); this script reads it and publishes the `format-spec-vote` commit status, which blocks merging until: - * 3 PMC members have approved the PR (excluding the author), counted only on - the head commit so new pushes invalidate stale approvals; + * 3 PMC members have approved the PR (excluding the author), counted on any + commit so a rebase or a typo fix doesn't send everyone back to re-vote; + * at least one of those approvals is on the head commit. That member is + vouching that nothing substantive changed since the earlier approvals; * no PMC member has an outstanding "Request changes" review (a veto); and * the 72-hour voting period has elapsed. The clock starts once the PR is both labeled and out of draft, and pauses over weekends. @@ -56,9 +58,11 @@ def tally_reviews(reviews, head_sha, author, is_pmc): """Tally PMC votes from a PR's reviews. `reviews` is an ordered list of dicts with `login`, `state`, `commit_id`. - A member's stance is their most recent stance review. Approvals only count - on the head commit; earlier ones are stale. A "changes requested" review is - a veto regardless of commit. The PR author never counts. + A member's stance is their most recent stance review. Approvals count + whatever commit they were cast on; the ones on the head commit are returned + separately, because the gate requires at least one of those. A "changes + requested" review is a veto regardless of commit. The PR author never + counts. """ latest = {} for review in reviews: @@ -69,22 +73,27 @@ def tally_reviews(reviews, head_sha, author, is_pmc): continue latest[login.lower()] = review - approvals, stale_approvals, vetoes = [], [], [] + approvals, head_approvals, vetoes = [], [], [] for review in latest.values(): if review["state"] == "APPROVED": - target = approvals if review["commit_id"] == head_sha else stale_approvals - target.append(review["login"]) + approvals.append(review["login"]) + if review["commit_id"] == head_sha: + head_approvals.append(review["login"]) elif review["state"] == "CHANGES_REQUESTED": vetoes.append(review["login"]) - return approvals, stale_approvals, vetoes + return approvals, head_approvals, vetoes -def decide_verdict(veto_count, approval_count, period_elapsed, required): +def decide_verdict( + veto_count, approval_count, head_approval_count, period_elapsed, required +): """Return the blocking condition (if any), in priority order.""" if veto_count > 0: return "veto" if approval_count < required: return "insufficient" + if head_approval_count == 0: + return "unconfirmed" if not period_elapsed: return "waiting_period" return "pass" @@ -153,7 +162,9 @@ def _as_utc(dt): return dt.replace(tzinfo=timezone.utc) if dt.tzinfo is None else dt -def _build_comment(headline, approval_cell, vetoes, period_cell, rerun_url): +def _build_comment( + headline, approval_cell, head_approval_cell, vetoes, period_cell, rerun_url +): return "\n".join( [ COMMENT_MARKER, @@ -162,17 +173,23 @@ def _build_comment(headline, approval_cell, vetoes, period_cell, rerun_url): "", "This PR modifies the Lance format specification, so it requires " f"**{REQUIRED_APPROVALS} binding +1 votes from PMC members** " - "(excluding the proposer) and a minimum " - f"**{PERIOD_HOURS}-hour** voting period, weekends excluded, before " - "it can merge. " + "(excluding the proposer), **at least one of them on the latest " + f"commit**, and a minimum **{PERIOD_HOURS}-hour** voting period, " + "weekends excluded, before it can merge. " "Vote by approving this PR (+1) or requesting changes (−1, a veto). " f"See the [voting process]({VOTING_URL}).", "", + "Approvals carry over across pushes, so a rebase or a typo fix does " + "not send everyone back to re-vote. Whoever approves the latest " + "commit is vouching that nothing substantive has changed since the " + "earlier approvals; if something has, ask for fresh votes.", + "", f"**Status: {headline}**", "", "| | |", "|---|---|", - f"| Approvals (this commit) | {approval_cell} |", + f"| Approvals | {approval_cell} |", + f"| Latest commit approved by | {head_approval_cell} |", f"| Vetoes | {_fmt_list(vetoes)} |", f"| Voting period | {period_cell} |", "", @@ -286,7 +303,7 @@ def evaluate(self, number): } for review in pr.get_reviews() ] - approvals, stale, vetoes = tally_reviews( + approvals, head_approvals, vetoes = tally_reviews( reviews, head_sha, pr.user.login, self.is_pmc ) @@ -299,7 +316,11 @@ def evaluate(self, number): period_ends = weekday_deadline(opened_at or now, PERIOD_HOURS) period_elapsed = now >= period_ends verdict = decide_verdict( - len(vetoes), len(approvals), period_elapsed, REQUIRED_APPROVALS + len(vetoes), + len(approvals), + len(head_approvals), + period_elapsed, + REQUIRED_APPROVALS, ) deadline = _fmt_deadline(period_ends) @@ -308,10 +329,15 @@ def evaluate(self, number): headline = f"❌ Blocked — vetoed by {_fmt_list(vetoes)}" elif verdict == "insufficient": state = "failure" - summary = ( - f"{len(approvals)}/{REQUIRED_APPROVALS} PMC approvals on this commit." - ) + summary = f"{len(approvals)}/{REQUIRED_APPROVALS} PMC approvals." headline = f"❌ Blocked — {len(approvals)} of {REQUIRED_APPROVALS} required approvals" + elif verdict == "unconfirmed": + state = "failure" + summary = f"{len(approvals)} PMC approvals, but none on the latest commit." + headline = ( + f"❌ Blocked — {len(approvals)} approvals, but none on the latest " + "commit; one PMC member must approve it" + ) elif verdict == "waiting_period": state, summary = "failure", f"Approved; voting period ends {deadline}." headline = ( @@ -329,14 +355,22 @@ def evaluate(self, number): approval_cell = ( f"{_fmt_list(approvals)} ({len(approvals)}/{REQUIRED_APPROVALS})" ) - if stale: - approval_cell += f" — stale, re-approve needed: {_fmt_list(stale)}" + head_approval_cell = ( + _fmt_list(head_approvals) + if head_approvals + else "none — one PMC member must approve the latest commit" + ) self.set_status(head_sha, state, summary) self.upsert_comment( issue, _build_comment( - headline, approval_cell, vetoes, period_cell, self.rerun_url + headline, + approval_cell, + head_approval_cell, + vetoes, + period_cell, + self.rerun_url, ), ) print(f"PR #{number}: {summary}") diff --git a/ci/test_format_vote_gate.py b/ci/test_format_vote_gate.py index 2186b5deacd..944b5053d93 100644 --- a/ci/test_format_vote_gate.py +++ b/ci/test_format_vote_gate.py @@ -28,7 +28,7 @@ def review(login, state, commit_id=HEAD): def test_counts_distinct_pmc_approvals_on_head_commit(): - approvals, stale, vetoes = tally_reviews( + approvals, head_approvals, vetoes = tally_reviews( [ review("alice", "APPROVED"), review("bob", "APPROVED"), @@ -39,7 +39,7 @@ def test_counts_distinct_pmc_approvals_on_head_commit(): is_pmc, ) assert sorted(approvals) == ["alice", "bob", "carol"] - assert stale == [] + assert sorted(head_approvals) == ["alice", "bob", "carol"] assert vetoes == [] @@ -55,15 +55,28 @@ def test_only_latest_review_per_member_counts(): assert vetoes == ["alice"] -def test_approvals_on_earlier_commit_are_stale(): - approvals, stale, _ = tally_reviews( +def test_approvals_on_earlier_commits_still_count(): + # A push doesn't send earlier voters back to re-vote; only the head-commit + # approval is tracked separately, because the gate requires one of those. + approvals, head_approvals, _ = tally_reviews( [review("alice", "APPROVED", "old_sha"), review("bob", "APPROVED")], HEAD, "author", is_pmc, ) - assert approvals == ["bob"] - assert stale == ["alice"] + assert sorted(approvals) == ["alice", "bob"] + assert head_approvals == ["bob"] + + +def test_no_head_approval_when_every_vote_predates_the_push(): + approvals, head_approvals, _ = tally_reviews( + [review("alice", "APPROVED", "old_sha"), review("bob", "APPROVED", "older")], + HEAD, + "author", + is_pmc, + ) + assert sorted(approvals) == ["alice", "bob"] + assert head_approvals == [] def test_ignores_author_non_pmc_and_dismissed(): @@ -83,16 +96,26 @@ def test_ignores_author_non_pmc_and_dismissed(): @pytest.mark.parametrize( - ("veto_count", "approval_count", "period_elapsed", "expected"), + ("veto_count", "approvals", "head_approvals", "period_elapsed", "expected"), [ - (1, 5, True, "veto"), # veto wins even with enough approvals + elapsed - (0, 2, True, "insufficient"), - (0, 3, False, "waiting_period"), - (0, 3, True, "pass"), + # A veto wins even with enough approvals, one on head, and time elapsed. + (1, 5, 1, True, "veto"), + (0, 2, 1, True, "insufficient"), + # Enough votes carried over, but nobody has approved the latest commit. + (0, 3, 0, True, "unconfirmed"), + (0, 3, 1, False, "waiting_period"), + (0, 3, 1, True, "pass"), + # The head-commit approval counts toward the three; it is not a fourth. + (0, 3, 3, True, "pass"), ], ) -def test_decide_verdict_priority(veto_count, approval_count, period_elapsed, expected): - assert decide_verdict(veto_count, approval_count, period_elapsed, 3) == expected +def test_decide_verdict_priority( + veto_count, approvals, head_approvals, period_elapsed, expected +): + assert ( + decide_verdict(veto_count, approvals, head_approvals, period_elapsed, 3) + == expected + ) def utc(text): diff --git a/docs/src/community/voting.md b/docs/src/community/voting.md index 0a2218752c3..59a9133b75f 100644 --- a/docs/src/community/voting.md +++ b/docs/src/community/voting.md @@ -90,9 +90,14 @@ such PRs are labeled `format-change` automatically. The blocks merging a `format-change` PR until all of the following hold: - **Three binding +1 votes.** Three PMC members have approved the PR, excluding - the proposer. Cast +1 by approving the PR. Only approvals on the latest commit - count — pushing new commits invalidates earlier approvals, since the proposal - has changed. + the proposer. Cast +1 by approving the PR. An approval counts no matter what commit + it was cast on, so a rebase or a typo fix does not send everyone back to + re-vote. +- **One +1 on the latest commit.** At least one of those approvals — from a PMC + member who is not the proposer — must be on the latest commit. That member is + vouching that nothing substantive has changed since the earlier approvals; if + something has, they should ask the other voters for fresh votes rather than + approving. This approval counts toward the three; it is not a fourth vote. - **No veto.** No PMC member has an outstanding "Request changes" review. A `-1` binding vote (cast by requesting changes) is a veto and blocks the merge until withdrawn.