Skip to content

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

Merged
argszero merged 1 commit into
masterfrom
fix/a-landed-heads-vote-history
Oct 28, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/a-landed-heads-vote-history

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Closes #2627

check-vote-count.py reported 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 at bf5b3f89:

#2626 MERGED (head 0e5c303c, merged 2026-10-28T14:18:13Z)
    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

#2626 carries three approving reviews naming three different cycles, and #2624 behaves the same way. The cause is structural rather than a bad payload: check_pr returns early for a terminal state (the fix that stopped a merged head being reported as mergeable=UNKNOWN -> "could not measure") and that early return builds a Verdict without ever asking for the reviews, so votes was 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/reviews plus a regex over the bodies.

What changed

  • One review reader for both readings (_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.
  • A landed head's votes are read as history (_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.
  • The live clock is deliberately not asked. 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" — 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.
  • Nothing is reported as counting on a landed head, because there is no head left for a vote to be about: the history line prints what the review said.
  • Plain comments stay skipped, exactly as on the live path, and a payload that lost its at field 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:

2026-10-28T14:01:19Z OK   cyc20261028-215859 - the review approved
2026-10-28T14:09:33Z OK   cyc20261028-220813 - the review approved
2026-10-28T14:17:55Z OK   cyc20261028-221539 - the review approved

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):

Arm Mutation Result
A the history is never fetched — the defect itself KILLED — 4 failed / 91 passed
B the live clock is reused on a landed head KILLED — 3 failed / 92 passed, and the undated-payload test stays green, so the two directions are separable rather than one test failing twice
C prose only, no behaviour SURVIVED — 95 passed, so the arms are not red from the edit alone

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 --help exits 0.

The live path's output and its gh read 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

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 pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 at f4c42aab, 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

@argszero
argszero merged commit 7e76668 into master Oct 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check-vote-count.py promises a landed head's vote history and prints none

3 participants