Review Skill for Claude

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

• 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

• 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

• 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)

Error Handling (Should Fix)

Reference Chapter 4:

• thiserror for libraries: Are custom errors using thiserror with proper #[from] and #[error] attributes?
• anyhow in libraries: Is anyhow being used in library code? (should use thiserror instead)
• Error propagation: Is ? used instead of verbose match chains?
• Error context: Are errors descriptive and actionable?

API Design (Consider)

Reference Chapter 6 and 7:

• Breaking changes: Are public API changes intentional and documented?
• Error types: Are custom errors descriptive and actionable?
• Builder pattern: For structs with many optional fields
• Naming: Does it follow Rust conventions (snake_case, clear verb prefixes)?
• Static vs dynamic dispatch: Is dyn Trait used only when necessary? (see Chapter 6)
• Type state pattern: Could compile-time state validation improve safety? (see Chapter 7)

Documentation (Consider)

Reference Chapter 8:

• Public API docs: Are public items documented with ///?
• Error/Panic sections: Do fallible functions document their error conditions?
• Comments explain why: Are comments explaining the "why" not the "what"?
• TODOs have issues: Are TODO comments linked to tracked issues?

Output Format

Post review comments directly on the PR using inline comments on specific lines of code:

CRITICAL: Inline comment parameters:

• Always use line and side parameters — NEVER use position. The position parameter refers to a diff hunk offset and will fail with HTTP 422 errors.
• The line value must be a line number that appears in the PR diff for that file. If the line you want to comment on is not in the diff, comment on the nearest changed line and reference the actual line in the body, or use a general PR comment instead.
• Use -F line=N (not -f line=N) so the value is sent as an integer.

# For line-specific comments:
gh api repos/{owner}/{repo}/pulls/{pr_number}/comments \
  -f body="**[Blocking]** Issue description here. See Chapter X for details." \
  -f commit_id="<commit_sha>" \
  -f path="src/foo.rs" \
  -f side="RIGHT" \
  -f subject_type="line" \
  -F line=42

# For the summary comment, always update an existing one if present (instead of creating a new one):
COMMENT_ID=$(gh api repos/{owner}/{repo}/issues/{pr_number}/comments --jq '.[] | select(.body | contains("Reviewed by Claude Code")) | .id' | tail -1)
BODY="## Review Summary\n\n...\n\n---\n_Reviewed by Claude Code {model}_"
if [ -n "$COMMENT_ID" ]; then
  gh api repos/{owner}/{repo}/issues/comments/$COMMENT_ID -X PATCH -f body="$BODY"
else
  gh pr comment {pr_number} --body "$BODY"
fi

Each inline comment should:

• Start with severity: [Blocking], [Should Fix], or [Consider]
• Explain the problem clearly
• Reference the relevant best practice chapter when applicable
• Suggest a fix
• Important: For duplicate findings in multiple locations within a file (E.g. multiple tests have the same issue), leave a single online comment referencing the various places in the file it needs to be fixed instead of leaving several inline comments about the same issue.

After posting inline comments, add a summary comment with:

• Overall assessment (summary of the change, keep this to a couple of sentences maximum)
• Findings (bullet point, concise summary of issues found)
• Test coverage assessment
• Final recommendation (Approve / Approve with suggestions / Request changes)
• Sign-off line at the end: _Reviewed by Claude Code {model}_ where {model} is the current model name (e.g., "Opus 4.5")

Important: Always update the existing summary comment rather than creating a new one. Use the find-and-update pattern shown above.

Guidelines for High-Signal Reviews

• Be specific: Always reference exact file and line numbers
• Explain why: Don't just say "this is wrong" - explain the consequence
• Reference best practices: Link to the relevant chapter when suggesting changes
• Suggest fixes: Provide concrete code examples when helpful
• Skip the obvious: Don't mention things clippy would catch (run it instead)
• Focus on this PR: Don't request unrelated refactoring
• Acknowledge good work: Note well-designed solutions briefly
• Don't Overwhelm: Stick to a maximum of the top 5 most important findings.

Posting Review Comments

Batch all findings into a single turn:

• Collect all inline comments (blocking, should-fix, and consider issues) during analysis
• Post all inline comments in one go using multiple gh api calls
• Post the summary comment as the final action

This approach reduces turn count by avoiding iterative comment posting.