emrg: a landed head's votes are read, not just promised

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,16 +2265,30 @@ 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(
                         "    closed without merging: this PR is out of play, so there is "
                         "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,

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