|
| 1 | +# Pull Request Review Playbook |
| 2 | + |
| 3 | +Standard process for reviewing and merging community pull requests. |
| 4 | + |
| 5 | +## Steps |
| 6 | + |
| 7 | +### 1. Triage (30 seconds) |
| 8 | + |
| 9 | +- Read the PR title, body, and file list |
| 10 | +- Check the author: first-time contributor? returning contributor? |
| 11 | +- Check PR size: files changed, additions/deletions |
| 12 | +- Check if it references an open issue (`Closes #NNN`) |
| 13 | + |
| 14 | +### 2. Code Review (2-5 minutes) |
| 15 | + |
| 16 | +Read the diff. Check for: |
| 17 | + |
| 18 | +- **Correctness**: Does the code do what the PR says it does? |
| 19 | +- **Tests**: Are there tests? Do they cover the main paths? |
| 20 | +- **Style**: Follows project conventions? (frozen dataclasses, Google docstrings, no dict soup) |
| 21 | +- **Security**: No hardcoded secrets, no command injection, no path traversal |
| 22 | +- **Scope**: Does it stay within the issue scope? No unrelated changes? |
| 23 | +- **Breaking changes**: Does it change any public API signatures? |
| 24 | + |
| 25 | +### 3. Approve with Thank-You Comment |
| 26 | + |
| 27 | +Write a review comment that: |
| 28 | + |
| 29 | +1. **Thanks the contributor by name** (`@username`) |
| 30 | +2. **Summarizes what you liked** (specific, not generic — mention the design decisions) |
| 31 | +3. **Notes any non-blocking observations** (prefix with "Minor notes (non-blocking):") |
| 32 | +4. **References the issue** if applicable (`Closes #NNN`) |
| 33 | + |
| 34 | +Template: |
| 35 | +``` |
| 36 | +[Specific praise about the implementation approach] |
| 37 | +
|
| 38 | +[1-2 sentences about what the code does well] |
| 39 | +
|
| 40 | +[Optional: Minor notes (non-blocking): |
| 41 | +- observation 1 |
| 42 | +- observation 2] |
| 43 | +
|
| 44 | +Thanks @username! |
| 45 | +``` |
| 46 | + |
| 47 | +### 4. Merge |
| 48 | + |
| 49 | +- Use **merge commit** (not squash) to preserve contributor's commit history |
| 50 | +- Use `--admin` flag if CI is slow or has pre-existing failures unrelated to the PR |
| 51 | + |
| 52 | +### 5. Post-Merge |
| 53 | + |
| 54 | +- **Close linked issues** with comment: "Implemented via PR #NNN. Merged to main." |
| 55 | +- **Update CONTRIBUTORS.md**: Add the contributor with their PRs listed |
| 56 | +- **Pull latest** to local: `git pull --rebase origin main` |
| 57 | + |
| 58 | +## Quality Bar |
| 59 | + |
| 60 | +### Auto-approve (merge immediately) |
| 61 | + |
| 62 | +- Bug fixes with tests |
| 63 | +- Documentation improvements |
| 64 | +- New modules with tests that don't modify existing code |
| 65 | +- CI/config improvements |
| 66 | + |
| 67 | +### Request changes |
| 68 | + |
| 69 | +- No tests for new functionality |
| 70 | +- Modifies existing public APIs without backward compat |
| 71 | +- Security concerns (injection, path traversal, hardcoded creds) |
| 72 | +- Scope creep (PR does more than the issue asks) |
| 73 | + |
| 74 | +### Close without merging |
| 75 | + |
| 76 | +- Spam / AI-generated garbage with no real value |
| 77 | +- Duplicate of existing functionality with no improvement |
| 78 | +- Fundamentally wrong approach that can't be fixed with review feedback |
| 79 | + |
| 80 | +## Anti-Patterns |
| 81 | + |
| 82 | +- Don't leave PRs open for days without response — review within 24 hours |
| 83 | +- Don't ask for trivial style changes that ruff/pyright would catch |
| 84 | +- Don't block on SonarCloud quality gate failures caused by pre-existing issues |
| 85 | +- Don't forget to update CONTRIBUTORS.md — recognition matters |
| 86 | + |
| 87 | +## Commands Cheat Sheet |
| 88 | + |
| 89 | +```bash |
| 90 | +# List open PRs |
| 91 | +gh pr list --repo chernistry/bernstein --state open |
| 92 | + |
| 93 | +# View PR details |
| 94 | +gh pr view NNN --repo chernistry/bernstein --json title,body,files,additions,deletions |
| 95 | + |
| 96 | +# View diff |
| 97 | +gh pr diff NNN --repo chernistry/bernstein |
| 98 | + |
| 99 | +# Approve with comment |
| 100 | +gh pr review NNN --repo chernistry/bernstein --approve --body "Message" |
| 101 | + |
| 102 | +# Merge |
| 103 | +gh pr merge NNN --repo chernistry/bernstein --merge --admin |
| 104 | + |
| 105 | +# Close issue |
| 106 | +gh issue close NNN --repo chernistry/bernstein --comment "Implemented via PR #NNN." |
| 107 | +``` |
0 commit comments