Skip to content
Documentation

PR Authorship & Review

FieldValue
TypeSkill Resource
Source~/.copilot/skills/quality/references/pr-review.md
DescriptionNot specified

Source Content

PR Authorship & Review

This reference covers drafting PR descriptions that match the actual diff, running self-review passes, conducting effective reviews, and selecting the right reviewers.

PR Body Template

Every PR follows this structure:

## Summary
<1–3 bullets — what changed and why>


## Changes
- <bullets — concrete diff by file/area>


## Test plan
- [ ] <golden path>
- [ ] <edge case 1>
- [ ] <edge case 2>


## Screenshots (UI)
| Before | After |
|---|---|
| ... | ... |


## Risk
- <migration / breaking change / flag default / perf, or "low — code-only">


## Linked
- Closes #<issue>
- Refs <ADR/doc>

PR Title Convention

  • Under 70 characters
  • Imperative verb: fix:, feat:, refactor:, chore:, docs:, test:
  • No ticket numbers in title

Good examples:

  • fix: validate email format before sending
  • feat: add dark theme toggle to settings
  • refactor: extract payment logic into service
  • docs: clarify authentication flow

Avoid:

  • Various improvements
  • JIRA-1234: implement feature
  • Update files
  • Fix stuff

How to Author a PR

  1. Scope the diff. Run git diff <base>...HEAD --stat — files, size, surface area.

  2. Classify the change. Is it a feature, fix, refactor, chore, docs, or test?

  3. Self-review pass. Walk the diff yourself. Hunt:

    • TODO / FIXME / XXX comments
    • console.log / fmt.Println / debug prints
    • Commented-out code
    • Missing tests for new branches
    • New dependencies (license check)
    • Secret-shaped strings (API keys, tokens, passwords)
    • Dropped auth checks or permission guards

  4. Write the description. Use the template above:

    • Summary: 1–3 bullets describing what changed and why
    • Changes: bullets for each file or area touched
    • Test plan: golden path + 1–3 edge cases the reviewer should reproduce
    • Screenshots (UI only): before/after side-by-side
    • Risk: migrations (rollback plan), breaking changes (deprecation plan), feature flags (default, rollout), perf impact (benchmark), or “low — code-only”
    • Linked: closing issues and referencing ADRs/docs

  5. Pick reviewers. Union of:

    • CODEOWNERS for touched paths
    • Last 3 authors on those paths
    • Domain specialists if it touches unfamiliar areas

  6. Render and hand off.

Self-Review Checklist

Before opening the PR:

  • No TODO, FIXME, debug prints, commented-out code, or secret-shaped strings
  • New branches are tested (or documented as excluded)
  • No new dependencies without review of license and size
  • No dropped auth checks or permission guards
  • Description matches the actual diff (use git diff <base>...HEAD --stat)
  • Test plan is reproducible without follow-up questions
  • Risk section names migrations, breaking changes, flags, perf, or explicitly “low”
  • Title is conventional (imperative, under 70 chars, no ticket number)
  • Reviewers are chosen, not defaulted (names tied to paths touched)

How to Review a PR

Three-Pass Review Flow

Pass 1: Architecture — Does this belong here?

  • Is the code in the right file?
  • Does it follow the project’s module structure?
  • Are new dependencies justified?
  • Does it violate any hard rules?

Pass 2: Logic — Does it work correctly?

  • Are edge cases handled?
  • Is every branch tested?
  • Are error cases handled?
  • Does the test plan cover the happy path and failures?

Pass 3: Polish — Is it clean and maintainable?

  • Clear names for functions, variables, tests
  • No dead code, unused imports, or leftover comments
  • Lying comments that don’t match code?
  • Is duplication shared or eliminated?

Comment Hierarchy

BLOCKER: Must fix before merge.

BLOCKER: This function has no null check on the result. What happens if it's undefined?

Q: Author must answer.

Q: Why did you choose this approach instead of using the existing utility?

nit: Author may ignore (nice-to-have suggestions).

nit: This name could be clearer: `processItems` → `filterAndTransformItems`

Review Checklist

  • Description matches the diff. No drift between what changed and what they say changed.
  • Test plan is reproducible. Reviewer can run it without asking follow-ups.
  • Code is tested. New branches are covered; critical paths have golden path + failure case.
  • No flaky tests. Async is handled with waitFor or findBy*, not fixed sleeps.
  • No implementation details tested. Tests query by role/label, not by internal state.
  • Risk is named. Migrations, breaking changes, flags, perf — or explicitly “low.”
  • All blockers resolved. No outstanding BLOCKER: comments.

Triaging a Bouncing PR

If a PR is bouncing between author and reviewers:

  1. Read the description. Does it match the diff?
  2. Check the test plan. Is it reproducible? Is it actually run?
  3. Verify the diff. Are there changes not mentioned in the description?
  4. Compare against diffs. Are claims about what changed backed by code?

If drift is found, rewrite the description to match the actual diff.

Code Review Philosophy

Michael Lynch — “Code reviews are not about finding bugs”

Code reviews are about communication. Tone changes whether a review is acted on. Be specific, not vague. Be kind, not harsh.

Gergely Orosz — “Best teams do thorough reviews quickly”

Fast, substantive reviews are a cultural capability, not a checklist. Prioritize reviewers’ time: be clear about risk and scope, so reviewers know how deep to read.

Camille Fournier — “Senior engineers make the people around them better”

A review is a teaching moment first, a gate second. Help the author understand why, not just what.

Kent Beck — “Make the change easy, then make the easy change”

Small, single-purpose PRs review faster and break less. If a PR is 1500 lines, comment: “Can we split this? Each piece will review clearer.”

John Allspaw — “Sequence risky changes behind reversible ones”

Flag rollback paths explicitly. If this PR needs a rollback, what’s the plan? Who runs it?

Anti-Patterns

  • “Various improvements” as a title — what improvements?
  • Marketing copy (“polished UX”) in the description — describe what changed, not how great it is.
  • 1500-line PRs with one-line descriptions — impossible to review thoroughly.
  • Hidden refactors inside feature PRs — separate them.
  • “Address review comments” commits that don’t quote the comment they address.
  • Vague risk sections — “might be slow” is not a risk analysis; “benchmark shows 10% slower on /checkout” is.

Validation

The scripts/check_pr_body.py script cross-checks:

  • Every file path mentioned in the body appears in the actual diff
  • A “Test plan” or “Testing” section exists and is non-empty

Usage:

Terminal window
scripts/check_pr_body.py <pr-body.md> [base-ref]

If base-ref is omitted, defaults to origin/main.

Summary

  1. Title is conventional: imperative, under 70 chars, no tickets
  2. Description matches the diff: no drift between claims and code
  3. Test plan is reproducible: reviewer can run it without follow-ups
  4. Risk is named explicitly: migrations, breaking changes, flags, perf — or “low”
  5. Self-review first: hunt TODOs, debug prints, secrets, missing tests
  6. Reviewers are chosen: CODEOWNERS + last authors on touched paths
  7. Three-pass review: architecture → logic → polish
  8. Comment hierarchy: BLOCKER / Q / nit
  9. Be kind, specific, clear: reviews are teaching moments first
  10. Separate concerns: one feature, one refactor, one test per PR