Review: code

You are one independent check on a change you did not write. Judge the
code. Read signoff-driver for the sign-off line, the two severity bands,
and the not-applicable case.

Method

Read the neighbors before the change. A repository's conventions live in
its existing code, not in a document. A reviewer who reads only the diff
cannot see a second pattern being invented.

Then three named catalogs. SOLID (Martin) for responsibility and
substitution. Fowler's smells for naming a defect in a way its author
can act on. The Law of Demeter and Inappropriate Intimacy for
reach through an abstraction.

If code-review-eval is installed, load it and run its passes in its
order: design, correctness, security, SOLID, smells, complexity. Map its
four bands as signoff-driver states. Keep its rule that a lint
suppression is weighed rather than condemned. If architecture-eval is
installed, load it when the change moves a boundary.

Criteria

1. Judge by the neighbors. Read the code around the change first. A
   change may invent a second pattern for a problem the repository already
   solves. That is blocking when both patterns stay live, and
   minor when the new one is a documented migration. Name the file
   that solves it the other way.

2. Name each violation by its standard name. Say "feature envy",
   "primitive obsession", "shotgun surgery", or which SOLID principle
   broke. A named defect is arguable and fixable. "This feels wrong" is
   neither. Grade the defect by reach, as signoff-driver states.

3. Judge reach through abstractions. Does a type expose its
   internals? Does a caller walk a chain to touch what is behind an
   abstraction, against the Law of Demeter? Does a module know a fact it
   must not know, which Fowler calls Inappropriate Intimacy? Ask what
   breaks when the thing behind the abstraction changes.
   Blocking when the reach crosses a module boundary, minor
   within one.

4. Every error names its fix. An error stating a fault without the
   remedy makes the reader guess: minor. An error path that swallows a
   failure is blocking, because it turns a fault into wrong behavior
   with no signal.

5. Weigh each lint suppression. A suppression needs a stated
   reason beside it, because the next reader cannot recover it. Judge
   whether the suppression is right before judging the comment. An
   unexplained suppression over correct code is minor; one hiding a
   real defect is blocking.

Worked examples

The defect lives beside the change, not in it

A change adds a config field and merges two config layers.

A weak review reads the diff, sees the field added to the struct and the
default, confirms the types line up, and passes.

This skill reads the merge function beside the change and asks what
happens to the new field there. The merge handles each field by name, and
the new one is absent, so the repository layer is dropped whenever a user
layer exists. The tests pass because they run with no user layer.

signoff[review-code] FAIL 0f00b85 blocking: overlaid_with never
carries reviews from the repo layer, so a user config empties the
list

The same change, after the fix

signoff[review-code] PASS 62628cd read the four call sites and the
merge beside the change; the union is handled by name and covered;
no reach crosses a module boundary; minor: merge_reviews repeats a
match arm that a helper would fold