Repository navigation
review-pr: gates that can be checked, and 200 PRs of evidence about them - #5289
Merged
Merged
Conversation
…eceipt `main()` caught SystemExit only, so a target dying on an uncaught AssertionError -- the standard shape of aiter's op_tests -- left `status` at 0 and the `finally` block wrote a receipt claiming `pytest_exitstatus: 0` before the exception propagated. The process exit code was right; the evidence on disk was not, and a red run was indistinguishable from a green one to anything reading the receipt. Reproduced with two script targets, one passing and one asserting: before ok exit=0 receipt=0 fail exit=1 receipt=0 after ok exit=0 receipt=0 fail exit=1 receipt=1 Re-raising keeps the traceback and the exit code intact; SystemExit(3) still records 3.
`kernel_files_changed` came from `git diff --name-only HEAD`, which never lists untracked files, and the script has no `git add`/`--intent-to-add` anywhere. Files that `git apply` introduces as new therefore did not exist as far as the test-policy analyzer was concerned: a PR whose whole point is a new kernel read as touching no kernel code, and a tolerance widened alongside it was reported under the weaker finding. Reproduced on a scratch repo carrying one modified test and one added kernel: before changed=['op_tests/test_thing.py'] kernel_changed=False after changed=[..., 'csrc/kernels/new_kernel.cu'] kernel_changed=True `--exclude-standard` keeps .gitignore'd build output out; the index is untouched.
…e Tier table Step 3 was a 20-type checklist mapping onto 44 rules, applied by the model to itself. A model asked to read all 44 attends to none of them, and Step 1 already had the diff in hand and was already running Python over it, so the classification costs nothing to derive structurally. triage.py has two modes. `rules` emits only the families the diff triggers. `evidence` answers the question those families actually turn on: for a removed guard or a changed signature, how the symbol is handled ON HEAD -- fetched, rather than left to a prose reminder to go and grep. That reminder was already in Step 1 and was read and not acted on, producing a withdrawn `q_out is not None` finding on aiter#5143; `q_out` is std::optional on both sides, every call site is `has_value() ? data_ptr() : nullptr`, and the kernel guards `if(is_q && q_out != nullptr)`. The collector puts those three lines in front of the reviewer. Measured over 597 open aiter PRs (every open PR at the time, diffs fetched from the API): rules read median 12 of 44 (27%), max 29, none needed the full set coverage 597/597 matched at least one family precision most common family fires on 48%, none above half Derivation is conservative -- a family it cannot decide structurally is included, never dropped -- and it degrades rather than failing: an unparseable diff, an empty diff, or GitHub refusing to serve one over 20000 lines all fall back to the full 44-rule set, which is the pre-derivation behaviour. D9 is deliberately absent from the emitted sets. scan_index_width.py already decides that family structurally and its candidate list is the input to the finding; re-reading the prose adds nothing and costs attention the scanner-less families need. Step 4's Tier table listed `aiter/ops/*.py (any)` as Tier 1. By the table's own Q1 test that is defensible -- `__init__.py` does `from .ops.xxx import *`, so breaking any one of them breaks `import aiter` -- but there are 200+ files under aiter/ops/, and putting every single-kernel wrapper in the same tier as jit/core.py empties the tier of meaning: measured across the same 597 PRs it fires on 71% of them, and a mandatory full assessment demanded of 71% of PRs stops being performed at all. Tier 1 is now jit/core.py and __init__.py, whose failure mode is every op; a wrapper is Tier 3, whose blast radius is its own op, and the import-chain risk it does share is covered by the B6 export check attached to the ops-wrapper family. The full Step 4 assessment now fires on 6%.
`triage.py` grew a `symbols` mode that resolves every first-party import a diff adds, but nothing invoked it and the SKILL never mentioned it, so the code was dead. Step 1b now runs it against a fresh checkout of the branch the PR merges INTO -- not the PR base, and not whatever the reviewer has on disk. The root is the whole check. aiter#4994 added `from aiter.ops.flydsl.utils import is_flydsl_available`; #5116 (3b2a9ce) had deleted that module from main 19 hours earlier. Resolved against #4994's own base the import is fine and the sweep returns clean; resolved against main-at-merge-time it reports. #4994 merged green (83571d9) and was reverted 5.5 hours later (283e1d4). A stale root reproduces exactly the silence that let it through. Two corrections that fall out of that: - The finding is a REBASE signal, not an accusation of invention. Renamed HALLUCINATED-SYMBOL to UNRESOLVED-IMPORT; a reviewer who reads "hallucinated" about an import that was valid when written will dismiss the whole class. - `from pkg import sub` binds a submodule, which need not appear in the package's `__init__.py`. Searching only the __init__ text called four real imports invented across the 600-PR corpus (aiter.jit/core, aiter.jit.utils/chip_info, flydsl.kernels/{buffer_ops,vector}). Report rate over that corpus drops from 14% to 10%, and every module-level hit that remains is a module absent from today's main. Verified: reports on #4994 against main, silent against the pre-deletion tree, silent on #5143.
… checkout Three ways the same PR produced different results, or no result, depending on who ran the skill and what they happened to have on disk. 1. `$BASE_SHA` and `$HEAD_SHA` were referenced in Step 1b and assigned nowhere. Under `set -u` that aborts the block, so the cross-file evidence collector -- the thing added specifically because prose telling a reviewer to go and grep was read and not acted on -- never ran for anyone, and neither did any step after it. Both are now assigned once in Step 1 from the API: the base branch tip already fetched into base_head.txt, and headRefOid from the PR metadata. 2. `git fetch origin ...` assumed the remote named `origin` is the repo under review. Anyone who has pushed a branch has a fork there instead, and `origin refs/pull/N/head` then resolves against the FORK's pull refs: a different PR's head, or nothing. Every fetch now names $REPO_URL explicitly. 3. The evidence collector prefixed changed paths with $PROJECT_ROOT, reading the reviewer's WORKING TREE -- whatever branch is checked out, plus uncommitted edits -- rather than the PR's head. It now materialises head blobs under $WORK and names the ones it could not get. Measured on the 600-PR corpus, resolving against a checkout one day stale rather than the base tip changes the symbol sweep's answer on 33 PRs, 31 of them missed reports -- the aiter#4994 failure mode, arrived at silently. Verified end to end against live PRs (#5240/#5242/#5244 clean, #4166/#4441 report their flydsl.utils import) from two checkouts: current main, and one 130 commits stale with a fork origin, a dirty tree, and the deleted module still present. Byte-identical output from both.
Step 5 asked the model to work 44 rules -- now a derived 12 -- and marked them off
to itself. Nothing downstream could tell a run that adjudicated every rule from one
that read the list and went straight to the card, and the skill already has its own
evidence that the second is what happens under load: on a 14-PR controlled run the
revised D9 text caught 0 of 3 known overflow defects and the scanner it names was
never invoked once. That is why the D9 scan was moved into Step 1. This applies the
same move to the rule pass itself.
Step 5 now writes one line per derived rule to $WORK/verdicts.txt:
<RULE-ID> FIRE|CLEAR|N/A — <reason naming file:line, symbol, or condition>
and Step 8 will not write a card until `triage.py ledger` is green. The gate reports
UNADJUDICATED for a derived rule with no line and NO-EVIDENCE for a verdict with no
reason, and fails closed on an empty rules.txt -- the state of a run whose Step 1b
never happened, which must not be the cheapest path to a verdict.
It checks that each rule was answered, not that the answer is right. Answering
wrongly is a review error a human can see in the card; not answering is invisible,
and invisible is the one that scales.
Verified against the four shapes of skipping: no Step 1b (exit 1, LEDGER UNUSABLE),
card without a rule pass (exit 1, 4 UNADJUDICATED), two rules cherry-picked (exit 1),
every rule stamped "ok" (exit 1, 4 NO-EVIDENCE). A real adjudication passes 4/4.
Triton is 195 of the 600 open aiter PRs in the replay corpus, and the derivation
said almost nothing about them: `ops-wrapper` fired on 88% and `modified-kernel` on
79%, so a Triton PR arrived labelled rather than triaged. The generic families were
tuned on the whole corpus, and this is the subpopulation where that shows.
Four families, mined from what the 195 diffs actually contain, each firing on at
most 52% of Triton PRs and 17% of all of them:
triton-mask-bounds T1 T2 27% unmasked tl.load/tl.store; mask= without other=
triton-launch-cfg T3 T4 52% num_warps/num_stages/num_ctas; waves_per_eu,
matrix_instr_nonkdim, kpack across archs
triton-accum-prec T5 25% tl.dot, .to(tl.float32), allow_tf32
triton-grid-map T6 27% host grid vs the kernel's program_id mapping
`tl.load`/`tl.store` arguments are extracted paren-balanced rather than by regex:
`[^)]*` stops at the first close paren, so `tl.load(p, mask=(a<b), other=0.0)` reads
as having no `other=`. Twenty of the 195 contain that shape; the balanced scan drops
the mask family from 55 PRs to 52, all three of them false.
T1/T5/T6 are 🔴-eligible and carry their own FP self-checks -- an unmasked load is
fine when the axis is a constexpr multiple of the block, and T3 is a perf finding
that needs a base-vs-head number or it is `[inferred]` per Step 8.
Full-corpus replay with the six new rules in: median 13/49 (27%, unchanged --
the additions did not dilute), coverage 598/598, no family over half.
The ledger checked that every derived rule had a reason, which closes "did not answer" but not "wrote something plausible". A reason is free text and mostly unjudgeable, but one part of it points outside the model: the path it cites. `ledger` now takes the diff and rejects a FIRE whose reason names a file this PR does not touch -- a defect claimed against untouched code is either carried over from another review or invented. Only FIRE is checked, deliberately. A CLEAR legitimately cites files outside the diff: "E5 CLEAR: aiter/__init__.py unchanged" is a correct reason precisely because that file is absent, and flagging it would teach the reviewer to stop citing anything -- worse than not checking. Reasons that name no file are not flagged. This narrows the gap between "answered" and "answered honestly"; it does not close it, and nothing here judges whether a CLEAR is correct. Verified: a fabricated FIRE citing aiter/fused_moe.py on a PR that changes only aiter/mla.py and op_tests/test_mla_persistent.py exits 1 with UNTOUCHED-CITATION, while the same ledger with the FIRE moved onto aiter/mla.py:120 passes 4/4, and the two CLEARs citing untouched files pass in both.
SKILL.md went 487 -> 632 -> 1372 lines between 2026-07-09 and 2026-09-03, and almost none of that was instructions. At 1210 lines, 825 were inside code fences and 299 were prose; Step 1 alone was 828 lines, 95% of it bash. It was a ~300-line skill wrapped around a 785-line shell script, and a reader had to scroll past the script to reach the part written for them. Two moves, same principle: what is conditional gets derived, what is executable gets called, and only what every review needs stays resident. - Rule bodies move verbatim to rules.md, byte-identical to the span they came from. `triage.py expand` writes $WORK/rules_expanded.txt: the full text of exactly the derived rules, under their headings, nothing else. Cutting which rules the reviewer is TOLD to check from 44 to a median of 12 while still shipping all 44 texts was half a fix -- 383 lines of bodies against a median need of 103. Corpus replay: 0/600 expansion failures, median 117 lines against a fixed 410 (29%). - Step 1 and 1b move verbatim to fetch.sh, which SKILL.md calls in three lines and which prints its scratch dir for the steps that follow. SKILL.md: 1210 -> 440 lines, below the 487 it started at, with everything added since still in force. Two shapes the expander had to learn, both found by running it over 600 real PRs rather than by reading the file: Housekeeping rules are table ROWS, not `**HKn — **` blocks (parsing only blocks reported HK4/HK6/HK9 as undocumented on 277 PRs -- they are documented, the parser was wrong), and STEP4 names Step 4 rather than a rule body. A rule whose body genuinely cannot be found is a hard failure naming the ids, because emitting 11 of 12 bodies reads exactly like emitting 12. Verified equivalent, not assumed: fetch.sh and the previous inline blocks were run against aiter#5222 and #4166 from the same checkout. stdout matches line for line and pr.diff, rules.txt, rules_expanded.txt, symbols.txt, evidence.txt, validation_requirement.json and base_head.txt are byte-identical. The move broke four tests in validate-kernel-pr that assert on strings in review-pr's SKILL.md. The contracts are right -- perf-harness detection must agree across both skills, the scanner must be required, the identity gate must be the one shipped -- they were reading half the skill. They now read SKILL.md and fetch.sh together, and the gate block is selected by containing `expected_verdict` rather than by being the second heredoc, which is not a property anyone maintains. Mutation-testing those repairs found two holes. `assertIn("fetch.sh", step1_block)` was satisfied by the COMMENT above the call, so breaking the call site left it green; it now requires a non-comment line. And the identity gate had no test at all: every case fed it `headRefOid` copied from the report under test, so replacing `actual_head != expected_head` with `if False` -- a gate accepting a report from any checkout -- kept all 88 tests green. test_review_gate_rejects_a_report_for_another_head feeds a mismatched head and is confirmed to fail against that mutant. rules.md and fetch.sh are force-added: .gitignore line 21 ignores .claude/ wholesale, and everything already tracked under it was force-added too. Without -f these two files stayed untracked while SKILL.md was committed calling them. 89 validator tests, 241s.
Mutation testing over triage.py: 170 predicate flips, 20 caught. `and` to `or`, a
dropped `not`, `p.count("/") == 1` to `!=` -- 150 of 170 mutants left the suite green.
Those predicates decide which rules a reviewer is shown, so any edit could silently
change every review with nothing failing. The 600-PR corpus existed, but as something
run by hand, not as a test.
corpus.tgz pins 59 real PRs: 19 chosen so every one of the 31 families fires at least
once, 40 more drawn with a fixed seed. expected.json holds their exact `rules` and
`symbols` output. When a deriver change is intended, re-bless with bless.py and commit
the diff -- that diff names which real PRs would now be triaged differently, and it is
the review. Two invariants beyond byte-equality: every corpus PR derives at least one
rule (an empty result silently clears a PR), and median expansion stays under 45% of
rules.md (the derivation can stop discriminating while every individual output still
looks reasonable).
That took the score to 38%, and the survivors clustered. A third sat in
sweep_symbols/symbol_defined/resolve_module, because only 6 of the 59 corpus PRs emit
any UNRESOLVED-IMPORT line -- the paths that decide NOT to report were barely reached,
and those are the ones that fail silently. Nine cases cover them directly: namespace
package, submodule binding, star-import and __all__ re-export, module added by the PR
itself, symbol added by the PR itself, deleted-import lines, third-party and relative
imports, and the two genuine misses. Every one had been hand-verified when written and
then not encoded. Eight more cover check_citations, where seven mutants survived:
nested paths, no line number, a FIRE naming no file at all (demanding one would push
the reviewer to invent a citation), CLEAR and N/A citing untouched files, and the
two-argument call with no diff to judge against.
The structural half holds the shape the entry file was losing: SKILL.md budgeted at
500 lines, no code fence over 30 lines, every rule id the deriver can emit has a body
in rules.md, every family expands to something, and fetch.sh's `required ... missing`
guards must each be followed by a non-zero exit -- mutation turned all six of those
into `exit 0`, so a missing scanner or schema would have let the run continue and
report nothing, and nothing failed. That last one is a lint over the script's text,
not an execution test; running fetch.sh needs a GitHub round trip.
Both structural limits were confirmed to bite by pushing the file past them. Without
the budget this comes back: nothing stopped the file growing the first time, so it
grew. Raising the number is now a commit that has to say why.
35 tests, 5.4s, stdlib only, no GPU and no network.
Reviewing a random ten of the 619 open aiter PRs, one at a time, the way the skill is actually used. Two PRs carry defects that would land: #5220 inserts two ints into the middle of a kargs struct whose layout is pinned by `static_assert(sizeof == 112)` plus one `offsetof` per field, in the same translation unit it edits, so all three assertions fail; #4146 imports `aiter.ops.triton.utils.core`, which #5061 renamed to `utils.config_utils`, so it is a ModuleNotFoundError on merge. Seven open PRs still import that module and 41 still import the deleted `aiter.ops.flydsl.utils`. Five gaps in the skill, each found by using it rather than by reading it: - **No rule covered struct layout at all.** D11 fires when a struct in C-like source gains or loses a field and the diff touches no `offsetof`/`static_assert(sizeof` line -- derivable purely from the diff, 3% of the corpus, 40 such assertions across 37 files in csrc/. It does not fire when the PR updates the table in the same diff, which is the correct shape. The struct declaration usually lives in the hunk header's scope context rather than the hunk body, which is why detection reads `@@ ... @@ struct X`. - **The sweep skipped relative imports entirely.** `from .chip_info import ...` inside aiter/jit/utils/asm_guard.py resolves to aiter.jit.utils.chip_info. Skipping them left the case where an unresolvable import is worst: a Tier-1 PR wiring a new module into aiter/__init__.py, where it breaks `import aiter` outright. #5107's chain is sound, but only because it was checked by hand. Corpus report rate 9% -> 13%, all of it the same deleted modules reached through a second syntax. The `mod.startswith(".")` skip is now dead and removed. - **The sweep read non-Python files.** docs/tutorials/add_new_op.rst walks a reader through writing an op, imports and all, and two PRs were reported for a module named in a tutorial. - **HK9 fired on benchmark scripts.** PROF_WARMUP/PROF_ITERS in op_tests/flydsl_tests/profile_flydsl_bwd.py are not permanent runtime knobs; HK9 exists because aiter reverted one that was. The family now reads runtime files only. - **The evidence collector ignored the most common shape of the finding it serves.** The invariant-removed family fires on a deleted `.contiguous()`, but the collector only extracted symbols from assert/*_CHECK lines, so a removed contiguity normalisation produced no evidence and the reviewer was back to grepping -- which is what the collector exists to stop. #5235 relocates two of them; the answer is "it moved", and now the collector shows it. Also generated: Step 3's type -> rule table moves to MAPPING.md, emitted by `triage.py mapping`. The hand-written copy had drifted -- it claimed D9 was derived when D9 is scanner-backed and deliberately is not, and omitted 21 rules that are, including every Triton rule. A mapping the reviewer is told not to apply by hand should not be maintained by hand. Step 6's first structural check asked the model to list every new symbol and grep it; `triage.py symbols` had already done that, so it now points at $WORK/symbols.txt and scopes the manual work to what the static sweep cannot judge. The other five checks write one line each into $WORK/ai_diagnostic.txt, and Step 8 runs answers, diagnostic and ledger before a card may be written. 600 PRs: median 15/50 rules (30%), 600/600 covered, no family over half, symbols 13%. 99 structural tests, 89 validator tests. Mutation score on triage.py 93% of 137 code mutants; the nine survivors are equivalent mutants, listed and not papered over with tests that cannot kill them.
Asked why a ten-PR pass reported no redundant code, no AI slop and no weak op_tests, the answer was that none of those has a detector. Step 6 asks for all three in prose -- "identify mirrored code ... compare it field by field", "is the reference impl structurally a twin", "is atol/rtol loosened with no justification" -- and prose asking for a search is the shape that does not happen, which is why D9's scan moved into Step 1 and why the rule pass grew a ledger. Two collectors, both evidence rather than verdicts, because measuring first showed a verdict would be wrong more often than right: - `twins` names the file each new file was copied from, with the overlap ratio. 3% of the 600-PR corpus adds a file at least 60% identical to one already in the tree: fused_gemm_a16w16_copy_x.py against _quant_x.py at 65%, test_opus_gmem_gfx1100.cu against _gfx1201.cu at 75%. The pair is not the finding -- an arch-specific variant is the normal shape -- the asymmetry between them is, and that needs a human diff. - `testquality` prints, per added test file, the number of assertion primitives, the tolerances and the shapes. It does NOT conclude "this test asserts nothing", because that is not decidable from a diff: assertions routinely live in a shared helper (`run_fp8(..., verify=True)`), and the first two corpus cases inspected under a firing rule were both false. The strict form -- a whole new test file with no assertion primitive anywhere -- fires on 2 of 600, one of which is a benchmark. A zero count is printed with the reason it is not yet a finding. Measured and rejected as rules, recorded here so they are not re-proposed: intra-diff six-line duplication (30% of PRs, no discrimination), test bodies with no assert (over half false), and a reference implementation calling the op under test (1 of 600). 112 structural tests, 89 validator tests.
Batch two. aiter#4821 ships op_tests/flydsl_tests/test_dispatch_tdm.py and compile_dispatch_tdm.py: both named like tests, both written as `main()` scripts with `_check()` helpers, neither containing a single `def test_`. pytest imports them and collects zero tests. HK6 asks a new op to ship `op_tests/test_*.py` and this satisfies it by name while contributing nothing to CI. The rule is deliberately narrow. aiter's tree already holds such files, so the style is tolerated and the file is not itself a defect; HK12 fires only when the PR ships no collectable test at all -- the case where HK6 reads as satisfied and the new code is in fact untested. 3.8% of the 600-PR corpus, against 4.3% that add such a file at all. The evidence collectors added for batch one earned their place immediately on batch two, in both directions: - test_quality flagged `atol, rtol = 0.125, 0.125` in aiter#3940, and the four lines above it in the diff derive that number from e4m3's three mantissa bits. Not a finding, and it took one look rather than a grep -- which is what putting the number on screen is for. - test_quality reported `test functions added : 0` for #4821's two files, which is what led to HK12. Median rules 15/51 (29%), HK12 3.8%, 117 structural tests.
…ks like one Challenged on the HK12 finding from batch two -- "op test CI should pick all of those up, did you judge it wrong" -- and the answer is yes, the mechanism was wrong and the real one is worse. .github/scripts/split_tests.sh shard-scans `op_tests` with `-maxdepth 1` and `op_tests/triton_tests` recursively. Nothing else is scanned by anything: op_tests/*.py 127 files aiter shard, runs by default op_tests/triton_tests/** 107 triton shard, runs by default op_tests/opus/ 1 opus-test.yaml op_tests/multigpu_tests/ 29 only with the `multigpu` label, skipped by default op_tests/flydsl_tests/ 5 no workflow scans it at all So aiter#4821's two files would not run even if every function in them were a `def test_`. The absence of `def test_` was a real observation about the wrong thing. `triage.py citest` answers the question that matters -- will a CI job run this file -- and answers it from .github/ rather than from a table here, because a copy of the shard script would drift the way Step 3's rule table did. 9% of the 600-PR corpus adds a test that never runs: 48 into unscanned directories, 6 label-gated. One trap on the way: a path MENTIONED in a workflow is not a path that gets RUN. pr-title-tags.yaml names op_tests/flydsl_tests/ to choose a label, and counting that as coverage reported #4821 as covered -- the exact case the collector exists to catch. Only lines that execute something count now, and a test pins that. HK12 is now about location and is evidence-backed, listed in UNREACHABLE_BY_DESIGN alongside D9 since the diff alone cannot answer it. The original observation survives as HK12b at 📝, explicitly secondary: check where the file lives before asking what is in it, because a file in an unscanned directory does not run whatever it contains. The unreachable-rule invariant caught this during the edit -- HK12 documented with no family emitting it -- which is what it was added for. Lettered HK ids (HK12b) needed the id pattern widened; the expander was silently dropping them. 124 structural tests, 89 validator tests.
P1 asks for a number with its units and its comparison, and P6 asks for base-vs-head. Neither had anything reading the description, so a claim with no other side to it reached the reviewer as prose. `triage.py perfclaims` reads the PR body, lists every numeric claim, and marks the ones that say nothing about what they are measured against. Four things it deliberately does not flag, each found by running it rather than by reasoning about it: - Markdown table rows whose header says `before | after | speedup`. aiter#4443 ships six such rows and judging each row alone called all six unbaselined. - Signed deltas. `+8.64% end-to-end` compares against not having the change, which is a stated baseline in ordinary English; interrogating it is the pedantry that makes a check ignorable. - Shares. `33.07% of GPU time`, `81.8% prefill by GPU time` say where the time goes. - GPU counts. `8x MI355X` is eight cards. Substituting the model name away first left a bare `8x` reading as a speedup. Scoped to the description on purpose: reading claims out of kernel comments over the 600-PR corpus was 61% noise -- `4x DS_READ`, `<4 x i32>`, `num_tokens x 384 x 7168`, `5% of elements`. The claim lives in the description, which is where P1 and P3 ask for it. Measured and rejected in the same pass, recorded so they are not re-proposed: - Comment density. Median 7% of added code lines across 409 PRs, P90 21%. The five PRs over 35% were all read: aiter#4061 repeats one explanatory line across 140 generated CK files, #3557 is a section banner for the RDNA4 WMMA path, #2814 explains non-temporal loads. High density tracks codegen and documented kernels, not slop. - "Comment restates the next line", 12% of PRs. Inspected: it is mostly matching two adjacent comment lines, and the one real comment-then-code case was a useful comment above an assert. 132 structural tests, 89 validator tests.
Batch five caught the rule added in batch one being noisy. aiter#5223 changes six headers and D11 fired, because the family was "a struct field moved and no assertion line changed" -- which is true of any struct field churn anywhere. None of those headers has an assertion within reach. A layout change is a defect only when something asserts the layout, and that is a fact about the tree. `triage.py structabi` extracts the struct names whose fields moved, then looks for `offsetof(<name>` or `static_assert(sizeof(<name>))` in the repository, and reports only the ones it finds. Corpus report rate 3% -> 0.3%: two PRs, both touching `pa_sparse_prefill_kargs`, which is the struct that actually carries the table. The true positive from batch one, aiter#5220, survives the tightening. D11 joins D9 and HK12 in UNREACHABLE_BY_DESIGN, all three for the same reason: the diff alone cannot answer the question, so a Step 1b collector answers it and the rule reads the artifact. Also from batch three: BASELINE was English-only, and aiter's descriptions are partly Chinese. aiter#5043's table is headed `场景 | 输入数据 | 时间 | TFLOPS` and correctly flagged -- it reports absolute times for a new ASM kernel whose own description says the shape previously fell back to CK/Triton, with no measurement of what it replaces -- but a table headed `优化前 | 优化后 | 提速` would have been flagged too. Both directions pinned. 136 structural tests, 89 validator tests.
Batch nine. aiter#4062, "docs(python): condense verbose comments", is 252 files and 7963 changed lines. A reviewer either skims it or spends hours on it, and both of those miss whatever code is hiding in it. The answer turns out to be: nothing. It is worth being able to say that in one line. `triage.py commentonly` fires only when a diff is at least 90% comment churn and at least 60 lines -- 2 of the 600-PR corpus -- and prints the changed lines that are not prose. Three passes were needed before it told the truth about #4062: - Trailing comments are stripped before comparing. `REVISION = 25 # rev24, drop the token-major path` becoming `REVISION = 25 # g layout fixed head-major` changes no code, and #4062 read as four code lines until that was handled. - `/* ... */` continuation lines with no leading `*` are prose. aiter#4061's rewritten quant_utils.cuh header is exactly that, and every prose line in it was listed as something to review. - Those lines are marked as prose rather than dropped, because the floor and the ratio are about the whole diff; dropping them made a 6000-line comment rewrite look like a two-line diff that fell under the floor. Both PRs now reduce to zero code lines, which is the truthful answer. Same batch, a smaller correction: `invariant-removed` matched `assert` / `.contiguous()` in deleted COMMENT lines, so #4062 fired D4 on prose about asserts. Measured across the corpus this is 3 of 71 fires, so it was noise rather than a hole -- fixed because it is free, not because it was urgent. 144 structural tests, 89 validator tests.
answers, diagnostic and ledger all check that the work happened. None of them looks at the card, which is the one artifact a reader actually gets -- so a review could adjudicate 27 rules honestly and then write a finding that came from none of them. And the red threshold, "before firing any red, write down the concrete input that triggers it", was prose with nothing reading it. `triage.py card` checks three things per finding and only three: - UNTOUCHED-FINDING: it cites a file this PR does not change. - UNBACKED-FINDING: it carries a rule id no verdict marked FIRE, or names files that appear in no verdict, diagnostic or blind-spot line. - UNPROVEN-RED: a red naming neither a value nor two code identifiers. Whether a finding is correct is not checked and cannot be; that is what a human reads the card for. The concreteness test needed a second pass. Requiring a digit rejected a real finding -- "row=(tail_blk*num_kv_heads+kv_head_idx)*BLOCK_M against nrows=tail*num_kv_heads*BLOCK_M" is as concrete as a finding gets and contains none. Two distinct code identifiers is the line between that and "the reduce kernel looks racy". Adding the gate pushed SKILL.md to 512 lines and the budget test failed, which is what it is for. The budget was not raised: Step 1b had grown a paragraph per artifact, nine of them, each restating what the artifact prints about itself. They condense to a table -- file, what it answers, the trap it exists for -- and the entry file lands at 477, twenty under where it started this session and thirty under the budget. 152 structural tests, 89 validator tests.
Batch eleven. aiter#4515 was reported twice for the same module under two names, one of which does not exist anywhere: `aiter.ops.kernels.pa_mqa_logits_fp4`. The resolver stripped `.__init__` from `aiter/ops/flydsl/__init__.py` to get the package name and then dropped a component as well, which is right for `aiter/ops/flydsl/x.py` and wrong for the package's own `__init__.py` -- the containing package IS aiter.ops.flydsl there. Corpus report rate 13% -> 10%, 144 lines -> 104: fifteen PRs were being reported for imports that resolve fine one level up. The remaining finding on #4515 survives and is real: `pa_mqa_logits_fp4.py` was deleted by #4609's raw-dialect cleanup and the kernels live under `mqa_logits/` now, so the PR modifies a file that no longer exists on main -- a modify/delete conflict on merge, and imports pointing at nothing. That is the fifteenth distinct module broken by a reorganisation in the open queue. Same batch, worth recording rather than fixing: aiter#4848 fixes MoE expert addressing past 4 GB -- a 32-bit overflow class -- and ships op_tests/flydsl_tests/test_flydsl_moe_4gib_addressing.py, which lands in the directory no CI job scans. The test for a subtle addressing bug will never run. ci_coverage caught it, which is what it was added for. 154 structural tests, 89 validator tests.
…takes
Batch twelve. aiter#4255 deletes `assert gfx_version in ("gfx942", "gfx950", "gfx1250")`
and the invariant-removed family fired, correctly. The answer is one line away and was
not on screen: the assert is REPLACED, by `assert _is_gluon_pa_mqa_logits_supported(
gfx_version)`, which is a stronger guard than the allowlist it replaces. The evidence
collector said nothing, so clearing it meant grepping by hand -- which is the thing the
collector exists to stop.
It extracted symbols from exactly two shapes, `X is not None` and `X->`. Across the
corpus, 57 PRs delete a guard and 51 of them -- 89% -- produced no evidence at all:
assert causal, "Only causal attention is supported" -> causal
assert WQ.dtype == dtypes.fp8, "only fp8" -> WQ
assert num_head_qo % 16 == 0 -> num_head_qo
assert gfx_version in ("gfx942", "gfx950") -> gfx_version
TORCH_CHECK(out.is_contiguous(), "out must be ...") -> out
The subject of a guard is the first identifier after the keyword, skipping the words that
are never a subject (`not`, `is`, `None`, `len`, `isinstance`, dtype namespaces). 89% ->
4%, averaging 3.6 symbols per PR.
This is the third time the same shape of gap has appeared: a family fires on a pattern
wider than the collector serving it understands. First `.contiguous()` removals, then
comment lines, now every assert that is not `X is not None`. The family and its collector
are written in different places and drift apart; both are now covered by cases that name
the shapes explicitly.
Rest of batch twelve was quiet: no unresolved imports, no twins, no unrun tests, no
pinned-layout changes, no unbaselined claims.
156 structural tests, 89 validator tests.
Asked whether the "is this nailed down" step was actually nailed into the skill, the answer was no. Probing my own gate found three ways past it: - A missing $WORK/card.md read as an empty card, which read as no findings, which passed. Not writing the card was the cheapest way past the check. Now a hard failure. - A card with no findings passed while the ledger held nine FIRE verdicts. I had checked that findings trace back to work and not that work reaches the card, which is the inverse and the one that matters more: a review can adjudicate nine rules FIRE, report none of them, and look clean. - A card carrying only a heading passed the same way. FIRE now means it goes in the card. The 5-finding cap is a real reason to leave one out, so the escape is `-- not reported: <reason>` appended to the verdict line -- written down rather than silent. Verified against the real aiter#5157 ledger: nine FIRE, five on the card, two withdrawn in writing, and the gate holds out for the remaining two until they are accounted for. One test had to change rather than the product: a note citing aiter/mla.py with nothing in any verdict, diagnostic or blind-spot line mentioning it is a finding from nowhere, and Step 7.5 is where free-form observations get written down. Both directions are now covered -- a note grounded in the blind-spot answer passes, one from nowhere does not. 161 structural tests, 89 validator tests.
Batch thirteen. One evidence line across ten PRs, and it was noise: aiter#4016 ships op_tests/triton_tests/bench_gdn_chunk_prepare.py beside a real test, and test_quality told the reviewer its zero assertions "may be in a helper, or there may be none". A benchmark measures; it asserts nothing by design. 68 of 682 rows over the corpus are that shape. bench_*/profile_* files and anything under op_benchmarks/ are now labelled, and their zero count reads "expected for a benchmark". The same count on a real test still asks about the helper, because there it is a real question. Batch thirteen otherwise clean: no unresolved imports, no twins, no unrun tests, no pinned-layout changes, no unbaselined claims, no comment-dominated diffs. While adding the tests, a behaviour worth pinning turned up: a test file with no tests, no assertions, no tolerances and no shapes is not listed at all. There is nothing to put in front of the reviewer, and that is right, but nothing said so. 165 structural tests, 89 validator tests.
aiter#5137 was reverted on one review comment: "Please change the config file jsons instead of hardcoding it in the code waves_per_eu=2". T4 already covers a knob forced across archs; this is the single-arch case and a different failure. The config system exists so a tuning sweep and the code disagree loudly, and a literal in kernel source is invisible to the sweep -- the next sweep either overwrites it or fights it, and neither shows up in a diff. #5137 carried a six-line justification for the value; the justification was not the problem, the location was. Fires on waves_per_eu / matrix_instr_nonkdim / kpack / num_stages / num_warps / num_ctas set to a literal in a .py outside configs/ and op_tests/. 11% of the 600-PR corpus. Exempt: a knob read from a config (`num_warps=config["num_warps"]`, `**cfg`), a literal in a config file, a test pinning a knob deliberately, and a commented-out line. Recorded with it: the pipeline is lenient. Measured across all 600 PRs, 77% produce no evidence line at all from symbols, twins, citest, structabi, commentonly or the test-quality flags. Some of that is correct -- most PRs are config bumps and tuning rows -- but 77% is not a number to be comfortable with, and the next passes should be spent raising recall rather than trimming noise. 171 structural tests, 89 validator tests.
… untested Two of eleven candidates measured against the 600-PR corpus survived being read. api-signature required a `def`/`void`/`template` line on BOTH sides of the diff -- a signature rewritten in place. Deleting one line of a multi-line signature never touches the `def` line, so B6/E1/E5 were never asked about it. Reported as 9.3% of the corpus; 9.3% is what the shape scores before it is read. A hunk header names the ENCLOSING def, so `transpose_out=True,` passed to a call inside `def asm_moe(` counted as a dropped parameter of asm_moe, and the same line pattern over C++ reads `std::optional<Tensor>& fp8_out,` as a parameter named `std`. Walking the old side back to an unbalanced `(`, and staying out of C++, leaves 3.5% (21/600) at roughly 8 in 10 on a sample of ten. It moves the family from 17.5% to 20.3% of the corpus, and four PRs in the pinned 59 now derive it -- 1831, 2577, 2592, 4997, each a real kernel parameter. That is the whole blessed diff; symbols and MAPPING are untouched. HK6 already fired on new-kernel but was prose, so the reader established the absence of a test by hand. kernel_tests.txt does it: 6.5% (39/600) add a kernel with no test pytest would collect. Collectibility is decided by filename, which is the rule pytest uses -- an op_tests/-only whitelist called aiter#3991 untested while it added five test files under aiter/aot/flydsl/tests/, and excluding any path containing "bench" called aiter#2889 untested on the strength of test_rmsnorm_bench_against_aiter.py, which holds two collectible tests. An evidence line that states a falsehood is worse than one that says nothing, so the corpus is checked for that specific lie and reports none. Rejected, with the numbers, so it is not proposed again: added `except` blocks whose body is only pass/continue/return. 10.7% of the corpus, and reported at 4/4 true. 38 of the 64 PRs match nothing but a capability probe -- `except ImportError: return False` is correct code -- and six sampled from the narrowed pass/continue set were all deliberate: a barrier timeout whose comment explains that hanging is the alternative, `int(os.environ["MAX_JOBS"])` falling through to a default, a ROCm-discovery fallback chain, a best-effort rmtree, a tuning loop skipping unsupported configs. That is the shape of the weak no-assertion check this project already rejected: high rate, plausible story, false on reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A1 asks whether a sibling kernel has the same bug and has never had a forensic answer, so the candidate for one was measured: deleted lines still present verbatim in a file this PR does not touch, three or more times. 13.2% of 600 open PRs, 10.0% once the line is required to carry an operation instead of being a declaration, a bare `for`, or a docstring. Eight read: about half are what A1 means -- variant MoE ops sharing `A_scale.stride(0) if A_scale is not None and A_scale.ndim == 2 else 0,` across moe_op_silu_fused.py and moe_op_gelu.py, where fixing one and not the other is the defect. The rest are boilerplate two files share for no reason: `for(int i = 0; i < 4; i++)`, an OptionalHIPGuardMasqueradingAsCUDA line, `HEAD_DIMENSION_OPTIONS = [128]`. Narrowing does not fix it. It silenced two of the four known-noise hits, left the stream guard and the config constant firing, and cost one of the three real ones. That is the shape of the problem rather than a tuning depth: a sibling that needs the same fix and a sibling that shares boilerplate emit the same evidence, because it is the same line. Verbatim identity is the wrong signal. A1's own example -- PR#3841, a decode kernel fixed while `_prefill_opt` beside it was not -- is same-FILE. Sibling FUNCTIONS within the changed file is the direction that has not been tried, and it needs its own numbers before it goes in. Recorded in the rule body so the next pass starts there instead of re-measuring this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A1 asks whether the sibling kernel carries the same bug and has never had a forensic answer. Its own example is aiter#3841 -- a decode kernel fixed while `_prefill_opt` beside it was not -- which is two functions in ONE file, and twins compares whole files. So the last attempt looked across files for a deleted line still present verbatim somewhere untouched: 13.2%, half of eight read were boilerplate two files share for no reason, and narrowing cost a real hit for every two noise hits removed. That is recorded in the rule body; this is the version scoped the way A1 means. Same file, and the two function names must share an eight-character stem. `_moe_gemm_a8w4_decode` against `_moe_gemm_a8w4_prefill`; `_stage1_ragged_k` against `_ragged_k`, still carrying `mask = context_idx + tl.arange(0, ChunkK) <= context_length`, which is the bounds check A1 names; `mla_decode_fwd` against `mla_v40_decode_fwd` over `and q.dtype == dtypes.fp8`. 18.0% of 600 open PRs. Without the stem, any two functions sharing a file qualify and it is 23.3%, mostly helpers that merely sit together. The stem is deliberately not a list of variant suffixes. Adding one -- _opt, _v2, _prefill, _decode, _fwd -- gives a tidier 10.8%, and 43 of the 43 it drops include kernel_unified_attention_2d against _3d three times over and select_2d_config against select_3d_config. Those are the shape the rule exists for. D9's body already records this lesson from the other end: it was a name list, it missed three real defects, and it is now an AST pass with no names in it. Do not add the suffix list back. Python only. Every hit was Python, and splitting C bodies by brace depth is a heuristic that earned nothing -- it produced a span bug that matched a function's own `def` line inside another function. Evidence, not a finding: a variant legitimately diverges. What is handed over is the pair, so that the judgement gets made instead of skipped. Checked on ten PRs outside the corpus (batch 23): it fires on aiter#2478, "Fix GPU memory access fault in CK MoE FP4 kernel", pointing at `_flydsl_moe_sorting` still holding the allocation-size line that `_moe_sorting_impl` just had fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A sweep reported that D11's example was fiction: aiter#5220 adds two ints to `pa_sparse_prefill_kargs`, the rule body says three assertions in the same translation unit fail, and grepping the tree for `offsetof(` near that struct finds nothing. The header does hold twelve static_asserts and every one of them is tile or vector arithmetic, which made the conclusion look doubly confirmed. I rewrote the docstring and the rule body around it -- #5220 recast as the case D11 correctly stays silent on. Both greps read /mnt/raid0/zufa/aiter, a second checkout sitting on af81aa0 from 2026-07-13. On the branch under review, `pa_sparse_prefill_opus_kernels.cu` asserts `sizeof(pa_sparse_prefill_kargs) == 112` and fixes every field offset through the PA_GFX1250_CO_ABI macro -- which is the second disguise, because a literal `offsetof(` does not appear either. Given the right root, `triage.py structabi` on #5220 prints PINNED-LAYOUT. The rule was right and the measurement was stale, so the rewrite is reverted and only the test survives. The test is the point: the example is now executed, not asserted in prose. One case pins that the struct really is pinned in this tree, one runs #5220's shape through the detector and requires PINNED-LAYOUT, one requires silence when the same diff updates the assertion. If the tree moves, the failure says so and names which passage to rewrite. The tree is found from the skill's own location, so there is no second checkout to pick wrong. Also corrects sibling_variants: 18.0% was measured against that same stale tree, and the merge target gives 18.3%. The conclusion does not move. Nothing else needs re-measuring -- dropped_parameter and untested_new_kernel read the diff and never the tree, and fetch.sh takes PROJECT_ROOT from `git rev-parse --show-toplevel`, so every artifact under /tmp/rev was always computed against the right root. Only the hand-run measurements were wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KERNEL_PY lists `aiter/ops/triton/`, `_triton_kernels/`, `_gluon_kernels/` and `/gluon/`. A FlyDSL kernel matches none of them, so it reached `flydsl` -- D10 and D10b, which are about what happens to a compile result -- and nothing else. 108 of 600 open PRs edit `aiter/ops/flydsl/kernels/*.py`; 95 of them derive neither kernel family. A1's sibling variant, D1's uninitialised accumulator, D8's missing contiguous check and P6's unmeasured cost were never put to an entire backend. The siblings forensic had been pairing `_flydsl_stage1_wrapper` against `_flydsl_stage2_wrapper` in exactly these files for three commits. The evidence was being produced and no rule was asking for it. A separate family rather than another entry in KERNEL_PY, because FlyDSL is not Triton and `modified-kernel` would say it is. B2 is deliberately not in the set: it is `tl.load` or `tl.store` without a mask, and there is no tl here. Median rules per PR stays 15 and the mean moves 14.7 to 15.1 -- these PRs already carried most of what this adds. Eight of the pinned 59 gain the family and none loses anything. Found while auditing something else, which is worth recording: `perf` fires on a title keyword, and `fuse` accounts for 43 of its 90 firings on its own. Every one of twelve read was a kernel NAME -- "Expose the fused_moe activation dtype resolution", "Add `repr` to DiT fused kernels" -- not a claim about speed. Dropping `fuse` and `%` takes the family from 15% to 7.5%, and the reason that is not in this commit is that it interacts with this one: some of what it drops was reaching P6 only through `perf`, and now reaches it through `flydsl-kernel` instead. It needs re-measuring against this baseline, not the old one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of these suites at once make a third of the run look broken. The validator claims a GPU with `flock -n` on /tmp/gpu-N.lock; a second one finds every candidate locked, degrades to NO_GPU, skips the runtime stages and returns INCONCLUSIVE. That is correct, and the report says so -- "GPU claim raced with another process". What the reader sees is `'PASS' != 'INCONCLUSIVE'` on five or six tests, which names the symptom and hides the cause one file away. Run alone: 89 passed, six separate times. Two runs overlapping: 5 failures and 6. Both of the overlaps were mine -- a run started with a bare `&`, then a second launched before it finished, twice. I spent several sessions reporting this suite as flaky and proposing to isolate the timing-sensitive tests, which would have deleted a true signal from a suite that was working. The reason never reached me because I had been piping the output through `tail -3`, so the one line that explained it was cut off every time. The guard asks the lock rather than the process list: it tries to flock each /tmp/gpu-*.lock and reports the ones it cannot take. Written first as `pgrep -af validate_pr.sh`, which matched the shell running the pgrep -- the string is in its own argv. Verified by holding a lock and watching it fail with the right message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything until now was measured against the 600-PR corpus, which exercises the deriver and the forensics and never writes a card. Four full Step 1-8 reviews -- aiter#4295 and #2478 for signal, #3836 and #5072 as controls -- found six defects in one pass. Both controls returned NO FINDINGS, so the gates are not manufacturing work; what they were doing was corrupting the reviews that had something to say. The citation gates punished the forensics the rules ask for. card and ledger rejected any finding naming a file the diff does not change, and three of the four runs hit it independently. aiter#2478's two strongest findings rest on the mask contract in csrc/include/moe_sorting_opus.h -- an unchanged header, and the only place the convention and the local-id derivation are written down -- so the review deleted the filename and described the header in prose to get through. E4's own rule body sends the reader to .github/workflows/*.yaml to check a label, and doing so tripped the same gate. Both now require an anchor: at least one cited file the PR changes, with everything else free. A finding citing nothing from the diff is still rejected, which is what these were for. The card gate demanded rule codes that SKILL.md forbids. It matched a finding to its FIRE by a leading `D8:`, and SKILL.md line 466 says "Do NOT use rule codes (P1, D4, A1...) in output -- they are internal labels only". Every card written to spec reported every fired rule as UNREPORTED-FIRE. A finding naming a file the verdict cited is the same claim without the label. `torch.cuda.device(...)` parsed as a file called torch.cu, and `x.contiguous.cuda()` as x.contiguous.cu. Those are the two most ordinary expressions in a HIP review. SKILL.md told the reviewer that a runtime change with no test target is a finding in its own right. Triage calls anything under aiter/ a runtime surface, so both control PRs -- a tuner input CSV and a tuned-config table, neither loaded at run time -- arrived carrying a defect the reviewer was instructed to report. That is a fabrication inducer on the family of PRs where the deriver has least to say. Two artifacts could not say they had run. An empty symbols.txt means "this axis was not checked" per SKILL.md, and a clean sweep wrote zero bytes; ci_coverage said "every new test file lands where a CI job will run it" on a PR that adds no test file, which reads as a pass. And the artifacts line promised evidence.txt unconditionally though it is written only for guard or signature diffs, while never learning about guards.txt, siblings.txt or kernel_tests.txt -- three files I added, three commits apart, without touching that line. Also drops two .pyc files that had been force-added into validate-kernel-pr. They are build output and would land in any upstream PR. Not fixed, recorded for the next pass: twins compares within a language, so aiter#4295 -- a .cu to .py port, which is twin divergence by definition -- got "no new file closely mirrors an existing one" while the review found three divergences by hand; perf_claims parses line by line and called three claims baseline-less when the baseline was the line above; $WORK/merge-target is named in a warning but only changed files are materialised; there is no verdict token for "no defects, but notes". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-ran aiter#2478 and #5072 against the fixed gates. Both passed all five gates on the first attempt with no rework, which is the result being checked for: #2478 had previously been pushed back twice and had to delete `csrc/include/moe_sorting_opus.h` from its two strongest findings and describe the header in prose. This time it cites :337, :984 and :902 directly, plus .github/workflows/kimi-downstream.yaml and the guard #4839 landed -- all files the PR does not touch, all now allowed because each finding is anchored in one it does. #5072 wrote its validation line as NOT RUN and did not report the missing test target as a defect, having traced how the CSV is actually consumed. Three things those runs found. Relaxing the FIRE claim made it freeloadable. #2478 fired three rules and every verdict cited aiter/fused_moe.py, so one sentence naming that path accounted for all three. Where N fired rules share a citation, N distinct findings now have to name it. fetch.sh still carried the instruction the last commit removed from SKILL.md: "if the reason is a missing test target, report it as a finding about the PR", unconditional. The document was fixed and the string the program prints was not, which is the same path the original defect took. #5072 followed the document and overrode the tool, and said so; the next reviewer may do the reverse. Both texts are now pinned by a test. Nothing asked whether the diff still applies. #2478 is five months old and both hunks conflict with the merge target -- the reviewer established that by thinking to run `git apply --check`, while symbols.txt reporting every import resolved reads as evidence of no drift. It is one command; applies.txt now carries the answer either way. fetch.sh reached 901 lines with that addition, one over budget, and the budget test caught it. The number was not raised: two comment lines and a message were compressed instead, which is what the budget is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five gates check that every step happened and that the card reports it. None of them can tell a true finding from a confident false one, and the calibration runs produced one: aiter#5072 reported 125 of 174 tuned-config rows as unreachable, marked `[verified]`, reasoning about how `getPaddedM` pads M before the lookup. `get_CKGEMM_config` loops `for gl in [None, 0, 1]` -- exact M is tried first, so those rows are reachable and the claim was false. It passed all five gates. Ten seconds in the lookup function would have ended it, and nothing asked for those ten seconds. Step 7.6 asks for them. One line per finding in $WORK/refutations.txt -- `RED|WARN|NOTE SURVIVED|KILLED -- what you opened and what it said` -- and a sixth gate that rejects a missing artifact, "I checked it", reasoning that names no file or symbol or command, and a card whose findings have no surviving line. It cannot judge whether a refutation is correct; what it can require is that one was attempted and that a second person could repeat it. rules.md § Refutation lists what to attack, ordered by what has actually killed something here: the premise first, then which tree the line numbers came from, then whether the trigger is reachable, then suspicion versus defect, then severity. It also says this is the weaker of the two tiers, because it is. Refuting your own finding catches a premise you never tested; it does not catch a conclusion you are attached to. The stronger tier is an independent reviewer handed the card and told the findings are false until proven otherwise -- which is how #5072's error was actually found. A test pins the words "weaker" and "independent" into that section so the limitation cannot quietly be edited out. SKILL.md hit 501 lines. The budget was not raised: the Tier-3 rationale moved to rules.md § Tiering, where a paragraph read once belongs, one redundant blank line went, and "Step 1b writes nine artifacts" became true again -- it writes more than nine, and has since guards.txt, siblings.txt and kernel_tests.txt landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifty full reviews with an independent adversary behind each. 47 of 50 finished all six
gates; 234 findings reached cards, and the adversary killed 47 of them -- one in five
survived every gate and did not survive a reader told to assume it was wrong. That is the
number for whether this is ready to be handed to anyone: it is not, yet.
Step 7.6 earned its place on the first run. It killed 264 candidates before they reached a
card, more than the 234 that got there.
The gates spent the wave rejecting correct citations. 64 pushbacks, and the reviews said
plainly what they did to get past them: "an edit made for gate mechanics, not for review
content"; "No claim changed -- only the citation surface"; "I passed it by inserting a
cross-reference to a *different* file, which is weaker anchoring than what it rejected".
Five mechanical bugs, all in gates I wrote or changed:
`cu` preceded `cuh` in the citation alternation with no boundary after it, so
`gemm_a8w8_blockscale_cktile_common.cuh` matched as `...common.cu` and a file the PR
changes was read as one it does not.
The path class had no `=`, so a tuned-config row cited as
`gfx1201-GEMM-A8W8_BLOCKSCALE-N=1024-K=1024.json` matched as `1024.json`.
`lstrip("./")` strips every leading dot and slash, not a leading `./`. A PR that ADDS
`.github/workflows/perf-parity.yaml` had that finding rejected for citing nothing it
changes.
ANCHOR_IDENT knew snake_case, ALLCAPS and camelCase. aiter is full of `gfx1250`, `a8w8`,
`bf16`, `mxfp4` -- no underscore, no capital. A one-line dict edit adding `18: "gfx1250",`
names its subject with a token all three shapes rejected, so the reason that named
exactly the right thing was the one that failed to anchor.
`[infra-only] NONE` is what derive() correctly returns for a diff confined to .github/.
`NONE` is not a rule id, so the ledger read the right answer as proof the derivation
never ran, and three .github-only PRs were unpassable. All three reviews diagnosed it
identically and left the gate red rather than writing rule ids the deriver never emitted,
which is why it was found rather than hidden.
The Step 1b artifact table listed nine files; fetch.sh writes thirteen, and it describes
evidence.txt unconditionally though it is written only for guard or signature diffs.
Reviews went looking for a file that was not there and recorded the absence as a skipped
axis. Three forensics were added without that table being touched, all three times by me,
so a test now derives the expected list from fetch.sh itself.
SKILL.md went to 503 and the budget was not raised: the Promotion bar's argument moved to
rules.md, where a policy read once belongs. The header still says, first thing, that this
never blocks a merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second wave, 50 more PRs against the fixed gates. The gate fixes worked and did not matter: pushbacks halved from 64 to 32, all 50 finished where 47 did before, and the adversary killed 49 of 232 findings -- 21%, against 20% in the first wave. The gates were making reviews harder to write. They were never what made them wrong. What makes them wrong is visible in the kills: roughly three quarters are premises about how the code behaves, asserted without reading the code that decides it, and most often on the wrong tree. fetch.sh created a worktree of the merge target for the symbol sweep and deleted it on the next line, so a reviewer wanting the base fell back to grepping /mnt/raid0/zufa/skillwork -- the skill's own development branch. Step 7.6 already tells them to check which tree they read. Nothing let them read it. The checkout is kept now, announced in merge_target.txt with the resolved sha and a `git show` recipe beside it, and worktrees older than a day are pruned at the start of the next run so this does not accumulate 182M per review. MAX_FETCH_LINES went 900 -> 930 to hold it: a deliberate raise, argued in the constant's own comment, after several rounds where the answer was to compress instead. Five more from the wave. The N-bullets-for-N-rules rule from the last commit was an overcorrection. aiter#3165 fired B4 and E5 on the same changed file and one accurate bullet covered both, so the review annotated E5 `-- not reported: as a separate bullet` -- false on its face, since E5 was on the card inside the B4 bullet. Making a reviewer write something untrue to pass a gate is worse than the freeloading it prevents, and freeloading stays visible: one bullet against nine FIREs reads as one bullet against nine FIREs. $WORK artifacts are evidence and could not be cited. P6 is by construction about the absence of a measurement, so validation_requirement.json is where its evidence lives; naming it tripped UNTOUCHED-CITATION on four PRs and each rewrote the line to cite source files instead, making the ledger less accurate about where the evidence actually was. `.co` was missing from the citation whitelist, so a PR adding hsa/gfx950/pa/*.co could not cite what it added. CORE_LINE had no `=`, so a tuned-config path could not be written into core_files.txt at all and one review deleted the line to get past it. The refutation parser rejected `WARN SURVIVED (severity reduced) --` as malformed, and did not accept `$WORK/x.txt` as a source -- the form SKILL.md uses throughout. One review, to pass the card gate, deleted a concrete remediation path from its Action and replaced it with "move the file up one level". That is the whole failure mode in one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third wave, 50 PRs. Seven reported Step 1 failing, and six of them diagnosed the same line
independently: the stale-worktree cleanup I added one commit earlier ends its loop body
with an `&&` chain, and a chain's status is the loop's status. A worktree younger than a
day makes the last iteration return 1, the `while` return 1, and `set -euo pipefail` kill
the script -- after the diff, before the symbol sweep, the merge-target checkout and the
`WORK=` line. Every machine that had run a review that day hit it, which is every machine
that uses this. The loop existed to make keeping that checkout affordable and it destroyed
the checkout instead.
Six of the seven finished anyway: found $WORK by mtime among ~600 concurrent temp dirs,
recreated the worktree and the symbol sweep by hand, ran all six gates, and said in their
report exactly which artifacts they had reconstructed. That is the behaviour that made the
bug findable.
aiter#4045 exposed an older one. Its base is `dev/randomflow_pr`, deleted after merge, so
the branch-tip lookup 404s and `set -e` took the run with it -- output on any stream was
"gh: Branch not found (HTTP 404)", naming neither the PR nor the ref, after the diff had
fetched fine. It falls back to the base OID the PR metadata carries and says the fallback
is historical, so staleness against the current tip cannot be judged from it. Verified
against the real PR: it now completes with a usable merge target.
Writing the test for that found a third: a missing baseRefOid prints an empty line, and a
file containing "\n" is not empty, so `[ -s ]` waved through a base that was not there.
The regression test registers a fresh worktree before running fetch.sh, because that is
the state that triggers it -- an empty worktree list exits the loop cleanly and proves
nothing. A second test pins the `if` in the source, since the failure is a shape.
MAX_FETCH_LINES 930 -> 945, the second raise, argued in the constant. Three rounds went
into shaving the comments that explain these bugs to fit the old number, which is the
wrong trade: the budget exists so growth is noticed and argued, and shaving the argument
defeats it. Both budget tests also counted `split("\n")` on a file ending in a newline,
so every limit silently meant one less than it said.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four waves, 200 PRs, an independent adversary behind every review. Step 7.6's self-refutation killed 260-280 candidates per 50 PRs before they reached a card -- real work by any measure -- and 28% of what survived it died anyway to a reader told the findings were false until defended. Four rounds of fixing the tooling moved gate pushbacks from 64 to 33 and completion from 47/50 to 49/50, and moved that 28% not at all. Refuting your own finding catches a premise you never tested. It does not catch a conclusion you are committed to, and instructing yourself more firmly is not a substitute for a reader who was never committed. Step 7.7 hands the card, the diff and the merge-target path to that reader with none of the reasoning attached, and records one line per finding in $WORK/independent.txt. A killed finding comes off the card. The gate needs no ability to spawn anything; it checks that the handover happened, per finding, naming something a third party could open. When no such reader exists the step is not skipped -- `NONE AVAILABLE -- <reason>` passes only if the card also says `not independently refuted`. A review that had an independent reader and one that did not are different objects, and the card is the only place a consumer can tell them apart. Two things are recorded beside the 28% in rules.md rather than left to be worked out. It is an upper bound: the adversary was told to assume falsity and has not itself been audited, so some of those kills were probably sound. And it is not this skill's own bar -- § Promotion bar asks about false CLEARANCE, reporting nothing when something is wrong, which has never been measured here at all. Four rounds went into a number that is not the acceptance criterion. Also rewrites Step 8's description of the gates, which had been describing the ledger as rejecting any FIRE that cites a file the PR does not change. That stopped being true two commits earlier, when the gate moved to requiring an anchor instead. Reviews went on avoiding citations of unchanged files -- the exact behaviour the change was meant to permit -- because the document still forbade what the gate now allowed. Prose that outlives its implementation keeps enforcing the old rule socially. Writing the test for that turned up `diagnostic`, run by Step 8 and described nowhere in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
PR title tags & labels: |
The first version of this branch put 3852 lines of tests, a 256-line blessed baseline and a 528K corpus.tgz into aiter. None of that is needed to run a review. It is the harness I used to build the skill, and asking every aiter checkout to carry a compressed archive of 59 of its own PR diffs to review a sixtieth is the wrong trade -- doubly so for a binary, which upstream's .claude tree otherwise contains none of. What ships is the skill: SKILL.md, fetch.sh, triage.py, rules.md, MAPPING.md. 4520 lines, all text, no fixtures, nothing to unpack. Verified by running fetch.sh against a live PR with the tests deleted from the tree. The tests are not gone, they are elsewhere: branch `skill-enhance-tests`, 299 cases with a 93% mutation score on triage.py and the seven fail-closed guards executed against a stubbed gh. Every measurement quoted in the PR description came from them and can be reproduced there. They stay a maintenance tool for whoever changes this skill next, which is what they always were. Three passages referred to test files that no longer sit beside them -- "Adding New Rules" step 2, the SKILL.md budget note, and MAPPING.md's own header, which is generated, so the generator was the thing to fix. Each now names the command that catches the drift rather than the test that used to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zufayu
force-pushed
the
skill-enhance
branch
from
September 5, 2026 08:11
b0488ce to
f14a580
Compare
`struct_field_churn` was the loose ABI check, superseded by `pinned_struct_churn`, which asks the tree whether a layout is actually pinned instead of firing on any field churn. `ci_test_scope` was superseded by `uncovered_test_paths`. Both survived their replacements as dead code: 65 lines carrying docstrings that explain a design no longer in force, which is worse than no explanation, because the next reader has to work out which of the two struct checks is live. Neither is referenced by triage.py, fetch.sh, SKILL.md, rules.md or the test suite. Found by walking the AST for top-level functions whose name appears exactly once in the file -- their own definition. The skill runs unchanged, MAPPING.md still matches the deriver, and the suite is 298 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
perf_claims knew x, %, us, ms, TFLOPS, GB/s and tok/s. Run against aiter#5221, whose headline is `gsm8k 200q from **100.0 s to 62-67 s**` -- a 38% wall-clock win and the whole reason the PR exists -- it printed "no numeric performance claim in the PR description". That is worse than missing it quietly: the reviewer is told the axis was checked and came back empty, and P1 and P3 are adjudicated against that. What is matched is the transition between two durations, not the unit. A bare second over 279 PR descriptions hits four lines and two of them are not claims at all: a `600s wait` timeout constant and a `4 passed in 541.10s` pytest summary. `from A s to B s` hits one line in the same 279 -- #5221's -- and neither trap. Both traps are tests. A transition also carries its own baseline. "from 100.0 s to 62-67 s" names both sides of the comparison in the sentence; asking what it was measured against is asking something the line already answered. 0.36% of the corpus, which is below the rate at which detectors have been turned down here, and the rate is the wrong test again: this is not a detector finding less, it is an artifact making a false statement about what it looked at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both found by running the whole skill on aiter#5221 -- one file, +99/-45 -- before it found anything about the PR. `refutations` and `independent` decide "was every reported finding attacked" by counting the findings on the card. With no card to read they counted zero, skipped the check, and returned green: the one state in which they certainly cannot do their job was the state they passed. Both now refuse without a card. SKILL.md's ordering already wrote the card first, so nothing correct changes; what changes is that skipping it is no longer free. The card gate matched a finding to its backing by literal string, so `62–67 s` in a card against `62-67 s` in a verdict, or `414,720` against `414720`, came back as "appears in no verdict, diagnostic or blind-spot line" -- with the message naming neither the dash nor the comma, so the reviewer repaired it by guessing. SKILL.md's own example card is full of em dashes, which means the document was teaching the shape the gate rejected. Dashes, quotes and thousands separators are folded before comparison; a finding backed by nothing is still caught, and there is a test for each direction. Also restores the five tests written for the seconds-unit fix in the previous commit. They were lost between writing and committing, because the suite is kept outside this branch and was recovered from a snapshot older than the tests. That is a bad place to keep them, and the next commit puts them on skill-enhance-tests where they belong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
junhaha666
previously approved these changes
Sep 7, 2026
Step 4's tiers were all Python -- "can `import aiter` still succeed" -- so a review of a pure C++ PR reached the gate with nothing to declare and passed on one `NONE` line. aiter#5273 rewrites a quant kernel and changes csrc/include/aiter_opus_plus.h, which 18 translation units include; Step 4 exerted no force on it at all, and the reviewer said so. A header is Tier 2 when 10 or more translation units include it, counted from the tree. The threshold is measured: over 600 open PRs, >= 10 puts 56 diffs into tier 2 (9.3%) beside the Python table's 137 (22.8%); >= 20 drops to 47 and excludes aiter_opus_plus.h at fan-in 18, and no touched header sits between 20 and 50, so the choice was this band or only the three giants. Counted, not listed, for the reason D9's body already records about name lists: one goes stale, and a header that becomes widely included later joins on its own. Fan-in is computed per tree and cached, so the answer is about the merge target rather than whatever is on disk. One exemption, by name, because there is exactly one of its kind. rocm_ops.hpp is the pybind11 op registry: fan-in 104, changed in 36 of the 600, and every one of those is a mechanical append for a new operator. A tier-2 rule firing on all 36 teaches the reader to skip it, which costs more than the rule is worth. The gate call in SKILL.md now passes $PROJECT_ROOT. Without it `core_files_in` falls back to the Python table and this change does nothing in the real flow -- the same implementation-changed-but-call-site-not defect that has already landed here three times, caught this once by testing the flow rather than the function. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It ran past the column the surrounding prose wraps at, and read as one sentence joining "counted from the tree" to "ranked by commit frequency", which are two different things.
FlyDSL needs patching after merge 1.66x as often as the repo average: 20 of the 98 fix-commits that reference another PR touch it, against a 12.3% share of all commits (194/1577), one-sided binomial p=0.015, measured on aiter main 636098e. Its revert rate is NOT elevated -- 5 of 40, against that same 12.3% -- but n=40 has no power in either direction, so that is "cannot tell", not "no". The patching is where the signal is, and post-merge patching is what review is for. Of its 21 precise fixes, 9 fall into two mechanisms and neither had a rule. B8 -- a buffer or descriptor bound not tied to the tensor's real extent. de9f1f8 bound arg_scale_a with max_size=True while scale_a is per-row(M) and M is ragged; the comment it also corrected ("B and scales are exact-multiple in N") was false for scale_a, so the code was written to match a premise that did not hold. Nothing covered this class: num_records, max_size, buffer_resource and oob_ appear nowhere in rules.md, and B2 -- the one out-of-bounds rule -- is tl.load without a mask, which cannot fire on a backend that has no tl. A FlyDSL PR deriving memory-safety got B2 D1 G1. D12 -- a new ops-side contract the AOT pre-compiler was not taught. #4397 added resolve_flydsl_stage1_tile_n and resolve_flydsl_stage2_tile_k in one commit, wired only stage2 into AOT, and the stage1 drift lived 32 commits until #4429. Both collectors were checked against the commits that introduced the defects, not only against fixtures. flydslbounds reports arg_scale_a on 3910ab3 and falls silent on de9f1f8, so the fix does not trip its own rule. aotpair reports resolve_flydsl_stage1_tile_n on 3135022 and stays quiet about stage2 -- which that same commit did wire in -- which is the distinction the rule exists to make. The first cut of D12 looked for "ops changed, AOT untouched" and would have found nothing: #4397 does touch AOT (grouped_moe.py +19, moe.py +9). The defect is a path missed, not a side skipped. Separately, the card gate had exactly one way through and it was dishonest. On wave5 PR4862 an agent found a CI problem at Step 7 that no rule covered, the gate called it UNBACKED, and the only move that clears that gate is for the finding to have been known earlier -- so it edited answers.txt, a file that had already passed its own gate, and described this itself as "slightly dishonest bookkeeping". A gate whose only path through is fabrication trains fabrication. `-- late finding:` backed by an append-only late_findings.txt is the exit; a hash ledger keyed on the FIRST passing hash is the lock. Neither half works alone: the exit alone leaves the door open, the lock alone leaves a reviewer who genuinely found something late with nowhere to put it. Two changes planned from reading the code and dropped after measuring them: adding .cpp to is_kernel (aiter has 12 .cpp/.cc files and none contains a device kernel, so excluding them is correct) and dropping the .py test in flydsl-kernel (non-Python is deliberately left to the C path, and a test says so).
Both found by running the skill on #5301 -- 2385 added lines of new FlyDSL kernels -- not by reading it. It came back with no candidates and neither new kernel had been looked at. The tree. The collector took $PROJECT_ROOT, the merge target. A file the PR ADDS is not there at all, so #5301's sparse_mla_prefill.py and sparse_mla_decode.py were invisible -- and new kernel files are the ones most worth reading. Worse for a MODIFIED file: the collector matches AST node line numbers against the diff's HEAD-side line numbers, and the base-side text does not line up with those at all, so any hit would have been at the wrong site. $WORK/head is the tree of the diff's own files at head and is what both collectors now read. aotpair takes the merge target separately, because the symbol table describes what AOT depends on today. The historical checks passed because they were hand-built as `git show <sha>:<path>`, which is head-side content. The fixture and the real invocation disagreed and only the fixture was ever run. The spelling. `max_size` DEFAULTS to True in both constructors, and flydsl's own docstring says what that means: "max_size=True (default) sets the descriptor to 0xFFFFFFFF". Matching the literal text `max_size=True` therefore read every call that omits it as bounded -- 21 of the 138 calls in aiter/ops/flydsl, 15%. #5301's one added buffer call is exactly that shape: fx.rocdl.make_buffer_tensor(view), bare. The test is now "neither num_records_bytes nor an explicit max_size=False", and positional forms are read per constructor, since ptr_buffer_resource's second positional IS the bound and reading it like the other two reports every use of it. On #5301 B8 now reports one candidate: _flat_buffer_tensor builds a view whose extent is the runtime `size` and hands it to make_buffer_tensor bare, so the descriptor is 0xFFFFFFFF. The PR's own new test says that path has faulted -- "Memory access fault by GPU node-N" for a KV pool past 2 GiB -- and is guarded host-side rather than in the descriptor. Whether that is the right trade is the reviewer's call; B8 emits candidates. A test now asserts fetch.sh passes $WORK/head to both, because this class of defect is invisible to every unit test: the fixtures build their own root.
…y looked right Pointing the FlyDSL collectors at $WORK/head was correct and insufficient: that tree was populated only inside `if grep -q "invariant-removed\|api-signature"`. On a PR deriving neither family it is an empty directory, and a collector that reads files finds nothing to read. #5207 has six buffer calls in its diff -- five bounded, one a bare `make_buffer_tensor(W_scale, max_size=True)` -- and flydslbounds returned zero. #5301, which the previous commit was verified against, deletes a guard and so derives invariant-removed. That is the only reason the fix appeared to work. A fixture cannot see this: every test here builds its own root and hands it over, so the tree is always populated. Materialising head is now unconditional; only the `evidence` call stays behind the family test. The cost is a few `git show` calls against objects already fetched at line 125. On #5207 B8 now reports exactly one candidate and it is worth asking about: `W_scale` is bound with `num_records_bytes=n_heads * self.n_per_head * 4` at line 184 of the same file, and with `max_size=True` at line 92, inside a BlockScale whose constructor already receives head and n_scale_cols. Same tensor, two treatments, one of them unbounded. Also verified silent for the right reason on #5146 (its one added buffer call is buffer_ops.py forwarding variables, not a kernel binding a tensor) and on #5282 (no buffer construction in the diff at all).
Five comments translated. The sixth is not a comment: BASELINE matches the words a perf table uses to name what it is measuring against, and aiter PR descriptions are partly Chinese, so an English-only list marks a table that DOES name its baseline as one that does not -- a false positive, not a missed one. Those alternatives are kept and spelled as \uXXXX escapes, with the comment giving each one's meaning in English, so the file is ASCII and the verdict is unchanged. Verified word by word: all eleven still match, and so do the English terms and the arrow. grep for CJK over the eight files in this PR now returns nothing.
Measured with the pinned CI version (ruff 0.16.0), against the file as it stood at 28dcc0b: 104 findings before, 104 after. This branch now adds none. Note that CI's Black and Ruff jobs do not cover these files at all -- .claude/ is in .gitignore and both tools respect it, so `ruff check .` from the repo root matches triage.py zero times. Those two green checks say nothing about this PR's eight files. The findings were found by pointing ruff at the file directly. Fixed: six UP031 (percent format to f-strings), two ISC004 (parenthesise the implicit concatenation so it cannot read as two collection elements), two SIM115 (read through pathlib rather than leaving a file object to the GC), one FURB167 (re.I to re.IGNORECASE on the constant this branch added). One BLE001 is suppressed rather than narrowed. It guards ast.unparse in a display helper: the failure surface is not enumerable, and a crash there would take a whole forensic down over a label. The noqa states that. Two findings on lines this branch touched are left alone because they predate it and the counts confirm it: I001 on the single-line import block, which is this file's existing style, and FURB167 on BASELINE, which already used re.I. Fixing either would put two styles in one file. Output verified byte-identical on #5207 after the rewrite, and 345 tests pass.
junhaha666
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
review-pr: gates that can be checked, and 200 PRs of evidence about them
The
review-prskill on main is one 1372-line document. A reviewer reads it, decides whatto look at, and writes a card. Nothing establishes that any step actually happened, so the
cheapest path through it is to skip to the card. That is not hypothetical: aiter#4994 was
approved by a reviewer who never ran the validator, which dies on line 24 of that PR's
target.
This replaces the document with the same review expressed as artifacts and gates. Every
step writes a file; a program checks the file before the card may be written. It also carries
three fixes to
validate-kernel-pr, whichreview-prcalls at runtime — the two skills areone system and change together.
What a reviewer does differently
Step 1 is a program.
fetch.shpulls the diff, derives which rules apply, and runsthirteen forensics up front instead of telling the reviewer to go and grep. The 1372-line
document becomes a 500-line entry that expands per diff: measured over the 600 open PRs at
the time of writing, the median review carries 15 rules of 64 and 130 lines of rule
text, against 44 rules and 319 lines read every time before. (That measurement predates
B8 and D12. Re-measured on the same 59-PR corpus after adding them, counting
triage.py expandoutput lines: 63 rules total, 496 lines if read end to end; median 16 rules and153 lines per review. The two sets of figures use different denominators — the earlier
one is quoted as it was taken rather than silently restated under a definition it was not
measured with.) Two reviewers on two checkouts derive
the same rule set for the same PR, which they did not before.
The forensics answer questions a reviewer would otherwise have to grep for, against the
merge target rather than whatever branch happens to be checked out:
aiter.ops.flydsl.utils, which [Triton/Gluon] [HIP] [CK] Remove obsolete availability helpers #5116 deletedones are where a null check becomes a size check and a
TORCH_CHECKbound loses a factorSeven gates run before the card:
answers,diagnostic,corefiles,ledger, thencard,refutations,independent. Each fails closed — a missing artifact is a hardfailure, not a warning. They check that the work happened and that the card reports that
work. None of them judges whether a finding is correct, which a program cannot do.
Two are aimed at the findings rather than the process. Step 7.6 requires an attempt to
kill each finding before reporting it. Step 7.7 requires that someone who has not seen
the reasoning tried too — a second agent or a person, findings false until defended — or
that the card says plainly that nobody did.
The numbers, including the ones that do not flatter it
The skill was run end to end on 200 open PRs, each review followed by an independent
adversary instructed to assume every finding was false. Four waves, 397 agents.
reasoning and was told to assume they were false. The other 28% did not.
That is the signal-to-noise figure to plan around: roughly three findings in four hold up
under someone actively trying to break them, and the fourth costs a reviewer the minutes it
takes to dismiss. Four rounds of fixing the tooling moved gate pushbacks from 64 to 33 and
completion from 47/50 to 49/50, and moved that ratio not at all.
Two things belong next to that ratio, and both are written into
rules.mdrather than leftfor a reader to work out:
not itself been audited, so some of the 28% it killed were probably sound.
rules.md§ Promotion bar asks about falseclearance — reporting nothing when something is wrong. That has never been measured
here. Until it is, the skill stays advisory and no part of it may gate a merge, which is
the first line of
SKILL.md.A tool that hides this number stops being believed the first time someone measures it. It is
stated so the card is read as a set of leads, which is what it is.
validate-kernel-prThree changes, all found by running it:
AssertionErroristhe standard shape of a failing
op_teststarget; the receipt recordedexitstatus 0forit, so a red run could be read as green.
git applyintroduces added files asuntracked and
git diff HEADdoes not list them, so a PR whose whole point is a new kernelread as touching no kernel code — and a tolerance widened beside it was reported under the
weaker finding.
third of the suite look broken: the second finds every candidate GPU locked, degrades to
NO_GPU, and returnsINCONCLUSIVEwhere the test expectsPASS. That is correctbehaviour reported as five mystery failures. It now says so in the first line.
What is in this PR, and what is not
Five files for
review-pr—SKILL.md,fetch.sh,triage.py,rules.md,MAPPING.md— plus three fixes to
validate-kernel-pr. 4850 lines, all text, no fixtures, nothing tounpack. Running a review needs nothing else.
The test suite is not here, deliberately. It is 345 cases, a 93% mutation score on
triage.py, the seven fail-closed guards infetch.shexecuted against a stubbedgh,and a 59-PR corpus pinning the deriver's output. That corpus was a 528K compressed archive,
and asking every aiter checkout to carry an archive of 59 of its own PR diffs in order to
review a sixtieth is the wrong trade — doubly so for a binary, which this tree otherwise
contains none of.
It is kept outside this repository, against the same commit this PR branches from, and
every number quoted above was produced by it. Say the word and I will push it wherever you
want it reviewed — or bring it into this PR minus the corpus, since the 59-PR pin is the
only part that needs a fixture and the only part that cannot be made small.
Six of the gate defects fixed here were found by running the skill on real PRs, not by
reading it. The citation checks had been rejecting the cross-file evidence the rules
explicitly ask for — reviews were deleting filenames and describing headers in prose to get
through, and one deleted a concrete remediation path from its own recommendation. Each fix
carries a test, on that branch, naming the PR that exposed it.
Added since first review: two rules for FlyDSL, and an exit for the card gate
FlyDSL needs patching after merge 1.66x as often as the repo average. 20 of the 98
fix-commits that reference another PR touch it, against a 12.3% share of all commits
(194/1577) — one-sided binomial p = 0.015, measured on
636098e5a.Its revert rate is not elevated: 5 of 40, against that same 12.3%. With n=40 that is
"cannot tell", not "no elevation". The post-merge patching is where the signal is, and
post-merge patching is what review is for.
Of FlyDSL's 21 precise fixes, 9 fall into two mechanisms and neither had any rule:
num_records,max_size,buffer_resource,oob_appear nowhere inrules.md. B2 — the one out-of-bounds rule — istl.loadwithout a mask, and there is notlin FlyDSL, so a FlyDSL PR derivingmemory-safetygotB2 D1 G1, none of which can fire on itaiter/aot/flydslappears in one comment in the whole skill. A1 pairs by name stem, andparse_csvshares nothing withresolve_flydsl_stage1_tile_nBoth collectors were checked against the commits that introduced the defects, not only
against fixtures.
flydslboundsreportsarg_scale_aon3910ab397— the unboundedmax_size=Truethatde9f1f84afixed 395 commits later — and falls silent onde9f1f84aitself, so theremedy does not trip its own rule. On
ed33d78a5it reports the descriptor missingoob_inner_bound, 1 candidate, 1 real.aotpairreportsresolve_flydsl_stage1_tile_non313502261([Kimi-K3] FlyDSL SiTUv2 MoE kernels, strided grouped-topk, tuned configs #4397) and stays quietabout
resolve_flydsl_stage2_tile_k, which that same commit did wire into AOT. Thatdistinction is the rule: the drift lived 32 commits until fix(flydsl-aot): map Situv2 activation in MoE AOT parse_csv #4429.
The first cut of D12 looked for "ops changed, AOT untouched" and would have found nothing —
#4397 does touch AOT (
grouped_moe.py+19,moe.py+9). The defect is a path missed,not a side skipped. Measuring it before shipping it is the only reason that is known.
The card gate had one way through, and it was dishonest
On wave5 PR4862 an agent found a CI problem at Step 7 that no rule covered. It appeared in
no verdict, diagnostic or blind-spot line, so the gate called it
UNBACKED. The only movethat clears that gate is for the finding to have been known earlier — so the agent went
back and edited
answers.txt, a file that had already passed its own gate, writing thefinding in as a Step 7.5 blind spot. It described this itself as "slightly dishonest
bookkeeping".
That is worse than any regex defect in this PR: the gate was rewarding fabrication.
Two halves, and neither works alone:
-- late finding: <step>on the card, backed by an append-onlylate_findings.txt. Alone, it only adds a cheaper honest path; the back door stays open.re-checks it, reporting
BACKDATED-ARTIFACT. The baseline is the first recorded pass,so editing an artifact and re-running its gate cannot launder the edit. Alone, it leaves a
reviewer who genuinely found something late with nowhere to put it.
No recorded hash is not an accusation — a hand-run gate never went through the answers gate,
and treating a missing baseline as tampering would make the honest path the noisy one.
Two changes planned from reading the code, dropped after measuring
Recorded because the reasoning that produced them looked sound:
.cpptois_kernel()— aiter has 12.cpp/.ccfiles and none contains__global__,__device__,hipLaunchKernelor<<<. They are the C++ interface layer,tests, and AOT tooling. Excluding them is correct;
C_LIKEcontainscppbut serves thestruct-ABI scan, which is a different question.
.pytest inflydsl-kernel— not a hardcoding slip but a deliberatesplit: non-Python goes to the C path, and a test says so in as many words.
Blast radius
expected.jsonmoves on 11 of 59 corpus PRs. All 11 are FlyDSL; nothing else changestriage. 345 tests pass, plus the 90 in
validate-kernel-prrun exclusively.Verified by running it, on four FlyDSL PRs
Fixture-only verification hid three defects, each invisible to unit tests because every
fixture builds its own root and passes it in:
$PROJECT_ROOT(the merge target), where a file the PR adds doesnot exist and a modified one does not line up with head-side line numbers;
max_sizedefaults to True — flydsl's docstring: "max_size=True (default) sets thedescriptor to 0xFFFFFFFF" — so matching the literal text read 21 of 138 calls (15%) as
bounded;
$WORK/headwas materialised only inside theinvariant-removed|api-signaturetest, soon a PR deriving neither it is an empty directory. [FlyDSL] [Kernel] gfx950 FP8 sparse MLA prefill and decode (FlyDSL) #5301 deletes a guard and derives one,
which is the only reason fix (1) appeared to work.
_flat_buffer_tensorhands a runtimesizeview to a baremake_buffer_tensor; the PR's own new test says that path returnedMemory access fault by GPU node-Npast 2 GiBbuffer_ops.pyitselfW_scaleis boundnum_records_bytes=n_heads * self.n_per_head * 4at line 184 andmax_size=Trueat line 92, in aBlockScalewhose constructor already hasheadandn_scale_cols345 tests. Two of them read
fetch.shrather than the collectors, because that is wheredefects (1) and (3) lived.