The 5-minute Rust PR review

An agent just opened a PR. The CI gates passed: cargo fmt, cargo clippy, cargo test all green. That tells you the code compiles, lints, and the tests it wrote pass. It does not tell you the code is right.

This is the playbook for the human part. Five questions, in order, five minutes total. Stop at the first one that fails.

Question 1 — Diff shape

Before you read a line of code, look at the diff stats. They tell you whether the agent did what was asked.

The 30-second version: gh pr diff <id> --name-only and gh pr diff <id> --stat.

Question 2 — Public API changes

Look at every pub item that changed: structs, traits, enums, functions, modules. Public surface is the contract callers depend on.

# What public items moved?
git diff main...HEAD -- '*.rs' | rg '^\+pub'
git diff main...HEAD -- '*.rs' | rg '^-pub'

Ask yourself:

• Did the agent add public items that should have been private?
• Did the agent break an existing public signature? If yes, is this a semver break the caller needs to know about?
• Are new public items documented? Public items without doc comments are half-finished.

Question 3 — Error handling

Open every Result-returning function in the diff. Three checks:

1. Concrete error type? No Box<dyn Error> or bare anyhow::Result<T> in library code. The error type should be a named enum (typically thiserror::Error) with variants you can match on.
2. Errors propagated with ?? No match chains that re-wrap the same error five times.
3. unwrap / expect count? Zero in non-test code unless there is an inline comment justifying it.

# Quick smell test on the diff.
git diff main...HEAD -- '*.rs' | rg '^\+.*\.unwrap\(\)|^\+.*\.expect\('

Every line that returns indicates a new panic site. Read each one and decide if it is acceptable.

Question 4 — Allocations, clones, locks

This is where idiomatic Rust shows its colors. Three searches:

git diff main...HEAD -- '*.rs' | rg '^\+.*\.clone\(\)'   # new clones
git diff main...HEAD -- '*.rs' | rg '^\+.*Arc<Mutex|^\+.*Arc<RwLock'  # new shared mutable state
git diff main...HEAD -- '*.rs' | rg '^\+.*Box<dyn'      # new dynamic dispatch

For each new .clone(): is it on a small type (Copy, Arc, Rc, small enum)? Fine. Is it on a Vec, String, or large struct? Push back. Most can be replaced with a borrow.

For each new Arc<Mutex<T>> / Arc<RwLock<T>>: justify the sharing and the mutation. Atomics, channels, or &mut self often work.

For each new Box<dyn Trait>: confirm dynamic dispatch is needed. Heterogeneous collections, plugin boundaries: yes. Hot paths with a known concrete type: use generics.

Question 5 — Test coverage of the change

Find the tests added in the diff. Three checks:

1. Happy path covered? At least one test that exercises the new behavior end-to-end.
2. Error paths covered? For each new Err variant, at least one test that produces it.
3. Edge cases? Empty input, large input, unicode, zero, negative, overflow, concurrent access if relevant.

# Tests added in this PR.
git diff main...HEAD -- '*.rs' | rg '^\+.*#\[test\]|^\+.*#\[tokio::test\]'

If the agent added one test for the happy path and nothing else, ask for the error path tests by name: "Add a test for ConfigError::MissingPort and ConfigError::InvalidPort."

The merge gate

After the five questions:

The agent is your peer reviewer too. Use it.

A worked example: real Sail-style PR

Pretend you are reviewing a PR that adds a new catalog provider backed by SQLite. Here is the actual review flow:

What this playbook is not

This is not a replacement for understanding the change. For risky diffs (concurrency, unsafe, public API breaks, security paths), read every line. For routine diffs, the five questions are the floor that prevents the worst failures from shipping.

The playbook does not catch design problems. If the agent built the wrong thing, the five questions cannot save you. That is a prompting problem, addressed in the Prompting chapter.