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

Member

• Copy link
• Copy Markdown

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 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
• Copy Markdown

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

epic

Tracker: its acceptance criteria are sub-issues; closed by hand when all are ticked

  

kind

feature

New feature or capability

  

loop/paused

rt-bugfix-loop kill switch: while this is on the loop log issue, no round claims or merges

  

mirror-drift

Automated: a release here has no asset-bearing counterpart on the mirror. One tracker per drift episode; the check never reopens a closed one (#1586)

  

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+

  

status

blocked

Cannot start until a named dependency resolves

No labels

bump

major

bump

minor

bump

patch

kind

bug

kind

chore

kind

docs

kind

epic

kind

feature

loop/paused

mirror-drift

priority

critical

priority

high

priority

low

priority

medium

size

L

size

M

size

S

size

XL

status

blocked

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.