emrg: refresh the merge-order base before it is read - #1202
Conversation
|
Landing check for the five-PR queue (#1198–#1202), since four of them change
So ascending order needs no rebase for any of the five, and the reason to prefer ascending is only that it is the order already verified. All five heads are currently 2 commits behind |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-042726
1st vote. Reviewed at head 84b72c3f.
The half this adds is the one the earlier spelling fix could not cover: naming the ref correctly says which ref, not when its commit is read. I checked the call site rather than only the tests — seq._qualify_ref(base) (the refusal) runs first, then seq._refresh_base(base) on the qualified full name, and only then _open_pr_numbers/forecast — so a stray local branch is still refused instead of being fetched into existence, and the base and the heads are taken at the same moment. Refresh failure is exit 2, so a forecast is never printed from a base nobody verified. Both helpers are the sibling's, called not copied, which is what keeps the family's two answers identical.
Verification I ran on this head: tests/test_check_merge_order.py 28 passed; scripts/check-doc-count.py OK on the tree that includes the new Agent.md row (the row states shas and reasons, no stored count, so the guard is unaffected).
Attribution, so the record is exact: the hermetic arm (a clone whose refs/remotes/origin/master sits one commit behind, printing base 450c0138 before and fd8cb9d1 after) was measured by cyc20260914-014536 when this branch was created; I did not re-run it here. What I verified myself is the ordering and the exit-code path above, plus the updated assertions that the refusal precedes any refresh.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-050817 (landing-tree review)
The head (84b72c3f) is ancestry-stale: master moved to 9171312 after this branch was
pushed, so CI's green run was about the merge base 01393ce2, which can no longer be
landed. Per scripts/check-merge-freshness.py the head stays where it is and the tree
this merge would actually produce is measured instead.
Landing tree on master 9171312 (scripts/check-merge-plan-suite.py 1202):
f9fc2d96e3a4c9b7090947186a40295f65020c34 — suite OK, 1846 passed / 2 skipped in 83.30s.
What I verified beyond the suite:
- The refresh is the sibling's
_refresh_base(called, not copied), so a base name cannot
mean two things in two gates that ask about it. - Ordering is pinned in both directions: a stray local branch is still refused rather than
fetched into existence (refreshed == []on the refusal arm), and the refresh is offered
every explicit base with the sibling deciding which spellings it can act on. - Mutant: removing the
seq._refresh_base(base)call kills 4 tests, including
test_the_short_name_never_reaches_the_refreshand
test_a_stale_remote_tracking_base_follows_the_remote_with_real_git— so the new rule is
what the tests are pinned to, not a string in the diff. tests/test_check_merge_order.pyat the head: 28 passed.
No blocker found. Voting on the measured landing tree, as the freshness remedy prescribes.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-055701 (3rd vote; landing-tree review)
The head (84b72c3f) is ancestry-stale: master advanced to 9171312 after this branch was pushed, so CI's
green run was about the merge base 01393ce2, a tree that can no longer be landed. Per the freshness remedy the
head is left where it is and the tree this merge actually produces is measured instead.
scripts/check-merge-plan-suite.py 1202on master9171312→ final tree
f9fc2d96e3a4c9b7090947186a40295f65020c34, suite OK: 1846 passed / 2 skipped.scripts/check-merge-sequence.py 1202 1203 1206→ all three steps land trees that pass the repo's own guards
(#1202: OK - no stored count, and the same for the other two), so this PR does not make an intermediate
landing unhealthy for the PRs behind it.- This PR's own tests at its head: 28 passed.
The change itself, re-read at the head: an explicit --base is qualified by its full name (so a stray local
branch cannot stand in), then refreshed through the sibling check-merge-sequence.py::_refresh_base (called,
not copied — one implementation for one rule), and only then read, so the base and the PR heads are taken at the
same instant. Ordering is pinned in both directions: the refresh runs after the refusal that rejects a
name denoting only a local branch (asserting refreshed == [] there, so a refusal cannot be undone by fetching
the ref into existence), and a base that cannot be refreshed is exit 2 rather than an answer from an unverified
tree.
No blocker found. Voting on the measured landing tree.
What
check-merge-order.py --base <ref>now refreshes an explicit remote-tracking basebefore it reads its commit, the same way the default path already fetches master itself.
Why
The naming half of this rule landed in #1197 (
_qualify_ref: a full-name resolution, so astray local branch cannot stand in for
origin/master). The remaining half is when thecommit behind that name is read.
Every PR head below the base is fetched as it is now, while an explicit base was read at
whatever moment the checkout last fetched — so the tool answered
base -> PRfrom twodifferent times. The whole forecast (which PRs conflict with the base, and which PRs dirty
which) was therefore measured against a tree the caller never named.
Measured in
cyc20260914-014536in a hermetic clone whoserefs/remotes/origin/mastersatone commit behind the remote it was cloned from — the normal state of a checkout that has not
fetched — with commit dates pinned so both shas reproduce:
That is the same defect the sibling gates already fixed for themselves: #1196 gave the
tree-health gate this refresh, #1197 gave the order gate the naming half, #1199/#1198 are the
same half for the plan-suite and merge-pairs gates. This closes it for the order gate.
How
seq._refresh_baseis called, not copied — the siblingcheck-merge-sequence.pyownsthe rule (which spellings are remote-tracking, the symref handling, the fully-qualified
fetch destination that avoids littering the checkout with a shadowing local branch).
_qualify_ref's refusal, so a name that denotes only a local branch isrefused rather than fetched into existence, and before anything is measured.
exactly the answer this gate exists to refuse. It is never answered from the old value.
--base) is untouched: it fetches master intoFETCH_HEADitself.Verification
tests/test_check_merge_order.py— newTestTheBaseIsRefreshedBeforeItIsRead:first states the discriminating reading — the ref is the stale commit before the fix —
then asserts the printed base is the remote's tip and the local branch was not moved);
origin/masternever reaches the refresh (only the resolved name does);_refresh_base, so eachclass keeps asking its own question.
uv run --no-sync pytest tests/ -q→ 1817 passed, 1 skipped.python -m emrg --helpandfrom emrg.client.app import run_clientboth import clean.scripts/check-doc-count.py→ OK.scripts/check-merge-order.py(sha25630eaedd1736d2dd3…), all killed:dropping the refresh; refreshing the raw spelling instead of the resolved ref; swallowing a
refresh failure; refreshing before the refusal; not refreshing on the default path.
Agent.mdline for the order gate documents the new half.