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

[zufayu]

@junhaha666 heads-up — I synced the general v3 hardening from the aiter review-pr skill into this one, so the new commit dismissed your earlier approval. Could you re-approve?

What was ported (general, non-aiter-specific):

• cross-file verification (grep .cu+.cuh+.h, not just diff files, before claiming missing sync/branch)
• CI-failure triage (infra/unrelated vs real — e.g. an RTD docs failure on a .claude-only change is infra)
• [verified]/[inferred] finding discipline (never ship an unconfirmed root cause)

The aiter-only rules (E4 downstream-CI / E5 owner-signoff) were intentionally NOT ported since ATOM is the downstream. The docs/readthedocs.com check is the usual repo-wide baseline failure (same as recently merged ATOM PRs), so this can be admin-merged once approved. Thanks!

[zufayu]

@valarLip this skill-sync PR is ready for an admin-merge when you have a moment.

• junhaha666 has approved.
• Substance is green: ATOM vLLM Test and ATOM SGLang Test both pass; Accuracy is correctly skipped (this is a .claude/skills/**-only change).
• The two red checks are the usual repo-wide baselines, not this PR: docs/readthedocs.com (fails on every PR) and the Check Pre Checkin Signal gate ("Wait for Pre Checkin workflow" — same flaky gate pattern). Same situation as the recent ATOM PRs that were admin-merged (e.g. [Frontend] Abort engine request on client disconnect (free leaked KV) #1562, [Bugfix] DeepSeek-V4: fix sglang/vllm plugin SWA after paged-SWA migration (#1417/#1423) #1573).

Could you squash + admin-merge? Thanks!

[zufayu]

@valarLip Same as aiter#4171: this PR is stuck on the required Accuracy (...) checks, which stay pending forever. It only changes a .md file, excluded by paths-ignore: **/*.md in atom-sglang-test.yaml / atom-test.yaml, so those workflows never trigger and the required checks never report → BLOCKED. A required-check + paths-filter deadlock a doc-only PR cannot satisfy on its own. Could you bypass-merge it?