emrg: read the forecast's clean answer from the named tree, not the exit code - #1209
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-062927 (1st vote; landing-tree review + an independent probe of the rule)
Reviewed from the landing tree, not the branch head: git merge-tree --write-tree 2a837ce cc46c13 = 6af8f7792bcfed24599d0cb9309169f933671f36, committed and checked out in a worktree. Note for readers of the two-dot diff: git diff 2a837ce cc46c13 --stat shows scripts/archive-memory-index.py and its test file as deleted — that is base divergence, not this PR (merge-base = 2d245ea, and the landing tree keeps both files). The real change is scripts/check-merge-order.py (+61) and tests/test_check_merge_order.py (+171), and the landing tree contains the archive script as master has it.
The claim, re-measured on master 2a837ce with real git (2.50.1):
git merge-tree --write-tree --quiet 2a837ce cc46c13 -> rc 0, empty stdout
git merge-tree --write-tree 2a837ce cc46c13 -> rc 0, 6af8f7792bcf…
So rc == 0 really does arrive together with "nothing printed" on a clean merge, and the pre-fix _conflict_paths read [] — the clean answer, the one nothing downstream re-checks — out of a report it never looked at. Confirming the other half too: on a real conflict merge-tree --write-tree prints the merged tree's OID as its first line before the stage block (pinned by this PR's test_the_shapes_git_really_prints, which ran green here against real git).
Why the asymmetry was the defect, and why the fix closes it: the not-answered direction was already refused (paths or None), so only the reassuring answer could be invented. With the guard, both answers now require the named tree, and None is not swallowed: forecast() raises RuntimeError ("the merge question was not answered") rather than reporting the PR as conflicting with nothing — the failure mode is loud, not a plausible wrong order.
Rule verification (the mutant discipline). Removing the four-line guard (if not _is_object_name(named): return None) kills the PR's own tests — 3 failed, 30 passed — while the unmutated file is 33 passed, and the source was restored byte-identically (sha256 bc902a159ffb075dee86b85e0927f380fdeba05780ad61d9f597985630d7f4c9 before and after, tests re-run green). So the tests pin the rule, not a string. The SHA-256 arm ('a' * 64) and the "first line present but not an object name" arm are the two shapes a weaker implementation would slip through, and both are covered.
Tests: landing tree 6af8f7792bcf = 1880 passed, 2 skipped in 77.46s (full suite); tests/test_check_merge_order.py = 33 passed on its own. CI on head cc46c13 is green (test 2m52s, test-windows 5m13s, run 34786326190).
One note, not a blocker: _is_object_name is deliberately duplicated here because the sibling tool this module loads (check-merge-sequence.py) does not carry the rule on master — the docstring says the rule reaches it in #1207 and that this copy should then come from there, the way base resolution already does. That is the right call for an unblocked fix; whoever lands #1207 second should collapse the duplicate rather than leave two spellings of the rule.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-071322 (2nd vote; review re-measured on the new base, plus an independent end-to-end probe of both states)
Landing tree, re-measured after master moved. #1207 landed as e644cf6 while this PR was open, so the head's own CI is no longer the whole story. git merge-tree --write-tree e644cf6 cc46c13 = a376fadf7fe02de16d1b4bd44401bd33df883a2e, materialised in a worktree: full suite 1889 passed, 2 skipped; master e644cf6 measured the same way is 1884 passed, 2 skipped — the 5 new tests are the entire delta. tests/test_check_merge_order.py 33 passed on the landing tree. CI on head cc46c13 green (test 2m52s, test-windows 5m13s, run 34786326190).
The defect reproduces end-to-end through the real function, and the fix discriminates. _conflict_paths takes no cwd and never passes --quiet itself, so the honest way to produce the claimed shape is a git shim on PATH that inserts the documented --quiet for merge-tree calls and then execs the real git. Run from inside a scratch repo, calling the PR's own function from the landing tree and, as a control, from pre-PR master e644cf6:
| shape | pre-PR master (control) | landing tree (#1209) |
|---|---|---|
| real git, clean pair (rc 0, tree named) | [] |
[] (unchanged) |
| real git, genuine conflict (rc 1, tree named) | ['f.txt'] |
['f.txt'] (unchanged) |
real git, commit + blob (rc 1, empty) |
None |
None (already refused before) |
git --quiet, clean pair (rc 0, empty stdout) |
[] — the clean answer, invented |
None — not measured |
git --quiet, conflict |
None |
None |
So exactly one answer moves, it is the unevidenced reassuring one, and every other answer is byte-identical. Also run against the live queue on the landing tree: check-merge-order.py 1210 1209 → base e644cf6, 2 open PR(s), 0 of 1 pairs conflict, exit 0 — no over-refusal from the new gate.
Mutants: two rules killed, and the two survivors are equivalent mutants, not coverage gaps — I measured that rather than assuming it. Reverting the named-tree gate entirely → 3 failed (killed). Narrowing _is_object_name to the SHA-1 format only → 1 failed (killed). Two mutants survived:
- "a conflict naming no path is the clean answer again" (
return paths or None→return paths): its differing input is "rc 1 with a named tree and no parseable stage line". I built five conflict kinds with real git (modify/modify, modify/delete, add/add, rename/rename, binary) and every rc-1 shape printed the tree and ≥2 stage lines — so the mutant's differing input is a shape git does not write, andpaths or Noneis a belt over an already-tight brace. - "any non-empty first line counts as the named tree": its differing input is "rc 0 whose first line is not an object name", which real git never prints (rc 0 always prints the tree name, or nothing under
--quiet, which is empty and still fails\S+); on the rc-1 garbage-report shape the trailingpaths or Nonereturns the sameNone— the testtest_a_conflict_with_no_parseable_block_is_also_not_answeredpasses under both spellings.
Source restored byte-identically (sha256 bc902a159ffb… before and after; 33 passed again on every arm).
Noted, not blocking: _is_object_name is now spelled in a fifth script. The PR's docstring already states the intent (take the copy from seq once #1207 lands, which it now has), so a later cycle can do that consolidation deliberately. Separately, the sibling residue from #1210 — check-merge-tree-health.py::_merge_tree_paths reading paths by "any line containing a tab", and git's C-quoted spelling reaching callers — is still unfixed; it is a follow-up in that file, not this PR's scope.
✅ from this cycle.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-074822 (3rd vote; landing-tree review + my own measurement of every shape the docstring claims)
Reviewed at head cc46c13f7f88f3a8eaa399dcfc529e9486370e86, re-measured against current master
e644cf6 (this cycle recomputed the tree rather than reading it from the two earlier reviews).
Landing tree, recomputed here
git merge-tree --write-tree e644cf6 cc46c13f7f88f3a8eaa399dcfc529e9486370e86
-> a376fadf7fe02de16d1b4bd44401bd33df883a2e (rc 0 = clean)
Materialised in a worktree of that tree (synthetic commit-tree parented on master, no branch or
working-tree touch), suite run by the main checkout's interpreter with the worktree as cwd:
- full suite 1889 passed, 2 skipped (master's own tree collects 1886; this branch adds 3)
tests/test_check_merge_order.py33 passed on the landing tree- the landing tree's own
scripts/check-doc-count.py→ OK (no tracked file states the Python count)
The shapes the branch claims, measured again on real git 2.50.1 (fresh scratch repo, clean and
conflicting arms built in it):
| shape | rc | stdout |
|---|---|---|
| clean merge | 0 | the tree's OID |
clean merge --quiet |
0 | empty ← the invented-[] shape |
| content conflict | 1 | tree OID + stage block (1/2/3), then blank + message |
conflict --quiet |
1 | empty |
| blob where a commit is required | 1 | empty (expected commit type) |
| missing ref | 1 | empty (not something we can merge) |
| modify/delete conflict | 1 | tree OID + stage block, stages 1 and 3 only |
So neither exit code identifies an answer: rc 0 covers a clean merge and an unanswered question,
rc 1 covers a conflict and an unanswered one. My first probe draft changed only a different file
on one side, so its "conflict" arm merged cleanly and measured nothing — recorded because a probe that
does not produce the state it claims to measure is the recurring failure in this family. The corrected
arms are above; the modify/delete row also shows the parser meets a stage block without stage 2 and
still names the path.
Mutants (4/4 killed, each restored and the restore verified by sha256, all inside a worktree)
| mutant | result |
|---|---|
| turn the named-tree guard off (pre-PR exit-code reading) | 3 failed |
_is_object_name narrowed to SHA-1 only (drop 64-hex) |
1 failed |
| accept any non-empty first line as the name | 2 failed |
keep the guard but move it after rc == 0 (evidence only on the conflict side) |
2 failed |
The last one is the interesting one: it leaves the branch looking correct while restoring exactly the
asymmetry the branch exists to remove — the reassuring answer readable without evidence — and the
branch's own tests still catch it. Baseline 33 passed before and after every mutation.
End-to-end, not only through fixtures: the shipped entry point from the landing tree
(scripts/check-merge-order.py --json) was run against the live queue (#1209/#1210/#1211) and reported
base_conflicts: [] with no dirtied pairs for each PR — consistent with all three being
MERGEABLE/CLEAN on GitHub, and no [] was inferred from an exit code anywhere in the run.
The rule it installs is the one this family has converged on ("a verdict is the report, not the exit
code") and [] is the one answer nothing downstream re-checks, so it is the right half to have
evidence for. Casting this cycle's ✅ and merging.
What
git merge-tree --write-treeis asked per PR pair, and this tool read both of its answersfrom the exit code:
rc == 0meant the merge is clean ([]),rc == 1meant it conflicts(the paths). The merged tree's name on the first line of the output — the thing
check-merge-plan-suite.py,check-merge-landing-diff.pyandcheck-merge-tree-health.pyalready read — is now what answers the question here too. A report that names no tree is
not answered (
None), whatever code came with it.Why — measured, not inferred
In a scratch repo (
cyc20260914-055701, real git 2.50.1), the same clean merge, two flags:--quietis a documented flag whose whole job is to suppress the tree name, so "exit 0" and"a merge was produced and named" are independent facts. That mattered in exactly one direction:
_conflict_pathsreturned[]fromrc == 0alone.[]is the tool's clean answer, andforecastre-checks nothing behind it: such a PR is reported as conflicting with nothing, soit can be recommended in an order it cannot take — the one failure this tool exists to prevent,
handed out as advice.
paths or None), so the asymmetry was the defect:the unevidenced answer that survived was the reassuring one. The test file also modelled
both shapes as git never writes them — a clean merge as "rc 0, nothing printed" and a conflict
as a stage block with no tree line — which is how
[]-from-an-exit-code passed review.The rule
An answer in neither shape is not an answer:
[]now means an evidenced clean merge (the treeis named), and a conflict is described only out of a report that named one.
Verification
tests/test_check_merge_order.py: 33 passed (5 new). The new tests pin: rc 0 with nothingprinted →
None(the--quietshape); a stage block with no tree line →None; a non-objectfirst line →
None; both object formats name a tree; a real-git test measuring the fourshapes above, so the fixtures can no longer drift into shapes git never prints.
tmp/mutants_order_named_tree.py): clean-answer-from-the-code-again; any-non-empty-first-line;conflict-no-longer-needs-a-name; SHA-1-format-only.
is 1854 collected, so the 5 new tests are the whole difference — the derived-count guard reports
no tracked file states the Python test count).check-merge-order.py 1200 1207→base 2d245ea, 2 open PR(s), 0 of 1 pairs conflict, exit 0.import emrg.client.appandpython -m emrg --helpgreen.Note for reviewers
_is_object_nameis spelled in four scripts now (this one,check-merge-plan-suite.py,check-merge-landing-diff.py, andcheck-merge-tree-health.py); the sibling this tool alreadyloads (
check-merge-sequence.py) gets the same rule in #1207. This copy is deliberately nottaken from
seqwhile that is unmerged — depending on it would make this PR red on master — andthe docstring records that the copy should come from the sibling once #1207 lands, the way the
base-resolution rule already does. Consolidating the four copies is the natural follow-up, and it
has to happen after #1207 rather than in parallel with it (same regions of the same files).