| name | analysis-review |
| description | Chess-correctness review for ChessBeast analysis changes. Use when a diff touches packages/core/src/classifier, packages/core/src/exploration, packages/core/src/storage, packages/pgn/src, packages/llm/src/explorer, packages/cli/src/orchestrator, or services/. Also use when the user says things like "chess correctness", "check the notation change", "review the classifier change", or "are these plies right". Reads docs/analysis-conventions.md as the source of truth. |
Chess-Correctness Review
This is a review tool, not a rubber stamp. Every rule below was earned by a real defect: double UCI-to-SAN conversion broke candidate building (PR #91), comments landed one move early until the after-move ply convention was unified (PR #95), and isolated engine failures were swallowed instead of surfaced (PR #94). The job is to catch the next one of those before it merges.
Step 1: Load the source of truth
Read docs/analysis-conventions.md in full, including the review checklist at the bottom. The doc is authoritative over this skill: if they disagree, the doc wins. Do not review from memory of the rules; the doc changes as new defects earn new rules.
Step 2: Gather the change under review
If the user named a target (a PR, a branch, specific files), review that. Otherwise review the working state:
git diff origin/main...HEAD
git diff HEAD
If both are empty, or the change touches nothing this doc governs (no notation, ply, classifier, perspective, eval, DAG, or engine-failure code), say so plainly and stop. Do not manufacture findings from out-of-scope files.
Step 3: Walk the review checklist
Work through the checklist at the bottom of docs/analysis-conventions.md in order. Run its mechanical grep commands rather than eyeballing the diff; the commands exist because eyeballing missed the UCI-leak and ply-placement regressions more than once.
For every hit, read the full post-change file around it, not just the diff hunk. A sanToUci( call is only a violation if it is a second notation boundary outside the adapter; a plyIndex - 1 is only a violation if no nearby comment states the after-move basis; a centipawn literal is fine in a test fixture. Context decides, and context lives outside the hunk. When a change touches placement, NAG assignment, or criticality, read the enclosing function end to end.
Step 4: Report
Structure the report exactly as follows, findings first, no praise.
Verdict: one of clean, clean with notes, or violations found.
Violations: for each, give:
file:line- The rule broken, by section number in docs/analysis-conventions.md (e.g. "violates 1.1")
- The concrete harm (a raw UCI string in output, a comment on the wrong ply, a flipped eval sign, a swallowed engine failure)
- The compliant fix, naming an existing primitive:
ChessPosition.convertPvToSan at the adapter, moveWithUci(), the isUciMove guard, the after-move plyIndex convention, RATING_THRESHOLDS in thresholds.ts, the evalToPositionNag/evalToVerbalDescription conventions, or the onWarning path into the runner's warnings.
Risks worth a look: things that are not clear violations but smell like the start of one (an eval consumer that reads a sign without naming its perspective, a catch that surfaces nothing today but sits on a path that will fail).
Confirmed clean: list only the checklist areas you actually exercised (commands run, files read), so the reader knows what was covered versus skipped.
Keep it tight. A three-line report on a clean diff beats a page of hedging.
