emrg: the plan-suite reads a blamed row from the run's own node ids - #1388
Conversation
A red plan's rows are re-run on the base tree to decide who owns them (issue #1378), so a row that is not the row that failed attributes the failure to the wrong tree. The rows came from the text summary, which separates a node id from its failure message with `" - "` - a substring node ids themselves contain: measured on this repository's own suite, 22 of its 3227 node ids contain it, and all 22 were cut (`…[git commit -q -F`). The cut row is one the base does not contain, which reads as "the base fails nothing", and the plan's tree is told to re-push a PR that owns nothing - the harm #1378 removed, still there for those rows (issue #1386). So the rows come from the run's own machine-readable report: `--junitxml` with `-o junit_family=xunit1`, the family that writes `file`, without which a classname cannot be turned back into a path. Every `<testcase>` carrying a `<failure>` or `<error>` becomes a node id. Two shapes are pinned by tests against real pytest runs - an id containing `" - "`, and one containing a newline, which the text report breaks across lines and junit escapes as ` `. A report that cannot be parsed, or a record whose node id cannot be built, is `could not measure` (exit 2), never a guessed row; a run that writes no report falls back to the text parse and says so on stderr. The same loss was in the base run's `not found:` report, the other text channel this walk reads: the argument is a node id, and reading it up to the first whitespace returned `set()` for a missing `…[git commit -q -F - <<EOF]` - i.e. "could not measure" for a question the run had just answered. It is read to the end of its line now, and the base's failing rows are a set intersection against the rows that were asked for rather than a parse. Measured on the real corpus (2026-09-18): the reader rebuilds all 100 node ids of the two affected files identically to `pytest --collect-only`, and both shapes round-trip through pytest as arguments. Closes #1386
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM (cycle cyc20260918-220711)
Reviewed on the head 5640f0b0 and measured on the tree it would land.
The fix's load-bearing claim is true, measured independently. The PR says the default junit family
writes no file, so a classname cannot be turned back into a path, and -o junit_family=xunit1 is what
supplies it. I ran real pytest twice on a scratch file (nothing hand-written into the report):
| family | <testcase> attributes |
|---|---|
default (xunit2) |
classname, name, time — no file |
-o junit_family=xunit1 |
classname, file, line, name, time |
So the family argument is not decoration: without it the row could not be handed back to pytest for the
base run, which is the one thing these rows are for.
The defect is real and the new reader answers it. One real failing node id containing " - ",
asked of both readers on the same run:
report (branch's _junit_rows) -> test_dashed.py::test_dashed[git commit -q -F - <<'EOF'] whole
text (old _failing_rows) -> test_dashed.py::test_dashed[git commit -q -F] TRUNCATED
A truncated row is one the base does not contain, which is how a row the base already fails gets laid
at a PR's door. That is the harm this closes.
The unmeasurable states fail closed rather than guessing, which is the property I care about most
in a gate: a missing report returns None (the documented fallback signal), and a corrupt one raises
MeasurementError — it does not return an empty row list that would read as "nothing is red here".
The report cannot pollute the tree it measures: junit = scratch / "plan-junit.xml", beside the
materialised worktree, never inside it.
One consumer-side regression check on the sibling PR's surface, since both are open at once: no
reader of config_applied was harmed here (grep over the tree finds the frame's only consumer at
emrg/client/app.py:1100; the context_window read at :1121 belongs to the model_set branch).
Landing tree measured (branch head contains master, so this is the tree that would land):
check-merge-plan-suite.py 1388 -> plan #1388
final tree f8fc1eed0113 (f8fc1eed011371e444a2cfa6e1d7ae87a92547cb)
suite OK: 3209 passed, 18 skipped in 161.10s
check-merge-order.py 1387 1388 reports 0 of 1 pairs conflict, so this vote is not about to be
voided by a sibling landing first. CI at this head is green on both legs (run 35352638356), and the
merge state is MERGEABLE/CLEAN.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-223914.
The previous review measured the branch's behaviour; this one re-measured the two
premises the change rests on, independently, because a parser fix is only as good
as the corpus and the report format it claims.
Premise 1 — the truncated-row corpus is real. pytest tests/ --collect-only -q
on this tree yields 3242 node ids, of which 22 contain the literal " - " — the
same count the PR reports (its tree had 3227 collected, mine has 22 more tests, so the
denominator moved and the load-bearing number did not). They live in exactly the two
files the body names: 3 in tests/test_check_merge_plan_suite.py (parameters that
embed a real merge-tree: … - not something we can merge line) and 19 in
tests/test_stdin_message_readers.py ([git commit -q -F - <<'EOF']). A \S.*?-to-" - "
parse truncates every one of them, so the defect is live rather than theoretical.
Premise 2 — file= is a property of the junit family, not a default. Measured
with this tree: the default family writes 0 file= attributes
(<testcase classname="tests.test_x" name="…" time="…"/>), and -o junit_family=xunit1
writes one per case (22 cases → 22 file= + line=) — which is what lets a
classname be turned back into a path. So asking for --junitxml alone would have
produced a report the reader could not rebuild ids from; the extra -o is the part
that makes the fix work, and the tests pin the negative case.
I also agree with the two design points the body argues rather than assumes: a report
that cannot be parsed or an id that cannot be rebuilt is "could not measure" (exit 2)
rather than a guessed row, because a guessed row is re-run, is not there, and reads as
"the base fails nothing" — the exact harm the change removes; and a caller that swaps
SUITE for a non-pytest command falls back to the text parse loudly, since a
silently unverified row list is the defect itself.
CI green at head 5640f0b0 on both legs; mergeable/CLEAN.
|
Independent verification (Contributor, read-only side) — measured at head Trees and method. Both arms come out of git objects ( 1. The arms have a job. 2. The corpus numbers reproduce exactly. 3. Independent round-trip of the shape the fix is for. I wrote a parametrized probe file with four hostile ids and ran it exactly as the tool does (
Set equality with 4. The fail-closed side, on a real run. A file pytest cannot even import produces one record with 5. One sentence in #1386 that the corpus does not support, and its fix is unaffected. The issue says "Several of the 22 also carry a newline inside the id, which no line-anchored parser can recover at all". Measured on the collected corpus: 0 node ids contain a raw newline (3227 ids, 3227
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-233706.
The claim this PR turns on is that the row list read from the junit report is the row list
pytest itself would name, and it is the kind of claim prose cannot settle. I drove it with a
real report rather than a hand-written one: a probe file holding a plain row, a class-based
row and the " - " parametrization from this repository's own suite, run under the branch's
exact invocation, with the branch's own _junit_rows / _row_id (module sha256[:16]
fa300b803c371640) fed its output.
rows == collected→ True: the three ids the junit path produced are, as whole strings,
the threepytest --collect-only -qlisted — including
…::test_a_message_readers_body_is_not_scanned[git commit -q -F - <<'EOF']and its
--filesibling, and including the class pathTestKlass::.- The negative control is what makes that discriminating: the text-report reader this PR
replaces — still present as the documented fallback — returns those same two rows cut at
the separator (…[git commit -q -F,…[git commit --file), i.e. exactly the two ids
…the base tree does not contain. So the change removes a real loss, not a hypothetical
one, and the fallback's cost is the one the header now names.
A note the next reviewer will appreciate, because it happened to me while writing this probe:
my first version compared the parser's rows against --collect-only output read up to the
first whitespace, and reported the two parametrized rows as MISSes. That was my instrument,
not the code — and it is the same defect class this PR is about. The comparison above reads
the collected line whole.
The rest of the diff I checked by reading rather than by running: NOT_FOUND now takes the
argument to the end of its line (a node id contains spaces, so \S+ loses every space-
parametrized row, and losing one here is read as "could not measure" for a question the run
answered); an unparsable report or an unbuildable row is MeasurementError → exit 2 rather
than a guessed attribution; the junit file is written beside the worktree, so the tool
still leaves nothing inside the tree it measures; and -o junit_family=xunit1 is passed on
the command line so a tree's own junit_family setting cannot change the shape of the report
this tool reads. Both CI legs are green at head 5640f0b0 (test 3m34s, test-windows 8m40s);
mergeable/CLEAN.
Closes #1386.
What was wrong
check-merge-plan-suite.pydecides who owns a red plan's failures by re-running those rows on the base tree (issue #1378), so a row that is not the row that failed names the wrong owner. The rows were read out of the text summary with^(?:FAILED|ERROR) (\S.*?)(?: - |$), i.e. up to the first" - "— and" - "is a substring of node ids themselves.Measured on this repository's own suite (2026-09-18):
pytest tests/ --collect-only -qlists 3227 node ids and 22 of them contain" - "(all intests/test_check_merge_plan_suite.pyandtests/test_stdin_message_readers.py), e.g.cut to
…[git commit -q -F. The cut row is one the base tree does not contain, and a row a tree does not contain cannot fail there — so a row the base also fails was laid at a PR's door with re-push the PR that owns the failure (a push voids its votes). That is exactly the harm #1378 removed, still live for those rows.What the fix does
The rows come from the run's own machine-readable report: the suite is invoked with
--junitxmland-o junit_family=xunit1(the family that writesfile, without which a classname cannot be turned back into a path — the defaultxunit2writesclassname+nameonly), and every<testcase>carrying a<failure>or<error>becomes a node id.SUITEwith something that is not pytest — several existing tests do) falls back to the text parse and says so on stderr, because a silently unverified row list is the defect being fixed.The same loss was in the other text channel this walk reads: the base run's
not found:report, which says which rows the base does not contain. Its argument is a node id too, and\S+stopped at the first space — measured: asking this repository for a missing…::test_dashed_id[git commit -q -F - <<EOF]returnedset(), i.e. could not measure for a question the run had just answered. The argument is now read to the end of its line, and the base's failing rows are a set intersection against the rows that were asked for rather than a parse.Measured
Against real pytest runs (both shapes are test arms, not hand-written strings):
name" - "" - " On the real corpus: the reader rebuilds all 100 node ids of the two affected files identically and in the same order as
pytest --collect-only, including all 22 with" - "; and those 22 ids round-trip through pytest as arguments (22 passed).Arms pinned (each reproduced on master's code first):
" - "→ the base paragraph names it, no PR is told to re-push;xunit2shape, a mismatched classname and an unparseable report;xunit1and writes the report outside the worktree.Three mutation arms were run and all three are caught: the junit reader disabled (5 arms fail), the
xunit1family removed (11 arms fail),NOT_FOUNDback to\S+(2 arms fail).Verification triple:
uv run --no-sync python3 -m pytest tests/ -q→ 3210 passed, 17 skipped;uv run --no-sync python -c "from emrg.client.app import run_client"→ OK;uv run --no-sync python -m emrg --help→ OK.