existential-birds/beagle/rust-code-review

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. Edition 2024 introduces breaking changes to unsafe semantics, RPIT lifetime capture, temporary scoping, and ! type fallback. 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 — Complete Gates (below), including the verification-protocol gate, before submitting findings.

Gates

These steps are sequenced: do not skip ahead with “mental verification.” Each step has an objective Pass you can satisfy from files on disk and your own read path.

1. Crate context — Before relying on edition-specific checklist rows (Edition 2024, MSRV-sensitive APIs) or dependency assumptions. Pass: You opened the relevant Cargo.toml (package or workspace manifest) and can state edition and rust-version (if set) in one line.
2. Expanded read — Before reporting a Major or Critical finding. Pass: You read the full function, unsafe block, or impl / trait item that contains the cited line (not only a diff hunk).
3. Severity match — Before each finding line in the report. Pass: The Severity label matches Severity Calibration for that issue class, or you use Informational and give a one-line rationale.
4. Verification protocol — Before finalizing the report. Pass: the review-verification-protocol skill is loaded and every step in it that applies to this review is completed (do not substitute a vague “I checked”).

Output Format

Report findings as:

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

Quick Reference

For development guidance on performance, pointer types, type state, clippy config, iterators, generics, and documentation, load the rust-best-practices skill.

Review Checklist

Ownership and Borrowing

• [ ] No unnecessary .clone() to silence the borrow checker (hiding design issues)
• [ ] No .clone() inside loops — prefer .cloned() or .copied() on iterators
• [ ] No cloning to avoid lifetime annotations (take ownership explicitly or restructure)
• [ ] References have appropriate lifetimes (not overly broad 'static when shorter lifetime works)
• [ ] Edition 2024: RPIT (-> impl Trait) captures all in-scope lifetimes by default; use + use<'a> for precise capture control
• [ ] &str preferred over String, &[T] over Vec<T> in function parameters
• [ ] 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
• [ ] Iterator chains preferred over index-based loops for collection transforms
• [ ] No premature .collect() — pass iterators directly when the consumer accepts them
• [ ] .sum() preferred over .fold() for summation (compiler optimizes better)
• [ ] _or_else variants used when fallbacks involve allocation
• [ ] Edition 2024: if let temporaries drop at end of the if let — code relying on temporaries living through the else branch needs restructuring
• [ ] Edition 2024: Box<[T]> implements IntoIterator — prefer direct iteration over into_vec() first

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
• [ ] #[diagnostic::on_unimplemented] used on public traits to provide clear error messages when users forget to implement them

Interface Design

Detailed guidance: references/interface-design.md

• [ ] Methods on dyn-intended traits don't use Self by value, generic params, or associated constants (or are gated where Self: Sized)
• [ ] New traits ship with blanket impls for &T, &mut T, Box<T> so reference and smart-pointer arguments work
• [ ] Iterable types implement IntoIterator for &Self and &mut Self, not just Self
• [ ] Deref only used for transparent forwarding, never as "inheritance" — inherent-method ambiguity is a real bug class
• [ ] Fallible cleanup uses an explicit close()/shutdown() returning Result; Drop is best-effort fallback only
• [ ] No block_on(...) or new runtime in Drop (deadlock under async runtimes)
• [ ] Public types have a compile-time fn is_normal<T: Sized + Send + Sync + Unpin>() {} test so auto-trait regressions surface at build time
• [ ] Re-exported foreign types in public API are flagged — downstream major bumps become this crate's breaking change
• [ ] Getter methods follow the convention fn name(&self) not fn get_name(&self) (reserve get_* for Option-returning or interesting lookups)
• [ ] as_* is cheap reference-to-reference, to_* may allocate, into_* consumes — verify the cost matches the prefix
• [ ] Standard derives (Debug, Clone, Default, PartialEq, Eq, Hash) considered for every public type; Copy only when truly cheap and value-like (removing it later is breaking)

Patterns in the Wild

Detailed guidance: references/patterns-in-the-wild.md

• [ ] Index-pointer graphs use generational indices (slotmap::DefaultKey, petgraph::NodeIndex) — bare usize orphans after Vec::swap_remove
• [ ] Drop guards bound to let _guard = ..., never let _ = ... (the second drops immediately, not at scope end)
• [ ] Drop-guard cleanup not relied upon under panic = "abort" (destructors do not run)
• [ ] Extension traits used only when the type is foreign; for owned types use inherent impl directly
• [ ] Extension-trait methods don't shadow popular existing methods on the same type (ambiguity at call sites)
• [ ] Crate prelude module additions treated as semver-minor (RFC 1105); reserve for major releases when possible

Concurrency Design (Models)

Detailed guidance: references/concurrency-models.md

• [ ] Concurrency model (shared memory vs worker pool vs actor) named explicitly in the design; primitives match the model
• [ ] Mutex critical section is short, measurable, and excludes I/O / network calls / .await points
• [ ] Worker-pool queues are bounded; backpressure strategy is named
• [ ] Actor mailbox channels are sized to expected load; cross-actor cycles reviewed for deadlock
• [ ] async is not conflated with parallelism — join! interleaves on one thread; only tokio::spawn (or equivalent on a multi-threaded runtime) parallelizes
• [ ] CAS retry loops on a hot atomic considered for replacement with fetch_add/sharding (CAS is O(N²) under contention)
• [ ] println! / dbg! not used as a debugging tool for race conditions (the Stdout mutex changes the race)

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
• [ ] Edition 2024: unsafe fn bodies use explicit unsafe {} blocks around unsafe ops (unsafe_op_in_unsafe_fn is deny)
• [ ] Edition 2024: extern "C" {} blocks written as unsafe extern "C" {}
• [ ] Edition 2024: #[no_mangle] and #[export_name] written as #[unsafe(no_mangle)] and #[unsafe(export_name)]