From f4c42aab04f22fed593878a28c98f8bcd2c9adb8 Mon Sep 17 00:00:00 2001 From: argszero Date: Wed, 28 Oct 2026 22:30:46 +0800 Subject: [PATCH] emrg: a landed head's votes are read, not just promised check-vote-count.py's merged and closed lines have always ended "the votes below are the history of how it was reviewed", and printed nothing under them: check_pr's terminal return builds a Verdict and never asks for the reviews, so votes was empty by construction while the print site's own comment states the opposite intent. Both readings now share one review reader, and a landed head's votes are reported as history: each line carries the review's own instant, its mark, and the cycle id it names - or the reason there is no single one. The live clock is deliberately not asked, because a terminal PR's push_time is the merge instant and reusing the gate would stamp every review of a correctly merged PR as "submitted before the head push". --- scripts/check-vote-count.py | 110 ++++++++++++++++++++++++++++----- tests/test_check_vote_count.py | 98 +++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 15 deletions(-) diff --git a/scripts/check-vote-count.py b/scripts/check-vote-count.py index ae9c8673..6042746e 100644 --- a/scripts/check-vote-count.py +++ b/scripts/check-vote-count.py @@ -1857,6 +1857,78 @@ def _view_with_computed_mergeability(number: int, wait: float) -> dict: time.sleep(min(_MERGEABILITY_POLL_SECONDS, remaining)) +def _review_rows(number: int) -> list[dict]: + """Every review on this PR, oldest first, in the projected shape. + + **Every page, not the first 30**: a truncated list drops the newest reviews, which + are exactly the votes that count (and the endpoint orders oldest first, so the loss + is invisible in the output - it just looks short). + + One reader for both the live and the landed path, because the two readings ask the + same question of the same endpoint and would otherwise drift: the merged-PR line has + always promised its votes ("the votes below are the history of how it was reviewed") + while `check_pr`'s terminal return built a `Verdict` without ever asking for them, so + the promise pointed at an empty list. Measured 2026-10-28 on #2626 and #2624. + """ + reviews = _gh_json_paginated( + [ + "api", + f"repos/{REPO}/pulls/{number}/reviews", + "--jq", + ".[] | {at: .submitted_at, body: .body, author: .user.login}", + ] + ) + reviews.sort(key=lambda r: str(r.get("at") or "")) + return reviews + + +def _history_votes(number: int, reviews: list[dict]) -> list[Vote]: + """The votes on a head that has already landed, read as history rather than a count. + + Two things make this a different reading from the live one, and collapsing them + would misreport a landed PR: + + * **The clock is not asked.** `own_head_window` answers "would this vote still count + against that head", and a landed head has no head to count against. Asking it is + worse than skipping it, because a terminal PR's `push_time` is the *merge* instant: + every review predates it, so the whole history would come back voided as "submitted + before the head push" - a wrong reading of a correct fact. + * **`counted` is not filled**, because nothing is being counted. The line prints what + the review said, which is the fact a reader comes to this report for. + + The two reasons a vote is unusable *regardless of state* are still reported, because + they are about the body and not about time: a body naming no cycle id, and one naming + several (which cycle wrote it cannot be measured, so it credits none of them). + """ + votes: list[Vote] = [] + for r in reviews: + assert isinstance(r, dict) + # The same refusal as the live path: without the projection `at` reads as "", + # and an undated history is a history a reader cannot place against anything. + at = str(r.get("at") or "") + if not at: + raise RuntimeError( + f"review payload for #{number} has no `at` field (keys: " + f"{sorted(r)[:5]}) - the --jq projection did not apply, so vote " + "times are unknown; refusing to report a history" + ) + kind = classify(str(r.get("body") or "")) + if kind == "comment": + continue + ids = distinct_cycle_ids(str(r.get("body") or "")) + cycle = ids[0] if len(ids) == 1 else None + if len(ids) > 1: + votes.append(Vote(at, kind, None, False, + f"the vote body names {len(ids)} cycle ids ({', '.join(ids)}) - which " + "cycle wrote it cannot be measured, so it credits none of them", + tuple(ids))) + elif cycle is None: + votes.append(Vote(at, kind, None, False, "no cycle id in the vote body")) + else: + votes.append(Vote(at, kind, cycle, True, "", tuple(ids))) + return votes + + def check_pr( number: int, needed: int, @@ -1886,6 +1958,11 @@ def check_pr( merge_state=str(view.get("mergeStateStatus") or ""), state=state, merged_at=str(view.get("mergedAt") or ""), + # Asked here, not left empty: this branch returns before the live path + # reaches its review fetch, and the report's own merged-PR line promises + # the history. One `gh` call, and only for a head someone asks about after + # it landed - the open path's read costs are untouched. + votes=_history_votes(number, _review_rows(number)), ) mergeable, merge_state = _merge_state(view) @@ -1926,18 +2003,7 @@ def check_pr( checks, checks_truncated = _head_check_runs(head) checks_read = True - # Every page, not the first 30: a truncated list drops the newest reviews, - # which are exactly the votes that count (and the endpoint orders oldest - # first, so the loss is invisible in the output - it just looks short). - reviews = _gh_json_paginated( - [ - "api", - f"repos/{REPO}/pulls/{number}/reviews", - "--jq", - ".[] | {at: .submitted_at, body: .body, author: .user.login}", - ] - ) - reviews.sort(key=lambda r: str(r.get("at") or "")) + reviews = _review_rows(number) votes: list[Vote] = [] # One resolve per cycle, not per vote: the window is read from the cycle records @@ -2199,7 +2265,8 @@ def main(argv: list[str] | None = None) -> int: print( " merged PR: this head has landed, so there is nothing here " "to vote on, merge or refresh - the votes below are the history " - "of how it was reviewed" + "of how it was reviewed, and the clock that voids a vote is not " + "asked of a landed head" ) else: print( @@ -2207,8 +2274,21 @@ def main(argv: list[str] | None = None) -> int: "nothing here to vote on or merge - the votes below are its " "history" ) - for index, vote in enumerate(v.votes): - print(f" {vote.at} {_cycle_label(vote)} - {vote.why}") + # History reads differently from a live count on purpose: no "counts" + # column, because nothing is counted against a head that has landed, + # and what the review *said* is the fact worth printing. The two marks + # are the live columns' for a veto ("NO") and an approval ("OK"); a + # vote that cannot be attributed to a cycle keeps "VOID", because that + # is a fact about the body and not about the head having landed. + for vote in v.votes: + if vote.kind == "veto": + mark = "NO " + note = "the review objected" if vote.valid else vote.why + elif vote.valid: + mark, note = "OK ", "the review approved" + else: + mark, note = "VOID", vote.why + print(f" {vote.at} {mark} {_cycle_label(vote)} - {note}") continue # A missing run is both a caveat about the count *and* the CI conjunct # unverified, so the line says which: `mark` is already BLOCKED here, diff --git a/tests/test_check_vote_count.py b/tests/test_check_vote_count.py index dbbf40df..9a18a544 100644 --- a/tests/test_check_vote_count.py +++ b/tests/test_check_vote_count.py @@ -719,6 +719,104 @@ def test_a_closed_unmerged_pr_reads_as_closed_and_not_as_merged(mod, monkeypatch assert "closed without merging" in out +def test_a_merged_pr_prints_the_vote_history_its_line_promises(mod, monkeypatch, capsys): + """Issue #2627. The merged line promised the votes and printed none. + + Measured 2026-10-28 on the live repo: #2626 and #2624 both reported + "the votes below are the history of how it was reviewed" with nothing below it, + although each carried three approving reviews naming three different cycles. The + cause was structural rather than a bad payload - `check_pr`'s terminal return builds + a `Verdict` and never asks for the reviews, so `votes` was empty by construction, + while the print site's own comment says the opposite. + + Both halves are asserted: the history is printed, and each line carries the review's + **own** instant, which is what lets a reader check a landed head against the + approvals it needed (three from three cycles, for a release PR, is a rule that can + only be checked after the merge). + """ + fake = FakeGh(_three_votes(), state="MERGED", merged_at="2026-10-03T14:46:59Z", + mergeable="UNKNOWN", merge_state="UNKNOWN") + rc = _run(mod, monkeypatch, fake) + out = capsys.readouterr().out + assert rc == 0 + for cycle, at in ( + ("cyc20260911-090000", "2026-09-11T01:00:00Z"), + ("cyc20260911-100000", "2026-09-11T02:00:00Z"), + ("cyc20260911-110000", "2026-09-11T03:00:00Z"), + ): + assert f"{at} OK {cycle} - the review approved" in out, out + # A history, not a count: nothing on a landed head is reported as counting, because + # there is no head left for a vote to be about. + assert "counts" not in out + + +def test_a_landed_heads_history_is_not_voided_by_the_merge_instant(mod, monkeypatch, capsys): + """The control for the tempting fix: reuse the live clock on the landed path. + + A terminal PR's `push_time` is the **merge** instant, so reusing the `at <= + push_time` clause would stamp every review of a correctly merged PR with "submitted + before the head push (merge time)". That is a wrong reading of a correct fact, and + it is worse than the empty list it replaces: the empty list at least said nothing. + + The fixture keeps every vote before `merged_at` on purpose, so an implementation + that reuses the gate cannot pass this by accident of the timeline. + """ + votes = _three_votes() + merged_at = "2026-10-03T14:46:59Z" + assert all(v["at"] < merged_at for v in votes), "the fixture must predate the merge" + fake = FakeGh(votes, state="MERGED", merged_at=merged_at, + mergeable="UNKNOWN", merge_state="UNKNOWN") + rc = _run(mod, monkeypatch, fake) + out = capsys.readouterr().out + assert rc == 0 + assert "submitted before the head push" not in out + assert "VOID" not in out, "the clock is not asked of a head that has landed" + assert out.count("the review approved") == 3, out + + +def test_a_landed_heads_history_names_a_veto_and_a_body_it_cannot_attribute( + mod, monkeypatch, capsys): + """What the landed path keeps from the live one, and what it does not. + + Kept: the verdict-bearing reviews, told apart by mark (a veto is "NO", not "OK"), + and the two reasons a vote cannot be attributed to a cycle - which are facts about + the *body*, not about the clock, so a landed head reports them too. Dropped: plain + comments, exactly as on the live path. + """ + fake = FakeGh([ + _approve("cyc20260911-090000", "2026-09-11T01:00:00Z"), + _veto("cyc20260911-110000", "2026-09-11T03:00:00Z"), + _review("2026-09-11T04:00:00Z", "\u2705 LGTM - looked fine to me"), + _review("2026-09-11T05:00:00Z", "just a note, no verdict here"), + ], state="MERGED", merged_at="2026-10-03T14:46:59Z", + mergeable="UNKNOWN", merge_state="UNKNOWN") + rc = _run(mod, monkeypatch, fake) + out = capsys.readouterr().out + assert rc == 0 + assert "OK cyc20260911-090000 - the review approved" in out + assert "NO cyc20260911-110000 - the review objected" in out + assert "VOID (no cycle id) - no cycle id in the vote body" in out + assert "just a note" not in out, "a body with no verdict is not a vote" + + +def test_a_landed_heads_undated_review_fails_loud(mod, monkeypatch, capsys): + """The `at` refusal is shared with the live path, not lost on the terminal one. + + A projection that did not apply would leave every history line undated, and an + undated history cannot be placed against a merge, a head push or anything else: it + would look like a reading while answering nothing. + """ + class NoAt(FakeGh): + def paginated(self, args): + return [{"body": "\u2705 LGTM - cycle `cyc20260911-090000`", "author": SELF_LOGIN}] + + fake = NoAt([], state="MERGED", merged_at="2026-10-03T14:46:59Z", + mergeable="UNKNOWN", merge_state="UNKNOWN") + rc = _run(mod, monkeypatch, fake) + assert rc == 2 + assert "has no `at` field" in capsys.readouterr().err + + def test_an_uncomputed_mergeability_fails_loud(mod, monkeypatch, capsys): """`UNKNOWN` is a question not yet answered, not an answer of "mergeable".