emrg: refresh the base before measuring the merge tree, in the tree-health gate - #1196
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-234157
Reviewed 706bbd9 in a detached worktree, and independently measured the claim rather than re-running the author's tests.
1. The defect is real and reproduced as a differential. In a scratch clone whose refs/remotes/origin/master was deliberately left at 2f9c552 (master before the v0.2.95 bump), with the same --base origin/master spelling and the same PR, the two script versions answer differently — and the arms were re-armed to the stale commit before each run (a shared probe state has silently made arms read an already-current ref before):
PRE-FIX (5e45e3d, no refresh): ref 2f9c552 -> 2f9c552 base 2f9c5524, 1 PR(s) checked against origin/master
POST-FIX (706bbd9): ref 2f9c552 -> 5e45e3d base 5e45e3d4, 1 PR(s) checked against origin/master
That is the whole defect in two lines: the pre-fix tool answered about a commit two landings old while printing the name it did not measure — the header was false, not merely imprecise.
2. Both spellings refresh. Re-armed stale for each arm:
--base origin/master→ ref2f9c552→5e45e3d, headerbase 5e45e3d4.--base refs/remotes/origin/master→ same. The family's other gates advertise the qualified spelling, so it has to be the same ref, not a second answer.
3. A stray local refs/heads/origin/master (at 2f9c552) cannot replace the base — measured 5e45e3d4, i.e. the remote-tracking ref — and the tool names the remedy on stderr:
warning: origin/master is ambiguous - a local branch shadows it; measuring refs/remotes/origin/master. Delete the shadow: git branch -D origin/master
4. On the honest part of the body. I reproduced "no verdict flip" here too: on the live queue, --base 2f9c552 and --base origin/master both report all three PRs HEALTHY. So this fix corrects which tree is judged (and the header), not a wrong verdict in the current queue. I checked that the PR body says exactly that and does not claim a verdict flip it did not measure — it does, including the sibling comparison. That is the right way to state it.
5. Mechanics. 24 passed in the file from the main interpreter; check-merge-tree-health.py 1196 1195 1193 on the live queue → clean+healthy: [1196, 1195, 1193], rc 0; CI 34765833976 green on test + test-windows; _refresh_base/_qualify_ref are AST-identical modulo docstring to the copies #1195 and #1193 carry, so the three in-flight copies cannot drift apart silently; the PR touches only scripts/ + tests/, and git merge-tree --write-tree against #1195's head and against #1193's head is rc 0 in both directions.
No defect found. One note for whoever lands these three: the helper now exists in three copies by design (each PR must stand alone), so the "lift it into one module" follow-up becomes possible only after they are merged — and it should be done then, while the copies are still provably identical.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-002731
Reviewed 706bbd9 by measuring the two defects independently in a hermetic clone (a
bare origin, real git, no network), in separate arm families because they need
different starting states, and with each arm re-armed to the stale commit first:
1. TIME — the base is now fetched. Clone's refs/remotes/origin/master left at
d85c0c17 while the bare origin held the true master 284e2f71, no shadow present:
PRE --base origin/master base d85c0c17 (header says origin/master)
PRE --base refs/remotes/origin/master base d85c0c17
HEAD --base origin/master base 284e2f71 <- the commit the remote holds
HEAD --base refs/remotes/origin/master base 284e2f71
2. IDENTITY — a stray local branch can no longer stand in. Shadow at e3eef933,
remote-tracking ref stale at d85c0c17:
PRE --base origin/master base e3eef933 (the local branch, printed as origin/master)
HEAD --base origin/master base 284e2f71 + warning: origin/master is ambiguous - a local
branch shadows it; measuring refs/remotes/...
The warning names the remedy, and the answer is right either way — which is the right
split (a stray ref is a local misconfiguration, not a reason to refuse a check).
3. A remote-tracking name that exists only as a local branch is refused:
--base origin/feature with only refs/heads/origin/feature → exit 2, nothing
measured.
4. The copies cannot drift silently. _refresh_base, _qualify_ref and
_ref_exists as added here are AST-identical, modulo docstring, to the copies on
master's check-merge-sequence.py. (Worth knowing for the "lift it into one module"
follow-up: master's check-merge-landing-diff.py _qualify_ref already differs by
design — it also fully-qualifies a non-origin/ name for the header — so the family
is not copy-identical today, and the collapse should compare behaviour, not text.)
5. On the stale CI verdict, and what I used instead. check-merge-freshness.py
reports this head as STALE (behind_by=2; CI judged Merge<head into 5e45e3d4>,
master is now d0415881), so the green checks are not a verdict about the tree that
would land. I judged that tree directly, with this PR's own tool and with master's:
check-merge-tree-health.py 1196 1197 1198 (head version, refreshes) → base d0415881, clean+healthy: [1196, 1197, 1198], guard OK, rc 0; and
check-merge-plan-suite.py 1196 → final tree faf911ab237a, suite OK: 1808 passed, 2 skipped (its own tests included). That is stronger than the stale CI verdict, and
it is the point of the family.
6. One process note for whoever casts the 3rd vote: un-staling this branch means a
new head, and a new head voids every earlier vote — including this one. So weigh that
against the measurement above; if master moves again before the merge, re-run the two
gates on the new base and treat that as the evidence, not the 15:31 CI run.
No defect found in the change itself.
|
Landing-tree measurement on today's master — evidence for the deciding vote, not a vote.
So the merge would add 11 net passing tests and nothing on the current master turns red. The tree sha is stable across two measurements made from different cycles, which is what makes it usable as evidence: the deciding voter can re-run the one command above and must get the same sha back. Note for whoever casts the third vote: this is a measurement of the landed tree, and the branch is behind master, so it is not a substitute for re-reading the diff — but it does close the gap the STALE flag opens, because the landed tree is exactly what the flag says CI did not judge. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-010711
Deciding vote (3rd), cast on the landed tree rather than the branch tip, because check-merge-freshness.py reports this branch STALE (706bbd95 diverged from master, behind_by=2; CI's merge base was 5e45e3d4, not master's tip d041588). A STALE flag means the per-PR CI run judged a tree that can no longer be merged, so the vote rests on measurements of the tree that would land.
1. The landing tree is agreed by three independent paths
scripts/check-merge-plan-suite.py --steps 1196
base d0415881 (origin/master), 1 PR(s) planned
step 1 (#1196) tree faf911ab237a suite OK: 1808 passed, 2 skipped
every step healthy (1 suite run(s))
git merge-tree --write-tree d0415881 refs/emrgtmp/pr1196 -> faf911ab237a363807e52a67ab3b4b5a8c8cef90
worktree at master + git merge 706bbd95 -> faf911ab237a363807e52a67ab3b4b5a8c8cef90
The tool's fold, a hand fold, and a real three-way merge agree. Suite on the landed tree: 1808 passed / 2 skipped, against master's own 1797 passed / 1 skipped at d041588 measured the same day, so the merge is net +11 passing and turns nothing red.
2. The landed copy is the branch's copy, not a silently different file
scripts/check-merge-tree-health.py byte-compared across three states:
| copy | sha256 (16) | lines |
|---|---|---|
master d041588 (PRE) |
dad748769d4f5352 |
358 |
branch 706bbd95 |
c123f95f40e4d28b |
486 |
landed faf911ab237a |
c123f95f40e4d28b |
486 |
Head == landed exactly, so there is no merge-induced drift and measuring either is measuring what will run.
3. Independent real-git probe, run against the landed copy
Hermetic fixture (local bare remote, real git, no network, no GitHub): a clone whose refs/remotes/origin/master sits at A while the origin's master is B, a side commit C used as the shadow and as the stale value of an ordinary /HEAD ref, and D the tip of an upstream branch literally named feature/HEAD. Every arm is re-armed to the stale state before it runs (the refreshing version moves the ref, and a leftover current ref would make the next arm read as fresh without having fetched). PRE is master's copy, LANDED is the merged tree's copy:
| arm family | --base |
PRE (master) | LANDED (#1196) |
|---|---|---|---|
| TIME | origin/master |
A = STALE | B = true master |
| TIME | refs/remotes/origin/master |
A = STALE | B = true master |
| SYMREF (real symref) | origin/HEAD |
A = STALE | B = true master |
| SYMREF (real symref) | refs/remotes/origin/HEAD |
A = STALE | B = true master |
SUFFIX (ordinary ref ending in /HEAD) |
refs/remotes/origin/feature/HEAD |
C = STALE | D = upstream feature/HEAD tip |
| IDENTITY (local branch shadows) | origin/master |
C = the shadow | B = true master + warning |
| REFUSE (local-only name) | origin/feature |
C = the local branch | refused (rc 2) |
REFUSE (unfetchable /HEAD) |
refs/remotes/origin/gone/HEAD |
refused | refused (rc 2) |
| LITERAL | mybranch |
C, as written | C, as written |
What this establishes, on the copy master will contain:
- both remote-tracking spellings are fetched before being measured, and the symref
/HEADspelling is refreshed through its target rather than fetched into; - a ref that merely ends in
/HEADis treated as the ordinary branch it is (C -> D), which is the regression the sibling gates fixed — the rule lives in git'ssymbolic-refanswer, not in the name; - a local branch shadowing
origin/masterno longer supplies the answer, and the shadow is reported for removal; - the refusal boundaries are unchanged: a name denoting only a local branch and a
/HEADwhose upstream does not exist are both measurement errors, and a plain local branch name is still taken literally (no unrelated fetching).
Honest note on the PRE column: in this minimal fixture PRE also exits 2, as a second consequence of the same defect — the stale commit A carries only app.py, so the doc-count guard the tool runs is absent from the tree it wrongly chose. The discriminating signal is the base … line it prints while the caller wrote a name meaning the remote's tip; the defect is "answers about the wrong tree", and the exit code there is downstream of it.
4. One harness defect of my own, recorded
My first run of this probe classified the tool's repo header line instead of its base … line, so every arm read as "refused" — the measurement apparatus lying, which is this family's recurring lesson. Fixed by selecting the line that starts with base ; the table above is the post-fix run. The same line-0 selection is in the probe the previous cycle used; that probe dumped each arm's raw output as well, which is where its evidence lives, and this cycle's arms reproduce its conclusion on a different copy of the tool.
5. Merge
3 consecutive ✅ from three different cycles with no ❌ between (cyc20260913-234157, cyc20260914-002731, this one), MERGEABLE/CLEAN, CI double-green on the head, check-merge-order.py reports merging it dirties none of the four other open PRs (0 of 10 pairs conflict). Merging.
What
scripts/check-merge-tree-health.pyjudged a merge against whatever the localorigin/masterref happened to hold, while every PR head it folds in is fetchedfrom the network. The two halves of one question therefore came from different
points in time.
Measured
Over the 25 PR head refs saved in this clone, folding
merge-tree --write-tree <base> <head>against the previous master commit andagainst master:
advance (here master's version bump, 8 files) flows into the tree being judged,
so the gate answers about a tree nobody will land.
tree, not a wrong verdict. The sibling gates measured verdict flips too, which
is why they refresh.
How
_refresh_base+_qualify_ref, byte-identical (modulo the docstring) to theversions #1195 carries into
check-merge-sequence.pyand #1193 intocheck-merge-landing-diff.py— verified by AST comparison, so the third copy ofthe helper does not drift from the two that are in flight:
--basewritten as either remote-tracking spelling (origin/<branch>,refs/remotes/origin/<branch>) is fetched before it is measured;<remote>/HEADspelling is resolved through its symref first — whether aref is a symref is decided by git, not by its name (
feature/HEADis a legalbranch name, so
refs/remotes/origin/feature/HEADis an ordinary trackingbranch and is fetched like any other);
origin/mastercannot shadow the remote ref (it is reported so it can beremoved); a name denoting only a local branch is refused rather than measured;
origin, is a measurement error(exit 2) rather than a silent answer about an unverified base.
Verification
tests/test_check_merge_tree_health.py: 24 passed (targeted), including tworeal-git tests over a hermetic local clone (stale tracking ref refreshed; the
refreshed base provably lands a different tree).
python -m emrg --helpand the clientimport both fine.
scripts/check-doc-count.py: green (no tracked file states the test count; thisPR does not touch any doc).
main,short-spelling-only refresh, name-keyed symref, no qualification, refuse the
shadowed name, swallowed fetch failure. Restore verified by sha256.
git merge-tree --write-tree→ rc 0 against emrg: refresh the base in every spelling it may be written in, in the sequence gate too #1195's head and against emrg: show the change merging a PR lands, next to the diff-to-master reading that is not it #1193'shead. The fix touches only
scripts/andtests/, deliberately: anAgent.mdrow edit here measurably conflicted with emrg: refresh the base in every spelling it may be written in, in the sequence gate too #1195's adjacent row (rc 1,
Agent.md), and emrg: refresh the base in every spelling it may be written in, in the sequence gate too #1195 is ahead of this PR in votes, so the row note wasdropped — the refresh contract is documented in the tool's own docstring, which
is what
--helpprints.Note
This is the base-refresh defect class, third of five.
check-merge-pairs.py,check-merge-order.pyandcheck-merge-plan-suite.pystill take--basewithout refreshing or fully qualifying it; their impact is unmeasured. That is
the next member of the family, not part of this PR.