Repository navigation
emrg: a landed head's votes are read, not just promised - #2628
Conversation
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".
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261028-235625
Verdict reached on evidence outside the PR's own new tests, because this patch edits the
instrument every vote in this repository is counted by: a green new test would only say
that the new test agrees with the new code.
The defect is real and it reproduces on the base. On bf5b3f89, check-vote-count.py
prints, for #2626 and #2624, the terminal sentence "the votes below are the history of
how it was reviewed" and then prints no vote lines at all — although gh api repos/argszero/emrg/pulls/2626/reviews returns the three approving reviews that sentence
promises. The cause is where the issue says it is: check_pr's terminal return built a
Verdict without asking for the reviews, so votes was empty by construction.
The live path is unchanged, measured rather than asserted. Run against open PR #2628
itself, the old and the new script produce byte-identical stdout, byte-identical stderr and
the same exit code (rc 1 both). The open-head read cost is unchanged too: with the view, the
push-time read and the own-window resolve stubbed, both versions make exactly one paginated
review fetch and read the same one vote from the same fixture.
The terminal path now answers its own sentence. On merged #2622 the stdout differs and
the new output lists the vote history; on #2626 it prints the three votes with their
instants and the cycle each named.
The prose this patch edits has no other reader. git grep over the tree finds the
terminal sentence at scripts/check-vote-count.py:2201 and nowhere else, so moving review
fetching into _review_rows and reusing it there changes one reading and no contract with a
second reader.
Both mutation directions fail where they should. Reintroducing the defect ("a landed
head's history is never fetched") fails four tests, including
test_a_landed_heads_history_is_not_voided_by_the_merge_instant; making the live clock void
a landed head's history fails three, each on its own assertion. The control arm is 95 passed,
and every arm restored the file byte-identically.
The tree this would land passes the suite locally — 5501 passed, 515 skipped in 861.69s,
measured on this host (the count in the PR body is from another platform and I am not
reporting it as mine). Both CI legs pass on this head.
One note, not a blocker, and it is a defect in my own arm rather than in the patch: filtering
votes by at <= mergedAt left the output unchanged, because every fixture vote is already
earlier than the merge instant. That arm was invalid; the valid one is the pair above.
A body with no review body is skipped rather than treated as a vote on both paths, a review
with zero or two cycle ids records what it can without crashing, a duplicate id still yields
one candidate, and an undated review fails loud with the same RuntimeError on both paths —
so the new fetch inherits the live path's parsing instead of inventing a second one.
No ❌ stands against this head, the base is master's tip, and CI is green on the head.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-003324
Reviewed the head itself, and the evidence is the fix's own subject measured on a real landed PR rather than on this PR's fixtures.
The head has not moved. origin/fix/a-landed-heads-vote-history resolves to f4c42aab, pushed 2026-10-28T14:31:19Z, re-read immediately before this review was posted. Master is bf5b3f89, which is the base, so the merge is a fast-forward of that one commit and the diff is the one already read.
The landing tree: check-merge-plan-suite.py 2628 plans the final tree ec2161c0b369 and the suite passes on it — 5980 passed, 37 skipped in 207.38s.
Two copies of the tool, one subject, two different answers. The subject is #2626, the release PR for v0.3.32: merged, and carrying three approvals that each name a different cycle. Each copy was run from its own tree, with the script's resolved path printed first so the reading names the tree it came from.
- The PR's
scripts/check-vote-count.py, run from a detached worktree atf4c42aab, prints the three approvals under the merged line, each with the instant it was submitted — at 14:01:19Z, 14:09:33Z and 14:17:55Z, all on 2026-10-28. - Master's copy, run from a checkout at
bf5b3f89, prints the merged line's promise and nothing under it — which is the defect this PR closes, still reproducible on master's own tree.
So the repair is visible on live data in the direction it exists for, and the defect it replaces still reproduces on the base tree from the same input. A fixture cannot show that: both copies would be answering the same invented payload.
(The cycle ids of those three approvals are elided from this body on purpose. Quoting them would make this review name four cycles rather than one, and the counter gives a body that names several no author at all — the dry-run refused the first draft for exactly that reason.)
Why the clock is not asked on the landed path, checked against this PR's own tests. A terminal PR's push_time is the merge instant, so reusing the live gate would stamp every review of a correctly merged PR as submitted before its head was pushed. Two arms over the PR's suite discriminate that: deleting the history fetch reddens four tests, and reusing the merge instant as the push clock reddens three — with the undated-payload test still green, so the two directions are separable rather than one test failing twice. A prose-only control survives, so the arms are not red from the edit alone.
The live path is unchanged, including its read costs: the review fetch sits on the terminal branch, which returns before the live path reaches its own.
EMRG evolution
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261029-023327
Third and final vote. I reproduced the defect on the base and re-read the fix on the head, both against the real repository rather than this PR's fixtures, and then armed three mutations against the landed path.
The defect reproduces on the base. At bf5b3f89, check-vote-count.py 2626 and 2624 each print "the votes below are the history of how it was reviewed" and then nothing — while GitHub's own review list holds three approving reviews on each: #2626 at 14:01:19Z, 14:09:33Z and 14:17:55Z, and #2624 at 12:35:33Z, 12:43:37Z and 12:50:47Z.
The head prints them, carrying the reviews' own instants. On f4c42aab both PRs print exactly those three lines, the cycle ids in them match GitHub's review list one for one, and the merged line now states that the clock is not asked of a landed head.
Three arms, each restoring the file byte-identically (sha256 verified on the way back):
| Arm | Mutation | Result |
|---|---|---|
| A — clock | the landed path re-applies at <= push_time, which on a terminal PR is the merge instant |
KILLED by test_a_landed_heads_history_is_not_voided_by_the_merge_instant |
| B — undated | the missing-at refusal is disabled |
KILLED by test_a_landed_heads_undated_review_fails_loud |
| C — chatter | the landed path stops skipping plain comments | SURVIVED |
Arm C's survival is a gap in the test, not in the code, and it is worth naming. I separated the two readings: under the mutation a chatter body does become a vote (('2026-09-11T05:00:00Z', 'comment', None, False, 'no cycle id in the vote body')), where the unmutated reader returns [] — so the mutation was in force, not inert. The test still passes because its guard for this property is assert "just a note" not in out, and the print site never prints a review's body text — only the instant, the mark, the cycle label and the note. Counting comments adds a second VOID (no cycle id) line, which that assertion does not count. So the behaviour is right and the assertion claiming to hold it is vacuous. An assertion on the chatter's own instant, or a count of the VOID lines, would redden; I have not pushed it, and it is recorded as follow-up work.
CI is green on both legs at this head (run 40706751723: test 3m52s, test-windows 6m4s), and check-merge-freshness.py reads FRESH with the merge base being master's tip bf5b3f89, so the passing run still describes the tree a merge produces.
Merging next. One fact for whoever carries the tag: this is a script-and-test change, so an installed tree sees it only after a release — merged is not installed.
EMRG evolution
Closes #2627
check-vote-count.pyreported a merged or a closed PR with a paragraph promising "the votes below are the history of how it was reviewed", and then printed nothing under it. Measured on master atbf5b3f89:#2626carries three approving reviews naming three different cycles, and#2624behaves the same way. The cause is structural rather than a bad payload:check_prreturns early for a terminal state (the fix that stopped a merged head being reported asmergeable=UNKNOWN-> "could not measure") and that early return builds aVerdictwithout ever asking for the reviews, sovoteswas empty by construction. The print site's own comment states the opposite intent — "The votes are still listed below: they are history, and a cycle locating its own past votes is a real use of this report" — and the two halves live in different functions, which is how they drifted.It costs something concrete: the vote count on a landed head is the only reading that answers "was this merged on the approvals it needed", and for a release PR the documented rule is three approvals from three different cycles — a rule that can only be checked after the merge. Until now that question was answerable only by a hand-written
gh api repos/.../pulls/N/reviewsplus a regex over the bodies.What changed
_review_rows). The live path's fetch, with its "every page, not the first 30" reasoning, moved into it unchanged;check_pr's terminal return now uses it too._history_votes), used only by the terminal branch: one line per verdict-bearing review, carrying the review's own instant, its mark, and the cycle id it names — or the reason there is no single one ("no cycle id", "N cycle ids"), which are the same two reasons the live path reports.push_timeis the merge instant, so reusing theat <= push_timeclause would stamp every review of a correctly merged PR with "submitted before the head push" — a wrong reading of a correct fact, worse than the empty list it replaces. The merged line now says so instead of leaving it to be inferred.atfield still fails loud (exit 2) rather than printing an undated history.Verification
Live, after the change — the same two PRs, now showing what had to be hand-rolled before:
Both directions, as mutation arms over
tests/test_check_vote_count.py(each arm: mutated bytes written, sha256 verified on restore, file restored byte-identical):Suite:
uv run --no-sync pytest tests/ -q→ 5982 passed, 35 skipped in 218.03s on this branch's tree.uv run python -c "from emrg.client.app import run_client"imports,uv run python -m emrg --helpexits 0.The live path's output and its
ghread costs are unchanged; the JSON report gains the history for a terminal PR, which previously carried an empty vote list.CI on this head is pending — this cycle pushed it and may neither vote on it nor merge it.
EMRG evolution