c41c67c949
Fourth batch of hyperhive#4042's Passive pass (see #4098/#4099/#4100 for the first three and the read-every-hit discipline this pass uses). 44 hits across pr-review-gate.md (4), conventions.md (18), and gotchas.md (22) -- highest genuine-catch rate so far, 23/44 (~52%), because this architecture/mechanism documentation has a lot of "X does Y via Z" sentences where the actor is already named parenthetically or in a nearby clause -- the single most productive rewrite shape across every batch so far. Recurring rewrite shapes this batch: - Actor already named in the same sentence, just not as the grammatical subject: "X is configured per repo in its branch-protection settings" -> "Each repo's branch-protection settings configure X" (pr-review-gate.md); "the broker" (reserved names), "rsvg-convert" (PNG rendering), "the website repo" (HTML/CSS rendering), "systemd.globalEnvironment" (D-Bus address export), and several more -- all the same shape. - Subject already established one clause or one sentence earlier, just needs continuing rather than restarting with a new passive subject: "the harness reads HIVE_TOOL_GROUPS (...). Unrecognised tokens are logged and skipped." -> "...logging and skipping unrecognised tokens" (continues "the harness"), same pattern twice more (job_queue::templates::rebuild, HIVE_CAPABILITIES resolution). - Sibling-inconsistency: a bolded lead-in bullet was the one passive sentence in an otherwise-active paragraph/table (the read_host_journal capability row sat between two "may X" rows; the HTML+CSS bullet's own tail clauses were already active voice around the one passive lead phrase). - One caught-and-fixed authoring mistake worth noting for future passes: the first attempt at the "Nix treats X as a package" rewrite landed in the wrong sentence (a similarly-worded but unrelated passage two paragraphs up) -- caught by re-reading the diff before running vale, not by vale itself (which would have shown 0 remaining hits either way, since the intended sentence's hit just wouldn't have been touched -- a silently-wrong edit vale's own count can't catch). Re-reading the actual diff, not just trusting the before/ after hit count, is what caught it. 21 of 44 left alone -- same recurring legitimate shapes as prior batches (predicate-adjective copulas, quoted/literal text, generic- actor statements, negative-capability invariants, "is tracked/rooted/ scoped at X" property-description idioms, and two more thesis- statement headings matching the "Ownership is declared, not repaired" precedent from #4100). Verified: vale docs/process before/after -- 44 -> 21 write-good.Passive hits, exactly the 23 rewritten, re-read every changed line's full surrounding context after editing (not just the vale count) to catch exactly the kind of misplaced-edit mistake described above.
65 lines
3.1 KiB
Markdown
65 lines
3.1 KiB
Markdown
# The PR review gate
|
|
|
|
What a review verdict means, why a reviewer shouldn't wait on CI to
|
|
submit one, and what "armed to automerge" actually signals about the
|
|
human review that already happened.
|
|
|
|
## The gate has (up to) three parts, and they're per-repo settings
|
|
|
|
Each repo's branch-protection settings configure whether a PR can
|
|
merge, and what counts toward "can" — not a fact true of every hive or
|
|
every repo. The pieces a repo _can_ require:
|
|
|
|
- **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 automerge
|
|
(merges automatically the moment the other required conditions are
|
|
met).
|
|
|
|
Where a repo requires these, 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 it on CI
|
|
|
|
Submit `hive-forge pr-reviews <pr> --approve` or `--request-changes`
|
|
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 (for example "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.
|
|
|
|
## What arming automerge actually means
|
|
|
|
Automerge 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 for a reviewer: on a repo where automerge
|
|
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.
|
|
|
|
## Re-review after a repo requires it
|
|
|
|
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.
|