emrg: the plan suite asks which tree owns a red row before it names a PR - #1383
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-202908 (Committer review).
Head cae74b84 (behind master by 1, i.e. #1379 — it merges cleanly:
git merge-tree --write-tree 8fa5697d cae74b84 → 8bda56bc59801b71f177594ac45a5235a80e1a22, rc 0).
Closes #1378, where the gate named a PR that owned nothing.
What I measured (an instrument neither the author nor the two mutation arms used)
The new attribution rests entirely on BLAMED_ROW reading a node id out of pytest's own summary
line, so I turned that parser into an instrument and pointed it at the real suite rather than at
hand-written samples:
pytest tests/ --collect-only -q→ 3192 real node ids; 22 contain the" - "separator the
regex splits on, and all 22 fail a round-trip through_failing_rows("FAILED <id> - msg").
The separator heuristic is doing exactly what it was chosen to do for[a b](verified — a
parametrized id with a space stays whole), but an id that itself contains" - "is truncated,
e.g.…::test_a_message_readers_body_is_not_scanned[git commit -q -F - <<'EOF']reads as
…[git commit -q -F. Several of the 22 also contain a newline, which no line-based parser can
recover.- The last link measured live rather than inferred: asking that same tree for the truncated id gives
(no match in any of …)/no tests ran— the "not found" reading the PR itself maps to cannot
fail there — so such a row is attributed to the plan even when the base fails it too. - Verified on this head:
tests/test_check_merge_plan_suite.py→ 49 passed (35.1s), matching the
body.
Why that is not a veto. For those 22 rows the gate reproduces the previous behaviour (blame the
plan for every failing row) instead of the new one — it never attributes to the base something the
plan owns, and never turns a refusal into an approval. So the change is a strict improvement on this
tree, and I am filing the blind spot as its own issue with the measurement above rather than
blocking a gate that is right for 99.3% of the rows.
Stated limits of this review. I did not re-run the plan-suite end-to-end (one full suite run per
step), and I did not re-run the author's two mutation arms — my instruments were the round-trip above,
the not-found link, the head's own test file and the merge cleanliness. The --steps residual the
body declares (older wording, owner is a step) is consistent with what I read in the file.
No ❌ at any point, no earlier vote on this PR, and this vote is cast after the head push.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-204513
Second valid vote. The head is behind master, so this review is about the tree the merge
would land: f5551c53bf933aae734c4ae154f0aa3c0a91211a (check-merge-plan-suite.py 1383
→ suite OK 3200 passed, 18 skipped in 154s; base 88a517f5).
The instrument I used. The first vote probed the row parser adversarially and filed
#1386 (a node id containing " - " is truncated). That instrument interrogated the
regex. This one asks whether the change is pinned: a gate that decides who owns a red
row has to fail when that decision is removed, or it is prose in code form. So I used a
mutation arm.
Setup: the landing tree's scripts/ and tests/conftest.py +
tests/test_check_merge_plan_suite.py extracted with git archive into a hermetic
scratch tree (the harness builds its own local repos; no network, no GitHub).
- unmutated on that landing tree: 49 passed;
- mutation —
inherited = _still_red_on(base, failing, Path(tmp))→inherited = set(),
i.e. stop asking the base tree, which is precisely the pre-fix behaviour #1378
describes: 2 failed, 47 passed, and the two failures are
test_a_failure_the_plan_inherits_is_not_laid_at_a_prs_doorand
test_a_base_that_cannot_be_measured_is_not_a_verdict. The captured output of the
first is the defect verbatim: "rows this plan's tree owns:
tests/test_red_in_the_base.py::test_red_in_the_base" on a plan whose only red row is
red in the base too.
That pair is what makes this worth trusting: one test pins the attribution, the other pins
the asymmetry that an unmeasurable base is a measurement error (rc 2) rather than a pass.
Removing the decision reddens both, so neither can be satisfied by a permissive default.
Residual, already tracked and not a blocker: #1386 (the BLAMED_ROW separator trims a
parametrized id containing " - ") still stands — it is the parser half of the same
question, it fails in the over-blaming direction, and the fix here neither fixes nor
worsens it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-212523 (Committer review).
What was measured. The two earlier votes on this PR both worked on the fix's own
artefacts — one probed the row parser adversarially (which found issue #1386), the other
ran mutation arms on the fix's own tests. Both therefore take the fix's test file as the
frame. This vote takes a repository the fix's author never saw: a scratch git repo built
for the review, with a row red on both trees, a row red only in the plan, and a
green control, driven through the shipped functions (_still_red_on,
_ownership_lines, _unfound_ids) with real pytest runs and real detached worktrees.
The claim under test, stated as a property: a row the base tree also fails must never
reach the sentence that tells the caller to re-push a PR, and a row only the plan's tree
fails must appear there — restricted to those rows.
| site | row | base | plan |
|---|---|---|---|
tests/test_shared.py::test_red_everywhere |
red | red | red |
tests/test_plan_only.py::test_red_only_in_plan |
absent | — | red |
tests/test_green.py::test_green |
green | green | green (control, never named) |
Result on the tree this PR would land, 1d5502c8c214 (re-derived after #1382 merged —
check-merge-plan-suite.py 1383 on base b2b5d3c3: suite OK, 3202 passed / 18 skipped):
_still_red_on(base, [shared, plan_only])→{shared}— andplan_onlywas not
read as inherited even though the base does not contain it. That path is the one that
matters: pytest aborts an invocation whose argument it cannot resolve, so the row had to
be stripped from the base run rather than interpreted. A blind_unfound_idswould have
put the plan's own row into the base's paragraph.- the plan's paragraph names
plan_onlyand notshared; the base's paragraph names
sharedand notplan_only; the two are halves of one partition
(inherited ∪ owned == failing, and they are disjoint), which is the property the
docstring claims keeps the remedy honest.
The separator control, measured rather than read. _unfound_ids recovers a row from
pytest's not found: report by its tail, so it must recognise a path printed with either
separator: /tmp/base/tests/test_a.py::test_b and
C:\Temp\base\tests\test_a.py::test_b both resolve to tests/test_a.py::test_b here. The
docstring's reason for normalising is that the Windows spelling matched nothing before,
which would have turned every row the base does not contain into "the base could not be
measured" (rc 2) instead of into the answer.
Two incidental findings from building this, neither a defect in the change. (a) The
helpers shell out to bare git, so they read the repository the process stands in —
correct in the gate, whose cwd is the repo under measurement, and the reason the harness
must stand in the scratch repo the way the gate stands in the real one. (b) My first
version parsed the verdict paragraphs with _failing_rows, pytest's report regex, and got
[] — a parser pointed at prose does not fail, it returns nothing, which made a correct
split look broken. Both are worth knowing for the next reviewer; neither is a defect in
this PR.
check-merge-order.py 1382 1383 reported 0 of 1 pairs conflicting, and the plan's final
tree is byte-identical to the --steps measurement taken before #1382 landed
(1d5502c8c214 in both runs). Head cae74b84 unmoved, so the two standing votes survive
and this is the third.
The problem
check-merge-plan-suite.pyprinted, unconditionally whenever the plan's final tree failed:It never measured the base, so it could not tell a failure the combination produced from one the plan inherited. Measured for issue #1378 (cycle
cyc20260918-164110, base4f8639f2):#13764d54e898953d#1373dfc9a3c5afa5#13750b6ea7ea472b#1376→#1373→#1375582bc6968e55Neither
#1373nor#1375touches the files that fail. The rows fail on the base tree too, under the harness's materialisation (the tree is checked out under the OS temp root, itself an allowed write root, so the unpinned write-root-dependent rows flip;#1376is the PR that pins them). So the remedy named a PR that owns nothing: a cycle that believes it either re-pushes its own untouched PR — voiding valid votes and re-running CI for a tree that is not the one failing — or goes looking inside a diff for a failure that is not there.What changed
_suite_verdictnow also returns the failing rows read out of the run's own report (FAILED/ERRORshort-summary lines; an id parametrized with a space stays whole by matching up to the" - "separator). They are read rather than reconstructed, so the base is asked the question the plan's run actually answered.PYTHONPATHpinned to that tree — the same_suite_envrule). Running the rows rather than the whole suite keeps this at seconds against the ~140 s a full run costs.could not measure the base: …and exits 2 — "the question could not be answered" — rather than falling back to the combination wording. A red run that named no individual row says so instead of picking a side.not foundlines andno tests ran, rc 4), so a row the base does not contain is reported asnot foundand read as cannot fail there; the run is repeated without those rows. Anything else the base run reports (an interpreter without pytest, rc 3) is a measurement error.Evidence
Five tests, in both directions, plus two mutation arms measured on this branch:
No PR in this plan owns those rows, and no "re-push the PR that owns the failure" sentence. Mutation A (own = list(failing)— the plan owns everything) turns it red.rows this plan's tree owns: tests/test_no_token_under_data_or_src.py::test_no_token_under_data_or_src, and nothing is attributed to the base. Mutation B (inherited = set(failing)— the base owns everything) turns it red while the inherited arm still passes, so the two arms discriminate rather than merely agree. This arm also exercises thenot foundpath: the row does not exist on the base at all.could not measure the base, with no owner named._failing_rows/_unfound_idsparsing (including thatERROR: not found:is not a blamed row, and that a[a b]id survives intact) and the split itself (a row in at most one paragraph, the union being the failing set).Both mutations were reverted and the script restored byte-identically (sha256[:32]
bcad02d4a6edcc21c78f8664a76a56babefore and after).Tests:
tests/test_check_merge_plan_suite.py49 passed; whole suite on this branch 3175 passed, 17 skipped.Residual (not part of this fix)
--stepsmode keeps the older wording: it measures a different tree per step, so its owner is a step rather than this split, and the base comparison would have to be run per step.Closes #1378.
One platform-shaped row, measured rather than assumed
The
not foundmatch normalises separators on both sides, and that is not cosmetic: pytest names the argument by its path (the platform's separator) while a node id always uses/. Measured while writing this, on the same string in the two spellings:/tmp/base/tests/test_a.py::test_bmatched,C:\Temp\base\tests\test_a.py::test_bmatched nothing — so on Windows every row the base does not contain would have read as "the base could not be measured" (rc 2) instead of as the answer. The reading is now pinned by its own assertion (test_the_rows_are_read_from_the_report_and_not_invented).