| 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
"$(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 |
