Code Review

Code review

This skill covers what to review and what counts as evidence and as a real problem. The standing instructions of the repository are background that always applies; do not repeat them here.

It does not prescribe how a review is presented: the structure, the wording and any severity labels of comments follow the conventions of the review system that runs the review.

Review flow

1. Name the object and the intent. State what the change claims and what would make it wrong.
2. Walk the catalog family by family, and one dimension at a time inside a family: a single read-through lets the most visible change absorb the attention and hides the rest.
3. Cover the whole catalog for every pull request, whatever it changes: code, documentation, dependencies, build configuration, generated files, release metadata, or repository material. A dimension that does not apply is still accounted for, with the reason it does not apply; nothing is dropped silently.
4. Check the evidence. Recompute what can be recomputed: counts, commands, exit codes, file lists, tree hashes.
5. Name the gaps and the conflicts. Tradeoffs between families, and every question the reviewed material does not answer.
6. State the conclusion, with the artifact behind each problem it raises and an action for each of them.

What settles a dimension

A dimension is settled by an artifact: command output, a file, a test, a hash, or a documented decision. A claim that cannot be checked against an artifact is not verified, and "looks fine" is not evidence.

When the artifact is missing, name the one that would settle the question and treat the question as open rather than deciding it.

Dimension catalog

Nine families, nineteen dimensions. Each dimension states a principle and the artifact that settles it; the concrete form of that artifact is whatever the repository under review documents.

A · Intent

• 1 Scope and intent. The change does only what it claims, and every file it touches belongs to that claim; unrelated reformatting, drive-by refactors, moved files and dependency churn are problems to raise.

B · Correctness

• 2 Behavioural correctness. The observable behaviour matches the modelled API and the reference implementation, including the alternative addressing forms a request may use; name the suite that covers the behaviour, and treat a behaviour change without one as a gap in the evidence.
• 3 Error paths and failure modes. Empty input, missing resources, zero-length payloads, concurrent writers, timeouts, half-closed connections, resets and streams that end early; a silent fallback is a real problem, because reporting success while nothing happened is the worst failure mode.
• 4 Wire format and specification compliance. Status codes, error codes, document shape, timestamp precision and date forms, framing and trailers, percent-encoding and query parsing follow the specification exactly; say which specification or RFC the behaviour is checked against.

C · Compatibility

• 5 Public interface and versioning. Public interfaces and feature combinations keep their promises, generated output is identical after a regeneration, and the compatibility check of the project runs when a published surface changes.
• 6 Toolchain and platform baseline. The toolchain baseline the project declares is respected, and the behaviour that differs per platform is stated for the platforms the project supports.
• 7 Dependency surface structure. The default dependency set is an invariant: a new optional dependency must not appear without its feature, and a development dependency must not pull runtime components into a default build. State a dependency invariant on two bases, the node count and the enabled feature set, because a change can leave the node count identical while the feature set shrinks. An unused direct dependency is a real problem, not a harmless leftover.

D · Security

• 8 Memory and cryptography, transport. Unsafe constructs are absent or justified, and transport security, certificate handling and signature verification use their primitives correctly.
• 9 Input handling and amplification. Work stays linear in the input with no amplification and no unbounded buffers, and untrusted input is validated before it is used.
• 10 Known vulnerabilities. The dependency set is checked for known advisories, and an accepted advisory is recorded with its rationale and its exposure.

E · Performance and resources

• 11 Runtime performance and measurement validity. Measurements use valid input and are reproducible, and the baseline they are compared against is stated; timings from malformed input are robustness data, not design evidence.
• 12 Memory streaming and build resources. Streaming stays bounded in memory with back pressure, and heavy builds respect the build resources of the environment.