review-verification-protocol

Review Verification Protocol

This protocol MUST be followed before reporting any code review finding. Skipping these steps leads to false positives that waste developer time and erode trust in reviews.

Hard gates (sequenced)

Complete these in order before you add a finding. Skip a gate only when it clearly does not apply (e.g. skip the usages gate if the finding is not about dead code or “unused”).

1. Read scope — Pass: You name the exact file path(s) and the function, impl, or macro_rules! block you read in full (not only a diff hunk or partial snippet).
2. Usages (dead / unused) — Pass: You ran a repo-wide reference search (rg, IDE references, or equivalent) and either state zero matches for the symbol you call unused, or list each match and why it still supports the finding.
3. Surrounding behavior — Pass: You checked callers, trait impls, #[cfg], or error propagation that could make the pattern intentional; note one concrete checked location (path + rough location) or state “none relevant after search.”
4. Edition and API — Pass: You opened the relevant Cargo.toml for the crate under review and either quote the [package] edition = "..." line or state the default edition applies and name the manifest path you checked.
5. Wrong vs style — Pass: In one sentence, you explain why the code is incorrect, unsound, or risky for this project—not merely a different valid style.

Pre-Report Verification Checklist

Before flagging ANY issue, verify:

• [ ] I read the actual code - Not just the diff context, but the full function/impl block
• [ ] I searched for usages - Before claiming "unused", searched all references
• [ ] I checked surrounding code - The issue may be handled elsewhere (trait impls, error propagation)
• [ ] I verified syntax against current docs - Rust edition, crate versions, and API changes
• [ ] I checked the project's Rust edition - Edition 2021 vs 2024 changes what is required vs optional (see [Edition-Aware Review](#edition-aware-review))
• [ ] I distinguished "wrong" from "different style" - Both approaches may be valid
• [ ] I considered intentional design - Checked comments, CLAUDE.md, architectural context

Verification by Issue Type

"Unused Variable/Function"

Before flagging, you MUST:

1. Search for ALL references in the codebase (grep/find)
2. Check if it's pub and used by other crates in the workspace
3. Check if it's used via derive macros, trait implementations, or conditional compilation (#[cfg])
4. Verify it's not a trait method required by the trait definition

Common false positives:

• Trait implementations where the method is defined by the trait
• #[cfg(test)] items only used in test builds
• Derive-generated code that uses struct fields
• Types used via From/Into conversions

"Missing Error Handling"

Before flagging, you MUST:

1. Check if the error is handled at a higher level (caller propagates with ?)
2. Check if the crate has a top-level error type that wraps this error
3. Verify the unwrap() isn't in test code or after a safety-ensuring check

Common false positives:

• unwrap() in tests and examples (expected pattern)
• expect("reason") after validation (e.g., regex::Regex::new on a literal)
• Error propagation via ? (the caller handles it)
• let _ = tx.send(...) — intentional when receiver may have dropped

"Unnecessary Lifetime" / RPIT Capture (Edition 2024)

Before flagging, you MUST:

1. Check the project's Rust edition in Cargo.toml
2. In edition 2024, -> impl Trait captures ALL in-scope lifetimes by default
3. A lifetime that appears "unnecessary" may be implicitly captured — the code is correct
4. If the author uses + use<'a> syntax, this is precise capture control, not a mistake

Common false positives:

• Lifetime parameters on functions returning impl Trait — edition 2024 captures them implicitly
• + use<'a, T> syntax — this is the new precise capturing syntax, not an error
• Removing an explicit lifetime bound that edition 2024 now provides automatically

"Missing Unsafe Block" (Edition 2024)

Before flagging, you MUST:

1. Check if the code is inside an unsafe fn
2. In edition 2024, unsafe_op_in_unsafe_fn is deny-by-default — unsafe operations inside unsafe fn REQUIRE explicit unsafe {} blocks
3. This is edition-required behavior, not unnecessary verbosity

Common false positives:

• unsafe {} blocks inside unsafe fn — REQUIRED in edition 2024, not redundant
• unsafe extern "C" {} — REQUIRED in edition 2024, not optional
• #[unsafe(no_mangle)] / #[unsafe(export_name)] — REQUIRED in edition 2024

"Unnecessary Clone"

Before flagging, you MUST:

1. Confirm the clone is actually avoidable (borrow checker may require it)
2. Check if the value needs to be moved into a closure/thread/task
3. Verify the type isn't Copy (clone on Copy types is a no-op)
4. Check if the clone is in a hot path (test/setup code cloning is fine)

Common false positives:

• Arc::clone(&arc) — this is the recommended explicit clone for Arc
• Clone before tokio::spawn — required for 'static bound
• Clone in test setup — clarity over performance

"Potential Race Condition"

Before flagging, you MUST:

1. Verify the data is actually shared across threads/tasks
2. Check if Mutex, RwLock, or atomic operations protect the access
3. Confirm the type doesn't already guarantee thread safety (e.g., Arc<Mutex<T>>)
4. Check if the "race" is actually benign (e.g., logging, metrics)

Common false positives:

• Arc<Mutex<T>> — already thread-safe
• Tokio channel operations — inherently synchronized
• std::sync::atomic operations — designed for concurrent access

"Performance Issue"

Before flagging, you MUST:

1. Confirm the code runs frequently enough to matter
2. Verify the optimization would have measurable impact
3. Check if the compiler already optimizes this (iterator fusion, inlining)

Do NOT flag:

• Allocations in startup/initialization code
• String formatting in error paths
• Clone in test code
• .collect() on small iterators

Severity Calibration

Critical (Block Merge)

ONLY use for:

• unsafe code with unsound invariants
• SQL injection via string interpolation
• Use-after-free or memory safety violations
• Data races (concurrent mutation without synchronization)
• Panics in production code paths on user input

Major (Should Fix)

Use for:

• Missing error context across module boundaries
• Blocking operations in async runtime
• Mutex guards held across await points
• Missing transaction for multi-statement database writes

Minor (Consider Fixing)

Use for:

• Missing doc comments on public items
• String parameters where &str would work
• Suboptimal iterator patterns
• Missing #[must_use] on functions with important return values

Informational (No Action Required)

Use for:

• Suggestions for newtypes, builder patterns, or type state
• Performance optimizations without measured impact
• Suggestions to add #[non_exhaustive]
• Refactoring ideas for trait design

These are NOT review blockers.

Do NOT Flag At All

• Style preferences where both approaches are valid (e.g., if let vs match for single variant)
• Optimizations with no measurable benefit
• Test code not meeting production standards
• Generated code or macro output
• Clippy lints that the project has intentionally suppressed

Valid Patterns (Do NOT Flag)

Rust

| Pattern | Why It's Valid | |---------|----------------| | unwrap() in tests | Standard test behavior — panics on unexpected errors | | .clone() in test setup | Clarity over performance | | use super::* in test modules | Standard pattern for accessing parent items | | Box<dyn Error> in binaries | Not every app needs custom error types | | String fields in structs | Owned data is correct for struct fields