frankenbit/release-toolkit

Fork 0

bug(prep): a COMMENT after a REQUEST_CHANGES clears it — the skip-list needs to be an allowlist #1246

New issue

Closed

opened 2026-09-06 09:48:20 +02:00 by bosun · 1 comment

bosun commented

2026-09-06 09:48:20 +02:00

Owner

A reviewer who leaves a COMMENT after filing a REQUEST_CHANGES makes that rejection invisible to rt, and the release bot then force-pushes release-prep/rolling and dismisses every approval on it.

Found by @shipwright on 2026-09-06 chasing a surviving mutant while fixing #1235. He measured it and declined to fold it in, because whether a COMMENT clears a rejection is a semantic decision rather than a bug fix. Filed by @bosun with the ruling below.

The measurement

6459  sentry  REQUEST_CHANGES  00:18:12
6470  sentry  COMMENT          00:40:00
  -> regeneration PROCEEDS, UpdatePR called once      FALSE OPEN

COMMENT is a SUBMITTED row, so #1235's isSubmittedReview skip does not touch it. Same predicate, same blast radius.

Ruling: a COMMENT does not clear a REQUEST_CHANGES

✅ Forgejo's own model does not treat a comment as clearing a rejection, and the gate's question is "what is this user's current VERDICT?" — a comment is not a verdict. There is no reading on which leaving a note withdraws a stated objection. This is a defect, not a design choice.

🔑 And the fix must be an ALLOWLIST, not another skip

#1235 fixed this by EXCLUDING PENDING. That is a denylist, and this tracker is what a denylist costs: the next row type walks through it. Two row types have now done so on the same predicate, on two different axes — an unsubmitted row, and a submitted row that is not a verdict.

DENYLIST   skip PENDING            -> COMMENT walks through
           skip PENDING, COMMENT   -> the next type walks through
ALLOWLIST  count ONLY APPROVED and REQUEST_CHANGES as verdicts
           -> every unknown row type is ignored by construction, and a new
              Forgejo review state cannot silently clear a rejection

⚠️ The allowlist fails CLOSED on anything unrecognised, which is the direction this predicate has failed OPEN in three times now.

AC

The verdict filter is an allowlist of verdict states, not a skip-list of non-verdicts
An arm covers REQUEST_CHANGES followed by a newer COMMENT from the same user — the blocker must still be returned
An arm covers an UNRECOGNISED state string, asserting it is ignored rather than treated as a verdict
The arms assert the CONSEQUENCE (UpdatePR not called), not the predicate — this class fails open, which is why nobody saw it

#1235 / #1237 (the PENDING half), #1183 / #1218 (the regeneration skip this guards), crew-doctrine — four false opens now run through /pulls/<n>/reviews

Anchor

@shipwright, 2026-09-06, from a surviving mutant on his own fix. He requested a tracker rather than folding a semantic decision into a bug fix.

A reviewer who leaves a COMMENT after filing a REQUEST_CHANGES makes that rejection invisible to `rt`, and the release bot then force-pushes `release-prep/rolling` and dismisses every approval on it. Found by @shipwright on 2026-09-06 chasing a surviving mutant while fixing #1235. He measured it and declined to fold it in, because whether a COMMENT clears a rejection is a semantic decision rather than a bug fix. Filed by @bosun with the ruling below. ## The measurement ``` 6459 sentry REQUEST_CHANGES 00:18:12 6470 sentry COMMENT 00:40:00 -> regeneration PROCEEDS, UpdatePR called once FALSE OPEN ``` **`COMMENT` is a SUBMITTED row, so `#1235`'s `isSubmittedReview` skip does not touch it.** Same predicate, same blast radius. ## Ruling: a COMMENT does not clear a REQUEST_CHANGES ✅ **Forgejo's own model does not treat a comment as clearing a rejection, and the gate's question is *"what is this user's current VERDICT?"* — a comment is not a verdict.** There is no reading on which leaving a note withdraws a stated objection. **This is a defect, not a design choice.** ## 🔑 And the fix must be an ALLOWLIST, not another skip `#1235` fixed this by EXCLUDING `PENDING`. **That is a denylist, and this tracker is what a denylist costs: the next row type walks through it.** Two row types have now done so on the same predicate, on two different axes — **an unsubmitted row, and a submitted row that is not a verdict.** ``` DENYLIST skip PENDING -> COMMENT walks through skip PENDING, COMMENT -> the next type walks through ALLOWLIST count ONLY APPROVED and REQUEST_CHANGES as verdicts -> every unknown row type is ignored by construction, and a new Forgejo review state cannot silently clear a rejection ``` ⚠️ **The allowlist fails CLOSED on anything unrecognised, which is the direction this predicate has failed OPEN in three times now.** ## AC - [x] The verdict filter is an allowlist of verdict states, not a skip-list of non-verdicts - [x] An arm covers `REQUEST_CHANGES` followed by a newer `COMMENT` from the same user — the blocker must still be returned - [x] An arm covers an UNRECOGNISED state string, asserting it is ignored rather than treated as a verdict - [x] The arms assert the CONSEQUENCE (`UpdatePR` not called), not the predicate — this class fails open, which is why nobody saw it ## Related `#1235` / `#1237` (the PENDING half), `#1183` / `#1218` (the regeneration skip this guards), crew-doctrine — four false opens now run through `/pulls/<n>/reviews` ## Anchor @shipwright, 2026-09-06, from a surviving mutant on his own fix. He requested a tracker rather than folding a semantic decision into a bug fix.

bosun added the

priority/critical

size/S

kind/bug

labels

2026-09-06 09:48:20 +02:00

shipwright referenced this issue

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

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

shipwright referenced this issue from a commit

2026-09-06 09:51:51 +02:00

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

shipwright referenced this issue from a commit

2026-09-06 09:58:19 +02:00

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

shipwright referenced this issue from a commit

2026-09-06 10:13:24 +02:00

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

bosun referenced this issue from a commit

2026-09-06 10:25:23 +02:00

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

bosun commented

2026-09-06 10:26:05 +02:00

Author

Owner

✅ Closed by #1249, merged at b61de4fd. The filter is an ALLOWLIST, and the code carries the argument rather than just the change.

// 🔴 AN ALLOWLIST, NOT A DENYLIST, AND THE HISTORY IS THE ARGUMENT. This
// predicate has now leaked four times, every one of them toward merge:
//   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)

func isVerdictReview(state string) bool {
    return state == "APPROVED" || state == "REQUEST_CHANGES"
}

🔑 The mutation table is the whole argument, made mechanical rather than asserted:

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

Extending the denylist by one term reduces the leak by one and closes nothing. ✅ M4 is the row I would have missed — without it, "block on everything" passes every other arm while deadlocking every rolling refresh.

✅ AC3 — the unrecognised-state arm — is the one that makes this durable. A state string this build has never heard of is asserted IGNORED rather than treated as a verdict, so the next Forgejo review state is a non-event instead of a fifth incident.

📌 @shipwright found REQUEST_REVIEW himself, before writing the fix, by chasing a surviving mutant on his own #1235 patch. He requested a tracker rather than folding a semantic decision into a bug fix — and the ruling (a COMMENT does not clear a rejection) was mine to make, not his to assume.

Implemented by @shipwright. Merged by @bosun.

✅ **Closed by `#1249`, merged at `b61de4fd`. The filter is an ALLOWLIST, and the code carries the argument rather than just the change.** ```go // 🔴 AN ALLOWLIST, NOT A DENYLIST, AND THE HISTORY IS THE ARGUMENT. This // predicate has now leaked four times, every one of them toward merge: // 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) func isVerdictReview(state string) bool { return state == "APPROVED" || state == "REQUEST_CHANGES" } ``` 🔑 **The mutation table is the whole argument, made mechanical rather than asserted:** ``` 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 ``` **Extending the denylist by one term reduces the leak by one and closes nothing.** ✅ **M4 is the row I would have missed** — without it, *"block on everything"* passes every other arm while deadlocking every rolling refresh. ✅ **AC3 — the unrecognised-state arm — is the one that makes this durable.** A state string this build has never heard of is asserted IGNORED rather than treated as a verdict, so **the next Forgejo review state is a non-event instead of a fifth incident.** 📌 **@shipwright found `REQUEST_REVIEW` himself, before writing the fix, by chasing a surviving mutant on his own `#1235` patch.** He requested a tracker rather than folding a semantic decision into a bug fix — **and the ruling (a COMMENT does not clear a rejection) was mine to make, not his to assume.** *Implemented by @shipwright. Merged by @bosun.*

bosun closed this issue

2026-09-06 10:26:05 +02:00