Add validate-kernel-pr skill + structural D9 trigger for review-pr

[jhinpan]

Motivation

review-pr is a static reviewer — it never builds and never runs. Two concrete gaps follow from that, both reproducible from this repo's own history.

1. Rule D9 never fires. D9 covers 32-bit overflow in pointer arithmetic, and its trigger is a list of variable names (token_id, seq_start, batch_offset, total_tokens). Three real defects in this repo used none of those names, so the rule stayed silent on all three:

introduced in	expression	later fixed by	failure
#1674	stride_out_batch not tl.int64	#4244	output offset wraps at large MTP batch; tail rows keep stale sparse-KV indices, draft acceptance halves
#1674	block_id * stride, no .to(int64)	#4774	every block past INT32_MAX returns logits of exactly 0.0, silently
#3541	ArithValue(physical_block) * stride	#4680	wraps on a ~150M-row KV pool; the wrapped offset still lands inside the allocation, so no fault

Running the current skill over #1674 produces a verdict card in which int32 / int64 / overflow appear zero times, despite that PR carrying two of the three.

2. A green test suite is not evidence. Two failure modes are invisible to any static reviewer:

• a suite whose non-aligned shapes are commented out passes on an out-of-bounds tail store;
• a change that widens a comparison tolerance leaves every test passing and the kernel unguarded.

A backtest of the current skill over 14 merged PRs (labels derived from the later [Fix]/revert PRs that repaired them) found 9 of 17 known defects, measured with each case pinned to its merge commit and gh time-boxed so the later fix PR is not visible to the reviewer. Its precision was good and its false-positive rate low; the gap was recall, and the misses clustered into the families above. That is why this PR adds a runtime layer and a mechanical scan rather than more prose rules.

Technical Details

New skill validate-kernel-pr — SKILL.md, validate_pr.sh, report_schema.json. Emits validation_report.json:

merge_sim → gpu_claim → runtime_compat → test_policy → correctness → index_width_scan → verdict

• correctness runs the PR's own tests and a shape grid the skill owns (non-toy, boundary/odd, long-context), reported separately — the informative case is when they disagree.
• test_policy compares tolerances head-vs-base before running anything, so a suite that can no longer fail is caught rather than reported green.
• gpu_claim picks a GPU over a sampling window and holds flock for the run; the report records host, HIP index, BDF, arch and pre-run activity.
• runtime_compat distinguishes "checkout does not match the installed runtime" from "the PR is broken", so a stale prebuilt runtime is not charged to the author.
• The report states what it did not do: arch_coverage marks each arch runtime or compile-only, isolation records the real level, degraded_mode marks COMPILE_ONLY when no GPU was claimable.

scan_index_width.py — structural pre-filter for the D9 family: an index-shaped value multiplied by a stride-shaped value on a line with no 64-bit widening. Output is candidates for the reviewer to clear, not findings.

review-pr changes — four:

• D9's trigger becomes structural, with the scanner as its mechanical pre-filter.
• REPO was hard-coded to ROCm/aiter, so the skill could not review a FlyDSL PR at all; it now takes a repo argument or owner/repo#N.
• FlyDSL/Triton kernel PRs require a validation_report.json; without one the card marks itself [static-only review] and no finding may assert runtime behaviour as fact.
• The card caps at 5 findings, most-severe first, and states its validation evidence. The skill averaged 7.1 findings/PR in the backtest; dropping everything judged noise still left 5.4, so the cap has to be explicit.

Skills live under .claude/, which .gitignore excludes; the seven existing skills are tracked anyway, so these are added the same way (git add -f).

Test Plan

1. Seed three defects into a FlyDSL softmax kernel — dropped tail guard, loosened tolerance, wrong index unit — and run validate_pr.sh against each plus a clean baseline.
2. Re-run scan_index_width.py from its shipped path against Support mqa_logits blocksize is multiple of ChunkK cases #1674 and Add fused QK/KV norm + RoPE + group-quant ops #3541.
3. Syntax-check both scripts, the schema, and the bash snippet review-pr Step 1 tells the agent to run.

Test Result

Run on MI355X (gfx950). Each mutant is caught by a different stage, and the clean baseline stays green:

run	PR's own tests	S1 shape grid	tolerance policy	verdict
clean baseline	pass	pass	pass	PASS
tail guard dropped	pass	fail	pass	BLOCK
tolerance loosened	pass	pass	fail	BLOCK
wrong index unit	fail	pass	pass	BLOCK

Row 2 is the case this skill exists for: the kernel writes out of bounds on unaligned N, the repo's own suite reports green because its non-aligned shapes are commented out, and only the S1 grid (N=2000, N=257) fails it.

scan_index_width.py flags all three D9 defects above, including the one whose source comment already reads # i32: physical_block*kcache_block_stride + slot*kcache_token_stride. Scripts, schema and the Step 1 snippet all parse.

Limitations

• gfx942 was not exercised. The validation host has 8× MI355X and no gfx942 device; reports say gfx942: compile-only. Nothing here claims gfx942 numerical validation.
• No perf or claims stage. report_schema.json reserves both; the script emits neither. A report carries no performance evidence today, and a missing perf stage must not be read as "no regression".
• No PR-fetch orchestration. The caller supplies the worktree and the pytest target; picking the right test target from a diff is unsolved, and a wrong target produces a confident green.
• The backtest's precision figures are self-adjudicated, not third-party reviewed. The reproducible claims are the ones above: the D9 misses (traceable to the fix PRs named) and the mutant table.
• The D9 change here is not yet shown to work end to end. The scanner flags all three defects offline, but a controlled re-run — same 14 PRs, worktrees pinned to each PR's merge commit so the later fix is not visible, gh time-boxed — found the revised skill catching 0 of 3, the same as the unrevised skill. No run shows the scanner being invoked: the instruction to run it sits mid-checklist in Step 5, and only Step 1's bash block is reliably executed. The fix is to move the scan into Step 1 so the candidate list is in context before the rule pass; that is not in this PR. Treat the D9 edit as a corrected rule statement, not a demonstrated improvement.
• The 5-finding cap is demonstrated: it binds on 14/14 cases (5.0 findings/PR vs 7.7 uncapped), with no evidence it removed a true finding.

Out of scope

While backtesting #4394 the review found that its rebase reverted the k_batch >= 2 split-k guard added by #3997 — present at merge^, absent at merge, and still absent on main today (aiter/fused_moe.py:3703). Not touched by this PR; it wants its own fix.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

🏷️ CI Guide

Runs automatically on every PR:

• ✅ Pre-checks (submodule verification, code formatting)
• ✅ Aiter op tests (gfx942 + gfx950)
• ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label	Tests
ci:gfx1250-ffm-triton	Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x	Run an additional Triton test job on MI300X in PRs; main branch always runs both MI35X and MI300X
ci:sglang	SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom	ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full	ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm	vLLM benchmark: GPT-OSS-120B, DeepSeek-R1-0528, Kimi-K2.5
ci:all	All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 4870 --add-label <label>

PR title tags:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf] and op tags like [MLA] are left untouched. Add the no-auto-title label to opt this PR out of title tagging.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.