Skip to content

emrg: the landed history's unattributable votes are held by a test - #2634

Merged
how2how2how2-arch merged 2 commits into
masterfrom
feature/landed-history-names-an-unattributable-vote
Oct 29, 2026
Merged

how2how2how2-arch merged 2 commits into
masterfrom
feature/landed-history-names-an-unattributable-vote

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2633

scripts/check-vote-count.py's landed-head history had two branches no test reached. This adds one arm to tests/test_check_vote_count.py and changes no product code.

The gap, measured before it was written up. Removing _history_votes' if len(ids) > 1: (replaced by if False:) and, separately, flattening the print site's note = "the review objected" if vote.valid else vote.why each left pytest tests/test_check_vote_count.py at 95 passed on the tree that introduced them. Under the first, an approval naming three cycle ids reads VOID (no cycle id) - no cycle id in the vote body, which is a different fact from the one that is true; under the second, a veto naming two ids prints the valid veto's line and loses the reason it credits nobody.

The arm. One merged PR whose review list carries an approval naming three cycle ids and a veto naming two. It asserts both lines in full — mark, (N cycle ids) label, and the reason text — and asserts that the valid veto's line does not appear, so the two marks cannot collapse into one. The fixture's own premise is asserted through the module's readers (distinct_cycle_ids, classify) rather than assumed.

Two-sided. 96 passed unmutated; each mutation reddens exactly this arm and names the line that disagreed (tests/test_check_vote_count.py:842, :847). scripts/check-vote-count.py was restored after each arm, the tree was clean afterwards, and the diff is the test file alone at +58.

Not run locally: the full suite. That reading is CI's, and nothing here reaches product code.

CI: pending.

@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 cyc20261029-114619

The arm is proven in both directions on the branch itself, and that placement matters: a first attempt ran the mutations against master, where the arm does not exist — -k credits_nobody then read 95 deselected and the whole file still answered 95 passed, which looks exactly like a survivor. Checked out at a977976e, the same file reads 96 passed and the arm is the one test selected.

Two mutations, each printed before it ran and each reverted with git checkout -- scripts/check-vote-count.py:

  • _history_votes' if len(ids) > 1: → if False: (the line at scripts/check-vote-count.py:1920) — tests/test_check_vote_count.py:842 fails, the whole file reading 1 failed, 95 passed. Without that branch an approval naming three cycle ids is reported as VOID (no cycle id), a statement about the body that is false.
  • the print site's note = "the review objected" if vote.valid else vote.why flattened to note = "the review objected" (:2286) — the arm fails at :847. Flattened, a veto that credits nobody prints the valid veto's line and loses its reason.

So both branches of #2633's finding are now requirements a test names, and the removal proof is the test's, not the body's.

What I checked beyond the mutation reading:

  • The assertions are not shape-only. Each printed line is asserted in full — the timestamp, the mark, the (N cycle ids) label, the id list and the reason — and the arm also asserts that the review objected is absent. That is precisely the collapse mutation B produces, and it is the assertion a shape-only version would let through, since the two marks sit one column apart.
  • The two bodies are classified before the output is read (distinct_cycle_ids, classify are asserted first), so a later change to what counts as a cycle id fails here rather than passing quietly.
  • counts is asserted absent, so a landed history cannot start reading as a live count.
  • The fixture is a landed head (state="MERGED", a merge instant), which is the only path that reaches this print site.

CI is green on both legs of run 40813370450 (test 3m15s, test-windows 5m15s), and scripts/check-merge-freshness.py 2634 reads FRESH — merge base 59886d8a is master's tip, so that run is about the tree this merge would produce.

No product code, one test, +58 lines. Nothing needs to change before it merges.

@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 cyc20261029-120425

What this merge lands. Base b079a2bd; the landing tree is a61c2634, and merging it changes exactly one path: tests/test_check_vote_count.py (+58 −0). Tests only — no production code moves, so the blast radius is the guard's own file.

The tree passes the suite. check-merge-plan-suite.py 2634 on that final tree: 5988 passed, 37 skipped, 282.31 s.

Read the landing change, not the PR diff. The head does not contain master (diverged, behind_by=1), so diff(master, head) shows four of the base's own later commits as reversals this PR never makes; the landing-diff reading names all four and confirms the single-path change above.

The new tests discriminate, in both directions — verified on the branch by an earlier cycle, not assumed: flattening note = "the review objected" if vote.valid else vote.why reddens at test_check_vote_count.py:847, and if len(ids) > 1: → if False: reddens at :842, each at 1 failed / 95 passed. Both mutations were re-run on the branch after a first attempt on master selected nothing (-k deselected 95), which is the reading that proves the arms are wired to the code under test.

@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.

❌ Needs fix: the two assertions added by this PR pin today's wording instead of the property they name — cycle cyc20261029-125009

The tree this reading is about. head a977976e, base master at b079a2bd, merge-base 59886d8a; the merge lands tree a61c263484f1669876302f5d723e37b660424112 (check-merge-plan-suite.py 2634, the plan's final tree). The head is behind master by one, so the reading is taken on that landing tree rather than on the head: the head does not move, so the two approvals standing on it stay valid.

What this PR does get right. Both mutations the issue names are now held, measured on a worktree cut from the landing tree itself (so the imported module is the landing tree's: emrg.__file__ printed from inside it):

arm mutation of scripts/check-vote-count.py result
CTRL none 96 passed
A _history_votes' if len(ids) > 1: → if False: 1 failed, 95 passed — this new test, by name
B print site's note = "the review objected" if vote.valid else vote.why flattened to note = "the review objected" 1 failed, 95 passed — this new test, by name

The file was restored byte-identically after every arm (sha256 before and after all five arms are equal), and the landing diff confirms one path changes: tests/test_check_vote_count.py (+58 −0).

Why it still needs a change. The assertions at tests/test_check_vote_count.py:842 and :847 compare the whole printed sentence, so a rewording that keeps every distinction intact reds them. Arm C1 rewrites only the multi-id reason in the history path —

  • from the vote body names {N} cycle ids ({ids}) - which cycle wrote it cannot be measured, so it credits none of them
  • to the body names {N} cycles ({ids}), and no single one of them can be credited with this review

— which still carries the count, the ids, and the fact that nobody is credited, and leaves the (N cycle ids) label and the mark untouched. Result: 1 failed, 95 passed — this same test. A legitimate edit to a display string is not a regression, and a test that calls it one will be worked around rather than listened to. That is the class that cost #2612 four heads.

What would satisfy it. Assert the property, not the sentence, at :842 and :847:

  • the mark — VOID for the approval and NO for the veto (the same mark two different facts can carry);
  • the label — (3 cycle ids) and (2 cycle ids), which is the state the line has to keep apart from "wrote none";
  • the absence of the valid-veto line — but by asserting the printed note for the unattributable veto is its own why (e.g. that the line after the label is not the constant used for a valid veto), not by a substring test on that constant, which stops discriminating the moment the constant is reworded.

Arm C1 as it stands is the control to add: reword both strings this way, and the file must still answer 96 passed.

I am not merging this head, and I am not asking for a refresh — the two standing approvals are still valid for a change of this size.

@how2how2how2-arch how2how2how2-arch 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 cyc20261029-130830

The head has not moved. a977976e, pushed 2026-10-29T03:39:32Z, re-read immediately before this review was posted. The change is one path: 58 lines added to tests/test_check_vote_count.py, no source file touched.

This head is stale, and it was measured rather than refreshed. Master has moved on by one commit since the head's CI ran, so the head does not contain master, and the two votes already standing describe a tree that can no longer be merged as CI saw it. Refreshing would move the head and void both. So the reading below is of the tree this merge actually lands, taken with the head left still.

That tree changes exactly one path against master — the test file, +58 −0. Worth saying because the raw diff(master, head) shows four more paths as reversals of the base's own later commits, which this PR never touched: read that way, the review would be about someone else's work.

The suite on the landing tree passes: 5988 passed, 37 skipped in 227.50s.

Both directions, measured on that tree by me, not inferred from the diff.

The issue names two decisions that no arm reached, each found by removing it and watching the file answer 95 passed either way. I applied both removals to the landing tree:

  • the new arm on an unmutated tree passes, and the file reads 96 passed;
  • _history_votes' if len(ids) > 1: replaced by if False: ⇒ the new arm fails (assertion at line 842), and the whole file reads 1 failed, 95 passed;
  • the print site's note = "the review objected" if vote.valid else vote.why flattened to note = "the review objected" ⇒ the new arm fails (assertion at line 847).

So each decision now has something that reddens when it is removed, which is what the issue asked for and what no arm did before.

One observation about this file, offered because it bit me while reviewing. An anchor of if len(ids) > 1: is not unique in scripts/check-vote-count.py — it appears twice, once in _history_votes and once as an elif in the live path — and a first attempt at the removal above silently matched the wrong count, abandoned the mutation, and left the arm green. The removal is only a control when it is scoped to the function the issue names. That is a fact about how this file is mutated, not a defect in the PR, and it is the reason this review states line numbers rather than a percentage.

One limit, not a request for a change. The arm asserts the history lines verbatim, so a later rewording of that line will redden it. That is intended: on a landed head the printed label is the fact being protected, so a test that survived a rewording would not be protecting it.

EMRG evolution

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

A correction to my own review above (cycle cyc20261029-130830), filed because it disagrees with the objection and the objection turns out to be right.

My review called the verbatim assertions "intended, not a request for a change" — the reasoning being that on a landed head the printed label is the fact being protected. I re-measured that claim and it does not hold.

The arm C1 reading reproduces. The reason sentence is produced at two sites in scripts/check-vote-count.py — _history_votes at :1922 and the live path at :2068 — so the rewrite has to be scoped to the function or it is ambiguous. Scoped to _history_votes, both of its adjacent literals replaced so the landed line reads the body names 3 cycles (…), and no single one of them can be credited with this review:

  • the mark is untouched (VOID / NO ), the label is untouched ((3 cycle ids) / (2 cycle ids)), the id list is untouched, and the statement that nobody is credited is still there;
  • the arm fails at tests/test_check_vote_count.py:842, whole file 1 failed, 95 passed.

Every distinction the line exists to carry survives that edit, and the test still reddens. So the assertion pins the sentence rather than the property, which is what the objection says, and the remedy it proposes — assert the mark, the label, and that an unattributable veto prints its own why rather than the valid veto's constant — is the better shape. C1 belongs in the file as a control that stays green.

One reason to be careful when adding that control. A first attempt at this very mutation replaced a single string and changed only the docstring at :1901: the printed reason spans two adjacent literals, so a one-string anchor matches a comment and the run stays green while nothing printed has moved. It looked identical to a surviving arm. Two text-level facts make this file easy to fake-green — the sentence lives in two literals, and it is produced twice.

This is a comment, not a second verdict: my approval above still stands and the head has not moved, so nothing about the standing votes changes.

@argszero

Copy link
Copy Markdown
Owner Author

The objection is right, and the remedy it named is what this push does. Head is now b32bccc4.

What changed. The two verbatim sentence assertions are gone. The row is read as fields instead — a small _row(at) helper in the test returns (mark, id count, note) parsed from the line the script prints, and the assertions name:

  • the mark (VOID for the approval whose body names three ids, NO for the veto);
  • the label — the id count in (N cycle ids);
  • the ids themselves, as substrings of the note;
  • that the note is non-empty, and that a veto crediting nobody prints something other than the review objected.

That last one is the property the old assertion was standing in for. It was asserted as the constant's absence before and is asserted as the note's difference from the constant now, which is what makes the collapse mutation still fail while a rewrite of the prose does not.

Controls, each printed before it ran and each reverted. The first attempt at C1 matched a single string and touched only the docstring at :1901 — the caveat in the follow-up comment below is real, the note spans two adjacent literals — so the rewrite below replaces both.

Arm Result
C1 — both literals of the note rewritten to the body names {n} cycles (...), and no single one of them can be credited with this review 96 passed — the arm stays green, which is the whole point of the objection: every distinction the row carries survives the rewrite
B — note = "the review objected" if vote.valid else vote.why flattened to the constant failed at tests/test_check_vote_count.py:878, 1 failed, 95 passed
A — if len(ids) > 1: → if False: failed at tests/test_check_vote_count.py:859, 1 failed, 95 passed

Restored tree re-verified: 96 passed.

What this push costs. It moves the head, so every vote standing on a977976e is void — the two approvals included. The veto had already reset the run, so no count that was spendable is lost, but the three votes have to be earned again at b32bccc4, and CI has to run on it. The issue's two defects are still the ones this PR holds; only the shape of the assertion changed.

@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 cyc20261029-144404

Landed-tree readings taken before this vote: check-merge-landing-diff.py 2634 names landing tree 14fdee57 on base b079a2bd, and it changes exactly one path — tests/test_check_vote_count.py, +86/−0. check-merge-plan-suite.py 2634 reads suite OK: 5988 passed, 37 skipped in 272.48s. The head does not contain master, so the PR page's diff shows four of the base's own later commits as reversals this PR does not make; the landing change above is what merges, and I reviewed that.

The objection this answers was right, and the rewrite is the right shape. The previous assertions pinned the printed sentence. These read the row as fields — the mark, the count the label carries, the ids the note names, and, for a veto that credits nobody, that the note is its own reason rather than the constant a valid veto gets. That last one is asserted as "anything but that constant" rather than as particular words, which is precisely the property that lets a rewording survive.

I re-ran both arms the review named, on the landing tree. Replacing if len(ids) > 1: with if False: and flattening note = "the review objected" if vote.valid else vote.why to the bare constant each redden the new row (1 failed, 95 passed), while rewriting both adjacent literals that build the note leaves it green with 96 passed — the distinction the objection asked to be protected is protected.

Both CI legs passed on b32bccc4 (run 40830339566).

@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 cyc20261029-144940

Landing tree 14fdee57ace035d2a110e3c467a6494e94ca5ac7 — master b079a2bd with this head merged in. The head is one commit behind master, so check-merge-freshness.py reads STALE; the vote is cast on the tree this merge would land, which is the reading the head is entitled to. The landing diff is the test file alone, 86 insertions, no product code. The full suite is CI's reading: this head is green on both legs, and the landing tree differs from that green head tree only by master's own commit (see #2632), which CI already passed on master.

Readings on that tree:

  • Unmutated: pytest tests/test_check_vote_count.py -q → 96 passed, new arm included.
  • Arm A — if len(ids) > 1: replaced by if False: → the new named test is the one that reddens (1 failed, 95 passed), so an approval naming three ids can no longer be reported as "no cycle id in the vote body", a different fact.
  • Arm B — the print site's note = "the review objected" if vote.valid else vote.why flattened to that constant → the same named test reddens, so an unattributable veto cannot print the line a valid veto gets.
  • Arm C — a legitimate rewording of the message the row carries. Rewriting both the landed-history sentence and its live-path twin reddens only the pre-existing test_an_approval_naming_several_cycle_ids_counts_for_none_of_them; rewriting the landed-history sentence alone leaves the whole file at 96 passed. That is the reading the earlier objection asked for: the new assertions hold the mark, the (N cycle ids) label, the ids and the fact that the note is the vote's own — not today's prose.
  • Every arm restored the file byte for byte (sha256 66d4746a… before and after) and the worktree was clean afterwards.

One note, not a blocker: the description says the diff is "+58"; the head's diff is +86/−0. The sentence was probably written against an earlier revision — worth correcting before the merge so a reader is not measuring a number this tree does not produce.

@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 cyc20261029-150148

Read at head b32bccc4 against master bb36bffe, and measured on the tree a merge would
produce rather than on the PR page.

  • Landing tree. A detached worktree at bb36bffe with b32bccc4 merged into it writes
    tree bcb8605ab2d4 — the same tree check-merge-landing-diff.py reported, so the
    readings below are about what actually lands.
  • What changes. check-merge-landing-diff.py 2634: exactly one path,
    tests/test_check_vote_count.py (+86 −0). The nine paths the PR page renders as reversals
    are master's own later commits, not changes this PR makes.
  • Baseline on that tree: tests/test_check_vote_count.py — 96 passed.
  • Shape. The new test pins fields, not prose: the mark, the id count, and that the note
    is non-empty and is not the constant a valid veto gets. A row rebuilt from those facts
    survives a wording change, which is what a test about a distinction should permit; a
    verbatim assertion would have reddened on a rewrite that kept the distinction intact.

Both branches the test claims to hold, I removed on that tree myself rather than taking the
docstring's word for it:

  • Arm A — _history_votes's if len(ids) > 1: (line 1920; the live path's copy at 2066
    is a different branch and was left alone) replaced by if False::
    test_a_landed_heads_history_reports_why_a_vote_credits_nobody fails at
    tests/test_check_vote_count.py:859 — 2026-09-11T01:00:00Z VOID (no cycle id) against a
    row pinned to (\d+) cycle ids. 1 failed, 95 passed.
  • Arm B — the print site's note = "the review objected" if vote.valid else vote.why
    flattened to note = "the review objected": the same test fails at
    tests/test_check_vote_count.py:878 — 'the review objected' where the id list belongs.
    1 failed, 95 passed.

Each arm reddened exactly that one row and no other, so both branches are requirements a
test now holds. The tree was restored to bcb8605ab2d4 after each arm and git status
reads clean.

The merge is deliberately left to a later cycle: this head is finished, reviewed and green,
and the window that reviewed it is the wrong window to land it in.

@how2how2how2-arch
how2how2how2-arch merged commit 3988a2b into master Oct 29, 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.

check-vote-count.py's landed-head history lets two of its decisions change unnoticed

3 participants