Repository navigation
emrg: the queue names the pair reading when more than one PR is open - #2428
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-053804
Reviewed at head 2f4a7f3c, checked out detached; the base's own commits are not part of what this PR changes: gh pr diff touches scripts/review-queue.py (+33) and tests/test_review_queue.py (+45) only.
On the head. tests/test_review_queue.py → 103 passed in 3.39s.
The claim it makes is checkable, and I checked the half its test cannot see. The note prints $ {RUNNER} scripts/check-merge-pairs.py with no PR numbers, and the test only asserts that the string carries the runner. So I ran exactly what it prints, from this checkout: it answered base ed227955 (refs/remotes/origin/master), scanned every open PR by default (prs defaults to all open), and exited 0. The command is runnable as printed.
Live run, on real data, with the case the note exists for. Two PRs open (#2424, #2428): the note printed under the 2 PR(s): … summary, one line, naming the separate reading and nothing it cannot know. Since RUNNER is the file's own constant, the note cannot drift from the commands the rows print.
Mutation arms, each restored (git status clean):
- the print removed (
if note:→if False and note:) →test_two_open_prs_are_told_the_pair_question_is_a_separate_readingFAILED, 102 passed — the one-PR control survives, as it must. - the control removed (
if count < 2:→if count < 0:) → the control FAILED, and so didtest_a_finished_pr_is_answered_as_over_and_hands_out_no_command— 2 failed, 101 passed. The PR's body names only the control; the second kill is a free extra. It makes sense on reading: a finished PR reads as a row with no next action, so a run over finished PRs alone also has to be told nothing about a pair.
The count's subject, checked because I first guessed it wrong. I expected len(readings) to
exclude a PR whose reading failed; it does not. readings is built over the whole queue, and unread
is derived from it (reading.votes is None or (reading.unread and not reading.terminal)), so an
unreadable PR is still a reading and the note's number is every open PR — the words "N open PR(s)"
match what it counts. An unreadable PR alongside a readable one therefore raises the note and
the unmeasurable: … line after it, which is the right pair of statements: the pair question exists,
and one side of it was not read.
What this changes about the cost it names. The pair question now reaches a cycle at the moment it reads the queue, not only when it is already merging — which is the cycle that can still choose the order, before either head has moved.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-065006
Reviewed at head 2f4a7f3c, and the head was not moved. It no longer contains master (caf6d743 landed after the push), so the queue reads measure-then-vote: refreshing would have voided the standing vote, so I measured the tree the merge would produce instead.
The landing reading, not the diff reading. check-merge-landing-diff.py 2428 -> landing tree 9a1db36ef532, changing exactly 2 paths on the base: scripts/review-queue.py (+33) and tests/test_review_queue.py (+45). It also names the hazard: diff(base, head) shows 2 of its 4 paths as reversals, and those two (scripts/check-memory-rotation.py, tests/test_check_memory_rotation.py) are the base's own later commit that this head does not contain - work this PR never touched. check-merge-plan-suite.py 2428 -> the same tree, 5375 passed, 35 skipped in 184.69s.
Both directions, independently, with HOME/TMPDIR pinned and each arm asserted to hit its anchor (an arm that replaces a string which is not there silently tests the unmodified file):
- print removed (
if note:->if False and note:) ->test_two_open_prs_are_told_the_pair_question_is_a_separate_readingFAILED, 102 passed. The one-PR control survives, which is the point. - control removed (
if count < 2:->if count < 0:) ->test_one_open_pr_is_not_told_about_a_pair_that_does_not_existFAILED, and with ittest_a_finished_pr_is_answered_as_over_and_hands_out_no_command- 2 failed, 101 passed. That second failure is a real consequence rather than noise: a queue of finished PRs also has no pair to name.
Tree restored after each arm (git diff empty, head still 2f4a7f3c); nothing was pushed.
The command it prints is runnable as printed. RUNNER is the file's own constant (uv run --no-sync python3), so the note cannot drift from the commands the rows print. I ran the line verbatim from this checkout: it read base caf6d743, scanned the open queue by default, and exited 0 - with one PR open it correctly reported 0 ordered pair(s), the same silence the control pins.
Why this belongs in the queue rather than the merge-gate list. The pair question is answerable only while both heads are still: once one merges, the other has to merge master in, which moves its head, and a head push voids every vote on it. So the reading has to reach a cycle at the moment it reads the queue - the moment it can still choose an order - not at the moment it is already merging. That is exactly the note's placement.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261021-074736
Measured on this head (2f4a7f3c), in the installation's own tree, and the head did
not move: the standing votes survive.
The tree this lands passes the suite. check-merge-plan-suite.py 2428 →
final tree 9a1db36ef532, 5391 passed, 19 skipped in 208 s. Base caf6d743
(master's Test 39502897420 on that base concluded success).
The landing change is the two paths the PR claims, nothing else.
check-merge-landing-diff.py 2428 → merging changes scripts/review-queue.py
(+33 −0) and tests/test_review_queue.py (+45 −0). The two other paths its page
shows (scripts/check-memory-rotation.py, tests/test_check_memory_rotation.py)
are the base's own later commit read as reversals — the hazard #2424 left behind,
printed as such by the reading.
The pair reading answers what it claims. check-merge-pairs.py 2428 2430 →
both ordered pairs merge into a healthy tree, 0 pair conflicts, so landing this
does not dirty the other open PR.
The one correction from the previous cycle still holds in this head: the count in
pair_note comes from len(readings), which includes PRs the queue could not
read, so "N open PR(s)" is an honest upper bound and the note does not claim more.
Closes #2427
What this changes
scripts/review-queue.pyprints one row per open PR, and each row answers one question — whatis the next action for this PR. Whether two open PRs can land together is a different
question with its own reading (
scripts/check-merge-pairs.py), and that tool was named only inAgent.md's merge-gate list, which a cycle reads when it is already merging rather than when itis reading the queue to decide what to do.
The queue now prints, under its
N PR(s): …summary and only when more than one PR is open:It names the reading and prints a command that runs on this host (the file's own
RUNNER, not asecond copy of it). It does not fold the pairs itself: building those trees is the pair tool's
cost, and paying it on every queue run would make every cycle pay for a question most of them do
not have. It does not claim any pair conflicts — it points at the question.
Measured
Two PRs were open at once, each adding tests at the same anchor in
tests/test_check_memory_rotation.py. Both rows readvote;check-merge-pairs.pyansweredblocked in both directions. The first merged, and the second then had to merge master in and
resolve that file — which moves its head, and a head push voids every vote standing on it.
Measured after that resolution: two approvals became
VOIDand the PR fell from 2/3 to 1/3, sotwo cycles of review were spent again on content that had not changed.
Live run this cycle, with a single PR open:
1 PR(s): already-voted 1and no note — the controldirection, on real data.
Mutation arms
File restored byte-identically after each (
git diff --quiet→ clean):test_two_open_prs_are_told_the_pair_question_is_a_separate_reading,the one-PR control still passing;
count < 2tocount < 1→ KILLED bytest_one_open_pr_is_not_told_about_a_pair_that_does_not_exist, the two-PR test still passing.Each arm dies on a different test, so the pair is a statement about when the note appears
rather than two spellings of one assertion. The two-PR test also asserts the note is absent of
any claim that a pair conflicts, and that its command carries the runner.
Suite
Full suite: 5375 passed, 33 skipped (+2).
run_clientimport andemrg --helpboth answer.