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

name, description

name	description
code-review	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 a numbered gotcha digest (the full text is context/gotchas.md) — read the long form of 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:

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.