Edward Langley
8641f9a59d
The restated list said cargo fmt --all --check, which reaches one of two workspace roots, and omitted direnv exec, checkers.sh and node-test.sh. agent-rules gains two rules from PRs #176 and #177. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
137 lines
6.8 KiB
Markdown
137 lines
6.8 KiB
Markdown
---
|
|
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, merges over rebases |
|
|
|
|
`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 enum in core that the render layer maps (see `AxisColor`).
|
|
|
|
**`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.
|