From 0c880bfbb386a91db099edd1e1ad85c784e7291a Mon Sep 17 00:00:00 2001 From: argus Date: Tue, 4 Aug 2026 17:23:12 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20revise=20per=20review=20=E2=80=94=20no?= =?UTF-8?q?=20specific=20example,=20gate=20is=20per-repo=20config,=20softe?= =?UTF-8?q?n=20auto-merge=20framing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the concrete incident reference; the doc should stand on its own without pointing at one PR. - Reframe the three-part gate as per-repo branch-protection settings, not universal fact — CI-required, stale-review-dismissal, and auto-merge are each configurable, not hive law. - Reframe what auto-merge signals: the arming human already judged the PR sound at a coarse level and expects only minor tweaks past that point, not zero human review ever. A reviewer's bar is substantive doubt, not any nit. --- docs/pr-review-gate.md | 121 +++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 70 deletions(-) diff --git a/docs/pr-review-gate.md b/docs/pr-review-gate.md index 46cb4105..f4ae3fad 100644 --- a/docs/pr-review-gate.md +++ b/docs/pr-review-gate.md @@ -1,84 +1,65 @@ # The PR review gate -What a review verdict means in this hive, why a reviewer shouldn't -wait on CI to submit one, and what an operator setting up a fresh -hive should know before arming auto-merge. Prompted by a live -mistake: a reviewer held a formal approve/request-changes verdict -back "until CI is green," which is not the reviewer's job to gate on -and just delayed a merge that was already waiting on the review alone -(hyperhive/hyperhive#3050). +What a review verdict means, why a reviewer shouldn't wait on CI to +submit one, and what "armed to auto-merge" actually signals about the +human review that already happened. -## The gate has three independent parts +## The gate has (up to) three parts, and they're per-repo settings -A PR merges when **all** of: +Whether a PR can merge, and what counts toward "can", is configured +per repo in its branch-protection settings — not a fact true of every +hive or every repo. The pieces a repo *can* require: -1. **CI is green** — the repo's required status checks pass on the - PR's current head commit. -2. **Every requested review is `APPROVED`, and none is stale** — - Forgejo marks a review stale the moment a new commit lands on the - branch after it; a stale approve does not count. -3. **Someone with write access has armed the PR to merge** — either a - manual merge once 1 and 2 hold, or Forgejo's auto-merge (merge the - instant the other two conditions are met, no human back in the - loop at that moment). +- **CI is green** — the repo's required status checks pass on the + PR's current head commit, if the repo requires any. +- **Requested reviews are `APPROVED`** — and whether a review is + invalidated by a later commit ("stale") is itself a setting + (Forgejo's "dismiss stale approvals" branch-protection option), not + universal behavior. +- **Someone with write access has armed the PR to merge** — a manual + merge once the required conditions hold, or Forgejo's auto-merge + (merges automatically the moment the other required conditions are + met). -These three are independent. A reviewer is only ever responsible for -part 2. Waiting on part 1 before submitting a verdict doesn't make -the review "more correct" — it just delays the gate for no reason, -since CI resolves on its own regardless of what the review says. +Where these are required, they're independent of each other. A +reviewer only ever owns the review-approval piece — CI resolves (or +doesn't) on its own regardless of what a review says, and merge-arming +is someone else's call. -## Reviewers: submit the verdict, don't gate on CI +## Reviewers: submit the verdict, don't gate it on CI Submit `hive-forge pr-reviews --approve` or `--request-changes` -as soon as you've finished checking the diff — **not** once CI -happens to be green. Mention CI's current state in the review body if -it's relevant (e.g. "approving; `nix flake check` is still running"), -but don't hold the formal verdict hostage to it. CI is not a signal a -reviewer waits on; it's a parallel, independent gate that resolves on -its own. +as soon as you've finished checking the diff — don't hold it back +waiting for CI to go green first. Mention CI's current state in the +review body if it's relevant (e.g. "approving; `nix flake check` is +still running"), but don't gate the formal verdict on it: CI isn't a +signal a reviewer waits on, it's a separate condition that resolves +independently. -This matters concretely because of how auto-merge interacts with -review timing: +## What arming auto-merge actually means -**An operator may arm auto-merge before your review lands.** If they -have, and CI is already green, your `APPROVED` review is the -*trigger* — the merge happens the instant Forgejo records your -review, with no further human check. There is no "let me look this -over once more before it actually merges" step once you've approved. +Auto-merge isn't "no human ever looked at this." Whoever arms it has +already judged the PR sound at a coarse level — the signal it sends is +roughly *"apart from maybe minor tweaks a reviewer can still catch, +I think this is fine."* That's the human-in-the-loop step, and it +already happened. No large changes are expected to surface after +that point — a reviewer's job past that point is to flag it if one +does, not to assume none ever will. -The practical consequence: **lean hard toward `request-changes` when -in doubt**, including for things that feel minor — a comment that's -slightly wrong, a claim you haven't fully verified, a doc that might -be stale. On a repo without auto-merge armed, an unnecessary -`request-changes` costs a re-review round-trip. On one where -auto-merge is armed, an `approve` you weren't fully sure of ships -immediately. That asymmetry is why the default under uncertainty is -to hold, not wave through. +The practical consequence for a reviewer: on a repo where auto-merge +may already be armed before your review lands, a plain `APPROVED` can +be the last step before the merge actually happens, with no further +review pass after yours. That's a reason to actually finish checking +before approving — not a reason to hesitate over every small thing. +Genuine, substantive doubt (a claim you haven't verified, a real +correctness question) is worth a `request-changes` or a clarifying +comment before approving; a stray style nit isn't the same category. -Corollary: don't read "submit promptly" as "submit hastily." The -fix isn't rushing the check, it's not padding the check with an -extra, unrelated wait (CI) that was never yours to hold. +## Re-review after a repo requires it -## Re-review after every push - -A stale review (Forgejo flags it in the PR's review list once a new -commit lands) does not count toward the gate. If a PR you've reviewed -gets a follow-up commit — even a "just a wording fix" or "just -trimming comments" commit — re-diff it and submit a fresh verdict -before it can merge. Don't assume a small-sounding commit message -accurately describes the diff; verify it the same way you verified -the original. - -## Operators: what arming auto-merge actually commits you to - -Setting a PR to auto-merge in Forgejo means: the moment CI is green -**and** every requested review is an approve, the PR merges — with no -further chance for a human to look at it. If you arm it *before* a -review has landed, you are trusting that reviewer's eventual -`APPROVED` completely; there's no second gate after them. - -If that's not what you want for a given PR (e.g. you want to glance -at it yourself even after the requested reviewer approves), don't arm -auto-merge — merge manually once the gate conditions are met, or add -yourself as a required reviewer too so the gate's part 2 doesn't -close on the requested reviewer's approve alone. +Where a repo dismisses stale approvals on a new commit, a review you +already gave stops counting the moment a follow-up commit lands — even +one whose message reads as trivial ("just a wording fix", "just +trimming comments"). Re-diff and re-verify before submitting a fresh +verdict; don't take a small-sounding commit message as an accurate +description of the diff.