vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

docs: revise per review — no specific example, gate is per-repo config, soften auto-merge framing

Browse source 
- 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.
This commit is contained in:
argus 2026-08-04 17:23:12 +02:00
commit 0c880bfbb3
1 changed files with 61 additions and 80 deletions
Download patch file Download diff file Expand all files Collapse all files

121
docs/pr-review-gate.md
View file

@ -1,84 +1,65 @@
# The PR review gate # The PR review gate
What a review verdict means in this hive, why a reviewer shouldn't What a review verdict means, why a reviewer shouldn't wait on CI to
wait on CI to submit one, and what an operator setting up a fresh submit one, and what "armed to auto-merge" actually signals about the
hive should know before arming auto-merge. Prompted by a live human review that already happened.
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 ## 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 - **CI is green** — the repo's required status checks pass on the
PR's current head commit. PR's current head commit, if the repo requires any.
2. **Every requested review is `APPROVED`, and none is stale** — - **Requested reviews are `APPROVED`** — and whether a review is
Forgejo marks a review stale the moment a new commit lands on the invalidated by a later commit ("stale") is itself a setting
branch after it; a stale approve does not count. (Forgejo's "dismiss stale approvals" branch-protection option), not
3. **Someone with write access has armed the PR to merge** — either a universal behavior.
manual merge once 1 and 2 hold, or Forgejo's auto-merge (merge the - **Someone with write access has armed the PR to merge** — a manual
instant the other two conditions are met, no human back in the merge once the required conditions hold, or Forgejo's auto-merge
loop at that moment). (merges automatically the moment the other required conditions are
met).
These three are independent. A reviewer is only ever responsible for Where these are required, they're independent of each other. A
part 2. Waiting on part 1 before submitting a verdict doesn't make reviewer only ever owns the review-approval piece — CI resolves (or
the review "more correct" — it just delays the gate for no reason, doesn't) on its own regardless of what a review says, and merge-arming
since CI resolves on its own regardless of what the review says. 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 <pr> --approve` or `--request-changes` Submit `hive-forge pr-reviews <pr> --approve` or `--request-changes`
as soon as you've finished checking the diff — **not** once CI as soon as you've finished checking the diff — don't hold it back
happens to be green. Mention CI's current state in the review body if waiting for CI to go green first. Mention CI's current state in the
it's relevant (e.g. "approving; `nix flake check` is still running"), review body if it's relevant (e.g. "approving; `nix flake check` is
but don't hold the formal verdict hostage to it. CI is not a signal a still running"), but don't gate the formal verdict on it: CI isn't a
reviewer waits on; it's a parallel, independent gate that resolves on signal a reviewer waits on, it's a separate condition that resolves
its own. independently.
This matters concretely because of how auto-merge interacts with ## What arming auto-merge actually means
review timing:
**An operator may arm auto-merge before your review lands.** If they Auto-merge isn't "no human ever looked at this." Whoever arms it has
have, and CI is already green, your `APPROVED` review is the already judged the PR sound at a coarse level — the signal it sends is
*trigger* — the merge happens the instant Forgejo records your roughly *"apart from maybe minor tweaks a reviewer can still catch,
review, with no further human check. There is no "let me look this I think this is fine."* That's the human-in-the-loop step, and it
over once more before it actually merges" step once you've approved. 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 The practical consequence for a reviewer: on a repo where auto-merge
in doubt**, including for things that feel minor — a comment that's may already be armed before your review lands, a plain `APPROVED` can
slightly wrong, a claim you haven't fully verified, a doc that might be the last step before the merge actually happens, with no further
be stale. On a repo without auto-merge armed, an unnecessary review pass after yours. That's a reason to actually finish checking
`request-changes` costs a re-review round-trip. On one where before approving — not a reason to hesitate over every small thing.
auto-merge is armed, an `approve` you weren't fully sure of ships Genuine, substantive doubt (a claim you haven't verified, a real
immediately. That asymmetry is why the default under uncertainty is correctness question) is worth a `request-changes` or a clarifying
to hold, not wave through. comment before approving; a stray style nit isn't the same category.
Corollary: don't read "submit promptly" as "submit hastily." The ## Re-review after a repo requires it
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 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
A stale review (Forgejo flags it in the PR's review list once a new one whose message reads as trivial ("just a wording fix", "just
commit lands) does not count toward the gate. If a PR you've reviewed trimming comments"). Re-diff and re-verify before submitting a fresh
gets a follow-up commit — even a "just a wording fix" or "just verdict; don't take a small-sounding commit message as an accurate
trimming comments" commit — re-diff it and submit a fresh verdict description of the diff.
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.