code-review

---
name: code-review
description: Review guidance for pull requests in this repository. Use when reviewing a pull request, to keep comments on evidence you can see rather than on predicted build outcomes, remembered API signatures, or assumed conventions.
license: MIT
---

<!-- GENERATED BY cargo-anvil. DO NOT EDIT DIRECTLY. -->

# Code review

This skill applies when reviewing pull requests. It does not apply to the coding
agent when it writes code — see `AGENTS.md` for that.

Every rule below exists because a review comment was filed, argued, and
withdrawn. The cost of a wrong comment is not zero: the author has to reproduce
your claim, disprove it, and write the rebuttal.

## The evidence rule

**Comment only on what you can see in the diff, in a file you have read, or in
this repository's own configuration.** Anything else — what an API's signature
is, what a constant's value is, what compiles, what the local convention is — is
a guess. If you cannot point at the evidence, do not file the comment.

When you do file one, name the evidence: the file and line you read, the config
key you checked, the sibling cases you compared against.

## 1. Never predict build, lint, or test outcomes

Every pull request is validated by CI. Do not claim code "won't compile", "fails
to build", "has a type error", "is missing an import", "won't link", or "will
throw at runtime" — CI is the source of truth, and you are not running it.

This is the single largest source of withdrawn comments. Real examples, all
wrong:

- "`size_of` is not in the prelude" — it is, in edition 2024, and this
  workspace's Clippy rejects the explicit import as redundant.
- "`Future` is not in scope" — also in the 2024 prelude.
- "`Duration::from_mins` does not exist" — it exists on the pinned toolchain,
  and a Clippy lint here *requires* it over `from_secs(15 * 60)`.
- "`LocalKey` has no `set`, use `with(|cell| …)`" — `LocalKey<Cell<T>>` has
  `set`/`get`.
- "`array::from_fn` takes two generic parameters" — it takes three (`T`, `N`,
  and the closure).
- "`self.0.1` is not valid tuple access" — rustc splits a float literal in field
  position back into `.0` and `.1`.
- "splatting a `List[string]` will throw" — PowerShell splatting enumerates
  generic lists.

If you believe there is a real logic, correctness, security, or design problem,
describe *that* without predicting a compiler, linter, or test result.

## 2. Do not assert an API from memory

Signatures, generic arity, trait bounds, and constant values are the things you
are most confident and most often wrong about. Before writing "X does not
exist", "X takes N arguments", or "X equals V":

- Read the definition, or the vendored header, or the crate's docs.
- If you cannot, say what you observed and ask, rather than asserting.

One withdrawn comment built an entire boundary-condition bug report on
`WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH` being `u32::MAX`. It is `0`.

## 3. Check the edition and toolchain before "not in scope"

A missing `use` is not evidence of a missing import. Read `Cargo.toml` for
`edition` and `rust-toolchain.toml` for the pinned version first. The 2024
prelude added `Future`, `IntoFuture`, and the `std::mem` size/align functions,
among others — code that looks under-imported for edition 2015 is usually
correct.

The same applies to MSRV: a constructor you do not recognise may simply be newer
than your training data and older than the pin.

## 4. Convention claims need evidence from this repo

Do not write "this repo prefers X" or "the convention here is Y" from a single
observed file. Before claiming a convention:

1. Check the machine-enforced source first — `clippy.toml`, `rustfmt.toml`,
   lint tables, `AGENTS.md`, `.editorconfig`. If a tool already permits both
   forms, there is no convention to enforce.
2. Count the counter-examples. "Prefer `unwrap` over `expect` in tests" was
   filed against a workspace where 60+ test files use `expect`, and where
   `clippy.toml` sets `allow-unwrap-in-tests = true` precisely to allow both.
3. Never contradict a finding you made earlier in the same review.

If a convention is real but only partly applied, say so and scope the request —
filing it on five of twenty sites leaves the codebase less consistent than
before, not more.

## 5. Do not generalise a local pattern past the policy that governs it

"This entry should also include X, like the others" requires checking *all* the
others. Two withdrawn comments asked for an exception to be added to a list that
was deliberately uniform: a mutation-test group that intentionally never pulls in
a `*_testing` sibling, and a Miri exclusion list that intentionally covers every
`*_macros_impl` crate as a cost policy.

If the surrounding entries are consistent and the new one matches them, the new
one is correct. Changing the convention is a separate pull request.

## 6. Design changes need a concrete failure path

Before asking for a different design — extra defensive layers, different
gating, decoupled delivery — state the specific input or sequence that breaks
the current one. Without it, the request is speculative, and the alternative is
often worse:

- Wrapping a waker call in `catch_unwind` was asked for and declined: it would
  have turned a loud abort into a silent hang.
- A "validate the return type" check in a proc-macro was asked for and declined:
  `Result` is routinely reached through aliases, so the check would reject
  working code.

Prefer asking a question over prescribing a redesign.

## 7. One comment per root cause

- Do not file the same finding twice on the same file, or on both a template and
  the file generated from it. Comment on the source; note that the generated
  copy follows.
- Do not split one root cause across several threads.
- Do not re-file a finding that was already answered in this review.

## 8. Grammar and style: only when unambiguous

Prose nits are the lowest-value comments in a review, and a wrong one is pure
noise. Skip them unless the text is genuinely ambiguous or wrong. In particular,
check for a compound subject before "correcting" verb agreement — "field
enumeration and visitor work still occur" is correct as written.

## What is worth commenting on

In priority order:

1. **Correctness** — logic that produces a wrong result for a describable input.
2. **Security** — trust boundaries, permission scope, injection, unsafe
   invariants.
3. **Concurrency and lifetimes** — races, ordering, aliasing, leaks.
4. **API and contract drift** — documentation that no longer matches behaviour,
   breaking changes not reflected in the version.
5. **Missing coverage for a specific risk** — name the untested branch, not
   "add more tests".

If a finding does not fit one of these, and you cannot point at the evidence
for it, leave it out.