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.

When reviewing tests, test helpers, or tests in examples/, apply Test review rules.

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.