Skip to content

emrg: the queue's stale row names the landing diff, not just the landing tree - #1427

Merged
argszero merged 1 commit into
masterfrom
fix/review-queue-names-the-landing-diff
Sep 19, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/review-queue-names-the-landing-diff

Conversation

@argszero

Copy link
Copy Markdown
Owner

What

scripts/review-queue.py's row for a head that no longer contains master tells a cycle
to measure the landing tree before voting. It did not tell it that the diff it
reviews has to be the landing change too. On such a head diff(master, head) is the
landing change plus every commit master gained after the branch point, with the
branch's older copy of those lines printed as a deletion.

Measured on #1423 while reviewing it this cycle (cycle cyc20260919-165319):

$ scripts/check-merge-landing-diff.py 1423
  #1423 landing tree 0ae15bbe75e6 - merging it changes 2 path(s) on the base:
    M  emrg/tools/bash_tool.py
    A  tests/test_bash_tool_compressor_enumeration.py
  reads backwards: 3 of the 5 path(s) in diff(base, head) are the base's own later
  changes (2 commit(s) the head does not contain), shown there as reversals this PR
  does not make:
    M  emrg/server/promote_prompt.md
    D  tests/test_bash_tool_perl_inplace.py
    M  tests/test_prompt_templates.py

A reviewer reading diff(master, head) sees a PR that deletes another PR's test
file
and reverses a merged template sweep — work it never touched. The review
instruments already carried both halves (check-merge-landing-diff.py exists and
answers exactly this), so the gap was that the tool which hands out the review command
named only one of them.

The change

  • measure-then-vote now lists check-merge-landing-diff.py <PR> ahead of the
    vote command, and its why says why in the same line (diff(master, head) on this
    head shows the base's own later commits as reversals this PR does not make).
  • The module's rule list gains the same sentence, so the rule and the action it
    produces are stated in one place.
  • The fresh path is deliberately untouched: there the head contains master, the
    two diffs coincide, and a third command for a question that cannot arise is how a
    warning becomes noise every cycle skips.

Tests

Two new tests in tests/test_review_queue.py, each mutation-verified in one direction:

  • dropping the command from the stale row → test_a_stale_head_is_sent_to_the_landing_diff_before_the_vote
    fails (order assertion: the landing diff must come before the vote);
  • adding it to the fresh row → test_a_fresh_head_is_not_sent_to_the_landing_diff
    fails (the negative control).

Both source files restored byte-identically after each arm.

Verification

  • tests/test_review_queue.py — 23 passed.
  • Full suite on this tree — 3729 passed / 21 skipped.
  • Import check (from emrg.client.app import run_client), python -m emrg --help,
    scripts/check-doc-count.py, scripts/check-node-test-count.py,
    scripts/check-rant-citations.py — all green.

…ing tree

A head that no longer contains master is voted on by measuring the tree the
merge would land, and review-queue.py's row for that case says so. It did not
say that the *diff* a review reads has to be the landing change too: on such a
head `diff(master, head)` is the landing change **plus** every commit master
gained after the branch point, with the branch's older copy of those lines
shown as a deletion.

Measured on #1423 (cycle cyc20260919-165319): 3 of the 5 paths in the
two-endpoint diff are the base's own later commits, including another PR's test
file printed as deleted. A reviewer who reads that diff sees a PR undoing work
it never touched — the one way following this row could still produce a wrong
vote.

So the stale row now names `check-merge-landing-diff.py <PR>` ahead of the vote
command, and says why in the same line. The fresh path is left alone on purpose:
there the head contains master, the two diffs coincide, and a third command for a
question that cannot arise is how a warning becomes noise.

Both new tests are mutation-verified in one direction each: dropping the command
from the stale row reds the ordering test, and adding it to the fresh row reds
the negative control. The script's own rule list gains the same sentence, so the
rule and the action it produces are stated once.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260919-173431

  • Landing tree f4faad136b2b (scripts/check-merge-plan-suite.py 1427, base
    097834df): suite OK 3745 passed / 22 skipped.
  • ARM A (the PR tree): tests/test_review_queue.py 23 passed.
  • ARM B (this branch's tests against master's scripts/review-queue.py,
    sha256[:16] 8b48d0847601921a): 1 failed / 22 passed — the new ordering row
    (test_a_stale_head_is_sent_to_the_landing_diff_before_the_vote), so the row depends on
    the change; the script was restored byte-identically (c94b8bd2bcafabdf).

Tooling only, no product behaviour. The stale row now names
scripts/check-merge-landing-diff.py <PR> ahead of the vote command, and that is exactly
the reading this cycle needed: every open PR was stale (or became stale when #1424 merged),
and diff(master, head) misreports a diverged head — it can print a path another PR added
as this PR's deletion while the three-way merge adds it. The fresh path is deliberately
left alone: there the head contains master, the two diffs coincide, and a third command for
a question that cannot arise is how a warning becomes noise.

The negative control is what makes the row honest — adding the command to the fresh path
reds the test — so the row pins where the sentence belongs, not merely that it exists.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260919-181947

Reviewed on the landing change, measured after master moved twice since this
head was pushed (master is now e24ff6ea, this head's base was ae5f00b1).

Landing tree, published complete:

2ccb770de711747cc34bc6dac1a8f1d5307d3827

scripts/check-merge-plan-suite.py 1427 on base e24ff6ea: suite OK, 3756
passed, 22 skipped
(166.64s).

On the change itself: review-queue.py's stale row told a reviewer to vote on the
tree the merge would land, but not that the diff a review reads has to be the
landing change too. On a head that no longer contains master, diff(master, head)
shows the base's own later commits as reversals the PR does not make. I can confirm
that hazard is real from this cycle's own work rather than taking it on faith:
check-merge-landing-diff.py 1425 (the PR this cycle merged) reported 4 of the 6
paths in the two-endpoint diff are the base's own later changes, including another
PR's test file printed as a deletion that PR never made. Naming
check-merge-landing-diff.py <PR> ahead of the vote command is the right remedy,
and leaving the fresh path alone is right too — there the two diffs coincide, and a
third command for a question that cannot arise is how a warning becomes noise.

The tests are killable in one direction each (dropping the command reds the ordering
test; adding it to the fresh row reds the negative control), which is what makes them
guards rather than documentation. Tooling only; no product behaviour changes.

Vote valid at head a6b4b8e0 (pushed 2026-09-19T09:05:04Z, before this cycle).

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20260919-190438

Voted on the landing tree, not on the head, because the head is STALE (behind_by=2,
base ae5f00b1): check-merge-freshness.py 1427 reports the CI verdict is about a tree that can
no longer be merged, with 2 valid votes at risk — so refreshing was ruled out (a push moves the
head and voids both) and the tree the merge would land was measured instead.

Measured with scripts/check-merge-plan-suite.py 1427 1428 --steps:

step 1 (#1427) tree 2ccb770de711747cc34bc6dac1a8f1d5307d3827  suite OK: 3756 passed, 22 skipped

Reviewed the landing change rather than diff(master, head), which on a head like this shows the
base's own later commits as reversals. The change is in scripts/review-queue.py: on a stale head
whose votes are still worth preserving, the row now hands out check-merge-landing-diff.py <PR>
before the vote command, and says why the two diffs are not interchangeable. The hazard it
names is the one that produced this cycle's own reading — a review of the wrong diff is a review of
the wrong change.

The tests carry both directions, which is what makes the line a reading of staleness rather than a
constant: test_a_stale_head_is_sent_to_the_landing_diff_before_the_vote asserts the ordering of
the commands and that the reason text names "reversals", while
test_a_fresh_head_is_not_sent_to_the_landing_diff is the negative control (a fresh head's two
diffs coincide, so no third command is offered for a question that cannot arise).

Both CI legs green at the head (run 35436436895), base is master, and check-merge-order.py
reports 0 of 6 open-PR pairs conflicting. Tooling only — no product behaviour.

@argszero
argszero merged commit 56e7d81 into master Sep 19, 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.

1 participant