feat(skills): add supplemental FlyDSL review checks by jhinpan · Pull Request #1047 · ROCm/FlyDSL

jhinpan · 2026-08-20T23:39:40Z

FlyDSL review workflows currently lack deterministic preflight checks for two
recurring candidate shapes: legacy APIs added to kernel consumers, and new tests
that are not reached by a file's script entry point.

This PR adds flydsl-review-checks as a supplement to an existing review. The
caller supplies the pinned diff and matching head checkout; the skill does not
retrieve the PR, create worktrees, perform the semantic review, or publish
findings.

Checks

scan_legacy_spelling.py reports legacy API spellings on added
kernels/**/*.py lines while filtering the sanctioned buffer implementation
and visible comments, strings, and imports.
scan_unreachable_tests.py matches added test definitions against the head
AST and reports definitions without a statically visible script entry path. It
recognizes direct local calls and unfiltered pytest.main([__file__])
dispatch.

check_candidates.py runs both checks through one entry point:

python3 .claude/skills/flydsl-review-checks/check_candidates.py \
  --diff "$review_diff" \
  --head "$head_worktree"

The combined command returns 0 for no candidates in the supported scopes, 1
when source review is required, and 2 when an input or scanner fails. A zero
status is not a clean PR verdict; correctness, API, performance, and runtime
validation remain with the calling reviewer.

Why these two

The original August 20 experiment reported 479 human review comments across 163
of 865 FlyDSL PRs and grouped repeated feedback into six candidate families.
Legacy API spellings were the largest family. Prompt-only review variants did not
reliably surface those candidates, which supports a deterministic preflight.

In the seeded comparison, the baseline reviewer caught four of five candidate
families. Test entry-point reachability was the only miss; adding that check
changed the result from 4/5 to 5/5. Replaying PR #481 at
7e117e29c9c4582158aed33da4ee6dfe14d2c76e reports the same three
fused/quant test definitions omitted from the script path that the maintainer
identified.

F2, F3, F4, and F6 are not active rules because the baseline reviewer already
caught their seeded cases. Adding them had no measured benefit.

The selection record, source links, and September refresh boundaries are in
review-evidence.md.

Evidence and limits

The 41 CPU regressions cover both scanners and the combined entry point, including
the false-positive and false-clean cases identified during review. All nine prior
review threads have been addressed at
2480f877aa7f74a24d1ce857b48ba92ff0699cf5.

These checks parse source and diff text; they do not execute reviewed code or
establish kernel correctness or performance. The historical corpus counts and
seeded comparison are selection evidence, not held-out precision or recall.
Aliases, partial diff context, dynamic dispatch, pytest configuration, fixtures,
plugins, and runtime branches can require manual review.

This PR contains only the FlyDSL candidate checks. The manylinux repository
refresh fix discovered while validating the earlier branch has moved to
#1115.

The remaining merge gates are the current CI run and maintainer approval; there
is no known unresolved implementation finding at the current head.

…view history An agent reviewing a FlyDSL kernel PR currently has two rules for FlyDSL (in aiter's review-pr) and no knowledge of what this repo's maintainers actually ask for. This adds that, taken from the history rather than invented. Corpus: all 865 PRs, of which 163 carry human review, 479 comments total. coderfeli (273 comments over 101 PRs, Jan-Aug 2026) and sjfeng1999 (57 over 32) dominate; the taste is consistent enough across eight months to encode. Every rule cites the PRs the objection was actually made on. Six families, ordered by how often they occurred: F1 legacy spelling where the current one exists -- 46 of the 479 comments, by far the largest. ir.*/ArithValue -> fx types, scf.If -> plain Python if, buffer_ops -> fx.copy, SmemAllocator -> SharedAllocator, make_ptr -> recast_iter. Ships as scan_legacy_spelling.py rather than prose: validated against the files maintainers commented on, it flags 10 of 12; the two it does not flag are cases where the pattern is not on an added line. F2 a default argument that silently aliases another buffer (yanguahe #844 filed three blockers on one PR for this; the same shape recurred in #629) F3 duplicated code paths that should be shared F4 config/dispatch/tile selection living inside the kernel file F5 a new test the entry point CI runs cannot reach F6 unvalidated cast at the C ABI boundary It deliberately does not restate flydsl-kernel-authoring or kernel-code-cleanup, does not duplicate review-pr's correctness rules, and does not judge performance. F1 is a scanner and not a rule for a measured reason: across four controlled arms on 14 PRs, a rule rewritten in prose caught 0 of 3 of the defects it targeted, including after the instruction was moved to where the agent reliably executes it.

Copilot

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

Five of the six families are removed before merge because a controlled test said they add nothing. Each was seeded into the real softmax backward kernel, opened as a PR, and reviewed twice -- once with review-pr alone, once with review-pr plus this skill. review-pr alone already caught four of the five: the default argument aliasing another buffer, the duplicated path, the config table inside the kernel file, and the unvalidated cast. Only the unreachable-test family needed this skill. Rules that change nothing are not free. They cost reviewer attention and dilute the findings that matter, and the output contract allows five findings per PR. What remains is F1, the legacy-spelling family: 46 of the 479 human review comments in this repo, shipped as scan_legacy_spelling.py, which flags 10 of the 12 files where a maintainer actually raised it. The seeded test is biased toward the control -- a 4-to-20 line diff whose only change is the defect is far easier to spot than the same pattern buried in a 2000-line PR -- so this is not proof the dropped families are worthless. It is that they have no measured benefit today. The corpus analysis is retained outside the repo and they can return with evidence; the file now states that standard for anything added.

…ged an outcome Dropping F5 with the other four was wrong. It is the single family review-pr alone missed in the seeded test (4/5 -> 5/5), so it is the only one with direct evidence that it adds something, and it was cut for tidiness rather than for a reason. It returns as a scanner rather than prose, which is what the evidence supports. scan_unreachable_tests.py parses the test file, resolves what `__main__` reaches transitively, and reports only tests the diff ADDS. Validated three ways: - replaying #481 it names exactly the three tests coderfeli named (test_rmsnorm_fused_add and its dynamicquant/smoothquant variants -- "the fused/quant variants are not exercised in that path") - on a seeded PR adding one pytest-only test, it flags that test and nothing else - on a comment-only control it reports nothing Only added tests count. A first version reported absolute state and fired on 6 of 7 untouched tests in one file, because pytest-first files with a one-line __main__ are the norm here; that version would have been noise. F5 pairs with validate-kernel-pr from the other side: this catches a test that will not run, the shape grid catches shapes a running test never reaches.

Make the skill a supplemental preflight with one combined entry point and leave PR retrieval and semantic review to the caller. Remove unrelated manual review guidance and revert the manylinux CI change for separate delivery.

Adapt the two source scanners and their regressions from #1047 at 2480f87. Feed raw leads through the existing G/I verification path, preserve preflight identity and failures, and accept verbose CLI result arrays.

Copilot AI lite review requested due to automatic review settings August 20, 2026 23:39

Copilot AI reviewed Aug 20, 2026

jhinpan changed the title ~~Add review-flydsl-kernel skill distilled from this repo's review history~~ Add review-flydsl-kernel: legacy-spelling scanner distilled from 479 maintainer comments Aug 21, 2026