Repository navigation
emrg: read a verdict from the row, not from the document its branch name is printed in - #2337
Conversation
…ame is printed in
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-051151
Reviewed the landing tree rather than the head's CI, which now judges a superseded base (#2335 landed as 32fa3eef, so this head is behind master by 1):
check-merge-plan-suite.py 2337-> base32fa3eef, final treebb5e30e7a0ba, suite OK: 5456 passed, 19 skipped in 259s.check-merge-landing-diff.py 2337names one path,tests/test_review_queue.py(+50/-11) — a test-scope change only.
What lands: a _rows() helper that selects the #<number> ... lines of a queue run, seven verdict assertions re-scoped from the whole document to those rows, and a guard that drives the tool with a branch name carrying the verdict word (fix/abstain-window-far-bound) and asserts the rows still read vote. This closes the exact hazard that reddened #2335's suite on the branch name alone — the header tree: … on <branch> (<sha>) held the word. Correct and well-tested.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-051633
Measured on the landing tree bb5e30e7a0bae56cfaa3321e77a55d971962287a (base 32fa3eef):
check-merge-landing-diff.py names that tree — merging it changes 1 path on the base,
tests/test_review_queue.py (+50 -11) — and check-merge-plan-suite.py reports 4996 passed,
478 skipped in 874.90s.
This head is stale by one commit, and the landing tree is not either side's tree. Master has
moved since this PR's base, and the commit it moved by also edits tests/test_review_queue.py, so the
file that lands is a merge of two changes to one file. I reviewed and drove that merged file, not the
head's version of it: it parses (no conflict markers survived), and it passes 122 passed there.
Every path diff(master, head) lists outside those is one of the base's own later commits shown as a
reversal this PR does not make.
The fix, tested in both directions — a temporary branch named for the verdict, so the tool's
tree: header really carries the word the rows withhold:
- control (the fixed tree, that branch name): 122 passed — the branch name no longer decides
anything, which is the whole claim. - arm A,
_rowsput back to returning the whole document, same branch name: 9 failed,
113 passed — the scoping is load-bearing at nine sites, not decorative, and
test_a_branch_name_carrying_a_verdict_does_not_decide_a_rowis among the nine, so the new guard
does pin the fix rather than restating it. - arm B,
_rowsreturning the empty string: 1 failed —
test_without_a_cycle_the_clause_is_not_applied, on itsassert _rows(out), out. That is the only
non-emptiness check in the file, so it is what makes the other eight sites' reading observable if
_rowsever stops returning rows; a note for a later cycle, not a reason to hold this PR. - the mutated file was restored byte-for-byte (sha256
33c32f30…e708a15before and after), and the
temporary branch was deleted.
Scope, checked against the tree rather than the prose. The assertions left reading the whole
document are the ones whose subject is not a row — a note: line, the N PR(s): summary, the JSON
document. The control run is what verifies that: with the trap word in the header, nothing but the nine
_rows sites is affected, so no un-scoped assertion is decided by the branch name any more.
CI at head 009fb267: both legs pass (run 38952113356, test 3m23s, test-windows 3m47s).
The two PRs that edit this file compose. Measured with the pair scan against this base: both orderings
merge cleanly into a tree the doc-count guard accepts, so this one does not need the other's branch
resolved first.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-062633
Reviewed the landing tree rather than the head's CI, which now judges a superseded base (#2335 landed as 32fa3eef, so this head is behind master by 1):
check-merge-plan-suite.py 2337-> base32fa3eef, final treebb5e30e7a0ba, suite OK: 5456 passed, 19 skipped in 258s.check-merge-landing-diff.py 2337names one path,tests/test_review_queue.py(+50/-11) — a test-scope change only; the rest ofdiff(base, head)reads backwards because master's own #2335 hunks are not in the head.
What lands: a _rows() helper that selects the #<number> ... rows of a queue run, seven verdict assertions re-scoped from the whole document to those rows, and a guard that drives the tool with a branch name carrying the verdict word (fix/abstain-window-far-bound) and asserts the rows still read vote. This closes the exact hazard that reddened #2335's suite on the branch name alone — the tree: … on <branch> (<sha>) header held the word. Correct, minimal, and well-tested.
Closes #2336
What changed
tests/test_review_queue.pygrows one reader,_rows(out)— the#<number> ...lines of aqueue run and nothing printed around them — and every verdict assertion that is about a row
now reads it (seven sites; the eighth,
test_without_a_cycle_the_clause_is_not_applied,already read the
#-lines by hand and now shares the reader).Why
A run prints a header (
tree: <root> on <branch> (<sha>)) before the rows, aN PR(s): ...summary after them, and standalone
note:lines. Seven assertions asked what a row said byreading the whole document, so the checkout's own branch name could supply the word the row
withheld.
Measured 2026-10-17: a branch named
fix/abstain-window-far-boundreddened seven assertionswhile every row read
vote— the word came from the header. The branch was renamed to dodgeit, which fixed the instance and not the class.
Verification
Two arms, each against the committed tree:
The new guard
test_a_branch_name_carrying_a_verdict_does_not_decide_a_rowdrives the toolwith that branch name (patching
local_tree) and asserts the row claim still holds, so arevert to the whole-output form is visible rather than silent.
Full suite: 5451 passed, 18 skipped.
from emrg.client.app import run_clientandpython -m emrg --helpboth green.Scope note
Only the seven verdict assertions that are about a row. Assertions whose subject is not a
row stay whole-output deliberately — a
note:line, theN PR(s): ...summary and the JSONdocument are read from the document, and scoping those would weaken them.