Skip to content

ci: lint the crates the test deny-list was quietly excluding from compilation - #2243

Merged
justinchuby merged 1 commit into
mainfrom
squad/gaff-lint-coverage-denied-crates
Aug 26, 2026
Merged

justinchuby merged 1 commit into
mainfrom
squad/gaff-lint-coverage-denied-crates

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Four workspace members are linted by nothing, because a runtime exclusion was silently doing duty as a compile exclusion. One of them was uncompilable on main for 22 commits earlier today and no required check said so.

The mechanism

#2058 already established the principle: cargo clippy only ever checks — it never links and never runs a test binary — so "needs a GPU", "needs wheel staging", "isn't a unit-test target" have no force over it. That PR restored lint coverage to 21 tested crates that had lost it for a reason that did not apply.

The argument was never extended to the denied crates themselves. The lint lane stayed:

selected = packages - denied

and every one of the five DENYLIST reasons is a runtime reason:

crate DENYLIST reason kind of reason
onnx-genai-ort-sys build script downloads a native ORT distribution build
onnx-genai-bench benchmark/perf crate; not a unit-test CI target execution
onnx-runtime-ep-cuda runtime tests require a CUDA GPU execution
onnx-genai-python requires wheel packaging/runtime DLL staging execution
onnx-runtime-python requires wheel packaging/runtime DLL staging execution

Measured on main before this PR — 4 of 60 members reached by no clippy invocation anywhere:

linted by NOTHING anywhere : ['onnx-genai-bench', 'onnx-genai-ort-sys',
                             'onnx-genai-python', 'onnx-runtime-python']

Why it matters, concretely

onnx-runtime-python did not compile on main from 66ca26a5d (#2132) to 881c3165c (#2173) — 22 commits, ~7.5 hours.

The only job that builds that crate is CUDA compile, via a hand-written -p onnx-runtime-python outside the resolver. It is not a required check. On the breaking merge commit it reported exactly what it should have:

CUDA compile (Linux x86_64)   = failure
CUDA compile (Windows x86_64) = failure

Required checks on main are only Fast (Linux x86_64) and Rust quality. The signal existed, was correct, and was discarded. This PR puts that same breakage in front of a required check.

The two-arm proof

I reverted #2173 to reintroduce the real break, and ran both lane definitions against that identical tree:

lane packages rc E0277
old (packages - DENYLIST) 55 0 — green 0
new (packages - LINT_EXEMPT) 58 101 — red 78

The old lane is green not because it compiled the crate and found it sound, but because the broken crate was never in its input set. Both are "green"; only one of them is evidence.

The change

  • New LINT_EXEMPT set, separate from DENYLIST, whose entries must name a reason clippy itself cannot get past. The docstring says plainly that "needs a GPU at runtime" / "isn't a unit-test target" / "needs wheel staging" do not qualify — that is the 20 crates are tested by CI and linted by nothing; two clippy denials are live on main because of it #2058 mistake with new nouns.
  • lint lane is now packages - LINT_EXEMPT (55 → 58).
  • The lint-coverage guard's domain widens from tested - linted to (packages - LINT_EXEMPT) - linted. The old domain could not name a denied crate by construction — the guard whose entire job is to notice unlinted crates was structurally unable to see these four.
  • Stale-entry validation for LINT_EXEMPT, matching the existing DENYLIST check.
  • Fixes the one clippy error this exposed: a cfg-conditional vec_init_then_push in onnx-runtime-python, rewritten as two mutually exclusive bindings with identical semantics.

What stays exempt, and why that is honest

  • onnx-runtime-ep-cuda — its build script needs a CUDA toolchain the quality runner does not install. It passed clippy locally, but this box has nvcc, so a local pass here is not evidence about the runner. Recorded as such. It is already linted by the CUDA job.
  • onnx-genai-bench — 28 dead-code findings in src/bin/compare.rs; an entire Direct* reporting path that nothing constructs. Filed as onnx-genai-bench: linted by nothing, and its Direct* report path is entirely dead code #2242, including the possibility that matters more than the lint: if that path was meant to be wired up, benchmark output someone believes is produced is not being produced. Exempted on explicitly temporary grounds rather than left in DENYLIST where the exclusion looks principled.

onnx-genai-ort-sys costs nothing new: six of its dependents are already in the lane, so its build script already runs there. Confirmed no download occurs.

Tests

Two self-test arms; the first is a real discriminator, not a restatement:

  • verify --simulate-unlinted onnx-runtime-python (a denied crate) — under the old domain this returned 0 and named nobody, because a denied crate could not enter tested - linted. Verified by reverting the one-line domain change and re-running:

    FAIL  verify --simulate-unlinted onnx-runtime-python (a denied crate)
          expected the report to name: ['onnx-runtime-python']
    

    It fails against the previous code and passes against this one.

  • verify --simulate-unlinted onnx-genai-bench (lint-exempt) pins the other direction, so the widened domain cannot quietly grow to include crates the runner cannot build. It correctly does not flip between the two versions.

Suite is self-counting (82/82 arms behaved as stated), so a miscount fails loudly rather than under-reporting.

Validation

workspace test package coverage ok: 55 tested, 5 denied
workspace lint coverage ok: 58 linted by 11 clippy invocation(s), 2 lint-exempt
workspace test package self-test: 82/82 arms behaved as stated
required-lane coverage ok: 55 package(s) tested by ['Fast (Linux x86_64)', 'Rust quality']
cargo clippy --locked --all-targets <58 packages> -- -D warnings   ->  rc=0

The clippy run was checked for having done real work rather than trusting rc=0: 49 compile/check units, with onnx-runtime-python, onnx-genai-python and onnx-genai-ort-sys each confirmed present by name in the log.

Limitations

Local validation is necessary and not sufficient — merging on required CI via normal auto-merge, no admin, no bypass.

…pilation

#2058 established that a *runtime* reason has no force over clippy, which only
ever checks -- it never links and never runs a test binary -- and restored lint
coverage to 21 tested crates that had lost it for a reason that did not apply.
That argument was never extended to the denied crates themselves. `lint` stayed
`packages - DENYLIST`, and every DENYLIST reason is a runtime reason, so four
workspace members ended up linted by nothing at all:

    onnx-genai-bench  onnx-genai-ort-sys  onnx-genai-python  onnx-runtime-python

The gap is not theoretical. #2132 left `onnx-runtime-python` uncompilable on
every platform for 22 commits (66ca26a..881c316, ~7.5h) until #2173 fixed
it. The only job that builds that crate is `CUDA compile`, which is not a
required check: it went red on the breaking merge and did not block it. The
signal existed and was discarded.

Derive `lint` from a new LINT_EXEMPT set instead, whose entries must name a
reason clippy itself cannot get past, and widen the lint-coverage guard's
domain from `tested - linted` to `(packages - LINT_EXEMPT) - linted`. The old
domain could not name a denied crate by construction, so the guard whose job is
to notice unlinted crates was structurally unable to see these four.

Lane goes 55 -> 58 packages. onnx-genai-ort-sys costs nothing new: six of its
dependents are already in the lane, so its build script already runs there.
onnx-runtime-ep-cuda stays exempt because its build script needs a CUDA
toolchain the quality runner does not install -- it passed locally only because
this box has nvcc, which is not evidence about the runner. onnx-genai-bench
stays exempt on explicitly temporary grounds (#2242).

Fixes the one clippy error this exposed in onnx-runtime-python: a
cfg-conditional vec_init_then_push, rewritten as two mutually exclusive
bindings with identical semantics.

Two self-test arms, one of which is the discriminator: --simulate-unlinted on a
DENYLIST crate returned 0 and named nobody under the old domain, so it fails
against the previous code and passes against this one. The second pins the
exempt direction so the widened domain cannot grow to include crates the
runner cannot build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby enabled auto-merge (squash) August 26, 2026 23:29
@codecov

codecov Bot commented Aug 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.64%. Comparing base (bb08c5a) to head (637c2bc).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2243      +/-   ##
==========================================
- Coverage   80.64%   80.64%   -0.01%     
==========================================
  Files         432      432              
  Lines      220114   220114              
  Branches   220114   220114              
==========================================
- Hits       177512   177503       -9     
- Misses      36717    36725       +8     
- Partials     5885     5886       +1     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (ø)
cli-ort-windows 72.10% <ø> (+0.09%) ⬆️
mlas 85.90% <ø> (ø)
offline 80.76% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant