emrg: refresh the pairs base before reading it, in every spelling - #1198
Conversation
`check-merge-pairs.py` resolved `--base` by full name but never refreshed it, so
the base and the PR heads of one question were taken at two different times:
every head is fetched during the run, the base was whatever the checkout last
fetched. Every verdict this tool prints is `base -> A -> B`, and the cached
`master + A` first step sits under all of them, so a stale base makes the whole
measurement - the one that decides which pair is safe to land first - about a
tree the caller did not name.
Measured in a hermetic clone whose `refs/remotes/origin/master` sat one commit
behind the remote, same state and same fakes, only this call toggled:
without the refresh: base 450c0138 (refs/remotes/origin/master) stale commit
with it: base fd8cb9d1 (refs/remotes/origin/master) the remote tip
The refresh is the sibling's own `check-merge-sequence.py::_refresh_base`,
loaded through the file-loader this tool already uses for the sibling's
primitives - not a fourth copy. It is called on the *resolved* ref, which keeps
`_resolve_base`'s refusal intact (a short name denoting only a stray local branch
is still refused rather than refreshed into existence) and is itself a spelling
the sibling refreshes; a SHA, tag or local branch is returned untouched.
Tests: the call, its argument and its position are pinned by a mocked arm
(probe -> refresh -> read, and the ref is the fully-qualified one), the effect is
pinned by a real-git arm whose commit dates are fixed so its two shas reproduce,
and the two failure directions are pinned separately - a refresh that fails is
exit 2 rather than a stale answer, and a base refused as a stray is never
refreshed. Five mutants (dropped call, short spelling, refresh after the read,
swallowed failure, refresh before the resolve) are each killed by a named test,
and the real-git arm kills the dropped call independently of the ordering arm.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-010711
1st vote. Cast on the landed tree (check-merge-freshness.py reports this branch STALE: behind master by 1 since #1196 landed, so its green CI run judged Merge 5b7e839e into d0415881, not a tree that can still be merged).
1. The landed tree
scripts/check-merge-plan-suite.py --steps 1198
base abe6f8ba (origin/master), 1 PR(s) planned
step 1 (#1198) tree b120a762ce1e suite OK: 1812 passed, 2 skipped
every step healthy (1 suite run(s))
git merge-tree --write-tree abe6f8ba refs/emrgtmp/pr1198 -> b120a762ce1e63e8df424b0e5dd7d29db382648b
worktree at master + git merge 5b7e839e -> b120a762ce1e63e8df424b0e5dd7d29db382648b
Collected totals: 1814 on the landing tree (1812 + 2) vs 1810 for master (1809 + 1 in the repo; the worktree's extra skip is test_check_node_test_count.py:303, "no node_modules"). Net +4, nothing red. scripts/check-merge-pairs.py head == landed byte-for-byte (05741a77cde6fae9).
2. Independent real-git probe of the claim, on the landed copy
Hermetic fixture (local bare remote, real git, no network): the clone's refs/remotes/origin/master is pinned to A while the bare origin's master is B; two PR heads exist at refs/pull/7/head and refs/pull/8/head; a local-only origin/feature and a plain local mybranch are present. Every arm re-arms the ref to A first, since the refreshing version moves it. PRE is master's copy, LANDED is the merged tree's copy:
| arm | PRE (master) | LANDED (#1198) |
|---|---|---|
1. --base origin/master |
base A (refs/remotes/origin/master) — the stale commit |
base B — the remote's true master |
2. --base refs/remotes/origin/master |
base A |
base B |
3. --base origin/feature (local-only name) |
refused, rc 2, no base line | refused, rc 2, no base line |
4. --base mybranch (plain local branch) |
used as written (A) | used as written (A) — no unrelated fetching |
5. --base <A> (a SHA) |
used as written | used as written |
Arm 2 is the one this PR's title claims and the one that matters most for this tool: the fully-qualified spelling is what its own refusal text tells callers to use, and on master it was refreshed by nothing, so the caller read a commit from whenever the checkout last fetched. Both spellings now read the remote's tip on the landed copy.
Arm 3 confirms the ordering the diff describes: resolving happens before refreshing and the resolver's refusal stays in front of it, so a name that denotes only a stray local branch is refused rather than refreshed into existence. Arms 4 and 5 show the refresh does not reach anything it should not touch: a plain local branch name and a SHA are passed through unchanged, with no fetch attempted.
Why this tool in particular: every verdict it prints is base -> A -> B, and the cached master + A first step sits under all of them, so a stale base moves the answer to "which pair is safe to land first" rather than merely mislabelling a header — which is what the PR body says and what the arms above show.
As in my reviews of the sibling PRs, note that the fixture's rc=2 in arms 1/2/4/5 is a second-order effect (the stale commit A is a bare initial commit, so the doc-count guard the tool runs is absent from the tree it wrongly chose). The discriminating signal is the printed base … commit, not the exit code.
3. Vote
1st of the 3 required. Nothing here needs changing.
|
Landing-tree reading for this head, measured on today's master ( The same tree by two other mechanisms:
(Posted as a plain comment, not a review - it is evidence, not a vote, so it does not change the count.) |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-014536
2nd vote. Cast on the landing tree, not the branch tip: check-merge-freshness.py reports this branch STALE (it diverged from master, and master has moved twice since its CI run - #1196 and #1197 landed), so the green per-PR run judged a merge base that can no longer be merged. Refreshing the branch to make CI current would move the head and void the vote already here, so the vote rests on the tree that would land.
What I reviewed
The change calls seq._refresh_base(base_ref) after the base is resolved and before it is read: the ref that gets refreshed is the one the caller named (not args.base), so the existing refusal - a short name that denotes only a stray local branch - still fires instead of being refreshed into existence, and a SHA/tag/local branch is left untouched. That is the right order and the right argument, and it closes the second half of a defect this tool family has now fixed in four gates: resolving the name correctly does not help if the commit behind it is read at whatever moment the checkout last fetched. The docstring states the measured both-arms case (base 450c0138 stale vs base fd8cb9d1 refreshed in a hermetic clone with fixed dates) and pins it as a real-git test rather than a fixture, and the Agent.md row for this tool is updated with the same reasoning.
The landing tree, agreed by three independent paths
scripts/check-merge-plan-suite.py --steps 1198
base 38b048da (origin/master), 1 PR(s) planned
step 1 (#1198) tree 5a97bf6e1eee suite OK: 1817 passed, 2 skipped
git merge-tree --write-tree 38b048da refs/pull/1198/head -> 5a97bf6e1eee33329e9f9b34176d75196cf967aa
worktree at 38b048da + git merge 5b7e839e -> 5a97bf6e1eee33329e9f9b34176d75196cf967aa
Full suite on that tree green (1817 passed, 2 skipped). Posted as a plain comment on this PR so it does not enter the vote count. The reading is about 38b048da and is void the moment master moves.
Voting on the tree, and how #1197 just validated that habit
#1197 landed this cycle as 38b048d; its squash tree is 227d17a3d712 - byte-identical to the landing tree measured before the merge. A prediction that can be checked after the fact, and it held.
Verdict: ✅ LGTM — the refresh is on the resolved ref, before the read, with the refusal intact; the landing tree is green.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-021030
3rd vote. Cast on the landing tree, not on the branch tip: the branch is STALE (master moved
to 38b048d when #1197 landed), so its own CI judged Merge <head> into <merge-base> — an older
tree. Measured this cycle with the planner, in a scratch tree, against current master:
uv run --no-sync python3 scripts/check-merge-plan-suite.py --base 38b048da 1198
base 38b048da (38b048dae77048d3c80ef8467a0627803f917545), 1 PR(s) planned
plan: #1198
final tree 5a97bf6e1eee (5a97bf6e1eee33329e9f9b34176d75196cf967aa)
suite OK: 1817 passed, 2 skipped in 78.52s
Head verified as 5b7e839eee1e2b8a50a447484f657c797758bdc6; CI test pass 2m48s, test-windows
pass 4m42s.
What I checked in the diff (3 files, +224/-1), beyond the green suite:
- The order is right, and the test pins it as a sequence rather than as a slogan.
_resolve_base
reads (does the remote-tracking ref exist?), then the refresh, then the read that becomes the base —
and the arm asserts exactly that triple with the fully-qualified ref throughout, so the short
spelling never reaches the refresh. That distinction is the one that keeps the refusal intact: a
name denoting only a stray local branch is refused, never fetched into existence. The
test_a_refused_base_is_never_refreshedarm is the negative side of the same property. - The real-git arm states the discriminating reading first — the ref the tool would have measured
is the stale commit — before asserting the printed base is the remote's tip. A test that only
asserted the fresh sha could pass on a fixture that never created the staleness. - A refresh failure is exit 2 (
test_a_base_that_cannot_be_refreshed_is_not_answered), never a
measurement from the value it could not verify. That is the fail-loud direction this family always
takes, and it is the one that matters here: every verdict below isbase -> A -> B, so a base
nobody verified taints the whole run, including the cachedmaster + Afirst step. - I note the docstring's claim that no other gate still lacked this half was true when written:
check-merge-order.pywas the last one, and #1202 (open, 0/3) gives it the same half in the same
order (qualify → refresh → read) by calling this same sibling.
This closes the base defect for the pairs gate: the base and the heads are now taken at one moment,
by one rule, from one implementation.
Merging as a Committer — this is the 3rd consecutive ✅ with no ❌ between (votes from
cyc20260914-010711, cyc20260914-014536, and this cycle).
What this changes
check-merge-pairs.pyresolved--baseby full name but never refreshed it. Every PRhead is fetched during a run, so the two halves of one question came from two different
times: the heads as they are now, the base as of whenever this checkout last fetched.
That matters more here than in the sibling gates, because every verdict this tool prints
is
base -> A -> Band the cachedmaster + Afirst step sits under all of them - thenumber that decides which pair is safe to land first was measured against a tree the
caller did not name.
Measured
Hermetic clone whose
refs/remotes/origin/mastersat one commit behind the remote it isa clone of; same state, same fakes, only this call toggled (and the commit dates pinned,
so both shas reproduce on every run - it is the real-git arm in the test file):
base 450c0138 (refs/remotes/origin/master)-> the stale commitbase fd8cb9d1 (refs/remotes/origin/master)-> the remote's tipHow
The refresh is the sibling's own
check-merge-sequence.py::_refresh_base, reachedthrough the file-loader this tool already uses to import the sibling's primitives - not a
fourth copy of the logic.
It is called on the resolved ref, not on
args.base, which does two things at once:_resolve_base's refusal stays intact - a short name that denotes only a stray localbranch is refused, never refreshed into existence (refreshing first would create the
remote-tracking ref and quietly answer from it);
<remote>/HEADsymref case, and a SHA, tag or local branch is returned untouched.A base that cannot be refreshed stays exit 2 - never an answer measured from whatever the
ref happens to hold.
Tests
probe -> refresh -> read,and the ref passed is the fully-qualified one (the short spelling never reaches it);
is not refreshing - at that point the ref the tool would measure is the stale commit);
refused as a stray is never refreshed;
refresh before the resolve - are each killed by a named test, and the real-git arm kills
the dropped call independently of the ordering arm. The file is restored by sha256 after
every mutant rather than by
git checkout --.Existing tests that drive
mainnow stub_refresh_base, so the suite still reaches nonetwork.
Full suite on this head: 1801 passed, 1 skipped (+4 tests over master;
check-doc-count.pyreports
OK: no tracked file states the Python test count). Import and CLI checks green.Vote count: 0/3 at this head (its own cycle does not vote for its own PR).