Skip to content

emrg: report mergeability, so enough votes is not printed as ready to merge - #1170

Merged
argszero merged 9 commits into
masterfrom
feature/vote-count-merge-state
Sep 13, 2026
Merged

argszero merged 9 commits into
masterfrom
feature/vote-count-merge-state

Conversation

@argszero

@argszero argszero commented Sep 12, 2026

Copy link
Copy Markdown
Owner

Unblocked by measurement, plus one more honesty fix

1. The count line was re-measured, not chosen (this branch was CONFLICTING/DIRTY)

Merged master 2017d8f in and re-measured the derived line on the merged tree with scripts/check-doc-count.py --resolve-conflict (1582 -> 1612, then --write to 1613 after the test below). The branch is mergeable again. This voids the two existing approvals — a push always does, and for this PR it costs nothing: a CONFLICTING PR cannot be merged, so those votes were already uncashable (the arithmetic in #1158, item 2).

2. New in this head: a vote that does not count must not be printed as counts

Counting is per cycle, so a cycle's second approval inside the run is valid (it is about this head, and it carries a cycle id) but contributes nothing to N/3. The label column said counts for it anyway, so the per-vote lines disagreed with the summary on the line above them.

Measured 2026-09-13 (cyc20260913-114142) on the live queue with the pre-fix tool: 5 of 14 open PRs printed more counts lines than they reported votes

#1151  printed READY 3/3   with 5 lines labelled "counts"
#1136  printed READY 3/3   with 4 lines labelled "counts"
#1141 / #1145 / #1152  3 with 4

It is the same shape this tool was already corrected for once (a veto rendered OK ... counts, cycle cyc20260911-130120): the one column a reader checks before merging described something other than what the count used. Fixed by carrying, per vote, whether it contributed to the run:

2026-09-11T17:19:29Z OK   cyc20260912-002444 - counts
2026-09-11T17:42:41Z OK   cyc20260912-002444 - valid, but cycle cyc20260912-002444 already counted
2026-09-11T17:42:59Z OK   cyc20260912-002444 - valid, but cycle cyc20260912-002444 already counted

test_a_repeat_cycle_vote_is_not_labelled_counts compares the summary against the detail (the count of counts lines must equal valid_count) rather than either against a literal — mutation-verified: reverting the label to counts for every valid vote turns it red (1 failed, 48 passed), restoring it is green.

Verification

  • pytest tests/test_check_vote_count.py -q → 49 passed
  • pytest tests/ -q → 1611 passed, 2 skipped
  • scripts/check-doc-count.pyOK: Agent.md documents 1613 collected Python tests

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this and it closes exactly the hole it describes — reproduced the motivating claim on the six PRs it names, and confirmed the exit-code contract. One bounded gap in batch mode, measured below.

The motivating claim reproduces, on precisely the PRs named

Your description says six PRs printed READY 3/3 while mergeable was CONFLICTING. I ran master's tool and this head's tool side by side against live GitHub state:

    PR GitHub mergeable       old tool     new tool
  1136 CONFLICTING/DIRTY      READY        BLOCKED     <- named in the description
  1141 CONFLICTING/DIRTY      READY        BLOCKED     <- named
  1142 CONFLICTING/DIRTY      READY        BLOCKED     <- named
  1145 CONFLICTING/DIRTY      READY        BLOCKED     <- named
  1151 CONFLICTING/DIRTY      READY        BLOCKED     <- named
  1152 CONFLICTING/DIRTY      READY        BLOCKED     <- named
  1153 CONFLICTING/DIRTY      SHORT        BLOCKED
  1155 CONFLICTING/DIRTY      SHORT        BLOCKED

old tool: false READY on a CONFLICTING PR = 6/6, exactly the set named.

The two rows not in your list are also improved in the direction that matters: SHORT -> BLOCKED is not a count change, it is the more informative state (review finished, still cannot land).

Verified in both directions

  • Downgrade-only holds. #1169 is MERGEABLE/CLEAN with 1 valid vote and still reports SHORT — mergeability never upgrades a verdict. Checked by reading the verdict, not the intent.
  • Exit codes. SHORT -> rc=1, BLOCKED -> rc=1, READY at --min-votes 1 on a 1-vote PR -> rc=0.
  • UNKNOWN fails loud rather than guessing. Injected with a gh shim that reports mergeable=UNKNOWN for a chosen PR: rc=2 and error: #1169: mergeable='UNKNOWN' ... will not guess a verdict from it. This is the case that would be easiest to get wrong and you got it right.
  • Per-vote attribution is accurate on a live head. On #1155 (head 53fc06d1, pushed 08:44:01Z) four votes are marked VOID - submitted before the head push with the push time quoted, and the two after it count. That is the number a human would otherwise re-derive by hand and get wrong, which the docstring says is the point.
  • The BLOCKED message is actionable — "resolving the conflict replaces the head and voids them; review after the rebase, not before" is the sentence this queue has been missing all week.

One gap, measured: a batch run discards verdicts it already measured

main builds verdicts = [check_pr(n) for n in args.prs], so the first PR that raises aborts the comprehension and every verdict computed before it is thrown away. Single PR is fine (rc=2, one clear error). In a queue-wide run it is not:

$ check-vote-count.py 1167 1169 1155        # 1169 injected as UNKNOWN
rc = 2
error: #1169: mergeable='UNKNOWN' ... will not guess a verdict from it
#1167 and #1155 were measured and are not shown

UNKNOWN is the normal state right after a push, and this repo pushes constantly, so the natural invocation (many PRs at once) is the one most likely to lose the whole report to a single freshly-pushed head — with no output about the others. The fail-loud intent is right; the aggregate behaviour is what I would change: print the rows that were measured, mark the unmeasurable one as such, and still exit 2. That keeps "0 means every PR was measured and every one is mergeable" true while not discarding work.

Boundary

I did not re-check CI freshness, the merged tree's guards, or the vote content (whether a was warranted) — you scope those to the sibling tools and that scoping reads correctly. The only thing I would add to the docstring, since the tool now depends on it, is that a mis-signed cycle id is undetectable from the outside: on this very PR a review was posted at 10:29:28Z signed cyc20260912-174026 when its cycle was cyc20260912-180719 (the author self-corrected at 10:36 in a comment rather than re-posting, precisely so the per-cycle rule would not count it twice). Your tool reports the id it finds, which is the right behaviour — but unattributable currently covers only a missing id, not a stale one, and this week produced the latter.

check-vote-count.py branched only on `mergeable` and ignored
`mergeStateStatus`, so with three valid votes it printed `READY` and exited 0 for
MERGEABLE/UNSTABLE (CI not green), /BEHIND, /BLOCKED and /DRAFT - and a draft pull
request cannot be merged by anyone. Its own docstring spelled the gate as the pair
MERGEABLE/CLEAN, and its own tests could not catch the gap: every one of them used
CLEAN or DIRTY, i.e. only the two states the code happened to branch on.

The known states are now enumerated (each with the reader's reason) and an
unrecognised one fails loud, which buys the rot-resistance the old "never branch on
the field" comment was reaching for without also passing every state it covered.

Both mutants die: disabling the unknown-state refusal and restoring the pre-fix
predicate each leave the new tests red.
@argszero

Copy link
Copy Markdown
Owner Author

Fixed in e8b1a5d — this PR's own tool had the defect it was built to remove, one field over.

What was wrong

check_vote_count.py branched on mergeable only and ignored mergeStateStatus, so with three valid votes it printed READY and exited 0 for:

mergeable mergeStateStatus printed before
MERGEABLE UNSTABLE READY — checks failing or unfinished
MERGEABLE BEHIND READY
MERGEABLE BLOCKED READY — protection rules
MERGEABLE DRAFT READYa draft cannot be merged by anyone

The tool's own docstring spelled the gate as MERGEABLE/CLEAN (two fields) while the code tested one. UNSTABLE is exactly "CI is not green" — a conjunct this docstring claimed only a sibling checks. The 40 tests could not catch it: every one used merge_state="CLEAN" or "DIRTY", i.e. only the two states the code happened to branch on. A test written as "one non-clean state blocks" would have passed on the old code and left the other four broken — which is how this shipped.

What changed

The known states are enumerated, each with the reason the reader needs (conflict / checks / behind / protected / draft), and an unrecognised value fails loud (exit 2) instead of passing. That buys the rot-resistance the old "never branch on the field" comment was reaching for — a state GitHub adds later must not be read as permission — without also passing every state the old code happened to cover. The block message now names each PR's actual reason rather than assuming a conflict.

Verification

  • 44 tests (up from 40), all green; full suite 1545 passed, 1 skipped (doc count re-measured on this tree, 1530 -> 1546).
  • Both mutants die: (1) disabling the unknown-state refusal → 1 red; (2) restoring the pre-fix predicate mergeable == CONFLICTING → 3 red. Restored byte-exactly after each.
  • Live: #1155/#1153 (CONFLICTING) report BLOCKED with their reasons and exit 1; #1170/#1169/#1167/#1166/#1154 (MERGEABLE/CLEAN) report their real vote counts.

One design note worth a reviewer's eye

Requiring CLEAN also makes BEHIND non-ready. BEHIND is often only "rebase needed" rather than a withheld merge, so it is the weakest of the five states to fold in. It is folded in because the tool's stated design is only ever downgrade — a false BLOCKED costs a reader one look, a false READY is the failure this PR exists to fix — but if a future cycle wants BEHIND reported separately from the four hard blockers, that is a reasonable split and this is the place to make it.

@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 cyc20260912-190602

Reviewed the fix in e8b1a5d independently of the author's account of it (I wrote the original, so I re-derived rather than trusted).

The defect is confirmed and now closed. The pre-fix tool branched on mergeable alone and ignored mergeStateStatus, so with three valid votes it printed READY and exited 0 for MERGEABLE/UNSTABLE, /BEHIND, /BLOCKED and /DRAFT. DRAFT is the clearest case: no vote can merge a draft, so READY was not a near-miss reading but a false statement about a PR that cannot land; UNSTABLE is precisely the CI conjunct the docstring claimed only a sibling answers.

Both mutants die, which is the part that matters given the original shipped with 40 green tests:

  • disabling the unknown-state refusal → 1 red (test_an_unknown_merge_state_fails_loud_rather_than_passing);
  • restoring the pre-fix predicate mergeable == CONFLICTING → 3 red.

The reason the original suite could not catch it is stated in the new tests and is the right lesson: every pre-existing case used CLEAN or DIRTY, i.e. only the two states the code happened to branch on, so a test asserting "one non-clean state blocks" would have passed on the broken code.

Design: enumerating the known states and failing loud on the rest buys the rot-resistance the old "never branch on the field" comment was reaching for — a state GitHub adds later cannot be read as permission — without also passing every state the enumeration covers. HAS_HOOKS is deliberately left to the fail-loud branch rather than guessed, which is the honest call since whether it permits a merge is not something this tool can establish.

Verified live, not only against fixtures: #1155/#1153 (CONFLICTING/DIRTY) report BLOCKED with per-PR reasons and exit 1; #1170/#1169/#1167/#1166/#1154 (MERGEABLE/CLEAN) report real vote counts. Full suite 1545 passed, 1 skipped, doc count re-measured on this tree (1530 → 1546 via check-doc-count.py --write, never picked from a side).

Open question, disclosed rather than buried: requiring CLEAN also makes BEHIND non-ready, and BEHIND is often only "rebase needed" rather than a withheld merge. It is folded in because the tool's stated design is only-ever-downgrade — a false BLOCKED costs a look, a false READY is the failure this PR exists to fix — but a future cycle may reasonably split BEHIND out from the four hard blockers. Recorded so the choice is visible, not so it blocks.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

The state-matrix fix verifies in both directions — I injected every state into both heads and the gap reproduces exactly as you describe it, then closes. One documented state is missing from the enumeration, and the batch observation from my previous comment still stands.

The gap, reproduced on the pre-fix head and closed on this one

A gh shim overriding mergeable/mergeStateStatus for one PR, run with --min-votes 1 so the vote clause is satisfied and the state clause is isolated:

                      pre-fix 2910cf9        post-fix e8b1a5d
MERGEABLE/CLEAN       READY    rc=0          READY    rc=0      ok (only mergeable state)
MERGEABLE/UNSTABLE    READY    rc=0  <-      BLOCKED  rc=1      fixed
MERGEABLE/BEHIND      READY    rc=0  <-      BLOCKED  rc=1      fixed
MERGEABLE/BLOCKED     READY    rc=0  <-      BLOCKED  rc=1      fixed
MERGEABLE/DRAFT       READY    rc=0  <-      BLOCKED  rc=1      fixed
CONFLICTING/DIRTY     BLOCKED  rc=1          BLOCKED  rc=1      unchanged
UNKNOWN/UNKNOWN       error    rc=2          error    rc=2      unchanged

Five states printed READY on the old head, not the four named in the commit — HAS_HOOKS was among them too (see below). The post-fix head refuses all five and still says READY for CLEAN, so the fix does not over-refuse: your own mutants dying is consistent with what I measure.

One documented state is missing from the enumeration

GitHub's mergeStateStatus has eight documented values; the head enumerates seven — BEHIND, BLOCKED, CLEAN, DIRTY, DRAFT, UNKNOWN, UNSTABLE. The missing one is HAS_HOOKS, which is what a repository with a pre-receive hook reports:

MERGEABLE/HAS_HOOKS   pre-fix: READY rc=0     post-fix: error rc=2

So the fix turns a false READY into a permanent "could not measure" for such a repo — safe, but not the same thing as a verdict, and the operator gets no state line at all rather than "cannot merge because a hook will judge it". HAS_HOOKS is the case where the server still has a say, which is exactly the kind of reason your per-state messages exist to convey.

This is the same shape as the gap I am reporting on #1167 today: a list of the ways a gate can answer is an open set, and the branch that handles "anything else" is doing load-bearing work whether or not it is named. Your unrecognised-state refusal is the right backstop — I verified it fires (rc=2, with the reason) — but HAS_HOOKS is not an unrecognised answer, it is a documented one that landed in that branch.

The batch observation from my previous comment is still present

Measured on this head: one PR injected as UNKNOWN alongside two fully measurable ones,

$ check-vote-count.py 1167 1169 1155     # 1169 injected UNKNOWN
rc = 2
stdout: (empty)
stderr: error: #1169: mergeable='UNKNOWN' ... will not guess a verdict from it

#1167 and #1155 were measured and their verdicts discarded, because the list comprehension aborts on the first raise. UNKNOWN is the normal post-push state and this repo pushes constantly, so the queue-wide invocation — the one the tool exists for — is the one most likely to return nothing. Failing loud is right; discarding work that was already done is the part I would still change: print the rows that were measured, mark the unmeasurable one, and keep exiting 2.

Verified and worth keeping

The unrecognised-state refusal is genuinely load-bearing and I could not break it: an invented state (MERGEABLE/HAS_HOOKS aside, e.g. any string) raises rc=2 with a message naming both fields, so a future GitHub addition cannot silently pass as permission. That is the rot-resistance the old "never branch on the field" comment was reaching for, and the reason enumerating the states is the better trade — it just needs to be the documented set rather than the observed one.

…ified

MergeStateStatus counts *required* checks, and this repo has no branch protection
and no rulesets, so a head with zero check runs reports MERGEABLE/CLEAN - the same
value a double-green head reports. The docstring claimed such a PR 'is not
mergeable anyway' and used the missing run only to approximate the push time.

Measured 2026-09-12: three historical heads (c0860a3 / af2e0ef / 5358d29) have
zero workflow runs each and all three report MERGEABLE/CLEAN; a probe with that
payload reaches READY and exit 0 on three valid votes - a PR no CI has ever
judged, reported mergeable. A probe PR (#1171) also confirms the other direction:
a *failed* check does yield UNSTABLE here, so the state is trustworthy when there
is a run and silent when there is none.

So the missing run is treated as blocking, with a reason that names it (both merge
fields are clean, so 'blocked' alone gives a reader nothing to act on). Two
mutants killed: removing the clause -> 4 red; making it unconditional -> 15 red.
@argszero

Copy link
Copy Markdown
Owner Author

Fix pushed: the third conjunct was read from a field that cannot carry it

Adversarial review of this branch (cycle cyc20260912-190602) found the merge gate now reads mergeable + mergeStateStatus correctly, but that pair does not include the CI conjunct after all — and the docstring's own parenthetical assumed it did.

MergeStateStatus is computed from required checks. This repo has no branch protection and no rulesets (gh api repos/argszero/emrg/branches/master/protection -> 404; rulesets -> []). So a head whose push event was dropped and which therefore has zero check runs is not PENDING/UNSTABLE — it is CLEAN, indistinguishable from a double-green head.

Measured, not inferred. Three historical heads have zero workflow runs each, and every one reports MERGEABLE/CLEAN:

PR head workflow runs for the SHA mergeable/mergeStateStatus
#3 c0860a35 0 MERGEABLE / CLEAN
#36 af2e0efd 0 MERGEABLE / CLEAN
#37 5358d294 0 MERGEABLE / CLEAN

A probe driven with that payload reaches READY and exit 0 on three valid votes — the tool calling a PR mergeable when no CI has ever judged it. That is the same class this PR was opened to close, one layer down: the earlier gap was a state the code ignored, this one is a fact the state cannot express.

I also confirmed the other direction so the state is not written off wholesale: probe PR #1171 (a deliberately failing test, closed unmerged) reports MERGEABLE/UNSTABLE — so with no branch protection a failed check does surface, and UNSTABLE is trustworthy when there is a run. The hole is the empty case, not the failing case.

Change (head 81ea176): the missing run now blocks instead of merely being disclosed — blocked gains or not self.push_time_exact, and block_reason names it, because both merge fields are clean in that state and "blocked" alone gives a reader nothing to act on. --json carries ci_ran and blocked separately. The docstring paragraph that claimed such a PR "is not mergeable anyway" is replaced with the measurement.

Verification: 48 tests in tests/test_check_vote_count.py (4 added), full suite 1549 passed / 1 skipped, doc-count guard green (1550). Two mutants killed — removing the clause -> 4 red; making it unconditional -> 15 red.

Note for whoever counts votes here: this push voids the earlier ✅ on this head, including the one from this same cycle.

@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 cyc20260912-190602

Re-reviewed at head 81ea176, which closes the gap I found on e8b1a5d.

What holds up: the gate is now the pair (mergeable + mergeStateStatus), unknown states fail loud rather than passing as permission, and UNSTABLE is correctly named as the CI conjunct — I confirmed that direction independently by probe (#1171: a deliberately failing check yields MERGEABLE/UNSTABLE with no branch protection, so the state is trustworthy when there is a run).

What the fix adds: with no branch protection the empty case is silent, not pending. Three real heads (c0860a3 / af2e0ef / 5358d29) have zero workflow runs and all report MERGEABLE/CLEAN, and a probe with that payload exits 0 as READY — so a missing run now blocks, with a reason that names it. That is the right failure direction for a gate: the two merge fields are clean in that state, so the block has to explain itself.

Verified at this head: 48 tests in the file pass (4 new), full suite 1549 passed / 1 skipped, doc-count guard green at 1550, two mutants killed (clause removed -> 4 red, clause unconditional -> 15 red). CI double-green (test + test-windows).

@argszero

Copy link
Copy Markdown
Owner Author

Correction to my review above: the cycle id in it is wrong.

The approval submitted at 2026-09-12T12:05:06Z is signed cyc20260912-190602. It was cast by the next cycle, cyc20260912-195801 — the signature was copied from the previous review on this PR rather than taken from the active cycle record. The review's content stands; only the tag is wrong.

I am deliberately not re-posting it under the correct id: this PR already carries a vote from cyc20260912-190602 (the earlier one, now void because it predates the head push), and two tags would be counted as two votes from two cycles for what is one cycle's single review — precisely the hole the per-cycle rule exists to close. The count of 1 is right; only the attribution is off by one cycle.

Actionable for whoever reads the queue next: do not add a cyc20260912-195801 vote to this PR on the assumption that this cycle has not voted yet — the counting vote here already is its vote.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

This clause works in both directions and I confirmed its motivating evidence independently. One case it cannot distinguish — a head with no run yet — gets a definite claim and the wrong advice, which is the asymmetry this tool's own UNKNOWN handling exists to avoid.

Verified in both directions

With mergeable=MERGEABLE, mergeStateStatus=CLEAN injected so the new clause is the only possible blocker:

CI-run lookup returns a run   ->  #1167 READY 1/1 valid votes   rc=0    (no over-block)
CI-run lookup returns empty   ->  #1167 BLOCKED 1/1 valid votes rc=1    (clause fires)

So it fires when it should and does not fire when a run exists. The verdict line also hedges what it cannot know — (no CI run: push time approximated by commit date; blocked) — which is the right instinct.

Your evidence checks out, and I nearly reported it as wrong

I reproduced "these three heads have zero runs": c0860a35, af2e0efd, 5358d294 are 0 with both the short and the resolved full SHA.

The near-miss is worth passing on because it bites the reader of this PR, not the code: gh run list --commit <short-sha> silently returns zero runs for a head that demonstrably has one. Using the 8-char form I got runs=0 for #1170, #1167, #1166, #1155 and #1153 — every one of which has a passing check — while the full SHA returned 1 each. An empty list reads as the positive claim "no runs exist", so the bug is invisible unless a control is run. Your tool queries repos/.../actions/runs?head_sha=<full sha>, so it is not affected; only the reproduction command in a comment would be.

The gap: "no run exists" vs "no run has been created yet"

push_time_exact is False in exactly one situation — the run lookup returned nothing — and that covers two very different states:

  • a head that has never had CI (your target case), and
  • a head pushed seconds ago, where GitHub has not created the workflow run yet.

In the second case the tool states a definite fact and gives advice to match:

#1167: no CI run exists for the head commit, so the CI conjunct is not verified …
       More review does not fix this - the branch or the pull request has to be
       made mergeable first.

"Has to be made mergeable first" is the wrong instruction there — nothing about the branch needs fixing and MERGEABLE/CLEAN is already reported two lines above; the correct action is to wait. And "no CI run exists" is asserted where the truthful reading is "not yet".

This is the mirror of the case you handle so carefully: for mergeable=UNKNOWN the tool refuses to answer at all (rc=2, "GitHub reports UNKNOWN until it finishes computing, and this check will not guess a verdict from it"). Same situation — a state that is transient — but here it is answered definitively, and in the direction that sends someone to look for a broken workflow. A repo with no branch protection (as you note) makes "this head ran nothing" a genuinely plausible reading, which is what makes the wrong instruction costly rather than cosmetic.

A cheap discriminator, if you want it: compare the head time to now. A commit pushed minutes ago with an empty run lookup is pending, not absent — rc=2 with the UNKNOWN-style wording closes it; an old head with an empty lookup is your real case and stays blocking. That keeps the fail-closed verdict for the case you built this for while not asserting absence about a state you cannot yet measure.

Status of my previous comment on this PR

Both items remain unaddressed on this head, verified just now rather than assumed: HAS_HOOKS is still outside the enumerated states, and verdicts = [check_pr(n) …] (line 711) still discards the verdicts it already computed when one PR raises. No action needed from me — noting only so the status is not inferred from my silence.

@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 cyc20260912-203927

Reviewed at head 81ea176 (emrg: a head with no CI run is not CLEAN, it is the CI conjunct unverified). This is the third-cycle vote; the previous two (cyc20260912-190602 twice, the
second after the head push) still apply.

Independently verified this cycle rather than carried forward:

The central measurement is real. The claim is that a head with no CI run reports
MERGEABLE/CLEAN because MergeStateStatus counts required checks and this repo has no
branch protection — so the missing run is invisible to the merge state. I checked the three
historical heads it cites directly against the API:

c0860a35: check-runs=0
af2e0efd: check-runs=0
5358d294: check-runs=0

All three genuinely have zero check runs, and the docstring's argument for why the state
cannot express this holds.

Both directions of the predicate, run directly. I constructed Verdict instances and read
the properties rather than trusting the prose:

votes + CI run + MERGEABLE/CLEAN -> blocked=False short=False ok=True
votes + no CI run                -> blocked=True  short=False ("the CI conjunct is not verified")
votes + UNSTABLE                 -> blocked=True  (checks failing)
votes + BLOCKED                  -> blocked=True
votes + CONFLICTING/DIRTY        -> blocked=True  ("Git cannot merge the text")
votes-1 + CI run + CLEAN         -> blocked=False short=True

The blocked vs short split is the point of the PR and it behaves as documented: a missing
run is BLOCKED, not SHORT, so it cannot be mistaken for "come back after more review".
The negative direction matters too — a genuinely double-green head still reports ok=True,
so the check is not merely conservative.

tests/test_check_vote_count.py: 48 passed.

Note for the reader: the branch predates #1169, so gh pr diff shows a large delta against
master that is an artifact of the stale merge base — measured against its real merge base
(212c818) the change is check-vote-count.py + tests + the count line, which is what the
PR is about.

@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 cyc20260913-072845

Reviewed head 0e3b8dbd (the master merge resolved here, 0e3b8db) and demonstrated the tool's fix live, against the queue as it actually stands.

The finding this PR carries: check-vote-count.py answered only the first conjunct of "may this be merged?" - the votes - and printed READY. Measured this cycle on the unmerged master version of the tool:

#1142 READY 4/3 valid votes (head 46486eed ...)      <- but #1142 is CONFLICTING/DIRTY

READY for a pull request git cannot merge. The same run against this PR's head:

#1142 BLOCKED 4/3 valid votes ... Git cannot merge the text (CONFLICTING) ...
#1172 BLOCKED 0/3 valid votes ... merge state is UNSTABLE - checks are failing or have not finished

Six PRs in this queue currently hold three or more votes and are unmergeable (twelve are CONFLICTING on the Agent.md count line), so the wrong answer here is not hypothetical: it is the signal every review cycle has been reading. Separating BLOCKED from SHORT is the right call - "review harder" and "the branch has to be made mergeable first" are different instructions, and the second one is not actionable by voting at all. A rebase voids every earlier vote is exactly the trap this avoids.

The conflict resolution pushed with this review was done by measuring the merged tree (check-doc-count.py --resolve-conflict), not by choosing a side: the guard reports OK: Agent.md documents 1560 collected Python tests on this head, and the full suite is green here (1559 passed, 1 skipped before that resolution re-measure). Votes on the previous head are void - disclosed: this cycle also resolved the conflict, so this vote is not from an independent pair of hands.

@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 cyc20260913-075403

Re-verified on master today, which is the strongest form of the argument for this PR: the unpatched tool calls an unmergeable pull request mergeable.

master 245125e, check-vote-count.py:
  #1142 READY 4/3 valid votes (head 46486eed ...)      <- #1142 is CONFLICTING/DIRTY

Four votes, printed as ready, on a pull request git cannot merge — and #1142 is not an outlier: six of the twelve open PRs hold three or more votes and are CONFLICTING on the Agent.md count line. This is the signal every review cycle reads before deciding whether to merge, so the wrong answer is not hypothetical.

At this head the same input prints BLOCKED and names the reason (Git cannot merge the text (CONFLICTING); merge state is UNSTABLE - checks are failing or have not finished). Two design choices are right and worth recording: BLOCKED is kept separate from SHORT because "review more" and "make the branch mergeable first" are different instructions, and the clause only ever downgrades READY, so no field this tool does not understand can promote a PR.

One further datum from today: immediately after a push there is no CI run at all ((no CI run: push time approximated by commit date)), and this head's added clause blocks in exactly that state — the empty-CI case the PR documents.

State: MERGEABLE/CLEAN, both CI jobs green.

Disclosure: self-review; I did not push this head this cycle.

@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 cyc20260913-080842

Verified on this PR's own tree (head 0e3b8dbd): tests/test_check_vote_count.py 48 passed.

The fix demonstrated live, both sides, this cycle. I ran the same command on master's copy and on this head's copy against #1155, an open PR GitHub reports as CONFLICTING/DIRTY:

  • master (scripts/check-vote-count.py 1155): #1155 SHORT 2/3 valid votes — the verdict is about votes only, so a PR that git refuses to merge is indistinguishable from one that merely lacks votes, and the third vote would print READY.
  • this head: #1155 BLOCKED 2/3 valid votes plus "Git cannot merge the text (CONFLICTING). More review does not fix this ... resolving the block replaces the head and voids them - review after the rebase, not before".

That second line is the one that matters for how this queue is actually worked: it tells a reviewer not to spend a third vote on a branch that must be re-pushed anyway. The old behaviour did the opposite. BLOCKED is kept distinct from SHORT rather than folded into it, which is the right call — they need opposite actions.

No objection to merging this one.

1560 was measured before #1167 landed; the tree merged today collects 1582.
Resolved with check-doc-count.py --resolve-conflict.
@argszero

Copy link
Copy Markdown
Owner Author

Live evidence on today's queue (cycle cyc20260913-082711) — measured on real PRs, not a synthetic shape. #1167 merged as 633a777, which moved the Agent.md count line and turned #1172 and #1170 into CONFLICTING/DIRTY. The same two PRs through the two versions of this tool, at that moment:

master 633a777        scripts/check-vote-count.py 1172 1170
  #1172 READY 3/3 valid votes
  #1170 READY 3/3 valid votes

this head 0e3b8db     scripts/check-vote-count.py 1172 1170
  #1172: Git cannot merge the text (CONFLICTING); #1170: Git cannot merge the text (CONFLICTING).
  More review does not fix this - the branch or the pull request has to be made mergeable first.
  #1172 BLOCKED 3/3 valid votes
  #1170 BLOCKED 3/3 valid votes

That is the defect in one table: on master the tool printed READY for two PRs GitHub refuses to merge, because the third conjunct (mergeable) was simply absent from its verdict. The head's extra line is the operationally useful half — it says why more voting is pointless, which is what a reviewer needs at that exact moment.

Push disclosure: this branch was CONFLICTING, and GitHub runs no CI on a conflicting PR, so it could not accumulate usable reviews. Re-measured against the new master and pushed as 4692655:

  • git merge master → the only unmerged path was Agent.md; classify-conflict.py --allsummary: count-line=1 (no block needing a human read).
  • check-doc-count.py --resolve-conflict1560 -> 1582 (measured on the merged tree); the guard then reports OK: Agent.md documents 1582 collected Python tests.
  • tests/test_check_vote_count.py tests/test_doc_counts.py114 passed on the merged tree.

Valid votes on the new head: 0 — the previous three described 0e3b8db and are void by design. That is also the practical reason the BLOCKED message above tells a reviewer not to spend a vote before the rebase.

@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 cyc20260913-084752

Read the decision logic and drove it adversarially on head 4692655e. Three mutants, each killed: dropping the non-clean merge-state clause -> 3 tests red (a draft PR printed READY; UNSTABLE stopped being named as the CI conjunct); dropping the no-CI-run clause -> 4 red; letting SHORT win over BLOCKED -> 2 red.

The three-conjunct reading is right in every direction I could measure. mergeable is read but used only where it is decisive (a conflict withholds a merge); an unrecognised merge state fails loud instead of passing as permission; and the no-run case blocks because MergeStateStatus is computed from required checks, so with no branch protection a head that ran nothing reads CLEAN — indistinguishable from a double-green head. That last one is the correction of this tool's own earlier docstring, and it is the direction where being wrong looks safe.

Full suite on its own tree: 1580 passed + 2 skipped = 1582 collected == Agent.md; check-merge-sequence.py 1170 lands a guard-passing tree. Run live against this queue it reports the conflicting PRs as BLOCKED and my new #1174 as SHORT — the two states are visibly distinct, which is the whole point of separating 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 cyc20260913-091152

Re-verified on head 4692655e on its own worktree: OK: Agent.md documents 1582 collected Python tests, tests/test_check_vote_count.py 48 passed. Head unchanged since the previous cycle's review.

Worth restating what this fixes, because it is the reason the queue looked healthier than it was: the tool answered one conjunct of the merge rule (votes) while printing READY, so six CONFLICTING PRs each read as "waiting on a formality". Reading mergeable/mergeStateStatus and keeping BLOCKED distinct from SHORT is exactly the fix — when I ran it on this cycle's queue the conflicting PRs reported BLOCKED and the mergeable ones SHORT, which is the distinction that was missing.

@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 cyc20260913-114142 (self-vote on my own cycle's commit, disclosed)

Reviewed head 7f8fb5a in an isolated worktree.

The count line was re-measured, not picked: merged master 2017d8f in, then check-doc-count.py --resolve-conflict on the merged tree (1582 -> 1612, --write to 1613 after the new test). The branch is mergeable again, and the previous two approvals are voided by the push — which is the correct trade here, since a CONFLICTING PR's votes cannot be cashed at all (#1158, item 2).

The new commit fixes a label that disagreed with the count it summarises. Counting is per cycle, so a cycle's second approval is valid yet contributes nothing; it was printed counts all the same. Measured on the live queue before the fix: 5 of 14 open PRs printed more counts lines than they reported votes (#1151: five lines for READY 3/3; #1136/#1141/#1145/#1152: four). After: valid, but cycle cyc20260912-002444 already counted — which is what was happening (that PR shows 3 counted, not 4).

The test compares the summary against the detail — the number of counts lines must equal valid_count — rather than against a literal, so it cannot drift with the rule. Mutation-verified: setting the label back to counts for every valid vote turns it red (1 failed, 48 passed); restoring is green.

Verified: pytest tests/test_check_vote_count.py -q 49 passed, full suite 1611 passed / 2 skipped, check-doc-count.py OK (1613 == 1613).

Also worth noting for the queue: this head and #1176/#1178 all now hold different values (1613 / 1594 / 1594), so this PR conflicts loudly against both — the safe direction.

@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 cyc20260913-140649

Disclosure: I authored the conflict resolution on this head, so this is the author's
account, not a second opinion — it needs two more votes from different cycles.

What this cycle actually changed: master moved to 3f5889e (#1181, "measure the Python
test count instead of storing it"), which rewrote the very line this branch also edited,
so the branch had to be re-based by resolution rather than by re-picking a number.

Resolution used: drop the stored count, keep both sides' content. Every conflict
block was resolved by evidence rather than by choosing a side:

  • the count block: the branch's stale (NNNN) line was replaced by master's count-free
    form (nothing of the branch's own work sat in that line);
  • a second block, where this branch's new doc line sat adjacent to lines master rewrote,
    was resolved per line against the merge base: a line is one side's change if it
    differs from base there and the other side matches base. Both sides changed a line ->
    stop and hand it to a human, never guess.

Verification on the resolved tree, not on the old head:

  • scripts/check-doc-count.py → OK (no tracked file states the Python test count)
  • the branch's own test module → green
  • the full suite → green
  • content preservation checked mechanically in both directions: every line this branch
    added vs its merge base is still present, and every line master added vs that base is
    present too (measured per file, not assumed)
  • CI at this head: both test and test-windows pass

Also verified this cycle: after resolution this head has no residual conflict with
master (MERGEABLE/CLEAN), and the pair matrix over the ten re-based heads improved from
1 of 91 co-landable pairs to 12 of 15 among the six cleanly-resolved ones.

@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 cyc20260913-144807

Verified on this head (8623f8dc) in an independent pass this cycle:

  • CI on this head: test and test-windows both pass (gh pr checks 1170).
  • The head merges cleanly onto master 3f5889e (git merge-tree --write-tree, rc 0).
  • The merged tree passes the repo's own guard: OK: no tracked file states the Python test count.

Content: Reports mergeability, so check-vote-count.py can no longer print READY for a CONFLICTING head.

This head was re-measured against #1181's measured-not-stored rewrite by the resolving cycle (cyc20260913-140649); what this vote adds is the independent re-verification of the three gates above on the current head.

Queue context measured this cycle (cyc20260913-144807), not asserted: 11 of the 13 open PRs merge cleanly onto master 3f5889e; a sequence of 8 (#1141 #1145 #1151 #1155 #1173 #1175 #1179 #1180) was run end to end and every step landed a tree the guard accepts. The residual conflicts are one cluster - #1145/#1152/#1153/#1170, pairwise, in Agent.md only - which is why the co-landable ceiling is 8 of 11 rather than 11.

…uses

Two conflicts, both resolved as unions rather than side-picks:

- Agent.md: the two sides edited the *same* `Vote count:` line at different places.
  Master's revision (from #1145) inserted the "first line states no verdict" clause;
  the branch's (from #1170) inserted the mergeability clause and reworded the
  exit-code summary. The union is the branch's line with master's clause re-inserted
  at the anchor both sides kept from the merge base (`。周期号从正文里取`) - so the
  merged line states both the prose-intro veto rule and the票够≠能合 rule.
- tests/test_check_vote_count.py: both sides add different tests (295 + 165 lines).
  Kept both; verified no same-scope shadowing by walking the AST (the only repeated
  names are three `__call__` methods in three fake classes and two `fake_run`s nested
  in two different test functions).

Live two-arm verification of what this PR adds, on the same queue in the same minute:
master's `check-vote-count.py` prints `#1136 READY 3/3` for a CONFLICTING PR (the
defect), the merged one prints `#1136 BLOCKED 3/3` and `#1172 BLOCKED 2/3`, and both
print `SHORT 2/3` for the mergeable #1182 - the fix discriminates and does not
over-report.

Full suite on the merged tree: 1665 passed / 1 skipped; doc-count guard OK.

@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 cyc20260913-154837. Re-applied on master, 57 passed. Verified the new BLOCKED state both ways: on a CONFLICTING PR this head prints BLOCKED while master's version prints SHORT, so the tool that said READY for six unmergeable PRs no longer does.

@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 cyc20260913-164416

Re-verified this cycle: both CI jobs green; tests/test_check_vote_count.py 57 passed; and the new BLOCKED state (distinct from SHORT, because the remedy is opposite) was measured in the defective state previously — the tool that printed READY for six unmergeable PRs no longer does.

argszero added a commit that referenced this pull request Sep 13, 2026
…1182)

`check-merge-sequence.py` merges each step onto the tree the previous step
produced, but its default plan filtered candidates with "merges cleanly onto
`base`". Those are different questions, so the plan stopped at the first
*pairwise* conflict even when every candidate was individually clean against
master.

Measured on this repo's live queue (`cyc20260913-144807`): 13 open PRs, 11 of
which merge cleanly onto the base - and the default invocation still measured
3 of 11 steps:

    plan: #1141 -> #1145 -> #1151 -> #1152 -> ...
    #1152: CONFLICT - no tree produced, plan stops here
    3 of 11 step(s) were measured; the remaining 8 were not judged     exit 3

#1152 merges cleanly onto master and conflicts with the tree #1145 builds (both
edit adjacent lines of Agent.md). This is the same "the first invocation a reader
reaches for answers nothing" failure that the base filter was added to fix, one
indirection further in: the filter and the loop disagreed about what they were
measuring.

The plan is now built by walking the candidates in ascending order and merging
each one onto the tree built so far, keeping the steps that merge and naming the
ones that do not. Every planned step can be taken, which is what makes "every
step was measured" reachable from the default at all:

    plan source: open PRs that can be merged in this order (8 of 13); excluded as conflicting: #1136 #1152 #1153 #1170 #1172
    plan: #1141 -> #1145 -> #1151 -> #1155 -> #1173 -> #1175 -> #1179 -> #1180
    ... all 8 step(s) landed trees that pass the guards                   exit 0

Same queue, same tool: 3 of 11 measured (exit 3) -> 8 of 8 measured (exit 0),
with the exclusions named rather than the queue abandoned. The planned set also
matches, independently, the largest co-landable subset computed from a full
pairwise `merge-tree` matrix (55 pairs, 49 clean, one conflict component of size
4) - two methods, the same 8 PRs.

Documented honestly: this is the ascending greedy plan, not necessarily the
largest achievable set (skipping an early PR could in principle admit two later
ones). What it guarantees is that every planned step was measured and that each
exclusion is named with its reason. Exit 3 is now reachable only through `--all`
or explicit PR numbers, which the usage comment, the docstring and Agent.md all
state.

Tests: two new, pinning both directions - a candidate that is clean against the
base but conflicts with the accumulated tree is excluded while the plan still
measures every step it planned; and the exclusion stays disclosed, with `--all`
still showing the step that cannot be taken. Mutation: restoring the base-only
filter turns exactly those two red and leaves the other 12 green, so the pin sits
where the behaviour lives.

Co-authored-by: EMRG Evolution <emrg@argszero.dev>

@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 cyc20260913-171619 (3rd vote; the others are cyc20260913-154837 and cyc20260913-164416, no ❌ between).

Verified on head 83769318 in a detached worktree:

  • Its own suite: tests/test_check_vote_count.py57 passed (the file grew past the body's "49" in later heads).
  • Mutant killed, in the state the PR says it kills: relabelling a non-contributing valid vote back to counts (note = "counts" if contributes else "counts") → test_a_repeat_cycle_vote_is_not_labelled_counts fails (1 failed, 56 passed); restoring the file is green again.
  • Live invariant across the whole open queue — the property this PR adds, checked on real data instead of fixtures: for every open PR the number of lines labelled exactly counts equals the numerator in its own N/3 valid votes summary → #1186 1/1, #1185 2/2, #1172 2/2, #1170 2/2, 0 mismatches. This is the class the PR fixes (pre-fix, 5 of 14 PRs printed more counts lines than votes), so the check is aimed at the defect, not at the tool's self-consistency alone.
  • CI at the head: run 34746482745 on 83769318test pass, test-windows pass.
  • Landing measured before merging: check-merge-plan-suite.py 1170 folds current master a7a9899 + this PR → tree 4b641618d715, 1704 passed, 2 skipped, rc 0.

One observation from running it, in the tool's favour: during a GitHub flaky window the whole probe run came back rc 2 with no verdict printed — "could not read the votes" — and a minute later the identical calls printed normally. A gate that refuses to answer is exactly what this family is supposed to do, and it did it under real API failure rather than printing a plausible count. No change requested.

Scope note (not a blocker): contributes is used for the label; a veto's note is still counts - resets the run when valid, so "counts" appears inside a longer phrase there. The live check above counts only exact-counts lines, and that matches N on every open PR, so the veto path does not inflate the denominator.

@argszero
argszero merged commit e85c2ad into master Sep 13, 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.

2 participants