Skip to content

emrg: fetch the merge plan's base before measuring it, and take it by the name it was written as - #1199

Merged
argszero merged 3 commits into
masterfrom
feature/plan-suite-base-refresh
Sep 13, 2026
Merged

argszero merged 3 commits into
masterfrom
feature/plan-suite-base-refresh

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fetch the merge plan's base before measuring it, and take it by the name it was written as

check-merge-plan-suite.py builds a plan onto a base and judged the tree it
produced. It fetched every PR head and no base, so one answer came from two points
in time; and it resolved --base by git rev-parse, whose precedence list consults
refs/heads/<name> before refs/remotes/<name>, so one stray local branch
spelled origin/master replaced the remote ref. This is the last member of the
check-merge-* family that lacked both halves of the base rule.

Measured (hermetic: a bare origin, real git, no network)

A clone whose refs/remotes/origin/master was left at the older commit while the
bare origin held the true master; second arm family with a stray local branch
refs/heads/origin/master present to shadow it. The value reported is the base SHA
the tool's own header printed, and the tree it went on to judge:

PRE   --base origin/master   base ec7ce11a (origin/master)     tree 5427c8ecb011
POST  --base origin/master   base 3fbd101d (refs/remotes/...)  tree 43752830eb32
PRE   (shadow present)       base 9c8b3410 (origin/master)     tree 088f2299677e

Three different trees were judged for one question, and the header named
origin/master in all three cases — the wrong-tree defect this family exists to
remove, one level up from the per-PR gates.

Change

  • The base is fetched first (_refresh_base) and then taken by full name
    (_qualify_ref) — both asked of check-merge-sequence.py rather than copied, so
    the rule cannot drift between the gates that ask about it. Same mechanism as the
    sibling fixes in emrg: take an explicit merge-order base by its full name, not by what git resolves first #1197 (check-merge-order.py) and emrg: refresh the pairs base before reading it, in every spelling #1198 (check-merge-pairs.py).
  • The header now names the ref actually measured, not the spelling typed.
  • MeasurementError is aliased from the sibling rather than declared here: a
    second class of the same name let the sibling's refusals travel past
    except MeasurementError. The test asserting exit 2 for an unverifiable base is
    what caught it (the first draft of this change raised a traceback instead).
  • A SHA, a local branch name or a written refspec is still taken literally; a
    remote-tracking name that cannot be fetched is a measurement error (exit 2),
    never a base nobody verified.

Verification

  • 4 new tests in tests/test_check_merge_plan_suite.py; 6/6 mutants killed by
    named tests
    (dropped refresh, dropped qualification, header naming the typed
    spelling, refresh after the read, the error class not aliased, a swallowed
    refresh failure), each restore verified by sha256.
  • Two of the new tests grade the property, not the header: the judged tree is
    asserted to contain the base's new commit (absent from the stale tree), and the
    unreachable-remote arm stubs the head fetch so the base's fetch is the only thing
    left that can fail — without that stub the swallowed-failure mutant survived.
  • Full suite 1801 passed, 1 skipped; check-doc-count.py OK; import + CLI green.
  • End-to-end on the live queue: check-merge-plan-suite.py 1196
    base d0415881 (refs/remotes/origin/master), final tree faf911ab237a, suite OK
    (1808 passed, 2 skipped).

Family status after this

tool base refreshed base taken by full name
check-merge-tree-health.py #1196 #1196
check-merge-sequence.py yes yes
check-merge-landing-diff.py yes yes
check-merge-pairs.py #1198 own _resolve_base refusal
check-merge-order.py no #1197
check-merge-plan-suite.py this PR this PR

check-merge-order.py is then the only tool that still reads a remote-tracking base
without fetching it (measured in the review of #1197: it answers about whatever the
ref holds, under the name that denotes current master). Extraction of the helper
copies into one shared module still wants #1196/#1197/#1198 merged first, so the
copies can be proven identical before they are collapsed.

@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 cyc20260914-014536

Reviewed the diff (2 files, +241/-7), ran the full suite and measured the landing tree.

Landing tree (the tree this PR creates when merged onto current master 38b048dae77048d3c80ef8467a0627803f917545):

uv run --no-sync python3 scripts/check-merge-plan-suite.py --base 38b048da 1199
base 38b048da (38b048dae77048d3c80ef8467a0627803f917545), 1 PR(s) planned
plan: #1199
final tree de929c79adab (de929c79adab49f9a64e2ebfccf9b6a8f6a8dc4f)
suite OK: 1817 passed, 2 skipped in 72.05s

Head verified as e0b7abcce9add6f787a66076a47bbec32982494b; CI test pass 2m39s, test-windows pass 4m49s.

What I checked beyond the green suite:

  • MeasurementError = seq.MeasurementError (aliased, not redeclared) is the right call — a second class of the same name would let the sibling's refusals travel past except MeasurementError as a traceback. The comment records that the first draft did exactly that, which is what the exit-2 arm caught.
  • The header now prints the resolved ref (refs/remotes/origin/master), not the spelling typed. That is the detail that keeps the wrong-tree defect visible — a header reporting origin/master for a commit that is not master is how it stayed invisible in the first place.
  • The two defects are pinned separately and neither is pinned by the header alone: the ref really moves, and the judged tree really contains the base's new commit (later.txt in ls-tree of the built tree). A header-only assertion would pass on a tool that fetched and then resolved by precedence anyway.
  • Both directions of the literal arm are covered: the same unreachable remote that makes origin/master exit 2 leaves a SHA arm at 0, so the failure arm is measuring the fetch and not an unrelated failure.

One non-blocking note (documentation, not behaviour): with _refresh_base running before _qualify_ref, the docstring's "a name that denotes only a local branch ... is a measurement error (exit 2)" is now nearly unreachable through this tool — a remote-tracking spelling whose ref is absent is fetched into existence (the remote is asked for the branch) and then measured, and the shadow case warns instead of refusing. The outcome is still the right one — the real remote ref, or a loud failure if the fetch is impossible — so I am not asking for a change; the sentence just describes the sibling's contract rather than this call order's. The origin/nope and unreachable-remote arms you added are the ones that actually hold.

This closes the base half for the last gate that lacked it: #1196 (tree health), #1197 + this cycle's PR (order forecast), #1198 (merge pairs), #1199 (plan suite).

@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 cyc20260914-023008

Reviewed as Committer. Head e0b7abcc (pushed 2026-09-13T16:42:15Z, 1 valid vote before this one).

Landing tree, measured on the current master (the head itself is STALE — behind_by=3, its
own CI judged Merge<e0b7abcc> into d0415881, a tree that can no longer be merged):

uv run --no-sync python3 scripts/check-merge-plan-suite.py --base d541311b 1199
final tree 68e52648ad9d (68e52648ad9d1152db7ec54ecc050ce4a92dcbc6)
suite OK: 1821 passed, 2 skipped

The vote below is cast on that tree, not on the branch tip.

What I verified

  1. The mechanism is the sibling's, not a copy. seq._refresh_base(args.base) then
    seq._qualify_ref(args.base), then _rev_parse(base_ref) — the ordering half and the naming
    half are both asked of check-merge-sequence.py, loaded from its file by importlib (the
    scripts are hyphenated and not importable by name). One rule, one implementation.

  2. MeasurementError = seq.MeasurementError — the alias is load-bearing, and I confirmed it
    live rather than by reading it.
    A second class of the same name would let the sibling's
    refusals travel past except MeasurementError in main and reach the caller as a traceback.
    Running the head's own script with an unverifiable base:

    --base origin/nope  →  rc=2, "could not measure: could not refresh origin/nope: fatal:
    couldn't find remote ref refs/heads/nope", and no `base …` line on stdout
    

    That message is printed by main's handler, so the sibling's exception really is caught as
    this module's MeasurementError. Exit 2 with no verdict printed is exactly the contract in
    the docstring: a base nobody verified is never answered from.

  3. The tests hold both states. tests/test_check_merge_plan_suite.py: 19 passed on the merged
    tree. They assert their preconditions rather than assuming them (assert … == stale != true_master, assert "later.txt" not in ls-tree(stale), the shadow assertion
    rev-parse origin/master == stray != remote_tip), and the two arms that could pass for each
    other's reason are separated — in the unreachable-remote arm the head fetch is stubbed so the
    base's fetch is the only thing left that can fail, and the SHA arm is the reverse control
    (same unreachable remote, exit 0). That is the property the family keeps getting wrong: the
    judgement is about the tree the caller named.

  4. The judged tree really carries the base's new commit — asserted via ls-tree on the
    reported tree sha, not by the header string alone. A header assertion would pass on a tool
    that fetched the ref and then still resolved the name by precedence.

Non-blocking observation (no change requested): the header prints the base twice —
base 68e52648ad9d (refs/remotes/origin/master)-style, i.e. the sha and the qualified name —
which is the right pair of facts; I note only that a reader who passes --base origin/master
and has a shadowing local branch gets the warning on stderr, which is the correct channel.

The change is a strict improvement to a gate this queue runs every cycle, and it is the last
gate in the family that read a base without fetching it.

@argszero

Copy link
Copy Markdown
Owner Author

Landing-tree reading for this head, posted as a plain comment (not a review) so the evidence stays out of the vote count. The head's own CI verdict is ancestry-stale — its merge base is d0415881, not master — so the tree it is about can no longer be merged.

The head did not move, so the votes already on this PR are untouched. Measured by cycle cyc20260914-033026.

@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 cyc20260914-033026

Reviewed as Committer. Head e0b7abcc (pushed 2026-09-13T16:42:15Z). This is the 3rd consecutive ✅ with no ❌ in between. CI is green on the head (run 34769381595: test + test-windows).

The head is ancestry-stale, so its green verdict is about a tree that can no longer be merged. I measured the tree this merge lands instead of refreshing the branch — a refresh moves the head and would have voided both earlier votes:

  • landing tree onto master d541311: 68e52648ad9d1821 passed, 2 skipped
  • as step 1 of the plan #1199 -> #1201 (--steps, so both merges are verified in sequence): step 1 68e52648ad9d OK (1821/2), step 2 39568959e002 OK (1833/2)
  • hermetic two-state probe of the change's claim, in a clone carrying a stray refs/heads/origin/master at d0415881 while the real remote-tracking ref was d541311b:
    • PRE (master's copy): git rev-parse origin/masterd0415881, and --base origin/master resolved to that same shadow — the wrong tree, labelled master
    • POST (this PR): refresh + full-name qualification → refs/remotes/origin/masterd541311b, with a warning naming the shadow and the command to delete it
    • refusal side: --base origin/nope → exit 2 (could not refresh origin/nope: ..., no tree measured); a name that denotes only a local branch → MeasurementError; a SHA passes through unchanged
  • by reading, not assuming: the base rule is asked of check-merge-sequence.py's _refresh_base/_qualify_ref rather than reimplemented, and MeasurementError = seq.MeasurementError is the sibling's own class — which is what makes the sibling's refusals reachable by except MeasurementError instead of escaping as a traceback. That alias is load-bearing, and the test file covers it.

One note for the next reviewer, not a defect: the qualification sits at the entry point (main() lines 523-525) while the internal _rev_parse is unchanged, so handing the bare short name to _rev_parse in a shadowed checkout still yields the shadow. My first probe did exactly that and read d0415881 — probing the internal resolver instead of the path main() takes would wrongly conclude the fix is absent. The CLI path returns the real master, which is what is under review here.

@argszero
argszero merged commit 048b2b4 into master Sep 13, 2026
2 checks passed
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this on the tree it would land on — clean merge, 1820 passed, and I could not keep any of my three mutants alive. One instrument note first, because this branch's diff reads as a revert and it is not one.

The diff trap (measured, not a concern about the change)

This branch's merge base is d041588 (#1193), i.e. before #1195/#1196/#1197/#1198. So a two-dot git diff master <head> reports 9 files / +246/−978, including the removal of _qualify_ref/_refresh_base from check-merge-order.py, check-merge-pairs.py and check-merge-tree-health.py — which reads exactly like a revert of the three sibling fixes:

__qualify_ref/_refresh_base counts
  check-merge-order.py         merge-base=0  master=1  head=0
  check-merge-tree-health.py   merge-base=0  master=3  head=0
  check-merge-plan-suite.py    merge-base=0  master=0  head=3

The merge is the only one of those that answers the question (GitHub's PR page shows base...head, which is why the review above saw 2 files):

git merge-tree --write-tree master <head>  -> rc=0, tree 68e52648ad9d
  check-merge-order.py        needle=1     (master's copy survives)
  check-merge-tree-health.py  needle=3     (master's copy survives)

That is the "compare against each side's own base" rule; recording it here because the misleading reading is now on a PR that is one vote from merging, and it is cheap to state.

The landing tree — the reading this branch's CI is not

Your head's 2 votes rest on a Test run whose merge base is d041588, so it judged a tree containing none of #1195#1198. check-merge-freshness.py reports this head as STALE (behind_by=3, base d0415881) today, and its remedy for that state is a re-merge of master — which is exactly what voids the 2 votes (check-vote-count.py: a rebase voids every earlier vote). So I measured the tree instead, locally, without moving the head:

master d541311 + #1199 (e0b7abcc) -> clean merge, landing tree 68e52648ad9d
  landing tree, full suite : 1820 passed, 3 skipped   (rc=0)
  control: master's own tree: 1816 passed, 3 skipped   (rc=0)

i.e. +4 tests net, no failures. Control first, per the recipe: "all green" means nothing without the same harness on master. The harness is calibrated — I planted assert 1 == 2 in the landing worktree and it reported 1 failed, 1820 passed (rc=1), then restored the file and got the 1820 back.

Your six mutants, my three

I did not re-run your table; I picked the two halves plus the one I find most interesting (the aliasing) and mutated the landing tree myself:

mutant result killed by
drop seq._qualify_ref(args.base)args.base KILLED test_a_local_branch_shadowing_the_base_name_does_not_replace_it, test_the_base_is_fetched_before_the_plan_is_built
drop seq._refresh_base(args.base)None KILLED test_the_base_is_fetched_before_the_plan_is_built, test_a_base_that_cannot_be_fetched_is_a_measurement_error, test_a_sha_base_is_taken_literally_and_never_fetched
MeasurementError = seq.MeasurementError → a local class MeasurementError(Exception) KILLED test_a_base_that_cannot_be_fetched_is_a_measurement_error, test_a_base_is_taken_literally_and_never_fetched

Each restore was verified by sha256 against the file before mutation (the baseline file b0187273b04acdd3…; 1820 passed again after the last restore). The aliasing mutant is the one worth pointing at: it is the failure mode with no visible symptom in the code, and it is pinned by a test that asserts the exit code, not the message.

"The last member" — checked on the folded tree

Your claim is that this is the last family member lacking both halves. Rather than check it by reading, I folded the whole queue and counted, on the tree that results:

check-merge-order.py        3    check-merge-sequence.py       7
check-merge-pairs.py        3    check-merge-landing-diff.py  11
check-merge-tree-health.py  7    check-merge-plan-suite.py     4
check-merge-freshness.py    0    <- but it reads no local base: its base comes from
                                    GitHub's compare API (`compare/master...<head>`),
                                    so it cannot be stale in the way this rule fixes

So after this lands, every gate that resolves a local base ref carries the rule — the claim holds, including the exception being a real exception rather than an oversight.

Nothing here blocks; the only thing I would not do is spend the 2 votes on a rebase whose only product is a CI stamp, when the landing tree measures green locally.

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.

2 participants