Non-blocking nits cost a full re-review cycle under dismiss_stale_reviews_on_push

[mitch-mitchel]

What

Approving a PR with non-blocking nits, then having the author push the fix, clears the approval. dismiss_stale_reviews_on_push treats every push the same, so the PR returns to REVIEW_REQUIRED and needs a second round for a change the reviewer already signed off on in advance.

Pick how we handle it. Options, not exhaustive:

• Reviewers withhold approval until nits are resolved — no more "approve with nits"
• Reviewers leave nits as a COMMENT review and approve after the fix push
• Batch nits as GitHub suggested changes so the author applies them in one push before any approval
• Relax the ruleset: keep require_last_push_approval, drop dismiss_stale_reviews_on_push

Why

The rule is doing its job. It closed the approve-then-push hole in #725 and has already fired twice (#763, #754). The cost lands hardest on the smallest changes: a one-line nit fix buys a full re-review cycle, which pushes reviewers toward either not raising nits at all or rubber-stamping the second pass. Neither is what we want. We need the security property without taxing the good-faith path.

Definition of done

• Decision recorded, with the ruleset either changed or deliberately left as-is
• Convention written down (CONTRIBUTING.md or docs/wiki/definition-of-done.md) so reviewers know when to approve
• Harden the main ruleset review gate against post-approval pushes #725 closed or updated to reflect the outcome

[mitch-mitchel]

Unchecking dismiss_stale_reviews_on_push (option 4) is probably a dead end for us.

GitHub frames require_last_push_approval as the intended compromise: "a compromise that avoids the need to dismiss all stale reviews… If you are concerned about pull requests being hijacked (where unapproved content is added to approved pull requests), it is safer to dismiss stale reviews."

But that compromise pays off in repos requiring multiple approvals — dismissal kills all N, last-push-approval costs one. We require one. And the docs say prior reviewers "can reapprove after the most recent push to meet this requirement," which reads as an approval predating the push not satisfying it.

If so, both settings produce the same outcome here: the author pushes a nit fix, one fresh approval is needed either way. Dropping dismissal buys no flow and gives back the hijack protection #725 added.

That points at options 1-3 — a process fix, not a ruleset change.

Worth confirming empirically before deciding: on a low-stakes PR, approve, have the author push a trivial change, and see whether merge unblocks. That settles whether a pre-push approval counts.