Appendix R — Code Review Checklist

If the diff vanished after deployment, what evidence would let you reconstruct why the change was safe to merge?

Syntax and green checks would not be enough. You would need the intended behavior, the invariant that constrained the implementation, the ownership and lifetime model, failure and cancellation behavior, performance assumptions, compatibility decision, tests that discriminate the new behavior, and the people who accepted obligations that tools cannot prove.

This checklist follows that causal chain. It is not a demand that every pull request produce thirteen essays. A private rename may close most dimensions with one sentence and existing automation. A public async API with unsafe internals may need API, concurrency, unsafe, performance, and platform specialists plus separate artifacts. The reviewer’s first job is to route the change at its actual risk.

The completed record at examples/rust-engineering-handbook/appendices/release-review-policy-pack/review-record.json applies every required dimension to a relay-service batching change. Its verifier checks completeness and fail-closed disposition; it does not perform code review.

Begin with the change packet, not the patch order

Before reading line by line, require a compact packet:

• user or system outcome and explicit non-goals;
• governing invariant and current behavior;
• affected public, unsafe, concurrent, persistent, protocol, target, build, and operational boundaries;
• alternatives considered and why this design was chosen;
• evidence commands and retained artifacts;
• rollout, observation, rollback or fix-forward plan when runtime behavior changes;
• named owners for findings and specialist claims.

Then restate the change without the author’s vocabulary. If the author says “make flushing faster,” the reviewer might restate: “acknowledge an accepted event only after durable commit while batching up to 64 records or 10 ms, and resolve every accepted record during cancellation.” That sentence exposes correctness, time, capacity, ownership, and cancellation questions that “faster flush” hides.

Trace review from the invariant outward:

intent
  └─> invariant and legal states
        └─> ownership + lifetime boundaries
              └─> success, error, panic, cancellation, drop
                    └─> concurrency + unsafe obligations
                          └─> cost and resource assumptions
                                └─> tests + documentation + compatibility
                                      └─> disposition, owners, rollout evidence

Do not start with formatting or local cleverness while the governing claim is still ambiguous. Tooling can remove mechanical noise before human review. Human attention belongs first at contract boundaries.

Correctness: require a falsifiable claim

Ask what must be true before and after every changed operation. Identify inputs, outputs, side effects, legal state transitions, ordering, idempotency, precision, and partial completion. For parsers, include malformed and adversarial structure. For persistent work, identify the commit point. For time-dependent work, distinguish a deadline from an elapsed-time guess.

Observable review questions:

• Is the requested behavior stated precisely enough to write a counterexample?
• Are boundary values, empty states, duplicates, reordering, retries, and partial effects defined?
• Does the code preserve behavior on every early return and operator branch?
• Are numerical conversions, overflow modes, units, encodings, and platform widths explicit?
• Does the implementation solve the named problem without adding unrelated behavior?

Reject a proof consisting only of examples that the author selected after implementation. At least one test or witness should distinguish the new implementation from a plausible wrong one.

Invariants: connect every mutation to a legal state

An invariant names what remains true across public calls, internal transitions, failures, and cleanup. Review where it is established, which operations assume it, and which mutations can temporarily violate it. Temporary invalid state needs a containment boundary that survives error and panic paths.

For a batching service, a useful invariant is not “the queue works.” It is: every accepted event is in exactly one of pending, durably committed, acknowledged, or terminally failed; no event is acknowledged before commit; shutdown resolves every accepted event once.

Ask:

• Can the type system eliminate an illegal state or transition?
• If runtime state remains necessary, is there one authoritative state machine?
• Do serialization, restart, cache rebuild, and migration re-establish the invariant?
• Can callbacks, reentrancy, cancellation, or panic observe a partially mutated value?
• Are comments and tests attached to the invariant rather than to incidental representation?

A large validation function at one entrance is not proof if other constructors, deserializers, setters, FFI calls, or recovery paths bypass it.

Ownership: draw who is responsible for progress and cleanup

Review moves, borrows, clones, shared ownership, resource handles, and destruction as architecture. Name the owner of each long-lived resource and the event that ends ownership.

• Does the callee need to retain the value, or would a borrow express the real boundary?
• Does a clone represent an intentional snapshot, retry copy, or ownership split, or does it hide an unclear lifecycle?
• Can Arc ownership keep work, files, sockets, tasks, callbacks, or caches alive after their system scope ends?
• Is mutable state partitioned by invariant, or wrapped in one broad lock for convenience?
• Do Drop, explicit shutdown, and error cleanup have distinct responsibilities?
• Can a cycle or detached handle prevent cleanup indefinitely?

“The borrow checker accepts it” proves that the encoded ownership rules are internally consistent. It does not prove the encoded lifecycle matches the product.

Lifetime coupling: reject both accidental ties and artificial escape

Lifetime parameters should express required validity relationships. Review what each returned or stored reference borrows from, whether multiple inputs are coupled unnecessarily, and whether a generic or callback bound quantifies the intended relationship.

Common warning signs include:

• one lifetime parameter applied to independent inputs, shortening valid use;
• a returned view whose owner can mutate, reallocate, or disappear unexpectedly;
• adding 'static to make a spawn or storage error disappear;
• converting a local borrow problem into global Arc ownership;
• self-referential or pinned designs where stable handles, indices, owned data, or a different boundary would be simpler;
• callbacks permitted to retain a borrow that should last only for one invocation.

Require a caller-shaped example. A signature can compile in its defining crate while imposing unusable coupling on downstream code.

Errors: preserve decision information

Review errors as caller and operator contracts. Each failure should say what failed, retain an appropriate source, avoid secrets, and expose classifications needed for retry, user correction, rollback, or escalation without parsing display text.

Ask:

• Is absence distinct from failure?
• Can callers distinguish transient, permanent, conflict, invalid-input, authorization, capacity, cancellation, and internal failures where behavior differs?
• Does conversion retain useful context without freezing unstable dependency types into a public API?
• Are errors bounded and redacted when inputs are untrusted?
• Do retryable errors identify idempotency and partial-effect conditions?
• Do binaries map errors to stable exit, protocol, and operator behavior?

Opaque reports may be appropriate at an application edge; stable typed errors may be necessary at a library boundary. Choose by consumer action, not fashion.

Panic safety: review the unwind and abort stories

Safe Rust can panic. Determine whether the profile unwinds or aborts, which resources and invariants are affected, and whether a panic may cross an FFI boundary, poison shared state, strand an external transaction, double-apply an effect, or trigger another panic during destruction.

Inspect:

• indexing, arithmetic, unwrap/expect, assertions, allocation, formatting, user callbacks, and destructor code;
• partial mutation before a fallible or panicking call;
• guards that restore or finalize state during unwinding;
• lock poisoning policy and whether recovery can actually re-establish the invariant;
• destructor work that blocks, invokes user code, or assumes runtime availability;
• containment at thread, task, process, and foreign boundaries.

A catch_unwind boundary is not a universal repair. It catches only unwinding panics under its documented conditions and does not make damaged external state trustworthy.

Concurrency: prove safety, liveness, and capacity separately

Rust’s type and auto-trait checks address important memory-safety boundaries; they do not prove absence of deadlock, starvation, lost wakeups, race conditions, unbounded queues, unfairness, or overload collapse.

• Which values cross threads, and why are Send and Sync obligations satisfied?
• Which data and invariant does each lock protect? Is the lock order explicit?
• Can a lock remain held across I/O, callback, blocking operation, .await, or another lock acquisition?
• Are condition-variable predicates checked in a loop and changed under the associated lock?
• Do atomic orderings establish the required happens-before relation, or merely atomicity?
• Are queues, workers, inflight operations, retries, and memory bounded under worst supported load?
• How do threads, channels, and producers stop, disconnect, join, and expose saturation?

Lock-free or atomic code needs a specialist when correctness depends on memory ordering or progress guarantees. A microbenchmark is not a memory-model proof.

Async cancellation: inspect every suspension and spawned lifetime

For each .await, determine what state and resources remain live, what happens if the future is dropped, and whether an external operation continues. For each spawn, name the parent lifecycle, retained handle, cancellation signal, join behavior, and panic propagation.

Ask:

• Where are commit points, and can cancellation occur on either side?
• Does timing out the waiter cancel underlying work or merely stop waiting?
• Can a losing branch of a race keep running or retain a resource?
• Is retry safe after cancellation or partial I/O?
• Are lock guards, borrowed values, and large buffers retained across suspension deliberately?
• Does shutdown stop intake, signal children, drain or reject accepted work, enforce a deadline, abort if policy permits, and join every owned task?
• Are runtime-specific scheduling, Send, timer, and I/O assumptions documented?

Cancellation tests need deterministic coordination. Sleeps are weak synchronization and make both false passes and intermittent failures likely.

Unsafe obligations: review the safe caller’s proof boundary

Inventory every new or affected unsafe block, unsafe function, unsafe trait or implementation, FFI call, representation attribute, and safe abstraction whose invariant changed. A small safe-looking diff can invalidate an old unsafe proof by changing layout, aliasing, initialization, drop, auto traits, callbacks, or concurrency.

For each unsafe operation record:

“No unsafe lines changed” closes nothing when representation or callers changed. Conversely, demand no invented unsafe analysis for a change whose inventory and enclosing assumptions are demonstrably unaffected; record that reasoning as not applicable.

Performance assumptions: make the cost claim measurable

Identify allocation, copying/cloning, indirection, dynamic checks, locks/atomics, queueing, syscalls, formatting, cache behavior, code size, compilation, and target effects relevant to the change. Then separate correctness bounds from optimization hypotheses.

• What workload and target make the cost material?
• What baseline, profile, metric, sample units, variance, and guardrails support the claim?
• Does the change move cost into memory, tail latency, startup, shutdown, binary size, compilation, or another tenant?
• Can batching, caching, pooling, or concurrency violate freshness, fairness, capacity, or cancellation contracts?
• Is the regression threshold tied to user or capacity consequences?
• Does the rollout expose the same signals used in the experiment?

Require Appendix O’s experiment record for a consequential optimization. Do not block an ordinary clear change on decorative benchmarking, and do not accept “zero cost” without a defined equivalent and evidence.

Tests: map evidence to the contract

Review what each test proves and which failure would make it fail. Use the narrowest suitable layer: unit tests for local transitions, integration tests for public behavior, doctests for documented use, UI/compile-fail tests for rejected programs, property/model tests for state spaces, and specialized tools for their named classes.

The evidence set should include relevant boundaries, negative paths, feature and target configurations, MSRV/current toolchains, cancellation or panic injection, concurrency schedules, and regression witnesses. Avoid tests coupled so tightly to implementation that a correct refactor becomes impossible.

Green tests show that selected observations held under selected conditions. They do not prove untested behavior, absence of races, unsafe soundness, representative performance, platform support, or compatibility with unknown consumers. State residual risk.

Documentation: make invisible contracts reviewable

Public documentation should identify ownership, errors, panic conditions, safety requirements, blocking, allocation or complexity when important, cancellation, feature/target availability, MSRV, and complete examples. Internal documentation should name invariants, representation choices, lock/atomic rules, and why unsafe preconditions hold.

Operational changes update configuration references, dashboards, alerts, runbooks, rollout and rollback steps, known limits, and incident queries. A code comment is not a replacement for operator documentation, and a pull-request description will not follow a public API into a downstream editor.

Check that documentation describes the accepted implementation, not the proposal’s earlier shape. Run doctests and link checks where applicable.

Compatibility: identify every permitted user of the old contract

Apply Appendix Q’s four rails. Review source compatibility, trait and inference space, features/defaults, public dependency types, MSRV, targets, documented behavior, serialized data, protocols, configuration, generated code, build inputs, and operational transitions.

• Can a downstream crate’s existing implementation or exhaustive match conflict?
• Did an input bound become stronger or an output guarantee weaker?
• Did Send, Sync, panic, cancellation, allocation, or blocking behavior change?
• Does a new default feature or dependency change builds, licenses, targets, MSRV, or behavior?
• Can old and new application versions share state and traffic during rollout?
• Can the old artifact safely roll back after the new one writes state?
• Is deprecation usable, documented, tested, and long enough under policy?

Classify only after compiling credible downstream witnesses and exercising mixed-version operations where relevant. An API-diff tool inventories changes; organizational policy decides compatibility.

Worked review: the batching diff is not ready

The example change batches accepted relay events and acknowledges them after a durable flush. The intended invariant is strong: an accepted event is acknowledged only after commit; cancellation resolves every accepted event; no flush task outlives the service.

The implementation owns batches in one task, uses a bounded channel, and preserves borrowed configuration within service scope. Error classification and state-transition tests are present. Those are meaningful positives.

Review still finds three release-relevant failures:

1. a timeout drops the waiter but leaves a spawned flush task running, violating the lifecycle and cancellation contract;
2. public docs omit the acknowledgement commit point, drop behavior, and retry implications;
3. the claimed batching improvement has no representative baseline, variance, overload guardrail, or regression threshold.

Panic injection is also missing around the commit guard, and the changed default lacks an operator migration and rollback note. The review disposition is request-changes, not an average score. One blocker cannot be canceled by nine green dimensions.

The required repair owns the flush task under the service scope, propagates cancellation, resolves accepted records, and joins before shutdown completes. Deterministic tests must prove cancellation before commit, after commit, during flush error, and during injected panic. Documentation and performance evidence then receive their own reviewers.

review-record.json retains all thirteen dimensions, findings, owners, deadlines, exit conditions, specialist routes, and disposition. Its Node verifier ensures a blocker cannot coexist with approval. It cannot determine whether the repaired task tree or durability claim is correct.

Route claims to the reviewer who can accept them

The author may supply evidence and answer questions. The author does not become independent merely by listing a second role in the pull request. When the required reviewer is unavailable, wait, narrow the change, or redesign it to remove the obligation. Emergency paths need predeclared authority, containment, and mandatory follow-up; urgency does not prove a claim.

Findings and disposition

Write findings so another engineer can close them:

• stable ID and exact affected contract;
• blocker, major, minor, or editorial severity;
• evidence showing the problem;
• consequence if unresolved;
• owner and deadline;
• exit condition and required re-reviewer.

Use approve, approve-with-follow-up, request-changes, or reject only as your organization defines them. A blocker or major correctness, safety, compatibility, or operational gap requires changes before merge. A minor follow-up needs scope, owner, date, and reason it is safe to defer. Style preferences should not masquerade as correctness findings.

Separate merge from rollout. A change may be mergeable behind an inert feature and still unfit for activation. Record build, migration, canary, observation, rollback/fix-forward, and communication gates against the artifact that will run.

Exercise: review the small cancellation optimization

Type: audit and design. Level: review board.

A pull request replaces per-request writes with a background batcher. It adds 70 lines, one channel, a spawned task, a 20 ms timer, and an Arc<Mutex<Vec<Request>>>. Unit tests pass. The author reports 30% higher throughput from one local run. Dropping the request future removes its response sender but not the queued request. The public API documents neither cancellation nor the acknowledgement point. The service supports rolling deployment and retries timed-out requests.

Produce a review record that:

1. states the accepted-request and commit invariant;
2. traces ownership from admission through queue, batch, storage, response, cancellation, shutdown, and panic;
3. determines whether a timed-out request may still commit and whether retry can duplicate it;
4. assesses bounds, lock scope, task lifetime, fairness, timer behavior, and overload;
5. replaces the performance claim with a reproducible experiment plan and guardrails;
6. specifies deterministic tests, public and operator documentation, mixed-version behavior, and rollback constraints;
7. routes specialist claims and issues findings with owners and exit evidence;
8. chooses a disposition without averaging away a blocker.

Assume durable storage offers an atomic batch commit but no per-record cancellation once commit begins; do not assume a particular async runtime. Use the release-review-policy-pack review record as the repository stage and preserve its thirteen dimension IDs. Evaluation requires a coherent invariant, lifecycle trace, measurable evidence plan, explicit residual risk, and findings another engineer can close.

Multiple designs can pass: cancellation may revoke uncommitted requests, or the API may define durable continuation after caller cancellation with an idempotency/result-query contract. A strong review rejects ambiguity, not necessarily background batching.

Printable review card

Before approving, be able to answer:

• What outcome changes, what is excluded, and what invariant governs the change?
• Which states are legal, and do errors, panic, cancellation, drop, restart, and rollback preserve them?
• Who owns every value, resource, lock, task, callback, and cleanup action, and when does that ownership end?
• Do lifetimes express real coupling without artificial 'static, cloning, boxing, or global shared ownership?
• Can callers and operators act on failures without parsing prose or leaking sensitive data?
• Are concurrency safety, liveness, capacity, fairness, and shutdown proved separately?
• Does every async child remain attached to a lifecycle, and is losing or timed-out work resolved?
• Did any safe-looking change invalidate an unsafe, FFI, representation, or auto-trait assumption?
• Are performance statements hypotheses backed by representative evidence and correctness guardrails?
• Does each test discriminate a plausible defect at the right layer and configuration?
• Do public, internal, and operational docs state the accepted contract?
• Which downstream source, behavior, MSRV, feature, target, state, protocol, or recovery path can change?
• Are findings owned, time-bounded, independently re-reviewed, and reflected in merge and rollout dispositions?

Unknown is a review result, not permission. Record the missing evidence and route it.

That routing depends on shared language. If reviewers use crate for a package, task for a thread, lifetime for elapsed time, or safe for operationally correct, the apparent agreement is not yet a proof. Resolve the terminology boundary before closing the finding.

Sources and version notes

• Rust API Guidelines provides a community-maintained checklist for idiomatic and interoperable public APIs; product contracts and current compiler evidence remain authoritative.
• Cargo SemVer Compatibility documents Rust-specific source-compatibility hazards, including additive-looking changes. Apply the project’s published policy and downstream witnesses.
• The Rust Reference: behavior considered undefined, the unsafe keyword documentation, and The Rustonomicon support unsafe review. Tools do not replace a safety argument.
• Send, Sync, the atomic ordering documentation, and Future::poll define key standard-library contracts. Runtime cancellation and task behavior must be sourced to the selected runtime and version.
• Rustdoc’s documentation tests, Cargo’s test command, and Clippy usage document verification mechanisms and current options. Passing them is evidence for their exercised scope.
• The Cargo Book: features, Rust version, and resolver support feature, MSRV, and dependency review.

The worked record is a teaching artifact dated 2026-07-13. Its runtime behavior is illustrative and not evidence for a particular async runtime. Recheck official Rust/Cargo documentation, pinned runtime behavior, targets, dependencies, and organizational review authority before applying the record to a live change.