improvise/.agents/skills/code-review/SKILL.md

---
name: code-review
description: Review a pull request or diff against this repository's own written architecture and testing doctrine — the Cmd/Effect split, exhaustive enums over AppMode/Axis/CategoryKind, crate layering, the CellKey/layout/persistence invariants, and when a green suite is not evidence. Use when reviewing any change to this repo, and before pushing your own work.
---

# Code review

This repo has already written down what "correct shape" means, in more detail
than most. A review here is mostly checking a change against that, rather than
against generic standards.

Two passes, in order:

1. **Correctness** — does the change do what it claims? Wrong logic, missed
   edge cases, error paths that swallow failures. Ordinary review judgement.
2. **Conformance** — does it fit the design? That is what the rest of this
   file is for, and it is the part a reviewer who has not read `context/`
   will miss.

## Read the sources, don't trust this file

This skill is an index. The rules live in files that change, and restating
them here would let the copy drift:

| Source | What it holds |
|---|---|
| `context/design-principles.md` | The "why" — functional-first, polymorphism over conditionals, correctness by construction, layering, §7 testing doctrine |
| `context/repo-map.md` | The "where", plus **numbered gotchas** — read the ones your diff touches |
| `prolog-sessions/kb.pl` | Executable knowledge: `invariant/2`, `doctrine/2`, `forbidden/2`, `decision/3`, and prior `work_done/3` |
| `AGENTS.md` | Working rules: bug-first tests, `Option`/`Result`/`Box` at the edges, history reconciled only by merge, a new conventional commit by default (amend only an unpushed HEAD, for a small fix), and nothing that needs a force push |
| `.agents/skills/wave/agent-rules.md`, "GitHub" | The reviewer's rule: what to read before your first comment, how to answer each existing review, and what every review you post must state. Read it before reviewing a PR |

`kb.pl` is the highest-value and least-obvious of these. Grep it before
reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`.
A `decision/3` fact often explains why the thing you are about to flag looks
the way it does.

## What to check

**Command/effect split.** `Cmd::execute` takes `&CmdContext` and returns
`Vec<Box<dyn Effect>>`. Mutation happens only in `Effect::apply`. A command
reaching for mutable state is the single most common shape error.

**New effects.** `kind()` has no default, so the compiler asks — but check
the classification is right, not just present (`doctrine(derive_then_verify)`
records naive extraction mis-classifying three effects). Note there is no
`changes_mode()` any more: mode settling is observed after the batch, not
predicted per type (gotcha 31, improvise-toc). Don't reintroduce a static
flag for a conditional behaviour.

**Exhaustive matches.** `AppMode`, `Axis`, `BinOp`, `CategoryKind` and friends
should match exhaustively — a `_ =>` arm is how adding a variant silently
skips a site. When a new variant shares one property with an existing variant
but not another, name both as predicates and route call sites through the one
they mean, rather than testing for the variant
(`doctrine(two_questions_one_enum)`; `Axis::filters_data` /
`is_user_assignable` are the worked example).

**Layering.** `forbidden/2` in `kb.pl` is the authority:
`improvise-core` no UI/IO, `improvise-io` no UI/commands, `improvise-command`
no ratatui/crossterm. A ratatui type in core is the usual violation — the fix
is a semantic value the render layer maps; for appearance that is a `Face` in
`improvise-protocol`, resolved by `draw.rs::face_style` (ADR 004).

**`Option`/`Result`/`Box` at the boundaries.** Wrap at parse/lookup/IO, unwrap
early, keep core logic on concrete values. Heap-container plumbing should be
split from logic (`AGENTS.md`).

**Invariants the type system does not carry.** Check the relevant gotchas
rather than reasoning from first principles — `CellKey` built through its
constructors, `IndexMap` order being load-bearing for persistence, display
rounding never fed back into eval, `app.layout` going stale mid-batch and
needing `RebuildLayout` in front.

**Persistence.** The pest grammar is the source of truth; update it first.
Wire tokens are not UI labels and must not follow them.

## Testing doctrine

From `design-principles.md` §7 and `doctrine/2`:

- **A bug fix needs a test that failed before it.** If the diff claims a fix
  with no such test, that is the finding. Ask whether it was run against the
  old code.
- Prefer unit tests over integration/e2e; test the observable contract, not
  internal storage.
- Suite stays **under 2 seconds**. Proptest stays at 256 cases — constrain the
  strategy instead of raising them.
- `doctrine(check_the_checker)`: a new check should be proven by injecting the
  fault it targets. A check nobody has seen fail is indistinguishable from one
  that always passes.
- `doctrine(populate_before_roundtrip)`: a roundtrip over a default-constructed
  value proves almost nothing.
- `doctrine(binding_dump_diff)`: most keybindings have no direct test, so a
  green suite is **not** evidence for a keymap refactor — diff the dumped
  bindings.

## Verifying

Run the gate **CI runs**, flags and all. The list is
`.agents/skills/wave/agent-rules.md`, "Gates", and it is kept only there. Run
it from that file rather than from memory or from an older copy. A second
copy of that list lived here and drifted: it said `cargo fmt --all --check`,
which reaches only one of this repo's two workspace roots, and it left out
`direnv exec .`, `scripts/checkers.sh` and `scripts/node-test.sh`
(improvise-vjus). Flags matter. Clippy exits 0 on warnings, so without
`-D warnings` a clippy run was green on exactly the input CI is red on.

**`--check-evidence` is additive**, so it answers the comment cap too and both
verdicts print. It did not always: the flag used to *select*, so the flagged
run skipped the cap and said nothing about having skipped it, and three
separate agents hit that in one campaign — a local gate using only the flagged
form went green while CI went red on a three-line doc comment (improvise-ijql).
Run the bare form anyway when you only want the cap: it is 0.3s and needs no
toolchain, where the flagged one needs a compiled workspace.

The knowledge base is checked in CI as code:

```bash
nix shell nixpkgs#trealla --command tpl prolog-sessions/kb.pl \
  -g "(kb_consistent -> halt(0) ; (kb_report, halt(1)))"
```

In a cloud container that needs one step first: `nix shell` resolves nixpkgs
as a GitHub tarball, which is not served there. Run
`scripts/nix-seed-flake-inputs.sh` to seed the locked inputs over git (it does
not modify `flake.lock`), after which the command works. Details and the two
alternatives are in `context/cloud-agents.md`. If you would rather not spend
the time, review `kb.pl` edits by reading and let CI run the check.

## Closing the loop

If the review surfaces a durable fact — a decision, a new invariant, a method
that worked — add it to `prolog-sessions/kb.pl` as `decision/3`,
`invariant/2` or `doctrine/2`. If it changes where something lives or how it
behaves, update `context/repo-map.md`. Those two files and
`context/design-principles.md` are maintained by hand and are the reason this
review is cheap next time.