Add kernel PR validation and structural D9 scanning - #4870
Conversation
…kernel PRs review-pr is static: it never builds and never runs. Three failure modes are invisible to it, and a 14-PR backtest of review-pr alone found 8 of 17 known defects, so the gap is real rather than theoretical. This skill produces validation_report.json as the evidence base: merge_sim -> gpu_claim -> runtime_compat -> test_policy -> correctness -> index_width_scan -> verdict Two stages exist because a green test suite is not evidence: - correctness runs the PR's own tests AND a shape grid this skill owns (non-toy, boundary/odd, long-context). Seeding a dropped tail guard into a FlyDSL softmax kernel leaves the repo's own suite green -- its non-aligned shapes are commented out, only one aligned shape runs -- while the grid at N=2000 / N=257 fails it. - test_policy compares tolerances head-vs-base before running anything. A change that widens a tolerance while leaving the kernel path unchanged makes the suite unable to fail, and pytest still reports green. Verified against three seeded mutants plus a clean baseline: baseline passes every stage; dropped tail guard blocks via the grid alone; loosened tolerance blocks via test_policy alone; wrong index unit blocks via the repo tests. The report states what it did not do, so it cannot overclaim by omission: arch_coverage marks each arch runtime or compile-only (a gfx950 host cannot validate a gfx942 claim), isolation records the real level, and degraded_mode marks COMPILE_ONLY when no GPU was claimable. runtime_compat exists because a pinned prebuilt runtime drifts behind the tree and the resulting ImportError otherwise reads as a defect in the PR. scan_index_width.py ships alongside it; see the review-pr D9 change.
…dation evidence Four changes, each from a measured failure in a 14-PR backtest of this skill. 1. D9 never fires. Its trigger was a list of variable names (token_id, seq_start, batch_offset, total_tokens). Three real 32-bit overflow defects used none of them -- stride_out_batch, block_id, physical_block, context_kv_idx -- and int32/int64/overflow appears zero times in the whole verdict card for aiter#1674, which carries two of them. The trigger is now structural: an index-shaped value multiplied by a stride-shaped value on a line with no 64-bit widening. scan_index_width.py is the mechanical pre-filter and flags all three; a bare model missed the same family, so this is not something to leave to recall. 2. REPO was hard-coded to ROCm/aiter, so the skill could not review a FlyDSL PR at all. It now takes a repository argument, or owner/repo#N. 3. FlyDSL/Triton kernel PRs now require a validation_report.json and load validate-kernel-pr. A kernel PR's own green suite is not evidence. 4. The verdict card caps at 5 findings, ordered most-severe first, and states its validation evidence. This skill averaged 7.1 findings per PR across the backtest; dropping everything judged noise still left 5.4, so the cap has to be explicit. Without a report the card says [static-only review], and no finding may then assert runtime behaviour as fact. Backtest numbers for the frozen version being changed here (blob 3fdb11a, 14 PRs, 17 provenance-checked labels): 76 of 100 findings useful, 0 factually wrong, 8/17 defects found.
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
PR title tags: |
The Invocation section showed `validate_pr.sh --pr 4394`, a flag the script does not implement -- it exits 2 with "unknown arg --pr". That is the hallucinated-API failure review-pr Step 6 exists to catch, shipped in the skill meant to catch it. Document the interface that is actually implemented and verified: the caller supplies the worktree and the pytest target. Every flag named in the doc is now present in the argument parser. Also declare what is absent rather than leaving it to be inferred: - no PR fetch orchestration; choosing the right --tests target from a diff is the unsolved part, and a wrong target produces a confident green - perf and claims stages are reserved in report_schema.json and emitted by nothing, so a report today carries no performance evidence and a review must not read a missing perf stage as "no regression"
A scan that Step 5 asks for does not happen. In a controlled 14-PR run of the revised D9 text, no session invoked the scanner and the arm caught 0 of 3 known overflow defects -- the same as the unrevised skill. Moving the invocation into Step 1's fetch block does make it run: the session transcript for aiter#1674 shows the scanner executing and its candidate list entering the context. D9 now works that list instead of asking for a scan. This is necessary but NOT sufficient, and the PR text says so: with the scan in Step 1 the arm still caught 0 of 3. What closes the gap is supplying the production-scale facts the 5xx-gate demands -- see the limitations section.
Found by running validate-kernel-pr end to end on a real PR for the first time (FlyDSL#969, conv3d fp8 padded-M store guard). Both the pre-fix and post-fix trees failed the same test shape -- one unrelated to the PR -- and the harness reported "the PR's own test target fails" as a blocker in both. It charged pre-existing red to the author, which is the misattribution this skill exists to prevent. With --patch, the suite now runs on the base first and a failure is only a blocker when the base is clean; otherwise it is a note saying the target is red with and without the change. The same run goes from BLOCK to PASS with the pre-existing failure recorded rather than blamed. Also carries the production-scale table as a separate file printed by Step 1 beneath the scan candidates. Its effect is documented in the PR description and is small: 1 of 3 on the target defect family, versus 0 of 3 without it.
…po incident Downstream tests here are label-gated and skipped by default, so the question "can this break vLLM or SGLang" decides whether they run -- and it is normally answered from memory. aiter#4530 is why that is not good enough. It changed one file, aiter/ops/triton/_triton_kernels/moe/moe_routing/topk.py: kernel internals, no public signature touched. It broke vLLM's gpt-oss MXFP4 MoE on MI355 after the 0.1.16.post5 -> 0.1.19 bump (vllm#49361, test plan blank), needed a hotfix that names it as the companion fix (vllm#50859), and SGLang reverted its aiter pin the same week (sglang#32879). scan_downstream_consumers.py walks aiter's imports upward from the changed files and stops at the first module a downstream checkout imports. On ROCm#4530 it reconstructs the chain from the internal topk kernel through moe_routing and moe_op_gemm_a16w4 to vLLM's aiter_mxfp4_w4a8_moe.py, and correctly reports no reachability for SGLang, which does not import that subtree. The first version asked "did a public symbol change" and answered "no downstream impact" for that exact PR. Signature is the wrong question; reachability is the right one. That earlier criterion is recorded in the file so it is not reintroduced. Validated on one positive and one negative, which is thin -- the skill says so, along with the rest of what it cannot see: static imports only, five hops, the downstream's current main rather than the pinned version, and no ATOM checkout. Wired into review-pr Step 1 alongside the other scanners, because a check the reviewer has to remember to run does not run.
Proposed acceptance bar, so this can keep convergingThe measurement discipline here is the best this repo has applied to tooling — reporting that the 1. Declare the tier. Advisory (hints; human reviews as before) needs only proof of no net Corollary: an LLM judgement never gates; only a deterministic executor can. 2. The durable asset is the corpus, not the skill text. 14 PRs, 17 defects, each pinned to its 3. Then acceptance is a ratchet, not a debate: no per-family recall@5 may regress, false Phase 0 — this PR: stop asserting cleanliness that wasn't establishedA silent miss costs nothing; the reviewer's skepticism survives. A false clearance spends it. A
Everything else — recall@5 (unmeasured, though the cap is 5), n=1 across all four arms, an Appendix — the measurement programme behind the bar above (phases, thresholds, kill criteria)Phase 1 — build the asset
Exit: one command reproduces the 9/17 number, and the ratchet is enforceable. Phase 2 — pay the measurement debt
Exit: the tier-B bar is met, or a kill criterion fires. Phase 3 — tier B (triage), with the joint system watched
Tier C additionally needs deterministic stages only, a false-block SLA, and an appeal path. The LLM Every number in this table is a placeholder with no empirical basis — I made them up to give the The unit of evaluation is human + toolThe baseline is not "no tool", it's "a human reviews it". A tool that raises recall can still make
Kill criteriaA bar that cannot fail is not a bar — the same argument this PR makes about test suites.
Docs should state
|
Separate advisory review output from deterministic validation, make every missing evidence path explicit, and preserve exact base/head attribution. Commit the pinned mutant corpus so these safeguards remain reproducible.
Treat shadowed FlyDSL source, unused shape-grid hooks, and base-run artifacts as inconclusive instead of allowing them to produce false validation clearance.
Require an exercised shape-grid handshake, isolated base/head state, and runtime-backed architecture coverage so incomplete evidence cannot be accepted as a clean result.
jhinpan
changed the title
|
@zufayu We have reworked this PR against the acceptance bar in your comment and would appreciate another review of the current head ( The Phase 0 issues are now addressed:
The description now calls out the remaining boundaries: target selection, perf/claim reproduction, gfx942 coverage, and the uncommitted historical train/holdout corpus. Could you please take another look? |
Prevent green pytest from becoming clearance unless validator-owned instrumentation observes the expected route and shapes, JUnit records real executions, and runtime identity is captured. Align report semantics with process exits and reject untrusted FlyDSL runtime rebuild claims.
|
@zufayu One more evidence-hardening follow-up is now at head The historical GPU campaign exposed that green pytest alone still did not prove route/shape execution. This head now requires validator-owned observed evidence for It also refuses prebuilt The clean control now returns |
|
| target shape | runner |
|---|---|
caller passed path::name |
pytest |
defines test* functions or Test* classes |
pytest |
no test*, but has if __name__ == "__main__" |
python <file> |
| neither | none → correctness skip, verdict INCONCLUSIVE |
An AST scan is enough and avoids paying the ~15 s import twice. Recording the choice and its justification (test_selection.runner, test_selection.runner_reason) keeps the report honest about how the verdict was obtained, and naming each log after the runner used avoids filing a script run under a pytest-looking path.
The last row is the important one: a target with no runnable entry point should be skip, never fail. That is a real limitation on what the report proves, and it belongs in review-pr's advisory judgement rather than masquerading as a reproducible kernel defect.
I have this working locally against #4538 — same PR, same GPU — and it turns the false BLOCK into INCONCLUSIVE with correctness_repo_tests: pass (exit 0), runner: script, and arch_coverage: {"gfx950": "runtime"}. It stays INCONCLUSIVE for an honest reason: that target takes shapes only from argparse and reads no environment variable, so no independent grid can be injected. Happy to open it as a PR against your branch if useful.
Two second-order consequences of the same assumption
-
mark_runtime_coverageis pytest-only. It matchescollected N items/N passed, which a script target never prints, so a run that genuinely executed on the claimed device earns noarch_coverage. Per-runner evidence fixes it, but the two are not equally strong — a script's exit-0-with-output is weaker proof than a case count, so I record which one was obtained in a siblingarch_coverage_basisfield rather than flattening both into the same word. -
execution_receiptcannot be satisfied by a script target at all, since the receipt comes from a validator-owned pytest plugin (-p "$PROBE_MODULE"). With an observed route required forPASS, script targets top out atINCONCLUSIVEeven once they execute correctly. That may well be the right call given the receipt's threat model, but it seems worth stating deliberately in the skill rather than emerging from the runner assumption — otherwise "we could not instrument it" and "it did not run" stay indistinguishable in the report.
Environment for the run above: gfx950 (MI355X), ROCm 7.2.4, torch 2.10.0, FlyDSL 0.3.1, one GPU locked for the whole run. Two notes in case they save someone time: merge_sim rejects a worktree holding any gitignored artifact and baseline_control fails if the base run creates new ones, so an aiter checkout needs AITER_JIT_DIR pointed outside the worktree and PYTHONDONTWRITEBYTECODE=1, otherwise its own JIT output and __pycache__ trip both gates. And pick-idle-gpu.py is not in this PR or on main; I wrote one to the documented contract, and on this host amdsmi_get_gpu_process_list reports the same PID against all 8 handles, so idleness had to come from sampled activity plus resident VRAM rather than the process list.
Feedback from running this skill end-to-end on a real PRI used 1. The runner is assumed to be pytest, which turns a packaging mismatch into a false
$ python -m pytest op_tests/test_flydsl_fp8_mqa_logits.py --collect-only -q
no tests collected in 15.39s # exit 5The validator recorded that as
That is a red verdict with no defect behind it, which I'd argue is worse than not running at all, because it looks like evidence. The same target run the way its repo runs it passes 56/56 on gfx950 in 24 s.
What I did locally: parse the target and pick the runner from it — pytest for Whether script targets should also be able to earn a receipt is a real design question — the route/shape reconstruction is the strongest part of this PR and I'd rather not see it dropped. But 2.
One implementation note if you do add it, because I got this wrong first: do not judge idleness by the KFD process list. On this host 3. The ignored-artifact rule makes an aiter checkout unvalidatable by default, and that prerequisite isn't documented.
Two exports fix it, and I think they belong in SKILL.md's invocation block: export AITER_JIT_DIR=/tmp/.../jit # keep JIT output out of the worktree
export PYTHONDONTWRITEBYTECODE=1 # ...and __pycache__ too4. The S1 grid is unreachable for exactly the PRs that most need it.
The natural workaround is to supply a small grid-capable target yourself. That is currently forbidden by two rules that are individually right and jointly exclusive:
So I ended up producing two reports for #4538: one that satisfies the evidence boundary but can only test what the PR ships, and one that carries the real grid evidence but is disqualified as PR evidence. Both are honest, but a reviewer should not have to choose. A For what it's worth, the parts I leaned on hardest all held up: the base-vs-head control correctly reported |
Run script targets through their real entry points so successful non-pytest validation stays inconclusive instead of producing a false BLOCK. Ship the AMD GPU picker so the validator can discover an idle translated HIP device.
|
@zhiding512, thank you for the detailed end-to-end report. At exact head |
Held-out validation at head
|
| site | effect |
|---|---|
pick-idle-gpu.py:67 |
exception escapes sample() → picker exits 2 → gpu_claim: skip |
validate_pr.sh:462 |
gpu_claim metadata probe → GPU_INFO_RC != 0 → gpu_claim: skip |
validate_pr.sh:314 |
record_gpu_activity_after — tolerated, but prints a traceback that reads as a crash |
The second is what makes this structural: SKILL.md documents PICKER as the override knob, but
the executor re-calls the same API inline, so supplying a working picker does not help.
As shipped the run ends degraded_mode: NO_GPU, all four correctness stages skip,
INCONCLUSIVE/exit 2 — on a box with eight verified-idle GPUs.
Change: tolerate the missing metric at all three sites, and report the basis rather than
silently treating "activity unknown" as "activity 0" — that substitution is the
overclaim-by-omission this PR argues against. In the shape the PR already uses for
arch_coverage_basis:
gpu_claim.idleness_basis: "activity+vram" | "vram-only"
Please also split the two conditions that currently share one string: "no GPU on this host" and
"idle GPUs present, activity API unavailable" both report no verified-idle GPU was claimed. The
first is an environment fact; the second is a portability gap in the validator.
2. The worktree is left patched, so a second run in it always fails
merge_sim applies the patch at validate_pr.sh:372. BASE_ACTIVE=1 is set in exactly one place,
:1039, inside the baseline path. When correctness is skipped — e.g. the NO_GPU branch above —
that block never runs, BASE_ACTIVE stays 0, and the cleanup() trap at :288 does nothing. The
process exits with the candidate patch still applied.
Reproduced: after the first run, git status --porcelain --ignored -uall in the supplied worktree
was M aiter/ops/flydsl/utils.py + ?? op_tests/flydsl_tests/test_flydsl_utils.py — exactly the
patch. The next run reported "worktree is not isolated-clean, so merge simulation is inconclusive"
plus eight stage result was missing notes, which reads as the caller's mistake.
Change: give the merge_sim application its own flag and revert it in cleanup(). BASE_ACTIVE
means "currently in base state" and is a different fact, so it should not be reused for this.
3. pick-idle-gpu.py needs a shebang
In-repo this is harmless: the file ships 100644, so :418-419 falls through to
python3 "$PICKER". But the search list at :399-401 includes /usr/local/bin,
$HOME/.local/bin and /opt/bin, and the normal way to install it there is chmod +x — after
which /bin/sh interprets it and emits Syntax error: "(" unexpected.
Change: one line, #!/usr/bin/env python3.
Patch for 1–3
+23/−3 across the two files, verified here: three consecutive runs in the same worktree with no
cleanup between them, all three reaching gpu_claim: pass (idleness_basis: "vram-only"),
arch_coverage: {"gfx942": "runtime"}, correctness_repo_tests: pass, leaving zero residue. Happy
to open it against your branch, or leave it to you.
4. Correction — this item was wrong, please disregard it
Edited after posting. I originally asked here for review-pr to make the validation report
expected rather than required. That premise was false. At 706eafafc the skill already treats
the report as optional: Step 1 prints validation report not supplied: this is a static-only advisory review, and the FlyDSL checklist line ends absence of a report is not itself a blocker.
The its own green suite is not evidence phrasing quoted in the earlier review round is no longer
in the file. I wrote this item from that earlier round's description instead of checking the current
text — my mistake. There is nothing to change here, and items 1–3 above are the only outstanding
ones.
The observation underneath it survives as an observation rather than a request.
validate_pr.sh:1004-1006 gates CAN_TEST on $PICK before considering what the target needs, so
a CPU-only target still has to claim a GPU — #5089's test is "CPU unit tests for FlyDSL helpers (no
GPU required)" and passes in 1.19 s with HIP_VISIBLE_DEVICES="". Together with
execution_receipt requiring a shape grid, whole classes of otherwise correct PR — script-only
targets, and bugfix unit tests with no shape dimension — cannot reach PASS. Because the report is
optional that costs nothing today. It would matter the moment anyone proposes making it mandatory.
What held up
Worth recording, because it is what makes the four asks above worth making:
- Runner selection is correct on unseen code:
runner: pytest,runner_reason: "target defines pytest test nodes". - No false
BLOCK. The failure mode from the previous round did not recur. - Baseline attribution is correct:
baseline_control.repo_tests.state: target-not-present— a
PR-added test, not a base failure blamed on the author. - It goes red for the right reason. Positive control: I removed the
aiter/ops/flydsl/utils.py
fix and kept only the PR's new test — "shipped the test, forgot the implementation". Result
BLOCK/exit1,correctness_repo_tests: failwithtests:1 failures:1, and the log carries
the real assertion,AssertionError: assert 65536 == 163840. It caught the defect, not an
infrastructure artifact. - It does not overclaim: the clean run stayed
INCONCLUSIVEwithexecution_receipt: skip — no shape grid was configured, andmerge_simrefused a worktree that was not isolated-clean.
The plan after this PR
This PR merges at the advisory tier. That is the right tier for the evidence, and it is also where
the evidence stops: the description states the historical campaign is local and makes no durable
recall or consistency claim. So the work that makes this tool improvable rather than static is all
still ahead, and I would rather it were filed before merge than promised after it.
I opened #5128 for this and have since closed it as premature, which I think was the right call and
is worth recording here rather than leaving a dangling reference. The measurement work only has a
trigger when someone proposes a rule change, or proposes to let the tool gate a merge; neither has
happened, and a scoring harness built for a tool that is not yet in use would decay before it is
needed. When someone does want to argue that a rule edit is an improvement, that is the moment to
build the corpus, and the person making the claim is its natural owner.
One part of it should survive and does not need an issue: the promotion bar belongs in this skill's
own header, because a header is read every time the tool is used while an issue sinks — the tool
stays advisory until false clearance is measured and near zero for the families that raise a red
verdict, and an LLM judgement never gates a merge. That is the wording change in item 4 above.
The four reports from this run — as-shipped, instrumented, the seeded positive control, and post-fix
— are available as the first holdout records for step 1.
Two loose ends that are not part of that and do not need issues from me. --extra-target is already
on the record in this thread with your reason for deferring it, and the shape you described sounds
right to me.
downstream-impact-check was correctly split out of this PR and is now unowned — worth either
reviving as its own PR with the incident stated up front, or closing it on the record.
Where I land
Changes 1–3 belong in this PR — without the first, the skill is inert on a common class of developer
host. With those, I approve. On data it had never seen the tool produced
neither a false clearance nor a false blocker, failed safe in every degraded path, and the positive
control shows it genuinely goes red on a real defect. That is what advisory tier should look like.
There was a problem hiding this comment.
Requesting changes to keep this from being merged in its current state. Head is still 706eafafc,
so none of the outstanding items have landed on this branch.
Two corrections to my review above, both edited into it:
- Item 4 was wrong.
review-pralready treats the validation report as optional at this head
(absence of a report is not itself a blocker), so there is nothing to change there. I wrote it
from an earlier round's description rather than the current text. Items 1–3 are the only
outstanding ones. - Those three are now implemented in
ROCm/aiter#5142—read_activity()making the activity
metric optional with a recordedgpu_claim.idleness_basis, aPATCH_APPLIEDflag socleanup()
hands the worktree back clean, and the shebang. That implementation is better than what I
sketched: it also handles the case where the baseline run already reversed the patch out.
jhinpan/aiter-fork#2, which offered the same three fixes against this branch, was closed without
being merged, and #5142 is now titled as superseding this PR. Which of the two proceeds is for
@jhinpan and @zhiding512 to settle, not something a reviewer should decide — I have no objection to
either path. I am leaving the change request in place only because this branch does not carry the
fixes; it is not a judgement about which PR should land.
Dismissing: this approval was submitted in error against the wrong pull request. See the change request below.
amdsmi_get_gpu_activity was a hard dependency at three call sites, so a host where that single query fails degrades the whole run to NO_GPU even with idle GPUs available. @zufayu hit this on MI308X / ROCm 7.0, where the call returns AMDSMI_STATUS_UNEXPECTED_DATA (43) while enumeration, BDF, ASIC and VRAM queries all work. PICKER could not route around it because the executor re-queries the API inline rather than going through the picker's selection. Activity is now optional. A new read_activity() helper is shared by the picker and both inline probes, and gpu_claim.idleness_basis names the evidence the claim rests on: activity+vram, or vram-only when only resident VRAM separated the devices. Unknown stays distinct from zero -- the previous `gfx if isinstance(gfx, int) else 0` substitution reported an unavailable metric as a measured idle GPU. The gpu_claim skip also stops conflating "GPUs present but none idle", an environment fact, with "AMD SMI is unqueryable", a portability gap in the validator. The import fallback did not probe $ROCM_PATH/share/amd_smi, where the bindings ship on ROCm >= 7.1, so on those containers the picker exited 2 before reaching any activity query -- the same NO_GPU symptom from an unrelated cause. Separately, merge_sim applied the patch but cleanup was guarded by BASE_ACTIVE, which is set only inside the baseline path. A run that skipped correctness exited with the patch applied, and when the baseline path did run, cleanup's restore_head re-applied it, so every path left the caller's worktree patched and the next run reported not isolated-clean against the caller. The application now carries its own flag and is reverted on exit, including on interrupt. Reported by @zufayu on #4870.
@zufayu asked for this in item 4 of #4870: the bar belongs in the header because a header is read on every use while an issue sinks. #5128 was opened for the measurement work and closed as premature, so without this the condition for ever leaving advisory tier is recorded nowhere. Half of it was already present -- "its judgement is stochastic and never blocks a merge". What was missing is the bar itself: the tool stays advisory until false clearance is measured and near zero for the families that raise a red verdict. That number is named as the one that matters, distinct from recall or a spot check, and stated not to exist today because no committed replay corpus establishes it. Ownership is attached to whoever proposes a rule edit or proposes gating, which is where the trigger actually is. Also completes the item-4 headline ask. The report was already optional in the code (`VALIDATION_REPORT="${3:-}"`, the no-report branch, "absence of a report is not itself a blocker", and Step 8's NOT RUN line), so nothing gated on it. The one place that read as mandatory was the front-matter description, which said to invoke with "an explicit validation report path"; it now says a review without one is a supported outcome. Takes the second option offered for exempt target classes and names them on the FlyDSL kernel row, because the combination is what item 4 objected to: a CPU-only target claims no GPU and therefore no architecture, and a bugfix with no shape dimension has no grid for correctness_s1_grid to consume. Both are structurally unable to reach PASS, so their INCONCLUSIVE is the expected output and that author must not be asked for a passing report. Co-authored-by: Cursor <cursoragent@cursor.com>
|
would update to #5142 |
amdsmi_get_gpu_activity was a hard dependency at three call sites, so a host where that single query fails degrades the whole run to NO_GPU even with idle GPUs available. @zufayu hit this on MI308X / ROCm 7.0, where the call returns AMDSMI_STATUS_UNEXPECTED_DATA (43) while enumeration, BDF, ASIC and VRAM queries all work. PICKER could not route around it because the executor re-queries the API inline rather than going through the picker's selection. Activity is now optional. A new read_activity() helper is shared by the picker and both inline probes, and gpu_claim.idleness_basis names the evidence the claim rests on: activity+vram, or vram-only when only resident VRAM separated the devices. Unknown stays distinct from zero -- the previous `gfx if isinstance(gfx, int) else 0` substitution reported an unavailable metric as a measured idle GPU. The gpu_claim skip also stops conflating "GPUs present but none idle", an environment fact, with "AMD SMI is unqueryable", a portability gap in the validator. The import fallback did not probe $ROCM_PATH/share/amd_smi, where the bindings ship on ROCm >= 7.1, so on those containers the picker exited 2 before reaching any activity query -- the same NO_GPU symptom from an unrelated cause. Separately, merge_sim applied the patch but cleanup was guarded by BASE_ACTIVE, which is set only inside the baseline path. A run that skipped correctness exited with the patch applied, and when the baseline path did run, cleanup's restore_head re-applied it, so every path left the caller's worktree patched and the next run reported not isolated-clean against the caller. The application now carries its own flag and is reverted on exit, including on interrupt. Reported by @zufayu on #4870.
@zufayu asked for this in item 4 of #4870: the bar belongs in the header because a header is read on every use while an issue sinks. #5128 was opened for the measurement work and closed as premature, so without this the condition for ever leaving advisory tier is recorded nowhere. Half of it was already present -- "its judgement is stochastic and never blocks a merge". What was missing is the bar itself: the tool stays advisory until false clearance is measured and near zero for the families that raise a red verdict. That number is named as the one that matters, distinct from recall or a spot check, and stated not to exist today because no committed replay corpus establishes it. Ownership is attached to whoever proposes a rule edit or proposes gating, which is where the trigger actually is. Also completes the item-4 headline ask. The report was already optional in the code (`VALIDATION_REPORT="${3:-}"`, the no-report branch, "absence of a report is not itself a blocker", and Step 8's NOT RUN line), so nothing gated on it. The one place that read as mandatory was the front-matter description, which said to invoke with "an explicit validation report path"; it now says a review without one is a supported outcome. Takes the second option offered for exempt target classes and names them on the FlyDSL kernel row, because the combination is what item 4 objected to: a CPU-only target claims no GPU and therefore no architecture, and a bugfix with no shape dimension has no grid for correctness_s1_grid to consume. Both are structurally unable to reach PASS, so their INCONCLUSIVE is the expected output and that author must not be asked for a passing report. Co-authored-by: Cursor <cursoragent@cursor.com>
…des #4870) (#5142) * feat(skills): add validate-kernel-pr, a runtime validation layer for kernel PRs review-pr is static: it never builds and never runs. Three failure modes are invisible to it, and a 14-PR backtest of review-pr alone found 8 of 17 known defects, so the gap is real rather than theoretical. This skill produces validation_report.json as the evidence base: merge_sim -> gpu_claim -> runtime_compat -> test_policy -> correctness -> index_width_scan -> verdict Two stages exist because a green test suite is not evidence: - correctness runs the PR's own tests AND a shape grid this skill owns (non-toy, boundary/odd, long-context). Seeding a dropped tail guard into a FlyDSL softmax kernel leaves the repo's own suite green -- its non-aligned shapes are commented out, only one aligned shape runs -- while the grid at N=2000 / N=257 fails it. - test_policy compares tolerances head-vs-base before running anything. A change that widens a tolerance while leaving the kernel path unchanged makes the suite unable to fail, and pytest still reports green. Verified against three seeded mutants plus a clean baseline: baseline passes every stage; dropped tail guard blocks via the grid alone; loosened tolerance blocks via test_policy alone; wrong index unit blocks via the repo tests. The report states what it did not do, so it cannot overclaim by omission: arch_coverage marks each arch runtime or compile-only (a gfx950 host cannot validate a gfx942 claim), isolation records the real level, and degraded_mode marks COMPILE_ONLY when no GPU was claimable. runtime_compat exists because a pinned prebuilt runtime drifts behind the tree and the resulting ImportError otherwise reads as a defect in the PR. scan_index_width.py ships alongside it; see the review-pr D9 change. * fix(skills): make review-pr's D9 trigger structural, and require validation evidence Four changes, each from a measured failure in a 14-PR backtest of this skill. 1. D9 never fires. Its trigger was a list of variable names (token_id, seq_start, batch_offset, total_tokens). Three real 32-bit overflow defects used none of them -- stride_out_batch, block_id, physical_block, context_kv_idx -- and int32/int64/overflow appears zero times in the whole verdict card for aiter#1674, which carries two of them. The trigger is now structural: an index-shaped value multiplied by a stride-shaped value on a line with no 64-bit widening. scan_index_width.py is the mechanical pre-filter and flags all three; a bare model missed the same family, so this is not something to leave to recall. 2. REPO was hard-coded to ROCm/aiter, so the skill could not review a FlyDSL PR at all. It now takes a repository argument, or owner/repo#N. 3. FlyDSL/Triton kernel PRs now require a validation_report.json and load validate-kernel-pr. A kernel PR's own green suite is not evidence. 4. The verdict card caps at 5 findings, ordered most-severe first, and states its validation evidence. This skill averaged 7.1 findings per PR across the backtest; dropping everything judged noise still left 5.4, so the cap has to be explicit. Without a report the card says [static-only review], and no finding may then assert runtime behaviour as fact. Backtest numbers for the frozen version being changed here (blob 3fdb11af, 14 PRs, 17 provenance-checked labels): 76 of 100 findings useful, 0 factually wrong, 8/17 defects found. * docs(skills): correct validate-kernel-pr's documented interface The Invocation section showed `validate_pr.sh --pr 4394`, a flag the script does not implement -- it exits 2 with "unknown arg --pr". That is the hallucinated-API failure review-pr Step 6 exists to catch, shipped in the skill meant to catch it. Document the interface that is actually implemented and verified: the caller supplies the worktree and the pytest target. Every flag named in the doc is now present in the argument parser. Also declare what is absent rather than leaving it to be inferred: - no PR fetch orchestration; choosing the right --tests target from a diff is the unsolved part, and a wrong target produces a confident green - perf and claims stages are reserved in report_schema.json and emitted by nothing, so a report today carries no performance evidence and a review must not read a missing perf stage as "no regression" * fix(skills): run the D9 index-width scan in Step 1, not mid-checklist A scan that Step 5 asks for does not happen. In a controlled 14-PR run of the revised D9 text, no session invoked the scanner and the arm caught 0 of 3 known overflow defects -- the same as the unrevised skill. Moving the invocation into Step 1's fetch block does make it run: the session transcript for aiter#1674 shows the scanner executing and its candidate list entering the context. D9 now works that list instead of asking for a scan. This is necessary but NOT sufficient, and the PR text says so: with the scan in Step 1 the arm still caught 0 of 3. What closes the gap is supplying the production-scale facts the 5xx-gate demands -- see the limitations section. * fix(skills): attribute test failures against a baseline control Found by running validate-kernel-pr end to end on a real PR for the first time (FlyDSL#969, conv3d fp8 padded-M store guard). Both the pre-fix and post-fix trees failed the same test shape -- one unrelated to the PR -- and the harness reported "the PR's own test target fails" as a blocker in both. It charged pre-existing red to the author, which is the misattribution this skill exists to prevent. With --patch, the suite now runs on the base first and a failure is only a blocker when the base is clean; otherwise it is a note saying the target is red with and without the change. The same run goes from BLOCK to PASS with the pre-existing failure recorded rather than blamed. Also carries the production-scale table as a separate file printed by Step 1 beneath the scan candidates. Its effect is documented in the PR description and is small: 1 of 3 on the target defect family, versus 0 of 3 without it. * feat(skills): add downstream-impact-check, built from a real cross-repo incident Downstream tests here are label-gated and skipped by default, so the question "can this break vLLM or SGLang" decides whether they run -- and it is normally answered from memory. aiter#4530 is why that is not good enough. It changed one file, aiter/ops/triton/_triton_kernels/moe/moe_routing/topk.py: kernel internals, no public signature touched. It broke vLLM's gpt-oss MXFP4 MoE on MI355 after the 0.1.16.post5 -> 0.1.19 bump (vllm#49361, test plan blank), needed a hotfix that names it as the companion fix (vllm#50859), and SGLang reverted its aiter pin the same week (sglang#32879). scan_downstream_consumers.py walks aiter's imports upward from the changed files and stops at the first module a downstream checkout imports. On #4530 it reconstructs the chain from the internal topk kernel through moe_routing and moe_op_gemm_a16w4 to vLLM's aiter_mxfp4_w4a8_moe.py, and correctly reports no reachability for SGLang, which does not import that subtree. The first version asked "did a public symbol change" and answered "no downstream impact" for that exact PR. Signature is the wrong question; reachability is the right one. That earlier criterion is recorded in the file so it is not reintroduced. Validated on one positive and one negative, which is thin -- the skill says so, along with the rest of what it cannot see: static imports only, five hops, the downstream's current main rather than the pinned version, and no ATOM checkout. Wired into review-pr Step 1 alongside the other scanners, because a check the reviewer has to remember to run does not run. * fix(skills): prevent false kernel validation clearance Separate advisory review output from deterministic validation, make every missing evidence path explicit, and preserve exact base/head attribution. Commit the pinned mutant corpus so these safeguards remain reproducible. * fix(skills): reject unexercised validation paths Treat shadowed FlyDSL source, unused shape-grid hooks, and base-run artifacts as inconclusive instead of allowing them to produce false validation clearance. * fix(skills): harden validation evidence boundaries Require an exercised shape-grid handshake, isolated base/head state, and runtime-backed architecture coverage so incomplete evidence cannot be accepted as a clean result. * fix(skills): bind reports to the live base Resolve the current base branch tip instead of trusting the historical PR merge base, so validation reports become stale as soon as their merge simulation does. * fix(skills): require observed execution evidence Prevent green pytest from becoming clearance unless validator-owned instrumentation observes the expected route and shapes, JUnit records real executions, and runtime identity is captured. Align report semantics with process exits and reject untrusted FlyDSL runtime rebuild claims. * fix(skills): support script validation targets Run script targets through their real entry points so successful non-pytest validation stays inconclusive instead of producing a false BLOCK. Ship the AMD GPU picker so the validator can discover an idle translated HIP device. * Treat GPU activity as optional and hand the worktree back clean amdsmi_get_gpu_activity was a hard dependency at three call sites, so a host where that single query fails degrades the whole run to NO_GPU even with idle GPUs available. @zufayu hit this on MI308X / ROCm 7.0, where the call returns AMDSMI_STATUS_UNEXPECTED_DATA (43) while enumeration, BDF, ASIC and VRAM queries all work. PICKER could not route around it because the executor re-queries the API inline rather than going through the picker's selection. Activity is now optional. A new read_activity() helper is shared by the picker and both inline probes, and gpu_claim.idleness_basis names the evidence the claim rests on: activity+vram, or vram-only when only resident VRAM separated the devices. Unknown stays distinct from zero -- the previous `gfx if isinstance(gfx, int) else 0` substitution reported an unavailable metric as a measured idle GPU. The gpu_claim skip also stops conflating "GPUs present but none idle", an environment fact, with "AMD SMI is unqueryable", a portability gap in the validator. The import fallback did not probe $ROCM_PATH/share/amd_smi, where the bindings ship on ROCm >= 7.1, so on those containers the picker exited 2 before reaching any activity query -- the same NO_GPU symptom from an unrelated cause. Separately, merge_sim applied the patch but cleanup was guarded by BASE_ACTIVE, which is set only inside the baseline path. A run that skipped correctness exited with the patch applied, and when the baseline path did run, cleanup's restore_head re-applied it, so every path left the caller's worktree patched and the next run reported not isolated-clean against the caller. The application now carries its own flag and is reverted on exit, including on interrupt. Reported by @zufayu on ROCm/aiter#4870. * Let script targets carry grid and receipt evidence A target driven by a `__main__` guard could reach neither correctness_s1_grid nor execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter `op_tests/*.py` -- the repository's standard test shape. Neither skip described a property of the target: * the route profiler is sys.setprofile plus a receipt write, and pytest was only where the hook happened to be installed; * the grid was injected solely through an environment variable, while these targets take shapes on the command line. That is a limit of the injector. Reported as skips, both read as honest abstention while actually hiding a capability gap -- the failure mode this skill exists to prevent. The probe is now installed for script targets by run_script_with_probe.py, which executes the file under runpy with run_name="__main__". It calls the probe's own hooks rather than re-implementing the profiling, so `producer` stays truthful and the tested PR still cannot forge a receipt. The probe module is generated whenever a route is named, not only on the pytest branch; without that the runner had nothing to import. --shape-arg names the target's own CLI flag for the grid. The flag is named by the caller rather than guessed, because a wrong guess appends argv the target silently ignores. The hook is held to the same standard of proof as the env-var channel: the flag must appear in an add_argument call, and the existing invalid-grid probe still has to make the target fail before the stage is credited. A receipt is now validated whenever a route was named, including when no grid was configured or the grid channel could not be established. Two branches used to abandon the receipt along with the grid, discarding evidence already collected. With no grid it asserts route execution and nothing about shapes, which is all it is then entitled to claim. report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest` to `enum [pytest, script]`, since a script target can now supply both missing stages. shape_arg is declared but deliberately not required, so reports from earlier validators stay valid. Verified on ROCm/aiter#4538 (gfx950, MI355 OAM), base 78b9440e/891788643: * script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable, receipt naming _auto_variant over the four injected production shapes * unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass * seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with correctness_s1_grid failing independently of the PR's own suite * tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one pre-shape_arg report all validate against the schema * Ask the target whether it needs a GPU, and name what that answer rests on An unclaimable GPU suppressed all four correctness stages, so a target that needs no device reported nothing rather than reporting what it could establish. The target is now asked directly: when no GPU was claimed 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 rather than a guess -- a suite guarded by skipif(not torch.cuda.is_available()) also exits 0 having proved nothing, and a verified fixture covers that case. This is deliberately an observation, not 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. Asking the target cannot be wrong in the dangerous direction: a target that needs a device fails or skips without one and stays required. PASS is deliberately NOT relaxed. `complete` still requires gpu_claim: pass, so this path cannot produce a clearance that was previously unreachable -- verified by a case where merge_sim, runtime_compat, test_policy, baseline_control, correctness_repo_tests, correctness_s1_grid, execution_receipt and index_width_scan all pass and the verdict is still INCONCLUSIVE with arch_coverage empty. gpu_claim stays `skip` because no device was claimed, which remains the fact; only the suppression is lifted. Two notes on scope, because this is a contract change rather than a defect fix. It edits behaviour @zufayu explicitly did not ask to change ("the gate is right and INCONCLUSIVE is the correct output"), and it changes an invariant the suite asserted, so test_no_gpu_is_inconclusive_and_every_skip_is_declared was updated and test_no_gpu_withholds_correctness_from_a_target_that_needs_a_device added to keep the original protection under test. It also does not by itself let a shape-less bugfix reach PASS: correctness_s1_grid has no passing path without a consumed grid. Also splits the last conflation in the gpu_claim skip. A host with no GPUs now exits 3 with "this host reports no GPUs" instead of borrowing the activity-unavailable wording, which blamed the validator for an absent device. Co-authored-by: Cursor <cursoragent@cursor.com> * State the promotion bar in review-pr's header @zufayu asked for this in item 4 of ROCm/aiter#4870: the bar belongs in the header because a header is read on every use while an issue sinks. #5128 was opened for the measurement work and closed as premature, so without this the condition for ever leaving advisory tier is recorded nowhere. Half of it was already present -- "its judgement is stochastic and never blocks a merge". What was missing is the bar itself: the tool stays advisory until false clearance is measured and near zero for the families that raise a red verdict. That number is named as the one that matters, distinct from recall or a spot check, and stated not to exist today because no committed replay corpus establishes it. Ownership is attached to whoever proposes a rule edit or proposes gating, which is where the trigger actually is. Also completes the item-4 headline ask. The report was already optional in the code (`VALIDATION_REPORT="${3:-}"`, the no-report branch, "absence of a report is not itself a blocker", and Step 8's NOT RUN line), so nothing gated on it. The one place that read as mandatory was the front-matter description, which said to invoke with "an explicit validation report path"; it now says a review without one is a supported outcome. Takes the second option offered for exempt target classes and names them on the FlyDSL kernel row, because the combination is what item 4 objected to: a CPU-only target claims no GPU and therefore no architecture, and a bugfix with no shape dimension has no grid for correctness_s1_grid to consume. Both are structurally unable to reach PASS, so their INCONCLUSIVE is the expected output and that author must not be asked for a passing report. Co-authored-by: Cursor <cursoragent@cursor.com> * Let review-pr accept a script target's PASS `Let script targets carry grid and receipt evidence` relaxed the PASS-requires-pytest assumption in the validator and in report_schema.json, but review-pr re-derives the verdict independently and still required `selection["runner"] == "pytest"`. A script target that legitimately reached PASS was therefore rejected by the consumer as self-contradictory: validation verdict contradicts its stages/findings: expected INCONCLUSIVE, got PASS Two of the three places that encode the contract were updated and the third was not, so the change silently made the evidence unusable at exactly the point it was meant to be consumed. The coverage-basis check immediately above already handled `script`, which is what made the omission easy to miss. Verified against the report from ROCm/aiter#4538 on gfx950 (script target, --shape-arg -s, PASS 9/9): rejected before, "validation report accepted for head 22850902..." after. tests/test_validator.py 25 passed. * Have review-pr triage validation need, and run it when it can A judgement the checklist asks for mid-review is a judgement that does not get made, so Step 1 now classifies the changed paths itself and, when the PR changes runtime surface and ships exactly one test target, invokes bin/validate-kernel-pr and consumes the result through the unchanged identity gate. Producing a fresh head-bound report is not the same act as adopting one found on disk, so "never auto-load ./validation_report.json" still holds. The verdict line gains the two states it was missing. A README fix used to report the same "NOT RUN" as an unvalidated kernel rewrite, which teaches a reader to skip the line; "N/A - no runtime surface changed" and "required but not run" are different facts and now read as such. Co-authored-by: Cursor <cursoragent@cursor.com> * Call the validator from the skill, not from bin/ The auto-run reached outside .claude/skills for bin/validate-kernel-pr, whose only addition over validate_pr.sh is a PR-number front end: it re-fetches the diff and re-resolves the base tip. Step 1 already holds both, so it now creates a detached worktree at the base it recorded and hands that, its own pr.diff and the head OID straight to the validator. Deriving those a second time was not merely redundant. main can advance between the two gh api calls, and the resulting report names a base the review's own identity gate then rejects as stale -- a self-inflicted failure on a report we just produced. Handing over the recorded base makes repo.base agree by construction, since the validator reads it as rev-parse HEAD of that worktree. The dependency also now sits beside the index scanner and the report schema, and fails loudly like they do instead of warning and continuing, since a skill tree missing one of the three is broken rather than differently configured. Finally, each way the run can give up records why. Triage's blocker only covers attempts never made, so a run that was attempted and failed printed "required but not run: None" -- the exact uninformative verdict line this step exists to remove. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(skills): make the D9 index-width scan an AST pass with no name lists The scanner matched a regex over raw diff text whose operand character class contains neither a comma nor a space, so it could not span a broadcast subscript -- and ptr[:, None] * stride is the standard Triton pointer-arithmetic idiom. Measured on aiter#4978, the PR that introduced the moe_wgrad overflow later fixed by #5132: both real defect lines were missed, the one moe_wgrad candidate emitted was an already-int64 line, and 390 candidates were produced overall. The defect lived in the syntax tree; the scanner was reading text. It also matched operand names against two hard-coded lists. Both are gone. A multiplication is now a candidate when it feeds an addition chain, at least one operand derives from a non-constexpr parameter of the enclosing kernel, and no operand is widened -- counting a widening carried in from an earlier line, which is how #5132 fixes it. Post images come from --source-root, else the diff's own index blobs; a file whose post image cannot be recovered is reported as NOT SCANNED, never dropped, because an empty candidate list that silently excluded files is not evidence of absence. On #4978 it now reports 4 of 4. On #5132 it reports none. review-pr's D9 text claimed a structural trigger while still defining index- and stride-shaped by name; it now describes what the scanner actually does. * feat(skills): deliver the S1 shape grid through pytest parametrization Applying this validator to four open aiter FlyDSL kernel PRs (#5167, #5141, #4992, #5011) reached INCONCLUSIVE on all four, for one reason: correctness_s1_grid skipping is on its own enough to force it, and the stage is inert on this repository's dominant test shape. Zero of the seven files in op_tests/flydsl_tests/ read a shape env var or parse a shape CLI flag; every one declares its shapes as literals inside @pytest.mark.parametrize. --shape-argnames adds parametrization itself as a third channel, via a generated plugin whose pytest_generate_tests substitutes the grid. The burden of proof is unchanged: a deliberately unusable grid must still make the target fail, so a target that ignores the grid produces a skip and never a pass. The invalid-grid row keeps its arity and poisons its values, so the probe fails inside the target rather than inside the plugin. A mark binding the requested names together with others is refused rather than stripped: removing it would leave those others as unfilled fixtures and the target would error for a reason unrelated to the kernel. Two channel defects found alongside it. The env probe overwrote the CLI probe unconditionally, so supplying both flags discarded a working CLI channel and then reported the env channel's absence as the reason no grid ran; the probes are now independent and run_pytest dispatches on the channel that actually probed positive. And the skip text read 'kernel exposes no configured shape override' even when the cause was the validator ignoring the named channel -- grid_channel_reason now names each channel tried, what was found, and what the target does offer instead. * fix(skills): stop reporting pass on evidence that was requested and never collected Three stages decided their status from 'this code path completed' rather than 'evidence was actually collected', and published pass on empty fields. execution_receipt: the profiler sampled f_locals only on the call event, so it saw parameters and nothing else. A route deriving its shapes in its body -- the common case for a kernel whose arguments are tensors -- could never satisfy --shape-vars, and the receipt reported pass with executed_shapes: []. Observed on three separate PRs. The frame is now sampled again on return, keyed by the frame OBJECT rather than id(), whose values are reused once a frame is freed; the first version of this fix used id() and dropped the second of two sequential calls. When capture was requested and produced nothing, the receipt now carries a shape_capture block saying the receipt asserts route execution and makes no claim about shapes. test_policy: the tolerance pattern matched only literal atol=/rtol=, so a file declaring DEFAULT_REL_TOL = 2e-2 reported zero tolerances and the check passed on an empty list. Loosening a named constant is exactly the m2 mutant this stage exists to catch, and it was undetectable. Named constants are now captured, and a tolerance passed by name is recorded so an empty list is never read as 'none'. * fix(skills): claim a GPU that is actually free, and name the basis honestly PICKER resolved from PATH before the picker this skill ships. The shipped picker is what prints idleness-basis:, so a PATH copy that omits it won by default and the report published idleness_basis: 'unknown' beside a concrete gfx_activity_before_pct: 0 -- an unavailable reading presented as a measured idle one, which SKILL.md argues against in its own text. Resolution is now explicit PICKER, then shipped, then PATH. The picker is deterministic and returned one device, and a contended flock -n gave up. Concurrent validators therefore all selected the same GPU and all but one reported no idle GPU while seven sat idle; observed twice in one run with every lock free between attempts. The picker gained --all, and the executor walks the eligible ranking until a lock is taken. * test(skills): cover the validator changes Thirteen tests for the AST scanner, the three grid channels, receipt honesty and the GPU claim. Verified against the pre-fix sources: 5 of 6 scanner tests and all 8 of the executor tests fail there, each for the intended reason. The exception is the earlier-line-widening test, which passes pre-fix coincidentally -- the old regex also failed to span token_row[:, None], for its own wrong reason. test_json_count_is_deduplicated is rewritten rather than restored: it asserted the name-list contract that was deliberately removed. * fix(skills): stop the validator from publishing its own faults as the PR's Round-2 application to five real FlyDSL kernel PRs found six ways a fact about the run was published as a defect of the code under test. Three were introduced by the pytest grid channel itself. --shape-argnames with exactly ONE name passed argnames as a list while unwrapping rows to scalars. pytest sets force_tuple only for a string argnames, so collection died with "object of type 'int' has no len()" and the executor published that crash as 'the PR adds this target and its independent shape grid fails' -- a BLOCK verdict against the author, on three separate PRs. One name is the dominant shape in the targets this channel was built for. The partial-overlap guard was evaluated over the whole FILE, so an unrelated test whose parametrize mark overlapped the requested names disabled the channel for every test in it. The plugin decides per metafunc; the probe now decides per test function, as it always should have. A relative --patch was resolved against the caller's cwd in one place and the worktree in another, so merge_sim reported 'patch does not apply to the current base' and returned BLOCK. Paths are resolved once, before anything moves. head-repo and head-grid shared one receipt path through their phase directory, so a grid run that crashed during collection erased a receipt that had already proved the route, and the report then said the route never ran. Receipts are per label, and one helper decides which speaks for the head run: a phase that observed nothing never speaks over one that observed something. The invalid-grid probe treated any non-zero exit as proof the grid was consumed. On a held-out PR whose module could not be imported, and again when the plugin itself crashed, the channel was credited although no shape reached the kernel. The control run must now pass before the probe's failure means anything. JUnit errors were counted as executed tests, so a collection error credited arch_coverage with runtime coverage on the strength of the error itself. WIDEN_ATTRS was matched case-sensitively; FlyDSL writes fx.Int64(...), so explicitly widened FlyDSL code was reported as an overflow candidate. Found only on the held-out PR -- the four PRs used while fixing all happen to write tl.int64. Two long-standing gaps closed alongside them. --tol-table was parsed, validated and compared against nothing; it now yields a finding when the suite accepts error beyond the loosest reference supplied. And runtime_identity published native_artifacts: [] as a measurement when the probe runs before the target and imports only aiter -- it now states what that emptiness does and does not mean. The pytest channel delivers the grid as test PARAMETERS while the receipt records the ROUTE's locals. Requiring containment across those two vocabularies produced a missing-shapes skip on a run whose every grid case passed; the requirement is recorded, the cross-namespace assertion is not made, and review-pr's ingestion gate knows the difference. * test(skills): cover the false-attribution fixes Twelve tests for the second batch. All twelve fail against the pre-fix sources and none pass there, so each one discriminates. The single-name --shape-argnames test asserts the injected values reach the route rather than the target's own literal, which is what the false BLOCK hid. One needed the caller's directory nested two levels rather than one: with a single level, a relative --patch resolves to the same file whether it is read from the caller's cwd or from the worktree, and the defect is invisible. The fixture's synthetic HIP index moved from 7 to 57. The validator flocks /tmp/gpu-<index>.lock, and an unrelated job on this host holds the lock for 7, so every GPU-claiming test degraded to NO_GPU for a reason that had nothing to do with the code. No assertion was weakened; only the synthetic device number changed. * fix(skills): gate the pytest grid on parametrize VALUES, not just argnames Round 3 found the false BLOCK on #5141 was not gone. The plugin crash was fixed; the crash had moved into the author's file, which makes the misattribution harder to see rather than less real. The gate read a parametrize mark's argnames and never its values. A target parametrizing a single `case: dict` therefore passed it, the validator substituted integers, and the target raised `TypeError: 'int' object is not subscriptable` -- published as '[blocker] the PR adds this target and its independent shape grid fails', against an author whose own 138 tests passed in the same report. SKILL.md already documented that such targets stay INCONCLUSIVE; the executor did not implement it. Both the AST gate and the plugin now require the mark's own values to be scalar cells, and values of unknown shape count as not scalar -- a grid this channel cannot express must cost an INCONCLUSIVE, never a blocker aimed at an author. Three more false attributions closed with it. review-pr asserted `executed == tests - skipped` while the validator had been corrected to subtract errors. They disagree only when errors > 0, which is the shape of a report carrying a real runtime blocker -- so a report that had correctly found one was rejected at ingestion and could not be used. Worse than the overclaim it replaced, and caught only by re-running the case that had produced a true BLOCK. The invalid-grid probe's causality guard had been added on the base side only, so the head branch still credited the channel when the target failed for an unrelated reason. And review-pr turned a grid the caller supplied into a requirement even when no channel had carried it, so the receipt's honest empty list read as a contradiction and discarded exactly the runs carrying the accurate diagnostic. The scanner claimed kernel scope in its docstring and its class name while visiting every Python function: on a held-out PR all four candidates were host-side float FLOP accounting and none were in the 916-line kernel. Device scope is now a predicate -- a constexpr-annotated parameter, or a jit/kernel decorator -- and host-scope hits are listed separately rather than dropped, because that predicate is a spelling test and a wrong answer should cost a glance, not a miss. The index-width scan now reads post images from the applied worktree, so modified files are examined rather than disclosed as unscanned. gpu_requirement no longer reports a conservative default in language that reads as an observation. * fix(skills): scope the scan to the enclosing kernel, and reject a caller's typo at the door Two defects found by re-running the cases that had produced decisive verdicts. A grid whose rows did not match --shape-argnames produced a BLOCK. The arity check lived inside the plugin generator, whose exit status run_pytest never read: the generator failed, the PREVIOUS phase's plugin file survived on disk, head-grid re-ran the invalid-grid sentinel it still carried, and that failure was published as 'the PR adds this target and its independent shape grid fails'. A check whose result nobody reads is worse than no check -- it creates the appearance of a guard while the system falls back to stale state. The arity check now runs at argument parsing, where a caller's mistake cannot become a finding about the PR, and the generated plugin is removed before regeneration so no phase can inherit another's. The index-width scan was classifying real kernel code as host-side. _scan_body used ast.walk, which descends through nested defs, so every expression inside a nested helper was scanned in the ENCLOSING function's scope: its parameters, its widened locals, its device/host verdict. Four candidates inside a @flyc.kernel body were demoted to host scope, and a miss matters more here than noise. The walk now stops at function boundaries, and scope, parameter provenance and widening follow the scope chain -- a nested helper closes over its kernel's parameters, so it inherits them. Fixing that exposed a second bug it had been masking: visit_FunctionDef recursed with generic_visit, which visits a statement's CHILDREN, so a nested def that IS a statement of the body was never dispatched at all. Evidence unchanged where it should be and corrected where it was wrong: aiter#4978 still reports 4 of 4 on the moe_wgrad overflow, #5132 still reports none, #5141's nested-kernel arithmetic moves from 4 host to 5 device, and the held-out #5025's host-side FLOP accounting stays out of the device list. Two abstentions also stated the wrong cause -- 'does not take all of these as test parameters' for a target that does take them, when the real refusal was non-scalar mark values. The probe now returns the refusal it actually made. * fix(skills): let auto-validation reach the pytest shape channel The auto-validation block added on this branch forwards REVIEW_SHAPE_ENV and REVIEW_SHAPE_ARG but not the parametrization channel, so the one channel that reaches this repository's dominant test shape was unreachable from the caller that was just wired up. None of the seven files in op_tests/flydsl_tests/ reads a shape env var or parses a shape flag; all declare their shapes as literals in @pytest.mark.parametrize. Also records where a caller learns which channel a target actually offers, so a wrong guess costs one run rather than a reading of the target's source. * Measure a kernel PR's cost, not just its correctness A kernel PR could pass every stage while running slower, because nothing timed it. Add a `perf` stage: bench the target once with the patch reversed and once with it applied, both inside the base phase's lock on the same GPU, and compare. The comparator (scrape_perf.py) reads both of aiter's timing formats -- the `df.to_markdown()` tables from `@benchmark`/`run_perftest`, and the older `[perf] ... ck avg: N us` lines that 11 kernel targets print. A scraper that only knew tables would burn a full base+head run on those and report nothing. Three choices worth recording, each forced by a measurement rather than by taste: - Take the *minimum* over per-column medians, not the mean. A PR touching the ck path leaves `torch avg` at 1.0, and a mean would let that untouched reference column bury the regression next to it. - Run each side three times and keep the best sample per cell. Five warm runs of unchanged test_layernorm2d.py gave `torch avg` 14.24 14.64 14.18 14.23 14.31 (1.03x spread) but `ck avg` 13.10 20.98 20.70 13.28 13.17 -- 1.60x, and bimodal rather than noisy. Single-shot at a 0.95 threshold would have called unchanged code a regression about half the time. Best-of-3 brings the same data to 1.014x, worst ratio 0.986. - Snapshot the worktree around the timing run and roll back only the paths it dirtied. Without this a bench that writes a results file (see op_tests/test_gemm_a8w8.py, test_moe_2stage.py) fails the base phase's cleanliness check, which sets BASE_READY=0 and silently skips head correctness entirely. Measured: the same target went PASS with --no-perf and INCONCLUSIVE with perf on. A perf stage that quietly disables correctness is worse than no perf stage. A regression under the threshold writes a should-fix finding, so the deterministic verdict becomes NEEDS_WORK and the process exits 1. review-pr's identity gate now cross-checks the stage's status against its own numbers, so neither a laundered pass nor a laundered fail survives, and its P6 rule fires only when no usable stages.perf exists. The harness detector is shared in spirit by both skills and pinned by a test, since a silent disagreement there means one skill demands a measurement the other cannot supply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Describe the validator this branch actually ships The perf stage landed six commits ago and its own skill still says it does not exist: "the schema reserves both -- and the script emits neither. A report today carries no performance evidence, and a review must not read the absence of a `perf` stage as 'no regression'." That instructs a reader to disbelieve a stage that now measures, gates, and can turn a verdict into NEEDS_WORK. review-pr says the opposite in three places, so the two skills contradicted each other on the one stage a reviewer is most likely to act on. Document `perf` where the implementation lives: the flag table gains --perf-args and --no-perf, the env knobs gain PERF_TIMEOUT, PERF_REPEAT, PERF_THRESHOLD and PERF_MIN_ROWS -- all of them ${VAR:-default} overrides with their own argument validation, so they were public already and only undocumented -- and the stage list gains a section that says why best-of-N is what makes a 0.95 threshold usable and why every non-measurement path reports `skip`. The verdict section now states that PASS does not imply a timing comparison happened. What remains genuinely unimplemented is narrower than the old bullet claimed: reproducing the specific numbers a PR description states. That is now what "Not implemented yet" says. Three smaller corrections in the same vein: - "review-pr never builds and never runs" has been false since review-pr started invoking this script itself. The split is at judgement, not at invocation, and the text now says so. - The comment describing bin/validate-kernel-pr as an existing wrapper outlived the wrapper; bin/ is not in the repository. The reasoning it carried -- do not re-resolve a base that main can advance past -- is worth keeping, so it stays, phrased about a front end that does not exist rather than one that does. - The mutant manifest's tolerance_table disagreed with the file it describes: test_softmax.py at the pinned base uses f16=1e-2 and bf16=2e-2, not 2e-3 and 1e-2. Inert today, because reference tolerances are recorded and not compared, which is exactly how a wrong number survives unnoticed until something starts reading it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop the report schema promising evidence nothing produces report_schema.json is the contract review-pr validates against, so a field declared there reads as a shipped capability. Three did not hold. `claims` declared a stage that reproduces the numbers in a PR description, with an `unreproducible` array. No producer writes it -- not validate_pr.sh, not validate_evidence.py, not scrape_perf.py. Its only other mention was an allowlist entry saying it may be absent. It is removed rather than left standing; the honesty rule that promised `[unreproducible]` tagging now describes what `stages.perf` actually carries. `source_sha` was hardcoded null while the same honesty rules listed source SHA as recorded evidence. It is fillable: the resolved module has a path, and that path is usually inside a checkout. Now it records that checkout's commit, suffixed `-dirty` when the tree has uncommitted changes -- which the head phase always does, since it runs with the candidate patch applied, so the bare commit would name the base and describe a tree that is not the one under test. A module resolved from an installed package stays null: attributing a commit to a prebuilt binary is precisely the provenance claim this skill refuses to make. `reference_tolerances` is recorded and never compared, so --tol-table influences no verdict. Wiring it up is not possible as the data stands -- the table is keyed by dtype while the scraped tolerances are a positional list, with nothing to join them on -- so the field now says that it is context and not a gate, rather than implying otherwise by sitting next to the comparison that is one. Also collapses the PASS gate's two byte-identical correctness blocks into definitions/passing_correctness_stage, and has its `stats` $ref the execution_stats definition that was already there instead of respelling it inline. 74 duplicated lines in the clause that decides what earns a clearance, where divergence between the copies would change the answer silently. Verified equivalent: a clean PASS report still validates, and executed=0, failures=1, errors=1, exit=1, status=skip, missing stats and a stats object missing `skipped` are each still rejected on both stages. Adds the fields the producers already emit but the schema never described -- gpu_claim.post_run_note and .requirement_note, perf.regressed_rows[].base_repeats and .head_repeats -- which passed only because additionalProperties defaults to true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Name the channel the shape grid actually travelled on --shape-arg was added so script targets could receive an S1 grid, but the code around it still assumed the grid could only be an environment variable. Three consequences, one of them visible in a report. GRID_HOOK_OK was assigned twice. The --shape-arg AST scan set it, then an unconditional second block re-derived it from the --shape-env scan. Supplying both flags therefore proved the env var while run_pytest put the shapes on argv -- the report attested a channel the run did not use. Only-one-flag runs worked, which is why this survived. The channel is now resolved once, before either scan, and --shape-arg wins for a script target because that is what run_pytest does. A --shape-arg naming a flag the target does not accept fell through to the branch that reports "no shape grid was configured". Both cases skip, so the reason is the only thing distinguishing a validator that could not find the hook from a caller who never asked for one -- and the wrong one of those is an environment fact standing in for a capability gap, which is the overclaim-by-omission this skill exists to prevent. The branch conditions now test whether a grid was requested at all, so the same input reports hook-not-found, names the flag, and records test_selection.grid_channel. --shape-arg on a pytest target gets its own answer rather than the same silence: argv never reaches it. The runtime skips likewise said "shape environment variable" whichever channel was in play. They name the real one now. run_pytest took "$SHAPE_ENV=$GRID" and re-split on the first `=`. For a CLI-only run that worked only because an unset SHAPE_ENV left a leading `=` the split then removed, so the shapes rode inside a string shaped like the channel they were not using. It takes the grid value directly and picks the channel from the same decision that credited the hook. Collapses the two grid-less branches, which were a 17-line receipt block duplicated verbatim with two different skip strings. The new test fails on the previous script for the reason that matters -- 'no configured shape override' unexpectedly found -- not merely on the field added to carry it, which is why the note assertions come before the grid_channel one. 40 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Find the index-width overflow the D9 scan was written for The scanner matched a regex over raw diff text whose operand character class contains neither a comma nor a space, so it could not span a broadcast subscript -- and `ptr[:, None] * stride` is the standard Triton pointer-arithmetic idiom. It also matched operand names against two hard-coded lists. Measured on aiter#4978, the PR that introduced the moe_wgrad overflow later fixed by #5132: 390 candidates emitted, none of them the four real defect lines, and the single moe_wgrad candidate was `off_expert * stride_dwe` -- a line whose operand is widened with `.to(tl.int64)` forty rows earlier. Recall 0 of 4, on the defect family the rule is named for. The defect lived in the syntax tree; the scanner was reading text. Rewritten as an AST pass, taken from #5174. Both name lists are gone. A multiplication is a candidate when it feeds an addition chain, at least one operand derives from a non-constexpr parameter of the enclosing kernel, and no operand is widened -- counting a widening hoisted to an earlier line, which is how #5132 fixes it. Scope follows the scope chain rather than ast.walk, so a nested helper inherits its kernel's parameters instead of being scanned in the wrong frame. Post images come from --source-root or the diff's index blobs; a file whose post image cannot be recovered is reported as NOT SCANNED rather than dropped. It reports 4 of 4 on #4978 and 0 on #5132. Two defects fixed on top of what #5174 ships. AugAssign was never scanned. _scan_body matched ast.BinOp with an Add op, but `a_ptrs += BLOCK_K * stride_ak` is an ast.AugAssign, which is not a BinOp -- so the most common Triton pointer-advance idiom was a blind spot. The constexpr filter in _is_compile_time was written for exactly that expression and could never reach it. Adding it moves #4978 from 296 candidates to 306; the moe_wgrad four and the #5132 zero are unchanged. WIDEN_CALL_RE was left behind by the regex implementation this replaces and had no remaining reader. The --json path returned `0 if not unscanned else 0`. It stays 0, because both callers require that: #5174's own test asserts the plain and --json runs exit 0, and validate_pr.sh turns a non-zero exit into a skipped stage rather than a louder finding. Making the exit status honest needs validate_pr.sh to pass --source-root or to read the `unscanned` field, so the reason is recorded where the return is instead of a ternary whose two branches were the same value. Base's IndexScannerTests asserted the name-list contract that was deliberately removed, down to a JSON key that no longer exists; #5174's rewritten IndexScannerTests and ScannerScopeTests replace it. 48 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Stop the receipt reporting pass on shapes it never captured The probe sampled a frame's f_locals on the `call` event only, so it saw parameters and nothing else. A route that derives its shapes in its body -- the common case for a kernel whose arguments are tensors -- could never satisfy --shape-vars, and the receipt published `pass` beside `executed_shapes: []`. Deciding status from "this code path completed" rather than "the evidence was collected" is the failure this skill argues against in its own text, and it was in the probe. From #5174: the frame is sampled again on return, keyed by the frame OBJECT rather than id(), whose values are reused once a frame is freed and would drop the second of two sequential calls to the same route. When capture was requested and produced nothing, the receipt now carries a shape_capture block saying it attests route execution and makes no claim about shapes, so no consumer can read the empty list as evidence that no shapes were needed. The evidence checker stops counting errors as executed tests. Errors are collection and fixture failures where the body never ran, so a target that failed to import was crediting arch_coverage with runtime coverage on the strength of the collection error itself. This also makes the checker agree with review-pr's ingestion gate, which already asserts `executed == tests - skipped - errors`: they disagree only when errors > 0, which is the shape of a report carrying a real runtime blocker, so the two had to be corrected together or a correct report would be rejected at the door. runtime_identity stops publishing `native_artifacts: []` as a measurement. The probe runs before the target and imports only the module under test, so an empty list describes that import's reach and not the kernel's; the basis string now says which. validate_receipt gains a `--grid-channel` argument, used to suppress the cross-namespace containment assertion when the grid travelled as pytest parameters. It defaults to empty and validate_pr.sh does not pass it on this branch, so that branch is inert until the pytest grid channel lands. 50 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Let the GPU picker rank every eligible device, not just one The picker returned a single device and the executor took a contended `flock -n` as a refusal. Concurrent validators therefore all selected the same GPU and all but one reported "no idle GPU" with seven sitting idle -- observed twice in one run with every lock free between attempts. A deterministic picker and a non-blocking lock are only safe together if the caller can fall through to the next candidate. From #5174: --all prints the eligible devices in ranking order rather than the top one. The executor side of this, walking that ranking until a lock is taken, lives in validate_pr.sh and is not in this commit, so nothing consumes --all yet; the default single-device output is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix(skills): stop the validator reporting coverage it did not obtain Applying validate-kernel-pr to ROCm/aiter#4538 (a rewritten gfx950 FP8 MQA logits kernel) returned PASS in 93 seconds. The PR has a reproducible correctness failure -- flydsl_fp8_mqa_logits asserts at num_heads=16 on gfx950, where the pre-PR base computes it correctly -- and its central performance claim went unmeasured. Every gap below was found by running the tool, and each fix owns a generic invariant rather than that PR's shape. A shape grid that duplicates the target's own defaults is not a control. All three requested cells were already in the target's `--shapes` default, so the "independent" grid re-ran a strict subset of correctness_repo_tests and was credited `pass` -- the exact duplication SKILL.md says the stage exists to prevent. Grid cells are now compared against the flag's declared default; `test_selection.grid_independence` reports the outcome, and a passing duplicate is downgraded to `skip`. A duplicate that FAILS keeps its `fail`. A coverage axis the shape flag cannot reach could not be requested at all. `--grid` is one ordered tuple on one channel. A target whose head counts, dtypes or window modes live on separate flags had entire configurations unreachable however the grid was spelled. `--axis NAME=FLAG:v1;v2` adds them, under the same burden of proof as `--shape-arg`: the flag must be declared in the target's own argparse AND observed refusing a deliberately invalid value. An axis that fails either is dropped and NAMED, never dropped quietly. A script target's `executed: 1` stood for 56 graded cases. It is the same number a silently-returning target produces, and it earned the same `arch_coverage: runtime` on basis `script-exit-zero-with-output` -- a statement about the process, not the kernel. `stats.observed_work` now carries route calls counted in that run's own receipt, and a script run that observed no work earns no architecture credit. head-repo and head-grid shared one execution receipt. The second erased the first, and with the grid cells a subset of the defaults a receipt written by either run satisfied `--grid`. One receipt per run; `execution_receipt.receipt_scope` names the one that was published. "The PR adds this target, so base has nothing to time against" is not true. The file being new does not make the code it drives new. When the target only exercises an entry point that exists on base, the target is transplanted into the base tree and times the pre-PR implementation through the same harness. Because that spans two trees, `--perf-control-column` names a column the patch does not touch and the stage skips unless it reproduces within PERF_CONTROL_TOL. On #4538 this yields 56 matched rows, the kernel column 1.29x faster on head, and the untouched Triton control within 0.0%. scrape_perf matched 0 of 56 comparable rows. A header with no recognised unit was treated as identifying, but aiter bench tables print unlabeled MEASUREMENTS (`flydsl rel`, `triton err`, `speedup`) that differ between base and head by construction. The strict key is still tried first; only if it matches nothing is a relaxed key used, dropping identity columns whose cells are non-integral. `row_key_basis` records which. The target ran with the reviewer's environment attached. `env VAR=... cmd` adds to the inherited environment, so unmerged third-party code could read every token in the calling shell -- reproduced, with real credentials reaching a stage log. The target now runs under `env -i` plus a name/prefix allowlist and a secret-shaped denylist, and `isolation.target_environment` reports what was passed through. The exit code was read back out of `--out`. That made a previous run's report a fallback source of truth: a run killed before finish_report exited on the earlier run's verdict. `--out` is removed at startup and the code comes from a verdict file inside this run's own $WORK. Ten regression tests, each verified to fail against a8d46dcb0 and pass here; the suite is 62 and green. The synthetic picker moved off a real device index, which was making the suite contend with genuine validations on the same host. * fix(skills): let the mutant replay reach its own verdict check The driver reverse-applied each case's patch unconditionally after the run. But validate_pr.sh already hands the worktree back with the patch reversed, so that second reverse-apply failed with "patch does not apply" on EVERY case, and under `set -euo pipefail` it aborted the driver before the summary block that compares each verdict against the manifest. The mutants were being discriminated correctly the whole time -- m0 PASS, m1 and m3 BLOCK in correctness, m2 BLOCK in test_policy -- but the suite could not say so, and its exit status was 1 regardless of the result. A regression asset that cannot report itself green is not one. Reverse only when `git apply -R --check` says the patch is still applied. End to end on ROCm/FlyDSL@421935cc with the installed flydsl 0.3.1 (verified identical to that commit's python/flydsl tree), the replay now prints "mutant replay matched all expected verdicts". Found by running the committed replay as a held-out case, not by reading it. * fix(skills): say which fact a skipped comparison rests on Three defects that only appeared on ROCm/aiter#5172, a fresh held-out PR frozen before any of this work started and never looked at while fixing. A requested axis vanished from the report when it could not be honoured. #5172's target is collected by pytest, so argv-borne axes cannot reach it and axis_state became "unusable" - correct - but test_selection.axes was published as [], so a reader could not see that an axis had been requested at all. An empty list beside a non-"none" state loses the request itself, which is exactly the silently narrowed test space these fields exist to make visible. Axes are now recorded as asked for, with hook_proof "not-evaluated" when the run never got far enough to scan the target for the flag. grid_independence published a false statement about the target. Its default reason, "the channel exposes no declared defaults to compare against", was emitted whenever the comparison did not happen - including when the channel had been demoted for an unrelated reason. On #5172 that is untrue: the target's -c flag does declare a default list, and all three requested cells were outside it. The reason now names which of the several skip paths applied. "Red on both baseline and head" is an attribution, not an explanation. #5172's target defines pytest nodes AND parses argv in its module body, so pytest imports it at collection with its own argv and argparse exits. The file is green run as a script - the perf stage in the SAME run proves it, three times per side. The report said the target was red and gave no hint that the runner selection was the cause. test_selection.runner_risk now names that structurally, and a run that executes nothing under the selected runner cites it. Thirteen new regression tests in all, each verified to fail against a8d46dcb0; the suite is 63 and green. * fix(skills): stop charging a runner-selection artefact to the PR author A second script-target case, ROCm/aiter#5081, was picked after the first held-out round showed that the mechanisms added here had only ever executed their positive path on ROCm/aiter#4538. Selection was structural and outcome-blind ("an aiter PR that ADDS an op_tests script target with a shape flag and a sibling axis flag"); it immediately produced a false blocker. "Defines a test* function" is not "pytest can collect it". aiter's dominant op_tests convention is a SCRIPT whose worker happens to be named test_<op>(m, d, dtype) and is called from main() with real arguments. pytest collects it, cannot supply the parameters, errors -- and the validator published "the PR adds this test target and it fails on head" against an author whose target is green as a script with the very shapes the run had requested (verified by hand: -m 97 513 -n 512, all checkAllclose passed). A test* function is now only counted as a pytest node when it takes no required positional parameters or carries a parametrize/fixture decorator. #5081 selects `script`, the grid and the axis reach it, and no blocker is raised. `--runner` lets a caller settle the case the classifier still gets wrong, recording both the forced choice and what selection had said. A route behind functools.wraps could never be observed. A frame's identity is f_globals["__name__"] + ":" + f_code.co_name, and wraps copies __name__ onto the wrapper object while leaving co_name alone -- so aiter's whole @compile_ops family executes as `aiter.jit.core:wrapper` and naming the op matched nothing. The probe now resolves the declared route to code objects through the __wrapped__ chain; the string match remains as a fallback for a module that cannot be imported. On #5081 the receipt goes from observed '' to a real observed route, and INCONCLUSIVE now rests on the honest remaining gap (a (*args, **kwargs) dispatch wrapper binds no shape locals, so the receipt reports missing shapes rather than passing). Two fields answered the same question differently. When a proven axis rescues a duplicate shape grid, the override reached stages.correctness_s1_grid.independence but not test_selection, which had been written earlier -- so one report said "duplicates-target-defaults" and "adds-coverage" at once. Both are now written from the same decision. A control-gated perf stage still published the numbers it had rejected. `status: skip` beside a median_ratio and a regressed_rows list reads as a regression that was merely not acted on. When the control column moves outside PERF_CONTROL_TOL the ratio fields are dropped. Verified on #5081 end to end: novel grid -> grid_independence adds-coverage with axis `-n` proven (target_defaults 384/768/1024/4224, novel 512/1536); duplicate grid + no axis -> the stage downgrades to skip and both fields agree; duplicate grid + proven axis -> rescued, and they still agree. Seventeen new regression tests, all verified to fail against a8d46dcb0; the suite is 68 and green. --------- Co-authored-by: Jin Pan <jin.pan@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: zhiding512 <zhiding512@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Motivation
review-pris static and stochastic: it can surface useful risks, but it cannot establish runtime behavior or act as a merge gate. The original D9 rule also missed three known 32-bit index-overflow defects because its trigger depended on variable names rather than index × stride structure.This PR separates the two authority levels:
review-pris explicitly advisory. Its LLM judgement never blocks a merge.validate-kernel-prsupplies reproducible execution evidence. Only a deterministic validator finding may be treated as a blocker.scan_index_width.pysupplies structural D9 candidates; production-scale evidence is still required before a candidate becomes a defect.Validation pipeline
merge_sim → gpu_claim → runtime_compat → test_policy → baseline_control → correctness_repo_tests → correctness_s1_grid → execution_receipt → index_width_scan → verdictmerge_simapplies the exact PR diff to the live base-branch tip.gpu_claimuses the shippedpick-idle-gpu.pyto sample activity plus resident VRAM, maps the selected HIP index to the matching AMD SMI device, and holds the lock for the full run; metadata probes reuse the picker's ROCmamdsmiimport fallback.runtime_compatrecords interpreter, package, and loaded native-library identities. A prebuiltPYLIBcannot validate FlyDSL runtime/build-input changes because trusted source-to-binary provenance is not implemented.test_policycompares tolerances and disabled shape rows against base before execution.baseline_controlruns the same target with the exact patch removed under base-only caches.path::nodeselector or top-leveltest*/Test*definition uses pytest; a file with only a__main__entry point runs as a script; a target with neither is skipped.correctness_repo_testsandcorrectness_s1_gridrecord runner-specific execution counters; pytest uses JUnit counts, while scripts use process-based counts. All-skipped or unrunnable targets are not passes.test_selection.runnerandrunner_reasonpreserve the choice, andarch_coverage_basisdistinguishes pytest JUnit execution from script exit/output evidence.execution_receiptis generated by a validator-owned, private pytest profiler. Because this receipt is pytest-only, a successful script target tops out atINCONCLUSIVE;PASSstill requires the expectedmodule:functionroute, an observed route symbol, and every grid shape reconstructed from the route's actual call locals.index_width_scanrecords deterministic index × stride candidates for D9 review.verdictisBLOCK,NEEDS_WORK,INCONCLUSIVE, orPASS; process exits are respectively1,1,2, and0.The final review card keeps
Review (advisory)andValidation (deterministic)on separate lines.Evidence boundary
A report is accepted only when its PR head, live base tip, and diff SHA-256 match the current PR.
review-prruns the canonical JSON schema, checks JUnit arithmetic, runtime identity, route/shape receipt, findings, and process-exit contract, then independently recomputes the verdict.The executor also:
AITER_JIT_DIRroots, disables Python bytecode writes, and aborts the head run if base leaves artifacts;INCONCLUSIVE, notPASS, when GPU, runtime, baseline, pytest, route, or shape evidence is missing;downstream-impact-checkwas removed because it addresses a separate incident and should be reviewed independently.Reproducible evidence
The committed corpus pins
ROCm/FlyDSL@421935cc6f09fd9b27d5d5ae52e0960e18834bd5and contains one behavior-neutral control plus three seeded softmax defects.On MI355X (
gfx950, HIP 1, BDF0000:05:00.0), with pre/post GFX activity at 0–1%:PASS, process exit0, route and all four shapes observed;BLOCK, process exit1, from the independent shape grid;BLOCK, process exit1, fromtest_policy;BLOCK, process exit1, from correctness.Twenty-four synthetic behavior partitions pass, including all-skipped pytest, missing/wrong/incomplete receipts, worktree probe shadowing, runtime/native/package-source changes, contradictory reports, cache contamination, and exit-code mapping. The schema also rejects hollow
PASSreports with failed/error tests, malformed counters, zero execution, missing identity, or wrong exit status.Revalidation of real #4538 at head
2285090on gfx950 selected the script runner and completed 56/56 cases with no blocker; the report correctly remainedINCONCLUSIVEwith process exit2because route receipts are pytest-only and no independent grid hook was available.Limitations
--target,--expected-route, and--shape-vars; automatic relevance selection is not implemented.--extra-target, because the adapter needs its own hash identity without changing the PR diff hash or live-base identity.INCONCLUSIVEwith a prebuiltPYLIBrather than accepting caller-authored provenance.perfand performance-claim reproduction are not implemented.