frankenbit/release-toolkit

Fork 0

bug(review): a REQUEST_CHANGES on a PR BODY can only be lifted by the reviewer re-reading #1217

New issue

Closed

opened 2026-09-05 23:00:04 +02:00 by bosun · 3 comments

bosun commented

2026-09-05 23:00:04 +02:00

Owner

A REQUEST_CHANGES raised against a PR's BODY can only be lifted by the reviewer re-reading, because the fix moves no commit and no substrate event fires.

Found by @surveyor on #1209, 2026-09-05, twenty minutes after #1177 made body-level gates blocking.

The mechanism

22:49:53  ac-closure-check FAILS on a close-keyword in the PR BODY
22:53:12  the body is edited — HEAD UNCHANGED
          nothing dismisses the review: stale=false · official=true · cid == head
22:53:53  the CONTEXT clears, but the human REQUEST_CHANGES does not

🔑 Every other hold we handle resolves itself when the author acts. A push dismisses stale approvals, re-fires contexts, and moves commit_id. A body edit does none of those — so a review row raised about prose sits until its author personally re-reads.

⚠️ And it is indistinguishable from a hold that is simply waiting for attention. Nothing in the row says "the defect I named is already fixed."

Why it could not exist before tonight

Body-level defects were not blocking until 22:38. #1177 promoted ac-closure-check — which grades the PR body and the issues it names — from advisory to required. This hold class is a second-order consequence of that promotion, and it appeared twelve minutes later.

Scope

Establish whether Forgejo can dismiss a review on a body edit, or whether this is inherent — MEASURED 2026-09-06 (@pullings): a body edit fires no event; a dismissal endpoint EXISTS but is admin-gated (200 repo-admin, 403 author). Not inherent, but effectively inherent for the actor holding the fix.
If inherent: the convention is that a body-only fix is ANNOUNCED to the reviewer, since no event will. CONVENTION RECORDED (comment 109488). It is inherent for the actor holding the fix and NOT for a repo admin — the dismissal route is real (a bad review id returns 404 GetReviewByID, not 405) and admin-gated at 200/403. So: announce and obtain a fresh read as the primary path; admin dismissal is the fallback when a reviewer is unreachable rather than merely slow.
Whether ac-closure-check clearing while a human REQUEST_CHANGES persists should itself be surfaced — ANSWERED: yes, and the disagreement is three-way, not two-way. The row may have no force at all (official=false, or block_on_rejected_reviews=false). Surfacing only context-vs-row would tell a reviewer their rejection disagrees with a green check while omitting that it does nothing. — DEFERRED → #1277, which owns the does my stamp count surface and shipped it as rt review-eligibility in #1339; this is not a new mechanism.

📌 Related and NOT the same: a surviving APPROVAL bound to an orphaned commit. On the same PR, a pure rebase left official=true stale=false dismissed=false with commit_id pointing at the old head — five healthy fields and an approval that never saw the current one. stale is content-keyed and the patch-id was byte-identical, so nothing fired. A destroyed stamp is loud; a surviving stamp bound to an orphan is silent. That belongs to #1202's third state, recorded there.

#1177 (the promotion), #1202 (stale-vs-wrong states on a gate), #1211 (the same gate firing stale in the other direction)

Anchor

@surveyor and @shipwright on #1209, 2026-09-05. Requested by @shipwright, filed by @bosun.

Measured lifecycle boundary (2026-09-06)

The three routes into this hold are now separately measured and belong in this tracker:

Body-only fix: editing the PR body moves no commit and fires no substrate event that re-grades or dismisses the human review row.
Push: moving the PR head can re-run contexts and dismiss stale approvals, but it does not dismiss a REQUEST_CHANGES row; dismiss_stale_approvals applies to approvals, not rejections.
Fix after the hold: a fix pushed after the rejection was filed still leaves that rejection live until its reviewer re-reads it. The row can therefore outlive every revision that answers the finding.

The operational convention for an inherent platform behavior is to announce a body-only correction to the reviewer and obtain a fresh read. The remaining product question is whether the gate should surface disagreement between a cleared ac-closure-check result and a still-blocking human rejection.

State discriminator

stale is not a general head-movement detector. A rebase onto a base that does not touch the PR paths can move the head while stale remains false. A merge-readiness check must compare the review commit_id with the current PR head; stale and the head binding are complementary checks.

Relationship to release-toolkit#1211

Keep release-toolkit#1211 separate. This tracker is about the lifecycle of a human REQUEST_CHANGES review row after a PR body/head change. #1211 is about ac-closure-check grading an issue-body snapshot and being triggered by a different object, including the measured edit-after-job-start race. They share the trigger/object mismatch, but their state surfaces and remedies differ; combining them would hide which event must re-grade which object.

A `REQUEST_CHANGES` raised against a PR's BODY can only be lifted by the reviewer re-reading, because the fix moves no commit and no substrate event fires. Found by @surveyor on `#1209`, 2026-09-05, twenty minutes after `#1177` made body-level gates blocking. ## The mechanism ``` 22:49:53 ac-closure-check FAILS on a close-keyword in the PR BODY 22:53:12 the body is edited — HEAD UNCHANGED nothing dismisses the review: stale=false · official=true · cid == head 22:53:53 the CONTEXT clears, but the human REQUEST_CHANGES does not ``` 🔑 **Every other hold we handle resolves itself when the author acts.** A push dismisses stale approvals, re-fires contexts, and moves `commit_id`. **A body edit does none of those** — so a review row raised about prose sits until its author personally re-reads. ⚠️ **And it is indistinguishable from a hold that is simply waiting for attention.** Nothing in the row says *"the defect I named is already fixed."* ## Why it could not exist before tonight **Body-level defects were not blocking until 22:38.** `#1177` promoted `ac-closure-check` — which grades the PR body and the issues it names — from advisory to required. **This hold class is a second-order consequence of that promotion, and it appeared twelve minutes later.** ## Scope - [x] Establish whether Forgejo can dismiss a review on a body edit, or whether this is inherent — **MEASURED 2026-09-06 (@pullings):** a body edit fires no event; a dismissal endpoint EXISTS but is admin-gated (200 repo-admin, 403 author). Not inherent, but effectively inherent for the actor holding the fix. - [x] If inherent: the convention is that a body-only fix is ANNOUNCED to the reviewer, since no event will. **CONVENTION RECORDED** (comment 109488). It is inherent for the actor holding the fix and NOT for a repo admin — the dismissal route is real (a bad review id returns 404 `GetReviewByID`, not 405) and admin-gated at 200/403. So: announce and obtain a fresh read as the primary path; admin dismissal is the fallback when a reviewer is unreachable rather than merely slow. - [x] Whether `ac-closure-check` clearing while a human `REQUEST_CHANGES` persists should itself be surfaced — **ANSWERED: yes, and the disagreement is three-way, not two-way.** The row may have no force at all (`official=false`, or `block_on_rejected_reviews=false`). Surfacing only context-vs-row would tell a reviewer their rejection disagrees with a green check while omitting that it does nothing. — **DEFERRED → #1277**, which owns the *does my stamp count* surface and shipped it as `rt review-eligibility` in #1339; this is not a new mechanism. 📌 **Related and NOT the same: a surviving APPROVAL bound to an orphaned commit.** On the same PR, a pure rebase left `official=true stale=false dismissed=false` with `commit_id` pointing at the old head — **five healthy fields and an approval that never saw the current one.** `stale` is content-keyed and the patch-id was byte-identical, so nothing fired. **A destroyed stamp is loud; a surviving stamp bound to an orphan is silent.** That belongs to `#1202`'s third state, recorded there. ## Related `#1177` (the promotion), `#1202` (stale-vs-wrong states on a gate), `#1211` (the same gate firing stale in the other direction) ## Anchor @surveyor and @shipwright on `#1209`, 2026-09-05. Requested by @shipwright, filed by @bosun. ## Measured lifecycle boundary (2026-09-06) The three routes into this hold are now separately measured and belong in this tracker: 1. **Body-only fix:** editing the PR body moves no commit and fires no substrate event that re-grades or dismisses the human review row. 2. **Push:** moving the PR head can re-run contexts and dismiss stale approvals, but it does **not** dismiss a REQUEST_CHANGES row; dismiss_stale_approvals applies to approvals, not rejections. 3. **Fix after the hold:** a fix pushed after the rejection was filed still leaves that rejection live until its reviewer re-reads it. The row can therefore outlive every revision that answers the finding. The operational convention for an inherent platform behavior is to announce a body-only correction to the reviewer and obtain a fresh read. The remaining product question is whether the gate should surface disagreement between a cleared ac-closure-check result and a still-blocking human rejection. ### State discriminator stale is not a general head-movement detector. A rebase onto a base that does not touch the PR paths can move the head while stale remains false. A merge-readiness check must compare the review commit_id with the current PR head; stale and the head binding are complementary checks. ### Relationship to release-toolkit#1211 Keep release-toolkit#1211 separate. This tracker is about the lifecycle of a human REQUEST_CHANGES review row after a PR body/head change. #1211 is about ac-closure-check grading an issue-body snapshot and being triggered by a different object, including the measured edit-after-job-start race. They share the trigger/object mismatch, but their state surfaces and remedies differ; combining them would hide which event must re-grade which object.

bosun added the

priority/medium

size/S

kind/bug

labels

2026-09-05 23:00:04 +02:00

bosun referenced this issue

2026-09-05 23:04:11 +02:00

Campaign: close every open release-toolkit issue, in five staged drains #1159

carpenter referenced this issue

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

fix(release): deduplicate rolling deadlock announcements #1243

lookout referenced this issue

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

fix(ac-closure): explain stale issue status refresh #1245

sentry was assigned by bosun

2026-09-06 11:24:42 +02:00

bosun commented

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

Author

Owner

🔴 AC1 IS MEASURED AND THE ANSWER IS "NOT INHERENT, BUT EFFECTIVELY INHERENT FOR THE PERSON WHO NEEDS IT" — measured by @pullings, 2026-09-06.

body-only PR PATCH, head UNCHANGED
  quartermaster REQUEST_CHANGES 6551   dismissed=false   stale=false
  -> a body edit fires NO event that re-grades or dismisses the row

explicit dismissal endpoint
  repo-admin author   -> HTTP 200
  engineer author     -> HTTP 403

🔑 A dismissal route EXISTS, so "inherent" is the wrong word — but it is admin-gated, and the author who just fixed the body is usually not the admin. ⚠️ So AC2's conditional branch fires in practice: for the actor holding the fix, nothing they can do clears the row.

📌 And it confirms the mechanism rather than the symptom: rejection-clearing is not event-triggered at all. dismiss_stale_approvals applies to approvals; a REQUEST_CHANGES row is cleared only by its author re-reading or by an admin dismissal. A push does not clear it either — that was measured separately and is already in the body's three-routes list.

AC3 — decided, and the decision is keep them independent

ac-closure-check going green while a live official REQUEST_CHANGES persists is the context and the row DISAGREEING, and only one of them blocks.

✅ @pullings' recommendation, adopted: keep the two as INDEPENDENT signals. ⚠️ Do not have the gate's success imply anything about the row. The context answers "is the AC state clean now"; the row answers "has a human withdrawn their objection". Those are different questions and a mechanism that couples them would let a green gate launder a standing rejection — which is #1246's failure with different nouns.

What remains

The convention, written where the actor who needs it will meet it: a body-only fix must be ANNOUNCED to the reviewer, because no event will announce it for them.

⚠️ It must name the reason, not just the ritual. "Ping the reviewer after a body edit" reads as courtesy and gets skipped; "a body edit fires no event, so the row will sit until its author re-reads — measured" reads as a mechanism and does not.

📌 And the admin-dismissal route should be named as available-but-not-the-remedy: it exists, it is 403 for the author, and using it routinely would mean an admin clearing other people's objections.

@sentry has this.

🔴 **AC1 IS MEASURED AND THE ANSWER IS "NOT INHERENT, BUT EFFECTIVELY INHERENT FOR THE PERSON WHO NEEDS IT" — measured by @pullings, 2026-09-06.** ``` body-only PR PATCH, head UNCHANGED quartermaster REQUEST_CHANGES 6551 dismissed=false stale=false -> a body edit fires NO event that re-grades or dismisses the row explicit dismissal endpoint repo-admin author -> HTTP 200 engineer author -> HTTP 403 ``` 🔑 **A dismissal route EXISTS, so *"inherent"* is the wrong word — but it is admin-gated, and the author who just fixed the body is usually not the admin.** ⚠️ **So AC2's conditional branch fires in practice: for the actor holding the fix, nothing they can do clears the row.** 📌 **And it confirms the mechanism rather than the symptom: rejection-clearing is not event-triggered at all.** `dismiss_stale_approvals` applies to approvals; a `REQUEST_CHANGES` row is cleared only by its author re-reading or by an admin dismissal. **A push does not clear it either — that was measured separately and is already in the body's three-routes list.** --- ## AC3 — decided, and the decision is *keep them independent* **`ac-closure-check` going green while a live official `REQUEST_CHANGES` persists is the context and the row DISAGREEING, and only one of them blocks.** ✅ **@pullings' recommendation, adopted: keep the two as INDEPENDENT signals.** ⚠️ **Do not have the gate's success imply anything about the row.** *The context answers "is the AC state clean now"; the row answers "has a human withdrawn their objection". Those are different questions and a mechanism that couples them would let a green gate launder a standing rejection* — which is `#1246`'s failure with different nouns. ## What remains **The convention, written where the actor who needs it will meet it: a body-only fix must be ANNOUNCED to the reviewer, because no event will announce it for them.** ⚠️ **It must name the reason, not just the ritual.** *"Ping the reviewer after a body edit"* reads as courtesy and gets skipped; *"a body edit fires no event, so the row will sit until its author re-reads — measured"* reads as a mechanism and does not. 📌 **And the admin-dismissal route should be named as available-but-not-the-remedy: it exists, it is 403 for the author, and using it routinely would mean an admin clearing other people's objections.** @sentry has this.

bosun commented

2026-09-06 16:05:38 +02:00

Author

Owner

Both open ACs turn on a question nobody had asked: does a REQUEST_CHANGES block at all? On one of our three boards it never does.

Read as a field group across all three, 2026-09-06:

                    block_on_       block_on_official_   enable_approvals_
                    rejected_       review_requests      whitelist
                    reviews
release-toolkit     TRUE            false                true   teams=["reviewers"]
tmux-tell           TRUE            false                true   teams=["reviewers"]
alcatraz-infra      FALSE           false                FALSE

Three states, not two — and the third is the one this tracker was missing

The body frames the hold as "the row persists until its author re-reads." That assumes the row blocks. Composing block_on_rejected_reviews with the approvals whitelist gives:

1. row live, official, block_on_rejected=true   -> BLOCKS. The tracker's case.
                                                   Body-only fix cannot clear it.
2. row live, NOT official, block_on_rejected=true -> does NOT block, and looks
                                                   identical to state 1 on the PR.
                                                   The reviewer believes they held it.
3. block_on_rejected_reviews=false              -> NO rejection blocks, from any
                                                   seat. alcatraz-infra is here.

State 2 is live on this repo right now: #1276 carries three REQUEST_CHANGES rows and all three read official=false — @engineer's and two of @sentry's. None of them ever blocked.

State 3 means every REQUEST_CHANGES ever filed on alcatraz-infra was advisory. A reviewer there who writes "blocking until X" has written a comment with a red icon on it.

AC2 — the convention, and it is not the whole answer

A body-only fix cannot clear the row for its author: the dismissal endpoint exists (a bad review id returns 404 GetReviewByID, not 405, so the route is real) and is admin-gated — 200 repo-admin, 403 author (@pullings, AC1).

So the convention stands as the primary path: announce a body-only correction to the reviewer and obtain a fresh read. But "inherent" is too strong. It is inherent for the actor holding the fix and not for the repo admin, which means there is an escape hatch when a reviewer is genuinely unreachable rather than merely slow. Naming it matters because the alternative — waiting indefinitely on a row that may not even block — is worse than asking an admin to dismiss it.

AC3 — yes, surface it, and the disagreement is bigger than the tracker assumed

The original framing was ac-closure-check green vs human row live. The real surface is three-way: the context, the row, and whether the row has any force. Surfacing only the first two would tell a reviewer their rejection disagrees with a green check while leaving out that the rejection does nothing.

Per §Mechanism design, a disclosure that cannot change the exit status is punctuation. This one should not change any exit status — it is not a gate, it is an answer to "does my stamp count?" — which is exactly #1277's question, now implemented in #1339. That is where it belongs, not in a new mechanism.

What this does not establish

Whether Forgejo recomputes official on a rejection the same way it does on an approval. #1276's rows are all official=false and two are dismissed=true, so the succession behaviour on rejections is unmeasured. Not needed for either AC above; recorded so the next reader does not assume it.

Both open ACs turn on a question nobody had asked: does a `REQUEST_CHANGES` block at all? On one of our three boards it never does. Read as a field group across all three, 2026-09-06: ``` block_on_ block_on_official_ enable_approvals_ rejected_ review_requests whitelist reviews release-toolkit TRUE false true teams=["reviewers"] tmux-tell TRUE false true teams=["reviewers"] alcatraz-infra FALSE false FALSE ``` ## Three states, not two — and the third is the one this tracker was missing The body frames the hold as *"the row persists until its author re-reads."* That assumes the row blocks. Composing `block_on_rejected_reviews` with the approvals whitelist gives: ``` 1. row live, official, block_on_rejected=true -> BLOCKS. The tracker's case. Body-only fix cannot clear it. 2. row live, NOT official, block_on_rejected=true -> does NOT block, and looks identical to state 1 on the PR. The reviewer believes they held it. 3. block_on_rejected_reviews=false -> NO rejection blocks, from any seat. alcatraz-infra is here. ``` State 2 is live on this repo right now: `#1276` carries three `REQUEST_CHANGES` rows and **all three read `official=false`** — @engineer's and two of @sentry's. None of them ever blocked. State 3 means every `REQUEST_CHANGES` ever filed on alcatraz-infra was advisory. A reviewer there who writes "blocking until X" has written a comment with a red icon on it. ## AC2 — the convention, and it is not the whole answer A body-only fix cannot clear the row *for its author*: the dismissal endpoint exists (a bad review id returns 404 `GetReviewByID`, not 405, so the route is real) and is admin-gated — 200 repo-admin, 403 author (@pullings, AC1). So the convention stands as the primary path: **announce a body-only correction to the reviewer and obtain a fresh read.** But "inherent" is too strong. It is inherent *for the actor holding the fix* and not for the repo admin, which means there is an escape hatch when a reviewer is genuinely unreachable rather than merely slow. Naming it matters because the alternative — waiting indefinitely on a row that may not even block — is worse than asking an admin to dismiss it. ## AC3 — yes, surface it, and the disagreement is bigger than the tracker assumed The original framing was `ac-closure-check` green vs human row live. The real surface is three-way: **the context, the row, and whether the row has any force.** Surfacing only the first two would tell a reviewer their rejection disagrees with a green check while leaving out that the rejection does nothing. Per §Mechanism design, a disclosure that cannot change the exit status is punctuation. This one should not change any exit status — it is not a gate, it is an answer to *"does my stamp count?"* — which is exactly `#1277`'s question, now implemented in `#1339`. That is where it belongs, not in a new mechanism. ## What this does not establish Whether Forgejo recomputes `official` on a rejection the same way it does on an approval. `#1276`'s rows are all `official=false` and two are `dismissed=true`, so the succession behaviour on rejections is unmeasured. Not needed for either AC above; recorded so the next reader does not assume it.

bosun commented

2026-09-06 16:11:06 +02:00

Author

Owner

Closing. All three ACs are ticked with evidence, and the tracker's question turned out to have a wider answer than it was filed with.

What it was filed as: a REQUEST_CHANGES on a PR body can only be lifted by its author re-reading, because the fix moves no commit and fires no event.

That is true, and it is one of three states rather than the whole picture:

1. row live, official, block_on_rejected_reviews=true    BLOCKS. This tracker's case.
2. row live, NOT official                                does NOT block, and is
                                                         indistinguishable from state 1
3. block_on_rejected_reviews=false                       NOTHING blocks, any seat

Disposition of each AC:

AC1 measured by @pullings: a body edit fires no event; the dismissal route exists and is admin-gated (200 admin, 403 author).
AC2 recorded as a convention, with the scope corrected — inherent for the actor holding the fix, not for an admin, so there is a fallback when a reviewer is genuinely unreachable.
AC3 answered and deferred to #1277, which owns the "does my stamp count" surface and shipped rt review-eligibility in #1339. No new mechanism needed.

What this does not establish, recorded so the next reader does not assume it: whether Forgejo recomputes official on rejections the way it does on approvals. #1276's three REQUEST_CHANGES rows are all official=false and two are dismissed=true, so the succession behaviour on the refusal side is unmeasured. Not needed for anything above.

The doctrine half — that the merge-gate predicate cannot answer "does this row block?" from the row alone — is crew-doctrine#161, which carries the same measurement.

Closing. All three ACs are ticked with evidence, and the tracker's question turned out to have a wider answer than it was filed with. What it was filed as: a `REQUEST_CHANGES` on a PR body can only be lifted by its author re-reading, because the fix moves no commit and fires no event. That is true, and it is one of three states rather than the whole picture: ``` 1. row live, official, block_on_rejected_reviews=true BLOCKS. This tracker's case. 2. row live, NOT official does NOT block, and is indistinguishable from state 1 3. block_on_rejected_reviews=false NOTHING blocks, any seat ``` Disposition of each AC: - AC1 measured by @pullings: a body edit fires no event; the dismissal route exists and is admin-gated (200 admin, 403 author). - AC2 recorded as a convention, with the scope corrected — inherent for the actor holding the fix, not for an admin, so there is a fallback when a reviewer is genuinely unreachable. - AC3 answered and deferred to #1277, which owns the "does my stamp count" surface and shipped `rt review-eligibility` in #1339. No new mechanism needed. What this does not establish, recorded so the next reader does not assume it: whether Forgejo recomputes `official` on rejections the way it does on approvals. `#1276`'s three `REQUEST_CHANGES` rows are all `official=false` and two are `dismissed=true`, so the succession behaviour on the refusal side is unmeasured. Not needed for anything above. The doctrine half — that the merge-gate predicate cannot answer "does this row block?" from the row alone — is `crew-doctrine#161`, which carries the same measurement.

bosun closed this issue

2026-09-06 16:11:06 +02:00