Skip to content

code-review-standards.md

Latest commit

 

History

History
82 lines (54 loc) · 3.25 KB

code-review-standards.md

File metadata and controls

82 lines (54 loc) · 3.25 KB
Raw

Code Review Standards — Organization Baseline

Scope: organization
Status: enabled
Path: org/code-review-standards.md

Purpose

Consistent code review practices reduce defects, spread knowledge, and build shared ownership. These guidelines apply to all repositories. They set expectations for both authors and reviewers.

Author Responsibilities

Before Opening a PR

  • Scope tightly. A PR should do one thing. Refactoring, feature work, and dependency bumps belong in separate PRs.
  • Write a meaningful description. Include: what changed, why it changed, and how to test it. Link the relevant issue or ticket.
  • Self-review first. Read your own diff before requesting review. Catch typos, debug logs, and commented-out code yourself.
  • Keep it reviewable. PRs over 400 lines of non-generated code are hard to review well. Break them up.

Draft PRs

Use draft status to share work-in-progress or get early design feedback. Mark a PR ready for review only when it is genuinely ready to merge.

Reviewer Responsibilities

Mindset

  • Review the code, not the author. All feedback should be about the change, never about the person.
  • Be specific. "This function is confusing" is not actionable. "This function has three responsibilities; I'd extract the validation logic into a helper" is.
  • Distinguish blocking from non-blocking. Use prefixes:
    • [blocking] — must be addressed before merge
    • [nit] — style or minor quality; author's call
    • [question] — you want to understand, not necessarily change
    • [suggestion] — a potentially better approach, but not required

What to Look For

  1. Correctness — Does the code do what the description claims? Are edge cases handled?
  2. Security — Does the change introduce injection, broken auth, or secret exposure? (See org/secure-coding-standards.md.)
  3. Tests — Are new behaviors tested? Are tests meaningful (not just coverage theater)?
  4. Readability — Will a new team member understand this in six months?
  5. Performance — Are there obvious N+1 queries, unbounded loops, or blocking calls in async contexts?

Response Time

  • First review within 1 business day of the ready-for-review signal.
  • Re-review after author updates within 4 business hours.

Merge Criteria

A PR may be merged when:

  1. At least one approved review from a non-author team member.
  2. All [blocking] comments are resolved.
  3. CI is green (all required checks pass).
  4. The branch is up to date with the target branch.

Merging your own PR without review is permitted only for hotfixes during an active incident, and requires a follow-up review within 24 hours.

Review Anti-Patterns to Avoid

Anti-pattern Why it's harmful
Rubber-stamping without reading Increases defect escape rate
Reviewing only what you wrote previously Creates knowledge silos
Blocking on style preferences already caught by linter Wastes author time; automate it
Leaving a PR without response for > 2 days Blocks teammates and kills flow
Making sweeping architectural suggestions on a small PR Wrong venue; open a design doc discussion

Questions about this policy → #engineering-process on Slack.