Repository navigation
fix(ep-cuda): restore the CUDA lane -- five defects, only the first of which was reported - #1881
Conversation
Independent scan: this is the only instance in the workspace, and
|
45bb380 to
ae1879b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1881 +/- ##
==========================================
+ Coverage 80.48% 80.84% +0.36%
==========================================
Files 413 415 +2
Lines 200747 204450 +3703
Branches 200747 204450 +3703
==========================================
+ Hits 161564 165282 +3718
+ Misses 33666 33592 -74
- Partials 5517 5576 +59
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ae1879b to
65b9aa8
Compare
Status: the lane this PR fixes is green in CI; remaining reds are inherited and attributed
This matters more than the local result: the honesty script's whole purpose is that it must run in CI, where both feature configurations are built. A local green could have been an artifact of my machine. It wasn't. The five failing lanes are not this branch
Verified rather than assumed. The Fast lane's only Structurally this branch cannot reach those lanes: Not self-mergingI have the local green for my own scope — honesty script exits 0, lane clippy clean, 539 lib + 4 new + 1 capture-sync passing, fmt clean, rebased onto I am leaving auto-merge armed rather than using If someone with context on #1890 wants this in sooner to unblock the CUDA lane, say so and I will act on it. |
Two strengthenings from @gaff-1's workspace scan, plus a correction to the precedent framing1. This is the only instance of the class in the workspace. @gaff-1 scanned every Worth noting this PR's own verification already covers the CUDA crates by compiler rather than by grep: the honesty script runs 2. The precedent is stronger than I claimed, and my description of it was imprecise. I cited So this is not a single precedent I found and matched — it is that crate's established convention, applied twice, and this PR brings the third 3. Correction, because it changes what the fix has to be. @gaff-1 characterised #[cfg(feature = "gpu-tests")] // :59
fn install_faults(allocator: &mut CudaVmmAllocator, plan: Arc<DriverFaultPlan>) {
allocator.install_driver_faults(plan); // :61
}
#[cfg(not(feature = "gpu-tests"))] // :64
fn install_faults(_allocator: &mut CudaVmmAllocator, _plan: Arc<DriverFaultPlan>) {
unreachable!("driver fault injection is only compiled under the gpu-tests feature");
}There is a second arm. It is not "gate the consumer" — it is precisely the two-arm shim this PR adds. And the second arm is load-bearing: This matters beyond pedantry, because "gate the consumer" is not an available fix in this file. Gating the His underlying diagnosis is right and is the crisp statement of the root cause: the difference between the two crates is that @gaff-1 has also retracted the grep-based falsifier he published on #1817 after his own negative control showed it produced 2 false positives out of 3 hits — grep sees the name, not whether the occurrence sits under an active |
Correction: I described the
|
65b9aa8 to
71a7221
Compare
…f which was reported The `CUDA compile (Linux x86_64)` lane has been red since #1836 (`6e4b0ebb3`); last green was `cb81745b0`. The reported compile error was masking two further failures, so fixing it alone would have moved the lane from "red at build" to "red at the honesty script". Two more arrived on `main` while this PR was open, in #1884 and #1895. All five are fixed here; the lane cannot go green on any proper subset. 1. Unresolved import (the reported error). `content_preserving_transition_gpu.rs` imported `transition_granule_range_with_phase8_faults` unconditionally, but that item is gated `#[cfg(any(test, feature = "gpu-tests"))]`. The `test` arm does not cover an integration test: `tests/*.rs` are separate crates linking the library built *without* `cfg(test)`, so the item is genuinely absent in the `without-gpu-tests` configuration. Invisible to any local run that passes `--features gpu-tests`. Fixed with a two-arm local shim, mirroring `with_faults` in `crates/onnx-runtime-cuda-memory/tests/virtual_memory_gpu.rs`, which already solves this exact problem for `CudaVirtualBacking::with_driver_faults`. The gate itself is left alone: widening it would ship a driver fault injector that can force `cuMemUnmap`/`cuMemMap`/`cuMemSetAccess` to fail into production, and `required-features` is precisely what the honesty script exists to forbid ("exists only with gpu-tests enabled; CUDA tests must not hide from CPU inventory"). #1895 (`1be9f2cc2`) reached this file first, with `#![cfg(feature = "gpu-tests")]` on the whole target -- the option rejected above. It fixed the compile and traded it for an inventory failure on the same lane: `main` at `1be9f2cc2` reports "content_preserving_transition_gpu: Cargo reported no integration tests" plus 19 x "test exists only with gpu-tests enabled" (job `97266415620`). The target was not repaired, it was hidden. That line is removed here and the module doc records why, so the option is not re-tried a third time. 2. Five non-ignored tests in a `_gpu` target (4 honesty violations). `verify_cuda_test_honesty.py` requires every test in a `_gpu` target to be ignored, not passed, on a CPU-only runner -- that is how the suite is stopped from reporting green for a GPU it never touched. Three were genuinely CPU-only predicate tests over `verify_safe_point`. Moved to a new non-`_gpu` target rather than `#[ignore]`d: silencing them would have greened the lane by deleting coverage, and target naming is the escape hatch the script's own comment names for CPU-only tests in a CUDA crate. Moving them alone would *also* have stopped them running. A non-`_gpu` target is skipped by the honesty script, and `workspace_test_packages.py` deny-lists this crate from every offline lane, so the target would have been compiled and never executed. The CUDA lane therefore gains an explicit `--test content_preserving_transition` step, alongside the two CPU-only targets in this crate that already have one for the same reason. The other two asserted nothing -- `fault_injection_safe_point_recheck_rejects` is comments plus a `println!` and self-describes as "a documentation test"; `zero_len_is_committed_noop` `println!`s that the behaviour is "verified by implementation". Both passed unconditionally. Removed. The first has real sibling coverage in `fault_injection_recheck_safe_point_rejected_gpu`; the second does not -- the `len == 0` early return has no test anywhere. Deleting a test that asserts nothing loses no coverage, but the gap is pre-existing and real, and a genuine test needs a device (the early return still takes `&CudaRuntime`/`&mut CudaReservation`), so it is left for a GPU-capable change rather than papered over. Added `safe_point_accepts_a_clean_state`, because the three moved tests only ever assert `is_err()` and so all three survive a mutant that makes `verify_safe_point` reject unconditionally. Verified: under that mutant the inherited three pass and only the new test fails. 3. The manifest guard checked a proxy, not the property it claimed. The check was the literal substring `"gpu-tests = []"`, so #1860 turned it red by changing the value to `["onnx-runtime-cuda-memory/gpu-tests"]` -- a legitimate and necessary forwarding. Now matches the feature *key* whatever its value, extracted into `declares_gpu_tests_feature` so it is covered by the script's own `--self-test` fixtures, which it previously was not. The key is looked for only inside the `[features]` table. Review falsified the first version of this fix: a file-wide regex also accepts a *dependency* named `gpu-tests`, or one under `[target.'cfg(...)'.dependencies]` -- the same proxy mistake in a new costume, inside the fix for that mistake. Twelve fixtures now, including both false-positive shapes. 4. A GPU test in a `_gpu` target with no `#[ignore]` (#1895). `causal_conv_with_state_gpu::the_standard_domain_spelling_reaches_the_same_kernel` calls `require_cuda()` exactly like its two siblings but is missing their `#[cfg_attr(not(feature = "gpu-tests"), ignore = ...)]`, so it ran and failed on a CPU runner. Fixed by adding the sibling attribute verbatim; no design choice involved. 5. A CPU-only test inside a `_gpu` target (#1884). `expert_route_telemetry_probe_gpu::cpu_oracle_and_validator_self_consistent` passes in both configurations, which the script reports as "executed without gpu-tests". Its doc comment states the intent plainly: it "runs without a GPU so the reference cannot silently rot". `#[ignore]` would clear the checker by destroying exactly that property -- an ignored test runs nowhere -- so this takes the same route as defect 2. The pure-CPU oracle (`cpu_bitmap`, `cpu_dedup`, `consume_and_validate`, `synth_routes` and the header indices) moves verbatim to a shared `tests/expert_route_oracle/mod.rs`, which is a module and not a target: Cargo auto-discovers `tests/*.rs` and `tests/*/main.rs` only. Both the `_gpu` target and a new `expert_route_telemetry_probe` target declare it, so there is one copy of the oracle and the seven GPU tests keep diffing against the same code the CPU test checks. The new target gets its own `ci.yml` step, for the reason in defect 2. Verified live in its new home by mutation rather than by its own green: perturb the shift in `cpu_bitmap` -> FAILED, revert -> ok. "It compiles in the new file" is not evidence that it executes there, which is the defect an earlier review caught in defect 2's target. Each fix independently falsified by re-introducing it: import -> E0432; un-ignored test -> "must be ignored, not pass" (+3 inventory errors); manifest value -> "must define a gpu-tests feature"; missing `cfg_attr` -> "1 tests failed while checking ignored status"; relocated CPU test -> "executed without gpu-tests; CUDA tests must be ignored, not pass". Honesty script exits 0: 530 tests/79 targets identical in both configurations, 530 ignored without gpu-tests, 0 passed on this no-CUDA host. `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` clean (the lane's exact command, both invocations of it); both explicit `ci.yml` test steps pass 4/4 and 1/1; fmt clean. Not fixed here: `cargo clippy -p onnx-runtime-ep-cuda --features cuda --all-targets -- -D warnings` reports 23 pre-existing errors and dies on `index_share_gpu` and `qmoe_zero_copy_cold_expert_spike_gpu` before reaching the rest, so 23 is a floor, not a total. None are in a file this PR touches -- no diagnostic location matches any of them. No lane runs that command: both ep-cuda clippy steps omit `--all-targets` and `workspace_test_packages.py` deny-lists the crate. A real gap, but a separate one; folding it in would put unrelated files in a lane-restoration PR. Closes #1875 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
71a7221 to
e602295
Compare
Scope expanded: three defects → five, and one of my own fixes was falsified in reviewTwo more defects landed on #1895 reached my file first, and took the option I had rejected
The target was not repaired, it was hidden. Its module doc argued a compile failure "is not a useful signal about a suite that cannot run on a machine with no GPU anyway" — which is the exact proposition No criticism of the author intended: this is a genuinely non-obvious gate, and the compile error is the visible half. Defect 4 —
|
| reverted | resulting failure |
|---|---|
| the shim | error[E0432]: unresolved import …with_phase8_faults |
| one un-ignored test | 1 tests executed without gpu-tests; must be ignored, not pass + 3 inventory errors |
| the manifest check | must define a gpu-tests feature |
the cfg_attr (4) |
1 tests failed while checking ignored status |
| the relocation (5) | 1 tests executed without gpu-tests; must be ignored, not pass |
Plus: cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings clean (both places ci.yml invokes it); both new CI steps run verbatim, 4/4 and 1/1; cargo fmt --all --check clean; 538 lib tests pass.
Correcting one number I had overstated in the other direction: the out-of-scope --all-targets clippy gap is 23 errors and dies on two targets before reaching the rest, so 23 is a floor, not a total. None are in a file this PR touches.
|
Merged as At merge time So the five defects above are fixed and A count, in a four-test target, from a CI job on a merge commit. Diagnosing it took a rebase and a grep. It now reads The pattern is the thing worth recording. Four instances of one omission in three days (#1884, #1895, #1905, and the original #1836 lineage) is not four careless authors — it is what happens when the only check that would object has been red for unrelated reasons since #1836. While that is true, adding a Two things I got wrong in the course of this PR, both caught by review rather than by me, both worth more than the fix:
Both are the same failure as the bug being fixed: something that looks like coverage and isn't. |
… test it means (#1911) `CUDA compile (Linux x86_64)` is **red on `main` again**, one commit after #1881 restored it. ``` CUDA test honesty check failed: - activations_gpu: 1 tests failed while checking ignored status - activations_gpu: Cargo inventory has 4 tests but libtest reported 3 ignored ``` #1905 added `activations_gpu::mish_matches_cpu_including_the_saturating_tail` without the `#[cfg_attr(not(feature = "gpu-tests"), ignore = …)]` that its **three siblings in the same file** carry, so it runs and fails on a CPU runner. Fix is the sibling attribute, verbatim. No criticism of @-the-author intended, and I want to be explicit about why. **This is the fourth instance of the same omission in three days** — #1884, #1895, and now #1905. That is a property of the *signal*, not of the authors: while the lane is red for an unrelated reason, adding a `_gpu` test without an ignore is free, because the only check that would object is a job nobody can distinguish from already-broken. #1881 restored the lane; this keeps it restored. --- ## The second half: the checker didn't say which test The message above names a **count**, in a four-test target, and the run that produced it was a CI job on a merge commit. Finding out which test needed a rebase and a grep. A check whose entire job is to notice that a `_gpu` test was added without an ignore should **say which one**. `run_libtest` now parses libtest's per-test outcome lines, so the same failure reads: ``` activations_gpu: 1 tests failed while checking ignored status (mish_matches_cpu_including_the_saturating_tail) ``` **Verified on the real path, not only in fixtures.** With the `cfg_attr` reverted, the full script emits exactly that line; restored, it exits 0. Fixtures alone would only have proved the formatter works. Naming also applies to `executed without gpu-tests` and `passed with gpu-tests on a no-CUDA host`, which have the same problem — those are the two messages that fired for #1884 and #1895. `IgnoredResult` and `ActiveResult` now share a `LibtestResult` base for it. It **degrades to the bare count** if the per-test lines can't be parsed, rather than rendering an empty `()`. The count is still true, so a future change in libtest's output format must not turn a real failure into a confusing one. Four new `--self-test` fixtures cover the naming, the inventory-mismatch message, the empty-name degradation, and the outcome regex itself — because a guard whose own correctness is unverified is precisely the defect this checker exists to catch, and I'd rather not add one to it while fixing it. ## Validation ``` CUDA test honesty check passed: 531 tests/79 targets without gpu-tests (531 ignored), 531 tests/79 targets with gpu-tests (475 fail-loud, 56 ignored, 0 passed on this no-CUDA host) ``` - run on this branch, based on current `main` (`7e274a4e2`) — not on an older tree - `--self-test` passes - `cargo fmt --all --check` clean - mutation-effective both ways: revert the `cfg_attr` → the named failure above; revert the naming → the bare count Follow-up to #1881. Refs #1875. 🤖 Generated with [Copilot CLI](https://githubnext.com/projects/copilot-cli) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…with a source scan The CUDA lane is red again on main. `celu_matches_cpu_including_nan_and_alpha_variants` (#1909) carries no ignore, so on a machine with no device it does not skip -- it runs. That is the fifth instance of the same defect in three days, after `content_preserving_transition_gpu`, `causal_conv_with_state_gpu`, `expert_route_telemetry_probe_gpu` and `activations_gpu::mish`. Five instances is a property of the signal, not of the authors. The authoritative check, verify_cuda_test_honesty.py, needs two full CUDA test builds and so runs only on the CUDA lane. An author therefore learns about the omission after merge, and only if that lane is green enough to be believed -- which it has not been. Every one of the five was reported by a lane already red for an unrelated reason, where one more red line is free. So this fixes the instance and adds `--source-scan`: the same rule read straight off the source in 75ms with no CUDA toolchain, wired into the `Rust quality` lane so it runs on every pull request. It supplements the inventory check rather than replacing it -- it cannot see macro-expanded tests the compiler sees, nor inventory parity between the two feature configurations. The first version of the scan was vacuous, and the fixtures record why. It searched the item's head for the substring "ignore", and Celu's doc comment says "a kernel that ignored the attribute entirely" -- so the scan passed on the exact defect it was written to catch. The fix is to read only attribute lines, never doc prose, and to anchor on the punctuation an attribute must have around it. Two of the fourteen fixtures exist solely to keep both defences falsifiable: relaxing the regex to a bare substring, or reading doc comments as attributes, each fails a distinct fixture. Evidence it detects what it claims: - positive control, per instance: restoring the pre-fix tree for Celu, for `mish` (3f81034^) and for `causal_conv_with_state_gpu` (e42fa94^) flags exactly one test each, by name and line, and nothing else. - negative control: passes on the fixed tree, all 79 CUDA targets. In particular it does not false-positive on `qmoe_gpu`, whose 10 macro-generated tests bake the attribute into the macro body, nor on `coarse_residency_plan_gpu` -- 2536 lines and 13 tests landed by #1854 while this branch was open, correctly clean. - self-test: 14 fixtures, mutation-checked in both directions. verify_cuda_test_honesty.py full run: 532 tests/79 targets without gpu-tests (532 ignored), 532 tests/79 targets with gpu-tests (476 fail-loud, 0 passed on this no-CUDA host). Refs #1875, #1909. Follows #1881 (e42fa94) and #1911 (3f81034). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…a target-level cfg (#1927) ## `coarse_residency_plan_gpu` contains no tests in the configuration CI builds `crates/onnx-runtime-ep-cuda/tests/coarse_residency_plan_gpu.rs:90` carries `#![cfg(feature = "gpu-tests")]`, so the whole target vanishes without the feature: ``` $ cargo test -p onnx-runtime-ep-cuda --features cuda --test coarse_residency_plan_gpu -- --list 0 tests, 0 benchmarks $ cargo test -p onnx-runtime-ep-cuda --features cuda,gpu-tests --test coarse_residency_plan_gpu -- --list 13 tests, 0 benchmarks ``` That is the inventory drift `verify_cuda_test_honesty.py` exists to catch, and it is currently red on `CUDA compile (Linux x86_64)`. ## The gate was load-bearing, not gratuitous It was the only thing making the file compile. `apply_residency_plan_at_boundary_with_phase8_faults` is gated `#[cfg(any(test, feature = "gpu-tests"))]`, and **the `test` arm does not reach an integration test** — `tests/*.rs` are separate crates linking the library built *without* `cfg(test)`, so with `gpu-tests` off the item does not exist and the top-level `use` naming it is `E0432`. Removing line 90 alone reproduces exactly that: ``` error[E0432]: unresolved import --> tests/coarse_residency_plan_gpu.rs:112 | 112| apply_residency_plan_at_boundary_with_phase8_faults, | ^^^ no `apply_residency_plan_at_boundary_with_phase8_faults` in `coarse_residency` note: found an item that was configured out --> src/coarse_residency.rs:411:8 409| #[cfg(any(test, feature = "gpu-tests"))] ``` Gating the target makes that compile, at the cost of the target silently containing nothing. **This is the sixth instance of the #1875 class in four days, and the second sub-class of it** — not a missing `#[ignore]`, but a test that is not there at all. Both sub-classes have the same cause: the lane that reports them has been red for unrelated reasons, so the signal cost nothing to ignore. ## The fix The shape this repo already uses for exactly this problem: **gate a helper, never the tests**, so every test stays in both inventories. Precedent, same repo, same shape — `crates/onnx-runtime-cuda-memory/tests/vmm_release_quarantine_gpu.rs`'s `install_faults`. A two-arm local shim keeps the name identical, so **the three call sites are unchanged**, and the production gate on the underlying entry point is untouched: fault injection is still compiled only under `gpu-tests`, and the other arm is `unreachable!`. **Ungating the library item was the alternative and is rejected.** It would put a fault-injection seam into production builds, and its documented mirror `transition_granule_range_with_phase8_faults` (`granule_transition.rs:246`) carries the same gate — ungating one of a documented pair would be inconsistent as well as wrong. ## Validation | check | result | |---|---| | inventory without `gpu-tests` | **13 tests** (was 0) | | inventory with `gpu-tests` | **13 tests** | | `verify_cuda_test_honesty.py` | every `coarse_residency_plan_gpu` error gone | | `clippy --features cuda --test coarse_residency_plan_gpu -- -D warnings` | clean | | `clippy --features cuda,gpu-tests ...` | clean | | `cargo fmt --all -- --check` | clean | **On local red, stated plainly:** the honesty script is not fully green on this branch. The only failures left are the two inherited `activations_gpu` / Celu errors from #1909 — this branch does not touch that file, and **#1920** fixes them. Verified by running the script here and confirming `coarse_residency_plan_gpu` no longer appears in the output at all. I will rebase and confirm a fully green run once #1920 lands, and will not merge before that. No test bodies, assertions or GPU semantics changed — the thirteen tests still carry their `#[ignore]` and still require a device. ## Follow-up, deliberately not in this PR The fast source scan added by #1920 is blind to this sub-class: it checks that every test carries an ignore, and a target with no tests trivially satisfies that. Detection for target-level `#![cfg(...)]` in a policed CUDA target belongs in a follow-up on top of #1920, so the guard and the green tree land together rather than a guard landing red. Refs #1875, #1854. Related: #1920, #1881 (`e42fa9470`), #1911 (`3f8103478`). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…with a source scan The CUDA lane is red again on main. `celu_matches_cpu_including_nan_and_alpha_variants` (#1909) carries no ignore, so on a machine with no device it does not skip -- it runs. That is the fifth instance of the same defect in three days, after `content_preserving_transition_gpu`, `causal_conv_with_state_gpu`, `expert_route_telemetry_probe_gpu` and `activations_gpu::mish`. Five instances is a property of the signal, not of the authors. The authoritative check, verify_cuda_test_honesty.py, needs two full CUDA test builds and so runs only on the CUDA lane. An author therefore learns about the omission after merge, and only if that lane is green enough to be believed -- which it has not been. Every one of the five was reported by a lane already red for an unrelated reason, where one more red line is free. So this fixes the instance and adds `--source-scan`: the same rule read straight off the source in 75ms with no CUDA toolchain, wired into the `Rust quality` lane so it runs on every pull request. It supplements the inventory check rather than replacing it -- it cannot see macro-expanded tests the compiler sees, nor inventory parity between the two feature configurations. The first version of the scan was vacuous, and the fixtures record why. It searched the item's head for the substring "ignore", and Celu's doc comment says "a kernel that ignored the attribute entirely" -- so the scan passed on the exact defect it was written to catch. The fix is to read only attribute lines, never doc prose, and to anchor on the punctuation an attribute must have around it. Two of the fourteen fixtures exist solely to keep both defences falsifiable: relaxing the regex to a bare substring, or reading doc comments as attributes, each fails a distinct fixture. Evidence it detects what it claims: - positive control, per instance: restoring the pre-fix tree for Celu, for `mish` (3f81034^) and for `causal_conv_with_state_gpu` (e42fa94^) flags exactly one test each, by name and line, and nothing else. - negative control: passes on the fixed tree, all 79 CUDA targets. In particular it does not false-positive on `qmoe_gpu`, whose 10 macro-generated tests bake the attribute into the macro body, nor on `coarse_residency_plan_gpu` -- 2536 lines and 13 tests landed by #1854 while this branch was open, correctly clean. - self-test: 14 fixtures, mutation-checked in both directions. verify_cuda_test_honesty.py full run: 532 tests/79 targets without gpu-tests (532 ignored), 532 tests/79 targets with gpu-tests (476 fail-loud, 0 passed on this no-CUDA host). Refs #1875, #1909. Follows #1881 (e42fa94) and #1911 (3f81034). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…with a source scan The CUDA lane is red again on main. `celu_matches_cpu_including_nan_and_alpha_variants` (#1909) carries no ignore, so on a machine with no device it does not skip -- it runs. That is the fifth instance of the same defect in three days, after `content_preserving_transition_gpu`, `causal_conv_with_state_gpu`, `expert_route_telemetry_probe_gpu` and `activations_gpu::mish`. Five instances is a property of the signal, not of the authors. The authoritative check, verify_cuda_test_honesty.py, needs two full CUDA test builds and so runs only on the CUDA lane. An author therefore learns about the omission after merge, and only if that lane is green enough to be believed -- which it has not been. Every one of the five was reported by a lane already red for an unrelated reason, where one more red line is free. So this fixes the instance and adds `--source-scan`: the same rule read straight off the source in 75ms with no CUDA toolchain, wired into the `Rust quality` lane so it runs on every pull request. It supplements the inventory check rather than replacing it -- it cannot see macro-expanded tests the compiler sees, nor inventory parity between the two feature configurations. The first version of the scan was vacuous, and the fixtures record why. It searched the item's head for the substring "ignore", and Celu's doc comment says "a kernel that ignored the attribute entirely" -- so the scan passed on the exact defect it was written to catch. The fix is to read only attribute lines, never doc prose, and to anchor on the punctuation an attribute must have around it. Two of the fourteen fixtures exist solely to keep both defences falsifiable: relaxing the regex to a bare substring, or reading doc comments as attributes, each fails a distinct fixture. Evidence it detects what it claims: - positive control, per instance: restoring the pre-fix tree for Celu, for `mish` (3f81034^) and for `causal_conv_with_state_gpu` (e42fa94^) flags exactly one test each, by name and line, and nothing else. - negative control: passes on the fixed tree, all 79 CUDA targets. In particular it does not false-positive on `qmoe_gpu`, whose 10 macro-generated tests bake the attribute into the macro body, nor on `coarse_residency_plan_gpu` -- 2536 lines and 13 tests landed by #1854 while this branch was open, correctly clean. - self-test: 14 fixtures, mutation-checked in both directions. verify_cuda_test_honesty.py full run: 532 tests/79 targets without gpu-tests (532 ignored), 532 tests/79 targets with gpu-tests (476 fail-loud, 0 passed on this no-CUDA host). Refs #1875, #1909. Follows #1881 (e42fa94) and #1911 (3f81034). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…with a source scan (#1920) ## What Two things, one cause. 1. **The fix:** `celu_matches_cpu_including_nan_and_alpha_variants` (landed in #1909, `d6e542509`) has no `#[cfg_attr(not(feature = "gpu-tests"), ignore = "...")]`, so it *runs* on the CUDA lane's no-GPU builder and reds it. 2. **The guard:** a `--source-scan` mode in `verify_cuda_test_honesty.py`, wired into the fast `rust-quality` lane, that catches this class **in ~75 ms** instead of after a 20-minute CUDA build. ## Why a guard and not just the fix This is the **sixth** instance of the class in four days, and the fifth of this exact sub-class: | # | test | landed | fixed by | |---|---|---|---| | 1-3 | `content_preserving_transition_gpu` (+2 more) | | #1881 `e42fa9470` | | 4 | `causal_conv_with_state_gpu` | | #1881 | | 5 | `activations_gpu::mish` | | #1911 `3f8103478` | | 6 | `activations_gpu::celu` | #1909 | **this PR** | The shared cause isn't carelessness. The authoritative check only runs on `CUDA compile (Linux x86_64)`, which takes ~20 minutes and has itself been red for unrelated reasons for much of this week — so an author gets no usable signal at the time they can act on it. Another one-off fix leaves instance seven to the same trap. The scan runs on a lane that is fast, always-on for any `.rs` change, and green. ## Scope of the guard, stated honestly The scan detects **exactly one** sub-class: a `#[test]` in a policed CUDA target with no effective ignore. It is **blind** to the target-level `#![cfg(feature = "gpu-tests")]` inventory-drift sub-class — which I found while validating this PR, and which is fixed separately in **#1927**. Extending the scan to that sub-class is deliberately deferred to a follow-up so a guard and a green tree land together. It reuses `is_cuda_test_target()` rather than re-deriving "policed target", so there remains one definition. ## Validation **Positive controls** (would a real defect be caught?) — reverting the ignore flags **exactly one** test, by correct name and line, in each case: - Celu, on this branch - `mish` at `3f8103478^` — a genuine historical instance - `causal_conv_with_state_gpu` at `e42fa9470^` — another **Negative controls** (does correct code pass?) — clean on all 79 CUDA targets in 75 ms, including `qmoe_gpu` (10 macro-generated tests) and `coarse_residency_plan_gpu` (2536 lines / 13 tests, written by someone else, landed mid-branch — a negative control I did not construct). **Combined with #1927**, the authoritative script is green: `554 tests / 81 targets` in both feature configurations, `exit 0`. Each branch alone is red only on what the other fixes. `--self-test`: **30 fixtures**. `cargo fmt --all -- --check` clean. ## Two rounds of independent Opus review, and what they found I reproduced every reported defect myself before fixing it, and mutation-tested every defense afterwards. **Round 1 — three real defects, one of them serious:** - **Fail-open (major):** bracket counting did not ignore string literals, so the upward attribute walk could escape an item and adopt the *previous* test's ignore — silently clearing a genuinely un-ignored test. - That same escape **resurrected the exact vacuity this PR exists to fix**: v1 of the scan searched the item head for the substring `"ignore"`, and Celu's doc comment reads "a kernel that **ignored** the attribute entirely". The guard passed on English prose. It failed its own positive control, which is the only reason I know. - `#[cfg_attr(not(feature = "something-else"), ignore)]` was accepted as a valid ignore. Fixed by rewriting the internals: length-preserving, column-exact masking of literals and comments; hard item boundaries; predicate validation by `fullmatch`. While fixing, I **found a fourth defect myself** — a false positive on `vmm_kv_layout_residency_gpu.rs:355`, caused by `.strip()`ing lines when building the masked and raw blobs, which broke the column alignment the span mapping depends on. My own fail-closed length guard caught it. **Round 2 — four residual holes:** - A blank line inside an attribute list broke the walk → **false positive on correct code**, which would red a green lane. Worse than a miss, in a guard whose whole value is being trusted. - The `cfg_attr` predicate was matched as a substring, so a nested `cfg_attr` could smuggle a different predicate through. - Non-canonical `#[test]` spellings (e.g. `#[ test ]`) were never inspected at all. - `pub(crate) fn` was not matched by the `pub\s` item boundary. All four fixed, each with a fixture. ## Mutation matrix Every defense is falsifiable — I broke each one and confirmed a **distinct** failure: | mutation | caught by | |---|---| | ungate the blank-line break | blank-line-in-attribute fixture | | match the predicate by substring | nested-`cfg_attr` fixture | | permit a nested `cfg_attr` | predicate fixture | | match only canonical `#[test]` | spacing fixture | | always walk downward | one-line `#[test] fn a() {}` fixture | | remove the `fn` item boundary | `pub(crate)` fixture | | relax `IGNORE_ATTR` to a substring | doc-prose fixture | | disable masking | literal-bracket fixture | One finding worth passing on: `FN_DECLARATION` and an enumerated `pub[\s(]` boundary turned out to be **redundant**, and the symptom was that *neither could be mutation-caught* — each was covered by the other. An unfalsifiable guard is dead weight regardless of whether it is correct. I removed the enumeration, kept the token anchor, and confirmed the fixture then fires. ## Risk The scan is additive and read-only. Its failure mode if wrong is a false positive on the fast lane — visible immediately, fixable in one line, and specifically fixture-guarded above. The Celu change is a single attribute. Closes part of #1875. --- ## Maintainer follow-up: rebased, fix half already landed, and one number corrected **The fix half is gone from this PR.** I hit the same Celu failure independently and — not having checked the open PR list first, which was my mistake — landed the one-line ignore separately as #1941. Rebasing this branch onto `main` dropped that hunk cleanly. What remains is **only the guard**, which was always the valuable half: ``` .github/scripts/verify_cuda_test_honesty.py | 456 +++++++++++++++++++++++++++- .github/workflows/ci.yml | 17 ++ ``` **Independently re-verified the guard on a Windows CUDA host**, since a guard I have not personally seen fire is not a guard I should merge: | Check | Result | |---|---| | Scan on current `main` | `CUDA test source scan passed: every test in a CUDA target carries an ignore`, exit 0 | | **Falsified** — Celu's `cfg_attr` removed again | exit 1, `activations_gpu.rs:336: celu_matches_cpu_including_nan_and_alpha_variants has no ignore attribute` — correct file, correct line, correct test, with the fix spelled out | **One number is wrong and I am correcting it rather than repeating it.** The body claims the scan is clean *"in 75 ms"*. On this Windows host the median of three steady-state runs is **~3.4 s wall** (3383 / 3546 / 4438 ms), of which ~0.6 s is interpreter startup alone. I could not reproduce 75 ms here; it may hold on a Linux runner with a warm cache, but as written it reads as a cross-platform property and is not one. This changes nothing about the argument. The comparison that matters is 3.4 s on an always-on fast lane against ~20 minutes on a CUDA lane that has been red for unrelated reasons much of this week — still three orders of magnitude, and still the difference between signal an author can act on and signal they cannot. But a number that is off by 160x reads as evidence, so it should not stand. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`CUDA compile (Linux x86_64)` is red on main at `d3bf2ebaa` in `capture_sync_contract`, which is a pin over the set of kernel functions that synchronize unconditionally. #1913 implemented `com.microsoft::MultiHeadAttention`, whose `execute` drains the trailing transpose before returning per-call scratch to the pool, and did not add it to the pin. I checked that the delta is accounted for rather than making the numbers match. The set differs by exactly one entry, added and none removed, and it comes from exactly one commit touching `src/kernels/` -- #1913. The pin's own comment states the admission rule: an entry must be a path whose `capture_support` is explicitly `Unsupported`. `MultiHeadAttentionKernel::capture_support` returns `CaptureSupport::unsupported("... allocates scratch and synchronizes")`, so the kernel is already excluded from graph capture and the sync is sound. It is the same shape as `packed_varlen_attention.rs::execute`, which is in the list for the same reason. So this is a pin that fell behind a deliberate change, not a regression in capture behaviour, and the fix is to admit the entry with the justification next to it. Worth noting how it went unseen: the CUDA lane has been red more or less continuously this week -- #1881, #1911, #1920, #1927 -- so a lane that was already failing absorbed a new failure without anyone learning anything from the colour. That is the argument for the pre-merge source scan in #1920 rather than for more diligence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ded (#1939) ## What `CUDA compile (Linux x86_64)` is red on `main` (reproduced at `d3bf2ebaa`, and on `origin/main` locally with no changes applied). The failing step is **Audit CUDA kernel capture-sync contract**, not the honesty script: ``` ---- unconditional_syncs_are_limited_to_capture_unsupported_paths ---- assertion `left == right` failed left has "multi_head_attention.rs::execute" right does not ``` #1913 implemented `com.microsoft::MultiHeadAttention`. Its `execute` drains the trailing transpose before returning per-call scratch to the allocator pool, which is an unconditional `synchronize()`. The pin that exists to notice exactly that was not updated. ## Why bumping the pin is the right fix here I checked that the delta is **accounted for** rather than just making the assertion pass — a pin you bump reflexively is worse than no pin. 1. **The delta is exactly one entry, added, with none removed.** Both sets are otherwise identical. 2. **It comes from exactly one commit.** `git log -- src/kernels/multi_head_attention.rs` is a single commit, #1913 (`027e0cfb8`). 3. **It satisfies the pin's own stated admission rule.** The comment above `expected` reads: *"Every entry is a path whose `capture_support` is explicitly `Unsupported`, or a dynamically-admitted kernel's fallback path that `capture_support` rejects."* `MultiHeadAttentionKernel::capture_support` returns: ```rust CaptureSupport::unsupported( "cuda_ep MultiHeadAttention uses the per-call Phase-2a workspace path (allocates scratch and synchronizes)", ) ``` So the kernel is already excluded from graph capture; the sync cannot be captured because the path cannot be. 4. **It is structurally the same as an entry already in the list.** `packed_varlen_attention.rs::execute` is admitted for the identical reason — trailing stream synchronize on a path declared `unsupported`. This is not a new category. So: a pin that fell behind a deliberate change, not a regression in capture behaviour. The entry carries its justification inline so the next reader doesn't have to re-derive it. ## Verification - **Positive control, already observed:** the pin *did* fire on a real behavioural change. That is the test working, not failing. - After this change: `test result: ok. 1 passed; 0 failed`. - Reproduced the failure on `origin/main` with zero local modifications first, to establish it was not mine. - `cargo fmt --all -- --check` clean. ## How it went unseen, which is the part worth fixing The CUDA lane has been red more or less continuously this week — #1881, #1911, #1920, #1927 — so a lane that was **already failing** absorbed a new failure without anyone learning anything from the colour. Resch hit the identical shape with the `op_rules` count pin (#1873), and Gaff with a grep-based falsifier that couldn't distinguish the case it claimed to detect. That is an argument for the pre-merge source scan in #1920 rather than for more diligence: a check nobody can read the output of is not a check. Unblocks #1920. Part of #1875. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… the worse half The testing page said the CUDA honesty check runs on "CUDA CI". That has been stale since #1920 put --self-test/--source-scan on the rust-quality lane, and the staleness is not incidental: believing the check only runs on a 20-minute lane is exactly what let this class recur across four successive fixes (#1881, #1911, #1920, #1927) under #1875. An author who thinks the signal arrives after merge does not look for it before. Records both lanes and their division of labour -- the CUDA lane is authoritative because it builds the test targets twice and compares inventories, the fast lane is the same rule read off the source in about a second and cannot see macro-expanded tests or inventory parity -- and the two hard rules for CUDA targets: 1. every #[test] needs cfg_attr(not(feature = "gpu-tests"), ignore = ...) 2. never cfg a test, its mod, or the whole target on gpu-tests Rule 2 is the dangerous one and was undocumented. It does not red a lane; it deletes the tests from the inventory, so the lane is green because the tests are not there (#1927 removed thirteen that way). The fix is a two-arm shim around the helper, quoted verbatim from crates/onnx-runtime-cuda-memory/tests/vmm_release_quarantine_gpu.rs rather than invented, so the example compiles by construction. Also records the trap that produces rule-2 violations in the first place: #[cfg(any(test, feature = "gpu-tests"))] does not cover integration tests, because tests/*.rs are separate crates linking the library built without cfg(test). Reaching such an item from a top-level use is E0432, and gating the whole target is the plausible-looking "fix" that hides the tests instead. Docs-only, so this PR sets docs_only=true and rust-quality is skipped: CI green here is not evidence for any claim above. Verified locally instead -- the lane assertions against ci.yml (L367, L370, L1011), and the shim example by exact string match against the source file. Refs #1875 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… the worse half (#1959) The testing wiki said the CUDA test-honesty check runs on "CUDA CI". That has been stale since #1920 put `--self-test` / `--source-scan` on the `rust-quality` lane — and the staleness is the interesting part, because *believing the check only runs on the slow lane is the belief that let this class recur*. Under #1875 the same defect class needed four successive fixes: #1881, #1911, #1920, #1927. The authoritative check is real and correct, but it lives on a ~20-minute lane that was red continuously for other reasons, so an author got no signal at a point where they could still act on it. A check nobody reads in time is not a check. ## What the section now says **Both lanes, and the division of labour.** The CUDA lane is authoritative — it builds the CUDA test targets twice (with and without `gpu-tests`) and requires identical target sets *and* identical per-target test-name sets. The fast lane is the same rule read off the source in ~1 s with no CUDA toolchain. It **supplements and does not replace**: it cannot see macro-expanded tests, nor inventory parity between feature configs. **Which targets are policed**, quoted from `is_cuda_test_target()` rather than paraphrased: stem ends `_gpu` **or** is in `CUDA_TARGETS_WITHOUT_SUFFIX`, **and** is not in `ALWAYS_RUN`. **The two hard rules**, which were not written down anywhere: 1. Every `#[test]` in a policed target needs `#[cfg_attr(not(feature = "gpu-tests"), ignore = "...")]`, or it really runs on a machine with no GPU — *except* `ALWAYS_RUN` (`suite_canary_gpu`), which is deliberately un-ignored because it exists to run on deviceless machines. 2. Never `cfg` a test, its `mod`, or the whole target on `gpu-tests` (nor use manifest `required-features`). Rule 2 is the one worth documenting, and it is the more dangerous of the two. Rule 1 reds a lane loudly. Rule 2 **deletes the tests from the inventory** — the lane is green *because the tests are not there*. #1854 lost thirteen tests exactly that way, until #1927 put them back. **The trap that produces rule-2 violations.** `#[cfg(any(test, feature = "gpu-tests"))]` does **not** cover integration tests: `tests/*.rs` are separate crates linking the library built *without* `cfg(test)`, so the `test` arm never applies. Naming such an item from a top-level `use` is `E0432` — and gating the entire target is the plausible-looking "fix" that silently hides the tests instead. Documenting the trap next to the rule matters more than documenting the rule, since the trap is what makes the wrong fix look right. **The correct shape** is a two-arm shim around the *helper* — same name, same signature in both arms — leaving the test and its call sites byte-identical across configs. The example is quoted **verbatim** from `crates/onnx-runtime-cuda-memory/tests/vmm_release_quarantine_gpu.rs` rather than invented, so it compiles by construction and cannot drift into being wrong on its own. ## Independent review No blocking defects, but it falsified three claims, all in the direction of stating more than the evidence supports — which is the exact failure mode this page is about, so they are fixed in `248cceba2`: 1. **The policed set was actionably wrong, not merely simplified.** I wrote "files ending in `_gpu`". That both omits `matmul_nbits_marlin_numerics` (policed, no suffix) and wrongly includes `suite_canary_gpu`, which has two `#[test]` and zero `cfg_attr` *by design*. A reader applying rule 1 to the canary would have removed it from precisely the deviceless runs it exists to police. The worst kind of doc bug: confidently instructing a change that breaks a guard. 2. **"The disabled arm is never executed" was over-claimed.** `cargo test -- --ignored` with `gpu-tests` off reaches it. That is what `unreachable!()` is for — the code was right, the sentence was not. 3. **The `#1927` attribution was backwards.** #1854 (`26974e5fd`) introduced the target gate that deleted the thirteen tests; #1927 (`be788b924`) removed it and restored them. Confirmed with `git log -S` on the exact gate string. A fourth finding — that my commit message's `ci.yml` line numbers were off by seven — I checked and did **not** accept as stated: the reviewer read `ci.yml` from a different branch of mine whose comment edit shifted those lines. `L367`/`L370` were correct for this branch's base. The point survives in a better form, though: the honesty-script step has independently moved `1011` → `1021` on main since I wrote it, so I dropped the line numbers rather than correcting them. The rendered page cites none. ## Verification, and its limits **This is a docs-only change, so `changes` sets `docs_only=true` and `rust-quality` is skipped** — along with every other Rust job, each of which carries `if: needs.changes.outputs.docs_only != 'true'`. Green CI on this PR is therefore *not* evidence for anything asserted above. Saying so explicitly because the alternative — letting a green tick imply coverage it did not get — is the same failure mode this page documents. Verified locally instead: - Both lane claims checked against `.github/workflows/ci.yml`: `--self-test` and `--source-scan` in `rust-quality`, the bare script on the CUDA lane. - `wiki/*` confirmed a docs path in `is_docs_path()`, hence the skip above. - `ALWAYS_RUN` and `CUDA_TARGETS_WITHOUT_SUFFIX` read out of the script, not recalled. - `suite_canary_gpu.rs` confirmed at 2 `#[test]` / 0 `cfg_attr`, which is what makes finding 1 load-bearing. - The shim example checked by **exact string containment** against the source file, not by eye — before and after the edits. My first attempt at this check compared the wrong line span and reported a spurious `1d0`/`9a9`; the framing was off, not the code. - Markdown fences balanced (20, even); no docs/markdown lint gate exists in `ci.yml` to satisfy. Refs #1875 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
490b846 removed .commitmsg as an accidentally tracked PR artifact -- as a .commitmsg/ directory. #1881 (e42fa94) re-committed it as a file. That commit is mine. The prior cleanup deleted the artifact but did not add the name to .gitignore, so nothing stopped the next one. The block at .gitignore:63 already exists for exactly this class -- .msg.txt, .pr-body.md, .git-msg.txt, CM*.txt, PR*.md -- and its own comment notes that "a stray commit-message file has reached a PR before". .commitmsg matches none of those patterns, which is the same failure the /*.log entry a few lines below records from #1951: the file was named outside every pattern and 'git add -A' does not care what a file is for. The file is referenced by nothing (grepped repo-wide, excluding .git and target/). Removing it changes no behaviour; the .gitignore line is what stops the recurrence. Anchored as /.commitmsg, following the /*.log precedent in the same block: the artifact is only ever created where you run 'git commit -F', so a nested file of that name belongs to somebody and should not be silently swallowed. Verified both directions. Accept: a root .commitmsg present on disk is left unstaged by 'git add -A', in both the file form and the original .commitmsg/ directory form, attributed to .gitignore:76. Reject: a nested crates/*/.commitmsg is NOT ignored, and no currently tracked file is shadowed by the rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
490b846 removed .commitmsg as an accidentally tracked PR artifact -- as a .commitmsg/ directory. #1881 (e42fa94) re-committed it as a file. That commit is mine. The prior cleanup deleted the artifact but did not add the name to .gitignore, so nothing stopped the next one. The block at .gitignore:63 already exists for exactly this class -- .msg.txt, .pr-body.md, .git-msg.txt, CM*.txt, PR*.md -- and its own comment notes that "a stray commit-message file has reached a PR before". .commitmsg matches none of those patterns, which is the same failure the /*.log entry a few lines below records from #1951: the file was named outside every pattern and 'git add -A' does not care what a file is for. The file is referenced by nothing (grepped repo-wide, excluding .git and target/). Removing it changes no behaviour; the .gitignore line is what stops the recurrence. Anchored as /.commitmsg, following the /*.log precedent in the same block: the artifact is only ever created where you run 'git commit -F', so a nested file of that name belongs to somebody and should not be silently swallowed. Verified both directions. Accept: a root .commitmsg present on disk is left unstaged by 'git add -A', in both the file form and the original .commitmsg/ directory form, attributed to .gitignore:76. Reject: a nested crates/*/.commitmsg is NOT ignored, and no currently tracked file is shadowed by the rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
490b846 removed .commitmsg as an accidentally tracked PR artifact -- as a .commitmsg/ directory. #1881 (e42fa94) re-committed it as a file. That commit is mine. The prior cleanup deleted the artifact but did not add the name to .gitignore, so nothing stopped the next one. The block at .gitignore:63 already exists for exactly this class -- .msg.txt, .pr-body.md, .git-msg.txt, CM*.txt, PR*.md -- and its own comment notes that "a stray commit-message file has reached a PR before". .commitmsg matches none of those patterns, which is the same failure the /*.log entry a few lines below records from #1951: the file was named outside every pattern and 'git add -A' does not care what a file is for. The file is referenced by nothing (grepped repo-wide, excluding .git and target/). Removing it changes no behaviour; the .gitignore line is what stops the recurrence. Anchored as /.commitmsg, following the /*.log precedent in the same block: the artifact is only ever created where you run 'git commit -F', so a nested file of that name belongs to somebody and should not be silently swallowed. Verified both directions. Accept: a root .commitmsg present on disk is left unstaged by 'git add -A', in both the file form and the original .commitmsg/ directory form, attributed to .gitignore:76. Reject: a nested crates/*/.commitmsg is NOT ignored, and no currently tracked file is shadowed by the rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`490b846c3` ("Remove accidentally tracked PR artifacts") removed
`.commitmsg`. **#1881 (`e42fa9470`) re-committed it.** That commit is
mine, so this is my cleanup.
## Why it came back
The prior cleanup deleted the artifact but did not add the name to
`.gitignore`. The block at `.gitignore:63` exists for precisely this
class:
```
# Scratch files used to pass long commit messages to git commit -F.
.msg.txt
.pr-body.md
.git-msg.txt
# Belt and braces: the convention above is easy to forget, and a stray commit-
# message file has reached a PR before.
CM*.txt
PR*.md
```
`.commitmsg` matches **none** of them. That is the same failure the
`.log` entry a few lines below records from #1951 — *"the file was named
outside every pattern above, and `git add -A` does not care what a file
is"*. Deleting an artifact removes the instance; only the pattern
removes the class. This one recurred within a day of being cleaned up,
which is the evidence that instance-removal is not a fix.
## What changed
- `git rm .commitmsg` (141 lines, my #1881 commit message)
- one entry + comment in the existing commit-message block
Referenced by nothing — grepped repo-wide excluding `.git` and
`target/`. Removing it changes no behaviour; **the `.gitignore` line is
the actual fix.**
## Verification, both directions
Per the rule from #1817/#1916 — run the guard against the case it must
reject *and* the case it must accept.
| arm | check | result |
|---|---|---|
| accept | `.commitmsg` present on disk, `git add -A` | index shows only
the deletion — not re-added |
| accept | original `.commitmsg/` **directory** form (the `490b846c3`
shape) | ignored, same rule |
| attribution | `git check-ignore -v` | `.gitignore:74:.commitmsg` — the
new line, not an incidental match |
| reject | `.commitmsg-notes.md`, `commitmsg.rs` | still **not** ignored
— pattern is not over-broad |
The directory arm matters because the artifact has appeared in both
shapes; a pattern that only caught the file form would have missed the
original.
### One correction on my own method
My first accept-arm check was `git status --porcelain | grep -q
commitmsg`, which reported failure. The grep was matching `D .commitmsg`
— the staged deletion — not an untracked file. Same shape as the `grep`
error I published on #1891: **the instrument resolved against my own
change rather than against the subject.** The sound test is behavioural
(`git add -A`, then read the index), not a status-string match.
Recording it because the wrong version looked conclusive and reported
the alarming direction.
## Scope
Root-level tracked files audited; after this removal every remaining one
is legitimate. `roy_validate.sh` is a deliberately committed tool —
`.gitignore` ignores *its logs* — and is untouched. No other strays. No
Rust files touched.
Draft pending independent review.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## What `.body.md` — the PR body for #2026 — is tracked on `main` right now. This removes it and adds a `Diff guard` job that makes the root an enumerated set rather than a pattern-matching problem. ## Why not another .gitignore pattern It is the **fourth** file of this class, and it landed *after* the two most recent fixes for it: | file | arrived | removed by | |---|---|---| | `.commitmsg/` | — | `490b846c3` | | `.commitmsg` | #1881 | #1999 | | `.pris_v4.log` (1364-line cargo transcript) | #1951 | #1975 | | **`.body.md`** | **#2026, today** | **this PR** | Every repair added one more pattern. The block's own comments predict the failure twice — *"the name matching none of the patterns above and nothing stopped the next one"* and *"the file was named outside every pattern above"* — and then it happened again, hours later, to a file whose intended name (`.pr-body.md`) **is** already listed. Someone typed a shorter one. Three non-converging iterations is enough evidence. A pattern list has to predict the next filename; an allowlist does not. ## The check Asserts the repository root is exactly `.github/root-file-allowlist.txt`. It lives beside `deletion-ratio` because it guards the same blind spot from the other end: that job catches a change too large for anyone to read, this one catches a single added line invisible in a diff of hundreds. Both are cases where *the shape of the change*, not its content, is the signal. Adding a legitimate root file = add a line to the list in the same PR. That is deliberately not a label: a label is ephemeral, a line in a tracked file is a permanent record of the decision, which is the property the existing `.gitignore` comments were reaching for. ## Validation Six arms, driving the script **extracted from the workflow YAML** rather than a copy — a seam that reimplements the check passes whether or not CI runs the same thing (per #1897): | arm | expect | result | |---|---|---| | clean root | accept | `rc=0`, "Root is exactly the 16 allowlisted file(s)" | | stray root file (`.body.md`) | reject | `rc=1`, names the file | | **nested** `crates/…/.body.md` | accept | `rc=0` — not over-broad | | stale allowlist entry | reject | `rc=1` — the list cannot rot into a rubber stamp | | allowlist file **missing** | reject | `rc=1` — fails closed, no vacuous pass | | new root file, allowlisted in-PR | accept | `rc=0` — escape hatch works | Two of those are the ones I would have skipped if I were not being careful. **Missing-allowlist** matters because the natural implementation returns success when it cannot find its own input — the guard would go permanently green the moment someone moved the file. **Stale-entry** matters because an entry that outlives its file silently pre-approves that exact filename, so a rotting list is worse than none. `.gitignore` also gets `/.body.md`, verified both directions (root ignored, nested not, shadows zero tracked files). That line is explicitly the *trailing* half of the repair — it is the layer that has failed four times, kept because it stops the local `git add -A`, not because it is the fix. ## Not claimed This does not stop a stray file in a **subdirectory**; the root is where the evidence is (4/4), and a repo-wide version would need a scratch-file heuristic, which is the guessing game this PR is trying to end. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…2052) Follow-up to #2036, merged earlier today. **The guard it shipped cannot see a stray *directory* at the root — and two of the seven historical instances are directories, one of them cited in #2036's own header.** ## The gap #2036 compares tracked files whose path contains no separator: ```bash git ls-files | grep -v '/' ``` A stray directory has no such path. `.commitmsg/m.txt` contains a separator, so it is discarded as "not at the root" — and the thing that *is* at the root, `.commitmsg`, never appears in `ls-files` output at all. The guard doesn't fail to complain; it affirmatively reports a clean root. Not hypothetical: `.commitmsg/` reached `main` as `faedea4d1`, removed three minutes later by `490b846c3` *"Remove accidentally tracked PR artifacts"*. #2036's header lists it, with the trailing slash. **I enumerated the instance and then validated against six arms that all used a file.** Naming an instance is not testing its shape. ## The fix Compare **first path segments** — a root file contributes itself, a nested file contributes its top-level directory: ```bash git -c core.quotePath=false ls-files | sed 's#/.*##' | LC_ALL=C sort -u ``` The allowlist gains the 20 tracked root directories (36 entries), and unlisted entries are labelled `(directory)` or `(symlink)` where they are one, because the remedy differs. ## The inventory, corrected by review I claimed six instances "found by walking every root path ever added, not by collecting what people reported". **The walk had the same blind spot as the guard** — it filtered to paths without a separator, so it could not see a stray directory either. Redone on first segments: | entry | added → removed | on `main` | |---|---|---| | `.msg.txt` | `39675330b` → `bbc193117` | 1.6h | | **`.commitmsg/`** | `faedea4d1` → `490b846c3` | 3m | | **`.goldens/`** | `faedea4d1` → `490b846c3` | 3m | | `.wa64.log` | `83a51bfa6` → `c07acaa78` | 17.2h | | `.commitmsg` | `e42fa9470` (#1881) → `398cff8e5` (#1999) | 19.8h | | `.pris_v4.log` | `589d48ffd` (#1951) → `7a6482c83` (#1975) | 1.3h | | `.body.md` | `79196f89d` (#2026) → `54625db9d` (#2036) | 2.7h | **Seven entries, six incidents** — `.commitmsg/` and `.goldens/` arrived and left together. `.goldens/` is the most on-point instance available (a directory removed as a "PR artifact") and my method could not see it. Excluded, with reasons recorded in the file rather than silently: `site` (moved to onnx-genai-wiki, #1488), `third_party` (oneDNN removal), and `abresults` — 131h on `main`, the longest of any, but added by a `docs(benchmarks): record the … result` commit that says it is recording a result. Intentional-when-added is the line; `.wa64.log` rode in on a `test(cpu):` commit that never mentions it. `.wa64.log` still matters beyond the count: it predates `.pris_v4.log` by two days, so `/*.log` in #1975 was reactive to the *second* log incident. No authorship attributed — squash-merge rewrites `%an` to the merging account, so it reads identically for all seven and says nothing about who staged the file. ## Validation — 15/15 Driving the script **extracted from the workflow YAML**, never a copy. | arm | rc | |---|---| | clean root | 0 — `Root is exactly the 36 allowlisted entr(ies).` | | **stray root directory → new guard** | **1**, labelled `(directory)` | | **same stray → #2036 guard + #2036 allowlist** | **0** — `Root is exactly the 16 allowlisted file(s).` | | `.goldens/`, the second directory instance | 1 | | duplicate allowlist entry → not reported stale | 0, warning names it | | root symlink | 1, labelled `(symlink)` | | stray root file / stale entry / comments-only / missing list | 1 / 1 / 1 / 1 | | nested file under an allowlisted dir | 0 | | non-ASCII root file, allowlisted | 0 | | new root directory allowlisted in-PR / not | 0 / 1 | | CRLF allowlist | 0 | Row 3 is the control and must pair **both** of #2036's halves. My first attempt paired the old guard with the *new* allowlist: it returned 1 and looked like coverage, but the 1 came from 20 directory entries reading as stale. ## Two instrument bugs in my own battery 1. **Wrong control**, above — a control that changes two things measures neither. 2. **The staging check had the defect the guard was fixed for.** Each arm asserts its input reached the index before believing the output, but that check used bare `git diff --cached --name-only`, which renders non-ASCII as `"caf\303\251.txt"` while the guard uses `core.quotePath=false`. It reported the non-ASCII arm VACUOUS against a setup that had worked. Opus caught exactly this in #2036's guard; it reappeared in the thing measuring the guard. ## Also fixed, from review - An entry listed twice left one copy unpaired in `comm` and was reported as "not present at the root" for a name that is. Deduped both sides; duplicates now raise a `::warning::` naming them. - `[ -d ]` follows symlinks, so a root symlink to a directory was labelled a directory and advised `git rm -r --cached`. `-L` tested first. - Recorded the cost of first-segment comparison: the guard sees **root children only**. Scratch under an already-blessed directory is invisible to it. ## Scope CI-config only — `.github/workflows/diff-guard.yml` and `.github/root-file-allowlist.txt`. No Rust, no runtime behaviour, no test changes. Adding a root entry, file or directory, means adding it to the allowlist in the same PR; the error message says so. --------- Co-authored-by: holden <holden@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #1875.
CUDA compile (Linux x86_64)has been red since #1836 (6e4b0ebb3); last green wascb81745b0. Resch handed this off explicitly rather than guessing at another domain's intended API visibility — that was the right call, because the reported compile error was masking two further failures on the same lane. Fixing only the error would have moved it from "red at build" to "red at the honesty script".Two more arrived on
mainwhile this PR was open — #1884 and #1895, both today. Five independent defects from five different PRs, reviewable separately. The lane cannot go green on any proper subset of them.1. Unresolved import — the reported error
content_preserving_transition_gpu.rsimportstransition_granule_range_with_phase8_faults, gated#[cfg(any(test, feature = "gpu-tests"))]. Thetestarm does not cover an integration test:tests/*.rsare separate crates linking the library built withoutcfg(test), so in thewithout-gpu-testsconfiguration the item is genuinely absent. Invisible to any local run passing--features gpu-tests.I rejected both options in the issue, with evidence:
cuMemUnmap/cuMemMap/cuMemSetAccessto fail to every consumer.required-featuresis exactly what the honesty lane exists to forbid:compare_inventoriesemits "exists only with gpu-tests enabled; CUDA tests must not hide from CPU inventory". It trades a compile error for a lane failure and deletes the CPU-side inventory of 14 tests.Fix: a two-arm local shim, mirroring
with_faultsincrates/onnx-runtime-cuda-memory/tests/virtual_memory_gpu.rs, which already solves this identical problem forCudaVirtualBacking::with_driver_faults. The gate stays intact; the target compiles and lists its tests in both configurations. Theunreachable!arm is genuinely unreachable — after change 2 every test in that file is#[ignore]d.2. Five non-ignored tests in a
_gputarget — 4 honesty violationsThe script requires every test in a
_gputarget to be ignored, not passed on a CPU-only runner. That rule is how the suite is stopped from reporting green for a GPU it never touched.Three were genuinely CPU-only predicate tests over
verify_safe_point— pure logic overResizeSafePoint, no driver. Moved to a new non-_gputarget, not#[ignore]d: silencing them would green the lane by deleting coverage, and target naming is the escape hatch the script's own comment names, so that "a genuinely CPU-only target is not policed as a device test merely because it lives in a CUDA crate".Two asserted nothing and are removed:
fault_injection_safe_point_recheck_rejects— comments plus aprintln!; self-describes as "this test documents the contract".zero_len_is_committed_noop—println!s that the behaviour is "verified by implementation".Both passed unconditionally. This touches another author's tests, so I flag it explicitly rather than burying it in the diff. Deleting a test that asserts nothing loses no coverage — but to be precise about what is and isn't covered: the first does have real sibling coverage in
fault_injection_recheck_safe_point_rejected_gpu, and the second does not — thelen == 0early return is untested anywhere. That gap is pre-existing, and a genuine test needs a device (the early return still takes&CudaRuntime/&mut CudaReservation), so I have left it for a GPU-capable change rather than claim coverage that doesn't exist.Added
safe_point_accepts_a_clean_state. The three moved tests only ever assertis_err(), so they cannot distinguish "rejects the unsafe field" from "rejects everything". Empirically confirmed — withverify_safe_pointmutated to reject unconditionally:All three inherited tests survive the mutant. Only the new one kills it.
3. The manifest guard checked a proxy, not the property it claimed
A literal substring match. #1860 turned this red by changing the value to
["onnx-runtime-cuda-memory/gpu-tests"]— a legitimate and necessary forwarding. The guard's message claims to check that the feature is defined; it actually checked that it was defined with one particular empty value.Now matches the feature key whatever its value, extracted into
declares_gpu_tests_feature()so it is covered by the script's own--self-testfixtures — which it previously was not. Fixtures include the forwarding form, a multi-line list, a commented-out line, and afeatures = ["gpu-tests"]dependency mention that must not count.4. A GPU test in a
_gputarget with no#[ignore]— #1895causal_conv_with_state_gpu::the_standard_domain_spelling_reaches_the_same_kernelcallsrequire_cuda()exactly like its two siblings in the same file, but is missing their#[cfg_attr(not(feature = "gpu-tests"), ignore = …)]. So it ran on a CPU runner and failed:Fix: add the sibling attribute verbatim. No design choice involved — a four-line omission.
5. A CPU-only test inside a
_gputarget — #1884expert_route_telemetry_probe_gpu::cpu_oracle_and_validator_self_consistentpasses in both configurations, which the script reports asexecuted without gpu-tests. Its doc comment states the intent plainly:That intent is correct and worth keeping.
#[ignore]would clear the checker by destroying exactly the property the test was written for — an ignored test runs nowhere, so the reference could then rot precisely as its author feared, with a green check over it. Same reasoning as defect 2, so the same route.Fix: the pure-CPU oracle —
cpu_bitmap,cpu_dedup,consume_and_validate,synth_routes, theDecisionenum and the header indices — moves verbatim to a sharedtests/expert_route_oracle/mod.rs, and the test moves to a newexpert_route_telemetry_probetarget that declares it.tests/expert_route_oracle/mod.rsis a module, not a target: Cargo auto-discoverstests/*.rsandtests/*/main.rsonly. Both targets declare it, so there is one copy of the oracle and the seven GPU tests keep diffing against the same code the CPU test checks — the alternative, duplicating it, would let the two copies drift and quietly defeat the point.The new target gets its own
ci.ymlstep, for the reason in defect 2's correction.Verified live in its new home by mutation, not by its own green. Perturbing the shift in
cpu_bitmap:and
okagain on revert. "It compiles in the new file" is not evidence that it executes there — which is the defect the review caught in defect 2, so this time it is checked rather than asserted.Validation
Every fix independently falsified by re-introducing it — each re-reds with a distinct error:
error[E0432]: unresolved import ...with_phase8_faults1 tests executed without gpu-tests; CUDA tests must be ignored, not pass+ 3 inventory errorsmust define a gpu-tests featurecfg_attr(4)1 tests failed while checking ignored status1 tests executed without gpu-tests; CUDA tests must be ignored, not passcargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings— clean (the lane's exact command, and both of the two placesci.ymlinvokes it)cargo test -p onnx-runtime-ep-cuda --features cuda --lib— 538 passed, 0 failedcontent_preserving_transition4 passed,expert_route_telemetry_probe1 passedcargo fmt --all --check— clean496597764; honesty script re-run green on the exact tree being shipped (not on an earlier one — main moved four times during this work).One observation I am not fixing here. The lane's clippy step omits
--all-targets, so this crate's test code is linted by no lane —workspace_test_packages.pyalso deny-lists it. Running it now yields 23 errors and dies onindex_share_gpuandqmoe_zero_copy_cold_expert_spike_gpubefore reaching the rest, all pre-existing and none in a file this PR touches (verified: no diagnostic location matches any of them). Unrelated and out of scope — folding it in would put a pile of unrelated files into a lane-restoration PR — but it is the same absent-coverage shape that let defects 2, 4 and 5 sit un-noticed.Credit
@Gaff established that this is the only instance of the class in the workspace — 24
cfg(any(test, feature = …))sites, the other 23 either unreferenced fromtests/or referenced only from gated positions — and stated the root cause more crisply than I had: an item so gated is broken only when atests/file names it from a position not itself gated, in practice a top-leveluse, because auseresolves unconditionally regardless of how the functions below it are gated. A gated call site is safe. He also retracted his own grep-based falsifier for the class on #1817 after running the negative control on it.One correction to his note, since it changes what the precedent demonstrates:
onnx-runtime-cuda-memorydoes not gate the consumers.vmm_release_quarantine_gpu.rs:59-67is a two-arm shim — a#[cfg(feature = "gpu-tests")]helper and a#[cfg(not(feature = "gpu-tests"))]unreachable!twin — with its callers ungated. That distinction is load-bearing here: genuinely gating the consumers would delete the tests from the base inventory and red this lane undercompare_inventories. So the crate is still the right worked example; it is an example of the shim, which is what this PR adopts (making it the third in the repo, aftervirtual_memory_gpu.rs::with_faultsandvmm_release_quarantine_gpu.rs::install_faults).🤖 Generated with Copilot CLI