review-pr skill v2: dispatch gap rule, arch-constant FP fix, P5 timing error

[zufayu]

Summary

Second iteration of the review-pr skill, based on a 191-PR test sweep (88 aiter + 93 ATOM open PRs). Key findings from that sweep drove all rule changes here.

New rules

B6 — New dispatch value not handled by all paths (highest-frequency finding, ~18% of PRs)
When a PR adds a new dtype/arch/flag/getattr attribute key to a multi-way dispatch, every reachable branch must handle it or assert/warn on unsupported combos. Includes FP self-check (upstream assert/isinstance guard → skip).

• Real examples: ATOM#1548 (getattr(self.args,'n_shared_experts',0) silent zero), ATOM#841 (ATOM calls unmerged aiter kwarg → TypeError at dispatch)

C3 — New GPU arch string or arch-specific constant hardcoded in dispatch
Extended beyond arch strings to cover magic constants tied to specific model configs. FP self-check front-loaded: search unchanged lines first; if constant already exists → skip (pre-existing style).

• Real example: ATOM#860 _bind_kv_cache_to_modules() hardcodes 576 for MLA kv_lora_rank + qk_rope_head_dim — breaks any MLA variant where this sum ≠ 576

P5 — Benchmark timing excludes one-time setup cost
Distinct from P1 (missing numbers). Covers cases where setup steps (shuffle_weight, JIT compile) are excluded from the timing window — making a regression look like a speedup.

Improved rules

B5 real examples updated: added ATOM#860 needs_independent_noise dropped in prefill_forward — sampler called without it, first token uses wrong sampling mode for sequences requiring independent Gaussian noise.

C3 FP self-check front-loaded: moved before the trigger condition so agents verify pre-existing style before firing.

Step 3 classification: wired B6/P5 into trigger lines — new constexpr/routing-flag PRs and perf PRs now auto-prompt the relevant checks. New aiter API usage PRs trigger B6 (new kwarg unhandled by all ATOM dispatch branches?).

Step 8 output format: findings now require three parts — Problem + Impact + Action verb ("Author must" / "Reviewer should ask"). Findings without an action verb are not included in output.

Test plan

• 93 ATOM PRs reviewed with prior skill version; all rule additions motivated by real findings
• B6 validated against ATOM#1548 (silent zero fallback) and ATOM#841 (cross-repo sync TypeError)
• C3 validated against ATOM#860 (hardcoded 576 MLA dimension — confirmed by diff analysis)
• P5 validated against real measurement error in aiter#4166 (1.14x claim → 0.83x actual)
• Step 8 three-part format tested in batch 7 (13 PRs, all findings include action verbs in output)

🤖 Generated with Claude Code

[github-actions]

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

• ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

• ✅ Run after the PR is approved and Pre Checkin passes
• ✅ Run immediately when an approval review is submitted
• ✅ Can be requested before approval with labels

Label	Tests
ci:full	Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom	Run native ATOM model accuracy tests
ci:vllm	Run ATOM vLLM OOT model accuracy tests
ci:sglang	Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 1549 --add-label <label>

[zufayu]

Merge button dropdown → "Merge without waiting for requirements to be met". Thanks! @valarLip