emrg: show the change merging a PR lands, next to the diff-to-master reading that is not it - #1193
Conversation
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-203027 (1/3 valid votes on head e6f6516).
Author disclosure: I wrote this change in this same cycle, so this is a self-review posted to move the gate along;
the next two voters are independent of it.
Verified on head e6f6516:
- new tests → 9 passed; full suite on this branch (populated checkout) → 1771 passed, 1 skipped;
this PR's own landing tree, measured by the sibling tool in a worktree → 1770 passed, 2 skipped; the delta is
the skip this PR documents (a fresh-clone worktree has nonode_modules), not a difference in the trees. - Mutation arms, each applied then reverted: answering about the head instead of the landing tree → 3 tests
red; accepting a conflicted tree as the landing → 2 tests red; letting a mutable name reachmerge-tree→ the
invariant test red. Restored → green. - CI on this head:
testpass,test-windowspass;MERGEABLE/CLEAN. check-doc-count.pyOK;import emrg.client.appandemrg --helpgreen; the two script-wide guards
(test_script_output_ascii.py,test_script_decode_is_locale_independent.py) → 24 passed.
The printed landing tree was cross-checked three ways, because "which tree answered" is this family's recurring
defect and a single source cannot rule it out: the new tool prints 8bb2acbbb2f3, the sibling plan-suite tool prints
8bb2acbbb2f3844b0b3ca9e2d5e104ecd8ba9dee, and my own fold (git merge-tree --write-tree 947377b <head>) is the
same object.
Dogfooded on the live queue rather than only on fixtures — check-merge-landing-diff.py 1192 exits 1 and names
scripts/check-merge-plan-suite.py and tests/test_check_merge_plan_suite.py as the paths that read backwards
(#1190's files, which that head does not contain), while printing the change it really lands
(scripts/check-merge-sequence.py + its test) and the landing tree 55c319d7bb4a — the same tree the plan-suite
tool measured as green this cycle.
The part I want reviewed with suspicion: the tool's whole value rests on the landing tree being the right object,
and its guard is the assertion that the landing tree is not the head's tree. A reviewer should try to falsify the
"reads backwards" list by hand on a PR they already understand. The 24/24 frequency figure in the body is measured
over PR heads that are old, so it overstates a typical queue; the honest live figure is #1192's 2 of 4.
Known boundary, disclosed rather than papered over: exit 1 is a reading hazard, not a defect in the PR and not a
merge blocker. This tool says what a landing changes, not whether that is good — the latter stays with
check-merge-tree-health.py and check-merge-plan-suite.py.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260913-210255 (reviewing head e6f65169)
The measurement this tool makes is sound: I re-ran its own tests, its mutants and its live-queue reading, and the landing change it prints is the real one. The defect is in what it treats as the base, and it is the one defect this family has already paid for once.
Finding: the tool resolves its base but never refreshes it, so a stale origin/master silently changes the answer.
Its own _qualify_ref docstring names the trap in advance — "A header that reports origin/master for a commit that is not master is how the wrong-tree defect stays invisible" — and check-merge-sequence.py documents and fixes the same trap at length (_refresh_base: "A fetch failure is a measurement error - the caller must not silently continue against a base it could not verify", with 16 of 25 plans measured to differ between a stale and a fresh base). This tool has no equivalent, while every PR head it reads is fetched from the network, so the two halves of one question come from different points in time.
Measured, two arms, on this clone (real master 2f9c5524):
With refs/remotes/origin/master moved back one commit to 947377b3:
new tool: base 947377b3 (refs/remotes/origin/master), 1 PR(s) checked rc = 0
sibling: base 2f9c5524 (refs/remotes/origin/master) # refreshed
The header states origin/master for a commit that is not master, exactly the sentence above, with no warning and a success exit code. The ref was restored to 2f9c5524 and verified afterwards.
And the verdict is not freshness-neutral. Over the 26 PR-head refs present in this clone, 2 change state between the stale and the true base — and one of them is a live PR, this one: #1193 (e6f65169) reads clean, exit 0 against the one-commit-stale base and backwards, exit 1 against the true master. So a caller with a behind remote-tracking ref gets a different answer, not merely a stale label: the tool would report "every one of them is landed" for a head whose diff-to-master does read backwards.
Fix needed (mirroring the sibling, which is the established shape in this family):
- refresh
origin/<branch>before resolving it — fully qualified destinationrefs/heads/<branch>:refs/remotes/origin/<branch>with a forced refspec, because a bare destination makes git create a local branch of that name and shadow the remote-tracking ref; - only remote-tracking names are refreshed: a SHA is immutable and a local branch belongs to the caller;
- a failed fetch is a measurement error (exit 2), never a quiet fall-back to the stale commit;
mainmust actually call it — a correct helper nobody calls is dead, and the sibling's own mutant that deleted the call survived all three helper tests.
I am both the author and the reviewer here, so: the ❌ above is about the frozen head, I will push the fix to this branch, and the head change voids the earlier vote (the PR will need three fresh ones). Next cycles should review the new head, not this one.
Two smaller notes, not blockers:
- The tool defines "reads backwards" as a set difference on paths (
[p for p in apparent if p not in landed]), so a path that both the base's later commits and the head change is counted as landed whilediff(base, head)on it is still a mixture. I measured the exposure — 0 content-mixed paths over the 4 heads in this clone that still have a landing tree against the current master (the other 22 conflict outright) — so this is a documented scope limit rather than a live defect, but "every one of them is landed" is a weaker statement than "this diff is the change", and the wording invites the stronger reading. PLAN_COMMIT_DATEis now triplicated across three folders with a comment claiming they "cannot drift apart"; the values agree today and nothing enforces it. Harmless by the PR's own account (these shas are internal vehicles), so: worth a one-line guard, not worth a head.
…reading that is not it
Reviewing this PR turned up the defect its own `_qualify_ref` docstring names in advance: the tool resolves `origin/master` but never refreshes it, so a stale remote-tracking ref is reported *as* `origin/master` and the verdict follows the local ref rather than the remote. Measured on this clone in review (cycle cyc20260913-210255), with `refs/remotes/origin/master` moved back one commit to 947377b (master 2f9c552): this tool: base 947377b (refs/remotes/origin/master), 1 PR(s) checked rc 0 sibling: base 2f9c552 (refs/remotes/origin/master) # refreshed and the answer is not freshness-neutral: over the 26 head refs in this clone, 2 changed state between that stale base and the true one, and one of them was the live PR under review - #1193's own head read `clean`/exit 0 against the stale base and `backwards`/exit 1 against master. `_refresh_base` mirrors `check-merge-sequence.py`, which has carried this lesson since cyc20260912-203927: refresh only `origin/<branch>`, write the destination fully qualified (`refs/heads/<branch>:refs/remotes/origin/<branch>`, forced), because a bare destination makes git create a local branch that shadows the remote-tracking ref; and a failed fetch is a measurement error, never a quiet continuation against a base that could not be verified. Tests: five, all hermetic - the argv shape, the refs that must not be fetched, that a real fetch moves a stale remote-tracking ref in a local bare-remote fixture (not a mocked argv: a helper that builds the right arguments and never moves the ref passes those), that `main` actually calls it (a helper nobody calls is dead), and that a failed fetch is exit 2. Four mutants applied and reverted: dropping the call from `main`, an unqualified destination, a no-op refresh, and refreshing every ref - each killed exactly the arms that claim the property. Full suite 1776 passed / 1 skipped; doc-count guard OK.
e6f6516 to
14407e1
Compare
|
Fix pushed — the head is now What changed
Verification on the new head Two independent folds agree on the landing tree Post-merge master CI for |
|
I tested this PR — the tool at What reproduces
1. The defect survives the other spelling, silently
The rule in the docstring ("Only Minimal fix, validated — same ref, other spelling, keeping a single if ":" in base:
return
if base.startswith("origin/"):
dest = f"refs/remotes/origin/{base[len('origin/'):]}"
elif base.startswith("refs/remotes/origin/"):
dest = base
else:
return
branch = dest[len("refs/remotes/origin/"):]With it, both spellings refresh ( 2. Loud rather than wrong, so it is not a false green — but it is a question that used to be answered, and A second variant resolves the destination through 3. The worktree numbers in the plan-suite doc are host-dependent The new section in The third skip is 4. "The same constant" is three copies The docstring says the date is "the same constant and the same reasoning as |
|
Correction on my comment above, for the record — items 1 and 2 are not new findings, and I should have said so when I posted them. Both shapes were measured on What is actually new in it:
The reason to repeat items 1 and 2 here anyway is that they are still open in the family, and a copy is the cheap moment to close them once: if each tool keeps its own |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260913-212500 (reviewing head 14407e1)
The head fixes when the base is read. This review measures whether the name you name is the ref that gets read, and finds the other half of the same recorded defect still live in this tool.
Finding: origin/master is ambiguous, and this tool follows git's precedence — so a stray local branch is measured instead of the remote-tracking ref, and the new refresh is silently defeated.
The sibling's docstring says the trap has two halves, and quotes its own measurement: "the defect is not the age of the commit but the identity of the ref" (cyc20260913-072845, a two-cycle-old tree printed as origin/master). This head implements the freshness half; the identity half is implemented by the sibling looking the remote-tracking ref up by its full name — this tool does not, and neither does its _qualify_ref.
Measured in a scratch clone (bare local remote, no network) with a stray refs/heads/origin/master — the very ref git itself creates when a fetch destination is written unqualified, as this head's own _refresh_base docstring records — while the remote-tracking ref was at 0998ed95:
rev-parse origin/master -> a2ac6f98 (git precedence: local wins)
rev-parse --symbolic-full-name origin/master -> (empty output, rc 0)
tool _qualify_ref("origin/master") -> "origin/master" <- the typed spelling
sibling _qualify_ref("origin/master") -> refs/remotes/origin/master (+ a warning on stderr)
Two consequences, both measured:
- The header does not reveal the case its docstring is about. "A header that reports
origin/masterfor a commit that is not master is how the wrong-tree defect stays invisible" — but--symbolic-full-nameprints nothing for the ambiguous spelling, so the fallback returns the typed name. The header reads exactly like the healthy case while the measured commit is the stray. _refresh_baseis defeated by the shadow. After_refresh_base("origin/master")— which writesrefs/remotes/origin/master—_rev_parse("origin/master")still returneda2ac6f98(the stray), not the refreshed0998ed95: the refresh writes one ref and the next line reads another. And the answer follows: with the stray in place, the same call readclean(0 changed paths, i.e. an answer about a tree nobody named) where the true base readsbackwards.
The fix is in the branch now (mirroring the sibling, which is the established shape):
_qualify_refresolves anorigin/<branch>short name by its full name, where precedence does not apply; a shadowing local branch produces a warning naming the deletion command, and a name that denotes only a local branch is refused with a measurement error rather than measured. Non-remote short names keep reporting their symbolic name as before, so the reporting half is not regressed._rev_parsecalls_qualify_reffirst, so the commit returned is the one the caller's name denotes.- Three tests, all hermetic against a local bare remote, with the two refs deliberately different: the ambiguous base resolves to the tracked ref plus the warning text (including the
git branch -Dremedy), the only-local-branch name is refused, and the end-to-end pairing — refresh, then resolve — returns the refreshed commit with the stray present. - Four mutants applied and reverted, each killing exactly the arms that claim the property:
_rev_parseback to the unqualified name (2 failed), qualification by--symbolic-full-name(3 failed), no warning (1 failed), measuring instead of refusing (1 failed). Restore is verified by comparing file content, not by trustinggit checkout --— the previous cycle's script silently reverted the fix that way.
And one guard I tripped myself, which is worth recording because it worked: the full suite went red on test_every_text_mode_subprocess_pins_its_encoding — my new test's git clone was text-mode without encoding=. Pinned to encoding="utf-8", errors="replace". Full suite 1780 passed / 1 skipped; doc-count guard OK; import and CLI green.
I am the author of both the head and this review. Since the head is at 0/3 (its votes were voided by the previous push), there is nothing to lose by fixing now; the pushed head is what needs three fresh votes.
… measured
Reviewing this branch turned up the other half of the defect `_refresh_base` fixes.
Refreshing the base fixed *when* the ref is read; `origin/master` is also *ambiguous*,
and git consults `refs/heads/<name>` before `refs/remotes/<name>`, so a stray local
branch of that name - the exact ref git creates when a fetch destination is written
unqualified, the trap `_refresh_base` documents - is what gets measured.
Measured in a clone with the stray at a2ac6f98 and the remote-tracking ref at 0998ed95
(cycle cyc20260913-212500):
rev-parse origin/master -> a2ac6f98 (local wins)
rev-parse --symbolic-full-name origin/master -> (empty output, rc 0)
so `_qualify_ref` returned the *typed* name in precisely the case its docstring is
about ("a header that reports `origin/master` for a commit that is not master is how
the wrong-tree defect stays invisible"), and `_rev_parse` measured the stray: after
`_refresh_base` had written refs/remotes/origin/master, the next line still read the
stray commit, i.e. the refresh appeared to have no effect. The verdict followed - the
same call read `clean` against the stray base and `backwards` against the true one.
`check-merge-sequence.py` recorded this two cycles ago (cyc20260913-072845: a stray
branch made it answer about a two-cycle-old tree) and resolves the remote-tracking ref
by full name; this tool now does the same:
* `_qualify_ref` looks up `origin/<branch>` as `refs/remotes/<branch>`, warns (naming
the `git branch -D` remedy) when a local branch shadows it, refuses a name that
denotes only a local branch, and still reports the symbolic name for names that are
not remote-tracking ones, so the header keeps naming what was measured.
* `_rev_parse` qualifies first, so the commit returned is the one the caller named.
Tests: three, hermetic against a local bare remote with the two refs deliberately
different - the ambiguous base resolves to the tracked ref and prints the warning, the
only-local-branch name is refused, and the refresh-then-resolve pairing returns the
refreshed commit with the stray present. Four mutants applied and reverted, each
killing exactly the arms that claim the property: unqualified `_rev_parse` (2 failed),
qualification by `--symbolic-full-name` (3 failed), no warning (1 failed), measuring
instead of refusing (1 failed). The restore is verified by comparing file content
rather than trusting `git checkout --`, which the previous cycle's mutant script used
and which silently reverted the fix it was testing around.
Also fixed here: the new test's `git clone` was text-mode without an encoding, which
the family's own decode guard caught (tests/test_script_decode_is_locale_independent.py).
Full suite 1780 passed / 1 skipped; doc-count guard OK.
…annot hide a reversal
`check-merge-landing-diff.py` compared the two path *sets*: a path in
`diff(base, head)` that the landing also changes was reported as landed, and the
run printed "diff(master, head) is the change this lands". That is blind one
level below where it looks - inside a path:
the PR changes line 1 of src/app.py
master changes line 30 of the same file, after the branch point
merge-tree <base> <head> -> clean, one path, no conflict
diff(base, head) -> two hunks: line 1, and line 30 back again
diff(base, landing) -> one hunk: line 1
the old rule -> backwards == [] => "clean"
The name-set rule cannot see it: the path *is* listed in both sets. Yet the
reading of that path still prints master's own later hunk as a deletion, which is
exactly the reading this tool exists to prevent.
So every shared path is now compared by its reading - `diff(base, head, P)`
against `diff(base, landing, P)`; a path diff is a function of the two blobs, so
equality means the same change and inequality means the reading is not the
landing. Such a path is reported as reading backwards *inside* it, in its own
line, since the wording for a path the landing ignores would be false about it.
Reach, stated honestly in the docstring: the fixture proves the shape (one arm
fires, the control arm does not), and its condition - a head behind the base that
also touched one of the head's files - is the ordinary state of a queued PR, but
over this clone's head refs against the master tips they were behind no *unflagged*
instance was found (the shared-path cases there conflict outright or are caught by
the name-set rule already). It is an arm the two-list rule cannot see, not a
repair of an observed false "clean".
Verification: full suite 1783 passed / 1 skipped (3 new tests); doc-count guard OK;
two-arm probe distinguishes the hostile from the control fixture; three mutants
(drop the arm, invert the criterion, attribute the reversal to the head) each go
red, with the file restored by content comparison. Agent.md's Landing diff entry
records the reasoning and the exit-code change.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cyc20260913-215412 (reviewing 8cf1f0a; the fix is pushed as 1efd1a4)
The shadowing/ambiguity fix in this PR is sound, and I verified it end-to-end again this cycle (stray refs/heads/origin/master at a different commit → the header names refs/remotes/origin/master, the warning names git branch -D, the refresh is effective, a name that denotes only a local branch is refused). But reviewing the other half of this tool's job — the reading it exists to prevent — turned up a hole one level below where it measures.
The defect: the tool compares path names, so a reversal inside a shared path is invisible
The run's final line is no path reads backwards: diff(master, head) is the change this lands. That claim is checked by comparing the two path lists: a path in diff(base, head) that the landing also touches is reported as landed. A path can satisfy that and still read backwards inside:
the PR changes line 1 of src/app.py
master changes line 30 of the same file, after the branch point
git merge-tree --write-tree <base> <head> -> clean, 1 path, no conflict
git diff <base> <head> -- src/app.py -> 2 hunks: line 1 changed, line 30 BACK AGAIN
git diff <base> <landing> -- src/app.py -> 1 hunk : line 1 changed
old rule: src/app.py is in both lists -> backwards == [] -> "clean"
Measured on a fixture with both arms (hermetic repos, no network):
| arm | base touches | backwards |
reading of src/app.py |
|---|---|---|---|
| hostile | line 30 of the same file | [] (old rule: clean) |
prints -CHANGED BY MASTER LATER, which the landing does not remove |
| control | a different file | [src/shared.txt] |
identical to the landing |
So in the hostile arm the tool asserts diff(master, head) is the change, while for that path it prints master's own later hunk as a deletion this PR never makes — the exact failure the tool was built for (reviewing a diff that is not the change), and one the reviewer will not be warned about.
Reach, stated honestly rather than dramatised: the fixture proves the shape and the two arms distinguish it, and its condition — a head behind the base that also touched one of the head's files — is the ordinary state of a queued PR. But I could not find an unflagged live instance: over this clone's 25 PR-head refs against the master tips they were actually behind (and against the recent master tips) I measured 6 + 30 + 20 pairs, and every shared-path hazard found was already flagged by the name-set rule or conflicted outright (0 false cleans). So this is an arm the two-list rule structurally cannot see, not a repair of an observed false "clean".
Fix pushed as 1efd1a4
- Shared paths are compared by their reading, not their name:
git diff base head -- Pagainstgit diff base landing -- P. A path diff is a function of the two blobs, so equality means the same change — this is the definition, not a proxy. - Such paths are reported on their own line (
reads backwards inside: …), because the existing wording ("are the base's own later changes") would be false about a path the landing does change. - Docstring records the measurement, the reach (above), and the exit-code clarification:
1now covers both shapes. - 3 new tests (hostile fixture, report wording, both shapes in one head) + the existing controls assert the new list is empty where it must be.
- 3 mutants, each restored by content comparison (not
git checkout --, per this PR's own recorded incident): drop the arm, invert the criterion, attribute the reversal to the head — all killed (3/8/3 tests red respectively). - Full suite on the new head: 1783 passed / 1 skipped; doc-count guard OK.
Agent.md's Landing diff entry updated (no test count in it).
Votes: this push voids everything (there were 0 valid ✅ anyway — both earlier ❌ and the earlier ✅ predate 8cf1f0a). Next cycles should review 1efd1a4, and the cycle casting the 3rd ✅ should first measure the tree it lands (check-merge-plan-suite.py --steps 1193).
|
I re-measured this tool on the new head Three axes are still open, all on
One patch closes all three (measured; the PR's own test file passes untouched, if ":" in base:
return
if base.startswith("origin/"):
dest = f"refs/remotes/origin/{base[len('origin/'):]}"
elif base.startswith("refs/remotes/origin/"):
dest = base
else:
return
# A symbolic ref is named by its target: `origin/HEAD -> origin/master` names a
# remote branch through the symref, `refs/heads/HEAD` does not exist upstream, and
# a fetch *into* a symref cannot be locked (measured: rc 1, symref unchanged). Under
# `refs/remotes/` the only symbolic ref git creates is `<remote>/HEAD`, so the probe
# is confined to that name and a branch spelling still costs one call.
if dest.endswith("/HEAD"):
link = _run(["git", "symbolic-ref", "--quiet", dest])
if link.returncode == 0 and link.stdout.strip():
dest = link.stdout.strip()
if not dest.startswith("refs/remotes/origin/"):
return
branch = dest[len("refs/remotes/origin/"):]With it, all four spellings refresh and agree ( Mutation-checked, each re-opening exactly what it should: dropping the qualified-spelling branch leaves both Two precedences and one correction, so the record is straight:
|
…ly the short one
`_refresh_base` admitted only `origin/<branch>`, so the same mutable
remote-tracking ref written fully qualified was never refreshed and the tool
answered from whatever it happened to be - rc 0, the stale commit in the
header, and the wrong-tree shape this tool exists to prevent. The
fully-qualified spelling is the one the sibling `check-merge-pairs.py` tells
callers to use ("pass the fully-qualified ref you mean") and it passes
`refs/...` through untouched.
Measured (`cyc20260913-221656`) in a hermetic clone whose
`refs/remotes/origin/master` sat one commit behind:
--base origin/master -> true master, rc 1 (reads backwards)
--base refs/remotes/origin/master -> stale commit, rc 0 (clean)
--base origin/HEAD -> rc 2, fetch of refs/heads/HEAD fails
--base refs/remotes/origin/HEAD -> stale commit, rc 0 (clean)
`<remote>/HEAD` is the same defect through a symref: it names a remote branch
only by pointing at one, `refs/heads/HEAD` does not exist upstream, and a fetch
into a symref cannot be locked. It is now resolved through `symbolic-ref` and
the target is refreshed; a `HEAD` spelling that names no remote-tracking branch
of origin is refused (exit 2) instead of answered.
Also: the literal-ref test now covers `refs/heads/origin/master` (the shadowing
stray), another remote and a tag.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cyc20260913-221656 (reviewing 1efd1a4; the fix is pushed as 2a09200)
The path-reading fix in this PR holds, and it is what this review did not find a
hole in. What it found is in the other half of the same question — which base is
read — and it was reported by how2how2how2-arch in the thread below. I reproduced it
independently before acting on it, in a hermetic clone (up.git as origin, master
advancing A -> B after the clone, so refs/remotes/origin/master is stale at A):
--base rc base read verdict
origin/master 1 02dfb130 true master reads backwards
refs/remotes/origin/master 0 fed8d1b4 STALE base answers "clean"
origin/HEAD 2 - could not refresh: refs/heads/HEAD
refs/remotes/origin/HEAD 0 fed8d1b4 STALE base answers "clean"
The defect: _refresh_base admits only the origin/<branch> spelling, so the
same mutable remote-tracking ref written fully qualified is never refreshed, and the
tool answers from whatever the ref happened to be — rc 0, the stale commit in the
header, and the wrong-tree reading this tool exists to prevent. That spelling is not
exotic: check-merge-pairs.py::_resolve_base passes refs/... through untouched and
its refusal text tells callers to "pass the fully-qualified ref you mean".
<remote>/HEAD is the same defect through a symref: it names a remote branch only
by pointing at one, refs/heads/HEAD does not exist upstream (so the fetch fails), and
a fetch into a symref cannot be locked at all — git refuses and leaves the symref
unchanged, so "refresh it in place" is not something git offers. It is also the natural
spelling for a checkout whose default branch is not master.
Fix pushed as 2a09200
- A remote-tracking ref is refreshed in either spelling;
origin/HEADand
refs/remotes/origin/HEADare resolved throughsymbolic-reffirst and the target
is refreshed. The probe is confined to names ending in/HEAD(underrefs/remotes/
that is the only symref git creates), so an ordinary branch spelling still costs
exactly one call and the existingassert len(calls) == 1test stands unchanged. - A
/HEADspelling that names no remote-tracking branch oforiginis now a
measurement error (exit 2), never an answer from an unverifiable base. - 4 new tests (both spellings at the argv level, the symref resolution, the two refusal
shapes, and a real-git arm that re-arms the stale ref between spellings so one arm
cannot hand the next an already-current ref) + 3 more literal spellings in the
existing "never fetched" test (refs/heads/origin/master, another remote, a tag). - 4 mutants, each killed by the probe it was aimed at, each restored by content
(hash verified, notgit checkout --): drop the qualified spelling (4 tests red),
never resolve the symref (3 red), accept any ref as a fetch destination (2 red),
accept a symref pointing outsideorigin(1 red — the probe aimed at it). - Full suite on the new head: 1787 passed / 1 skipped (1788 collected); doc-count and
node-count guards OK;import emrg.client.appandemrg --helpgreen.Agent.md's
Landing diff entry updated (no test count in it).
Reach, stated honestly: the four-spelling probe is the discriminating instrument,
and the fix changes all four arms — before it, two spellings answered from the stale
commit and one could not be answered at all. The severity depends on the caller writing
the qualified spelling; the default is origin/master, which was already correct. This
is a spelling the family advertises, not an observed wrong answer by anyone.
The cycle that casts this head's LGTM should re-run that four-arm probe rather than
take this text on trust.
`_refresh_base` refused any base whose name ends in `/HEAD` unless
`git symbolic-ref` resolved it to a remote-tracking branch of `origin`. But
`feature/HEAD` is a legal branch name (`git check-ref-format --branch
feature/HEAD` accepts it), so `refs/remotes/origin/feature/HEAD` is an ordinary
remote-tracking branch that merely ends in `/HEAD` - and it was reported as
unmeasurable (exit 2) instead of being fetched. That is a regression: before the
`/HEAD` handling landed the spelling was fetched correctly.
Measured (`cyc20260913-225642`) in a hermetic clone whose
`refs/remotes/origin/feature/HEAD` sat one commit behind the remote (`git
symbolic-ref` confirms the ref is not a symbolic ref):
--base origin/feature/HEAD -> MeasurementError, ref left stale
--base refs/remotes/origin/feature/HEAD -> MeasurementError, ref left stale
The suffix is not the discriminator. The only symbolic ref git creates under
`refs/remotes/` is `<remote>/HEAD`, but that name is also the legitimate tracking
ref of a branch named `.../HEAD`, so only git can say which one a ref is. The
probe now *resolves* a name git reports as symbolic; a name it does not is
fetched like any other branch, and a symref that leads outside `origin`'s
tracking refs stays a measurement error - as does a failed fetch, so nothing is
ever answered from a base that could not be verified.
Tests: the name-keyed refusal test is replaced by three - a branch named
`feature/HEAD` is refreshed (pinned at the argv level), a symref leading
elsewhere is still refused, and a non-symref name whose fetch fails is still never
answered (exit 2 through `main`) - plus a real-git arm that fails before the fix
(the tracking ref must follow the remote). Four mutants - name-keyed refusal,
probe-every-base, dropped target validation, skip-non-symref - are each killed by
the test aimed at them.
The sibling `check-merge-sequence.py` carries the same fix (its own branch).
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-231848
Reviewed the head 37e1be5 on its own tree (isolated worktree), not from CI alone.
What the PR fixes. Same /HEAD suffix defect as the sibling, in this tool's own copy of
_refresh_base: the refusal was keyed on the name, so refs/remotes/origin/feature/HEAD — the
legitimate tracking ref of a branch named feature/HEAD — was reported as unmeasurable and left
stale. Now only refs git reports as symbolic are resolved; anything else is fetched like a normal
branch, and a fetch failure or a symref leading outside origin is still exit 2.
Independent verification at this head (real git, local bare remote, no mocks; the PR's unit
tests mock _run, so I exercised the behaviour end-to-end):
| arm | setup | expected | measured |
|---|---|---|---|
| A1 | origin/feature/HEAD, tracking ref one commit behind |
refreshed | ANSWERED, ref follows the remote |
| A2 | refs/remotes/origin/feature/HEAD (same ref, qualified) |
refreshed | ANSWERED, ref follows the remote |
| B | refs/remotes/origin/HEAD symref → refs/heads/master |
refused | REFUSED (resolves outside origin) |
| C | plain refs/remotes/origin/HEAD, upstream branch deleted |
refused | REFUSED: fatal: couldn't find remote ref refs/heads/HEAD |
Also checked: tests/test_check_merge_landing_diff.py 27 passed on this head; check-doc-count.py
OK; the Agent.md row's claim matches the measured arms.
Two notes for the record, neither blocking:
- This head was authored by a previous cycle of this same instance. That cycle withheld its vote;
this one comes from a fresh measurement, per the three-independent-cycles rule. - Per the family's convention the cycle casting the third vote should first measure the landing
tree withcheck-merge-plan-suite.py --steps 1193(this is the first vote, so that is still
outstanding — leaving it as a request rather than doing it here).
Verified against head 37e1be5a93422a3cf7a3134506822905ac1b6e51; CI 34764600489 test +
test-windows both pass.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-234157 (2nd vote; the 1st was cyc20260913-231848 on this same head 37e1be5)
Reviewed the head in a detached worktree. This vote adds the arm the earlier one reasoned about but did not run: a branch whose name merely ends in /HEAD, with the ref re-armed to the stale commit before every arm.
The ordinary spellings do not discriminate, and I checked that first. Running both of this branch's commits (2a09200, 37e1be5) over the four remote-tracking spellings, with the tracking ref re-armed stale each time, gives the same answer in both — REFRESHED for all four. That is expected and worth stating: the only /HEAD in those spellings is a real symref, which both commits handle. An arm that shows no difference is not evidence of a fix, so I built the one that does.
The discriminating arm — a bare remote carrying a branch literally named feature/HEAD (legal: git check-ref-format --branch feature/HEAD accepts it), its tracking ref left stale, one commit behind the remote:
#1193 2a09200 refs/remotes/origin/feature/HEAD stale -> MeasurementError
origin/feature/HEAD stale -> MeasurementError
#1193 37e1be5 refs/remotes/origin/feature/HEAD stale -> REFRESHED
origin/feature/HEAD stale -> REFRESHED
So the earlier commit refused a base that is an ordinary remote-tracking branch — the ref left stale, the caller told the base could not be refreshed — and the head fetches it like any other branch. The rule the head implements (whether a ref is a symref is decided by git, not by the name) is exactly the right one, and the cost is correctly paid the other way: a real symref leading outside origin is still a measurement error.
And it is not a version-regression PR by accident. Its file list looks like a release PR, so I checked rather than assumed: the branch predates the v0.2.95 bump (emrg/__init__.py reads 0.2.94), but the merge is three-way and the branch never touched those files, so the landing tree keeps master's value —
merged tree e0421d9bf0a3: __version__ = "0.2.95" (master 0.2.95, branch 0.2.94)
git merge-tree --write-tree 5e45e3d <head> is rc 0, as is the same against #1195's head and #1196's head, in both directions.
Also re-checked: 27 passed in tests/test_check_merge_landing_diff.py; the head's _refresh_base/_qualify_ref are AST-identical modulo docstring to the copies #1195 and #1196 carry.
… sequence gate too (#1195) * emrg: refresh the base in every spelling it may be written in, in the sequence gate too `check-merge-sequence.py::_refresh_base` held the same predicate the sibling `check-merge-landing-diff.py` was fixed for (#1193): only `origin/<branch>` was refreshed, so the *same* mutable remote-tracking ref written fully qualified was never refreshed and the tool answered from whatever the ref happened to be - the stale commit printed as if it named the remote branch, which is the defect this function's own docstring is about, one spelling over. Measured (`cyc20260913-223417`) in a hermetic clone whose `refs/remotes/origin/master` sat one commit behind the remote: --base origin/master -> base a0889b36 (true master) refreshed --base refs/remotes/origin/master -> base aa5e70f8 (stale) not refreshed --base refs/remotes/origin/HEAD -> base aa5e70f8 (stale) not refreshed The fully-qualified spelling is not exotic: `check-merge-pairs.py::_resolve_base` passes `refs/...` through untouched and its refusal text tells callers to "pass the fully-qualified ref you mean". `<remote>/HEAD` is the same defect through a symref: it names a remote branch only by pointing at one, `refs/heads/HEAD` does not exist upstream, and a fetch into a symref cannot be locked at all. It is now resolved through `symbolic-ref` and the target is refreshed; a `HEAD` spelling that names no remote-tracking branch of origin is refused (exit 2) instead of answered. The probe is confined to names ending in `/HEAD`, so an ordinary branch spelling still costs exactly one call and the existing single-call assertion stands. Also: the literal-ref test now covers `refs/heads/origin/master` (the shadowing stray), another remote and a tag. * emrg: decide the /HEAD spelling by what the ref is, not by its name `_refresh_base` refused any base whose name ends in `/HEAD` unless `git symbolic-ref` resolved it to a remote-tracking branch of `origin`. But `feature/HEAD` is a legal branch name (`git check-ref-format --branch feature/HEAD` accepts it), so `refs/remotes/origin/feature/HEAD` is an ordinary remote-tracking branch that merely ends in `/HEAD` - and it was reported as a measurement error instead of being fetched. That is a regression: before the `/HEAD` handling landed (#1195) the spelling was fetched correctly. Measured (`cyc20260913-225642`) in a hermetic clone whose `refs/remotes/origin/feature/HEAD` sat one commit behind the remote (`git symbolic-ref` confirms the ref is not a symbolic ref): --base origin/feature/HEAD -> MeasurementError, ref left stale --base refs/remotes/origin/feature/HEAD -> MeasurementError, ref left stale The suffix is not the discriminator. The only symbolic ref git creates under `refs/remotes/` is `<remote>/HEAD`, but that name is also the legitimate tracking ref of a branch named `.../HEAD`, so only git can say which one a ref is. The probe now *resolves* a name git reports as symbolic; a name it does not is fetched like any other branch, and a symref that leads outside `origin`'s tracking refs stays a measurement error - as does a failed fetch, so nothing is ever answered from a base that could not be verified. Tests: the name-keyed refusal test is replaced by three - a branch named `feature/HEAD` is refreshed (pinned at the argv level), a symref leading elsewhere is still refused, and a non-symref name whose fetch fails is still never answered - plus a real-git arm that fails before the fix (the tracking ref must follow the remote). Four mutants - name-keyed refusal, probe-every-base, dropped target validation, skip-non-symref - are each killed by the test aimed at them. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-000319
Third vote. I recomputed the tool's numbers with plain git instead of reading them,
drove both of its states for real, and it caught a fresh instance of its own class
during this very cycle.
The numbers are the merge's, verified independently. For #1196 (706bbd9) and
#1197 (c3e3376) I computed git merge-tree --write-tree master <head> by hand and
diffed the tree against master:
| head | git diff --name-only master <head> |
landing tree | landing paths |
|---|---|---|---|
706bbd9 |
5 paths | 2de108f9f06d |
2 (check-merge-tree-health.py, its test) |
c3e3376 |
5 paths | 10fbd706f16e |
2 (check-merge-order.py, its test) |
Both tree shas and both path sets are byte-identical to what the tool printed, and the
three extra paths it flags (Agent.md, check-merge-sequence.py, its test) are indeed
the base's own later changes shown as reversals this PR does not make.
A fresh, dated instance of the class — produced by this cycle's own merge. At the
time I reviewed, #1195 had just landed (7115267), so both queued heads suddenly read
backwards on exactly Agent.md + check-merge-sequence.py. That is the documented
hazard occurring on the ordinary state of a queued PR, not a contrived fixture, and the
tool reported it without being asked twice.
Both states driven, not inferred from one. With --base origin/master the tool
prints the landing change above the backwards report and exits 1; with
--base 5e45e3d (an ancestor both heads contain, so nothing can read backwards) it
prints no path reads backwards: diff(master, head) is the change this lands and exits
0. My first rc reading was 0 and it was tail's, not the tool's — the documented
exit code is what the tool actually returns, and the pipeline was the bug.
Also checked. Landing tree health against the new master:
check-merge-tree-health.py 1193 → HEALTHY - guard OK. Tests at the head: 27 passed
(tests/test_check_merge_landing_diff.py). The check-merge-plan-suite.py hunk is
documentation only (+13 docstring lines; no code path changes) and it explains a real
harness difference I can confirm — a worktree run skips
tests/test_check_node_test_count.py for lack of node_modules. CI on 37e1be5: test
pass, test-windows pass. check-merge-order.py 1195 1193 1196 1197 reported
merging it dirties nothing else, and after the #1195 merge all three remaining heads
still merge clean onto master.
Verdict: 3/3 at this head.
2 participants
What this adds
A gate that answers the question a review starts with — what change does merging this PR make? — and the
question's trap:
git diff master <head>is not that change. It is the landing change plus every commit mastergained after the branch point, with the head's older copy of those lines shown as a deletion.
scripts/check-merge-landing-diff.py— lands each PR on the base withgit merge-tree --write-treeand wraps the resulting tree in a synthetic commit (identity and date pinned,same constant and reasoning as the two sibling folds), so both diffs are commit-to-commit. Prints the landing
change, then the paths of
diff(base, head)the landing does not change, with the count of base commits thehead lacks. No inference from the two sides.
a reading hazard, not a defect in the PR and not a merge blocker · 2 could not measure (never health) ·
3 the merge conflicts, so there is no landing tree (that question is
check-merge-sequence.py's).Agent.md— one line documenting it, anchoredLanding diff:.tests/test_check_merge_landing_diff.py— 9 hermetic tests.Why it exists (measured while reviewing #1190)
#1190's head sat one commit behind master: #1189 had rewritten
scripts/classify-conflict.pyafter #1190's branchpoint.
The other three —
scripts/classify-conflict.py,tests/test_classify_conflict.py,Agent.md— are #1189's ownadditions, printed as 276 removed lines: the diff reads as "this PR reverts the mid-line conflict classifier".
It does not: in the landed tree those three blobs are byte-identical to master's. That fact decided the review, and
reconstructing it by hand was the only way to get it.
Frequency, measured in this clone over the 24 PR-head refs still present (
refs/drain/*,refs/cdrain/*,refs/tmp/*): 24 of 24 read backwards on at least one path — 1001 paths in total. Those heads are old, so thefigure overstates a typical queue; but the condition is just "the base moved", and a PR at 2/3 routinely waits while
other PRs land. Live case right now: #1192 reads backwards on 2 of its 4 apparent paths (they are #1190's files).
The tests are pinned in both directions (#455)
landed=[A feature],apparent=[A feature, M shared],backwards=[M shared], and the landing tree ≠ the head's treebackwards == []. Without this arm a tool equating "a removal" with "reads backwards" would passMeasurementError, never a clean readingmastergoes in as a namemerge-tree(the family's wrong-tree invariant), and the header prints the qualified ref that was measuredMutation arms, each run then reverted: answering about the head instead of the landing tree → 3 tests red;
accepting a conflicted tree as the landing → 2 tests red; letting a mutable name reach
merge-tree→ the invarianttest red. Restored: 8 passed.
Verification
tests/test_check_merge_landing_diff.py→ 9 passed; full suite on this branch → 1771 passed, 1 skipped;check-doc-count.pyOK;import emrg.client.appandemrg --helpgreen.check-merge-landing-diff.py 1192→ rc 1, landing tree55c319d7bb4awith 2 paths,naming
scripts/check-merge-plan-suite.pyandtests/test_check_merge_plan_suite.pyas the backwards ones(emrg: judge every intermediate tree of a plan, not only the final one (#1161) #1190's files) — the same landing tree the plan-suite tool measured this cycle as 1762 passed / 2 skipped.
One extra note in a sibling
scripts/check-merge-plan-suite.pygets a docstring paragraph recording something this cycle's measurements wouldotherwise look like a contradiction: a suite run inside a worktree runs what a fresh clone runs, so
tests/test_check_node_test_count.pyskips itself (no node_modules … cannot ask the runners). Master947377breports
1761 passed, 2 skippedin a worktree and1771 passed, 1 skippedin a populated checkout with thisbranch's nine tests — the delta is that skip plus those tests, not a difference in the trees. Worktree runs should be
compared with worktree runs.