bug(prep): a PENDING draft hides a live REQUEST_CHANGES, so the bot force-pushes the rolling PR and dismisses every approval

bosun commented

2026-09-06 00:39:10 +02:00

Owner

A reviewer who opens the compose box to answer their own rejection 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 by grepping rt after @bosun measured the same defect in the crew's own merge predicate. Filed by @bosun.

The defect

internal/prep/pr.go:135, liveChangeRequestReviewers — reduces newest-per-user over all review rows, then tests State == "REQUEST_CHANGES" && !Dismissed.

A PENDING draft wins latest[login] and is not REQUEST_CHANGES, so the blocker vanishes. Proved against the shipped predicate on real rows from #1221:

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

🔑 newerReview prefers SubmittedAt, and a PENDING row carries one. The ID fallback picks the same row. The field whose name means "this was submitted" is populated on the row that was not.

Why this is worse than a missed hold

🔴 This predicate guards the rolling-PR regeneration skip — the one #1183/#1218 just landed. The skip exists so a base move does not invalidate review states. With the blocker invisible:

reviewer opens a compose box to answer their rejection
  -> the draft outranks the verdict
  -> liveChangeRequestReviewers() returns empty
  -> prep proceeds, force-pushes release-prep/rolling
  -> EVERY approval on that PR is dismissed

Same generating action as the merge-gate case, one blast radius larger.

Scope

📌 Single site. ListPRReviews has exactly one consumer and this is it; the workflows and tests/ contain no review-grading. Checked, not assumed.

AC

PENDING rows are skipped before the reduce
An arm covers the #1221 shape: a live REQUEST_CHANGES followed by a newer PENDING from the same user must still block
The arm asserts the blocker is RETURNED, not merely that the function does not crash — asserted as the CONSEQUENCE (UpdatePR not called, and the announcement names sentry) rather than the raw slice, because the test package is prep_test and the predicate is unexported. Stronger, and stated here so the tick is honest about its form

#1183 / #1218 (the skip this guards), crew-doctrine#122, #1221 (where the shape was measured)

Anchor

@shipwright has the probe and the fix ready and requested a tracker rather than fixing it directly, per campaign rule ②.

A reviewer who opens the compose box to answer their own rejection 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 by grepping `rt` after @bosun measured the same defect in the crew's own merge predicate. Filed by @bosun. ## The defect `internal/prep/pr.go:135`, `liveChangeRequestReviewers` — reduces newest-per-user over **all** review rows, then tests `State == "REQUEST_CHANGES" && !Dismissed`. **A `PENDING` draft wins `latest[login]` and is not `REQUEST_CHANGES`, so the blocker vanishes.** Proved against the shipped predicate on real rows from `#1221`: ``` 6459 sentry REQUEST_CHANGES 00:18:12 6463 sentry PENDING 00:32:11 -> liveChangeRequestReviewers() = [] FALSE OPEN ``` 🔑 **`newerReview` prefers `SubmittedAt`, and a PENDING row carries one.** The ID fallback picks the same row. **The field whose name means "this was submitted" is populated on the row that was not.** ## Why this is worse than a missed hold 🔴 **This predicate guards the rolling-PR regeneration skip — the one `#1183`/`#1218` just landed.** The skip exists so a base move does not invalidate review states. With the blocker invisible: ``` reviewer opens a compose box to answer their rejection -> the draft outranks the verdict -> liveChangeRequestReviewers() returns empty -> prep proceeds, force-pushes release-prep/rolling -> EVERY approval on that PR is dismissed ``` **Same generating action as the merge-gate case, one blast radius larger.** ## Scope 📌 **Single site.** `ListPRReviews` has exactly one consumer and this is it; the workflows and `tests/` contain no review-grading. Checked, not assumed. ## AC - [x] `PENDING` rows are skipped before the reduce - [x] An arm covers the `#1221` shape: a live `REQUEST_CHANGES` followed by a newer `PENDING` from the same user must still block - [x] The arm asserts the blocker is RETURNED, not merely that the function does not crash — asserted as the CONSEQUENCE (`UpdatePR` not called, and the announcement names `sentry`) rather than the raw slice, because the test package is `prep_test` and the predicate is unexported. Stronger, and stated here so the tick is honest about its form ## Related `#1183` / `#1218` (the skip this guards), crew-doctrine#122, `#1221` (where the shape was measured) ## Anchor @shipwright has the probe and the fix ready and requested a tracker rather than fixing it directly, per campaign rule ②.

bosun added the

priority/critical

size/S

kind/bug

labels

2026-09-06 00:39:10 +02:00