fix(prep): only a submitted verdict clears a live REQUEST_CHANGES #1249

Merged

bosun merged 2 commits from i/1246-verdict-allowlist into main 2026-09-06 10:25:21 +02:00

Conversation 1 Commits 2 Files changed 4 +197 -3

shipwright commented 2026-09-06 09:51:50 +02:00

Owner

Copy link

A comment, a re-request, or an unrecognised review state each cleared a live REQUEST_CHANGES, letting the bot force-push the rolling PR and dismiss its approvals.

Intended-targets: #1246

Why this is an allowlist and not a longer denylist

#1237 fixed this predicate by excluding PENDING. A denylist admits the next row type, and two more walked through it:

PENDING          an open compose box, never submitted        #1235
COMMENT          submitted, but NOT a verdict                #1246
REQUEST_REVIEW   a re-request row, not an opinion at all     #1246
<anything new>   whatever Forgejo adds next

⚠️ Each fix admitted the next one on a different axis. "Skip PENDING" let COMMENT through precisely because COMMENT is submitted — so the property the first fix keyed on was not the property that mattered. REQUEST_REVIEW is neither an unsubmitted draft nor a verdict.

All four fail toward merge. This predicate has never once failed closed.

The gate's question is "what is this user's current VERDICT?", and exactly two row states answer it. Everything else is ignored by construction, so an unrecognised state is a non-event rather than the next incident.

The mutations are the argument for the shape

M0 baseline                        rc=0 ran=134 red=0
M1 back to the #1235 denylist      rc=1 ran=134 red=4   COMMENT, REQUEST_REVIEW, unknown
M2 denylist extended to COMMENT    rc=1 ran=134 red=3   REQUEST_REVIEW, unknown — STILL LEAKS
M3 allowlist opened to everything  rc=1 ran=134 red=6
M4 allowlist closed to nothing     rc=1 ran=134 red=9

🔑 M1 and M2 together are the point: extending the denylist by one term reduces the leak by one term and closes nothing. M3 and M4 pin both directions, so "ignore everything" and "block on everything" are both red.

Measured before the change

6459 sentry REQUEST_CHANGES 00:18:12
6480 sentry REQUEST_REVIEW  00:50:00
  -> regeneration PROCEEDS (UpdatePR called once)

The arm that matters

SOME_FUTURE_STATE — a state string this build has never heard of, asserted to be ignored rather than treated as a verdict. That is the arm that makes the next Forgejo review type a non-event instead of a fourth incident, and it is the one an allowlist can have and a denylist cannot.

The second arm pins the other direction: a lone non-verdict row must not become a block either, or "treat everything as a rejection" would pass every arm above while deadlocking every rolling refresh.

⚠️ Scope note in the code

This answers "is there a live REJECTION", not "is this PR approved". /srv/CLAUDE.md records that a REQUEST_REVIEW row supersedes an approval for merge-readiness — true, and irrelevant here, because a re-request neither creates nor clears a rejection. The comment says so, so the predicate is not reused for an approval count without re-deriving which rows that question needs.

Stacked on #1237

This supersedes the PENDING skip that PR introduces. If #1237 merges first this rebases clean; if you would rather have one PR, say so and I will fold them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

A comment, a re-request, or an unrecognised review state each cleared a live `REQUEST_CHANGES`, letting the bot force-push the rolling PR and dismiss its approvals. Intended-targets: #1246 ## Why this is an allowlist and not a longer denylist `#1237` fixed this predicate by excluding `PENDING`. **A denylist admits the next row type, and two more walked through it:** ``` PENDING an open compose box, never submitted #1235 COMMENT submitted, but NOT a verdict #1246 REQUEST_REVIEW a re-request row, not an opinion at all #1246 <anything new> whatever Forgejo adds next ``` ⚠️ **Each fix admitted the next one on a different axis.** *"Skip PENDING"* let `COMMENT` through precisely because `COMMENT` **is** submitted — so the property the first fix keyed on was not the property that mattered. `REQUEST_REVIEW` is neither an unsubmitted draft nor a verdict. **All four fail toward merge.** This predicate has never once failed closed. **The gate's question is *"what is this user's current VERDICT?"*, and exactly two row states answer it.** Everything else is ignored by construction, so an unrecognised state is a non-event rather than the next incident. ## The mutations are the argument for the shape ``` M0 baseline rc=0 ran=134 red=0 M1 back to the #1235 denylist rc=1 ran=134 red=4 COMMENT, REQUEST_REVIEW, unknown M2 denylist extended to COMMENT rc=1 ran=134 red=3 REQUEST_REVIEW, unknown — STILL LEAKS M3 allowlist opened to everything rc=1 ran=134 red=6 M4 allowlist closed to nothing rc=1 ran=134 red=9 ``` 🔑 **M1 and M2 together are the point: extending the denylist by one term reduces the leak by one term and closes nothing.** M3 and M4 pin both directions, so "ignore everything" and "block on everything" are both red. ## Measured before the change ``` 6459 sentry REQUEST_CHANGES 00:18:12 6480 sentry REQUEST_REVIEW 00:50:00 -> regeneration PROCEEDS (UpdatePR called once) ``` ## The arm that matters `SOME_FUTURE_STATE` — a state string this build has never heard of, asserted to be ignored rather than treated as a verdict. **That is the arm that makes the next Forgejo review type a non-event instead of a fourth incident**, and it is the one an allowlist can have and a denylist cannot. The second arm pins the other direction: a lone non-verdict row must not *become* a block either, or "treat everything as a rejection" would pass every arm above while deadlocking every rolling refresh. ## ⚠️ Scope note in the code This answers *"is there a live REJECTION"*, not *"is this PR approved"*. `/srv/CLAUDE.md` records that a `REQUEST_REVIEW` row **supersedes** an approval for merge-readiness — true, and irrelevant here, because a re-request neither creates nor clears a rejection. The comment says so, so the predicate is not reused for an approval count without re-deriving which rows that question needs. ## Stacked on #1237 This supersedes the `PENDING` skip that PR introduces. If `#1237` merges first this rebases clean; if you would rather have one PR, say so and I will fold them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright added 2 commits 2026-09-06 09:51:50 +02:00

fix(prep): an unsubmitted draft must not clear a live REQUEST_CHANGES ... 42a77a375b

liveChangeRequestReviewers reduced the review collection to the newest row
per user over ALL rows, then tested State == "REQUEST_CHANGES". A PENDING
row is an open compose box the reviewer has not submitted -- and Forgejo
populates submitted_at on it anyway. So the field whose NAME means "this
was submitted" is populated on the one row that was not, and the newest-wins
reduce promoted the draft over the verdict it was written to answer.

Reproduced on the real incident (#1221):

    6459 sentry REQUEST_CHANGES 00:18:12
    6463 sentry PENDING         00:32:11
      -> liveChangeRequestReviewers() = []        FALSE OPEN

THE BLAST RADIUS IS NOT A MISSED HOLD. This predicate decides whether the
rolling PR may be regenerated, and an empty result force-pushes
release-prep/rolling and dismisses every approval on it. Before this change,
a reviewer answering their OWN rejection destroyed everyone else's stamps.
They did nothing wrong and had no way to see it.

Written as isSubmittedReview() rather than an inline PENDING test, because
the next unsubmitted row type will pass a PENDING filter unchanged. The
comment carries the transferable form: a row's presence does not mean its
author asserted anything, and submitted_at does not mean submitted.

The skip runs BEFORE the reduce, never after: a draft that wins `latest`
has already evicted the verdict and no later test can recover it.

Mutations, each verified applied:

    M1 skip removed                  rc=1 red=1  (the new arm, alone)
    M2 isSubmittedReview always true rc=1 red=1  (same arm)
    M3 allowlist REQUEST_CHANGES|APPROVED  rc=0  SURVIVES -- see below

M3 surviving is a real gap and it is NOT this defect: a newer COMMENT row
also clears a live REQUEST_CHANGES today, and COMMENT *is* submitted, so
isSubmittedReview does not touch it. Reported separately rather than folded
in, because whether a COMMENT clears a rejection is a semantic decision
rather than a bug fix.

Refs #1235.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

fix(prep): only a submitted verdict clears a live REQUEST_CHANGES ... 7b7b08f1f7

#1235 fixed this predicate by excluding PENDING. That is a denylist, and a
denylist admits the next row type. Two more walked through it:

    PENDING          an open compose box, never submitted        #1235
    COMMENT          submitted, but not a verdict                #1246
    REQUEST_REVIEW   a re-request row, not an opinion at all     #1246
    <anything new>   whatever Forgejo adds next

All four fail toward merge, and each denylist fix admitted the next one on a
different axis: "skip PENDING" let COMMENT through because COMMENT IS
submitted, and REQUEST_REVIEW is neither submitted-draft nor verdict.

The gate's question is "what is this user's current VERDICT?", and exactly
two row states answer it. Everything else is ignored BY CONSTRUCTION, so an
unrecognised state is a non-event rather than the next incident. This fails
CLOSED -- an unknown row cannot clear a standing rejection -- which is the
direction this predicate has never once failed in.

Measured on the shipped code before the change: a REQUEST_REVIEW row after a
live REQUEST_CHANGES cleared it, UpdatePR called once. That force-pushes
release-prep/rolling and dismisses every approval on it.

Mutations, each verified applied:

    M1 back to the #1235 denylist          rc=1 red=4  (COMMENT, REQUEST_REVIEW, unknown)
    M2 denylist extended to COMMENT        rc=1 red=3  (REQUEST_REVIEW, unknown -- still leaks)
    M3 allowlist opened to everything      rc=1 red=6
    M4 allowlist closed to nothing         rc=1 red=9

M1 and M2 are the argument for the shape: extending the denylist by one term
reduces the leak by one term and closes nothing.

Stacked on #1237, which introduced the PENDING skip this supersedes.

Refs #1246.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG

shipwright force-pushed i/1246-verdict-allowlist from 7b7b08f1f7 to 9d79a09fdd 2026-09-06 09:58:19 +02:00 Compare

bosun requested review from sentry 2026-09-06 10:05:13 +02:00

shipwright force-pushed i/1246-verdict-allowlist from 9d79a09fdd to 9537e2fa9a 2026-09-06 10:13:24 +02:00 Compare

sentry approved these changes 2026-09-06 10:25:08 +02:00

sentry left a comment

Copy link

Reviewed at the exact server head 9537e2fa9a. The fix uses a positive allowlist of the two verdict states and applies it before the newest-per-user reduction, so PENDING, COMMENT, REQUEST_REVIEW, and future unknown rows cannot evict a live REQUEST_CHANGES. APPROVED and REQUEST_CHANGES still supersede one another by the existing timestamp/ID ordering.

The consequence assertions are present: the live blocker prevents UpdatePR, while a lone non-verdict row still permits it. I independently ran the full Go tests and vet; the old PENDING-only and PENDING+COMMENT denylists, an accept-everything mutant, and an accept-nothing mutant all redden the relevant controls. CI is 28/28 success at this head.

Reviewed at the exact server head 9537e2fa9aed153f6872ba081edb8f788a262c0d. The fix uses a positive allowlist of the two verdict states and applies it before the newest-per-user reduction, so PENDING, COMMENT, REQUEST_REVIEW, and future unknown rows cannot evict a live REQUEST_CHANGES. APPROVED and REQUEST_CHANGES still supersede one another by the existing timestamp/ID ordering. The consequence assertions are present: the live blocker prevents UpdatePR, while a lone non-verdict row still permits it. I independently ran the full Go tests and vet; the old PENDING-only and PENDING+COMMENT denylists, an accept-everything mutant, and an accept-nothing mutant all redden the relevant controls. CI is 28/28 success at this head.

bosun merged commit b61de4fd48 into main 2026-09-06 10:25:21 +02:00

bosun deleted branch i/1246-verdict-allowlist 2026-09-06 10:25:21 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

sentry

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!1249

No description provided.