Rust Code Review

Review Workflow

Follow this sequence to avoid false positives and catch edition-specific issues:

1. Check

   Cargo.toml

   — Note the Rust edition (2018, 2021, 2024) and MSRV if set. This determines which patterns apply. Check workspace structure if present.
2. Check dependencies — Note key crates (thiserror vs anyhow, tokio features, serde features). These inform which patterns are expected.
3. Scan changed files — Read full functions, not just diffs. Many Rust bugs hide in ownership flow across a function.
4. Check each category — Work through the checklist below, loading references as needed.
5. Verify before reporting — Load beagle-rust:review-verification-protocol before submitting findings.

Output Format

Report findings as:

[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.

Quick Reference

Review Checklist

Ownership and Borrowing

• No unnecessary

  .clone()

  to silence the borrow checker (hiding design issues)
• References have appropriate lifetimes (not overly broad

  'static

  when shorter lifetime works)

• &str

  preferred over

  String

  in function parameters when ownership isn't needed

• impl AsRef<T>

  or

  Into<T>

  used for flexible API parameters
• No dangling references or use-after-move
• Interior mutability (

  Cell

  ,

  RefCell

  ,

  Mutex

  ) used only when shared mutation is genuinely needed
• Small types (≤24 bytes) derive

  Copy

  and are passed by value

• Cow<'_, T>

  used when ownership is ambiguous

Error Handling

• Result<T, E>

  used for recoverable errors, not

  panic!

  /

  unwrap

  /

  expect

• Error types provide context (thiserror with

  #[error("...")]

  or manual

  Display

  )

• ?

  operator used with proper

  From

  implementations or

  .map_err()

• unwrap()

  /

  expect()

  only in tests, examples, or provably-safe contexts
• Error variants are specific enough to be actionable by callers

• anyhow

  used in applications,

  thiserror

  in libraries (or clear rationale for alternatives)

• _or_else

  variants used when fallbacks involve allocation (

  ok_or_else

  ,

  unwrap_or_else

  )

• let-else

  used for early returns on failure (

  let Ok(x) = expr else { return ... }

  )

• inspect_err

  used for error logging,

  map_err

  for error transformation

Traits and Types

• Traits are minimal and cohesive (single responsibility)

• derive

  macros appropriate for the type (

  Clone

  ,

  Debug

  ,

  PartialEq

  used correctly)
• Newtypes used to prevent primitive obsession (e.g.,

  struct UserId(Uuid)

  not bare

  Uuid

  )

• From

  /

  Into

  implementations are lossless and infallible;

  TryFrom

  for fallible conversions
• Sealed traits used when external implementations shouldn't be allowed
• Default implementations provided where they make sense

• Send + Sync

  bounds verified for types shared across threads

Unsafe Code

• unsafe

  blocks have safety comments explaining invariants

• unsafe

  is minimal — only the truly unsafe operation is inside the block
• Safety invariants are documented and upheld by surrounding safe code
• No undefined behavior (null pointer deref, data races, invalid memory access)

• unsafe

  trait implementations justify why the contract is upheld

Naming and Style

• Types are

  PascalCase

  , functions/methods

  snake_case

  , constants

  SCREAMING_SNAKE_CASE

• Modules use

  snake_case

• is_

  ,

  has_

  ,

  can_

  prefixes for boolean-returning methods
• Builder pattern methods take and return

  self

  (not

  &mut self

  ) for chaining
• Public items have doc comments (

  ///

  )

• #[must_use]

  on functions where ignoring the return value is likely a bug
• Imports ordered: std → external crates → workspace → crate/super

• #[expect(clippy::...)]

  preferred over

  #[allow(...)]

  for lint suppression

Performance

• No unnecessary allocations in hot paths (prefer

  &str

  over

  String

  ,

  &[T]

  over

  Vec<T>

  )

• collect()

  type is specified or inferable
• Iterators preferred over indexed loops for collection transforms

• Vec::with_capacity()

  used when size is known
• No redundant

  .to_string()

  /

  .to_owned()

  chains
• No intermediate

  .collect()

  when passing iterators directly works

• .sum()

  preferred over

  .fold()

  for summation
• Static dispatch (

  impl Trait

  ) used over dynamic (

  dyn Trait

  ) unless flexibility required

Linting

• cargo clippy --all-targets --all-features -- -D warnings

  passes
• Key lints respected:

  redundant_clone

  ,

  large_enum_variant

  ,

  needless_collect

• Workspace lint configuration in

  Cargo.toml

  for consistent enforcement
• Doc lints enabled for library crates (

  missing_docs

  ,

  broken_intra_doc_links

  )

Severity Calibration

Critical (Block Merge)

• unsafe

  code with unsound invariants or undefined behavior
• Use-after-free or dangling reference patterns

• unwrap()

  on user input or external data in production code
• Data races (concurrent mutation without synchronization)
• Memory leaks via circular

  Arc<Mutex<...>>

  without weak references

Major (Should Fix)

• Errors returned without context (bare

  return err

  equivalent)

• .clone()

  masking ownership design issues in hot paths
• Missing

  Send

  /

  Sync

  bounds on types used across threads

• panic!

  for recoverable errors in library code
• Overly broad

  'static

  lifetimes hiding API design issues

Minor (Consider Fixing)

• Missing doc comments on public items

• String

  parameter where

  &str

  or

  impl AsRef<str>

  would work
• Derive macros missing for types that should have them
• Unused feature flags in

  Cargo.toml

• Suboptimal iterator chains (multiple allocations where one suffices)

Informational (Note Only)

• Suggestions to introduce newtypes for domain modeling
• Refactoring ideas for trait design
• Performance optimizations without measured impact
• Suggestions to add

  #[must_use]

  or

  #[non_exhaustive]

When to Load References

• Reviewing ownership transfers, borrows, or lifetimes → ownership-borrowing.md
• Reviewing Result/Option handling or error types → error-handling.md
• Reviewing async code, tokio usage, or Send/Sync bounds → async-concurrency.md
• General review (unsafe, performance, API design, clippy) → common-mistakes.md

Valid Patterns (Do NOT Flag)

These are acceptable Rust patterns — reporting them wastes developer time:

• .clone()

  in tests — Clarity over performance in test code

• unwrap()

  in tests and examples — Acceptable where panicking on failure is intentional

• Box<dyn Error>

  in simple binaries — Not every application needs custom error types

• String

  fields in structs — Owned data in structs is correct;

  &str

  fields require lifetime parameters

• #[allow(dead_code)]

  during development — Common during iteration

• todo!()

  /

  unimplemented!()

  in new code — Valid placeholder during active development

• .expect("reason")

  with clear message — Self-documenting and acceptable for invariants

• use super::*

  in test modules — Standard pattern for

  #[cfg(test)]

  modules
• Type aliases for complex types —

  type Result<T> = std::result::Result<T, MyError>

  is idiomatic

• impl Trait

  in return position — Zero-cost abstraction, standard pattern
• Turbofish syntax —

  collect::<Vec<_>>()

  is idiomatic when type inference needs help

• _

  prefix for intentionally unused variables — Compiler convention

• #[expect(clippy::...)]

  with justification — Self-cleaning lint suppression

• Arc::clone(&arc)

  — Explicit Arc cloning is idiomatic and recommended

• std::sync::Mutex

  for short critical sections in async — Tokio docs recommend this

• for

  loops over iterators — When early exit or side effects are needed

Context-Sensitive Rules

Only flag these issues when the specific conditions apply:

.clone()

pub

#[cfg(test)]

unwrap()

Send + Sync

#[must_use]

#[allow]

#[expect]

Copy

Before Submitting Findings

Load and follow

beagle-rust:review-verification-protocol