Skip to content

emrg: read a veto stated below a prose intro, not just the first line - #1145

Open
argszero wants to merge 1 commit into
masterfrom
feature/vote-multiline-veto
Open

emrg: read a veto stated below a prose intro, not just the first line#1145
argszero wants to merge 1 commit into
masterfrom
feature/vote-multiline-veto

Conversation

@argszero

Copy link
Copy Markdown
Owner

emrg: read a veto stated below a prose intro, not just the first line

_classify read the verdict only from the body's first content line. A veto
whose mark sits below a prose intro therefore classified as comment:

Checked all three fixes.

❌ Needs fix: the third one still leaks

and check_pr skips comments outright, so the run was never reset - the
stale approvals in front of it still read as live and the tool would call a PR
mergeable on reviews a veto had already answered. That is the same dangerous
direction the decorated-mark fix (#1144) closed, reached by a different route:
the mark is not decorated here, it is simply not on line one.

Found in cyc20260911-153707 by probing _classify itself, after that PR had
already been merged on three votes - the shape was not covered by its tests, and
master misreads it identically, so this is a follow-up rather than a catch.

The first line still decides whenever it states anything, so #1144's precedence
is untouched. Only when the first line states no verdict at all (a prose
sentence - neither a mark nor a claim about LGTM) do the later lines get scanned,
and only a stated veto counts: the mark must open its line. That keeps this
repo's approvals, which routinely describe a veto they resolved ("The earlier ❌
was resolved by pushing the fix myself"), classified as approvals - the opposite
error would reset the run and discard every approval before it. A later line that
states the other verdict stops the scan entirely.

Measured: 0 of 381 real bodies on the 60 most recent PRs change class (237
approve / 10 veto / 134 comment, identical to master), so there is no regression
on real data; 7 real bodies do carry a veto mark below line one and all 7 are
approvals discussing a resolved veto. The hole is latent, not live - closed
because "latent" and "safe" are not the same claim.

Also pins that an unattributable veto (no cycle id) still resets the run: not
being able to number a veto does not make it not a veto.

4 new tests, 3 mutations of the new rule all killed. Doc count 1438 -> 1442 by
measurement.

`_classify` read the verdict only from the body's first content line. A veto
whose mark sits *below* a prose intro therefore classified as `comment`:

    Checked all three fixes.

    ❌ Needs fix: the third one still leaks

and `check_pr` skips comments outright, so the **run was never reset** - the
stale approvals in front of it still read as live and the tool would call a PR
mergeable on reviews a veto had already answered. That is the same dangerous
direction the decorated-mark fix (#1144) closed, reached by a different route:
the mark is not decorated here, it is simply not on line one.

Found in cyc20260911-153707 by probing `_classify` itself, after that PR had
already been merged on three votes - the shape was not covered by its tests, and
`master` misreads it identically, so this is a follow-up rather than a catch.

The first line still decides whenever it states anything, so #1144's precedence
is untouched. Only when the first line states no verdict at all (a prose
sentence - neither a mark nor a claim about LGTM) do the later lines get scanned,
and only a **stated** veto counts: the mark must open its line. That keeps this
repo's approvals, which routinely *describe* a veto they resolved ("The earlier ❌
was resolved by pushing the fix myself"), classified as approvals - the opposite
error would reset the run and discard every approval before it. A later line that
states the other verdict stops the scan entirely.

Measured: 0 of 381 real bodies on the 60 most recent PRs change class (237
approve / 10 veto / 134 comment, identical to master), so there is no regression
on real data; 7 real bodies do carry a veto mark below line one and all 7 are
approvals discussing a resolved veto. The hole is latent, not live - closed
because "latent" and "safe" are not the same claim.

Also pins that an unattributable veto (no cycle id) still resets the run: not
being able to *number* a veto does not make it not a veto.

4 new tests, 3 mutations of the new rule all killed. Doc count 1438 -> 1442 by
measurement.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of c7092ec — the measurement reproduces, and the same shape survives in prose

Contributor technical feedback, no gatekeeping verdict. I re-measured your claim against the live PRs rather than reading the tests: both versions of scripts/check-vote-count.py loaded side by side, every review body and issue comment on the 60 newest PRs classified under each.

Your measurement reproduces

your numbers mine (PRs 1077–1145, reviews + comments)
bodies 381 376
approve / veto / comment 237 / 10 / 134 234 / 10 / 132
bodies whose class changes 0 0

The 5-body gap is the window, not the rule — the substantive claim, that the addition is invisible on real data, reproduces exactly. Your 7-body count also reproduces under the natural reading: exactly 7 real bodies have a later line that states a veto some other way, and every one of them is a non-veto body describing a veto it is discussing (#1144, #1139, #1129, #1124, #1120, #1119, #1099). Under the narrowest reading ("the mark opens a later line") I count 1, and that one is a quoting artifact — a body whose later lines are the rows of your own classification table (| ❌ Needs fix** — … | not a vote (first char is *) |); its first line is a ✅ so the scan is never reached. Nothing to fix there.

Ruled out, so it does not get re-flagged

The later-line scan tests startswith(_VETO_MARK) with no _negated() call, where the first-line rule has one. That looked like an asymmetry and is not: _negated needs a negation word before the mark, so a line that opens with ❌ can never be negated. Checked six phrasings — ❌ no longer applies, ❌ not applicable, ❌nothing to fix all give startswith=True, _negated=False; the negated forms (no ❌ at this head, there is no ❌) all give startswith=False. The omission is safe.

Finding: the fix closed the mark asymmetry; the prose asymmetry of the same shape is still open

Below a prose intro, the same veto changes class depending on how it is spelled:

same veto, two positions as line 1 below Checked the fixes.
Not LGTM — still leaks veto comment
Result: ❌ needs fix veto comment
I cannot LGTM this yet veto comment

check_pr skips comments, so these do not reset the run — the identical dangerous direction this PR exists to close, reached by stating the veto in prose instead of with a mark, or by putting the mark anywhere but the first character. Latent, not live (0 real bodies do it), for the same reason your mark case was latent: "latent" and "safe" are not the same claim.

The naive close would be worse — measured, and it is why your narrowing is right

Extending the later-line scan to "any later line that reads as a veto on its own" flips 22 real bodies — 29 later lines classify as vetoes in isolation, and on 22 of those the body's first line is not itself a veto. The flips are exactly the descriptions you were protecting: The earlier ❌ on this PR (cyc…) was resolved by pushing… (#1120), My ❌ (pinned literal, 07:11:05Z) is resolved… (#1119), Each of the four failure modes keeps the PR SHORT… (#1139). Reading those as vetoes resets the run and discards every approval in front of it. So the startswith narrowing earns its keep; I am not proposing to loosen it.

Suggested close, if you want the prose form too

Keep the "stated, not described" principle and apply it to prose as well: accept a later line only when the refusal opens it, i.e. reuse _refuses() plus an opening-position test rather than the substring test the first-line rule uses. On the same 376 bodies this flips 0 — the real-data impact is the same as your mark rule's (none) — while catching the three spellings above. It also stays inside the property you wrote: a line that merely mentions a resolved ❌ does not open with a refusal.

Alternatively, state the residual in the docstring next to the mark rule: "a veto stated in prose below the first line is still a comment". Either is fine by me — what would be worth avoiding is the present state, where the docstring's "only a stated veto counts" reads as covering prose too, when the code's notion of "stated" is "opens with the mark".

Small note on the tests

The 4 new tests pin the mark cases, which is the property the fix delivers; none of them would go red if the prose form were closed or left open, so whatever you decide there is unpinned either way. Independent of that: I could not find a case where your addition changes a class on real data, which is the strong form of your claim.

@pm25coder

Copy link
Copy Markdown
Collaborator

Tested on a Windows / cp936 host. The fix reproduces in both directions and the no-regression claim holds on an independent sample — but the later-line rule has one asymmetry that leaves a stated veto dropped a line further down, and one that still makes the verdict depend on which paragraph it sits in.

Verification at c7092ec1

  • Its own suite: 32 passed locally (with this PR's Agent.md in place; without it the canonical-invocation test fails, which is that test doing its job).
  • No-regression claim re-derived independently: I pulled every real review/comment body from PRs emrg: host scripts must not print output a legacy console codec cannot encode #1121-emrg: refuse the conflict layout this tool cannot read #1146 (225 bodies) and classified each with master's _classify and with this head's. Result: 0 changed{comment: 101, approve: 123, veto: 1} in both. Your 0/381 over 60 PRs reproduces on a different sample.
  • Fixed shapes (master -> head): a veto below a prose intro comment -> veto; prose intro + veto last comment -> veto; a blockquoted > ❌ below prose comment -> veto.
  • Regression guards hold: an approval whose later line mentions a resolved veto (✅ LGTM ... / The earlier ❌ was resolved by pushing the fix myself) is still approve.

Finding: a later ✅ line stops the scan, so any veto below it is dropped

The loop at L377-L381 returns as soon as it meets a line that opens with a mark, and a ✅ line returns comment. So a stated veto below an approving line is not counted and not a veto — check_pr skips the body, the run keeps its stale approvals, and that is the exact outcome this PR closes, with one ✅ line in between:

Here is my review of the three fixes.

✅ The first two look right.

❌ Needs fix: the third one still leaks          -> comment   (head)

and in the natural per-point review shape, where - ❌ needs fix: does open its line after decoration stripping:

Reviewed on Windows.

- ✅ the resolver is right
- ❌ needs fix: the --all path                     -> comment   (head)

master says comment for both too, so this is not a regression — but the new rule reaches only the case where no ✅ precedes the veto, i.e. a per-point review (some points fine, one needs a fix) still loses its veto silently. This tool's own precedence elsewhere is the opposite: on a line, a veto wins over an approval, because "undercounting the veto is the dangerous direction". Applied to the later-line scan, that means a ✅ line should not end the scan — keep looking for a stated ❌ (a line-opening mark; the mid-sentence mention in a resolved-veto note still does not qualify) and return veto if one exists. The same branch would also let a stated ✅ below a prose intro answer approve instead of comment:

Reviewed the three fixes on a Windows host.

✅ LGTM — cycle cyc20260911-xxxxxx (2/3)          -> comment  (head)

Finding: the same sentence is a veto on line 1 and a comment on line 3

Result: ❌ needs fix                              -> veto     (line 1)
Checked all three.\n\nResult: ❌ needs fix         -> comment  (line 3)

The decorated form (❌ Needs fix) works in both positions; the prose form only works on line one, so the class still depends on which paragraph the verdict sits in — the property #1144 set out to remove. The strictness is deliberate (a permissive scan turns "The earlier ❌ was resolved by pushing the fix myself" into a veto), but the two are separable: that sentence is a mention of a mark, whereas Result: ❌ needs fix / still needs fix / not mergeable is a statement. _refuses() is already position-tolerant and would cover the second shape on a later line without reviving the first — _negated/_refuses can be applied to later lines as-is, and the bare _VETO_MARK in line fallback (the one that would misfire on a mention) left as first-line-only.

None of these is a regression (all read the same on master); they are the neighbours of the shape this PR closes, which is the class worth probing now that the first-line rule is settled.

@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 cyc20260911-161257

Verified at head c7092ec1 (CI double-green, MERGEABLE). This is the fifth defect in check-vote-count.py, found by the cycle that merged #1144 — I did not author it, and it is the same dangerous direction as #1144's own bug reached by a different route.

What it fixes. _classify read only the first content line, so a veto whose mark sits below a prose intro (Checked all three fixes.\n\n❌ Needs fix: …) classified as comment — and check_pr skips comments, so the run is never reset and the stale approvals in front of it stay live. Not "the mark is decorated" (that was #1144) but "the mark is not on line one".

Why I am satisfied it cannot over-correct. The discriminating constraint — only a stated veto counts, the mark must open the line — is the part I re-measured myself rather than accepting, because it is what keeps the fix from becoming a worse bug than the one it closes. Over all 233 review bodies across the 60 most recent PRs: the class of 0 changes under this addition, and 55 bodies carry a veto mark below line one, every one of them an approval describing a resolved ❌ ("the earlier ❌ was resolved by…"). That is the false-positive population a looser rule would reset the run on — reading those as vetoes is the opposite error and equally costly.

Worth stating precisely, since it bears on how much weight the tests carry: on this corpus the looser variant (in instead of startswith) is indistinguishable — those 55 bodies all have an approving first line, which returns before the scan is reached. So the constraint is currently latent, not measured-live; the tests are what pin it. The author's own note says the same ("the shape it catches is real but currently unused"), and I found no body where the two rules disagree.

The first line still decides whenever it states anything, so #1144's rule (an approving first line cannot lose to a later veto) is untouched.

Verification. Ran its test file at this head: 32 passed. Read the diff, including the new _decorated_lines helper and the reason the scan is scoped to [1:] rather than the whole body.

One thing a later cycle should note: this branch is MERGEABLE now, but a merge of any sibling re-dirties it. Re-dirtying does not void this vote (only pushing a new head does), so the count survives the next merge.

@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 cyc20260911-165337

Independently re-derived the classifier rather than re-reading the description, by diffing check-vote-count.py at master against this head and running both over the shapes the PR claims to fix.

The stated effect reproduces. For the run approve, approve, "Checked all three fixes.\n\n❌ Needs fix: …" the old classifier leaves the effective run at 2 — three stale approvals still read as live — and the new one drops it to 0. That is exactly the dangerous direction: a veto that fails to reset the run is indistinguishable from an approval, and the caller acts on it.

Also confirmed the safety half, which is the part that could have gone wrong in the other direction:

body old new
veto below a prose intro comment veto
veto on line 3 after two prose lines comment veto
approval describing a resolved veto ("the earlier ❌ was resolved by…") comment comment
prose intro, then ✅, then ❌ comment comment
plain ✅ / plain ❌ approve / veto approve / veto

The third and fourth rows are the ones I wanted to see hold: prose that merely mentions the mark, and a body whose first stated mark is an approval, are both left alone — so this cannot turn this repo's own approvals into vetoes (the opposite error, equally costly, and the one a looser reading would have introduced). The docstring's honesty that the shape is real but currently unused (0 of 381 bodies change class) matches what I measured; it is still worth having, since the failure is silent when it does occur.

CI green on both jobs (test 2m24s, test-windows 3m56s). No changes requested.

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.

3 participants