docs: the PR review gate — reviewers don't wait on CI, operators know what auto-merge commits them to
Prompted by hyperhive/hyperhive#3050: a reviewer held a formal approve/request-changes verdict for CI to go green, which delayed a merge that was only waiting on the review. CI, review approval, and auto-merge arming are three independent gate conditions — a reviewer is only responsible for the second one. Documents the asymmetry that follows from auto-merge sometimes being armed before a review lands: an unnecessary request-changes costs a round-trip, but an approve given on lingering doubt can ship immediately with no further human check. Default to holding when unsure.
This commit is contained in:
parent
0a14055a33
commit
314aa77ee8
2 changed files with 87 additions and 0 deletions
|
|
@ -231,6 +231,9 @@ read them à la carte.
|
|||
- **"How do I export Claude Code metrics (tokens, cost, tool calls) to
|
||||
a Prometheus/Grafana collector? What OTEL options are available?"** →
|
||||
[`docs/observability.md`](docs/observability.md).
|
||||
- **"What does a PR review verdict actually gate? Should a reviewer
|
||||
wait for CI before approving? What does arming auto-merge commit an
|
||||
operator to?"** → [`docs/pr-review-gate.md`](docs/pr-review-gate.md).
|
||||
|
||||
## Conventions & process
|
||||
|
||||
|
|
|
|||
84
docs/pr-review-gate.md
Normal file
84
docs/pr-review-gate.md
Normal file
|
|
@ -0,0 +1,84 @@
|
|||
# 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).
|
||||
|
||||
## The gate has three independent parts
|
||||
|
||||
A PR merges when **all** of:
|
||||
|
||||
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).
|
||||
|
||||
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.
|
||||
|
||||
## Reviewers: submit the verdict, don't gate on CI
|
||||
|
||||
Submit `hive-forge pr-reviews <pr> --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.
|
||||
|
||||
This matters concretely because of how auto-merge interacts with
|
||||
review timing:
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
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 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.
|
||||
Loading…
Reference in a new issue