rust-code-review

name: rust-code-review description: Use when reviewing Rust code changes. Scores 0-100 with deductions per category.

Rust Code Review

Overview

Score every code review from 100 → 0. Start at 100 and apply deductions.

Issues detectable by clippy, rustfmt, or the compiler are NOT scored here.

Scoring Summary

Architecture

Bidirectional Dependency (-20 per occurrence)

Crates and modules must depend in one direction only. Types-only modules/crates are allowed as shared dependencies.

// ❌ BAD: bidirectional dependency between modules
// order/mod.rs imports payment
mod order {
    use crate::payment::Processor;
    pub fn new_order(p: &Processor) { ... }
}

// payment/mod.rs imports order
mod payment {
    use crate::order::Order;
    pub fn charge(o: &Order) { ... }
}

// ✅ GOOD: unidirectional with shared types
// types/mod.rs — no imports from order or payment
pub mod types {
    pub struct Order {
        pub id: String,
        pub amount: u64,
    }
}

// payment depends on types only
mod payment {
    use crate::types::Order;

    pub fn charge(o: &Order) -> Result<(), PaymentError> { ... }
}

// order depends on types and payment
mod order {
    use crate::types::Order;
    use crate::payment;

    pub fn place(o: &Order) -> Result<(), Box<dyn std::error::Error>> {
        payment::charge(o)?;
        Ok(())
    }
}

Duplicated Logic (-10 per occurrence)

Extract shared behavior into a function or module.

// ❌ BAD: duplicated validation
fn create_user(name: &str) -> Result<(), AppError> {
    if name.is_empty() || name.len() > 100 {
        return Err(AppError::InvalidName);
    }
    // ...
}

fn update_user(name: &str) -> Result<(), AppError> {
    if name.is_empty() || name.len() > 100 {
        return Err(AppError::InvalidName);
    }
    // ...
}

// ✅ GOOD: shared validation
fn validate_name(name: &str) -> Result<(), AppError> {
    if name.is_empty() || name.len() > 100 {
        return Err(AppError::InvalidName);
    }
    Ok(())
}

fn create_user(name: &str) -> Result<(), AppError> {
    validate_name(name)?;
    // ...
}

fn update_user(name: &str) -> Result<(), AppError> {
    validate_name(name)?;
    // ...
}

Project Design Violation (-10 per occurrence)

Code must follow the architecture described in the project's AGENTS.md. Check module boundaries, naming conventions, and crate responsibilities.

Unit Testing

Non-Refactoring-Tolerant Test (-3 per test case)

Tests must assert results, not how the code works internally.

// ❌ BAD: asserts internal call behavior via mock
#[test]
fn test_transfer() {
    let mut mock_repo = MockRepo::new();
    mock_repo.expect_debit()
        .with(eq("alice"), eq(100))  // asserting internal args
        .returning(|_, _| Ok(()));

    let svc = Service::new(mock_repo);
    svc.transfer("alice", "bob", 100).unwrap();
}

// ✅ GOOD: asserts observable outcome
#[test]
fn test_transfer() {
    let repo = InMemoryRepo::new();
    repo.set_balance("alice", 200);
    repo.set_balance("bob", 50);
    let svc = Service::new(repo.clone());

    svc.transfer("alice", "bob", 100).unwrap();

    assert_eq!(repo.get_balance("alice"), 100);
    assert_eq!(repo.get_balance("bob"), 150);
}

WhiteBox Test (-3 per test case)

Always use BlackBox testing. Tests must exercise the public API only.

// ❌ BAD: tests private internals
#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_internal_ledger() {
        let mut w = Wallet { ledger: vec![] };  // accessing private field
        w.add_entry(100);                        // calling private method
        assert_eq!(w.ledger.len(), 1);
    }
}

// ✅ GOOD: tests public API from integration test or public interface
// tests/wallet_test.rs (integration test)
use myapp::wallet::Wallet;

#[test]
fn test_deposit() {
    let mut w = Wallet::new();

    w.deposit(100).unwrap();

    assert_eq!(w.balance(), 100);
}

Test Module Structure (-3 per file)

Prefer integration tests in tests/ directory for BlackBox testing. When using #[cfg(test)] inline modules, only test via public API.

// ❌ BAD: inline test accessing private state
#[cfg(test)]
mod tests {
    use super::*;  // imports private items
}

// ✅ GOOD: integration test file
// tests/wallet_test.rs
use myapp::wallet;  // only public API

Uncovered Code (-1 per line)

Every line of production code should be exercised by tests. Run:

cargo tarpaulin --out html

Security

Review security against the project context (README.md / AGENTS.md).

// ❌ HIGH: SQL injection
fn get_user(pool: &PgPool, id: &str) -> Result<User, sqlx::Error> {
    let query = format!("SELECT * FROM users WHERE id = '{}'", id);
    sqlx::query_as(&query).fetch_one(pool).await
}

// ✅ GOOD: parameterized query
fn get_user(pool: &PgPool, id: &str) -> Result<User, sqlx::Error> {
    sqlx::query_as("SELECT * FROM users WHERE id = $1")
        .bind(id)
        .fetch_one(pool)
        .await
}

// ❌ HIGH: unsafe without justification
unsafe {
    std::ptr::write(ptr, value);
}

// ✅ GOOD: justified unsafe with safety comment
// SAFETY: `ptr` is valid, aligned, and non-null because it was
// obtained from `Box::into_raw` one line above.
unsafe {
    std::ptr::write(ptr, value);
}

Specification

Spec Not Met (-100)

If the code does not satisfy the specification, the review score is effectively 0. Verify behavior against the requirements before anything else.

Rust Best Practices

Unwrap in Production Code (-5 per occurrence)

// ❌ BAD: panics on failure
let config = std::fs::read_to_string("config.toml").unwrap();

// ✅ GOOD: propagate error
let config = std::fs::read_to_string("config.toml")
    .map_err(|e| AppError::Config(format!("read config: {e}")))?;

Error Type Design (-5 per occurrence)

// ❌ BAD: stringly-typed errors
fn process() -> Result<(), String> {
    Err("something failed".to_string())
}

// ✅ GOOD: typed errors with thiserror
#[derive(Debug, thiserror::Error)]
pub enum ProcessError {
    #[error("database connection failed: {0}")]
    Database(#[from] sqlx::Error),
    #[error("invalid input: {field}")]
    InvalidInput { field: String },
}

Ownership & Borrowing (-3 per occurrence)

// ❌ BAD: unnecessary clone
fn greet(name: String) {
    println!("Hello, {name}");
}

let name = "alice".to_string();
greet(name.clone());  // clone not needed if name is not used after
println!("{name}");    // oh wait, it is used — but still consider &str

// ✅ GOOD: borrow when ownership is not needed
fn greet(name: &str) {
    println!("Hello, {name}");
}

let name = "alice".to_string();
greet(&name);
println!("{name}");

Missing #[must_use] (-2 per occurrence)

// ❌ BAD: caller can silently ignore result
pub fn validate(input: &str) -> bool {
    !input.is_empty()
}

// ✅ GOOD: compiler warns if result ignored
#[must_use]
pub fn validate(input: &str) -> bool {
    !input.is_empty()
}

Stringly-Typed API (-3 per occurrence)

// ❌ BAD: status as string
fn set_status(order: &mut Order, status: &str) {
    order.status = status.to_string();
}

// ✅ GOOD: enum for known variants
enum OrderStatus {
    Pending,
    Shipped,
    Delivered,
}

fn set_status(order: &mut Order, status: OrderStatus) {
    order.status = status;
}

Large Function / God Function (-3 per occurrence)

Functions exceeding ~50 lines should be broken into smaller, well-named functions.

Review Procedure

1. Spec — Does the code meet the requirements? If not, stop (-100).
2. Architecture — Check dependency direction, duplication, project design.
3. Unit Tests — BlackBox? Refactoring-tolerant? Integration tests? Coverage?
4. Security — Review against project context for vulnerabilities.
5. Rust Best Practices — Error handling, ownership, types, safety.
6. Calculate Score — Start at 100, apply all deductions. Minimum is 0.

Output Format

## Code Review: [component/file]

**Score: XX/100**

### Deductions

| # | Category | Detail | Deduction |
|---|----------|--------|-----------|
| 1 | Architecture | Bidirectional dep: order ↔ payment | -20 |
| 2 | Unit Test | WhiteBox test: test_internal_ledger | -3 |
| ... | ... | ... | ... |

**Total Deductions: -XX**

### Summary
[brief summary of key issues and recommendations]