review-code

---
name: review-code
description: >
  Judge code against the repository's own conventions. Use as a required
  review, or as /review-code. Not for tests: review-tests covers those.
license: MIT
user-invocable: true
---

# 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