validate-kernel-pr

# validate-kernel-pr `review-pr` reads the diff; it does not build and it does not run. It is a static reviewer, and a good one — but three failure modes are invisible to it, and this skill exists for exactly those three: 1. **The PR's own tests pass while the kernel is wrong.** A suite whose non-aligned shapes are commented out reports green on an out-of-bounds tail store. 2. **A green suite that cannot fail.** Loosening a comparison tolerance leaves every test passing and the kernel unguarded. 3. **Defects that only exist at runtime.** LDS over-allocation on one arch, an accuracy gate failing against the reference, a JIT path that no-ops on cache miss. Output is `validation_report.json`: deterministic execution evidence kept separate from `review-pr`'s advisory judgement. A review may consume it only when `repo.head` matches the exact PR head; a review written without one must mark validation `NOT RUN`. The two skills stay split at judgement, not at invocation. `review-pr` triages whether a PR has runtime surface at all and, when it does and the PR ships a single target, runs this script itself rather than asking a human to. Everything below is still produced here and merely consumed there: the executor never writes an advisory verdict, and `review-pr` never manufactures evidence it did not get from a report. --- ## Invocation The caller supplies a clean base checkout, the base-to-head patch, the exact head OID, and the test target; this script does not fetch PRs itself (see [Not implemented yet](#not-implemented-yet)). ```bash # Example for a FlyDSL softmax PR. REPO=ROCm/FlyDSL PR="${PR:?set PR to the open softmax PR number}" # 1. pin the PR identity and put its base in an isolated worktree BASE_REF=$(gh pr view "$PR" --repo "$REPO" --json baseRefName --jq .baseRefName) BASE_REF_PATH=$(python3 -c \ 'import sys,urllib.parse; print(urllib.parse.quote(sys.argv[1], safe=""))' \ "$BASE_REF") BASE=$(gh api "repos/$REPO/branches/$BASE_REF_PATH" --jq .commit.sha) HEAD=$(gh pr view "$PR" --repo "$REPO" --json headRefOid --jq .headRefOid) git worktree add --detach "/tmp/pr-$PR" "$BASE" gh pr diff "$PR" --repo "$REPO" > "/tmp/pr-$PR.patch" # 2. validate base and head under the same runner and GPU lock .claude/skills/validate-kernel-pr/validate_pr.sh \ --repo "/tmp/pr-$PR" \ --patch "/tmp/pr-$PR.patch" \ --head-sha "$HEAD" \ --target tests/kernels/test_softmax.py \ --expected-route kernels.softmax_kernel:build_softmax_module \ --shape-vars M,N,dtype_str \ --shape-env ROCDSL_SOFTMAX_SHAPES \ --grid "64,2048,f32;64,2000,f32" \ --tol-table "f32=1e-5,f16=2e-3,bf16=1e-2" \ --out validation_report.json ``` For a local candidate with no remote head, omit `--head-sha`. The report then records `repo.head: null`; it remains useful locally but `review-pr` will reject it as PR evidence. | flag | meaning | |---|---| | `--repo` | worktree to validate (required) | | `--target` | script file or pytest node/file the PR ships (`--tests` remains an alias) | | `--patch` | patch to apply first; conflict is a blocker | | `--head-sha` | exact remote PR head represented by the patch | | `--expected-route` | exact `module:function` route the validator-owned profiler must observe | | `--shape-vars` | comma-separated local names captured from each route call, in grid order | | `--shape-env` `--grid` | env var and shape list for the S1-owned grid | | `--shape-arg` | the target's own CLI flag that accepts shapes, for script targets that read no env var | | `--shape-argnames` | the pytest parameter names the grid should replace, for targets whose shapes are literals inside `@pytest.mark.parametrize` | | `--axis` | repeatable `NAME=FLAG:v1;v2;…` — an extra independent test axis on its own CLI flag (see [`axes`](#axes-when-the-failing-configuration-is-not-a-shape)) | | `--runner` | force `pytest` or `script` when the structural classifier gets it wrong; the report records both the forced choice and what selection had said | | `--perf-control-column` | a timing column the patch does not touch; required before a transplanted baseline is believed (see [`perf`](#8--perf)) | | `--tol-table` | reference tolerances recorded alongside the head-vs-base comparison (see [`test_policy`](#4--test_policy--run-before-the-suite)) | | `--perf-args` | benchmark entry point for the timing stage; also forces perf on when detection would decline | | `--no-perf` | skip the timing stage entirely | | `--label` `--out` | run name and report path (default `./validation_report.json`) | Several settings are environment variables rather than flags, because they describe the **host** rather than the PR under test, and a caller validating many PRs on one machine sets them once: | env | meaning | |---|---| | `PYLIB` | runtime modules living outside the checkout | | `PYTHON_BIN` | the one interpreter used for both pytest and script targets | | `PICKER` | override the shipped `pick-idle-gpu.py`; unset, the **shipped** picker is used, and only then one found on `PATH` | | `TIMEOUT` | per-target budget, default 1800s | | `PERF_TIMEOUT` | the timing stage's own budget, defaulting to `TIMEOUT`, because a bench sweep is legitimately longer than a correctness run | | `PERF_REPEAT` | runs per side, default 3 | | `PERF_THRESHOLD` | head/base ratio that counts as a regression, default 0.95 | | `PERF_MIN_ROWS` | matched rows required before any ratio is reported, default 3 | Everything that describes the PR is a flag. The executor also overrides `AITER_JIT_DIR` with separate fresh base/head directories and sets `PYTHONDONTWRITEBYTECODE=1`, so repository JIT output cannot cross phases or dirty the worktree. ### Which shape channel to name The three shape flags are alternatives, not a sequence; supply the one the target actually has. The report says which channel was established in `test_selection.grid_channel`, and when none was, `test_selection.grid_channel_reason` names each channel tried and what was found in the target — so a failed guess costs one run, not a reading of this file. | the target takes its shapes from | flag | what is checked before the channel is credited | |---|---|---| | an environment variable it reads | `--shape-env` | the source reads that name via `os.getenv` / `os.environ` | | its own CLI flag | `--shape-arg` | the source passes that flag literal to `add_argument` | | `@pytest.mark.parametrize` literals | `--shape-argnames` | the source binds all those names as test parameters | All three are then held to the same proof: a deliberately unusable grid must make the target **fail**. A target that ignores the grid produces a skip, never a pass. --- ## Stages Each stage writes its own status into the report. A stage that cannot run says `skip` with a reason — it never reports `pass` for work it did not do. ### 1 — `merge_sim` Apply the PR head on top of the current base. A conflict is a blocker and short-circuits: no number produced downstream would describe the merged code. Known collision surfaces worth a second look because they are edited by many PRs at once: tuning CSVs (duplicate shape rows), `csrc/include/rocm_ops.hpp`, `aiter/jit/optCompilerConfig.json`. The supplied worktree must be clean. The report records the base commit, patch SHA-256, and the caller-supplied head OID. A direct head checkout without a patch can run diagnostics, but cannot prove mergeability or base attribution and therefore cannot produce `PASS`. The patch is reverted when the process exits, including on interrupt and on every degraded path, so the worktree is handed back in the state it was supplied. Consecutive runs in the same worktree are therefore supported; a run that left the patch applied would make the next one report `not isolated-clean` and blame the caller. ### 2 — `gpu_claim` Claim a GPU over a **sampling window**, not one instantaneous reading, and acquire a non-blocking lock immediately after selection. Hold that file descriptor for the whole run: ```bash PICK=$(python3 .claude/skills/validate-kernel-pr/pick-idle-gpu.py \ --samples 10 --interval 1 --quiet) flock /tmp/gpu-$PICK.lock <command> ``` The report records host, HIP index, matching AMD SMI index, BDF, market name, architecture, and GFX activity before the run. `pick-idle-gpu.py` emits the **translated HIP index**; the validator maps it back through AMD SMI enumeration instead of incorrectly using it as an AMD SMI index. `amdsmi_get_gpu_activity` is not available everywhere — some driver and amd-smi combinations fail it outright or report `N/A` while enumeration, BDF, ASIC and VRAM queries all work. Activity is therefore treated as optional, and `gpu_claim.idleness_basis` names the evidence the claim rests on: `activity+vram` when busy percentages were measured, `vram-only` when only resident VRAM separated the devices. In the `vram-only` case `gfx_activity_before_pct` is `null`, which means unknown, not zero — an unavailable metric is never reported as an observed idle GPU. If no GPU stays idle, `gpu_claim` is `skip`, `degraded_mode` is `NO_GPU`, and the script performs no architecture-specific compile in this branch, so it does not call the result `compile-only`. That skip names which fact it rests on: no GPUs on this host (picker exit 3) and GPUs present but none idle (exit 1) are environment facts, while AMD SMI being unqueryable (exit 2) says nothing about the GPUs at all. When no GPU was claimed, the target is then asked whether it needs one: it is run once with no visible device, and `test_selection.gpu_requirement` becomes `not-required` only if it **passes and executes at least one test**. The executed count is what makes this evidence — a suite guarded by `skipif(not torch.cuda.is_available())` also exits 0 while proving nothing. This is deliberately an observation rather than a judgement about the diff: a Python-level dispatch change reroutes kernels without touching kernel source, and ROCm/aiter#5089 decides whether 34 gfx950 kernels compile from a seven-line helper, so no static rule over changed paths could settle it. A `not-required` target runs its correctness stages instead of skipping them, which is the evidence a CPU-only fix is able to supply. It still claims nothing further: `arch_coverage` stays empty because only a passing `gpu_claim` credits an architecture, and `PASS` is unchanged — it continues to require `gpu_claim: pass`, so this path cannot produce a clearance that was previously unreachable. ### 3 — `runtime_compat` Does the repository's own package import from the supplied checkout against the runtime that is actually installed? The probe is repository-aware: Aiter resolves `aiter` from the checkout; FlyDSL resolves the pinned package from `PYLIB` (when supplied) and compares its version with the checkout's `python/flydsl`. This keeps compiled `_mlir` bindings available without pretending an unrelated FlyDSL install validates an Aiter checkout. A pinned prebuilt runtime can drift behind the tree, and the resulting `ImportError` looks exactly like a defect in the PR. A mismatch is an environment fact: `runtime_compat` and correctness are skipped, the verdict is `INCONCLUSIVE`, and nothing is attributed to the author. The report records the Python executable/version, resolved package path/version, and SHA-256 identities for native libraries loaded by the runtime probe. If a FlyDSL PR changes Python, C++/MLIR bindings, headers, CMake, or packaging inputs, a prebuilt `PYLIB` is not accepted: trusted build-system provenance is not implemented, and caller-authored metadata cannot prove which source produced a binary. Such runs return `INCONCLUSIVE` instead of testing a stale package. This matters most for FlyDSL kernels: the Python kernels import symbols from a compiled runtime, so "one fresh container per PR" would mean rebuilding MLIR/LLVM per PR. The workable shape is a pinned prebuilt image plus this compatibility gate. ### 4 — `test_policy` — run **before** the suite A suite that cannot fail is worse than no suite, because it produces a green report. Two checks: - **Tolerance, compared head-vs-base.** Repos legitimately differ per kernel; the question is whether *this change* loosened what was there. A test-only widening is a deterministic blocker. If kernel code changed too, the widening is `NEEDS_WORK` pending numerical justification rather than a false deterministic block. - **Commented-out shape rows, compared head-vs-base.** Existing rows are recorded as coverage context; only rows newly disabled by the change produce `NEEDS_WORK`. The independent grid remains visible either way. ### 5 — `correctness` — the repo's tests, then a grid the repo does not run Runner selection is structural, not assumed: - an explicit `path::node`, or a file defining `test*`/`Test*`, uses pytest; - otherwise a file with an `if __name__ == "__main__"` guard runs as `python <file>`; - a file with neither is `skip`, never a test failure. Selection can still pick a runner the target cannot survive. A file that defines `test*` nodes **and** parses argv in its module body is collected by pytest, which imports it with pytest's own argv — and argparse exits the process, while the same file is green run as a script. `test_selection.runner_risk` names that structurally, and a run that executes nothing under the selected runner says so: *"red on both sides"* is an attribution, not an explanation, and a reader who is not told otherwise concludes the code is broken when the runner choice is. The report records `test_selection.runner` and `runner_reason`. A script target is profiled the same way a pytest target is: the probe is installed by a validator-owned runner that then executes the file under `runpy` with `run_name="__main__"`, so `execution_receipt` is reachable for both. Nothing about `sys.setprofile` needed pytest; pytest was only where the hook was installed. Both, and they are reported separately, because the interesting case is when they disagree. Pytest runs emit JUnit XML and a zero-executed/all-skipped target is `skip`, never `pass`. A script target publishes no per-case count, so its `executed` is a liveness signal and nothing more — it says the process ran. What it must not do is stand in for work. A target that returns silently with exit 0 and a log line (aiter#4538's does, when the arch is unsupported or an optional package is missing) produced exactly the same `executed: 1` as the run that graded 56 cases, and earned the same `arch_coverage: runtime` on basis `script-exit-zero-with-output` — a statement about the process, not the kernel. So: - `stats.observed_work` carries the only number backed by evidence: route calls counted in **that run's own** execution receipt. It is `null` when no route was named, because nothing was watched; - `arch_coverage` credits nothing when `observed_work` is `0` — a route was watched for and never reached the device — and `arch_coverage_basis` prints the count and its provenance rather than implying a measurement. For a patch run, the validator reverses the exact patch to create the baseline, verifies that the worktree is clean, runs both targets under base-only caches, and reapplies the patch before a head run with separate caches. This removes new files too; a PR-added failing test is therefore `target-not-present` on base, not falsely classified as a pre-existing failure. Any worktree artifact, reverse/reapply failure, or cache-isolation failure aborts the head run and produces `INCONCLUSIVE`. The S1-owned grid must cover three classes the PR's own tests routinely miss: | class | why | |---|---| | non-toy | `M=1` / `M=16` only is the standard agent-generated test | | boundary / odd | odd N, N not a multiple of the tile — where tail masks fail | | long-context / large M | where 32-bit index arithmetic wraps | The grid reaches the target through whichever channel that target actually reads, and the channel must be proven structurally before it is used: | channel | flag | proof the hook exists | |---|---|---| | environment variable | `--shape-env` | the source references `os.getenv(VAR)` / `os.environ[VAR]` | | the target's own CLI flag | `--shape-arg` | the source passes that flag literal to `add_argument` | Injecting through an env var only would have made this stage permanently inert for repositories whose tests take shapes on the command line — a limit of the injector, not of the target. The flag is named by the caller rather than guessed, because a wrong guess appends argv the target silently ignores. Neither channel is trusted on the strength of the AST scan alone: the stage re-runs the target with a deliberately invalid grid value and requires it to fail. A target that passes with garbage shapes is not consuming the grid, so the stage is `skip`, never credited. With no channel configured the stage is `skip` and the verdict is `INCONCLUSIVE`. This is a positive control against reporting the same default test run twice under different stage names. When the kernel exposes no shape override, the report says `repo-default-only` rather than claiming coverage it does not have. **A proven channel is not the same as added coverage.** A target that consumes the grid can still be handed cells it already runs by default. On ROCm/aiter#4538 all three requested shapes were in the target's own `--shapes` default list, so the "independent" grid re-ran a strict subset of the repository run and the stage reported `pass` — the exact duplication this stage exists to prevent, invisible in the report. The grid cells are therefore compared against the target's own declared default for the same flag, and `test_selection.grid_independence` is one of: | value | meaning | |---|---| | `adds-coverage` | at least one cell is outside the target's own default | | `duplicates-target-defaults` | every cell is already a default; a passing run is downgraded to `skip` and the verdict to `INCONCLUSIVE`, because a passing duplicate proves only what `correctness_repo_tests` already said | | `unknown` | the channel exposes no literal default to compare against (every env-var channel, and a flag whose default is computed) | A duplicate grid that **fails** keeps its `fail`. The finding is real; what a duplicate cannot do is earn a pass. #### Axes: when the failing configuration is not a shape `--grid` is one ordered tuple on one channel, which is all a target's shape flag accepts. A target whose remaining knobs are separate flags — head counts, dtypes, window modes — could not be gridded over them at all, so entire configurations were unreachable however the grid was spelled. That is not a missing shape; it is a missing axis. aiter#4538 is the case: `--shapes` carries `(seq_len, seq_len_kv)` while `--num-heads` is its own flag defaulting to `64 128`, and the public API asserts at `num_heads=16` — a real blocker the validator had no way to request. ``` --axis 'num_heads=--num-heads:16;32' ``` Axes obey the same burden of proof as `--shape-arg`, in two steps, and `test_selection.axis_state` names which one it reached: | state | meaning | |---|---| | `none` / `unusable` | none requested, or the target is not a script (argv reaches script targets only) | | `hook-not-found` | the target's source declares no `add_argument` for that flag | | `hook-not-consumed` | the flag was declared but accepted `__VALIDATOR_INVALID_AXIS__`; the axis is **dropped and named**, never dropped quietly | | `proven` | every axis flag refused an invalid value, and its values rode the grid run's argv | Each axis records its own `independence` against the flag's declared default, on the same terms as the shape cells. A proven axis that asks for values outside the default makes the run independent even when the shape cells duplicate.