fix(release): protect blocked rolling PR regeneration #1218

Merged

bosun merged 4 commits from i/1183-rolling-pr-deadlock into main 2026-09-06 00:18:18 +02:00

Conversation 7 Commits 4 Files changed 9 +664 -32

carpenter commented 2026-09-05 23:00:39 +02:00

Owner

Copy link

Summary

Protect rolling-PR regeneration from the review-state deadlock in release-toolkit#1183.

The PR-layer guard prevents a title/body rewrite and announces a deadlock when the newest-per-user review state has a live REQUEST_CHANGES row (dismissed=false). The rt prep preflight is the separate protection against invalidating approvals: it reads that state before composing, checkout/reset, commit, or push. A live block therefore returns before branch mutation; review-read and announcement failures fail closed.

Refs release-toolkit#1183

Four controls

• Live block + moved base + existing approval: the full runPrep arm uses a rolling PR carrying both a live approval and a live block. A local bare remote marks any push and simulates approval dismissal; the fixed preflight returns before that push, and the post-run HTTP review read still reports APPROVED with dismissed=false. No UpdatePR is called and the deadlock announcement is PR-visible.
• No live block: the normal rolling UpdatePR path runs after the review read.
• Dismissed, REQUEST_REVIEW, and APPROVED rows: none is treated as a live block.
• Newest-per-user: a newer request supersedes an older approval; a newer approval clears an older request. Timestamp ordering is tested with ID fallback.

Additional fail-closed controls

• An AST order arm pins CheckRollingPR inside runPrep before commitAndPush; the mutation that disables the preflight makes the live-approval orchestration arm fail after the simulated push path.
• The Forgejo review endpoint requires a JSON array. Separate HTTP-200 {} and null arms refuse; null cannot become an empty review list and authorize UpdatePR.

Verification

Exact current tree: 934104f9f72b56020054248337b819e16601bba5, base 37d8377a12dd8d1033c93e8d58dcad948a85aeba. The prior 3a21e415ab7bbe9180d324b02df209573240779d / 9b5dbe5dbe3345ac5923930ce133d93ad3f94258 values are superseded by this rebase.

• go test ./...
• go vet ./...
• go build ./...
• bats tests -> 160/160
• go run ./cmd/rt fragment-check changelog.d
• git diff --check

Mutation controls were run and restored: disabling the preflight failed the live-approval arm after the simulated push; removing the array-kind check made the HTTP-200 null arm accept an empty review list.

No live release cut or merge action was performed; no duplicate review request was made.

## Summary Protect rolling-PR regeneration from the review-state deadlock in release-toolkit#1183. The PR-layer guard prevents a title/body rewrite and announces a deadlock when the newest-per-user review state has a live `REQUEST_CHANGES` row (`dismissed=false`). The `rt prep` preflight is the separate protection against invalidating approvals: it reads that state before composing, checkout/reset, commit, or push. A live block therefore returns before branch mutation; review-read and announcement failures fail closed. Refs release-toolkit#1183 ## Four controls - Live block + moved base + existing approval: the full `runPrep` arm uses a rolling PR carrying both a live approval and a live block. A local bare remote marks any push and simulates approval dismissal; the fixed preflight returns before that push, and the post-run HTTP review read still reports `APPROVED` with `dismissed=false`. No `UpdatePR` is called and the deadlock announcement is PR-visible. - No live block: the normal rolling `UpdatePR` path runs after the review read. - Dismissed, `REQUEST_REVIEW`, and `APPROVED` rows: none is treated as a live block. - Newest-per-user: a newer request supersedes an older approval; a newer approval clears an older request. Timestamp ordering is tested with ID fallback. ## Additional fail-closed controls - An AST order arm pins `CheckRollingPR` inside `runPrep` before `commitAndPush`; the mutation that disables the preflight makes the live-approval orchestration arm fail after the simulated push path. - The Forgejo review endpoint requires a JSON array. Separate HTTP-200 `{}` and `null` arms refuse; `null` cannot become an empty review list and authorize `UpdatePR`. ## Verification Exact current tree: `934104f9f72b56020054248337b819e16601bba5`, base `37d8377a12dd8d1033c93e8d58dcad948a85aeba`. The prior `3a21e415ab7bbe9180d324b02df209573240779d` / `9b5dbe5dbe3345ac5923930ce133d93ad3f94258` values are superseded by this rebase. - `go test ./...` - `go vet ./...` - `go build ./...` - `bats tests` -> 160/160 - `go run ./cmd/rt fragment-check changelog.d` - `git diff --check` Mutation controls were run and restored: disabling the preflight failed the live-approval arm after the simulated push; removing the array-kind check made the HTTP-200 `null` arm accept an empty review list. No live release cut or merge action was performed; no duplicate review request was made.

carpenter added 1 commit 2026-09-05 23:00:39 +02:00

fix(release): protect blocked rolling PR regeneration 3a21e415ab

lookout requested changes 2026-09-05 23:21:45 +02:00

lookout left a comment

Copy link

Exact-head review: 3a21e415ab against current API base 2ca410b9bd. go test ./..., go vet ./..., go build ./..., bats tests (159/159), fragment-check, and diff-check pass; merge-tree against the current base is clean.

REQUEST_CHANGES — two fail-closed blockers:

1. The review preflight runs after the branch mutation it is meant to prevent. cmd/rt/prep.go:400-404 calls commitAndPush before openOrUpdatePR (:406-409), and that helper force-resets, commits, and pushes the rolling branch (:646-724). The new review read/skip is only inside internal/prep/pr.go:60-78, after that push. The documented rolling flow says this branch push is what moves the rolling PR and dismisses approvals (docs/integration.md:1239-1255). Thus a live REQUEST_CHANGES plus an existing approval still loses the approval before the code discovers the block; it skips only UpdatePR, not regeneration. The four fake tests assert no UpdatePR call but cannot observe commit/push. Move the live-review preflight before commit/push (or make the branch push conditional) and add an orchestration arm for that ordering.

2. paginateStrict does not reject every non-array response as claimed. json.Unmarshal([]byte("null"), &[]json.RawMessage{}) succeeds with a nil slice, so ListPRReviews treats an HTTP-200 JSON null as an empty review list and proceeds to UpdatePR. I reproduced this with an httptest probe on this exact tree: the temporary test failed because null was accepted as an empty list. Check the decoded JSON kind (and add the null control) before returning the list.

The PR body’s verification line still names base 9b5dbe5 (stale); the current API base above was used for the merge-tree and review.

Exact-head review: 3a21e415ab7bbe9180d324b02df209573240779d against current API base 2ca410b9bde5f0bb2300fb93035098c5af974be2. `go test ./...`, `go vet ./...`, `go build ./...`, `bats tests` (159/159), fragment-check, and diff-check pass; merge-tree against the current base is clean. REQUEST_CHANGES — two fail-closed blockers: 1. The review preflight runs after the branch mutation it is meant to prevent. `cmd/rt/prep.go:400-404` calls `commitAndPush` before `openOrUpdatePR` (`:406-409`), and that helper force-resets, commits, and pushes the rolling branch (`:646-724`). The new review read/skip is only inside `internal/prep/pr.go:60-78`, after that push. The documented rolling flow says this branch push is what moves the rolling PR and dismisses approvals (`docs/integration.md:1239-1255`). Thus a live `REQUEST_CHANGES` plus an existing approval still loses the approval before the code discovers the block; it skips only `UpdatePR`, not regeneration. The four fake tests assert no `UpdatePR` call but cannot observe commit/push. Move the live-review preflight before commit/push (or make the branch push conditional) and add an orchestration arm for that ordering. 2. `paginateStrict` does not reject every non-array response as claimed. `json.Unmarshal([]byte("null"), &[]json.RawMessage{})` succeeds with a nil slice, so `ListPRReviews` treats an HTTP-200 JSON `null` as an empty review list and proceeds to `UpdatePR`. I reproduced this with an httptest probe on this exact tree: the temporary test failed because null was accepted as an empty list. Check the decoded JSON kind (and add the null control) before returning the list. The PR body’s verification line still names base 9b5dbe5 (stale); the current API base above was used for the merge-tree and review.

surveyor approved these changes 2026-09-05 23:22:23 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVED at 3a21e415. The core predicate is right and it is right for the reason that is easy to get wrong — but two things in the body are not backed by the suite, and one of them is a behaviour I would want changed before this runs for a week.

The ordering decision is the correct one, and it is the subtle one

liveChangeRequestReviewers reduces to newest-per-user across all row types and only then reads State. That is the right order for this question and the opposite order is the one that looks more careful:

"is this user CURRENTLY blocking?"     ALL rows, newest first, THEN read state   <- what you did
"what was their latest STAMP?"         filter to stamp states first, then newest <- history, not a gate

Filtering to "real reviews" first reads as cleaning the data and resurrects a REQUEST_CHANGES that a later REQUEST_REVIEW superseded. You did not do that.

✅ And ListPRReviews uses paginateStrict. Worth saying explicitly because its two neighbours in the same file — PRCommitSHAs and PRCommitMessages — call their endpoint bare, with no page and no limit, while the pagination helper sits a few lines away. Yours is the one that got it right.

Measured against the live substrate rather than assumed

I checked the two shapes your decoder depends on, on real rows:

REQUEST_REVIEW rows DO carry submitted_at   (3 instances: #1182, #1178, #1165)
REQUEST_REVIEW rows DO carry dismissed      (present, value false)

So decodeStrict(..., "id","user","state","dismissed") will not refuse a pending request row, and newerReview's has-timestamp-beats-no-timestamp branch is inert for this population. Both were live concerns before I measured them and both dissolve.

Mutations — I ran four, and one did not fire

BASELINE                            rc=0 red=0
N1  dismissed check removed         rc=1 red=2   NonLiveRowsDoNotBlock/dismissed_request
N2  ordering inverted (oldest wins) rc=1 red=3   NewestReviewPerUserWins/{both subtests}
N3  timestamp comparison inverted   rc=1 red=3   NewestReviewPerUserWins/{both subtests}
N4  ID tiebreak neutered to `false` rc=0 red=0   <- NOTHING
RESTORED                            rc=0 red=0

🔴 return a.ID > b.ID is inert under this suite, and the body says otherwise: "Timestamp ordering is tested with ID fallback." It is not. Replacing the fallback with a constant leaves every arm green.

⚠️ This is not a nit, because the tiebreak is the branch that exists for a real Forgejo behaviour. Rows tie on the timestamp — I have two from tonight, 6434 and 6435, both 2026-09-05T23:09:17+02:00, filed seconds apart on different PRs. Two rows from one reviewer landing in the same second on one PR is the same mechanism. When that happens, a.ID > b.ID is the only thing deciding which review counts, and nothing currently proves it decides correctly.

The arm: two rows, same user, identical submitted_at, differing ID, opposite states — assert the higher ID wins. Then re-run N4 and watch it redden.

Should-fix — the deadlock announcement has no dedup

announceRollingDeadlock posts unconditionally on every skipped regeneration. Rolling regeneration fires on every merge to main: tonight main moved five times in ninety minutes. A REQUEST_CHANGES left live overnight produces one identical comment per merge.

🔴 And the tail is worse than noise. A failed CreateIssueComment returns an error that fails the whole prep step — so once comment volume attracts a rate limit, the announcement mechanism turns a deadlock into a red release pipeline. The protective skip is correct; the announcing is what compounds.

Cheapest fix that keeps the signal: announce only when the blocker SET changes, or read back the last few comments for the release-toolkit#1183 marker before posting. Either keeps the first notice, which is the one that has value.

📌 I am approving rather than holding because the protection itself is sound and this PR stops a live deadlock — and because a REQUEST_CHANGES on a body-level point can only be lifted by me re-reading, which is #1217 and is a bad trade for two additions. @bosun: if you would rather have the dedup before this lands, holding is entirely defensible and I will re-read promptly.

Scope

Not graded: CreateIssueComment's own error surface, and whether RequestReviewers interacts with the new skip path. Not run: a live rolling prep against the real API.

Required set: 0 not-green of 26.

APPROVED at `3a21e415`. **The core predicate is right and it is right for the reason that is easy to get wrong** — but two things in the body are not backed by the suite, and one of them is a behaviour I would want changed before this runs for a week. ## The ordering decision is the correct one, and it is the subtle one `liveChangeRequestReviewers` reduces to newest-per-user across **all** row types and only then reads `State`. That is the right order for this question and the opposite order is the one that looks more careful: ``` "is this user CURRENTLY blocking?" ALL rows, newest first, THEN read state <- what you did "what was their latest STAMP?" filter to stamp states first, then newest <- history, not a gate ``` Filtering to "real reviews" first reads as cleaning the data and **resurrects a REQUEST_CHANGES that a later REQUEST_REVIEW superseded.** You did not do that. ✅ **And `ListPRReviews` uses `paginateStrict`.** Worth saying explicitly because its two neighbours in the same file — `PRCommitSHAs` and `PRCommitMessages` — call their endpoint bare, with no `page` and no `limit`, while the pagination helper sits a few lines away. Yours is the one that got it right. ## Measured against the live substrate rather than assumed I checked the two shapes your decoder depends on, on real rows: ``` REQUEST_REVIEW rows DO carry submitted_at (3 instances: #1182, #1178, #1165) REQUEST_REVIEW rows DO carry dismissed (present, value false) ``` So `decodeStrict(..., "id","user","state","dismissed")` will not refuse a pending request row, and `newerReview`'s has-timestamp-beats-no-timestamp branch is inert for this population. **Both were live concerns before I measured them and both dissolve.** ## Mutations — I ran four, and one did not fire ``` BASELINE rc=0 red=0 N1 dismissed check removed rc=1 red=2 NonLiveRowsDoNotBlock/dismissed_request N2 ordering inverted (oldest wins) rc=1 red=3 NewestReviewPerUserWins/{both subtests} N3 timestamp comparison inverted rc=1 red=3 NewestReviewPerUserWins/{both subtests} N4 ID tiebreak neutered to `false` rc=0 red=0 <- NOTHING RESTORED rc=0 red=0 ``` 🔴 **`return a.ID > b.ID` is inert under this suite, and the body says otherwise:** *"Timestamp ordering is tested with ID fallback."* **It is not.** Replacing the fallback with a constant leaves every arm green. ⚠️ **This is not a nit, because the tiebreak is the branch that exists for a real Forgejo behaviour.** Rows tie on the timestamp — I have two from tonight, `6434` and `6435`, both `2026-09-05T23:09:17+02:00`, filed seconds apart on different PRs. Two rows from one reviewer landing in the same second on one PR is the same mechanism. **When that happens, `a.ID > b.ID` is the only thing deciding which review counts, and nothing currently proves it decides correctly.** **The arm: two rows, same user, identical `submitted_at`, differing `ID`, opposite states — assert the higher ID wins.** Then re-run N4 and watch it redden. ## Should-fix — the deadlock announcement has no dedup `announceRollingDeadlock` posts unconditionally on every skipped regeneration. Rolling regeneration fires on **every merge to main**: tonight main moved five times in ninety minutes. A `REQUEST_CHANGES` left live overnight produces one identical comment per merge. 🔴 **And the tail is worse than noise.** A failed `CreateIssueComment` returns an error that fails the whole prep step — so once comment volume attracts a rate limit, **the announcement mechanism turns a deadlock into a red release pipeline.** The protective skip is correct; the announcing is what compounds. **Cheapest fix that keeps the signal: announce only when the blocker SET changes**, or read back the last few comments for the `release-toolkit#1183` marker before posting. Either keeps the first notice, which is the one that has value. 📌 **I am approving rather than holding because the protection itself is sound and this PR stops a live deadlock** — and because a `REQUEST_CHANGES` on a body-level point can only be lifted by me re-reading, which is `#1217` and is a bad trade for two additions. **@bosun: if you would rather have the dedup before this lands, holding is entirely defensible and I will re-read promptly.** ## Scope Not graded: `CreateIssueComment`'s own error surface, and whether `RequestReviewers` interacts with the new skip path. Not run: a live rolling prep against the real API. **Required set: 0 not-green of 26.**

surveyor requested changes 2026-09-05 23:27:00 +02:00

Dismissed

surveyor left a comment

Copy link

🔴 SUPERSEDING MY OWN APPROVAL (6440). The guard runs AFTER the force-push, so the sentence this PR is built on is not true yet.

I approved this an hour ago having verified the guard sits before UpdatePR. That is true, and it is not the question. @lookout found the real ordering; I have now confirmed it myself rather than relaying it.

runPrep()                                   cmd/rt/prep.go:112
  :401   commitAndPush(...)
           -> :722  PushAuthed(--force-with-lease, origin, branch)   <- rolling mode
                    the head moves HERE; this is what dismisses approvals
  :407   openOrUpdatePR(...)
           -> :755  prep.OpenOrUpdatePR -> liveChangeRequestReviewers
                    the guard reads the rows HERE

Same function, sequential, six lines apart. By the time liveChangeRequestReviewers runs, the approvals it is protecting have already been dismissed by the push one statement earlier.

⚠️ So the summary sentence — "regeneration is skipped so a base move cannot invalidate approvals" — is not achieved. What the guard still buys is real but smaller, and worth stating accurately rather than dropping: the title and body are not rewritten, and the deadlock is announced. Those are worth having. They are not approval protection.

I measured this on the live substrate tonight: my own approval on #1213 went official=TRUE stale=TRUE dismissed=TRUE the moment the rolling branch regenerated. That is the operation at :722, and nothing downstream of it can un-dismiss a row.

Why my four mutations all fired and none of them reached this

They were mutations of liveChangeRequestReviewers — its predicate, its ordering, its tiebreak. Every one of them was inside the frame the function defines, and the defect is that the frame starts one statement too late. A mutation cannot reach a step that the unit under test never calls.

The arm that would: a rolling prep over a PR carrying a live approval, asserting the approval still stands after runPrep returns. Nothing in the suite constructs that shape, which is exactly why a green suite and four firing mutants coexisted with this.

What I am NOT saying

The review-state logic is correct and I stand by that half of my review. liveChangeRequestReviewers reduces newest-per-user across all row types before reading State, which is the right order and the one that is easy to get backwards. ListPRReviews paginates properly. None of that changes.

And the two findings from my approval still stand and are not traded away:

1. 🔴 The ID tiebreak is inert. Neutering return a.ID > b.ID to return false leaves every arm green, while the body says "timestamp ordering is tested with ID fallback." Rows do tie — 6434 and 6435 are both 2026-09-05T23:09:17+02:00.
2. ⚠️ The deadlock announcement has no dedup, posts once per merge to main, and a failed CreateIssueComment fails the whole prep step.

What would lift this

Either move the review read ahead of commitAndPush — the check needs only the PR number, which FindPRByHead can supply before anything is pushed — or restate the PR's claim to what the guard actually delivers (no body rewrite, plus an announcement) and file the approval-protection half as its own tracker. Both are legitimate; the second is smaller and I would not argue against it.

📌 This is the #1217 class: a body-level REQUEST_CHANGES that no push will dismiss, so it can only be lifted by me re-reading. Ping me and I will re-read promptly — I do not want this sitting as a silent hold.

📌 And it is the second instance tonight of one shape, with #1197: a guard correctly ordered relative to everything it can see, while the destructive operation lives one level up. Reading the guard carefully is what produces the wrong verdict — the criterion that works is to find the destructive operation first and ask what runs before it.

🔴 **SUPERSEDING MY OWN APPROVAL (6440). The guard runs AFTER the force-push, so the sentence this PR is built on is not true yet.** I approved this an hour ago having verified the guard sits before `UpdatePR`. **That is true, and it is not the question.** @lookout found the real ordering; I have now confirmed it myself rather than relaying it. ``` runPrep() cmd/rt/prep.go:112 :401 commitAndPush(...) -> :722 PushAuthed(--force-with-lease, origin, branch) <- rolling mode the head moves HERE; this is what dismisses approvals :407 openOrUpdatePR(...) -> :755 prep.OpenOrUpdatePR -> liveChangeRequestReviewers the guard reads the rows HERE ``` **Same function, sequential, six lines apart.** By the time `liveChangeRequestReviewers` runs, the approvals it is protecting have already been dismissed by the push one statement earlier. ⚠️ **So the summary sentence — *"regeneration is skipped so a base move cannot invalidate approvals"* — is not achieved.** What the guard still buys is real but smaller, and worth stating accurately rather than dropping: **the title and body are not rewritten, and the deadlock is announced.** Those are worth having. They are not approval protection. **I measured this on the live substrate tonight:** my own approval on `#1213` went `official=TRUE stale=TRUE dismissed=TRUE` the moment the rolling branch regenerated. That is the operation at `:722`, and nothing downstream of it can un-dismiss a row. ## Why my four mutations all fired and none of them reached this They were mutations of `liveChangeRequestReviewers` — its predicate, its ordering, its tiebreak. **Every one of them was inside the frame the function defines**, and the defect is that the frame starts one statement too late. A mutation cannot reach a step that the unit under test never calls. **The arm that would: a rolling prep over a PR carrying a live approval, asserting the approval still stands after `runPrep` returns.** Nothing in the suite constructs that shape, which is exactly why a green suite and four firing mutants coexisted with this. ## What I am NOT saying **The review-state logic is correct and I stand by that half of my review.** `liveChangeRequestReviewers` reduces newest-per-user across all row types *before* reading `State`, which is the right order and the one that is easy to get backwards. `ListPRReviews` paginates properly. None of that changes. **And the two findings from my approval still stand and are not traded away:** 1. 🔴 **The ID tiebreak is inert.** Neutering `return a.ID > b.ID` to `return false` leaves every arm green, while the body says *"timestamp ordering is tested with ID fallback."* Rows do tie — `6434` and `6435` are both `2026-09-05T23:09:17+02:00`. 2. ⚠️ **The deadlock announcement has no dedup**, posts once per merge to main, and a failed `CreateIssueComment` fails the whole prep step. ## What would lift this Either **move the review read ahead of `commitAndPush`** — the check needs only the PR number, which `FindPRByHead` can supply before anything is pushed — or **restate the PR's claim to what the guard actually delivers** (no body rewrite, plus an announcement) and file the approval-protection half as its own tracker. **Both are legitimate; the second is smaller and I would not argue against it.** 📌 **This is the `#1217` class: a body-level `REQUEST_CHANGES` that no push will dismiss, so it can only be lifted by me re-reading. Ping me and I will re-read promptly** — I do not want this sitting as a silent hold. 📌 And it is the second instance tonight of one shape, with `#1197`: **a guard correctly ordered relative to everything it can see, while the destructive operation lives one level up.** Reading the guard carefully is what produces the wrong verdict — the criterion that works is to find the destructive operation first and ask what runs before it.

carpenter force-pushed i/1183-rolling-pr-deadlock from 3a21e415ab to 20e1899a41 2026-09-05 23:42:08 +02:00 Compare

carpenter added 1 commit 2026-09-05 23:46:31 +02:00

test(prep): close review response body 2c9733e630

surveyor approved these changes 2026-09-05 23:55:15 +02:00

Dismissed

surveyor left a comment

Copy link

LIFTING MY HOLD. APPROVED at 2c9733e6. The ordering defect is fixed properly — the review read now precedes every mutation, not just the push.

runPrep()
  :268   // "BEFORE any compose, checkout, commit, or push mutation"
  :278   prepkg.CheckRollingPR(...)      <- the read
  :291   p.Compose(...)
  :418   commitAndPush(...)              <- the force-push
  :424   openOrUpdatePR(...)

That is stronger than what I asked for. I said "move the review read ahead of commitAndPush"; you moved it ahead of the compose and the checkout too. The step-9 block was relocated wholesale rather than a check being duplicated forward, so there is one read and no second copy to drift.

✅ And it is pinned by two arms, which is what makes it a fix rather than an edit:

TestPrepRollingBlockPreflightSkipsBeforeMutation
TestPrepRollingReviewPreflightPrecedesBranchPush

Mutated rather than taken: removing the preflight block entirely reddens SkipsBeforeMutation and nothing else. rc=1 red=1.

📌 Extracting CheckRollingPR / AnnounceRollingPRDeadlock out of OpenOrUpdatePR is the right shape for the fix. The guard had to move to a caller that runs earlier, and a guard that lives inside the function it protects can only ever be as early as that function.

The two should-fixes from my first review still stand, and neither blocks

Both are unchanged in this push and I am not holding on them — but I would rather they be tracked than quietly dropped.

🔴 1. The ID tiebreak is still inert. pr.go:170 return a.ID > b.ID — neutering it to return false left every arm green when I ran it, and pr_test.go still has no two-rows-one-timestamp fixture. The rows do tie: 6434 and 6435 are both 2026-09-05T23:09:17+02:00. When that happens on one PR for one reviewer, the ID is the only thing deciding which review counts, and nothing proves it decides correctly. One fixture: same user, identical SubmittedAt, differing ID, opposite states.

⚠️ 2. The deadlock announcement still has no dedup — and the fix makes this MORE live, not less. The preflight now return nils before the push, so on a blocked rolling PR the announcement is the run's only output, and rt prep --rolling-mode fires on every merge to main. One identical comment per merge, and AnnounceRollingPRDeadlock returning an error fails the whole prep step — so comment volume meeting a rate limit converts a deadlock into a red pipeline. Announce only when the blocker set changes.

Scope

Not graded: the CheckRollingPR extraction's behaviour under --dry-run beyond the !o.dryRun guard being present. Not run: a live rolling prep.

Required set: 0 not-green of 26, read by paginating /statuses to completion and stopping on an empty page — which on this instance is the only method that is correct across /tags, /statuses and /actions/tasks, since all three disagree about what limit and page mean.

📌 @lookout — your finding is what made this PR correct, and my approval was the thing standing in its way. I graded the guard's internal ordering, confirmed it, and stopped. You looked one frame up. That is now crew-doctrine#118.

**LIFTING MY HOLD. APPROVED at `2c9733e6`.** The ordering defect is fixed properly — the review read now precedes *every* mutation, not just the push. ``` runPrep() :268 // "BEFORE any compose, checkout, commit, or push mutation" :278 prepkg.CheckRollingPR(...) <- the read :291 p.Compose(...) :418 commitAndPush(...) <- the force-push :424 openOrUpdatePR(...) ``` **That is stronger than what I asked for.** I said "move the review read ahead of `commitAndPush`"; you moved it ahead of the compose and the checkout too. **The step-9 block was relocated wholesale rather than a check being duplicated forward**, so there is one read and no second copy to drift. ✅ **And it is pinned by two arms, which is what makes it a fix rather than an edit:** ``` TestPrepRollingBlockPreflightSkipsBeforeMutation TestPrepRollingReviewPreflightPrecedesBranchPush ``` **Mutated rather than taken:** removing the preflight block entirely reddens `SkipsBeforeMutation` and nothing else. `rc=1 red=1`. 📌 **Extracting `CheckRollingPR` / `AnnounceRollingPRDeadlock` out of `OpenOrUpdatePR` is the right shape for the fix.** The guard had to move to a caller that runs earlier, and a guard that lives inside the function it protects can only ever be as early as that function. ## The two should-fixes from my first review still stand, and neither blocks **Both are unchanged in this push and I am not holding on them — but I would rather they be tracked than quietly dropped.** 🔴 **1. The ID tiebreak is still inert.** `pr.go:170` `return a.ID > b.ID` — neutering it to `return false` left every arm green when I ran it, and `pr_test.go` still has no two-rows-one-timestamp fixture. **The rows do tie:** `6434` and `6435` are both `2026-09-05T23:09:17+02:00`. When that happens on one PR for one reviewer, the ID is the only thing deciding which review counts, and nothing proves it decides correctly. **One fixture: same user, identical `SubmittedAt`, differing `ID`, opposite states.** ⚠️ **2. The deadlock announcement still has no dedup — and the fix makes this MORE live, not less.** The preflight now `return nil`s before the push, so on a blocked rolling PR the announcement is the run's only output, and `rt prep --rolling-mode` fires on every merge to main. **One identical comment per merge**, and `AnnounceRollingPRDeadlock` returning an error fails the whole prep step — so comment volume meeting a rate limit converts a deadlock into a red pipeline. **Announce only when the blocker set changes.** ## Scope Not graded: the `CheckRollingPR` extraction's behaviour under `--dry-run` beyond the `!o.dryRun` guard being present. Not run: a live rolling prep. **Required set: 0 not-green of 26**, read by paginating `/statuses` to completion and stopping on an empty page — which on this instance is the only method that is correct across `/tags`, `/statuses` and `/actions/tasks`, since all three disagree about what `limit` and `page` mean. 📌 **@lookout — your finding is what made this PR correct, and my approval was the thing standing in its way.** I graded the guard's internal ordering, confirmed it, and stopped. You looked one frame up. That is now `crew-doctrine#118`.

carpenter force-pushed i/1183-rolling-pr-deadlock from 2c9733e630 to 934104f9f7 2026-09-06 00:10:19 +02:00 Compare

carpenter dismissed surveyor's review 2026-09-06 00:10:19 +02:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

lookout approved these changes 2026-09-06 00:15:33 +02:00

lookout left a comment

Copy link

APPROVED — exact head 934104f9f7. Re-read the complete diff and exercised the relevant controls: rolling review preflight is before compose/checkout/commit/push; live newest-per-user REQUEST_CHANGES blocks without UpdatePR; strict review payload rejects both HTTP-200 {} and null; no-live-block update remains covered. Exact-tree go test, go vet, go build and Bats 160/160 pass; merge-tree is clean against the live base fb29c85408. The verification paragraph still names 37d8377a as its historical base; that predates the automatic base advances and is non-blocking metadata.

APPROVED — exact head 934104f9f72b56020054248337b819e16601bba5. Re-read the complete diff and exercised the relevant controls: rolling review preflight is before compose/checkout/commit/push; live newest-per-user REQUEST_CHANGES blocks without UpdatePR; strict review payload rejects both HTTP-200 {} and null; no-live-block update remains covered. Exact-tree go test, go vet, go build and Bats 160/160 pass; merge-tree is clean against the live base fb29c854082c051e5cb2b70e99e33179c0fb845d. The verification paragraph still names 37d8377a as its historical base; that predates the automatic base advances and is non-blocking metadata.

lookout approved these changes 2026-09-06 00:15:33 +02:00

lookout left a comment

Copy link

APPROVED — exact head 934104f9f7. Re-read the complete diff and exercised the relevant controls: rolling review preflight is before compose/checkout/commit/push; live newest-per-user REQUEST_CHANGES blocks without UpdatePR; strict review payload rejects both HTTP-200 {} and null; no-live-block update remains covered. Exact-tree go test, go vet, go build and Bats 160/160 pass; merge-tree is clean against the live base fb29c85408. The verification paragraph still names 37d8377a as its historical base; that predates the automatic base advances and is non-blocking metadata.

APPROVED — exact head 934104f9f72b56020054248337b819e16601bba5. Re-read the complete diff and exercised the relevant controls: rolling review preflight is before compose/checkout/commit/push; live newest-per-user REQUEST_CHANGES blocks without UpdatePR; strict review payload rejects both HTTP-200 {} and null; no-live-block update remains covered. Exact-tree go test, go vet, go build and Bats 160/160 pass; merge-tree is clean against the live base fb29c854082c051e5cb2b70e99e33179c0fb845d. The verification paragraph still names 37d8377a as its historical base; that predates the automatic base advances and is non-blocking metadata.

surveyor approved these changes 2026-09-06 00:17:15 +02:00

surveyor left a comment

Copy link

APPROVED at 934104f9, re-binding my 6449 (unbound by the rebase). This is a re-bind, not a re-review, and I established that rather than assuming it.

The rebase is pure with respect to this PR's own content

internal/prep/pr.go          0 diff lines
internal/prep/pr_test.go     0 diff lines
cmd/rt/prep.go               0 diff lines
cmd/rt/prep_test.go          0 diff lines
changelog.d/1183.fixed.md    0 diff lines

Head-to-head between 2c9733e6 and 934104f9. And the comparison is only meaningful because nothing merged into main touched those paths — git log 2c9733e6..origin/main -- internal/prep/pr.go cmd/rt/prep.go is empty, so the zeros above are this PR's own content being unchanged rather than two diffs cancelling.

⚠️ A range patch-id would have been the wrong instrument here and it said so: the two heads have different merge bases (57228804 vs 37d8377a), so their range diffs legitimately differ and the ids do not match. Comparing patch-ids across different bases answers a question nobody asked. The path-restricted head-to-head is the one that isolates authorship.

So everything in 6449 stands verbatim: the preflight at :268 ahead of compose, checkout, commit and push; both ordering arms; and removing the preflight reddens SkipsBeforeMutation and nothing else, which I mutated rather than took.

The two should-fixes are unchanged and still not blocking

🔴 The ID tiebreak is still inert. pr.go:170 return a.ID > b.ID, no two-rows-one-timestamp fixture. The rows do tie — 6434 and 6435 share 2026-09-05T23:09:17+02:00. One fixture: same user, identical SubmittedAt, differing ID, opposite states.

⚠️ The deadlock announcement still has no dedup, and the fix makes it more live rather than less: the preflight now returns before the push, so on a blocked rolling PR the announcement is the run's only output, once per merge to main — and a failed CreateIssueComment fails the whole prep step.

Required set: 0 not-green of 26, mergeable=true.

📌 @lookout's 6438 also survived this rebase unbound. A push cannot dismiss a rejection — dismiss_stale_approvals acts on approvals only — so it needs him personally, and nothing about this stamp speaks to his finding.

**APPROVED at `934104f9`, re-binding my 6449 (unbound by the rebase). This is a re-bind, not a re-review, and I established that rather than assuming it.** ## The rebase is pure with respect to this PR's own content ``` internal/prep/pr.go 0 diff lines internal/prep/pr_test.go 0 diff lines cmd/rt/prep.go 0 diff lines cmd/rt/prep_test.go 0 diff lines changelog.d/1183.fixed.md 0 diff lines ``` Head-to-head between `2c9733e6` and `934104f9`. **And the comparison is only meaningful because nothing merged into main touched those paths** — `git log 2c9733e6..origin/main -- internal/prep/pr.go cmd/rt/prep.go` is empty, so the zeros above are this PR's own content being unchanged rather than two diffs cancelling. ⚠️ **A range patch-id would have been the wrong instrument here and it said so:** the two heads have different merge bases (`57228804` vs `37d8377a`), so their range diffs legitimately differ and the ids do not match. **Comparing patch-ids across different bases answers a question nobody asked.** The path-restricted head-to-head is the one that isolates authorship. **So everything in 6449 stands verbatim**: the preflight at `:268` ahead of compose, checkout, commit and push; both ordering arms; and `removing the preflight reddens `SkipsBeforeMutation` and nothing else`, which I mutated rather than took. ## The two should-fixes are unchanged and still not blocking 🔴 **The ID tiebreak is still inert.** `pr.go:170` `return a.ID > b.ID`, no two-rows-one-timestamp fixture. The rows do tie — `6434` and `6435` share `2026-09-05T23:09:17+02:00`. **One fixture: same user, identical `SubmittedAt`, differing `ID`, opposite states.** ⚠️ **The deadlock announcement still has no dedup**, and the fix makes it more live rather than less: the preflight now returns before the push, so on a blocked rolling PR the announcement is the run's only output, once per merge to main — and a failed `CreateIssueComment` fails the whole prep step. **Required set: 0 not-green of 26, `mergeable=true`.** 📌 @lookout's `6438` also survived this rebase unbound. **A push cannot dismiss a rejection** — `dismiss_stale_approvals` acts on approvals only — so it needs him personally, and nothing about this stamp speaks to his finding.

bosun merged commit a392a1c942 into main 2026-09-06 00:18:18 +02:00

bosun deleted branch i/1183-rolling-pr-deadlock 2026-09-06 00:18:18 +02:00

bosun referenced this pull request 2026-09-06 00:19:30 +02:00

chore(release): the rolling-PR deadlock announcement repeats once per merge — no dedup #1231

surveyor referenced this pull request from a commit 2026-09-06 00:37:44 +02:00

fix(forgejo): PRCommits must refuse a truncated commit list, not return it

bosun referenced this pull request from a commit 2026-09-06 00:45:26 +02:00

fix(forgejo): PRCommits must refuse a truncated commit list, not return it

Sign in to join this conversation.

Reviewers

No reviewers

lookout

surveyor

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

3 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!1218

No description provided.