review-pr

name	review-pr
description	Advisory AI code review for aiter and FlyDSL PRs. Catches perf regressions, silent correctness bugs, dispatch gate holes, and AI-generated code patterns, but never acts as a merge gate. Invoke with a PR number (optionally owner/repo#N) and, when one exists, a validation report path. Step 1 triages whether the PR changes runtime surface at all and, when it does and the PR ships a single test target, runs validate-kernel-pr itself; a PR with no runtime surface is reported N/A rather than unvalidated. That run also times the target on base and head back to back on one locked GPU, so a kernel PR's latency is measured rather than assumed. The review line stays advisory; deterministic correctness and perf results are judged only from a head-matched report.
argument-hint	<PR number> [owner/repo] [validation-report]

aiter PR Review — advisory tier

This skill supplies hints to a human reviewer. Its judgement is stochastic and never blocks a merge. Only a reproducible blocker from an explicitly supplied, head-matched validation_report.json may be used as a deterministic gate.

Promotion bar

The two conditions under which this could stop being advisory, and why neither holds yet: rules.md § Promotion bar.

---

Step 1 — Fetch

# Everything Step 1 does is in fetch.sh; read its output, then keep the printed $WORK.
"$(git rev-parse --show-toplevel)/.claude/skills/review-pr/fetch.sh" "$@"

Read the diff and PR body before proceeding.

Step 1b — Derive the applicable rules, and collect the evidence they need

Step 1b writes its artifacts into $WORK. Read them; each explains its own output, so what follows is the map, not the manual.

file	what it answers	the trap it exists for
rules_expanded.txt	the full text of exactly the rules this diff derives	reading all 51 means attending to none
applies.txt	whether the diff still applies to the merge target	a stale PR's CI result describes a tree that moved
merge_target.txt	where the base tree is checked out — read base files from there	grepping the local worktree answers about the wrong branch
guards.txt	each deleted assert/check: moved, returned changed, or gone	"it came back" and "it was weakened" look identical
siblings.txt	a variant of a changed function still carrying a changed line	A1's sibling is in the same file, not another file
flydsl_bounds.txt	FlyDSL buffer/descriptor bounds not tied to the tensor's real extent	B2 is tl.load without a mask and FlyDSL has no tl, so this class had no rule at all
aot_pairing.txt	new ops-side contracts aiter/aot/flydsl/ was not taught	#4397 wired stage2 into AOT and missed stage1; it drifted for 32 commits
symbols.txt	first-party imports that do not resolve against the merge target	a rebase signal, not invented code — #4994's import was valid when written
twins.txt	which existing file each new file was copied from	the defect is the asymmetry between them, not the copy
test_quality.txt	assertion count, tolerances, shapes of added tests	zero assertions may mean a helper asserts — read before firing
kernel_tests.txt	new kernels for which no test pytest collects was added	a benchmark is the shape these ship instead of a test
ci_coverage.txt	whether a CI job will ever run the added tests	HK6 is satisfied by a file in a directory nothing scans
perf_claims.txt	every number claimed, and which name no baseline	a signed delta and a before | after table already carry theirs
struct_abi.txt	structs whose pinned layout this diff shifts	the assertions exist to force a code-object rebuild
comment_only.txt	the non-prose lines of a comment-dominated diff	7963 lines that reduce to none (aiter#4062)
evidence.txt	how a removed guard is handled on head — only written when a guard or signature changed	prose telling you to grep was read and not acted on (#5143)

A SKIPPED: artifact means that axis was not checked — say so rather than reading silence as clean. A forensic that ran and found nothing says so in words.

Read $WORK/rules_expanded.txt — it is the full text of exactly the rules in $WORK/rules.txt, and it is the rule list for this review. They are derived from paths, added/deleted lines and the title, and the derivation is conservative — a family it cannot decide structurally is included, never dropped. Over 597 open PRs every one matched at least one family, and no family fired on more than half of them.

Cross-file verification — $WORK/evidence.txt already holds it; read that file before writing any finding about a removed guard or a changed signature. The diff shows changed lines, not the whole story, and prose telling you to go and grep is not enough: it was in this skill already and was read and not acted on, producing a q_out is not None finding on aiter#5143 that was withdrawn once head was read (q_out is std::optional on both sides, every call site is has_value() ? data_ptr() : nullptr, and the kernel guards if(is_q && q_out != nullptr)). The collector puts those three lines in front of you. Where it produced nothing, grep the entire symbol family yourself — .cu + .cuh + .h together, since sync/fence/atomics or the other half of a scatter often live in the header (aiter#3802: a "kernel has no sync" finding was false, the barrier was in the .cuh; aiter#4098: "compares raw uint8 vs float" was false, the reader had a conditional maybe_view_fp8() the diff never showed).

Classify every CI failure before blaming the PR. A red check is not automatically the PR's fault:

• Read the failed step. check-signal / "Wait for Checks" timeouts, "Expected exactly one wheel artifact", and dep-resolver noise are infra flakes, not code failures (aiter#3593, #4171).
• Compare against main: if main fails the same shard in the same window, it's baseline/flaky, not a regression introduced here.
• Expired logs (HTTP 410 Gone) on old runs mean the failure is months-stale and meaningless against today's main — ask for a rebase + fresh run instead of quoting it (aiter#2565).

---

Step 2 — Semantic Understanding (answer all 5 before rules)

Work through these by reading the diff, not the description alone, and write the answers into $WORK/answers.txt, one line each, Q1: … Q5: — Step 8 will not let you write a card without them. Inline _Answer:_ blanks left nothing behind: a review that skipped Step 2 was textually identical to one that did it, which is the same hole D9 had before its scan moved into Step 1.

Q1 — What specifically changed computationally? Not "improves perf" — what algorithm/formula/data flow changed? Answer:

Q2 — Hardware scope: which arch(es), precision(s), execution phase(s)? gfx942 / gfx950 / gfx1250? fp16/bf16/fp8? decode / prefill / both? Answer:

Q3 — Does this change any public aiter API? New symbol in aiter/ops/*.py, new kwarg on existing op, change to aiter/__init__.py? Answer:

Q4 — Performance claim: what is the mechanism? Not "faster" — WHY is it faster? (fewer memory round-trips, fewer kernel launches, better tiling?) Answer:

Q5 — Does the description explain WHY or only WHAT? "Fuses kernels for speedup" = surface. "Eliminates intermediate HBM write between rmsnorm and quant" = understanding. If surface-level only → treat as elevated AI-code risk. Answer:

---

Step 3 — PR Type Classification

Step 1b already derived this. Read $WORK/rules.txt and work only those families; $WORK/rules_expanded.txt holds their text. Do not classify by hand — the failure mode was self-application: a model asked to tick 20 types over 44 rules attends to none of them.

The family → rule mapping lives in MAPPING.md, generated by triage.py mapping so it cannot drift from the deriver. The hand-written copy that used to sit here had: it claimed D9 was derived (D9 is scanner-backed and deliberately is not) and omitted 21 rules that are, including every Triton rule. tests/ fails if the committed mapping and the code disagree.

Measured over 600 open aiter PRs: median 15 rules against a 49-rule set (31%), every PR matched at least one family, no family fired on more than half, and the expensive full Step 4 assessment fires on 6% rather than on everything touching aiter/ops/.

---

Step 4 — Core File Risk Assessment

Write one line per backbone file this diff touches into $WORK/core_files.txt — Step 8 gates on it. This step was prose only, so a review that skipped it was textually identical to one that performed it — the hole Step 2 had before answers.txt. Format:

<path> TIER1|TIER2|TIER3 COVERED|GAP|N/A -- <reason naming what THIS PR changed>
aiter/fused_moe.py TIER2 COVERED -- num_local_experts is threaded through to moe_sorting_fwd and op_tests/test_moe.py adds a DSv3 TP=8 decode case for it
aiter/__init__.py TIER1 GAP -- the new `from .ops.gemm_op_a4w4 import *` sits above the rest of the block, so an ImportError inside it truncates the namespace silently

COVERED = the blast radius below is exercised by this PR's tests or is unreachable from the change; GAP = it is not, and that goes on the card as a finding; N/A = the change cannot reach it at all (comment, docstring). Touching no Tier 1/2 file: write one NONE -- <reason> line naming the Tier 3 files it does touch.

The gate rejects: a backbone file with no line (UNASSESSED); a tier recorded below the table's (TIER-MISMATCH — downgrading is not a way past the checks a tier requires); a formulaic or sub-30-character reason (NO-EVIDENCE); a reason naming no file or symbol this PR changes (UNANCHORED — "core file, large blast radius" is equally true of every PR ever opened against that file); a line about a file the diff does not contain (UNTOUCHED-FILE); NONE declared while a backbone file is present (UNDECLARED-CORE). A missing core_files.txt is a hard failure.

What makes a file "backbone"? Apply these three questions to any file in the diff — including new files not in the table below.

Q1 — If this file has a syntax error or fails to import, does `import aiter`
     still succeed?  → NO → Tier 1 (system-critical: aiter itself breaks)
Q2 — Does it hold the Python dispatch that selects which kernel runs for an op
     class, AND is that op used by >1 production model family (DSv3, Kimi…)?
     → YES → Tier 2 (op-class critical: wrong result for ALL users of that op)
Q3 — Is it the public aiter API for an op (`from aiter import X` lands here)?
     → YES → Tier 2 (signature change silently breaks all consumers)
Otherwise → Tier 3 (individual kernel or model-specific code).

The table is the snapshot the gate demands a line for; use Q1/Q2/Q3 on new files and add a line for any you judge Tier 1 or 2. A header 10+ TUs include is Tier 2 too, counted from the tree, not listed — rules.md § Tiering. Ranked by commit frequency, blast radius:

Tier	File	Git commits	Blast radius	Failure mode
1	aiter/jit/core.py	182	ALL ops — JIT compilation engine	Any import of aiter fails; zero ops load
1	aiter/__init__.py	52	ALL vLLM/SGLang/ATOM users	ImportError or silent namespace truncation below broken import
2	aiter/fused_moe.py	119	All MoE models (DeepSeek, Kimi, MiniMax)	Wrong expert routing, silent accuracy drop
2	aiter/ops/mha.py	89	All MHA attention paths	Wrong attention output, crash
2	aiter/ops/attention.py	66	MLA/paged attention dispatch	Wrong KV, accuracy drop
2	aiter/ops/gemm_op_a8w8.py	59	All FP8 quantized GEMM	Wrong matmul result, silent accuracy drop
2	aiter/mla.py	57	All MLA decode/prefill (DSv3/Kimi)	Wrong KV, accuracy drop, crash
2	aiter/tuned_gemm.py	52	All GEMM-backed ops	assert False crash or silent fallback to slow path
2	aiter/ops/moe_op.py	51	MoE op dispatch table	Wrong dispatch, wrong expert weights
2	aiter/ops/quant.py	49	All quantization paths	Wrong scale, silent accuracy drop
3	aiter/ops/*.py (a single op's wrapper), individual kernel .py/.cu	varies	Consumers of that one op	AttributeError at call time in downstream

Why aiter/ops/*.py is Tier 3 and not Tier 1, and what happens to the assessment if it is not: rules.md § Tiering.

aiter/__init__.py special rule: The import block must NOT be wrapped in try/except. Any new import added here → check the imported module for bare ImportError paths that could silently truncate the namespace.

aiter/jit/core.py special rule: This file bootstraps the entire JIT compilation pipeline. A syntax error, wrong default, or broken env-var handling here means zero aiter ops load. Changes here require e2e smoke test across all GPU arch targets.

What the reason has to answer — this is what makes a COVERED checkable. For Tier 1 files (jit/core.py, __init__.py — these two only):

• Every public symbol changed, and its callers across aiter itself (grep -rn '<symbol>' aiter/). A caller not covered by the PR's test is a GAP.
• For __init__.py: does the new import have a bare ImportError path that could silently truncate the namespace?
• For jit/core.py: is there an e2e smoke test that loads all kernels on gfx942 AND gfx950 after this change?
• If this change is wrong, what breaks and how would it be detected? (all ops fail / one op family fails / silent wrong value)

For Tier 2 files (fused_moe, mha, attention, gemm, mla, tuned_gemm, quant):

• Which model families (DSv3, Kimi, MiniMax, GLM…) use this op? Is at least one from each family in the test?
• Are production shapes tested? At minimum: decode (M=1, TP=4/TP=8) AND prefill (ISL=4096, TP=4/TP=8).
• Does the change affect gfx942 only, gfx950 only, or both? If both, are both arch paths tested?

AI code red flag — verbatim duplication across backbone files: Same algorithm copy-pasted into 2+ backbone files with only variable names changed. See D5.

---

Step 5 — Rule Checklist

Adjudicate every rule in $WORK/rules.txt, one line each, into $WORK/verdicts.txt. The derivation already cut the list to what this diff can actually trigger — 12 rules at the median over 597 PRs, 14 on the Triton subset — so there is no rule here you may pass over because the list looked long. Format, one per rule id:

<RULE-ID> FIRE|CLEAR|N/A — <the specific reason, naming file:line, symbol, or the condition>

CLEAR means you looked and it does not apply to this diff; it is a claim, and the reason is what makes it checkable. "ok", "n/a", "fine" are not reasons — Step 8's gate rejects them.

Step 8 will not let you write a verdict card until every derived rule has a line with a reason. This is the same move as running the D9 scan inside Step 1 rather than asking for it mid-checklist: on a 14-PR controlled run the revised D9 prose caught 0 of 3 known overflow defects and the scanner it names was never once invoked. A checklist a reviewer marks off to itself decays under load, silently, and the queue ahead is large.

Six failure categories — work all six in order. Advisory severity per finding: 🔴 high risk / ⚠️ should fix / 📝 note. These labels prioritize human attention; they do not themselves gate a merge.

🔴 evidence threshold — before firing any 🔴, write down the concrete input that triggers it. Name the specific shape / scale / dtype / arch / value that makes the finding fire (e.g. "at token_id > 16M with H=32, D=128 the int32 product exceeds 2^31", or "when arch=='gfx1250' with fp4 input the branch assumes fp8"). If you cannot state a concrete triggering case, the 🔴 is unproven — downgrade to ⚠️ ("worth checking") or drop it. A 🔴 that reads as a definite defect but names no demonstrable triggering input is exactly how a false positive lands on a maintainer's PR. This threshold applies to every rule below — including those whose own text omits an explicit FP self-check (e.g. D9): the same index expression is safe in a capped/small-batch kernel and unsafe only at a scale you must actually exhibit.

Category	Core question	Key triggers
A. Coverage gaps	Same bug elsewhere? Same code other configs?	_opt, _prefill_opt, _v2; shared path; broad if condition
B. Silent bypass	Does every input reach the right branch?	gated-off param; string alias; non-aligned dim; proxy metric
C. Hardcoded arch/dtype	Does the constant break on another GPU or fp8 flavor?	240.0, 448.0; arch name for fnuz; bf16 fixed
D. Uninitialized state	Is the buffer clean before atomic/kernel launch?	::empty()+atomic_fmax; fill_(0) missing
E. Cross-repo sync	Does the consumer know about this change?	new aiter symbol; default-preserving new param; plugin bridge
F. Resource duplication	Does the change double GPU memory silently?	new _preshuffled/_quantized weight alongside original

---

The rule bodies are not in this file. $WORK/rules_expanded.txt, written by Step 1b, holds the full text of exactly the rules this diff derives — read that. All 51 blocks live in rules.md; the median PR needs 103 of their 383 lines, so keeping them here loaded 280 lines of irrelevant rule text into every review, on top of a skill that is already long enough that "read all of it" is a hope rather than a guarantee. Cutting which rules you are told to check from 44 to 12 while still shipping all 44 rule texts was half a fix.

Step 6 — AI Code Diagnostic

For each question below, note if the answer is a warning sign:

Question	Warning sign
Does description explain mechanism (WHY) or just action (WHAT)?	Only WHAT → elevated risk
Are perf numbers suspiciously clean? (exact 2.0x, 1.5x, 3.0x)	Could be cherry-picked or fabricated
Are perf claims only trace screenshots with no numeric values?	Screenshots ≠ numbers; reviewer will ask
Does the test only cover M=1 or M=16?	AI defaults to toy shapes
Are gated-off parameters asserted or silently ignored?	Silent → B1 violation
Does code introduce sys.path, os.environ mutations at module level?	Global state leak → HK3
Were unrelated files committed alongside the actual change?	AI commit artifact → HK2
Is the new default path revertible?	No env-var gate → D2 violation
Is "Test Plan" / "Test Result" section left as template comment?	Empty = untested, AI-generated description
PR description footer says "🤖 Generated with Claude Code" or similar AI attribution?	Author may not understand the change — elevated manual review priority