Review

Rust Pull Request Review

Review the current PR with a focus on high-signal, actionable feedback. Avoid nitpicks and style preferences unless they impact correctness or maintainability.

Usage

/review

This skill runs in GitHub Actions and automatically reviews the current PR, posting inline comments and a summary directly to GitHub.

Best Practices Reference

Before reviewing, familiarize yourself with Apollo's Rust best practices by reading the rust-best-practices skill. Read ALL relevant chapters in the same turn in parallel. Reference these files when providing feedback:

• Chapter 1 - Coding Styles and Idioms: Borrowing vs cloning, Copy trait, Option/Result handling, iterators, comments
• Chapter 2 - Clippy and Linting: Clippy configuration, important lints, workspace lint setup
• Chapter 3 - Performance Mindset: Profiling, avoiding redundant clones, stack vs heap, zero-cost abstractions
• Chapter 4 - Error Handling: Result vs panic, thiserror vs anyhow, error hierarchies
• Chapter 5 - Automated Testing: Test naming, one assertion per test, snapshot testing
• Chapter 6 - Generics and Dispatch: Static vs dynamic dispatch, trait objects
• Chapter 7 - Type State Pattern: Compile-time state safety, when to use it
• Chapter 8 - Comments vs Documentation: When to comment, doc comments, rustdoc
• Chapter 9 - Understanding Pointers: Thread safety, Send/Sync, pointer types

Additionally, you MUST ALWAYS read @.claude/MEMORIES.md which is a long term memory file of feedback you have been given on your reviews on previous PRs. You should utilize and consider these memories when determining what feedback to leave on the PR.

Pull Request Context

Fetch PR information:

PR Metadata: !gh pr view --json number,title,body,author,baseRefName,headRefName,commits,headRepository,headRepositoryOwner PR Diff: !gh pr diff PR Level Comments: !gh pr view --json comments Get existing inline review comments: gh api repos/{owner}/{repo}/pulls/{pr_number}/comments

Important: Review Only

DO NOT:

• Pull down the branch or checkout the code
• Run tests, clippy, cargo build, or any other commands
• Attempt to run or validate the code locally

All automated checks (tests, linting, clippy, formatting) are handled by GitHub Actions. Focus exclusively on reviewing the code diffs.

Review Process

You have a maximum of 10 turns to complete this review. Allocate turns strategically:

• Turn 1: Gather all context (parallel commands)
• Turns 2-4: Analysis and evaluation
• Turns 5+: Post comments and summary

1. Gather all context (single turn - run all commands in parallel)

   • Fetch PR metadata, diff, and all existing comments
   • This gives you complete information before starting analysis

2. Analyze the changes

   • Read the PR description and linked issues to understand intent
   • Review the complete diff to identify the scope
   • Review existing feedback to note what's already been raised
   • Do not duplicate comments that have already been made
   • Consider context from existing discussions when evaluating code
   • Important: If a user has responded to a comment/issue stating that they will not be following that suggestion, do not argue with the user, and do not bring up that suggestion again within this PR

3. Manual review using the criteria below, referencing the best practices documents

   • Focus on high-signal findings only
   • Skip issues already raised by other reviewers
   • Batch findings into cohesive inline comments

Review Criteria

Security (Blocking)

• Unsafe blocks: Is unsafe necessary? Is the invariant documented with // SAFETY:? Is there a safe alternative?
• Input validation: Is user/external input validated before use?
• Error exposure: Are internal errors or stack traces exposed to users?
• Unwrap on external data: unwrap() or expect() on user input, network data, or file contents (see Chapter 4)
• Secret handling: Are credentials, tokens, or keys properly handled (not logged, not in errors)?
• SQL/Command injection: Is external input used in queries or shell commands without sanitization?

Performance (Should Fix)

Reference Chapter 1 and Chapter 3 for details:

• Unnecessary clones: .clone() where a borrow would suffice
• Allocation in hot paths: String or Vec allocations in loops or frequently-called functions
• Inefficient iterators: .collect() followed by iteration, when chaining would work (see Chapter 1.5)
• Blocking in async: Synchronous I/O or heavy computation in async functions without spawn_blocking
• Missing Cow: Could Cow<str> or Cow<[T]> avoid allocations? (see Chapter 3.2)
• Large structs by value: Passing large structs by value instead of reference (see Chapter 3.3)
• Early allocation: Using unwrap_or, map_or instead of _else variants when allocation is involved (see Chapter 1.4)

Test Coverage (Should Fix)

Reference Chapter 5 for testing best practices:

• New public functions: Are they tested?
• Error paths: Are Err cases and edge conditions tested? (see Chapter 4.6)
• Changed logic: If behavior changed, are tests updated to reflect it?
• Integration tests: For public API changes, are there integration tests?
• Test naming: Do test names describe the behavior being tested? (see Chapter 5.1)
• One assertion per test: Are tests focused on a single behavior?

Correctness & Rust Idioms (Blocking/Should Fix)

Reference Chapter 1, 4, and 6:

• Panics in library code: unwrap(), expect(), panic!() in library code that should return Result
• Ignored errors: let _ = fallible_operation() without justification
• Missing #[must_use]: Functions returning Result or important values
• Lock poisoning: Proper handling of Mutex/RwLock poisoning (see Chapter 9)
• Iterator invalidation: Modifying collections while iterating
• Integer overflow: Arithmetic on user-controlled values without checked/saturating ops
• Lifetime issues: Unnecessary 'static bounds, overly restrictive lifetimes
• Clone traps: Auto-cloning inside loops, cloning references instead of taking ownership (see Chapter 1.1)