90 lines
4.2 KiB
Markdown
90 lines
4.2 KiB
Markdown
---
|
|
name: pr-review
|
|
description: Generic mechanics and conventions for reviewing a pull request - label every finding with a severity (block/note/good), don't approve with an unresolved one-line fix outstanding, check correctness/security/consistency/tests/docs, resolve any opaque reference before approving, and submit a formal approve/request-changes verdict rather than a plain comment. Use this whenever you're about to review someone else's pull request, from first read to submitting your verdict.
|
|
---
|
|
|
|
# PR Review
|
|
|
|
A review's job is to catch what the author missed, not to rewrite their
|
|
work in your own style. That framing should hold throughout.
|
|
|
|
## Before you start
|
|
|
|
1. **Read the linked issue/task** - understand *why* the change exists,
|
|
not just what it does.
|
|
2. **Check CI status first.** A failing build or test suite is an
|
|
automatic block - don't write a detailed review of code that doesn't
|
|
even pass its own checks. Unblock the build first.
|
|
3. **Pull the full source when context matters**, not just the diff -
|
|
diffs lie by omission.
|
|
|
|
## Every finding needs a severity
|
|
|
|
- **block** - must fix before merge: a bug, a security issue, broken
|
|
behavior, a failing test, a hardcoded value that will rot.
|
|
- **note** - worth fixing, author's call: an inconsistency, a missing
|
|
edge case, a stale comment, copy-paste smell.
|
|
- **good** - explicitly call out what's done well (optional, but
|
|
appreciated - a review that's only ever critical trains authors to
|
|
dread review instead of valuing it).
|
|
|
|
Mixing blocks and notes without labeling them forces the author to
|
|
guess what's actually required before merge.
|
|
|
|
## Don't approve with an outstanding one-line fix
|
|
|
|
If a finding is a trivial fix, it belongs in the PR before merge - don't
|
|
approve with it left as a dangling nit. Request changes when the fix is
|
|
that cheap. Approving with unresolved issues trains authors to treat
|
|
review comments as optional.
|
|
|
|
## What to check
|
|
|
|
- **Correctness** - does it do what the issue says? Handle edge cases
|
|
(empty input, off-by-one, concurrent access)? Any silent failure paths
|
|
(swallowed errors, unchecked returns)?
|
|
- **Security** - credentials/tokens ever logged or echoed? User-supplied
|
|
input injected into a shell command, query, or DOM? File paths
|
|
validated before use?
|
|
- **Consistency** - do new identifiers match the surrounding file's
|
|
conventions? Same error-handling pattern as neighboring code? Doc
|
|
comments in the same style and tense as existing ones?
|
|
- **Tests** - do they exercise the actual new behavior, including
|
|
failure paths, not just the happy path? Are names descriptive enough
|
|
to diagnose a failure without reading the body?
|
|
- **Documentation** - does any public interface, flag, or config option
|
|
need a doc update? Are stale docs cleaned up alongside the change?
|
|
|
|
## Opaque references
|
|
|
|
Any comment or identifier that references something by name (a ticket
|
|
number, an internal codename, a `NOTE(foo)`-style marker) is only useful
|
|
if it's actually resolvable. Search for the referenced thing before
|
|
approving; if you can't resolve it, flag it - missing context isn't a
|
|
reason to wave a change through.
|
|
|
|
## Submit a formal verdict, not just a comment
|
|
|
|
A plain comment does **not** set a PR's review state and won't
|
|
block or unblock a merge on most forges. Every review should end with
|
|
an explicit **approve** or **request-changes** verdict through
|
|
whatever formal review mechanism your forge tooling exposes - not just
|
|
prose in a comment that looks like a verdict but doesn't function as one.
|
|
|
|
## Format
|
|
|
|
Keep it tight - one finding per bullet, each labeled with its severity,
|
|
two sentences max per finding. If a finding needs more than that, it's
|
|
architectural enough to be its own comment, not a review bullet.
|
|
|
|
## Tone
|
|
|
|
Precise, not harsh. "This will panic on an empty slice" is useful. "This
|
|
is bad" is not - point at the exact spot, describe the failure mode,
|
|
suggest the fix. Don't soften a real block into a vague nit to be nice;
|
|
a watered-down block is just a block that slips into production.
|
|
|
|
If your project ships its own domain-specific review checklist (gates
|
|
that differ by author/area, project-specific tooling commands), treat
|
|
that as the canonical addition on top of this - project specifics don't
|
|
belong duplicated in here.
|