Reviewing Others' Code
You are reviewing a teammate's work, not rewriting it. Goal: catch what matters, help it ship
safely, and help the author grow — without nitpicking them to death or rubber-stamping.
Read for context first
- What problem does this PR solve? Read the description + linked issue. Review against the
author's INTENT, not your preferred implementation.
- Skim the whole diff before commenting on any line — understand the shape before the details.
- Pull and run it if the change is non-trivial. Observed behavior beats read behavior.
Prioritize — comment in this order, and LABEL severity
- Correctness — bugs, race conditions, edge/empty/error cases, off-by-ones, wrong logic.
- Security — injection, authz/authn, secrets in code, unsafe deserialization, input validation.
- Design / architecture — wrong abstraction, leaky boundaries, decisions that are hard to
reverse, missing tests for the risky part. This is where staff review adds the most value.
- Maintainability — clarity, naming, duplication, complexity that will bite in 6 months.
- Style / nits — last, and only if a linter doesn't already own it.
Label every comment [blocking], [consider], or [nit]. The author must never have to
guess what must change versus what's optional.
How to write each comment
- Specific: file:line, the concrete problem, and a suggested direction — not just "this is wrong."
- Ask when you might lack context: "what happens if
x is null here?" beats a wrong decree.
- Explain the why — that's the mentoring; a reason teaches, a demand doesn't.
- Call out genuinely good choices. Review isn't only fault-finding.
Calibrate to author and change
- A hotfix and a greenfield feature deserve different bars — don't gold-plate an urgent fix.
- Junior author → more teaching + links; senior author → trust more, focus on the hard parts.
- Team conventions and the linter win over your personal style. Don't dress up taste as correctness.
Close with a clear verdict
Approve / Approve with nits / Request changes, plus a 2–3 line summary: what's strong, what's
blocking, what's optional. Respect the author's time and their ownership of the code.
1---2name: reviewing-others-code3description: Use when reviewing someone else's code or pull request (you are the reviewer). Staff-level review discipline — prioritize by severity, separate blocking from non-blocking, review design not just lines, and give kind, specific, actionable feedback that also mentors. Distinct from auto-reviewing your own generated code.4---56# Reviewing Others' Code78You are reviewing a teammate's work, not rewriting it. Goal: catch what matters, help it ship9safely, and help the author grow — without nitpicking them to death or rubber-stamping.1011## Read for context first12- What problem does this PR solve? Read the description + linked issue. Review against the13 author's INTENT, not your preferred implementation.14- Skim the whole diff before commenting on any line — understand the shape before the details.15- Pull and run it if the change is non-trivial. Observed behavior beats read behavior.1617## Prioritize — comment in this order, and LABEL severity181. **Correctness** — bugs, race conditions, edge/empty/error cases, off-by-ones, wrong logic.192. **Security** — injection, authz/authn, secrets in code, unsafe deserialization, input validation.203. **Design / architecture** — wrong abstraction, leaky boundaries, decisions that are hard to21 reverse, missing tests for the risky part. This is where staff review adds the most value.224. **Maintainability** — clarity, naming, duplication, complexity that will bite in 6 months.235. **Style / nits** — last, and only if a linter doesn't already own it.2425Label every comment **[blocking]**, **[consider]**, or **[nit]**. The author must never have to26guess what *must* change versus what's optional.2728## How to write each comment29- Specific: file:line, the concrete problem, and a suggested direction — not just "this is wrong."30- Ask when you might lack context: "what happens if `x` is null here?" beats a wrong decree.31- Explain the *why* — that's the mentoring; a reason teaches, a demand doesn't.32- Call out genuinely good choices. Review isn't only fault-finding.3334## Calibrate to author and change35- A hotfix and a greenfield feature deserve different bars — don't gold-plate an urgent fix.36- Junior author → more teaching + links; senior author → trust more, focus on the hard parts.37- Team conventions and the linter win over your personal style. Don't dress up taste as correctness.3839## Close with a clear verdict40**Approve / Approve with nits / Request changes**, plus a 2–3 line summary: what's strong, what's41blocking, what's optional. Respect the author's time and their ownership of the code.
stevenfackley