| name | rust-review |
| description | Rust code-review rubric — the cargo quality gate, a severity-tiered checklist (safety, error handling, ownership, concurrency, performance, API quality), a public-API pass against the Rust API Guidelines, the Approve/Warning/Block verdict, and how to dispatch the craft review agents. Use when reviewing Rust code or a diff, deciding if it's mergeable, or reviewing a public API. Triggers: code review, is this mergeable, dispatch reviewer, public API, what proves done. |
Rust Review
The rubric for reviewing Rust changes: run the mechanical gate first, then read the diff against the severity checklist, then issue a verdict. This is the knowledge; the rust-reviewer agent applies it to an actual diff and reports back.
The review entry point is the rust-review workflow (workflows/rust-review.js): it scout-scales
depth to the diff, fans out the lenses below, grounds findings in tool output, and adversarially
verifies each one. This skill is the rubric the workflow and the rust-reviewer lens worker apply.
When to Use
- Reviewing a diff / PR of Rust code
- Deciding whether changes are safe to commit or merge
- A self-check before opening a PR
Pre-PR gate
Self-review runs before the PR opens, not after. Before gh pr create on a Rust change, run
the rust-review (or generic review) workflow on the branch diff, then close the loop —
triage the findings (triage-findings), fix them (addressing-findings), and re-review until the
verdict is Approve (or Warning with each remaining item justified in the PR body). Only
then open the PR. A one-shot review whose findings never loop back into fixes is not a gate; the
PR isn't ready until the loop is green.
Step 1 — Establish the gate (CI-aware)
The mechanical gate is non-negotiable: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test, and (if installed) cargo audit / cargo deny check must be green before human-style review is worth doing. But before running a check locally, ask whether CI already computed it on this PR; if a conclusive check covers it and is green, consume that result instead of recomputing. Re-running a cold build the PR already ran in CI is slow, sometimes impossible (no toolchain/network), and redundant.
Green does not have to be required. Most repositories have no branch protection — isRequired is then null on every check and gh api …/branches/<b>/protection returns 404 — so a rule that only consumes required checks never fires and always falls through to a local build. Required-ness governs whether a red check blocks the merge upstream; it says nothing about whether a green one is trustworthy. A passing job ran the project's own command on a clean machine with a warm cache; that is better evidence than anything reproducible locally.
Establish each signal:
- Detect the PR's CI for the current branch:
gh pr checks --json name,state,bucket,link
If gh is missing, unauthenticated, offline, or finds no PR → fall straight through to the local gate (never fail on detection).
fmt / clippy / test / build — if a check whose name matches the command (substring: fmt, clippy, test, build/check; match generously — cargo nextest, just clippy, ci / test (stable) all count) is conclusive — required or not:
audit / deny — always run locally if installed, regardless of CI (cheap, usually absent from CI):
cargo audit
cargo deny check
Setting up / interpreting cargo audit + cargo deny → rust-security.
If any fmt/clippy/test/build signal is red (CI or local), stop and report — don't review further. A green gate is the floor, not the ceiling — whatever its provenance, it means the change is reviewable, not that it's good.
Step 1.5 — Resolve dependency context
Review against the crate versions the project actually pins, not against crates-in-the-abstract.
A call that is idiomatic on tokio 1.40 may be deprecated on the 1.20 this repo locks; a default
may have flipped between minor versions. Before judging dependency usage:
- Resolve the real versions —
cargo metadata --format-version 1 (or read Cargo.lock) — and match
the external crates the changed files use to their locked versions.
- For any nontrivial dependency the diff touches, check the usage against that version's API:
a since-deprecated/removed/renamed item, a changed default, a known footgun of that exact version.
Consult live docs (context7, docs.rs for the pinned version) rather than memory — this ecosystem
moves fast. Version-specific misuse is
DEP-001 (Medium).
- Known-vulnerable versions are a separate axis, caught by
cargo audit / RUSTSEC (DEP-002,
High) in Step 1 — don't duplicate them here.
Best-effort: skip if there are no external-crate changes. Choosing/pinning versions and MSRV is the
rust-ecosystem skill's domain; this step only uses the pinned set to ground the review.
Step 2 — Severity checklist
Review the diff against these tiers. This skill owns only the review process; cite the owning skill for each fix. Each item has a stable ID in the rules.md catalog — cite it in a finding (e.g. CON-003) so the finding is addressable and dedup-able; novel issues without a catalog ID are still welcome.
- Safety / injection / secrets / untrusted-input limits →
rust-security
unsafe / missing // SAFETY: → rust-unsafe- Result-vs-panic, typed-error-vs-
anyhow → rust-errors
.clone() / &str-vs-String / lifetimes → rust-ownership- Blocking-in-async / lock-across-
.await / deadlock / Send+Sync → rust-concurrency
- Allocation / hot-path / N+1 →
rust-performance
- Generic code smells / naming / wildcard-match / missing-
/// → rust-idioms
- Structural simplification / file decomposition / spaghetti branching →
refactoring
- Missing tests →
rust-testing
CRITICAL — block on sight
Safety
unwrap() / expect() / panic! / todo! / unreachable! on a reachable production pathunsafe block without a // SAFETY: comment justifying every invariant (SAFETY-comment discipline & UB checklist → rust-unsafe)- SQL/command built by string interpolation of untrusted input (injection)
- User-controlled path used without canonicalize + prefix check (traversal)
- Hardcoded secret / key / token / password in source
- Deserializing untrusted input without size/depth limits
debug_assert! carrying a load-bearing invariant — an unsafe precondition, a bounds/length check, a trust-boundary validation. It compiles out in --release, so the shipped binary runs unguarded; a safety-critical check must be a real assert! (→ rust-unsafe; when debug_assert! is the right call → rust-performance)
Error handling
- Recoverable failure handled with
panic!/unwrap instead of Result
let _ = result; silently dropping a #[must_use] / error value
HIGH — block unless justified
Safety — build-profile divergence
-
Arithmetic on an untrusted-input path whose outcome differs between the dev/test and the shipping profile. overflow-checks is on in dev/test and off in release by default, so the same expression panics under review and wraps silently in production.
Grade against the profile the project actually ships — read [profile.release] in the crate and workspace root before assuming, it may be re-enabled there. The profile-gated panic is the floor of the impact, not the ceiling: a panic that vanishes in release is not thereby harmless. Ask what the release build does instead — a silent wrap that truncates a length, misresolves an index, or corrupts state is worse than the panic precisely because nothing reports it. "Doesn't reproduce in release, moving on" is the reflex that misses it. Report both facets.
Ownership & lifetimes
.clone() added to silence the borrow checker without understanding why- Takes
String where &str/impl AsRef<str> suffices; Vec<T> where &[T] suffices
- Explicit lifetimes where elision applies
Concurrency
- Blocking call (
std::thread::sleep, std::fs, blocking I/O) inside an async fn
- Unbounded channel without justification (prefer bounded back-pressure)
- Lock held across an
.await
- Inconsistent lock acquisition order (deadlock risk)
- Missing
Send/Sync where the type crosses threads
Test coverage (cross-check with the rust-testing skill)
- New error path or branch with no test
- Bug fix landed without a regression test reproducing it
MEDIUM — warn, may merge
Performance
- Allocation inside a hot loop;
to_string()/to_owned() where a borrow works
Vec::new() + push loop where the size is known (with_capacity)- N+1 queries / repeated work in a loop
API & quality
- Library returning
Box<dyn Error> / anyhow::Error instead of a typed error (API-design concern → rust-errors)
- Function > ~50 lines or nesting > 4 levels
- Wildcard
_ => on a business enum (hides new variants — prefer exhaustive)
pub item without a /// doc#[allow(...)] suppressing a lint without a justifying comment- Crate-root
#![deny(warnings)] (or other blanket lint-level attributes) — brittle: a new compiler/clippy lint becomes a hard build break for the crate and its dependents → move the denial to CI (rust-idioms anti-patterns)
Maintainability & structural simplification (fix → refactoring, rust-idioms)
- Missed code judo — a reframing that uses the existing architecture more effectively would make this change dramatically simpler or delete a whole category of complexity (a layer, a branch family). Don't flag hypothetical rewrites; flag a concrete, behavior-preserving restructuring the author could have taken.
- File-size growth — the diff pushes a file across ~700 lines (Rust files are dense; this is the decomposition smell, the per-function ~50-line rule above is separate) → split by responsibility.
- Spaghetti branching — a new ad-hoc conditional / one-off branch / scattered special-case spliced into an unrelated or shared flow, instead of behind a dedicated abstraction (→
refactoring "replace conditional with dispatch").
- Needless optionality / casts — superfluous
Option wrapping where the value always exists, as-casts where From/TryFrom belongs, Box<dyn Any> / downcasting where a typed model fits.
Maintainability bar — presumption of block (strict mode)
By default the maintainability items above are MEDIUM (warn, may merge). When the review is dispatched in strict mode (the rust-review workflow's strict flag, or an explicit "harsh maintainability review" request), they invert to a presumption of block: each is treated as a blocker unless the author has clearly justified it in the diff or brief. In strict mode, Approve additionally requires:
- no structural regression (a previously cohesive module didn't become more coupled or harder to scan);
- no obvious simplification missed (the code-judo check above came up empty);
- no unjustified file-size explosion;
- no spaghetti-growth from special-case branching;
- no hacky or magical abstraction obscuring the design.
Strict mode raises the bar; it does not invent findings — every item still goes through the verification protocol and a refuted item is dropped.
Public-API diffs — the API-design pass
When the diff changes a public API (a library crate, a published pub surface), also run the
condensed Rust API Guidelines checklist → api-design.md. It catches the
library-author concerns the general checklist above doesn't — missing common-trait impls,
Deref-as-inheritance, out-parameters, unsealed extensible traits, leaking unstable deps, absent
CHANGELOG/metadata. Each item points at the craft skill that owns the fix. Skip it for
application-internal code.
Review lenses
The workflow fans the rubric out into independent lenses, each reviewing ONE slice blind to the
others (higher recall than one broad pass):
| Lens | Slice | Owning skill for the fix |
|---|
| safety | injection / secrets / unsafe / untrusted-input limits / build-profile-divergent arithmetic and guards | rust-security, rust-unsafe |
| errors | Result-vs-panic, dropped errors, typed-vs-anyhow | rust-errors |
| ownership | needless clone, &str/&[T], lifetimes | rust-ownership |
| concurrency | blocking-in-async, lock-across-await, deadlock, Send/Sync | rust-concurrency |
| performance | hot-loop allocation, N+1, needless owning | rust-performance |
| api-idioms | typed errors, giant fns, wildcard match, missing docs, #![deny(warnings)] | rust-idioms |
| invariants | domain lifecycle/scope rules, derived/effective quantities, eligibility checks that diverge from an existing sibling gate (a new capacity/permission predicate dropping a fail-closed dimension), and the mirror walk on two-sided contracts (below) | rust-architecture, rust-fintech |
| compat | serialization / persistence / rolling-deploy compatibility — a changed serde/JSONB/wire representation vs data written by other code versions (rename with no alias, alias that only covers new-reads-old, unbackfilled migration) | rust-ecosystem |
| maintainability | structural simplification (code judo), file-size growth, spaghetti branching, needless optionality/casts | refactoring, rust-idioms |
| tests | test quality not just presence; missing regression/error-path tests | rust-testing |
| intent | does the change do what the brief/spec says? | specs |
The workflow runs additional context-dependent lenses the scout selects — api-boundary (error→HTTP-status
mapping, OpenAPI completeness), reconciler (controller idempotency), and negative-space (breakage the diff
enables in unchanged code); workflows/review.js is the authoritative lens set.
Each lens expands context (callers/impls/error paths via rust-navigation) and emits blast-radius
for changed public symbols.
Confidence tiers — surface, don't censor
- Confirmed — located and survived verification; drives the verdict.
- Suspected — borderline or unverified; surfaced for the author, never changes the verdict.
Report everything you suspect. Borderline findings go to Suspected, not the bin.
Tool grounding (seed findings)
Beyond the gate, the workflow runs real tools scoped to the diff and feeds their output in as seed
findings (each still verified): cargo clippy -W clippy::pedantic -W clippy::nursery; for
published libraries cargo semver-checks check-release; and semgrep as a SAST seed — in-repo
./semgrep/ custom rules always when present, plus p/rust/p/secrets when the diff is
security-sensitive (auth, crypto, input parsing, unsafe, FFI, deps). semgrep results are seeds, never
gate failures: taint/secrets over-report, so the downstream verification refutes the false positives
(see rust-security). Optional tools degrade gracefully when absent.
Severity magnitude — measure it, don't inherit it (SAF-009)
"Same class as that other finding" is a claim about mechanism, not about severity. A shared root
cause says nothing about shared magnitude, and the gap can be one or two orders of magnitude —
entirely invisible unless someone does the arithmetic.
For any resource-exhaustion or algorithmic-complexity finding, compute before you label:
- Attack throughput vs a real-data baseline. Measure the same code on representative real
input, not only on the crafted PoC. "8× slower than normal traffic" and "50,000× slower" are
different findings; a decoder still running at 10+ MB/s under attack is not a denial of service.
- Attacker cost per unit of victim cost — bytes (or requests) the attacker must send per second
of victim CPU. This is the metric that ranks two exhaustion bugs against each other; when citing a
prior finding as precedent, compare this number against that one's, not the mechanism.
- State the units. A severity backed by a measurement carries its numbers into the report.
Crashes have no throughput curve — a panic either fires or it doesn't. Rate those by convention
(malformed input causing a panic, no memory corruption, in a parsing library → Medium) and say
explicitly that it is a judgement call, so the label doesn't acquire false precision.
The check cuts both ways: run honestly, it demotes inflated findings and it promotes the ones that
turn out far worse than their class suggests.
The mirror walk — enforcement asymmetry (INV-005)
For a protocol, state machine, codec, or any two-sided contract, bugs cluster where an invariant is
enforced in one place and not at its mirror. The asymmetry itself is the finding — you do not
need a crash, and a fuzzer has no oracle for it.
- Enumerate the invariants. The error enum is the index: every variant names a rule someone
decided to enforce. The spec/RFC and doc comments name the rest.
- Grep every enforcement site for each invariant — the guard, the version check, the bounds or
limit test, the capability predicate.
- Walk the four mirror axes. For each site, where is the mirror and is it guarded the same?
- client ↔ server — the server rejects X; does the client?
- send ↔ receive — the outgoing value is filtered; is the incoming one re-validated?
- offered ↔ accepted — we constrain what we offer; do we constrain what we accept back?
- one-param ↔ all-params — one negotiated parameter is validated; are its siblings (version,
algorithm, limit, scope)? Missing siblings travel in packs.
- Release-diff each candidate —
git diff <last-tag> -- <file>. A guard present in the
release and gone at HEAD is a regression, not a long-standing gap; that changes its severity.
Report: the invariant · enforced-at file:line · missing-mirror-at file:line · which axis · what
the gap lets through (panic, silent drop, downgrade, accepted-but-should-be-rejected).
Proving one — the control/attack differential
These findings have no crash to point at: the defect is that a value is silently discarded or a
message that should be rejected is accepted. Build the oracle as an A/B where exactly one
variable changes:
- CONTROL — the benign arrangement; the value must arrive / the message must be rejected.
- ATTACK — byte-for-byte the same construction with the one variable flipped; the loss or the
wrong-accept must appear.
The oracle is the delta, not a panic. Assert both halves and exit non-zero unless CONTROL holds
and ATTACK reproduces — then the reproducer doubles as the regression test, and a fix that breaks
CONTROL cannot pass by making ATTACK stop firing.
Reach the state through the crate's own test helpers (its tests/common, an internal
*-test crate, a #[cfg(feature = "test-util")] surface) rather than hand-rolling the wire format
or mocking the protocol. A PoC over a mock measures the mock. Then state plainly what the harness
demonstrated versus what is inferred — "acceptance demonstrated at the API; on-path
reachability inferred, not exercised" is an honest and much stronger claim than blurring the two.
Premise grounding — cite it or drop the claim
Most findings rest on a premise that is not visible at the line they cite: "the dependency
rejects this", "this is reachable from untrusted input", "no caller guards it", "the sibling path
does X". That off-site premise is the claim a review most reliably invents — and a second, smarter
reading does not catch it, because it reproduces the same assumption. Only opening the code does.
So every off-site premise is pinned to a file:line you actually opened — dependency sources
included (~/.cargo/registry, the vendored tree) — and reported in the finding's whereChecked.
A premise you did not open is not admissible: open it, or drop the claim and report only what the
cited line itself shows.
This binds rejection exactly as much as confirmation. "A caller already validates this," waved
through without opening the caller, is the same unfounded claim as the finding it dismisses — and
it discards a real bug silently. Uncertain and unable to check? That is Suspected, not a rejection.
An off-site premise nobody could pin costs a finding its Confirmed tier — it drops to Suspected,
never to refuted. Unsupported is not disproven, and burying it as "refuted" would keep a possibly
real defect out of every later round.
Verification protocol
Every finding (lens or seed) is checked before it can be Confirmed:
- Mechanical first: if a tool can decide the finding (a clippy lint, semgrep rule,
cargo-audit advisory), the verifier re-runs it scoped to the cited file and the tool's output
overrides judgement in both directions. Tool-reported seeds can never be refuted on reasoning
alone — only by the tool demonstrably no longer reporting them.
- Adversarial: skeptics try to REFUTE it (default to refuted when uncertain — except
tool-reported seeds, per above). One skeptic by default; three-vote consensus for Critical/High.
- Self-verification (anti-hallucination): re-read the cited
file:line — does the code
actually say what the finding claims? A wrong citation drops the finding. A path that is
test/example-only (not production-reachable) does NOT drop it — it demotes the confirmed
severity one notch.
- Exclusion catalog: a rejection is a claim and carries the same burden as the finding. When
one of the false-positive precedents in fp-rules.md fires, cite its ID
(
refuted per FP-006) — and run the trace that rule demands, not the shape it matches. That file
also lists the KEEP-* non-reasons: dismissals that sound decisive and have repeatedly killed
real defects.
Step 3 — Verdict
| Verdict | When |
|---|
| Approve ✅ | gate green (CI or local), no Confirmed CRITICAL/HIGH/MEDIUM |
| Warning ⚠️ | gate green (CI or local), Confirmed MEDIUM only — Suspected items listed but don't block |
| Block ⛔ | gate red (CI or local), or any Confirmed CRITICAL/HIGH |
In strict mode, the maintainability bar applies: a Confirmed maintainability finding (code judo missed, file-size explosion, spaghetti branching, hacky abstraction) is a presumptive Block unless the author justified it — escalated from its default MEDIUM. Outside strict mode those findings stay MEDIUM (Warning at most).
Report findings as severity · file:line · [rule-id] · what · why · fix. Cite the rules.md catalog ID when the finding maps to one (e.g. CON-003); novel findings need no ID. Be specific and cite the line; a finding without a location isn't actionable.
"Gate green / red" is read from Step 1 — the signal may come from a green CI check or a local run. Cite which in the ## Gate line of the output.
Requesting a review & acting on the verdict
Review early and often — small reviews catch issues while they're cheap; one giant review at the end catches them after they've compounded. Request after a feature or meaningful unit of work, before merging, when stuck, or after a complex bug fix.
Hand the reviewer a crafted brief, not your chat history:
- the diff range —
BASE..HEAD (git merge-base main HEAD and HEAD); the agent derives the exact range itself via git diff --merge-base main, so the SHAs are just for your reference;
- what it should do — the requirement/spec it's meant to satisfy (→
specs);
- what you built — a one-paragraph summary.
In craft, dispatch the agents with that brief:
rust-reviewer — runs the cargo gate + this rubric → Approve/Warning/Block.rust-security-scanner — security-sensitive changes (deps, unsafe, input handling) → rust-security verdict.rust-miri — when the change touches unsafe (→ rust-unsafe).
Acting on the verdict — working a set of findings to green (triage accept/reject/defer/needs-decision, order blocking → simple → complex, fix, verify, re-review, close the loop on GitHub) is its own discipline → addressing-findings; for a large batch it dispatches the triage-findings workflow.
Proving a claim — what proves what
The Rust commands that actually prove each claim:
| Claim | Proof (run it) | Not proof |
|---|
| tests pass | cargo test / cargo nextest run → 0 failed | "should pass", an earlier run |
| doctests pass | cargo test --doc | unit tests passing |
| lint clean | cargo clippy --all-targets -- -D warnings → exit 0 | cargo check passing |
| formatted | cargo fmt --check | "I ran fmt earlier" |
| it builds | cargo build --release → exit 0 | clippy passing |
| bug fixed | re-run the case that reproduced it → passes | code changed, "looks right" |
| a panic is release-unreachable | re-run the repro under the shipping profile and state what release does instead (wrap? truncate? corrupt?) | overflow-checks is off in release |
| a silent-loss / wrong-accept bug is real | a control/attack pair differing in ONE variable: CONTROL correct and ATTACK reproducing (above) | no panic, "the code clearly drops it" |
| a defect is reachable from untrusted input | drive it through the real public entry point on crafted input | reproduced by constructing the state directly (builder/new/test fixture) |
| regression test works | saw it RED before the fix, GREEN after | it's green now |
| no vulns | cargo audit / cargo deny check clean | "deps look fine" |
| coverage target met | cargo llvm-cov --fail-under-lines N | tests pass |
| an agent finished | read the actual diff / its output | the agent said "success" |
| requirements met | check each one against the spec | tests pass |
A green CI check (required or not) for the same command is also valid proof of that command (see Step 1 — Establish the gate). The point of the table is that some fresh authoritative signal exists — CI or local — not that you must re-run it yourself.
State the claim with the evidence, or state the real status with the evidence. An earlier run, a "should pass", or a subagent's self-report is not evidence.
Boundaries
- How to write the missing tests →
rust-testing skill (this rubric only flags that they're missing).
- This skill judges; it does not rewrite. Propose the fix, let the author apply it.
