1
# PR Reviewer — Workflow
2
3
A structured procedure for reviewing a pull request the way a careful senior engineer
4
would: gather full context first, classify risk before reading line-by-line, work a
5
fixed checklist, and report findings in a consistent, severity-tagged format instead of
6
a stream-of-consciousness comment.
7
8
Inputs: `pr_ref` (required), `base_ref` (default `main`), `post_comments` (default `false`).
9
10
## Step 1 — Gather context
11
12
Do not start reading the diff yet. First collect:
13
14
1. **The diff itself.**
15
   - GitHub PR: `gh pr diff <pr_ref>` (also `gh pr view <pr_ref> --json title,body,files,additions,deletions`).
16
   - Local branch/range: `git diff <base_ref>...<pr_ref>` (three-dot = changes since merge-base).
17
2. **The PR description / linked issue.** Understand *why* this change exists before
18
   judging *how* it was made. A 400-line diff with a one-line "fixes bug" description is
19
   itself a finding (see Step 2).
20
3. **The file list with change size**, not just the diff text: `git diff --stat`. This
21
   tells you where to spend attention — a 3-line config tweak and a 300-line new module
22
   deserve very different scrutiny.
23
4. **CI status.** If checks are failing, note it up front; don't duplicate what a linter
24
   or test runner already caught.
25
5. **Blast radius.** Grep for callers/importers of any changed public function, exported
26
   type, API route, or schema. A correct-looking change can still break callers that
27
   assumed the old contract.
28
29
## Step 2 — Classify risk
30
31
Before checklist review, assign an overall risk tier. This sets how much scrutiny to
32
apply and belongs at the top of the final report.
33
34
| Tier | Criteria (any one qualifies) |
35
|------|-------------------------------|
36
| **High** | Touches auth/authz, payments, data deletion, migrations, crypto, secrets handling, or public API contracts; >400 lines changed; touches CI/deploy config |
37
| **Medium** | New business logic, non-trivial refactor, touches shared/widely-imported code, adds a new dependency |
38
| **Low** | Docs, tests-only, comments, formatting, isolated/leaf-module changes, config value tweaks with no behavior change |
39
40
Also flag **scope mismatch** here: if the PR description says "fix typo" but the diff
41
touches auth middleware, that mismatch is itself a High-severity finding — surprise
42
scope is a red flag independent of whether the code is correct.
43
44
## Step 3 — Work the checklist
45
46
Apply `rules/review-checklist.md` in order: correctness, security, tests, performance.
47
Do not skip categories because the PR "looks like" a docs change — a docs-only diff can
48
still leak an internal hostname or credential. For each checklist item that finds
49
something, record: file:line, what's wrong, why it matters, and a concrete suggested
50
fix (not just "this looks off"). If a checklist item is genuinely not applicable (e.g.
51
no SQL in a frontend-only PR), skip it silently rather than padding the report.
52
53
## Step 4 — Verify tests actually exercise the change
54
55
Don't just check "tests were added" as a checkbox. For each new/changed behavior:
56
- Find the test that covers it and confirm the assertion would actually fail if the
57
  behavior regressed (not just "the function was called").
58
- Check the diff for tests that were *weakened* to make CI pass (loosened assertions,
59
  increased timeouts, skipped/`.only`'d tests, removed edge cases).
60
- For bug fixes, confirm there's a regression test reproducing the original bug.
61
62
## Step 5 — Compose the report
63
64
Use `templates/review-comment.md`. Order findings by severity (blocker → major → minor
65
→ nit), each with file:line and a suggested fix. Lead with the risk tier from Step 2
66
and a one-paragraph summary of what the PR does and whether it should merge as-is,
67
merge with changes, or needs a design discussion first.
68
69
Do not manufacture findings to look thorough — "no issues found in this category" is a
70
valid and useful line. Nits (style, naming, minor readability) go at the bottom and
71
should be clearly optional; never block a PR on nits alone.
72
73
## Step 6 — Deliver
74
75
- If `post_comments` is `false` (default): print the report and stop. This is the safe
76
  default — do not post anything.
77
- If `post_comments` is `true`: this is a side-effectful, externally-visible action.
78
  Confirm with the user which specific comments will be posted before submitting
79
  (`gh pr comment` / `gh pr review`), and never post secrets, internal URLs, or anything
80
  copied verbatim from private context into a public PR thread.
81
82
## Stop conditions
83
84
- Diff is empty or `pr_ref` doesn't resolve → report the error, do not guess.
85
- Diff exceeds ~2000 changed lines → say so explicitly and either review in per-file
86
  batches or ask the user to scope the review (e.g. to specific files/directories)
87
  rather than silently skimming.
88
- Binary files or generated/vendored files (lockfiles, `dist/`, minified bundles) in the
89
  diff → skip them, note that they were skipped and why.
90