Review checklist

Use this page next to the diff. Work from the top: the first groups find the problems that cost the most. Each item links to the smell or concept that explains it. The page prints on two sides of paper (use your browser’s print command).

Tip

Ask the agent to run the build gate before it hands the code to you. Then you start at group 2.

1. Build gate

• cargo fmt --check passes.
• cargo clippy --all-targets -- -D warnings passes. See Tooling.
• cargo test passes, and the new code has tests.
• No new #[allow(...)] without a comment that gives the reason (S25).

2. Failure behavior

• Every unwrap() and expect() outside tests has a reason that it cannot fail, written in the expect message (S02).
• No panic! for errors that can happen in normal use: bad input, missing files, network errors (S36).
• No indexing v[i] or slicing s[a..b] on input data without a check (S24).
• No ignored Result: let _ = ..., .ok();, or .unwrap_or_default() that hides a failure (S08).
• Error types are enums or typed errors, not String (S07).
• Errors keep their cause and add context: file name, ID, line number (S09).
• Numeric conversions with as cannot truncate or change sign (S23).

3. Ownership and copies

• Each .clone() has a reason. Look hardest in loops and on collections (S01).
• Functions that only read take &str, &[T], or &T, not String, Vec<T>, &String, or &Vec<T> (S05, S06).
• No Rc<RefCell<T>> object graph that copies a Java design (S03).
• No Arc<Mutex<T>> in code that one thread uses (S04).
• See Ownership and moves, Borrowing, and Clone and Copy.

4. Types and design

• States are enums, not bool flags, string constants, or many Option fields (S15, S17).
• No wildcard _ => arm on an enum that the crate owns (S16).
• IDs and units have their own types where a mix-up is possible (S37).
• Traits are small. No trait hierarchy that copies a Java class hierarchy (S13).
• Box<dyn Trait> only where the set of types is open (S22).
• Only the necessary items are pub (S38).
• See Structs and impl, Traits vs interfaces, and Enums and pattern matching.

5. Concurrency, async, and unsafe

• No blocking calls (std::fs, std::thread::sleep, heavy CPU work) inside async code (S18).
• No std::sync::Mutex guard held across .await (S19).
• Spawned tasks and channels are bounded (S34).
• No static mut or other global mutable state (S29).
• Every unsafe block has a // SAFETY: comment, and the reason is convincing. In application code, ask why unsafe is necessary at all (S20).
• See Threads and shared state, Async, and Unsafe and FFI.

6. Dependencies

• Every crate exists on crates.io and does what the code expects. Check each new API on docs.rs (S26).
• Crates are maintained, versions are current, and default features are not pulling in large code that the project does not use (S28).
• No wildcard versions (*). Cargo.lock is committed for applications.
• See Modules, crates, and Cargo.

7. Readability

• Iterator chains are short enough to read. A for loop is fine when it is clearer (S10, S11).
• Nesting is shallow. ? and let ... else replace nested match and if let.
• Comments explain why, not what.
• See Iterators vs streams.

8. Tests

• Error paths have tests, not only the happy path.
• No thread::sleep in tests, and no mocks where a small fake or a real value is simpler (S30).
• Property tests (proptest) for parsers and other code with many inputs, where they fit.
• See Testing.