review-pr

# 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 ```bash # 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 |