Rust Code Review

Rust Code Review

A review is a statement about what a change owes — to the request that asked for it, and to the standards the repo already lives by. This skill runs both statements as separate passes and merges them into one report led by a verdict. It never restates a rule a smell maps to: the craft skills and /rust-testing own the standards, and the review routes to them.

Two axes, run separately

A single reviewer reading for both correctness-against-the-request and craft does neither well: spec drift hides behind clean-looking code, and style nits crowd out the missing edge case. Run a standards pass and a spec pass as separate passes — as parallel sub-agents when the harness supports it, sequentially when it does not — then merge. The prompts for both, the merge rules, and the report shape are in REVIEW-PASSES.md.

The machine goes first

Before either pass reads a line, run the verification step below and read the output. Anything clippy already reports is not a review finding; it is a build failure someone forgot to run. Reviewer attention is for what the tools cannot see: the judgment call, the missing edge case, the scope that crept in.

Read the repo lint configuration before judging style

[lints.clippy] in Cargo.toml, a clippy.toml, a rustfmt.toml, #![deny(...)] in the crate root. A finding that contradicts a level the repo deliberately set is not a finding — it is a proposal, and it says so explicitly. Never re-run at a stricter level than the repo configures and report the extra hits as defects. The smell baseline layers on top of this configuration, never in place of it — a finding that the repo lint config already permits is a finding about the config, raised separately.

The smell baseline

What this skill adds beyond clippy: the judgment calls no lint can make. The full table — the smell, how it shows up in a diff, why it is a defect, and the owning skill — is SMELLS.md. The top ones:

• an unexplained clone at the call site — /ownership-not-clone
• unwrap or expect on a path a caller controls — /rust-errors
• pub on an item with no external caller — /rust-api-design
• a boolean or stringly-typed parameter that should be an enum — /type-driven-design
• blocking work inside an async function — /async-rust
• an unsafe block with no // SAFETY: comment — /unsafe-rust
• a behaviour change with no test that would have caught it — /rust-testing

An #[allow(lint)] that outlived its reason is invisible and permanent. #[expect(lint, reason = "...")] fails once the situation it was written for goes away, which turns a silenced lint into one that reports itself when it becomes stale. In review, an #[allow] with no reason is a finding; with a reason it is a conversation.

Severity, and saying nothing

Three levels only: blocking (wrong behaviour, unsound, breaks the public API without a version bump), should-fix (a real defect the author would want to know about), note (a genuine improvement the author may decline). Anything below that threshold is dropped rather than written down — a review of forty nits and one soundness bug buries the soundness bug. No praise-padding, and no restating the diff back to the author.

Finding format

One line each, most severe first: path:line — severity: what is wrong. What to do instead.

src/cache.rs:41 — blocking: guard held across .await; drop it before the fetch.
src/parse.rs:8 — should-fix: unwrap on caller input; return the ParseError instead.
src/lib.rs:102 — note: this fn takes String where &str would do.

A finding with no concrete fix is a question, and is phrased as one.

What this skill does not do

It does not rewrite the code — findings, not patches, unless the author asks. It does not review a diff it cannot build: a check that cannot run is named in the report, not papered over. It does not decide the parity contract of a port; that is /port-to-rust.

Verification

Run before reporting, and quote the results in the report:

cargo fmt --check
cargo clippy --all-targets --all-features   # at the repo configured level; add -- -D warnings only if there is no lint config
cargo test --all-features

If any of the three cannot run — no network for dependencies, no nightly for a required component — the report says which check did not run rather than implying a clean bill.