Skip to content

emrg: read a verdict from the row, not from the document its branch name is printed in - #2337

Merged
argszero merged 1 commit into
masterfrom
fix/row-scoped-verdicts
Oct 16, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/row-scoped-verdicts

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2336

What changed

tests/test_review_queue.py grows one reader, _rows(out) — the #<number> ... lines of a
queue 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, a N PR(s): ...
summary after them, and standalone note: lines. Seven assertions asked what a row said by
reading 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-bound reddened seven assertions
while every row read vote — the word came from the header. The branch was renamed to dodge
it, which fixed the instance and not the class.

Verification

Two arms, each against the committed tree:

  • the unfixed tree, on a branch whose name carries the verdict: 7 failed, 109 passed
  • the fixed tree, the same branch name: 117 passed

The new guard test_a_branch_name_carrying_a_verdict_does_not_decide_a_row drives the tool
with that branch name (patching local_tree) and asserts the row claim still holds, so a
revert to the whole-output form is visible rather than silent.

Full suite: 5451 passed, 18 skipped. from emrg.client.app import run_client and
python -m emrg --help both 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, the N PR(s): ... summary and the JSON
document are read from the document, and scoping those would weaken them.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 -> base 32fa3eef, final tree bb5e30e7a0ba, suite OK: 5456 passed, 19 skipped in 259s.
  • check-merge-landing-diff.py 2337 names 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 pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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, _rows put 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_row is among the nine, so the new guard
    does pin the fix rather than restating it.
  • arm B, _rows returning the empty string: 1 failed —
    test_without_a_cycle_the_clause_is_not_applied, on its assert _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
    _rows ever 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…e708a15 before 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 -> base 32fa3eef, final tree bb5e30e7a0ba, suite OK: 5456 passed, 19 skipped in 258s.
  • check-merge-landing-diff.py 2337 names one path, tests/test_review_queue.py (+50/-11) — a test-scope change only; the rest of diff(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.

@argszero
argszero merged commit bbbd75e into master Oct 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: a queue run's verdict assertions read the whole document, so a checkout's branch name can decide them

2 participants