Imported from ra3orblade/magic-canvas (.claude/skills/chore-code-review/SKILL.md). Install upstream with npx skills add ra3orblade/magic-canvas --skill chore-code-review. Copyright stays with the author.

Code review chore

Generic review advice is not useful here. This project has a small set of invariants that are cheap to violate and expensive to undo, and they are what this review is for. Ordinary code quality matters less than any one of them.

1. Establish the diff

just check          # fmt, clippy -D warnings, tests, doc integrity
just verify-boundary

Review what actually changed. If the repo has no VCS yet, ask what was touched rather than re-reading the whole tree.

2. The invariants — check each against the diff

Read CLAUDE.md first; it is the short form of all of this.

Exactness (D1)

• No f32 or f64 holding authoritative world coordinates. f64 is transient within-frame math only and must never round-trip into storage.
• No epsilon comparisons on Fixed or WorldPoint. Exact == is the point. An abs() < 1e-9 on world geometry is a defect even when it passes.
• Multiplication of raw fixed values goes through i128. A bare i64 * i64 silently wraps and produces a plausible wrong answer.
• Raw fixed-point arithmetic stays inside mc-core::fixed. That module opts out of the cast lints; nothing else should need to.

The anchor pattern (docs/spec-m0.md §2.3)

• Vertices are stroke-local, paired with an exact WorldPoint anchor. Absolute world coordinates in a vertex buffer is the bug this pattern exists to prevent.
• Subtraction happens in fixed-point first, to_f64() only after the magnitude is small. Reversing that order looks identical and silently destroys precision far from the origin — it will pass every test run near 0.
• Per-frame work is one translation per stroke, not per vertex.

Op log authority (§4.2)

• Derived state — spatial index, GPU buffers, LOD — is never mutated without a corresponding op. Doing so breaks undo, sync, and moderation at once.
• Every new op variant carries an author. Attribution is what makes moderation a read-time filter instead of a cleanup job.
• Undo appends an inverse op; it never rewinds a cursor. A cursor works locally and collapses the moment the shared world arrives.
• Procedural strokes store (brush, seed, params) — never expanded geometry. Serialising generated output defeats the premise and inflates hosting cost.

The wasm boundary (§4.6)

• mc-core has not grown wgpu, wasm-bindgen, web-sys or js-sys. just verify-boundary checks this; confirm it ran.
• No per-stroke or per-object call crossing into wasm. One batched slice per frame.
• Logic has not migrated into mc-wasm. It is a shell; anything non-trivial there is a design smell.

Determinism (§5.3, D7)

• Brush expand is a pure function of (path, seed, params). No clock, no global RNG, no thread state.
• Seeded randomness is counter-based on (seed, index) — not a stateful RNG threaded through, which makes output depend on evaluation order.
• Nothing changes an existing terrain generator's output. New behaviour means a new version, never a migration.

Latency (§4.3, §7)

• The live stroke path does not touch settled-stroke buffers.
• Nothing was added to the per-frame path that allocates or blocks.

3. Scope check

• Does the change belong to the current milestone, or has it drifted forward? Scope is the project's most likely failure mode (§8).
• Does it touch a v1 non-goal — text editing, realtime co-editing, mobile optimisation?
• Does it contradict a decision in design.md §9? If it does, that is not automatically wrong — but the decision record must be updated with the reversal, not silently bypassed.

4. Report

Lead with invariant violations; they outrank everything else. For each: the file and line, which invariant, and the concrete failure it produces — "passes near the origin, drifts visibly at 10⁶ units" is useful; "violates D1" alone is not.

Then ordinary correctness, then quality. If nothing violates an invariant, say so plainly rather than manufacturing findings.