Code Review Culture

Code review is the team's main teaching loop for Rust craft: ownership, async pitfalls, unsafe boundaries, and test gaps. High-signal culture means clear standards, respectful feedback, and predictable turnaround - not perfectionism that blocks delivery.

Recipe

Quick-reference recipe card - copy-paste ready.

## PR description template
**Intent:** one sentence
**Risk:** migration / perf / unsafe / security
**Test evidence:** `cargo test`, miri, benchmark note
**Rollback:** flag or revert plan
 
## Review comment tiers
🔴 Must fix - correctness, safety, data loss
🟡 Should fix - clarity, missing test (defer with ticket OK)
🟢 Nit - style; author discretion

When to reach for this:

• Review latency exceeds 24 hours routinely
• Reviews are style wars without teaching
• Juniors fear posting PRs
• Incidents trace to missed review checks

Working Example

// Author documents intentional trade-off for reviewers
 
// REVIEW: kept sync read here - RFC-042 phase 1 read-only path.
// Async sqlx migration tracked in ENG-4412.
pub async fn get_catalog_item(pool: &sqlx::PgPool, id: &str) -> Result<Item, AppError> {
    let row = sqlx::query_as!(Item, "SELECT id, name FROM items WHERE id = $1", id)
        .fetch_one(pool)
        .await?;
    Ok(row)
}

# Reviewer local checklist
cargo test -p orders-api
cargo clippy -- -D warnings
# If PR touches unsafe:
cargo miri test -p ffi-bridge

What this demonstrates:

• Author links deferred work to ticket, reducing re-debate
• Reviewers run automated gates locally; humans focus on design
• unsafe PRs trigger extra CI expectations

Deep Dive

What to Review in Rust

• Correctness - Error handling, bounds, unsafe invariants documented.
• Operability - Logs, metrics, panic policy on hot path.
• API design - Public crate surface follows Rust API guidelines.
• Tests - Happy path + one failure mode; property tests for parsers.
• Scope - PR matches stated intent; drive-by refactors separated.

Turnaround Norms

• First response < 24h business hours.
• LGTM with nits is valid; author chooses nits.
• Escalate blocking disagreement to tech lead after 2 rounds.

Automation

• rustfmt + clippy in CI - not debated in review.
• cargo audit on dependency changes.
• CODEOWNERS for unsafe/ and migrations/.

Gotchas

• LGTM without reading tests - Fix: Require test evidence in template.
• Reviewers only senior - Bottleneck. Fix: Rotate; juniors review docs and tests.
• 4000-line PR - Shallow review. Fix: Split by crate or feature flag.
• Bikeshedding naming - Fix: Nit tier; team rustfmt/clippy defaults.
• No unsafe second reviewer - Fix: Governance policy requires two approvers.

Alternatives

FAQs

Related

• Mentoring & Leveling - grow reviewers
• Coding Standards & API Guidelines - standards
• Unsafe & Dependency Governance - unsafe review
• Linting Best Practices - automate style
• Leadership Best Practices - summary

Stack versions: This page was written for Rust 1.97.0 (edition 2024), Tokio 1.x, Axum 0.8, serde 1.0, sqlx 0.8, clap 4, and Polars 0.46+.